Reap child processes#4
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces explicit tracking and termination of child processes spawned by worker threads, aiming to prevent orphaned/zombie processes (especially for background processes) and to make worker-to-parent messaging more structured.
Changes:
- Add a worker message “port” abstraction and extend worker messages to include
child-spawnedand typedbuild-resultevents. - Introduce a
childProcessRegistryand wire it throughkeepWorkerAlive/manageWorkers/jarmuzso child PIDs can be terminated when workers stop or when the parent exits. - Update and expand tests/fixtures to use project-copy fixtures, structured build-result waiting, and new child-process lifecycle scenarios.
Reviewed changes
Copilot reviewed 79 out of 79 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/support/wait-for-pid-gone.mjs | New helper to poll for PID disappearance / zombie state (Linux /proc-based). |
| tests/support/wait-for-message.mjs | Removed generic “wait for any message” helper. |
| tests/support/wait-for-build-result.mjs | New helper to wait specifically for type: "build-result" messages. |
| tests/support/spawn-idle-process.mjs | New helper to spawn a long-running Node process for registry tests. |
| tests/support/run-worker-build.mjs | Switch to waitForBuildResult for deterministic build result collection. |
| tests/support/run-persist-restart-scenario.mjs | Switch to waitForBuildResult. |
| tests/support/kill-process.mjs | Removed test-local kill helper in favor of src killProcess. |
| tests/support/consumer-worker-sources.mjs | Removed dynamic worker source strings (migrated to fixtures). |
| tests/support/consumer-project.mjs | Replace dynamic worker writing with copying fixture projects via copyConsumerProject(). |
| tests/spawner-sigkills-running-process-on-next-build.test.mjs | Update imports and use waitForBuildResult. |
| tests/spawner-runs-background-process-without-waiting.test.mjs | Update to waitForBuildResult. |
| tests/spawner-background-process-dies-when-jarmuz-exits-on-sigterm.test.mjs | New integration-style test ensuring background child dies when parent exits on SIGTERM. |
| tests/scheduler-runs-callback-after-debounce-window.test.mjs | Update API usage from unique to debounce. |
| tests/scheduler-reschedule-cancels-the-earlier-callback.test.mjs | Update API usage from unique to debounce. |
| tests/persist-keepalive-throws-on-duplicate-exec-within-build.test.mjs | Update semantics: duplicate keepAlive within a build now fails; use structured build result. |
| tests/persist-keepalive-restarts-on-retrigger.test.mjs | New test for keepAlive restart behavior on retrigger. |
| tests/persist-keepalive-process-dies-when-jarmuz-exits-on-sigterm.test.mjs | New integration test verifying kept-alive process termination on SIGTERM exit. |
| tests/persist-keepalive-pipeline-progresses-to-the-next-stage.test.mjs | New integration test ensuring pipeline continues after keepAlive stage. |
| tests/manage-workers-stop-all-terminates-every-worker.test.mjs | Wire childProcessRegistry() into manageWorkers. |
| tests/manage-workers-invokes-on-success-for-a-successful-build.test.mjs | Wire childProcessRegistry() into manageWorkers. |
| tests/manage-workers-invokes-on-failure-for-a-failed-build.test.mjs | Wire childProcessRegistry() into manageWorkers. |
| tests/manage-pipeline-schedule-successor-runs-the-next-job.test.mjs | Pass registry into keepWorkerAlive test harness. |
| tests/manage-pipeline-schedule-skips-when-a-predecessor-is-pending.test.mjs | Update scheduler API usage to debounce. |
| tests/manage-pipeline-callback-posts-the-build-message-to-the-worker.test.mjs | Pass registry into keepWorkerAlive test harness. |
| tests/manage-pipeline-callback-drops-job-when-a-predecessor-becomes-pending.test.mjs | Update scheduler API usage to debounce. |
| tests/kill-process-rethrows-non-esrch-errors.test.mjs | New unit test for src/libs/kill-process.mjs behavior. |
| tests/keep-worker-alive-terminate-reaps-the-workers-background-child.test.mjs | New test ensuring terminate kills tracked background child. |
| tests/keep-worker-alive-stops-and-rejects-posting-after-terminate.test.mjs | Pass registry; add JSDoc type cast to posted message. |
| tests/keep-worker-alive-restarts-the-worker-after-an-unexpected-exit.test.mjs | Pass registry into keepWorkerAlive. |
| tests/keep-worker-alive-delivers-a-posted-message-to-the-worker.test.mjs | Pass registry; add JSDoc type cast to posted message. |
| tests/jarmuz-watch-mode-skips-changes-matching-ignore-patterns.test.mjs | Switch to fixture-based consumer project copying. |
| tests/jarmuz-watch-mode-schedules-jobs-on-matching-changes.test.mjs | Switch to fixture-based consumer project copying. |
| tests/jarmuz-uses-the-provided-base-directory.test.mjs | Switch to fixture-based consumer project copying. |
| tests/jarmuz-runs-the-decider-immediately-with-the-initial-flag.test.mjs | Switch to fixture-based consumer project copying. |
| tests/jarmuz-once-mode-runs-the-pipeline-after-the-watcher-is-ready.test.mjs | Switch to fixture project two-stage-pipeline. |
| tests/jarmuz-once-mode-exits-1-when-a-job-fails.test.mjs | Switch to fixture project bad-job. |
| tests/jarmuz-defaults-base-directory-to-the-working-directory.test.mjs | Switch to fixture-based consumer project copying. |
| tests/fixtures/workers/worker-spawner-background-pid.mjs | New worker fixture that spawns a background PID writer. |
| tests/fixtures/workers/worker-persist-keepalive-restart-on-retrigger.mjs | New worker fixture for keepAlive restart scenario. |
| tests/fixtures/workers/worker-persist-keepalive-dedup.mjs | Update fixture to trigger duplicate keepAlive within a build. |
| tests/fixtures/scripts/delayed-shutdown-on-sigterm.mjs | New script used to validate SIGTERM shutdown timing behavior. |
| tests/fixtures/scripts/create-lock-or-report.mjs | Removed script previously used for persist dedup tests. |
| tests/fixtures/projects/two-stage-pipeline/jarmuz/worker-second.mjs | New fixture project worker for two-stage pipeline. |
| tests/fixtures/projects/two-stage-pipeline/jarmuz/worker-first.mjs | New fixture project worker for two-stage pipeline. |
| tests/fixtures/projects/trigger-job/jarmuz/worker-trigger-job.mjs | New fixture project worker that fails (returns false). |
| tests/fixtures/projects/touch-file/jarmuz/worker-touch-file.mjs | New fixture project worker that appends to result file. |
| tests/fixtures/projects/spawner-background/jarmuz/worker-build.mjs | New fixture project spawner worker to run background PID script. |
| tests/fixtures/projects/persist-then-next/jarmuz/worker-next.mjs | New fixture project “next” stage worker. |
| tests/fixtures/projects/persist-then-next/jarmuz/worker-keep-alive-server.mjs | New fixture project keepAlive stage worker. |
| tests/fixtures/projects/persist-keepalive/jarmuz/worker-server.mjs | New fixture project keepAlive server worker. |
| tests/fixtures/projects/bad-job/jarmuz/worker-bad.mjs | New fixture project worker that fails (returns false). |
| tests/fixtures/entry-watch-schedule-initial.mjs | New entrypoint fixture for watch-mode scheduling on initial run. |
| tests/child-process-registry-tolerates-an-already-dead-pid.test.mjs | New registry test for ESRCH tolerance. |
| tests/child-process-registry-register-worker-throws-on-duplicate.test.mjs | New registry test for duplicate registration error. |
| tests/child-process-registry-kill-worker-waits-out-a-slow-shutdown.test.mjs | New registry test around SIGTERM slow shutdown behavior. |
| tests/child-process-registry-kill-worker-throws-when-worker-not-registered.test.mjs | New registry test for not-registered error. |
| tests/child-process-registry-kill-worker-kills-only-that-workers-pids.test.mjs | New registry isolation test for per-worker kill. |
| tests/child-process-registry-kill-all-kills-every-tracked-pid.test.mjs | New registry test for killAll behavior. |
| tests/child-process-registry-add-child-throws-when-worker-not-registered.test.mjs | New registry test for addChild error. |
| src/libs/worker-port.mjs | New worker messaging abstraction (typed build-result and child-spawned). |
| src/libs/worker-not-registered-error.mjs | New error type for registry operations. |
| src/libs/worker-already-registered-error.mjs | New error type for registry operations. |
| src/libs/scheduler.mjs | Rename scheduler API unique -> debounce. |
| src/libs/manage-workers.mjs | Accept registry dependency and route worker messages through structured build-result messages. |
| src/libs/manage-pipeline.mjs | Update to use scheduler debounce. |
| src/libs/kill-process.mjs | New shared kill helper (SIGTERM; ignore ESRCH). |
| src/libs/keep-worker-alive.mjs | Add child PID tracking via registry using worker messages (child-spawned). |
| src/libs/jarmuz.mjs | Install registry and process-level exit/signal hooks to kill tracked children on shutdown. |
| src/libs/duplicate-keep-alive-error.mjs | New error thrown on duplicate keepAlive calls within a build. |
| src/libs/child-process-registry.mjs | New registry implementation mapping worker->PIDs with killWorker/killAll. |
| src/job-types/spawner.mjs | Emit child-spawned messages to parent via worker port. |
| src/job-types/persist.mjs | Emit child-spawned, restart kept-alive processes, and throw on duplicate keepAlive within a build. |
| src/job-types/basic.mjs | Route build messages/results via workerPort() instead of direct parentPort. |
| scripts/check-coverage.mjs | Add Node test timeouts to coverage command. |
| Makefile | Enforce 10s suite timeout and 1s per-test timeout for make test. |
| .claude/rules/testing.md | Add rule metadata and document time budgets / no-mock policy. |
| .claude/rules/nix-shell.md | Removed rules file. |
| .claude/rules/javascript.md | New rules file for JS conventions. |
| .claude/rules/error-handling.md | New rules file describing error handling expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.