max / goingson

fix(storage): blob GC, attachment write-order, atomic positive-only manual time Axis 4 of the Run #26 ultra-fuzz remediation. UF-7: add blob_gc, a startup reconcile that unlinks orphaned attachment blobs. The reference set is the union of attachments.blob_hash AND every blob hash in emails.attachment_meta, so a blob an email still points at is never deleted. Runs before the schedulers spawn and before the UI can create attachments, so it is race-free, and it reclaims both ON DELETE CASCADE leaks and ordinary deletes (which previously left blobs on disk forever). UF-8: add_attachment verifies the parent task/project exists before writing any blob, and removes a freshly-written blob if the row INSERT fails (dedup hits are preserved). CHRONIC-B: log_manual_time takes a validated PositiveMinutes(NonZeroU32) instead of a raw i32, and runs the ownership check, session INSERT, and task-cache UPDATE in a single transaction. The redundant command-layer guard is removed. Low: search_repo documents its total as a deliberate lower bound; task_repo only changes completed_at on an actual status transition; the email-id migration detaches its FK-disabled connection so an early return can't hand it back to the pool.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: Max Johnson <me@maxj.phd> · 2026-06-16 02:55 UTC

Signed with PGP, not checked

Commit: 735059d650e3794992e4717608bddb9b5ed8dd19

Parent: 7025358

13 files changed, +284 insertions, -60 deletions

M src-tauri/src/lib.rs +4

			@@ -5,6 +5,7 @@
5	5		//! or via `mobile_entry_point` (iOS/Android).
6	6
7	7		pub mod backup_scheduler;
	8	+	pub mod blob_gc;
8	9		pub mod commands;
9	10		pub mod email;
10	11		pub mod email_sync_scheduler;
			@@ -314,6 +315,9 @@
314	315		tauri::async_runtime::block_on(async move {
315	316		let state = AppState::new(&app_handle).await
316	317		.expect("Failed to initialize app state");
	318	+	// Reclaim orphaned attachment blobs before the schedulers spawn
	319	+	// and before the UI can create attachments — race-free here.
	320	+	blob_gc::reconcile(&state.pool, &state.data_dir).await;
317	321		app_handle.manage(Arc::new(state));
318	322		});
319	323

M crates/core/src/lib.rs +1 -1

			@@ -72,7 +72,7 @@
72	72		Project, ParseableEnum, ProjectStatus, ProjectType, Recurrence, RecurrenceRule, MonthlySpec,
73	73		SavedView, SortDirection,
74	74		SyncAccount,
75		-	SortField, Subtask, Task, TaskFilterQuery, TaskSortColumn, TaskStatus, TimeSession,
	75	+	PositiveMinutes, SortField, Subtask, Task, TaskFilterQuery, TaskSortColumn, TaskStatus, TimeSession,
76	76		TimeTrackingSummary, UpdateEvent, UpdateProject, UpdateTask, User,
77	77		ViewFilters, ViewType, WeeklyReview,
78	78		format_file_size, mime_from_extension,

M crates/core/src/repository.rs +5 -2

			@@ -21,7 +21,7 @@
21	21		use crate::models::{
22	22		Annotation, Attachment, Email, EmailAccount, EmailAuthType, EmailThread, Event,
23	23		FolderSyncState, NewAttachment, NewEmail, NewEmailWithTracking, NewEvent, NewProject,
24		-	NewSavedView, NewTask, Project, SavedView, Subtask, Task, TaskFilterQuery, TimeSession,
	24	+	NewSavedView, NewTask, PositiveMinutes, Project, SavedView, Subtask, Task, TaskFilterQuery, TimeSession,
25	25		TimeTrackingSummary, UpdateTask, User,
26	26		};
27	27
			@@ -226,7 +226,10 @@
226	226		async fn list_all_time_sessions(&self, user_id: UserId) -> Result<Vec<TimeSession>>;
227	227
228	228		/// Logs a manual time entry (retroactive, no live timer).
229		-	async fn log_manual_time(&self, task_id: TaskId, user_id: UserId, minutes: i32, date: DateTime<Utc>) -> Result<TimeSession>;
	229	+	///
	230	+	/// `minutes` is a validated [`PositiveMinutes`], so the repository cannot be
	231	+	/// handed a zero/negative duration.
	232	+	async fn log_manual_time(&self, task_id: TaskId, user_id: UserId, minutes: PositiveMinutes, date: DateTime<Utc>) -> Result<TimeSession>;
