Give Completion<T> a value-handling contract, and stop a benchmark racing on a dead frame

[Yaraslaut]

Two tickets that both turn on what Completion<T> does with a value, and on
what a wait loop is allowed to leave behind.

Closes #553
Closes #565

---

#565 — the benchmark writes into a destroyed stack frame

tests/test_server_limits.cpp's throughput benchmark held std::atomic<int> done as a stack local of the BENCHMARK body and captured it by reference
into reply callbacks that run on ThreadPoolExecutor workers. The 1 s deadline
lets the wait loop exit with replies still in flight; the body returns, done
is destroyed, and Catch2 re-enters the body for the next sample, constructing a
fresh counter over the same stack slot.

Reproduced first, clang-tsan preset, clang 22.1.8,
./morph_tests "benchmark: in-process execute round-trip" "[!benchmark]":

WARNING: ThreadSanitizer: data race (pid=496638)
  Write of size 4 at 0x7ffc616f336c by main thread:
    #1 std::atomic<int>::atomic(int)
    #2 CATCH2_INTERNAL_TEST_11()::$_0::operator()(int) const
       tests/test_server_limits.cpp:192:26
  Previous atomic write of size 4 at 0x7ffc616f336c by thread T4:
    #0 std::__atomic_base<int>::fetch_add(int, std::memory_order)
    #1 ...::operator()(std::string const&) const
       tests/test_server_limits.cpp:196:63
    #6 morph::backend::RemoteServer::dispatchExecute(...)
       include/morph/core/remote.hpp:1487:17
  Location is stack of main thread.

ThreadSanitizer: reported 2 warnings

Two races, both frames in this test — exactly what the issue reported.

After, same command, same binary flags:

RemoteServer round-trip (echo, 5 bytes)   100 samples   7.0912 ms mean
All tests passed (2 assertions in 1 test case)

Zero TSan output, and the benchmark still reports a number rather than becoming
a no-op. The whole [limits] set under TSan: 970 assertions, 16 test cases, no
warnings.

Which of the two candidate fixes. The issue named both. I took the
shared_ptr one and kept the deadline, demoted to what it always was in
practice: a benchmark timeout that stops a wedged server becoming a hang. An
unconditional wait removes the lifetime question but trades a slow result for a
hang, and the lifetime question is removable on its own terms — the counter is
heap-allocated and co-owned by every callback, so a straggler writes into a live
object whether or not the deadline was right.

---

#553 — the Completion<T> value contract

Verdict on the pre-existing design doc

I read docs/superpowers/specs/2026-09-16-completion-value-contract-design.md
on branch completion-value-contract before designing anything, and adopted
it: the contract as stated, both mechanism changes (erase onOk as
std::function<void(const T&); give CompletionState<T>
enable_shared_from_this), design A over B for the reason it gives, and its
"out of scope" list including leaving IExecutor::post alone. Its call-site
analysis held up against the tree.

Three places where I did something different, all additive:

1. The doc's mechanism does not compile as written for a move-only T.
   It leaves value = std::move(val) in setValue, which goes through
   std::optional<T>::operator=(U&&) and so requires T to be move-assignable
   as well as move-constructible. Caught by the static_assert's own test:
   a Probe with operator= deleted fails to compile with
   object of type 'std::optional<Probe>' cannot be assigned because its copy assignment operator is implicitly deleted. Changed to value.emplace(std::move(val)),
   which needs only move-construction and keeps the same strong exception
   guarantee (emplace leaves the optional disengaged if construction throws).
   Without this, std::move_constructible<T> would have been a claim the
   implementation quietly did not honour.

2. The doc counts three sites that reject a move-only T. There are four
   — setValue's auto savedVal = val and its fan-out savedFns[i](savedVal),
   plus attachThen's auto savedVal = *value and its by-copy capture. Measured
   against master's header with g++ 16.2.1: four use of deleted function 'std::unique_ptr::unique_ptr(const std::unique_ptr&)' errors at
   completion.hpp:72, :89, :170, :171.

