fix(drive) locks + 1.7.1

crates/pdfs-core/src/db/migrations.rs

@@ -9,7 +9,7 @@ use super::Db;
 use crate::Result;
 
 /// Current schema version. Bump on every forward migration added below.
-pub(super) const SCHEMA_VERSION: i64 = 25;
+pub(super) const SCHEMA_VERSION: i64 = 26;
 
 impl Db {
     pub(super) fn migrate(&self) -> Result<()> {
@@ -161,6 +161,27 @@ impl Db {
                 tx.execute_batch(MIGRATION_V25)?;
             }
         }
+        if current < 26 {
+            // Same guard as V22: a fixture that never exercised the queue may
+            // not have `pending_op`, and altering an absent table is an error
+            // rather than a no-op.
+            // A fixture built from the current schema and then rewound to an
+            // older `schema_version` already has the column, and `ADD COLUMN`
+            // has no `IF NOT EXISTS`.
+            let has_column: bool = tx.query_row(
+                "SELECT COUNT(*) FROM pragma_table_info('pending_op') WHERE name = 'claimed_at'",
+                [],
+                |row| row.get::<_, i64>(0),
+            )? > 0;
+            let has_ops: bool = tx.query_row(
+                "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'pending_op'",
+                [],
+                |row| row.get::<_, i64>(0),
+            )? > 0;
+            if has_ops && !has_column {
+                tx.execute_batch(MIGRATION_V26)?;
+            }
+        }
         tx.execute(
             "INSERT INTO sync_state (key, value) VALUES ('schema_version', ?1)
              ON CONFLICT(key) DO UPDATE SET value = excluded.value",
@@ -725,3 +746,26 @@ UPDATE nodes
 
 DROP TABLE node_paths;
 ";
+
+/// V26: `pending_op.claimed_at` — the drain's claim column.
+///
+/// The drain was one thread taking one op at a time, so a 10 GiB upload held
+/// every queued rename and small write behind it for as long as it ran. Several
+/// workers can share the queue only if a claim is visible to the others: this
+/// column is what makes "already being drained by someone" a fact about the row
+/// rather than about one thread's local variable.
+///
+/// `0` is unclaimed; anything else is the ms at which a worker took it. A
+/// claim is process-local state that outlives nothing — the single-writer lock
+/// means a non-zero value found at open belongs to a crashed run, and
+/// [`Db::clear_op_claims`](super::Db::clear_op_claims) drops the lot.
+///
+/// The index is partial so it costs nothing on the ordinary row: only the
+/// handful of claimed rows are in it, which is exactly the set the claim query
+/// has to subtract to keep two workers off one node.
+const MIGRATION_V26: &str = "
+ALTER TABLE pending_op ADD COLUMN claimed_at INTEGER NOT NULL DEFAULT 0;
+
+CREATE INDEX IF NOT EXISTS idx_pending_op_claimed
+    ON pending_op(uid) WHERE claimed_at <> 0;
+";

crates/pdfs-core/src/db/mod.rs

@@ -230,7 +230,7 @@ impl Db {
             }
             Err(e) => return Err(e),
         };
-        Ok(Self {
+        let db = Self {
             conn: Mutex::new(conn),
             // No pool for `:memory:`. A second connection to that name is a
             // second, empty database rather than another view of this one, so
@@ -240,7 +240,17 @@ impl Db {
                 idle: Mutex::new(Vec::new()),
             }),
             _single_writer: single_writer,
-        })
+        };
+        // A drain claim names a worker thread in the process that took it, and
+        // the single-writer lock above says that process is not running. Any
+        // claim still on disk is therefore a crashed run's, and leaving it would
+        // hide those ops from every worker for good.
+        match db.clear_op_claims() {
+            Ok(0) => {}
+            Ok(n) => tracing::info!(ops = n, "released drain claims left by a previous run"),
+            Err(e) => tracing::warn!(error = %e, "clearing stale drain claims failed"),
+        }
+        Ok(db)
     }
 
     /// A connection for a `SELECT`-only method.

crates/pdfs-core/src/db/ops.rs

@@ -512,17 +512,115 @@ impl Db {
         Ok(op)
     }
 
