mirror of
https://github.com/stablyai/orca.git
synced 2026-09-23 00:02:29 +00:00
stack-final
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Neil
|
b33d1972bc |
docs(relay): correct why the ConPTY teardown asset diverges from the desktop patch (#18636)
The divergence pinned by #18601 is real and worth keeping, but its stated reason was wrong. It claimed the desktop patch carries the early conin placement "and therefore the +2 File / +1 Process regression, measured against its exact installed tree" -- i.e. that the shipped desktop app leaks because of its own leak fix. It does not, for any terminal a user opens. node-pty defaults `_useConptyDll` to false. Every desktop site that opens a pane sets it true (`local-pty-utils.ts` twice, `native-pty-spawn.ts`), as does the `windows-conpty-warmup.ts` warm-up, so they take the `else` branch, where upstream already destroys the input socket. The relay passes no such option (`src/relay/pty-handler.ts`) and takes the `!useConptyDll` branch -- the one both this asset and the desktop patch edit. The desktop is not entirely off that branch, though: the hidden rate-limit probes in `src/main/rate-limits/claude-pty.ts` and `codex-pty-rate-limit-probe.ts` omit the option, recur, and tear down through `kill()`, so the hunk is live there -- just never for a visible pane. Whether the early placement costs the same +2 File / +1 Process across a probe's lifecycle is unmeasured; the numbers in this comment were taken on relay-style spawn/kill cycles, and the comment now says so. What is settled is the replaced claim: not every Windows user, and not every terminal. Those two probes were missed three enumerations running because they use `await import('node-pty')`, which no static-import grep finds. The comment now tells the next reader to grep for `node-pty` instead. The measurement that produced the wrong claim was taken by a standalone harness that passed no `useConptyDll` and so defaulted into the branch it was not trying to measure -- the same standalone-is-not-the-real-host trap #18601's own body warns about, one level down. Also refreshes the self-exit paragraph, which #18635 made stale. That leak is now fixed for the desktop, and the note records why the fix cannot reach a Windows relay. The fix is mostly native (`src/win/conpty.cc`) and this asset only rewrites `lib/*.js`, and all three delivery paths stop short of Windows: pnpm patches do not cross the SSH boundary; `MATRIX_SLOTS` in `build-orcad-prebuilds.mjs` has no win32 entry; and the one relay asset that does patch native source and rebuild on the host (`node-pty-1.1.0-master-cloexec-patch.cjs`) returns `skipped:unsupported-platform` for anything but linux/darwin. #18635's flat self-exit relay numbers were measured against a locally rebuilt binary, so they describe the relay code path on a patched tree, not the tree a relay host installs -- the note says so explicitly rather than leaving the next reader to conflate them. Assertion and hashes unchanged: the relay must still release conin after the console-list fork, and a patch sync must still not copy the early placement onto the relay's branch, where it does cost +2 File and +1 Process per terminal. Only the justification changes, plus the test name, which said "like the desktop patch" where it meant "unlike the desktop patch placement". |
||
|
Neil
|
f7e3af254a |
fix(pty): close the pseudoconsole and dispose the conout worker on Windows self-exit (F24) (#18635)
* fix(pty): close the pseudoconsole when a Windows shell exits by itself
`ClosePseudoConsole` is the only thing that reaps a ConPTY's console host.
node-pty calls it from one place, `PtyKill`, which starts by looking the baton
up by id -- and the exit watcher in `SetupExitCallback` erased that baton the
moment the shell died. So on the self-exit path (typing `exit`, how panes
usually close) the lookup missed, `PtyKill` did nothing at all, and the
pseudoconsole was never closed.
The baton now survives until BOTH the shell has exited and `kill()` has run;
whichever arrives second frees it. `PtyKill` copies `hpc` out under the lock and
closes it afterwards, guards `TerminateProcess` on a shell handle the watcher
may already have closed, and duplicates that handle rather than reordering, so
upstream's close-then-terminate sequence is unchanged.
Measured on Windows 11, 20 self-exit cycles driven exactly as Orca drives them
(`onExit -> destroy()`), handles bucketed by NT object type:
relay spawn (no useConptyDll) 225 -> 285 (+1 Process +2 File/term)
after 219 -> 219 FLAT
desktop spawn (useConptyDll) 239 -> 439 (+1 Process +2 Thread +5 File/term)
after 235 -> 395 (+2 Thread +4 File/term)
The desktop residue is a separate defect in the `useConptyDll` branch of
`WindowsPtyAgent.kill()`, which disposes the conout worker only from an
`_outSocket.on('data')` handler -- and no data arrives after the shell has gone.
Fixing that line as well takes the desktop to 222 -> 222 FLAT, but it lives in
the `kill()` hunk owned by F23, so it is left to that change.
Refs F24.
* fix(pty): dispose the conout worker when a Windows shell exits by itself
Second, independent defect on the same self-exit path, and the larger half of
the desktop's leak. The `useConptyDll` branch of `WindowsPtyAgent.kill()`
disposed the conout worker only from an `_outSocket.on('data')` handler -- and
once the shell has gone no more data ever arrives, so the worker was never
disposed. The non-DLL branch three lines above already disposed unconditionally,
which is why only the desktop (the only spawner that sets `useConptyDll`) hit it.
Measured on Windows 11, 20 cycles, handles bucketed by NT object type, totals:
self-exit, relay spawn 225 -> 285 now 219 -> 219 FLAT
self-exit, desktop spawn 239 -> 439 now 222 -> 222 FLAT
explicit kill, relay spawn 225 -> 285 now 219 -> 219 FLAT
explicit kill, desktop spawn 235 -> 395 now 219 -> 219 FLAT
Neither fix alone is enough on the desktop: the pseudoconsole close is worth
+1 Process +1 File per terminal, this dispose +2 Thread +4 File.
The relay asset (config/relay-assets/node-pty-1.1.0-windows-pty-teardown-patch.cjs)
deliberately gets no counterpart: the relay takes the non-DLL branch, where the
dispose is already unconditional. Its reconstruction table needs the new hunk
though, or un-applying the desktop hunks no longer yields published node-pty.
Taken over from F23 at win-relay-qa's request after they verified that the
desktop never executes the non-DLL branch F23 was scoped around.
Refs F24.
* fix(pty): harden PtyKill against a failed handle duplication and a missing DLL
Both from review of #18635.
DuplicateHandle's result was dropped. On the live explicit-kill path a failed
duplication left hShellDup null, which the guard below could not tell apart from
the self-exit case, so TerminateProcess was skipped and the shell kept running
after its pane closed -- a worse outcome than the handle leak this patch exists
to fix. The failure now terminates through handle->hShell under the lock, where
it is valid and where TerminateProcess does not block. The only cost is that the
rare path kills before the console closes instead of after.
LoadConptyDll is now resolved BEFORE any baton state is touched, matching what
PtyConnect already does for the same reason. It throws when conpty.dll is
missing, and a throw after consoleClosed was set would strand the pseudoconsole
permanently: the retry finds the work claimed and does nothing.
Also corrects three comments the earlier commits made stale:
- the ptyJobMutex note still said PtyKill reads the table unlocked
- PtyListJobProcessIds said the baton is gone once the shell exits; it now
outlives the shell, and the nulled hJob is what makes the answer null
- windows-pty-job.ts said node-pty drops its handle record on exit
Re-measured on Windows 11 with the rebuilt binary, 20 cycles, all four paths
still flat: self-exit relay 219->219, self-exit desktop 222->222, explicit-kill
relay 219->219, explicit-kill desktop 219->219. Both explicit-kill runs report
22/22 shells exited, so the kill still lands.
Refs F24.
|
||
|
Neil
|
766b5b153c |
fix(relay): release the ConPTY conin handle after teardown, not before it (#18601)
A Windows SSH relay leaked one Windows File handle per terminal, for the life of the relay process, across reconnects. node-pty's `kill()` flips `readable` on the conin and conout sockets and destroys neither; `_cleanUpProcess` destroys `_outSocket`, so only conin is stranded, and it wraps a real named-pipe handle from `fs.openSync(term.conin, 'w')`. The obvious fix -- and the one config/patches/node-pty@1.1.0.patch ships for the desktop -- releases it at the top of the branch, before `_getConsoleProcessList()` forks and before the native kill. Measured against a real Windows SSH host, that is three times worse than leaving the leak alone: teardown aborts partway, the forked console-list agent is never reaped, and both pipe handles stay alive. Releasing it at the end of the branch instead is flat. 20 spawn/kill cycles, handles bucketed by NT object type, identical numbers standalone and through a real relay: published node-pty File +1/terminal, Process flat desktop patch placement File +2/terminal, Process +1/terminal released last (this) File flat, Process flat `windowsTerminal.js` takes the desktop's error-listener hunks verbatim. The conin listener is not what fixes the leak -- adding it alone changed nothing -- but it is what keeps a pipe error retiring one terminal instead of the host. The desktop patch has the early placement and therefore the regression, measured against its exact installed tree. Correcting it there needs its own verification on a Windows desktop build, so the trees diverge on this one hunk deliberately and a test pins that so a future patch sync cannot copy the bug back. |