3. Dropped the tests/compile_checks/ entry the doc proposes (a negative
   compile test that a by-value handler on a move-only T is rejected). It
   would pin a property of std::function's converting constructor, not of
   morph, and the repo's compile_checks mechanism is a per-entry
   try_compile — a configure-time cost for a check whose subject is the
   standard library. The positive half is pinned instead, at runtime, in
   test_completion_value_contract.cpp. Say the word and I will add it.

I did not reproduce the doc's ns/settle benchmark table. Those numbers were
measured against standalone models rather than a patched header, and the claim
this ticket turns on is the copy count, which is measured below against the real
header. The small-T atomic-refcount regression the doc records is real and is
documented in the spec rather than re-measured.

Before / after, measured

Instrumented T counting copy and move constructions, against the real
include/morph/core/completion.hpp, g++ 16.2.1 -O2, Linux x86-64. N handlers
attached before the completion settles, M after; an executor that runs posted
closures inline, so no scheduling is in the numbers.

Copies

N before	M after	const T& before	const T& after	by-value before	by-value after
0	0	0	0	0	0
1	0	1	0	1	1
2	0	2	0	2	2
3	0	3	0	3	3
0	1	2	0	2	1
0	2	4	0	4	2
1	1	3	0	3	2
3	3	9	0	9	6

N + 2M for every handler signature, before. After: zero for a const T&
handler, exactly one for a by-value handler, independent of handler count and
of which side of the settle the handler attached.

Moves, same runs: 1–8 before (17 in the 3+3 by-value case), a constant 2
after — into setValue's by-value parameter, then into value, with the
prvalue elided into resolve's.

Move-only T

Works. Completion<std::unique_ptr<int>> instantiates, fans out to three
const T& handlers across the settle (sum 63 from a unique_ptr<int>(21)), and
also works through the CallbackScope-gated then(scope, fn) overload,
including refusing delivery after requestStop(). On master the same program
does not compile — four use of deleted function errors, quoted above.
IExecutor::post is untouched: the dispatch closure captures a shared_ptr,
not the value, so it stays copy-constructible.

Evidence that the new tests fail on the unfixed code

tests/test_completion_value_contract.cpp, with its two move-only cases removed
so the rest can be run rather than merely fail to compile, built against
origin/master's completion.hpp:

test cases:  5 |  0 passed |  5 failed
assertions: 61 | 38 passed | 23 failed

  CHECK( g_counts.copies == 2 )   with expansion: 7 == 2
  CHECK( byValue.copies == handlers )  with expansion: 9 == 6   (before=3 after=3)

The two move-only cases do not compile against that header at all.

Counts are asserted exactly, not as an upper bound — an upper bound absorbs
the regression the file exists to catch.

One existing test replaced, deliberately

test_completion_multi_handler.cpp's "a throwing T copy leaves the state
unsettled with every handler intact" pinned the old shape: it required
setValue(ThrowOnCopy{7, true}) to throw, because setValue copied. There is
no copy on the value path any more, so it failed — correctly. It is replaced by
two tests rather than deleted:

• "settling never copies T — an armed throwing copy constructor never runs":
  the same ThrowOnCopy, armed, settled through const T& handlers, required
  not to throw. That turns "zero copies" from a comment into a trap, and it
  fails on master.
• "a throwing T move leaves the state unsettled with every handler intact":
  a new ThrowOnMove fixture pinning the strong exception guarantee setValue
  actually still has — emplace before draining onOk, so an escape leaves the
  state unready with every handler attached, and a later successful settle still
  fires them.

What I checked, inline (no /code-review, no /simplify)

• Is the unlocked read of value in the dispatch closures a race? No.
  value is write-once: setValue and setException both return early when
  ready, and grep confirms nothing else assigns it. The store happens under
  mtx before the closure reaches the executor, and the executor's queue
  supplies the happens-before edge. Argued in the header and the spec, and the
  full suite is green under TSan.
• Is shared_from_this() always legal here? Every CompletionState<T> in
  the tree is created by std::make_shared — grepped for CompletionState<
  across include/ src/ examples/ tests/; the only non-shared_ptr hits are
  comments. A stack-constructed one would throw std::bad_weak_ptr.
