Skip to content

fix(delete): guard before preview, block on running work, reclaim by reference#363

Merged
LukasWodka merged 3 commits into
developfrom
fix/delete-guard-preview-image-copy
Jul 21, 2026
Merged

fix(delete): guard before preview, block on running work, reclaim by reference#363
LukasWodka merged 3 commits into
developfrom
fix/delete-guard-preview-image-copy

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Fixes #362 · epic backend#1142. Driven by Lukas running tb delete live.

Bugs fixed

  • 🅐 Manifest before the check — the "Removed from this machine …" manifest printed before the work-guard/confirm, then the command errored. Reordered: preconditions first (fail fast), then preview, then confirm. Regression test asserts the preview does not print when the guard blocks.
  • 🅒 Blocked a healthy idle environment — guarded on status == online (always true for a healthy env) and said "stop your training jobs" when there were none. Now guards on num_running_experiments (new client field) with an accurate message; idle-but-online offboards after the typed-name confirm. --force still skips it.
  • 🅑 Manifest → preview — "This will remove:" in plain language ("secure environment", no "Helm release / local cluster / ghcr.io"), no banner.
  • 🅕 Image reclaim by reference — was docker rmi <id>, which fails on a multi-repo image (conflict … must be forced). Now docker images --format {{.Repository}}:{{.Tag}}docker rmi <refs> — untags only tracebloc refs, never a force (which could evict a shared image), and skips dangling <none>.
  • 🅖 No raw daemon error — the reclaim failure is now a quiet plain note (it's pure disk cleanup, excluded from the degraded verdict).

Verification

go test ./internal/cli/... ./internal/nodeboot/... ./internal/api/... green; gofmt/vet clean. (Verified via unit tests — the command is fully seamed; Lukas's env is offboarded so no live run.)

Note

This is a destructive command — reviewer should sanity-check the reorder (guard→preview→confirm→teardown→honest verdict) and the by-reference reclaim.


Note

Medium Risk
Changes destructive offboard ordering, guard criteria, and Docker image removal; mistakes could allow offboard during active training or leave images behind, though unit tests cover the main paths.

Overview
tracebloc delete offboarding is tightened so preconditions run before any removal preview, idle environments can offboard, and image cleanup is safer.

The work-guard now uses GetClient and blocks only when num_running_experiments > 0 (new ProvisionedClient field), not when the client is merely online. Flow is guard → plain-language preview → typed confirm → teardown. If the guard blocks, the "This will remove" preview no longer prints first.

PruneImages lists ghcr.io/tracebloc/* by repo:tag and runs docker rmi on those refs (skips <none>), avoiding multi-repo ID conflicts and never using force. Reclaim failures surface as a short manual-cleanup hint instead of raw daemon errors.

Reviewed by Cursor Bugbot for commit 78d1c7a. Bugbot is set up for automated code reviews on this repo. Configure here.

@LukasWodka LukasWodka self-assigned this Jul 20, 2026
@LukasWodka
LukasWodka requested a review from saadqbal July 20, 2026 17:13
@LukasWodka

Copy link
Copy Markdown
Contributor Author

@BugBot run

Comment thread internal/cli/resources.go Outdated
Comment thread internal/cli/resources.go Outdated
…reference

`tb delete` had several bugs Lukas hit live:

- Manifest printed BEFORE the work-guard/confirm — a full "Removed from this
  machine …" receipt, then an error, for something that didn't happen. Reordered:
  preconditions first (fail fast), then the preview, then confirm.
- Work-guard blocked on `status == online` — but a healthy environment is always
  online, and it told the user to "stop training jobs" when there were none.
  Now blocks on num_running_experiments (a NEW field on the client), with an
  accurate message; a live-but-idle environment offboards after the typed-name
  confirm. --force still skips the guard.
- Preview reworded to a PREVIEW ("This will remove:") in plain language — "secure
  environment" not "client/Helm release/local cluster"; no banner.
- Image reclaim removed by image ID (`docker rmi <id>`), which fails on a
  multi-repo image (`conflict … must be forced`). Now lists by reference
  (repo:tag) and removes those — untags only tracebloc refs, never a force that
  could evict a shared image. Skips dangling <none> refs.
- Softened the reclaim failure (it's pure disk cleanup) and stopped leaking the
  raw docker/daemon error at the user.

Tests: work-guard now blocks on running experiments (and the preview must NOT
print when it blocks); PruneImages by reference + <none>-skip + no-force guard;
new client field decodes.

Fixes #362. Part of epic tracebloc/backend#1142.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
Contributor Author

@BugBot run

@LukasWodka
LukasWodka force-pushed the fix/delete-guard-preview-image-copy branch from b61897d to 995d143 Compare July 20, 2026 18:53

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 995d143. Configure here.

@LukasWodka

Copy link
Copy Markdown
Contributor Author

@BugBot run

Comment thread internal/cli/delete.go Outdated
`docker image prune` (no flags) only removes dangling/untagged images, so it
never clears the leftover tagged ghcr.io/tracebloc/* images a failed best-effort
reclaim leaves behind — the recovery hint didn't match the failure mode. Point
at `docker rmi $(docker images ghcr.io/tracebloc/* -q)`, the same scoped
reference PruneImages targets (never a blanket prune).

Bugbot (PR #363, head 995d143): "Wrong reclaim failure cleanup tip".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
Contributor Author

@BugBot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit bc2935d. Configure here.

Comment thread internal/cli/delete.go Outdated
The failed-reclaim hint recommended `docker rmi $(docker images … -q)`, which
removes by image ID — the exact "referenced in multiple repositories" conflict
PruneImages was changed to avoid by untagging via repo:tag. Match it: the tip
now uses `--format '{{.Repository}}:{{.Tag}}'`, consistent with both the code and
the adjacent comment. Also fix PruneImages' stale doc comment (still said `-q`).

Bugbot (PR #363): "Reclaim tip uses broken ID rmi".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@aptracebloc aptracebloc left a comment

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.

Verified end-to-end — approving.

  • Reorder (🅐): renderOffboardSummary now runs after the work-guard → guard → preview → typed-name confirm → teardown. Test-locked: with active runs, the guard blocks, revoke does NOT run, and the "This will remove" preview does NOT print.
  • Work-guard (🅒): blocks on NumRunningExperiments > 0 (not status == online), via GetClient; preserves the 426 / 401·403 fast-fails and the transient/stale-pointer warn-and-continue; --force skips. id validated (non-empty + numeric) before use.
  • Image reclaim (🅕): by {{.Repository}}:{{.Tag}}, scoped to ghcr.io/tracebloc/*, skips <none>, docker rmi by reference — never forces (a shared image can't be evicted). Fixes the multi-repo rmi <id> conflict.
  • 🅖 reclaim failure is a quiet note, excluded from the degraded verdict.

The work-guard is advisory (transient lookup failure → continue), backstopped by the typed-name confirm — reasonable for a retirement command. Bugbot clean, CI green.

LGTM.

— drafted with Claude (Opus 4.8), sent by @aptracebloc

@LukasWodka
LukasWodka merged commit 1bd94ff into develop Jul 21, 2026
19 checks passed
@LukasWodka
LukasWodka deleted the fix/delete-guard-preview-image-copy branch July 21, 2026 07:55
LukasWodka added a commit that referenced this pull request Jul 21, 2026
The e2e workflow's PR trigger was label-only (`e2e`), so PRs that break
the suites' assertions merge unexercised: #363/#367/#368 reworded the
`tracebloc delete` teardown copy the offboard e2e asserts on, no PR run
ever executed the suite, and every develop push went red until #378
realigned the assertions post-merge.

Add a ~5s dorny/paths-filter probe (`changes`, PR events only) and gate
both e2e jobs on it alongside the existing label opt-in. PRs touching
the union of the suites' dependency surface — the offboard flow
(delete*.go, nodeboot, api, config), the rendering layer (ui — the #367
lesson), the kind-suite seams (cluster, push), the suites themselves,
or the build inputs (Makefile, go.mod/sum, cmd) — now run the e2e
pre-merge (~2 min, jobs in parallel). Unrelated PRs skip both jobs as
before. Push / nightly / dispatch behavior is unchanged: the probe is
skipped there, hence the !cancelled() guards — the implicit success()
on a skipped need would otherwise skip those runs too.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@saadqbal

Copy link
Copy Markdown
Collaborator

/fr-pass

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.

3 participants