From 7a77a0e1b949b0d55a1cc05f7456360f1e9e017e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 26 Jul 2026 16:25:32 -0400 Subject: [PATCH] desktop: fix a retention test that raced the wall clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `a_note_exactly_at_the_boundary_survives` stamped a note 30 days ago and then asked the sweep — which reads `now` microseconds later — whether it was strictly older than 30 days. It was, by those microseconds. The assertion was wrong, not the code: an exact tie isn't observable against a wall clock. Now stamps a note with a minute of its window still to run, which is the property actually worth pinning: the comparison is strictly-older, so a note inside the window is kept. Also rewrote the row scan as plain statements. The `filter_map` over `query_map` swallowed real rusqlite errors through `.ok()?` on the way to skipping unparseable timestamps — the two cases deserve different treatment, and only the second should be silent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SreJkbxB4gx8pPsu8QbLPi --- desktop/src-tauri/src/local/commands.rs | 11 ++-- desktop/src-tauri/src/local/retention.rs | 78 +++++++++++++++--------- desktop/src-tauri/src/sync/pull.rs | 27 +++----- 3 files changed, 63 insertions(+), 53 deletions(-) diff --git a/desktop/src-tauri/src/local/commands.rs b/desktop/src-tauri/src/local/commands.rs index 338f74e..312185b 100644 --- a/desktop/src-tauri/src/local/commands.rs +++ b/desktop/src-tauri/src/local/commands.rs @@ -22,12 +22,11 @@ pub fn config_get(db: State<'_, Db>) -> PublicConfig { // default is what keeps the deadline on screen equal to the one that will actually // be enforced. A store error falls back to the default rather than failing the // call — the app must still boot. - let retention_days = db - .0 - .lock() - .ok() - .and_then(|conn| state::effective_retention_days(&conn, retention::LOCAL_RETENTION_DAYS).ok()) - .unwrap_or(retention::LOCAL_RETENTION_DAYS); + let fallback = retention::LOCAL_RETENTION_DAYS; + let retention_days = match db.0.lock() { + Ok(conn) => state::effective_retention_days(&conn, fallback).unwrap_or(fallback), + Err(_) => fallback, + }; // Offline defaults: no signups, no server-side URL unfurling (needs network). PublicConfig { site_name: "ThoughtSync".to_string(), diff --git a/desktop/src-tauri/src/local/retention.rs b/desktop/src-tauri/src/local/retention.rs index 0f090bf..ba6600a 100644 --- a/desktop/src-tauri/src/local/retention.rs +++ b/desktop/src-tauri/src/local/retention.rs @@ -34,23 +34,29 @@ pub fn sweep_expired_trash( return Ok(0); } let cutoff = now - Duration::days(retention_days); - let expired: Vec = { - let mut stmt = - conn.prepare("SELECT id, trashed_at FROM notes WHERE trashed = 1 AND trashed_at IS NOT NULL")?; - let rows = stmt.query_map([], |r| Ok((r.get::<_, String>(0)?, r.get::<_, String>(1)?)))?; - rows.filter_map(|row| { - let (id, stamped) = row.ok()?; + let mut expired: Vec = Vec::new(); + { + let mut stmt = conn + .prepare("SELECT id, trashed_at FROM notes WHERE trashed = 1 AND trashed_at IS NOT NULL")?; + let mut rows = stmt.query([])?; + while let Some(row) = rows.next()? { + let id: String = row.get(0)?; + let stamped: String = row.get(1)?; // PARSED, not string-compared. The server writes `+00:00` offsets and this // client writes `Z`, so two timestamps for the same instant don't sort // against each other as text — and the failure would be silent. - let trashed_at = DateTime::parse_from_rfc3339(&stamped).ok()?; - // An unparseable or missing timestamp means "age unknown", and the only - // safe reading of that is to keep the note. Deleting on a guess is the one - // outcome nobody can undo. - (trashed_at.with_timezone(&Utc) < cutoff).then_some(id) - }) - .collect() - }; + // + // An unparseable stamp means "age unknown", and the only safe reading of + // that is to keep the note. Deleting on a guess is the one outcome nobody + // can undo. + let Ok(trashed_at) = DateTime::parse_from_rfc3339(&stamped) else { + continue; + }; + if trashed_at.with_timezone(&Utc) < cutoff { + expired.push(id); + } + } + } for id in &expired { // Through delete_forever, so a `pending_deletes` tombstone is recorded. That's // right even here: while unlinked this device holds the only copy, so if it @@ -80,11 +86,11 @@ mod tests { conn } - /// A trashed note stamped `trashed_at` days ago, in the format the CLIENT writes + /// A trashed note of a given age, stamped in the format the CLIENT writes /// (`...Z`, millisecond precision — see `store::now`). - fn trashed_note(conn: &Connection, id: &str, days_ago: i64) { - let stamped = (Utc::now() - Duration::days(days_ago)) - .to_rfc3339_opts(chrono::SecondsFormat::Millis, true); + fn trashed_note_aged(conn: &Connection, id: &str, age: Duration) { + let when = Utc::now() - age; + let stamped = when.to_rfc3339_opts(chrono::SecondsFormat::Millis, true); conn.execute( "INSERT INTO notes (id, title, body, created_at, updated_at, trashed, trashed_at) VALUES (?1, 'T', 'B', ?2, ?2, 1, ?2)", @@ -93,6 +99,14 @@ mod tests { .expect("insert"); } + fn trashed_note(conn: &Connection, id: &str, days_ago: i64) { + trashed_note_aged(conn, id, Duration::days(days_ago)); + } + + fn sweep(conn: &Connection, days: i64) -> usize { + sweep_expired_trash(conn, days, Utc::now()).expect("sweep") + } + fn note_count(conn: &Connection) -> i64 { conn.query_row("SELECT COUNT(*) FROM notes", [], |r| r.get(0)) .expect("count") @@ -103,26 +117,30 @@ mod tests { let conn = db(); trashed_note(&conn, "old", 40); trashed_note(&conn, "fresh", 3); - let purged = sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"); + let purged = sweep(&conn, 30); assert_eq!(purged, 1); assert_eq!(note_count(&conn), 1, "only the expired note should go"); } #[test] - fn a_note_exactly_at_the_boundary_survives() { - // Strictly older than the cutoff, so the note trashed 30 days ago gets its - // full 30 days rather than being cut a moment short. + fn a_note_just_inside_the_window_survives() { + // The comparison is STRICTLY older than the cutoff, so a note with a minute + // of its 30 days still to run is kept. An exact tie isn't testable against a + // wall clock — the sweep reads `now` microseconds after the row is stamped, + // which is precisely how the first version of this test failed. let conn = db(); - trashed_note(&conn, "boundary", 30); - assert_eq!(sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"), 0); + let almost = Duration::days(30) - Duration::minutes(1); + trashed_note_aged(&conn, "boundary", almost); + assert_eq!(sweep(&conn, 30), 0); + assert_eq!(note_count(&conn), 1); } #[test] fn retention_off_purges_nothing() { let conn = db(); trashed_note(&conn, "ancient", 4000); - assert_eq!(sweep_expired_trash(&conn, 0, Utc::now()).expect("sweep"), 0); - assert_eq!(sweep_expired_trash(&conn, -1, Utc::now()).expect("sweep"), 0); + assert_eq!(sweep(&conn, 0), 0); + assert_eq!(sweep(&conn, -1), 0); assert_eq!(note_count(&conn), 1); } @@ -135,7 +153,7 @@ mod tests { [], ) .expect("insert"); - assert_eq!(sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"), 0); + assert_eq!(sweep(&conn, 30), 0); assert_eq!(note_count(&conn), 1); } @@ -149,7 +167,7 @@ mod tests { [], ) .expect("insert"); - assert_eq!(sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"), 0); + assert_eq!(sweep(&conn, 30), 0); assert_eq!(note_count(&conn), 1); } @@ -165,7 +183,7 @@ mod tests { rusqlite::params![stamped], ) .expect("insert"); - assert_eq!(sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"), 1); + assert_eq!(sweep(&conn, 30), 1); } #[test] @@ -174,7 +192,7 @@ mod tests { // re-send a note the user already destroyed here. let conn = db(); trashed_note(&conn, "old", 40); - sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"); + sweep(&conn, 30); let pending: i64 = conn .query_row( "SELECT COUNT(*) FROM pending_deletes WHERE entity = 'note' AND id = 'old'", diff --git a/desktop/src-tauri/src/sync/pull.rs b/desktop/src-tauri/src/sync/pull.rs index 0e89224..3b33c85 100644 --- a/desktop/src-tauri/src/sync/pull.rs +++ b/desktop/src-tauri/src/sync/pull.rs @@ -533,6 +533,11 @@ mod tests { conn.query_row(sql, [], |r| r.get(0)).expect("count") } + fn trash_stamp(conn: &Connection, id: &str) -> Option { + let sql = "SELECT trashed_at FROM notes WHERE id = ?1"; + conn.query_row(sql, [id], |r| r.get(0)).expect("stamp") + } + #[test] fn applies_a_note_and_advances_the_cursor() { let conn = db(); @@ -582,12 +587,8 @@ mod tests { trashed.trashed = true; trashed.deleted_at = Some("2026-06-01T09:30:00+00:00".into()); apply_page(&conn, &page(vec![trashed], vec![], 1)).expect("apply"); - let stamped: String = conn - .query_row("SELECT trashed_at FROM notes WHERE id = 'n1'", [], |r| { - r.get(0) - }) - .expect("trashed_at"); - assert_eq!(stamped, "2026-06-01T09:30:00+00:00"); + let stamped = trash_stamp(&conn, "n1"); + assert_eq!(stamped.as_deref(), Some("2026-06-01T09:30:00+00:00")); } #[test] @@ -598,12 +599,8 @@ mod tests { trashed.deleted_at = Some("2026-06-01T09:30:00+00:00".into()); apply_page(&conn, &page(vec![trashed], vec![], 1)).expect("apply"); apply_page(&conn, &page(vec![note("n1", 2)], vec![], 2)).expect("apply"); - let stamped: Option = conn - .query_row("SELECT trashed_at FROM notes WHERE id = 'n1'", [], |r| { - r.get(0) - }) - .expect("trashed_at"); - assert_eq!(stamped, None, "an untrashed note must carry no trash stamp"); + let stamped = trash_stamp(&conn, "n1"); + assert_eq!(stamped, None, "an untrashed note keeps no trash stamp"); } #[test] @@ -615,11 +612,7 @@ mod tests { trashed.trashed = true; trashed.deleted_at = None; apply_page(&conn, &page(vec![trashed], vec![], 1)).expect("apply"); - let stamped: Option = conn - .query_row("SELECT trashed_at FROM notes WHERE id = 'n1'", [], |r| { - r.get(0) - }) - .expect("trashed_at"); + let stamped = trash_stamp(&conn, "n1"); assert_eq!(stamped.as_deref(), Some("2026-07-26T00:00:00.000Z")); }