Files
lanspread/FOLLOW_UP_PLAN.md
T
ddidderr b5d20c1e72 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
2026-05-16 08:50:51 +02:00

6.5 KiB

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-finishedgame-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 restorerestore_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).