Compare commits
8 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 17b610e1a1 | |||
| ca913c76ea | |||
| 20d4b12522 | |||
| ec43c0e0d1 | |||
| 93cc7b1a44 | |||
| 851e49e2d7 | |||
| 4101df566b | |||
| 9d04cad360 |
@@ -407,6 +407,12 @@ async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
|
||||
activityTimeout: 300_000,
|
||||
onActivityTimeout: params.onActivityTimeout,
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
// run claude in its own process group so SIGKILL on activity timeout /
|
||||
// outer cancellation reaches any subprocesses it spawns (rg, file
|
||||
// watchers, mcp transports, etc). claude itself is a node bundle so
|
||||
// there's no shim-orphan issue like opencode-ai/bin/opencode, but
|
||||
// detached + killGroup is the right default for any agent runtime.
|
||||
killGroup: true,
|
||||
onStdout: async (chunk) => {
|
||||
const text = chunk.toString();
|
||||
output += text;
|
||||
|
||||
@@ -59,6 +59,22 @@ type OpenCodeConfig = {
|
||||
[key: string]: unknown;
|
||||
};
|
||||
|
||||
/**
|
||||
* Per-inference `max_tokens` reservation the agent sends to the upstream
|
||||
* model. OpenCode's default is 32_000 (sized for long-running TUI sessions
|
||||
* where a human user might want big outputs). Pullfrog runs are headless and
|
||||
* short — typical outputs are 1-3K tokens — so we cap at 5_000. This
|
||||
* drastically reduces the upfront budget reservation OpenRouter requires per
|
||||
* call (~$0.38 vs ~$2.40 for Opus), which is what lets low-wallet runs
|
||||
* actually start.
|
||||
*
|
||||
* Plumbed via `OPENCODE_EXPERIMENTAL_OUTPUT_TOKEN_MAX` env var rather than the
|
||||
* config JSON. OpenCode's `OUTPUT_TOKEN_MAX` (session/llm.ts) is sourced
|
||||
* exclusively from this env var; top-level `limit.output` in the config
|
||||
* has no read site and is silently dropped on merge.
|
||||
*/
|
||||
const PULLFROG_OPENCODE_OUTPUT_LIMIT = 5000;
|
||||
|
||||
function buildSecurityConfig(ctx: AgentRunContext, model: string | undefined): string {
|
||||
const config: OpenCodeConfig = {
|
||||
permission: {
|
||||
@@ -352,6 +368,16 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
|
||||
// mismatched callID" signal).
|
||||
const knownNonTaskCallIDs = new Set<string>();
|
||||
|
||||
// a subagent is "in flight" while at least one `task` tool_use is awaiting
|
||||
// its tool_result. used to suspend spawn's activity timer (which only sees
|
||||
// opencode's stdout) so a long-running subagent — whose internal events
|
||||
// don't surface on the parent NDJSON stream — can't trip the 5min timeout.
|
||||
// boolean state instead of a heartbeat so there's no race window between a
|
||||
// periodic tick and a subagent that finishes between ticks.
|
||||
function isSubagentInFlight(): boolean {
|
||||
return taskDispatchByCallID.size > 0 || pendingTaskDispatches.length > 0;
|
||||
}
|
||||
|
||||
function emitSubagentFinished(
|
||||
dispatch: TaskDispatch,
|
||||
status: string,
|
||||
@@ -693,6 +719,20 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
|
||||
activityTimeout: 300_000,
|
||||
onActivityTimeout: params.onActivityTimeout,
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
// node_modules/opencode-ai/bin/opencode is a Node shim that spawnSyncs
|
||||
// the native opencode-<plat>-<arch> binary with stdio:"inherit". without
|
||||
// a process-group kill, SIGKILL hits only the shim, the native binary
|
||||
// is reparented to PID 1, holds our stdout pipe open, and `child.close`
|
||||
// never fires — producing zombie runs. detached + killGroup nukes the
|
||||
// whole tree.
|
||||
killGroup: true,
|
||||
// suspend the inner activity timer while a `task` subagent is in flight.
|
||||
// opencode's task tool encapsulates subagent execution in-process — the
|
||||
// subagent's internal events don't surface on the parent NDJSON stream,
|
||||
// so without this the 5min timeout would falsely fire mid-subagent.
|
||||
// suspend/resume is preferable to a heartbeat because there's no race
|
||||
// between a periodic tick and a subagent finishing between ticks.
|
||||
isPausedExternally: isSubagentInFlight,
|
||||
onStdout: async (chunk) => {
|
||||
const text = chunk.toString();
|
||||
output += text;
|
||||
@@ -929,6 +969,7 @@ export const opencode = agent({
|
||||
...homeEnv,
|
||||
OPENCODE_CONFIG_CONTENT: buildSecurityConfig(ctx, model),
|
||||
OPENCODE_PERMISSION: permissionOverride,
|
||||
OPENCODE_EXPERIMENTAL_OUTPUT_TOKEN_MAX: PULLFROG_OPENCODE_OUTPUT_LIMIT.toString(),
|
||||
GOOGLE_GENERATIVE_AI_API_KEY:
|
||||
process.env.GOOGLE_GENERATIVE_AI_API_KEY || process.env.GEMINI_API_KEY,
|
||||
};
|
||||
|
||||
@@ -35,6 +35,7 @@ import { executeLifecycleHook } from "./utils/lifecycle.ts";
|
||||
import { normalizeEnv } from "./utils/normalizeEnv.ts";
|
||||
import { aggregateUsage, patchWorkflowRunFields } from "./utils/patchWorkflowRunFields.ts";
|
||||
import { resolvePayload, resolvePromptInput } from "./utils/payload.ts";
|
||||
import { isRouterKeylimitExhaustedError } from "./utils/providerErrors.ts";
|
||||
import { readSummaryFile, seedSummaryFile } from "./utils/prSummary.ts";
|
||||
import { postReviewCleanup } from "./utils/reviewCleanup.ts";
|
||||
import { handleAgentResult } from "./utils/run.ts";
|
||||
@@ -186,6 +187,14 @@ function billingConsoleUrl(owner: string, anchor: "billing" | "model-access"): s
|
||||
* - `router_requires_card`: user is on Router mode with no card AND no
|
||||
* wallet balance. Lead with the carrot ($20 free credit), link to
|
||||
* `#model-access` where the Add Card flow lives.
|
||||
* - `router_balance_exhausted`: user has a card on file but auto-reload is
|
||||
* disabled and they've spent past their $5 overdraft buffer. Frame as
|
||||
* "balance ran out" and surface both remediation paths (top up, or flip
|
||||
* on auto-reload).
|
||||
* - `router_keylimit_exhausted`: OpenRouter rejected mid-run because the
|
||||
* per-run key budget was exhausted while the agent was working. The
|
||||
* wallet is now negative; same remediation as `router_balance_exhausted`
|
||||
* but framed for the after-the-fact case ("this run was cut short").
|
||||
* - `needsReauthentication`: issuer requires 3DS on every off-session
|
||||
* charge. Re-adding the card won't help — the only escape is a manual
|
||||
* top-up where 3DS runs interactively in Stripe Checkout.
|
||||
@@ -205,6 +214,26 @@ function formatBillingErrorSummary(error: BillingError, owner: string): string {
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
if (error.code === "router_balance_exhausted") {
|
||||
return [
|
||||
"**Your Pullfrog Router balance is exhausted.**",
|
||||
"",
|
||||
"You have a card on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer.",
|
||||
"",
|
||||
`[Top up balance →](${billingConsoleUrl(owner, "billing")}) · [Enable auto-reload →](${billingConsoleUrl(owner, "model-access")})`,
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
if (error.code === "router_keylimit_exhausted") {
|
||||
return [
|
||||
"**This run was cut short — your Pullfrog Router balance ran out mid-run.**",
|
||||
"",
|
||||
"OpenRouter stopped the agent because the per-run budget was exhausted. Your wallet is now negative; top up or enable auto-reload to keep runs flowing.",
|
||||
"",
|
||||
`[Top up balance →](${billingConsoleUrl(owner, "billing")}) · [Enable auto-reload →](${billingConsoleUrl(owner, "model-access")})`,
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
if (error.needsReauthentication) {
|
||||
const code = error.declineCode ?? "authentication_required";
|
||||
return [
|
||||
@@ -549,6 +578,12 @@ export async function main(): Promise<MainResult> {
|
||||
const resolvedModel = payload.proxyModel ? undefined : resolveModel({ slug: payload.model });
|
||||
const agent = resolveAgent({ model: resolvedModel });
|
||||
|
||||
// surface the effective model in comment/review footers. payload.model is
|
||||
// just the stored slug (often undefined for router/oss runs that derive
|
||||
// the target from proxyModel). matching priority with resolveModelForLog
|
||||
// so the "Using `…`" badge reflects what actually ran.
|
||||
toolState.model = payload.proxyModel ?? resolvedModel ?? payload.model;
|
||||
|
||||
validateAgentApiKey({
|
||||
agent,
|
||||
model: payload.proxyModel ?? resolvedModel ?? payload.model,
|
||||
@@ -887,16 +922,32 @@ export async function main(): Promise<MainResult> {
|
||||
killTrackedChildren();
|
||||
log.error(errorMessage);
|
||||
|
||||
// Reclassify OpenRouter "key budget exhausted" mid-run errors as
|
||||
// BillingError. The agent runtime surfaces this as a generic APIError,
|
||||
// but it's a Pullfrog billing concern — the user's Router wallet ran
|
||||
// out partway through the run. Route through the same formatBillingErrorSummary
|
||||
// path as proxy-token 402s so the user gets actionable copy + a top-up
|
||||
// CTA on both the job summary and the PR progress comment, instead of
|
||||
// a generic "❌ Pullfrog failed" stack-trace dump.
|
||||
const billingError = isRouterKeylimitExhaustedError(errorMessage)
|
||||
? new BillingError(errorMessage, { code: "router_keylimit_exhausted" })
|
||||
: null;
|
||||
|
||||
// best-effort summary — write the error so it's visible in the Actions summary tab
|
||||
try {
|
||||
const errorSummary = `### ❌ Pullfrog failed\n\n\`\`\`\n${errorMessage}\n\`\`\``;
|
||||
const errorSummary = billingError
|
||||
? formatBillingErrorSummary(billingError, runContext.repo.owner)
|
||||
: `### ❌ Pullfrog failed\n\n\`\`\`\n${errorMessage}\n\`\`\``;
|
||||
const usageSummary = formatUsageSummary(toolState.usageEntries);
|
||||
const parts = [errorSummary, toolState.lastProgressBody, usageSummary].filter(Boolean);
|
||||
await writeSummary(parts.join("\n\n"));
|
||||
} catch {}
|
||||
|
||||
try {
|
||||
await reportErrorToComment({ toolState, error: errorMessage });
|
||||
const commentBody = billingError
|
||||
? formatBillingErrorSummary(billingError, runContext.repo.owner)
|
||||
: errorMessage;
|
||||
await reportErrorToComment({ toolState, error: commentBody });
|
||||
} catch {
|
||||
// error reporting failed, but don't let it mask the original error
|
||||
}
|
||||
|
||||
+65
-7
@@ -11,6 +11,7 @@ import {
|
||||
} from "../utils/diffCoverage.ts";
|
||||
import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
|
||||
import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
|
||||
import { retry } from "../utils/retry.ts";
|
||||
import { deleteProgressComment } from "./comment.ts";
|
||||
import type { ToolContext } from "./server.ts";
|
||||
import { execute, tool } from "./shared.ts";
|
||||
@@ -21,6 +22,29 @@ function getHttpStatus(err: unknown): number | undefined {
|
||||
return typeof status === "number" ? status : undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* detect GitHub's generic server-side 422 ("An internal error occurred,
|
||||
* please try again.") that sometimes fires on `POST /pulls/{n}/reviews`.
|
||||
*
|
||||
* the body is stable across occurrences and distinct from every other 422
|
||||
* cause we care about (anchor validation, body length, malformed suggestion
|
||||
* blocks) — those all cite the specific problem. treating this as a
|
||||
* transient server error unlocks bounded in-tool retry instead of surfacing
|
||||
* it to the agent with the generic "likely causes (1)(2)(3)" prompt, which
|
||||
* induces whack-a-mole comment dropping on content that was never the issue.
|
||||
*/
|
||||
export function isTransientReviewError(err: unknown): boolean {
|
||||
if (getHttpStatus(err) !== 422) return false;
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
return /internal error occurred, please try again/i.test(msg);
|
||||
}
|
||||
|
||||
// backoff schedule for transient GitHub 422 "internal error" responses on the
|
||||
// reviews endpoint. 3 attempts total (initial + 2 retries) with 1s/3s delays
|
||||
// — most transient GH errors clear within a few seconds, and longer delays
|
||||
// push review submission past agent-perceived responsiveness.
|
||||
export const TRANSIENT_REVIEW_RETRY_DELAYS_MS = [1_000, 3_000];
|
||||
|
||||
type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number];
|
||||
export type CommentableLines = { RIGHT: Set<number>; LEFT: Set<number> };
|
||||
|
||||
@@ -483,16 +507,50 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
|
||||
|
||||
// no body → single-step createReview (no footer needed)
|
||||
// has body → pending + submit so we can build footer with Fix links using review ID
|
||||
//
|
||||
// wrap the submission in `retry` so GitHub's transient 422 "internal
|
||||
// error" body (distinct from anchor / body-length / suggestion 422s,
|
||||
// which all cite the specific cause) clears on its own instead of
|
||||
// surfacing through the generic 422 handler — that framing sent the
|
||||
// agent dropping valid inline comments chasing a non-issue.
|
||||
// `shouldRetry` scopes retries to the transient body only, so real
|
||||
// validation 422s still fail fast.
|
||||
let result;
|
||||
try {
|
||||
result = body
|
||||
? await createAndSubmitWithFooter(ctx, params, {
|
||||
body,
|
||||
approved: approved ?? false,
|
||||
hasComments: (params.comments?.length ?? 0) > 0,
|
||||
})
|
||||
: await createReviewWithStrandedRecovery(ctx, params);
|
||||
result = await retry(
|
||||
() =>
|
||||
body
|
||||
? createAndSubmitWithFooter(ctx, params, {
|
||||
body,
|
||||
approved: approved ?? false,
|
||||
hasComments: (params.comments?.length ?? 0) > 0,
|
||||
})
|
||||
: createReviewWithStrandedRecovery(ctx, params),
|
||||
{
|
||||
delaysMs: TRANSIENT_REVIEW_RETRY_DELAYS_MS,
|
||||
shouldRetry: isTransientReviewError,
|
||||
label: "review submission",
|
||||
}
|
||||
);
|
||||
} catch (err: unknown) {
|
||||
// GitHub's transient 422 "internal error" is distinct from anchor /
|
||||
// body-length / suggestion validation failures — framing it with the
|
||||
// generic "likely causes (1)(2)(3)" prompt sends the agent dropping
|
||||
// comments that were never the problem. after bounded in-tool retry
|
||||
// we surface a dedicated message that tells the agent to wait-and-
|
||||
// retry or fall back to a body-only review.
|
||||
if (isTransientReviewError(err)) {
|
||||
const rawMsg = err instanceof Error ? err.message : String(err);
|
||||
throw new Error(
|
||||
`GitHub returned a transient 422 "internal error" on the reviews endpoint after ${TRANSIENT_REVIEW_RETRY_DELAYS_MS.length + 1} attempts. ` +
|
||||
`This is a GitHub-side issue, not a problem with your review content. ` +
|
||||
`Do NOT modify or drop inline comments — their content is not the cause. ` +
|
||||
`Wait ~30 seconds and call this tool once more with the SAME arguments. ` +
|
||||
`If it still fails, submit a body-only review (move all inline feedback into \`body\` as text) so nothing is lost. ` +
|
||||
`GitHub said: ${rawMsg}`,
|
||||
{ cause: err }
|
||||
);
|
||||
}
|
||||
if (getHttpStatus(err) !== 422 || !params.comments?.length) throw err;
|
||||
|
||||
const details = params.comments.map((c) => {
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "pullfrog",
|
||||
"version": "0.0.205",
|
||||
"version": "0.1.1",
|
||||
"type": "module",
|
||||
"bin": {
|
||||
"pullfrog": "dist/cli.mjs",
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { resolveDisplayAlias } from "../models.ts";
|
||||
import { modelAliases, resolveDisplayAlias } from "../models.ts";
|
||||
|
||||
export const PULLFROG_DIVIDER = "<!-- PULLFROG_DIVIDER_DO_NOT_REMOVE_PLZ -->";
|
||||
|
||||
@@ -28,7 +28,13 @@ export interface BuildPullfrogFooterParams {
|
||||
function formatModelLabel(slug: string): string {
|
||||
// walk the fallback chain so a deprecated stored slug shows the model the
|
||||
// run actually executed against (e.g. "GPT", not "GPT Codex").
|
||||
const alias = resolveDisplayAlias(slug);
|
||||
const alias =
|
||||
resolveDisplayAlias(slug) ??
|
||||
// reverse-lookup: when the caller passes an effective model (proxy or
|
||||
// resolved target like "openrouter/anthropic/claude-opus-4.7") instead of
|
||||
// a stored alias slug, find the alias whose resolve target matches so we
|
||||
// still render a friendly display name.
|
||||
modelAliases.find((a) => a.resolve === slug || a.openRouterResolve === slug);
|
||||
if (!alias) return `\`${slug}\``;
|
||||
return alias.isFree ? `\`${alias.displayName}\` (free)` : `\`${alias.displayName}\``;
|
||||
}
|
||||
|
||||
@@ -14,7 +14,6 @@ export type WorkflowRunArtifactPatchKey =
|
||||
| "issueNodeId"
|
||||
| "reviewNodeId"
|
||||
| "planCommentNodeId"
|
||||
| "summaryCommentNodeId"
|
||||
| "summarySnapshot";
|
||||
|
||||
/**
|
||||
@@ -38,7 +37,6 @@ const STRING_KEYS: WorkflowRunArtifactPatchKey[] = [
|
||||
"issueNodeId",
|
||||
"reviewNodeId",
|
||||
"planCommentNodeId",
|
||||
"summaryCommentNodeId",
|
||||
"summarySnapshot",
|
||||
];
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { detectProviderError } from "./providerErrors.ts";
|
||||
import { detectProviderError, isRouterKeylimitExhaustedError } from "./providerErrors.ts";
|
||||
|
||||
describe("detectProviderError", () => {
|
||||
describe("false positives previously seen in production", () => {
|
||||
@@ -78,3 +78,50 @@ describe("detectProviderError", () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("isRouterKeylimitExhaustedError", () => {
|
||||
it("matches the canonical OpenRouter mid-run error", () => {
|
||||
expect(
|
||||
isRouterKeylimitExhaustedError(
|
||||
"APIError: This request requires more credits, or fewer max_tokens. " +
|
||||
"You requested up to 32000 tokens, but can only afford 22800. " +
|
||||
"To increase, visit https://openrouter.ai/settings/keys and create a key with a higher total limit"
|
||||
)
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("matches the 'requires more credits' phrasing on its own", () => {
|
||||
expect(
|
||||
isRouterKeylimitExhaustedError("This request requires more credits, or fewer max_tokens.")
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("matches the 'requested up to ... can only afford' phrasing on its own", () => {
|
||||
expect(
|
||||
isRouterKeylimitExhaustedError("You requested up to 8000 tokens but can only afford 1234")
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("does not match generic out-of-credit text", () => {
|
||||
expect(isRouterKeylimitExhaustedError("Your account has insufficient credits")).toBe(false);
|
||||
expect(isRouterKeylimitExhaustedError("rate_limit_exceeded")).toBe(false);
|
||||
expect(isRouterKeylimitExhaustedError('{"limit": 0}')).toBe(false);
|
||||
});
|
||||
|
||||
it("does not match unrelated mentions of max_tokens", () => {
|
||||
expect(isRouterKeylimitExhaustedError("max_tokens parameter must be a positive integer")).toBe(
|
||||
false
|
||||
);
|
||||
});
|
||||
|
||||
it("matches across newlines (defends against upstream wrapping/reformatting)", () => {
|
||||
expect(
|
||||
isRouterKeylimitExhaustedError(
|
||||
"APIError: This request requires more credits, or\nfewer max_tokens. You requested up to 32000 tokens"
|
||||
)
|
||||
).toBe(true);
|
||||
expect(
|
||||
isRouterKeylimitExhaustedError("You requested up to 32000 tokens,\nbut can only afford 22800")
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -36,3 +36,29 @@ export function detectProviderError(text: string): string | null {
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* OpenRouter's response when the per-run key's remaining budget can't cover
|
||||
* the agent's `max_tokens` reservation. Distinct from a generic provider error
|
||||
* because it's a Pullfrog billing concern, not an upstream outage — the user's
|
||||
* Router wallet ran out (or the key budget was undersized at mint time and the
|
||||
* agent ran out of headroom partway through).
|
||||
*
|
||||
* Match must be specific to this exact OpenRouter error class. Generic "credits"
|
||||
* or "limit" text shows up in unrelated errors and would mis-classify them.
|
||||
*
|
||||
* Sample:
|
||||
* `APIError: This request requires more credits, or fewer max_tokens.
|
||||
* You requested up to 32000 tokens, but can only afford 22800.`
|
||||
*/
|
||||
// `/s` (dotAll) lets `.*?` cross newlines so we still detect the error if any
|
||||
// upstream layer reformats the message onto multiple lines. Without it, a
|
||||
// single inserted `\n` would silently bypass the BillingError reclassification
|
||||
// and the user would see the generic `❌ Pullfrog failed` dump instead of the
|
||||
// actionable top-up CTA.
|
||||
const ROUTER_KEYLIMIT_EXHAUSTED_PATTERN =
|
||||
/requires more credits.*?fewer max_tokens|requested up to \d+ tokens.*?can only afford/is;
|
||||
|
||||
export function isRouterKeylimitExhaustedError(text: string): boolean {
|
||||
return ROUTER_KEYLIMIT_EXHAUSTED_PATTERN.test(text);
|
||||
}
|
||||
|
||||
+14
-3
@@ -4,6 +4,12 @@ import { log } from "./cli.ts";
|
||||
export type RetryOptions = {
|
||||
maxAttempts?: number;
|
||||
delayMs?: number;
|
||||
/**
|
||||
* explicit delay schedule — one entry per retry (length N ⇒ N+1 attempts).
|
||||
* when set, overrides `maxAttempts` and `delayMs`. e.g. `[1_000, 3_000]`
|
||||
* means up to 3 attempts, sleeping 1s before retry 2 and 3s before retry 3.
|
||||
*/
|
||||
delaysMs?: readonly number[];
|
||||
shouldRetry?: (error: unknown) => boolean;
|
||||
label?: string;
|
||||
};
|
||||
@@ -20,10 +26,15 @@ const defaultShouldRetry = (error: unknown): boolean => {
|
||||
};
|
||||
|
||||
export async function retry<T>(fn: () => Promise<T>, options: RetryOptions = {}): Promise<T> {
|
||||
const maxAttempts = options.maxAttempts ?? 3;
|
||||
const delayMs = options.delayMs ?? 1000;
|
||||
const shouldRetry = options.shouldRetry ?? defaultShouldRetry;
|
||||
const label = options.label ?? "operation";
|
||||
const delays = options.delaysMs
|
||||
? Array.from(options.delaysMs)
|
||||
: Array.from(
|
||||
{ length: (options.maxAttempts ?? 3) - 1 },
|
||||
(_, i) => (options.delayMs ?? 1000) * (i + 1)
|
||||
);
|
||||
const maxAttempts = delays.length + 1;
|
||||
|
||||
let lastError: unknown;
|
||||
|
||||
@@ -37,7 +48,7 @@ export async function retry<T>(fn: () => Promise<T>, options: RetryOptions = {})
|
||||
throw error;
|
||||
}
|
||||
|
||||
const delay = delayMs * attempt;
|
||||
const delay = delays[attempt - 1]!;
|
||||
log.info(`» ${label} failed (attempt ${attempt}/${maxAttempts}), retrying in ${delay}ms...`);
|
||||
await sleep(delay);
|
||||
}
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { performance } from "node:perf_hooks";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { spawn } from "./subprocess.ts";
|
||||
|
||||
@@ -48,6 +49,79 @@ describe("spawn error path", () => {
|
||||
expect(afterHandles).toBeLessThanOrEqual(beforeHandles);
|
||||
});
|
||||
|
||||
it("killGroup: true propagates SIGKILL to grandchildren so close fires promptly", async () => {
|
||||
// regression: node_modules/opencode-ai/bin/opencode is a Node shim that
|
||||
// spawnSyncs the native binary with stdio:"inherit". without killGroup,
|
||||
// child.kill("SIGKILL") hit only the shim — the native binary was
|
||||
// reparented to PID 1, kept holding our stdout pipe via the inherited
|
||||
// fds, and `child.on("close")` never fired (because pipes stayed open).
|
||||
// a 5-min outer safety-net timer eventually rejected the agent promise,
|
||||
// but the grandchild kept running until the GitHub Actions job-level
|
||||
// timeout. this test replicates the shape with bash + a backgrounded
|
||||
// sleep grandchild: with killGroup, close fires promptly after SIGKILL;
|
||||
// without it, the parent would wait for sleep to exit (30s).
|
||||
//
|
||||
// the activity-check interval is fixed at 5s so the earliest the kill
|
||||
// can fire is ~5s after start. budget 15s end-to-end.
|
||||
const before = performance.now();
|
||||
const result = await spawn({
|
||||
cmd: "bash",
|
||||
args: ["-c", "sleep 30 & wait"],
|
||||
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
||||
activityTimeout: 1000,
|
||||
killGroup: true,
|
||||
}).catch((err) => err);
|
||||
const elapsed = performance.now() - before;
|
||||
|
||||
expect(result).toBeInstanceOf(Error);
|
||||
// 10s ceiling: 5s activity-check tick + signal delivery. a regression
|
||||
// here (no killGroup) would hang for the full 30s sleep.
|
||||
expect(elapsed).toBeLessThan(10_000);
|
||||
}, 20_000);
|
||||
|
||||
it("isPausedExternally suspends the activity timer while it returns true", async () => {
|
||||
// opencode's `task` tool encapsulates subagent execution in-process —
|
||||
// subagent-internal events don't reach our parent NDJSON stream. this
|
||||
// means the child's stdout falls silent for the full subagent duration
|
||||
// even when real work is happening. opencode passes a predicate keyed
|
||||
// off its in-flight task dispatch tracker; while it returns true, the
|
||||
// activity timer must skip the kill decision and reset its baseline.
|
||||
const result = await spawn({
|
||||
cmd: "bash",
|
||||
args: ["-c", "sleep 8; echo done"],
|
||||
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
||||
activityTimeout: 1000,
|
||||
isPausedExternally: () => true,
|
||||
});
|
||||
|
||||
expect(result.exitCode).toBe(0);
|
||||
expect(result.stdout).toContain("done");
|
||||
}, 20_000);
|
||||
|
||||
it("isPausedExternally fires normally once it returns false again", async () => {
|
||||
// verifies suspend/resume rather than unconditional bypass: the predicate
|
||||
// flips to false after a few hundred ms, so the timer should resume from
|
||||
// a fresh baseline (advanced during the paused window) and fire after
|
||||
// activityTimeout idle time. critical that lastActivityTime is reset
|
||||
// while paused — otherwise the resume tick would see a stale baseline
|
||||
// and kill instantly.
|
||||
let paused = true;
|
||||
setTimeout(() => {
|
||||
paused = false;
|
||||
}, 500);
|
||||
|
||||
const result = await spawn({
|
||||
cmd: "sleep",
|
||||
args: ["30"],
|
||||
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
||||
activityTimeout: 1000,
|
||||
isPausedExternally: () => paused,
|
||||
}).catch((err) => err);
|
||||
|
||||
expect(result).toBeInstanceOf(Error);
|
||||
expect(String(result)).toMatch(/activity timeout/i);
|
||||
}, 20_000);
|
||||
|
||||
it("reports signal-killed subprocesses as failures, not success", async () => {
|
||||
// regression: before the fix, `child.on("close", (exitCode) => ...)`
|
||||
// discarded the signal parameter and `exitCode || 0` coerced the
|
||||
|
||||
+54
-5
@@ -106,6 +106,25 @@ export interface SpawnOptions {
|
||||
stdio?: ("pipe" | "ignore" | "inherit")[];
|
||||
onStdout?: (chunk: string) => void;
|
||||
onStderr?: (chunk: string) => void;
|
||||
// when true, spawn the child detached (its own process group) and route all
|
||||
// kill paths (timeout, activity timeout, ctrl-c) through `process.kill(-pid, ...)`
|
||||
// so signals reach grandchildren too. critical for binaries that fork through
|
||||
// a shim (e.g. node_modules/opencode-ai/bin/opencode is a Node shim that
|
||||
// spawnSync's the native binary; without killGroup, SIGKILL only hits the
|
||||
// shim and the native binary is reparented to PID 1, holds our stdout pipe
|
||||
// open, keeps emitting NDJSON, and `child.on("close")` never fires —
|
||||
// producing zombie runs that hang until the GitHub Actions job timeout).
|
||||
killGroup?: boolean;
|
||||
// optional pause predicate consulted on every activity check. when it
|
||||
// returns true the activity timer skips the kill decision *and* resets
|
||||
// its idle baseline so a clean unpause can't immediately fire on a stale
|
||||
// lastActivityTime. opencode uses this because its `task` tool encapsulates
|
||||
// subagent execution in-process — subagent-internal events don't surface
|
||||
// on the parent's NDJSON stream, so the local stdout-only signal would
|
||||
// falsely fire mid-subagent. preferred over fake-activity heartbeats
|
||||
// because there's no race window between a heartbeat tick and a subagent
|
||||
// that finishes between ticks.
|
||||
isPausedExternally?: () => boolean;
|
||||
}
|
||||
|
||||
export interface SpawnResult {
|
||||
@@ -127,6 +146,8 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
||||
let stdoutBuffer = "";
|
||||
let stderrBuffer = "";
|
||||
|
||||
const killGroup = options.killGroup ?? false;
|
||||
|
||||
return new Promise((resolve, reject) => {
|
||||
// security: caller must provide complete env object, not merged with process.env
|
||||
const child = nodeSpawn(options.cmd, options.args, {
|
||||
@@ -136,10 +157,28 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
||||
},
|
||||
stdio: options.stdio || ["pipe", "pipe", "pipe"],
|
||||
cwd: options.cwd || process.cwd(),
|
||||
detached: killGroup,
|
||||
});
|
||||
|
||||
// sends `signal` to the entire process group when killGroup is set, so
|
||||
// grandchildren (e.g. the native opencode binary spawned by the
|
||||
// opencode-ai Node shim) die with the parent. falls back to a direct
|
||||
// child kill if the process-group send fails (common when the child
|
||||
// already exited or was never made a process group leader).
|
||||
const killSelf = (signal: NodeJS.Signals): void => {
|
||||
if (killGroup && child.pid) {
|
||||
try {
|
||||
process.kill(-child.pid, signal);
|
||||
return;
|
||||
} catch {
|
||||
// fall through to direct kill
|
||||
}
|
||||
}
|
||||
child.kill(signal);
|
||||
};
|
||||
|
||||
// track child for cleanup on Ctrl+C
|
||||
trackChild({ child });
|
||||
trackChild({ child, killGroup });
|
||||
|
||||
let timeoutId: NodeJS.Timeout | undefined;
|
||||
let sigkillEscalatorId: NodeJS.Timeout | undefined;
|
||||
@@ -157,7 +196,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
||||
if (options.timeout) {
|
||||
timeoutId = setTimeout(() => {
|
||||
isTimedOut = true;
|
||||
child.kill("SIGTERM");
|
||||
killSelf("SIGTERM");
|
||||
|
||||
// track the escalator so a graceful SIGTERM response (close fires
|
||||
// before the 5s elapses) can clear it. without capture, this timer
|
||||
@@ -165,7 +204,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
||||
// past a timed-out subprocess's clean exit.
|
||||
sigkillEscalatorId = setTimeout(() => {
|
||||
if (!child.killed) {
|
||||
child.kill("SIGKILL");
|
||||
killSelf("SIGKILL");
|
||||
}
|
||||
}, 5000);
|
||||
}, options.timeout);
|
||||
@@ -177,6 +216,16 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
||||
`spawn activity timer: pid=${child.pid} cmd=${options.cmd} timeout=${activityTimeoutMs}ms`
|
||||
);
|
||||
activityCheckIntervalId = setInterval(() => {
|
||||
// when an external pause predicate says we're suspended (e.g.
|
||||
// opencode in a long-running task subagent whose internal events
|
||||
// don't surface on stdout), advance lastActivityTime to "now" so a
|
||||
// clean unpause doesn't immediately fire on a stale baseline, and
|
||||
// skip the kill decision for this tick.
|
||||
if (options.isPausedExternally?.()) {
|
||||
lastActivityTime = performance.now();
|
||||
log.debug(`spawn activity check: pid=${child.pid} paused externally`);
|
||||
return;
|
||||
}
|
||||
const idleMs = performance.now() - lastActivityTime;
|
||||
log.debug(
|
||||
`spawn activity check: pid=${child.pid} idle=${Math.round(idleMs)}ms / ${activityTimeoutMs}ms`
|
||||
@@ -186,9 +235,9 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
||||
killedAtIdleMs = idleMs;
|
||||
const idleSec = Math.round(idleMs / 1000);
|
||||
log.info(
|
||||
`no output for ${idleSec}s from pid=${child.pid} (${options.cmd}), killing process`
|
||||
`no output for ${idleSec}s from pid=${child.pid} (${options.cmd}), killing process${killGroup ? " group" : ""}`
|
||||
);
|
||||
child.kill("SIGKILL");
|
||||
killSelf("SIGKILL");
|
||||
clearInterval(activityCheckIntervalId);
|
||||
try {
|
||||
options.onActivityTimeout?.();
|
||||
|
||||
Reference in New Issue
Block a user