fix: resolve scheduler no-eval env before subprocess launch by yohamta0 · Pull Request #2269 · dagucloud/dagu

yohamta0 · 2026-06-06T08:12:25Z

Summary

add in-memory env provenance so no-eval scheduler metadata is not treated as resolved
stop persisting raw no-eval env as presolved build env
reload source-backed raw env before subprocess launch while preserving evaluated/source and programmatic no-source behavior

Root cause

Scheduler metadata loads use OnlyMetadata with WithoutEval, leaving env values such as ${MY_SECRET} raw in dag.Env. The subprocess preparation path treated any non-empty dag.Env as already resolved and serialized it as presolved build env, causing child DAG loads to reuse the raw literal instead of expanding the OS environment.

Testing

go test ./internal/core/... ./internal/service/scheduler ./internal/cmd ./internal/launcher ./internal/service/worker -count=1

Summary by cubic

Fix scheduler to resolve no-eval env vars before launching subprocesses and handle source-file-only metadata correctly. Prevents ${VAR} literals from leaking into child DAG loads and recomputes env from source when needed.

Bug Fixes
- Track env provenance via DAG.EnvEvaluated to know when Env is safe to reuse.
- Skip capturing PresolvedBuildEnv when building with WithoutEval.
- Update ResolveEnvWithWarnings to reload from SourceFile, recompute when params change or raw source-backed metadata exists, and reuse only when EnvEvaluated or no source is present.
- Extend runtime param resolution to support SourceFile.
- Add tests for source-file-only reload, evaluated-source reuse, programmatic env without source, and no-eval metadata.

^{Written for commit dd880c7. Summary will update on new commits.}

Summary by CodeRabbit

Release Notes

Performance Improvements
- Optimized environment variable caching to reduce unnecessary recomputation during builds by properly tracking when resolved environments can be safely reused.
Tests
- Added comprehensive test coverage for environment variable resolution and caching behavior across various scenarios.

coderabbitai · 2026-06-06T08:12:41Z

Need the big picture first? Review this PR in Change Stack to see what changed before going file by file.

📝 Walkthrough

Walkthrough

The PR introduces a runtime flag DAG.EnvEvaluated to track whether environment variables were safely evaluated at build time, and refactors the runtime env resolution logic to respect this flag when deciding whether to reuse presolved environment or recompute it.

Changes

DAG Environment Evaluation and Reuse Logic

Layer / File(s)	Summary
DAG structure and build-time env evaluation marking `internal/core/dag.go`, `internal/core/spec/dag.go`	The `DAG` struct gains an `EnvEvaluated` boolean field (JSON-omitted). During DAG building, `markEnvEvaluated()` sets this flag to `true` unless `BuildFlagNoEval` is set. `capturePresolvedBuildEnv()` skips env capture when `BuildFlagNoEval` is enabled.
Env resolution decision logic using evaluated flag `internal/core/spec/runtime_env.go`	`ResolveEnvWithWarnings` refactored to use two new predicates: `canReuseCurrentEnv` (reuses when no runtime params, `dag.Env` exists, and either `dag.EnvEvaluated` is true or DAG has no source) and `shouldRecomputeEnv` (recomputes when runtime params exist or when source exists but `dag.EnvEvaluated` is false). Three helper functions (`canReuseCurrentEnv`, `shouldRecomputeEnv`, `hasDAGSource`) encapsulate the logic.
Test cases for env resolution behavior `internal/core/spec/runtime_env_external_test.go`	Four new integration tests cover: reloading unevaluated metadata env from source, ensuring `WithoutEval` load does not populate `PresolvedBuildEnv`, verifying reuse of already-evaluated source env after env variable changes, and confirming programmatic env without source is preserved independently.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

dagucloud/dagu#2268: Both changes target the same code path in runtime_env.go — this PR implements the fix by adding DAG.EnvEvaluated flag and predicates (canReuseCurrentEnv / shouldRecomputeEnv) to avoid returning raw, unevaluated dag.Env for DAGs with source.

Possibly related PRs

dagucloud/dagu#2267: Both PRs modify internal/core/spec/runtime_env.go's ResolveEnvWithWarnings decision logic around reusing/recomputing DAG env based on runtime parameters and how env values are prepared and resolved.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: fixing scheduler environment variable resolution for no-eval metadata before subprocess launch.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description covers all required sections: summary, detailed changes, root cause analysis, and testing approach.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix-scheduler-noeval-env

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

internal/core/spec/runtime_env_external_test.go (1)

161-239: ⚡ Quick win

Add a SourceFile-only regression case for env reload.

These tests cover metadata/evaluated/programmatic paths well, but they don’t assert behavior when Location is empty and only SourceFile is set (the provenance path used when runtime location is rewritten).

🤖 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/core/spec/runtime_env_external_test.go` around lines 161 - 239, Add
a regression test that mirrors
TestResolveEnvWithWarningsReloadsNoEvalMetadataEnvFromSource but simulates the
rewritten-runtime provenance (i.e., clear metadata.Location and keep
metadata.SourceFile set) so ResolveEnvWithWarnings still reloads env from the
source file; specifically: create the YAML file and load it with spec.Load (or
spec.LoadYAML) into metadata, set metadata.Location = "" while leaving
metadata.SourceFile populated, call spec.ResolveEnvWithWarnings(ctx, metadata,
nil, spec.ResolveEnvOptions{}), and assert that
runtimeEnvSliceMap(result.Env)["TOKEN"] == "secret123" and
runtimeEnvSliceMap(metadata.Env)["TOKEN"] == "${ISSUE_2268_TOKEN}" to ensure
reload happened from SourceFile when Location is empty.

🤖 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/core/spec/runtime_env.go`:
- Around line 125-135: The DAG is considered source-backed only via YamlData or
Location but misses core.DAG.SourceFile, causing unnecessary env recomputation;
update hasDAGSource(dag *core.DAG) to also check dag.SourceFile (e.g., return
len(dag.YamlData) > 0 || dag.Location != "" || dag.SourceFile != ""), and ensure
canReuseCurrentEnv and shouldRecomputeEnv naturally pick up that change so raw
no-eval dag.Env can be reused when SourceFile is present and Location was
cleared.

---

Nitpick comments:
In `@internal/core/spec/runtime_env_external_test.go`:
- Around line 161-239: Add a regression test that mirrors
TestResolveEnvWithWarningsReloadsNoEvalMetadataEnvFromSource but simulates the
rewritten-runtime provenance (i.e., clear metadata.Location and keep
metadata.SourceFile set) so ResolveEnvWithWarnings still reloads env from the
source file; specifically: create the YAML file and load it with spec.Load (or
spec.LoadYAML) into metadata, set metadata.Location = "" while leaving
metadata.SourceFile populated, call spec.ResolveEnvWithWarnings(ctx, metadata,
nil, spec.ResolveEnvOptions{}), and assert that
runtimeEnvSliceMap(result.Env)["TOKEN"] == "secret123" and
runtimeEnvSliceMap(metadata.Env)["TOKEN"] == "${ISSUE_2268_TOKEN}" to ensure
reload happened from SourceFile when Location is empty.

🪄 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: 72c52093-d2db-420e-bd92-c06c0ee2dc54

📥 Commits

Reviewing files that changed from the base of the PR and between d7fe20f and 4679b42.

📒 Files selected for processing (4)

internal/core/dag.go
internal/core/spec/dag.go
internal/core/spec/runtime_env.go
internal/core/spec/runtime_env_external_test.go

cubic-dev-ai

1 issue found across 4 files

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

fix: resolve scheduler no-eval env before subprocess launch

4679b42

coderabbitai Bot reviewed Jun 6, 2026

View reviewed changes

Comment thread internal/core/spec/runtime_env.go

cubic-dev-ai Bot reviewed Jun 6, 2026

View reviewed changes

Comment thread internal/core/spec/runtime_env.go

fix: handle source-file-only env resolution

dd880c7

yohamta0 merged commit 50d9551 into main Jun 6, 2026
11 checks passed

yohamta0 deleted the fix-scheduler-noeval-env branch June 6, 2026 08:50

coderabbitai Bot mentioned this pull request Jun 10, 2026

fix: bound scheduler latest status lookup #2278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: resolve scheduler no-eval env before subprocess launch#2269

fix: resolve scheduler no-eval env before subprocess launch#2269
yohamta0 merged 2 commits into
mainfrom
fix-scheduler-noeval-env

yohamta0 commented Jun 6, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 6, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

yohamta0 commented Jun 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Testing

Summary by cubic

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Jun 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

yohamta0 commented Jun 6, 2026 •

edited

Loading

coderabbitai Bot commented Jun 6, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading