feat: workers integration and lifecycle hooks#85
Conversation
…aces Add 5 new optional interfaces following the existing CBStopper/CBGracefulStopper pattern — services implement only what they need, discovered via type assertion: - CBWorkerProvider: run background workers managed by go-coldbrew/workers - CBPreStarter: setup before servers start (DB connect, interceptor config) - CBPostStarter: act after servers are listening (service discovery) - CBPreStopper: act before graceful shutdown (deregister, flush) - CBPostStopper: final cleanup after everything stops Run() lifecycle: PreStart → initGRPC → initHTTP → start workers → start servers → PostStart → block → PreStop → FailCheck → drain → stop workers → stop servers → Stop → PostStop Also adds env:"" tags alongside envconfig:"" on all Config fields for modern ecosystem compat, and ValidateStrict() []error for programmatic use. 100% backward compatible — CB interface unchanged, existing services work without modification.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes introduce a service lifecycle hook system with optional interfaces for pre/post startup and shutdown phases, plus worker provider support. Core server logic now invokes these hooks during startup/shutdown sequences. Configuration validation gains strict mode, and toolchain dependencies are updated. Changes
Sequence DiagramsequenceDiagram
participant Server as Server (cb)
participant Service as Service<br/>(optional hooks)
participant Workers as Workers
participant GRPC as gRPC/HTTP<br/>Servers
rect rgba(100, 200, 100, 0.5)
Note over Server,GRPC: Startup Phase
Server->>Service: PreStart(ctx)
Service-->>Server: error or nil
Server->>Service: Workers()
Service-->>Server: []*workers.Worker
Server->>Workers: Start workers
Server->>GRPC: Initialize & launch
Server->>Service: PostStart(ctx)
Service-->>Server: void
end
rect rgba(200, 100, 100, 0.5)
Note over Server,GRPC: Shutdown Phase
Server->>Service: PreStop(ctx)
Service-->>Server: void
Server->>Workers: Cancel execution
Server->>GRPC: Graceful shutdown
Server->>Service: PostStop(ctx)
Service-->>Server: void
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR extends ColdBrew’s service lifecycle with optional hooks and integrates go-coldbrew/workers so background workers can be started/stopped alongside the HTTP/gRPC servers. It also adds env:"" struct tags to config fields and introduces a ValidateStrict() helper for programmatic config validation.
Changes:
- Add optional lifecycle interfaces:
CBWorkerProvider,CBPreStarter,CBPostStarter,CBPreStopper,CBPostStopper. - Start workers in
Run()(viaerrgroup) and cancel them duringStop()to participate in graceful shutdown. - Add
env:""tags across config, plusConfig.ValidateStrict() []error, and update docs/tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
types.go |
Introduces new optional lifecycle and worker-provider interfaces. |
core.go |
Wires PreStart/PostStart and PreStop/PostStop hooks into Run/Stop; runs workers via workers.Run. |
lifecycle_test.go |
Adds tests for optional interface discovery and basic hook invocation behavior. |
config/config.go |
Adds env:"" tags and introduces ValidateStrict() wrapper. |
config/README.md |
Regenerates config docs to include ValidateStrict() and tag updates. |
README.md |
Regenerates main docs to include new interfaces. |
go.mod / go.sum |
Adds go-coldbrew/workers dependency (and indirect suture). |
Makefile |
Updates make test to run with coverage enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix ValidateStrict doc comment to accurately describe behavior - Add sequence tracking to TestStopHooks to verify hook ordering (PreStop → FailCheck → Stop → PostStop)
- Remove redundant context.Canceled check (workers.Run already returns nil on cancellation) - Use errors.New instead of fmt.Errorf in ValidateStrict - Fix "shutdow" typo in Stop() comment - Bump go-coldbrew/workers v0.2.0 → v0.2.1
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
💡 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.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lifecycle_test.go (3)
158-168:TestPreStart_SkippedForPlainServiceis tautological.This test only asserts that
*plainServicedoesn't satisfyCBPreStarter, which is already covered byTestOptionalInterfaces_Discoveryand is enforced at compile time anyway (sinceplainServicehas noPreStartmethod). Consider removing it, or replacing it with a test that callsc.Run()on aplainServiceand asserts no PreStart-related panic/error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifecycle_test.go` around lines 158 - 168, The test TestPreStart_SkippedForPlainService is tautological (it only asserts that *plainService does not implement CBPreStarter) — remove it or replace it with a behavioral test: instantiate plainService as a CBService and call its Run (or the lifecycle runner that invokes PreStart) and assert that no panic or error occurs related to PreStart; locate the test named TestPreStart_SkippedForPlainService and either delete it or rewrite it so it exercises c.Run() (or the lifecycle runner used in tests) against plainService and verifies normal completion without PreStart invocation.
27-30: Sequence fields aren't goroutine-safe — fine for current tests, brittle later.
preStopSeq,failCheckSeq,stopSeq,postStopSeqare plainint32written from the hook callbacks. TodayStop()invokes the hooks serially on a single goroutine so this is fine, but if someone later parallelizes hook invocation (or adds a test that runs Stop concurrently),-racewill fire. Either drop them toatomic.Int32(you already use that for the bool flags) or add a comment that they assume serial invocation.Also applies to: 52-58, 73-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifecycle_test.go` around lines 27 - 30, The sequence counters preStopSeq, failCheckSeq, stopSeq and postStopSeq are plain int32 and can race when hooks run concurrently; change their types to atomic.Int32 (consistent with existing atomic flags) and update all increments/reads (locations around the other occurrences you noted) to use atomic methods (AddInt32/LoadInt32) in the hook callbacks and tests so access is goroutine-safe.
170-207:TestWorkerProvider_*tests reimplement the production loop instead of exercising it.These two tests copy-paste the type-assertion + collection loop from
Run()rather than actually callingc.Run()(or a small extracted helper). If someone refactors the collection logic incore.go, these tests will keep passing even when production behavior is broken.Consider extracting the worker-collection step into a small helper (e.g.
func (c *cb) collectWorkers() []*workers.Worker) and asserting against that, so the tests cover real code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lifecycle_test.go` around lines 170 - 207, The tests duplicate the worker-collection loop instead of exercising production code; extract the loop from Run() into a new helper method func (c *cb) collectWorkers() []*workers.Worker that performs the type-assertion against CBWorkerProvider and returns the collected []*workers.Worker, update Run() to call collectWorkers(), and change the two tests TestWorkerProvider_CollectsWorkers and TestWorkerProvider_SkippedForPlainService to call c.collectWorkers() (or assert via c.Run() if preferable) and verify the returned slice and that svc.workersCalled is set; keep the existing Workers() method name and CBWorkerProvider interface usage so tests and Run() use the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config.go`:
- Around line 298-306: Update the documentation so it matches the actual
behavior of Config.ValidateStrict: it converts every string returned by
Config.Validate() into an error and returns an empty slice (nil-equivalent) when
there are no issues, rather than implying a filtered "fatal-only" subset; edit
the doc text in the comment on the ValidateStrict method if needed to be
explicit, then run the documentation generator (make doc) to regenerate
config/README.md so the README reflects the exact behavior of
Config.ValidateStrict and Config.Validate.
In `@core.go`:
- Around line 907-913: The PostStart hooks (CBPostStarter/PostStart) are running
before servers have actually bound sockets because Run() currently starts server
goroutines with g.Go(...) but doesn't create listeners first; update Run() to
eagerly create listeners (call net.Listen for TCP/gRPC ports and reuse the
existing unix socket listener pattern) and pass those listeners into the
Serve/ServeGRPC goroutines so that after listeners are created you can invoke
PostStart, or alternatively change the comment and CBPostStarter docs to state
hooks run after server goroutines are scheduled (not after sockets are bound);
modify the code paths around g.Go(...), listener creation, and the PostStart
invocation to implement the chosen approach and ensure PostStart is called only
after net.Listen has succeeded for each server.
- Around line 783-791: If PreStart on service N fails, earlier services in c.svc
whose PreStart already returned nil are left running and never cleaned up;
modify the Run() startup loop (the PreStart section that iterates over c.svc and
calls CBPreStarter.PreStart) to perform rollback cleanup before returning the
error by invoking each previously-started service's Stop() and/or PostStop()
(use the same CBStop/CBPostStop interfaces or methods you already have) in
reverse order, ensuring errors from cleanup are aggregated or logged but do not
mask the original PreStart error; alternatively, document in Run()'s contract
that callers must call Stop() on failure if you choose not to auto-cleanup.
- Around line 971-975: The comment above the shutdown sequence contradicts the
actual ordering: it claims "Stop workers before servers..." but the code calls
c.workerCancel() before grpcServer.GracefulStop, which causes workers to be
cancelled before servers drain; decide desired behavior and make code and
comment consistent—either move the c.workerCancel() call to after the
grpcServer.GracefulStop call if you want servers to drain while workers remain
active, or keep the current call order and update the comment to state that
workers are cancelled before servers to prevent new outbound work; update the
comment text near workerCancel and grpcServer.GracefulStop accordingly to
reflect the chosen intent.
---
Nitpick comments:
In `@lifecycle_test.go`:
- Around line 158-168: The test TestPreStart_SkippedForPlainService is
tautological (it only asserts that *plainService does not implement
CBPreStarter) — remove it or replace it with a behavioral test: instantiate
plainService as a CBService and call its Run (or the lifecycle runner that
invokes PreStart) and assert that no panic or error occurs related to PreStart;
locate the test named TestPreStart_SkippedForPlainService and either delete it
or rewrite it so it exercises c.Run() (or the lifecycle runner used in tests)
against plainService and verifies normal completion without PreStart invocation.
- Around line 27-30: The sequence counters preStopSeq, failCheckSeq, stopSeq and
postStopSeq are plain int32 and can race when hooks run concurrently; change
their types to atomic.Int32 (consistent with existing atomic flags) and update
all increments/reads (locations around the other occurrences you noted) to use
atomic methods (AddInt32/LoadInt32) in the hook callbacks and tests so access is
goroutine-safe.
- Around line 170-207: The tests duplicate the worker-collection loop instead of
exercising production code; extract the loop from Run() into a new helper method
func (c *cb) collectWorkers() []*workers.Worker that performs the type-assertion
against CBWorkerProvider and returns the collected []*workers.Worker, update
Run() to call collectWorkers(), and change the two tests
TestWorkerProvider_CollectsWorkers and TestWorkerProvider_SkippedForPlainService
to call c.collectWorkers() (or assert via c.Run() if preferable) and verify the
returned slice and that svc.workersCalled is set; keep the existing Workers()
method name and CBWorkerProvider interface usage so tests and Run() use the same
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 601c766e-fe45-4a6e-908b-08e3c3092f7b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
MakefileREADME.mdconfig/README.mdconfig/config.gocore.gogo.modlifecycle_test.gotypes.go
- Fix misleading comment about worker stop ordering - Regenerate config/README.md to include ValidateStrict godoc
Summary
CBWorkerProvider,CBPreStarter,CBPostStarter,CBPreStopper,CBPostStopper) following the existingCBStopper/CBGracefulStopperpatterngo-coldbrew/workersintoRun()/Stop()lifecycle — workers start in errgroup alongside servers, stopped during graceful shutdownenv:""tags on all Config fields alongsideenvconfig:""for modern ecosystem compatConfig.ValidateStrict() []errorfor programmatic validationLifecycle
Test plan
make test— all existing tests pass (backward compat)make lint— 0 issues, govulncheck cleanmake bench— no regressionlifecycle_test.go— interface discovery, PreStart error abort, worker collection, Stop hook ordering, plain service compatSummary by CodeRabbit
New Features
Documentation
Tests