Skip to content

fix(storage): preserve packed Bundle cleanup outcome - #4419

Merged
MicroGery merged 2 commits into
apache:mainfrom
MicroGery:codex/issue-2369-snapshot-followups
Sep 1, 2026
Merged

fix(storage): preserve packed Bundle cleanup outcome#4419
MicroGery merged 2 commits into
apache:mainfrom
MicroGery:codex/issue-2369-snapshot-followups

Conversation

@MicroGery

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #4361 addressing the three non-blocking review observations for #2369:

  • Return a successfully written Bundle together with explicit pending-cleanup state when release of private staging fails; preserve the primary pack failure when packing itself fails.
  • Preserve AbortError from the admitted Runtime operation so the snapshot coordinator classifies it as cancellation rather than an admission I/O failure.
  • Distinguish Session Bundle V1 portability-invalid workspace paths from structurally unsafe paths, with bounded diagnostics.

Tests

  • Biome check for the touched files.
  • Storage build and typecheck.
  • Runtime typecheck.
  • Storage distribution suite: 1,096 tests, 1,088 passed, 0 failed, 8 skipped.
  • Runtime snapshot boundary suite: 6 passed.

AI usage

This PR was developed with AI assistance and reviewed and validated by the author.

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 1, 2026
@MicroGery
MicroGery requested a review from Astro-Han September 1, 2026 03:36

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for turning these around so quickly. Verified against head b621c4e67d.

All three fixes hold up:

  • Pack cleanup outcome. ProductionSessionBundleArtifact carries snapshotCleanup alongside the ordinary artifact fields, so a successful Bundle survives a failed staging release instead of being thrown away, while a genuine pack failure still surfaces as the primary error. The regression test drives it through the real release() identity check by displacing the staging root from an injected bundleFileService, so it exercises the production owner rather than a stubbed cleanup, and it would fail on the previous behavior.
  • Cancellation normalization. Dropping the blanket rewrite and rethrowing lets normalizePreparationError see the bare AbortError again and classify it as snapshot_cancelled. The new runtime test asserts the pass-through directly.
  • Portability diagnostics. unsupported_portable_path cleanly separates USTAR-V1-unrepresentable names from symlinks and hard links, on both the policy and the preparer side.

One thing to fix before this can merge — it is mechanical, not a design issue:

  • CI test is red on Check Windows test inventory: "Windows test skip inventory is stale; run npm run windows:inventory:write". The new { skip: process.platform === 'win32' } case needs the inventory regenerated and committed.

Non-blocking observations, fine to leave or fold in while you are here:

  1. details.observed: 1 is a constant. Elsewhere observed means the measured value that breached a limit; here it never varies, so it adds no locating power that the new policyCategory does not already give. Dropping it would keep the field's meaning consistent.
  2. pack consumes the prepared handle, so a caller holding pending_recovery has no in-process way to retry the release — and recover() deliberately skips this process's own lease (session-copy-cleanup.ts:197), so the staging necessarily waits for a restart. The comment is honest about this, but PreparedSessionBundleHandle documents release failures as retryable; a retry() on the cleanup object would restore that.
  3. production-session-snapshot.ts:544-547 runs assertWorkspacePathBudget before policy.classify, so the preparer's own isSessionBundleUstarPathV1 check always wins and the policy's new unsupported_portable_path reject branch is unreachable on this production path (the two also report different codes: unsafe_source vs policy_rejected). Since the coordinator defines the preparer as the policy's enforcement point, the convergent shape is to delete the duplicate check and let the policy decision stand.
  4. The fourth observation from #4361 (collapsing the staging-root-keyed budget Map into a closure variable) is still open — the summary says three, there were four. Entirely optional.

Also worth deciding: this branch and #4361 both start flat from main and touch the same files, so whichever lands first will require the other to rebase.

Review assistance: Claude (Claude Code) diffed this branch against #4361, re-checked the cleanup path, the cancellation normalization, and the preparer/policy ordering against current main, and read the failing CI job; I reviewed and confirmed those findings myself and own this approval.

@Astro-Han

Copy link
Copy Markdown
Contributor

Looking at the three fixes together instead of one at a time — they are the same bug, not three.

All three are the composition layer deciding something a composed part already owns, and every time the downstream branch went dead:

  • release() decided the outcome, but the lease already owns staging recovery.
  • The runtime catch classified the error, so normalizePreparationError's isAbortError branch never ran.
  • assertWorkspacePathBudget classifies the path first, so the policy's unsupported_portable_path branch never runs either.

The first two you fixed by deleting code. The third is still there, and it is in files this PR already touches:

Drop the isSessionBundleUstarPathV1 call at production-session-snapshot.ts:645 and let the policy reject stand — one authority, one code, instead of unsafe_source in one path and policy_rejected in the other. Keep the maxPathBytes / maxPathDepth checks below it: quota is the composition layer's fact, portability is the policy's.

Worth knowing this hits the tests too — the five unsupported_portable_path cases added to quiescent-session-snapshot.test.ts currently assert a branch production never reaches. Removing the duplicate check makes them real.

Sorry for filing these as three separate nits the first time round; the shape only showed up once they were side by side.

Also, CI is still red on Check Windows test inventory — the new skip: process.platform === 'win32' case needs npm run windows:inventory:write.

@MicroGery
MicroGery merged commit 460d982 into apache:main Sep 1, 2026
4 of 5 checks passed
abhinav-phi pushed a commit to abhinav-phi/maka that referenced this pull request Sep 1, 2026
* fix(storage): preserve packed Bundle cleanup outcome

* test: update Windows skip inventory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants