fix(serve): differentiate HTTP error codes instead of catch-all 500

[fagemx]

Problem

All ledger/database errors in edda-serve become `AppError::Internal` → HTTP 500. There is no distinction between:

• Transient errors (database locked, connection timeout) → should be 503
• Data validation errors (missing field from ledger query) → could be 422
• Not found via ledger (project doesn't exist) → should be 404

Current `AppError` has 5 variants (Validation/400, NotFound/404, Conflict/409, Unauthorized/401, Internal/500) but only Validation and Internal are used in most handlers. The other variants exist but are underutilized.

Scope

1. Audit each handler's `?` propagation to identify where more specific status codes apply
2. Add error classification at the handler level:
   • Ledger `open()` fails → 404 if project not found, 500 if I/O error
   • Query returns empty → 404 (not 500)
   • Database locked → 503 with Retry-After header
3. Consider adding `AppError::ServiceUnavailable` variant for transient failures

Notes

• `AppError` definition at `edda-serve/src/lib.rs:72`
• `IntoResponse` impl at same file
• This pairs well with the .context() work (GH-XXX) — better errors + better codes

[fagemx]

Research Phase: GH-379 — Differentiate HTTP Error Codes

Problem Statement

All ledger/database errors in edda-serve become AppError::Internal -> HTTP 500. There is no distinction between transient errors (database locked), data validation errors, or not-found-via-ledger scenarios.

Current Architecture (Post-PR #383)

AppError Enum (crates/edda-serve/src/error.rs)

enum AppError {
    Validation(String),       // 400
    NotFound(String),         // 404
    Conflict(String),         // 409
    Unauthorized(String),     // 401
    Internal(#[from] anyhow::Error),  // 500
}

Additional From impls: serde_json::Error, serde_yaml::Error, globset::Error, std::io::Error — all map to Internal.

Module Layout (16 files after PR #383)

File	Key error patterns
api/events.rs	state.open_ledger().context(...)? — all ledger failures become 500
api/analytics.rs	anyhow::anyhow!("chronicle feature not enabled").into() — becomes 500 (should arguably be 501 or 400)
api/auth.rs	Already uses NotFound, Validation correctly in some places; open_ledger() still 500
api/briefs.rs	Uses NotFound for missing actors/briefs; open_ledger() 500
api/dashboard.rs	No direct ledger open (uses list_projects()); silent if let Ok(...) swallowing
api/drafts.rs	Uses NotFound, Conflict correctly; open_ledger() 500
api/ingestion.rs	Uses NotFound, Conflict for suggestions; open_ledger() 500; no .context()
api/metrics.rs	anyhow::anyhow!("chronicle feature not enabled").into() — 500
api/policy.rs	Pure logic handlers; open_ledger() via approval check
api/snapshots.rs	Uses NotFound for missing snapshots; Internal(anyhow::anyhow!(...)) for missing events
api/stream.rs	Silent error swallowing in SSE loop (Err(_) => continue)
api/telemetry.rs	open_ledger().context(...) — 500
middleware.rs	Uses Unauthorized correctly; open_ledger() 500

Error Propagation Audit

Pattern 1: state.open_ledger().context("...")? (Most Common)

This calls Ledger::open() which can fail because:

1. .edda/ directory doesn't exist -> "not an edda workspace" -> conceptually 404 (project not found) or 500 (shouldn't happen if server started correctly)
2. SQLite Connection::open() fails -> I/O error -> 500
3. SQLite apply_pragmas() fails -> database corrupt/locked -> 500 or 503

Since the server validates workspace existence at startup (serve() line 38-40), case 1 should never happen during normal operation. But it can happen if the .edda/ directory is deleted while the server is running.

Pattern 2: ledger.method()? (Query Propagation)

All Ledger methods return anyhow::Result and use .context(). These become AppError::Internal via the blanket From<anyhow::Error> impl. Failure modes:

• head_branch() -> "HEAD not set in refs table" -> 500 (should not happen normally)
• last_event_hash() -> SQLite query error -> 500
• append_event() -> SQLite write error -> could be 503 if database locked
• find_active_decision() -> query error -> 500
• iter_events_filtered() -> query error -> 500
• get_task_brief() -> returns Option -> properly handled as 404 in briefs handler
• decision_outcomes() -> returns Option -> properly handled as 404 in events handler
• causal_chain() -> returns Option -> properly handled as 404 in events handler

Pattern 3: anyhow::anyhow!("...").into() (Feature Not Enabled)

In analytics.rs and metrics.rs:

if state.chronicle.is_none() {
    return Err(anyhow::anyhow!("chronicle feature not enabled").into());
}

This becomes 500 but should be 501 Not Implemented or 400 (the server simply doesn't support this feature).

Pattern 4: Silent Error Swallowing

In dashboard.rs and analytics.rs:

if let Ok(ledger) = Ledger::open(root) {
    if let Ok(events) = ledger.iter_events() {
        all_events.extend(events);
    }
}

Cross-project iteration intentionally swallows errors (one project failing shouldn't block the aggregate), but this means transient database lock errors are silently ignored.

In stream.rs:

let ledger = match edda_ledger::Ledger::open(&repo_root) {
    Ok(l) => l,
    Err(_) => continue,
};

SSE loop silently retries on open failure — acceptable for a polling loop.

Pattern 5: Missing .context() Calls

In ingestion.rs, several state.open_ledger()? calls lack .context():

• post_ingestion_evaluate line 87
• post_ingestion_record line 178
• get_ingestion_records line 206
• get_ingestion_suggestions line 238
• post_suggestion_accept line 249
• post_suggestion_reject line 273

This doesn't affect HTTP status codes but makes debugging harder.

SQLite "Database Locked" Analysis

SQLite uses file-level locking. With rusqlite:

• SQLITE_BUSY error code is the primary "database locked" scenario
• rusqlite::Error::SqliteFailure with code: ErrorCode::DatabaseBusy is the Rust representation
• The busy_timeout pragma (if set) mitigates short contention windows
• WorkspaceLock (file-based, in lock.rs) is used for write operations but not reads

From sqlite_store/mod.rs:35-39:

pub fn open(db_path: &Path) -> anyhow::Result<Self> {
    let conn = Connection::open(db_path)?;
    let store = Self { conn };
    store.apply_pragmas()?;
    Ok(store)
}

Current pragmas should be checked to see if busy_timeout is set.

Ledger Error Types (Not Structured)

The ledger uses anyhow::Result everywhere — no typed errors. This means the serve layer cannot easily match on specific failure modes. All errors are opaque anyhow::Error.

Key observation: There are no typed error enums in edda-ledger. Every error is wrapped in anyhow::anyhow!() or propagated via ? with .context(). This makes it impossible for edda-serve to pattern-match on specific ledger failure types without string matching (which is fragile).

Existing Good Practices

Some handlers already use specific error variants correctly:

• briefs.rs:131 — AppError::NotFound for missing task brief
• events.rs:277 — AppError::NotFound for missing decision
• events.rs:330 — AppError::NotFound for missing causal chain root
• drafts.rs:289 — AppError::NotFound for missing draft file
• drafts.rs:300-302 — AppError::Conflict for draft already applied/rejected
• ingestion.rs:255-261 — AppError::NotFound and Conflict for suggestion state
• auth.rs:226-229 — AppError::NotFound for no active device token

Summary of Findings

1. Most ? propagation sites correctly use AppError::Internal for genuine internal errors (SQLite query failures, I/O errors). The issue's premise is somewhat overstated — many "not found" cases are already handled with AppError::NotFound.

2. The real gaps are:

   • open_ledger() failures all become 500 even when the root cause is "project not initialized" (should be 404 or 422)
   • "chronicle feature not enabled" returns 500 instead of 501 or 400
   • No ServiceUnavailable (503) variant for transient SQLite busy errors
   • Missing .context() in ingestion handlers

3. The architectural constraint: Since edda-ledger returns anyhow::Error (no typed errors), classify-at-handler-level is the only viable approach without a larger refactor of the ledger crate.

4. Low-hanging fruit: Add AppError::ServiceUnavailable variant, classify open_ledger() failures into 404 vs 500, and fix the chronicle-not-enabled handlers.

[fagemx]

Innovation Phase: GH-379 — Differentiate HTTP Error Codes

Approach Options

Option A: Handler-Level Classification (Minimal, Surgical)

Strategy: Keep AppError as-is, add 1-2 new variants, and classify errors at the handler level using helper functions.

Changes:

1. Add AppError::ServiceUnavailable(String) variant -> 503 with Retry-After header
2. Add AppError::NotImplemented(String) variant -> 501
3. Create a helper classify_ledger_error(anyhow::Error) -> AppError that inspects the error chain:
   • If rusqlite::Error::SqliteFailure with DatabaseBusy -> ServiceUnavailable
   • If error message contains "not an edda workspace" -> NotFound
   • Otherwise -> Internal
4. Replace state.open_ledger().context(...)? with state.open_ledger().map_err(|e| classify_ledger_error(e))?
5. Replace chronicle-not-enabled errors with AppError::NotImplemented

Pros: Minimal blast radius. No changes to edda-ledger. Works with existing anyhow-based errors.
Cons: String matching on error messages is fragile. Adding .map_err() to every call site is verbose.

Option B: Smart open_ledger() with From Classification

Strategy: Modify AppState::open_ledger() to return Result<Ledger, AppError> instead of anyhow::Result<Ledger>, classifying at the source.

Changes:

1. Add ServiceUnavailable and NotImplemented variants to AppError
2. Change AppState::open_ledger() to catch and classify:

   pub(crate) fn open_ledger(&self) -> Result<Ledger, AppError> {
       Ledger::open(&self.repo_root).map_err(classify_open_error)
   }

3. Implement classify_open_error that downcasts anyhow::Error to check for:
   • rusqlite::ffi::Error / rusqlite::Error via anyhow::Error::downcast_ref()
   • "not an edda workspace" string pattern
4. For post-open ledger calls, keep ? as-is (they correctly become 500)
5. For chronicle-not-enabled, use NotImplemented directly

Pros: Classification happens in one place (open_ledger). Handler code barely changes. Downcasting is more robust than string matching.
Cons: Only classifies open errors, not query errors. Rusqlite errors might be wrapped in layers of anyhow context, making downcast unreliable.

Option C: Typed Ledger Errors (Larger Scope)

Strategy: Add a LedgerError enum to edda-ledger and implement From<LedgerError> for AppError in edda-serve.

Changes:

1. Define LedgerError in edda-ledger:

   enum LedgerError {
       NotInitialized(PathBuf),
       DatabaseBusy,
       Io(std::io::Error),
       Query(String),
       Schema(String),
   }

2. Migrate key ledger functions from anyhow::Result to Result<T, LedgerError>
3. Implement From<LedgerError> for AppError:
   • NotInitialized -> NotFound
   • DatabaseBusy -> ServiceUnavailable
   • Io, Query, Schema -> Internal
4. Add NotImplemented variant for chronicle handlers

Pros: Most robust. Clean separation of concerns. No string matching.
Cons: Significant refactor of edda-ledger (touches many files). Gate 3 material (cross-module contract change). Could be a separate issue.

Evaluation

Criterion	Option A	Option B	Option C
Blast radius	Low (5 files)	Low (3 files)	High (20+ files)
Robustness	Medium (string match)	Good (downcast)	Best (typed)
Time to implement	Small	Small	Large
Future-proof	Fair	Good	Best
Addresses issue scope	Fully	Mostly	Over-delivers

Recommendation: Option B (Smart open_ledger())

Option B gives the best ROI:

• Classifies the most common error source (open_ledger()) at a single choke point
• Uses anyhow::Error::downcast_ref() which is more robust than string matching
• Handler code requires minimal changes (just remove .context() on open_ledger calls since the new method handles classification)
• Add NotImplemented variant for chronicle feature guards
• Add ServiceUnavailable variant with Retry-After: 1 header for database busy

Option C (typed ledger errors) could follow later as a dedicated refactoring issue for edda-ledger, without blocking this serve-layer fix.

Additional Improvements (Bundled)

1. Add missing .context() calls in ingestion.rs — 6 call sites
2. Chronicle feature guard: Extract to a shared helper to reduce duplication between analytics.rs and metrics.rs
3. reconstruct_snapshot in snapshots.rs: Change AppError::Internal(anyhow::anyhow!("event {} not found")) to AppError::NotFound — a missing event for a snapshot row is a data integrity issue, but from the API consumer's perspective it's a "not found"

Error Classification Logic

fn classify_open_error(err: anyhow::Error) -> AppError {
    // Check for "not an edda workspace" (workspace not initialized)
    let msg = err.to_string();
    if msg.contains("not an edda workspace") {
        return AppError::NotFound(msg);
    }

    // Check for SQLite busy (database locked)
    if let Some(rusqlite_err) = err.downcast_ref::<rusqlite::Error>() {
        if matches!(
            rusqlite_err,
            rusqlite::Error::SqliteFailure(
                rusqlite::ffi::Error { code: rusqlite::ffi::ErrorCode::DatabaseBusy, .. },
                _
            )
        ) {
            return AppError::ServiceUnavailable("database is temporarily unavailable".into());
        }
    }

    AppError::Internal(err)
}

Note: rusqlite is not currently a dependency of edda-serve. The downcast approach requires either:

• Adding rusqlite as a dependency to edda-serve (just for the error type), OR
• Doing string matching on the error chain (check for "database is locked" in the error message), OR
• Having edda-ledger re-export the rusqlite error types needed for classification

The pragmatic choice is string matching for now, since rusqlite errors format consistently and this avoids adding a new dependency. We can revisit with Option C (typed errors) later.

[fagemx]

Plan Phase: GH-379 — Differentiate HTTP Error Codes

Chosen Approach

Option B: Smart open_ledger() with error classification at the choke point, plus new AppError variants for 503 and 501.

Implementation Steps

Step 1: Add New AppError Variants (error.rs)

File: crates/edda-serve/src/error.rs

Add two new variants to AppError:

#[error("{0}")]
ServiceUnavailable(String),

#[error("{0}")]
NotImplemented(String),

Update IntoResponse impl to handle them:

• ServiceUnavailable -> StatusCode::SERVICE_UNAVAILABLE (503) + Retry-After: 1 header + code "SERVICE_UNAVAILABLE"
• NotImplemented -> StatusCode::NOT_IMPLEMENTED (501) + code "NOT_IMPLEMENTED"

For ServiceUnavailable, override the default response to include the Retry-After header:

AppError::ServiceUnavailable(_) => {
    let body = serde_json::json!({ "error": self.to_string(), "code": "SERVICE_UNAVAILABLE" });
    let mut resp = (StatusCode::SERVICE_UNAVAILABLE, Json(body)).into_response();
    resp.headers_mut().insert("retry-after", "1".parse().unwrap());
    return resp;
}

Step 2: Create Error Classification Helper (error.rs)

Add a classify_ledger_error function that inspects the anyhow::Error chain:

pub(crate) fn classify_open_error(err: anyhow::Error) -> AppError {
    let msg = format!("{err:#}");

    // "not an edda workspace" -> workspace not initialized
    if msg.contains("not an edda workspace") {
        return AppError::NotFound(err.to_string());
    }

    // SQLite "database is locked" -> transient failure
    if msg.contains("database is locked") {
        return AppError::ServiceUnavailable(
            "database is temporarily unavailable, please retry".into(),
        );
    }

    AppError::Internal(err)
}

Why string matching: rusqlite is not a dependency of edda-serve. Adding it just for error type downcasting is unnecessary. The SQLite error message "database is locked" is stable and well-known. Using format!("{err:#}") captures the full error chain including context layers.

Step 3: Update AppState::open_ledger() (state.rs)

Change the return type and apply classification:

pub(crate) fn open_ledger(&self) -> Result<Ledger, AppError> {
    Ledger::open(&self.repo_root).map_err(crate::error::classify_open_error)
}

This is the single choke point — all handler open_ledger() calls will automatically benefit from the classification. The .context("GET /api/...") calls on open_ledger() in handlers become unnecessary for error classification (though they could be kept for tracing/debugging).

Impact: Since open_ledger() now returns Result<Ledger, AppError> instead of anyhow::Result<Ledger>, any .context() call on it will fail to compile because AppError doesn't implement the anyhow::Context trait. We need to remove .context() from all open_ledger() call sites.

Step 4: Remove .context() from open_ledger() Call Sites

Remove .context("GET /api/...") from all state.open_ledger().context(...) call sites across all handler modules. The classification is now handled by classify_open_error.

Affected files (all .context() on open_ledger()):

• api/events.rs — 7 call sites (lines 43, 76, 113, 198, 267-269, 325, 400, 466, 514, 636)
• api/auth.rs — 4 call sites (lines 129, 186, 217, 276)
• api/briefs.rs — 3 call sites (lines 38, 61, 93, 128)
• api/drafts.rs — 1 call site (line 85, 283)
• api/metrics.rs — 2 call sites (lines 37, 67)
• api/snapshots.rs — 4 call sites (lines 66, 168, 190, 230, 271)
• api/telemetry.rs — 3 call sites (lines 83, 134, 185)
• api/stream.rs — 1 call site (line 74)
• middleware.rs — 1 call site (line 68)
• api/ingestion.rs — 6 call sites (no context, but need to verify they work with new return type)

For ingestion.rs where open_ledger() is called without .context(), those will "just work" since ? can convert AppError directly (handler return type is already Result<_, AppError>).

Step 5: Fix Chronicle Feature Guard (analytics.rs, metrics.rs)

Replace anyhow::anyhow!("chronicle feature not enabled").into() with AppError::NotImplemented:

In analytics.rs (3 occurrences: get_recap, get_recap_cached, get_overview, get_projects):

// Before
return Err(anyhow::anyhow!("chronicle feature not enabled").into());

// After
return Err(AppError::NotImplemented("chronicle feature not enabled".into()));

In metrics.rs (2 occurrences: get_metrics_overview, get_metrics_trends):

// Same change
return Err(AppError::NotImplemented("chronicle feature not enabled".into()));

Step 6: Fix reconstruct_snapshot Internal Error (snapshots.rs)

Change the "event not found" error from Internal to NotFound:

// Before
.ok_or_else(|| AppError::Internal(anyhow::anyhow!("event {} not found", row.event_id)))?;

// After
.ok_or_else(|| AppError::NotFound(format!("event {} not found", row.event_id)))?;

Step 7: Add Tests

Add tests to crates/edda-serve/src/lib.rs in the existing #[cfg(test)] mod tests block:

1. Test 503 for database locked — This is hard to simulate in a unit test without concurrent access. Consider a structural test that verifies classify_open_error returns ServiceUnavailable for the right error message:

   #[test]
   fn classify_open_error_database_locked() {
       let err = anyhow::anyhow!("database is locked");
       match classify_open_error(err) {
           AppError::ServiceUnavailable(_) => {}
           other => panic!("expected ServiceUnavailable, got {:?}", other),
       }
   }

2. Test 404 for missing workspace — Verify that calling an endpoint with a non-edda repo root returns 404:

   #[tokio::test]
   async fn status_returns_404_for_non_edda_workspace() {
       let tmp = tempfile::tempdir().unwrap();
       // Do NOT run edda init — leave it as a bare directory
       let app = router_raw(tmp.path()); // needs a variant that doesn't init
       let resp = app.oneshot(
           Request::get("/api/status").body(Body::empty()).unwrap()
       ).await.unwrap();
       assert_eq!(resp.status(), StatusCode::NOT_FOUND);
   }

3. Test 501 for chronicle not enabled — Call /api/recap without chronicle:

   #[tokio::test]
   async fn recap_returns_501_without_chronicle() {
       let tmp = tempfile::tempdir().unwrap();
       edda_ledger::Ledger::open_or_init(tmp.path()).unwrap();
       let app = router(tmp.path()); // chronicle is None
       let resp = app.oneshot(
           Request::get("/api/recap").body(Body::empty()).unwrap()
       ).await.unwrap();
       assert_eq!(resp.status(), StatusCode::NOT_IMPLEMENTED);
   }

4. Test classify_open_error fallthrough — Unknown errors still become 500:

   #[test]
   fn classify_open_error_unknown_becomes_internal() {
       let err = anyhow::anyhow!("some random error");
       match classify_open_error(err) {
           AppError::Internal(_) => {}
           other => panic!("expected Internal, got {:?}", other),
       }
   }

Step 8: Verify and Clean Up

1. Run cargo fmt --check and cargo clippy
2. Run cargo test -p edda-serve
3. Run cargo test --workspace
4. Remove temporary files (research-379.md, innovate-379.md, plan-379.md)

File Change Summary

File	Change Type	Description
crates/edda-serve/src/error.rs	Modify	Add ServiceUnavailable, NotImplemented variants + classify_open_error helper + updated IntoResponse
crates/edda-serve/src/state.rs	Modify	Change open_ledger() return type to Result<Ledger, AppError>
crates/edda-serve/src/api/events.rs	Modify	Remove .context() from open_ledger() calls
crates/edda-serve/src/api/auth.rs	Modify	Remove .context() from open_ledger() calls
crates/edda-serve/src/api/briefs.rs	Modify	Remove .context() from open_ledger() calls
crates/edda-serve/src/api/drafts.rs	Modify	Remove .context() from open_ledger() calls
crates/edda-serve/src/api/metrics.rs	Modify	Remove .context() from open_ledger() calls + NotImplemented for chronicle guard
crates/edda-serve/src/api/analytics.rs	Modify	NotImplemented for chronicle guard
crates/edda-serve/src/api/snapshots.rs	Modify	Remove .context() from open_ledger() calls + NotFound for missing event
crates/edda-serve/src/api/telemetry.rs	Modify	Remove .context() from open_ledger() calls
crates/edda-serve/src/api/stream.rs	Modify	Remove .context() from open_ledger() call
crates/edda-serve/src/middleware.rs	Modify	Remove .context() from open_ledger() call
crates/edda-serve/src/lib.rs	Modify	Add tests

Total: 13 files modified, 0 new files.

Risk Assessment

• Low risk: Adding new enum variants is backward-compatible for API consumers (new status codes that weren't returned before)
• Medium risk: Changing open_ledger() return type touches all handlers, but the change is mechanical (remove .context())
• Gate 3: This touches 13 files across the serve layer but stays within a single crate. The API contract change (new status codes) is additive, not breaking. The module-level contract (open_ledger() signature) changes but is pub(crate).

Test Strategy

Following /testing conventions:

• Unit test classify_open_error for each classification path
• Integration test the HTTP status codes via tower::ServiceExt::oneshot (existing pattern in lib.rs)
• Test both the happy path (existing tests still pass) and error paths (new 404, 501, 503)
• No mocking — use real temp directories with/without edda init

[fagemx]

Work completed. PR created: #384

All CI checks pending — 112 tests pass locally, clippy/fmt clean.