Repository navigation
fix: reject stale Herdr endpoints after session resets - #6760
cloud-practitioner wants to merge 17 commits into
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [ "$child_backend" = herdr ]; then | ||
| FM_STATE_OVERRIDE="$sub_state" teardown_herdr_cleanup_preflight "$child_t" "$child_id" "$child_wt" || return 1 | ||
| fi | ||
| if FM_STATE_OVERRIDE="$sub_state" teardown_treehouse_return "$child_wt" "$child_proj" "child worktree" "" "$child_backend" "$child_t" "$child_id"; then |
There was a problem hiding this comment.
If Herdr resets during treehouse return and a foreign pane starts using the child’s worktree, a non-lock error falls through to safe_rm_rf_child_worktree without checking ownership again. That helper checks only the path and Git registration before deleting the worktree.
Run the child’s ownership preflight again before either direct-removal fallback, using sub_state, child_t, and child_id.
Knowledge Base Used: Prevent Herdr focus flashes during workspace cleanup
| FM_STATE_OVERRIDE="$state" fm_backend_herdr_endpoint_foreign "$session" "$pane_id" "fm-$task" || ownership=$? | ||
| [ "$ownership" -eq 1 ] || return 1 | ||
| break |
There was a problem hiding this comment.
After a reset, stale a.meta and a new, correctly bound b.meta can both name the recycled pane address. If a.meta sorts first, its mismatch rejects every event before b.meta is checked. The new worker loses immediate blocked alerts, and working events cannot clear the pane’s marker. Polling remains available.
Resolve the current owning record rather than letting the first stale claim disable push updates.
|
Speaking as Kun's firstmate: thanks, @cloud-practitioner. This is a careful piece of work on a real hazard. I read the thread and the diff of head What I checked: after a Herdr reset, recorded Still open (waiting on you):
Contract-class: new-default. On main, an unconfigured Herdr home trusts the recorded pane ids. With this PR, every Herdr spawn records a new process binding, and every liveness, capture, input and cleanup read applies a new ownership check that can declare an endpoint missing and drive a relaunch. That's a new default-on check across the whole Herdr path, not a restore of an already-specified one, so it won't auto-merge. Kun decides once it's otherwise ready. Safety note: the open P1 is a data-loss path (worktree removal) rather than a vulnerability. I'm noting it for Kun's awareness, and it doesn't change who this is waiting on. VISION.md, rule by rule
Next step: waiting on you for items 1 and 3, ideally with item 2, and a fresh no-mistakes raise. After that it goes to Kun, because it's new-default. |
7cea60a to
acd136a
Compare
After a Herdr server restart, or a container rebuild that does not persist Herdr's session state, the fresh session numbers workspaces and panes from w1/p1 again while surviving task records still name the old ids. A recorded id could then alias an unrelated pane, so liveness, capture, input and cleanup could act on the wrong pane instead of reporting the endpoint dead and relaunching it. Record the pane's owning shell process identity at spawn and relaunch and treat a mismatched binding as a missing endpoint rather than adopting a recycled pane id. Records without a binding fall back to a best-effort worktree and tab-label check. Apply the ownership check to liveness, recovery, capture, input and cleanup, leaving a pane proven to belong to someone else untouched. The identity is derived from /proc/<pid>/stat start ticks plus the boot id, which stay fixed for a live process. Where those are unavailable it falls back to ps start time, compared with one second of tolerance because ps derives lstart from boot time plus ticks and can drift between reads; a strict comparison would read a live agent as missing and relaunch a duplicate. Relaunch and secondmate restart refresh the recorded identity whenever they replace the pane process.
… and push transitions
…te endpoint cleanup
…ponses to direct-child and nested-child teardown fixtures; corrected concurrent Herdr recovery assertions to accept only serialized success or a documented lock refusal that preserves the endpoint; and added source directives and scoped ShellCheck annotations without runtime behavior changes. All four suites passed via bin/fm-test-run.sh: backend Herdr, endpoint boundaries, remote secondmate control, and teardown. Full extended-analysis lint passed for all five affected files; Bash syntax and git diff checks passed. Live presentation validation remains blocked because worktree-confined Herdr configuration lacks the running default session required by the lab tripwire; no default session was started or modified. Revised recovery assertions were separately exercised for both homes and failure modes. Diagnostic files were removed, leaving no untracked files
…h: retained multi-home fixtures crossed session restarts but cleanup still used stale process bindings. Cleanup now verifies those endpoints read missing, explicitly prepares legacy compatibility records, and asserts exact-pane removal; the fresh fixture retains its binding. Production ownership checks are unchanged. All three targeted suites passed via bin/fm-test-run.sh. A deterministic replay reproduced the ownership refusal and confirmed successful legacy cleanup. Full extended-analysis ShellCheck, Bash syntax, and git diff checks passed. Live presentation verification remains blocked by the lab tripwire requiring a running default session under worktree-confined configuration; no default session was modified. Scratch files were removed, leaving no untracked files
…d push events Two gaps remained in the endpoint ownership checks. Forced cleanup of a child worktree fell back to deleting the directory outright whenever the Treehouse return failed for a reason other than a git lock, without repeating the ownership check the return itself performs. A Herdr reset between that check and the fallback could leave a foreign pane working in the directory that was then removed. Repeat the preflight before both direct-removal fallbacks and refuse when a foreign process is proven. Native push events consulted only the first record claiming the pane address, so a stale record sorting ahead of the correctly bound one rejected the event and silenced the new worker's blocked alert. Accept an event when any claiming record is bound to the live pane and reject it when none is. Regression tests cover both cases and fail without the fixes.
acd136a to
d75911a
Compare
…ests/fm-herdr-endpoint-boundaries.test.sh; bin/fm-lint.sh on that file passed with ShellCheck 0.11.0. Restored all four original lifecycle wait bounds. Ran the lifecycle e2e exactly once: the reported blocked-inheritance step passed, but a later watcher auto-relaunch timed out at its original bound. Compared relevant changes against origin/main; the lifecycle test is unchanged there, but no product-code root cause was established. ci-1 remains unresolved—not classified as a load flake. No speculative source changes or timeout increases remain. Test scratch was cleaned; git diff --check passed, and no untracked files remain
|
Known limitation A Herdr reset landing during an in-flight capture, input, or watcher delivery can still race the ownership check. |
|
Thanks for the review. Item by item against the current head:
All 19 checks pass on the current head. |
|
Speaking as Kun's firstmate: thanks, @cloud-practitioner. That's a thorough round. I re-reviewed head Earlier blockers, item by item (all cleared):
CI: all green on this head, including "PR must be raised via no-mistakes" and body compliance. The previous head failed Behavior serial 5 in Contract-class: new-default (no change). Every unconfigured Herdr home now records a pane process binding at spawn and relaunch, and every liveness, capture, input and cleanup read applies a new ownership check that can declare an endpoint missing and drive a relaunch. That's a new default-on check, not the restore of a specified path, so it won't auto-merge. What Kun will weigh: whether to take a default-on ownership gate on the whole Herdr path now, with the known in-flight race left for follow-up. Also whether the best-effort label/cwd fallback for unbound records is enough on upgrade. VISION.md, rule by rule
Next step: this is otherwise ready. It now waits on Kun's merge-or-leave decision, not on you. Nothing more is needed from you unless he asks. |
|
Speaking as Kun's firstmate: thanks again, @cloud-practitioner. This fix closes a real hazard, and the last round was careful work. One ask, only if you have the time. Your Test section says the live scenarios weren't driven against a real Herdr session. Could you run them on the current head
You can post the evidence here, or re-raise through no-mistakes so the live verdict is bound to this head. If you happen to hit the reset-during-in-flight-capture race you already noted, a quick note on it would help too. Thank you! |
|
Thanks for the nudge. I drove the six scenarios against a real Herdr 0.9.3 session on head How a reset was produced. Herdr 0.9.3 restores its saved layout (and ids) across a plain server restart, so that alone does not recycle ids. To get the real condition, I spawned tasks, then stopped and deleted the session and provisioned a fresh one under the same name. Ids restarted from
Not driven. Real agent harnesses: the panes ran an agent-named stub process registered through In-flight race. I did not hit the reset-during-in-flight-capture race: every reset here happened between operations, and I made no attempt to time one inside a capture, so that window remains as described in the known-limitation note. No code changes were made for this; the scenarios exercise the head as it stands. |
|
Speaking as Kun's firstmate: thank you, @cloud-practitioner. That's exactly the evidence I asked for, and more careful than I expected. Building a real id recycle (deleting and re-provisioning the session under the same name), and confirming Live validation ask: answered. Head State now: CI is all green on this head, including "PR must be raised via no-mistakes" and body compliance. The attestation matches. GitHub reports MERGEABLE/CLEAN against main Contract-class: new-default (unchanged). Every unconfigured Herdr home now records a pane process binding at spawn and relaunch. Every liveness, capture, input and cleanup read applies an ownership check that can declare an endpoint missing and drive a relaunch. That's a new default-on gate, so it doesn't auto-merge. VISION.md, rule by rule
Next step: this is now waiting on Kun for a merge-or-leave decision. That's on him, not you. Thank you again. |
2 participants
Refs #6167: this identity-checks bindings and reads a pane that belongs to someone else as missing, but it adds no explicit refusal keyed on the supervisor's own pane id and does not address the briefly-dead just-spawned endpoint.
Intent
Reject stale Herdr endpoints after a Herdr session reset, using a stable pane process identity, as one pull request.
Reproduction: after a Herdr server restart, or a container rebuild that does not persist Herdr's session state, Herdr starts a fresh session whose workspace and pane ids restart from the beginning (w1, p1, ...), while surviving firstmate task records (the
window=,herdr_pane_id=,herdr_workspace_id=andherdr_tab_id=fields ofstate/*.meta, in the main home and in second-mate homes) still name the old ids.A recorded id can then alias an unrelated pane in the new session, so liveness, capture, input and cleanup can act on the wrong pane instead of reporting the endpoint dead and relaunching it.
Wanted fix:
ps -o lstartis derived from boot time plus starttime/HZ and drifts by a second between calls, so a live agent read through it would look missing and be relaunched as a duplicate. Derive the identity from a stable source (/proc/<pid>/statstart ticks plus the boot id), keep a portable fallback where those are unavailable that tolerates plus or minus one second of drift, and refresh the recorded identity whenever relaunch or restart replaces the pane process.Persisting Herdr state, other runtime backends, and worker slot leases are out of scope.
What Changed
Risk Assessment
Testing
All three targeted fixture suites completed successfully, covering recycled endpoints, identity drift and refresh, ownership boundaries, remote routing, and the new push-claimant regression. Diagnostic logs and timing data were saved in the evidence directory, but no live-product artifact was captured for the final head. No broad suite, linter, formatter, source edit, or live-fleet operation was performed.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/backends/herdr.sh:4241- The new stale-first acceptance discards the identity of the matching task before push handling. Concrete sequence: surviving a.meta and newly bound b.meta claim the recycled pane; A's status says paused, while B's agent reports blocked. This loop correctly accepts B's binding, but returns only the original pane record. bin/fm-push-transition-lib.sh:152 then selects A through window_to_task's first-match lookup, and :157–160 absorbs B's alert and commits its dedupe marker because A is paused. Thus the claimed durable fix still silently loses the live worker's blocked alert. Both changed callers need to preserve this invariant: bin/backends/herdr.sh:4352 (reconnect levels) and :4375 (streamed edges); status bookkeeping at bin/fm-push-transition-lib.sh:170 also needs the correct task. Use ownership-aware claimant resolution at the shared push-handling boundary for wait checks and status bookkeeping. Extend tests/fm-herdr-endpoint-boundaries.test.sh:812 through handle_push_transition with stale A paused and bound B blocked; reconcile the guarantee in docs/herdr-backend.md:766.🔧 Fix applied.
✅ Re-checked - no issues remain.
bin/fm-test-run.sh --jobs 1 --per-script-timeout-secs 600 --json ~/.no-mistakes/evidence/01M4B51JW2ET2V7MXFW0ZC192C/targeted-tests.json tests/fm-backend-herdr.test.sh tests/fm-herdr-endpoint-boundaries.test.sh tests/fm-remote-secondmate-control.test.shVerified the endpoint-boundaries suite exercised stale-paused/bound-blocked push handling for reconnect levels and streamed edges, including wakes, dedupe commitment, and correct-task status bookkeeping.Readdocs/herdr-backend.mdand thebin/fm-herdr-lab.shsafety contract; checked thatherdrandjqare on PATH without accessing operator configuration or fleet data.git status --short; git diff --exit-codeconfirmed the worktree remained clean with no untracked files.✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.