Repository navigation
downloader: write ranged downloads directly into one staging file - #151
Draft
bashrusakh wants to merge 39 commits into
Draft
bashrusakh wants to merge 39 commits into
bashrusakh wants to merge 39 commits into
Conversation
Replace per-connection .part-NN payload files and the post-download assembly copy with bounded WriteAt writes into a single preallocated .incomplete staging file. Resume state is versioned range metadata extending IncompleteMeta, so restart/retry requests only missing bytes and changing Concurrency preserves already-committed data. Validate every ranged response strictly (206, exact Content-Range, expected total, exact body length, write bounds) and emit progress from in-memory atomic counters instead of per-tick stat scans. Make Settings.Verify control LFS verification: none / size (default, no full-file SHA reread) / sha256 (at most one post-download pass, with hash-on-write for fresh sequential transfers). Finalization on the normal HF-cache path is rename plus link/metadata work only. Legacy .part-NN/.parts.json partials are migrated once and removed only after the migrated range state is durable; a claim heartbeat keeps cross-process exclusion effective during long retry waits. Co-Authored-By: opencode <noreply@opencode.ai>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Normalize range completion at every checkpoint so a failure after the final byte cannot replay an out-of-range request (416). Trust a resume record only when the payload exists at the expected logical size, and remove metadata before payload on discard. Replace PID-based ownership with a per-process instance token plus same-host liveness and one heartbeat covering transfers, retry waits, strict SHA verification, and finalization. Cleanup skips staging that a live claim owns. Resume identity requires repo/path/size/resolved commit and both-or-neither blob SHA; legacy single .part files migrate as a committed prefix. Co-Authored-By: opencode <noreply@opencode.ai>
Treat publish-target existence as completion only for content-addressed blobs; arbitrary destinations follow the authoritative pre-download skip decision and are re-downloaded and atomically replaced. Content-prove legacy-migrated bytes with one full-file SHA pass before publication when a known digest exists and Verify is not none, keeping the marker durable across resume. Discard oversized single .part files instead of clamping them. Serialize claim, takeover, checkpoint, heartbeat, and removal through a sibling O_EXCL transition lock with ownership re-reads so two processes cannot both own a staging file. Make legacy migration context-aware, start the heartbeat before it, and run cancel cleanup for a user cancel during strict hashing. Preserve historical etag LFS hashing and scope size-mode documentation to the normal transfer path. Co-Authored-By: opencode <noreply@opencode.ai>
Hold an OS-level exclusive lock on the persistent staging lock file for the whole ownership interval (Unix flock, Windows exclusive-share handle, documented lease fallback elsewhere), so a suspended owner cannot resume writing after a takeover and a crashed owner releases immediately. Publish now re-reads the claim under the transition lock and refuses missing or foreign records. Bring the single-request path under the same trust model: validate every 206 against the exact requested Content-Range, expected total and body length with a bounded writer; require the exact expected size before the rename; and content-prove inherited .part bytes with one full-file SHA pass when a known digest exists and Verify is not none. Fall back to a valid single .part when a legacy layout contributes no bytes, mark LegacyMigrated only when bytes were copied, and drop the sequential hash state on an oversized-body retry so strict mode falls back to one final pass. Count only durable unique bytes in progress. Co-Authored-By: opencode <noreply@opencode.ai>
Re-check a content-addressed publish target under the transition lock before renaming, so a blob completed by another writer mid-flight is never overwritten; remove our staging instead. Persist whether a record is OS-lock fenced and let CheckBlob treat a free lock with a fenced record as a crashed owner regardless of PID reuse, while the lease fallback keeps its heuristic. Probe ownership with a non-owning shared lock so concurrent probes are not mistaken for writers. Poison a mismatched staging payload only while the claim is still owned under the transition lock; otherwise leave the durable proof-pending flag as the guard. Finalize a single-path transfer whose final byte was already written instead of retrying an out-of-range request, and emit the finalizing event and full progress before the pre-publish proof. Reconcile links for skipped same-SHA plan paths after the job, filter staging temp/lock artifacts from cache stats, selection and mirroring, clean legacy multipart artifacts after a single-path success, and use the verified publish path in the size check. Co-Authored-By: opencode <noreply@opencode.ai>
A free OS ownership lock is authoritative only for a record written under it: claims and cleanup now apply the lease heuristic whenever the record is not LockFenced, so a live fallback owner is never displaced or deleted by a lock-capable process. Before routing an item to the single-request path, detect usable multipart resume state (valid v2 staging pair or usable legacy .part-NN/.parts.json layout). Ranged-capable items keep the multipart route regardless of the configured threshold so migration/resume consumes the state; fresh items keep configured routing. Without range support, warn, download the full file on the single path, and remove the obsolete v2 pair only after success and a claim check. Clear the inherited-bytes flag when an ignored-Range 200 restart truncates the partial, avoiding an unnecessary size-mode proof. Co-Authored-By: opencode <noreply@opencode.ai>
Reject shared staging mutations when reliable lifetime OS ownership is unavailable instead of falling back to an expiring lease. Serialize local record transitions under that ownership without age-based lock eviction, and preserve ambiguous old claims and uncertain ownership status. Keep rejected oversized single-response bytes out of restartable prefixes, use consistent appended-filter resume identity, and honor cancellation while waiting to publish and immediately before rename. Add process ownership, lock-error, replay, route-resume and cancellation regressions, and update the documented safety model. Co-Authored-By: opencode <noreply@opencode.ai>
…ically Require clean response framing before completing a requested range and retain a recoverable nonempty suffix after final-byte transport errors. Use versioned framing semantics while conservatively migrating prior range records and keeping progress unique during revalidation. Stage complete claim documents in fresh inodes and atomically install or replace them without truncating a predecessor through leftover temp aliases. Keep downloads functional when hard links are unavailable. Honor cancellation before single-path rename and propagate the actual publication outcome of unknown-size ranged-to-single fallback so cache storage and snapshot entries are finalized by the correct owner. Co-Authored-By: opencode <noreply@opencode.ai>
Derive resumed transfer tasks and their committed baseline under the metadata mutex shared with heartbeat normalization. Keep task snapshots private before starting network work. Serialize sequential hash creation, writes, invalidation and digest selection so parallel oversized-response handling cannot race on the shared hash state. Preserve one final verification pass after rewinds. Add race-detecting resume and parallel-overflow regressions plus the sequential retry hash-count control. Co-Authored-By: opencode <noreply@opencode.ai>
Observe the completed-proof, closed-handle boundary rather than counting Context.Err calls, whose invocation count differs between supported Go versions. Keep cancellation, no-publication and partial-preservation assertions and cover explicit-cancel cleanup at the same boundary. Co-Authored-By: opencode <noreply@opencode.ai>
Observe ownership only with non-owning shared probes. Retry exclusive acquisition through transient reader contention with bounded context- aware waits; unresolved contention returns an actionable error rather than silently marking the missing file skipped. Keep exclusive locks and deny-all handles confined to actual lifetime writers so concurrent observations cannot manufacture a live owner. Preserve immediate recognition of real owners and prompt cancellation. Add download-level reader, timeout and cancellation regressions plus concurrent observer and cross-process ownership controls. Co-Authored-By: opencode <noreply@opencode.ai>
bashrusakh
marked this pull request as ready for review
October 21, 2026 20:26
bashrusakh
marked this pull request as draft
October 22, 2026 03:01
Give each content-addressed blob key one publication domain shared by the ranged route, the single-request route and the cache store, so the "the blob does not exist yet" decision and the placement that follows it cannot be overtaken by the other writer. Place the payload with a no-replace primitive: an existing blobs/<sha> is completion proof and is never replaced. Where the filesystem cannot create hard links, the placement is domain-exclusive and refuses to run without the domain. Report three ownership outcomes: a proven live owner may be skipped, a provably dead owner may be resumed, and an unknown ownership state is an observable error that never becomes a successful skip with the blob missing. Take the domain before the transfer so an unavailable domain fails before any payload is written and keeps earlier partials. Route the exported FinalizeBlob through the same domain and primitive, and record the mirror writer and the selected-GGUF delete path as the documented writers that remain outside the domain. Co-Authored-By: opencode <noreply@opencode.ai>
Integrate the upstream Windows desktop host and its structured logging subsystem with the #140 downloader model, so both survive the merge. Conflict resolution in pkg/hfdownloader/downloader.go keeps the #140 verification model: the Verify-mode switch (none / sha256+etag / size), the pre-publication contentVerified proof contract and the published blob path as the verification target. The upstream hunk that hashed every LFS file unconditionally is not reintroduced; only its logging is, as downloadLog records on the failed sha256, size and HEAD-probe paths. The old downloadMultipart function stays deleted, because the ranged downloader replaced it, and upstream's logging improvements for the equivalent code move to the ranged implementation: the HEAD-retry and range-retry warnings and the range-resume record. The cleanup helpers adopt upstream's signatures: removePartialArtifacts and removeBlobStagingForTemp no longer take the stale timeout, which was a dead parameter left behind when the lease model was replaced by the lifetime OS lock and the per-path metadata gate. Liveness stays decided by that lock and that gate, so no stale-time authority for ownership is reintroduced. CHANGELOG.md keeps both the upstream #141 proxy entry and the #140 entries. Co-Authored-By: opencode <noreply@opencode.ai>
bashrusakh
marked this pull request as ready for review
October 22, 2026 06:33
bashrusakh
marked this pull request as draft
October 22, 2026 08:11
A blob key owned by another live writer is never a successful skip: the file is not present while that owner holds the key. Wait for the key's ownership domain instead, bounded and context-aware, and re-check the published content-addressed target afterwards. A published blob completes the item as an ordinary skip with its own links and no second transfer; an owner that released the domain without publishing lets this writer download the file; an exhausted wait fails the item with an actionable error, so the job no longer reports success while a file is missing. Make cleanup participate in the same protocol: the artifacts of a blob key are removed only while that key's domain is held, so cancelling one job cannot delete another live writer's partial, while a job can still clean up its own artifacts. Document the wait budget, the slot it holds and the residuals, and align the surrounding comments with the new contract.
Integrate the upstream native service mode (#154) with the #140 downloader model, so both survive the merge. The merge auto-resolved without textual conflicts: the overlapping files changed in disjoint regions, so both sides are kept in full. pkg/hfdownloader keeps the #140 model byte for byte. The only upstream change in that package is targets.go, a comment on DefaultTargetsPath that we did not touch, so it takes upstream's version; the #140 files (downloader.go, ranged.go, blob_domain.go, blob_publish.go, hfcache.go, staging_*.go, verify.go, types.go) are unchanged by this merge. internal/server/api.go and internal/server/mirror.go keep both sides: the runtime-path wiring of the service mode and the #140 staging-artifact exclusion. The exclusion still covers every artifact name the #140 model introduces - the .incomplete payload, the .meta resume record, the .meta.tmp atomic record temp and the persistent .lock ownership files (the staging lock, the record lock and the blob-key domain lock blobs/tmp-<sha>.lock) - and it stays applied at every blobs/ listing, selection, mirroring and deletion site. Upstream added no new blobs listing or selection path, so the filter needed no new application. AGENTS.md, CHANGELOG.md and docs/API.md keep both sides: the service-mode rules, the service-mode reference and the service-mode changelog entry alongside the #140 ownership/publication model, the job contract, the cleanup ownership rule and the Verify-mode semantics. No section is lost and the two rule sets do not overlap semantically: the service mode governs the lifecycle and the runtime paths, the #140 rules govern the downloader. Co-Authored-By: opencode <noreply@opencode.ai>
Name the current range-record version (3) in the metadata field comment and explain that version 2 is accepted and migrated with its completed ranges revalidated. Restore the resume paragraph, where stray tokens had split sentences, and replace the ambiguous "v2" shorthand for the versioned staging state with wording that cannot be read as a record version, in production and test files. Co-Authored-By: opencode <noreply@opencode.ai>
bashrusakh
marked this pull request as ready for review
October 22, 2026 11:44
The writer took the ownership domain only for content-addressed items, so a path-derived destination (an item without a known digest) had no owner while the cleanup derived the same key and took a domain nobody held. Its protection was therefore vacuous: two writers of one destination were not mutually excluded, and a cancelled job could delete another live job's partial for the same path. Key the domain on the destination and let the writer and the cleanup use one identity, so the protection covers every cache destination; a writer that cannot take its domain now fails before it writes any payload. The legacy/output mode keeps its historical behavior, since that namespace has no internal directory for a domain lock. Treat a link-creation failure as fatal for the item in the skip paths, as the transfer path already did: the payload in blobs/ is not the requested path, so the job must not report success when the snapshot or friendly entry cannot be created. The end-of-job reconciliation surfaces its link failures as a job error instead of a warning. Co-Authored-By: opencode <noreply@opencode.ai>
The flat/output namespace has no destination domain, but it is not lock-free: the multipart route stages at <destination>.incomplete and holds the lifetime ownership lock on the persistent sibling <destination>.incomplete.lock, and a concurrent second writer of that destination is excluded with the actionable live-owner error instead of racing. The single-request route takes no ownership lock and is not excluded. Correct the statements that claimed otherwise, in AGENTS.md, the package docs, the API docs and the changelog. Route both transfer paths through the same helper that creates the cache entries, so a link-creation failure carries the exported ErrCacheLinkUnreachable sentinel on every path and the item fails identically wherever the entries are created. Pin the flat-mode behavior with regression tests for the multipart route's persistent lock artifact and its second-writer exclusion, and for the single-request route taking no ownership lock. Co-Authored-By: opencode <noreply@opencode.ai>
An item without a known digest stages under a name derived from its path, so two different objects, or two revisions of one object, share one staging name; the destination domain excluded concurrent writers but never established which object the staged bytes belonged to. The single-request path resumed from the file size alone and published anything it found, so a partial left by another file or another revision could reach the cache as this item's content (a mixture at verify=size, and with no request at all when the inherited size already matched). Record the staged bytes' identity in a versioned sidecar next to the partial - repository, resolved commit, item path, expected size and whether the commit was actually resolved - and inherit only when that record proves the bytes belong to this exact object and is pinned on both sides. Otherwise discard the inherited bytes and restart from zero; an unresolved commit pins nothing, so nothing is inherited from it. The sidecar is written before inherited bytes are trusted and removed with the partial it describes. Classify cache entries with one exhaustive classifier: every staging producer the downloader creates, a positively recognized canonical digest for a published blob, and an explicit unusual-name category for the names the public storage API accepts. Apply the staging exclusion inside blobs/ only, so the mirror copies and verifies the same set and a legitimate snapshot entry such as tokenizer.model.meta is no longer copied but skipped by verification. Co-Authored-By: opencode <noreply@opencode.ai>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Build the mirror destination path with SafeJoin instead of a plain filepath.Join, so the lexical containment barrier is explicit and modeled by the analyzer; the symlinked-parent check still applies because SafeJoin does not see symlinks. Document the suppression on the symlink target, whose containment is already proven by the absolute and resolved-inside-source checks. No change to what the mirror copies, skips or verifies. Co-Authored-By: opencode <noreply@opencode.ai>
Check the live and the persisted range layout immediately before the rename: every range must be Done and the committed total must equal the expected size, otherwise publish refuses with an actionable error and nothing is renamed, so the staged bytes stay on disk for a resume. The control flow already implied completeness; asserting it keeps a later refactor from silently publishing an incomplete payload. Report an unproven staging record as what it is instead of claiming another live writer: a record written without the lock fence, or one that cannot be read, is preserved and never taken over, and its error now says so and names the staging pair to remove, distinguishable through the exported ErrStagingOwnershipUnproven sentinel. A proven live owner keeps its transient "try again later" message. Document the operator recovery procedure for the unproven record. Co-Authored-By: opencode <noreply@opencode.ai>
The single-request path already inherits staged bytes only when the recorded identity is pinned on both sides, but the ranged record compared the commit string alone and its CommitPinned flag was never consulted. With an item that has no known digest, an endpoint that fails to resolve the commit and a moved branch, a stale record with the same revision string and size was accepted, and with no digest there is no content proof to catch the mixture either. Route both paths through one shared pinned-identity rule, persist the flag in the ranged record and treat a record without it as not inheritable. Correct the documentation that still called the single-request partial a prefix with no record at all, and state that the classification is by name shape, so a blobs/ name carrying a staging shape is treated as staging by every surface including deletion. Co-Authored-By: opencode <noreply@opencode.ai>
The planner resolved the requested revision to a commit and stored it as the resume identity, but the tree walk and every per-file URL still named the requested revision. A branch that moved between the info call and the transfer could therefore change the file set or the bytes behind the name the plan recorded, and under the default Verify=size a fresh ranged download has no content proof, so the wrong bytes could be published under the recorded digest. Build the whole transfer from the resolved commit when the plan is pinned. When no commit can be resolved, keep the requested revision and close the hole with exactly one pre-publication content proof for items that carry a known digest. Require the staging file to hold exactly the expected number of bytes before the rename, separately from the range layout, and fix the positive control that published a short payload against metadata that claimed a complete one. Refuse a blob key whose name is a staging shape at the store boundary, so a published blob can never be invisible to the cache surfaces and the mirror. Keep a non-empty single-request partial's identity sidecar when a permanent error aborts the attempt, so the next run can still prove the bytes and resume instead of discarding them. Co-Authored-By: opencode <noreply@opencode.ai>
Flat/output mode has no blobs/-style internal identity, so the single-request route (<destination>.part) and the multipart route (<destination>.incomplete) were not mutually excluded: two writers of one destination could interleave, and a job's cleanup could delete another writer's live partial. A flat destination now owns ONE domain, borrowing the multipart staging name: the domain key is the destination path and the lifetime lock is the persistent sibling <destination>.incomplete.lock - the very lock the multipart claim already takes - so no new artifact name appears next to the user's model file. Both routes and the cleanup resolve that one identity. Download takes the domain only for the route that does not take it internally (the multipart claim holds it inside the transfer and an exclusive lock conflicts even within one process), and the size-unknown hand-off to the single-request path takes it before the claim. CleanupJob- PartFiles holds the destination's domain before removing its artifacts, so a cancelled job cannot delete a live writer's partial. Windows CI now runs the critical downloader runtime tests (ownership lock, cross-process claim exclusion, ranged resume, no-replace publication, single-request identity, flat-mode domain) instead of only building and vetting the package. Co-Authored-By: opencode <noreply@opencode.ai>
An item whose destination is owned by a proven live writer waited for that ownership domain while already holding one of the job's MaxActiveDownloads slots. With a low limit one waiting item kept every other file of the job from starting, so the concurrency budget was spent on a wait and the job looked stalled. The slot is now a per-item lease. The domain acquisition itself signals the lease when it is about to block - for both the in-process slot wait and the cross-process OS-lock wait - returns the slot for the duration, and takes it back before it returns a domain the caller will transfer under. The free fast path neither releases nor reacquires, a failed acquisition keeps the slot returned (nothing will transfer), and the lease is released on every item exit. The same lease covers the cache destination domain, the flat destination domain and the flat-mode size-unknown hand-off inside the multipart route. MaxActiveDownloads therefore bounds concurrent transfers, not waiters; cancellation stays prompt and the ownership/publication/cleanup models are unchanged. Stabilize TestClaimStagingConcurrentTakeover: a takeover installs the replacement claim record before it recreates the staging payload, so reading the new record does not prove the payload exists. Synchronize on the winner's first range request instead, which is sent only after the payload has been opened and preallocated. Co-Authored-By: opencode <noreply@opencode.ai>
The slot lease guarantees that a DESTINATION-DOMAIN waiter does not hold a MaxActiveDownloads slot. The multipart staging claim is a separate ownership wait and remains the documented exception: a cross-process claimant waits at most stagingOwnershipAcquireWait (5s) before the actionable ownership error, and an in-process claimant waits for the current owner bounded by the item's context; that wait still runs with the item's slot held and cannot deadlock. Say so in the downloader contract, the API document, the settings comment and the changelog instead of claiming that every waiter is slot-free. Also document the two consequences of the flat destination owning a domain: a destination whose domain cannot be acquired fails the item before it writes payload, and every flat single-request download now leaves the persistent <destination>.incomplete.lock (the shared domain/staging lock), not only the multipart route. Add TestDownloadSlotLeaseFreePathDoesNotReleaseOrRetake: the uncontended acquisition must never signal a wait, so it neither returns nor retakes the slot. Co-Authored-By: opencode <noreply@opencode.ai>
removeEmptyPartialFile treated every os.Stat error as "the file is gone", so the fail-fast branches removed the identity sidecar on EACCES/ELOOP/EIO while a non-empty partial could still exist; the next run then could not prove the surviving bytes and discarded them. Only a not-exist result proves the partial is gone now. Any other stat failure keeps the sidecar and logs the path and error. The multipart staging claim did not use the MaxActiveDownloads slot lease, so its in-process owner wait, its cross-process OS-lock window and its local metadata gate all ran with a transfer slot held. The claim now signals the same per-item lease: the slot is returned for the duration of every wait and taken back before the claim is returned to a caller that will transfer under it. A failed claim keeps the slot returned, and a cancellation while the slot is being reacquired releases the claim and reports the context error instead of handing out a claim that cannot transfer. Docs now state that both the destination-domain waits and the multipart staging-claim waits return the slot; the former "documented exception" is gone. Tests cover the stricter stat contract (including a symlink-loop stat failure), every claim wait, a Download-level case where another file transfers while the claim waits, and the unchanged takeover test. Co-Authored-By: opencode <noreply@opencode.ai>
The claim finalizer releases the OS lock and the in-process staging owner entry and returns ctx.Err() when the slot cannot be taken back after a successful claim. That branch had no test: the recorder's endWait always returned true, so the release was established only by inspection. Add a notice whose endWait cancels the item context and returns false, drive a real claim through an in-process owner wait, and assert a nil claim plus context.Canceled. A fresh claim on the same staging path then succeeds, which proves the previous claim's OS lock and registry entry were released. Fix the download_slot.go comment that still described the lease as reacquiring the slot on an error return; a failed acquisition returns no domain/claim and keeps the slot returned. Co-Authored-By: opencode <noreply@opencode.ai>
bashrusakh
marked this pull request as draft
October 26, 2026 14:09
Path-derived staging becomes structurally non-canonical. A digest-less item now stages at blobs/tmp-path-<sanitized path>, whose name-based domain (blobs/path-<...>, lock blobs/tmp-path-<...>.lock) can never be a canonical blob key or lock, and can never share the staging name of a content-addressed tmp-<sha> destination. Before this, a digest-less item whose relative path was a digest string could hold the blob domain another item's store needed, and two such items could deadlock on the unbounded in-process domain wait; the same name shape could also make a path-derived and a content-addressed item share one staging file while holding different domains. The destination domain stays name-based and one per destination; content-addressed identities are unchanged. The multipart migration now honours the single-request identity record: a .part carrying a sidecar is imported only when the recorded identity is the current plan's (repo, commit, path, size, pinned on both sides); a mismatch, an unreadable record or an unpinned identity imports nothing and preserves the partial and its record. A sidecar-less .part keeps the historical pre-#140 migration. The single-request sidecar also records framing_complete, set only after io.Copy returns nil and every exact body-length check passes. An exactly-sized partial is published without a request only when the matching identity records that proof; otherwise the final byte is reserved and re-requested as [size-1, size), and a body error during that revalidation leaves it uncommitted. The same rule applies when a fully-sized, unframed partial crosses into multipart staging. The single-path proof cancellation now closes the writable payload handle before the explicit-cancel cleanup, so Windows cannot retain a cancelled .part and its sidecar; a Windows runtime test covers it. An unreadable staging record now carries ErrStagingOwnershipUnproven while preserving the underlying read error. Co-Authored-By: opencode <noreply@opencode.ai>
…al bytes The multipart route's HEAD fail-fast removed <dst>.part and its identity sidecar even though the multipart route creates <dst>.incomplete, not .part, and in flat/output mode reaches that HEAD holding no destination domain. A concurrent flat single-request writer could therefore lose its live partial and fail its later rename. The multipart HEAD failure now removes nothing: a route removes only the artifacts it creates. The single-request fail-fast keeps its own cleanup because it holds the destination domain and knows it created that partial. Legacy migration no longer treats a written byte count as completion proof. A sidecarless, fully-sized .part reserves its final byte and a ranged worker re-requests [size-1, size); the whole prefix is imported only when a matching, pinned sidecar records cleanly-framed completion. Every contributing pre-#140 .part-NN part is imported one byte short and left not-Done, so its final byte is re-requested with clean framing. A torn final byte of a "complete" legacy partial can no longer be published with zero GETs for a digest-less item in Verify=size mode. Tests cover the preserved and live-writer partials, the sidecarless .full-part reserve, a torn legacy part-NN final byte, and the updated resume oracles; docs state the route-ownership and prefix rules. Co-Authored-By: opencode <noreply@opencode.ai>
bashrusakh
marked this pull request as ready for review
October 26, 2026 16:10
bashrusakh
marked this pull request as draft
October 27, 2026 01:36
The downloader no longer migrates incomplete downloads from old or unsupported formats. Only a version-4 staging record is resume state; every other version, including the branch-only version-3 record whose legacy_migrated flag described imported bytes, and every pre-#140 .part-NN/.parts.json family is discarded under the claim/domain and the file is re-downloaded from zero. A warning names the discarded format. Unreadable or unproven current-format records are still refused and preserved; published blobs, completed files and cache entries are never touched. This removes the legacy migration surface: the pre-#140 detection, routing and migration functions, the v2 Done-range rewrite, the LegacyMigrated proof gate, the uncalled WriteIncompleteMeta helper and the legacy layout writers. Current-format resume (.incomplete/.meta and the single-path .part sidecar), ownership locks, the metadata gate, ProofPending, atomic no-replace publication and cleanup safety are unchanged. stagingMetaVersion is bumped 3 -> 4 so an old branch record can never be mistaken for ordinary resume state. The removal deliberately drops upgrade resumability; issue #140 criterion 11 needs an explicit acceptance amendment, which the owner handles separately. Co-Authored-By: opencode <noreply@opencode.ai>
Every route (cache single, cache multipart, flat single and flat multipart) now acquires the destination's ownership domain once at item level, after the skip decision and before the HEAD/claim, and holds it through the claim, transfer, publication, store and cancel cleanup. The multipart staging claim receives that domain, verifies that it covers its staging path, and fails closed otherwise; it no longer takes a second <staging>.lock or registers a second in-process owner. In flat mode the claim lock and the domain lock were literally the same file, which is why the old code had to skip the item-level acquisition for the multipart route; that self-conflict rule and the route-specific exception are gone. CheckBlob now probes the destination domain lock blobs/tmp-<sha>.lock, the fence every writer actually holds, instead of the obsolete claim lock. The cleanup entry points collapse to one owner-checked removal under the domain and one record-checked removal for external cleanup; the always-empty staging argument and the unclaimed/self-conflict variants are deleted. The slot lease now belongs only to the domain acquisition; the claim's own lease plumbing and the metadata gate's wait callback are gone. Current-format resume, pinned identity, ProofPending, atomic no-replace publication, ErrStagingOwnershipUnproven, B1 route ownership and the path-derived digest acyclicity are unchanged. Mixed-version writers that held the old claim lock are intentionally unsupported. Co-Authored-By: opencode <noreply@opencode.ai>
After the destination domain became the only ownership fence, the periodic staging-record heartbeat had no production reader: no ownership decision consumed record freshness, range workers already flush every committed checkpoint, and ProofPending guards the content-proof transition. The heartbeat was a goroutine, a periodic record rewrite and several fatal checks that could only turn a timer into a transfer failure. Remove startHeartbeat/stopHeartbeat/heartbeatError, its fields, constants and the metaFreshWithin helper, together with every timer check in transfer, finalization, publish and cancel. The record remains diagnostics/resume state, not a freshness-based lease; ownership is the destination domain plus LockFenced. Per-write range flushes, the metadata gate, ProofPending, the v4 record and the unproven-record refusal are unchanged. Tests cover concurrent checkpoint normalization without the heartbeat; the heartbeat-specific fixtures are gone. A long wait or finalization now surfaces a record write failure at the next checkpoint/proof transition, which already carries the durable proof guard. Co-Authored-By: opencode <noreply@opencode.ai>
The Windows downloader step still named the pre-refactor TestDownloadFlatModeUnknownSizeHandoffTakesTheDestinationDomain, which no longer exists; the regex therefore passed without exercising the unknown-size hand-off on Windows. Point it at the actual ...UsesOneDomain test and verify the whole selection (15/15) resolves. Remove the dead isPublishedBlobName helper and the inert IncompleteMeta.Revision field, and align the remaining comments and CHANGELOG history with the one-destination-domain, no-heartbeat model: the ownership fence is the item-level destination domain, the metadata gate is an in-process map gate, and the five-minute budget is derived from the historical acquisition unit rather than bounding an attempt. No behavior change; the final whole-PR review's notes are closed. Co-Authored-By: opencode <noreply@opencode.ai>
13 tasks
bashrusakh
marked this pull request as ready for review
October 27, 2026 05:23
bashrusakh
marked this pull request as draft
October 27, 2026 05:25
Both resume records now expose their object identity as one in-memory resumeIdentity value and compare it with one matches rule (repo, commit, path, size, pinned on both sides). The persisted formats stay separate: the version-1 .part sidecar and the version-4 ranged record keep their own fields, versions and lifecycle data, but the identity decision can no longer drift between the routes. The age-based StaleTimeout configuration is dead after the destination domain and LockFenced became the ownership signal and the heartbeat was removed: no production path uses record age for takeover or status. Remove Settings.StaleTimeout, HFCache.StaleTimeout, DefaultStaleTimeout and the now-dead constructor argument; update the tests and the public docs. This is a deliberate config/API simplification under the no-legacy decision, not a compatibility path. Co-Authored-By: opencode <noreply@opencode.ai>
pkg/hfdownloader/doc.go is now the single detailed contract for the destination domain, claim, slot lease, v4 resume, single-path identity and framing, publication, verification, cleanup and the job contract. AGENTS.md keeps the agent-level summary and a pointer to it; the duplicated downloader narrative shrinks from 595 to 82 lines. API.md keeps the user/API-facing contract and removes the internal implementation duplication. CHANGELOG.md keeps user-facing release notes; the current entries describe the no-legacy, one-domain, no-heartbeat, no-StaleTimeout model. No code, test or behavior change. Rules that existed only in the summary documents were moved into doc.go before their duplicates were removed. Co-Authored-By: opencode <noreply@opencode.ai>
Remove the exported CleanupIncomplete helper: it deleted staging paths with raw os.Remove, outside the destination domain, the metadata gate and the record check, so it could delete a live writer's partial and accepted an unconfined caller key. No in-tree caller remains; the removal is recorded in the changelog. Align the remaining comments, GoDoc and API/contract text with the one-domain, no-heartbeat, no-StaleTimeout model: the multipart claim takes no ownership wait and does not move the MaxActiveDownloads slot; the flat lock is the item-level domain lock; the multipart HEAD holds the domain but still removes nothing because the .part belongs to the single-route writer; every flat TRANSFER (not every skipped download) leaves the domain lock. State the old-version rule precisely: fenced/provably-dead/self old records are discarded, while an unfenced old record whose owner cannot be proven dead is refused as unproven and preserved. Check the context immediately before the ranged publication placement, so "publish checks cancellation immediately before rename" is true for both the content-addressed and flat rename branches; the staged bytes remain for a resume. Co-Authored-By: opencode <noreply@opencode.ai>
Make the old-version rule exact everywhere: a fenced old-version record (or one whose owner is provably dead or is this process) is discarded; an unfenced old-version record whose owner cannot be proven dead is refused as unproven and preserved before the version branch. Document the flat lock as the one item-level domain lock, the single-request .part case behind BlobMissing, and the ranged pre-placement cancel exception that leaves the proven staging pair for a resume. Comments and documentation only; no behavior change. Co-Authored-By: opencode <noreply@opencode.ai>
bashrusakh
marked this pull request as ready for review
October 27, 2026 06:44
bashrusakh
marked this pull request as draft
October 28, 2026 01:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #140.
blobs/<sha>.incompletein HF-cache mode,<destination>.incompleteotherwise) via boundedWriteAtwrites. The legacy per-connection.part-NNpayload files, the assembly copy and the unconditional full-file LFS reread are gone. Publication is a no-replace placement plus link work, never a payload-sized copy.206, exactContent-Range, expected total, exact body count, bounded writes, and clean end-of-body framing. A copy error after the final requested byte leaves that byte uncommitted for a non-empty retry/restart range. The ranged publication also checks cancellation immediately before the placement/rename..incomplete/.metarecord and the version-1.part.metasidecar are the only resume state. Both expose their object identity through one sharedresumeIdentity.matchesrule (repo, commit, path, size, pinned on both sides); the persisted formats stay separate. An exactly-sized.partis published without a request only withframing_complete..part-NN/.parts.jsonfamily is discarded, never migrated. Already-published content is untouched. This deliberately supersedes issue Downloader: eliminate multipart assembly with direct ranged writes and make verification mode effective #140 criterion 11.claimStagingreceives it, verifies coverage and fails closed; it takes no second OS lock and keeps no second registry.CheckBlobprobes the destination domain lock; cleanup has two domain-gated entry points. TheMaxActiveDownloadsslot is a per-item lease: a domain waiter returns it, a transfer holds it.StaleTimeout. The staging record is diagnostics/resume state; ownership is the destination domain plusLockFenced. Per-range flushes,ProofPending, the metadata gate and the publication gates remain.Settings.Verifycontrols LFS verification:none,size(default; exact size and complete validated coverage, no full-file reread on the normal path),sha256(at most one pre-publication pass),etag(the same single pass for a known digest). A known digest is never hashed only after publication.pkg/hfdownloader/doc.gois the canonical detailed contract; AGENTS.md is a short agent-level summary; docs/API.md is user/API-facing; CHANGELOG is user-facing.Deliberate API removals
Under the no-legacy/single-package decision:
RepoDir.CleanupIncomplete,RepoDir.WriteIncompleteMeta,IncompleteMeta.LegacyMigrated/Revision,Settings.StaleTimeout,HFCache.StaleTimeout,DefaultStaleTimeout, and the now-deadNewHFCachestale-timeout argument. The unguardedCleanupIncompletewas removed because it bypassed the destination domain and path containment.Type of change
Verification
Latest cleanup (
751171a)go test ./... -count=1, binary build, formatting and diff checks passed on the published cleanup content.Review and integration boundary
26b0552and green CI oned275fapredate this cleanup. No new whole-PR review was performed before this push; the local tests/comment cleanup had its own scoped independent reviews and final verification.mainchanges from smartdl: add branch-based EXL2 and EXL3 variants #152 have not been merged/rebased into this branch. This cleanup does not claim current-main integration or merge readiness.Risks / limitations
<destination>.incomplete.locknext to the file for every flat transfer; it is the one ownership fence and is never unlinked.sizemode for a pinned identity; no fsync;pkg/smartdlstill walks the requested revision.<staging>.lockare intentionally unsupported; v4 records are the only resume format.