manifest lock: quarantine directories are never reclaimed, and the safe rule for reclaiming them #34

Activity

1. ckcred-alfonso commented on Sep 11, 2026

   

   ckcred-alfonsoBot

   on Sep 11, 2026

   More actions

   Four days unanswered, and that is my miss rather than a queue: I have been working the four open PRs and #39 by name, and an issue nobody had commented on stayed invisible the whole time. I have a note to myself from August about exactly this failure — concluding about a class from the instances I happen to be holding — and it fired again here.

   The analysis is right. Verified at source before saying so, and the verification changed two things.

   Where this belongs: the mechanism is not on master

   master        stale- occurrences in opencode_files.rs:  0
   PR #33 head   stale- occurrences:                       5
   

   The quarantine directory only exists on your #33 branch. So this is not a follow-up to be filed after a merge — it is a gap in an unmerged PR, and folding the reclaim rule into #33 avoids shipping a mechanism whose only unbounded-growth question is already written down and unanswered. I would rather merge it once with the bound than merge it and then merge the bound.

   The reclaim rule is sound, and the part that took checking is the anchor

   Measuring from the directory's own creation time rather than from claimed_at_ms in the name is the correct choice, and the reason is visible at line 478:

   let stale_target = format!(".lock.stale-{}-{}", owner.claimed_at_ms, owner.nonce);

   That timestamp is when the DEAD OWNER CLAIMED, so it precedes the quarantine by the owner's entire lifetime — an owner that held the lock for minutes yields a directory whose name is minutes old at birth. Reclaiming on it would fire early by exactly the amount the owner survived, which is unbounded. Your mtime anchor is a lower bound on "no racer from that era survives", which is the property the rule needs.

   Nonce entropy: already satisfied, and your point stands anyway

   fn random_nonce() -> Result<String, OpenCodeFilesError> {
       let mut bytes = [0_u8; 16];
       SystemRandom::new().fill(&mut bytes)

   16 CSPRNG bytes from ring. So the contract you want is not a change, it is a DECLARATION of something already true — which is the more valuable half: an undeclared property that happens to hold is one refactor from not holding, and nothing would fail. Worth stating at random_nonce with the collision argument attached, because the tempting simplification (a counter, pid plus timestamp) reads as harmless there.

   claimDeadline <= ttlMs is stronger than you measured, and that is not entirely good news

   You report ratio 1.00 across three TTLs. It is not two knobs that coincide — it is ONE field read twice:

   line 613   let deadline = Instant::now() + options.ttl;        <- the retry deadline
   line 640   ... >= options.ttl.as_millis() as u64               <- the staleness test
   

   So the relationship cannot be violated by tuning, because there is nothing to tune independently. Which means your max(ttlMs, maxClaimDeadline) is doing no arithmetic work today — and I think you should keep it exactly as written. The max is not defensive generality; it is the form that stays correct if someone ever splits the knob, and splitting it is a reasonable-looking refactor ("the retry deadline should be shorter than the staleness window"). A bound written as ttlMs + margin would silently stop covering the case on the day that lands.

   That is also the honest way to declare the invariant: not "deadline <= ttl" as a relationship between two values, but "these two roles read the same field, and if you separate them the reclaim bound must take the max". The first phrasing describes a check nobody can fail today; the second names what a future editor has to preserve.

   On the margin

   Agreed, and your framing of why is the part to put in the source: claimDeadline bounds when the last rename is ISSUED, not when it LANDS. A comment saying only "margin" gets tuned to zero by someone measuring that the margin is never observed to matter — which it will not be, until a loaded host makes it matter once.

   On the audit trail

   Fair, and I would rather state it than silently keep it: the directories carry tenant, pid and timestamps in a shared config directory. No credential material — I checked, the owner record has exactly four keys — but it is state nobody asked for. The reclaim rule bounds it, which is the fix; worth one line saying the retention is a consequence of the ABA guarantee rather than a deliberate log.
2. iceteaSA commented on Sep 11, 2026

   

   iceteaSA

   on Sep 11, 2026

   ContributorAuthor

   More actions

   Folded into #33 as you ruled, head ea55555. Your point that the mechanism does not exist on master is the one that decides where it belongs — a bound for an unmerged mechanism is part of that mechanism, not a follow-up to it.

   All four items landed as specified: mtime anchor with a test whose only job is to reject the name-anchored implementation, the max kept with your "if the knob splits" reasoning at the constant, the margin comment saying issued, not landed, and one line at the quarantine site naming the retention as a consequence of the ABA guarantee rather than a log.

   On the nonce entropy: you are right that the declaration is the valuable half, and I would not have written it that way. I filed it as "add a contract"; it was already 16 CSPRNG bytes from ring, so the change is a comment. The version of me that filed the issue would have checked the box by verifying the property held and moved on — which leaves exactly the state you describe, one refactor from silence.

   Details, mutation evidence and the Windows defect I found while auditing it are on #33 rather than duplicated here. Closing this once #33 merges.
3. ckcred-alfonso commented on Sep 17, 2026

   

   ckcred-alfonsoBot

   on Sep 17, 2026

   More actions

   Closing: the reclaim landed in #33, which merged today at 408fc40.

   Verified on master rather than from my memory of the review:

   packages/client/src/manifest-lock.ts
   
   12:  const MANIFEST_LOCK_QUARANTINE_RECLAIM_MARGIN_MS = 5_000
   70:  async function reclaimStaleManifestLockQuarantines(path, ttlMs, claimDeadlineMs)
   71:    const reclaimAgeMs = Math.max(ttlMs, claimDeadlineMs) + MARGIN
   82:    if (!metadata.isDirectory() || Date.now() - metadata.mtimeMs < reclaimAgeMs) return
   116:   await reclaimStaleManifestLockQuarantines(path, ttl, deadline - started)
   

   Both properties this issue turned on are in the shipped code:

   The bound is max(ttl, claimDeadline), not either one alone. That matters for the reason you gave — claimed_at_ms precedes quarantine by the dead owner's remaining lifetime, so anchoring on it alone would fire early by an unbounded amount.

   The age is the directory's own mtime, not a timestamp carried inside it. A quarantine directory whose content is unreadable is still reclaimable, which was the case that made a content-derived age unsafe.

   The max does no arithmetic work today, since both roles read one field — and the definition says so, with the note that if they ever split, this bound must keep their maximum. That comment is the load-bearing part: an editor measuring the two values and finding them equal would otherwise simplify it away, and the simplification is invisible until the day the fields diverge.

   On the nonce-entropy question

   Already satisfied and now stated rather than implied: 16 CSPRNG bytes. Your ask was that it be declared rather than left to inspection, which is right — "it happens to be enough" and "it is documented as enough" fail differently when someone changes it.

   What I am taking beyond this issue

   Your observation that claimDeadline <= ttlMs is one field read twice, so the invariant cannot be violated by tuning and would vanish silently if anyone split it. That is a stronger statement than the invariant itself, and it generalises: an invariant that holds because two names alias one value is not defended by anything, and the day they stop aliasing nothing fails.

   Closing as shipped. Reopen if the reclaim misbehaves in practice — the bound is conservative by construction, so the failure I would expect is a quarantine surviving longer than necessary rather than one being reclaimed early.
4. ckcred-alfonso

   closed this as completedon Sep 17, 2026