fix(sdk): retain failed chat notifications for replay - #4070
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: 69f80c0b19
ℹ️ 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".
| attached.generation, | ||
| ); | ||
| } catch (error) { | ||
| if (seq !== undefined && ownsSequence && attached.cursor.seq === seq) attached.cursor.seq = cursorBeforeFrame; |
There was a problem hiding this comment.
When a notification rejects while a later frame is already queued, enqueueFrame() deliberately continues with that frame through previous.catch(...); this rollback therefore lasts only until the later frame advances the cursor beyond the failed sequence. The next reconnect then requests replay from the newer cursor and permanently omits the failed notification, including a potentially critical action_needed event. The failure must prevent subsequent cursor advancement—such as by retiring/fencing the attachment—rather than merely moving the cursor backward.
Useful? React with 👍 / 👎.
| attached.generation, | ||
| ); | ||
| } catch (error) { | ||
| if (seq !== undefined && ownsSequence && attached.cursor.seq === seq) attached.cursor.seq = cursorBeforeFrame; |
There was a problem hiding this comment.
If Slack or Discord accepted an ordinary notification but the provider response or immediate reconciliation failed, notify() can reject with the original durable effect left uncertain even though the message exists remotely. Rolling back here causes replay to call notify() again, but both daemons allocate a fresh random effect identity for non-action notifications, so the original effect can reconcile while the replay publishes a second copy. Replay needs an occurrence identity derived from the event generation/sequence, or it must distinguish ambiguous delivery from a definite pre-send failure.
Useful? React with 👍 / 👎.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Independent red-team review — PR #4070
Head reviewed: 69f80c0b19d1805d381524fd6ebde72a61345df2 (exact match, re-fetched)
Base: 95c00d09e7f68fc39469c185a4019385f331676b (dev)
Diff: 2 files, +56/−17 (1 commit)
Verdict: REQUEST_CHANGES
(Posted as COMMENT because GitHub does not allow a repo owner to submit REQUEST_CHANGES on their own PR. The terminal verdict below is REQUEST_CHANGES.)
The fix is correct for the single-frame scenario its test covers, but demonstrably incomplete for the concurrent-frame case the PR description explicitly claims to handle ("cover reconnect replay after a transient notifier rejection"). I reproduced the data-loss path independently.
P0 — A later queued frame permanently defeats the cursor rollback (data loss)
Confirmed by reproduction at this exact head.
enqueueFrame() (chat-daemon-runtime.ts:976-989) chains frames with previous.catch(() => undefined).then(...) — the .catch deliberately swallows the rejection so the next frame runs regardless. When a notification fails:
- Frame at
seq=1:notify()rejects → catch block rollscursor.seqback from 1 to 0 →throw errorrejectshandleFrame. - The queued frame at
seq=2runs (.catch(() => undefined)absorbs the rejection), itsnotify()succeeds, andcursor.seqadvances from 0 → 2. - Reconnect asks for
sinceSeq: 2→ seq=1 is permanently skipped, including a criticalaction_needednotification.
I reproduced this with an adversarial test in the existing harness (chat-daemon-session-reconnect.test.ts):
ADVERSARIAL RESULT: {
"postedBeforeReconnect": [],
"firstNotificationLostBeforeReconnect": true,
"replayRequestsAfterReconnect": [
{"sinceGeneration":4,"sinceSeq":0},
{"sinceGeneration":4,"sinceSeq":2} ← seq=1 skipped
],
"sinceSeqAfterReconnect": 2,
"firstNotificationSkippedByReplay": true,
"PERMANENT_LOSS": true
}
The rollback is only effective when no later frame is in flight — exactly the narrow case the PR test exercises. Any interleaving (the common production case) silently re-introduces the bug this PR fixes. The new test passes only because it emits a single frame, waits, then reconnects; it never races a second frame.
This mirrors and confirms the Codex P1. The correct fix is to fence/retire the attachment on notification failure (so no subsequent frame advances the cursor), as #failBarrier already does for replay failures — not to roll the cursor backward and hope nothing runs behind it.
P2 — Replay after rollback can publish a duplicate notification
Both daemons allocate a fresh random effect identity for non-action notifications:
- Slack:
notification:${sessionId}:${this.#randomId()}(slack-daemon.ts:610), with a randomclientMsgId(:614) - Discord:
notification:${threadId}:${randomUUID()}(discord-daemon.ts:329)
If a provider accepted a post but the response/reconciliation was ambiguous (the journal records it as "uncertain", slack-daemon.ts:1943-1971), the original message may exist remotely. Rolling back the cursor and replaying calls notify() again with a different journal effect id and client_msg_id, so the durable reconciliation (findMessageByClientMsgId) cannot match the original → a second copy is published. For the action_needed path this is mitigated (stable effectId = action:${sessionId}:${actionId}:${clientMsgId}), but ordinary notifications have no occurrence identity.
This is partly pre-existing, but the rollback amplifies its blast radius by making replay more frequent.
What is correct
- The narrow rollback logic (
:1102-1104) is mechanically sound for a single-frame sequence. - The test harness is well-constructed.
- Biome passes. TypeScript shows only the pre-existing
docs-index.generatederror (documented by the author).
Verification performed at this head
bun test ...chat-daemon-session-reconnect.test.ts --test-name-pattern "notification failure"→ 1 pass (the PR new test)bun test ...chat-daemon-session-reconnect.test.ts→ 17 pass / 1 fail (the 256-frame host-ring test times out waiting for its third socket — the same known flake the PR author reported)bun --cwd=packages/coding-agent run check→ Biome clean; only the pre-existingdocs-index.generatedTS2307- Independent adversarial reproduction of the P0 data-loss path (above)
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
69f80c0 to
2fba17b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fba17b29b
ℹ️ 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".
| await awaitRefusals(provider, 1); | ||
| await Bun.sleep(50); |
There was a problem hiding this comment.
On a slow filesystem or loaded CI runner, awaitRefusals() only proves that FakeSlackProvider.postMessage() appended to refused; the Slack daemon still awaits its journal update before the error reaches #failDelivery() and marks the attachment failed. If that work exceeds this arbitrary 50 ms delay, reconcile() retains the old attachment, and the subsequent replay is discarded when the delayed failure retires it, making the test time out intermittently. Wait for the publication-failure warning or another runtime-level completion signal before reconciling.
Useful? React with 👍 / 👎.
9c3b8b9 to
08b88a7
Compare
|
The current head Integration is bounded-held behind active #4098 because both change — |
Independent exact-head review — REQUEST_CHANGESHead reviewed: The delivery contract itself is sound: a failed sequenced publication calls However, the exact-head full file suite is nondeterministic and therefore not merge-ready. It failed twice across three runs, once in Required change: make this scenario and the shared fake transport/runtime fixture deterministic when the complete file runs (for example, serialize the tests that mutate the shared transport state or eliminate that shared state), then show repeated full-suite passes on the new exact head. Review doctrine completed: author is repository owner/admin; the PR has no external-branch modification path; all exact-head GitHub checks are terminal (14 success, 5 intentionally skipped); full diff, source delivery/fail-closed contract, ancestry, prior automated/Codex findings, and review comments were inspected. No LGTM while the exact-head suite flakes. — |
|
Independently verified on current What I didApplied this PR's only changed file (
Mutation check — the test is load-bearingThis is test-only, so the question is whether it actually pins production behavior. I removed the retirement call in // chat-daemon-runtime.ts:931 — replaced
this.#failBarrier(attached, `publication failed at seq ${seq} (${reason})`);
// with a bare logger.warn carrying the same textThat keeps the Result: 21 pass / 3 fail, with the new test among the failures. Restoring So the test genuinely pins the behavior described in the comment — that only retiring the attachment prevents the queued frame's cursor from advancing past the undelivered sequence — and it is not satisfied by the rollback a naive fix would apply. NoteNeeds a rebase before merge for exact-head CI. The test itself is correct against current |
08b88a7 to
b2bd19d
Compare
The attached-session cursor advanced before notification delivery completed, so a transient notifier rejection permanently skipped the frame on reconnect.\n\nResetting the cursor on a failed delivery keeps the frame eligible for the next replay.\n\nLore-id: 4063\nConstraint: preserve filtering behavior for non-deliverable frames\nTested: bun test packages/coding-agent/test/chat-daemon-session-reconnect.test.ts --test-name-pattern 'notification failure'\nNot-tested: full reconnect suite has a pre-existing 256-frame host-ring timeout\nConfidence: high\nScope-risk: narrow
b2bd19d to
f0900db
Compare
2 participants
Summary
Fixes #4063 (the confirmed #4033 cursor-publication finding).
Verification
bun test packages/coding-agent/test/chat-daemon-session-reconnect.test.ts --test-name-pattern 'notification failure'— passedbun test packages/coding-agent/test/chat-daemon-session-reconnect.test.ts— 17 passed; existing unrelated 256-frame host-ring test timed out waiting for its third socketbun --cwd=packages/coding-agent run check— Biome passed; TypeScript is blocked by the pre-existing missingsrc/internal-urls/docs-index.generatedmoduleAudit scope
Open PR #4065 credibly owns the #4060 lost-root admission-queue finding. The #4040 report is not reproducible against current
dev: the replay path only publishes tool results paired with a recorded tool call and closes known unreplayable results. The #4039 report is not reproducible from current source, which retains only unpersisted execution components on transcript rebuild. #4037 has no reproducible path in the report.