Skip to main content

max / goingson

Storage remediation: atomic position + upsert, friendly timer-race error Ultra-fuzz Run #28 Phase 3, three concurrency fixes: - subtask add: compute position inside an INSERT ... SELECT COALESCE(MAX,-1)+1 ... RETURNING instead of a separate SELECT MAX then INSERT, closing the duplicate-position race. Both add paths. - daily_notes upsert: single INSERT ... ON CONFLICT(user_id, note_date) DO UPDATE instead of a non-transactional get-then-insert/update, then read the canonical row back. No more raw UNIQUE violation on a concurrent same-day write. - start_timer: map the migration-053 partial-unique-index backstop violation to the friendly "A timer is already running" message instead of a raw db error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-19 19:58 UTC
Commit: 7e48a4aa420f3d1d9131b0bbf222570eba349d3e
Parent: 15b1025
3 files changed, +59 insertions, -78 deletions
M crates/db-sqlite/src/repository/daily_note_repo.rs +33 -50
@@ -102,56 +102,39 @@ impl DailyNoteRepository for SqliteDailyNoteRepository {
102 102 let now_str = format_datetime(&now);
103 103 let reviewed_at_str = if is_reviewed { Some(now_str.clone()) } else { None };
104 104
105 - let existing = self.get_by_date(user_id, date).await?;
106 -
107 - let (id, created_at) = if let Some(ref existing) = existing {
108 - sqlx::query(
109 - "UPDATE daily_notes SET went_well = ?, could_improve = ?, is_reviewed = ?, reviewed_at = ?, updated_at = ?
110 - WHERE id = ?"
111 - )
112 - .bind(went_well)
113 - .bind(could_improve)
114 - .bind(is_reviewed as i32)
115 - .bind(&reviewed_at_str)
116 - .bind(&now_str)
117 - .bind(existing.id.to_string())
118 - .execute(&self.pool)
119 - .await
120 - .map_err(CoreError::database)?;
121 -
122 - (existing.id, existing.created_at)
123 - } else {
124 - let id = DailyNoteId::new();
125 - sqlx::query(
126 - "INSERT INTO daily_notes (id, user_id, note_date, went_well, could_improve, is_reviewed, reviewed_at, created_at, updated_at)
127 - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"
128 - )
129 - .bind(id.to_string())
130 - .bind(&user_id_str)
131 - .bind(&date_str)
132 - .bind(went_well)
133 - .bind(could_improve)
134 - .bind(is_reviewed as i32)
135 - .bind(&reviewed_at_str)
136 - .bind(&now_str)
137 - .bind(&now_str)
138 - .execute(&self.pool)
139 - .await
140 - .map_err(CoreError::database)?;
141 -
142 - (id, now)
143 - };
105 + let id = DailyNoteId::new();
106 +
107 + // Atomic upsert. A non-transactional get-then-insert/update let two
108 + // concurrent first-writes for the same (user, date) both see "none" and
109 + // both INSERT, surfacing a raw UNIQUE violation (ultra-fuzz Run #28). The
110 + // conflict now folds into an UPDATE; created_at is preserved on that path.
111 + sqlx::query(
112 + "INSERT INTO daily_notes (id, user_id, note_date, went_well, could_improve, is_reviewed, reviewed_at, created_at, updated_at)
113 + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)
114 + ON CONFLICT(user_id, note_date) DO UPDATE SET
115 + went_well = excluded.went_well,
116 + could_improve = excluded.could_improve,
117 + is_reviewed = excluded.is_reviewed,
118 + reviewed_at = excluded.reviewed_at,
119 + updated_at = excluded.updated_at"
120 + )
121 + .bind(id.to_string())
122 + .bind(&user_id_str)
123 + .bind(&date_str)
124 + .bind(went_well)
125 + .bind(could_improve)
126 + .bind(is_reviewed as i32)
127 + .bind(&reviewed_at_str)
128 + .bind(&now_str)
129 + .bind(&now_str)
130 + .execute(&self.pool)
131 + .await
132 + .map_err(CoreError::database)?;
144 133
145 - Ok(DailyNote {
146 - id,
147 - user_id,
148 - note_date: date,
149 - went_well: went_well.to_string(),
150 - could_improve: could_improve.to_string(),
151 - is_reviewed,
152 - reviewed_at: reviewed_at_str.as_deref().map(parse_datetime).transpose()?,
153 - created_at,
154 - updated_at: now,
155 - })
134 + // Read back the canonical row — on the conflict path the stored id and
135 + // created_at are the pre-existing ones, not the values we just generated.
136 + self.get_by_date(user_id, date)
137 + .await?
138 + .ok_or_else(|| CoreError::internal("daily note missing immediately after upsert"))
156 139 }
157 140 }
M crates/db-sqlite/src/repository/subtask_repo.rs +14 -27
@@ -113,28 +113,23 @@ pub(crate) async fn add_subtask(
113 113 return Ok(None);
114 114 }
115 115
116 - let max_position: (Option<i32>,) = sqlx::query_as(
117 - "SELECT MAX(position) FROM subtasks WHERE task_id = ?"
118 - )
119 - .bind(task_id.to_string())
120 - .fetch_one(pool)
121 - .await
122 - .map_err(CoreError::database)?;
123 -
124 - let position = max_position.0.unwrap_or(-1) + 1;
125 116 let id = SubtaskId::new();
126 117
127 - sqlx::query(
118 + // Compute the next position inside the INSERT so the read-modify-write can't
119 + // race a concurrent add (SQLite serializes writers, so the subquery sees every
120 + // committed sibling). RETURNING gives back the position actually stored.
121 + let (position,): (i32,) = sqlx::query_as(
128 122 r#"
129 123 INSERT INTO subtasks (id, task_id, text, position)
130 - VALUES (?, ?, ?, ?)
124 + SELECT ?, ?, ?, COALESCE(MAX(position), -1) + 1 FROM subtasks WHERE task_id = ?
125 + RETURNING position
131 126 "#,
132 127 )
133 128 .bind(id.to_string())
134 129 .bind(task_id.to_string())
135 130 .bind(text)
136 - .bind(position)
137 - .execute(pool)
131 + .bind(task_id.to_string())
132 + .fetch_one(pool)
138 133 .await
139 134 .map_err(CoreError::database)?;
140 135
@@ -274,25 +269,17 @@ pub(crate) async fn add_subtask_link(
274 269 return Ok(None);
275 270 }
276 271
277 - // Get next position
278 - let max_position: (Option<i32>,) = sqlx::query_as(
279 - "SELECT MAX(position) FROM subtasks WHERE task_id = ?"
280 - )
281 - .bind(task_id.to_string())
282 - .fetch_one(pool)
283 - .await
284 - .map_err(CoreError::database)?;
285 -
286 - let position = max_position.0.unwrap_or(-1) + 1;
287 272 let id = SubtaskId::new();
288 273
289 274 // Determine completion status based on linked task
290 275 let is_completed = *linked_task_status == TaskStatus::Completed;
291 276
292 - sqlx::query(
277 + // Atomic position assignment + RETURNING (see add_subtask) — no MAX+1 race.
278 + let (position,): (i32,) = sqlx::query_as(
293 279 r#"
294 280 INSERT INTO subtasks (id, task_id, text, linked_task_id, is_completed, position)
295 - VALUES (?, ?, ?, ?, ?, ?)
281 + SELECT ?, ?, ?, ?, ?, COALESCE(MAX(position), -1) + 1 FROM subtasks WHERE task_id = ?
282 + RETURNING position
296 283 "#,
297 284 )
298 285 .bind(id.to_string())
@@ -300,8 +287,8 @@ pub(crate) async fn add_subtask_link(
300 287 .bind(linked_task_description)
301 288 .bind(linked_task_id.to_string())
302 289 .bind(is_completed as i32)
303 - .bind(position)
304 - .execute(pool)
290 + .bind(task_id.to_string())
291 + .fetch_one(pool)
305 292 .await
306 293 .map_err(CoreError::database)?;
307 294
M crates/db-sqlite/src/repository/time_session_repo.rs +12 -1
@@ -110,7 +110,18 @@ pub(crate) async fn start_timer(
110 110 .bind(&now)
111 111 .execute(&mut *tx)
112 112 .await
113 - .map_err(CoreError::database)?;
113 + .map_err(|e| {
114 + // The partial unique index on (user_id) WHERE ended_at IS NULL (migration
115 + // 053) is the real TOCTOU backstop if a concurrent caller slips past the
116 + // existence check above. Surface the same friendly message instead of a
117 + // raw database error (ultra-fuzz Run #28).
118 + match e {
119 + sqlx::Error::Database(ref db) if db.is_unique_violation() => {
120 + CoreError::validation("timer", "A timer is already running. Stop or discard it first.")
121 + }
122 + other => CoreError::database(other),
123 + }
124 + })?;
114 125
115 126 let row = sqlx::query_as::<_, TimeSessionRow>(
116 127 "SELECT id, task_id, user_id, started_at, ended_at, duration_minutes, created_at FROM time_sessions WHERE id = ?"