diff --git a/packages/sidecar/dist/cli.js b/packages/sidecar/dist/cli.js index 0c97fbd..37ebbc5 100755 --- a/packages/sidecar/dist/cli.js +++ b/packages/sidecar/dist/cli.js @@ -8957,11 +8957,14 @@ function armRemoteAt(state, key, at, trigger) { state.remoteTimers.set(key, { timer, at }); } function armReviewDue(state, key, when) { - const at = reviewDueAt(readReviewDue(key), when, Date.now(), scheduleFor(key, state.options).debounceSeconds * 1000); + const at = reviewDueAt(readReviewDue(key), when, Date.now(), scheduleFor(key, state.options).debounceSeconds * 1000, comingBack(state, key)); if (at !== undefined) armRemoteAt(state, key, at, "review-due"); } -function reviewDueAt(due, when, now, debounceMs) { +function comingBack(state, key) { + return state.syncing.has(key) || state.remoteTimers.has(key) || state.cycleCount < (state.skipUntilCycle.get(key) ?? 0); +} +function reviewDueAt(due, when, now, debounceMs, comingBack2) { if (due === undefined) return; if (due > now) @@ -8970,7 +8973,7 @@ function reviewDueAt(due, when, now, debounceMs) { return now; if (when === "after-sync") return now + Math.max(SETTLE_WINDOW_MS, debounceMs); - return; + return comingBack2 ? undefined : now; } function remoteIsDue(state, key) { const last = state.lastRemoteSyncAt.get(key) ?? 0; diff --git a/packages/sidecar/src/daemon.ts b/packages/sidecar/src/daemon.ts index 9fc4c0b..9263d6a 100644 --- a/packages/sidecar/src/daemon.ts +++ b/packages/sidecar/src/daemon.ts @@ -645,10 +645,19 @@ function armRemoteAt(state: DaemonState, key: PeerKey, at: number, trigger: stri * `when` says what is asking; see reviewDueAt. */ function armReviewDue(state: DaemonState, key: PeerKey, when: ReviewDueCheck): void { - const at = reviewDueAt(readReviewDue(key), when, Date.now(), scheduleFor(key, state.options).debounceSeconds * 1000); + const at = reviewDueAt(readReviewDue(key), when, Date.now(), scheduleFor(key, state.options).debounceSeconds * 1000, comingBack(state, key)); if (at !== undefined) armRemoteAt(state, key, at, "review-due"); } +/** + * Whether something already brings this peer back to the remote: a sync in + * flight (its end checks again), a round trip booked, or a backoff the + * interval poll ends. + */ +function comingBack(state: DaemonState, key: PeerKey): boolean { + return state.syncing.has(key) || state.remoteTimers.has(key) || state.cycleCount < (state.skipUntilCycle.get(key) ?? 0); +} + export type ReviewDueCheck = "start" | "change" | "after-sync"; /** @@ -662,15 +671,18 @@ export type ReviewDueCheck = "start" | "change" | "after-sync"; * that sync never reached the gate (it lost the lock to another) or ran a * CLI too old to clear it. Ask again a debounce later, never under the * settle window, so neither case costs more than the cadence the peer keeps. - * - "change": another peer's record changed. Nothing new is known about this - * one, and booking it now would override the retry already booked for it. + * - "change": a record changed, this one or another peer's. If something + * already brings this peer back (`comingBack`), leave it: booking now would + * override that retry or cut a backoff short. Otherwise go now — a record + * written moments ahead (an approval's) is already past by the time the + * change is read, and nothing else would come for it before the poll. */ -export function reviewDueAt(due: number | undefined, when: ReviewDueCheck, now: number, debounceMs: number): number | undefined { +export function reviewDueAt(due: number | undefined, when: ReviewDueCheck, now: number, debounceMs: number, comingBack: boolean): number | undefined { if (due === undefined) return undefined; if (due > now) return due; if (when === "start") return now; if (when === "after-sync") return now + Math.max(SETTLE_WINDOW_MS, debounceMs); - return undefined; + return comingBack ? undefined : now; } /** diff --git a/packages/sidecar/tests/daemon.test.ts b/packages/sidecar/tests/daemon.test.ts index 384cf44..3c02f1e 100644 --- a/packages/sidecar/tests/daemon.test.ts +++ b/packages/sidecar/tests/daemon.test.ts @@ -262,22 +262,34 @@ describe("reviewDueAt", () => { test("books a due time still ahead as is, whoever asks", () => { for (const when of ["start", "change", "after-sync"] as const) { - expect(reviewDueAt(now + 30_000, when, now, 60_000)).toBe(now + 30_000); + for (const comingBack of [false, true]) { + expect(reviewDueAt(now + 30_000, when, now, 60_000, comingBack)).toBe(now + 30_000); + } } }); test("leaves a peer without a due review alone", () => { - expect(reviewDueAt(undefined, "start", now, 60_000)).toBeUndefined(); + expect(reviewDueAt(undefined, "start", now, 60_000, false)).toBeUndefined(); + expect(reviewDueAt(undefined, "change", now, 60_000, false)).toBeUndefined(); }); - test("an overdue review goes now at start, and never on another peer's change", () => { - expect(reviewDueAt(now - 1, "start", now, 60_000)).toBe(now); - // Booking it here would override a retry already booked for this peer. - expect(reviewDueAt(now - 1, "change", now, 60_000)).toBeUndefined(); + test("an overdue review goes now at start", () => { + expect(reviewDueAt(now - 1, "start", now, 60_000, false)).toBe(now); + }); + + test("a change goes now for an overdue review nothing else will come back for", () => { + // An approval writes its due time moments ahead; the debounced scan reads + // it after it has passed, with no retry booked to catch it. + expect(reviewDueAt(now - 400, "change", now, 60_000, false)).toBe(now); + }); + + test("a change leaves an overdue review to whatever already brings the peer back", () => { + // Booking now would override a booked retry or cut a backoff short. + expect(reviewDueAt(now - 1, "change", now, 60_000, true)).toBeUndefined(); }); test("an overdue review a sync left behind waits a debounce, and never less than the settle window", () => { - expect(reviewDueAt(now - 1, "after-sync", now, 60_000)).toBe(now + 60_000); - expect(reviewDueAt(now - 1, "after-sync", now, 0)).toBe(now + 5000); + expect(reviewDueAt(now - 1, "after-sync", now, 60_000, false)).toBe(now + 60_000); + expect(reviewDueAt(now - 1, "after-sync", now, 0, false)).toBe(now + 5000); }); });