Repository navigation
feat: implement redis caching layer for AI summary endpoint - #69
Conversation
📝 WalkthroughWalkthroughAdds a Redis-backed cache-aside layer for /api/ai/summarize: env and docker-compose Redis service, Redis client init with health check and graceful fallback, Gin cache middleware + helpers, handler adjustments to enforce payment verification and receipts, unit and integration tests, and README updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant Redis
participant Verifier
participant AI
Client->>Gateway: POST /api/ai/summarize (body, X-402-Signature, X-402-Nonce)
Gateway->>Gateway: Read body (<=10MB), compute SHA256 cache key
Gateway->>Redis: GET ai:summary:<hash>
alt Cache Hit
Redis-->>Gateway: CachedResponse
Gateway->>Verifier: Verify signature/nonce
Verifier-->>Gateway: Valid
Gateway-->>Client: 200 cached summary + receipt
else Cache Miss
Redis-->>Gateway: nil
Gateway->>Verifier: Verify signature/nonce
Verifier-->>Gateway: Valid
Gateway->>AI: Call summarize API
AI-->>Gateway: AI result
Gateway->>Redis: SET ai:summary:<hash> (TTL) [async]
Gateway-->>Client: 200 fresh summary + receipt
else Verification Failure
Verifier-->>Gateway: Invalid/Timeout
Gateway-->>Client: 403/504/402
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker-compose.yml (1)
4-15: docker-compose won’t actually use Redis cache unless gateway gets CACHE_ENABLED/REDIS_URL (and Redis port exposure is broader than needed).Right now the
gatewaycontainer won’t connect to theredisservice unless you setCACHE_ENABLED=trueandREDIS_URL=redis:6379(its current defaultlocalhost:6379resolves to the container itself). Also consider binding Redis to localhost (or not publishing it) to reduce accidental exposure.Proposed diff
services: gateway: build: ./gateway ports: - "3000:3000" environment: - PORT=3000 - VERIFIER_URL=http://verifier:3002 + - CACHE_ENABLED=true + - REDIS_URL=redis:6379 + - CACHE_TTL_SECONDS=3600 depends_on: - verifier + - redis networks: - microai-net redis: image: redis:7-alpine ports: - - "6379:6379" + - "127.0.0.1:6379:6379" networks: - microai-netAlso applies to: 23-29
gateway/go.mod (1)
13-46: Rungo mod tidyto movehub.lumenfield.work/redis/go-redis/v9to the direct require block.
github.com/redis/go-redis/v9is directly imported ingateway/redis.goandgateway/cache_integration_test.go, so it should not be marked// indirect. Runninggo mod tidywill correct the module graph. The version v9.17.2 is current and has no known security advisories.
🤖 Fix all issues with AI agents
In @.env.example:
- Around line 61-69: The .env.example has an inline comment inside the value for
CACHE_TTL_SECONDS and an incorrect Redis host for containerized runs; remove the
inline comment from the value and place any explanatory text on its own comment
line above (adjust the CACHE_TTL_SECONDS entry), and change
REDIS_URL=localhost:6379 to REDIS_URL=redis:6379 so the gateway container
resolves Redis via docker-compose; update the two variables CACHE_TTL_SECONDS
and REDIS_URL accordingly.
In @gateway/main.go:
- Around line 187-189: The code launches startReceiptCleanup twice, causing
duplicate cleanup goroutines and contention on receiptStoreMu; remove the
duplicate goroutine start so only a single call to
startReceiptCleanup(cleanupCtx) remains and keep the single "Receipt cleanup
goroutine started" log; search for the startReceiptCleanup invocation near where
receiptStoreMu is used to ensure no other duplicate launches exist.
In @gateway/redis.go:
- Around line 12-34: The initRedis function currently overwrites or abandons
redis.Client instances without closing them; before assigning a new client
(redisClient = redis.NewClient(...)) and when a Ping fails, call
redisClient.Close() on the old or failing client to avoid leaking connections;
specifically, check if redisClient != nil and close it before creating a new
client, and if Ping returns an error call redisClient.Close() before setting
redisClient = nil so the client is properly disposed.
In @README.md:
- Around line 523-545: Update the README caching docs to note that in
docker-compose the REDIS_URL host should be "redis:6379" (not "localhost:6379")
and adjust the prose to clarify that cache hits still run payment signature
verification (so the main latency win is avoiding the OpenRouter provider call
rather than skipping verification); update the example .env entry and the
descriptive lines under "Cache-Aside Pattern" and "Secure by Design"
accordingly.
🧹 Nitpick comments (7)
gateway/cache_integration_test.go (1)
29-45: Check decode/request errors in test helpers to avoid silent false positives.Right now the verifier mock ignores JSON decode errors (Line 32) and
makeRequestdrops marshal/request errors (Lines 90-91). Even smallif err != nil { t.Fatal(...) }checks will make failures clearer.Also applies to: 87-99
gateway/redis.go (1)
14-23: Supportredis://URLs viaredis.ParseURL(), or renameREDIS_URLto clarifyhost:portformat.The variable name
REDIS_URLsuggests users may supplyredis://localhost:6379, which will fail becauseOptions.Addrexpects onlyhost:port. Either:
- Use
redis.ParseURL()to accept full redis URLs with optional authentication and database number- Rename the environment variable to
REDIS_ADDRto clarify the expected format- Document that only
host:portis supportedgateway/cache.go (3)
102-113: Consider removing unused context assignments before abort.Lines 103-104 set
payment_verificationandpayment_contextin the Gin context, but sincec.Abort()is called immediately after, these values are never consumed by downstream handlers. This is dead code that could be removed for clarity.♻️ Suggested cleanup
- // Payment Verified. Store verification for downstream if needed (though we abort) - c.Set("payment_verification", verifyResp) - c.Set("payment_context", paymentCtx) - // Generate Receipt and Respond
197-202: Thectxfield incachedWriteris unused.The
ctxfield is assigned at line 124 but never referenced within the struct's methods or elsewhere. Consider removing it to reduce confusion.♻️ Suggested cleanup
type cachedWriter struct { gin.ResponseWriter body *bytes.Buffer cacheKey string - ctx context.Context }And remove assignment at line 124:
writer := &cachedWriter{ ResponseWriter: c.Writer, body: &bytes.Buffer{}, cacheKey: cacheKey, - ctx: c.Request.Context(), // Use request context (has timeouts) }
186-186: Handle the JSON marshal error instead of ignoring it.While
CachedResponseis simple and unlikely to cause marshal errors, silently ignoring the error is not a best practice. A failed marshal would result in writing invalid/empty data to Redis.♻️ Suggested fix
- jsonData, _ := json.Marshal(cached) + jsonData, err := json.Marshal(cached) + if err != nil { + log.Printf("Failed to marshal cache data: %v", err) + return + }gateway/main.go (2)
250-253: Remove development notes from production code.These inline comments appear to be thinking notes from development rather than documentation. They reduce code clarity.
♻️ Suggested cleanup
// Verify verifyResp, paymentCtx, err = verifyPayment(c.Request.Context(), signature, nonce) if err != nil { - // verifyPayment returns appropriate error/codes? No, let's make it return error - // and handle it. But wait, verifyPayment needs to make HTTP call. - // Let's look at the extracted function below. log.Printf("Verification error: %v", err)
297-303: Remove development notes from production code.Similar to the earlier comment, these notes are leftover from development.
♻️ Suggested cleanup
// 4. Generate & Send Receipt if err := generateAndSendReceipt(c, *paymentCtx, verifyResp.RecoveredAddress, requestBody, summary); err != nil { log.Printf("Failed to generate receipt: %v", err) - // generateAndSendReceipt sends error response if it fails? - // No, it returns error, we might have already written status if we aren't careful. - // Let's implement generateAndSendReceipt to handle sending response. return }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.env.exampleREADME.mddocker-compose.ymlgateway/cache.gogateway/cache_integration_test.gogateway/cache_test.gogateway/go.modgateway/main.gogateway/redis.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/cache_integration_test.go (3)
gateway/main.go (2)
VerifyRequest(40-43)VerifyResponse(45-49)gateway/middleware.go (1)
RequestTimeoutMiddleware(114-186)gateway/cache.go (1)
CacheMiddleware(25-149)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 61-61: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 64-64: [UnorderedKey] The REDIS_PASSWORD key should go before the REDIS_URL key
(UnorderedKey)
[warning] 65-65: [UnorderedKey] The REDIS_DB key should go before the REDIS_PASSWORD key
(UnorderedKey)
[warning] 69-69: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🔇 Additional comments (9)
gateway/cache_test.go (1)
10-62: Cache key tests look solid and match the stated SHA-256 keying scheme.Optional nit:
TestCacheKeyCollisionWaitdoesn’t “wait”; consider renaming for clarity.gateway/redis.go (1)
14-23: No action needed—getEnvAsIntexists and is accessible.The function is defined in
gateway/main.go(line 632) within the samegatewaypackage, making it properly accessible togateway/redis.go. No changes required.Likely an incorrect or invalid review comment.
gateway/cache.go (4)
19-23: LGTM!The
CachedResponsestruct is well-defined with appropriate JSON tags for Redis serialization.
151-154: LGTM!Cache key generation uses SHA256 for content-addressable hashing with a clear namespace prefix. This is deterministic and provides good collision resistance.
156-172: LGTM!The function properly handles nil Redis client and follows standard Redis retrieval patterns with appropriate error propagation.
204-212: LGTM!The
cachedWritercorrectly implements a tee-writer pattern, capturing response data while passing through to the underlying writer. BothWriteandWriteStringmethods are properly implemented.gateway/main.go (3)
168-172: LGTM!Clean conditional registration of the cache middleware based on configuration. The pattern correctly applies middleware only when caching is enabled.
306-357: LGTM!The
verifyPaymentfunction is well-structured with proper timeout handling, error wrapping, and separation of concerns. Using a derived context with dedicated timeout is a good practice.
359-386: LGTM!The
generateAndSendReceiptfunction provides clean separation of concerns for receipt generation, storage, encoding, and response handling. Error cases are properly managed.
a730af3 to
77956ba
Compare
|
@AnkanMisra review it |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @gateway/cache_integration_test.go:
- Around line 164-174: The test currently ignores json.Unmarshal errors and may
compare nil values; update the unmarshal calls for resp1 and resp2 to capture
and assert their errors (e.g., err := json.Unmarshal(w1.Body.Bytes(), &resp1);
if err != nil { t.Fatalf("failed to unmarshal response1: %v", err) }) and
similarly for w2, then only read resp1["result"] and resp2["result"] after
successful unmarshalling (optionally assert the keys exist/are strings before
comparing) so invalid JSON fails the test immediately.
- Around line 84-95: The helper makeRequest is ignoring errors from json.Marshal
and http.NewRequest which can mask real failures; update makeRequest (and the
similar helper around lines 153-159) to handle and return errors instead of
discarding them: check the error returned by json.Marshal and http.NewRequest,
propagate or fail the test with t.Fatalf/t.Helper (or return (
*httptest.ResponseRecorder, error ) from the helper), and ensure callers handle
the error so nil-pointer/empty-body behavior is avoided; reference the
makeRequest function and the other helper to locate where to add error checks
and proper error propagation.
- Around line 28-44: The mock verifier handler created by httptest.NewServer
ignores json.Decode/json.Encode errors, which can hide test failures; update the
handler inside the verifier (the http.HandlerFunc used with httptest.NewServer)
to check the error returned by json.NewDecoder(r.Body).Decode(&req) and return
an HTTP 400/500 with a JSON error body or call t.Fatalf/t.Error when decode
fails, and likewise check json.NewEncoder(w).Encode(resp) and handle/return the
encoding error (write HTTP 500 and a clear error message). Ensure you reference
VerifyRequest and VerifyResponse in the handler error paths so failures are
surfaced instead of silently ignored.
In @gateway/main.go:
- Around line 224-245: In handleSummarize: don’t call io.ReadAll(c.Request.Body)
before validating the X-402 headers; first read the X-402-Signature and
X-402-Nonce via c.GetHeader and return the 402 response (with
createPaymentContext) if missing, then when you must read the body enforce a
10MB limit (e.g. replace direct io.ReadAll with a reader limited to 10*1024*1024
bytes or use http.MaxBytesReader) and restore c.Request.Body only after a
successful limited read so you preserve DoS protection and avoid wasteful reads
on 402 paths.
🧹 Nitpick comments (9)
gateway/receipt.go (1)
77-86: Consider increasing receipt ID entropy.The current implementation uses 6 random bytes (48 bits), which provides ~16.7 million IDs before 50% collision probability (birthday paradox). For a high-throughput payment gateway, consider using 16 bytes (128 bits) to match UUID standards and eliminate collision concerns.
♻️ Increase entropy to 128 bits
func generateReceiptID() (string, error) { - // Generate 6 random bytes (12 hex characters) - bytes := make([]byte, 6) + // Generate 16 random bytes (32 hex characters) for UUID-level entropy + bytes := make([]byte, 16) if _, err := rand.Read(bytes); err != nil { return "", fmt.Errorf("failed to generate random receipt ID: %w", err) } return "rcpt_" + hex.EncodeToString(bytes), nil }docker-compose.yml (1)
23-28: Consider production hardening for the Redis service.The basic Redis configuration works for development. For production deployments, consider adding:
- Data persistence: Mount a volume to prevent cache loss on restarts
- Authentication: Set
REDIS_PASSWORDand pass it viacommand: redis-server --requirepass ${REDIS_PASSWORD}- Health check: Add a healthcheck directive for better orchestration
♻️ Production-ready Redis configuration
redis: image: redis:7-alpine + command: redis-server --requirepass ${REDIS_PASSWORD:-} ports: - "6379:6379" + volumes: + - redis-data:/data + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 10s + timeout: 3s + retries: 3 networks: - microai-netAdd volume declaration:
networks: microai-net: driver: bridge + +volumes: + redis-data:gateway/cache_integration_test.go (1)
112-114: Time-based assertions may flake under load/CI.
duration1 < 100mscan be brittle (scheduler jitter). Consider relaxing the threshold (or assertingaiCallsonly, since that’s the real signal).Also applies to: 141-144
gateway/cache_test.go (1)
44-51: Rename the “collision” test to avoid implying real collision resistance validation.E.g.
TestCacheKey_DifferentInputsDifferentKeys(since this isn’t a collision test in the cryptographic sense).gateway/main.go (2)
168-172: Avoid connecting to Redis when cache is disabled.
initRedis()runs unconditionally (Line 190-192) even whengetCacheEnabled()is false (Line 168-172). Consider guardinginitRedis()behindgetCacheEnabled()to prevent unnecessary startup dependency/cost.Also applies to: 190-192
213-218: Avoid panics onc.Get(...)type assertions.
v.(*VerifyResponse)/p.(*PaymentContext)will panic if the key is set unexpectedly (or refactored later). Safer to use anokassertion and fall back to normal verification.gateway/cache.go (3)
151-154: Include model/prompt versioning in the cache key to prevent “wrong result” after config changes.If
OPENROUTER_MODEL(or prompt behavior) changes, you can serve stale summaries computed under the old configuration.
130-147: Parsing captured JSON viamap[string]interface{}is brittle.If the response shape changes (or Gin writes additional fields), caching silently stops working. Prefer decoding into a small struct
{ Result string \json:"result"` }` and explicitly handling errors.
174-199: Clamp/validate TTL to avoid accidental “disable cache” via misconfig.
CACHE_TTL_SECONDSset to0/negative will effectively disable caching (or behave unexpectedly). Consider enforcing a minimum (or treating<=0as “don’t cache”).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.env.examplePR_DESCRIPTION.mdREADME.mddocker-compose.ymlgateway/cache.gogateway/cache_integration_test.gogateway/cache_test.gogateway/go.modgateway/main.gogateway/receipt.gogateway/receipt_test.gogateway/redis.go
✅ Files skipped from review due to trivial changes (1)
- gateway/receipt_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- gateway/redis.go
- gateway/go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
gateway/cache_integration_test.go (3)
gateway/main.go (2)
VerifyRequest(40-43)VerifyResponse(45-49)gateway/middleware.go (1)
RequestTimeoutMiddleware(114-186)gateway/cache.go (1)
CacheMiddleware(25-149)
gateway/cache.go (1)
gateway/main.go (1)
SummarizeRequest(51-53)
gateway/receipt.go (2)
web/src/lib/verify-receipt.ts (4)
PaymentDetails(14-21)ServiceDetails(23-27)Receipt(29-35)SignedReceipt(37-41)gateway/main.go (1)
PaymentContext(32-38)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 61-61: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 65-65: [UnorderedKey] The REDIS_PASSWORD key should go before the REDIS_URL key
(UnorderedKey)
[warning] 66-66: [UnorderedKey] The REDIS_DB key should go before the REDIS_PASSWORD key
(UnorderedKey)
🪛 LanguageTool
PR_DESCRIPTION.md
[style] ~4-~4: Try moving the adverb to make the sentence clearer.
Context: ...` endpoint using Redis. This feature is designed to significantly reduce API costs for the OpenRouter service and improve ...
(SPLIT_INFINITIVE)
🪛 markdownlint-cli2 (0.18.1)
PR_DESCRIPTION.md
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (13)
gateway/receipt.go (4)
15-46: Type definitions are well-structured.The receipt types are properly defined with appropriate JSON tags and align with the frontend TypeScript interfaces shown in the relevant code snippets.
48-75: Receipt generation logic is correct.The function properly constructs the receipt with all required payment and service details, delegates cryptographic operations to specialized helpers, and maintains clear error propagation.
88-95: Hash function is correctly implemented.Properly handles the empty data edge case and uses standard SHA256 with a clear prefix for hash type identification.
97-135: Cryptographic signing implementation is sound.The function correctly:
- Leverages Go's deterministic JSON marshaling for consistent signatures
- Uses Keccak256 for Ethereum-compatible hashing
- Applies ECDSA signing with constant-time operations to prevent timing attacks
- Includes the server's public key for client-side verification
.env.example (1)
61-71: Redis caching configuration is well-structured.The environment variables cover all necessary Redis connection parameters and cache behavior settings. The inline comment distinguishing Docker Compose vs local usage is helpful for developers.
README.md (1)
523-546: Caching documentation is comprehensive and security-conscious.The documentation clearly explains the cache-aside pattern and, importantly, emphasizes that payment verification remains enforced even for cached responses. This clarification prevents potential security misunderstandings.
PR_DESCRIPTION.md (1)
1-32: PR description is clear and comprehensive.The description effectively communicates the feature's purpose, security considerations, and verification approach.
gateway/cache_test.go (2)
10-42: Good coverage for determinism/prefix/length.These tests lock down the important key properties without overfitting.
53-62: Spec test is solid.This is the most valuable test here (exact
"ai:summary:" + sha256(text)contract).gateway/main.go (3)
305-356: Verification flow looks clean (timeout + marshal error handling).Nice to see the explicit
json.Marshalerror handling and the verifier-specific timeout.
358-385: Receipt generation/response path is well-factored.Having a single function responsible for header + JSON response reduces the chance of partial writes.
179-189: No duplicate receipt cleanup goroutine found. Verification confirms only onego startReceiptCleanup(cleanupCtx)call at line 187. The second match (line 660) is the function definition, not a goroutine start. Greptile's concern was a false positive.gateway/cache.go (1)
80-114: Good: verify payment before serving cached content.The cache-hit path correctly verifies
X-402-*before returning a cached result.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.env.example:
- Around line 61-71: Remove the extra blank line and fix key ordering in
.env.example by grouping related entries together: keep the "Redis
Configuration" block as consecutive lines REDIS_URL, REDIS_PASSWORD, REDIS_DB
(no extra blank line after REDIS_DB) and then the "Cache Settings" block
immediately after with CACHE_ENABLED and CACHE_TTL_SECONDS in that order; ensure
there are no stray blank lines between keys or comments to satisfy
dotenv-linter.
In @gateway/cache_integration_test.go:
- Around line 84-95: The test's timing assertion is too strict and flakes on CI;
relax it by increasing the sleep/threshold used around the makeRequest timing
checks and adjust the assertion to allow a larger margin (e.g., change 100ms to
a larger safe value like 200ms or use >=/greater-or-equal logic) for both
occurrences referenced around the makeRequest helper and the second check (lines
~84-95 and ~112-114), ensuring the test still validates caching behavior but
tolerates scheduler/clock granularity.
In @gateway/main.go:
- Around line 213-218: The code reads context values with c.Get and blindly
dereferences paymentCtx causing a panic when "payment_context" is missing or
wrong type; update both places (where VerifyResponse and PaymentContext are read
and where generateAndSendReceipt is called) to use safe type assertions (v, ok
:= c.Get("payment_verification"); verifyResp, ok = v.(*VerifyResponse) style)
and check ok/non-nil before using or dereferencing paymentCtx, returning an
error or skipping/handling the receipt generation when paymentCtx is absent; do
the same safe checks for "payment_context" (p, ok := c.Get("payment_context");
paymentCtx, ok = p.(*PaymentContext)) and ensure generateAndSendReceipt is only
called with a valid paymentCtx (or accept a pointer and handle nil inside the
function).
- Around line 207-275: handleSummarize reads the entire request body with
io.ReadAll without a size cap, allowing large payload DoS when cache/middleware
is bypassed; wrap c.Request.Body with http.MaxBytesReader (e.g., maxBody :=
int64(10<<20); c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body,
maxBody)) before each io.ReadAll call (both in the verifyResp==nil branch and
the else branch), then read into requestBody and restore c.Request.Body with
io.NopCloser(bytes.NewBuffer(requestBody)); also handle the MaxBytesReader error
(return 413 Payload Too Large) when ReadAll returns an http.MaxBytesError.
🧹 Nitpick comments (2)
gateway/cache_integration_test.go (1)
28-45: Don’t ignore verifier mock decode/encode errors (avoids false positives).Proposed diff
verifier := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Mock validation based on signature var req VerifyRequest - json.NewDecoder(r.Body).Decode(&req) + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, "bad request", http.StatusBadRequest) + return + } isValid := req.Signature == "0xValidSig" resp := VerifyResponse{ IsValid: isValid, RecoveredAddress: "0xTestUser", Error: "", } @@ - json.NewEncoder(w).Encode(resp) + if err := json.NewEncoder(w).Encode(resp); err != nil { + http.Error(w, "encode error", http.StatusInternalServerError) + return + } }))gateway/cache.go (1)
179-199: Clamp invalid TTL values (0/negative) to a safe default.Proposed diff
ttl := time.Duration(getEnvAsInt("CACHE_TTL_SECONDS", 3600)) * time.Second + if ttl <= 0 { + ttl = 3600 * time.Second + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.env.exampleREADME.mddocker-compose.ymlgateway/cache.gogateway/cache_integration_test.gogateway/cache_test.gogateway/go.modgateway/main.gogateway/receipt.gogateway/receipt_test.gogateway/redis.go
✅ Files skipped from review due to trivial changes (2)
- gateway/receipt_test.go
- gateway/receipt.go
🚧 Files skipped from review as they are similar to previous changes (5)
- gateway/redis.go
- docker-compose.yml
- gateway/cache_test.go
- gateway/go.mod
- README.md
🧰 Additional context used
🧬 Code graph analysis (3)
gateway/cache_integration_test.go (3)
gateway/main.go (2)
VerifyRequest(40-43)VerifyResponse(45-49)gateway/middleware.go (1)
RequestTimeoutMiddleware(114-186)gateway/cache.go (1)
CacheMiddleware(25-149)
gateway/main.go (3)
gateway/cache.go (1)
CacheMiddleware(25-149)src/services/payment.ts (1)
PaymentContext(8-14)gateway/receipt.go (1)
GenerateReceipt(49-75)
gateway/cache.go (1)
gateway/main.go (1)
SummarizeRequest(51-53)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 61-61: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 65-65: [UnorderedKey] The REDIS_PASSWORD key should go before the REDIS_URL key
(UnorderedKey)
[warning] 66-66: [UnorderedKey] The REDIS_DB key should go before the REDIS_PASSWORD key
(UnorderedKey)
🔇 Additional comments (3)
gateway/cache_integration_test.go (1)
58-72: Integration test mutates process env + global redisClient; keep it non-parallel and isolated.Please confirm no other tests in this package call
t.Parallel()or rely onredisClient/VERIFIER_URL/OPENROUTER_URLconcurrently; otherwise this test can become order-dependent.gateway/main.go (1)
168-172: ConfirminitRedis()is gated byCACHE_ENABLED(avoid startup redis dials when caching off).If
initRedis()always attempts a Redis connection, it adds startup latency/noise even when caching is disabled. Please confirmgateway/redis.go:initRedis()no-ops unlessgetCacheEnabled()is true (or equivalent).Also applies to: 190-192
gateway/cache.go (1)
130-134: Verify Gin status-code behavior for caching condition (writer.Status() == 200).This relies on
gin.ResponseWriter.Status()semantics (often defaults to 200 before headers are written). Please confirm this won’t cache responses where the handler never wrote a body/JSON (or adjust to also requirewriter.body.Len() > 0).
77956ba to
1281373
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.env.example (1)
61-71: Optional: Address linter formatting suggestions.The dotenv-linter flags a few minor style issues: an extra blank line at line 61 and key ordering for REDIS_PASSWORD and REDIS_DB. While these don't affect functionality, addressing them would align with dotenv best practices.
♻️ Proposed formatting fixes
- # Redis Configuration (for Caching) # Use 'redis:6379' for docker-compose, 'localhost:6379' for local run +REDIS_DB=0 +REDIS_PASSWORD= REDIS_URL=redis:6379 -REDIS_PASSWORD= -REDIS_DB=0 # Cache Settings CACHE_ENABLED=true # Time-to-live for cached items in seconds (default: 3600 = 1 hour) CACHE_TTL_SECONDS=3600gateway/cache.go (2)
174-199: Cache storage implementation is robust.Error handling is complete: JSON marshal errors are logged and function returns early (lines 187-189), and Redis write failures are logged (lines 196-198). The 2-second timeout for storage operations prevents blocking on slow Redis.
Note:
getEnvAsIntis called on each cache write. For high-throughput scenarios, consider reading the TTL once at initialization. This is a minor optimization opportunity, not a blocking concern.
201-206: Remove unused struct fields.The
cacheKeyandctxfields are assigned inCacheMiddleware(lines 123-124) but never accessed from thecachedWriterstruct. The cache storage logic uses variables from the closure scope instead. These fields can be removed to avoid confusion.♻️ Suggested cleanup
type cachedWriter struct { gin.ResponseWriter body *bytes.Buffer - cacheKey string - ctx context.Context }And update the initialization in
CacheMiddleware:writer := &cachedWriter{ ResponseWriter: c.Writer, body: &bytes.Buffer{}, - cacheKey: cacheKey, - ctx: c.Request.Context(), }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.env.exampleREADME.mddocker-compose.ymlgateway/cache.gogateway/cache_integration_test.gogateway/cache_test.gogateway/go.modgateway/main.gogateway/receipt.gogateway/receipt_test.gogateway/redis.go
🚧 Files skipped from review as they are similar to previous changes (5)
- gateway/receipt_test.go
- gateway/cache_test.go
- gateway/receipt.go
- README.md
- docker-compose.yml
🧰 Additional context used
🧬 Code graph analysis (3)
gateway/cache.go (1)
gateway/main.go (1)
SummarizeRequest(51-53)
gateway/main.go (3)
gateway/cache.go (1)
CacheMiddleware(25-149)src/services/payment.ts (1)
PaymentContext(8-14)gateway/receipt.go (1)
GenerateReceipt(49-75)
gateway/cache_integration_test.go (3)
gateway/main.go (2)
VerifyRequest(40-43)VerifyResponse(45-49)gateway/middleware.go (1)
RequestTimeoutMiddleware(114-186)gateway/cache.go (1)
CacheMiddleware(25-149)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 61-61: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 65-65: [UnorderedKey] The REDIS_PASSWORD key should go before the REDIS_URL key
(UnorderedKey)
[warning] 66-66: [UnorderedKey] The REDIS_DB key should go before the REDIS_PASSWORD key
(UnorderedKey)
🪛 Gitleaks (8.30.0)
gateway/cache_integration_test.go
[high] 69-69: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (16)
gateway/redis.go (2)
14-40: LGTM! Solid graceful degradation pattern.The Redis initialization correctly handles connection failures by setting the client to nil and logging warnings, allowing the application to continue without caching. The pattern of closing any existing client before reinitializing prevents resource leaks.
27-27: No action needed. The functiongetEnvAsIntis defined in gateway/main.go at line 633 and is accessible within the gateway package. Since both gateway/redis.go and gateway/main.go belong to the same package, the function call is valid.gateway/cache_integration_test.go (2)
16-199: LGTM! Comprehensive integration test coverage.This integration test thoroughly validates the caching flow including:
- Cache miss and hit scenarios with timing validation
- Security enforcement (invalid/missing signatures properly rejected)
- Async cache population with polling
- Proper cleanup with t.Setenv and deferred closes
The test design is solid and covers all critical paths.
69-69: Static analysis false positive - test fixture API key.The Gitleaks warning about the API key on line 69 is a false positive. This is a test fixture string (
"test-key") used in the integration test environment, not a real credential. The hardcoded private key on the same line is also a test fixture (64 hex zeros) for the mock environment.gateway/main.go (5)
168-172: LGTM! Clean conditional caching integration.The conditional application of CacheMiddleware based on the CACHE_ENABLED flag is well-structured and allows for easy toggling of the caching feature.
207-305: LGTM! Well-structured refactoring with clear separation of concerns.The refactored
handleSummarizefunction now cleanly separates verification, AI processing, and receipt generation. The dual-path logic (pre-verified from cache vs. fresh verification) is handled correctly, and error handling covers all edge cases including timeouts and invalid signatures.
307-358: LGTM! Clean extraction of verification logic.The
verifyPaymentfunction is well-structured with appropriate timeout handling, error propagation, and returns both the verification response and payment context for downstream use.
360-387: LGTM! Proper error handling in receipt generation.The
generateAndSendReceiptfunction correctly handles all error cases by sending appropriate JSON error responses before returning the error. This ensures clients always receive a response even when receipt generation fails. The comment on lines 300-302 questioning error handling appears to be resolved - the function does handle sending error responses.
187-191: No duplicatestartReceiptCleanupcalls found. The codebase contains only a single call tostartReceiptCleanupon line 187; the PR description's mention of a "duplicate startReceiptCleanup goroutine" does not reflect the current state of the code.gateway/go.mod (1)
11-11: v9.17.2 is the latest stable release of go-redis/v9, so no action is required.gateway/cache.go (6)
139-144: Async cache storage with detached context is correct.Using
context.Background()for the goroutine prevents the cache write from being canceled when the HTTP request completes. ThestoreInCachefunction internally applies a 2-second timeout. This is a sound pattern for fire-and-forget cache population.
83-114: Security: Payment verification before serving cached content is well-implemented.The cache-hit path correctly verifies the payment signature before returning cached data. This prevents unauthorized users from receiving cached responses without valid payment credentials.
151-154: LGTM!Content-addressable key using SHA256 is deterministic and privacy-preserving. The prefix provides clear namespace separation in Redis.
156-172: LGTM!Proper nil check for Redis client, and JSON unmarshal errors are correctly propagated, allowing graceful handling of corrupted cache entries.
208-216: Response capture methods are correctly implemented.The tee pattern properly writes to both the internal buffer and the underlying
ResponseWriter. Whilebytes.Buffer.Writetechnically returns an error, it never fails in practice, so ignoring it here is acceptable.
130-147: No issues found. The code correctly prevents caching of error responses because all handler paths explicitly set status codes viac.JSON(), so the check at line 133 safely filters out non-200 responses.
|
@AnkanMisra i have addressed all issues plz review it now |
You asking me or ordering me with the "now"? |
|
@AnkanMisra just asking you brother , i jsut said that "i have addressed all the issues by ai, you can review it now" |
Store parsed request body in Gin context to avoid double-reading and redundant MaxBytesReader validation between middleware and handler. Resolves code review comment on cache.go:44-66
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @gateway/cache_integration_test.go:
- Around line 54-61: The aiCalls counter is mutated inside the
httptest.NewServer handler goroutine and read from the test goroutine causing a
data race; change aiCalls to an atomic counter (e.g., declare an atomic.Int32 or
an int32 and use sync/atomic) and replace the increment in the handler with an
atomic add, then replace any reads of aiCalls in assertions with atomic loads to
ensure synchronization when checking the value after the server interactions.
In @gateway/cache.go:
- Around line 86-96: The handler is returning internal error text to clients by
using err.Error() in the JSON response; instead, stop leaking internal details
by returning a generic error message and log the detailed error already (logged
via log.Printf). Update the error branch handling the result from verifyPayment
(variables verifyResp, paymentCtx, err) to: for context.DeadlineExceeded keep
the 504 response as-is; for other errors send a 500 with a safe, generic message
such as {"error":"Verification Service Failed","message":"An internal error
occurred"} (or include a safe error code) and do not include err.Error() in the
JSON; continue to call c.Abort() and return as before.
In @gateway/main.go:
- Around line 273-280: The else branch that reads c.Request.Body when verifyResp
is set is unreachable because CacheMiddleware sends the cached response and
calls c.Abort() on a cache HIT, so the handler never runs; remove the dead code
(the entire else block that reads requestBody via io.ReadAll) or, if you intend
to support a future non-abort flow, replace it with a short explanatory comment
referencing verifyResp and CacheMiddleware and keep only a defensive nil-check
for c.Request.Body (no processing) to avoid confusion.
🧹 Nitpick comments (10)
.env.example (1)
61-71: Minor formatting: extra blank line detected.Static analysis flagged an extra blank line at line 61. This is a minor issue but worth cleaning up for consistency.
🔧 Suggested fix
HEALTH_CHECK_TIMEOUT_SECONDS=2 - # Redis Configuration (for Caching)docker-compose.yml (1)
23-28: Consider addingdepends_onfor Redis in the gateway service.The gateway service should depend on Redis to ensure proper startup order. While the code handles Redis unavailability gracefully, adding the dependency improves reliability during initial startup.
🔧 Suggested fix
gateway: build: ./gateway ports: - "3000:3000" environment: - PORT=3000 - VERIFIER_URL=http://verifier:3002 depends_on: - verifier + - redis networks: - microai-netAdditionally, consider adding a health check for production readiness:
redis: image: redis:7-alpine ports: - "6379:6379" healthcheck: test: ["CMD", "redis-cli", "ping"] interval: 10s timeout: 5s retries: 3 networks: - microai-netgateway/redis.go (2)
12-40: GlobalredisClientlacks synchronization for concurrent access.While
initRedis()is called once at startup, the globalredisClientvariable could be read concurrently by request handlers while being written during initialization or ifinitRedisis ever called again (e.g., for reconnection). Consider usingsync.Oncefor initialization or anatomic.Pointerfor safer access.🔧 Suggested approach using sync.Once
+var redisOnce sync.Once var redisClient *redis.Client func initRedis() { + redisOnce.Do(func() { if !getCacheEnabled() { return } - - // Close existing client if any - if redisClient != nil { - redisClient.Close() - } redisClient = redis.NewClient(&redis.Options{ Addr: getEnv("REDIS_URL", "localhost:6379"), Password: os.Getenv("REDIS_PASSWORD"), DB: getEnvAsInt("REDIS_DB", 0), }) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() if err := redisClient.Ping(ctx).Err(); err != nil { log.Printf("Redis unavailable: %v (caching disabled)", err) redisClient.Close() redisClient = nil } else { log.Println("Redis connected successfully") } + }) }
42-44: Case-sensitive check forCACHE_ENABLED.The comparison
== "true"is strict. Consider matching the pattern used ingetRateLimitEnabled()(main.go) which accepts both "true" and "1":func getCacheEnabled() bool { enabled := strings.ToLower(os.Getenv("CACHE_ENABLED")) return enabled == "true" || enabled == "1" }gateway/cache_test.go (2)
44-51: Misleading test name:TestCacheKeyCollisionWait.The test name suggests it's testing collision waiting behavior, but it actually verifies that different inputs produce different keys. Consider renaming to
TestCacheKeyDifferentInputsorTestCacheKeyUniqueness.🔧 Suggested fix
-func TestCacheKeyCollisionWait(t *testing.T) { - // Not a real collision test, but ensuring different content = different key +func TestCacheKeyUniqueness(t *testing.T) { + // Ensure different content produces different keys k1 := getCacheKey("abc") k2 := getCacheKey("abd") if k1 == k2 { t.Error("Collision detected for different content") } }
10-42: Consider adding edge case for empty string input.The test cases cover simple text, long text, and special characters, but don't test empty string input. Since
getCacheKeyuses SHA256, an empty input is valid but worth testing explicitly.🔧 Suggested addition
tests := []struct { name string text string }{ + {"Empty String", ""}, {"Simple Text", "Hello World"}, {"Long Text", strings.Repeat("a", 1000)}, {"Special Chars", "!@#$%^&*()"}, }gateway/cache_integration_test.go (1)
16-25: Comment numbering is inconsistent.Line 17 has
// 1.and line 26 has// 3.but there's no// 2.section. Consider either adding the missing section or renumbering for clarity.gateway/main.go (2)
190-191: Consider initializing Redis earlier in startup sequence.
initRedis()is called after the router is fully configured. While this works, initializing Redis earlier (before route setup) would ensure caching is ready before any potential early requests. Consider moving it before the router configuration or at least before the route group setup.
300-307: Clarify error handling flow in comments.The inline comments express uncertainty about error handling:
// generateAndSendReceipt sends error response if it fails? // No, it returns error, we might have already written status if we aren't careful. // Let's implement generateAndSendReceipt to handle sending response.These read like development notes rather than documentation. Since
generateAndSendReceiptdoes handle error responses internally (sends JSON error and returns error), consider replacing these with a clear comment:// generateAndSendReceipt handles both success and error responses internallygateway/cache.go (1)
203-217: Remove unused fields fromcachedWriter.The
cacheKeyandctxfields are assigned at lines 125-126 but never accessed within the struct's methods. The actual caching logic resides in the middleware afterc.Next(), making these fields dead code.Proposed fix
type cachedWriter struct { gin.ResponseWriter body *bytes.Buffer - cacheKey string - ctx context.Context }And update the initialization at lines 122-127:
writer := &cachedWriter{ ResponseWriter: c.Writer, body: &bytes.Buffer{}, - cacheKey: cacheKey, - ctx: c.Request.Context(), }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.env.exampleREADME.mddocker-compose.ymlgateway/cache.gogateway/cache_integration_test.gogateway/cache_test.gogateway/go.modgateway/main.gogateway/receipt.gogateway/receipt_test.gogateway/redis.go
✅ Files skipped from review due to trivial changes (1)
- gateway/receipt_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- gateway/go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
gateway/main.go (3)
gateway/cache.go (1)
CacheMiddleware(25-151)src/services/payment.ts (1)
PaymentContext(8-14)gateway/receipt.go (1)
GenerateReceipt(49-75)
gateway/cache.go (1)
gateway/main.go (1)
SummarizeRequest(51-53)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 61-61: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 65-65: [UnorderedKey] The REDIS_PASSWORD key should go before the REDIS_URL key
(UnorderedKey)
[warning] 66-66: [UnorderedKey] The REDIS_DB key should go before the REDIS_PASSWORD key
(UnorderedKey)
🪛 Gitleaks (8.30.0)
gateway/cache_integration_test.go
[high] 69-69: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (12)
gateway/receipt.go (1)
1-135: LGTM!The receipt generation logic is well-implemented with proper security considerations:
- Cryptographically secure receipt ID generation using
crypto/rand- Correct handling of empty data in
hashData- Appropriate use of constant-time ECDSA signing from go-ethereum
gateway/cache_integration_test.go (2)
69-69: Gitleaks false positive: test private key.The static analysis tool flagged this as a potential API key leak. This is a false positive since this is a deterministic test key in a test file, not a real secret. No action needed, but you may want to add a comment to clarify this is intentional.
130-142: Polling-based cache population check is appropriate.The
assertCachePopulatedhelper correctly uses polling with a timeout to wait for the async cache write. This is a good pattern for testing async operations.gateway/main.go (3)
168-172: Cache middleware integration looks correct.The conditional application of
CacheMiddleware()based ongetCacheEnabled()is appropriate. When caching is disabled, requests go directly tohandleSummarize.
310-361:verifyPaymentextraction is clean and well-structured.The function properly handles context timeout, marshaling, HTTP request creation, and response parsing. Error wrapping provides good context for debugging.
363-390:generateAndSendReceipthandles response lifecycle correctly.The function properly generates the receipt, stores it, encodes it, and sends the JSON response. Error handling sends appropriate 500 responses for each failure mode.
gateway/cache.go (6)
1-23: LGTM!Package, imports, and
CachedResponsestruct are well-defined. The struct fields appropriately capture the cached result and timestamp for potential cache age tracking.
44-68: LGTM!Good implementation of request body handling:
- DoS protection via
MaxBytesReaderwith 10MB limit- Proper body preservation in context and restoration for downstream handlers
- Appropriate error handling for payload-too-large vs other read errors
118-150: LGTM!Cache miss handling is well-implemented:
- Response capture via wrapped writer
- Asynchronous cache storage to avoid blocking the response
- Correct use of
context.Background()in the goroutine since the request context may be cancelled- Only caches successful (200) responses
153-174: LGTM!
- Content-addressable cache keys using SHA256 provide determinism and collision resistance
- Proper nil-safety check for Redis client
- Clean JSON unmarshaling with appropriate error propagation
176-201: LGTM!Robust cache storage implementation:
- Configurable TTL via environment variable with sensible default
- 2-second timeout prevents blocking on Redis issues
- Errors are logged but don't crash the service (graceful degradation)
25-151: All external dependencies are properly defined.redisClientis declared ingateway/redis.go, and the functionsverifyPayment,generateAndSendReceipt,getFromCache,getCacheKey, andstoreInCacheare all properly defined with correct signatures that match their usage in the middleware.
Additional Comments (1)
The gateway service doesn't have Redis configuration passed via environment variables. Looking at This means when running via The gateway service needs to:
Without this, caching will be silently disabled in Docker deployments since Redis connection will fail during initialization. Prompt To Fix With AIThis is a comment left during a code review.
Path: docker-compose.yml
Line: 4:14
Comment:
**Critical: Gateway cannot connect to Redis in Docker**
The gateway service doesn't have Redis configuration passed via environment variables. Looking at `gateway/redis.go` line 25, the default `REDIS_URL` is `"localhost:6379"`, but in the Docker network, Redis is accessible at `"redis:6379"` (the service name).
This means when running via `docker-compose up`, the gateway will try to connect to `localhost:6379` (which doesn't exist inside the container) and fail. The .env.example correctly specifies `REDIS_URL=redis:6379` for docker-compose, but this environment variable needs to be passed to the container.
The gateway service needs to:
1. Include Redis environment variables (REDIS_URL, CACHE_ENABLED, etc.)
2. Add `redis` to the `depends_on` list so the gateway starts after Redis is ready
Without this, caching will be silently disabled in Docker deployments since Redis connection will fail during initialization.
How can I resolve this? If you propose a fix, please make it concise. |
- Add 5s timeout guard to async cache storage goroutine - Fix inefficient string->[]byte->string conversion in storeInCache - Make getCacheEnabled() consistent with getRateLimitEnabled() (accept 'true'/'1', case-insensitive) - Remove unreachable dead code in handleSummarize checking for pre-verified payment - Configure Docker Compose: add Redis env vars to gateway, enable persistence, set memory limits - Fix critical issue: gateway can now connect to Redis in Docker (redis:6379 vs localhost:6379)
| ctx, cancel := context.WithTimeout(ctx, 2*time.Second) | ||
| defer cancel() |
There was a problem hiding this comment.
[P3] The function creates a 2s timeout context when it already receives a context with a 5s timeout from the caller (line 142). This creates a nested timeout that will always hit the 2s limit. Consider removing this nested timeout since the caller already provides timeout protection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: gateway/cache.go
Line: 194:195
Comment:
[P3] The function creates a 2s timeout context when it already receives a context with a 5s timeout from the caller (line 142). This creates a nested timeout that will always hit the 2s limit. Consider removing this nested timeout since the caller already provides timeout protection.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @gateway/main.go:
- Around line 227-229: The code uses an unsafe type assertion on
c.Get("request_body") which can panic; change the block around c.Get,
requestBody and the "request_body" key to use the comma-ok form: retrieve body
via c.Get("request_body"), check the first ok, then assert body.([]byte) using
the second ok and only assign to requestBody when that assertion succeeds;
otherwise handle the mismatch (e.g., leave requestBody nil/empty and log or
return an error) to avoid a panic.
🧹 Nitpick comments (5)
docker-compose.yml (1)
11-15: Gateway Redis configuration is functional but consider adding optional env vars.The gateway connects to Redis correctly. Consider explicitly setting
CACHE_TTL_SECONDSfor visibility, even if the default (3600s) is acceptable.gateway/main.go (1)
283-286: Remove stale/confusing comments.These comments question the implementation ("generateAndSendReceipt sends error response if it fails? No, it returns error...") but
generateAndSendReceiptdoes send error responses before returning. The comments are misleading.Proposed fix
// 4. Generate & Send Receipt if err := generateAndSendReceipt(c, *paymentCtx, verifyResp.RecoveredAddress, requestBody, summary); err != nil { log.Printf("Failed to generate receipt: %v", err) - // generateAndSendReceipt sends error response if it fails? - // No, it returns error, we might have already written status if we aren't careful. - // Let's implement generateAndSendReceipt to handle sending response. return }gateway/cache.go (3)
104-106: Dead code: context values stored but never used.Since
c.Abort()is called at line 114, these stored values are never consumed by downstream handlers.Proposed fix
- // Payment Verified. Store verification for downstream if needed (though we abort) - c.Set("payment_verification", verifyResp) - c.Set("payment_context", paymentCtx) - // Generate Receipt and Respond
140-145: Nested timeout contexts - the 2s timeout instoreInCachewill take precedence.The goroutine creates a 5s timeout context, but
storeInCache(line 194) creates another 2s timeout. The shorter deadline wins, so the 5s timeout here is effectively unused. Consider removing one of them for clarity.Option: Remove inner timeout and rely on caller's context
In
storeInCache, change lines 193-195 to use the passed context directly:- // Create context with timeout for storage - ctx, cancel := context.WithTimeout(ctx, 2*time.Second) - defer cancel() - if err := redisClient.Set(ctx, key, jsonData, ttl).Err(); err != nil {This way the 5s timeout in the caller governs the operation.
180-180: Environment variable read on every cache store.
getEnvAsInt("CACHE_TTL_SECONDS", 3600)is called on each store operation. Consider caching this value at init time if high throughput is expected, though current impact is negligible.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker-compose.ymlgateway/cache.gogateway/main.gogateway/redis.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/redis.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/cache.go (1)
gateway/main.go (1)
SummarizeRequest(51-53)
🔇 Additional comments (11)
docker-compose.yml (2)
35-38:deploy.resources.limitsonly applies in Docker Swarm mode.The
deploysection is ignored when usingdocker-compose up. For standalone Compose, use the legacymem_limitdirective or run withdocker stack deployin Swarm mode.Proposed fix for non-Swarm environments
command: redis-server --appendonly yes --maxmemory 512mb --maxmemory-policy allkeys-lru - deploy: - resources: - limits: - memory: 512M + mem_limit: 512mNote: Redis's
--maxmemory 512mbalready caps Redis data, butmem_limitenforces the container-level limit if you want defense-in-depth.
26-34: Redis service configuration looks good.AOF persistence with
--appendonly yes, memory limit with LRU eviction, and volume mount for data durability are well-chosen for a caching layer.gateway/main.go (4)
168-172: LGTM!Conditional middleware wrapping based on
getCacheEnabled()is clean and avoids unnecessary overhead when caching is disabled.
190-192: LGTM!Redis initialization placed appropriately before server starts. The middleware gracefully degrades if Redis is unavailable.
290-341: LGTM!The
verifyPaymentfunction properly creates a timeout-bounded sub-context for the verifier call, handles errors comprehensively with wrapping, and returns structured results. Clean separation of concerns.
343-370: LGTM!The function cleanly handles receipt generation, storage, encoding, and response. Error responses are sent before returning errors, preventing double-writes.
gateway/cache.go (5)
19-23: LGTM!Simple, well-defined struct for cached data with timestamp for debugging/observability.
44-68: LGTM!Body reading with size limit, context storage, and restoration for downstream handlers is correctly implemented.
152-155: LGTM!Content-addressable cache key using SHA256 ensures determinism and avoids embedding raw text in keys, as required by the objectives.
157-173: LGTM!Nil-check for graceful degradation, straightforward get-and-unmarshal pattern.
202-215: LGTM!The
cachedWritercorrectly captures response body while passing through to the original writer. For the summarize endpoint's typical response sizes, the unbounded buffer is acceptable.
- Remove redundant 2s timeout in storeInCache (caller already provides 5s timeout) - Use comma-ok idiom for request_body type assertion to prevent panic
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @gateway/redis.go:
- Around line 15-58: The initRedis function currently calls log.Fatalf on
redis.ParseURL errors but handles redisClient.Ping failures by logging a
warning, closing the client, setting redisClient=nil, and returning; make the
error handling consistent by replacing the fatal exit on parse errors with the
same graceful-degradation flow: catch the error from redis.ParseURL, log a
warning including the parse error, ensure redisClient remains nil, and return so
caching is disabled instead of calling log.Fatalf; update references in
initRedis around redis.ParseURL, log.Fatalf, and the existing redisClient.Ping
error branch to use the same warning/disable/return pattern and include the
parse error details in the log.
🧹 Nitpick comments (5)
gateway/cache_test.go (2)
10-43: Avoid hardcoding key constants in multiple places (prefix/version).
TestCacheKeybakes in"ai:summary:"and assumesprefix + 64length; ifgateway/cache.goalready has constants (e.g.,cacheVersion,cachePrefix), reference them to reduce drift. If the intent is to lock the wire format, consider a single “spec” test and keep the others focused on behavior.
45-53: Consider also asserting model affects the key (collision prevention).Right now uniqueness is only demonstrated for two texts with the same model; adding a quick
same text + different model => different keycheck would better match the “text+model” requirement.gateway/redis.go (1)
25-58: Add explicit Redis client timeouts to avoid long stalls in edge failure modes.
Ping(ctx)is bounded, but other Redis ops will use go-redis defaults; consider settingDialTimeout,ReadTimeout,WriteTimeoutinredis.Options(especially important for degraded network conditions).gateway/main.go (2)
116-118: Route wiring is OK, but “CACHE_ENABLED=true + Redis down” becomes “middleware installed but inert” — confirm that’s intentional.Because
initRedis()can nil outredisClienton ping failure whilegetCacheEnabled()still returns true, the route will still attachCacheMiddleware()(which immediately falls through whenredisClient == nil). If you want clearer behavior/metrics, consider a derived flag likecacheReady := getCacheEnabled() && redisClient != nil.Also applies to: 171-175
305-356: Verifier call should ideally avoidhttp.DefaultClient(transport timeouts), even with context.You’re correctly using
NewRequestWithContext+ a verifier-specific deadline, but consider a dedicatedhttp.Clientwith conservative transport timeouts (dial/TLS/response header) to reduce tail latency risk in pathological network scenarios.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
gateway/cache.gogateway/cache_test.gogateway/main.gogateway/redis.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/cache.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/main.go (3)
gateway/cache.go (1)
CacheMiddleware(27-188)src/services/payment.ts (1)
PaymentContext(8-14)gateway/receipt.go (1)
GenerateReceipt(49-75)
🔇 Additional comments (3)
gateway/cache_test.go (1)
55-67: Good: spec-style test validates the exact SHA256 input composition.This is a nice “contract” test for key construction (version/text/model) and should catch accidental changes.
gateway/redis.go (1)
60-70:getCacheEnabled()/getEnv()look fine.Small and predictable parsing (true/1).
gateway/main.go (1)
213-303: Nice: avoids double-reading body by reusingrequest_bodyfrom middleware.The handler’s “middleware already read body” flow (Lines 231-252) is a solid integration point with
CacheMiddleware().
- Make Redis parse errors consistent with ping failures (graceful degradation) - Remove redundant timeout wrapping (verifyPayment handles its own timeout) - Remove unused getBodyBytes() method (dead code) All tests passing.
|
@roshankumar0036singh the pr is now that big it is now making more heavy load on my github server that runs the ci pipelines you clearly need to learn about ethics, ask someone before you use their tech or service |
Add Redis-backed response caching to reduce OpenRouter API costs and improve latency. Key features: - Cache-aside pattern with SHA256 content-addressable keys - Payment verification on every cache hit (security) - Configurable TTL via CACHE_TTL_SECONDS (default: 1 hour) - Graceful degradation when Redis unavailable - Race-safe implementation with sync.RWMutex Closes #31
|
Merged manually via local squash merge. Commit: 84ad6da |
There was a problem hiding this comment.
@roshankumar0036singh
Please resolve conflicts in:
gateway/cache.gogateway/redis.go
Run:
git fetch origin main
git merge origin/main
# Resolve conflicts
git pushOnce conflicts are resolved, I'll re-approve.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
gateway/redis.go (1)
13-13: Race condition on globalredisClientpointer.The
redisClientglobal is accessed from multiple goroutines—written ininitRedis()and read inCacheMiddleware()and cache helper functions—without synchronization. This can cause data races ifinitRedis()is ever called while requests are in flight (e.g., during hot-reload or reinitialization scenarios).Consider protecting access with a
sync.RWMutexor usingatomic.Pointer[redis.Client](Go 1.19+).♻️ Suggested fix using sync.RWMutex
+var ( + redisClient *redis.Client + redisClientMu sync.RWMutex +) -var redisClient *redis.ClientThen wrap reads with
redisClientMu.RLock()/RUnlock()and writes withredisClientMu.Lock()/Unlock().gateway/cache.go (3)
114-147: Inconsistent indentation in cache hit block.Lines 115-147 have inconsistent indentation (some lines appear to have an extra tab/spaces). This doesn't affect functionality but hurts readability.
♻️ Fix indentation
// Cache HIT! -> Verify Payment *BEFORE* serving - // verifyPayment creates its own timeout context, so pass request context directly - verifyResp, paymentCtx, err := verifyPayment(c.Request.Context(), signature, nonce) - if err != nil { + // verifyPayment creates its own timeout context, so pass request context directly + verifyResp, paymentCtx, err := verifyPayment(c.Request.Context(), signature, nonce) + if err != nil {(Apply similar fixes to the rest of the block)
257-269: Buffer write errors are discarded.
w.body.Write(data)andw.body.WriteString(s)errors are ignored. Whilebytes.Buffernever returns errors in practice (it panics on OOM), this pattern could mask issues if the implementation changes.For consistency, you could log if these unexpectedly fail:
if _, bufErr := w.body.Write(data); bufErr != nil { log.Printf("[WARNING] Unexpected buffer write error: %v", bufErr) }
250-255: UnusedcacheKeyfield incachedWriter.The
cacheKeyfield is set during construction (line 158) but never read. Consider removing it or using it for diagnostic logging.♻️ Remove unused field
type cachedWriter struct { gin.ResponseWriter body *bytes.Buffer - cacheKey string mu sync.RWMutex }And update the initialization on line 155-159 accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gateway/cache.gogateway/redis.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/cache.go (1)
gateway/main.go (1)
SummarizeRequest(51-53)
🔇 Additional comments (8)
gateway/redis.go (2)
15-61: LGTM: Graceful degradation pattern.The initialization handles failures gracefully—logging warnings and disabling caching rather than crashing. Properly closes the client on ping failure before clearing the reference.
63-73: LGTM!Helper functions are straightforward and correct. Using
os.LookupEnvproperly distinguishes between unset and empty values.gateway/cache.go (6)
21-25: LGTM!Clean struct definition with appropriate fields for caching. The
CachedAttimestamp enables cache age visibility for debugging and monitoring.
46-81: LGTM: Robust body size enforcement.Good defense-in-depth: checking
Content-Lengthheader first for early rejection, then usingMaxBytesReaderfor chunked/unknown-length bodies. Proper error differentiation and body restoration for downstream handlers.
83-108: Consider supportingmodelfield in request.The cache key includes the model from
OPENROUTER_MODELenv var, but ifSummarizeRequestis extended to accept a model override in the request body, the cache key derivation here would need updating. The inline comment (lines 84-86) acknowledges this—just flagging for awareness.
164-183: LGTM: Async cache storage.Good approach: captures response, only caches on success (200), and stores asynchronously with a bounded timeout using
context.Background()(correctly avoiding the cancelled request context).
187-198: LGTM: Content-addressable cache key design.Good practices: versioned prefix for invalidation, deterministic SHA256 hashing, and clear documentation about adding parameters in future.
200-240: LGTM!Cache operations handle nil client gracefully and use caller-provided context for proper timeout propagation. Safe key prefix logging is a nice touch.
AnkanMisra
added
level:advanced
Feature: Intelligent Redis Caching Layer for AI Summarization
Description
closes #31
This PR implements a robust, secure, and efficient caching layer for the
/api/ai/summarizeendpoint using Redis. This feature is designed to significantly reduce API costs for the OpenRouter service and improve response latency for repeat requests.Key Features
X-402-Signatureto ensure payment rules are enforced even for cached content.Configuration
New environment variables added to
.env.example:REDIS_URL: Connection string (default variable).REDIS_PASSWORD: Optional password.REDIS_DB: Database index (default 0).CACHE_ENABLED: Set totrueto enable caching.CACHE_TTL_SECONDS: Time-to-live for cached items (default 3600s).Verification
gateway/cache_test.goverifies key generation, determinism, and format.gateway/cache_integration_test.goverifies the full flow including cache hits, misses, and security enforcement (signature validation).Checklist
go-redis/v9added.Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Greptile Overview
Greptile Summary
This PR implements a Redis-backed caching layer for the
/api/ai/summarizeendpoint to reduce OpenRouter API costs and improve response latency. The implementation follows a cache-aside pattern with security-first design - payment verification is enforced even for cached responses.Key Implementation Details
Cache Flow:
Security Architecture:
The implementation correctly enforces payment verification on both cache hits and misses. The
X-402-Signatureis validated before serving any cached content, preventing unauthorized access to cached responses.Issues Found
Critical Issues
Goroutine leak on server shutdown (cache.go:176): Async cache storage uses
context.Background()instead of a shutdown-aware context. When the server shuts down, pending goroutines may try to write to an already-closed Redis client, causing errors or panics.Resource leak in integration test (cache_integration_test.go:18): Redis test client is never closed, causing connection leaks during test runs.
Style Issues
Positive Aspects
Confidence Score: 3/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Client participant Gateway participant CacheMiddleware participant Redis participant Verifier participant OpenRouter Note over Client,OpenRouter: Cache MISS Flow (First Request) Client->>Gateway: POST /api/ai/summarize<br/>(signature, nonce, text) Gateway->>CacheMiddleware: Process request CacheMiddleware->>CacheMiddleware: Read body & generate cache key<br/>SHA256(v1:text:model) CacheMiddleware->>Redis: GET cache key Redis-->>CacheMiddleware: Cache MISS (not found) CacheMiddleware->>Gateway: Continue to handler Gateway->>Verifier: POST /verify (signature, context) Verifier-->>Gateway: {is_valid: true, recovered_address} Gateway->>OpenRouter: POST /chat/completions (text) OpenRouter-->>Gateway: AI summary result Gateway->>Gateway: Generate receipt<br/>(hash request & response) Gateway->>Client: 200 + receipt + result Note over CacheMiddleware,Redis: Async (background goroutine) CacheMiddleware->>Redis: SET cache key = result (TTL: 1h) Note over Client,OpenRouter: Cache HIT Flow (Subsequent Request) Client->>Gateway: POST /api/ai/summarize<br/>(signature, nonce, same text) Gateway->>CacheMiddleware: Process request CacheMiddleware->>CacheMiddleware: Read body & generate cache key CacheMiddleware->>Redis: GET cache key Redis-->>CacheMiddleware: Cache HIT (cached result) CacheMiddleware->>Verifier: POST /verify (STILL VERIFY!) Verifier-->>CacheMiddleware: {is_valid: true, recovered_address} CacheMiddleware->>CacheMiddleware: Generate receipt with cached result CacheMiddleware->>Client: 200 + receipt + cached result Note over CacheMiddleware: Handler never runs - request aborted Note over Client,OpenRouter: Cache HIT with Invalid Signature Client->>Gateway: POST /api/ai/summarize<br/>(INVALID signature, same text) Gateway->>CacheMiddleware: Process request CacheMiddleware->>Redis: GET cache key Redis-->>CacheMiddleware: Cache HIT CacheMiddleware->>Verifier: POST /verify Verifier-->>CacheMiddleware: {is_valid: FALSE} CacheMiddleware->>Client: 403 Forbidden Note over CacheMiddleware: Cached response NOT served!