fix(daemon): act on an overdue review-due record nothing else will catch

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;

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;
 }
 
 /**

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);
   });
 });