mirror of
https://github.com/deepseek-ai/deepseek-harness.git
synced 2026-09-14 04:01:35 +00:00
test: own the default spill dir in subprocess specs; correct retention wording
Second review round (ds-review-bot v4p/v5/v6): the exit cleanup claim that collectors unlink their spill files on dispose was wrong — completed spill files are retained (seal() only closes, discardSpill() is the only unlink), so the exit removal only ever applies to directories that never spilled. Reword the JSDoc, README, and Agent Note accordingly, drop the redundant readdirSync precheck (rmdirSync ENOTEMPTY is caught anyway), and recount the v8-ignore window. The spawn.spec default-spill test now records the default per-process directory it created and removes it in afterAll, so a completed spill file is never left behind by the unit suite.
This commit is contained in:
@@ -2,5 +2,5 @@
|
||||
# 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-28-test-temp-dir-self-cleanup.md
|
||||
2026-08-28-test-temp-dir-self-cleanup.md: d8c7d6ef3efd0538adfc3b6d0269119f61861e08
|
||||
2026-08-28-test-temp-dir-self-cleanup.zh.md: 1e811dd820d2e5ede62ae7d4fb523e317d2372e7
|
||||
2026-08-28-test-temp-dir-self-cleanup.md: 9d10227e431cd2e8a771f3807f0933ccb084ec4c
|
||||
2026-08-28-test-temp-dir-self-cleanup.zh.md: 01cd53b6f191fe0f43d6579557b3c89fcc408a95
|
||||
|
||||
@@ -15,11 +15,11 @@ Retrofit removal of every `dsh-*` temp dir a spec file creates, at the owning te
|
||||
- Spec files that created dirs without removing any now track each created root in a module-level list and delete the list in `afterEach`/`afterAll` (`rm`/`rmSync` with `recursive: true, force: true`), the convention already used across the session packages. Root-creating helpers (`tmp()`, `tempDir()`, `fakeLauncher()`, harness functions) register the root at creation, so every caller is covered at one point.
|
||||
- Module-scope fixture dirs shared by a whole file (executor spill dirs) are removed in `afterAll` after the last test.
|
||||
- The file list came from the observed-residue inventory on the CI host (a template histogram of current `/tmp/dsh-*` dirs): only spec files whose dirs actually appeared were leak sources. Files that already remove their dirs (agent-team, tool-subagent, list-children, hooks coverage cases) were confirmed clean on the normal-exit path and left unchanged.
|
||||
- Product cleanup is limited to the per-process spill directory of `dsh-subprocess-local/spawn` (`privateSpillDir`): it is removed at a JavaScript-observable process exit while it is EMPTY — collectors unlink their spill files on dispose, so a normal exit leaves at most empty residue, and a directory still holding spill files keeps them (their content may outlive the process). The removal is best-effort (a Windows-held handle must not change the exit code). `dsh-spill-local`'s default root is deliberately NOT exit-deleted: it is covered by the package's own 30-day startup sweep, and the [retention decision](../architecture/2026-07-17-local-spill-startup-cleanup.md) forbids deleting fresh spill artifacts that resumed or forked sessions may still reference.
|
||||
- Product cleanup is limited to the per-process spill directory of `dsh-subprocess-local/spawn` (`privateSpillDir`): it is removed at a JavaScript-observable process exit when it holds no completed spill file — completed spill files are retained as full-output recovery artifacts until an external cleanup, so only directories that never spilled (the dominant residue shape on the CI host: 92% of sampled `dsh-subprocess-*` dirs are empty) are removed. The removal is best-effort (ENOENT/ENOTEMPTY/EBUSY/EPERM must not change the exit code). `dsh-spill-local`'s default root is deliberately NOT exit-deleted: it is covered by the package's own 30-day startup sweep, and the [retention decision](../architecture/2026-07-17-local-spill-startup-cleanup.md) forbids deleting fresh spill artifacts that resumed or forked sessions may still reference.
|
||||
|
||||
## Verification
|
||||
|
||||
- Targeted local run of every changed spec (32 files, 700 tests) passed, including the suites that exercise the changed product sources.
|
||||
- Targeted local runs of every changed unit spec passed (the 35 changed `*.spec.ts` files, exercised in grouped runs), including the suites that exercise the changed product source; the two changed web `*.e2e.ts` files run under the web e2e lane.
|
||||
- CI runs the changed specs on the Linux and Windows coverage lanes; after a full green run, the fixed files' residue templates (observed at up to ~5,000 dirs per two hours each, e.g. `dsh-profile-`, `dsh-app-boot-`, `dsh-presets-*`, `dsh-upload-index-`) should no longer appear in fresh `/tmp` residue on the CI host.
|
||||
|
||||
## Alternatives considered
|
||||
@@ -34,6 +34,6 @@ Not chosen: the files that leak each create roots through their own small helper
|
||||
|
||||
## Consequences
|
||||
|
||||
- Bought: on normal completion — including failed tests — a spec's `dsh-*` dirs are removed at teardown; the empty per-process spill directory of `dsh-subprocess-local` is removed at a JavaScript-observable process exit.
|
||||
- Bought: on normal completion — including failed tests — a spec's `dsh-*` dirs are removed at teardown; a `dsh-subprocess-local` per-process spill directory holding no completed spill file is removed at a JavaScript-observable process exit.
|
||||
- Cost: a process killed with SIGKILL (a cancelled run, a timeout kill) cannot run any in-process teardown; its in-flight residue remains. The machine-side timer stays as the backstop for that path.
|
||||
- Cost: dirs created by a spawned child are covered only when the test knows their paths; product-owned spill dirs holding spill files, and `dsh-spill-local`'s default root, keep their files per the existing retention policy and are cleaned by that package's own sweep.
|
||||
|
||||
@@ -15,11 +15,11 @@ Status: implemented
|
||||
- 创建目录但从不删除的 spec 文件,现在把每个创建的 root 记入模块级列表,并在 `afterEach`/`afterAll` 里删除(`rm`/`rmSync` 带 `recursive: true, force: true`)——与 session 包既有的 `roots.splice(0)` 约定一致。创建 root 的 helper(`tmp()`、`tempDir()`、`fakeLauncher()`、harness 函数)在创建处登记,一个点覆盖全部调用方。
|
||||
- 整文件共享的模块级 fixture 目录(executor spill 目录)在最后一个测试之后的 `afterAll` 里删除。
|
||||
- 目标文件清单来自 CI 主机上的残留实测清单(当前 `/tmp/dsh-*` 目录的模板直方图):只有目录确实出现在残留里的 spec 文件才是泄漏源。已有删除逻辑的文件(agent-team、tool-subagent、list-children、hooks coverage cases)确认在正常结束路径上本来干净,不改。
|
||||
- 产品侧清理限定在 `dsh-subprocess-local/spawn` 的每进程 spill 目录(`privateSpillDir`):在 JavaScript 可观察的进程退出时仅当目录**为空**才删除——collector 在 dispose 时删除自己的 spill 文件,因此正常退出最多留下空目录残留;仍持有 spill 文件的目录会保留(其内容可能比进程更长命)。删除是 best-effort(Windows 上被占用的句柄不得改变退出码)。`dsh-spill-local` 的默认 root **刻意不做**退出删除:该 root 由包自带的 30 天启动 sweep 覆盖,且[保留策略 note](../architecture/2026-07-17-local-spill-startup-cleanup.zh.md)禁止删除 resume/fork 会话仍可能引用的新 spill 产物。
|
||||
- 产品侧清理限定在 `dsh-subprocess-local/spawn` 的每进程 spill 目录(`privateSpillDir`):在 JavaScript 可观察的进程退出时,仅当目录**未持有任何已完成的 spill 文件**才删除——已完成的 spill 文件作为完整输出恢复产物保留到外部清理,因此只有从未 spill 过的目录(CI 主机残留的主流形态:抽样 `dsh-subprocess-*` 目录 92% 为空)会被删除。删除是 best-effort(ENOENT/ENOTEMPTY/EBUSY/EPERM 不得改变退出码)。`dsh-spill-local` 的默认 root **刻意不做**退出删除:该 root 由包自带的 30 天启动 sweep 覆盖,且[保留策略 note](../architecture/2026-07-17-local-spill-startup-cleanup.zh.md)禁止删除 resume/fork 会话仍可能引用的新 spill 产物。
|
||||
|
||||
## Verification
|
||||
|
||||
- 本地定向跑过全部改动 spec(32 个文件、700 个测试)通过,含直接使用改动后产品源码的套件。
|
||||
- 本地定向跑过全部改动单测 spec 通过(35 个改动的 `*.spec.ts` 文件,分组运行),含直接使用改动后产品源码的套件;2 个改动的 web `*.e2e.ts` 由 web e2e lane 承载。
|
||||
- CI 在 Linux 与 Windows coverage lane 跑改动 spec;一次全绿后,被修文件的残留模板(实测每两小时最多各约 5,000 个目录,如 `dsh-profile-`、`dsh-app-boot-`、`dsh-presets-*`、`dsh-upload-index-`)应不再出现在 CI 主机的新鲜 `/tmp` 残留里。
|
||||
|
||||
## Alternatives considered
|
||||
@@ -34,6 +34,6 @@ Sweep 步骤与 timer 只删已存在的残留;本地运行仍会累积,机
|
||||
|
||||
## Consequences
|
||||
|
||||
- 收益:正常结束(含测试失败)时,spec 的 `dsh-*` 目录在 teardown 删除;`dsh-subprocess-local` 的空每进程 spill 目录在 JavaScript 可观察的进程退出时删除。
|
||||
- 收益:正常结束(含测试失败)时,spec 的 `dsh-*` 目录在 teardown 删除;`dsh-subprocess-local` 未持有任何已完成 spill 文件的每进程 spill 目录在 JavaScript 可观察的进程退出时删除。
|
||||
- 代价:被 SIGKILL 的进程(run 被取消、超时被杀)无法运行任何进程内 teardown,飞行中的残留仍在——机器侧 timer 继续兜底该路径。
|
||||
- 代价:子进程创建的目录只有在测试知道其路径时才被覆盖;产品自有、仍持有 spill 文件的目录与 `dsh-spill-local` 的默认 root 按既有保留策略保留文件,由该包自身的 sweep 清理。
|
||||
|
||||
@@ -2,5 +2,5 @@
|
||||
# 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 packages/subprocess/subprocess-local/README.md
|
||||
README.md: 28b61eeec972b3bcd80c91824eaa972d4d4e469b
|
||||
README.zh.md: fce2bf42fdfe8a751b104eb01cad824dfe41cd37
|
||||
README.md: 3727be3ba274838ee5e483652a7d359b57491715
|
||||
README.zh.md: d5a8ae4fcabd5a1e0c3ef10c2c9ae0a85180c5de
|
||||
|
||||
@@ -50,7 +50,7 @@ Collect mode keeps the last `maxBytes` of a stream in memory — errors and fina
|
||||
|
||||
### Shutdown behavior
|
||||
|
||||
Normal disposal terminates every running tree and terminal and awaits their exit. During a JavaScript-observable host exit — direct `process.exit()`, default uncaught exceptions, default unhandled rejections — a synchronous finalization force-terminates everything still owned (SIGKILL to the group, `taskkill /T /F` on Windows) without creating promises or timers. The same exit removes the private per-process spill directory when it is empty; a directory still holding spill files keeps them (their content outlives the process) until an external cleanup. Unhandled `SIGTERM`/`SIGINT`/`SIGHUP`, `SIGKILL`, fatal OOM, native crashes, and power loss need an external supervisor.
|
||||
Normal disposal terminates every running tree and terminal and awaits their exit. During a JavaScript-observable host exit — direct `process.exit()`, default uncaught exceptions, default unhandled rejections — a synchronous finalization force-terminates everything still owned (SIGKILL to the group, `taskkill /T /F` on Windows) without creating promises or timers. The same exit removes the private per-process spill directory when it holds no completed spill file (completed spill files are retained as full-output recovery artifacts until an external cleanup). Unhandled `SIGTERM`/`SIGINT`/`SIGHUP`, `SIGKILL`, fatal OOM, native crashes, and power loss need an external supervisor.
|
||||
|
||||
### What can go wrong
|
||||
|
||||
@@ -127,7 +127,7 @@ These limits define when the provider is a poor fit or needs special operational
|
||||
- **A daemonized terminal descendant can still escape the observable boundary** — on macOS, a child that reparents before any foreground-inspection snapshot is no longer discoverable from the PTY root; on Linux, a `setsid` child leaves both the tree and the owned terminal session; the provider adds no continuous process-table monitor.
|
||||
- **In-process cleanup requires a JavaScript-observable exit** — direct `process.exit()`, default uncaught exceptions, and default unhandled rejections emit Node's synchronous `exit` event; an unhandled `SIGTERM`, `SIGINT`, or `SIGHUP`, `SIGKILL`, fatal OOM, `process.abort()`, native crashes, and power loss require an external supervisor, container init, or equivalent OS owner.
|
||||
- **The credential scrub is a name heuristic** — `*KEY*`/`*PASSWORD*`/`*SECRET*`/`*TOKEN*` only; differently named secrets (for example `*PASSPHRASE*`) pass through, and a whitelist for over-scrubbed variables is noted future work.
|
||||
- **Completed spill files are not deleted** — bounded full-output recovery files accumulate under the OS tmpdir until something external cleans them; the private per-process spill directory is removed at a JavaScript-observable exit once it is empty.
|
||||
- **Completed spill files are not deleted** — bounded full-output recovery files accumulate under the OS tmpdir until something external cleans them; the private per-process spill directory is removed at a JavaScript-observable exit only when it holds no completed spill file.
|
||||
|
||||
<a id="dev-note"></a>
|
||||
### Dev Note
|
||||
|
||||
@@ -50,7 +50,7 @@ kind: "package-reference"
|
||||
|
||||
### 关闭行为
|
||||
|
||||
正常 dispose 会终止每棵仍在运行的进程树与终端并等待其退出。在 JavaScript 可观察的宿主退出期间——直接 `process.exit()`、默认未捕获异常、默认未处理 rejection——同步最终清理会强制终止所有仍归本包所有的对象(对进程组发送 SIGKILL,Windows 上运行 `taskkill /T /F`),且不创建任何 Promise 或定时器。同一退出阶段会删除每进程私有 spill 目录(仅当其为空时);仍持有 spill 文件的目录会保留这些文件(其内容比进程更长命),直到外部机制清理。未处理的 `SIGTERM`/`SIGINT`/`SIGHUP`、`SIGKILL`、fatal OOM、native crash 与断电则需要外部 supervisor。
|
||||
正常 dispose 会终止每棵仍在运行的进程树与终端并等待其退出。在 JavaScript 可观察的宿主退出期间——直接 `process.exit()`、默认未捕获异常、默认未处理 rejection——同步最终清理会强制终止所有仍归本包所有的对象(对进程组发送 SIGKILL,Windows 上运行 `taskkill /T /F`),且不创建任何 Promise 或定时器。同一退出阶段会删除每进程私有 spill 目录(仅当其未持有任何已完成的 spill 文件时;已完成的 spill 文件作为完整输出恢复产物保留,直到外部机制清理)。未处理的 `SIGTERM`/`SIGINT`/`SIGHUP`、`SIGKILL`、fatal OOM、native crash 与断电则需要外部 supervisor。
|
||||
|
||||
### 可能出错的地方
|
||||
|
||||
@@ -127,7 +127,7 @@ spill 文件以 `0600` 权限、`O_EXCL` 与随机名称在 `0700` 每进程目
|
||||
- **守护化的终端后代仍可能逃出可观察边界**——在 macOS 上,子进程如果在任何前台检查快照之前重新设定父进程,将无法再从 PTY 根进程发现;在 Linux 上,调用 `setsid` 的子进程会同时离开进程树与自有终端会话;本提供方不新增持续进程表监视器。
|
||||
- **进程内清理要求退出阶段仍能执行 JavaScript**——直接 `process.exit()`、默认未捕获异常和默认未处理 rejection 会发出 Node 同步 `exit` 事件;未处理的 `SIGTERM`、`SIGINT` 或 `SIGHUP`、`SIGKILL`、fatal OOM、`process.abort()`、native crash 与断电,都需要外部 supervisor、容器 init 或等价的 OS 所有者负责。
|
||||
- **凭据清除依赖名称启发式规则**——只匹配 `*KEY*`/`*PASSWORD*`/`*SECRET*`/`*TOKEN*`;名称不同的 secret(例如 `*PASSPHRASE*`)会继续传递,对误删变量引入白名单属于已记录的后续工作。
|
||||
- **不会删除已完成的 spill 文件**——有界的完整输出恢复文件会在 OS tmpdir 下累积,直到外部机制进行清理;每进程私有 spill 目录在 JavaScript 可观察的退出阶段一旦为空即被删除。
|
||||
- **不会删除已完成的 spill 文件**——有界的完整输出恢复文件会在 OS tmpdir 下累积,直到外部机制进行清理;每进程私有 spill 目录仅在未持有任何已完成 spill 文件时于 JavaScript 可观察的退出阶段删除。
|
||||
|
||||
<a id="dev-note"></a>
|
||||
### 开发备注
|
||||
|
||||
@@ -10,7 +10,7 @@
|
||||
import { type ChildProcess, spawn, spawnSync } from 'node:child_process'
|
||||
import type { Readable } from 'node:stream'
|
||||
import { randomBytes } from 'node:crypto'
|
||||
import { closeSync, mkdtempSync, openSync, readdirSync, rmdirSync, unlinkSync, writeSync } from 'node:fs'
|
||||
import { closeSync, mkdtempSync, openSync, rmdirSync, unlinkSync, writeSync } from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { setTimeout as sleepMs } from 'node:timers/promises'
|
||||
@@ -85,26 +85,24 @@ let defaultSpillDir: string | undefined
|
||||
* The default spill location: a private (0700) per-process directory under
|
||||
* the OS tmpdir, created lazily. Predictable world-readable paths would let
|
||||
* other local users read command output or pre-create symlinks. At a
|
||||
* JavaScript-observable process exit the directory is removed while empty
|
||||
* (collectors unlink their spill files on dispose); a directory still holding
|
||||
* spill files keeps them for external cleanup.
|
||||
* JavaScript-observable process exit the directory is removed only when it
|
||||
* holds no completed spill file (spill files are retained as full-output
|
||||
* recovery artifacts until an external cleanup).
|
||||
*/
|
||||
function privateSpillDir(): string {
|
||||
defaultSpillDir ??= mkdtempSync(join(tmpdir(), 'dsh-subprocess-'))
|
||||
return defaultSpillDir
|
||||
}
|
||||
|
||||
// The per-process spill directory is removed at process exit only while it is
|
||||
// EMPTY: collectors unlink their spill files on dispose, so a normal exit
|
||||
// leaves at most empty residue. A directory still holding spill files keeps
|
||||
// them (their content outlives the process), and a SIGKILLed process cannot
|
||||
// run this at all; both are left to OS temp hygiene.
|
||||
/* v8 ignore next 3 -- exit listeners run after the coverage dump; empty-only removal is verified by the CI /tmp residue measurement. */
|
||||
// The per-process spill directory is removed at process exit when it holds no
|
||||
// completed spill file: a directory that never spilled is empty and is safe to
|
||||
// remove, while a directory holding completed spill files keeps them (their
|
||||
// content is retained until an external cleanup). A SIGKILLed process cannot
|
||||
// run this at all; its residue is left to OS temp hygiene.
|
||||
/* v8 ignore next 4 -- exit listeners run after the coverage dump; removal is verified by the CI /tmp residue measurement. */
|
||||
process.once('exit', () => {
|
||||
if (defaultSpillDir === undefined) return
|
||||
try {
|
||||
if (readdirSync(defaultSpillDir).length === 0) rmdirSync(defaultSpillDir)
|
||||
} catch { /* best-effort: a Windows-held handle must not change the exit code. */ }
|
||||
try { rmdirSync(defaultSpillDir) } catch { /* best-effort: ENOENT/ENOTEMPTY/EBUSY/EPERM must not change the exit code. */ }
|
||||
})
|
||||
|
||||
/**
|
||||
|
||||
@@ -82,8 +82,12 @@ vi.mock('node:fs', async (importOriginal) => {
|
||||
|
||||
const spillDir = mkdtempSync(join(tmpdir(), 'dsh-subprocess-spec-'))
|
||||
|
||||
/** The per-process default spill dir captured by the default-spill test. */
|
||||
let defaultSpillDir: string | undefined
|
||||
|
||||
afterAll(() => {
|
||||
rmSync(spillDir, { recursive: true, force: true })
|
||||
if (defaultSpillDir !== undefined) rmSync(defaultSpillDir, { recursive: true, force: true })
|
||||
})
|
||||
|
||||
type SpecOverrides = Partial<Parameters<typeof spawnSubprocess>[0]> & {
|
||||
@@ -1090,6 +1094,7 @@ describe('environment and spill-file hardening', () => {
|
||||
spec('for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', { stdoutMaxBytes: 500, stderrMaxBytes: 500 }),
|
||||
))
|
||||
const dir = dirname(result.stdout.spillPath!)
|
||||
defaultSpillDir = dir
|
||||
expect(dir).toMatch(/dsh-subprocess-/)
|
||||
const mode = statSync(dir).mode & 0o777
|
||||
expect(mode).toBe(0o700)
|
||||
|
||||
Reference in New Issue
Block a user