Repository navigation
fix: preserve scoped notification routing - #2469
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDAG files and workspace labels now persist from task and attempt status through event snapshots and notification events. Notification and incident services use ChangesEvent metadata and routing
Managed process lifecycle
Scheduler test isolation
Filesystem identity validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant Coordinator
participant EventStore
participant NotificationEvent
participant NotificationService
Worker->>Coordinator: reports source file and labels
Coordinator->>EventStore: persists status metadata
EventStore->>NotificationEvent: supplies DAG file and labels
NotificationEvent->>NotificationService: provides DAGKey()
NotificationService-->>NotificationEvent: returns matching scoped destinations
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- proto/coordinator/v1/coordinator.pb.go: Generated file
- proto/coordinator/v1/coordinator_protoopaque.pb.go: Generated file
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/incident/service_test.go (1)
238-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
PolicyIDpreservation to directly cover the fix.This test correctly validates that resolution keys off
event.Status.Name("daily") rather thanevent.DAGFile("daily-file"). Sinceinternal/service/incident/service.goLine 625 to Line 627 now preserves the existingPolicyIDinstead of always overwriting it, add an assertion thatstate.PolicyIDstill equals"existing-policy"after Line 282. This directly verifies the behavior that motivated that change.✅ Proposed assertion addition
state, err = store.GetState(context.Background(), provider.ID, "existing-dedup-key") require.NoError(t, err) assert.Equal(t, incidentmodel.IncidentStatusResolved, state.Status) + assert.Equal(t, "existing-policy", state.PolicyID) }🤖 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/incident/service_test.go` around lines 238 - 283, Extend TestServiceResolvesRuntimeNameIncidentWithDAGFileRouting to assert that the resolved state retains the existing PolicyID value "existing-policy" after retrieving it with GetState. Keep the assertion alongside the existing status verification.
🤖 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 `@internal/service/incident/service_test.go`:
- Around line 238-283: Extend
TestServiceResolvesRuntimeNameIncidentWithDAGFileRouting to assert that the
resolved state retains the existing PolicyID value "existing-policy" after
retrieving it with GetState. Keep the assertion alongside the existing status
verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cff73517-beb8-419a-aae8-178bfe6f4c30
⛔ Files ignored due to path filters (2)
proto/coordinator/v1/coordinator.pb.gois excluded by!**/*.pb.goproto/coordinator/v1/coordinator_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
internal/service/coordinator/handler.gointernal/service/coordinator/handler_test.gointernal/service/incident/service.gointernal/service/incident/service_test.gointernal/service/worker/coordreport/status_pusher.gointernal/service/worker/coordreport/status_pusher_test.gointernal/service/worker/remote_handler.goproto/coordinator/v1/coordinator.proto
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 22 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/persis/file/dagrun/attempt.go (1)
175-182: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd tests for successful DAG restoration.
TestAttempt_OpenRejectsCorruptDAGDefinitioncovers only the error path. Add tests that reopen an attempt with a validDAGDefinitionand verify that the persisted DAG metadata is restored. Also verify that a missingDAGDefinitionremains non-fatal.🤖 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/persis/file/dagrun/attempt.go` around lines 175 - 182, Add tests around Attempt.Open and the ReadDAG restoration branch covering a valid persisted DAGDefinition, asserting reopened attempts restore the DAG metadata, plus a missing DAGDefinition case that remains non-fatal. Keep the existing corrupt-definition rejection test unchanged and use the established attempt/DAG fixtures and assertions.
🤖 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/intg/reschedule_source_file_api_test.go`:
- Around line 81-84: Update requireSameFile to use fileutil.Stat for both
expected and actual paths instead of os.Stat, preserving the existing error
assertions and subsequent os.SameFile comparison.
---
Nitpick comments:
In `@internal/persis/file/dagrun/attempt.go`:
- Around line 175-182: Add tests around Attempt.Open and the ReadDAG restoration
branch covering a valid persisted DAGDefinition, asserting reopened attempts
restore the DAG metadata, plus a missing DAGDefinition case that remains
non-fatal. Keep the existing corrupt-definition rejection test unchanged and use
the established attempt/DAG fixtures and assertions.
🪄 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 Plus
Run ID: 6b3c1733-6d2c-46c0-8f7e-72acaea6668e
📒 Files selected for processing (4)
internal/intg/reschedule_source_file_api_test.gointernal/persis/file/dagrun/attempt.gointernal/persis/file/dagrun/attempt_test.gointernal/service/incident/service_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/persis/file/dagrun/attempt_test.go
- internal/service/incident/service_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- proto/coordinator/v1/coordinator.pb.go: Generated file
- proto/coordinator/v1/coordinator_protoopaque.pb.go: Generated file
Suppressed comments (1)
conformance/harness/runner.go:162
Process.Stopignores theManagedProcess.Stoperror/outcome. If the stop request fails (e.g., process-tree termination issues on Windows), the test harness may leak a runningdaguprocess and still proceed as if cleanup succeeded. Consider at least surfacing the error (and optionally logging partial outcomes) so CI flakes are diagnosable.
_, _ = p.proc.Stop(cmdutil.StopRequest{
Intent: cmdutil.ForceTermination(),
Reason: cmdutil.StopReasonShutdown,
})
})
|
Review follow-up: I did not add duplicate DAG restoration tests. TestAttempt_Open already proves a missing dag.json remains non-fatal, and TestAttempt_OpenRestoresLastEmittedLifecycleState reopens a valid persisted definition and verifies the restored DAG file in the emitted snapshot. The new corrupt-definition test covers the only previously missing branch. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- proto/coordinator/v1/coordinator.pb.go: Generated file
- proto/coordinator/v1/coordinator_protoopaque.pb.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 27 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- proto/coordinator/v1/coordinator.pb.go: Generated file
- proto/coordinator/v1/coordinator_protoopaque.pb.go: Generated file
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/service/incident/service.go">
<violation number="1" location="internal/service/incident/service.go:368">
P1: A transient `ListOpenStates` failure drops canonical state destinations, so the monitor prunes pending recovery batches instead of retrying them after storage recovers. Preserve the last successfully discovered state destinations (or otherwise avoid reconciliation/removal when this lookup fails).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| } | ||
| } | ||
| states, err := s.store.ListOpenStates(ctx) |
There was a problem hiding this comment.
P1: A transient ListOpenStates failure drops canonical state destinations, so the monitor prunes pending recovery batches instead of retrying them after storage recovers. Preserve the last successfully discovered state destinations (or otherwise avoid reconciliation/removal when this lookup fails).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/service/incident/service.go, line 368:
<comment>A transient `ListOpenStates` failure drops canonical state destinations, so the monitor prunes pending recovery batches instead of retrying them after storage recovers. Preserve the last successfully discovered state destinations (or otherwise avoid reconciliation/removal when this lookup fails).</comment>
<file context>
@@ -365,6 +365,16 @@ func (s *Service) NotificationDestinations() []string {
}
}
}
+ states, err := s.store.ListOpenStates(ctx)
+ if err != nil {
+ s.logger.Warn("Failed to list open incident destinations", slog.String("error", err.Error()))
</file context>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 27 changed files in this pull request and generated no new comments.
Files not reviewed (2)
- proto/coordinator/v1/coordinator.pb.go: Generated file
- proto/coordinator/v1/coordinator_protoopaque.pb.go: Generated file
Summary
Root cause
Notification and incident configuration is stored under the DAG file identifier, while DAG runs and lifecycle events use the runtime DAG name. Those values usually match, but YAML can declare a different name. Events did not retain the file identifier needed for scoped configuration lookup. Reconstructed attempts also lost their DAG metadata, and early distributed failures could replace persisted workspace labels with an empty set.
Impact
DAG-scoped settings now continue to apply when a YAML file declares a different runtime name. Workspace-scoped failures retain their routing boundary across coordinator recovery and cannot fall through to global routing because metadata was lost or invalid. Existing open incidents remain compatible because runtime-name state and
dagu:v1deduplication semantics are unchanged.CI hardening
An earlier Windows run exposed two test-only lifecycle problems while the same PR commit passed on rerun. Scheduler lifecycle tests were waiting on an unrelated file-backed service registry before observing startup, and conformance cleanup could stop the parent Dagu process without owning its child process tree. The follow-up commit removes the registry dependency from those focused scheduler tests and uses the existing managed-process abstraction for conformance commands. No production behavior changed, and no timeouts or retries were added.
Validation
make testmake lintCloses #2467
Summary by CodeRabbit