fix(chat): recover pending inputs after restart - #2129
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCold-start recovery now holds durable Queue inputs for explicit ChangesPending input restart recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/main/agent/deepchat/runtime/pendingInputPump.ts`:
- Around line 583-588: Reformat the deferred wakeup assignment in the pending
wakeup handling code around deferredWakeups.set so it complies with the
configured 100-column width, using Oxfmt or an equivalent multiline call layout
without changing its logic.
In `@test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts`:
- Around line 2632-2681: Extend the test around createDeepChatAgentHarness to
assert that the provider execution path is not called when recovering the
unclaimed Steer input. Keep the existing consumption and recoverPendingMessages
expectations, and use the harness’s existing provider stream/mock symbol to
verify it remains unused.
- Around line 8267-8289: Strengthen the test around
restartedAgent.resumePendingQueue by asserting queued.id is absent from
sqlitePresenter.deepchatPendingInputsTable before provider.coreStream begins
execution. Add the assertion inside the provider mock or otherwise verify the
call order, while preserving the existing assertions for successful resumption
and eventual processStream invocation.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 50f6b4d8-6e0d-4c44-a5dc-46cf4a4d9db8
📒 Files selected for processing (58)
docs/architecture/deepchat-agent-harness-boundaries/spec.mddocs/architecture/session-management.mddocs/features/im-style-steer-messages/spec.mddocs/issues/pending-input-restart-recovery/spec.mdsrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/harness/deepChatAgentHarness.tssrc/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.tssrc/main/agent/deepchat/runtime/pendingInputPump.tssrc/main/agent/deepchat/runtime/runLifecycleCoordinator.tssrc/main/agent/deepchat/runtime/transcriptMutationCoordinator.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/manager/deepChatAgentBackend.tssrc/main/agent/manager/sessionHandles.tssrc/main/app/composition.tssrc/main/session/contracts.tssrc/main/session/data/pendingInputs.tssrc/main/session/routes.tssrc/main/session/transcriptMutations.tssrc/main/session/turn.tssrc/renderer/api/SessionClient.tssrc/renderer/src/components/chat/PendingInputLane.vuesrc/renderer/src/components/message/MessageItemUser.vuesrc/renderer/src/features/chat-page/ChatPage.vuesrc/renderer/src/features/chat-page/composables/usePendingInputActions.tssrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/de-DE/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/es-ES/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/id-ID/chat.jsonsrc/renderer/src/i18n/it-IT/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ms-MY/chat.jsonsrc/renderer/src/i18n/pl-PL/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/tr-TR/chat.jsonsrc/renderer/src/i18n/vi-VN/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/stores/ui/pendingInput.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/sessions.routes.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.test.tstest/main/agent/deepchat/runtime/pendingInputPump.test.tstest/main/agent/deepchat/runtime/transcriptMutationCoordinator.test.tstest/main/session/data/pendingInputs.test.tstest/main/session/transcriptMutations.test.tstest/main/session/turn.test.tstest/renderer/components/PendingInputLane.test.tstest/renderer/components/message/MessageItemUser.test.tstest/renderer/features/chat-page/composables/usePendingInputActions.test.tstest/renderer/stores/pendingInputStore.test.ts
yyhhyyyyyy
left a comment
yyhhyyyyyy
left a comment
There was a problem hiding this comment.
I agree with the overall direction: pending-input listing should be a pure read, historical Queue items should require an explicit resume after restart, and an unclaimed Steer should become a terminal transcript fact rather than being executed by a later hydration/listing edge.
I found three restart-safety issues that I think should block the merge:
-
A claimed Queue does not durably record that its user message was materialized.
Startup recovery decides whether to consume or release a claimed Queue from
input.messageIds.length, but the normalappendUserFactpath inTurnCoordinatorcreates/adopts the user message without linking it back to the claimed pending-input row.linkClaimedQueueMessage()appears to be used only by the pre-stream Steer handoff path.If the process exits after the user message is persisted (or even after provider dispatch) but before the Queue claim settles, the next startup sees
messageIds: [], releases and holds the row, and also recovers the existing transcript attempt as an error. Resuming or retrying can then execute the same input again.This may predate the PR, but it directly invalidates the new claimed-Queue recovery contract. Please make user-message materialization/adoption and the claimed-row association durable in the same transaction, or establish another authoritative recovery marker. A follow-up link in a second write would retain the same crash window.
-
Pending Steer terminalization is not crash-atomic.
recoverInputsAfterRestart()consumes the Steer row in one transaction and returns its linked message IDs through an in-memoryforceRecoverMessagesBySessionmap. The harness callsrecoverPendingMessages()only afterwards.If the process exits or recovery throws after the Steer row is consumed but before every linked user message is changed to
error, the next startup cannot repair the state: the row is no longer active, while an unforced pending user message with Steer receipt metadata is intentionally kept pending. This leaves a permanentUnreadmessage with no pending row behind it.Please terminalize all linked Steer user messages and consume the Steer row in the same database transaction. A fault-injection test should restart again after the first commit / after the first message of a merged Steer and verify that every message is
errorand no active row remains. -
Manual Resume semantics are lost when attachment preparation blocks.
consumeClaimBeforeProviderStreamis derived only from the current wake reason. A manually resumed item that returnsneeds_user_actionbecomesblocked; resolving it schedules an ordinaryenqueuewake, so the next launch no longer has consume-before-provider enabled. If that provider attempt returns an error, the item is rolled back and released to pending, even though it is still the same explicitly resumed Queue head.Please preserve the manual-resume disposition by item ID across
blocked -> pending -> claimed, and clear it only when the item is consumed, deleted, or promoted. Please add coverage for both attachment Retry and “send without image content,” followed by a provider error.
One non-blocking scope issue: the renderer shows Resume for any idle pending Queue, and the backend ignores whether releaseRestartHoldForSession() actually released a hold. This applies the new manual semantics to ordinary live Queue rows as well. At minimum, the route should return started: false when no restart hold was released; ideally the renderer should receive an authoritative resumeAvailable signal.
The existing CI is green, but the current tests do not cover these crash boundaries or the blocked-attachment resume sequence.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts (1)
8292-8297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the observed state instead of asserting inside the provider mock.
The
expectcall runs insidecoreStream. If it fails, the error propagates into theprocessStreammock and is rejected there, not reported by the test runner. The later assertions still pass, becauseprocessStreamwas called and the item is consumed by then. A regression that consumes the claim after the provider starts can therefore go undetected.Capture the state in a variable and assert it after the wait.
💚 Proposed fix
- const provider = llmProvider.getProviderInstance('openai') - provider.coreStream.mockImplementationOnce(() => { - expect(sqlitePresenter.deepchatPendingInputsTable.get(queued.id)).toBeUndefined() - return (async function* () { - yield { type: 'stop', stop_reason: 'provider_error' } - })() - }) + const provider = llmProvider.getProviderInstance('openai') + let pendingRowAtProviderStart: unknown = 'not-observed' + provider.coreStream.mockImplementationOnce(() => { + pendingRowAtProviderStart = sqlitePresenter.deepchatPendingInputsTable.get(queued.id) + return (async function* () { + yield { type: 'stop', stop_reason: 'provider_error' } + })() + })Then assert after
await vi.waitFor(() => expect(processStream).toHaveBeenCalledOnce()):expect(pendingRowAtProviderStart).toBeUndefined()As per coding guidelines, "Add the smallest regression test for user-visible behavior or a documented contract."
🤖 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 `@test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts` around lines 8292 - 8297, Replace the in-provider expect in the coreStream mock with a variable capturing sqlitePresenter.deepchatPendingInputsTable.get(queued.id) when the provider starts. After awaiting processStream with vi.waitFor, assert that the captured pending row is undefined, ensuring failures are reported by the test runner.Source: Coding guidelines
test/main/agent/deepchat/runtime/pendingInputPump.test.ts (1)
279-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider resolving the blocked record through the production API.
Lines 303-308 mutate the fake store record directly to clear the blocked state. The production path resets a blocked Queue input through the resolve-blocked flow. The direct mutation couples the test to the record shape rather than to the observable reset contract.
If the pump port exposes no reset entry point, keep the current approach. Otherwise, drive the reset through that entry point.
As per coding guidelines, "Keep committed tests lean and focused on project reliability, stability, and observable contracts; remove temporary checks that only test implementation internals before handoff."
🤖 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 `@test/main/agent/deepchat/runtime/pendingInputPump.test.ts` around lines 279 - 314, Update the test around the blocked queue record to use the production resolve-blocked/reset API exposed by the pending-input pump instead of mutating pendingInputs.records directly. Preserve the existing assertions for the second drain and consumption semantics, and only retain direct mutation if no suitable reset entry point exists.Source: Coding guidelines
test/main/routes/dispatcher.test.ts (1)
4393-4400: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd dispatcher coverage for
sessions.resumePendingQueue.
sessions.listPendingInputsis covered, but the Queue resume route defined bysessionsResumePendingQueueRoutecallsdeps.turn.resumePendingQueue(sessionId)and has no dispatcher test. Add a small Vitest case that dispatchessessions.resumePendingQueueand assertssessionTurnPort.resumePendingQueueis called for the inputsessionId.🤖 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 `@test/main/routes/dispatcher.test.ts` around lines 4393 - 4400, Add a focused Vitest case alongside the existing dispatcher coverage that calls dispatchDeepchatRoute with sessions.resumePendingQueue and a sessionId, then assert sessionTurnPort.resumePendingQueue was called with that same ID. Use the existing runtime and route-context setup patterns without changing the sessions.listPendingInputs test.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts`:
- Around line 8292-8297: Replace the in-provider expect in the coreStream mock
with a variable capturing
sqlitePresenter.deepchatPendingInputsTable.get(queued.id) when the provider
starts. After awaiting processStream with vi.waitFor, assert that the captured
pending row is undefined, ensuring failures are reported by the test runner.
In `@test/main/agent/deepchat/runtime/pendingInputPump.test.ts`:
- Around line 279-314: Update the test around the blocked queue record to use
the production resolve-blocked/reset API exposed by the pending-input pump
instead of mutating pendingInputs.records directly. Preserve the existing
assertions for the second drain and consumption semantics, and only retain
direct mutation if no suitable reset entry point exists.
In `@test/main/routes/dispatcher.test.ts`:
- Around line 4393-4400: Add a focused Vitest case alongside the existing
dispatcher coverage that calls dispatchDeepchatRoute with
sessions.resumePendingQueue and a sessionId, then assert
sessionTurnPort.resumePendingQueue was called with that same ID. Use the
existing runtime and route-context setup patterns without changing the
sessions.listPendingInputs test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ddb94204-ca22-4ead-9e8e-a7c4961e5e5a
📒 Files selected for processing (31)
docs/architecture/deepchat-agent-harness-boundaries/spec.mddocs/architecture/session-management.mddocs/features/im-style-steer-messages/spec.mddocs/issues/pending-input-restart-recovery/spec.mdsrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/harness/deepChatAgentHarness.tssrc/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.tssrc/main/agent/deepchat/runtime/pendingInputPump.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/agent/manager/deepChatAgentBackend.tssrc/main/agent/manager/sessionHandles.tssrc/main/app/composition.tssrc/main/session/contracts.tssrc/main/session/data/pendingInputStore.tssrc/main/session/data/pendingInputs.tssrc/main/session/data/transcript.tssrc/main/session/routes.tssrc/main/session/turn.tssrc/renderer/api/SessionClient.tssrc/renderer/src/features/chat-page/ChatPage.vuesrc/renderer/src/stores/ui/pendingInput.tssrc/shared/contracts/routes/sessions.routes.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.test.tstest/main/agent/deepchat/runtime/pendingInputPump.test.tstest/main/agent/manager/deepChatAgentBackend.test.tstest/main/routes/dispatcher.test.tstest/main/session/data/pendingInputs.test.tstest/main/session/data/tables/deepchatPendingInputsTable.test.tstest/main/session/turn.test.tstest/renderer/stores/pendingInputStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- test/main/session/turn.test.ts
- src/main/agent/deepchat/harness/createDeepChatAgentHarness.ts
- docs/architecture/deepchat-agent-harness-boundaries/spec.md
- src/renderer/src/features/chat-page/ChatPage.vue
- src/renderer/api/SessionClient.ts
- src/main/app/composition.ts
- src/main/agent/deepchat/harness/deepChatAgentHarness.ts
- docs/architecture/session-management.md
- test/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.test.ts
- src/main/session/turn.ts
- src/main/agent/deepchat/runtime/pendingInputAdmissionCoordinator.ts
- test/main/session/data/pendingInputs.test.ts
- docs/issues/pending-input-restart-recovery/spec.md
- src/main/session/contracts.ts
- src/main/agent/deepchat/runtime/pendingInputPump.ts
|
Addressed the two applicable CodeRabbit nitpicks in
No change was made to the direct fake-store reset in |
2 participants
Summary
Root cause
Durable rows in
deepchat_pending_inputssurvived a process restart, but their in-memory scheduling edge and user-visible disposition did not. Queue and Steer were also treated as equivalent pending work even though they represent different user commitments. A later read or lifecycle wake could therefore execute an old input, while a failed manually resumed Queue claim could be released back topendingand reappear on the next Session entry.Behavior
Before
After
The existing message toolbar remains available for normal retry. No Steer-specific failure receipt or recovery action is added.
Implementation
sessions.resumePendingQueueroute.errortranscript message, consume its active row, and hide the receipt in the renderer.Impact
Validation
pnpm formatpnpm i18npnpm lintpnpm typecheckThe full main-process run has 60 pre-existing Windows-environment failures across 27 unchanged files, dominated by POSIX path expectations, symlink permissions, executable-bit checks, and packaging fixtures. All affected main-process suites pass.
Closes #2111
Summary by CodeRabbit
New Features
Bug Fixes