Repository navigation
fix(probe): signal the app's group through /proc, not a negative pid - #32
Merged
Merged
Conversation
Next.js's `next dev` keeps its `start-server` worker bound to the port after the parent handles SIGTERM, so stopBackgroundApp returned with the old server still answering. The next probe then booted on another port while the stale server took the probe's requests and answered 500 for routes it had never compiled — the two-product eval's oracle passed the login probe and failed every probe after it on Next.js. The stop now gives the group five seconds after TERM and then SIGKILLs it.
The inline start command carried single quotes that the launcher's own sh -c '…' wrapper cut short, so the background job kept the test's stdout pipe open and the test timed out on Linux.
[Medium impact] The PR appears safe to merge, with both previous test issues addressed. Summary
|
dash's builtin kill rejects a negative pid ("Illegal number"), so the
group signal `kill -- -<pgid>` never worked on the Debian-based
sandbox images: the fallback terminated the group leader alone and a
dev server's worker lived on. The stop now enumerates the group from
/proc and signals each member, TERM first and KILL five seconds later,
keeping the negative-pid form only where /proc is absent.
The behavioural test now kills its own group and removes its script in a finally block, polls up to five seconds for the group to vanish after SIGKILL instead of asserting at once, and both it and the stop ignore zombies: a killed process can sit unreaped in /proc for a moment, and an unreaped process is not a running server.
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
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
stopBackgroundAppnow finds every process in the app's group by reading/proc/<pid>/statand signals each one: SIGTERM, up to five seconds for the group to empty, then SIGKILL. Where/procis absent (a host runtime on macOS) it keeps the shell's negative-pid group kill.sh= dash, whose builtinkillrejects a negative pid with "Illegal number", so the group signalkill -TERM -- -<pgid>the harness has always sent failed and its fallback terminated only the group leader. Replaying the probe lifecycle insidequickstudy/workos-nextjsshowednext devand itsstart-serverchild still alive and bound to port 3000 thirty seconds after the stop; the next probe then booted on another port while the stale server answered the probe's requests.authkit-webhookseval (workos/evals#26, run01M50M4XPG5KR3WKQ2WC4DY5HW): the Next.js attempt passed the login probe and answered 500 to every later probe, routes the stale server had never compiled. The consumer'swaitForAppStoppedwaits 30 s for exactly this and then gives up; the harness should finish the job.setsidexists, i.e. Linux and the sandbox images; skips on macOS). Verified inoven/bun:1.