-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix stream_end recovery race for live assistant settle #3885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
franksong2702
wants to merge
6
commits into
nesquena:master
from
franksong2702:franksong2702/fix-stream-end-recovery-race
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
c2e8c6b
Fix stream_end recovery race
b92f5bf
Preserve settled restore helper signature
dac194a
Update stream_end ownership test for status restore
58db7df
Address stream_end recovery review comments
b2cd9aa
Update restore signature tests
d99a34f
Stabilize session index rebuild test
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| """Regression coverage for stream-end recovery ordering. | ||
|
|
||
| #3877-style recovery relies on one subtle path: | ||
| when `stream_end` arrives while the active live assistant row is still | ||
| present, cleanup should be deferred briefly to allow pending final SSE updates to | ||
| settle, then performed through the shared terminal recovery helper. | ||
| """ | ||
|
|
||
| from pathlib import Path | ||
|
|
||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parent.parent | ||
| MESSAGES_JS = (REPO_ROOT / "static" / "messages.js").read_text(encoding="utf-8") | ||
|
|
||
|
|
||
| def _event_block(event_name: str) -> str: | ||
| marker = f"source.addEventListener('{event_name}'" | ||
| start = MESSAGES_JS.find(marker) | ||
| assert start >= 0, f"missing {event_name} listener" | ||
| brace = MESSAGES_JS.find("{", start) | ||
| assert brace >= 0, f"missing {event_name} listener body" | ||
| depth = 0 | ||
| i = brace | ||
| while i < len(MESSAGES_JS): | ||
| if MESSAGES_JS[i] == "{": | ||
| depth += 1 | ||
| elif MESSAGES_JS[i] == "}": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| return MESSAGES_JS[brace : i + 1] | ||
| i += 1 | ||
| raise AssertionError(f"unclosed {event_name} listener body") | ||
|
|
||
|
|
||
| def _function_body(name: str) -> str: | ||
| marker = f"async function {name}(" | ||
| start = MESSAGES_JS.find(marker) | ||
| if start < 0: | ||
| marker = f"function {name}(" | ||
| start = MESSAGES_JS.find(marker) | ||
| assert start >= 0, f"missing function: {name}" | ||
| brace = MESSAGES_JS.find("{", start) | ||
| assert brace >= 0, f"missing {name} body" | ||
| depth = 0 | ||
| i = brace | ||
| while i < len(MESSAGES_JS): | ||
| if MESSAGES_JS[i] == "{": | ||
| depth += 1 | ||
| elif MESSAGES_JS[i] == "}": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| return MESSAGES_JS[brace : i + 1] | ||
| i += 1 | ||
| raise AssertionError(f"unclosed function body: {name}") | ||
|
|
||
|
|
||
| def test_stream_end_defers_settlement_when_live_assistant_still_present(): | ||
| body = _event_block("stream_end") | ||
| assert "if(S.activeStreamId===streamId && _liveStreamEndScenePresent())" in body, ( | ||
| "stream_end should defer terminal cleanup while active live scene content is still present" | ||
| ) | ||
| assert "_scheduleStreamEndRecovery(source);" in body, ( | ||
| "stream_end should schedule the deferred recovery timer before returning" | ||
| ) | ||
| assert "_scheduleStreamEndRecovery(source)" in body, ( | ||
| "stream_end must delegate deferred cleanup to helper" | ||
| ) | ||
|
|
||
|
|
||
| def test_stream_end_fallback_does_not_finalize_when_session_is_still_active(): | ||
| body = _event_block("stream_end") | ||
| assert "const status=await _restoreSettledSession(source,{status:true});" in body | ||
| assert "if(status==='active'&&S.activeStreamId===streamId)" in body | ||
| assert "_scheduleStreamEndRecovery(source,200);" in body | ||
| assert "_finalizeStreamEndFallback(source);" in body | ||
|
|
||
|
|
||
| def test_stream_end_recovery_helper_retries_while_session_is_still_active(): | ||
| fn = _function_body("_runStreamEndRecovery") | ||
| assert "if(_streamFinalized || _terminalStateReached || !_pendingStreamEndRecovery)" in fn | ||
| assert "_restoreSettledSession(source,{status:true})" in fn | ||
| assert "if(status==='active'&&_streamEndRecoveryAttempts<10)" in fn | ||
| assert "_scheduleStreamEndRecovery(source,200);" in fn | ||
| assert "_finalizeStreamEndFallback(source);" in fn | ||
|
|
||
|
|
||
| def test_stream_end_fallback_helper_clears_owner_state_before_closing(): | ||
| fn = _function_body("_finalizeStreamEndFallback") | ||
| assert "_terminalStateReached=true;" in fn | ||
| assert "_streamFinalized=true;" in fn | ||
| assert "_clearOwnerInflightState();" in fn | ||
| assert "_clearApprovalForOwner();" in fn | ||
| assert "_clearClarifyForOwner('terminal');" in fn | ||
| assert "renderMessages({preserveScroll:true});" in fn | ||
| assert "_setActivePaneIdleIfOwner();" in fn | ||
| assert "_closeSource(source)" in fn | ||
|
|
||
|
|
||
| def test_stream_end_live_scene_detection_includes_empty_text_activity(): | ||
| fn = _function_body("_liveStreamEndScenePresent") | ||
| assert "if(assistantText||assistantRow) return true;" in fn | ||
| assert "liveReasoningText||reasoningText" in fn | ||
| assert "inflight.toolCalls.length" in fn | ||
| assert "data-live-worklog-shell" in fn | ||
| assert "data-thinking-active" in fn | ||
|
|
||
|
|
||
| def test_restore_settled_session_can_report_active_pending_status(): | ||
| fn = _function_body("_restoreSettledSession") | ||
| assert "async function _restoreSettledSession(source, options=null)" in MESSAGES_JS | ||
| assert "arguments[1]" not in fn | ||
| assert "const returnStatus=!!(options&&options.status);" in fn | ||
| assert "return returnStatus?'active':false;" in fn | ||
| assert "return returnStatus?'restored':true;" in fn | ||
|
|
||
|
|
||
| def test_stream_end_recovery_state_is_cleared_on_done_and_terminal_events(): | ||
| assert "_clearStreamEndRecovery();" in _event_block("done") | ||
| assert "_clearStreamEndRecovery();" in _event_block("stream_end") | ||
| assert "_clearStreamEndRecovery();" in _event_block("cancel") | ||
| assert "_clearStreamEndRecovery();" in _event_block("apperror") |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_finalizeStreamEndFallbackmissing stream-ownership guard before clearing global session stateWhen
_restoreSettledSessionreturns'stale'(i.e._isActiveSession()is true butS.activeStreamId !== streamId), a newer stream has already taken over the same session._finalizeStreamEndFallbackis called regardless, and itsif(_isActiveSession())branch unconditionally executesS.activeStreamId=null,clearLiveToolCards(), andrenderMessages()— clobbering the live UI of that new stream.The concrete failure path:
stream_endarrives while live content is present → 180 ms deferred recovery is scheduled → within that window the user submits a new message on the same session (S.activeStreamIdbecomes the new stream's ID) → recovery fires →_restoreSettledSessionseesS.activeStreamId !== streamIdand returns'stale'→_finalizeStreamEndFallbacknullifiesS.activeStreamIdand re-renders from stale persisted state, erasing the new stream's live content.Compare with
_handleStreamError(line 3786), which has an explicit early-return guard for exactly this condition:if(_isActiveSession() && S.activeStreamId!==streamId){ _closeSource(source); return; }. The same guard — or at minimum changingif(_isActiveSession())toif(_isActiveSession() && S.activeStreamId===streamId)— is needed in_finalizeStreamEndFallbackbefore theS.activeStreamId=null/clearLiveToolCards()/renderMessages()block. The same gap exists in the non-deferred directstream_endpath, which can also reach_finalizeStreamEndFallbackafter an async API roundtrip during whichS.activeStreamIdmay have advanced.