fix: delete coglet-managed output files after upload - #3096
Conversation
Coglet creates output files (IOBase writes, oversized JSON spills) in
/tmp/coglet/predictions/{id}/outputs/ but never deletes them. This can
cause stale-output bugs where models return outputs from previous
predictions if an overwrite fails (issue #1434).
Add a managed bool field to the FileOutput IPC protocol message so
the orchestrator knows which files are safe to delete (coglet-created)
vs user-authored Path outputs (returned by reference, must not be
deleted). The orchestrator deletes managed files immediately after
reading their bytes into memory. A backstop remove_dir_all in
remove_prediction cleans up the entire prediction directory to catch
files from aborted uploads or cancelled predictions.
Closes #1434
|
LGTM |
michaeldwan
left a comment
michaeldwan
left a comment
There was a problem hiding this comment.
Thanks for taking this on. Cleaning up Coglet-created spill files is worthwhile, and making ownership explicit in the protocol is a useful direction.
I’m requesting changes because the recursive cleanup currently trusts a client-controlled prediction ID, duplicate IDs can delete a live prediction’s state, and predictor-returned paths -- the files described in #1434 -- remain intentionally undeleted. I’ve left details inline.
Please also add behavioral coverage around cleanup. The snapshots prove that managed serializes, but they don’t prove that managed files are deleted, that returned paths follow the intended contract, or that one prediction can’t remove another prediction’s directory.
Addresses review feedback on PR #3096 by validating prediction IDs, rejecting duplicate active IDs, hardening remove_dir_all, and safely handling returned cog.Path files via the managed output directory.
|
LGTM |
|
LGTM |
|
Follow-up summary for Michael’s review:
Validation completed:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds useful managed-file cleanup for IOBase writes and oversized JSON spills, and strengthens prediction-ID validation before directory deletion. However, two claims in the PR description and review responses don't match the actual code.
-
Stale-file bug (#1434) not fixed for Path outputs —
send_user_file_outputcopies external paths into the managed output directory but never deletes the original file. The PR description explicitly states "removes the source only after a successful copy," and this was the core concern raised by Michael in the prior review. Without deleting the source, a predictor that returns a fixed temp path and fails to overwrite it on a subsequent prediction will still upload stale data. -
ID reservation not held through cleanup —
remove_predictionremoves the DashMap entry before callingremove_dir_all, so a reused ID could be submitted and have its new files deleted by the old cleanup task. The PR description claims the reservation is held through deletion, but the code does the opposite.
Suggested fixes are inline.
|
Posted review comments on PR #3096: #3096 (review) |
|
LGTM |
|
@anish-sahoo Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| self.send_response(200) | ||
| self.send_header( | ||
| "Location", | ||
| f"http://127.0.0.1:{self.server.server_port}{self.path}", |
Fixes #1434.
BaseModeloutputs without flattening the response. The protocol carries indexed structured-output envelopes and keeps internal staging names separate from original upload filenames.Validation:
mise run test:rustmise run test:coglet:python