Repository navigation
fix(storage): connect over TLS and keep transaction refs addressable - #913
Conversation
🦋 Changeset detectedLatest commit: 2886883 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen 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 |
[Medium impact] The PR appears safe to merge; the remaining TLS test suggestion is non-blocking. Findings
|
| 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"] } |
There was a problem hiding this comment.
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!
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:40selectedtls-none, so a database that requiressslmode=requirecould not be reached: sqlx refuses the upgrade before a handshake. The workspace dependency now enablestls-rustls-ring-webpki— ring with webpki roots, nonative-tls.Cargo.lockgains 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-nonecannot go green again):parity.rs—a_tls_connection_is_attempted_rather_than_refused_by_this_buildopens asslmode=requireconnection, requiresSELECT 1when the server offers TLS, and otherwise requires the server to refuse rather than the build. It runs in the existingStorage paritystep, with no new service.Measured against PostgreSQL 16.15, self-signed cert,
ssl=on:tls-none(old)TLS upgrade required by connect options but SQLx was built without TLS support enabledtls-rustls-ring-webpki(new)SELECT 1= 12. A transaction group keeps its refs map (#912)
run_transactioncalledrun_op(..., &mut HashMap::new()), so every nested op started with an empty side channel: anoutbox.enqueuein a group could not be addressed by anoutbox.mark_deliveredin 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'srunTransactiongoes throughrunOpfor the same reason.A new transaction group in
fixtures/repository-parity.jsonenqueuestransaction.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 aregenerated_onand two randomref.idUUIDs.the_typescript_script_replays_with_an_empty_diff ... okop 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).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.