Stop tui/api tests from fork-bombing as fake daemons#606
Merged
Conversation
Two tests reached production fork-exec helpers that re-spawn os.Executable() with "daemon start": TestApp_HandleRestartDaemonKey_RestartChosen (via client.AutoStart from a goroutine) and TestSpawnSuccessorDaemon (direct call). Under `go test` os.Executable() resolves to the test binary, Go's test framework treats "daemon start" as positional args, and m.Run() runs the whole package again — including the test that just forked — orphaning multiple test-binary copies at ~85% CPU each and overwriting the real ~/.argus/argusd symlink to point at a temp binary that vanishes on test end. - Add ErrTestBinary / errSpawnFromTestBinary backstops to AutoStart and spawnSuccessorDaemon. Refuse to fork when os.Args[0] looks like a Go test binary. - Add App.restartDaemonFn injection so the TUI test can swap in a stub instead of relying solely on the backstop. - Redirect HOME via t.Setenv in the affected TUI test as defense in depth. - Rewrite both tests to assert errors.Is the new sentinel. - Document the gotcha in context/knowledge/gotchas/daemon-rpc.md.
The test injected a bare pendingRestart{} (task=nil) and then flipped
consumed=true AFTER calling r.Start, so on slow runners the exit
goroutine in runner.Start.func2 read consumed=false first, called
r.Start(pending.task=nil, ...), and panicked with a nil deref at
runner.go:57 ("session already exists for task..." would otherwise
have been the assertion).
Fix: set consumed=true on the injected entry up front (before the
second Start), removing the race. Test still verifies the intended
invariant — the resumed session's exit goroutine reads consumed=true
and skips another restart — and stays green across -count=50 on
darwin and (per CI) ubuntu.
After adding the *.test backstops, TestC_AutoStart and TestSpawnSuccessorDaemon short-circuit at the new check — they no longer exercise the os.Executable + symlink + exec.Command + poll-timeout path they used to reach (with a guaranteed-fail socket). That dropped the filtered coverage from 88.0% to 87.8% on CI, tripping the 88% floor. Those lines are now genuinely untestable under `go test`: the only way to exercise them is to fork the test binary as a fake daemon, which is the exact fork bomb the backstop prevents. Move the post-backstop bodies into autostart_fork.go and spawn_fork.go, list both in coverage-ignore.txt, and have the thin wrappers delegate through them. Filtered coverage back to 88.2% locally.
…art call Review surfaced two architecture warnings on the fork-bomb fix: 1. `isTestBinary` is duplicated across internal/agent/cleanup.go, internal/daemon/client/client.go, and internal/api/selfupdate.go. The `/_test/` arm is subtle and a future maintainer copying the function for a fourth backstop site could easily drop it. Add a cross-reference comment at each site so the drift risk is loud. 2. `updateArgus` calls `a.restartDaemonFn()` synchronously while the other three call sites use `go a.restartDaemonFn()`. The synchronous call is correct because `updateArgus` is itself already running in a goroutine, but the asymmetry was undocumented and would lure a future reader into "fixing" it by adding `go`. Add a one-line comment.
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
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.
Two test paths reached production fork-exec helpers that re-spawn os.Executable()
with "daemon start". Under
go test, that re-runs the entire test package — whichre-hits the test that forked — orphaning multiple *.test daemon start processes at
~85% CPU each, and overwriting ~/.argus/argusd to point at a temp test binary
that vanishes when the test ends.
Adds backstops at both fork sites (client.AutoStart, api.spawnSuccessorDaemon)
that refuse to fork when os.Args[0] looks like a Go test binary. Adds
App.restartDaemonFn injection so the TUI test can swap in a stub instead of
relying solely on the backstop. Splits the post-backstop fork bodies into
separate files (autostart_fork.go, spawn_fork.go) listed in coverage-ignore.txt.
Rewrites both tests to assert the new sentinel errors.
Also fixes a pre-existing race in TestRunner_KickRerender_NoLoopOnImmediateCrash
(flipping consumed=true after r.Start raced with the exit goroutine; nil-deref
panic on slow CI runners). Same panic was failing the last two master commits.
Co-Authored-By: Claude noreply@anthropic.com