diff --git a/internal/llmadapters/pi_rpc_extension_unix_test.go b/internal/llmadapters/pi_rpc_extension_unix_test.go index e2f21b37..8395ba81 100644 --- a/internal/llmadapters/pi_rpc_extension_unix_test.go +++ b/internal/llmadapters/pi_rpc_extension_unix_test.go @@ -26,16 +26,25 @@ func TestPiRPCReviewerHelperStaysInParentProcessGroup(t *testing.T) { t.Fatalf("WriteFile(helper): %v", err) } extensionPath := filepath.Join(tempDir, "extension.mjs") - extension := piRPCReviewerExtension(helperPath, filepath.Join(tempDir, "config.json"), tempDir, 2048, 5*time.Second, "") + const helperTimeout = 5 * time.Second + extension := piRPCReviewerExtension(helperPath, filepath.Join(tempDir, "config.json"), tempDir, 2048, helperTimeout, "") if err := os.WriteFile(extensionPath, []byte(extension), 0o600); err != nil { // #nosec G703 -- extensionPath is rooted in t.TempDir. t.Fatalf("WriteFile(extension): %v", err) } runnerPath := filepath.Join(tempDir, "runner.mjs") - runner := "import extension from " + strconv.Quote(extensionPath) + ";\nconst tools = {};\nextension({ registerTool(tool) { tools[tool.name] = tool; } });\nvoid tools.cr_diff.execute('call-1', {}, new AbortController().signal);\n" + runner := "import extension from " + strconv.Quote(extensionPath) + ";\nconst tools = {};\nextension({ registerTool(tool) { tools[tool.name] = tool; } });\nvoid tools.cr_diff.execute('call-1', {}, new AbortController().signal).then(result => { if (result.isError) { console.error(JSON.stringify(result)); process.exitCode = 1; } });\n" if err := os.WriteFile(runnerPath, []byte(runner), 0o600); err != nil { t.Fatalf("WriteFile(runner): %v", err) } cmd := exec.Command(nodePath, runnerPath) // #nosec G204 -- test launches the discovered Node executable with a test-owned script. + outputPath := filepath.Join(tempDir, "runner.log") + output, err := os.Create(outputPath) // #nosec G304 -- outputPath is rooted in t.TempDir. + if err != nil { + t.Fatalf("Create(runner log): %v", err) + } + defer func() { _ = output.Close() }() + cmd.Stdout = output + cmd.Stderr = output cmd.Dir = tempDir cmd.Env = append(os.Environ(), "PI_RPC_TEST_PID="+pidPath) cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} @@ -45,10 +54,24 @@ func TestPiRPCReviewerHelperStaysInParentProcessGroup(t *testing.T) { defer func() { _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) _ = cmd.Wait() + if t.Failed() { + data, err := os.ReadFile(outputPath) // #nosec G304 -- outputPath is rooted in t.TempDir. + if err != nil { + t.Logf("ReadFile(runner log): %v", err) + } else { + t.Logf("runner output: %s", data) + } + } }() - eventually(t, 3*time.Second, func() bool { - _, err := os.Stat(pidPath) - return err == nil + // Runner startup precedes the helper's own timeout; allow both stages rather + // than treating a slow Node startup as a process-group cleanup failure. + eventually(t, 2*helperTimeout, func() bool { + data, err := os.ReadFile(pidPath) // #nosec G304 -- pidPath is rooted in t.TempDir. + if err != nil { + return false + } + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + return err == nil && pid > 0 }) pidData, err := os.ReadFile(pidPath) // #nosec G304 -- pidPath is rooted in t.TempDir. if err != nil { @@ -59,6 +82,13 @@ func TestPiRPCReviewerHelperStaysInParentProcessGroup(t *testing.T) { t.Fatalf("Atoi(pid): %v", err) } t.Cleanup(func() { _ = syscall.Kill(helperPID, syscall.SIGKILL) }) + helperGroup, err := syscall.Getpgid(helperPID) + if err != nil { + t.Fatalf("Getpgid(helper): %v", err) + } + if helperGroup != cmd.Process.Pid { + t.Fatalf("helper process group = %d, want runner group %d", helperGroup, cmd.Process.Pid) + } if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil { t.Fatalf("kill runner process group: %v", err) diff --git a/internal/llmadapters/testdata/pi_rpc_detached_helper.cjs b/internal/llmadapters/testdata/pi_rpc_detached_helper.cjs new file mode 100644 index 00000000..3559f8ed --- /dev/null +++ b/internal/llmadapters/testdata/pi_rpc_detached_helper.cjs @@ -0,0 +1,8 @@ +// Negative control: deliberately violate the helper process-group contract. +if (process.argv[1]?.endsWith('/runner.mjs')) { + const childProcess = require('node:child_process'); + const originalSpawn = childProcess.spawn; + childProcess.spawn = (executable, args, options) => + originalSpawn(executable, args, { ...options, detached: true }); + require('node:module').syncBuiltinESMExports(); +} diff --git a/internal/llmadapters/testdata/pi_rpc_slow_start.cjs b/internal/llmadapters/testdata/pi_rpc_slow_start.cjs new file mode 100644 index 00000000..d29bc2c3 --- /dev/null +++ b/internal/llmadapters/testdata/pi_rpc_slow_start.cjs @@ -0,0 +1,4 @@ +// Delay only the process-group test's runner, not its helper. +if (process.argv[1]?.endsWith('/runner.mjs')) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 3100); +}