Repository navigation
fix(storage): bound GC receipt capture to original descriptors - #6500
Conversation
Scalar preallocation checks missed original link identity and complete named metadata fences. Keep admission, bounded reads and cleanup on the retained descriptor so rejected captures cannot expand allocation or replace errors. Lore-id: 706deaf2 Constraint: stored quotas do not establish invocation budgets or quiescence Constraint: preserve original descriptor and scope generation fences Confidence: high Scope-risk: bounded Tested: 420 tests across 13 GC and storage suites, 3765 assertions; agent checks Not-tested: Windows and Darwin native execution
|
Architect source advisory 163: CLEAR for exact head c474cb0 against dev 7ac5842; 76 production lines, no discounts. Original descriptor size/link identity is admitted before allocation; retained ranges are bounded to admitted size, named generation fences remain, and primary capture errors survive secondary cleanup errors. Existing genuine leased, readonly, discovery/bootstrap, protocol and publisher consumers use the same capture path. Prepared/absence/populated fences and stored quotas remain. Cleanup is attempted on failure; not every success-path final metadata check executes after an earlier error. This advisory is source-only, not runtime/platform/write approval, quiescence, reclamation or full 46/35/40 operation-budget closure. |
|
Joined bounded QA advisory 165: CLEAR for exact head c474cb0. Full-index diff independently anchors storage test c0f114fef75ae5b59d7de28c4daeba2e0f94a617 and GC journal test 206d4d938ca9f42ff5a7ed8a2aa493d3ea6e7251; source/doc blobs match 163. Parent full GC plus storage union: 420 pass, 30 platform skips, 0 fail, 3765 assertions/13 files/93.84s; vendor/Biome/types/diff/docs-index/inventory pass with one existing large-file warning. Genuine leases, no-follow descriptors, scope locks, original metadata, physical 64 MiB/512 MiB histories and precodec/prepublication refusal are covered. Valid maxBytes=0 phase refusals with unchanged actual files/fstat identities and healthy default controls prove per-receipt propagation only, not aggregate invocation/traversal/read/hash budgets. Authentic interrupted partials and observer/type reds are preserved. Linux native49 addon source-proven reuse is not a fresh build or Windows/Darwin runtime proof. Full Task stack remains incomplete. Exact-head write-access maintainer approval is required; no autonomous merge. |
probepark
left a comment
There was a problem hiding this comment.
Code review (CI still running on c474cb0; approval decision follows once CI finishes).
Review (head c474cb0, gajae-reviewer on behalf of probepark)
Blocking: None found in the production diff.
ocr: blocking 0 / nit 0
CI: Approval held. gajae-approve-gate.py 6500 --head c474cb0feb4b1dd77c43f379bf60b8f001bf9b40 returned HOLD_CI at 2026-10-08 05:13 UTC. Virtual integration validation remains pending. The coding-agent check, storage test, GC retirement-journal test, native build, and affected-path aggregate passed in run 37729403833. The gate reports no missing local command. These observations apply to this exact head; no local build or test was run here.
Scope: +1006 / -267, six files. OCR selects two production files, totaling 76 changed lines. The remainder comprises two regression-test files, session documentation, and a release-note fragment.
Conventions: The package changelog fragment is present, as AGENTS.md requires. No released changelog section or declared generated file changed. No labels.
Notable:
packages/coding-agent/src/session/internal/managed-session-storage.ts:3026-3069— The retained lease opens before admission and allocation. Admission, allocation, reading, and hashing failures all reach lease cleanup. A secondary close failure does not replace the primary failure.packages/coding-agent/src/session/internal/managed-session-storage.ts:2809-2811,2857,2917— Named-file size and nanosecond timestamps join the generation fence. Both lease branches reject reads beyond the original size.packages/coding-agent/src/session/internal/managed-session-storage.ts:3745-3779— Path-backed capture admits the opened descriptor, checks descriptor and named generations, and attempts close on success or failure.packages/coding-agent/src/session/internal/managed-session-scope.ts:2975-2989— The existing receipt reader forwards descriptor identity to stored-history admission before buffer allocation. Unique-history accounting remains unchanged.
Claim coverage: The PR's descriptor-bounded capture claim matches the inspected production path. Regression cases cover admission failure, growth, replacement, original range limits, and physical receipt-history boundaries. The PR explains the replacement of simulated quota tests with physical fixtures. I checked test names and changed cleanup hooks, not every excluded test body. Author advisory comments are supplemental reports, not independent execution evidence.
Checked and clean: Production error precedence, admission order, range fences, generation checks, release-note fragment, and git diff --check (exit 0).
Not assessed: Pending virtual integration; independent local runtime execution; complete excluded-test bodies; platform behavior beyond the reported CI coverage. Operation-wide work budgets and writer quiescence are explicitly outside this PR's claim.
Decision: COMMENT only. No APPROVE while CI is pending. PR body remains unchanged; it contains no verdict line.
probepark
left a comment
There was a problem hiding this comment.
Code review follow-up (head c474cb0, gajae-reviewer on behalf of probepark)
CI: All required checks are complete and passing; gajae-approve-gate.py returned ALLOW for this exact head.
Scope: +1006 / -267, 6 files — storage/session capture and regression coverage.
Conventions: Release-note fragment present; no declared generated files changed.
Notable:
- The prior exact-head code review found no blocking production defects; this is the gate-only follow-up after CI completion.
- The prior review identified descriptor admission, range/generation fences, cleanup precedence, and regression coverage as clean.
blocking: None.
ocr: blocking 0 / nit 0
Verdict: APPROVE. The exact-head approval gate is satisfied.
Independently functional prerequisite
Bound retained file reads to their originally admitted no-follow descriptor. Admission receives the actual size/link identity before allocation; range leases cannot expand past that descriptor, and named size/mtimeNs/ctimeNs fences preserve accepted-capture final generation checks. Admission/read failures attempt real lease cleanup without replacing the original error with cleanup failure; secondary close diagnostics are not separately surfaced by these helpers.
The existing leased, readonly, nested discovery/bootstrap and protocol GC receipt consumers use this reader directly. No dormant API or alternate scalar-callback path is introduced. Stored 64 MiB receipt, 50,000-state and 512 MiB unique-history quotas and prepared/absence/populated-scope fences remain intact.
Review cost and verification
Production cost: 76 added+deleted lines across two files against dev 7ac5842, no source/native discounts. Exact committed head: c474cb0.
Fresh bounded source advisory 163 and joined QA advisory 165 are CLEAR and rebound to this exact committed head. Permitted full-index diff independently binds the two test blobs c0f114fef75ae5b59d7de28c4daeba2e0f94a617 and 206d4d938ca9f42ff5a7ed8a2aa493d3ea6e7251. The guard stopped before commit on the dev advance; both complete Git trees were equal, so the fast-forward changed ancestry only and preserved every tested/reviewed blob. Neither advisory is maintainer approval.
Explicit remaining boundaries
Stored quota is not an invocation budget. Phase-specific per-receipt capacity refusals prove error propagation, not aggregate 512 MiB or operation-wide traversal/read/inspection admission. Full readonly inspection closure and full-writer future physical-work reservations remain separate protected prerequisites (46 and 35/40). This change grants no private owner authority, FD revocation, quiescence, deletion permission or committed Task admission.
Interrupted fixture partials, authentic six-regression failure, allocation-observer failure after mock restoration, runtime-green/type-red overload errors and their repairs are preserved. No fabricated descriptor, scalar compatibility fallback, positive-only phase-coverage waiver or suppressed warning is used. The original 44-file reference and unfinished stack remain preserved; full-goal completion is not claimed.
No autonomous merge: exact-current-head approving review by a write-access maintainer is required.