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.
|
|
||
| /// Send a user metric to the parent process. | ||
| pub fn send_metric( | ||
| &self, |
There was a problem hiding this comment.
The PR description says returned paths "transfer ownership to Coglet" and the source is "removed only after a successful copy." This line is missing the remove_file call. Without it, a predictor that returns a fixed temp path and fails to overwrite it on the next prediction will still produce stale output (the exact bug described in #1434).
| &self, | |
| std::fs::copy(&path, &dest)?; | |
| std::fs::remove_file(&path)?; | |
| self.send_file_output(dest, mime_type, true) |
If deleting the source is intentionally omitted (the docs were updated to say "The original file is left untouched"), then the PR description should be updated and #1434 should not be claimed as closed.
| /// its on-disk prediction directory as a backstop for any output files | ||
| /// that were not deleted individually (e.g. aborted uploads, cancelled | ||
| /// predictions). | ||
| pub fn remove_prediction(&self, id: &str) { |
There was a problem hiding this comment.
self.predictions.remove(id) is called before the directory cleanup. This means a new prediction with the same ID can be accepted while the old directory is still being deleted, creating a race where the new prediction's files are removed by the old cleanup task. The PR description claims "Cleanup holds that reservation until the prediction directory is removed," but the entry is released first.
Consider either moving remove_dir_all before self.predictions.remove(id), or holding the DashMap entry through cleanup.
|
Posted review comments on PR #3096: #3096 (review) |
|
LGTM |
2 participants
Fixes #1434 by making returned output files an explicit ownership handoff to Coglet. Returned
Pathfiles are moved into per-prediction managed storage, the original path is removed after a successful copy, and the managed file is deleted as soon as the parent has read it. Prediction-directory cleanup remains as a backstop for cancellation and failed uploads.This also hardens the cleanup path: prediction IDs cannot escape the prediction root, duplicate active IDs return 409 without consuming a slot, ID reservations are held through cleanup, and directory symlinks are removed without following them.
Added behavioral coverage for managed and unmanaged files, Python
Pathhandoff, stale-path reuse, duplicate IDs, permit capacity, traversal attempts, symlinks, and cleanup failures. Updated thecog.Pathdocumentation to match the ownership contract.Validation: 235 Coglet tests and 63 Coglet-Python tests pass, with Rust formatting and clippy clean.
Closes #1434.