Skip to content

fix: isolate parent exit watcher environment#2256

Merged
yohamta0 merged 1 commit into
mainfrom
fix/parent-exit-watcher-env
Jun 3, 2026
Merged

fix: isolate parent exit watcher environment#2256
yohamta0 merged 1 commit into
mainfrom
fix/parent-exit-watcher-env

Conversation

@yohamta0

@yohamta0 yohamta0 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • isolate the Unix parent-exit watcher from inherited shell control variables
  • prevent exported TMOUT from timing out the watcher's blocking read and killing healthy workflow steps
  • add regression coverage for both inherited TMOUT and real parent-exit cleanup behavior

Related Issues

Checklist

  • Code style follows existing patterns
  • Self-review completed
  • Tests added or updated where needed
  • Documentation updated or not required
  • Local validation completed

Testing

  • go test ./internal/cmn/cmdutil -run 'TestParentExitWatcher(IgnoresInheritedShellTimeout|TerminatesChildWhenParentExits)$' -count=1
  • go test ./internal/cmn/cmdutil -count=1
  • make test TEST_TARGET=./internal/cmn/cmdutil
  • git diff --check
  • patched CLI repro: TMOUT=3 with a sleep 5 DAG completed successfully instead of dying at 3 seconds

Summary by cubic

Isolates the Unix parent-exit watcher from inherited shell control variables to prevent TMOUT from aborting its blocking read and killing healthy steps. Adds regression tests for TMOUT inheritance and parent-exit cleanup. Closes #2253.

  • Bug Fixes
    • Set a minimal env for the watcher (Env = ["PATH=/usr/bin:/bin"]) so shell vars like TMOUT don’t affect its blocking read.
    • Added external tests verifying the watcher ignores inherited TMOUT and still terminates the child when the parent exits.

Written for commit 162c532. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Fixed parent exit watcher stability on Unix systems by preventing shell environment variables from interfering with the watcher process.
  • Tests

    • Added test coverage for parent exit watcher behavior on Unix platforms, including shell timeout handling and child process termination scenarios.

Review Change Stack

@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7c5a9237-9b55-4d70-860d-a50a1b94118c

📥 Commits

Reviewing files that changed from the base of the PR and between 83640d8 and 162c532.

📒 Files selected for processing (2)
  • internal/cmn/cmdutil/parent_exit_watcher_unix.go
  • internal/cmn/cmdutil/parent_exit_watcher_unix_external_test.go

📝 Walkthrough

Walkthrough

This PR isolates the parent-exit watcher subprocess environment by setting a fixed PATH to prevent inherited shell variables from disrupting its blocking read. Comprehensive regression tests validate that inherited shell timeouts do not cause premature termination and that child processes are properly terminated when the parent exits.

Changes

Parent Exit Watcher Environment Isolation

Layer / File(s) Summary
Environment isolation for watcher subprocess
internal/cmn/cmdutil/parent_exit_watcher_unix.go
The watcher subprocess is configured with a deterministic PATH (/usr/bin:/bin) via watcher.Env to prevent inherited shell control variables from disrupting the blocking read.
Regression tests for parent-exit watcher behavior
internal/cmn/cmdutil/parent_exit_watcher_unix_external_test.go
Three test cases validate that the watcher ignores inherited shell timeouts, properly terminates child processes when the parent exits, and includes a helper test that runs a long-lived process. A utility function checks process existence using signal-based checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • dagucloud/dagu#2125: Both PRs modify the Unix StartParentExitWatcher behavior in internal/cmn/cmdutil/parent_exit_watcher_unix.go to adjust subprocess environment and signal handling for managing child process lifecycles.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% 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 accurately summarizes the main change: isolating the parent exit watcher's environment to fix shell control variable issues.
Description check ✅ Passed The description includes summary, changes, related issues, and completed checklist; it fully aligns with the template requirements.
Linked Issues check ✅ Passed The PR addresses issue #2253 by setting a minimal environment for the watcher to prevent TMOUT from killing healthy steps, and adds regression tests to validate the fix.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the parent exit watcher environment isolation and adding related tests; no extraneous modifications detected.

✏️ 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/parent-exit-watcher-env

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Re-trigger cubic

@yohamta0 yohamta0 merged commit 1eb4dc9 into main Jun 3, 2026
11 checks passed
@yohamta0 yohamta0 deleted the fix/parent-exit-watcher-env branch June 3, 2026 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

signal: killed

1 participant