Repository navigation
refactor: unify serve + daemon — single process owns the stack - #991
Conversation
- Remove GENIE_APP check from db.ts: only GENIE_IS_DAEMON spawns pgserve - autoStartDaemon() now launches `genie serve --headless` instead of `genie daemon start` - Add --headless flag to `genie serve` (services only, no TUI) - `genie daemon start` redirects to `genie serve --headless` with deprecation notice - `genie daemon stop/status/logs` check both serve.pid and scheduler.pid - systemd unit now points to `genie serve start --headless --foreground` - Add service-registry.ts for PID tracking and orphan cleanup - Enhanced shutdown: proper ordering, 10s force-kill timeout, pgserve lockfile cleanup
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
![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 unifies the daemon and serve processes, transitioning genie daemon to an alias for genie serve --headless. It introduces a new service-registry module to track and manage the lifecycle of child processes like pgserve and the scheduler. The review identifies several critical issues in the shutdown implementation: the main process incorrectly registers its own PID, which would cause it to attempt to kill itself during cleanup; the force-kill timeout is rendered ineffective by an immediate process.exit() call; and there is redundant cleanup logic for the database port file that is already handled elsewhere.
| export function killAllServices(): void { | ||
| for (const [_name, entry] of registry) { | ||
| try { | ||
| process.kill(entry.pid, 'SIGTERM'); | ||
| } catch { | ||
| // Already dead — clean up | ||
| registry.delete(_name); | ||
| } | ||
| } | ||
|
|
||
| // Brief grace period for SIGTERM to take effect | ||
| const deadline = Date.now() + 3000; | ||
| const checkInterval = 200; | ||
|
|
||
| const stillAlive = () => { | ||
| for (const [, entry] of registry) { | ||
| try { | ||
| process.kill(entry.pid, 0); | ||
| return true; | ||
| } catch { | ||
| // dead | ||
| } | ||
| } | ||
| return false; | ||
| }; | ||
|
|
||
| // Synchronous busy-wait for up to 3s (shutdown context, blocking is acceptable) | ||
| while (Date.now() < deadline && stillAlive()) { | ||
| const waitUntil = Date.now() + checkInterval; | ||
| while (Date.now() < waitUntil) { | ||
| // busy wait | ||
| } | ||
| } | ||
|
|
||
| // SIGKILL any survivors | ||
| for (const [name, entry] of registry) { | ||
| try { | ||
| process.kill(entry.pid, 0); // check alive | ||
| process.kill(entry.pid, 'SIGKILL'); | ||
| } catch { | ||
| // Already dead | ||
| } | ||
| registry.delete(name); | ||
| } | ||
| } |
There was a problem hiding this comment.
The killAllServices function has a critical flaw: if the current process's PID is registered (as is currently done in serve.ts), it will attempt to SIGKILL itself. This terminates the process immediately, preventing any subsequent cleanup logic (such as removing PID files or closing other handles) from executing. Additionally, the presence of process.pid in the registry causes the stillAlive check to always return true, forcing the full 3-second busy-wait even if all other services have exited. You should explicitly skip process.pid during the kill and liveness check phases.
export function killAllServices(): void {
for (const [name, entry] of registry) {
if (entry.pid === process.pid) continue;
try {
process.kill(entry.pid, 'SIGTERM');
} catch {
// Already dead — clean up
registry.delete(name);
}
}
// Brief grace period for SIGTERM to take effect
const deadline = Date.now() + 3000;
const checkInterval = 200;
const stillAlive = () => {
for (const [, entry] of registry) {
if (entry.pid === process.pid) continue;
try {
process.kill(entry.pid, 0);
return true;
} catch {
// dead
}
}
return false;
};
// Synchronous busy-wait for up to 3s (shutdown context, blocking is acceptable)
while (Date.now() < deadline && stillAlive()) {
const waitUntil = Date.now() + checkInterval;
while (Date.now() < waitUntil) {
// busy wait
}
}
// SIGKILL any survivors
for (const [name, entry] of registry) {
if (entry.pid === process.pid) {
registry.delete(name);
continue;
}
try {
process.kill(entry.pid, 0); // check alive
process.kill(entry.pid, 'SIGKILL');
} catch {
// Already dead
}
registry.delete(name);
}
}References
- It is acceptable to use hardcoded numeric limits (magic numbers) in non-critical fallback logic, especially when they serve as intentional caps to prevent performance issues like excessive I/O.
| const { registerService } = await import('../lib/service-registry.js'); | ||
| // pgserve child PID is internal to db.ts — register our own process | ||
| // as the owner; db.ts exit handler cleans up pgserve child | ||
| registerService('pgserve-owner', process.pid); |
There was a problem hiding this comment.
Registering process.pid in the service registry is incorrect and dangerous. The registry is intended to track child processes for cleanup. Registering the main process PID will cause killAllServices to attempt to kill the current process, which can abort the shutdown sequence prematurely and leave stale PID files or other resources. Since pgserve is already managed by db.ts via its own exit handlers, this registration is redundant and should be removed. The same issue applies to the registration on line 484.
| process.on('SIGTERM', () => { | ||
| forceKillShutdown(); | ||
| process.exit(143); | ||
| }); | ||
| process.on('SIGINT', () => { | ||
| shutdown(); | ||
| forceKillShutdown(); | ||
| process.exit(130); | ||
| }); |
There was a problem hiding this comment.
The signal handlers call process.exit() immediately after forceKillShutdown(). Since forceKillShutdown schedules a 10-second timeout but returns immediately, the process will exit right away, rendering the 10-second force-kill safety net useless. To implement a proper graceful shutdown with a timeout, the signal handler should be async, await the graceful shutdown process, and only then exit. However, given that shutdown() is currently synchronous, the process will exit as soon as shutdown() finishes anyway, making the 10s timeout redundant in its current form.
| try { | ||
| const lockfilePath = join(genieHome(), 'pgserve.port'); | ||
| if (existsSync(lockfilePath)) { | ||
| unlinkSync(lockfilePath); | ||
| } | ||
| } catch { | ||
| // Best effort | ||
| } |
There was a problem hiding this comment.
This manual cleanup of the pgserve.port lockfile is redundant. The db.ts module already manages this lockfile via its own shutdown() and process exit handlers (see src/lib/db.ts line 389). Duplicating this logic here is unnecessary and could lead to race conditions.
![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: 604b26f908
ℹ️ 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".
| // pgserve child PID is internal to db.ts — register our own process | ||
| // as the owner; db.ts exit handler cleans up pgserve child | ||
| registerService('pgserve-owner', process.pid); |
There was a problem hiding this comment.
startForeground() registers process.pid as both pgserve-owner and scheduler, but killAllServices() later sends SIGTERM/SIGKILL to every registered PID. That means the owner process is treated as a child and can be killed mid-shutdown, skipping the remaining cleanup steps (e.g., PID/lockfile removal) whenever this path executes. Register only real child PIDs (or explicitly skip process.pid in the registry kill path) to avoid self-termination during shutdown.
Useful? React with 👍 / 👎.
| if (!pid) { | ||
| pid = readPid(); | ||
| source = 'scheduler'; | ||
| } |
There was a problem hiding this comment.
The fallback to scheduler.pid only runs when serve.pid is missing, not when it exists but points to a dead process. In that stale-serve case, daemon stop returns after cleanup and never attempts to stop a live legacy scheduler process, which breaks the stated backward-compat behavior and can leave the scheduler running orphaned. Resolve the primary PID candidate based on liveness before deciding whether to fall back.
Useful? React with 👍 / 👎.
Summary
genie daemonandgenie serveunder a single process that owns pgserve, scheduler, and all servicesgenie serve --headlessreplacesgenie daemon startas the headless service mode (no TUI)service-registry.ts) for tracking child processes and orphan cleanupChanges
GENIE_APPcheck; onlyGENIE_IS_DAEMONspawns pgserve.autoStartDaemon()now launchesgenie serve --headlessand checks bothserve.pidandscheduler.pid--headlessflag, service registry integration, enhanced shutdown with force-kill timeoutdaemon startredirects togenie serve --headlesswith deprecation notice.stop/status/logscheck both PID files. systemd unit updatedTest plan
bun run buildpassesgenie serve --headlessstarts services without TUIgenie daemon startredirects to serve --headlessgenie daemon stop/statuswork with serve.pid