fix(net): keep spliced lag honest when a route is replaced

[kixelated]

Summary

A spliced subscription's lag histogram stays honest across a route switch.

• A switch unwatches the replaced copy, as before, and records the bytes it produced since the last sample at the edge it had then. That holds when the replacement has no frame yet and in the sample taken on drop. Those bytes never take the replacement's lag (previously a sample with no live frame dropped them).
• A copy replaced before its first frame is never left watching: a ready copy is unwatched at the switch, and a pending one is not watched once it answers.
• A route that comes back (A, then B, then A) is weighed again, and dropping its old copy does not silence it.
• Known approximation: frames still written into a group the reader holds from a replaced copy, after the switch, go unweighed. This is documented at the switch in resume.rs.
• Deletes quest/m1/qos/lag-splice.md and the references to it on this line.

Decisions

• How should a replaced copy be weighed? (OpenAI P2 on 3f2da83a: keeping it watched counted groups past its cap)
  • Keep it watched until dropped (3f2da83a): counts groups the reader never receives.
  • Cap-aware per-cap shares (9b7845ad): exact for held groups, but adds a mutex to the frame path while a cap exists, and broke a route flap (OpenAI P2 on 9b7845ad).
  • ✅ Unwatch at the switch and record its pending bytes at its edge (maintainer's choice): simple, no per-frame cost, at the price of the approximation above.
• Keep the fix for the lost-weight case (pending bytes recorded at the retired edge, including on drop). ✅

Public API and wire

None. Only the histogram's values change.

Stacks on #4133.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Automated review by review (OpenAI)

Reviewed commit 3f2da83a6c191dfb60132973246057ebd731d117.

P2 — Keep capped copies from weighing unreachable future groups: resume.rs:485–492. A route switch caps A at its current group, but A remains watched until its last held group is released. Frontier watches the whole track's cumulative Production, so any later group A produces during that interval contributes bytes and moves newest even though this reader cannot receive it. The new lag_weighs_a_replaced_copy_until_it_is_dropped test demonstrates the mismatch: it holds A/0, switches routes, writes A/1 beyond the cap, and expects those four unreachable bytes to count. Keep accounting for additional frames in eligible held groups, but exclude post-cap groups (e.g. cap-aware/group-scoped production tracking); add an assertion that A/1 does not change this viewer's histogram.

Direction: retaining the retired edge alongside pending weight fixes the lost-weight problem, but delaying whole-track unwatch introduces incorrect weight during overlap.

Verification: static diff and sampler/splice-context review; no runtime tests were run.

[kixelated]

Automated review of head 9b7845ad: a full review, since there's no earlier Grok review. It covers both commits, so the OpenAI P2 fix on 3f2da83a is included.

The cap-aware share fixes the P2. lag_weighs_a_replaced_copy_up_to_its_cap now shows A/1 past the cap isn't weighed, while frames still written into A/0 are. Folding retired bytes at their own edge also fixes the lost-weight case when B has no frame yet. The race comments in Production::record and Frontier::cap hold up: a frame can go uncounted but can't be counted twice. I found no lock-order problem (frontier state, then the production shares, and record never takes the frontier lock). Review was static only; I ran no tests.

Should fix

1. A route that comes back (A → B → A) loses its lag weight, and dropping the old A copy silences it. Sources in Frontier are keyed by Production, which means per cache track, not per copy. Frontier::watch (stats.rs:1957) returns early if that production already has a source. Frontier::unwatch (stats.rs:2001) removes every source for it. Before this PR, a capped copy was unwatched at the switch, so this didn't matter. Now a capped copy stays watched until let_go drops it, and that causes the bug.

Here's the failure. The front splices A, then B, then A again with the same source's copy (TrackIo::splice in origin.rs:2277 serves asked.copy, which is the same cache track while that source still has it). The reader still holds an A/gen1 group, so A/gen1 is still in copies and capped at cap1.

• When A/gen3 answers, Copy::watch (resume.rs:305) calls Frontier::watch, which does nothing because of the dedup. The serving copy is then weighed only through A/gen1's cap1 share, so every new group on the live route is ignored.
• Once the held group is released, let_go (resume.rs:554) calls copy.unwatch() on A/gen1. That removes the only source for A's production. A/gen3 is never re-watched (Copy::watch only runs on answer, on switch, and from Subscriber::watch), so the serving route stops adding to lag for the rest of the subscription.

To fix it, count watchers per production: when an uncapped copy watches a capped source, fold and drop the share and resume whole-track counting, and have unwatch only remove the source when the last watcher leaves. A cheaper option is for let_go to skip unwatch when another remaining copy has the same cache track, and for an uncapped Copy::watch to lift an existing cap. A test could serve a, hold A/0, serve b, serve a.consume() again, drop the held group and drain, then write A/5 and assert the weight grows.

Non-blocking

2. A share created after the track has moved past the cap takes post-cap edges. Production::share (stats.rs:1806-1813) copies first and newest from the whole track, which assumes "every group produced so far is up to the cap". That's only true at the switch. A capped copy whose subscription answers later is watched and capped in poll_ready, via let_go's poll_ready on a pending copy. If A produced groups past cap in that window (the tests show a replaced route keeps publishing), the share starts with a post-cap newest. newest only moves forward, so that reader's lag stays overstated until the copy is dropped. Frontier::watch likewise seeds acked from the whole track (stats.rs:1961). One fix is to start a share created when track.latest() > cap with empty edges, so the first frame in a capped group sets them. Another is to skip watching a capped copy that answers after the switch, since its groups up to the cap are already in the cache. There's no test for the pending-then-answered capped copy.

3. Hot-path cost while a cap exists. Every frame written to a capped track takes the shares mutex. That mutex is on the shared cache track, so on a relay it's shared by every viewer of that copy, and it stays taken as long as any reader keeps a capped copy (for example, one that never reads an unread group below the cap). That's fine during a normal overlap and is documented. I'm only flagging that how long it lasts depends on let_go, not on the switch.

The docs are fine: deleting lag-splice.md and its two references (the qos README and quest-flat-lines.md) leaves no dangling links in the branch. CI (Check, Test, WASM, Windows, macOS, Android) is still queued at this head.

Verdict: ITERATE. Finding 1 is a small fix and undercuts the PR's "exact across a switch" goal in the route-flap case. Everything else is fine.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 9b7845a.

Follow-up to 3f2da83. The previous P2 is fixed for a one-way switch: capped shares exclude A/1 while retaining later frames in A/0, and the updated regression checks both weight and edge. Sharing counters per cap is a sensible direction.

P2 — Restore live accounting when a retained route returns. Independently confirmed the same A → B → A defect already described in Grok finding 1, so no duplicate inline comment: stats.rs:1991–1994 installs a capped share, but Frontier::watch at :1957–1958 deduplicates only by Production and never lifts it when the same A track serves again. Hold A/0 across the flap, resolve the new A subscription, then produce A/1: that deliverable group's bytes and edge are excluded. Later, resume.rs:554–555 retires old A and stats.rs:2001–2012 removes the shared source, silencing current A altogether. Preserve watcher/generation ownership, resume whole-track counting for an uncapped serving copy, and remove accounting only after the last dependent copy leaves. Add an egress-stats flap regression asserting weight both before and after old A retires; the existing payload-only flap test does not cover this.

Verification: GitHub-only static review of the one-commit delta, full PR diff, sampler, splice/cursor context and regressions. This remains stacked on #4133; no main/base merge was mistaken for a PR change. No builds or tests run. Exact-head Check, Test, WASM, Windows, macOS and Android are queued. Open/non-draft state, head and review history rechecked before submission.

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: cbc522b.

No new actionable correctness finding in this update. The A → B → A P2 from the previous review is fixed: resume.rs:483–495 unwatches A at the switch, :284–294 watches only a still-serving copy when its subscription answers, and :518–544 no longer unwatches the returning route when old A retires. The new lag_weighs_a_route_that_comes_back regression checks weight before and after releasing old A.

Direction: serving-copy-only accounting is a simpler implementation of the revised scope. Removing per-cap shares also removes their frame-path mutex. Post-switch frames in held retired groups are intentionally unweighed, as now documented; the original post-cap overcount does not return. stats.rs:1894–1915, 1985–2005 preserves retired pending weight at its own edge when the replacement has no frame, and the drop sampler uses the same path.

Verification: GitHub-only static review of the one-commit delta, net diff against the #4133 stack base, sampler, cursor/splice lifecycle and regression assertions. No builds or tests run. Exact-head Check, Test, WASM, Windows, macOS and Android are queued. Open/non-draft state, head and review history rechecked immediately before submission.

[kixelated]

Automated follow-up review of head cbc522b3, a re-review after the push from 9b7845ad. The push is one commit (cbc522b3, "unwatch a replaced copy at the switch again"). It drops the per-cap shares and goes back to the base behavior: the replaced copy is unwatched at the switch, its unsampled bytes are folded at its own edge, and frames written afterwards into a group the reader still holds go unweighed. The PR body and the comment at the switch in resume.rs document that approximation. Review was static only; I ran no tests.

Earlier findings

• 1 (A → B → A loses weight): fixed. The only watched copy is now the one with until == None, and let_go no longer calls unwatch. On the way back to A, A/gen1 was already unwatched at the first switch, so A/gen3's Frontier::watch (stats.rs:1874) adds a fresh source instead of hitting the dedup, and dropping A/gen1 later removes nothing. lag_weighs_a_route_that_comes_back covers exactly this path (hold A/0, go to B, come back to A, drop the held group, then write A/2 and expect 12).
• 2 (a late-answering capped copy seeded post-cap edges): fixed. Copy::poll_ready only watches on answer when until.is_none(), so a copy replaced while pending is never watched.
• 3 (the shares mutex on the frame path): fixed. Production::record is back to atomics only, and the Share, Tally, cap and AtomicBool leftovers are all gone. I found no stale references to caps or shares in stats.rs, resume.rs or track.rs.

Non-blocking

4. A serving copy dropped during a no-route gap stays watched. This existed on the base branch before the PR, and 9b7845ad had fixed it as a side effect ("Unwatched on every retire"); this push brings it back. When the route goes to none, Reader::sync (resume.rs:475-479) records the new generation and returns early, so the old copy keeps until == None and stays watched. If the reader is caught up, let_go (resume.rs:519) then drops that copy without unwatching it, because needed(false) is false and there are no unread groups. When B arrives later, the loop in sync never sees A, so A's source stays in FrontierState::sources for as long as A's cache track is alive. That happens on a relay where other viewers hold the track. Every frame A writes afterwards gets weighed into this reader's lag, which is the over-counting this PR is meant to remove. It's harmless when A's publisher has really gone away, but not when the route was withdrawn while A keeps producing. The fix is small and safe: in let_go, unwatch a copy that's being dropped while until.is_none(). Only such a copy can still be watched, and no other copy shares a watched source with it. A test could serve A, drain, serve none, drain so A is dropped, serve B, write to A, and assert the weight doesn't move.

CI is still running at this head: Test, Check, WASM and macOS are queued, Windows is in progress, and Android passed.

Verdict: MERGE once CI is green. The trade-off you chose is simpler and costs nothing per frame, and all three earlier findings are resolved. Finding 4 is a small follow-up that can go in this PR or a separate one.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Merging at cbc522b3.

• A switch unwatches the replaced copy and records its unsampled bytes at its own edge, including when the replacement has no frame yet and on drop.
• Frames written into a held group of a replaced copy after the switch go unweighed: a documented approximation, chosen by the maintainer over per-cap shares (which added a frame-path mutex and broke route flaps).
• A route flap (A, B, A) stays weighed, with a regression test.
• Grok's findings on 9b7845ad: the flap is fixed; the share-edge and hot-path notes no longer apply since shares were removed.
• OpenAI review of cbc522b3: no actionable findings. CI green.

(Written by Claude Opus 5.5)