fix: refresh DAG list SSE after file changes#2319
Conversation
📝 WalkthroughWalkthrough
ChangesApp stream → multiplexer invalidation bridge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/frontend/server_test.go (1)
252-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit assertion that SSE bridge prerequisites initialized.
Right after
setupSSERoute, assertsrv.appStreamandsrv.sseMultiplexerare non-nil. Otherwise this test can fail later via timeout, hiding the root setup failure.🤖 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/service/frontend/server_test.go` around lines 252 - 260, The test lacks assertions to verify that the SSE bridge prerequisites were properly initialized after calling setupSSERoute. Add explicit assertions right after the setupSSERoute call to check that srv.appStream and srv.sseMultiplexer are both non-nil, so that any setup failure is caught immediately rather than manifesting as a timeout later in the test execution.
🤖 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 `@internal/service/frontend/server.go`:
- Around line 1199-1207: The app stream initialization failure at the
NewAppStreamService call only logs a warning and allows execution to continue
with srv.appStream remaining nil. This causes registerDedicatedSSEFetchers to
later put TopicTypeDAGsList into on-demand mode when eventService is enabled,
but the on-demand mode cannot function without a valid app stream, breaking DAG
list refresh on external edits. Either fail fast by returning the error when
NewAppStreamService fails instead of just logging a warning, or add a nil check
for srv.appStream in registerDedicatedSSEFetchers before switching
TopicTypeDAGsList to on-demand mode, keeping it in polling mode when the app
stream is unavailable.
---
Nitpick comments:
In `@internal/service/frontend/server_test.go`:
- Around line 252-260: The test lacks assertions to verify that the SSE bridge
prerequisites were properly initialized after calling setupSSERoute. Add
explicit assertions right after the setupSSERoute call to check that
srv.appStream and srv.sseMultiplexer are both non-nil, so that any setup failure
is caught immediately rather than manifesting as a timeout later in the test
execution.
🪄 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: c2607268-43cc-44fc-9919-6b062e1d32e6
📒 Files selected for processing (2)
internal/service/frontend/server.gointernal/service/frontend/server_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Root Cause
The workflow list consumes the multiplexed dagslist SSE topic. When the event store is enabled, that topic is on-demand and only refreshes after explicit wakeups. API mutations and DAG-run events already wake it, but external DAG file writes did not because setupSSERoute disabled the app filesystem stream that detects those changes. The detail/spec page fetched the DAG directly, so it could show the new schedule while the already-open workflow list still displayed the old schedule.
Testing
Closes #2316
Summary by cubic
Fixes stale workflow lists by waking the SSE
dagslisttopic when DAG files change. Starts the app filesystem watcher and bridges its events to multiplexed invalidation; falls back to polling when the watcher isn’t available.AppStreamServiceduring SSE setup; warn and continue if init fails.dagslist, DAG, runs, queues, docs, queue items, and reset topics.dagslistpolling without the app stream; switch it to on-demand with publish‑on‑wake when the watcher is running.Written for commit 37d642c. Summary will update on new commits.
Summary by CodeRabbit