feat: expose retry and execution outcome telemetry #115
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { Stdout } from "../stdout" | ||
| import { SCHEMA_VERSION } from "./run.errors" | ||
| import { Installation } from "../../installation" | ||
|
|
||
| export type RunInvocationPhase = "validation" | "stdin" | "bootstrap" | "session" | ||
|
|
||
|
|
@@ -19,6 +20,7 @@ export function createRunInvocation(enabled: boolean) { | |
| type, | ||
| timestamp: Date.now(), | ||
| schemaVersion: SCHEMA_VERSION, | ||
| cliVersion: Installation.VERSION, | ||
| invocationID: id, | ||
| ...data, | ||
| }), | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,7 @@ import { Shutdown } from "../shutdown" | |||||
| import { Stdout } from "../stdout" | ||||||
| import { attempt, signals, type Signals } from "../signals" | ||||||
| import { createRunInvocation } from "./run.invocation" | ||||||
| import { Installation } from "../../installation" | ||||||
|
|
||||||
| type ToolProps<T extends Tool.Info> = { | ||||||
| input: Tool.InferParameters<T> | ||||||
|
|
@@ -527,6 +528,7 @@ export const RunCommand = cmd({ | |||||
| type, | ||||||
| timestamp: Date.now(), | ||||||
| schemaVersion: SCHEMA_VERSION, | ||||||
| cliVersion: Installation.VERSION, | ||||||
| invocationID: invocation.id, | ||||||
| sessionID, | ||||||
| ...data, | ||||||
|
|
@@ -542,6 +544,70 @@ export const RunCommand = cmd({ | |||||
| const childSessions = new Set<string>() | ||||||
| const emitted = new Set<string>() | ||||||
| const seqBySession = new Map<string, number>() | ||||||
| type Retry = { | ||||||
| retryID: string | ||||||
| messageID: string | null | ||||||
| providerID: string | null | ||||||
| modelID: string | null | ||||||
| attempt: number | ||||||
| reason: string | ||||||
|
There was a problem hiding this comment. 🟡 Retry.reason typed string; status shape re-declared.
Suggested change
🤖 Fix with your agentWhy this mattersThe bounded Reason union now lives in three mirrors (SessionRetry.Reason in retry.ts, the zod enum in status.ts, the SDK union in types.gen.ts), but run.ts types the stored/emitted Retry.reason as plain type Retry = {\n retryID: string\n messageID: string | null\n providerID: string | null\n modelID: string | null\n attempt: number\n reason: string\n delayMs: number\n } |
||||||
| delayMs: number | ||||||
| } | ||||||
| const retries = new Map<string, Retry>() | ||||||
| const outcomes = new Map<string, { status: "completed" | "error" | "aborted"; finish: string | undefined }>() | ||||||
|
|
||||||
| function retryOutcome(retry: Retry) { | ||||||
| if (!retry.messageID) return "unknown" as const | ||||||
| const outcome = outcomes.get(retry.messageID) | ||||||
| if (!outcome) return "unknown" as const | ||||||
| if (outcome.status === "aborted") return "aborted" as const | ||||||
| if (outcome.status === "error" || outcome.finish === "error" || outcome.finish === "content-filter") { | ||||||
| return "failed" as const | ||||||
| } | ||||||
| return "recovered" as const | ||||||
| } | ||||||
|
|
||||||
| function resolveRetry(sid: string, outcome: "recovered" | "failed" | "aborted" | "unknown") { | ||||||
| const retry = retries.get(sid) | ||||||
| if (!retry) return | ||||||
| retries.delete(sid) | ||||||
| emit("retry_complete", { ...retry, outcome }) | ||||||
| } | ||||||
|
|
||||||
| function scheduleRetry( | ||||||
| sid: string, | ||||||
| status: { | ||||||
| attempt: number | ||||||
| next: number | ||||||
| retryID?: string | ||||||
| messageID?: string | ||||||
| providerID?: string | ||||||
| modelID?: string | ||||||
| reason?: string | ||||||
| delayMs?: number | ||||||
| }, | ||||||
| ) { | ||||||
|
There was a problem hiding this comment. 🟡 Legacy retry statuses cannot dedupe on replay.
Suggested change
🤖 Fix with your agentWhy this mattersscheduleRetry dedupes repeated retry statuses by comparing current.retryID === status.retryID. For an older server without correlation fields, the stored retryID is a locally generated UUID while status.retryID is undefined, so the comparison can never be true. Any redelivery of the same legacy retry status (SSE reconnect/replay or status re-broadcast) emits a fresh retry_scheduled plus a supersession retry_complete pair with a new retryID, double-counting retries and scheduled backoff in the documented per-retryID metrics. Correlated retries dedupe correctly; only legacy servers are affected. Repro: Given an older server (retry status without retryID) whose SSE stream reconnects with event replay, When the same retry status is delivered twice, Then two retry_scheduled/retry_complete pairs are emitted with distinct generated retryIDs. const current = retries.get(sid)\n if (current && current.retryID === status.retryID) return\n if (current) {\n resolveRetry(\n sid,\n current.messageID && status.messageID === current.messageID ? "failed" : retryOutcome(current),\n )\n } |
||||||
| const current = retries.get(sid) | ||||||
| if (current && current.retryID === status.retryID) return | ||||||
| if (current) { | ||||||
| resolveRetry( | ||||||
| sid, | ||||||
| current.messageID && status.messageID === current.messageID ? "failed" : retryOutcome(current), | ||||||
| ) | ||||||
| } | ||||||
| const retry = { | ||||||
| retryID: status.retryID ?? crypto.randomUUID(), | ||||||
| messageID: status.messageID ?? null, | ||||||
| providerID: status.providerID ?? null, | ||||||
| modelID: status.modelID ?? null, | ||||||
| attempt: status.attempt, | ||||||
| reason: status.reason ?? "unknown", | ||||||
| delayMs: status.delayMs ?? Math.max(0, status.next - Date.now()), | ||||||
| } | ||||||
| retries.set(sid, retry) | ||||||
| emit("retry_scheduled", retry) | ||||||
| } | ||||||
|
|
||||||
| function nextSeq(sid: string): number { | ||||||
| const n = (seqBySession.get(sid) ?? 0) + 1 | ||||||
| seqBySession.set(sid, n) | ||||||
|
|
@@ -569,7 +635,17 @@ export const RunCommand = cmd({ | |||||
| if (event.type === "message.updated" && event.properties.info.role === "assistant") { | ||||||
| const info = event.properties.info | ||||||
| if (args.format === "json") { | ||||||
| if (info.sessionID === sessionID) { | ||||||
| const retry = retries.get(info.sessionID) | ||||||
| if (retry?.messageID && retry.messageID !== info.id) { | ||||||
| resolveRetry(info.sessionID, retryOutcome(retry)) | ||||||
| } | ||||||
| } | ||||||
| if (info.sessionID === sessionID && info.time.completed !== undefined) { | ||||||
| const status = | ||||||
| info.error?.name === "MessageAbortedError" ? "aborted" : info.error ? "error" : "completed" | ||||||
| outcomes.set(info.id, { status, finish: info.finish }) | ||||||
| if (emitted.has(info.id)) continue | ||||||
|
There was a problem hiding this comment. 🟡 continue skips loop tail for duplicate completions. 🤖 Fix with your agentWhy this mattersThe original code guarded only the emission with outcomes.set(info.id, { status, finish: info.finish })\n if (emitted.has(info.id)) continue\n emitted.add(info.id)\n const usage = terminalUsage(info) |
||||||
| emitted.add(info.id) | ||||||
| const usage = terminalUsage(info) | ||||||
|
|
||||||
|
|
@@ -606,7 +682,7 @@ export const RunCommand = cmd({ | |||||
| cost: info.cost, | ||||||
| tokens: usage.tokens, | ||||||
| usageStatus: usage.usageStatus, | ||||||
| status, | ||||||
| finish: info.finish, | ||||||
| }) | ||||||
| } | ||||||
|
|
@@ -711,6 +787,10 @@ export const RunCommand = cmd({ | |||||
| if (!control.current) process.exitCode = 1 | ||||||
| invocation.error(props.error) | ||||||
| const classified = classifySessionError(props.error) | ||||||
| resolveRetry( | ||||||
| props.sessionID, | ||||||
| classified.reason === "interrupted" || classified.reason === "terminated" ? "aborted" : "failed", | ||||||
| ) | ||||||
| // Structured session_error is the telemetry/CI channel for the | ||||||
| // primary session. The legacy "error" event below is the raw | ||||||
| // pass-through for both primary and child-session failures. | ||||||
|
|
@@ -760,15 +840,23 @@ export const RunCommand = cmd({ | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (event.type === "session.status") { | ||||||
| const status = event.properties.status | ||||||
| if (status.type === "retry" && event.properties.sessionID === sessionID) { | ||||||
| scheduleRetry(event.properties.sessionID, status) | ||||||
| } | ||||||
| if (status.type === "idle") { | ||||||
| if (event.properties.sessionID === sessionID) { | ||||||
| const retry = retries.get(event.properties.sessionID) | ||||||
| if (retry) resolveRetry(event.properties.sessionID, retryOutcome(retry)) | ||||||
| break | ||||||
| } | ||||||
| if (childSessions.has(event.properties.sessionID)) { | ||||||
| emit("subagent_complete", { | ||||||
| subagentSessionID: event.properties.sessionID, | ||||||
| parentSessionID: sessionID, | ||||||
| }) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -862,6 +950,7 @@ export const RunCommand = cmd({ | |||||
| function interrupt(signal: Signals.Info) { | ||||||
| error ??= signal.message | ||||||
| invocation.error(signal.message) | ||||||
| resolveRetry(sessionID, "aborted") | ||||||
| report(signal.reason, String(signal.code), signal.message) | ||||||
| abort() | ||||||
| } | ||||||
|
|
@@ -878,6 +967,7 @@ export const RunCommand = cmd({ | |||||
| const classified = classifySessionError(cause) | ||||||
| error ??= classified.message | ||||||
| invocation.error(cause) | ||||||
| resolveRetry(sessionID, "failed") | ||||||
|
There was a problem hiding this comment. 🟠 Exit path mislabels cancelled retry as "failed". --- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -967,6 +967,9 @@
const classified = classifySessionError(cause)
error ??= classified.message
invocation.error(cause)
- resolveRetry(sessionID, "failed")
+ resolveRetry(
+ sessionID,
+ classified.reason === "interrupted" || classified.reason === "terminated" ? "aborted" : "failed",
+ )
report(classified.reason, classified.code, classified.message)🤖 Fix with your agentWhy this mattersThe fatal-exit handler hardcodes resolveRetry(sessionID, "failed") immediately after computing classified = classifySessionError(cause), which can classify the cause as cancellation ("interrupted" for the new MessageAbortedError branch, or "terminated"). The sibling session.error handler maps those same reasons to "aborted", and interrupt() maps signal cancellation to "aborted"; EVENTS.md states "cancellation is const classified = classifySessionError(cause)
error ??= classified.message
invocation.error(cause)
resolveRetry(sessionID, "failed")
report(classified.reason, classified.code, classified.message) |
||||||
| report(classified.reason, classified.code, classified.message) | ||||||
| complete(error) | ||||||
| if (control.current) { | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,6 +380,12 @@ export namespace SessionProcessor { | |
| attempt, | ||
| message: retry, | ||
| next: Date.now() + delay, | ||
| retryID: crypto.randomUUID(), | ||
| messageID: input.assistantMessage.id, | ||
| providerID: input.model.providerID, | ||
| modelID: input.model.id, | ||
| reason: SessionRetry.reason(error), | ||
| delayMs: delay, | ||
| }) | ||
| await SessionRetry.sleep(delay, input.abort).catch(() => {}) | ||
| continue | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,8 @@ import { MessageV2 } from "./message-v2" | |
| import { iife } from "@/util/iife" | ||
|
|
||
| export namespace SessionRetry { | ||
| export type Reason = "rate_limit" | "timeout" | "network" | "provider" | "unknown" | ||
|
|
||
| export const RETRY_INITIAL_DELAY = 2000 | ||
| export const RETRY_BACKOFF_FACTOR = 2 | ||
| export const RETRY_MAX_DELAY_NO_HEADERS = 30_000 // 30 seconds | ||
|
|
@@ -99,4 +101,32 @@ export namespace SessionRetry { | |
| return undefined | ||
| } | ||
| } | ||
|
|
||
| /** A bounded category for telemetry dimensions. This does not affect retry policy. */ | ||
| export function reason(error: ReturnType<NamedError["toObject"]>): Reason { | ||
| const data = error.data && typeof error.data === "object" ? error.data : undefined | ||
| const status = | ||
| data && "statusCode" in data && typeof data.statusCode === "number" | ||
| ? data.statusCode | ||
| : data && "status" in data && typeof data.status === "number" | ||
| ? data.status | ||
| : undefined | ||
| const detail = (() => { | ||
| if (!data) return "" | ||
| const values = ["message", "responseBody"].flatMap((key) => { | ||
| if (!(key in data)) return [] | ||
| const value = data[key as keyof typeof data] | ||
| return typeof value === "string" ? [value] : [] | ||
| }) | ||
| return values.join(" ") | ||
| })() | ||
|
|
||
| if (status === 429 || /rate[-_\s]?limit|too[-_\s]many[-_\s]requests/i.test(detail)) return "rate_limit" | ||
| if (status === 408 || /\btimeout\b|timed out|time out/i.test(detail)) return "timeout" | ||
| if (/network[-_\s]?error|fetch failed|connection|socket|ECONN|ENOTFOUND|EAI_AGAIN/i.test(detail)) return "network" | ||
| if ((status !== undefined && status >= 500) || /overloaded|unavailable|resource[-_\s]exhausted/i.test(detail)) { | ||
| return "provider" | ||
| } | ||
| return "unknown" | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,12 @@ export namespace SessionStatus { | |||||
| attempt: z.number(), | ||||||
| message: z.string(), | ||||||
| next: z.number(), | ||||||
| retryID: z.string().optional(), | ||||||
|
There was a problem hiding this comment. 🟡 Unvalidated server fields re-emitted as metric dimensions. 🤖 Fix with your agentWhy this mattersThe new optional fields on the SSE-parsed retry status are unconstrained: retryID/messageID/providerID/modelID are bare z.string() and delayMs is a bare z.number() with no non-negative/finite/size bounds. In --attach mode these values cross the trust boundary from the attached server and are forwarded verbatim by run.ts scheduleRetry into retry_scheduled/retry_complete NDJSON events, where EVENTS.md designates providerID/modelID as metric dimensions and delayMs as a summed quantity per distinct retryID. A buggy or compromised server can inject arbitrary high-cardinality dimension labels or negative/huge delayMs values into CI measurement pipelines. The fallback path clamps via Math.max(0, next - Date.now()), but server-supplied delayMs passes through unchecked. retryID: z.string().optional(),\n messageID: z.string().optional(),\n providerID: z.string().optional(),\n modelID: z.string().optional(),\n reason: z.enum(["rate_limit", "timeout", "network", "provider", "unknown"]).optional(),\n delayMs: z.number().optional(), |
||||||
| messageID: z.string().optional(), | ||||||
| providerID: z.string().optional(), | ||||||
| modelID: z.string().optional(), | ||||||
| reason: z.enum(["rate_limit", "timeout", "network", "provider", "unknown"]).optional(), | ||||||
|
There was a problem hiding this comment. 🟡 Closed reason enum vs documented open set.
Suggested change
🤖 Fix with your agentWhy this mattersEVENTS.md's new retry_scheduled docs tell consumers to treat retryID: z.string().optional(),\n messageID: z.string().optional(),\n providerID: z.string().optional(),\n modelID: z.string().optional(),\n reason: z.enum(["rate_limit", "timeout", "network", "provider", "unknown"]).optional(),\n delayMs: z.number().optional(), |
||||||
| delayMs: z.number().optional(), | ||||||
| }), | ||||||
| z.object({ | ||||||
| type: z.literal("busy"), | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,14 @@ describe("classifySessionError (#63)", () => { | |
| expect(classifySessionError(err).reason).toBe("timeout") | ||
| }) | ||
|
|
||
| test("stored MessageAbortedError → interrupted", () => { | ||
| const res = classifySessionError({ | ||
| name: "MessageAbortedError", | ||
| data: { message: "Session cancelled" }, | ||
| }) | ||
| expect(res.reason).toBe("interrupted") | ||
| }) | ||
|
|
||
| test("heap OOM → oom", () => { | ||
| const err = new Error("JavaScript heap out of memory") | ||
| expect(classifySessionError(err).reason).toBe("oom") | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Doc: superseded retry not always "failed" in code.
🤖 Fix with your agent
Why this matters
EVENTS.md states unconditionally "A retry superseded by another retry is
failed", but scheduleRetry in run.ts resolves the superseded retry as "failed" only when both retries carry the same non-null messageID; otherwise it falls back to retryOutcome(current), which can emit "recovered", "aborted", or "unknown". A pending retry for one assistant message superseded by a retry for a different message (or by an uncorrelated older-server retry) is therefore not necessarily "failed" — the PR's own test ("keeps retry outcomes scoped to their owning message across tool turns") relies on the cross-message behavior. Either the doc or the implementation is the contract; consumers following the doc will mis-aggregate the retry-recovery-rate metric.