feat(debug-trace-server): inbound admission control and response-size caps by flyq · Pull Request #188 · megaeth-labs/stateless-validator

flyq · 2026-08-21T16:11:06Z

Summary

Adds inbound admission control to debug-trace-server, plus the response-size caps it depends on. Requests beyond a configured capacity budget are now refused immediately with -32013 "Request queue is full" — byte-identical to mega-reth's ConcurrencyLimiter contract, so whatever already backs off for the node backs off for this server. Lands backlog items P0-0 PR③ and P1-1 (TODO-H).

Root cause

Nothing bounded what clients could ask of this process. Every existing concurrency cap is outbound — the witness, data and R2 semaphores limit what we ask of others, and they are unbounded waits, so overflow could only ever surface as a timeout. EVM tracing runs inline on the runtime's worker threads (tracing_executor.rs has no blocking pool), so enough concurrent requests starve chain sync, the accept loop and the metrics exporter along with each other. Two production incidents already followed from this: an OOM kill from concurrently materialised large prestateTracer responses, and a co-located node degraded ~15× by concurrent cold reads.

Design — the gate is split in two, and that is load-bearing

AdmissionLayer (admission.rs) sits inside ConcurrentBatchLayer and does only a non-blocking CAS against max_concurrent + max_queue. The execution permit is taken in the handler instead, after the response cache misses (rpc_service.rs:267).

A single gate in the middleware would break the metrics accounting identity. CancelGuard arms on a request's first poll (rpc_middleware.rs:242) and the handlers record their arrival synchronously in that same poll — trace_block_by_number:560 → classify_and_gate → record_request_shape:353, with the first .await only at :570. Arm and arrival are atomic. Anything that parks in between makes a client hangup record a cancellation with no matching arrival, i.e. permanent negative drift, worst under exactly the overload the gate exists for. A layer that only CAS-es preserves that by construction, and the permit wait then sits after the arrival is already booked, so a hangup there balances for free.

Placement inside the batch layer is also what makes batch entries admit individually: ConcurrentBatch::batch never delegates to an inner batch, it decomposes into per-entry service.call (rpc_middleware.rs:292). Reversing the two .layer() calls silently lets whole batches through ungated, so batch_entries_are_individually_gated pins the order.

Three further consequences: a cache hit never takes a permit; the typed RequestShape is already in hand, so the heavy-tracer sub-cap costs no second parse of attacker-controlled JSON; and what the permits count is blocks actually being fetched and replayed.

Sizing

Defaults are derived from the most recent capacity run rather than from a constant. Recomputing that run by Little's law — request_duration_seconds_sum 51,549s ÷ 87.65s wall — gives 588 blocks inside handlers, not the 1,766 in-flight figure that run reported; the latter is batch-arrival occupancy, and the 3.0× gap is exactly --batch 30 ÷ --batch-item-concurrency 16. They size different knobs, so --admission-max-concurrent defaults to 640 (just above the measured 588) and --admission-max-queue to 8192. Both sit above every clean measurement: no saturation point has been established for this workload, and a default that throttles a known-good one is the worse failure. Tighten once debug_trace_admission_in_flight and debug_trace_admission_permit_wait_seconds show what production does.

The binding resource is the upstream, not CPU: across that run's concurrency ladder, upstream concurrency rose 97× while eth_getBlockByHash mean went 3.9 ms → 125.2 ms (32×), EVM moved 1.2× and request-path CPU 1.15×.

Two bugs in the reference implementation, deliberately not carried over

mega-reth's concurrency_limiter.rs:260-283 creates its Notified after the capacity check, so a permit freed in that window is never observed and the request parks until some unrelated request finishes — at the tail of a burst, indefinitely. And its setters (:189-191) are a bare store, so raising a limit never wakes parked waiters; combined with an accepted max_concurrent = 0 that is unrecoverable without a restart. Both are avoided by using a resizable FIFO tokio::sync::Semaphore (grow via add_permits, shrink via forget_permits plus a debt counter settled on release), and both directions are regression-tested. Note the comment there claiming a Semaphore cannot shrink is out of date as of tokio 1.36.

