Skip to content

fix(storage): connect over TLS and keep transaction refs addressable - #913

Merged
cameri merged 2 commits into
rewrite/mainfrom
factory/007-followups
Oct 20, 2026
Merged

cameri merged 2 commits into
rewrite/mainfrom
factory/007-followups

Conversation

@chappie-daemon

@chappie-daemon chappie-daemon commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

The two follow-ups from factory job 007, found by Greptile after #910 merged as 827558a.

Refs #911 (the TLS parity leg against a TLS-enabled Postgres stays open there). Fixes #912.

1. sqlx can negotiate TLS (#911)

Cargo.toml:40 selected tls-none, so a database that requires sslmode=require could not be reached: sqlx refuses the upgrade before a handshake. The workspace dependency now enables tls-rustls-ring-webpki — ring with webpki roots, no native-tls. Cargo.lock gains only the TLS stack (ring, rustls, rustls-webpki, webpki-roots, zeroize and their platform shims).

A committed regression guard is included (so a later tls-none cannot go green again): parity.rs — a_tls_connection_is_attempted_rather_than_refused_by_this_build opens a sslmode=require connection, requires SELECT 1 when the server offers TLS, and otherwise requires the server to refuse rather than the build. It runs in the existing Storage parity step, with no new service.

Measured against PostgreSQL 16.15, self-signed cert, ssl=on:

build against a TLS server against a plaintext server
tls-none (old) — fails: TLS upgrade required by connect options but SQLx was built without TLS support enabled
tls-rustls-ring-webpki (new) passes: connects over TLS, SELECT 1 = 1 passes: the server refuses the upgrade, not the build

2. A transaction group keeps its refs map (#912)

run_transaction called run_op(..., &mut HashMap::new()), so every nested op started with an empty side channel: an outbox.enqueue in a group could not be addressed by an outbox.mark_delivered in the same group, and the group recorded {failed: true, at: 1} where TypeScript succeeded. The caller's map is now threaded through the nested calls, exactly as the top-level loop already did, and the capture generator's runTransaction goes through runOp for the same reason.

A new transaction group in fixtures/repository-parity.json enqueues transaction.enqueue.mark, then marks the same row delivered. The fixture was regenerated from the TypeScript capture (gen-storage-parity.mjs, capture commit 242a300, PostgreSQL 16.15, TZ=UTC), not hand-edited: the only differences from the committed fixture besides the new group are generated_on and two random ref.id UUIDs.

parity replay result
threaded map (this PR) op 117 passes; the_typescript_script_replays_with_an_empty_diff ... ok
fresh map per op (reverted) fails: op 117 (transaction) differs from the TypeScript capture — left: {"at": 1, "failed": true}, right: {"failed": false, "results": [...]}

Both controls were run in the 007 worktree with the fix reverted and restored; the shared checkout was not touched.

Checks

Against local PostgreSQL 16 containers with the pinned toolchain 1.99.1:

  • cargo check --workspace --all-features — clean.
  • cargo fmt --all --check — clean.
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — clean.
  • cargo test --workspace --all-features — every binary ok, no failures.
  • cargo test -p nostream-storage --features postgres --test parity -- --ignored — 2 passed (TLS guard + replay).
  • Under tls-none, the replay (plain connection) still passed while the TLS guard failed, which isolates the control to the TLS build.

A changeset is included.

@changeset-bot

changeset-bot Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2886883

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@greptile-apps

greptile-apps Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; the remaining TLS test suggestion is non-blocking.

Findings

  1. P2 TLS support lacks regression coverage ▶

Summary

Enables TLS connections through sqlx and keeps enqueue IDs available across operations in a transaction.

  • Postgres connections can use TLS when the server requires it.
  • Transaction replay keeps enqueued row IDs available to later steps.

Reviews (2) · Last reviewed commit: "test(storage): guard the TLS feature in ..." · Reviewed by Greptile

Comment thread Cargo.toml
serde_yaml = "0.9"
sha2 = "0.11.1"
sqlx = { version = "0.9", default-features = false, features = ["runtime-tokio", "postgres", "migrate", "macros", "chrono", "json", "tls-none"] }
sqlx = { version = "0.9", default-features = false, features = ["runtime-tokio", "postgres", "migrate", "macros", "chrono", "json", "tls-rustls-ring-webpki"] }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 TLS support lacks regression coverage

The TLS fix has no committed regression test. The database-backed CI tests use PostgreSQL without TLS, so switching sqlx back to tls-none could pass those tests while breaking TLS-required connections again. Keep a test that connects to a TLS-enabled PostgreSQL server with sslmode=require and runs a query.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants