Force factory daemon exit after graceful stop#273
Conversation
kjgbot
added
the
no-agent-relay-review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds optional ChangesDaemon output flushing on SIGTERM shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for flushing output streams and gracefully exiting the daemon process when stopping the factory command, along with corresponding unit tests. The review feedback highlights two key issues: first, the stream flushing mechanism could hang indefinitely if the output streams are blocked or piped to a terminated process, which can be resolved by introducing a timeout; second, a failure during the flush operation could trigger an unhandled promise rejection, which should be handled with a catch block.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async function flushProcessOutput(): Promise<void> { | ||
| await Promise.all([ | ||
| flushWritable(process.stdout), | ||
| flushWritable(process.stderr), | ||
| ]) | ||
| } | ||
|
|
||
| function flushWritable(stream: NodeJS.WriteStream): Promise<void> { | ||
| if (stream.destroyed || stream.writableEnded || stream.writable === false) { | ||
| return Promise.resolve() | ||
| } | ||
|
|
||
| return new Promise((resolve) => { | ||
| stream.write('', () => resolve()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
If the output streams (process.stdout or process.stderr) are piped to another process that has terminated or stopped reading, writing to them can block indefinitely or the callback might never be invoked. This would cause the daemon to hang indefinitely during shutdown, defeating the purpose of a fast and graceful exit.
To prevent this, we should:
- Introduce a timeout (e.g., 200ms) to the flush operation so that we always proceed to exit even if the streams are clogged.
- Safely catch any synchronous errors during
stream.writeto avoid unhandled promise rejections.
async function flushProcessOutput(): Promise<void> {
let timeoutId: NodeJS.Timeout | undefined
const flushPromise = Promise.all([
flushWritable(process.stdout),
flushWritable(process.stderr),
])
const timeoutPromise = new Promise<void>((resolve) => {
timeoutId = setTimeout(resolve, 200)
})
await Promise.race([flushPromise, timeoutPromise])
if (timeoutId) {
clearTimeout(timeoutId)
}
}
function flushWritable(stream: NodeJS.WriteStream): Promise<void> {
if (stream.destroyed || stream.writableEnded || stream.writable === false) {
return Promise.resolve()
}
return new Promise<void>((resolve) => {
try {
stream.write('', () => resolve())
} catch {
resolve()
}
})
}| const flushAndExit = async (code: number): Promise<void> => { | ||
| try { | ||
| await (deps.flushDaemonOutput ?? flushProcessOutput)() | ||
| } finally { | ||
| const daemonExit = deps.daemonExit ?? ((exitCode: number) => process.exit(exitCode)) | ||
| daemonExit(code) | ||
| waiter.resolve(code) | ||
| } | ||
| } |
There was a problem hiding this comment.
If deps.flushDaemonOutput or flushProcessOutput rejects, the error will propagate out of flushAndExit because there is no catch block. Since flushAndExit is called as a floating promise (void flushAndExit(code)), this will trigger an unhandled promise rejection. This is especially problematic in tests where daemonExit does not terminate the process.
Adding a catch block ensures that any flush errors are safely ignored and do not cause unhandled rejections.
const flushAndExit = async (code: number): Promise<void> => {
try {
await (deps.flushDaemonOutput ?? flushProcessOutput)()
} catch {
// Ignore flush errors to ensure we still exit cleanly
} finally {
const daemonExit = deps.daemonExit ?? ((exitCode: number) => process.exit(exitCode))
daemonExit(code)
waiter.resolve(code)
}
}
|
Implemented the scoped fixes in Addressed comments
Advisory NotesNone. Local validation
I did not run the macOS-only |
1 participant
Summary
Fixes the remaining
fleet factory start --mode liveSIGTERM latency by forcing daemon process exit after graceful factory shutdown has fully completed.Details
factory startdaemon signal path only.factory.stop()resolves first, then daemon output is flushed, then the daemon exit hook callsprocess.exit(code).run-once,loop, andreap-orphanson their existing natural return/drain path.Proof
npx vitest run packages/factory-sdk/src/cli/fleet.test.tsstop->flush->exitrun-onceandreap-orphansdo not invoke daemon flush/exit hooksnpm run typecheck:nodenpx vitest run packages/factory-sdkLive cert handoff
fv2 live cert target: idle daemon SIGTERM should return fast rc0, and the in-flight daemon SIGTERM variant should show pair-tree empty plus fast rc0.