Codex harness: add working-set rebuild circuit breaker to stop runaway context loops - #55562
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎯 Great work addressing the codex engine reliability crisis! This circuit breaker implementation is exactly the kind of defense-in-depth mitigation needed for the 49.4% failure rate issue (#55550). What's solid here:
This looks ready for review by the maintainers. The code is well-structured, the tests provide good coverage, and the solution is pragmatic — it won't fix the underlying crash but will prevent the catastrophic token burn (worst case was 5M+ tokens before crash) on future incidents.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Pull request overview
Adds a Codex context-rebuild circuit breaker to limit runaway token consumption and classify terminated runs as infrastructure-incomplete.
Changes:
- Adds configurable working-set thresholds and token-usage monitoring.
- Extends the process runner with runtime guards.
- Adds circuit-breaker and termination tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/codex_harness.cjs |
Implements and integrates the circuit breaker. |
actions/setup/js/codex_harness.test.cjs |
Tests configuration and threshold evaluation. |
actions/setup/js/process_runner.cjs |
Adds runtime-guard polling and termination. |
actions/setup/js/process_runner.test.cjs |
Tests guard-triggered termination. |
actions/setup/js/harness_retry_runner.cjs |
Extends attempt-result metadata. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
| if (!stat || stat.size <= 0) continue; | ||
| const content = fs.readFileSync(candidate, "utf8"); | ||
| if (!content.trim()) continue; | ||
| return calculateWorkingSetFromJSONL(content).workingSet; |
| } | ||
| : undefined, | ||
| }); | ||
| return { ...result, safeOutputsByteOffset }; |
| return { | ||
| enabled, | ||
| maxRebuildFactor: Number.isFinite(maxRebuildFactorRaw) && maxRebuildFactorRaw > 1 ? maxRebuildFactorRaw : DEFAULT_CONTEXT_REBUILD_FACTOR_LIMIT, | ||
| minCumulativeInputTokens: Number.isFinite(minCumulativeInputTokensRaw) && minCumulativeInputTokensRaw > 0 ? Math.floor(minCumulativeInputTokensRaw) : DEFAULT_CONTEXT_REBUILD_MIN_CUMULATIVE_INPUT_TOKENS, |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This guardrail is heading in the right direction, but the token-usage reader is still making a path-ordering assumption that can terminate the wrong run or miss the runaway one entirely.
Blocking theme
The new circuit breaker stops at the first non-empty token-usage.jsonl candidate. If that path contains stale or partial data while a later candidate has the active run's metrics, the harness will classify the run from the wrong dataset. For infrastructure protection logic, that is a correctness bug, not just an observability gap.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 8.08 AIC · ⌖ 6.95 AIC · ⊞ 7K
Comment /review to run again
| return { | ||
| enabled, | ||
| maxRebuildFactor: Number.isFinite(maxRebuildFactorRaw) && maxRebuildFactorRaw > 1 ? maxRebuildFactorRaw : DEFAULT_CONTEXT_REBUILD_FACTOR_LIMIT, | ||
| minCumulativeInputTokens: Number.isFinite(minCumulativeInputTokensRaw) && minCumulativeInputTokensRaw > 0 ? Math.floor(minCumulativeInputTokensRaw) : DEFAULT_CONTEXT_REBUILD_MIN_CUMULATIVE_INPUT_TOKENS, |
There was a problem hiding this comment.
The circuit breaker reads the first existing token-usage.jsonl candidate and stops there, but this repo writes the same metrics to multiple path variants during setup/teardown; if the first file is a stale partial copy while a later path has the current run's data, you'll either miss a runaway loop or trip on old tokens from another phase.
💡 Why this blocks merge
readWorkingSetFromTokenUsage() now returns on the first non-empty file instead of reconciling freshness. That makes correctness depend on path ordering, not on which file actually belongs to the active run. In the failure mode this change is supposed to prevent, stale metrics are worse than no metrics: the harness can terminate a healthy run or ignore the real runaway one.
Prefer selecting the newest file by mtime (or validating a run/session identifier in the JSONL if one exists) before computing the working set, e.g.
const candidates = paths
.filter(p => fs.existsSync(p) && fs.statSync(p).size > 0)
.sort((a, b) => fs.statSync(b).mtimeMs - fs.statSync(a).mtimeMs);
for (const candidate of candidates) {
// read newest valid file first
}That keeps the circuit breaker tied to the current run instead of whichever path happens to be checked first.
There was a problem hiding this comment.
Fixed. readWorkingSetFromTokenUsage now stats all candidates, sorts them by mtimeMs descending, and reads the newest one first, so selection no longer depends on path ordering. Candidates whose parsed measurement_state is "unavailable" are skipped so a stale/malformed file can't mask a valid one. Covered by the new prefers the most recently written token-usage candidate and skips token-usage candidates whose measurements are unavailable tests in codex_harness.test.cjs.
There was a problem hiding this comment.
This one-off circuit breaker does not need a generic process-level guard abstraction; folding the stop logic back into the harness would remove indirection and keep the flow easier to follow. net: -18 lines possible.
Generated by ✂️ Ponytail Reviewer for #55562 · codex · mai10 · 8.02 AIC · ⌖ 1.65 AIC · ⊞ 16.7K
Comment /ponytail to run again
| * @returns {Promise<{exitCode: number, output: string, hasOutput: boolean, durationMs: number, watchdogFired: boolean, runtimeGuardFired: boolean, runtimeGuardReason: string}>} | ||
| */ | ||
| function runProcess({ command, args, attempt, log, logArgs, env, postResultWatchdog, stallWarningIntervalMs }) { | ||
| function runProcess({ command, args, attempt, log, logArgs, env, postResultWatchdog, runtimeGuard, stallWarningIntervalMs }) { |
There was a problem hiding this comment.
actions/setup/js/process_runner.cjs:103: yagni: generic runtimeGuard hook with poll/term config and extra result fields for a single caller. Inline the termination logic in codex_harness.cjs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, and /diagnosing-bugs — requesting changes on a correctness risk and missing test coverage. Positive: the circuit-breaker abstraction is well-isolated and the new runtimeGuard interface in process_runner.cjs is clean and generic.
📋 Key Themes & Highlights
Key Themes
- Blocking I/O in event-loop timer (
readWorkingSetFromTokenUsageuses syncfsinsidesetInterval): at the default 15 s poll interval this is low severity, but it is still a code-smell that conflicts with Node.js conventions and could bite if the poll interval is shortened. - Shared signal-state between two independent timers:
sentSigtermAt/sentSigkillAtare shared betweenpostResultWatchdogTimerandruntimeGuardTimer, creating subtle ordering-dependent behaviour around grace period enforcement. - Missing boundary and null tests for
evaluateContextRebuildCircuitBreaker: exact-threshold andnull/NaN input paths are untested. - Off-by-one in config validation:
maxRebuildFactor > 1silently ignores the value1.
Positive Highlights
- ✅ Clean separation:
evaluateContextRebuildCircuitBreakeris a pure function — easy to unit-test and reason about. - ✅ Env-variable parsing is defensive (NaN / non-finite checks throughout).
- ✅
runtimeGuardinterface inprocess_runneris generic and reusable — good deep-module design. - ✅
runtimeGuardFired/runtimeGuardReasonresult fields make retry-loop classification deterministic. - ✅ Good test for the end-to-end guard-requested termination path in
process_runner.test.cjs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 45 AIC · ⌖ 10.5 AIC · ⊞ 7.6K
Comment /matt to run again
| } | ||
|
|
||
| /** | ||
| * @param {ReturnType<typeof calculateWorkingSetFromJSONL>["workingSet"] | null} workingSet |
There was a problem hiding this comment.
[/codebase-design] readWorkingSetFromTokenUsage uses synchronous fs.existsSync / fs.statSync / fs.readFileSync inside a setInterval callback that fires every 15 s by default. These blocking calls stall the Node.js event loop on each tick, which can delay the postResultWatchdog and log draining.
💡 Suggestion: use async fs
Switch readWorkingSetFromTokenUsage to async and make shouldTerminate return a Promise:
async function readWorkingSetFromTokenUsage(paths = TOKEN_USAGE_PATHS) {
for (const candidate of paths) {
try {
const content = await fs.promises.readFile(candidate, "utf8");
if (!content.trim()) continue;
return calculateWorkingSetFromJSONL(content).workingSet;
} catch {
continue;
}
}
return null;
}The runtime-guard poll loop in process_runner.cjs would then await the result before acting, keeping the event loop free.
@copilot please address this.
There was a problem hiding this comment.
Fixed. readWorkingSetFromTokenUsage is now async and uses fs.promises.stat / fs.promises.readFile; shouldTerminate returns a promise and the runtime-guard poll loop in process_runner.cjs awaits it, with an in-flight flag so slow reads can't overlap across ticks. New test: supports an asynchronous shouldTerminate without overlapping polls in process_runner.test.cjs.
| log(`attempt ${attempt + 1}: post-result watchdog forcing process exit after ${watchdogTermGraceMs}ms grace (SIGKILL)`); | ||
| const source = runtimeGuardFired ? "runtime guard" : "post-result watchdog"; | ||
| log(`attempt ${attempt + 1}: ${source} forcing process exit after ${termGraceMs}ms grace (SIGKILL)`); | ||
| child.kill("SIGKILL"); |
There was a problem hiding this comment.
[/codebase-design] The runtimeGuardTimer shares sentSigtermAt / sentSigkillAt with postResultWatchdogTimer, so if the watchdog has already sent SIGTERM the runtime-guard timer's SIGKILL branch (line ~237) uses runtimeGuardTermGraceMs instead of watchdogTermGraceMs. This is subtle: whichever timer fires second changes the grace period in-flight, potentially cutting short the watchdog's intended grace window.
💡 Suggestion
Track separate sentSigtermAt timestamps for each timer (e.g. watchdogSentSigtermAt / guardSentSigtermAt) so each enforces its own grace period independently. This also removes the implicit coupling between the two signal sources.
@copilot please address this.
There was a problem hiding this comment.
Fixed. The shared sentSigtermAt / sentSigkillAt are now split into watchdogSentSigtermAt / watchdogSentSigkillAt and guardSentSigtermAt / guardSentSigkillAt, so each source enforces its own grace period independently. The runtime guard also escalates via a dedicated setTimeout(termGraceMs) instead of waiting for the next poll tick, and watchdogFired is now derived solely from watchdogSentSigtermAt.
| evaluateContextRebuildCircuitBreaker( | ||
| { | ||
| measurement_state: "measured", | ||
| rebuild_factor: 4.5, |
There was a problem hiding this comment.
[/tdd] The test for evaluateContextRebuildCircuitBreaker only checks the "both exceeded → trip" and "token count below → no trip" cases. Missing coverage:
rebuild_factorexactly at threshold (boundary:factor === maxRebuildFactorshould trip,factor < maxRebuildFactorshould not)workingSetisnullor missing fields (guard should returnterminate: false)- Non-finite / NaN values for
rebuild_factor
💡 Suggested additional cases
it("does not trip when rebuild_factor is exactly below threshold", () => {
const cfg = { maxRebuildFactor: 4, minCumulativeInputTokens: 1000 };
expect(evaluateContextRebuildCircuitBreaker({ rebuild_factor: 3.99, cumulative_input_tokens: 2000 }, cfg).terminate).toBe(false);
});
it("trips when rebuild_factor equals threshold", () => {
const cfg = { maxRebuildFactor: 4, minCumulativeInputTokens: 1000 };
expect(evaluateContextRebuildCircuitBreaker({ rebuild_factor: 4, cumulative_input_tokens: 2000 }, cfg).terminate).toBe(true);
});
it("does not trip when workingSet is null", () => {
expect(evaluateContextRebuildCircuitBreaker(null, { maxRebuildFactor: 4, minCumulativeInputTokens: 1000 }).terminate).toBe(false);
});@copilot please address this.
There was a problem hiding this comment.
Added the suggested cases: does not trip when rebuild_factor is just below the threshold, trips when rebuild_factor is exactly at the threshold, and does not trip for null, empty, or non-finite working sets (covering null, {}, NaN/Infinity rebuild factors, and a NaN cumulative token count).
| const enabled = enabledValue == null || !/^(0|false|off|no)$/i.test(String(enabledValue).trim()); | ||
| const maxRebuildFactorRaw = Number(env.GH_AW_CODEX_MAX_REBUILD_FACTOR); | ||
| const minCumulativeInputTokensRaw = Number(env.GH_AW_CODEX_REBUILD_MIN_CUMULATIVE_INPUT_TOKENS); | ||
| const pollIntervalRaw = Number(env.GH_AW_CODEX_REBUILD_GUARD_POLL_MS); |
There was a problem hiding this comment.
[/diagnosing-bugs] maxRebuildFactor > 1 is the validity check, but 1 itself is rejected and falls back to the default (25). A value of exactly 1 is arguably a valid and very aggressive threshold. Consider using >= 1 or documenting the intended lower bound explicitly.
💡 Context
maxRebuildFactor: Number.isFinite(maxRebuildFactorRaw) && maxRebuildFactorRaw > 1
? maxRebuildFactorRaw
: DEFAULT_CONTEXT_REBUILD_FACTOR_LIMIT,If an operator sets GH_AW_CODEX_MAX_REBUILD_FACTOR=1 intending "trip immediately when any rebuild occurs", they'll silently get 25. A comment or >= 1 would remove ambiguity.
@copilot please address this.
There was a problem hiding this comment.
Changed to >= 1 with a comment explaining that a factor of exactly 1 means "no rebuild at all" and is therefore the most aggressive valid threshold, while anything below 1 is unreachable. Covered by the accepts a rebuild factor threshold of exactly 1 test.
There was a problem hiding this comment.
Circuit breaker review — harden + audit pass
The working-set rebuild circuit breaker is a solid reliability improvement. The core logic is well-structured with clean separation between config resolution, working-set reading, and evaluation. Tests cover the main happy/unhappy paths.
Three issues found, one reliability-impacting:
Blocking
- Dual SIGKILL escalation paths (process_runner.cjs:334): when
runtimeGuardfires, bothpostResultWatchdogTimerandruntimeGuardTimercan independently escalate to SIGKILL. While Node.js single-threading prevents an actual double-kill via thesentSigkillAtguard, the termination grace period applied is determined by whichever timer polls first — non-deterministic behavior for a safety feature. See inline comment.
Non-blocking
-
Silent error swallowing in
readWorkingSetFromTokenUsage(codex_harness.cjs:75): ifcalculateWorkingSetFromJSONLthrows, the circuit breaker silently disables itself for the run. Add diagnostic logging in the catch block. -
Off-by-one at threshold boundary in
evaluateContextRebuildCircuitBreaker(codex_harness.cjs:93):rebuildFactor < maxRebuildFactormeansrebuild_factor === maxRebuildFactordoes not trip the breaker. Clarify whether this is intentional or use<=.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 41.3 AIC · ⌖ 9.57 AIC · ⊞ 6.2K
Comments that could not be inline-anchored
actions/setup/js/process_runner.cjs:334
Dual SIGKILL escalation paths for the runtime guard — redundant and fragile
When runtimeGuard fires SIGTERM and both postResultWatchdogTimer and runtimeGuardTimer are running, the SIGKILL escalation is handled by both timers. The modified watchdog branch (line 334) now selects runtimeGuardTermGraceMs when runtimeGuardFired, and runtimeGuardTimer also independently checks and fires SIGKILL (lines 347–351). Both check sentSigkillAt === 0 before acting, so there is no double-k…
actions/setup/js/codex_harness.cjs:75
readWorkingSetFromTokenUsage silently swallows all errors — consider structured logging
The catch block in readWorkingSetFromTokenUsage uses a bare continue with no logging. If calculateWorkingSetFromJSONL throws an unexpected error (e.g., a JSON parse bug, OOM on a huge file), the circuit breaker silently returns null and is disabled for the rest of the run with no diagnostic trace. Given that this is a safety feature, silent failure is the worst possible mode.
A minimal im…
actions/setup/js/codex_harness.cjs:93
evaluateContextRebuildCircuitBreaker: rebuildFactor < config.maxRebuildFactor returns early — boundary value is not checked
Line 93: rebuildFactor < config.maxRebuildFactor means a rebuild factor exactly equal to maxRebuildFactor does NOT trip the breaker (returns false). The default value is 25, so a rebuild_factor of 25.00 would pass through unchecked while 25.01 trips it. This is likely unintentional — standard circuit breaker semantics use >= (trip at or above the lim…
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32800975910
|
…, and SIGKILL escalation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…dependent grace periods Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Forward-progress pass done. Addressed the blocking review threads:
Replies with details were posted in each thread. Tests: 173 passing across |
|
🎉 This pull request is included in a new release. Release: |
Codex-engine runs were intermittently failing with
driver_exit, including a costly failure mode where context rebuilding spirals and burns large token volume before crash. This change adds a guardrail in the codex harness to terminate runaway rebuild behavior early and classify it as infrastructure-incomplete instead of retrying into further spend.Circuit breaker for context-rebuild runaway
token-usage.jsonl.rebuild_factor >= max_rebuild_factorcumulative_input_tokens >= min_cumulative_input_tokensreport_incomplete, and stops retrying.Configurable policy with safe defaults
GH_AW_CODEX_CONTEXT_REBUILD_CIRCUIT_BREAKER(on/off)GH_AW_CODEX_MAX_REBUILD_FACTOR(default:25)GH_AW_CODEX_REBUILD_MIN_CUMULATIVE_INPUT_TOKENS(default:1000000)GH_AW_CODEX_REBUILD_GUARD_POLL_MSGH_AW_CODEX_REBUILD_GUARD_TERM_GRACE_MSRuntime guard plumbing in process runner
runProcess(...)with a genericruntimeGuardcallback/poll loop.runtimeGuardFired,runtimeGuardReason) so harness retry logic can classify guard-triggered exits deterministically.Token-usage input correctness
gh-aw-pr-sous-chefRun: https://github.com/github/gh-aw/actions/runs/32800975910