feat(api): add singleton flag to enqueue API#1483
Conversation
WalkthroughAdds an optional Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as EnqueueDAGDAGRun
participant Singleton as ensureSingletonEnqueue
participant DAG as DAG Service
participant Runs as Runs DB
participant Queue as Queue Service
Client->>API: POST /dags/{name}/enqueue (singleton=true)
API->>Singleton: ensureSingletonEnqueue(ctx, dag)
Singleton->>DAG: fetch DAG config
DAG-->>Singleton: DAG details (MaxActiveRuns,...)
Singleton->>Runs: query active runs
Runs-->>Singleton: active count
Singleton->>Runs: query queued runs
Runs-->>Singleton: queued count
alt conflict found
Singleton-->>API: return 409 Conflict
API-->>Client: 409 Error (Error payload)
else no conflict
Singleton-->>API: ok
API->>Queue: enqueue DAG run
Queue-->>API: enqueue success
API-->>Client: 200/202 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Enqueue should consider overriden name (and parameters, if any) while taking decision rather than using actual dag filename |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/v2/api.gen.go(3 hunks)api/v2/api.yaml(2 hunks)internal/cmd/enqueue.go(0 hunks)internal/cmd/retry.go(1 hunks)internal/cmd/start.go(0 hunks)internal/service/frontend/api/v2/dagruns.go(0 hunks)internal/service/frontend/api/v2/dags.go(2 hunks)ui/src/api/v2/schema.ts(3 hunks)
💤 Files with no reviewable changes (3)
- internal/service/frontend/api/v2/dagruns.go
- internal/cmd/enqueue.go
- internal/cmd/start.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/service/frontend/api/v2/dags.gointernal/cmd/retry.goapi/v2/api.gen.go
{api/v1,api/v2}/**/*.{proto,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
API definitions live in
api/v1andapi/v2; generated server stubs land ininternal/service, while matching TypeScript clients flow intoui/src/api
Files:
api/v2/api.yaml
ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (ui/CLAUDE.md)
ui/**/*.{ts,tsx,jsx,js}: Use developer-centric UI design with high information density, minimal whitespace, compact components, and no unnecessary decorations
Support both light and dark modes for all UI components using Tailwind CSS class pairs likedark:bg-slate-700
NEVER use full-page loading overlays or LoadingIndicator components that hide content - show stale data while fetching updates instead
Use compact modal design with small headers, minimal padding (p-2 or p-3), tight spacing, and support keyboard navigation (arrows, enter, escape)
Use small heights for form elements: select boxes h-7 or smaller, buttons h-7 or h-8, inputs with compact padding (py-0.5 or py-1)
Minimize row heights in tables and lists while maintaining readability, merge related columns, and always handle long text withwhitespace-normal break-words
Use consistent metadata styling withbg-slate-200 dark:bg-slate-700backgrounds and maintain text hierarchy with primary/secondary/muted text colors
Use flexbox-first layouts withmin-h-0andoverflow-hiddento prevent layout breaks, account for fixed elements when setting heights
Maintain keyboard navigation support in all interactive components with appropriate focus indicators and ARIA labels
Files:
ui/src/api/v2/schema.ts
ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
ui/**/*.{ts,tsx}: The React + TypeScript frontend resides inui/, with production bundles copied tointernal/service/frontend/assetsbymake ui
UI code follows ESLint + Prettier (2-space indent) and Tailwind utilities; name React components in PascalCase (JobList.tsx) and hooks withuse*(useJobs.ts)
Files:
ui/src/api/v2/schema.ts
🧬 Code graph analysis (2)
internal/cmd/retry.go (1)
internal/core/execution/proc.go (1)
ProcStore(8-27)
api/v2/api.gen.go (1)
api/v1/api.gen.go (1)
Error(343-352)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (8)
api/v2/api.yaml (1)
800-803: Singleton enforcement implementation verified and correct.The
singletonfield implementation correctly prevents enqueuing when a DAG is already running or queued, returning a 409 conflict withErrorCodeMaxRunReached:
ensureSingletonEnqueue()(lines 677–692 in dags.go) checks both live (running) and queued instances before allowing enqueue- Error messages distinguish between "already running" and "already queued" cases
- Consistent error handling with the
/dags/{fileName}/startendpoint, which uses the sameisSingletonExecution()logic and error code- API spec description matches implementation behavior
api/v2/api.gen.go (3)
1474-1477:Singleton *boolis the right API shape for optional singleton-mode enqueue.
Using*bool+omitemptypreserves “unset” vs “false”, and the comment clarifies 409 behavior.
6009-6016: 409 response wiring matches existing generated patterns (Content-Type + status + JSON body).
This should integrate cleanly withEnqueueDAGDAGRunResponseObject/strict handler responses the same way other...409JSONResponsetypes do in this file.
8462-8653: swaggerSpec blob update is expected auto-generated output; do not hand-edit.
Verified that api/v2/api.yaml contains the newsingletonfield (lines 741, 759, 800, 818, 1238, 1264) and corresponding 409 conflict responses across multiple endpoints. The generated code correctly includesEnqueueDAGDAGRun409JSONResponsetype at line 6009. Ensure CI regenerates this deterministically from api/v2/api.yaml.internal/service/frontend/api/v2/dags.go (1)
831-835: LGTM! Clean integration of singleton mode.The singleton check is correctly placed before the enqueue operation, allowing early rejection when conflicts are detected. The implementation follows the existing patterns in this file for optional request body fields.
ui/src/api/v2/schema.ts (3)
2469-2473: Singleton field properly defined.The TypeScript schema correctly reflects the backend API: optional boolean field with clear documentation about 409 conflict behavior. The definition is consistent with the similar singleton field in the executeDAG operation.
2490-2498: 409 conflict response correctly added.The conflict response properly documents singleton mode rejection when a DAG is already running or queued, maintaining consistency with the executeDAG endpoint's singleton handling.
4007-4010: Note: Additional auth-related error codes included.The schema includes new auth-specific error codes (
auth.unauthorized,auth.token_invalid,auth.forbidden) that appear unrelated to the singleton feature. Since this file is auto-generated from the OpenAPI spec, these likely reflect other API surface changes in the same generation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/scheduler/queue_processor.go (1)
505-528: Consider caching the DAG read to avoid redundant I/O.This helper reads the DAG from the first queued item. Later,
processDAG(lines 405-412) reads the same DAG again when processing that item. For correctness this is fine, but it results in duplicateFindAttempt+ReadDAGcalls for the first item in each batch.A future optimization could cache the DAG reference or pass it through to avoid the redundant read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/integration/scheduler_test.go(1 hunks)internal/service/scheduler/queue_processor.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/integration/scheduler_test.gointernal/service/scheduler/queue_processor.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/integration/scheduler_test.go
🧬 Code graph analysis (2)
internal/integration/scheduler_test.go (5)
internal/test/helper.go (1)
WithConfigMutator(72-76)internal/core/execution/dagrun.go (3)
DAGRunStore(23-47)NewDAGRunAttemptOptions(106-111)DAGRunRef(144-147)internal/runtime/transform/status.go (4)
NewStatusBuilder(17-19)WithLogFilePath(119-123)WithAttemptID(43-47)WithHierarchyRefs(25-30)internal/core/status.go (1)
Queued(12-12)internal/core/execution/queue.go (1)
QueueStore(16-35)
internal/service/scheduler/queue_processor.go (3)
internal/common/logger/context.go (2)
Warn(45-47)Error(50-52)internal/common/logger/tag/tag.go (1)
Error(20-22)internal/core/execution/queue.go (1)
QueuedItemData(66-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test on ubuntu-latest
- GitHub Check: Build
🔇 Additional comments (3)
internal/service/scheduler/queue_processor.go (1)
281-291: LGTM - Correct placement for first-batch concurrency fix.The early update of
maxConcurrencybefore calculatingfreeslots ensures DAG-based queues process up tomaxActiveRunsitems in the first batch rather than defaulting to 1.internal/integration/scheduler_test.go (2)
284-291: Good test coverage for the first-batch concurrency fix.The test correctly validates that DAG-based queues initialize with the proper
maxActiveRunsvalue before processing the first batch, which is the core behavior being fixed.
360-361: Shallow copy is intentional here.The pattern
thCopy := thfollowed bythCopy.Context = ctxis consistent with other tests in this file. The shallow copy is appropriate since only theContextfield needs to differ for the goroutine's timeout, while other fields (stores, config) are intentionally shared.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1483 +/- ##
==========================================
- Coverage 59.92% 59.91% -0.01%
==========================================
Files 195 195
Lines 21913 21918 +5
==========================================
+ Hits 13131 13132 +1
- Misses 7373 7381 +8
+ Partials 1409 1405 -4
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
cc: @ghansham, @kriyanshii
Summary by CodeRabbit
New Features
API Changes
Behavioral Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.