fix(herdr): detect socket replacement after inode reuse - #4745
fix(herdr): detect socket replacement after inode reuse#4745Yeachan-Heo wants to merge 5 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Supersession evidence: this is the sole canonical successor to #4744. It contains the external contributor’s original Current-head evidence: |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head b0f0f0e0 — merge blocked.
the identity mechanism is the right one
Detection uses lstatSync socket identity ${dev}:${ino}, with a matching directory rename event to cover inode reuse. That is a real identity rather than a pathname/mtime/existence comparison, which is the usual mistake here. Good starting point.
major 1 — two windows remain, and the connection is not bound to the checked identity
utils/herdr-pane.ts:647-670. The initial idle report is sent before the watcher is installed, and watchSocketReplacement snapshots identity before registering fs.watch. So a handoff that happens after the old server accepts the initial report but before watcher registration is missed entirely — the new socket silently becomes the baseline and never gets a re-report. That is the exact bug this PR exists to fix, surviving in its own setup path.
Second window: identity is checked with lstat, then a separately spawned herdr CLI process connects by pathname. Nothing carries the verified identity into that connection, so a swap in between is undetected.
Install the watch first and compare identities before and after registration, and bind the connection to the checked identity (or authenticate at the connection level).
major 2 — every error collapses to undefined
:396-427. All lstat errors and all non-socket objects return undefined. ENOENT, EACCES/EPERM, ENOTDIR/ELOOP, symlink or regular-file replacement, and transient absence are then indistinguishable — same retries, same eventual silent stop. An existing but unreachable stale socket also reads as healthy identity.
This is the error-distinction collapse that has been a blocking finding on several PRs in this repo. Use a discriminated result: retry only ENOENT, preserve diagnostics for permission and other errors, and fail closed when the replacement is not a socket.
trust boundary — worth stating explicitly
On detection the code spawns fresh CLI processes using the pathname from HERDR_SOCKET_PATH, and production never verifies the owner or mode of dirname(HERDR_SOCKET_PATH). Any principal with write+execute on that directory can unlink and rebind the name, and then receives the automatic state/title reassertion. The test directory is mkdtemp-private, but that says nothing about the production path. Validate a user-private directory, or authenticate the connection.
major 3 — the real-socket test does not pin the fix
test/herdr-pane.test.ts:902-950 replaces a real Unix socket while the reporter is live and exercises the production watcher, which is genuinely good integration coverage. But it never asserts that (dev, ino) was reused — when the filesystem hands out a different inode, the pre-head implementation passes too. And spawn is mocked, so it does not prove connection to the new server either.
Add an injected watcher/identity test with identical dev:ino snapshots plus a rename event, and keep the real handoff as integration coverage.
nit
:408 introduces ReturnType<typeof setTimeout>. The repo contract forbids ReturnType<>; write the concrete timer type.
coverage
Fake-watcher tests pin callback behavior properly: current-state re-report, sequence increase, title resend, disposer behavior, no-watch behavior, environment propagation.
Uncovered: replacement during watcher setup, a swap between identity check and CLI connect, ENOENT versus EACCES, invalid or symlink replacement, and an existing stale/unreachable socket. The first two are majors 1 and the third is major 2.
Reviewed by @probepark — method: detached worktree at b0f0f0e0, ordering analysis of initial report versus watcher registration and of identity snapshot versus fs.watch, error-classification sweep of the identity helper, trust analysis of the socket directory and the spawned CLI connection, per-test discrimination including inode-reuse dependence. Tests not executed.
gajae.pr-review-verdict.v1 merge-blocked sha256:08e24ac547ec33ad2bbfa20c43a7078e2ae5ec6277579d6d8e2cdfe7af49ed02 reviewer:human reviewer-id:probepark evidence:exact-head-b0f0f0e0-replacement-during-watcher-setup-is-missed-and-connection-is-not-bound-to-checked-identity
b0f0f0e to
fe13b52
Compare
|
Claim: project-all-open-pr-rebase-20260820 — I am rebasing #4745 onto current |
fe13b52 to
12911ba
Compare
|
Rebase lane completed on exact head Exact diff digest: Current-head validation: |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-review at exact head 12911ba4 — approved. Both behavioral majors are fixed.
the setup windows are closed
herdr-pane.ts:715-721 now calls watch(...) before report("idle"), and :467-509 takes beforeWatch and afterWatch identities around fs.watch registration, scheduling a reassert when the path changed or was temporarily absent between them.
That closes both halves of my first finding: a handoff during watcher installation is no longer missed, and a swap after installation is caught by the directory rename watcher so the pathname-spawned CLI connection is followed by a reassert to the replacement server.
errors are discriminated
:396-409 now returns { kind: "socket" | "absent" | "invalid" } with code === "ENOENT" ? { kind: "absent" } : { kind: "invalid", ... }. Only absent retries (:447-452); permission errors, symlinks and non-sockets are rejected and logged. ENOENT, EACCES and a symlink replacement are now three different outcomes instead of one silent undefined.
the trust boundary got tightened beyond what I asked
:412-428 additionally requires the socket directory to be owned, non-symlink, and not group- or world-writable. I raised the directory-ownership concern as context rather than a required change, so this is more than the finding asked for and it is the right call.
nit fixed
:432 uses NodeJS.Timeout | undefined rather than ReturnType<typeof setTimeout>.
minor — the inode-reuse test still does not pin inode reuse
test/herdr-pane.test.ts:916-963, named "detects ... even when Linux reuses its inode", never captures or compares (dev, ino). When the filesystem hands out a fresh inode the old implementation passes too.
Not blocking, because I checked the implementation rather than trusting the test: :479-487 sets replacementPending = true on a matching rename, and :457-461 reasserts even when current.value === seen. The same-inode branch is correct — it is just unproven. A deterministic same-identity plus rename-event test would close that.
Reviewed by @probepark — method: detached worktree at 12911ba4, ordering check of watcher installation against the initial report and of the identity snapshots around fs.watch, read of the discriminated identity result and its retry policy, directory-permission validation, and inspection of the same-inode path to decide whether the unpinned branch is actually correct. Tests not executed.
gajae.pr-review-verdict.v1 merge-approved sha256:00999c11ecbd9257a9f7d0d3c78209a4de7e770d1237041b62746a618758a728 reviewer:human reviewer-id:probepark evidence:exact-head-12911ba4-watcher-precedes-report-identity-snapshotted-both-sides-and-errors-discriminated
12911ba to
0638e8b
Compare
|
Merging. My approval at
The minor from my review stands as a follow-up, not a blocker: the inode-reuse case is verified correct in the implementation but not pinned by a test. |
|
Correction to the comment above: the merge did not go through.
The approval stands at |
0638e8b to
f7a5375
Compare
|
Fix-forward is pushed to the existing PR head without opening a duplicate PR. Exact evidence:
The three attributed semantic commits are preserved via cherry-pick provenance, the current-dev checks are green, and the deterministic injected same-identity-plus-rename regression is included. The previous approval was intentionally invalidated by the new head; please reaffirm the implementation at this exact head with an APPROVED review. Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
f7a5375 to
217062b
Compare
|
The PR is now rebased onto the current dev base so exact-head CI can validate the replacement cleanly. Current exact evidence:
The focused Herdr test, coding-agent check, native build, package build, install-method smoke, state gates, virtual integration validation, and affected-path plan all passed against this head. Please reaffirm the prior review with an APPROVED review targeting this exact commit. Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
The PR body now carries exactly one Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
Bounded blocker: all implementation and current-head verification evidence is green, but terminal integration cannot proceed until the required independent maintainer approval is recorded against exact head Current bound evidence:
No duplicate PR was opened and PR #4745 remains open under active maintainer ownership. Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
acfe8e0 to
ee34d01
Compare
|
Replacement head is pushed to the existing PR branch; no duplicate PR. Exact evidence:
What changed vs the last-reviewed implementation: nothing in the herdr files. The four attributed commits are the same implementation probepark approved at New adversarial verification performed at the identical product diff (pre-rebase head, byte-identical herdr hunks):
All attributable gates green locally (focused suite x4, package check, native build, package build, install-methods, state gates, virtual integration, state-writers gate, affected plan). The prior approval is intentionally invalidated by the new head; @probepark please reaffirm with an APPROVED review against this exact head Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
Bounded blocker: all implementation and current-head verification evidence is green; terminal integration cannot proceed until the required independent exact-head approval is recorded. Current bound evidence:
If dev advances before the approval lands, the head will be rebased onto the new tip, the digest regenerated, and reaffirmation re-requested — the prior commitment was that a content-neutral rebase is reaffirmed without re-deriving findings. No duplicate PR will be opened. Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
A Herdr server restart or `herdr update --handoff` replaces the server under live panes and starts with an empty agent registry. The reporter deduplicates against the last state it sent, so a session sitting at its prompt never reports again and stays invisible in Herdr's sidebar until it happens to change state — for an idle session, that means until the user types into it. Watch the pane's API socket and re-assert on replacement. Replacement is unlink-then-bind on the same path, so the socket's inode is the signal, and the watch is on the containing directory because the socket itself is recreated. The event usually arrives while the path has no inode at all, so an event schedules a bounded re-check instead of deciding at once. The last reported title is re-sent with the state, since the replaced server's metadata store is empty too and nothing else would ever resend it. Co-Authored-By: Claude <noreply@anthropic.com> (cherry picked from commit 741dce2324ef7d47fce4707e5a69a435ea13fc03)
Linux can reuse a Unix socket inode during an immediate handoff. Treating inode equality as continuity made the reporter miss the replacement. Confidence: high Scope-risk: narrow Reversibility: revert Tested: bun test packages/coding-agent/test/herdr-pane.test.ts x20; bun --cwd=packages/coding-agent run check (cherry picked from commit bd516bd8efeca59efecf79749c8ad2bd2ef23c47)
Install and verify the socket watcher before the initial report. Classify socket identity failures and accept only a user-private socket directory. Confidence: high Scope-risk: narrow Reversibility: revert Tested: bun test packages/coding-agent/test/herdr-pane.test.ts x20; bun --cwd=packages/coding-agent run check (cherry picked from commit cac7ef4f3ec9b9d5effea92f4f5cd92382cf8521)
The real socket handoff test cannot force Linux to reuse an inode. An injected watcher now pins the rename evidence path deterministically without changing product behavior. Confidence: high Scope-risk: narrow Reversibility: revert Tested: bun test packages/coding-agent/test/herdr-pane.test.ts
Replacement detection treated the directory rename event as the only evidence when Linux recycles the socket inode: if that single event is not delivered — fresh watchers are not guaranteed to observe events raised in their registration window, and a witness watcher in testing received events the product watcher never saw — the re-assert is lost with no trace, because the settled inode number compares equal. The socket identity now carries the inode's ctime, so a rebound path is a different identity even when its inode number was recycled, and one deferred identity re-check after registration closes the window without needing the event at all. An unchanged identity makes the re-check a no-op, so installs that observe nothing pay one lstat. Verified with a per-trial child-process racer whose replacements are confirmed on the reporter's own lstat before judging: 50/50 confirmed replacements re-asserted at randomized 0-3ms post-install offsets. Lore-id: 4745-activation-window Constraint: must not fire on unchanged identity (no spurious reports) Rejected: relying on rename events alone | single lost event = silent miss Rejected: connect-probing the socket | cannot distinguish reused inodes either Confidence: high Scope-risk: narrow Reversibility: revert Tested: bun test packages/coding-agent/test/herdr-pane.test.ts (53) Tested: bun artifacts/pr4745/ground-truth-race.ts (50/50 confirmed, 0 misses) Not-tested: macOS FSEvents delivery in the registration window Supersedes: none
ee34d01 to
0cf6687
Compare
|
Second replacement head pushed (fix-forward; still the existing PR, no duplicate). Exact evidence:
What changed since Why: continued adversarial verification traced a real fragility in the recycled-inode path. When Linux rebinds the socket path to a recycled inode number, the directory rename event is the only replacement evidence — and event delivery to a freshly registered watcher is not guaranteed (a witness watcher registered earlier received events the product watcher never did in repeated instrumented trials). One lost event = silent miss with identical settled identity. The fix makes the identity carry the inode's ctime (a recycled number is a new inode with a new ctime) and schedules one deferred identity re-check after registration; an unchanged identity no-ops it, so a quiet install pays one lstat and zero extra reports. Validation of the new head: focused suite 53/53 fresh processes (includes a new regression pinning an immediate post-install replacement); a ground-truth race harness — per-trial child-process racer, every replacement confirmed on the reporter's own lstat before the trial is judged — re-asserted 50/50 and 20/20 confirmed replacements at randomized 0-3ms post-install offsets with 0 misses; package check green. A worker-thread racing harness that initially suggested misses was itself disqualified: it produced physically impossible observations (a socket resurrecting with identical inode AND ctime after a confirmed unlink+rebind), and the disk-confirmed child harness shows no miss in the same window. @probepark the delta from your last-reviewed implementation is the single commit above (identity gains Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
@chatgpt-codex-connector — requesting an adversarial code review of exact head Review scope, in priority order:
Context for reviewers: commit — |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
Summary
Verification
bun test packages/coding-agent/test/herdr-pane.test.ts— 52 passedbun --cwd=packages/coding-agent run check— passedbun run build:native— passedbun --cwd=packages/coding-agent run build— passedbun run ci:test:install-methods— passedbun scripts/ci-gjc-state-gates.ts— passedbun test scripts/ci-virtual-integration.test.ts— passed21ed9bdea02ca6da712091b5d593374b5507ab70217062b3de87fffbd6a7a1e35cb2bc445eb33fcabase...headbinary diff digest:de3f0428e6017752e99099325a8bc01118fa20db6c64b53c53064a0cc0b7b853Risk classification
regression-riskThe prior approval is stale by construction after this current-dev rebase; the verdict remains blocked until an independent exact-head approval is reaffirmed.
gajae.pr-review-verdict.v1 merge-blocked sha256:de3f0428e6017752e99099325a8bc01118fa20db6c64b53c53064a0cc0b7b853 reviewer:human reviewer-id:probepark evidence:exact-head-217062b3-current-dev-replacement-awaiting-independent-reaffirmation