SIGN IN SIGN UP

fix(test): measure publish cost, not sleep overshoot, in the SSE test (#338)

## Summary

`fifty_concurrent_clients_do_not_stall_the_publisher` claimed to prove that `FrameBus::publish` is non-blocking while measuring something else entirely: `last` was reset after `publish` returned and the delta was taken before the next call, so the measured interval spanned only the `sleep` plus scheduler overhead. The cost of `bus.publish()` never entered any measurement, yet the failure message blamed publish for blocking. The test now measures the publish call itself and asserts on that, with the loop-period check kept as a separate, separately-budgeted cadence assertion.

## The old assertion was vacuous, demonstrated

With a 5 ms stall injected inside `FrameBus::publish` (roughly 5000x its real cost of 25-35 µs), the old assertion **still passed**. The same stall makes the new assertion fail with:

```
publish cost: FrameBus::publish averaged 6.809984ms per call over 200 calls with 50 subscribers
attached and reading nothing (1.361996845s spent inside publish in total), over the 1ms per-call
budget. [...] Slowest single call was 7.249042ms, reported for diagnosis only [...]
```

The injection was removed before committing; `src/` is untouched by this PR.

## What changed

`tests/sse_events_test.rs`, one test:

- **Publish cost (new, property B).** 200 calls, each timed from just before `publish` to just after it returns, with all 50 subscribers attached and reading nothing so the 16-slot ring is overwritten many times over. That is precisely the state in which a backpressuring implementation would have to wait. Asserted on the mean at 1 ms per call.
- **Cadence (was the only assertion, now property A).** The 20-tick loop is timed as a whole against twice its nominal 1 s, and its message says it measures sleep overshoot and scheduler slop rather than publish cost. Budgeting the loop instead of each tick means one descheduled tick, the 108.5 ms observation in run 30994446017 that prompted this issue, cannot fail the run, while a genuine cadence collapse still does.
- **Hang guard.** The burst is wrapped in `timeout`, so an implementation that awaited a receiver fails with a message instead of hanging the CI job until the runner kills it.
- **Comment block** rewritten to describe the assertions actually present, including why the issue #193 "±20 ms tick jitter" figure is not directly assertable on a shared runner.

## Deriving the budget

`FrameBus::publish` bumps an `AtomicU64`, wraps the snapshot in one `Arc`, takes the `latest` write lock (uncontended in this test, no `/snapshot` request runs alongside), and calls `broadcast::Sender::send`. Broadcast applies no backpressure: once the ring is full the send overwrites the oldest slot and the lagging receiver learns about the gap through `RecvError::Lagged`. So publish is O(1) work plus waking whichever receivers are parked, and it never waits on a client.

Measured at **25-35 µs per call** with 50 subscribers. A publish that waited on even one subscriber would pay a scheduler round trip per subscriber per call, milliseconds at best, and would never return for clients that read nothing. The 1 ms budget sits ~35x above the measurement and at least an order of magnitude below the failure mode, and is 2% of the collection interval.

Two details are load-bearing and are documented in the test:

- **The 1 ms spacing between burst calls** sits outside the timed window and exists so the SSE tasks get back to `recv()` and register a waker between publishes. Back-to-back publishing measures a ~1 µs path with nothing to wake, which would make subscriber count, the whole point of this test, nearly invisible to the measurement.
- **The budget is on the mean, not the max.** A wall-clock max cannot distinguish a blocked publish from a measuring thread the OS descheduled mid-call; those samples come back quantized to whole scheduler quanta. That is the same noise that produced the original flake. The max is still reported on failure as a diagnostic.

## Stability

Eleven local runs, all passing: eight instrumented single-test runs (means 24.6, 25.4, 27.1, 31.0, 31.8, 32.7, 35.0 and 218.1 µs) plus three full-binary runs at 11/11. The 218 µs run absorbed a single 14 ms deschedule inside a timed window and stayed 4.6x inside budget; any max-based budget below 14 ms would have failed it. Cadence measured 1.035-1.041 s against the 2 s budget across all runs.

## Noted, not changed

`snapshot_error_response_carries_no_cache_headers` is named for the error path but exercises only the success path. Its own comment is candid about this, so it is not misleading in the way #327 was, but the name still describes something the body does not measure. Left alone to keep this diff narrow.

## Test plan

- [x] `cargo test --test sse_events_test` (11/11, run three times)
- [x] `cargo test --test sse_events_test fifty_concurrent_clients_do_not_stall_the_publisher` (8 consecutive instrumented runs)
- [x] Mutation check: 5 ms stall injected in `FrameBus::publish` makes the new assertion fail and left the old one passing
- [x] `cargo fmt --check`
- [x] `cargo clippy --lib --tests -- -D warnings`
- [x] `cargo clippy --bin all-smi -- -D warnings`

Closes #327
J
Jeongkyu Shin committed
a56707a61ed8d1c825705dc1d4d451760cb1e13a
Parent: 11ccefa
Committed by GitHub <noreply@github.com> on 8/6/2026, 1:36:44 PM