test(integration): add contention validation tests for scoped cipher#373
test(integration): add contention validation tests for scoped cipher#373
Conversation
📝 WalkthroughWalkthroughA new test module for multitenant contention is introduced with three tests that measure mutex contention characteristics, concurrent insert performance scaling, and isolation between concurrent tenant connections using a PostgreSQL database. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
…ped cipher Add three integration tests that prove shared mutex contention exists across concurrent proxy connections. Tests measure scaling factor, per-connection latency under concurrency, and cross-connection blocking. Assertions are set to fail under current shared-client architecture and will pass after the per-connection cipher fix.
tobyhede
force-pushed
the
connection-scoped-cipher
branch
from
3b293b1 to
cd98c26
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/cipherstash-proxy-integration/src/contention.rs`:
- Around line 185-186: The 50ms hardcoded delay in the tokio::time::sleep call
is timing-sensitive and can let task A not yet be in pg_sleep when B starts;
replace this fragile sleep with a deterministic synchronization mechanism (e.g.,
a oneshot or Barrier) or, if you prefer a simpler change, increase the delay and
add a clarifying comment explaining why the longer wait is required; locate the
tokio::time::sleep(std::time::Duration::from_millis(50)).await invocation in
contention.rs (the section that sets up connections A and B and expects A to be
in pg_sleep) and update it to wait on an explicit signal from A (or use a larger
duration with a comment) so the test is reliable under CI/cold-TLS conditions.
🧹 Nitpick comments (1)
packages/cipherstash-proxy-integration/src/contention.rs (1)
11-28: Consider usingexecute()instead ofquery()for INSERT statements.Since INSERT doesn't return meaningful rows here,
execute()would be more idiomatic and clearer in intent.♻️ Suggested change
- client - .query( - "INSERT INTO encrypted (id, encrypted_text) VALUES ($1, $2)", - &[&id, &val], - ) - .await - .unwrap(); + client + .execute( + "INSERT INTO encrypted (id, encrypted_text) VALUES ($1, $2)", + &[&id, &val], + ) + .await + .unwrap();
| // Small delay so A is likely in-flight before B starts | ||
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; |
There was a problem hiding this comment.
Timing sensitivity: 50ms delay may not guarantee A is in pg_sleep.
The 50ms delay assumes connection A has completed the INSERT and entered pg_sleep before B starts. Under slow conditions (CI load, cold TLS handshake), A might still be setting up, reducing test reliability.
Consider adding a synchronization mechanism or increasing the delay with a comment explaining the rationale.
💡 Alternative: use a longer delay with documentation
// Small delay so A is likely in-flight before B starts
- tokio::time::sleep(std::time::Duration::from_millis(50)).await;
+ // Delay to allow A to complete INSERT and enter pg_sleep.
+ // 200ms accounts for TLS handshake + INSERT under CI load.
+ tokio::time::sleep(std::time::Duration::from_millis(200)).await;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Small delay so A is likely in-flight before B starts | |
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; | |
| // Delay to allow A to complete INSERT and enter pg_sleep. | |
| // 200ms accounts for TLS handshake + INSERT under CI load. | |
| tokio::time::sleep(std::time::Duration::from_millis(200)).await; |
🤖 Prompt for AI Agents
In `@packages/cipherstash-proxy-integration/src/contention.rs` around lines 185 -
186, The 50ms hardcoded delay in the tokio::time::sleep call is timing-sensitive
and can let task A not yet be in pg_sleep when B starts; replace this fragile
sleep with a deterministic synchronization mechanism (e.g., a oneshot or
Barrier) or, if you prefer a simpler change, increase the delay and add a
clarifying comment explaining why the longer wait is required; locate the
tokio::time::sleep(std::time::Duration::from_millis(50)).await invocation in
contention.rs (the section that sets up connections A and B and expects A to be
in pg_sleep) and update it to wait on an explicit signal from A (or use a larger
duration with a comment) so the test is reliable under CI/cold-TLS conditions.
Contention tests must run in the multitenant phase where different tenant keysets compete for the shared ZerokmsClient mutexes — the actual customer scenario. Tests now use SET CIPHERSTASH.KEYSET_ID with 3 tenant keysets, separate connection setup from timing, and increase volume to 50 inserts per tenant to surface contention.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Tests are failing. Given @tobyhede is absent, I will fix up the tests.
2 participants
Add three integration tests that prove shared mutex contention exists across concurrent proxy connections. Tests measure scaling factor, per-connection latency under concurrency, and cross-connection blocking. Assertions are set to fail under current shared-client architecture and will pass after the per-connection cipher fix.
Summary by CodeRabbit