Skip to content

fix: give RPC log events stable ids so replayed/re-ingested rows don't duplicate - #3633

Merged
chelojimenez merged 3 commits into
mainfrom
fix/duplicate-request-logging
Aug 2, 2026
Merged

fix: give RPC log events stable ids so replayed/re-ingested rows don't duplicate#3633
chelojimenez merged 3 commits into
mainfrom
fix/duplicate-request-logging

Conversation

@chelojimenez

@chelojimenez chelojimenez commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Symptom

Single JSON-RPC exchanges rendered as multiple identical rows in the traffic log panel. Captured against stateless.mcpjam.com/mcp on protocol 2026-07-28: one tools/call for run-task produced two byte-identical SEND entries, two byte-identical HTTP entries, and two RECEIVE entries — all sharing the same Cloudflare ray id a2504a801d1e99a0 and the same JSON-RPC id: 14. One cf-ray and one JSON-RPC id proves this was a single physical request; the duplication was purely client-side.

The duplication was selectivetasks/get ids 15–17 and subscriptions/listen in the same session appeared once each.

Root cause

Not a doubled emitter and not a React effect running twice.

The local Logs SSE route seeds every new connection with the tail of the in-process replay buffer (replay=3). Nothing in that delivery carried an identity: the bus stored bare {serverId, direction, timestamp, message} objects and the browser store minted a random id per received event. So any re-subscribe re-ingested the tail as brand-new rows.

subscribeToRpcStream is refcounted — when the last LoggerView unmounts the EventSource is closed and nulled, and the next mount reopens with replay=3 again. onerror does the same. The store is a singleton that survives those remounts, so replayed events landed on top of copies it already held.

This explains every detail: duplicates were byte-identical including cf-ray and durationMs because they were literally the same buffered object replayed; and it was selective because one exchange emits exactly ~3 bus events (send frame, HTTP exchange, receive frame), which is exactly the replay depth — only events inside that window at reconnect time were affected.

Fix

Give every published event a stable identity — eventId on the wire — and let the store's already-existing id-keyed upsert do its job.

  • server/services/rpc-log-event-id.ts (new) — nextRpcLogEventId(): process nonce + counter, rpc:<nonce>:<n>. The nonce prevents id reuse across a restart.
  • server/services/rpc-log-bus.tspublish() stamps eventId via a generic stampEventId helper (type-checked, no cast, and it preserves the concrete union member); buffer and subscribers carry it.
  • server/routes/mcp/servers.ts — the SSE route spreads the event onto the wire, so eventId reaches the browser unchanged.
  • client/src/stores/traffic-log-store.ts — the SSE handler passes data.eventId into addMcpServerLog, whose existing id-keyed branch updates the row instead of appending.

The wire field is deliberately eventId, not id — it travels next to the JSON-RPC message.id, and shared/hosted-rpc-log.ts is a cross-repo shape where claiming the generic id name would collide if the backend ever adds its own. The browser store's own pre-existing row key (McpServerRpcItem.id) is a separate concept and is unchanged; eventId feeds into it.

Why replay was kept

Replay is the only recovery for events missed while an EventSource was down. Removing it would trade duplicates for silent loss. Keying the delivery is the emission-level fix and it also covers reconnects and multiple tabs.

Hosted mode

Extended to the hosted delivery path using the same id scheme (the minter was extracted precisely so hosted reuses it rather than inventing a second mechanism).

  • shared/hosted-rpc-log.tseventId?: added to HostedRpcLogEvent / HostedHttpLogEvent. Optional deliberately: the type guards check required fields only, so events from an older backend still validate and still append. No deploy ordering constraint, no dropped rows.
  • server/routes/web/hosted-rpc-logs.tsHostedRpcLogCollector stamps the id at capture, not at delivery, so both deliveries read the same buffered event.
  • client/src/stores/traffic-log-store.tsingestHostedRpcLogs / ingestHostedHttpLogs pass eventId into the same keyed upsert.

