Skip to content

feat(download): metalink mirrors fallback in engine (task 40)#153

Merged
mpiton merged 9 commits into
mainfrom
feat/task-40-metalink-mirrors
May 6, 2026
Merged

feat(download): metalink mirrors fallback in engine (task 40)#153
mpiton merged 9 commits into
mainfrom
feat/task-40-metalink-mirrors

Conversation

@mpiton
Copy link
Copy Markdown
Owner

@mpiton mpiton commented May 5, 2026

Summary

Adds Metalink RFC 5854 mirror failover to the download engine: a multi-source Download walks its mirror list highest-priority-first, switches automatically when a mirror returns 4xx/5xx, and only marks the download Failed once every mirror is exhausted. Sprint task 40, PRD-v2 §P1.21 / PRD §7.1.

Why

.metalink files ship N alternative URLs per file; a single-source engine that doesn't fall back over to mirror 2 when mirror 1 returns 404 defeats the point of Metalink — the user pastes a Metalink with three mirrors and gets one failed download instead of one completed one. The engine already had segment retry, but no concept of "switch source", so the segment loop just kept retrying the dead URL.

Changes

  • New domain aggregate Mirror (Url + priority 1..=100 + optional ISO-3166 alpha-2 country) in src-tauri/src/domain/model/mirror.rs. Pure std, zero serde — domain stays serde-free per architecture rule.
  • Download gained mirrors: Vec<Mirror> + current_mirror_index: u32, plus with_mirrors, advance_mirror, reset_mirror_cursor, active_url() returning mirrors[current_mirror_index] and falling back to the canonical url field when the list is empty (single-source downloads see no behaviour change).
  • MAX_MIRRORS_PER_DOWNLOAD = 64 cap in set_mirrors so a malformed .metalink with thousands of entries cannot bloat mirrors_json or stall the failover loop.
  • Engine restructured: start() snapshots the mirror URL list + persisted cursor, then loops over run_mirror_attempt(MirrorAttemptParams). Each attempt creates attempt_token = cancel_token.child_token() so an internal segment-failure teardown cancels peers without raising the user-cancel signal — user_cancel_token.is_cancelled() remains the only path to AttemptOutcome::Cancelled.
  • New DomainEvent::MirrorSwitched { id, new_mirror_index, new_url } published on each failover; DownloadFailed only on full exhaustion.
  • New SQLite migration m20260505_000009_add_mirrors: mirrors_json TEXT NULL + current_mirror_index INTEGER NOT NULL DEFAULT 0 on downloads. Adapter-side MirrorJsonDto keeps the domain serde-free; serialize_mirrors / deserialize_mirrors round-trip the list.
  • IPC + frontend: DownloadDetailViewDto carries mirrors: Vec<MirrorViewDto> + currentMirrorIndex in camelCase; new MirrorView TS interface; new MirrorsSection panel renders the active mirror (host + priority + country) plus alternatives, active row highlighted via aria-current="true". DownloadDetailsPanel only inserts the section when mirrors.length > 0 so non-Metalink downloads keep their compact layout.
  • Event bridge: mirror-switched Tauri event (camelCase payload id / newMirrorIndex / newUrl); the download log bridge ignores it (no log spam on every failover).

Testing

  • cargo test --workspace — 1472 passed (including 8 Mirror unit tests, 7 Download mirror-handling tests, 3 engine wiremock tests covering the three acceptance criteria, 2 SQLite round-trip tests, 1 detail-DTO camelCase test).
  • cargo clippy --workspace -- -D warnings — clean.
  • cargo fmt --check — clean.
  • npx vitest run — 691 passed (including 3 new MirrorsSection tests).
  • npx oxlint . + npx oxfmt . — clean.
  • Engine wiremock acceptance tests:
    • test_three_mirrors_first_404_triggers_failover_to_second — HEAD 404 on mirror 1 → switch to mirror 2 → DownloadCompleted, exactly one MirrorSwitched.
    • test_all_mirrors_fail_publishes_download_failed — all mirrors return 5xxDownloadFailed, exactly one switch between the two slots, no DownloadCompleted.
    • test_priority_respected_highest_first — mirrors inserted as low/high/mid → engine sorts and tries high first, switches once to mid, never reaches low.
cargo test --workspace
npx vitest run

Related Issues

  • Refs sprint task 40 (.claude/output/sprints/prd-v2-roadmap/tasks/40-metalink-mirrors-fallback.md)
  • Refs PRD-v2 §P1.21 / PRD §7.1

