docs(skills): refactor-protocol — a session owns the processes it backgrounds - #345
Merged
Merged
Conversation
…kgrounds `kill $(jobs -p)` is always a no-op: command substitution forks, and the forked shell has an empty job table. #332 hit it twice, from two worktrees, and left twenty-four spin loops running after the task closed cleanly. Collect PIDs with $! at spawn, clean up in a trap ... EXIT, never redirect the cleanup's stderr away, and verify the PIDs are gone before claiming so.
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.
One rule in
refactor-protocol, and the machine cleanup that produced it. No source changes.What changed
## Cleanupgains a### Background processes you spawnedsubsection. The section wasabout the worktree only; a worktree is not the only thing a session leaves behind.
$!— never reconstruct the list later withjobs -p.trap … EXIT, so the path that skips cleanup does not exist.2>/dev/nullon it, and verifythe PIDs are actually gone rather than trusting that
killran.kill -- -<PGID>(the leader is dead, so the PGID is a freenumber) and not
pkill -fon the loop body (matches innocent shells).PPID == 1, not by grepping for the command you remember writing.Why — #332 is the worked example
LOADPIDS=$(jobs -p)is always empty. Command substitution forks, and the forked shellinherits no job table. Reproduced directly:
killgot nothing,2>/dev/nullate the error, and;(not&&) let the script exit zeroreporting success.
#332 closed cleanly — PR #338 merged, worktree removed, git metadata pruned — and left
twenty-four spin loops reparented to PID 1. Two batches of twelve, from two different
worktrees (
visionset-brace-expansionat 22:09,visionset-332-race-gateat 22:19), becausethe same technique was reused and the same line failed the same way both times. Between them
they burned ~24 CPU-hours, held 24 on a load average of 45–95, and were a direct cause of
#339 and #340 both having to invoke the declared-fallback rule instead of completing
scripts/check.sh. Nothing inside #332 could see any of it.All twenty-four were verified individually (PPID 1, spin-loop command line, matching start
time, cwd a deleted worktree, no children, group leader dead) and killed by explicit PID with
plain
SIGTERM. No escalation toSIGKILLwas needed. Load average before the first kill:57.43 57.81 56.30. #332 is closed and is not reopened by this.Corrections to my own earlier reporting
harness timeout". Wrong — all twenty-four were alive the whole time; the first batch
surfaced only when a later
pssorted by CPU put it in view. I asserted a negative from afiltered
pswithout running the positive check. ThePPID == 1bullet above is thatlesson, and it is why the rule says to hunt by parentage rather than by command text.
Coordination note — resolved, not pending
The brief warned that
visionset-rename-sweepmight hold uncommitted edits to this same file.It does not: that worktree is gone and its edits merged as PR #341 (
72e52d7), which isthe
origin/mainthis branch is cut from. Its three bullets are in## Scope disciplineabove my subsection and are untouched here.
One other worktree is active —
visionset-pytest-q-trap— butgit statusthere (read-only)shows it modifying
.agents/skills/backend/python-setup/SKILL.md, a different file. Nocollision. cf. #333.
Found, not fixed
report-3.tmp.mdandreport-4.tmp.mdwere to be deleted here. Both were already gonewhen I looked, removed by something outside this session. They are
*.tmp.mdand thereforegit-ignored, so they were never going to appear in a diff regardless; there is nothing to
commit for them. Their durable content is the rule above.
node_modules, so 9 of the 33node --testgates fail withCannot find package 'openapi-typescript'untilpnpm install --frozen-lockfileis run.That is check.sh: a partial run is indistinguishable from a full one on stdout, and no test guards it #336 (
check.shstepping over a missingnode_modulesinstead of failingloudly), reproduced again. Untouched. cf. check.sh: a partial run is indistinguishable from a full one on stdout, and no test guards it #336.
Test plan
Docs only — one Markdown file, no executable change.
bash scripts/check.shran in full,all three suites, on a machine no longer saturated (load 57 → 16 after part 1). No fallback
was invoked.
annotator + app e2efailed twice onPort 5273 is already in use— never on anassertion.
playwright.config.tspins 5273 with--strictPort, and two other worktrees(
visionset-223-polyline-kernel,visionset-pytest-q-trap) were cycling their owncheck.shruns concurrently; the port was verifiably free at launch each time and taken bythe time vite bound it. Re-run standalone with the port confirmed free:
185 passed (2.2m), 0 failures.browser cyclepassed on every attempt (35s, 32.9s).Found, not fixed (second)
Two concurrent
check.shruns on one machine cannot both run the e2e suite. Port 5273 isfixed and
--strictPort, so the loser fails with a message that reads like a broken devserver rather than "somebody else is using the port". Same family as #336 — a local-gate
ergonomics gap, not a product bug. Untouched.