fix: hub pull can destroy an existing directory on a mid-write failure - #274
Open
SpiliosDmk (SpiliosDimakopoulos) wants to merge 1 commit into
Open
Conversation
writeFilesToDirectory wiped dest (os.RemoveAll + os.MkdirAll) and then wrote each file directly into it. If any single file failed to write partway through -- disk full, a permission error, a path collision like the server returning both "bad" and "bad/x" as entries, the process being interrupted, etc. -- the function returned an error, but dest had already been wiped and only partially repopulated. The pre-existing directory (which may have held content unrelated to the hub repo, if the "wipe non-hub dir" confirmation had been accepted) was gone for good, with no way to recover it. This is the same class of bug as langchain-ai#225 ("apps pull destroys the destination before archive validation succeeds"), applied to a different command (hub pull) and a different failure window (mid-way through a multi-file write loop, rather than before validation). Fix: stage every file write in a temporary sibling directory created next to dest (so the final rename stays on the same filesystem). dest is only removed and replaced -- via os.RemoveAll + os.Rename -- once every file has been written to the staging directory successfully. On any error, the staging directory is cleaned up via a deferred os.RemoveAll and the pre-existing dest is left completely untouched. Added TestHubPull_PreservesExistingDirOnMidWriteFailure, which deterministically forces a mid-loop write failure (the server returns both "bad" and "bad/x" as file entries, so writing whichever one lands second in map iteration order fails: either MkdirAll finds "bad" is already a file, or WriteFile finds "bad" is already a directory) and asserts that the command returns an error while the pre-existing SKILL.md/old.txt content in dest is preserved byte-for-byte, no new files leak into dest, and no staging directory is left behind. Verification note: same go1.25 toolchain limitation as the --include-feedback fix I submitted earlier in this repo -- I could not run `go test ./internal/cmd/...` directly in my sandbox. I did verify the core logic exhaustively outside the module system: I copied writeFilesToDirectory/confirmDirWipe/validateHubFilePath/hasHubMarker verbatim (byte-for-byte) into a standalone go1.23 program with no external dependencies (they only use os/path/filepath/fmt/strings) and: 1. Ran the *unmodified* original version of the function through the exact same "bad" + "bad/x" collision scenario used in the new test, and confirmed it reproduces the bug: the pre-existing SKILL.md is deleted ("open .../dest/SKILL.md: no such file or directory"). 2. Ran the new staged-write version through the same scenario and confirmed: an error is returned, the original SKILL.md and old.txt survive with unchanged content, good.txt is never partially applied to dest, and no staging directory is left behind. 3. Also verified the normal (non-failure) path: all files land in the right place with the right content, and the staging directory is cleaned up after a successful pull. `gofmt -l` is clean on both changed files. Please still run `go build ./...` and `go test ./...` in a proper go1.25 environment before merging.
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
1 participant
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.
Problem
writeFilesToDirectorywipeddest(os.RemoveAll+os.MkdirAll)then wrote files directly into it. A failure partway through the
write loop (disk full, permission error, a path collision, an
interrupted process) left
destwiped-but-incomplete, with no way torecover the pre-existing content.
Same bug class as #225 ("apps pull destroys the destination before
archive validation succeeds"), applied to
hub pull.Fix
Stage every write in a temporary sibling directory.
destis onlyremoved and replaced (
os.RemoveAll+os.Rename) once every filehas been staged successfully. On any error, staging is cleaned up via
a deferred
os.RemoveAllanddestis left untouched.Tests
Added
TestHubPull_PreservesExistingDirOnMidWriteFailure, whichdeterministically forces a mid-loop failure (server returns both
"bad" and "bad/x" as file entries) and asserts the pre-existing
content survives byte-for-byte, no partial files leak into
dest,and no staging directory is left behind.
Verification
My sandbox can't run Go 1.25 (this repo's requirement). I verified
the core logic by copying the exact function bodies into a standalone
go1.23 program (stdlib-only, no external deps) and:
confirmed it destroys the pre-existing SKILL.md.
confirmed the original content survives and no partial files leak.
gofmt -lis clean. Please still rungo build ./...andgo test ./...before merging.