-
Notifications
You must be signed in to change notification settings - Fork 2
fix(lifecycle): run initializeCommand named sub-commands in parallel #100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e28defb
cfbc5b6
6322fba
6bc2997
3041906
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "image": "ghcr.io/devsy-org/test-images/go:1", | ||
| "initializeCommand": { | ||
| "write-one": "echo -n initCmdOne > ./init-cmd-one.out", | ||
| "write-two": ["sh", "-c", "echo -n initCmdTwo > ./init-cmd-two.out"] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| package devcontainer | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/devsy-org/devsy/pkg/devcontainer/config" | ||
| "github.com/devsy-org/devsy/pkg/types" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func newInitCmdConfig(hook types.LifecycleHook) *config.DevContainerConfig { | ||
| return &config.DevContainerConfig{ | ||
| DevContainerConfigBase: config.DevContainerConfigBase{ | ||
| InitializeCommand: hook, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func TestRunInitializeCommand_ParallelTiming(t *testing.T) { | ||
| // Two "sleep 0.5" commands that, if run in parallel, complete in ~0.5s. | ||
| cfg := newInitCmdConfig(types.LifecycleHook{ | ||
| "sleep-a": {"sleep", "0.5"}, | ||
| "sleep-b": {"sleep", "0.5"}, | ||
| }) | ||
|
|
||
| start := time.Now() | ||
| err := runInitializeCommand(t.TempDir(), cfg, nil) | ||
| elapsed := time.Since(start) | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.Less(t, elapsed, 900*time.Millisecond, | ||
| "two 0.5s commands should complete in ~0.5s when parallel, not ~1s") | ||
| } | ||
|
|
||
| func TestRunInitializeCommand_ParallelErrorCollection(t *testing.T) { | ||
| dir := t.TempDir() | ||
| markerFile := filepath.Join(dir, "ran.txt") | ||
|
|
||
| cfg := newInitCmdConfig(types.LifecycleHook{ | ||
| "fail": {"sh", "-c", "exit 1"}, | ||
| "succeed": {"sh", "-c", fmt.Sprintf("sleep 0.1 && echo done > %s", markerFile)}, | ||
| }) | ||
|
|
||
| err := runInitializeCommand(dir, cfg, nil) | ||
|
|
||
| // The combined error should mention which named command failed. | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), `named command "fail" failed`) | ||
|
|
||
| // The succeed command should have run to completion despite the failure. | ||
| _, statErr := os.Stat(markerFile) | ||
| assert.NoError(t, statErr, "succeed command should run even when fail command errors") | ||
| } | ||
|
Comment on lines
+24
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip parallel/error tests on Windows — they rely on
Proposed guard func TestRunInitializeCommand_ParallelTiming(t *testing.T) {
+ if runtime.GOOS == "windows" {
+ t.Skip("requires POSIX sleep/sh")
+ }
// Two "sleep 0.5" commands that, if run in parallel, complete in ~0.5s.
...
func TestRunInitializeCommand_ParallelErrorCollection(t *testing.T) {
+ if runtime.GOOS == "windows" {
+ t.Skip("requires POSIX sh")
+ }
dir := t.TempDir()And add 🤖 Prompt for AI Agents |
||
|
|
||
| func TestRunInitializeCommand_SingleKey(t *testing.T) { | ||
| dir := t.TempDir() | ||
| outFile := filepath.Join(dir, "out.txt") | ||
|
|
||
| cfg := newInitCmdConfig(types.LifecycleHook{ | ||
| "": {"sh", "-c", fmt.Sprintf("echo hello > %s", outFile)}, | ||
| }) | ||
|
|
||
| err := runInitializeCommand(dir, cfg, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| data, err := os.ReadFile(outFile) // #nosec G304 -- test path from t.TempDir | ||
| require.NoError(t, err) | ||
| assert.Contains(t, string(data), "hello") | ||
| } | ||
|
|
||
| func TestRunInitializeCommand_StringFormat(t *testing.T) { | ||
| dir := t.TempDir() | ||
| outFile := filepath.Join(dir, "out.txt") | ||
|
|
||
| // String format results in a single key with a shell command string. | ||
| cfg := newInitCmdConfig(types.LifecycleHook{ | ||
| "": {fmt.Sprintf("echo hello > %s", outFile)}, | ||
| }) | ||
|
|
||
| err := runInitializeCommand(dir, cfg, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| data, err := os.ReadFile(outFile) // #nosec G304 -- test path from t.TempDir | ||
| require.NoError(t, err) | ||
| assert.Contains(t, string(data), "hello") | ||
| } | ||
|
|
||
| func TestRunInitializeCommand_Empty(t *testing.T) { | ||
| cfg := &config.DevContainerConfig{} | ||
| err := runInitializeCommand(t.TempDir(), cfg, nil) | ||
| assert.NoError(t, err) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -325,9 +325,17 @@ func pipe( | |
| // Wait for the first copy to complete — its error is the meaningful one. | ||
| first := <-errChan | ||
|
|
||
| // Close both endpoints so the remaining goroutine unblocks promptly. | ||
| // Close all closable endpoints so the remaining goroutine unblocks | ||
| // promptly instead of leaking. | ||
| _ = toStdin.Close() | ||
| _ = fromStdout.Close() | ||
| if c, ok := fromStdin.(io.Closer); ok { | ||
| _ = c.Close() | ||
| } | ||
|
|
||
| // Drain the second goroutine so callers can safely read toStdout after | ||
| // pipe returns without a data race. | ||
| <-errChan | ||
|
Comment on lines
+328
to
+338
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Locate call sites of Inject to see whether Stdin is ever set to os.Stdin
# or another externally-owned *os.File that survives past Inject.
rg -nP -C4 '\bInject\s*\(\s*InjectOptions\b|InjectOptions\s*\{'
rg -nP -C2 '\bStdin\s*:\s*os\.Stdin\b'
# And any direct callers of pipe() within the package.
rg -nP -C3 '\bpipe\s*\('Repository: devsy-org/devsy Length of output: 11081 🏁 Script executed: # First, check the InjectOptions definitions and the pipe() call context
# Check around line 226 in pkg/inject/inject.go where pipe() is called
sed -n '200,250p' pkg/inject/inject.go
# Also check the pipe() function signature and the closing logic (lines 328-338)
sed -n '308,350p' pkg/inject/inject.go
# Check how stdin flows into inject.Inject
sed -n '1,50p' pkg/inject/inject.goRepository: devsy-org/devsy Length of output: 3262 🏁 Script executed: # Check the InjectOptions struct definition in both packages
rg -A 15 'type InjectOptions struct' pkg/inject/ pkg/agent/Repository: devsy-org/devsy Length of output: 1881 🏁 Script executed: # Check agent.InjectAgent to see what it passes for Stdin
sed -n '1,100p' pkg/agent/inject.go | head -100Repository: devsy-org/devsy Length of output: 2829 🏁 Script executed: # Check where stdin gets its default value in either package
rg -n 'Stdin.*os\.Stdin|Stdin.*=.*os\.Stdin|\.Stdin\s*=' pkg/inject/ pkg/agent/Repository: devsy-org/devsy Length of output: 104 🏁 Script executed: # Check where stdin value gets assigned in agent.InjectAgent and inject.Inject
sed -n '220,280p' pkg/agent/inject.go
# Also check line 442 context in agent.go
sed -n '430,450p' pkg/agent/agent.go
# Check if there's any default assignment to Stdin in either package
rg -n 'opts\.Stdin|Stdin\s*=' pkg/agent/inject.go pkg/inject/inject.go | head -20Repository: devsy-org/devsy Length of output: 2373 🏁 Script executed: # Let's see the complete Inject function flow - what stdin value reaches pipe()?
sed -n '50,150p' pkg/inject/inject.goRepository: devsy-org/devsy Length of output: 2462 🏁 Script executed: # Let's verify what types os.Stdin is and whether it implements io.Closer
# Also check if bytes.NewReader implements io.Closer
python3 <<'EOF'
# os.Stdin is *os.File which implements io.Closer
# bytes.NewReader does NOT implement io.Closer
print("os.Stdin type: *os.File")
print("*os.File implements io.Closer: YES")
print("bytes.NewReader implements io.Closer: NO")
print()
print("Flow analysis:")
print("1. If opts.Stdin is nil -> becomes bytes.NewReader(nil) -> SAFE (no Close)")
print("2. If opts.Stdin is os.Stdin -> remains os.Stdin -> UNSAFE (will Close FD 0)")
print("3. Current callers don't set Stdin -> always uses case 1 (safe)")
print("4. But code allows case 2 (unsafe) if caller sets it")
EOFRepository: devsy-org/devsy Length of output: 444 🏁 Script executed: # Final verification: check if there are any test cases that pass os.Stdin or similar
rg -n 'Stdin.*os\.|os\.Stdin' pkg/agent/inject_test.go pkg/inject/inject_test.goRepository: devsy-org/devsy Length of output: 960 🏁 Script executed: # Double-check: make sure there's no hidden default assignment of os.Stdin somewhere
rg -n 'opts\.Stdin\s*=' pkg/agent/ pkg/inject/Repository: devsy-org/devsy Length of output: 41 Closing a caller-provided The drain of Consider either:
🤖 Prompt for AI Agents |
||
|
|
||
| return first | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixture assumes a POSIX host shell.
echo -nis not portable (cmd.exe prints the-nliterally), andwrite-twoinvokesshdirectly, which isn't available on default Windows hosts. If this Docker e2e suite is ever exercised on a Windows runner this spec will silently write wrong marker contents (or fail). If Docker e2e is intentionally Linux/macOS only, this is fine as-is; otherwise considerprintf/platform-gated fixtures.🤖 Prompt for AI Agents