Repository navigation
test(proxy): cover mapping-disabled recovery after failed queries - #469
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1146cd4fd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9c14803 to
010e7f3
Compare
1146cd4 to
e0173cd
Compare
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The fix for simple queries is correct. It does the same thing as the mapped error arm, and the regression test fails without it. But the mapping-disabled arm has the same defect for Sync, so extended-protocol clients can still hang.
Blocking: Sync in the mapping-disabled arm does not record a boundary.
In frontend.rs, the if self.context.mapping_disabled() arm sends FrontendMessage::Sync to _ => {}. The mapped arm calls self.context.sync(operation)? (lifecycle.rs:344), which records the schema boundary and the settings boundary. The passthrough arm records neither.
Scenario (development.disable_mapping = true):
- A driver sends Parse/Bind/Execute/Sync in one packet. The Execute fails (for example, a unique violation).
- The proxy reads the Sync before the ErrorResponse comes back. There is no boundary at or after the failed operation.
backend.rscallsreport_outcome→postgres_failed→SettingsProtocol::failed, which setsdiscarding_until_sync = true.ready()does not clear that flag. Onlysync()clears it.- The next client message is not a Sync, so the gate at the top of
interceptsuppresses it. A simpleQueryhangs. A new P/B/E/S batch forwards only its Sync, so the client gets ReadyForQuery without ParseComplete.
Most drivers use the extended protocol, so this case is more common than the simple-query case.
Suggested change: in the same arm, add
FrontendMessage::Sync => self.context.sync(operation)?,For Query, call self.context.sync(operation)? too, not only settings_protocol_boundary, so that the schema boundary is also recorded, as in the mapped error arm. Add a test with the real config value: mapping disabled, P/B/E/S, an ErrorResponse on E, then the next Query is forwarded.
Suggestion: record the boundary in one place.
There are now three sites that must remember to record a boundary: the passthrough Query, the mapped Query error arm, and simple_query. The defect class comes from this. One check near the top of intercept, for Query and Sync in all mapping modes, would make it impossible to forget the boundary in a new arm.
Minor: the test.
fail_querysendsReadyForQuery(FailedTransaction). A failed autocommitSELECT 1 / 0returnsIdle. Thus the test shows the explicit-transaction case only. Please add theIdlecase too, or make the status a parameter.- Also assert
!client.frontend.context.discarding_extended_messages().unwrap()(or a similar check), so that the test shows why the query is forwarded.
I agree with your reply about the CHANGELOG. This bug was never released.
|
@freshtonic thanks, the Sync case was real. Update: the base branch now fixes the hang itself in 189296c, and the merge in 5ecc8e3 takes that implementation unchanged. This PR now adds regression tests only.
All 315 proxy unit tests and CI pass on 5ecc8e3. Generated by Claude Code |
tobyhede
changed the title
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
One change before merging: make the simple-query test detect a missing unmapped_query boundary. Today it passes when that boundary is removed, and no other unit test detects that regression. The pipelined-test reorder and the query_is_forwarded output change are optional.
Both new tests send ReadyForQuery before they check the next Query. SettingsProtocol::ready always clears the discard state, so the tests check ready and not the boundaries that the base branch records in unmapped_query and unmapped_sync. I confirmed this with mutation runs in an isolated copy of HEAD. I also ran the suggested fixes: each one fails on the mutated code and passes on HEAD. I made no changes to this checkout.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 2 posted |
| claude | claude-opus-5-5 | infracode | 0 found, 0 posted |
| claude | claude-opus-5-5 | rust | 3 found, 2 posted |
| codex | gpt-5.6-terra | test-gap | 0 found, 0 posted |
| codex | gpt-5.6-terra | infracode | 0 found, 0 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. 2 comment(s) had a problem that stopped the reader acting; it rewrote 2.
Stack: 1 earlier and 0 later pull request(s) (#467, *️⃣ #469). *️⃣ marks this pull request.
Context loaded: the description, 0 linked issue(s) and 6 discussion entries.
1ecf3a5 to
f83b959
Compare
Check that a failed simple query (autocommit and failed transaction) and a failed pipelined Execute leave the next Query forwarded with mapping disabled. Each test asserts the discard state before ReadyForQuery, so it fails if unmapped_query or unmapped_sync stops recording its boundary. Claude-Session: https://claude.ai/code/session_01UeSgBAqVkHyKtZh6vWgtLR
532bd5b to
b04a123
Compare
This branch has not been deployed
4 participants
Requested by Toby · project thread
Before: with mapping disabled, nothing tested that a connection keeps working after a failed simple query in autocommit (
Idle) or inside a failed transaction.After: two regression tests in
settings_rollback_tests.rscover the mapping-disabled recovery paths.This PR started as the fix for the mapping-disabled simple-query hang. The base branch now fixes it itself in 189296c ("end discard state at ReadyForQuery"), including the pipelined Sync case from review. The merge takes that implementation unchanged, so this PR adds tests only.
How:
failed_simple_query_with_mapping_disabled_does_not_suppress_next_queryfailsSELECT 1 / 0withReadyForQueryIdleand thenFailedTransaction, and asserts the nextQueryis forwarded and the connection is not discarding.failed_pipelined_execute_with_mapping_disabled_does_not_suppress_next_querysends Parse/Bind/Execute/Sync before any backend reply, fails the Execute, and asserts the same. It overlaps with the base'smapping_disabled_extended_error_after_sync_does_not_suppress_pipelined_query, but goes through the full frontend and backend middleware pair.fail_querynow takes theReadyForQuerystatus.Both tests failed before the fix. All 315 proxy unit tests pass against the base's implementation.
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.
https://claude.ai/code/session_01UeSgBAqVkHyKtZh6vWgtLR