feat: add bulk DAG-run deletion in web UI#2009
Merged
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds DAG run deletion functionality across the API specification, backend service, and frontend UI. It introduces a DELETE endpoint for concrete DAG run IDs (excluding 'latest'), enforces authorization and state validation, and provides batch deletion capabilities in the UI with confirmation dialogs. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Frontend UI
participant Hook as useDAGRunBatchSubmission
participant API as API Handler<br/>(DeleteDAGRun)
participant Auth as Authorization
participant Store as DAGRunStore
participant Log as Audit Log
User->>UI: Click "Delete selected" batch action
UI->>UI: Show confirmation dialog<br/>with delete warning
User->>UI: Confirm deletion
UI->>Hook: submitBatchItem(action='delete',<br/>name, dagRunId, remoteNode)
Hook->>API: DELETE /dag-runs/{name}/{dagRunId}<br/>?remoteNode=...
API->>Auth: Check developer authorization
Auth-->>API: ✓ Authorized
API->>API: Validate dagRunId !== 'latest'
API->>Store: FindAttempt(name, dagRunId)
Store-->>API: DAG run attempt + status
API->>API: Check if status.IsActive()
alt Active Run
API-->>Hook: HTTP 400 (stop/dequeue first)
Hook-->>UI: Show error message
else Inactive Run
API->>Store: RemoveDAGRun(name, dagRunId)
Store-->>API: ✓ Deleted
API->>Log: Log audit event<br/>(dag_run_delete)
Log-->>API: ✓ Logged
API-->>Hook: HTTP 204 No Content
Hook-->>UI: { ok: true }
UI->>UI: Show "Delete request accepted"
UI->>UI: Update selection (clear)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ui/src/features/dag-runs/hooks/useDAGRunBatchSubmission.ts (1)
111-230: Guard against implicit fallthrough to reschedule.
submitBatchItemdispatchesretryanddeleteexplicitly but treats every other value asreschedule. If a newBatchActionTypeis added to the union in the future, it will silently fall through and perform a reschedule — which for a destructive or unrelated action would be a correctness bug. Consider aswitch(action)with an exhaustivenevercheck (or an explicitif (action === 'reschedule')guard with a final throw).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dag-runs/hooks/useDAGRunBatchSubmission.ts` around lines 111 - 230, submitBatchItem currently treats any action other than 'retry' and 'delete' as a reschedule, causing silent fallthrough if BatchActionType later expands; update submitBatchItem to use a switch(action) (or an explicit if (action === 'reschedule') branch) and add an exhaustive check that throws on unknown actions (use a never/assertUnreachable pattern) so new/invalid actions fail fast instead of implicitly performing reschedule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/api.yaml`:
- Around line 6936-6945: The DAGRunConcreteId schema currently allows the
literal "latest" but must reject it for concrete/delete endpoints; update the
DAGRunConcreteId schema (the name DAGRunConcreteId and its pattern field in the
spec) to explicitly exclude "latest" (for example by using a negative lookahead
in the regex such as one that rejects the exact string "latest" while still
allowing the existing [a-zA-Z0-9_-]+ tokens), save the OpenAPI spec, and then
regenerate the server (run make api) so the updated validation is applied.
In `@internal/service/frontend/api/v1/dagruns.go`:
- Around line 1721-1750: TOCTOU: ensure the active-state check and removal are
atomic by either acquiring the per-run lock before reading status or moving the
guard into RemoveDAGRun so it is checked under the same lock; specifically, in
the handler around attempt.ReadStatus and a.dagRunStore.RemoveDAGRun, acquire
the same per-run synchronization used by DequeueDAGRun (or call a dagRunStore
method that does so), then re-read status while holding the lock and refuse
deletion if status.IsActive(); alternatively, implement the active-state check
inside RemoveDAGRun (under its lock) so RemoveDAGRun atomically rejects active
runs before removing.
---
Nitpick comments:
In `@ui/src/features/dag-runs/hooks/useDAGRunBatchSubmission.ts`:
- Around line 111-230: submitBatchItem currently treats any action other than
'retry' and 'delete' as a reschedule, causing silent fallthrough if
BatchActionType later expands; update submitBatchItem to use a switch(action)
(or an explicit if (action === 'reschedule') branch) and add an exhaustive check
that throws on unknown actions (use a never/assertUnreachable pattern) so
new/invalid actions fail fast instead of implicitly performing reschedule.
🪄 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: 1a49f7af-c314-4ad7-aafd-fcd814312e8c
📒 Files selected for processing (8)
api/v1/api.gen.goapi/v1/api.yamlinternal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_test.goui/src/api/v1/schema.tsui/src/features/dag-runs/components/common/DAGRunBatchActions.tsxui/src/features/dag-runs/components/common/__tests__/DAGRunBatchActions.test.tsxui/src/features/dag-runs/hooks/useDAGRunBatchSubmission.ts
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
1 participant
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
latestaliases or active DAG-runs while removing completed run data through the existing store cleanup pathTesting
Summary by CodeRabbit