fix(models): hold an exclusive OS lock on the destination claim for the transfer - #856
fix(models): hold an exclusive OS lock on the destination claim for the transfer#856mattmillerai wants to merge 1 commit into
Conversation
…he transfer Ownership of a background download's destination was re-derived on every access: a submitter read the claim file's `download_id`, resolved it to a state record, and asked whether that record looked live. Deriving left three windows open. * `release_claim` compared the recorded owner and then unlinked, so a claim that changed hands between the two steps was deleted out from under its new owner. * `_claim_holder` read a record to judge liveness, so both a record that reads terminal while its worker is demonstrably still running (`download-cancel` writes `cancelled` even when `stop_worker` fails) and a transient failure to read that record at all made a live claim look stale — and the destination was stolen from a worker mid-transfer. * Clearing a stale claim and re-creating it were two separate steps, so the steal needed a once-only retry that another submitter could interleave with. Ownership is now *held*: the process performing the transfer takes an exclusive OS lock (`flock` on POSIX, `msvcrt.locking` on Windows) on the claim file and keeps it for the whole transfer. A held lock is the liveness proof, so no record has to be consulted and no liveness has to be guessed. A SIGKILLed worker's lock is dropped by the kernel, so the next submitter acquires immediately — no reconcile, no grace wait, no sweeper. New `ClaimLock` primitive in `download_state`: `lock_claim` opens `O_CREAT` (no `O_EXCL` — the file is a lock target that outlives every holder), locks non-blocking, then re-stats `fstat(fd)` against `stat(path)` so a lock taken on an inode that was unlinked underneath us is discarded and retried. The payload is read and rewritten in place on the locked descriptor, never via temp+rename, because a rename swaps the inode the lock lives on. `release()` closes the fd and deliberately does not unlink: a claim is ~100 bytes keyed by destination hash, and unlinking under a lock is how two holders end up guarding two different inodes for one destination. `prune` bounds the directory instead, unlinking only claims it can both lock and prove dead (POSIX only — Windows has no reliable inode identity, so it never unlinks). The submit path takes the lock non-blocking, arbitrates, stamps its id and releases before spawning; the worker re-takes it with a short blocking window and holds it for the transfer. The gap between those two is the one thing a bare lock cannot arbitrate, so the payload closes it: a payload naming an active record younger than `STARTUP_GRACE_S` is a submit whose worker has not booted yet and the destination is its own. `acquire_claim`, `release_claim` and `_claim_holder` are removed with no callers left, and `model_download_claim_unclearable` goes with the clear-then-retry loop that was its only source. The `OSError` degradation to `_enforce_claim` on a filesystem that cannot lock is kept, as is `_enforce_claim` itself — it remains the foreground path's only guard.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughBackground downloads now use persistent OS locks for destination claims. Workers retain ownership during transfers, update claim payloads in place, handle spawn gaps and lock failures, and preserve claim files for later sweeping. Tests cover cross-process arbitration, lifecycle failures, platform behavior, and cleanup. ChangesDestination claim locking
Sequence Diagram(s)sequenceDiagram
participant BackgroundSubmitter
participant ClaimLock
participant DownloadWorker
participant Destination
BackgroundSubmitter->>ClaimLock: acquire and stamp destination claim
BackgroundSubmitter->>DownloadWorker: spawn worker
DownloadWorker->>ClaimLock: reacquire claim lock
DownloadWorker->>ClaimLock: retain lock during transfer
DownloadWorker->>Destination: write downloaded model
DownloadWorker->>ClaimLock: clear or update payload
DownloadWorker->>ClaimLock: release descriptor
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Background downloads now retain OS-level destination locks throughout transfers, with covered contention and cleanup behavior. No actionable merge-blocking risk is currently identified. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 5 |
| 🟢 Low | 4 |
Panel: 5/6 reviewers contributed findings.
Reviewers that did not contribute: gpt-5.6-sol-max:edge-case (error)
| _enforce_claim(state, dest) | ||
| return | ||
|
|
||
| if lock is None: |
There was a problem hiding this comment.
🟠 High — The submit path takes the claim lock with the default blocking_timeout=0 and treats any None as proof that a live owner has the destination, but non-owners now take the same lock: prune() runs at the head of every background submit (and of every comfy model downloads), and _sweep_claims locks every claim file in the workspace, holding each one across read_payload(), reconcile() (which stats dest) and worker_alive() (a psutil query). A submit to destination X can therefore be hard-refused with model_download_claim_contested because an unrelated submit — or an agent polling downloads — is sweeping X's claim at that instant. Give the submit path the same short blocking window the worker gets, and/or shorten the sweep's hold to the payload read with the unlink re-validated under a second short lock.
Raised by 3 of 6 reviewers (claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max adversarial, kimi-k3-high adversarial).
| path = Path(path) | ||
| deadline = time.monotonic() + max(0.0, float(blocking_timeout)) | ||
| for _ in range(CLAIM_LOCK_RESTAT_ATTEMPTS): | ||
| fd = os.open(str(path), os.O_CREAT | os.O_RDWR | _O_BINARY, STATE_FILE_MODE) |
There was a problem hiding this comment.
🟡 Medium — lock_claim opens the deterministic claim path with O_CREAT | O_RDWR and no O_NOFOLLOW, and never checks that the descriptor is a regular file before os.fchmod, os.ftruncate and the payload write. A symlink planted at claims/<sha>.claim therefore becomes an arbitrary-file truncate-and-overwrite, and a FIFO or device node makes read_payload's loop hang; claims_dir's 0700 chmod is best-effort and suppressed, so a pre-existing directory the user does not own stays reachable. The old os.link publish failed closed with EEXIST on anything already at the path, so this is a new capability — add O_NOFOLLOW on POSIX plus an S_ISREG(os.fstat(fd).st_mode) check before truncating.
Raised by 3 of 6 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max adversarial, kimi-k3-high adversarial).
| payload = lock.read_payload() | ||
| holder_id = payload.get("download_id") if payload is not None else None | ||
| if holder_id is not None and holder_id != state.id: | ||
| holder = _spawn_gap_holder(holder_id) |
There was a problem hiding this comment.
🟡 Medium — _spawn_gap_holder runs while the claim lock is held and is far more than the "~100 byte read and rewrite" that WORKER_CLAIM_LOCK_TIMEOUT_S is explicitly sized against: it reads another state file, runs _reconciled (stat on the destination plus a psutil query) and persists a status correction. The write_payload failure branch below is worse, calling _enforce_claim, which list_alls and reconciles the whole state directory under the same held lock. On a slow or hung destination mount this exceeds the worker's 5 s window, so the legitimately-owning worker concludes it lost its own destination and writes failed with "another download claimed this destination first"; release the lock before doing this work.
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).
| # download that never runs. Stamp it and carry on. | ||
| payload = claim_lock.read_payload() if claim_lock is not None else None | ||
| owner = payload.get("download_id") if payload is not None else None | ||
| claim_lost = claim_lock is None or (owner is not None and owner != state.id) |
There was a problem hiding this comment.
🟡 Medium — The worker treats any payload naming a different id as decisive, with no liveness or staleness check on that owner — unlike the submit side, which routes the same question through _spawn_gap_holder (active after reconcile and younger than the grace). Payloads are never blanked on release and never swept on Windows, and a submitter that degraded past write_payload never re-stamps, so a payload naming a long-dead download is a normal state: the worker reads it, marks its own record failed with "another download claimed this destination first", and a download that should have run never does. Mirror _spawn_gap_holder's liveness test here so the worker adopts the destination instead of refusing.
Raised by 2 of 6 reviewers (kimi-k3-high edge-case, kimi-k3-high adversarial).
| # means no lock was taken and the caller has to degrade rather than conclude | ||
| # somebody else owns the destination. | ||
| _LOCK_CONTENDED_ERRNOS = frozenset( | ||
| getattr(errno, name) for name in ("EACCES", "EAGAIN", "EWOULDBLOCK", "EDEADLK", "EDEADLOCK") if hasattr(errno, name) |
There was a problem hiding this comment.
🟡 Medium — _LOCK_CONTENDED_ERRNOS includes EACCES unconditionally, but EACCES is only the contention signal for msvcrt.locking on Windows — POSIX flock reports contention as EWOULDBLOCK/EAGAIN. A genuine EACCES (a mount or LSM that denies the lock outright) is therefore silently reported as "somebody else holds this", so _acquire_dest_claim takes the lock is None branch and refuses every submit to that destination with model_download_claim_contested forever, rather than degrading to the advisory guard with a warning the way ENOLCK does. Gate EACCES and EDEADLOCK behind sys.platform == "win32".
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, kimi-k3-high adversarial).
| if holder is not None: | ||
| return _in_flight_failure(holder, dest) | ||
| named = f" ({holder_id})" if holder_id else "" | ||
| return _download_failure( |
There was a problem hiding this comment.
🟡 Medium — _refuse_held_claim resolves the payload's id to a record and unconditionally emits model_download_in_flight from it with no liveness check, but the lock and the payload are independent: a worker writes its completed record before releasing the lock, and a non-owner (_sweep_claims) can hold the lock over a payload naming a long-finished download. Both cases produce "A background download (id) is already writing to " with details.status set to completed/failed plus a hint to cancel a download that has already finished, contradicting the documented contract of that code. A terminal status on the resolved record should fall through to the model_download_claim_contested wording instead.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
| # Every attempt raced a concurrent unlink. Reporting "held" rather than | ||
| # "acquired" is the safe direction: the caller refuses instead of letting a | ||
| # second transfer into a destination whose ownership we could not settle. | ||
| return None |
There was a problem hiding this comment.
🟢 Low — Exhausting CLAIM_LOCK_RESTAT_ATTEMPTS returns None, which is indistinguishable from "held" to every caller: the submitter refuses, and _download_worker sets claim_lost and writes a terminal failed record for a download nobody actually contested. Since _sweep_claims is the only thing that unlinks claims and it runs from prune() on every submit and every downloads call, a few unlucky interleavings of internal bookkeeping can kill a legitimate transfer. Note also that deadline is computed once before the retry loop, so a caller that asked for a blocking window can reach attempt 2 or 3 with zero budget; a distinct signal (or a raised OSError, which callers already degrade on) would let callers tell "contested" from "could not settle".
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).
| os.lseek(fd, 0, os.SEEK_SET) | ||
| chunks: list[bytes] = [] | ||
| while True: | ||
| block = os.read(fd, 65536) |
There was a problem hiding this comment.
🟢 Low — read_payload slurps to EOF with an unbounded while True: os.read(fd, 65536) loop and then duplicates the result via b"".join(chunks), with no size cap on a payload that is supposed to be ~100 bytes. A corrupt or planted oversized claim can exhaust memory while the destination lock is held, wedging every submit and prune for that claim; a cheap byte cap that bails to None bounds the allocation for free.
Raised by 2 of 6 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max adversarial).
| path.unlink() | ||
| continue | ||
| if not path.name.endswith(".claim"): | ||
| if not path.name.endswith(".claim") or sys.platform == "win32": |
There was a problem hiding this comment.
🟢 Low — The or sys.platform == "win32" short-circuit makes the sweep skip every .claim file on Windows, and ClaimLock.release never unlinks one, so claims/ grows by one file per distinct destination for the life of the workspace — the same unbounded growth prune's docstring says this sweep exists to prevent — while _sweep_claims keeps materialising sorted(base.iterdir()) on every prune(), i.e. every submit and every downloads call. A Windows-safe reclaim (age-based, or moving the file aside under the lock instead of unlinking) would keep the stated bound.
Raised by 2 of 6 reviewers (gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max adversarial).
| fd, self._fd = self._fd, None | ||
| if fd is None: | ||
| return | ||
| with contextlib.suppress(OSError): |
There was a problem hiding this comment.
🟢 Low — On Windows the msvcrt.locking(fd, LK_NBLCK, 1) byte-range lock is never explicitly released — release() only closes the descriptor, and Windows documents unlock-on-close timing as depending on available system resources. The normal spawn-gap handoff (submitter releases, worker immediately re-locks) can therefore observe spurious contention, surfacing as a bogus model_download_claim_contested refusal or a worker claim_lost on a destination that is actually free. An explicit msvcrt.locking(fd, LK_UNLCK, 1) at offset 0 before os.close would make release deterministic.
Raised by 1 of 6 reviewers (kimi-k3-high edge-case).
1 participant
ELI-5
Two
comfy model download --backgroundcommands must never write the same file at once, so the first one to arrive "claims" the destination. Until now that claim was a note on disk saying downloadabc123owns this, and anyone else who came along had to look upabc123's record and guess whether it was still alive. Guessing was the problem: a worker that had been asked to cancel but had not actually died yet, or one whose record simply could not be read for a moment, both looked dead — so a second download would take the destination away from a worker that was still busy writing to it, and both would stream into the same file.Now the worker doesn't leave a note, it holds the door shut. It takes a real exclusive OS lock on the claim file and keeps it for the whole transfer. Nobody has to guess whether it is alive: if the lock is held, it is alive. And if the worker is
kill -9'd, the operating system drops the lock for it, so the next download gets the destination straight away instead of waiting out a timeout.Description
Destination ownership was derived on every access — read the claim's
download_id, resolve it to a state record, judge whether that record looks live. This makes it held: an exclusive OS file lock (flockon POSIX,msvcrt.lockingon Windows) taken on the claim file and kept for the transfer's lifetime by the process moving the bytes. A held lock is the liveness proof, so no record is consulted and nothing is inferred.Three windows that derived ownership left open, all closed:
release_claim. It compared the recorded owner and then unlinked, so a claim that changed hands between the compare and the unlink was deleted out from under its new owner.release_claimis gone; nothing unlinks on release at all._claim_holder.download-cancelwritesstate.status = "cancelled"unconditionally even whenstop_workerreturns false (models.py:1968, witherrorset to "worker may still be running; partial file left in place"), and a record that momentarily fails to read came back indistinguishable from a record that is gone. Both made a live claim read stale. That worker now still holds its lock, so the refusal stands regardless of what its record says or whether the record can be read at all.for attempt in (1, 2)loop existed. The steal now happens in one step under the lock, so the loop and itsmodel_download_claim_unclearableerror code are both deleted.The primitive
lock_claim(path, *, blocking_timeout=0) -> ClaimLock | Noneindownload_state.py:O_CREATwithoutO_EXCL— the claim file is a lock target that outlives every holder, so it usually already exists and creating it decides nothing. What decides is the lock.openand the lock, the file can be unlinked and replaced, leaving us holding a real lock on an orphan inode. After locking,os.fstat(fd)is compared againstos.stat(path); a differing(st_dev, st_ino)— or a vanished path — means we lost that race, so the fd is closed and reopened, bounded at 3 attempts. Windows skips the guard (inode identity is unreliable there) and pays for it by never unlinking a claim file at all, which removes the race rather than detecting it.Nonefor contention, and raisesOSErrorfor a filesystem that cannot lock — the two must not be confused, because only the first means somebody owns the destination.read_payload()/write_payload()work in place on the locked descriptor (lseek+ftruncate+write). Never temp+rename: a rename swaps the inode the lock lives on. Payload shape is unchanged ({download_id, dest, created_at}, still no url — fix(models): close the background download race with an atomic O_EXCL destination claim #777's credential rationale stands).release()closes the fd and does not unlink. Unlinking a file while holding its lock leaves every descriptor already opened on that path guarding an orphan inode, so two holders could each hold a genuine lock on a different inode for one destination. A claim is ~100 bytes keyed by destination hash, so the directory is bounded by distinct destinations;prunebounds it further by unlinking only claims it can both lock and prove dead (POSIX only).Submit and worker
The submitter locks non-blocking, arbitrates, stamps its id, releases, then spawns. The worker re-takes the lock with a short blocking window (5s — a competitor may hold it for the ~100 byte read/rewrite it takes to inspect), verifies the payload names it, and then holds it for the whole transfer. A payload naming a different download means it lost the destination during the spawn gap: it exits refused without touching
dest, writing its record terminal exactly as the submit-side withdraw path does.The gap between the submitter's release and the worker's lock is the one thing a bare lock cannot arbitrate, so the payload closes it (
_spawn_gap_holder): a payload naming an active record younger thanSTARTUP_GRACE_Sis a submit whose worker has not booted yet, and the destination is its own. Past the grace, a genuinely live download would be holding the lock — so having got there proves it is not, and refusing on the payload alone would resurrect the derived-ownership guess this change removes._enforce_claimand itsOSErrordegradation are both kept: it is still the foreground path's only guard.Judgment calls
_spawn_gap_holdernarrows an existing check, so per Chesterton's Fence it is called out explicitly. The old_claim_holdertreated any active-after-reconcile record as live; the new one additionally requires it to be younger thanSTARTUP_GRACE_S. The two can only disagree when the lock is not held, and under this design a live worker always holds it — so the narrowed branch is reachable only in the spawn gap, which is exactly what it exists to arbitrate. This is the shape the ticket specifies, and the reasoning is written into the function's docstring rather than only here.write_payloadonENOSPC, or a sweep that removed an unowned file our ownO_CREATthen re-made). Refusing there would turn a recoverable submit-side hiccup into a download that never runs._sweep_claimsunlinks under the lock, whichClaimLock.release()'s docstring warns against. That is deliberate and POSIX-only: it is precisely the case the re-stat guard exists to absorb, and the guard exists only on POSIX. On Windows the sweep skips.claimfiles entirely.Provenance
pytest(full suite): 7413 passed, 39 skipped, 1 failed — the single failure,tests/comfy_cli/test_http.py::test_an_unloadable_supplement_falls_through_to_the_platform_roots, reproduces identically on a cleanorigin/mainworktree (an environment-dependent CA-bundle cert count,assert 182 == 145) and is unrelated to this change;pytest tests/comfy_cli/command/test_model_download_background.py: 236 passed, 1 skipped (the skip is the Windows-onlymsvcrttest);ruff check .: all checks passed;ruff format --check .: 447 files already formatted.Residual
Not fixed here, and actionable on their own:
msvcrt/ Windows locking path is entirely unexercised — by this change and by CI. Its unit test carriesskipif(sys.platform != 'win32')exactly as specified, so it does not run on the Linux/macOS machine this was written and verified on. It will not run in CI either:.github/workflows/pytest.ymlisruns-on: ubuntu-latest, and.github/workflows/test-windows.ymlpip install pytestbut never invokes it — it is acomfy install/comfy launchsmoke test. So every Windows-specific claim in this change is reasoned, not measured: thatmsvcrt.locking(fd, LK_NBLCK, 1)afterseek(0)reports contention asEACCES; thatftruncateto zero is permitted on a byte-0 range locked by the same handle; that a locked byte range makes another process's unlockedread_bytes()fail (which the refusal path relies on to fall back to an id-less message); and that the winerror set{1, 50}is what a volume with no byte-range locking returns. Worth either running the unit suite on the Windows runner or exercising these four behaviours by hand on Windows before relying on them.comfy model downloadwrites no claim file and is guarded only by_enforce_claim's advisory re-scan, so a foreground transfer and a background worker can still both write one destination. This is the ticket's stated non-goal and belongs to the follow-on ticket that converts the foreground path.flockover NFS/SMB is advisory at best and may be a local-only lock, so two machines sharing a mount can still both win. Strictly no worse than the pid-based liveness it replaces (which was meaningless across machines), and documented as such in the module comment — but it is not fixed, and cross-machine correctness was an explicit non-goal.get_workspace(), and--relative-pathis onlyexpanduser-ed, so two invocations run from different workspaces against one destination still consult different claim directories and neither sees the other. Pre-existing, carried forward from fix(models): close the background download race with an atomic O_EXCL destination claim #777.STARTUP_GRACE_S(60s). This is the residual the startup grace has always had; the held lock does not remove it, because in that window there is no process left to hold anything.download-cancelleg directly against live source (models.py:1968) and the other two legs against the pre-changerelease_claim/_claim_holderbodies, but I cannot confirm the spike found nothing further.