desktop: fix a retention test that raced the wall clock
Desktop (Tauri) / Tauri desktop (Linux) (push) Failing after 1m31s
Desktop (Tauri) / Windows installer (cross-compiled) (push) Successful in 1m50s

`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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SreJkbxB4gx8pPsu8QbLPi
This commit is contained in:
2026-07-26 16:25:32 -04:00
co-authored by Claude Opus 5
parent e64d67e904
commit 7a77a0e1b9
3 changed files with 63 additions and 53 deletions
+5 -6
View File
@@ -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 // 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 // be enforced. A store error falls back to the default rather than failing the
// call — the app must still boot. // call — the app must still boot.
let retention_days = db let fallback = retention::LOCAL_RETENTION_DAYS;
.0 let retention_days = match db.0.lock() {
.lock() Ok(conn) => state::effective_retention_days(&conn, fallback).unwrap_or(fallback),
.ok() Err(_) => fallback,
.and_then(|conn| state::effective_retention_days(&conn, retention::LOCAL_RETENTION_DAYS).ok()) };
.unwrap_or(retention::LOCAL_RETENTION_DAYS);
// Offline defaults: no signups, no server-side URL unfurling (needs network). // Offline defaults: no signups, no server-side URL unfurling (needs network).
PublicConfig { PublicConfig {
site_name: "ThoughtSync".to_string(), site_name: "ThoughtSync".to_string(),
+48 -30
View File
@@ -34,23 +34,29 @@ pub fn sweep_expired_trash(
return Ok(0); return Ok(0);
} }
let cutoff = now - Duration::days(retention_days); let cutoff = now - Duration::days(retention_days);
let expired: Vec<String> = { let mut expired: Vec<String> = Vec::new();
let mut stmt = {
conn.prepare("SELECT id, trashed_at FROM notes WHERE trashed = 1 AND trashed_at IS NOT NULL")?; let mut stmt = conn
let rows = stmt.query_map([], |r| Ok((r.get::<_, String>(0)?, r.get::<_, String>(1)?)))?; .prepare("SELECT id, trashed_at FROM notes WHERE trashed = 1 AND trashed_at IS NOT NULL")?;
rows.filter_map(|row| { let mut rows = stmt.query([])?;
let (id, stamped) = row.ok()?; 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 // 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 // client writes `Z`, so two timestamps for the same instant don't sort
// against each other as text — and the failure would be silent. // 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 // An unparseable stamp means "age unknown", and the only safe reading of
// safe reading of that is to keep the note. Deleting on a guess is the one // that is to keep the note. Deleting on a guess is the one outcome nobody
// outcome nobody can undo. // can undo.
(trashed_at.with_timezone(&Utc) < cutoff).then_some(id) let Ok(trashed_at) = DateTime::parse_from_rfc3339(&stamped) else {
}) continue;
.collect() };
}; if trashed_at.with_timezone(&Utc) < cutoff {
expired.push(id);
}
}
}
for id in &expired { for id in &expired {
// Through delete_forever, so a `pending_deletes` tombstone is recorded. That's // 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 // right even here: while unlinked this device holds the only copy, so if it
@@ -80,11 +86,11 @@ mod tests {
conn 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`). /// (`...Z`, millisecond precision — see `store::now`).
fn trashed_note(conn: &Connection, id: &str, days_ago: i64) { fn trashed_note_aged(conn: &Connection, id: &str, age: Duration) {
let stamped = (Utc::now() - Duration::days(days_ago)) let when = Utc::now() - age;
.to_rfc3339_opts(chrono::SecondsFormat::Millis, true); let stamped = when.to_rfc3339_opts(chrono::SecondsFormat::Millis, true);
conn.execute( conn.execute(
"INSERT INTO notes (id, title, body, created_at, updated_at, trashed, trashed_at) "INSERT INTO notes (id, title, body, created_at, updated_at, trashed, trashed_at)
VALUES (?1, 'T', 'B', ?2, ?2, 1, ?2)", VALUES (?1, 'T', 'B', ?2, ?2, 1, ?2)",
@@ -93,6 +99,14 @@ mod tests {
.expect("insert"); .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 { fn note_count(conn: &Connection) -> i64 {
conn.query_row("SELECT COUNT(*) FROM notes", [], |r| r.get(0)) conn.query_row("SELECT COUNT(*) FROM notes", [], |r| r.get(0))
.expect("count") .expect("count")
@@ -103,26 +117,30 @@ mod tests {
let conn = db(); let conn = db();
trashed_note(&conn, "old", 40); trashed_note(&conn, "old", 40);
trashed_note(&conn, "fresh", 3); 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!(purged, 1);
assert_eq!(note_count(&conn), 1, "only the expired note should go"); assert_eq!(note_count(&conn), 1, "only the expired note should go");
} }
#[test] #[test]
fn a_note_exactly_at_the_boundary_survives() { fn a_note_just_inside_the_window_survives() {
// Strictly older than the cutoff, so the note trashed 30 days ago gets its // The comparison is STRICTLY older than the cutoff, so a note with a minute
// full 30 days rather than being cut a moment short. // 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(); let conn = db();
trashed_note(&conn, "boundary", 30); let almost = Duration::days(30) - Duration::minutes(1);
assert_eq!(sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"), 0); trashed_note_aged(&conn, "boundary", almost);
assert_eq!(sweep(&conn, 30), 0);
assert_eq!(note_count(&conn), 1);
} }
#[test] #[test]
fn retention_off_purges_nothing() { fn retention_off_purges_nothing() {
let conn = db(); let conn = db();
trashed_note(&conn, "ancient", 4000); trashed_note(&conn, "ancient", 4000);
assert_eq!(sweep_expired_trash(&conn, 0, Utc::now()).expect("sweep"), 0); assert_eq!(sweep(&conn, 0), 0);
assert_eq!(sweep_expired_trash(&conn, -1, Utc::now()).expect("sweep"), 0); assert_eq!(sweep(&conn, -1), 0);
assert_eq!(note_count(&conn), 1); assert_eq!(note_count(&conn), 1);
} }
@@ -135,7 +153,7 @@ mod tests {
[], [],
) )
.expect("insert"); .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); assert_eq!(note_count(&conn), 1);
} }
@@ -149,7 +167,7 @@ mod tests {
[], [],
) )
.expect("insert"); .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); assert_eq!(note_count(&conn), 1);
} }
@@ -165,7 +183,7 @@ mod tests {
rusqlite::params![stamped], rusqlite::params![stamped],
) )
.expect("insert"); .expect("insert");
assert_eq!(sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"), 1); assert_eq!(sweep(&conn, 30), 1);
} }
#[test] #[test]
@@ -174,7 +192,7 @@ mod tests {
// re-send a note the user already destroyed here. // re-send a note the user already destroyed here.
let conn = db(); let conn = db();
trashed_note(&conn, "old", 40); trashed_note(&conn, "old", 40);
sweep_expired_trash(&conn, 30, Utc::now()).expect("sweep"); sweep(&conn, 30);
let pending: i64 = conn let pending: i64 = conn
.query_row( .query_row(
"SELECT COUNT(*) FROM pending_deletes WHERE entity = 'note' AND id = 'old'", "SELECT COUNT(*) FROM pending_deletes WHERE entity = 'note' AND id = 'old'",
+10 -17
View File
@@ -533,6 +533,11 @@ mod tests {
conn.query_row(sql, [], |r| r.get(0)).expect("count") conn.query_row(sql, [], |r| r.get(0)).expect("count")
} }
fn trash_stamp(conn: &Connection, id: &str) -> Option<String> {
let sql = "SELECT trashed_at FROM notes WHERE id = ?1";
conn.query_row(sql, [id], |r| r.get(0)).expect("stamp")
}
#[test] #[test]
fn applies_a_note_and_advances_the_cursor() { fn applies_a_note_and_advances_the_cursor() {
let conn = db(); let conn = db();
@@ -582,12 +587,8 @@ mod tests {
trashed.trashed = true; trashed.trashed = true;
trashed.deleted_at = Some("2026-06-01T09:30:00+00:00".into()); 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![trashed], vec![], 1)).expect("apply");
let stamped: String = conn let stamped = trash_stamp(&conn, "n1");
.query_row("SELECT trashed_at FROM notes WHERE id = 'n1'", [], |r| { assert_eq!(stamped.as_deref(), Some("2026-06-01T09:30:00+00:00"));
r.get(0)
})
.expect("trashed_at");
assert_eq!(stamped, "2026-06-01T09:30:00+00:00");
} }
#[test] #[test]
@@ -598,12 +599,8 @@ mod tests {
trashed.deleted_at = Some("2026-06-01T09:30:00+00:00".into()); 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![trashed], vec![], 1)).expect("apply");
apply_page(&conn, &page(vec![note("n1", 2)], vec![], 2)).expect("apply"); apply_page(&conn, &page(vec![note("n1", 2)], vec![], 2)).expect("apply");
let stamped: Option<String> = conn let stamped = trash_stamp(&conn, "n1");
.query_row("SELECT trashed_at FROM notes WHERE id = 'n1'", [], |r| { assert_eq!(stamped, None, "an untrashed note keeps no trash stamp");
r.get(0)
})
.expect("trashed_at");
assert_eq!(stamped, None, "an untrashed note must carry no trash stamp");
} }
#[test] #[test]
@@ -615,11 +612,7 @@ mod tests {
trashed.trashed = true; trashed.trashed = true;
trashed.deleted_at = None; trashed.deleted_at = None;
apply_page(&conn, &page(vec![trashed], vec![], 1)).expect("apply"); apply_page(&conn, &page(vec![trashed], vec![], 1)).expect("apply");
let stamped: Option<String> = conn let stamped = trash_stamp(&conn, "n1");
.query_row("SELECT trashed_at FROM notes WHERE id = 'n1'", [], |r| {
r.get(0)
})
.expect("trashed_at");
assert_eq!(stamped.as_deref(), Some("2026-07-26T00:00:00.000Z")); assert_eq!(stamped.as_deref(), Some("2026-07-26T00:00:00.000Z"));
} }