Conversation
There was a problem hiding this comment.
What Changed
This PR introduces a new DELETE /recordings/{recorder_id} endpoint to allow for the deletion of recordings, addressing concerns about unbounded disk space usage. The implementation adds a Delete method to the Recorder interface and handles the file removal and status update asynchronously in a new goroutine within the DeleteRecording handler. The FFmpegRecorder is updated to track a deleted state to prevent further operations on deleted recordings.
Risks / Concerns
The primary concern is the use of the request's context within the asynchronous deletion goroutine in server/cmd/api/api/api.go. If the client cancels the request after the HTTP response is sent, the context will be cancelled, which could interrupt the file deletion process. It's recommended to use context.WithoutCancel(ctx) for this background task to ensure it runs to completion.
6 files reviewed | 1 comments | Review on Mesa | Edit Reviewer Settings
Description
Allow folks to delete recordings. A concern for long running containers is the space used by recordings grows unbounded. We have ~some safeguards in place (e.g. total amount of space for a single recording, total amount of time for a single recording) but no way of actually marking recordings for clean up. I chose what felt like a simple path of "whoever is telling me to record can also tell me when to clean up". To be clear: this isn't meant to prevent all failure modes wrt storage but in typical workflows this should keep run times well within bounds
Testing
Units + end to end with our API
TL;DR
Added a new API endpoint and underlying functionality to allow users to delete recordings, addressing concerns about unbounded storage growth.
Why we made these changes
To provide a mechanism for cleaning up old recordings, preventing them from consuming excessive disk space, especially in long-running container environments.
What changed?
/recordings/:id/deleteendpoint to trigger recording deletion.Recorderinterface now includesDeleteandIsDeletedmethods.FFmpegRecorderimplements deletion, removing associated files and marking recordings as deleted.api.goprevents downloads of deleted recordings and handles deletion requests.ListActiveRecordersnow excludes deleted recordings.openapi.yamland regeneratedoapi.goto reflect the new deletion endpoint.api_test.gomock recorder updated to simulate deletion scenarios.Validation