core: stop a cancelled control call reaching the wrapped backend, and pin parkIfInFrame's unreachable double-claim arm (fixes #636, fixes #648)

[Yaraslaut]

Two tickets on the core dispatch/cancellation tree. One behaviour fix, one
"this guard is dead, decide what to do about it" — resolved differently from
either option the ticket offered, with the reason measured rather than argued.

Branched from 0067b5bf. Three commits: one per ticket, one for the six line
hints the edits moved.

---

#636 — a cancelled control call still registered on the wrapped backend

Landed. SynchronousBackendAdapter::cancelPending rejected the completions
the adapter produced (morph#619) and did nothing about the work behind them. A
task still queued on _control reached the head of the strand afterwards and
made its blocking call anyway; its resolve then found the state rejected and
did nothing. The caller was told the bind was cancelled while the registration
went through — a live instance on a backend whose Bridge is gone, or which
switchBackend just replaced, that nothing will ever deregisterModel.

Settling a promise cannot stop a task, so the check is inside the task. Each
dispatch allocates a PendingControl (the promise plus an atomic_bool cancelled); the strand task reads it (acquire) before calling op() and
cancelPending sets it (release) before rejecting. _pending tracks those
records weakly exactly as it tracked bare promises, so the
expiry-means-settled bookkeeping and the amortised compaction are unchanged.

Only the queued-but-not-started window closes. A task already inside op()
still completes — this adapter has no way to interrupt a blocking verb it does
not implement — and that is now recorded in the header and the spec as a limit
rather than as a defect.

The test, and why it is the hard part

A case asserting "the completion was rejected" passes on the pre-fix code and
proves nothing, which is the failure mode AGENTS.md names first. The new case
observes the wrapped backend instead:

• GatedBackend::entered counts entries to a control call, not exits.
• Call 1 holds the strand open inside the wrapped backend, so call 2 is
  provably queued-and-not-started rather than merely unobserved.
• A third call, dispatched after the cancellation, is not one of the
  promises cancelPending snapshotted, so it runs. The strand is FIFO, so its
  arrival at the wrapped backend proves call 2's task has already run and
  declined.
• Assertion: two entries, not three.

It fails before the fix. Disabling the cancelled check and rebuilding,
both sections fail:

tests/test_backend_registration_surface.cpp:835: FAILED:
  CHECK( inner->entered.load() == 2 )
with expansion:
  3 == 2
tests/test_backend_registration_surface.cpp:836: FAILED:
  CHECK( inner->finished.load() == 2 )
with expansion:
  3 == 2

test cases:  1 |  0 passed | 1 failed
assertions: 18 | 14 passed | 4 failed

---

#648 — parkIfInFrame's double-claim guard

Landed, but not as either option the ticket listed. The ticket offered
delete-or-allowlist. The guard is instead kept and tested, which dominates
both: deletion removes a caller-invariant safety net to satisfy a coverage
gate, and an allowlist entry records "nothing reaches this" forever without
ever detecting the day something does.

Measurements

Both on this branch, GCC 16.2.1, Debug, Linux.

1. The arm is unreached — measured directly, not inferred. Replacing its
return true with std::abort():

=== morph_tests ===
test cases:  1556 |  1555 passed | 1 failed as expected
assertions: 22932 | 22931 passed | 1 failed as expected
=== morph_net_tests ===
All tests passed (1112 assertions in 191 test cases)

Zero aborts. handoff.fired is never true on entry. This is stronger than
the coverage run the ticket asked for: a coverage report shows an arm untaken,
whereas an abort() shows the entry condition never holds. It matters here
because the two are not the same — a second park while still in-frame would be
indistinguishable from the first in a deletion test, since both return true.

2. Deleting the arm leaves the suite green, DoubleFiringBackend included
(completions == 1). So the ticket is not wrong. And that is exactly the
problem: dead defensive code whose removal nothing in the tree would notice.

What changed

parkIfInFrame is a free function in detail, and the invariant that makes
the arm unreachable belongs to every current caller, not to the function — a
ninth site that does not park one Completion's outcome would need it again.
So the arm stays, and a new case calls parkIfInFrame directly, twice on one
handoff, asserting the second claim returns true and leaves the first
outcome intact. With the arm deleted, that case and only that case fails:

test cases:  1558 |  1556 passed | 1 failed | 1 failed as expected
assertions: 22956 | 22951 passed | 4 failed | 1 failed as expected

Three statements disagreed, and one was simply false

• tests/test_async_registration.cpp:2160 — "the guard must swallow the
  second, already-claimed callback". Stale since morph#571: what makes that
  test pass is CompletionState, which drops the second settle before any
  Bridge code sees it. Rewritten to say so and to point at the new case.
• docs/spec/core/backend.md:638 — the guard is kept "because parkIfInFrame
  is also called from the dispatching frame". This is false. The
  dispatching frame calls claimHandoff/awaitHandoff; git grep parkIfInFrame finds the definition, eight completion-callback call sites,
  and nothing else. Replaced with the measured reason. Worth flagging on its
  own: the ticket quoted that sentence as a candidate allowlist reason, so
  taking the allowlist route as written would have entered a false statement
  into a file whose whole purpose is to hold checked reasons.
• bridge.hpp's own comment said only that a backend gets one callback. It
  now carries the reachability argument and what it rests on.

The catch (...) sibling — decided separately, left alone

The ticket named the try/catch (...) around the dispatch as the same class
of finding. It is not. Unlike the double-claim arm it is not dead: any
out-of-tree IBackend override that throws out of bindModel reaches it,
IBackend is a public extension point, and ThrowingDispatchBackend already
exercises it in-tree. Deleting it would let an exception escape execute()'s
documented never-throws contract. Covered defensive code, not dead code — no
change, and the spec now says why.

---

Gate repoints (third commit)

Six line hints moved. Two were reported as ambiguous rather than drifted,
and both were resolved by reading the candidates rather than taking the first:

• backend.hpp registerCount: 1087 (LocalBackend::registerModel, which
  the entry's own reason names) vs 1106 (registerModelShared).
• bridge.hpp if (deadlineHandle && schedulerRef): 1679 is the catch
  block that decrements _pendingCalls and cancels the deadline before
  rethrowing, which is what the entry says it covers; 1703 and 1792 are the
  .then()/.onError() continuations the same entry explicitly excludes.

The third (executeInFlight) was ambiguous between the fetch_add at 1251
and the fetch_sub at 1296; the reason says "the increment side". The two
prose line numbers inside the bridge entry's own reason are refreshed with it
(1686/1775 → 1703/1792, and its cross-reference 1540 → 1557) — free text the
gate does not audit, which is why it rots.

---

Review reasoning, inline

Done here rather than through /code-review, which forks background agents.

• Memory ordering. cancelled is release-stored by cancelPending and
  acquire-loaded by the strand task. The flag alone would be fine with relaxed
  (it gates only a branch on an atomic bool), but acquire/release means a task
  that observes the cancellation also observes everything the cancelling
  thread did first, which is what a caller reasoning about ~Bridge ordering
  expects. The promise itself is internally synchronised by CompletionState.
• Order inside cancelPending: flag first, promise second. Reversed, a
  task could read a clear flag, run op(), and then find its promise rejected
  — the pre-fix outcome, for a wider window than necessary.
• The remaining race is narrower, not gone. A task that reads the flag a
  few instructions before the store registers exactly as one already inside
  op(). Stated in the header, the spec and the commit message, because the
  fix's value is the queued window and claiming more would be the thing this
  repository keeps finding.
• _pending's weak-expiry contract is preserved. The strand task holds the
  only shared_ptr to a PendingControl, as it held the only one to the bare
  promise, so entries still expire exactly when their task is destroyed and
  the success path still erases nothing.
• Lifetime. The task captures the shared_ptr<PendingControl> and the
  wrapped backend's shared_ptr, never this — unchanged from before.
• No public API change. PendingControl is private; BindPromise (public,
  documented) keeps its meaning.

Verification summary

What	Result
morph_tests (GCC 16.2.1, Debug, Linux)	1558 cases, 22956 assertions, 1557 passed, 1 failed as expected
morph_net_tests	1112 assertions in 191 test cases, all passed
Clang -Weverything build of morph_tests	clean
clang-tidy 22.1.8 on both changed test TUs	no finding on any line this branch changed (pre-existing findings in quantity.hpp and untouched bridge.hpp/backend.hpp lines remain)
check_mutation_survivors.py	ok — 15 structured citations resolve
check_nolint_directives.sh	ok — 164 directives, all annotating code
check_bidi_controls.py	ok — 1244 files, 0 occurrences
check_spec_citations.sh	ok — 861 references, 66 sections
check_spec_sync.sh (driven with the real path list, not empty stdin)	ok — 10 sub-domains classified
branch_partial_allowlist.json / error_path_allowlist.json resolvers	every entry resolves

Not verified

• No coverage run. check_branch_coverage.py needs an lcov report and was
  not produced here; the allowlist entries were audited with its own
  resolve_allowlist_source_line, which checks the citations but not whether
  a line is still reported partial.
• Linux/GCC and Linux/Clang only. No Windows, no macOS, no Emscripten, no
  sanitizer run. The #636 test is timing-shaped (it holds a strand open), so
  TSan and the slower runners are where it would misbehave if it is going to.
• Qt not built (MORPH_BUILD_QT=OFF), so the abort() reachability
  measurement for core: parkIfInFrame's double-claim guard is unreachable from a backend now that every dispatch goes through one Completion #648 covers morph_tests and morph_net_tests only.
  QtWebSocketBackend settles inline on its !_connected branch, which is the
  case parkIfInFrame exists for — but a double settle there is still
  blocked by CompletionState, which is backend-independent.
• CI not waited on. Reported immediately per the lane's hand-off rule.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner verification

The spec statement really is false, and this is the finding I care most about. Master's docs/spec/core/backend.md:638:

"The guard is kept because parkIfInFrame is also called from the dispatching frame."

Every parkIfInFrame reference in the tree:

bridge.hpp:338   the definition
bridge.hpp:684/740, 849/892, 1932/1938, 2110/2116   eight completion-callback call sites
(comments in bridge.hpp:311, test_async_registration.cpp, test_coverage_gaps.cpp)

and the dispatching frame calls claimHandoff (bridge.hpp:779, :918) / awaitHandoff, never parkIfInFrame. So the sentence is wrong in the file AGENTS.md makes authoritative.

What makes it worth more than a typo fix: #648 quoted that same sentence as a candidate allowlist reason. Taking the allowlist route as the ticket wrote it would have copied a false statement into the file whose entire purpose is to hold reasons somebody checked. Correcting it inside the #648 commit is right — it is the same contradiction the ticket is about, not a separate finding.

My brief asked for the wrong mutation test, and the lane explained why. I said: delete the guard and confirm DoubleFiringBackend still yields completions == 1. That cannot settle reachability — with the arm deleted, an in-frame second park takes the fall-through path, which also returns true, so both worlds look identical from the test. Replacing the arm's return true with std::abort() is the probe that discriminates, and zero hits across 1556 + 191 cases is the answer my version could not have produced.

And the conclusion it reached is better than either option the ticket offered. Deletion removes a caller-invariant safety net to satisfy a coverage gate; an allowlist entry records "nothing reaches this" permanently without detecting the day something does. Keeping the guard and pinning it with a direct-call test dominates both — and the lane demonstrated the gap it closes: with the arm deleted the whole suite stays green including DoubleFiringBackend, so today nothing at all detects its removal. That is dead defensive code whose disappearance no test notices, which is a real defect even though the ticket named the wrong remedy.

Deciding the catch (...) sibling separately and leaving it alone is also right, and for a stated reason rather than by omission: it is reachable by any out-of-tree IBackend throwing out of bindModel, IBackend is a public extension point, and ThrowingDispatchBackend exercises it. Covered defensive code, not dead code.

I verified #655 myself rather than relaying it:

$ echo -n "" | bash scripts/check_spec_sync.sh
Spec sync OK: the change touches no files.
  exit=0
$ git diff --name-only HEAD~1 HEAD | bash scripts/check_spec_sync.sh
Spec sync OK: 10 sub-domain(s) classified; every touched header sub-domain has a matching spec change.
  exit=0

Reproduces. Worth noting in its favour that the empty-input message is honest — it says "touches no files" rather than claiming to have checked something — so the exposure is a broken upstream pipe rather than a lying gate. The lane labelled the severity half as weak, correctly. And it re-ran the gate with a real path list before resting the branch's claim on it, which is the part that matters here.

Not verified by me: the abort instrumentation and the #636 before/after (both need a full build), and anything about coverage — check_branch_coverage.py needs an lcov report, so whether #648's arm was an unaccounted partial line before, and is now covered, remains inferred as the lane says. The #636 test is timing-shaped, holding a strand open; TSan and the slower runners are where that will show if it is going to.

Not merged: CI incomplete at hand-off. Next sweep counts the checks.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner: clang-format was red; fixed in 0644d6cf

The clang-format leg failed on 9 violations, all on lines this branch added:

tests/test_async_registration.cpp:2206:23        auto const first =
tests/test_async_registration.cpp:2211:62        parkIfInFrame(
tests/test_backend_registration_surface.cpp:773  TEST_CASE("… cancelPending stops a queued …
tests/test_backend_registration_surface.cpp:800  adapter.promoteModel(PromoteRequest{…
(and five more)

Read from the run's clang-format-report artifact, since the run was still in progress.

It is not a version skew, and that matters because the branch's notes imply one. ci.yml pins CLANG_VERSION: "22" and the clang-format job installs that same pin; the local binary is clang-format 22.1.8. Running it over the two files here reproduces all nine. (The "CI pins 20" note that has been circulating in lane reports is wrong — worth correcting before someone builds on it.) So the pre-push clang-format --dry-run -Werror this branch reported as green cannot have covered these two files.

Fixed by running the pinned formatter over exactly those two files. Whitespace only — verified rather than assumed:

tests/test_async_registration.cpp              tokens identical: True
tests/test_backend_registration_surface.cpp    tokens identical: True

(both files compared before and after with all whitespace stripped, so no test, assertion or string literal changed).

Fixing this here rather than dispatching a lane: the diagnosis is complete, the remedy is mechanical, and the alternative is a full CI cycle of waiting to hand someone a one-command job. The rest of the branch's verification is unaffected — the formatter touched no tokens.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
include/morph/core/backend.hpp	93.33%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Yaraslaut]

Runner, sweep 06:41 — one failure diagnosed, one deliberately left alone

gh pr checks 654: 49 pass, 2 fail, 2 pending. The clang-format leg I fixed in 0644d6cf is green; these are different.

clang-tidy-diff — diagnosed, trivial. From the run's clang-tidy-report artifact:

include/morph/core/backend.hpp:953:45: error: parameter name 'p' is too short,
    expected at least 3 characters [readability-identifier-length,-warnings-as-errors]
  953 |         explicit PendingControl(BindPromise p) : promise{std::move(p)} {}

One parameter in the new PendingControl constructor. A rename fixes it.

Application ladder — not diagnosed, and I am not guessing. It failed after 48m56s, and the job log is unreachable while the run has 2 checks outstanding.

I am deliberately not pushing the clang-tidy fix yet. ci.yml sets cancel-in-progress: true on the concurrency group, so a push now cancels this run — and a cancelled run's logs are purged, which this repository has already cost itself once (#630, where five red legs left nothing to read). That would trade a one-line fix for the only evidence about a 49-minute failure.

So the order is: let this run finish, read the Application ladder log, then push the rename together with whatever that failure needs. The next sweep does both.

Worth flagging for that diagnosis: this branch's #636 test is timing-shaped — it holds a strand open so a second call is provably queued-and-not-started — and the lane that wrote it said plainly that "TSan and the slower runners are where it would misbehave if it is going to." Application ladder is one of the slower legs. That is a hypothesis to test against the log, not a conclusion: the ladder leg could equally be failing for a reason unrelated to this branch, and it has no local reproduction yet.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner: both failures resolved — and the ladder one was never this branch's

The run completed, so the Application ladder log became readable. My earlier hypothesis was wrong and I am glad I labelled it as one: I suggested the timing-shaped #636 test might be misbehaving on a slower runner. It is not. Every ladder test passed:

100% tests passed, 0 tests failed out of 1031

The failing step was Run the scenario corpus against the built servers, and the cause is a SQLite lock:

ledger: 16 file(s) against ws://127.0.0.1:35615
FAIL step 35 (line 140): do SetCategory accountId=$spend categoryId=$loser ruleId=$rule ruleVersion=7
  expected: @kind == ok
  actual:   @kind == err
  reply:    err message="HY000 (5) - [SQLite]database is locked (5)"
  FAIL update-rule-version-conflict.scenario
FAILED: ledger

This branch touches SynchronousBackendAdapter and parkIfInFrame; the ledger scenario server goes near neither. Filed as #658 with the full log excerpt, labelled observed once in CI, not reproduced — including the detail I could not explain, that step 35 appears twice, ok at line 139 and FAIL at line 140.

clang-tidy-diff fixed in 180edc99. The one finding this branch introduced:

include/morph/core/backend.hpp:953:45: error: parameter name 'p' is too short,
    expected at least 3 characters [readability-identifier-length]

Renamed p → dispatched, and the @param line with it — the Docs workflow runs Doxygen with WARN_AS_ERROR=FAIL_ON_WARNINGS, so a @param naming a parameter that no longer exists would have traded one red leg for another. The file's line count is unchanged (1442 → 1442), so no citation in either line-hint allowlist moves; both resolvers still pass, as does the NOLINT gate.

On the sequencing: I held this one-line fix back last sweep rather than pushing it, because ci.yml sets cancel-in-progress: true and a push would have cancelled the run mid-flight, purging the very log that turned out to contain the answer. That cost one sweep of latency and bought the actual diagnosis — without it I would have pushed a rename, watched the ladder go red again, and still not known why.

Fresh CI is running. If Application ladder is red again on the same scenario, that is #658 recurring and this PR is mergeable regardless; if it is green, #658 stays a single observation.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW