-
Notifications
You must be signed in to change notification settings - Fork 173
feat(api): add idempotent pledge endpoint #717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import type { Request, Response, NextFunction } from 'express'; | ||
| import { | ||
| getIdempotencyCacheEntry, | ||
| setIdempotencyCacheEntry, | ||
| buildIdempotencyCacheKey, | ||
| } from '../services/idempotencyCache'; | ||
| import type { RequestWithApiKey } from './apiKeyAuth'; | ||
|
|
||
| interface IdempotencyRequest extends Request { | ||
| idempotencyKey?: string; | ||
| } | ||
|
|
||
| export function idempotencyMiddleware( | ||
| req: IdempotencyRequest, | ||
| res: Response, | ||
| next: NextFunction, | ||
| ): void { | ||
| const idempotencyKey = req.header('Idempotency-Key'); | ||
|
|
||
| if (!idempotencyKey) { | ||
| return next(); | ||
| } | ||
|
|
||
| const apiKey = (req as unknown as RequestWithApiKey).apiKey ?? 'anonymous'; | ||
| const campaignId = req.params.id as string; | ||
| const cacheKey = buildIdempotencyCacheKey(apiKey, campaignId, idempotencyKey); | ||
|
Comment on lines
+24
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Scope the key to the contributor contract. The cache key omits the contributor entirely. Requests from different contributors with the same API key—or any requests using the 🤖 Prompt for AI Agents |
||
|
|
||
| getIdempotencyCacheEntry(cacheKey).then( | ||
| (cached) => { | ||
| if (cached) { | ||
| res.setHeader('Content-Type', 'application/json'); | ||
| res.setHeader('X-Idempotency-Cache', 'HIT'); | ||
| for (const [name, value] of Object.entries(cached.headers)) { | ||
| res.setHeader(name, value); | ||
| } | ||
| res.status(cached.statusCode).send(cached.body); | ||
| return; | ||
| } | ||
|
|
||
| const originalSend = res.send.bind(res); | ||
| res.send = function (data: unknown) { | ||
| if (res.statusCode >= 200 && res.statusCode < 300) { | ||
| const body = typeof data === 'string' ? data : JSON.stringify(data); | ||
| const entry = { | ||
| statusCode: res.statusCode, | ||
| body, | ||
| headers: { | ||
| 'Content-Type': res.getHeader('Content-Type') as string, | ||
| }, | ||
| }; | ||
| setIdempotencyCacheEntry(cacheKey, entry).catch(() => { | ||
| // Silently fail cache writes | ||
| }); | ||
| res.setHeader('X-Idempotency-Cache', 'MISS'); | ||
| } | ||
|
|
||
| return originalSend(data); | ||
| }; | ||
|
|
||
| next(); | ||
| }, | ||
| () => { | ||
| next(); | ||
| }, | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import { LRUCache } from 'lru-cache'; | ||
| import { getCacheValue, setCacheValue, isCacheAvailable } from './cache'; | ||
|
|
||
| const IDEMPOTENCY_TTL_SECONDS = 86_400; | ||
|
|
||
| const MAX_CACHE_SIZE = Number(process.env.IDEMPOTENCY_CACHE_MAX_SIZE ?? 1000); | ||
|
|
||
| interface IdempotencyCacheEntry { | ||
| statusCode: number; | ||
| body: string; | ||
| headers: Record<string, string>; | ||
| } | ||
|
|
||
| const memoryCache = new LRUCache<string, IdempotencyCacheEntry>({ | ||
| max: MAX_CACHE_SIZE, | ||
| ttl: IDEMPOTENCY_TTL_SECONDS * 1000, | ||
| }); | ||
|
|
||
| export function buildIdempotencyCacheKey( | ||
| apiKey: string, | ||
| campaignId: string, | ||
| idempotencyKey: string, | ||
| ): string { | ||
| return `idempotency:${apiKey}:${campaignId}:${idempotencyKey}`; | ||
| } | ||
|
|
||
| export async function getIdempotencyCacheEntry( | ||
| key: string, | ||
| ): Promise<IdempotencyCacheEntry | null> { | ||
| if (isCacheAvailable()) { | ||
| const cached = await getCacheValue(key); | ||
| if (cached) { | ||
| return JSON.parse(cached) as IdempotencyCacheEntry; | ||
| } | ||
| } | ||
|
|
||
| const memoryEntry = memoryCache.get(key); | ||
| if (memoryEntry) { | ||
| return memoryEntry; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| export async function setIdempotencyCacheEntry( | ||
| key: string, | ||
| entry: IdempotencyCacheEntry, | ||
| ): Promise<void> { | ||
| const serialized = JSON.stringify(entry); | ||
|
|
||
| if (isCacheAvailable()) { | ||
| await setCacheValue(key, serialized, IDEMPOTENCY_TTL_SECONDS).catch(() => { | ||
| // Silently fail Redis writes; memory cache still works | ||
| }); | ||
| } | ||
|
|
||
| memoryCache.set(key, entry); | ||
| } | ||
|
Comment on lines
+27
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift Make idempotency acquisition atomic. A lookup followed by a later write cannot prevent two concurrent requests with the same key from both missing and reaching the database. Add an atomic “claim/in-progress” operation (for Redis, e.g. 🤖 Prompt for AI Agents |
||
|
|
||
| export function clearIdempotencyCache(): void { | ||
| memoryCache.clear(); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
These tests never send
Idempotency-Key—post()takes only two arguments.post(apiPath, body)(Lines 58-66) has no third parameter, so the{ 'Idempotency-Key': ... }object is silently dropped at runtime and every one of these "idempotency" tests actually exercises the unkeyed path. This also failstsc(TS2554: expected 2 arguments, got 3) if typecheck runs in CI. Concretely, Line 513 (secondRes.datadeep-equalsfirstRes.data) and Line 535 (pledge count unchanged) cannot hold, since the second request creates a real second pledge.All six call sites passing a third argument (Lines 487-491, 500-511, 519-532, 553-564, 575-587, 595-606) must use
postWithHeadersinstead.🐛 Proposed fix (apply the same change to each keyed call site)
Alternatively, give
postan optionalheadersparameter and droppostWithHeadersto avoid two near-identical helpers.📝 Committable suggestion
🤖 Prompt for AI Agents