core: amortise LocalBackend's pending-completion compaction (fixes #528)

[Yaraslaut]

Fixes #528 (finding F10 of the #518 sweep).

The ticket asked to be measured before it was fixed

#528's own verification status is "Inferred from reading the code; not reproduced", and its re-open condition is "N concurrent executes against a slow model, plot admission latency against N. Quadratic growth confirms it." So that came first, and the fix only after it held.

Rig. A standalone harness driving LocalBackend::execute directly against one model whose localOp parks on a gate, so every posted task — and so every CompletionState — stays live and _pending really reaches depth N. Only the execute() calls themselves are timed; nothing downstream. clang 22.1.8, -O2, against the header-only tree, on an 8-core Linux box. Every figure below is from that one rig, so the three sets are comparable with each other; they are not a claim about any other compiler or machine.

Before (master @ 26bfdb8f):

N=1000    total_admit_ms=0.593     mean_us=0.593    tail10pct_mean_us=0.815
N=2000    total_admit_ms=1.490     mean_us=0.745    tail10pct_mean_us=1.254
N=4000    total_admit_ms=5.008     mean_us=1.252    tail10pct_mean_us=2.191
N=8000    total_admit_ms=19.241    mean_us=2.405    tail10pct_mean_us=4.577
N=16000   total_admit_ms=77.778    mean_us=4.861    tail10pct_mean_us=9.566
N=32000   total_admit_ms=362.383   mean_us=11.324   tail10pct_mean_us=24.106

Total admission time per doubling of N: ×2.5, ×3.4, ×3.8, ×4.0, ×4.7 — converging on ×4, i.e. O(n²). Mean per-admission cost doubles per doubling, i.e. O(n). The ticket's premise holds, and is accepted.

Attribution. A measurement that the cost grows is not yet a measurement that this line causes it. Deleting the erase_if outright (incorrect — the list would grow unbounded — but it isolates the term):

N=1000    total_admit_ms=0.349     mean_us=0.349    tail10pct_mean_us=0.300
N=8000    total_admit_ms=1.998     mean_us=0.250    tail10pct_mean_us=0.306
N=32000   total_admit_ms=7.571     mean_us=0.237    tail10pct_mean_us=0.325

Flat in N. So all of the growth is the sweep, and none of it is the surrounding make_shared / registry lookup / strand post — which also gives a floor to aim at.

After (this branch):

N=1000    total_admit_ms=0.320     mean_us=0.320    tail10pct_mean_us=0.288
N=2000    total_admit_ms=0.464     mean_us=0.232    tail10pct_mean_us=0.303
N=4000    total_admit_ms=1.040     mean_us=0.260    tail10pct_mean_us=0.304
N=8000    total_admit_ms=2.663     mean_us=0.333    tail10pct_mean_us=0.307
N=16000   total_admit_ms=4.053     mean_us=0.253    tail10pct_mean_us=0.367
N=32000   total_admit_ms=7.632     mean_us=0.239    tail10pct_mean_us=0.305

7.63 ms against 7.57 ms with the sweep deleted entirely: the amortised compaction costs nothing measurable while still reclaiming. 47× on total admission time at N=32 000, 74× on the tail.

On the other close condition

#528 also says to close it if "in-flight depth is bounded low by construction in every intended deployment". It is not. RemoteServer::LimitPolicy::maxInFlightExecutes defaults to 0 (unbounded), and it is in any case a RemoteServer policy — it does not sit in front of LocalBackend at all. Nothing else bounds the depth; Bridge::pendingCalls() counts but does not gate. And _pending is keyed on state liveness, not on in-flight-ness: a caller that retains its Completion handles keeps their entries live long after the calls have settled, so the list can exceed the in-flight count. The condition is not met.

The fix

Sweep amortised, not per-append: trackPending sweeps only when _pending.size() reaches _compactAt, and each sweep re-arms _compactAt at twice the number of entries that survived it, floored at 32. A sweep costs O(size) and at least _compactAt / 2 appends must happen before the next one, so per-append cost is amortised O(1) at any depth.

The issue's cheap shape, not its better one. The intrusive option — a slot index on CompletionState, unlinked on settle — was weighed against docs/spec/core/completion.md and rejected, with the reasoning recorded as a design-decision row rather than left in this PR:

• It puts a back-reference to one backend's private table into a type every backend shares, which then has to be weak (the state can outlive the backend), so the O(1) unlink is not free of the lifetime problem it was meant to avoid.
• It moves a _pendingMtx acquisition onto the settle path of every completion, i.e. onto every strand thread on every result — trading a cost paid once per burst for contention paid continuously, on the exact path Give Completion<T> a value-handling contract, and stop a benchmark racing on a dead frame #579 just gave a value-handling contract.
• It puts new failure modes in front of cancelPending, which is the one thing that must not break here.

The amortised sweep buys the same O(1) admission for one size_t confined to LocalBackend.

The price, stated plainly: _pending is now bounded at twice the live count plus the floor, rather than at exactly the live count. At 10 000 live that is ~320 KB of weak_ptr instead of ~160 KB. LocalBackend::trackedPendingCount() is added so that bound is measured rather than asserted — the test quotes 112 entries where an uncompacted list would hold 3 120.

cancelPending still works

This is the constraint that mattered, and it is preserved by construction rather than by care: cancelPending never saw dead entries in the first place. Its weak.lock() has always skipped them, so a longer-lived dead entry changes nothing it observes. What changed for it is one line — it resets _compactAt to the floor, having just emptied the list.

Checked three ways:

1. By construction. The sweep's predicate is unchanged (weak.expired()); it only runs less often. A live entry was never a candidate for removal and still is not.
2. By test. The new fixture parks 48 completions across 48 rounds — spanning many sweeps — while 3 072 others settle and are dropped around them, then calls cancelPending(BackendChangedError{}) and requires all 48 to arrive. ~Bridge's blocking teardown contract in concurrency_and_lifetimes.md is untouched: the snapshot-then-deliver-outside-the-lock shape is byte-for-byte the same, and no callback runs under _pendingMtx.
3. By mutation (below).

Proving the test is not vacuous

Two mutations, each reverted after measuring:

A — never sweep (if (_pending.size() >= _compactAt) → if (false)), which is the "did the optimisation delete the feature?" case:

tests/test_backend_extra.cpp:410: FAILED:
  CHECK( tracked < 1024 )
with expansion:
  3120 (0xc30) < 1024 (0x400)
with message:
  tracked=3120 admitted=3120

B — sweep everything (predicate → always true), which is the "did the optimisation break cancellation?" case:

tests/test_backend_extra.cpp:426: FAILED:
  CHECK( cancelledCount == kRounds )
with expansion:
  0 == 48
with message:
  tracked=16 admitted=3120

Unmutated, same fixture: tracked=112 admitted=3120, 48 == 48, 51 assertions, 65 ms.

What the test does not cover, said plainly: admission latency itself — the whole point of the change — is not asserted anywhere. A wall-clock bound on a shared CI runner would be a flake rather than evidence, so that measurement lives in the benchmark above and in the spec, and the test guards only the two properties that can be checked deterministically. This is stated in the test's own comment too, so a later reader does not mistake it for a perf gate.

Review reasoning (Copilot has no seats on this org, so this is the review)

• Is _compactAt guarded? Yes — it is read and written only under _pendingMtx, in trackPending and cancelPending, the only two functions that touch _pending. No new lock, no new ordering, no new lock held across a callback.
• Can the threshold run away? No. It is re-armed from the post-sweep size, so it tracks the live count and falls as the live count falls; it cannot ratchet upward across a drain. cancelPending resets it to the floor.
• Is the amortisation argument actually amortised, or just "less often"? Amortised. The threshold is a multiple of the post-sweep size, not a constant interval: between two sweeps at least _compactAt / 2 appends occur, and the sweep costs O(_compactAt), so the per-append share is bounded by a constant independent of N. A fixed interval would have been O(interval) per append and would still degrade — that distinction is why the doubling is load-bearing and is spelled out in the code comment.
• Does the floor of 32 hide anything? It means a backend with fewer than 32 admissions ever never sweeps at all, holding at most 32 dead weak_ptrs (512 bytes). That is the intended trade and is documented on the constant.
• New public API. trackedPendingCount() is deliberately not named pendingCount: it is an upper bound on in-flight, not the in-flight count, and its doc comment and the spec row both say so and point at Bridge::pendingCalls() for the other question. It exists to make the memory price measurable; without it the bound would be an assertion rather than a measurement.
• Const-correctness. _pendingMtx became mutable for the new const accessor. That is the standard shape and does not widen anything — the mutex was already the only guard for _pending.
• Includes. <algorithm> for std::max and <cstddef> for std::size_t, both previously transitively available and now named. misc-include-cleaner is off in this repo, so this is hygiene rather than a gate.

Verification

The first push failed one leg, and the finding was in the fixture rather than the change — worth stating rather than quietly amending away. Linux / clang-asan reported a stack-use-after-scope at the parked op's gate.load(): the fixture declared gate after the ThreadPoolExecutor, so it was destroyed first, while 47 of the 48 parked tasks were still queued and ran during ~StrandExecutor's blocking teardown.

ERROR: AddressSanitizer: stack-use-after-scope
READ of size 1 at 0x7b8beeef0500 thread T1
  #2 ... CATCH2_INTERNAL_TEST_16()::$_0::operator()()
         tests/test_backend_extra.cpp:365:57
  #7 ... pendingCall(std::function<void ()>)::$_0::operator()(IModelHolder&)
  #12 ... LocalBackend::execute(...)::'lambda'()::operator()()
         include/morph/core/backend.hpp:1303:25

Reproduced locally under the clang-asan preset at the same line, so the fix was verified against a failing run rather than against CI going quiet. Fixed on both axes: every atomic a pool task touches is now declared before the pool, and the parked ops count themselves so the test drains all 48 before leaving the scope instead of trusting teardown to. The second commit carries that; the assertions are untouched and both mutation proofs above were re-run against the corrected fixture.

Check	Result
Full suite, GCC Release	1536 cases, 1535 passed, 1 failed as expected (the deliberate NoOpReplayLedger negative-conformance case); 22 629 assertions
New fixture, AddressSanitizer	passes, no reports (was the failure above)
New fixture, ThreadSanitizer	passes, no reports
New fixture, clang Debug -Weverything	passes, 51 assertions, 65 ms
[backend],[local],[bridge] under TSan	205 cases, 204 passed; the one failure is OomInjector: unusable under ASan/TSan, pre-existing and unrelated
scripts/check_spec_sync.sh	10 sub-domain(s) classified; every touched header sub-domain has a matching spec change
scripts/check_spec_citations.sh	Prose lint OK — 845 references, 76 cited sections
scripts/check_catch_test_names.sh	Catch2 test-name lint OK, 2 962 names
Doxygen WARN_AS_ERROR=FAIL_ON_WARNINGS	builds clean
clang-tidy-diff over changed lines, clang-debug db	zero findings
clang-format --dry-run -Werror	clean
Compiler launcher	LAUNCHER = ... fastcache-cc on every tree used (not ccache)

Filed separately

scripts/mutation_survivors.json's line hints are unaudited and 5 of its 7 structured entries already point at the wrong line on master — backend.hpp:749 is off by 477 lines. Noticed while checking whether my own allowlist edit had siblings; filed as #608 rather than folded in here.

Files

scripts/branch_partial_allowlist.json carries a line-hint refresh only: 1228 → 1230, because this change inserted two #include lines above the allowlisted notifyBackendChanged line. No entry added, removed or reworded; the gate resolves entries by source text and fails on a drifted hint, which is what this avoids.

docs/spec/core/backend.md is edited in three scoped places — a new "The pending list and its amortised compaction" subsection under LocalBackend, the trackedPendingCount() row in its method table (plus the cancelPending row's threshold reset), and one design-decision row recording why the intrusive alternative was declined. #585 also touches this file; the edit is kept additive and local so the conflict, if any, is mechanical.

🤖 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!