fix(agent): stop a failed detached run from crashing the process - #4411
Conversation
registerExecution's .finally() derives a new promise that is only ever read by waitForDrain, which runs on shutdown. During normal operation nothing attaches a rejection handler to it, so a failed detached run became an unhandled promise rejection and crashed the process instead of just failing that one run. Surfaced by staging's veryfront-agent pods crash-looping (6 restarts each in 16 minutes) once #4407 let run-scoped inference credentials actually reach requireSecureInferenceApiBaseUrl, which currently rejects staging's plain-HTTP internal VERYFRONT_API_URL. That rejection is a separate, pre-existing policy question; this change only stops it (or any other detached-run failure) from taking the whole process down.
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
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. |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. Code Review ✅ ApprovedStops a failed detached run from crashing the process by adding a rejection handler to the promise derived in OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Important Your trial ends in 4 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
|
Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe detached run tracker now attaches a no-op rejection handler to registered execution promises. A test verifies that rejected executions do not emit ChangesDetached run rejection handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Failed detached runs will remain individual run failures rather than causing unhandled promise rejections that terminate the process. The targeted regression coverage supports merge readiness. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Automated Code ReviewScore: 93/100 — Excellent. Minimal, well-tested fix for a real production crash, with the fix and the unrelated policy question cleanly separated. Strengths
Minor / optional suggestions (non-blocking)
This is a textbook example of a scoped incident-response fix — small diff, clear regression test, and no scope creep into the adjacent (and genuinely separate) TLS/loopback policy decision. Generated by Claude Code |
There was a problem hiding this comment.
🟢 Approval recommended
The change addresses the unhandled rejection root cause with minimal behavioral impact and includes a focused regression test.
Pull request overview
Fixes a production stability issue in the agent detached-run tracker where a failed detached execution could surface as an unhandled promise rejection and crash the process, even though the failure should be isolated to that run.
Changes:
- Attach a rejection handler to the internally tracked execution promise to prevent unhandled rejections during normal operation.
- Add a regression test that installs a real
unhandledrejectionlistener and asserts no event fires for a failing registered execution.
File summaries
| File | Description |
|---|---|
| src/agent/service/detached-run-tracker.ts | Prevent unhandled rejections by attaching a catch handler to the derived tracked promise. |
| src/agent/service/detached-run-tracker.test.ts | Add regression coverage ensuring a failing tracked execution does not emit unhandledrejection. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
globalThis.addEventListener is a Deno/browser-only API; it does not exist under Node, where the CI node-shard job runs this exact test. Replace it with a small helper that uses the WHATWG event API when available and falls back to process.on/process.off (bound to process, since EventEmitter methods rely on their receiver) otherwise. Verified directly under plain Node (v25.9.0) in both directions: the fix observes no unhandled rejection, and a deliberately un-fixed version does.
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Summary
DetachedRunTracker.registerExecution's.finally()derives a new promise (trackedExecution) that's only ever read bywaitForDrain, which only runs during shutdown. During normal operation nothing attaches a rejection handler to it, so a failed detached run became an unhandled promise rejection and crashed the whole process instead of just failing that one run.How this was found
Staging's
veryfront-agentpods were crash-looping — 6 restarts each in 16 minutes — once code#4407 let run-scoped inference credentials actually reachrequireSecureInferenceApiBaseUrlinsrc/provider/veryfront-cloud/shared.tsfor the first time (that check only runs when a run-scoped credential is present, which #4407 fixed the binding for). That check currently rejects staging'sVERYFRONT_API_URL, which is a legitimate internal-cluster address (http://veryfront-api.veryfront-staging.svc.cluster.local) but plain HTTP, not HTTPS or loopback.That URL-policy question is separate and intentionally not addressed by this PR. Whether a trusted internal cluster hostname should be treated as safe for run-scoped credentials is a real security tradeoff, not something to silently loosen as a side effect of a crash fix. This PR only stops the crash: the underlying request will still fail cleanly with a 400 (
CONFIG_INVALID) until that policy question is resolved separately — it just won't take the process down anymore.Verification
unhandledrejectionlistener onglobalThis, negative-controlled: reverted the fix and confirmed the test fails (unhandledcaptures the rejection) before restoring it.detached-run-tracker.test.tssuite: 7/7 passing.deno fmt/deno lint/deno checkclean on both changed files.Summary by CodeRabbit