core: add run-count history retention#2053
Merged
yottahmd merged 3 commits intodagucloud:mainfrom Apr 30, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces run-count-based DAG history retention as an alternative to day-based retention. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent<br/>(setupDAGRunAttempt)
participant Store as DAGRun Store
participant DataRoot as DataRoot
participant Persist as File System
Agent->>+Store: RemoveOldDAGRuns(ctx, name, opts...)
alt RetentionRuns configured
Store->>Store: Parse options.RetentionRuns
Store->>+DataRoot: RemoveOldByRuns(retentionRuns, dryRun)
DataRoot->>DataRoot: List all runs, sort by timestamp+baseDir
DataRoot->>DataRoot: Keep newest N runs
DataRoot->>DataRoot: Filter active runs (cannot remove)
loop For each removable run
DataRoot->>Persist: Delete run directory & metadata
end
DataRoot->>-Store: Return removed run IDs
else RetentionDays configured
Store->>+DataRoot: RemoveOld(retentionDays, dryRun)
DataRoot->>DataRoot: Filter runs by mod-time cutoff
DataRoot->>Persist: Delete old run records
DataRoot->>-Store: Return removed run IDs
end
Store->>-Agent: Cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 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 (2)
api/v1/api.yaml (1)
8608-8610: ⚡ Quick winClarify mutual exclusivity in the field description.
Please explicitly document that
histRetentionRunsis mutually exclusive withhistRetentionDaysin this schema description so generated clients/UI validations don’t misinterpret allowed combinations.♻️ Proposed OpenAPI description update
histRetentionRuns: type: integer - description: "Number of DAG runs to retain historical logs" + description: "Number of DAG runs to retain historical logs. Mutually exclusive with `histRetentionDays`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 8608 - 8610, Update the OpenAPI description for the histRetentionRuns field to state it is mutually exclusive with histRetentionDays: modify the description of histRetentionRuns to explicitly mention "Mutually exclusive with histRetentionDays" and, if applicable, add a similar note to the histRetentionDays description; ensure the text references both field names (histRetentionRuns, histRetentionDays) so generated clients and UI validators can detect the exclusivity constraint.internal/core/spec/dag_test.go (1)
661-670: ⚡ Quick winAdd a negative-value case to complete boundary validation.
The builder rejects
<= 0, but the table only checks0. Adding a negative case improves regression coverage for the same rule.Proposed test addition
tests := []struct { name string input *int expected int errContains string }{ {name: "NilDefaultsTo0", input: nil, expected: 0}, {name: "CustomValue", input: &runs, expected: 3}, {name: "ZeroValueInvalid", input: &zero, errContains: "hist_retention_runs must be > 0"}, + {name: "NegativeValueInvalid", input: new(-1), errContains: "hist_retention_runs must be > 0"}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/spec/dag_test.go` around lines 661 - 670, The test table in dag_test.go only checks zero as an invalid value but the builder rejects any value <= 0; add a negative-value case to the tests slice (e.g., add a test named "NegativeValueInvalid" with input pointing to a negative int variable like neg and errContains "hist_retention_runs must be > 0") and ensure you declare the neg variable (e.g., neg := -1) alongside runs/zero so the test verifies the <= 0 boundary is enforced.
🤖 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/core/spec/dag.go`:
- Around line 682-688: applyHistoryRetentionOverride currently treats zero as
"unset" because it checks built ints (current.HistRetentionDays > 0), so an
explicit hist_retention_days: 0 in the raw spec cannot override an inherited
HistRetentionRuns; change the logic to inspect the authored/raw pointer field on
the incoming DAG (e.g., check current.Raw.HistRetentionDays != nil and
current.Raw.HistRetentionRuns != nil or whatever *int raw fields exist) and use
those presence checks to clear the opposite mode on effective (still mutate
effective.HistRetentionDays and effective.HistRetentionRuns as now) so explicit
zeroes in current override inherited settings.
In `@internal/persis/filedagrun/dataroot.go`:
- Around line 411-430: LatestAttempt only returns attempts with a status file so
fresh attempts (created by CreateAttempt in internal/runtime/agent/agent.go) can
be invisible and cause an older finished attempt to mark the DAGRun removable;
change the removal logic in this block to first detect any newer status-less
attempts and bail if any exist: call the repository method that lists attempts
(or add a helper like HasAttemptWithoutStatus or ListAttempts) for the same run
ID and check for any attempt whose creation time is after the returned
latestAttempt or that has no status file, and if found return false,nil (or
alternatively explicitly exclude the current dagRunID from run-count cleanup
until its initial status is persisted). Ensure you reference LatestAttempt,
ModTime, ReadStatus, CreateAttempt and the run-count cleanup path when adding
the new check.
---
Nitpick comments:
In `@api/v1/api.yaml`:
- Around line 8608-8610: Update the OpenAPI description for the
histRetentionRuns field to state it is mutually exclusive with
histRetentionDays: modify the description of histRetentionRuns to explicitly
mention "Mutually exclusive with histRetentionDays" and, if applicable, add a
similar note to the histRetentionDays description; ensure the text references
both field names (histRetentionRuns, histRetentionDays) so generated clients and
UI validators can detect the exclusivity constraint.
In `@internal/core/spec/dag_test.go`:
- Around line 661-670: The test table in dag_test.go only checks zero as an
invalid value but the builder rejects any value <= 0; add a negative-value case
to the tests slice (e.g., add a test named "NegativeValueInvalid" with input
pointing to a negative int variable like neg and errContains
"hist_retention_runs must be > 0") and ensure you declare the neg variable
(e.g., neg := -1) alongside runs/zero so the test verifies the <= 0 boundary is
enforced.
🪄 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: 8074b540-4311-4ad9-9f37-3c67be6a7844
📒 Files selected for processing (23)
api/v1/api.gen.goapi/v1/api.yamlinternal/cmn/schema/dag.schema.jsoninternal/core/dag.gointernal/core/dag_test.gointernal/core/exec/dagrun.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/spec/key_hints.gointernal/core/spec/loader.gointernal/core/spec/loader_test.gointernal/persis/filebaseconfig/default_base_config.gointernal/persis/filedag/examples_unix.gointernal/persis/filedag/examples_windows.gointernal/persis/filedagrun/dataroot.gointernal/persis/filedagrun/dataroot_test.gointernal/persis/filedagrun/store.gointernal/runtime/agent/agent.gointernal/runtime/agent/agent_test.gointernal/service/frontend/api/v1/transformer.gointernal/service/frontend/api/v1/transformer_test.goskills/dagu/references/schema.mdui/src/api/v1/schema.ts
yottahmd
approved these changes
Apr 30, 2026
Collaborator
yottahmd
left a comment
yottahmd
left a comment
There was a problem hiding this comment.
LGTM, thank you very much for implementing this!
reachyogeshp-afk
force-pushed
the
hist-retention-runs
branch
from
5a57dff to
0e1a17e
Compare
Contributor
Author
Thanks for quick review. I have rebased with main . LMK if anything else is required at my end. |
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
2 participants
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
hist_retention_runsDAG config to retain history by most recent run count.hist_retention_daysandhist_retention_runsmutually exclusive.Fixes #1069
Testing
make lintmake api-validatepnpm --dir ui gen:apimake apigo test ./internal/core ./internal/core/spec ./internal/core/exec ./internal/persis/filedagrun -count=1go test ./internal/runtime/agent -run ^TestAgent_Run$/^DeleteOldHistoryByRuns -count=1go test ./api/v1 ./internal/service/frontend/api/v1 -run ^TestToDAGDetailsIncludesHistoryRetentionRuns -count=1go test ./internal/core/spec ./internal/persis/filedagrun -count=1Manual feature test:
Created
$DAGU_HOME/dags/retention-runs.yaml:Ran five DAG runs:
Verified only the latest three runs remain:
Verified mutually exclusive validation by configuring both fields:
Verified explicit
hist_retention_days: 0can override inherited run-count retention:Expected: validation succeeds.
Note: full
make testwas run but failed under full-suite load in integration/flaky areas; each failing test passed when rerun individually.