fix(herdr): make the presentation lock namespace per account - #2730
fix(herdr): make the presentation lock namespace per account#2730YogiGit wants to merge 12 commits into
Conversation
The presentation lock namespace was the fixed path /tmp/firstmate-herdr-presentation, and resolving a lock asserted that the directory is owned by the current uid at mode 700. On a machine with more than one account, whichever account created it first owned it, and every other account's ownership assertion then refused permanently: the directory is mode 700 so it cannot even be inspected, and the only remedy was a privileged removal. That refusal blocks teardown outright, so a task whose work has already landed stays recorded as in flight and keeps raising stale-endpoint alarms. It also delays cleanup unboundedly, and a delayed cleanup can act on a stale target: an observed case cleared a worktree that had since been reassigned to a different live task. Name the namespace /tmp/firstmate-herdr-presentation-<uid> so the collision is impossible by construction. The ownership and mode assertions are kept unchanged; they are correct, and they are now applied to a name no other account can claim. The directory holds only lock files, so the old path is left alone and nothing is migrated. Separate the two faults that made this refusal misleading. An unresolvable named session may clear on a later attempt; an unusable lock namespace never does. fm_backend_herdr_presentation_lock_namespace_fault classifies the second and prints the exact path, the fault, the owning uid, and the remedy, and teardown, spawn recovery, task kill, and session cleanup report it instead of blaming the session. Regression coverage: the namespace name is per account and another account's namespace is inert; a foreign-owned namespace is diagnosed as permanent and names its remedy; and teardown reports that fault rather than the session, and completes while another account's namespace exists.
…es, fix doc and test
…uncreatable namespace, unify refusals
…obe, fix refusals
…espace-resolving refusals
…account namespace evidence
Non-vacuity evidence for the tests in this changeRecording this as a comment because the commit bodies here are pipeline-generated and cannot carry it. Each new or changed guard below was proven non-vacuous by reverting what it guards, observing the named assertion fail, then restoring and observing it pass. 1. Account-id divergence in the namespace fault probe Reverting the resolve-once ordering in Note the empty id. That is the defect precisely: a transient 2. The old shared namespace is never mutated by the tests
This matters beyond hygiene: chmodding that path in place can lock a concurrently running pre-change process out of its own live lock namespace, and because releasing a lock also requires entering that namespace, such a process could not even release locks it already held. Suite results at this commit ( Not verified locally. |
kunchenguid
left a comment
There was a problem hiding this comment.
Speaking as Kun's firstmate:
VISION verdict: align. This only scopes an existing Herdr presentation lock to a per-uid namespace and separates a permanent namespace fault from a retryable session miss. That strengthens a refusal path and restores teardown when another local account claimed the old shared /tmp name.
Class: corrective. Ownership and mode-700 assertions stay. Acquisition semantics are unchanged aside from the namespace name. The documented cross-revision window is an honest trade: older worktrees still use /tmp/firstmate-herdr-presentation, this revision uses the per-uid name.
No security issue in the lock mechanics. The uid is read back from a self-created /tmp probe, an inherited env uid is discarded, and a foreign-owned name is still refused. Approving first-time fork workflow runs. Not merge-eligible until CI including no-mistakes is green.
Intent
The developer was fixing a defect in firstmate's Herdr backend where fm_backend_herdr_presentation_lock_namespace() in bin/backends/herdr.sh returned the fixed path /tmp/firstmate-herdr-presentation, which a second macOS account could create first at mode 700, permanently blocking every teardown in this home with no override, no fallback, and no remedy short of a privileged rmdir. They wanted the namespace made per-uid so a cross-account collision is impossible by construction rather than by check, the ownership and mode assertions kept but applied to a name no other account can claim, and the diagnostics separated so that "session unresolvable" is distinguished from "namespace owned by another uid and therefore permanently unusable", with the latter naming the owning uid and the actual remedy, because the old message sent supervisors into an infinite retry against a condition that could never clear and held cleanup open long enough that a teardown once ran against a reassigned worktree. Stated constraints: bin/.sh and bin/backends/.sh must pass shellcheck via bin/fm-lint.sh as the single owner of the lint definition, tests must be colocated in tests/ extending existing scripts and must exercise behaviour through an executable interface rather than asserting source bytes, other fixed shared /tmp paths in bin/ and other runtime backends must be surveyed and reported rather than folded into this task, and tracked Markdown must use one full sentence per line with plain dashes. During the no-mistakes validation run the developer accepted four gate decisions: narrow an overstated documentation claim about collision impossibility, route the two additional bin/fm-spawn.sh degradation sites through the same fault helper, delete a vacuous test case arm, and file the uncreatable-namespace case as a separate task rather than fixing it here. On test coverage they rejected building a portable non-gated spawn fixture as more machinery than the one-line change warranted, and after their proposed middle path was checked and found to drive lock contention rather than a faulted namespace, they directed adding one negative assertion per gated site asserting that no fault fragment appears under contention. They also required the handover to state the coverage shape without softening, naming that positive fault-fragment coverage at those two spawn sites is absent and tracked as fm-herdr-spawn-fault-fragment-coverage-k3, and to attribute the gate-repo git identity fix to firstmate rather than the captain.
What Changed
fm_backend_herdr_presentation_lock_namespaceinbin/backends/herdr.shnow returns/tmp/firstmate-herdr-presentation-<uid>instead of the fixed shared path, so a second local account can no longer claim the namespace first and permanently block every teardown in this home. The uid is read back from the owner of a directory the process creates under/tmpitself and memoized for the process, so neither anidonPATHnor an inherited environment variable can move one process of an account onto a different namespace than another. The existing ownership and mode-700 assertions are kept and still catch a deliberate claim of the per-uid name.fm_backend_herdr_presentation_lock_namespace_faultandfm_backend_herdr_presentation_lock_refusal_suffix, which separate a retryable "session unresolvable" refusal from a permanent namespace fault (foreign owner, symlink, non-directory, wrong mode, unreadable ownership, uncreatable, or an untrustworthy account id) and print the offending path, the owning uid, and the remedy that actually clears it. The suffix is appended at the refusal sites inbin/fm-teardown.sh,bin/fm-spawn.sh(both degradation paths),bin/fm-herdr-session-cleanup.sh, andfm_backend_herdr_kill; refusals reached after the lock was already acquired deliberately do not consult it. The teardown refusal now states that the task and its work were not touched, since consulting the diagnostic may itself create the namespace directory.tests/fm-backend-herdr.test.sh(8 cases),tests/fm-teardown.test.sh(5 cases), andtests/lib.shwith a shared fake-uid helper, all driving the behaviour through the executable interface;tests/fm-backend-herdr-presentation-e2e.test.shgained two negative assertions that no namespace fault fragment appears under ordinary lock contention. Documented the per-account namespace, the untrustworthy-id case, and the cross-revision rollout window indocs/herdr-backend.md, and recorded the fixture run indocs/verification/runtime-backends.md. Positive fault-fragment coverage at the twobin/fm-spawn.shsites remains absent and is tracked separately asfm-herdr-spawn-fault-fragment-coverage-k3; the gated presentation E2E lane fails in this environment identically on the base commit, so it stays owned by the remote Herdr CI lane.Risk Assessment
✅ Low: Every accepted finding from the prior round is verifiably resolved at the source level, the one behavioural change (resolve-once in the fault helper) narrows a diagnostic path and is covered by a non-vacuous regression test, and the remaining work is test-hygiene and documentation with no change to the lock's acquisition semantics.
Testing
I ran the two targeted non-gated suites that own this behaviour (tests/fm-backend-herdr.test.sh and tests/fm-teardown.test.sh) and both passed with the 13 new namespace cases included, then went past unit-level pass/fail and drove the real bin/fm-teardown.sh CLI end to end against a staged cross-account collision, comparing the base commit and the target commit under identical machine state. The transcript shows the defect and the fix as an operator sees them: on base, a shared /tmp/firstmate-herdr-presentation reporting a foreign owner makes teardown exit 1 with "nothing was changed - rerun teardown once the session is reachable and unambiguous" while the task stays recorded in flight, which is the unbounded retry the intent describes; on target, the same state resolves /tmp/firstmate-herdr-presentation-501 instead, teardown completes, the pane closes under its lock, and the meta is retired. I also captured the two diagnostics staying apart - a genuinely foreign-owned per-uid namespace names the owning uid, says no later attempt will clear it, and gives the remove-the-directory remedy, while an unresolvable session still yields only the retryable wording with no namespace fragment. The real-Herdr-gated presentation e2e could not contribute evidence here: it fails at "treehouse get did not enter a worktree within 60s" before reaching any lock code, and reproduces identically from a clean base-commit checkout, so I verified it is pre-existing rather than caused by this change and left that lane to remote CI. Worktree is clean and all transient /tmp staging was removed.
Evidence: fm-teardown CLI transcript: base vs target under a cross-account lock namespace collision
SCENARIO 1 - another macOS account claimed the shared lock namespace first /tmp/firstmate-herdr-presentation reports owner uid 947161; this account is uid 501. --- BEFORE: base revision 1cb900c --- $ bin/fm-teardown.sh task-x1 --force error: herdr session presentation lock could not be resolved for task-x1; nothing was changed - rerun teardown once the session is reachable and unambiguous [exit status: 1] [herdr pane closed under the lock: no] [task still recorded in flight (state/task-x1.meta present): yes] --- AFTER: target revision, identical machine state --- $ bin/fm-teardown.sh task-x1 --force teardown task-x1 complete (window default:wG:pQ, worktree .../herdr-legacy-claimed-after/wt) [exit status: 0] [herdr pane closed under the lock: yes] [task still recorded in flight (state/task-x1.meta present): no] [lock namespace this account actually resolved:] /tmp/firstmate-herdr-presentation-501 SCENARIO 2 - target revision, this account's OWN per-uid namespace is deliberately owned by another account (mode 700) $ bin/fm-teardown.sh task-x1 --force error: herdr session presentation lock could not be resolved for task-x1; the task and its work were not touched - the presentation lock namespace /tmp/firstmate-herdr-presentation-947162 is owned by uid 947163 but this account is uid 947162, so its ownership check can never pass and no later attempt will clear it; have uid 947163 or an administrator remove that directory (it holds only lock files) and rerun [exit status: 1] [herdr pane closed under the lock: no] [task still recorded in flight (state/task-x1.meta present): yes] SCENARIO 3 - target revision, usable namespace but the named session cannot be resolved $ bin/fm-teardown.sh task-x1 --force error: herdr session presentation lock could not be resolved for task-x1; the task and its work were not touched - rerun teardown once the session is reachable and unambiguous [exit status: 1]Evidence: Evidence driver that produced the transcript (real CLI, base vs target)
Evidence: Targeted suite results
Evidence: Real-Herdr-gated e2e: same failure on target and on base (pre-existing)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/backends/herdr.sh:794-fm_backend_herdr_presentation_lock_namespace_faultswallows the resolve failure with|| trueat line 794 and readsexpected_uidfrom the parent frame (795), butdirat 797 comes from$(fm_backend_herdr_presentation_lock_namespace), a subshell that runs an entirely independent second probe. The two can disagree. Fix: replace... || truewith an earlyif ! fm_backend_herdr_presentation_lock_self_uid_resolve; then printf '<the id could not be read back ...>'; return 0; fi. Thendiris built from the same memoized value thatexpected_uidholds and cannot diverge, and the now-dead[ -z "$dir" ]branch collapses into it.bin/backends/herdr.sh:742- The block comment onfm_backend_herdr_presentation_lock_namespacestill asserts that per-uid naming "removes the collision by construction" on "a name no other account can claim" - the exact claim the accompanying doc change deliberately walked back at docs/herdr-backend.md:110 ("A second local account can still deliberately create that name"). /tmp is world-writable with the sticky bit, so any local account canmkdir /tmp/firstmate-herdr-presentation-<victim-uid>first; only accidental collision is removed, not deliberate claiming. This comment sits directly above the ownership and mode assertions it justifies, so a future maintainer reading it could reasonably conclude those assertions are now redundant and drop them.bin/backends/herdr.sh:806- The fault probe'smkdir -m 700 "$dir"means consulting the diagnostic can create the namespace directory. bin/fm-teardown.sh:2121 was reworded for this ("the task and its work were not touched" instead of "nothing was changed"), but the four warning-only sites that also call the suffix - bin/fm-spawn.sh:722 (inside the EXIT trapspawn_abort_cleanup), bin/fm-spawn.sh:2017, bin/fm-herdr-session-cleanup.sh:224 ("skipped"), and bin/backends/herdr.sh:3115 - previously touched no filesystem state on that path and now may. Noting as an accepted tradeoff, not a defect: the created entry is a mode-700 directory under /tmp owned by the running account, identical to what the next successful lock resolution would create anyway, and the herdr.sh block comment documents it explicitly.tests/fm-backend-herdr.test.sh:2684- The new cases stage fixed machine-global paths (/tmp/firstmate-herdr-presentation-424241/424242/424245/424251here;-424247/424253andchmod 000 /tmp/firstmate-herdr-presentationin tests/fm-teardown.test.sh:1550,1595,1739) rather than a TMPDIR-private root, which is what the isolation contract in bin/fm-test-isolation-proof.sh otherwise requires of tests/fm-backend-herdr.test.sh as a proven-isolated parallel candidate. The staged uids are disjoint per file, so no two scripts in the shard set collide today, and the /tmp dependency is unavoidable because the production namespace lives there. Residual exposure is narrow: two concurrent runs of the same script on one host would destroy each other's staging (rmdir "$ns"at case entry, thenmkdir -m 500 "$ns" || fail), and killing tests/fm-teardown.test.sh:1739 mid-case leaves the now-unused legacy path at mode 000 with no trap-based restore.🔧 Fix: resolve lock uid once in fault, dedupe test stat shim
1 info still open:
tests/fm-backend-herdr.test.sh:2620-mkdir -m 700 "$staged" 2>/dev/null || chmod 700 "$staged" || failkeeps an already-exists fallback that is now unreachable:$path_a/$path_bare derived fromfm_test_fake_uid, so the name is unique to this process and cannot pre-exist from another run. It is also the last in-placechmodof a machine-global /tmp namespace name left in the suite - the same shape that was removed from tests/fm-teardown.test.sh this round. It is safe as written (a genuinely foreign directory would make thechmodfail and reachfail), but a plainmkdir -m 700 "$staged" || failsays what the test actually requires and drops the silent-adopt path.tests/fm-backend-herdr-presentation-e2e.test.sh:715- tests/fm-backend-herdr-presentation-e2e.test.sh (real-Herdr-gated lane) fails in this environment at its first case witherror: treehouse get did not enter a worktree within 60s, before any presentation-lock code executes. I confirmed it is pre-existing by running the same file from a clean checkout of the base commit 1cb900c, which fails identically in the same 76s. The change is not implicated, but it means the two negative assertions this change added there (! grep -F "presentation lock namespace"under lock contention, at the two bin/fm-spawn.sh degradation sites) were not exercised locally and remain owned by the required remote Herdr CI lane. Note this is the same file that, per the accepted handover, carries no positive fault-fragment coverage for those two spawn sites (tracked as fm-herdr-spawn-fault-fragment-coverage-k3).bash bin/fm-test-run.sh tests/fm-backend-herdr.test.sh- passed (exit 0), covering the 8 new cases:test_presentation_lock_namespace_is_per_account,test_presentation_lock_namespace_foreign_owner_is_diagnosed,test_presentation_lock_namespace_untrustworthy_account_id_is_diagnosed,test_presentation_lock_namespace_fault_survives_a_transient_probe_failure,test_presentation_lock_namespace_ignores_an_inherited_account_id,test_presentation_lock_namespace_uncreatable_is_diagnosed,test_presentation_lock_refusal_suffix_separates_the_two_faults,test_presentation_lock_namespace_usable_reports_no_faultbash bin/fm-test-run.sh tests/fm-teardown.test.sh- passed (exit 0), covering the 5 new cases:test_herdr_teardown_diagnoses_a_foreign_owned_lock_namespace,test_herdr_teardown_diagnoses_an_untrustworthy_account_id,test_herdr_teardown_diagnoses_an_uncreatable_lock_namespace,test_herdr_teardown_ignores_an_inherited_account_id,test_herdr_teardown_ignores_another_accounts_lock_namespaceManual end-user E2E: drove the realbin/fm-teardown.sh task-x1 --forceCLI through three scenarios on the target revision and, for the collision case, on a cleangit archivecheckout of base commit 1cb900c, capturing stdout/stderr, exit status, whether the herdr pane was closed under the lock, and whetherstate/task-x1.metasurvived (driver:/var/folders/t0/l7l3qdgn6p51q2rbn48sv30w0000gn/T/no-mistakes-evidence/01M0H75QKS1SQ4G7X3PV0BJ5SK/drive-lock-namespace-e2e.sh)Manual check of the resolved namespace through the executable interface:bash -c '. "$0/bin/backends/herdr.sh"; fm_backend_herdr_presentation_lock_namespace' "$PWD"->/tmp/firstmate-herdr-presentation-501bash bin/fm-test-run.sh tests/fm-backend-herdr-presentation-e2e.test.sh- failed at its first case in this environment; re-run from a base-commit checkout to confirm the failure is pre-existing and identicalPost-run hygiene:git status --porcelainclean, temporary base checkout removed, no fake-uid/tmp/firstmate-herdr-presentation-*or/tmp/firstmate-herdr-lock-uid.*residue left behinddocs/herdr-backend.md:113- docs/herdr-backend.md:113-117 documents a rollout window in which two worktrees of one account on differing revisions resolve different lock namespaces and both order at once. It is load-bearing operator guidance today, but it is written in transitional terms ("the older revision", "this revision") and becomes wrong once no worktree runs the pre-change name, with no recorded removal trigger. Left in place as in-scope guidance for this change; worth a follow-up to delete it once the old unsuffixed namespace is no longer reachable in any deployed worktree.✅ **Push** - passed
✅ No issues found.