fix(drive,docs): refuse an already-leased file before backing up - #1715
Merged
Merged
Conversation
drive lease acquire's only live-lease check lived inside insert_record, after the Touch ID prompt and the full backup - the normal outcome of any second acquire on an already-leased file, not just a rare race, and the orphaned backup was unreachable by drive lease prune (#1678), which only ever iterates ledger rows. Add a lock-free live_lease_for_file pre-check before authenticating at all, refusing the common case for free. Leave the ledger lock itself unheld across the prompt - it's a non-blocking, machine-wide lock, so holding it across a 120s human prompt would fail every other concurrent lease op - so insert_record's lock-held check stays the sole authoritative gate for the pre-check's narrow remaining race. Reclaim the backup this attempt took on every exit but Acquired, reusing prune's own clear_backup, as belt-and-braces for that race and for the other outcomes that can still follow a real backup: a post-backup metadata-fetch failure, a missing post-backup version, or insert_record itself losing to a concurrently held lock. Warn and audit a reclamation failure with a -backup-orphaned verdict suffix naming the surviving backup's location, rather than silently orphaning it. Fixes #1690.
… migration a_ledger_insert_failure_reclaims_the_backup_just_taken pre-created an empty <ledger>.lock file to simulate a busy lock, which stopped working once issue #1687 switched to flock(2): a bare file's existence holds nothing under flock, only an actual lock does, so insert_record succeeded instead of failing. Fixed by holding a real LedgerLock, matching restore.rs's own already-correct pattern. The same pre-existing-file pattern was used by six reports_a_lock_ acquisition_failure_as_failed tests across sheets/format, sheets/write, sheets/validation, sheets/protection, sheets/structure and docs/write. These route through check_and_lock_lease, which #1687 changed to *wait* for a busy lock rather than hard-fail, so simply holding a real LedgerLock here would make the tests block for the full lock-wait timeout instead of failing fast. Fixed by creating a directory at the lock path instead: an open() for write against it fails immediately with a genuine I/O error, which acquire_waiting never retries.
CoverageTotal: 97.27% ⚪ 0 pp vs Comparing
🔇 5 ignored region(s), 0 tolerated region(s)
Patch coveragePatch: 96.42% (323/335 new lines covered)
Uncovered new lines (12)
|
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 acquireno longer spends a Touch ID prompt and a real backup on a file that's already leased — a new lock-freelive_lease_for_filepre-check runs before authenticating at all.Acquired, instead of being silently orphaned when a later step fails.Test plan
cargo buildcargo test --lib drive::(1458 passed)cargo clippy --all-targets -- -D warnings/code-review high. Fixed two real test regressions the review surfaced (both pre-existing from refactor(drive): lease ledger lock is ledger-global, non-waiting, Drop-unlinks-by-path; prune deletes backup before locking #1687, not this PR's own bug): a test simulating a busy ledger lock via a pre-created empty file, which stopped working once refactor(drive): lease ledger lock is ledger-global, non-waiting, Drop-unlinks-by-path; prune deletes backup before locking #1687 switched toflock(2)(fixed by holding a realLedgerLock), and sixreports_a_lock_acquisition_failure_as_failedtests across the sheets/docs write engines that hit the same issue but additionally route throughcheck_and_lock_lease's new wait-not-hard-fail behavior, so a held lock there would have made them block for the full timeout instead of failing fast (fixed by simulating a genuine I/O error — a directory at the lock path — instead).acquire_waiting_with_timeout's poll loop isn't wrapped inblock_in_place) — out of scope for this PR.