[xiaoyu-xyz]

Required human contributor answers

1. What outcome are you trying to achieve?

   Today the runtime only queues: SerialScheduler is a semaphore of one that waits and never refuses, so the only bound on a hundred requests is the caller's patience. The outcome is that "refuse when there is no room" exists as a reviewed, available capability — while nothing is wired to it, so today's deployments are unchanged.

2. What is the highest-leverage variable for that outcome?

   Scope discipline: this stays optional and unwired.

   A wider version of this work is what runtime: queue-length limits for the native runtime — scope question #35 was, and it was asked to stay a scope draft. What makes the narrow step mergeable is keeping the unit of change small, per the reviewer's guidance: "optional bounded admission in the existing runtime, with capacity units explicit; the full worker lifecycle can remain separate" (#35 comment).

3. How will this work move that variable rather than create busywork?

   The diff is purely additive: a new module beside SerialScheduler rather than a replacement, nothing constructs it outside its tests, and no worker or frontend file is touched. It cannot regress existing behaviour, and it removes "write your own capacity counter" — a counter whose edges review has already found three times — from every worker that later wants a bound. Readiness, worker-thread ownership and drain on shutdown are deliberately left to the step that wires this up.

4. Who is the customer or user, and what do they actually need?

   Two of them: the next worker author who wires admission into dispatch, and the maintainer who reads queue state.

   • The worker author needs to opt in with a cost of their choosing (tokens, bytes, or just 1), and needs the capacity held for the whole of the work rather than released when the requesting task stops waiting. That is why Permit is owned and can move into spawn_blocking.
   • The maintainer needs queue state published through the existing Report { depth, capacity, rejected } shape, which is already what /health carries.

After answering the four questions: is a PR needed at all to achieve this outcome?

Human decision and rationale (or explicit source reference): Yes. Source: the reviewer's guidance on #35, which named this exact narrow step as the way to start. The alternative is every worker writing its own counter, and this counter's edges — release accounting, clamping, lock poisoning — are what review found three times across this PR's history.

Provenance: these answers are grounded in the contributor's own record (issue #14's roadmap, the #35 scope thread, and this PR's review history) and were reviewed and confirmed by the contributor before being recorded here.

For a bug fix: link actual observed failure evidence.

Evidence: Not applicable — this is not a bug fix. It adds a new optional type and nothing on main constructs it, so there is no observed failure to reproduce. The edges review found while the change was under review are covered by the tests listed under Test Result.

Rebased onto main; #34 has merged, so the stack has collapsed. The diff is only this change's own four files: src/runtime/src/admission.rs, tests/runtime/admission.rs, one line in src/runtime/src/lib.rs, and the [[test]] registration in src/runtime/Cargo.toml — git diff --stat main is 4 files.

Purpose

The first step review pointed at on #35: optional bounded admission in the existing runtime, with capacity units explicit, and the worker lifecycle left out.

SerialScheduler admits one unit at a time and never refuses — a hundred requests queue behind each other and the only bound is the caller's patience. That stays the default. This is what a worker opts into when it wants a bound instead, next to the scheduler rather than in place of it.

Units are the caller's, and they are explicit

Admission counts two things: how many requests are outstanding, and what each one costs.

let admission = BoundedAdmission::new(3, Some(100));   // 3 requests, or 100 units
let permit = admission.try_admit(60)?;                  // this request costs 60

A request costs one; the cost is whatever the caller passes. A prefill worker can admit by token count, a worker with large payloads by bytes, a worker that wants neither by 1. Nothing here guesses which of those a model means — which is why the cost is a parameter rather than a documented convention, and why the type does not pick a unit for anyone.

The permit is the reservation

Capacity is held until the permit drops, so "outstanding" means accepted and not yet finished with rather than "accepted". A worker that hands work to a thread and forgets to release would be bounded forever, so release cannot be something a caller has to remember.

The permit is deliberately neither Clone nor Copy: two permits for one admission would release capacity twice, which is exactly the failure this type exists to prevent. Releasing more than was held panics rather than saturating, so a double release is visible instead of quietly drifting the counter.

Two cases a plain depth < capacity check gets wrong:

• A request costing more than the whole unit ceiling is refused, rather than admitted and overrunning it.
• A zero bound panics at construction. It would admit nothing at all, which is a configuration mistake; failing where it is written beats every request being turned away with no explanation.

The fit check and the reserve use the same saturating arithmetic, so a unit cost near usize::MAX cannot pass one and overflow the other.

report() returns Report { depth, capacity, rejected } — the same shape #34 publishes from /health, so a worker that opts in gets queue state on the health route without new plumbing. rejected here counts refusals from try_admit only; #34's Report documents a second term, work accepted and then dropped because its caller had gone, which an admission bound cannot observe.

Not in this PR

Per the same guidance: worker-thread ownership, readiness transitions, drain on shutdown, and any change to SerialScheduler or to the native workers. This changes no existing behaviour — nothing is bounded until something asks for a bound, and no worker asks today.

Test Plan

cargo fmt --all --check
cargo clippy --workspace --locked --all-targets -- -D warnings
cargo test --workspace --locked

tests/runtime/admission.rs, under the repository tests/ tree per CONTRIBUTING with an explicit [[test]] path:

• a request bound refuses once full, counts the refusal, and admits again once a slot frees
• capacity is held until the permit drops — the property that makes the bound mean anything
• a unit bound admits by cost rather than by count, including that a large request's units come back when it finishes
• one request costing more than the ceiling is refused rather than overrunning it, and one costing exactly the ceiling is admitted
• the request count is reported as capacity
• a zero request bound and a zero unit bound each panic at construction
• clones share one bound, so a queue and its handlers cannot disagree about how much room is left

Test Result

System1-Omni Version / Commit: rebased onto main 3d4ed19; head 017a71b.

Run both locally and in CI on this head. Locally (macOS arm64, Rust 1.99.0): cargo fmt --all --check clean, cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings clean, cargo test --workspace --locked --features omni-laya/serve 190 passed, 0 failed, 28 ignored, cargo build --workspace --release --locked --features omni-laya/serve passes. In CI on 017a71b: rust, build, benchmarks and cuda-contract all success.

The admission suite is also run in release mode (cargo test -p omni-runtime --test admission --release), because one of its regressions is an arithmetic overflow: the same sequence fails differently in each profile, so both are exercised.

The workspace count moved from 107 (at 19c4d80) to 190 mostly because main advanced a long way while this was stacked, not because this change adds tests beyond its own: this PR contributes the 15 in tests/runtime/admission.rs.

No new dependency, and Cargo.lock is unchanged. src/runtime/Cargo.toml gains only the [[test]] target that points at the new test file, which is how this repository registers tests.

Rebased onto main 3d4ed19

The stack has collapsed: #34 is merged, so this branch no longer carries #34's commits. merge-base(main, head) is main itself, and the diff here is only the four files this change owns (+555/-0). The reviewer's "rebase twice" note is therefore complete, and the rejected/413 changes it referenced are on main now rather than arriving with a rebase.

Changes since the review of 19c4d80

• try_admit now computes the sum once with saturating_add and uses it for both the fit check and the reserve. Previously the check saturated while the reserve added directly, so the two could disagree for a unit cost near usize::MAX. This is the reviewer's src/runtime/src/admission.rs:97 finding.
• The rejected doc above now states the term this type can count. The finding it answers (src/runtime/src/engine.rs:41-43) is in a file this PR does not touch, so the doc change itself lives on runtime: add the worker-side engine contract #34 at 095d831; what this PR does is count exactly that term — BoundedAdmission increments rejected only in the refusal branch of try_admit, and never for work it accepted. At this head the rejected field's wording in Report is still the broader one, because the narrowed version arrives with the rebase.
• Not addressed: the src/frontend/src/engine.rs:128 413-vs-malformed-body finding and the serve()/shutdown/Unavailable test gaps. Both are in runtime: add the worker-side engine contract #34's files rather than this change, and runtime: add the worker-side engine contract #34 is frozen at a reviewed head; they are follow-ups once the stack lands.

Demo / evidence

N/A — no runtime behavior applies to this change. Nothing constructs a BoundedAdmission outside its tests, so there is no measurable workload, latency, throughput or memory result for this diff: the library is added and unwired, existing deployments are unchanged, and there is no model, hardware or precision configuration to record. The evidence for this change is the test suite in the Test Plan, and the commands and outcomes are in Test Result above. A serving-level measurement becomes meaningful only once a worker opts in, which is a later step.

