Found while reviewing #566. Pre-existing; the scope note on #566 explicitly asked that a broad sweep be a separate decision, so this is that decision.
The pattern
tokio::time::timeout(D, async {
while flag.load(Ordering::SeqCst) == 0 {
tokio::task::yield_now().await;
}
}).await.expect("...");yield_now() never sleeps — it reschedules immediately — so the waiter stays runnable for the whole wait and contends with the rest of the test binary. 73 live instances across the workspace.
Most are merely wasteful. Two are not.
The dangerous cases: a spin under a paused clock is a hang, not a flake
crates/opc-session-net/src/consensus.rs:3854-3860, inside #[tokio::test(start_paused = true)] (attribute at :3810)crates/opc-session-net/src/consensus.rs:5155, same shape
Under start_paused, tokio auto-advances the virtual clock only when the runtime goes idle. A yield_now spin keeps the run queue permanently non-empty, so auto-advance never fires and the enclosing timeout(1s) — which is measured on that same virtual clock — never expires.
The failure mode is therefore not "test flakes at 1.4%". It is "test hangs until the CI job's wall-clock kill". A timeout that cannot fire is not a timeout.
Also worth folding in
crates/opc-session-net/tests/three_node_quorum.rs:4330-4339 — historical_cas_is_rejected_after_server_restart_without_redispatch, timeout(2s, loop { .. yield_now().await }), same file and same current_thread shape as the wait converted in #566.
Suggested direction
Replace the spin with real signalling — tokio::sync::watch is the right default, because Receiver::wait_for is level-triggered (it evaluates the predicate against the current value before awaiting), so a state already reached still resolves immediately and there is no lost-wakeup hazard. Notify requires its permit armed before the trigger and is the wrong tool here.
Priority order:
- The two
start_paused sites — these can hang.
three_node_quorum.rs:4330.- The remaining instances, as cleanup, only where a test actually contends.
Do not address these by raising deadlines. On #566 that approach silently destroyed the assertion it claimed to protect: raising a wait past the server's own with_backend_operation_timeout let the test pass with the production cancellation removed entirely.
Evidence tier
Verified by reading for the start_paused interaction and the instance count. The #566 sibling case was measured.
Found while reviewing #566. Pre-existing; the scope note on #566 explicitly asked that a broad sweep be a separate decision, so this is that decision.
The pattern
yield_now()never sleeps — it reschedules immediately — so the waiter stays runnable for the whole wait and contends with the rest of the test binary. 73 live instances across the workspace.Most are merely wasteful. Two are not.
The dangerous cases: a spin under a paused clock is a hang, not a flake
crates/opc-session-net/src/consensus.rs:3854-3860, inside#[tokio::test(start_paused = true)](attribute at:3810)crates/opc-session-net/src/consensus.rs:5155, same shapeUnder
start_paused, tokio auto-advances the virtual clock only when the runtime goes idle. Ayield_nowspin keeps the run queue permanently non-empty, so auto-advance never fires and the enclosingtimeout(1s)— which is measured on that same virtual clock — never expires.The failure mode is therefore not "test flakes at 1.4%". It is "test hangs until the CI job's wall-clock kill". A timeout that cannot fire is not a timeout.
Also worth folding in
crates/opc-session-net/tests/three_node_quorum.rs:4330-4339—historical_cas_is_rejected_after_server_restart_without_redispatch,timeout(2s, loop { .. yield_now().await }), same file and samecurrent_threadshape as the wait converted in #566.Suggested direction
Replace the spin with real signalling —
tokio::sync::watchis the right default, becauseReceiver::wait_foris level-triggered (it evaluates the predicate against the current value before awaiting), so a state already reached still resolves immediately and there is no lost-wakeup hazard.Notifyrequires its permit armed before the trigger and is the wrong tool here.Priority order:
start_pausedsites — these can hang.three_node_quorum.rs:4330.Do not address these by raising deadlines. On #566 that approach silently destroyed the assertion it claimed to protect: raising a wait past the server's own
with_backend_operation_timeoutlet the test pass with the production cancellation removed entirely.Evidence tier
Verified by reading for the
start_pausedinteraction and the instance count. The #566 sibling case was measured.