mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(relay): preserve reconnect backoff (#2759)
## Why Manual relay recovery polls every three seconds, repeatedly consuming the session's exponential-backoff timer and leaving reconnect behavior stuck or noisy on degraded networks. ## What - Replace fixed-cadence Phase 3 polling with observation of the RelayClient background reconnect loop - Raise the fast-path deadline above the native websocket timeout and enforce that contract in a regression test - Keep the existing 120-second backstop as a soft UI timeout without stopping background retries ## Risk Assessment Medium — this changes live relay recovery timing, but removes a competing retry loop rather than adding one. The existing connection-state subscription remains the success signal. ## References - Stacked on #2310 (`lazyjoe/reconnect-testability-refactor`) - Investigation: `RESEARCH/BUG_RELAY_RECONNECT_HANG.md` - `just ci` - `cd desktop && pnpm typecheck && pnpm check && pnpm test` (3485 pass) - `git diff --check` Generated with Codex Signed-off-by: npub1x4hk035p3p9q39a3fcrd2fe30lpkrhr5dwe0cqzzjphxyyh8m0gsq4vqap <356f67c681884a0897b14e06d527317fc361dc746bb2fc0042906e6212e7dbd1@sprout-oss.stage.blox.sqprod.co> Co-authored-by: npub1x4hk035p3p9q39a3fcrd2fe30lpkrhr5dwe0cqzzjphxyyh8m0gsq4vqap <356f67c681884a0897b14e06d527317fc361dc746bb2fc0042906e6212e7dbd1@sprout-oss.stage.blox.sqprod.co>
This commit is contained in:
co-authored by
npub1x4hk035p3p9q39a3fcrd2fe30lpkrhr5dwe0cqzzjphxyyh8m0gsq4vqap
parent
2f0041595d
commit
499c5d349d
@@ -7,7 +7,10 @@
|
||||
*/
|
||||
|
||||
import assert from "node:assert/strict";
|
||||
import { readFileSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
import test, { mock } from "node:test";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
import {
|
||||
DEFAULT_RECONNECT_TIMING_POLICY,
|
||||
@@ -24,9 +27,7 @@ function makeDeps({
|
||||
connectionStateListener = null,
|
||||
} = {}) {
|
||||
const timers = [];
|
||||
const intervals = [];
|
||||
let timerSeq = 0;
|
||||
let intervalSeq = 0;
|
||||
|
||||
const deps = {
|
||||
preconnect: mock.fn(preconnectResult),
|
||||
@@ -47,38 +48,44 @@ function makeDeps({
|
||||
const idx = timers.findIndex((t) => t.id === id);
|
||||
if (idx !== -1) timers.splice(idx, 1);
|
||||
}),
|
||||
setInterval: mock.fn((fn, ms) => {
|
||||
const id = ++intervalSeq;
|
||||
intervals.push({ id, fn, ms });
|
||||
return id;
|
||||
}),
|
||||
clearInterval: mock.fn((id) => {
|
||||
const idx = intervals.findIndex((t) => t.id === id);
|
||||
if (idx !== -1) intervals.splice(idx, 1);
|
||||
}),
|
||||
// Test helpers to fire timers/intervals.
|
||||
// Test helper to fire timers.
|
||||
_fireTimer: (id) => timers.find((t) => t.id === id)?.fn(),
|
||||
_fireInterval: (id) => intervals.find((t) => t.id === id)?.fn(),
|
||||
_timers: timers,
|
||||
_intervals: intervals,
|
||||
};
|
||||
return deps;
|
||||
}
|
||||
|
||||
// ── Timing policy ─────────────────────────────────────────────────────────────
|
||||
|
||||
const testDir = path.dirname(fileURLToPath(import.meta.url));
|
||||
|
||||
test("fast path outlives the native websocket connection timeout", () => {
|
||||
const rustSource = readFileSync(
|
||||
path.resolve(testDir, "../../../src-tauri/src/native_websocket.rs"),
|
||||
"utf8",
|
||||
);
|
||||
const match = rustSource.match(
|
||||
/const CONNECT_TIMEOUT: Duration = Duration::from_secs\((\d+)\);/,
|
||||
);
|
||||
assert.ok(match, "native websocket connect timeout is declared in seconds");
|
||||
|
||||
const nativeConnectTimeoutMs = Number(match[1]) * 1_000;
|
||||
assert.ok(
|
||||
DEFAULT_RECONNECT_TIMING_POLICY.fastPathTimeoutMs > nativeConnectTimeoutMs,
|
||||
"fast path must wait for the native websocket attempt to settle",
|
||||
);
|
||||
});
|
||||
|
||||
test("default timing policy preserves current reconnect timings", () => {
|
||||
assert.deepEqual(DEFAULT_RECONNECT_TIMING_POLICY, {
|
||||
fastPathTimeoutMs: 4_000,
|
||||
pollIntervalMs: 3_000,
|
||||
fastPathTimeoutMs: 11_000,
|
||||
backstopMs: 120_000,
|
||||
});
|
||||
});
|
||||
|
||||
test("injected timing policy drives fast-path, poll, and backstop timers", async () => {
|
||||
test("injected timing policy drives fast-path and backstop timers", async () => {
|
||||
const ctrl = new RelayReconnectController({
|
||||
fastPathTimeoutMs: 11,
|
||||
pollIntervalMs: 22,
|
||||
backstopMs: 33,
|
||||
});
|
||||
const deps = makeDeps({
|
||||
@@ -91,7 +98,6 @@ test("injected timing policy drives fast-path, poll, and backstop timers", async
|
||||
await ctrl.start(deps);
|
||||
|
||||
assert.equal(deps.setTimeout.mock.calls[0].arguments[1], 11);
|
||||
assert.equal(deps.setInterval.mock.calls[0].arguments[1], 22);
|
||||
assert.equal(deps.setTimeout.mock.calls[1].arguments[1], 33);
|
||||
});
|
||||
|
||||
@@ -113,13 +119,12 @@ test("fast-path success — hook never invoked, onSuccess fires, state resets",
|
||||
});
|
||||
});
|
||||
|
||||
test("fast-path success — no poll interval or backstop scheduled", async () => {
|
||||
test("fast-path success — no backstop scheduled", async () => {
|
||||
const ctrl = new RelayReconnectController();
|
||||
const deps = makeDeps({ preconnectResult: async () => {} });
|
||||
|
||||
await ctrl.start(deps);
|
||||
|
||||
assert.equal(deps.setInterval.mock.calls.length, 0, "no poll interval");
|
||||
// The fast-path withDeadline schedules one timeout for the deadline, but
|
||||
// that is cleared by finally(). After success no backstop should remain.
|
||||
assert.equal(
|
||||
@@ -136,7 +141,7 @@ test("escalation fires only when fast path fails and hook is configured", async
|
||||
let callIdx = 0;
|
||||
const deps = makeDeps({
|
||||
preconnectResult: async () => {
|
||||
// First call (fast path) fails; subsequent calls (poll) succeed.
|
||||
// First call (fast path) fails.
|
||||
if (++callIdx === 1) throw new Error("relay unreachable");
|
||||
},
|
||||
hookConfiguredResult: async () => true,
|
||||
@@ -174,7 +179,7 @@ test("escalation skipped when hook not configured", async () => {
|
||||
);
|
||||
});
|
||||
|
||||
test("hook failure is non-fatal — polling still starts", async () => {
|
||||
test("hook failure is non-fatal — background wait still starts", async () => {
|
||||
const ctrl = new RelayReconnectController();
|
||||
const deps = makeDeps({
|
||||
preconnectResult: async () => {
|
||||
@@ -195,48 +200,12 @@ test("hook failure is non-fatal — polling still starts", async () => {
|
||||
}
|
||||
|
||||
assert.equal(threw, false, "hook failure does not propagate");
|
||||
assert.ok(
|
||||
deps._intervals.length > 0 || deps._timers.length > 0,
|
||||
"phase 3 timers scheduled",
|
||||
);
|
||||
assert.ok(deps._timers.length > 0, "phase 3 backstop scheduled");
|
||||
});
|
||||
|
||||
// ── Phase 3: poll-until-connected ─────────────────────────────────────────────
|
||||
// ── Phase 3: wait for background reconnect ───────────────────────────────────
|
||||
|
||||
test("poll preempts backstop — onSuccess fires when poll wins", async () => {
|
||||
const ctrl = new RelayReconnectController();
|
||||
let callIdx = 0;
|
||||
const deps = makeDeps({
|
||||
preconnectResult: async () => {
|
||||
if (++callIdx <= 2) throw new Error("not yet");
|
||||
// Third call succeeds — simulates poll winning.
|
||||
},
|
||||
hookConfiguredResult: async () => false,
|
||||
});
|
||||
|
||||
// Phase 1 fails, enters phase 3. start() returns false.
|
||||
const promise = ctrl.start(deps);
|
||||
await promise;
|
||||
|
||||
// Fire the poll interval twice to reach the succeeding call.
|
||||
const intervalId = deps._intervals[0]?.id;
|
||||
assert.ok(intervalId !== undefined, "poll interval is active");
|
||||
|
||||
await deps._fireInterval(intervalId); // call 2 — fails
|
||||
await deps._fireInterval(intervalId); // call 3 — succeeds
|
||||
|
||||
assert.equal(
|
||||
deps.onSuccess.mock.calls.length,
|
||||
1,
|
||||
"onSuccess fired after poll win",
|
||||
);
|
||||
assert.deepEqual(ctrl.getState(), {
|
||||
isPending: false,
|
||||
isWaitingOnReconnectHook: false,
|
||||
});
|
||||
});
|
||||
|
||||
test("connection-state emitter fires onSuccess and cancels poll/backstop", async () => {
|
||||
test("connection-state emitter fires onSuccess and cancels backstop", async () => {
|
||||
let capturedListener = null;
|
||||
const ctrl = new RelayReconnectController();
|
||||
const deps = makeDeps({
|
||||
@@ -253,7 +222,7 @@ test("connection-state emitter fires onSuccess and cancels poll/backstop", async
|
||||
|
||||
await ctrl.start(deps);
|
||||
|
||||
// Emitter fires "connected" — should resolve without waiting for poll.
|
||||
// Emitter fires "connected" — should resolve without waiting for backstop.
|
||||
assert.ok(capturedListener !== null, "connection-state listener registered");
|
||||
capturedListener("connected");
|
||||
|
||||
@@ -266,7 +235,6 @@ test("connection-state emitter fires onSuccess and cancels poll/backstop", async
|
||||
isPending: false,
|
||||
isWaitingOnReconnectHook: false,
|
||||
});
|
||||
assert.equal(deps._intervals.length, 0, "poll interval cancelled");
|
||||
assert.equal(deps._timers.length, 0, "backstop cancelled");
|
||||
});
|
||||
|
||||
@@ -299,7 +267,7 @@ test("backstop fires onBackstop, not onSuccess, and resets state", async () => {
|
||||
|
||||
// ── Cancellation token ────────────────────────────────────────────────────────
|
||||
|
||||
test("cancellation token — superseded poll callback does not call onSuccess", async () => {
|
||||
test("cancellation token — superseded attempt does not call onSuccess", async () => {
|
||||
const ctrl = new RelayReconnectController();
|
||||
let preconnectCallCount = 0;
|
||||
let resolveFirstPreconnect;
|
||||
@@ -375,11 +343,6 @@ test("cancel mid fast-path — preconnect resolves SUCCESSFULLY after cancel but
|
||||
"state is idle",
|
||||
);
|
||||
assert.equal(deps._timers.length, 0, "no timers outstanding after cancel");
|
||||
assert.equal(
|
||||
deps._intervals.length,
|
||||
0,
|
||||
"no intervals outstanding after cancel",
|
||||
);
|
||||
});
|
||||
|
||||
test("last subscriber unsubscribe cancels in-flight attempt", async () => {
|
||||
@@ -505,7 +468,7 @@ test("OSS build (hookConfigured returns false) — runHook never called", async
|
||||
|
||||
// ── Synchronous connected emission ───────────────────────────────────────────
|
||||
|
||||
test("sync connected emission — onSuccess fires once, no interval/backstop installed, subscription cleaned up", async () => {
|
||||
test("sync connected emission — onSuccess fires once, no backstop installed, subscription cleaned up", async () => {
|
||||
const ctrl = new RelayReconnectController();
|
||||
|
||||
// subscribeToConnectionState fake that invokes the listener synchronously
|
||||
@@ -538,7 +501,6 @@ test("sync connected emission — onSuccess fires once, no interval/backstop ins
|
||||
1,
|
||||
"onSuccess fires exactly once",
|
||||
);
|
||||
assert.equal(deps._intervals.length, 0, "no poll interval installed");
|
||||
assert.equal(deps._timers.length, 0, "no backstop timer installed");
|
||||
assert.equal(cleanupCalled, true, "subscription cleanup handle was called");
|
||||
assert.deepEqual(
|
||||
|
||||
@@ -12,11 +12,9 @@
|
||||
* returns quickly; any browser-based reconnect flow it triggers runs
|
||||
* asynchronously.
|
||||
*
|
||||
* 3. **Poll-until-connected** — retry `preconnect()` every POLL_INTERVAL_MS.
|
||||
* The moment the relay becomes reachable the next poll succeeds and
|
||||
* success is declared — the backstop is a ceiling, not a delay. The
|
||||
* connection-state emitter is also watched: if the session's background
|
||||
* retry loop reconnects first, we catch it immediately.
|
||||
* 3. **Wait for background reconnect** — observe the session's exponential-
|
||||
* backoff reconnect loop. Success is declared when its connection-state
|
||||
* emitter reports connected; the backstop is a UI ceiling, not a delay.
|
||||
*
|
||||
* The controller is a module-level singleton so all hook instances in the
|
||||
* same app share a single in-flight state. A cancellation token is bumped on
|
||||
@@ -32,16 +30,13 @@
|
||||
export type ReconnectTimingPolicy = {
|
||||
/** Short deadline for the optimistic fast-path attempt. */
|
||||
fastPathTimeoutMs: number;
|
||||
/** Interval between poll attempts during phase 3. */
|
||||
pollIntervalMs: number;
|
||||
/** Maximum total time to keep polling before giving up. */
|
||||
/** Maximum total time to wait before returning control to the UI. */
|
||||
backstopMs: number;
|
||||
};
|
||||
|
||||
/** Current production reconnect timings. */
|
||||
export const DEFAULT_RECONNECT_TIMING_POLICY: ReconnectTimingPolicy = {
|
||||
fastPathTimeoutMs: 4_000,
|
||||
pollIntervalMs: 3_000,
|
||||
fastPathTimeoutMs: 11_000,
|
||||
backstopMs: 120_000,
|
||||
};
|
||||
|
||||
@@ -61,8 +56,6 @@ export type ReconnectDeps = {
|
||||
onBackstop: () => void;
|
||||
setTimeout: (fn: () => void, ms: number) => number;
|
||||
clearTimeout: (id: number) => void;
|
||||
setInterval: (fn: () => void, ms: number) => number;
|
||||
clearInterval: (id: number) => void;
|
||||
};
|
||||
|
||||
function withDeadline<T>(
|
||||
@@ -101,14 +94,12 @@ export class RelayReconnectController {
|
||||
// All async continuations capture the token at their creation point and
|
||||
// bail if it has since been superseded.
|
||||
private attemptToken = 0;
|
||||
// Active timers and subscription for the current attempt. The timer-clear
|
||||
// functions are stored from the deps of the most recent start() call so
|
||||
// that cancel() and teardown do not need the caller to supply deps again.
|
||||
private pollIntervalId: number | null = null;
|
||||
// Active timer and subscription for the current attempt. The timer-clear
|
||||
// function is stored from the deps of the most recent start() call so that
|
||||
// cancel() and teardown do not need the caller to supply deps again.
|
||||
private backstopId: number | null = null;
|
||||
private unsubscribeConnectionState: (() => void) | null = null;
|
||||
private clearTimeoutFn: ((id: number) => void) | null = null;
|
||||
private clearIntervalFn: ((id: number) => void) | null = null;
|
||||
|
||||
/** Subscribe to state changes. Fires immediately with the current state. */
|
||||
subscribe(listener: Listener): () => void {
|
||||
@@ -117,7 +108,7 @@ export class RelayReconnectController {
|
||||
return () => {
|
||||
this.listeners.delete(listener);
|
||||
// Cancel the in-flight attempt when the last subscriber leaves — no UI
|
||||
// is watching the result, so letting poll/backstop callbacks fire
|
||||
// is watching the result, so letting backstop callbacks fire
|
||||
// (possibly mutating state or invoking onSuccess/onBackstop into a
|
||||
// stale closure) would be a resource and correctness leak.
|
||||
if (this.listeners.size === 0) {
|
||||
@@ -141,7 +132,6 @@ export class RelayReconnectController {
|
||||
|
||||
// Store timer-clear fns so cancel() can clear timers without caller deps.
|
||||
this.clearTimeoutFn = deps.clearTimeout;
|
||||
this.clearIntervalFn = deps.clearInterval;
|
||||
|
||||
// Bump the cancellation token before any await.
|
||||
const token = ++this.attemptToken;
|
||||
@@ -164,7 +154,7 @@ export class RelayReconnectController {
|
||||
return true;
|
||||
} catch {
|
||||
if (cancelled()) return false;
|
||||
// Fast path failed — continue to escalation or polling.
|
||||
// Fast path failed — continue to escalation or background reconnect.
|
||||
}
|
||||
|
||||
// ── Phase 2: escalation (hook-configured builds only) ───────────────────
|
||||
@@ -184,7 +174,7 @@ export class RelayReconnectController {
|
||||
await deps.runHook();
|
||||
} catch (err) {
|
||||
// Non-fatal — hook failure means the browser-based reconnect flow may
|
||||
// not have opened, but we still poll for relay reachability.
|
||||
// not have opened, but the session still retries in the background.
|
||||
console.warn(
|
||||
"[RelayReconnectController] transport recovery hook failed:",
|
||||
err,
|
||||
@@ -194,11 +184,10 @@ export class RelayReconnectController {
|
||||
this.setState({ isPending: true, isWaitingOnReconnectHook: true });
|
||||
}
|
||||
|
||||
// ── Phase 3: poll-until-connected ────────────────────────────────────────
|
||||
// Retry preconnect on a fixed interval. The moment the relay becomes
|
||||
// reachable the next poll fires success. The connection-state emitter is
|
||||
// also watched; if the session's background retry loop reconnects first,
|
||||
// we catch it here too.
|
||||
// ── Phase 3: wait for background reconnect ──────────────────────────────
|
||||
// The fast-path preconnect arms the session's exponential-backoff loop.
|
||||
// Observe it rather than issuing fixed-cadence attempts that consume its
|
||||
// pending retry timer.
|
||||
let resolved = false;
|
||||
|
||||
// Capture onSuccess/onBackstop at phase-3 entry so that cancel() (which
|
||||
@@ -236,21 +225,10 @@ export class RelayReconnectController {
|
||||
return false;
|
||||
}
|
||||
|
||||
this.pollIntervalId = deps.setInterval(() => {
|
||||
if (resolved || cancelled()) return;
|
||||
void deps
|
||||
.preconnect()
|
||||
.then(onConnected)
|
||||
.catch(() => {
|
||||
// Poll failed — keep trying.
|
||||
});
|
||||
}, this.timingPolicy.pollIntervalMs);
|
||||
|
||||
this.backstopId = deps.setTimeout(() => {
|
||||
if (resolved || cancelled()) return;
|
||||
resolved = true;
|
||||
// keepAliveRequested remains true via preconnect() — the session's
|
||||
// background retry loop keeps running. The notification is soft.
|
||||
// The session's exponential-backoff reconnect loop remains armed.
|
||||
onBackstop();
|
||||
this.finish(() => {}, false);
|
||||
}, this.timingPolicy.backstopMs);
|
||||
@@ -281,10 +259,6 @@ export class RelayReconnectController {
|
||||
}
|
||||
|
||||
private cancelTimers(): void {
|
||||
if (this.pollIntervalId !== null && this.clearIntervalFn !== null) {
|
||||
this.clearIntervalFn(this.pollIntervalId);
|
||||
this.pollIntervalId = null;
|
||||
}
|
||||
if (this.backstopId !== null && this.clearTimeoutFn !== null) {
|
||||
this.clearTimeoutFn(this.backstopId);
|
||||
this.backstopId = null;
|
||||
|
||||
@@ -30,8 +30,6 @@ function buildDeps(onSuccess: () => void, onBackstop: () => void) {
|
||||
onBackstop,
|
||||
setTimeout: window.setTimeout.bind(window),
|
||||
clearTimeout: window.clearTimeout.bind(window),
|
||||
setInterval: window.setInterval.bind(window),
|
||||
clearInterval: window.clearInterval.bind(window),
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user