Notes for Reviewer

  • 28 files / +1511 / −302. Suggested review order, commit-by-commit:
    1. a77f1ed — domain Mirror + engine refactor + migration + frontend section.
    2. 05a174e/simplify cleanup (extracted getHostname/getProtocol helper, MirrorAttemptParams struct, safe_u32 helper, MAX_MIRRORS_PER_DOWNLOAD cap, doc trims).
    3. 93cd7c3 — fixture fix for DownloadsView.test.tsx (caught by pre-push hook).
  • The persisted current_mirror_index cursor is a forward-looking hook: the engine still drives failover with its in-task cursor, so a crash mid-failover restarts from slot 0. Documented inline on Download::current_mirror_index and the entity column.
  • attempt_token = cancel_token.child_token() is the load-bearing piece for separating "user clicked cancel" from "all peers must die because one segment failed" — please verify the cancellation propagation in run_mirror_attempt.

Checklist

  • Tests added and passing locally (Rust + TS)
  • CHANGELOG.md updated under [Unreleased]
  • No secrets, debug prints, or commented-out code
  • Self-reviewed via /simplify (3 parallel review agents)
  • CI green expected

Summary by cubic

Adds Metalink mirror failover to the download engine with automatic switching on 4xx/5xx. Persists and shows the active mirror, resets progress/segments on switch, only resets the cursor when all mirrors are exhausted, and prevents advancing on user cancel between attempts. Aligns with Task 40 requirements.

  • New Features

    • Engine: tries mirrors by priority, auto-switches on errors, emits MirrorSwitched; on full exhaustion emits AllMirrorsExhausted then DownloadFailed.
    • Domain/DB: new Mirror model; Download stores mirrors + active index (cap 64); SQLite adds mirrors_json and current_mirror_index.
    • IPC/UI: Tauri events mirror-switched and mirrors-exhausted; detail DTO includes mirrors and currentMirrorIndex; MirrorsSection highlights the active mirror.
  • Bug Fixes

    • Mirror cursor: persist on MirrorSwitched; exclude from upsert updates to avoid races; clamp on read to valid range.
    • Reset cursor to 0 only on AllMirrorsExhausted so post-download failures don’t wipe the last-known-good mirror.
    • On MirrorSwitched, zero downloaded_bytes, clear total_bytes, and delete download_segments so progress and the plan rebuild cleanly.
    • Clean up stale .vortex-meta and partial files before each mirror retry.
    • Honor cancel between attempts: if the user cancels after a failed attempt and before the next starts, do not advance the cursor or emit MirrorSwitched.

Written for commit cd1f304. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Metalink mirror support with persisted mirror list, automatic failover, and a Mirrors panel showing active source, alternatives, priority and country
    • UI emits mirror-switched notifications and displays active mirror status
  • Bug Fixes / Reliability

    • Improved mirror failover, retry, stale-file cleanup and exhaustion handling; clearer cancellation outcomes
    • Safer error propagation and read-model consistency
  • Tests

    • Expanded unit and end-to-end tests covering mirror behavior and UI rendering

mpiton added 3 commits May 5, 2026 15:53
Adds Mirror domain aggregate (priority + ISO-3166 country) and engine
failover loop so a download with multiple Metalink mirrors retries the
next candidate when the primary fails.

- domain/model/mirror.rs: Mirror struct, sort_by_priority (ties by URL)
- Download: with_mirrors / advance_mirror / reset_mirror_cursor /
  active_url cursor, persisted via current_mirror_index
- engine: AttemptOutcome + run_mirror_attempt; user-cancel detected via
  child token so internal segment teardown does not raise the cancel
  signal; emits MirrorSwitched then DownloadFailed on full exhaustion
- migration m20260505_000009: mirrors_json TEXT + current_mirror_index
- entity adapter MirrorJsonDto keeps domain serde-free
- read repo / detail DTO / Tauri IPC: mirrors + currentMirrorIndex
- frontend MirrorsSection: active highlight via aria-current, listing
  alternatives with priority + country tags

Acceptance: 3 wiremock engine tests (404 → switch, all-fail → Failed,
priority sort), 2 SQLite round-trip tests, 1 IPC camelCase test, 3
Vitest tests for the panel — full suite green: 1473 backend + 691
frontend.
- Extract getHostname/getProtocol to src/lib/url.ts; consume from both
  MirrorsSection and SourceInfoSection (removes the duplicated helper).
