Verify server PID identity before teardown kills (#31)#39
Merged
TheRealDinghyDog merged 1 commit intoJul 10, 2026
Merged
Conversation
Stale persisted server records were torn down by signalling the stored
PID and its process group with no check that the PID still belonged to
the plugin's `opencode serve` — after OS PID reuse this could SIGTERM an
unrelated process.
Teardown now re-reads the PID's live command line (cross-platform, via
the same helper the task-worker guard uses) and signals only when it
still looks like the plugin-owned `opencode serve` for the session's
port. The port is derived from the persisted URL when the explicit field
is absent, so records written before this change remain verifiable
instead of falling open. Anything unverifiable is left untouched; only
the stale metadata is cleared, reported as
{ killSkipped: true, reason, diagnostic } — `skipped` keeps its existing
"teardown did not run" meaning (leases/lock timeouts), so the SessionEnd
hook still clears the record correctly. Cancel logs the skip.
The matcher requires an opencode-ish executable token plus
`serve --port <port>`, and re-splits shell-wrapped Windows command lines
(cmd.exe /c "opencode serve ...") so win32 servers do not become
unkillable orphans. The spawn-time command line is recorded for
forensics; verification always matches the live value.
Drafted by OpenCode via the plugin's own rescue path (issue #31 task),
then reviewed and reworked: fail-closed for legacy records, URL-derived
port, de-duplicated cleanup, return-shape fix for the SessionEnd hook,
Windows shell-blob matching, and expanded tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Fixes #31 (review finding M-03).
Problem
When a persisted server record failed a health check, teardown sent
SIGTERMto the stored PID and its process group with no identity check — after OS PID reuse, an unrelated process could be killed. The branch already verifies identity for task workers; servers had no equivalent guard.Fix
readProcessCommandLine, same cross-platform helper the task-worker guard uses) and signals only when it still matches the plugin-ownedopencode servesignature: an opencode-ish executable token +serve+--port <port>.{ killSkipped: true, reason: "identity-mismatch" | "identity-unverified", diagnostic }whileskippedkeeps its existing "teardown did not run" meaning — the SessionEnd hook therefore still clears the session record correctly (returningskipped: truehere would have wedged the record forever). Cancel appends the skip to the job log.cmd.exe /c "opencode serve ...") are re-split before matching so win32 servers don't become unkillable orphans.Provenance & tests
Drafted by OpenCode through the plugin's own
--writerescue path (part of the parallel-delegation live test), then reviewed and reworked: the draft failed open for legacy records (gate skipped when identity fields were missing → blind kill preserved for all pre-upgrade records), returned a hook-breakingskipped: true, duplicated the cleanup block, and never actually used the recorded command line.Tests: identity match kills; PID-reuse mismatch and unreadable command line skip the kill and clear the record; legacy records verify via the URL-derived port (both match and mismatch); matcher unit tests incl. the Windows shell-blob shape; two pre-existing lease tests updated to inject identity.
npm test: 100 passed, 0 failed, 0 skipped.🤖 Generated with Claude Code