From 4db9a6d8d04b4c1858371cad3be8d277d944cf3f Mon Sep 17 00:00:00 2001 From: Joey Krug Date: Sat, 21 Mar 2026 00:56:36 -0400 Subject: [PATCH] fix: address codex review comments on #46417 - Increase MAX_TIMEOUT_COMPACTION_ATTEMPTS to 2 so timeout retries can reach failover rotation after consecutive failures - Increment timeoutCompactionAttempts before the attempt so failed compactions count toward the retry cap - Use dynamic counter for attempt/maxAttempts (consistent with overflow path) - Call runPostCompactionSideEffects after successful timeout compaction to run hooks that the normal compaction entrypoint performs - Add proper test mocking for compact.js import --- src/agents/pi-embedded-runner/compact.ts | 2 +- .../run.overflow-compaction.harness.ts | 7 ++ .../run.timeout-triggered-compaction.test.ts | 91 ++++++++++++++----- src/agents/pi-embedded-runner/run.ts | 15 ++- 4 files changed, 84 insertions(+), 31 deletions(-) diff --git a/src/agents/pi-embedded-runner/compact.ts b/src/agents/pi-embedded-runner/compact.ts index d76a01ed5af..55ecb9881b4 100644 --- a/src/agents/pi-embedded-runner/compact.ts +++ b/src/agents/pi-embedded-runner/compact.ts @@ -360,7 +360,7 @@ function syncPostCompactionSessionMemory(params: { return Promise.resolve(); } -async function runPostCompactionSideEffects(params: { +export async function runPostCompactionSideEffects(params: { config?: OpenClawConfig; sessionKey?: string; sessionFile: string; diff --git a/src/agents/pi-embedded-runner/run.overflow-compaction.harness.ts b/src/agents/pi-embedded-runner/run.overflow-compaction.harness.ts index b1664434d67..99363d4652c 100644 --- a/src/agents/pi-embedded-runner/run.overflow-compaction.harness.ts +++ b/src/agents/pi-embedded-runner/run.overflow-compaction.harness.ts @@ -62,6 +62,7 @@ export const mockedContextEngine = { export const mockedContextEngineCompact = mockedContextEngine.compact; export const mockedCompactDirect = mockedContextEngine.compact; +export const mockedRunPostCompactionSideEffects = vi.fn(async () => {}); export const mockedEnsureRuntimePluginsLoaded = vi.fn<(params?: unknown) => void>(); export const mockedPrepareProviderRuntimeAuth = vi.fn(async () => undefined); export const mockedRunEmbeddedAttempt = @@ -243,6 +244,8 @@ export function resetRunOverflowCompactionHarnessMocks(): void { ); mockedResolveAuthProfileOrder.mockReset(); mockedResolveAuthProfileOrder.mockReturnValue([]); + mockedRunPostCompactionSideEffects.mockReset(); + mockedRunPostCompactionSideEffects.mockResolvedValue(undefined); } export async function loadRunOverflowCompactionHarness(): Promise<{ @@ -408,6 +411,10 @@ export async function loadRunOverflowCompactionHarness(): Promise<{ sessionLikelyHasOversizedToolResults: mockedSessionLikelyHasOversizedToolResults, })); + vi.doMock("./compact.js", () => ({ + runPostCompactionSideEffects: mockedRunPostCompactionSideEffects, + })); + vi.doMock("./utils.js", () => ({ describeUnknownError: vi.fn((err: unknown) => { if (err instanceof Error) { diff --git a/src/agents/pi-embedded-runner/run.timeout-triggered-compaction.test.ts b/src/agents/pi-embedded-runner/run.timeout-triggered-compaction.test.ts index 647d449ceca..d74b689e695 100644 --- a/src/agents/pi-embedded-runner/run.timeout-triggered-compaction.test.ts +++ b/src/agents/pi-embedded-runner/run.timeout-triggered-compaction.test.ts @@ -15,6 +15,7 @@ import { resetRunOverflowCompactionHarnessMocks, mockedSessionLikelyHasOversizedToolResults, mockedTruncateOversizedToolResultsInSession, + mockedRunPostCompactionSideEffects, overflowBaseRunParams, } from "./run.overflow-compaction.harness.js"; @@ -48,6 +49,8 @@ describe("timeout-triggered compaction", () => { mockedGlobalHookRunner.runBeforeCompaction.mockReset(); mockedGlobalHookRunner.runAfterCompaction.mockReset(); mockedPickFallbackThinkingLevel.mockReset(); + mockedRunPostCompactionSideEffects.mockReset(); + mockedRunPostCompactionSideEffects.mockResolvedValue(undefined); mockedContextEngine.info.ownsCompaction = false; mockedCompactDirect.mockResolvedValue({ ok: false, @@ -112,7 +115,7 @@ describe("timeout-triggered compaction", () => { runtimeContext: expect.objectContaining({ trigger: "timeout_recovery", attempt: 1, - maxAttempts: 1, + maxAttempts: 2, }), }), ); @@ -145,6 +148,13 @@ describe("timeout-triggered compaction", () => { // Verify the loop continued (retry happened) expect(mockedRunEmbeddedAttempt).toHaveBeenCalledTimes(2); + // Post-compaction side effects (transcript update, memory sync) should fire + expect(mockedRunPostCompactionSideEffects).toHaveBeenCalledTimes(1); + expect(mockedRunPostCompactionSideEffects).toHaveBeenCalledWith( + expect.objectContaining({ + sessionFile: "/tmp/session.json", + }), + ); expect(result.meta.error).toBeUndefined(); }); @@ -270,10 +280,10 @@ describe("timeout-triggered compaction", () => { } as never, }), ); - // Compaction succeeds on first timeout + // First compaction succeeds mockedCompactDirect.mockResolvedValueOnce( makeCompactionSuccess({ - summary: "timeout recovery compaction", + summary: "timeout recovery compaction 1", tokensBefore: 150000, tokensAfter: 80000, }), @@ -287,13 +297,29 @@ describe("timeout-triggered compaction", () => { } as never, }), ); + // Second compaction also succeeds + mockedCompactDirect.mockResolvedValueOnce( + makeCompactionSuccess({ + summary: "timeout recovery compaction 2", + tokensBefore: 140000, + tokensAfter: 70000, + }), + ); + // Third attempt after second compaction: still times out + mockedRunEmbeddedAttempt.mockResolvedValueOnce( + makeAttemptResult({ + timedOut: true, + lastAssistant: { + usage: { input: 130000 }, + } as never, + }), + ); const result = await runEmbeddedPiAgent(overflowBaseRunParams); - // Compaction was only attempted once (first timeout); second timeout - // should NOT trigger compaction because the counter is exhausted. - expect(mockedCompactDirect).toHaveBeenCalledTimes(1); - expect(mockedRunEmbeddedAttempt).toHaveBeenCalledTimes(2); + // Both compaction attempts used; third timeout falls through. + expect(mockedCompactDirect).toHaveBeenCalledTimes(2); + expect(mockedRunEmbeddedAttempt).toHaveBeenCalledTimes(3); // Falls through to timeout error payload (failover rotation path) expect(result.payloads?.[0]?.isError).toBe(true); expect(result.payloads?.[0]?.text).toContain("timed out"); @@ -367,6 +393,7 @@ describe("timeout-triggered compaction", () => { it("counts compacted:false timeout compactions against the retry cap across profile rotation", async () => { useTwoAuthProfiles(); + // Attempt 1 (profile-a): timeout → compaction #1 fails → rotate to profile-b mockedRunEmbeddedAttempt .mockResolvedValueOnce( makeAttemptResult({ @@ -377,6 +404,7 @@ describe("timeout-triggered compaction", () => { } as never, }), ) + // Attempt 2 (profile-b): timeout → compaction #2 fails → cap exhausted → rotation .mockResolvedValueOnce( makeAttemptResult({ timedOut: true, @@ -386,39 +414,49 @@ describe("timeout-triggered compaction", () => { } as never, }), ); - mockedCompactDirect.mockResolvedValueOnce({ - ok: false, - compacted: false, - reason: "nothing to compact", - }); + mockedCompactDirect + .mockResolvedValueOnce({ + ok: false, + compacted: false, + reason: "nothing to compact", + }) + .mockResolvedValueOnce({ + ok: false, + compacted: false, + reason: "nothing to compact", + }); const result = await runEmbeddedPiAgent(overflowBaseRunParams); - expect(mockedCompactDirect).toHaveBeenCalledTimes(1); - expect(mockedCompactDirect).toHaveBeenCalledWith( + expect(mockedCompactDirect).toHaveBeenCalledTimes(2); + expect(mockedCompactDirect).toHaveBeenNthCalledWith( + 1, expect.objectContaining({ runtimeContext: expect.objectContaining({ authProfileId: "profile-a", attempt: 1, - maxAttempts: 1, + maxAttempts: 2, + }), + }), + ); + expect(mockedCompactDirect).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + runtimeContext: expect.objectContaining({ + authProfileId: "profile-b", + attempt: 2, + maxAttempts: 2, }), }), ); expect(mockedRunEmbeddedAttempt).toHaveBeenCalledTimes(2); - expect(mockedRunEmbeddedAttempt).toHaveBeenNthCalledWith( - 1, - expect.objectContaining({ authProfileId: "profile-a" }), - ); - expect(mockedRunEmbeddedAttempt).toHaveBeenNthCalledWith( - 2, - expect.objectContaining({ authProfileId: "profile-b" }), - ); expect(result.payloads?.[0]?.isError).toBe(true); expect(result.payloads?.[0]?.text).toContain("timed out"); }); it("counts thrown timeout compactions against the retry cap across profile rotation", async () => { useTwoAuthProfiles(); + // Attempt 1 (profile-a): timeout → compaction #1 throws → rotate to profile-b mockedRunEmbeddedAttempt .mockResolvedValueOnce( makeAttemptResult({ @@ -429,6 +467,7 @@ describe("timeout-triggered compaction", () => { } as never, }), ) + // Attempt 2 (profile-b): timeout → compaction #2 throws → cap exhausted → rotation .mockResolvedValueOnce( makeAttemptResult({ timedOut: true, @@ -438,11 +477,13 @@ describe("timeout-triggered compaction", () => { } as never, }), ); - mockedCompactDirect.mockRejectedValueOnce(new Error("engine crashed")); + mockedCompactDirect + .mockRejectedValueOnce(new Error("engine crashed")) + .mockRejectedValueOnce(new Error("engine crashed again")); const result = await runEmbeddedPiAgent(overflowBaseRunParams); - expect(mockedCompactDirect).toHaveBeenCalledTimes(1); + expect(mockedCompactDirect).toHaveBeenCalledTimes(2); expect(mockedRunEmbeddedAttempt).toHaveBeenCalledTimes(2); expect(mockedRunEmbeddedAttempt).toHaveBeenNthCalledWith( 1, diff --git a/src/agents/pi-embedded-runner/run.ts b/src/agents/pi-embedded-runner/run.ts index 5c4c1cd1665..35f10eb0a7d 100644 --- a/src/agents/pi-embedded-runner/run.ts +++ b/src/agents/pi-embedded-runner/run.ts @@ -65,6 +65,7 @@ import { import { ensureRuntimePluginsLoaded } from "../runtime-plugins.js"; import { derivePromptTokens, normalizeUsage, type UsageLike } from "../usage.js"; import { redactRunIdentifier, resolveRunWorkspaceDir } from "../workspace-run.js"; +import { runPostCompactionSideEffects } from "./compact.js"; import { buildEmbeddedCompactionRuntimeContext } from "./compaction-runtime-context.js"; import { resolveGlobalLane, resolveSessionLane } from "./lanes.js"; import { log } from "./logger.js"; @@ -815,7 +816,7 @@ export async function runEmbeddedPiAgent( } }; - const MAX_TIMEOUT_COMPACTION_ATTEMPTS = 1; + const MAX_TIMEOUT_COMPACTION_ATTEMPTS = 2; const MAX_OVERFLOW_COMPACTION_ATTEMPTS = 3; const MAX_RUN_LOOP_ITERATIONS = resolveMaxRunRetryIterations(profileCandidates.length); let overflowCompactionAttempts = 0; @@ -1112,14 +1113,13 @@ export async function runEmbeddedPiAgent( ); } else if (tokenUsedRatio > 0.65) { const timeoutDiagId = createCompactionDiagId(); - const nextTimeoutCompactionAttempt = timeoutCompactionAttempts + 1; + timeoutCompactionAttempts++; log.warn( `[timeout-compaction] LLM timed out with high prompt token usage (${Math.round(tokenUsedRatio * 100)}%); ` + - `attempting compaction before retry diagId=${timeoutDiagId}`, + `attempting compaction before retry (attempt ${timeoutCompactionAttempts}/${MAX_TIMEOUT_COMPACTION_ATTEMPTS}) diagId=${timeoutDiagId}`, ); let timeoutCompactResult: Awaited>; await runOwnsCompactionBeforeHook("timeout recovery"); - timeoutCompactionAttempts = nextTimeoutCompactionAttempt; try { timeoutCompactResult = await contextEngine.compact({ sessionId: params.sessionId, @@ -1149,7 +1149,7 @@ export async function runEmbeddedPiAgent( ownerNumbers: params.ownerNumbers, trigger: "timeout_recovery", diagId: timeoutDiagId, - attempt: nextTimeoutCompactionAttempt, + attempt: timeoutCompactionAttempts, maxAttempts: MAX_TIMEOUT_COMPACTION_ATTEMPTS, }, }); @@ -1162,6 +1162,11 @@ export async function runEmbeddedPiAgent( await runOwnsCompactionAfterHook("timeout recovery", timeoutCompactResult); if (timeoutCompactResult.compacted) { autoCompactionCount += 1; + await runPostCompactionSideEffects({ + config: params.config, + sessionKey: params.sessionKey, + sessionFile: params.sessionFile, + }); log.info( `[timeout-compaction] compaction succeeded for ${provider}/${modelId}; retrying prompt`, );