Uh oh!
There was an error while loading. Please reload this page.
fix(scripts): gen-sdui-manifest cleanup reaps the session it started instead of leaking a flock-holding orphan - #9580
Merged
Conversation
The EXIT trap armed `kill "$DUMP_DEV_PID"`, ran, and left the dev server alive 20 minutes later holding the container's shared heavy-verify flock, so every later agent's build queued out at exit 99 with no signal. Three measured properties of the old form were each independently wrong: - `kill "$!"` does not reap the tree. Descendants reparent to init and no single-pid kill reaches them; signalling the process GROUP does. - The group could not be signalled safely: a background job in a non-interactive shell inherits the SCRIPT's process group, which under the heavy-verify discipline is led by the wrapping `flock` itself. `setsid` makes the group kill bounded. - `flock(1)` holds its lock on an open fd and background children inherit open fds, so the orphan held the caller's lock. Closing inherited descriptors makes a missed kill untidy rather than a container-wide stall. Cleanup now escalates TERM to KILL, verifies, and reports what it could not kill. Traps INT/TERM/HUP too, since bash runs no EXIT trap when signalled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
The lock-fd assertion was a phantom check: it ran after the stub leader had exited, so it only ever inspected the reparented helper, which never inherits the descriptor. Measured — with the fd hygiene deleted the test still passed. Counting while the leader lives turns that ablation red (LOCKFDS=1). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. |
os-steve
marked this pull request as ready for review
August 18, 2026 12:44
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Fixes#9399
The card's diagnosis was partly wrong, and the wrong half matters
The card inferred: "the wrapper re-execs, so
$!is not the pid that matters." Measured here, that is false./opt/node22/bin/pnpmis a node script with a#!/usr/bin/env nodeshebang, so bash execs it in the process it forked for the background job —$!is thenode .../pnpm --filter @object-ui/console exec vite devprocess, which is exactly the shape of the survivor in the incident'spsline. A fix aimed at "track the pid that actually binds the port" would have been aimed at a non-problem.The second card observation, "
ss -ltnshowed nothing on 5180", is an artefact, not a measurement:ssandnetstatare absent from these containers. The usual spelling hides it —ss -ltn | grep :5180prints nothing and exits 0 whether or not anything is listening. So "it was no longer serving anything" is unproven. (The companion claim — that the vite child was gone — does come frompsand stands.) The wait loop is now commented to say why it usescurl, perdocs/qa/platform-checklist/RUNNER.md.What I could not determine, and why the fix does not depend on it
I could not reproduce the specific hang in which that particular SIGTERM failed to reap that particular run: it needs a real vite under a real console build tree, and
.cache/objectui-*is absent from a fresh worktree. With stubs, plain SIGTERM to the pnpm wrapper reaps it cleanly every time, including under an early-signal race.So the cleanup here is deliberately cause-agnostic: it escalates TERM to KILL, it verifies, and it prints what it could not kill. Picking one diagnosis and fixing only that is the failure mode where the leak looks fixed and recurs identically.
Three properties, each measured, each independently load-bearing
1.
kill "$!"cannot reap the tree. Measured: after SIGTERM to the wrapper, a descendant survives reparented to init (PPID 1). No single-pid kill reaches it. Signalling the process group does.2. The group could not be signalled safely before this change. A background job in a non-interactive shell does not get its own process group — it inherits the script's. Measured under the agent heavy-verify discipline, the backgrounded server's PGID was the PID of the wrapping
flockitself. So the card's suggestedkill -- -$PGIDon the inherited group would have killed the caller's lock holder and the script.setsidis what makes a group kill bounded, and it is a prerequisite for the fix, not a flourish.3. The fd inheritance is real, and it is what makes this a container outage.
flock(1)holds its lock on an open fd; background children inherit open fds. Measured before the change: the backgrounded wrapper held the caller's lock fd andfuser -vlisted it as a holder. Its own grandchild held zero — node closes the fd across its spawn — so the wrapper is precisely the process that converts "a leaked dev server" into "every later agent in this container queues out at exit 99". Measured after:fuser -vlists onlyflock/bash, the server is absent, and the lock is re-acquirable.The triage comment asked for the fd half to be treated as a required half rather than an optional garnish. It is: it is the half that makes a missed kill survivable.
The change
setsid, in its own session, with every inherited descriptor above stderr closed beforeexec.$!. Here the two agreed (setsid exec'd in place instead of forking) — the pidfile is used because whethersetsidexecs or forks depends on job control, and the failure mode of guessing is a silently wrong pid.INT/TERM/HUPas well asEXIT, re-raising after cleanup: bash runs noEXITtrap when the script is signalled, which is how an agent container reclaims a run.packages/spec/scripts/check-generated-ledger.test.tsasserts this script still containscheck:react-declaration-parity; it does.Pinning it — including one green line that was a lie
The new test sources the script (it returns right after defining its helpers, so no generation runs) and drives the real functions against a stub that reproduces the reparented-orphan shape.
Ablations, run against the final commit, each turning a different assertion red:
setsidpathLOCKFDS=1, expected015876: expected '15876' to be '', and the script's owncould not stopdiagnostic firedThe middle row is the point of running ablations at all. On the first version of this test that ablation stayed green. The fd count ran after the stub leader had already exited, so it only ever inspected the reparented helper — which never inherits the descriptor anyway. It was a phantom check that would have shipped a permanently-unfailable assertion. Moving the count to while the leader is alive is what makes it real, and the test now asserts the leader was alive at the moment of counting so the ordering cannot silently regress. The orphan's prior existence is asserted too, so the survivor check can never pass vacuously.
Verification
All at
68bdc2ee1, the final commit; worktree clean.pnpm --filter @objectstack/spec test— 410 files / 10938 tests passedpnpm --filter @objectstack/spec typecheck— green, includingcheck:test-typecheck(the new file compiles; debt held at 55 files / 263 errors, unchanged)node scripts/pm/dispatch-gates.mjs, which named a family the dispatch did not, because adding a test file moves gates the script path alone does not:check:nul-bytes,check:engine-double-contract,check:where-matcher,check:query-options-erasure,check:merge-driver,check:type-source-resolution,check:affected-docs, and speccheck:empty-state/check:liveness/check:strictness-ledger/check:variant-docs— all greencheck:dev-prereqs,check:type-check-coverage,check:type-check-debt. All three require the whole workspace built; on this worktreecheck:dev-prereqsexits 1 reporting 67/67 packages missingdist/, which is the unbuilt-tree precondition and not a finding about this diff. Reported as not measured rather than green — CI runs them.Scope
The dispatch scoped the file surface to
scripts/gen-sdui-manifest.sh. The fix is entirely in that file. The one other path is a new test file,packages/spec/scripts/gen-sdui-manifest-cleanup.test.ts, which can collide with nothing because it did not previously exist; it is the regression pin the triage comment asked for. Flagging it rather than burying it.No changeset: this changes repo tooling and a test, and releases nothing — the
skip-changesetcase named inlint.yml.Out of scope: #9578, filed separately and remaining open — the script hardcodes port 5180 and one log path, so two concurrent runs in one container may dump each other's server.
Generated by Claude Code