Repository navigation
fix(runtime): keep catalog probes async and release aborted translator budgets - #6796
Conversation
Carry the runtime portion of #6723: cancellable bounded Codex probes, same-selection snapshots, guarded persistence, shared flights and retry cooldown. Release owned translator accounting on request abort without changing prepared transport verdicts or awaiting producer cancellation. Own subprocess abort/deadline handling rather than execFile's built-in observer. Reject the caller promptly while the existing bounded-subprocess helper reaps actual exit, escalating SIGTERM to SIGKILL after the shared 2-second grace. Readiness-synchronized Bun children prove termination even when SIGTERM is ignored; Windows keeps forceful termination and still asserts exit. Keep Windows removal ownership, Rust diagnostics and workflow changes in their separate lanes. Refs #6671; this does not establish the Windows leak is resolved. Co-authored-by: mashfromband <matsumoto.yukuhashi@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 27 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (22)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dcc26366e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void waitForSubprocessExit({ exited, kill: () => child.kill("SIGKILL") }, SUBPROCESS_KILL_GRACE_MS); | ||
| finish(error); |
There was a problem hiding this comment.
When a version probe times out or is aborted, stop() rejects the caller immediately while waitForSubprocessExit continues detached. This lets probeVersionAsync enter its finally and call removeProbeHome() while the Codex child may remain alive for the two-second grace period; on Windows, a child holding files under its temporary CODEX_HOME can make that best-effort removal fail with EBUSY, permanently leaking an ocx-codex-probe-* directory on every stalled refresh. Keep prompt caller rejection if needed, but defer sandbox removal until the reaper confirms actual exit.
Useful? React with 👍 / 👎.
1 participant
Summary
Carries the runtime part of #6723 by @mashfromband so it runs under repository CI. Refs #6671 (Windows memory growth); this does not claim to resolve it.
Two problems, both seen on Windows:
/healthzfor about 20 s in the source PR's measurement. Catalog reads now serve the same selection's last confirmed catalog while an async refresh runs. The refresh is single-flight with a 45 s deadline and bounded backoff, rejects a late result whose input or selection epoch changed, and keeps caller cancellation separate from the shared refresh. Explicit synchronous CLI calls are unchanged, and the observed-catalog effort clamp reads the runtime snapshot instead of spawning.Change beyond the source PR, from review: the async executor no longer relies on
execFile's own abort/timeout. A reaper independent of promise settlement sends SIGTERM, escalates to SIGKILL after 2 s, and waits for exit, so an aborted or timed-out probe cannot leave a child running. The caller still gets a prompt rejection.src/server/responses/core.tsstays at its 210-line cap. The Rust contract crate and the dedicated workflow from #6723 are not part of this PR; this PR carries Bun regressions instead.Verification
tests/adapters/translator-budget-lifetime.test.tsandtests/codex-integration/catalog-async-lifetime.test.ts(registered in both layout tables). On the unchanged source: budget 5 pass / 20 fail, catalog 0 pass / 21 fail. With the change: all pass. The child-reaping cases use READY-synchronized children that ignore SIGTERM and assert SIGKILL and PID exit (about 2.01 s).tests/lab/core-lab-boundary.test.ts, layout): 472 pass / 0 fail.bun run typecheck,bun run privacy:scan,bun run structure:check,git diff --check: pass.9dcc26366e.Checklist
troubleshooting/windows-memory.mdwording.)Co-authored-by: mashfromband matsumoto.yukuhashi@gmail.com