Fix a permanent RemoteServer deadlock and a lost completion value - #546
Merged
Merged
Conversation
Member
Author
|
Status: the two fixes (#519, #520) are committed, individually reviewed, and verified — full suite green at The tip commit ( Do not merge past |
Member
Author
|
Status update — this batch is now feature-complete and heavily verified. Summary of the 4 commits:
Verification: full suite green throughout (1442 test cases, only the same pre-existing, unrelated No rebase needed — Still draft pending your review — marking it ready is your call, not mine. |
Yaraslaut
force-pushed
the
batch/519-520
branch
from
September 15, 2026 21:28
f1c6e7c to
310e742
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Yaraslaut
force-pushed
the
batch/519-520
branch
from
September 16, 2026 17:13
7d07551 to
5ff137a
Compare
RemoteServer::handleImpl took an execute-ordering ticket and posted the dispatch work to the worker pool as two separate, unlocked steps. Two transport threads calling handle() concurrently for the same model could take tickets in order but race to enqueue: if the later ticket's task reached the pool's FIFO queue first, a pool worker picked it up, blocked in ExecuteOrderGate::awaitTurn waiting for the earlier ticket, and -- with a pool no larger than the number of same-model callers -- every worker ended up parked that way while the earlier ticket's task never got a worker to run on. That is exactly the topology morph::net::SocketServer uses (one thread per connection, all calling handle()), which registerModelShared exists to support. Add ExecuteOrderGate::takeAndPost, which holds a per-model enqueue lock (Gate::enqueueMtx, separate from the gate's own ticket-bookkeeping mutex) across assigning the ticket and invoking the caller's enqueue callback, so two concurrent callers' take-then-enqueue sequences for the same model can never interleave. Per-model rather than global, and the enqueue lock is released before the ticket-bookkeeping mutex is ever touched, so a synchronous executor running the whole dispatch chain (including a re-entrant awaitTurn/release, or a nested takeAndPost for a different model) inline cannot self-deadlock. New regression test forces the exact interleaving deterministically (a StallFirstPostExecutor wrapping a pool of one) rather than racing real threads; verified it fails (times out) against the pre-fix code and passes against the fix. Fixes #519 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
… value CompletionState<T>::setValue moved out of its own value optional to build the settle-time fan-out closure for handlers attached before settling, leaving value engaged but holding a moved-from T. A then() attached after settling (attachThen's ready && value branch) tested value's engaged flag, saw it true, and copied the moved-from husk instead of the real value -- silently, with nothing logged. The onError counterpart never had this bug because setException copies its error rather than moving it. Copy from the setValue parameter instead, and do it before onOk is drained or value/ready are mutated, so a throwing T copy constructor leaves the state exactly as it was (onOk intact, still unready) rather than a state that already looks settled with its handlers already lost and the exception escaping through Promise::resolve(), which is documented only as a no-op. New regression test attaches one handler before settling and one after, and asserts both observe the same value; verified it fails (the second handler sees an empty string) against the pre-fix code and passes against the fix. Fixes #520 Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
…MERGE Whole-branch /simplify pass per lib/team-protocol.md. Introduces a real regression: tests/test_server_limits.cpp's "benchmark: in-process execute round-trip" (tag [!benchmark], hidden unless a matching tag filter like [remote] is passed explicitly) hangs -- a pool worker parks forever in ExecuteOrderGate::awaitTurn. Confirmed via `sample`: not present at 6da1dc1 (before this pass); the same benchmark passes cleanly there. Root cause not yet isolated between the three changes in this commit. Full investigation notes, what's been ruled out, and next steps are in HANDOFF.md. Do not build on top of this commit's changes to execute_order_gate.hpp, remote.hpp or completion.hpp until the hang is root-caused. 6da1dc1 remains the last known-good state. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
…lookup The previous commit's takeAndPost fix for #519 let a same-model producer run well ahead of the pool workers -- exactly what closed the original deadlock. It also opened a much wider window on a second, pre-existing hazard in ExecuteOrderGate: awaitTurn(mid, ticket) and release(mid, ticket) both re-derive "the gate for mid" via a fresh map lookup, which is only correct if the entry found is still the same Gate the ticket was issued from. A model's entry is erased once it fully drains; a later ticket for the same model, still in flight (its dispatch task enqueued but not yet reached by a worker), can find a newer Gate already sitting where the old one used to be and wait on that gate's independent counter for a ticket number it will never produce. Reproduced reliably under concurrent load (6/6 runs of tests/test_server_limits.cpp's hidden benchmark hung); did not reproduce at all on unfixed master, confirmed via a scratch instrumented build showing multiple distinct Gate objects created for one ModelId within a single run, and confirmed by disabling the erasure entirely as a diagnostic (6/6 passed). Fix: ExecuteOrderGate::takeAndPost now hands out an opaque Ticket bound to the exact Gate shared_ptr it came from. New awaitTurn(Ticket)/release(Ticket) overloads operate on that object directly, never a fresh lookup, so a drain-and-recreate of the map entry cannot redirect them. ExecuteTicketGuard and RemoteServer's dispatch path carry Ticket end to end instead of a bare (ModelId, ticket) pair. The existing take(mid)/awaitTurn(mid,.)/release(mid,.) trio is kept, documented as still carrying this hazard, for test_execute_order_gate.cpp's direct tests of the gate's raw counter state machine (which never cross an asynchronous gap) and takeAndPost's own internals (which never let the gap open). Verified against the same load that reliably hung before this fix: 18/18 runs across three rounds of 6 concurrent processes now pass. Full suite green (1440 cases, +1 new regression test for the drain-and-recreate scenario). Note: a narrower variant of the same race class remains in takeAndPost's own gate acquisition (resolving the Gate for a model and locking its enqueueMtx are two separate critical sections) -- flagged during review, not fixed here. It does not reproduce the hang this commit closes; closing it needs a design decision (e.g. a validate-and-retry loop) this batch's scope doesn't cover. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Written mid-investigation so another session could pick up the gate-erasure race if this one ran out of room. The branch is complete now and the commit history carries the same information. Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
…lease
The previous commit's Ticket redesign closed the hang from an erased-and-
recreated Gate, but a final whole-branch review surfaced two more severe
hazards in the same shape -- takeAndPost holding a per-model lock across
postFn's entire, potentially-reentrant, synchronous execution:
- Self-deadlock: with a synchronous (inline) executor -- used as _pool in
15+ existing test files, and the shape RemoteServer's own docs describe
as supported (a model handler calling back into handle() via
SimulatedRemoteBackend::execute()) -- a same-model reentrant takeAndPost
call on the same thread tried to re-lock Gate::enqueueMtx, a plain
std::mutex, and hung forever.
- Corrupted out-of-order state: a non-std::exception throw escaping
dispatchMessage's narrower `catch (const std::exception&)` unwinds
through both its local ExecuteTicketGuard's destructor and takeAndPost's
own `catch (...) { release(ticket); throw; }`, releasing the same
logical ticket twice. The second release inserted an already-passed
ticket number into releasedOutOfOrder, where it sits as the set's
permanent minimum and silently breaks every future out-of-order release
for that gate -- issue #449's own fix, defeated from the other end.
Fix, addressing the root cause rather than each symptom:
- Gate::enqueueMtx is now a std::recursive_mutex. Same-thread re-entrancy
is never concurrent with itself, so it needs no ordering against itself,
only exclusion against other threads -- exactly what a recursive mutex
gives, without the self-deadlock a plain one forces.
- Ticket carries a shared, atomic "already released" flag, checked and
claimed by release(Ticket) before doing any work. Two independent owners
of the same logical ticket can each call release(); only the first one
actually runs the release logic, so a double-release is a safe no-op
instead of a corrupting one.
- takeAndPost's gate-fetch, enqueueMtx acquisition and ticket increment now
happen in one continuous _mtx hold (previously two separate critical
sections), closing a narrower TOCTOU window a review also found: a
concurrent release() draining and erasing the gate between "fetched" and
"ticket assigned" could mint a ticket on an already-orphaned generation.
Also: dropped a redundant `this` capture in handleImpl's doPost (self
already reaches _pool), and corrected docs/spec/core/completion.md, which
still described setValue's pre-#520 moved-from-value behavior as intended
semantics.
New regression tests: same-model reentrant takeAndPost no longer
deadlocks; releasing the same Ticket twice no longer corrupts
out-of-order state. Full suite green (1442 cases, +2), and the load
scenario that previously hung under concurrent CPU contention passes
6/6 again with this design.
Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Yaraslau Tamashevich <yaraslau.tamashevich@gmail.com>
**clang-tsan.** ThreadSanitizer reported `lock-order-inversion (potential deadlock)` on the new same-model reentrancy test, and it is a genuine cycle, not a sanitizer artefact. takeAndPost held `_mtx` while acquiring `gate->enqueueMtx`, then carried `enqueueMtx` across postFn -- whose re-entrant takeAndPost needs `_mtx` while still holding `enqueueMtx`. Thread A re-entering holds enqueueMtx and waits for _mtx; thread B in a plain takeAndPost for the same model holds _mtx and waits for enqueueMtx. Neither proceeds. Fixed by establishing one global order -- enqueueMtx is always acquired first, `_mtx` only after. The ticket increment cannot simply move earlier to avoid re-acquiring `_mtx`: minting the number outside enqueueMtx would let two threads take numbers 0 and 1 and enqueue in the opposite order, which is the take-then-enqueue atomicity this gate exists to provide. Splitting the hold reopens the TOCTOU the single hold closed, so rather than prevent it the code now detects it: after enqueueMtx is held, the map is re-checked under `_mtx` and a gate that is no longer the registered one is abandoned and the sequence retried. The re-entrant path never retries and so never drops a lock its caller holds -- the outer frame's ticket is still outstanding, so the gate cannot report itself drained and its entry cannot have been erased. **clang-tidy-diff.** `(void)pool.release()` trips bugprone-unused-return-value, which a cast to void does not satisfy. Now a named `[[maybe_unused]] auto* const leakedPool`, which says the same thing. Applied to all four sites in the file, not only the linted one, so the deliberate-leak idiom stays spelled one way. **clang-coverage.** Three branch_partial_allowlist.json line hints for remote.hpp had drifted (1433->1447, 1511->1525, 1352->1366) because this branch added lines above them. The stored `source` text still matches at the new lines, so the dispositions themselves are unchanged. Verified locally: the reentrancy test reports the lock-order inversion under clang-tsan before this commit and is clean after; morph_tests 1,442 cases green; clang-tidy-diff clean on changed lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…drift A /code-review pass found 15 issues. This commit takes the ones that are unambiguous correctness or are doc drift I introduced; the structural findings are left for a decision and listed at the end. **awaitTurn(Ticket) could park a pool worker forever.** The predicate was `nextToRun == ticket._number`, an exact match with no deadline. `Ticket` is deliberately copyable with two owners that can each release it, so if takeAndPost's `catch (...)` releases after postFn already enqueued a task holding its own copy, `nextToRun` advances past that number and the task's awaitTurn waits on a value it will never see again. Now `>=`: once nextToRun has moved past this ticket, its turn has come and gone. This mirrors the by-ModelId overload, which already returned immediately once the entry was gone. Compounding it, advanceOnReleaseLocked returned on the fully-drained path *before* cv.notify_all(), so a waiter parked on an already-released ticket was never even woken to re-evaluate. The notify now happens first. **release(Ticket) claimed `_released` before the bookkeeping that can throw.** `releasedOutOfOrder.insert()` is a std::set node allocation. With the flag already set, a bad_alloc there left the ticket permanently un-releasable by every owner -- nextToRun never reaches nextTicket, the gate entry is never erased, and every later awaitTurn for that model blocks forever. The claim now happens under `_mtx` and *after* the bookkeeping succeeds; every release serialises on that mutex, so it is still exactly-once, and a throw now leaves the flag clear for the other owner to retry. **A throw between minting a ticket and owning it stranded the number.** `Ticket`'s constructor called make_shared for its shared `_released` flag, so a bad_alloc after `nextTicket++` -- reachable, the repo ships an OOM injector -- left a number nothing could release, with the same permanent-stall consequence. The flag is now allocated before the number is minted and passed in. Same fix in takeTicket. **setValue's reordering created the hazard its own comment disclaimed.** The sequence drained `onOk` into a local before `value = std::move(val)`, so a T with a throwing move constructor unwound with every handler in that local's destructor while `onOk` was already empty: permanently unsettled, no handlers, and silent, because the orphan logger only fires when `error` is set. Draining last restores the strong guarantee for moves as well as copies, and costs nothing -- moving a vector is noexcept and savedVal is already an independent copy. **The morph#520 test only covered the degenerate arm.** One pre-settle handler means savedFns.size() == 1, so the fan-out loop never ran and only the `back()(std::move(savedVal))` arm was exercised -- while the fan-out is exactly where savedVal is read repeatedly. A second pre-settle handler covers both. **Two doc blocks asserted the opposite of the code, and of each other.** The @file block still read "Nothing here is new logic" (morph#519 added Ticket, takeTicket, a per-Gate recursive_mutex and a two-phase lock protocol), and takeAndPost's @brief still described "one continuous `_mtx` hold" -- contra its own body comment forty lines below, which my earlier CI fix had added. A maintainer trusting the @brief would delete the stale-generation re-check as redundant and reintroduce the TOCTOU it exists to detect. Also verified and dismissed: the review claimed this branch breaks the error-path coverage gate. It does not -- check_error_path_coverage.py runs only as `--self-test` in drift-guard.yml, and coverage.sh invokes only check_branch_coverage.py. CI on this branch is 49/49 green, which settles it. (An earlier review pass refuted the same theory.) Deliberately NOT addressed here, as they are design decisions rather than defects to patch: enqueueMtx being held across the caller-supplied postFn (a cross-model ABBA hazard and a same-model ticket deadlock on the inline executors used in-tree), the resulting blocking behaviour of a handle() that is documented as async, and the backend.md spec drift that follows from it. Verified: morph_tests 1,442 cases green; gate/ordering/completion suites clean under clang-tsan with no lock-order inversion (the two races reported at tests/test_server_limits.cpp:192 are pre-existing and byte-identical on master -- this branch does not touch that file); clang-format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
readability-simplify-boolean-expr on the if/return-true/return-false shape my previous commit left behind when it hoisted notify_all() out of the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The code review claimed takeAndPost's per-model enqueue mutex allowed a cross-model ABBA deadlock. It does. Reproduced with a two-thread probe against the plain gate -- no RemoteServer, no timing race, a 5s deadline: per-model (pre-fix) : cross-model nesting DEADLOCKED gate-wide (fixed) : cross-model nesting COMPLETED And it is a regression, not a pre-existing hazard: master has no takeAndPost and no enqueue mutex at all, so it holds no lock across a caller callback and cannot form the cycle. Measured separately, master's equivalent probe runs to completion. The cause is that `postFn` is opaque. On a ThreadPoolExecutor it only enqueues; on a synchronous executor it runs the whole dispatch chain inline, including any re-entrant takeAndPost. Synchronous executors are not hypothetical -- morph::testing::InlineExecutor is RemoteServer's pool in test_policy_hardening, test_register_authorization, test_pinned_facts and five examples/concepts files. A mutex per model therefore hands two threads two locks to take in opposite orders. One mutex cannot form a cycle with itself. It stays recursive, because the same thread can legitimately re-enter through a synchronous executor. This also *simplifies* the gate. Because the enqueue mutex is now acquired before `_mtx` and never inside it, the gate fetch and the ticket increment go back into one continuous `_mtx` hold -- which closes the drain-and-recreate TOCTOU outright, so the stale-generation re-check and its retry loop are gone. The cost is real and documented rather than hidden: takeAndPost now serialises across every model, not per model, and on a synchronous executor that serialises the dispatch those callbacks perform. A caller that blocks inside postFn now blocks every other takeAndPost. That makes a re-entrant same-model awaitTurn -- a ticket waiting on one its own caller has not released -- worse, and it is recorded in backend.md as documented misuse. It is worth noting that case already deadlocks on master (measured), so this widens a pre-existing hang rather than creating one. The regression test deliberately does not require both threads to be inside postFn at once: a gate-wide lock prevents that by design, so a barrier would deadlock on itself and prove nothing. It runs 200 nested rounds per thread in opposite model order and leaks the fixture on the hang path, so a regression reports as a failed assertion rather than a hung binary. Verified to fail against per-model locking and pass against the fix. backend.md updated for all of it, plus three statements the review found already stale: handleImpl no longer holds a guard across the pool post (takeAndPost folded them), disarm() has no production caller, and the two awaitTurn overloads differ in whether a vanished gate entry returns early. Verified: morph_tests 1,443 cases green; gate/ordering/remote/completion clean under clang-tsan with no lock-order inversion; clang-tidy-diff, clang-format and the prose lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `sccache stats` step is skipped whenever the probe puts a job on the
self-hosted fleet -- sccache is not the launcher there, fastcache-cc is -- and
nothing replaced it. So those legs ran with no cache reporting at all: exactly
the state that step's own comment warns about, "a thrashing cache and a working
one look identical from the outside, and only the build-step duration hints at
which one you have."
Added to the five jobs that actually use fastcache-cc (linux-compilers,
linux-sanitizers, ladder-tests, ladder-sanitizers, linux-all-features), gated
on the same `contains(needs.probe-self-hosted.outputs.runs_on, 'self-hosted')`
condition the sccache step negates, so exactly one of the two runs per job.
linux-sanitizers had no cache step of any kind. The other four stats steps in
the workflow (linux-coverage, kanban-tsan, linux-qt, valgrind) are on jobs that
never take the fastcache path and are left alone.
What prompted it: `Linux / gcc-debug` on a self-hosted runner spent 8m03s of
its 9m39s in Build, while configure reported
-- [cache] Enabling fastcache-cc at host.docker.internal:6674 ... for C/C++
"Enabled" is the only thing currently observable. Whether a single compile is
served from the cache is measured nowhere.
The line that answers it is
unavailable : N (x% of all compiles -- CACHE NOT REACHED)
which is how a daemon that is configured but never answering shows up.
Configure still prints "Enabling" in that case and the build simply takes as
long as a cold one, so nothing else in the job would reveal it. A high
`unavailable` and a high `misses` need opposite fixes, which is why this
measures before anything is changed.
A self-hosted runner with no launcher on disk emits a `::warning::` rather than
passing quietly -- that combination means the build ran uncached, which is the
thing worth surfacing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Yaraslaut
marked this pull request as ready for review
September 16, 2026 18:43
Yaraslaut
force-pushed
the
batch/519-520
branch
from
September 16, 2026 18:43
5ff137a to
bc8ee94
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The gate can still deadlock during same-model inline re-entry, and several regression and CI checks are unreliable.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes RemoteServer execute-order deadlocks and preserves completion values for late handlers.
Changes:
- Adds atomic ticket-and-post ordering with regression coverage.
- Preserves settled completion values and documents semantics.
- Adds self-hosted cache telemetry, outside the stated PR scope.
File summaries
| File | Description |
|---|---|
include/morph/core/remote.hpp |
Integrates atomic execute scheduling. |
include/morph/core/detail/execute_order_gate.hpp |
Adds bound tickets and concurrency controls. |
include/morph/core/completion.hpp |
Preserves values during fan-out. |
tests/test_remote_execute_ordering.cpp |
Tests concurrent RemoteServer execution. |
tests/test_execute_order_gate.cpp |
Tests ticket and locking behavior. |
tests/test_completion_multi_handler.cpp |
Covers late completion handlers. |
docs/spec/core/backend.md |
Documents ordering semantics. |
docs/spec/core/completion.md |
Documents completion copy behavior. |
scripts/branch_partial_allowlist.json |
Updates shifted source lines. |
.github/workflows/ci.yml |
Adds fastcache telemetry. |
Review details
Suppressed comments (2)
.github/workflows/ci.yml:1562
- This filtered job has the same missing
steps.filter.outputs.run == 'true'guard. As written, a skipped self-hosted job reports stale/global fastcache statistics despite compiling nothing, so the telemetry is attributed to the wrong leg.
if: "always() && contains(needs.probe-self-hosted.outputs.runs_on, 'self-hosted')"
include/morph/core/detail/execute_order_gate.hpp:118
- This documentation describes an
exchange(true)before release work, but the implementation does the opposite: it checks under_mtx, advances bookkeeping, then storestrueso allocation failure remains retryable. Update the comment because the stated ordering contradicts the exception-safety invariant implemented below.
// both). `release(Ticket)` claims this flag with `exchange(true)`
// before doing any work, so only the first caller actually runs the
// release logic -- a second call is a silent no-op instead of
- Files reviewed: 10/10 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // First, and with no other lock held. Held across postFn, including any | ||
| // re-entrant takeAndPost it triggers. | ||
| std::scoped_lock const enqueueLock{_enqueueMtx}; |
| // when `error` is set. `std::move` on a vector is noexcept and | ||
| // `savedVal` is already an independent copy, so the reordering | ||
| // costs nothing. | ||
| auto savedVal = val; |
| // affect the fixed code's correctness, which holds regardless of scheduling, but | ||
| // maximises the chance of reproducing the inversion when this test is run | ||
| // deliberately against the pre-fix code. | ||
| std::this_thread::sleep_for(std::chrono::milliseconds{50}); |
| # Mirror of the sccache step's condition, so exactly one of the two runs. | ||
| # `|| true` for the same reason it has one: a stats failure must never | ||
| # fail the job. | ||
| - name: fastcache-cc stats |
| Ticket() = default; | ||
|
|
||
| /// @brief True if this handle carries no ticket. | ||
| [[nodiscard]] bool empty() const noexcept { return _gate == nullptr; } |
| /// use) is held. If it throws, the ticket is released before | ||
| /// the exception propagates, exactly as if it had never been | ||
| /// taken. | ||
| template <typename PostFn> |
…a CI guard Three of the seven review comments held up under checking; the CI one was a real defect. - tests/test_completion_multi_handler.cpp: `setValue` documents a strong exception guarantee against a throwing `T` copy constructor -- the copy morph#520 introduced is taken before `onOk` is drained or `value`/`ready` are set -- and nothing tested it. The suite only covered throwing *handlers*. Adds `ThrowOnCopy` and asserts the whole claim: the state stays unready, `value` stays disengaged, both handlers stay attached, and a later non-throwing settlement still runs them. The guarantee holds as written; it is now pinned. - .github/workflows/ci.yml: the `fastcache-cc stats` steps in `ladder-tests` and `ladder-sanitizers` omitted `steps.filter.outputs.run == 'true'`, which their own comment claims they mirror from the sccache step beside them. On a self-hosted run where the ladder is filtered out, they reported process-wide cache statistics as if they belonged to a leg that built nothing. The other three fastcache steps live in unfiltered jobs and are left alone. - tests/test_remote_execute_ordering.cpp: "Forced, not raced" overclaimed. Thread A's stall inside post() is forced; thread B winning the window before A is released is not, and cannot be -- the fix holds `_enqueueMtx` across postFn, so a handshake waiting for B's own post before releasing A would wait forever against the fixed code. Says that instead, and scopes the sleep to what it is: an aid for reproducing the inversion by hand against pre-fix code, not part of what CI asserts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ence performance-unnecessary-value-param, on the two handlers added in the previous commit: neither reads its argument, so a by-value parameter is a copy per invocation that buys nothing. `std::function<void(T)>` accepts a handler taking `const T&` just as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 the two correctness defects the framework review sweep (#518) called out to fix first, from the sweep's R core-async lane: a permanent deadlock when two transports drive the same model instance, and a fan-out completion handler silently receiving a moved-from value.
Changes
RemoteServer::handleImpltook an execute-ordering ticket and enqueued the dispatch work as two separate, unlocked steps, letting two concurrent same-model callers' tickets and enqueue order diverge and park every pool worker permanently.ExecuteOrderGate::takeAndPostmakes the two atomic, per model.CompletionState<T>::setValuemoved out of its ownvaluestore to build the settle-time fan-out closure, leaving athen()attached after settlement to copy a moved-from husk. Settling now copies instead, with the copy taken before any state is mutated so a throwingTcopy leaves the completion unchanged rather than partially settled.