feat: close out #12 — container-artifact retention (workspace cleanup + logs)#62
Merged
Merged
Conversation
…iner artifacts With the container backend (#5) merged, the repo checkout is now removed on every path — host and container (--rm) — so the last #12 gap is that a cleanup FAILURE was silently swallowed (.ok()), which #12 flags as leaving private code on disk. Now a failure that leaves the checkout behind is logged loudly and audited (workspace_cleanup_failed). Docs move repo_checkout and container-log handling from deferred to covered. Closes #12. Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR closes out issue #12’s container-artifact retention gap by ensuring per-task workspaces (which contain private repo checkouts) are not left on disk silently when cleanup fails, and by documenting the worker/container artifact retention guarantees (including that raw container stdout/stderr are not persisted).
Changes:
- Replace silent workspace cleanup (
.ok()) with explicit error logging plus an auditableworkspace_cleanup_failedrecord when the workspace still exists. - Update data-retention documentation to mark
repo_checkouthandling as covered and to explicitly document container-log non-retention. - Add a “Worker execution artifacts” section describing workspace teardown behavior and failure surfacing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| docs/data-retention.md | Updates retention/audit documentation to cover repo checkout cleanup behavior and container log non-retention; adds worker artifact section. |
| crates/worker/src/lib.rs | Makes workspace cleanup failures visible (log + audit) instead of silently swallowed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+347
to
+367
| // Workspace cleanup ALWAYS runs — success or failure. The workspace holds | ||
| // the private repository checkout, so a cleanup FAILURE that leaves it on | ||
| // disk must be surfaced and audited, never silently swallowed (issue #12). | ||
| if let Err(e) = tokio::fs::remove_dir_all(&workspace).await { | ||
| // remove_dir_all also errors when the dir never existed; only alarm | ||
| // when the checkout is genuinely still on disk. | ||
| if tokio::fs::try_exists(&workspace).await.unwrap_or(true) { | ||
| error!( | ||
| task_id = %task.id, | ||
| workspace = %workspace.display(), | ||
| "workspace cleanup FAILED — a private checkout may remain on disk: {e:#}" | ||
| ); | ||
| let _ = store | ||
| .record_api_read( | ||
| &format!("worker:{}", task.id), | ||
| &format!("{}/{}", task.repo_owner, task.repo_name), | ||
| "workspace_cleanup_failed", | ||
| "error", | ||
| ) | ||
| .await; | ||
| } |
Comment on lines
+28
to
+30
| | `repo_checkout` | **Never** after task cleanup | ephemeral workspace | The per-task workspace is deleted after every task (success or failure); the container backend also removes the container (`--rm`). A cleanup failure that would leave a checkout on disk is surfaced and audited (`workspace_cleanup_failed`), never swallowed. | | ||
| | `container_logs` | **Not persisted** | — | The container backend (#5) captures no stdout/stderr into any store; only the redacted failure detail reaches `task_attempts.detail`. | | ||
| | `tokens` / `secrets` | **Never** | — | The brief is tokenless (#4); the git token travels to the runtime by environment name only (never in argv or container-inspect output); results, comments, Check Runs, and stored fields are redacted. | |
Comment on lines
+84
to
+86
| The worker runs each task in a per-task workspace, then deletes it — on every | ||
| path, success or failure. In the container backend (#5) the task runs in a | ||
| fresh container removed by `--rm` (and killed by name on timeout), with a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The close-out follow-up now that #5 (container isolation) has merged. Completes #12's container-artifact criteria and closes #12.
What #5 already gave us
The container backend runs each task in a fresh container removed by
--rm(killed by name on timeout), read-only root, dropped caps, resource limits, only the workspace mounted, and the git token forwarded by env name (never in argv). The per-task workspace — which holds the private checkout — is deleted after every task, and a test already asserts removal on the success path.The one remaining gap this closes
Workspace cleanup used
.ok(), silently swallowing failures — exactly the "cleanup failures can leave private code on disk" risk #12 names. Now a removal failure that leaves the checkout on disk is logged loudly and audited (workspace_cleanup_failed), distinguishing a genuine failure (dir still present) from the benign already-gone case.task_attempts.detail. Documented.docs/data-retention.mdmovesrepo_checkoutand container-log handling from deferred to covered, with a new "Worker execution artifacts" section.#12 acceptance criteria — all met
durable stores never persist raw tokens ✅ (+ scan test, #60) · task API redacted + tenant-scoped ✅ · audit records complete ✅ · retention deletes artifacts by installation + age ✅ · repo_checkout never retained, cleanup failures surfaced ✅ (this PR) · no raw container logs retained ✅ · docs self-hosted vs hosted ✅.
Closes #12.
Local gates:
cargo check --all-targets+cargo clippy --all-targets -- -D warnings+cargo test --all(224 passed, 0 failed).🤖 Generated with Claude Code