From 43d6b5c1a919fed5bbd6f8231155cdfcd3e39354 Mon Sep 17 00:00:00 2001 From: aurumflux20 Date: Fri, 7 Aug 2026 23:50:08 -0700 Subject: [PATCH] send_sms: stop a retry from sending the SMS twice transactionId was minted with randomUUID() on every call, so two calls describing the same shipment looked like two different shipments. An agent that retries after a timeout -- the response was lost, the SMS already went -- sends the message again. Making transactionId deterministic is not enough on its own: the README is explicit that the API neither assigns nor interprets that field ("transactionId generuje serwer (UUID) - API go nie nadaje"), so there is no basis to assume it deduplicates on it. The guard has to live in this server. send_sms now takes an optional transactionId, and derives one from the shipment when the caller does not supply it, so an identical resend collapses while a genuinely different message is unaffected. A shipment already sent within SMSON_DEDUP_TTL_MS (default 15 min) returns its original result instead of being sent again. A FAILED send is deliberately not remembered, so a transient failure stays retryable. Scope stated honestly: the record is in-memory, so it covers a retry within one running server and not a restart. That is the case people actually hit; a durable store would close the rest. test-dedup.mjs drives the real server over stdio against a local stand-in for the API and asserts all seven behaviours, including that a different message still sends and that a failure stays retryable. Co-Authored-By: Claude Opus 5 --- index.js | 118 +++++++++++++++++++++++++++++++++++++++++-------- test-dedup.mjs | 100 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 19 deletions(-) create mode 100644 test-dedup.mjs diff --git a/index.js b/index.js index cbdee7e..804a148 100644 --- a/index.js +++ b/index.js @@ -1,4 +1,4 @@ -import { randomUUID } from "node:crypto"; +import { createHash } from "node:crypto"; import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; import { z } from "zod"; @@ -32,6 +32,56 @@ async function call(path, { body, params } = {}) { return { content: [{ type: "text", text }], isError: text.includes('"FAILED"') }; } +/** + * Client-side send guard. + * + * `transactionId` was minted with randomUUID() on every call, so two calls + * describing the SAME shipment looked like two different shipments to the API. + * An agent that retries after a timeout — the response was lost, the SMS was + * already sent — therefore sends the message twice. + * + * Making transactionId deterministic is not on its own enough: the README is + * explicit that the API does not assign or interpret that field + * ("transactionId generuje serwer (UUID) - API go nie nadaje"), so there is no + * basis to assume it deduplicates on it. The guard has to live here. + * + * Scope, stated plainly: this is in-memory, so it protects a retry within one + * running server. It does NOT survive a restart, and it is per-process. That + * covers the common case (an agent retrying a call seconds later) and not the + * rarer one (the process dies mid-send). A durable store would close that too; + * this is the smallest change that fixes the failure people actually hit. + */ +const DEDUP_TTL_MS = Number(process.env.SMSON_DEDUP_TTL_MS ?? 15 * 60 * 1000); +const recentSends = new Map(); // transactionId -> { at, result } + +/** Derive a stable id from what makes one shipment different from another. */ +function deriveTransactionId(payload) { + return ( + "tx_" + + createHash("sha256").update(JSON.stringify(payload)).digest("hex").slice(0, 32) + ); +} + +function rememberedResult(id) { + const hit = recentSends.get(id); + if (!hit) return null; + if (Date.now() - hit.at > DEDUP_TTL_MS) { + recentSends.delete(id); + return null; + } + return hit.result; +} + +function remember(id, result) { + // Bound the map so a long-running server cannot grow without limit. + if (recentSends.size > 1000) { + for (const [k, v] of recentSends) { + if (Date.now() - v.at > DEDUP_TTL_MS) recentSends.delete(k); + } + } + recentSends.set(id, { at: Date.now(), result }); +} + const server = new McpServer({ name: "smson", version: "1.0.0" }); server.registerTool( @@ -53,30 +103,60 @@ server.registerTool( ttl: z.string().optional().describe("Message validity, e.g. 1_HOUR"), normalize: z.boolean().optional().describe("Strip Polish diacritics from content"), timestamp: z.string().optional().describe("Scheduled send time, e.g. 2025-08-02T13:45:00"), + transactionId: z + .string() + .optional() + .describe( + "Idempotency key for this send. Repeat the same value to make a retry safe: if this server already sent that shipment within SMSON_DEDUP_TTL_MS (default 15 min) it returns the original result instead of sending again. Omit it and one is derived from the shipment itself, so retrying an identical send is safe by default." + ), }, }, - ({ messages, sender, ttl, normalize, timestamp }) => - call("/createShipment", { - body: { - transactionId: randomUUID(), - ...(timestamp && { timestamp }), - system: SYSTEM, - createShipment: { - sender: { textId: sender ?? SENDER }, - ...(ttl && { ttl }), - normalize: String(normalize ?? false), - messages: { - multiContent: messages.map((m, i) => ({ - externalId: m.externalId ?? String(i + 1), - MSISDN: m.msisdn, - content: m.content, - })), - }, + async ({ messages, sender, ttl, normalize, timestamp, transactionId }) => { + const shipment = { + ...(timestamp && { timestamp }), + system: SYSTEM, + createShipment: { + sender: { textId: sender ?? SENDER }, + ...(ttl && { ttl }), + normalize: String(normalize ?? false), + messages: { + multiContent: messages.map((m, i) => ({ + externalId: m.externalId ?? String(i + 1), + MSISDN: m.msisdn, + content: m.content, + })), }, }, - }) + }; + + // Caller-supplied key wins; otherwise derive one from the shipment so that + // an identical resend collapses and a genuinely different send does not. + const txId = transactionId ?? deriveTransactionId(shipment); + + const already = rememberedResult(txId); + if (already) { + return { + ...already, + content: [ + { + type: "text", + text: + `Already sent (transactionId ${txId}). Returning the original result ` + + `rather than sending again.\n\n` + + already.content.map((c) => c.text).join("\n"), + }, + ], + }; + } + + const result = await call("/createShipment", { body: { transactionId: txId, ...shipment } }); + // Only remember a send that actually succeeded — a failure must stay retryable. + if (!result.isError) remember(txId, result); + return result; + } ); + server.registerTool( "get_sms_status", { diff --git a/test-dedup.mjs b/test-dedup.mjs new file mode 100644 index 0000000..40e3764 --- /dev/null +++ b/test-dedup.mjs @@ -0,0 +1,100 @@ +/** + * Proves the send guard: a repeated shipment does not reach the API twice, + * a different shipment does, and a failure stays retryable. + * + * Runs the real server over stdio against a local stand-in for smsOn, so it + * exercises the actual tool handler rather than a copy of its logic. + */ +import http from "node:http"; +import { spawn } from "node:child_process"; + +let sends = 0; +let failNext = false; + +const api = http.createServer((req, res) => { + let body = ""; + req.on("data", (c) => (body += c)); + req.on("end", () => { + if (req.url.startsWith("/createShipment")) { + sends++; + const payload = JSON.parse(body); + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify( + failNext + ? { status: "FAILED", transactionId: payload.transactionId } + : { shipmentUid: "s" + sends, transactionId: payload.transactionId }, + )); + return; + } + res.writeHead(404); res.end("{}"); + }); +}); + +await new Promise((r) => api.listen(0, "127.0.0.1", r)); +const port = api.address().port; + +const child = spawn(process.execPath, ["index.js"], { + env: { ...process.env, + SMSON_TOKEN: "t", SMSON_ENDPOINT: `http://127.0.0.1:${port}`, + SMSON_SYSTEM: "sys", SMSON_SENDER: "TEST" }, + stdio: ["pipe", "pipe", "inherit"], +}); + +let buf = ""; +const pending = new Map(); +child.stdout.on("data", (d) => { + buf += d; + let i; + while ((i = buf.indexOf("\n")) >= 0) { + const line = buf.slice(0, i).trim(); buf = buf.slice(i + 1); + if (!line) continue; + try { const m = JSON.parse(line); if (pending.has(m.id)) { pending.get(m.id)(m); pending.delete(m.id); } } catch {} + } +}); + +let id = 0; +const rpc = (method, params) => new Promise((resolve) => { + const msgId = ++id; + pending.set(msgId, resolve); + child.stdin.write(JSON.stringify({ jsonrpc: "2.0", id: msgId, method, params }) + "\n"); +}); + +await rpc("initialize", { protocolVersion: "2026-07-28", capabilities: {}, clientInfo: { name: "t", version: "0" } }); +child.stdin.write(JSON.stringify({ jsonrpc: "2.0", method: "notifications/initialized" }) + "\n"); + +const send = (args) => rpc("tools/call", { name: "send_sms", arguments: args }); +const MSG = { messages: [{ msisdn: "48600600600", content: "hello" }] }; + +let failures = 0; +const check = (name, ok, detail = "") => { + console.log(`${ok ? "PASS" : "FAIL"} ${name}${detail ? " — " + detail : ""}`); + if (!ok) failures++; +}; + +await send(MSG); +check("first send reaches the API", sends === 1, `sends=${sends}`); + +await send(MSG); +check("an identical retry does NOT send again", sends === 1, `sends=${sends}`); + +const r = await send(MSG); +check("the retry returns the original result", + JSON.stringify(r).includes("Already sent")); + +await send({ messages: [{ msisdn: "48600600600", content: "different text" }] }); +check("a genuinely different message DOES send", sends === 2, `sends=${sends}`); + +await send({ ...MSG, transactionId: "caller-supplied-key-1" }); +check("a caller-supplied key is honoured", sends === 3, `sends=${sends}`); +await send({ ...MSG, transactionId: "caller-supplied-key-1" }); +check("...and repeating it does not resend", sends === 3, `sends=${sends}`); + +failNext = true; +await send({ messages: [{ msisdn: "48600600601", content: "will fail" }] }); +const after = sends; +await send({ messages: [{ msisdn: "48600600601", content: "will fail" }] }); +check("a FAILED send stays retryable", sends === after + 1, `sends=${sends}`); + +child.kill(); api.close(); +console.log(failures ? `\n${failures} check(s) failed` : "\nsend guard: all checks passed"); +process.exit(failures ? 1 : 0);