From d9c7b60714ad657606c5f1cc8e002ea7310803d6 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 19:16:24 +1000 Subject: [PATCH 1/4] feat(drive,cli): add drive lease prune command (#1678) Bounds the lease ledger's and the backup directory/folder's unbounded growth, the ADR-0080 Consequences fast-follow named at #1664's close. Mirrors `omni-dev log prune`'s shape (--older-than/--max-size/--dry-run) but a live lease is never a candidate, and a ledger row is dropped only together with the backup it points at: a Bytes backup is deleted from disk, a DriveCopy backup is moved to Drive Trash via a new FilesApi::trash (the integration's first delete-adjacent capability, deliberately reversible rather than a permanent files.delete). --- src/cli/drive/lease.rs | 191 ++++- src/cli/log.rs | 4 + src/drive/files_api.rs | 107 +++ src/drive/lease.rs | 5 +- src/drive/lease/ledger.rs | 58 +- src/drive/lease/prune.rs | 688 ++++++++++++++++++ .../integration_test__help_all_output.snap | 17 + 7 files changed, 1061 insertions(+), 9 deletions(-) create mode 100644 src/drive/lease/prune.rs diff --git a/src/cli/drive/lease.rs b/src/cli/drive/lease.rs index 1b74f0a4..91b417f6 100644 --- a/src/cli/drive/lease.rs +++ b/src/cli/drive/lease.rs @@ -12,6 +12,7 @@ use crate::drive::lease::acquire::{ }; use crate::drive::lease::authenticate::{self, AuthPolicy}; use crate::drive::lease::ledger::{self, LeaseBackup}; +use crate::drive::lease::prune::{self, PruneOptions}; use crate::drive::lease::restore::{self, RestoreOptions, RestoreResult}; use crate::drive::lease::settings as lease_settings; use crate::drive::sheets::client::SheetsClient; @@ -53,6 +54,10 @@ enum LeaseAction { /// Restores a file from a backup lease's recorded content, minting a /// fresh lease of its own before writing. Restore(RestoreCommand), + /// Bounds the ledger's and the backup directory/folder's growth by + /// dropping expired rows together with the backups they point at + /// (ADR-0080 Consequences fast-follow, #1678). + Prune(PruneCommand), } impl LeaseCommand { @@ -60,6 +65,7 @@ impl LeaseCommand { match self.action { LeaseAction::Acquire(cmd) => cmd.execute(client).await, LeaseAction::Restore(cmd) => cmd.execute(client).await, + LeaseAction::Prune(cmd) => cmd.execute(client).await, } } } @@ -228,6 +234,89 @@ impl RestoreCommand { } } +/// Bounds the lease ledger's and the backup directory/folder's growth +/// (ADR-0080 Consequences fast-follow, #1678). Mirrors `omni-dev log +/// prune`'s shape: `--older-than`/`--max-size`, at least one required, +/// applied sequentially (age first, then size trims what's left), +/// `--dry-run` reports without mutating anything. Unlike `log prune`, there +/// is no `--audit` flag to refuse — this command's underlying artifacts +/// (the ledger, the backup directory/folder) never include `audit.jsonl` in +/// the first place. +/// +/// No [`LeaseFlags`] here: prune touches no per-lease `backup_dir`/ +/// `expiry_minutes`/Touch-ID policy — every row already carries its own +/// absolute backup path or Drive file id, and pruning never mints a new +/// lease. +#[derive(Parser)] +pub struct PruneCommand { + /// Drop non-live rows whose expiry is older than this relative window + /// (e.g. `7d`, `24h`, `2w`). + #[arg(long, value_name = "DUR")] + older_than: Option, + /// After `--older-than`, additionally drop the oldest-expiring + /// survivors until their local backup bytes total at most this size + /// (e.g. `10mb`, `512kb`, `1048576`). A `DriveCopy` backup counts as + /// zero bytes here — it consumes no local disk. + #[arg(long, value_name = "SIZE")] + max_size: Option, + /// Report what would be removed without deleting/trashing any backup + /// or modifying the ledger. + #[arg(long)] + dry_run: bool, + /// Output format. + #[arg(short = 'o', long, value_enum, default_value_t = OutputFormat::Table)] + pub output: OutputFormat, +} + +impl PruneCommand { + pub async fn execute(self, client: &DriveClient) -> Result<()> { + if self.older_than.is_none() && self.max_size.is_none() { + anyhow::bail!("nothing to prune: pass --older-than and/or --max-size "); + } + let older_than = match self.older_than.as_deref() { + Some(s) => Some( + crate::cli::log::parse_since(s) + .map_err(|e| anyhow::anyhow!("invalid --older-than: {e}"))?, + ), + None => None, + }; + let max_size = match self.max_size.as_deref() { + Some(s) => Some( + crate::request_log::parse_size(s) + .map_err(|e| anyhow::anyhow!("invalid --max-size: {e}"))?, + ), + None => None, + }; + + let ledger_path = ledger::ledger_path()?; + let opts = PruneOptions { + older_than, + max_size, + dry_run: self.dry_run, + ledger_path, + }; + let outcome = prune::prune(client, &opts).await?; + if output_as(&outcome, &self.output)? { + return Ok(()); + } + let verb = if self.dry_run { + "Would remove" + } else { + "Removed" + }; + println!( + "{verb} {} lease(s); kept {} ({} trashed Drive backup(s), {} failure(s), freed \ + {} bytes of local backups).", + outcome.removed, + outcome.kept, + outcome.trashed_drive_copies, + outcome.failed, + outcome.bytes_freed, + ); + Ok(()) + } +} + fn print_result(result: &AcquireResult) { match result { AcquireResult::Acquired { @@ -791,8 +880,23 @@ mod tests { Wrapped::Lease(cmd) => match cmd.action { LeaseAction::Acquire(acquire) => acquire, // omni-dev: coverage ignore reason="guards this test helper against misuse; every call site below passes an acquire subcommand" - LeaseAction::Restore(_) => panic!("expected an Acquire command"), - // omni-dev: coverage end + LeaseAction::Restore(_) | LeaseAction::Prune(_) => { + panic!("expected an Acquire command") + } // omni-dev: coverage end + }, + } + } + + fn parse_prune(args: &[&str]) -> PruneCommand { + let mut full = vec!["omni-dev", "lease"]; + full.extend_from_slice(args); + match Wrapper::try_parse_from(full).unwrap().cmd { + Wrapped::Lease(cmd) => match cmd.action { + LeaseAction::Prune(prune) => prune, + // omni-dev: coverage ignore reason="guards this test helper against misuse; every call site below passes a prune subcommand" + LeaseAction::Acquire(_) | LeaseAction::Restore(_) => { + panic!("expected a Prune command") + } // omni-dev: coverage end }, } } @@ -897,4 +1001,87 @@ mod tests { assert!(cmd.flags.biometrics_only); assert!(cmd.flags.allow_headless); } + + // ── prune ──────────────────────────────────────────────────────── + + #[test] + fn prune_flags_parse() { + let cmd = parse_prune(&[ + "prune", + "--older-than", + "7d", + "--max-size", + "10mb", + "--dry-run", + ]); + assert_eq!(cmd.older_than.as_deref(), Some("7d")); + assert_eq!(cmd.max_size.as_deref(), Some("10mb")); + assert!(cmd.dry_run); + } + + #[tokio::test] + async fn prune_requires_at_least_one_bound() { + let guard = crate::drive::test_support::EnvGuard::take(); + let dir = guard.clear_credentials(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let cmd = parse_prune(&["prune"]); + let err = cmd.execute(&client).await.unwrap_err(); + assert!(err.to_string().contains("nothing to prune"), "{err}"); + } + + #[tokio::test] + async fn prune_rejects_an_invalid_older_than() { + let guard = crate::drive::test_support::EnvGuard::take(); + let dir = guard.clear_credentials(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let cmd = parse_prune(&["prune", "--older-than", "not-a-duration"]); + let err = cmd.execute(&client).await.unwrap_err(); + assert!(err.to_string().contains("invalid --older-than"), "{err}"); + } + + #[tokio::test] + async fn prune_command_removes_an_expired_lease_end_to_end() { + let guard = crate::drive::test_support::EnvGuard::take(); + let dir = guard.clear_credentials(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + + let ledger_path = crate::drive::lease::ledger::ledger_path().unwrap(); + let backup_path = dir.path().join("old-backup.bin"); + std::fs::write(&backup_path, b"stale").unwrap(); + let mut ledger = crate::drive::lease::ledger::LeaseLedger::default(); + ledger.insert(crate::drive::lease::ledger::LeaseRecord { + token: "old-token".to_string(), + file_id: "file-1".to_string(), + version: "1".to_string(), + modified_time: None, + backup: LeaseBackup::Bytes { + path: backup_path.clone(), + sha256: "deadbeef".to_string(), + size: 5, + }, + acquired_at: chrono::Utc::now() - chrono::Duration::days(10), + expires_at: chrono::Utc::now() - chrono::Duration::days(9), + released_at: None, + restored_at: None, + }); + ledger.save(&ledger_path).unwrap(); + + let cmd = PruneCommand { + older_than: Some("1d".to_string()), + max_size: None, + dry_run: false, + output: OutputFormat::Json, + }; + cmd.execute(&client).await.unwrap(); + + assert!(!backup_path.exists()); + let reloaded = crate::drive::lease::ledger::LeaseLedger::load(&ledger_path).unwrap(); + assert!(reloaded.get("old-token").is_none()); + } } diff --git a/src/cli/log.rs b/src/cli/log.rs index bae69a4d..0b3c0aca 100644 --- a/src/cli/log.rs +++ b/src/cli/log.rs @@ -16,6 +16,10 @@ use clap::{Parser, Subcommand, ValueEnum}; use crate::request_log; use query::Filter; +/// Shared relative-duration parser, also reused by `drive lease prune`'s own +/// `--older-than` (#1678) so both prune commands accept the identical +/// `30m`/`2h`/`1d`/`1w` syntax. +pub(crate) use query::parse_since; /// Shared `--since`/`--until` parser (relative durations, dates, or RFC3339), /// reused by the `count` subcommand so its time bounds match `omni-dev log`. pub(crate) use query::parse_time_bound; diff --git a/src/drive/files_api.rs b/src/drive/files_api.rs index 718d35d5..40334881 100644 --- a/src/drive/files_api.rs +++ b/src/drive/files_api.rs @@ -214,6 +214,34 @@ impl<'a> FilesApi<'a> { .map_err(|err| append_write_scope_hint(err, WriteCapability::Metadata)) } + /// Moves a file to Drive Trash (`files.update` with `trashed: true`) — + /// the `drive lease prune` (#1678) mechanism for dropping a + /// [`LeaseBackup::DriveCopy`](crate::drive::lease::ledger::LeaseBackup::DriveCopy) + /// backup together with the ledger row that points at it. Deliberately + /// trash, not permanent delete: this is the integration's first + /// delete-adjacent capability, and trashing keeps it reversible (Drive + /// Trash, auto-purged after ~30 days, or recoverable by hand before + /// then) rather than adding the one irreversible Drive mutation this + /// codebase has otherwise never needed. Requires the `drive.metadata` + /// scope (`drive auth login --write`) — same as [`Self::rename`], since + /// a backup copy is always a file `omni-dev` itself created via + /// [`Self::copy`]. + /// + /// Restricted to `crate::drive` — only `crate::drive::lease::prune` may + /// call this, never an ungated CLI command directly (same restriction + /// [`Self::copy`]/[`Self::create`] carry). + pub(in crate::drive) async fn trash(&self, file_id: &str) -> Result { + let url = build_file_update_url(self.client.base_url(), file_id, None, None)?; + let response = self + .client + .patch_json(url.as_str(), &serde_json::json!({ "trashed": true })) + .await?; + self.client + .parse_response(response, "Failed to parse files.update (trash) response") + .await + .map_err(|err| append_write_scope_hint(err, WriteCapability::Trash)) + } + /// Creates a new file or folder (`files.create`, metadata-only — no /// content). Requires the `drive.file` or `drive` scope (`drive auth /// login --write-file`/`--write-full`). @@ -578,6 +606,10 @@ pub(crate) enum WriteCapability { /// no app-created-it case to consider, since the lease exists /// precisely to back up files `omni-dev` did not create. CopyForBackup, + /// Trashing a `DriveCopy` lease backup during `drive lease prune` + /// (#1678) — `drive.metadata`, the same scope [`Self::Metadata`] names, + /// but worded for trashing rather than rename/move. + Trash, } /// Appends an actionable hint to a mutating-call failure caused by an @@ -623,6 +655,10 @@ pub(in crate::drive) fn append_write_scope_hint( "Run `omni-dev drive auth login --write-full` to grant the unrestricted scope \ needed to back up a native document before a leased write, then retry" } + WriteCapability::Trash => { + "Run `omni-dev drive auth login --write` to grant the drive.metadata scope needed \ + to trash a lease backup, then retry" + } }; err.context(hint) } @@ -1295,6 +1331,68 @@ mod tests { ); } + // ── trash ──────────────────────────────────────────────────────── + + #[tokio::test] + async fn trash_sends_a_trashed_true_body() { + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/f1")) + .and(wiremock::matchers::body_json( + serde_json::json!({"trashed": true}), + )) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "f1", "name": "backup", "trashed": true, + })), + ) + .expect(1) + .mount(&server) + .await; + + let file = FilesApi::new(&client).trash("f1").await.unwrap(); + assert_eq!(file.id, "f1"); + } + + #[tokio::test] + async fn trash_propagates_api_errors() { + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/f1")) + .respond_with(wiremock::ResponseTemplate::new(404).set_body_string("not found")) + .mount(&server) + .await; + + let err = FilesApi::new(&client).trash("f1").await.unwrap_err(); + assert!(err.to_string().contains("404")); + } + + #[tokio::test] + async fn trash_appends_write_scope_hint_on_insufficient_permissions() { + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/f1")) + .respond_with( + wiremock::ResponseTemplate::new(403).set_body_json(serde_json::json!({ + "error": { + "message": "Insufficient Permission", + "errors": [{"reason": "insufficientPermissions"}], + } + })), + ) + .mount(&server) + .await; + + let err = FilesApi::new(&client).trash("f1").await.unwrap_err(); + assert!( + err.to_string().contains("drive auth login --write"), + "{err}" + ); + } + // ── create ────────────────────────────────────────────────────── #[tokio::test] @@ -1679,6 +1777,15 @@ mod tests { assert!(!msg.contains("--write-file"), "{msg}"); } + #[test] + fn append_write_scope_hint_trash_names_write_flag() { + let msg = append_write_scope_hint(insufficient_permissions_error(), WriteCapability::Trash) + .to_string(); + assert!(msg.contains("--write"), "{msg}"); + assert!(msg.contains("trash a lease backup"), "{msg}"); + assert!(!msg.contains("--write-file"), "{msg}"); + } + // ── check_download_size ───────────────────────────────────────── #[test] diff --git a/src/drive/lease.rs b/src/drive/lease.rs index c1a1cb90..446e3dd3 100644 --- a/src/drive/lease.rs +++ b/src/drive/lease.rs @@ -9,11 +9,14 @@ //! content-mutating engine (`drive edit`, every Sheets/Docs write verb) //! calls; [`restore`] is `drive lease restore` (ADR-0080 §10); [`settings`] //! resolves the global CLI-flag/env-var/`settings.json` policy layer -//! (ADR-0080 §13, issue #1677). +//! (ADR-0080 §13, issue #1677); [`prune`] is `drive lease prune`, the +//! ADR-0080 Consequences fast-follow (#1678) that bounds the ledger's and +//! the backup directory/folder's unbounded growth. pub(crate) mod acquire; pub(crate) mod authenticate; pub(crate) mod check; pub(crate) mod ledger; +pub(crate) mod prune; pub(crate) mod restore; pub(crate) mod settings; diff --git a/src/drive/lease/ledger.rs b/src/drive/lease/ledger.rs index d8c42531..fe7174d4 100644 --- a/src/drive/lease/ledger.rs +++ b/src/drive/lease/ledger.rs @@ -10,12 +10,13 @@ //! (`src/cli/gmail/insert/ledger.rs`): an in-memory `BTreeMap` is the //! source of truth, and every state change atomically rewrites the whole //! file (`tempfile::NamedTempFile` + `persist`), never appends an event. -//! Unlike `InsertLedger`, a row is **never dropped** on rewrite once -//! expired or released — `drive lease restore` (a later phase) locates a -//! backup by token, and a restore is almost always wanted *after* the -//! expiry window, once a bad write has been noticed. A `lease prune` that -//! drops a row together with the backup it points at is the deliberate -//! fast-follow ADR-0080's Consequences name, not implemented here. +//! Unlike `InsertLedger`, a row is not dropped on an ordinary rewrite once +//! expired or released — `drive lease restore` locates a backup by token, +//! and a restore is almost always wanted *after* the expiry window, once a +//! bad write has been noticed. [`super::prune`] is the one caller that does +//! drop rows, deliberately: it is the ADR-0080 Consequences fast-follow +//! (#1678) that drops a row together with the backup it points at, never +//! one without the other. use std::collections::BTreeMap; use std::io::Write as _; @@ -167,6 +168,25 @@ impl LeaseLedger { self.0.insert(record.token.clone(), record); } + /// Every row currently in the ledger, in token order. Read-only — + /// [`super::prune`] uses this to decide what's eligible, never to + /// mutate a row directly (a row is only ever dropped wholesale, via + /// [`Self::remove`]). + pub(crate) fn iter(&self) -> impl Iterator { + self.0.values() + } + + /// Drops `token`'s row entirely — unlike every other mutator here, this + /// removes state rather than updating it. The sole caller is + /// [`super::prune`], and only ever after that row's backup has already + /// been deleted/trashed: a ledger row and the backup it points at must + /// never be dropped one without the other (ADR-0080 Consequences, + /// #1678). Returns the removed row, if any, so a caller can account for + /// what it freed. + pub(crate) fn remove(&mut self, token: &str) -> Option { + self.0.remove(token) + } + /// The live (unexpired, unreleased) lease already covering `file_id`, /// if any. /// @@ -469,6 +489,32 @@ mod tests { assert!(ledger.get("missing").is_none()); } + #[test] + fn iter_yields_every_row() { + let mut ledger = LeaseLedger::default(); + ledger.insert(sample_record("t1")); + ledger.insert(sample_record("t2")); + let tokens: Vec<&str> = ledger.iter().map(|r| r.token.as_str()).collect(); + assert_eq!(tokens.len(), 2); + assert!(tokens.contains(&"t1")); + assert!(tokens.contains(&"t2")); + } + + #[test] + fn remove_drops_a_present_row_and_returns_it() { + let mut ledger = LeaseLedger::default(); + ledger.insert(sample_record("t1")); + let removed = ledger.remove("t1").unwrap(); + assert_eq!(removed.token, "t1"); + assert!(ledger.get("t1").is_none()); + } + + #[test] + fn remove_on_an_absent_token_is_a_noop() { + let mut ledger = LeaseLedger::default(); + assert!(ledger.remove("missing").is_none()); + } + #[test] fn expired_row_is_kept_on_save_not_dropped() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/drive/lease/prune.rs b/src/drive/lease/prune.rs new file mode 100644 index 00000000..c85b70d8 --- /dev/null +++ b/src/drive/lease/prune.rs @@ -0,0 +1,688 @@ +//! `drive lease prune` — the ADR-0080 Consequences fast-follow (#1678) that +//! bounds the ledger's and the backup directory/folder's otherwise-unbounded +//! growth. Modeled on `omni-dev log prune` (`request_log::prune`): +//! `--older-than`/`--max-size`, applied sequentially (age first, then size +//! trims what's left), `--dry-run` reports without mutating anything. +//! +//! One invariant is non-negotiable and enforced structurally, not by a +//! flag: a live lease ([`LeaseRecord::is_live`]) is never a removal +//! candidate, regardless of `--older-than`/`--max-size`. The other is the +//! ADR's own: a ledger row and the backup it points at are dropped +//! **together, never one without the other** — a +//! [`LeaseBackup::Bytes`] backup is deleted from local disk, a +//! [`LeaseBackup::DriveCopy`] backup is moved to Drive Trash +//! ([`FilesApi::trash`]), and only once that step succeeds (or finds the +//! backup already gone) does the row itself get dropped from the ledger. +//! A backup-deletion failure for one row skips just that row — it is left +//! for a future prune, not treated as a hard error for the whole command. + +use std::path::PathBuf; + +use anyhow::Result; +use chrono::{DateTime, Utc}; +use serde::Serialize; + +use crate::cli::drive::format::JsonlSerialize; +use crate::drive::client::DriveClient; +use crate::drive::error::DriveError; +use crate::drive::files_api::FilesApi; +use crate::drive::lease::ledger::{LeaseBackup, LeaseLedger, LeaseRecord, LedgerLock}; + +/// Options controlling [`prune`]. +pub struct PruneOptions { + /// Drop non-live rows whose `expires_at` is at or before this cutoff. A + /// live row is never a candidate regardless of this bound. + pub older_than: Option>, + /// After the age filter, additionally drop the oldest-expiring + /// survivors until the local backup bytes they account for total at + /// most this many bytes. A `DriveCopy` backup counts as zero bytes here + /// (it consumes no local disk) but remains eligible via `older_than`. + pub max_size: Option, + /// Compute and report the outcome without deleting/trashing any backup + /// or modifying the ledger. + pub dry_run: bool, + /// Path to the lease ledger. Production callers pass + /// [`crate::drive::lease::ledger::ledger_path`]'s own result; tests + /// pass a path under a `tempdir`. + pub ledger_path: PathBuf, +} + +/// What a [`prune`] run did (or, when `dry_run`, would do). +#[derive(Debug, Clone, Default, Serialize)] +pub struct PruneOutcome { + /// Rows (and their backups) removed. + pub removed: usize, + /// Rows remaining in the ledger afterward (live and non-live alike). + pub kept: usize, + /// Bytes freed from local disk by removed `Bytes` backups. + pub bytes_freed: u64, + /// `DriveCopy` backups moved to Drive Trash. + pub trashed_drive_copies: usize, + /// Removal candidates skipped this run because deleting/trashing their + /// backup failed — left in the ledger for a future prune. + pub failed: usize, +} + +impl JsonlSerialize for PruneOutcome { + fn write_jsonl(&self, out: &mut dyn std::io::Write) -> Result<()> { + crate::cli::drive::format::write_scalar_jsonl(self, out) + } +} + +/// A row's local backup byte footprint — `0` for a `DriveCopy`, which +/// consumes no local disk. +fn backup_size(backup: &LeaseBackup) -> u64 { + match backup { + LeaseBackup::Bytes { size, .. } => *size, + LeaseBackup::DriveCopy { .. } => 0, + } +} + +/// Whether `err` is Drive's 404 for an already-absent file — tolerated as +/// "already clean" rather than a failure, since a prior manual cleanup or a +/// previous partially-failed prune run could have already trashed it. +fn is_drive_not_found(err: &anyhow::Error) -> bool { + matches!( + err.downcast_ref::(), + Some(DriveError::ApiRequestFailed { status: 404, .. }) + ) +} + +/// Deletes/trashes one candidate's backup. `Ok(())` means the row's backup +/// is gone (deleted just now, or already absent) and the row may be +/// dropped; an `Err` means it is left in place for a future prune. +async fn clear_backup(files_api: &FilesApi<'_>, backup: &LeaseBackup) -> Result<()> { + match backup { + LeaseBackup::Bytes { path, .. } => match std::fs::remove_file(path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e.into()), + }, + LeaseBackup::DriveCopy { file_id } => match files_api.trash(file_id).await { + Ok(_) => Ok(()), + Err(e) if is_drive_not_found(&e) => Ok(()), + Err(e) => Err(e), + }, + } +} + +/// Longest prefix of `sorted_desc` (sorted newest-`expires_at`-first) whose +/// backup bytes fit within `max` — but never fewer than the single +/// most-recently-expired candidate, even if it alone exceeds the budget. +/// Mirrors `request_log::keep_by_size`, adapted from reverse iteration over +/// chronological lines to forward iteration over an already-descending +/// slice. +fn keep_count_by_size(sorted_desc: &[&LeaseRecord], max: u64) -> usize { + let mut acc = 0u64; + let mut keep = 0usize; + for rec in sorted_desc { + acc += backup_size(&rec.backup); + if acc > max { + break; + } + keep += 1; + } + if keep == 0 && !sorted_desc.is_empty() { + keep = 1; + } + keep +} + +/// Prunes the lease ledger at `opts.ledger_path` by age and/or size, +/// dropping each removed row together with the backup it points at. +/// +/// Runs under [`LedgerLock`] for its whole duration (load through save), so +/// a concurrent `drive lease acquire`/`restore` can't race the rewrite — +/// the same lock those commands themselves hold while mutating the ledger. +pub async fn prune(client: &DriveClient, opts: &PruneOptions) -> Result { + let _lock = LedgerLock::acquire(&opts.ledger_path)?; + let mut ledger = LeaseLedger::load(&opts.ledger_path)?; + let now = Utc::now(); + + let (live_count, non_live): (usize, Vec) = { + let mut live_count = 0usize; + let mut non_live = Vec::new(); + for rec in ledger.iter() { + if rec.is_live(now) { + live_count += 1; + } else { + non_live.push(rec.clone()); + } + } + (live_count, non_live) + }; + + // Age filter: a non-live row survives into the size filter unless it's + // past the `--older-than` cutoff (no cutoff means every non-live row + // proceeds to the size filter, mirroring `request_log::prune` with + // `older_than: None`). + let (mut age_survivors, mut removed): (Vec, Vec) = + non_live.into_iter().partition(|rec| match opts.older_than { + Some(cutoff) => rec.expires_at > cutoff, + None => true, + }); + + // Size filter, applied only to the age survivors: keep the + // most-recently-expired ones whose backups fit `max_size`, moving the + // rest into `removed` too. + if let Some(max) = opts.max_size { + age_survivors.sort_by_key(|rec| std::cmp::Reverse(rec.expires_at)); + let refs: Vec<&LeaseRecord> = age_survivors.iter().collect(); + let keep = keep_count_by_size(&refs, max); + removed.extend(age_survivors.split_off(keep)); + } + + let files_api = FilesApi::new(client); + let mut outcome = PruneOutcome { + kept: live_count + age_survivors.len(), + ..Default::default() + }; + + for rec in &removed { + if opts.dry_run { + outcome.removed += 1; + match &rec.backup { + LeaseBackup::Bytes { size, .. } => outcome.bytes_freed += size, + LeaseBackup::DriveCopy { .. } => outcome.trashed_drive_copies += 1, + } + continue; + } + if clear_backup(&files_api, &rec.backup).await.is_ok() { + ledger.remove(&rec.token); + outcome.removed += 1; + match &rec.backup { + LeaseBackup::Bytes { size, .. } => outcome.bytes_freed += size, + LeaseBackup::DriveCopy { .. } => outcome.trashed_drive_copies += 1, + } + } else { + outcome.failed += 1; + outcome.kept += 1; + } + } + + if !opts.dry_run && outcome.removed > 0 { + ledger.save(&opts.ledger_path)?; + } + + Ok(outcome) +} + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] +mod tests { + use super::*; + use crate::drive::auth::{DriveCredentials, DriveGrantedScopes}; + use crate::utils::secret::Secret; + use chrono::Duration as ChronoDuration; + + fn test_credentials() -> DriveCredentials { + DriveCredentials { + client_id: "client-1".to_string(), + client_secret: Secret::new("secret-1"), + refresh_token: Secret::new("refresh-1"), + scope: DriveGrantedScopes::READONLY, + } + } + + async fn client_with_bootstrapped_token(server: &wiremock::MockServer) -> DriveClient { + wiremock::Mock::given(wiremock::matchers::method("POST")) + .and(wiremock::matchers::path("/token")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "access_token": "test-token", + "expires_in": 3600, + })), + ) + .mount(server) + .await; + let mut client = DriveClient::new(&server.uri(), &test_credentials()).unwrap(); + crate::drive::client::test_support::replace_session( + &mut client, + &test_credentials(), + &format!("{}/token", server.uri()), + ); + client + } + + fn bytes_record( + token: &str, + expires_at: DateTime, + size: u64, + path: PathBuf, + ) -> LeaseRecord { + LeaseRecord { + token: token.to_string(), + file_id: format!("file-{token}"), + version: "1".to_string(), + modified_time: None, + backup: LeaseBackup::Bytes { + path, + sha256: "deadbeef".to_string(), + size, + }, + acquired_at: expires_at - ChronoDuration::minutes(30), + expires_at, + released_at: None, + restored_at: None, + } + } + + fn drive_copy_record(token: &str, expires_at: DateTime, file_id: &str) -> LeaseRecord { + LeaseRecord { + token: token.to_string(), + file_id: format!("file-{token}"), + version: "1".to_string(), + modified_time: None, + backup: LeaseBackup::DriveCopy { + file_id: file_id.to_string(), + }, + acquired_at: expires_at - ChronoDuration::minutes(30), + expires_at, + released_at: None, + restored_at: None, + } + } + + #[tokio::test] + async fn a_live_lease_is_never_pruned_regardless_of_bounds() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let backup_path = dir.path().join("live-backup"); + std::fs::write(&backup_path, b"x").unwrap(); + + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "live", + Utc::now() + ChronoDuration::hours(1), + 1, + backup_path.clone(), + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now() + ChronoDuration::days(365)), + max_size: Some(0), + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 0); + assert_eq!(outcome.kept, 1); + assert!(backup_path.exists()); + assert_eq!(LeaseLedger::load(&ledger_path).unwrap().iter().count(), 1); + } + + #[tokio::test] + async fn older_than_alone_drops_only_rows_past_the_cutoff() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let old_backup = dir.path().join("old-backup"); + let recent_backup = dir.path().join("recent-backup"); + std::fs::write(&old_backup, b"old").unwrap(); + std::fs::write(&recent_backup, b"recent").unwrap(); + + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "old", + Utc::now() - ChronoDuration::days(10), + 3, + old_backup.clone(), + )); + ledger.insert(bytes_record( + "recent", + Utc::now() - ChronoDuration::hours(1), + 6, + recent_backup.clone(), + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now() - ChronoDuration::days(1)), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert_eq!(outcome.kept, 1); + assert_eq!(outcome.bytes_freed, 3); + assert!(!old_backup.exists()); + assert!(recent_backup.exists()); + let reloaded = LeaseLedger::load(&ledger_path).unwrap(); + assert!(reloaded.get("old").is_none()); + assert!(reloaded.get("recent").is_some()); + } + + #[tokio::test] + async fn max_size_alone_keeps_the_most_recently_expired_that_fit() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let b1 = dir.path().join("b1"); + let b2 = dir.path().join("b2"); + let b3 = dir.path().join("b3"); + for p in [&b1, &b2, &b3] { + std::fs::write(p, b"x").unwrap(); + } + + let mut ledger = LeaseLedger::default(); + // Expired 3h, 2h, 1h ago — "t3" is the most recently expired. + ledger.insert(bytes_record( + "t1", + Utc::now() - ChronoDuration::hours(3), + 10, + b1.clone(), + )); + ledger.insert(bytes_record( + "t2", + Utc::now() - ChronoDuration::hours(2), + 10, + b2.clone(), + )); + ledger.insert(bytes_record( + "t3", + Utc::now() - ChronoDuration::hours(1), + 10, + b3.clone(), + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + // Budget for exactly the two most-recently-expired (t3 + t2 = 20). + let outcome = prune( + &client, + &PruneOptions { + older_than: None, + max_size: Some(20), + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert_eq!(outcome.bytes_freed, 10); + assert!(!b1.exists(), "the oldest-expiring backup should be dropped"); + assert!(b2.exists()); + assert!(b3.exists()); + let reloaded = LeaseLedger::load(&ledger_path).unwrap(); + assert!(reloaded.get("t1").is_none()); + assert!(reloaded.get("t2").is_some()); + assert!(reloaded.get("t3").is_some()); + } + + #[tokio::test] + async fn max_size_alone_never_prunes_a_drive_copy_since_it_is_zero_bytes() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + + let mut ledger = LeaseLedger::default(); + ledger.insert(drive_copy_record( + "copy", + Utc::now() - ChronoDuration::hours(2), + "drive-copy-1", + )); + ledger.save(&ledger_path).unwrap(); + + // Deliberately no PATCH mock mounted — a DriveCopy is 0 bytes, so + // even `--max-size 0` never makes it a removal candidate on its + // own; only `--older-than` does. Reaching `FilesApi::trash` here + // fails the test. + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: None, + max_size: Some(0), + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 0); + assert_eq!(outcome.kept, 1); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("copy") + .is_some()); + } + + #[tokio::test] + async fn a_drive_copy_backup_is_trashed_and_dropped_together_with_its_row() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + + let mut ledger = LeaseLedger::default(); + ledger.insert(drive_copy_record( + "copy", + Utc::now() - ChronoDuration::hours(2), + "drive-copy-1", + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/drive-copy-1")) + .and(wiremock::matchers::body_json( + serde_json::json!({"trashed": true}), + )) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "drive-copy-1", "name": "backup", "trashed": true, + })), + ) + .expect(1) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert_eq!(outcome.trashed_drive_copies, 1); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("copy") + .is_none()); + } + + #[tokio::test] + async fn a_drive_copy_already_trashed_is_tolerated_as_success() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + + let mut ledger = LeaseLedger::default(); + ledger.insert(drive_copy_record( + "copy", + Utc::now() - ChronoDuration::hours(2), + "drive-copy-1", + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/drive-copy-1")) + .respond_with(wiremock::ResponseTemplate::new(404).set_body_string("not found")) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert_eq!(outcome.failed, 0); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("copy") + .is_none()); + } + + #[tokio::test] + async fn a_backup_deletion_failure_leaves_the_row_in_place() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + + let mut ledger = LeaseLedger::default(); + ledger.insert(drive_copy_record( + "copy", + Utc::now() - ChronoDuration::hours(2), + "drive-copy-1", + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/drive-copy-1")) + .respond_with(wiremock::ResponseTemplate::new(500).set_body_string("boom")) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 0); + assert_eq!(outcome.failed, 1); + assert_eq!(outcome.kept, 1); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("copy") + .is_some()); + } + + #[tokio::test] + async fn dry_run_reports_without_deleting_or_saving() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let backup_path = dir.path().join("backup"); + std::fs::write(&backup_path, b"x").unwrap(); + + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "old", + Utc::now() - ChronoDuration::days(1), + 1, + backup_path.clone(), + )); + ledger.save(&ledger_path).unwrap(); + let before = std::fs::read(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: true, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert_eq!(outcome.bytes_freed, 1); + assert!(backup_path.exists(), "dry-run must not delete the backup"); + let after = std::fs::read(&ledger_path).unwrap(); + assert_eq!(before, after, "dry-run must not modify the ledger"); + } + + #[tokio::test] + async fn no_op_prune_does_not_rewrite_the_ledger() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "live", + Utc::now() + ChronoDuration::hours(1), + 1, + dir.path().join("backup"), + )); + ledger.save(&ledger_path).unwrap(); + let before_mtime = std::fs::metadata(&ledger_path).unwrap().modified().unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + prune( + &client, + &PruneOptions { + older_than: Some(Utc::now() - ChronoDuration::days(1)), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + let after_mtime = std::fs::metadata(&ledger_path).unwrap().modified().unwrap(); + assert_eq!(before_mtime, after_mtime); + } + + #[tokio::test] + async fn prune_refuses_while_another_lock_is_held() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let _held = LedgerLock::acquire(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let err = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("already be in progress")); + } +} diff --git a/tests/snapshots/integration_test__help_all_output.snap b/tests/snapshots/integration_test__help_all_output.snap index f05f076b..ae734a6a 100644 --- a/tests/snapshots/integration_test__help_all_output.snap +++ b/tests/snapshots/integration_test__help_all_output.snap @@ -4035,6 +4035,7 @@ Usage: lease Commands: acquire Backs up a file and mints a lease token, prompting for device-owner authentication (Touch ID or the account password) restore Restores a file from a backup lease's recorded content, minting a fresh lease of its own before writing + prune Bounds the ledger's and the backup directory/folder's growth by dropping expired rows together with the backups they point at (ADR-0080 Consequences fast-follow, #1678) help Print this message or the help of the given subcommand(s) Options: @@ -4061,6 +4062,22 @@ Options: -h, --help Print help (see more with '--help') +================================================================================ + +omni-dev drive lease prune - Bounds the ledger's and the backup directory/folder's growth by dropping expired rows together with the backups they point at (ADR-0080 Consequences fast-follow, #1678) + +Bounds the ledger's and the backup directory/folder's growth by dropping expired rows together with the backups they point at (ADR-0080 Consequences fast-follow, #1678) + +Usage: prune [OPTIONS] + +Options: + --older-than Drop non-live rows whose expiry is older than this relative window (e.g. `7d`, `24h`, `2w`) + --max-size After `--older-than`, additionally drop the oldest-expiring survivors until their local backup bytes total at most this size (e.g. `10mb`, `512kb`, `1048576`). A `DriveCopy` backup counts as zero bytes here — it consumes no local disk + --dry-run Report what would be removed without deleting/trashing any backup or modifying the ledger + -o, --output Output format [default: table] [possible values: table, json, yaml, yamls, jsonl] + -h, --help Print help (see more with '--help') + + ================================================================================ omni-dev drive lease restore - Restores a file from a backup lease's recorded content, minting a fresh lease of its own before writing From 9e3537fc327e0ed01cf01c9ad3a65bfec27f628c Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 20:43:10 +1000 Subject: [PATCH 2/4] fix(drive,docs): close review gaps in drive lease prune MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Persist the ledger immediately after each row's backup is cleared, inside the removal loop, instead of once at the end — a crash mid-run now strands at most the one row it was working on rather than the whole batch. Align the --older-than cutoff boundary with request_log::keep_by_age's inclusive semantics (a row expiring exactly at the cutoff survives). Fold WriteCapability::Trash into Metadata, since they gate the identical scope and only differed in wording. Document drive lease prune in docs/drive.md's Lease section and update ADR-0080's Consequences/Sync-obligation text now that it has landed. --- docs/adrs/adr-0080.md | 45 +++++++------ docs/drive.md | 48 +++++++++++++ src/drive/files_api.rs | 31 +++------ src/drive/lease/prune.rs | 141 +++++++++++++++++++++++++++++++++++---- 4 files changed, 210 insertions(+), 55 deletions(-) diff --git a/docs/adrs/adr-0080.md b/docs/adrs/adr-0080.md index c548e225..84fb1dd0 100644 --- a/docs/adrs/adr-0080.md +++ b/docs/adrs/adr-0080.md @@ -306,7 +306,7 @@ backup through that same row, and a restore is almost always wanted *after* the expiry window, once a bad write has been noticed. Dropping the row on expiry would make every backup unreachable by token exactly when it is needed. The ledger therefore grows with the number of leases ever acquired, -bounded only by the `lease prune` fast-follow (see Consequences), which +bounded by `drive lease prune` (#1678, landed — see Consequences), which drops a row together with the backup it points at, never one without the other. Concurrent access — two overlapping `drive lease acquire`/write invocations against the same @@ -750,26 +750,33 @@ under the lease regime. codebase's usual best-effort logging posture** (§11), justified because a forensic log that can silently drop the one record proving a write happened is not a forensic log. -- **All three local artifacts grow unboundedly until pruned.** `audit.jsonl` - is append-only by design (§11), nothing here deletes an old backup, and - the ledger keeps every row so that `restore` can find a backup by token - after expiry (§4). A `lease prune` that drops a ledger row *and* the - backup it points at together (never one without the other), mirroring - `log prune`, is a known, deliberate - fast-follow — the same posture ADR-0071 §11 took for read-path - enforcement — not a silently dropped requirement. +- **Two of the three local artifacts grow unboundedly until pruned; + `audit.jsonl` grows unboundedly by design.** The ledger kept every row so + that `restore` could find a backup by token after expiry (§4), and + nothing deleted an old backup — bounded since #1678 (landed) by + `drive lease prune --older-than`/`--max-size`/`--dry-run`, which drops a + ledger row *and* the backup it points at together (never one without the + other: a byte backup is deleted from local disk, a Drive-copy backup is + moved to Drive Trash, and the row's removal is persisted immediately, + one row at a time — not batched across a run — so an interrupted prune + strands at most the row it was working on), mirroring `log prune`. A live + lease is never a removal candidate regardless of either bound. + `audit.jsonl` stays append-only by design (§11) and is deliberately out + of `lease prune`'s scope, the same posture ADR-0071 §11 took for + read-path enforcement — not a silently dropped requirement. - **Tamper-evident, not tamper-proof** (§12): all three local artifacts are owned by the same account an agent could compromise. The audit log's value is as a checkable trail against Drive's own revision history, never as a security boundary in its own right. - **Sync obligation.** [docs/drive.md](../drive.md) documents `drive lease - acquire`/`restore`, the `--lease` requirement per verb, the exemption - list, the config surface (§13) and the headless opt-out; ADR-0077's - recovery-message language in the codebase (not this document) now points - at the lease's own backup by Drive file id and names `drive lease restore - ` (Phase 4, landed) alongside the Drive UI, since restore only - locates a native document's copy rather than applying it; [docs/log.md](../log.md) - documents `RecordKind::Audit`, its fields, `omni-dev log --audit`, and - `audit.jsonl`'s location - and exemption from `OMNI_DEV_LOG_DISABLE`/rotation/`prune`. Keep all three - in sync when this ADR's decisions change. + acquire`/`restore`/`prune` (#1678, landed), the `--lease` requirement per + verb, the exemption list, the config surface (§13) and the headless + opt-out; ADR-0077's recovery-message language in the codebase (not this + document) now points at the lease's own backup by Drive file id and names + `drive lease restore ` (Phase 4, landed) alongside the Drive UI, + since restore only locates a native document's copy rather than applying + it; [docs/log.md](../log.md) documents `RecordKind::Audit`, its fields, + `omni-dev log --audit`, and `audit.jsonl`'s location + and exemption from `OMNI_DEV_LOG_DISABLE`/rotation/`prune` (and, by the + same reasoning, from `drive lease prune`). Keep all three in sync when + this ADR's decisions change. diff --git a/docs/drive.md b/docs/drive.md index d7384012..34715b5b 100644 --- a/docs/drive.md +++ b/docs/drive.md @@ -1209,6 +1209,54 @@ token in the ledger and in `audit.jsonl`, which records both tokens on a restore (`lease_id` the fresh one, `restored_from_lease_id` the backup one read from) — see [docs/log.md](log.md#audit-log). +### Prune + +```bash +$ omni-dev drive lease prune --older-than 30d --dry-run +$ omni-dev drive lease prune --older-than 30d +``` + +`drive lease prune` bounds the ledger's and the backup directory/folder's +otherwise-unbounded growth ([ADR-0080](adrs/adr-0080.md) Consequences, +#1678) by dropping expired rows together with the backups they point at. +It mirrors [`omni-dev log prune`](log.md#omni-dev-log-prune)'s shape: + +| Flag | Effect | +|------|--------| +| `--older-than ` | Drop non-live rows whose expiry is strictly before this relative window (`7d`, `24h`, `2w`). A row expiring exactly at the cutoff survives. | +| `--max-size ` | After age pruning, additionally drop the oldest-expiring survivors until their local backup bytes total at most `` (`10mb`, `512kb`, or a bare byte count). A Drive-copy backup counts as zero local bytes, so it's only reachable through `--older-than`. | +| `--dry-run` | Report what would be removed without deleting/trashing any backup or modifying the ledger. | + +At least one of `--older-than`/`--max-size` is required. A **live** lease +(unexpired and unreleased) is never a removal candidate regardless of +either bound — pruning can never invalidate a lease a write is still +relying on. `--max-size` always keeps at least the single +most-recently-expired row's backup, even if it alone exceeds the budget. + +A row and the backup it points at are always dropped **together, never one +without the other**: a byte backup is deleted from local disk, a +Drive-copy backup is moved to Drive Trash (recoverable by hand for ~30 +days via the Drive UI) — and the ledger row is dropped, with that removal +persisted to disk, only once its own backup has been cleared (or found +already gone). Persistence happens one row at a time, not batched across +the whole run, so an interrupted prune (a crash, a killed process) can +leave at most the one row it was working on inconsistent with its +already-cleared backup — never the rest of the run. A backup +deletion/trash failure for one row (e.g. a transient Drive error) skips +just that row, leaving it for a future prune run, rather than failing the +whole command. + +```bash +$ omni-dev drive lease prune --older-than 30d +Removed 12 lease(s); kept 4 (3 trashed Drive backup(s), 0 failure(s), freed 8241203 bytes of local backups). +``` + +`audit.jsonl` is out of scope for this command: it is append-only forensic +history by design ([ADR-0080](adrs/adr-0080.md) §11) and is never touched +here, the same exemption it has from `OMNI_DEV_LOG_DISABLE` and +`omni-dev log prune`'s own rotation — see +[docs/log.md](log.md#audit-log). + ## Sheets `drive sheets` reads and writes the *cells* of a Google Sheet through the diff --git a/src/drive/files_api.rs b/src/drive/files_api.rs index 40334881..b3605540 100644 --- a/src/drive/files_api.rs +++ b/src/drive/files_api.rs @@ -225,7 +225,8 @@ impl<'a> FilesApi<'a> { /// codebase has otherwise never needed. Requires the `drive.metadata` /// scope (`drive auth login --write`) — same as [`Self::rename`], since /// a backup copy is always a file `omni-dev` itself created via - /// [`Self::copy`]. + /// [`Self::copy`]; uses [`WriteCapability::Metadata`] rather than a + /// dedicated variant, since the scope it needs is identical. /// /// Restricted to `crate::drive` — only `crate::drive::lease::prune` may /// call this, never an ungated CLI command directly (same restriction @@ -239,7 +240,7 @@ impl<'a> FilesApi<'a> { self.client .parse_response(response, "Failed to parse files.update (trash) response") .await - .map_err(|err| append_write_scope_hint(err, WriteCapability::Trash)) + .map_err(|err| append_write_scope_hint(err, WriteCapability::Metadata)) } /// Creates a new file or folder (`files.create`, metadata-only — no @@ -590,7 +591,10 @@ fn build_file_update_url( /// 403 instead of hardcoding a single hint (issue #1574 generalization of /// [ADR-0070](../../docs/adrs/adr-0070.md) §2's rename/move-only hint). pub(crate) enum WriteCapability { - /// `files.update` on `name`/`parents` (rename/move) — `drive.metadata`. + /// `files.update` on `name`/`parents` (rename/move) or `trashed` + /// (`drive lease prune`, #1678) — `drive.metadata`. One variant covers + /// all three verbs since they share the identical scope; only the hint + /// wording below distinguishes them. Metadata, /// Creating a new file/folder, or uploading new content — `drive.file` /// or `drive`. @@ -606,10 +610,6 @@ pub(crate) enum WriteCapability { /// no app-created-it case to consider, since the lease exists /// precisely to back up files `omni-dev` did not create. CopyForBackup, - /// Trashing a `DriveCopy` lease backup during `drive lease prune` - /// (#1678) — `drive.metadata`, the same scope [`Self::Metadata`] names, - /// but worded for trashing rather than rename/move. - Trash, } /// Appends an actionable hint to a mutating-call failure caused by an @@ -641,7 +641,7 @@ pub(in crate::drive) fn append_write_scope_hint( let hint = match capability { WriteCapability::Metadata => { "Run `omni-dev drive auth login --write` to grant the drive.metadata scope needed \ - for rename/move" + for rename/move/trash" } WriteCapability::CreateOrUpload => { "Run `omni-dev drive auth login --write-file` (or `--write-full`) to grant the \ @@ -655,10 +655,6 @@ pub(in crate::drive) fn append_write_scope_hint( "Run `omni-dev drive auth login --write-full` to grant the unrestricted scope \ needed to back up a native document before a leased write, then retry" } - WriteCapability::Trash => { - "Run `omni-dev drive auth login --write` to grant the drive.metadata scope needed \ - to trash a lease backup, then retry" - } }; err.context(hint) } @@ -1740,7 +1736,7 @@ mod tests { append_write_scope_hint(insufficient_permissions_error(), WriteCapability::Metadata) .to_string(); assert!(msg.contains("--write"), "{msg}"); - assert!(msg.contains("rename/move"), "{msg}"); + assert!(msg.contains("rename/move/trash"), "{msg}"); } #[test] @@ -1777,15 +1773,6 @@ mod tests { assert!(!msg.contains("--write-file"), "{msg}"); } - #[test] - fn append_write_scope_hint_trash_names_write_flag() { - let msg = append_write_scope_hint(insufficient_permissions_error(), WriteCapability::Trash) - .to_string(); - assert!(msg.contains("--write"), "{msg}"); - assert!(msg.contains("trash a lease backup"), "{msg}"); - assert!(!msg.contains("--write-file"), "{msg}"); - } - // ── check_download_size ───────────────────────────────────────── #[test] diff --git a/src/drive/lease/prune.rs b/src/drive/lease/prune.rs index c85b70d8..ba29e629 100644 --- a/src/drive/lease/prune.rs +++ b/src/drive/lease/prune.rs @@ -12,7 +12,9 @@ //! [`LeaseBackup::Bytes`] backup is deleted from local disk, a //! [`LeaseBackup::DriveCopy`] backup is moved to Drive Trash //! ([`FilesApi::trash`]), and only once that step succeeds (or finds the -//! backup already gone) does the row itself get dropped from the ledger. +//! backup already gone) does the row itself get dropped from the ledger — +//! and the ledger is saved immediately, per row, not batched until the run +//! finishes, so a crash mid-run strands at most the one row it interrupts. //! A backup-deletion failure for one row skips just that row — it is left //! for a future prune, not treated as a hard error for the whole command. @@ -30,8 +32,10 @@ use crate::drive::lease::ledger::{LeaseBackup, LeaseLedger, LeaseRecord, LedgerL /// Options controlling [`prune`]. pub struct PruneOptions { - /// Drop non-live rows whose `expires_at` is at or before this cutoff. A - /// live row is never a candidate regardless of this bound. + /// Drop non-live rows whose `expires_at` is strictly before this + /// cutoff (a row expiring exactly at the cutoff survives) — mirrors + /// `request_log::keep_by_age`'s inclusive boundary. A live row is never + /// a candidate regardless of this bound. pub older_than: Option>, /// After the age filter, additionally drop the oldest-expiring /// survivors until the local backup bytes they account for total at @@ -131,9 +135,14 @@ fn keep_count_by_size(sorted_desc: &[&LeaseRecord], max: u64) -> usize { /// Prunes the lease ledger at `opts.ledger_path` by age and/or size, /// dropping each removed row together with the backup it points at. /// -/// Runs under [`LedgerLock`] for its whole duration (load through save), so -/// a concurrent `drive lease acquire`/`restore` can't race the rewrite — -/// the same lock those commands themselves hold while mutating the ledger. +/// Runs under [`LedgerLock`] for its whole duration (load through the last +/// save), so a concurrent `drive lease acquire`/`restore` can't race the +/// rewrite — the same lock those commands themselves hold while mutating +/// the ledger. That single continuous lock is also what makes the per-row +/// save below safe: nothing else can observe or rewrite the ledger between +/// one row's removal and the next, so there is no window for a concurrent +/// writer (e.g. `restore`'s `mark_restored`, ADR-0080 §4) to race a save +/// this function didn't yet make. pub async fn prune(client: &DriveClient, opts: &PruneOptions) -> Result { let _lock = LedgerLock::acquire(&opts.ledger_path)?; let mut ledger = LeaseLedger::load(&opts.ledger_path)?; @@ -153,12 +162,13 @@ pub async fn prune(client: &DriveClient, opts: &PruneOptions) -> Result=`). let (mut age_survivors, mut removed): (Vec, Vec) = non_live.into_iter().partition(|rec| match opts.older_than { - Some(cutoff) => rec.expires_at > cutoff, + Some(cutoff) => rec.expires_at >= cutoff, None => true, }); @@ -189,6 +199,12 @@ pub async fn prune(client: &DriveClient, opts: &PruneOptions) -> Result outcome.bytes_freed += size, @@ -200,10 +216,6 @@ pub async fn prune(client: &DriveClient, opts: &PruneOptions) -> Result 0 { - ledger.save(&opts.ledger_path)?; - } - Ok(outcome) } @@ -367,6 +379,107 @@ mod tests { assert!(reloaded.get("recent").is_some()); } + #[tokio::test] + async fn older_than_boundary_keeps_a_row_expiring_exactly_at_the_cutoff() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let backup_path = dir.path().join("backup"); + std::fs::write(&backup_path, b"x").unwrap(); + + let cutoff = Utc::now() - ChronoDuration::days(1); + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "on-the-boundary", + cutoff, + 1, + backup_path.clone(), + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(cutoff), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!( + outcome.removed, 0, + "a row expiring exactly at the cutoff must survive, mirroring \ + `request_log::keep_by_age`'s inclusive `>=` boundary" + ); + assert_eq!(outcome.kept, 1); + assert!(backup_path.exists()); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("on-the-boundary") + .is_some()); + } + + #[tokio::test] + async fn a_mixed_batch_persists_successful_removals_independently_of_a_failing_one() { + let dir = tempfile::tempdir().unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + + let mut ledger = LeaseLedger::default(); + ledger.insert(drive_copy_record( + "ok", + Utc::now() - ChronoDuration::hours(2), + "drive-copy-ok", + )); + ledger.insert(drive_copy_record( + "bad", + Utc::now() - ChronoDuration::hours(2), + "drive-copy-bad", + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/drive-copy-ok")) + .respond_with( + wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "drive-copy-ok", "name": "backup", "trashed": true, + })), + ) + .mount(&server) + .await; + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/drive-copy-bad")) + .respond_with(wiremock::ResponseTemplate::new(500).set_body_string("boom")) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert_eq!(outcome.failed, 1); + assert_eq!(outcome.kept, 1); + let reloaded = LeaseLedger::load(&ledger_path).unwrap(); + assert!( + reloaded.get("ok").is_none(), + "the successful removal must be persisted regardless of the other row's outcome" + ); + assert!(reloaded.get("bad").is_some()); + } + #[tokio::test] async fn max_size_alone_keeps_the_most_recently_expired_that_fit() { let dir = tempfile::tempdir().unwrap(); From a081c8f4ab2ede2c8a334c92f88b8e609a551ee4 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 22:53:13 +1000 Subject: [PATCH 3/4] fix(drive,docs): close correctness/availability gaps in drive lease prune MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a real correctness bug: --max-size alone could trash a zero-byte DriveCopy backup when it sorted behind an oversized Bytes backup that alone exceeded the budget, contradicting the documented "DriveCopy is only reachable via --older-than" contract. The size filter now sets DriveCopy rows aside before computing the byte budget, so they never participate in it regardless of position. Shrink the ledger lock's hold time: rather than one continuous lock across the whole run (including every sequential Drive trash call), the lock is now taken once to snapshot removal candidates and once per row (via the existing mutate_locked) to persist that row's own removal — the same brief load-mutate-save discipline every other lease command already uses, so a large prune no longer blocks concurrent acquire/restore/writes for the run's full duration. Log and audit every removal attempt: a clear_backup failure now carries its token/file id/error into a tracing::warn! instead of collapsing into an opaque count, and every attempt (pruned or failed) writes its own best-effort audit.jsonl record, mirroring acquire's audit trail so "why is this backup gone" stays answerable per lease. Smaller fixes: the CLI summary no longer claims backups were "trashed"/bytes were "freed" in past tense under --dry-run; a pre-existing acquire.rs comment claiming prune reclaims the AlreadyLeased race's orphaned backup is corrected (prune only ever walks existing ledger rows, so that orphan is a known, separate gap); a ledger-save failure immediately after a backup was cleared now surfaces a loud, actionable error naming the stranded token instead of propagating a generic one. --- docs/adrs/adr-0080.md | 13 +- docs/drive.md | 26 ++- src/cli/drive/lease.rs | 8 +- src/drive/lease/acquire.rs | 8 +- src/drive/lease/prune.rs | 384 ++++++++++++++++++++++++++++++------- 5 files changed, 353 insertions(+), 86 deletions(-) diff --git a/docs/adrs/adr-0080.md b/docs/adrs/adr-0080.md index 84fb1dd0..c965fd84 100644 --- a/docs/adrs/adr-0080.md +++ b/docs/adrs/adr-0080.md @@ -760,10 +760,15 @@ under the lease regime. moved to Drive Trash, and the row's removal is persisted immediately, one row at a time — not batched across a run — so an interrupted prune strands at most the row it was working on), mirroring `log prune`. A live - lease is never a removal candidate regardless of either bound. - `audit.jsonl` stays append-only by design (§11) and is deliberately out - of `lease prune`'s scope, the same posture ADR-0071 §11 took for - read-path enforcement — not a silently dropped requirement. + lease is never a removal candidate regardless of either bound, and every + removal attempt writes its own best-effort audit record (`"pruned"` or + `"prune-failed"`, ADR-0080 §11's usual fail-open posture) so a pruned + backup stays discoverable via `omni-dev log --audit`. `audit.jsonl` + itself stays append-only by design (§11) and is deliberately out of + `lease prune`'s scope *as a pruning target* — it is never rotated or + truncated by this command, the same posture ADR-0071 §11 took for + read-path enforcement — not a silently dropped requirement; that is + distinct from `lease prune` writing *to* it. - **Tamper-evident, not tamper-proof** (§12): all three local artifacts are owned by the same account an agent could compromise. The audit log's value is as a checkable trail against Drive's own revision history, never diff --git a/docs/drive.md b/docs/drive.md index 34715b5b..81f8c732 100644 --- a/docs/drive.md +++ b/docs/drive.md @@ -1241,20 +1241,30 @@ persisted to disk, only once its own backup has been cleared (or found already gone). Persistence happens one row at a time, not batched across the whole run, so an interrupted prune (a crash, a killed process) can leave at most the one row it was working on inconsistent with its -already-cleared backup — never the rest of the run. A backup -deletion/trash failure for one row (e.g. a transient Drive error) skips -just that row, leaving it for a future prune run, rather than failing the -whole command. +already-cleared backup — never the rest of the run. The ledger lock +itself is likewise held only briefly per step (once to decide candidates, +then once per row to persist that row's own removal), not for the whole +run, so a large batch never blocks a concurrent `drive lease +acquire`/`restore`/write for longer than a single row's own local disk +I/O. A backup deletion/trash failure for one row (e.g. a transient Drive +error) is logged and skips just that row, leaving it for a future prune +run, rather than failing the whole command. ```bash $ omni-dev drive lease prune --older-than 30d Removed 12 lease(s); kept 4 (3 trashed Drive backup(s), 0 failure(s), freed 8241203 bytes of local backups). ``` -`audit.jsonl` is out of scope for this command: it is append-only forensic -history by design ([ADR-0080](adrs/adr-0080.md) §11) and is never touched -here, the same exemption it has from `OMNI_DEV_LOG_DISABLE` and -`omni-dev log prune`'s own rotation — see +Every removal attempt — successful or failed — writes its own best-effort +`audit.jsonl` record (`verdict: "pruned"` or `"prune-failed"`, the latter +carrying the underlying error), the same fail-open posture `drive lease +acquire`'s own audit trail uses, so `omni-dev log --audit` can always +answer "why is this backup gone" for a specific lease. This is distinct +from `audit.jsonl` itself being out of scope *as a pruning target*: the +file is append-only forensic history by design +([ADR-0080](adrs/adr-0080.md) §11) and `drive lease prune` never rotates +or deletes its content, the same exemption it has from +`OMNI_DEV_LOG_DISABLE` and `omni-dev log prune`'s own rotation — see [docs/log.md](log.md#audit-log). ## Sheets diff --git a/src/cli/drive/lease.rs b/src/cli/drive/lease.rs index 91b417f6..7ee59f1c 100644 --- a/src/cli/drive/lease.rs +++ b/src/cli/drive/lease.rs @@ -299,13 +299,13 @@ impl PruneCommand { if output_as(&outcome, &self.output)? { return Ok(()); } - let verb = if self.dry_run { - "Would remove" + let (verb, backup_verb, byte_verb) = if self.dry_run { + ("Would remove", "would trash", "would free") } else { - "Removed" + ("Removed", "trashed", "freed") }; println!( - "{verb} {} lease(s); kept {} ({} trashed Drive backup(s), {} failure(s), freed \ + "{verb} {} lease(s); kept {} ({} {backup_verb} Drive backup(s), {} failure(s), {byte_verb} \ {} bytes of local backups).", outcome.removed, outcome.kept, diff --git a/src/drive/lease/acquire.rs b/src/drive/lease/acquire.rs index 11f73e3c..e21e3b16 100644 --- a/src/drive/lease/acquire.rs +++ b/src/drive/lease/acquire.rs @@ -298,9 +298,11 @@ async fn acquire_inner( Ok(InsertOutcome::Inserted) => {} // Refuse rather than mint a second, independent lease on a file // that already has a live one — see `AcquireResult::AlreadyLeased`'s - // doc comment. The backup just taken above is orphaned; a `lease - // prune` reclaiming stray backups is the same deliberate fast-follow - // the ledger module doc names for expired/released rows. + // doc comment. The backup just taken above is orphaned: it was + // never inserted into the ledger, so `drive lease prune` (#1678, + // which only ever iterates existing ledger rows) cannot discover + // or reclaim it — a known gap this race leaves open, distinct from + // the expired/released-row growth `lease prune` does bound. Ok(InsertOutcome::AlreadyLeased(existing)) => { return AcquireResult::AlreadyLeased { token: existing.token, diff --git a/src/drive/lease/prune.rs b/src/drive/lease/prune.rs index ba29e629..5f9da246 100644 --- a/src/drive/lease/prune.rs +++ b/src/drive/lease/prune.rs @@ -15,8 +15,20 @@ //! backup already gone) does the row itself get dropped from the ledger — //! and the ledger is saved immediately, per row, not batched until the run //! finishes, so a crash mid-run strands at most the one row it interrupts. -//! A backup-deletion failure for one row skips just that row — it is left -//! for a future prune, not treated as a hard error for the whole command. +//! A backup-deletion failure for one row skips just that row, logging and +//! auditing why — it is left for a future prune, not treated as a hard +//! error for the whole command. +//! +//! `--max-size` bounds *local* bytes only: a `DriveCopy` backup counts as +//! zero and never participates in that budget, so it is reachable only +//! through `--older-than` — a `DriveCopy` sorted behind an oversized +//! `Bytes` backup must never be dropped as that backup's collateral +//! damage. The ledger lock is held only briefly per step (once to decide +//! candidates, then once per row to persist that row's removal) rather +//! than for the whole run, so a large batch never blocks a concurrent +//! `drive lease acquire`/`restore`/write for longer than a single row's +//! own local disk I/O — the same lock discipline every other lease command +//! already uses. use std::path::PathBuf; @@ -110,12 +122,56 @@ async fn clear_backup(files_api: &FilesApi<'_>, backup: &LeaseBackup) -> Result< } } +/// Applies one removed row's backup to `outcome`'s tallies. Shared by the +/// dry-run and real-removal paths so their accounting can never drift +/// apart — both must treat "this row is being removed" identically. +fn account_removal(outcome: &mut PruneOutcome, backup: &LeaseBackup) { + outcome.removed += 1; + match backup { + LeaseBackup::Bytes { size, .. } => outcome.bytes_freed += size, + LeaseBackup::DriveCopy { .. } => outcome.trashed_drive_copies += 1, + } +} + +/// Writes `drive lease prune`'s own best-effort audit record (ADR-0080 +/// §11) for one row, mirroring `acquire.rs`'s `record_attempt`'s +/// backup-field mapping. Never fails the prune itself — a logging failure +/// here only warns, the same posture `acquire`/`restore` already take, +/// since the destructive step (the backup already cleared, or refused to +/// be) has already happened by the time this is called. +fn record_prune_attempt(rec: &LeaseRecord, verdict: &str, error: Option) { + let (backup_location, backup_sha256, backup_size) = match &rec.backup { + LeaseBackup::Bytes { path, sha256, size } => ( + Some(path.display().to_string()), + Some(sha256.clone()), + Some(*size), + ), + LeaseBackup::DriveCopy { file_id } => (Some(file_id.clone()), None, None), + }; + let outcome = crate::request_log::AuditOutcome { + command: vec!["drive".to_string(), "lease-prune".to_string()], + integration: "drive", + file_id: rec.file_id.clone(), + lease_id: Some(rec.token.clone()), + verdict: verdict.to_string(), + backup_location, + backup_sha256, + backup_size, + error, + ..Default::default() + }; + if let Err(err) = crate::request_log::record_audit_event(outcome) { + tracing::warn!("drive lease prune: failed to write audit record: {err}"); + } +} + /// Longest prefix of `sorted_desc` (sorted newest-`expires_at`-first) whose /// backup bytes fit within `max` — but never fewer than the single /// most-recently-expired candidate, even if it alone exceeds the budget. /// Mirrors `request_log::keep_by_size`, adapted from reverse iteration over /// chronological lines to forward iteration over an already-descending -/// slice. +/// slice. Only ever called with `Bytes`-backed candidates (see +/// [`prune`]) — a `DriveCopy` never participates in this budget at all. fn keep_count_by_size(sorted_desc: &[&LeaseRecord], max: u64) -> usize { let mut acc = 0u64; let mut keep = 0usize; @@ -135,84 +191,121 @@ fn keep_count_by_size(sorted_desc: &[&LeaseRecord], max: u64) -> usize { /// Prunes the lease ledger at `opts.ledger_path` by age and/or size, /// dropping each removed row together with the backup it points at. /// -/// Runs under [`LedgerLock`] for its whole duration (load through the last -/// save), so a concurrent `drive lease acquire`/`restore` can't race the -/// rewrite — the same lock those commands themselves hold while mutating -/// the ledger. That single continuous lock is also what makes the per-row -/// save below safe: nothing else can observe or rewrite the ledger between -/// one row's removal and the next, so there is no window for a concurrent -/// writer (e.g. `restore`'s `mark_restored`, ADR-0080 §4) to race a save -/// this function didn't yet make. +/// The ledger lock is held only briefly, not for this whole call: once to +/// decide the removal candidates (below), then once per row (via +/// [`LeaseLedger::mutate_locked`]) to persist that row's own removal — +/// mirroring the brief load-mutate-save hold every other lease command +/// uses, rather than one continuous lock for a potentially long batch of +/// sequential Drive `trash` calls. Nothing else in this codebase ever +/// removes a ledger row, so re-removing a candidate's token by name from +/// whatever the ledger looks like at that later moment is always safe, +/// even if a concurrent `acquire`/`restore` changed unrelated rows in the +/// gap between the snapshot and this row's own turn. pub async fn prune(client: &DriveClient, opts: &PruneOptions) -> Result { - let _lock = LedgerLock::acquire(&opts.ledger_path)?; - let mut ledger = LeaseLedger::load(&opts.ledger_path)?; let now = Utc::now(); - let (live_count, non_live): (usize, Vec) = { - let mut live_count = 0usize; - let mut non_live = Vec::new(); - for rec in ledger.iter() { - if rec.is_live(now) { - live_count += 1; - } else { - non_live.push(rec.clone()); + let (kept_after_filters, removed) = { + let _lock = LedgerLock::acquire(&opts.ledger_path)?; + let ledger = LeaseLedger::load(&opts.ledger_path)?; + + let (live_count, non_live): (usize, Vec) = { + let mut live_count = 0usize; + let mut non_live = Vec::new(); + for rec in ledger.iter() { + if rec.is_live(now) { + live_count += 1; + } else { + non_live.push(rec.clone()); + } } + (live_count, non_live) + }; + + // Age filter: a non-live row survives into the size filter unless + // it's strictly past the `--older-than` cutoff (no cutoff means + // every non-live row proceeds to the size filter, mirroring + // `request_log::prune` with `older_than: None`; a row expiring + // exactly at the cutoff survives, mirroring + // `request_log::keep_by_age`'s `>=`). + let (mut age_survivors, mut removed): (Vec, Vec) = + non_live.into_iter().partition(|rec| match opts.older_than { + Some(cutoff) => rec.expires_at >= cutoff, + None => true, + }); + + // Size filter, applied only to the `Bytes`-backed age survivors — + // a `DriveCopy` contributes zero local bytes and must stay + // reachable only through `--older-than`, so it is set aside first + // and always kept here regardless of position, never dropped as + // collateral damage from an oversized `Bytes` backup sorted ahead + // of it in the same budget. + if let Some(max) = opts.max_size { + let (mut bytes_survivors, copy_survivors): (Vec, Vec) = + age_survivors + .into_iter() + .partition(|rec| matches!(rec.backup, LeaseBackup::Bytes { .. })); + bytes_survivors.sort_by_key(|rec| std::cmp::Reverse(rec.expires_at)); + let refs: Vec<&LeaseRecord> = bytes_survivors.iter().collect(); + let keep = keep_count_by_size(&refs, max); + removed.extend(bytes_survivors.split_off(keep)); + age_survivors = bytes_survivors; + age_survivors.extend(copy_survivors); } - (live_count, non_live) - }; - // Age filter: a non-live row survives into the size filter unless it's - // strictly past the `--older-than` cutoff (no cutoff means every - // non-live row proceeds to the size filter, mirroring - // `request_log::prune` with `older_than: None`; a row expiring exactly - // at the cutoff survives, mirroring `request_log::keep_by_age`'s `>=`). - let (mut age_survivors, mut removed): (Vec, Vec) = - non_live.into_iter().partition(|rec| match opts.older_than { - Some(cutoff) => rec.expires_at >= cutoff, - None => true, - }); - - // Size filter, applied only to the age survivors: keep the - // most-recently-expired ones whose backups fit `max_size`, moving the - // rest into `removed` too. - if let Some(max) = opts.max_size { - age_survivors.sort_by_key(|rec| std::cmp::Reverse(rec.expires_at)); - let refs: Vec<&LeaseRecord> = age_survivors.iter().collect(); - let keep = keep_count_by_size(&refs, max); - removed.extend(age_survivors.split_off(keep)); - } + (live_count + age_survivors.len(), removed) + }; - let files_api = FilesApi::new(client); let mut outcome = PruneOutcome { - kept: live_count + age_survivors.len(), + kept: kept_after_filters, ..Default::default() }; + if opts.dry_run { + for rec in &removed { + account_removal(&mut outcome, &rec.backup); + } + return Ok(outcome); + } + + let files_api = FilesApi::new(client); for rec in &removed { - if opts.dry_run { - outcome.removed += 1; - match &rec.backup { - LeaseBackup::Bytes { size, .. } => outcome.bytes_freed += size, - LeaseBackup::DriveCopy { .. } => outcome.trashed_drive_copies += 1, + match clear_backup(&files_api, &rec.backup).await { + Ok(()) => { + // The backup is already gone (or was found already gone), + // so the ledger must catch up before anything else — a + // crash right after this call strands at most this one + // row, never the rest of the batch. A failure *here* + // (distinct from a crash) can't be made fully atomic with + // the backup step above across two different storage + // systems, so make it loud and actionable instead of + // silently leaving a dangling row: name the stranded token + // and stop, rather than compounding the same failure + // across every remaining candidate. + if let Err(err) = LeaseLedger::mutate_locked(&opts.ledger_path, |ledger| { + ledger.remove(&rec.token); + }) { + return Err(err.context(format!( + "drive lease prune: cleared the backup for lease {} but failed to \ + persist its ledger removal — that row may now dangle, pointing at a \ + backup that no longer exists; remove it from the ledger by hand once \ + the underlying issue is fixed", + rec.token + ))); + } + account_removal(&mut outcome, &rec.backup); + record_prune_attempt(rec, "pruned", None); } - continue; - } - if clear_backup(&files_api, &rec.backup).await.is_ok() { - ledger.remove(&rec.token); - // Persisted immediately, one row at a time: the backup is - // already gone (or was found already gone), so the ledger must - // catch up before this function does anything else — a crash - // right after this call strands at most this one row, never - // the rest of the batch. - ledger.save(&opts.ledger_path)?; - outcome.removed += 1; - match &rec.backup { - LeaseBackup::Bytes { size, .. } => outcome.bytes_freed += size, - LeaseBackup::DriveCopy { .. } => outcome.trashed_drive_copies += 1, + Err(err) => { + tracing::warn!( + "drive lease prune: failed to clear the backup for lease {} (file {}): \ + {err:#}; leaving it for a future prune", + rec.token, + rec.file_id + ); + record_prune_attempt(rec, "prune-failed", Some(err.to_string())); + outcome.failed += 1; + outcome.kept += 1; } - } else { - outcome.failed += 1; - outcome.kept += 1; } } @@ -224,6 +317,7 @@ pub async fn prune(client: &DriveClient, opts: &PruneOptions) -> Result Date: Tue, 15 Sep 2026 00:34:06 +1000 Subject: [PATCH 4/4] test(drive): close patch-coverage gaps in drive lease prune Adds direct/targeted tests for the lines PR #1684's coverage bot flagged: backup_size's DriveCopy arm, PruneOutcome's JsonlSerialize impl, a Bytes backup's NotFound-tolerated and non-NotFound-failure clear_backup paths, the best-effort audit-write failure being warned and swallowed, the --max-size-only/older_than:None CLI branch dispatched through LeaseCommand::execute, the dry-run and non-dry-run human-readable table summaries, and the "cleared the backup but failed to persist its ledger removal" dangling-row error path (forced deterministically via a delayed mock response plus a background thread that revokes the ledger directory's write permission mid-run). --- src/cli/drive/lease.rs | 114 ++++++++++++++++++++ src/drive/lease/prune.rs | 224 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 338 insertions(+) diff --git a/src/cli/drive/lease.rs b/src/cli/drive/lease.rs index 7ee59f1c..7adff58d 100644 --- a/src/cli/drive/lease.rs +++ b/src/cli/drive/lease.rs @@ -1084,4 +1084,118 @@ mod tests { let reloaded = crate::drive::lease::ledger::LeaseLedger::load(&ledger_path).unwrap(); assert!(reloaded.get("old-token").is_none()); } + + #[tokio::test] + async fn prune_command_max_size_only_dispatches_through_lease_command_and_prints_a_table_summary( + ) { + // `--max-size` alone (no `--older-than`) through the top-level + // `LeaseCommand::execute` dispatch, with the default `Table` + // output — covers the `older_than: None` branch, the `--max-size` + // parse branch, and the human-readable non-dry-run summary, none + // of which the other prune tests (which always pass `--older-than` + // and always request JSON output) reach. + let guard = crate::drive::test_support::EnvGuard::take(); + let dir = guard.clear_credentials(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + + let ledger_path = crate::drive::lease::ledger::ledger_path().unwrap(); + let old_backup = dir.path().join("old-backup.bin"); + let new_backup = dir.path().join("new-backup.bin"); + std::fs::write(&old_backup, vec![0u8; 100]).unwrap(); + std::fs::write(&new_backup, vec![0u8; 100]).unwrap(); + let mut ledger = crate::drive::lease::ledger::LeaseLedger::default(); + ledger.insert(crate::drive::lease::ledger::LeaseRecord { + token: "old-token".to_string(), + file_id: "file-1".to_string(), + version: "1".to_string(), + modified_time: None, + backup: LeaseBackup::Bytes { + path: old_backup.clone(), + sha256: "deadbeef".to_string(), + size: 100, + }, + acquired_at: chrono::Utc::now() - chrono::Duration::hours(3), + expires_at: chrono::Utc::now() - chrono::Duration::hours(2), + released_at: None, + restored_at: None, + }); + ledger.insert(crate::drive::lease::ledger::LeaseRecord { + token: "new-token".to_string(), + file_id: "file-2".to_string(), + version: "1".to_string(), + modified_time: None, + backup: LeaseBackup::Bytes { + path: new_backup.clone(), + sha256: "deadbeef".to_string(), + size: 100, + }, + acquired_at: chrono::Utc::now() - chrono::Duration::hours(2), + expires_at: chrono::Utc::now() - chrono::Duration::hours(1), + released_at: None, + restored_at: None, + }); + ledger.save(&ledger_path).unwrap(); + + let lease_cmd = LeaseCommand { + action: LeaseAction::Prune(PruneCommand { + older_than: None, + max_size: Some("150".to_string()), + dry_run: false, + output: OutputFormat::Table, + }), + }; + lease_cmd.execute(&client).await.unwrap(); + + assert!(!old_backup.exists()); + assert!(new_backup.exists()); + let reloaded = crate::drive::lease::ledger::LeaseLedger::load(&ledger_path).unwrap(); + assert!(reloaded.get("old-token").is_none()); + assert!(reloaded.get("new-token").is_some()); + } + + #[tokio::test] + async fn prune_command_dry_run_prints_a_would_remove_table_summary() { + // The dry-run half of the human-readable summary's verb/backup-verb + // tuple — the non-dry-run half is covered above. + let guard = crate::drive::test_support::EnvGuard::take(); + let dir = guard.clear_credentials(); + let _audit = crate::test_support::AuditLogGuard::redirect(dir.path()); + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + + let ledger_path = crate::drive::lease::ledger::ledger_path().unwrap(); + let backup_path = dir.path().join("old-backup.bin"); + std::fs::write(&backup_path, b"stale").unwrap(); + let mut ledger = crate::drive::lease::ledger::LeaseLedger::default(); + ledger.insert(crate::drive::lease::ledger::LeaseRecord { + token: "old-token".to_string(), + file_id: "file-1".to_string(), + version: "1".to_string(), + modified_time: None, + backup: LeaseBackup::Bytes { + path: backup_path.clone(), + sha256: "deadbeef".to_string(), + size: 5, + }, + acquired_at: chrono::Utc::now() - chrono::Duration::days(10), + expires_at: chrono::Utc::now() - chrono::Duration::days(9), + released_at: None, + restored_at: None, + }); + ledger.save(&ledger_path).unwrap(); + + let cmd = PruneCommand { + older_than: Some("1d".to_string()), + max_size: None, + dry_run: true, + output: OutputFormat::Table, + }; + cmd.execute(&client).await.unwrap(); + + assert!(backup_path.exists(), "dry-run must not delete the backup"); + let reloaded = crate::drive::lease::ledger::LeaseLedger::load(&ledger_path).unwrap(); + assert!(reloaded.get("old-token").is_some()); + } } diff --git a/src/drive/lease/prune.rs b/src/drive/lease/prune.rs index 5f9da246..003151a2 100644 --- a/src/drive/lease/prune.rs +++ b/src/drive/lease/prune.rs @@ -389,6 +389,38 @@ mod tests { } } + #[test] + fn backup_size_is_zero_for_a_drive_copy() { + // A `DriveCopy` never reaches `backup_size` through `prune`'s own + // call site today (only `Bytes`-backed survivors are ever passed to + // `keep_count_by_size`) — covered directly here so the zero-bytes + // contract documented on `backup_size` itself stays pinned even if + // that call site ever changes. + assert_eq!( + backup_size(&LeaseBackup::DriveCopy { + file_id: "drive-copy-1".to_string(), + }), + 0 + ); + } + + #[test] + fn prune_outcome_serializes_to_jsonl() { + let mut buf = Vec::new(); + PruneOutcome { + removed: 1, + kept: 2, + bytes_freed: 3, + trashed_drive_copies: 1, + failed: 0, + } + .write_jsonl(&mut buf) + .unwrap(); + let text = String::from_utf8(buf).unwrap(); + assert!(text.contains("\"removed\":1"), "{text}"); + assert!(text.contains("\"kept\":2"), "{text}"); + } + #[tokio::test] async fn a_live_lease_is_never_pruned_regardless_of_bounds() { let dir = tempfile::tempdir().unwrap(); @@ -831,6 +863,92 @@ mod tests { .is_none()); } + #[tokio::test] + async fn a_bytes_backup_already_removed_is_tolerated_as_success() { + let dir = tempfile::tempdir().unwrap(); + let _audit = AuditGuard::redirect(dir.path()); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + // Deliberately never written to disk — a prior manual cleanup or a + // previous partially-failed prune run could have already removed + // it; `clear_backup` must tolerate `NotFound` the same way it does + // for a `DriveCopy`'s already-trashed 404. + let backup_path = dir.path().join("already-gone"); + + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "old", + Utc::now() - ChronoDuration::days(1), + 1, + backup_path, + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert_eq!(outcome.failed, 0); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("old") + .is_none()); + } + + #[tokio::test] + async fn a_bytes_backup_deletion_failure_leaves_the_row_in_place() { + let dir = tempfile::tempdir().unwrap(); + let _audit = AuditGuard::redirect(dir.path()); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + // A directory in place of the backup file forces `remove_file` to + // fail with something other than `NotFound` (mirrors the same + // trick used elsewhere in this crate to force a non-`NotFound` + // I/O failure deterministically). + let backup_path = dir.path().join("not-a-file"); + std::fs::create_dir(&backup_path).unwrap(); + + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "old", + Utc::now() - ChronoDuration::days(1), + 1, + backup_path, + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 0); + assert_eq!(outcome.failed, 1); + assert_eq!(outcome.kept, 1); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("old") + .is_some()); + } + #[tokio::test] async fn a_backup_deletion_failure_leaves_the_row_in_place() { let dir = tempfile::tempdir().unwrap(); @@ -1026,6 +1144,112 @@ mod tests { ); } + #[tokio::test] + async fn a_best_effort_audit_write_failure_is_warned_and_swallowed() { + // Mirrors `restore.rs`'s identically-named test: a directory in + // place of the audit file forces `record_audit_event` to fail. + // `record_prune_attempt` is best-effort — the prune itself (already + // committed by the time this is called) must still succeed. + let dir = tempfile::tempdir().unwrap(); + let _audit = AuditGuard::redirect(dir.path()); + std::fs::create_dir(dir.path().join("audit.jsonl")).unwrap(); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + let backup_path = dir.path().join("backup"); + std::fs::write(&backup_path, b"x").unwrap(); + + let mut ledger = LeaseLedger::default(); + ledger.insert(bytes_record( + "old", + Utc::now() - ChronoDuration::days(1), + 1, + backup_path.clone(), + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + let client = client_with_bootstrapped_token(&server).await; + let outcome = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap(); + + assert_eq!(outcome.removed, 1); + assert!(!backup_path.exists()); + assert!(LeaseLedger::load(&ledger_path) + .unwrap() + .get("old") + .is_none()); + } + + #[cfg(unix)] + #[tokio::test] + async fn a_ledger_persist_failure_after_clearing_the_backup_is_reported_loudly() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let _audit = AuditGuard::redirect(dir.path()); + let ledger_path = dir.path().join("lease-ledger.jsonl"); + + let mut ledger = LeaseLedger::default(); + ledger.insert(drive_copy_record( + "copy", + Utc::now() - ChronoDuration::hours(2), + "drive-copy-1", + )); + ledger.save(&ledger_path).unwrap(); + + let server = wiremock::MockServer::start().await; + // An artificial delay on the `trash` response gives the background + // thread below a wide window to break the ledger directory's + // writability *after* this run's own initial (candidate-selection) + // lock has already been acquired and released, but *before* the + // per-row removal reaches its own `mutate_locked` call — the exact + // gap `prune`'s own doc comment calls out as the one case a crash + // (or, here, a filesystem fault) can strand a row. + wiremock::Mock::given(wiremock::matchers::method("PATCH")) + .and(wiremock::matchers::path("/drive/v3/files/drive-copy-1")) + .respond_with( + wiremock::ResponseTemplate::new(200) + .set_body_json(serde_json::json!({ + "id": "drive-copy-1", "name": "backup", "trashed": true, + })) + .set_delay(std::time::Duration::from_millis(150)), + ) + .mount(&server) + .await; + let client = client_with_bootstrapped_token(&server).await; + + let dir_path = dir.path().to_path_buf(); + let jammer = std::thread::spawn(move || { + std::thread::sleep(std::time::Duration::from_millis(30)); + std::fs::set_permissions(&dir_path, std::fs::Permissions::from_mode(0o500)).unwrap(); + }); + + let err = prune( + &client, + &PruneOptions { + older_than: Some(Utc::now()), + max_size: None, + dry_run: false, + ledger_path: ledger_path.clone(), + }, + ) + .await + .unwrap_err(); + + jammer.join().unwrap(); + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); + + assert!(err.to_string().contains("may now dangle"), "{err:?}"); + } + #[tokio::test] async fn prune_refuses_while_another_lock_is_held() { let dir = tempfile::tempdir().unwrap();