Repository navigation
fix(runtime): stop the main thread and startForeground waiting on local-runtime work - #412
Merged
Merged
Conversation
…al-runtime work Crashlytics (14 days, v1.2.27) shows two groups of failures that share one cause, slow local-runtime work running where it must not: ANRs with the main thread parked in our own code - LocalRuntimeAccessCoordinator.write (2 users): Compose reads ClaudeCodeTarget.capabilities on the main thread. Its getter went permissionBridgeReady() -> installedRuntime(), which takes the runtime *write* lock and extracts the command suite, so the UI froze behind any running install. The hook check only needs the files, so it now reads them directly, without the lock. - LocalRuntimeManager.readMetadata (1 user): ChatViewModel calls the local backend from the main thread and every call re-read the metadata and the server password from disk to find the delegate. The resolution now runs on Dispatchers.IO; the request itself stays where the caller put it. ForegroundServiceDidNotStartInTimeException (LocalRuntimeService x2, CodexKeepAliveService x1) The reports do not say why startForeground was missed, so this closes the two defects the code shows rather than guessing at one: - LocalRuntimeService.onCreate called manager.status() to build the first notification. That reads metadata and probes the server's port (up to 2.5 s) on the main thread, inside the platform's deadline. It now uses the last known status and refreshes it on a worker. - startForeground was only called from onCreate. After a Stop, Delete or idle stop the instance outlives stopForeground() until its own stopSelf() lands, and a start arriving in that gap is held to the same deadline without onCreate running again. onStartCommand now answers every start, in both services. Tests: ClaudePermissionBridgeReadinessTest reproduces the lock wait with a thread holding the write lock; LocalOpenCodeBackendThreadingTest asserts the port and password are read off the caller's thread. Both failed before the change. The service lifecycle itself is not unit-testable here. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ckend.events() The previous commit made events() resolve the local server inside the flow instead of while the flow is built. Pre-PR review asked for that change in timing to be covered before merge: - events() alone does not call the port or password providers; collecting does, on a worker thread rather than the collector's. - a runtime that is not installed now fails the stream at collection, where the .catch callers chain after events() can handle it, instead of throwing while the flow is being built. Test-only; production code is unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes the ANRs and
ForegroundServiceDidNotStartInTimeExceptioncrashes that Crashlytics reports for v1.2.27 (last 14 days). They share one cause: slow local-runtime work running on the main thread, or ahead ofstartForeground(), where it must not.LocalRuntimeAccessCoordinator.write(2 issues)ClaudeCodeTarget.capabilitieson the main thread. Its getter wentpermissionBridgeReady()→installedRuntime(), which takes the runtime write lock and extracts the command suite, so the UI froze behind any running install.LocalRuntimeManager.readMetadataLocalOpenCodeBackendcall re-read the runtime metadata and the server password from disk to find its delegate, andChatViewModelcalls it from the main thread.ForegroundServiceDidNotStartInTimeException—LocalRuntimeService(2 issues),CodexKeepAliveService(1)The main-thread stacks for the first two rows were read from the ANR events, so those causes are confirmed. For the foreground-service crashes the reports do not say why
startForeground()was missed, so this closes the two defects the code shows instead of guessing at one:LocalRuntimeService.onCreatecalledmanager.status()to build the first notification. That reads metadata and probes the server's port (up to 2.5 s) on the main thread, inside the platform's deadline.startForeground()was only called fromonCreate. After a Stop, Delete or idle stop the instance outlivesstopForeground()until its ownstopSelf()lands, and a start arriving in that gap is held to the same deadline withoutonCreaterunning again.What changed
ClaudeCodeRuntime.permissionBridgeReady()looks at the hook files directly instead of going through the installer: no lock, no extraction. A file caught mid-write reads as "not ready".LocalOpenCodeBackendresolves its delegate throughbackend()onDispatchers.IO. Only the resolution moves; the request itself still runs where the caller put it.events()now resolves lazily inside the flow, so a runtime that is not installed fails the stream where callers'.catchcan handle it instead of throwing while the flow is built.LocalRuntimeServicebuilds its first notification frommanager.state.value(no I/O) and refreshes the status on a worker. Both services callstartForeground()on everyonStartCommand.Not in this PR (known follow-ups)
From the review, none blocking:
LocalRuntimeManager.status()can now overlap an operation that the next command launches, so its write can briefly overwrite a fresherStartingwith a staleStopped. Cosmetic and corrected by the watchdog's next tick;compareAndSetwould close it.RestoreandInstallFullDevelopmentToolsinonStartCommand, and severalLocalRuntimeTargetmethods, still callmanager.status()(a port probe plus a metadata read) on the calling thread. That is afterstartForeground()so it cannot cause the crash, but it can still block the main thread.CodexKeepAliveServicelogs twice when entering the foreground fails, once fromonCreateand once fromonStartCommand."environment/rootfs"is duplicated across the repo; a shared constant would stop a layout change from silently turning the readiness check intofalse.Test plan
./gradlew spotlessCheck detekt :app:testGithubDebugUnitTest— 1623 tests, 0 failures. New:ClaudePermissionBridgeReadinessTest(4),LocalOpenCodeBackendThreadingTest(3). The lock-wait and thread tests were written first and failed before the change (TimeoutExceptionwhile another thread holds the write lock; providers running on the caller's thread)../gradlew :app:assembleGithubDebug :app:lintGithubDebug— 0 errors, no findings in the changed filesService). Worth a manual check that the runtime still starts, stops and restores, including a Stop immediately followed by a start.Pre-PR review
The
repo-reviewersubagent is defined for OpenCode (.opencode/agents/repo-reviewer.md) and is not available in the Claude Code session that produced this change. The review below was done by a read-only Claude Code subagent given that definition as its role and output format. It is a stand-in, not therepo-reviewersubagent itself.It reviewed commit
28b278d9. The branch has one further commit, test-only, that adds the twoevents()tests the reviewer recommended before merge; no production code differs between the reviewed commit andHEAD.Pre-PR review
判定: APPROVE
対象: ブランチ fix-fgs-start-timeout(コミット 28b278d の1件、
origin/mainの先頭を祖先に持つ)。ブロッカー
なし
提案(非ブロッキング)
events()の遅延解決)— 挙動変更(解決のタイミングとスレッド、失敗が surfaced するタイミング)に対するテストがない。LocalOpenCodeBackendThreadingTest はanswerQuestion1本だけを検証している。JVM だけでテストできるので、次の2点を追加すると堅くなる。マージ前を推奨する。events()を呼んだだけではportProvider/passwordProviderが呼ばれず、collect で初めて IO スレッド上で呼ばれること。portProviderが null)のとき、エラーがevents()の呼び出し時ではなく collect 時に届き、.catch {}で捕捉できること。status())と LocalRuntimeService.kt:327 — 今回 onCreate のmanager.status()を IO に移したことで、後続コマンドの操作と並行する可能性が新たに生じた。status()がInstalling/Starting/Updating以外の状態を読んだ直後に、操作側がStartingを書き込み、その後computeStatus()の古い結果(Stoppedなど)で上書きされうる。mutableState.compareAndSet(operation, computed)にするか、status()の書き込みをoperationMutex外で行わない形にすると安全。InstallFullDevelopmentTools)と LocalRuntimeService.kt:444(Restore)のmanager.status()は、enterForeground()の後なので FGS 期限には影響しないが、onStartCommand(メイン)では依然としてブロックする。launchOperationの中に移すか、manager.state.valueを使えば避けられる。runtimeManager.status()も呼び出しスレッドで実行される。connect()は ChatViewModel 系の呼び出し元のスレッドで動く。AntigravityTarget と ClaudeCodeTarget のconnect()は、同じ理由でwithContext(IO)に包んである。onCreateとonStartCommand)—onCreateでenterForeground()が失敗してstopSelf()した場合でも、続けて届くonStartCommandでもう一度enterForeground()とstopSelf()が走る。ログが二重になるだけで実害はない。LocalRuntimeService のようにinForegroundガードを持たせると揃う。stopForegroundを呼ぶ経路がない。AMS は「すでに foreground のインスタンスへの2回目のstartForegroundService」ではstartForegroundの要求を無視するので、「実行中のインスタンスにも応える」という追加は主に防御的で、現状の実害はない。ACTIVE_ROOTFS)—"environment/rootfs"のリテラルがリポジトリ内で既に8箇所以上重複していて、WorkspaceViewModel:403 にはRUNTIME_ROOTFS_PATHもある。既存の流儀と揃っているのでブロッカーではないが、定数を共通化すると、LocalRuntimeInstaller のレイアウト変更で読み取り側が黙って false を返すドリフトをコンパイル時に防げる。テスト側(rootfs())も同じリテラルをハードコードしているため、いまはドリフトを検出できない。チェック済み項目
LocalRuntimeInstaller.installedRuntime()の rootfs はFile(File(runtimeDirectory,"environment"),"rootfs")で、ClaudeCodeRuntime のruntimeDirectoryは AndCodeApplication.kt:331 でインストーラと同じものを受け取っている。LocalRuntimeManager.computeStatus(:622)や ClaudeCodeTarget:237 も同じリテラルを使っている。environment.stagingに展開され、アクティブな環境が切り替わるのはactivateRuntimeEnvironmentの rename の瞬間だけ。ただしinstallFullDevelopmentToolsや Claude のインストールは、アクティブな rootfs を書き込みロック下でその場で変更する。この途中で読むと、フック欠落や設定ファイルの途中書きで false になる。runCatchingで false に畳まれる。フックのFile.isFile/readTextは旧実装も読んでいた同じ処理。capabilitiesが参照する機能は Claude Code が動くときだけ意味を持つので影響はない。②旧実装は呼び出し時にnormalizeRuntimeSymlinksとensureAndCodeAgentContextの副作用が付いてきたが、新実装にはない。フックのパス(usr/local/bin/...、root/.claude/settings.json)に内部シンボリックリンクの正規化が必要なレイアウトは通常ない。capabilitiesは Compose 上で observable ではないため、切替中に一度 false を読むと、次の再コンポーズまで引きずられる可能性がある。UI のsupportsPermissionsが一時的に違う表示になるだけで、ロック待ちの ANR と比べて許容できる。capabilitiesの getter は、Claude 以外は定数(Codex・Antigravity)か、キャッシュ済みのbackend.protocolを読むだけ(Local・Remote)で、ロックやディスクには触れない。ClaudeCodeTarget のconnect・health・commands・skillsはwithContext(IO)済み。installedRuntime()を呼ぶ他の呼び出し元(AndCodeApplication.kt:442 など)は、いずれも IO 側かコレクタ内。backend()化:delegate()を直接呼ぶ箇所は、定義(:63)以外にない(grep 済み)。internalのdelegate()自体は同期のまま。テストは通っている。withContextは元の例外を再スローし、runCatchingを使う呼び出し元(ChatViewModel の health ループ、LocalRuntimeTarget.connect:112 など)の挙動も変わらない。リクエスト本体は呼び出し元のディスパッチャで実行される。events()はbackend()を完了させてからemitAllするので、フローのコンテキスト保存不変条件にも違反しない。ScheduleExecutionService のCompletionSignalによる collect 中断も、そのまま伝播する。events()の呼び出し元4箇所(ChatViewModel:778、ScheduleExecutionService:510、RuntimeActivityRepository:210、AndCodeVoiceSession:227)を実際に開いて確認した。どれも失敗を collect 側で受けられる。ChatViewModelとAndCodeVoiceSessionは.catchがevents()の後ろに付く構成で、旧実装では未インストール時の同期 throw が.catchの外に出ていたが、新実装では.catchで捕捉される。ScheduleExecutionServiceはtryの中、RuntimeActivityRepositoryはflow { emitAll(...) }.retryWhenの中なので、旧実装・新実装とも捕捉される。つまり例外処理は退行せず、むしろ堅くなっている。delegate()はこれまでメインで直列に呼ばれていたが、IO 上では並行に呼ばれる。内部は二重チェックロックで安全。LocalServerAuth.passwordも生成時はsynchronized。enterForeground():inForegroundの意味は「onCreate での昇格に成功した」から、「直近のstartForegroundが成功した」に変わった。ただし読み取りは LocalRuntimeService.kt:322 と :377 の2箇所だけで、stopForeground(REMOVE)で false に戻さなくても、onStartCommand が毎回再アサートするので矛盾しない。inForeground=false)の場合は||の左辺で即stopSelf()するため、コレクタやウォッチドッグが未起動のままenterForeground()を再試行して中途半端に動く事態はない。enterForeground()を通るため、manager.state.valueで一度通知が再投稿され、その後にstopForeground(REMOVE)で消える。ユーザーが FGS 通知をスワイプで消していた場合は復活する。stopForeground(REMOVE)からstopSelf()が着地するまでの隙間に届いた Start は、再昇格できて期限切れは避けられる。その後stopSelf()が効くと Start 自体は取り消される。これは旧実装から続く競合で、今回の変更で悪化はしていない。startForegroundが再入で失敗した場合は、inForeground=falseにしてstopSelf()する。実行中の操作は onDestroy で中断される。ただし、すでに foreground のサービスへの2回目のstartForegroundが投げる経路は現実的にほぼなく、期限切れで殺されるよりは安全側の判断。manager.state.valueが古い場合の通知内容:stateの初期値はMutableStateFlow(computeStatus())(LocalRuntimeManager.kt:77)で、アプリ起動時点のものなので、新規プロセスでは新鮮。長寿命プロセスでも、稼働中はウォッチドッグがstatus()で更新し続ける。プロセスが生きたままサービスだけが止まっていた後は、古い Ready が一瞬表示されうるが、onCreate の IOstatus()(:327)が更新し、:329-336 のコレクタが再描画する。FGS の種類はspecialUseで、dataSyncのようなタイムアウトを持たず、onTimeoutの実装も不要。manager.status()の効果は、上記の「更新と再描画」だけ。コレクタは IO の launch より先に購読される(Main.immediate、かつ同一 onCreate 内)。errorで落ちる)、フック無し・未インストールで false、書き込みロックを別スレッドが保持している間もメインに相当するスレッドが3秒以内に応答する(旧実装はcoordinator.writeを踏み、get(3s)で TimeoutException になる)。finallyで必ずロックを解放して後始末している。portProviderとpasswordProviderが呼び出しスレッド以外で実行されることを検証していて、旧実装では落ちる設計。ただしevents()は未検証(上記の提案)。scripts/new-worktree.shの流儀(.worktree/配下、origin/main基点)。spotless・detekt・単体テスト(1621件)は依頼文の報告に依拠しており、再実行はしていない(読み取り専用のため)。assembleGithubDebug と lintGithubDebug は実行中とのことなので、マージ前に緑であることを確認してほしい。🤖 Generated with Claude Code