Skip to content

test(scaling): prove live CSR allocation budget after ranking merge - #719

Closed
seonghobae wants to merge 1 commit into
mainfrom
test/lsr-live-csr-allocation-red-612
Closed

seonghobae wants to merge 1 commit into
mainfrom
test/lsr-live-csr-allocation-red-612

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Post-integration fail-first correction for #612

Protected main at branch creation and final pre-PR refetch: 640de7dd39d944749b92b1a88bafa8e1787a0847.

PR #692 has merged, but fresh protected-main inspection shows that the accepted implementation still does not prove the memory contract documented by _rankings_to_csr(). Logical (flat_count + start_count) * 8 is bounded, while internal NumPy buffers grow with a minimum/geometric capacity (max(..., 8, old_capacity * 2)) and each ranking is copied through np.asarray(ranking_items, dtype=np.uint64). Therefore actual fixed-width allocations can exceed a tiny declared budget even when final logical CSR payload fits, and the per-ranking conversion introduces an additional unbudgeted uint64 temporary beside live CSR arrays.

This Draft adds only two allocation-boundary REDs:

  • with a 32-byte test ceiling, the one-ranking final payload is exactly 32 bytes but the current internal growth requests a 64-byte uint64 capacity; and
  • accepted rankings must not create a list→uint64 temporary in addition to the live CSR buffers.

These tests instrument actual NumPy allocation/conversion requests and call _rankings_to_csr() directly; setup/import/fixture failure is not valid RED.

Minimum GREEN

  1. Make the documented memory statement true. Prefer a storage strategy whose fixed-width capacity is bounded before allocation and whose growth overlap is included in the contract. If the intended supported contract is deliberately only a final encoded-payload ceiling, rename/narrow the public/internal wording and tests accordingly rather than claiming a live-memory guarantee.
  2. Remove or account for the per-ranking np.asarray(list, dtype=np.uint64) temporary while preserving bounded n + 1 consumption.
  3. Preserve ordinary list/tuple/one-shot-generator acceptance, stable non-reflective iteration errors, process-control propagation, duplicate/out-of-range validation, contiguous uint64 Rust transport and exact LSR/I-LSR numerical results.
  4. Keep Rust ranking/stationary-distribution arithmetic unchanged. _top1_to_csr remains issue security: bound top-1 loser iterables before CSR materialization #632 and is not part of this correction.
  5. Add exact allocation boundary-1/boundary/boundary+1 coverage and doctoring/changelog only after GREEN, then run all same-head Python/Rust/PyO3/package/GPU/fuzz/Security/SAST/current-head review gates.

Do not weaken the resource limit, create a process-wide memory claim, change dependencies/workflows/version/release, or touch canonical #604 docs. Keep Draft through RED→GREEN. This is required before #612 may be closed despite #692's merge.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

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: Pro Plus

Run ID: ad989890-228d-4172-8a04-c543ea410027

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

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.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh exact-current reconciliation + GREEN handoff for issue #612 / Draft #719 after valid RED. Immediately before any write refetch source head, live protected main, comments/reviews and the exact scaling.py blob; proceed only if source remains 04d43ff955062fe7ed6a606c55a9838b1c53895c, protected main is still compatible with 557a065350abcfb963d5d0a010e2ab79b7e9ff47, current python/fast_mlsirm/scaling.py remains blob 23e9843d695d571087af8d53c68502acb2483e90, and no other writer targets this branch/file. Abort on movement.

The branch is 13 protected-main commits behind with exactly one unique fail-first test file. Reconcile current main non-destructively first; preserve RED evidence from CI run 31477831011 / synthetic merge 9304b90eef1a6fed85a3d285cbad63f210e29cc6 as predecessor proof. That full Rust-primary Python 3.12 run completed 2 failed, 3225 passed, 2 skipped and the failures are exactly the intended allocation assertions: internal uint64 capacity requested 64 bytes under a 32-byte ceiling, and two list→uint64 temporary conversions were observed. Rust/package/GPU/fuzz/Security/SAST are GREEN on that predecessor integration. The unrelated marginal overflow warning remains #583/#564.

Implement the minimum GREEN on the reconciled head:

  1. Make the declared resource contract true. Prefer a fixed-width accumulation strategy that never allocates a capacity before proving all simultaneously live CSR-related uint64 buffers (including growth overlap) fit the declared ceiling. If the defensible intended contract is only a final encoded-payload ceiling, narrow/rename the contract and acceptance tests explicitly; do not continue to call it a live payload budget.
  2. Remove or fully account for the np.asarray(ranking_items, dtype=np.uint64) per-ranking temporary. Preserve at-most-n+1 consumption and bounded outer/total work.
  3. Preserve accepted list/tuple/one-shot-generator behavior, non-reflective ordinary iteration errors, process-control propagation, duplicate/out-of-range semantics, contiguous uint64 Rust transport and exact LSR/I-LSR numerical output.
  4. Add boundary-1/boundary/boundary+1 tests that measure actual allocation capacity/overlap, not merely logical counts. Keep Python-int container memory explicitly outside the fixed-width contract unless separately bounded/documented; make no process-wide memory claim.
  5. Update doctoring/changelog wording only after focused GREEN, then run all same-head Python 3.12/3.14, Rust/PyO3/package/GPU/fuzz/Security/SAST/current-head review gates.

