Skip to content

fix: parallel inherit cwd bug#20

Merged
matt-FFFFFF merged 6 commits into
mainfrom
fix/parallelNoCwd
Jun 27, 2025
Merged

fix: parallel inherit cwd bug#20
matt-FFFFFF merged 6 commits into
mainfrom
fix/parallelNoCwd

Conversation

@matt-FFFFFF

@matt-FFFFFF matt-FFFFFF commented Jun 27, 2025

Copy link
Copy Markdown
Owner

Fix Parallel CWD Inheritance Bug

🐛 Problem Statement

Fixed critical issues with current working directory (cwd) propagation in Porch YAML workflows:

  • Parallel batches were not inheriting working directory changes from serial predecessors
  • After copycwdtotemp, subsequent parallel commands behaved inconsistently:
    • ✅ Direct shell commands correctly used the temp directory
    • foreachdirectory commands incorrectly used the original directory
  • Race conditions in parallel execution when setting cwd/env for child commands
  • Path duplication bug causing malformed directory paths

🔧 Root Cause Analysis

Investigation revealed two critical issues:

  1. Race Condition: ParallelBatch was setting cwd/env inside goroutine loops, creating race conditions
  2. Path Duplication: SetCwd method incorrectly joined identical relative paths, creating paths like:
    testdata/foreachdir/testdata/foreachdir  # ❌ Duplicated
    

🛠️ Solutions Implemented

1. Fixed Race Condition (parallelBatch.go)

// Before: Race condition - setting cwd/env inside goroutines
for _, cmd := range b.Commands {
    go func(c Runnable) {
        c.InheritEnv(b.Env)      // ❌ Race condition
        c.SetCwd(b.Cwd, false)   // ❌ Race condition
        // ...
    }(cmd)
}

// After: All setup before launching goroutines
for _, cmd := range b.Commands {
    cmd.InheritEnv(b.Env)        // ✅ Thread-safe setup
    cmd.SetCwd(b.Cwd, false)     // ✅ Thread-safe setup
}
for _, cmd := range b.Commands {
    go func(c Runnable) {
        resChan <- c.Run(ctx)    // ✅ Only execution in goroutine
    }(cmd)
}

2. Fixed Path Duplication (base.go)

// Before: Always joining paths, causing duplication
if c.Cwd == "" || !filepath.IsAbs(c.Cwd) {
    if c.Cwd == "" {
        c.Cwd = cwd
    } else {
        c.Cwd = filepath.Join(cwd, c.Cwd)  // ❌ Always joins
    }
}

// After: Smart path resolution prevents duplication  
if c.Cwd == "" || !filepath.IsAbs(c.Cwd) {
    if c.Cwd == "" {
        c.Cwd = cwd
    } else {
        // ✅ Only join if paths are different
        if c.Cwd != cwd {
            c.Cwd = filepath.Join(cwd, c.Cwd)
        }
    }
}

3. Enhanced Example Workflow

Added "echo item" command to demonstrate $ITEM environment variable:

commands:
  - type: "shell"
    name: "echo pwd"
    command_line: "echo $(pwd)"
  - type: "shell"
    name: "echo item"           # ✅ New command
    command_line: "echo $ITEM"  # ✅ Shows directory name

🧪 Testing & Verification

Fixed Failing Tests

  • TestForEachDirectoryParallel: All test cases now pass
  • All runbatch tests: Continue to pass (no regressions)
  • Integration tests: Verify end-to-end workflow behavior

Verified Workflow Behavior

✓ Copy CWD then Parallel Execution
  ✓ test cwd propagation
    ✓ echo pwd → /Volumes/code/porch/cmd  # Original directory

  ✓ Copy to Temp  # 📂 Creates temp directory
  
  ✓ Run Commands in Parallel
    ✓ pwd → /private/var/.../temp         # ✅ Inherited temp directory
    ✓ For Each Directory OK (parallel)
      ✓ [porch]
        ✓ echo pwd → /private/var/.../temp/cmd/porch  # ✅ Correct resolutionecho item → porch                           # ✅ Environment variable

📋 Behavior Guarantees

  • Parallel batches inherit cwd from serial predecessors
  • Commands within parallel batches do NOT inherit cwd from siblings
  • Relative paths correctly resolved against inherited cwd
  • Thread-safe parallel execution with no race conditions
  • Environment variables ($ITEM) work correctly in foreachdirectory
  • Backward compatibility maintained

🎯 Impact

This fix ensures consistent and predictable working directory behavior:

  • Users can reliably chain copycwdtotempparallel batch workflows
  • Shell commands and foreachdirectory commands behave consistently
  • Thread-safe implementation follows Go best practices
  • All tests pass with comprehensive coverage

Result: Resolves core cwd propagation issues while maintaining backward compatibility and improving parallel execution robustness.

Copilot AI review requested due to automatic review settings June 27, 2025 10:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that parallel batch commands correctly inherit the working directory set by preceding serial commands. Key changes include:

  • Always calling SetCwd with overwrite on parallel commands
  • Refactoring the serial batch loop to use slices.Values
  • Adding unit tests and updating examples to verify cwd inheritance

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
internal/runbatch/progressiveSerialBatch.go Refactored loop to update CWD using slices.Values
internal/runbatch/parallelBatch.go Changed SetCwd call to use overwrite=true
internal/runbatch/parallelBatch_test.go Added tests for cwd inheritance in parallel batches
examples/copycwd-then-parallel.yml New example showing cwd propagation in parallel runs
examples/avm-test-examples.yaml Removed outdated AVM test examples
Comments suppressed due to low confidence (1)

internal/runbatch/parallelBatch_test.go:6

  • The test uses assert and require but the imports for github.com/stretchr/testify/assert and github.com/stretchr/testify/require are missing. Add them to avoid compile errors.
import (

Comment thread internal/runbatch/progressiveSerialBatch.go
Comment thread examples/copycwd-then-parallel.yml Outdated
@matt-FFFFFF matt-FFFFFF merged commit 19245b5 into main Jun 27, 2025
6 checks passed
@matt-FFFFFF matt-FFFFFF deleted the fix/parallelNoCwd branch June 27, 2025 10:57
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.

2 participants