fix: sweep extraction directories abandoned by a crash - #80
Merged
Conversation
download's temp extraction directory is cleaned up by a defer, which never runs on a hard kill: SIGKILL, an OOM kill, a power loss. Whatever partial extraction existed at that instant was left on disk, and nothing in the codebase ever noticed. reap explicitly skips anything dot-prefixed -- by design, since it cannot tell an abandoned extraction apart from one a concurrently running agent is still writing into, and deleting a live one out from under it would be worse than leaking one. The next reconciliation does not help either: sync only checks whether the final directory exists, and calls download again on a fresh, differently-named temp directory each time, with no awareness the old one is still there. So a crash mid-download left a permanent leak, growing by one directory per crash, forever. Age is what actually distinguishes the two cases. A live extraction keeps creating entries under its directory, which keeps bumping that directory's own mtime; an abandoned one stops the instant the process dies and never moves again. reapStaleExtractions sweeps a dot-prefixed directory once it has sat untouched past MinAge -- the bound already used elsewhere in reap for "how long before we are sure nothing still needs this" -- rather than introducing a second, unconfigurable threshold. performance.md's fixture unpacks in well under a second, so even the 2-hour default leaves an enormous margin before this could ever mistake a live extraction for a dead one. Called once per Once() cycle, fleet-wide rather than per-environment, since an abandoned extraction is not tied to converging any particular environment. Documents the fix in agent.md's Reaping section and the disk-sizing math in production.md, which previously had no way to account for this. go test -race ./... passes; gofmt, go vet clean. Signed-off-by: Michael Harp <mike@mikeharp.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Michael Harp <mike@mikeharp.com>
#79 (streaming) and #80 (crash cleanup) were built in parallel, so the "fetch is streamed" section couldn't yet point at the crash-handling section it's really asking about. Now that both are on main, restore the cross-reference and drop the "or left behind by a crash mid-download" claim from the streaming section, since it was really about the artifact never being staged as a plain file, not a claim about what happens to the temp directory on a crash -- which is a different, now-documented, question. Signed-off-by: Michael Harp <mike@mikeharp.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Why
Came up while explaining
download()'s temp-extraction cleanup: what actually happens to that temp directory if the agent process is killed mid-extraction, not just returns an error?download()'s cleanup is adefer os.RemoveAll(tmp), which never runs on a hard kill —SIGKILL, an OOM kill, a power loss. Whatever partial extraction existed at that instant is left on disk. Traced every path that could plausibly clean it up afterward, and found none do:reap()explicitly skips anything dot-prefixed. That's deliberate, not an oversight — it has no way to tell a crash-abandoned extraction apart from one a concurrently running agent process is still actively writing into, and deleting a live one out from under it would be worse than leaking one.sync()only checks whether the final directory exists; if not, it callsdownload()again, which callsos.MkdirTempagain, generating a brand-new, differently-named temp directory each time. It has no awareness the old orphaned one exists.So a crash mid-download left a permanent leak — one more directory, forever, per crash — until an operator noticed and removed it by hand.
Fix
Age is what actually distinguishes "abandoned" from "still live": a live extraction keeps creating entries under its directory, which keeps bumping that directory's own
mtime; an abandoned one stops moving the instant the process dies.reapStaleExtractions()sweeps a dot-prefixed directory once it has sat untouched pastMinAge— reusing the same boundreapalready uses elsewhere for "how long before we're sure nothing still needs this," rather than inventing a second, unconfigurable threshold.performance.md's fixture unpacks in well under a second, so even the 2-hour default leaves an enormous safety margin before this could ever mistake a live extraction for a dead one.Called once per
Once()cycle, fleet-wide rather than per-environment — an abandoned extraction isn't tied to converging any particular environment.Test
TestReapRemovesAbandonedExtractionsPastMinAge: creates one dot-prefixed directory backdated 2 hours (simulating a crash) and one with a fresh mtime (simulating a live extraction,MinAgeset to 1 hour), runs a reconciliation, and asserts the old one is gone and the fresh one survives.Docs
Added an "Extractions abandoned by a crash" section to
agent.md's Reaping, and a short note toproduction.md's disk-sizing math, which previously had no way to account for this.Checks
go test -race ./...passes (full suite).gofmt -l .andgo vet ./...clean.markdownlint-cli2andcspellclean on both docs.🤖 Generated with Claude Code