Do not widen to _top1_to_csr (#632), alter Rust scaling arithmetic, weaken the ceiling, or touch dependencies/workflows/version/release/canonical #604 docs. Keep Draft until current-head gates/review are complete.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Reacquire the bounded source-writer lease for Draft #719 / issue #612 only if a final refetch still shows source head 04d43ff955062fe7ed6a606c55a9838b1c53895c and protected main 9b7335466c1d61349228d33e7b1e58d279f984c4; abort without writing if either ref moved or another current source writer appeared. The predecessor handoff 5251539764 was explicitly bound to protected main 557a065350abcfb963d5d0a010e2ab79b7e9ff47, instructed itself to abort on movement, has no reaction/receipt, and produced no source movement. Treat it as stale predecessor evidence.

Fresh compare is diverged: this branch has exactly one unique fail-first allocation test path and is 17 protected-main commits behind, merge base 640de7dd39d944749b92b1a88bafa8e1787a0847. Reconcile current main non-destructively first and preserve the valid RED history from CI 31477831011 rather than rewriting it.

The exact RED proved two real resource-contract mismatches: under a 32-byte declared CSR budget the implementation requests a 64-byte uint64 capacity, and accepted rankings create list→uint64 temporaries in addition to live CSR buffers. Implement only the bounded GREEN:

  1. Make the documented memory contract literally true. Bound every simultaneously live fixed-width CSR-related allocation, including capacity growth overlap and any per-ranking conversion storage, before allocation.
  2. Prefer direct bounded writes/streaming into governed storage; remove the unbudgeted np.asarray(list, dtype=np.uint64) temporary. Preserve the n + 1 consumption ceiling.
  3. If engineering evidence shows the intended contract is only the final encoded payload rather than live fixed-width memory, rename/narrow the contract and tests explicitly; never claim a live-memory guarantee while geometric capacity can exceed it.
  4. Preserve list/tuple/one-shot-generator behavior, duplicate/out-of-range checks, stable redacted iteration errors, process-control propagation, contiguous uint64 Rust transport and exact LSR/I-LSR numerical outputs.
  5. Add boundary-1/boundary/boundary+1 allocation tests and accepted-generator parity. Keep _top1_to_csr issue security: bound top-1 loser iterables before CSR materialization #632 out of this slice.
  6. After GREEN add concise resource-contract doctoring/changelog, then run focused scaling/allocation tests, full Python 3.12/3.14, Rust/PyO3/package/reinstall/GPU-existing/fuzz/Security/SAST, changelog check and git diff --check on one unchanged head.

Do not weaken the resource ceiling, make process-wide memory claims, modify ranking/stationary-distribution arithmetic, dependencies/workflows/version/release, or canonical #604 docs. Keep Draft; final exact-head review/Ready/merge returns to the maintainer loop.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh replacement reconciliation/GREEN lease for issue #612 / Draft #719 after protected main movement invalidated the prior exact-base handoff. Immediately before writing refetch source head, live protected main, comments/reviews and the exact python/fast_mlsirm/scaling.py blob; proceed only while source remains 04d43ff955062fe7ed6a606c55a9838b1c53895c, live main remains compatible with 198f236241955b5f707d140d4883ea481a2beca1, and no other writer targets the branch/file. Fresh compare is diverged ahead 1 / behind 22 with exactly one unique fail-first allocation test file. Reconcile protected main non-destructively first; preserve CI 31477831011 and its two intentional allocation REDs as predecessor evidence.

Implement the minimum GREEN already defined by #719/#612: make the declared live fixed-width CSR budget true or explicitly narrow/rename the contract to what is actually guaranteed; do not weaken a live-memory ceiling while retaining stronger wording. Remove or fully budget the per-ranking list→uint64 temporary, include growth/overlap of simultaneously live uint64 buffers, preserve at-most-n+1 inner consumption, outer/total bounds, accepted list/tuple/one-shot generator behavior, non-reflective ordinary iteration errors, process-control propagation, duplicate/out-of-range semantics, contiguous uint64 Rust transport and exact LSR/I-LSR numerical results. Add boundary-1/boundary/boundary+1 allocation evidence and clarify that Python-int container memory is outside the fixed-width contract unless separately bounded.

Keep Rust stationary/ranking arithmetic unchanged and do not widen into _top1_to_csr (#632), dependencies/workflows/version/release or canonical #604 docs. After focused GREEN, update doctoring/changelog, run Python 3.12/3.14, Rust/PyO3/package/GPU/fuzz/Security/SAST, coverage/docstring and current-head review gates on one unchanged head. Stop writing on any source/base/lease movement.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Superseded by GREEN implementation PR (constraint/allocation/uncertainty ownership). Closing RED draft.

@seonghobae seonghobae closed this Aug 11, 2026
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