diff --git a/cli/__tests__/poll-retry.test.mjs b/cli/__tests__/poll-retry.test.mjs new file mode 100644 index 0000000000..289fe6a4a4 --- /dev/null +++ b/cli/__tests__/poll-retry.test.mjs @@ -0,0 +1,71 @@ +/** + * TASK-025: the run loop had a circuit-breaker for auth errors and none for + * network errors, so a failed fetch retried at a flat interval forever with + * one indistinct log line per attempt. + * + * Each case below pins a property the flat-retry version got wrong. The + * escalation cases matter most: the measured outage was 797 consecutive + * failures whose only trace was 797 identical lines, so "does a sustained + * outage announce itself" is the actual defect, not the delay curve. + */ +import { describe, expect, it } from '@jest/globals'; +import { + POLL_ESCALATE_AT, + POLL_RETRY_MAX_MS, + pollRetryPolicy, +} from '../src/lib/poll-retry.js'; + +const at = (n, over = {}) => pollRetryPolicy({ + consecutiveFailures: n, + intervalMs: 5000, + ...over, +}); + +describe('pollRetryPolicy', () => { + it('costs a single blip nothing — the first retry is still the poll interval', () => { + expect(at(1).delayMs).toBe(5000); + }); + + it('backs off exponentially after that, which flat retry never did', () => { + expect(at(2).delayMs).toBe(10000); + expect(at(3).delayMs).toBe(20000); + expect(at(4).delayMs).toBe(40000); + }); + + it('bounds the backoff, so a long outage does not schedule a retry past the ceiling', () => { + // 2^30 * 5s is ~178 years without the clamp. + const far = at(30); + expect(far.delayMs).toBe(POLL_RETRY_MAX_MS); + expect(far.atCeiling).toBe(true); + }); + + it('escalates on the declared thresholds and stays quiet between them', () => { + // The whole point: 797 failures must not produce 797 equally loud lines. + expect(at(3).escalate).toBe(true); + expect(at(10).escalate).toBe(true); + expect(at(4).escalate).toBe(false); + expect(at(9).escalate).toBe(false); + expect(at(797).escalate).toBe(false); + }); + + it('escalates a bounded number of times across a very long outage', () => { + const loud = Array.from({ length: 800 }, (_, i) => at(i + 1)) + .filter((r) => r.escalate).length; + expect(loud).toBe(POLL_ESCALATE_AT.length); + }); + + it('applies anti-herd jitter, clamped so a bad caller cannot widen the window', () => { + expect(at(2, { jitterRatio: 0.2 }).delayMs).toBe(12000); + // Over the cap: clamped to 0.2 rather than honoured. + expect(at(2, { jitterRatio: 5 }).delayMs).toBe(12000); + expect(at(2, { jitterRatio: -1 }).delayMs).toBe(10000); + expect(at(2, { jitterRatio: NaN }).delayMs).toBe(10000); + }); + + it('falls back to a sane interval rather than NaN when the caller passes junk', () => { + expect(pollRetryPolicy({ consecutiveFailures: 1, intervalMs: 0 }).delayMs).toBe(5000); + expect(pollRetryPolicy({ consecutiveFailures: 1, intervalMs: -1 }).delayMs).toBe(5000); + expect(pollRetryPolicy({ consecutiveFailures: 0, intervalMs: 5000 }).delayMs).toBe(5000); + expect(pollRetryPolicy({ intervalMs: 5000 }).delayMs).toBe(5000); + }); +}); diff --git a/cli/package.json b/cli/package.json index f3fff2bb59..c0683914a8 100644 --- a/cli/package.json +++ b/cli/package.json @@ -1,6 +1,6 @@ { "name": "@commonlyai/cli", - "version": "0.1.15", + "version": "0.1.16", "license": "Apache-2.0", "description": "The Commonly CLI \u2014 connect agents, manage pods, iterate fast", "type": "module", diff --git a/cli/src/commands/agent.js b/cli/src/commands/agent.js index 224243870c..0aca631ada 100644 --- a/cli/src/commands/agent.js +++ b/cli/src/commands/agent.js @@ -29,6 +29,7 @@ import { recordHandledEvent, } from '../lib/session-store.js'; import { readLongTerm, syncBack } from '../lib/memory-bridge.js'; +import { pollRetryPolicy } from '../lib/poll-retry.js'; import { detectMemorySources, composeImport, importMemory } from '../lib/memory-import.js'; import { detectSkills, importSkills } from '../lib/skills-import.js'; import { parseEnvironmentFile, resolveWorkspace } from '../lib/environment.js'; @@ -732,6 +733,7 @@ export const performRun = ({ // reprovision-all; 5+ wastes rate-limit budget after the real-revoke case. let consecutiveAuthErrors = 0; const MAX_AUTH_ERRORS = 3; + let consecutivePollFailures = 0; let consecutiveSpawnFailures = 0; const spawnJitterRatio = retryJitterRatio ?? spawnRetryJitter(agentName); // Per-seat cascade state — lives with the process, like the session store. @@ -1175,6 +1177,7 @@ export const performRun = ({ agentName, instanceId, limit: 10, }); consecutiveAuthErrors = 0; + consecutivePollFailures = 0; for (const event of events) { if (!running) break; let result; @@ -1265,6 +1268,33 @@ export const performRun = ({ running = false; return; } + } else { + // TASK-025: everything that is not an auth rejection used to fall + // through to `onError` with `nextPollDelayMs` still at `intervalMs`, + // because that variable is only reassigned in the spawn-retry branch + // above and a failed fetch never reaches it. So a network outage + // retried flat at the poll interval, forever, with one indistinct + // line per attempt — 797 consecutive `fetch failed` across three + // seats, ~66 minutes, and nothing that read as an outage. + // + // Deliberately NOT a stop. `MAX_AUTH_ERRORS` halts because a rejected + // token cannot heal itself; a network failure usually can, and a seat + // that stops on one is dead until a human notices. + consecutivePollFailures += 1; + const retry = pollRetryPolicy({ + consecutiveFailures: consecutivePollFailures, + intervalMs, + jitterRatio: spawnJitterRatio, + }); + nextPollDelayMs = retry.delayMs; + if (retry.escalate) { + log( + `poll failed ${consecutivePollFailures}x in a row ` + + `(${err?.message || 'unknown error'}) — backing off to ` + + `${formatRetryDelay(retry.delayMs)}` + + `${retry.atCeiling ? ', at the ceiling' : ''}`, + ); + } } onError?.(err); } diff --git a/cli/src/lib/poll-retry.js b/cli/src/lib/poll-retry.js new file mode 100644 index 0000000000..3cab9f04b6 --- /dev/null +++ b/cli/src/lib/poll-retry.js @@ -0,0 +1,81 @@ +/** + * Retry policy for the run loop's own FETCH failures (TASK-025). + * + * `spawn-retry.js` bounds failures of the subprocess a poll produces. This + * bounds failures of the poll itself — the request to + * `/api/agents/runtime/events` that has to succeed before there is anything to + * spawn. They are different failures with different remedies and, until now, + * only one of them had a policy. + * + * The gap, measured: `agent run`'s tick set `nextPollDelayMs = intervalMs` and + * only ever reassigned it inside the spawn-retry branch. A fetch that threw + * never reached that branch, so a network failure retried at a flat 5s + * forever — no backoff, no ceiling, and no escalation. 797 consecutive + * `fetch failed` ran across three seats that way: ~66 minutes at 5s, and the + * only trace was one `onError` line per attempt in a log nobody was tailing. + * + * Two deliberate differences from the auth path directly above it: + * + * 1. NO STOP. `MAX_AUTH_ERRORS` halts the loop because a rejected token does + * not heal on its own — retrying is pure cost and the operator must act. + * A network failure is usually transient, and a seat that stops on one is + * dead until someone notices. So this backs off and stays alive. + * + * 2. ESCALATION IS THE POINT. The harm in the measured outage was not the + * retry rate, it was that 797 failures produced no signal distinguishable + * from one failure. `escalate` fires on a small set of thresholds so a + * sustained outage announces itself at widening intervals instead of + * disappearing into per-attempt noise. + */ + +// Same ceiling as the spawn path: past this, more waiting buys nothing and a +// human is the only thing that resolves it. +export const POLL_RETRY_MAX_MS = 15 * 60 * 1000; + +// Backoff starts only after the first failure has been retried once at the +// normal interval, so a single blip costs nothing. +export const POLL_BACKOFF_AFTER = 1; + +// Failure counts that emit a loud line. Chosen against the measured outage: +// at intervalMs=5000 these land at roughly 15s, 1min, 5min, 20min and 1h of +// sustained failure, so an operator reading the log sees the shape of the +// outage rather than 797 identical lines. +export const POLL_ESCALATE_AT = Object.freeze([3, 10, 30, 60, 120]); + +/** + * Next poll delay after a failed fetch, and whether this attempt should be + * announced loudly. + * + * Exponential from `intervalMs`, bounded by POLL_RETRY_MAX_MS. Jitter is the + * same anti-herd offset the spawn path uses — without it, every seat that lost + * the same upstream retries in lockstep and re-creates the thundering herd on + * recovery. + */ +export const pollRetryPolicy = ({ + consecutiveFailures, + intervalMs, + jitterRatio = 0, +}) => { + const safeIntervalMs = Number.isFinite(intervalMs) && intervalMs > 0 ? intervalMs : 5000; + const failureCount = Number.isInteger(consecutiveFailures) && consecutiveFailures > 0 + ? consecutiveFailures + : 1; + + const steps = Math.max(0, failureCount - POLL_BACKOFF_AFTER); + const raw = safeIntervalMs * (2 ** steps); + const bounded = Math.min(POLL_RETRY_MAX_MS, raw); + + // Clamp rather than trust the caller: a jitterRatio above the cap would + // widen the herd window instead of narrowing it. + const safeJitter = Number.isFinite(jitterRatio) + ? Math.min(0.2, Math.max(0, jitterRatio)) + : 0; + + return { + delayMs: Math.round(bounded * (1 + safeJitter)), + escalate: POLL_ESCALATE_AT.includes(failureCount), + atCeiling: bounded >= POLL_RETRY_MAX_MS, + }; +}; + +export default pollRetryPolicy;