worker-list is the enumerating command the guides point at, but its text
rows omitted projection.nextAction.argv, so non-JSON callers could not
follow the documented loop. worker-show already prints it.
exposeRun had zero coverage: emptying INTERNAL_RUN_COLUMNS left every
suite green. Adds a unit pin on the column set plus an RPC-level pin
through orchestration.runCreate.
* Shorten orchestration skill description under the Agent Skills 1024-char limit
The folded description was 1038 chars, so spec-conforming installers such
as SkillStar rejected the bundled orchestration skill. Drop the two clauses
already covered elsewhere in the same description: "decomposing work across
agents" (implied by "structured multi-agent coordination") and "automation
of the browser embedded inside Orca" (restated by the locked `orca-cli`
embedded-pages sentence). Every routing trigger asserted by
orchestration-skill-guidance.test.mjs, the orca-cli handoff boundary, and
the Computer Use boundary are unchanged. Result: 958 chars.
Add config/scripts/skill-description-length.test.mjs, which parses every
skills/*/SKILL.md frontmatter with `yaml` and fails on an empty or >1024
char description, so the regression cannot return. orca-cli sits at 1015
and is left as is.
Fixes#17935
* Keep the embedded browser in the orchestration description's orca-cli routing
Restores the word "browser" in the orca-cli sentence ("and the Orca embedded
browser") so agents scanning for it still route embedded-browser control to
orca-cli. Description is 985 chars, 39 under the spec limit.
Every path that re-points a Dispatch at a pane/process bumps its consumer generation and fences the outstanding dispatch:<id> Delivery in the same transaction.
The orchestration description on main is 1,038 chars and conforming
installers reject it (#17935). This branch's rewrite already sits at 866;
the guard keeps it there. Guard adapted from #17959.
run-create/use/current/show/list returned the raw row plus a duplicate
binding object; strip home_database and coordinator_pane_key and drop the
duplicate. consumer_generation stays on the row.
* Add interactive scroll indicator for tab strip
- Support thumb dragging and track clicking to scroll
- Auto-expand on hover and show when scrolling
- Forward wheel events to tab container
- Disable indicator during tab drag operations
* Fully hide scroll indicator when disabled
When disabled, the tab strip scroll indicator is hidden, unexpandable on hover, and doesn't forward wheel events. Consolidates disabled state checks across visibility, event handling, and CSS classes.
* Cleanup active scroll indicator drag when disabled
When the scroll indicator becomes disabled or hidden mid-drag, immediately cancel the drag operation to prevent window listeners and body styles (cursor, user-select) from remaining in a stuck state.
* Refine tab scroll indicator appearance and sizing
Reduce idle and expanded heights by 1px and 2px respectively, simplify color
expressions from color-mix to straightforward opacity utilities, and use
transparent background at rest for a more subtle, cohesive appearance.
* Simplify palette attention glyph styling
Remove visual styling from the container so the glyph appears as a
lightweight overlay on the row icon, not a selection bubble.
* Keep attention glyph knockout white when row is selected
The glyph now uses a white background (bg-popover) with a ring to create a
visual knockout effect that separates it from the icon. This prevents the
glyph from inheriting the row selection styling, ensuring it stays visible
and distinct regardless of selection state.
* Correct attention glyph knockout color description to popover-colored
* fix(remote): stop a disclosure list latching the mirror completeness gate
`hostScope.omittedHostIds` was doing two jobs with opposite requirements. As
disclosure it must over-name: `omitted-host-scope-selectors.ts` deliberately
keeps ids for servers that are no longer paired so a caller can still see the
gap, and `docs/reference/ssh-execution-boundary.md` requires a listing to name
what it did not cover. As a completeness gate it must name only coverage that
was owed and not delivered, or it latches.
It latched. `workspaceSessionsByHostId` keeps a partition for every runtime a
machine has ever paired with and nothing prunes it, and a mirrored `remote:` row
names its peer too — so any client that has ever paired outward publishes a
permanently non-empty `omittedHostIds`. `probeHostLiveTerminals` read that as
`unverifiable`, `markHostSessionMirrorHydrated` never fired, and panes parked on
`parkUntilHostSessionMirrorHydrates` never drained.
`hostScopeCensusIsComplete` gives the gate its own answer and leaves the
disclosure list alone. A `runtime:` host is never owed coverage by the runtime
answering: a paired runtime is a peer with its own control plane reached with
`--environment`, and there is no paired-runtime PTY provider for this runtime to
have queried. Two other branches stay load-bearing — an absent scope is a host
too old to claim one, and a listing that covered no host proves nothing.
No wire change: the host publishes byte-identical content and only the client's
reading moves, so this reaches the reporter by updating their client alone
rather than waiting for their remote. That also avoids a new field's fallback
rule, where "absent means complete" would recreate the bug with the polarity
flipped.
`queried-host-kinds.test.ts` pins the invariant the predicate rests on at its
source, because the consolidation moving the SSH path onto orcad is the change
most likely to introduce a runtime-backed PTY provider and quietly invalidate it.
Fixes#18595
* test(remote): pin the orphan-recovery host-scope gate and narrow the invariant claim
The readiness review found the second gate unpinned: reverting
`web-session-terminal-orphan-recovery-inventory.ts` alone to the pre-PR
expression left the whole renderer suite green, because every existing fixture
passes `omittedHostIds: []`. The commit claimed two gates and proved one.
Four cases now drive `resolveTerminalOrphanInventory` through a non-empty scope.
Reverting that gate alone fails the peer-runtime case. Note the absent-scope case
deletes the key rather than passing `undefined`, because `listResult` substitutes
its default for `undefined` — routing through the fixture there silently tests
the default instead.
`queried-host-kinds.test.ts` also claimed more than it caught: a runtime-backed
transport registered under an SSH connection id reports as `ssh:` and passes,
which is the shape the orcad consolidation is expected to take. It pins the
spelling this function emits, which is what the gate keys on, and now says so.
* fix(remote): require a legible covered host before believing a census
CodeRabbit found a real asymmetry: the predicate refused an omitted host id it
could not parse, but accepted an unparseable *covered* id as proof of coverage.
`isTerminalListResult` validates only that `hostIds` is an array, so
`{hostIds: ['runtime:'], omittedHostIds: ['runtime:env-7']}` was `unverifiable`
before this PR and would have become `complete` after it.
Taken as "at least one legible covered host" rather than the suggested "every id
parses". A host that later gains a kind this client cannot parse would otherwise
report an incomplete census forever — which is this bug in a new coat, and the
failure mode the predicate exists to prevent.
The check exposed four tests publishing `hostIds: ['remote-runtime']`, a bare
environment id that `parseExecutionHostId` rejects. No host emits that: a runtime
answering `terminal.list` names the execution hosts it covered, which is `local`
— verified against a live paired runtime. Those fixtures are corrected to the
shape the wire actually carries, which is why the assertions move.
Two of the pins added in #18635 did not discriminate. Found by review of the
merged change; both are test-only defects, the fix itself is unaffected.
`resolves the ConPTY DLL before it claims the close` searched the whole patch
for `HANDLE hLibrary = LoadConptyDll(info, useConptyDll);`. That line occurs
twice -- PtyConnect's copy comes first -- so indexOf always matched PtyConnect,
and the ordering assertion held no matter where PtyKill resolved the DLL.
Verified by simulation: moving PtyKill's resolve back below the claim left the
suite green.
`reaches hShell only under the null check` used a marker as a slice END bound
without checking it existed. If that marker vanished the slice ran to the end of
the patch, the stray-line filter found nothing, and the test passed silently.
Both now anchor inside the PtyKill hunk only, located by its header's function
context rather than line numbers. `indexIn` throws on a missing marker instead
of returning -1, so a marker that moves fails the assertion that depends on it
rather than making it vacuous.
Adds the pin that was missing entirely: PtyKill's half of the two-sided baton
free. Without it a self-exit followed by kill() -- the ordinary pane close --
leaks one baton and one entry in the vector get_pty_baton scans linearly.
Mutation-tested rather than only revert-tested, because wholesale reverting the
patch is what hid this: it fails every assertion for the trivial reason that
nothing matches. Simulating each specific regression instead:
- move PtyKill's DLL resolve below the claim -> 1 failed (was: 0)
- drop PtyKill's baton free, line-count-neutral -> 2 failed (was: 0)
Wholesale revert still fails all 9.
Refs F24.
* 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.
* fix(runtime): let a scoped worktree listing report the host it could not cover
`orca worktree list --repo <id>` passed `[]` as `knownHostIds`, so a scoped
listing could never report a gap — for any host kind, reachable or not. With zero
matched rows both scope lists are empty by construction, and the answer is
`{hostIds: [], omittedHostIds: []}`: byte-identical to a repo that genuinely has
no worktrees. docs/reference/ssh-execution-boundary.md forbids a listing from
implying exactly that.
Measured on one runtime with one refusing SSH host, in the same second:
unscoped totalCount 0 omittedHostIds ["local","ssh:<target>"] (+ --host selectors)
scoped totalCount 0 hostScope {"hostIds":[],"omittedHostIds":[]}
The same runtime reports nine omitted hosts unscoped and zero scoped against a
live profile, so this is not a subtle inconsistency: it is one runtime giving two
contradictory answers about its own coverage.
A scoped listing now names the one host the caller asked about. That costs
nothing when rows come back — the host lands in `covered`, so it is never
reported omitted — and is the whole answer when they do not. Hosts the caller
scoped out are still never named, which a test pins, because naming them all is
the obvious over-correction.
* test(runtime): pin the scoped host derivation for local and executionHostId repos
Review flagged that the host-scope cases only covered a connectionId repo.
getRepoExecutionHostId reads two spellings, and a scoped listing naming the
wrong host would be worse than naming none, so both are pinned. Both fail with
the fix reverted.
A context-only dispatch to the caller's own pane writes nothing into it, and low-level topologies (plus the idle-mail e2e) depend on it. Only an injected preamble self-adopts.
* fix(relay): observe Windows PTY child processes instead of answering false
`processHasChildren` returned a hardcoded `false` on Windows, and a hardcoded
negative is indistinguishable from a measurement. Every close guard reads it as
"nothing is running in this pane", so an SSH-to-Windows tab running a build
closed with no prompt. Measured on a real Windows SSH host: a live `PING.EXE`
under the pane's `cmd.exe` still reported `hasChildProcesses: false`, while the
identical harness on Linux reported `sleep` / `true`.
Windows has no `ps`, but it does have a process table, and the pane walk over it
already existed for the foreground reader. The answer now comes from
`queryWindowsPaneProcessInventory`; a table it could not read reports
`unverifiable` rather than a fabricated negative.
`hasChildProcesses` is a boolean, which cannot hold the third answer, and it is
read both as "busy, do not close" and as "the agent took the PTY, safe to type
into" — so no single mapping of `unverifiable` is safe for both. The verdict
moves to a new optional `childProcessEvidence` member that the close paths read;
the boolean keeps its exact meaning for every client that cannot.
Cost: `pty.inspectProcess` is the polled path and a relay host has no
`@vscode/windows-process-tree`, so its table read falls back to the 1.36s CIM
scan. Polling that would reinstate the fork storm the shared table exists to
prevent, so only a caller whose answer decides something asks for the scan.
* fix(runtime): forward scanChildProcesses through the environment inspection RPC
`guardRunningTerminalClose` asks the host to pay for a real child-process read,
but the environment path dropped the option before it reached the wire: the
renderer sent only `expectedIncarnationId`, and the RPC schema — the shared
`TerminalHandle` — silently stripped anything else. A host routing that pane
through an SSH relay then declined to scan and answered `unverifiable`, which
`inspectionReportsRunningWork` reads as running work. The result was a close
confirmation on an idle pane, which is the nag this PR exists to avoid.
Forwarded through all four layers: renderer payload, RPC schema, method handler,
and the runtime/controller signatures. The schema is a dedicated extension rather
than a field on `TerminalHandle`, so `clearBuffer`/`agentStatus`/`isRunningAgent`
keep refusing an option they have no use for.
The silent strip is not itself the defect — it is what makes a new optional member
safe to send to an old host, per docs/reference/remote-wire-compatibility.md. The
defect was the schema and its caller drifting inside one version, so the tests pin
the registered method rather than the schema alone: pointing it back at
`TerminalHandle` compiles, parses, and drops the option.
Found by review on #18591.
* fix(terminal): teach the shared running-work probe the third child-process answer
Rebasing onto main landed `probePtyRunningWork`, which is a better home for this
than the close guard: it already speaks `live` / `unverifiable` / `exited`, and it
exists so the tab-close and window-close guards cannot drift. The child-process
verdict belongs there, not in a parallel predicate beside it.
So the mapping moves into the probe and `inspectionReportsRunningWork` is deleted
rather than kept alongside. The probe now asks for the scan, and a host that could
not observe the pane reports `unverifiable` instead of collapsing onto `exited` --
which is what `hasChildProcesses: false` meant on every Windows relay.
The pane-close path is routed through the same probe for the same reason; it was
the third caller asking this question through a direct inspect of its own.
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.
* fix(hosts): resolve a folder workspace's SSH host from the repo's host, not its raw connectionId
`resolveFolderWorkspaceHost` inferred a workspace's host by reading
`repo.connectionId` directly. SSH ownership has two spellings on a repo row, and
a row carrying only `executionHostId: 'ssh:<target>'` has no `connectionId` to
read — so it counted as a local repo and the workspace resolved `{ kind: 'local' }`.
That is an execute-here answer for a workspace whose files are on an SSH host,
the #11163 class, and it fires on a well-formed row.
Resolve the host first, then read the target off it. Every other row keeps its
existing contribution, including a `runtime:` row's nested SSH target: that
target is not this client's to dial, but narrowing it here would be a second
behaviour change riding on this one. The runtime branch above still answers
`local`, and now says so — `FolderWorkspaceHost` has no runtime variant, and
widening the type is its own change, not an oversight to be silently corrected.
Three smaller items that stand on their own:
- `resolveWorktreeExecutionHost` gains a `malformed` reason distinct from
`unknown`. `unknown` (nothing carries the id) is a verdict the launch path may
legitimately dispose of as a plain local folder; `malformed` (the row named a
host that cannot be parsed) must fail closed. One word for two situations is
the shape that lost the distinction in #18006. The strict read is private to
that module: `getRepoExecutionHostId` stays the answer everywhere else, since
its fall-through to `local` is harmless for the grouping, label and index
callers that are nearly all of its ~340 call sites.
- `readAllWorktreeMetaForRepo` / `readWorktreeMetaForRepo` replace four
open-coded copies of the same host-qualified read (the F7/F8 lockstep shape).
- `getExecutionHostLabel` answers 'Unknown host' rather than 'All hosts' for an
id that names no host. Showing one unroutable row as though it were on every
host is wrong on its own terms. Plain English like every other label in that
module, none of which resolve through the renderer's i18n catalog.
* fix(hosts): resolve the host in candidate selection too, not just in resolution
The first pass fixed how a repo row is classified once it reaches
`resolveFolderWorkspaceHost`. The candidate filter decides which rows reach it at
all, and it read `repo.connectionId` raw as well — so an SSH-only row outside the
project-group subtree was dropped before the new logic could see it, and the
execute-here bug survived for the population the fix was for, via a different
path. Found in review by CodeRabbit.
Three repo-row reads had the same root cause, not one:
- the scope-connection filter, comparing a path repo's raw field against the
workspace/group connection;
- the group-connection set, built from group repos' raw fields;
- that set's membership test against path repos' raw fields.
The last two are one comparison with the mismatch on either side, so resolving
only the path side would have reintroduced it from the other direction.
All three, plus the resolution loop, now go through one `getRepoScopeConnectionId`
helper. Non-SSH hosts still fall back to the raw field, so a `runtime:` row keeps
contributing its nested target exactly as before.
The new tests use a repo matched only by path, outside the subtree — the
population every existing test missed, which is why four passing revert-tests
did not catch this. One of them is labelled as pinning the resolver rather than
the filter: under the old raw read both rows came back connectionless and matched
each other by accident, so it survives a filter revert and must not be counted as
coverage for it.
* fix(relay): stop taking the fleet-wide cell inventory lock on per-connection paths
activateControl, acquireActivity, changeActivity and
removeSupersededSameCellControls each adjust exactly one cell's
reservation, yet took SELECT * FROM relay_cells FOR UPDATE, so every
desktop rebind and phone reconnect in the fleet queued behind every
other one and behind placement. They now use the single-row atomic
update (or lock only their own cell row), leaving the inventory lock to
placement and sweeps.
Fleet-wide 55P03 retries ran p50 430 / p99 1320 per five minutes on
2026-09-03, every cell pinned sqlLatencyMsMax at the lock timeout, and
the old cell image crashed on the resulting pool timeouts ~every 15
minutes. A real-Postgres test holds another cell's row and asserts a
rebind proceeds; re-adding the inventory lock fails it.
* fix(relay): lock the touched cell rows in order on cross-cell activity moves
Review found that acquireActivity's existing-lease branch could lock the
old lease's cell row (via removeActivityLease) before the new cell's row,
which cycles with placement's ascending inventory lock; reproduced on
real Postgres as paired 55P03 retries. lockCellRows now takes the one or
two rows a per-connection path touches in cell_id order with the 500 ms
request bound, and the census fails on any inline relay_cells FOR UPDATE
outside the named lock helpers. A three-cell Postgres test moves an
activity from the highest cell to a lower one while the target row is
held and asserts the mover holds nothing else; five revert-mutants
(inventory lock on each path, dropped ordering, dropped ORDER BY) fail it.
* test(relay): make the inline relay_cells lock census scan whole statements
Review showed two evasions: a FOR UPDATE inside query() and a queryLocked
whose FROM relay_cells sat past a fixed line window. The guard now matches
every query()/queryLocked() template statement in full; both evasions
fail it. Also clears relay_cell_connection_snapshots in the connection-
headroom Postgres suite so an aborted run does not poison the next.
* fix(ssh): move Windows file writes off PowerShell 5.1 stdin onto sftp
#16432 was fixed by chunking writes to 32KB, on the belief that a
`DefaultShell=cmd.exe` host caps one stdin at roughly 50KB. Re-measured on
Windows 11 26200.9168 / OpenSSH_for_Windows_10.0p2, that premise is wrong in
both directions, and the chunking does not fix the hang.
The real constraint: a read on Windows PowerShell 5.1's redirected-stdin handle
over a non-pty ssh exec can die permanently when it finds the stream momentarily
empty, taking both the remaining data and the EOF with it. It is probabilistic
per such read — not a size threshold, and not certain on the first one. Measured
by swapping the copy loop for a counting reader:
a 1.5s gap before any byte -> 0 bytes received, 6 of 6
1 byte, 1.5s gap, then 32767 -> exactly 1 byte
32768, 1.5s gap, then 32768 -> exactly 32768
a continuous 2MB -> 167936 / 270336 / 372736
Those three 2MB figures are one payload run three times under the same
conditions, which is what rules out a threshold. Independently reproduced by a
second harness where one 1.9MB counted read completed through 39 reads and
another died after 11.
A payload that fits one burst usually presents only one read that can find the
stream empty, which is why 32KB mostly works — and it still failed 15 times in
120 under load, and 1 in 40 on a quiet host. Neither rate survives the 62 execs
a 1.9MB file needs: even 2.5% compounds to about four uploads in five failing.
No chunk size helps, because the defect is per blocking read, not per byte.
Three controls on the same host, same DefaultShell, rule out both a size limit
and cmd.exe: `findstr` took 2,016,000 bytes through one exec's stdin, sftp moved
1.9MB 5/5, and PowerShell 7 took 2MB in one exec.
Windows writes now go over the sftp subsystem, whose batch script is read by
the *local* client, so no remote process reads a pipe at all. PowerShell 7 is
the fallback where sftp is unavailable, and Windows PowerShell 5.1 is last,
still bounded, and now reports the host limitation and its remedy instead of a
bare timeout.
Measured on the same host, through this code: 1.9MB x20 all succeeded,
hash-verified, median 315ms, against 0/6 before. 32KB x120 zero hangs, against
15/120.
Also:
- Stage under a unique name per attempt. An abandoned write leaves a remote
process that may still hold the staging file, and losing contact is not
evidence it died (docs/reference/ssh-execution-boundary.md), so a retry must
not reuse a name its predecessor may own. Sweep is best-effort and never
treated as proof of anything.
- Create upload directories over sftp too; the JSON mkdir batch rode the same
defective read.
- Cover makeWindowsWriteFileCommand and the publish command against the
8000-char budget, which F11 flagged as untested.
* fix(ssh): replace the staged Windows write atomically, and translate ssh -l
Three review findings, all on the failure path that the success-path
measurements say nothing about.
CodeRabbit, Critical: the publish deleted the destination before moving the
staged file onto it, so a failed move destroyed the user's existing file and
left a window where a reader saw no file at all. That is worse than the
truncated partial the staging discipline exists to prevent. Now File.Replace
(Win32 ReplaceFile, atomic), falling back to a plain Move only when the
destination is absent — and that race is safe, because a destination appearing
in between makes Move throw with the staged file preserved. The exclusive
branch already had it right: Move throwing on an existing destination is the
exclusive contract. Append stays non-atomic and now says why.
buildSshArgs can emit '-l <username>' for a config alias no Host block claims,
and the translator threw on it. isSftpUnavailableError read that throw as 'this
host cannot do sftp', so those hosts fell back to the defective PowerShell 5.1
path and had the refusal cached against them for 30 minutes, silently. '-l' now
maps to '-o User=', with a test for the exact argument shape buildSshArgs
produces in that case.
CodeRabbit, minor: two assertions passed on an absent observation — an
unmatched regex yields '' and every() is true of an empty list. Both now assert
the positive form first, and the same audit was applied to the three other
some()/every() assertions in the file. The temp-file test now asserts mode 0600
rather than only that the file is cleaned up.
* fix(ssh): keep a path sftp cannot spell from becoming a verdict about the host
Audit of isSftpUnavailableError, prompted by the '-l' gap having the same
shape: a per-operation condition being written into a per-host cache that
holds for 30 minutes.
It had a second instance, and this one was mine. UnsupportedSftpPathError was
classified as 'this host cannot do sftp', but it is thrown for a UNC or
relative destination and for any path sftp's batch lexer cannot quote --
including a *local* filename containing a newline, which POSIX clients allow.
One such file would have routed every later Windows write to that host down
the defective PowerShell 5.1 path for the rest of the cache window.
The host verdict is now only the errors that really are host-scoped: a refused
subsystem, a client that will not start, and an untranslatable argument list.
A path refusal falls back for that one write and leaves the cache alone, in
both the file-write and directory-creation paths.
Revert-tested. Removing the operation-scoped catch fails all three new tests,
whether or not the predicate is also widened. Widening the predicate alone
does not fail them, correctly: with the catch in place the predicate no longer
gates that path, so keeping it narrow is defence-in-depth rather than the live
mechanism.
Flag audit at the same time: -F, -o, -T, -S, -p, -i, -J, -l and -- are now the
complete set buildSshArgs can emit, and all are handled.
* fix(ssh): make the atomic publish actually run, and unroll the mkdir batch
Two runtime defects that only a real host could surface. Both were invisible
to unit tests that assert the shape of the generated command string, because
both are PowerShell rejecting an argument at execution time.
File.Replace was passed a bare $null for destinationBackupFileName. PowerShell
coerces $null to an empty string when binding a .NET string parameter, and
Replace rejects that with 'The path is not of a legal form' -- so every
create-mode publish failed. The Critical fix was inert as shipped. Now
[NullString]::Value, which is the construct that exists for this.
Measured on awin, same staging-file lock, opposite outcomes:
old publish rc=1 destination MISSING <- prior contents destroyed
new publish rc=1 destination PRESENT, sha 7f06b7e0... unchanged
control, destination present, no lock rc=0 replaced exactly
control, destination absent, no lock rc=0 Move fallback created it
End-to-end through the real uploader afterwards: 1.9MB x15 all hashes exact,
median 303ms; overwrite of an existing destination exact both times.
Separately, the PowerShell mkdir fallback could not create a tree of more than
one directory. '@($json | ConvertFrom-Json)' wraps the parsed array in another
array, so the loop variable binds to the whole thing and [string] of it is the
paths joined by spaces. It only ever worked for a one-element batch, where
stringifying a single-element array happens to yield the element -- which is
why no existing test caught it. Pre-existing on main; fixed here because this
PR puts that command on the fallback tier and claims the ladder works.
Both tiers now verified live against a three-directory tree.
* fix(crash-reporting): sample system memory before the renderer dies
* fix(crash-reporting): make the pre-gone host sample decisive, not just present
Round-1 review said the shipped field set could not decide G4-oom. Fixed.
Decisive field (blocking #1). The investigation's own win-lowspec repro
falsified "low available commit kills": at a 127 MB commit floor Windows grew
the pagefile to 2029 MB and nothing died, and it named the missing datum —
pagefile-growth headroom / system-drive free space. `getSystemMemoryInfo()`
gives neither. Added `swap-volume-free-space.ts`: one `fs.statfs` on the volume
backing the pagefile (SystemRoot on Windows, the root fs elsewhere, resolved
via `path.parse().root`), published as `systemMemoryPreGoneSwapVolumeFreeMB`.
Together with the already-emitted commit limit that separates "commit was low"
from "commit was refused". Pagefile *max* size needs a registry read; skipped
deliberately — per-operation interpreter spawning is exactly what
docs/reference/windows-edr-posture.md says not to add for telemetry.
Darwin honesty (blocking #2). Every reading now carries
`systemMemoryPressureSignal`: `available-commit` on Windows (swapFree is
ullAvailPageFile), `mem-available` on Linux when MemAvailable is present,
`none` otherwise — which is always on darwin. A future analyst cannot now table
`freeMB: 272` from a healthy Mac as evidence of exhaustion, because the same
record says the platform gave no pressure verdict. Partial rebuttal on the
suggested reuse: `host-memory.ts:86` was considered and rejected as a periodic
source. It spawns `/usr/bin/memory_pressure` per call, and the sampler this PR
needs runs every 10 s for the app's lifetime; a subprocess at that cadence is
worse than the gap it closes. The reviewer conceded this tradeoff is arguable —
what was not acceptable was shipping the darwin gap silently, so it is now in
the data, not only in a comment.
Staleness (blocking #3). Confirmed the measurement: four of five G4 reports
carried a ~37 s-old sample (4872/36796/37332/38017/39715 ms). Host memory no
longer rides the 60 s process-metrics sweep; `pre-gone-host-memory.ts` samples
it on its own 10 s timer with its own `systemMemoryPreGoneSampleAgeMs`. One
GlobalMemoryStatusEx-class call plus one statfs is cheap enough at that rate. A
refusal shorter than the interval stays invisible and the module comment says
so — no polling cadence fixes that.
Non-blocking, all taken: renamed `gone-time-system-memory.ts` ->
`system-memory-details.ts` with the now-false "reads AFTER the crash" framing
scoped to the gone-time caller; pre-gone host keys moved out of the
`processMetrics` namespace to `systemMemoryPreGone*`, so the string-surgery
`preGoneDetailKey` helper is gone and a `systemMemory` prefix scan sees both
reads; the bare catch no longer spans both halves of the sample, and a test
pins that a throwing host read leaves the process-metric sample intact; the
inert second test is replaced by three that go red without this change
(verified: swap-volume, pressure-signal and cadence assertions all fail when
the production hunks are reverted).
Rebuttal, non-blocking #5 (duplicated electron mock across two test files):
declined. `vi.mock` is hoisted per file, so the mock cannot be shared without a
setup module, and this directory already has 26 focused test files that each
re-declare it. Splitting by concern is the local convention.
`startPreGoneProcessMetricsSampling` is renamed `startPreGoneCrashSampling`
since it now starts two samplers.
* fix(crash-reporting): test the arming, gate the swap volume, unblock the host read
Round-2 review blocked on four items. All four addressed.
WHAT THIS BRANCH ACTUALLY DOES, AT HEAD (blocking #4). The commit-1 message
("13 lines, 1 production file, no new module, new optional numeric fields
only", `processMetricsPreGoneSystemMemory*` keys, a `preGoneDetailKey` helper,
a `pre-gone-system-memory.test.ts`) describes a superseded revision; every one
of those claims is false now, so it must not be used as the PR description.
The change against origin/main is: 3 new production modules
(`pre-gone-host-memory.ts`, `system-memory-details.ts`,
`swap-volume-free-space.ts`), 1 deleted (`gone-time-system-memory.ts`), plus
edits to `process-gone-diagnostics.ts` and `main-process-ready-runtime.ts` and
2 test files. It adds a second main-process interval timer that runs for the
life of the app: every 10 s one synchronous GlobalMemoryStatusEx-class read,
and on win32/darwin one `fs.statfs` on the swap-backing volume. Details are
`systemMemoryPreGone*`, and two of them are STRINGS, not numbers:
`systemMemoryPreGonePressureSignal` (enum) and `systemMemoryPreGoneSwapVolume`
(a drive label, separator-trimmed so it is not a path). Both are assigned after
`sanitizeCrashReportDetails`; neither carries user content.
Arming is now tested (blocking #1). The reviewer deleted
`startPreGoneSystemMemorySampling(...)` from `startPreGoneCrashSampling` and
all 264 tests stayed green — confirmed and fixed. `pre-gone-host-memory.test.ts`
now calls `startPreGoneCrashSampling()` with production defaults and asserts
both `setInterval` calls, their literal periods `[60_000, 10_000]`, that both
timers are unref'd, and that advancing 10 s takes a fresh host sample that
reaches `buildProcessGoneCrashDetails` with `SampleAgeMs: 0`. Verified red on
revert: deleting the arming line -> 1 failure; changing the interval constant
to 30_000 -> 1 failure (the old assertion compared the constant to itself and
caught neither). The tautological `10_000 < 60_000 / 2` test is gone,
superseded by this one.
Swap volume is win32/darwin only (blocking #2). On Linux swap is a fixed
partition, a fixed-size swapfile, or zram; none grow into root-fs free space,
so `SwapVolumeFreeMB: 380000` beside `SwapFreeMB: 0` would have invited exactly
the wrong verdict on the two Linux cluster members. `swapVolumeAnchor` returns
undefined off win32/darwin, so no field and no statfs at all. The comment
claiming "elsewhere swap is on the root fs" was wrong and is gone. The Windows
anchor is still the DEFAULT pagefile volume, so the measured volume now ships
with the number (`systemMemoryPreGoneSwapVolume: 'C:'`) instead of being
implied. The honesty label covers it: win32 reads `available-commit` only when
the volume datum is present, and `available-commit-unqualified` otherwise —
which also fixes non-blocking #5, where the synchronous gone-time read claimed
a verdict its own fields could not support.
Host read no longer waits on statfs (blocking #3). `samplePreGoneSystemMemory`
now commits the synchronous memory reading first and merges volume free space
in afterwards, so the cadence is 10 s regardless of disk-metadata latency and a
hung volume can no longer stop host sampling — precisely the paging-storm case
this exists for. The in-flight latch now guards only the statfs. Verified red
on revert to the serialized shape (2 failures). A stale-but-slow-moving volume
value merging into a newer memory sample is deliberate and commented.
Non-blocking #3 (reset does not invalidate an in-flight sample): fixed with a
generation counter bumped by `resetPreGoneSystemMemorySamplingForTest`, so a
late statfs cannot repopulate a reset sample. Separately, the volume read now
only runs after a host sample committed, which removes the real `statfs('/')`
side effect from `process-gone-diagnostics.test.ts` entirely.
REBUTTAL, darwin `memory_pressure` reuse (non-blocking #2): declined, with
evidence. `readDarwinAvailableMemory` at src/main/memory/host-memory.ts:87 is
reached only via `collectHostMemory` <- `runSnapshot` <- `collectMemorySnapshot`,
whose only callers are the `memory:getSnapshot` IPC handler and
orca-runtime-pty-foreground-process-reads.ts:170 — both on demand. There is no
periodic snapshot, so there is no cached reading to reuse for free; adopting it
means spawning `/usr/bin/memory_pressure` on a main-process timer for the life
of the app, and its module-global `darwinAvailabilitySupported` latch is shared
with the memory UI. The gap is not hidden: darwin ships
`PressureSignal: 'none'` in the data, and the module comment now cites the
existing reader and why it is not used here rather than claiming Orca lacks
one.
Verified: `vitest src/main/crash-reporting src/main/startup src/main/memory` =
769 passed / 6 skipped (crash-reporting re-run 5x, no flake);
`tsc --noEmit -p config/tsconfig.node.json` 0; `oxlint` 0; `oxfmt --check` 0.
* fix(crash-reporting): stop a stale statfs qualifying the commit verdict
Round-3 adversarial review, 2 blocking. Both fixed with mutation-verified
tests.
1. `mergeSwapVolumeFreeSpace` merged the volume reading into whatever sample
was current at RESOLUTION time, and `pressureSignal` then upgraded win32
from `available-commit-unqualified` to the decisive `available-commit` on
the strength of it. The `swapVolumeReadInFlight` latch makes every
intervening tick skip the merge, so the lag is as old as the last STARTED
statfs, not the last tick — and no age field exposed it, because
`systemMemoryPreGoneSampleAgeMs` describes only the synchronous memory read.
Reviewer's executed scenario: a statfs issued at t=0 on a healthy host
(40 GB free) resolving at t=20 s of commit pressure emitted
`SwapFreeMB: 200` beside `SwapVolumeFreeMB: 40000`, labelled
`available-commit`, with `SampleAgeMs: 0`. That reads as "the pagefile had
room, so this was not a commit refusal" — the opposite conclusion, wearing
the branch's highest-confidence label, on exactly the win32 G4-oom reports
this exists to decide.
The datum still ships (it is the only pagefile-expandability signal there
is), but now:
- the sample carries `swapVolumeSampledAtMs` — the tick that ISSUED the
statfs, never the one it resolved on — surfaced as
`systemMemoryPreGoneSwapVolumeAgeMs`;
- only a statfs that answers on its own tick may qualify the verdict.
`withSwapVolumeFreeSpace` takes `coTimed`; false keeps
`available-commit-unqualified`.
The next tick issues a fresh statfs, so the verdict recovers on its own.
2. The branch's sole production entry point — `startPreGoneCrashSampling()` at
main-process-ready-runtime.ts:128 — was untested. Deleting it left 691
tests across crash-reporting/ and startup/ green, while a comment in the
new test file claimed that gap was why the test was written. This is pure
instrumentation, so that one line is the whole of its value in the shipped
app. Added a source-level wiring test (the pattern this repo already uses
for arm-once ready-phase lines) that pins the import, exactly one call, the
call at statement indent, and that `main-process-ready.ts` awaits the
function it lives in. The misleading comment is gone.
Mutation-verified — each goes red alone:
coTimed -> always true 1 failed (verdict)
drop swapVolumeSampledAtMs age 1 failed (verdict test)
delete startPreGoneCrashSampling() 1 failed (wiring)
wrap it in `if (!is.dev) { ... }` 1 failed (wiring)
Verified: tsc -p config/tsconfig.node.json exit 0; oxlint
src/main/crash-reporting src/main/startup exit 0; 268 tests in
crash-reporting/ pass. Across crash-reporting/ + startup/ + memory/: 769
passed, 2 failed — both environment-dependent and failing identically on the
unmodified tree (Xvfb rebind, and a whole-repo glob census that times out).
* fix(crash-reporting): stop free disk standing in for pagefile growability
The win32 reading was promoted to the decisive `available-commit` whenever a
co-timed volume number merely existed, which the data cannot support: a fixed
or disabled pagefile grows into no amount of empty disk, its maximum is
unreadable here, and the measured volume is only the DEFAULT pagefile drive. A
host with 180 MB of available commit, a commit limit at RAM and 812 GB free
read as "the pagefile had room" — the opposite conclusion, under the branch's
most confident label.
The volume datum is now named for what it is (`available-commit-volume-cotimed`,
context beside the commit number), and the one decisive win32 case — a commit
limit at or below RAM, i.e. no pagefile behind it — gets its own label.
Also: carry the last volume reading onto the sample that replaces it, aged and
non-qualifying, so a statfs slower than one tick no longer makes the field
vanish from the reports it exists for; don't commit a reading whose every
memory field failed, which shipped an age and a disk-free number with no host
memory beside them; and move the startup wiring test beside the file it pins,
scoped to the ready-phase entry's own body so the call cannot satisfy it from
a sibling export nothing calls.
* fix(crash-reporting): co-time the statfs by tick, not sample identity
A tick whose host read fails leaves the pre-gone sample object in place, so
the identity check still read a 25 s-late statfs as co-timed.
* fix(terminals): bail out of no-op pane-expansion store writes
* test(terminals): lock the root-state identity of the bailout
A `return {}` bailout keeps the map reference but still allocates a new
root state, so zustand walks every listener. Assert root identity too.
* fix(crash-reporting): stop the codex POSIX teardown claiming a group that was already gone
terminatePosixTree's default group signal swallowed every process.kill error and
then recorded a self_tree_kill unconditionally, so an ESRCH — proof the group was
already gone and this teardown killed nothing — still put a suspect in the
five-second render-process-gone attribution window.
Every sibling group-kill in the tree already records only on a proven signal:
terminateDedicatedPosixGroup in this same file, forceKillPosixPtyProcessGroups,
and the claude account-login teardown. This makes the outlier match them.
* fix(crash-reporting): leave proof when the own-Chromium pid set cannot be read
`readOrcaChromiumProcessPids` returns an empty set when `getAppMetrics()`
throws, which is the right decision — refusing every kill would orphan every
PTY, git, codex and notebook tree main tears down, and on main a refusal from
`killSourceControlAgentProcess` releases the managed-home lock with the agent
still alive. But the empty set was byte-identical to "no Chromium on this
host", so the fail-open was invisible in a field bundle.
Keeps the decision, adds a coalesced durable `own_chromium_pids_unreadable`
crumb so the two cases are distinguishable. Coalesced because the gate reads
this set on every tree kill.
* style(crash-reporting): tighten the group-signal comments to the WHY
`terminal_replay_guard_wedged_release` was not in
COALESCED_RENDERER_BREADCRUMB_NAMES, and its per-pane hashes give every entry a
unique ring identity. One mount/reveal/wake transition expires every in-flight
replay write at once, so a burst arrives as N distinct entries against a 30-slot
FIFO ring.
Measured, from the 09-02 corpus (121 `renderer.breadcrumb` spans across 9 of 55
diagnostic bundles):
- bundle 26461769: 26 events in 0.96s
- murlock1000: 62 events over 85s
- 8907a508 mixes two call sites in one window (2 crumbs carry `tabIdHash`, 2 do not)
Not measured: no captured report's ring actually lost slots to this crumb. All
57 reports have zero wedge crumbs in `Recent activity:`, and in all 9 bundles the
burst predates the report's ring window — for 26461769 the burst ran
13:36:03.784Z-13:36:04.742Z while the ring-owning main process started at
13:43:44.380Z, 7m40s later. So this bounds a demonstrated hazard, not an observed
loss. An earlier draft of this commit asserted "26 of 30 slots / 87% of the
pre-crash trail" as a measurement; that was a model, and it is removed.
The burst evidence lives entirely in the durable span stream, and suppressed
repeats normally emit no span (see the 1000-emissions/1-span case in
crash-reporting-renderer-breadcrumbs.test.ts), so coalescing alone would have cut
that 121-event corpus to 13 with the multiplicity recorded nowhere. Instead:
- the ring coalesces: one slot per call site, plus `suppressedSinceLast`
- every wedge event still emits its own `renderer.breadcrumb` span, via
PER_EVENT_TRACED_COALESCED_BREADCRUMB_NAMES. Span volume is unchanged at 121,
and the span deliberately carries no count so a span-stream total cannot
double-count what the ring already claims
- the coalesce key is `ptyId`/`tabIdHash` *presence*, not name alone: those
fields are absent on the restore call site (restoreScrollbackBuffers) and
present on reattach, so name-only keying would collapse 8907a508's two call
sites into whichever crumb landed last. Bounded at 4 slots per storm, matching
the webgl `kind` and duplicate-tab `resolvedToActiveWorktree` precedents in the
same file. Replaying the corpus timestamps: 121 events -> 14 ring writes.
This is a diagnostics fix, not a crash fix. It does not stop panes wedging, and
it does not explain the "can't type" reports in this round.
The native window-close warning was built from a local-only pty set: any
worktree with a connectionId was dropped whole, and any remote runtime pty
was filtered out. A build, test run, or agent on an SSH or Orca Remote host
was therefore structurally invisible to it, on every platform. The quit path
skipped the check entirely (#524), so remote work got no prompt at all.
Route both paths through the same probe the tab-close guard uses, so the two
cannot drift, and keep the verdict vocabulary of the SSH execution boundary:
only a host that answers "no children" suppresses the warning. An unreachable
host is `unverifiable`, never `exited`, so it warns rather than quitting
silently — with its own copy, because "could not reach the host" is a
different claim than "processes are running".
Quit still ignores local ptys, preserving #524: quitting is an unambiguous
instruction to end this machine's processes, but not to end execution on
someone else's, which a bounded relay grace period will SIGKILL once the
countdown expires.
The probe budget is 1.5s (vs the tab guard's 4s) because quit is time
sensitive; expiry raises the prompt, so an unreachable host costs a click
rather than the 15s RPC timeout or a silently orphaned build.
setSleepingAgentAutomaticResumeBlocked had no production caller: the stamp only
ran at startup and the sweep only after release/retain/takeover, so a worker
settled by worker_done, stop, abandon or its own process exit left its pane
unfenced and reopening it in the same session respawned the agent. Every
settlement now sweeps, and the sweep pushes each fence change to the live
renderer over agentStatus:legacyWorkerTerminalResumeFence.
worker-observation's dispatch receipt publishes it as retryOfDispatchId, and
--retry-of validates the prior attempt and gates failed -> dispatched. Only the
index stays dropped: nothing queries by the column. The other five v31 columns
remain deleted.
Nothing ever stamps AgentStatusIpcPayload.processIncarnation/endpointId/
endpointIncarnation, so matchesOptionalIdentity could only reject a status the
Dispatch had already claimed. The reminted-pane branch still requires the
terminal handle.
WorkerTerminalTailArchive.sourceExact/contentComplete were written as constant
false and read with ?? false; derive them at the read boundary instead.
Jinwoo-H
Jinwoo Hong
Jinjing
Neil
github-actions[bot]