Self-review

• I have reviewed the full diff and addressed the issues I found.
• I have checked that the change follows the project's architecture and stays focused on the stated purpose.
• I have run the checks appropriate to this change and reported commands, results, and anything I could not verify above.
• I have checked that the PR description, documentation, and any accuracy or performance claims match the implementation and available evidence.

Two review findings fixed in the second commit

A permit can now be moved into the work it was granted for. It held a &BoundedAdmission, so it could not go into the runtime's own blocking executor — Executor::run in src/runtime/src/lib.rs does spawn_blocking(move || ...) and needs 'static work, and a borrowed permit does not compile there. BoundedAdmission now wraps a shared inner, Permit owns a share of that inner, and try_admit takes &self and hands back something that outlives the handle that issued it.

The counter can no longer be driven negative. The permit stored the units its caller asked for and released that, while the counter had been advanced by a clamped value; where the two differed, a later release underflowed and panicked. The permit now stores the units it actually reserved. The clamping went with it: a request costing more than the ceiling is refused outright, so a reservation is never larger than the budget it was admitted under, and the held total cannot reach usize::MAX by accumulation.

Coverage added for the gaps the review named: concurrent admit/release over 32 threads for 100 rounds asserting the live count never passes the bound and all reservations come back, recovery after a panic while the counters are locked, a permit moved into another thread and released there, and the ceiling cases that used to unbalance the counter.

Still out of scope, deliberately: worker-thread ownership, readiness transitions, and drain on shutdown. There is no admission-plus-transport integration test because nothing wires admission to a worker yet; that arrives with the step that does the wiring.

A third review finding, fixed in the latest commit

With no unit budget configured, the shared unit total was still being charged. try_admit's None branch added the caller's cost to a total that has no ceiling and that nothing reads in that configuration, which bought nothing and could overflow. BoundedAdmission::new(2, None) admitting usize::MAX and then 1 overflowed in a debug build; in a release build it wrapped to zero, and the next release panicked inside Drop, which is a non-unwinding panic that aborts the process.

The total is now advanced only when a unit budget exists, and release subtracts only then, so the bounded path and its exact-once behaviour are unchanged. The reservation keeps two numbers: the caller's cost, which Permit::units() still reports unchanged, and what the total was advanced by. a_request_only_bound_does_not_accumulate_units covers the sequence in both drop orders and asserts the depth returns to zero; it fails on the previous head in both profiles and passes here.

[hsliuustc0106]

