[tobyhede]

Requested by Toby

Adds tests for how CIPHERSTASH.* settings behave under rollback, covering cases nothing tested yet.
Tests only: every new test passed against the existing behaviour, and breaking rollback, ROLLBACK TO or the DISCARD ALL reset in settings.rs makes them fail.

What's covered

• Rolled-back keyset name resets. RESET CIPHERSTASH.KEYSET_NAME and SET CIPHERSTASH.KEYSET_NAME TO DEFAULT, rolled back fully or to a savepoint, through both the simple and the extended protocol, restore the keyset the next encrypted INSERT uses.
• Extended-protocol SET and reset, then rollback. Each statement goes through Parse/Bind/Execute/Sync, with ROLLBACK and with ROLLBACK TO SAVEPOINT, for keyset, mapping, SET LOCAL, RESET ALL and a mixed set of changes.
• Pipelined batches. A new Client::pipeline harness helper sends a whole batch before PostgreSQL replies. Like the driver, it only reads replies when an Execute leaves a settings change in flight. Covered: BEGIN; SET; SET; ROLLBACK, the same with ROLLBACK TO, and an implicit transaction aborted by a later failing statement. A control batch without a rollback checks that the changes do take effect.
• Integration test with default_keyset_id. rolled_back_mapping_changes_are_undone now asserts that Proxy refuses SET CIPHERSTASH.KEYSET_NAME because a default keyset is configured. After each rolled-back mapping SET, it writes a row and reads the stored ciphertext straight from PostgreSQL to check that it was encrypted under CS_DEFAULT_KEYSET_ID.

DISCARD ALL in an implicit transaction

I probed PostgreSQL 14, 15, 16 and 17 (the CI matrix) directly over the wire protocol:

• DISCARD ALL is accepted only as the first Execute of an extended-protocol batch, and PostgreSQL commits it at once. A later failure in the batch rolls back only what follows it, so work_mem and a custom cipherstash.* GUC stay at their defaults.
• After any earlier Execute, PostgreSQL rejects it with DISCARD ALL cannot be executed within a pipeline.

So Proxy keeping the defaults after such a failure is correct, not a gap. Two unit tests pin both cases.

Not run here

The integration test compiles and passes clippy, but it needs ZeroKMS credentials, so CI is its first run.

Acknowledgment

By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.

🤖 Generated with Claude Code

[cipherstash-agent]

Review attestation — automated review by Claude Code at head 7bf6b86: /code-review medium, then company-skills:cross-check-review-findings (from cipherstash/skills) against the PR's existing feedback.

Result: no critical or blocking issues. No new findings survived verification.

• Scope: tests only (settings_rollback_tests.rs, disable_mapping.rs). No production code changed.
• Checked: the Client::pipeline / run / reply harness (reply ordering, the post-failure skip of queued replies, reading replies only when settings_in_flight), the rollback and ROLLBACK TO cases, the two DISCARD ALL pipeline cases, and the new keyset assertions in the integration test.
• Existing feedback: none on this PR (no reviews, threads or comments), so there was nothing to de-duplicate against.
• Not verified here: CI was still in progress at review time. The Test (PostgreSQL 14–17) jobs run the unit tests, and the integration test needs ZeroKMS credentials, as the PR description says. Treat those results as the gate.

---

Generated by Claude Code

[freshtonic]

Reviewed head 7bf6b86d. This PR adds tests only and changes no production code. I found nothing that must change. The tests catch the bugs they are written for, and the DISCARD ALL analysis agrees with the code in settings.rs.

Nits

• Client::pipeline doc: "Like the driver". In a test of client behaviour, "the driver" reads as the client driver, for example tokio-postgres, which does not wait for replies. What the harness copies is the drain in Proxy's postgresql/driver.rs:214-264: after an Execute (or a Query or Sync) that leaves a settings change in flight, Proxy reads the backend until the outcome settles, and then it accepts more frontend messages. Write "Like Proxy's driver.rs", so that no one "fixes" the harness to send the whole batch first.
• discard_all_rejected_later_in_a_pipeline_keeps_settings passed under all my mutations. This is expected, because the test guards against a reset applied before PostgreSQL confirms the DISCARD, and none of my mutations did that. It is useful as a regression guard. I note it only so that no one counts it in the mutation claim.

What I checked and found correct

• Mutations in settings.rs. For each change below, at least one of the new tests failed:
  • With no restore_transaction() on ROLLBACK, rolled_back_keyset_name_reset…, rolled_back_extended_protocol… and pipelined_batch… fail.
  • When ROLLBACK TO does not restore the keyset, the same three tests fail.
  • When DiscardAll does nothing, discard_all_that_opens_a_pipeline… fails.
  • With no restore after a failed implicit transaction (ready_for_query), pipelined_batch… and discard_all_that_opens_a_pipeline… fail.
• The DISCARD ALL case. reset() clears transaction_start, so the later SET … 'after' takes its snapshot from the defaults, and the failure restores the defaults. This agrees with the PostgreSQL behaviour in the PR description: a DISCARD ALL that opens the batch is committed, and only the statements after it are rolled back.
• The guard in rollback_restores_original_keyset. The assert_ne! before the rollback proves that each change set really changed a setting. Without it, a rollback test could pass on a no-op.
• The skipped messages in the harness. After a rejected Execute, Proxy still sees the rest of the batch but gets no replies for it. This is what PostgreSQL does.
• The integration test. disable_mapping runs in test:integration:without_multitenant, where CS_DEFAULT_KEYSET_ID is set, so the refusal of SET CIPHERSTASH.KEYSET_NAME and the keyset check are valid there. The INSERT after each rolled-back mapping change fails if mapping stays disabled, because Proxy then does not encrypt the parameter and the write is refused. The test reads the stored ciphertext directly from PostgreSQL, so it checks the keyset that was really used.

What I ran

• cargo test -p cipherstash-proxy --lib settings_rollback_tests: 16 of 16 passed.
• The four mutations above. I reverted all changes after the runs.

CI

All 9 checks pass, including the integration jobs on PostgreSQL 14 to 17 and the encrypted burn-in.

[tobyhede]

Thanks @freshtonic. I took the doc nit in 52d71aa: the Client::pipeline comment now names Proxy's postgresql/driver.rs as the drain it copies, so it can't be read as the client driver. That commit changes only the comment.

On discard_all_rejected_later_in_a_pipeline_keeps_settings: agreed. It guards against applying the reset before PostgreSQL confirms it, so none of the mutations should break it, and it isn't part of the mutation claim.

---

Generated by Claude Code