Skip to content

[Reliability] Lock model downloads by canonical target#245

Open
fettpl wants to merge 5 commits into
sybil-solutions:mainfrom
fettpl:p1/issue-227-download-target-locking
Open

[Reliability] Lock model downloads by canonical target#245
fettpl wants to merge 5 commits into
sybil-solutions:mainfrom
fettpl:p1/issue-227-download-target-locking

Conversation

@fettpl

@fettpl fettpl commented Jul 17, 2026

Copy link
Copy Markdown

Summary

Closes #227

Depends on #244. Its reviewed commit 3cdd3a6402308c1d7a25816205cdedd7e73688f6 is an ancestor of this branch, and all six release-gating files remain byte-identical.

Root cause: download runs were isolated only by random job ID. Two jobs resolving to the same destination could open the same .part file concurrently, mix bytes across revisions, race final renames, and release state belonging to a newer run. A writer error arriving immediately after a terminal reader result could also be missed before rename.

Solution design:

  • Reserve the canonical physical destination tree synchronously before metadata fetch or job mutation.
  • Normalize symlink ancestors, host-appropriate casing and Unicode aliases, and ancestor/descendant tree overlap while permitting unrelated targets to run concurrently.
  • Use opaque owner tokens for compare-and-delete release, keeping target ownership separate from the existing per-run identity guard.
  • Keep the reservation until response-reader cleanup and writer close both settle on completion, failure, pause, cancel, or shutdown.
  • Race pending reads against writer failure, recheck after each read, and recheck stored failure after the writer close barrier so terminal writer errors fail closed.
  • Preserve .part files and HTTP Range resume behavior while preventing pause/resume handoff from opening a second writer.
  • Return a typed HTTP 409 conflict containing only the active download ID and target; rejected starts and resumes do not mutate either job.

Validation

  • cd controller && bun test --coverage tests/downloads/download-manager.test.ts tests/downloads/download-target-reservations.test.ts tests/downloads/stream-backpressure.test.ts: passed, 25 tests and 119 assertions.
  • Focused coverage: download-manager.ts 92.73% functions / 99.82% lines; download-target-reservations.ts 100% / 100%; stream-backpressure.ts 100% / 100%; download-routes.ts 71.43% / 82.56%.
  • cd controller && bun test tests: passed, the complete available controller suite (25 tests, 119 assertions).
  • npm run check: passed, including contracts, repository structure, frontend quality and production build, and controller type checks, lint, dependency/dead-code/duplicate checks, and standards audit.
  • git diff --check origin/main...HEAD: passed.
  • Normal pre-push hook: passed conventional-commit validation and the complete frontend quality/production-build gate.
  • npm --prefix frontend run desktop:dist -- --config.electronDist=<approved Electron archive>: passed.
  • Canonical /Applications/Local Studio.app reinstall: strict deep signature valid, bundle ID org.local.studio.desktop, and GET /api/desktop-health returned 200 on loopback.
  • Independent P0/P1 review approved exact head 0edd8bc2ffebf2d6a69bae9c66f8f326de7513d3 with no findings.
  • Current main defines neither a root test:integration script nor a frontend test script, so no result is claimed for unavailable commands.

UI changes

No visible UI changes. The inherited release-gating dependency touches frontend package/workflow files, so the required production desktop rebuild and canonical reinstall were still completed.

Risks / rollout notes

  • Reservations are intentionally process-local. Multiple controller processes sharing one models directory still require an OS-level or distributed lock and remain out of scope.
  • A reader cancellation that has not settled intentionally retains the reservation; this favors filesystem integrity over allowing a second writer onto a target whose prior stream has not finished cleanup.
  • Clients that collide on a destination now receive HTTP 409 and may retry after the active run completes or finishes teardown.

@fettpl
fettpl force-pushed the p1/issue-227-download-target-locking branch from 8ad367b to 86edca8 Compare July 18, 2026 10:43
@fettpl
fettpl marked this pull request as ready for review July 18, 2026 10:44
@fettpl
fettpl requested a review from 0xSero as a code owner July 18, 2026 10:44
@fettpl

fettpl commented Jul 18, 2026

Copy link
Copy Markdown
Author

@0xSero The refreshed ready-for-review head 0edd8bc2ffebf2d6a69bae9c66f8f326de7513d3 passed independent P0/P1 review, focused coverage, full local repository gates, and the required desktop distribution/reinstall/health check. The PR is mergeable; GitHub marked both current-head fork workflows as requiring maintainer approval. Please approve CI and Security when convenient.

@fettpl
fettpl force-pushed the p1/issue-227-download-target-locking branch from 3210f33 to 0edd8bc Compare July 20, 2026 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Reliability] Lock model downloads by canonical target

1 participant