Repository navigation
Clean up older remote run dirs and unused source tars (OR-575) - #757
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[High impact] The change removes data from remote SSH and Slurm storage. Fix lost cleanup retries and the project-deletion race before merging. Findings
Fix with agent prompt### Issue 1
src/jobs/ssh/cleanup.rs:95
If a container is stopped after its run finishes, `docker exec` fails. This script ignores that failure and reports `REMOVED` when host removal succeeds. `record` then drops the queue entry, leaving the container’s files behind even after it restarts. Check the container removal result and keep transient failures queued.
### Issue 2
src/local/remote_cleanup.rs:110
Deleting a project while SSH cleanup is in flight can lose the request for full removal. The batch may have read `deleted=false` and therefore keep `tensorboard/`. Project deletion upgrades the row to `deleted=true`, but this completion deletes that newer row when the old batch returns. The remote TensorBoard files are then left behind permanently. Carry the processed flag or a queue version through the result, and only finish the matching request.
### Issue 3
src/local/remote_cleanup.rs:58-65
For short runs, the first cleanup usually happens before the source archive has been idle for an hour, so GC skips it. That pass removes the run’s queue entry, and later passes only visit hosts with queued runs. If the user submits no more runs there, the archive stays forever-even with the app running. Schedule a later source-cache pass independently of run removal.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds queued cleanup for finished SSH and Slurm runs, a keep setting, source-cache GC, and full cleanup on project deletion.
myles332 explicitly leaves old runs and failed log drains untouched until project deletion, and intentionally removes the full container copy.
|
| .as_ref() | ||
| .map_or_else(String::new, |(id, dir)| { | ||
| format!( | ||
| "docker exec {} rm -rf -- {} </dev/null >/dev/null 2>&1; ", |
There was a problem hiding this comment.
Container cleanup failures are lost
If a container is stopped after its run finishes, docker exec fails. This script ignores that failure and reports REMOVED when host removal succeeds. record then drops the queue entry, leaving the container’s files behind even after it restarts. Check the container removal result and keep transient failures queued.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/jobs/ssh/cleanup.rs
Line: 95
Comment:
**Container cleanup failures are lost**
If a container is stopped after its run finishes, `docker exec` fails. This script ignores that failure and reports `REMOVED` when host removal succeeds. `record` then drops the queue entry, leaving the container’s files behind even after it restarts. Check the container removal result and keep transient failures queued.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in fa544b7 / 702139a. The cleanup script now checks the container: if docker container inspect finds it but docker exec rm fails (stopped/paused), or Docker can't be queried, the run reports FAILED <id> container: … and stays queued with backoff. The host copy is still removed. Only a container Docker reports gone counts as cleaned. Covered by container_runs_stay_queued_until_docker_reports_the_container_gone (stopped / removed / daemon-down cases).
| let mut failures = Vec::new(); | ||
| for (run_id, outcome) in results { | ||
| match outcome { | ||
| Outcome::Removed => store.finish_remote_cleanup(&run_id)?, |
There was a problem hiding this comment.
Project deletion loses cleanup
Deleting a project while SSH cleanup is in flight can lose the request for full removal. The batch may have read deleted=false and therefore keep tensorboard/. Project deletion upgrades the row to deleted=true, but this completion deletes that newer row when the old batch returns. The remote TensorBoard files are then left behind permanently. Carry the processed flag or a queue version through the result, and only finish the matching request.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/local/remote_cleanup.rs
Line: 110
Comment:
**Project deletion loses cleanup**
Deleting a project while SSH cleanup is in flight can lose the request for full removal. The batch may have read `deleted=false` and therefore keep `tensorboard/`. Project deletion upgrades the row to `deleted=true`, but this completion deletes that newer row when the old batch returns. The remote TensorBoard files are then left behind permanently. Carry the processed flag or a queue version through the result, and only finish the matching request.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in fa544b7. finish_remote_cleanup and defer_remote_cleanup now take the deleted flag the pass acted on and only touch a row with that flag (… AND deleted = ?). Project deletion only ever raises the flag (MAX) and makes the row due immediately, so a pass that read deleted=false can no longer consume the upgraded row; the next pass removes tensorboard/ too. Covered in the store test (finish("r2", false)/defer("r2", false) leave the deleted row due).
| let batches = hosts | ||
| .into_iter() | ||
| .map(|(host, runs)| { | ||
| Ok(HostBatch { | ||
| keep_digests: active_source_digests(store, &host)?, | ||
| host, | ||
| runs, | ||
| }) |
There was a problem hiding this comment.
Source archives never get revisited
For short runs, the first cleanup usually happens before the source archive has been idle for an hour, so GC skips it. That pass removes the run’s queue entry, and later passes only visit hosts with queued runs. If the user submits no more runs there, the archive stays forever—even with the app running. Schedule a later source-cache pass independently of run removal.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/local/remote_cleanup.rs
Line: 58-65
Comment:
**Source archives never get revisited**
For short runs, the first cleanup usually happens before the source archive has been idle for an hour, so GC skips it. That pass removes the run’s queue entry, and later passes only visit hosts with queued runs. If the user submits no more runs there, the archive stays forever—even with the app running. Schedule a later source-cache pass independently of run removal.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in fa544b7 / 702139a. A new remote_source_sweeps (host, root) table: every pass that cleaned runs schedules a sweep of that host's source cache just past the idle window (61 min). When it comes due, a sweep-only batch runs the same GC (still honouring active runs' digests) and the row is finished, best effort. Covered by source_sweeps_come_due_after_the_idle_window.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#754's squash is identical to the myles/remote-storage-root changes already merged here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…etions; revisit source caches Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…weeps Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # src/store.rs
Closes OR-575, which supersedes OR-94. This is phase 3 of OR-569. It is stacked on #754; retarget it to
mainonce #754 merges.Summary
orx upremoves the remote run dir, keepingtensorboard/soorx tensorboardstill works. The in-container copy is removed for container runs.FULL_RUNS_PER_PROJECTconstant and one sharedruns_past_newestSQL helper (ROW_NUMBER()per project overcreated_at DESC, id DESC). As locally, an unfinished newer run takes a slot, and local runs don't count toward the remote slots.orx compute configure ssh --host H --keep-remote-runs true, or... slurm --keep-remote-runs true, keeps every run on the target.--clear keep-remote-runsresets it. It is exposed incompute showand the settings API, and a dashboard save leaves it alone.<root>/source/<sha256>.tarfiles (and stale upload temps) when no active local run on that host references them and they have been idle for an hour. Only orx-shaped names are touched, so user tars under a custom root are safe.touched at the cache check, so other installs sharing the host don't race the GC.Store::delete_local_project's transaction queues every SSH/Slurm run in the project for full removal, ignoring keep and includingtensorboard/. Failures never fail the delete; they stay queued.remote_cleanupstable holds the run's descriptor snapshot, a deleted flag, attempts and the last error. Retries back off from 5 minutes up to 1 day.bash -sSSH call per host, with a 120s timeout. The script goes over stdin to stay under Windows' command-line limit, and ControlMaster is already forced off on Windows.<recorded root>/runs/<uuid>dirs are removed.jobIds must match that layout, and container dirs must end in/.orx/runs/<id>.ttlSecondsAfterFinished) and the managed sandbox (the box is deleted) are left as is.Judgment calls
deletedflag matches what it acted on. If a project delete upgrades the row mid-pass, the next pass removestensorboard/too.orx upis running.Test plan
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings,cargo build --locked,cargo test --locked$HOME. It covers:tensorboard/unless deleteddataset.tarand others survivetensorboard/), andorx tensorboardstill works for it. With--keep-remote-runs truenothing is removed🤖 Generated with Claude Code