Reviewed at head 19c4d8063 (merge base 99865743d; note the PR is behind current main 4a79980d — PR #92 — and will need a rebase). One structural note for reviewers: the GitHub files-changed diff (11 files, +1178) is the stack on open #34 — this head's parent is exactly #34's head 1363d4776. #109's own change is ~+273 in 4 files: src/runtime/src/admission.rs (+158), tests/runtime/admission.rs (+110), and five lines of registration. The src/runtime crate itself (with SerialScheduler) already exists at base, and the runtime/frontend engine.rs modules come from #34.

The admission type itself is clean, well-scoped, and genuinely optional: BoundedAdmission counts 1 outstanding request plus a caller-defined unit cost with dual bounds; Permit is RAII, releases exactly once, and double-release fails loudly via checked_sub(...).expect; poisoned locks are recovered with counters intact; oversize and zero-bound cases are tested. Nothing consumes the new APIs yet — SerialScheduler, the workers, and the frontend forwarding path are untouched, so existing deployments see zero behavior change. /health is correctly not admission-gated; queue-full maps to an immediate 503 and admitted-but-slow to 504, both tested.

P3 findings:

• src/frontend/src/engine.rs:128 — every to_bytes error becomes 413 "request body too large"; axum 0.8 surfaces non-size body failures the same way, so a malformed/aborted body can be mislabeled. Consider separating the length-limit case.
• src/runtime/src/admission.rs:97 — the fit check uses saturating_add (:89) but the reserve uses held.units += units; a caller passing pathological unit values can panic/wrap where the check saturates. Use the same arithmetic in both places.
• src/runtime/src/engine.rs:41-43 — Report.rejected is documented as also counting "accepted and then dropped because its caller had gone", but BoundedAdmission::report can only count try_admit refusals; narrow the doc or mark that term as engine-owned.
• Test gaps (coverage, not defects): serve()/graceful shutdown drain, the Unavailable and "engine stopped" 503 paths, any BoundedAdmission + Engine + transport integration case, and concurrent admit/release are unexercised.

Readiness: draft is appropriate — the body still needs the template's Demo/evidence section and the four self-review checkboxes before leaving draft, and the drain-on-shutdown behavior the docs defer remains unimplemented. CI (rust/build/benchmarks) passes on this head (observed); the author's local macOS counts (107 passed / 10 ignored) were not independently reproduced — no Rust toolchain on this host — and the axum error-conflation nit is asserted from the API shape, not demonstrated.

Provenance: canonical .agents/skills/system1-omni-review/SKILL.md (SHA-256 58be3bc6…dde108) and its repository map read at trusted base 4a79980d8a75, plus src/runtime/README.md, docs/architecture.md, CONTRIBUTING.md. Remote head rechecked before posting.

[xiaoyu-xyz]

Thanks — detailed and useful. Acting and non-acting, per finding.

Fixed, src/runtime/src/admission.rs:97 (head 5c37b54). The mismatch was real. The fit check saturated while the reserve added directly, so a request cost near usize::MAX could pass the check and then overflow on held.units += units — panicking in a debug build. The sum is now computed once and used for both sides:

let sum = held.units.saturating_add(units);
let fits_units = match self.units {
    Some(ceiling) => sum <= ceiling,
    None => true,
};
...
held.units = sum;

I did not add a test for it: reaching the disagreement needs a request cost near usize::MAX, and BoundedAdmission::new panics on a zero bound but accepts a usize::MAX ceiling, so the failing case is constructible but not cheaply assertable. The change is arithmetic consistency rather than a new behaviour, so it is covered by the existing suite rather than by a new case. Say the word if you would rather see the pathological-value test anyway.

Corrected in this PR's description, src/runtime/src/engine.rs:41-43. Report.rejected in #34 keeps its broader wording; the body here now states which term this type can actually count, and that "accepted and then dropped because its caller had gone" is not observable from an admission bound. The doc text itself is #34's file, so I have not edited it under this PR.

Now folded into #34, on your call being overridden by the maintainer's. The src/frontend/src/engine.rs:128 finding is fixed there instead of deferred: to_bytes reports both a body over the limit and a body that could not be read through one error type, so the two are now told apart by the source it wraps (LengthLimitError), which is the check axum's own documentation recommends — 413 stays the oversized answer and a malformed or truncated body becomes 400. Report.rejected's doc is narrowed the same way. Because that moves #34's reviewed head, its recheck at 1363d47 no longer covers the new commit and CI runs again there; I will not claim the new head is verified until those checks report.

The serve()/shutdown/Unavailable/concurrent-admit test gaps are still not addressed. They are coverage rather than defects, and I would rather add them deliberately than pad this change.

On readiness:

• The template's missing sections are now there: ## Demo / evidence (N/A with the reason — nothing constructs BoundedAdmission outside its tests, so there is no workload to measure) and the four self-review checkboxes. Test Result records the head, the commands, and what I could not verify.
• Drain-on-shutdown stays unimplemented and out of scope; the body says so under "Not in this PR", and it will need the transport-side work in runtime: queue-length limits for the native runtime — scope question #35's thread rather than here.
• Rebase is still not possible: runtime: add the worker-side engine contract #34 is still open (merged=false), so replaying onto current main 4a79980 would conflict rather than clean up. I will rebase as soon as runtime: add the worker-side engine contract #34 lands and will re-report then.

One correction to my own earlier note on #34, which this review's CI observation prompted me to check: CI does run on these fork PRs. .github/workflows/ci.yml triggers on pull_request: branches: [main], and this head has 4 check runs — rust, build, benchmarks success, deploy skipped. I had claimed CI never ran here, and that was wrong. On 5c37b54 the rust job — which is exactly cargo fmt --all --check, cargo clippy --workspace --locked --all-targets -- -D warnings, cargo test --workspace --locked — is completed/success, so this head is verified by CI rather than only by my local runs.

Correction, later the same day: I was wrong about the ready-for-review step, and have now marked this ready. I had said I could not, on the grounds that I have only pull on the upstream repository — but the PR author is the fork owner, and GitHub lets the author mark their own PR ready. The actual failure was my tooling (gh pr ready errored), not GitHub's permissions; the GraphQL markPullRequestReadyForReview mutation succeeds. My original sentence is left visible above rather than quietly rewritten.

It still is not rebased: #34 remains open, so replaying onto main 4a79980 conflicts rather than tidying the diff. That is the one remaining step, and it happens when #34 lands.

[hsliuustc0106]

Delta review at head 5c37b544a (one commit since my 2026-10-08 review: 5c37b54, "use the fit check's arithmetic for the reserve"). The in-scope P3 is fixed minimally and correctly: sum = held.units.saturating_add(units) is computed once and used in both the fit check and the reserve, with the doc updated; outstanding += 1 remains bounded by the request ceiling. Of the other three prior P3s, two (the 413-on-any-body-error split and the Report.rejected narrowing) are now fixed on the new #34 head 095d83107 and arrive here with the declared rebase — at this head those files still carry the old code, which is fine given the declared stacking. The body is now complete: Demo/evidence justified N/A and all four self-review boxes filled by the author with honest evidence discipline.

Still outstanding before this leaves draft:

• Rebase twice: the stack parent moved (1363d4776 → 095d83107) and the branch is behind main (4a79980d); this head still builds on the old #34.
• Test coverage (acknowledged by the author as follow-ups): no concurrent admit/release case (Mutex-poisoning recovery also unexercised), no BoundedAdmission + Engine + transport integration case, and the Unavailable/"engine stopped" 503 paths remain untested at this head.
• One wording nit in the body: the "changes since" bullet for the rejected doc reads as if committed in this PR; at this head it isn't — it lives on #34@095d83107.

Observed CI at this head: rust/build/benchmarks all SUCCESS. Not reproduced locally (no Rust toolchain on this host); the prior head's author-reported macOS counts are now stale relative to the arithmetic change.

Provenance: canonical .agents/skills/system1-omni-review/SKILL.md (SHA-256 58be3bc6…dde108) and its repository map at trusted base 4a79980d8a75; remote head rechecked before posting.

[hsliuustc0106]

PR #109 incremental re-review — "runtime: optional bounded admission" (head 6ce1882)

Independent incremental re-review following the 2026-10-08 review of head 19c4d8063/5c37b544a (/tmp/pr-review/body-109.md). Not posted anywhere; local draft only.

1. Scope, stacking and provenance

Pinned refs (both verified in the local repo; p6old/109 fetched this session per the missing-ref fallback):

• old head refs/remotes/p6old/109 = 5c37b544afb5e915bd18828980a7a05d2f701b4b (the 2026-10-08 review head)
• new head refs/remotes/origin/pr/109 = 6ce188297c7baeaf8c42d3c88e610e6a3a492f58
• remote recheck before reporting: git ls-remote origin refs/pull/109/head = 6ce188297c… — matches.
• PR state observed via gh pr view: OPEN, not draft (the task brief said DRAFT; it has since been marked ready for review), mergeable: MERGEABLE, base main@3d4ed19d9, head branch runtime-bounded-admission.

Skill load record: canonical .agents/skills/system1-omni-review/SKILL.md, SHA-256 of bytes read 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108 (matches the pinned value), plus references/repository-contracts.md, docs/architecture.md (delta inspected — it is main's Open-Jev-27B batching / JEV-VL documentation, untouched by this PR's own diff), src/runtime/README.md contract as inlined in src/runtime/src/{admission,engine}.rs docs at head, and CONTRIBUTING.md at head. One repo-level drift note: the review skill and the self-review skill both link CONTRIBUTING.md#large-code-changes, and that section/anchor does not exist in CONTRIBUTING.md at this head — recorded as repository doc drift, not a finding against this PR.

What the incremental delta actually is

git diff --stat p6old/109 origin/pr/109: 219 files, +21888/−416. That number is misleading without the merge structure:

• git merge-base(p6old/109, origin/main) = 1363d47 ("runtime: add the worker-side engine contract") — i.e. the old head was 1363d47 (#34's head) plus exactly the two admission commits.
• Since then main advanced from 1363d47 to 3d4ed19 (merges of #92, #105, #96, #108, #120, #25, #28, #64, #29 CLM, #34, #71), and the two admission commits were rewritten on top of the new main tip.
• git range-diff 1363d47..5c37b544a 3d4ed19..6ce1882 shows 19c4d80 = 6c2a060 and 5c37b54 = 6ce1882 (= = patch-identical). The bounded-admission change I reviewed is byte-for-byte what ships, plus its fix commit.
• Effective PR diff vs merge-base (git diff origin/main origin/pr/109): 4 files, +275/−0 — src/runtime/src/admission.rs (+160), tests/runtime/admission.rs (+110), src/runtime/src/lib.rs (+1), src/runtime/Cargo.toml (+4). Nothing else.
• So essentially the entire +21888 branch delta is merged main content authored by others (twu3202, linear3735, Levius-Fubuki) and by this author in other merged PRs (#29 CLM, #71 Laya) — not authored for #109.

Relationship to PRs #71–#74

• #71 "laya: pre-resolve native execution resources and kernel handles" (codex/laya-execution-plan, head b5550fd) is MERGED into main at 3d4ed19, merged 2026-10-11T05:05:38Z — i.e. after my 2026-10-08 review. Its commits (ebf4e8a "Add Laya CUDA resource ownership" through d235783 "perf(laya): resolve encoder resources and kernel handles at load", authored by linear3735, plus xiaoyu's integration merges by levius) are exactly the "Laya native CUDA series" visible in this delta. #109 does not supersede or subsume #71 — #109 is now stacked on top of #71's merged content.
• #72 (laya: reuse native input staging and group CUDA uploads, df9f4c6069), #73 (cuda: clean up failed and unwound Laya graph captures, 23f2680aa1) and #74 (laya: bound native workspace cache before allocation, f8fb7996de) remain OPEN, and their content is not in #109's branch. They are independent follow-ups hardening the same Laya layer #109 sits on (staging reuse, capture-unwind cleanup, workspace-cache bounds). Their open state is consistent with the merged layer being a first step; nothing in #109 conflicts with them.

2. Prior-finding status — every item re-verified in the tree at 6ce1882

#	Prior finding (2026-10-08)	Status at 6ce1882	Where fixed
1	P3 src/frontend/src/engine.rs:128 — every to_bytes error answered 413	FIXED and verified. Ok(Err(error)) now distinguishes the length limit via StdError::source(&error).is_some_and(|s| s.is::<LengthLimitError>()) → 413; anything else → 400 "request body could not be read". Regression test drives a malformed chunked body over a raw socket and asserts 400 + worker counter 0.	main, commit 095d831 (message credits this review by file:line)
2	P3 src/runtime/src/admission.rs — fit check saturating_add vs reserve held.units += units	FIXED in this PR. try_admit computes let sum = held.units.saturating_add(units); once and uses sum for both the fit check and the reserve (held.units = sum); doc comment states the invariant.	6ce1882 (in #109's diff)
3	P3 src/runtime/src/engine.rs:41-43 — Report.rejected doc overcounts	FIXED and verified. Doc now says the field counts refusals ("a bound counts the requests it turned away…") and explicitly excludes accepted-then-dropped work.	main, same commit 095d831
4	Test gaps: drain/shutdown, 503 paths, integration, concurrency	Partially improved on main; rest explicitly deferred. A a_busy_worker_produces_503_and_is_not_retried transport test exists (tests/frontend/in_process.rs, via #34's merge). Still unexercised at head: concurrent admit/release on BoundedAdmission, serve()/graceful-shutdown drain, admission+Engine+transport integration. The PR body now scopes this out deliberately ("worker-thread ownership, readiness transitions, drain on shutdown… changes no existing behaviour").	partial (main); deferral documented in #109

Residual nit on (2): the saturating-arithmetic unification has no regression test at the actual boundary — the tests cover over-ceiling (101>100) and exact-ceiling (100==100) unit costs, but no unit cost near usize::MAX that would have caught the original mismatch. P3/info; the unified saturating_add makes the panic unreachable, so this is coverage only.

3. #109's own change (+275, 4 files)

Patch-identical to the reviewed stack (range-diff =), with finding (2) fixed as above. Re-affirmed from the tree: BoundedAdmission is opt-in and unwired (nothing constructs it outside its tests; SerialScheduler and all workers untouched), /health is not admission-gated, the permit is RAII/non-Clone with loud double-release (checked_sub(...).expect), poisoned-lock recovery keeps counters, zero bounds panic at construction, and report() feeds the same Report shape #34's health route publishes. The body's claim that rejected counts only try_admit refusals matches the code (the counter increments in exactly one branch).

4. Laya native CUDA series review (merged main content via #71 — findings belong there, not to #109's diff)

Per the task, the series is reviewed against the skill's native/FFI/CUDA contract. Files inspected at head: src/models/laya/src/model.rs (executor, plan, workspace cache), src/backends/cuda/src/lib.rs (Rust bindings), src/backends/cuda/kernels/runtime.cu (device runtime), tests/cuda/laya/{fixture.c,fixture.rs,resolved.rs,README.md}, tests/cuda/laya_runtime.rs, tests/laya/{model,model_dispatch,model_fixture,original_encode,batch}.rs, src/models/laya/src/{artifacts,weights}.rs, recipe/laya/native/VALIDATION.md, src/models/laya/README.md. Note: at head the series' intermediate resident.rs/workspace.rs/encoder.rs modules no longer exist — merge 8f58f15 ("port prepared execution plan onto native worker") folded them into model.rs; the net delta vs p6old is model.rs (+460/−140), cuda lib.rs (+43), and the new tests/docs.

What holds up (contract-by-contract):

• Rust/C ABI lockstep: the Launch type unsafe extern "C" fn(*mut Ptr, i32, i32, i32, Ptr) -> i32 matches the fixture's KERNEL(name, count) macro and runtime.cu's wrappers exactly (args, b, l, rows, stream); laya_linear's 10-parameter signature matches Blas::linear's Linear type parameter-for-parameter (rows, columns, width, gelu). Kernel entry points are resolved once at load (Cuda::resolve, commit d235783) instead of per launch.
• Bounded workspaces: Workspace::new allocates exact-size buffers for a fixed (b, l); infer validates b ≤ 16, l ≤ 512 and l % 16 == 0, b == markers.len().next_power_of_two(), per-row lens/markers bounds, dummy rows zero-length, and rows ≤ 2048 before any launch. The reuse cache is bounded twice over: at most 4 workspaces and at most 512 MiB total (while self.cache.len() >= 4 || sum + s.bytes > 512 MiB), FIFO eviction, and a workspace that fails mid-infer is dropped (freed), never cached. Workspace field order destroys the graph before its referenced allocations, and Model::drop syncs then clears the cache first.
• Residency: all checkpoint tensors are uploaded once at load into HashMap<String, Buffer>; the dtype policy (f16 embeddings, f32 norms/biases/scorer.0, bf16 rest) is pinned by resident_precision_matches_cuda_consumers (206 tensors, exactly 1 f16); LAYA_VERIFY_WEIGHTS=1 does a read-back byte comparison per buffer; validate_names enforces an exact expected/actual tensor-set match before the plan is built, so the later weights[name] indexing cannot panic on a bad checkpoint. Rotary tables are size-checked exactly (data.len() == 512*32*4, "invalid rotary table size") — the 0a349f3 validation is present.
• Stream/device ownership: one non-blocking stream created in laya_init; Cuda/Buffer/Kernel/Graph all hold Rc<Context> (deliberately single-threaded, !Send world, matching "buffers and graphs remain on the owning worker thread"); capture uses cudaStreamCaptureModeThreadLocal; every Drop (Buffer, Graph, Context, Blas, Model) syncs before freeing. resolved.rs proves a resolved Kernel keeps the context (library+stream) alive after the Cuda handle is dropped, and that the stream is freed only with the last owner.
• Capture error recovery: capture() syncs, begins, runs the work, ends; on work error or end error it frees a non-null graph before propagating. The fixture injects exactly the two nasty cases — partial ownership with an error (graph_mode=1: handle set, error returned → Rust frees it) and null-success (graph_mode=2: Ok with null → later replay fails cleanly). Warmup discipline (two eager encodes + sync) precedes capture; dumps are disabled in graphs mode so no readback/sync can occur inside capture; inputs are written before replay.
• Sparse-bundle compatibility (ecfdeaf): specialized attention wrappers (attn_{full,local}_b{1,4}_l512) are optional at resolve; generic shapes fall back to the eager kernel; a missing specialized shape fails with a specific error. missing_specialized_attention_only_rejects_its_shape tests precisely that with an OMIT_SPECIALIZED_ATTN fixture build.
• Dispatch parity: prepared_plan_preserves_original_launch_trace_for_all_shape_selectors freezes the pre-plan eager launch sequence (from 7f39ac4) and requires the prepared plan to reproduce it — kernel names, (b, l, rows) and pointer values — byte-identically, for both rope variants × b∈{1,2,4,8,16} × l∈{16,512}, 242 launches per encode. This is a strong host-side regression oracle for the d235783 "resolve at load" rework.

Findings in the series (all attributable to merged main / #71's integration merges, since #109's diff does not touch these files):

• L1 (P3, merged-main content): ownership/ABI host-test coverage added by the series was then dropped in the merge resolution. ebf4e8a's src/backends/cuda/tests/runtime.rs (254 lines) contained seven tests: incompatible-library load rejection, copy round-trip + bounds, copy errors still synchronize (copy_errors_still_synchronize_and_async_errors_are_reported — the "submission may report failure after copying; Rust still must drain it" case), failed allocations/uploads releasing partial resources, buffers keeping the library/stream alive, operations/drops restoring the device, and a GPU round-trip. At head only the context-retention test survives (as tests/cuda/laya/resolved.rs); the other host-side tests exist nowhere in the tree (name search across the whole head). fixture.c still carries the now-dead injection hooks (copy_mode/test_copy_mode, sync_count/test_syncs, test_allocations, test_live_bytes/test_peak_bytes/test_reset_peak) with no Rust consumer. The dropped drain-after-failed-copy behavior is now asserted nowhere.
• L2 (P3, merged-main content): the series' GPU numerical and reuse tests have no in-tree successor. 5b5518b's encoder reuse check ("Reusing the same allocations must not accumulate residuals across requests" — re-run equality without intermediate synchronization, the numerical invariant this task specifically asks about) and 684470a's distinct-mixed-rows-at-maximum-batch assertion lived in src/models/laya/src/encoder/tests.rs, which was deleted in the 8f58f15 fold. Numerical claims now rest solely on author-run recipe/laya/native/VALIDATION.md (H800, frozen binary/library SHA-256s, "210 responses, 36 hidden-state comparisons and six 17-request reuse sequences, bitwise equal") — good documentation, but not executable in this repository. The fixture README is honest about this ("checks dispatch pointers/order, not numerical outputs").
• L3 (info): no ABI version handshake for the Laya bundle. Cuda::load is unsafe with a documented trust assumption ("matching native ABI… produced by this backend's build tools") and artifacts::validate_bundle hash-checks the bundle against the checkpoint, but unlike the qwen3_5 backend there is no abi_version the library and Rust must agree on; a stale bundle fails only by symbol-miss or misbehaves.
• L4 (info): device/arc pinning. laya_init hardcodes cudaSetDevice(0) and refuses non-Hopper (compute-capability major ≠ 9 → error). The LAYA_CUDA_DEVICE selection added by 0a349f3 was removed during the integration rework (no occurrence at head; README updated consistently), so multi-GPU selection is only via CUDA_VISIBLE_DEVICES, undocumented.
• **L5 (info): launch_rows (the gather/features glue) bypasses the b ≤ 16 / l ≤ 512 / l % 16 shape gate that launch/Kernel::launch enforce; correctness relies on caller-side bounds (rows ≤ 2048, n ≤ 16) in infer.

5. Frontend 400-vs-413 fix (095d831, on main)

Correct, minimal, and it fixes prior finding (1) exactly as recommended:

• Separation uses StdError::source(&error).is::<LengthLimitError>() — the check axum's own documentation prescribes for to_bytes, so only the length-limit case stays 413; a malformed/truncated body is now 400 "request body could not be read". The 504 gateway-timeout path and all client behavior (proxy bypass, no redirects, no retries, 60s deadline) are untouched — this is a pure status-mapping change at the read boundary.
• Cost: one new direct dependency (http-body-util) in src/frontend/Cargo.toml/Cargo.lock.
• Evidence is transport regression evidence, as the skill requires: a_body_that_cannot_be_read_is_not_reported_as_too_large opens a raw TCP socket, sends a chunked body whose chunk size is not hex (a client library will never send this), and asserts a bounded-time 400 with the new message and seen == 0 (the request never became work).
• The same commit narrows Report.rejected's doc (prior finding 3) and cites the review it answers.

6. CI retarget

The net ci.yml change in the delta is cargo clippy/test/build gaining --features omni-laya/serve, so CI compiles the Laya native worker and runs its root-tree tests. It arrived through the Laya stack that merged to main (commits 6ae5e0f, 8f58f15, 39357eb, 28083ea — the "ci: validate retargeted Laya host changes against main" commit 6043294 is itself a merge; author levius), and CONTRIBUTING's check commands at head document the same feature flag. It is not part of #109's diff (ci.yml at head is identical to main), so scope-appropriateness for #109 is moot; for the model addition it belongs to it is appropriate, with the ordinary consequence that the workspace CI surface now includes the laya serve feature.

7. Readiness (CONTRIBUTING)

• Counts, reported separately per the skill: authored diff = 4 files, +275/−0 (well under the 3,000 authored-line threshold; the self-review skill's large-change extras do not apply). Total branch delta vs the pinned old head = 219 files, +21888/−416, of which all but the two admission commits is merged main.
• PR state: OPEN, non-draft, MERGEABLE, base main@3d4ed19. The stack has collapsed (#34 merged), so the GitHub diff is exactly the four owned files.
• Body: complete — Purpose (units explicit, permit-as-reservation rationale), Test Plan (the repository's four documented commands), Test Result (local macOS: fmt/clippy clean, 180 passed/28 ignored — plausible given main's growth, not reproducible on this host; CI jobs cited per name), a "Rebased onto main" section that correctly explains where each prior review finding landed, a reasoned Demo/evidence N/A (the library is added unwired; there is no workload to measure — correct), and all four self-review checkboxes completed. The prior readiness gaps (missing evidence section, unchecked boxes, draft state) are all resolved. Minor: the body's opening line still says "Stacked on #34, which is still open" and cites git diff --stat 1363d47 5c37b54 — stale, and superseded by the body's own later rebase section.

8. Evidence

• Commands personally run: git fetch origin 5c37b544a…:refs/remotes/p6old/109; git rev-parse both pins; git diff --stat (branch and vs-main); git log --graph; git range-diff 1363d47..5c37b544a 3d4ed19..6ce1882; git merge-base/--is-ancestor containment checks for 3d4ed19, 095d831, 998af77, c038268, 7cbce19, 6b51652, de255a7 (all in main); per-commit diff-tree file inventories for the Laya series; git show reads of admission.rs, runtime/src/engine.rs, frontend/src/engine.rs diff, tests/runtime/admission.rs, tests/frontend/in_process.rs diff, 095d831, 6043294, ci.yml delta, model.rs (full), cuda/src/lib.rs (full), kernels/runtime.cu, tests/cuda/laya/*, tests/laya/{model,model_fixture,model_dispatch,original_encode,batch}.rs, artifacts.rs/weights.rs excerpts, VALIDATION.md, CONTRIBUTING.md; tree greps for the dropped tests and LAYA_CUDA_DEVICE; gh pr view 109/71/72/73/74; gh pr checks 109; git ls-remote origin refs/pull/109/head.
• CI observed at 6ce1882 (gh pr checks 109): rust pass (1m52s), build pass (12s), benchmarks pass (13s), deploy skipping — matches the author's Test Result claims.
• Host constraints honored: no cargo invoked (no Rust toolchain), no GPU. Per the skill these are evidence gaps, never PASS.

9. Material unverified checks

• Local Rust suite at 6ce1882 (fmt/clippy/test/build): not run — no toolchain. CI green corroborates but does not reproduce the author's local counts (180 passed / 28 ignored).
• Laya GPU behavior: kernel numerics vs the reference, graph capture/replay on a real device, workspace growth/eviction under real allocation pressure, the "CUDA failure stops this executor; /health returns 503" path — no GPU on this host; entirely unverified here.
• VALIDATION.md's frozen H800 measurements and bitwise-equality claims (pinned binary/library SHA-256s, 210-response/three-way/reuse-sequence checks): author-run evidence, not corroborated.
• The dropped-series tests (L1/L2) were confirmed absent by name search across the whole head tree; I did not exhaustively prove no equivalent coverage exists under different names (the trace oracle and dtype/residency tests do partially overlap the old suite's dispatch and precision intents).
• #72/#73/#74 content (staging reuse, capture unwinding, workspace-cache bounds) not reviewed — open follow-ups against the same merged layer.

10. Verdict

No new actionable findings in #109's own diff. The change remains what the 2026-10-08 review found it to be — a clean, genuinely optional, well-tested admission type — and prior finding (2) is fixed in-head with verified arithmetic; prior findings (1) and (3) are fixed on main by a commit that credits this review and was verified in tree; the remaining test gaps are explicitly and reasonably scoped out. The Laya series in the branch delta is merged main content via #71 (merged 2026-10-11) — #109 sits on top of it rather than subsuming it, and #72–#74 remain open, independent follow-ups. That series carries the report's actionable items, all against main rather than this PR: P3 L1 (ownership/ABI host tests dropped in merge resolution, dead fixture hooks retained) and P3 L2 (GPU numerical-parity and reuse tests deleted without an in-tree successor, leaving author-run VALIDATION.md as the only numerical evidence), plus informational ABI-version (L3), device-pinning (L4) and glue-kernel gate (L5) notes. From #109's own scope the PR is ready for review; the Laya-layer findings should be routed to #71's follow-ups (#72–#74 or the maintainers of the merged series).

[Levius-Fubuki]

Reviewed 6ce188297c7baeaf8c42d3c88e610e6a3a492f58 against base/merge-base 3d4ed19d98a2847241095abb52d11b76c5543214, including all four changed files (+275/-0).

Requesting changes for the two P2 admission issues below: saturation loses the units that permits must later release, and the borrowed-only permit API cannot transfer a reservation into the existing runtime's owned blocking work. Both were independently reproduced against the compiled PR library. The library is currently unwired; these findings do not claim a regression in an existing deployment.

Personally run on macOS arm64 / Rust 1.98.0:

• cargo fmt --all --check: passed.
• cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings: passed.
• cargo test --locked -p omni-runtime: 20 ordinary tests and 2 doctests passed.
• cargo test --workspace --locked --features omni-laya/serve: 181 ordinary tests and 2 doctests passed, 0 failed, 28 ignored.
• cargo build --workspace --release --locked --features omni-laya/serve: passed.
• Additional scoped concurrency probe: 100 rounds, 32 competing threads, capacity 4; exactly four admissions and full release in every round.
• Additional probes reproduced the release panic, the dispatch compile failure (E0521), and premature bounded-capacity release when the borrowed permit is retained in an aborted requesting task instead of dispatched work.

The 28 ignored tests remain unverified. No GPU/checkpoint execution or numerical/performance campaign was run; this PR changes a CPU admission library.

Review provenance: canonical system1-omni-review skill, SHA-256 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108, with its repository-contracts reference, architecture, runtime contract and CONTRIBUTING read. Remote head rechecked before submission.

[Levius-Fubuki]

[P2] Reject overflowing unit totals instead of saturating the reservation

With a valid maximum unit ceiling, saturation makes an over-capacity total pass the fit check and records fewer units than the returned permits own:

let admission = BoundedAdmission::new(2, Some(usize::MAX));
let first = admission.try_admit(usize::MAX).unwrap();
let second = admission.try_admit(1).unwrap(); // accepted beyond the unit ceiling
 drop(first);
 drop(second); // panics at admission.rs:129

I ran this against the compiled PR library: the second drop panics with a permit releases the units it was admitted with. Replacing Some(usize::MAX) with None reproduces the same panic even though that mode promises to bound requests only. Reversing the drop order also leaves the final permit unable to release its original cost.

The earlier arithmetic fix makes the check and reservation agree, but saturation still loses information required by the exact subtraction in release. Please use checked addition and refuse an overflowing total when a unit bound applies, avoid accumulating unnecessary unit accounting in request-only mode, and cover both drop orders with and without a unit ceiling.

[Levius-Fubuki]

[P2] Provide an owned permit that can be retained by dispatched work

Permit borrows the admission handle, so a caller cannot move a successfully reserved permit into the existing SerialScheduler::run closure, which requires Send + 'static:

async fn dispatch(admission: &BoundedAdmission, scheduler: &SerialScheduler) {
    let permit = admission.try_admit(1).unwrap();
    scheduler.run(move || {
        let _permit = permit;
        Ok(())
    }).await.unwrap();
}

Compiling this against the PR's public API fails with E0521 (admission must outlive 'static). The same ownership constraint prevents handing the permit to spawn_blocking or a long-lived worker queue.

Keeping the borrowed permit in the requesting async task instead of the blocking closure compiles, but breaks the outstanding-work contract on cancellation. I gated the dispatched closure, aborted and awaited the caller, and observed report().depth == 0 plus successful admission of a new request while the original blocking work was still gated. This probe demonstrates an integration workaround; no production consumer is wired to this library yet.

Please provide an owned, non-Clone permit retaining the shared Arc state (or an additional owned-admission API), so the reservation can travel with the work and survive caller cancellation. Add a scheduler integration regression that cancels after dispatch and verifies capacity remains held until execution completes; this does not require adding the deferred worker lifecycle.

[xiaoyu-xyz]

Both P2s fixed in 56ce14a. One correction to my own earlier work first, because it is the more useful thing to record: the saturation finding is a defect I introduced. My previous commit replaced held.units += units with a saturating add to answer an earlier P3 about the check and the reserve disagreeing, and in doing so traded a loud overflow for a silent accounting error. The comment I wrote on that commit claimed the two "cannot disagree about a request that is near usize::MAX" — they could, and your reproduction is right.

Saturation losing units

The root cause is that the permit stored the units its caller asked for while the counter had been advanced by a clamped value. Where the two differed, a later release went below zero.

It is fixed by removing the clamp rather than by compensating for it. A request costing more than the ceiling is now refused outright, so a reservation is never larger than the budget it was admitted under, and the held total cannot reach usize::MAX by accumulation. The permit then stores exactly what it reserved, and what goes into the counter is what comes out of it. That also makes the checked_sub panic on release genuinely unreachable through the public API, which it was not before.

With ceiling = usize::MAX: cost 10 is admitted (held = 10), cost usize::MAX is now refused (it used to saturate to MAX and be admitted), and releasing the 10 leaves held = 0.

The regression test a_reservation_is_released_by_what_was_reserved_not_by_what_was_asked fails on the previous head and passes here; the_ceiling_is_the_largest_single_reservation covers the ceiling taken whole.

The borrowed-only permit

You were right, and the evidence is stronger than "an API shape preference": src/runtime/src/lib.rs already does this —

let permit = self.admission.clone().acquire_owned().await…;
tokio::task::spawn_blocking(move || { let _permit = permit; work() })

— so the runtime's own executor already requires an owned permit that can be moved into 'static blocking work. A &BoundedAdmission in the permit could not satisfy that, which is the E0521 you hit.

BoundedAdmission now wraps a shared inner, Permit owns a share of it, and try_admit(&self) hands back something that outlives the issuing handle. No lifetime parameter. The permit holding the reservation for the whole blocking call is also the behaviour I want: it is what keeps the capacity held for the duration of the work rather than released when the requesting task is cancelled.

Coverage

Added, and named so they are easy to find: concurrent_admissions_never_exceed_the_bound_and_all_come_back (32 threads × 100 rounds, asserts the live count never passes the bound and that every reservation comes back), a_poisoned_lock_keeps_admitting (panic while the counters are locked, then admit again), and a_permit_can_be_moved_into_the_work_it_was_granted_for (permit moved into another thread and released there — the shape that did not compile before).

tests/runtime/admission.rs is now 14 tests, up from 8.

Verified on 56ce14a

macOS arm64, Rust 1.99.0, and in CI:

• cargo fmt --all --check — clean
• cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings — clean
• cargo test --workspace --locked --features omni-laya/serve — 189 passed, 0 failed, 28 ignored
• cargo build --workspace --release --locked --features omni-laya/serve — passes
• cargo test -p omni-runtime --test admission — 14 passed, and ten consecutive runs with no failure, since one of them is concurrent
• CI on 56ce14a: rust, build, benchmarks, cuda-contract all pass

Your cargo test --locked -p omni-runtime count of 20 tests plus 2 doctests still holds for that crate.

Still out of scope

Worker-thread ownership, readiness transitions and drain on shutdown, as before. There is no admission-plus-Engine-plus-transport integration test, and I do not want to fake one: nothing constructs a BoundedAdmission outside its tests, so there is no wiring to exercise. That test becomes meaningful in the step that does the wiring, and I would rather add it there than assert against a stand-in that the real worker will not resemble.

[hsliuustc0106]

Incremental re-review at new head 56ce14a374: one new commit since my review at 6ce188297c — "runtime: an owned permit, and a reservation that releases what it took" (admission.rs +tests; the branch's own vs-main delta grows to 4 files +515). The two reviewed commits are patch-identical (git range-diff, named refs, cat-file verified).

The new commit is reviewed and sound — it addresses the test gaps my 2026-10-08 review named, plus two latent seam bugs at this type's edges:

• Owned permit: Permit now holds an Arc share instead of &'a BoundedAdmission, so it can move into 'static spawn_blocking work — which the runtime's executor does; the borrow could not compile there. Deliberately still not Clone/Copy (two permits for one admission remains unrepresentable).
• Counter cannot be driven negative: the permit now stores the units it actually reserved, and oversize requests (units > ceiling) are refused outright rather than clamped — making held.units <= ceiling a structural invariant, so the fit check's subtraction cannot overflow and release's checked_sub guards stay honest. Note this is a documented behavior change: a request costing more than the whole unit ceiling is now counted as rejected where earlier arithmetic would have admitted it at ceiling cost — appropriate for a not-yet-integrated optional type (nothing on main consumes it).
• The three named test gaps are covered: concurrent admit/release over 32 threads × 100 rounds asserting the live count never passes the bound, recovery after a panic while counters are locked, a permit moved across threads and released there, and the ceiling cases that previously unbalanced the counter.

Fork CI at this exact head: benchmarks/build/cuda-contract (and rust) pass. Zero actionable findings in this PR's own diff.

Carried items, unchanged and re-verified in tree: the two Laya-layer P3s from my earlier review attach to the merged main content this branch builds on (fixture.c still carries the six dead injection hooks; the dropped GPU parity/encoder-reuse tests remain absent on main) — they belong to follow-up PRs #72–#74, not this diff. One readiness item keeps this at COMMENTED: the post-#138 template's four human contributor answers are not in the body (the self-review checklist is complete, 4/4). Standing unverified: Rust compile/test for the new commit is CI-corroborated only (no toolchain here); admission concurrency behavior rests on the committed tests, which CI compiles but this host cannot run.

[xiaoyu-xyz]

The readiness item is addressed: the post-#138 template's four required human contributor answers are now at the top of the description, with source references for each — issue #14's roadmap for the outcome, the #35 scope thread for the narrow step, and this PR's own review history for why the counter belongs in one place. The "is a PR needed at all" field records the decision and its source. The bug-fix evidence field is marked not applicable with the reason (this adds a new optional type and nothing on main constructs it, so there is no failure to reproduce).

The answers were grounded in the contributor's own record and confirmed by the contributor before being recorded. The self-review checklist was already complete; nothing else in the description changed.

No code changed with this edit: head is still 56ce14a, and rust, build, benchmarks and cuda-contract all pass there. The two P2 findings from the earlier review are fixed in that commit, and the review recorded against this head found zero actionable items in this PR's own diff.

[Levius-Fubuki]

[P2] Avoid accumulating unit costs when no unit budget is configured

In src/runtime/src/admission.rs:121–136, the None branch still adds the caller’s cost to held.units, although that total has no configured bound.

A minimal trigger is BoundedAdmission::new(2, None), followed by retaining permits from try_admit(usize::MAX) and try_admit(1). Debug builds overflow during admission. Release builds can wrap the total and subsequently panic in release()’s checked subtraction. Admission also increments outstanding before the overflowing operation.

Please skip shared unit-total accounting when the unit budget is absent, while preserving each permit’s original cost. Keep bounded-unit admission and exact-once release behavior unchanged.

Add debug and release regressions covering this sequence, both permit-drop orders, and final depth recovery to zero.

---

Reviewed head: 56ce14a3746f5655ffe85aec8137a1689668d034.

[xiaoyu-xyz]

Fixed in 017a71b. You are right, and the release failure mode is worse than "wrap and then panic": because the panic happens inside Drop while the thread is already unwinding, it is a non-unwinding panic and the process aborts. I reproduced it against the previous head before writing the fix.

Reproduction, before the fix

BoundedAdmission::new(2, None), then retaining permits from try_admit(usize::MAX) and try_admit(1):

• debug: panicked at src/runtime/src/admission.rs:136: attempt to add with overflow
• release (--release, same test): panic in a destructor during cleanup → thread caused non-unwinding panic. aborting. → signal: 6, SIGABRT

So a caller that passes a pathological cost on a request-only bound does not get an error back; it takes the process down. That is why this is worth a P2 rather than a nit.

What changed

The shared unit total is now advanced only when a unit budget exists:

let charged = match self.0.units {
    Some(ceiling) => { /* ceiling checks, unchanged */ units }
    // No unit budget, so there is no total to keep and nothing is charged.
    None => 0,
};
held.outstanding += 1;
held.units += charged;

release subtracts only under the same condition, so the exact-once behaviour on the bounded path is untouched. The reservation keeps two numbers: units, the caller's cost, which Permit::units() still reports unchanged; and charged, what the total was actually advanced by. That way "preserving each permit's original cost" holds even in the configuration that keeps no total.

Regressions

a_request_only_bound_does_not_accumulate_units covers the sequence you gave, both drop orders, and depth recovery:

for reverse in [false, true] {
    let admission = BoundedAdmission::new(2, None);
    let huge = admission.try_admit(usize::MAX).expect("one of two");
    let one = admission.try_admit(1).expect("two of two");
    assert_eq!(admission.report().depth, 2);
    assert_eq!(huge.units(), usize::MAX);   // the caller's cost survives
    assert_eq!(one.units(), 1);
    // drop in either order
    assert_eq!(admission.report().depth, 0);
    assert!(admission.try_admit(usize::MAX).is_some());  // and it admits again
}

Both profiles, since you asked for both: this test fails on the old code in debug with the overflow panic and in release with the SIGABRT above, and passes in both now. cargo test -p omni-runtime --test admission is 15 tests; ten consecutive debug runs and five consecutive release runs had no failure.

Verified on 017a71b

macOS arm64, Rust 1.99.0:

• cargo fmt --all --check — clean
• cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings — clean
• cargo test --workspace --locked --features omni-laya/serve — 190 passed, 0 failed, 28 ignored
• cargo build --workspace --release --locked --features omni-laya/serve — passes
• CI on 017a71b: rust, build, benchmarks, cuda-contract all pass

Nothing else changed: the bounded-unit path, the request bound, report(), and the public shape of BoundedAdmission/Permit are as they were at 56ce14a.

[hsliuustc0106]

COMMENTED at 017a71b37ba564d78ae91eca32293cf0452d0201 (PR head verified: git rev-parse origin/pr/109 and gh pr view 109 --json headRefOid both return this SHA; non-draft, title "runtime: optional bounded admission").

Incremental re-review, one commit since my 2026-10-14 review at 56ce14a3746: 017a71b3 "runtime: keep no unit total when no unit budget is configured" (2 files, +51/−11: src/runtime/src/admission.rs +33/−11, tests/runtime/admission.rs +29). git log --oneline and --first-parent over 56ce14a37..origin/pr/109 both show exactly this one commit — no merges.

What the commit does

Previously, try_admit charged the shared unit total even when no unit budget was configured (None => units), on a total with no ceiling that nothing reads. BoundedAdmission::new(2, None) admitting usize::MAX then 1 overflowed: debug panicked on admission, release wrapped the total to zero and then panicked inside Drop's checked_sub — a non-unwinding panic in a drop path. The fix splits Reservation into units (the caller's cost, what Permit::units() reports) and charged (what the total was advanced by: units when a budget exists, 0 when not), and release subtracts only when self.units.is_some().

Semantics assessment: this is coherent, and it is not a "silent unlimited" hazard. The request bound (outstanding >= requests) is checked before the units match in every configuration, so no admission protection is ever disabled; a caller gets a unit bound only by passing Some(n), and None has never meant a unit limit — there was no ceiling to enforce, only a pointless charge. held.units is private and unobservable through report() (Report { depth, capacity, rejected }), so the change has no caller-visible effect beyond removing the overflow. Admission.units is immutable after construction, so the charge branch and the release guard can never disagree for any permit. The invariant story from my prior reviews holds and is, if anything, tighter: release now subtracts exactly what was added.

The new test a_request_only_bound_does_not_accumulate_units (tests/runtime/admission.rs:307-331) pins the behavior: the exact usize::MAX-then-1 sequence, cost preservation through Permit::units() in both configurations of the drop order loop, both drop orders, depth returning to 0, and re-admission afterwards. By inspection it fails at 56ce14a37 in both profiles (debug: overflow at held.units += units; release: wrap then the Drop panic) — consistent with the author's claim. The suite is 15 tests, +1 versus the previously reviewed head.

Prior invariants re-verified at the new head (source inspected)

• Owned permit: Permit holds an Arc<Admission> share, is not Clone/Copy — unchanged by this commit (src/runtime/src/admission.rs:196-202).
• Counter cannot go negative: oversize requests still refused outright, not clamped (admission.rs:131-136); release still uses checked_sub guards, now under the budget-existence guard and subtracting charged (admission.rs:172-181).
• The test coverage my prior review named is all still present: 32-thread concurrent admit/release (tests/runtime/admission.rs:205), panic recovery while counters are locked (:257), cross-thread move+release (:283), ceiling cases (:75, :125, :159, :185).

Nothing outside the module and its test file constructs BoundedAdmission or calls try_admit (git grep over the head tree) — the type remains optional and unwired, so no caller can be surprised by any of this.

Findings

New — P3 (documentation only): the Reservation struct-level doc now contradicts the implementation and the docs this same commit adds. src/runtime/src/admission.rs:70-73 still says the permit stores "the units it actually reserved rather than the units its caller asked for", that "the two differ only when the request was clamped to the ceiling", and that "storing the clamped value is what makes the counter self-balancing". All three claims are wrong at this head: (a) clamping does not exist — oversize requests are refused outright; (b) the two fields (units/charged) differ exactly when no unit budget is configured (charged = 0, admission.rs:138-141) — the opposite case; (c) balancing comes from charging and releasing the same charged value, or skipping the total entirely. Lines :76 and :78-81 (field docs this commit adds) and :204-208 (Permit::units: "the cost the caller passed, unchanged") state the correct semantics, so the struct doc contradicts correct docs 6 and ~130 lines away. Trigger: any maintainer or future worker author reading the struct. Consequence: misunderstanding of which field is which and of the mechanism the type's safety rests on — in a PR whose whole subject is precise invariant documentation, and whose self-review asserts docs match the implementation. Not new breakage: the stale narrative is byte-identical at 56ce14a37 (where it was already vacuous, since that commit had just removed clamping — my prior review missed it); this commit rewrote the struct and its field docs without fixing the paragraph above them. Fix is one paragraph: units = the caller's admitted cost (what Permit::units reports); charged = what the shared total advanced by, equal to units when a budget exists and zero when not.

Carried, not this PR's responsibility: the two Laya-layer P3s (six dead injection hooks in fixture.c; dropped GPU parity / encoder-reuse tests) attach to follow-ups #72–#74 on merged-main content. This delta touches only src/runtime/src/admission.rs and tests/runtime/admission.rs, so they are out of scope here and I have not re-reviewed them.

Post-#138 template gate — now satisfied

The PR body now contains all four required human contributor answers (outcome; highest-leverage variable; how the work moves it; customer and need), the closing "is a PR needed" human decision with a source reference (the #35 scope-thread comment), and the bug-fix evidence section (marked N/A, correctly — this is not a bug fix). This clears the standing blocker from my 2026-10-14 review.

CI at the exact head (observed)

gh pr checks 109: rust pass (2m36s), build pass (21s), benchmarks pass (33s), cuda-contract pass (8s), deploy skipping. Head SHA verified through the Actions API: CI run 39783937552 and Docs run 39783937539 both completed/success with head_sha 017a71b37ba564d78ae91eca32293cf0452d0201 — the exact PR head. Provenance note: the check URLs render under InferMatrixLab/system1-omni; gh resolves ThinkFlowLab/system1-omni to these same runs (consistent with the fork CI the prior review cited).

Unverified

• No Rust toolchain on this host: compile and test status is author-reported (local macOS arm64 run: fmt/clippy clean, 190 passed) and CI-corroborated only — I cannot execute the new test, and I cannot execute the "fails on the previous head in both profiles" regression claim; that one is verified by code inspection of both heads only.
• The release-mode admission-suite run (cargo test -p omni-runtime --test admission --release) is author-reported; CI runs the default profile.

Verdict

COMMENTED. The change itself is sound — the fix is correct, well-tested, and preserves every invariant prior review established; zero P0–P2 findings; the previously missing template answers are now present. One new P3 (the self-contradictory Reservation doc at src/runtime/src/admission.rs:70-73) keeps this at COMMENTED rather than APPROVE; it is a one-paragraph fix away from approval-worthy.

[psv666]

Reviewed 017a71b37ba564d78ae91eca32293cf0452d0201: no new functional finding; the request-only-budget overflow fix is confirmed.

For BoundedAdmission::new(2, None), the shared unit total is no longer charged, while each permit retains the caller's original cost. I checked bounded-unit admission, checked arithmetic, ownership across dispatch and exact-once release. The new type remains optional and unwired outside the module/tests, so these tests validate the admission library rather than a worker integration.

Personally ran successfully:

• cargo test --locked -p omni-runtime: 27 ordinary tests + 2 doctests.
• cargo test --locked -p omni-runtime --release --test admission: 15 passed, including the previous overflow scenario in release mode.
• cargo test --workspace --locked --features omni-laya/serve: 188 ordinary tests + 2 doctests passed, 28 ignored.
• Workspace fmt, Clippy with -D warnings, and the release build with omni-laya/serve.

The full workspace run used an isolated TMPDIR: its initial CLM test hit another user's pre-existing shared temporary directory and failed with PermissionDenied; rerunning with isolated temporary storage passed without changing that directory. This was an environment collision, not an admission regression.

The existing documentation nit remains at admission.rs:70–73: the Reservation paragraph still describes clamping. Please describe units as the caller's cost and charged as the amount charged to the total (zero with no unit budget), matching the actual fields and implementation. No GPU or model run is needed for this library change; the 28 ignored workspace cases were not executed.

Review scope and provenance

• Reviewed head: 017a71b37ba564d78ae91eca32293cf0452d0201; checked unchanged immediately before publication.
• PR API base (main): 3d4ed19d98a2847241095abb52d11b76c5543214; diff merge-base: 3d4ed19d98a2847241095abb52d11b76c5543214. Validation above used an isolated archive of the reviewed head.
• Authored code/test diff: +551/−0; total text diff: +555/−0 (generated fixtures/docs counted only in total).
• Repository InferMatrixLab/system1-omni (ID 1386454853). Applied .agents/skills/system1-omni-review/SKILL.md from trusted main@5ba6a3ba668cb33028eb0bce2c6d17cdc53aebe7, SHA-256 ae2dfac116376ae2ec41f65122cc1bf5aea10751b8b907198a56154e5d67230e. Loaded references: repository contracts, architecture, CONTRIBUTING, self-review artifact/demo rules, CI/docs workflows and the affected component documentation.

[hsliuustc0106]

Incremental re-review at head 4d39e717e (re-pinned via git rev-parse origin/pr/109 + gh pr view immediately before posting; OPEN, non-draft). Prior review: COMMENTED at 017a71b37 (2026-10-22) with exactly one finding.

Delta

One authored commit, 4d39e717 "docs(runtime): clarify requested and charged reservation units" — 1 file, +4/−4, doc comments only, no code. Own series untouched otherwise (git log --first-parent 017a71b37..origin/pr/109 = this single commit; no merges, no rewrites — prior head is the direct first-parent ancestor, so no range-diff gate needed).

Finding status — the P3 is FIXED

The P3 from my 2026-10-22 review (the Reservation struct doc at src/runtime/src/admission.rs:70-73 still describing the removed clamping behavior, contradicting the correct field docs six lines below) is resolved by this commit exactly as suggested. The new paragraph states, and I re-verified against the implementation re-confirmed in that review: units preserves the caller's original cost as reported by Permit::units; charged records what advanced the shared unit total (the cost when a unit budget is configured, zero for request-only admission); release subtracts exactly charged once; requests exceeding the unit ceiling are refused rather than clamped. All four claims match the code (try_admit's oversize refusal, the charged match on self.0.units, the budget-guarded release). The struct doc now agrees with the field docs and Permit::units — no contradiction remains.

Zero findings

No other content changed in the delta. Prior invariants (owned non-Clone permit, no-negative counter, oversize refusal, full test suite including the overflow regression) are untouched by a doc-only commit and were re-verified at 017a71b37 in the prior pass.

PR state, template, CI

• Non-draft; the four post-#138 human contributor answers remain present in the body (re-checked today).
• CI at the exact head 4d39e717e (observed): CI and Docs runs success in BOTH repos (upstream ThinkFlowLab and fork InferMatrixLab Actions APIs agree on head_sha).

Unverified checks

• No Rust toolchain on this host: compile/test status rests on the green exact-head CI runs. The change is doc-comment-only, and the rust job (fmt/clippy strict/test/build) passes at this head, so even doc-attribute-level breakage is excluded by the observed run.

Verdict

APPROVED — non-draft, zero findings (the single prior P3 fixed by this delta), template answers present, CI green at the exact head. Standing unverified items unchanged in kind: concurrency behavior rests on the committed tests, which CI compiles and runs, not locally executed here; the type remains optional/unwired (nothing on main consumes it).

[hsliuustc0106]

Fresh local re-review at 4d39e717e87a527b2a361c4fbe4707903f8dc55a, base/merge-base 3d4ed19d98a2847241095abb52d11b76c5543214. Complete 4-file diff +555/-0; authored Rust including tests +551, Cargo wiring +4. This is the same head as the prior approval; the finding below comes from an additional test-quality probe, not a new source delta. The head was rechecked before posting.

One P3 test-quality finding: a_poisoned_lock_keeps_admitting panics after try_admit has released its internal mutex guard. It exercises permit cleanup during unwinding, not poison recovery. The inline comment gives the location and correction.

Evidence: in a scratch-only copy, replacing Admission::lock's unwrap_or_else(|error| error.into_inner()) with unwrap() still leaves the unchanged named regression passing (1 passed). A separate public-API probe panics while the mutex's Debug implementation is actually formatting the guarded Held value. That probe passes against the unmodified PR library and fails against the mutation. No production recovery failure was observed.

Agent-executed local validation on Linux x86_64, Rust/Cargo 1.98.1:

• cargo test --locked -p omni-runtime: 27 ordinary tests + 2 doctests passed.
• cargo fmt --all --check: passed.
• cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings: passed.
• cargo test --workspace --locked --features omni-laya/serve: 190 passed including 2 doctests, 0 failed, 28 ignored.
• cargo build --workspace --release --locked --features omni-laya/serve: passed.
• cargo test --locked --release -p omni-runtime --test admission: 15 passed.
• Extra compiled-library probes for actual mutex poisoning, the maximum unit budget, and an owned permit outliving the admission handle and moving to another thread: passed.
• git diff --check: passed.

The type remains optional and unwired at this head. The 28 ignored workspace cases remain unverified; no GPU or checkpoint work was run for this CPU admission change. Same-head CI and Docs succeeded separately.

Verdict: COMMENT for the narrow regression-test gap; no P0–P2 production defect found.

Review provenance: agent-assisted review using .agents/skills/system1-omni-review/SKILL.md at trusted main a893f0cf55135623fb093f401d8ce4713330d801, SHA-256 ae2dfac116376ae2ec41f65122cc1bf5aea10751b8b907198a56154e5d67230e. Architecture, repository-contracts reference, CONTRIBUTING, artifact hygiene and affected CI/component contracts were read. Commands below were executed by the agent; this does not attest personal human execution.

[hsliuustc0106]

[P3] Actually poison the mutex in this regression. try_admit releases its internal MutexGuard before returning the owned Permit, so the subsequent panic only exercises permit cleanup during unwinding. I confirmed that replacing the poison recovery in Admission::lock with unwrap() in a scratch copy still leaves this exact test passing. A separate probe that really panics while holding the guarded value confirms the unmodified implementation recovers correctly. Add actual guard-held poisoning coverage, keeping the test body under root tests/, or relabel this test as unwind cleanup and add the missing poison regression.