-
-
Couldn't load subscription status.
- Fork 221
idempotency vibeCoded #282
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
| @@ -1,3 +1,15 @@ | ||
| --- | ||
| openapi: post /v1/emails | ||
| --- | ||
|
|
||
| Send a transactional email via the public API. | ||
|
|
||
| ## Idempotency | ||
|
|
||
| Pass the optional `Idempotency-Key` header to make the request safe to retry. The key can be up to 256 characters. The server stores the canonical request body and behaves as follows: | ||
|
|
||
| - Same key + same request body → returns the original `emailId` with `200 OK` without re‑sending. | ||
| - Same key + different request body → returns `409 Conflict` with `code: NOT_UNIQUE` so you can detect the mismatch. | ||
| - Same key while another request is still being processed → returns `409 Conflict`; retry after a short delay or once the first request completes. | ||
|
|
||
| Entries expire after 24 hours. Use a unique key per logical send (for example, an order or signup ID). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,21 @@ import { createRoute, z } from "@hono/zod-openapi"; | |
| import { PublicAPIApp } from "~/server/public-api/hono"; | ||
| import { sendEmail } from "~/server/service/email-service"; | ||
| import { emailSchema } from "../../schemas/email-schema"; | ||
| import { IdempotencyService } from "~/server/service/idempotency-service"; | ||
| import { canonicalizePayload } from "~/server/utils/idempotency"; | ||
| import { UnsendApiError } from "~/server/public-api/api-error"; | ||
| import { logger } from "~/server/logger/log"; | ||
|
|
||
| const route = createRoute({ | ||
| method: "post", | ||
| path: "/v1/emails", | ||
| request: { | ||
| headers: z | ||
| .object({ | ||
| "Idempotency-Key": z.string().min(1).max(256).optional(), | ||
|
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. 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 |
||
| }) | ||
| .partial() | ||
| .openapi("Idempotency headers"), | ||
| body: { | ||
| required: true, | ||
| content: { | ||
|
|
@@ -31,24 +41,93 @@ const route = createRoute({ | |
| function send(app: PublicAPIApp) { | ||
| app.openapi(route, async (c) => { | ||
| const team = c.var.team; | ||
| const requestBody = c.req.valid("json"); | ||
|
|
||
| let html = undefined; | ||
| let html: string | undefined; | ||
| const rawHtml = requestBody?.html?.toString(); | ||
| if (rawHtml && rawHtml !== "true" && rawHtml !== "false") { | ||
| html = rawHtml; | ||
| } | ||
|
|
||
| const _html = c.req.valid("json")?.html?.toString(); | ||
| const clientPayload = { | ||
| ...requestBody, | ||
| text: requestBody.text ?? undefined, | ||
| html, | ||
| }; | ||
|
|
||
| if (_html && _html !== "true" && _html !== "false") { | ||
| html = _html; | ||
| const idemKey = c.req.header("Idempotency-Key") ?? undefined; | ||
| if (idemKey !== undefined && (idemKey.length < 1 || idemKey.length > 256)) { | ||
| throw new UnsendApiError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Invalid Idempotency-Key length", | ||
| }); | ||
| } | ||
|
|
||
| const email = await sendEmail({ | ||
| ...c.req.valid("json"), | ||
| teamId: team.id, | ||
| apiKeyId: team.apiKeyId, | ||
| text: c.req.valid("json").text ?? undefined, | ||
| html: html, | ||
| }); | ||
| let payloadHash: string | undefined; | ||
| let lockAcquired = false; | ||
|
|
||
|
Comment on lines
+66
to
+68
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. 🛠️ 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 |
||
| if (idemKey) { | ||
| ({ bodyHash: payloadHash } = canonicalizePayload(clientPayload)); | ||
|
|
||
| const existing = await IdempotencyService.getResult(team.id, idemKey); | ||
| if (existing) { | ||
| if (existing.bodyHash === payloadHash) { | ||
| logger.info({ teamId: team.id }, "Idempotency hit for email send"); | ||
| return c.json({ emailId: existing.emailIds[0] }); | ||
| } | ||
|
|
||
| throw new UnsendApiError({ | ||
| code: "NOT_UNIQUE", | ||
| message: "Idempotency-Key already used with a different payload", | ||
| }); | ||
| } | ||
|
|
||
| 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 email send" | ||
| ); | ||
| return c.json({ emailId: again.emailIds[0] }); | ||
| } | ||
|
|
||
| throw new UnsendApiError({ | ||
| code: "NOT_UNIQUE", | ||
| message: "Idempotency-Key already used with a different payload", | ||
| }); | ||
| } | ||
|
|
||
| return c.json({ emailId: email?.id }); | ||
| throw new UnsendApiError({ | ||
| code: "NOT_UNIQUE", | ||
| message: | ||
| "Request with same Idempotency-Key is in progress. Retry later.", | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| const email = await sendEmail({ | ||
| ...clientPayload, | ||
| teamId: team.id, | ||
| apiKeyId: team.apiKeyId, | ||
| }); | ||
|
|
||
| if (idemKey && payloadHash) { | ||
| await IdempotencyService.setResult(team.id, idemKey, { | ||
| bodyHash: payloadHash, | ||
| emailIds: [email.id], | ||
| }); | ||
| } | ||
|
|
||
| return c.json({ emailId: email?.id }); | ||
| } finally { | ||
| if (idemKey && lockAcquired) { | ||
| await IdempotencyService.releaseLock(team.id, idemKey); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,78 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getRedis } from "~/server/redis"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const IDEMPOTENCY_RESULT_TTL_SECONDS = 24 * 60 * 60; // 24h | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const IDEMPOTENCY_LOCK_TTL_SECONDS = 60; // 60s | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+4
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. 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export type IdempotencyRecord = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bodyHash: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| emailIds: string[]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function resultKey(teamId: number, key: string) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return `idem:${teamId}:${key}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function lockKey(teamId: number, key: string) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return `idemlock:${teamId}:${key}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const IdempotencyService = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async getResult( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| teamId: number, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| key: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<IdempotencyRecord | null> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const redis = getRedis(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const raw = await redis.get(resultKey(teamId, key)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!raw) return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const parsed = JSON.parse(raw); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parsed && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof parsed === "object" && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof (parsed as any).bodyHash === "string" && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Array.isArray((parsed as any).emailIds) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return parsed as IdempotencyRecord; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async setResult( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| teamId: number, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| key: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| record: IdempotencyRecord | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const redis = getRedis(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await redis.setex( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resultKey(teamId, key), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| IDEMPOTENCY_RESULT_TTL_SECONDS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| JSON.stringify(record) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+56
to
+70
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.
The idempotency lock is saved with a fixed value and Useful? React with 👍 / 👎. 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. 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+56
to
+71
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. 🛠️ 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 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const IDEMPOTENCY_CONSTANTS = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RESULT_TTL_SECONDS: IDEMPOTENCY_RESULT_TTL_SECONDS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LOCK_TTL_SECONDS: IDEMPOTENCY_LOCK_TTL_SECONDS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
🛠️ Refactor suggestion | 🟠 Major
Adopt token-based lock API (pair with service change).
Mirror the safe lock ownership pattern here.
Also applies to: 152-156, 76-78