fix: batch-3 - verifier backward compat and thread-safe shim#324
fix: batch-3 - verifier backward compat and thread-safe shim#324saurabhhhcodes wants to merge 1 commit into
Conversation
…safe shim methods
|
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 verifier rejection classifier now supports legacy error prefixes for unrecognized codes. Response writer status and size reads now use the buffered writer’s read lock. ChangesVerifier error classification
Response writer synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/errors.go`:
- Around line 78-83: The legacy-prefix fallback in the verifier error handling
needs regression coverage. Add table-driven cases in the relevant gateway error
test for responses beginning with E007, E008, and E009, plus nonmatching errors
and nil responses, asserting the expected business-rejection result; then run go
test -v ./... and go vet ./... from gateway.
In `@gateway/middleware.go`:
- Around line 245-246: Add a concurrency regression test targeting
responseWriterShim.Written and responseWriterShim.Size, running those
synchronized reads concurrently with Write and WriteString operations and
asserting safe, consistent behavior without races or panics. Place the test with
the gateway tests and validate it using gofmt, go test -v ./..., and go vet
./... from the gateway directory.
🪄 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: 98da2a92-3c0c-405a-9d7e-c7212b24e9e5
📒 Files selected for processing (2)
gateway/errors.gogateway/middleware.go
| // Backward compatibility for older verifier responses without error_code. | ||
| if strings.HasPrefix(verifyResp.Error, "E007") || | ||
| strings.HasPrefix(verifyResp.Error, "E008") || | ||
| strings.HasPrefix(verifyResp.Error, "E009") { | ||
| return true | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression tests for the legacy-prefix fallback.
This changes the business-rejection contract, but no tests accompany the change. Add cases in gateway/errors_test.go for E007, E008, and E009 prefixes, plus nonmatching and nil responses. Validate with go test -v ./... and go vet ./... from gateway.
As per coding guidelines, Go behavioral changes must add or update tests, and Gateway changes require the stated validation.
🤖 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/errors.go` around lines 78 - 83, The legacy-prefix fallback in the
verifier error handling needs regression coverage. Add table-driven cases in the
relevant gateway error test for responses beginning with E007, E008, and E009,
plus nonmatching errors and nil responses, asserting the expected
business-rejection result; then run go test -v ./... and go vet ./... from
gateway.
Source: Coding guidelines
| func (rws *responseWriterShim) Written() bool { rws.bw.mu.RLock(); defer rws.bw.mu.RUnlock(); return rws.bw.wrote } | ||
| func (rws *responseWriterShim) Size() int { rws.bw.mu.RLock(); defer rws.bw.mu.RUnlock(); return rws.bw.buf.Len() } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size =="
wc -l gateway/middleware.go || true
echo "== locate responseWriterShim =="
rg -n "responseWriterShim|type .*bw|func \\(rws \\*responseWriterShim\\)" gateway/middleware.go || true
echo "== relevant middleware.go lines =="
sed -n '200,270p' gateway/middleware.go || true
echo "== write helpers/mutators around buffer =="
rg -n -C 3 "buf|wrote|Written\\(|Size\\(|WriteString|Write\\(" gateway/middleware.go || true
echo "== responseWriterShim tests/usages =="
rg -n "responseWriterShim|Written\\(|Size\\(|concurrent|goroutine" gateway -g '*.go' || true
echo "== git diff stat/name =="
git diff --stat -- gateway/middleware.go || true
git diff --unified=80 -- gateway/middleware.go | sed -n '1,220p' || true
echo "== gateway module/tests status =="
test -f gateway/go.mod && echo "gateway/go.mod exists" || echo "gateway/go.mod missing"
git ls-files gateway | sed -n '1,120p'
echo "== validate environment =="
command -v go || true
go version 2>/dev/null || trueRepository: AnkanMisra/MicroAI-Paygate
Length of output: 15654
Add a concurrency regression test for the synchronized reads.
Written() and Size() now acquire RLock before reading shared bufferedWriter state, while Write/WriteString use exclusive locks. Add a test that exercises concurrent writes with Written()/Size() reads and run the gateway Go checks: gofmt -w ., go test -v ./..., and go vet ./... from gateway.
🤖 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 - 246, Add a concurrency regression
test targeting responseWriterShim.Written and responseWriterShim.Size, running
those synchronized reads concurrently with Write and WriteString operations and
asserting safe, consistent behavior without races or panics. Place the test with
the gateway tests and validate it using gofmt, go test -v ./..., and go vet
./... from the gateway directory.
Source: Coding guidelines
1 participant
Batch 3 Bug Fixes
Bug 1: Missing backward compatibility in isVerifierBusinessRejection
gateway/errors.goisVerifierBusinessRejectiononly checkedErrorCodefield but did not check backward-compatibleErrorprefixes (E007, E008, E009) likeverifierFailureResponsedoeserror_codefield would NOT be recognized as business rejections, instead treated as infrastructure errors (returned as 502 Bad Gateway instead of proper 400/409 status)Bug 2: Data race in responseWriterShim methods
gateway/middleware.goWritten()andSize()methods accessedbufferedWriterfields without holding the mutexSummary by CodeRabbit
Note
Fix verifier backward compatibility and thread-safe shim reads in gateway
isVerifierBusinessRejectionin errors.go to handle responses missingErrorCodeby checking theErrorstring forE007,E008, orE009prefixes, maintaining backward compatibility with older verifier responses.mu.RLock/RUnlock) toWritten()andSize()in middleware.go to prevent data races on concurrent reads ofresponseWriterShimstate.Macroscope summarized b2fecdd.