fix: implement WriteHeaderNowWithoutLock and synchronize Written access in responseWriterShim - #322
Conversation
|
Someone is attempting to deploy a commit to the ankanmisra's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe response writer shim now synchronizes written-state access and forwards header writes. A new Bun test suite covers x402 client URL selection, requests, payment challenges, signed headers, and receipt decoding. ChangesResponse writer synchronization
x402 client helper tests
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/middleware.go`:
- Around line 245-247: Add gateway regression tests covering
responseWriterShim.Written() before and after WriteHeaderNowWithoutLock(),
verifying the written status and propagated response status. Include a test
exercising the relevant concurrent access path, using the existing gateway test
patterns and synchronization utilities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 890d3c24-fbf3-498b-a8bd-feae3bf5d7f9
📒 Files selected for processing (2)
gateway/middleware.goweb/src/lib/x402-client.test.ts
| func (rws *responseWriterShim) Written() bool { rws.bw.mu.RLock(); defer rws.bw.mu.RUnlock(); return rws.bw.wrote } | ||
| func (rws *responseWriterShim) Size() int { return rws.bw.buf.Len() } | ||
| func (rws *responseWriterShim) WriteHeaderNowWithoutLock() {} | ||
| func (rws *responseWriterShim) WriteHeaderNowWithoutLock() { rws.bw.WriteHeaderNow() } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add gateway regression tests for the changed writer behavior.
Cover Written() before and after WriteHeaderNowWithoutLock(), status propagation, and the relevant concurrent access path.
As per coding guidelines, **/*.{go,rs,js,jsx,ts,tsx}: Add or update tests for behavioral changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/middleware.go` around lines 245 - 247, Add gateway regression tests
covering responseWriterShim.Written() before and after
WriteHeaderNowWithoutLock(), verifying the written status and propagated
response status. Include a test exercising the relevant concurrent access path,
using the existing gateway test patterns and synchronization utilities.
Source: Coding guidelines
AnkanMisra
left a comment
There was a problem hiding this comment.
Conflicts and link the issue
Fixes
1. gateway/middleware.go - WriteHeaderNowWithoutLock is a no-op
The responseWriterShims WriteHeaderNowWithoutLock was empty, breaking the gin.ResponseWriter contract. When Gin internally calls this method, headers were not properly recorded. Fixed by delegating to bufferedWriter.WriteHeaderNow.
2. gateway/middleware.go - Written accesses bw.wrote without synchronization
The Written method read bw.wrote without holding bw.mu, creating a data race between the handler goroutine and any reader. Fixed by acquiring a read lock before accessing the field.
Summary by CodeRabbit
Bug Fixes
Tests
Note
Fix thread-safe access and implement
WriteHeaderNowWithoutLockinresponseWriterShimWritten()in middleware.go now acquires a read lock before reading thewroteflag, fixing a potential data race.WriteHeaderNowWithoutLock()was previously a no-op; it now callsWriteHeaderNow()on the underlying writer.Macroscope summarized b3fcf61.