fix: correct pubsub provider lifecycle - #3038
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 3d5f1edafc4cbbf4d0fa27a70638c24a54c33fa9 and 8deefb9. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughPubsub provider ownership moves from ChangesRouter PubSub Lifecycle Refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
c972bab to
2d466e1
Compare
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
- Coverage 61.46% 60.39% -1.07%
==========================================
Files 261 261
Lines 30596 30594 -2
==========================================
- Hits 18805 18478 -327
- Misses 10277 10581 +304
- Partials 1514 1535 +21
🚀 New features to boost your workflow:
|
2d466e1 to
440fb1f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/core/graph_server_test.go (1)
812-846: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover provider context lifetime, not only
Shutdowncalls.This test would still pass if reused providers were started with the previous server context and then stopped when
prev.Shutdowncancels that context. Consider extending the fake provider to capture its startup context and assert it remains live for reused muxes.🤖 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 `@router/core/graph_server_test.go` around lines 812 - 846, The current TestGraphServerShutdown only asserts that reuseTrackingProvider.shutdown stays false after prev.Shutdown, but it does not verify the provider was started with a context that outlives the previous server. Update the fake provider used in graphServer/commitReusedMuxes to capture its startup context, then extend the TestGraphServerShutdown reused-mux case to assert that the provider’s context is still active after prev.Shutdown, using the existing reuseTrackingProvider, graphServer, and commitReusedMuxes symbols to locate the right test and helper.
🤖 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 `@router/core/graph_server.go`:
- Around line 1543-1547: The pub/sub providers are being started against
s.graphServerCtx and only attached to gm.pubSubProviders after startup, which
breaks reused muxes and leaves partially started providers untracked on failure.
Update buildGraphMux/startupPubSubProviders so providers are registered on the
graph mux before startup and are started with the mux’s own context/lifecycle,
and ensure any startup error path cleans up already-started providers through
gm.pubSubProviders/graphMux.Shutdown handling.
---
Nitpick comments:
In `@router/core/graph_server_test.go`:
- Around line 812-846: The current TestGraphServerShutdown only asserts that
reuseTrackingProvider.shutdown stays false after prev.Shutdown, but it does not
verify the provider was started with a context that outlives the previous
server. Update the fake provider used in graphServer/commitReusedMuxes to
capture its startup context, then extend the TestGraphServerShutdown reused-mux
case to assert that the provider’s context is still active after prev.Shutdown,
using the existing reuseTrackingProvider, graphServer, and commitReusedMuxes
symbols to locate the right test and helper.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e5cf0f5-1aeb-410b-8366-585c3cecb346
📥 Commits
Reviewing files that changed from the base of the PR and between 1d3b30e and 440fb1f12d580c417538ce0f2ebb7cf5541dfd4d.
📒 Files selected for processing (2)
router/core/graph_server.gorouter/core/graph_server_test.go
9855988 to
3d5f1ed
Compare
3d5f1ed to
356c72d
Compare
dkorittki
left a comment
dkorittki
left a comment
There was a problem hiding this comment.
lgtm. Commented some optional things and fixed the failing test
| // pubSubProviders are the EDFS providers built for this mux. They are owned by | ||
| // the mux (not the server) so that a mux reused by the next server keeps its | ||
| // providers alive: Shutdown skips reused muxes, so their providers are not torn | ||
| // down until the mux itself is finally discarded. |
There was a problem hiding this comment.
imo we don't need this comment
| func (s *graphServer) shutdownPubSubProviders(ctx context.Context) error { | ||
| // Default timeout for pubsub provider shutdown | ||
| const defaultShutdownTimeout = 5 * time.Second | ||
|
|
||
| return s.providersActionWithTimeout(ctx, func(ctx context.Context, provider datasource.Provider) error { | ||
| return provider.Shutdown(ctx) | ||
| }, defaultShutdownTimeout, "pubsub provider shutdown timed out") | ||
| } |
There was a problem hiding this comment.
I would also remove the startupPubSubProviders method from the graph server. The graph server is not responsible for anything regarding pubsub providers anymore, so imo it makes sense there are no methods related to this on a graph server.
| const defaultShutdownTimeout = 5 * time.Second | ||
| if pErr := providersActionWithTimeout(ctx, s.pubSubProviders, func(ctx context.Context, provider datasource.Provider) error { | ||
| return provider.Shutdown(ctx) | ||
| }, defaultShutdownTimeout, "pubsub provider shutdown timed out"); pErr != nil { | ||
| err = errors.Join(err, pErr) | ||
| } |
There was a problem hiding this comment.
Idea: Have start/stop methods for pubsub providers on graph muxes, which are called here instead of inlined code. Matches what startupPubSubProviders / shutdownPubSubProviders has been but for graph muxes instead of graph servers.
|
Further handled via #3047 |
2 participants
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.