fix:(tui) reset stale streaming state after event gaps#62602
fix:(tui) reset stale streaming state after event gaps#62602sahilsatralkar wants to merge 6 commits into
Conversation
Greptile SummaryAdds Confidence Score: 5/5Safe to merge — scoped TUI state fix with no protocol, schema, or plugin contract changes. All remaining findings are P2 or lower. The fix correctly mirrors the cleanup logic of terminateRun (dropping the assembler entry, clearing sessionRuns, nulling activeChatRunId, setting idle status) and is consistent with existing abort/error handling — neither that path nor this one calls chatLog.dropAssistant directly; partial content is replaced by the subsequent history reload. One minor observation: when multiple runs are present in sessionRuns at gap time, only the active run's assembler entry is explicitly dropped while the rest are orphaned in the TuiStreamAssembler map — but they are bounded, unreachable after sessionRuns.clear(), and not a functional regression. No files require special attention. Reviews (1): Last reviewed commit: "fix(tui): reset stale streaming state af..." | Re-trigger Greptile |
![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: 63774fdef1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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: 1722974f36
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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: d9b5144fcb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (shouldReload) { | ||
| refreshAfterGap(); |
There was a problem hiding this comment.
When a gap occurs during an active run, this path always calls loadHistory(), even if pendingOptimisticUserMessage is true. In that case (run-a active, user already sent run-b), loadHistory clears the chat log and can remove the optimistic user turn for run-b; later, because run-b is pre-tagged local, its normal final path often skips history reload, so the missing user turn can persist in the UI. The no-active-run branch already special-cases pending optimistic sends, and the active-run branch needs the same guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ignoring this comment for now as it is P2
…treaming-gap-reset # Conflicts: # src/tui/tui-event-handlers.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2040974ad2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // to recover if the stream resumes with the same run id. | ||
| streamAssembler.drop(previousRunId); | ||
| sessionRuns.clear(); | ||
| state.activeChatRunId = null; |
There was a problem hiding this comment.
Clearing state.activeChatRunId here makes /abort unusable after a gap/disconnect even when the backend run is still executing. abortActive in src/tui/tui-session-actions.ts requires this field and immediately returns "no active run" when it is null, so after onGap/onDisconnected we can no longer cancel a still-running job until another chat event rebinds the run id (which may never happen for a dropped stream).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ignoring this for now as it is P2
…treaming-gap-reset # Conflicts: # src/tui/tui-event-handlers.test.ts # src/tui/tui.ts
openclaw-barnacle
Bot
added
app: web-ui
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5425f5346a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (gapClearedRunId === params.runId) { | ||
| gapClearedRunId = null; |
There was a problem hiding this comment.
Clearing gapClearedRunId as soon as the pre-gap run reaches final/aborted/error lets a foreign run consume pendingOptimisticUserMessage before the local pending run emits its first event. In the sequence run-a active -> local run-b queued (pending=true) -> gap -> run-a final -> foreign run-x delta, the (!gapClearedRunId || isKnownLocalRun) check becomes true again for run-x, so run-x is incorrectly marked local and the pending flag is consumed. Fresh evidence is that this new early clear re-opens the claim path even though the optimistic local run has not been bound yet.
Useful? React with 👍 / 👎.
|
Closing this as duplicate or superseded after Codex automated review. PR #62602 is superseded by merged PR #67401, which shipped the current TUI streaming watchdog in v2026.4.15. Current main resets stale streaming state after lost or late final chat events, has regression coverage, and the older explicit event-gap reset patch still carries unresolved optimistic-binding and abortability risks. Best possible solution: Close #62602 as superseded by shipped PR #67401. Keep the v2026.4.15 watchdog implementation on main; if maintainers still want immediate event-gap recovery, track it as a fresh focused change with tests for optimistic sends, foreign post-gap runs, history reload behavior, and What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a; fix evidence: release v2026.4.15, commit 352527393079. |
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
If this PR fixes a plugin beta-release blocker, title it
fix(<plugin-id>): beta blocker - <summary>and link the matchingBeta blocker: <plugin-name> - <summary>issue labeledbeta-blocker. Contributors cannot label PRs, so the title is the PR-side signal for maintainers and automation.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should catch this. Otherwise write
N/A.User-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.Diagram (if applicable)
For UI changes or non-trivial logic flows, include a small ASCII diagram reviewers can scan quickly. Otherwise write N/A.
Before:
[event gap during stream] -> [stale activeChatRunId remains] -> [status can stay "streaming"]
After:
[event gap during stream] -> [handleEventGap clears stale run + sets idle] -> [history refresh] -> [next run streams normally]
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.Built with Codex