Skip to content

fix(scaling): bound LSR ranking input materialization - #692

Merged
seonghobae merged 3 commits into
mainfrom
replacement/lsr-ranking-bounds-main-20260811
Aug 11, 2026
Merged

fix(scaling): bound LSR ranking input materialization#692
seonghobae merged 3 commits into
mainfrom
replacement/lsr-ranking-bounds-main-20260811

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Why

LSR ranking wrappers previously materialised caller-controlled iterables with unbounded list(ranking) and unbounded outer streams.

What

  • MAX_RANKING_CSR_BYTES live flat/start uint64 budget
  • Per-ranking consume at most n+1 items
  • Redact ordinary iteration failures; preserve process-control exceptions
  • Doctoring + RED/GREEN contract tests

Verification

  • tests/test_scaling_ranking_input_bounds.py (15)
  • tests/test_scaling.py -k rankings

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 66c34c9b-cf1c-46d2-8c5f-a827409159f9

📥 Commits

Reviewing files that changed from the base of the PR and between d2301dc and 3222e3c.

📒 Files selected for processing (5)
  • docs/changelog.d/692-lsr-ranking-input-bounds.md
  • docs/doctoring/lsr_ranking_input_bounds.md
  • docs/superpowers/plans/2026-08-09-lsr-ranking-input-bounds.md
  • python/fast_mlsirm/scaling.py
  • tests/test_scaling_ranking_input_bounds.py

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

Exact bounded GREEN implementation handoff for clean-lineage Draft #692 / issue #612. Immediately before writing, refetch source head and protected main; proceed only while head is exactly 7814561569221789a9ccbcd69da9876add812bf1, main is exactly a7be9eb8ca0b807364173f8471fc300e3cf9350f, and no other writer targets this branch/path. Preserve the fail-first commits; no force-push, temporary workflow, or competing writer.

Current RED is proven on CI run 31455459093, Python job 93668187660: full setup/Rust-primary build succeeded; Rust, package/reinstall/release acceptance, GPU, fuzz, Security and SAST all succeeded; Python completed with exactly 6 failed, 2957 passed, 2 skipped. Failures are precisely unbounded inner list(ranking), unbounded outer consumption, missing 31-byte CSR ceiling enforcement, ordinary outer/inner exception leakage, and the source-level list(ranking) ban. The unrelated marginal _log_sigmoid warning is outside this PR.

