feat: add run --cwd/--log-file, ls --full, clean --all#10
Merged
Conversation
Fix a correctness bug where spawned processes always inherited the long-lived daemon's original working directory instead of the caller's — `run` now defaults to the client's cwd and accepts --cwd to override it. Also add --log-file to tee full raw output to disk (unbounded by the in-memory ring buffer), ls --full to show the untruncated command and cwd, and clean --all to stop and remove every process in one shot.
Invalid PR TitlePR title must follow the Conventional Commits format since we use squash merge: Allowed types: Examples:
|
StructLint — All checks passed70 rules validated against
|
machado144
changed the title
…ed sleep The fixed 300ms delay before signaling assumed the spawned shell had already reached the trap statement, which isn't guaranteed on a slower or more contended CI runner — flaked in CI with the process observed already gone. Wait for a marker line the trap script itself prints after installing the trap instead.
Spawn() already runs the given command through `sh -c`, so wrapping the test command in another `sh -c '...'` forked an extra, unprotected outer shell (no trap of its own). bash collapses that redundant wrapper via its own tail-call exec optimization, masking the bug locally, but dash (Ubuntu's /bin/sh, used by the CI runner) does not — the outer shell died to the default SIGTERM handler and took the tracked process down with it, regardless of whether the inner trap actually worked. Reproduced and verified the fix against the actual CI shell (dash) in a matching Ubuntu/Go container, not just locally.
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
1 participant
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.
Summary
runalways ran the spawned process in the daemon's own cwd (wherever it happened to be auto-started from), not the caller's — now it defaults to the client's current directory, with--cwd <dir>to override (relative paths resolve against the caller's cwd).run --log-file <path>tees full raw PTY output to a file, unbounded by the in-memory ring buffer (useful for long/noisy processes wherelog/log --newonly ever see the buffered tail).ls --fullshows the untruncated command plus a CWD column instead of the default 60-char-truncated command.clean --all(optionally with--force) stops every still-running process and removes everything in one shot, instead of requiringkill+clean.Test plan
go build ./...,go vet ./...go test ./... -race -timeout 120s(matches the release CI invocation)services/process_manager_test.go: cwd is actually passed to the process, default cwd stays empty, log-file captures output the ring buffer discards, log-file open failures don't leak a registered process,CleanAllremoves running + finished processes, and — the important edge case — a process that traps SIGTERM survives non-forceCleanAllbut dies under--force.services/daemon_service_test.goforCwd/LogFile/Allrequest fields.integration/cli_test.go, including the regression test that provesrunuses the client's cwd rather than the daemon's,--cwdoverriding/resolving relatively,ls --fullvs default truncation, andcleanvsclean --allside effects.golangci-lint run— no new findings (33 pre-existing errcheck issues, identical onmain)docs/COMMANDS.mdflag tables andREADME.mdcommon-commands table