Skip to content

Prevent duplicate messages and AI replies when sends are retried - #190

Merged
NetworkTheoryAppliedResearchInstitute merged 3 commits into
NTARI-RAND:mainfrom
nirmalhk7:nirmal/agrinet-message-retries
Oct 29, 2026
Merged

NetworkTheoryAppliedResearchInstitute merged 3 commits into
NTARI-RAND:mainfrom
nirmalhk7:nirmal/agrinet-message-retries

Conversation

@nirmalhk7

@nirmalhk7 nirmalhk7 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

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/:conversationId accepts an optional Idempotency-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 returns 409; a send already in progress cannot run a second time. Keys are scoped to the authenticated caller and conversation, not the request's from label.

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 410 instead of recreating them. Requests without a key retain the existing behavior.

Rollout and limits

Provision the separate MessageSends table 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.md records 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

  • All 102 backend tests passed: 56 Node tests and 46 Jest tests. The 19 new HTTP tests cover replay, payload conflicts, concurrent sends, caller isolation, attachments, deletion, storage failures, and browser preflight. Initial retry tests reproduced the duplicate messages and replies before implementation.
  • The DynamoDB Local check passed with two real HTTP worker processes, eight competing retries, completed-response replay after restart, and process death during an AI call. The external AI provider was replaced with a local fixture; no paid AI calls were made.
  • ESLint passed on all changed JavaScript files; git diff --check passed. No dependencies were added.

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
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>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. 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.
  3. 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.
  4. retrySafeMessage.js:81-84. When only the receipt write fails, consider returning the real 201 with a marker header instead of 503.
  5. models/messageSend.js:9 and complete(). Add createdAt and completedAt so stuck receipts can be aged.
  6. 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.
  7. 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
NetworkTheoryAppliedResearchInstitute merged commit 9373310 into NTARI-RAND:main Oct 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants