feat: propagate schedule time through DAG run lifecycle#1763
Merged
Conversation
Thread the scheduled time from the tick planner dispatch through the CLI flags, agent, runtime status, persistence layer, and REST API so that each DAG run records when it was originally scheduled to execute.
- Extract parseScheduleTimeParam() in helper.go, used by start and enqueue - Use stringutil.FormatTime() in dag_executor instead of manual formatting - Fix agent Status() to avoid overwriting ScheduleTime with empty retryTarget value
|
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:
📝 WalkthroughWalkthroughThe PR adds schedule time tracking throughout the system by introducing a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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)
📝 Coding Plan
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/scheduler/tick_planner.go (1)
775-781:⚠️ Potential issue | 🟠 Major
restart_schedulestill drops the scheduled fire time.
run.ScheduledTimeis only forwarded in theScheduleTypeStartbranch.ScheduleTypeRestartstill callsRestart(ctx, dag)with no way to carry that timestamp, andDAGExecutor.Restart()currently starts a fresh run without settingScheduleTime. Any DAG run created byrestart_schedulewill therefore still persist an empty schedule time. Please thread the value throughRestartFunc/RestartOptionstoo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/scheduler/tick_planner.go` around lines 775 - 781, The Restart path is dropping run.ScheduledTime: when ScheduleTypeRestart you call tp.cfg.Restart(ctx, run.DAG) but do not pass run.ScheduledTime through, so DAGExecutor.Restart() creates a fresh run with no ScheduleTime; update the Restart API to accept RestartOptions (or a RestartFunc signature) that includes ScheduledTime, thread run.ScheduledTime from the ScheduleTypeRestart branch into tp.cfg.Restart as an option, and ensure the implementation in DAGExecutor.Restart (and any RestartFunc) applies that ScheduledTime to the newly created DAG run before persisting it.internal/cmd/start.go (1)
223-229:⚠️ Potential issue | 🟠 MajorCoordinator handoff ignores the new
scheduleTimeinput.
tryExecuteDAG()now acceptsscheduleTime, but theworkerID == "local"coordinator branch still callsdispatchToCoordinatorAndWait()without it. In distributed mode, anystartcaller that sets--schedule-timewill silently lose the value before the worker creates the run, so the recorded schedule time now depends on execution mode. Please propagate it through the coordinator task/start path as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/start.go` around lines 223 - 229, The coordinator dispatch branch in tryExecuteDAG ignores the new scheduleTime param; update the call to dispatchToCoordinatorAndWait to pass scheduleTime through (i.e., change dispatchToCoordinatorAndWait(ctx, dag, dagRunID, coordinatorCli) to include scheduleTime) and update dispatchToCoordinatorAndWait’s signature and any callers (and the coordinator task/start path) to accept and forward scheduleTime so the worker-created run preserves the provided schedule time; locate tryExecuteDAG, dispatchToCoordinatorAndWait, NewCoordinatorClient and the coordinator task/start handler to propagate the new parameter end-to-end.
🧹 Nitpick comments (2)
internal/service/scheduler/tick_planner_test.go (1)
1257-1273: Tighten this dispatch test to assertScheduledTimeis forwarded.Lines 1259-1261 ignore the new callback argument, and Line 1267 builds a
PlannedRunwithoutScheduledTime. As written, this still passes ifDispatchRun()drops or zeroes the schedule timestamp.Suggested test tightening
func TestTickPlanner_DispatchRunStart(t *testing.T) { t.Parallel() - var dispatched bool + var ( + dispatched bool + gotScheduleTime time.Time + ) tp := NewTickPlanner(TickPlannerConfig{ - Dispatch: func(_ context.Context, _ *core.DAG, _ string, _ core.TriggerType, _ time.Time) error { + Dispatch: func(_ context.Context, _ *core.DAG, _ string, _ core.TriggerType, scheduleTime time.Time) error { dispatched = true + gotScheduleTime = scheduleTime return nil }, Events: make(chan DAGChangeEvent, 1), }) require.NoError(t, tp.Init(context.Background(), nil)) + scheduledTime := time.Date(2026, 2, 7, 12, 0, 0, 0, time.UTC) tp.DispatchRun(context.Background(), PlannedRun{ - DAG: &core.DAG{Name: "start-dag"}, - RunID: "run-1", - ScheduleType: ScheduleTypeStart, - TriggerType: core.TriggerTypeScheduler, + DAG: &core.DAG{Name: "start-dag"}, + RunID: "run-1", + ScheduledTime: scheduledTime, + ScheduleType: ScheduleTypeStart, + TriggerType: core.TriggerTypeScheduler, }) assert.True(t, dispatched, "Dispatch callback should be invoked for ScheduleTypeStart") + assert.Equal(t, scheduledTime, gotScheduleTime, "Dispatch callback should receive the scheduled time") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/scheduler/tick_planner_test.go` around lines 1257 - 1273, The test currently ignores the Dispatch callback's ScheduledTime parameter and builds a PlannedRun without a ScheduledTime, so it won't detect if DispatchRun drops or zeroes the timestamp; update the test for NewTickPlanner/TickPlannerConfig to capture the ScheduledTime passed into the Dispatch callback (the Dispatch func in TickPlannerConfig) and assert it equals the ScheduledTime set on the PlannedRun you pass to tp.DispatchRun(...); specifically, set a non-zero ScheduledTime on the PlannedRun you create and have the Dispatch stub store the received scheduledTime argument so the test asserts that stored value matches the PlannedRun.ScheduledTime after calling tp.DispatchRun.ui/src/features/dag-runs/components/dag-run-list/DAGRunTable.tsx (1)
287-293: Add explicit wrapping classes for new schedule-time fields.The newly added schedule text in card/table views should include overflow-safe wrapping classes.
Suggested patch
- <div className="flex justify-between items-center"> - <div> + <div className="flex justify-between items-center"> + <div className="whitespace-normal break-words"> <span className="text-muted-foreground">Scheduled: </span> {dagRun.scheduleTime} </div> </div> @@ - <TableCell className="py-1 px-2 text-left"> + <TableCell className="py-1 px-2 text-left whitespace-normal break-words"> {dagRun.scheduleTime || '-'} </TableCell>As per coding guidelines "Always handle long text in tables and lists with
whitespace-normal break-wordsto prevent layout overflow".Also applies to: 419-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dag-runs/components/dag-run-list/DAGRunTable.tsx` around lines 287 - 293, The scheduleTime text in DAGRunTable (the JSX rendering dagRun.scheduleTime) lacks overflow-safe classes; update the element that prints dagRun.scheduleTime (and the other similar schedule display occurrences) to include explicit wrapping classes such as "whitespace-normal break-words" (e.g., add these classes to the containing div or span that renders dagRun.scheduleTime) so long schedule strings won't overflow table/card layouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runtime/agent/agent.go`:
- Around line 1073-1085: The early-return branch in Status() (when a.runner ==
nil) omits adding transform.WithScheduleTime, so fallback status can lose
schedule metadata; update that branch to mirror the main-path logic: if
a.retryTarget != nil include transform.WithQueuedAt(a.retryTarget.QueuedAt),
transform.WithCreatedAt(a.retryTarget.CreatedAt) and, if
a.retryTarget.ScheduleTime != "" also add
transform.WithScheduleTime(a.retryTarget.ScheduleTime), otherwise if
a.scheduleTime != "" add transform.WithScheduleTime(a.scheduleTime); keep using
the same transform options assembly used in the non-nil runner path.
In
`@ui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunGroupedView.test.tsx`:
- Around line 1-6: This test file is missing the GPL v3 license header; add the
required header to the top of the file (the one containing imports and
references to DAGRunGroupedView, Status, StatusLabel, TriggerType) by running
the repository tool that manages headers (run "make addlicense") or by inserting
the standard GPL v3 header used across the project so the file matches the
project's license header conventions.
In
`@ui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunTable.test.tsx`:
- Around line 1-8: This test file is missing the required GPL v3 license header;
add the repository's standard GPL v3 header block at the very top of the
DAGRunTable.test.tsx file (before any imports) following the same format used
across other .ts/.tsx files, or run the repository helper (make addlicense) to
apply it automatically, ensuring the header remains intact above the existing
imports and tests referencing DAGRunTable and imports like Status/ConfigContext.
In
`@ui/src/features/dags/components/dag-details/__tests__/DAGStatusOverview.test.tsx`:
- Around line 1-6: Add the GPL v3 license header to this source file by
inserting the standard project header at the very top of the file (above all
imports); you can run the repo tooling to do this automatically (run make
addlicense) or manually paste the project's GPL v3 header used across other
files, ensuring it appears before the imports and applies to this test file that
contains imports like DAGStatusOverview, Status, StatusLabel, dayjs and testing
utilities.
In `@ui/src/lib/__tests__/dagRunTiming.test.ts`:
- Line 1: This test file is missing the repository-required GPL v3 license
header; add the standard GPL v3 header to the top of
ui/src/lib/__tests__/dagRunTiming.test.ts (the file that currently only imports
"describe, expect, it")—best fix is to run the repo tool to apply headers (run
"make addlicense") or manually prepend the canonical GPL v3 header used across
the repo to the top of the file so the test source includes the required license
block.
In `@ui/src/lib/dagRunTiming.ts`:
- Line 1: The new TypeScript file ui/src/lib/dagRunTiming.ts is missing the
required GPL v3 license header; add the project-standard GPL v3 header at the
very top of the file (above the existing import dayjs from './dayjs') following
the repository's header format, then run the repository tool to ensure
consistency (e.g., make addlicense) or paste the exact header used across other
.ts/.tsx/.js files.
---
Outside diff comments:
In `@internal/cmd/start.go`:
- Around line 223-229: The coordinator dispatch branch in tryExecuteDAG ignores
the new scheduleTime param; update the call to dispatchToCoordinatorAndWait to
pass scheduleTime through (i.e., change dispatchToCoordinatorAndWait(ctx, dag,
dagRunID, coordinatorCli) to include scheduleTime) and update
dispatchToCoordinatorAndWait’s signature and any callers (and the coordinator
task/start path) to accept and forward scheduleTime so the worker-created run
preserves the provided schedule time; locate tryExecuteDAG,
dispatchToCoordinatorAndWait, NewCoordinatorClient and the coordinator
task/start handler to propagate the new parameter end-to-end.
In `@internal/service/scheduler/tick_planner.go`:
- Around line 775-781: The Restart path is dropping run.ScheduledTime: when
ScheduleTypeRestart you call tp.cfg.Restart(ctx, run.DAG) but do not pass
run.ScheduledTime through, so DAGExecutor.Restart() creates a fresh run with no
ScheduleTime; update the Restart API to accept RestartOptions (or a RestartFunc
signature) that includes ScheduledTime, thread run.ScheduledTime from the
ScheduleTypeRestart branch into tp.cfg.Restart as an option, and ensure the
implementation in DAGExecutor.Restart (and any RestartFunc) applies that
ScheduledTime to the newly created DAG run before persisting it.
---
Nitpick comments:
In `@internal/service/scheduler/tick_planner_test.go`:
- Around line 1257-1273: The test currently ignores the Dispatch callback's
ScheduledTime parameter and builds a PlannedRun without a ScheduledTime, so it
won't detect if DispatchRun drops or zeroes the timestamp; update the test for
NewTickPlanner/TickPlannerConfig to capture the ScheduledTime passed into the
Dispatch callback (the Dispatch func in TickPlannerConfig) and assert it equals
the ScheduledTime set on the PlannedRun you pass to tp.DispatchRun(...);
specifically, set a non-zero ScheduledTime on the PlannedRun you create and have
the Dispatch stub store the received scheduledTime argument so the test asserts
that stored value matches the PlannedRun.ScheduledTime after calling
tp.DispatchRun.
In `@ui/src/features/dag-runs/components/dag-run-list/DAGRunTable.tsx`:
- Around line 287-293: The scheduleTime text in DAGRunTable (the JSX rendering
dagRun.scheduleTime) lacks overflow-safe classes; update the element that prints
dagRun.scheduleTime (and the other similar schedule display occurrences) to
include explicit wrapping classes such as "whitespace-normal break-words" (e.g.,
add these classes to the containing div or span that renders
dagRun.scheduleTime) so long schedule strings won't overflow table/card layouts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9870ce2b-8e63-4bbb-8fdf-7b3972482b5a
⛔ Files ignored due to path filters (3)
proto/coordinator/v1/coordinator.pb.gois excluded by!**/*.pb.goproto/coordinator/v1/coordinator_grpc.pb.gois excluded by!**/*.pb.goproto/index/v1/index.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (37)
api/v1/api.gen.goapi/v1/api.yamlinternal/cmd/enqueue.gointernal/cmd/exec.gointernal/cmd/helper.gointernal/cmd/start.gointernal/core/exec/runstatus.gointernal/persis/filedagrun/dagrun.gointernal/persis/filedagrun/dagrunindex/dagrunindex.gointernal/persis/filedagrun/dataroot.gointernal/persis/filedagrun/dataroot_test.gointernal/persis/filedagrun/store.gointernal/persis/filedagrun/store_test.gointernal/runtime/agent/agent.gointernal/runtime/subcmd.gointernal/runtime/transform/status.gointernal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/transformer_test.gointernal/service/scheduler/dag_executor.gointernal/service/scheduler/dag_executor_test.gointernal/service/scheduler/queue_processor.gointernal/service/scheduler/scheduler.gointernal/service/scheduler/scheduler_test.gointernal/service/scheduler/tick_planner.gointernal/service/scheduler/tick_planner_test.goproto/index/v1/index.protoui/src/api/v1/schema.tsui/src/features/cockpit/components/KanbanCard.tsxui/src/features/dag-runs/components/dag-run-list/DAGRunGroupedView.tsxui/src/features/dag-runs/components/dag-run-list/DAGRunTable.tsxui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunGroupedView.test.tsxui/src/features/dag-runs/components/dag-run-list/__tests__/DAGRunTable.test.tsxui/src/features/dags/components/dag-details/DAGStatusOverview.tsxui/src/features/dags/components/dag-details/__tests__/DAGStatusOverview.test.tsxui/src/features/queues/components/QueueCard.tsxui/src/lib/__tests__/dagRunTiming.test.tsui/src/lib/dagRunTiming.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1763 +/- ##
==========================================
- Coverage 69.32% 68.39% -0.94%
==========================================
Files 402 402
Lines 45352 45467 +115
==========================================
- Hits 31441 31095 -346
- Misses 11279 11325 +46
- Partials 2632 3047 +415
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This was referenced Mar 14, 2026
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
Testing
Summary by CodeRabbit
--schedule-timeflag in the start and enqueue commands. Scheduled times are displayed in the UI alongside queued, started, and finished times for better visibility into DAG run lifecycle.