mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(ssh): compare the relay id in the exit proof, and stop requiring a layout to hoist
F1 (P1): isPtyProvenGoneForReplacement compared the proof's id against the APP-scoped ptyId, but the relay formats the RELAY-scoped one it was sent. The comparison could never be true, so the previous commit's 'accept a proof naming this shell' was behaviourally a blanket refusal — the exact stranding it was written to fix. Now compares parsedPtyId.relayPtyId. The function had ZERO tests, which is how an always-false branch survived two review rounds; it is exported and covered now, including the mutation that reinstates the app-id comparison. F2 (P1): requiring a layout to hoist a tab re-created the original tab loss for the CLI/headless cohort. A background or CLI-created tab persists with tab.ptyId and no layout entry until its pane first mounts, and hydration falls back to tab.ptyId for exactly that shape. The orphan-layout direction was already handled independently by the hasTab guard, so the tab-side clause bought nothing.
This commit is contained in:
@@ -0,0 +1,49 @@
|
||||
/**
|
||||
* `isPtyProvenGoneForReplacement` decides whether a failure authorizes destroying a pane's shell
|
||||
* and spawning a replacement. It had no test, and an always-false id comparison shipped through
|
||||
* two review rounds because of it — the relay names a PTY by its RELAY id while the caller holds
|
||||
* the APP-scoped one.
|
||||
*/
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { isPtyProvenGoneForReplacement } from './pty'
|
||||
import { formatPtyExitedError, SSH_SESSION_EXPIRED_ERROR } from '../../shared/ssh-pty-failure-tokens'
|
||||
import { toAppSshPtyId } from '../../shared/ssh-pty-id'
|
||||
|
||||
const TARGET = 'ssh-target-1'
|
||||
const RELAY_PTY = 'pty-7'
|
||||
const APP_PTY = toAppSshPtyId(TARGET, RELAY_PTY)
|
||||
const INCARNATION = '6f1c9a2e-7b4d-4e1a-9c8f-2d5e6a7b8c90'
|
||||
|
||||
describe('what authorizes replacing a remote shell', () => {
|
||||
it('accepts the expiry the reattach mints after verifying the proof', () => {
|
||||
expect(
|
||||
isPtyProvenGoneForReplacement(new Error(`boom ${SSH_SESSION_EXPIRED_ERROR}`), APP_PTY)
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
// The regression: the relay formats the relay-scoped id, so comparing the app-scoped id was
|
||||
// always false and every un-attested exit read as "not proven" — stranding the pane.
|
||||
it('accepts a raw exit proof that names this shell by its relay id', () => {
|
||||
expect(
|
||||
isPtyProvenGoneForReplacement(
|
||||
new Error(formatPtyExitedError(RELAY_PTY, 0, INCARNATION)),
|
||||
APP_PTY
|
||||
)
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it('refuses a raw exit proof about a different shell', () => {
|
||||
expect(
|
||||
isPtyProvenGoneForReplacement(
|
||||
new Error(formatPtyExitedError('pty-9', 0, INCARNATION)),
|
||||
APP_PTY
|
||||
)
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('refuses a bare not-found, which never proves a shell exited', () => {
|
||||
expect(isPtyProvenGoneForReplacement(new Error(`PTY "${RELAY_PTY}" not found`), APP_PTY)).toBe(
|
||||
false
|
||||
)
|
||||
})
|
||||
})
|
||||
+8
-3
@@ -1118,8 +1118,10 @@ function normalizeNodePtySpawnError(err: unknown): Error {
|
||||
* not-found proves an exit only from a provider that owns its ptys; a replacement relay answers the
|
||||
* same for shells its predecessor is still running.
|
||||
*/
|
||||
function isPtyProvenGoneForReplacement(err: unknown, ptyId: string): boolean {
|
||||
if (!parseAppSshPtyId(ptyId)) {
|
||||
/** Exported for test: this had zero coverage, which is how an always-false id comparison shipped. */
|
||||
export function isPtyProvenGoneForReplacement(err: unknown, ptyId: string): boolean {
|
||||
const parsedPtyId = parseAppSshPtyId(ptyId)
|
||||
if (!parsedPtyId) {
|
||||
return isPtyAlreadyGoneError(err)
|
||||
}
|
||||
const message = err instanceof Error ? err.message : String(err)
|
||||
@@ -1134,7 +1136,10 @@ function isPtyProvenGoneForReplacement(err: unknown, ptyId: string): boolean {
|
||||
// panes whose exit really was proven and for which no incarnation exists to check. Require the
|
||||
// proof to name THIS pty: without an incarnation that is the only evidence there is, and an id
|
||||
// mismatch is exactly the case that must not authorize a respawn.
|
||||
return parsePtyExitedError(message)?.id === ptyId
|
||||
// Against the RELAY id: the relay formats `params.id`, which is what the caller sent it, and
|
||||
// callers send the relay-scoped id. Comparing the app-scoped id here never matched, so this
|
||||
// read as a blanket refusal — the stranding it exists to avoid.
|
||||
return parsePtyExitedError(message)?.id === parsedPtyId.relayPtyId
|
||||
}
|
||||
|
||||
function isPtyAlreadyGoneError(err: unknown): boolean {
|
||||
|
||||
@@ -2889,15 +2889,13 @@ export function hoistSshPartitionsIntoLocalSession(
|
||||
for (const [worktreeId, tabs] of Object.entries(partition.tabsByWorktree ?? {})) {
|
||||
const existing = local.tabsByWorktree?.[worktreeId] ?? []
|
||||
const known = new Set(existing.map((tab) => tab.id))
|
||||
// Both halves or neither: field-level salvage can drop a tab while keeping its layout, or
|
||||
// the reverse. A tab with no layout blanks its pane, and an orphan layout is unreachable —
|
||||
// so a partially-salvaged pane stays quarantined in the partition rather than moving the
|
||||
// damage into local.
|
||||
// Deliberately NOT requiring a layout: a background or CLI-created tab persists with
|
||||
// `tab.ptyId` and no layout entry until its pane first mounts, and hydration falls back to
|
||||
// `tab.ptyId` for exactly that shape. Refusing those stranded the CLI/headless cohort —
|
||||
// the original tab loss, for the very users whose live writes this hoist exists to carry.
|
||||
// The orphan-layout direction is handled independently by the `hasTab` guard below.
|
||||
const missing = (tabs ?? []).filter(
|
||||
(tab) =>
|
||||
!known.has(tab.id) &&
|
||||
!alreadyHoisted.has(tab.id) &&
|
||||
Boolean(partition.terminalLayoutsByTabId?.[tab.id])
|
||||
(tab) => !known.has(tab.id) && !alreadyHoisted.has(tab.id)
|
||||
)
|
||||
if (missing.length === 0) {
|
||||
continue
|
||||
|
||||
@@ -225,7 +225,7 @@ describe('an SSH pane whose membership was persisted into the ssh partition', ()
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [LEAF_TWO]: PTY }
|
||||
}
|
||||
}
|
||||
} as typeof second.terminalLayoutsByTabId
|
||||
upgraded.setWorkspaceSession(second as never, SSH_PARTITION)
|
||||
upgraded.flushOrThrow()
|
||||
|
||||
@@ -255,4 +255,23 @@ describe('an SSH pane whose membership was persisted into the ssh partition', ()
|
||||
'an orphan layout was hoisted into local with no tab to reach it'
|
||||
).toBeUndefined()
|
||||
})
|
||||
|
||||
// A background or CLI-created tab persists with `tab.ptyId` and no layout until its pane first
|
||||
// mounts, and hydration falls back to `tab.ptyId` for that shape. Requiring a layout stranded
|
||||
// exactly the CLI/headless cohort whose live writes this hoist exists to carry.
|
||||
it('hoists a tab that has no layout yet', async () => {
|
||||
const before = await createStore()
|
||||
const layoutless = paneSession()
|
||||
layoutless.terminalLayoutsByTabId = {} as typeof layoutless.terminalLayoutsByTabId
|
||||
before.setWorkspaceSession(layoutless as never, SSH_PARTITION)
|
||||
before.flushOrThrow()
|
||||
stripHoistMarker()
|
||||
|
||||
const store = await reopenStore()
|
||||
|
||||
expect(
|
||||
store.getWorkspaceSession().tabsByWorktree?.[WORKTREE] ?? [],
|
||||
'a tab whose pane had not mounted yet was stranded in the partition'
|
||||
).toHaveLength(1)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user