ddidderr
7e97d6a83a
While reviewing the cancel-cleanup fix (`fix(peer): delete partial files
when a download is cancelled`), it became clear that the new
`discard_cancelled_download` sweep only runs from inside the in-flight
orchestrator. If the process dies mid-download (kill, crash, power loss)
the partial `.eti` archives are left behind: `install::recover_on_startup`
calls `recover_download_transients`, which only removes
`.version.ini.tmp` and `.version.ini.discarded`. The user is left with
a card that looks "downloaded" but with corrupted archives that can only
be cleared via the explicit `Remove files` action.
Closing the gap would mean running the same discard pass during recovery
for any game root whose install intent is `None` and whose `version.ini`
is absent (intent log already distinguishes installed-and-then-broken
from interrupted-download). Not blocking — the user-initiated cancel
button is now correct in its scope; this is the symmetric crash recovery
case captured for a future cleanup pass.
Refs: c380046 (fix(peer): delete partial files when a download is cancelled)
54 lines
2.6 KiB
Markdown
54 lines
2.6 KiB
Markdown
# Findings
|
|
|
|
## Open
|
|
|
|
### Crash-during-download leaves orphan archive files
|
|
|
|
`crates/lanspread-peer/src/install/transaction.rs:329` — `recover_download_transients`
|
|
sweeps only `.version.ini.tmp` and `.version.ini.discarded` on startup. The new
|
|
cancel-cleanup (`download/storage.rs::discard_cancelled_download`) is only invoked
|
|
from the in-flight orchestrator, so a crash mid-download leaves partial `.eti`
|
|
archives in the game root. After restart the user sees a game that looks
|
|
half-downloaded with no way to clean it up except `RemoveDownloadedGame`. Closing
|
|
this would mean calling the same discard pass during recovery for any game root
|
|
whose intent is `None` and whose `version.ini` is absent.
|
|
|
|
Not blocking. The cancel-button fix is correct in its scope; this is the symmetric
|
|
crash-recovery case.
|
|
|
|
### `handleErrorEvent` still writes status fields directly
|
|
|
|
`crates/lanspread-tauri-deno-ts/src/hooks/useGames.ts:80-89` — the error
|
|
handler writes `install_status`, `status_message`, `status_level`, and
|
|
`download_progress` from a lifecycle event, which is the same "two sources of
|
|
truth" pattern that commit `5df82aa` ("fix(ui): derive operation status from
|
|
snapshots") removed everywhere else. That commit explicitly carved out error
|
|
messages as a preserved side effect, so this is a documented exception rather
|
|
than a regression — but if we want strict snapshot-is-truth, the error handler
|
|
should stop writing status fields and let the next snapshot reconcile the card,
|
|
keeping only the error message overlay (which the snapshot does not carry).
|
|
|
|
Not blocking. Captured here for a future cleanup pass.
|
|
|
|
## Claude Review Scope Triage
|
|
|
|
No out-of-scope code smells or issues were identified in Claude's review. All
|
|
four points were direct follow-up cleanup for the current protocol change and
|
|
were handled in code.
|
|
|
|
The previous three findings have landed in code and tests:
|
|
|
|
- `update_game` now uses `PeerCommand::FetchLatestFromPeers` to skip local
|
|
manifest serving and fetch fresh peer metadata. Covered by
|
|
`update_fetch_emits_fresh_manifest_from_latest_peer` and
|
|
`update_request_skips_local_manifest_even_when_download_exists`.
|
|
- Download-to-install handoff no longer relies on `OperationGuard::Drop` for
|
|
ordered state transitions. Covered by
|
|
`download_handoff_waits_for_readers_and_auto_installs` and the liveness
|
|
cancellation tests.
|
|
- Library index reads and writes are serialized by `LIBRARY_INDEX_LOCK`.
|
|
Covered by `concurrent_rescans_preserve_both_index_updates`.
|
|
|
|
Manual install/update/uninstall smoke testing is still a useful release check,
|
|
but there are no known blocking findings left in this file.
|