Add ProviderError::Indeterminate for a submit that timed out after sending

[stephane-segning]

Summary

• crates/sms-provider/src/error.rs — new ProviderError::Indeterminate { message } variant and RoutingConsequence::HoldIndeterminate, so the routing() match stays a compiler-checked total function.
• crates/sms-provider-orange-cm/src/lib.rs — a new classify_transport_error that sorts a .send() failure into Unavailable (safe to retry) vs Indeterminate (unsafe) by connect-vs-post-connect phase; a 2xx with an unparseable body or a missing resourceURL is now Indeterminate too, not Unavailable. OrangeCmConfig gains connect_timeout/request_timeout (defaulted in production()) so tests can force a real, deterministic timeout instead of waiting on the 10s/30s production values.
• crates/sms-worker/src/dispatch.rs — classify() routes Indeterminate to routed -> uncertain with no backoff (no next attempt); write_transition gained a provider_ref_alt parameter so the Indeterminate path stamps Message.providerMessageRefAlt.
• docs/architecture.md — §2.10's transition table and §7.4's mermaid diagram both gain routed -> uncertain; §6.1's illustrative ProviderError code sample updated to match.
• schema/migrations/postgres/0002_bootstrap/up.sql — regenerated via ci/gen-bootstrap-sql.py (one-line diff: the new transition row). Not hand-edited.
• crates/sms-worker/tests/dispatch_live_postgres.rs — three new live tests proving the state transition, the no-double-submit guarantee, the connect-failure non-regression, and DLR resolution.

Intent

Source of truth: AGENTS.md's own documented gap — app/sms-worker/tests/kill9_reclaim_live.rs already pins as a known, accepted limitation that a crash-and-resubmit in a certain window produces two real outbound SMS, because "providerMessageRef has no DB-level uniqueness constraint and nothing today gives Orange a dedup key." This PR addresses the sibling case: a submit that times out after the HTTP request was already written to the wire was, before this change, classified identically to "provider is down" (ProviderError::Unavailable), and dispatch's own classify() sends every Unavailable back to queued for another attempt — i.e. a guaranteed resubmit on exactly the request shape most likely to have already succeeded once.

Scope

