Add a fault-injecting fake of Orange and a chaos suite over the message state machine - #121
Merged
Merged
Conversation
… machine
crates/sms-fake-orange is a participant in the send/DLR lifecycle, not a
response stub: it answers the token/submit HTTP calls per a FaultPolicy
(Scripted for deterministic CI-gate tests, Seeded for a reproducible
weighted-random mix) and independently schedules DLR deliveries as real
HTTP POSTs against a caller-supplied /dlr/{providerKey} endpoint, plus a
request ledger queryable from the provider's own side of the wire.
crates/sms-worker/tests/chaos_live_postgres.rs hand-rolls the same small
POST /dlr/{providerKey} handler app/sms-gateway/src/dlr.rs owns in
production (app/sms-gateway is a binary crate with no lib.rs) against a
real axum::serve on an ephemeral port, so DLR delivery is a genuine HTTP
round trip. Ten scripted tests cover one fault mode each; five seeded
sweeps (fixed seeds 1-5) assert invariants over a weighted-random fault
mix rather than exact outcomes. Connection-level nastiness (RST, refused
connections, byte-dribble) is explicitly out of scope - noted as future
work, not half-done.
This is the automatable complement to docs/runbooks/36-handset-gate.md
and does not close #36: it cannot prove anything about Orange's real DLR
payload shape, receiptRequest honouring, or a real handset.
Two real findings surfaced by actually running the suite, not invented:
DeliveryReceipt's list/detail policy was missing hasRole('system') - the
seventh instance of a gap shape this codebase has now found six times
before - fixed in schema.cstack only (an @@Allow change never touches
DDL, confirmed both by #100's own precedent and by diffing this repo's
own migration output before/after). Second, the seeded sweep empirically
confirms dlr.rs's own documented `undelivered` gap is real: nothing
drives undelivered -> queued and expire_stale doesn't reap it either, so
a message can sit in undelivered forever under current scope - the
sweep's own invariant treats that as accepted, not a new bug.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
4 tasks
stephane-segning
added a commit
that referenced
this pull request
Aug 7, 2026
…NTS.md (#123) Four PRs landed in one session (#118 harness, #120 gate stabilisation, #119 ProviderError::Indeterminate, #121 fake Orange + chaos suite) and the context file only picked up what the last two agents wrote inline. The biggest omission was the most consequential change: `sms-test-support` and the fact that CI now runs the 14 live-Postgres suites at all — previously `cargo test --workspace` ran without `--ignored`, so they were skipped in CI and had only ever run by hand. Records, in each case why it exists rather than just that it does: - The harness's five load-bearing constraints, each learned by something breaking: no container handle in a `static` (Drop never runs for statics, which leaked 56 containers), label-scoped cleanup only (an image-scoped sweep would destroy a developer's own database), one database per test binary (a shared one let a real dispatch subprocess steal other suites' messages), migration-content fingerprinting (an existence check served a stale schema silently), and the global container name that makes concurrent test runs corrupt each other. - That `ca653a1`'s mutex was never applied to two suites, both of which flaked in CI within hours of #118 running them for the first time. - `Indeterminate`'s connect-vs-read predicate, the `routed -> uncertain` edge it needed, and the fact that it trades a possibly lost message for never sending a duplicate — a product decision, not a free win. - A CLI-skew warning placed at both sites that tell you to run `cratestack migrate diff`, since the installed 0.7.4 CLI emits DDL the pinned 0.6.7 library never does. The warning previously existed only in an unrelated paragraph 150 lines away from the commands themselves. - The missing-`hasRole('system')` gap as an explicitly *unguarded* failure mode, now found seven times. The three entries above it each turned a silent failure loud; this one has no equivalent, which is why it recurs. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crates/sms-fake-orange— a fault-injecting fake of Orange Cameroon's SMS HTTP API. A participant in the send/DLR lifecycle (inbound stubbing, an autonomous DLR scheduler over real HTTP, a request ledger), not a response stub — see the crate's own module doc.crates/sms-worker/tests/chaos_live_postgres.rs— 10 scripted tests (one fault mode each) plus a 5-seed (1–5) seeded chaos sweep asserting invariants over the message state machine, wired into the existingliveCI job automatically (it's a normal*_live_postgres.rssuite,cargo test --workspace -- --ignoredalready picks it up — no workflow YAML change needed).schema/schema.cstack—DeliveryReceipt.list/.detailgains|| hasRole('system'), the same fix shape applied six times before to other models. Found live by this PR's owna_duplicate_delivered_dlr_is_idempotenttest.AGENTS.md— records the new suite, both real findings below, and an environment note about a CLI/library version skew encountered while verifying the schema change.Intent
Source of truth: this PR's own assignment brief — build the automatable complement to
docs/runbooks/36-handset-gate.mdby fuzzing the message state machine against a fault-injecting fake of Orange, per #36's own framing that "lease reclamation is the kind of code that is easy to write and easy to never actually exercise" (the same reasoning #114's kill-9 gate already applied to lease reclaim; this PR applies it to the DLR/submit fault space instead).This PR does not close #36 and does not claim to. A mock cannot tell you Orange's real DLR payload shape, whether
receiptRequestis genuinely honoured, or whether a handset ever buzzes —docs/runbooks/36-handset-gate.mdstays the actual gate for that, unchanged by this PR.Scope
In scope:
Scripted(exact ordered sequence, for deterministic tests) andSeeded(a seeded PRNG drawing a realistic-weighted mix — reproducible by construction, seed named in every assertion).tokiotasks POST realdeliveryInfoNotificationbodies to a genuine, real, in-processPOST /dlr/{providerKey}route (hand-rolled here sinceapp/sms-gatewayis a binary crate with nolib.rs— same constraintoidc_flow_live.rsalready worked around, same fix shape).Indeterminate), 200 OK with no DLR ever, a DLR racing the submit response (the one the brief called out for "real attention" — see Verification), duplicate DLRs, out-of-order DLRs, DLRs for an unknown ref, DLRs after a terminal state, 429 (with recovery), 5xx, a malformed/missing-resourceURL201, and a 401 at the token endpoint.wiremock'sRespondtrait can only ever emit a well-formed HTTP response, delayed or not; modelling a severed connection needs connection-level control this crate doesn't take on. Noted as future work in the crate's own module doc, not half-done. The connect-refused half of that same distinction is already covered directly, against a real socket, bysms-provider-orange-cm's owna_connect_refusal_is_still_unavailable— this PR doesn't re-prove it.Out of scope, deliberately: no change to
crates/sms-provider-orange-cmorcrates/sms-worker/src/dispatch.rs— the fake exercises existing, already-shipped classification logic, it doesn't change it. The one production change (schema.cstack'sDeliveryReceiptpolicy) is the smallest fix that unblocks this PR's own test from reading back what it just wrote, following an established six-times-precedented pattern rather than inventing a new one.Verification
just check,just lint(fmt + clippy-D warnings),just test,just parity,./ci/assert-no-raw-sqlx.sh— all green, workspace-wide, including the new crate and test file.cargo metadata --locked— green,Cargo.lockup to date.Migration safety for the
schema.cstackedit: an@@allowchange never touches DDL (@@allowcompiles into generated Rust policy code viainclude_server_schema!, not SQL) — confirmed two ways: (1)#100's own commit message documents its analogousJobpolicy fix as producing a byte-identical migration diff, and (2)cratestack migrate diffwas actually rerun against both the pre- and post-edit schema here. It turned out this machine's globally installedcratestackCLI is0.7.4, newer than the pinned library family (=0.6.7), and emits foreign-keyALTER TABLEstatements the pinned emitter never has — confirmed this is pre-existing CLI/library skew, unrelated to this PR's edit, by rerunning the identical diff against the unmodifiedmainschema and finding it already disagrees with the committed0001_initthe same way. The committed migrations were left untouched, correctly, on the strength of the policy-only-change reasoning above rather than regenerated with a mismatched CLI. Recorded inAGENTS.mdas an environment note for whoever hits this next.Three full
just test-liveruns (cargo test --workspace -- --ignored), container-count-verified before and after each:chaos_live_postgresEvery other live suite (
claim_live_postgres,dispatch_live_postgres,jobs_live_postgres,send_message_live_postgres,dlr_ingestion_live_postgres,policy_golden_list_live_postgres,errors_live_postgres,kill9_reclaim_live,m1_acceptance_gate_live_postgres,oidc_flow_live,provision_app_client_live_postgres,rbac_layer2_live_postgres,live_postgres×2) stayed green across all 3 runs too — no regression from theDeliveryReceiptpolicy change.Container count: 12 before, 12 after every run (
docker ps -a), the onevsms-test-harness-postgresreused (not recreated) each time, the pre-existingtenant-provisioner-*stack (11 containers) never touched. Scoped throughout by the harness's owndev.vsms.test-harness=truelabel, never by image or bare name.Added CI runtime:
chaos_live_postgresitself runs in ~7.4s per the table above — comfortably inside the ~60s budget for the live job (whose 15 non-chaos suites summed to roughly 85s of the ~93s total per run above, so the chaos suite's own share is small relative to the existing job, not the dominant cost).The race scenario proven, not just modelled:
a_dlr_racing_the_submit_response_is_dropped_then_the_message_expiresschedules aDeliveredDLR with zero delay against a submit response deliberately held open for 120ms. At the moment the DLR arrives,Message.providerMessageRefandproviderMessageRefAltare both stillNULL(the latter is only stamped bywrite_submitted/write_transition, both of which run strictly after the submit HTTP response is processed) — sosms_api::dlr::ingest_one's correlation query matches nothing, the DLR is silently dropped, and the message reachessubmittednormally moments later with no further DLR ever arriving. The test then forcesexpire_staleforward and confirms the message reaches a real terminal state (expired) rather than sitting stuck or being wrongly resolved todeliveredby a receipt that shouldn't have matched. This is the scenario the assignment brief called out for "real attention," and it's now a permanent regression assertion, not a one-off finding.Invariant list actually asserted by the seeded sweep (
run_seed's own doc has the full reasoning): no message lost (terminal, or the documentedundeliveredgap below); nothing left inaccepted/queued/routedafter the tick loop settles;attempts <= maxAttempts; and — checked against the fake's own ledger, not this system's database — a message that wentuncertainvia anIndeterminate-shaped submit call is never submitted again afterward (#119's own guarantee).Two real findings — not invented
DeliveryReceipt.list/.detailwas missinghasRole('system')— the seventh instance of the exact policy-gap shape this codebase has now found six times before (App,AppClient,SenderIdRegistration,OperatorPrefixRule,Provider,Job). Found bya_duplicate_delivered_dlr_is_idempotenttrying to read backDeliveryReceiptrows under asystemcontext after two DLRs landed, and getting zero rows instead of two. Fixed the same way as every prior instance, inschema.cstackonly. Not seed-reproducible (a schema bug, not a fault-injection finding), but a direct, real defect the chaos suite's own assertions surfaced by actually running against a live database.undeliveredgapdlr.rs's own module doc already documents in prose is empirically real, not just a documented intention: nothing currently drivesundelivered -> queued, andexpire_staleonly reapssubmitted/uncertain, neverundelivered— so a message that receives exactly one retryable-failure DLR (submitted -> undelivered) and no follow-up sits inundeliveredforever under this system's current scope. The seeded sweep's own final-state invariant treats this as an accepted, already-documented outcome rather than a sweep failure — re-litigating a known gap would have been the wrong response, and the sweep's own reasoning for why is spelled out inline inrun_seed's doc comment.No SM001/illegal-transition surfaced as a real failure in any of the 3×5=15 seeded runs, and none of the 3×10=30 scripted runs. Finding none of that particular class is itself a real, reportable (not invented) result.
Risk Assessment
Low. The only production-code change is the single-model, two-line
schema.cstackpolicy addition — same shape, same verification discipline (byte-identical-DDL check) as six prior instances of this exact fix in this codebase. Everything else is new, test-only code: a new dev-only crate (sms-fake-orange, never depended on by any production binary — same discipline assms-test-support) and a new#[ignore]d live test file that only runs under--ignored, exactly like every other live suite in this workspace.AI Usage Declaration
This PR was implemented by Claude Code (Claude Sonnet 5) under the user's direction, from a detailed assignment brief specifying the fake's design (three responsibilities: inbound stubbing, an autonomous DLR scheduler, a request ledger), the fault taxonomy to prioritise, the invariants to assert, and the verification bar.
ProviderError/RoutingConsequence, the realOrangeCmProvideradapter and its own transport-error classification,dispatch::tick's claim-then-submit split,claim.rs's no-real-leaseaccepted -> queuedhop,dlr.rs's correlation logic and its own documentedundeliveredgap, the message state-transition table) rather than assuming any of it from the brief's own prose, which the brief itself warned may contain errors (and did — flaggingRoutingConsequence::RetryThisProvideras belonging toRoutingConsequence, notProviderError, matches what I verified against the source).cheapest_active_providerhas no tie-breaker and every test in this file seeds a provider at the same fixed cost, so a later test could route to an earlier test's row and its DLR would then correlate against the wrongprovider_row_id) by reproducing it live (4 of 16 tests genuinely failed on the first real run against Postgres), root-causing it againstdispatch_live_postgres.rs's own already-documented instance of the identical shape, and reverifying the fix with 3 full clean reruns — not by reasoning about it in the abstract.Reviewer Focus
POST /dlr/{providerKey}a second time (a third time countingapp/sms-gateway/src/dlr.rsitself) inside the test file is the right call, versus givingapp/sms-gatewayalib.rsat some point so every live suite that needs its real routes stops re-deriving them. Out of scope for this PR (the existingoidc_flow_live.rsprecedent already made this call), but worth a second opinion given this is now the second time it's been duplicated.Seededfault-weight distribution incrates/sms-fake-orange/src/fault.rs(seeded_decision/accepted_with_random_dlr) — chosen to be realistic-skewed per §6.1's own framing, but the exact percentages are a judgement call, not derived from any real traffic data (none exists yet for this system).DeliveryReceiptpolicy fix should also extend to anappId-scoped clause (the wayMessage's own policy hasappId == auth().appId) rather than justhasRole('system')— out of scope here since nothing in this deployment currently needs an app-scoped read ofDeliveryReceipt, but it's the same shapeMessage's policy already has and a future admin-console read path will likely want it.