- run_mirror_attempt: 13 positional args → MirrorAttemptParams struct,
  matching the existing SegmentParams pattern in the same module and
  dropping the #[allow(clippy::too_many_arguments)] suppression.
- entities/download.rs + download_read_repo.rs: replace ad-hoc
  u32::try_from(...).unwrap_or(0) with safe_u32() for consistency.
- domain/model/download.rs: add MAX_MIRRORS_PER_DOWNLOAD = 64 and
  truncate in set_mirrors so a malformed Metalink with thousands of
  entries cannot bloat mirrors_json or hang the failover loop.
- Trim doc comments on Download::mirrors / current_mirror_index / the
  entity columns to keep only the WHY (cap rationale, future-extension
  hook). The honest behaviour of the persisted cursor is now spelled
  out — the engine still drives failover with its own in-task cursor,
  so a crash mid-failover restarts from slot 0; the column is retained
  as a hook for the future call site that will mark a failing slot at
  the domain level.
- DownloadDetailsPanel.tsx: drop the redundant `mirrors && …` guard;
  the field is non-optional in TS so length>0 is sufficient.
- MirrorsSection.tsx: same — the inner `!download.mirrors` short-circuit
  was dead code given the parent gate.

Tests: cargo test 1472 passed (16 engine + 15 download_repo + the rest)
all green; clippy --workspace --all-targets -- -D warnings clean;
oxlint + oxfmt clean; vitest 49/49 in DownloadDetailsPanel + lib.
Inline download_detail mock missed mirrors: [] + currentMirrorIndex: 0
after simplify pass dropped null guards in DownloadDetailsPanel.

Caught by lefthook pre-push ts-test hook.
@github-actions github-actions Bot added documentation Improvements or additions to documentation rust frontend labels May 5, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds Metalink mirror support end-to-end: new Mirror domain type, per-download mirror cursor, DB migration and JSON column, engine failover loop emitting DomainEvent::MirrorSwitched, progress-bridge persistence/cleanup, tauri event wiring, frontend DTOs and a MirrorsSection, plus tests and fixture updates.

Changes

Mirror failover feature

Layer / File(s) Summary
Domain types & behavior
src-tauri/src/domain/model/mirror.rs, src-tauri/src/domain/model/download.rs, src-tauri/src/domain/model/views.rs
Add Mirror type (url, priority, country); extend Download with mirrors, current_mirror_index, mirror APIs (set/advance/reset/active_url), sorting & cap; add MirrorView.
Domain events
src-tauri/src/domain/event.rs
Add DomainEvent::MirrorSwitched { id, new_mirror_index, new_url }.
Persistence schema & entity mapping
src-tauri/src/adapters/driven/sqlite/migrations/m20260505_000009_add_mirrors.rs, src-tauri/src/adapters/driven/sqlite/entities/download.rs
Migration adds mirrors_json (nullable text) and current_mirror_index (int default 0); entity adds serde DTO plus serialize/deserialize helpers; Model/ActiveModel map mirrors and cursor.
Read/Write repos & progress bridge
src-tauri/src/adapters/driven/sqlite/download_repo.rs, src-tauri/src/adapters/driven/sqlite/download_read_repo.rs, src-tauri/src/adapters/driven/sqlite/progress_bridge.rs
Upsert/read updated to persist/restore mirrors and cursor; progress_bridge adds BridgeMessage::MirrorSwitched/MirrorCursorReset, maps events to messages, persists cursor via update_mirror_cursor, resets bytes and clears per-download segments on mirror switch; tests added/updated.
Download engine
src-tauri/src/adapters/driven/network/download_engine.rs
Factor per-mirror logic into run_mirror_attempt; start snapshots mirror candidates and runs a failover loop with attempt-scoped cancellation; cleans stale .vortex-meta/partial files before retry; publishes MirrorSwitched on retry and DownloadFailed when exhausted; unit tests for failover, exhaustion, and priority ordering added.
Event forwarding & logging
src-tauri/src/adapters/driven/event/tauri_bridge.rs, src-tauri/src/adapters/driven/logging/download_log_bridge.rs
Tauri bridge emits "mirror-switched" with camelCased payload (id, newMirrorIndex, newUrl); download log bridge treats MirrorSwitched as a no-op.
Application read-models & DTOs
src-tauri/src/application/read_models/download_detail_view.rs, src-tauri/src/application/queries/get_download_detail.rs
Add MirrorViewDto; extend DownloadDetailViewDto with mirrors and current_mirror_index; update mappers, fixtures, and serialization tests.
Frontend types, helpers & UI
src/types/download.ts, src/lib/url.ts, src/views/DownloadDetailsPanel/MirrorsSection.tsx, src/views/DownloadDetailsPanel/DownloadDetailsPanel.tsx, src/views/DownloadDetailsPanel/SourceInfoSection.tsx
TypeScript MirrorView and DownloadDetailView extended; add getHostname/getProtocol helpers; new MirrorsSection renders active mirror and alternatives with aria-current; conditional rendering wired into details panel.
Tests & fixtures
src-tauri/..., src/views/..., src/hooks/...
Updated fixtures to include mirrors/currentMirrorIndex; added tests across domain, engine, persistence, progress-bridge, DTO serialization, and UI component tests for mirrors.
Changelog & minor cleanup
CHANGELOG.md, src/lib/url.ts
Changelog updated; extracted URL helpers and small refactors documented.

