From b5d20c1e729b061c25717814a1f2b0a026aa926a Mon Sep 17 00:00:00 2001 From: ddidderr Date: Sat, 16 May 2026 08:50:51 +0200 Subject: [PATCH] fix(peer): refresh settled install state after operations The follow-up review found a few stale lifecycle edges around local game transactions. Recovery could sweep active roots, post-operation refreshes still re-ran full startup recovery, and the UI kept inferring local-only state from downloaded and installed flags instead of the backend availability. This updates the peer lifecycle so startup recovery skips active operations, install/update/uninstall refresh only the affected game after the operation guard is dropped, and path-changing game-directory updates are rejected while operations are active. It also removes the dead UpdateGame command, drops the unused manifest_hash write field while preserving old JSON reads, renames the internal install-finished event, and carries availability through the DB, peer summaries, Tauri refreshes, and the React model. The included follow-up documents record the review source, implementation decisions, and the remaining FOLLOW_UP_2.md work so later commits can stay small instead of reopening the completed plan items. Test Plan: - git diff --check - just fmt - just clippy - just test Follow-up-Plan: FOLLOW_UP_PLAN.md --- FOLLOW_UP_2.md | 68 +++ FOLLOW_UP_PLAN.md | 124 ++++++ IMPL_DECISIONS.md | 24 +- PLAN.md | 25 +- REVIEW_STEP_1.md | 118 +++++ REVIEW_STEP_2.md | 137 ++++++ REVIEW_STEP_3.md | 132 ++++++ REVIEW_STEP_4.md | 146 +++++++ crates/lanspread-compat/src/eti.rs | 3 +- crates/lanspread-db/src/db.rs | 26 +- crates/lanspread-peer/README.md | 2 +- crates/lanspread-peer/src/download.rs | 73 +++- crates/lanspread-peer/src/handlers.rs | 409 ++++++++++++++---- crates/lanspread-peer/src/install/intent.rs | 55 ++- .../lanspread-peer/src/install/transaction.rs | 67 ++- crates/lanspread-peer/src/lib.rs | 6 - crates/lanspread-peer/src/local_games.rs | 18 +- crates/lanspread-peer/src/peer_db.rs | 12 +- crates/lanspread-peer/src/remote_peer.rs | 17 +- crates/lanspread-proto/src/lib.rs | 2 + .../src-tauri/src/lib.rs | 36 +- crates/lanspread-tauri-deno-ts/src/App.tsx | 20 +- 22 files changed, 1389 insertions(+), 131 deletions(-) create mode 100644 FOLLOW_UP_2.md create mode 100644 FOLLOW_UP_PLAN.md create mode 100644 REVIEW_STEP_1.md create mode 100644 REVIEW_STEP_2.md create mode 100644 REVIEW_STEP_3.md create mode 100644 REVIEW_STEP_4.md diff --git a/FOLLOW_UP_2.md b/FOLLOW_UP_2.md new file mode 100644 index 0000000..38810e5 --- /dev/null +++ b/FOLLOW_UP_2.md @@ -0,0 +1,68 @@ +# Follow-up Plan #2 + +State of FOLLOW_UP_PLAN.md after two implementation rounds. Items here are what's still open. + +## Context for next time + +Branch `p2p-codex-muenchhausen` has uncommitted work that completes the bulk of FOLLOW_UP_PLAN.md (52 tests pass via `just test`). Specifically these items in the original plan are **done** and shouldn't be re-opened: #1, #2, #3, #4, #5, #6, #7, #8, #9, #15, #17, #20, plus parts of #12 (update success, version-ini begin/rollback, multi-eti sorted order, corrupt + mismatched-id intent JSON). + +The OperationGuard ordering fix in `handlers.rs` is the structurally most important change: install/update/uninstall now drop the guard _before_ the finish event and `refresh_local_game`, so peers see settled state in the next announcement instead of waiting for a scan tick. Tests `*_refreshes_settled_state_after_guard_release` pin this. + +## Still open + +### Correctness / runtime + +#### 1. Tauri-side `active_operations` has no reconciliation + +If a `PeerEvent::InstallGameFinished` / `…Failed` is dropped, the UI is stuck "installing" until app restart. Include an in-progress snapshot in `PeerEvent::LocalGamesUpdated` so the UI recomputes from authoritative state instead of accumulating from event history. (Was `#10` in original plan, deferred both rounds.) + +### Tests still missing + +#### 2. Install-recovery matrix (~10 rows) + +Only two intent-driven rows are covered: `Updating | no local | .local.installing | .local.backup` (`recovery_restores_backup_for_interrupted_update`) and the active-skip case (`startup_recovery_skips_active_game_roots`). The other ~9 combinations of `(intent_state ∈ {Installing, Updating, Uninstalling}, local present?, .local.installing present?, .local.backup present?)` are unverified. + +**Approach:** convert `recovery_restores_backup_for_interrupted_update` into a table-driven test. Iterate over the named matrix rows in PLAN.md, set up FS + intent, run `recover_game_root`, assert resulting FS + intent. + +#### 3. Update rollback on commit-rename failure + +Only extract-failure is tested (`update_failure_restores_previous_local`). The branch where extract succeeds but the staging→local rename fails is unexercised. Force it by making `local/` un-renamable (e.g. swap in a directory under that name post-extract) or by injecting a rename hook. + +#### 4. Uninstall delete-failure restore + +`restore_backup` rollback in `transaction.rs:107-114` is unreached by tests. + +#### 5. Installed-only → Ready rescan + +PLAN listed this. No test exercises the transition where a user drops `version.ini` into a `local/`-only game root and the next rescan flips it to Ready. + +#### 6. Scanner gating dispatch + +`handle_watch_event` and `RescanGate` / `run_gated_rescan` (`local_monitor.rs:208-287`) have zero tests. Cover: + +- Event for ID under active operation is dropped. +- Burst of events collapses to ≤2 rescans for the same ID. +- Sideload picked up by fallback scan. +- Non-catalog game produces no library index entry and no `LibraryDelta`. + +#### 7. Serve gating dispatch + +`local_download_available` predicate is tested; `handle_get_game_command`, `handle_get_game_file_data`, `handle_get_game_file_chunk` dispatch paths aren't. Small tests against an in-memory `Ctx` covering: non-catalog ID, mid-operation, missing sentinel. + +### Code hygiene + +#### 8. Consolidate `TempDir` test helper (getting worse) + +Reimplemented in **five** test modules now: `install/intent.rs`, `install/transaction.rs`, `local_games.rs`, `download.rs`, `handlers.rs`. Each round of new tests adds another copy. Move to a single `test_support` module (or use the `tempfile` crate, already stable). The longer this waits, the bigger the eventual consolidation diff. + +#### 9. `availability` field is `String`, not a typed enum + +`lanspread-db::Game::availability: String` with `AVAILABILITY_*` consts works but allows invalid values. `remote_peer::summary_from_game` already has a defensive fallback for unknown strings. A typed enum + serde would be tighter, but is a wire-format change — coordinate if/when other peers are upgraded. + +#### 10. `save_library_index` non-atomic + +`local_games.rs:141-148` writes without temp+rename. Corrupt index → next scan rebuilds, so blast radius is small. Match the intent-log atomic pattern if touching that module. + +#### 11. Split `download.rs` (1162 lines, growing) + +Mixes chunk planning, retry orchestration, version-sentinel transaction, in-memory buffer, and the main loop. Future split into `download/{transaction,chunks,orchestrator}.rs`. Pure cosmetic. diff --git a/FOLLOW_UP_PLAN.md b/FOLLOW_UP_PLAN.md new file mode 100644 index 0000000..4334e6e --- /dev/null +++ b/FOLLOW_UP_PLAN.md @@ -0,0 +1,124 @@ +# Follow-up Plan + +Distilled from REVIEW_STEP_1..4. Ordered by impact, not by review number. + +## Correctness / risk + +### 1. Recovery vs. in-flight downloads race + +`install::recover_on_startup` (`transaction.rs:118-156`) sweeps `.version.ini.tmp` / `.version.ini.discarded` for every game root without consulting `active_operations`. If `load_local_library` runs while a download is in-flight (e.g. `SetGameDir` mid-download), the sweep can delete files the download is renaming. + +**Fix:** in `recover_download_transients` (and the per-root recovery), skip game IDs present in `Ctx::active_operations`. + +### 2. Recovery re-runs on every state change + +`load_local_library` (and therefore `recover_on_startup`) runs at startup, on `SetGameDir`, and after every install/update/uninstall (`handlers.rs:487-497`). On a 100+ game library this is O(N) fs reads per operation for no gain — the just-completed transaction already cleared its own state. + +**Fix:** either run `recover_on_startup` only at startup + `SetGameDir`, or scope post-operation recovery to the single affected game ID. + +## Dead / misleading code + +### 3. `PeerCommand::UpdateGame` is unreachable + +Variant exists in `lib.rs:163` but has no sender. Tauri's `update_game` sends `GetGame`; auto-install uses `RequestedInstallOperation::Auto`. +**Fix:** remove the variant, or wire `update_game` to send it. + +### 4. `InstallIntent::manifest_hash` never populated + +Field defined in `intent.rs` but always `None`. +**Fix:** remove it, or add a one-line comment explaining it's reserved. + +### 5. `Availability::Downloading` never emitted + +`build_game_summary` only produces `Ready`/`LocalOnly`. Operation-table gating already covers in-progress state. +**Fix:** add a comment in the proto enum that `Downloading` is intentionally unreachable from `build_game_summary`, so the next implementor doesn't add a third gate. + +### 6. `RequestedInstallOperation::Auto` and `Install` collapse to the same thing + +`handlers.rs:355-363`: both pick `Updating` if `local/` exists, `Installing` otherwise. Three variants, two distinct behaviors. +**Fix:** merge `Auto`+`Install` into one variant (e.g. `Inferred`) or a bool flag. + +### 7. `partition_download_descriptions` doesn't actually partition + +`download.rs:110-137` keeps `version.ini` inside `transfer_descs` and only additionally extracts a `version_desc`. The runtime routes sentinel chunks via `version_buffer.matches(...)`, not by list separation. +**Fix:** rename to reflect what it does (e.g. `extract_version_descriptor`), or actually split into two lists. + +## UI / event naming + +### 8. Rename `game-unpack-finished` → `game-install-finished` + +There is no longer an "unpack" stage distinct from "install". The event is internal; no external consumer. Update both `src-tauri/src/lib.rs:880-894` and `src/App.tsx:293-320`. + +### 9. UI `LocalOnly` is reverse-engineered from `installed && !downloaded` + +`build_game_summary` computes `Availability` then throws it away in `game_from_summary`. UI recomputes (`App.tsx:742-744`). Works today, but silently misses any future `Availability` variant. +**Fix:** add `availability: string` to the TS `Game` and serialize it end-to-end. + +### 10. Tauri-side `active_operations` has no reconciliation + +If a `PeerEvent::InstallGameFinished` / `…Failed` is dropped, the UI is stuck "installing" until app restart. +**Fix (defer):** include an in-progress snapshot in `PeerEvent::LocalGamesUpdated` so the UI can recompute from authoritative state instead of accumulating from event history. + +## Test breadth (largest gap) + +### 11. Install-recovery matrix — only 1 of ~11 rows tested + +Only `recovery_restores_backup_for_interrupted_update` (`transaction.rs:591`) exercises an intent-driven row. +**Fix:** convert to a table-driven test over `(intent_state, local, .local.installing, .local.backup)` combinations and assert resulting FS + intent for each. + +### 12. Missing transaction test cases + +- Successful **update** path (only failure path is tested). +- Update rollback on **commit-rename failure** (not extract failure). +- Uninstall **delete-failure restore** — `restore_backup` rollback in `transaction.rs:107-114`. +- `begin_version_ini_transaction` initial-rename behavior (`download.rs:193-206`). +- `rollback_version_ini_transaction` (`download.rs:208-221`). +- Multi-`.eti` download. +- Intent JSON: parse error / corrupt body / mismatched `id` field (only schema-mismatch is tested). +- Installed-only → Ready transition when user drops in a `version.ini`. + +### 13. Scanner gating: zero dispatch-level tests + +`handle_watch_event` (`local_monitor.rs:208-236`) and `RescanGate` / `run_gated_rescan` (`local_monitor.rs:261-287`) have no tests. Add: + +- Event for ID under active operation is dropped. +- Burst of events collapses to ≤2 rescans for the same ID. +- Sideload picked up by fallback scan. +- Non-catalog game produces no library index entry and no `LibraryDelta`. + +### 14. Serve gating: only predicates tested, not dispatch + +`local_download_available` is tested; `handle_get_game_command`, `handle_get_game_file_data`, `handle_get_game_file_chunk` dispatch paths are not. Add small tests against an in-memory `Ctx` covering: non-catalog ID, mid-operation, missing sentinel. + +### 15. Windows-path coverage for `is_version_ini` + +`db.rs:181-184` normalizes `\\`→`/`. Add one test with a backslash path. + +## Code hygiene + +### 16. Consolidate `TempDir` test helper + +Reimplemented in 5 files (`install/intent.rs:128`, `install/transaction.rs:498`, `local_games.rs:615`, `download.rs:926`, +variants). Move to a single `test_support` module or use the `tempfile` crate. + +### 17. Rename `partition_requires_exactly_one_root_version_ini` + +The test's "duplicate" case is actually a nested-decoy case under the new tight predicate. Split into two named tests: one for nested-decoy ignored, one for true duplicates rejected. + +### 18. Split `download.rs` (1162 lines) + +Mixes chunk planning, retry orchestration, version-sentinel transaction, in-memory buffer, and the main loop. Future split into `download/{transaction,chunks,orchestrator}.rs`. + +### 19. `save_library_index` is not atomic + +`local_games.rs:141-148` writes without temp+rename. Blast radius is small (corrupt index → next scan rebuilds), but matching the intent-log atomic pattern would be cheap. + +## Documentation + +### 20. Add undocumented decisions to IMPL_DECISIONS.md + +Observed but not recorded: + +- `partition_download_descriptions` keeps version.ini in `transfer_descs` (see #7). +- `recover_on_startup` re-runs on every `load_local_library` (see #2). +- `PeerCommand::UpdateGame` left as dead variant (see #3). +- `Auto`/`Install` collapse in `run_install_operation` (see #6). diff --git a/IMPL_DECISIONS.md b/IMPL_DECISIONS.md index 8696cce..01c23cb 100644 --- a/IMPL_DECISIONS.md +++ b/IMPL_DECISIONS.md @@ -3,18 +3,30 @@ - Added a `just test` recipe so unit tests can be run through the repository's required `just ...` command surface instead of invoking `cargo test` directly. -- Kept the existing frontend `game-unpack-finished` event name for successful - transactional installs. The peer now emits install lifecycle events, but the - compatibility event still lets the UI reuse its existing "install complete" - path. +- Renamed the frontend success event to `game-install-finished`; the old + unpack name no longer matched the transactional install/update lifecycle. - Implemented watcher rescans by reusing the existing `.lanspread/library_index.json` cache and updating a single game entry in that index. This satisfies the per-ID optimized rescan requirement without adding a second cache format. +- Split full startup recovery from ordinary settled refreshes. Startup and real + `SetGameDir` changes run recovery plus a scan; install/update/uninstall + completion only rescans the affected game after operation tracking has been + cleared. +- Rejected path-changing `SetGameDir` while operations are active. Same-path + refreshes are allowed and deliberately skip full recovery so they cannot sweep + download transients for in-flight work. - Kept a separate `active_downloads` cancellation-token map next to the single `active_operations` table. The operation table is the authoritative state for gates; the token map is only cancellation plumbing for in-flight downloads. - Treated a downloaded-but-not-installed game as immediately installable from Tauri by sending `PeerCommand::InstallGame` directly. A not-downloaded game still uses `GetGame`, and the peer auto-installs after the sentinel commit. -- Derived the UI's `LocalOnly` badge from `installed && !downloaded` because the - UI-facing `Game` type does not carry the protocol-level `Availability` enum. +- Removed the dead internal `PeerCommand::UpdateGame` path. The UI update button + intentionally sends `GetGame`, and the peer infers install versus update from + the presence of `local/` after archives are available. +- Kept `Availability::Downloading` in the wire protocol for compatibility, but + local summaries do not emit it today because active operations are gated out + of scans and serving decisions. +- Threaded availability through the UI-facing `Game` payload so `LocalOnly` + rendering follows backend state instead of reverse-engineering it from + `installed && !downloaded`. diff --git a/PLAN.md b/PLAN.md index 1475521..32e55a4 100644 --- a/PLAN.md +++ b/PLAN.md @@ -65,10 +65,10 @@ installed = local/ is a directory The two predicates are independent. -- `installed && downloaded` → Ready, fully shareable. -- `installed && !downloaded` → installed locally; peer announces in deltas but `availability = LocalOnly` (refuses to serve archives). -- `!installed && downloaded` → ready to install; UI shows pending; `eti_version` is surfaced from `version.ini`. -- `!installed && !downloaded` → blank slate. +- `installed && downloaded` → Ready, fully shareable. +- `installed && !downloaded` → installed locally; peer announces in deltas but `availability = LocalOnly` (refuses to serve archives). +- `!installed && downloaded` → ready to install; UI shows pending; `eti_version` is surfaced from `version.ini`. +- `!installed && !downloaded` → blank slate. `is_committed_install` (the predicate) requires `local/` to be a **directory**, not merely "a path that exists." This handles symlinks, leftover regular files named `local`, etc. @@ -82,8 +82,7 @@ Per-game JSON at `/.lanspread.json`: "id": "...", "recorded_at": , "state": "None" | "Installing" | "Updating" | "Uninstalling", - "eti_version": "20251028"?, - "manifest_hash": ? + "eti_version": "20251028"? } ``` @@ -98,7 +97,7 @@ Per-game JSON at `/.lanspread.json`: ### Intent transitions -- **Begin operation**: write intent with new state *before* any FS mutation. +- **Begin operation**: write intent with new state _before_ any FS mutation. - **Commit operation**: write intent with `None` immediately after the commit rename, before emitting events or starting cleanup. - **Cleanup** (e.g., deleting `.local.backup` after a successful update): best-effort, after intent flush and event emit. Failure logs but doesn't fail the operation. @@ -173,7 +172,7 @@ A failed download is destructive to the previous downloaded state but leaves `lo 1. Write intent `Installing`. 2. Create `.local.installing/`, drop `.lanspread_owned` marker. 3. Extract archives into `.local.installing/`. Non-zero unrar exit ⇒ error ⇒ rollback. -4. `rename(.local.installing, local)`. ← commit point. +4. `rename(.local.installing, local)`. ← commit point. 5. Write intent `None`. 6. Emit success event; trigger rescan. @@ -185,7 +184,7 @@ A failed download is destructive to the previous downloaded state but leaves `lo 2. `rename(local, .local.backup)`; drop `.lanspread_owned` marker inside. 3. Create `.local.installing/`, drop marker. 4. Extract new archive into `.local.installing/`. Non-zero unrar exit ⇒ error ⇒ rollback. -5. `rename(.local.installing, local)`. ← commit point. +5. `rename(.local.installing, local)`. ← commit point. 6. Write intent `None`. 7. Emit success event; trigger rescan. 8. Best-effort `rm -rf .local.backup`; failure logged, not fatal. @@ -253,7 +252,7 @@ Catalog enforcement points inside the peer: - `scan_local_library` and the optimized rescan: a game root whose ID is not in the catalog is skipped — no `GameSummary` emitted, no entry in the library index, no contribution to `library_rev`. The intent log and recovery still operate on it (so we don't corrupt on-disk state for an unknown ID), but it is invisible to peers and the UI. - `local_download_available` and the file-stream handlers (see "Operation table" below): refuse all serve requests for non-catalog IDs. -- `InstallGame` / `UpdateGame` / `UninstallGame` commands for non-catalog IDs are rejected with an error; the peer logs and emits no events. +- `InstallGame` / `UninstallGame` commands for non-catalog IDs are rejected with an error; the peer logs and emits no events. This is also why the watcher reacting to a new game root cannot announce it without consulting the catalog — events for non-catalog IDs flow through gating but never become deltas. @@ -269,7 +268,7 @@ This is also why the watcher reacting to a new game root cannot announce it with - Sweep `.version.ini.discarded`. - Remove `Downloading` from the operation table. - `download_chunk` for version.ini paths: writes to memory rather than disk. -- New `PeerCommand` variant: `InstallGame { id }` — runs the install transaction over an already-downloaded game (auto-fired by the peer when a download commits; can also be fired by Tauri for explicit re-install). Plus `UpdateGame { id }` and `UninstallGame { id }`. +- New `PeerCommand` variant: `InstallGame { id }` — runs the install transaction over an already-downloaded game (auto-fired by the peer when a download commits; can also be fired by Tauri for explicit re-install). Plus `UninstallGame { id }`. ### Operation table @@ -369,8 +368,8 @@ Non-fatal. Log a warning; the fallback scan is sufficient on its own. - `game-install-failed` — extraction or commit-rename failed, rollback complete. - `game-uninstall-begin`, `game-uninstall-finished`, `game-uninstall-failed`. - Existing `game-download-finished` fires when the `version.ini` rename commits (which happens inside `download_game_files`, before the auto-fired `InstallGame`). -- Existing `game-unpack-finished` fires only on true transactional install success (peer-driven). It does NOT fire on extract failure or commit-rename failure — those route to `game-install-failed`. -- **Uninstall affordance**: secondary action (icon button or row-context menu), *not* a mode of the main button. Main button stays Install / Open. +- `game-install-finished` fires only on true transactional install success (peer-driven). It does NOT fire on extract failure or commit-rename failure — those route to `game-install-failed`. +- **Uninstall affordance**: secondary action (icon button or row-context menu), _not_ a mode of the main button. Main button stays Install / Open. - Surface `eti_version` for games that are downloaded but not installed. - Render the `LocalOnly` availability badge for installed-but-not-downloaded games: the user can play them locally but the row should communicate that this peer can't serve archives. diff --git a/REVIEW_STEP_1.md b/REVIEW_STEP_1.md new file mode 100644 index 0000000..af3d6be --- /dev/null +++ b/REVIEW_STEP_1.md @@ -0,0 +1,118 @@ +# Review Step 1 — Faithfulness to PLAN.md + +Scope: latest three commits (`6c8a2bb`, `c5dfbf9`, `fce34c7`) against `PLAN.md`. + +## Verdict + +The implementation is **substantially faithful** to PLAN.md. The major design pillars — version.ini as the download sentinel, `local/` as the install predicate, the per-game intent log, transactional install/update/uninstall, version.ini-only download transaction, operation table gating, catalog filtering, notify-based watcher with per-ID gating, and the move of install logic from Tauri into `lanspread-peer::install` — are all present and structurally match the plan. + +There are a handful of small deviations and a few items that don't quite line up with the wording in PLAN.md. None are correctness regressions; some are worth following up. + +## What lines up cleanly + +### State model +- `local_games::version_ini_is_regular_file` (`crates/lanspread-peer/src/local_games.rs:40`) and `local_games::local_dir_is_directory` (`local_games.rs:32`) implement the plan's two independent predicates. `is_committed_install` from the plan isn't named explicitly, but `local_dir_is_directory` enforces the "is-a-directory, not a path that exists" rule. +- `Availability::Ready` and `Availability::LocalOnly` are emitted from `build_game_summary` (`local_games.rs:328-368`) exactly as the plan specifies. `installed && !downloaded` correctly maps to `LocalOnly`. +- `is_version_ini` is tightened to a strict `/version.ini` match in `crates/lanspread-db/src/db.rs:181-184`, with a test (`db.rs:240`) that rejects nested `aoe2/local/version.ini`. + +### Intent log +- `.lanspread.json` is written atomically through `.lanspread.json.tmp` + rename + parent-dir fsync on Unix (`crates/lanspread-peer/src/install/intent.rs:86-100`). +- Schema mismatch / corruption / wrong ID all fall back to `None` (`intent.rs:68-83`), which matches the plan's "treat missing/unparseable as None" rule. + +### Transaction primitives +- `install::install`, `install::update`, `install::uninstall`, `install::recover_on_startup` in `crates/lanspread-peer/src/install/transaction.rs` follow the plan's step ordering (intent → FS work → commit rename → intent None → cleanup). +- `prepare_owned_empty_dir` / `prepare_backup_slot` refuse to touch markerless reserved dirs, matching the ownership-marker rule for the `None` intent case (`transaction.rs:354-375`). +- Recovery dispatches by recorded intent (`transaction.rs:148-156`) and matches every named row of the install matrix; unlisted combinations fall through to `_ => {}`, which is exactly the plan's "write intent matching FS observation; do not mutate FS" default. + +### Download transaction +- `begin_version_ini_transaction` parks an existing `version.ini` as `.version.ini.discarded` and clears stale scratch files (`crates/lanspread-peer/src/download.rs:193-206`). +- `commit_version_ini_buffer` writes `.version.ini.tmp`, fsyncs, renames to `version.ini`, fsyncs the parent on Unix, and sweeps `.version.ini.discarded` (`download.rs:223-240`). +- `prepare_game_storage` skips `is_version_ini()` entries (`download.rs:148-191`). +- Rollback never restores the previous sentinel (`download.rs:208-221`), matching the plan's explicit "downloads are destructive to prior downloaded state" rule. +- Recovery for download transients lives in `transaction::recover_download_transients` and is invoked both per-root in `recover_game_root` and once unconditionally over the game_dir top level (`transaction.rs:118-156`). + +### Catalog injection and gating +- `start_peer` receives `Arc>>` (`crates/lanspread-peer/src/lib.rs:189-215`); the Tauri side populates it from the bundled `game.db` in `ensure_bundled_game_db_loaded` (`crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs:711-721`). +- All four enforcement points the plan calls out are present: + - Scanner: `update_index_for_game` skips non-catalog IDs (`local_games.rs:394-449`). + - `local_download_available` checks catalog membership (`local_games.rs:48-66`). + - File-stream handlers in `services/stream.rs:293-362` route through `can_serve_game` and also reject any path that resolves under `local/`. + - Install/Update/Uninstall command handlers reject non-catalog IDs (`handlers.rs:342-345, 411-414`). + +### Operation table +- A single `active_operations: HashMap` carries all four states (`crates/lanspread-peer/src/context.rs:17-44`). +- `update_and_announce_games` preserves the previous summary for IDs under an active operation (`handlers.rs:521-538`), and `handle_list_games` filters them out (`services/stream.rs:155-163`). +- Watcher events for IDs with an active operation are dropped (`services/local_monitor.rs:229-236`). + +### Aggregation (peer_db) +- `get_all_games`, `peers_with_game`, `peers_with_latest_version`, `get_latest_version_for_game`, and even `majority_game_size` filter on `availability == Ready` via `game_is_ready` (`peer_db.rs:268, 322, 380, 394, 420, 450, 726`). The plan's three explicit aggregation rules all hold. + +### Scanner + watcher +- `notify` 7.x is added to `Cargo.toml` and consumed in `services/local_monitor.rs`. +- Non-recursive watches on `game_dir` and each game root, plus a reconcile pass on every tick (`local_monitor.rs:107-173`). +- Per-ID gating uses a `RescanGate { running, pending }` structure with no time-based debounce (`local_monitor.rs:32-36, 238-287`). +- Fallback scan runs every `LOCAL_GAME_FALLBACK_SCAN_SECS`; verified `300` in `config.rs` would be the expected value (not re-read here but the README and ARCHITECTURE both call out 300s). +- `should_ignore_game_child` covers `.local.*`, `.version.ini.*`, `.lanspread`, `.lanspread.json`, `.sync`, `.softlan_game_installed` (`local_monitor.rs:322-330`). +- Watcher init failure is non-fatal — `build_watch_state` returns `None` and logs a warning (`local_monitor.rs:71-105`); the fallback scan continues to run. + +### Tauri / UI +- `unpack_game` / `do_unrar` standalone command is removed; `do_unrar` survives only as the body of `SidecarUnpacker::unpack` (`src-tauri/src/lib.rs:58-66, 643-684`). +- The `___TO_BE_DELETE___{id}` whole-folder backup scheme is gone. +- `update_game_installation_state` (`src-tauri/src/lib.rs:348-389`) no longer requires `downloaded` to flag a game as installed. +- New events `game-install-begin`, `game-install-failed`, `game-uninstall-begin`, `game-uninstall-finished`, `game-uninstall-failed` are wired both Rust-side (`src-tauri/src/lib.rs:860-953`) and TS-side (`src/App.tsx:390-457`). +- `InstallStatus.Installing` and `InstallStatus.Uninstalling` added (`src/App.tsx:22-29`); `LocalOnly` badge rendered for installed-but-not-downloaded (`src/App.tsx:742-744`); secondary uninstall button is present and only shown when installed and idle (`src/App.tsx:766-778`). +- `install_game` Tauri command routes by local state (`src-tauri/src/lib.rs:88-128`): missing `version.ini` → `GetGame`; downloaded but no `local/` → `InstallGame`; already installed → no-op. Matches the plan. +- `update_game(id)` is a thin wrapper around `PeerCommand::GetGame` (`src-tauri/src/lib.rs:131-157`). +- New `uninstall_game` Tauri command forwards to `PeerCommand::UninstallGame` (`src-tauri/src/lib.rs:159-187`). + +## Where it deviates from PLAN.md + +### 1. Download partition is structural, not enforced +Plan: "Partition `game_file_descs` into version.ini and non-version.ini." + +Implementation (`download.rs:110-137`): `partition_download_descriptions` keeps `version.ini` *inside* `transfer_descs` and just additionally extracts a `version_desc`. The version.ini path then rides through `build_peer_plans` and `download_chunk` like any other file, where the runtime check `version_buffer.matches(&chunk.relative_path)` routes its chunks to memory instead of disk. Functionally equivalent, but the implementation does not actually partition; the variable name `transfer_descs` is misleading. `prepare_game_storage` is what actually keeps the sentinel off disk. + +This is fine, but a future maintainer reading the plan and looking for "two plans, one for archives, one for the sentinel" won't find it. + +### 2. Auto-install via `Auto`/`Install` collapsing +Plan: "Peer auto-fires `InstallGame { id }` (see install transaction)" after a successful download commit. + +Implementation: `handle_download_game_files_command` (`handlers.rs:268-281`) calls `run_install_operation(..., RequestedInstallOperation::Auto)` directly — it doesn't actually re-enter the command loop with a `PeerCommand::InstallGame`. The `Auto` variant then collapses to `Installing` or `Updating` depending on whether `local/` exists (`handlers.rs:336-372`). End behavior is correct, but the dispatch path is different from "fire a command." Worth noting because: + - `PeerCommand::UpdateGame` (`lib.rs:163`) exists but never has a sender — Tauri's `update_game` always sends `GetGame`, and the auto-install path uses `RequestedInstallOperation::Auto`. The variant is technically dead code today. + +### 3. `recover_on_startup` is re-run on every `load_local_library` +`load_local_library` runs both at startup and on every `SetGameDir`, install, update, and uninstall (`handlers.rs:487-497`). Each call re-sweeps `.version.ini.tmp`/`.version.ini.discarded` across every game root and re-reads every intent file. Plan describes recovery as a startup pass. + +This is not a correctness issue (recovery is idempotent and operation-table gating prevents touching games mid-operation), but on a 100-game directory it adds an O(N) read-dir + a pile of intent reads on every state-changing command. Worth flagging. + +### 4. `recover_on_startup` doesn't filter `local_only` recovery from announcements +PLAN: "The intent log and recovery still operate on it (so we don't corrupt on-disk state for an unknown ID)." + +Implementation: `recover_on_startup` (`transaction.rs:118-143`) walks every dir under `game_dir` and runs the matrix for each ID, ignoring catalog membership. That matches the plan. ✓ + +### 5. Order of `write_intent` vs rollback in `uninstall` +PLAN order: +1. Write intent Uninstalling +2. rename(local, .local.backup); drop marker +3. rm -rf .local.backup +4. Write intent None +5. Emit success event + +Implementation (`transaction.rs:93-116`) on step-3 failure calls `restore_backup` *before* writing `None`. Plan says "**Rollback** (step 3 fails): attempt `rename(.local.backup, local)`; surface error." It does not specify intent ordering. The implementation does write `None` after the restore — but only on the success branch of the rollback. On the error-of-rollback path, the original error is wrapped and returned without writing `None`. That leaves the intent log saying `Uninstalling` if rollback also fails, which is conservative (next startup will retry uninstall). This is consistent with the plan's "never delete unless we have positive proof of ownership" bias. + +### 6. `download_chunk` truncate guard +The `chunk.length == 0 && chunk.offset == 0` branch in `download.rs:377-380` truncates the file to zero. This isn't from the plan but was inherited from earlier code. With the new sentinel-last contract it's still correct, because version.ini chunks never reach this branch (they take the in-memory path above). + +### 7. `update_game_installation_state` still exists +Plan didn't mandate full deletion. The function is now uncoupled (good) but the comment about archives-without-sentinel still emits a log line and resets `local_version` to `None` when not downloaded. Tauri's UI-facing `Game` still gets `downloaded` and `installed` from this function on every `refresh_games_list`. Acceptable. + +## Items the plan called for but I didn't independently verify + +These are implemented but I am not 100% certain without running the suite: + +- The 300s constant (`LOCAL_GAME_FALLBACK_SCAN_SECS`) — only seen indirectly. +- mDNS/QUIC pipeline behavior unchanged in practice (no protocol bytes touched). + +## Summary + +PLAN.md is honored in spirit and almost entirely in letter. The deviations above are either cosmetic (partition naming), minor (recovery re-run cost), or vestigial (`PeerCommand::UpdateGame` dead variant). The core invariants — sentinel-last download, marker-or-intent-proves-ownership recovery, catalog-and-operation-gated serve, watcher-as-trigger with rescan gating, and the install state machine living in the peer crate — all hold. diff --git a/REVIEW_STEP_2.md b/REVIEW_STEP_2.md new file mode 100644 index 0000000..f26c9ea --- /dev/null +++ b/REVIEW_STEP_2.md @@ -0,0 +1,137 @@ +# Review Step 2 — On-the-fly Decisions (IMPL_DECISIONS.md) + +The implementor recorded six decisions made during implementation. Each is evaluated below. + +--- + +## 1. Added `just test` recipe + +> Added a `just test` recipe so unit tests can be run through the repository's required `just ...` command surface instead of invoking `cargo test` directly. + +**Verdict: good.** + +CLAUDE.md explicitly says "Never use normal `cargo ...` commands, use the `just ...` commands instead." A `just test` recipe was a missing primitive, and adding it (`justfile:22-23`: `cargo test --workspace`) closes that gap cleanly. The implementation is the smallest possible — a one-line wrapper that runs the workspace tests. Nothing to argue with. + +Tiny nit, not blocking: it doesn't take any args. If unit tests grow, you'll want `just test [args]` so the user can target a single test. That's a future polish. + +--- + +## 2. Kept `game-unpack-finished` event name for compatibility + +> Kept the existing frontend `game-unpack-finished` event name for successful transactional installs. The peer now emits install lifecycle events, but the compatibility event still lets the UI reuse its existing "install complete" path. + +**Verdict: defensible, but worth a follow-up.** + +The plan said `game-unpack-finished` "fires only on true transactional install success (peer-driven). It does NOT fire on extract failure or commit-rename failure — those route to `game-install-failed`." Implementation matches this: `PeerEvent::InstallGameFinished` is bridged to `game-unpack-finished` in `src-tauri/src/lib.rs:880-894`, and `InstallGameFailed` is bridged to `game-install-failed`. So the contract is correct. + +What's questionable is the *name*. There is no longer an "unpack" stage that's distinct from "install" — they're one transaction. A name like `game-install-finished` would be more honest. Keeping the old name makes the React side's `setupUnpackListener` (`src/App.tsx:293-320`) look stale: it's now an install-finished listener wearing an unpack-finished label. + +The compatibility argument is thin because there is no external consumer of this event — it's an internal name shared between Rust and TS in this repo. The TS side could be renamed in the same change. + +That said: this is a cosmetic decision and the implementor correctly noted it. Not a blocker. Plan said the event "fires on transactional install success" without prescribing a name. Recommend renaming in a follow-up, but the decision is acceptable as written. + +--- + +## 3. Reused `.lanspread/library_index.json` for per-ID rescan + +> Implemented watcher rescans by reusing the existing `.lanspread/library_index.json` cache and updating a single game entry in that index. This satisfies the per-ID optimized rescan requirement without adding a second cache format. + +**Verdict: good call.** + +The plan said: + +> ### Optimized rescan (single ID) +> 1. Stat `version.ini` (existence + mtime), check `local/` is-a-directory, stat any `*.eti` files at root level. +> 2. Compare to cached fingerprint. +> 3. Recursive walk only on fingerprint change. + +It didn't prescribe a storage format — only behavior. Reusing the existing index file (`local_games.rs:111-148`) is the right call: one cache, one schema, one path to keep coherent. `update_index_for_game` (`local_games.rs:394-449`) stats the fingerprint, short-circuits on equality, and rebuilds only when the fingerprint differs. `rescan_local_game` (`local_games.rs:568-589`) is the single-ID entry point. + +Two minor concerns: +- `save_library_index` (`local_games.rs:141-148`) does *not* use the atomic temp+rename pattern. The plan reserves atomic writes for the intent log, so this is consistent — but the index file is at risk of corruption on a crash mid-write. The blast radius is small: corrupt index → rescan rebuilds from scratch on next load. So this is acceptable, just worth knowing. +- The index file's `revision` is incremented inside both `scan_local_library` and `rescan_local_game`. If both paths race (fallback scan running while a watcher-triggered rescan finishes), the in-memory views could diverge briefly. They're each protected by reading and writing the same path with no global lock. In practice the gate in `local_monitor.rs:32-36` prevents concurrent rescans for the *same* ID but not across IDs vs. fallback. Worth a thought; not a correctness bug because the next scan reconciles. + +These caveats don't change the verdict. + +--- + +## 4. Separate `active_downloads` cancellation-token map + +> Kept a separate `active_downloads` cancellation-token map next to the single `active_operations` table. The operation table is the authoritative state for gates; the token map is only cancellation plumbing for in-flight downloads. + +**Verdict: pragmatic.** + +The plan called for a single per-peer operation table. The implementation has it (`active_operations`) and additionally keeps `active_downloads: HashMap` for cancellation tokens (`context.rs:38`). + +This is justifiable: `CancellationToken` doesn't fit naturally inside `HashMap` because `OperationKind` is `Copy`. Storing the token alongside as a separate map keeps `OperationKind` simple and lets the gate-path (which only needs presence/kind) stay cheap. The download operation guard (`OperationGuard::download`, `context.rs:142-153`) clears both maps on drop. + +Alternative: store `OperationState { kind: OperationKind, cancel: Option }`. Cleaner conceptually but invasive. The current shape is fine. + +One thing to verify: the gate-path readers (`local_download_available`, watcher event handler, stream handlers) all check only `active_operations`. They don't need the token map. So the split doesn't leak. Good separation. + +--- + +## 5. Downloaded-but-not-installed routes directly to `InstallGame` + +> Treated a downloaded-but-not-installed game as immediately installable from Tauri by sending `PeerCommand::InstallGame` directly. A not-downloaded game still uses `GetGame`, and the peer auto-installs after the sentinel commit. + +**Verdict: matches the plan.** + +This decision is actually *prescribed* by PLAN.md — see the `install_game(id)` routing rules: + +> - `version.ini` absent → send `PeerCommand::GetGame { id }` (fetch first; peer auto-fires `InstallGame` on download commit). +> - `version.ini` present, `local/` absent → send `PeerCommand::InstallGame { id }` directly (no re-download). +> - `version.ini` present, `local/` present → no-op. + +Looking at `src-tauri/src/lib.rs:88-128`, the implementation matches the plan exactly. So this isn't really a "decision" — it's the plan. The implementor may have flagged it because they wanted to be explicit that they didn't add an intermediate download step. Either way, no concern. + +--- + +## 6. `LocalOnly` badge derived from `installed && !downloaded` + +> Derived the UI's `LocalOnly` badge from `installed && !downloaded` because the UI-facing `Game` type does not carry the protocol-level `Availability` enum. + +**Verdict: acceptable, but a smell worth noting.** + +The protocol-level `Availability` is computed in `build_game_summary` (`local_games.rs:352-356`), then thrown away when `game_from_summary` (`local_games.rs:370-387`) converts to `Game`. The TS side has no `availability` field, so the badge is recomputed from `installed && !downloaded` in `src/App.tsx:742-744`. + +This works because: +- `Ready` ⇔ `downloaded == true` +- `LocalOnly` ⇔ `installed && !downloaded` (because by construction we only emit `LocalOnly` when not downloaded) +- The third value `Availability::Downloading` is never emitted by `build_game_summary`. + +So the predicate `installed && !downloaded` covers every case `build_game_summary` will produce. It's correct *today*. + +The smell: if `Availability::Downloading` ever starts being emitted (or if a future state is added — e.g., `Quarantined`), the UI would silently miss it because the TS side reverse-engineers availability from two booleans. The single source of truth should flow through. A small, future-proofing follow-up is to add `availability: string` to the TS `Game` interface and serialize the protocol value end-to-end. + +Not a blocker for this change. + +--- + +## Other implementation decisions NOT recorded in IMPL_DECISIONS.md but observed + +These were made silently. The implementor may want to add them to the doc. + +### a. `partition_download_descriptions` keeps version.ini in `transfer_descs` + +Plan says "partition" into two lists; implementation keeps both in one list and uses an in-memory `version_buffer` to route chunks. The end behavior is correct; the naming is misleading. See REVIEW_STEP_1 §1. + +### b. `recover_on_startup` re-runs on every `load_local_library` + +Re-runs on `SetGameDir` and after every install/uninstall. Plan describes it as a one-shot startup. Likely intentional ("recovery is also a safety net for `SetGameDir`"), but never written down. See REVIEW_STEP_1 §3. + +### c. `PeerCommand::UpdateGame` is dead + +The variant exists but has no sender path. The auto-install path uses `RequestedInstallOperation::Auto` and the Tauri `update_game` command sends `GetGame`. Either remove the variant or wire `update_game` to send it. + +### d. `Auto`/`Install` collapse in `run_install_operation` + +`Auto` and `Install` both pick `Updating` when `local/` exists and `Installing` otherwise (`handlers.rs:356-363`). That means an explicit `PeerCommand::InstallGame` against an already-installed game becomes an update. The user-facing semantics are surprising (calling "Install" produces an update) but the `install_game` Tauri command shields the UI from this by checking `local/` first. Worth recording. + +--- + +## Summary + +The six recorded decisions are all defensible. Two have follow-up smells worth fixing later (the `game-unpack-finished` legacy name and the UI's reverse-engineered `LocalOnly` predicate). The other four are either pragmatic minor refinements (`just test`, `active_downloads` map split) or directly implement the plan (`InstallGame` routing). + +There are at least four additional decisions the implementor made but did not record. Recommend adding them to IMPL_DECISIONS.md so the rationale is preserved. diff --git a/REVIEW_STEP_3.md b/REVIEW_STEP_3.md new file mode 100644 index 0000000..eb4cb45 --- /dev/null +++ b/REVIEW_STEP_3.md @@ -0,0 +1,132 @@ +# Review Step 3 — Test Quality + +## Headline + +38 tests pass across the workspace (`just test`). The new tests cover most of the load-bearing primitives. **They are real tests — they exercise actual filesystem and atomic-rename behavior, not mocked surfaces — and the assertions are tight.** Coverage of the install state machine, the version-sentinel commit path, and aggregation gating is strong. + +The weakness is **coverage breadth**: PLAN.md's test plan listed roughly 25 specific scenarios, of which 13 are actually verified. The plan's "recovery: each row of the install matrix" item is only one row deep. Scanner gating and serve gating are only covered by unit tests of helper functions; the dispatch paths themselves are untested. + +## Detailed test-by-test assessment + +### Genuinely useful, well-designed + +These tests exercise real behavior with clear assertions and would catch regressions reliably. + +| Test | File | What makes it good | +|---|---|---| +| `install_success_promotes_staging_and_clears_intent` | `install/transaction.rs:537` | Runs a full install with a fake unpacker, asserts the commit-rename happened, intent flipped back to None, and no staging dir leaked. | +| `update_failure_restores_previous_local` | `install/transaction.rs:554` | Uses a deliberately failing unpacker, then asserts the old `local/old.txt` is still readable and both `.local.installing` and `.local.backup` are gone. Tests the full rollback path including the backup→local restore. | +| `uninstall_removes_only_local_install` | `install/transaction.rs:574` | Confirms uninstall removes `local/` but preserves `game.eti` and `version.ini`. Directly maps to PLAN.md's Assumption: "Uninstall removes only `local/`; archives and `version.ini` stay." | +| `recovery_restores_backup_for_interrupted_update` | `install/transaction.rs:591` | Stages a realistic crash scenario (intent=Updating, both `.local.installing` and `.local.backup` present with markers), runs recovery, asserts backup is restored to `local/`. Good integration of intent + FS state. | +| `none_recovery_leaves_markerless_reserved_dirs_untouched` | `install/transaction.rs:624` | The sideload / user-data safety test. Critical for the "never delete user files" invariant. | +| `download_recovery_sweeps_reserved_version_files` | `install/transaction.rs:637` | Both `.version.ini.tmp` and `.version.ini.discarded` get cleaned up. | +| `tmp_write_without_rename_leaves_previous_intent_intact` | `install/intent.rs:154` | Simulates a crash between tmp write and rename by writing the tmp file directly and never renaming. Asserts the old intent survives. Real test of atomic write semantics. | +| `schema_mismatch_is_treated_as_missing` | `install/intent.rs:183` | Writes schema_version=2 to disk, reads back, asserts state=None. | +| `scan_uses_version_ini_and_local_dir_as_independent_state` | `local_games.rs:650` | Parameterizes all three meaningful states (ready, local-only, eti-only) plus a non-catalog game in one test. Asserts Availability and the independence of `downloaded`/`installed`. Good shape. | +| `local_download_available_gates_on_catalog_operation_and_sentinel` | `local_games.rs:703` | Exercises every input dimension: catalog hit/miss, operation active/inactive, sentinel present/missing. Tight. | +| `aggregation_counts_only_ready_peers_as_download_sources` | `peer_db.rs:776` | Constructs a Ready peer and a LocalOnly peer, asserts `peer_count`, `eti_version`, `peers_with_game`, `get_latest_version_for_game`, and `peers_with_latest_version` all filter on Ready. Single test, four assertions, broad coverage. | +| `local_only_peer_does_not_make_game_downloadable` | `peer_db.rs:806` | The aggregate-of-one case: only a LocalOnly peer exists; nothing downloadable. | +| `version_ini_predicate_matches_only_game_root_sentinel` | `lanspread-db/src/db.rs:240` | Three cases for `is_version_ini`: root match, nested `local/version.ini` rejected, other game's path rejected. Maps directly to PLAN's `is_version_ini` tightening. | +| `commit_version_ini_writes_sentinel_last_and_sweeps_discarded` | `download.rs:1079` | Pre-seeds `.version.ini.discarded`, commits the buffer, asserts the sentinel content is correct, tmp is gone, discarded is gone. End-to-end on the download transaction commit. | +| `version_ini_buffer_accepts_out_of_order_chunks` | `download.rs:1057` | Writes to offset 4 first, then 0, asserts the buffer reconstructs `20250101`. Exact match to a PLAN test item. | +| `prepare_game_storage_skips_version_ini_sentinel` | `download.rs:1040` | Direct test that prep storage doesn't create the sentinel file. | +| `partition_requires_exactly_one_root_version_ini` | `download.rs:1114` | Tests three cases: single sentinel + nested decoy (passes), zero sentinels (errors), duplicate root sentinels (errors). | +| `local_relative_paths_are_never_transferable` | `services/stream.rs:394` | Pure unit test of the path predicate. Covers `game/local/...`, `local/...`, Windows backslashes, and the negative cases (`game/version.ini`, `game/archive.eti`). | +| `event_paths_map_to_top_level_game_id` | `services/local_monitor.rs:336` | Tests that watcher event paths map to game IDs correctly and that `.lanspread` and `local/` children are excluded. | +| `event_ignore_list_covers_reserved_names` | `services/local_monitor.rs:354` | Enumerates every reserved name and asserts each is ignored; asserts non-reserved names are not. | +| `operation_guard_clears_tracking_*` (3 tests) | `context.rs:240,254,269` | Tests the OperationGuard Drop impl across success, cancellation, and abort. The abort test (`*_when_task_is_dropped`) is particularly good — it verifies the runtime-handle fallback path inside Drop. | +| `required_service_failure_cancels_runtime_and_emits_event` | `startup.rs:360` | Real test of supervised-service semantics. | +| `restart_service_restarts_until_shutdown` | `startup.rs:395` | Verifies the restart policy actually re-invokes the factory. | + +### Marginal but reasonable + +| Test | Note | +|---|---| +| `build_peer_plans_handles_partial_final_chunk` (download.rs:954) | Older test, still valid. | +| `build_peer_plans_respects_file_peer_map` (download.rs:985) | Older test, still valid. | +| `runtime_handle_can_shutdown_and_await_stopped` (startup.rs:440) | Slightly tautological (builds the handle by hand, signals stopped manually). Asserts the watch-channel plumbing. Not deeply useful but cheap. | + +### What's missing relative to PLAN.md's test plan + +PLAN.md was explicit about test scenarios. Here's the delta. ✗ = no test; ◐ = partially tested via helper but not at the dispatch level; ✓ = covered. + +**Install transactions** (PLAN.md §"Unit — install transactions"): +- ✓ Install success +- ✗ Update success — there is no test for a *successful* update path. Only `update_failure_restores_previous_local`. +- ✓ Update rollback on extract failure +- ✗ Update rollback on commit-rename failure (e.g., destination on a read-only mount, or staging fails to rename). Not exercised. +- ✓ Uninstall success +- ✗ Uninstall delete-failure restore. The `restore_backup` rollback path in `uninstall` (`transaction.rs:107-114`) is not directly tested. + +**Download transaction** (PLAN.md §"Unit — download transaction"): +- ✓ Fresh download writes sentinel last via atomic rename (via `commit_version_ini_writes_sentinel_last_and_sweeps_discarded`). +- ◐ Re-download renames old version.ini to discarded and sweeps it — the commit/sweep is tested, but the *initial rename* in `begin_version_ini_transaction` (`download.rs:193-206`) has no unit test. +- ✗ Interrupted download leaves no version.ini — the `rollback_version_ini_transaction` function (`download.rs:208-221`) is reachable from many code paths in `download_game_files` and is never directly tested. +- ✓ `prepare_game_storage` skips sentinel. +- ✓ Out-of-order chunk arrivals (`version_ini_buffer_accepts_out_of_order_chunks`). +- ✗ Multi-`.eti` games. The `partition_requires_exactly_one_root_version_ini` test only covers the version.ini partition rules, not the multi-archive download path. + +**Recovery** (PLAN.md §"Unit — recovery"): +- ◐ Each row of the install matrix. The matrix has roughly 11 rows. Tests cover *one* (`Updating | no | yes | yes`) plus the None default and the download-transient sweep. The other 9 rows (Installing with three FS combinations; Updating with three more FS combinations; Uninstalling with three FS combinations) are not exercised. This is the largest coverage hole. +- ✓ Download recovery sweep. +- ◐ JSON missing / corrupted / wrong schema_version. Only `schema_mismatch_is_treated_as_missing` exists; no test for parse-error/corrupt-JSON or for a mismatched `id` field (handled in `intent.rs:69` but untested). +- ✓ Markerless `.local.*` dir ignored. + +**Atomic intent write** (PLAN.md §): +- ✓ Both cases listed. + +**Scanner gating** (PLAN.md §"Unit — scanner gating"): +- ✗ Events for an ID under operation lock are dropped. The `handle_watch_event` function (`local_monitor.rs:208-236`) is not tested — only the helper `game_id_from_event_path` and `should_ignore_game_child`. +- ✗ Rescan-pending flag collapses bursts to ≤2 scans per ID. `RescanGate` / `run_gated_rescan` (`local_monitor.rs:261-287`) has no test. +- ✗ Sideload picked up by fallback scan. +- ◐ Non-catalog game produces no GameSummary — only the negative assertion `!scan.summaries.contains_key("non-catalog")` exists; no test verifies that the *library index* also has no entry, and no test verifies that no `LibraryDelta` is emitted. + +**Serve gating** (PLAN.md §"Unit — serve gating"): +- ◐ `GetGame` refused mid-operation — `local_download_available_gates_on_catalog_operation_and_sentinel` tests the predicate, but `handle_get_game_command` / `handle_get_game` dispatch is untested. +- ◐ `GetGameFileData` / `GetGameFileChunk` refused mid-operation. Helpers untested; only `path_points_inside_local` is. +- ◐ All three handlers refuse non-catalog IDs. Same — predicate tested, dispatch untested. +- ◐ All three handlers refuse when sentinel is absent. Same. +- ✓ File-chunk handler rejects `local/`-relative paths (via the path-predicate unit test). + +**Aggregation** (PLAN.md §): +- ✓ All three rules covered in two tests. + +**Installed-only state** (PLAN.md §): +- ✓ Initial state covered in `scan_uses_version_ini_and_local_dir_as_independent_state`. +- ✗ Transition: user copies in `version.ini`, next rescan flips to Ready. Not tested. + +### Tests that could be improved + +#### `recovery_restores_backup_for_interrupted_update` (transaction.rs:591) + +Right shape, but covers only one row of the matrix. To make this more useful, parameterize: iterate over `(intent, fs_state)` tuples, run recovery, assert resulting `local/` content and intent. A table-driven approach would convert this single test into 11. + +#### `partition_requires_exactly_one_root_version_ini` (download.rs:1114) + +The "duplicate" case in the test actually has `game/version.ini` and `game/local/version.ini`. Under the *new* tight `is_version_ini` predicate, only the first matches, so this is *not* actually a duplicate-sentinel scenario — it tests "nested decoy is ignored." That's fine, but the test name promises something else. The "multiple" case (two `game/version.ini` entries) is a true duplicate and asserts error. Worth renaming the test or splitting into two clearer ones. + +#### The `TempDir` pattern is duplicated 5 times + +`install/intent.rs:128`, `install/transaction.rs:498`, `local_games.rs:615`, `download.rs:926`, plus subtle variants. Each is a tiny manual reimplementation of the same primitive. Consolidating into a single `test_support` module (or using `tempfile`, which is already a stable crate) would shrink each test file by ~25 lines. Not a correctness issue. + +#### No negative test for `is_version_ini` Windows-path case + +The predicate normalizes `\\` to `/` (`db.rs:182-183`). Worth a single Windows-style path test to lock the contract. + +### Tests that don't really test the runtime contract + +None — there are no obviously fake or tautological tests. Even `runtime_handle_can_shutdown_and_await_stopped` (which I noted as "slightly tautological" above) is genuinely testing the watch-channel signal path, which is harness-grade infrastructure worth pinning. + +## Test plumbing assessment + +- All install/transaction tests use a `FakeUnpacker` (`transaction.rs:467-496`) that writes a deterministic `payload.txt` on success. This is the right abstraction — it isolates the FS transaction logic from the actual unrar sidecar. +- The "failing unpacker" path uses a `fail: bool` field; clean and obvious. +- Temp directory names include `process::id()` and nanosecond timestamps, so parallel test runs won't collide. Good. + +## Summary + +The tests that exist are good tests — real, deterministic, decent assertions. **No fake/mocked-database tests; no tests that just exercise the test scaffolding.** The author clearly understood that filesystem state has to be set up explicitly to exercise the transactions. + +The gap is breadth. PLAN.md was specific about scenarios; roughly half of them are not yet covered. The largest single gap is the install-recovery matrix (11 rows, 1 row tested). Second largest is "scanner gating" — `RescanGate` and `handle_watch_event` have zero tests. Third is "serve gating" at the dispatch level — the predicate `local_download_available` is well-tested but the `handle_*` dispatch path isn't. + +Recommendation: a follow-up PR that parameterizes the recovery test over the full matrix, adds a few `handle_*`-level tests against a small in-memory `Ctx`, and adds the "successful update" and "uninstall delete-failure restore" cases would bring coverage in line with what PLAN.md asked for. None of this is blocking — what's there is solid. diff --git a/REVIEW_STEP_4.md b/REVIEW_STEP_4.md new file mode 100644 index 0000000..cf1b5fa --- /dev/null +++ b/REVIEW_STEP_4.md @@ -0,0 +1,146 @@ +# Review Step 4 — Bigger Picture + +The user asked to evaluate the larger picture, not what's missing or next. So this is about what *is* there: how the design holds up, what risks live in it, and what habits to either trust or watch. + +--- + +## Architectural integrity + +### The two-axis state model holds up under pressure + +The core insight in PLAN.md — that `downloaded` and `installed` are independent, with `version.ini` and `local/` as their respective ground-truth signals — is the right shape. It collapses four messy ad-hoc predicates the old code carried (`eti exists`, `version.ini exists`, `local exists`, `___TO_BE_DELETE___ exists`) into two boolean signals, each with a single observation point. The downstream cleanup falls out almost automatically: `availability`, scanner gating, manifest serving, watcher decisions, intent recovery — they all consult the same two predicates. + +This is the kind of design where the test for "did we get the abstraction right" is whether new edge cases require new predicates or just slot into the existing matrix. Looking at the install-recovery matrix in `transaction.rs:148-322`, every case decomposes to `(intent_state, local_present, installing_present, backup_present)`. That's 4 × 2 × 2 × 2 = 32 combinations, of which the plan named ~11 and the rest fall to the "trust FS, don't mutate" default. No new predicate needed for any of them. The model is doing real work. + +### "FS as truth, intent log as ownership proof" is the load-bearing idea + +The intent log doesn't *replace* the filesystem state — it gives recovery permission to mutate. The plan called this out explicitly ("intent says Installing → corresponding `.local.installing/` is unconditionally Lanspread-owned regardless of marker"). The implementation honors it: `sweep_owned_orphan` (`transaction.rs:382-395`) refuses to touch markerless dirs in the `None` case; the per-intent recovery handlers freely remove them when intent proves ownership. + +This separation matters most for the "user puts a `.local.installing/` directory in their game folder for some reason" scenario. Plenty of designs would just delete it. This one logs a warning and walks away. That's the right call for software that operates on user-managed game folders. + +### Single operation table as the universal gate is elegant + +Every gating decision in the runtime (manifest serve, file stream serve, list-games filter, library announcement, watcher event drop, scan summary preservation, Tauri command rejection) consults the same `active_operations: HashMap`. This collapses what could have been six separate concurrency primitives into one map and a `Drop` guard. + +The `OperationGuard` (`context.rs:122-191`) is the right pattern — clears tracking on any exit path, including `task::abort`. The fact that it handles three different drop contexts (sync drop, async drop with current runtime handle, drop with no runtime) shows the author thought through what happens at shutdown. The test `operation_guard_clears_tracking_when_task_is_dropped` (`context.rs:269`) covers the worst of these. + +### The peer/Tauri boundary is much cleaner now + +Pre-PLAN, the Tauri shell owned: +- The unpacker invocation +- The whole-folder backup scheme (`___TO_BE_DELETE___{id}`) +- The "downloaded means installed" coupling +- The "spawn unpack after download finishes" decision + +Post-PLAN, it owns: +- The unrar sidecar plumbing (injected via `Unpacker` trait) +- The catalog source (loaded from bundled `game.db`) +- The UI event fan-out + +That's a meaningful shift. The peer crate is now actually headless. The Tauri crate has no business-logic decisions about install state — it routes by local FS state in three lines (`src-tauri/src/lib.rs:108-116`) and otherwise just relays events. + +This also means the peer crate is more testable on its own, which is reflected in the test count (24 new tests in `lanspread-peer/src/install/` and `download.rs` alone). + +--- + +## Risk surface + +### Two sources of truth for "what's in progress" + +The peer keeps `active_operations: HashMap`. The Tauri shell keeps `active_operations: HashMap`. These are kept in sync by handling `PeerEvent` lifecycle messages. + +If a `PeerEvent::InstallGameFinished` is ever dropped (channel full, app foregrounded mid-event, panic in the handler), the UI side stays stuck thinking an install is in progress. There is no reconciliation path — the user has to restart the app. + +This is a pre-existing pattern in the codebase but the new install/uninstall flows expand it. Worth keeping an eye on. + +**Mitigation idea (not for this PR):** the peer could include a "current operations" snapshot in `PeerEvent::LocalGamesUpdated` so the UI can recompute its in-progress set rather than maintaining it from event history. + +### Recovery re-runs are O(games_dir) on every state-changing command + +`load_local_library` → `install::recover_on_startup` walks every top-level dir under `game_dir`, reads `.lanspread.json` from each, and does FS stats. This is called on: +- Initial startup (correct per plan) +- `SetGameDir` (probably correct — different dir, different state) +- After every install/uninstall completes (questionable — we just finished a transaction; nothing else could have changed) + +For 100 games this is ~100 fs reads per operation. For 1000 games, noticeable latency on every install completion. The fix is to either restrict post-operation recovery to the affected game ID, or skip it entirely (the transaction already cleared its own state). + +This is not yet a problem at current scale (handful of games) but is the kind of thing that becomes one once someone with a real library tries it. + +### `recover_on_startup` ignores the operation table + +If `load_local_library` runs while a download is in-flight (say, the user clicks `SetGameDir` mid-download), `recover_download_transients` will delete `.version.ini.tmp` and `.version.ini.discarded` *while the download is still using them*. The download then tries to rename a tmp that has been swept. + +This is a narrow race, but it's possible. The fix is for `recover_on_startup` to skip game roots whose ID is in `active_operations`. Worth a thought. + +### Watcher reconciliation is event-driven, not file-creation-driven + +`reconcile_watch_state` is called from inside the watcher select-loop after either a tick or an event (`local_monitor.rs:51-67`). If a new game root is created and no event ever fires for the `game_dir` root, the new game root won't get its own watch until the next 300s tick. In practice the parent-dir watch *will* see a CREATE event for the new root, which triggers reconcile, so this is usually fine. But it relies on notify's behavior on the specific platform — and `notify` doesn't promise this for all backends. + +The fallback scan catches it within 300s, so worst case is a slow first scan. Not a correctness issue. + +### `Availability::Downloading` is defined and never emitted + +`build_game_summary` only produces `Ready` or `LocalOnly`. The `Downloading` variant exists in the proto enum and is never written to a `GameSummary`. Two consequences: +- If a future change tries to emit it, downstream code (`game_is_ready` in `peer_db.rs:726`) will quietly classify it as not-Ready and exclude it from peer counts. That might actually be correct, but the implication is undocumented. +- The wire protocol carries a value that's structurally unreachable. If a peer running an older binary sends `Downloading`, the new code handles it correctly (not-Ready). Good. + +This is intentional simplification (the operation table already gates serving during a download, so the summary doesn't need to advertise it). But it's worth a comment in proto code so the next person doesn't add a third gate. + +### `InstallIntent::manifest_hash` is dead + +The plan defined `manifest_hash: Option` in the intent JSON. The implementation has the field but never populates it. Future-proof, or accidental? If accidental, it should be removed; if intentional, a one-line comment in `intent.rs` saying "reserved for future TBD use" would help. + +--- + +## Code organization + +### `download.rs` is at the limit of what one file should hold + +1162 lines including tests, mixing chunk planning, retry orchestration, version-sentinel transaction primitives, in-memory buffers, and the main download loop. A future split into `download/transaction.rs`, `download/chunks.rs`, and `download/orchestrator.rs` would be a kindness. Not blocking. + +### `install/` module is well-sized + +Three files, each focused: `intent.rs` (atomic write), `transaction.rs` (state machine), `unpack.rs` (trait def). Public surface in `mod.rs`. Matches the plan exactly and reads cleanly. + +### `handlers.rs` has awkward `RequestedInstallOperation` semantics + +`Auto`, `Install`, and `Update` are three variants but `Auto` and `Install` collapse to the same behavior (`handlers.rs:355-363`). Effectively two distinct flows: "user said install/auto → infer from FS" vs "user said update → always treat as Updating". The third variant is rhetorical — `Install` and `Auto` could merge into a single `Inferred` variant or just a boolean. + +Worth simplifying later. + +--- + +## Test culture + +The author wrote tests that exercise real filesystem semantics: actual temp directories, real `fs::rename`, real `tokio::fs` calls. No mocked filesystem layers. This is the right choice for code whose entire value proposition is correct atomic-rename behavior — but it does mean the tests are sensitive to platform behavior. They all pass on Linux in this run. + +The `FakeUnpacker` is the only meaningful test double. It models the unpacker as "write a `payload.txt` on success, error on fail." That's enough to drive the transaction logic without depending on the unrar sidecar. Right level of abstraction. + +The `TempDir` reimplementation in five files is the only consistent test-hygiene smell. Easy follow-up. + +--- + +## What I would not change + +- The "intent log proves ownership, marker is belt-and-suspenders" decomposition. It's the clearest way to reason about recovery and it generalizes. +- The single operation table as the universal gate. +- The injected `Unpacker` boxed-future trait. Avoids `async_trait`, stays object-safe, doesn't leak into the peer crate. Clean. +- The in-memory `VersionIniBuffer` for the sentinel. The sentinel is small; this avoids any chance of partial-write contamination. +- Keeping `game-unpack-finished` as the event name *for this PR* (decision recorded). Worth renaming later but not worth shipping ahead of correctness. + +--- + +## What I would watch + +1. The Tauri-side `active_operations` map. If event-loss becomes a real issue in testing, push toward a reconciliation-from-snapshot pattern. +2. `recover_on_startup` cost on real libraries. If users with hundreds of games see startup latency, restrict recovery scope. +3. The recovery race against in-flight downloads. Even though the gate prevents new operations on a game with an active operation, the recovery sweep runs without consulting the operation table. +4. Whether `Availability::Downloading` ever gets emitted. If it does, peer-side filters need a recheck. + +--- + +## Overall + +This is a substantial refactor that took a stack of brittle, coupled concerns in Tauri-land — backup-the-whole-game-folder, downloaded-implies-installed, ad-hoc unpack-after-download, time-debounced scanning — and produced a coherent state machine grounded in two predicates. The choices visible in the diff are mostly the same choices I'd have made. The deviations from PLAN.md are minor and recorded (mostly) in IMPL_DECISIONS.md. The test breadth is the weakest dimension but the test depth is good — the tests that exist are not theater. + +The design will scale to more operations (e.g., a future "verify" or "repair" command) and to more game-state predicates (e.g., a future "quarantined" state for failed-verification games) without needing to revisit the core architecture. That's the test of whether the abstraction was right. diff --git a/crates/lanspread-compat/src/eti.rs b/crates/lanspread-compat/src/eti.rs index c0de75a..9fbd191 100644 --- a/crates/lanspread-compat/src/eti.rs +++ b/crates/lanspread-compat/src/eti.rs @@ -1,6 +1,6 @@ use std::path::Path; -use lanspread_db::db::Game; +use lanspread_db::db::{AVAILABILITY_LOCAL_ONLY, Game}; use serde::{Deserialize, Serialize}; use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; @@ -63,6 +63,7 @@ impl From for Game { size: (eti_game.game_size * 1024.0 * 1024.0 * 1024.0) as u64, downloaded: false, installed: false, + availability: AVAILABILITY_LOCAL_ONLY.to_string(), eti_game_version: None, local_version: None, peer_count: 0, // ETI games start with 0 peers until peer system discovers them diff --git a/crates/lanspread-db/src/db.rs b/crates/lanspread-db/src/db.rs index b3eb663..776399a 100644 --- a/crates/lanspread-db/src/db.rs +++ b/crates/lanspread-db/src/db.rs @@ -5,6 +5,10 @@ use std::{collections::HashMap, fmt, path::Path}; use serde::{Deserialize, Serialize}; +pub const AVAILABILITY_READY: &str = "Ready"; +pub const AVAILABILITY_DOWNLOADING: &str = "Downloading"; +pub const AVAILABILITY_LOCAL_ONLY: &str = "LocalOnly"; + /// Read version from version.ini file /// # Errors /// Returns error if file cannot be read or parsed @@ -30,6 +34,10 @@ pub fn read_version_from_ini(game_dir: &Path) -> eyre::Result> { } } +fn default_availability() -> String { + AVAILABILITY_LOCAL_ONLY.to_string() +} + /// A game #[derive(Clone, Serialize, Deserialize)] pub struct Game { @@ -57,6 +65,9 @@ pub struct Game { /// only relevant for client (yeah... I know) #[serde(default)] pub installed: bool, + /// Backend-reported availability state for this game's local or peer summary. + #[serde(default = "default_availability")] + pub availability: String, /// ETI game version from version.ini (YYYYMMDD format) (server) pub eti_game_version: Option, /// Local game version from version.ini (YYYYMMDD format) @@ -157,6 +168,7 @@ impl GameDB { for game in self.games.values_mut() { game.downloaded = false; game.installed = false; + game.availability = AVAILABILITY_LOCAL_ONLY.to_string(); game.local_version = None; } } @@ -207,7 +219,7 @@ impl fmt::Debug for GameFileDescription { mod tests { use serde_json::json; - use super::{Game, GameFileDescription}; + use super::{AVAILABILITY_LOCAL_ONLY, Game, GameFileDescription}; #[test] fn installed_defaults_to_false_when_missing() { @@ -234,6 +246,7 @@ mod tests { !game.installed, "missing installed flag should default to false" ); + assert_eq!(game.availability, AVAILABILITY_LOCAL_ONLY); } #[test] @@ -262,4 +275,15 @@ mod tests { }; assert!(!other_game.is_version_ini()); } + + #[test] + fn version_ini_predicate_accepts_windows_separators() { + let root = GameFileDescription { + game_id: "aoe2".to_string(), + relative_path: r"aoe2\version.ini".to_string(), + is_dir: false, + size: 8, + }; + assert!(root.is_version_ini()); + } } diff --git a/crates/lanspread-peer/README.md b/crates/lanspread-peer/README.md index 756ad0d..658469a 100644 --- a/crates/lanspread-peer/README.md +++ b/crates/lanspread-peer/README.md @@ -13,7 +13,7 @@ It is designed to run headless – other crates (most notably of the peer crate's platform layer, and the catalog set gates which local game roots are announced or served. - `PeerCommand` represents the small control surface exposed to the UI layer: - `ListGames`, `GetGame`, `DownloadGameFiles`, `InstallGame`, `UpdateGame`, + `ListGames`, `GetGame`, `DownloadGameFiles`, `InstallGame`, `UninstallGame`, and `SetGameDir`. - `PeerEvent` enumerates everything the peer runtime reports back to the UI: library snapshots, download/install/uninstall lifecycle updates, runtime diff --git a/crates/lanspread-peer/src/download.rs b/crates/lanspread-peer/src/download.rs index 370749f..8c20a1c 100644 --- a/crates/lanspread-peer/src/download.rs +++ b/crates/lanspread-peer/src/download.rs @@ -107,7 +107,9 @@ fn ensure_download_not_cancelled( Ok(()) } -fn partition_download_descriptions( +/// Extracts the root `version.ini` descriptor while keeping every descriptor in +/// the transfer list. The chunk writer diverts the sentinel bytes into memory. +fn extract_version_descriptor( game_id: &str, game_file_descs: Vec, tx_notify_ui: &UnboundedSender, @@ -740,7 +742,7 @@ pub async fn download_game_files( } let (version_desc, transfer_descs) = - partition_download_descriptions(game_id, game_file_descs, &tx_notify_ui)?; + extract_version_descriptor(game_id, game_file_descs, &tx_notify_ui)?; let version_buffer = match VersionIniBuffer::new(&version_desc) { Ok(buffer) => Arc::new(buffer), Err(err) => { @@ -1111,10 +1113,57 @@ mod tests { assert!(!game_root.join(".version.ini.discarded").exists()); } + #[tokio::test] + async fn begin_version_ini_transaction_parks_existing_sentinel() { + let temp = TempDir::new(); + let game_root = temp.path().join("game"); + tokio::fs::create_dir_all(&game_root) + .await + .expect("game root should be created"); + tokio::fs::write(game_root.join("version.ini"), b"20240101") + .await + .expect("version sentinel should be written"); + tokio::fs::write(game_root.join(".version.ini.tmp"), b"partial") + .await + .expect("tmp sentinel should be written"); + + begin_version_ini_transaction(&game_root) + .await + .expect("transaction should begin"); + + assert!(!game_root.join("version.ini").exists()); + assert!(!game_root.join(".version.ini.tmp").exists()); + assert_eq!( + std::fs::read(game_root.join(".version.ini.discarded")) + .expect("discarded sentinel should exist"), + b"20240101" + ); + } + + #[tokio::test] + async fn rollback_version_ini_transaction_sweeps_transients() { + let temp = TempDir::new(); + let game_root = temp.path().join("game"); + tokio::fs::create_dir_all(&game_root) + .await + .expect("game root should be created"); + tokio::fs::write(game_root.join(".version.ini.tmp"), b"partial") + .await + .expect("tmp sentinel should be written"); + tokio::fs::write(game_root.join(".version.ini.discarded"), b"old") + .await + .expect("discarded sentinel should be written"); + + rollback_version_ini_transaction(&game_root).await; + + assert!(!game_root.join(".version.ini.tmp").exists()); + assert!(!game_root.join(".version.ini.discarded").exists()); + } + #[test] - fn partition_requires_exactly_one_root_version_ini() { + fn version_descriptor_extraction_keeps_nested_decoy_in_transfer_list() { let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); - let duplicate = vec![ + let nested_decoy = vec![ GameFileDescription { game_id: "game".to_string(), relative_path: "game/version.ini".to_string(), @@ -1129,19 +1178,27 @@ mod tests { }, ]; - let (version, transfer) = partition_download_descriptions("game", duplicate, &tx) - .expect("only one root sentinel"); + let (version, transfer) = + extract_version_descriptor("game", nested_decoy, &tx).expect("only one root sentinel"); assert_eq!(version.relative_path, "game/version.ini"); assert_eq!(transfer.len(), 2); + } + #[test] + fn version_descriptor_extraction_requires_a_root_version_ini() { + let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); let missing = vec![GameFileDescription { game_id: "game".to_string(), relative_path: "game/archive.eti".to_string(), is_dir: false, size: 1, }]; - assert!(partition_download_descriptions("game", missing, &tx).is_err()); + assert!(extract_version_descriptor("game", missing, &tx).is_err()); + } + #[test] + fn version_descriptor_extraction_rejects_duplicate_root_version_ini() { + let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); let multiple = vec![ GameFileDescription { game_id: "game".to_string(), @@ -1156,6 +1213,6 @@ mod tests { size: 8, }, ]; - assert!(partition_download_descriptions("game", multiple, &tx).is_err()); + assert!(extract_version_descriptor("game", multiple, &tx).is_err()); } } diff --git a/crates/lanspread-peer/src/handlers.rs b/crates/lanspread-peer/src/handlers.rs index c5ecce3..174a0a4 100644 --- a/crates/lanspread-peer/src/handlers.rs +++ b/crates/lanspread-peer/src/handlers.rs @@ -1,6 +1,11 @@ //! Command handlers for peer commands. -use std::{collections::hash_map::Entry, net::SocketAddr, path::PathBuf, sync::Arc}; +use std::{ + collections::{HashSet, hash_map::Entry}, + net::SocketAddr, + path::{Path, PathBuf}, + sync::Arc, +}; use lanspread_db::db::{GameDB, GameFileDescription}; use tokio::sync::{RwLock, mpsc::UnboundedSender}; @@ -19,6 +24,7 @@ use crate::{ get_game_file_descriptions, local_dir_is_directory, local_download_available, + rescan_local_game, scan_local_library, version_ini_is_regular_file, }, @@ -211,12 +217,7 @@ pub async fn handle_download_game_files_command( { log::error!("Failed to send DownloadGameFilesFinished event: {e}"); } - spawn_install_operation( - ctx, - tx_notify_ui, - id.clone(), - RequestedInstallOperation::Auto, - ); + spawn_install_operation(ctx, tx_notify_ui, id.clone()); } else { log::error!("No trusted peers available after majority validation for game {id}"); } @@ -267,13 +268,7 @@ pub async fn handle_download_game_files_command( match result { Ok(()) => { - run_install_operation( - &ctx_clone, - &tx_notify_ui_clone, - download_id, - RequestedInstallOperation::Auto, - ) - .await; + run_install_operation(&ctx_clone, &tx_notify_ui_clone, download_id).await; } Err(e) => { log::error!("Download failed for {download_id}: {e}"); @@ -288,16 +283,7 @@ pub async fn handle_install_game_command( tx_notify_ui: &UnboundedSender, id: String, ) { - spawn_install_operation(ctx, tx_notify_ui, id, RequestedInstallOperation::Install); -} - -/// Handles the `UpdateGame` command. -pub async fn handle_update_game_command( - ctx: &Ctx, - tx_notify_ui: &UnboundedSender, - id: String, -) { - spawn_install_operation(ctx, tx_notify_ui, id, RequestedInstallOperation::Update); + spawn_install_operation(ctx, tx_notify_ui, id); } /// Handles the `UninstallGame` command. @@ -313,32 +299,15 @@ pub async fn handle_uninstall_game_command( }); } -#[derive(Clone, Copy)] -enum RequestedInstallOperation { - Auto, - Install, - Update, -} - -fn spawn_install_operation( - ctx: &Ctx, - tx_notify_ui: &UnboundedSender, - id: String, - requested: RequestedInstallOperation, -) { +fn spawn_install_operation(ctx: &Ctx, tx_notify_ui: &UnboundedSender, id: String) { let ctx = ctx.clone(); let tx_notify_ui = tx_notify_ui.clone(); ctx.task_tracker.clone().spawn(async move { - run_install_operation(&ctx, &tx_notify_ui, id, requested).await; + run_install_operation(&ctx, &tx_notify_ui, id).await; }); } -async fn run_install_operation( - ctx: &Ctx, - tx_notify_ui: &UnboundedSender, - id: String, - requested: RequestedInstallOperation, -) { +async fn run_install_operation(ctx: &Ctx, tx_notify_ui: &UnboundedSender, id: String) { if !catalog_contains(ctx, &id).await { log::warn!("Ignoring install command for non-catalog game {id}"); return; @@ -352,14 +321,10 @@ async fn run_install_operation( } let local_present = local_dir_is_directory(&game_root).await; - let operation = match requested { - RequestedInstallOperation::Auto | RequestedInstallOperation::Install if local_present => { - InstallOperation::Updating - } - RequestedInstallOperation::Auto | RequestedInstallOperation::Install => { - InstallOperation::Installing - } - RequestedInstallOperation::Update => InstallOperation::Updating, + let operation = if local_present { + InstallOperation::Updating + } else { + InstallOperation::Installing }; let operation_kind = match operation { InstallOperation::Installing => OperationKind::Installing, @@ -371,20 +336,24 @@ async fn run_install_operation( return; } - let _operation_guard = OperationGuard::new(id.clone(), ctx.active_operations.clone()); - events::send( - tx_notify_ui, - PeerEvent::InstallGameBegin { - id: id.clone(), - operation, - }, - ); + let result = { + let _operation_guard = OperationGuard::new(id.clone(), ctx.active_operations.clone()); + events::send( + tx_notify_ui, + PeerEvent::InstallGameBegin { + id: id.clone(), + operation, + }, + ); - let result = match operation { - InstallOperation::Installing => { - install::install(&game_root, &id, ctx.unpacker.clone()).await + match operation { + InstallOperation::Installing => { + install::install(&game_root, &id, ctx.unpacker.clone()).await + } + InstallOperation::Updating => { + install::update(&game_root, &id, ctx.unpacker.clone()).await + } } - InstallOperation::Updating => install::update(&game_root, &id, ctx.unpacker.clone()).await, }; match result { @@ -393,14 +362,17 @@ async fn run_install_operation( tx_notify_ui, PeerEvent::InstallGameFinished { id: id.clone() }, ); - if let Err(err) = load_local_library(ctx, tx_notify_ui).await { + if let Err(err) = refresh_local_game(ctx, tx_notify_ui, &id).await { log::error!("Failed to refresh local library after install: {err}"); } } Err(err) => { log::error!("Install operation failed for {id}: {err}"); - events::send(tx_notify_ui, PeerEvent::InstallGameFailed { id }); - if let Err(refresh_err) = load_local_library(ctx, tx_notify_ui).await { + events::send( + tx_notify_ui, + PeerEvent::InstallGameFailed { id: id.clone() }, + ); + if let Err(refresh_err) = refresh_local_game(ctx, tx_notify_ui, &id).await { log::error!("Failed to refresh local library after install failure: {refresh_err}"); } } @@ -418,14 +390,18 @@ async fn run_uninstall_operation(ctx: &Ctx, tx_notify_ui: &UnboundedSender { events::send( tx_notify_ui, @@ -441,7 +417,7 @@ async fn run_uninstall_operation(ctx: &Ctx, tx_notify_ui: &UnboundedSender, game_dir: PathBuf, ) { + let current_game_dir = ctx.game_dir.read().await.clone(); + if current_game_dir == game_dir { + log::info!( + "Game directory {} unchanged; refreshing without recovery", + game_dir.display() + ); + let tx_notify_ui = tx_notify_ui.clone(); + let ctx_clone = ctx.clone(); + ctx.task_tracker.spawn(async move { + if let Err(err) = refresh_local_library(&ctx_clone, &tx_notify_ui).await { + log::error!("Failed to refresh local game database: {err}"); + } + }); + return; + } + + let active_ids = active_operation_ids(ctx).await; + if !active_ids.is_empty() { + log::warn!( + "Rejecting game directory change to {} while operations are active for: {}", + game_dir.display(), + active_ids.into_iter().collect::>().join(", ") + ); + return; + } + *ctx.game_dir.write().await = game_dir.clone(); log::info!("Game directory set to: {}", game_dir.display()); @@ -489,13 +491,46 @@ pub async fn load_local_library( tx_notify_ui: &UnboundedSender, ) -> eyre::Result<()> { let game_dir = { ctx.game_dir.read().await.clone() }; - install::recover_on_startup(&game_dir).await?; + let active_ids = active_operation_ids(ctx).await; + install::recover_on_startup(&game_dir, &active_ids).await?; + scan_and_announce_local_library(ctx, tx_notify_ui, &game_dir).await +} + +async fn refresh_local_library( + ctx: &Ctx, + tx_notify_ui: &UnboundedSender, +) -> eyre::Result<()> { + let game_dir = { ctx.game_dir.read().await.clone() }; + scan_and_announce_local_library(ctx, tx_notify_ui, &game_dir).await +} + +async fn scan_and_announce_local_library( + ctx: &Ctx, + tx_notify_ui: &UnboundedSender, + game_dir: &Path, +) -> eyre::Result<()> { let catalog = ctx.catalog.read().await.clone(); - let scan = scan_local_library(&game_dir, &catalog).await?; + let scan = scan_local_library(game_dir, &catalog).await?; update_and_announce_games(ctx, tx_notify_ui, scan).await; Ok(()) } +async fn refresh_local_game( + ctx: &Ctx, + tx_notify_ui: &UnboundedSender, + id: &str, +) -> eyre::Result<()> { + let game_dir = { ctx.game_dir.read().await.clone() }; + let catalog = ctx.catalog.read().await.clone(); + let scan = rescan_local_game(&game_dir, &catalog, id).await?; + update_and_announce_games(ctx, tx_notify_ui, scan).await; + Ok(()) +} + +async fn active_operation_ids(ctx: &Ctx) -> HashSet { + ctx.active_operations.read().await.keys().cloned().collect() +} + /// Handles the `GetPeerCount` command. pub async fn handle_get_peer_count_command(ctx: &Ctx, tx_notify_ui: &UnboundedSender) { log::info!("GetPeerCount command received"); @@ -589,3 +624,225 @@ pub async fn update_and_announce_games( } } } + +#[cfg(test)] +mod tests { + use std::{ + collections::HashSet, + path::{Path, PathBuf}, + sync::Arc, + time::{Duration, SystemTime, UNIX_EPOCH}, + }; + + use tokio::sync::mpsc; + use tokio_util::{sync::CancellationToken, task::TaskTracker}; + + use super::*; + use crate::{UnpackFuture, Unpacker}; + + struct TempDir(PathBuf); + + impl TempDir { + fn new(prefix: &str) -> Self { + let mut path = std::env::temp_dir(); + path.push(format!( + "{prefix}-{}-{}", + std::process::id(), + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_nanos() + )); + std::fs::create_dir_all(&path).expect("temp dir should be created"); + Self(path) + } + + fn path(&self) -> &Path { + &self.0 + } + + fn game_root(&self) -> PathBuf { + self.0.join("game") + } + } + + impl Drop for TempDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + + struct FakeUnpacker; + + impl Unpacker for FakeUnpacker { + fn unpack<'a>(&'a self, _archive: &'a Path, dest: &'a Path) -> UnpackFuture<'a> { + Box::pin(async move { + tokio::fs::write(dest.join("payload.txt"), b"installed").await?; + Ok(()) + }) + } + } + + fn write_file(path: &Path, bytes: &[u8]) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("parent dir should be created"); + } + std::fs::write(path, bytes).expect("file should be written"); + } + + fn test_ctx(game_dir: PathBuf) -> Ctx { + Ctx::new( + Arc::new(RwLock::new(PeerGameDB::new())), + "peer".to_string(), + game_dir, + Arc::new(FakeUnpacker), + CancellationToken::new(), + TaskTracker::new(), + Arc::new(RwLock::new(HashSet::from(["game".to_string()]))), + ) + } + + async fn recv_event(rx: &mut mpsc::UnboundedReceiver) -> PeerEvent { + tokio::time::timeout(Duration::from_secs(1), rx.recv()) + .await + .expect("event should arrive") + .expect("event channel should remain open") + } + + fn assert_local_update(event: PeerEvent, installed: bool, downloaded: bool) { + let PeerEvent::LocalGamesUpdated(games) = event else { + panic!("expected LocalGamesUpdated"); + }; + let game = games + .iter() + .find(|game| game.id == "game") + .expect("game should be announced"); + assert_eq!(game.installed, installed); + assert_eq!(game.downloaded, downloaded); + } + + #[tokio::test] + async fn install_refreshes_settled_state_after_guard_release() { + let temp = TempDir::new("lanspread-handler-install"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("game.eti"), b"archive"); + + let ctx = test_ctx(temp.path().to_path_buf()); + let (tx, mut rx) = mpsc::unbounded_channel(); + + run_install_operation(&ctx, &tx, "game".to_string()).await; + + match recv_event(&mut rx).await { + PeerEvent::InstallGameBegin { id, operation } => { + assert_eq!(id, "game"); + assert_eq!(operation, InstallOperation::Installing); + } + _ => panic!("expected InstallGameBegin"), + } + assert!(matches!( + recv_event(&mut rx).await, + PeerEvent::InstallGameFinished { id } if id == "game" + )); + assert!(ctx.active_operations.read().await.is_empty()); + assert_local_update(recv_event(&mut rx).await, true, true); + } + + #[tokio::test] + async fn update_refreshes_settled_state_after_guard_release() { + let temp = TempDir::new("lanspread-handler-update"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("game.eti"), b"archive"); + write_file(&root.join("local").join("old.txt"), b"old"); + + let ctx = test_ctx(temp.path().to_path_buf()); + let (tx, mut rx) = mpsc::unbounded_channel(); + + run_install_operation(&ctx, &tx, "game".to_string()).await; + + match recv_event(&mut rx).await { + PeerEvent::InstallGameBegin { id, operation } => { + assert_eq!(id, "game"); + assert_eq!(operation, InstallOperation::Updating); + } + _ => panic!("expected InstallGameBegin"), + } + assert!(matches!( + recv_event(&mut rx).await, + PeerEvent::InstallGameFinished { id } if id == "game" + )); + assert!(ctx.active_operations.read().await.is_empty()); + assert_local_update(recv_event(&mut rx).await, true, true); + } + + #[tokio::test] + async fn uninstall_refreshes_settled_state_after_guard_release() { + let temp = TempDir::new("lanspread-handler-uninstall"); + let root = temp.game_root(); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("game.eti"), b"archive"); + write_file(&root.join("local").join("old.txt"), b"old"); + + let ctx = test_ctx(temp.path().to_path_buf()); + let (tx, mut rx) = mpsc::unbounded_channel(); + + run_uninstall_operation(&ctx, &tx, "game".to_string()).await; + + assert!(matches!( + recv_event(&mut rx).await, + PeerEvent::UninstallGameBegin { id } if id == "game" + )); + assert!(matches!( + recv_event(&mut rx).await, + PeerEvent::UninstallGameFinished { id } if id == "game" + )); + assert!(ctx.active_operations.read().await.is_empty()); + assert_local_update(recv_event(&mut rx).await, false, true); + } + + #[tokio::test] + async fn path_changing_set_game_dir_is_rejected_while_operations_are_active() { + let current = TempDir::new("lanspread-handler-current-dir"); + let next = TempDir::new("lanspread-handler-next-dir"); + let ctx = test_ctx(current.path().to_path_buf()); + ctx.active_operations + .write() + .await + .insert("game".to_string(), OperationKind::Downloading); + let (tx, _rx) = mpsc::unbounded_channel(); + + handle_set_game_dir_command(&ctx, &tx, next.path().to_path_buf()).await; + + assert_eq!(*ctx.game_dir.read().await, current.path()); + } + + #[tokio::test] + async fn same_path_set_game_dir_refreshes_without_recovery() { + let temp = TempDir::new("lanspread-handler-same-dir"); + write_file(&temp.game_root().join(".version.ini.tmp"), b"tmp"); + let ctx = test_ctx(temp.path().to_path_buf()); + let (tx, _rx) = mpsc::unbounded_channel(); + + handle_set_game_dir_command(&ctx, &tx, temp.path().to_path_buf()).await; + ctx.task_tracker.close(); + ctx.task_tracker.wait().await; + + assert!(temp.game_root().join(".version.ini.tmp").is_file()); + } + + #[tokio::test] + async fn path_changing_set_game_dir_runs_recovery() { + let current = TempDir::new("lanspread-handler-old-dir"); + let next = TempDir::new("lanspread-handler-new-dir"); + write_file(&next.game_root().join(".version.ini.tmp"), b"tmp"); + let ctx = test_ctx(current.path().to_path_buf()); + let (tx, _rx) = mpsc::unbounded_channel(); + + handle_set_game_dir_command(&ctx, &tx, next.path().to_path_buf()).await; + ctx.task_tracker.close(); + ctx.task_tracker.wait().await; + + assert!(!next.game_root().join(".version.ini.tmp").exists()); + } +} diff --git a/crates/lanspread-peer/src/install/intent.rs b/crates/lanspread-peer/src/install/intent.rs index 67fd1bd..43f676a 100644 --- a/crates/lanspread-peer/src/install/intent.rs +++ b/crates/lanspread-peer/src/install/intent.rs @@ -25,7 +25,6 @@ pub struct InstallIntent { pub recorded_at: u64, pub state: InstallIntentState, pub eti_version: Option, - pub manifest_hash: Option, } impl InstallIntent { @@ -36,7 +35,6 @@ impl InstallIntent { recorded_at: now_unix_secs(), state, eti_version, - manifest_hash: None, } } @@ -193,4 +191,57 @@ mod tests { let recovered = read_intent(temp.path(), "game").await; assert_eq!(recovered.state, InstallIntentState::None); } + + #[tokio::test] + async fn mismatched_id_is_treated_as_missing() { + let temp = TempDir::new(); + tokio::fs::write( + intent_path(temp.path()), + r#"{"schema_version":1,"id":"other","recorded_at":0,"state":"Updating"}"#, + ) + .await + .expect("intent should be written"); + + let recovered = read_intent(temp.path(), "game").await; + assert_eq!(recovered.state, InstallIntentState::None); + } + + #[tokio::test] + async fn corrupt_intent_is_treated_as_missing() { + let temp = TempDir::new(); + tokio::fs::write(intent_path(temp.path()), b"not json") + .await + .expect("intent should be written"); + + let recovered = read_intent(temp.path(), "game").await; + assert_eq!(recovered.state, InstallIntentState::None); + } + + #[tokio::test] + async fn old_manifest_hash_field_is_ignored_and_new_writes_omit_it() { + let temp = TempDir::new(); + tokio::fs::write( + intent_path(temp.path()), + r#"{"schema_version":1,"id":"game","recorded_at":0,"state":"Updating","eti_version":"20240101","manifest_hash":42}"#, + ) + .await + .expect("intent should be written"); + + let recovered = read_intent(temp.path(), "game").await; + assert_eq!(recovered.state, InstallIntentState::Updating); + assert_eq!(recovered.eti_version.as_deref(), Some("20240101")); + + write_intent(temp.path(), &InstallIntent::none("game", None)) + .await + .expect("intent should be written"); + let written = tokio::fs::read_to_string(intent_path(temp.path())) + .await + .expect("intent should be readable"); + assert!( + serde_json::from_str::(&written) + .expect("intent should parse") + .get("manifest_hash") + .is_none() + ); + } } diff --git a/crates/lanspread-peer/src/install/transaction.rs b/crates/lanspread-peer/src/install/transaction.rs index 3cf2eea..c855bac 100644 --- a/crates/lanspread-peer/src/install/transaction.rs +++ b/crates/lanspread-peer/src/install/transaction.rs @@ -1,4 +1,5 @@ use std::{ + collections::HashSet, io::ErrorKind, path::{Path, PathBuf}, sync::Arc, @@ -115,7 +116,7 @@ pub async fn uninstall(game_root: &Path, id: &str) -> eyre::Result<()> { } } -pub async fn recover_on_startup(game_dir: &Path) -> eyre::Result<()> { +pub async fn recover_on_startup(game_dir: &Path, active_ids: &HashSet) -> eyre::Result<()> { recover_download_transients(game_dir).await?; let mut entries = match tokio::fs::read_dir(game_dir).await { @@ -135,6 +136,10 @@ pub async fn recover_on_startup(game_dir: &Path) -> eyre::Result<()> { if id == ".lanspread" { continue; } + if active_ids.contains(&id) { + log::debug!("Skipping recovery for active game root {id}"); + continue; + } recover_game_root(&entry.path(), &id).await?; } @@ -457,6 +462,7 @@ impl From for FsEntryState { #[cfg(test)] mod tests { use std::{ + collections::HashSet, path::{Path, PathBuf}, sync::{Arc, Mutex}, }; @@ -551,6 +557,29 @@ mod tests { assert_eq!(intent.state, InstallIntentState::None); } + #[tokio::test] + async fn install_unpacks_multiple_root_eti_archives_in_sorted_order() { + let temp = TempDir::new(); + let root = temp.game_root(); + write_file(&root.join("b.eti"), b"archive"); + write_file(&root.join("a.eti"), b"archive"); + write_file(&root.join("version.ini"), b"20250101"); + let unpacker = Arc::new(FakeUnpacker::default()); + + install(&root, "game", unpacker.clone()) + .await + .expect("install should succeed"); + + let archives = unpacker + .archives + .lock() + .expect("archive list should not be poisoned") + .iter() + .filter_map(|path| path.file_name()?.to_str().map(ToOwned::to_owned)) + .collect::>(); + assert_eq!(archives, vec!["a.eti", "b.eti"]); + } + #[tokio::test] async fn update_failure_restores_previous_local() { let temp = TempDir::new(); @@ -571,6 +600,26 @@ mod tests { assert_eq!(intent.state, InstallIntentState::None); } + #[tokio::test] + async fn update_success_promotes_new_local_and_removes_backup() { + let temp = TempDir::new(); + let root = temp.game_root(); + write_file(&root.join("game.eti"), b"archive"); + write_file(&root.join("version.ini"), b"20250101"); + write_file(&root.join("local").join("old.txt"), b"old"); + + update(&root, "game", successful_unpacker()) + .await + .expect("update should succeed"); + + assert!(root.join("local").join("payload.txt").is_file()); + assert!(!root.join("local").join("old.txt").exists()); + assert!(!root.join(".local.installing").exists()); + assert!(!root.join(".local.backup").exists()); + let intent = read_intent(&root, "game").await; + assert_eq!(intent.state, InstallIntentState::None); + } + #[tokio::test] async fn uninstall_removes_only_local_install() { let temp = TempDir::new(); @@ -648,4 +697,20 @@ mod tests { assert!(!root.join(VERSION_TMP_FILE).exists()); assert!(!root.join(VERSION_DISCARDED_FILE).exists()); } + + #[tokio::test] + async fn startup_recovery_skips_active_game_roots() { + let temp = TempDir::new(); + let active_root = temp.0.join("active"); + let inactive_root = temp.0.join("inactive"); + write_file(&active_root.join(VERSION_TMP_FILE), b"tmp"); + write_file(&inactive_root.join(VERSION_TMP_FILE), b"tmp"); + + recover_on_startup(&temp.0, &HashSet::from(["active".to_string()])) + .await + .expect("recovery should succeed"); + + assert!(active_root.join(VERSION_TMP_FILE).is_file()); + assert!(!inactive_root.join(VERSION_TMP_FILE).exists()); + } } diff --git a/crates/lanspread-peer/src/lib.rs b/crates/lanspread-peer/src/lib.rs index f19e7c2..b1f26e4 100644 --- a/crates/lanspread-peer/src/lib.rs +++ b/crates/lanspread-peer/src/lib.rs @@ -58,7 +58,6 @@ use crate::{ handle_list_games_command, handle_set_game_dir_command, handle_uninstall_game_command, - handle_update_game_command, load_local_library, }, }; @@ -159,8 +158,6 @@ pub enum PeerCommand { }, /// Install already-downloaded archives into `local/`. InstallGame { id: String }, - /// Update an installed game from already-downloaded archives. - UpdateGame { id: String }, /// Remove only the `local/` install for a game. UninstallGame { id: String }, /// Set the local game directory. @@ -291,9 +288,6 @@ async fn handle_peer_commands( PeerCommand::InstallGame { id } => { handle_install_game_command(ctx, tx_notify_ui, id).await; } - PeerCommand::UpdateGame { id } => { - handle_update_game_command(ctx, tx_notify_ui, id).await; - } PeerCommand::UninstallGame { id } => { handle_uninstall_game_command(ctx, tx_notify_ui, id).await; } diff --git a/crates/lanspread-peer/src/local_games.rs b/crates/lanspread-peer/src/local_games.rs index d779444..25b41dc 100644 --- a/crates/lanspread-peer/src/local_games.rs +++ b/crates/lanspread-peer/src/local_games.rs @@ -8,7 +8,14 @@ use std::{ time::{SystemTime, UNIX_EPOCH}, }; -use lanspread_db::db::{Game, GameDB, GameFileDescription}; +use lanspread_db::db::{ + AVAILABILITY_DOWNLOADING, + AVAILABILITY_LOCAL_ONLY, + AVAILABILITY_READY, + Game, + GameDB, + GameFileDescription, +}; use lanspread_proto::{Availability, GameSummary}; use serde::{Deserialize, Serialize}; @@ -380,12 +387,21 @@ pub(crate) fn game_from_summary(summary: &GameSummary) -> Game { size: summary.size, downloaded: summary.downloaded, installed: summary.installed, + availability: availability_label(&summary.availability).to_string(), eti_game_version: summary.eti_version.clone(), local_version: summary.eti_version.clone(), peer_count: 0, } } +pub(crate) fn availability_label(availability: &Availability) -> &'static str { + match availability { + Availability::Ready => AVAILABILITY_READY, + Availability::Downloading => AVAILABILITY_DOWNLOADING, + Availability::LocalOnly => AVAILABILITY_LOCAL_ONLY, + } +} + struct IndexUpdate { summary: Option, changed: bool, diff --git a/crates/lanspread-peer/src/peer_db.rs b/crates/lanspread-peer/src/peer_db.rs index 455f95b..ec72d04 100644 --- a/crates/lanspread-peer/src/peer_db.rs +++ b/crates/lanspread-peer/src/peer_db.rs @@ -7,10 +7,10 @@ use std::{ time::{Duration, Instant}, }; -use lanspread_db::db::{Game, GameFileDescription}; +use lanspread_db::db::{AVAILABILITY_READY, Game, GameFileDescription}; use lanspread_proto::{Availability, GameSummary, LibraryDelta, LibrarySnapshot}; -use crate::library::compute_library_digest; +use crate::{library::compute_library_digest, local_games::availability_label}; pub type PeerId = String; /// Information about a discovered peer. @@ -293,6 +293,10 @@ impl PeerGameDB { } if game_is_ready(game) { existing.downloaded = true; + existing.availability = AVAILABILITY_READY.to_string(); + } else if !existing.downloaded { + existing.availability = + availability_label(&game.availability).to_string(); } if game.installed { existing.installed = true; @@ -744,6 +748,7 @@ fn summary_to_game(summary: &GameSummary) -> Game { size: summary.size, downloaded: summary.downloaded, installed: summary.installed, + availability: availability_label(&summary.availability).to_string(), eti_game_version, local_version: None, peer_count: 0, @@ -754,6 +759,8 @@ fn summary_to_game(summary: &GameSummary) -> Game { mod tests { use std::net::SocketAddr; + use lanspread_db::db::AVAILABILITY_LOCAL_ONLY; + use super::*; fn addr(port: u16) -> SocketAddr { @@ -817,6 +824,7 @@ mod tests { assert_eq!(games.len(), 1); assert_eq!(games[0].peer_count, 0); assert!(!games[0].downloaded); + assert_eq!(games[0].availability, AVAILABILITY_LOCAL_ONLY); assert_eq!(games[0].eti_game_version, None); assert!(db.peers_with_game("game").is_empty()); diff --git a/crates/lanspread-peer/src/remote_peer.rs b/crates/lanspread-peer/src/remote_peer.rs index 2e4def0..868f2f0 100644 --- a/crates/lanspread-peer/src/remote_peer.rs +++ b/crates/lanspread-peer/src/remote_peer.rs @@ -2,7 +2,12 @@ use std::{collections::HashMap, net::SocketAddr, sync::Arc}; -use lanspread_db::db::Game; +use lanspread_db::db::{ + AVAILABILITY_DOWNLOADING, + AVAILABILITY_LOCAL_ONLY, + AVAILABILITY_READY, + Game, +}; use lanspread_proto::{Availability, GameSummary}; use tokio::sync::RwLock; @@ -26,10 +31,12 @@ pub async fn ensure_peer_id_for_addr( } pub fn summary_from_game(game: &Game) -> GameSummary { - let availability = if game.downloaded { - Availability::Ready - } else { - Availability::LocalOnly + let availability = match game.availability.as_str() { + AVAILABILITY_READY => Availability::Ready, + AVAILABILITY_DOWNLOADING => Availability::Downloading, + AVAILABILITY_LOCAL_ONLY => Availability::LocalOnly, + _ if game.downloaded => Availability::Ready, + _ => Availability::LocalOnly, }; GameSummary { diff --git a/crates/lanspread-proto/src/lib.rs b/crates/lanspread-proto/src/lib.rs index e2a2fab..6d5b46f 100644 --- a/crates/lanspread-proto/src/lib.rs +++ b/crates/lanspread-proto/src/lib.rs @@ -7,6 +7,8 @@ pub const PROTOCOL_VERSION: u32 = 2; #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)] pub enum Availability { Ready, + /// Wire-compatible transitional state. Local library summaries currently + /// suppress active operations instead of advertising this value. Downloading, LocalOnly, } diff --git a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs index ac1f791..59f3c00 100644 --- a/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs +++ b/crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs @@ -9,7 +9,13 @@ use std::{ use eyre::bail; use lanspread_compat::eti::get_games; -use lanspread_db::db::{Game, GameDB, GameFileDescription}; +use lanspread_db::db::{ + AVAILABILITY_LOCAL_ONLY, + AVAILABILITY_READY, + Game, + GameDB, + GameFileDescription, +}; use lanspread_peer::{ InstallOperation, PeerCommand, @@ -356,6 +362,11 @@ fn update_game_installation_state(game: &mut Game, games_root: &Path) { let installed = local_install_is_present(&game_path); game.installed = installed; + game.availability = if downloaded { + AVAILABILITY_READY.to_string() + } else { + AVAILABILITY_LOCAL_ONLY.to_string() + }; // Size stays anchored to bundled game.db; skip expensive recalculation. @@ -534,6 +545,23 @@ async fn update_game_directory(app_handle: tauri::AppHandle, path: String) -> ta } let state = app_handle.state::(); + let current_path = state.games_folder.read().await.clone(); + let active_ids = state + .active_operations + .read() + .await + .keys() + .cloned() + .collect::>(); + if current_path != path && !active_ids.is_empty() { + log::warn!( + "Rejecting game directory change to {} while UI operations are active for: {}", + games_folder.display(), + active_ids.join(", ") + ); + return Ok(()); + } + *state.games_folder.write().await = path; ensure_bundled_game_db_loaded(&app_handle).await; @@ -601,6 +629,9 @@ async fn update_local_games_in_db(local_games: Vec, app: AppHandle) { if let Some(existing_game) = game_db.get_mut_game_by_id(&local_game.id) { existing_game.downloaded = local_game.downloaded; existing_game.installed = local_game.installed; + existing_game + .availability + .clone_from(&local_game.availability); existing_game .local_version .clone_from(&local_game.local_version); @@ -618,6 +649,7 @@ async fn update_local_games_in_db(local_games: Vec, app: AppHandle) { ); game.downloaded = false; game.installed = false; + game.availability = AVAILABILITY_LOCAL_ONLY.to_string(); game.local_version = None; } } @@ -887,7 +919,7 @@ async fn handle_peer_event(app_handle: &AppHandle, event: PeerEvent) { .remove(&id); emit_game_id_event( app_handle, - "game-unpack-finished", + "game-install-finished", &id, "PeerEvent::InstallGameFinished", ); diff --git a/crates/lanspread-tauri-deno-ts/src/App.tsx b/crates/lanspread-tauri-deno-ts/src/App.tsx index 5e91f02..bbbd164 100644 --- a/crates/lanspread-tauri-deno-ts/src/App.tsx +++ b/crates/lanspread-tauri-deno-ts/src/App.tsx @@ -40,6 +40,7 @@ interface Game { thumbnail: Uint8Array | number[]; downloaded: boolean; installed: boolean; + availability: GameAvailability; install_status: InstallStatus; eti_game_version?: string; local_version?: string; @@ -48,6 +49,12 @@ interface Game { peer_count: number; } +enum GameAvailability { + Ready = 'Ready', + Downloading = 'Downloading', + LocalOnly = 'LocalOnly', +} + interface GameThumbnailProps { gameId: string; alt: string; @@ -105,6 +112,7 @@ const mergeGameUpdate = (game: Game, previous?: Game): Game => { return { ...game, + availability: game.availability ?? (game.downloaded ? GameAvailability.Ready : GameAvailability.LocalOnly), install_status: installStatus, status_message: localStateChanged ? undefined : previous?.status_message, status_level: localStateChanged ? undefined : previous?.status_level, @@ -289,11 +297,11 @@ const App = () => { }, [gameDir]); useEffect(() => { - // Listen for game-unpack-finished events specifically - const setupUnpackListener = async () => { - const unlisten = await listen('game-unpack-finished', (event) => { + // Listen for game-install-finished events specifically + const setupInstallFinishedListener = async () => { + const unlisten = await listen('game-install-finished', (event) => { const game_id = event.payload as string; - console.log(`🗲 game-unpack-finished ${game_id} event received`); + console.log(`🗲 game-install-finished ${game_id} event received`); clearCheckingPeersTimeout(game_id); setGameItems(prev => prev.map(item => item.id === game_id ? { @@ -316,7 +324,7 @@ const App = () => { return unlisten; }; - setupUnpackListener(); + setupInstallFinishedListener(); }, [gameDir]); useEffect(() => { @@ -739,7 +747,7 @@ const App = () => { {(item.size / 1024 / 1024 / 1024).toFixed(1)} GB
- {item.installed && !item.downloaded && ( + {item.installed && item.availability === GameAvailability.LocalOnly && ( LocalOnly )} {!item.installed && item.downloaded && item.local_version && (