fix(drive,docs): refuse a repeat restore of an already-restored sheet - #1716
Merged
Merged
Conversation
Re-running `drive lease restore` on a token whose deleted-sheet restore had already succeeded silently copied the sheet in again, one "Copy of ..." per run, each costing a Touch ID prompt and a fresh Drive backup copy. `spreadsheets.sheets.copyTo` assigns the destination a fresh sheetId, so the backup sheet's own id stayed missing from the live spreadsheet and the structural detection ADR-0080 §10 relies on kept firing; the pre-write recheck compared against that same original id rather than the one the previous restore created, so it passed too. Live spreadsheet state alone cannot settle it. A restored sheet is structurally indistinguishable from a live sheet that merely shares the backup sheet's title -- an id absent from the backup, a matching title -- and §10 deliberately restores *through* the latter rather than refusing it. So the ledger row now records `restored_sheet_id`, the live id each restore creates, and a repeat run is refused before the authentication prompt and before the fresh backup copy, naming the sheet and the still-live lease the earlier restore minted. The guard keys on that id still being live rather than on `restored_at` being set, so re-deleting the restored sheet and re-running legitimately restores it again. The recheck immediately before the write applies the same test, for a restore landing during the up-to-two-minute prompt window. The Bytes restore path was already idempotent and is unchanged. `restored_sheet_id` is additive, so ledgers written by earlier builds keep loading. Boxes InsertOutcome::AlreadyLeased's payload, which the new field pushed over clippy's large_enum_variant threshold. Also corrects five docs/drive.md examples that showed lease expiry timestamps as RFC3339 when the code prints chrono's Display form, and three verdicts missing from docs/log.md's drive-lease-restore vocabulary. Closes #1689
…1689 Intra-doc links still pointed at detect_deleted_sheet after its rename to detect_sheet_restore, breaking `cargo doc --document-private-items` (the CI Docs job's exact command). SheetAlreadyRestored's "that restore's own lease is still live" message claimed a specific provenance that live_lease_for can't actually guarantee — it returns any live lease for the file, which could have been minted by an unrelated operation after the original lease expired; reworded to not over-claim. Also avoids rebuilding live.sheet_ids() twice in the pre-write recheck.
The rebase onto main (which now carries #1690's InsertsALiveLeaseDuringAuth test helper) merged cleanly with no conflict markers, but silently left a LeaseRecord struct literal missing the restored_sheet_id field this branch added — a real compile error in test code only, since a non-test `cargo build` never catches it.
CoverageTotal: 97.27% ⚪ 0 pp vs Comparing
🔇 5 ignored region(s), 0 tolerated region(s)
Patch coveragePatch: 98.82% (417/422 new lines covered)
Uncovered new lines (5)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
drive lease restoreno longer duplicates a sheet it has already restored — the ledger row now recordsrestored_sheet_id, and a repeat run is refused before the authentication prompt and the fresh backup copy.Test plan
cargo buildcargo test --lib drive::lease::(175 passed),cargo test --lib drive::(1464 passed)cargo clippy --all-targets -- -D warnings/code-review high. Fixed the findings: broken intra-doc links to a renamed function (would have failed CI's Docs job), an over-claiming CLI message ("that restore's own lease" when the ledger can't actually guarantee which operation minted a live lease for the file — reworded), and a redundantHashSetrebuild in the pre-write recheck.main(carrying fix(drive): lease acquire spends the prompt and backup before the live-lease check; orphaned backups unreachable by prune #1690), a test'sLeaseRecordstruct literal was missing this branch's newrestored_sheet_idfield — no textual conflict markers, but a real compile error in test code thatcargo buildalone doesn't catch.