Lumotia/docs/superpowers/plans/2026-05-12-area-a-storage-errors-survey.md

name, type, tags, description, status

name	type	tags	description	status
Area A — storage error inventory and proposed taxonomy	survey	engine residuals area-a storage errors survey	Pre-migration survey for engine-slop residuals Area A. Catalogues every MagnotiaError::StorageError construction site in the storage crate, groups by failure mode, proposes a typed lumotia_storage::Error enum with a serializable MagnotiaError::Storage variant on top, and flags ambiguities + migration risks. No code changes yet.	draft

TL;DR

There is no StorageError enum to extend. The storage crate has zero local error type and produces failures directly as MagnotiaError::StorageError(String) via 78 format!() sites across database.rs (72) and migrations.rs (6). The residuals plan's framing of "Storage-layer typed errors (~25 sites)" undercounted — actual count is 3× higher — because the plan came from a residuals pass that did not survey the storage crate.

The Tauri command layer stringifies every error before crossing into the frontend (Result<T, String> everywhere via .map_err(|e| e.to_string())?), so MagnotiaError's Serialize impl is presently unused at the FE/BE boundary. Area E is where that ossifies; Area A's job is to give backend code something to branch on — not to fix the frontend.

Proposed: introduce a new lumotia_storage::Error enum in the storage crate, replace the String tail of MagnotiaError::StorageError(String) with a structured MagnotiaError::Storage { kind, operation, detail } variant, and wire the conversion through a From<storage::Error> for MagnotiaError impl that flattens the structured error into a serde-friendly shape. Backend code can pattern-match on storage::Error; the frontend keeps receiving (slightly nicer) strings until Area E.

Inventory

Headline numbers

File	Sites	What they wrap
crates/storage/src/database.rs	72	sqlx CRUD operations, pre-condition checks, post-update invariant checks
crates/storage/src/migrations.rs	6	Schema version table creation/query, per-migration tx/exec/record/commit
crates/storage/src/file_storage.rs	0	Uses From<io::Error> via ? — already typed
crates/storage/src/lib.rs	0	Re-exports only
Total	78

Storage crate context

• Uses lumotia_core::error::{MagnotiaError, Result} directly — no local error type.
• No From<sqlx::Error> impl exists, which is why every sqlx call has an explicit .map_err(...).
• From<std::io::Error> for MagnotiaError does exist (in crates/core/src/error.rs), so directory-creation paths use ? cleanly.

Grouped by failure mode, not by file

Bucket 1 — Database connection / init / pragma (3 sites)

Site	Current message prefix
database.rs:22	Database connect failed: {e}
database.rs:27	foreign_keys pragma failed: {e}
database.rs:50	Read-only connect failed: {e}

Source: sqlx::Error. Backend interest: could plausibly want to distinguish read-only fallback failure from primary connect failure for telemetry, but no code currently branches on this.

Bucket 2 — Migration framework (6 sites, all in migrations.rs)

Site	Step	Has version?
migrations.rs:557	schema_version_table_create	no
migrations.rs:563	schema_version_query	no
migrations.rs:570	tx_begin	yes
migrations.rs:578	apply	yes
migrations.rs:588	record_version	yes
migrations.rs:592	commit	yes

Source: sqlx::Error. Backend interest: the migration retry-poison test (migrations.rs:1082) wants to assert these specifically, which already implies the type-system benefit of a dedicated variant.

Bucket 3 — Query execution (sqlx wrap) (~64 sites)

Every CRUD operation — INSERT, SELECT, UPDATE, DELETE — wrapped with .map_err(|e| MagnotiaError::StorageError(format!("<Operation name> failed: {e}"))).

A non-exhaustive sample (the full 64 share the same shape):