+    /// Take the oldest due op for this worker, marking it claimed so no other
+    /// drain worker takes it, or `None` when there is nothing to do.
+    ///
+    /// The drain is several workers over one queue, which is only safe because
+    /// the claim is a column: a row is offered to exactly one worker, and the
+    /// worker that took it is the only one that may retire, defer or fail it.
+    ///
+    /// Two exclusions on top of [`next_due_op`](Self::next_due_op)'s:
+    ///
+    /// * `claimed_at = 0` — not already in flight somewhere else.
+    /// * no *other* claimed op shares this `uid`. Ordering only has to hold per
+    ///   node, but there it has to hold absolutely: a queued rename and a queued
+    ///   revision of one file must land in the order they were made, and two
+    ///   workers on one node would also race over its staged blob and its
+    ///   pending entry. `ORDER BY id` plus this exclusion gives per-uid serial
+    ///   order for free — the older op is claimed first and the newer one is
+    ///   invisible until it retires.
+    ///
+    /// Select and mark are one transaction, so two workers cannot both see the
+    /// row as unclaimed.
+    pub fn claim_next_due_op(&self, now: i64) -> Result<Option<PendingOp>> {
+        let mut conn = self.conn.lock();
+        let tx = conn.transaction()?;
+        let op = {
+            let mut stmt = tx.prepare_cached(&format!(
+                "SELECT id, kind, uid, parent_uid, name, blob_path, meta_json, created_at,
+                        attempts, last_error, next_attempt_at
+                 FROM pending_op
+                 WHERE next_attempt_at <= ?1
+                   AND claimed_at = 0
+                   AND (parent_uid IS NULL OR substr(parent_uid, 1, {n}) <> '{v}~')
+                   AND uid NOT IN (SELECT uid FROM pending_op WHERE claimed_at <> 0)
+                 ORDER BY id LIMIT 1",
+                v = LOCAL_VOLUME,
+                n = LOCAL_VOLUME.len() + 1,
+            ))?;
+            stmt.query_row(params![now], |r| {
+                Ok(PendingOp {
+                    id: r.get(0)?,
+                    kind: r.get(1)?,
+                    uid: r.get(2)?,
+                    parent_uid: r.get(3)?,
+                    name: r.get(4)?,
+                    blob_path: r.get(5)?,
+                    meta_json: r.get(6)?,
+                    created_at: r.get(7)?,
+                    attempts: r.get(8)?,
+                    last_error: r.get(9)?,
+                    next_attempt_at: r.get(10)?,
+                })
+            })
+            .optional()?
+        };
+        if let Some(op) = op.as_ref() {
+            tx.execute(
+                "UPDATE pending_op SET claimed_at = ?2 WHERE id = ?1",
+                params![op.id, now.max(1)],
+            )?;
+        }
+        tx.commit()?;
+        Ok(op)
+    }
+
+    /// Give a claimed op back to the queue, whatever became of the attempt.
+    ///
+    /// Called on every path out of a drain attempt rather than only the failing
+    /// ones: a handler that retires its own row leaves nothing to release (the
+    /// `UPDATE` matches nothing), and a handler that returns without retiring
+    /// *and* without failing would otherwise leave the row claimed by a worker
+    /// that has moved on — invisible to every worker, forever.
+    pub fn release_op_claim(&self, id: i64) -> Result<()> {
+        let conn = self.conn.lock();
+        conn.execute(
+            "UPDATE pending_op SET claimed_at = 0 WHERE id = ?1",
+            params![id],
+        )?;
+        Ok(())
+    }
+
+    /// Drop every claim in the table.
+    ///
+    /// A claim describes a worker in *this* process, so one surviving on disk
+    /// means the previous run died holding it. The single-writer lock
+    /// ([`Db::open`]) is what makes that inference safe: nobody else can be
+    /// draining this queue while we are opening it.
+    pub fn clear_op_claims(&self) -> Result<usize> {
+        let conn = self.conn.lock();
+        Ok(conn.execute(
+            "UPDATE pending_op SET claimed_at = 0 WHERE claimed_at <> 0",
+            [],
+        )?)
+    }
+
     /// The earliest `next_attempt_at` among all queued ops, or `None` if the
     /// queue is empty. Used by the drain loop to sleep exactly until the next
     /// debounced or backed-off op becomes eligible rather than waiting the full
     /// idle-poll interval.
