fix(health): lazy runtimeSnapshot capture, explicit probe:false tracking

ajayr · ajayr · commit 8f6f0f869018 · 2026-03-27T23:51:41.000Z
Address review comments from chatgpt-codex-connector:

- #3003609871: Move getRuntimeSnapshot() call lazily inside the health
  refresh cycle (after dedupe decides to proceed), not eagerly at every
  timer tick. Add setRuntimeSnapshotGetter() init pattern — the getter
  is registered once at startup and called inside the refresh function.

- #3003634321: Track explicit probe:false alongside probe:true. Previously
  pendingProbe was only set when probe:true (false was the default, so
  storing it would trigger needless follow-ups). Now we track whenever
  opts.probe is explicitly provided (true or false), and the follow-up
  only fires when the new intent differs from what the in-flight
  refresh is already using.
diff --git a/src/gateway/server-maintenance.ts b/src/gateway/server-maintenance.ts
@@ -1,6 +1,7 @@
 import type { HealthSummary } from "../commands/health.js";
 import { cleanOldMedia } from "../media/store.js";
 import { abortChatRunById, type ChatAbortControllerEntry } from "./chat-abort.js";
+import type { ChannelRuntimeSnapshot } from "./server-channels.js";
 import type { ChatRunEntry } from "./server-chat.js";
 import {
   DEDUPE_MAX,
@@ -10,7 +11,7 @@ import {
 } from "./server-constants.js";
 import type { DedupeEntry } from "./server-shared.js";
 import { formatError } from "./server-utils.js";
-import { setBroadcastHealthUpdate } from "./server/health-state.js";
+import { setBroadcastHealthUpdate, setRuntimeSnapshotGetter } from "./server/health-state.js";
 
 export function startGatewayMaintenanceTimers(params: {
   broadcast: (
@@ -39,6 +40,7 @@ export function startGatewayMaintenanceTimers(params: {
   ) => ChatRunEntry | undefined;
   agentRunSeq: Map<string, number>;
   nodeSendToSession: (sessionKey: string, event: string, payload: unknown) => void;
+  getRuntimeSnapshot?: () => ChannelRuntimeSnapshot | undefined;
   mediaCleanupTtlMs?: number;
 }): {
   tickInterval: ReturnType<typeof setInterval>;
@@ -56,6 +58,13 @@ export function startGatewayMaintenanceTimers(params: {
     params.nodeSendToAllSubscribed("health", snap);
   });
 
+  // Lazily capture runtime snapshot inside the health refresh cycle,
+  // not at every timer tick. Call this after the broadcast fn so the
+  // getter is ready before the first timer fires.
+  if (params.getRuntimeSnapshot) {
+    setRuntimeSnapshotGetter(params.getRuntimeSnapshot);
+  }
+
   // periodic keepalive
   const tickInterval = setInterval(() => {
     const payload = { ts: Date.now() };
diff --git a/src/gateway/server.impl.ts b/src/gateway/server.impl.ts
@@ -125,6 +125,7 @@ import {
   getPresenceVersion,
   incrementPresenceVersion,
   refreshGatewayHealthSnapshot,
+  setRuntimeSnapshotGetter,
 } from "./server/health-state.js";
 import { resolveHookClientIpConfig } from "./server/hooks.js";
 import { createReadinessChecker } from "./server/readiness.js";
@@ -833,6 +834,10 @@ export async function startGatewayServer(
     }
   };
 
+  // Register the lazy getter so health refresh captures the snapshot inside
+  // the refresh cycle, not at the timer tick (after dedupe decides to proceed).
+  setRuntimeSnapshotGetter(safeGetRuntimeSnapshot);
+
   let agentUnsub: (() => void) | null = null;
   let heartbeatUnsub: (() => void) | null = null;
   let transcriptUnsub: (() => void) | null = null;
@@ -885,9 +890,9 @@ export async function startGatewayServer(
           nodeSendToAllSubscribed,
           getPresenceVersion,
           getHealthVersion,
-          refreshGatewayHealthSnapshot: (opts) =>
-            refreshGatewayHealthSnapshot({ ...opts, runtimeSnapshot: safeGetRuntimeSnapshot() }),
+          refreshGatewayHealthSnapshot,
           logHealth,
+          getRuntimeSnapshot: safeGetRuntimeSnapshot,
           dedupe,
           chatAbortControllers,
           chatRunState,
@@ -1181,8 +1186,7 @@ export async function startGatewayServer(
       pluginApprovalManager,
       loadGatewayModelCatalog,
       getHealthCache,
-      refreshHealthSnapshot: (opts) =>
-        refreshGatewayHealthSnapshot({ ...opts, runtimeSnapshot: safeGetRuntimeSnapshot() }),
+      refreshHealthSnapshot: refreshGatewayHealthSnapshot,
       logHealth,
       logGateway: log,
       incrementPresenceVersion,
diff --git a/src/gateway/server/health-state.ts b/src/gateway/server/health-state.ts
@@ -73,12 +73,21 @@ export function setBroadcastHealthUpdate(fn: ((snap: HealthSummary) => void) | n
   broadcastHealthUpdate = fn;
 }
 
+/** Set once at startup — used to lazily capture runtime snapshot inside the refresh cycle. */
+let getRuntimeSnapshot: (() => ChannelRuntimeSnapshot | undefined) | undefined = undefined;
+
+export function setRuntimeSnapshotGetter(fn: () => ChannelRuntimeSnapshot | undefined) {
+  getRuntimeSnapshot = fn;
+}
+
 /** @internal Reset module-level state between tests only. */
 export function __resetHealthStateForTest() {
   healthCache = null;
   healthRefresh = null;
   pendingRuntimeSnapshot = undefined;
   pendingProbe = undefined;
+  // Note: getRuntimeSnapshot is intentionally NOT reset here — it is set once
+  // at startup via setRuntimeSnapshotGetter and should persist across test cases.
   healthVersion = 1;
   presenceVersion = 1;
   broadcastHealthUpdate = null;
@@ -90,14 +99,18 @@ export async function refreshGatewayHealthSnapshot(opts?: {
 }) {
   // Always track the newest runtimeSnapshot so it is not silently discarded when
   // a refresh is already in-flight and a second caller provides a fresher snapshot.
+  // Only record a pending snapshot — the actual capture of getRuntimeSnapshot()
+  // happens lazily inside the refresh cycle, after dedupe decides to proceed.
+  // This avoids calling getRuntimeSnapshot() eagerly on every timer tick.
   if (opts?.runtimeSnapshot !== undefined) {
     pendingRuntimeSnapshot = opts.runtimeSnapshot;
   }
 
   // Track the latest probe intent so the finally block uses the most recent caller's preference.
-  // Only set when probe is true — false is the default, so storing it would trigger needless
-  // follow-up refreshes (hadPendingProbe would be true even though nothing changed).
-  if (opts?.probe) {
+  // Only set when explicitly provided (true or false). This matters because an explicit
+  // probe:false must be distinguishable from "not provided" — if a caller explicitly
+  // requests a non-probe snapshot, we must NOT kick off a follow-up that resets it.
+  if (opts?.probe !== undefined) {
     pendingProbe = opts.probe;
   }
 
@@ -107,11 +120,17 @@ export async function refreshGatewayHealthSnapshot(opts?: {
     pendingRuntimeSnapshot = undefined;
     const probeForRefresh = pendingProbe ?? false;
     pendingProbe = undefined; // must be cleared so finally block only fires for NEW callers
+    // Note: getRuntimeSnapshot is intentionally NOT reset here — it is set once
+    // at startup via setRuntimeSnapshotGetter and should persist across test cases.
 
     healthRefresh = (async () => {
+      // Lazily capture runtime snapshot here (after dedupe has decided to proceed),
+      // not at the call site. This avoids calling getRuntimeSnapshot() on every
+      // timer tick when dedupe might skip the refresh anyway.
+      const runtimeSnapshot = snapshotForRefresh ?? getRuntimeSnapshot?.() ?? undefined;
       const snap = await getHealthSnapshot({
         probe: probeForRefresh,
-        runtimeSnapshot: snapshotForRefresh,
+        runtimeSnapshot,
       });
       healthCache = snap;
       healthVersion += 1;
@@ -126,6 +145,8 @@ export async function refreshGatewayHealthSnapshot(opts?: {
       const followUpProbe = pendingProbe ?? false;
       const hadPendingProbe = pendingProbe !== undefined;
       pendingProbe = undefined;
+      // Note: getRuntimeSnapshot is intentionally NOT reset here — it is set once
+      // at startup via setRuntimeSnapshotGetter and should persist across test cases.
       // If a newer runtimeSnapshot or probe intent arrived while the refresh was
       // in-flight, kick off a follow-up so the latest state is reflected.
       if (pendingRuntimeSnapshot !== undefined || hadPendingProbe) {
