fix: claude-lane/task_1780281408931_2vxzzbu7l-20260602125813 - #30
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✨ 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 |
Review Summary by QodoFix terminal sessions memory leak with idle timeout and cleanup
WalkthroughsDescription• Implement session memory leak prevention with idle timeout • Add automatic cleanup of expired terminal sessions • Enforce maximum session limit to prevent resource exhaustion • Purge idle sessions on list and get operations Diagramflowchart LR
A["Terminal Session Created"] --> B["Check Max Sessions Limit"]
B --> C["Session Active"]
C --> D["Idle Timeout Check"]
D --> E["Purge Expired Sessions"]
E --> F["Session Cleanup Complete"]
G["List/Get Sessions"] --> E
File Changes1. src/lib/server/terminal-sessions.ts
|
Code Review by Qodo
1. Idle timeout not guaranteed
|
![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 introduces idle session purging and a maximum session limit of 50 to the terminal session manager. It adds logic to check for idle sessions (10-minute timeout for running sessions and a 1-minute grace period for exited sessions) and purges them during list, get, and create operations. A critical bug was identified in the purging logic where running sessions are immediately deleted from the sessions map upon receiving a SIGTERM signal. This bypasses the intended 60-second grace period for users to view final output and can leak orphaned processes if they do not terminate instantly. The reviewer provided a code suggestion to update the last activity timestamp instead of immediate deletion, allowing the process close event to handle the transition to exited status.
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.
| const purgeIdleSessions = (now: number = Date.now()): void => { | ||
| for (const [id, session] of sessions) { | ||
| if (!isIdleExpired(session, now)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (session.status === "running") { | ||
| session.child.kill("SIGTERM"); | ||
| } | ||
|
|
||
| sessions.delete(id); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Bug: Immediate Deletion of Running Sessions Bypasses Grace Period & Can Leak Processes
When a running session is determined to be idle, purgeIdleSessions calls session.child.kill("SIGTERM") and immediately deletes the session from the sessions map. This introduces two major issues:
- Resource Leak / Orphaned Processes: If the child process does not terminate instantly (or ignores
SIGTERM), we lose the reference to it. It will continue running in the background as an orphaned process and can never be cleaned up or killed during a shutdown (e.g., inshutdownAllTerminalSessions). - Bypassed Grace Period: The user will not be able to see the termination notice or final output of the idle-purged session because it is deleted from the map immediately, completely bypassing the 60-second grace period (
EXPIRED_OUTPUT_GRACE_MS) that exited sessions are supposed to have.
Solution
Instead of deleting the session immediately when it is running, we should send SIGTERM and update its lastActivityAt to the current time. This prevents it from being killed again on subsequent checks. Once the process actually exits, the close event will transition its status to "exited" and update lastActivityAt again, allowing the session to remain visible to the user for the 60-second grace period before being deleted in a future purge.
| const purgeIdleSessions = (now: number = Date.now()): void => { | |
| for (const [id, session] of sessions) { | |
| if (!isIdleExpired(session, now)) { | |
| continue; | |
| } | |
| if (session.status === "running") { | |
| session.child.kill("SIGTERM"); | |
| } | |
| sessions.delete(id); | |
| } | |
| }; | |
| const purgeIdleSessions = (now: number = Date.now()): void => { | |
| for (const [id, session] of sessions) { | |
| if (!isIdleExpired(session, now)) { | |
| continue; | |
| } | |
| if (session.status === "running") { | |
| session.child.kill("SIGTERM"); | |
| // Prevent killing the process repeatedly before it closes, and allow | |
| // the "close" event to transition it to "exited" for the grace period. | |
| session.lastActivityAt = new Date().toISOString(); | |
| } else { | |
| sessions.delete(id); | |
| } | |
| } | |
| }; |
![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: 57054845dc
ℹ️ 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".
| if (session.status === "running") { | ||
| return now - lastActivity > IDLE_TIMEOUT_MS; | ||
| } |
There was a problem hiding this comment.
For running sessions, this treats the absence of input/output for 10 minutes as expiration, and purgeIdleSessions() then sends SIGTERM before any GET/list/create response. A user running a legitimate quiet command such as sleep 900, a long build step with no output, or a paused debugger will have the process killed and the session removed even though it is still active; terminal reads do not refresh lastActivityAt, so simply keeping the terminal open does not prevent this.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds lifecycle controls around server-side terminal sessions to reduce resource leaks, and updates the pnpm lockfile to reflect additional workspace package dependency graph changes.
Changes:
- Add idle session expiry + purge logic and a hard cap on the number of concurrent terminal sessions.
- Invoke idle-session purging on terminal session list/get/create operations.
- Update
pnpm-lock.yamlwith new/updated workspace importer dependency resolutions (including new package entries and resolved versions).
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/lib/server/terminal-sessions.ts |
Introduces idle timeout purging and a max-session guard for in-memory terminal sessions. |
pnpm-lock.yaml |
Updates pnpm lock state for workspace packages and their resolved dependency versions. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const listTerminalSessions = (): TerminalSessionSummary[] => { | ||
| purgeIdleSessions(); | ||
| return [...sessions.values()] | ||
| .map(toSummary) | ||
| .sort((left, right) => right.startedAt.localeCompare(left.startedAt)); |
| purgeIdleSessions(); | ||
| if (sessions.size >= MAX_SESSIONS) { | ||
| throw new Error( | ||
| `Terminal session limit reached (${MAX_SESSIONS}). Close an existing session before opening a new one.` | ||
| ); |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| export const listTerminalSessions = (): TerminalSessionSummary[] => { | ||
| purgeIdleSessions(); | ||
| return [...sessions.values()] | ||
| .map(toSummary) | ||
| .sort((left, right) => right.startedAt.localeCompare(left.startedAt)); | ||
| }; | ||
|
|
||
| export const getTerminalSession = (id: string): TerminalSessionSnapshot | null => { | ||
| purgeIdleSessions(); | ||
| const session = sessions.get(id); | ||
| return session ? toSnapshot(session) : null; | ||
| }; |
There was a problem hiding this comment.
1. Idle timeout not guaranteed 🐞 Bug ☼ Reliability
Idle session eviction only happens when listTerminalSessions/getTerminalSession/createTerminalSession are called, so abandoned sessions can keep their shell processes alive indefinitely if no further API requests occur. This defeats the intent of IDLE_TIMEOUT_MS and can leak OS processes and memory until the server receives another terminal-related request.
Agent Prompt
### Issue description
`purgeIdleSessions()` is only invoked opportunistically (on list/get/create). If a client disconnects and stops calling these endpoints, idle sessions will never be purged and the underlying shell processes can continue running.
### Issue Context
The code defines `IDLE_TIMEOUT_MS`/`EXPIRED_OUTPUT_GRACE_MS`, but there is no background timer or per-session timer to enforce expiry.
### Fix Focus Areas
- src/lib/server/terminal-sessions.ts[34-179]
### Suggested fix
- Add a background `setInterval(() => purgeIdleSessions(), N)` (and `.unref()` if appropriate) so expiry is enforced even without inbound requests.
- Alternatively (often better), attach a per-session timer on creation that is reset whenever `lastActivityAt` changes; on timer fire, kill and cleanup that specific session.
- Add a small vitest to simulate time passing and verify sessions are purged without calling list/get/create.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const purgeIdleSessions = (now: number = Date.now()): void => { | ||
| for (const [id, session] of sessions) { | ||
| if (!isIdleExpired(session, now)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (session.status === "running") { | ||
| session.child.kill("SIGTERM"); | ||
| } | ||
|
|
||
| sessions.delete(id); | ||
| } |
There was a problem hiding this comment.
2. Orphaned terminal processes 🐞 Bug ☼ Reliability
purgeIdleSessions() sends SIGTERM and immediately deletes the session from the map; if the child ignores SIGTERM or takes time to exit, it can keep running with no remaining handle for escalation/cleanup. This can leak long-lived processes and their event listeners/output buffers beyond the session’s lifecycle.
Agent Prompt
### Issue description
Expired sessions are removed from `sessions` immediately after sending SIGTERM. If the process does not exit promptly, it becomes unmanaged and may continue consuming resources.
### Issue Context
`purgeIdleSessions()` and `closeTerminalSession()` both call `child.kill('SIGTERM')` and then delete the session right away.
### Fix Focus Areas
- src/lib/server/terminal-sessions.ts[54-66]
- src/lib/server/terminal-sessions.ts[264-276]
### Suggested fix
- When expiring/closing a running session:
- Mark it as closing (optional), send SIGTERM.
- Wait for the child's `close` event (or a short timeout).
- If it does not close within the timeout, send SIGKILL.
- Only then delete from the map.
- Ensure listeners are removed on cleanup to avoid retaining session objects longer than necessary.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Closing as superseded by #41. The cleanup commit PR #41 opens a fresh branch from Two follow-up issues from the code-quality review are tracked separately (not blocking this PR):
No action needed on this branch. |
…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>
…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>
Automated DevPulse recovery — see commit history.
Recovered by recover_unprd_tasks.py