fix: preserve retry params for dotenv resolution#2267
Merged
Conversation
📝 WalkthroughWalkthroughThis PR fixes parameter loss during DAG step retry by preserving ChangesRuntime parameter preservation for retry operations
Sequence DiagramssequenceDiagram
participant PrevRun as Previous DAG Run
participant Status as status.ParamsList
participant Restore as restoreDAGFromStatus
participant Quote as QuoteRuntimeParams
participant Rebuild as rebuildDAGFromYAML
participant Dotenv as LoadDotEnv
participant Env as dag.Env
PrevRun->>Status: Capture ParamsList<br/>(e.g., COL=foo)
Status->>Restore: ParamsList passed to restore
Restore->>Restore: Copy ParamsList to dag.Params
Restore->>Dotenv: Load unparameterized .env files
Restore->>Quote: Quote(ParamsList, dag.ParamDefs)
Quote->>Restore: Quoted params
Restore->>Rebuild: Call with paramsOverride=quoted
Rebuild->>Rebuild: Use override for spec.WithParams
Rebuild->>Dotenv: Expand ".env.${COL}" → ".env.foo"<br/>Load .env.foo
Dotenv->>Env: TARGET_TABLE=foo added to env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/service/frontend/api/v1/dagruns.go`:
- Around line 569-581: rescheduleDAGRun is still using the lossy
exec.DAGRunStatus.Params when useCurrentDAGFile==true, which reintroduces
space-splitting bugs; update rescheduleDAGRun to prefer the preservedParams
returned by restoreDAGRunSnapshot (the quoted, safe form) instead of
status.Params when available (fall back to status.Params only if preservedParams
is empty), and ensure you reference restoreDAGRunSnapshot, rescheduleDAGRun,
preservedParams and exec.DAGRunStatus.Params when making the change.
In `@internal/service/scheduler/dag_executor.go`:
- Around line 240-245: The fix only handled the local retry path
(prepareDAGForSubprocess) but scheduled distributed retries still populate
task.Params from exec.DAGRunStatus.Params which flattens values with spaces;
update the code that prepares tasks for remote/scheduled retries to use
previousStatus.ParamsList (or otherwise preserve/round-trip quoting) instead of
previousStatus.Params so the remote worker receives the ParamsList
representation; look for code paths that set task.Params or read
exec.DAGRunStatus.Params during scheduled retry and switch them to use
ParamsList (or pass the original ParamsList through) to prevent unquoted
flattening.
🪄 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: 7bf40df0-5703-4b4d-8791-12695883dcb2
📒 Files selected for processing (13)
internal/cmd/export_test.gointernal/cmd/helper.gointernal/cmd/retry_params_external_test.gointernal/core/spec/dparams_runtime.gointernal/core/spec/runtime_env.gointernal/core/spec/runtime_env_external_test.gointernal/core/spec/runtime_params_external_test.gointernal/service/frontend/api/v1/dagruns.gointernal/service/scheduler/dag_executor.gointernal/service/worker/export_test.gointernal/service/worker/handler.gointernal/service/worker/remote_handler.gointernal/service/worker/remote_retry_params_external_test.go
Comment on lines
569
to
+581
| func restoreDAGRunSnapshot(ctx context.Context, dag *core.DAG, status *exec.DAGRunStatus) (*core.DAG, string, error) { | ||
| quotedParams := spec.QuoteRuntimeParams(status.ParamsList, dag.ParamDefs) | ||
| dag.Params = quotedParams | ||
| runtimeParams := append([]string(nil), status.ParamsList...) | ||
| dag.Params = runtimeParams | ||
| dagwarning.LoadDotEnv(ctx, dag) | ||
|
|
||
| restored, err := rebuildDAGRunSnapshotFromYAML(ctx, dag) | ||
| quotedParams := spec.QuoteRuntimeParams(runtimeParams, dag.ParamDefs) | ||
| restored, err := rebuildDAGRunSnapshotFromYAML(ctx, dag, quotedParams) | ||
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
|
|
||
| return restored, strings.Join(quotedParams, " "), nil | ||
| preservedParams := strings.Join(quotedParams, " ") | ||
| return restored, preservedParams, nil |
There was a problem hiding this comment.
The preserved retry params are still bypassed on current-file reschedules.
This helper now returns the correctly quoted preservedParams, but rescheduleDAGRun still prefers status.Params whenever useCurrentDAGFile=true. That reintroduces the space-splitting bug on that path, because status.Params is the lossy joined form.
🛠️ Suggested change
- currentFileParams := status.Params
- if currentFileParams == "" {
- currentFileParams = preservedSnapshotParams
- }
+ currentFileParams := preservedSnapshotParams
+ if currentFileParams == "" {
+ currentFileParams = status.Params
+ }Based on learnings: exec.DAGRunStatus.Params stores unquoted inline values, so it does not safely round-trip values containing spaces.
🤖 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/frontend/api/v1/dagruns.go` around lines 569 - 581,
rescheduleDAGRun is still using the lossy exec.DAGRunStatus.Params when
useCurrentDAGFile==true, which reintroduces space-splitting bugs; update
rescheduleDAGRun to prefer the preservedParams returned by restoreDAGRunSnapshot
(the quoted, safe form) instead of status.Params when available (fall back to
status.Params only if preservedParams is empty), and ensure you reference
restoreDAGRunSnapshot, rescheduleDAGRun, preservedParams and
exec.DAGRunStatus.Params when making the change.
Comment on lines
240
to
245
| // Local execution | ||
| var params any | ||
| if previousStatus != nil { | ||
| params = spec.QuoteRuntimeParams(previousStatus.ParamsList, dag.ParamDefs) | ||
| params = previousStatus.ParamsList | ||
| } | ||
| dag, err := e.prepareDAGForSubprocess(ctx, dag, params) |
There was a problem hiding this comment.
This only fixes the local retry path.
Scheduled distributed retries still populate the task with previousStatus.Params in the branch above, and the remote worker prefers task.Params over previous_status. So a value like COL=foo bar is still flattened before it reaches the worker, even though the local branch now uses ParamsList.
🛠️ Suggested change
- if previousStatus != nil && previousStatus.Params != "" {
+ if previousStatus != nil && len(previousStatus.ParamsList) == 0 && previousStatus.Params != "" {
taskOpts = append(taskOpts, executor.WithTaskParams(previousStatus.Params))
}Based on learnings: exec.DAGRunStatus.Params stores unquoted inline values, so it does not safely round-trip values containing spaces.
🤖 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/scheduler/dag_executor.go` around lines 240 - 245, The fix
only handled the local retry path (prepareDAGForSubprocess) but scheduled
distributed retries still populate task.Params from exec.DAGRunStatus.Params
which flattens values with spaces; update the code that prepares tasks for
remote/scheduled retries to use previousStatus.ParamsList (or otherwise
preserve/round-trip quoting) instead of previousStatus.Params so the remote
worker receives the ParamsList representation; look for code paths that set
task.Params or read exec.DAGRunStatus.Params during scheduled retry and switch
them to use ParamsList (or pass the original ParamsList through) to prevent
unquoted flattening.
There was a problem hiding this comment.
1 issue found across 13 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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
Preserves raw retry
ParamsListthrough retry restoration so dotenv paths such as.env.${COL}resolve with logical values and parameter values containing spaces survive retries.Changes
ParamsListfor dotenv loading and quote runtime params only at parser/reload boundaries.Related Issues
Fixes #2265
Checklist
Root Cause
Retry restored
status.ParamsListthrough quoted parser tokens before dotenv loading. That changedCOL=foointoCOL="foo"for dotenv path expansion, so.env.${COL}became.env."foo"and missed.env.foo. Some retry paths also relied on task params only and did not rehydrate params from embedded previous status.Testing
go test ./internal/cmd ./internal/core/spec ./internal/service/frontend/api/v1 ./internal/service/scheduler ./internal/service/worker -count=1Summary by cubic
Preserves raw retry params during dotenv resolution so .env.${COL} resolves correctly and values with spaces aren’t split. Extends this to reschedule and distributed retry paths to keep ParamsList intact across CLI, API/subprocess, scheduler, and remote worker. Fixes #2265.
.envexpansion is accurate.Written for commit 61ceaf6. Summary will update on new commits.
Summary by CodeRabbit
Release Notes