diff --git a/11-Features.md b/11-Features.md index b755c78..4389560 100644 --- a/11-Features.md +++ b/11-Features.md @@ -3886,3 +3886,57 @@ deliberately fail-closed: if `lsof` ever fails for the *primary's own* connectio refused until its next call resolves. That costs availability and it is the right trade, because the old behaviour spent it on a silent escalation instead. A new DEBUG line in `LsofPeerPidLookup` now names the previously-silent no-match case, so a refusal that does happen can be diagnosed. + +## The check that authorises deleting a worktree is taken after the worker stops + +**What.** When a member is released, fleetd re-reads whether its worktree has uncommitted work +immediately before the `git worktree remove --force`, with the pane already closed. If the tree is +dirty by then, the removal is skipped and the work is also snapshotted into `refs/wip/*`. + +**On.** Always on. It costs one extra `git status`, and only on a release that was about to delete +something — a release that already decided to preserve, and every `SHUTDOWN`, pay nothing. + +**Why it exists.** The old code read `hasUncommitted` once, while the worker was still running, and +used that one boolean after `launcher.stop` to authorise the force-delete. A worker that committed, +was released, and then wrote one more file during teardown lost that file. Worse, the same stale +boolean gated the snapshot, so both defences — CB-576's preserve and CB-578 stage C's `refs/wip` +copy — failed together. There was no third layer: pane gone, registry entry gone, directory +force-deleted. + +**One thing to know for maintenance.** The snapshot deliberately did **not** move after the stop. +`notifyReleased` fires *before* `launcher.stop` on purpose (CB-516/CB-581), so a caller blocked in a +`fleet_ask` rendezvous fails fast instead of waiting on the herdr RPC; moving the snapshot would +force that notification to move too or to carry a `snapshotRef` that was never computed. The late +snapshot's ref therefore reaches the log and not the listener — a known, accepted gap. Also note +what is **not** proven: whether herdr's `pane.close` returning means the worker process is really +dead is decided inside herdr, whose source is not in this repo. The re-check narrows the race; it is +not proof the race is gone. And the fail-toward-preserving rule in `dirtyImmediatelyBeforeRemoval`'s +`catch` is the whole point — flipping it to `return false` turns this guard into a cause of the data +loss it prevents. `releasePreservesAWorktreeWhoseLateRecheckCannotBeRead` exists because that +mutation once passed the entire suite. + +## A reply that arrives while a target is being released is requeued, not stranded + +**What.** `AmqpReplyInbox.release` now leaves a `RELEASED` tombstone in its `held` map instead of +removing the key. A delivery that lands during or after the release sees the tombstone and is +nacked with requeue, so a later owner or a connection drop can still recover it. + +**On.** Always on, wherever the AMQP inbox is used. + +**Why it exists.** `basicCancel` stops new dispatches but does not flush one already handed to the +client's consumer work pool. That delivery reached `deliverCallback`, found the key gone, and +created a brand-new map under it — one `release` had already walked past and would never read +again. The message then sat delivered-but-unacked until the whole inbox closed: never requeued, +never redelivered, and nothing peeks a released target again. A worker's real report disappeared +with no log line naming it. #298 fixed the case where the delivery was *already* held; this is the +case where it *arrives during* the release. + +**One thing to know for maintenance.** The tombstone closes the window rather than narrowing it, +and the reason is specific: `ConcurrentHashMap` serializes `compute` and `computeIfAbsent` for the +same key against each other, so the swap and a racing insert cannot interleave. That is why the +tombstone lives in the same map rather than in a separate "released" set — a second structure would +have to be kept in sync, which is the shape that keeps producing defects here. `RELEASED` is one +shared mutable map instance, so every read site must compare it by reference before touching it: +`peek`, `ack` and `deliverCallback` all do, and `own` clears a stale tombstone with the two-argument +`remove` so it can never delete a real map. Both halves of the fix are separately load-bearing — +reverting either one alone fails `AmqpReplyInboxReleaseRaceTest`.