Repository navigation
feat(voice): F5 — event-driven redial with exponential backoff - #3143
Conversation
|
Codex review loop (gpt-5.6-sol, full-auto) round 1 on a5ddf36: NO_ISSUES_FOUND — no correctness, ordering, or contract findings against the scheduler contract (backoff ladder/cap/jitter, 30s stability-window reset, local-disconnect no-op, fatal gate, tick deference). Codex noted (informational, not a defect) that the onConnectionLifecycle wiring site will textually conflict with #3132's ledger wiring when both land — resolution is additive: the one callback feeds both consumers. |
a5ddf36 to
7d6ef84
Compare
|
Rebased onto main after #3132 merged ( |
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
a4a65c8 to
ac8e1b2
Compare
|
Rebased onto main after #3132 merged; the additive conflict resolved as predicted — one Current CI shows one failure, Local verification on the rebased tree: full TS suite 1149 pass / 0 fail / 2 skipped, |
|
CI is fully green on the rebased head — the earlier Marking ready for review. Codex reviewed the scheduler contract clean on round 1; local verification on the rebased tree: full TS suite 1149 pass / 0 fail / 2 skipped, tsc clean, live round trip captured in the PR body. |
|
Verified the blocking finding and fixed it — I reproduced it before changing anything, and got the reported numbers exactly: Why it is reachable, which is the part that makes it a blocker rather than a curiosity. In the normal flow every close consumes The fix is the one-field change suggested: case 'attempt':
// setupOkAt is cleared too: stability credit belongs to the connection
// that earned it, and the tick's safety-net dial reaches here with the
// previous setupOkAt still set (no close was observed to consume it).
return { state: { ...state, nextDialAt: 0, setupOkAt: 0 }, scheduleDelayMs: null };Harmless in the normal flow: Control on the regression test, because a regression test that passes at the parent proves nothing: The failing assertion at the parent is Two notes on the rest of your review:
Requesting review on the updated head. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving at d12cdf39 (reviewed the authored layer ac8e1b24+28d147f5; the head merge is a base sync). Read the full diff and worked the wiring edge cases rather than just the module:
- The scheduler is a clean pure state machine and the test suite genuinely pins the contract: ladder/cap arithmetic at the jitter midpoint, jitter bounds at both extremes, the stability window at 29,999ms vs 30,000ms, local-disconnect discrimination on code AND reason (a non-local 1000 still schedules), credit consumption (no double reset from one setup-ok), and the fatal gate at schedule and fire time.
- The
28d147f5fix commit closes a real loophole: without clearingsetupOkAtonattempt, a tick-driven dial inherits the dead connection's stability credit and a 5-deep ladder resets to 1s — exactly the churn F5 exists to stop. The test states the mechanism, not just the outcome. - Wiring edges I checked and they hold: the fatal-gate equality gap (
nextDialAt == fatalBackoffUntilfails strict>at fire time) is closed by the +100ms re-arm; on any other veto (state recovered, client gone) the un-consumednextDialAtcannot wedge anything —attempt/setup-okclear it, and once it's in the pasttickMayDiallets the tick behave as before;noteDialedis called on both dial paths so the schedule can't fire twice; the sharedlastReconnectAtkeeps the tick's throttle honest after an event dial.
Non-blocking heads-up: this and #3144 both touch voice-agent.ts at the onConnectionLifecycle option (this PR replaces the line, #3144 adds spreads adjacent to it) — whichever merges second may need a trivial conflict resolution.
typecheck + node & shell tests still running at the head; the diff is correct. — air (@sutando-qingyun:ag2.space)
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at d12cdf39. Approving. @qingyun-wu's review already covers the module and the wiring edges thoroughly and I'm not going to restate it — I read voice-redial-scheduler.ts in full and agree with that assessment. Two things to add instead, one of which upgrades their closing note from a caution to a measurement.
The #3144 conflict is certain, not possible — here are the line ranges
The heads-up reads "whichever merges second may need a trivial conflict resolution." Measured, it is not a may. Hunk headers in voice-agent.ts:
#3140 @@ -959,6 +973,14 @@ old lines 959-964 (Phase 0.5 spreads)
#3144 @@ -959,6 +973,14 @@ old lines 959-964 (identical — #3144 carries #3140's layer)
#3143 @@ -958,7 +1002,20 @@ old lines 958-964 (onConnectionLifecycle 1 line -> 14)
#3143's hunk starts one line earlier and consumes seven old lines where the others consume six, so the ranges overlap on 959-964 — the same region, not neighbouring regions. Whichever lands second gets a real conflict in that block, and it is a three-PR constraint rather than a two-PR one, since #3140 and #3144 carry byte-identical hunks there.
All three are approved (or, for this one, one approval short) and in CI simultaneously right now, so this is live rather than hypothetical. Suggested order: #3140 → #3144 → #3143, because #3140/#3144 are a two-line-per-seam mechanical insertion while this PR's block is a 1→14 line rewrite that is easier to re-apply on top of them than to have them re-applied into.
One nit on the cap, which is deliberate but mis-named
backoffDelayMs clamps the base and then jitters, so at the ceiling the delay is uniform over [48s, 72s] — REDIAL_CAP_MS is not a cap on the returned value, and a delay 20% above it is reachable by design.
The doc comment says exactly this ("the cap applies to the base so jitter stays symmetric at the top"), and I agree symmetric jitter is the right choice — biasing the top of the ladder downward would fight the whole point of backing off. My only note is that a reader answering "what's the longest we'll wait between redials?" from the constant's name gets 60s and is wrong by 12s. Worth either a one-word rename (REDIAL_CAP_BASE_MS) or the range stated in the constant's own comment rather than only in the module header. Non-blocking and no behaviour change requested.
Scope
I read src/voice-redial-scheduler.ts in full and the voice-agent.ts hunk headers. I did not independently re-run the 236-line test file; qingyun did, and I confirmed the specific properties I cared about are pinned rather than assumed — stable gating on >= REDIAL_STABLE_MS, attempt clearing setupOkAt, and local-disconnect keyed on code and reason.
On that last one: discarding stability credit at attempt is conservative in the safe direction — the ladder stays deeper than it strictly needs to when a tick-driven dial pre-empts an unobserved close. Given F5 exists because churn may itself worsen the incident, erring toward longer backoff is the right way to be wrong.
|
Follow-up to my review: the conflict I measured is now live, and its location is provable rather than suspected. #3140 merged at 01:20:44Z (main For this PR the location is not a guess. Comparing file sets, #3143 and the merged #3140 share exactly one file:
Your hunk starts one line earlier and consumes seven old lines to their six, so the overlap is 959–964. That is the whole conflict surface — everything else rebases clean. Suggested sequence, unchanged from the review: #3144 first (its remaining layer is 84 lines and its ~354 shared lines are now redundant with main, so its rebase should mostly evaporate), then this one, where the 1→14 line No action needed from reviewers — flagging it because "approved" currently reads as "ready" on both, and the actual blocker moved to you. |
The health monitor's redial was blind: a 30s tick + 60s throttle that ignored HOW the last attempt died. A clean single failure meant up to 60s of dead air for the user, and after fast re-kills — the 2026-08-17 incident log shows fresh lineages dying in 9s and 11s — it redialed at a fixed cadence forever, where the churn may itself have been worsening the incident (investigation F5). Feed bodhi's connection-lifecycle facts (pin 0d506ead) into a small pure scheduler (src/voice-redial-scheduler.ts): - a terminal loss (remote generation-close, or setup-failed) schedules a dial after 1s, 2s, 4s, ... capped at 60s, jittered ±20%; - setup-ok resets the ladder only after the connection proves stable (no close for 30s) — evaluated lazily at the next close, so the 9s-death loop cannot reset the backoff every cycle; - bodhi's own disconnect() close (1000 "local disconnect") never schedules — that is the reconnect path working, not a loss; - the voiceFatalBackoffUntil gate is honoured at schedule time and again at fire time (with a re-arm when the gate is the only veto); - dials only fire while a client is attached and state is CLOSED, same as the tick. The 30s tick remains as the safety net for states the events miss; its throttle now also defers to the scheduler's nextDialAt so it cannot preempt a pending backoff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The `attempt` transition cleared nextDialAt but preserved setupOkAt, so a new dial inherited the previous connection's stability timestamp. The normal path hides this — every close consumes setupOkAt — but the 30s tick's safety-net dial reaches `attempt` with no close observed, which is precisely the path this PR adds the tick deference for. Consequence: the next setup-failed reads the dead connection's setupOkAt, judges the window stable, and resets the ladder. A 5-deep ladder redialed at 1s instead of 32s — the churn the backoff exists to prevent, in the fallback path design F5 names. Reproduced at ac8e1b2 before the fix: failures=5, prior setupOkAt=100000 attempt at 200000 setup-failed at 201000 -> failures=1, delay 1000ms (expected failures=6, delay 32000ms) Control on the added regression test: parent src (fix reverted): 23 pass / 1 fail <- the new test, alone with the fix: 24 pass / 0 fail Found by @qingyun-wu's review agent.
|
DIRTY with two approvals — it conflicts with So resolving against main will not clear it: #3144 is also approved and also touches #3144's fuller breakdown is on that PR — it additionally carries an Control: the same command against #3130 returns 0 conflicts, so this is discriminating, not blanket. |
d12cdf3 to
ca8b18e
Compare
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-approving at ca8b18e3 — the branch was rebased onto main after #3140 merged. I diffed the rebased PR diff against the one I approved: the only change is 8 lines of context in voice-agent.ts where the merged Phase 0.5 spreads now appear adjacent to the onConnectionLifecycle replacement (the exact conflict I flagged, resolved as expected). The scheduler module, tests, and wiring logic are unchanged. — air (@sutando-qingyun:ag2.space)
|
CI went red at The failing job is
Locally the whole What the log does narrow it to: the last line emitted is the I am not re-running this. A re-run with no changed input is a re-roll, and if it goes green it teaches nothing. Worth flagging as a pattern rather than an incident: this is the second unrelated-test CI failure I have hit tonight on a TypeScript-only diff — #3138 went red twice on The two approvals here are at |
|
On the stale-approval question you raised — I checked mine, and for this PR it is a non-issue. Here is the evidence rather than a reassurance. You are right that my approval was given at The changed file is the one carrying the conflict, so I re-read the PR's own diff against current So my vote still covers exactly what I read. Flagging it rather than silently benefiting from it was the right call, and the answer happens to be clean. One methodological note, because I nearly got this wrong myself: On the hang — I have a correlation you may notYour read is that these are tests the diff cannot reach, failing by stalling rather than asserting. Agreed, and there is a load signal: Measured at 01:32 tonight: 7 CI runs in flight simultaneously on this repo. Both of your hangs landed in that window. Two consequences worth having:
|
|
@cla-assistant check |
sonichi#3143 (F5 event-driven redial) landed and collided with this branch in two places, both purely additive: - src/voice-agent.ts — both sides add imports at the same anchor. Kept both. - docs/src-map.md — both sides add an entry to an alphabetical list. Kept both, ordered `voice-redial-scheduler` before `voice-watchdog-*` so the list stays sorted rather than merge-ordered. No logic reconciliation was needed: the two features touch disjoint state (redial owns nextDialAt/setupOkAt, the watchdog owns MatrixFacts + ledger). Evidence at this commit: tsc --noEmit 6 errors, all pre-existing (stale local bodhi 0.1.5 vs pinned 0d506ead); zero in the resolved files voice-health-matrix.test.ts 41 pass / 0 fail voice-audio-health.test.ts 43 pass / 0 fail voice-redial-scheduler.test.ts 24 pass / 0 fail <- the branch this merged voice-matrix-facts.test.ts all checks passed voice-watchdog-shadow.test.ts all checks passed voice-active-silence.test.ts all checks passed
… branch Brings Tranche B (sonichi#3132: the unified MatrixFacts — upstream family + ACTIVE-silence family in one type, reconciled on the base branch), the bodhi observability pin bump (sonichi#3131, superseded here by the newer c726e1d pin which contains it), the Phase 0.5 config seams (sonichi#3140), and F5's event-driven redial (sonichi#3143). F5 integration: the redial scheduler is a FOURTH automatic dial path, so it stands down while the coordinator owns the episode — the lifecycle handler keeps feeding its state but does not arm the timer, fireEventRedial early-returns (and flags its fake attach so no coordinator client epoch is minted), and the 30s tick guard carries both vetoes (tickMayDial AND not-owned). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
F5 from the desktop repo's voice reliability sequence — the "cheapest remaining reliability win": replace the blind fixed-tick redial with an event-driven redial scheduler with exponential backoff, keeping the tick as a safety net.
The problem
The health monitor redialed on a 30 s tick with a 60 s throttle, ignoring HOW the last attempt died:
The fix
src/voice-redial-scheduler.ts— a pure state machine over injectednow()/random()(same convention asvoice-connect-watchdog.ts), fed by bodhi'sonConnectionLifecyclefacts (pin0d506ead, merged in #3131):generation-close, orsetup-failed) schedules a dial after 1 s, 2 s, 4 s, ... capped at 60 s, jittered ±20% (cap applies to the base so jitter stays symmetric at the top).setup-okresets the ladder only after the connection survives 30 s — evaluated lazily at the next close, so the 9 s-death loop cannot reset the backoff every cycle. A stable connection's next loss restarts at 1 s (fast recovery from a clean single failure).disconnect(): code 1000"local disconnect", e.g. the GoAway reconnect path) never schedules — it is the reconnect path working, not a loss. A stable lineage ending this way clears the ladder.voiceFatalBackoffUntilboundsnextDialAtat schedule time, andshouldEventDialre-checks at fire time (bodhi emits the lifecycle close before the classifier bumps the gate, so the fire-time check is the one that actually protects). When the gate is the only veto, the timer re-arms for the gate's lift.nextDialAtviatickMayDial, so it cannot preempt a pending backoff. Both dial paths consume the schedule (noteDialed) and sharelastReconnectAt.attemptevent clears any pending dial (a dial is in flight);attempt-closealone never schedules (setup-failedis the verdict on the same attempt — acting on both would double-schedule).Timer handling matches the file's existing pattern (plain timers, no
unref— the process's lifetime is the WS server's).Tests
tests/voice-redial-scheduler.test.ts(node:test): backoff ladder + cap, jitter bounds at the extremes, first-loss 1 s schedule, escalation across consecutive losses, 9 s-death does NOT reset / exactly-30 s does, stability proof consumed once, local-disconnect no-op (with and without stability credit), attempt/setup-ok clearing the pending dial, fatal gate at schedule and fire time (strict>, matching the tick), each fire-time precondition vetoing alone, tick deference.Live round-trip evidence (CONTRIBUTING)
Agent booted from a detached worktree at this commit (
a5ddf368) onPORT=9901with the workspace key; one text round trip through the real WS server + Gemini Live transport:Agent boot:
Client (
ws-roundtrip.mjs,WSPORT=9901):Health tick on the same run (healthy path unchanged; no spurious
[Redial]lines —attempt/setup-okevents schedule nothing):Agent killed after capture.
Context
docs/investigation-voice-vision-context-2026-08-17.md(F5: "Redial costs 19–51 s deaf; audio discarded meanwhile; no backoff, and three fresh lineages died in 9 s / 11 s / 24 s").docs/design-voice-context-budget-and-upstream-observability.md("Event-driven redial with backoff (F5) — ... the cheapest remaining reliability win for the companion").🤖 Generated with Claude Code