Convert ActorCollection to standard coroutines

[tclinkenbeard-oai]

Summary

• Convert ActorCollection.actor.cpp into a standard C++ coroutine implementation in ActorCollection.cpp.
• Preserve the existing public API, source-ordered event handling, deterministic cancellation, error propagation, and callback lineage.
• Use persistent callbacks and separate cancellation/completion paths to avoid per-event allocations.
• Add regression coverage for cancellation order, reentrant collection cancellation, sampling lineage, queued-ready actors, and add-stream errors.

Benchmark results

flow_bench; Clang 19.1.5; Release build; sanitizers disabled; pinned to one CPU; seven repetitions; 0.15-second minimum benchmark time.

Benchmark	Before	After	Change
Ready actor	169 ns	154 ns	−9.0%
Add 1 pending	2.91 µs	2.90 µs	−0.5%
Add 16 pending	4.04 µs	3.97 µs	−1.8%
Add 256 pending	20.28 µs	18.65 µs	−8.0%
Complete 1 pending	2.90 µs	2.89 µs	−0.3%
Complete 16 pending	4.08 µs	3.79 µs	−7.1%
Complete 256 pending	21.23 µs	15.91 µs	−25.0%

Testing

• flow_test -f /flow/actorCollection/ --seed 12345: 12 passed.
• flow_test -f /flow/coro/noThrowOnCancel/ --seed 12345: 7 passed.
• flow_test --ignore /flow/coro/FlowMutex --seed 12345: 189 passed; excludes an unrelated long-running mutex stress test.
• flow_test -f /flow/actorCollection/testSamplingLineage --seed 12345 with ENABLE_SAMPLING: 1 passed.
• Built flow_bench and flow_sampling, including the sampling-enabled implementation.
• clang-format 19.1.5 check passed.

[tclinkenbeard-oai]

Generated by Codex.

What is it trying to do?

Convert ActorCollection from an actorcompiler-generated actor to standard C++ coroutines while preserving its public API, event ordering, deterministic cancellation, error propagation, accounting, and sampling lineage.

Is it correct?

Yes. I compared the replacement runtime against the previous implementation and inspected child ownership, synchronous callbacks, completion ordering, returnWhenEmptied, cancellation and error propagation, nested clear(), counter and timing updates, sampling lineage, and the shared NoThrowOnCancel change.

The cancellation state is latched before waiter or frame teardown, runtime destruction prevents reentrant callbacks from accessing invalid state, nested clear() cannot create duplicate stream consumers, and callback lineage explicitly restores the actorCollection name. Added regressions cover cancellation order, reentrant cancellation, sibling completion and errors during teardown, queued-ready actors, stream errors, and sampling.

The sole failing check, FoundationDB CI - PR Builder, compiled successfully and passed all 62 CTests, but its Joshua campaign passed 9,998/10,000 simulations. Both failures were tests/fast/DDPipelineSaturation.toml, with seeds 925654854 and 1246470200. The same PR head passed 10,000/10,000 simulations under Clang, and the immediately preceding commit passed 10,000/10,000 under GCC; the current commit adds only five comments. The workload and relevant data-distribution/recovery sources are identical to the PR base, and separate PR #13778 addresses this failure class. This strongly suggests an existing data-distribution issue rather than an ActorCollection regression, although exact-seed causality has not been proven.

No builds, tests, or simulations were run for this review.

Are there bugs?

I did not find any correctness bugs in the current PR.

Are there omissions?

No blocking omissions. Explicit regression coverage for idleTime/allTime accounting and reentrant cancellation during waiter detachment would strengthen coverage, but the existing implementation preserves those accounting transitions and latches cancellation before waiter teardown.

Are there better ways of doing things?

No substantial alternative is necessary. Persistent callbacks and the intrusive completion queue preserve constant-time common-path processing, while the ready-actor fast path avoids allocating an unnecessary Runner.

Should this CL be LGTMd?

Yes, LGTM for the reviewed code. The failing GCC builder remains a merge gate and should be rerun or otherwise resolved before merging.

[saintstack]

Both failures were tests/fast/DDPipelineSaturation.toml, with seeds 925654854 and 1246470200.

Hopefully fixed by 'Bound the post-move destination metrics refresh in DD (#13863)'

For 'RandomSeed="2364947726" SourceVersion="dbd9b0fbdc0f7053a1dfccabd97c3d10478966f2" Time="1786727684" BuggifyEnabled="1" DeterminismCheck="0" FaultInjectionEnabled="1" TestFile="tests/fast/ShardEncodeRollback.toml"', let me take a look at this one..... It seems unrelated.

[saintstack]

Local LLM came back with following... I'd be interested in your comments on 2 and 3 in the below.

Code review: Convert ActorCollection to standard coroutines

Verdict: needs work before merge. No provable correctness bug found, but the PR bundles four independent changes into one, replaces a 275-line choose loop with a 735-line hand-rolled reentrancy-sensitive scheduler in one of FDB's most load-bearing primitives (151 references across 85 files), and justifies that complexity with a benchmark comparison against the wrong baseline.

