feat: use ding for ordered go routine shutdown order#896
Merged
Conversation
dosubot
Bot
added
the
size:L
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughReplaces sync.WaitGroup coordination with an injected *ding.Ding across bootstrap, router, services, and tests; background goroutines are scheduled with ring priorities and accept context.Context for shutdown. ChangesDing Lifecycle Refactor
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
go.mod (1)
143-143: ⚡ Quick winDependency should be direct, not indirect.
The
dingpackage is directly imported inapp_bootstrap.goandrouter_bootstrap.go, so it should be listed as a direct dependency (without// indirect). Rungo mod tidyto correct this.🤖 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 `@go.mod` at line 143, The go.mod entry for github.com/steveiliop56/ding is marked // indirect but the package is directly imported by app_bootstrap.go and router_bootstrap.go; fix this by updating module metadata — run `go mod tidy` (or remove the `// indirect` marker and re-run `go mod tidy`) so github.com/steveiliop56/ding becomes a direct dependency, then verify imports in app_bootstrap.go and router_bootstrap.go still build.internal/bootstrap/app_bootstrap.go (2)
383-386: 💤 Low valueRedundant
ticker.Stop()call.Same issue as
heartbeatRoutine— the defer at line 369 handles cleanup.🧹 Suggested cleanup
case <-ctx.Done(): app.log.App.Debug().Msg("Stopping database cleanup routine") - ticker.Stop() return }🤖 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 `@internal/bootstrap/app_bootstrap.go` around lines 383 - 386, Redundant explicit ticker.Stop() inside the select case for <-ctx.Done() should be removed because the ticker is already stopped by the defer ticker.Stop() declared earlier in the same function; locate the function containing the ticker variable and the select with the <-ctx.Done() branch (the database cleanup routine) and delete the ticker.Stop() call from that case so the defer handles cleanup only once.
359-362: 💤 Low valueRedundant
ticker.Stop()call.Line 361 is unnecessary since
defer ticker.Stop()at line 308 already ensures the ticker is stopped when the function returns.🧹 Suggested cleanup
case <-ctx.Done(): app.log.App.Debug().Msg("Stopping heartbeat routine") - ticker.Stop() return }🤖 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 `@internal/bootstrap/app_bootstrap.go` around lines 359 - 362, Remove the redundant ticker.Stop() call in the ctx cancellation branch of the heartbeat routine: the ticker is already stopped by the defer ticker.Stop() declared earlier (around line 308), so delete the explicit ticker.Stop() inside the case <-ctx.Done() block in the function handling the heartbeat loop (the routine that logs "Stopping heartbeat routine") to avoid duplicate cleanup calls.
🤖 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.
Nitpick comments:
In `@go.mod`:
- Line 143: The go.mod entry for github.com/steveiliop56/ding is marked //
indirect but the package is directly imported by app_bootstrap.go and
router_bootstrap.go; fix this by updating module metadata — run `go mod tidy`
(or remove the `// indirect` marker and re-run `go mod tidy`) so
github.com/steveiliop56/ding becomes a direct dependency, then verify imports in
app_bootstrap.go and router_bootstrap.go still build.
In `@internal/bootstrap/app_bootstrap.go`:
- Around line 383-386: Redundant explicit ticker.Stop() inside the select case
for <-ctx.Done() should be removed because the ticker is already stopped by the
defer ticker.Stop() declared earlier in the same function; locate the function
containing the ticker variable and the select with the <-ctx.Done() branch (the
database cleanup routine) and delete the ticker.Stop() call from that case so
the defer handles cleanup only once.
- Around line 359-362: Remove the redundant ticker.Stop() call in the ctx
cancellation branch of the heartbeat routine: the ticker is already stopped by
the defer ticker.Stop() declared earlier (around line 308), so delete the
explicit ticker.Stop() inside the case <-ctx.Done() block in the function
handling the heartbeat loop (the routine that logs "Stopping heartbeat routine")
to avoid duplicate cleanup calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c938dea0-7bc3-491e-886a-59f09f066b93
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modinternal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/controller/oidc_controller_test.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/controller/well_known_controller_test.gointernal/middleware/context_middleware_test.gointernal/service/auth_service.gointernal/service/docker_service.gointernal/service/kubernetes_service.gointernal/service/ldap_service.gointernal/service/oidc_service.gointernal/service/oidc_service_test.gointernal/service/tailscale_service.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@go.mod`:
- Line 18: The go.mod currently depends on an unstable personal library
"github.com/steveiliop56/ding v0.1.0" for goroutine lifecycle, which is risky
for production; either replace/remove that dependency by switching to a stable
alternative (e.g., use golang.org/x/sync/errgroup with context in your goroutine
coordination code where you currently use ding), or vendor/fork and pin the ding
code under your org with tests and a migration plan, or explicitly document and
accept the risk in project docs and add upgrade/migration steps; locate the
dependency reference "github.com/steveiliop56/ding v0.1.0" in go.mod and update
your code paths that use ding's APIs (the goroutine/waitgroup replacement
functions) to the chosen approach.
- Line 18: The dependency github.com/steveiliop56/ding v0.1.0 lacks demonstrated
goroutine lifecycle guarantees; either replace it with a more established
alternative or add concrete evidence: update code that uses ding to implement
proper context cancellation and coordinated shutdown (use context.Context
propagation and a WaitGroup or equivalent in the components that spawn ding
goroutines), add unit/integration tests that exercise cancellation/shutdown
paths and detect leaks (race detector and deadline-based shutdown tests), and
add CI steps that run go test -race and the shutdown/leak tests; reference the
module name github.com/steveiliop56/ding and the code locations that spawn its
goroutines for these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Rycochet
approved these changes
May 27, 2026
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
2 participants
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.
Summary by CodeRabbit