mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
fix(embedded-runner): preserve provider errors on cleanup takeover (#84321)
Summary: - The PR preserves provider-facing embedded-runner prompt errors when cleanup detects session takeover, keeps the takeover signal fatal for fallback, and adds focused regressions. - PR surface: Source +52, Tests +92. Total +144 across 5 files. - Reproducibility: yes. Source inspection shows current main can let cleanup takeover replace a prior prompt/p ... rror and can normalize a provider-looking takeover wrapper before fallback sees it as coordination failure. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(embedded-runner): preserve takeover during fallback - PR branch already contained follow-up commit before automerge: fix(clawsweeper): address review for automerge-openclaw-openclaw-8405… Validation: - ClawSweeper review passed for head050c779cfa. - Required merge gates passed before the squash merge. Prepared head SHA:050c779cfaReview: https://github.com/openclaw/openclaw/pull/84321#issuecomment-4492087335 Co-authored-by: abnershang <abner.shang@gmail.com> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com> Approved-by: takhoffman Co-authored-by: takhoffman <781889+takhoffman@users.noreply.github.com>
This commit is contained in:
co-authored by
abnershang
clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com>
clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com>
takhoffman
parent
bcde7b138a
commit
7fbca96a0c
@@ -279,6 +279,9 @@ export function isNonProviderRuntimeCoordinationError(err: unknown): boolean {
|
||||
if (isFailoverError(err)) {
|
||||
return false;
|
||||
}
|
||||
if (isEmbeddedAttemptSessionTakeover(err)) {
|
||||
return true;
|
||||
}
|
||||
return resolveFailoverClassificationFromError(err) === null;
|
||||
}
|
||||
|
||||
|
||||
@@ -940,6 +940,38 @@ describe("runWithModelFallback", () => {
|
||||
expect(run).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("aborts fallback when a provider prompt error carries cleanup session takeover", async () => {
|
||||
const cfg = makeCfg({
|
||||
agents: {
|
||||
defaults: {
|
||||
model: {
|
||||
primary: "openai/gpt-5.4",
|
||||
fallbacks: ["anthropic/claude-sonnet-4-6", "openai/gpt-4.1-mini"],
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
const cleanupTakeover = new Error(
|
||||
"session file changed while embedded prompt lock was released: /tmp/session.jsonl",
|
||||
);
|
||||
cleanupTakeover.name = "EmbeddedAttemptSessionTakeoverError";
|
||||
const providerFacingError = new Error("provider rejected request: rate limit", {
|
||||
cause: cleanupTakeover,
|
||||
});
|
||||
providerFacingError.name = "EmbeddedAttemptSessionTakeoverError";
|
||||
const run = vi.fn().mockRejectedValue(providerFacingError);
|
||||
|
||||
await expect(
|
||||
runWithModelFallback({
|
||||
cfg,
|
||||
provider: "openai",
|
||||
model: "gpt-5.4",
|
||||
run,
|
||||
}),
|
||||
).rejects.toBe(providerFacingError);
|
||||
expect(run).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("aborts the fallback chain on session write-lock timeout instead of trying every model (#83510)", async () => {
|
||||
const cfg = makeCfg({
|
||||
agents: {
|
||||
|
||||
@@ -258,6 +258,9 @@ async function runFallbackCandidate<T>(params: {
|
||||
if (isCommandLaneTaskTimeoutError(err)) {
|
||||
throw err;
|
||||
}
|
||||
if (isNonProviderRuntimeCoordinationError(err)) {
|
||||
throw err;
|
||||
}
|
||||
// Normalize abort-wrapped rate-limit errors (e.g. Google Vertex RESOURCE_EXHAUSTED)
|
||||
// so they become FailoverErrors and continue the fallback loop instead of aborting.
|
||||
const normalizedFailover = coerceToFailoverError(err, {
|
||||
|
||||
@@ -20,6 +20,7 @@ import {
|
||||
resolvePromptCacheTouchTimestamp,
|
||||
runAttemptContextEngineBootstrap,
|
||||
} from "./attempt.context-engine-helpers.js";
|
||||
import { EmbeddedAttemptSessionTakeoverError } from "./attempt.session-lock.js";
|
||||
import {
|
||||
cleanupTempPaths,
|
||||
createDefaultEmbeddedSession,
|
||||
@@ -1313,6 +1314,65 @@ describe("runEmbeddedAttempt context engine sessionKey forwarding", () => {
|
||||
expectInitialLockReleasedBeforePostTurnWrite(lockEvents);
|
||||
});
|
||||
|
||||
it("preserves provider prompt errors when cleanup reacquire detects session takeover", async () => {
|
||||
const providerError = new Error("provider rejected request: HTTP 400");
|
||||
let acquireCount = 0;
|
||||
let cleanupReacquireSessionFile: string | undefined;
|
||||
hoisted.acquireSessionWriteLockMock.mockImplementation(async (params) => {
|
||||
acquireCount += 1;
|
||||
if (acquireCount === 3) {
|
||||
cleanupReacquireSessionFile = params.sessionFile;
|
||||
await fs.appendFile(params.sessionFile, '{"type":"message","id":"takeover"}\n', "utf8");
|
||||
}
|
||||
return { release: async () => {} };
|
||||
});
|
||||
|
||||
const error = await createContextEngineAttemptRunner({
|
||||
contextEngine: createContextEngineBootstrapAndAssemble(),
|
||||
sessionKey,
|
||||
tempPaths,
|
||||
sessionPrompt: async () => {
|
||||
throw providerError;
|
||||
},
|
||||
}).catch((err: unknown) => err);
|
||||
|
||||
expect(error).toBeInstanceOf(Error);
|
||||
expect((error as Error).name).toBe("EmbeddedAttemptSessionTakeoverError");
|
||||
expect((error as Error).message).toBe(providerError.message);
|
||||
expect((error as Error).cause).toBeInstanceOf(EmbeddedAttemptSessionTakeoverError);
|
||||
if (!cleanupReacquireSessionFile) {
|
||||
throw new Error("expected cleanup lock reacquire");
|
||||
}
|
||||
expect(((error as Error).cause as Error).message).toContain(cleanupReacquireSessionFile);
|
||||
expect((error as { promptError?: unknown }).promptError).toBe(providerError);
|
||||
expect(hoisted.flushPendingToolResultsAfterIdleMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("keeps cleanup session takeover fatal when no provider prompt error exists", async () => {
|
||||
let releasingCleanupLock = false;
|
||||
hoisted.flushPendingToolResultsAfterIdleMock.mockImplementation(async () => {
|
||||
releasingCleanupLock = true;
|
||||
});
|
||||
hoisted.acquireSessionWriteLockMock.mockImplementation(async (params) => ({
|
||||
release: async () => {
|
||||
if (releasingCleanupLock) {
|
||||
throw new EmbeddedAttemptSessionTakeoverError(params.sessionFile);
|
||||
}
|
||||
},
|
||||
}));
|
||||
|
||||
await expect(
|
||||
createContextEngineAttemptRunner({
|
||||
contextEngine: createContextEngineBootstrapAndAssemble(),
|
||||
sessionKey,
|
||||
tempPaths,
|
||||
sessionPrompt: async (session) => {
|
||||
session.messages = [...session.messages, doneMessage];
|
||||
},
|
||||
}),
|
||||
).rejects.toBeInstanceOf(EmbeddedAttemptSessionTakeoverError);
|
||||
});
|
||||
|
||||
it("uses assembled context as the default precheck authority", async () => {
|
||||
let sawPrompt = false;
|
||||
const hugeHistory = "large raw history ".repeat(2_000);
|
||||
|
||||
@@ -339,6 +339,7 @@ import {
|
||||
shouldInjectHeartbeatPrompt,
|
||||
} from "./attempt.prompt-helpers.js";
|
||||
import {
|
||||
EmbeddedAttemptSessionTakeoverError,
|
||||
createEmbeddedAttemptSessionLockController,
|
||||
installPromptSubmissionLockRelease,
|
||||
installSessionExternalHookWriteLock,
|
||||
@@ -1060,6 +1061,28 @@ export function shouldRunLlmOutputHooksForAttempt(params: { promptErrorSource: s
|
||||
return params.promptErrorSource !== "hook:before_agent_run";
|
||||
}
|
||||
|
||||
function shouldPreservePromptErrorAfterCleanupError(params: {
|
||||
promptError: unknown;
|
||||
cleanupError: unknown;
|
||||
}): boolean {
|
||||
return (
|
||||
Boolean(params.promptError) &&
|
||||
params.cleanupError instanceof EmbeddedAttemptSessionTakeoverError
|
||||
);
|
||||
}
|
||||
|
||||
class EmbeddedAttemptPromptErrorWithCleanupTakeoverError extends Error {
|
||||
readonly promptError: unknown;
|
||||
readonly cleanupError: EmbeddedAttemptSessionTakeoverError;
|
||||
|
||||
constructor(params: { promptError: unknown; cleanupError: EmbeddedAttemptSessionTakeoverError }) {
|
||||
super(formatErrorMessage(params.promptError), { cause: params.cleanupError });
|
||||
this.name = "EmbeddedAttemptSessionTakeoverError";
|
||||
this.promptError = params.promptError;
|
||||
this.cleanupError = params.cleanupError;
|
||||
}
|
||||
}
|
||||
|
||||
function hasVisiblePendingToolMediaReply(
|
||||
reply: { mediaUrls?: string[]; audioAsVoice?: boolean } | null | undefined,
|
||||
): boolean {
|
||||
@@ -5173,8 +5196,17 @@ export async function runEmbeddedAttempt(
|
||||
} catch (err) {
|
||||
cleanupError = err;
|
||||
}
|
||||
const synthesizedCleanupTakeoverError =
|
||||
!cleanupError && promptError && sessionLockController.hasSessionTakeover()
|
||||
? new EmbeddedAttemptSessionTakeoverError(params.sessionFile)
|
||||
: undefined;
|
||||
const cleanupFailure = cleanupError ?? synthesizedCleanupTakeoverError;
|
||||
const shouldPreservePromptError = shouldPreservePromptErrorAfterCleanupError({
|
||||
promptError,
|
||||
cleanupError: cleanupFailure,
|
||||
});
|
||||
emitDiagnosticRunCompleted?.(
|
||||
cleanupError
|
||||
cleanupFailure
|
||||
? "error"
|
||||
: beforeAgentRunBlocked
|
||||
? "blocked"
|
||||
@@ -5183,13 +5215,27 @@ export async function runEmbeddedAttempt(
|
||||
: aborted || timedOut || idleTimedOut || timedOutDuringCompaction
|
||||
? "aborted"
|
||||
: "completed",
|
||||
cleanupError ?? promptError,
|
||||
shouldPreservePromptError ? promptError : (cleanupFailure ?? promptError),
|
||||
beforeAgentRunBlocked
|
||||
? { blockedBy: beforeAgentRunBlockedBy ?? "before_agent_run" }
|
||||
: undefined,
|
||||
);
|
||||
if (cleanupError) {
|
||||
await Promise.reject(cleanupError);
|
||||
if (cleanupFailure) {
|
||||
if (shouldPreservePromptError) {
|
||||
log.warn(
|
||||
`embedded attempt cleanup detected session takeover after prompt failure; preserving prompt error: ` +
|
||||
`runId=${params.runId} sessionId=${params.sessionId} ` +
|
||||
`promptError=${formatErrorMessage(promptError)} cleanupError=${formatErrorMessage(cleanupFailure)}`,
|
||||
);
|
||||
await Promise.reject(
|
||||
new EmbeddedAttemptPromptErrorWithCleanupTakeoverError({
|
||||
promptError,
|
||||
cleanupError: cleanupFailure as EmbeddedAttemptSessionTakeoverError,
|
||||
}),
|
||||
);
|
||||
} else {
|
||||
await Promise.reject(cleanupFailure);
|
||||
}
|
||||
}
|
||||
}
|
||||
} finally {
|
||||
|
||||
Reference in New Issue
Block a user