Repository navigation
fix(graph): bound sealed receipts and refuse unverified cleanup - #4804
ScriptedAlchemy wants to merge 14 commits into
Conversation
Use a 64 KiB capped receipt reader across serving, census and obsolete-format cleanup. Only verified obsolete-format receipts authorize deletion; missing, invalid and oversized receipts remain untouched. Add Rust behavioral fixtures for old-format reclamation and ~70 sparse 220 MiB containers. Refs #4616
|
Only receipts explicitly older than this graph format authorize retirement; a future format is not a superseded format. Exercise both versions in the same real filesystem cleanup regression. Refs #4616
Use the existing no-follow, nonblocking bounded file reader so an oversized, symlinked or nonregular receipt cannot authorize deletion. A real filesystem regression proves an old-format symlinked receipt cannot cause its generation directory to be reclaimed. Refs #4616
A syntactically valid sealed receipt with graph_format=0 is not a valid GraphFormatVersion and must not authorize directory removal. Guard the deletion condition and exercise the preservation path alongside valid retired-format cleanup. Refs #4616
The format-reset integration fixture still expected an artifact without a receipt to be deleted, contradicting this PR's fail-closed ownership rule. Supply an explicit matching older-format receipt for the artifact that is reclaimable and separately assert unverified bytes survive. Retain the current-generation and fresh-store behavior assertions. No production deletion guard or test budget is weakened. Refs #4616
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Reviewed head 86620da5dd6236918916f38660bd96fa805eaf55, including the concurrent format-reset fixture correction, against the PR's narrow scope and #4616 acceptance requirements.
The bounded, no-follow receipt reader and preservation of malformed/zero/future-format receipts are useful partial hardening. The format-reset integration fixture now distinguishes a proven obsolete artifact from an unverified artifact rather than expecting both to be deleted.
I left two inline findings with concrete implementation/test follow-ups: (1) the remaining receipt-validation/pathname-deletion ownership race; (2) the gap between the 70-file sealed-census test and actual bounded storage report coverage. These are remaining #4616 safety/acceptance gaps, not a claim that this PR introduced both problems, and not a request to import work from held draft #4776.
Validation: a disposable Linux filesystem analog reproduced the deletion race; this was NOT repository Rust/Bazel or native macOS execution. Exact-head CI run https://github.com/ScriptedAlchemy/tracedecay/actions/runs/42442665227 was queued when checked. No native test pass, merge readiness, or issue completion is asserted. Required repository checks remain bazel test //crates/tracedecay-graph-db:unit_test //crates/tracedecay-graph-db:graph_db_suite --test_output=errors, plus the eventual reporting-path regression and native macOS coverage; confirm nonzero test execution.
Refs #4616. Keep the issue open until all acceptance requirements are proven on master. This review makes no branch, source, merge, release, infrastructure, or schedule changes.
| continue; | ||
| } | ||
| if receipt.graph_format > 0 && receipt.graph_format < GraphFormatVersion::current().get() { | ||
| match std::fs::remove_dir_all(&path) { |
There was a problem hiding this comment.
[P1, remaining ownership gap] Bind deletion to the directory whose receipt was validated. load_sealed_store_receipt(&path) validates one opened file, but this remove_dir_all(&path) resolves the directory pathname again. Replacing the sealed-root/ancestor pathname after receipt validation can therefore redirect deletion into a foreign directory with the same child name. O_NOFOLLOW on the final sealed.json component and the earlier DirEntry::file_type() do not bind this later operation to the validated directory.
I reproduced that sequence in a disposable Linux filesystem analog: read the genuine receipt through O_NOFOLLOW|O_NONBLOCK, rename the owned sealed root, replace its pathname with a symlink to a foreign root, then recursively remove the previously captured child pathname. The foreign child disappeared while the original owned child survived. This is an OS-level reproduction, not a Rust/Bazel test.
Reuse the existing directory-capability authority, anchored to the store owner's held identity, for receipt reads and recursive removal; preserve/refuse substituted children rather than relying on another pathname precheck. Add a coordinated native regression that swaps the root/ancestor and a real child directory after validation, proves foreign sentinels survive, and separately proves an ordinary verified obsolete generation is reclaimed. This is the pre-existing race acknowledged in the PR body, but it still prevents counting this path as #4616 ownership-safe cleanup.
| .set_len(FILE_BYTES) | ||
| .unwrap(); | ||
| } | ||
| let census = census_sealed_store(&database, &BTreeSet::from(["g69".to_owned()])).unwrap(); |
There was a problem hiding this comment.
Coverage follow-up: this does not exercise the failing storage report journey. The fixture gives the sealed-census helper 70 small receipts, one sparse container per directory, and a manually supplied head set. It does not construct the issue's ~15 GB code-index-v1 tree or invoke the report collector/CLI. The pre-change metadata census can also handle these sparse containers without reading their bodies, so this test alone cannot catch an OOM regression in the actual report path.
There is also no work/entry budget in this census's directory_bytes: its pending-directory vector and hard-link identity set grow with the traversed tree, independently of the new per-receipt 64 KiB cap. That is a remaining bound to address, not evidence that this particular 70-file fixture OOMs.
Keep this focused census test, but add a behavioral test through the actual storage-report production entry point with both sealed generations and code-index data. Cover many small/hard-linked entries as well as large sparse payloads, require correct totals or explicitly partial coverage when the shared traversal budget is exhausted, and verify the selected Bazel test actually runs. Until then, retain the PR's partial-scope wording and do not treat this test as acceptance item 5.
Match two exact hosted Bazel rustfmt diagnostics while retaining zero-format refusal and fail-closed directory preservation coverage. Refs #4616
Directory-byte census followed directory symlinks, charging unrelated payloads to sealed generations and allowing cycles or dangling links to break large-store reporting. Inspect each entry without following its final component; charge a symlink's own metadata, not its target. Extend the 70-generation sparse filesystem regression with an external 20 GiB target and dangling link. The pre-fix census follows or fails on these links, while the corrected census completes without counting them. Refs #4616
|
Refs #4616 — published fec6972 to this existing issue-specific PR via exact expected head Root cause: Behavioral regression: the existing 70-generation (~15 GiB sparse) census fixture now includes a symlink to a foreign directory with a 20 GiB sparse file and a dangling link. It requires the exact 1-head/69-superseded classification and a superseded-byte bound that excludes the foreign 20 GiB payload. The former code fails that scenario; the changed code still measures ordinary file/hard-link bytes. No additional dependencies or CI config changes. Validation: GitHub compare confirms a one-commit fast-forward touching only the graph sealed-store source. No local Bazel/native macOS execution; no Cargo. Fresh exact-head CI #42469034128 is pending. The previous head's hosted Linux fleet did run and pass |
…links The sealed.checked proof-hint reader still used an unrestricted read_to_string even after receipt reads were bounded. A growing or sparse marker could OOM ordinary serving or staging-release checks. Replace this with the shared bounded no-follow regular-file reader; invalid lengths are cache misses and require the normal full proof. Marker persistence also used std::fs::write, which could truncate an unrelated file through a substituted symlink. Stage and atomically replace using the existing private-fs publisher instead. Filesystem regressions cover valid hints, oversized metadata, a sparse 15 GiB hint, and a foreign symlink that cannot supply evidence or be overwritten. Refs #4616
|
Published scoped reviewer follow-up 7a1a7cc: bounded no-follow sealed.checked proof-hint reads and atomic replacement instead of std::fs::write to an attacker-substitutable final symlink. Native filesystem regression checks valid/oversized/15 GiB sparse markers and foreign symlink preservation. Expected-head nonforce publish verified. CI run 42470659905 queued; native Bazel/macOS unverified. Refs #4616. Existing unresolved ancestor/root directory deletion and production-report coverage reviews remain open; this commit does not claim to address those. |
Replace pathname-based obsolete-format cleanup with the shared capability-relative, no-follow recursive removal authority. Bounded receipts are opened and validated beneath held generation directory capabilities; a substituted root or child name after validation cannot redirect deletion. Preserve invalid, absent or unknown receipts, and reject receipt names that traverse outside the held directory. Add filesystem regressions for replacement root symlinks and replacement real child directories, requiring foreign sentinels to survive while validated old directories are retired. Refs #4616
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Refs #4616 — published Root cause addressed (obsolete-format cleanup path only): the prior Two new Unix filesystem regressions coordinate (1) moving the validated root and replacing its pathname with a symlink to a foreign root and (2) moving the validated child and putting an unrelated real directory in the old slot. They require both foreign sentinels to survive and the validated old artifact to be removed. Another regression rejects receipt filenames containing parent traversal. Scope/remaining safety: this fixes the post-validation path re-resolution in obsolete-format cleanup, not the separate ordinary Validation: source and exact diff reviewed; local checkout/Bazel/native macOS execution unavailable. No Rust test pass or merge readiness is claimed. Exact-head CI run #42479619425 was pending when checked. Run |
Replace remaining pathname-based recursive sealed cleanup with no-follow directory capabilities. Durable replay retirement now requires a bounded receipt proving exact namespace, projection, generation, format, and when available the adopted digest. Refuse unknown ownership for retry. Add behavioral Rust regressions for mismatched receipt/digest, verified idempotent reclaim, child real-directory replacement after validation, and a substituted sealed-root symlink. Move Unix imports to module scope. Partial ownership hardening: the ambient store parent still needs a stable owner-identity anchor against pre-acquisition substitution. Refs #4616
|
Refs #4616 — published Source fix: ordinary accepted-replay sealed-generation retirement (both installed-reader and durable tombstone retries) formerly called pathname-based New behavioral Rust regressions: a valid retired generation is physically removed and an idempotent retry succeeds; a wrong-generation receipt and wrong recovered digest preserve a sentinel; a real child directory substituted after receipt validation preserves the replacement and deletes only the held original; a sealed-root symlink substituted before opening cannot redirect the new helpers into a foreign directory. The reviewed inline Unix imports are now module-scoped. No dependencies or CI policy changed. Limits / validation: this is partial ownership hardening. The parent of the sealed root still enters through an ambient path; a replaced symlinked ancestor or foreign real directory before capability acquisition is not bound to the original owner and needs a separate owner-identity repair. Full automatic N>3 retired-generation acceptance is not demonstrated by this commit. No repository Bazel/macOS pass is claimed: the working container has no checkout/Bazel, and no Cargo was invoked. Exact-head CI #42484407257 is queued. Required targets: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A no-follow cleanup failure previously fell through to create_dir_all, which accepts a foreign staging symlink as an existing directory. The subsequent container builder then wrote through the symlink target. Propagate cleanup refusal, preserve a genuine abandoned directory's rebuild path, and use single-directory creation so an existing staged entry cannot be accepted by recursive creation. Add a Unix regression that places a foreign staging symlink and proves no container or receipt is written to the target. Refs #4616
After checking that a foreign staging symlink is preserved and no foreign payload is written, remove only the link and build the same generation successfully. This proves the regression is not satisfied by unconditionally refusing all sealed builds. Refs #4616
|
Refs #4616 — published and independently checked two focused commits directly to this issue-specific PR: b0fa1675 and 7ab6c2b9. Both were fast-forwarded with exact expected-old-head, Root cause: In Repair: Create the root if absent, require the existing no-follow child-removal authority to succeed (no suppressed symlink refusal), then use nonrecursive Validation status: independent source/diff review and exact-head remote verification; not a Bazel/native macOS test pass. This environment has no repository checkout or Bazel; no Cargo was invoked. Run Limits: this addresses the pre-existing staging symlink case identified by review. It does not anchor the store-parent identity before opening, nor prevent a hostile rename/replacement after staging creation while the path-based container writer runs. Those require separate ownership-safe staging I/O design and native regressions. Nor does it prove N>3 automatic sealed retirement, text-artifact isolation, sustained writer progress, Doctor guidance or full report memory acceptance on master. Keep #4616 OPEN and #4776 DRAFT/unmerged. No release #4763, infrastructure, tags, credentials, CI-gate or schedules changed. |
|
Refs #4616 — superseded by clean-history PR #4819, commit |
Root cause and scope
Refs #4616 — partial OOM safety and ownership-fail-closed slice, not complete sealed-generation retirement.
The default-branch sealed store has three unrestricted
std::fs::read(sealed.json)paths: direct serving, the read-only sealed census, and ordinary receipt loading. A sufficiently large or concurrently growing receipt can allocate unbounded heap even though a receipt is only metadata. Worse, obsolete-format cleanup interprets any absent, unreadable or malformed receipt as "noncurrent" and recursively deletes its directory. Unknown bytes do not establish deletion ownership.Focused change
tracedecay-private-fs::framed_log::read_boundedauthority: a held no-follow, nonblocking file handle with regular-file/type checking, 64 KiB limit, and changed-length refusal. This also rejects symlinked receipts and avoids indefinitely blocking on substituted FIFOs. No dependencies, contracts or build configuration changed.Review, validation and boundaries
Current issue-specific head:
c99173e0d2790aa7f5495dd3873571b9dfa4c00e; initial three source commits remain in history, followed by two focused review corrections. Exact-head CI: https://github.com/ScriptedAlchemy/tracedecay/actions/runs/42440436843 (queued at update).Exact one-file source commit
71a2379f1d38e78759f8be1076989518fec2f444with Unix-only test-import follow-up7fb3c92655601ab655826355f2a226056fab2dd4and future-format safety follow-upa1b105f08e6d44eb7e4dd1b7420267500d2d2477, parentmaster91c72aba7750a398dd28e83d64bf0c6d6fef4172; branch creation and update used the exact expected old head withforce=false. Review fixes:a39ffd186fixes the failing rustfmt CI gate;c99173e0dreplaces the reader with the shared no-follow/nonblocking authority and adds a falsifiable symlinked-receipt deletion regression. Both were published to the exact old head with nonforce fast-forwards, preserving peer changes. No native Bazel/Rust test pass is claimed yet. Runbazel test //crates/tracedecay-graph-db:unit_test //crates/tracedecay-graph-db:graph_db_suite --test_output=errors, CI format/clippy, and native macOS. Confirm a nonzero test count.This is not a solution for ancestor/root real-directory substitution, path-based recursive deletion TOCTOU, daemon retention scheduling under continuous writers, or proof that
tracedecay storage reportitself completes on a 70-generation/15-GB production store. Keep #4616 open until all acceptance criteria are proven onmaster. Do not merge or auto-merge mixed draft #4776 or Kev/decision-model PRs. Release #4763 and production/infra untouched.