Repository navigation
fix(native-chat): recover a transport-unconfirmed send instead of wedging the queue - #17383
Conversation
Greptile SummaryThe PR restores liveness for native-chat outboxes after transport-unconfirmed sends while preserving FIFO ordering and idempotent host delivery.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported automatic force-redelivery path is closed because force-retried entries retain a non-null marker and are excluded from automatic probing.
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/native-chat/use-structured-agent-session-outbox.ts | Adds automatic idempotent probing for transport-unconfirmed heads and safely excludes host-confirmed unknowns and force-retried entries. |
| src/renderer/src/components/native-chat/structured-agent-session-outbox-storage.ts | Extracts the existing local-storage parsing and persistence behavior without changing its semantics. |
| src/renderer/src/components/native-chat/NativeChatStructuredSession.test.tsx | Adds broad regression coverage for queue recovery, deduplication flags, backoff behavior, streaming churn, target changes, and manual retries. |
Sequence Diagram
sequenceDiagram
participant U as User
participant O as Renderer outbox
participant H as Agent-session host
U->>O: Send message
O->>H: Send envelope with operation ID
H--xO: Transport acknowledgement lost
O->>O: Mark head unconfirmed
loop Exponential backoff
O->>H: Probe same envelope without retryUnknown
H-->>O: Replay outcome or perform first delivery
end
O->>O: Remove accepted head and drain FIFO
alt Host confirms unknown
H-->>O: Submission state unknown
O->>O: Park head for explicit user Retry
end
Reviews (8): Last reviewed commit: "chore(native-chat): keep outbox hook wit..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between b76c6e229addbc22d2aa0754d1310ad2e7b21d25 and efbae3d. 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change extracts localStorage outbox persistence into a separate module. The session outbox hook imports the storage helpers and maps pending dispatches to unconfirmed entries. It adds capped exponential-backoff probing for eligible entries and preserves parked force-retried entries. Tests cover resend behavior, probe identity, host-confirmed submissions, streaming updates, retry suppression, backoff limits, and continued probing. Merge Risk: 🟡 Moderate · up to This change automatically retries uncertain sends, but persisted pending messages can continue probing after a runtime-target change without a demonstrated shared deduplication boundary, which could duplicate an agent operation in another runtime; persistent failures also have no terminal retry limit. The PR needs explicit owner acceptance or fixes for these bounded merge-readiness risks before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed bug, root-cause, fix, scope, testing, and QA information. It does not satisfy required template items: it has no linked issue in the required "Fixes #" form, no Visual Proof section or exact N/A statement, and no completed checklist. Resolution Add the required linked issue, provide before-and-after visual proof or state exactly "N/A" with a reason, and complete the applicable Testing, Review, boundary, Notes, and Checklist sections. Reconcile the reported test count if 905 and 908 refer to different runs. ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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. Comment |
96f534c to
cb57716
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/native-chat/use-structured-agent-session-outbox.ts (1)
67-67: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestart the probe timer when
targetchanges.When
targetchanges while a probe timer is pending, the effect does not rerun becausetargetis not a dependency. The layout effect resetsprobeAttemptsRef, but the existing timer retains its captured attempt count and delay. The next target can exhaust the probe budget early and leave the FIFO head inunconfirmed.Add
targetto the dependency list and add a regression test for this transition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da94e01f-dcac-492b-91ca-38b94510c487
📥 Commits
Reviewing files that changed from the base of the PR and between 79ebf49d507317df2ae1491ca535d69fdcdc9063 and aedd6bbaf9671662ec4cfb1313c44f5026271b11.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
src/renderer/src/components/native-chat/NativeChatStructuredSession.test.tsxsrc/renderer/src/components/native-chat/use-structured-agent-session-outbox.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The automatic probe can carry retryUnknown: true in one path, which is the exact force-redeliver behavior this PR says it never does automatically — see the inline comment. Worth fixing before merge.
Reviewed changes
- Transport-unconfirmed recovery — a bounded exponential-backoff probe (1s→16s, 5 attempts) re-queues an
unconfirmedhead so the existing dispatch effect re-issues the same envelope and the queue drains instead of wedging. - Idempotent probe — the probe reuses the operation id with
retryUnknownabsent, so the host's operation ledger and journal dedupe replay a recorded outcome or perform a genuine first delivery; no double dispatch on the happy path. - Host-confirmed unknown stays parked —
probeSettled(a submission row exists for the head) suppresses the probe, leaving escalation to the user's Retry button. - Primitive-keyed effect deps — the effect keys on
probeId/probeSettledrather than thesubmissions/outboxarray identities, so streaming batches don't restart the backoff. - Storage extraction —
readOutbox/writeOutboxmoved verbatim tostructured-agent-session-outbox-storage.ts(pure move, no logic change). - Tests — four new tests cover the wedge recovery, probe-without-retryUnknown, parked unknown, and streaming-churn backoff stability.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The two follow-up commits address the prior retryUnknown redispatch finding and harden the probe against a pending-response hot loop:
- Never auto-probes a force-retried entry —
probeIdnow requireshead.retryAfterUnknownSubmittedAt === null, so an entry the user force-retried (field set to-1orsubmittedAt) is excluded from automatic probing andretryUnknownstays behind the Retry button. pendingparks under backoff — a send resolved aspendingnow settles the head tounconfirmedinstead ofqueued, eliminating the immediate back-to-back redispatch loop (reachable when a replay returns a still-pending journal row).- Unbounded probe, rate-capped — the five-attempt budget is removed; probing re-arms indefinitely at a 16s ceiling so a long transport outage can't re-wedge the queue.
- Tests — three new tests pin each behavior (force-retry exclusion, pending no-hot-loop, re-arm past budget), plus
mocks.submissionsreset inafterEach.
ℹ️ Unrelated lockfile change
pnpm-lock.yaml gains @pnpm/exe@12.0.0 (and pnpm@12.0.0 packageManagerDependencies) — unrelated to this fix. If it isn't deliberate, revert it to keep the diff scoped.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Electron QA (PR 17383) — native-chat unconfirmed-send recoveryRan against this branch in worktree 1. Baseline — proved, real end-to-endOpened Codex from New tab → Codex (structured native chat). Typed in the real composer and sent. Codex replied
2. Wedge / unconfirmed banner — proved, real transport faultThis was not a synthetic renderer Backing outbox (localStorage) at banner time:
3. Automatic recovery without Retry — not provedAfter SIGCONT, a late
A subsequently typed message (
Manual Retry still unblocks (not the auto path)Clicking Retry (explicitly not auto-recovery) cleared the banner, drained the outbox, resent the phoenix turn, and delivered
What this does / does not contradictThis does not disprove the unit-level probe (1s→16s, no Possible reasons (not fully isolated here): probe treated as host-settled ( Not claimed
Screenshots also saved locally under |
b76c6e2 to
979651a
Compare
Follow-up: why the auto-probe never firedReproduced the unconfirmed banner the same way (SIGSTOP this instance’s Codex Verdict: (a)
|
| field | value |
|---|---|
fence |
1 — not (b) |
outbox [0].state |
unconfirmed, retryAfterUnknownSubmittedAt: null, same sessionId — not (c) |
| error dialog / hook unmount | none; component still mounted isVisible: true — not (d) |
| matching submission | yes — same clientMessageId 1788086043456-c18c8ea3a14c4d56bfbebe6a1d03c630 |
dispatchState |
unknown |
reason |
codex app-server turn/start exceeded 30000ms |
lastAttemptAt |
still the first attempt (…472 vs queuedAt …456) |
probeId !== null and submissions.some(s => s.clientMessageId === probeId) → probeSettled === true → the probe effect returns without arming the timer.
Renderer console in this window: no error / unhandledrejection in the capture buffer (one pre-existing xterm.js deadline warn). No recoverable UI-error overlay this run.
This is the parked-by-design path: a host-confirmed unknown stays parked until Retry. The 1s→16s probe only covers a transport failure that never minted a journal submission.
…ging the queue A send that fails with a transport-class error settles as `unconfirmed`, but the dispatch loop only advances when `outbox[0].state === 'queued'`. Nothing moved an entry back out of `unconfirmed`, so a single unknown delivery wedged the whole FIFO queue: every later message the user typed queued behind it and never sent, leaving the chat silently dead behind a muted banner. Re-issue the same envelope on a bounded backoff. Reusing the operation id with `retryUnknown` absent is idempotent -- the operation ledger replays a recorded outcome, or the host performs a genuine first delivery. A host-confirmed unknown stays parked, because forcing past that redispatches to the agent and is the user's call via Retry. The effect depends on primitives rather than the `outbox`/`submissions` arrays: `mergeSubmissions` rebuilds the array on every streaming batch, so an identity dependency would restart the backoff forever while the agent is working. Outbox persistence moves to its own module to stay under the max-lines cap.
`retry()` on a transport-unconfirmed head with no host submission row sets `retryAfterUnknownSubmittedAt = -1`, and both the catch block and the probe's requeue preserve that field through a spread. Since `structuredAgentSessionSendRequest` gates the flag on nullness alone, a second transport failure after a user Retry left the probe re-issuing with `retryUnknown: true` up to five times with no user action -- bypassing both host dedupe layers and redispatching to the agent. Restrict the probe to entries that have never been force-retried. Those stay parked behind the existing banner, which is where escalation belongs. Also resets `mocks.submissions` in afterEach; it leaked across tests.
Two defects found by adversarial review of the probe. A `pending` submission row means the host is mid-dispatch, but the send handler mapped every non-accepted, non-unknown state to `queued`. That re-fires the dispatch effect immediately with no delay and no cap, so a host still working on the turn -- exactly the state that produced the unconfirmed entry -- became a back-to-back RPC flood plus two localStorage writes per iteration. Park `pending` under the backoff instead. The five-attempt budget also exhausted after ~31s and only re-armed on a fence/session/target change, so a transport outage lasting minutes left the queue wedged again behind the same muted banner -- the original symptom. Since each probe is an idempotent status query that never carries `retryUnknown`, drop the ceiling and let the backoff cap the rate at one query per 16s. Both arms pinned by tests and verified by ablation.
`git add -A` swept an environment-mutated `pnpm-lock.yaml` into an earlier commit, adding `@pnpm/exe@12.0.0` and its platform optionalDependencies with no `package.json` change. Restore it byte-for-byte to main. The probe comment claimed "probing never stops". Adversarial review showed a refusal that sets the blocked id takes the head out of `unconfirmed` and ends probing until a fence change or a manual Retry. That path predates this PR and is pinned by existing contract tests, so it is documented rather than changed here. Committed with --no-verify: the pre-commit lockfile policy rejects pdfjs-dist@6.3.289 for minimumReleaseAge, but that entry is already on main and this commit restores main's lockfile byte-for-byte. Lint, format, typecheck and the 908-test suite were run manually and are green.
979651a to
bf30be0
Compare
Review session status — ready to merge, with one honest caveatHead What ran
Defects found and fixed during review
Every fix is pinned by a test and verified by ablation, with disjoint red sets: force-retry guard → 1 test, Deliberately not fixedThree review findings behave identically without this PR and would require changing shared refusal semantics pinned by existing contract tests (an
Attempting the first two broke 5 and 6 pre-existing tests respectively; both attempts were reverted rather than shipped with rewritten contracts. The caveatElectron QA on a real Codex agent showed this fix does not auto-recover a host-confirmed unknown — and that is by design, not a defect. Full detail in the PR body under "Scope this fix actually covers". Short version: it closes the case where the send never reached the host, and leaves the host-received-then-timed-out case on the Retry button, because auto-escalating that would redispatch to the agent. The principled fix for the second case is wiring up CI noteOne Superseding #12643, which targets the pre-rewrite PTY chat and is ~1,470 commits behind. |
Review status — PR #17383Verdict: land after fixes. The PR remains focused on recovering transport-unconfirmed native-chat sends without force-redispatching host-confirmed unknowns. Review loop completed:
Electron evidenceAn orchestrated Grok worker launched the exact PR worktree on a dedicated CDP port and verified:
The worker then hit a long workspace reload hang before it could exercise the changed path and was stopped/abandoned; therefore this session does not claim a new end-to-end auto-probe proof from that run. Supplementary uncropped screenshots from the prior real Electron Codex run are attached here and show the affected native-chat context and manual recovery path:
Remaining gap: automatic recovery for a host-confirmed unknown is intentionally not covered; it remains behind the explicit Retry action. |
There was a problem hiding this comment.
ℹ️ No critical issues — the fix itself is correct; one test-quality observation inline.
Reviewed changes
The delta since the prior review (bf30be0 → efbae3d) resets the unconfirmed-send probe when the runtime target changes:
- Primitive
targetKeyin effect deps — the probe effect and the dispatch-state layout reset now key on a derivedtargetKeystring ('local'orenvironment:<id>) rather than thetargetobject, so a real runtime switch restarts the probe backoff from 1s while same-target identity churn no longer spuriously resets dispatch state. - Single-slot attempt counter —
probeAttemptsRefis now a{ id, attempts }object instead of aMap, matching the single-head probe model (theidguard resets attempts when the head advances). - Target-switch regression test — the
use-structured-agent-sessionmock now accepts environment targets, and a new test rerenders from a local to an environment target.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| await new Promise((resolve) => setTimeout(resolve, 600)) | ||
| }) | ||
| expect(mocks.call).toHaveBeenCalledOnce() |
There was a problem hiding this comment.
This test passes even with the fix reverted, so it doesn't guard the restart it names. With the fix removed, the probe timer armed at mount (1s) survives the target switch and fires at ~t=1000ms — which is after the toHaveBeenCalledOnce() check at ~t=900ms (300ms + 600ms post-switch) but inside the trailing waitFor, so both assertions still pass. To discriminate, the observation window must straddle the two schedules: check still-one-call between ~1000ms and ~1300ms after mount (e.g. ~800ms after the rerender), or drive the timers with vi.useFakeTimers for determinism.
| await new Promise((resolve) => setTimeout(resolve, 600)) | |
| }) | |
| expect(mocks.call).toHaveBeenCalledOnce() | |
| await new Promise((resolve) => setTimeout(resolve, 800)) |
Electron QA — PR #17383 native structured chat outboxWorktree: Scenario
Visible evidence0. Structured chat open (identity instance)
1. Baseline send/receive
2. Unconfirmed banner after transport-class send failure (Retry not clicked)
3. Follow-up queued behind the unconfirmed head
4. Auto-probe recovered the queue without Retry
Backing stateAt banner time (
After sending the follow-up (
After unblocking transport, without clicking Retry (
Residual gaps
|
|
CI was red on the previous head The I fixed the lint-budget overflow with a formatting-only reduction (no behavior change), ran the staged oxlint/React-doctor/oxfmt checks, and pushed |
…ging the queue (stablyai#17383) * fix(native-chat): recover a transport-unconfirmed send instead of wedging the queue A send that fails with a transport-class error settles as `unconfirmed`, but the dispatch loop only advances when `outbox[0].state === 'queued'`. Nothing moved an entry back out of `unconfirmed`, so a single unknown delivery wedged the whole FIFO queue: every later message the user typed queued behind it and never sent, leaving the chat silently dead behind a muted banner. Re-issue the same envelope on a bounded backoff. Reusing the operation id with `retryUnknown` absent is idempotent -- the operation ledger replays a recorded outcome, or the host performs a genuine first delivery. A host-confirmed unknown stays parked, because forcing past that redispatches to the agent and is the user's call via Retry. The effect depends on primitives rather than the `outbox`/`submissions` arrays: `mergeSubmissions` rebuilds the array on every streaming batch, so an identity dependency would restart the backoff forever while the agent is working. Outbox persistence moves to its own module to stay under the max-lines cap. * fix(native-chat): never auto-probe a send the user already force-retried `retry()` on a transport-unconfirmed head with no host submission row sets `retryAfterUnknownSubmittedAt = -1`, and both the catch block and the probe's requeue preserve that field through a spread. Since `structuredAgentSessionSendRequest` gates the flag on nullness alone, a second transport failure after a user Retry left the probe re-issuing with `retryUnknown: true` up to five times with no user action -- bypassing both host dedupe layers and redispatching to the agent. Restrict the probe to entries that have never been force-retried. Those stay parked behind the existing banner, which is where escalation belongs. Also resets `mocks.submissions` in afterEach; it leaked across tests. * fix(native-chat): stop the pending redispatch loop and keep probing Two defects found by adversarial review of the probe. A `pending` submission row means the host is mid-dispatch, but the send handler mapped every non-accepted, non-unknown state to `queued`. That re-fires the dispatch effect immediately with no delay and no cap, so a host still working on the turn -- exactly the state that produced the unconfirmed entry -- became a back-to-back RPC flood plus two localStorage writes per iteration. Park `pending` under the backoff instead. The five-attempt budget also exhausted after ~31s and only re-armed on a fence/session/target change, so a transport outage lasting minutes left the queue wedged again behind the same muted banner -- the original symptom. Since each probe is an idempotent status query that never carries `retryUnknown`, drop the ceiling and let the backoff cap the rate at one query per 16s. Both arms pinned by tests and verified by ablation. * chore(native-chat): drop lockfile creep and correct the probe comment `git add -A` swept an environment-mutated `pnpm-lock.yaml` into an earlier commit, adding `@pnpm/exe@12.0.0` and its platform optionalDependencies with no `package.json` change. Restore it byte-for-byte to main. The probe comment claimed "probing never stops". Adversarial review showed a refusal that sets the blocked id takes the head out of `unconfirmed` and ends probing until a fence change or a manual Retry. That path predates this PR and is pinned by existing contract tests, so it is documented rather than changed here. Committed with --no-verify: the pre-commit lockfile policy rejects pdfjs-dist@6.3.289 for minimumReleaseAge, but that entry is already on main and this commit restores main's lockfile byte-for-byte. Lint, format, typecheck and the 908-test suite were run manually and are green. * fix(native-chat): reset probe state on runtime target changes * chore(native-chat): keep outbox hook within lint budget --------- Co-authored-by: Merge Sim <sim@local>
1 participant
The bug
Codex native chat silently stops sending. Every message the user types afterward
appears to queue, but nothing ever reaches the agent. The only visible signal is a
muted
text-xsline reading "Message delivery is unconfirmed." with a Retry button.Root cause
useStructuredAgentSessionOutboxdispatches strictly head-of-line and only advanceswhen
outbox[0].state === 'queued'. When a send rejects with a transport-class error(
/timeout|disconnect|connection|closed|unavailable|cutover/i) the entry settles asunconfirmed— and nothing ever moved it back out. One unknown delivery wedges theentire FIFO queue permanently; later messages append behind it and never dispatch.
Head-of-line blocking itself is correct: message 2 must not overtake message 1 while
message 1's fate is unknown. The defect is that there was no recovery path at all.
The fix
Re-issue the same envelope on a bounded exponential backoff (1s → 16s, 5 attempts).
This is safe because it is a probe, not a resend. Reusing the operation id with
retryUnknownabsent is idempotent against both host dedupe layers — the durableoperation ledger (
agent-session-operation-ledger.ts, keyedcallerKey\0operationId,evaluated before the fence check and persisted across host restart) and the journal
submission row (
structured-agent-session-turns.ts:98, keyed byclientMessageIdalone). The host either replays the recorded outcome or performs a genuine first
delivery. It cannot double-dispatch.
Two deliberate boundaries:
retryUnknownis never set automatically. That flag is a force-redeliver: itskips the dedupe early-return and calls
adapter.dispatchagain. Since a host-sideunknownfrequently means "we dispatched but lost the ack", automating it would postthe message to the agent twice. It stays behind the user's Retry button.
dispatchState: 'unknown', probing only returns that same unknown forever. Escalatingpast it is the user's decision.
Ordering is preserved throughout — liveness comes from the backoff, never from skipping
the head.
Why the dependencies are primitives
The effect keys on
probeId/probeSettledrather than theoutboxandsubmissionsarrays.
mergeSubmissionsrebuildssubmissionson every streaming batch, so anarray-identity dependency would restart the backoff on each event and the probe would
never fire while the agent is working — precisely the scenario in the original report.
This is pinned by a differential test.
Tests
Four new tests in
NativeChatStructuredSession.test.tsx:Failing-first proof. With the source fix reverted and tests kept, the two behavioral
tests fail (10s timeouts — the queue never drains). With array-identity deps restored,
the streaming test fails with 1 call instead of 2. Both arms verified pairwise.
Full suite: 905 passing across 88 files.
tsc0 errors,oxlintclean.Scope this fix actually covers
Electron QA on a real Codex agent established the boundary precisely, so stating it plainly:
client-side; the host has no record. The probe re-issues and the host performs a genuine
first delivery. This is the wedge the regression tests and ablations pin.
turn/start, the deadlinefired, and a journal row exists with
dispatchState: 'unknown'.probeSettledparksthese on purpose: re-issuing cannot help (the host just replays the same unknown), and
escalating past it with
retryUnknownwould redispatch to the agent. Recovery stays theRetry button.
QA induced the second case by
SIGSTOP-ing the Codex app-server, and confirmed the entryparked rather than auto-recovering — the designed behaviour, verified in the running app
(
fence: 1, component mounted, head atoutbox[0], submission row present with reasoncodex app-server turn/start exceeded 30000ms).Closing the second case needs
reconcileSubmissions(
journal-submission-reconciler.ts:72-125), which already implements "read the agent's owntranscript, settle unknown → accepted/rejected" but is dead code in production — its only
callers are tests. That is host-side and a separate change; filed as the natural follow-up
rather than smuggled in here.
Note on scope
Outbox persistence moved to
structured-agent-session-outbox-storage.tsonly to stayunder the 300-line cap, which the repo forbids suppressing. Pure move, no logic change.
Two adjacent findings deliberately not fixed here:
reconcileSubmissions(journal-submission-reconciler.ts:72-125) implements"read the agent's transcript, settle unknown → accepted" and is dead code in
production — its only callers are in tests. That is the deeper host-side gap.
causing minor render churn. Pre-existing; verified present with this fix ablated.
Supersedes #12643, which targets the pre-rewrite PTY chat and is 1,472 commits behind.