Skip to content

perf(retention): stop metadata scans at complete manifests - #4714

Closed
ScriptedAlchemy wants to merge 2 commits into
masterfrom
perf/retention-bounded-metadata-prefix
Closed

ScriptedAlchemy wants to merge 2 commits into
masterfrom
perf/retention-bounded-metadata-prefix

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Nov 3, 2026 •

Copy link
Copy Markdown
Owner

Problem

The storage-report/retention metadata scanner eagerly reserved the entire 16 MiB prefix and allocated a 1 MiB read buffer. Even MetadataOnly continued reading until EOF or the prefix ceiling, although a complete manifest and format revision could be available near the beginning. This is avoidable allocation and payload I/O in the read-only report path.

Change

  • Start with a 4 KiB prefix and stop metadata reads when both required fields are complete. Grow only when necessary, keeping the existing 16 MiB refusal bound.
  • Probe the growing prefix at geometric boundaries so repeated parsing remains linear in the prefix length rather than rescanning after every small read.
  • Release the prefix allocation before full-payload verification. Full continues hashing from the exact position of the same open file and still includes every byte; MetadataOnly remains non-authoritative for deletion.
  • Preserve typed cancellation and malformed/over-bound metadata failures. The separate bounded format probe now handles short reads through Read::take/read_to_end.
  • Require a complete JSON integer and delimiter before accepting a revision, so an incremental prefix cannot accept a partial, fractional, exponent, leading-zero, negative or overflowing revision.

No retention eligibility, locks, reader protection, destructive execution, file format, reporting schema, or request budget changes.

Regression coverage

Seven added tests exercise actual temporary generation files and the production scanner: early metadata stopping with an observed file position, complete hashing and tail mutation detection, multi-chunk manifests, the unchanged prefix ceiling, malformed manifest refusals in both modes, cancellation before/after I/O, and complete integer decoding. The end-to-end scanner result retains the original file size and expected digest semantics.

Validation and limits

Reviewed the report call path, metadata-only versus full-verification distinction, shared fixture shape, and complete GitHub-generated production diff. Staged-source whitespace/final-newline checks passed, and its Git blob hash exactly matches the published source (706bb2e3035f29612fe612970d30bdbe4a6f4d11).

Rust compilation, rustfmt, Clippy and the seven regressions have not run locally: this environment has no supplied checkout or Rust/Bazel toolchain. No Cargo commands were run. Hosted CI must compile and execute the affected code-index-retention and maintenance/report suites before merge.

Refs #4616, specifically the storage-report resource-use portion of acceptance item 5. This does not establish the cause of the reported OOM or prove completion on the reported 70-generation/15-GB store. Sealed-generation GC, unsafe-artifact isolation, retention fairness and reclaim guidance remain open. No production RSS or elapsed-time improvement is claimed.


Devin Review

@changeset-bot

changeset-bot Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 96a94e8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@devin-ai-integration devin-ai-integration Bot 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.

🔍 Devin Review: 1 flag

Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

@ScriptedAlchemy

Copy link
Copy Markdown
Owner Author

Consolidated into #4742 (integrate/open-pull-requests) at the captured head shown in its description, as requested. Continue review and further fixes on #4742. This closure does not assert CI readiness; the replacement remains draft. Source branch retained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant