mosaicstack/stack
1
0
Fork 0
Code Issues 81 Pull Requests 1 Actions Packages Projects Releases 15 Wiki Activity

fleet: migrate all fleet writers to an fd-held advisory lock (retire pathname sentinel locks; close release TOCTOU) #814

New Issue
Open
opened 2026-07-17 03:00:36 +00:00 by jason.woltje · 0 comments
jason.woltje commented 2026-07-17 03:00:36 +00:00
Owner
Copy Link

Problem

All three fleet writers — agent CRUD, reconcile, and regen (added in #813) — serialize on pathname-based sentinel locks (fleet/roster.yaml.mutation.lock, fleet/roster.yaml.reconcile.lock) created with wx and released by proving ownership (device/inode + a random token) and then unlink-ing the path.

That ownership-proof-then-unlink is a check-then-act TOCTOU (CWE-367): between the final ownership check and the unlink, an external actor — a stale-lock reaper or an operator running rm — can vacate our inode, another writer can wx-create a new lock at the same path, and our unlink then deletes that writer's live lock, admitting a third writer and breaking mutual exclusion over roster/projection mutation.

This is pre-existing — byte-identical to the reconcile-lock release already merged on main — and is unreachable within the wx writer protocol itself (no Mosaic writer removes a lock it doesn't own; only external interference can vacate the inode mid-release). #813 deliberately did not attempt to fix it, because the only correct fix is a cross-cutting mechanism change and #813 is a projection-only recovery PR. Both the independent reviews and Codex's code/security reviews of #813 agreed it is non-blocking for that PR, but it should not linger unaddressed.

Proposed fix

Migrate all fleet writers to a single fd-held advisory lock (flock/lockf or a vetted equivalent) on a stable lock file:

  • acquire the lock on an open descriptor, hold the descriptor for the entire critical section, release by closing the fd — never delete/recreate the lock file on the normal path;
  • adopt it uniformly across CRUD + reconcile + regen so there is one lock discipline, not a mix of wx sentinels and fd locks;
  • keep the non-blocking-on-contention semantics (throw rather than wait) that the current locks provide.

Acceptance

  • CRUD, reconcile, and regen all acquire/release via the shared fd-held advisory lock; no writer relies on pathname ownership-proof-then-unlink for release.
  • A synchronized regression test that replaces the lock file after a writer's critical section begins and verifies the replacement remains locked / the original writer's release cannot delete it.
  • The wx sentinel locks and the assertLockOwnership/removeOwnedLockLeafBestEffort path-based ownership machinery are removed once superseded.

References

  • Introduced/most-recently-touched in #813 (packages/mosaic/src/fleet/fleet-reconciler.ts, acquirePrivateManagedRosterLock release closure + init-cleanup).
  • Part of the #791 upgrade-safety family (follow-up, not a blocker for #791).
## Problem All three fleet writers — agent CRUD, reconcile, and regen (added in #813) — serialize on **pathname-based sentinel locks** (`fleet/roster.yaml.mutation.lock`, `fleet/roster.yaml.reconcile.lock`) created with `wx` and released by proving ownership (device/inode + a random token) and then `unlink`-ing the path. That ownership-proof-then-`unlink` is a **check-then-act TOCTOU** (CWE-367): between the final ownership check and the `unlink`, an external actor — a stale-lock reaper or an operator running `rm` — can vacate our inode, another writer can `wx`-create a new lock at the same path, and our `unlink` then deletes *that* writer's live lock, admitting a third writer and breaking mutual exclusion over roster/projection mutation. This is **pre-existing** — byte-identical to the reconcile-lock release already merged on `main` — and is **unreachable within the `wx` writer protocol itself** (no Mosaic writer removes a lock it doesn't own; only external interference can vacate the inode mid-release). #813 deliberately did **not** attempt to fix it, because the only correct fix is a cross-cutting mechanism change and #813 is a projection-only recovery PR. Both the independent reviews and Codex's code/security reviews of #813 agreed it is non-blocking for that PR, but it should not linger unaddressed. ## Proposed fix Migrate **all** fleet writers to a single **fd-held advisory lock** (`flock`/`lockf` or a vetted equivalent) on a stable lock file: - acquire the lock on an open descriptor, hold the descriptor for the entire critical section, release by closing the fd — **never** delete/recreate the lock file on the normal path; - adopt it uniformly across CRUD + reconcile + regen so there is one lock discipline, not a mix of `wx` sentinels and fd locks; - keep the non-blocking-on-contention semantics (throw rather than wait) that the current locks provide. ## Acceptance - [ ] CRUD, reconcile, and regen all acquire/release via the shared fd-held advisory lock; no writer relies on pathname ownership-proof-then-`unlink` for release. - [ ] A synchronized regression test that replaces the lock file *after* a writer's critical section begins and verifies the replacement remains locked / the original writer's release cannot delete it. - [ ] The `wx` sentinel locks and the `assertLockOwnership`/`removeOwnedLockLeafBestEffort` path-based ownership machinery are removed once superseded. ## References - Introduced/most-recently-touched in #813 (`packages/mosaic/src/fleet/fleet-reconciler.ts`, `acquirePrivateManagedRosterLock` release closure + init-cleanup). - Part of the #791 upgrade-safety family (follow-up, not a blocker for #791).
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#814
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?