fix(codeflow-store): terminal-sessions idle purge, grace period, and SIGKILL escalation - #41
Conversation
…LL escalation Apply review feedback from PR #30: - Fix A (Gemini CRITICAL): purgeIdleSessions no longer drops a running idle session from the map. Instead, the session is SIGTERMed (with SIGKILL escalation) and its lastActivityAt is refreshed; the existing close handler transitions the entry to "exited", and a future purge cycle deletes it after EXPIRED_OUTPUT_GRACE_MS. This preserves the 60 s grace period semantics for callers holding the snapshot. - Fix C (Qodo): SIGTERM is now paired with a 5 s SIGKILL escalation timer (killWithEscalation helper). The timer is unref'd so it does not keep the event loop alive. closeTerminalSession waits for the close event before deleting the map entry, so a SIGTERM in flight cannot leave a leaked session pointer. - Fix B (Qodo): a module-level setInterval (30 s) sweeps the session map, so idle sessions are reaped even when no inbound API call triggers purgeIdleSessions. The interval is unref'd and gated on a lazy "first call" guard so test re-imports cannot double-start it. - Fix D (Copilot): writeTerminalInput calls purgeIdleSessions at the top so a client cannot POST input to a long-idle session and revive it. Defense in depth alongside the background scheduler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a vitest suite for the four fix areas from PR #30 review: - Fix A: purgeIdleSessions does not remove a running session from the map; its lastActivityAt is refreshed; the close handler transitions status to "exited" and a future purge cycle deletes the entry after EXPIRED_OUTPUT_GRACE_MS. - Fix B: a module-level scheduler is started by the first createTerminalSession call and not double-started on subsequent creates. - Fix C: closeTerminalSession sends SIGTERM and schedules a SIGKILL escalation timer; the map entry is removed only after the close event. - Fix D: writeTerminalInput calls purgeIdleSessions and rejects writes against a long-idle (now exited) session. Tests use real child processes (the shell is the system of record) and manipulate the system clock with vi.setSystemTime to drive IDLE_TIMEOUT_MS and EXPIRED_OUTPUT_GRACE_MS cutoffs without waiting real-world time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The monorepo root (packages/Codeflow_master) installs react and
react-dom at ^19.0.0, but the canvas package pinned ^18.0.0 in
both dependencies and peerDependencies. pnpm reported these as
unmet-peer conflicts on every install.
- Move `react` and `react-dom` out of `dependencies` (where they
shadowed the consumer's React) and into `devDependencies` (where
the package's own build, type-check, and tests need them).
- Widen `peerDependencies` to `^18 || ^19` so consumers on either
React major can install.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 23 minutes and 14 seconds. Learn how PR review limits work. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 updates React peer dependencies in codeflow-canvas and implements a robust terminal session idle purging and SIGKILL escalation mechanism in codeflow-store, complete with comprehensive unit tests. The feedback suggests several improvements to the session lifecycle management: checking the return value of child.kill() to prevent scheduling unnecessary escalation timers, skipping already-killed sessions during the purge sweep to avoid redundant signals, and ensuring the escalation timer is explicitly cleared when a session is manually closed.
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.
| try { | ||
| session.child.kill(signal); | ||
| } catch { | ||
| // Process may have exited between our status check and the kill call; | ||
| // the close handler will run on its own and clean up state. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The child.kill() method in Node.js returns a boolean indicating whether the signal was successfully delivered. If the process has already exited, it returns false without throwing an error. Checking this return value allows us to avoid scheduling an unnecessary setTimeout escalation timer when signal delivery fails.
try {
const sent = session.child.kill(signal);
if (!sent) {
return null;
}
} catch {
// Process may have exited between our status check and the kill call;
// the close handler will run on its own and clean up state.
return null;
}| for (const [id, session] of sessions) { | ||
| if (session.status !== "running") { | ||
| // Exited or error sessions: only delete after the grace period has elapsed. | ||
| const lastActivity = Date.parse(session.lastActivityAt); | ||
| if (Number.isFinite(lastActivity) && lastActivity <= expiredCutoff) { | ||
| sessions.delete(id); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| // Running session: refresh activity before deciding it's idle. | ||
| const lastActivity = Date.parse(session.lastActivityAt); | ||
| if (!Number.isFinite(lastActivity) || lastActivity > idleCutoff) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
If a session is already in the process of being terminated (i.e., session.child.killed is true), we should skip it during the idle purge sweep to avoid sending redundant signals or scheduling duplicate escalation timers.
| for (const [id, session] of sessions) { | |
| if (session.status !== "running") { | |
| // Exited or error sessions: only delete after the grace period has elapsed. | |
| const lastActivity = Date.parse(session.lastActivityAt); | |
| if (Number.isFinite(lastActivity) && lastActivity <= expiredCutoff) { | |
| sessions.delete(id); | |
| } | |
| continue; | |
| } | |
| // Running session: refresh activity before deciding it's idle. | |
| const lastActivity = Date.parse(session.lastActivityAt); | |
| if (!Number.isFinite(lastActivity) || lastActivity > idleCutoff) { | |
| continue; | |
| } | |
| for (const [id, session] of sessions) { | |
| if (session.status !== "running") { | |
| // Exited or error sessions: only delete after the grace period has elapsed. | |
| const lastActivity = Date.parse(session.lastActivityAt); | |
| if (Number.isFinite(lastActivity) && lastActivity <= expiredCutoff) { | |
| sessions.delete(id); | |
| } | |
| continue; | |
| } | |
| if (session.child.killed) { | |
| continue; | |
| } | |
| // Running session: refresh activity before deciding it's idle. | |
| const lastActivity = Date.parse(session.lastActivityAt); | |
| if (!Number.isFinite(lastActivity) || lastActivity > idleCutoff) { | |
| continue; | |
| } |
| if (escalation !== null) { | ||
| session.child.once("close", () => { | ||
| sessions.delete(id); | ||
| }); |
There was a problem hiding this comment.
When a terminal session is closed manually, the escalation timer is scheduled but never cleared if the process exits before the timeout. Although the timer is unreferenced, clearing it explicitly prevents unnecessary execution of the timeout callback and avoids leaving active timers in the event loop.
if (escalation !== null) {
session.child.once("close", () => {
clearTimeout(escalation);
sessions.delete(id);
});
}
…SIGKILL escalation (#41) * fix(codeflow-store): terminal-sessions purge, grace period, and SIGKILL escalation Apply review feedback from PR #30: - Fix A (Gemini CRITICAL): purgeIdleSessions no longer drops a running idle session from the map. Instead, the session is SIGTERMed (with SIGKILL escalation) and its lastActivityAt is refreshed; the existing close handler transitions the entry to "exited", and a future purge cycle deletes it after EXPIRED_OUTPUT_GRACE_MS. This preserves the 60 s grace period semantics for callers holding the snapshot. - Fix C (Qodo): SIGTERM is now paired with a 5 s SIGKILL escalation timer (killWithEscalation helper). The timer is unref'd so it does not keep the event loop alive. closeTerminalSession waits for the close event before deleting the map entry, so a SIGTERM in flight cannot leave a leaked session pointer. - Fix B (Qodo): a module-level setInterval (30 s) sweeps the session map, so idle sessions are reaped even when no inbound API call triggers purgeIdleSessions. The interval is unref'd and gated on a lazy "first call" guard so test re-imports cannot double-start it. - Fix D (Copilot): writeTerminalInput calls purgeIdleSessions at the top so a client cannot POST input to a long-idle session and revive it. Defense in depth alongside the background scheduler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(codeflow-store): cover terminal-sessions purge, grace, and SIGKILL Add a vitest suite for the four fix areas from PR #30 review: - Fix A: purgeIdleSessions does not remove a running session from the map; its lastActivityAt is refreshed; the close handler transitions status to "exited" and a future purge cycle deletes the entry after EXPIRED_OUTPUT_GRACE_MS. - Fix B: a module-level scheduler is started by the first createTerminalSession call and not double-started on subsequent creates. - Fix C: closeTerminalSession sends SIGTERM and schedules a SIGKILL escalation timer; the map entry is removed only after the close event. - Fix D: writeTerminalInput calls purgeIdleSessions and rejects writes against a long-idle (now exited) session. Tests use real child processes (the shell is the system of record) and manipulate the system clock with vi.setSystemTime to drive IDLE_TIMEOUT_MS and EXPIRED_OUTPUT_GRACE_MS cutoffs without waiting real-world time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(codeflow-canvas): align React peerDeps with monorepo React 19 root The monorepo root (packages/Codeflow_master) installs react and react-dom at ^19.0.0, but the canvas package pinned ^18.0.0 in both dependencies and peerDependencies. pnpm reported these as unmet-peer conflicts on every install. - Move `react` and `react-dom` out of `dependencies` (where they shadowed the consumer's React) and into `devDependencies` (where the package's own build, type-check, and tests need them). - Widen `peerDependencies` to `^18 || ^19` so consumers on either React major can install. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: nehraa <nehraa@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Applies the review feedback from PR #30 (Gemini, Qodo, Copilot) to
packages/codeflow-store/src/shared/terminal-sessions.tsand fixes the React 18/19 peerDeps conflict inpackages/codeflow-canvas/package.json.The original PR #30 was opened against a
src/lib/server/terminal-sessions.tsthat no longer exists — it was moved topackages/codeflow-store/src/shared/terminal-sessions.tsby the cleanup commita896899. This PR applies the same review fixes to the new location.Changes
packages/codeflow-store/src/shared/terminal-sessions.tsFix A — Gemini CRITICAL (
purgeIdleSessions):lastActivityAtis updated. Thecloseevent handler transitions status to"exited"and refresheslastActivityAt, so the session remains visible to the user for the 60sEXPIRED_OUTPUT_GRACE_MSgrace period before deletion in a future purge."exited"status are deleted (after the grace period).Fix B — Qodo #1 (background scheduler):
ensurePurgeScheduler()registers a 30ssetIntervalthat callspurgeIdleSessions()..unref()-ed so it doesn't block process exit.purgeTimercheck so test re-imports don't double-register.createTerminalSession().Fix C — Qodo #2 (SIGTERM→SIGKILL escalation):
killWithEscalation(child, signal)helper: sends the signal, schedulesSIGKILLafterSIGKILL_TIMEOUT_MS = 5_000,.unref()s the timer, returns the timer for cancellation. Timeout is cleared onclose.purgeIdleSessionsandcloseTerminalSession.Fix D — Copilot (
writeTerminalInput):purgeIdleSessions()at the top so an idle session cannot be revived by a stray write.packages/codeflow-canvas/package.jsonFix E — React 18/19 peerDeps conflict:
reactandreact-domfromdependenciestodevDependencies.peerDependenciesto^18 || ^19so consumers on either major can install.Skipped (out of scope or obsolete)
/api/terminal/sessionsPOST route no longer exists in the new structure. Would be implementing dead code.pnpm-lock.yamlchanges from original PR — not required for these fixes.Test plan
terminal-sessions.test.tspasspnpm -F @abhinav2203/codeflow-store check— 0 errors interminal-sessions.tsandterminal-sessions.test.tspnpm -F @abhinav2203/codeflow-store test— 192/192 tests pass in files that compile onorigin/maincheckpoint/reasoning.test.tsetc.) are due toCannot find package 'zod'onorigin/main(cleanup commit side effect) — unrelated to this PRsleep 60child +killWithEscalationconfirmedclosehandler clears the escalation timer before it fires (race-safety)Code review
Follow-up (not in this PR)
Code quality review surfaced 2 issues that are not part of PR #30's original review thread and are recommended for a separate PR:
shutdownAllTerminalSessions(terminal-sessions.ts:367-375) bypasses the SIGKILL escalation path — directkill("SIGTERM")thensessions.clear()orphans escalation timers and can leak SIGTERM-ignoring children past process exit. Fix: reusekillWithEscalationandunrefthe timer.writeTerminalInputdoes a fullpurgeIdleSessionsper write — O(N) per keystroke. Fix: short-circuit by checking the target session'slastActivityAtfirst.🤖 Generated with Claude Code