230	233
231	234		/// Gets aggregated time tracking summary grouped by project and date.
232	235		async fn get_time_summary(&self, user_id: UserId, start: DateTime<Utc>, end: DateTime<Utc>) -> Result<Vec<TimeTrackingSummary>>;

M crates/db-sqlite/src/migrations.rs +9 -4

			@@ -41,12 +41,17 @@
41	41
42	42		let mut updated = 0u64;
43	43
44		-	// Use a dedicated connection with FK enforcement off, wrapped in a transaction
	44	+	// Use a dedicated connection with FK enforcement off, wrapped in a transaction.
	45	+	// `detach()` removes it from the pool: `PRAGMA foreign_keys` is per-connection,
	46	+	// so any early-return between here and the re-enable below must NOT hand a
	47	+	// FK-disabled connection back to the pool. A detached connection is simply
	48	+	// closed on drop instead.
45	49		let mut conn = pool.acquire().await
46		-	.map_err(\|e\| format!("Failed to acquire connection: {e}"))?;
	50	+	.map_err(\|e\| format!("Failed to acquire connection: {e}"))?
	51	+	.detach();
47	52
48	53		sqlx::query("PRAGMA foreign_keys = OFF")
49		-	.execute(&mut *conn)
	54	+	.execute(&mut conn)
50	55		.await
51	56		.map_err(\|e\| format!("Failed to disable FK: {e}"))?;
52	57
			@@ -95,7 +100,7 @@
95	100
96	101		// Re-enable FK enforcement on the same connection
97	102		sqlx::query("PRAGMA foreign_keys = ON")
98		-	.execute(&mut *conn)
	103	+	.execute(&mut conn)
99	104		.await
100	105		.map_err(\|e\| format!("Failed to re-enable FK: {e}"))?;
101	106

M crates/db-sqlite/tests/restore_tests.rs +2 -2

			@@ -11,7 +11,7 @@
11	11		use goingson_core::backup_restore::RestoreInput;
12	12		use goingson_core::{
13	13		AttachmentRepository, DailyNoteRepository, EventRepository, MilestoneRepository, NewAttachment,
14		-	NewEvent, NewMilestone, NewProject, NewTask, ProjectRepository, SyncAccountRepository,
	14	+	NewEvent, NewMilestone, NewProject, NewTask, PositiveMinutes, ProjectRepository, SyncAccountRepository,
15	15		TaskRepository, TaskStatus,
16	16		};
17	17		use goingson_db_sqlite::{
			@@ -340,7 +340,7 @@
340	340		.await
341	341		.unwrap();
342	342		let session = tasks
343		-	.log_manual_time(task.id, user, 45, chrono::Utc::now())
	343	+	.log_manual_time(task.id, user, PositiveMinutes::try_new(45).unwrap(), chrono::Utc::now())
344	344		.await
345	345		.unwrap();
346	346		let note = daily_notes

M crates/db-sqlite/tests/time_session_repo_tests.rs +3 -3

			@@ -2,7 +2,7 @@
2	2
3	3		mod common;
4	4
5		-	use goingson_core::{CoreError, TaskId, TaskRepository};
	5	+	use goingson_core::{CoreError, PositiveMinutes, TaskId, TaskRepository};
6	6		use goingson_db_sqlite::SqliteTaskRepository;
7	7
8	8		#[tokio::test]
			@@ -261,7 +261,7 @@
261	261		let repo = SqliteTaskRepository::new(pool);
262	262
263	263		let session = repo
264		-	.log_manual_time(task_id, user_id, 30, chrono::Utc::now())
	264	+	.log_manual_time(task_id, user_id, PositiveMinutes::try_new(30).unwrap(), chrono::Utc::now())
265	265		.await
266	266		.expect("logging manual time on an owned task should succeed");
267	267		assert_eq!(session.duration_minutes, Some(30));
			@@ -284,7 +284,7 @@
284	284
285	285		let bogus_task = TaskId::new();
286	286		let result = repo
287		-	.log_manual_time(bogus_task, user_id, 30, chrono::Utc::now())
	287	+	.log_manual_time(bogus_task, user_id, PositiveMinutes::try_new(30).unwrap(), chrono::Utc::now())
288	288		.await;
289	289
290	290		assert!(matches!(result, Err(CoreError::NotFound { .. })), "expected NOT_FOUND, got {result:?}");

M src-tauri/src/commands/attachment.rs +29 -4

			@@ -77,6 +77,17 @@
77	77		return Err(ApiError::validation_msg("Either taskId or projectId is required"));
78	78		}
79	79
	80	+	// Verify the parent exists before touching the blob store, so a bad id
	81	+	// fails fast instead of after a disk write orphans a blob.
	82	+	if let Some(tid) = task_id {
	83	+	state.tasks.get_by_id(tid, DESKTOP_USER_ID).await?
	84	+	.or_not_found("task", tid)?;
	85	+	}
	86	+	if let Some(pid) = project_id {
	87	+	state.projects.get_by_id(pid, DESKTOP_USER_ID).await?
	88	+	.or_not_found("project", pid)?;
	89	+	}
	90	+
80	91		let source_path = Path::new(&file_path);
81	92
82	93		// Validate path exists and is a file
			@@ -116,9 +127,11 @@
116	127		std::fs::create_dir_all(&blobs_dir)
117	128		.map_api_err("Failed to create blobs directory", ApiError::internal)?;
118	129
119		-	// Copy to blob store (skip if hash already exists — dedup)
	130	+	// Copy to blob store (skip if hash already exists — dedup). Track whether we
	131	+	// were the writer so a failed insert can roll back exactly the blob we added.
120	132		let blob_path = blobs_dir.join(&hash);
121		-	if !blob_path.exists() {
	133	+	let wrote_new_blob = !blob_path.exists();
	134	+	if wrote_new_blob {
122	135		std::fs::write(&blob_path, &file_data)
123	136		.map_api_err("Failed to write blob", ApiError::internal)?;
124	137		}
			@@ -133,7 +146,7 @@
133	146		let mime_type = mime_from_extension(&filename).to_string();
134	147		let file_size = file_data.len() as i64;
135	148
136		-	let attachment = state.attachments
	149	+	let create_result = state.attachments
137	150		.create(DESKTOP_USER_ID, NewAttachment {
138	151		task_id,
139	152		project_id,
			@@ -143,7 +156,19 @@
143	156		blob_hash: hash,
144	157		source_email_id: None,
145	158		})
146		-	.await?;
	159	+	.await;
	160	+
	161	+	let attachment = match create_result {
	162	+	Ok(a) => a,
	163	+	Err(e) => {
	164	+	// The row never landed; reclaim the blob we just wrote so it doesn't
	165	+	// leak. Only remove a blob we created this call (dedup hits must stay).
	166	+	if wrote_new_blob {
	167	+	let _ = std::fs::remove_file(&blob_path);
	168	+	}
	169	+	return Err(e.into());
	170	+	}
	171	+	};
147	172
148	173		Ok(to_response(attachment, &state.data_dir))
149	174		}

M src-tauri/src/commands/time_tracking.rs +7 -10

			@@ -112,16 +112,13 @@
112	112		state: State<'_, Arc<AppState>>,
113	113		input: LogManualTimeInput,
114	114		) -> Result<TimeSession, ApiError> {
115		-	// Reject non-positive durations: a negative value produces ended_at <
116		-	// started_at and decrements the task's actual_minutes cache (can go
117		-	// negative); zero records an empty session.
118		-	if input.minutes < 1 {
119		-	return Err(ApiError::validation("minutes", "Minutes must be at least 1"));
120		-	}
121		-	// Ownership of `task_id` is verified inside the repo method (it must precede
122		-	// the session INSERT to avoid an orphan row); a foreign/bogus id surfaces as
123		-	// a NOT_FOUND error here.
124		-	Ok(state.tasks.log_manual_time(input.task_id, DESKTOP_USER_ID, input.minutes, input.date).await?)
	115	+	// Validate the duration via the newtype: anything below 1 is rejected here
	116	+	// (a negative value would make ended_at < started_at and drive the task's
	117	+	// actual_minutes cache negative; zero records an empty session).
	118	+	let minutes = goingson_core::PositiveMinutes::try_new(input.minutes)?;
	119	+	// Ownership of `task_id` is verified inside the repo method, within the same
	120	+	// transaction as the writes; a foreign/bogus id surfaces as NOT_FOUND.
	121	+	Ok(state.tasks.log_manual_time(input.task_id, DESKTOP_USER_ID, minutes, input.date).await?)
125	122		}
126	123
127	124		/// Gets time tracking summary grouped by project and date.

M crates/core/src/models/time_session.rs +43

			@@ -4,10 +4,40 @@
4	4		//! when a timer is started and closed when it's stopped. At most one session
5	5		//! per user can be active (ended_at IS NULL) at any time.
6	6
	7	+	use std::num::NonZeroU32;
	8	+
7	9		use chrono::{DateTime, Utc};
8	10		use serde::{Deserialize, Serialize};
	11	+	use crate::error::CoreError;
9	12		use crate::id_types::{TaskId, TimeSessionId, UserId, ProjectId};
10	13
	14	+	/// A validated, strictly-positive minute count for a manual time entry.
	15	+	///
	16	+	/// Constructing this is the single gate that rejects zero/negative durations:
	17	+	/// a negative value would make `ended_at < started_at` and drive a task's
	18	+	/// cached `actual_minutes` negative, and zero records an empty session. Because
	19	+	/// the repository takes `PositiveMinutes` (not a raw `i32`), that whole class is
	20	+	/// unrepresentable below the command layer.
	21	+	#[derive(Debug, Clone, Copy, PartialEq, Eq)]
	22	+	pub struct PositiveMinutes(NonZeroU32);
	23	+
	24	+	impl PositiveMinutes {
	25	+	/// Validate a raw minute count. Returns a `Validation` error for anything
	26	+	/// less than 1 (including negatives, which fail the `u32` conversion).
	27	+	pub fn try_new(minutes: i32) -> crate::Result<Self> {
	28	+	u32::try_from(minutes)
	29	+	.ok()
	30	+	.and_then(NonZeroU32::new)
	31	+	.map(Self)
	32	+	.ok_or_else(\|\| CoreError::validation("minutes", "Minutes must be at least 1"))
	33	+	}
	34	+
	35	+	/// The validated count as an `i32` for binding into SQL.
	36	+	pub fn as_i32(self) -> i32 {
	37	+	self.0.get() as i32
	38	+	}
	39	+	}
	40	+
11	41		/// A single time tracking session on a task.
12	42		#[derive(Debug, Clone, Serialize, Deserialize)]
13	43		#[serde(rename_all = "camelCase")]
			@@ -90,4 +120,17 @@
90	120		let session = make_session(start, None);
91	121		assert!(session.elapsed_minutes() >= 9);
92	122		}
	123	+
	124	+	#[test]
	125	+	fn positive_minutes_accepts_positive_values() {
	126	+	assert_eq!(PositiveMinutes::try_new(1).unwrap().as_i32(), 1);
	127	+	assert_eq!(PositiveMinutes::try_new(30).unwrap().as_i32(), 30);
	128	+	}
	129	+
	130	+	#[test]
	131	+	fn positive_minutes_rejects_zero_and_negatives() {
	132	+	assert!(PositiveMinutes::try_new(0).is_err());
	133	+	assert!(PositiveMinutes::try_new(-1).is_err());
	134	+	assert!(PositiveMinutes::try_new(-30).is_err());
	135	+	}
93	136		}

M crates/db-sqlite/src/repository/search_repo.rs +6 -1

M crates/db-sqlite/src/repository/task_repo.rs +14 -14

M crates/db-sqlite/src/repository/time_session_repo.rs +25 -19

A src-tauri/src/blob_gc.rs +136