• Does the closure holding the state create a cycle or change orphan
  logging? No cycle: the closure is destroyed when the executor runs or drops
  it. Orphan logging is on the error path only, which is untouched
  (exception_ptr is already a refcounted handle — left alone, per the design
  doc's out-of-scope list).
• Call-site audit. All 312 .then(/.thenDetached( sites, with their
  handler parameter spellings extracted mechanically. The shapes present are
  T by value, const T&, and one auto by value
  (bridge.hpp:2523) — all three convert to std::function<void(const T&)>.
  A handler taking T& or T&& would not convert; none exists. The two
  completion-chaining sites (bridge.hpp:2207, :2241) take R value by
  value and still cost one copy — a floor, since setValue(T) must own a
  value, and the spec now says so.
• Spec. docs/spec/core/completion.md gains a "Value-handling contract"
  section; the summary-table row claiming setValue "moves it only into the
  last handler's invocation" — now false — is replaced, as are the onOk
  member type, the four then signatures, attachThen's signature, the "Copy
  vs. move of the value on dispatch" bullets and the fan-out paragraph under
  Failure modes. scripts/check_spec_citations.sh passes.

Verification runs

Build	Result
clang-debug (tests + examples)	1528/1528 ctest pass
clang-tsan, full ctest	1498/1498 pass, zero TSan reports
clang-asan (ASan + UBSan), full ctest	1498/1498 pass
gcc-debug	1502 cases, 22362 assertions, only the intentional "failed as expected"
ladder, MORPH_BUILD_LADDER=ON MORPH_LADDER_RUNGS=all MORPH_BUILD_QT=ON	builds clean; 2636/2636 ctest pass
clang-tidy-diff over origin/master...HEAD	clean; the NOLINTs verified by deleting one and watching the finding come back
Doxygen -DMORPH_BUILD_DOCUMENTATION=ON	builds clean
scripts/check_spec_citations.sh	pass

---

Rebase, and re-verification after it

Rebased onto current master (70792bee). origin/master is an ancestor of the
branch head, the tree has zero conflict markers, and the CHANGELOG.md overlap
with #581 is resolved. Files touched, unchanged from the list above:

CHANGELOG.md
docs/spec/core/completion.md
include/morph/core/completion.hpp
tests/CMakeLists.txt
tests/test_completion_multi_handler.cpp
tests/test_completion_value_contract.cpp
tests/test_server_limits.cpp

No overlap with #585's files. bridge.hpp, executor.hpp, the Qt websocket
backend and its tests are untouched here.

The one thing the rebase actually changed

The call-site audit above was taken before #586 (70792bee) landed
include/morph/net/socket_backend.hpp, which creates CompletionState<T>
directly and calls setValue/setException on it. That is new code the audit
never saw, and it is exactly the shape this PR changes.

Checked, not assumed. MORPH_BUILD_NET is OFF in the clang-debug preset, so
the ordinary local build does not compile that header; it was syntax-checked
explicitly against the new completion.hpp, with the real compile flags:

$ clang++ -fsyntax-only tu_net.cpp <flags from compile_commands.json>   # includes morph/net/socket_backend.hpp
$                                                                       # no output — clean

Its eleven CompletionState<T> sites are all std::make_shared, which is the
precondition the new shared_from_this() needs, and none of them is a .then(
call site, so the erased-handler change cannot reach them.

Independent re-check of the two safety arguments

• value is write-once, so the unlocked read in the dispatch closures is
  sound. Confirmed mechanically rather than by reading prose — across the whole
  header there is exactly one write:

  $ grep -n 'value\s*=\|value\.emplace\|value\.reset' include/morph/core/completion.hpp | grep -v '^\s*[0-9]*:\s*//'
  104:            value.emplace(std::move(val));
  

• shared_from_this() is always legal. Every CompletionState<T> in
  include/ src/ examples/ tests/ is created by std::make_shared — including
  the three new sites in socket_backend.hpp. The remaining grep hits are
  shared_ptr members and parameters, not constructions.

Mutation testing — are these tests non-vacuous?

Every claim below was produced by building this branch's tests against
origin/master's completion.hpp and running them.

The move-only cases are compile-level assertions and do not compile on
master — which is the point, but it also means they cannot be "run and fail",
so they are reported separately:

FAILED: tests/CMakeFiles/morph_tests.dir/test_completion_value_contract.cpp.o
completion.hpp:170:22: error: call to deleted constructor of 'std::unique_ptr<int>'
completion.hpp:171:58: error: call to deleted constructor of 'std::unique_ptr<int>'
completion.hpp:72:22:  error: call to deleted constructor of 'std::unique_ptr<int>'
std_function.h:429:18: error: static assertion failed ...: std::function target must be copy-constructible

A fourth error on master is worth naming on its own, because it confirms the
emplace change was load-bearing and not a tidy-up. Probe deliberately
declares no assignment operator, and master's setValue needs one:

completion.hpp:73:23: error: object of type 'std::optional<(anonymous namespace)::Probe>'
                      cannot be assigned because its copy assignment operator is implicitly deleted

The counting cases fail on master. With the two move-only cases excised and
Probe given a move-assignment operator purely so master's header compiles at
all, the rest run:

test_completion_value_contract.cpp:141: failed: counts.copies == 0 for: 1 == 0  'before=1 after=0'
test_completion_value_contract.cpp:141: failed: counts.copies == 0 for: 9 == 0  'before=3 after=3'
test_completion_value_contract.cpp:154: failed: byValue.copies == handlers for: 9 == 6  'before=3 after=3'
test_completion_value_contract.cpp:161: failed: erased.copies == handlers for: 9 == 6  'before=3 after=3'
test_completion_value_contract.cpp:204: failed: gCounts.copies == 2 for: 7 == 2
test_completion_value_contract.cpp:230: failed: gCounts.copies == 2 for: 3 == 2
test_completion_multi_handler.cpp:298:  failed: unexpected exception with message: 'copy ctor blew up';
                                        expression was: state->setValue(ThrowOnCopy{7, true})

test cases:  7 |  1 passed | 6 failed
assertions: 69 | 45 passed | 24 failed

One case passes on both sides, and it is reported rather than claimed.
"a throwing T move leaves the state unsettled with every handler intact" does
not discriminate master from this branch — both orderings keep the strong
exception guarantee, so as evidence for this change it is worth nothing, and
saying otherwise would be the kind of control this repository keeps getting
burned by.

It is not vacuous, though, and that was checked rather than asserted. It pins
the store-before-drain ordering, so it was mutated directly: draining onOk
before value.emplace in the fixed header makes it fail, and nothing else in
the tag does.

test_completion_multi_handler.cpp:329: failed: state->onOk.size() == 2U for: 0 == 2
test_completion_multi_handler.cpp:338: failed: fired == 2 for: 0 == 2
test cases:  9 |  8 passed | 1 failed

Header restored, tree clean, and the tag back to All tests passed (77 assertions in 9 test cases).

Compile time

completion.hpp is widely included and #573 records that core's compile budget
is already strained, so this was measured rather than waved past. An isolated TU
including only morph/core/completion.hpp, clang 22.1.8 -std=c++23, three
runs each:

header	runs (ms)	median
origin/master	2358, 2427, 2517	2427
this branch	2623, 2368, 2553	2553

The ranges overlap completely (branch minimum 2368 is below master's maximum
2517). No compile-time cliff, and nothing here worth filing against #573.

Local runs

Check	Result
clang-debug full build	clean, 390 s wall (133 targets, cold cache)
ctest full suite	1542/1542 passed, 7.7 s
[issue-553] tag	9 cases, 77 assertions, all passed
scripts/check_spec_citations.sh	pass (837 references, 73 sections)
socket_backend.hpp syntax check	clean against the new header

Compiler cache: this tree was pinned to ccache by an earlier configure and has
been moved back onto fastcache-cc against the daemon on 127.0.0.1:6674 — all
172 LAUNCHER lines in build.ninja, zero ccache.

Filed in passing

#592 — cmake/CompileCache.cmake tells you to reconfigure with --fresh to
change a stuck launcher, which discards the whole build tree, when
-UCMAKE_C_COMPILER_LAUNCHER -UCMAKE_CXX_COMPILER_LAUNCHER does the same thing
and keeps every object. Reproduced both paths. Not folded into this PR.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!