Bladeren bron

test(subagent): preserve lane budgets and await teardown completion

Tianyi Cui 2 weken geleden
bovenliggende
commit
421e3b075c

+ 6 - 0
.agents/notes/implemented/testing/2026-09-07-subagent-teardown-test-budgets.i18n.yaml

@@ -0,0 +1,6 @@
+# Bilingual-pair consistency record (docs/i18n/README.md): the git blob hash of each
+# side as of the last confirmed-consistent state. Both languages carry equal authority;
+# after editing either side, bring the other along and re-record with:
+#   pnpm run verify-translation-pairing --write .agents/notes/implemented/testing/2026-09-07-subagent-teardown-test-budgets.md
+2026-09-07-subagent-teardown-test-budgets.md: 2e8c8915813cff54741ebc620caa1df869c1d896
+2026-09-07-subagent-teardown-test-budgets.zh.md: b5b023261f22157fd5e491e2252856560ee7b9a5

+ 28 - 0
.agents/notes/implemented/testing/2026-09-07-subagent-teardown-test-budgets.md

@@ -0,0 +1,28 @@
+# Agent Note: Subagent teardown tests inherit their execution lane budgets
+
+Status: implemented
+
+English | [中文](2026-09-07-subagent-teardown-test-budgets.zh.md)
+
+## Problem
+
+The [Windows coverage run](https://github.com/deepseek-harness/deepseek-harness/actions/runs/34085536250/job/101628739668) reports two teardown failures despite granting tests and hooks 90 seconds. The ACP ignored-EOF test races disposal against its own five-second timer. The real Codex test overrides the hook budget with 30 seconds. Neither deadline tests a product latency guarantee. The Codex body has already observed process-tree exit before its hook fails; the log does not identify whether context disposal, HTTP closure, or temporary-directory removal exceeded the hook budget.
+
+## Decision
+
+The [ACP test](../../../../packages/subagent/subagent-acp/tests/subagent-acp.spec.ts) awaits disposal under the execution lane’s test budget, then checks the actual child outcome. Failure cleanup awaits disposal and child completion before removing the private directory. A deferred exit observation proves that disposal cannot finish merely because termination was requested. The production EOF and termination grace periods remain unchanged.
+
+The [Codex test](../../../../packages/subagent/subagent-codex/tests/real-product.spec.ts) inherits the execution lane’s hook budget. Cleanup captures its contexts, HTTP fixtures, and temporary roots before its first asynchronous wait, so an overdue hook cannot drain resources registered by another test. It preserves context-disposal, server-closure, and directory-removal ordering. Ordinary cleanup errors identify the failing stage while retaining their cause.
+
+The [native Windows CI decision](../process/2026-08-08-native-windows-pull-request-ci.md) continues to own lane scheduling and budgets. This change only removes conflicting local deadlines and strengthens resource-lifetime assertions; it does not establish a Windows process-kill or filesystem defect.
+
+## Alternatives considered
+
+- Increase production grace periods or filesystem retries: the failures do not demonstrate incorrect product timing or exhausted removal retries.
+- Replace local deadlines with larger constants: that would still override future lane budgets.
+- Return from cleanup immediately after requesting termination: that would permit children or sockets to outlive the fixture.
+- Serialize coverage: unrelated tests need not lose concurrency to accommodate two local deadline overrides.
+
+## Consequences
+
+The lane timeout remains a bound on hangs. Focused tests verify observed child completion and cleanup ownership instead of host termination speed. Native Windows runs remain necessary for taskkill, process-exit delivery, and NTFS removal evidence; passing macOS tests cannot prove those mechanisms. No model-visible output, Session fixture, production timeout, or CI routing changes.

+ 28 - 0
.agents/notes/implemented/testing/2026-09-07-subagent-teardown-test-budgets.zh.md

@@ -0,0 +1,28 @@
+# Agent Note: 子代理清理测试继承执行通道的时间预算
+
+Status: implemented
+
+[English](2026-09-07-subagent-teardown-test-budgets.md) | 中文
+
+## 问题
+
+[Windows 覆盖率运行](https://github.com/deepseek-harness/deepseek-harness/actions/runs/34085536250/job/101628739668) 为测试和钩子提供 90 秒预算,却报告了两个清理失败。ACP 忽略 EOF 测试让清理与自设的五秒定时器竞争。真实 Codex 测试将钩子预算覆盖为 30 秒。这两个期限都不用于验证产品延迟保证。Codex 测试正文在钩子失败前已经观察到进程树退出;日志未指出究竟是上下文释放、HTTP 关闭还是临时目录删除超出了钩子预算。
+
+## 决策
+
+[ACP 测试](../../../../packages/subagent/subagent-acp/tests/subagent-acp.spec.ts) 在执行通道的测试预算内等待清理完成,然后检查真实子进程的结果。失败清理先等待释放和子进程完成,再删除私有目录。延迟的退出观察证明,清理不能仅因已请求终止而完成。生产环境的 EOF 与终止宽限期保持不变。
+
+[Codex 测试](../../../../packages/subagent/subagent-codex/tests/real-product.spec.ts) 继承执行通道的钩子预算。清理在第一次异步等待前取得其上下文、HTTP 夹具和临时根目录,因此超时钩子不能取走其他测试注册的资源。清理保留上下文释放、服务器关闭、目录删除的顺序。普通清理错误指出失败阶段并保留原始原因。
+
+[原生 Windows CI 决策](../process/2026-08-08-native-windows-pull-request-ci.zh.md) 继续负责通道调度和预算。本次改动仅移除冲突的局部期限并加强资源生命周期断言;它并不证明 Windows 进程终止或文件系统存在缺陷。
+
+## 曾考虑的替代方案
+
+- 增加生产环境宽限期或文件系统重试次数:这些失败不能证明产品时序错误或删除重试耗尽。
+- 用更大的常量替换局部期限:这样仍会覆盖未来的通道预算。
+- 请求终止后立即结束清理:这样会允许子进程或套接字存活超过夹具的生命周期。
+- 将覆盖率测试串行化:无关测试不应为两个局部期限覆盖而失去并发能力。
+
+## 后果
+
+通道超时仍为挂起提供时间上限。定向测试验证观察到的子进程完成和清理所有权,而不是宿主机终止速度。taskkill、进程退出通知和 NTFS 删除仍需原生 Windows 运行提供证据;macOS 测试通过不能证明这些机制。模型可见输出、Session 夹具、生产环境超时与 CI 路由均不变。

+ 76 - 9
packages/subagent/subagent-acp/tests/subagent-acp.spec.ts

@@ -1,4 +1,4 @@
-import { describe, expect, it } from 'vitest'
+import { describe, expect, it, vi } from 'vitest'
 import { Context } from '@deepseek-ai/cordis'
 import Loader from '@deepseek-ai/cordis-plugin-loader'
 import { chmodSync, existsSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs'
@@ -294,6 +294,57 @@ describe('disposeAcpChild (the backend-owned teardown ladder over seam verbs)',
     expectHostTermination(outcome, 'SIGKILL')
   })
 
+  it('waits for observed tree exit after the EOF grace and termination request', async () => {
+    vi.useFakeTimers()
+    const exited = Promise.withResolvers<boolean>()
+    const stdin = new PassThrough()
+    const calls: string[] = []
+    const child: SubprocessHandle = {
+      pid: 123,
+      stdin,
+      stdout: undefined,
+      stderr: undefined,
+      collected: {},
+      done: exited.promise.then(() => ({ exitCode: 1, signal: null })),
+      terminate: () => { calls.push('terminate') },
+      waitForExit: (signal?: AbortSignal) => {
+        if (signal === undefined) {
+          calls.push('wait for exit')
+          return exited.promise
+        }
+        calls.push('wait for EOF')
+        return new Promise((resolve) => {
+          signal.addEventListener('abort', () => { resolve(false) }, { once: true })
+        })
+      },
+    }
+    let disposal: Promise<void> | undefined
+    try {
+      let disposed = false
+      disposal = disposeAcpChild(child, 150).then(() => { disposed = true })
+      expect(stdin.writableEnded).toBe(true)
+      await vi.advanceTimersByTimeAsync(149)
+      expect(calls).toEqual(['wait for EOF'])
+      await vi.advanceTimersByTimeAsync(1)
+      expect(calls).toEqual(['wait for EOF', 'terminate', 'wait for exit'])
+      // Advancing the clock cannot stand in for the process owner's exit proof.
+      await vi.advanceTimersByTimeAsync(10_000)
+      expect(disposed).toBe(false)
+      exited.resolve(true)
+      await disposal
+      expect(disposed).toBe(true)
+    } finally {
+      exited.resolve(true)
+      try {
+        await vi.runAllTimersAsync()
+        await disposal
+      } finally {
+        stdin.destroy()
+        vi.useRealTimers()
+      }
+    }
+  })
+
   it('observes a spawn-level rejection and returns without a process to reap', async () => {
     const child = spawnSubprocess({
       argv: [process.execPath, '--input-type=module', '--eval', ''],
@@ -859,6 +910,8 @@ describe('dsh-subagent-acp', () => {
     const tmp = mkdtempSync(join(tmpdir(), 'acp-ignore-eof-'))
     const ready = join(tmp, 'ready')
     const sigterm = join(tmp, 'sigterm')
+    let child: SubprocessHandle | undefined
+    let run: Awaited<ReturnType<typeof startAcpRun>> | undefined
     try {
       const spec: AcpRunSpec = {
         command: process.execPath,
@@ -872,18 +925,32 @@ describe('dsh-subagent-acp', () => {
         // Tiny EOF grace so the ignored-EOF window elapses quickly.
         disposeEofGraceMs: 150,
         disposeGraceMs: 2000,
-        spawn: spawnSubprocess,
+        spawn: (spec) => {
+          child = spawnSubprocess(spec)
+          return child
+        },
       }
-      const run = await startAcpRun(request(), spec)
+      run = await startAcpRun(request(), spec)
       await waitForFile(ready)
-      // Bound it so a hang fails loud rather than stalling the suite.
-      await expect(Promise.race([
-        run.dispose(),
-        new Promise((_r, reject) => { setTimeout(() => { reject(new Error('dispose did not return')) }, 5000) }),
-      ])).resolves.toBeUndefined()
+      await run.dispose()
+      const outcome = await child!.done
+      expect(outcome.signal).toBeNull()
+      if (process.platform === 'win32') {
+        expect(outcome.exitCode).not.toBeNull()
+        expect(outcome.exitCode).not.toBe(0)
+      } else {
+        expect(outcome.exitCode).toBe(0)
+      }
       expect(existsSync(sigterm)).toBe(process.platform !== 'win32')
     } finally {
-      rmSync(tmp, { recursive: true, force: true })
+      try {
+        await run?.dispose()
+      } finally {
+        child?.terminate()
+        await child?.waitForExit()
+        await child?.done
+        rmSync(tmp, { recursive: true, force: true })
+      }
     }
   })
 

+ 72 - 0
packages/subagent/subagent-codex/tests/real-product-cleanup.spec.ts

@@ -0,0 +1,72 @@
+import { existsSync, mkdtempSync, rmSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { expect, it, vi } from 'vitest'
+import { cleanupRealProduct } from './real-product-cleanup.ts'
+
+it.each(['context', 'HTTP fixture'] as const)('attributes %s cleanup failures without losing the cause', async (stage) => {
+  const cause = new Error('fixture failure')
+  const fail = (): Promise<void> => Promise.reject(cause)
+  await expect(cleanupRealProduct({
+    contexts: stage === 'context' ? [{ fiber: { dispose: fail } }] : [],
+    fixtures: stage === 'HTTP fixture' ? [{ close: fail }] : [],
+    roots: [],
+  })).rejects.toMatchObject({
+    message: stage === 'context'
+      ? 'Codex test context disposal failed'
+      : 'Codex test HTTP fixture closure failed',
+    cause,
+  })
+})
+
+it('attributes root removal failures to the owned path', async () => {
+  const root = 'invalid\0root'
+  await expect(cleanupRealProduct({ contexts: [], fixtures: [], roots: [root] }))
+    .rejects.toMatchObject({
+      message: `Codex test temporary root removal failed: ${root}`,
+      cause: { code: 'ERR_INVALID_ARG_VALUE' },
+    })
+})
+
+it('keeps resources registered during pending cleanup for their own cleanup', async () => {
+  const oldRoot = mkdtempSync(join(tmpdir(), 'dsh-codex-cleanup-old-'))
+  const nextRoot = mkdtempSync(join(tmpdir(), 'dsh-codex-cleanup-next-'))
+  const releaseContext = Promise.withResolvers<undefined>()
+  const oldContext = { fiber: { dispose: vi.fn(() => releaseContext.promise) } }
+  const nextContext = { fiber: { dispose: vi.fn(async () => {}) } }
+  const oldFixture = { close: vi.fn(async () => {
+    expect(existsSync(oldRoot)).toBe(true)
+  }) }
+  const nextFixture = { close: vi.fn(async () => {}) }
+  const resources: Parameters<typeof cleanupRealProduct>[0] = {
+    contexts: [oldContext], fixtures: [oldFixture], roots: [oldRoot],
+  }
+  const cleanup = cleanupRealProduct(resources)
+  try {
+    expect(oldContext.fiber.dispose).toHaveBeenCalledOnce()
+    expect(oldFixture.close).not.toHaveBeenCalled()
+    resources.contexts.push(nextContext)
+    resources.fixtures.push(nextFixture)
+    resources.roots.push(nextRoot)
+    releaseContext.resolve(undefined)
+    await cleanup
+
+    expect(oldFixture.close).toHaveBeenCalledOnce()
+    expect(existsSync(oldRoot)).toBe(false)
+    expect(nextContext.fiber.dispose).not.toHaveBeenCalled()
+    expect(nextFixture.close).not.toHaveBeenCalled()
+    expect(existsSync(nextRoot)).toBe(true)
+    expect(resources).toEqual({ contexts: [nextContext], fixtures: [nextFixture], roots: [nextRoot] })
+
+    await cleanupRealProduct(resources)
+    expect(nextContext.fiber.dispose).toHaveBeenCalledOnce()
+    expect(nextFixture.close).toHaveBeenCalledOnce()
+    expect(existsSync(nextRoot)).toBe(false)
+    expect(resources).toEqual({ contexts: [], fixtures: [], roots: [] })
+  } finally {
+    releaseContext.resolve(undefined)
+    await cleanup
+    rmSync(oldRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })
+    rmSync(nextRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })
+  }
+})

+ 37 - 0
packages/subagent/subagent-codex/tests/real-product-cleanup.ts

@@ -0,0 +1,37 @@
+import { rm } from 'node:fs/promises'
+import type { Context } from '@deepseek-ai/cordis'
+import type { ResponsesFixture } from './responses-fixture.ts'
+
+interface RealProductResources {
+  contexts: { fiber: Pick<Context['fiber'], 'dispose'> }[]
+  fixtures: Pick<ResponsesFixture, 'close'>[]
+  roots: string[]
+}
+
+/**
+ * Dispose Codex test contexts and HTTP fixtures before removing their files.
+ * Captures all registries before awaiting, so later tests retain their resources.
+ * @param resources - mutable registries of resources owned by the test.
+ */
+export async function cleanupRealProduct(resources: RealProductResources): Promise<void> {
+  const contexts = resources.contexts.splice(0)
+  const fixtures = resources.fixtures.splice(0)
+  const roots = resources.roots.splice(0)
+  try {
+    await Promise.all(contexts.map(ctx => ctx.fiber.dispose()))
+  } catch (cause) {
+    throw new Error('Codex test context disposal failed', { cause })
+  }
+  try {
+    await Promise.all(fixtures.map(fixture => fixture.close()))
+  } catch (cause) {
+    throw new Error('Codex test HTTP fixture closure failed', { cause })
+  }
+  for (const root of roots) {
+    try {
+      await rm(root, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })
+    } catch (cause) {
+      throw new Error(`Codex test temporary root removal failed: ${root}`, { cause })
+    }
+  }
+}

+ 2 - 8
packages/subagent/subagent-codex/tests/real-product.spec.ts

@@ -7,7 +7,6 @@ import {
   readFileSync,
   writeFileSync,
 } from 'node:fs'
-import { rm } from 'node:fs/promises'
 import { createRequire } from 'node:module'
 import { tmpdir } from 'node:os'
 import { delimiter, dirname, join, resolve } from 'node:path'
@@ -31,6 +30,7 @@ import {
   type ResponsesBehavior,
   type ResponsesFixture,
 } from './responses-fixture.ts'
+import { cleanupRealProduct } from './real-product-cleanup.ts'
 
 const execFileAsync = promisify(execFile)
 const packageRoot = resolve(fileURLToPath(new URL('..', import.meta.url)))
@@ -47,13 +47,7 @@ const roots: string[] = []
 const fixtures: ResponsesFixture[] = []
 const contexts: Context[] = []
 
-afterEach(async () => {
-  await Promise.all(contexts.splice(0).map(ctx => ctx.fiber.dispose()))
-  await Promise.all(fixtures.splice(0).map(fixture => fixture.close()))
-  for (const root of roots.splice(0)) {
-    await rm(root, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })
-  }
-}, 30_000)
+afterEach(() => cleanupRealProduct({ contexts, fixtures, roots }))
 
 interface RealHarness {
   readonly ctx: Context