fix(web): late callback dedup across invocation boundaries#410
fix(web): late callback dedup across invocation boundaries#410
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2069219c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mindfn
force-pushed
the
fix/late-callback-dedup
branch
2 times, most recently
from
f18fc65 to
fa9c482
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa9c482a65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…vocation boundary Root cause: When invocation_created fired, it cleared finalizedStreamRef, preventing late callbacks (without msg.invocationId) from finding their finalized stream bubble. This caused duplicate message bubbles when: 1. Backend omitted invocationId on callback (scheduler, rich blocks) 2. A new invocation started before the old callback arrived 3. All three lookup paths failed (strict match, invocationless scan, finalized ref) Two-layer fix: - Hook layer: Stop clearing finalizedStreamRef in invocation_created handler. Late callbacks can now find their target via the existing fallback path. Thread switch (resetRefs) still correctly clears all refs. - Store layer: Add TD112 Phase 3 safety net in findAssistantDuplicate — merges callback into recent finalized stream bubble (with invocationId, non-streaming, within 30s, matching visibility/replyTo) as last resort. Tests: 5 new test cases covering the specific gap + Phase 3 constraints. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…y loss (#586 P1+P2) Address gpt52 review findings: P1 — Silent history loss: When inv-2 is callback-only (no stream), its callback would merge into inv-1's finalized bubble via finalizedStreamRef, silently replacing inv-1's content. Fix: invocation_created now "fences" the finalizedStreamRef entry instead of clearing it. findInvocationless- StreamPlaceholder applies a dual guard: time fence (5s grace) + content match. Callbacks with different content are rejected — they create new bubbles instead of overwriting the previous invocation's output. P2 — Background thread parity: Apply the same fence mechanism to finalizedBgRefs in useSocket-background-system-info.ts and useSocket-background.ts. Background threads now use identical time + content guards when matching callbacks across invocation boundaries. Store-level: TD112 Phase 3 (findAssistantDuplicate) also gains a content guard — callbacks with different content won't merge into recent finalized stream bubbles at the store dedup layer. Tests: 6 new/updated regression tests covering both P1 (different content → no merge) and P2 (background path parity) scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…2 inline P1) Empty-string content was skipping the comparison due to truthy checks, allowing silent overwrite when finalized bubble had empty content. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…boundaries [砚砚/GPT-5.4🐾]
mindfn
force-pushed
the
fix/late-callback-dedup
branch
from
2dd346d to
8f4204a
Compare
|
@codex review Rebased onto latest
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f4204a5a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the two new P1s on top of What changed:
Local verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c67618c5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Follow-up on Adjustment:
Local verification:
|
There was a problem hiding this comment.
💡 Codex Review
clowder-ai/packages/web/src/hooks/useAgentMessages.ts
Lines 472 to 473 in ad2f6b1
Skip inferred replacement lock after fenced callback merge
In the callback replacement path, invocationId may be inferred from the current cat state even when the replacement target came from a fenced finalized bubble (i.e., a late callback from a prior invocation). Persisting that inferred ID in replacedInvocationsRef makes shouldSuppressLateStreamChunk drop subsequent stream chunks for the current invocation, so users can lose live output right after the late callback merge. This became reachable with the new finalized-ref fence flow, so replacement suppression should not be locked from inferred IDs in this branch.
clowder-ai/packages/web/src/hooks/useSocket-background.ts
Lines 372 to 373 in ad2f6b1
Avoid inferred suppression lock in bg fenced-merge path
The background callback replacement path has the same issue: replacementTarget.invocationId can be inferred from current thread state when the match came from finalizedBgRefs after fencing, and storing it in replacedInvocations causes shouldSuppressLateBackgroundStreamChunk to drop real stream text for the active invocation. In a late-callback-from-old-invocation scenario, this incorrectly suppresses the new invocation’s stream and can hide output until recovery logic runs.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the latest P1 on Local verification:
This patch prevents fenced fallback callback merges from writing an inferred replacement lock for the new invocation, while preserving callback-first late-stream suppression when the replacement matched by invocation ID. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4c01ad35a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the latest P2s on Local verification:
This keeps inferred suppression only for exact matches or active invocationless placeholders, while fenced finalized fallback merges still avoid locking the new invocation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13fb8eff9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the latest P2 on Local verification:
Background callback replacement now skips invocationless fallback matching whenever the callback carries an explicit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0b17dc85a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the latest P1 on What changed:
Local verification:
|
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a81c8f1fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the latest P2s on What changed:
Local verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf1bb5919
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the latest P2 on What changed:
Local verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed28553a3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Addressed the latest P2 on What changed:
Local verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9bcc669d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Latest fixes are in |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a033cd227a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Hi @mindfn — thanks for the deep investigation and the thorough regression coverage. The race condition you've identified is real: clearing That said, we think the fix can be approached more cleanly at a different layer. Why we're hesitant about the current approachThe PR adds four new coordination mechanisms ( What we think is a better pathOption A — Protocol-level fix (preferred): The backend callback routes already carry This is a one-time backend change that eliminates a whole class of client-side race conditions rather than fencing them case by case. Option B — Lightweight client-side alternative (if backend change is deferred): Instead of fencing // On done(isFinal): record invocationId → messageId with auto-expiry
const LATE_CALLBACK_GRACE_MS = 5_000;
finalizedStreamRef.current.set(catId, { messageId, invocationId });
setTimeout(() => {
const entry = finalizedStreamRef.current.get(catId);
if (entry?.messageId === messageId) finalizedStreamRef.current.delete(catId);
}, LATE_CALLBACK_GRACE_MS);
What we'd love to keepYour regression test cases are excellent — they precisely describe real race conditions ( Suggested next stepCould you open a dedicated issue describing the specific race condition (late callback after Thanks again for the thorough work — this is clearly a well-understood problem, and we appreciate the contribution. 🐾 |
|
Thanks — I agree the protocol-level fix is the cleaner long-term direction. I opened a dedicated issue to track that root cause and invariant here: #454. The main reason this PR went down the client-side path was to stop the user-visible duplicate-bubble race without waiting on a backend contract change. Regardless of which implementation path we choose, I think the regression scenarios captured here are still valuable and should be preserved. At this point I see two reasonable options:
I'm fine with either direction; the key thing is that we now have a focused issue that tracks the actual race condition separately from #266. |
|
Thanks for the clean follow-up and for opening #454 — that's exactly the right anchor. Our decision: we'll pursue the protocol-level fix via #454 and won't merge this PR. Reasoning:
We've triaged #454 as Thanks again for the thorough analysis — the issue quality and willingness to pivot to the right fix layer is exactly the kind of contribution we value. 🐾 |
|
Understood. I won't push the client-side mitigation on this branch any further. I'll treat #410 as superseded by #454, and the regression scenarios from this PR can be carried over there as red-light fixtures for the protocol-level implementation. Closing this PR so the tracking state matches the agreed direction. |
2 participants
Summary
Scope
packages/web/src/hooks/useAgentMessages.tspackages/web/src/hooks/useSocket-background.ts,packages/web/src/hooks/useSocket-background-system-info.ts,packages/web/src/hooks/useSocket.ts,packages/web/src/hooks/useSocket-background.types.tspackages/web/src/hooks/reply-target-compat.tspackages/web/src/stores/chatStore.ts,packages/web/src/stores/chat-types.tspackages/web/src/hooks/__tests__/useAgentMessages-late-callback-dedup.test.ts,packages/web/src/hooks/__tests__/useSocket-background.test.ts,packages/web/src/stores/__tests__/chatStore-multithread.test.tsRelated
fix(hub): duplicate message rendering in console)always propagate callback invocationId to avoid late-callback duplicate bubbles)#586/issue-586-state-trifectalabels, but this repo does not have a GitHub issue#586; this PR does not close oneVerification
pnpm checkpnpm --filter @cat-cafe/web exec vitest run src/hooks/__tests__/useAgentMessages-late-callback-dedup.test.ts src/hooks/__tests__/useSocket-background.test.ts src/stores/__tests__/chatStore-multithread.test.ts