Skip to content

fix(logs): dedupe a pending entry already persisted to call_logs - #13112

Closed
hartmark wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
hartmark:fix/call-logs-pending-persisted-dedup
Closed

hartmark wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
hartmark:fix/call-logs-pending-persisted-dedup

Conversation

@hartmark

@hartmark hartmark commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

/dashboard/logs occasionally threw a React console error:

Encountered two children with the same key, `<id>`. Keys should be unique
so that components maintain their identity across updates. Non-unique
keys may cause children to be duplicated and/or omitted -- the behavior
is unsupported and could change in a future version.

    at RequestLoggerV2.tsx:1321:23  (<tr key={log.id}>)

Root Cause

buildCallLogListRows() merges three sources into one API response:
in-flight ("pending") requests tracked in memory, recently-completed
in-memory entries, and persisted call_logs rows.

The completed-entries loop already guards against double-counting an id
that had already landed in the persisted rows or the pending map:

for (const detail of completedDetails) {
  if (persistedIds.has(detail.id) || pendingIds.has(detail.id)) continue;
  ...

The pending-entries loop had no equivalent guard. A request that just
finished can be written to call_logs a moment before its in-memory
pending-tracker entry is removed. In that window, the same id appeared in
both activeEntries and logs in a single response -- the exact
duplicate key React reported.

Fix

Mirror the same persistedIds.has(detail.id) guard already used for
completedEntries onto the pendingDetails loop.

Evidence

New regression test proves the id appears exactly once once both sources
report it, and that the persisted row wins (no stale active: true
flag). Confirmed it fails against the pre-fix code (returns 2 rows for
the same id) and passes after (1 row).

$ node --test tests/unit/call-logs-correlation-sort.test.ts
tests 5, pass 5, fail 0

$ node --test tests/unit/call-log-provider-display.test.ts tests/unit/request-logger-endpoints.test.ts
tests 40, pass 40, fail 0
  • eslint: clean (3 pre-existing, unrelated any lint errors in the test
    file, confirmed present on unmodified release/v3.8.51 too -- not
    touched by this PR)
  • tsc --noEmit: no errors in touched files

buildCallLogListRows() merges three sources into one response: in-flight
("pending") requests tracked in memory, recently-completed in-memory
entries, and persisted call_logs rows. The completed-entries loop already
guarded against double-counting an id that had already landed in the
persisted rows or the pending map -- but the pending-entries loop itself
had no equivalent guard against an id that had already been persisted.

A request that just finished can be written to call_logs a moment before
its in-memory pending-tracker entry is removed. In that window, the same
id appeared in both `activeEntries` and `logs` in a single API response,
producing a duplicate React key in the request logger's table:

    Encountered two children with the same key, `<id>`. Keys should be
    unique so that components maintain their identity across updates.

Observed live on /dashboard/logs (RequestLoggerV2.tsx's <tr key={log.id}>).

Fix: mirror the same `persistedIds.has(detail.id)` guard already used for
completedEntries onto the pendingDetails loop.

New regression test (buildCallLogListRows: dedupes a pending in-memory
entry already persisted to the DB) proves the id appears exactly once
once both sources report it, and that the persisted row wins (no stale
`active: true` flag). Confirmed it fails against the pre-fix code (returns
2 rows for the same id) and passes after (1 row).

Evidence:
- node --test tests/unit/call-logs-correlation-sort.test.ts: 5/5 pass
- node --test tests/unit/call-log-provider-display.test.ts tests/unit/request-logger-endpoints.test.ts:
  40/40 pass (unaffected)
- eslint: clean (3 pre-existing unrelated `any` lint errors in the test
  file, confirmed present on unmodified release/v3.8.51 too, not touched)
- tsc --noEmit: no errors in touched files

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@hartmark
hartmark marked this pull request as ready for review September 9, 2026 09:11
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 9, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 9, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 11, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 11, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 13, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 13, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 14, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 14, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
@diegosouzapw

Copy link
Copy Markdown
Owner

Good catch on the asymmetry with the completedEntries guard three lines above — this mirrors
an existing, proven pattern exactly. One thing worth flagging: since you opened this, #13546
landed and changed persistAttemptLogs() to key call_logs rows on traceId instead of the
pendingRequestId your reproduction relies on, so the concrete duplicate-key scenario you
found may no longer trigger through the combo path. That doesn't make the fix wrong — the two
loops are still asymmetric, and this closes that gap defensively at near-zero cost — just
flagging it so the fix is understood as hardening rather than an active production bug going
forward. Your test suite passes clean in isolation (5/5). Looks merge-ready.

@hartmark

Copy link
Copy Markdown
Contributor Author

Confirmed — checked the current release/v3.8.51 tip and persistAttemptLogs() does now key saveCallLog on traceId (#13546), not pendingRequestId. Agreed this reframes it as defensive hardening rather than an active bug through the combo path today; the underlying asymmetry with the completedEntries guard is still real and worth closing regardless. No other changes needed from my side.

hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 15, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 15, 2026
…y persisted to call_logs) into dev/omniroute-dev-combined
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for this — the guard is correct in isolation, and it mirrors the one already applied to the completed entries. Closing because the collision it defends against can no longer happen on the current tip:

The two id spaces are generated independently and never meet, so persistedIds.has(detail.id) in the pending loop cannot be true in production any more. If you can still reproduce the duplicate-key warning in the dashboard on the current release branch, reopen with the two ids from the same request and I will take it straight back — that would mean something else is reusing an id and it is worth knowing.

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.

2 participants