Skip to content

Clean up older remote run dirs and unused source tars (OR-575) - #757

Merged
myles332 merged 17 commits into
mainfrom
myles/remote-run-cleanup
Oct 23, 2026
Merged

myles332 merged 17 commits into
mainfrom
myles/remote-run-cleanup

Conversation

@myles332

@myles332 myles332 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Closes OR-575, which supersedes OR-94. This is phase 3 of OR-569. It is stacked on #754; retarget it to main once #754 merges.

Summary

  • Default retention, matching local slimming (Slim finished local runs' environments (OR-572) #743): each project's 3 newest SSH/Slurm runs stay whole on their targets. Once an SSH or Slurm run is terminal and the supervisor has collected its log, the run is queued. The entry waits until newer runs push it past its project's newest 3, then a background task in orx up removes the remote run dir, keeping tensorboard/ so orx tensorboard still works. The in-container copy is removed for container runs.
    • The ordering and count reuse the local rule: the same FULL_RUNS_PER_PROJECT constant and one shared runs_past_newest SQL helper (ROW_NUMBER() per project over created_at DESC, id DESC). As locally, an unfinished newer run takes a slot, and local runs don't count toward the remote slots.
    • A dir whose recorded process group is still alive is skipped and retried.
  • Opt-out: 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-runs resets it. It is exposed in compute show and the settings API, and a dashboard save leaves it alone.
  • Source cache GC: while cleaning a host, orx removes its cached <root>/source/<sha256>.tar files (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.
    • A reused tar is now touched at the cache check, so other installs sharing the host don't race the GC.
  • Project delete: one statement inside Store::delete_local_project's transaction queues every SSH/Slurm run in the project for full removal, ignoring keep and including tensorboard/. Failures never fail the delete; they stay queued.
  • Queue and retry: a new remote_cleanups table holds the run's descriptor snapshot, a deleted flag, attempts and the last error. Retries back off from 5 minutes up to 1 day.
    • The task processes at most 50 entries per minute and makes one bash -s SSH 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.
    • It holds the data-dir gate only around store access.
  • Safety:
    • Only <recorded root>/runs/<uuid> dirs are removed.
    • SSH jobIds must match that layout, and container dirs must end in /.orx/runs/<id>.
    • Anything else is dropped from the queue and logged once.
  • Not changed: k8s (ttlSecondsAfterFinished) and the managed sandbox (the box is deleted) are left as is.

Judgment calls

  • Containers: a container that still exists but can't run the removal (stopped or paused), or a Docker daemon that can't be queried, keeps the run queued with backoff. The host copy is still removed. Only a container Docker reports gone counts as cleaned.
  • Racing a project delete: a pass only finishes or defers a queue row whose deleted flag matches what it acted on. If a project delete upgrades the row mid-pass, the next pass removes tensorboard/ too.
  • Source-cache revisits: each host and root is swept again just past the 1-hour idle window. That sweep is best effort: it runs once and isn't retried if the host is unreachable.
  • Cleanup is queued only from supervisors that collected logs. Runs that finished before this version, or whose log drain failed, are untouched until project delete. There is no backfill sweep of existing cluster data.
  • The keep setting is read when the cleanup pass runs, so turning it on protects runs not yet cleaned up.
  • Retention follows the user's decision to match local retention: the newest 3 remote runs per project are kept whole, and removal covers older runs only. Waiting entries stay in the queue until they become eligible, so a run queued long ago is removed on the first pass after a newer run pushes it out.
  • Cleanup only happens while the app or orx up is running.

Test plan

  • cargo fmt --all --check, cargo clippy --all-targets -- -D warnings, cargo build --locked, cargo test --locked
  • The generated host script runs locally under bash against a fake $HOME. It covers:
    • removes finished dirs
    • keeps tensorboard/ unless deleted
    • skips a live process group
    • treats a missing dir as removed
    • GC removes only stale, unreferenced 64-hex tars and temps; dataset.tar and others survive
  • Store tests:
    • newest 3 remote runs per project are kept; a 4th makes the oldest due; an unfinished newer run takes a slot
    • local and other kinds don't count toward the remote slots
    • finished vs deleted queueing, backoff, deletion overriding a deferred entry and including the newest runs Layout validation tests and CLI configure body tests.
  • Live SSH: finish 3 runs in a project and confirm all stay whole. Finish a 4th: within about a minute only the oldest run's dir is removed (except tensorboard/), and orx tensorboard still works for it. With --keep-remote-runs true nothing is removed
  • Live Slurm with a custom root: same checks, plus source tar GC after an hour idle
  • Container run: host and in-container dirs removed
  • Project delete with an unreachable host: delete succeeds, entry retries and is removed once the host is reachable
  • Windows: cleanup over Win32-OpenSSH

🤖 Generated with Claude Code

myles332 and others added 6 commits October 22, 2026 14:39
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>
@greptile-apps

greptile-apps Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 3/5

[High impact] The change removes data from remote SSH and Slurm storage.

Fix lost cleanup retries and the project-deletion race before merging.

Findings

  1. P1 Container cleanup failures are lost ▶
  2. P1 Project deletion loses cleanup ▶
  3. P2 Source archives never get revisited ▶
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.

Summary

Adds queued cleanup for finished SSH and Slurm runs, a keep setting, source-cache GC, and full cleanup on project deletion.

  • Collected SSH and Slurm runs are cleaned up in the background.
  • Unused source archives are removed from SSH and Slurm caches.
  • SSH hosts and Slurm clusters can keep finished run directories.

myles332 explicitly leaves old runs and failed log drains untouched until project deletion, and intentionally removes the full container copy.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Supervisor collects final log] --> B[Queue finished run]
  C[Delete project] --> D[Queue full removal]
  B --> E[Read settings and queued requests]
  D --> E
  E --> F[Group runs by host]
  F --> G[Remove remote files over SSH]
  G --> H[Check old source archives]
  H --> I{Run removal result}
  I -->|Removed| J[Delete queue entry]
  I -->|Busy or failed| K[Retry later]
Loading

Reviews (1) · Last reviewed commit: "Address round-2 review nits for remote c..." · Reviewed by Greptile

Comment thread src/jobs/ssh/cleanup.rs Outdated
.as_ref()
.map_or_else(String::new, |(id, dir)| {
format!(
"docker exec {} rm -rf -- {} </dev/null >/dev/null 2>&1; ",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread src/local/remote_cleanup.rs Outdated
let mut failures = Vec::new();
for (run_id, outcome) in results {
match outcome {
Outcome::Removed => store.finish_remote_cleanup(&run_id)?,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +58 to +65
let batches = hosts
.into_iter()
.map(|(host, runs)| {
Ok(HostBatch {
keep_digests: active_source_digests(store, &host)?,
host,
runs,
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@myles332 myles332 mentioned this pull request Oct 22, 2026
4 of 7 tasks
@myles332 myles332 changed the title Remove finished remote run dirs and unused source tars (OR-575) Clean up older remote run dirs and unused source tars (OR-575) Oct 22, 2026
@myles332
myles332 changed the base branch from myles/remote-storage-root to main October 23, 2026 00:47
myles332 and others added 5 commits October 22, 2026 17:47
#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>
@myles332
myles332 merged commit 96cb2b6 into main Oct 23, 2026
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant