fix(gateway): keep the routed error's SQLSTATE class - #360
Conversation
farhan-syah
left a comment
There was a problem hiding this comment.
Delegation verified: every arm the gateway dropped has an identical-class arm in error_to_sqlstate (NotLeader, NoLeader, DeadlineExceeded, CrossCollectionNotColocated, RemoteTyped, DataPlane, BadRequest, PlanError, CollectionNotFound). error_map lib tests 32 pass, clippy --lib -D warnings clean, fmt clean.
Blocker: an unchecked consumer of the changed message.
RetryableSchemaChanged on the routed path rendered schema changed during execution (X); please retry. It now renders the variant's Display, retryable schema change on X, still XX000. Three cluster tests classify a transient retry by that text:
nodedb-cluster-tests/tests/common_suite/cases/cross_node_pk_lookup.rs:71-72nodedb-cluster-tests/tests/common_suite/cases/cross_node_pk_write.rs:85-86nodedb-cluster-tests/tests/common_suite/cases/install_snapshot_e2e_cluster.rs:94-95
Their predicate is msg.contains("schema changed during execution") || msg.contains("please retry"). Neither matches the new text and XX000 matches nothing, so a lease conflict during those tests becomes a hard failure. CI here runs stage 1 only, so it never surfaced.
Direction: one message in one place. Put the retry wording in the variant's #[error(...)] (nodedb/src/error/types.rs:350) so the direct path, this mapper, and error_map/http.rs:26-29 (which duplicates the same literal) all render it from Display. The test predicates then match unchanged.
Should-fix: three doc comments narrate the pre-change state (inline).
After the change, rebuild the commits rather than appending fix-up commits on top.
| /// One table answers for the direct and the routed path. This mapper's 14 | ||
| /// arms restated the direct table, but its own catch-all sent the 26 | ||
| /// variants the direct table classifies and it did not list to `XX000`. | ||
| /// A client must not see a different class because a statement crossed the | ||
| /// gateway. |
There was a problem hiding this comment.
Narrates the pre-change state ("arms restated ... catch-all sent the 26 variants"). Comments state what is: one table answers for the direct and the routed path, so a client sees the same class either way.
|
All four addressed in
Evidence: |
farhan-syah
left a comment
There was a problem hiding this comment.
Round 2, 395a0594e re-verified: error_map 32 pass, clippy --all-targets -D warnings clean (covers the crash-harness edit), fmt clean.
| Round-1 point | Landed |
|---|---|
Blocker: wording at the variant's #[error] |
Yes — types.rs:351. The cluster-test predicates match. |
| Consumer trace | Yes, and further than asked: the crash-harness matcher keyed on the old Display text and is updated. |
| Three history comments | Yes, all restated as invariants. |
Still open — two copies of the wording remain, one now divergent. Neither file is in the diff, so no inline anchor:
nodedb/src/control/gateway/error_map/http.rs:28— a hand-writtenformat!of the same text. Round 1 asked that every path render from Display; this one still does not.nodedb/src/error_classify.rs:181—plan_error(format!("retryable schema change on {descriptor}"))carries the old wording. It matched Display before this PR and diverges after it. The classify path reaches clients through the bridge, so one path will say one thing and the other path another.
Both become err.to_string(). Then the variant's #[error] is the only source, which is what the new doc line on it claims.
After the change, rebuild the commits rather than appending fix-up commits on top.
395a059 to
3fd20e7
Compare
|
Round 2 addressed in the rebuilt commit (
Auditing the same invariant found three more consumers, all now rendering from Display:
The variant's Local evidence: |
GatewayErrorMap::to_pgwire restated 14 of the direct mapper's arms and sent every variant beyond them to XX000. The direct table classifies 26 of those, so a routed query reported an internal fault where the direct path reported the real class: constraint 23505, rate and quota rejections, undefined column/function/object, write conflict, memory exhaustion, fan-out, and the rest. The mapper now delegates to error_to_sqlstate, and the agreement test pins the contract for 24 of them.
…play The delegation moved this condition's message to the direct table's Display text, and three cluster tests key on 'schema changed during execution'. The wording now lives in the #[error(...)] attribute so every path renders it from Display: the HTTP, RESP, and native gateway maps, the classify table, the crash harness, and the descriptor-lease integration test all read that same text. The gateway comments state the invariant (one table answers both paths) instead of the pre-change layout.
3fd20e7 to
d854b04
Compare
|
Rebased onto Rebased-tree evidence: full |
Why
GatewayErrorMap::to_pgwirerestated 14 of the direct mapper's arms, and its own catch-all sent every variant beyond them toXX000. The direct table classifies 26 of those, so the same fault reached the client with a different class depending on whether the statement crossed the gateway: a constraint violation asXX000instead of23505, a rate rejection, an undefined column, a write conflict, memory exhaustion, a fan-out refusal, and more.Mechanical evidence:
crate::Errorhas 101 variants, the direct mapper 36 explicit arms, the gateway 14. The red run failed withleft: "XX000", right: "23505"onRejectedConstraint; the same test passes after delegation.What
to_pgwiredelegates toerror_to_sqlstate: one table answers for the direct and the routed path.XX000.RetryableSchemaChangedkeepsINTERNAL_ERRORon both paths; its message now comes from the variant'sDisplay, still naming the descriptor.How to test
cargo test -p nodedb --lib gateway::error_map— 30 pass (agreement + surface tests).cargo test -p nodedb --lib error_map— 32 pass.cargo clippy -p nodedb --lib -- -D warnings— clean ·cargo fmt --check— clean.origin/main.Notes
XX000for them. Wiring the direct fallback toclassify()plus the numeric table is the follow-up; the gateway now inherits whatever the direct table answers.TYPE_MISMATCH(42804 vs 42846) andRATE_EXCEEDED(53300 vs 54001); tracked as stage S3 of the classification plan.Fixes #362
Tradeoffs
Tradeoff: the gateway keeps no arms of its own — a gateway-only class now belongs in the shared table, not in this mapper.