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
This commit is contained in:
@@ -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<String, CancellationToken>` for cancellation tokens (`context.rs:38`).
|
||||
|
||||
This is justifiable: `CancellationToken` doesn't fit naturally inside `HashMap<String, OperationKind>` 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<CancellationToken> }`. 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.
|
||||
Reference in New Issue
Block a user