• database.rs:113 — Insert transcript
• database.rs:124 — Get transcript
• database.rs:147 — List transcripts
• database.rs:157 — Count transcripts
• database.rs:186/197/208 — Update transcript (three branches in one function)
• database.rs:256 — update_transcript_meta
• database.rs:268 — Delete transcript
• database.rs:292 — FTS search
• database.rs:330/342/355 — task CRUD
• database.rs:393/414 — task update / energy
• database.rs:433/446/463/470/479/494 — subtask CRUD + auto-complete-parent transaction sub-steps
• database.rs:506-553 — complete_task / uncomplete_task transaction sub-steps
• database.rs:610/621/633 — analytics queries
• database.rs:666/683/700/720/745/761 — implementation_rule CRUD
• database.rs:774/783 — setting get/set
• database.rs:969-1122 — profile + profile_term CRUD
• database.rs:1156/1188/1200 — error_log
• database.rs:1305/1342 — feedback_log

Source: sqlx::Error. Backend interest: programmatic branching on sqlx error kind (e.g., RowNotFound, Database with constraint name, connection drops) is meaningful at this layer. The operation label is purely descriptive — not a useful axis for an enum variant.

Bucket 4 — Invariant violation (post-update rows-affected check) (3 sites)

Site	Entity	Condition
database.rs:259	transcript	update_transcript_meta UPDATE affected 0 rows
database.rs:396	task	update_task UPDATE affected 0 rows
database.rs:417	task	set_task_energy UPDATE affected 0 rows

These are logically distinct from query failure — the SQL succeeded; the row simply isn't there. Today they're conflated with sqlx errors in StorageError(String). Worth typing because (a) callers might want to surface a different message for "not found" vs "DB error", and (b) it removes ambiguity in logs.

Subtlety: there's no guard against the race "row existed at check time, was deleted before update" — these errors fire correctly in that race but the caller has no way to distinguish that from a stale id passed in by the user.

Bucket 5 — Pre-condition validation (1 site)

Site	Reason
database.rs:85	insert_transcript called with a profile_id that doesn't exist in profiles table

Today: return Err(MagnotiaError::StorageError(format!("Insert transcript failed: unknown profile id '{}'", params.profile_id))).

This is not a sqlx failure. It's an integrity check we perform manually before issuing the INSERT. Properly speaking, this is "invalid reference" — distinct from "query failed" and from "row not found".

Out-of-scope (already typed at MagnotiaError level)

• std::fs::create_dir_all(parent)? at database.rs:11 — propagates through From<std::io::Error> for MagnotiaError to MagnotiaError::Io { kind, message, raw_os_error }. Already structured.

Proposed taxonomy

Where it lives

Option A (recommended): new lumotia_storage::Error enum in the storage crate.

// crates/storage/src/error.rs
use std::borrow::Cow;
use thiserror::Error;

#[derive(Debug, Error)]
pub enum Error {
    #[error("database open failed during {operation}: {source}")]
    DatabaseOpen {
        operation: OpenOp,
        #[source]
        source: sqlx::Error,
    },

    #[error("migration step {step} (version {version:?}) failed: {source}")]
    Migration {
        version: Option<i64>,
        step: MigrationStep,
        #[source]
        source: sqlx::Error,
    },

    #[error("query failed during {operation}: {source}")]
    Query {
        operation: Cow<'static, str>,
        #[source]
        source: sqlx::Error,
    },

    #[error("{entity} not found: '{key}'")]
    NotFound {
        entity: Entity,
        key: String,
    },

    #[error("invalid {entity} reference: {reason}")]
    InvalidReference {
        entity: Entity,
        reason: Cow<'static, str>,
    },
}

#[derive(Debug, Clone, Copy)]
pub enum OpenOp { Connect, ReadOnlyConnect, ForeignKeysPragma }

#[derive(Debug, Clone, Copy)]
pub enum MigrationStep {
    SchemaVersionTableCreate,
    SchemaVersionQuery,
    TxBegin,
    Apply,
    RecordVersion,
    Commit,
}

#[derive(Debug, Clone, Copy)]
pub enum Entity {
    Transcript,
    Task,
    Subtask,
    Profile,
    ProfileTerm,
    ImplementationRule,
    Setting,
    ErrorLogRow,
}

pub type Result<T> = std::result::Result<T, Error>;

Why option A:

• The taxonomy lives where the failures originate. Storage owns its own error vocabulary.
• The storage crate can be tested in isolation without lumotia_core understanding its internals.
• From<storage::Error> for MagnotiaError keeps propagation automatic via ?.
• Sets up Area E to selectively expose storage-error kinds to the frontend without a second migration of call sites.

Option B (rejected): structured variant inside MagnotiaError directly.

Mentioned for completeness:

// In crates/core/src/error.rs
pub enum MagnotiaError {
    // ...
    Storage {
        kind: StorageKind,
        operation: String,
        detail: String,
    },
    // ...
}

• Less ceremony, one file changes.
• But couples the storage taxonomy into lumotia_core, which is a leaky abstraction.
• Harder to extend later when, say, we add a vocabulary crate (per D1 in the architecture spec) that also wants typed errors — every crate ends up dumping its enum into MagnotiaError.

How it crosses the MagnotiaError boundary

sqlx::Error does not implement Serialize. MagnotiaError does — that contract has to be preserved for any future Tauri serialisation path (Area E).

Proposed: flatten on the boundary.

// In crates/core/src/error.rs
#[derive(Debug, thiserror::Error, Serialize)]
pub enum MagnotiaError {
    // ... existing variants unchanged ...

    #[error("storage error: {detail}")]
    Storage {
        kind: StorageKind,    // serializable enum: Open | Migration | Query | NotFound | InvalidReference
        operation: String,    // human-readable, e.g. "insert_transcript" or "migration_apply_v7"
        detail: String,       // current Display output of storage::Error
    },
}

#[derive(Debug, Clone, Copy, Serialize)]
#[serde(rename_all = "snake_case")]
pub enum StorageKind {
    DatabaseOpen,
    Migration,
    Query,
    NotFound,
    InvalidReference,
}

impl From<lumotia_storage::Error> for MagnotiaError {
    fn from(e: lumotia_storage::Error) -> Self {
        let kind = match &e {
            lumotia_storage::Error::DatabaseOpen { .. } => StorageKind::DatabaseOpen,
            lumotia_storage::Error::Migration { .. } => StorageKind::Migration,
            lumotia_storage::Error::Query { .. } => StorageKind::Query,
            lumotia_storage::Error::NotFound { .. } => StorageKind::NotFound,
            lumotia_storage::Error::InvalidReference { .. } => StorageKind::InvalidReference,
        };
        let operation = e.operation_label().into_owned();
        let detail = e.to_string();
        MagnotiaError::Storage { kind, operation, detail }
    }
}

operation_label() is a helper on storage::Error returning a Cow<'static, str> from the inner enum data. Backend code that wants programmatic recovery downcasts (or rather, the storage call signatures return Result<_, lumotia_storage::Error> directly — only public Tauri-facing wrappers eat the From conversion).

Function-signature decision

The storage crate has 70+ public functions returning lumotia_core::Result<...>. Two options:

1. Change all to lumotia_storage::Result<...> and let ? in callers do the From conversion. Pros: backend code that wants typed errors gets them. Cons: ~70 signatures to touch, and any backend code that wanted the structured error has to import lumotia_storage::Error.

2. Keep public signatures as lumotia_core::Result<...>, do the conversion at the storage-crate boundary via an internal Result<T> = std::result::Result<T, lumotia_storage::Error> for internal helpers. Public functions then end with ? (or .map_err(Into::into)). Pros: caller signatures unchanged. Cons: callers wanting to pattern-match on storage failure type have nothing to match against (back to square one).

Recommended: option 1. It's the more honest signature. ~70 public functions get changed, but it's mechanical: Result<T> → lumotia_storage::Result<T> and the body's MagnotiaError::StorageError(format!(...)) is what we're replacing anyway. Callers (Tauri commands) that currently .map_err(|e| e.to_string())? keep working because the storage error From-converts into MagnotiaError, which is what Display'd into the string they're already producing.

This also means Area E later only has to delete .map_err(|e| e.to_string())? and change return types to receive structured errors — no second pass over the storage call sites.

Sites → proposed variant

For the migration pass itself. Format: <file>:<line> → variant + fields.

→ Error::DatabaseOpen

• database.rs:22 → DatabaseOpen { operation: OpenOp::Connect, source }
• database.rs:27 → DatabaseOpen { operation: OpenOp::ForeignKeysPragma, source }
• database.rs:50 → DatabaseOpen { operation: OpenOp::ReadOnlyConnect, source }

→ Error::Migration

• migrations.rs:557 → Migration { version: None, step: SchemaVersionTableCreate, source }
• migrations.rs:563 → Migration { version: None, step: SchemaVersionQuery, source }
• migrations.rs:570 → Migration { version: Some(v), step: TxBegin, source }
• migrations.rs:578 → Migration { version: Some(v), step: Apply, source }
• migrations.rs:588 → Migration { version: Some(v), step: RecordVersion, source }
• migrations.rs:592 → Migration { version: Some(v), step: Commit, source }

→ Error::Query

The 64 sites in Bucket 3. Operation labels match the current message stem in snake_case:

• Insert transcript failed → Query { operation: "insert_transcript".into(), source }
• Get transcript failed → Query { operation: "get_transcript".into(), source }
• List transcripts failed → Query { operation: "list_transcripts".into(), source }
• ... (mechanical for the rest)

→ Error::NotFound

• database.rs:259 → NotFound { entity: Entity::Transcript, key: id.to_string() }
• database.rs:396 → NotFound { entity: Entity::Task, key: id.to_string() }
• database.rs:417 → NotFound { entity: Entity::Task, key: id.to_string() }

→ Error::InvalidReference

• database.rs:85 → InvalidReference { entity: Entity::Profile, reason: format!("unknown profile id '{}'", params.profile_id).into() }

Ambiguous cases / decisions needed

1. Migration "Schema version query" — Migration step or Query? Currently classified as MigrationStep::SchemaVersionQuery (within Error::Migration) because it's logically part of the migration framework even though it executes a SELECT. Alternative: treat it as a Query { operation: "schema_version_query" }. Recommendation: keep under Migration. The migration test (migrations.rs:1082) asserts on migration-level failures and would benefit from the unified variant. Decision needed before implementation.

2. Sub-step transactions inside CRUD operations. complete_subtask_and_check_parent (database.rs:453) issues four sqlx calls inside one transaction. Each currently has its own .map_err(...) ("Begin transaction failed", "Complete subtask failed", "Get parent_task_id failed", "Count pending subtasks failed", "Auto-complete parent failed", "Commit transaction failed"). Two ways to type:

   • One Query variant per call, six unique operation labels — preserves current granularity.
   • One Query { operation: "complete_subtask_and_check_parent::<step>" } style, with step in the label — slightly more readable from the FE but loses some debuggability. Recommendation: preserve current granularity (option a). Cheap and matches existing logs.

3. Entity enum cardinality. Proposal includes 8 entities. Realistic check: do we need all 8? Going by NotFound + InvalidReference sites, only Transcript, Task, and Profile actually appear. The other 5 (Subtask, ProfileTerm, ImplementationRule, Setting, ErrorLogRow) are speculative. Recommendation: start with the 3 we need and add more only when a NotFound for that entity actually materialises. Avoid taxonomy theatre.

4. Cow<'static, str> vs &'static str vs String for Query::operation. With ~64 unique labels, we have a choice:

   • All literal &'static str — most efficient, but requires every call site to use a string literal (which it would anyway).
   • Cow<'static, str> — accommodates future dynamic operation names without a heap allocation for the common case.
   • String — flexible, one alloc per error. Recommendation: Cow<'static, str>. The common case is a literal; the escape hatch is free.

5. Serialize implementation strategy. Three options for storage::Error:

   • Derive Serialize and #[serde(skip)] the sqlx::Error source. Loses source detail in serialised form.
   • Custom Serialize impl that stringifies the source.
   • Don't derive Serialize — flatten only at the MagnotiaError boundary as proposed above. Recommendation: the third. Keeps the storage crate's API surface backend-only; serialisation concerns live in lumotia_core. Already reflected in the proposal.

6. Public re-exports from storage crate. Need to decide: do we re-export Error, Result, OpenOp, MigrationStep, Entity, StorageKind (or its storage-side equivalent) from crates/storage/src/lib.rs? Yes — backend code (Tauri commands, future test code) will want to pattern-match. Suggest a pub mod error; + pub use error::{Error, Result, OpenOp, MigrationStep, Entity}; block.

Migration risk notes

Visible behaviour changes

• Display messages change. Current "Insert transcript failed: unique constraint violated" becomes "query failed during insert_transcript: unique constraint violated". Functionally equivalent but slightly different. Logs and the Tauri error toast strings will reflect the new format. Worth eyeballing on the Settings → Profiles page, Files page, and Tasks page once landed.

• Tauri command return shape unchanged. All Result<T, String> signatures stay the same. .map_err(|e| e.to_string())? keeps working because From<storage::Error> for MagnotiaError produces a Display-able MagnotiaError.

Compile-time gotchas

• ? operator semantics depend on which Result is in scope. If function signature returns lumotia_core::Result<T> and body uses lumotia_storage::Result<T> internally, every helper boundary needs an explicit Into::into or a ? cascade with From plumbed. Doable but easy to miss — recommend going function-by-function, not file-by-file.

• Test ergonomics. Storage crate tests at crates/storage/src/migrations.rs:1027+ currently match on MagnotiaError::StorageError patterns. They'll need updating to match on storage::Error::Migration { step: MigrationStep::Apply, .. } style. Roughly 5–10 test assertions, all bounded to that file.

Scope creep guards

• Do not retitle error messages. Goal is to preserve the current Display output verbatim where possible, with the operation label carrying the same information as the current message stem. Wording changes belong in a separate pass.

• Do not change MagnotiaError::StorageError(String) → MagnotiaError::Storage { ... } in one PR alongside the storage-crate work. Two commits:

  1. Add lumotia_storage::Error, From<storage::Error> for MagnotiaError, keep the old StorageError(String) variant temporarily as a no-op (compile-only).
  2. Remove the old StorageError(String) variant; all consumers now go through Storage { ... }.

  Two commits means we can verify the type-system migration is sound before deleting the safety net.

• Resist adding Other(String). The whole point. If a case doesn't fit, surface the ambiguity and decide explicitly.

Verification plan

Once the migration lands:

cargo fmt --all -- --check
cargo check -p lumotia-storage
cargo check -p lumotia-core
cargo check --workspace --all-targets
cargo test -p lumotia-storage
cargo test --workspace --lib
rg 'StorageError\(' crates/ src-tauri/src/   # must be zero
rg 'Other\(String\)' crates/ src-tauri/src/  # must remain zero
rg 'format!\("[A-Z][^"]+failed' crates/storage/src/   # must be zero (catches missed map_errs)

The third grep is the structural assertion: there should be no string-formatted "Whatever failed" messages emerging from the storage crate post-migration — every one becomes structured.

Out of scope (explicit deferrals)

• Area E — frontend/backend error boundary cleanup. The Tauri commands stay on Result<T, String> for now. Once Area A lands, Area E becomes mechanical: change return types, delete .map_err(|e| e.to_string())?, plumb MagnotiaError::Storage { kind, operation, detail } into typed frontend handlers.

• Wording rewrites of user-visible error toasts. The current strings will read slightly differently post-migration. Not changing copy as part of this; copy review is its own task.

• From<sqlx::Error> for storage::Error. Tempting because it would eliminate the .map_err(...) boilerplate, but it would either lose the operation label (single From impl can't know which operation it's wrapping) or require a thin helper macro. Leaving the explicit .map_err(..., op: "insert_transcript") per site for now. Macro investigation can be a tiny follow-up.

• Other(String) introduction. Already absent. Stays absent.