Browse Source

Merge pull request #3354 from deepseek-harness/fix/windows-coverage-flaky-test-budgets

test: stabilize the Windows coverage lane against timing flakes
Chinesezjc 1 week ago
parent
commit
c5a94f11df

+ 6 - 0
.agents/notes/implemented/process/2026-08-31-windows-coverage-flaky-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/process/2026-08-31-windows-coverage-flaky-test-budgets.md
+2026-08-31-windows-coverage-flaky-test-budgets.md: 475bd79adf2721202d860d3c7bc86e1e4b68d6c2
+2026-08-31-windows-coverage-flaky-test-budgets.zh.md: 2a16ba8c6ea983d7ad457eb579c9e5e367cf4d68

+ 51 - 0
.agents/notes/implemented/process/2026-08-31-windows-coverage-flaky-test-budgets.md

@@ -0,0 +1,51 @@
+# Agent Note: deterministic assertions and dispose budgets for the Windows coverage lane
+
+Status: implemented
+
+English | [中文](2026-08-31-windows-coverage-flaky-test-budgets.zh.md)
+
+## Problem
+
+The `windows node 24 / coverage` lane is excluded from `all-checks-passed.needs` because it is unstable, not because its verdict is unimportant. The instability is a set of timing-sensitive tests that pass on a quiet runner and fail on a contended one. Two failure shapes recur across many PRs (3184, 3185, 3179, 3181) and are unrelated to the PR diffs that trigger them:
+
+1. `packages/session/session-projection-cache/tests/cache.spec.ts` — `SessionProjectionCache` writes are fail-soft and fire-and-forget (the event listener calls `void flushSoft(...)`, `coldSnapshot` calls `void this.put(...)`). Six tests asserted the durable outcome after a fixed `settle()` of 40 ms. On a contended runner the write does not drain within 40 ms, so the mock assertion fails with `AssertionError: expected "Mock" to be called with arguments: [ StringContaining{…} ]` at the `expect(warn).toHaveBeenCalledWith(...)` lines, or the stored-row assertion reads a stale cut. These are the two cache.spec cases that fail on every affected run.
+2. `packages/sdk/client/tests/sdk-client.spec.ts` and `packages/subagent/subagent-dsh-sdk/tests/subagent-dsh-sdk.spec.ts` — dispose ladder tests launch a real child process and pass tight confirmation budgets (`disposeGraceMs: 100`–`300`). On a contended runner the child's exit edge after SIGKILL can arrive after the budget, so `close()` rejects with `runtime process did not exit within 100ms after SIGKILL` even though the child was reaped correctly. The product defaults are `disposeEofGraceMs: 6000` / `disposeGraceMs: 3000`; the tight values were test-only speed choices that misreport slow reaps as dispose failures.
+
+A separate, historical coverage gap in `packages/workflow/workflow-worker-thread` (host.ts/index.ts below the per-file 100% gate) was tracked as part of this lane's instability. Investigation in this change found that the local reproduction was a DSH-session environment artifact, not a code defect: the session exports `TSX_TSCONFIG_PATH` pointing at the DSH staging checkout's tsconfig, which redirects the tsx-in-worker resolution of workspace bare specifiers to the staging copy and drops their named exports. With `TSX_TSCONFIG_PATH` unset, `workflow-worker-thread.spec.ts` passes 54/54. The Windows-side reports of that gap predate the ReFS clone install (#3342) and have not recurred since; any recurrence needs Windows-side per-line uncovered lists before it can be attributed.
+
+## Decision
+
+Replace every fixed-wait assertion in cache.spec.ts with `vi.waitFor` polling of the observable outcome, with a 5 s timeout (the same pattern the file already used for the cold-read write-back cases since `7746ed64f0`). The two mock-assertion cases poll for the warning call itself:
+
+```ts ignore-check
+await vi.waitFor(() => {
+  expect(warn).toHaveBeenCalledWith(expect.stringContaining('turn/end write for "fail-soft" failed'))
+}, { timeout: 5_000 })
+```
+
+The polling assertion still fails when the condition never becomes true — the negative case (an impossible string) times out and fails the test — so the fail-soft contract stays enforced.
+
+For the dispose-ladder tests, pass the product-default budgets instead of the tight test-only values:
+
+- `sdk-client.spec.ts`: `disposeGraceMs` `100` → `3_000` (bounds-profile case), `1_000` → `3_000` (SIGTERM ladder), `300` → `3_000` (SIGKILL escalation).
+- `subagent-dsh-sdk.spec.ts`: the concurrent diagnostic-isolation case uses `DEFAULT_SHUTDOWN_TIMEOUT_MS` / `DEFAULT_DISPOSE_EOF_GRACE_MS` / `DEFAULT_DISPOSE_GRACE_MS` from `run.ts` instead of `100`/`200`/`200`.
+
+The dispose.spec.ts negative cases (`disposeGraceMs: 10` with a fake child that never exits) still verify that a truly stuck child fails the ladder within its budget; only the real-child tests with overly tight budgets were widened.
+
+## Verification
+
+- cache.spec.ts: 17/17 pass locally; negative case (impossible warning string) fails via the `vi.waitFor` timeout.
+- sdk-client.spec.ts: 42/42 pass locally; dispose.spec.ts 16/16 pass (the 10 ms refused/accepted negative cases still fail correctly).
+- subagent-dsh-sdk.spec.ts: 55/55 pass locally.
+- workflow-worker-thread.spec.ts: 54/54 pass locally with `TSX_TSCONFIG_PATH` unset — no code change made for the historical coverage gap.
+- CI on this PR: the windows coverage lane should stop failing on these tests.
+
+## Alternatives considered
+
+**Keep the fixed settle windows and rerun flaky lanes.** Reruns eventually pass, but every affected PR pays a re-run cycle and the lane stays excluded from `all-checks-passed.needs`. The polled assertion costs nothing when the write is prompt and removes the timing dependency entirely, matching the file's existing `vi.waitFor` pattern from `7746ed64f0`.
+
+**Keep the tight dispose budgets and treat SIGKILL timeouts as runner faults.** A truly stuck child must still fail the ladder, which the fake-child negative cases in dispose.spec.ts already cover at 10 ms. The real-child cases were widened to the product defaults because they measure the ladder's escalation, not a performance bound, and a contended runner's exit edge is not a code defect.
+
+## Consequences
+
+The windows coverage lane keeps its per-file 100% gate while its tests no longer depend on a 40 ms wall-clock window or a 100–300 ms SIGKILL confirmation. The two cache.spec mock-assertion cases and the concurrent subagent-dsh-sdk case stop failing under runner contention, so the lane's flake rate drops without weakening any assertion: every polled condition still fails on timeout, and every dispose negative case still bounds a stuck child.

+ 51 - 0
.agents/notes/implemented/process/2026-08-31-windows-coverage-flaky-test-budgets.zh.md

@@ -0,0 +1,51 @@
+# Agent Note:Windows coverage lane 的确定性断言与 dispose 预算
+
+Status: implemented
+
+[English](2026-08-31-windows-coverage-flaky-test-budgets.md) | 中文
+
+## Problem
+
+`windows node 24 / coverage` lane 不在 `all-checks-passed.needs` 里,是因为它不稳定,而不是它的结论不重要。不稳定来自一组对时序敏感的测试:在空闲 runner 上通过,在争抢的 runner 上失败。两种失败形态在多个 PR(3184、3185、3179、3181)反复出现,与触发它们的 PR diff 无关:
+
+1. `packages/session/session-projection-cache/tests/cache.spec.ts` —— `SessionProjectionCache` 的写入是 fail-soft 且 fire-and-forget(事件监听器调 `void flushSoft(...)`,`coldSnapshot` 调 `void this.put(...)`)。六个测试在固定 `settle()` 40 ms 后断言持久化结果。争抢的 runner 上写入 40 ms 内没有排空,mock 断言以 `AssertionError: expected "Mock" to be called with arguments: [ StringContaining{…} ]` 失败(在 `expect(warn).toHaveBeenCalledWith(...)` 行),或 stored-row 断言读到陈旧 cut。这正是每次受影响 run 都失败的 cache.spec 两个用例。
+2. `packages/sdk/client/tests/sdk-client.spec.ts` 与 `packages/subagent/subagent-dsh-sdk/tests/subagent-dsh-sdk.spec.ts` —— dispose 梯子测试启动真实子进程并传入紧的确认预算(`disposeGraceMs: 100`–`300`)。争抢的 runner 上 SIGKILL 后子进程的退出边缘可能晚于预算到达,于是 `close()` 以 `runtime process did not exit within 100ms after SIGKILL` reject,即使子进程已被正确回收。产品默认是 `disposeEofGraceMs: 6000` / `disposeGraceMs: 3000`;紧值是测试只为提速的选择,却把慢回收误报成 dispose 失败。
+
+另有一个历史性的覆盖率缺口在 `packages/workflow/workflow-worker-thread`(host.ts/index.ts 低于 per-file 100% 门禁),曾被当作本 lane 不稳定的一部分跟踪。本次调查发现本机复现是 DSH 会话的环境假象而非代码缺陷:会话导出了指向 DSH staging checkout tsconfig 的 `TSX_TSCONFIG_PATH`,把 tsx-in-worker 对 workspace bare specifier 的解析重定向到 staging 副本并丢掉了 named exports。unset `TSX_TSCONFIG_PATH` 后 `workflow-worker-thread.spec.ts` 54/54 通过。Windows 侧对该缺口的报告早于 ReFS clone 安装(#3342),此后未再出现;任何复发都需要 Windows 侧逐行未覆盖清单才能归因。
+
+## Decision
+
+把 cache.spec.ts 里每个固定等待断言改成用 `vi.waitFor` 轮询可观察结果,超时 5 s(与该文件自 `7746ed64f0` 起在 cold-read write-back 用例中使用的模式一致)。两个 mock 断言用例轮询警告调用本身:
+
+```ts ignore-check
+await vi.waitFor(() => {
+  expect(warn).toHaveBeenCalledWith(expect.stringContaining('turn/end write for "fail-soft" failed'))
+}, { timeout: 5_000 })
+```
+
+轮询断言在条件永远不成立时仍会失败——负例(不可能的字符串)超时并使测试失败——所以 fail-soft 契约仍被强制。
+
+对 dispose 梯子测试,改用产品默认预算,去掉只属于测试的紧值:
+
+- `sdk-client.spec.ts`:`disposeGraceMs` `100` → `3_000`(bounds-profile 用例)、`1_000` → `3_000`(SIGTERM 梯子)、`300` → `3_000`(SIGKILL 升级)。
+- `subagent-dsh-sdk.spec.ts`:并发诊断隔离用例改用 `run.ts` 的 `DEFAULT_SHUTDOWN_TIMEOUT_MS` / `DEFAULT_DISPOSE_EOF_GRACE_MS` / `DEFAULT_DISPOSE_GRACE_MS`,而不是 `100`/`200`/`200`。
+
+dispose.spec.ts 的负例(`disposeGraceMs: 10`,fake child 永不退出)仍验证真正卡住的子进程会在预算内使梯子失败;只有真实子进程用例的过紧预算被放宽。
+
+## Verification
+
+- cache.spec.ts:本地 17/17 通过;负例(不可能的警告字符串)经 `vi.waitFor` 超时失败。
+- sdk-client.spec.ts:本地 42/42 通过;dispose.spec.ts 16/16 通过(10 ms refused/accepted 负例仍正确失败)。
+- subagent-dsh-sdk.spec.ts:本地 55/55 通过。
+- workflow-worker-thread.spec.ts:unset `TSX_TSCONFIG_PATH` 后本地 54/54 通过——历史覆盖率缺口未改代码。
+- CI on this PR:windows coverage lane 应不再因这些测试失败。
+
+## Alternatives considered
+
+**保留固定 settle 窗口并重跑 flaky lane。** 重跑最终会通过,但每个受影响的 PR 都要付出一次重跑周期,lane 仍被排除在 `all-checks-passed.needs` 之外。轮询断言在写入及时时零成本,并完全消除时序依赖,与该文件自 `7746ed64f0` 起已有的 `vi.waitFor` 模式一致。
+
+**保留紧 dispose 预算并把 SIGKILL 超时当作 runner 故障。** 真正卡住的子进程仍必须让梯子失败——dispose.spec.ts 的 fake-child 负例已用 10 ms 覆盖。真实子进程用例放宽到产品默认,因为它们测的是梯子的升级路径而不是性能上限,争抢 runner 上的退出边缘不是代码缺陷。
+
+## Consequences
+
+windows coverage lane 保留 per-file 100% 门禁,而其测试不再依赖 40 ms 墙钟窗口或 100–300 ms 的 SIGKILL 确认。cache.spec 两个 mock 断言用例与 subagent-dsh-sdk 并发用例在 runner 争抢下不再失败,lane 的 flake 率下降而不削弱任何断言:每个轮询条件超时仍失败,每个 dispose 负例仍约束卡住的子进程。

+ 6 - 3
packages/sdk/client/tests/sdk-client.spec.ts

@@ -308,7 +308,10 @@ describe('HarnessClient', () => {
         description: 'dsh profile "profile-without-sdk-server"',
         initializeTimeoutMs: 50,
         disposeEofGraceMs: 100,
-        disposeGraceMs: 100,
+        // Wide SIGKILL confirmation: the hang-init child may still be
+        // starting up on a contended runner when close() escalates, so a
+        // tight window misreports a slow reap as a dispose failure.
+        disposeGraceMs: 3_000,
       },
     ))
     cleanups.push(() => client.close())
@@ -416,7 +419,7 @@ describe('HarnessClient', () => {
     const sigtermFile = join(dir, 'sigterm.txt')
     const client = processClient(fakeLaunch(
       { FAKE_IGNORE_EOF: '1', FAKE_SIGTERM_FILE: sigtermFile },
-      { shutdownTimeoutMs: 100, disposeEofGraceMs: 100, disposeGraceMs: 1_000 },
+      { shutdownTimeoutMs: 100, disposeEofGraceMs: 100, disposeGraceMs: 3_000 },
     ))
     await client.initialize({ cwd: process.cwd(), provider: 'p', model: 'm' })
     await client.close()
@@ -430,7 +433,7 @@ describe('HarnessClient', () => {
   it('escalates to SIGKILL when the runtime traps SIGTERM too', async () => {
     const client = processClient(fakeLaunch(
       { FAKE_IGNORE_EOF: '1', FAKE_TRAP_SIGTERM: '1' },
-      { shutdownTimeoutMs: 100, disposeEofGraceMs: 100, disposeGraceMs: 300 },
+      { shutdownTimeoutMs: 100, disposeEofGraceMs: 100, disposeGraceMs: 3_000 },
     ))
     await client.initialize({ cwd: process.cwd(), provider: 'p', model: 'm' })
     // Resolves (does not hang or reject): the SIGKILL rung reaped the child.

+ 38 - 24
packages/session/session-projection-cache/tests/cache.spec.ts

@@ -141,9 +141,6 @@ async function seedRecord(
   await writeFile(path, JSON.stringify({ version: projectionCacheDomainSpec.version, record: { identity, rows } }))
 }
 
-/** Wait until queued fail-soft writes (event-listener fire-and-forget over real fs I/O) drain. */
-const settle = () => new Promise(resolve => setTimeout(resolve, 40))
-
 afterEach(async () => {
   vi.useRealTimers()
   await Promise.all(contexts.splice(0).map(ctx => ctx.fiber.dispose()))
@@ -157,12 +154,14 @@ describe('SessionProjectionCache write policy', () => {
     mark(session, ['a'])
     // Creation already wrote the init cut; the mark is throttled, so the
     // stored row is still the creation-time cut (no marks folded).
-    await settle()
-    expect((await storedRows(root, session.id))?.['cache-test/marks']?.seq).toBe(-1)
+    await vi.waitFor(async () => {
+      expect((await storedRows(root, session.id))?.['cache-test/marks']?.seq).toBe(-1)
+    }, { timeout: 5_000 })
     const end = endTurn(session)
-    await settle()
-    const rows = await storedRows(root, session.id)
-    expect(rows?.['cache-test/marks']).toEqual({ ver: 1, seq: end.seq, val: { marks: ['a'] } })
+    await vi.waitFor(async () => {
+      expect((await storedRows(root, session.id))?.['cache-test/marks'])
+        .toEqual({ ver: 1, seq: end.seq, val: { marks: ['a'] } })
+    }, { timeout: 5_000 })
   })
 
   it('writes a checkpoint at session creation, capturing the seed-derived cut', async () => {
@@ -173,9 +172,10 @@ describe('SessionProjectionCache write policy', () => {
     const session = ctx.sessions.create(SessionId('seeded'), {
       seed: [{ type: 'cache-test/mark', seq: 0, time: 1, data: { marks: ['seed'] } }] as SessionEvent[],
     })
-    await settle()
-    expect((await storedRows(root, session.id))?.['cache-test/marks']?.val)
-      .toEqual({ marks: ['seed'] })
+    await vi.waitFor(async () => {
+      expect((await storedRows(root, session.id))?.['cache-test/marks']?.val)
+        .toEqual({ marks: ['seed'] })
+    }, { timeout: 5_000 })
   })
 
   it('writes at session disposal (detach, the live-to-cold moment)', async () => {
@@ -188,8 +188,10 @@ describe('SessionProjectionCache write policy', () => {
     if (session === undefined) throw new Error('session was not created')
     mark(session, ['live'])
     await owner.dispose()
-    await settle()
-    expect((await storedRows(root, session.id))?.['cache-test/marks']?.val).toEqual({ marks: ['live'] })
+    const detached = session
+    await vi.waitFor(async () => {
+      expect((await storedRows(root, detached.id))?.['cache-test/marks']?.val).toEqual({ marks: ['live'] })
+    }, { timeout: 5_000 })
   })
 
   it('flushes when the in-turn event count reaches the configured threshold', async () => {
@@ -197,11 +199,13 @@ describe('SessionProjectionCache write policy', () => {
     const session = ctx.sessions.create(SessionId('count'))
     mark(session, ['1'])
     mark(session, ['2'])
-    await settle()
-    expect((await storedRows(root, session.id))?.['cache-test/marks']?.seq).toBe(-1) // still the creation cut
+    await vi.waitFor(async () => {
+      expect((await storedRows(root, session.id))?.['cache-test/marks']?.seq).toBe(-1) // still the creation cut
+    }, { timeout: 5_000 })
     mark(session, ['3'])
-    await settle()
-    expect((await storedRows(root, session.id))?.['cache-test/marks']?.val).toEqual({ marks: ['3'] })
+    await vi.waitFor(async () => {
+      expect((await storedRows(root, session.id))?.['cache-test/marks']?.val).toEqual({ marks: ['3'] })
+    }, { timeout: 5_000 })
   })
 
   it('flushes on the configured interval when the count threshold is not reached', async () => {
@@ -270,15 +274,22 @@ describe('SessionProjectionCache write policy', () => {
     const session = ctx.sessions.create(SessionId('fail-soft'))
     mark(session, ['x'])
     endTurn(session)
-    await settle()
-    expect(await storedRows(root, session.id)).toBeUndefined()
-    expect(warn).toHaveBeenCalledWith(expect.stringContaining('turn/end write for "fail-soft" failed'))
+    // The failed creation/turn-end writes are fire-and-forget: wait for the
+    // warn (the write actually failed), then assert no row landed — the
+    // property under test is that a failed write leaves no partial row.
+    await vi.waitFor(() => {
+      expect(warn).toHaveBeenCalledWith(expect.stringContaining('turn/end write for "fail-soft" failed'))
+    }, { timeout: 5_000 })
+    await vi.waitFor(async () => {
+      expect(await storedRows(root, session.id)).toBeUndefined()
+    }, { timeout: 5_000 })
     // Self-heal: once the blocker clears, the next mandatory point writes.
     await rm(recordPath(root, session.id), { recursive: true })
     mark(session, ['y'])
     endTurn(session)
-    await settle()
-    expect((await storedRows(root, session.id))?.['cache-test/marks']?.val).toEqual({ marks: ['y'] })
+    await vi.waitFor(async () => {
+      expect((await storedRows(root, session.id))?.['cache-test/marks']?.val).toEqual({ marks: ['y'] })
+    }, { timeout: 5_000 })
   })
 })
 
@@ -473,7 +484,10 @@ describe('SessionProjectionCache cold-read seeding', () => {
     const meta = headerOf(SessionId('cold-fail'))
     await mkdir(recordPath(root, meta.id), { recursive: true })
     expect(ctx.sessionProjectionCache.coldSnapshot(meta, [])).toBeDefined()
-    await settle()
-    expect(warn).toHaveBeenCalledWith(expect.stringContaining('cold-read write-back for "cold-fail" failed'))
+    // The failed write-back is fire-and-forget: poll for the warn instead of
+    // assuming a fixed settle window (slow runners exceed it).
+    await vi.waitFor(() => {
+      expect(warn).toHaveBeenCalledWith(expect.stringContaining('cold-read write-back for "cold-fail" failed'))
+    }, { timeout: 5_000 })
   })
 })

+ 5 - 3
packages/subagent/subagent-dsh-sdk/tests/subagent-dsh-sdk.spec.ts

@@ -621,9 +621,11 @@ describe('dsh-subagent-dsh-sdk provider', () => {
       provider: 'p',
       model: 'm',
       env: { FAKE_REASON_KIND: reason },
-      shutdownTimeoutMs: 100,
-      disposeEofGraceMs: 200,
-      disposeGraceMs: 200,
+      // Product-default dispose budgets: two real children are reaped under
+      // runner contention, where tight windows misreport slow SIGKILL reaps.
+      shutdownTimeoutMs: DEFAULT_SHUTDOWN_TIMEOUT_MS,
+      disposeEofGraceMs: DEFAULT_DISPOSE_EOF_GRACE_MS,
+      disposeGraceMs: DEFAULT_DISPOSE_GRACE_MS,
     })
     const [errored, unknown] = await Promise.all([start('error'), start('unknown-reason')])
     const [errorResult, unknownResult] = await Promise.all([errored.result, unknown.result])