fix(snapshot): bound git subprocesses so a wedged git cannot stall the turn - #6859
Merged
Merged
Conversation
…e turn The snapshot side repo shells out to git with blocking Command::output calls that have no time bound: a wedged git — a stalled NFS/FUSE mount, lock contention, a hung hook — blocks the per-turn snapshot path, the restore path, and the prune path indefinitely. Every caller already treats a snapshot error as snapshot-disabled-with-warning, so the bound only has to surface as a normal error to degrade gracefully. All three unbounded call sites in snapshot/repo.rs — the one-time `git init`, the date-pinned `commit-tree` on the per-turn prune path, and the shared run_git helper — now route through one bounded core with a generous 300s budget (`git add -A` on a large workspace is legitimately slow). The core drains both pipes while the child runs, exactly what Command::output does: the restore path's `ls-tree -r` and the diff commands emit output that grows with workspace size, and a child blocked on a full pipe buffer never exits, which would turn every such call into a guaranteed timeout. After git exits, the drain collect is bounded by a short grace so a grandchild that inherited the pipes (a daemonizing post-checkout hook, a gc pack worker) cannot hold the call either; whatever was captured is returned with an explanatory note on stderr instead of being truncated silently. On timeout the child is killed and reaped on a detached thread — on a hard-wedged mount the kernel may not deliver SIGKILL until an uninterruptible syscall returns, and a blocking wait would stall the pipeline exactly like the wedged git — and the caller gets io::ErrorKind::TimedOut. Tests: a wedged child driven through the core with a short injected budget must fail with TimedOut promptly; a grandchild holding the pipes must not hold the call past the grace; and a 5000-file tree listing (above the 64 KiB pipe buffer) must still drain instead of dying at the command timeout. Known follow-up: snapshot/delta.rs's read-view git calls (diff-tree, ls-tree, cat-file) are not yet bounded; they are read-view commands on the same wedge class and are left for a follow-up. Signed-off-by: asto <asto18089@126.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.
Summary
The snapshot side repo shelled out to git with blocking
Command::outputcalls that have no time bound: a wedged git — a stalled NFS/FUSE mount, lock contention, a hung hook — blocks the per-turn snapshot path, the restore path, and the prune path indefinitely. Every caller already treats a snapshot error as snapshot-disabled-with-warning, so the bound only has to surface as a normal error to degrade gracefully.All three unbounded call sites in
snapshot/repo.rs— the one-timegit init, the date-pinnedcommit-treeon the per-turn prune path, and the sharedrun_githelper — now route through one bounded core with a 300s budget (git add -Aon a large workspace is legitimately slow). The core drains both pipes while the child runs, exactly whatCommand::outputdoes: the restore path'sls-tree -rand the diff commands emit output that grows with workspace size, and a child blocked on a full pipe buffer never exits, which would turn every such call into a guaranteed timeout. After git exits, the drain collect is bounded by a short grace so a grandchild that inherited the pipes (a daemonizing post-checkout hook, a gc pack worker) cannot hold the call either. On timeout the child is killed and reaped on a detached thread — on a hard-wedged mount the kernel may not deliver SIGKILL until an uninterruptible syscall returns — and the caller getsio::ErrorKind::TimedOut. Killing without git's own cleanup can leave a freshindex.lock; later snapshots then fail fast on the lock with an error naming it.Tests: a wedged child with a short injected budget fails with
TimedOutpromptly; a grandchild holding the pipes cannot hold the call past the grace; a 5000-file tree listing (above the 64 KiB pipe buffer) still drains instead of dying at the command timeout.Known follow-up:
snapshot/delta.rs's read-view git calls (diff-tree,ls-tree,cat-file) are not yet bounded; same wedge class, read-view commands, left for a follow-up.Testing
cargo test -p codewhale-tui --lib snapshot— the bounded core's behavioral tests (real children: timeout, grandchild drain grace, large-output drain)cargo clippy -p codewhale-tui --all-targets --all-features --lockedcargo fmt --all -- --checkAdapted from the Pinvou fork's timeout audit (Pinvou/CodeWhale
d349f2537, the snapshot slice), reworked onto the current snapshot architecture.Checklist
CHANGELOG.mdchanges