Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughModule bumped to v2, Goridge upgraded v3→v4, logging migrated from go.uber.org/zap to log/slog across the codebase, numerous imports updated to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the project from zap to the Go stdlib log/slog logger while simultaneously updating the module major version to v2 and bumping key dependencies (notably goridge to v4). It aligns the codebase imports, constructors, and tests with the new module path and logging API.
Changes:
- Replace
zap.Loggerusage with*slog.Loggerand convert log field encoding to slog key/value pairs. - Update module path to
github.com/roadrunner-server/pool/v2and adjust internal imports accordingly. - Upgrade
goridgetov4and refresh Go module dependencies/sums.
Reviewed changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| worker_watcher/worker_watcher.go | Switch watcher logging to slog and update imports to /v2. |
| worker_watcher/container/channel/vec.go | Update fsm/worker imports to /v2. |
| worker_watcher/container/channel/vec_test.go | Update worker import to /v2 for tests. |
| worker/worker.go | Replace zap with slog, update goridge v4 imports, default logger handling. |
| worker/options.go | Convert options logging types to slog and adjust jitter logging call. |
| worker/worker_test.go | Update payload import to /v2. |
| state/process/state.go | Update worker import to /v2. |
| pool/static_pool/pool.go | Convert pool logging to slog, update goridge v4 + /v2 imports. |
| pool/static_pool/options.go | Update WithLogger option to accept *slog.Logger. |
| pool/static_pool/stream.go | Update payload import to /v2. |
| pool/static_pool/supervisor.go | Update imports to /v2 and convert logs to slog key/value pairs. |
| pool/static_pool/debug.go | Update goridge to v4, /v2 imports, convert log calls to slog format. |
| pool/static_pool/dyn_allocator.go | Convert allocator logging to slog, update /v2 imports. |
| pool/static_pool/pool_test.go | Replace zap test logger with slog usage; update /v2 imports. |
| pool/static_pool/supervisor_test.go | Replace zap test logger with slog usage; update /v2 imports. |
| pool/static_pool/dyn_allocator_test.go | Replace zap test logger with slog usage; update /v2 imports. |
| pool/static_pool/fuzz_test.go | Replace zap test logger with slog usage; update /v2 imports. |
| pool/allocator.go | Update allocator signature to *slog.Logger, update /v2 imports, convert log fields. |
| ipc/pipe/pipe.go | Convert pipe factory logger type to *slog.Logger, update goridge v4 + /v2 imports. |
| ipc/pipe/pipe_test.go | Update /v2 imports and switch benchmark factory logger to slog. |
| ipc/pipe/pipe_spawn_test.go | Replace zap nop logger with slog discard logger; update /v2 imports. |
| ipc/socket/socket.go | Convert socket factory logger type to *slog.Logger, update goridge v4 + /v2 imports. |
| ipc/socket/socket_test.go | Update payload import to /v2. |
| ipc/socket/socket_spawn_test.go | Replace zap nop logger with slog discard logger; update payload import to /v2. |
| internal/protocol.go | Update goridge imports to v4. |
| fsm/fsm.go | Update FSM logger type to *slog.Logger and convert debug field to slog format. |
| fsm/state_test.go | Replace zap logger creation with slog.Default() for tests. |
| go.mod | Bump module path to /v2, move to goridge v4, remove zap dependency. |
| go.sum | Reflect dependency upgrades/removals (zap removed, goridge v4 added, etc.). |
| .gitignore | Ignore .claude/settings.local.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pool/static_pool/dyn_allocator.go (1)
68-68:⚠️ Potential issue | 🔴 CriticalCritical bug:
new(time.Now().UTC())is invalid Go syntax and will fail to compile.The
new()builtin requires a type argument, not an expression.time.Now().UTC()returns atime.Timevalue, not a type. This cannot be passed tonew().To store a pointer to the current time, use:
- da.lastAllocTry.Store(new(time.Now().UTC())) + now := time.Now().UTC() + da.lastAllocTry.Store(&now)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pool/static_pool/dyn_allocator.go` at line 68, The call new(time.Now().UTC()) is invalid Go; replace it by creating a time value and storing its address. For example, compute t := time.Now().UTC() and then call da.lastAllocTry.Store(&t) (or store t directly if lastAllocTry expects a time.Time), updating the usage around da.lastAllocTry.Store to use the local t variable instead of new(...).
🧹 Nitpick comments (6)
.gitignore (1)
33-33: Minor scope note: Unrelated change in a logging migration PR.Adding
.claude/settings.local.jsonis a reasonable addition for local configuration, and it's consistent with existing tool-specific entries like.idea. However, this change is unrelated to the PR's stated objectives (zap → slog migration, SDK v2, goridge v4 upgrade).For cleaner commit history, consider moving tool-specific
.gitignoreupdates to a separate housekeeping PR or commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 33, The .gitignore entry ".claude/settings.local.json" is an unrelated housekeeping change and should be moved out of this logging-migration PR; remove that line from the current diff (the .gitignore modification) and create a separate commit/PR that adds the ".claude/settings.local.json" entry (or stash the change, create a new branch, commit there, and open a new PR) so the current PR only contains zap→slog, SDK v2, and goridge v4 changes.pool/static_pool/options.go (1)
9-12: Optional: renameztologgerfor clearer option API readability.
Behavior is correct; this is just a small readability polish.Proposed diff
-func WithLogger(z *slog.Logger) Options { +func WithLogger(logger *slog.Logger) Options { return func(p *Pool) { - p.log = z + p.log = logger } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pool/static_pool/options.go` around lines 9 - 12, The parameter name `z` in the option constructor WithLogger is unclear; rename the parameter to `logger` in the function signature and update its usage inside the returned closure (function WithLogger(logger *slog.Logger) Options { ... p.log = logger ... }) to improve API readability while keeping behavior unchanged.ipc/pipe/pipe_test.go (1)
445-445: Inconsistent logger usage in benchmark.This benchmark uses
slog.Default()while all other tests in this file use thelogvariable (defined inpipe_spawn_test.goasslog.New(slog.DiscardHandler)). Usingslog.Default()will produce log output to stderr during benchmark runs, which may be undesirable.🔧 Suggested fix for consistency
- w, err := NewPipeFactory(slog.Default()).SpawnWorkerWithContext(ctx, cmd) + w, err := NewPipeFactory(log).SpawnWorkerWithContext(ctx, cmd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ipc/pipe/pipe_test.go` at line 445, The benchmark is using slog.Default() instead of the test-local logger, causing stderr output; update the call to NewPipeFactory in the benchmark to use the shared test logger (the log variable defined in pipe_spawn_test.go) so it matches other tests—i.e., replace slog.Default() with log when calling NewPipeFactory(...). Ensure the change is applied to the call site that invokes NewPipeFactory(...).SpawnWorkerWithContext(ctx, cmd).fsm/fsm.go (1)
41-41: Consider using a more descriptive key name.The key
"debug"is unconventional for structured logging. Consider using"details"or"reason"to better describe the error context, since the message already indicates this is informational.- s.log.Debug("transition info, this is not an error", "debug", err.Error()) + s.log.Debug("transition info, this is not an error", "reason", err.Error())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fsm/fsm.go` at line 41, Change the structured log key in the s.log.Debug call inside the FSM (function/method where s.log.Debug is invoked) from the ambiguous key "debug" to a clearer name like "details" or "reason" so the error context is more descriptive; update the call on s.log.Debug(...) to use the new key (e.g., "details") while keeping the same message and err.Error() argument.worker/worker.go (1)
469-473: Consider using Warn level for stderr output.Worker stderr typically contains error messages or warnings from the child process. Logging at
Infolevel may cause these to be missed in production configurations that filter to Warn and above.func (w *Process) Write(p []byte) (int, error) { // unsafe to use utils.AsString - w.log.Info(string(p)) + w.log.Warn(string(p)) return len(p), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worker/worker.go` around lines 469 - 473, The Write method on Process currently logs incoming bytes at Info level in func (w *Process) Write, which is stderr and should be treated as warnings/errors; change the logging call from w.log.Info(...) to a warning-level call (e.g., w.log.Warn or w.log.Warnf) so stderr output is emitted at Warn level or higher; keep the existing behavior of converting p to string (or optionally trim trailing newlines) but ensure the log API used is the warning-level method on w.log.worker_watcher/worker_watcher.go (1)
420-433: Logging"error", errwhenerris nil adds noise.At line 431, the
errvariable fromw.Wait()(line 421) may benilif the worker exited cleanly. Including"error", nilin the log output is harmless but adds unnecessary noise to debug logs.♻️ Proposed fix
if w.State().Compare(fsm.StateDestroyed) { // worker was manually destroyed, no need to replace - ww.log.Debug("worker destroyed", "pid", w.Pid(), "internal_event_name", events.EventWorkerDestruct.String(), "error", err) + ww.log.Debug("worker destroyed", "pid", w.Pid(), "internal_event_name", events.EventWorkerDestruct.String()) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@worker_watcher/worker_watcher.go` around lines 420 - 433, In WorkerWatcher.wait, avoid logging a nil error: change the ww.log.Debug calls around w.Wait() so they only include the "error" key when err != nil; use the result of w.Wait() and conditionally call ww.log.Debug("worker stopped", ...) with "error", err only if err is non-nil, and likewise when logging "worker destroyed" (both in the wait method that references w.Wait and ww.log.Debug) so you don't emit `"error": nil` in debug output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pool/static_pool/dyn_allocator.go`:
- Line 68: The call new(time.Now().UTC()) is invalid Go; replace it by creating
a time value and storing its address. For example, compute t := time.Now().UTC()
and then call da.lastAllocTry.Store(&t) (or store t directly if lastAllocTry
expects a time.Time), updating the usage around da.lastAllocTry.Store to use the
local t variable instead of new(...).
---
Nitpick comments:
In @.gitignore:
- Line 33: The .gitignore entry ".claude/settings.local.json" is an unrelated
housekeeping change and should be moved out of this logging-migration PR; remove
that line from the current diff (the .gitignore modification) and create a
separate commit/PR that adds the ".claude/settings.local.json" entry (or stash
the change, create a new branch, commit there, and open a new PR) so the current
PR only contains zap→slog, SDK v2, and goridge v4 changes.
In `@fsm/fsm.go`:
- Line 41: Change the structured log key in the s.log.Debug call inside the FSM
(function/method where s.log.Debug is invoked) from the ambiguous key "debug" to
a clearer name like "details" or "reason" so the error context is more
descriptive; update the call on s.log.Debug(...) to use the new key (e.g.,
"details") while keeping the same message and err.Error() argument.
In `@ipc/pipe/pipe_test.go`:
- Line 445: The benchmark is using slog.Default() instead of the test-local
logger, causing stderr output; update the call to NewPipeFactory in the
benchmark to use the shared test logger (the log variable defined in
pipe_spawn_test.go) so it matches other tests—i.e., replace slog.Default() with
log when calling NewPipeFactory(...). Ensure the change is applied to the call
site that invokes NewPipeFactory(...).SpawnWorkerWithContext(ctx, cmd).
In `@pool/static_pool/options.go`:
- Around line 9-12: The parameter name `z` in the option constructor WithLogger
is unclear; rename the parameter to `logger` in the function signature and
update its usage inside the returned closure (function WithLogger(logger
*slog.Logger) Options { ... p.log = logger ... }) to improve API readability
while keeping behavior unchanged.
In `@worker_watcher/worker_watcher.go`:
- Around line 420-433: In WorkerWatcher.wait, avoid logging a nil error: change
the ww.log.Debug calls around w.Wait() so they only include the "error" key when
err != nil; use the result of w.Wait() and conditionally call
ww.log.Debug("worker stopped", ...) with "error", err only if err is non-nil,
and likewise when logging "worker destroyed" (both in the wait method that
references w.Wait and ww.log.Debug) so you don't emit `"error": nil` in debug
output.
In `@worker/worker.go`:
- Around line 469-473: The Write method on Process currently logs incoming bytes
at Info level in func (w *Process) Write, which is stderr and should be treated
as warnings/errors; change the logging call from w.log.Info(...) to a
warning-level call (e.g., w.log.Warn or w.log.Warnf) so stderr output is emitted
at Warn level or higher; keep the existing behavior of converting p to string
(or optionally trim trailing newlines) but ensure the log API used is the
warning-level method on w.log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dad128a3-dd66-44f7-b5d0-1cf03daf3c4c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.gitignorefsm/fsm.gofsm/state_test.gogo.modinternal/protocol.goipc/pipe/pipe.goipc/pipe/pipe_spawn_test.goipc/pipe/pipe_test.goipc/socket/socket.goipc/socket/socket_spawn_test.goipc/socket/socket_test.gopool/allocator.gopool/static_pool/debug.gopool/static_pool/dyn_allocator.gopool/static_pool/dyn_allocator_test.gopool/static_pool/fuzz_test.gopool/static_pool/options.gopool/static_pool/pool.gopool/static_pool/pool_test.gopool/static_pool/stream.gopool/static_pool/supervisor.gopool/static_pool/supervisor_test.gostate/process/state.goworker/options.goworker/worker.goworker/worker_test.goworker_watcher/container/channel/vec.goworker_watcher/container/channel/vec_test.goworker_watcher/worker_watcher.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44 +/- ##
==========================================
+ Coverage 70.34% 72.25% +1.90%
==========================================
Files 19 19
Lines 1494 1492 -2
==========================================
+ Hits 1051 1078 +27
+ Misses 389 361 -28
+ Partials 54 53 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pool/static_pool/supervisor_test.go`:
- Around line 716-732: The test currently may skip or false-pass because it
doesn't ensure a worker entered Working and doesn't verify replacement PID;
modify the assertion block that uses p.Exec, p.Workers(),
workers[i].State().CurrentState() so it first waits (with a small retry
loop/time.After timeout) until the observed worker's state becomes
fsm.StateWorking (fail the test if timeout or no workers), then wait until its
state transitions to fsm.StateInvalid (or times out), and if you observe
fsm.StateReady use the worker PID before the transition and assert the current
worker PID differs to prove replacement; apply the same change to the other
occurrence at lines 794-806.
In `@worker_watcher/worker_watcher_test.go`:
- Around line 31-47: fakeAllocator spawns a long-lived "sleep" process but
doesn't clean it up, leaking child processes when tests call shutdownWatcher
(which doesn't invoke watcher.Destroy()); after successfully starting cmd in
fakeAllocator (and after creating w via worker.InitBaseWorker), register a
t.Cleanup handler that ensures the started process is terminated and reaped
(e.g., kill/terminate the cmd.Process and call cmd.Wait(), or call w.Destroy()
if that properly stops the child) so the test run doesn't leave lingering
"sleep" processes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1024b4e8-39c4-435d-a209-4a1146f57fec
📒 Files selected for processing (7)
fsm/state_test.gopool/static_pool/dyn_allocator_test.gopool/static_pool/pool_test.gopool/static_pool/supervisor_test.goworker_watcher/container/channel/vec.goworker_watcher/container/channel/vec_test.goworker_watcher/worker_watcher_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- worker_watcher/container/channel/vec.go
- pool/static_pool/pool_test.go
- worker_watcher/container/channel/vec_test.go
- pool/static_pool/dyn_allocator_test.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pool/static_pool/debug_test.go (1)
68-70: Consider checkingresp.Error()before usingresp.Body().If the worker execution fails,
resp.Body()might contain an error message instead of the expected PID, leading to misleading test behavior. For robustness, check for errors before collecting PIDs.🔧 Suggested improvement
r, err := p.Exec(t.Context(), &payload.Payload{Body: []byte("hello")}, make(chan struct{})) require.NoError(t, err) resp := <-r + require.NoError(t, resp.Error()) pids[string(resp.Body())] = struct{}{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pool/static_pool/debug_test.go` around lines 68 - 70, The test currently reads resp := <-r and immediately uses resp.Body() to collect PIDs; instead check resp.Error() first (via resp.Error()) and fail the test or skip that response if non-nil before using resp.Body(), e.g., use require.NoError(t, resp.Error()) or t.Fatalf with the error to avoid treating error messages as PIDs; update the collection logic around resp, resp.Error(), and resp.Body() in the test to validate the response before inserting into the pids map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pool/static_pool/debug_test.go`:
- Around line 68-70: The test currently reads resp := <-r and immediately uses
resp.Body() to collect PIDs; instead check resp.Error() first (via resp.Error())
and fail the test or skip that response if non-nil before using resp.Body(),
e.g., use require.NoError(t, resp.Error()) or t.Fatalf with the error to avoid
treating error messages as PIDs; update the collection logic around resp,
resp.Error(), and resp.Body() in the test to validate the response before
inserting into the pids map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ae52a0d-ef90-46f0-b22f-ef1d72bb2eaf
📒 Files selected for processing (12)
fsm/fsm.goipc/pipe/pipe_test.goipc/socket/socket.goipc/socket/socket_spawn_test.goipc/socket/socket_test.gopool/static_pool/debug_test.gopool/static_pool/options.gopool/static_pool/options_test.gopool/static_pool/supervisor_test.goworker/options.goworker_watcher/worker_watcher.goworker_watcher/worker_watcher_test.go
✅ Files skipped from review due to trivial changes (2)
- pool/static_pool/options_test.go
- worker_watcher/worker_watcher_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pool/static_pool/options.go
- ipc/socket/socket_test.go
- worker/options.go
- fsm/fsm.go
- worker_watcher/worker_watcher.go
- ipc/socket/socket_spawn_test.go
- ipc/socket/socket.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
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.
Reason for This PR
Description of Changes
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]git commit -s).CHANGELOG.md.Summary by CodeRabbit