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
2026-03-21 00:56:36 -04:00 · 2026-03-21 00:56:36 -04:00 · 4db9a6d8d0
commit 4db9a6d8d0
parent b80bba8e12
4 changed files with 84 additions and 31 deletions
--- 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;
--- 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) {
--- 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,
--- 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<ReturnType<typeof contextEngine.compact>>;
              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`,
                );