From 75a36959fd7ade657582c5060bd2619387917531 Mon Sep 17 00:00:00 2001 From: SivanCola <32437197+SivanCola@users.noreply.github.com> Date: Tue, 27 Oct 2026 10:48:46 +0800 Subject: [PATCH] fix(proc): retain published command job ownership Problem: Repeated cancellation or shell reaping can discard an established Windows Job Object owner and perform a new PID-based stop attempt. Root cause: TrackedCommand removes its one-shot TrackedJob on the first kill and also starts a redundant PID tree observer after successful job assignment. Fix: Keep the published owner across repeated and late cancellation, and let an established Job Object own its tree. Preserve existing non-job fallbacks. Add native Windows owned-child probes with a harmless taskkill recorder and an exact preceding-owner counterfactual inventory gate. Verification: Root inventory reconciles 239 packages, 24060 test/subtest passes and 215 existing skips. Original Boot command hit its 15m package alarm; all 845 top-level tests close through the recorded disjoint remainder groups without a timeout increase. Complete proc and shell race suites pass. Root vet, full lint, make check and actionlint pass. Windows new/old owners compile and Windows vet passes; actual native regressions remain a hosted gate. No full PID-identity, descendant-quiescence or physical-scope claim. --- .github/workflows/ci.yml | 26 +++ docs/PUBLISHED_COMMAND_JOB_OWNER.md | 92 +++++++++ .../base/proc/published_job_windows_test.go | 176 ++++++++++++++++++ internal/base/proc/run.go | 11 +- .../14025-published-command-job-owner.md | 17 ++ scripts/published-job-evidence.mjs | 50 +++++ 6 files changed, 366 insertions(+), 6 deletions(-) create mode 100644 docs/PUBLISHED_COMMAND_JOB_OWNER.md create mode 100644 internal/base/proc/published_job_windows_test.go create mode 100644 release-notes/studio/unreleased/14025-published-command-job-owner.md create mode 100644 scripts/published-job-evidence.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7fd807de55..19fb2134f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -223,6 +223,32 @@ jobs: WINDOWS_SANDBOX_WAIT_MS: "20000" run: go test -count=1 -p 4 -timeout=18m ./internal/runtime/agent/... ./internal/platform/appidentity/... ./internal/assembly/boot/... ./internal/state/checkpoint/... ./internal/frontend/cli/... ./internal/session/control/... ./internal/base/filelock/... ./internal/base/fileutil/... ./internal/ext/hook/... ./internal/state/instruction/... ./internal/ext/mcplaunch/... ./internal/platform/notify/... ./internal/base/proc/... ./internal/platform/remote/... ./internal/platform/repair/... ./internal/safety/sandbox/... ./internal/base/sysproxy/... ./internal/state/workspacelease/... ./cmd/... + - name: test (Windows published command job owner) + if: env.RUN_STEPS == 'true' && runner.os == 'Windows' && github.event_name == 'pull_request' + shell: bash + run: | + set -euo pipefail + evidence="$RUNNER_TEMP/published-job-owner" + mkdir -p "$evidence" + go test -count=1 -timeout=3m -json -run '^TestPublishedJob' ./internal/base/proc | tee "$evidence/positive.jsonl" + node scripts/published-job-evidence.mjs positive "$evidence/positive.jsonl" + node scripts/published-job-evidence.mjs overlay "$evidence" + set +e + go test -count=1 -timeout=3m -json -overlay="$evidence/overlay.json" -run '^TestPublishedJob' ./internal/base/proc > "$evidence/negative.jsonl" + old_result=$? + set -e + test "$old_result" -eq 1 + node scripts/published-job-evidence.mjs negative "$evidence/negative.jsonl" + + - name: upload Windows published command job owner evidence + if: always() && env.RUN_STEPS == 'true' && runner.os == 'Windows' && github.event_name == 'pull_request' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7 + with: + name: published-job-owner-windows-${{ github.run_attempt }} + path: ${{ runner.temp }}/published-job-owner/ + if-no-files-found: error + retention-days: 7 + - name: test (Windows isolation owners) if: env.RUN_STEPS == 'true' && runner.os == 'Windows' && github.event_name == 'pull_request' timeout-minutes: 25 diff --git a/docs/PUBLISHED_COMMAND_JOB_OWNER.md b/docs/PUBLISHED_COMMAND_JOB_OWNER.md new file mode 100644 index 0000000000..3a684c9a40 --- /dev/null +++ b/docs/PUBLISHED_COMMAND_JOB_OWNER.md @@ -0,0 +1,92 @@ +--- +owner: @SivanCola +backup: @esengine +status: active +reviewed: 2026-10-27 +--- + +# Published command Job Object owner / 已发布命令的 Job Object 所有权 + +## Behavior / 行为 + +Repeated cancellation of a tracked Windows command keeps the same published +`TrackedJob`. Its one-shot release closes the Job Object once; later cancellation +uses the original Go process object instead of invoking PID-based `taskkill`. +A successfully assigned Job Object also replaces the auxiliary PID tree walker. + +Windows 跟踪命令重复取消时保留同一个已发布 `TrackedJob`。Job Object 只关闭一次； +后续取消使用原 Go 进程对象，不再因丢失 owner 而调用按 PID 寻址的 `taskkill`。 +成功分配 Job Object 后，也不再启动额外的 PID 树扫描器。 + +The shared owner is `proc.RunCommand`, used by the shell tool. Cancellation, +bounded cleanup retries and shell reaping all reach `TrackedCommand.Kill`. + +Keeping the released guard matters after `cmd.Wait`: a PID can eventually be +reused after its process object is released. See Microsoft's +[process identifier lifetime explanation](https://devblogs.microsoft.com/oldnewthing/20110107-00/?p=11803). + +共享 owner 是 shell 工具调用的 `proc.RunCommand`。取消、有限重试清理和 shell +收尾都会调用 `TrackedCommand.Kill`。`cmd.Wait` 后仍须保留已释放的 guard：进程 +对象释放后，PID 最终可能被重用；不能把数字继续当作原命令身份。 + +## Mechanism boundary / 机制边界 + +| Route | Contract / 约定 | +| --- | --- | +| Published Windows Job Object | Keep its one-shot owner; no auxiliary PID walker / 保留一次性 owner，不启动额外 PID 扫描 | +| Late published Job Object after cancellation | Retain and release that same owner / 保存并释放同一 owner | +| Unix process group or unavailable Windows Job Object | Existing fallback and retry behavior remains / 沿用原回退与重试 | +| Untracked command and direct KillTree | Existing PID-based Windows fallback remains / 原按 PID 回退仍在 | +| Standalone TreeTracker | Generation refresh, identity checks and termination handles need separate work / 代际刷新、身份检查与终止句柄另需闭环 | +| Launch failure before owner publication | Existing cleanup remains / 沿用原启动失败清理 | + +This does not establish complete descendant quiescence or a recovery receipt. +Unknown execution scope still needs retention and diagnosis. No permission, +configuration, API, persisted format, cancellation budget, model request or +cache-stable prefix changes. There is no demonstrated PID-reuse incident here. + +本修改不证明后代进程完全静止，也不产生恢复凭证。执行范围未知时仍须保留资源并 +诊断。不改权限、配置、API、持久化格式、取消预算、模型请求或缓存稳定前缀。 +这里没有已复现的 PID 重用或误杀事故。 + +## Verification / 验证 + +Windows fixtures launch only the test's own helper process through `RunCommand`. +A renamed test executable records `taskkill` arguments without terminating any +PID. + +After normal or canceled waiting returns the published owner, the probe is +reset and repeated reaping must leave no marker. Stop attempts before that +returned-owner boundary are outside this assertion. An explicit legacy fallback +control must create a marker. + +Windows 回归只通过 `RunCommand` 启动测试自有子进程。重命名的测试程序只记录 +`taskkill` 参数，不终止任何 PID。正常或取消等待返回已发布 owner 后重置探针， +重复收尾不得产生标记；该返回边界之前的停止尝试不在此断言范围。显式旧回退对照 +必须产生标记。 + +The dedicated Windows gate runs unchanged fixtures against the new owner and an +overlay reversing the ownership changes. It records the reconstructed SHA-256. + +For this unit that digest must match the exact preceding owner, +`61b4e1f88e32322fa287b9d1ae71edeb0cd7f2511e0894cbd811d49794590c3a`, +before qualifying its native counterfactual evidence. + +Later unrelated owner edits remain in the reconstruction. Its digest then +describes an ownership counterfactual, not this complete historical file. + +It requires six named +pass events for the new owner, four expected fail events and two passing controls +for the preceding owner, plus each package terminal. + +Parent and helper events are +included; these are not six independent cases. Cross-compilation alone does not +qualify this native behavior. + +专用 Windows 门禁运行新 owner 与反转所有权修改的 overlay，记录重建文件 SHA-256， +测试保持相同。本单元原生反例资格还须核对它等于上述精确旧 owner 的摘要。 +以后无关 owner 修改保留在重建文件中；届时它是所有权反例，不冒称完整历史文件。 + +新版须有六个具名通过事件；反例须有四个预期失败事件和两个通过对照，且均 +核对包终态。计数包含父测试和辅助测试，不是六个独立用例；交叉编译不能替代原生 +行为验证。 diff --git a/internal/base/proc/published_job_windows_test.go b/internal/base/proc/published_job_windows_test.go new file mode 100644 index 0000000000..250d92c536 --- /dev/null +++ b/internal/base/proc/published_job_windows_test.go @@ -0,0 +1,176 @@ +//go:build windows + +package proc + +import ( + "bufio" + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" +) + +const publishedJobMarker = "REASONIX_PUBLISHED_JOB_TASKKILL_MARKER" + +// A renamed copy records a legacy PID fallback without terminating any PID. +func TestMain(m *testing.M) { + if strings.EqualFold(filepath.Base(os.Args[0]), "taskkill.exe") && os.Getenv(publishedJobMarker) != "" { + if err := os.WriteFile(os.Getenv(publishedJobMarker), []byte(strings.Join(os.Args[1:], " ")), 0o600); err != nil { + os.Exit(2) + } + os.Exit(0) + } + os.Exit(m.Run()) +} + +func publishedJobStub(t *testing.T) string { + t.Helper() + dir := t.TempDir() + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(exe) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "taskkill.exe"), data, 0o700); err != nil { + t.Fatal(err) + } + marker := filepath.Join(dir, "fallback.txt") + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv(publishedJobMarker, marker) + return marker +} + +func publishedJobCommand(t *testing.T, ctx context.Context, hold bool) *exec.Cmd { + t.Helper() + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + cmd := exec.CommandContext(ctx, exe, "-test.run=^TestPublishedJobChild$", "-test.timeout=30s") + mode := "return" + if hold { + mode = "hold" + } + cmd.Env = append(os.Environ(), "REASONIX_PUBLISHED_JOB_CHILD="+mode) + HideWindow(cmd) + return cmd +} + +func TestPublishedJobChild(t *testing.T) { + mode := os.Getenv("REASONIX_PUBLISHED_JOB_CHILD") + if mode == "" { + return + } + fmt.Println("PUBLISHED-JOB-READY") + if mode == "hold" { + _, _ = bufio.NewReader(os.Stdin).ReadString('\n') + } +} + +func TestPublishedJobNoPIDFallback(t *testing.T) { + for _, hold := range []bool{false, true} { + name := "after_wait" + if hold { + name = "cancelled_wait" + } + t.Run(name, func(t *testing.T) { + marker := publishedJobStub(t) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + cmd := publishedJobCommand(t, ctx, true) + input, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + defer input.Close() + pipe, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if cmd.Process != nil { + _ = cmd.Process.Kill() + } + }) + type result struct { + tracked *TrackedCommand + err error + } + done := make(chan result, 1) + go func() { p, e := RunCommand(ctx, cmd, RunOptions{Track: true}); done <- result{p, e} }() + line, err := bufio.NewReader(pipe).ReadString('\n') + if err != nil || strings.TrimSpace(line) != "PUBLISHED-JOB-READY" { + t.Fatalf("child readiness=%q %v", line, err) + } + if hold { + cancel() + } else if _, err := fmt.Fprintln(input, "release"); err != nil { + t.Fatal(err) + } + var got result + select { + case got = <-done: + case <-time.After(10 * time.Second): + t.Fatal("owned child did not finish") + } + if hold && got.err == nil { + t.Fatalf("cancel=%v", got.err) + } + if !hold && got.err != nil { + t.Fatal(got.err) + } + if got.tracked == nil || !got.tracked.Diagnostics().JobObjectCreated { + t.Fatal("required native fixture has no Job Object") + } + // The assertion starts with the returned owner, after launch publication. + if err := os.Remove(marker); err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + got.tracked.Kill() + got.tracked.Kill() + if data, err := os.ReadFile(marker); !os.IsNotExist(err) { + t.Fatalf("published Job Object fell through to PID taskkill: %q %v", data, err) + } + }) + } +} + +func TestPublishedJobUsesKernelTree(t *testing.T) { + cmd := publishedJobCommand(t, t.Context(), false) + tracked, err := RunCommand(t.Context(), cmd, RunOptions{Track: true}) + if err != nil { + t.Fatal(err) + } + if tracked == nil || !tracked.Diagnostics().JobObjectCreated { + t.Fatal("required native fixture has no Job Object") + } + tracked.mu.Lock() + owner := tracked.job + tracked.mu.Unlock() + if owner != nil { + t.Cleanup(func() { owner.Kill(cmd) }) + } + if tracked.Diagnostics().TreeTrackerStarted { + t.Fatal("owned Job Object also started a PID tree walker") + } +} + +func TestPublishedJobFallbackControl(t *testing.T) { + marker := publishedJobStub(t) + cmd := publishedJobCommand(t, t.Context(), false) + if err := cmd.Run(); err != nil { + t.Fatal(err) + } + KillTree(cmd) + data, err := os.ReadFile(marker) + if err != nil || !strings.Contains(string(data), "/PID") { + t.Fatalf("legacy control did not reach the recording stub: %q %v", data, err) + } +} diff --git a/internal/base/proc/run.go b/internal/base/proc/run.go index 228afbdb15..2d86b50fff 100644 --- a/internal/base/proc/run.go +++ b/internal/base/proc/run.go @@ -75,7 +75,9 @@ func RunCommand(ctx context.Context, cmd *exec.Cmd, opts RunOptions) (*TrackedCo return tracked, err } tracked.setStarted(job) - tracked.setTree(TrackTree(cmd)) + if !job.Tracked() { + tracked.setTree(TrackTree(cmd)) + } return tracked, waitForTrackedCommand(ctx, tracked, cmd.Wait, opts.CancelWaitGrace, opts.CancelRetryInterval, opts.CancelRetryFor) } @@ -180,10 +182,9 @@ func (p *TrackedCommand) Kill() { p.killed = true p.diag.KillCalls++ job := p.job - p.job = nil tree := p.tree p.mu.Unlock() - if !firstKill { + if !firstKill && !job.Tracked() { job = nil } job.Kill(p.cmd) @@ -230,9 +231,7 @@ func (p *TrackedCommand) setStarted(job *TrackedJob) { } p.mu.Lock() killed := p.killed - if !killed { - p.job = job - } + p.job = job p.diag.RootPID = rootPID p.diag.Tracked = true p.diag.JobObjectCreated = job.Tracked() diff --git a/release-notes/studio/unreleased/14025-published-command-job-owner.md b/release-notes/studio/unreleased/14025-published-command-job-owner.md new file mode 100644 index 0000000000..e3fa9404bb --- /dev/null +++ b/release-notes/studio/unreleased/14025-published-command-job-owner.md @@ -0,0 +1,17 @@ +Windows 跟踪命令保留已发布的 Job Object owner。 + +## 修复 + +- 重复取消和收尾时保留原一次性 owner，避免重新调用按 PID 寻址的停止路径。关联 #14025。 +- 成功分配 Job Object 后不再启动额外 PID 扫描器。关联 #14025。 +- 无 Job Object、未跟踪命令及独立树追踪的旧边界仍在，不代表完整进程隔离已完成。关联 #14025。 + +--- + + + +## Fixed + +- Tracked Windows commands retain their published Job Object owner across repeated cancellation and reaping, avoiding a new PID-based stop attempt. Refs #14025. +- A successfully assigned Job Object replaces the auxiliary PID walker. Refs #14025. +- Existing non-job, untracked and standalone tree-tracking limits remain; full process isolation is not established by this change. Refs #14025. diff --git a/scripts/published-job-evidence.mjs b/scripts/published-job-evidence.mjs new file mode 100644 index 0000000000..81e935dc7b --- /dev/null +++ b/scripts/published-job-evidence.mjs @@ -0,0 +1,50 @@ +import { createHash } from 'node:crypto'; +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { join, resolve } from 'node:path'; + +const required = [ + 'TestPublishedJobChild', + 'TestPublishedJobNoPIDFallback', + 'TestPublishedJobNoPIDFallback/after_wait', + 'TestPublishedJobNoPIDFallback/cancelled_wait', + 'TestPublishedJobUsesKernelTree', + 'TestPublishedJobFallbackControl', +]; +const failures = new Set(required.slice(1, 5)); +const [mode, destination] = process.argv.slice(2); + +if (mode === 'overlay') { + const source = resolve('internal/base/proc/run.go'); + let old = readFileSync(source, 'utf8').replaceAll('\r\n', '\n'); + for (const [current, preceding] of [ + ['\tif !job.Tracked() {\n\t\ttracked.setTree(TrackTree(cmd))\n\t}', '\ttracked.setTree(TrackTree(cmd))'], + ['\tjob := p.job\n\ttree := p.tree', '\tjob := p.job\n\tp.job = nil\n\ttree := p.tree'], + ['\tif !firstKill && !job.Tracked() {', '\tif !firstKill {'], + ['\tkilled := p.killed\n\tp.job = job', '\tkilled := p.killed\n\tif !killed {\n\t\tp.job = job\n\t}'], + ]) { + if (old.split(current).length !== 2) throw new Error('owner reversal is not unique'); + old = old.replace(current, preceding); + } + const sha256 = createHash('sha256').update(old).digest('hex'); + const dir = resolve(destination); + mkdirSync(dir, { recursive: true }); + const replacement = join(dir, 'run.go'); + writeFileSync(replacement, old); + writeFileSync(join(dir, 'overlay.json'), JSON.stringify({ Replace: { [source]: replacement } })); + writeFileSync(join(dir, 'owner-proof.json'), JSON.stringify({ source, sha256 }, null, 2)); +} else if (mode === 'positive' || mode === 'negative') { + const events = readFileSync(destination, 'utf8').trim().split('\n').map(line => JSON.parse(line)); + const pkg = 'reasonix/internal/base/proc'; + const terminals = events.filter(e => e.Package === pkg && ['pass', 'fail', 'skip'].includes(e.Action)); + if (terminals.length !== required.length + 1) throw new Error('unexpected terminal inventory'); + for (const name of [...required, undefined]) { + const entries = terminals.filter(e => e.Test === name); + const failed = mode === 'negative' && (name === undefined || failures.has(name)); + if (entries.length !== 1 || entries[0].Action !== (failed ? 'fail' : 'pass')) { + throw new Error(`unexpected terminal for ${name ?? pkg}`); + } + } + if (events.some(e => e.Package !== pkg || e.Action === 'build-fail')) throw new Error('unexpected package or build failure'); +} else { + throw new Error('expected overlay, positive or negative mode'); +}