fix(pi): keep one watcher owner and exit cleanup per Pi session - #1183
Open
v4nimus wants to merge 5 commits into
Open
fix(pi): keep one watcher owner and exit cleanup per Pi session#1183v4nimus wants to merge 5 commits into
v4nimus wants to merge 5 commits into
Conversation
…e re-verification
…ve-owner contention
5 tasks
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.
Intent
Fix the Pi primary watcher extension so a same-process session_shutdown followed by session_start restores a clean reusable lifecycle, re-arms supervision without duplicate watcher children or weakened lock checks, reinstalls exactly one process-exit cleanup listener per active logical session, resets retry and restoration state safely, and ships the correction as an unmerged PR with focused regression coverage and green checks.
What Changed
.pi/extensions/fm-primary-pi-watch.tsmakes generation activation first-owner-wins: a factory bind that overlaps a still-live generation now defers instead of taking over the live arm child, andsession_shutdowndeactivates the ending generation (clearingactiveGeneration) rather than only stopping it, so a replacementsession_startcan claim ownership and arm again in the same process.process.once("exit", …)to install/remove tied to the active generation, so exactly one exit listener is installed per active logical session and it is dropped when that session ends.docs/supervision-protocols/pi.md,docs/verification/supervision.md, anddocs/watcher-continuity.mdupdated to match, andtests/fm-pi-watch-extension.test.shextended to cover replacement arming with reset retry state, the overlapping-bind refusal, the deferred binding's refusal after its incumbent ends plus arming through its ownsession_start, and per-session listener install/removal across/new,/resume,/fork, and quit.Risk Assessment
✅ Low: The branch is well-bounded to one Pi extension lifecycle path, satisfies every source-verifiable intent criterion, and the round-1 messaging inaccuracy is now resolved by a discriminator that provably only claims a live competing owner when one exists, with focused regression coverage for the previously-untested owner-ended state.
Testing
Ran the branch's own targeted suite (tests/fm-pi-watch-extension.test.sh, 33 checks green) and then proved the intent at the product level with a manual harness that loads the tracked extension exactly as Pi does and drives a real session lifecycle: startup arm, then /new, /resume and /fork same-process restarts, an overlapping stray binding, that binding outliving its owner, and terminal quit. Running the identical harness against the base commit isolates the correction: base armed a second watcher child for the overlapping binding (2 live children) and falsely told the real live owner "Pi session is shutting down", while the branch refuses with the new live-owner message, keeps one child, later reports the binding as inactive rather than naming a vanished owner, and arms only after its own session_start. Across four replacements the branch held exactly one live watcher child and one process-exit cleanup listener, dropping to zero on quit where base left the listener installed; every replacement arm reported "arm child 1", showing retry/restoration state reset. Swapping the base extension under the branch tests confirmed the new generation-owner coverage genuinely fails on the old behavior. No screenshot applies: the change touches no rendering code, and the end-user surface is the fm_watch_arm_pi tool-result text, captured verbatim in the transcripts. Worktree left clean and the leaked test watcher child cleaned up.
Evidence: Pi lifecycle transcript — branch a4d3724 (fixed)
[5] A stray extra extension binding activates while session A's generation is still live. agent(stray binding) calls fm_watch_arm_pi -> watcher: not armed - another live Pi session generation owns watcher supervision; this binding stays idle until that session ends observed: live watcher children=1 (pid 241072), process-exit cleanup listeners=1 agent(live owner) calls fm_watch_arm_pi -> watcher: unchanged - Pi extension already owns an arm child; no manual re-arm needed [6] The live owner's session ends (/new). The stray binding is now behind no live owner. agent(stray binding) calls fm_watch_arm_pi -> watcher: not armed - this Pi session binding is inactive and does not own watcher supervision; it stays idle until it receives its own session start observed: live watcher children=0, process-exit cleanup listeners=0 [8] Operator quits Pi: session_shutdown(quit). agent(after quit) calls fm_watch_arm_pi -> watcher: not armed - Pi session is shutting down observed: live watcher children=0, process-exit cleanup listeners=0Evidence: Pi lifecycle transcript — base fa0d85d (before)
[5] A stray extra extension binding activates while session A's generation is still live. agent(stray binding) calls fm_watch_arm_pi -> watcher: started Pi extension arm child 1; future ordinary re-arms are automatic observed: live watcher children=2 (pid 240348, 240360), process-exit cleanup listeners=1 agent(live owner) calls fm_watch_arm_pi -> watcher: not armed - Pi session is shutting down [8] Operator quits Pi: session_shutdown(quit). observed: live watcher children=0, process-exit cleanup listeners=1Evidence: Before/after diff of the same lifecycle run (PIDs normalized)
Evidence: Reproducible lifecycle demo harness
Evidence: Branch tests run against the base extension (regression guard proof)
ok - Pi watcher arm distinguishes all session lock ownership states not ok - Pi session transitions must rearm through an explicit generation owner: expected exit 0, got 1Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
.pi/extensions/fm-primary-pi-watch.ts:365- The new non-owner refusal is selected by!owner.stoppingalone, which does not prove another generation is live, and a deferred binding has no path back to ownership. Reachable sequence: binding A activates and arms; an overlapping factory bind B defers (activateGeneration returns early at :212); A's session_shutdown runs deactivateGeneration and setsactiveGeneration = null(:232). B's fm_watch_arm_pi now returns "watcher: not armed - another live Pi session generation owns watcher supervision; this binding stays idle until that session ends" even though no generation owns supervision at all. Nothing re-activates B when the owner ends — the message's own promise is unimplemented, since B only re-activates if it later receives its own session_start, by which point another binding will normally have claimed ownership. The new guidance at docs/supervision-protocols/pi.md:12 ("is not a repair condition ... do not retry the call or restart Pi") turns that state into a terminal, silent loss of watcher supervision — the exact failure class this extension exists to prevent. The regression tests cover the overlap-while-live case (tests/fm-pi-watch-extension.test.sh:1119-1133) but not the owner-ended case. Single-binding sessions cannot reach it (a non-live generation is alwaysstopping), so this is a follow-up rather than a blocker. Earliest shared boundary for the fix: have startArm/activateGeneration adoptownerwhenactiveGeneration === nullinstead of refusing, and keep the non-owner message for the genuinely-contended case only.🔧 Fix: distinguish inactive-binding arm refusal from live-owner contention
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-pi-watch-extension.test.sh— full targeted file for this change, 33/33 ok, includingPi scheduled retry remains extension-owned and resets for a replacement session,Pi session transitions use a generation owner across /new /resume /fork, stale callbacks, and quit, andPi process-exit cleanup listener tracks each active replacement session exactly onceManual end-to-end lifecycle demo against the branch extension:REPO_ROOT=<worktree> bash /tmp/no-mistakes-evidence/01KYM87QBGGQCJ038GW0J82AZZ/pi-session-restart-demo.sh <worktree>/.pi/extensions/fm-primary-pi-watch.ts fixed— loads the tracked extension the way Pi does, claims a real session lock, drives session_start/session_shutdown(new|resume|fork|quit) plus fm_watch_arm_pi, and records tool-result text, live watcher child PIDs, and process-exit listener countsSame demo against the base-commit extension for contrast:bash pi-session-restart-demo.sh base-fm-primary-pi-watch.ts base(extension extracted viagit show fa0d85d:.pi/extensions/fm-primary-pi-watch.ts)Regression-guard check: temporarily swapped the base extension into the worktree, ranbash tests/fm-pi-watch-extension.test.sh(fails atPi session transitions must rearm through an explicit generation owner), then restored withgit checkout -- .pi/extensions/fm-primary-pi-watch.tsand verifiedgit status --porcelainis emptyCleanup verification: killed the watcher child leaked by the deliberately-failing base run, removed its temp dir, and confirmed no diff againsta4d3724docs/verification/supervision.md:135- docs/verification/supervision.md:135 records that tests/fm-pi-primary-types.test.sh skipped its typecheck because tsc was unavailable, so the changed TypeScript extension has no refreshed typecheck evidence on this branch. I confirmed tsc is still absent on this host (no PATH entry, no local node_modules), so the caveat is currently accurate and I left it in place rather than overstating verification. If the validation phase runs the typecheck on a host with tsc, that clause should be dropped from the record.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.