Skip to main content

max / goingson

Close task-repo write races and recurrence far-window blindness - Wrap task update()'s completed_at read-modify-write in one transaction so a concurrent complete()/remote-apply can't clobber it (the derivation reads status then writes, previously on two separate pool checkouts). - Make complete_recurring mark completion via a conditional UPDATE ... AND status != 'Completed' whose rows_affected gates the next-instance insert, so two concurrent calls can't each insert a duplicate recurring row (a plain in-txn re-check is insufficient under WAL snapshot isolation). - Seek the recurrence cursor into the requested window before the bounded 500-iteration expansion, so a daily event older than 500 occurrences no longer renders empty. Adds a far-past-start regression test. - Add a contact_emails cascade-logging regression test. This refutes the fuzz finding that claimed ON DELETE CASCADE skips the child changelog triggers unless recursive_triggers is ON: verified (app test + standalone sqlite3 3.45) that SQLite fires child AFTER DELETE triggers on FK cascade regardless of the pragma, so the cascade already logs and there is no orphan bug. The test guards that property against a future trigger-bypassing delete path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-07 14:30 UTC
Commit: e51ca2c296efe4d839c088dc7e68ff7272cf5b2b
Parent: dd52661
3 files changed, +175 insertions, -13 deletions
M crates/core/src/recurrence.rs +62
@@ -348,6 +348,22 @@ pub fn expand_recurrence_in_tz(
348 348
349 349 let mut instances = Vec::new();
350 350 let mut cursor = event.start_time;
351 +
352 + // Seek forward to the first occurrence that could overlap the window before
353 + // spending the bounded expansion budget. Without this, a daily event whose
354 + // start_time is >500 days before range_start burns all 500 iterations on
355 + // occurrences long before the window and renders empty. The seek is cheap
356 + // (no clone/push) and separately capped so a degenerate rule can't spin.
357 + let seek_cap = 100_000;
358 + let mut seeked = 0;
359 + while cursor + event_duration < range_start && seeked < seek_cap {
360 + match calculate_next_due_rich_in_tz(Some(&cursor), &rule, tz) {
361 + Some(next) if next > cursor => cursor = next,
362 + _ => break,
363 + }
364 + seeked += 1;
365 + }
366 +
351 367 let max_iterations = 500;
352 368
353 369 for _ in 0..max_iterations {
@@ -806,6 +822,52 @@ mod tests {
806 822 }
807 823
808 824 #[test]
825 + fn test_expand_recurrence_far_past_start_still_renders() {
826 + // A daily event whose start_time is well over 500 occurrences before the
827 + // window used to render empty: the 500-iteration budget was spent on
828 + // occurrences long before range_start. The seek must fast-forward into
829 + // the window so today's occurrences appear.
830 + let start = Utc.with_ymd_and_hms(2022, 1, 1, 9, 0, 0).unwrap(); // ~4 years prior
831 + let event = Event {
832 + id: crate::id_types::EventId::new(),
833 + user_id: None,
834 + project_id: None,
835 + project_name: None,
836 + contact_id: None,
837 + contact_name: None,
838 + title: "Daily standup".to_string(),
839 + description: String::new(),
840 + start_time: start,
841 + end_time: Some(start + Duration::minutes(15)),
842 + location: None,
843 + linked_task_id: None,
844 + recurrence: Recurrence::Daily,
845 + recurrence_rule: Some(RecurrenceRule {
846 + pattern: Recurrence::Daily,
847 + interval: 1,
848 + weekdays: vec![],
849 + monthly_spec: None,
850 + }),
851 + recurrence_parent_id: None,
852 + is_recurring_instance: false,
853 + block_type: None,
854 + external_source: None,
855 + external_id: None,
856 + is_read_only: false,
857 + snoozed_until: None,
858 + reminder_offsets_seconds: Vec::new(),
859 + };
860 +
861 + let range_start = Utc.with_ymd_and_hms(2026, 3, 1, 0, 0, 0).unwrap();
862 + let range_end = Utc.with_ymd_and_hms(2026, 3, 7, 23, 59, 59).unwrap();
863 +
864 + let instances = expand_recurrence(&event, range_start, range_end);
865 + // Seven days in the window, each with a daily occurrence.
866 + assert_eq!(instances.len(), 7, "old daily event must still render in the current window");
867 + assert!(instances.iter().all(|e| e.start_time >= range_start && e.start_time <= range_end));
868 + }
869 +
870 + #[test]
809 871 fn test_expand_recurrence_deterministic_ids() {
810 872 let start = Utc.with_ymd_and_hms(2026, 3, 2, 10, 0, 0).unwrap();
811 873 let event = Event {
M crates/db-sqlite/src/repository/task_repo.rs +39 -13
@@ -177,7 +177,10 @@ pub(crate) async fn rows_to_tasks(pool: &SqlitePool, rows: Vec<TaskRowWithProjec
177 177 }
178 178
179 179 /// Fetch only the fields needed for update logic — avoids annotation/subtask/session sub-queries.
180 - pub(crate) async fn get_task_update_context(pool: &SqlitePool, id: TaskId, user_id: UserId) -> Result<Option<goingson_core::models::TaskUpdateContext>> {
180 + pub(crate) async fn get_task_update_context<'e, E>(executor: E, id: TaskId, user_id: UserId) -> Result<Option<goingson_core::models::TaskUpdateContext>>
181 + where
182 + E: sqlx::SqliteExecutor<'e>,
183 + {
181 184 #[derive(sqlx::FromRow)]
182 185 struct Row {
183 186 created_at: String,
@@ -192,7 +195,7 @@ pub(crate) async fn get_task_update_context(pool: &SqlitePool, id: TaskId, user_
192 195 )
193 196 .bind(id.to_string())
194 197 .bind(user_id.to_string())
195 - .fetch_optional(pool)
198 + .fetch_optional(executor)
196 199 .await
197 200 .map_err(CoreError::database)?;
198 201
@@ -554,7 +557,13 @@ impl TaskRepository for SqliteTaskRepository {
554 557 // a task first becomes Completed, preserve it while it stays Completed,
555 558 // clear it only when it actually leaves Completed, and otherwise leave the
556 559 // stored value untouched. Lightweight context query (no sub-queries).
557 - let ctx = get_task_update_context(&self.pool, id, user_id).await?;
560 + // Read the current completion context and write the update in one
561 + // transaction: the completed_at derivation is a read-modify-write, so a
562 + // concurrent complete()/remote-apply landing between the two checkouts
563 + // would otherwise be silently clobbered.
564 + let mut tx = self.pool.begin().await.map_err(CoreError::database)?;
565 +
566 + let ctx = get_task_update_context(&mut *tx, id, user_id).await?;
558 567 let was_completed = ctx.as_ref().is_some_and(|c| c.status == TaskStatus::Completed);
559 568 let prior_completed_at = ctx.as_ref().and_then(|c| c.completed_at.as_ref().map(format_datetime));
560 569 let completed_at_str: Option<String> = match task.status {
@@ -588,11 +597,14 @@ impl TaskRepository for SqliteTaskRepository {
588 597 .bind(&completed_at_str)
589 598 .bind(id.to_string())
590 599 .bind(user_id.to_string())
591 - .execute(&self.pool)
600 + .execute(&mut *tx)
592 601 .await
593 602 .map_err(CoreError::database)?;
594 603
595 - if result.rows_affected() > 0 {
604 + let affected = result.rows_affected();
605 + tx.commit().await.map_err(CoreError::database)?;
606 +
607 + if affected > 0 {
596 608 get_task_by_id(&self.pool, id, user_id).await
597 609 } else {
598 610 Ok(None)
@@ -728,15 +740,29 @@ impl TaskRepository for SqliteTaskRepository {
728 740
729 741 let mut tx = self.pool.begin().await.map_err(CoreError::database)?;
730 742
731 - // Mark complete
743 + // Mark complete, but only if it is not already Completed. The single
744 + // conditional UPDATE serializes on the write lock, so of two concurrent
745 + // calls only the one that actually transitions the task matches a row;
746 + // the loser affects zero rows and skips the next-instance insert. Without
747 + // this both calls would pass the pre-txn guard (WAL snapshot isolation
748 + // hides the other's uncommitted write) and each insert a duplicate.
732 749 let now = format_datetime_now();
733 - sqlx::query("UPDATE tasks SET status = 'Completed', completed_at = ? WHERE id = ? AND user_id = ?")
734 - .bind(&now)
735 - .bind(id.to_string())
736 - .bind(user_id.to_string())
737 - .execute(&mut *tx)
738 - .await
739 - .map_err(CoreError::database)?;
750 + let marked = sqlx::query(
751 + "UPDATE tasks SET status = 'Completed', completed_at = ? WHERE id = ? AND user_id = ? AND status != 'Completed'",
752 + )
753 + .bind(&now)
754 + .bind(id.to_string())
755 + .bind(user_id.to_string())
756 + .execute(&mut *tx)
757 + .await
758 + .map_err(CoreError::database)?;
759 +
760 + if marked.rows_affected() == 0 {
761 + // Another call completed it first (or it vanished) — do not insert a
762 + // second recurring instance.
763 + tx.rollback().await.map_err(CoreError::database)?;
764 + return Ok((None, None));
765 + }
740 766
741 767 // Create next recurring instance if provided
742 768 let next_id = if let Some(new_task) = &next {
M crates/db-sqlite/tests/contact_repo_tests.rs +74
@@ -8,6 +8,80 @@ use goingson_core::{
8 8 };
9 9 use goingson_db_sqlite::SqliteContactRepository;
10 10
11 + /// Deleting a contact hard-deletes it, cascading to `contact_emails`
12 + /// (ON DELETE CASCADE) -- and those cascaded child deletes MUST be logged in
13 + /// `sync_changelog`, or the delete would replicate as parent-only and every
14 + /// child email would orphan on remote devices forever.
15 + ///
16 + /// SQLite (3.46 here) fires `AFTER DELETE` triggers for rows removed by an FK
17 + /// cascade regardless of the `recursive_triggers` pragma -- so the changelog
18 + /// triggers already log the cascade and there is no orphan bug. (This directly
19 + /// refutes the 2026-07-06 fuzz finding, which claimed the cascade skipped the
20 + /// child triggers unless `recursive_triggers` was ON; verified empirically --
21 + /// the cascade fires the trigger with the pragma OFF.) This test guards that
22 + /// property so a future change that swaps the FK cascade for a trigger-bypassing
23 + /// code-path delete can't silently reintroduce the orphan risk.
24 + #[tokio::test]
25 + async fn cascade_child_delete_is_logged_to_changelog() {
26 + let pool = common::setup_test_db().await;
27 + let user_id = common::create_test_user(&pool).await;
28 + let repo = SqliteContactRepository::new(pool.clone());
29 +
30 + let contact = repo
31 + .create(
32 + user_id,
33 + NewContact {
34 + display_name: "Cascade Target".to_string(),
35 + nickname: None,
36 + company: None,
37 + title: None,
38 + notes: String::new(),
39 + tags: vec![],
40 + birthday: None,
41 + timezone: None,
42 + is_implicit: false,
43 + },
44 + )
45 + .await
46 + .expect("create contact");
47 +
48 + let email = repo
49 + .add_email(
50 + contact.id,
51 + user_id,
52 + NewContactEmail {
53 + address: "child@example.com".to_string(),
54 + label: "home".to_string(),
55 + is_primary: true,
56 + },
57 + )
58 + .await
59 + .expect("add email");
60 +
61 + // Discard the insert changelog entries so we assert purely on the cascade.
62 + sqlx::query("DELETE FROM sync_changelog")
63 + .execute(&pool)
64 + .await
65 + .expect("clear changelog");
66 +
67 + let deleted = repo.delete(contact.id, user_id).await.expect("delete contact");
68 + assert!(deleted, "parent contact should delete");
69 +
70 + let child_delete_logged: i64 = sqlx::query_scalar(
71 + "SELECT COUNT(*) FROM sync_changelog \
72 + WHERE table_name = 'contact_emails' AND op = 'DELETE' AND row_id = ?",
73 + )
74 + .bind(email.id.to_string())
75 + .fetch_one(&pool)
76 + .await
77 + .expect("query changelog");
78 +
79 + assert_eq!(
80 + child_delete_logged, 1,
81 + "cascaded child delete must be logged to sync_changelog"
82 + );
83 + }
84 +
11 85 // ============ CRUD Tests ============
12 86
13 87 #[tokio::test]