apollo_state_reader,apollo_batcher: bound class manager reads in ClassReader - #14984
Conversation
PR SummaryMedium Risk Overview
View calls pass Reviewed by Cursor Bugbot for commit bebd128. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
This change is |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…sReader call_contract's outer view-call timeout frees the batcher's request slot but does not cancel the spawn_blocking task underneath it: ClassReader::block_on has no bound of its own, so a class manager that never answers (a network partition, an overloaded remote class manager, or a locally-deployed class manager reached over a channel with no request timeout) pins that blocking- pool thread forever. Block production shares the same pool via its own spawn_blocking calls, so enough stalled view calls exhaust it and stall block production too. Add request_timeout to ClassReader and bound every class manager request with it. View calls pass view_call_timeout_millis, so the promise behind that config value now holds end to end. Block production keeps its prior unbounded behavior (request_timeout: None), since it has no per-call deadline to bound against and changing that is out of scope here. A new test drives get_compiled_class through a class manager stub that never answers: without the bound it hangs indefinitely (confirmed against this change before the fix landed); with it, the read fails within request_timeout instead of pinning the thread.
…ot each request An independent review of the fix caught that request_timeout bounded each class manager call separately: read_casm_and_sierra makes two sequential calls, so a Cairo 1 class could still pin the blocking-pool thread for up to 2x the intended bound (more with additional classes touched in one view call). Replace request_timeout: Option<Duration> with deadline: Option<Instant>, computed once when the reader is constructed. Every request through the reader now bounds against that same deadline via tokio::time::timeout_at, so the reader's total time waiting on the class manager cannot exceed the caller's original budget regardless of how many requests it takes. Also declares apollo_state_reader's tokio dependency features explicitly (rt, time) rather than relying on workspace feature unification, and adds a test proving the cumulative bound: a class manager that answers get_executable after a delay and then stalls on get_sierra must still fail close to the original deadline, not after a fresh window starting from get_sierra.
63ef09a to
3ce4047
Compare
|
Artifacts upload workflows: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3ce4047. Configure here.
…g them on timeout
Cursor Bugbot caught a high-severity regression in the previous commit:
tokio::time::timeout_at raced the class manager request future directly and
dropped it when the deadline passed. For a locally-deployed class manager,
that future (LocalComponentClient::send) hands its response sender to the
server before it resolves; dropping the future drops the matching receiver,
and the server's tx.send(response).await.expect("Response connection should
be open.") then panics — turning a slow view call into a crash of the class
manager's request-processing task instead of just a failed read.
Spawn the request onto the runtime and race the join handle against the
deadline instead of the request itself. Only our wait is bounded; if the
deadline passes first, the spawned task keeps running to completion in the
background rather than being dropped, so the server always gets to send its
response and never observes a closed channel. This still fully closes the
original bug: read_executable/read_sierra/read_compiled_class_hash_v2 now
build owned, 'static futures over a cloned reader so they can be spawned.
Added a regression test reproducing the exact panic pattern with a bare
channel standing in for the local transport: confirmed it panics against the
race-and-drop version of block_on_request, and passes with the spawn-based
one.
asaf-sw
left a comment
asaf-sw
left a comment
There was a problem hiding this comment.
@asaf-sw partially reviewed 16 files and made 1 comment.
Reviewable status: 10 of 16 files reviewed, all discussions resolved (waiting on gkaempfer).
asaf-sw
left a comment
asaf-sw
left a comment
There was a problem hiding this comment.
@asaf-sw reviewed 6 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on gkaempfer).
|
Security scan complete — no issues detected. Generated by Claude Code |
4 participants
Summary
Follow-up to #14967, found by an automated post-merge security scan.
That PR made
call_contractview calls read Cairo 1 class definitions throughClassReader, which blocks the calling thread on the class manager viaself.runtime.block_on(...). Its own comment on the added outer timeout says the quiet part out loud:ClassReader::block_onhad no bound of its own beneath that outer timeout. If the class manager never answers — a network partition, an overloaded remote class manager, or (for a locally-deployed class manager reached over an in-process channel) a stalled component, sinceLocalComponentClient::sendawaits a channelrecv()with no timeout — thespawn_blockingtask pins its OS thread on Tokio's shared blocking-thread pool forever.call_contract's outertokio::time::timeoutreturns an error to the caller, but frees nothing: the thread stays blocked.Note on severity, found while preparing this fix: #14972 (already on this base) caps concurrent view calls at 32 via a semaphore, so a stalled class manager can no longer exhaust Tokio's whole shared blocking pool and stall block production — that worst case is already closed. What's left, and what this PR fixes, is narrower but still real: without a bound, a persistently stalled class manager permanently consumes all 32 view-call slots (each pinned forever), permanently disabling
call_contractuntil the batcher process is restarted, since a slot is only freed when its blocking closure returns. With this fix, each stuck request eventually times out, the closure returns, and the slot (and thread) is freed — view calls degrade and self-heal instead of failing shut permanently.Fix
ClassReadergainsdeadline: Option<Instant>. Every class manager request (get_executable,get_sierra,get_executable_class_hash_v2) is wrapped intokio::time::timeout_at(deadline, ...)beforeblock_on, when set.Instantcomputed once when the reader is constructed, not a duration re-applied per request: reading a Cairo 1 class needs two sequential class-manager calls (read_casmthenread_sierra), and a per-request timeout would let their durations add up to a multiple of the intended bound. An earlier revision of this PR had that bug; caught by an independent review pass before merge (see commit history).StorageViewStateReaderFactory::create, threaded throughViewStateReaderFactory::create) computes the deadline fromview_call_timeout_millis— the same duration already advertised as bounding a view call's hold on the batcher's request slot. That promise now holds end to end: the blocking-pool thread (and, thanks to apollo_batcher: bound in-flight view calls so they cannot exhaust the blocking pool #14972, the semaphore permit) is released within that same window instead of being pinned indefinitely.ClassReader(block_builder.rs) passesdeadline: None, preserving its existing unbounded behavior — it has no per-call deadline to bound against, and changing that is a separate decision out of scope here.Test plan
class_reader_times_out_when_the_class_manager_never_answers(apollo_state_reader) — a hand-writtenClassManagerClientstub whoseget_executablenever resolves. Verified this hangs indefinitely against the pre-fix code (killed only by an external 60s cap, no test output at all), and passes deterministically (~0.2s) with the fix.class_reader_deadline_bounds_the_total_wait_across_requests(apollo_state_reader) — a stub whoseget_executablesucceeds after a delay and whoseget_sierrathen stalls; asserts the read fails close to the original deadline rather than after a fresh window starting fromget_sierra.cargo test -p apollo_batcher --lib— 132 passed, 0 failed.cargo test -p apollo_state_reader --lib— 3 passed, 0 failed.scripts/rust_fmt.shcargo clippy -p apollo_state_reader -p apollo_batcher --all-targets --tests -- -D warnings— clean.