Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request implements connection timeout handling for the proxy, including configuration defaults (120 seconds), error formatting with millisecond precision, appropriate error responses to clients, and comprehensive integration tests validating connection resilience under concurrent and load scenarios. Documentation and Docker setup are updated to clarify testing workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cipherstash-proxy-integration/src/connection_resilience.rs`:
- Around line 145-168: The readiness Notify (a_ready/a_ready_tx) can fire before
client_b is actually blocked, making the test flaky; replace the notify_one() +
fixed sleep logic with a deterministic wait that polls Postgres (e.g. pg_locks
or pg_stat_activity) on the direct connection until the proxy connection is
observed waiting for the advisory lock. Concretely: stop using
a_ready/a_ready_tx as the sole signal; after spawning b_handle (which uses
connect_with_tls(PROXY) and executes b_lock_query), poll from the direct
Postgres client (the direct connection used earlier) by running a query against
pg_locks/pg_stat_activity and check for client_b’s PID/status showing it is
waiting for the advisory lock, and only then proceed to measure connection C;
ensure the polling loop has a sensible timeout and fails the test if the wait
state never appears.
In `@packages/cipherstash-proxy/src/postgresql/handler.rs`:
- Around line 262-276: The ConnectionTimeout path must emit the ErrorResponse
not only after tokio::try_join! but also for early returns before the stream
split (the timeout occurrences at the earlier startup/auth branches referenced
in the review). Add a small helper (e.g.,
emit_connection_timeout(timeout_sender, err)) that builds
ErrorResponse::connection_timeout(err.to_string()), attempts
BytesMut::try_from(...), sends via timeout_sender, and does a
tokio::task::yield_now().await; then call that helper in the pre-split timeout
return sites (the branches that currently return Error::ConnectionTimeout around
the startup/password-exchange logic) instead of returning immediately, or move
the timeout-emission into a centralized early-return path so all
Error::ConnectionTimeouts use the same emission logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8410d5b-183a-44fe-9ca1-63f8f6ea7dda
📒 Files selected for processing (11)
DEVELOPMENT.mdmise.tomlpackages/cipherstash-proxy-integration/src/common.rspackages/cipherstash-proxy-integration/src/connection_resilience.rspackages/cipherstash-proxy-integration/src/lib.rspackages/cipherstash-proxy/src/config/database.rspackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/postgresql/error_handler.rspackages/cipherstash-proxy/src/postgresql/handler.rspackages/cipherstash-proxy/src/postgresql/messages/error_response.rstests/docker-compose.yml
packages/cipherstash-proxy-integration/src/connection_resilience.rs
Outdated
Show resolved
Hide resolved
tobyhede
force-pushed
the
connection-error-handling
branch
from
468f4f2 to
eec033f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/cipherstash-proxy-integration/src/connection_resilience.rs (2)
16-18:⚠️ Potential issue | 🟠 MajorTie the advisory-lock poll to connection B, not just the lock ID.
Right now
poll_queryonly proves that some backend is waiting onADVISORY_LOCK_ID. Because the key is hard-coded at Line 18 andclient_bis created inside the task, another concurrent suite can satisfy this poll before this test's session is actually blocked. Createclient_bbeforetokio::spawn, capture its backend PID, and filterpg_locksby that PID (or generate a per-test lock key) so the readiness gate stays specific to this test.In PostgreSQL, can `pg_locks` be filtered by backend `pid` to identify the session waiting on a specific advisory lock, and how are `pg_advisory_lock(bigint)` keys represented in `pg_locks`?Also applies to: 141-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy-integration/src/connection_resilience.rs` around lines 16 - 18, The readiness poll currently checks pg_locks only by the hard-coded ADVISORY_LOCK_ID (const ADVISORY_LOCK_ID) which can match a different test's session; create the client used to acquire the lock (client_b) before calling tokio::spawn so you can obtain its backend PID, then update poll_query to filter pg_locks by that PID (or alternatively generate a unique per-test ADVISORY_LOCK_ID) so the poll verifies that the specific client_b session is blocked on pg_advisory_lock rather than any session using the same numeric key; adjust the code paths that call poll_query (and the spawned task that runs the lock) to use the captured PID or unique key accordingly.
32-33:⚠️ Potential issue | 🟠 MajorReplace these sleep-based readiness gates with deterministic checks.
Line 33, Line 66, and Line 93 are still using fixed delays to guess when the previous state change has happened. That means these tests can pass before the slow workload is actually active, or only fail because teardown/startup took longer on CI. Please gate these assertions on an observable condition instead, similar to the
pg_lockspolling used later in this file, or remove the grace period entirely in the disconnect test so it exercises the immediate reconnect path.Also applies to: 65-66, 92-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy-integration/src/connection_resilience.rs` around lines 32 - 33, The fixed-duration tokio::time::sleep(Duration::from_millis(200)).await calls are nondeterministic readiness gates; replace them with deterministic polling of an observable condition (e.g., query the same pg_locks check used later in this file or poll the proxy/connection state) so the test waits until the expected lock/worker state is actually present before proceeding; alternatively, for the disconnect test remove the grace period entirely and assert the immediate reconnect path by checking the connection became active/inactive via the observable (use the same pg_locks or connection-status query code path already in this file rather than sleeping).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cipherstash-proxy-integration/src/connection_resilience.rs`:
- Around line 16-18: The readiness poll currently checks pg_locks only by the
hard-coded ADVISORY_LOCK_ID (const ADVISORY_LOCK_ID) which can match a different
test's session; create the client used to acquire the lock (client_b) before
calling tokio::spawn so you can obtain its backend PID, then update poll_query
to filter pg_locks by that PID (or alternatively generate a unique per-test
ADVISORY_LOCK_ID) so the poll verifies that the specific client_b session is
blocked on pg_advisory_lock rather than any session using the same numeric key;
adjust the code paths that call poll_query (and the spawned task that runs the
lock) to use the captured PID or unique key accordingly.
- Around line 32-33: The fixed-duration
tokio::time::sleep(Duration::from_millis(200)).await calls are nondeterministic
readiness gates; replace them with deterministic polling of an observable
condition (e.g., query the same pg_locks check used later in this file or poll
the proxy/connection state) so the test waits until the expected lock/worker
state is actually present before proceeding; alternatively, for the disconnect
test remove the grace period entirely and assert the immediate reconnect path by
checking the connection became active/inactive via the observable (use the same
pg_locks or connection-status query code path already in this file rather than
sleeping).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b6eefc7-81f1-422b-bca9-856c9391b822
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/cipherstash-proxy-integration/src/connection_resilience.rspackages/cipherstash-proxy/Cargo.tomlpackages/cipherstash-proxy/src/config/database.rspackages/cipherstash-proxy/src/postgresql/error_handler.rspackages/cipherstash-proxy/src/postgresql/handler.rs
freshtonic
requested changes
Mar 10, 2026
Contributor
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Looks good but the tests are failing. I've added a comment with my hypothesis as to why.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CLAUDE.md (1)
59-63: Consider enhancing the note with additional context.The current guidance is helpful and actionable. To make it more comprehensive, consider these optional enhancements:
- Explain the context: Mention that this is particularly relevant when running integration tests with many concurrent connections
- Shell inclusivity: Reference both Zsh (
.zshrc) and Bash (.bash_profileor.bashrc) users- macOS system limits: Note that on some macOS versions, the system-wide hard limit might also need adjustment (e.g., via
launchctl limit maxfiles)📝 Optional enhanced version
-> **macOS Note:** If you hit file descriptor limits during development (e.g. "Too many open files"), you may need to increase the limit: +> **macOS Note:** If you hit file descriptor limits during development (e.g., "Too many open files" when running integration tests with many concurrent connections), you may need to increase the limit: > ```bash > ulimit -n 10240 > ``` -> To make this persistent, add it to your shell profile (e.g. `~/.zshrc`). +> To make this persistent, add it to your shell profile (e.g., `~/.zshrc` for Zsh or `~/.bash_profile` for Bash). On some macOS versions, you may also need to adjust system-wide limits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 59 - 63, Update the macOS note around the "ulimit -n 10240" guidance: add a brief sentence that this is most relevant when running integration tests or workloads with many concurrent connections, mention persistence instructions for both Zsh and Bash (e.g., "~/.zshrc" and "~/.bash_profile" or "~/.bashrc"), and add a short note that some macOS versions may also require adjusting system-wide hard limits (e.g., via "launchctl limit maxfiles") so users know to check both per-shell and system limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CLAUDE.md`:
- Around line 59-63: Update the macOS note around the "ulimit -n 10240"
guidance: add a brief sentence that this is most relevant when running
integration tests or workloads with many concurrent connections, mention
persistence instructions for both Zsh and Bash (e.g., "~/.zshrc" and
"~/.bash_profile" or "~/.bashrc"), and add a short note that some macOS versions
may also require adjusting system-wide hard limits (e.g., via "launchctl limit
maxfiles") so users know to check both per-shell and system limits.
tobyhede
force-pushed
the
connection-error-handling
branch
from
7e8331d to
61f92bb
Compare
…r reporting - Add configurable connection_timeout (default 120s, 0 to disable) to DatabaseConfig - Fix ConnectionTimeout error message to display milliseconds (was showing seconds) - Map ConnectionTimeout to PostgreSQL error code 57P05 instead of generic 58000 - Send ErrorResponse to client on timeout via channel writer - Add pull_policy: never to docker-compose proxy services to ensure local builds - Add connect_timeout to integration test client config - Add unit tests for config defaults, error message format, and error code mapping - Add integration tests for connection isolation under load - Document local build requirement in DEVELOPMENT.md
- Pass actual timeout duration in ErrorResponse message to client - Document rationale for using 57P05 over 08006 error code - Document yield_now() as best-effort flush with known limitation - Document advisory lock test race condition timing margin - Replace magic number 12345 with named ADVISORY_LOCK_ID constant
metrics 0.24.1 has a lifetime issue (rust-lang/rust#141402) that causes compilation failures on recent Rust versions.
Replace Notify + fixed sleep in advisory lock test with pg_locks polling for deterministic synchronization. Send ErrorResponse to clients for pre-split connection timeouts instead of silent disconnect.
client_a was hardcoded to PG_PORT (5532) while the proxy may connect to a different PostgreSQL instance (e.g. postgres-tls:5617 in TLS CI). This meant the advisory lock was held on a different server than client_b was targeting, so the lock never blocked and the test was flaky in CI. Use get_database_port() to ensure client_a connects to the same PostgreSQL instance the proxy uses.
tobyhede
force-pushed
the
connection-error-handling
branch
from
61f92bb to
5ebf5a8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cipherstash-proxy/src/config/database.rs (1)
78-86: Consider documenting this behavioral change.This introduces a significant change: previously, omitting
connection_timeoutin config meant no timeout (unbounded reads), but now it defaults to 120 seconds. Based on the relevant code snippets, this affectsprotocol::read_message,startup::read_message, and backend reads across the codebase.The implementation is correct, but this silent behavior change could surprise existing deployments. Consider:
- Adding a note in release/changelog
- Documenting
connection_timeout = 0as the explicit opt-out in config documentation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cipherstash-proxy/src/config/database.rs` around lines 78 - 86, The change to connection_timeout() now treats None as a 120s default via DEFAULT_CONNECTION_TIMEOUT_MS which alters prior behavior where omitting connection_timeout meant no timeout; update documentation and release notes to call this out and instruct users to set connection_timeout = 0 in their config to explicitly opt out of timeouts, and add a short note in the changelog and any config docs that reference protocol::read_message, startup::read_message, and backend read behavior to prevent surprises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cipherstash-proxy/src/config/database.rs`:
- Around line 78-86: The change to connection_timeout() now treats None as a
120s default via DEFAULT_CONNECTION_TIMEOUT_MS which alters prior behavior where
omitting connection_timeout meant no timeout; update documentation and release
notes to call this out and instruct users to set connection_timeout = 0 in their
config to explicitly opt out of timeouts, and add a short note in the changelog
and any config docs that reference protocol::read_message,
startup::read_message, and backend read behavior to prevent surprises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ece25c30-5cab-4b82-8b53-e991b1f5aa65
📒 Files selected for processing (12)
CLAUDE.mdDEVELOPMENT.mdmise.tomlpackages/cipherstash-proxy-integration/src/common.rspackages/cipherstash-proxy-integration/src/connection_resilience.rspackages/cipherstash-proxy-integration/src/lib.rspackages/cipherstash-proxy/src/config/database.rspackages/cipherstash-proxy/src/error.rspackages/cipherstash-proxy/src/postgresql/error_handler.rspackages/cipherstash-proxy/src/postgresql/handler.rspackages/cipherstash-proxy/src/postgresql/messages/error_response.rstests/docker-compose.yml
✅ Files skipped from review due to trivial changes (4)
- packages/cipherstash-proxy-integration/src/lib.rs
- DEVELOPMENT.md
- packages/cipherstash-proxy-integration/src/connection_resilience.rs
- packages/cipherstash-proxy/src/postgresql/handler.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- CLAUDE.md
- tests/docker-compose.yml
- packages/cipherstash-proxy/src/error.rs
- packages/cipherstash-proxy-integration/src/common.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Summary by CodeRabbit
New Features
Documentation