• In scope: a new ProviderError variant, the Orange adapter's transport-error classification, the routed -> uncertain state edge (doc + regenerated bootstrap SQL), driving it from dispatch, and deterministic wiremock-backed tests.
• Explicitly out of scope (per the assignment): a chaos-testing harness — that's a follow-up. Also out of scope, and unchanged by this PR: providerMessageRef's missing DB-level uniqueness/dedup key (the kill9_reclaim_live.rs-documented gap), and any change to undelivered -> queued retry (still undriven, per crates/sms-api/src/dlr.rs's own module doc).

Verification

just check       # green
just lint         # fmt --check + clippy -D warnings, green
just test          # 0 failed across the whole non-live suite
just parity        # message: 25 edges, job: 8 edges, diagram and table agree
./ci/assert-no-raw-sqlx.sh   # R1 OK
just test-live     # run twice, see below

The reqwest predicate, and why it's correct:

fn classify_transport_error(error: &reqwest::Error) -> ProviderError {
    if error.is_connect() {
        return ProviderError::Unavailable { .. };   // never wrote a byte — safe
    }
    if error.is_timeout() || error.is_body() {
        return ProviderError::Indeterminate { .. }; // past connect — unsafe
    }
    ProviderError::Unavailable { .. }                // Builder/Request-kind — never sent
}

reqwest::Error::is_connect() is true only for a failure establishing the connection itself (DNS, TCP handshake, TLS handshake — including a connect-phase timeout), verified by reading reqwest 0.12.28's own source (is_connect walks the error chain for a hyper_util::client::legacy::Error reporting connect). At that point this adapter has not written a single byte onto a socket Orange controls, so retrying is exactly as safe as it always was. Checked first and returns early.

is_timeout() is true for a timeout at either phase; by construction (the is_connect branch already returned) reaching this check means the connection was already established and the timeout fired while writing the request body or waiting on Orange's response. .json(&body) fully buffers the request before .send() starts writing it, so by the time this fires the write is complete or in progress — Orange's server may already have the full request. is_body() covers the same "past connect" territory from a different failure shape (the connection was reset/closed mid-transfer rather than the client's own timeout firing) — grouped with the timeout case for the same reason.

Verified against real reqwest::Error values, not synthetic ones: a_connect_refusal_is_still_unavailable binds an ephemeral TCP port, drops the listener (so the address is valid but refuses every connection), and asserts error.is_connect() before checking the classification; a_post_connect_timeout_is_indeterminate uses a real wiremock server with a response delayed past the client's own timeout, and asserts !error.is_connect() as a test-setup guard before checking the classification — so a broken predicate would fail the test's own setup assertion, not just the final matches!.

The 2xx-but-malformed-body decision: yes, Indeterminate, not Unavailable. Orange returning 201 means the submission was accepted — full stop — regardless of whether we can then parse the confirmation body. Two new unit tests (submit_returns_201_but_an_unparseable_body_is_indeterminate, submit_returns_201_but_a_missing_resource_url_is_indeterminate) cover this directly.

The state edge — both doc surfaces changed, bootstrap regenerated, not hand-edited:

-    ('routed','expired'),       ('routed','cancelled'),
+    ('routed','expired'),       ('routed','cancelled'),     ('routed','uncertain'),

That's the entire diff ci/gen-bootstrap-sql.py schema/migrations/postgres/0002_bootstrap/up.sql produced after editing §2.10's SQL block and §7.4's mermaid diagram in docs/architecture.md — confirmed this change touches only 0002_bootstrap (hand-written SQL), not schema.cstack, so no cratestack migrate diff / 0001_init regeneration was needed. ci/assert-state-machine-parity.py passes: message: 25 edges, diagram and table agree.

The providerMessageRefAlt correlation claim — did NOT hold on the first pass, found live, then fixed:

The assignment asked me to verify this claim against crates/sms-api/src/dlr.rs rather than assume it, and specifically to report plainly if it didn't hold. It did not, initially. OrangeCmProvider::submit only returns a SubmitAck (which carries provider_ref/provider_ref_alt) on success; on an Indeterminate failure there is no SubmitAck at all, and the original write_transition never wrote anything to providerMessageRef/providerMessageRefAlt. My first version of the live DLR test (a_dlr_after_an_indeterminate_submit_still_correlates_and_resolves) failed exactly this way — the message stayed uncertain because sms_api::dlr::ingest_one's correlation query (providerId + providerMessageRef OR providerMessageRefAlt) found nothing to match.

Fixed by having dispatch.rs stamp providerMessageRefAlt = message.id on the Indeterminate path specifically: SubmitRequest::reference (always message.id) is sent to Orange as receiptRequest.callbackData before the network call that might time out, so it's known regardless of whether a response ever comes back — unlike SubmitAck's fields, which only exist on success. With that fix, a_dlr_after_an_indeterminate_submit_still_correlates_and_resolves passes: a synthetic DLR echoing that same reference now correlates and drives the message from uncertain to delivered.

A second, unrelated bug found live while writing these tests, worth flagging explicitly: the shared sms-test-support Postgres container (from #118) reuses its template database across runs, and its migration check only looks for public.messages's existence — not migration currency. Since this PR's schema change adds a row rather than a table, the container from a prior session did not pick up the new routed -> uncertain transition until I ran just test-live-clean and let it re-migrate from scratch. Both of my first two live-test failures traced back to this (a stale DB, not a code bug) — worth calling out since it'll bite the next schema-only bootstrap change the same way.

Live suite, run twice, from a freshly re-migrated container:

suite	tests
dlr_ingestion_live_postgres	5
errors_live_postgres	3
policy_golden_list_live_postgres	6
send_message_live_postgres	9
sms-auth live_postgres	3
oidc_flow_live	1
provision_app_client_live_postgres	2
rbac_layer2_live_postgres	7
m1_acceptance_gate_live_postgres	1
claim_live_postgres	7
dispatch_live_postgres	8 (5 pre-existing + 3 new)
jobs_live_postgres	9
sms-worker live_postgres	4
kill9_reclaim_live	1

66 tests, 14/14 suites green, on both the run immediately after test-live-clean and a second, fully independent rerun. One transient failure on the very first (pre-clean-container) attempt — sms-auth's rbac_layer2_live_postgres::a_route_this_middleware_does_not_gate_is_unaffected hit duplicate key value violates unique constraint "pg_type_typname_nsp_index", the pre-existing, previously-documented pg_type-cache race (#102) — passed cleanly on its own in isolation immediately after; not a regression from this change (I touched no sms-auth code).

Containers: 12 before I started (11 of the developer's own unrelated tenant-provisioner-*/vsms-test-harness-postgres containers were already present from prior work) → just test-live-clean removed the one stale harness container (11) → both full just test-live runs left the count at 12 (11 unrelated + 1 harness), no growth across either run.

Screenshots/Evidence

N/A — backend state-machine/error-taxonomy change, no UI surface. Evidence is the test output above and the diff itself.

Risk Assessment

Low for the code paths touched. The new variant only activates on a specific, narrow transport shape (post-connect timeout/body error, or a malformed 2xx) that did not previously have any handling more correct than "treat as Unavailable" — so this is strictly a refinement of existing error handling, not a new failure mode. routed -> uncertain is an additive state-machine edge; nothing that previously reached uncertain (via DLR) changes behavior.

The tradeoff, named plainly, not hidden: this deliberately trades a possible lost message for never sending a duplicate. If a message reaches uncertain via this path and Orange genuinely never sent it (a true, non-transient failure that merely looked like a timeout), no DLR will ever arrive, and the message sits until expire_stale's 6-hour grace (crates/sms-worker/src/jobs/expire_stale.rs, already handles uncertain — confirmed, unchanged by this PR) reaps it to expired rather than being retried quickly. That's the accepted cost of closing the double-send gap: a slower failure mode for the rare case, instead of a real duplicate SMS to a real handset for the more consequential one.

Also unverified, honestly: whether Orange's real production behavior actually matches the "connect vs. post-connect" model this predicate assumes — this repo has no Orange sandbox credentials (documented pre-existing limitation, crates/sms-provider-orange-cm/src/lib.rs's own module doc), so everything here is proven against wiremock, not a real Orange endpoint under real network conditions.

AI Usage Declaration

Implemented end-to-end by Claude Code (Claude Sonnet 5) under my direction: reading the existing ProviderError/dispatch/dlr code first rather than trusting the assignment's own paraphrase of it (which named a nonexistent RetryThisProvider ProviderError variant — that name only exists on RoutingConsequence; the actual five pre-existing ProviderError variants are Permanent/Transient/Unavailable/Rejected/Unsupported), designing and implementing the fix, and verifying it against a live Postgres and real reqwest errors rather than trusting a green cargo check alone.

• I reviewed the connect-vs-timeout reqwest predicate and the reasoning behind it, and independently verified it against reqwest 0.12.28's actual source (error.rs) rather than trusting my own or the tooling's assumption about its semantics.
• I confirmed the providerMessageRefAlt correlation claim by writing a live test that initially failed, diagnosed why, and fixed the actual gap rather than adjusting the test to hide it — see "Verification" above.
• I ran just test-live against real Docker/Postgres, twice, and take responsibility for the pass counts and container counts reported above being copied from actual command output, not estimated.

Reviewer Focus

• Whether is_timeout() || is_body() is the right boundary, or whether is_body() deserves its own, more specific branch — I grouped it with the timeout case on the reasoning that both mean "past the connect phase," but is_body() can in principle fire for a request-body streaming error before any bytes reached the server in some hyper internals I did not independently trace line-by-line (this adapter's request body is fully buffered via .json(), which is why I believe this doesn't apply here — but a second pair of eyes on that assumption specifically would be valuable).
• Whether stamping providerMessageRefAlt only on the Indeterminate path (not on Transient/Unavailable too) is the right scope — I limited it to the one case that actually needs future DLR correlation, but flag it in case a reviewer sees value in doing this unconditionally for future-proofing.
• The stale-test-container finding (sms-test-support's migration check not detecting bootstrap-SQL-only changes) is real and will recur on the next such change — worth deciding whether to file it as a follow-up against Add a testcontainers-backed live-Postgres harness; wire it into CI (#102) #118 rather than leaving it as tribal knowledge in this PR body.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 3815e09

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR