feat: properly handle concurrent requests from Memory and Redis stores #15
Conversation
WalkthroughThe pull request introduces changes to the Rack Idempotency Key library, focusing on enhancing error handling and simplifying request processing. Key modifications include the introduction of specific error classes like Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
lib/rack/idempotency_key/error.rb (1)
17-18: Consider adding a default message for StoreError.For consistency with
ConflictError, consider adding a default message toStoreErrorto provide more context when the error occurs.- StoreError = Class.new(Error) + # Error raised for general store failures + class StoreError < Error + DEFAULT_MESSAGE = "An error occurred while accessing the store." + + def initialize(msg = DEFAULT_MESSAGE) + super(msg) + end + endlib/rack/idempotency_key.rb (1)
25-28: Consider using application/json for error responses.While the error handling is good, consider:
- Using JSON format for error responses to maintain API consistency
- Including error codes/types for better client-side handling
- [409, { "Content-Type" => "text/plain" }, [e.message]] + [409, { "Content-Type" => "application/json" }, [{ + error: { + type: "conflict", + message: e.message + } + }.to_json]]spec/support/shared_examples/describe_store_get_and_set_methods.rb (1)
43-44: LGTM! Consider updating the test description.The test correctly verifies that attempting to set a duplicate key raises a
ConflictErrorand preserves the original value. This aligns well with the PR objective.Consider updating the test description to better reflect the expected behavior:
- it "does not override the existing value" do # rubocop:disable RSpec/MultipleExpectations + it "raises ConflictError and preserves the existing value" do # rubocop:disable RSpec/MultipleExpectations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/rack/idempotency_key.rb(2 hunks)lib/rack/idempotency_key/error.rb(1 hunks)lib/rack/idempotency_key/memory_store.rb(1 hunks)lib/rack/idempotency_key/redis_store.rb(1 hunks)lib/rack/idempotency_key/request.rb(0 hunks)spec/rack/idempotency_key/redis_store_spec.rb(1 hunks)spec/support/shared_examples/describe_store_get_and_set_methods.rb(1 hunks)
💤 Files with no reviewable changes (1)
- lib/rack/idempotency_key/request.rb
🔇 Additional comments (5)
lib/rack/idempotency_key/error.rb (1)
8-15: LGTM! Well-structured error class with clear default message.The
ConflictErrorimplementation follows Ruby best practices with:
- Clear documentation
- Inherits from base Error class
- Customizable message with sensible default
lib/rack/idempotency_key.rb (1)
36-39: LGTM! Clean and efficient request handling.The simplified request handling logic is more maintainable and follows a clear flow:
- Check cache first
- Execute request if not cached
- Cache the response
spec/rack/idempotency_key/redis_store_spec.rb (1)
19-19: LGTM! Comprehensive error handling tests.The tests properly verify that Redis errors are wrapped in
StoreError, maintaining a clean abstraction of the underlying storage implementation.Also applies to: 27-27
lib/rack/idempotency_key/redis_store.rb (2)
20-20: LGTM! Consistent error class usage.The error class has been consistently updated from
Store::ErrortoStoreErroracross both error handling blocks.Also applies to: 31-31
24-28: LGTM! Robust implementation of conflict detection.The implementation correctly uses Redis's atomic
SET NXoperation to handle race conditions and conflicts. This ensures thread-safety and properly raisesConflictErrorwhen attempting to set a duplicate key.Let's verify that Redis is configured with the appropriate parameters:
✅ Verification successful
Redis configuration approach is well-designed and flexible ✅
The implementation correctly delegates Redis configuration to the application layer while supporting both direct Redis connections and connection pools. The documentation provides good examples with reasonable defaults.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Redis configuration in the codebase # Look for Redis configuration files or initialization code rg -g '!spec/*' 'Redis.(new|create)' -A 5Length of output: 1089
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/rack/idempotency_key/memory_store.rb (1)
28-34:⚠️ Potential issueFix race condition and simplify store assignment.
- The race condition issue between checking for key existence and setting the value still exists.
- The
||=operator is unnecessary since we're already checking for key existence.def set(key, value, ttl: expires_in) mutex.synchronize do - raise Rack::IdempotencyKey::ConflictError if store.key?(key) - - store[key] ||= { value: value, expires_at: Time.now.utc + ttl } + if store.key?(key) + raise Rack::IdempotencyKey::ConflictError + else + store[key] = { value: value, expires_at: Time.now.utc + ttl } + end store[key][:value] end end
🧹 Nitpick comments (2)
lib/rack/idempotency_key/request.rb (2)
6-6: Consider making the lock TTL configurable.While 60 seconds is a reasonable default, it might be insufficient for long-running operations. Consider making this value configurable through initialization options.
class Request + class << self + attr_accessor :lock_ttl + end + + self.lock_ttl = 60 # seconds + - DEFAULT_LOCK_TTL = 60 # seconds
72-74: Enhance lock key generation for distributed systems.The current implementation might lead to key conflicts in distributed systems. Consider using a more robust key generation strategy.
def lock_key - "#{idempotency_key}_lock" + "#{self.class.name.downcase}:lock:#{Digest::SHA256.hexdigest(idempotency_key)}" endThis change:
- Adds namespace to prevent conflicts with other systems
- Hashes the key to ensure consistent length and valid characters
- Uses colon separator following Redis conventions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/rack/idempotency_key.rb(2 hunks)lib/rack/idempotency_key/memory_store.rb(2 hunks)lib/rack/idempotency_key/redis_store.rb(1 hunks)lib/rack/idempotency_key/request.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/rack/idempotency_key.rb
🔇 Additional comments (6)
lib/rack/idempotency_key/memory_store.rb (3)
Line range hint
19-23: LGTM! Improved expiration check logic.The change from
added_attoexpires_atmakes the expiration check more direct and accurate by using an absolute expiration time.
37-39: LGTM! Thread-safe key removal.The
unsetmethod is correctly implemented with proper mutex synchronization.
45-46: LGTM! Simplified expiration check.The expiration check is now more straightforward with a direct comparison against the absolute expiration time.
lib/rack/idempotency_key/redis_store.rb (2)
20-20: LGTM! Consistent error handling.The error class update aligns with the new error handling approach across the codebase.
23-32: LGTM! Robust Redis key setting with conflict handling.The implementation is solid with:
- Atomic operation using Redis's
nxflag- Proper TTL handling
- Consistent error handling for conflicts
lib/rack/idempotency_key/request.rb (1)
28-36: Verify the migration fromwith_lock!tolocked!.Since
with_lock!was removed, we need to ensure all callers have been updated.✅ Verification successful
Migration from
with_lock!tolocked!is complete and correct ✅🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of with_lock! rg "with_lock!" . # Search for store implementations to verify locking support rg "class.*Store" -A 10Length of output: 2265
Script:
#!/bin/bash # Search for set/unset methods in store implementations rg "def (set|unset)" -A 5 # Search for locked! usage rg "locked!" -A 5Length of output: 2564
matteoredz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
spec/rack/idempotency_key/redis_store_spec.rb (1)
42-46: Consider adding tests for edge cases.While the current tests cover the basic success and error scenarios, consider adding tests for:
- Attempting to unset a non-existent key
- Race condition scenarios (if relevant to your use case)
- Handling of nil/empty keys
context "when the key doesn't exist" do it "doesn't raise an error" do expect { store.unset("non_existent_key") }.not_to raise_error end end context "with invalid input" do it "handles nil keys gracefully" do expect { store.unset(nil) }.to raise_error(ArgumentError) end it "handles empty keys gracefully" do expect { store.unset("") }.to raise_error(ArgumentError) end endspec/rack/idempotency_key_spec.rb (3)
95-106: Consider enhancing error response testing.While the test correctly verifies the 409 status code, it would be valuable to also test the error response body and headers to ensure they provide meaningful information about the conflict.
Consider adding these test cases:
context "when another concurrent request was issued" do before do allow(app).to( receive(:handle_request!).and_raise(Rack::IdempotencyKey::ConflictError) ) end it "responds with a 409" do post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } expect(last_response.status).to eq(409) end + + it "includes error details in the response" do + post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } + expect(last_response.headers["Content-Type"]).to eq("application/json") + expect(JSON.parse(last_response.body)).to include( + "error" => "conflict", + "message" => match(/concurrent request/i) + ) + end end
108-119: Enhance store failure test coverage.While the test verifies the 503 status code, consider adding more comprehensive test coverage for store failures:
- Test response body/headers for error details
- Test different store failure scenarios (e.g., connection timeout, storage full)
Consider expanding the test cases:
context "when the underlying store fails" do - before do - allow(app).to( - receive(:handle_request!).and_raise(Rack::IdempotencyKey::StoreError) - ) - end + shared_examples "store failure response" do |error_message| + it "responds with a 503 and error details" do + post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } + expect(last_response.status).to eq(503) + expect(last_response.headers["Content-Type"]).to eq("application/json") + expect(JSON.parse(last_response.body)).to include( + "error" => "store_error", + "message" => match(/#{error_message}/i) + ) + end + end + context "when store is unavailable" do + before do + allow(app).to( + receive(:handle_request!).and_raise( + Rack::IdempotencyKey::StoreError, "Store connection failed" + ) + ) + end + include_examples "store failure response", "connection failed" + end + context "when store is full" do + before do + allow(app).to( + receive(:handle_request!).and_raise( + Rack::IdempotencyKey::StoreError, "Insufficient storage" + ) + ) + end + include_examples "store failure response", "insufficient storage" + end end
94-119: Consider additional test scenarios for edge cases.The new error handling tests are well-structured, but consider adding these scenarios to improve coverage:
- Test with various idempotency key formats (e.g., UUID, custom formats)
- Test key expiration scenarios
- Test response caching behavior with different HTTP methods
Would you like me to provide example test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/rack/idempotency_key/redis_store.rb(1 hunks)spec/rack/idempotency_key/redis_store_spec.rb(1 hunks)spec/rack/idempotency_key_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/rack/idempotency_key/redis_store.rb
🔇 Additional comments (3)
spec/rack/idempotency_key/redis_store_spec.rb (3)
19-19: LGTM! Error class updates are consistent.The error class updates from
Store::ErrortoStoreErroralign with the broader refactoring of error handling in the library.Also applies to: 27-27
32-40: LGTM! Well-structured test for the success case.The test properly verifies the basic functionality of the
unsetmethod.
19-19: Verify error handling consistency.Let's ensure the error class changes are consistent with the implementation in
lib/rack/idempotency_key/redis_store.rb.Also applies to: 27-27, 46-46
✅ Verification successful
Error handling is consistent across the codebase ✅
The implementation in
lib/rack/idempotency_key/redis_store.rbcorrectly usesRack::IdempotencyKey::StoreErrorfor all error cases, matching the test expectations. No instances of the old error class format were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the StoreError class is consistently used in the Redis store implementation # Check the implementation file for error class usage rg -A 2 "StoreError|Store::Error" "lib/rack/idempotency_key/redis_store.rb" # Check for any remaining instances of the old error class rg -l "Store::Error" .Length of output: 371
matteoredz
force-pushed
the
distributed-lock
branch
from
c5f87ba to
b8487e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
spec/rack/idempotency_key/redis_store_spec.rb (1)
32-59: Consider adding tests for these edge cases:To ensure complete coverage, consider adding test cases for:
- Keys with special characters
- Very long keys
- Empty string keys
- Keys with different encodings
Example:
context "with special cases" do it "handles special characters in keys" do special_key = "test@#$%^" store.set(special_key, 1) expect { store.unset(special_key) }.not_to raise_error end it "handles empty string keys" do expect { store.unset("") }.not_to raise_error end endspec/rack/idempotency_key_spec.rb (3)
95-106: Enhance test coverage for concurrent request handling.While the test correctly verifies the 409 status code, consider adding the following test cases for more comprehensive coverage:
- Verify the response body contains the error message.
- Check for any specific headers that provide additional context about the conflict.
- Test with different HTTP methods to ensure consistent behavior.
Example additions:
it "includes the error message in the response" do post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } expect(last_response.body).to include("concurrent request") end it "sets appropriate error headers" do post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } expect(last_response.headers).to include("X-Error-Type" => "conflict") end %w[put patch delete].each do |method| it "handles #{method.upcase} requests consistently" do send(method, "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" }) expect(last_response.status).to eq(409) end end
108-119: Enhance test coverage for store failure handling.While the test correctly verifies the 503 status code, consider adding the following test cases for more comprehensive coverage:
- Verify the response body contains a meaningful error message.
- Check for headers that provide additional context about the store failure.
- Test different store operations (get/set) to ensure consistent error handling.
- If implemented, test any retry mechanisms.
Example additions:
it "includes a meaningful error message in the response" do post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } expect(last_response.body).to include("store unavailable") end it "sets appropriate error headers" do post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } expect(last_response.headers).to include( "X-Error-Type" => "store_error", "Retry-After" => be_present ) end context "when different store operations fail" do it "handles get operation failure" do allow(store).to receive(:get).and_raise(Rack::IdempotencyKey::StoreError) post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } expect(last_response.status).to eq(503) end it "handles set operation failure" do allow(store).to receive(:set).and_raise(Rack::IdempotencyKey::StoreError) post "/", {}, { "HTTP_IDEMPOTENCY_KEY" => "123456789" } expect(last_response.status).to eq(503) end end
94-119: Add documentation for error handling test cases.Consider adding documentation comments above each error handling context to explain:
- The scenario being tested
- Expected behavior
- Any specific requirements or edge cases
- References to related documentation or issues
Example:
# Tests the middleware's behavior when concurrent requests are made with the same # idempotency key. This ensures that the middleware properly handles race conditions # and prevents duplicate processing of requests. context "when another concurrent request was issued" do # ... end # Tests the middleware's behavior when the underlying store (Redis/Memory) fails. # The middleware should fail gracefully and inform clients to retry their requests # after the store becomes available again. context "when the underlying store fails" do # ... end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/rack/idempotency_key/redis_store.rb(2 hunks)spec/rack/idempotency_key/redis_store_spec.rb(1 hunks)spec/rack/idempotency_key_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/rack/idempotency_key/redis_store.rb
🔇 Additional comments (2)
spec/rack/idempotency_key/redis_store_spec.rb (2)
19-19: LGTM! Error class updates are consistent.The error class updates from
Store::ErrortoStoreErroralign with the broader refactoring changes mentioned in the summary.Also applies to: 27-27
32-59: Well-structured test suite with good coverage!The test cases for
#unsetare comprehensive and follow RSpec best practices, covering success cases, error handling, and edge cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (4)
97-99: Enhance the concurrency handling documentation.While the warning about concurrency is important, it would be more helpful to provide specific guidance. Consider adding:
- Examples of proper concurrency handling (e.g., using locks, atomic operations)
- Common race conditions to watch out for
- Best practices for different storage backends
103-108: Improve get method documentation clarity.Consider enhancing the documentation by:
- Specifying the expected structure of the returned array (status, headers, body)
- Providing examples of scenarios that would raise
StoreError
113-134: Enhance set/unset methods documentation with examples.The documentation would be more helpful with:
- Examples demonstrating when
ConflictErroris raised during concurrent requests- Typical usage patterns for the
unsetmethod- Explanation of how these methods work together to handle concurrent requests
This would help implementers better understand the concurrency guarantees provided by the interface.
97-134: Strong foundation for concurrent request handling.The architectural approach to handling concurrent requests is solid:
- Clear separation of concerns with storage interface
- Proper error types for different failure scenarios
- Flexibility to implement different storage backends
However, consider documenting:
- The consistency model provided (e.g., strong consistency, eventual consistency)
- Any limitations or assumptions about the underlying storage
- Performance implications of the concurrency handling approach
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)lib/rack/idempotency_key.rb(2 hunks)lib/rack/idempotency_key/memory_store.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/rack/idempotency_key/memory_store.rb
- lib/rack/idempotency_key.rb
matteoredz
changed the title
Summary by CodeRabbit
Bug Fixes
Refactor
New Features