Просмотр исходного кода

fix(pty): close retained lifecycle review gaps

Tianyi Cui 2 месяцев назад
Родитель
Сommit
90ac883512

+ 1 - 1
docs/config-catalog.md

@@ -803,7 +803,7 @@ Source: [`packages/plan/plan-mode/src/index.ts:57`](../packages/plan/plan-mode/s
 
 ## `@deepseek-ai/dsh-pty-local`
 
-Requires: `agents` · `pty` · `sandbox` · `sandboxPolicy`
+Requires: `pty` · `sandbox` · `sandboxPolicy`
 
 ```ts config-catalog
 /** Public plugin configuration. */

+ 2 - 2
packages/pty/pty-local/README.md

@@ -4,11 +4,11 @@ Local `node-pty` backend for `ctx.pty`. It starts an interactive shell under the
 
 ## Plugin (`pty-local`)
 
-The plugin injects `agents`, `pty`, `sandbox`, and `sandboxPolicy`, then registers the configured backend type (`shell`). `danger-full-access` starts the shell directly; confined modes wrap the exact shell argv through `ctx.sandbox`. The effective session mode is resolved at spawn. A change to a different effective mode is rejected before its `sandbox/mode` event commits while that owner has an open PTY or a spawn in progress; wait for creation to settle and close the sessions before changing modes, so a terminal opened with wider access cannot survive a downgrade.
+The plugin injects `pty`, `sandbox`, and `sandboxPolicy`, then registers the configured backend type (`shell`). `danger-full-access` starts the shell directly; confined modes wrap the exact shell argv through `ctx.sandbox`. The effective session mode is resolved at spawn. A change to a different effective mode is rejected before its `sandbox/mode` event commits while that owner has an open PTY or a spawn in progress; the fence is attached to the exact owner and therefore outlives a local-provider reload that retains existing sessions. Wait for creation to settle and close the sessions before changing modes, so a terminal opened with wider access cannot survive a downgrade.
 
 Linux readiness combines a foreground-verified private bash prompt marker, foreground-process-group syscall inspection, silence fallback, and absolute timeout. macOS uses the verified prompt marker plus silence/timeout because it has no `/proc` syscall surface. A marker is not ready until printable prompt text arrives, including when the OSC marker and `PS1` are split across data callbacks. Unrecognized or unreadable process state is never a positive exact-idle signal. During unpublished startup, a fallback requires observed output; zero-output silence cannot publish an empty session, and timeout rejects the spawn. Incomplete terminal-control sequences are bounded by `maxReadBytes` and discarded through their terminator after crossing that limit; a trailing carriage return is carried across callbacks so split CRLF becomes one newline.
 
-Send cancellation resolves the current foreground process group and delivers a real `SIGINT`; it never emulates interruption by writing `\x03`, so raw-mode programs remain cancellable. Close sends `SIGTERM` to descendants, waits, rescans and sends `SIGKILL` to the remaining tree, verifies that descendants left the process table while the shell can still reap them, and only then stops the shell. A survivor failure does not cache a permanently rejected close; a later close retries the teardown.
+Send cancellation resolves the current foreground process group and delivers a real `SIGINT`; it never emulates interruption by writing `\x03`, so raw-mode programs remain cancellable. Close sends `SIGTERM` to descendants, waits, then sends `SIGKILL` to the union of captured survivors and newly scanned descendants so reparenting cannot hide a process from teardown. It verifies that every retained identity left the process table while the shell can still reap it and only then stops the shell. A survivor failure does not cache a permanently rejected close; a later close retries the teardown.
 
 ## Model Experience
 

+ 31 - 15
packages/pty/pty-local/src/index.ts

@@ -7,6 +7,7 @@
 import { Context } from 'cordis'
 import * as nodePty from 'node-pty'
 import type { IPtyForkOptions } from 'node-pty'
+import type { Agent } from '@deepseek-ai/dsh-agent'
 import type { Session, SessionEvent } from '@deepseek-ai/dsh-session'
 import type { PtyBackend, PtyBackendSpawnSpec } from '@deepseek-ai/dsh-pty'
 import type { SandboxMode } from '@deepseek-ai/dsh-sandbox'
@@ -21,10 +22,37 @@ export type { Config as PtyLocalConfig } from './config.ts'
 
 /** Cordis plugin name. */
 export const name = 'pty-local'
-/** Required services: owner/PTY registries plus the one shared confinement policy. */
-export const inject = ['agents', 'pty', 'sandbox', 'sandboxPolicy']
+/** Required services: PTY registry plus the one shared confinement policy. */
+export const inject = ['pty', 'sandbox', 'sandboxPolicy']
 
 const SENSITIVE_ENV_PATTERN = /KEY|SECRET|TOKEN/i
+interface SandboxModeFenceState {
+  pty: Context['pty']
+  sandboxPolicy: Context['sandboxPolicy']
+}
+
+const sandboxModeFences = new WeakMap<Agent, SandboxModeFenceState>()
+
+function ensureSandboxModeFence(ctx: Context, owner: Agent): void {
+  const existing = sandboxModeFences.get(owner)
+  if (existing !== undefined) {
+    existing.pty = ctx.pty
+    existing.sandboxPolicy = ctx.sandboxPolicy
+    return
+  }
+  const state: SandboxModeFenceState = { pty: ctx.pty, sandboxPolicy: ctx.sandboxPolicy }
+  sandboxModeFences.set(owner, state)
+  owner.ctx.on('internal/dispatch', (_mode, eventName, args) => {
+    if (eventName !== 'session/event') return
+    const [session, event] = args as [Session, SessionEvent]
+    if (session !== owner.session || event.type !== 'sandbox/mode') return
+    const currentMode = effectiveSandboxMode(session.events) ?? state.sandboxPolicy.defaultMode
+    if (event.data.mode === currentMode || !state.pty.hasOwnerActivity(owner)) return
+    throw new Error(
+      `cannot change sandbox mode from "${currentMode}" to "${event.data.mode}" while persistent terminal sessions are open or being created; wait for creation to settle and close them first`,
+    )
+  }, { global: true })
+}
 
 function childEnvironment(spec: PtyBackendSpawnSpec): NodeJS.ProcessEnv {
   const env: NodeJS.ProcessEnv = {}
@@ -75,6 +103,7 @@ export class LocalPtyBackend implements PtyBackend {
 
   async spawn(spec: PtyBackendSpawnSpec): Promise<LocalPtySession> {
     spec.signal?.throwIfAborted()
+    ensureSandboxModeFence(this.ctx, spec.owner)
     const argv = spawnArgv(this.ctx, this.config, spec)
     const file = argv[0]
     if (file === undefined) throw new Error('pty-local: sandbox returned empty argv')
@@ -106,17 +135,4 @@ export function apply(ctx: Context, config: Config): void {
   validateConfig(config)
   const inspector = createProcessInspector()
   ctx.pty.registerBackend(new LocalPtyBackend(ctx, config, inspector))
-  ctx.on('internal/dispatch', (_mode, eventName, args) => {
-    if (eventName !== 'session/event') return
-    const [session, event] = args as [Session, SessionEvent]
-    if (event.type !== 'sandbox/mode') return
-    const currentMode = effectiveSandboxMode(session.events) ?? ctx.sandboxPolicy.defaultMode
-    if (event.data.mode === currentMode) return
-    const owner = ctx.agents.get(session.id)
-    if (owner === undefined) return
-    if (!ctx.pty.hasOwnerActivity(owner)) return
-    throw new Error(
-      `cannot change sandbox mode from "${currentMode}" to "${event.data.mode}" while persistent terminal sessions are open or being created; wait for creation to settle and close them first`,
-    )
-  }, { global: true })
 }

+ 22 - 7
packages/pty/pty-local/src/session.ts

@@ -394,16 +394,31 @@ export class LocalPtySession implements PtyBackendSession {
     }
   }
 
+  private unionMembers(...groups: ProcessIdentity[][]): ProcessIdentity[] {
+    const members: ProcessIdentity[] = []
+    const seen = new Set<string>()
+    for (const group of groups) {
+      for (const member of group) {
+        const key = JSON.stringify([member.pid, member.started])
+        if (seen.has(key)) continue
+        seen.add(key)
+        members.push(member)
+      }
+    }
+    return members
+  }
+
   private async stopDescendants(): Promise<ProcessIdentity[]> {
-    let members = this.descendants()
-    this.signalMembers(members, 'SIGTERM')
-    await this.waitForExit(members)
+    const captured = this.descendants()
+    this.signalMembers(captured, 'SIGTERM')
+    const capturedSurvivors = await this.waitForExit(captured)
     // A TERM-handling descendant may have forked while winding down. Rescan
-    // while the shell can still reap every member, then kill the fresh tree.
-    members = this.descendants()
+    // while the shell can still reap every member, then kill both the fresh
+    // tree and captured survivors that were reparented out of that tree.
+    const members = this.unionMembers(capturedSurvivors, this.descendants())
     this.signalMembers(members, 'SIGKILL')
-    await this.waitForExit(members)
-    return this.descendants().filter(member => this.inspector.isAlive(member))
+    const survivors = await this.waitForExit(members)
+    return this.survivors(this.unionMembers(survivors, this.descendants()))
   }
 
   private async stopShell(): Promise<void> {

+ 51 - 29
packages/pty/pty-local/tests/index.spec.ts

@@ -9,7 +9,6 @@ import SandboxProvider from '@deepseek-ai/dsh-sandbox'
 import type { ConfinedArgv, SandboxPolicy } from '@deepseek-ai/dsh-sandbox'
 import SandboxPolicyService, { setSandboxMode } from '@deepseek-ai/dsh-sandbox-policy'
 import PtyService, { PtySessionId } from '@deepseek-ai/dsh-pty'
-import type { PtyBackendSession } from '@deepseek-ai/dsh-pty'
 import { LocalPtyBackend } from '@deepseek-ai/dsh-pty-local'
 import * as ptyLocal from '@deepseek-ai/dsh-pty-local'
 import type { ResolvedConfig } from '@deepseek-ai/dsh-pty-local/src/config.ts'
@@ -64,6 +63,30 @@ function spec(owner: Agent, signal?: AbortSignal) {
   }
 }
 
+function stubLocalSession(initialize: () => Promise<void> = () => Promise.resolve()): LocalPtySession {
+  return {
+    motd: '',
+    initialize,
+    startSend: () => { throw new Error('unused') },
+    read: () => { throw new Error('unused') },
+    signal: () => Promise.resolve({ delivered: true, targetPgid: 1 }),
+    status: () => ({ kind: 'running' as const }),
+    close: () => Promise.resolve(),
+  } as unknown as LocalPtySession
+}
+
+function registerStubLocalBackend(ctx: Context, createSession: () => LocalPtySession) {
+  return ctx.inject(['pty', 'sandbox', 'sandboxPolicy'], (providerCtx) => {
+    providerCtx.pty.registerBackend(new LocalPtyBackend(
+      providerCtx,
+      { ...config(), backendType: 'stub' },
+      inspector,
+      (() => ({})) as never,
+      createSession,
+    ))
+  })
+}
+
 describe('LocalPtyBackend startup rollback', () => {
   it('rejects pre-aborted setup and empty sandbox argv', async () => {
     const ctx = new Context()
@@ -172,7 +195,7 @@ describe('pty-local plugin shape', () => {
     const loader = Object.create(Loader.prototype) as Loader
     const unwrapped = loader.unwrapExports(ptyLocal) as Record<string, unknown>
     expect(unwrapped.name).toBe('pty-local')
-    expect(unwrapped.inject).toEqual(['agents', 'pty', 'sandbox', 'sandboxPolicy'])
+    expect(unwrapped.inject).toEqual(['pty', 'sandbox', 'sandboxPolicy'])
     expect(unwrapped.Config).toBeDefined()
   })
 
@@ -204,39 +227,45 @@ describe('pty-local plugin shape', () => {
     expect(() => { setSandboxMode(session, 'read-only') }).not.toThrow()
   })
 
-  it('rejects an effective sandbox-mode change until the owner closes live terminals', async () => {
+  it('keeps the owner-lifetime sandbox fence after the local provider unloads', async () => {
     const ctx = new Context()
     await ctx.plugin(SessionStore)
     await ctx.plugin(AgentRegistry)
     await ctx.plugin(PtyService)
-    await ctx.plugin(EmptySandbox)
+    await ctx.plugin(RecordingSandbox)
     await ctx.plugin(SandboxPolicyService, { mode: 'danger-full-access', workspaceRoot: '/tmp' })
-    await ctx.plugin(ptyLocal, config())
 
     const session = ctx.sessions.create(SessionId('mode-owner'))
+    const ownerFiber = await ctx.plugin(() => {})
     const owner: Agent = {
-      id: session.id, options: {}, session, status: 'idle', ctx,
+      id: session.id, options: {}, session, status: 'idle', ctx: ownerFiber.ctx,
       send() {}, steer() {}, inject() {}, cancel() {}, whenIdle: () => Promise.resolve(),
     }
     ctx.agents.register(owner)
-    const backendSession = {
-      motd: '',
-      startSend: () => { throw new Error('unused') },
-      read: () => { throw new Error('unused') },
-      signal: () => Promise.resolve({ delivered: true, targetPgid: 1 }),
-      status: () => ({ kind: 'running' as const }),
-      close: () => Promise.resolve(),
-    } satisfies PtyBackendSession
-    ctx.pty.registerBackend({ type: 'stub', spawn: () => Promise.resolve(backendSession) })
+    const providerFiber = await registerStubLocalBackend(ctx, () => stubLocalSession())
     const created = await ctx.pty.spawn(owner, { type: 'stub' })
 
+    const unrelated = ctx.sessions.create(SessionId('unrelated-mode'))
+    expect(() => { setSandboxMode(unrelated, 'read-only') }).not.toThrow()
+    expect(() => {
+      session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } })
+    }).not.toThrow()
+
     expect(() => { setSandboxMode(session, 'danger-full-access') }).not.toThrow()
+    await providerFiber.dispose()
+    expect(ctx.pty.listBackends()).toEqual([])
     expect(() => { setSandboxMode(session, 'read-only') }).toThrow(
       'cannot change sandbox mode from "danger-full-access" to "read-only" while persistent terminal sessions are open or being created; wait for creation to settle and close them first',
     )
     expect(session.events.filter(event => event.type === 'sandbox/mode')).toHaveLength(1)
 
+    const replacementFiber = await registerStubLocalBackend(ctx, () => stubLocalSession())
+    const second = await ctx.pty.spawn(owner, { type: 'stub' })
+    await replacementFiber.dispose()
+    expect(() => { setSandboxMode(session, 'read-only') }).toThrow('open or being created')
+
     await ctx.pty.kill(owner, created.sessionId)
+    await ctx.pty.kill(owner, second.sessionId)
     expect(() => { setSandboxMode(session, 'read-only') }).not.toThrow()
     expect(session.events.filter(event => event.type === 'sandbox/mode')).toHaveLength(2)
   })
@@ -246,30 +275,23 @@ describe('pty-local plugin shape', () => {
     await ctx.plugin(SessionStore)
     await ctx.plugin(AgentRegistry)
     await ctx.plugin(PtyService)
-    await ctx.plugin(EmptySandbox)
+    await ctx.plugin(RecordingSandbox)
     await ctx.plugin(SandboxPolicyService, { mode: 'danger-full-access', workspaceRoot: '/tmp' })
-    await ctx.plugin(ptyLocal, config())
 
     const session = ctx.sessions.create(SessionId('pending-mode-owner'))
+    const ownerFiber = await ctx.plugin(() => {})
     const owner: Agent = {
-      id: session.id, options: {}, session, status: 'idle', ctx,
+      id: session.id, options: {}, session, status: 'idle', ctx: ownerFiber.ctx,
       send() {}, steer() {}, inject() {}, cancel() {}, whenIdle: () => Promise.resolve(),
     }
     ctx.agents.register(owner)
-    const gate = Promise.withResolvers<PtyBackendSession>()
-    ctx.pty.registerBackend({ type: 'slow', spawn: () => gate.promise })
-    const spawning = ctx.pty.spawn(owner, { type: 'slow' })
+    const gate = Promise.withResolvers<undefined>()
+    await registerStubLocalBackend(ctx, () => stubLocalSession(() => gate.promise))
+    const spawning = ctx.pty.spawn(owner, { type: 'stub' })
 
     expect(ctx.pty.hasOwnerActivity(owner)).toBe(true)
     expect(() => { setSandboxMode(session, 'read-only') }).toThrow('open or being created')
-    gate.resolve({
-      motd: '',
-      startSend: () => { throw new Error('unused') },
-      read: () => { throw new Error('unused') },
-      signal: () => Promise.resolve({ delivered: true, targetPgid: 1 }),
-      status: () => ({ kind: 'running' as const }),
-      close: () => Promise.resolve(),
-    })
+    gate.resolve(undefined)
     const created = await spawning
     await ctx.pty.kill(owner, created.sessionId)
     expect(ctx.pty.hasOwnerActivity(owner)).toBe(false)

+ 22 - 0
packages/pty/pty-local/tests/session.spec.ts

@@ -415,6 +415,28 @@ describe('LocalPtySession bounds, signals, and teardown', () => {
     expect(terminal.kills).toEqual(['SIGTERM'])
   })
 
+  it('retains captured survivors that are reparented out of the teardown rescan', async () => {
+    vi.useFakeTimers()
+    const terminal = new FakeTerminal()
+    const inspector = new FakeInspector()
+    const captured = { pid: 124, started: 'captured' }
+    let reads = 0
+    inspector.alive.add(captured.pid)
+    inspector.processTree = () => reads++ === 0 ? [captured] : []
+    inspector.signalProcess = (identity, signal) => {
+      inspector.processes.push([identity.pid, signal])
+      if (signal === 'SIGKILL') inspector.alive.delete(identity.pid)
+    }
+    const session = new LocalPtySession(terminal.asPty(), inspector, config({ disposeGraceMs: 20 }))
+
+    const closing = session.close('test')
+    await vi.advanceTimersByTimeAsync(25)
+    await closing
+
+    expect(inspector.processes).toEqual([[124, 'SIGTERM'], [124, 'SIGKILL']])
+    expect(terminal.kills).toEqual(['SIGTERM'])
+  })
+
   it('allows teardown to retry after a descendant-survivor failure', async () => {
     vi.useFakeTimers()
     const terminal = new FakeTerminal()

+ 10 - 2
packages/tasks/tool-tasks/src/index.ts

@@ -171,10 +171,10 @@ export function apply(ctx: Context, config: Config): void {
     },
     execute(args, exec) {
       const id = validateTaskId(args.task_id)
+      const snapshot = ctx.tasks.get(id, exec.agent)
       const result = ctx.tasks.kill(id, exec.agent, args.reason)
       if (result === 'already-finished') {
         // A snapshot describes terminal state without consuming pending output.
-        const snapshot = ctx.tasks.get(id, exec.agent)
         return Promise.resolve([{
           type: 'text',
           text: fitWithSuffix(
@@ -185,7 +185,15 @@ export function apply(ctx: Context, config: Config): void {
           ),
         }])
       }
-      return Promise.resolve([{ type: 'text', text: `requested cancellation of task ${id}` }])
+      return Promise.resolve([{
+        type: 'text',
+        text: fitWithSuffix(
+          `requested cancellation of task ${id}`,
+          '',
+          snapshot.outputLimitBytes,
+          '\n[notice truncated]',
+        ),
+      }])
     },
     presentCall: args => presentTaskCall(`Kill background task ${args.task_id}`, 'execute', args.task_id),
   }))

+ 10 - 0
packages/tasks/tool-tasks/tests/tool-tasks.spec.ts

@@ -213,6 +213,16 @@ describe('task_kill', () => {
     expect(p.cancels).toEqual(['superseded'])
   })
 
+  it('applies the producer output limit to a cancellation acknowledgement', async () => {
+    const { ctx } = await setup()
+    const p = producer({ outputLimitBytes: 8 })
+    ctx.tasks.start(p.spec)
+
+    const result = await call(ctx, 'task_kill', { task_id: 'bash-1' })
+    expect(Buffer.byteLength(text(result))).toBeLessThanOrEqual(8)
+    expect(p.cancels).toEqual([undefined])
+  })
+
   it('reports an already-finished task without consuming its pending delta', async () => {
     const { ctx } = await setup()
     let delta = 'unread tail'