guru-connect/docs/specs/SPEC-008-valuable-error-messages.md

SPEC-008: Valuable Error Messages (Structured Errors + No Silent Swallows)

Status: Proposed Priority: P2 Requested By: Mike (2026-05-30) Estimated Effort: Large (phaseable)

Overview

Make every error in GuruConnect carry information a human can act on: a single structured API error envelope with a stable code and a correlation id that also appears in the server logs, consistent contextual logging on the server and agent, an end to silent error swallowing, and a dashboard that shows the real cause (with the id to quote to support) instead of a hardcoded generic line. Success = given any error a tech sees, they (or we) can determine what failed and why — and a user-reported "it broke" comes with a correlation id that pulls the exact server log line.

This is a cross-cutting quality initiative. A parallel feature request governs GuruRMM (both are Rust/Axum); keep the conventions consistent where practical, but this spec governs GuruConnect only.

Current state (researched — this is the problem)

• Inconsistent API error shapes. Bare (StatusCode, &str) tuples (server/src/api/mod.rs:101,106), a per-file ErrorResponse struct (server/src/api/auth.rs:36), and two different JSON envelopes in the wild — {error} and {detail, error_code, status_code} — which the dashboard already has to union to cope (dashboard/src/api/client.ts:48-52). No single error type.
• No correlation/request id. Only TraceLayer::new_for_http() (server/src/main.rs:471); nothing ties a response a user saw to a log line.
• 37 let _ = swallows in server/src/ — many on fallible DB ops: create_session (relay/mod.rs:594), end_session (:686), log_event (:132,191,303,604), mark_machine_offline (:687), link_session_to_code (:640). This is the exact pattern that hid the migration-005 missing-columns bug — update_machine_metadata failed silently for an unknown period.
• Generic dashboard message. MachinesPage.tsx:202 shows "The GuruConnect server did not respond. Check the relay status, then retry." regardless of cause (auth expired? 500? network? CORS?), even though ApiError (client.ts:12) already carries status + code.
• Thin agent errors. The GuruConnect-client auth-failure-loop incident (2026-05-30) produced no useful local signal about why auth failed or which server it was hitting.

Scope

Included in v1

• One structured API error type. A server AppError enum (thiserror) with a single IntoResponse producing one envelope: { error_code: string, message: string, detail?: object, correlation_id: string } + consistent HTTP status mapping per variant. Replace the ad-hoc tuples and per-file ErrorResponse; converge the two envelopes to one.
• Correlation-id middleware. Generate (or accept inbound x-request-id) a request id, put it on the tracing span, echo it in the response header and in every AppError body, so a reported id greps directly to the log.
• Logging convention. Errors logged via tracing include the operation + relevant identifiers (agent_id/machine_uid/session_id) + the underlying error chain (anyhow context / thiserror source) — never a bare "failed". Use #[instrument] span fields so context is attached automatically.
• Kill silent swallows. Sweep all let _ = <fallible>(): convert to if let Err(e) = … { warn!/error!(error = %e, op = "…", id = …) }. Where fire-and-forget is genuinely correct (e.g. best-effort socket close on an already-dead connection), keep it but add a one-line comment stating why. A clippy lint / CI grep guards against new bare let _ = on Result.
• Dashboard surfaces the real error. Render ApiError.code + message + correlation_id ("quote this to support: …"); distinguish network-down (fetch threw) vs auth-expired (401 → re-login) vs server-500. Remove the hardcoded generic at MachinesPage.tsx:202; reuse one error component across pages.
• Agent error detail. Connection/auth/capture failures log what failed and why — the target server URL, the relay's stated reason, the auth mode attempted — so a misbehaving agent is self-diagnosing (directly addresses the auth-loop incident).

Explicitly out of scope

• Centralized log aggregation / external error tracking (Sentry-style) — local structured logs + correlation id only in v1.
• User-facing localized error catalog / i18n.
• Changing protobuf-level agent↔relay error frames beyond adding context to existing disconnect reasons (a deeper protocol error model is a later item).
• Rewriting business logic — this is error plumbing/observability, not behavior change.

Architecture

• Relay-server: new server/src/error.rs (AppError + IntoResponse + status map); handlers return Result<T, AppError>; correlation-id middleware in server/src/main.rs (layer beside TraceLayer, :471). relay/mod.rs swallows fixed in place with context logging.
• Agent: anyhow context added at error boundaries in agent/src/transport/ and agent/src/session/; connection/auth failures error!-logged with server URL + reason.
• Dashboard: dashboard/src/api/client.ts keeps/extends ApiError to carry correlation_id; a shared ErrorBanner/ErrorState component; pages stop hardcoding messages.
• Protobuf/DB: none required (existing Disconnect.reason string can carry richer text; no schema change).

Implementation details

• Files to touch: server/src/error.rs (new); server/src/api/*.rs (adopt AppError, drop ErrorResponse/tuple returns — mod.rs:101,106, auth.rs:36); server/src/main.rs (correlation-id middleware, :471); server/src/relay/mod.rs (swallow sweep — :132,191, 303,594,604,640,686,687,695, …); agent/src/transport/*, agent/src/session/mod.rs (error context); dashboard/src/api/client.ts (correlation id), shared error component, dashboard/src/features/machines/MachinesPage.tsx:202 (use it).
• Stable error_code values are an enum (e.g. AUTH_EXPIRED, MACHINE_NOT_FOUND, DB_UNAVAILABLE, VALIDATION_FAILED) — documented and reused, not free-text.

Security considerations

• Don't leak internals to clients. The structured body carries a safe message + code; the full error chain / SQL / stack stays in the server log keyed by correlation id. A client gets "DB_UNAVAILABLE (id abc123)", not the connection string.
• Correlation id is a random opaque token (no embedded PII/sequence that leaks volume).
• Auth errors stay deliberately coarse to the client (e.g. don't reveal whether a username exists), while the log records the specific reason — valuable internally, safe externally.
• The swallow sweep must not start logging secrets (api keys, tokens) that were previously silently dropped — scrub identifiers, never log key material.

Testing strategy

• Unit: each AppError variant maps to the right status + code + envelope; correlation id present on every error body and matches the response header.
• Integration: a handler error returns the structured envelope with a correlation id that appears in the captured log; an inbound x-request-id is honored end-to-end. A forced DB failure on a former let _ = path now emits a warn!/error! (assert via log capture) instead of vanishing.
• Dashboard: 401 renders "session expired / re-login"; 500 renders code + correlation id; fetch-throw renders "can't reach server" — three distinct states, not one generic.
• CI guard: lint/grep fails the build on a new bare let _ = over a Result in server/.

Effort estimate & dependencies

• Size: Large but phaseable. Phase 1 (highest value): AppError + correlation-id middleware + the let _ = swallow sweep. Phase 2: dashboard error rendering. Phase 3: agent error context. Each ships independently.
• Depends on: nothing blocking. Synergizes with SPEC-004 (the agent-auth-loop diagnosis would have been trivial with agent error detail) and improves every other spec's debuggability.
• Unblocks: faster incident triage across the product; a real correlation-id workflow for support.

Open questions

1. Envelope migration. Converge to the single new envelope and update the dashboard union in lockstep, or keep accepting both for a deprecation window? (Dashboard already unions both — low risk to converge.)
2. error_code taxonomy. Agree the initial enum of stable codes and their HTTP status mapping.
3. Correlation id source. Generate server-side always, or trust an inbound x-request-id from the dashboard/NPM when present (and from where is it trusted)?
4. Swallow policy granularity. Blanket "log all let _ = on Result", or an allowlist for the genuinely best-effort socket sends? Proposed: log all DB/state ops; comment + keep best-effort socket closes.