core: parkIfInFrame's double-claim guard is unreachable from a backend now that every dispatch goes through one Completion

[Yaraslaut]

Found while doing #571, filed rather than folded in.

The finding

detail::parkIfInFrame's double-claim arm (include/morph/core/bridge.hpp) is
no longer reachable from any backend:

inline bool parkIfInFrame(AsyncDispatchHandoff& handoff, bool succeeded, ::morph::exec::detail::ModelId modelId,
                          std::exception_ptr failure) {
    bool inFrame = false;
    {
        std::scoped_lock const guard{handoff.mtx};
        if (handoff.fired) {
            // A backend is contractually allowed exactly one callback per dispatch;
            // swallow a second one rather than reporting twice.
            return true;
        }

Before #571, Bridge handed a backend two raw std::functions
(onRegistered, onError) and a backend that violated "exactly one callback"
by calling one of them twice reached this arm. That is what
tests/test_async_registration.cpp's DoubleFiringBackend was written to
exercise ("attachHandlerAsync reports exactly once even when the backend fires
its callback twice inline").

After #571 every dispatch site has the shape

completion.then([...](ModelId newId){ if (parkIfInFrame(*handoff, true, newId, nullptr)) return; ... })
          .onError([...](const std::exception_ptr& f){ if (parkIfInFrame(*handoff, false, {}, f)) return; ... });

over one Completion. morph::async::detail::CompletionState settles once
and once only — Promise::resolve/reject are documented no-ops on an
already-settled state — so exactly one of the two lambdas ever runs, exactly
once. handoff.fired is therefore always false on entry.

Five call sites, all the same shape: attachHandlerAsync, ensureBoundAsync,
assignHandlerPrimary, registerHandlerImpl and rebindThroughSurface.

A second, smaller instance of the same class sits next to it: the
try { ... } catch (...) around the dispatch in attachHandlerAsync and
ensureBoundAsync. IBackend::bindModel's default wraps bindModelBlocking
in its own try/catch and rejects rather than throws, and no in-tree override
throws out of the dispatch call, so only an out-of-tree override can reach
those two catch blocks. In-tree they are reachable only from
tests/test_async_registration.cpp's ThrowingDispatchBackend.

Verification status

Established by reading the code, on laneLADDER-batch-570-571 at
085b401b (#571's commit), not by a coverage run. What is measured is only
that the suite still passes with the twins gone:

tests/morph_tests            22931 assertions in 1556 test cases  (1 failed as expected)
tests/qt/morph_qt_tests        578 assertions in   79 test cases
tests/net/morph_net_tests     1112 assertions in  191 test cases

I did not run scripts/coverage.sh, so I have not confirmed by measurement
that the arm now shows zero hits — which is the evidence that would settle this
either way. No Emscripten toolchain was available either, so nothing about a
WASM configuration is covered.

DoubleFiringBackend was kept and migrated: it now settles the same promise
twice from inside bindModel, so the test still pins the observable contract
(exactly one onDone). What it no longer pins is this guard, because
CompletionState absorbs the second settle first. That substitution is
recorded in the double's own comment and in docs/spec/core/backend.md's
migration-status section, so this issue is not the only record of it.

Why it is worth a ticket rather than a shrug

This repository's branch-coverage gate (scripts/check_branch_coverage.py plus
scripts/branch_partial_allowlist.json) exists to stop exactly this drifting
into an unexplained partial arm that somebody later allowlists with a guessed
reason. Either the arm should be deleted, or it should carry an allowlist entry
whose reason says "unreachable from a backend since morph#571; retained because
parkIfInFrame is also called from the dispatching frame" — and that sentence
should be written by whoever checks it, not inferred from a red gate.

Note that deleting it is not obviously right: parkIfInFrame is a free
function in detail, and the invariant that makes the arm dead is a property of
every current caller, not of the function. A sixth call site that does not go
through a single Completion would resurrect the need.

What would change the verdict

• A coverage run showing the arm taken — then it is reachable by a route I
  did not find, and this should be closed as invalid with that route named.
• A new AsyncDispatchHandoff caller that can settle the handoff twice — then
  the guard is live again and this should be closed.
• A decision to delete the guard, with DoubleFiringBackend either deleted or
  repurposed — then this closes as done.

Not in scope of #571

#571 is a deletion of four IBackend verbs. Deciding whether a now-defensive
guard in Bridge should be deleted, allowlisted or left alone is a separate
judgement about Bridge's internals, and folding it into the removal would
hide it inside a 1000-line diff.

[Yaraslaut]

Triage: valid — confirmed on 0067b5bf, and the tree already disagrees with itself about it

#649 has landed, so this is measurable against master.

The premise holds. All eight parkIfInFrame call sites in bridge.hpp (four pairs — 684/740, 849/892, 1932/1938, 2110/2116) now sit on a Completion's .then/.onError. A CompletionState settles once, so exactly one lambda runs, exactly once, and handoff.fired is false on every entry. The double-claim arm cannot be reached by a backend.

And here is the part worth having in the ticket: tests/test_async_registration.cpp now states both answers. The class comment, written during #571:

*"…the observable contract ("exactly one onDone") while the guard inside parkIfInFrame is no longer reachable from a backend. See the PR for morph#571."

and the comment on the test that drives it, four hundred lines later, unchanged from before #571:

"detail::parkIfInFrame's handoff.fired guard must swallow the second, already-claimed callback rather than letting attachHandlerAsync invoke onDone … twice for a single dispatch."

The second is now false. DoubleFiringBackend::bindModel calls promise.resolve(mid) twice, and the second is a documented no-op on an already-settled state, so .then runs once and parkIfInFrame is entered once. What makes that test pass today is CompletionState, not the guard — the test still asserts the right thing (completions == 1) for a reason its own comment misdescribes.

That is the sharper half of this finding: an unreachable guard is dead weight, but a test whose stated reason is wrong is a trap — the next person to touch parkIfInFrame will read that comment and conclude the guard is load-bearing.

triage: valid, bug, area: core.

For whoever takes it

• Removing the guard needs a mutation test, not a green suite. With the guard deleted, DoubleFiringBackend must still produce completions == 1; if it does, the guard was dead. If it does not, this ticket is wrong and should say so.
• Fix the test's comment whichever way it goes. If the guard is removed, the comment describes a function that no longer exists; if it is kept, the comment still misattributes the mechanism.
• The issue also names the catch (...) around the dispatch as a sibling case reachable in-tree only by a test double. Treat that as a separate decision and say which way you went, rather than sweeping both out together.
• Verification status here: read from the code, plus the two contradicting comments. I did not delete the guard and re-run the suite, which is the measurement that settles it.

What would change the verdict

Close as invalid if deleting the guard makes any existing test fail — that would mean something other than a Completion still reaches it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW