Repository navigation
fix(core): keep running terminals out of eviction - #60073
iyernaveenr wants to merge 1 commit into
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
With this change, a location that has a running terminal is no longer evicted, and it is evicted normally once the terminal is gone.
I ran the reproduction script from #48691 from source on the base and on this branch (1-minute location_idle_timeout, a sleep PTY, no session activity). On the base, the location was evicted after about 2 minutes, the process was killed and GET /api/pty/<id> returned PtyNotFoundError. On this branch, the terminal was still running after 150 s. After it was removed with DELETE, the next sweep evicted the location. location-activity.test.ts and the pty tests pass, and tsc -b for packages/core is clean. The new test fails on the base as intended.
Public API: unchanged.
One thing to watch when you rebase: latest dev rewrote the jobs.runningBackground owner mapping right next to the edited lines (recovery.kind === "subagent" ? [child, parent] : [sessionID]). Please keep that version rather than this branch's kind === "shell" form.
No other concerns: the probe follows the same pattern as the existing idleTimeout config read, and exited-but-not-removed terminals not keeping the location alive matches the issue.
Related: #51828
84e3f22 to
a27857c
Compare
There was a problem hiding this comment.
The new commit is a clean rebase onto current dev with the same change. It keeps dev's recovery.kind === "subagent" owner mapping, so the rebase note from my last review is addressed. I re-ran the #48691 reproduction on the new head: the terminal stays running past the idle timeout, and the location is evicted once the terminal is removed. location-activity.test.ts passes and tsc -b for packages/core is clean.
Public API: unchanged.
No new concerns.
a27857c to
36b9729
Compare
There was a problem hiding this comment.
The rebase onto #59399 is clean. The terminal check now sits next to retained(), and eviction is skipped if either one finds activity.
I re-ran the #48691 reproduction from source:
- On the new base, a running terminal with no client attached is still evicted and killed (
PtyNotFoundError). - With this PR it stays
runningpast the idle timeout, and the location is evicted after the terminal is removed.
location-activity.test.ts passes, and its new case fails on the base. tsc -b for packages/core is clean.
Public API: unchanged.
No new concerns.
36b9729 to
632a4a1
Compare
There was a problem hiding this comment.
The new commit is a clean rebase onto current dev and the change is identical. I re-ran the #48691 reproduction on the new head: a running terminal survives the idle timeout, and the location is evicted after the terminal is removed. location-activity.test.ts passes and tsc -b for packages/core is clean.
Public API: unchanged.
No new concerns.
LocationActivity evicts a Location after the idle timeout unless a running Session or background job owns it. A running terminal counts as neither: terminals emit no session events, so a Location whose only activity is an open terminal is evicted on schedule and the Pty finalizer kills every terminal in it. Attached clients lose their shell and scrollback and reconnect to "PTY session not found". Before evicting an expired Location, ask its cached Pty service for running terminals and refresh the timer while any exist, the same way the sweep already treats active Sessions and background jobs. The probe uses contextEffectOption, so it never boots a Location that is not cached, and a graph that failed to build counts as having none. The test fixture gains a second LocationServiceMap whose graph is the compiled Pty node, and a case that starts a real terminal, lets the deadline pass, removes the terminal and checks the next sweep evicts. Signed-off-by: Naveen R. Iyer <[email protected]>
632a4a1 to
1b6ce3d
Compare
There was a problem hiding this comment.
The new commit is a clean rebase onto current dev and the change is identical. I re-ran the #48691 reproduction on the new head: a running terminal survives the idle timeout, and the location is evicted after the terminal is removed. location-activity.test.ts passes and tsc -b for packages/core is clean.
Public API: unchanged.
No new concerns.
Issue for this PR
Closes #48691
Type of change
What does this PR do?
Supersedes #48730, which was closed by the automated PR cleanup while waiting for review. Complements #59399: that PR keeps a location alive while a terminal socket is attached; this one keeps it alive while a terminal is still running after its client detached (panel closed, tab switched away, app restarted, connection dropped), which is the case in #48691. The two checks compose, and this branch is rebased onto #59399 now that it has landed.
Since #57416 the inactivity sweep keeps a location alive while a running session or a background job owns it. A running terminal counts as neither: terminals produce no session events, so a location whose only activity is an open terminal is still evicted on schedule and the Pty finalizer kills every terminal in it. That is the blank pane and "PTY session not found" in #48691, which the triage bot reproduced on v2.2.0 and current
dev.Before evicting an expired location, the sweep now also asks that location's Pty service for running terminals and refreshes the timer while any exist, the same way it already treats active sessions and background jobs. The probe uses
contextEffectOption, so it only reads the cached graph and never boots a location; a graph that failed to build counts as having no terminals, so failed locations are cleaned up as before.How did you verify your code works?
packages/core/test/location-activity.test.ts: a location whose graph is the realPtynode, with a runningcatterminal, stays cached when its deadline passes, and is evicted by the next sweep after the terminal is removed. The case fails without the fix.experimental.location_idle_timeout, non-canonical refs) pass unchanged.packages/coretypecheck, the pty, location-activity, location-filesystem and browser-idle suites, oxlint and prettier are clean.POST /api/ptywithsleep 4000, no session activity, 1 minute idle timeout.Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #51828