I verified the mechanics against flow.h (NotifiedQueue::send/sendError, SingleCallback::remove, Callback::remove, LineageReference), CoroutinesImpl.h/CoroUtils.h/Coroutines.h at this PR's head, and flow/CMakeLists.txt.

Things that check out

• Build wiring: flow/CMakeLists.txt uses fdb_find_sources(FLOW_SRCS), so the .actor.cpp -> .cpp rename needs no CMake change. Confirmed.
• Event ordering matches the old choose positional priority exactly: pendingAdds -> addActor -> completed -> failure mirrors when(addActor) / when(complete) / when(errors).
• Persistent AddActorCallback is sound. NotifiedQueue::send fires SingleCallback::next without unregistering it, so never resetting addCallbackRegistered in onAdded is correct, and armAddCallback's copy-then-addCallbackAndClear correctly transfers exactly one future ref that remove() -> unwait() -> delFutureRef() gives back.
• No UAF in the delicate spots, as far as I can tell. NotifiedQueue::send/sendError both have the ->fire()/->error() call as their last statement, so deliverTerminal() removing the callback and freeing the runtime from inside AddActorCallback::fire is safe. deliverTerminal correctly copies done and terminalError to locals before the send, and drain() touches no members after deliverTerminal() returns.
• Cancellation ordering is preserved (dtor clear_and_dispose in insertion order), and Runner::detach()-before-act preserves the old contract that a callback may delete itself during fire.
• Sampling lineage is a genuine fix, not just a port: LineageReference's copy ctor resets actorName_ to "", so the old LineageReference callbackLineage = lineage; silently lost the actor name in every Runner callback. Re-applying actorCollectionLineage() on each copy is required. This is a user-visible profiling behavior change and isn't mentioned in the PR description — please add it.
• The CoroutinesImpl.h cancel guard is correct and necessary. Without it, a re-entrant cancel() during handler->cancelWait()/destroyFrame() reaches destroyFrame() with handle == {} and calls h.destroy() on a null handle. Using actorWaitStateIsCancelled as the guard is consistent with CoroActor::cancel and ChooseImplActor::cancel, which are already re-entrancy-safe by the same mechanism.
• ActorCollection::clear() fixes a real pre-existing bug: previously a re-entrant clear() from a child's cancellation cleanup would have its new collection immediately overwritten (and cancelled) by the outer call.

Blocking

1. Split this into at least three PRs. As written, a bisect that lands on this commit can't distinguish an ActorCollection scheduler bug from a core coroutine-cancellation bug. Suggested split:

• The CoroutinesImpl.h re-entrant-cancel guard + the noThrowOnCancel/reentrantCancelDuringCleanup test. This is an independent bug fix in core machinery affecting every NoThrowOnCancel coroutine and should land on its own.
• The ActorCollection::clear() re-entrancy fix + testCancelReentrantCollection. This is a behavior change to the existing ACTOR implementation and is independently testable/backportable.
• The coroutine port itself.

2. The benchmark baseline is wrong for the claim being made. "Before" is the ACTOR version, but the change on trial is hand-rolled scheduler vs. coroutines. Please add a third column for a straightforward Choose-based port:

loop { co_await Choose().When(addActor, ...).When(complete, ...).When(errors, ...).run(); }

which is a near-1:1 translation of the deleted code and enormously easier to audit. If that port already captures most of the win, the 460 extra lines of drain()/pendingAdds/terminalRequested/draining/finished state aren't worth the risk. 169ns -> 154ns is a small absolute win for this much machinery.

3. Test evidence is insufficient for this file. flow_test unit tests don't exercise actorCollection under the conditions that matter (every FDB role's actor teardown, error cascades, SignalableActorCollection reset paths). Please report a real simulation/correctness run (ctest / fdbserver -r simulation) with a decent seed count before merge.

Correctness / robustness concerns

4. addRunner ordering is load-bearing and undocumented.

auto runner = runners.insert(runners.end(), *new Runner(this));
runner->start(std::move(actor));   // ready actor -> fire() -> onCompleted() -> drain()
incrementCount();

This is only safe because every caller of handleAdded has already set draining = true, so the re-entrant drain() is a no-op. If that invariant is ever broken, handleCompleted runs decrementCount() before incrementCount() and *pCount goes to -1. Just increment first (behaviorally identical), and add ASSERT(draining) to handleAdded.

5. onAdded's if (finished) return; silently drops an added actor. I believe it's unreachable (both deliverTerminal and ~ActorCollectionRuntime set finished only after/with addActorCallback.remove()), but silently discarding an actor someone added is a nasty failure mode. Make it ASSERT(false) or an explicit documented drop.