+    ///
+    /// Claimed ops are excluded: another worker is already on them, so they are
+    /// not work this one is waiting for, and counting them would have an idle
+    /// worker spin on a row it cannot have.
     pub fn earliest_due_at(&self) -> Result<Option<i64>> {
         let conn = self.read();
         let ts: Option<i64> = conn
             .query_row(
                 &format!(
                     "SELECT MIN(next_attempt_at) FROM pending_op \
-                     WHERE parent_uid IS NULL OR substr(parent_uid, 1, {n}) <> '{v}~'",
+                     WHERE claimed_at = 0 \
+                       AND (parent_uid IS NULL OR substr(parent_uid, 1, {n}) <> '{v}~')",
                     v = LOCAL_VOLUME,
                     n = LOCAL_VOLUME.len() + 1,
                 ),

crates/pdfs-core/src/db/tests.rs

@@ -3545,3 +3545,114 @@ fn a_read_does_not_wait_for_the_write_connection() {
     let _ = std::fs::remove_file(&path);
     let _ = std::fs::remove_file(format!("{}.lock", path.display()));
 }
+
+/// §5: the drain is several workers over one queue, and the claim column is the
+/// only thing keeping them off each other's work. Two properties matter — no
+/// row is ever handed to two workers, and no *node* is ever worked by two
+/// workers even though its ops are separate rows.
+#[test]
+fn a_claimed_op_is_invisible_to_every_other_worker() {
+    let db = Db::open_in_memory().unwrap();
+    let root = uid("root").to_string();
+    for i in 0..3 {
+        db.enqueue_op(&bulk_op(i, &root)).unwrap();
+    }
+
+    let first = db.claim_next_due_op(10).unwrap().expect("an op is due");
+    let second = db
+        .claim_next_due_op(10)
+        .unwrap()
+        .expect("another op is due");
+    let third = db
+        .claim_next_due_op(10)
+        .unwrap()
+        .expect("a third op is due");
+    assert!(
+        db.claim_next_due_op(10).unwrap().is_none(),
+        "a fourth claim must find nothing left: every queued op is taken"
+    );
+
+    let mut ids = [first.id, second.id, third.id];
+    ids.sort_unstable();
+    assert_eq!(
+        ids,
+        [1, 2, 3],
+        "each worker must get a distinct row, oldest first"
+    );
+
+    // Releasing puts one back, and it is the one that comes out next.
+    db.release_op_claim(second.id).unwrap();
+    let again = db.claim_next_due_op(10).unwrap().expect("the released op");
+    assert_eq!(again.id, second.id);
+}
+
+#[test]
+fn two_workers_never_share_a_node() {
+    let db = Db::open_in_memory().unwrap();
+    let root = uid("root").to_string();
+    // Two ops of different kinds against one node — a queued rename and a
+    // queued revision, which is an ordinary `mv` plus a save.
+    let mut revision = bulk_op(0, &root);
+    revision.parent_uid = None;
+    db.enqueue_op(&revision).unwrap();
+    let rename = PendingOp {
+        kind: OP_RENAME.to_string(),
+        parent_uid: Some(root.clone()),
+        blob_path: None,
+        meta_json: None,
+        ..bulk_op(0, &root)
+    };
+    db.enqueue_op(&rename).unwrap();
+
+    let claimed = db.claim_next_due_op(10).unwrap().expect("an op is due");
+    assert_eq!(claimed.kind, OP_REVISION, "the older op goes first");
+    assert!(
+        db.claim_next_due_op(10).unwrap().is_none(),
+        "the second op targets the same node, so it must wait for the first: \
+         two workers on one uid would race over its staged blob and land its \
+         changes out of order"
+    );
+
+    db.release_op_claim(claimed.id).unwrap();
+    db.delete_op(claimed.id).unwrap();
+    assert_eq!(
+        db.claim_next_due_op(10).unwrap().map(|o| o.kind),
+        Some(OP_RENAME.to_string()),
+        "once the revision retires, the rename is claimable"
+    );
+}
+
+#[test]
+fn claims_do_not_survive_the_process_that_took_them() {
+    let db = Db::open_in_memory().unwrap();
+    let root = uid("root").to_string();
+    for i in 0..2 {
+        db.enqueue_op(&bulk_op(i, &root)).unwrap();
+    }
+    db.claim_next_due_op(10).unwrap().unwrap();
+    db.claim_next_due_op(10).unwrap().unwrap();
+    assert!(db.claim_next_due_op(10).unwrap().is_none());
+
+    // What `Db::open` does after the single-writer lock says the previous run
+    // is gone. Without it those ops are invisible to every worker forever.
+    assert_eq!(db.clear_op_claims().unwrap(), 2);
+    assert!(
+        db.claim_next_due_op(10).unwrap().is_some(),
+        "a crashed run's claims must not park its queue"
+    );
+}
+
+#[test]
+fn an_idle_worker_does_not_wait_on_an_op_another_worker_has() {
+    let db = Db::open_in_memory().unwrap();
+    let root = uid("root").to_string();
+    db.enqueue_op(&bulk_op(0, &root)).unwrap();
+
+    assert_eq!(db.earliest_due_at().unwrap(), Some(0));
+    db.claim_next_due_op(10).unwrap().unwrap();
+    assert_eq!(
+        db.earliest_due_at().unwrap(),
+        None,
+        "an op somebody else is draining is not work this worker is waiting for"
+    );
+}