test(proxy): check a connection's mirrored state against a PostgreSQL model by tobyhede · Pull Request #497 · cipherstash/proxy

tobyhede · 2026-10-28T02:06:49Z

Requested by Toby

Stacked on #496. I'll retarget it to main once #496 merges.

This adds a model-based property test for Proxy's mirror of a connection, and fixes the one bug it found.

Property test

postgresql/middleware/tests/connection_model.rs drives one connection through the middleware harness with random traffic:

Parse, Bind, Execute, Close and Sync, over named and unnamed statements and portals.
Simple Queries: SELECT, SET and SET LOCAL CIPHERSTASH.UNSAFE_DISABLE_MAPPING, BEGIN, COMMIT, ROLLBACK, DISCARD ALL, DEALLOCATE, and creating or dropping a temporary table.
Random execution failures.

A small model of PostgreSQL answers each message Proxy forwards.
It covers implicit and explicit transactions, failed transactions, skipping until Sync, duplicate names, the unnamed statement and portal, DISCARD ALL refused in a transaction block, SET LOCAL outside one, and prepared statements not being transactional.
Messages Proxy rewrites so that PostgreSQL fails them fail in the model too.

After every ReadyForQuery the test checks four things:

Proxy holds no statement PostgreSQL released.
At idle, Proxy holds no operation, portal, portal operation or suspended execution.
Proxy agrees with PostgreSQL on UNSAFE_DISABLE_MAPPING.
Proxy agrees with PostgreSQL on whether the temporary table exists.

It runs on a plaintext connection and on one with an encrypted column, at 256 cases each by default (PROPTEST_CASES raises that).
The module header lists what the model leaves out, mainly pipelining, partial Execute and savepoints.

I checked the test is sensitive by planting faults in the model.
Three of them were caught: DISCARD ALL keeping the temporary table, a failed batch not rolling back, and ROLLBACK keeping changes.
20,000 cases per config pass with the fix below.

Bug found: a simple Query kept the unnamed statement

PostgreSQL drops the unnamed prepared statement before it runs any simple Query. Proxy kept its copy.
The impact was small: one stale entry, and PostgreSQL rejects any Bind to it.

The fix is test-first:

7023ad0 adds two failing harness tests. One covers the plain case. The other covers a pipelined failing batch whose skipped unnamed Parse is rolled back after Proxy has read the following Query.
70f85aa makes Context release the unnamed statement when it reads a simple Query (mapped, refused, or with mapping disabled). It also clears earlier rollback snapshots for that statement, the same rule SQL DEALLOCATE already follows.

One existing test changed. sql_deallocate_all_keeps_the_unnamed_statement sent DEALLOCATE ALL as a simple Query, so it only passed because of this bug. It now runs DEALLOCATE ALL through the extended protocol, which is where the unnamed statement survives it.

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 by Claude Code

cipherstash-agent

Automated review attestation (Claude Code /code-review medium + cross-check against head 0e35fc1; no existing PR feedback to dedupe against).

Critical/blocking: none. No correctness bug found in the production change; the unnamed-statement release on a simple Query matches PostgreSQL's ordering. Static review only; I did not run the tests or wait on CI.

Non-blocking:

minor packages/cipherstash-proxy/src/postgresql/context/lifecycle.rs:59: no CHANGELOG.md [Unreleased] entry for the unnamed-statement fix, which CLAUDE.md asks for on notable changes.
Inline: one minor on the model's one-directional invariant.

Generated by Claude Code

cipherstash-agent

Inline comment from the review above.

Generated by Claude Code

cipherstash-agent · 2026-10-28T02:10:15Z

