fix(proxy): synchronize settings execution and gate pooler compatibility

[calvinbrewer]

CipherStash settings previously changed during preparation, could survive rollback incorrectly, and could leak when pooled connections were reassigned. This PR applies settings only after PostgreSQL confirms execution, restores transaction/savepoint state, and makes the compatibility matrix prove each declared outcome.

Behavior

• Correlate SET, SET LOCAL, RESET and DISCARD ALL with execution responses. Parse alone has no effect. Handle rollback, savepoints, LOCAL DEFAULT and failed commands before dependent encryption or connection reuse.
• Keep settings and encryption metadata in one protocol state. Reject unsafe mixed simple-query batches, stale encrypted preparations, and unsafe reuse after rejected statement replacement. Unavailable settings state fails closed.
• Drain execution responses without injecting illegal Flush messages after Query or Sync, including when a full protocol queue retries an operation.
• Support PostgreSQL RESET commands while preserving SQL error locations and rollback semantics.

Pooler validation

The matrix has 94 database cells for PgBouncer 1.19/1.24 and PgCat 1.2, plus runner regressions. Expected incompatibilities require specific evidence; setup errors, unrelated failures, timeouts and inconclusive probes fail. Isolation probes verify backend reuse and plaintext/ciphertext results. PgCat's protocol-3.2 socket refusal requires repeated authenticated protocol-3.0 controls.

A PostgreSQL 17 CI job gates the Rust matrix and fresh, complete Go suites on eight supported direct/session configurations. PgCat is built from checksummed official v1.2.0 source because the pinned release image was unavailable.

Standalone PgCat RESET cleanup and its missed session SET inside explicit transactions are tested separately. Session settings behind transaction pooling remain unsafe; broad transaction-driver support, Supavisor, additional drivers and pooler TLS are not claimed. Historical observations and current validation expectations are distinguished in docs/reference/connection-poolers.md.

Validation

• CI across PostgreSQL 14–17 passed 510 unit, 406 regular integration and 70 multitenant tests, including the new pipelined SET LOCAL regression.
• Formatting, typechecking and full Rust 1.99 Clippy passed. Eleven focused pooler regressions and the gate's induced-failure tests passed.
• The supported pooler CI gate passed: all 94 Rust matrix cells and all eight Go suites (474 tests each). The CRUD probe retains statements until disconnect; the separate early-close limitation is documented below.
• All seven checks passed on 013b768e0: PostgreSQL 14–17 full suites, supported pooler compatibility, encrypted burn-in, and performance regression. Test run; performance run.

Compatibility boundary

Cached mapped statements must be prepared again after mapping changes; the Python suite now asserts the precise fail-closed diagnostic. In PgBouncer 1.24 transaction mode with prepared-statement tracking, closing named statements before COMMIT can leave a standalone Sync that pg-proto 0.12 rejects. That sequence remains unsupported and is documented. Transaction-mode driver support is not claimed by the eight supported session-mode Go configurations.

Parent: CIP-4167.
Implementation: CIP-4922, CIP-4923, CIP-4924, CIP-4925.

[tobyhede]

Investigate the new context code, seems complicated, how can it be tightened?

[tobyhede]

Note to self: I hate it.

[freshtonic]

The settings and savepoint logic is careful work. I compared ConnectionSettings with the PostgreSQL rules, and it is correct for ROLLBACK TO, RELEASE, SET after SET LOCAL, COMMIT of a failed transaction, implicit blocks, SET LOCAL outside a block, and AND CHAIN. The Flush handling is also correct: the proxy adds a Flush only after Execute, never after Query or Sync.

I request changes for the items below. Line numbers refer to e22dc987.

Blocking

1. Some cipherstash.* spellings are ignored without an error (fail-open). context/settings.rs:51-77

• The variable name is matched with variable.to_string().to_ascii_uppercase(), which keeps the quote characters.
• A mapping value that is not a bare boolean returns Ok(None). The test at :828-830 asserts this. The statement then becomes Other, and PostgreSQL accepts it as a placeholder setting.
• Scenario A: after SET cipherstash.unsafe_disable_mapping = true, the client sends ... = off or ... = 'false'. Proxy stays in passthrough, so later INSERTs go to the database as plaintext.
• Scenario B: SET cipherstash."keyset_name" = 'tenant_b' or RESET "cipherstash.keyset_name" keeps the previous tenant's keyset.
• Fix: normalise each identifier part (quote rules and case folding as PostgreSQL does). Return an error for every cipherstash.* name or value that Proxy cannot interpret, as the PR already does for keyset values. I think the boolean case was also on main, but this PR makes settings fail closed, so this is the correct place to fix it.

2. With mapping disabled, Sync records no protocol boundary. middleware/frontend.rs:125-142

• In the mapping_disabled() arm, Sync goes to _ => {}. The mapped arm calls self.context.sync(operation)? (:256).
• Scenario: P/B/E/S in one packet, and E fails. The Sync is read before the ErrorResponse arrives, so SettingsProtocol::failed finds no boundary and sets discarding_until_sync. Only sync() clears this flag. The next Query is suppressed and the client hangs. A new P/B/E/S forwards only its Sync.
• #469 fixes the Query case only. I asked for the Sync case there. If #469 does not merge first, please fix both here.
• The tests do not find this because settings_rollback_tests.rs:63-64 and the frontend tests set disable_mapping = false again.

3. An ErrorResponse to FunctionCall can close the connection. frontend.rs:263-269 and :141

• Neither path registers an operation or a boundary for FunctionCall. If pg-proto 0.12 attributes the ErrorResponse to the FunctionCall operation (0.10.6 does, in grammar.rs), report_outcome → postgres_failed → finish_execution returns ContextError::UnknownOperation (context/mod.rs:277), and the driver closes the connection.
• Example: a libpq large-object call (lo_read with a bad descriptor). On main, this path could not fail. Please check against 0.12 and register FunctionCall in the same way as a simple Query.
• Also think about making finish_execution(Failed) on an unknown operation log a warning, not close the connection, so that a missing registration does not disconnect the client.

4. Remove proxy-architecture-walkthrough.html from the repository root. Its title is "CIP-3933 — Execution Lifecycle Walkthrough". It links to cipherstash/my-little-proxy at fixed SHAs and to Linear, and no file refers to it. If the content is useful, move it into the ADR or docs/.

Simplification (re: @tobyhede's question about the context code)

• Merge the duplicate maps. Statements are in Context.statements / statement_metrics_scopes and in SettingsProtocol.prepared. Portals are in Context.portals / portal_operations and in SettingsProtocol.portals. Each journal Mutation keeps two snapshots (Previous and MetadataSnapshot). Thus restore_failed_preparation, DISCARD ALL and close must copy between the maps. The maps already disagree: after an unnamed Execute, close_portal_if_current (mod.rs:998-1017) removes the portal from Context.portals, but SettingsProtocol.portals and PostgreSQL keep it. One StatementEntry and one PortalEntry, with the journal storing Option<Entry>, removes this class of problem.
• One way to get the state lock. About 40 call sites repeat .write().map_err(|_| ContextError::ProtocolStateUnavailable)?, and begin_bind takes the lock four times for one transition. A with_state(|s| …) entry point that returns effects would apply the ADR rule "change the state, then apply the effects" in one place.
• Remove dead and test-only code.
  • transaction_status (mod.rs:68) is written but only a test reads it, and its reader returns Idle if the lock is poisoned.
  • ConnectionSettings::execution_succeeded and is_default are not used in production.
  • settings_closed, settings_prepared_or_bound, closed and prepared_or_bound only forward to another method.
  • The #[cfg(test)] copies (begin_statement_preparation, close_portal, add_statement, close_statement_explicit) mean that those tests do not exercise the production lifecycle methods.
• Calculate the simple-query projection once. settings_simple_query_requires_fail_closed (mod.rs:1341) and SettingsProtocol::simple_query (settings_protocol.rs:219) contain the same may_change_effective / command_succeeded loop.

Non-blocking

• Correlation gaps are tolerated without an error. settings_protocol.rs:170-172, 197-199, 247 and mod.rs:1402-1404 make no change and raise no error when the statement, portal or pending entry is missing. I did not find a path that causes this today, but the ADR says that inconsistent state fails closed. A future gap would drop a confirmed SET and keep the old keyset.
• settings_in_flight() returns true when the lock is poisoned (mod.rs:1453-1458). The drain loop in driver.rs:231 can then wait without a limit if connection_timeout is not set. Return Result and close the connection.
• Cost per error grows with the square of the number of prepared statements. mod.rs:216 and :1429 pass all statement_metrics keys to take_unreferenced_metrics, and each check scans the scopes, portals, operations and the journal. A pooled application with many cached statements pays this on every error.
• The Parse refusal marker is predictable (frontend.rs:1492-1540). Bind and Execute now use refusal_name() for the reason you give in the comment, but the Parse marker is still __cipherstash_proxy_error_<code>_<msg>. A client that creates that type gets ParseComplete, and the refusal is lost. This does not leak data, but it desynchronises the client.
• Pooler matrix, StartupProtocol cells. checks.rs:713 connects to the pooler port, not to Proxy, on purpose. But connection-poolers.md:96,101,105,216 says "Proxy cannot connect". Change the wording to describe the pooler, or add one cell that goes through Proxy.
• checks.rs:306-316 runs DEALLOCATE ALL and retries on DUPLICATE_PSTATEMENT. Footnote 1 lists name collisions as part of the documented failure, so this recovery can hide a regression. Allow it only for cells that expect PreparedStatementMissing, or record it in the result.
• CI.
  • The pooler job has timeout-minutes: 45, but 94 cells × 90 s CELL_TIMEOUT can exceed it. A job timeout cancels the job, and then the !cancelled() upload step does not run. Put a step-level timeout on the gate step.
  • Every PR now also builds PgCat from source with no cache. Cache the image, or run the job on a schedule or a label.
  • tests/poolers/pgbouncer/Dockerfile installs pgbouncer without a version, but the docs name 1.19.0 and 1.24.0. Pin the package or assert the version.
• Docs.
  • The three new docs/errors.md entries (:289,306,312) are not in the table of contents.
  • encrypt-invalid-keyset-setting-value is in the Mapping section.
  • One entry has no "How to fix" heading and no separator.
  • connection-poolers.md:64 says that PgCat sends RESET ROLE;RESET ALL;, but the template says it also sends DEALLOCATE ALL;.
• CHANGELOG. "Pooler regression gate", "Connection pooler compatibility matrix", "Pooler test reliability", "Pooler compatibility validation", "execution lifecycle ownership" and "lifecycle regressions" are about CI or internal design, not about what users see. The security entry needs tokio-postgres 0.7.18, but Cargo.toml still allows "0.7". Raise the requirement so that a lockfile update cannot go back to an older version.
• Tests and conventions.
  • Test names such as review_r01_…, review_r02_…, review_r04_…, review_r07_… and review_r08_… refer to review rounds. Name them for the behaviour they test.
  • The new test in multitenant/set_keyset_name.rs:16-21 tests KEYSET_ID. It also falls back to CS_DEFAULT_KEYSET_ID, which the mise multitenant task unsets. Move it to set_keyset_id.rs.
  • ErrorChain and its 14 call sites are a logging change that does not relate to settings. A separate PR would make this one smaller.

[cipherstash-bot]

Recommendation: 🟡 merge after changes

Before merging, fix two tests. Add a test for the keyset-change refusal in SettingsProtocol, and make the DependentStatementAfterSettings test fail when the refusal is removed. The other findings can wait for a follow-up. Two of them (quoted setting names and UNSAFE_DISABLE_MAPPING = off) also exist on the base branch.

The settings rollback, savepoint and DISCARD ALL logic matched PostgreSQL in the orderings we checked. The CI job, container images and gate scripts have no merge-blocking problems.

Other findings not posted as comments

• Fix in a follow-up: test-only Context methods copy parts of production transitions. begin_statement_preparation (context/mod.rs:732) repeats only some steps of begin_parse. It does not start a metrics scope or store Preparation::Parse. review_r03_rejected_parse_removes_its_analysed_metadata and pipelined_batch_failure_rebases_analysed_metadata_without_replacing_later_work therefore check a state that production cannot create. Run them through prepare_lifecycle. Then delete the #[cfg(test)] methods that production does not call (add_statement, begin_statement_preparation, close_statement_explicit, set_statement_metrics_scope, close_portal). Merge the three #[cfg(test)] impl blocks of test_* wrappers into one.
• Optional: reset_tokens (parser.rs:55) clones every token of every statement, including the String in each word and literal. It also allocates two Vecs for each statement. parse_statement and parse_statements run for every Parse and simple Query. Return the tokenizer output unchanged when it contains no unquoted RESET word.
• Optional: statement_sql (parser.rs:26) renders any unquoted SET ALL TO DEFAULT as RESET ALL. PostgreSQL rejects SET ALL TO DEFAULT, because ALL is a reserved word. When Proxy rewrites a batch that has this text, PostgreSQL runs RESET ALL and succeeds instead.
• Optional: SettingsProtocol::execute (settings_protocol.rs:190) returns Result<bool, ()>. The caller maps every failure to PreparedSettingsChanged (context/mod.rs:1321), so the log cannot say if the mapping or the keyset changed. Return a small enum instead.
• Optional: no test covers a keyset setting with more than one value, such as SET CIPHERSTASH.KEYSET_NAME = 'first', 'second'. The values.len() == 1 check at settings.rs:94 rejects it today, but no test fails if the check is removed.
• Optional: the pooler user name and password are hard-coded in tests/poolers/pgbouncer/userlist.txt and 14 times in tests/poolers/pgcat/pgcat.toml.template. The Proxy services read CS_DATABASE__USERNAME and CS_DATABASE__PASSWORD from the environment. If a developer changes them, every matrix cell fails as an infrastructure error with no clear cause. Write the credentials from the environment at container start, as the entrypoints already do for other settings.
• Optional: the pooler-compatibility job (.github/workflows/test.yml:116) has no send-slack-notification step. The test and burn-in jobs have one, so a pooler failure on main sends no alert to #engineering.
• Optional: test:poolers stops and removes the shared proxy and proxy-mt containers (tests/tasks/test/poolers.sh:59, lines 70 and 97). DEVELOPMENT.md:420 says that the task "starts and removes its own containers". Update that sentence, or tell developers to run mise run proxy:up again after the matrix.

Checked and not reported

• One review said that a later error in a multi-statement simple query should keep an earlier session SET. PostgreSQL runs such a batch as one implicit transaction, so the error also rolls back the SET. The snapshot restore in ConnectionSettings::ready_for_query matches PostgreSQL, so we did not report it.

How this review was made

Agent	Model	Review type	Result
claude	claude-opus-5-5	test-gap	4 found, 4 posted
claude	claude-opus-5-5	rust	4 found, 2 posted
claude	claude-opus-5-5	infracode	4 found, 1 posted
codex	gpt-5.6-terra	test-gap	1 found, 1 posted
codex	gpt-5.6-terra	rust	1 found, 0 posted
codex	gpt-5.6-terra	infracode	1 found, 1 posted

Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.

Plain language: claude-opus-5-5 read every comment as a new reader would. 6 comment(s) had a problem that stopped the reader acting; it rewrote 6.

Stack: 0 earlier and 1 later pull request(s) (:asterisk: #467, #469). *️⃣ marks this pull request.

Context loaded: the description, 0 linked issue(s) and 2 discussion entries.

[cipherstash-bot]

Change before merge: No test checks that SettingsProtocol refuses encrypted work after the keyset changes.

Impact: These checks stop Proxy from running SQL that holds values encrypted under an earlier keyset. Lines 173-175 refuse Bind when the statement has encrypted literals and the keyset changed. Lines 200-203 refuse Execute when the portal has encrypted parameters and the keyset changed. If someone removes either keyset condition, no test fails. Proxy could then send values encrypted under the previous keyset.

Evidence: Tests cover only the mapping half of these conditions. Two examples are statement_prepared_without_mapping_is_refused_after_mapping_is_enabled and stale_portal_execute_returns_a_reprepare_error_without_executing_it in frontend.rs. No test calls capture_literals with true. No test changes the keyset between Parse, Bind and Execute. The new multitenant test pipelined_set_local_encrypts_bound_parameters_with_the_local_keyset checks only a case where nothing is refused.

Fix: Add this test to the review_regressions module in settings_protocol.rs. It expects bind to return false and execute to return Err(()) when the keyset is different.

    #[test]
    fn keyset_change_refuses_encrypted_bind_and_execute() {
        let name = Name::from_static(b"insert");
        let portal = Name::from_static(b"portal");
        let insert =
            SqlParser::parse_statement("INSERT INTO encrypted (secret) VALUES ($1)").unwrap();
        let tenant = |keyset: &str| {
            let mut settings = ConnectionSettings::default();
            settings.set_keyset(
                super::super::settings::SettingScope::Session,
                KeysetIdentifier(cipherstash_client::IdentifiedBy::Name(
                    keyset.to_owned().into(),
                )),
            );
            settings
        };
        let (first, second) = (tenant("first"), tenant("second"));

        // Literals are encrypted at Parse, so Bind under another keyset is refused.
        let mut protocol = SettingsProtocol::default();
        protocol.prepare(operation_id(), name.clone(), &insert, EffectiveSettings::new(&first));
        protocol.capture_literals(&name, true);
        assert!(!protocol.bind(
            operation_id(),
            portal.clone(),
            &name,
            EffectiveSettings::new(&second),
            false,
            Default::default()
        ));

        // Parameters are encrypted at Bind, so Execute under another keyset is refused.
        let mut protocol = SettingsProtocol::default();
        protocol.prepare(operation_id(), name.clone(), &insert, EffectiveSettings::new(&first));
        assert!(protocol.bind(
            operation_id(),
            portal.clone(),
            &name,
            EffectiveSettings::new(&first),
            true,
            Default::default()
        ));
        assert_eq!(protocol.execute(operation_id(), &portal, &second), Err(()));
    }

Notes on the test:

• The test uses operation_id(), SettingsProtocol::prepare, capture_literals, ConnectionSettings::set_keyset, KeysetIdentifier and cipherstash_client::IdentifiedBy::Name. Make sure that the review_regressions module can use each of these names. Add a use line for any name that it cannot use.
• In each bind call, the fifth argument is the encrypted_values flag that bind stores in the portal. The value true means that Bind encrypted the parameters.
• The test passes Default::default() as the sixth argument because it checks only the keyset.
• execute refuses a portal only when bound.prepared.needs_mapping is true. So the second part of the test reaches the keyset check only if prepare sets needs_mapping for this INSERT. If execute returns Ok, first check the value of needs_mapping.

_{Found by 1 model: claude}

[cipherstash-bot]

Change before merge: This assertion passes even when the DependentStatementAfterSettings refusal does not run.

Impact: The batch reads table_that_does_not_exist. settings_frontend sets enable_mapping_errors = true, so type checking also fails on that unknown table. Any error in query_handler becomes a DO 'BEGIN RAISE EXCEPTION ...' query. So this test still passes if someone removes the refusal at lines 340-345. No other test sends a batch through Frontend::intercept to that refusal.

Evidence: simple_error_query (line 1464) builds the RAISE EXCEPTION query for every error. The assertion checks only for the text RAISE EXCEPTION, not for the refusal message. The tests of settings_simple_query_requires_fail_closed in context/mod.rs do not cover the call from query_handler.

Fix: Assert on the refusal message. For example, add this test next to the existing one:

    #[tokio::test]
    async fn setting_before_mapped_statement_returns_the_settings_refusal() {
        let (mut frontend, _) = settings_frontend();
        let output = frontend
            .intercept(
                operation_id(),
                FrontendMessage::Query(bytes::Bytes::from_static(
                    b"SET CIPHERSTASH.UNSAFE_DISABLE_MAPPING = true; SELECT secret FROM table_that_does_not_exist",
                )),
            )
            .await
            .unwrap();
        assert!(matches!(output,
            pg_proto::FrontendMiddlewareOutput::Forward(FrontendMessage::Query(query))
            if String::from_utf8_lossy(&query)
                .contains("cannot change CipherStash settings before a statement that needs mapping")));
    }

_{Found by 1 model: claude}

[cipherstash-bot]

Fix in a follow-up: Proxy does not recognise a quoted setting name, so SET "cipherstash"."keyset_name" = 'tenant_b' changes PostgreSQL but not Proxy.

Impact: PostgreSQL treats the quoted and unquoted names as the same setting and reports success. Proxy classifies the statement as SettingsCommand::Other and keeps the previous keyset. Proxy then encrypts later values with the previous keyset, and it shows no error. Client libraries that quote identifiers create this form. One example is psycopg sql.Identifier("cipherstash", "keyset_name"). The same problem affects RESET and the UNSAFE_DISABLE_MAPPING name. SettingChange::parse returns Ok(None) for these names. So the UnexpectedSetKeyset check in validate_setting also does not run.

Evidence: The Display implementation of Ident keeps the quote characters. So line 51 produces "CIPHERSTASH"."KEYSET_NAME", which matches no known name. The base branch compared ObjectName values and had the same problem, so this is not a regression. This PR now accepts lower-case names, so quoted names are the remaining case.

Fix: Build the name from the unquoted identifier values. Add a quoted-name case to review_r04_known_keyset_setting_never_silently_ignores_value.

let variable = variable
    .0
    .iter()
    .filter_map(|part| part.as_ident())
    .map(|ident| ident.value.to_ascii_uppercase())
    .collect::<Vec<_>>()
    .join(".");

This code expects variable to be an ObjectName, the type that the base branch compared. It reads the parts of the name from the field .0. It calls as_ident() on each part to get the Ident without quotes. Before you use the code, check that the sqlparser version in this project has as_ident(). If it does not, use a match on each part to get its Ident. Then keep the same .value.to_ascii_uppercase() and .join(".") steps.

_{Found by 1 model: claude}

[cipherstash-bot]

Fix in a follow-up: Proxy ignores SET CIPHERSTASH.UNSAFE_DISABLE_MAPPING = off, but PostgreSQL accepts it.

Impact: Proxy changes this setting only for the true and false keywords. PostgreSQL accepts any value for a custom setting, including off, 'false' and 0. Suppose that mapping is disabled and a client turns it back on with = off. Mapping stays disabled in Proxy, so Proxy forwards SQL without encryption. Plaintext for encrypted columns can then appear in PostgreSQL error messages and server logs. For a keyset value that Proxy cannot read, Proxy already returns the error InvalidKeysetSettingValue. For a mapping value, Proxy returns no error.

Evidence: Every value that is not a boolean keyword reaches the _ => None arm at line 75. SettingsCommand::from_statement then returns Other. The base branch also ignored these values. Its tests covered only = 1, '1' and t. For these values, Proxy ignores the value and mapping stays on.

Fix: Accept a fixed set of boolean spellings. Return an error for any other value, so validate_setting refuses the statement before PostgreSQL runs it.

if variable == "CIPHERSTASH.UNSAFE_DISABLE_MAPPING" {
    let text = match values.as_slice() {
        [Expr::Value(value)] => match &value.value {
            Value::Boolean(disabled) => return Ok(Some(Self::Mapping(scope, *disabled))),
            Value::SingleQuotedString(text) => Some(text.to_ascii_lowercase()),
            Value::Number(number, _) => Some(number.to_string()),
            _ => None,
        },
        [Expr::Identifier(ident)] => Some(ident.value.to_ascii_lowercase()),
        _ => None,
    };
    return match text.as_deref() {
        Some("on" | "true" | "yes" | "1") => Ok(Some(Self::Mapping(scope, true))),
        Some("off" | "false" | "no" | "0") => Ok(Some(Self::Mapping(scope, false))),
        _ => Err(EncryptError::InvalidMappingSettingValue.into()),
    };
}

Also make these changes:

• Add InvalidMappingSettingValue as a new variant of the EncryptError enum in error.rs. Give it a documentation link, in the same way as InvalidKeysetSettingValue.
• The code uses .into() to convert the EncryptError to the Error type that parse returns. If this does not compile, return the new error in the same way that parse returns InvalidKeysetSettingValue.
• Change the existing tests for = 1, '1' and t. These tests expect Proxy to ignore the value. With this fix, = 1 and '1' disable mapping, and t returns the new InvalidMappingSettingValue error.

_{Found by 1 model: claude}

[cipherstash-bot]

Fix in a follow-up: No test reaches the COMMIT branch of may_change_effective (lines 158-163).

Impact: PostgreSQL answers COMMIT with ROLLBACK when the transaction already failed. This branch treats COMMIT as a possible settings change for that case. Proxy uses the result to refuse simple-query batches. Proxy also uses it to wait for the outcome of an extended-protocol Execute. If someone removes the branch, no test fails. Proxy could then map a dependent statement with a session keyset that the rollback removes.

Evidence: rollback_batch_is_refused_when_it_can_restore_another_keyset in context/mod.rs uses SET LOCAL. A successful COMMIT already clears a SET LOCAL value. So the first comparison at line 155 returns true, and the function returns before the COMMIT branch. review_r05_plain_transaction_and_reset_batches_are_allowed uses default settings, so both comparisons return false. That test passes with or without the branch.

Fix: Add this test to the tests module in settings.rs. It uses the existing succeeds helper. It calls SettingsCommand::from_statement to build a SettingsCommand from the parsed COMMIT statement. It expects may_change_effective to return true.

    #[test]
    fn commit_may_restore_a_session_setting_changed_in_the_transaction() {
        let mut settings = ConnectionSettings::default();
        succeeds(&mut settings, "BEGIN", "BEGIN");
        succeeds(
            &mut settings,
            "SET CIPHERSTASH.KEYSET_NAME = 'in-transaction'",
            "SET",
        );
        let commit = SettingsCommand::from_statement(
            &crate::postgresql::parser::SqlParser::parse_statement("COMMIT").unwrap(),
        );
        // An earlier pipelined error turns COMMIT into ROLLBACK, which
        // restores the keyset from before BEGIN.
        assert!(commit.may_change_effective(&settings));
    }

_{Found by 1 model: claude}

[cipherstash-bot]

Fix in a follow-up: No test checks that a successful DISCARD ALL removes Proxy's statement and portal metadata.

Impact: PostgreSQL removes all prepared statements and portals on DISCARD ALL. Lines 1425-1428 make Proxy do the same. If someone removes these lines, no test fails. Proxy then keeps encrypted statement metadata and metrics scopes from the previous client of a pooled connection.

Evidence: successful_discard_restores_mapping in frontend.rs checks only the mapping setting. discard_preserves_later_preparation_but_expires_its_rollback_snapshot in settings_protocol.rs checks only the SettingsProtocol maps. It does not check the Context metadata.

Fix: Add this test to the tests module in context/mod.rs. It uses the existing create_context, prepare_lifecycle and statement helpers. It expects get_statement to return None after PostgreSQL confirms DISCARD ALL.

    #[tokio::test]
    async fn successful_discard_all_removes_earlier_statement_metadata() {
        let mut context = create_context();
        let name = Name::from_static(b"statement");
        let parse = prepare_lifecycle(&mut context, &name, "SELECT 1", Some(statement())).await;
        context.preparation_acknowledged(parse).unwrap();
        let discard = operation_id();
        let metrics = context.start_metrics_scope().unwrap();
        let statements =
            crate::postgresql::parser::SqlParser::parse_statements("DISCARD ALL").unwrap();
        context
            .register_query(discard, metrics, Portal::passthrough(Some(metrics)), &statements)
            .unwrap();
        assert!(context.get_statement(&name).unwrap().is_some());
        context
            .execution_completed(Some(discard), ExecutionEnd::Command("DISCARD ALL"), false)
            .unwrap();
        assert!(context.get_statement(&name).unwrap().is_none());
    }

Notes on the test:

• The test prepares a statement, registers a DISCARD ALL query, and checks that the statement exists. Then it reports that DISCARD ALL completed and checks that the statement is gone.
• The test also uses context.preparation_acknowledged, context.start_metrics_scope, context.register_query, Portal::passthrough, context.execution_completed, ExecutionEnd::Command and operation_id(). Make sure that each of these exists and that the tests module can use it. Add a use line for any name that it cannot use.
• The test passes false as the last argument of execution_completed. Read the signature of execution_completed to find what this argument means. Use the value that reports a normal, successful DISCARD ALL.

_{Found by 1 model: claude}

[cipherstash-bot]

Fix in a follow-up: No test runs the retry path after the 256-message pipeline is full.

Impact: This PR changes the retry. A retained Query, Sync or expanded message now reaches the shared outcome drain at line 214. Before this PR, the retry used continue and skipped that drain. If this path breaks, a client that queues more than 256 messages can hang. Proxy could also map a dependent statement before the settings outcome arrives. The PR description lists this case, but no test runs it.

Evidence: The only new driver.rs test is review_r07_copy_input_resumes_frontend_while_outcomes_are_pending. It checks only backend_needs_frontend. No test fills BoundedPipeline::new(256). No test reaches the FrontendProjectionError::Capacity branch.

Fix: Add an integration test next to pipelined_statements in pipeline.rs. The test queues more than 256 queries, with a settings query last, before it reads any result. If responses arrive early, the pipeline can empty before it is full. So confirm once that the test reaches the Capacity branch. For example, add a temporary log line.

#[tokio::test]
async fn full_pipeline_retries_a_retained_setting_query() {
    use std::time::Duration;
    use tokio::task::JoinSet;

    let client = connect_with_tls(*PROXY).await;
    let mut requests = JoinSet::new();
    for sql in std::iter::repeat_n("SELECT 1", 256)
        .chain(["SET CIPHERSTASH.UNSAFE_DISABLE_MAPPING = true"])
    {
        let client = client.clone();
        requests.spawn(async move { client.simple_query(sql).await });
    }
    tokio::time::timeout(Duration::from_secs(10), async {
        while let Some(result) = requests.join_next().await {
            result.unwrap().unwrap();
        }
    })
    .await
    .unwrap();
}

The test uses connect_with_tls and PROXY. Make sure that pipeline.rs can use both names. The test also calls client.clone() on the value that connect_with_tls(*PROXY) returns. If that type cannot be cloned, put the client in a std::sync::Arc and clone the Arc instead.

_{Found by 1 model: codex}

[cipherstash-bot]

Fix in a follow-up: apk add pgbouncer installs the version that the Alpine branch has on the build day, so the PgBouncer version is not pinned.

Impact: The comment on lines 3-7 says the matrix pins exact PgBouncer versions, but the build does not check the version. If Alpine changes the package, CI tests a different PgBouncer with no change in this repository. The expected outcomes in pooler/mod.rs can then fail on an unrelated pull request. The gate can also pass for a version that it does not report. The PgCat image does not have this problem, because it checks a source checksum.

Fix: Pass the expected version as a build argument from each Compose service. Use an apk version constraint, so a different minor version stops the build. Set PGBOUNCER_VERSION to "1.19" and "1.24" next to ALPINE_VERSION in tests/docker-compose.poolers.yml.

ARG PGBOUNCER_VERSION=1.24
RUN apk add --no-cache "pgbouncer~${PGBOUNCER_VERSION}" \
  && (id pgbouncer >/dev/null 2>&1 || adduser -D -S pgbouncer)

_{Found by 2 models: claude, codex}

[auxesis]

Approved, thanks for this @tobyhede.

I have left some comments about Markdown formatting that should be applied to all markdown changes in this PR, and should also be recorded in CLAUDE.md.

[auxesis]

Change needed for this whole file:

Remove hard line wraps, switch line-per-sentence, to make reading diffs easier.

[auxesis]

Line-per-sentence again, like the additions further down in this same file.