crazy
This commit is contained in:
+154
@@ -0,0 +1,154 @@
|
||||
# Backlog
|
||||
|
||||
Smells and small inconsistencies found during post-PLAN.md review. None of
|
||||
these block merging — they are tracked here so they aren't forgotten and so
|
||||
they don't reopen as "new findings" the next time someone reads the code.
|
||||
|
||||
**Rule of engagement:** items in this file get touched only when (a)
|
||||
someone hits the symptom in practice, or (b) work in a nearby area makes
|
||||
fixing the smell incidental. No batch refactor passes. No "while we're
|
||||
here" cleanups that grow beyond the in-scope change.
|
||||
|
||||
---
|
||||
|
||||
## Legacy peer protocol fallback contradicts the wire policy
|
||||
|
||||
CLAUDE.md / AGENTS.md: *"There is only one wire version — the current one.
|
||||
No legacy peers, no compatibility shims, no fallback paths for older
|
||||
builds."*
|
||||
|
||||
Live legacy paths:
|
||||
|
||||
- `crates/lanspread-peer/src/services/legacy.rs` exists and is called from
|
||||
`discovery.rs:183-188` when the `Hello` handshake fails.
|
||||
- `discovery.rs:134` synthesizes `legacy-{addr}` peer IDs when `proto_ver`
|
||||
is absent from mDNS TXT records.
|
||||
- `discovery.rs:169` treats `proto_ver.is_none()` as handshake-eligible.
|
||||
- `update_and_announce_games` (`handlers.rs:605-624`) branches on
|
||||
`FEATURE_LIBRARY_DELTA` and falls back to `announce_games_to_peer`
|
||||
(sending `Request::AnnounceGames`) for peers that don't advertise the
|
||||
feature.
|
||||
- `Request::AnnounceGames` is still defined in
|
||||
`lanspread-proto/src/lib.rs:75` and handled in
|
||||
`services/stream.rs:116`.
|
||||
|
||||
Functionally inert today — current-build peers don't drop `Hello` — but
|
||||
code and stated policy disagree. Either delete the paths or revert the
|
||||
policy.
|
||||
|
||||
---
|
||||
|
||||
## Tauri keeps a parallel filesystem-derived scan
|
||||
|
||||
The peer now owns the install state machine (per PLAN.md:11), but Tauri
|
||||
still re-derives local install/download state from disk on every event:
|
||||
|
||||
- `refresh_games_list` (`src-tauri/src/lib.rs:489`) fires after every
|
||||
`update_game_db`, `update_local_games_in_db`, and
|
||||
`update_game_directory`. It calls `set_all_uninstalled()` and re-runs
|
||||
`update_game_installation_state` over every bundled-DB entry,
|
||||
re-reading `version.ini`, re-checking `local/`, re-parsing version
|
||||
strings.
|
||||
- `update_local_games_in_db` (`src-tauri/src/lib.rs:667-704`) just merged
|
||||
the peer's authoritative `Game` values into the Tauri-side `GameDB`.
|
||||
Immediately after, `refresh_games_list` re-derives the same fields
|
||||
from disk and overwrites the merged result.
|
||||
- The per-ID rescan optimization in `local_monitor.rs` is completely
|
||||
undone on the Tauri side: every peer event triggers a whole-library
|
||||
disk walk.
|
||||
|
||||
Today both paths reach the same conclusion. The risk is forward-looking:
|
||||
the moment one of the two derivation rules changes (a new `availability`
|
||||
rule, a new sentinel, a new ignore name), the two scanners can disagree
|
||||
silently with no rule for which wins.
|
||||
|
||||
**Fix when convenient:** `refresh_games_list` accepts the peer's `Game`
|
||||
slice and trusts it for local fields. Tauri's bundled DB stays as the
|
||||
source of truth for static metadata (name, description, max_players,
|
||||
thumbnail mapping), but `downloaded`/`installed`/`local_version`/
|
||||
`availability` come from the peer. `update_game_installation_state` and
|
||||
`set_all_uninstalled` go away. The dead log branch at
|
||||
`src-tauri/src/lib.rs:397-402` is obviated naturally by this.
|
||||
|
||||
---
|
||||
|
||||
## `Availability::Downloading` is wire-defined but unreachable
|
||||
|
||||
`crates/lanspread-db/src/db.rs:36-41`. The variant exists and serializes
|
||||
but `build_game_summary` only emits `Ready` or `LocalOnly`.
|
||||
Operation-table gating handles the in-progress case instead.
|
||||
|
||||
`peer_db::get_all_games` has a code path that lets a remote-advertised
|
||||
`Downloading` summary contribute `eti_version` to aggregation. If a
|
||||
future maintainer re-enables emitting `Downloading` from
|
||||
`build_game_summary`, aggregation will treat such peers as
|
||||
not-downloadable but still pull their version info.
|
||||
|
||||
**Decide-and-document task:** either remove the variant (matches the
|
||||
"current wire only" policy) or add a comment in the proto enum naming
|
||||
the contract.
|
||||
|
||||
---
|
||||
|
||||
## `update_game_installation_state` dead log branch
|
||||
|
||||
`src-tauri/src/lib.rs:397-402`:
|
||||
|
||||
```rust
|
||||
if eti_package_exists(&game_path, &game.id) && !downloaded {
|
||||
log::debug!("Game ... has archives but no version.ini sentinel; treating as not downloaded");
|
||||
}
|
||||
```
|
||||
|
||||
Side-effect-only log line. Either delete or wire to a UI affordance
|
||||
("partial download — retry?"). Obviated naturally if/when the Tauri
|
||||
parallel scan goes away.
|
||||
|
||||
---
|
||||
|
||||
## Untested edge: Tauri reconciliation with dropped lifecycle events
|
||||
|
||||
The Rust `reconcile_active_operations` test
|
||||
(`src-tauri/src/lib.rs:1117-1153`) covers map replacement but not the
|
||||
realistic case of an event sequence with a missing `begin` or `finish`.
|
||||
The TS side merges in `App.tsx`. Real failure mode: a missing `finish`
|
||||
followed by a `LocalGamesUpdated` snapshot should clear the spinner,
|
||||
and today's code does — but it's not pinned by a test.
|
||||
|
||||
Add a test if the spinner ever gets stuck in practice.
|
||||
|
||||
---
|
||||
|
||||
## Documentation drift
|
||||
|
||||
`FOLLOW_UP_2.md` still lists two items as "Still open" that have
|
||||
landed:
|
||||
|
||||
- **#10 `save_library_index` non-atomic** — landed in `fdad162`, atomic
|
||||
temp+fsync+rename at `local_games.rs:169-184`.
|
||||
- **#11 Split `download.rs`** — landed in `a251233`, split under
|
||||
`crates/lanspread-peer/src/download/`.
|
||||
|
||||
Either mark the doc complete or delete it. Anyone reading it as status
|
||||
will be misled.
|
||||
|
||||
The collection of plan/follow-up/review docs in the repo root
|
||||
(`PLAN.md`, `PLAN_AVAILABILITY.md`, `PLAN_ATOMIC_INDEX.md`,
|
||||
`PLAN_DOWNLOAD_SPLIT.md`, `FOLLOW_UP_PLAN.md`, `FOLLOW_UP_2.md`,
|
||||
`REVIEW_STEP_1..4.md`, `IMPL_DECISIONS.md`) is also getting noisy. A
|
||||
single retrospective archive folder for completed plans would help; a
|
||||
future PLAN.md should be self-terminating with explicit acceptance
|
||||
criteria so it doesn't spawn this many trailing docs.
|
||||
|
||||
---
|
||||
|
||||
## How items leave this file
|
||||
|
||||
- Closed by fix → delete the entry, mention it in the commit.
|
||||
- Closed by decision ("we're not doing this") → delete the entry, no
|
||||
commit message ceremony needed.
|
||||
- Promoted to active work → move back to `FINDINGS.md` only when there's
|
||||
a concrete plan to fix it now.
|
||||
|
||||
This file does not grow unboundedly. If it does, that's a signal to
|
||||
either close items or stop adding to it.
|
||||
+161
@@ -0,0 +1,161 @@
|
||||
# Findings — Bugs to Fix Before Merging
|
||||
|
||||
Three bugs found in the post-PLAN.md implementation. Fix these, then merge.
|
||||
Everything else lives in `BACKLOG.md` and does not block.
|
||||
|
||||
---
|
||||
|
||||
## 1. `update_game` never fetches a fresh manifest from peers
|
||||
|
||||
PLAN.md:357 calls for `update_game` to send `GetGame`, *fetch fresh remote
|
||||
archives*, and trigger an auto-install as a transactional update. The
|
||||
implementation makes that path unreachable.
|
||||
|
||||
Trace:
|
||||
|
||||
1. Tauri `update_game` sends `PeerCommand::GetGame`
|
||||
(`crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs:162`).
|
||||
2. Peer's `handle_get_game_command` calls `try_serve_local_game` first
|
||||
(`crates/lanspread-peer/src/handlers.rs:88`).
|
||||
3. `try_serve_local_game` consults `local_download_available`, which returns
|
||||
true whenever `version.ini` is present locally and the ID is in the catalog
|
||||
(`crates/lanspread-peer/src/local_games.rs:48-66`). For any game the user
|
||||
has already downloaded, this is *always* true.
|
||||
4. The **local** file descriptions are returned via `GotGameFiles`. Tauri
|
||||
routes those into `DownloadGameFiles`.
|
||||
5. `handle_download_game_files_command:204-227` consults `peer_game_db` via
|
||||
`validate_file_sizes_majority`, so cached remote metadata *is* read. But
|
||||
the descriptions actually used for chunk planning are the local ones
|
||||
(`handlers.rs:189-195`). When peers advertise a newer version with
|
||||
different file sizes, the whitelist is empty and the path falls into
|
||||
"instant install of local archives." When sizes happen to match, we
|
||||
plan chunks against local descriptions and request those offsets from
|
||||
peers — which works only when peer-side files are identical to local.
|
||||
Either way, peers' current manifests are never read.
|
||||
|
||||
Net effect: "update" = re-extract whatever archives are on disk into
|
||||
`local/`. The flow PLAN.md described — fetch the newer archive from peers,
|
||||
then auto-install — does not exist.
|
||||
|
||||
**Fix candidates:**
|
||||
|
||||
- New `PeerCommand::FetchLatestFromPeers { id }` that skips the local-serve
|
||||
gate and asks one peer for its current manifest.
|
||||
- `PeerCommand::GetGame { id, force_peer: true }` flag honored by
|
||||
`try_serve_local_game`.
|
||||
- `try_serve_local_game` short-circuits only when local `eti_version` is ≥
|
||||
`peer_db.get_latest_version_for_game(id)`. The aggregation function
|
||||
already exists in `peer_db.rs:320`; nothing calls it for this purpose.
|
||||
|
||||
**Tests to add:** `update_game` actually pulls the newer manifest from a
|
||||
peer when one exists. Today this can't be tested because the code path
|
||||
doesn't exist.
|
||||
|
||||
---
|
||||
|
||||
## 2. `OperationGuard::Drop` is doing ordered state transitions
|
||||
|
||||
`crates/lanspread-peer/src/handlers.rs:254-279`:
|
||||
|
||||
```rust
|
||||
ctx.task_tracker.spawn(async move {
|
||||
let result = {
|
||||
let _download_state_guard = OperationGuard::download(...);
|
||||
download_game_files(...).await
|
||||
}; // guard drops here
|
||||
match result {
|
||||
Ok(()) => run_install_operation(&ctx_clone, ..., download_id).await,
|
||||
...
|
||||
}
|
||||
});
|
||||
```
|
||||
|
||||
`OperationGuard::Drop` (`context.rs:156-191`) tries `try_write` first, then
|
||||
falls back to `tokio::spawn(async { ... .write().await.remove(...) })` if the
|
||||
lock is contended. The contention happens because `active_operations` is read
|
||||
on every watcher tick, every `list_games`, every `can_serve_game`, every
|
||||
liveness sweep, every `update_and_announce_games` snapshot.
|
||||
|
||||
This is the wrong shape for the state transition. Drop is fire-and-forget;
|
||||
the synchronous code after the guard scope keeps running before the deferred
|
||||
removal lands. Two distinct symptoms of the same root cause:
|
||||
|
||||
1. **Install rejected:** `run_install_operation` calls `begin_operation`
|
||||
(`handlers.rs:336-339`) which does `Entry::Vacant` on the same map. If
|
||||
`begin_operation` wins the lock before the spawned remove task does, it
|
||||
sees the leftover `Downloading` entry and rejects the install. User sees
|
||||
`version.ini` on disk, no `local/`, no `InstallGameBegin`, no
|
||||
explanation.
|
||||
2. **Stale snapshot:** Post-finish refresh calls
|
||||
`active_operation_snapshot` (`handlers.rs:558`) before the deferred
|
||||
removal runs. UI receives one final snapshot saying the operation is
|
||||
active even though `InstallGameFinished` was already sent.
|
||||
|
||||
**Fix:** Explicit `async end_operation(...)` call before finish/refresh,
|
||||
under a single write lock. The same write that removes `Downloading`
|
||||
should insert `Installing`/`Updating` for the auto-install path, making
|
||||
the handoff atomic. Demote `OperationGuard` to crash-safety: only fires
|
||||
when the task panics or is aborted, and logs loudly when it does.
|
||||
|
||||
**Tests to add:**
|
||||
|
||||
- Hold a read lock on `active_operations` while `download_game_files`
|
||||
returns; assert the auto-install still proceeds.
|
||||
- Liveness path cancellation while multiple downloads are in flight;
|
||||
assert no duplicate failure events and no stuck operation-table
|
||||
entries.
|
||||
|
||||
---
|
||||
|
||||
## 3. Uncoordinated library-index writes
|
||||
|
||||
`scan_local_library` (`local_games.rs:533-615`) and `rescan_local_game`
|
||||
(`local_games.rs:617-639`) both load `library_index.json`, mutate the
|
||||
deserialized state, and save. Nothing serializes the two paths.
|
||||
|
||||
Call sites:
|
||||
|
||||
- `run_fallback_scan` (`local_monitor.rs:289`) → `scan_local_library`.
|
||||
- `run_gated_rescan` (`local_monitor.rs:261`) → `rescan_local_game`,
|
||||
spawned on the task tracker (line 253-258).
|
||||
- `load_local_library` (`handlers.rs:491`) → `scan_local_library`.
|
||||
- `refresh_local_game` (`handlers.rs:520`) → `rescan_local_game`.
|
||||
|
||||
A fallback-scan tick can land between a gated-rescan's load and save (or
|
||||
vice versa). Last writer wins; intermediate updates are silently dropped.
|
||||
|
||||
The piece of state that drifts in a user-visible way is `revision`: both
|
||||
writers compute `old.saturating_add(1)` and save `old+1`, while the
|
||||
in-memory `LocalLibraryState.revision` bumps independently in
|
||||
`update_from_scan`. After a restart, disk-revision can be lower than
|
||||
peers expect, breaking `LibraryDelta.from_rev` matching — peers will
|
||||
fall back to snapshots and the delta optimization is undone.
|
||||
|
||||
**Fix candidates:** `tokio::Mutex` around index I/O, or move the index
|
||||
ownership into the same actor that owns `LocalLibraryState` so all
|
||||
mutations go through one channel.
|
||||
|
||||
---
|
||||
|
||||
## What's *not* in this file
|
||||
|
||||
Everything else found during review is in `BACKLOG.md`. Notable items
|
||||
include: Tauri-side parallel scanning, legacy peer protocol fallback,
|
||||
unreachable `Availability::Downloading` variant, stale FOLLOW_UP_2.md.
|
||||
None of those block merging.
|
||||
|
||||
---
|
||||
|
||||
## Definition of done for this branch
|
||||
|
||||
- Fixes for #1, #2, #3 land.
|
||||
- Tests listed under each fix land.
|
||||
- `just test`, `just clippy`, `just build` clean.
|
||||
- Manual: install a game, then update it while a peer advertises a newer
|
||||
version, then uninstall it. Verify the version actually changes after
|
||||
update (covers #1) and that the UI doesn't get stuck on a spinner
|
||||
after operations complete (covers #2).
|
||||
|
||||
Once those are green, this branch is done. Re-reviewing will surface
|
||||
more smells; don't run another review unless something behaves wrong
|
||||
when tested manually.
|
||||
@@ -1,54 +0,0 @@
|
||||
# Follow-up Plan #2
|
||||
|
||||
State of FOLLOW_UP_PLAN.md after two implementation rounds. Items here are
|
||||
what's still open, plus notes for follow-up items completed after this file was
|
||||
created.
|
||||
|
||||
## 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.
|
||||
|
||||
## Completed after this file was created
|
||||
|
||||
- Tauri-side `active_operations` reconciliation: `PeerEvent::LocalGamesUpdated`
|
||||
now carries an authoritative active-operation snapshot, the Tauri bridge
|
||||
replaces its UI operation map from that snapshot, and the React game-list
|
||||
merge uses the snapshot to clear stale download/install/update/uninstall
|
||||
spinners even when a begin/finish/fail event was missed.
|
||||
- Install-recovery matrix: `recovery_covers_install_matrix_rows` now covers the
|
||||
ten named Installing / Updating / Uninstalling rows from PLAN.md, including
|
||||
markerless reserved directories while an intent proves ownership.
|
||||
- Update rollback on commit-rename failure:
|
||||
`update_commit_rename_failure_restores_previous_local` forces a post-extract
|
||||
commit conflict without production hooks and verifies backup restore.
|
||||
- Uninstall delete-failure restore:
|
||||
`uninstall_delete_failure_restores_backup` covers the rollback path on Unix,
|
||||
where directory permissions can force recursive delete failure without hooks.
|
||||
- Installed-only → Ready rescan:
|
||||
`rescan_promotes_installed_only_game_to_ready_when_sentinel_appears` pins the
|
||||
cached-index transition after a user adds `version.ini` to a local-only root.
|
||||
- Scanner gating dispatch: local monitor tests now cover active-operation event
|
||||
drops, burst collapse, fallback sideload pickup, and non-catalog roots leaving
|
||||
no library entry or delta.
|
||||
- Serve gating dispatch: stream tests now cover `GetGame` response gates and
|
||||
the shared full-file/chunk transfer gate for non-catalog, active-operation,
|
||||
missing-sentinel, and local-path cases.
|
||||
- TempDir test helper: peer tests now share `test_support::TempDir` instead of
|
||||
carrying per-module copies.
|
||||
- Typed availability: `lanspread-db::Game::availability` now uses the shared
|
||||
serde enum, with `lanspread-proto` re-exporting it for wire summaries. The
|
||||
JSON spelling stays `"Ready" | "Downloading" | "LocalOnly"`.
|
||||
|
||||
## Still open
|
||||
|
||||
### Code hygiene
|
||||
|
||||
#### 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.
|
||||
@@ -1,124 +0,0 @@
|
||||
# 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).
|
||||
@@ -1,463 +0,0 @@
|
||||
# Plan: Transactional Local Installs, Intent-Logged Recovery, Event-Gated Scans
|
||||
|
||||
## Summary
|
||||
|
||||
- `local/` being a directory is the install signal. The contents of `local/` are the user's responsibility, not ours.
|
||||
- `version.ini` is the **download completion sentinel**: a game is downloaded iff `version.ini` exists at the game-root level. `version.ini` is the last file written by every successful download, via atomic rename.
|
||||
- Each game root carries a per-game JSON **intent log** for install operations, updated atomically before and after every state-changing FS operation. Startup recovery is a `(recorded_intent × fs_state)` lookup.
|
||||
- Installs/updates/uninstalls run as transactions over each game root using fixed-name staging and backup directories, marked Lanspread-owned to prevent collisions with user data.
|
||||
- Downloads are transactional via a single sentinel: old `version.ini` is renamed aside at the start, new `version.ini` is written atomically at the end. No intent log entry needed for downloads — the FS state is authoritative.
|
||||
- Filesystem watcher replaces 15s polling. Events are gated by per-game operation locks and a per-game rescan-pending flag — no time-based debounce. A 5-minute fallback scan reconciles missed events.
|
||||
- Install transaction logic moves out of `src-tauri` into a new `lanspread-peer::install` module so the peer owns the state machine end-to-end. Unpacker is injected at peer startup so the peer crate stays Tauri-free.
|
||||
- A game is transferable to peers only when its ID is in the ETI catalog, `version.ini` is present, and no operation is active. Non-catalog game roots are ignored for peer announcements — we currently have no system to determine title/thumbnail for non-catalog games, so they're future scope.
|
||||
|
||||
## Wire protocol
|
||||
|
||||
Unchanged. Existing `library_rev` / `LibraryDelta` / `manifest_hash` machinery already handles announcement on any state change; uninstall is just another library change. `version.ini` remains part of the per-game manifest.
|
||||
|
||||
## Layout
|
||||
|
||||
### Game-dir root
|
||||
|
||||
```
|
||||
<game_dir>/
|
||||
.lanspread/ -- peer-wide cache (ignored by watcher and scans)
|
||||
<game_id_1>/...
|
||||
<game_id_2>/...
|
||||
...
|
||||
```
|
||||
|
||||
### Per game root
|
||||
|
||||
```
|
||||
<game_dir>/<game_id>/
|
||||
<game_id>.eti -- main archive
|
||||
*.eti -- additional archives (multi-archive games are valid)
|
||||
version.ini -- download sentinel. Presence = downloaded.
|
||||
local/ -- committed install. Is-a-directory ⇒ installed.
|
||||
|
||||
.version.ini.tmp -- transient: version.ini being staged for atomic rename
|
||||
.version.ini.discarded -- transient: previous version.ini parked aside during re-download
|
||||
.local.installing/ -- extraction staging dir (Lanspread-owned)
|
||||
.local.backup/ -- previous install retained for rollback (Lanspread-owned)
|
||||
.lanspread.json -- per-game install intent log (atomic writes)
|
||||
```
|
||||
|
||||
Fixed names; no timestamps. The intent log disambiguates "fresh" vs "stale" install-staging/backup dirs; for downloads the FS state alone is authoritative.
|
||||
|
||||
### Ownership marker
|
||||
|
||||
When the peer creates `.local.installing/` or `.local.backup/`, it drops a zero-byte `.lanspread_owned` file inside as soon as the directory exists.
|
||||
|
||||
Two ownership rules apply at recovery time:
|
||||
|
||||
- **Intent says `None`**: the marker is the only signal. Only `.local.*` dirs carrying `.lanspread_owned` may be touched; markerless `.local.*` dirs are user data — log and leave alone.
|
||||
- **Intent says `Installing` / `Updating` / `Uninstalling`**: the corresponding `.local.installing/` and `.local.backup/` are unconditionally Lanspread-owned regardless of marker. This covers the inevitable crash window between `rename(local → .local.backup)` and the marker drop, and between `mkdir(.local.installing)` and the marker drop. The intent log already proves ownership; the marker is belt-and-suspenders for the `None`-intent case.
|
||||
|
||||
Download-related transient files (`.version.ini.tmp`, `.version.ini.discarded`) use fixed reserved names — no marker is needed because the names are reserved by Lanspread and any conflict would already be a name collision with our own scheme.
|
||||
|
||||
## State model
|
||||
|
||||
```
|
||||
downloaded = version.ini exists (regular file, at game-root level)
|
||||
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.
|
||||
|
||||
`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.
|
||||
|
||||
## Intent log (install operations only)
|
||||
|
||||
Per-game JSON at `<game_root>/.lanspread.json`:
|
||||
|
||||
```json
|
||||
{
|
||||
"schema_version": 1,
|
||||
"id": "...",
|
||||
"recorded_at": <unix_ts>,
|
||||
"state": "None" | "Installing" | "Updating" | "Uninstalling",
|
||||
"eti_version": "20251028"?
|
||||
}
|
||||
```
|
||||
|
||||
`state: "None"` means no install-side operation in flight; FS is the truth. The intent log does **not** track download state — that's read directly from `version.ini` / `.version.ini.*` FS signals.
|
||||
|
||||
### Atomic write
|
||||
|
||||
1. Write `.lanspread.json.tmp`.
|
||||
2. `file.sync_all()` (fsync the data).
|
||||
3. `rename(.lanspread.json.tmp, .lanspread.json)`.
|
||||
4. `fsync(parent_dir)` on Unix; on Windows skip (no portable directory-fsync equivalent).
|
||||
|
||||
### Intent transitions
|
||||
|
||||
- **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.
|
||||
|
||||
## Startup recovery
|
||||
|
||||
### Install recovery matrix
|
||||
|
||||
For each game root, read intent + inspect FS, then act:
|
||||
|
||||
```
|
||||
recorded | local/ | .local.installing | .local.backup | action
|
||||
-------------|--------|-------------------|---------------|----------------------------------------
|
||||
None | - | - | - | trust FS; leave markerless .local.* alone (user data); sweep only markered .local.* orphans
|
||||
Installing | yes | - | - | commit landed, intent crash; write None
|
||||
Installing | - | yes | - | sweep staging; write None
|
||||
Installing | - | - | - | crash before any staging; write None
|
||||
Updating | no | yes | yes | sweep staging, restore backup; write None (old ver)
|
||||
Updating | yes | yes (stale) | yes (stale) | sweep both; write None (new ver — commit landed before intent flush)
|
||||
Updating | yes | - | yes | commit landed, intent crash before cleanup; sweep backup, write None
|
||||
Updating | yes | - | - | commit + cleanup landed, intent crash; write None
|
||||
Uninstalling | no | - | yes | finish: delete backup; write None
|
||||
Uninstalling | no | - | - | write None
|
||||
Uninstalling | yes | - | - | intent crash before any FS work; redo uninstall from step 1
|
||||
```
|
||||
|
||||
**Default rule**: for any `(intent, fs)` combination not listed, write intent matching FS observation; do not mutate FS. Conservative bias — never delete unless we have positive proof of ownership (intent is `Installing` / `Updating` / `Uninstalling` for that ID, or the `.local.*` dir carries the marker).
|
||||
|
||||
**Sideloading** (user drops a game dir in via file manager): no intent log, FS-only observation. The "None" row handles it. JSON corruption or accidental deletion follows the same path — treat missing/unparseable intent as `None`.
|
||||
|
||||
### Download recovery
|
||||
|
||||
Independent of the install intent log; pure FS sweep:
|
||||
|
||||
- `.version.ini.tmp` present → partial commit; delete.
|
||||
- `.version.ini.discarded` present → interrupted download; delete (do not restore; `.eti` bytes may be partially overwritten).
|
||||
- After sweeping, the `version.ini` presence/absence is authoritative.
|
||||
|
||||
## Transaction primitives
|
||||
|
||||
All in the new `lanspread-peer::install` module (install) plus changes to `download.rs` (download). The same per-peer **operation table** holds `Downloading | Installing | Updating | Uninstalling` entries.
|
||||
|
||||
### Download transaction
|
||||
|
||||
Driven by `download_game_files`.
|
||||
|
||||
1. Operation table: insert `Downloading` for game ID.
|
||||
2. If `version.ini` exists in the game root, `rename(version.ini, .version.ini.discarded)`.
|
||||
3. `prepare_game_storage` runs over file descs **filtered to exclude** `is_version_ini()` entries — non-version.ini files are pre-allocated and overwritten in place by chunk writes as today.
|
||||
4. Run parallel chunk downloads as today.
|
||||
5. Version.ini chunks (small file, typically a few bytes) are routed to an in-memory buffer instead of disk during chunk receipt.
|
||||
6. After all non-version.ini downloads complete and the version.ini buffer is full:
|
||||
- Write buffer to `.version.ini.tmp`.
|
||||
- `file.sync_all()`.
|
||||
- `rename(.version.ini.tmp, version.ini)`.
|
||||
7. Sweep `.version.ini.discarded` (best-effort).
|
||||
8. Operation table: remove `Downloading`.
|
||||
9. Emit `DownloadGameFilesFinished`.
|
||||
10. Peer auto-fires `InstallGame { id }` (see install transaction).
|
||||
|
||||
**Rollback** (any failure or cancel):
|
||||
|
||||
- Sweep `.version.ini.tmp` if any.
|
||||
- Sweep `.version.ini.discarded`.
|
||||
- Do **not** restore old version.ini. `.eti` bytes may be partially overwritten and would no longer match the old state.
|
||||
- Operation table: remove `Downloading`.
|
||||
- Emit `DownloadGameFilesFailed`.
|
||||
|
||||
A failed download is destructive to the previous downloaded state but leaves `local/` untouched. The user can re-trigger download. This is acceptable because `local/` is what the user plays from, and download is fundamentally about replacing the on-disk archive.
|
||||
|
||||
### Install (no prior `local/`)
|
||||
|
||||
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.
|
||||
5. Write intent `None`.
|
||||
6. Emit success event; trigger rescan.
|
||||
|
||||
**Rollback** (any step before commit): `rm -rf .local.installing`; write intent `None`; emit failure.
|
||||
|
||||
### Update (prior `local/` present)
|
||||
|
||||
1. Write intent `Updating`.
|
||||
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.
|
||||
6. Write intent `None`.
|
||||
7. Emit success event; trigger rescan.
|
||||
8. Best-effort `rm -rf .local.backup`; failure logged, not fatal.
|
||||
|
||||
**Rollback**: `rm -rf .local.installing` (best-effort); `rename(.local.backup, local)`; write intent `None`; emit failure.
|
||||
|
||||
### Uninstall
|
||||
|
||||
1. Write intent `Uninstalling`.
|
||||
2. `rename(local, .local.backup)`; drop `.lanspread_owned` marker.
|
||||
3. `rm -rf .local.backup`.
|
||||
4. Write intent `None`.
|
||||
5. Emit success event; trigger rescan.
|
||||
|
||||
**Rollback** (step 3 fails): attempt `rename(.local.backup, local)`; surface error.
|
||||
|
||||
### unrar correctness
|
||||
|
||||
Non-zero exit status from unrar is an error. The `None` intent and the user-facing "unpack-finished" event fire only after both extraction and the promotion rename succeed.
|
||||
|
||||
## Peer crate changes
|
||||
|
||||
### New module `crates/lanspread-peer/src/install/`
|
||||
|
||||
```
|
||||
mod.rs -- public surface: install(), update(), uninstall(), recover_on_startup()
|
||||
transaction.rs -- staging/backup/commit/rollback primitives over a game root
|
||||
intent.rs -- atomic read/write of per-game intent JSON
|
||||
unpack.rs -- unrar invocation, via the injected Unpacker trait
|
||||
```
|
||||
|
||||
### `Unpacker` injection
|
||||
|
||||
Object-safe boxed-future trait — no `async_trait` dep, no `async fn` in trait:
|
||||
|
||||
```rust
|
||||
use std::{future::Future, path::Path, pin::Pin};
|
||||
|
||||
pub type UnpackFuture<'a> = Pin<Box<dyn Future<Output = eyre::Result<()>> + Send + 'a>>;
|
||||
|
||||
pub trait Unpacker: Send + Sync {
|
||||
fn unpack<'a>(&'a self, archive: &'a Path, dest: &'a Path) -> UnpackFuture<'a>;
|
||||
}
|
||||
```
|
||||
|
||||
`start_peer` gains a 4th argument:
|
||||
|
||||
```rust
|
||||
pub fn start_peer(
|
||||
game_dir: impl Into<PathBuf>,
|
||||
tx_notify_ui: UnboundedSender<PeerEvent>,
|
||||
peer_game_db: Arc<RwLock<PeerGameDB>>,
|
||||
unpacker: Arc<dyn Unpacker>,
|
||||
catalog: Arc<RwLock<HashSet<String>>>,
|
||||
) -> eyre::Result<PeerRuntimeHandle>
|
||||
```
|
||||
|
||||
Tauri provides the impl that wraps `app_handle.shell().sidecar("unrar")` and passes it in. The peer crate stays Tauri-free.
|
||||
|
||||
### Catalog injection
|
||||
|
||||
The peer must know which game IDs the ETI catalog covers — only those IDs are announced to peers, only those flow through the install state machine, and only those participate in download/serve paths. The catalog set is owned by Tauri (loaded from the bundled `game.db` in `load_bundled_game_db`) and shared with the peer as `Arc<RwLock<HashSet<String>>>`. The RwLock leaves room for a future "catalog refresh without peer restart" path; today the set is populated once at startup before `start_peer` is called.
|
||||
|
||||
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` / `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.
|
||||
|
||||
### Download changes (`crates/lanspread-peer/src/download.rs`)
|
||||
|
||||
- `prepare_game_storage`: filter `file_descs` to exclude `desc.is_version_ini()` entries. Non-version.ini files are pre-allocated as today.
|
||||
- `download_game_files`:
|
||||
- Partition `game_file_descs` into version.ini and non-version.ini.
|
||||
- Insert `Downloading` into the operation table; rename existing `version.ini` to `.version.ini.discarded` if present.
|
||||
- Run parallel chunked downloads for non-version.ini files (existing path).
|
||||
- Route version.ini chunk writes to an in-memory buffer keyed by relative path.
|
||||
- After all chunks succeed, commit each version.ini buffer atomically via `.version.ini.tmp` → rename.
|
||||
- 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 `UninstallGame { id }`.
|
||||
|
||||
### Operation table
|
||||
|
||||
A single per-peer table holds the in-progress operation for each game ID (`Downloading | Installing | Updating | Uninstalling`). Consulted by:
|
||||
|
||||
- `local_download_available` (manifest serve path, used by `handle_get_game`) — refuse `GetGame` for a game during any active operation, when `version.ini` is absent, or when the ID is not in the catalog.
|
||||
- File-stream handlers (`handle_file_data_request`, `handle_file_chunk_request` in `services/stream.rs`) — refuse `GetGameFileData` and `GetGameFileChunk` requests when the ID has an active operation, when `version.ini` is absent at the game root, or when the ID is not in the catalog. A remote peer may already hold a manifest from before our operation began; these handlers must re-verify per request rather than rely on `local_download_available` having gated the original manifest fetch. Additional rule: `local/`-relative paths are rejected unconditionally — manifests never contain `local/` entries, but defense-in-depth against a malicious or stale request.
|
||||
- Watcher event handler — drop events for IDs with active operations.
|
||||
- Scanner — preserve last-known summary for IDs with active operations; do not emit transient deltas.
|
||||
- Library announcement — do not announce in `LibraryDelta` while an operation is active.
|
||||
|
||||
### Aggregation and candidate selection (`peer_db.rs`)
|
||||
|
||||
The `Availability` flag must propagate through peer-side aggregation; today it does not, which would make `LocalOnly` peers count as downloadable.
|
||||
|
||||
- `get_all_games`: when aggregating `GameSummary` values across peers, `Game.peer_count` counts only peers whose advertised `availability == Ready`. `LocalOnly` peers do not contribute to `peer_count` and do not flip the aggregated `downloaded` flag.
|
||||
- `peers_with_game` and `peers_with_latest_version`: filter to peers with `availability == Ready` before returning addresses. A peer advertising `LocalOnly` must never be selected as a download source — the download will fail when the remote refuses `GetGame`.
|
||||
- `get_latest_version_for_game`: ignore `LocalOnly` entries when computing latest version. A version we can't actually transfer should not influence "is an update available?" decisions.
|
||||
|
||||
### State model implementation
|
||||
|
||||
- `local_games.rs` switches `downloaded` from `<id>.eti exists` to `version.ini exists` (exact `<game_root>/version.ini`, regular file).
|
||||
- `local_download_available` gates on `version.ini` + no active operation + catalog-known ID.
|
||||
- `build_game_summary` reads `eti_version` from `version.ini` whenever `downloaded`, not only when `installed`. The current "Game is not downloaded" error path is removed: a game with `local/` present but no `version.ini` returns a `GameSummary` with `downloaded = false`, `installed = true`, `availability = LocalOnly`.
|
||||
- `update_index_for_game` no longer short-circuits on missing `.eti`. The "no game here" decision is now: skip iff `version.ini` is absent **and** `local/` is not a directory **and** no `*.eti` is present at root level. Otherwise build a summary.
|
||||
- Manifest building (`scan_game_descriptions` / `should_skip_root_entry`) continues to skip `local/`; `local/` contents are user-owned and never enter manifests, fingerprints, or transfer paths.
|
||||
- Fingerprint still tracks `version.ini` mtime (already there); `.eti` mtime is retained as a cache invalidation hint. `local/` is-a-directory remains part of the fingerprint.
|
||||
|
||||
### `is_version_ini` predicate
|
||||
|
||||
`GameFileDescription::is_version_ini` is tightened to an exact root-level match: the predicate returns true only when `relative_path == "<game_id>/version.ini"`. The current `ends_with("/version.ini")` would misclassify nested `foo/bar/version.ini` as the sentinel; under the new sentinel rule that would break both download partitioning and the install state machine.
|
||||
|
||||
If a remote manifest contains zero or more than one root-level `version.ini` entry, `download_game_files` fails fast with a clear error — the sentinel is mandatory and must be unique.
|
||||
|
||||
## Scanner + watcher
|
||||
|
||||
### Workspace dependency
|
||||
|
||||
Add `notify = "7"` (or the current 7.x) to the top-level `Cargo.toml` `[workspace.dependencies]` and pull it into `lanspread-peer/Cargo.toml`. The crate is not currently in the dependency graph.
|
||||
|
||||
### Watcher topology
|
||||
|
||||
- `notify`-based, non-recursive watches on `game_dir` root and on each game root.
|
||||
- Never watch inside `local/`, `.local.installing/`, `.local.backup/`, or the root `.lanspread/`.
|
||||
- **Lifecycle**: the watcher reconciles its watched set whenever a game root appears or disappears, and tears down + rebuilds all watches on `SetGameDir`.
|
||||
|
||||
### No time-based debounce
|
||||
|
||||
Per-game-ID gating:
|
||||
|
||||
- Operation-lock present → drop event.
|
||||
- Rescan running for ID → set "rescan-pending" flag; the running task re-runs once on completion.
|
||||
- Else → spawn rescan for that ID.
|
||||
|
||||
### 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.
|
||||
|
||||
### Fallback scan
|
||||
|
||||
Every 300s, list top-level game-root dirs and run the optimized rescan per ID.
|
||||
|
||||
### Ignore list
|
||||
|
||||
In manifests, scans, and event handling: `.local.*`, `.version.ini.*`, `.lanspread`, `.lanspread.json`, `.sync`, `.softlan_game_installed` (latter two already skipped).
|
||||
|
||||
### Watcher init failure
|
||||
|
||||
Non-fatal. Log a warning; the fallback scan is sufficient on its own.
|
||||
|
||||
## Tauri / UI changes
|
||||
|
||||
### `src-tauri/src/lib.rs`
|
||||
|
||||
- Remove direct unpack logic: `unpack_game`, `do_unrar`. Remove the unpack-spawn inside `handle_download_finished` — unpack now happens inside the peer's install transaction. `handle_download_finished` becomes a plain `game-download-finished` event relay (no spawn, no sidecar invocation, no backup cleanup).
|
||||
- Remove the entire `___TO_BE_DELETE___{id}` backup-the-whole-game-dir scheme: `backup_game_folder`, `restore_game_folder`, `cleanup_backup_folder`, `cleanup_failed_download` (download rollback now lives in the peer). The whole-game-folder backup approach is incompatible with the new model (downloads no longer touch `local/`, so there's nothing to back up).
|
||||
- Remove `update_game_installation_state`'s "downloaded required for installed" coupling: `installed` is now `local/` is-a-directory, independent of `downloaded`.
|
||||
- Provide the `Unpacker` impl that shells out to the unrar sidecar; pass it to `start_peer`. Populate the catalog set from `load_bundled_game_db` and pass it to `start_peer` too.
|
||||
- Rename `games_in_download: HashSet<String>` to a generic in-progress operation set mirroring the peer's operation table (`HashMap<String, OpKind>` or similar — the variant tracks Downloading / Installing / Updating / Uninstalling so the UI can pick the right spinner).
|
||||
- `install_game(id)` is rewritten to route by local state rather than always sending `GetGame`:
|
||||
- `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 (already installed); the UI's "Open" affordance handles launch.
|
||||
- `update_game(id)` becomes a thin wrapper that sends `PeerCommand::GetGame { id }`. The fresh download triggers the peer's auto-install, which does a transactional update (rename `local → .local.backup`, extract, commit-rename). The old Tauri-side whole-folder backup is gone.
|
||||
|
||||
### New Tauri command
|
||||
|
||||
`uninstall_game(id)` → forwards to `PeerCommand::UninstallGame`.
|
||||
|
||||
### Frontend (`src/App.tsx`)
|
||||
|
||||
- New `InstallStatus.Installing` (extraction in flight) and `InstallStatus.Uninstalling`.
|
||||
- New install lifecycle events (peer-driven, separate from download and unpack):
|
||||
- `game-install-begin` — intent flipped to `Installing` / `Updating`, before extraction.
|
||||
- `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`).
|
||||
- `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.
|
||||
|
||||
## ARCHITECTURE.md
|
||||
|
||||
Update `crates/lanspread-peer/ARCHITECTURE.md`:
|
||||
|
||||
- Replace the "short debounce (1-2 seconds)" wording with the per-ID operation-lock + rescan-pending model.
|
||||
- Replace the fingerprint-cache description with the per-game intent log.
|
||||
- Document the `.local.installing` / `.local.backup` / `.lanspread.json` / `.lanspread_owned` layout and the ownership-marker rule.
|
||||
- Document the version.ini-as-sentinel rule and `.version.ini.*` reserved names.
|
||||
|
||||
## Test plan
|
||||
|
||||
### Unit — install transactions
|
||||
|
||||
- Install success, update success, update rollback on extract failure, update rollback on commit-rename failure, uninstall success, uninstall delete-failure restore.
|
||||
|
||||
### Unit — download transaction
|
||||
|
||||
- Fresh download writes `version.ini` last via atomic rename.
|
||||
- Re-download renames old `version.ini` to `.version.ini.discarded`, commits new one, sweeps discarded.
|
||||
- Download interrupted between phases 1 and 5 leaves no `version.ini`; subsequent scan sees game as not downloaded.
|
||||
- `prepare_game_storage` skips `version.ini` entries.
|
||||
- Version.ini chunk routing to memory: buffer accumulates correctly across out-of-order chunk arrivals.
|
||||
- Multi-`.eti` games: all archives downloaded, none of them used as the sentinel.
|
||||
|
||||
### Unit — recovery
|
||||
|
||||
- Each row of the install matrix, including the default-rule cases.
|
||||
- Download recovery: `.version.ini.tmp` and `.version.ini.discarded` swept unconditionally on startup.
|
||||
- JSON missing / corrupted / wrong schema_version → fall back to FS observation.
|
||||
- `.local.*` dir without `.lanspread_owned` marker → ignored, not deleted.
|
||||
|
||||
### Unit — atomic intent write
|
||||
|
||||
- Simulated crash between tmp-write and rename leaves the previous intent intact.
|
||||
- Schema version mismatch handled as "treat as missing."
|
||||
|
||||
### Unit — scanner gating
|
||||
|
||||
- Events for an ID under an operation lock are dropped.
|
||||
- Rescan-pending flag collapses bursts to ≤2 scans per ID.
|
||||
- Sideloaded dir is picked up by the fallback scan and ingested with correct state.
|
||||
- Non-catalog game root is observed by the scanner but produces no `GameSummary`, no library-index entry, and no `LibraryDelta` change.
|
||||
|
||||
### Unit — serve gating
|
||||
|
||||
- `GetGame` is refused when an operation is active for the ID.
|
||||
- `GetGameFileData` and `GetGameFileChunk` are refused mid-operation even when the requesting peer holds a pre-operation manifest.
|
||||
- All three handlers refuse non-catalog IDs.
|
||||
- All three handlers refuse when `version.ini` is absent (covers manually-deleted-sentinel case).
|
||||
- File-chunk handler rejects any `relative_path` that resolves under `local/`.
|
||||
|
||||
### Unit — aggregation
|
||||
|
||||
- `get_all_games` counts only `Ready` peers toward `peer_count`; a single `LocalOnly` peer reports `peer_count = 0` for a game no other peer carries.
|
||||
- `peers_with_game` and `peers_with_latest_version` exclude `LocalOnly` peers.
|
||||
- `get_latest_version_for_game` ignores `LocalOnly` entries.
|
||||
|
||||
### Unit — installed-only state
|
||||
|
||||
- A game root with `local/` present and `version.ini` absent produces a summary with `downloaded = false`, `installed = true`, `availability = LocalOnly`.
|
||||
- Same root, after the user copies in `version.ini`, flips to `Ready` on the next rescan.
|
||||
|
||||
### Manual
|
||||
|
||||
- New game appears via file-manager `mv` (atomic) and `cp` (piecemeal).
|
||||
- Game disappears via `rm -rf`.
|
||||
- Install / update / uninstall while watcher is active.
|
||||
- Failed unpack restores old install.
|
||||
- 100-game fallback scan stays fast.
|
||||
- Pull plug mid-download; verify on restart: game shown as not downloaded, no stale partials lingering.
|
||||
- Pull plug mid-update; verify install recovery restores old version.
|
||||
- Delete `version.ini` manually while game is installed; verify `LocalOnly` availability and that UI still shows installed.
|
||||
|
||||
### Verify
|
||||
|
||||
- `just fmt`
|
||||
- `just clippy`
|
||||
- `just build`
|
||||
|
||||
## Assumptions
|
||||
|
||||
- Uninstall removes only `local/`; the archives and `version.ini` stay so the game can be reinstalled without re-download.
|
||||
- All `.local.*`, `.version.ini.*`, and `.lanspread*` paths are reserved namespaces. Existing user dirs with `.local.*` names are not touched unless they carry the `.lanspread_owned` marker. `.version.ini.*` names are reserved by convention.
|
||||
- The peer process owns all install/update/uninstall and download FS mutations; users editing inside `local/` is explicitly unsupported.
|
||||
- A failed download is destructive to previous download state: old `version.ini` is not restored because `.eti` bytes may already be partially overwritten. `local/` is preserved (downloads never touch it). The user retries the download.
|
||||
- Watcher failure is non-fatal; the 5-minute fallback scan is sufficient on its own.
|
||||
- Sideload recommendation: users move complete game directories into `game_dir` (atomic rename), rather than copying file-by-file. For `cp`, the fallback scan reconciles once `version.ini` lands.
|
||||
- Non-catalog game roots are ignored because the app has no system yet to determine title and thumbnail for them. Supporting arbitrary non-ETI games is future scope.
|
||||
@@ -1,118 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,137 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,132 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,146 +0,0 @@
|
||||
# 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<String, OperationKind>`. 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<String, OperationKind>`. The Tauri shell keeps `active_operations: HashMap<String, UiOperationKind>`. 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<u64>` 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.
|
||||
Reference in New Issue
Block a user