send_sms: stop a retry from sending the SMS twice - #2
Open
aurumflux20 wants to merge 1 commit into
Open
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the issue I opened in #1, with the client-side guard rather than the fix I first suggested there.
Why not the fix from the issue
In #1 I suggested making
transactionIddeterministic so the API would collapse a retry. Your README says that won't work —transactionId generuje serwer (UUID) - API go nie nadaje— the API neither assigns nor interprets that field, so there's no basis to assume it deduplicates on it. My original suggestion was wrong and this PR does the other thing: the guard lives in this server.The change
send_smsnow takes an optionaltransactionId, and derives one from the shipment itself when the caller doesn't supply it. A shipment already sent withinSMSON_DEDUP_TTL_MS(default 15 minutes) returns its original result instead of going out again.Two details that matter more than the dedup itself:
FAILEDsend is not remembered, so a transient failure stays retryable rather than becoming permanently un-sendable.Scope, honestly
The record is in-memory. It covers a retry within one running server — an agent re-dispatching seconds later, which is the case people actually hit — and not a restart between the send and the retry. A durable store would close that too; this is the smallest change that fixes the common failure, with no new dependency.
Tests
test-dedup.mjsstarts the real server over stdio against a local stand-in for the API, so it exercises the actual tool handler:node test-dedup.mjs— no credentials needed, exits non-zero on failure.Disclosure
Same as in #1: I maintain open-source exactly-once tooling, so I have a commercial interest in this class of bug. Deliberately no dependency on any of it here — this is ~40 lines of your own code. Take it, change it, or close it; flagging it either way.