MaterializeSnapshot: a failure during extraction leaves a partial snapshot while the sidecar and the digest both claim it is intact #265

New issue

Open

opened 2026-08-03 01:30:42 +02:00 by dominik.polakovics · 0 comments

dominik.polakovics commented

2026-08-03 01:30:42 +02:00

Owner

Found while landing PR #264 (read-only imports, ADR-0063). Not a blocker for that PR — the window is narrow — but the code and the ADR both state a guarantee that does not hold across it.

Current behaviour

gitx.Engine.MaterializeSnapshot (internal/gitx/materialize.go:45-67) runs in this order:

e.Fetch(...)
e.refCommit(...) — resolve origin/<branch>
os.MkdirAll(destDir) + clearDir(destDir)
e.extractArchive(...)

Steps 1–2 before step 3 is the deliberate, valuable part of the design: a refresh against a dead remote (host down, credential rotated, branch gone) fails before anything is destroyed, so the existing snapshot survives. That works and is pinned by TestMaterializeSnapshot_fetchFailureLeavesSnapshotIntact.

The gap is a failure in step 4, after the destination has already been cleared: a git archive timeout (e.timeout), a disk-full or I/O error during extraction, or the process dying mid-stream. The destination is then a partial tree, and because the <name>.commit sidecar is only rewritten after a successful materialization, it still names the old commit.

Why that matters

Two places state the stronger guarantee:

internal/pull/pull.go, refreshImport doc: "a dead target … leaves the spawn's snapshot intact and the agent keeps a stale-but-real sibling rather than an empty directory", and ImportChange: "the snapshot on disk is still the one the run was spawned with, because gitx destroys nothing until the new commit is known."
ADR-0063: "the agent is told which import is stale and keeps its still-valid snapshot."

So on a mid-extraction failure /pull-base emits - <name>: refresh failed — <reason> and the agent is told it still holds the spawn's snapshot, while it actually holds a truncated tree whose sidecar advertises a commit the tree no longer matches. An agent reading that import for a signature could read a half-populated directory and conclude a file or symbol does not exist.

At spawn the consequence is milder — materializeImports refuses the launch and wipeHome() removes the whole per-run tree — so this is really a /pull-base refresh problem.

Acceptance criteria

A failure during extraction leaves the previous snapshot in place, OR the "snapshot is intact" claim is removed from internal/pull/pull.go (both the refreshImport doc and the ImportChange doc) and from ADR-0063, and the digest line for a failed refresh stops implying the old tree survived
Whichever route is taken, a test pins the post-clear failure the way TestMaterializeSnapshot_fetchFailureLeavesSnapshotIntact pins the pre-clear one (inject an extraction failure — e.g. a git stub that emits a truncated tar, or a very short e.timeout)
If the intact-on-failure route is chosen: the destination directory's inode must still be preserved, since it is bind-mounted into a live container (the constraint MaterializeSnapshot's doc and ADR-0063 both call out). Extracting to a sibling temp dir and swapping contents into the existing directory satisfies both; a rename of the directory itself does not

Notes for whoever picks this up

The two routes are a real trade, not a formality — swapping contents in doubles peak disk for the snapshot and widens the window where the directory is half-new, whereas relaxing the docs is honest and free but leaves the agent with a truncated tree. PRD #260 / ADR-0063 settled the ordering itself; this issue is only about the guarantee stated on top of it, so the ADR's decisions are not up for re-litigation.

Found while landing PR #264 (read-only imports, ADR-0063). Not a blocker for that PR — the window is narrow — but the code and the ADR both state a guarantee that does not hold across it. ## Current behaviour `gitx.Engine.MaterializeSnapshot` (`internal/gitx/materialize.go:45-67`) runs in this order: 1. `e.Fetch(...)` 2. `e.refCommit(...)` — resolve `origin/<branch>` 3. `os.MkdirAll(destDir)` + `clearDir(destDir)` 4. `e.extractArchive(...)` Steps 1–2 before step 3 is the deliberate, valuable part of the design: a refresh against a dead remote (host down, credential rotated, branch gone) fails **before** anything is destroyed, so the existing snapshot survives. That works and is pinned by `TestMaterializeSnapshot_fetchFailureLeavesSnapshotIntact`. The gap is a failure in **step 4**, after the destination has already been cleared: a `git archive` timeout (`e.timeout`), a disk-full or I/O error during extraction, or the process dying mid-stream. The destination is then a **partial tree**, and because the `<name>.commit` sidecar is only rewritten after a successful materialization, it still names the **old** commit. ## Why that matters Two places state the stronger guarantee: - `internal/pull/pull.go`, `refreshImport` doc: *"a dead target … leaves the spawn's snapshot intact and the agent keeps a stale-but-real sibling rather than an empty directory"*, and `ImportChange`: *"the snapshot on disk is still the one the run was spawned with, because gitx destroys nothing until the new commit is known."* - ADR-0063: *"the agent is told which import is stale and keeps its still-valid snapshot."* So on a mid-extraction failure `/pull-base` emits `- <name>: refresh failed — <reason>` and the agent is told it still holds the spawn's snapshot, while it actually holds a truncated tree whose sidecar advertises a commit the tree no longer matches. An agent reading that import for a signature could read a half-populated directory and conclude a file or symbol does not exist. At spawn the consequence is milder — `materializeImports` refuses the launch and `wipeHome()` removes the whole per-run tree — so this is really a `/pull-base` refresh problem. ## Acceptance criteria - [ ] A failure during extraction leaves the *previous* snapshot in place, OR the "snapshot is intact" claim is removed from `internal/pull/pull.go` (both the `refreshImport` doc and the `ImportChange` doc) and from ADR-0063, and the digest line for a failed refresh stops implying the old tree survived - [ ] Whichever route is taken, a test pins the post-clear failure the way `TestMaterializeSnapshot_fetchFailureLeavesSnapshotIntact` pins the pre-clear one (inject an extraction failure — e.g. a `git` stub that emits a truncated tar, or a very short `e.timeout`) - [ ] If the intact-on-failure route is chosen: the destination directory's **inode must still be preserved**, since it is bind-mounted into a live container (the constraint `MaterializeSnapshot`'s doc and ADR-0063 both call out). Extracting to a sibling temp dir and swapping *contents* into the existing directory satisfies both; a rename of the directory itself does not ## Notes for whoever picks this up The two routes are a real trade, not a formality — swapping contents in doubles peak disk for the snapshot and widens the window where the directory is half-new, whereas relaxing the docs is honest and free but leaves the agent with a truncated tree. PRD #260 / ADR-0063 settled the ordering itself; this issue is only about the guarantee stated on top of it, so the ADR's decisions are not up for re-litigation.

dominik.polakovics added the

needs-triage

bug

labels

2026-08-03 01:30:42 +02:00