fix(backup): short-retry ACL denials and abort on a lost VSS snapshot (#3259, #3260)

[ToddHebebrand]

Fixes #3259
Fixes #3260

Two coupled backup defects from v0.104.0 release QA on a real Windows Server 2022 agent. The first wastes wall-clock; the second turns that wasted wall-clock into total data loss. Same files, one PR.

#3259 — plain ERROR_ACCESS_DENIED burned ~30s per file

#3002/#2997 taught the per-file upload retry to fast-skip permanently-failing files, but deliberately excluded plain ERROR_ACCESS_DENIED (5) because it "can be a transient AV/indexer hold". So the single most common cause of an unreadable file — an ordinary NTFS ACL — paid the full 30s backoff. Measured +27-29s per denied file.

The ACL probe (the issue's first choice) was NOT taken. FILE_READ_ATTRIBUTES and FILE_READ_DATA are distinct rights, and a failed probe still cannot prove the original denial came from the DACL rather than an AV/EDR minifilter. There is no discriminator that cannot produce a false "permanent" verdict — and a false permanent verdict silently drops a healthy file from a backup, which is precisely the failure class of #3260. Took the issue's option 2 instead.

classifyPermanentUploadError (bool) becomes classifyUploadFailure, returning an uploadRetryPolicy:

policy	when	wait
skipWithoutRetry	not-found, sharing/lock violation, cloud placeholder	none
retryAfterShortDelay	source permission denial (new)	1s
retryAfterDefaultDelay	everything unrecognised	30s

The retry itself — the thing that actually recovers a transient hold — is preserved; only the wait shrinks. Anything unrecognised still gets the full backoff, so a misclassification costs wall-clock, never a file.

Matched two ways: via the Win32 table (ERROR_ACCESS_DENIED, so logs carry the precise symbolic name) and portably via fs.ErrPermission (EACCES/EPERM on Unix). The table is consulted first so ERROR_SHARING_VIOLATION — which Go also maps to fs.ErrPermission — keeps its skipWithoutRetry. A Windows-only test pins that ordering.

#3260 — the stalls outlived the shadow copy and lost everything

With 15 denied files of 40, the accumulated backoff ran ~390s, the shadow copy stopped resolving, and files 14-40 — every one readable — failed with ERROR_PATH_NOT_FOUND. Because PATH_NOT_FOUND is in the fast-skip set, the run drained its remaining file list at memory speed and recorded all of it as per-file failures: 0 of 40 backed up, status=failed, an error log that blamed the files.

New agent/internal/backup/source_liveness.go adds a probe over the shadow-copy device roots the run reads from, consulted only after a per-file upload has already failed — before the retry sleep, and again after it, since the retry is exactly where the reported ACCESS_DENIED -> PATH_NOT_FOUND flip happened. A root that has gone away aborts the run with an explicit error naming the root and the progress counts, instead of condemning every remaining file.

Three properties keep the abort from becoming its own false-positive bug:

• Self-calibrating — each root is stat'd at construction; only roots that resolved then are watched. The probe can never fail on a path form os.Stat could not resolve in the first place (\\?\GLOBALROOT\... is an unusual shape, and a probe that assumed it stats cleanly would abort every healthy backup if that assumption were wrong).
• Confirmed — a root must be missing on two looks, separated by a short delay, before a run dies.
• Per-volume — scoped to the failing file's own root, so losing volume D never aborts files still being read from volume C.

Only a not-exist answer counts; any other stat error is inconclusive and treated as alive.

Abort semantics: mirrors abortStopped, does not publish a partial manifest. With a journal, the remote prefix plus the journal remain this run's resume state, so the already-uploaded bytes survive and the next run picks them up. Publishing a manifest for the partial set would mint a snapshot that looks complete while silently missing every file the loop never reached — a restore point that lies. Flagging this as the one design call worth a second opinion.

No cumulative retry wall-clock budget, deliberately. A VSS_CTX_BACKUP shadow copy has no TTL, so age does not predict validity and any fixed budget would be an arbitrary guess. The liveness probe is the real defense.

Secondary (API) — malformed-payload errors named no field

All three Malformed backup result payload sites (jobs/backupWorker.ts, routes/agentWs.ts, services/commandResultHandlers.ts) joined only issue.message and dropped the Zod path, so the reported Invalid input: expected object, received null was unactionable.

Adds describeZodIssues to apps/api/src/lib/zodIssues.ts — reusing the existing flattening, which also unwraps invalid_union noise — and uses it at all three sites rather than adding a fourth ad-hoc .map(i => ...). An empty path renders as <root> rather than being omitted: that is the distinction that makes "the whole payload was null" legible instead of ambiguous.

Verification

check	command	result
backup pkg	cd agent && go test -race -count=1 ./internal/backup/...	7 packages ok
full agent	cd agent && go test -race -count=1 ./...	exit 0, 66 packages, 0 failures
windows build	GOOS=windows GOARCH=amd64 go build ./...	clean
windows tests compile	GOOS=windows GOARCH=amd64 go vet ./internal/backup/...	clean (only pre-existing unsafe.Pointer notes in untouched vss_windows.go)
linux build	GOOS=linux GOARCH=amd64 go build ./...	clean
API tests	pnpm exec vitest run --no-file-parallelism src/lib/zodIssues.test.ts src/jobs/backupWorker.test.ts src/routes/agentWs.test.ts src/routes/agentWs.terminalResultSchema.test.ts	4 files, 137 passed
API typecheck	pnpm exec tsc --noEmit -p tsconfig.json	clean

New coverage: the retry-policy table (incl. the ordering guarantee), the short-vs-long backoff split, the liveness probe's self-calibration / confirmation / per-volume scoping / inconclusive-error handling, an end-to-end "shadow copy dies at file 3 of 6" regression asserting the run stops at 3 attempts rather than walking a dead snapshot, and <root> vs named-path rendering.

Residual risk — likely true root cause of #3260 is NOT fixed here

vss.CreateShadowCopy releases IVssBackupComponents before returning (agent/internal/backup/vss/vss_windows.go, the defer callVtable(backupComponents, vtblRelease)), with a comment asserting Windows only reclaims a non-persistent VSS_CTX_BACKUP copy on process exit. Microsoft documents the opposite: releasing that object deletes auto-release shadow copies. The existing live test only reads through the device shortly after Release, so it would not catch a delayed reclaim — and a delayed reclaim fits the observed ~390s disappearance at least as well as diff-area exhaustion.

This PR does not attempt that fix. Keeping the requester alive on a dedicated locked COM thread across scan + upload + manifest, with BackupComplete/AbortBackup, is a lifetime rewrite of the vss package — Windows-only, untestable from here, and already tracked as a follow-up on #2999. #3260 names the explicit abort as the required minimum and snapshot refresh as stretch, so this PR delivers the abort and the liveness probe (which is still needed regardless — diff-area exhaustion and external deletion are real). Worth its own issue.

Also worth noting: the outer queue schema (backupProcessResultSchema) guarantees data.result is a non-null object before processResults runs, so the reported root-level "received null" most likely arose from a nested null. Either way the new message now names whatever it was.

🤖 Generated with Claude Code

---

Review round (3 agents: code-reviewer, silent-failure-hunter, pr-test-analyzer)

5 findings raised, all addressed. code-reviewer found no correctness bugs. The other two found real gaps:

#	From	Finding	Fix
1	silent-failure	VSS active but zero watchable roots returned nil with only Debug lines — the #3260 guard could be entirely OFF and look identical to "armed, nothing wrong"	Warn once, naming how many roots were offered
2	silent-failure	A root stat'ing with a persistent non-not-exist error was treated as alive and logged at Debug forever	shadowRootMissing is now three-way (alive / gone / inconclusive); caller warns once per root per run
3	self-found	matchShadowRoot used a bare strings.HasPrefix — ...ShadowCopy2 prefixes ...ShadowCopy26, so a file under an unwatched ShadowCopy26 would be checked against ShadowCopy2's root and could abort a healthy run	Match on a path boundary (separator or exact); new TestMatchShadowRoot_MatchesOnPathBoundaryNotBarePrefix
4	self-found	An inconclusive confirmation look was logged as "came back" — a message that misdescribes what happened, which is #3260's own complaint	Reported on its own terms
5	pr-test-analyzer	The post-retry liveness probe was not pinned by any test: disabling only it left the package green, because the existing regression test kills the snapshot synchronously with the first attempt	New TestCreateSnapshot_ShadowCopyLostDuringRetry_AbortsExplicitly, proven non-vacuous

None of these changed the abort decision — only a confirmed not-exist still aborts. They changed what an operator can see, and closed one genuine false-abort vector (#3 hardening).

A comment of mine was factually wrong and is corrected. I had written that Go maps both ERROR_ACCESS_DENIED and ERROR_SHARING_VIOLATION to fs.ErrPermission, making table-before-portable ordering the thing that keeps a sharing violation on skipWithoutRetry. Checked the stdlib (syscall/syscall_windows.go, Errno.Is): only ERROR_ACCESS_DENIED/EACCES/EPERM map to fs.ErrPermission. ERROR_SHARING_VIOLATION does not — it has no portable fallback at all. Ordering still matters (for the precise symbolic reason string support reads), and the sharing/lock/cloud rows guard against the table being dropped entirely. Comments now say what is true.

CI caught the rename, as designed. The Test Agent (Windows) step pins test names in its -run filter and asserts an exact PASS count precisely so a rename can't quietly drop coverage. The classifier rename tripped it; three names updated, count unchanged at 14 (the rename was one-for-one).

Known remaining gap (not fixed here)

The manager-level wiring in RunBackupContext — sourceLiveness = newShadowRootLiveness(vssSession.ShadowPaths) and its threading into createSnapshotWithProgress — has no test. Every liveness test calls the leaf functions directly. A refactor that dropped those two lines would pass CI and only resurface as a #3260 recurrence in production. Covering it needs the vss.Provider to become injectable into RunBackupContext (today it's constructed inline via vss.NewProvider, and the whole branch is runtime.GOOS == "windows"-gated so it never executes in CI's Linux/macOS runs). That's a testability refactor of the manager, deliberately out of scope here — worth its own issue alongside the requester-lifetime one.

[cloudflare-workers-and-pages]

Deploying breeze with    Cloudflare Pages

Latest commit:	dfb890e
Status:	✅  Deploy successful!
Preview URL:	https://245841a7.breeze-9te.pages.dev
Branch Preview URL:	https://toddhebebrand-fix-3259-3260.breeze-9te.pages.dev

View logs

[ToddHebebrand]

Review run: /pr-review-toolkit:review-pr — code-reviewer, silent-failure-hunter, pr-test-analyzer (all three, on the initial commit), plus a Codex xhigh read-only design consult before implementation.

Findings: 5 raised → all 5 addressed across 9c859521a, dc01661ec, dfb890e58; 0 outstanding.

• code-reviewer: 0 findings (checked the source-attribution guard, branch ordering, false-abort vectors, abort/cancel precedence, off-Windows errno safety).
• silent-failure-hunter: 2 — the guard could be entirely OFF with only Debug-level evidence, and a persistently inconclusive root stat was swallowed forever. Both now warn; neither changed the abort decision.
• pr-test-analyzer: 1 — the post-retry liveness probe was pinned by nothing (disabling only it left the package green). New TestCreateSnapshot_ShadowCopyLostDuringRetry_AbortsExplicitly, verified non-vacuous. It also confirmed the pre-existing regression test is non-vacuous by the same method.
• Self-found while reviewing: matchShadowRoot matched a bare string prefix (...ShadowCopy2 prefixes ...ShadowCopy26) — a genuine false-abort vector, now boundary-matched; and an inconclusive confirmation look was mislabelled "came back".

Also corrected a factually wrong comment I had written: only ERROR_ACCESS_DENIED/EACCES/EPERM map to fs.ErrPermission in syscall/syscall_windows.go — ERROR_SHARING_VIOLATION does not, so the ordering guarantee holds for a different (still real) reason than I claimed.

Tests: go test -race ./... in agent/ — exit 0, 66 packages, 0 failures. Cross-compile clean for GOOS=windows and GOOS=linux. API: 137 passed across the 4 affected files single-fork; tsc --noEmit clean.

CI: 54 pass / 0 fail / 1 skipping (55 total). Test Agent (Windows) failed on the first push — its -run filter pins test names and asserts an exact PASS count specifically so a rename can't silently drop coverage. The classifier rename tripped it; names updated, count unchanged at 14. It passes now.

Status: review-clean, CI green, awaiting maintainer merge.

Two things flagged for the maintainer rather than fixed here, both argued in the PR body:

1. The likely true root cause of [Agent][Backup] DATA LOSS: upload stalls outlive the VSS shadow copy — ~15 denied files destroys the backup of every healthy file #3260 is untouched — vss.CreateShadowCopy releases IVssBackupComponents before returning, and Microsoft documents that as deleting auto-release shadow copies. Fixing it is a COM-lifetime rewrite of the vss package (already a tracked follow-up on [Agent][Backup] VSS InitializeForBackup fails with E_INVALIDARG on every run (test VM) and the backup silently proceeds without a snapshot #2999). The liveness probe is needed regardless.
2. The manager-level wiring is untested — covering it needs vss.Provider to become injectable into RunBackupContext.