6. Leftover pendingAdds at destruction are silently cancelled — this contradicts the documented testCancelPromiseStream contract. In the old implementation, actors added after the collection stopped stayed queued in the PromiseStream and were picked up by the next actorCollection instance. Anything buffered in pendingAdds cannot be pushed back into a FutureStream, so it dies with the runtime. If you believe the window is unreachable, add ASSERT(pendingAdds.empty()) to the dtor; if not, document the semantic change.

7. handleAdded's error_code_actor_cancelled special case needs a comment. Routing a ready-cancelled future through addRunner deliberately preserves the old quirk where the count is incremented and never decremented, so a returnWhenEmptied collection never returns. That is entirely non-obvious to the next reader.

8. AddActorCallback::fire and Runner::fire can have their this freed by the call they make. Both are safe today (nothing touches this or owner afterward), but this is exactly the kind of invariant that a later innocuous edit breaks. One comment each.

Design / cleanliness

9. actorCollectionImpl vs actorCollectionUntilEmpty — why? The bodies are identical except for NoThrowOnCancel, and actorCollectionImpl retains a returnWhenEmptied parameter that is dead (the dispatcher only reaches it with false). I can't find a correctness reason for the asymmetry: NoThrowOnCancel only changes cancellation behavior, neither body has a catch block, and both deliver actor_cancelled on m_out identically. If the rationale is "returnWhenEmptied collections normally return rather than being cancelled, so the no-throw fast path doesn't pay," say so in a comment and collapse to one function templated on a bool. Please also drop the dead parameter.

10. RunnerListDestroyer is now redundant. ~ActorCollectionRuntime already does runners.clear_and_dispose(...); runnersDestroyer then runs on an empty list. The class's original reason for existing (the actor-state/return-path split) no longer applies. Keep one mechanism — the explicit dtor call, since it must happen after finished = true. Having both invites someone to "fix" the ordering and break the finished guard.

11. Null completedHead/completedTail in the dtor. After clear_and_dispose, both dangle. Harmless today; free hygiene and better ASAN behavior.

12. Document the new clear() contract in the header. The if (m_out != previous) return; early-out means a re-entrant clear(true) inside an outer clear(false) wins, and the outer's returnWhenEmptied is silently discarded. That belongs in a comment on clear().

13. ActorCollectionNoErrors::clear() has the same latent hazard (m_ac = Future<Void>() -> delFutureRef -> cancel -> re-entrant clear() mutating m_ac mid-operator=). Not introduced here, but since this PR is explicitly hardening the sibling class, it's worth either fixing or filing.

Minor

• Test style is inconsistent: some new TEST_CASEs co_await and fall off the end, others are plain functions ending in return Void();. Pick one per test intent and be consistent.
• #include "flow/IndexedSet.h" appears unnecessary in the new file.
• The Traceable<std::pair<T, U>> specialization is unused dead code carried over from the old file, and defining a template specialization in a .cpp is an ODR trap. Good opportunity to delete or relocate it.
• testSamplingLineage reaches into the private ActorCollectionRuntime rather than going through actorCollection(). It happens to cover both the AddActorCallback::fire and Runner::fire lineage scopes, so coverage is fine — but note it doesn't validate the lineage of the coroutine wrapper itself.

[foundationdb-ci]

Result of foundationdb-pr-clang-ide on Linux RHEL 9

• Commit ID: 6ca3974
• Duration 0:41:58
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-arm on Linux RHEL 9

• Commit ID: 6ca3974
• Duration 0:48:49
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: 6ca3974
• Duration 1:13:23
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos-m1 on macOS 14.x

• Commit ID: 6ca3974
• Duration 1:19:10
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: 6ca3974
• Duration 1:19:20
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

• Commit ID: 6ca3974
• Duration 1:38:35
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos on macOS 14.x

• Commit ID: 6ca3974
• Duration 4:12:50
• Result: ❌ FAILED
• Error: `Error while executing command: # compatible with bash and zsh
  ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key
  -o ServerAliveInterval=9 ec2-user@${MAC_EC2_HOST} "
  set -e -o noclobber
  BUILDNUM=${CODEBUILD_BUILD_NUMBER}
  WAIT_TIME=14400 # 4 hours
  LOCK_STALE=4500 # 75 minutes
  LOCK=/tmp/ci.lock
  "'

macOS stat flags, not linux compatible

lock_mtime() { stat -f %m $LOCK || echo 0 }
I=0
while ! (echo $BUILDNUM >$LOCK) 2>/dev/null ; do
if (( ++I >= WAIT_TIME )); then
echo "timeout waiting for ci.lock after $I seconds"
exit 1
fi
if (( $(date +%s) - $(lock_mtime) >= LOCK_STALE )); then
echo "lock expired after $LOCK_STALE seconds ..."
# racy but sleep so chances are low
sleep $(( 3 + RANDOM % 8 ))
if (( $(date +%s) - $(lock_mtime) >= LOCK_STALE )); then
echo "lock holder is gone"
rm -f $LOCK
else
echo "new lock holder"
fi
fi
sleep 1
done
echo "acquired ci.lock"
'
. Reason: exit status 1`

• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)