ddidderr
b5d20c1e72
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
119 lines
12 KiB
Markdown
119 lines
12 KiB
Markdown
# 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 `<game_id>/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<RwLock<HashSet<String>>>` (`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<String, OperationKind>` 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.
|