core: release the execute-ordering ticket refused by the shutdown gate - #352
Merged
Conversation
`handleImpl` takes a per-model execute-ordering ticket on the transport thread, before posting to the pool. `dispatchMessage`'s shutdown gate then returned before `dispatchExecute` — the only place a ticket was released — so an `execute` refused with `err "server shutting down"` dropped its ticket without releasing it. This was documented, in both `remote.hpp` and `docs/spec/core/backend.md`, as a benign exception to the "every path that took a ticket must release it" rule: `beginShutdown()` is irreversible, so every later `execute` is refused at the same gate and none reaches `awaitExecuteTurn`. That reasoning misses the window the ordering gate exists because of. Tickets are taken in send order on the transport thread, but the pool may run the two posted tasks in either order — so a later ticket can pass the gate while the earlier one is still upstream of it, and be parked in `awaitExecuteTurn` (a `cv.wait` with no deadline) by the time the earlier one is refused. Dropping the earlier ticket then costs three things, not a leaked map entry: the later caller never receives a reply at all, or a spurious `err "timeout"` where `LimitPolicy::executeTimeout` is configured; a pool worker is blocked for the rest of the process's life; and, because `_inFlightExecutes` is incremented immediately before that wait, `drainedWithin()` can never succeed — so the defect breaks the graceful-shutdown sequence during which it fires. The gate now releases the ticket before replying, and the two comments plus the spec section that presented the old analysis are corrected. The regression test forces the interleaving rather than racing for it: a wrapping executor holds back one request's dispatch task, so "the later ticket passed the gate, the earlier one did not" is decided, not hoped for. The natural window is nanoseconds wide — 400 jittered attempts did not hit it. Because a regression strands a pool worker in a deadline-less wait, the test leaks its fixture on the failing path rather than hanging the binary in `~ThreadPoolExecutor`'s join. Fixes #348 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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 #348.
The defect
handleImpltakes a per-model execute-ordering ticket on the transport thread, before posting to the pool (remote.hpp:354).dispatchMessage's shutdown gate then returned beforedispatchExecute— the only place a ticket was released — so anexecuterefused witherr "server shutting down"dropped its ticket without releasing it.Both
remote.hppanddocs/spec/core/backend.mddocumented this as a benign exception to the "every path that took a ticket must release it" rule, on the grounds thatbeginShutdown()is irreversible, so every laterexecuteis refused at the same gate and none reachesawaitExecuteTurn.That reasoning is wrong, and it misses the window the ordering gate exists because of. Tickets are taken in send order on the transport thread, but the pool may run the two posted tasks in either order — so a later ticket can pass the gate while the earlier one is still upstream of it, and be parked in
awaitExecuteTurn(acv.waitwith no deadline) by the time the earlier one is refused.What it actually costs
Three things, not a leaked map entry:
err "timeout"for a call that never ran, whereLimitPolicy::executeTimeoutis configured;drainedWithin()can never succeed, because_inFlightExecutesis incremented immediately before that wait — so the defect breaks the very graceful-shutdown sequence during which it fires.Measured, on this branch's parent:
The change
dispatchMessage's shutdown branch releases the ticket before replying. The two comments and the spec section that presented the old "benign" analysis are corrected to match — the release rule now holds without exception, which is what makes the rest of that spec section true.The test
tests/test_remote_execute_ordering.cppgains a shutdown-gate case that forces the interleaving rather than racing for it: a wrapping executor (HoldOnePostExecutor) holds back one request's dispatch task, so "the later ticket passed the gate, the earlier one did not" is decided, not hoped for.health().inFlight == 1is the deterministic signal that the later request is parked in the wait, rather than a sleep hoping that it is.This matters — the natural window is nanoseconds wide, and 400 jittered attempts did not hit it. A probabilistic test here would have been a control that passes while measuring nothing.
Because a regression strands a pool worker in a deadline-less wait,
~ThreadPoolExecutorwould block forever injoin()and turn a clean assertion failure into a whole-binary hang. The test deliberately leaks its fixture on the failing path instead.Fails before, passes after — verified in both directions:
Verification
build/gcc-debug): green.check_spec_citations.sh,check_test_type_names.sh,check_deprecated_markers.sh: all pass.Filed separately, not folded in
While fixing this I found and reproduced a second path with the same shape and the same consequence: a throw out of
dispatchExecute(a throwingIAuthorizerhook, ormissingRequiredFieldsunderRequireDeclaredFields) unwinds past everyrejectAndReleaseintodispatchMessage's outer catch, which replies but never releases. This PR does not fix that — per AGENTS.md it is filed as #351 rather than folded in.Two different paths have now missed the same per-call-site convention, which is the argument for the structural fix (an RAII ticket holder) that #351 records. Deliberately out of scope here.