fix(notifications): stop announcing aborted, failed or stray idles as completed - #234
Conversation
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| refreshContextUsage(targetSessionId) | ||
| } | ||
| if (interrupting) onSessionAborted(targetSessionId) |
There was a problem hiding this comment.
[bug · low]
onSessionAborted(markSessionAborted) が currentBackend.sendMessage の成功を待たずに呼ばれています。sendMessage が例外を投げた場合、この runCatching ブロックの onFailure でエラー表示になりますが、その時点で session は既に mutedSessionIds に追加され activeSessionIds からも削除されています。そのため、中断されるはずだった従来の実行が（sendMessage 失敗で実際には中断されず）継続し、後で完了しても SessionIdle 通知が抑制されたままになります。sendMessage 成功後に呼び出すか、失敗時に mute 状態を巻き戻す処理を検討してください。
| val currentBackend = backend ?: return | ||
| val sessionId = _uiState.value.sessionId ?: return | ||
| viewModelScope.launch { | ||
| onSessionAborted(sessionId) |
There was a problem hiding this comment.
[bug · medium]
abortSession の成否に関わらず onSessionAborted が先に呼ばれます。abortSession が失敗して実行が続いた場合でも、session は activeSessionIds から除去され mutedSessionIds に残るため、ドロワーのスピナーが早期に消え、かつ後続の完了通知（SessionIdle）も抑制されます。runCatching { ... }.onSuccess { onSessionAborted(sessionId) } のように成功時のみ通知するか、失敗時に markSessionRunning 相当で状態を復元することを検討してください。
| // A CLI that exits before its result line would otherwise leave the chat spinning | ||
| // forever; that is a failure, not a completion. A finished turn already emitted its | ||
| // own idle, and repeating it on process exit would re-announce the old run. | ||
| if (!parser.turnFinished) { |
There was a problem hiding this comment.
[other · low]
前回の実装ではプロセス終了時に常に events.tryEmit(OpenCodeEvent.SessionIdle(sessionId)) が送出されていましたが、今は parser.turnFinished == false のときだけ送出されます。turnFinished は beginTurn()（send 時）で false にリセットされるため、既存プロセス再利用中（ensureProcess が既存セッションを返すケース）に beginTurn と result 行の間に終了が起きる場合は問題ありませんが、一度 turnFinished == true になった直後（result 行受信後から次の send までの間）にプロセスが終了した場合、この分岐は何も送出しません。この期間では既に result 行で SessionIdle が送出済みなので問題ありません。ロジックは妥当です。
| if (!parser.turnFinished) { | ||
| val exitCode = runCatching { process.exitValue() }.getOrNull() | ||
| events.tryEmit( | ||
| OpenCodeEvent.SessionError(sessionId, messages.processExited(exitCode, streamFailure?.message)), | ||
| ) | ||
| events.tryEmit(OpenCodeEvent.SessionIdle(sessionId)) | ||
| } |
There was a problem hiding this comment.
[bug · high]
ここで parser.turnFinished が true（result 行を受信後にプロセス終了）の場合、streamFailure が null 以外でも SessionError はまったく発行されません。しかし handleLine はパースされたイベントだけで SessionError を emit するため、turnFinished == true で is_error の result 行を受信した場合でも、ここではエラーも idle も送出されません。この場合、呼び出し側でチャットがスピナー状態のままになる可能性があります。parser.turnFinished に加えて streamFailure やエラーフラグも考慮して終了処理を判断すべきです。
| // forever; that is a failure, not a completion. A finished turn already emitted its | ||
| // own idle, and repeating it on process exit would re-announce the old run. | ||
| if (!parser.turnFinished) { | ||
| val exitCode = runCatching { process.exitValue() }.getOrNull() |
There was a problem hiding this comment.
[maintainability · low]
runCatching { process.exitValue() } の exitCode == 0 を「正常終了」として扱わず、!parser.turnFinished だけでエラーを判定しています。正常終了コード 0 かつ未完了ターン（例: リザルト行未受信のまま終了）でも「エラー」扱いで問題ないなら、exitCode を条件に含めるかコメントで意図を明示することを推奨します。また詳細メッセージが null のときは exit code が使われますが、メッセージの生成は ClaudeMessages 側に委ねられており、ここでは判断が分かりにくいです。
| detail: String?, | ||
| ): String { | ||
| val cause = detail ?: exitCode?.let { "exit code $it" } ?: "process exited" | ||
| return context.getString(R.string.claude_error_process_exited, cause) |
There was a problem hiding this comment.
[other · low]
exit code や process exited という英語文字列が翻訳済みリソースの %1$s にそのまま埋め込まれます。多言語対応のアプリのため、日本語などの表示時にも英語が混在してしまいます。文言全体をリソース側に寄せる（例: exitCode を個別フォーマット引数として渡す）ことを検討してください。
| @Volatile | ||
| var turnFinished: Boolean = false | ||
| private set |
There was a problem hiding this comment.
[maintainability · low]
Parsed.turnFinished フィールド（42 行目）とこの @Volatile プロパティ turnFinished でターン完了状態が二重管理されています。どちらも parseResult 内で true に設定しており、将来の変更で不整合が生じやすくなっています。Parsed.turnFinished を廃止して本プロパティに一本化するか、プロパティを Parsed から導出する形にするなど、単一の情報源に整理することをお勧めします。
| @Volatile | ||
| var turnFinished: Boolean = false | ||
| private set |
There was a problem hiding this comment.
[maintainability · low]
Parsed.turnFinished フィールド（42 行目）とこの @Volatile プロパティ turnFinished でターン完了状態が二重管理されています。どちらも parseResult 内で true に設定しており、将来の変更で不整合が生じやすくなっています。Parsed.turnFinished を廃止して本プロパティに一本化するか、プロパティを Parsed から導出する形にして、単一の情報源に整理することをお勧めします。
| fun beginTurn() { | ||
| turnFinished = false | ||
| } |
There was a problem hiding this comment.
[bug · medium]
beginTurn() は turnFinished を無条件に false へリセットしますが、send() が書き込み失敗（broken pipe 等）で失敗したケースでは、send() の onFailure が SessionError/SessionIdle を送出した後も turnFinished == false のままになります。すると reader ジョブのプロセス終了処理（ClaudeCodeRuntime の if (!parser.turnFinished) 分岐）でも同じ SessionError/SessionIdle が再送出され、エラー/完了イベントが重複します。送信失敗パスで turnFinished を true にする、あるいはターン完了イベントの送出元を一箇所に集約し、二重送出を防いでください。
| fun beginTurn() { | ||
| turnFinished = false | ||
| } |
There was a problem hiding this comment.
[bug · medium]
beginTurn() は turnFinished を無条件に false へリセットしますが、ClaudeCodeRuntime.send() が書き込み失敗（broken pipe 等）で失敗した場合、onFailure が SessionError/SessionIdle を送出した後に turnFinished が false のまま残ります。すると reader ジョブのプロセス終了処理（ClaudeCodeRuntime の if (!parser.turnFinished) 分岐）でも同じ SessionError/SessionIdle が再送出され、エラー/完了イベントが重複するリスクがあります。送信失敗パスでも turnFinished を true にするか、ターン終了イベントの送出元を一箇所に集約して二重送出を防いでください。
| if (interrupting) onSessionAborted(targetSessionId) | ||
| currentBackend.sendMessage( |
There was a problem hiding this comment.
[bug · high]
割り込み送信時に onSessionAborted を呼んでいますが、これは abortSession を呼ぶ前の段階です。markSessionAborted は mutedSessionIds に session を追加し、以降の SessionIdle イベントによる完了通知を永久に抑制します。もし sendMessage（割り込み）が後のコルーチン内で失敗して abortSession が実行されない/失敗する場合、このセッションの完了通知が以後送られなくなります。onSessionAborted は実際に割り込み（abortSession/sendMessage）が成功した後に呼ぶか、失敗時に mutedSessionIds から除外するリカバリ処理を入れてください。
| onSessionAborted(sessionId) | ||
| runCatching { currentBackend.abortSession(sessionId) } |
There was a problem hiding this comment.
[bug · high]
onSessionAborted(sessionId) が runCatching { currentBackend.abortSession(sessionId) } の直前（実行前）に呼ばれています。abortSession が失敗して .onFailure でエラー表示になるケースでも、mutedSessionIds に session が残ったままとなり、その後の実際の SessionIdle イベントによる完了通知が抑制されます。markSessionAborted は abortSession の成功後（.onSuccess 内）に呼ぶように変更してください。
660b760 to
4ded110
Compare
| current.copy( | ||
| activeSessionIds = current.activeSessionIds - sessionId, | ||
| mutedSessionIds = current.mutedSessionIds + sessionId, | ||
| ) |
There was a problem hiding this comment.
[bug · medium]
markSessionAborted は SessionError ハンドラ（settledSessionIds + sessionId と mutedSessionIds + sessionId を同時に設定）と異なり、settledSessionIds を更新していません。abort 後に遅延して届く MessagePartUpdated / MessagePartDelta / MessageUpdated は settled 判定（sessionId in current.settledSessionIds）に該当しないため、abort 済みセッションが activeSessionIds に再追加され、ドロワーの「実行中」表示が復活してしまいます。SessionError と同様に settledSessionIds も同時に設定することをお勧めします（markSessionRunning が次の run 開始時に解除するため、再開は妨げません）。
Suggestion:
| current.copy( | |
| activeSessionIds = current.activeSessionIds - sessionId, | |
| mutedSessionIds = current.mutedSessionIds + sessionId, | |
| ) | |
| current.copy( | |
| activeSessionIds = current.activeSessionIds - sessionId, | |
| settledSessionIds = current.settledSessionIds + sessionId, | |
| mutedSessionIds = current.mutedSessionIds + sessionId, | |
| ) |
| runCatching { target.session(event.sessionId).parentId != null } | ||
| .getOrDefault(false) | ||
| if (!isSubagent) { | ||
| if (!notify) return |
There was a problem hiding this comment.
[bug · low]
mute の解除は SessionIdle / SessionStatusChanged / markSessionRunning の3箇所のみです。しかし実行中に新メッセージを送る割り込みフローでは、markSessionAborted は呼ばれる一方で isRunning が遷移しないため markSessionRunning は再呼び出しされず、ローカルランタイムは SessionStatusChanged も emit しません。そのため、割り込まれたターンが trailing SessionIdle を emit しないケースでは mute が次の run の完了通知まで残り、この return が onSessionIdle を飲み込むことになります（テスト a muted session notifies again once a new run starts は SessionStatusChanged で解除する前提ですが、ローカルランタイムではこのイベントは発生しません）。次の run 開始時に必ず mute が解除されること（例: 割り込み時は trailing idle のみを対象にした一回限りの mute にする、あるいは run 開始パスで明示的に解除する）を確認・テストで担保してください。
| events.tryEmit( | ||
| OpenCodeEvent.SessionError(sessionId, messages.processExited(exitCode, streamFailure?.message)), | ||
| ) |
There was a problem hiding this comment.
[bug · low]
sessions マップからの削除（synchronized ブロック）の後に、ロック外で SessionError/SessionIdle を送出しています。reader のクリーンアップ処理中に別スレッドの send() が同じ sessionId で新しいプロセスを作成した場合、古いプロセスの失敗イベントが新しい実行に誤って紐づき、誤ったエラー表示や早期の idle 通知を引き起こす可能性があります。イベント送出を削除と同一の同期ブロック内で行うか、送出直前に sessions[sessionId]?.process === process を再確認してから送出することを推奨します。
| events.tryEmit( | ||
| OpenCodeEvent.SessionError(sessionId, messages.processExited(exitCode, streamFailure?.message)), | ||
| ) |
There was a problem hiding this comment.
[bug · low]
sessions マップからの削除（synchronized ブロック）の後に、ロック外で SessionError/SessionIdle を送出しています。reader のクリーンアップ処理中に別スレッドの send() が同じ sessionId で新しいプロセスを作成した場合、古いプロセスの失敗イベントが新しい実行に誤って紐づき、誤ったエラー表示や早期の idle 通知を引き起こす可能性があります。イベント送出を削除と同一の同期ブロック内で行うか、送出直前に sessions[sessionId]?.process === process を再確認してから送出することを推奨します。
… completed The completion notification fired on every SessionIdle, including the idle that trails an abort, an error, a mid-turn CLI death, and subagent idles whose parent lookup failed. Mute the notification for a run that was aborted or errored until its trailing idle is consumed, fail closed when the subagent check cannot resolve the session, and surface a mid-turn Claude process exit as an error instead of a bare idle.
4ded110 to
16d716e
Compare
| activeSessionIds = current.activeSessionIds - sessionId, | ||
| mutedSessionIds = current.mutedSessionIds + sessionId, |
There was a problem hiding this comment.
[bug · medium]
markSessionAborted は呼び出し元（ChatViewModel.abort / 割り込み送信）で abortSession や sendMessage の成否に関わらず先に実行されます。abort が失敗したりランタイムが中断を無視して run が実際には継続している場合でも、ここで即座に activeSessionIds から除去され、drawer は停止状態を表示する一方、後続の SessionIdle が muted 扱いとなり完了通知も抑止されます。実際に run の終了が確認できてから muted へ移す（または abort 失敗時に状態を戻す）など、実行終了を確定させてから状態を変更する設計を検討してください。
| activeSessionIds = current.activeSessionIds - event.sessionId, | ||
| completedSessionIds = current.completedSessionIds + event.sessionId, | ||
| settledSessionIds = current.settledSessionIds + event.sessionId, | ||
| mutedSessionIds = current.mutedSessionIds - event.sessionId, |
There was a problem hiding this comment.
[maintainability · low]
mutedSessionIds のエントリは、同じセッションが以後 SessionIdle / SessionStatusChanged / markSessionRunning でクリアされるまで残り続けます。abort 直後に接続が切れるなど、以後そのセッションのイベントが一切来ないケースではエントリが残り続け、ランタイム切断時のリセットでも activeSessionIds と permissions しか消えないため累積します。さらに、古い muted エントリが残った状態で同じセッションが（markSessionRunning を伴わないイベント起点で）再実行されると、本来通知すべき完了の SessionIdle が誤って抑止されるリスクがあります。切断時・ランタイム切替時や新規 run 開始時に muted を解除するなど、ライフサイクルを明確にすることを推奨します。
1 participant
完了通知が「作業の完了」以外のidleでも飛んでいたケースを修正する。
mute は one-shot で、trailing idle か新しいラン (markSessionRunning / busy status) で解除される。
Tests: RuntimeActivityRepositoryTest (+4), ChatViewModelTest (+1), ClaudeStreamJsonParserTest (+1). testDebugUnitTest / lintDebug / detekt / spotlessCheck 通過済み。
Pre-PR review
判定: APPROVE
ブロッカー
提案（非ブロッキング）
RuntimeActivityRepository.kt—var notifyをmutableState.update{}のラムダ内で副作用として書き換えている。updateの CAS リトライで最後に実行されたラムダの値が採用されるため結果は正しいが、慣習として外部変数への副作用は読みにくい。mutedSessionIdsを退出前のスナップショットとして別途比較する構成にすると追跡性が上がる（振る舞いは等価）。ClaudeCodeRuntime.kt— リーダーJobの終了処理で、プロセスが mid-turn で終了し、かつ直後に同じ sessionId で新プロセスが start されたレアケースでは、旧Jobが発するSessionErrorが新ランのUIに一時的に表示されうる。mute機構で後続idle通知は正しく抑止されるため通知面は無害だが、errorメッセージが一瞬出うる。実害は小さい。ChatViewModel.kt— 中断送信時にonSessionAbortedを呼ぶが、Claudeバックエンドでは明示的なabortなしに新プロンプトがstdin追記される。Claudeが旧ターンを完了してから新ターンを処理する限り二重idleとなり最初がmute・二つ目が通知される設計は正しい。CLIが単一idleにマージするフローはないと想定するが、ドッグフーディングで要観察。チェック済み項目