flows: ~FlowSession's doc claims a cross-thread teardown guarantee CallbackScope explicitly does not give

[Yaraslaut]

include/morph/forms/flows.hpp's destructor comment makes two claims in one
paragraph that cannot both be true:

/// @brief Refuses every callback this session installed.
///
/// There is nothing to detach. A step's callbacks are `.then`/`.onError`
/// continuations on the `Completion` that `BridgeHandler::execute<A>()`
/// returned — already owned by the in-flight dispatch, not held in a map
/// this session could remove itself from, and possibly running on another
/// thread the moment this destructor starts. `_callbacks` gates every
/// installed callback on a token the callback checks *before* touching
/// `this`, so a callback still in flight when this destructor runs is
/// refused instead of touching a partially- or fully-destroyed object.
~FlowSession() { _callbacks.requestStop(); }

The first half is right: a continuation may be running on another thread when
the destructor starts. The second half — that such a callback "is refused
instead of touching a partially- or fully-destroyed object" — is stronger than
anything CallbackScope provides in that situation.

Why it is not true as written

requestStop() is a single store (include/morph/core/callback_scope.hpp):

void requestStop() noexcept {
    if (_state != nullptr) {
        _state->stopped.store(true, std::memory_order_release);
    }
}

and the token's own documentation is explicit that the check is not a barrier:

/// @brief Whether a callback gated on this token may run right now.
///
/// Advisory across threads: true here means "was active at the moment of
/// the check". See `CallbackScope`'s "Boundary of the guarantee".
[[nodiscard]] bool active() const noexcept { ... }

Nothing drains. A continuation that read Active a moment before
requestStop() lands goes on to run captureResult, which takes
FlowSession::_mtx and writes _resolvedValues — on an object whose
destructor has already returned.

The full check-then-run guarantee holds only for executor-affine teardown:
destroy the session on the thread the continuations are delivered on, and the
delivery cannot be concurrent with the destructor. FlowSession is not
constrained to that. Its own member comment says as much, twenty lines below:

// ...the state a step's result/error continuation also
// touches, which runs on whatever thread/executor resolves the underlying
// BridgeHandler completion -- not necessarily this same thread.

So the class knows its continuations are cross-thread, and its destructor
comment promises a guarantee that only applies when they are not.

Why it is worth fixing rather than leaving

A reader who takes the comment at face value destroys a FlowSession from a
GUI thread while a pool-thread continuation is mid-captureResult, and gets a
data race and a use-after-free that no test will reliably show. That reader
already existed: tests/test_flows_apps.cpp's no-coalescing case was written
this way and fixed in 820b2953 on lane/d-framework — five dispatches in
flight, an inline executor putting their continuations on pool threads, and the
scope ending without draining them.

Also stale in the same comment

/// `requestStop()` is called explicitly rather than left to the member's own
/// destruction ... so anything this body does that can pump an event loop
/// (a `sendSync`-style blocking call) would otherwise deliver into a
/// half-dead session.

The body is _callbacks.requestStop(); and nothing else, so it pumps no event
loop. The rationale describes a hazard the code no longer has.

Suggested shape

Qualify it the way callback_scope.hpp does rather than deleting it: full
refusal when the session is destroyed on the delivery executor's thread;
advisory otherwise, with the caller responsible for not destroying the session
while a reply may still be delivered. If FlowSession is meant to be
destroyable from any thread with a real guarantee, that is a different and much
larger change — a drain — and should be its own ticket.

Verification status

Read, not run. The claim is about what requestStop() does and what
CallbackToken::active() documents; both are quoted above from
origin/master as merged. No race was reproduced under TSan, and none is
needed to see that the comment overstates the API it is built on.

What would close this: the comment says what CallbackScope guarantees,
including which thread the session must be destroyed on for the strong reading
to hold — or ~FlowSession acquires a drain that makes the strong reading true.

Found while working morph#432. Filed rather than folded in, per AGENTS.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_0154xzWuBMPveLcdeUgydifb

[Yaraslaut]

Triaged: triage: valid.

Verified the premise directly against origin/master, independent of the
issue's own quotes:

• include/morph/core/callback_scope.hpp's "Boundary of the guarantee" section
  states outright: "Cross-thread stop is advisory. A scope torn down on a
  different thread from the delivery executor can turn inactive between the
  token's check and the callback body... external synchronisation there
  remains the caller's job." docs/spec/concurrency_and_lifetimes.md repeats
  the same split (executor-affine = full guarantee, cross-thread = advisory
  only).
• Completion::then(const CallbackScope&, ...) /
  onError(const CallbackScope&, ...) (include/morph/core/completion.hpp)
  compile down to token.guard(handler) — a plain status() == Active check
  immediately followed by invocation. No lock, no drain, nothing that makes
  check-then-run atomic across threads.
• FlowSession's own member comment, twenty lines below the destructor
  (include/morph/forms/flows.hpp:486-490), already says the continuation
  "runs on whatever thread/executor resolves the underlying BridgeHandler
  completion — not necessarily this same thread." So the class's own
  documentation contradicts its destructor's documentation.
• c49015b6 (already on master) independently hit and fixed exactly this
  race in tests/test_flows_apps.cpp's no-coalescing case — five dispatches
  left in flight with an inline pool-thread executor, destructor calling only
  requestStop(), and a continuation that had already passed its token check
  going on to run captureResult against a torn-down FlowSession. That
  commit's own description independently arrives at "CallbackToken::active()
  ... advisory across threads" as the reason, corroborating this issue's
  reasoning from a different investigation.

No TSan reproduction was run (matching the issue's own "read, not run" caveat
on the code-reading half) — but the "would this still pass with the feature
doing nothing" check doesn't apply here; this isn't a test/gate issue, and the
premise is confirmed by direct inspection of the executed guard logic
(CallbackToken::guard), not just by reading prose.

Verdict rationale: triage: valid, not rescope — the issue already
correctly scopes itself. It identifies the real, narrow fix (qualify the
destructor comment the same way callback_scope.hpp qualifies its own
guarantee) and explicitly declines to bundle in the larger fix (an actual
drain), flagging that as a separate ticket if ever wanted. That's the
"corrected framing already given" case, not a "framing needs rewriting" case.

Re-open/re-verify condition if this is ever disputed: show a code path where
CallbackToken::guard's check-then-invoke is made atomic with requestStop()
across threads (e.g. a lock shared between them) — none exists today.