Skip to content

feature: remove IPC#52

Open
rustatian wants to merge 1 commit into
masterfrom
feature/remove-stdio
Open

feature: remove IPC#52
rustatian wants to merge 1 commit into
masterfrom
feature/remove-stdio

Conversation

@rustatian

@rustatian rustatian commented Jun 12, 2026

Copy link
Copy Markdown
Member

Reason for This PR

  • Remove IPC layer, replaced by connectrpc

Description of Changes

  • Remove everything related to IPC

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

  • Removals

    • Removed pipe and socket IPC transport mechanisms.
    • Removed debug mode from worker pool configuration.
    • Removed configuration options: max_jobs, max_queue_size, stream_timeout, and exec_ttl.
  • Changes

    • Simplified worker process architecture for cleaner lifecycle management.
    • Refactored pool allocator for direct worker initialization.

Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Copilot AI review requested due to automatic review settings June 12, 2026 18:32
@rustatian rustatian self-assigned this Jun 12, 2026
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR completes a major architectural shift from a goridge relay-based inter-process communication model to a simplified OS process wrapper. It removes entire IPC subsystems (pipe, socket, protocol helpers), refactors the worker to handle only OS process lifecycle, and eliminates batch execution and debug mode features from the pool.

Changes

Worker Model & IPC Refactoring

Layer / File(s) Summary
Worker process model refactoring
worker/worker.go, worker/options.go
Worker refactored from goridge relay wrapper to OS process supervisor: cmd, pid, doneCh, stopTimeout fields replace relay/frame/IPC state; Stop() sends SIGTERM with configurable timeout, falls back to SIGKILL; Wait() transitions FSM based on exit status; removed Exec, StreamIter, AttachRelay, Write methods; added WithStopTimeout() option.
IPC factory and protocol elimination
internal/protocol.go, pool/allocator.go, pool/config.go
Removed SendControl(), Pid() helpers and StopCommand type from protocol; eliminated pipe.Factory and socket.Factory IPC layers; pool.Config simplified by removing Debug, MaxQueueSize, MaxJobs, StreamTimeout; SupervisorConfig.ExecTTL removed; NewPoolAllocator signature simplified to accept only ctx, cmd, command, log and directly initialize workers via InitBaseWorker() + Start() + FSM transition.
Pool refactoring for direct worker initialization
pool/static_pool/pool.go, pool/static_pool/options.go
Pool struct removed factory and queue fields (supervisedExec, queue, maxQueueSize); NewPool dropped factory parameter; removed ExecTTL supervision path; RemoveWorker/AddWorker no longer check debug mode; Reset() closes stopCh instead of resetting queue; internal takeWorker helper removed.
Worker lifecycle test suite rewrite
worker/worker_test.go
Tests migrated from relay/payload model to OS process: new tests cover stdio inheritance, graceful stop with SIGTERM timeout, SIGKILL fallback on timeout, state transitions based on exit code, and process kill handling.
Pool and supervisor test suite refactoring
pool/static_pool/pool_test.go, pool/static_pool/supervisor_test.go, pool/static_pool/dyn_allocator_test.go
Pool tests rewritten using sleep/sh commands instead of PHP; supervisor tests use TestHelperProcess helper (re-executes test binary as memhog worker) and supervisedCfg() helper; dynamic allocator tests simplified to white-box direct calls to addMoreWorkers(); all tests validate worker lifecycle, replacement, and resource limits via process state and PID changes.
Worker watcher test helper updates
worker_watcher/worker_watcher_test.go, worker_watcher/container/channel/vec_test.go
Updated createTestWorker to use sleep 100 command; fakeAllocator cleanup marks worker as fsm.StateDestroyed before kill to prevent respawn leakage; createStartedWorker cleanup transitions to Destroyed, kills process, and waits; added TestWorkerWatcher_Release_BadState_StopsWorker to validate graceful stop on non-Ready workers.
Test infrastructure cleanup and PHP script removal
tests/*.php, tests/http/*.php, tests/src/*, tests/*_test_script.sh, tests/composer.json
Removed all PHP test worker scripts, HTTP request handlers, HTTP client bootstrap, workflow activity/client helpers, and test shell wrappers (pipes_test_script.sh, socket_test_script.sh); these were tied to goridge relay testing.
Build configuration and schema updates
.github/workflows/tests.yml, Makefile, go.mod, schema.json
CI workflow adds permissions: contents: read and removes PHP setup; Makefile replaces ipc/pipe tests with fsm tests; go.mod removes goridge direct dependency and updates indirect deps; schema.json removes debug, max_queue_size, max_jobs, stream_timeout (pool config) and supervisor.exec_ttl properties.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • wolfy-j

Poem

🐰 No more relays, no more frames,
Just processes and cleaner names,
OS-bound, state-machines bright,
Worker pools run sleek and light!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feature: remove IPC' is concise and clearly describes the main objective of the PR, which is removing the IPC layer from the codebase.
Description check ✅ Passed The description includes the reason (IPC removal, replacement with connectrpc), description of changes (remove IPC), and license acceptance, with all checklist items marked complete. However, specific details about which IPC implementations were removed and how the transition to connectrpc affects users could be more comprehensive.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/remove-stdio

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.71%. Comparing base (543784c) to head (bea5ef1).

Files with missing lines Patch % Lines
pool/allocator.go 77.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   72.25%   81.71%   +9.46%     
==========================================
  Files          19       13       -6     
  Lines        1492      782     -710     
==========================================
- Hits         1078      639     -439     
+ Misses        361      104     -257     
+ Partials       53       39      -14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the Goridge-based IPC layer from the pool and repositions the library as an OS-process lifecycle/supervision manager, with request delivery handled externally (connectrpc per PR description).

Changes:

  • Refactors worker.Process to manage only process lifecycle (start/wait/stop/kill) and passes child stdio through by default.
  • Removes IPC/streaming/debug/max_jobs/max_queue_size/exec_ttl-related code paths, tests, fixtures, and config/schema surface area.
  • Updates pool allocation/supervision tests to use OS processes (and a Go helper process) instead of PHP/Goridge fixtures; simplifies CI to Go-only.

Reviewed changes

Copilot reviewed 92 out of 93 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
worker/worker.go Removes relay/IPC execution APIs; adds SIGTERM/SIGKILL-based Stop with configurable timeout and stdio passthrough defaults.
worker/worker_test.go Replaces IPC/PHP-based tests with OS-process lifecycle tests (stdio, stop/kill, state transitions).
worker/options.go Replaces max-execs option with WithStopTimeout for Stop() behavior.
worker_watcher/worker_watcher_test.go Adjusts watcher tests for non-IPC workers; adds coverage for Stop-on-release.
worker_watcher/container/channel/vec_test.go Updates test worker command from PHP to sleep.
tests/worker-slow-dyn.php Removed IPC-era PHP test worker.
tests/supervised.php Removed IPC-era PHP supervised worker fixture.
tests/stream_worker.php Removed IPC-era PHP stream worker fixture.
tests/stop.php Removed IPC-era PHP stop-command fixture.
tests/src/Workflow/SagaWorkflow.php Removed PHP/Temporal test fixture no longer used.
tests/src/Client/StartNewWorkflow.php Removed PHP/Temporal test fixture no longer used.
tests/src/Activity/SimpleActivity.php Removed PHP/Temporal test fixture no longer used.
tests/socket_test_script.sh Removed IPC-era script for socket tests.
tests/slow-pid.php Removed IPC-era PHP slow PID fixture.
tests/slow-destroy.php Removed IPC-era PHP slow destroy fixture.
tests/slow-client.php Removed IPC-era PHP slow client fixture.
tests/slow_req.php Removed IPC-era PHP slow request fixture.
tests/sleep.php Removed IPC-era PHP sleep fixture.
tests/sleep-ttl.php Removed IPC-era PHP TTL fixture.
tests/sleep_short.php Removed IPC-era PHP sleep-short fixture.
tests/should-not-be-killed.php Removed IPC-era PHP memory fixture.
tests/sample.txt Removed unused sample file.
tests/raw-error.php Removed IPC-era PHP raw-error fixture.
tests/psr-worker.php Removed IPC-era PHP PSR worker fixture.
tests/psr-worker-slow.php Removed IPC-era PHP PSR slow fixture.
tests/psr-worker-post.php Removed IPC-era PHP PSR post fixture.
tests/psr-worker-bench.php Removed IPC-era PHP bench fixture.
tests/pipes_test_script.sh Removed IPC-era script for pipe tests.
tests/pid.php Removed IPC-era PHP pid fixture.
tests/metrics-issue-571.php Removed IPC-era PHP metrics fixture.
tests/memleak.php Removed IPC-era PHP memleak fixture.
tests/issue659.php Removed IPC-era PHP issue fixture.
tests/idle.php Removed IPC-era PHP idle fixture.
tests/http/user-agent.php Removed IPC-era PHP HTTP fixture.
tests/http/upload.php Removed IPC-era PHP HTTP fixture.
tests/http/stuck.php Removed IPC-era PHP HTTP fixture.
tests/http/slow-client.php Removed IPC-era PHP HTTP slow-client fixture.
tests/http/server.php Removed IPC-era PHP HTTP fixture.
tests/http/request-uri.php Removed IPC-era PHP HTTP fixture.
tests/http/push.php Removed IPC-era PHP HTTP fixture.
tests/http/pid.php Removed IPC-era PHP HTTP fixture.
tests/http/payload.php Removed IPC-era PHP HTTP fixture.
tests/http/memleak.php Removed IPC-era PHP HTTP fixture.
tests/http/ip.php Removed IPC-era PHP HTTP fixture.
tests/http/headers.php Removed IPC-era PHP HTTP fixture.
tests/http/header.php Removed IPC-era PHP HTTP fixture.
tests/http/error2.php Removed IPC-era PHP HTTP fixture.
tests/http/error.php Removed IPC-era PHP HTTP fixture.
tests/http/env.php Removed IPC-era PHP HTTP fixture.
tests/http/echoerr.php Removed IPC-era PHP HTTP fixture.
tests/http/echoDelay.php Removed IPC-era PHP HTTP fixture.
tests/http/echo.php Removed IPC-era PHP HTTP fixture.
tests/http/data.php Removed IPC-era PHP HTTP fixture.
tests/http/cookie.php Removed IPC-era PHP HTTP fixture.
tests/http/client.php Removed IPC-era PHP HTTP client fixture.
tests/head.php Removed IPC-era PHP head fixture.
tests/gzip-large-file.txt Removed unused large test data file.
tests/failboot.php Removed IPC-era PHP failboot fixture.
tests/exec_ttl.php Removed IPC-era PHP exec-ttl fixture.
tests/error.php Removed IPC-era PHP error fixture.
tests/echo.php Removed IPC-era PHP echo fixture.
tests/delay.php Removed IPC-era PHP delay fixture.
tests/crc_error.php Removed IPC-era PHP CRC fixture.
tests/composer.json Removed PHP test dependency manifest.
tests/client.php Removed IPC-era PHP client fixture.
tests/broken.php Removed IPC-era PHP broken-worker fixture.
tests/allocate-failed.php Removed IPC-era PHP allocate-failed fixture.
schema.json Removes IPC-era config keys (debug/max_jobs/max_queue_size/stream_timeout/exec_ttl) and updates wording.
pool/static_pool/supervisor_test.go Rewrites supervision tests to use OS workers and a Go helper process for memory growth.
pool/static_pool/stream.go Removes stream wrapper type (streaming no longer handled in pool).
pool/static_pool/pool.go Simplifies pool to lifecycle/supervision manager; removes Exec/queue/debug/stream handling; updates allocator wiring.
pool/static_pool/options.go Removes queue-size option (no longer applicable without in-pool Exec).
pool/static_pool/options_test.go Removes tests for removed queue-size option.
pool/static_pool/fuzz_test.go Removes fuzz test tied to removed Exec path.
pool/static_pool/dyn_allocator.go Adjusts comments and keeps scale-up logic for plugin-triggered usage.
pool/static_pool/dyn_allocator_test.go Replaces request-driven dynamic allocator tests with white-box scale-up/idle-dealloc tests.
pool/static_pool/debug.go Removes debug execution mode (tied to removed Exec path).
pool/static_pool/debug_test.go Removes debug-mode tests (tied to removed Exec path).
pool/config.go Removes IPC-era config fields (Debug/MaxJobs/MaxQueueSize/StreamTimeout/Supervisor.ExecTTL).
pool/allocator.go Removes factory/IPC handshake; allocates by spawning and starting plain OS processes, transitioning to Ready.
payload/payload.go Removes IPC payload type (no longer used by pool).
Makefile Removes IPC package test targets; adds fsm test target to coverage/test.
ipc/socket/socket.go Removes socket IPC factory implementation.
ipc/socket/socket_test.go Removes socket IPC tests.
ipc/socket/socket_spawn_test.go Removes socket IPC spawn tests/benchmarks.
ipc/pipe/pipe.go Removes pipe IPC factory implementation.
ipc/pipe/pipe_test.go Removes pipe IPC tests/benchmarks.
ipc/pipe/pipe_spawn_test.go Removes pipe IPC spawn tests/benchmarks.
internal/protocol.go Removes IPC control protocol helpers (stop/pid frames).
go.sum Updates sums after removing goridge and related deps.
go.mod Drops goridge dependency; updates indirect deps accordingly.
.github/workflows/tests.yml Removes PHP setup/composer install; limits CI to Go build/test on Ubuntu with reduced permissions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread worker/worker.go
Comment on lines 142 to 146
func (w *Process) Wait() error {
const op = errors.Op("process_wait")
var err error
err = w.cmd.Wait()
err := w.cmd.Wait()
w.doneCh <- struct{}{}

Comment thread worker/worker.go
Comment on lines 169 to +173
func (w *Process) Stop() error {
const op = errors.Op("process_stop")
w.fsm.Transition(fsm.StateStopping)

go func() {
w.log.Debug("sending stop request to the worker", "pid", w.pid)
err := internal.SendControl(w.relay, &internal.StopCommand{Stop: true})
if err == nil {
w.fsm.Transition(fsm.StateStopped)
}
}()
w.log.Debug("sending SIGTERM to the worker", "pid", w.pid)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Makefile (1)

17-24: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Declare test targets as PHONY to avoid file-name collision behavior.

test (and ideally test_coverage) should be marked .PHONY; otherwise make can skip execution if same-named files exist in the repo/workdir.

Suggested patch
+.PHONY: test test_coverage
+
 test_coverage:
 	rm -rf coverage-ci
 	mkdir ./coverage-ci
 	go test -v -race -cover -tags=debug -timeout 30m -coverpkg=./... -coverprofile=./coverage-ci/pool_static.out -covermode=atomic ./pool/static_pool

As per coding guidelines, static analysis (checkmake minphony) flags that required target test must be declared PHONY.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 17 - 24, Add a .PHONY declaration for the Makefile
targets to prevent filename collisions: declare the test target (and
test_coverage if present) as phony by adding a .PHONY line referencing "test"
(and "test_coverage") so make always runs the recipe regardless of same-named
files; update the Makefile to include ".PHONY: test test_coverage" near the top
or with other phony declarations and ensure the "test" target remains unchanged.

Source: Linters/SAST tools

🧹 Nitpick comments (5)
worker/worker.go (1)

153-156: ⚡ Quick win

Redundant error wrapping in Wait().

stderr.Join(err, errors.E(op, err)) joins the original error with a wrapped version of itself, creating duplication in the error chain. Consider returning the wrapped error directly.

♻️ Suggested fix
 	if err != nil {
 		w.State().Transition(fsm.StateErrored)
-		err = stderr.Join(err, errors.E(op, err))
+		return errors.E(op, err)
 	}
+
+	if w.cmd.ProcessState.Success() {
+		w.State().Transition(fsm.StateStopped)
+	}
+
+	return nil

Note: This also requires removing the duplicate ProcessState.Success() check and return at the end of the function.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@worker/worker.go` around lines 153 - 156, In Wait(), remove the redundant
stderr.Join that merges the same error with a wrapped copy: replace "err =
stderr.Join(err, errors.E(op, err))" with simply "err = errors.E(op, err)" (or
return the wrapped error directly) after calling
w.State().Transition(fsm.StateErrored); also remove the duplicate
ProcessState.Success() check/return at the end of the function so the function
returns the single wrapped error rather than a duplicated error chain; target
the Wait function, the call sites to stderr.Join and errors.E(op, err), and the
duplicated ProcessState.Success() return to implement this change.
pool/static_pool/dyn_allocator_test.go (1)

67-70: ⚖️ Poor tradeoff

Rate-limiter interaction issue in Eventually loop.

The test repeatedly calls addMoreWorkers() in a tight loop until NumDynamic() == 3, but addMoreWorkers() has internal rate limiting (cooldown). The comment says "wait out the rate-limiter cooldown," but the implementation doesn't wait—it calls the function repeatedly, which could lead to unpredictable behavior or race conditions.

⏱️ Suggested fix: explicit cooldown wait
-	// wait out the rate-limiter cooldown; the next batch is capped at max_workers
-	require.Eventually(t, func() bool {
-		p.dynamicAllocator.addMoreWorkers()
-		return p.NumDynamic() == 3
-	}, 10*time.Second, 200*time.Millisecond)
+	// wait out the rate-limiter cooldown (1s default), then allocate
+	time.Sleep(time.Second + 100*time.Millisecond)
+	p.dynamicAllocator.addMoreWorkers()
+	
+	// the next batch is capped at max_workers (3 total: 1 base + 2 dynamic)
+	require.Eventually(t, func() bool {
+		return p.NumDynamic() == 2
+	}, 5*time.Second, 100*time.Millisecond)

Note: Updated assertion to check for 2 dynamic workers (not 3 total) since NumDynamic() tracks dynamic workers only, and the base worker count is 1.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pool/static_pool/dyn_allocator_test.go` around lines 67 - 70, The Eventually
loop is repeatedly calling p.dynamicAllocator.addMoreWorkers() but
addMoreWorkers() enforces a cooldown, so instead call addMoreWorkers() only once
per attempt and wait out the cooldown before retrying (or insert a time.Sleep
for the known cooldown duration inside the supplied func), and update the
expected value from 3 to 2 because NumDynamic() counts only dynamic workers
(base worker is separate); adjust the require.Eventually body to call
p.dynamicAllocator.addMoreWorkers() once per invocation, sleep for the cooldown
if needed, and assert p.NumDynamic() == 2.
worker/worker_test.go (2)

65-84: 💤 Low value

Potential test flakiness from sleep-based synchronization.

Lines 77: The test uses time.Sleep(300ms) to wait for the shell to install the TERM trap. This timing-based synchronization is fragile and could cause flakiness on slow CI systems or under load.

While signal handling tests are inherently difficult to synchronize robustly, consider whether the sleep duration could be increased slightly (e.g., 500ms) for more margin, or if there's a way to verify the trap is installed before proceeding. Given the nature of signal testing, the current approach may be the most practical solution.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@worker/worker_test.go` around lines 65 - 84, Test_Stop_Graceful currently
uses a fixed 300ms time.Sleep to wait for the shell to install the TERM trap
which is fragile; increase the wait margin (e.g., change the sleep to 500ms) or
replace the sleep with an active readiness check (for example poll for the child
process to be running or use a signaling mechanism) so the trap is reliably
installed before calling w.Stop(); update the Test_Stop_Graceful function and
its local goroutine/wait logic accordingly (references: Test_Stop_Graceful, w,
w.Stop(), w.Wait()).

86-115: 💤 Low value

Potential test flakiness from sleep-based synchronization.

Line 102: Same concern as the graceful stop test—the 300ms sleep to wait for trap installation could cause flakiness.

Consider increasing the sleep duration for more reliability on slower systems.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@worker/worker_test.go` around lines 86 - 115, The 300ms time.Sleep in
Test_Stop_KillFallback can cause flakiness; instead have the child signal
readiness and wait for that signal before calling w.Stop(): modify the
exec.Command passed to InitBaseWorker to emit a ready token (e.g. echo READY
after installing the trap and before exec sleep) and in the test read that token
from the worker process' stdout/stderr (or use a pipe) to know the trap is
installed, then proceed to call w.Stop() (retain WithStopTimeout and assertions
on w.Stop(), w.Wait(), and State()).
pool/static_pool/pool_test.go (1)

94-105: Clarify RemoveWorker “last worker” behavior: it intentionally returns nil

WorkerWatcher.RemoveWorker explicitly checks numWorkers == 1, logs a warning, and returns nil without changing worker count; pool/static_pool.RemoveWorker forwards this behavior. This is already covered by both Test_NewPool_RemoveLastWorker and TestWorkerWatcher_RemoveWorker_CannotRemoveLast.

Optional: document this “no-op on last worker” contract on the RemoveWorker API to set clear expectations for consumers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pool/static_pool/pool_test.go` around lines 94 - 105, Update the RemoveWorker
API docs to explicitly state that removing the last worker is a no-op: document
on the RemoveWorker method (and mirror in pool/static_pool.RemoveWorker) that
when numWorkers == 1 the implementation logs a warning and returns nil without
changing the worker count (as implemented in WorkerWatcher.RemoveWorker), so
callers know this behavior is intentional and covered by
Test_NewPool_RemoveLastWorker and
TestWorkerWatcher_RemoveWorker_CannotRemoveLast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@Makefile`:
- Around line 17-24: Add a .PHONY declaration for the Makefile targets to
prevent filename collisions: declare the test target (and test_coverage if
present) as phony by adding a .PHONY line referencing "test" (and
"test_coverage") so make always runs the recipe regardless of same-named files;
update the Makefile to include ".PHONY: test test_coverage" near the top or with
other phony declarations and ensure the "test" target remains unchanged.

---

Nitpick comments:
In `@pool/static_pool/dyn_allocator_test.go`:
- Around line 67-70: The Eventually loop is repeatedly calling
p.dynamicAllocator.addMoreWorkers() but addMoreWorkers() enforces a cooldown, so
instead call addMoreWorkers() only once per attempt and wait out the cooldown
before retrying (or insert a time.Sleep for the known cooldown duration inside
the supplied func), and update the expected value from 3 to 2 because
NumDynamic() counts only dynamic workers (base worker is separate); adjust the
require.Eventually body to call p.dynamicAllocator.addMoreWorkers() once per
invocation, sleep for the cooldown if needed, and assert p.NumDynamic() == 2.

In `@pool/static_pool/pool_test.go`:
- Around line 94-105: Update the RemoveWorker API docs to explicitly state that
removing the last worker is a no-op: document on the RemoveWorker method (and
mirror in pool/static_pool.RemoveWorker) that when numWorkers == 1 the
implementation logs a warning and returns nil without changing the worker count
(as implemented in WorkerWatcher.RemoveWorker), so callers know this behavior is
intentional and covered by Test_NewPool_RemoveLastWorker and
TestWorkerWatcher_RemoveWorker_CannotRemoveLast.

In `@worker/worker_test.go`:
- Around line 65-84: Test_Stop_Graceful currently uses a fixed 300ms time.Sleep
to wait for the shell to install the TERM trap which is fragile; increase the
wait margin (e.g., change the sleep to 500ms) or replace the sleep with an
active readiness check (for example poll for the child process to be running or
use a signaling mechanism) so the trap is reliably installed before calling
w.Stop(); update the Test_Stop_Graceful function and its local goroutine/wait
logic accordingly (references: Test_Stop_Graceful, w, w.Stop(), w.Wait()).
- Around line 86-115: The 300ms time.Sleep in Test_Stop_KillFallback can cause
flakiness; instead have the child signal readiness and wait for that signal
before calling w.Stop(): modify the exec.Command passed to InitBaseWorker to
emit a ready token (e.g. echo READY after installing the trap and before exec
sleep) and in the test read that token from the worker process' stdout/stderr
(or use a pipe) to know the trap is installed, then proceed to call w.Stop()
(retain WithStopTimeout and assertions on w.Stop(), w.Wait(), and State()).

In `@worker/worker.go`:
- Around line 153-156: In Wait(), remove the redundant stderr.Join that merges
the same error with a wrapped copy: replace "err = stderr.Join(err, errors.E(op,
err))" with simply "err = errors.E(op, err)" (or return the wrapped error
directly) after calling w.State().Transition(fsm.StateErrored); also remove the
duplicate ProcessState.Success() check/return at the end of the function so the
function returns the single wrapped error rather than a duplicated error chain;
target the Wait function, the call sites to stderr.Join and errors.E(op, err),
and the duplicated ProcessState.Success() return to implement this change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0657f251-55d5-4390-8e0d-081c7ce32682

📥 Commits

Reviewing files that changed from the base of the PR and between 543784c and bea5ef1.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (92)
  • .github/workflows/tests.yml
  • Makefile
  • go.mod
  • internal/protocol.go
  • ipc/pipe/pipe.go
  • ipc/pipe/pipe_spawn_test.go
  • ipc/pipe/pipe_test.go
  • ipc/socket/socket.go
  • ipc/socket/socket_spawn_test.go
  • ipc/socket/socket_test.go
  • payload/payload.go
  • pool/allocator.go
  • pool/config.go
  • pool/static_pool/debug.go
  • pool/static_pool/debug_test.go
  • pool/static_pool/dyn_allocator.go
  • pool/static_pool/dyn_allocator_test.go
  • pool/static_pool/fuzz_test.go
  • pool/static_pool/options.go
  • pool/static_pool/options_test.go
  • pool/static_pool/pool.go
  • pool/static_pool/pool_test.go
  • pool/static_pool/stream.go
  • pool/static_pool/supervisor_test.go
  • schema.json
  • tests/allocate-failed.php
  • tests/broken.php
  • tests/client.php
  • tests/composer.json
  • tests/crc_error.php
  • tests/delay.php
  • tests/echo.php
  • tests/error.php
  • tests/exec_ttl.php
  • tests/failboot.php
  • tests/gzip-large-file.txt
  • tests/head.php
  • tests/http/client.php
  • tests/http/cookie.php
  • tests/http/data.php
  • tests/http/echo.php
  • tests/http/echoDelay.php
  • tests/http/echoerr.php
  • tests/http/env.php
  • tests/http/error.php
  • tests/http/error2.php
  • tests/http/header.php
  • tests/http/headers.php
  • tests/http/ip.php
  • tests/http/memleak.php
  • tests/http/payload.php
  • tests/http/pid.php
  • tests/http/push.php
  • tests/http/request-uri.php
  • tests/http/server.php
  • tests/http/slow-client.php
  • tests/http/stuck.php
  • tests/http/upload.php
  • tests/http/user-agent.php
  • tests/idle.php
  • tests/issue659.php
  • tests/memleak.php
  • tests/metrics-issue-571.php
  • tests/pid.php
  • tests/pipes_test_script.sh
  • tests/psr-worker-bench.php
  • tests/psr-worker-post.php
  • tests/psr-worker-slow.php
  • tests/psr-worker.php
  • tests/raw-error.php
  • tests/sample.txt
  • tests/should-not-be-killed.php
  • tests/sleep-ttl.php
  • tests/sleep.php
  • tests/sleep_short.php
  • tests/slow-client.php
  • tests/slow-destroy.php
  • tests/slow-pid.php
  • tests/slow_req.php
  • tests/socket_test_script.sh
  • tests/src/Activity/SimpleActivity.php
  • tests/src/Client/StartNewWorkflow.php
  • tests/src/Workflow/SagaWorkflow.php
  • tests/stop.php
  • tests/stream_worker.php
  • tests/supervised.php
  • tests/worker-slow-dyn.php
  • worker/options.go
  • worker/worker.go
  • worker/worker_test.go
  • worker_watcher/container/channel/vec_test.go
  • worker_watcher/worker_watcher_test.go
💤 Files with no reviewable changes (78)
  • tests/http/stuck.php
  • tests/client.php
  • tests/gzip-large-file.txt
  • tests/http/ip.php
  • tests/echo.php
  • tests/crc_error.php
  • tests/allocate-failed.php
  • tests/issue659.php
  • pool/static_pool/debug_test.go
  • tests/src/Workflow/SagaWorkflow.php
  • tests/error.php
  • tests/delay.php
  • tests/http/headers.php
  • tests/http/request-uri.php
  • payload/payload.go
  • tests/pipes_test_script.sh
  • tests/sleep.php
  • tests/should-not-be-killed.php
  • tests/idle.php
  • tests/http/memleak.php
  • tests/metrics-issue-571.php
  • tests/memleak.php
  • tests/http/error.php
  • tests/slow-pid.php
  • tests/http/header.php
  • tests/psr-worker.php
  • tests/http/pid.php
  • tests/raw-error.php
  • tests/exec_ttl.php
  • tests/http/user-agent.php
  • tests/socket_test_script.sh
  • tests/http/client.php
  • tests/failboot.php
  • tests/http/echoDelay.php
  • tests/sleep-ttl.php
  • tests/worker-slow-dyn.php
  • tests/http/echoerr.php
  • tests/http/data.php
  • tests/supervised.php
  • tests/pid.php
  • tests/head.php
  • pool/static_pool/debug.go
  • tests/http/env.php
  • tests/http/payload.php
  • tests/http/upload.php
  • tests/composer.json
  • tests/psr-worker-bench.php
  • pool/static_pool/fuzz_test.go
  • ipc/socket/socket_spawn_test.go
  • pool/static_pool/stream.go
  • tests/http/echo.php
  • ipc/socket/socket.go
  • tests/broken.php
  • ipc/pipe/pipe.go
  • tests/http/cookie.php
  • tests/http/error2.php
  • tests/stop.php
  • ipc/pipe/pipe_test.go
  • ipc/socket/socket_test.go
  • pool/static_pool/options.go
  • tests/slow_req.php
  • tests/stream_worker.php
  • tests/http/push.php
  • tests/sleep_short.php
  • tests/src/Activity/SimpleActivity.php
  • tests/src/Client/StartNewWorkflow.php
  • internal/protocol.go
  • tests/psr-worker-post.php
  • tests/psr-worker-slow.php
  • tests/sample.txt
  • tests/http/server.php
  • tests/slow-destroy.php
  • ipc/pipe/pipe_spawn_test.go
  • tests/http/slow-client.php
  • tests/slow-client.php
  • pool/config.go
  • pool/static_pool/dyn_allocator.go
  • pool/static_pool/options_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants