refactor: rename single.Single() to command.StartWithLockAndLogging()#637
Open
imphil wants to merge 3 commits intoskevetter:mainfrom
Open
refactor: rename single.Single() to command.StartWithLockAndLogging()#637imphil wants to merge 3 commits intoskevetter:mainfrom
imphil wants to merge 3 commits intoskevetter:mainfrom
Conversation
📝 WalkthroughWalkthroughReplaces the PID-file based Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
The function `single.Single()` executes a process in the background and prevents that a process with the same (user-supplied) name is executed again until it finishes. Hence the name "single" -- only a single process of the given name can run at any time. The naming is not self-descriptive, however, and easily confused with `devcontainer/single.go` (where single means running a single container, as opposed to multiple containers with docker-compose). Rename the file and update all its users. Refactor its internal functionality to meet the requirements of lint (e.g., reduced cyclomatic complexity). Functional change: the temporary files are no longer named like `something.pid.streams`, but only `something.streams`, which seems more accurate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/command/single.go`:
- Around line 24-26: The temp file paths (lockFile, pidFile, streamsFile) are
created directly under os.TempDir() and later opened with
os.WriteFile/os.Create, which is unsafe; change to create and use an owner-only
state directory (e.g., create a dir with 0700 using os.MkdirAll or os.MkdirTemp)
and place lock/pid/streams inside it, create files with O_CREATE|O_EXCL and mode
0600 (or os.WriteFile with 0600) to avoid race/clobber, and before opening
refuse/signal error if target is not a regular file or is a symlink by using
os.Lstat and FileMode checks; update the code paths that use lockFile, pidFile,
and streamsFile (and the places that call os.WriteFile and os.Create) to use
this secure directory and safe-open semantics.
- Around line 41-44: isProcessRunning currently returns nil for both "process
alive" and "not running", so the caller in single.go never short-circuits;
change isProcessRunning to return a sentinel error when the recorded PID is
alive (e.g., define var ErrProcessRunning = errors.New("process already
running") at package scope) and return nil only when the PID is not running, and
update the caller (the call to isProcessRunning in single.go) to detect and
return early on ErrProcessRunning (use errors.Is if you wrap the error) so the
helper does not launch a second process.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1de8660-262d-4a4a-80f4-5b90f8062e83
📒 Files selected for processing (8)
cmd/agent/container/setup.gopkg/command/single.gopkg/daemon/agent/daemon.gopkg/ide/fleet/fleet.gopkg/ide/jupyter/jupyter.gopkg/ide/openvscode/openvscode.gopkg/ide/rstudio/rstudio.gopkg/single/single.go
💤 Files with no reviewable changes (1)
- pkg/single/single.go
Comment on lines
+24
to
+26
| lockFile := filepath.Join(os.TempDir(), commandName+".lock") | ||
| pidFile := filepath.Join(os.TempDir(), commandName+".pid") | ||
| streamsFile := filepath.Join(os.TempDir(), commandName+".streams") |
There was a problem hiding this comment.
Avoid predictable state files directly under os.TempDir().
These paths are later opened with os.WriteFile and os.Create at Line 114 and Line 118. In the daemon/container setup paths this helper can run with elevated privileges, so a non-privileged user can pre-create those names in a shared temp directory and turn this into file clobbering or stream disclosure. Please move the lock/pid/streams files into an owner-only state directory and refuse to follow symlinks/non-regular files; at minimum, the streams file should not be created with the default os.Create permissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/command/single.go` around lines 24 - 26, The temp file paths (lockFile,
pidFile, streamsFile) are created directly under os.TempDir() and later opened
with os.WriteFile/os.Create, which is unsafe; change to create and use an
owner-only state directory (e.g., create a dir with 0700 using os.MkdirAll or
os.MkdirTemp) and place lock/pid/streams inside it, create files with
O_CREATE|O_EXCL and mode 0600 (or os.WriteFile with 0600) to avoid race/clobber,
and before opening refuse/signal error if target is not a regular file or is a
symlink by using os.Lstat and FileMode checks; update the code paths that use
lockFile, pidFile, and streamsFile (and the places that call os.WriteFile and
os.Create) to use this secure directory and safe-open semantics.
There was a problem hiding this comment.
@imphil, understood — acknowledged and moving on.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/command/single.go (2)
121-121: Consider explicit permissions for the streams file.The PID file at Line 117 is created with
0o600, butos.Createhere uses the default0666(minus umask). For consistency and to ensure the output logs aren't world-readable, consider usingos.OpenFilewith explicit0o600permissions.♻️ Use explicit permissions
- f, err := os.Create(streamsFile) // `#nosec` G304: not user input + f, err := os.OpenFile(streamsFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) // `#nosec` G304: not user input🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/command/single.go` at line 121, The streams file is created with os.Create(streamsFile) which uses default permissions; change to use os.OpenFile(streamsFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) so the file is created with explicit 0600 permissions (matching the PID file) and keep the existing error handling for the returned file (variable f) — replace the os.Create call with os.OpenFile and retain or reapply the `#nosec` annotation if needed.
110-118: Unnecessary wait loop – PID is already set afterStart().In Go,
cmd.Start()does not return until the process is forked andcmd.Process.Pidis populated. The conditioncmd.Process.Pid < 0will never be true after a successfulStart(), making this loop dead code.♻️ Simplified version
func writePidToFile(cmd *exec.Cmd, pidFile string) error { - // wait until we have a process id - for cmd.Process.Pid < 0 { - time.Sleep(time.Millisecond) - } - // write pid to file return os.WriteFile(pidFile, []byte(strconv.Itoa(cmd.Process.Pid)), 0o600) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/command/single.go` around lines 110 - 118, The loop in writePidToFile is unnecessary because cmd.Process.Pid is populated by cmd.Start(); remove the for-loop and instead validate that cmd.Process is non-nil, then immediately write strconv.Itoa(cmd.Process.Pid) to pidFile with os.WriteFile (mode 0o600), returning any error; ensure you still handle the case where cmd.Process == nil by returning a clear error before attempting to access Pid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/command/single.go`:
- Around line 120-132: redirectStreams currently opens streamsFile with
os.Create and assigns it to cmd.Stdout/cmd.Stderr but never closes the parent
file handle, leaking descriptors; change redirectStreams to return the *os.File
(or nil on error) instead of closing it internally, update its signature and
callers (notably StartWithLockAndLogging) to receive that *os.File, call
cmd.Start(), and then close the returned file (ignoring close errors) after
Start succeeds so the parent relinquishes its descriptor while the child keeps
its copy.
- Around line 90-108: startAndRecordPid currently starts the process then writes
the PID; if writePidToFile(cmd, pidFile) fails the process is left running
without a recorded PID which can orphan it—update startAndRecordPid to handle
PID-write failures by attempting cleanup and reporting: after cmd.Start(), if
writePidToFile returns an error then try to terminate the started process (use
cmd.Process.Kill() and/or cmd.Wait() to ensure it stops), log a clear
warning/error that includes the PID and the write error, and return the original
write error (or a wrapped error). Keep the successful path that releases
resources (cmd.Process.Release()) only when PID was successfully recorded and
the process is intentionally left running.
---
Nitpick comments:
In `@pkg/command/single.go`:
- Line 121: The streams file is created with os.Create(streamsFile) which uses
default permissions; change to use os.OpenFile(streamsFile,
os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) so the file is created with explicit
0600 permissions (matching the PID file) and keep the existing error handling
for the returned file (variable f) — replace the os.Create call with os.OpenFile
and retain or reapply the `#nosec` annotation if needed.
- Around line 110-118: The loop in writePidToFile is unnecessary because
cmd.Process.Pid is populated by cmd.Start(); remove the for-loop and instead
validate that cmd.Process is non-nil, then immediately write
strconv.Itoa(cmd.Process.Pid) to pidFile with os.WriteFile (mode 0o600),
returning any error; ensure you still handle the case where cmd.Process == nil
by returning a clear error before attempting to access Pid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3127785c-844c-49d9-96e2-fcb0a61cff1a
📒 Files selected for processing (1)
pkg/command/single.go
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/command/single.go`:
- Around line 101-112: openStreamsFile currently always calls
os.Create(streamsFile) which creates/truncates the file even when cmd.Stdout and
cmd.Stderr are already set; change the logic in openStreamsFile to first check
cmd.Stdout and cmd.Stderr and only call os.Create/open the streamsFile when at
least one of them is nil, then assign the returned *os.File to whichever of
cmd.Stdout or cmd.Stderr is nil (cmd.Stderr = f and/or cmd.Stdout = f) and
return the file; keep error propagation the same and avoid creating the file
when no redirection is needed.
- Around line 64-78: The kill path currently calls cmd.Process.Kill() but
doesn't wait, so the child can become a zombie; after calling cmd.Process.Kill()
in the error block that follows os.WriteFile, call cmd.Wait() (handling and
returning any Wait() error appropriately) to reap the child before returning the
wrapped error. Also, after a successful Start() (before writing the pid file),
call cmd.Process.Release() to detach the child from the Cmd object so the parent
stops tracking it; keep the existing streamsF.Close() behavior and ensure errors
from Release() or Wait() are handled/wrapped similarly to other returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9624c56-fc11-423f-ade7-424fcf776543
📒 Files selected for processing (1)
pkg/command/single.go
Comment on lines
+64
to
+78
| if err := cmd.Start(); err != nil { | ||
| streamsF.Close() | ||
| return fmt.Errorf("start process: %w", err) | ||
| } | ||
| // Close the parent's copy of the streams fd. After Start() forks, the | ||
| // child has its own copy; the parent no longer needs it. | ||
| streamsF.Close() | ||
|
|
||
| if err := os.WriteFile(pidFile, []byte(strconv.Itoa(cmd.Process.Pid)), 0o600); err != nil { | ||
| // Process is running but untracked. Kill it to prevent orphans. | ||
| _ = cmd.Process.Kill() | ||
| return fmt.Errorf("write pid file (process killed to prevent orphan): %w", err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify process lifecycle handling in this helper.
rg -n 'cmd\.Start\(|cmd\.Wait\(|Process\.Release\(|Process\.Kill\(' pkg/command/single.go -C2Repository: skevetter/devpod
Length of output: 468
🏁 Script executed:
# First, let's read the full function to understand its intent
cat -n pkg/command/single.go | head -100Repository: skevetter/devpod
Length of output: 3581
🏁 Script executed:
# Also check if cmd.Wait() or Release() is called elsewhere in this file or related files
rg -n 'cmd\.Wait\(|Process\.Wait\(|Process\.Release\(' pkg/command/ -C3Repository: skevetter/devpod
Length of output: 42
🏁 Script executed:
# Check what happens to the process after Start() - is it meant to be a daemon?
rg -n 'daemoniz|background|detach|orphan' pkg/command/single.go -C2Repository: skevetter/devpod
Length of output: 352
Add Wait() on the kill path to reap killed processes.
After cmd.Process.Kill() at line 74, the child process should be waited on to ensure it's reaped. Otherwise, the killed process becomes a zombie until the parent exits.
Additionally, since this function starts a detached daemon process (per line 17 documentation), calling cmd.Process.Release() after successful Start() would explicitly declare the process as detached and prevent unnecessary resource tracking by the Cmd object in long-running parents.
Suggested changes
if err := os.WriteFile(pidFile, []byte(strconv.Itoa(cmd.Process.Pid)), 0o600); err != nil {
// Process is running but untracked. Kill it to prevent orphans.
_ = cmd.Process.Kill()
+ _ = cmd.Wait()
return fmt.Errorf("write pid file (process killed to prevent orphan): %w", err)
}
+
+ _ = cmd.Process.Release()
return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/command/single.go` around lines 64 - 78, The kill path currently calls
cmd.Process.Kill() but doesn't wait, so the child can become a zombie; after
calling cmd.Process.Kill() in the error block that follows os.WriteFile, call
cmd.Wait() (handling and returning any Wait() error appropriately) to reap the
child before returning the wrapped error. Also, after a successful Start()
(before writing the pid file), call cmd.Process.Release() to detach the child
from the Cmd object so the parent stops tracking it; keep the existing
streamsF.Close() behavior and ensure errors from Release() or Wait() are
handled/wrapped similarly to other returns.
Comment on lines
+101
to
+112
| func openStreamsFile(cmd *exec.Cmd, streamsFile string) (*os.File, error) { | ||
| f, err := os.Create(streamsFile) // #nosec G304: not user input | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if cmd.Stderr == nil { | ||
| cmd.Stderr = f | ||
| } | ||
| if cmd.Stdout == nil { | ||
| cmd.Stdout = f | ||
| } | ||
| return f, nil |
There was a problem hiding this comment.
Avoid creating/truncating .streams when no redirection is needed.
openStreamsFile always calls os.Create at Line [102]. If both outputs are already configured, this still creates/truncates the file unnecessarily.
♻️ Proposed fix
func openStreamsFile(cmd *exec.Cmd, streamsFile string) (*os.File, error) {
+ if cmd.Stdout != nil && cmd.Stderr != nil {
+ return nil, nil
+ }
+
f, err := os.Create(streamsFile) // `#nosec` G304: not user input
if err != nil {
return nil, err
}
@@
return f, nil
} streamsF, err := openStreamsFile(cmd, streamsFile)
if err != nil {
return err
}
if err := cmd.Start(); err != nil {
- streamsF.Close()
+ if streamsF != nil {
+ _ = streamsF.Close()
+ }
return fmt.Errorf("start process: %w", err)
}
@@
- streamsF.Close()
+ if streamsF != nil {
+ _ = streamsF.Close()
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/command/single.go` around lines 101 - 112, openStreamsFile currently
always calls os.Create(streamsFile) which creates/truncates the file even when
cmd.Stdout and cmd.Stderr are already set; change the logic in openStreamsFile
to first check cmd.Stdout and cmd.Stderr and only call os.Create/open the
streamsFile when at least one of them is nil, then assign the returned *os.File
to whichever of cmd.Stdout or cmd.Stderr is nil (cmd.Stderr = f and/or
cmd.Stdout = f) and return the file; keep error propagation the same and avoid
creating the file when no redirection is needed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The function
single.Single()executes a process in the background andprevents that a process with the same (user-supplied) name is executed
again until it finishes. Hence the name "single" -- only a single
process of the given name can run at any time.
The naming is not self-descriptive, however, and easily confused with
devcontainer/single.go(where single means running a single container,as opposed to multiple containers with docker-compose).
Rename the file and update all its users. Refactor its internal
functionality to meet the requirements of lint (e.g., reduced cyclomatic
complexity).
Functional change: the temporary files are no longer named like
something.pid.streams, but onlysomething.streams, which seems moreaccurate.
Summary by CodeRabbit