This closes a real latent duplication, not just a defensive gap: flushBufferedLogs drops the writer when a stream write throws and falls back to envelope delivery, and buildEnvelope() returns all buffered logs — including those already streamed as data-rpc-log / data-http-log parts. Before this change, a mid-turn stream failure duplicated every event streamed up to that point. Now the streamed part and the envelope copy carry the same id and fold onto one row.

Known limit of the test coverage

The route test stops at the serialized SSE frame; the browser's real EventSource + JSON.parse are not exercised end to end, and the client tests pick up from an already-parsed object. The two suites therefore meet at a hand-agreed field name rather than a shared type — a client-side-only typo (reading data.eventID) would be caught by the client tests, but nothing structurally binds the two halves. Standing up a live listening server for one field name was more harness than this warrants; both sides now assert on it explicitly, and both send sites carry a comment naming the guarding test.

Explicit non-goal: collapsing legitimately-distinct rows

The id is minted per physical published event — never per method, never per JSON-RPC id. Multi-round MRTR flows and retries send several real requests for one user action and must still each get their own row. Tests assert this directly, including a retry that reuses a JSON-RPC id and a second MRTR round.

Verification

  • npm run typecheck:client → clean.
  • npx tsc --noEmit -p server/tsconfig.json → 90 errors, exactly matching the unmodified baseline (stash-and-compare), none in any touched file. All pre-existing on main.
  • npx vitest run → 1037 files passed, 2 skipped; 11685 tests passed, 6 skipped.
  • Both regression tests confirmed to fail without the fix (stashing only traffic-log-store.ts): local expect(items).toHaveLength(3) fails, hosted re-ingesting the same batch keeps one row per event fails.
  • No lint script exists in this package, so lint was not run. shared/hosted-rpc-log.ts reports a prettier warning that is pre-existing on the baseline.

Tests added

  • server/routes/mcp/__tests__/rpc-stream-event-id.test.ts — mounts the real router and parses the real serialized SSE data: frames, so the send({ type, ...evt }) spread executes for real rather than being fabricated. Covers both send call sites separately (replay seeding with ?replay=2, live subscribe with ?replay=0), that a replayed frame keeps the eventId it had when live, and that identical-looking frames get distinct ones. Confirmed to fail on the refactor it exists to guard: rewriting the spread to explicit fields fails all 4.
  • server/services/__tests__/rpc-log-bus.test.ts — ids stamped, survive replay identically, identical-looking frames get distinct ids.
  • client/src/stores/__tests__/traffic-log-store.rpc-stream.test.ts — fake EventSource drives the real handler: a remount replaying 3 events leaves 3 rows; 5 distinct exchanges stay 5; id-less events append.
  • server/routes/web/__tests__/hosted-http-logs.test.ts — streamed ids and envelope ids are the same set (the fallback overlap); concurrent collectors never reuse an id.
  • client/src/stores/__tests__/traffic-log-store.hosted.test.ts — mirrors the local trio for hosted ingest.

🤖 Generated with Claude Code

chelojimenez and others added 2 commits August 2, 2026 15:04
The Logs panel seeds every new SSE connection from the bus replay buffer
(`/api/mcp/servers/rpc/stream?replay=3`). Nothing in that delivery carried
an identity, and the browser store minted a random id per received event,
so any RE-subscribe — the panel unmounting and remounting, an EventSource
reconnect, a second tab — re-ingested the tail of the buffer as brand-new
rows. One physical exchange then rendered twice: the same `tools/call`
send frame, the same HTTP exchange (identical cf-ray and durationMs) and
the same response, byte for byte. It looked selective because only the
events inside the replay window at reconnect time were affected.

`RpcLogBus.publish` now stamps each event with a process-unique id. The
SSE route already spreads the event onto the wire, so the id reaches the
browser, where the store passes it to `addMcpServerLog` — whose existing
keyed upsert then updates the row instead of appending a copy.

The key is one id per PUBLISHED event, never a method or JSON-RPC id, so
retries and multi-round MRTR flows keep a row each. Events without an id
(older server) still append, as before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes the hosted half of the duplicate-row gap. Hosted delivery carried
no per-event identity either, so `ingestHostedRpcLogs` minted a random id
per received event and any re-ingestion appended copies. That is not
hypothetical: `HostedRpcLogCollector` streams events as `data-rpc-log` /
`data-http-log` parts, but a stream write that fails mid-turn drops the
writer and falls back to envelope delivery — and the envelope carries the
ALREADY-STREAMED events too.

Both paths now mint from one place (`nextRpcLogEventId`), so there is a
single id scheme rather than two: the bus stamps at publish, the hosted
collector stamps at CAPTURE (not at delivery), so an event that is
streamed and then repeated in the envelope carries the same id both times
and folds onto one row.

`id` is optional on the shared hosted shapes, matching every other
addition across that repo boundary: a backend that predates it sends
none, the client appends as it always did, and neither side has to deploy
first.

Still one id per PHYSICAL event, never per method or JSON-RPC id — a
retry and each MRTR round keep a row each, now asserted on the hosted
path too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Aug 2, 2026
@cursor

cursor Bot commented Aug 2, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_dffbd97a-a8d7-4e37-8861-7f812622f4e2)

@dosubot

dosubot Bot commented Aug 2, 2026

Copy link
Copy Markdown

📄 Knowledge review

Dosu skipped reviewing this PR because your organization has used its 200 included credits for the month. Your usage will reset on 2026-09-01. To have Dosu review this PR before then, ask your organization admin to upgrade to a pro account.


Leave Feedback Ask Dosu about inspector Add Dosu to your team

@chelojimenez

chelojimenez commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Internal preview

Preview URL: https://mcp-inspector-pr-3633.up.railway.app
Deployed commit: 6400de6
PR head commit: 3ed9536
Backend target: staging fallback.
Health: ✅ Convex reachable
Access is employee-only in non-production environments.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

2 issues found across 10 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="mcpjam-inspector/server/services/rpc-log-event-id.ts">

<violation number="1" location="mcpjam-inspector/server/services/rpc-log-event-id.ts:24">
P2: Distinct events can still receive the same id when two processes happen to generate the same short `Math.random` nonce, contradicting the globally unique-id contract and causing rows to be overwritten. Use a cryptographically generated process nonce such as `crypto.randomUUID()` or sufficiently long `randomBytes` output instead.</violation>
</file>

<file name="mcpjam-inspector/shared/hosted-rpc-log.ts">

<violation number="1" location="mcpjam-inspector/shared/hosted-rpc-log.ts:37">
P2: Malformed hosted IDs pass the shared event guards and are silently treated as legacy id-less events, so the same captured event can be appended twice instead of deduplicated. Validate an optional ID as `undefined` or a non-empty string in both guards before handing the event to ingestion.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

* reconnecting browser must not mistake a brand-new frame for one it already
* rendered) and two hosted instances serving the same session cannot collide.
*/
const EVENT_ID_NONCE = Math.random().toString(36).slice(2, 10);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Distinct events can still receive the same id when two processes happen to generate the same short Math.random nonce, contradicting the globally unique-id contract and causing rows to be overwritten. Use a cryptographically generated process nonce such as crypto.randomUUID() or sufficiently long randomBytes output instead.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/server/services/rpc-log-event-id.ts, line 24:

<comment>Distinct events can still receive the same id when two processes happen to generate the same short `Math.random` nonce, contradicting the globally unique-id contract and causing rows to be overwritten. Use a cryptographically generated process nonce such as `crypto.randomUUID()` or sufficiently long `randomBytes` output instead.</comment>

<file context>
@@ -0,0 +1,30 @@
+ * reconnecting browser must not mistake a brand-new frame for one it already
+ * rendered) and two hosted instances serving the same session cannot collide.
+ */
+const EVENT_ID_NONCE = Math.random().toString(36).slice(2, 10);
+let eventIdCounter = 0;
+
</file context>

export type HostedLogEventId = string;

export interface HostedRpcLogEvent {
id?: HostedLogEventId;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Malformed hosted IDs pass the shared event guards and are silently treated as legacy id-less events, so the same captured event can be appended twice instead of deduplicated. Validate an optional ID as undefined or a non-empty string in both guards before handing the event to ingestion.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/shared/hosted-rpc-log.ts, line 37:

<comment>Malformed hosted IDs pass the shared event guards and are silently treated as legacy id-less events, so the same captured event can be appended twice instead of deduplicated. Validate an optional ID as `undefined` or a non-empty string in both guards before handing the event to ingestion.</comment>

<file context>
@@ -15,7 +15,26 @@ export interface HostedRpcLogPluginOrigin {
+export type HostedLogEventId = string;
+
 export interface HostedRpcLogEvent {
+  id?: HostedLogEventId;
   serverId: string;
   serverName: string;
</file context>

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@chelojimenez, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d5d4ff44-9791-4cca-9448-99380cf43ac3

📥 Commits

Reviewing files that changed from the base of the PR and between 1dcadef and 3ed9536.

📒 Files selected for processing (10)
  • mcpjam-inspector/client/src/stores/__tests__/traffic-log-store.hosted.test.ts
  • mcpjam-inspector/client/src/stores/__tests__/traffic-log-store.rpc-stream.test.ts
  • mcpjam-inspector/client/src/stores/traffic-log-store.ts
  • mcpjam-inspector/server/routes/mcp/__tests__/rpc-stream-event-id.test.ts
  • mcpjam-inspector/server/routes/mcp/servers.ts
  • mcpjam-inspector/server/routes/web/__tests__/hosted-http-logs.test.ts
  • mcpjam-inspector/server/routes/web/hosted-rpc-logs.ts
  • mcpjam-inspector/server/services/__tests__/rpc-log-bus.test.ts
  • mcpjam-inspector/server/services/rpc-log-bus.ts
  • mcpjam-inspector/shared/hosted-rpc-log.ts

Walkthrough

The change adds optional IDs to hosted log event types and unique IDs to RPC bus and hosted capture events. IDs persist across buffering, replay, streaming, and envelope delivery. Client ingestion forwards IDs for keyed updates while retaining ID-less events independently. Regression tests cover duplicate replay, distinct physical exchanges, identical-looking events, stable IDs, and separate collectors.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@mcpjam-inspector/server/services/rpc-log-event-id.ts`:
- Around line 24-29: Update the EVENT_ID_NONCE initialization used by
nextRpcLogEventId() to derive a process-scoped unique value rather than the
short Math.random() base-36 nonce. Preserve the existing counter and
rpc:${nonce}:${counter} format, ensuring IDs remain unique across processes and
restarted sessions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d2d86e7a-49ab-40ba-9c2d-c3d8684e73f1

📥 Commits

Reviewing files that changed from the base of the PR and between b65bc5e and 1dcadef.

📒 Files selected for processing (10)
  • mcpjam-inspector/client/src/stores/__tests__/traffic-log-store.hosted.test.ts
  • mcpjam-inspector/client/src/stores/__tests__/traffic-log-store.rpc-stream.test.ts
  • mcpjam-inspector/client/src/stores/traffic-log-store.ts
  • mcpjam-inspector/server/routes/mcp/servers.ts
  • mcpjam-inspector/server/routes/web/__tests__/hosted-http-logs.test.ts
  • mcpjam-inspector/server/routes/web/hosted-rpc-logs.ts
  • mcpjam-inspector/server/services/__tests__/rpc-log-bus.test.ts
  • mcpjam-inspector/server/services/rpc-log-bus.ts
  • mcpjam-inspector/server/services/rpc-log-event-id.ts
  • mcpjam-inspector/shared/hosted-rpc-log.ts

Comment on lines +24 to +29
const EVENT_ID_NONCE = Math.random().toString(36).slice(2, 10);
let eventIdCounter = 0;

export function nextRpcLogEventId(): string {
eventIdCounter += 1;
return `rpc:${EVENT_ID_NONCE}:${eventIdCounter}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Confirm the repository's declared Node.js and TypeScript targets support node:crypto randomUUID.
fd -HI -t f -g 'package.json' -g 'tsconfig*.json' . -0 |
  xargs -0 -r rg -n -C 2 '"engines"|"target"|"`@types/node`"'

# Inspect the ID generator after applying the change.
nl -ba mcpjam-inspector/server/services/rpc-log-event-id.ts | sed -n '20,34p'

Repository: MCPJam/inspector

Length of output: 50375


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '--- mcpjam-inspector package metadata ---\n'
sed -n '1,16p' mcpjam-inspector/package.json

printf '\n--- target/related TypeScript files ---\n'
fd -HI -t f 'tsconfig.*\.json$|vite.*\.config.*\.(ts|js|mjs|cjs)$|eslint.*\.(mjs|cjs|js|json)$' mcpjam-inspector mcpjam-inspector/server -x sh -c 'echo "### $1"; sed -n "1,120p" "$1"' sh {}

printf '\n--- current rpc-log-event-id generator ---\n'
nl -ba mcpjam-inspector/server/services/rpc-log-event-id.ts | sed -n '1,40p'

printf '\n--- usages of nextRpcLogEventId / rpc-log-event-id ---\n'
rg -n "nextRpcLogEventId|rpc-log-event-id|EVENT_ID_NONCE|eventIdCounter" mcpjam-inspector/server mcpjam-inspector -g '!node_modules'

Repository: MCPJam/inspector

Length of output: 245


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- mcpjam-inspector package metadata ---'
sed -n '1,16p' mcpjam-inspector/package.json

printf '%s\n' ''
printf '%s\n' '--- target/related TypeScript config files ---'
fd -HI -t f '^(tsconfig.*\.json$|vite.*\.config.*\.(ts|js|mjs|cjs)$|eslint.*\.(mjs|cjs|js|json)$)' mcpjam-inspector mcpjam-inspector/server -x sh -c 'echo "### $1"; sed -n "1,120p" "$1"' sh {}

printf '%s\n' ''
printf '%s\n' '--- current rpc-log-event-id generator ---'
nl -ba mcpjam-inspector/server/services/rpc-log-event-id.ts | sed -n '1,40p'

printf '%s\n' ''
printf '%s\n' '--- usages of nextRpcLogEventId / rpc-log-event-id ---'
rg -n "nextRpcLogEventId|rpc-log-event-id|EVENT_ID_NONCE|eventIdCounter" mcpjam-inspector/server mcpjam-inspector -g '!node_modules'

Repository: MCPJam/inspector

Length of output: 29271


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- current rpc-log-event-id generator ---'
awk 'NR>=1 && NR<=40 { printf "%5d  %s\n", NR, $0 }' mcpjam-inspector/server/services/rpc-log-event-id.ts

printf '%s\n' ''
printf '%s\n' '--- usages of nextRpcLogEventId / rpc-log-event-id ---'
rg -n "nextRpcLogEventId|rpc-log-event-id|EVENT_ID_NONCE|eventIdCounter" mcpjam-inspector/server mcpjam-inspector -g '!node_modules'

printf '%s\n' ''
printf '%s\n' '--- deterministic behavior probe: current formatter + random-collision model ---'
node - <<'JS'
const base = 36;
const len = 8;
const values = Math.pow(base, len);
const samples = 200_000;
let collisions = 0;
const nonce = Math.random().toString(36).slice(2, 10);
for (let c = 1; c <= 32768; c++) {
  const id = `rpc:${nonce}:${c}`;
  if (!id.startsWith('rpc:0:') && nonce.length !== len) {
    process.exitCode = 1;
    break;
  }
}
function sample(n) {
  const seen = new Set();
  let hits = 0;
  while (n--) {
    const v = Math.floor(Math.random() * values);
    if (seen.has(v)) hits++;
    seen.add(v);
  }
  return hits;
}
for (let i = 0; i < samples; i++) collisions += sample(2);
console.log(`base-36 ${len}-char space=${values} exact collision probability ~1-${numbersToString(values)}`);
console.log(`modeled duplicate nonce hits in 2-process sample: ${collisions} of ${samples} runs`);
JS

function numbersToString(val) {
  const s = `${val}`.split('');
  let out = '';
  for (let i = 0; i < s.length; i++) out += i && i % 3 === 0 ? s[i] + ',' : s[i];
  return out;
}

Repository: MCPJam/inspector

Length of output: 5086


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- short deterministic Math.random collision probe ---'
node - <<'JS'
const counts = {};
let iterations = 0;
let collisions = 0;
while (true) {
  iterations++;
  const nonce = Math.random().toString(36).slice(2, 10);
  if (counts[nonce]++) {
    collisions++;
    console.log(`${iterations} iterations produced ${collisions} collision(s); sample nonces: ${[...new Set(Object.keys(counts))].slice(0, 30).join(', ')}`);
    break;
  }
  if (iterations % 20_000_000 === 0 || iterations > 400_000_000) {
    console.log(`${iterations} iterations, ${collisions} collisions, unique nonces=${Object.keys(counts).length}`);
  }
}
JS

Repository: MCPJam/inspector

Length of output: 146


Replace the short Math.random() nonce with a process-scoped unique value.

The 8-character base-36 nonce can collide across processes, and the counter only guarantees uniqueness inside one process. A collision lets different instances or restarted sessions reuse traffic-log nextRpcLogEventId() values and merge distinct browser traffic-log rows.

Proposed fix
+import { randomUUID } from "node:crypto";
+
-const EVENT_ID_NONCE = Math.random().toString(36).slice(2, 10);
+const EVENT_ID_NONCE = randomUUID();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const EVENT_ID_NONCE = Math.random().toString(36).slice(2, 10);
let eventIdCounter = 0;
export function nextRpcLogEventId(): string {
eventIdCounter += 1;
return `rpc:${EVENT_ID_NONCE}:${eventIdCounter}`;
import { randomUUID } from "node:crypto";
const EVENT_ID_NONCE = randomUUID();
let eventIdCounter = 0;
export function nextRpcLogEventId(): string {
eventIdCounter += 1;
return `rpc:${EVENT_ID_NONCE}:${eventIdCounter}`;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mcpjam-inspector/server/services/rpc-log-event-id.ts` around lines 24 - 29,
Update the EVENT_ID_NONCE initialization used by nextRpcLogEventId() to derive a
process-scoped unique value rather than the short Math.random() base-36 nonce.
Preserve the existing counter and rpc:${nonce}:${counter} format, ensuring IDs
remain unique across processes and restarted sessions.

Source: Linters/SAST tools

Review follow-ups on the traffic-log identity work.

Test gap (the important one): nothing asserted that the SSE route actually
puts the identity on the wire. `send({ type, ...evt })` is the only link
carrying it from the bus to the browser, and the client tests fabricated
the wire object by hand — so a refactor picking fields explicitly would
have reintroduced duplicate rows with every test still green. The new
route test drives the REAL Hono route and parses the REAL serialized
`data:` frames, covering both `send` call sites (replay seeding and live
subscription). Verified it catches the refactor it exists for: patching
the spread to explicit fields fails 3 of its 4 cases.

Rename `id` -> `eventId` on the wire and on the shared hosted shapes. A
bare `id` sat next to the JSON-RPC `message.id` and claimed the most
generic field name on a shape the backend also consumes. Nothing consumes
it yet, so this is free now and never will be again. The store's own row
key (`McpServerRpcItem.id`, `addMcpServerLog({ id })`) is a different,
pre-existing concept and is deliberately unchanged — the wire `eventId`
still feeds it.

Also: document the last-write-wins invariant at the store upsert (same
eventId is expected to mean byte-identical, and enrichment must happen at
capture), and replace the `as DeliveredRpcLogEvent` cast in publish with
a generic `stampEventId` helper so the one line that establishes identity
is type-checked rather than asserted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 2, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_9a27e3df-d8f8-4846-937e-edf73e573274)

@chelojimenez
chelojimenez merged commit 1c88b8a into main Aug 2, 2026
13 checks passed
@chelojimenez
chelojimenez deleted the fix/duplicate-request-logging branch August 2, 2026 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant