Skip to content

Vault error handling branches on code instead of class — an unknown code is rejected as "config invalid" instead of paused #3

Description

@iceteaSA

Verified at b4543b4. Consumer-side defect in crates/synapse-module/src/remote/vault.rs; claustrum's surface already carries everything needed to fix it.

Summary

Claustrum's error contract says to branch on class, never code, because the code set grows and the class set does not. We branch on code in two places, and our fallback for an unrecognised code is not a neutral bucket — it rejects the job while blaming synapse's own configuration.

class is already present on every error frame we receive. We are discarding it, not missing it.

Site A — closed enum over codes, no fallback variant

#[derive(Deserialize)]
#[serde(rename_all = "snake_case")]
enum CredentialReadErrorCode {
    NotFound, NeedsReauth, RefreshUnsupported,
    RefreshFailed, VaultLocked, Corrupt, TooManyItems,
}

No #[serde(other)] variant. The enclosing CredentialGetOutcome is #[serde(untagged)] over {Success, AppError}, so an unrecognised code fails the AppError arm, then fails both arms, so serde_json::from_slice returns Err — which we map to VaultError::MalformedHandlesFile.

Note the shape: a new code does not reach a default branch, it fails parsing, so the resulting error names a malformed file when the response was perfectly well-formed.

Site B — string-matched codes with a cause-asserting catch-all

map_call_error() matches exactly "needs_reauth", "vault_locked", "not_found", then CallError::Module(_) => VaultError::MalformedHandlesFile.

Blast radius — worse than a mislabel

MalformedHandlesFile is not neutral. At crates/synapse-module/src/remote/runtime.rs:612-618:

VaultError::MalformedHandlesFile
| VaultError::MalformedHandle
| VaultError::NotFound => CredentialDisposition::Reject(
    RuntimeError::credential_config_invalid("credential handle configuration is invalid")),

while VaultLocked | NeedsReauth => CredentialDisposition::PauseJob.

So an unknown-to-us vault condition is:

  1. Rejected rather than paused — non-recoverable, the job dies;
  2. blamed on synapse's own config in the operator-facing string;
  3. when the true condition may be transient and repairable, whose correct disposition was PauseJob.

The operator reads "credential handle configuration is invalid", audits their handle config, finds nothing wrong, and never learns the vault said something new.

The generalisable defect: a catch-all that names a cause it cannot know. A default branch should report what is known ("unrecognised vault response"), not guess.

The fix — branch on class

Confirmed by Claustrum from their source (read_surface.rs:427): every error body carries both fields, class being the one intended for consumers.

pub struct ErrorBody { pub code: ReadError, pub class: ErrorClass }

Their class set is closed and golden-tested against the contract:

ERROR_CLASS_WIRE_SET: [&str; 4] = ["transient", "permanent", "auth_required", "context_overflow"];

A closed enum over class is legitimate precisely where a closed enum over code is not. Target mapping:

class disposition
transient retry (existing recoverable path)
auth_required PauseJob
permanent stop, do not retry
context_overflow not reachable on the credential path
unknown class PauseJob

unknown class -> PauseJob is safe here — verified, not assumed

Claustrum flagged the risk that pausing converts a dead job into a silently hung one if the pause is unbounded or invisible. Checked against our implementation; it does not apply:

  • Bounded at pause timepause_job_needs_reauth() (store.rs) sets resume_deadline_ms = now + resume_window_ms alongside state = paused_needs_reauth.
  • Enforced, not merely recorded — the expiry sweeper transitions them out:
    UPDATE jobs SET state = 'failed_permanent', terminal_at_ms = ?2, error_json = ?3
     WHERE state = 'paused_needs_reauth' AND resume_deadline_ms <= ?2
    There is a supporting index (jobs_resume_deadline_idx).
  • Resume is deadline-guarded — the resume path requires resume_deadline_ms > now.
  • Visible — the gateway surfaces StableError::needs_reauth() on the wire, and resume_deadline_ms is exposed on the job record.

Worst case a paused job becomes failed_permanent at its deadline. It cannot hang indefinitely.

Also worth fixing: the not_found operator string

Claustrum confirms not_found is a deliberate uniform mask — returned identically for never-existed, revoked, and not-servable handles, because distinguishing them would permit handle enumeration. That collapse is permanent on their side and we should not ask for a signal to split it.

What is ours to fix is the message. credential handle configuration is invalid asserts a cause we have no evidence for. Something like "the vault did not serve this handle — it may be revoked, unknown, or not servable" states what is actually known.

The property their contract cares about (not_found + permanent = authoritative stop, never retry-hammer) we already satisfy.

Non-issues, checked and cleared

  • Error-string leakage — we conform. CredentialReadError deserializes only { code }, map_call_error reads only body.code, and there is a regression test (credential_wire_decoder_never_exposes_payload_in_errors). Claustrum's hazard is that OAuth bodies echo submitted parameters; it cannot reach our logs on this path.
  • stale_pending — status-only, and correctly ignored by a get-only consumer like us. It predicts what a get will cost, so it has no value riding back on a get response. Confirmed by Claustrum from source; we call only credential.get and credential.report_auth_failure.

Cross-seam work agreed with Claustrum Legion via direct lane. Filed here rather than on their board: every violation is consumer-side.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions