Repository navigation
Prevent duplicate messages and AI replies when sends are retried - #190
Merged
NetworkTheoryAppliedResearchInstitute merged 3 commits intoOct 29, 2026
Conversation
A lost HTTP response currently makes a retry save another message and potentially invoke AI again. Claim a durable, caller-scoped receipt before delivery, replay completed message references, and reject payload conflicts or overlapping sends. Fail closed after uncertain outcomes instead of repeating uploads, broadcasts, or AI calls. Keep chat text out of receipts and prevent deleted conversations from being resurrected by retries. Preserve sends without a key. Add test-first HTTP coverage and a DynamoDB Local two-worker restart/crash check. Document receipt-table provisioning, client adoption, and the limits of retry protection without claiming cross-system exactly-once delivery. Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com>
nirmalhk7
marked this pull request as ready for review
October 25, 2026 23:30
Derive API-key receipt IDs with asynchronous scrypt and a deterministic public scope salt. Avoid storing a fast offline credential verifier while preserving JWT IDs and retry isolation. Add stable derivation, reload, rotation, WIX fallback, mixed-header and concurrent API retry coverage. Exercise API-key workers in the DynamoDB Local restart and crash check. Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com>
Signed-off-by: Nirmal Khedkar <nirmalhk7@gmail.com>
NetworkTheoryAppliedResearchInstitute
approved these changes
Oct 29, 2026
NetworkTheoryAppliedResearchInstitute
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, Nirmal. This is careful, well-tested work, and the scrypt change closes CodeQL alert #14.
What I tested: 80cf53d merged onto main cf9299f (merge ref db5ed88).
- Backend
npm test: 61/61 node --test (your 24 new tests) and 46/46 Jest. No baseline test lost. - Frontend lint, next build, chat-ui tests and vite build: pass, unchanged.
- CodeQL: no new alerts; #14 is fixed.
- I also planted 27 deliberate regressions, and your tests caught 23 of them.
- Your later push, 81c072b, only merges main and keeps both CONFORMANCE.md notes. I checked that the PR's own diff and backend/ are identical to what I tested.
- Not run: the DynamoDB Local check (no Docker here, and no CI job runs it).
None of these block the merge, but please pick them up in a follow-up:
- backend/services/retrySafeMessage.js:44-47. scrypt (about 200-300 ms) runs on every keyed API-key request, replays included, on Node's 4-thread pool, with no rate limit and a public key. Since only one server key exists, a constant principal such as hash(['message-send-v1', ['api-key'], conversationId, key]) gives the same isolation at no cost and keeps #14 closed.
- retrySafeMessage.js:36-37 and docs/retry-safe-messages.md:100-104. The fingerprint is a plain SHA-256 of the message and outlives deletion, so short messages can be guessed. An HMAC keyed by the Idempotency-Key (plus a server secret) fixes it. Please also correct the "deletion erases content" wording.
- retrySafeMessage.js:49, :75, :81. The catch blocks drop the error. Log the receipt id plus error.name and error.message (never the body, headers or key), so a missing MessageSends table or IAM grant shows up in the server logs.
- retrySafeMessage.js:81-84. When only the receipt write fails, consider returning the real 201 with a marker header instead of 503.
- models/messageSend.js:9 and complete(). Add createdAt and completedAt so stuck receipts can be aged.
- Tests:
- add a 409 case for the same filename with different bytes;
- the reorder test can't reach canonical();
- the mock ignores ConditionExpression and ConsistentRead;
- add a timeout to the parallel tests (or --test-timeout in scripts/test.js:6) so a regression fails instead of hanging.
- PR body: please update the counts (61 total, 24 new), link the current docs, and say whether both DynamoDB Local modes (JWT and --api-key) passed at 80cf53d.
Deploy note: create the MessageSends table and its IAM grants before any client sends Idempotency-Key.
Separate from this PR, and worth their own issues:
- Conversations have no owner, and /stream needs no auth.
- Conversation.remove deletes all messages in one BatchWriteItem, which fails above 25 messages after the conversation row is already gone.
- Uploaded files are never deleted.
Approved at 81c072b on Jodson Graves's instruction. Testing was done at 80cf53d merged onto main; 81c072b differs from that only by merging current main (the garden planner from #189 and CONFORMANCE.md), and all checks, CodeQL included, pass on it.
NetworkTheoryAppliedResearchInstitute
merged commit Oct 29, 2026
9373310
into
NTARI-RAND:main
5 checks passed
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
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.
A client can lose its connection after Agrinet saves a message but before the HTTP response arrives. Retrying currently creates another message, repeats its broadcasts, and can generate another AI reply. This change lets clients identify one deliberate send across those retries.
POST /messages/:conversationIdaccepts an optionalIdempotency-Key. A DynamoDB conditional write claims the send before uploads, message persistence, broadcasts, or AI work begin. Completed retries return the original persisted message and reply. Reusing a key with different content returns409; a send already in progress cannot run a second time. Keys are scoped to the authenticated caller and conversation, not the request'sfromlabel.Receipts keep hashes, state, and message references rather than another copy of chat text or credentials. If the original messages were deleted, a retry returns
410instead of recreating them. Requests without a key retain the existing behavior.Rollout and limits
Provision the separate
MessageSendstable and its permissions before enabling the header in clients. Existing Messages keys are unchanged. This PR adds backend support; it does not change frontend clients or enable automatic retries.A worker crash or an unrecorded outcome can leave a send pending. It stays blocked for operator inspection rather than repeating potentially completed side effects. This does not promise exactly-once delivery across the database, files, SSE, and an AI provider. The API and deployment guide explains status codes, retention, required permissions, and recovery limits.
The scope follows Fruitful's communications purpose, the DynamoDB guidance, and Slack's direction to use Janus and the bylaws.
CONFORMANCE.mdrecords the boundary: these are private application receipts, not sealed dialog or commons entries. Reputation, escrow, balances, and existing in-review conformance claims are unchanged.Validation
git diff --checkpassed. No dependencies were added.