Sequence Diagram

sequenceDiagram
    participant App as Download<br/>App
    participant Eng as Download<br/>Engine
    participant DB as SQLite<br/>Store
    participant Bridge as Tauri<br/>Bridge
    participant UI as Frontend<br/>UI

    App->>Eng: Start download (with mirrors)

    rect rgba(100, 150, 200, 0.5)
    Note over Eng: Failover loop
    Eng->>Eng: Attempt mirror[0] (run_mirror_attempt)
    alt success
        Eng->>App: DownloadCompleted
        Eng->>Bridge: Emit completed event
    else failure
        Eng->>DB: remove previous .vortex-meta / partial file
        Eng->>App: MirrorSwitched(id,index,url)
        Eng->>Eng: Attempt next mirror
    end
    alt all exhausted
        Eng->>App: DownloadFailed
        Eng->>Bridge: Emit failed event
    end
    end

    Bridge->>UI: Emit "mirror-switched" (payload)
    UI->>UI: Render MirrorsSection (active + alternatives)

    Eng->>DB: Persist mirrors & current_mirror_index
    DB->>Eng: Acknowledged
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly Related PRs

Suggested Labels

ui

"I hopped through hosts and lined them up,
mirrors sorted neat, priorities set high;
when one falls short I nudge the next in line,
a tidy switch, a saved file — downloads sigh with joy."

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(download): metalink mirrors fallback in engine (task 40)' directly and clearly describes the main change: adding Metalink mirror failover support to the download engine. It is specific, concise, and accurately reflects the primary objective of this changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/task-40-metalink-mirrors

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4871c71d93

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/adapters/driven/network/download_engine.rs
Comment thread src-tauri/src/adapters/driven/network/download_engine.rs
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src-tauri/src/domain/model/download.rs (1)

243-277: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize persisted mirrors in reconstruct().

Line 248 only clamps the cursor. It never reapplies the two new mirror invariants from set_mirrors(), so a stale/manual DB row can come back unsorted and with more than MAX_MIRRORS_PER_DOWNLOAD entries. After restart, the engine can then fail over in the wrong order and still walk an unbounded mirror tail.