+        for name in &held {
+            assert!(
+                postgres.statements.contains_key(name),
+                "Proxy holds statement {name:?}, which PostgreSQL released"


minor check() only asserts Proxy holds no statement PostgreSQL released, never that Proxy still holds every statement PostgreSQL holds. Proxy dropping a statement PostgreSQL kept (e.g. an over-eager release_unnamed_statement) lets the next Bind through unencrypted and still passes. Also assert postgres.statements.keys() ⊆ held, at least on the encrypted connection.

Generated by Claude Code

Agreed. I checked the reverse direction locally over 3,000 cases per config and it holds, so the assertion goes in with this PR's next code push.

Generated by Claude Code

Added in 902d9aa.

Generated by Claude Code

tobyhede

Review attestation

Reviewed 0e35fc1 · posted from Claude Code · claude-opus-5-5 · effort medium
Sources: code-review · claude-opus-5-5
Skills: code-review

Findings (9): 0 critical, 0 major, 9 minor — 8 inline, 1 below.

CHANGELOG.md (outside diff) minor The simple-Query fix in 70f85aa has no [Unreleased] Fixed entry, which CLAUDE.md asks for. Add one.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

tobyhede · 2026-11-02T23:27:55Z

        portal: Portal,
        statements: &[ast::Statement],
    ) -> Result<(), Error> {
+        self.release_unnamed_statement()?;


minor Releasing the unnamed statement when the Query is read is wrong while PostgreSQL is skipping until Sync after a failed pipelined message, since it ignores that Query and keeps the statement. With Parse('', S) + Sync, then Parse('s1', ) + Query with no Sync, a later Bind('') is refused with UnknownPreparedStatement that PostgreSQL would accept. Release only once the Query is known to run, such as on its first response.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

Moved in f38676c (pushing next). The frontend now only records that the Query releases the unnamed statement, and Context applies it when PostgreSQL first answers the Query, through the same path as SQL DEALLOCATE.

The ordering you describe can't reach Proxy today, though. pg-proto 0.12 has no (Extended, Query) arm in classify_frontend, so a Query sent while an extended batch is open fails with FrontendProjectionError::Illegal before any middleware sees it. A Query after Sync always runs. I made the change anyway so the release doesn't depend on that pg-proto rule. I didn't add a harness test for the ordering, because it can't happen through pg-proto.

Generated by Claude Code

tobyhede · 2026-11-02T23:27:57Z

+    fn check(&self) {
+        let postgres = &self.postgres;
+        let held: HashSet<String> = statement_names(&self.connection).into_iter().collect();
+        for name in &held {


minor check() asserts only that Proxy's statements are a subset of PostgreSQL's, so a Proxy that releases too early, the failure mode this fix risks, passes every case. Assert the reverse as well.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

Added in 902d9aa. check() now also asserts Proxy holds every statement PostgreSQL holds, on both connections. 20,000 cases per config pass.

Generated by Claude Code

tobyhede · 2026-11-02T23:28:00Z

+
+    async fn step(&mut self, step: Step) {
+        match step {
+            Step::Query { .. } if self.batch_open => {


minor A Query in an open batch always gets a Sync first, and Postgres::query never consults skipping, so a Query inside a failed extended batch is never generated. The ordering behind the lifecycle.rs:377 defect cannot occur in generated traffic.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

Right. pg-proto rejects a Query while an extended batch is open, so the model can't generate that ordering legitimately; there's a comment on that arm now. To give the test the reach you were after, 902d9aa adds pipelined runs that hold PostgreSQL's replies back until the next Sync or Query, so Proxy reads later messages before it learns how earlier ones ended. Those runs found an older bug: a skipped Close after DEALLOCATE ALL restored the statement. It's fixed test-first in 2562c19 and f38676c.

Generated by Claude Code

tobyhede · 2026-11-02T23:28:02Z

        metrics: SessionId,
        response: DiagnosticResponse,
    ) -> Result<(), Error> {
+        self.release_unnamed_statement()?;


minor refuse_query releases the unnamed statement but keeps the unnamed portal, which PostgreSQL drops along with it. Inside BEGIN, Bind unnamed, a refused Query, then Execute('') is runnable to Proxy but fails with "portal does not exist", and the portal's metrics scope lingers until the transaction ends. Remove the unnamed portal here too.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

This doesn't reproduce. refused_query_releases_the_unnamed_portal (2562c19) runs BEGIN, Bind to the unnamed portal, Sync, then a refused Query. Proxy holds no portal after the refused Query's ReadyForQuery, both before and after this change. I've kept the test to pin that.

Generated by Claude Code

tobyhede · 2026-11-02T23:28:05Z

+    /// Forgets a statement that PostgreSQL releases unconditionally, as a simple Query
+    /// releases the unnamed statement. A rollback of an earlier operation must not restore
+    /// it, so returns the metrics scopes its rollback snapshots referenced.
+    pub fn released(&mut self, name: &Name) -> Vec<SessionId> {


minor released() duplicates deallocated() (line 486) but has drifted: it clears every matching journal entry and returns their metrics, while deallocated() clears only the first and drops its metrics, so a statement_metrics entry referenced only by that snapshot is never finished. Share one helper that returns the cleared metrics.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

Fixed in f38676c. released() is gone, and ConnectionProtocolState::deallocate_statements is now the one path for both SQL DEALLOCATE and the simple-Query release. deallocated() now returns the metrics of the snapshot it clears, and they go through take_unreferenced_metrics. The leak was real: reparse_pipelined_after_deallocate_finishes_the_old_metrics in 2562c19 fails without the fix (2 metrics scopes instead of 1).

Generated by Claude Code

tobyhede · 2026-11-02T23:28:07Z

    }

+    /// PostgreSQL runs a simple Query in the unnamed statement, releasing the client's.
+    fn release_unnamed_statement(&self) -> Result<(), Error> {


minor release_unnamed_statement re-implements close_statement/remove_statement_metadata with an extra settings_protocol step, so Close and simple-Query release can drift. Extend the existing helper instead.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

Removed in f38676c. It now goes through deallocate_statements, which SQL DEALLOCATE also uses, rather than close_statement. A release confirmed by PostgreSQL has to skip a statement a later pipelined Parse prepared again, and only the DEALLOCATE path checks that.

Generated by Claude Code

tobyhede · 2026-11-02T23:28:10Z

    }

    pub fn unmapped_query(&mut self, operation: OperationId) -> Result<(), Error> {
+        self.release_unnamed_statement()?;


minor The release is called separately from unmapped_query, register_query and refuse_query, so any new simple-Query path must remember it, and the refuse path already misses the portal. Call it once in the frontend Query arm after the discard check.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

Done in f38676c. The frontend records it once in the Query arm, right after the discard check, which covers the mapped, refused and mapping-disabled paths.

Generated by Claude Code

tobyhede · 2026-11-02T23:28:12Z

+    }
+}
+
+fn bind_message_to(statement: String, portal: String) -> FrontendMessage {


minor bind_message_to duplicates bind_message in middleware/mod.rs:1153 and the Connection::bind builder. Generalise bind_message to take a statement and portal.

_{Source: code-review · claude-opus-5-5}

Generated by Claude Code

Done in 2562c19 and 902d9aa. bind_message now takes a statement and a portal, and the model uses it.

Generated by Claude Code

…removes When a client prepared a name again in the same pipeline as a SQL DEALLOCATE of that name, the re-Parse's rollback snapshot held the metrics of the deallocated statement. Confirming the DEALLOCATE cleared the snapshot without finishing those metrics, so they leaked for the life of the connection. Forgetting a rollback snapshot now returns its metrics scopes, and a single deallocate_statements path finishes every scope that nothing else references.

…L removed A pipelined Close or Parse takes a statement out of Proxy's view but journals it so a later failure can roll it back. DEALLOCATE ALL only forgot the statements still in view, so when the pipeline failed after it, rolling back the skipped Close or Parse brought back a statement PostgreSQL no longer held. DEALLOCATE ALL now also forgets every named statement whose rollback snapshot the journal holds.

…imple Query PostgreSQL runs every simple Query in the unnamed statement, so it drops the client's unnamed prepared statement first. Proxy kept its copy, and the statements it tracked drifted from PostgreSQL's. Proxy now marks a simple Query as releasing the unnamed statement when it reads the Query, and applies the release when PostgreSQL first answers it, through the same deallocate_statements path as DEALLOCATE. Waiting for the answer matters in a pipeline: by then PostgreSQL has resolved every earlier operation, so rolling back a failed batch ahead of the Query cannot bring the old copy back, and a Query PostgreSQL skips is never answered and releases nothing. The existing DEALLOCATE ALL test ran its statement as a simple Query, so it held only because of this gap; it now runs DEALLOCATE ALL through the extended protocol. A new test also pins that a refused simple Query leaves no unnamed portal behind.

… model Proxy mirrors the prepared statements, portals, transaction state and settings that PostgreSQL holds for each connection, and hand-written cases only cover the interleavings someone thought of. A property test now drives one connection with random extended-protocol and simple Query traffic and answers each message Proxy forwards from a small model of PostgreSQL: implicit and explicit transactions, failed transactions, skipping until Sync, duplicate names, the unnamed statement and portal, DISCARD ALL, SET LOCAL and temporary tables. Pipelined runs hold back the model's replies until the next Sync or Query, so Proxy reads later messages before it learns how earlier ones ended. After every ReadyForQuery it checks that Proxy holds exactly the statements PostgreSQL holds, holds no operation or portal state at idle, and agrees with PostgreSQL on UNSAFE_DISABLE_MAPPING and on whether the temporary table exists. It runs on a plaintext connection and on one with an encrypted column, and the module lists what the model leaves out. The pipelined runs found the DEALLOCATE ALL rollback bug fixed earlier in this series; the cases they shrank to are saved for replay. bind_message now takes a portal so the model can reuse it.

tobyhede self-assigned this Oct 28, 2026

cipherstash-agent reviewed Oct 28, 2026

View reviewed changes

tobyhede commented Nov 2, 2026

View reviewed changes

tobyhede added 4 commits November 4, 2026 13:01

tobyhede force-pushed the claude/project-thread-0sjl5m-proptest branch from 4d00c2b to 5a8e28a Compare November 4, 2026 02:25

Conversation

tobyhede commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Property test

Bug found: a simple Query kept the unnamed statement

Acknowledgment

Uh oh!

cipherstash-agent left a comment

Choose a reason for hiding this comment

Uh oh!

cipherstash-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobyhede left a comment

Choose a reason for hiding this comment

Review attestation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tobyhede commented Oct 28, 2026 •

edited

Loading