Skip to content

fix(storage): bound adjacency merge reader buffers (#1278) - #1281

Merged
DecisionNerd merged 6 commits into
mainfrom
fix/1278-query-rss-diagnosis
Sep 15, 2026
Merged

DecisionNerd merged 6 commits into
mainfrom
fix/1278-query-rss-diagnosis

Conversation

@DecisionNerd

@DecisionNerd DecisionNerd commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Query-time adjacency rebuilds allocated 1 MiB per merge reader, growing from 16 to 64 MiB at the default S20/S22 run counts. This change shares one aggregate 1 MiB budget across each merge, preserving decoding, ordering, fan-in, spill limits and publication behavior. Regression tests verify actual reader capacities and exact merged records through refill and 65-run compaction, including tiny/zero buffers and truncation rejection.

Comparable native S20→S22 process RSS is 182,755,328 → 193,495,040 bytes (5.88%), versus the preserved historical 31.59%. The accepted S18/S19/S20/S22 prefix passes all nine unchanged full S24 admission checks. A fresh replay after integrating the retention prerequisite also passes, using newly measured capacity and the unchanged reserve. No S24 workload ran; original measurements retain their original executable/source identities.

Acceptance evidence:

  • Preserved original refusal and phase attribution; paired Heaptrack profiles show exactly 1 MiB of live merge-reader buffers across S16/S17/S18.
  • Deterministic Rust allocation/merge regressions plus 135 real-facade lifecycle/query commands and 72 independent query-oracle comparisons, including reopen, full portable verification and clean import.
  • Comparable native prefix, exact source/imported counts, receipt hashes, measurement limitations and fresh admission replay are retained in docs/development/evidence/query-rss-1278-repair.md and its linked artifacts.
  • The separate retention-lock defect is fixed in merged PR fix(storage): release recovery writer locks on error exits (#1283) #1284 (issue fix(storage): release retention writer locks on bounded-error exits #1283 closed), with deterministic failing-before/passing-after tests, 15 retention and 29 recovery tests, and passing exact-head CI including the normal parallel Bazel storage aggregate. Historical failed CI and local validation attempts remain recorded.

Final exact-head CI Gate passes at 5cbeb91ff58d79e4b363f77bf416a33c50322b2e; GitHub reports CLEAN, no review threads, and closing reference exactly #1278. Independent review found no actionable findings. Final Clippy, formatting, fast checks, 1,113 storage tests, API/binding acceptance and host/admission tests pass.

Full local validation fails coverage floors: core Rust 91.33% < 95%, CLI 79.34% < 80%, Node wrapper 80.32% < 85%. Changed Rust lines are 9/9 covered; both Rust adapters and Python wrapper pass. The final valid coverage ledger retains the exact source identity. Earlier dirty-tree and stale-profile attempts remain archived; the detailed validation comment records recovery and every result. No threshold is changed. The maintainer explicitly instructed merging despite these documented local coverage failures and deferred coverage repair. The full local gate remains recorded as failed; exact-head CI Gate passes.

Closes #1278.

Note

Bound adjacency merge reader buffers with shared 1 MB budget

  • Replaces per-run fixed-size buffer allocation in merge_keyed_runs with a shared MERGE_READER_BUFFER_BYTES constant (1 MB) divided across all concurrently opened run cursors, including compaction passes.
  • open_run_cursors divides the budget by run count without rounding up; when fan-in exceeds the budget, individual readers get zero-capacity (unbuffered) reads.
  • RunCursor::open now accepts a caller-provided buffer size instead of selecting a fixed capacity internally.
  • Adds tests in tests.rs covering fan-in boundaries (63/64/65 runs), zero/small buffer decoding, and truncation rejection.
  • Adds diagnostic scripts (rss_1278.py, heaptrack_summary_1278.py) and evidence artifacts documenting the RSS investigation and repair validation.
  • Risk: large fan-in merges now use smaller or zero-capacity per-reader buffers, which may increase I/O syscall counts for workloads that previously benefited from the fixed-size buffer; check open_run_cursors when fan-in approaches or exceeds 1 MB worth of readers.

Macroscope summarized 5cbeb91.

@coderabbitai

coderabbitai Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 84dfe244-3451-4432-a7af-ebad1301ab5d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added core Core source code changes documentation Improvements or additions to documentation labels Sep 15, 2026
@blacksmith-sh

This comment has been minimized.

@DecisionNerd

Copy link
Copy Markdown
Contributor Author

Final validation at 5cbeb91ff58d79e4b363f77bf416a33c50322b2e (source tree clean throughout):

  • Exact-head CI run 34923250885 passed, including CI Gate, normal parallel Bazel storage testing, Python/Node bindings, Windows locks, macOS durability and concurrency. GitHub reports CLEAN; review threads are empty; closing references contain exactly fix(scale): resolve S20–S22 process RSS growth blocking S24 admission #1278.
  • Independent review found no actionable findings. The measured adjacency implementation/tests remain identical to measured source 6cbfde…; all 23 receipt hashes remain unchanged. Integrated admission replay passes all nine checks; no native remeasurement or S24 workload was performed.
  • cargo fmt --all -- --check, workspace Clippy with -D warnings, make pre-push-fast, and make gate-registry-check passed (14 registry tests).
  • Final instrumented Rust run passed, including 1,113 storage tests (two existing ignored tests), 118 API BDD scenarios, lifecycle/reopen/import checks, and the new retention regressions.
  • Real binding acceptance passed: 241 Python tests, 277 Node tests, 126 Node BDD scenarios and native smoke. PYTHONPATH=harness .venv/bin/python -m unittest tests.test_progressive_host_run tests.test_local_admission -v passed 20 tests from benchmarks/. An initial pytest invocation failed because pytest is absent in that benchmark environment; both logs are retained.
  • uv run --no-sync python scripts/ci/test-api-bdd-mutations.py rejected all five fail-closed mutations.

The full local gate failed; this is not a full validation pass. make pre-push produced a valid SHA-bound coverage ledger and rejected core Rust coverage. The complete independent threshold census is:

Surface Result Unchanged floor
Core Rust 168,092 / 184,044 = 91.33% 95% — FAIL
CLI Rust crate 2,853 / 3,596 = 79.34% 80% — FAIL
Changed Rust lines 9 / 9 = 100% 90% — PASS
Python Rust adapter 80.97% 80% — PASS
Node Rust adapter 80.92% 80% — PASS
Python wrapper (make coverage-python) 96.97%; 110 tests passed 85% — PASS
Node wrapper (make coverage-node) 80.32%; tests passed 85% — FAIL

The CLI and Node wrapper source is outside this diff. The core result is the measured final-tree total; no same-head comparison with a separately measured main baseline is claimed. No floor has been lowered or waived.

Local durable tests used the previously documented private ext4 /tmp namespace and parent-only storage serialization for the local counter limitation. Authoritative CI retained normal parallel storage testing. Native profile output from separate wrapper checks was isolated from the Rust ledger.

The first clean-tree coverage attempt failed provenance validation: resume mode retained 84 older Python and 95 older Node profiles from the prior source alongside new profiles. The entire failed report was archived. Both complete profile populations and their acceptance stamps were moved out; COVERAGE_RUST_RESUME=1 make pre-push then reused the verified same-commit core report and native artifacts and reran both acceptance phases into an empty profile directory. This produced the valid ledger above with exactly 84 fresh Python and 95 fresh Node profiles. No selective profile removal, timestamp rewriting or threshold override was used. Earlier dirty-tree and CI failures remain retained.

The retention prerequisite was resolved independently in merged #1284; #1283 is verified closed. #1281 remains unmerged pending disposition of the observed local coverage failures, as local floors remain required by docs/development/codecov-integration.md. All logs, reports, stamps and failed attempts are retained outside tracked changes.

@DecisionNerd
DecisionNerd marked this pull request as ready for review September 15, 2026 04:05
@DecisionNerd
DecisionNerd merged commit a07bf2d into main Sep 15, 2026
23 checks passed
@DecisionNerd
DecisionNerd deleted the fix/1278-query-rss-diagnosis branch September 15, 2026 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core source code changes documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(scale): resolve S20–S22 process RSS growth blocking S24 admission

1 participant