fix: preserve env-backed secrets for scheduler subprocess runs by yottahmd · Pull Request #1877 · dagucloud/dagu

yottahmd · 2026-03-29T09:47:19Z

Summary

pass the full parent environment to start/retry/restart subprocess launches while keeping workflow steps isolated to filtered BaseEnv
rehydrate a full execution DAG before persisting scheduler catchup runs so queued retries keep Secrets and other execution fields
add runtime, scheduler, command, and integration regressions for env-backed secrets on scheduler-owned execution paths

Root Cause

parent-spawned dagu start/retry/restart subprocesses were launched from a filtered environment, so provider: env secrets could disappear before the child runtime resolved them
scheduler metadata and catchup paths could use incomplete execution snapshots that dropped full DAG fields needed for later retries

Testing

go test ./internal/runtime ./internal/service/scheduler ./internal/core/exec ./internal/cmd
go test ./internal/intg -run TestOneOffScheduleResolvesEnvSecretsWithoutLeakingSourceEnv -count=1

Closes #1864

Summary by CodeRabbit

Release Notes

New Features
- Environment-backed secrets now resolve securely without exposing source environment variables to subprocess environments.
Bug Fixes
- Fixed environment variable leakage in DAG execution where source variables were inadvertently propagated when env-backed secrets were used.
Refactor
- Updated catch-up run enqueueing to support full DAG rehydration with base configuration data.

coderabbitai · 2026-03-29T09:47:36Z

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1ffde52a-e627-4f4e-a507-f98f0a4b6373

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR modifies environment secret resolution for scheduled DAGs by introducing full DAG rehydration during catch-up enqueueing, removing environment variable pre-resolution, and filtering base environment to prevent variable leakage across scheduled executions.

Changes

Cohort / File(s)	Summary
Test Assertion Updates `internal/cmd/restart_test.go`, `internal/cmd/start_test.go`	Changed test assertions to expect exact `RESULT` values (`from-host\|`) instead of substring matching; removed `strings` import where applicable.
Scheduled Catchup Test Coverage `internal/cmd/retry_test.go`	Added new test case for "queued catchup" behavior verifying env-backed secret resolution when DAG metadata lacks persisted secrets.
Secret Configuration Documentation `internal/cmn/secrets/env.go`	Updated comments for `PresolvedEnvPrefix` to describe subprocess resolution behavior without source variable whitelist requirements.
Environment Context Construction `internal/core/exec/context.go`, `internal/core/exec/context_test.go`	Changed `EnvScope` initialization to use filtered `BaseEnv` from config instead of raw OS environment; added test verifying filtered BaseEnv usage and exclusion of host-only variables.
Runtime Environment Resolution `internal/runtime/env.go`, `internal/runtime/presolved_secrets_test.go`	Updated comments describing EnvScope inheritance chain; removed unit test coverage for `preResolveEnvSecrets` function.
Subprocess Command Building `internal/runtime/subcmd.go`, `internal/runtime/subcmd_test.go`	Removed `preResolveEnvSecrets` helper; replaced with `parentEnv()` helper; removed `secretHints` parameter from `TaskStart` and `TaskRetry` signatures; updated tests to verify parent env inheritance and BaseEnv filtering without presolved secret transport.
Scheduler DAG Rehydration `internal/service/scheduler/execution_dag.go`, `internal/service/scheduler/enqueue.go`, `internal/service/scheduler/enqueue_test.go`, `internal/service/scheduler/scheduler.go`	Introduced `rehydrateExecutionDAG` to resolve fresh DAG snapshots; updated `EnqueueCatchupRun` signature to accept `baseConfig` parameter; added test verifying full DAG rehydration from metadata-only DAG before persistence.
Worker Integration `internal/service/worker/handler.go`	Updated `TaskStart` and `TaskRetry` calls to remove secret hints; removed `secrets` field from `subprocessHintSet` struct.
Fixture Updates `internal/intg/distr/fixtures_test.go`, `internal/intg/queue/fixture_test.go`	Updated `enqueueCatchup` invocations to pass `baseConfig` parameter to `scheduler.EnqueueCatchupRun`.
Integration Tests `internal/intg/one_off_schedule_test.go`	Added comprehensive integration test for env secret resolution in scheduled DAGs, verifying secret values are resolved at runtime while source environment variables are not leaked into subprocess execution.

Sequence Diagram

