diff --git a/README.md b/README.md index 7a3cbe2360c..2ae59035da8 100644 --- a/README.md +++ b/README.md @@ -238,9 +238,9 @@ Pair with your desktop app to monitor and steer your agents from your phone. - **Discord:** Join the community on **[Discord](https://discord.gg/fzjDKHxv8Q)**. - **Twitter / X:** Follow **[@orca_build](https://x.com/orca_build)** for updates and announcements. -- **WeChat:** Scan to join the Orca community WeChat group 8. +- **WeChat:** Scan to join the Orca community WeChat group 8. Group 8 may be full; if so, scan the Group 9 QR code instead. - WeChat group 8 QR code for the Orca community + WeChat group 8 QR code for the Orca community  WeChat group 9 QR code for the Orca community - **Feedback & Ideas:** We ship fast. Missing something? [Request a new feature](https://github.com/stablyai/orca/issues). - **Privacy:** See the [privacy & telemetry docs](https://www.onorca.dev/docs/telemetry) for what anonymous usage data Orca collects and how to opt out. diff --git a/cloud/apps/relay/src/assignment-connection-headroom-postgres.test.ts b/cloud/apps/relay/src/assignment-connection-headroom-postgres.test.ts index 6ac9521c3d6..80a74a47eeb 100644 --- a/cloud/apps/relay/src/assignment-connection-headroom-postgres.test.ts +++ b/cloud/apps/relay/src/assignment-connection-headroom-postgres.test.ts @@ -44,6 +44,12 @@ describePostgres('PostgreSQL assignment connection headroom', () => { `DELETE FROM relay_assignments WHERE user_id LIKE 'connection-headroom-postgres-%'` ) + // A snapshot left by an aborted run rejects the replayed watermark + // with stale_connection_snapshot. + await database.query( + `DELETE FROM relay_cell_connection_snapshots WHERE cell_id = ?`, + [cell.id] + ) await database.query( `DELETE FROM relay_cell_connection_runtime WHERE cell_id = ?`, [cell.id] diff --git a/cloud/apps/relay/src/assignment-control-supersession-postgres.test.ts b/cloud/apps/relay/src/assignment-control-supersession-postgres.test.ts index 10193b78cc6..cf8819686b5 100644 --- a/cloud/apps/relay/src/assignment-control-supersession-postgres.test.ts +++ b/cloud/apps/relay/src/assignment-control-supersession-postgres.test.ts @@ -38,6 +38,10 @@ describePostgres('PostgreSQL control supersession', () => { [identity.userId] ) await database.query(`DELETE FROM relay_assignments WHERE user_id = ?`, [identity.userId]) + // A snapshot left by an aborted run rejects the replayed watermark with stale_connection_snapshot. + await database.query(`DELETE FROM relay_cell_connection_snapshots WHERE cell_id = ?`, [ + cell.id + ]) await database.query(`DELETE FROM relay_cell_connection_runtime WHERE cell_id = ?`, [cell.id]) await database.query(`DELETE FROM relay_cell_connection_limits WHERE cell_id = ?`, [cell.id]) await database.query(`DELETE FROM relay_cell_runtime WHERE cell_id = ?`, [cell.id]) diff --git a/cloud/apps/relay/src/assignment-store.ts b/cloud/apps/relay/src/assignment-store.ts index d0517d46746..9d240e304a7 100644 --- a/cloud/apps/relay/src/assignment-store.ts +++ b/cloud/apps/relay/src/assignment-store.ts @@ -3202,8 +3202,7 @@ export class RelayAssignmentStore { ) const requestDelta = ACTIVITY_REQUEST_UNITS[kind] * (after - before) if (requestDelta !== 0) { - await this.lockCellInventory(transaction, 'request') - await this.adjustCellReservation(transaction, text(row, 'cell_id'), requestDelta) + await this.adjustCellReservationAtomically(transaction, text(row, 'cell_id'), requestDelta) } }) }) @@ -3263,9 +3262,12 @@ export class RelayAssignmentStore { } const units = ACTIVITY_REQUEST_UNITS[input.kind] if (existing) { - await this.lockCellInventory(transaction, 'request') + // Why: a client-chosen activity id can move between cells, so lock the + // one or two rows this path touches in cell_id order, the same order + // placement takes the inventory in, and no cycle can form. + await this.lockCellRows(transaction, [text(existing, 'cell_id'), input.cellId]) await this.removeActivityLease(transaction, identity, existing, now) - await this.adjustCellReservation(transaction, input.cellId, units) + await this.adjustCellReservationAtomically(transaction, input.cellId, units) } await this.adjustActivityCount(transaction, identity, input.kind, 1, expiresAt, now) await transaction.query( @@ -3580,8 +3582,7 @@ export class RelayAssignmentStore { ) await this.touchAssignment(transaction, identity, expiresAt, now) } else { - await this.lockCellInventory(transaction, 'request') - await this.adjustCellReservation(transaction, input.cellId, 1) + await this.adjustCellReservationAtomically(transaction, input.cellId, 1) await this.adjustActivityCount(transaction, identity, 'control', 1, expiresAt, now) await transaction.query( `INSERT INTO relay_assignment_activity_leases @@ -6954,6 +6955,19 @@ export class RelayAssignmentStore { return rows } + // Per-connection paths touch one or two cells. Locking exactly those rows, + // in the same ascending order the inventory lock uses (ORDER BY fixes the + // row-lock order), keeps them off the fleet-wide lock without a cycle. + private async lockCellRows(database: RelayDatabase, cellIds: string[]): Promise { + const distinct = [...new Set(cellIds)] + return await database.queryLocked( + `SELECT * FROM relay_cells WHERE cell_id IN (${distinct.map(() => '?').join(', ')}) + ORDER BY cell_id ASC`, + distinct, + { lockTimeoutMs: CELL_INVENTORY_LOCK_TIMEOUT_MS } + ) + } + private async lockGeneralCellInventory( database: RelayDatabase, mode: CellInventoryLockMode @@ -7590,7 +7604,10 @@ export class RelayAssignmentStore { ) { throw new Error('activity_lease_shape_mismatch') } - const cells = await this.lockCellInventory(database, 'request') + // Why: this recomputes one cell's reservation from its leases, so only that + // row needs to be held; the 23-row inventory lock here serialised every + // desktop control rebind in the fleet behind every other one. + const cellRow = (await this.lockCellRows(database, [cellId]))[0] await database.query( `DELETE FROM relay_assignment_activity_leases WHERE user_id = ? AND relay_host_id = ? AND activity_kind = 'control' @@ -7611,7 +7628,6 @@ export class RelayAssignmentStore { [cellId] ) )[0]! - const cellRow = cells.find((cell) => text(cell, 'cell_id') === cellId) const cellUnits = integer(cellUnitsRow, 'request_units') if (!cellRow) throw new Error('assigned_cell_missing') if (cellUnits > integer(cellRow, 'capacity_requests')) { diff --git a/cloud/apps/relay/src/cell-inventory-lock-census.test.ts b/cloud/apps/relay/src/cell-inventory-lock-census.test.ts index 8ca7cee55f5..a26e15f8e1d 100644 --- a/cloud/apps/relay/src/cell-inventory-lock-census.test.ts +++ b/cloud/apps/relay/src/cell-inventory-lock-census.test.ts @@ -25,10 +25,12 @@ const CENSUS: CensusEntry[] = [ { method: 'assignOnce', mode: 'nowait', reach: 'both' }, { method: 'assignOnce', mode: 'nowait', reach: 'both' }, { method: 'refreshDrainMigrationLeasesOnce', mode: 'request', reach: 'request' }, - // Reachable from neither: changeActivity has no production callers, only tests. - { method: 'changeActivity', mode: 'request', reach: 'orphan' }, - { method: 'acquireActivity', mode: 'request', reach: 'request' }, - { method: 'activateControl', mode: 'request', reach: 'request' }, + // changeActivity, acquireActivity, activateControl and + // removeSupersededSameCellControls no longer take the inventory: they lock + // only the one or two cell rows they touch, in cell_id order (lockCellRows), + // so they cannot cycle with placement's ordered inventory lock, and the + // 23-row lock there had serialised every reconnect in the fleet behind every + // other one. { method: 'startEvacuation', mode: 'request', reach: 'request' }, { method: 'completeEvacuationFromDeadSourceOnce', mode: 'request', reach: 'request' }, { method: 'completeEvacuationFromDeadSourceOnce', mode: 'nowait', reach: 'request' }, @@ -48,8 +50,31 @@ const CENSUS: CensusEntry[] = [ { method: 'releaseExpiredActivityLeases', mode: 'nowait', reach: 'sweep' }, { method: 'releaseExpiredActivity', mode: 'nowait', reach: 'sweep' }, { method: 'reconcileReservationAccounting', mode: 'pool-default', reach: 'both' }, - { method: 'leastLoadedCell', mode: 'pool-default', reach: 'both' }, - { method: 'removeSupersededSameCellControls', mode: 'request', reach: 'request' } + { method: 'leastLoadedCell', mode: 'pool-default', reach: 'both' } +] + +// Every inline `FROM relay_cells ... FOR UPDATE` outside the named lock helpers, +// in source order: whole-table locks in reconciliation and sticky placement, +// and single-row locks for a cell the method is already scoped to (heartbeat, +// fence, drain generation, configuration, or a reservation adjust that runs +// under a lock its caller already holds). A new inline lock fails the census +// below until it is listed here; per-connection paths that touch more than one +// cell go through lockCellRows so the order is fixed. +const NAMED_LOCK_HELPERS = ['lockCellInventory', 'lockGeneralCellInventory', 'lockCellRows'] + +const INLINE_CELL_LOCK_SITES = [ + 'reconcileCellsWithOptions', + 'assignStickyOnce', + 'recordCellHeartbeat', + 'attestCellFence', + 'adoptLegacyCellFence', + 'commitLegacyCellFenceAdoption', + 'prepareCellFenceAttempt', + 'attestCellFenceAttempt', + 'attestCellFenceAttempt', + 'configureCell', + 'assertDrainCellGeneration', + 'adjustCellReservation' ] // The background sweeps, and nothing else. A method reachable from one of these @@ -151,6 +176,42 @@ describe('cell inventory lock call-site census', () => { ) }) + // Why: the census only sees lockCellInventory calls, so a hand-written + // `relay_cells ... FOR UPDATE` would escape classification entirely. + it('routes every relay_cells row lock through a named lock helper', () => { + const lines = storeSource() + const rawSites: string[] = [] + // Whole statements, not a fixed window: a wide column list or a raw + // FOR UPDATE inside query() must not slip past. + const source = lines.join('\n') + const bounds: { name: string; start: number }[] = [] + lines.forEach((line, index) => { + const declaration = DECLARATION.exec(line) + if (declaration) bounds.push({ name: declaration[1]!, start: index }) + }) + const methodAt = (offset: number): string => { + const lineIndex = source.slice(0, offset).split('\n').length - 1 + let name = '' + for (const bound of bounds) if (bound.start <= lineIndex) name = bound.name + return name + } + const tick = String.fromCharCode(96) + const statementCall = new RegExp( + '\\.(queryLocked|query)\\(\\s*' + tick + '([^' + tick + ']*)' + tick, + 'g' + ) + for (const call of source.matchAll(statementCall)) { + const statement = call[2]! + if (!/\bFROM\s+relay_cells\b/.test(statement)) continue + const locks = call[1] === 'queryLocked' || /\bFOR\s+UPDATE\b/.test(statement) + if (!locks) continue + const method = methodAt(call.index) + if (NAMED_LOCK_HELPERS.includes(method)) continue + rawSites.push(method) + } + expect(rawSites).toEqual(INLINE_CELL_LOCK_SITES) + }) + it('leaves no call site taking the inventory without naming a mode', () => { const source = readFileSync(new URL('./assignment-store.ts', import.meta.url), 'utf8') const unclassified = source diff --git a/cloud/apps/relay/src/control-rebind-inventory-lock-postgres.test.ts b/cloud/apps/relay/src/control-rebind-inventory-lock-postgres.test.ts new file mode 100644 index 00000000000..e990ac1ed1a --- /dev/null +++ b/cloud/apps/relay/src/control-rebind-inventory-lock-postgres.test.ts @@ -0,0 +1,260 @@ +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { RelayAssignmentStore } from './assignment-store.js' +import { openRelayDatabase, type RelayDatabase } from './database.js' + +const databaseUrl = process.env.ORCA_RELAY_TEST_POSTGRES_URL +const describePostgres = databaseUrl ? describe : describe.skip + +// Three cells: the inventory lock covers more than the rows a move touches, and +// a high-to-low move exposes any lock taken out of cell_id order. +const cells = [ + { + id: 'rebind-inventory-postgres-a', + url: 'https://rebind-inventory-postgres-a.example.com', + capacityRequests: 1_000, + connectionHardCap: 600 as const, + connectionUnobservedBound: 50 + }, + { + id: 'rebind-inventory-postgres-b', + url: 'https://rebind-inventory-postgres-b.example.com', + capacityRequests: 1_000, + connectionHardCap: 600 as const, + connectionUnobservedBound: 50 + }, + { + id: 'rebind-inventory-postgres-c', + url: 'https://rebind-inventory-postgres-c.example.com', + capacityRequests: 1_000, + connectionHardCap: 600 as const, + connectionUnobservedBound: 50 + } +] +const identity = { userId: 'rebind-inventory-postgres-user', relayHostId: 'rebindinvhost001' } + +function heartbeat(cell: (typeof cells)[number]) { + return { + cellId: cell.id, + cellUrl: cell.url, + cellIncarnation: '11111111-1111-4111-8111-111111111111', + startedAt: 50, + ready: true, + observedRequests: 0, + totalConnections: 0, + inFlightConnections: 0, + reservedConnectionUnits: 0, + enforcedConnectionUnits: 0, + connectionInclusionWatermark: 1, + connectionHardCap: 600 as const, + connectionUnobservedBound: 50 + } +} + +// Why: every desktop control rebind used to take the fleet-wide relay_cells +// FOR UPDATE lock, so a rebind on one cell queued behind whatever held any +// other cell's row, until COMMIT (55P03 at the request bound). A rebind only +// touches its own cell row, so it must proceed while another cell's row is +// held elsewhere. +describePostgres('PostgreSQL control rebind under a held cell row', () => { + const databases: RelayDatabase[] = [] + + beforeAll(async () => { + databases.push( + await openRelayDatabase({ databaseUrl, dataDir: '' }), + await openRelayDatabase({ databaseUrl, dataDir: '' }) + ) + }) + + async function removeTestRows(database: RelayDatabase): Promise { + await database.query( + `DELETE FROM relay_control_connection_reservations WHERE user_id = ?`, + [identity.userId] + ) + for (const table of [ + 'relay_assignment_activity_leases', + 'relay_post_drain_migration_pins', + 'relay_assignment_migration_incarnations', + 'relay_assignment_migrations', + 'relay_assignments' + ]) { + await database.query(`DELETE FROM ${table} WHERE user_id = ?`, [identity.userId]) + } + for (const cell of cells) { + for (const table of [ + 'relay_cell_connection_snapshots', + 'relay_cell_connection_runtime', + 'relay_cell_connection_limits', + 'relay_cell_runtime', + 'relay_cells' + ]) { + await database.query(`DELETE FROM ${table} WHERE cell_id = ?`, [cell.id]) + } + } + } + + afterAll(async () => { + if (databases[0]) await removeTestRows(databases[0]) + for (const connection of databases) await connection.close() + }) + + it("rebinds and supersedes a control while another cell's row is held", async () => { + // A prior aborted run leaves connection snapshots that reject a replayed watermark. + await removeTestRows(databases[0]!) + const store = new RelayAssignmentStore(databases[0]!, () => 100) + await store.reconcileCells(cells) + for (const cell of cells) await store.recordCellHeartbeat(heartbeat(cell)) + // Pin the host to cell A so placement is deterministic. + await store.setCellEnabled(cells[1]!.id, false) + await store.setCellEnabled(cells[2]!.id, false) + const assignment = await store.assign(identity) + expect(assignment.cellId).toBe(cells[0]!.id) + await store.setCellEnabled(cells[1]!.id, true) + await store.setCellEnabled(cells[2]!.id, true) + await store.activateControl(identity, { + cellId: cells[0]!.id, + assignmentEpoch: assignment.assignmentEpoch, + generation: 1, + connectionInclusionWatermark: 10 + }) + + // Hold only cell B's row on a second connection, the way a rebind on B + // does, for longer than the request-path lock bound. + let releaseInventory!: () => void + const inventoryReleased = new Promise((resolve) => { + releaseInventory = resolve + }) + let inventoryHeld!: () => void + const inventoryHeldPromise = new Promise((resolve) => { + inventoryHeld = resolve + }) + const holder = databases[1]!.transaction(async (transaction) => { + await transaction.queryLocked(`SELECT * FROM relay_cells WHERE cell_id = ?`, [cells[1]!.id]) + inventoryHeld() + await inventoryReleased + }) + await inventoryHeldPromise + + // A generation-2 rebind on cell A supersedes generation 1. It must not + // wait on cell B's row. + const startedAt = Date.now() + const blockedStatement = async (): Promise => { + const rows = await databases[1]!.query( + `SELECT left(query, 160) AS q FROM pg_stat_activity + WHERE datname = current_database() AND wait_event_type = 'Lock'` + ) + return rows.map((row) => String(row.q)).join(' | ') + } + const timeout = new Promise((_, reject) => + setTimeout( + () => + void blockedStatement().then((statement) => + reject(new Error(`rebind on cell A blocked behind cell B's row: ${statement}`)) + ), + 2_000 + ) + ) + const rebound = await Promise.race([ + store.activateControl(identity, { + cellId: cells[0]!.id, + assignmentEpoch: assignment.assignmentEpoch, + generation: 2, + connectionInclusionWatermark: 11 + }), + timeout + ]) + const elapsedMs = Date.now() - startedAt + releaseInventory() + await holder + + expect(rebound).toBe(`control:${cells[0]!.id}:2`) + expect(elapsedMs).toBeLessThan(2_000) + const controls = await databases[0]!.query( + `SELECT activity_id FROM relay_assignment_activity_leases + WHERE user_id = ? AND activity_kind = 'control' ORDER BY activity_id`, + [identity.userId] + ) + expect(controls).toEqual([{ activity_id: `control:${cells[0]!.id}:2` }]) + const reserved = await databases[0]!.query( + `SELECT reserved_requests FROM relay_cells WHERE cell_id = ?`, + [cells[0]!.id] + ) + expect(Number(reserved[0]!.reserved_requests)).toBe(1) + }, 15_000) + + // Why: a phone's activity id is client-chosen and can follow the host across + // a migration, so acquireActivity may touch two cell rows. Moving from the + // higher cell to the lower one is where an unordered lock cycles with + // placement's ascending inventory lock (reproduced live before this fix). + it('moves an activity from a higher cell to a lower one in cell_id order', async () => { + await removeTestRows(databases[0]!) + const [cellA, cellB, cellC] = cells as [typeof cells[0], typeof cells[0], typeof cells[0]] + const store = new RelayAssignmentStore(databases[0]!, () => 100) + await store.reconcileCells(cells) + for (const cell of cells) await store.recordCellHeartbeat(heartbeat(cell)) + await store.setCellEnabled(cellA.id, false) + await store.setCellEnabled(cellB.id, false) + const assignment = await store.assign(identity) + expect(assignment.cellId).toBe(cellC.id) + await store.setCellEnabled(cellA.id, true) + await store.setCellEnabled(cellB.id, true) + const activityId = 'splice:rebind-inventory-postgres' + await store.acquireActivity(identity, { activityId, kind: 'splice', cellId: cellC.id }) + // The migration makes B authoritative; the lease still sits on C. + const migration = await store.startEvacuation(identity, cellB.id) + expect(migration.targetCellId).toBe(cellB.id) + + // Hold B elsewhere. An ordered move locks B first and queues here holding + // nothing else. Locking C first (the old lease's row, as an unordered move + // does) or the whole inventory (which takes A) shows up as a held row. + let releaseRow!: () => void + const rowReleased = new Promise((resolve) => { + releaseRow = resolve + }) + let rowHeld!: () => void + const rowHeldPromise = new Promise((resolve) => { + rowHeld = resolve + }) + const heldWhileMoverWaits: string[] = [] + const holder = databases[1]!.transaction(async (transaction) => { + await transaction.queryLocked(`SELECT * FROM relay_cells WHERE cell_id = ?`, [cellB.id]) + rowHeld() + await rowReleased + for (const cell of [cellA, cellC]) { + try { + await transaction.queryLocked(`SELECT * FROM relay_cells WHERE cell_id = ?`, [cell.id], { + failIfUnavailable: true + }) + } catch { + heldWhileMoverWaits.push(cell.id) + } + } + }) + await rowHeldPromise + const move = store.acquireActivity(identity, { activityId, kind: 'splice', cellId: cellB.id }) + let moved = false + void move.then(() => { + moved = true + }) + await new Promise((resolve) => setTimeout(resolve, 250)) + expect(moved).toBe(false) + releaseRow() + await holder + await move + expect(heldWhileMoverWaits).toEqual([]) + + const reservations = await databases[0]!.query( + `SELECT cell_id, reserved_requests FROM relay_cells + WHERE cell_id IN (?, ?, ?) ORDER BY cell_id ASC`, + [cellA.id, cellB.id, cellC.id] + ) + const reserved = reservations.map((row) => [String(row.cell_id), Number(row.reserved_requests)]) + expect(reserved).toEqual([ + [cellA.id, 0], + // Migration grant plus the moved splice, as in the SQLite origin-scoped + // reservation case: the lock change did not alter accounting. + [cellB.id, 6], + // The sticky grant stays on the source until the migration completes. + [cellC.id, 1] + ]) + }, 15_000) +}) diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index ff474f7d95e..8f5045b932a 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -603,7 +603,7 @@ index 7b4b9e1f990fbf95b51528bb56dc9717f5b87532..2ae787c5bd4f3eba470584dc658a01a5 } #endif diff --git a/src/win/conpty.cc b/src/win/conpty.cc -index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a6a4082ce 100644 +index 7b286d3d644c26141df516929703aa6e129df4b2..4aed260dd68e6a171dcfd349e9a7c5c97209248e 100644 --- a/src/win/conpty.cc +++ b/src/win/conpty.cc @@ -18,6 +18,7 @@ @@ -614,7 +614,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a #include #include #include -@@ -44,12 +45,29 @@ struct pty_baton { +@@ -44,12 +45,39 @@ struct pty_baton { HANDLE hOut; HPCON hpc; @@ -630,22 +630,32 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a + // refused to create or assign one (an outer job without breakaway rights), + // in which case callers fall back to their pre-job behaviour. + HANDLE hJob = nullptr; ++ ++ // Orca: teardown needs BOTH the shell's death and an explicit kill() before ++ // the baton can be freed, so each side records that it has run. Whichever ++ // arrives second frees it. Freeing on the shell's death alone -- what this ++ // file did before -- destroyed the only record of `hpc` while ++ // ClosePseudoConsole was still owed, which is why a self-exiting shell ++ // leaked its pseudoconsole and the console host it reaps (#18601 / F24). ++ bool shellExited = false; ++ bool consoleClosed = false; pty_baton(int _id, HANDLE _hIn, HANDLE _hOut, HPCON _hpc) : id(_id), hIn(_hIn), hOut(_hOut), hpc(_hpc) {}; }; static std::vector> ptyHandles; -+// Orca: guards the job accessors below against the exit watcher thread. It does -+// NOT make the whole table safe -- PtyResize/PtyClear/PtyKill read it unlocked, -+// as they always have -- but it closes the window this patch opened, where the -+// watcher can close hShell/hJob and free the baton between a lookup and its use. ++// Orca: guards the job accessors below, and PtyKill, against the exit watcher ++// thread. It does NOT make the whole table safe -- PtyResize and PtyClear still ++// read it unlocked, as they always have -- but it closes the window this patch ++// opened, where the watcher can close hShell/hJob and free the baton between a ++// lookup and its use. +// Handle VALUES are recycled aggressively, so an unguarded read could pass the +// shell-pid check against an unrelated process and terminate the wrong job. +static std::mutex ptyJobMutex; static volatile LONG ptyCounter; static pty_baton* get_pty_baton(int id) { -@@ -102,8 +120,27 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { +@@ -102,8 +130,31 @@ void SetupExitCallback(Napi::Env env, Napi::Function cb, pty_baton* baton) { // Get process exit code. GetExitCodeProcess(baton->hShell, (LPDWORD)(&exit_event->exit_code)); // Clean up handles @@ -665,9 +675,13 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a + // Why inside the lock: erasing frees the baton the job accessors hold a + // pointer to. Note remove_pty_baton must not be an assert() argument -- + // NDEBUG would compile the call away and leak every baton. -+ const bool removed = remove_pty_baton(baton->id); -+ assert(removed); -+ (void)removed; ++ baton->shellExited = true; ++ if (baton->consoleClosed) { ++ const bool removed = remove_pty_baton(baton->id); ++ assert(removed); ++ (void)removed; ++ } ++ // Else PtyKill has not run yet and still owns hpc. It frees the baton. + } + // Why the lock ends here: BlockingCall below waits on the JS thread, and the + // JS thread can be waiting on ptyJobMutex inside PtyTerminateJob. Holding @@ -675,7 +689,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a auto status = tsfn.BlockingCall(exit_event, callback); // In main thread switch (status) { -@@ -409,6 +446,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -409,6 +460,15 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "UpdateProcThreadAttribute failed"); } @@ -691,7 +705,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a PROCESS_INFORMATION piClient{}; fSuccess = !!CreateProcessW( nullptr, -@@ -416,7 +462,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -416,7 +476,10 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { nullptr, // lpProcessAttributes nullptr, // lpThreadAttributes false, // bInheritHandles VERY IMPORTANT that this is false @@ -703,7 +717,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a envArg, // lpEnvironment mutableCwd.get(), // lpCurrentDirectory &siEx.StartupInfo, // lpStartupInfo -@@ -426,8 +475,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -426,8 +489,47 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { throw errorWithCode(info, "Cannot create process"); } @@ -753,7 +767,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a if (useConptyDll && fLoadedDll) { PFNRELEASEPSEUDOCONSOLE const pfnReleasePseudoConsole = (PFNRELEASEPSEUDOCONSOLE)GetProcAddress( -@@ -440,6 +528,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { +@@ -440,6 +542,8 @@ static Napi::Value PtyConnect(const Napi::CallbackInfo& info) { // Update handle handle->hShell = piClient.hProcess; @@ -762,7 +776,91 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a // Close the thread handle to avoid resource leak CloseHandle(piClient.hThread); -@@ -567,6 +657,143 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { +@@ -544,29 +648,215 @@ static Napi::Value PtyKill(const Napi::CallbackInfo& info) { + int id = info[0].As().Int32Value(); + const bool useConptyDll = info[1].As().Value(); + +- const pty_baton* handle = get_pty_baton(id); ++ // Orca: resolve the DLL BEFORE touching any baton state, for the same reason ++ // PtyConnect does it before creating anything. LoadConptyDll throws when ++ // conpty.dll is missing, and a throw after consoleClosed was set would strand ++ // the pseudoconsole permanently: the retry would find the work already ++ // claimed and do nothing. Only the useConptyDll path can throw here; the ++ // other returns kernel32. ++ HANDLE hLibrary = LoadConptyDll(info, useConptyDll); ++ PFNCLOSEPSEUDOCONSOLE pfnClosePseudoConsole = nullptr; ++ if (hLibrary != nullptr) { ++ pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( ++ (HMODULE)hLibrary, ++ useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); ++ } + +- if (handle != nullptr) { +- HANDLE hLibrary = LoadConptyDll(info, useConptyDll); +- bool fLoadedDll = hLibrary != nullptr; +- if (fLoadedDll) +- { +- PFNCLOSEPSEUDOCONSOLE const pfnClosePseudoConsole = (PFNCLOSEPSEUDOCONSOLE)GetProcAddress( +- (HMODULE)hLibrary, +- useConptyDll ? "ConptyClosePseudoConsole" : "ClosePseudoConsole"); +- if (pfnClosePseudoConsole) +- { +- pfnClosePseudoConsole(handle->hpc); ++ // Orca: the baton now outlives the shell, so this runs on a self-exited pty ++ // too -- that is the whole point. Take what we need under the lock: the ++ // watcher thread nulls hShell the moment the shell dies, and TerminateProcess ++ // on a handle it just closed is an invalid-handle operation. Duplicating ++ // rather than reordering keeps upstream's close-then-terminate sequence. ++ HPCON hpc = nullptr; ++ HANDLE hShellDup = nullptr; ++ bool owed = false; ++ { ++ std::lock_guard guard(ptyJobMutex); ++ pty_baton* handle = get_pty_baton(id); ++ // Why the consoleClosed check: a second kill() would otherwise close the ++ // same pseudoconsole twice. Upstream relied on the baton being gone. ++ if (handle != nullptr && !handle->consoleClosed) { ++ hpc = handle->hpc; ++ owed = true; ++ handle->consoleClosed = true; ++ // Null hShell means a self-exited pty, where there is nothing to kill. ++ if (useConptyDll && handle->hShell != nullptr) { ++ if (!DuplicateHandle(GetCurrentProcess(), handle->hShell, GetCurrentProcess(), ++ &hShellDup, 0, FALSE, DUPLICATE_SAME_ACCESS)) { ++ // Why terminate here instead of skipping: a failed duplication leaves ++ // hShellDup null, which is indistinguishable from the self-exit case, ++ // and skipping would leave the shell RUNNING after its pane closed -- ++ // a worse outcome than the leak this all exists to fix. hShell is ++ // valid under this lock and TerminateProcess does not block, so the ++ // only cost is that this rare path kills before the console closes. ++ hShellDup = nullptr; ++ TerminateProcess(handle->hShell, 1); ++ } ++ } ++ if (handle->shellExited) { ++ const bool removed = remove_pty_baton(id); ++ assert(removed); ++ (void)removed; + } ++ // Else the shell is still running and the watcher frees the baton. + } +- if (useConptyDll) { +- TerminateProcess(handle->hShell, 1); ++ } ++ ++ // Why outside the lock: ClosePseudoConsole blocks until the conout side has ++ // drained, and the watcher must be able to take the lock while it does. ++ if (owed) { ++ if (pfnClosePseudoConsole) ++ { ++ pfnClosePseudoConsole(hpc); ++ } ++ if (hShellDup != nullptr) { ++ TerminateProcess(hShellDup, 1); ++ CloseHandle(hShellDup); + } + } + return env.Undefined(); } @@ -808,9 +906,11 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a + * Orca: the pids still alive in this pty's tree, straight from the kernel. + * + * Descendant liveness for a tree that is still tracked, including children that -+ * detached from the console. Once the shell exits the baton is gone, so this -+ * returns null rather than an empty list -- null means "no answer", never -+ * "they died". Also returns null when no job was assigned. ++ * detached from the console. Once the shell exits the watcher nulls hJob, which ++ * ownsShell rejects, so this returns null rather than an empty list -- null ++ * means "no answer", never "they died". (The baton itself now outlives the ++ * shell, until kill() runs; hJob is what makes the answer null.) Also returns ++ * null when no job was assigned. + * + * Does not include the ConPTY console host: CreatePseudoConsole spawns it + * before this job exists, so it is not a member and ClosePseudoConsole is what @@ -906,7 +1006,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a /** * Init */ -@@ -577,6 +804,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { +@@ -577,6 +867,9 @@ Napi::Object init(Napi::Env env, Napi::Object exports) { exports.Set("resize", Napi::Function::New(env, PtyResize)); exports.Set("clear", Napi::Function::New(env, PtyClear)); exports.Set("kill", Napi::Function::New(env, PtyKill)); @@ -917,7 +1017,7 @@ index 7b286d3d644c26141df516929703aa6e129df4b2..ec6bf3932c65b89c013ff133dc6bf46a }; diff --git a/lib/windowsPtyAgent.js b/lib/windowsPtyAgent.js -index a358ffb..fb3a96f 100644 +index a358ffb177357e177661033c1b092f9c9d0e5f5a..26c2a4c58799ce649f5113131e4c52f7ed2d87ad 100644 --- a/lib/windowsPtyAgent.js +++ b/lib/windowsPtyAgent.js @@ -136,6 +136,9 @@ var WindowsPtyAgent = /** @class */ (function () { @@ -930,6 +1030,20 @@ index a358ffb..fb3a96f 100644 this._outSocket.readable = false; this._getConsoleProcessList().then(function (consoleProcessList) { consoleProcessList.forEach(function (pid) { +@@ -154,9 +157,10 @@ var WindowsPtyAgent = /** @class */ (function () { + // Close the input write handle to signal the end of session. + this._inSocket.destroy(); + this._ptyNative.kill(this._pty, this._useConptyDll); +- this._outSocket.on('data', function () { +- _this._conoutSocketWorker.dispose(); +- }); ++ // Orca: dispose unconditionally, as the non-DLL branch above does. ++ // Waiting for another 'data' event leaks the conout worker on every ++ // self-exiting shell, because no more data ever arrives (F24). ++ this._conoutSocketWorker.dispose(); + } + } + else { diff --git a/lib/windowsTerminal.js b/lib/windowsTerminal.js index 3c38f89..e20b3e6 100644 --- a/lib/windowsTerminal.js @@ -1015,7 +1129,7 @@ index 3c38f89..e20b3e6 100644 \ No newline at end of file +//# sourceMappingURL=windowsTerminal.js.map diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts -index d705444..ce611b8 100644 +index d7054449516f0c9a62af351c2caa17331206d530..0c28a32e2e1db2b3f208ddde8443cd4e67bb1ad6 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -143,6 +143,9 @@ export class WindowsPtyAgent { @@ -1028,6 +1142,20 @@ index d705444..ce611b8 100644 this._outSocket.readable = false; this._getConsoleProcessList().then(consoleProcessList => { consoleProcessList.forEach((pid: number) => { +@@ -159,9 +162,10 @@ export class WindowsPtyAgent { + // Close the input write handle to signal the end of session. + this._inSocket.destroy(); + (this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll); +- this._outSocket.on('data', () => { +- this._conoutSocketWorker.dispose(); +- }); ++ // Orca: dispose unconditionally, as the non-DLL branch above does. ++ // Waiting for another 'data' event leaks the conout worker on every ++ // self-exiting shell, because no more data ever arrives (F24). ++ this._conoutSocketWorker.dispose(); + } + } else { + // Because pty.kill closes the handle, it will kill most processes by itself. diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts index 13f6c6d..eda63c8 100644 --- a/src/windowsTerminal.ts diff --git a/config/relay-assets/node-pty-1.1.0-windows-pty-teardown-patch.cjs b/config/relay-assets/node-pty-1.1.0-windows-pty-teardown-patch.cjs new file mode 100644 index 00000000000..dd26784ee46 --- /dev/null +++ b/config/relay-assets/node-pty-1.1.0-windows-pty-teardown-patch.cjs @@ -0,0 +1,158 @@ +const { createHash } = require('node:crypto') +const { readFileSync, renameSync, rmSync, writeFileSync } = require('node:fs') +const { join, resolve } = require('node:path') + +/** + * Release the ConPTY teardown handles a relay's npm-installed node-pty never releases. + * + * Two files, and the ORDER of one of the edits is the whole fix. + * + * `windowsPtyAgent.js` -- `kill()` flips `readable` on both sockets and destroys neither. + * `_cleanUpProcess` destroys `_outSocket`, so the conout handle comes back; nothing ever destroys + * `_inSocket`, and it wraps a real Windows named-pipe handle from `fs.openSync(term.conin, 'w')`. + * Every terminal leaks one File handle for the life of the host process. + * + * The obvious fix -- and the one the desktop patch ships -- releases it at the TOP of the branch, + * before `_getConsoleProcessList()` forks and before the native kill. That is measurably worse than + * leaving the leak alone: teardown aborts partway, the forked console-list agent is never reaped, + * and both pipe handles stay alive instead of one. This asset releases it at the END of the branch + * instead, after the fork and the kill have already happened. + * + * Measured on a Windows SSH host, 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 <-- 3x WORSE + * released last (here) File flat, Process flat + * + * `windowsTerminal.js` carries the desktop's error-listener hunks verbatim. The conin listener is + * what keeps a pipe error retiring one terminal instead of the host -- its own comment names the + * failure mode: "Without a listener, Node promotes errors such as write EAGAIN to uncaughtException". + * It is not what fixes the leak (adding it changed nothing on its own), but it is the guard that + * makes destroying conin safe at all. + * + * Why this ships as a relay asset rather than only in config/patches/node-pty@1.1.0.patch: pnpm + * patches do not cross the SSH boundary -- a relay host runs the tree `npm install` put there. + * + * DELIBERATE DIVERGENCE FROM THE DESKTOP: the desktop patch has the early placement and therefore + * the +2 File / +1 Process regression, measured against its exact installed tree. Correcting it + * there is a separate change with its own verification, so the two trees differ on this one hunk on + * purpose, and the test pins that so a future "sync the patches" does not copy the bug back. + * + * NOT ADDRESSED, AND A SEPARATE DEFECT THAT IS STILL OPEN: a terminal that exits on its own is + * still torn down through `kill()` -- both hosts call `destroy()` on natural exit and + * `WindowsTerminal.destroy()` is `kill()` -- but the shell is already gone by then, and the + * ordering this patch relies on does not hold. Measured over 20 self-exit cycles with that + * `destroy()` issued: published +3 File/+1 Process per terminal, desktop-patched +2/+1, this tree + * +2/+1. So this patch does not close it and the desktop patch does not either. It is reachable + * for every Windows user, local and relay, on every terminal closed by typing `exit`. + */ + +const EXPECTED_NODE_PTY_VERSION = '1.1.0' + +/** Each entry is one published file, its patched form, and the edits between them. */ +const PATCH_TARGETS = [ + { + relativePath: ['lib', 'windowsPtyAgent.js'], + originalSha256: '8636d16b38266112204061a22b135734177c242837982fd3a4055be726efa64a', + patchedSha256: '1e23ef480569e73706e3ab4f5482c7e553c76f51414ae8e7b0bdcc2fd75f7280', + replacements: [ + [ + ' this._ptyNative.kill(this._pty, this._useConptyDll);\n this._conoutSocketWorker.dispose();\n', + ' this._ptyNative.kill(this._pty, this._useConptyDll);\n this._conoutSocketWorker.dispose();\n // Orca: released AFTER the console-list fork and the native kill, not before them.\n // Destroying conin first aborts teardown partway -- measured on a Windows SSH relay\n // as +2 File and +1 Process handles per terminal, against +1 File unpatched.\n this._inSocket.destroy();\n' + ] + ] + }, + { + relativePath: ['lib', 'windowsTerminal.js'], + originalSha256: 'c3a65716f53fed0135a8a633373d5f9c2ab092544d651f27ef0a67096dd3bcd9', + patchedSha256: '8247ecd69be8b18257050fb026b290024612c5ffc6d492ff1d46f81e613be2cf', + replacements: [ + [ + ' _this._agent = new windowsPtyAgent_1.WindowsPtyAgent(file, args, parsedEnv, cwd, _this._cols, _this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor);\n _this._socket = _this._agent.outSocket;\n // Not available until `ready` event emitted.\n _this._pid = _this._agent.innerPid;', + " _this._agent = new windowsPtyAgent_1.WindowsPtyAgent(file, args, parsedEnv, cwd, _this._cols, _this._rows, false, opt.useConpty, opt.useConptyDll, opt.conptyInheritCursor);\n _this._socket = _this._agent.outSocket;\n // Attach before readiness so a broken ConPTY output pipe cannot be unhandled.\n _this._socket.on('error', function (err) {\n var code = err && err.code;\n // PTY output can report EPIPE before `_close()` wins the race.\n _this._close();\n if (code === 'EPIPE' || code === 'ERR_STREAM_PUSH_AFTER_EOF' || code === 'ERR_STREAM_DESTROYED') {\n return;\n }\n // EIO, happens when someone closes our child process: the only process\n // in the terminal.\n // node < 0.6.14: errno 5\n // node >= 0.6.14: read EIO\n if (typeof code === 'string') {\n if (~code.indexOf('errno 5') || ~code.indexOf('EIO'))\n return;\n }\n // Throw anything else.\n if (_this.listeners('error').length < 2) {\n throw err;\n }\n });\n // Not available until `ready` event emitted.\n _this._pid = _this._agent.innerPid;" + ], + [ + " }\n });\n // Shutdown if `error` event is emitted.\n _this._socket.on('error', function (err) {\n // Close terminal session.\n _this._close();\n // EIO, happens when someone closes our child process: the only process\n // in the terminal.\n // node < 0.6.14: errno 5\n // node >= 0.6.14: read EIO\n if (err.code) {\n if (~err.code.indexOf('errno 5') || ~err.code.indexOf('EIO'))\n return;\n }\n // Throw anything else.\n if (_this.listeners('error').length < 2) {\n throw err;\n }\n });\n // Cleanup after the socket is closed.\n _this._socket.on('close', function () {", + " }\n });\n // Cleanup after the socket is closed.\n _this._socket.on('close', function () {" + ], + [ + ' _this._readable = true;\n _this._writable = true;\n _this._forwardEvents();\n return _this;', + " _this._readable = true;\n _this._writable = true;\n // A ConPTY input-pipe error must retire only this terminal. Without a listener, Node promotes\n // errors such as write EAGAIN to uncaughtException and kills every PTY in the daemon.\n _this._agent.inSocket.on('error', function () {\n if (!_this._writable) {\n return;\n }\n _this._close();\n try {\n _this._agent.kill();\n }\n catch (_a) {\n // The failing pipe may have raced process exit; the terminal is already unwritable.\n }\n });\n _this._forwardEvents();\n return _this;" + ], + [ + 'exports.WindowsTerminal = WindowsTerminal;\n//# sourceMappingURL=windowsTerminal.js.map', + 'exports.WindowsTerminal = WindowsTerminal;\n//# sourceMappingURL=windowsTerminal.js.map\n' + ] + ] + } +] + +function inspectTarget(relayDir, target) { + const nodePtyDir = resolve(relayDir, 'node_modules', 'node-pty') + const packageJson = JSON.parse(readFileSync(join(nodePtyDir, 'package.json'), 'utf8')) + if (packageJson.version !== EXPECTED_NODE_PTY_VERSION) { + throw new Error( + `Refusing to patch node-pty ${packageJson.version}; expected ${EXPECTED_NODE_PTY_VERSION}` + ) + } + const filePath = join(nodePtyDir, ...target.relativePath) + return { filePath, source: readFileSync(filePath, 'utf8') } +} + +function assertPatchedNodePtyWindowsTeardown(relayDir = process.cwd()) { + for (const target of PATCH_TARGETS) { + const inspected = inspectTarget(relayDir, target) + if (sourceSha256(inspected.source) !== target.patchedSha256) { + throw new Error( + `node-pty ConPTY teardown release is not installed in ${target.relativePath.join('/')}` + ) + } + } +} + +function patchNodePtyWindowsTeardown(relayDir = process.cwd()) { + for (const target of PATCH_TARGETS) { + const inspected = inspectTarget(relayDir, target) + const sourceHash = sourceSha256(inspected.source) + if (sourceHash === target.patchedSha256) { + continue + } + if (sourceHash !== target.originalSha256) { + throw new Error( + `Refusing to patch unexpected node-pty source in ${target.relativePath.join('/')}` + ) + } + let patchedSource = inspected.source + for (const [from, to] of target.replacements) { + // Why the count check: an anchor that matched twice would patch the wrong site silently, and + // the hash below would then reject a tree this script had already rewritten. + if (patchedSource.split(from).length - 1 !== 1) { + throw new Error(`Refusing to patch ${target.relativePath.join('/')}; anchor is not unique`) + } + patchedSource = patchedSource.replace(from, to) + } + const temporaryPath = `${inspected.filePath}.orca-patch-${process.pid}` + // Why: a terminated remote install must leave either known source version recoverable on reconnect. + try { + writeFileSync(temporaryPath, patchedSource) + renameSync(temporaryPath, inspected.filePath) + } finally { + rmSync(temporaryPath, { force: true }) + } + } + assertPatchedNodePtyWindowsTeardown(relayDir) +} + +function sourceSha256(source) { + return createHash('sha256').update(source).digest('hex') +} + +if (require.main === module) { + patchNodePtyWindowsTeardown() +} + +module.exports = { + assertPatchedNodePtyWindowsTeardown, + patchNodePtyWindowsTeardown +} diff --git a/config/scripts/build-relay.mjs b/config/scripts/build-relay.mjs index 289c7a957bd..4d408712f97 100644 --- a/config/scripts/build-relay.mjs +++ b/config/scripts/build-relay.mjs @@ -57,6 +57,13 @@ const NODE_PTY_CONSOLE_LIST_PATCH_SOURCE = join( 'relay-assets', NODE_PTY_CONSOLE_LIST_PATCH_FILENAME ) +const NODE_PTY_WINDOWS_TEARDOWN_PATCH_FILENAME = 'node-pty-1.1.0-windows-pty-teardown-patch.cjs' +const NODE_PTY_WINDOWS_TEARDOWN_PATCH_SOURCE = join( + ROOT, + 'config', + 'relay-assets', + NODE_PTY_WINDOWS_TEARDOWN_PATCH_FILENAME +) const NODE_PTY_MASTER_CLOEXEC_PATCH_FILENAME = 'node-pty-1.1.0-master-cloexec-patch.cjs' const NODE_PTY_MASTER_CLOEXEC_PATCH_SOURCE = join( ROOT, @@ -132,6 +139,10 @@ for (const platform of RELAY_BUILD_PLATFORMS) { NODE_PTY_CONSOLE_LIST_PATCH_SOURCE, join(outDir, NODE_PTY_CONSOLE_LIST_PATCH_FILENAME) ) + copyFileSync( + NODE_PTY_WINDOWS_TEARDOWN_PATCH_SOURCE, + join(outDir, NODE_PTY_WINDOWS_TEARDOWN_PATCH_FILENAME) + ) } copyFileSync( NODE_PTY_MASTER_CLOEXEC_PATCH_SOURCE, diff --git a/config/scripts/node-pty-windows-pty-teardown-patch.test.mjs b/config/scripts/node-pty-windows-pty-teardown-patch.test.mjs new file mode 100644 index 00000000000..64fb1b056b8 --- /dev/null +++ b/config/scripts/node-pty-windows-pty-teardown-patch.test.mjs @@ -0,0 +1,213 @@ +// The relay's copy of the ConPTY teardown release, and the guard that keeps it in lockstep with the +// desktop's own node-pty patch. pnpm patches do not cross the SSH boundary, so a relay runs the tree +// `npm install` put there; the desktop had this fix and the relay did not, and every terminal on a +// Windows SSH host leaked one File handle for the life of the relay process. +// +// The ORDER of the conin release is the fix. Releasing it at the top of the branch -- what the +// desktop patch does -- was measured at 3x WORSE than shipping nothing (File +2/terminal and a new +// Process +1/terminal); releasing it after the console-list fork and the native kill is flat. +import { createRequire } from 'node:module' +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' + +const require = createRequire(import.meta.url) +const { + assertPatchedNodePtyWindowsTeardown, + patchNodePtyWindowsTeardown +} = require('../relay-assets/node-pty-1.1.0-windows-pty-teardown-patch.cjs') +const projectDir = resolve(import.meta.dirname, '..', '..') +const cleanupDirs = [] + +const PATCHED_FILES = ['windowsPtyAgent.js', 'windowsTerminal.js'] + +/** The hunks config/patches/node-pty@1.1.0.patch adds to the installed desktop tree. */ +const DESKTOP_HUNKS = { + 'windowsPtyAgent.js': [ + [ + [ + ' this._inSocket.readable = false;', + ' // The non-DLL path previously only flipped `readable`, leaving the', + ' // conin PipeWrap alive until the host exited (#947).', + ' this._inSocket.destroy();', + ' this._outSocket.readable = false;', + '' + ].join('\n'), + [ + ' this._inSocket.readable = false;', + ' this._outSocket.readable = false;', + '' + ].join('\n') + ], + // The useConptyDll branch, which only the DESKTOP runs -- the relay takes the + // non-DLL branch above, where the dispose is already unconditional. Listed here + // so un-applying still yields published; the relay asset needs no counterpart. + [ + [ + ' // Orca: dispose unconditionally, as the non-DLL branch above does.', + " // Waiting for another 'data' event leaks the conout worker on every", + ' // self-exiting shell, because no more data ever arrives (F24).', + ' this._conoutSocketWorker.dispose();', + '' + ].join('\n'), + [ + " this._outSocket.on('data', function () {", + ' _this._conoutSocketWorker.dispose();', + ' });', + '' + ].join('\n') + ] + ], + 'windowsTerminal.js': [ + [ + ' // Attach before readiness so a broken ConPTY output pipe cannot be unhandled.', + null + ], + [' // A ConPTY input-pipe error must retire only this terminal.', null] + ] +} + +function desktopPath(file) { + return join(projectDir, 'node_modules', 'node-pty', 'lib', file) +} + +afterEach(() => { + for (const dir of cleanupDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }) + } +}) + +describe('Windows SSH relay node-pty ConPTY teardown patch', () => { + // Why reconstruct rather than vendor upstream: the installed tree IS the published file plus the + // desktop's hunks, so un-applying them yields upstream exactly -- and pinning that against this + // asset's own hashes is what fails loudly if either side of the pair moves. + it('takes the desktop error listeners verbatim', () => { + const fixture = writeNodePtyFixture('1.1.0') + patchNodePtyWindowsTeardown(fixture.root) + + expect(readFileSync(join(fixture.libDir, 'windowsTerminal.js'), 'utf8')).toBe( + readFileSync(desktopPath('windowsTerminal.js'), 'utf8') + ) + }) + + // The one hunk that must NOT match the desktop, and the reason is measured, not stylistic: + // releasing conin before `_getConsoleProcessList()` forks aborts teardown partway. + it('releases conin after the console-list fork, not before it like the desktop patch', () => { + const fixture = writeNodePtyFixture('1.1.0') + patchNodePtyWindowsTeardown(fixture.root) + const patched = readFileSync(join(fixture.libDir, 'windowsPtyAgent.js'), 'utf8') + + const branch = patched.slice( + patched.indexOf('if (!this._useConptyDll) {'), + patched.indexOf('else {', patched.indexOf('if (!this._useConptyDll) {')) + ) + expect(branch).toContain('this._inSocket.destroy();') + expect(branch.indexOf('this._inSocket.destroy();')).toBeGreaterThan( + branch.indexOf('this._conoutSocketWorker.dispose();') + ) + expect(branch.indexOf('this._inSocket.destroy();')).toBeGreaterThan( + branch.indexOf('this._getConsoleProcessList()') + ) + // Pinned so a future "sync the relay asset to config/patches" cannot copy the regression back. + expect(patched).not.toBe(readFileSync(desktopPath('windowsPtyAgent.js'), 'utf8')) + }) + + it('installs and verifies idempotently', () => { + const fixture = writeNodePtyFixture('1.1.0') + + patchNodePtyWindowsTeardown(fixture.root) + const once = PATCHED_FILES.map((file) => readFileSync(join(fixture.libDir, file), 'utf8')) + for (const file of PATCHED_FILES) { + expect(existsSync(`${join(fixture.libDir, file)}.orca-patch-${process.pid}`)).toBe(false) + } + expect(() => assertPatchedNodePtyWindowsTeardown(fixture.root)).not.toThrow() + + patchNodePtyWindowsTeardown(fixture.root) + expect(PATCHED_FILES.map((file) => readFileSync(join(fixture.libDir, file), 'utf8'))).toEqual( + once + ) + }) + + it('refuses a different package version or unexpected source', () => { + const wrongVersion = writeNodePtyFixture('1.2.0-beta.11') + expect(() => patchNodePtyWindowsTeardown(wrongVersion.root)).toThrow('expected 1.1.0') + + for (const file of PATCHED_FILES) { + const drifted = writeNodePtyFixture('1.1.0') + const path = join(drifted.libDir, file) + writeFileSync(path, `${readFileSync(path, 'utf8')}\n// drift`) + expect(() => patchNodePtyWindowsTeardown(drifted.root)).toThrow('unexpected node-pty') + } + }) + + it('refuses a half-applied tree, so one file cannot pass for both', () => { + for (const file of PATCHED_FILES) { + const partial = writeNodePtyFixture('1.1.0') + const fixture = writeNodePtyFixture('1.1.0') + patchNodePtyWindowsTeardown(fixture.root) + writeFileSync(join(partial.libDir, file), readFileSync(join(fixture.libDir, file), 'utf8')) + expect(() => assertPatchedNodePtyWindowsTeardown(partial.root)).toThrow('is not installed') + } + }) +}) + +/** A published node-pty tree, rebuilt by un-applying the desktop hunks from the installed one. */ +function writeNodePtyFixture(version) { + const root = mkdtempSync(join(projectDir, '.node-pty-teardown-patch-test-')) + cleanupDirs.push(root) + const libDir = join(root, 'node_modules', 'node-pty', 'lib') + mkdirSync(libDir, { recursive: true }) + writeFileSync(join(root, 'node_modules', 'node-pty', 'package.json'), JSON.stringify({ version })) + for (const file of PATCHED_FILES) { + const desktop = readFileSync(desktopPath(file), 'utf8') + for (const [marker] of DESKTOP_HUNKS[file]) { + expect(desktop).toContain(marker) + } + writeFileSync(join(libDir, file), unapplyDesktopHunks(file, desktop)) + } + return { root, libDir } +} + +/** + * Reverse of the published-to-desktop transform. + * + * `windowsTerminal.js` is taken verbatim from the desktop, so the asset's own replacement table is + * the transform and reversing it is exact. `windowsPtyAgent.js` deliberately diverges, so its + * published form is rebuilt from the desktop hunk instead -- which is also what makes this file the + * place that notices if the desktop hunk itself ever moves. + */ +function unapplyDesktopHunks(file, desktop) { + if (file === 'windowsPtyAgent.js') { + let published = desktop + for (const [patched, original] of DESKTOP_HUNKS[file]) { + expect(published.split(patched).length - 1).toBe(1) + published = published.replace(patched, original) + } + return published + } + const asset = readFileSync( + join(projectDir, 'config', 'relay-assets', 'node-pty-1.1.0-windows-pty-teardown-patch.cjs'), + 'utf8' + ) + const { PATCH_TARGETS } = loadPatchTargets(asset) + const target = PATCH_TARGETS.find((entry) => entry.relativePath.at(-1) === file) + expect(target).toBeDefined() + let published = desktop + for (const [from, to] of target.replacements.toReversed()) { + expect(published.split(to).length - 1).toBe(1) + published = published.replace(to, from) + } + return published +} + +function loadPatchTargets(assetSource) { + const module = { exports: {} } + const factory = new Function( + 'module', + 'exports', + 'require', + `${assetSource}\nmodule.exports.PATCH_TARGETS = PATCH_TARGETS` + ) + factory(module, module.exports, require) + return module.exports +} diff --git a/config/scripts/skill-description-length.test.mjs b/config/scripts/skill-description-length.test.mjs index c0b4a8296f6..e7a9db79541 100644 --- a/config/scripts/skill-description-length.test.mjs +++ b/config/scripts/skill-description-length.test.mjs @@ -1,36 +1,39 @@ -import { readFileSync, readdirSync } from 'node:fs' +import { readdirSync, readFileSync } from 'node:fs' import { join, resolve } from 'node:path' -import { parse } from 'yaml' import { describe, expect, it } from 'vitest' +import { parse } from 'yaml' -// Why: the Agent Skills spec caps `description` at 1,024 characters after YAML folding, and -// conforming installers reject a skill over it (#17935). Every skills/*/SKILL.md must fit. -const SKILLS_ROOT = resolve(import.meta.dirname, '../../skills') +const skillsDir = resolve(import.meta.dirname, '../../skills') +// Why: the Agent Skills spec caps `description` at 1024 chars and conforming installers +// reject the whole skill (#17935); the frontmatter is what the installer parses, so check it. const MAX_DESCRIPTION_LENGTH = 1024 -function readDescription(skillText) { - const frontmatter = /^---\n([\s\S]*?)\n---\n/u.exec(skillText)?.[1] ?? '' - return parse(frontmatter)?.description ?? '' +function readDescription(skillName) { + const skillMarkdown = readFileSync(join(skillsDir, skillName, 'SKILL.md'), 'utf8') + const frontmatter = /^---\r?\n([\s\S]*?)\r?\n---\r?\n/u.exec(skillMarkdown)?.[1] + + expect(frontmatter, `${skillName}: missing frontmatter`).toBeDefined() + + return parse(frontmatter ?? '').description } describe('bundled skill descriptions', () => { - it('stay within the Agent Skills 1024-character limit', () => { - const skills = readdirSync(SKILLS_ROOT, { withFileTypes: true }).filter((entry) => - entry.isDirectory() - ) - expect(skills.length).toBeGreaterThan(0) + const skillNames = readdirSync(skillsDir, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) - const violations = skills.flatMap((entry) => { - const description = readDescription( - readFileSync(join(SKILLS_ROOT, entry.name, 'SKILL.md'), 'utf8') - ) - if (!description.trim()) { - return [`${entry.name}: missing description`] - } - return description.length > MAX_DESCRIPTION_LENGTH - ? [`${entry.name}: ${description.length} chars (limit ${MAX_DESCRIPTION_LENGTH})`] - : [] - }) - expect(violations).toEqual([]) + it('discovers the bundled skills', () => { + expect(skillNames).toContain('orchestration') + }) + + it.each(skillNames)('%s keeps description within the Agent Skills spec limit', (name) => { + const description = readDescription(name) + + expect(typeof description, `${name}: description must be a string`).toBe('string') + expect(description.trim().length, `${name}: description is empty`).toBeGreaterThan(0) + expect( + description.length, + `${name}: description is ${description.length} chars` + ).toBeLessThanOrEqual(MAX_DESCRIPTION_LENGTH) }) }) diff --git a/docs/assets/readme-downloads.svg b/docs/assets/readme-downloads.svg index ef8ebb61bb4..c240c965fee 100644 --- a/docs/assets/readme-downloads.svg +++ b/docs/assets/readme-downloads.svg @@ -1,5 +1,5 @@ - - downloads: 38m + + downloads: 39m @@ -15,7 +15,7 @@ downloads downloads - 38m - 38m + 39m + 39m diff --git a/docs/assets/wechat-qr-group9.jpg b/docs/assets/wechat-qr-group9.jpg new file mode 100644 index 00000000000..2bf46a28c3d Binary files /dev/null and b/docs/assets/wechat-qr-group9.jpg differ diff --git a/docs/readme/README.fr.md b/docs/readme/README.fr.md index 97c78d4e713..e601abc2344 100644 --- a/docs/readme/README.fr.md +++ b/docs/readme/README.fr.md @@ -243,9 +243,9 @@ Associez-la à l'app de bureau pour surveiller et piloter vos agents depuis votr - **Discord :** Rejoignez la communauté sur **[Discord](https://discord.gg/fzjDKHxv8Q)**. - **Twitter / X :** Suivez **[@orca_build](https://x.com/orca_build)** pour les news et annonces. -- **WeChat :** Scannez pour rejoindre le groupe WeChat 8 de la communauté Orca. +- **WeChat :** Scannez pour rejoindre le groupe WeChat 8 de la communauté Orca. Le groupe 8 est peut-être complet ; dans ce cas, scannez plutôt le QR code du groupe 9. - QR code WeChat groupe 8 de la communauté Orca + QR code WeChat groupe 8 de la communauté Orca  QR code WeChat groupe 9 de la communauté Orca - **Feedback & idées :** On ship vite. Il manque quelque chose ? [Demandez une feature](https://github.com/stablyai/orca/issues). - **Confidentialité :** Voir la [doc confidentialité & télémétrie](https://www.onorca.dev/docs/telemetry) pour ce qu'Orca collecte en anonyme et comment désactiver la télémétrie. diff --git a/docs/readme/README.ko.md b/docs/readme/README.ko.md index 4a75722ff8c..837ecf2133f 100644 --- a/docs/readme/README.ko.md +++ b/docs/readme/README.ko.md @@ -238,9 +238,9 @@ yay -S stably-orca-bin - **Discord:** **[Discord](https://discord.gg/fzjDKHxv8Q)** 커뮤니티에 참여하세요. - **Twitter / X:** 업데이트와 공지는 **[@orca_build](https://x.com/orca_build)** 를 팔로우하세요. -- **WeChat:** QR 코드를 스캔해 Orca 커뮤니티 WeChat 그룹 8에 참여하세요. +- **WeChat:** QR 코드를 스캔해 Orca 커뮤니티 WeChat 그룹 8에 참여하세요. 그룹 8이 가득 찼을 수 있으니, 그런 경우 그룹 9 QR 코드를 스캔하세요. - Orca 커뮤니티 WeChat 그룹 8 QR 코드 + Orca 커뮤니티 WeChat 그룹 8 QR 코드  Orca 커뮤니티 WeChat 그룹 9 QR 코드 - **피드백과 아이디어:** 우리는 빠르게 출시합니다. 필요한 기능이 있나요? [새 기능을 요청](https://github.com/stablyai/orca/issues)하세요. - **개인정보 보호:** Orca가 수집하는 익명 사용 데이터와 수집 거부 방법은 [개인정보 및 텔레메트리 문서](https://www.onorca.dev/docs/telemetry)를 참고하세요. diff --git a/docs/readme/README.zh-CN.md b/docs/readme/README.zh-CN.md index d7bae3fba9e..10f47e20fe6 100644 --- a/docs/readme/README.zh-CN.md +++ b/docs/readme/README.zh-CN.md @@ -235,9 +235,9 @@ yay -S stably-orca-bin - **Discord:** 加入 **[Discord](https://discord.gg/fzjDKHxv8Q)** 社区。 - **Twitter / X:** 关注 **[@orca_build](https://x.com/orca_build)** 获取更新和公告。 -- **微信:** 扫码加入 Orca 社区微信第 8 群。 +- **微信:** 扫码加入 Orca 社区微信第 8 群。第 8 群可能已满，如遇这种情况请扫描第 9 群二维码。 - Orca 社区微信第 8 群二维码 + Orca 社区微信第 8 群二维码  Orca 社区微信第 9 群二维码 - **反馈与想法:** 我们发布很快。缺少什么功能？[提交功能请求](https://github.com/stablyai/orca/issues)。 - **隐私:** 查看[隐私与遥测文档](https://www.onorca.dev/docs/telemetry)，了解 Orca 收集哪些匿名使用数据以及如何退出。 diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a045833577f..d7481f2e556 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -116,7 +116,7 @@ patchedDependencies: '@xterm/addon-webgl@0.20.0-beta.299': 94687e89a0115e6e6aa102837f986debdc029c091527ee5eb4a4e17ceaf9473e '@xterm/xterm@6.1.0-beta.303': 98756bcedc402bcdb7c6ab7b015d2e59cd18e97b03a2c06a27e95bb3ba429d9d lint-staged@16.4.0: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 - node-pty@1.1.0: e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa + node-pty@1.1.0: 7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1 importers: @@ -157,7 +157,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa) + version: 1.1.0(patch_hash=7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12195,7 +12195,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=e262847f57a1d4d3f2287a843822f7dcf3c9d8655892b07a69eba464e1317eaa): + node-pty@1.1.0(patch_hash=7cc9d45f3d2c38f142490d0805e75db55f0eef5174ad41c4b52abc5fbe079ad1): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/ipc/pty/ipc/inspect.ts b/src/main/ipc/pty/ipc/inspect.ts index c13c4242fea..041abaa7854 100644 --- a/src/main/ipc/pty/ipc/inspect.ts +++ b/src/main/ipc/pty/ipc/inspect.ts @@ -170,7 +170,10 @@ export function installPtyInspectIpcHandlers(deps: { ipcMain.handle( 'pty:inspectProcess', - async (_event, args: { id: string; expectedIncarnationId?: string }) => { + async ( + _event, + args: { id: string; expectedIncarnationId?: string; scanChildProcesses?: boolean } + ) => { // Why: same routing hazard as pty:hasPty — an unroutable id must read as client-only unverifiable, not as a local-provider answer or a raised IPC error. if (typeof args?.id !== 'string' || !args.id || args.id.startsWith('remote:')) { return clientOnlyUnverifiableInspection('terminal_gone') @@ -182,10 +185,14 @@ export function installPtyInspectIpcHandlers(deps: { if (!hasPtyProviderForInspection(args.id)) { return clientOnlyUnverifiableInspection('terminal_gone') } - return args.expectedIncarnationId - ? inspectPtyProviderProcessForRenderer(getProviderForPty(args.id), args.id, { - expectedIncarnationId: args.expectedIncarnationId - }) + const options = { + ...(args.expectedIncarnationId + ? { expectedIncarnationId: args.expectedIncarnationId } + : {}), + ...(args.scanChildProcesses === true ? { scanChildProcesses: true } : {}) + } + return Object.keys(options).length > 0 + ? inspectPtyProviderProcessForRenderer(getProviderForPty(args.id), args.id, options) : inspectPtyProviderProcessForRenderer(getProviderForPty(args.id), args.id) } ) diff --git a/src/main/ipc/pty/runtime/queried-host-kinds.test.ts b/src/main/ipc/pty/runtime/queried-host-kinds.test.ts new file mode 100644 index 00000000000..1f7ce2459d8 --- /dev/null +++ b/src/main/ipc/pty/runtime/queried-host-kinds.test.ts @@ -0,0 +1,60 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { parseExecutionHostId } from '../../../../shared/execution-host' +import { sshProviders } from '../provider/registry' +import { listProcessesWithHostScopeFromRuntimeController } from './inventory-operations' +import type { PtyRuntimeControllerDeps } from './controller-deps' + +/** + * `hostScopeCensusIsComplete` discounts a `runtime:` host in `omittedHostIds` on the strength of + * one fact about this process: it has no paired-runtime PTY provider, so it never queried that + * host and never owed it coverage. This file pins the producer side of that fact. + * + * What it catches: a new branch here that spells a queried host `runtime:`. Every id this + * function emits is built by `toSshExecutionHostId` or is `LOCAL_EXECUTION_HOST_ID`, so a third + * shape is the observable form of "a runtime host can now answer an inventory" — at which point + * the client predicate would start calling a genuine gap complete. + * + * What it does NOT catch, so do not lean on it: a runtime-backed transport registered under an + * SSH connection id still reports as `ssh:` and passes, which is fine — the predicate only + * discounts the `runtime:` spelling. The consolidation moving the SSH path onto orcad is expected + * to look exactly like that. The other route into `queriedHostIds` is separately fenced to + * `kind === 'ssh'` in `orca-runtime-refresh-pty-worktree-records-with-controller-inventory.ts`. + */ +describe('the hosts a PTY inventory can report having queried', () => { + afterEach(() => { + sshProviders.clear() + }) + + it('emits only local and ssh spellings, never a paired-runtime one', async () => { + const listProcesses = vi.fn(async () => []) + sshProviders.set('box-1', { listProcesses } as never) + // A connection id shaped like an environment uuid still has to come back `ssh:`; the spelling + // is what the gate keys on, so a `runtime:` id appearing here is the breakage that matters. + sshProviders.set('a2478221-1d5c-4603-b8bf-b6b728eac9df', { listProcesses } as never) + + const { hostIds } = await listProcessesWithHostScopeFromRuntimeController({ + runtime: null + } as unknown as PtyRuntimeControllerDeps) + + expect(hostIds).toContain('ssh:a2478221-1d5c-4603-b8bf-b6b728eac9df') + expect(new Set(hostIds.map((hostId) => parseExecutionHostId(hostId)?.kind))).toEqual( + new Set(['local', 'ssh']) + ) + }) + + it('drops a provider that threw rather than reporting its host as queried', async () => { + sshProviders.set('box-live', { listProcesses: vi.fn(async () => []) } as never) + sshProviders.set('box-down', { + listProcesses: vi.fn(async () => { + throw new Error('relay unavailable') + }) + } as never) + + const { hostIds } = await listProcessesWithHostScopeFromRuntimeController({ + runtime: { markPtyLivenessUnverifiable: vi.fn() } + } as unknown as PtyRuntimeControllerDeps) + + expect(hostIds).toContain('ssh:box-live') + expect(hostIds).not.toContain('ssh:box-down') + }) +}) diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing.ts b/src/main/ipc/worktrees/listing/detected-provider-listing.ts index 25c08d262fb..388c825530f 100644 --- a/src/main/ipc/worktrees/listing/detected-provider-listing.ts +++ b/src/main/ipc/worktrees/listing/detected-provider-listing.ts @@ -26,8 +26,7 @@ import { type DetectedWorktreeSideEffectToken } from './detected-worktree-scan-cache' import { loggedWorktreeListFailures, warnOnce } from './worktree-listing-diagnostics' -import { readAllWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' -import { getRepoExecutionHostId } from '../../../../shared/execution-host' +import { readAllWorktreeMetaForRepo } from '../../../persistence/host-qualified-worktree-meta' export async function listDetectedWorktreesForCapturedRepo( store: Store, @@ -40,9 +39,7 @@ export async function listDetectedWorktreesForCapturedRepo( providerAbort?.signal.aborted ? ({ providerAbortStatus: providerAbort.status() } as const) : undefined - const allMeta = isFolderRepo(repo) - ? undefined - : readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) + const allMeta = isFolderRepo(repo) ? undefined : readAllWorktreeMetaForRepo(store, repo) // Why: only the disconnected fallbacks read this, so keep parseWorktreeId over the whole host snapshot // off the connected path entirely. let cachedSshWorktreeMetaIndex: SshWorktreeMetaIndex | undefined diff --git a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts index d684461a381..4f3055c63a2 100644 --- a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts +++ b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts @@ -23,7 +23,10 @@ import { warnOnce } from './worktree-listing-diagnostics' import type { WorktreeIpcContext } from '../worktree-ipc-context' -import { readAllWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' +import { + readAllWorktreeMetaForHost, + readAllWorktreeMetaForRepo +} from '../../../persistence/host-qualified-worktree-meta' import type { WorktreeMeta } from '../../../../shared/worktree/meta-types' const WORKTREE_LIST_ALL_CONCURRENCY = 8 @@ -174,9 +177,7 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo if (!repo) { return [] } - const allMeta = repo.connectionId - ? readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) - : undefined + const allMeta = repo.connectionId ? readAllWorktreeMetaForRepo(store, repo) : undefined const sshWorktreeMetaIndex = repo.connectionId ? createSshWorktreeMetaIndex(Object.entries(allMeta ?? {})) : new Map() @@ -226,7 +227,7 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo }) } loggedWorktreeListFailures.delete(`${repo.id}:${repo.path}`) - const metadata = allMeta ?? readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) + const metadata = allMeta ?? readAllWorktreeMetaForRepo(store, repo) return buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) .filter((worktree) => worktree.visible) .map((worktree) => stampAndMergeVisibleDetectedWorktree(store, repo, worktree, metadata)) diff --git a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts index 5c734d8bcd9..ecf8ab3abc1 100644 --- a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts +++ b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts @@ -9,7 +9,7 @@ import type { GitWorktreeInfo, DetectedWorktree, Worktree } from '../../../../sh import type { Store } from '../../../persistence/loading-store/store' import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { - readWorktreeMetaForHost, + readWorktreeMetaForRepo, writeWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' import { getRepoOwnedWorktreeMeta } from '../../../worktree-metadata-ownership' @@ -159,7 +159,7 @@ export function buildDetectedGitWorktrees( const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const metaById = allMeta ?? (legacyMeta ? { [worktreeId]: legacyMeta } : {}) const meta = - readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(repo)) ?? + readWorktreeMetaForRepo(store, worktreeId, repo) ?? getRepoOwnedWorktreeMeta(repo, worktreeId, metaById, repoOwnerCount) const worktree = mergeWorktree(repo.id, gitWorktree, meta, repo.displayName) const detected = toDetectedWorktree({ diff --git a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts index b17677ddfcc..3edb8efc1d7 100644 --- a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts +++ b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts @@ -4,7 +4,7 @@ import type { WorktreeMeta } from '../../../../shared/worktree/meta-types' import { getProjectHostSetupWorktreeMeta } from '../../../../shared/project-host-setup-lookup' import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { - readWorktreeMetaForHost, + readWorktreeMetaForRepo, writeWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' import { getRepoOwnedWorktreeMeta } from '../../../worktree-metadata-ownership' @@ -44,7 +44,7 @@ export function resolveWorktreeMetaWithDiscoveryBackfill( // Why: the locator-keyed row is only a stand-in for a missing snapshot, so don't read it when we have one. const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const existing = - readWorktreeMetaForHost(store, worktreeId, executionHostId) ?? + readWorktreeMetaForRepo(store, worktreeId, repo) ?? getRepoOwnedWorktreeMeta( repo, worktreeId, diff --git a/src/main/persistence/host-qualified-worktree-meta.ts b/src/main/persistence/host-qualified-worktree-meta.ts index a9d1c0e8fb1..6267311f471 100644 --- a/src/main/persistence/host-qualified-worktree-meta.ts +++ b/src/main/persistence/host-qualified-worktree-meta.ts @@ -1,4 +1,5 @@ -import type { ExecutionHostId } from '../../shared/execution-host' +import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' import type { WorktreeMeta } from '../../shared/worktree/meta-types' /** @@ -52,6 +53,26 @@ export function readWorktreeMetaForHost( return store.getWorktreeMetaForHost?.(worktreeId, executionHostId) } +/** + * The same two reads keyed off a repo row, so the resolve-then-read pair lives in one place. Four + * call sites had open-coded it identically, which is the shape that lets one copy drift from the + * rest (F7/F8). + */ +export function readAllWorktreeMetaForRepo( + store: Pick, + repo: Pick +): Record { + return readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) +} + +export function readWorktreeMetaForRepo( + store: Pick, + worktreeId: string, + repo: Pick +): WorktreeMeta | undefined { + return readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(repo)) +} + export function writeWorktreeMetaForHost( store: Pick, worktreeId: string, diff --git a/src/main/providers/pty-process-inspection.ts b/src/main/providers/pty-process-inspection.ts index 6869899de8b..59d910b2238 100644 --- a/src/main/providers/pty-process-inspection.ts +++ b/src/main/providers/pty-process-inspection.ts @@ -11,14 +11,24 @@ export type PtyProcessInspection = TerminalProcessInspection type CompletionSensitivePtyProvider = IPtyProvider & { inspectProcess?: ( id: string, - options?: { expectedIncarnationId?: PtyIncarnationId } + options?: PtyProcessInspectionOptions ) => Promise } +/** + * `scanChildProcesses` marks a read whose answer decides something once, rather than a poll that + * self-corrects on its next tick. Only hosts where the child answer costs a process-table read + * act on it; everywhere else the answer was already captured. + */ +export type PtyProcessInspectionOptions = { + expectedIncarnationId?: PtyIncarnationId + scanChildProcesses?: boolean +} + export async function inspectPtyProviderProcess( provider: IPtyProvider, ptyId: string, - options?: { expectedIncarnationId?: PtyIncarnationId } + options?: PtyProcessInspectionOptions ): Promise { if (provider.hasPty?.(ptyId) === false) { throw new Error('terminal_gone') @@ -37,7 +47,7 @@ export async function inspectPtyProviderProcess( export async function inspectPtyProviderProcessForRenderer( provider: IPtyProvider, ptyId: string, - options?: { expectedIncarnationId?: PtyIncarnationId } + options?: PtyProcessInspectionOptions ): Promise { try { return await inspectPtyProviderProcess(provider, ptyId, options) diff --git a/src/main/providers/ssh-pty-provider-rpc-operations.ts b/src/main/providers/ssh-pty-provider-rpc-operations.ts index 71ddbefce97..2e273239cd4 100644 --- a/src/main/providers/ssh-pty-provider-rpc-operations.ts +++ b/src/main/providers/ssh-pty-provider-rpc-operations.ts @@ -56,13 +56,15 @@ export function createSshPtyProviderRpcOperations({ mux, toRelayPtyId }: SshPtyP // Guarded by ssh-pty-inspect-observation-identity.test.ts; #17525 removes the poll. inspectProcess: async ( id: string, - options?: { expectedIncarnationId?: string } + options?: { expectedIncarnationId?: string; scanChildProcesses?: boolean } ): Promise => { return (await mux.request('pty.inspectProcess', { id: toRelayPtyId(id), ...(options?.expectedIncarnationId ? { expectedIncarnationId: options.expectedIncarnationId } - : {}) + : {}), + // Additive request member: an older relay ignores it and answers as it always did. + ...(options?.scanChildProcesses === true ? { scanChildProcesses: true } : {}) })) as PtyProcessInspection }, serialize: async (ids: string[]): Promise => { diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index 70c01b5a1c7..3d0c46d7a03 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -63,7 +63,7 @@ export class SshPtyProvider implements IPtyProvider { this.rpcOperations.getForegroundProcess(id) inspectProcess = ( id: string, - options?: { expectedIncarnationId?: string } + options?: { expectedIncarnationId?: string; scanChildProcesses?: boolean } ): Promise => this.rpcOperations.inspectProcess(id, options) serialize = (ids: string[]): Promise => this.rpcOperations.serialize(ids) revive = (state: string): Promise => this.rpcOperations.revive(state) diff --git a/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts b/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts new file mode 100644 index 00000000000..1f9cae275c3 --- /dev/null +++ b/src/main/pty/node-pty-self-exit-pseudoconsole-close.test.ts @@ -0,0 +1,180 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * A shell that exits by itself must still close its pseudoconsole. + * + * `ClosePseudoConsole` is the only thing that reaps a ConPTY's console host — + * Orca's own job-ownership patch says so, because `CreatePseudoConsole` spawns + * that host before the per-pty job exists and it is therefore not a job member. + * Upstream node-pty calls it from exactly one place, `PtyKill`, which begins by + * looking the baton up by id — and the exit watcher in `SetupExitCallback` + * erased the baton the moment the shell died. So on the self-exit path (typing + * `exit`, which is how panes usually close) that lookup missed, `PtyKill` did + * nothing at all, and the pseudoconsole was never closed. + * + * There is a SECOND, independent defect on the same path: the `useConptyDll` + * branch of `WindowsPtyAgent.kill()` disposed the conout worker only from an + * `_outSocket.on('data')` handler, and no more data arrives once the shell has + * gone — so that worker leaked too. The non-DLL branch beside it already + * disposed unconditionally. The desktop always sets `useConptyDll`, so it hit + * both; the relay sets neither and hit only the first. + * + * Measured on Windows 11 / awin, 20 cycles, handles bucketed by NT object type, + * totals before -> after: + * + * self-exit, relay spawn 225 -> 285 becomes 219 -> 219 FLAT + * self-exit, desktop spawn 239 -> 439 becomes 222 -> 222 FLAT + * explicit kill, relay spawn 225 -> 285 becomes 219 -> 219 FLAT + * explicit kill, desktop spawn 235 -> 395 becomes 219 -> 219 FLAT + * + * Neither fix alone is enough on the desktop: the pseudoconsole close is worth + * +1 Process +1 File per terminal, the dispose +2 Thread +4 File. + * + * WHY THIS IS A PATCH-CONTENT PIN AND NOT A BEHAVIOURAL TEST: the defect is + * only observable as a per-NT-type handle count, which needs + * `NtQuerySystemInformation(SystemExtendedHandleInformation)`. Nothing in the + * repo can read that, and the cheaper Windows-observable proxies do not + * discriminate — the console host process is reaped either way (the leak is a + * handle to an already-exited object, not an orphaned process), and the + * `\\.\pipe\conpty-*` entries disappear either way. Both were measured and + * rejected as assertions rather than assumed. So this pins the mechanism + * instead, which is the real risk: a future resync of the vendored patch + * silently dropping the hunk. + */ + +const PATCH = readFileSync(join(__dirname, '../../../config/patches/node-pty@1.1.0.patch'), 'utf8') + +/** + * Just the `PtyKill` hunk. Several markers below also occur in the `PtyConnect` + * hunk above it, and a bare `indexOf` on the whole patch silently matched the + * wrong one — an assertion that then held regardless of what `PtyKill` did. + */ +const ptyKillHunk = (() => { + // Anchored on the hunk header's function context rather than its line + // numbers, which shift whenever anything above it in the patch changes. + const header = /^@@ .* @@ static Napi::Value PtyKill\(.*$/m.exec(PATCH) + if (!header) { + throw new Error('no PtyKill hunk in config/patches/node-pty@1.1.0.patch') + } + const from = header.index + const next = PATCH.indexOf('\n@@ ', from + 1) + return PATCH.slice(from, next === -1 ? undefined : next) +})() + +/** + * `indexOf` that throws instead of returning -1. A missing marker must fail the + * assertion that depends on it, not quietly make a slice or comparison vacuous. + */ +function indexIn(haystack: string, marker: string): number { + const at = haystack.indexOf(marker) + if (at === -1) { + throw new Error(`marker not found in the PtyKill hunk: ${marker}`) + } + return at +} + +describe('node-pty patch: pseudoconsole close on the self-exit path', () => { + it('does not let the exit watcher free the baton while the close is still owed', () => { + // Pinned as one block: the erase must stay INSIDE the consoleClosed guard. + // Upstream ran it unconditionally, which is the line that caused the leak, + // and a resync that re-flattens this is the failure mode worth catching. + expect(PATCH).toContain( + [ + '+ baton->shellExited = true;', + '+ if (baton->consoleClosed) {', + '+ const bool removed = remove_pty_baton(baton->id);', + '+ assert(removed);', + '+ (void)removed;', + '+ }' + ].join('\n') + ) + }) + + it('closes the pseudoconsole from PtyKill even after the shell has exited', () => { + // hpc is copied out under the lock, so the close survives the baton's removal. + expect(PATCH).toContain('+ hpc = handle->hpc;') + expect(PATCH).toContain('+ pfnClosePseudoConsole(hpc);') + }) + + it('resolves the ConPTY DLL before it claims the close', () => { + // LoadConptyDll throws when conpty.dll is missing. Throwing after + // consoleClosed was set would strand the pseudoconsole for good: the retry + // finds the work claimed and does nothing. + // + // Anchored inside PtyKill, not by a bare indexOf: the identical line also + // appears in the PtyConnect hunk, earlier in the file, and matching that one + // made this assertion pass no matter where PtyKill resolved the DLL. + const dllResolve = indexIn( + ptyKillHunk, + '+ HANDLE hLibrary = LoadConptyDll(info, useConptyDll);' + ) + const claim = indexIn(ptyKillHunk, '+ handle->consoleClosed = true;') + expect(dllResolve).toBeLessThan(claim) + }) + + it('reaches hShell only under the null check the watcher can trip', () => { + // Pinned as one block. The watcher nulls hShell on exit, and upstream + // dereferenced it unconditionally; every remaining use — the duplication and + // the failure fallback below it — must stay inside this guard. + const start = indexIn(ptyKillHunk, '+ if (useConptyDll && handle->hShell != nullptr) {') + const end = indexIn(ptyKillHunk, '+ if (handle->shellExited) {') + const guarded = ptyKillHunk.slice(start, end) + expect(guarded).toContain('DuplicateHandle(GetCurrentProcess(), handle->hShell') + expect(guarded).toContain('TerminateProcess(handle->hShell, 1);') + // No ADDED line outside that guard may terminate through hShell. Removed + // (`-`) lines still carry upstream's unguarded call, which is the point. + const strayAdds = PATCH.replace(guarded, '') + .split('\n') + .filter((line) => line.startsWith('+') && line.includes('TerminateProcess(handle->hShell')) + expect(strayAdds).toEqual([]) + }) + + it('frees the baton from PtyKill when the shell has already exited', () => { + // The other half of the two-sided handshake. 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, forever. + expect(ptyKillHunk).toContain( + [ + '+ if (handle->shellExited) {', + '+ const bool removed = remove_pty_baton(id);', + '+ assert(removed);', + '+ (void)removed;' + ].join('\n') + ) + }) + + it('still kills the shell when DuplicateHandle fails', () => { + // A null hShellDup is indistinguishable from the self-exit case, so a + // swallowed failure would leave the shell running after its pane closed — + // a worse outcome than the leak this patch exists to fix. + expect(PATCH).toContain( + [ + '+ hShellDup = nullptr;', + '+ TerminateProcess(handle->hShell, 1);', + '+ }' + ].join('\n') + ) + }) + + it('keeps the close idempotent so a second kill cannot double-close', () => { + expect(PATCH).toContain('+ if (handle != nullptr && !handle->consoleClosed) {') + expect(PATCH).toContain('+ handle->consoleClosed = true;') + }) +}) + +describe('node-pty patch: conout worker disposal on the self-exit path', () => { + // The desktop's larger half: 8 of its 10 leaked handles per terminal. + it('disposes the conout worker unconditionally in the useConptyDll branch', () => { + expect(PATCH).toContain('+ this._conoutSocketWorker.dispose();') + // The data handler is what never fired once the shell had gone. + expect(PATCH).toContain("- this._outSocket.on('data', function () {") + expect(PATCH).toContain('- _this._conoutSocketWorker.dispose();') + }) + + it('applies the same change to the TypeScript source the patch also carries', () => { + expect(PATCH).toContain('+ this._conoutSocketWorker.dispose();') + expect(PATCH).toContain("- this._outSocket.on('data', () => {") + }) +}) diff --git a/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts b/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts index 0ed3700ba99..4466594b0dc 100644 --- a/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts +++ b/src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts @@ -153,7 +153,7 @@ export class OrcaRuntimeWithRestoreStructuredAgentSessionTabsOnce extends OrcaRu async inspectTerminalProcess( terminalSelector: string, - options?: { expectedIncarnationId?: string } + options?: { expectedIncarnationId?: string; scanChildProcesses?: boolean } ): Promise { const leaf = this.resolveLiveLeafForHandle(terminalSelector) if (!leaf?.ptyId || !this.ptyController) { diff --git a/src/main/runtime/rpc/methods/terminal/terminal-inspect-process-params.test.ts b/src/main/runtime/rpc/methods/terminal/terminal-inspect-process-params.test.ts new file mode 100644 index 00000000000..48649bc372c --- /dev/null +++ b/src/main/runtime/rpc/methods/terminal/terminal-inspect-process-params.test.ts @@ -0,0 +1,92 @@ +// The host half of the same contract: an RPC schema silently strips keys it does not declare, which +// is exactly what forward compatibility needs and exactly how a caller's option can vanish inside +// one version. `scanChildProcesses` has to be declared here, and only here -- the sibling handle +// methods have no use for it and must keep refusing it. +import { describe, expect, it, vi } from 'vitest' +import type { ZodType } from 'zod' +import { TERMINAL_QUERY_METHODS } from './terminal-query-methods' +import { TerminalHandle, TerminalInspectProcess } from './unary-schemas' + +/** The method as registered, so a schema swap on the definition cannot pass unseen. */ +function inspectProcessMethod() { + const method = TERMINAL_QUERY_METHODS.find((entry) => entry.name === 'terminal.inspectProcess') + if (!method) { + throw new Error('terminal.inspectProcess is not registered') + } + return method +} + +async function callRegisteredHandler( + params: Record +): Promise<{ terminal: string; options: unknown }> { + const method = inspectProcessMethod() + const parsed = (method.params as ZodType).parse(params) + const inspectTerminalProcess = vi.fn(async () => ({ + foregroundProcess: null, + hasChildProcesses: false + })) + await method.handler(parsed, { runtime: { inspectTerminalProcess } } as never, undefined as never) + const [terminal, options] = inspectTerminalProcess.mock.calls[0] as unknown as [string, unknown] + return { terminal, options } +} + +describe('terminal.inspectProcess registration', () => { + // The half the schema test alone cannot see: pointing the method back at the shared handle schema + // compiles, parses, and silently drops the option. This exercises the registered definition. + it('carries scanChildProcesses from the wire into the runtime call', async () => { + await expect( + callRegisteredHandler({ terminal: 'term_1', scanChildProcesses: true }) + ).resolves.toEqual({ terminal: 'term_1', options: { scanChildProcesses: true } }) + }) + + it('carries it alongside the incarnation fence', async () => { + await expect( + callRegisteredHandler({ + terminal: 'term_1', + expectedIncarnationId: 'inc-1', + scanChildProcesses: true + }) + ).resolves.toEqual({ + terminal: 'term_1', + options: { expectedIncarnationId: 'inc-1', scanChildProcesses: true } + }) + }) + + it('keeps the legacy one-argument shape for a bare poll', async () => { + await expect(callRegisteredHandler({ terminal: 'term_1' })).resolves.toEqual({ + terminal: 'term_1', + options: undefined + }) + }) +}) + +describe('terminal.inspectProcess params', () => { + it('preserves scanChildProcesses', () => { + expect(TerminalInspectProcess.parse({ terminal: 'term_1', scanChildProcesses: true })).toEqual({ + terminal: 'term_1', + scanChildProcesses: true + }) + }) + + it('preserves it alongside the incarnation fence', () => { + expect( + TerminalInspectProcess.parse({ + terminal: 'term_1', + expectedIncarnationId: 'inc-1', + scanChildProcesses: true + }) + ).toEqual({ terminal: 'term_1', expectedIncarnationId: 'inc-1', scanChildProcesses: true }) + }) + + it('leaves it absent for a polling caller', () => { + expect(TerminalInspectProcess.parse({ terminal: 'term_1' })).toEqual({ terminal: 'term_1' }) + }) + + // The shape that produced the bug, pinned so nobody "simplifies" the method back onto the shared + // handle schema: TerminalHandle drops the option on the floor without complaining. + it('shows why the shared handle schema could not carry it', () => { + expect(TerminalHandle.parse({ terminal: 'term_1', scanChildProcesses: true })).toEqual({ + terminal: 'term_1' + }) + }) +}) diff --git a/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts b/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts index a8aec9ff573..bf7b4a5bd87 100644 --- a/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts +++ b/src/main/runtime/rpc/methods/terminal/terminal-query-methods.ts @@ -1,6 +1,7 @@ import { defineMethod, type RpcAnyMethod } from '../../core' import { TerminalHandle, + TerminalInspectProcess, TerminalListParams, TerminalRead, TerminalRecoverPane, @@ -65,15 +66,21 @@ export const TERMINAL_QUERY_METHODS: RpcAnyMethod[] = [ }), defineMethod({ name: 'terminal.inspectProcess', - params: TerminalHandle, - handler: async (params, { runtime }) => ({ - process: await runtime.inspectTerminalProcess( - params.terminal, - params.expectedIncarnationId + params: TerminalInspectProcess, + handler: async (params, { runtime }) => { + const options = { + ...(params.expectedIncarnationId ? { expectedIncarnationId: params.expectedIncarnationId } - : undefined - ) - }) + : {}), + ...(params.scanChildProcesses === true ? { scanChildProcesses: true } : {}) + } + return { + process: await runtime.inspectTerminalProcess( + params.terminal, + Object.keys(options).length > 0 ? options : undefined + ) + } + } }), defineMethod({ name: 'terminal.isRunningAgent', diff --git a/src/main/runtime/rpc/methods/terminal/unary-schemas.ts b/src/main/runtime/rpc/methods/terminal/unary-schemas.ts index b8700ab1992..2734de0af1e 100644 --- a/src/main/runtime/rpc/methods/terminal/unary-schemas.ts +++ b/src/main/runtime/rpc/methods/terminal/unary-schemas.ts @@ -13,6 +13,17 @@ export const TerminalFocus = TerminalHandle.extend({ navigation: z.enum(['caller', 'host']).optional() }) +/** + * `terminal.inspectProcess` carries one member the sibling handle methods must not: whether the + * caller's answer decides something once, which is what licenses the host to pay for a process-table + * read. Extended rather than added to `TerminalHandle` so `clearBuffer`/`agentStatus`/`isRunningAgent` + * keep refusing an option they have no use for. + */ +export const TerminalInspectProcess = TerminalHandle.extend({ + // Additive request member understood by newer hosts; legacy hosts safely ignore it. + scanChildProcesses: z.boolean().optional() +}) + export const TerminalListParams = z.object({ worktree: OptionalString, limit: OptionalFiniteNumber, diff --git a/src/main/runtime/runtime-managed-worktree-queries.test.ts b/src/main/runtime/runtime-managed-worktree-queries.test.ts index 01df53cbd1d..3fb792fcc7c 100644 --- a/src/main/runtime/runtime-managed-worktree-queries.test.ts +++ b/src/main/runtime/runtime-managed-worktree-queries.test.ts @@ -40,14 +40,18 @@ function metadata(overrides: Partial = {}): WorktreeMeta { } } -function queries(store: RuntimeStore): RuntimeManagedWorktreeQueries { +function queries( + store: RuntimeStore, + overrides: Partial[0]> = {} +): RuntimeManagedWorktreeQueries { return new RuntimeManagedWorktreeQueries({ getStore: () => store, listResolved: async () => [], resolveRepo: async () => store.getRepos()[0]!, selectRepos: () => store.getRepos(), scanRepo: async () => ({ ok: true, worktrees: [] }), - listKnownHostIds: () => [] + listKnownHostIds: () => [], + ...overrides }) } @@ -105,3 +109,115 @@ describe('RuntimeManagedWorktreeQueries.listDetected', () => { expect(legacy.worktrees[0]).not.toHaveProperty('visibilitySource') }) }) + +describe('RuntimeManagedWorktreeQueries.list host scope', () => { + // Measured on hardware before this fix, same runtime and same refusing SSH host in the same + // second: the UNSCOPED listing reported `omittedHostIds: ["local","ssh:ssh-scope-refused"]` with + // `--host` selectors, while the SCOPED listing reported `{"hostIds":[],"omittedHostIds":[]}`. + // A listing that covered nothing, reporting no gaps, is indistinguishable from a repo that + // genuinely has no worktrees -- the thing docs/reference/ssh-execution-boundary.md forbids. + function sshStore(): RuntimeStore { + const repo = folderRepo({ + id: 'repo-ssh', + kind: 'git', + connectionId: 'conn-1', + path: '/home/dev/app' + }) + return { + getRepos: () => [repo], + getRepo: () => repo, + getAllWorktreeMeta: () => ({}), + getWorktreeMeta: () => undefined, + setWorktreeMeta: vi.fn(), + getAllWorktreeLineage: () => ({}), + getSettings: () => settings + } as unknown as RuntimeStore + } + + it('names the scoped repo host as omitted when the listing covered nothing', async () => { + const result = await queries(sshStore()).list('repo-ssh', 50) + + expect(result.totalCount).toBe(0) + expect(result.hostScope).toEqual({ + hostIds: [], + omittedHostIds: ['ssh:conn-1'] + }) + }) + + it('does not report the scoped host as omitted once it contributes rows', async () => { + const store = sshStore() + const result = await queries(store, { + listResolved: async () => + [ + { + id: 'repo-ssh::/home/dev/app', + repoId: 'repo-ssh', + path: '/home/dev/app', + hostId: 'ssh:conn-1' + } + ] as never + }).list('repo-ssh', 50) + + expect(result.hostScope?.hostIds).toEqual(['ssh:conn-1']) + expect(result.hostScope?.omittedHostIds).toEqual([]) + }) + + // The caller scoped the listing, so the hosts they excluded must not come back as gaps. + it('never names a host the caller scoped out', async () => { + const scoped = await queries(sshStore(), { + listKnownHostIds: () => ['local', 'ssh:other', 'runtime:elsewhere'] as never + }).list('repo-ssh', 50) + + expect(scoped.hostScope?.omittedHostIds).toEqual(['ssh:conn-1']) + }) + + // `getRepoExecutionHostId` derives the host from two spellings, and a scoped listing that named + // the wrong one would be worse than naming none. These pin both. + it('names the local host for a scoped local repo', async () => { + const repo = folderRepo({ id: 'repo-local', kind: 'git', path: '/workspace/local' }) + const store = { + getRepos: () => [repo], + getRepo: () => repo, + getAllWorktreeMeta: () => ({}), + getWorktreeMeta: () => undefined, + setWorktreeMeta: vi.fn(), + getAllWorktreeLineage: () => ({}), + getSettings: () => settings + } as unknown as RuntimeStore + + const result = await queries(store).list('repo-local', 50) + + expect(result.hostScope?.omittedHostIds).toEqual(['local']) + }) + + it('prefers executionHostId over connectionId for the scoped host', async () => { + const repo = folderRepo({ + id: 'repo-runtime', + kind: 'git', + connectionId: 'conn-legacy', + executionHostId: 'runtime:env-1', + path: '/workspace/runtime' + }) + const store = { + getRepos: () => [repo], + getRepo: () => repo, + getAllWorktreeMeta: () => ({}), + getWorktreeMeta: () => undefined, + setWorktreeMeta: vi.fn(), + getAllWorktreeLineage: () => ({}), + getSettings: () => settings + } as unknown as RuntimeStore + + const result = await queries(store).list('repo-runtime', 50) + + expect(result.hostScope?.omittedHostIds).toEqual(['runtime:env-1']) + }) + + it('still reports every configured host when the listing is unscoped', async () => { + const unscoped = await queries(sshStore(), { + listKnownHostIds: () => ['local', 'ssh:conn-1'] as never + }).list(undefined, 50) + + expect(unscoped.hostScope?.omittedHostIds).toEqual(['local', 'ssh:conn-1']) + }) +}) diff --git a/src/main/runtime/runtime-managed-worktree-queries.ts b/src/main/runtime/runtime-managed-worktree-queries.ts index 5a5812236af..158ab77cdd0 100644 --- a/src/main/runtime/runtime-managed-worktree-queries.ts +++ b/src/main/runtime/runtime-managed-worktree-queries.ts @@ -2,7 +2,7 @@ import type { DetectedWorktreeListResult, Worktree } from '../../shared/worktree import type { Repo } from '../../shared/repo-types' import type { RuntimeWorktreeListResult } from '../../shared/runtime-types' import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' -import { buildWorktreeListingPage } from './worktree-listing-host-scope' +import { buildWorktreeListingPage, listingKnownHostIds } from './worktree-listing-host-scope' import { readWorktreeMetaForHost } from '../persistence/host-qualified-worktree-meta' import { getRepoOwnedWorktreeMeta } from '../worktree-metadata-ownership' import type { WorktreeMeta } from '../../shared/worktree/meta-types' @@ -80,7 +80,7 @@ export class RuntimeManagedWorktreeQueries { throw new Error('invalid_limit') } const resolved = await this.deps.listResolved() - const repoId = repoSelector ? (await this.deps.resolveRepo(repoSelector)).id : null + const scopedRepo = repoSelector ? await this.deps.resolveRepo(repoSelector) : null const pathsByRepo = new Map() for (const worktree of resolved) { const paths = pathsByRepo.get(worktree.repoId) ?? [] @@ -100,12 +100,12 @@ export class RuntimeManagedWorktreeQueries { ) const worktrees = resolved.filter( (worktree) => - (!repoId || worktree.repoId === repoId) && + (!scopedRepo || worktree.repoId === scopedRepo.id) && this.isVisible(worktree, matchers.get(worktree.repoId), sourceDefaultsSupported) ) - // Why: a `--repo` listing was scoped by the caller, so naming every configured host as - // omitted would report a gap the caller deliberately excluded. - return buildWorktreeListingPage(worktrees, limit, repoId ? [] : this.deps.listKnownHostIds()) + // See `listingKnownHostIds`: a scoped listing must still name the host it was asked about. + const knownHostIds = listingKnownHostIds(scopedRepo, () => this.deps.listKnownHostIds()) + return buildWorktreeListingPage(worktrees, limit, knownHostIds) } resolveRepoForConnection(selector: string, connectionId?: string | null): Promise { diff --git a/src/main/runtime/runtime-pty-controller-contract.ts b/src/main/runtime/runtime-pty-controller-contract.ts index 194d0bb665c..665c6fdb609 100644 --- a/src/main/runtime/runtime-pty-controller-contract.ts +++ b/src/main/runtime/runtime-pty-controller-contract.ts @@ -109,7 +109,7 @@ export type RuntimePtyController = { getForegroundProcess(ptyId: string): Promise inspectProcess?( ptyId: string, - options?: { expectedIncarnationId?: PtyIncarnationId } + options?: { expectedIncarnationId?: PtyIncarnationId; scanChildProcesses?: boolean } ): Promise confirmForegroundProcess?(ptyId: string): Promise confirmShellForeground?(ptyId: string): Promise diff --git a/src/main/runtime/worktree-launch-host-repo.ts b/src/main/runtime/worktree-launch-host-repo.ts index 4decb7acb56..7db9f4dae18 100644 --- a/src/main/runtime/worktree-launch-host-repo.ts +++ b/src/main/runtime/worktree-launch-host-repo.ts @@ -17,7 +17,10 @@ export type WorktreeHostRouting = | { kind: 'resolved'; hostId: ExecutionHostId; repo: T | null } /** No row carries this repo id and the worktree names no host — nothing ever named a host. */ | { kind: 'unowned' } - /** Rival rows disagree about the host; guessing one is the cross-host leak. */ + /** + * No single trustworthy host: rival rows disagree, or the resolved row named one that cannot be + * parsed. Guessing is the cross-host leak in both cases. + */ | { kind: 'ambiguous' } /** @@ -33,7 +36,10 @@ export function resolveWorktreeHostRouting { const resolution = resolveWorktreeExecutionHost(createRepoRowExecutionHostLookup(repos), worktree) if (resolution.kind === 'unresolved') { - return resolution.reason === 'ambiguous' ? { kind: 'ambiguous' } : { kind: 'unowned' } + // Only `unknown` — nothing anywhere carries the id — becomes `unowned`, which callers dispose of + // as a plain local folder. `malformed` is a row that declared a host and named an unparseable + // one, so it joins `ambiguous`: guessing is the cross-host leak either way. + return resolution.reason === 'unknown' ? { kind: 'unowned' } : { kind: 'ambiguous' } } return { kind: 'resolved', hostId: resolution.hostId, repo: resolution.owner } } diff --git a/src/main/runtime/worktree-listing-host-scope.ts b/src/main/runtime/worktree-listing-host-scope.ts index 99650882670..e499d7faaf3 100644 --- a/src/main/runtime/worktree-listing-host-scope.ts +++ b/src/main/runtime/worktree-listing-host-scope.ts @@ -1,4 +1,5 @@ -import type { ExecutionHostId } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' +import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import { selectHostBalancedPage } from '../../shared/host-balanced-listing-page' import type { RuntimeListingHostScope } from '../../shared/runtime-listing-host-scope' @@ -62,3 +63,28 @@ export function buildWorktreeListingHostScope(args: { } return { hostIds: [...covered].sort(), omittedHostIds: [...omitted].sort() } } + +/** + * Which hosts a listing claims to have been looking at. + * + * A `--repo` listing was scoped by the caller, so naming every configured host would report gaps + * the caller deliberately excluded. Naming NONE — which is what a scoped listing did before — means + * the scope can never report a gap at all, for any host kind, because `covered` and `omitted` are + * both derived from the returned rows plus this list. A scoped listing whose scan did not succeed + * then answers `{hostIds: [], omittedHostIds: []}`: byte-identical to a repo that genuinely has no + * worktrees, which is the one thing docs/reference/ssh-execution-boundary.md forbids a listing from + * implying. + * + * Measured before the fix, on one runtime with one refusing SSH host, in the same second: the + * unscoped listing reported `omittedHostIds: ["local", "ssh:"]` while the scoped listing + * reported `[]`. + * + * Naming the single host the caller asked about costs nothing when rows come back — it lands in + * `covered`, so it is never reported omitted — and is the whole answer when they do not. + */ +export function listingKnownHostIds( + scopedRepo: Repo | null, + listKnownHostIds: () => Iterable +): Iterable { + return scopedRepo ? [getRepoExecutionHostId(scopedRepo)] : listKnownHostIds() +} diff --git a/src/main/ssh/ssh-relay-deploy.ts b/src/main/ssh/ssh-relay-deploy.ts index 257eb984caa..e7450478d92 100644 --- a/src/main/ssh/ssh-relay-deploy.ts +++ b/src/main/ssh/ssh-relay-deploy.ts @@ -743,6 +743,7 @@ function uploadStageNamespaceIfSupported( const NODE_PTY_VERSION = '1.1.0' const NODE_PTY_CONSOLE_LIST_PATCH_FILENAME = 'node-pty-1.1.0-console-list-agent-patch.cjs' +const NODE_PTY_WINDOWS_TEARDOWN_PATCH_FILENAME = 'node-pty-1.1.0-windows-pty-teardown-patch.cjs' const NODE_PTY_MASTER_CLOEXEC_PATCH_FILENAME = 'node-pty-1.1.0-master-cloexec-patch.cjs' const NODE_PTY_CLOEXEC_STATUS_PREFIX = 'ORCA-NPTY-CLOEXEC:' /** @@ -791,7 +792,8 @@ function nativeDepsProbeJs(successToken: string): string { // Why: node-pty's Windows wrapper defers conpty.node until first spawn, so require("node-pty") alone can't prove the binding is healthy. const loadNodePty = 'require("node-pty"); require("node-pty/lib/utils").loadNativeModule(process.platform==="win32"&&Number(require("os").release().split(".")[2])>=18309?"conpty":"pty");' + - `if(process.platform==="win32"){require("./${NODE_PTY_CONSOLE_LIST_PATCH_FILENAME}").assertPatchedNodePtyConsoleListAgent(process.cwd())}` + `if(process.platform==="win32"){require("./${NODE_PTY_CONSOLE_LIST_PATCH_FILENAME}").assertPatchedNodePtyConsoleListAgent(process.cwd());` + + `require("./${NODE_PTY_WINDOWS_TEARDOWN_PATCH_FILENAME}").assertPatchedNodePtyWindowsTeardown(process.cwd())}` return `(()=>{const missing=[];try{${loadNodePty}}catch{missing.push("node-pty")}try{require("@parcel/watcher")}catch{missing.push("@parcel/watcher")}if(missing.length){console.log("${NATIVE_DEPS_MISSING_PREFIX}"+missing.join(","));process.exitCode=1}else{console.log(${JSON.stringify(successToken)})}})()` } @@ -1327,10 +1329,16 @@ async function applyNodePtyMasterCloexecPatch( nodePath: string, signal?: AbortSignal ): Promise { - // Both Unix relay platforms leak, by different bugs: Linux inherits the master through forkpty()'s - // no-O_CLOEXEC path, macOS orphans one throwaway /dev/ptmx fd per spawn in pty_posix_spawn. Only - // Windows, which has no fds, is short-circuited -- and answering 'fixed' from a gate that ran - // nothing is exactly how a leaking darwin tree got published to the shared cache. + // Both Unix relay platforms leak the pty master, by different bugs: Linux inherits it through + // forkpty()'s no-O_CLOEXEC path, macOS orphans one throwaway /dev/ptmx fd per spawn in + // pty_posix_spawn. Windows is short-circuited because it has no fds for a master to leak into -- + // and answering 'fixed' from a gate that ran nothing is exactly how a leaking darwin tree got + // published to the shared cache. + // + // What 'fixed' means here is exactly "this tree does not leak the pty MASTER", which is the only + // thing the shared native-deps cache keys on. It is NOT a statement that a Windows relay leaks + // nothing: it leaked one Windows File handle per terminal until the ConPTY teardown patch above, + // by a mechanism that has nothing to do with fds. Read this gate as scoped to its own question. if (isWindowsRemoteHost(hostPlatform) || isWindowsRelayPlatform(platform)) { return 'fixed' } @@ -1530,8 +1538,11 @@ async function rebuildNativeDeps( } function windowsNodePtyPatchCommand(nodePath: string): string { - // Why: pnpm patches do not cross the SSH boundary; apply the version-checked fallback to the remote npm package. - return `& ${powerShellLiteral(nodePath)} ${powerShellLiteral(NODE_PTY_CONSOLE_LIST_PATCH_FILENAME)}; if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }` + // Why: pnpm patches do not cross the SSH boundary; apply the version-checked fallbacks to the remote npm package. + return [ + `& ${powerShellLiteral(nodePath)} ${powerShellLiteral(NODE_PTY_CONSOLE_LIST_PATCH_FILENAME)}; if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }`, + `& ${powerShellLiteral(nodePath)} ${powerShellLiteral(NODE_PTY_WINDOWS_TEARDOWN_PATCH_FILENAME)}; if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }` + ].join('; ') } async function makeNodePtySpawnHelperExecutable( diff --git a/src/main/windows/windows-pty-job.ts b/src/main/windows/windows-pty-job.ts index 193b1d8825a..169db375f3b 100644 --- a/src/main/windows/windows-pty-job.ts +++ b/src/main/windows/windows-pty-job.ts @@ -118,8 +118,10 @@ export function terminatePtyJob(proc: IPty): JobTerminationOutcome { /** * Pids still alive in a PTY's tree, or null when there is no answer. * - * Measured on Windows 11: once the shell exits, node-pty drops its handle - * record and closes the job, so a terminated tree reports **null**, not `[]`. + * Measured on Windows 11: once the shell exits, node-pty closes the job, so a + * terminated tree reports **null**, not `[]`. (Its handle record now outlives + * the shell until `kill()` runs — see config/patches/node-pty@1.1.0.patch — but + * the nulled job handle is what makes the answer null either way.) * Null therefore means "unverifiable" in the sense of * docs/reference/ssh-execution-boundary.md — this build has no job support, * the terminal is not a ConPTY, or it is no longer tracked. It is never diff --git a/src/preload/api/pty-api.ts b/src/preload/api/pty-api.ts index bf012306675..a1850398357 100644 --- a/src/preload/api/pty-api.ts +++ b/src/preload/api/pty-api.ts @@ -112,7 +112,7 @@ export type PtyApi = { getForegroundProcess: (id: string) => Promise inspectProcess: ( id: string, - options?: { expectedIncarnationId?: string } + options?: { expectedIncarnationId?: string; scanChildProcesses?: boolean } ) => Promise confirmForegroundProcess: (id: string) => Promise getCwd: (id: string) => Promise diff --git a/src/preload/api/pty-bridge-stream-and-serialization.ts b/src/preload/api/pty-bridge-stream-and-serialization.ts index 7e2d7bbe4ff..414a5514bfa 100644 --- a/src/preload/api/pty-bridge-stream-and-serialization.ts +++ b/src/preload/api/pty-bridge-stream-and-serialization.ts @@ -7,7 +7,7 @@ import type { TerminalProcessInspection } from '../../shared/terminal-process-in export const ptyStreamAndSerializationApi = { inspectProcess: ( id: string, - options?: { expectedIncarnationId?: string } + options?: { expectedIncarnationId?: string; scanChildProcesses?: boolean } ): Promise => ipcRenderer.invoke('pty:inspectProcess', { id, ...options }), confirmForegroundProcess: (id: string): Promise => diff --git a/src/relay/pty-child-process-inspection.ts b/src/relay/pty-child-process-inspection.ts new file mode 100644 index 00000000000..6d246817053 --- /dev/null +++ b/src/relay/pty-child-process-inspection.ts @@ -0,0 +1,81 @@ +/** + * Whether anything is running under a pane's shell. + * + * Split out of `pty-shell-utils` because it is a distinct question from "what is in front" and + * carries its own platform reasoning, its own cost budget, and the verdict vocabulary from + * docs/reference/ssh-execution-boundary.md. + */ +import { queryWindowsPaneProcessInventory } from '../main/providers/windows-foreground-process-rows' +import { getProcessTableIndex } from '../shared/process-table-index' +import { + getFreshProcessTableSnapshot, + getProcessTableSnapshot +} from '../shared/process-table-snapshot-reader' +import type { PtyChildProcessVerdict } from '../shared/terminal-process-inspection' +import { isProcessAlive } from './pty-shell-utils' + +/** + * Check whether a process has child processes. + * + * Why the shared snapshot and not `pgrep -P`: this answers one field of + * `pty.inspectProcess`, which every tracked pane polls on a 750ms/2000ms + * cadence, and the fork was neither cached nor coalesced. procps-ng opens six + * procfs files per process to resolve a ppid — including a `/proc//ctty` + * that never exists on Linux — so one call cost O(host process count) syscalls, + * ~4k opens per pgrep on a 690-process host, at up to 8 forks/sec (#13537). + * `getForegroundProcessName` in the same RPC already captured the TTL-cached + * `ps` table, whose index carries the parent/child map, so the answer is free. + * + * `fresh` opts out of that TTL. A poll can read a 500ms-old table because its + * next tick corrects it, but a close or cleanup decision acts on the answer + * once and destructively — a child that started inside the TTL would be killed + * with no confirmation. `pgrep` scanned per call, so anything that decides + * has to keep scanning per call. + */ +export async function inspectPtyChildProcesses( + pid: number, + options?: { fresh?: boolean } +): Promise { + if (process.platform === 'win32') { + // Windows has no `ps`, but it does have a process table, and the pane walk over it already + // exists for the foreground reader. Answering `false` from nothing was the older shape: a + // hardcoded negative is indistinguishable from a measurement, and every close guard reads it + // as "nothing is running here". + // + // Deliberately the TTL-cached table even when `fresh` is asked for: on a relay without the + // native binding this falls back to the CIM scan, whose own 1.36s runtime is longer than the + // 500ms TTL a fresh read would be refreshing, so a "fresh" answer is not meaningfully fresher + // while N sequential ones are an N x 1.36s stall. + const inventory = await queryWindowsPaneProcessInventory(pid) + if (inventory) { + return inventory.candidates.length > 0 ? 'children' : 'no-children' + } + // A null inventory is an unreadable table OR a snapshot that never showed the root, and + // neither of those looked at the pane. The one answer available without the table is a root + // the kernel says is gone: nothing runs under a shell that does not exist. + return isProcessAlive(pid) ? 'unverifiable' : 'no-children' + } + try { + const rows = options?.fresh + ? await getFreshProcessTableSnapshot() + : await getProcessTableSnapshot() + return (getProcessTableIndex(rows).childrenByPpid.get(pid)?.length ?? 0) > 0 + ? 'children' + : 'no-children' + } catch { + return 'unverifiable' + } +} + +/** + * The boolean the wire has always carried. `unverifiable` keeps spelling itself `false` here on + * purpose: this value reaches clients too old to know the third answer, and it is read both as + * "busy, do not close" and as "the agent has taken over, safe to type into", so no single mapping + * of `unverifiable` is safe for both. Callers that can act on the distinction read the verdict. + */ +export async function processHasChildren( + pid: number, + options?: { fresh?: boolean } +): Promise { + return (await inspectPtyChildProcesses(pid, options)) === 'children' +} diff --git a/src/relay/pty-handler-spawn-admission.test.ts b/src/relay/pty-handler-spawn-admission.test.ts index c25a623f406..045ee2e412d 100644 --- a/src/relay/pty-handler-spawn-admission.test.ts +++ b/src/relay/pty-handler-spawn-admission.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' +import * as ptyChildProcessInspection from './pty-child-process-inspection' import * as ptyShellUtils from './pty-shell-utils' import * as processTableSnapshotReader from '../shared/process-table-snapshot-reader' @@ -92,7 +93,7 @@ describe('PtyHandler', () => { }) it('rescans the process table for a close decision but not for a poll', async () => { - const hasChildren = vi.mocked(ptyShellUtils.processHasChildren) + const hasChildren = vi.mocked(ptyChildProcessInspection.processHasChildren) const snapshot = vi .spyOn(processTableSnapshotReader, 'getStrictProcessTableSnapshotWithAge') .mockResolvedValue({ diff --git a/src/relay/pty-handler-test-harness.ts b/src/relay/pty-handler-test-harness.ts index e3d99213ea5..fe6e17c5d9c 100644 --- a/src/relay/pty-handler-test-harness.ts +++ b/src/relay/pty-handler-test-harness.ts @@ -1,6 +1,6 @@ import { vi } from 'vitest' import type { Mock } from 'vitest' -import * as ptyShellUtils from './pty-shell-utils' +import * as ptyChildProcessInspection from './pty-child-process-inspection' import { PtyHandler } from './pty-handler' import type { RelayDispatcher } from './dispatcher' @@ -115,7 +115,7 @@ export function beginPtyHandlerTest(mocks: PtyHandlerTestMocks): { notifyOutput: vi.fn(), dispose: vi.fn() }) - vi.spyOn(ptyShellUtils, 'processHasChildren').mockResolvedValue(false) + vi.spyOn(ptyChildProcessInspection, 'processHasChildren').mockResolvedValue(false) mockPtySpawn.mockReturnValue({ ...mockPtyInstance }) diff --git a/src/relay/pty-handler-windows-child-process-evidence.test.ts b/src/relay/pty-handler-windows-child-process-evidence.test.ts new file mode 100644 index 00000000000..6d723824a04 --- /dev/null +++ b/src/relay/pty-handler-windows-child-process-evidence.test.ts @@ -0,0 +1,132 @@ +// Regression guard for the Windows SSH child-process answer. The relay used to return a hardcoded +// `false` here, which every close guard reads as "nothing is running in this pane" -- so a Windows +// SSH pane running a build closed with no prompt. The answer now comes from the process table, and +// the one thing it may never do again is fabricate a negative. +// +// The second contract is cost. `pty.inspectProcess` is the polled path (750ms/2000ms per tracked +// pane) and a relay host has no `@vscode/windows-process-tree`, so its table read falls back to a +// 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. +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' + +const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ + mockPtySpawn: vi.fn(), + mockCreateShellPromptReadinessProbe: vi.fn(), + mockPtyInstance: { + pid: process.pid, + process: 'xterm-256color', + onData: vi.fn(), + onExit: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + clear: vi.fn(), + pause: vi.fn(), + resume: vi.fn() + } +})) + +vi.mock('node-pty', () => ({ spawn: mockPtySpawn })) + +vi.mock('../main/pty/posix-pty-process-groups', () => ({ + forceKillPosixPtyProcessGroups: vi.fn((_pid: number, fallback: () => void) => fallback()) +})) + +vi.mock('../main/shell-prompt-readiness-probe', () => ({ + createShellPromptReadinessProbe: mockCreateShellPromptReadinessProbe +})) + +import * as ptyChildProcessInspection from './pty-child-process-inspection' +import type { PtyHandler } from './pty-handler' +import { + beginPtyHandlerTest, + createPtyRequestHelpers, + endPtyHandlerTest +} from './pty-handler-test-harness' +import type { MockDispatcher } from './pty-handler-test-harness' + +type Inspection = { + foregroundProcess: string | null + hasChildProcesses: boolean + childProcessEvidence?: string +} + +describe('PtyHandler Windows child-process evidence', () => { + let dispatcher: MockDispatcher + let handler: PtyHandler + let originalPlatform: PropertyDescriptor | undefined + let inspectChildren: ReturnType + + const { spawnPty } = createPtyRequestHelpers(() => dispatcher) + + /** Spawn under the harness's POSIX platform, then answer as the Windows relay would. */ + async function spawnThenBecomeWindows(): Promise { + const { id } = await spawnPty() + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + return id + } + + async function inspect(params: Record): Promise { + return (await dispatcher.callRequest('pty.inspectProcess', params)) as Inspection + } + + beforeEach(() => { + ;({ dispatcher, handler, originalPlatform } = beginPtyHandlerTest({ + mockPtySpawn, + mockPtyInstance, + mockCreateShellPromptReadinessProbe + })) + inspectChildren = vi + .spyOn(ptyChildProcessInspection, 'inspectPtyChildProcesses') + .mockResolvedValue('no-children') + }) + + afterEach(async () => { + await endPtyHandlerTest(handler, originalPlatform) + }) + + it('publishes what the host observed when the caller pays for the scan', async () => { + const id = await spawnThenBecomeWindows() + inspectChildren.mockResolvedValue('children') + + const result = await inspect({ id, scanChildProcesses: true }) + + expect(inspectChildren).toHaveBeenCalledWith(mockPtyInstance.pid) + expect(result.childProcessEvidence).toBe('children') + expect(result.hasChildProcesses).toBe(true) + }) + + it('reports an observed-empty pane as no-children, not merely false', async () => { + const id = await spawnThenBecomeWindows() + inspectChildren.mockResolvedValue('no-children') + + const result = await inspect({ id, scanChildProcesses: true }) + + // Asserted alongside the value so the case fails if the answer stops coming from a real read. + expect(inspectChildren).toHaveBeenCalledWith(mockPtyInstance.pid) + expect(result.childProcessEvidence).toBe('no-children') + expect(result.hasChildProcesses).toBe(false) + }) + + it('keeps the compatibility boolean false when the host could not observe the pane', async () => { + const id = await spawnThenBecomeWindows() + inspectChildren.mockResolvedValue('unverifiable') + + const result = await inspect({ id, scanChildProcesses: true }) + + expect(result.childProcessEvidence).toBe('unverifiable') + // Clients too old to read the verdict also read `true` as "an agent took the PTY, safe to + // type into it", so `unverifiable` must not be promoted to `true` on the shared boolean. + expect(result.hasChildProcesses).toBe(false) + }) + + it('never reads the process table for a poll, and says so instead of guessing', async () => { + const id = await spawnThenBecomeWindows() + + const result = await inspect({ id }) + + expect(inspectChildren).not.toHaveBeenCalled() + expect(result.childProcessEvidence).toBe('unverifiable') + expect(result.hasChildProcesses).toBe(false) + }) +}) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 42f8bdc0a77..190bcabb3f9 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -10,11 +10,11 @@ import type { RelayDispatcher, RequestContext } from './dispatcher' import { resolveDefaultShell, resolveProcessCwd, - processHasChildren, getForegroundProcessName, isProcessAlive, listShellProfiles } from './pty-shell-utils' +import { inspectPtyChildProcesses, processHasChildren } from './pty-child-process-inspection' import { getRelayShellLaunchConfig, isRelayWslShell } from './pty-shell-launch' import { RetiredPaneSurfaceRegistry } from './retired-pane-surfaces' import { addWslEnvKeys } from '../shared/wsl-env' @@ -55,6 +55,7 @@ import { import { isTuiAgent } from '../shared/tui-agent-config' import type { TuiAgent } from '../shared/tui-agent' import { forceKillPosixPtyProcessGroups } from '../main/pty/posix-pty-process-groups' +import type { PtyChildProcessVerdict } from '../shared/terminal-process-inspection' import { terminatePtyJob } from '../main/windows/windows-pty-job' import { stripInheritedBuildModeEnv } from '../main/pty/build-mode-env' import { stripLegacyTerminalShimEnv } from '../main/pty/legacy-terminal-shim-dir' @@ -2610,6 +2611,7 @@ export class PtyHandler { private async inspectProcess(params: Record): Promise<{ foregroundProcess: string | null hasChildProcesses: boolean + childProcessEvidence?: PtyChildProcessVerdict foregroundProcessEvidence?: RemoteForegroundEvidence }> { pruneRetiredPtyIncarnations(this.retiredIncarnations) @@ -2716,14 +2718,28 @@ export class PtyHandler { evidence?.verdict === 'live' ? (evidence.processName ?? managed.pty.process) || null : managed.pty.process || null + // Derive child liveness from the same capture; do not fork a second process-table probe for + // each field/pane in an event burst. + // + // Why Windows is gated on the caller asking: this is the one field whose Windows answer costs + // a process-table read, and `inspectProcess` is the polled path (750ms/2000ms per tracked + // pane). A relay host has no `@vscode/windows-process-tree`, so the read falls back to the + // 1.36s CIM scan, and polling that would reinstate exactly the fork storm the shared table + // exists to prevent (#15209, #15036). Close and cleanup decisions ask for the scan by name; + // a poll gets the honest `unverifiable` instead of a fabricated negative. + const childProcessEvidence: PtyChildProcessVerdict = rows + ? rows.some((row) => row.ppid === managed.pty.pid) + ? 'children' + : 'no-children' + : process.platform === 'win32' && params.scanChildProcesses !== true + ? 'unverifiable' + : await inspectPtyChildProcesses(managed.pty.pid) return { foregroundProcess, - // Derive child liveness from the same capture; do not fork a second - // process-table probe for each field/pane in an event burst. Windows - // has no evidence capture, so preserve the compatibility child probe. - hasChildProcesses: rows - ? rows.some((row) => row.ppid === managed.pty.pid) - : await processHasChildren(managed.pty.pid), + // `unverifiable` keeps spelling itself `false` on the compatibility field, which is what + // every client too old to read the verdict receives. + hasChildProcesses: childProcessEvidence === 'children', + childProcessEvidence, ...(evidence ? { foregroundProcessEvidence: evidence } : {}) } } diff --git a/src/relay/pty-shell-utils.test.ts b/src/relay/pty-shell-utils.test.ts index 95d6a8e050f..94953aae1bc 100644 --- a/src/relay/pty-shell-utils.test.ts +++ b/src/relay/pty-shell-utils.test.ts @@ -14,10 +14,10 @@ vi.mock('child_process', () => ({ import { resetWindowsProcessRowsSnapshotForTests } from '../main/providers/windows-foreground-process-rows' import { __setWindowsProcessTreeLoaderForTests } from '../main/windows/windows-process-table' import { resetProcessTableSnapshotForTests } from '../shared/process-table-snapshot-reader' +import { inspectPtyChildProcesses, processHasChildren } from './pty-child-process-inspection' import { getForegroundProcessName, isProcessAlive, - processHasChildren, resolveDefaultCwd, resolveWindowsDefaultShell } from './pty-shell-utils' @@ -42,6 +42,14 @@ function mockExecFile( * Feed the native Windows snapshot. A real snapshot always contains the * querying process, and the reader rejects a table without it. */ +/** A native reader that answers, but with no snapshot -- an unreadable table, not an empty one. */ +function mockUnreadableWindowsProcessTable(): void { + __setWindowsProcessTreeLoaderForTests(() => ({ + ProcessDataFlag: { None: 0, Memory: 1, CommandLine: 2 }, + getAllProcesses: (cb: (value: undefined) => void) => cb(undefined) + })) +} + function mockWindowsProcessTable( rows: { pid: number; ppid: number; name: string; commandLine?: string }[] ): void { @@ -594,19 +602,79 @@ describe('processHasChildren', () => { }) }) - it('reports no children when the process table is unreadable', async () => { + it('reports an unreadable POSIX table as unverifiable, and still spells it false on the wire', async () => { await withProcessPlatform('linux', async () => { mockExecFile(() => new Error('ps table unavailable')) + await expect(inspectPtyChildProcesses(100)).resolves.toBe('unverifiable') await expect(processHasChildren(100)).resolves.toBe(false) }) }) +}) - it('spawns nothing on Windows, where the answer was always false', async () => { +describe('inspectPtyChildProcesses on Windows', () => { + // Why this describe exists: the relay used to `return false` here unconditionally, and a + // hardcoded negative is indistinguishable from a measurement. Every close guard reads it as + // "nothing is running here", so a Windows SSH pane running a build closed with no prompt. + it('walks the process table rather than answering from nothing', async () => { await withProcessPlatform('win32', async () => { - await expect(processHasChildren(100)).resolves.toBe(false) + mockWindowsProcessTable([ + { pid: 100, ppid: 99, name: 'cmd.exe', commandLine: 'cmd.exe' }, + { pid: 101, ppid: 100, name: 'PING.EXE', commandLine: 'ping -n 40 127.0.0.1' } + ]) - expect(execFileMock).not.toHaveBeenCalled() + await expect(inspectPtyChildProcesses(100)).resolves.toBe('children') + await expect(processHasChildren(100)).resolves.toBe(true) + }) + }) + + it('finds a grandchild the shell backgrounded, not just direct children', async () => { + await withProcessPlatform('win32', async () => { + mockWindowsProcessTable([ + { pid: 100, ppid: 99, name: 'cmd.exe', commandLine: 'cmd.exe' }, + { pid: 101, ppid: 100, name: 'node.exe', commandLine: 'node build.js' }, + { pid: 102, ppid: 101, name: 'tsc.exe', commandLine: 'tsc --watch' } + ]) + + await expect(inspectPtyChildProcesses(102)).resolves.toBe('no-children') + await expect(inspectPtyChildProcesses(101)).resolves.toBe('children') + }) + }) + + it('separates an observed-empty shell from a table it could not read', async () => { + await withProcessPlatform('win32', async () => { + mockWindowsProcessTable([{ pid: 100, ppid: 99, name: 'cmd.exe', commandLine: 'cmd.exe' }]) + await expect(inspectPtyChildProcesses(100)).resolves.toBe('no-children') + + resetWindowsProcessRowsSnapshotForTests() + mockUnreadableWindowsProcessTable() + const alive = vi.spyOn(process, 'kill').mockReturnValue(true as never) + try { + await expect(inspectPtyChildProcesses(100)).resolves.toBe('unverifiable') + // The compatibility boolean keeps spelling unverifiable `false`: it reaches clients that + // cannot read the verdict, and they read `true` as "an agent took the PTY, safe to type". + await expect(processHasChildren(100)).resolves.toBe(false) + } finally { + alive.mockRestore() + } + }) + }) + + it('does not read a missing shell as unverifiable when the kernel says it is gone', async () => { + await withProcessPlatform('win32', async () => { + // The root is absent from the snapshot, which on its own cannot distinguish a filtered + // table from an exited shell. Only ESRCH settles it. + mockWindowsProcessTable([{ pid: 900, ppid: 1, name: 'explorer.exe' }]) + const gone = vi.spyOn(process, 'kill').mockImplementation(() => { + const error = new Error('no such process') as NodeJS.ErrnoException + error.code = 'ESRCH' + throw error + }) + try { + await expect(inspectPtyChildProcesses(100)).resolves.toBe('no-children') + } finally { + gone.mockRestore() + } }) }) }) diff --git a/src/relay/pty-shell-utils.ts b/src/relay/pty-shell-utils.ts index d84faad6ae9..9c8585933a1 100644 --- a/src/relay/pty-shell-utils.ts +++ b/src/relay/pty-shell-utils.ts @@ -11,10 +11,7 @@ import { import { getFirstCommandToken } from '../shared/command-token-scanner' import { getProcessTableIndex, type ProcessTableIndex } from '../shared/process-table-index' import { PS_MAX_BUFFER_BYTES, type ProcessTableRow } from '../shared/process-table-snapshot' -import { - getFreshProcessTableSnapshot, - getProcessTableSnapshot -} from '../shared/process-table-snapshot-reader' +import { getProcessTableSnapshot } from '../shared/process-table-snapshot-reader' import { selectForegroundProcessCandidate } from '../shared/foreground-process-selection' import { resolveOuterWrapperForegroundProcess, @@ -169,43 +166,6 @@ export async function resolveProcessCwd(pid: number, fallbackCwd: string): Promi return fallbackCwd } -/** - * Check whether a process has child processes. - * - * Why the shared snapshot and not `pgrep -P`: this answers one field of - * `pty.inspectProcess`, which every tracked pane polls on a 750ms/2000ms - * cadence, and the fork was neither cached nor coalesced. procps-ng opens six - * procfs files per process to resolve a ppid — including a `/proc//ctty` - * that never exists on Linux — so one call cost O(host process count) syscalls, - * ~4k opens per pgrep on a 690-process host, at up to 8 forks/sec (#13537). - * `getForegroundProcessName` in the same RPC already captured the TTL-cached - * `ps` table, whose index carries the parent/child map, so the answer is free. - * - * `fresh` opts out of that TTL. A poll can read a 500ms-old table because its - * next tick corrects it, but a close or cleanup decision acts on the answer - * once and destructively — a child that started inside the TTL would be killed - * with no confirmation. `pgrep` scanned per call, so anything that decides - * has to keep scanning per call. - */ -export async function processHasChildren( - pid: number, - options?: { fresh?: boolean } -): Promise { - // Windows has no `ps`; the previous `pgrep` fork always failed here too, so - // this keeps the same answer without spawning anything to reach it. - if (process.platform === 'win32') { - return false - } - try { - const rows = options?.fresh - ? await getFreshProcessTableSnapshot() - : await getProcessTableSnapshot() - return (getProcessTableIndex(rows).childrenByPpid.get(pid)?.length ?? 0) > 0 - } catch { - return false - } -} - // Why: signal 0 probes existence without delivering a signal. Only ESRCH ("no // such process") proves the pid is gone; EPERM means it exists but is // unsignalable, so treat every non-ESRCH outcome as alive. Kept conservative so diff --git a/src/renderer/src/components/cmd-j/palette-live-status.test.tsx b/src/renderer/src/components/cmd-j/palette-live-status.test.tsx index a2656104d87..3cc6a9077dd 100644 --- a/src/renderer/src/components/cmd-j/palette-live-status.test.tsx +++ b/src/renderer/src/components/cmd-j/palette-live-status.test.tsx @@ -451,7 +451,7 @@ describe('palette live status', () => { expect(dotLabels()).toEqual(['Needs permission']) }) - it('cuts the pip out of the dialog surface, and out of accent when selected', async () => { + it('keeps the attention glyph knockout popover-colored when its row is selected', async () => { setAgentState('working') await act(async () => { testRoot.render( @@ -469,18 +469,11 @@ describe('palette live status', () => { ) }) - const pip = testContainer.querySelector('[aria-hidden="true"].rounded-full') + const pip = testContainer.querySelector('[aria-hidden="true"]') expect(pip).not.toBeNull() - // Why popover and not background: the CommandDialog surface is --popover (#171717 dark), while - // --background is the app canvas (#0a0a0a) — the mismatch punched a dark halo through each row. expect(pip?.className).toContain('bg-popover') expect(pip?.className).toContain('ring-popover') - expect(pip?.className).not.toContain('bg-background') - expect(pip?.className).toContain( - 'group-data-[selected=true]:bg-[var(--jump-palette-selection-surface)]' - ) - expect(pip?.className).toContain( - 'group-data-[selected=true]:ring-[var(--jump-palette-selection-surface)]' - ) + expect(pip?.className).toContain('rounded-full') + expect(pip?.className).not.toContain('group-data-[selected=true]') }) }) diff --git a/src/renderer/src/components/cmd-j/palette-live-status.tsx b/src/renderer/src/components/cmd-j/palette-live-status.tsx index 432b6c35210..da59663490f 100644 --- a/src/renderer/src/components/cmd-j/palette-live-status.tsx +++ b/src/renderer/src/components/cmd-j/palette-live-status.tsx @@ -9,7 +9,6 @@ import { buildExplicitEntriesByTabId, type TabPaneInputSources } from '@/components/sidebar/smart-attention' -import { cn } from '@/lib/utils' import { isExplicitAgentStatusFresh } from '@/lib/agent-status' import { getLiveAgentStatusByWorktreeId } from '@/lib/worktree-activity-state' import { @@ -255,15 +254,8 @@ export function PaletteRecentTabStatusDot({ {fallback}