fix(client): reconstruct typed errors from the server's SQLSTATE

[laksamanakeris]

Fixes #239.

What was wrong

check_error and response_to_query_result in nodedb-client/src/native/connection/mod.rs both read a server error frame with .map(|e| e.message), keeping the message and dropping e.code. Every server-returned error therefore reached callers as ErrorCode(9000) / ErrorDetails::Internal, no matter what SQLSTATE the server had computed for it.

Because every predicate on NodeDbError derives from ErrorDetails, this also broke classification: is_retriable(), is_not_found() and is_client_error() all reported the wrong answer. A cross-shard OCC abort is sent as 40001 specifically so the client retries it, and arrived as a non-retriable internal fault.

What this changes

Adds NodeDbError::from_sqlstate(sqlstate, message) in nodedb-types, beside the SQLSTATE constants, so both transports can use it, and routes both client call sites through it.

It returns a whole NodeDbError rather than a bare ErrorDetails, which would be the more obvious shape, because NodeDbError's fields are pub(super): nodedb-client cannot assemble an error from parts, so a bare ErrorDetails would be unusable at the call site. Returning the whole error also keeps code and details in lockstep, which is the invariant every existing constructor maintains.

Mapping policy

A SQLSTATE classifies a failure; it does not carry the operands. Mapping is therefore deliberately conservative, and the reasoning is recorded in the module doc rather than left implicit:

• Payload fields are left empty, never guessed. 42P01 becomes CollectionNotFound { collection: "" }. The failing name lives in the message, which is preserved verbatim; scraping it back out of prose would be brittle and would fabricate structured data. The variant, the ErrorCode and every derived predicate are exact.
• Overloaded codes stay unmapped. 55P03 is both LOCK_NOT_AVAILABLE (retriable) and STALE_READ_NOT_LEADER (not retriable); 57P03 and 53400 are likewise reused across two variants. Choosing either side would misreport retriability for the other.
• Load-bearing payloads stay unmapped. 57P04 needs NotLeader { leader_addr } and 54001 needs FanOutExceeded { shards_touched, limit }. Unlike a display name, a redirect address or a shard count is acted on, and an empty or zero value would be a fabricated instruction.
• Codes with no variant stay unmapped, for example 42P07 and 0A000.

Everything unmapped falls through to NodeDbError::internal, which is exactly what every server error produced before. That is what makes the change non-regressive: a server emitting a SQLSTATE the client does not know yet is no worse off than today.

Tests

• 42P01 yields CollectionNotFound, not Internal, with the message intact.
• An unmapped code (XX000) still yields Internal, asserted against NodeDbError::internal as the baseline. A deliberately-unmapped real code (55P03, 57P04) takes the same path.
• Retriability is restored: 40001 and 57014 are retriable again; 42P01, 42601, 42883 and 42501 are asserted not retriable, since reporting a client error as retriable would make a caller spin.
• Payload fields are asserted empty rather than populated, so the fidelity limit is pinned by a test instead of only by a comment.
• The existing message-only test, which is what let this through, now asserts the code and the predicate as well. Both client call sites are covered, plus the no-payload fallback.

Verified against a running server

Built main and drove the statements through nodedb-client on loopback. Before, all six error classes returned ErrorCode(9000). After, SELECT * FROM does_not_exist and DROP TABLE does_not_exist return ErrorCode(1100) / CollectionNotFound. The transport control is unchanged: a dead port still gives SyncConnectionFailed / ErrorCode(3000) / retriable.

Two classes in that sample still return Internal, and correctly so given this change's scope. Both are recorded in #239 as separate findings: the server's error_to_native sends XX000 for PlanError and UndefinedFunction over the native protocol even though the pgwire path types them as 42601 and 42883, and 42P07 has no ErrorDetails equivalent to map onto.

Gates

• cargo fmt --all --check clean.

• cargo clippy -p nodedb-types -p nodedb-client --all-targets --all-features -- -D warnings clean. The workspace-wide run fails in nodedb-vector/src/quantize/pq.rs (nonminimal_bool); confirmed identical on unmodified main with this local toolchain, so it is not from this change.

• cargo nextest run on the two changed crates: 787 pass, including the 13 added here.

• cargo nextest run -p nodedb-client-tests --all-features: 9 of 9 pass, including native_execute_sql_with_bound_params_round_trips, which drives the native protocol path both edited functions sit on. Needs RUST_MIN_STACK set, as CI already does.

• Workspace-wide, two failures occur on this machine that also occur on unmodified main, so neither comes from this change:

  • nodedb-cluster-tests/tests/ilp_gateway_migration.rs does not compile (missing fields returning and rls_filters). Excluded from the run.
  • nodedb data::executor::handlers::join::shuffle_join::tests::shuffle_grace_infeasible_budget_is_deterministic_error fails. Verified by checking out 28287607a clean and running that single test, where it fails identically. It is unrelated to error mapping: it asserts crate::Error::MemoryExhausted from drive_grace_build, which is internal to the join executor and never touches NodeDbError.

  I did not get a clean workspace-wide --no-fail-fast run locally; two attempts were killed by SIGTERM during compilation. The one run that completed reached 4010 tests with the pre-existing failure above as the only one. Deferring to CI for the authoritative full-gate result.

• cargo deny check not run: cargo-deny is not installed in this environment. This change adds no dependencies, so its inputs are unchanged.

Also checked, since it is what would most plausibly break: nothing in nodedb-client or nodedb-client-tests asserts is_internal, ErrorCode::INTERNAL, NDB-9000, or ErrorDetails::Internal, so no existing test depended on the old flattening.

[laksamanakeris]

Following up on the test note in the description.

nodedb-client-tests, the crate that exercises this change end to end: 9/9 pass, including native_execute_sql_with_bound_params_round_trips, which drives the native protocol path both edited functions sit on.

That run needs RUST_MIN_STACK=67108864. Without it, native_execute_sql_params_round_trip dies with SIGABRT / "has overflowed its stack" before reaching any assertion. It is the known debug-build stack artifact, not a behavioural failure, and it reproduces independently of this change.

Changed crates: 787/787 pass, including the 13 tests added here.

I was not able to finish a clean workspace-wide --no-fail-fast run locally. Two attempts were killed by SIGTERM during compilation, and the recompile is long enough that retrying was not a good use of time. What I do have:

• One workspace run completed and reached 4010 tests before fail-fast stopped it, with exactly one failure: nodedb data::executor::handlers::join::shuffle_join::tests::shuffle_grace_infeasible_budget_is_deterministic_error.
• That failure is pre-existing. Verified by checking out 28287607a clean and running that single test, where it fails identically. It asserts crate::Error::MemoryExhausted out of drive_grace_build, which is internal to the join executor and never touches NodeDbError.
• nodedb-cluster-tests/tests/ilp_gateway_migration.rs does not compile on main either (missing fields returning and rls_filters), so it was excluded from the run.

One further check worth recording, since it is the thing that would most plausibly break: nothing in nodedb-client or nodedb-client-tests asserts is_internal, ErrorCode::INTERNAL, NDB-9000, or ErrorDetails::Internal. No existing test depended on the old flattening behaviour, so there is no implicit contract this change violates.

Deferring to CI for the authoritative full-gate result.