fix(runtime): do not hand a thread a shut-down no-steal runtime - #982
Open
rickcrawford wants to merge 1 commit into
Open
fix(runtime): do not hand a thread a shut-down no-steal runtime#982rickcrawford wants to merge 1 commit into
rickcrawford wants to merge 1 commit into
Conversation
CURRENT_HANDLE is a ThreadLocal keyed by an id the thread_local crate recycles on thread exit, so a registration outlives the worker that made it and reappears under whichever thread is handed the same id next. A NoStealRuntime built after an earlier one has shut down can therefore start life holding the dead runtime's handles, and every task its workers spawn through current_handle() is canceled the instant it is spawned, on a runtime that is alive and healthy. This is issue cloudflare#981. Store the owning ThreadId beside the pools. A std::thread::ThreadId is never reused for the life of the process, so the comparison is exact. init_pools() now claims the slot by assignment rather than get_or, so a worker that inherits a recycled id overwrites the stale entry instead of adopting it, and current_handle() falls through to Handle::current() unless the registration belongs to the calling thread. The unwrap() on the pools OnceCell goes with it, since the owner check makes it unnecessary. The added test drives the registration side and fails without the fix. It lives in a file of its own with a single test because it depends on being the only thing in its process allocating thread_local ids.
rickcrawford
added a commit
to soapbucket/sbproxy
that referenced
this pull request
Aug 29, 2026
…es it (#1245) * fix(runtime): give Pingora workers an 8 MiB stack and measure what uses it main is aborting on CI's request-path smoke lane with thread 'Pingora HTTP Proxy Service' has overflowed its stack fatal runtime error: stack overflow, aborting edf8b93 is the commit that tipped it, but the AI request path was already using more than half a worker's 2 MiB stack before that landed, so it is the straw and not the load. Pingora never set thread_stack_size, so every worker ran on tokio's 2 MiB default. The fork now defaults to 8 MiB, the RLIMIT_STACK default Linux gives a main thread, and exposes it as runtime_thread_stack_size; this bumps the pinned rev and adds SB_WORKER_STACK_BYTES beside SB_WORKER_THREADS. The cost on a 64-bit target is reserved address space, not memory: roughly 264 MiB across 33 threads out of 128 TiB, resident nothing, because a thread stack is committed page by page as it is touched. Raising the ceiling without a floor gauge trades a painful signal for no signal, so the guards change shape. The two that measured size_of on a future are gone: they reported 13.9% and 74.9% of budgets that between them could see about 4% of what fills the stack, and both stayed green through three overflows. A future's size is the state it holds between polls; the stack is the chain of frames live during one. server::stack_probe measures the second thing. Pingora's runtime records each worker's stack base, the request path takes the address of a local at its deepest point, and the difference is bytes in use. the_ai_dispatch_path_stays_inside_its_stack_budget drives a whole streamed AI request through request_phase::request_filter on a worker sized to scripts/stack-budget-baseline.count, and checks two things: the request completes, which covers every frame below the probes; and the probe reported a non-zero depth, so the check cannot pass vacuously. The budget only falls, enforced by check-stack-budget-ratchet.sh. The probe costs a call, two thread-local reads, a subtract and an untaken branch per request, and uses no unsafe. The smoke lane sat behind a twelve-path trigger that neither breaking commit touched, which is why main sat broken with nothing to say so. A stack budget cannot be gated on a path list: a frame added in any crate spends the same budget. It moves to its own workflow with no filter and gains the budget test as its first step. * fix(test): filter the stack budget test by name, not by exact path `--exact` matches the full module path, so `--exact the_ai_dispatch_path_stays_inside_its_stack_budget` selected nothing and the run reported "0 passed; 2857 filtered out" with exit code 0. A CI step that cannot select its own test is a green lane that checks nothing, which is the failure this branch exists to end. * test(stack): fix the fixture YAML mangled when lifting the helper Lifting `ai_request_context` to module scope meant dedenting it, and the dedent ran over the raw string literal too, flattening the YAML the fixture pipeline compiles from. The test failed loudly rather than quietly measuring a shorter path, which is the behavior worth having. Also: the budget is now an assertion on a production-sized worker rather than an overflow on a budget-sized one. An overflow aborts, and an abort has no number and no message, which is the wrong failure for the check that exists to explain this class of bug. The abort stays underneath for a path that outgrows the whole production stack. Measured on this branch: 1,600,928 bytes of 4,194,304. * fix(stack): follow the fork's StackMark API and re-pin the rev pingora-runtime's worker_stack::here() now returns a StackMark rather than a bare usize. In a debug build the mark carries the ThreadId that made it and used_here asserts the two agree, so comparing a mark against a stack it does not belong to fails loudly where tests run; a release build carries neither the field nor the check, so the probe still costs a call, two thread-local reads, a subtract and an untaken branch. The module doc also stops promising more than the fork delivers. A work-stealing worker, which is the flavor sbproxy runs, is marked from tokio's on_thread_start callback rather than from the top of the thread, so the number under-reports by roughly one closure frame: tens of bytes against a budget in the megabytes. The fork documents which thread flavors are marked from where; this points at it. Lockfile: the pinned rev only, 11 lines, one per patched crate. * docs(pingora): record what the fork actually is, and fix the re-sync step Three things, all about the same confusion: the pinned fork declares 0.8.0 and is named for it, and neither is a release we track. Cargo.toml's comment above [patch.crates-io] now says what we run. The tree carries 167 upstream commits the 0.8.0 tag does not, plus our seven, so the version is an API generation and not something to match an advisory against. The concrete numbers come from scripts/divergence.sh in the fork, which its CI also prints on every PR: base: 0046038 (2026-08-07) ahead: 7 commits of ours behind: 23 upstream commits files: 20 differ, 14 outside .github/ CLAUDE.md and AGENTS.md both told the release procedure to rebase sbproxy-0.8.0 onto "the target upstream tag". That is the step somebody actually follows at a release, and following it would move the fork onto a different line: Cloudflare cuts releases on a release branch, so 0.8.1 is not an ancestor of main. It holds 8 commits main lacks while main holds 190 it lacks. It would also strand the current_handle() fix we have upstreamed at cloudflare/pingora#982, which lands on main. Both files now say to rebase onto a newer upstream main, give the release-branch reason in a sentence so nobody re-derives it, and point at scripts/divergence.sh for the before-and-after. The lockfile step gained the rule this branch followed: diff it and revert anything that is not the pingora-* rev bumps. Lockfile re-pinned to 2d9dc2b, the merged fork tip, which is the stack work plus the divergence report. Eleven rev lines and nothing else. * fix(stack): make the ratchet and the budget assertion able to fail Independent review found that the guard this branch adds could not fail, which is the defect the branch exists to remove. Four of the findings are that same shape. C1. check-stack-budget-ratchet.sh resolved its base with `git merge-base HEAD origin/main` and ran in ci.yml's lint job, whose checkout has no fetch-depth. On a pull_request event neither origin/main nor main resolves, so it printed a note and exited 0: it ran on every pull request and checked nothing. The three sibling ratchets in that lane count source sites, which a depth-1 checkout serves, which is why none of them exposed it. It now fails closed when no base resolves and takes STACK_BUDGET_BASE_REF the way the changelog guard next door does, and the step moved to the guards job, whose checkout is full depth for exactly this reason. Its --self-test is wired there too. M1. the_ai_dispatch_path_stays_inside_its_stack_budget built its worker with .thread_stack_size(STACK_BUDGET), so the thread's stack was the budget and `used <= budget` could not fail without the process having already aborted, while the doc claimed the opposite. The abort also sat at half the production stack, so it could abort on a path production serves. The worker is now sized to DEFAULT_THREAD_STACK_SIZE, the budget is the assertion, and the two tiers are named in the doc and printed in the log line. M2. The budget was half the stack, 4,194,304 against 1,600,952 measured: 2.6x of slack, when the commit that broke main moved the depth 31,152 bytes. Now set from the measurement, pending the Linux number this PR's own smoke lane prints. M4. ServerConf::validate() runs only from from_yaml and sbproxy passes a struct literal, so the sub-page refusal documented in docs/manual.md never ran: SB_WORKER_STACK_BYTES=8 started and aborted on the first request. resolve_worker_stack_bytes refuses it with a warning and falls back to the default, and the manual says what actually happens. M5 gives that hunk its first test. M3 adds the key to the env-var table. M6 corrects five comments that still called the stack 2 MiB, one of them inside an assertion message. m1 restores the buffered path's stack coverage, which deleting the non-streaming test lost along with the only reach to the probe in relay_ai_response_with_cache. m3 makes the smoke lane count the tests that ran, because cargo test exits 0 on a filter that matches nothing, which is the third appearance of that defect in this change. m4 scopes the workflow trigger. m5 retypes the fragment. Measured after: streaming 1,600,952 and buffered 1,468,456 of a 2,621,440 budget on an 8,388,608 stack. * test(ci): TEMPORARY C1 proof, ratchet in a shallow checkout Reverted in the next commit. Runs the stack-budget ratchet in ci.yml's lint job, whose checkout has no fetch-depth, which is the exact environment where it used to resolve no base and exit 0. With the fail-closed fix it has to turn this job red. The guards job runs the same script against a full-depth checkout in the same workflow, so one run shows both halves. * Revert "test(ci): TEMPORARY C1 proof, ratchet in a shallow checkout" The proof is recorded. Run 33220527341 on PR #1245: lint job, checkout fetch-depth: 1 cannot resolve a base to ratchet against job FAILED guards job, checkout fetch-depth: 0 check-stack-budget-ratchet self-test: ok stack budget: 2621440 bytes (new on this branch) job PASSED That is the exact environment where the ratchet used to print a note and exit 0, now failing closed, beside the job where it actually runs. This reverts the temporary step; the real one stays in guards. * test(eval): count timeouts against the jobs present, not a literal Moving the request-path smoke job to its own unfiltered workflow left context-compression-eval.yml with one job, and readme_and_workflow_cover_reproducibility_and_external_data_boundaries asserted `timeout-minutes:` appeared at least twice. The comment above that line already said to pin the property and not the number, and the line did the opposite. It now counts `runs-on:`, which appears once per job, and requires a timeout for each. Verified against both workflows: 1 job, 1 timeout, each. Not run locally. This harness needs the 1.98 pin and the only toolchains on this machine are Homebrew 1.95 and rustup 1.90, so CI is the check. * fix(stack): set the budget from the Linux measurement 2,359,296 = ceil(1_604_072 / 256 KiB) * 256 KiB + 512 KiB, where 1,604,072 is STACK_HIGH_WATER_BYTES printed by this PR's own production request-path smoke lane on Linux. Buffered measured 1,471,496 on the same run. One thing the two numbers settle, because I had assumed otherwise: the same fixture measures 1,600,952 on aarch64 macOS, 3,120 bytes less than Linux. Linux debug frames are not meaningfully larger for this path. The distance between 1.6 MiB here and the 2 MiB stack that actually overflowed in CI is the configuration that lane runs, compression and a CEL policy plane and guardrails, not the target. --------- Co-authored-by: t <t@t>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #981.
CURRENT_HANDLEis aThreadLocalkeyed by an id thethread_localcrate recycles on thread exit, so a registration outlives the worker that made it and turns up under whichever thread is handed the same id next. ANoStealRuntimebuilt after an earlier one has shut down can start life holding the dead runtime's handles, and every task its workers spawn throughcurrent_handle()is canceled the instant it is spawned, on a runtime that is otherwise healthy. The issue has the full mechanism.The fix stores the owning
ThreadIdbeside the pools. Astd::thread::ThreadIdis never reused for the life of the process, so the comparison is exact rather than probabilistic.init_pools()claims the slot by assignment rather thanget_or, so a worker that inherits a recycled id overwrites the stale entry instead of adopting it, andcurrent_handle()falls through toHandle::current()unless the registration belongs to the calling thread. Theunwrap()on the poolsOnceCellgoes with it, since the owner check makes it unnecessary. The assignment also drops the staleArcthatget_orused to keep alive, so this retains less, not more.The added test drives the registration side of the bug and fails on
mainwithout this change:It is in a file of its own with a single
#[test], because it depends on being the only thing in its process allocatingthread_localids. The reasoning is in the file's module comment so a later reader does not add a second test and quietly break it.cargo test -p pingora-runtime,cargo clippy -p pingora-runtime --all-targets -- -D warnings, andcargo fmt --checkare clean.