Behaviour changes to be aware of

Every struct-logger request is heavy, including the bare default. A debug_trace* call with no tracer emits a record per executed opcode, and the only thing separating it from its already-heavy flagged sibling is a flag that changes the size of that output, not its kind. So an opts-less call now passes --admission-heavy-max-concurrent (default 8). If tracerless calls are a normal part of the workload, raise that flag.

--max-batch-response-size (default 1GB) now bounds batch assembly, previously pinned to u32::MAX. It is deliberately a separate knob from --max-response-size (256MB): a batch retains every completed entry's body until the batch finishes, so its memory is the sum of its entries, and that accumulation — not any single response — is what has exhausted this process before. Reusing one value for both would have capped whole batches at the single-response limit.

Testing

cargo fmt --all --check, cargo clippy --workspace --all-targets --all-features (no warnings), cargo sort --check, and cargo test --workspace (466 tests) all clean.

Verified against a live server rather than only in tests: 30 concurrent requests at a 16-capacity gate shed exactly 14; shape="shed" = reason="overloaded" = 14; the per-method identity summed exactly (shape 30 = served 0 + errors 30 + cancelled 0) with reason="unattributed" at zero. The 16 admitted requests died on deadline_block against a deliberately-unreachable upstream while the 14 refused were refused promptly — the whole design in one measurement. Shrinking maxConcurrent 4→1 over the admin RPC with four permits held reported 4 executing and agreed with the Prometheus gauge.

Three claims were mutation-tested rather than assumed: swapping the two .layer() calls makes batch_entries_are_individually_gated fail; bypassing record_rpc_error in the shed path makes sheds land on the drift alarm; reverting executing() to the derived form makes the occupancy test report 1 where 4 are running.

Notes

A 24-agent adversarial review of the diff raised 20 findings, 9 of which survived independent refutation and are all fixed here. The most consequential: executing() was derived as limit - available_permits, so once a shrink left debt behind it reported the new limit as occupancy — meaning admin_setConcurrencyLimit would have answered a retune by claiming it had already taken effect while every old holder was still resident. Also fixed: by-hash and tx handlers minted the fetch deadline twice, so the queue wait was added on top of --block-fetch-timeout instead of carved out of it; and a legal --witness-timeout >= --block-fetch-timeout pair silently reduced --admission-max-queue to a no-op.

Deliberately out of scope, and stated as such in the docs rather than papered over: nothing here bounds a tracer's intermediate allocations, so the logged concurrency × size products are not a full resident-set bound — a per-transaction-count gate (TODO-G's deeper half) would be. Also unimplemented is the adaptive tier the capacity work argues for (measured service rate → predicted queue wait); this PR is a pure hard gate by design.

Before any overload benchmarking, the load generator needs two fixes: its open-loop --rate mode ignores --batch, and its verdict function scores a shed response as a failure, so every overload test would report FAIL.

… caps Nothing bounded what clients could ask of this process. Every existing concurrency cap is outbound — the witness, data and R2 semaphores limit what we ask of others, and they are unbounded waits, so overflow could only ever surface as a timeout. EVM tracing runs inline on the runtime's worker threads, so enough concurrent requests starve chain sync, the accept loop and the metrics exporter along with each other. Requests beyond the configured budget are now refused immediately with -32013 "Request queue is full", matching mega-reth's ConcurrencyLimiter byte for byte so existing client backoff applies unchanged. The gate is split in two, and the split is load-bearing rather than stylistic. AdmissionLayer sits inside ConcurrentBatchLayer — which decomposes batches into per-entry calls rather than delegating to an inner batch, so an inner layer sees single calls and every batch entry — and only ever runs a non-blocking CAS against max_concurrent + max_queue. The execution permit is taken in the handler, after the response cache misses. CancelGuard arms on a request's first poll and the handlers record their arrival synchronously in that same poll, so a middleware gate that parked before the handler would record a cancellation with no matching arrival for every client that hung up while queued: negative, permanent drift in the accounting identity, worst under exactly the overload the gate exists for. A layer that only CAS-es keeps arm and arrival in one poll, and the permit wait then sits after the arrival is already booked. The placement pays three more ways: a cache hit never takes a permit, the typed RequestShape is already in hand so the heavy-tracer sub-cap costs no second parse of attacker-controlled JSON, and what the permits count is blocks actually being fetched and replayed. Each handler mints its deadline once and passes it to both the permit wait and the fetch, so the queue is carved out of --block-fetch-timeout rather than added on top of it. The reserve held back for the witness stage falls back to half the budget when --witness-timeout does not fit inside --block-fetch-timeout, a legal pair that would otherwise leave a zero-length wait and silently reduce --admission-max-queue to a no-op. Two bugs in the reference implementation are deliberately not carried over: its wait future is created after the capacity check, so a permit freed in that window is never observed, and raising a limit never wakes parked waiters. Both are avoided by using a resizable FIFO Semaphore — grow with add_permits, shrink with forget_permits plus a debt counter settled on release — and both directions are regression-tested. Occupancy is counted rather than derived from available permits: once a shrink leaves debt behind, that difference reports the new limit, so the admin RPC would have answered a retune by claiming it had already taken effect while every old holder was still resident. Every struct-logger request is heavy, including the bare default one. It emits a record per executed opcode, and the only thing separating it from its already- heavy flagged sibling is a flag that changes the size of that output rather than its kind — so an opts-less debug_trace* call is limited by --admission-heavy-max-concurrent. --max-response-size is checked at this server's own serialization point, so an over-limit body is dropped before it can be copied again into the JSON-RPC envelope; --max-batch-response-size separately caps the assembled batch, because a batch retains every completed entry's body until it finishes and that accumulation, not any single response, is what has previously exhausted this process. Startup logs both the heavy and the overall concurrency-times-size products; neither bounds a tracer's intermediate allocations, and the doc says so. --admin-addr starts a second listener, loopback enforced, on its own thread and runtime — inline tracing on the main runtime would otherwise starve it exactly when it is needed — serving admin_getConcurrencyLimit / admin_setConcurrencyLimit so the limits can be retuned without a restart. It carries no RPC middleware, so a saturated public port cannot shed the call that relieves it. A shed records the balanced pair shape="shed" / reason="overloaded" from inside the request future, where the ERROR_SELF_REPORTED task-local suppresses settle_response's unattributed drift arm; settle_response itself is unchanged. Verified on a live server: 30 concurrent requests against a 16-capacity gate shed exactly 14, the balanced pair matched exactly, the per-method identity summed, and the drift alarm stayed at zero; shrinking maxConcurrent 4->1 with four permits held reported 4 executing and agreed with the Prometheus gauge. The layer-order pin, the task-local claim, and the occupancy fix were each mutation-tested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

mega-maxwell · 2026-08-21T16:11:27Z

Claude review status

Living comment — rewritten in place. The review workflow keeps this single comment up to date instead of posting a new one each round, so it always describes the latest reviewed commit and the earlier text is intentionally gone. No reply is needed here; reply to a finding in its own review thread, and answer an open question in a reply on this PR. The next review round reconciles your answer.

🛠️ Review did not finish

Attempted 129d043b..558594ed · updated 2026-08-22T13:43:34+00:00

This round did not publish: MODEL_ACTION_FAILED in phase review_retry. Anything listed below is from the last round that did. Re-run the workflow or push a new commit to try again.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 129d043b22

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-08-21T16:19:18Z

+    fn capacity(&self) -> u64 {
+        self.max_queue.load(Ordering::Relaxed).saturating_add(self.execution.limit())
+    }


Bound heavy waiters by the configured queue limit

When requests use a heavy tracer, this capacity is still calculated from the ordinary execution limit. With the defaults, up to 8,832 heavy requests are admitted even though only 8 can pass the heavy semaphore, leaving 8,824 queued—more than the configured 8,192—and causing subsequent normal requests to be shed while 632 ordinary execution permits remain idle. The issue is especially visible with --admission-max-queue=0, which still allows 632 heavy requests to wait despite its execute-or-shed meaning; heavy admissions need a capacity check based on their own bottleneck or separate queue accounting.

AGENTS.md reference: AGENTS.md:L113-L115

Useful? React with 👍 / 👎.

Confirmed and fixed in 7cd4134.

Reproduced first, at smaller numbers so it fits in a test — 12 heavy requests against a 12-slot gate:

in_flight=12 executing=1 ordinary_permits_idle=3 normal request admitted? false

One refinement to the framing: the "8,824 queued, more than the configured 8,192" half matters less than it reads, because the permit wait is clamped to deadline - witness_timeout — those waiters are shed at the cutoff rather than queueing unboundedly, so the latency bound still holds. The half that bites is the one you named second: ordinary traffic shed while execution permits sit idle, i.e. a priority inversion handed to whoever sends the most expensive shape, inside the feature meant to prevent exactly that.

Took the "own bottleneck" option rather than separate queue accounting, because the latter is not reachable where admission happens. AdmissionLayer runs before anything parses the tracer — deliberately, so the gate never does a second parse of attacker-controlled JSON — so its CAS is class-blind by construction. The check therefore lives in the handler, where RequestShape is already typed:

fn heavy_capacity(&self) -> u64 { let queue_per_slot = self.max_queue() / self.execution.limit().max(1); self.heavy.limit().saturating_mul(queue_per_slot.saturating_add(1)) }

The heavy class may queue in the same proportion to its execution budget as the process as a whole — 104 at the defaults — and requests over that share are refused outright rather than parking on a sub-cap only they can drain. That also covers your --admission-max-queue=0 case: queue_per_slot becomes 0, the share collapses to heavy_max_concurrent, and it is execute-or-shed for heavy requests too.

Regressions: a_heavy_flood_cannot_shed_ordinary_traffic and a_zero_queue_leaves_heavy_requests_nowhere_to_wait, both mutation-tested by restoring the shared budget. The zero-queue one asserts promptness rather than outcome — the old behaviour also ended in Overloaded, just after parking until the deadline, which is the queueing that configuration says it does not want.

chatgpt-codex-connector · 2026-08-21T16:19:19Z

            // The framework swapped a handler's *Ok* for the oversized-response error
            // after the handler already recorded arrival + served: the books are balanced,
            // and recording again here would both over-count the identity and false-fire
-            // the drift alarm. Unreachable while the server pins the response cap to
-            // u32::MAX — load-bearing the day a real `--max-response-size` lands. The
-            // client-saw-error / books-say-served mismatch is accepted like the other
-            // documented approximations.
+            // the drift alarm. Reachable since `--max-batch-response-size` became a real
+            // bound: an entry body that passes the per-response check can still push the
+            // assembled batch past the framework's cap. The client-saw-error /
+            // books-say-served mismatch is accepted like the other documented
+            // approximations.
            Some(OVERSIZED_RESPONSE_CODE) => {}


Account for envelope bytes before marking responses served

When --max-batch-response-size is configured equal or close to --max-response-size (which validation explicitly permits), the local size check measures only the raw RawJson result while the framework cap measures the complete JSON-RPC response, including its envelope and request ID. A result that passes the single-response limit can therefore be replaced with OVERSIZED_RESPONSE_CODE here, but this branch ignores that client-visible error after the handler has already recorded the request as served and without incrementing debug_trace_response_oversized_total; reserve space for the envelope or defer success accounting until the framework response is known.

AGENTS.md reference: AGENTS.md:L121-L123

Useful? React with 👍 / 👎.

Confirmed and fixed in 7cd4134.

Right on both counts: validation permitted the two caps to be equal, and our check measures the bare RawJson body while the framework's measures the whole response including the envelope and the client-supplied id.

Chose to make that configuration unreachable rather than reserve envelope space at the serialization point. It only bites in a combination an operator has to set deliberately, so a named startup error beats computing envelope overhead on the hot path:

--max-batch-response-size (512MB) must be at least --max-response-size (512MB) plus 65536 bytes of headroom: a batch holds whole entry bodies, and the framework's cap counts the JSON-RPC envelope and request id that our own size check does not

Took the second half of your point too: the OVERSIZED_RESPONSE_CODE arm in settle_response now increments debug_trace_response_oversized_total, so that series stays complete even if the headroom is somehow breached by a pathological id. It remains a no-op for the accounting identity, since the oversized counter is not one of its terms.

Deliberately left alone: the served/client-saw-error mismatch itself. That is the documented approximation on that arm — the handler already recorded arrival + served before the framework swapped the response, so recording again there would over-count the identity and false-fire the unattributed drift alarm.

Test: batch_response_cap_must_cover_one_response_plus_its_envelope.

chatgpt-codex-connector · 2026-08-21T16:19:19Z

+        let heavy_permit = if heavy { Some(acquire_by(&self.heavy, cutoff).await?) } else { None };
+        let permit = acquire_by(&self.execution, cutoff).await?;


Count heavy permits while they wait for execution

When the ordinary execution semaphore is saturated, a heavy request acquires its heavy permit here and then blocks on the ordinary permit, but debug_trace_admission_heavy_executing is not incremented until both acquisitions finish. Consequently the gauge can report zero while every heavy permit is reserved and new heavy requests are blocked; the admin RPC simultaneously reports the true value via checked_out(). Track heavy occupancy with a guard starting immediately after the heavy acquisition so the Prometheus and admin views agree during overload.

AGENTS.md reference: AGENTS.md:L115-L115

Useful? React with 👍 / 👎.

Confirmed and fixed in 7cd4134.

This is the same class as a bug fixed one commit earlier for ordinary occupancy — executing() was derived as limit - available_permits, which reports the new limit once a shrink leaves debt behind — so "the Prometheus view and the admin RPC disagree" was already something we had decided was wrong. Consistency alone justified it.

Now raised by an RAII guard at acquisition rather than once both permits are in hand:

impl HeavyPermit { fn new(permit: DebtAwarePermit) -> Self { metrics::record_admission_heavy_delta(1.0); Self { _permit: permit } } } impl Drop for HeavyPermit { fn drop(&mut self) { metrics::record_admission_heavy_delta(-1.0); } }

The guard also closes a path the previous code only got right by omission: a heavy request that takes the sub-cap permit and then loses the ordinary one on the deadline now decrements on the way out instead of relying on never having incremented.

Test: heavy_occupancy_is_visible_while_waiting_for_an_execution_permit.

…d budget Three findings from the Codex review of #188, all reproduced before fixing. The admitted budget is class-blind by design — the gate runs before anything parses the tracer — so heavy requests were admitted against `max_queue + max_concurrent` and then bottlenecked on a sub-cap a fraction of its size. A flood of them filled the budget while barely executing: reproduced with 12 heavy requests against a 12-slot gate, where 1 executed, 3 execution permits sat idle, and an ordinary request was refused. That is a priority inversion handed to whoever sends the most expensive shape, in the feature meant to prevent exactly that. Heavy requests now get their own share — they may queue in the same proportion to their execution budget as the process as a whole — which also makes `--admission-max-queue 0` mean execute-or-shed for them, where a share of the shared budget let 632 of them wait at the defaults. The heavy occupancy gauge was raised only once both permits were in hand, so it read zero while every sub-cap permit was reserved and further heavy requests were blocked on them — disagreeing with the admin RPC, which counted them immediately. It is now raised by a guard at acquisition, the same shape the ordinary occupancy counter already uses. `--max-batch-response-size` was allowed to equal `--max-response-size`, but our own check measures the bare body while the framework's cap measures the envelope and client `id` too. A body that just passed ours could still be swapped for an oversized-response error after the handler had counted the request as served, and without incrementing the oversized counter. Startup now requires headroom between the two caps and names why, and the framework-swapped case is counted on `debug_trace_response_oversized_total` so that series is complete either way. Both heavy-budget regressions were mutation-tested: restoring the shared budget fails them, and the zero-queue case asserts promptness rather than outcome, since the old behaviour also ended in `Overloaded` — just after parking until the deadline, which is the queueing that configuration says it does not want. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7cd41340b6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-08-22T11:55:06Z

+    fn notification<'a>(
+        &self,
+        n: Notification<'a>,
+    ) -> impl Future<Output = Self::NotificationResponse> + Send + 'a {
+        // jsonrpsee answers notifications without ever dispatching a handler, so there is
+        // no work here to protect and nothing to shed.
+        self.service.notification(n)


Gate JSON-RPC notifications through admission

When a client omits the JSON-RPC id, jsonrpsee dispatches the registered method through notification, but this branch delegates directly without try_admit. Trace notifications still invoke the handlers and their execution-permit waits, so clients can create an unbounded number of queued futures—especially across concurrent batches or connections—bypassing max_concurrent + max_queue and defeating the overload protection this change introduces. Apply admission accounting to gated notifications as well, silently dropping them when capacity is exhausted since notifications cannot receive an error response.

AGENTS.md reference: AGENTS.md:L112-L113

Useful? React with 👍 / 👎.

Respectfully, I think this one is incorrect — and the premise is checkable.

RpcService::notification in jsonrpsee-server-0.26.0/src/middleware/rpc.rs never dispatches the registered method:

fn notification<'a>(&self, n: Notification<'a>) -> impl Future<Output = Self::BatchResponse> + Send + 'a { // The notification should not be replied to with a response // but we propogate the extensions to the response which can be useful // for example HTTP transport to set the headers. async move { MethodResponse::notification().with_extensions(n.extensions) } }

So "Trace notifications still invoke the handlers and their execution-permit waits" does not hold: a notification reaches no handler, waits on no permit, and does no work. Gating one would spend a unit of capacity on nothing — and, as you note, a notification cannot carry an error, so shedding one could only be silent. Both are worse than the pass-through.

There was already indirect evidence in the suite: accounting_identity_holds_end_to_end drives a batch containing a notification and the identity closes. If a handler ran for it, that handler would record an arrival and the identity would not balance.

Added a direct pin anyway, since this is an inviting misreading and the next reader deserves better than a comment — a_notification_never_reaches_a_handler_or_takes_capacity in 558594e. It registers a gated method that flips an AtomicBool, saturates the gate so anything gated would be shed, sends the method as a notification, and asserts the flag is still false and in_flight is still 0.

Happy to reopen if you can point at a path where a notification does reach RpcModule's dispatch.

chatgpt-codex-connector · 2026-08-22T11:55:06Z

+            let removed = self.sem.forget_permits(wanted);
+            if removed < wanted {
+                self.debt.fetch_add((wanted - removed) as u64, Ordering::SeqCst);


Make semaphore shrink atomic with permit release

When the admin API shrinks a saturated limiter while a holder finishes, a release can occur after forget_permits observes no available permits but before the missing amount is added to debt. That release sees zero debt and returns its permit to the semaphore, allowing a queued request to acquire it even though the new limit is already below the remaining old occupancy; the subsequently recorded debt only corrects the oversubscription after later releases. Serialize this interval with release or publish the debt before a permit can be returned.

AGENTS.md reference: AGENTS.md:L119-L119

Useful? React with 👍 / 👎.

Confirmed and fixed in 558594e. Good catch — the window is exactly as you describe.

Fixed by inverting the ordering rather than serializing with release, since the release path is deliberately lock-free (the resize mutex is documented as never being taken there, and putting it on the hot path to close a nanosecond window would be a poor trade):

let wanted = previous - permits; self.debt.fetch_add(wanted, Ordering::SeqCst); let removed = self.sem.forget_permits(wanted as usize) as u64; self.cancel_debt(removed);

A release landing mid-shrink now observes a debt that is at worst too large, so it forgets its permit — the conservative direction — and the part that could be removed immediately is cancelled afterwards. Working the invariant limit + debt == available + checked_out through both interleavings, with and without a concurrent release, it balances either way.

One thing I want to be straight about: the new concurrent_resizes_and_releases_keep_the_budget_exact test does not pin this reordering. I mutation-tested it against the old ordering and it still passes, because the old defect is a transient over-admission and that test asserts a quiescent invariant. It earns its place guarding a different failure — a permit lost to a double-forget or conjured by a release that should have forgotten one — and it is bounded by a timeout so that failure comes back red rather than as a hung job. The reordering itself rests on the argument above, not on a regression test.

chatgpt-codex-connector · 2026-08-22T11:55:07Z

+    let required_batch_cap = args.max_response_size.saturating_add(RESPONSE_ENVELOPE_HEADROOM);
+    if args.max_batch_response_size < required_batch_cap {


Derive envelope headroom from accepted request IDs

The fresh validation still reserves only a fixed 64 KiB even though the response envelope contains the client-controlled JSON-RPC ID. With --max-batch-response-size configured near --max-response-size, an accepted string ID larger than the remaining headroom lets the raw result pass the local check and be counted as served, after which jsonrpsee replaces the enveloped response with an oversized-response error. Bound accepted IDs/request bodies to this allowance or account using the actual envelope size rather than assuming fixed headroom.

AGENTS.md reference: AGENTS.md:L121-L121

Useful? React with 👍 / 👎.

Confirmed and fixed in 558594e. You are right that 64 KiB was an assumption dressed as a bound.

Took the "bound accepted IDs" route, since the envelope's only unbounded part is the client-supplied id and the request body already bounds it:

/// Cap on an inbound request body, pinned rather than left to the framework's default so the /// envelope headroom below can be derived from it instead of guessed. const MAX_REQUEST_BODY_SIZE: u32 = 10 * 1024 * 1024; const RESPONSE_ENVELOPE_HEADROOM: u64 = MAX_REQUEST_BODY_SIZE as u64 + 1024;

The cap is now set explicitly on ServerConfig rather than inherited from the framework's default, so the derivation cannot silently drift on a dependency upgrade — which was the other half of what made the fixed number fragile.

Accounting by actual envelope size was the alternative, and I passed on it: the size check lives at RawJson::try_new, deep in the handler, where the request id is not in hand. Threading it down for a bound that a subtraction at startup already gives exactly did not seem worth the coupling.

Two more findings from the Codex re-review of #188; a third was refuted. Shrinking a saturated limiter recorded the debt only after `forget_permits` reported how little it could take. A release landing between the two observed zero debt, handed its permit back, and let a queued request through above the limit the shrink had just set — corrected only by a later release. Publishing the debt first inverts the window: a release now sees a debt that is at worst too large and forgets its permit, which is the conservative direction, and the part that could be removed immediately is cancelled afterwards. The accounting is unchanged in the quiescent case. The envelope headroom between `--max-batch-response-size` and `--max-response-size` was a fixed 64 KiB, which is a guess rather than a bound: the envelope's only unbounded part is the client-supplied `id`, and a client can pad one well past that. It is now derived from the request-body cap, which bounds the id outright, and that cap is pinned explicitly rather than inherited from the framework's default so the derivation cannot drift on an upgrade. Refuted: gating notifications. `RpcService::notification` answers without dispatching the method at all, so a notification reaches no handler, waits on no permit and does no work; gating one would spend capacity on nothing, and since a notification carries no response a shed could only be silent. Pinned by `a_notification_never_reaches_a_handler_or_takes_capacity`, which asserts the handler never runs and no capacity is taken even with the gate saturated. The new debt stress test guards end-state accounting — no permit lost to a double-forget, none conjured by a release that should have forgotten one — and is bounded by a timeout so a protocol that stops circulating permits fails red instead of hanging the job. It deliberately does not claim to pin the reordering above: the old ordering's defect is a transient over-admission that a quiescent invariant cannot observe, and it still passes under that mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 558594ed14

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-08-22T13:45:40Z

+            self.debt.fetch_add(wanted, Ordering::SeqCst);
+            let removed = self.sem.forget_permits(wanted as usize) as u64;
+            self.cancel_debt(removed);


Serialize shrink debt with permit release

When a permit is released after debt.fetch_add(wanted) but before forget_permits(wanted), the release consumes one debt unit and forgets its permit, yet forget_permits can still remove the original wanted count from already-available permits; the final cancel_debt then clears only the remaining debt, leaving the semaphore permanently short by one permit (potentially at zero until another resize or restart). Fresh evidence in this version is that publishing the debt before removal fixes the previously reported oversubscription race but introduces this inverse race, so release and immediate permit removal still need serialization or a single coordinated accounting operation.

AGENTS.md reference: AGENTS.md:L119-L119

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-22T13:45:40Z

+    let max_response_body_size = u32::try_from(args.max_batch_response_size).unwrap_or_else(|_| {
+        warn!(
+            requested = args.max_batch_response_size,
+            applied = u32::MAX,
+            "--max-batch-response-size exceeds the JSON-RPC framework's cap; clamping"


Reject per-response caps above the framework ceiling

When --max-response-size exceeds u32::MAX and --max-batch-response-size is set high enough to pass validation, this clamp silently makes the framework cap smaller than the configured per-response cap. A response between those limits passes check_response_size and is counted as served, but jsonrpsee replaces it with an oversized-response error; reject such configurations or apply the effective clamped ceiling to validation and the local response check. Fresh evidence beyond the earlier envelope-headroom finding is the remaining u64-to-u32 clamp path for caps above 4 GiB.

AGENTS.md reference: AGENTS.md:L121-L121

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-22T13:45:40Z

+            let response = service.call(request).await;
+            drop(guard);
+            response


Hold admission capacity until the response body drains

When clients read large responses slowly or stop reading without disconnecting, dropping the guard as soon as service.call produces a MethodResponse frees admission capacity before hyper streams that response body. New requests can therefore keep serializing additional responses while arbitrarily many completed bodies remain pinned by slow connections, bypassing the advertised max_concurrent × max_response_size memory bound and allowing an OOM despite the gate; retain a capacity guard through body completion/drop or add a separate bounded response-body budget.

AGENTS.md reference: AGENTS.md:L130-L131

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-22T13:45:40Z

+        let _permit =
+            self.acquire_execution(METHOD_DEBUG_TRACE_TRANSACTION, weight, deadline).await?;

        let (data, tx_index) = self
            .data_provider
-            .get_block_data_for_tx(tx_hash)
+            .get_block_data_for_tx(tx_hash, deadline)


Acquire trace permits after resolving transactions

When a transaction hash is missing, pending, or its upstream lookup is queued or stalled, this acquires an ordinary execution permit—and for default/prestate requests a scarce heavy permit—before get_block_data_for_tx has established that any block can be fetched or replayed. A stream of unresolved transaction requests can therefore occupy every execution/heavy slot for the fetch deadline and stall valid block traces while no EVM work is running; split transaction resolution from block-data fetching and acquire the permits only after a mined transaction has supplied its block binding, while retaining the same request deadline.

AGENTS.md reference: AGENTS.md:L115-L115

Useful? React with 👍 / 👎.

flyq requested review from Troublor and vincent-k2026 as code owners August 21, 2026 16:11

chatgpt-codex-connector Bot reviewed Aug 21, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 22, 2026

View reviewed changes

		let heavy_permit = if heavy { Some(acquire_by(&self.heavy, cutoff).await?) } else { None };
		let permit = acquire_by(&self.execution, cutoff).await?;

		let required_batch_cap = args.max_response_size.saturating_add(RESPONSE_ENVELOPE_HEADROOM);
		if args.max_batch_response_size < required_batch_cap {

Uh oh!

Conversation

flyq commented Aug 21, 2026

Summary

Root cause

Design — the gate is split in two, and that is load-bearing

Sizing

Two bugs in the reference implementation, deliberately not carried over

Behaviour changes to be aware of

Testing

Notes

Uh oh!

mega-maxwell Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude review status

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

flyq Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

flyq Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

flyq Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

flyq Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

flyq Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

flyq Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

mega-maxwell Bot commented Aug 21, 2026 •

edited

Loading