sequenceDiagram
    actor Scheduler
    participant EnqueueService as Enqueue Service
    participant DAGRehydrate as DAG Rehydrate
    participant DAGStore as DAG Store
    participant SubCmd as SubCmd Builder
    participant Subprocess as Subprocess Execution

    Scheduler->>EnqueueService: EnqueueCatchupRun(dag, baseConfig, ...)
    EnqueueService->>DAGRehydrate: rehydrateExecutionDAG(dag, baseConfig)
    DAGRehydrate->>DAGRehydrate: Resolve full DAG from source
    DAGRehydrate-->>EnqueueService: fullDAG with Secrets populated
    EnqueueService->>DAGStore: Persist fullDAG.Clone()
    DAGStore-->>EnqueueService: Confirmed
    EnqueueService-->>Scheduler: Catchup queued
    
    Scheduler->>SubCmd: TaskStart/TaskRetry(task, envHints, ...)
    Note over SubCmd: No secretHints parameter
    SubCmd->>SubCmd: Build parentEnv from os.Environ() + envHints
    Note over SubCmd: No presolved secret transport
    SubCmd->>Subprocess: Execute with parentEnv + BaseEnv
    Subprocess->>Subprocess: Resolve env-backed secrets at runtime
    Subprocess-->>SubCmd: Output with secret values

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

#1853: This PR directly reverses the presolved-secret transport mechanism introduced in #1853 by removing preResolveEnvSecrets, updating subcmd/task/worker code, and restoring runtime secret resolution.
#1772: Both PRs modify the scheduler's catch-up dispatch path to route catchup runs through EnqueueCatchupRun with queueing and planner integration.
#1735: Both PRs modify environment construction in NewContext (context.go), with this PR changing EnvScope/base-env layering and the other handling workDir propagation.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.86% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: preserve env-backed secrets for scheduler subprocess runs' accurately describes the main objective of the PR: ensuring env-backed secrets work correctly in scheduler subprocess executions.
Linked Issues check	✅ Passed	The PR comprehensively addresses issue `#1864` by implementing env-backed secret resolution for scheduler-launched DAG runs through multiple code changes ensuring secrets are preserved across scheduler subprocess executions.
Out of Scope Changes check	✅ Passed	All changes directly support the stated objectives: passing parent environment to subprocesses, rehydrating full execution DAGs for catchup runs, and adding regression tests covering the fixed functionality.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/1864-secret-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.}

codecov · 2026-03-29T11:15:46Z

Codecov Report

❌ Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.11%. Comparing base (6c54515) to head (518782b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/service/scheduler/enqueue.go	33.33%	2 Missing and 2 partials ⚠️
internal/service/scheduler/execution_dag.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1877      +/-   ##
==========================================
+ Coverage   69.02%   69.11%   +0.08%     
==========================================
  Files         446      447       +1     
  Lines       55352    55361       +9     
==========================================
+ Hits        38208    38261      +53     
+ Misses      13739    13698      -41     
+ Partials     3405     3402       -3

Files with missing lines	Coverage Δ
internal/cmn/secrets/env.go	`90.00% <ø> (ø)`
internal/core/exec/context.go	`91.04% <100.00%> (+0.13%)`	⬆️
internal/runtime/env.go	`64.73% <ø> (ø)`
internal/runtime/subcmd.go	`90.98% <100.00%> (-0.14%)`	⬇️
internal/service/scheduler/scheduler.go	`84.72% <100.00%> (ø)`
internal/service/worker/handler.go	`79.59% <100.00%> (-0.21%)`	⬇️
internal/service/scheduler/execution_dag.go	`71.42% <71.42%> (ø)`
internal/service/scheduler/enqueue.go	`47.27% <33.33%> (-2.73%)`	⬇️

... and 14 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c54515...518782b. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

fix: preserve env-backed secrets across subprocess runs

60520c2

test: align explicit env queue expectations

518782b

yottahmd merged commit 80409e8 into main Mar 29, 2026
5 checks passed

yottahmd deleted the fix/1864-secret-env branch March 29, 2026 11:15

ruxo mentioned this pull request Apr 11, 2026

Suggestion for Windows: Add more Windows' default environment variables to the whitelist #1991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: preserve env-backed secrets for scheduler subprocess runs#1877

fix: preserve env-backed secrets for scheduler subprocess runs#1877
yottahmd merged 2 commits intomainfrom
fix/1864-secret-env

yottahmd commented Mar 29, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 29, 2026 •

edited

Loading

Review skipped

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

codecov Bot commented Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

yottahmd commented Mar 29, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Testing

Summary by CodeRabbit

Release Notes

Uh oh!

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

Uh oh!

Review skipped

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

codecov Bot commented Mar 29, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

yottahmd commented Mar 29, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 29, 2026 •

edited

Loading