fix(proxy): protocol desync on transformation error after a mapped statement (CIP-3678) - #441
Conversation
…esponse stream ordered Once a connection had run a mapped (encrypted) statement, a later statement failing during transformation surfaced to the client as a protocol error (tokio_postgres UnexpectedMessage) instead of the proxy's actual error message (CIP-3678). The frontend wrote its ErrorResponse and ReadyForQuery straight to the client the moment a Parse/Bind/Query handler failed. Responses to earlier pipelined requests could still be in flight from the server — in the reproduction, the CloseComplete + ReadyForQuery for the Close + Sync that tokio_postgres issues when it drops the previous statement. The direct reply overtook them, so the client attributed the error to the Close and the Close's responses to the failed prepare, shifting every subsequent response onto the wrong request. A wire trace shows the client receiving E, Z, 3(CloseComplete), Z where it needed 3, Z, E, Z. Instead of replying out of band, the frontend now replaces the failed message with a 'DO ... RAISE EXCEPTION' statement carrying the proxy's message, SQLSTATE and hints via RAISE USING. The injected statement is a request like any other, so the server emits its ErrorResponse and ReadyForQuery in exactly the slot the client expects, with accurate transaction status; the batch's remaining messages, including Sync, are discarded. This also drains the session started by the failed statement through the backend's normal ErrorResponse completion path, instead of leaking it. FATAL errors (for example SET CIPHERSTASH.KEYSET with a configured default keyset) are still written directly: RAISE cannot produce FATAL, and ordering is moot once the client abandons the connection. Also fixes the ordering of close_statement/set_statement_session in parse_handler, which wiped the statement-session mapping it had just recorded and forced every Bind onto the latest-session fallback (the 'Session lookup failed for prepared statement' warnings). The exception body is nested through quote_literal at both levels — no dollar quoting — so error text containing quotes, % or $$ cannot escape the statement. CIP-3678
… statements Regression tests for CIP-3678: on a connection that has already run a mapped (encrypted) statement, a statement that fails during transformation must surface as a clean db error carrying the proxy's message — not desync the stream into UnexpectedMessage — and the connection must remain usable afterwards. The passthrough-warmed variant keeps the previously-working path covered. Adds a proxy_port() helper (CS_PROXY__PORT, defaulting to 6432) so a local run can target a proxy on a non-default port, mirroring get_database_port(). CIP-3678
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Comment-only review.\n\nThe ordering strategy—injecting the error through PostgreSQL and distinguishing injected exceptions from directly-sent fatal errors—makes sense, and the close-before-session-recording fix is important.\n\nFinding: the two regression tests added by this PR, and , currently fail in every PostgreSQL matrix job (14–17). Because those tests directly exercise the behavior this PR is intended to fix, this is not a check failure I would treat as incidental; the test/runtime mismatch needs investigation before merge.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Correction to my preceding review (shell formatting stripped identifiers): the failing regression tests are transformation_error_after_mapped_statement and transformation_error_after_passthrough_statement.
… not a transformation error
The two regression tests drove their proxy-side failure with an equality
predicate on the storage-only eql_v3_boolean column. That only errors when
CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS is on: with it off the type-check
failure silently falls back to passthrough, PostgreSQL describes $1 as the
raw eql_v3_boolean domain, and tokio-postgres fails client-side with
WrongType before Bind — the injected-exception path is never exercised.
The CI proxy containers run with that flag off: tests/mise.tcp.toml and
tests/mise.tls.toml set it to true, but tests/docker-compose.yml never
forwards the variable, so the containers get the default (false). This is
also the production configuration.
Use a statement the proxy's SQL parser rejects instead (same shape as
invalid_sql_statement). A parse error takes the same failure path
(handle_statement_error) in every configuration. Verified against the
pre-fix frontend: both tests fail with Error { kind: UnexpectedMessage } —
the original CIP-3678 desync — and pass with the fix, with the flag both
on and off.
tobyhede
left a comment
tobyhede
left a comment
There was a problem hiding this comment.
Don't have enough context for this one.
2 participants
Fixes CIP-3678.
The bug
Once a connection had successfully run a mapped (encrypted) statement, any later statement failing during transformation surfaced to the client as a protocol error — tokio_postgres
UnexpectedMessage— instead of the proxy's actual error, and the proxy's useful message was never delivered. With connection pools, mapped-then-failing is the common case.Diagnosis
A wire trace of the reproduction (protocol-level logging on both sides of the proxy) shows the desync is a response-stream reorder, not a client- or state-machine bug:
query()returns, tokio_postgres drops its cached prepared statement and pipelinesClose(s0)+Sync, immediately followed by the failing statement'sParse(s1),Describe,Sync.ErrorResponse+ReadyForQuerystraight to the client, while the server'sCloseComplete+ReadyForQueryfor the Close were still in flight.E, Z, CloseComplete, Zwhere the protocol requiresCloseComplete, Z, E, Z. It attributed the error to the Close request and the Close's responses to the failed prepare —UnexpectedMessage.On a fresh connection there is nothing in flight, so the direct reply happened to line up — which is why the bug only appeared after a prior statement. The ticket's stale-session hypothesis was a real (smaller) leak — the failed Parse's session-metrics entry was abandoned — but it was not the cause of the desync.
The fix
The frontend no longer replies to the client out of band. A failed Query/Parse/Bind is replaced with a
DO ... RAISE EXCEPTIONstatement sent to the server, carrying the proxy's message, SQLSTATE, hints and related fields viaRAISE ... USING. Because the injected statement is a request like any other, the server emits theErrorResponseandReadyForQueryin exactly the slot the client expects — after any in-flight responses, with accurate transaction status. The rest of the failed batch, including itsSync, is discarded (the injected statement already produced the batch'sReadyForQuery).This also drains the session the failed statement had started, through the backend's normal ErrorResponse completion path, instead of leaking it.
Two deliberate carve-outs:
SET CIPHERSTASH.KEYSET_IDwith a configured default keyset) are still written directly to the client:RAISEcannot produce FATAL severity, and ordering is moot because the client abandons the connection — and everything pipelined on it — on receipt.ErrorStateis now an enum making the two delivery modes and their differing Sync handling explicit.quote_literalat both levels (no dollar quoting), so error text containing quotes,%or$$cannot escape the statement. The previous (dormant)to_database_exceptionhelper had both hazards.Also fixed along the way:
parse_handlerrecorded the statement's session mapping and then immediately wiped it viaclose_statement(which drops the mapping for the rebound name). Every Bind then hit the "Session lookup failed for prepared statement, using latest session" fallback. The close now happens before the mapping is recorded.Regression tests
extended_protocol_error_messages::tests:transformation_error_after_mapped_statement— mapped warm-up, thenSELECT id FROM encrypted WHERE encrypted_bool = $1(storage-only column, no equality term). Asserts the error arrives as a properdb errorcontaining the proxy's message, and that the connection remains usable afterwards. Fails withUnexpectedMessageagainst the unfixed proxy; passes with the fix.transformation_error_after_passthrough_statement— same failing statement on a passthrough-warmed connection (this desynced locally too, timing-dependent; now deterministic either way).Also adds
common::proxy_port()(CS_PROXY__PORT, default 6432) so a local run can target a proxy on a non-default port.Test evidence
Run locally against a proxy built from this branch (port 6533,
CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS=true, shared dev postgres on 5532):mise run check— clean (fmt, clippy, compile).mise run test:unit— all green (258 result lines, 0 failures).UnexpectedMessage) against the unfixed frontend, pass with the fix.extended_protocol_error_messages,set_keyset_error,simple_protocol,pipeline,passthrough,map_params, 34 tests): 27 pass. The 7 failures were re-run against an unfixed build of the same branch and fail identically there — pre-existing in this environment, not regressions:multitenant::ore_order::*(3) — require a proxy with no default keyset (CI runs them in a separate phase).passthrough_{invalid_statement,insert_from_select,select_with_cardinality}andencrypted_column_not_defined_in_schema— expect type-check failures to fall through to the database; in this environment (shared dev DB, EQL 3.0.3 installed vs 3.0.4 built) the mapper rejects them before the DB does, on both builds.Notes for review
RAISE, sowhere/file/line/routinefields point at theinline_code_blockrather than being absent, and aPositionfield is no longer set (parse errors already embed line/column in the message). Message text, SQLSTATE, severity, hint, table/column fields are preserved viaRAISE ... USING.Querydestroys the server's unnamed prepared statement/portal (protocol-defined side effect). A client that re-binds a previously parsed unnamed statement across a failed batch would need to re-Parse; this was also the proxy's behaviour before the direct-write error path was introduced (ab60e96, Feb 2025).