idempotency vibeCoded #282
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request introduces idempotency support for email sending APIs. The implementation includes a new Redis-based idempotency service that stores request results and manages per-key locking for 24 hours. Backend handlers for single and batch email endpoints now validate the optional Idempotency-Key header, canonicalize payloads using SHA-256 hashing, and return cached results on repeated requests with identical payloads. Conflict detection returns 409 responses for mismatched payloads or in-progress requests. The TypeScript and Python SDKs are updated to accept optional idempotency keys and propagate them via headers. API documentation is enhanced with idempotency behavior specifications. Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Deploying usesend with
|
| Latest commit: |
c5094c5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4d84b736.usesend.pages.dev |
| Branch Preview URL: | https://km-2025-10-25-idempotency-ke.usesend.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async acquireLock(teamId: number, key: string): Promise<boolean> { | ||
| const redis = getRedis(); | ||
| const ok = await redis.set( | ||
| lockKey(teamId, key), | ||
| "1", | ||
| "EX", | ||
| IDEMPOTENCY_LOCK_TTL_SECONDS, | ||
| "NX" | ||
| ); | ||
| return ok === "OK"; | ||
| }, | ||
|
|
||
| async releaseLock(teamId: number, key: string): Promise<void> { | ||
| const redis = getRedis(); | ||
| await redis.del(lockKey(teamId, key)); |
There was a problem hiding this comment.
The idempotency lock is saved with a fixed value and releaseLock unconditionally deletes the key. If an email send takes longer than the 60‑second TTL, the lock expires and a second request can acquire it. When the original long‑running request eventually calls releaseLock, it removes the second request’s lock as well, letting further concurrent sends with the same idempotency key proceed. To prevent clobbering another client's lock, the lock should store a unique token and only be deleted when the caller proves ownership.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
packages/python-sdk/README.md (2)
40-51: Improve the positioning and clarity of the idempotency 409 behavior note.The comment on line 51 about HTTP 409 is positioned after the code block and lacks context. It should be integrated closer to (or within) the code examples it describes, with clearer language explaining when the conflict occurs.
Apply this diff to improve clarity and positioning:
~# Idempotent retries: same payload + same key returns the original response ~resp, _ = client.emails.send( ~ payload=payload, ~ idempotency_key="signup-123", ~) ~ ~# Works for batch requests as well ~resp, _ = client.emails.batch( ~ payload=[payload], ~ idempotency_key="bulk-welcome-1", ~) ~# If the same key is reused with a different payload, the API responds with HTTP 409. +# Idempotent retries: same payload + same key returns the original response. +resp, _ = client.emails.send( + payload=payload, + idempotency_key="signup-123", +) + +# Works for batch requests as well. +# Note: Reusing the same idempotency key with a different payload returns HTTP 409. +resp, _ = client.emails.batch( + payload=[payload], + idempotency_key="bulk-welcome-1", +)
96-99: Document idempotency_key parameter in "Available Resources" section.The idempotency feature is new and demonstrated in examples, but the method signatures in "Available Resources" should mention the optional
idempotency_keyparameter for completeness and discoverability.Consider updating the documentation:
-**Emails**: `client.emails.send()`, `client.emails.get()` +**Emails**: `client.emails.send(payload, idempotency_key=None)`, `client.emails.get()`, `client.emails.batch(payload, idempotency_key=None)`packages/sdk/src/usesend.ts (1)
15-17: Consider explicitly exporting RequestOptions.The
RequestOptionstype is used in public method signatures but isn't explicitly exported. While TypeScript infers it from usage, explicitly exporting types used in the public API improves developer experience and documentation generation.Apply this diff:
-type RequestOptions = { +export type RequestOptions = { headers?: HeadersInit; };packages/sdk/src/email.ts (1)
70-72: Consider explicitly exporting EmailRequestOptions.Similar to
RequestOptionsin usesend.ts, this type is used in public method signatures and should be explicitly exported for better API documentation and developer experience.Apply this diff:
-type EmailRequestOptions = { +export type EmailRequestOptions = { idempotencyKey?: string; };apps/web/src/server/utils/idempotency.ts (1)
63-68: Add explicit return type annotation.While TypeScript infers the return type correctly, adding an explicit annotation improves code documentation and catches potential type errors.
Apply this diff:
-export function canonicalizePayload(payload: unknown) { +export function canonicalizePayload(payload: unknown): { canonical: string; bodyHash: string } { const normalized = normalize(payload); const canonical = JSON.stringify(normalized ?? null); const bodyHash = createHash("sha256").update(canonical).digest("hex"); return { canonical, bodyHash }; }apps/web/src/server/public-api/api/emails/send-email.ts (1)
36-37: Nit: wrong response description.Change to “Create email” or “Email created”.
- description: "Retrieve the user", + description: "Email created",apps/web/src/server/public-api/api/emails/batch-email.ts (2)
59-66: Unify html normalization; avoid special-casing "true"/"false".Batch currently drops "true"/"false" string values and may pass non-strings if validation ever loosens. Align to “string only”.
-const normalizedPayloads = emailPayloads.map((payload) => ({ - ...payload, - text: payload.text ?? undefined, - html: - payload.html && payload.html !== "true" && payload.html !== "false" - ? payload.html - : undefined, -})); +const normalizedPayloads = emailPayloads.map((payload) => { + const html = typeof payload.html === "string" ? payload.html : undefined; + return { + ...payload, + text: payload.text ?? undefined, + html, + }; +});
19-25: Minor:.partial()is redundant on an object with an optional field.You can drop
.partial()to simplify the header schema without changing behavior.-headers: z - .object({ - "Idempotency-Key": z.string().min(1).max(256).optional(), - }) - .partial() - .openapi("Idempotency headers"), +headers: z + .object({ + "Idempotency-Key": z.string().min(1).max(256).optional(), + }) + .openapi("Idempotency headers"),packages/python-sdk/usesend/emails.py (1)
21-24: Validate idempotency_key length client-side (1–256).Preempt server errors and give clearer feedback.
-def _idem_headers(idempotency_key: Optional[str]) -> Optional[Dict[str, str]]: - if idempotency_key: - return {"Idempotency-Key": idempotency_key} - return None +def _idem_headers(idempotency_key: Optional[str]) -> Optional[Dict[str, str]]: + if idempotency_key is None: + return None + key = idempotency_key.strip() + if not (1 <= len(key) <= 256): + raise ValueError("idempotency_key must be 1..256 characters") + return {"Idempotency-Key": key}packages/python-sdk/usesend/usesend.py (1)
95-99: Add request timeout to avoid indefinite hangs.Set a sensible default (e.g., connect/read total ~10s). This improves resilience for clients.
- resp = self._session.request( + resp = self._session.request( method, f"{self.url}{path}", headers=self._build_headers(headers), json=json, + timeout=10, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/docs/api-reference/emails/batch-email.mdx(1 hunks)apps/docs/api-reference/emails/send-email.mdx(1 hunks)apps/docs/api-reference/introduction.mdx(1 hunks)apps/web/src/server/public-api/api/emails/batch-email.ts(3 hunks)apps/web/src/server/public-api/api/emails/send-email.ts(2 hunks)apps/web/src/server/service/idempotency-service.ts(1 hunks)apps/web/src/server/utils/idempotency.ts(1 hunks)packages/python-sdk/README.md(1 hunks)packages/python-sdk/usesend/emails.py(3 hunks)packages/python-sdk/usesend/usesend.py(2 hunks)packages/sdk/README.md(1 hunks)packages/sdk/src/email.ts(2 hunks)packages/sdk/src/usesend.ts(3 hunks)plan-idempotency.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier 3 (run pnpm format)
Files:
packages/sdk/README.mdpackages/python-sdk/README.mdapps/web/src/server/public-api/api/emails/batch-email.tsapps/web/src/server/service/idempotency-service.tsapps/web/src/server/utils/idempotency.tsapps/web/src/server/public-api/api/emails/send-email.tspackages/sdk/src/email.tsplan-idempotency.mdpackages/sdk/src/usesend.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/public-api/api/emails/batch-email.tsapps/web/src/server/service/idempotency-service.tsapps/web/src/server/utils/idempotency.tsapps/web/src/server/public-api/api/emails/send-email.tspackages/sdk/src/email.tspackages/sdk/src/usesend.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript-first: use .ts/.tsx for source code (avoid JavaScript source files)
Use 2-space indentation and semicolons (Prettier 3 enforces these)
Adhere to @usesend/eslint-config; fix all ESLint warnings (CI fails on warnings)
Do not use dynamic imports; always place imports at the top of the module
Files:
apps/web/src/server/public-api/api/emails/batch-email.tsapps/web/src/server/service/idempotency-service.tsapps/web/src/server/utils/idempotency.tsapps/web/src/server/public-api/api/emails/send-email.tspackages/sdk/src/email.tspackages/sdk/src/usesend.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: In apps/web, use the/ alias for src imports (e.g., import { x } from "/utils/x")
Prefer using tRPC in apps/web unless explicitly asked otherwise
Files:
apps/web/src/server/public-api/api/emails/batch-email.tsapps/web/src/server/service/idempotency-service.tsapps/web/src/server/utils/idempotency.tsapps/web/src/server/public-api/api/emails/send-email.ts
🧬 Code graph analysis (7)
apps/web/src/server/public-api/api/emails/batch-email.ts (6)
apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)apps/web/src/server/utils/idempotency.ts (1)
canonicalizePayload(63-68)apps/web/src/server/service/idempotency-service.ts (1)
IdempotencyService(19-72)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/types/index.ts (1)
EmailContent(1-18)apps/web/src/server/service/email-service.ts (1)
sendBulkEmails(368-789)
apps/web/src/server/service/idempotency-service.ts (1)
apps/web/src/server/redis.ts (1)
getRedis(6-13)
packages/python-sdk/usesend/emails.py (5)
packages/sdk/src/email.ts (4)
Emails(74-153)send(79-81)create(83-101)batch(109-125)packages/python-sdk/usesend/types.py (3)
EmailCreateResponse(229-230)APIError(449-451)EmailBatchResponse(261-262)packages/python-sdk/usesend/contacts.py (1)
create(25-32)packages/python-sdk/usesend/usesend.py (1)
post(120-126)packages/sdk/src/usesend.ts (1)
post(108-119)
apps/web/src/server/public-api/api/emails/send-email.ts (5)
apps/web/src/server/public-api/api-error.ts (1)
UnsendApiError(62-75)apps/web/src/server/utils/idempotency.ts (1)
canonicalizePayload(63-68)apps/web/src/server/service/idempotency-service.ts (1)
IdempotencyService(19-72)apps/web/src/server/logger/log.ts (1)
logger(31-63)apps/web/src/server/service/email-service.ts (1)
sendEmail(55-302)
packages/sdk/src/email.ts (1)
packages/sdk/src/usesend.ts (1)
UseSend(19-174)
packages/python-sdk/usesend/usesend.py (4)
packages/python-sdk/usesend/emails.py (2)
update(89-95)get(85-87)packages/python-sdk/usesend/contacts.py (3)
update(42-49)get(34-40)delete(60-66)packages/python-sdk/usesend/campaigns.py (1)
get(32-38)packages/python-sdk/usesend/domains.py (2)
get(34-36)delete(38-40)
packages/sdk/src/usesend.ts (2)
packages/python-sdk/usesend/usesend.py (1)
UseSend(34-155)packages/sdk/types/index.ts (1)
ErrorResponse(1-4)
🪛 LanguageTool
apps/docs/api-reference/emails/send-email.mdx
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...IQUE` so you can detect the mismatch. - Same key while another request is still bein...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
plan-idempotency.md
[style] ~127-~127: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...an Idempotency-Key is reused with the exact same request body (as per server canonicaliz...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
apps/docs/api-reference/emails/batch-email.mdx
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...` so you can detect accidental reuse. - Same key while another batch is still being ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
plan-idempotency.md
84-84: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
85-85: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
86-86: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
87-87: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (10)
plan-idempotency.md (1)
1-173: Comprehensive idempotency plan.The planning document is thorough and covers all necessary aspects: Redis schema, API contracts, SDK changes, canonicalization, error handling, and verification. The design choices (24h TTL, 60s locks, SHA-256 hashing, 409 conflicts) are appropriate for the use case.
The static analysis hints about "exact same" wordiness and list indentation are cosmetic and can be addressed optionally.
packages/sdk/src/usesend.ts (2)
55-67: Clean header merging implementation.The
mergeHeadershelper correctly combines base headers with per-request headers using the Headers API. The early return optimization for the no-extra-headers case is good.
108-172: HTTP methods consistently support per-request options.All HTTP verb methods now accept optional
RequestOptionsand correctly forward headers throughfetchRequest. Thedeletemethod appropriately handles optional body. The implementation is consistent and correct.packages/sdk/src/email.ts (2)
79-101: Idempotency support correctly integrated into email methods.The
sendandcreatemethods now acceptEmailRequestOptionsand properly forward the idempotency key as anIdempotency-Keyheader when provided. The conditional header construction (lines 95-97) is clean and correct.
109-125: Batch method consistently supports idempotency.The
batchmethod mirrors the same idempotency pattern ascreate, correctly propagating the key to the batch endpoint. The implementation is consistent with the single-send flow.apps/docs/api-reference/emails/send-email.mdx (1)
7-15: Clear and accurate idempotency documentation.The documentation clearly explains the three idempotency scenarios and the 24-hour expiry. The explanation is concise and actionable for API consumers.
The static analysis hint about repetitive sentence starts with "Same" is a minor style issue that could optionally be addressed by rewording, but the current text is clear.
packages/sdk/README.md (1)
52-82: Helpful idempotency examples.The examples clearly demonstrate how to use idempotency keys for both single and batch sends. The comment about HTTP 409 on payload mismatch is a useful warning for users.
apps/docs/api-reference/introduction.mdx (1)
26-28: Concise idempotency introduction.The brief overview appropriately introduces the idempotency feature at the API introduction level, with references to both affected endpoints. The explanation is clear and sufficient for this high-level page.
apps/docs/api-reference/emails/batch-email.mdx (1)
7-15: Consistent batch idempotency documentation.The documentation correctly explains that the idempotency key applies to the entire batch payload and describes the same three scenarios as the single-send endpoint. The consistency across endpoints is good.
The static analysis hint about repetitive "Same" is minor and optional to address.
apps/web/src/server/utils/idempotency.ts (1)
11-61: Solid canonicalization logic.The
normalizefunction handles the expected types for email payloads correctly:
- Sorts object keys for determinism
- Filters undefined values to ensure semantic equivalence
- Converts dates to ISO strings
- Preserves array order (by design for recipient lists)
The fallback to
String(value)at line 60 handles exotic types gracefully. For email payloads (which are JSON-serializable), this implementation is appropriate.
| lockAcquired = await IdempotencyService.acquireLock(team.id, idemKey); | ||
| if (!lockAcquired) { | ||
| const again = await IdempotencyService.getResult(team.id, idemKey); | ||
| if (again) { | ||
| if (again.bodyHash === payloadHash) { | ||
| logger.info( | ||
| { teamId: team.id }, | ||
| "Idempotency hit after contention for bulk email send" | ||
| ); | ||
| const responseData = again.emailIds.map((id) => ({ emailId: id })); | ||
| return c.json({ data: responseData }); | ||
| } | ||
|
|
||
| throw new UnsendApiError({ | ||
| code: "NOT_UNIQUE", | ||
| message: "Idempotency-Key already used with a different payload", | ||
| }); | ||
| } | ||
|
|
||
| throw new UnsendApiError({ | ||
| code: "NOT_UNIQUE", | ||
| message: | ||
| "Request with same Idempotency-Key is in progress. Retry later.", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Adopt token-based lock API (pair with service change).
Mirror the safe lock ownership pattern here.
- let payloadHash: string | undefined;
- let lockAcquired = false;
+ let payloadHash: string | undefined;
+ let lockToken: string | null = null;
@@
- lockAcquired = await IdempotencyService.acquireLock(team.id, idemKey);
- if (!lockAcquired) {
+ lockToken = await IdempotencyService.acquireLock(team.id, idemKey);
+ if (!lockToken) {
const again = await IdempotencyService.getResult(team.id, idemKey);
@@
- if (idemKey && lockAcquired) {
- await IdempotencyService.releaseLock(team.id, idemKey);
+ if (idemKey && lockToken) {
+ await IdempotencyService.releaseLock(team.id, idemKey, lockToken);
}Also applies to: 152-156, 76-78
| let payloadHash: string | undefined; | ||
| let lockAcquired = false; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Adopt token-based lock API (pair with service change).
Switch from boolean locks to token ownership and safe release.
- let payloadHash: string | undefined;
- let lockAcquired = false;
+ let payloadHash: string | undefined;
+ let lockToken: string | null = null;
@@
- lockAcquired = await IdempotencyService.acquireLock(team.id, idemKey);
- if (!lockAcquired) {
+ lockToken = await IdempotencyService.acquireLock(team.id, idemKey);
+ if (!lockToken) {
const again = await IdempotencyService.getResult(team.id, idemKey);
@@
- if (idemKey && lockAcquired) {
- await IdempotencyService.releaseLock(team.id, idemKey);
+ if (idemKey && lockToken) {
+ await IdempotencyService.releaseLock(team.id, idemKey, lockToken);
}Also applies to: 85-108, 126-130
🤖 Prompt for AI Agents
In apps/web/src/server/public-api/api/emails/send-email.ts around lines 66-68
(and similarly update ranges 85-108 and 126-130), replace the boolean lock
pattern with a token-based lock ownership API: change lockAcquired:boolean to a
lockToken:string|undefined, obtain a token when acquiring the lock (store it in
lockToken), pass that token into the lock-release call and only release when the
token matches, and ensure all early returns and finally/cleanup paths release
using the token-aware unlock function (or skip release if no token). Update
variable names and control flow so acquisition sets lockToken, failures do not
call boolean releases, and the unlock call uses the token to perform a safe,
owner-only release.
| const IDEMPOTENCY_RESULT_TTL_SECONDS = 24 * 60 * 60; // 24h | ||
| const IDEMPOTENCY_LOCK_TTL_SECONDS = 60; // 60s |
There was a problem hiding this comment.
Consider longer or renewable lock TTL.
60s may be shorter than worst-case processing (DB writes + queueing). Increase TTL (e.g., 300s) or add periodic renewal to avoid duplicate sends when locks expire mid-flight.
🤖 Prompt for AI Agents
In apps/web/src/server/service/idempotency-service.ts around lines 3 to 4, the
idempotency lock TTL is only 60s which can expire during long-running processing
and cause duplicate sends; either increase IDEMPOTENCY_LOCK_TTL_SECONDS to a
higher value (e.g., 300) or implement periodic lock renewal (extend the lock
before it expires while processing) and ensure renewal failures are handled and
locks are released on completion or error.
| async acquireLock(teamId: number, key: string): Promise<boolean> { | ||
| const redis = getRedis(); | ||
| const ok = await redis.set( | ||
| lockKey(teamId, key), | ||
| "1", | ||
| "EX", | ||
| IDEMPOTENCY_LOCK_TTL_SECONDS, | ||
| "NX" | ||
| ); | ||
| return ok === "OK"; | ||
| }, | ||
|
|
||
| async releaseLock(teamId: number, key: string): Promise<void> { | ||
| const redis = getRedis(); | ||
| await redis.del(lockKey(teamId, key)); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix distributed lock: ensure ownership on release (use token + compare-and-del).
Current release deletes the lock unconditionally. If the lock expires mid-flight and another request re-acquires it, your DEL can drop the new owner’s lock, enabling concurrent sends. Use a unique token on SET NX EX and only delete if the stored value matches.
Apply this diff:
@@
-import { getRedis } from "~/server/redis";
+import { getRedis } from "~/server/redis";
+import { randomUUID } from "crypto";
@@
-const IDEMPOTENCY_LOCK_TTL_SECONDS = 60; // 60s
+const IDEMPOTENCY_LOCK_TTL_SECONDS = 60; // 60s
@@
-export const IdempotencyService = {
+export const IdempotencyService = {
@@
- async acquireLock(teamId: number, key: string): Promise<boolean> {
+ async acquireLock(teamId: number, key: string): Promise<string | null> {
const redis = getRedis();
- const ok = await redis.set(
- lockKey(teamId, key),
- "1",
+ const token = randomUUID();
+ const ok = await redis.set(
+ lockKey(teamId, key),
+ token,
"EX",
IDEMPOTENCY_LOCK_TTL_SECONDS,
"NX"
);
- return ok === "OK";
+ return ok === "OK" ? token : null;
},
- async releaseLock(teamId: number, key: string): Promise<void> {
+ async releaseLock(teamId: number, key: string, token: string): Promise<void> {
const redis = getRedis();
- await redis.del(lockKey(teamId, key));
+ // Delete only if we still own the lock
+ const script = `
+ if redis.call("get", KEYS[1]) == ARGV[1] then
+ return redis.call("del", KEYS[1])
+ else
+ return 0
+ end
+ `;
+ await redis.eval(script, 1, lockKey(teamId, key), token);
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async acquireLock(teamId: number, key: string): Promise<boolean> { | |
| const redis = getRedis(); | |
| const ok = await redis.set( | |
| lockKey(teamId, key), | |
| "1", | |
| "EX", | |
| IDEMPOTENCY_LOCK_TTL_SECONDS, | |
| "NX" | |
| ); | |
| return ok === "OK"; | |
| }, | |
| async releaseLock(teamId: number, key: string): Promise<void> { | |
| const redis = getRedis(); | |
| await redis.del(lockKey(teamId, key)); | |
| }, | |
| async acquireLock(teamId: number, key: string): Promise<string | null> { | |
| const redis = getRedis(); | |
| const token = randomUUID(); | |
| const ok = await redis.set( | |
| lockKey(teamId, key), | |
| token, | |
| "EX", | |
| IDEMPOTENCY_LOCK_TTL_SECONDS, | |
| "NX" | |
| ); | |
| return ok === "OK" ? token : null; | |
| }, | |
| async releaseLock(teamId: number, key: string, token: string): Promise<void> { | |
| const redis = getRedis(); | |
| // Delete only if we still own the lock | |
| const script = ` | |
| if redis.call("get", KEYS[1]) == ARGV[1] then | |
| return redis.call("del", KEYS[1]) | |
| else | |
| return 0 | |
| end | |
| `; | |
| await redis.eval(script, 1, lockKey(teamId, key), token); | |
| }, |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 14 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="apps/web/src/server/public-api/api/emails/send-email.ts">
<violation number="1" location="apps/web/src/server/public-api/api/emails/send-email.ts:16">
Idempotency logic and Idempotency-Key header validation are duplicated in apps/web/src/server/public-api/api/emails/batch-email.ts. This critical logic should be extracted into a reusable middleware or utility function.</violation>
</file>
<file name="plan-idempotency.md">
<violation number="1" location="plan-idempotency.md:45">
`getResult` needs to return an object that includes the cached body hash so the later `stored.bodyHash` comparison can work; returning only `string[]` makes the documented idempotency check impossible.</violation>
<violation number="2" location="plan-idempotency.md:46">
`setResult` must accept a payload including both `bodyHash` and `emailIds`; otherwise the documented `setResult(... { bodyHash, emailIds })` call cannot work and the cache will lack the hash needed for idempotency.</violation>
</file>
<file name="apps/web/src/server/utils/idempotency.ts">
<violation number="1" location="apps/web/src/server/utils/idempotency.ts:33">
Creating the accumulator with `{}` drops `"__proto__"` keys because later assignments mutate the prototype instead of storing the value, so such payloads collide during canonicalization. Use a null-prototype object to preserve every key.</violation>
</file>
<file name="apps/web/src/server/service/idempotency-service.ts">
<violation number="1" location="apps/web/src/server/service/idempotency-service.ts:70">
Release deletes the lock unconditionally, which risks dropping a new owner’s lock if the original expires and is re-acquired. Use a token-based lock and compare-and-delete (e.g., Lua script) to ensure only the owner releases the lock.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| request: { | ||
| headers: z | ||
| .object({ | ||
| "Idempotency-Key": z.string().min(1).max(256).optional(), |
There was a problem hiding this comment.
Idempotency logic and Idempotency-Key header validation are duplicated in apps/web/src/server/public-api/api/emails/batch-email.ts. This critical logic should be extracted into a reusable middleware or utility function.
Prompt for AI agents
Address the following comment on apps/web/src/server/public-api/api/emails/send-email.ts at line 16:
<comment>Idempotency logic and Idempotency-Key header validation are duplicated in apps/web/src/server/public-api/api/emails/batch-email.ts. This critical logic should be extracted into a reusable middleware or utility function.</comment>
<file context>
@@ -2,11 +2,21 @@ import { createRoute, z } from "@hono/zod-openapi";
request: {
+ headers: z
+ .object({
+ "Idempotency-Key": z.string().min(1).max(256).optional(),
+ })
+ .partial()
</file context>
| 1. Common util (service level) | ||
| - Add `IdempotencyService` with helpers using existing Redis client (`getRedis`): | ||
| - `getResult(teamId: number, key: string): Promise<string[] | null>` | ||
| - `setResult(teamId: number, key: string, emailIds: string[]): Promise<void>` (EX 24h) |
There was a problem hiding this comment.
setResult must accept a payload including both bodyHash and emailIds; otherwise the documented setResult(... { bodyHash, emailIds }) call cannot work and the cache will lack the hash needed for idempotency.
Prompt for AI agents
Address the following comment on plan-idempotency.md at line 46:
<comment>`setResult` must accept a payload including both `bodyHash` and `emailIds`; otherwise the documented `setResult(... { bodyHash, emailIds })` call cannot work and the cache will lack the hash needed for idempotency.</comment>
<file context>
@@ -0,0 +1,174 @@
+1. Common util (service level)
+ - Add `IdempotencyService` with helpers using existing Redis client (`getRedis`):
+ - `getResult(teamId: number, key: string): Promise<string[] | null>`
+ - `setResult(teamId: number, key: string, emailIds: string[]): Promise<void>` (EX 24h)
+ - `acquireLock(teamId: number, key: string): Promise<boolean>` (`SET NX EX 60`)
+ - `releaseLock(teamId: number, key: string): Promise<void>` (best-effort `DEL`)
</file context>
| - `setResult(teamId: number, key: string, emailIds: string[]): Promise<void>` (EX 24h) | |
| - `setResult(teamId: number, key: string, payload: { bodyHash: string; emailIds: string[] }): Promise<void>` (EX 24h) |
| ## Server Implementation Plan | ||
| 1. Common util (service level) | ||
| - Add `IdempotencyService` with helpers using existing Redis client (`getRedis`): | ||
| - `getResult(teamId: number, key: string): Promise<string[] | null>` |
There was a problem hiding this comment.
getResult needs to return an object that includes the cached body hash so the later stored.bodyHash comparison can work; returning only string[] makes the documented idempotency check impossible.
Prompt for AI agents
Address the following comment on plan-idempotency.md at line 45:
<comment>`getResult` needs to return an object that includes the cached body hash so the later `stored.bodyHash` comparison can work; returning only `string[]` makes the documented idempotency check impossible.</comment>
<file context>
@@ -0,0 +1,174 @@
+## Server Implementation Plan
+1. Common util (service level)
+ - Add `IdempotencyService` with helpers using existing Redis client (`getRedis`):
+ - `getResult(teamId: number, key: string): Promise<string[] | null>`
+ - `setResult(teamId: number, key: string, emailIds: string[]): Promise<void>` (EX 24h)
+ - `acquireLock(teamId: number, key: string): Promise<boolean>` (`SET NX EX 60`)
</file context>
| ([keyA], [keyB]) => (keyA < keyB ? -1 : keyA > keyB ? 1 : 0) | ||
| ); | ||
|
|
||
| const result: Record<string, CanonicalValue> = {}; |
There was a problem hiding this comment.
Creating the accumulator with {} drops "__proto__" keys because later assignments mutate the prototype instead of storing the value, so such payloads collide during canonicalization. Use a null-prototype object to preserve every key.
Prompt for AI agents
Address the following comment on apps/web/src/server/utils/idempotency.ts at line 33:
<comment>Creating the accumulator with `{}` drops `"__proto__"` keys because later assignments mutate the prototype instead of storing the value, so such payloads collide during canonicalization. Use a null-prototype object to preserve every key.</comment>
<file context>
@@ -0,0 +1,69 @@
+ ([keyA], [keyB]) => (keyA < keyB ? -1 : keyA > keyB ? 1 : 0)
+ );
+
+ const result: Record<string, CanonicalValue> = {};
+ for (const [key, val] of entries) {
+ const normalized = normalize(val);
</file context>
| const result: Record<string, CanonicalValue> = {}; | |
| const result: Record<string, CanonicalValue> = Object.create(null); |
|
|
||
| async releaseLock(teamId: number, key: string): Promise<void> { | ||
| const redis = getRedis(); | ||
| await redis.del(lockKey(teamId, key)); |
There was a problem hiding this comment.
Release deletes the lock unconditionally, which risks dropping a new owner’s lock if the original expires and is re-acquired. Use a token-based lock and compare-and-delete (e.g., Lua script) to ensure only the owner releases the lock.
Prompt for AI agents
Address the following comment on apps/web/src/server/service/idempotency-service.ts at line 70:
<comment>Release deletes the lock unconditionally, which risks dropping a new owner’s lock if the original expires and is re-acquired. Use a token-based lock and compare-and-delete (e.g., Lua script) to ensure only the owner releases the lock.</comment>
<file context>
@@ -0,0 +1,78 @@
+
+ async releaseLock(teamId: number, key: string): Promise<void> {
+ const redis = getRedis();
+ await redis.del(lockKey(teamId, key));
+ },
+};
</file context>
Summary by cubic
Adds idempotency to POST /v1/emails and /v1/emails/batch so retries don’t send duplicates. SDKs support passing Idempotency-Key; docs describe behavior and 24h expiry.
Summary by CodeRabbit
New Features
idempotencyKey/idempotency_keyparametersDocumentation