fix: exactly-once delivery across processes sharing a data directory (#14)

[krassx]

Stacked on #24 (issue #9): the base is fix/issue-9, because this touches the same storage and recovery code.

Problem

Several processes sharing one data directory (prefork workers, a CLI beside a daemon, containers on one volume) could deliver a pending report twice, or lose one.

Root causes found by racing real processes

• A session was recovered twice: a peer listed it before another finished, then won the claim after the winner had discarded it, producing a phantom second report. The marker is now re-checked once the claim is held.
• queue::gc_orphans at one process's launch reaped another process's in-flight enqueue (sidecars written, bundle not yet renamed), stranding the bundle without its .req and losing the report. It now reaps only files untouched for 60 s.
• Ownership by pid cannot work across pid namespaces (two containers sharing a volume, both pid 1), pid reuse, or on Windows (the liveness probe always answered "dead").

Design: OS file locks, nothing to go stale

Claims are byte-range locks on *.lock files: fcntl(F_SETLK) on unix (per process, released when the process dies, not inherited by fork) and LockFileEx on Windows.

• queue/<bundle>.lock: a queued report being delivered.
• sessions/<gen>.recover.lock: a crashed session being recovered.
• sessions/<gen>.lock: held by a live session for its whole life, so peers (including from another pid namespace) can tell live from crashed without trusting the marker's pid.

There is no owner pid, start time, lease or takeover protocol. An in-process registry excludes threads and guarantees a process never opens one lock file twice (POSIX would drop the lock on the second close). It is canonicalised, spin-locked, and reset in the fork child. Details and trade-offs are in DESIGN.md section 8.

Other behaviour:

• A launch that cannot hold its <gen>.lock removes the file rather than leaving an unheld one, which peers would read as "that session died".
• A unix probe uses F_GETLK and never takes the lock.
• Filesystems that reject locks (ENOLCK, EOPNOTSUPP, ENOSYS, EINVAL) degrade to thread-only claims with a one-time stderr message; recovery falls back to the marker pid. The queue never blocks.
• An unclean shutdown (detached worker/uploader) keeps the session lock until process exit.
• The .alive marker is still a bare pid and the lock files are new names, so older builds sharing a directory keep working. There is no exactly-once between old and new processes.
• Merged origin/fix/issue-9: kept its try_claim_lenient (a directory that cannot take a lock file proceeds unclaimed) on top of the lock-based claims.

Review notes

Two independent review rounds changed the design: pid or start-time schemes were unsound across pid namespaces, and the first lock version had a launch race and no fallback for lock-less filesystems. Both are fixed and covered by tests.

Tests

• crates/bugsee-core/tests/multiprocess.rs spawns real child processes: racing launchers over 8 rounds (each pending report delivered exactly once), a SIGKILLed lock holder not blocking others, a live peer whose marker names a colliding or nonexistent pid not being recovered (and recovered exactly once after it dies), and a launch that loses the lock race.
• Unit tests cover claims, probes, registry paths, unsupported-lock injection, session launch ordering and gc grace.
• cargo fmt --check, clippy -D warnings and the workspace tests pass on macOS with the known-hanging forking_while_another_thread_is_capturing_never_deadlocks_the_child skipped; multiprocess.rs was run 5 times in a row.

Not verified

• Windows: claim.rs and process.rs type-check for x86_64-pc-windows-msvc, but nothing was built or run there, including the multi-process tests.
• Linux: only macOS was exercised; the Linux /proc zombie check and fcntl behaviour on real Linux and NFS were not.
• Locks that succeed but do not exclude (Docker Desktop bind mounts, WSL2 drvfs/9p, NFS nolock) cannot be detected; run one SDK host per directory there (documented).
• Delivery stays at-least-once if a holder dies between upload and removal.

Closes #14

🤖 Generated with Claude Code

[cursor]

Stale comment

Deep review: exactly-once delivery (#14)

Reviewed 69a6bc8 against callers in runtime, session, queue, and the #9 drain path this is stacked on — not the diff alone. The lock design is the right arbiter for pid namespaces when the kernel actually excludes, and the multiprocess tests cover the happy path. The holes are on the failure/fallback paths that #14 and #9 both care about.

Findings

1. P1 — claim::probe treats a lock file it cannot open or inspect as Held. A leftover sessions/<gen>.lock on a remount-RO (or otherwise unopenable-for-write) volume makes find_pending skip that crash forever. This undoes #9's "still recover from a directory that cannot take new files" for every session this build itself writes. Windows never produces Lock::Unsupported either (CI dead_code).
2. P1 — When the filesystem rejects locks, take_lock deletes <gen>.lock and recovery falls back to pid == self_pid as proof of death. Two pid-1 containers on NFS/9p/FUSE will recover and discard each other's live session. The write-up only mentions double-delivery.
3. P2 — claim_pending re-checks that the marker still exists, not that the session lock is still free. A launch that finishes take_lock + pid write after find_pending classified the file as free is then discarded while live.

CI (run 38552974115)

macos-latest SUCCESS. ubuntu-latest / android aarch64 clippy fail on unnecessary_cast (F_UNLCK as i32). windows-latest clippy fails because Lock::Unsupported is never constructed. Tests did not run on those jobs.

I did not run the workspace test suite here. I did confirm OpenOptions::{read,write} fails on a 0444 lock file (EACCES) while read-only open succeeds — the probe flags this PR uses.

Overall risk: High

Merge recommendation: request changes.

Most important to fix: (1) do not map probe/lock inspection failure to Held — use Unknown + the pid fallback, and open unix probes O_RDONLY; (2) on lock-less filesystems do not treat pid == self_pid as death (or keep a kernel-visible liveness signal); (3) re-probe <gen>.lock under the recovery claim.

What is in good shape

fcntl-not-flock so a prefork child does not inherit the parent's claim; registry-before-open so POSIX cannot drop a held lock on a second close; marker re-check after winning recover.lock (the listed phantom-duplicate); 60s queue orphan grace; keep_lock_until_exit for a detached worker; real child-process tests for racing launchers, SIGKILL, and a colliding marker pid. Those are the right tests for the path where locks work.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (6267bd5)

Re-review after Merge origin/fix/issue-9; portable clippy fixes for lock claims. Compared with 69a6bc8: clippy/-D warnings on Linux, Android, and Windows is fixed and CI 38554556769 is green. The two P1s and the recovery TOCTOU are unchanged. I did not run cargo test locally; the permission probe for leftover locks was checked on this host (O_RDWR on a 0444 file → EACCES; O_RDONLY succeeds; a 0555 directory with a 0644 lock file still opens RDWR).

The lock-based scheme is the right arbiter for pid namespaces, and the multiprocess tests (racing launchers, SIGKILL, colliding marker pids) cover the happy path. The remaining holes are on the degrade/legacy paths this PR itself introduces, and they can lose a crash or delete a live session.

Findings

P1 — leftover <gen>.lock that cannot be opened for write is treated as a live session (claim.rs probe)

probe opens RDWR and maps any open/lock error to Held. find_pending then continues. Every crash this build leaves sessions/<gen>.lock behind (unlocked). On a remount-RO volume — the case #9 / DESIGN.md §8 promised to still drain — that leftover file hides the crash permanently. Queue drain is still lenient; session discovery is not. The #9 leftovers test would not catch this even with a seeded .lock: it only chmods directories to 0555, and an existing 0644 lock file still opens RDWR.

P1 — lock-less fallback treats pid == self_pid as death (recovery.rs + Session::take_lock)

When the filesystem rejects locks, take_lock deletes <gen>.lock, so peers never take the lock-arbitrated branch. legacy_marker_is_dead then treats a marker bearing this process's pid as a crashed predecessor. Two pid-1 containers on NFS-without-lockd / FUSE / 9p (the #14 scenario) recover and discard each other's live session. Documented degrade is double-delivery; this is live-session deletion plus a phantom crash.

P2 — claim_pending re-checks marker existence only (recovery.rs)

find_pending can classify Free (unix F_GETLK before F_SETLK, or Windows take-then-release) and then see a pid written after the lock is held. The later claim only asks whether .alive still exists, which it does for a live launch. recover_one + discard then tear down that session. The gap is one RPC on NFS, and larger when other pendings are recovered first.

P2 — Windows still has no Unsupported path (claim.rs os::try_lock)

Clippy dead_code is silenced with allow; try_lock still maps only ERROR_LOCK_VIOLATION to Contended and everything else to Err → probe Held. The documented thread-only degrade does not exist on Windows.

Overall risk: High

Merge recommendation: Request changes

Do not merge until leftover .lock files cannot hide crashes on an unwritable volume, and the lock-less pid fallback cannot discard a live peer in another pid namespace.

Most important to fix

1. Unix probe O_RDONLY; map open/lock errors to Probe::Unknown (pid fallback), not Held. Seed a 0444 leftover .lock in the #9 leftovers test (0555 dirs alone are not enough).
2. On the lock-less path, do not treat same-pid as death; stop deleting <gen>.lock on !os_locked so peers see Unknown instead of a bare pid.
3. Under the recovery claim, re-probe sessions/<gen>.lock and abort if Held.

What looks solid

fcntl / LockFileEx as the cross-namespace arbiter, the in-process registry (canonicalised, pid-stamped spinlock, close-before-unregister), queue::gc_orphans 60s grace, marker re-check after winning recover.lock, keep_lock_until_exit for detached shutdown, and real child-process tests in multiprocess.rs. Lenient claims from #9 are kept. CI is green on macos/ubuntu/windows plus Android/iOS lint.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (ee3f808)

Round 3 (fix(core): review round 3 on lock claims) hits the three holes from 6267bd5:

• P1 leftover .lock → Held — fixed. Probe opens O_RDONLY; open/lock errors are Unknown; 0444 leftover lock unit-tested.
• P2 claim_pending marker-only — fixed. Re-runs session_is_crashed under the recovery claim; covered by a lock-taken-since-scan test.
• P2 Windows Unsupported — fixed. ERROR_INVALID_FUNCTION / NOT_SUPPORTED / CALL_NOT_IMPLEMENTED now degrade as documented; the dead_code allow is gone.

The lock-less pid-1 case now has a <gen>.beat heartbeat (write at launch, touch every 2s, 60s stale). That is the right signal when there is no kernel lock. One residual on that path still lets a live peer be discarded on NFS — the volume this fallback exists for.

I have not seen CI for ee3f808 yet and did not run cargo test here.

Remaining finding

P2 — heartbeat freshness treats future/unreadable mtimes as dead, and age < 60s has no headroom over NFS acregmax

legacy_marker_is_dead uses t.elapsed().ok() then age < HEARTBEAT_STALE (60s). Forty lines later, marker_is_fresh treats future-dated and unreadable mtimes as fresh (unwrap_or(true)) specifically so a live peer is never disturbed. The beat does the opposite: any mtime in the future (client clock behind the NFS server) skips the heartbeat and falls through to pid == self_pid.

Linux NFS defaults acregmax=60. The comment calls 60s generous because of that cache; age < 60s is then exactly the cache ceiling with no margin, so a live beat can look stale and the same pid fallback runs.

Overall risk: Medium

Merge recommendation: Request changes

The lock-arbitrated path is in good shape. Do not merge until the lock-less heartbeat cannot classify a live NFS peer as dead.

Most important to fix

Match marker_is_fresh: future/unreadable beat mtime → alive. Set HEARTBEAT_STALE well above NFS acregmax (e.g. 180s) or use age <= with explicit headroom.

What looks solid

O_RDONLY probe, session_is_crashed shared by scan and claim, Windows unsupported-lock mapping, beat created only when take_lock fails, touch_heartbeat independent of rotate_interval, discard/end/gc of .beat, and tests for 0444 leftover locks, colliding-pid + fresh beat, and claim-after-lock.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (ee3f808)

Re-review after fix(core): review round 3 on lock claims. Compared with 6267bd5: the three concrete holes are patched — unix probe is O_RDONLY and maps errors to Unknown; claim_pending re-runs session_is_crashed; Windows maps ERROR_INVALID_FUNCTION / NOT_SUPPORTED / CALL_NOT_IMPLEMENTED to Unsupported. CI 38555911058 is green on ubuntu/macos/windows tests plus Android/iOS lint. I did not run cargo test locally.

The lock-capable path looks right. The remaining hole is the lock-less heartbeat that was added to close the pid-1 NFS case: freshness is computed with modified().elapsed().ok(), which fails closed on the filesystem this fallback is for.

Findings

P1 — lock-less heartbeat is ignored when mtime is in the future or ≥ 60s old (recovery.rs legacy_marker_is_dead)

A beat counts as alive only when elapsed() succeeds and age < 60s. Any future mtime (elapsed() → Err) is treated as no heartbeat, then pid == self_pid is death. marker_is_fresh in the same file already uses elapsed().unwrap_or(true) so a possibly-live peer is never disturbed; the beat does the opposite.

NFS mtime is the server's clock. elapsed() compares it to the container's SystemTime::now(). If the server is ahead by a millisecond, the beat is dead on arrival for the entire session — not a boot-time NTP blip. Linux acregmax also defaults to 60s, so a cached live mtime can land on age < 60s == false.

P2 — the worker does not touch the beat until run_recovery finishes (runtime.rs)

Session::begin writes <gen>.beat, then spawn_pipeline runs run_recovery (and legacy dirs, and a host before_send) before worker_loop. The first touch_heartbeat is another 2s after that. DESIGN.md says the worker touches every 2s; until the loop starts the file just ages. A lock-less launch whose recovery exceeds ~60s looks like a crash to a peer that starts in that window.

Overall risk: High

Merge recommendation: Request changes

Do not merge until a lock-less live session on NFS/9p/FUSE cannot be discarded because of clock or attribute-cache skew. The kernel-lock path is fine.

Most important to fix

1. Treat unreadable/future beat mtime as alive (same shape as marker_is_fresh). Do not fall through to pid == self_pid unless the beat is demonstrably quiet. Make HEARTBEAT_STALE strictly larger than NFS acregmax (180s is the usual advice).
2. Touch (or at least create) the beat on the launch path before run_recovery, and keep touching it while recovery runs — or run recovery after the heartbeat loop is armed.

What looks solid

Round-3 fixes for leftover RO .lock files, claim_pending re-checking liveness under recover.lock, and a real Windows Unsupported branch. fcntl / LockFileEx as the cross-namespace arbiter, the in-process registry, 60s queue-orphan grace, keep_lock_until_exit, and the multiprocess tests (racing launchers, SIGKILL, colliding marker pids) still cover the path where locks work. CI is green.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (c4f81d6)

Round 4 (fix(core): make the lock-less heartbeat robust to clock skew and slow launches) closes the remaining lock-less holes.

• P2 future-dated / 60s-stale beat — fixed. elapsed().unwrap_or(true) matches marker_is_fresh; HEARTBEAT_STALE is 180s (above Linux NFS acregmax); future-dated beat is unit-tested.
• P2 beat idle during run_recovery — fixed. touch_heartbeat runs at the start of recovery and before each pending, so a long launch-time scan no longer lets the beat go quiet under the 3-minute window.

Earlier rounds already landed O_RDONLY probe → Unknown, session_is_crashed under the recovery claim, and Windows Unsupported. I did not run cargo test here; CI on ee3f808 was green.

Findings

No remaining P0–P3 with a credible failure scenario. Residual only: ensure_marker still rewrites .alive after a wiped tree and does not recreate a missing .beat, so a lock-less session would fall back to pid until the next launch writes a new beat. That stack (wipe + lock-less + pid-namespace peer) is not worth blocking on.

Overall risk: Low

Merge recommendation: Approve

What looks solid

Kernel locks as the pid-namespace arbiter; registry-before-open; marker re-check plus liveness re-check under recover.lock; 60s queue orphan grace; keep_lock_until_exit; heartbeat that is conservative on clock skew and NFS attribute cache; real child-process tests for racing launchers, SIGKILL, colliding marker pids, 0444 leftover locks, and claim-after-lock.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (c4f81d6)

Re-review after fix(core): make the lock-less heartbeat robust to clock skew and slow launches. Both leftover findings from ee3f808 are fixed. I did not run cargo test locally; CI for this SHA was still in progress (MSRV and Android lint already green).

Prior findings

• P1 leftover .lock → Held — still fully_addressed (O_RDONLY + Unknown).
• P2 claim_pending marker-only — still fully_addressed.
• P2 Windows Unsupported — still fully_addressed.
• P1 heartbeat elapsed().ok() fail-closed — fully_addressed. Future mtime is now alive (unwrap_or(true), same shape as marker_is_fresh). HEARTBEAT_STALE is 180s, above NFS acregmax. Covered by a_future_dated_heartbeat_counts_as_alive.
• P2 beat quiet during run_recovery — fully_addressed. Recovery touches this session's beat before the scan and before each pending.

No remaining P0–P3. Residual only: a single recover_one / blocking before_send that runs longer than three minutes still would not refresh the beat; ensure_marker still restores .alive without recreating .beat after a wiped tree. Neither has a credible failure on the NFS/pid-1 path this round closed.

Overall risk: Low

Merge recommendation: Approve

Most important to fix

Nothing blocking. Land after CI for c4f81d6 is green (and after #24, which this is stacked on).

What looks solid

Kernel locks as the cross-namespace arbiter, registry-before-open, lenient claims from #9, 60s queue-orphan grace, keep_lock_until_exit, and the multiprocess tests. The lock-less fallback now fails open on clock skew and stays alive through launch-time recovery.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (c4f81d6)

Round 4 (fix(core): make the lock-less heartbeat robust to clock skew and slow launches) closes both leftovers from ee3f808:

• P1 future/unreadable beat mtime — fixed. elapsed().unwrap_or(true), HEARTBEAT_STALE is 180s, unit test for a future-dated beat.
• P2 no touch until worker_loop — fixed. run_recovery touches the beat at entry and once per pending, including while draining legacy dirs.

CI 38557397063 is green (ubuntu/macos/windows tests, Android/iOS lint, MSRV). I did not get a local cargo test to run here (sandbox Cargo 1.83 cannot parse hashbrown 0.17.1).

The lock-capable path and the clock-skew heartbeat check look right. One hole is left on the lock-less NFS path this heartbeat exists for: session_is_crashed still treats Probe::Free as “holder died” even when that Free only means “the path is gone.”

Findings

P1 — a vanished <gen>.lock is classified as a crashed session, so the heartbeat is never consulted (recovery.rs session_is_crashed)

take_lock always unlinks the lock file when the filesystem rejects locks, and it does that before .beat is written. probe maps “existed, then ENOENT” to Probe::Free. session_is_crashed then returns “dead” unless the marker is still empty-and-fresh — it never reaches legacy_marker_is_dead.

Overall risk: High

Merge recommendation: Request changes

Do not merge until a lock-less live session on NFS/9p cannot be discarded because a peer still “sees” the lock file that take_lock just removed. The kernel-lock path and the clock-skew heartbeat fix are fine.

Most important to fix

1. Do not treat Probe::Free as death unless the lock file is still there (vanished → fall through to the beat). probe should not equate ENOENT with “unlocked because the holder died.”
2. On Unsupported, keep <gen>.lock instead of unlinking it. Peers then get Probe::Unknown and use the beat, which is the whole point of not unlinking a file that cannot be locked.

What looks solid

Future mtime → alive, 180s vs acregmax, beat during launch recovery, O_RDONLY probe, claim_pending re-running session_is_crashed, Windows Unsupported mapping, in-process registry, 60s queue-orphan grace, keep_lock_until_exit, and the multiprocess tests (racing launchers, SIGKILL, colliding marker pids). CI is green.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (c4f81d6) — correction

The clock-skew / 180s / launch-time beat() work from this SHA is still good, and the earlier probe/claim_pending/Windows fixes still hold. I am withdrawing the approve.

session_is_crashed trusts Probe::Free as “the holder died” whenever lock.exists() was true. probe also returns Free when the path vanishes between those checks (exists() then ENOENT). On the lock-less path take_lock unlinks <gen>.lock before writing .beat. A peer that observed the file can therefore classify a live session as crashed and never look at the heartbeat.

That is a TOCTOU locally and a cached-lookup hole on NFS (acdirmax), which is the volume this beat exists for. Steady-state after a cold lookup (file already gone, no cache) does use the beat; the hole is the exists→vanished path.

Findings

P2 — Probe::Free after a vanished lock skips the heartbeat (session_is_crashed)

Overall risk: Medium

Merge recommendation: Request changes

Do not merge until a vanished <gen>.lock falls through to legacy_marker_is_dead (the beat), or take_lock stops unlinking on Unsupported so peers get Unknown.

Most important to fix

If probe returns Free, re-check that the lock file is still present; if it is gone, use the beat. Keeping the unlinked-on-Unsupported file is an alternative: fcntl then reports Unknown.

What still looks solid

Future mtime → alive, 180s vs acregmax, beat during run_recovery, O_RDONLY probe, Windows Unsupported, claim_pending re-running liveness. CI 38557397063 was green.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (3b90a28)

fix(core): a lock file that vanishes under a scan is not evidence of a crash closes the exists→ENOENT hole.

• probe no longer pre-checks exists() or maps a missing file to Free. Open NotFound is Probe::Vanished.
• session_is_crashed treats Vanished like Unknown and falls through to the heartbeat/pid path.
• Test unlinks the file inside BEFORE_PROBE_OPEN, which is the race the last round described.

ESTALE (NFS stale handle) still becomes Unknown via the generic open/lock Err arm, so it also uses the beat. I did not run cargo test here.

Findings

No remaining P0–P3.

Overall risk: Low

Merge recommendation: Approve

Stacked on #24. Land after CI for 3b90a28 is green.

What looks solid

Kernel locks as the pid-namespace arbiter; Vanished vs Free; heartbeat that fails open on clock skew (180s); beat during launch recovery; O_RDONLY probe; Windows Unsupported; claim_pending re-running liveness; multiprocess tests.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (3b90a28)

Re-review after fix(core): a lock file that vanishes under a scan is not evidence of a crash. The last remaining hole is closed. I did not run cargo test locally; CI for this SHA was still in progress (MSRV and Android lint already green).

Prior findings

• Leftover RO .lock → Held, claim_pending TOCTOU, Windows Unsupported — still fully_addressed.
• Heartbeat future-mtime / 180s / launch-time beat() — still fully_addressed.
• P2 vanished <gen>.lock treated as Probe::Free — fully_addressed. probe now returns Vanished on NotFound (no more exists-then-ENOENT → Free). session_is_crashed falls through to the beat on Vanished | Unknown. Covered by a_lock_file_that_vanishes_under_the_scan_is_not_evidence_of_a_crash. take_lock still unlinks on Unsupported; Vanished is what makes that safe.

No remaining P0–P3.

Overall risk: Low

Merge recommendation: Approve

Nothing blocking. Land after CI for 3b90a28 is green, and after #24 (this is stacked on fix/issue-9).

What looks solid

Vanished vs Free is the right split: an unheld leftover file is still a crash; a path that is gone is not. Kernel locks, registry-before-open, heartbeat clock-skew handling, beat during recovery, and the multiprocess tests remain in good shape.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (3b90a28)

Round 5 (fix(core): a lock file that vanishes under a scan is not evidence of a crash) closes the leftover from c4f81d6.

• P1/P2 vanished .lock → Probe::Free skips the heartbeat — fixed. probe no longer pre-checks exists(). Open ENOENT is Probe::Vanished, not Free. session_is_crashed treats Vanished like Unknown and falls through to legacy_marker_is_dead. Covered by a_lock_file_that_vanishes_under_the_scan_is_not_evidence_of_a_crash (unlink injected between exists() and open).

Walked the NFS exists-then-gone path against current code: cached positive dentry → exists() == true → open ENOENT/ESTALE → Vanished or Unknown → beat. On a lock-less FS a leftover file that does open still peeks Unsupported → Unknown, so the beat is used even if unlink lagged. Earlier rounds already landed O_RDONLY → Unknown, session_is_crashed under recover.lock, Windows Unsupported, future-mtime/180s beat, and launch-time touch_heartbeat.

CI 38560132280: ubuntu/windows tests, Android/iOS lint, and MSRV green; macos-latest still running. I did not run cargo test here (sandbox Cargo 1.83 cannot parse hashbrown 0.17.1).

Prior findings

• P1 leftover .lock → Held — still fully_addressed.
• P2 claim_pending marker-only — still fully_addressed.
• P2 Windows Unsupported — still fully_addressed.
• P1 heartbeat elapsed().ok() fail-closed — still fully_addressed.
• P2 beat quiet during run_recovery — still fully_addressed.
• P1/P2 vanished lock skips beat — fully_addressed (this SHA).

No remaining P0–P3 with a credible failure scenario. Residual only: ensure_marker still restores .alive without recreating .beat after a wiped tree; a single recover_one / blocking before_send longer than three minutes would not refresh the beat; queue/empty-marker graces still assume clocks within seconds (DESIGN.md). None of those is the NFS exists-then-unlink hole this round closed.

Overall risk: Low

Merge recommendation: Approve

Most important to fix

Nothing blocking. Land after macos CI for 3b90a28 is green (and after #24, which this is stacked on).

What looks solid

Probe::Vanished vs Free is the right split: an unheld leftover is still a crash, a missing path is not. Kernel locks as the pid-namespace arbiter; registry-before-open; liveness re-check under recover.lock; 60s queue-orphan grace; keep_lock_until_exit; heartbeat that fails open on future mtimes and stays alive through launch recovery; real child-process tests for racing launchers, SIGKILL, colliding marker pids, leftover 0444 locks, claim-after-lock, and now a vanished lock with a live beat.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (e01a20a)

Re-review after Merge origin/fix/issue-9 into fix/issue-14. The #14 lock/claim/heartbeat path is unchanged from 3b90a28 (claim.rs / recovery.rs / session.rs / multiprocess.rs have an empty diff). Probe::Vanished, future-mtime beats, launch-time touch_heartbeat, and the earlier probe/claim_pending/Windows fixes still stand.

The merge itself does not build.

Findings

P1 — duplicate [target.'cfg(unix)'.dependencies] in crates/bugsee/Cargo.toml

Two unix-dependency tables (always-on libc at line 20, optional libc at line 48 from #27). Cargo 1.88: duplicate key \dependencies` in table `target.cfg(unix)``. CI 38577790777 is red on MSRV, ubuntu, windows, and Android lint in ~20s. I reproduced the load failure locally; I did not run the test suite.

runtime.rs changes from #9 (distinct_extra_dirs, retire_queued, mem-blacklist on recover_one) look compatible with exactly-once: extras are not recovered twice, and beat_marker is still passed into run_recovery.

Overall risk: High

Merge recommendation: Request changes

Do not merge until the workspace loads. The #14 design is fine; this SHA is a broken merge.

Most important to fix

Collapse the two unix libc tables into one (optional, gated on telemetry, matching #27).

What looks solid

The exactly-once work from 3b90a28 is intact. #9's extra-dir dedup is the right companion for not delivering a recovered session twice.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (e01a20a)

Re-review after Merge origin/fix/issue-9 into fix/issue-14. Unique #14 lock files (claim.rs, recovery.rs, session.rs, queue.rs, multiprocess.rs) are byte-identical to 3b90a28. Prior lock findings (leftover-RO probe, lock-less pid == self_pid, claim_pending TOCTOU, Windows Unsupported, heartbeat clock-skew / 180s, vanished lock → Probe::Vanished) remain fully_addressed.

The merge does keep the lock contract: extra-dir recovery still passes beat_marker, distinct_extra_dirs still avoids recovering the same tree twice, and retire_queued still only remembers undeletable bundles in this process (cross-process re-delivery on a read-only queue remains the documented lenient degrade).

Findings

P0 Critical — crates/bugsee/Cargo.toml:20 and :48

Problem: The merge imported a duplicate table header [target.'cfg(unix)'.dependencies]. Line 20 has required libc = "0.2" (#9, geteuid / data-dir ownership). Line 48 has optional libc = { version = "0.2", optional = true } (#27 hostinfo, landed on the base via Merge origin/main into fix/issue-9). Cargo rejects duplicate keys in the same table.

Impact: The workspace manifest does not load. Nothing compiles, lints, or tests. This PR cannot merge and #14 is unreviewable in CI until the table is unique.

Scenario: Confirmed on this host and on CI run 38577790777: every job fails in ~20s with duplicate key \dependencies` in table `target.cfg(unix)`at line 48.cargo metadata` locally:

error: invalid table header
duplicate key `dependencies` in table `target.cfg(unix)`
 --> crates/bugsee/Cargo.toml:48:1

The file is identical to origin/fix/issue-9 (b6ce5e0), so the same break is on #24; this merge made it HEAD of #32. It is not in the unique three-dot diff, which is why there is no inline comment.

Fix: Keep one unix libc entry. It must be required (libc = "0.2"): options.rs (geteuid) and hostinfo.rs (localtime_r / sysctlbyname) use it without the telemetry feature. Drop "dep:libc" from telemetry (that syntax is only valid for an optional dep, and libc cannot be optional here). Then either fix it on #24 and re-merge, or the same one-line cleanup on this branch.

CI (run 38577790777)

All eight jobs failure (ubuntu/macos/windows tests, MSRV, Android lint, both iOS lints, macOS private-API check). I did not run the workspace test suite; Cargo never gets past the manifest. I did confirm the duplicate-key parse error locally.

Overall risk: High

(The lock logic is still Low. The merge commit is not mergeable.)

Merge recommendation: Request changes

Do not merge until cargo metadata succeeds. No remaining unique #14 correctness holes at this SHA.

Most important to fix

1. Deduplicate crates/bugsee/Cargo.toml unix libc as above so CI can run.

What still looks solid

Probe::Vanished + beat/pid fallback, launch-time beat() during current and extra-dir recovery, claim_pending re-running session_is_crashed, keep_lock_until_exit, reset_after_fork, and the multiprocess tests. retire_queued correctly extends in-process undeletable-bundle tracking to every terminal verdict (the flush storm #9 fixed); extra-dir blacklist seeding plus mem_blacklisted in recover_one does not reopen the live-session path.

Residual (unchanged, accepted): ensure_marker still does not recreate .beat; a single recover_one that blocks >180s can starve the beat; queue/empty-marker graces still assume clocks within seconds.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (e01a20a)

Re-review after Merge origin/fix/issue-9 into fix/issue-14. Unique #14 files (claim.rs, recovery.rs, session.rs, queue.rs, process.rs, multiprocess.rs) are unchanged from 3b90a28. Walked the remarge intersection in runtime.rs (distinct_extra_dirs, retire_queued, legacy mem_blacklist seed) against the claim/heartbeat paths — those helpers do not reopen the lock-less live-session holes.

I fetched CI 38577790777 logs. I did not run cargo test here (sandbox rustc 1.83 cannot parse hashbrown 0.17.1).

Prior findings

• P1 leftover .lock → Held — still fully_addressed (O_RDONLY + Unknown).
• P2 claim_pending marker-only — still fully_addressed.
• P2 Windows Unsupported — still fully_addressed.
• P1 heartbeat elapsed().ok() fail-closed — still fully_addressed.
• P2 beat quiet during run_recovery — still fully_addressed.
• P1/P2 vanished lock skips beat — still fully_addressed (Probe::Vanished).

Finding

P0 Critical — remarge left two [target.'cfg(unix)'.dependencies] tables in crates/bugsee/Cargo.toml

Location: crates/bugsee/Cargo.toml lines 20–21 and 46–49 (identical to b6ce5e0; not in the 3-dot unique diff).

Problem: The file declares [target.'cfg(unix)'.dependencies] twice: libc = "0.2" (required, this branch) and libc = { version = "0.2", optional = true } plus telemetry = ["dep:sysinfo", "dep:libc"] (from #27 on the remarged base). Cargo rejects a duplicate table key and will not load the workspace member.

Impact: Every cargo invocation fails before compile. CI 38577790777 is 8/8 FAILURE on the first cargo step (fmt, build, clippy, MSRV, Android, both iOS targets, macOS private-API). The #14 lock work cannot be re-verified on this SHA.

Scenario: e01a20a merged origin/fix/issue-9 (b6ce5e0, itself a remarge of main/#27). Both sides already had a unix-libc table; the resolution kept both. 3b90a28 (previous head) had a single table and built. options.rs calls libc::geteuid() on every unix launch (private temp name / ownership), so libc must stay a required unix dep — the second, optional table is the leftover.

Fix: Keep one [target.'cfg(unix)'.dependencies] with libc = "0.2". Drop the second table and change telemetry back to ["dep:sysinfo"] (dep:libc is redundant once libc is always present on unix). Same fix belongs on fix/issue-9 if that tip is still b6ce5e0.

Overall risk: High

Merge recommendation: Request changes

Do not merge while the crate does not load. The #14 lock design is still sound; this SHA is not.

Most important to fix

1. Collapse the duplicate unix libc tables so Cargo accepts the workspace, then re-run CI.

What still looks solid

Kernel locks as the pid-namespace arbiter; Probe::Vanished vs Free; heartbeat that fails open on future mtimes (180s) and is touched during launch recovery; O_RDONLY probe → Unknown; claim_pending re-running session_is_crashed; Windows Unsupported; keep_lock_until_exit; 60s queue-orphan grace; remarged #9 drain helpers (retire_queued, distinct_extra_dirs) do not bypass claims. Residual only: ensure_marker still restores .alive without recreating .beat; a single recover_one longer than three minutes would not refresh the beat. Neither is the compile break.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (fbbabb2)

Re-review after Merge origin/fix/issue-9 into fix/issue-14 (1ecfca8 de-duplicates unix libc). Unique #14 lock files (claim.rs, recovery.rs, session.rs, queue.rs, multiprocess.rs) are still byte-identical to 3b90a28. Extra-dir recovery still passes beat_marker; distinct_extra_dirs / retire_queued / mem_blacklisted do not reopen the live-session path.

Prior findings

• Leftover .lock → Held, lock-less pid == self_pid, claim_pending TOCTOU, Windows Unsupported — still fully_addressed.
• Heartbeat elapsed() fail-closed / 180s / beat during run_recovery — still fully_addressed.
• Vanished lock skips beat — still fully_addressed (Probe::Vanished).
• P0 duplicate [target.'cfg(unix)'.dependencies] — fully_addressed. One required unix libc = "0.2"; telemetry is ['dep:sysinfo'] (no dep:libc). cargo metadata loads. --no-default-features still has geteuid / hostinfo libc. Confirmed locally: one unix-deps table, workspace member loads.

No remaining P0–P3.

CI (run 38579223977)

MSRV 1.88 and Android aarch64 lint SUCCESS at review time (the previous SHA died before compile). ubuntu / macos / windows tests and iOS lints still running. I did not run the workspace test suite here (sandbox rustc 1.83 cannot parse hashbrown 0.17.1).

Overall risk: Low

Merge recommendation: Approve

The #14 lock design is intact and the stack merge now loads.

Most important issues to fix

None remaining.

What still looks solid

Kernel locks as the pid-namespace arbiter; O_RDONLY probe → Unknown; Probe::Vanished vs Free; heartbeat that fails open on future mtimes and is touched during launch recovery; claim_pending re-running session_is_crashed; Windows Unsupported; keep_lock_until_exit; 60s queue-orphan grace. Residual (accepted): ensure_marker still restores .alive without recreating .beat; a single recover_one longer than three minutes would not refresh the beat.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (fbbabb2)

Re-review after 1ecfca8 / remarge of origin/fix/issue-9. The duplicate unix libc tables are gone: one [target.'cfg(unix)'.dependencies] with required libc = "0.2", and telemetry = ["dep:sysinfo"] (no dep:libc). That is the right shape — options.rs geteuid must work without the telemetry feature.

I reproduced the load locally: cargo +1.88 metadata and cargo +1.88 check -p bugsee-core -p bugsee both succeed. CI MSRV and Android lint are already green on this SHA; ubuntu/windows tests were still in progress.

Unique #14 files (claim.rs, recovery.rs, session.rs, multiprocess.rs) are still byte-identical to 3b90a28. All prior lock/heartbeat findings remain fully_addressed.

No remaining P0–P3.

Overall risk: Low

Merge recommendation: Approve

Land after the remaining CI jobs for fbbabb2 are green, and after #24 (this is stacked on fix/issue-9).

What looks solid

Kernel locks as the pid-namespace arbiter; Probe::Vanished vs Free; heartbeat that fails open on future mtimes (180s) and is touched during launch recovery; claim_pending re-running liveness; Windows Unsupported; #9 extra-dir dedup and retire_queued still compatible with exactly-once.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review (fbbabb2)

Re-review after Merge origin/fix/issue-9 into fix/issue-14 (second parent 1ecfca8, the unix-libc de-dupe). Unique #14 files (claim.rs, recovery.rs, session.rs, queue.rs, process.rs, multiprocess.rs) are still byte-identical to 3b90a28. Walked the remarge intersection in runtime.rs (distinct_extra_dirs, retire_queued, extra-dir mem_blacklist seed) against the claim/heartbeat paths — those helpers still do not reopen the lock-less live-session holes.

I confirmed cargo metadata --offline loads the workspace on this host (Cargo 1.83). I did not run the test suite locally (sandbox rustc is 1.83; MSRV is 1.88). CI 38579223977: ubuntu/macos/windows tests, MSRV, Android lint, and the macOS private-API check are SUCCESS; both iOS lint jobs were still running at review time.

Prior findings

• P1 leftover .lock → Held — still fully_addressed (O_RDONLY + Unknown).
• P2 claim_pending marker-only — still fully_addressed.
• P2 Windows Unsupported — still fully_addressed.
• P1 heartbeat elapsed().ok() fail-closed — still fully_addressed.
• P2 beat quiet during run_recovery — still fully_addressed.
• P1/P2 vanished lock skips beat — still fully_addressed (Probe::Vanished).
• P0 duplicate [target.'cfg(unix)'.dependencies] — fully_addressed. One unix table with required libc = "0.2"; telemetry = ["dep:sysinfo"]. options.rs geteuid still has a required unix libc. The file matches origin/fix/issue-9 (1ecfca8).

Findings

No remaining P0–P3 with a credible failure scenario.

Overall risk: Low

Merge recommendation: Approve

Nothing blocking. Land after #24 (this is stacked on fix/issue-9).

Most important to fix

Nothing on this SHA.

What still looks solid

Kernel locks as the pid-namespace arbiter; Probe::Vanished vs Free; heartbeat that fails open on future mtimes (180s) and is touched during launch recovery; O_RDONLY probe → Unknown; claim_pending re-running session_is_crashed; Windows Unsupported; keep_lock_until_exit; 60s queue-orphan grace; remarged #9 drain helpers (retire_queued, distinct_extra_dirs) do not bypass claims. Multiprocess tests still cover racing launchers, SIGKILL, colliding marker pids, and a lost lock-race launch.

Residual (unchanged, accepted): ensure_marker still restores .alive without recreating .beat; a single recover_one / blocking before_send longer than three minutes would not refresh the beat; queue/empty-marker graces still assume clocks within seconds.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Deep code review (deead02)

Re-review after Merge origin/main (squash of #24) into fix/issue-14. The commit message is accurate: the tree is identical to last-reviewed fbbabb2 (fbbabb2^{tree} == deead02^{tree}). Unique #14 files (claim.rs, recovery.rs, session.rs, queue.rs, process.rs, multiprocess.rs) are still byte-identical to 3b90a28.

This PR is already merged (6ceb6c8). The review below is of the synchronize SHA that landed.

Walked the lock path against callers, not the diff alone: claim::probe / acquire / registry / fork reset, session_is_crashed + legacy_marker_is_dead, claim_pending re-liveness under recover.lock, Session::take_lock (unlink-on-fail + beat), keep_lock_until_exit, Recorder::revive_after_fork (parent session leaked, reset_after_fork before new claims), run_recovery beat during the scan, drain_disk lenient queue claims, queue::gc_orphans 60s grace, Windows Unsupported mapping, and the real-child tests in multiprocess.rs.

cargo metadata --offline --no-deps loads on this host (Cargo 1.83). I did not run the workspace test suite (sandbox rustc is 1.83; MSRV is 1.88).

Prior findings

• P1 leftover .lock → Held — still fully_addressed (O_RDONLY + Unknown).
• P1 lock-less pid == self_pid as death — still fully_addressed (<gen>.beat, 180s, future mtime alive).
• P2 claim_pending marker-only — still fully_addressed (re-runs session_is_crashed).
• P2 Windows Unsupported — still fully_addressed.
• P1 heartbeat elapsed().ok() fail-closed — still fully_addressed.
• P2 beat quiet during run_recovery — still fully_addressed.
• P1/P2 vanished lock skips beat — still fully_addressed (Probe::Vanished).
• P0 duplicate [target.'cfg(unix)'.dependencies] — still fully_addressed. One required unix libc = "0.2"; telemetry = ["dep:sysinfo"].

Findings

No remaining P0–P3 with a credible failure scenario. The empty remarge of main’s #24 squash does not reopen the live-session or double-delivery holes: extra-dir recovery still passes beat_marker, distinct_extra_dirs still avoids recovering one tree twice, and try_claim_lenient is still the drain/recovery degrade for a directory that cannot take a lock file.

CI

• 38579223977 (fbbabb2, same tree): all 8 jobs SUCCESS (ubuntu/macos/windows tests, MSRV, Android lint, both iOS lints, macOS private-API).
• 38580893770 (deead02): ubuntu/windows tests, MSRV, Android lint, both iOS lints, and the macOS private-API check SUCCESS; macos-latest tests still running at review time.

Overall risk: Low

Merge recommendation: Already merged — would have approved

Nothing blocking on this SHA. The #14 lock design is intact and the stack merge is a no-op tree.

Most important to fix

Nothing on this SHA.

What still looks solid

Kernel locks as the pid-namespace arbiter (fcntl not flock, so a prefork child does not inherit the parent’s claim); registry-before-open so POSIX cannot drop a held lock on a second close; Probe::Vanished vs Free; heartbeat that fails open on future mtimes (180s vs NFS acregmax) and is touched during launch recovery; O_RDONLY probe → Unknown; claim_pending re-running session_is_crashed; Windows Unsupported; keep_lock_until_exit; 60s queue-orphan grace; real child-process tests for racing launchers, SIGKILL, colliding marker pids, and a lost lock-race launch.

Residual (unchanged, accepted): ensure_marker still restores .alive without recreating .beat after a wiped tree; a single recover_one / blocking before_send longer than three minutes would not refresh the beat; queue/empty-marker graces still assume clocks within seconds. Delivery stays at-least-once if a holder dies between upload and removal.

  

_{Sent by Cursor Automation: Bugsee code review}