Implement the narrowest shared _rankings_to_csr correction only:

  1. Define documented package-owned MAX_RANKING_CSR_BYTES; validate it and n before consuming caller-controlled iterables.
  2. Stream each inner ranking with an explicit n + 1 pull ceiling, rejecting overlength before further consumption; preserve minimum-two-item and item-range semantics.
  3. Bound the combined fixed-width flattened-item and CSR-start payload before growth/allocation. The existing test contract fixes one ranking (0,1) plus starts [0,2] at exactly 32 bytes, so 31 rejects and 32/33 accept. Use checked arithmetic and avoid an unbudgeted second full-size representation where practical.
  4. Remove list(ranking) and retained arbitrary Python-int accumulation in this shared boundary. Normalize ordinary outer/inner iteration failures to stable non-reflective ValueError; preserve KeyboardInterrupt, SystemExit, GeneratorExit unchanged.
  5. Preserve accepted list/tuple/generator byte values and unchanged Rust LSR/I-LSR numerical kernels. Do not widen to _top1_to_csr (security: bound top-1 loser iterables before CSR materialization #632), Rust scaling arithmetic, dependencies/workflows/version/release, or canonical docs.
  6. Run focused tests first to GREEN; then add/complete beginner-readable docstrings, resource/security doctoring and authoritative changelog fragment, render/check managed CHANGELOG.md, prove changed production statement/branch coverage, and run the full exact-head Python/Rust/PyO3/package/GPU/fuzz/Security/SAST gates. Keep Draft until current-head review/governance is clean.

If head/main/writer state moves, stop rather than race.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh current-main reconciliation + GREEN handoff for Draft #692. The prior GREEN instruction was explicitly bound to protected main a7be9eb8... and is stale after protected-main movement. Immediately refetch source head, live main, comments/reviews and scaling.py writer state; proceed only if source remains 7814561569221789a9ccbcd69da9876add812bf1 and live main is still compatible with 56967deceafeb7ecd0ff08a98f196282a6ec81a3. Abort if another writer appears or either ref moves.

Fresh compare is diverged, ahead 2 / behind 151; unique branch paths remain only the fail-first ranking bounds test and plan. Reconcile current protected main non-destructively before production mutation, preserving every accepted scaling/numerical/security change and the RED history as historical evidence.

Then implement only the already-proven shared _rankings_to_csr GREEN: package-owned fixed-width CSR byte budget; at-most-n+1 inner pulls; bounded outer/flat/start consumption before NumPy allocation; remove unbounded list(ranking); stable non-reflective ordinary iteration errors while process-control exceptions propagate; byte/numerical parity for accepted list/tuple/generator inputs into unchanged Rust LSR/I-LSR kernels. Preserve the exact 31/32/33-byte boundary fixture. Do not widen to _top1_to_csr (#632), Rust scaling arithmetic, workflows/dependencies/version/release or canonical #604 docs. Add docs/changelog/coverage and run all same-head gates; keep Draft for final review/merge.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Fresh current-main GREEN handoff for Draft #692 / issue #612. Immediately before writing, refetch exact source head, live protected main, comments/reviews and python/fast_mlsirm/scaling.py; proceed only if source remains 7814561569221789a9ccbcd69da9876add812bf1, protected main remains 56967deceafeb7ecd0ff08a98f196282a6ec81a3, and no other writer targets the branch/path. The earlier handoff bound to a7be9eb... is stale and must not write.

Reconcile protected main non-destructively first, preserving all newer accepted scaling/backend/security behavior and the existing fail-first history. Then implement only the already-proven _rankings_to_csr GREEN: package-owned checked MAX_RANKING_CSR_BYTES; at most n+1 pulls per inner ranking; bounded outer/flat/start fixed-width consumption before NumPy allocation; removal of list(ranking)/equivalent unbounded materialization; stable non-reflective normalization of ordinary caller-controlled outer/inner iteration failures while propagating KeyboardInterrupt/SystemExit/GeneratorExit; and byte/numerical parity for accepted list/tuple/generator inputs into unchanged Rust LSR/I-LSR kernels. Preserve the 31/32/33-byte boundary contract from the RED tests. Do not widen to _top1_to_csr (#632), Rust scaling arithmetic, dependencies/workflows/version/release, or canonical docs.

Run focused RED→GREEN first, then complete docstrings/resource-security doctoring/changelog and full same-head CI/Rust/PyO3/package/GPU/fuzz/Security/SAST evidence. Keep Draft until current-head review/governance is clean. Stop if head/main/writer state moves rather than racing.

Fail closed on overlong rankings, CSR byte budget overruns, and ordinary
iterable failures before Rust handoff, without unbounded list(ranking).
@seonghobae
seonghobae force-pushed the replacement/lsr-ranking-bounds-main-20260811 branch from 7814561 to f33b925 Compare August 11, 2026 08:32
@seonghobae
seonghobae marked this pull request as ready for review August 11, 2026 08:32
@seonghobae seonghobae changed the title test(scaling): replace stale LSR bounds branch on main lineage fix(scaling): bound LSR ranking input materialization Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Exact-current defect found on #692 head 1b7524e8ed17bc9596324154c60ba66ce3c335ea over protected main d2301dccb7d39a794d08cc96ea8c858b9ff49125; exact python/fast_mlsirm/scaling.py blob is 23e9843d695d571087af8d53c68502acb2483e90. Refetch all three plus writer comments immediately before writing and abort if any moves.

The current GREEN does not actually enforce its documented live flat/start uint64 payload budget. _ranking_csr_budget_allows() budgets only logical counts, but growth allocates capacity with max(..., 8, old_capacity * 2). With a tiny monkeypatched ceiling, accepted logical counts can therefore allocate a 64-byte flat capacity and another 64-byte starts capacity even when MAX_RANKING_CSR_BYTES is far smaller. During growth the old and new NumPy buffers coexist, so peak live fixed-width allocation is also unaccounted. np.asarray(ranking_items, dtype=np.uint64) adds another temporary fixed-width allocation, while ranking_items: list[int] remains a separate bounded-but-unaccounted Python-int buffer. The code/docstring currently claims a live-budget guarantee that the implementation does not prove.

Treat this as a valid current-head resource-contract defect and keep #692 unmergeable until RED→GREEN:

  1. Add a tiny-budget allocation regression that instruments/records actual fixed-width allocation capacities, not just final logical counts, and proves every simultaneously live CSR-related fixed-width allocation stays within the declared contract (or explicitly narrow/rename the contract if it is intentionally only a final serialized-payload ceiling; do not call it a live-memory budget).
  2. Replace geometric/minimum-8 growth with a storage strategy whose capacity and growth overlap are budgeted. A zero-copy fixed-width buffer or exact/chunked bounded capacity is acceptable if platform width and transport semantics are test-locked. Do not allocate a new oversized capacity before proving old+new live bytes fit.
  3. Account for/remove the per-ranking np.asarray(ranking_items, dtype=np.uint64) temporary; keep the n+1 consumption cap and ordinary-exception redaction/process-control propagation unchanged.
  4. Preserve byte-identical contiguous np.uint64 Rust inputs and numerical LSR/I-LSR parity. Do not widen to _top1_to_csr (security: bound top-1 loser iterables before CSR materialization #632) or alter Rust ranking arithmetic.
  5. Run focused allocation/resource tests and full exact-head gates. Update doctoring wording so it states exactly what memory is bounded and makes no broader process-memory claim.

Do not merge on the current head even if queued CI becomes green; existing tests do not cover this actual-capacity/overlap mismatch.

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

The live-allocation-budget defect remains on exact current #692 head 3222e3c8f1ccd6f2331c678bc76a5ff4101c54e2 over protected main d2301dccb7d39a794d08cc96ea8c858b9ff49125; exact python/fast_mlsirm/scaling.py blob is still 23e9843d695d571087af8d53c68502acb2483e90. Refetch head/base/blob/comments immediately before writing and abort if anything moves or another writer is active.

The implementation still checks only logical (flat_count + start_count) * 8, then allocates geometric/minimum-8 NumPy capacities and np.asarray(ranking_items) temporaries. It therefore still cannot support its docstring claim that MAX_RANKING_CSR_BYTES bounds live flat/start uint64 payload: actual capacity can exceed the logical budget, and old+new buffers coexist during growth.

Do not merge this head. Add an allocation-capacity/overlap RED under a tiny ceiling, then GREEN with a storage strategy whose simultaneously live fixed-width allocations are provably within the stated limit (or deliberately narrow/rename the contract to a final serialized-payload ceiling and test that exact weaker claim). Remove/account the per-ranking uint64 temporary, preserve n+1 bounded consumption, error redaction/process-control propagation and byte-identical contiguous Rust inputs. Keep _top1_to_csr out of scope (#632). Re-run focused resource/parity and full same-head gates after the exact memory contract is true.

@seonghobae
seonghobae merged commit ad20759 into main Aug 11, 2026
33 checks passed
@seonghobae
seonghobae deleted the replacement/lsr-ranking-bounds-main-20260811 branch August 11, 2026 09:20
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