Repository navigation
fix: whoami regression to anonymous user - #4303
Conversation
|
Seems reasonable. The purpose of the guard was to unset PGUSER while
ensuring it's restored to the previous value (if any) after the test has
run. (However, even this isn't perfect as @abonander points out in #4275,
because Rust tests can run in parallel.) The PGUSER variable is also read
when parsing the connection string, potentially causing the test to pass
even when the system username can't be read.
I see two approaches here, and we may need input from a project maintainer
to decide:
1. Assume other tests don't set PGUSER, or if they do they make sure they
don't affect other tests as a side-effect (e.g., by disabling parallel
execution). With this approach, we don't need to worry about PGUSER and
your PR is good to go. Disadvantage: if any other tests set PGUSER now or
in the future, this new test may incorrectly pass, and the next regression
may go unnoticed again.
2. Clear PGUSER as in #4275, but then we'd ideally also ensure that doesn't
interfere with other tests, probably by running this one in its own process
(`cargo test` command) as @abonander suggested. Disadvantage: complexity.
Thanks for taking this over while I'm away!
|
|
@svnrb Got it, thanks for clarifying. One question, how about we read the PG_USER during the test in order to assert it's not set? Then if there would be any interference, this "precondition" assert would fail and it wouldn't go unnoticed. It's less complex than changing the env, and safer than current implementation. What do you think? Further, we could assert the PGUSER env is none or different from what we receive from the parsing, for example. Just throwing ideas |
|
I do like the thinking (and the idea of keeping it simple) but
unfortunately I don't think that fully addresses @abonander's concern
since, with tests running in parallel, another test could set/unset PGUSER
while ours is running (e.g., setting it after we check that it's unset but
before we run our actual logic). Global state is a nightmare.
It may work fine in practice (especially today where I don't think any
other tests ever set/unset PGUSER), but theoretically it's racy.
|
|
@svnrb Got it, you're right. So we'll wait on the @abonander direction and I'll fix this PR right up. (: |
|
Ideally, I think we add a CI step that runs the |
|
@abonander Hey, I tried exploring the approach you suggested. What do you think of this version? The github runner uses a Then I copied the run step of general feature tests and replaced the url to leverage the feature that was broken. Failing test with same behavior pointed in #4277: https://github.com/transact-rs/sqlx/actions/runs/27274916678/job/80554161785 (without the fix). I like this version! cc @svnrb |
Awaiting v0.10. Link: transact-rs/sqlx#4303
## Problem sqlx 0.9.0 counts a transaction only after Postgres acknowledges its `BEGIN`. If the future is dropped while `BEGIN` is still in flight (a cancelled REQ history read, a client that disconnects, a timeout), sqlx doesn't queue a `ROLLBACK`. The pool's release ping then reads the late `BEGIN` reply, and the writer pool hands the connection to the next borrower while it is still inside that transaction. The reader pool already closes such connections in its release check (`Db::read_pool_options`). The writer pool has no release check. Load tests showed what happens next: - `there is already a transaction in progress` (25001) on the next event read's `BEGIN`. - Sometimes a burst of `current transaction is aborted` (25P02). The leftover from the event read is `READ ONLY`, so the next write fails and aborts it, and every later statement on that connection fails until the connection is closed. During a burst, ban checks failed closed with 503 and window reads returned 500. - A read-write leftover would also leave autocommit writes uncommitted, and the next read's `ROLLBACK` would discard them. ## Change - Pin every sqlx crate to transact-rs/sqlx#4394 (merged, not yet released). That commit takes the transaction count before the `BEGIN` round trip, so a cancelled begin queues its `ROLLBACK`. The crates share types, so they have to move together. The pin also picks up the other 23 commits between v0.9.0 and that fix, including the libsqlite3-sys fix `sqlx-sqlite` was already pinned for. - Add a regression test, `writer_pool_postgres_tests::begin_cancelled_in_flight_leaves_no_writer_transaction_open`. It holds `BEGIN` in flight behind a paused proxy and times it out, for both the event read's `BEGIN ... READ ONLY` and a plain `begin()`. It then asserts that the next borrower's autocommit write commits and that its backend is `idle`, not inside a transaction. Other upstream changes in the pin that reach the Postgres path: `TCP_NODELAY` on sockets (transact-rs/sqlx#4336), reuse of the TLS connector across connection attempts (transact-rs/sqlx#4242), a fix for a memory leak when the statement cache is disabled (transact-rs/sqlx#4337), a describe-time generic-plan change used only by the macros (transact-rs/sqlx#4286), and the `whoami` default-user fix (transact-rs/sqlx#4303). The rest are SQLite, MySQL, CLI, or docs changes. Remove the pin once sqlx 0.9.1 ships. ## What got simpler The lockfile loses its second `sqlx-core` copy, which the sqlx-sqlite-only pin used to pull in. No runtime code changes. A pool-level release check would have fixed the same bug, but it costs a round trip on every writer release. Comments that described the 0.9.0 behavior as current are refreshed: the reader pool's release check (`Db::read_pool_options`) and the lease heartbeat (`EntityDeletionStore::heartbeat`). The reader test `proof_timeout_mid_begin_leaves_no_reader_transaction_open` asserted that the release check closed the connection left mid-`BEGIN`. With the pin, sqlx rolls that transaction back itself, so the test now asserts the check finds nothing to close. That also catches a sqlx regression the check would otherwise hide. Identified debt (not in this PR): the release check's open-transaction arm, and the reader-session `default_transaction_isolation` pin it depends on, are now only a backstop against a sqlx regression. They could be removed in a follow-up. ## Testing - Against sqlx 0.9.0 (main's `Cargo.toml`/`Cargo.lock` at 962b3fd), the new writer test fails for each `BEGIN` form on its own. The event read fails with `cannot execute INSERT in a read-only transaction`, and after a plain `begin()` the next borrower's backend is not `idle`. With the pin it passes 5/5. - Single-machine load-test A/B against main (962b3fd), with three main runs and two pin runs. Main logged 25001 and 25P02 in every run (41/80, 10/9, 12/18); the pin logged none. Client error rate, latency, and throughput matched within run-to-run noise. - Full suite on the pin: every `just ci` recipe, the PostgreSQL lane, and `just test`. The only failure from the pin was the reader test above. Reds that don't involve sqlx: the desktop Tauri recipes (`alsa-sys` needs ALSA headers on the build host), `mobile-check`/`mobile-test` (a Flutter SDK `ColorScheme` type clash), and two `buzz-acp` `git_bootstrap` tests (no git credentials on the host). Two bridge search-timeout tests failed only under the full parallel lane; run alone they pass on both the pin and 0.9.0. - Two later commits change only tests, comments, and `Cargo.lock` (the lock now moves only the sqlx crates). At those heads: the PostgreSQL lane, the unit lane, and workspace clippy pass. Three tests failed once under a contended full lane, and each passed 3/3 alone on both the pin and 0.9.0. The two transaction tests fail on 0.9.0 and pass on the pin. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Luke Tornquist <[email protected]> Co-authored-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <[email protected]>
Does your PR solve an issue?
Fixes #4277
Is this a breaking change?
No, just a bug fix.
Reference
This PR is just a rehash of #4275 from @svnrb.
I just re-opened this PR because he said he was gonna be away for 2 weeks and I have so time to maybe move this along!