💡 Suggested fix
-        mirrors: Vec<Mirror>,
+        mut mirrors: Vec<Mirror>,
         current_mirror_index: u32,
         created_at: u64,
         updated_at: u64,
     ) -> Self {
+        sort_mirrors_by_priority(&mut mirrors);
+        mirrors.truncate(MAX_MIRRORS_PER_DOWNLOAD);
+
         // Clamp the persisted index to a valid slot — a corrupted DB row
         // pointing past the end of the list must not panic the engine.
         let clamped_index = if mirrors.is_empty() {
             0
         } else {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src-tauri/src/domain/model/download.rs` around lines 243 - 277, reconstruct()
currently only clamps current_mirror_index but doesn't reapply the invariants
enforced by set_mirrors(): mirrors must be sorted (preferred-first) and limited
to MAX_MIRRORS_PER_DOWNLOAD; update reconstruct() to normalize the persisted
mirrors vector before assigning it to the struct by sorting mirrors using the
same comparison used in set_mirrors() (or call/shared helper used by
set_mirrors()) and truncating to MAX_MIRRORS_PER_DOWNLOAD, then recompute/clamp
current_mirror_index against the normalized length so the engine resumes with a
bounded, correctly-ordered mirrors list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src-tauri/src/adapters/driven/sqlite/download_read_repo.rs`:
- Around line 299-307: Persisted current_mirror_index may be out-of-range for
the deserialized mirrors list; before constructing the DTO fields (the mirrors
vec built from download::deserialize_mirrors and the current_mirror_index
assigned via safe_u32(model.current_mirror_index as i64)), clamp the index into
the valid range: if mirrors.is_empty() use 0, otherwise convert the stored index
to usize and cap it to mirrors.len()-1 (e.g. use min/max) and then set
current_mirror_index from that clamped value so the active mirror is always
defined.

---

Outside diff comments:
In `@src-tauri/src/domain/model/download.rs`:
- Around line 243-277: reconstruct() currently only clamps current_mirror_index
but doesn't reapply the invariants enforced by set_mirrors(): mirrors must be
sorted (preferred-first) and limited to MAX_MIRRORS_PER_DOWNLOAD; update
reconstruct() to normalize the persisted mirrors vector before assigning it to
the struct by sorting mirrors using the same comparison used in set_mirrors()
(or call/shared helper used by set_mirrors()) and truncating to
MAX_MIRRORS_PER_DOWNLOAD, then recompute/clamp current_mirror_index against the
normalized length so the engine resumes with a bounded, correctly-ordered
mirrors list.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 64a3fa33-cfcb-4009-9172-05a8c99d4e0d

📥 Commits

Reviewing files that changed from the base of the PR and between 660f998 and 4871c71.

📒 Files selected for processing (32)
  • CHANGELOG.md
  • src-tauri/src/adapters/driven/event/tauri_bridge.rs
  • src-tauri/src/adapters/driven/logging/download_log_bridge.rs
  • src-tauri/src/adapters/driven/network/download_engine.rs
  • src-tauri/src/adapters/driven/notification/notification_bridge.rs
  • src-tauri/src/adapters/driven/sqlite/download_read_repo.rs
  • src-tauri/src/adapters/driven/sqlite/download_repo.rs
  • src-tauri/src/adapters/driven/sqlite/entities/download.rs
  • src-tauri/src/adapters/driven/sqlite/migrations/m20260505_000009_add_mirrors.rs
  • src-tauri/src/adapters/driven/sqlite/migrations/mod.rs
  • src-tauri/src/adapters/driven/sqlite/progress_bridge.rs
  • src-tauri/src/application/commands/change_directory.rs
  • src-tauri/src/application/queries/get_download_detail.rs
  • src-tauri/src/application/read_models/download_detail_view.rs
  • src-tauri/src/application/services/history_backfill.rs
  • src-tauri/src/domain/event.rs
  • src-tauri/src/domain/model/download.rs
  • src-tauri/src/domain/model/mirror.rs
  • src-tauri/src/domain/model/mod.rs
  • src-tauri/src/domain/model/views.rs
  • src/hooks/__tests__/useDownloadDetail.test.ts
  • src/lib/url.ts
  • src/types/download.ts
  • src/views/DownloadDetailsPanel/DownloadDetailsPanel.tsx
  • src/views/DownloadDetailsPanel/MirrorsSection.tsx
  • src/views/DownloadDetailsPanel/SourceInfoSection.tsx
  • src/views/DownloadDetailsPanel/__tests__/DownloadDetailsPanel.test.tsx
  • src/views/DownloadDetailsPanel/__tests__/FileInfoSection.test.tsx
  • src/views/DownloadDetailsPanel/__tests__/IntegritySection.test.tsx
  • src/views/DownloadDetailsPanel/__tests__/MetricsSection.test.tsx
  • src/views/DownloadDetailsPanel/__tests__/MirrorsSection.test.tsx
  • src/views/DownloadsView/__tests__/DownloadsView.test.tsx

Comment thread src-tauri/src/adapters/driven/sqlite/download_read_repo.rs Outdated
@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented May 5, 2026

Merging this PR will degrade performance by 15.06%

❌ 4 (👁 4) regressed benchmarks
✅ 22 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
👁 detect_sha256 705.3 ns 792.8 ns -11.04%
👁 detect_md5 570 ns 657.5 ns -13.31%
👁 reject_invalid 493.6 ns 581.1 ns -15.06%
👁 full_lifecycle 225.6 ns 254.7 ns -11.45%

Comparing feat/task-40-metalink-mirrors (cd1f304) with main (660f998)

Open in CodSpeed

- Persist current_mirror_index via progress_bridge subscriber on
  MirrorSwitched, so the read model surfaces the live mirror after
  runtime failover instead of the stale slot 0 from the row.
- Wipe .vortex-meta + partial file before each mirror retry in the
  failover loop. The meta-aware branch in run_mirror_attempt would
  otherwise collide with create_new(true) and sink the retry.
- Clamp the persisted current_mirror_index against mirrors.len() in
  download_read_repo so a row with mirrors_json shrunk manually
  cannot surface an out-of-range slot.

Two new progress_bridge tests cover the cursor write-through and
event-to-message mapping.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
CHANGELOG.md (1)

12-12: 💤 Low value

Consider splitting this entry into three separate bullets for easier scanning.

This single bullet describes three distinct issues (cursor persistence, meta collision, cursor clamping). While bundling them under "PR #153 review fixes" is logical, separating them would help users scanning the changelog quickly locate specific fixes. For example:

  • Task 40 follow-up — mirror cursor persistence (PR #153)
  • Task 40 follow-up — meta file collision on retry (PR #153)
  • Task 40 follow-up — cursor clamping on read (PR #153)

The current format is acceptable and consistent with the project's detailed style, but the suggested split would improve discoverability.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@CHANGELOG.md` at line 12, Split the long CHANGELOG bullet into three separate
bullets each focused on one fix: 1) mirror cursor persistence — mention
progress_bridge.rs subscribing to DomainEvent::MirrorSwitched and persisting
current_mirror_index (include test
progress_bridge::test_update_mirror_cursor_persists_new_index), 2) meta file
collision on retry — describe the change in the failover loop in start() and the
wipe-before-continue behavior that prevents storage.create_file collisions in
run_mirror_attempt, and 3) cursor clamping on read — note the defensive clamp
added in download_read_repo::find_download_detail (include test
progress_bridge::test_event_to_message_maps_mirror_switched if relevant). Keep
each bullet short, use the same PR reference (PR `#153`) and task/context tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@CHANGELOG.md`:
- Line 12: Split the long CHANGELOG bullet into three separate bullets each
focused on one fix: 1) mirror cursor persistence — mention progress_bridge.rs
subscribing to DomainEvent::MirrorSwitched and persisting current_mirror_index
(include test progress_bridge::test_update_mirror_cursor_persists_new_index), 2)
meta file collision on retry — describe the change in the failover loop in
start() and the wipe-before-continue behavior that prevents storage.create_file
collisions in run_mirror_attempt, and 3) cursor clamping on read — note the
defensive clamp added in download_read_repo::find_download_detail (include test
progress_bridge::test_event_to_message_maps_mirror_switched if relevant). Keep
each bullet short, use the same PR reference (PR `#153`) and task/context tags.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 480f16e0-d570-4f48-bc3d-0cf53224ec6d

📥 Commits

Reviewing files that changed from the base of the PR and between 4871c71 and 5838786.

📒 Files selected for processing (4)
  • CHANGELOG.md
  • src-tauri/src/adapters/driven/network/download_engine.rs
  • src-tauri/src/adapters/driven/sqlite/download_read_repo.rs
  • src-tauri/src/adapters/driven/sqlite/progress_bridge.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src-tauri/src/adapters/driven/sqlite/download_read_repo.rs
  • src-tauri/src/adapters/driven/network/download_engine.rs

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5838786e9b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/adapters/driven/network/download_engine.rs
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 32 files

After mirror exhaustion the round-1 cursor-persist fix left the
persisted current_mirror_index at the last slot. The next retry
resumed there and skipped the higher-priority mirrors that might
have recovered.

progress_bridge now maps DownloadFailed to MirrorCursorReset and
writes current_mirror_index = 0 through the same single-task
channel that handles segment / progress / cursor writes, so the
reset lands in event-bus order.

Two regression tests cover the event mapping and the write-through.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec6d2ad4ce

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/adapters/driven/network/download_engine.rs
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 32 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src-tauri/src/adapters/driven/sqlite/download_repo.rs">

<violation number="1" location="src-tauri/src/adapters/driven/sqlite/download_repo.rs:91">
P2: Updating `current_mirror_index` in the generic upsert path introduces a race that can overwrite newer event-driven mirror cursor writes with stale state.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread src-tauri/src/adapters/driven/sqlite/download_repo.rs Outdated
…psert

PR #153 review round 3 — two findings.

(1) Failover loop only wiped the file + .vortex-meta sidecar; segment
rows in download_segments survived. The next mirror could pick a
different plan (no range support, fewer splits) and the read model
would carry phantom / Error segments through detail panel + list
counts. progress_bridge now extends its MirrorSwitched handler with
a DELETE FROM download_segments WHERE download_id = ? step that runs
through the same single-task channel, so the purge lands in event-bus
order before the next attempt's SegmentStarted writes.

(2) Round-1 fix added CurrentMirrorIndex to download_repo's upsert
update_columns, racing with the event-driven cursor writes from
progress_bridge — a generic save() carrying a stale cursor could
clobber the live failover position. CurrentMirrorIndex is now
excluded from update_columns alongside the other bridge-owned
columns (SpeedBytesPerSec, DownloadedBytes).

One regression test for the segments purge + cross-id isolation.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98cc9c86f5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/adapters/driven/sqlite/progress_bridge.rs
Round-3 handler cleared the cursor + segments but left
downloaded_bytes / total_bytes pinned at the failed mirror's last
value. The MAX guard on update_download_progress would then swallow
the new mirror's events until they crossed the high-water mark,
freezing the UI progress bar.

progress_bridge now adds a reset_download_bytes step that zeroes
downloaded_bytes and clears total_bytes between the cursor write
and the segments purge. Next progress event repopulates total_bytes
through the existing NULLIF/COALESCE guard.

Two regression tests: zero-out write-through + round-trip with a
follow-up update_download_progress call.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src-tauri/src/adapters/driven/sqlite/progress_bridge.rs`:
- Around line 223-230: The handler currently maps every
DomainEvent::DownloadFailed to BridgeMessage::MirrorCursorReset (download_id ->
download_id) which incorrectly resets the mirror cursor for non-mirror terminal
failures (e.g., verify/extract errors emitted by extract_archive.rs); change the
logic in the match arm handling DomainEvent::DownloadFailed so it only emits
MirrorCursorReset when the failure reason is a mirror exhaustion/terminal mirror
error (or when the event carries an explicit mirror-failure variant), otherwise
do not reset the cursor — locate the DomainEvent::DownloadFailed match and
BridgeMessage::MirrorCursorReset construction and add a guard or pattern that
checks the failure kind (or propagate a specific failure-reason field from the
producer) before returning MirrorCursorReset.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 65e7fd6e-7d3e-461f-9815-0a8c20dc4280

📥 Commits

Reviewing files that changed from the base of the PR and between 98cc9c8 and 1a246ac.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • src-tauri/src/adapters/driven/sqlite/progress_bridge.rs

Comment thread src-tauri/src/adapters/driven/sqlite/progress_bridge.rs Outdated
Round-2 cursor-reset path mapped every DownloadFailed to a cursor
reset, but extract_archive and Download::fail() also publish
DownloadFailed for post-download errors. A successful download
from mirror N followed by an extract failure would zero the cursor
and lose the last-known-good slot — manual re-download then walks
the full mirror list from the top instead of going straight to the
mirror that already produced the bytes.

New DomainEvent::AllMirrorsExhausted { id } published by the
engine failover loop ahead of the existing DownloadFailed.
progress_bridge now keys MirrorCursorReset on the new event and
ignores DownloadFailed entirely. tauri_bridge maps it to a
mirrors-exhausted Tauri event; the log bridge stays silent on it.

Three tests cover the new mapping, the explicit non-reset on
generic DownloadFailed, and the persisted write-through.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 495410cbac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/adapters/driven/network/download_engine.rs
After AttemptOutcome::Failed, the failover loop bumped mirror_idx,
ran cleanup, and published MirrorSwitched unconditionally. A user
cancel arriving in the window between attempt completion and the
next iteration — including during the spawn_blocking file/meta
cleanup — would still persist a cursor advance through the bridge
before run_mirror_attempt observed the cancel signal.

Two cancel_token.is_cancelled() guards inside the failover branch:
one before bumping mirror_idx, one after cleanup completes. Both
emit DownloadCancelled and break, so no MirrorSwitched fires for
an aborted switch and the cursor stays where the user cancelled.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd1f304643

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src-tauri/src/adapters/driven/network/download_engine.rs
@mpiton mpiton merged commit edca498 into main May 6, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation frontend rust

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant