fix(desktop): restart v1 terminal stream subscription on reconnect#174
fix(desktop): restart v1 terminal stream subscription on reconnect#174
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughuseTerminalColdRestore.tsの Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codex 独立検証での指摘に対応 (
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
MocA-Love
force-pushed
the
fix/v1-terminal-retry-reconnect-stream
branch
from
9da543d to
d329b76
Compare
Codex 再レビュー対応 (`e1e8d7889`)Codex の独立コードベース分析で、前回の修正 (gate 削除) が PR #167 の invariant を破る との指摘:
具体的問題:
修正`!result.isColdRestore` gate を復活させました: ```diff
なぜ前回の Codex 指摘 (gate 狭い) と矛盾しないか前回 Codex は `handleStartShell` 側に stream 再開処理が無い問題を指摘していました。PR #167 が main にマージされた際、handleStartShell.onSuccess に v1TerminalCache.markSessionReady() が追加されました (`useTerminalColdRestore.ts:219`)。 そのため cold-restore reconnect 経路は:
end-to-end でカバーされているので、handleRetryConnection 側で gate 復活させても抜け落ちはありません。 検証
|
When the cache-owned stream subscription dies (v1-terminal-cache.ts onError nulls the subscription and resets streamReady), only the initial attach path in useTerminalLifecycle re-runs startStream / setStreamReady / markTerminalSessionReady. handleRetryConnection was a standalone re-attach: it happily succeeded at the tRPC level, wrote a "[Reconnected]" marker to xterm, and left the cache-owned stream in a dead state. The resulting terminal looked healthy but never received another byte of stdout / exit / error. Long-running tail -f processes, dev servers, and shells waiting on idle commands silently stopped updating and the user had to restart the pane (or the whole app) to notice. The failure mode was especially pernicious for background builds — "[Reconnected]" gave the impression that recovery had worked. Restart the cache subscription and mark readiness at the end of the retry success path, mirroring the initial attach path. Gate the call on !result.isColdRestore: cold-restore returns from main without a real session, and the existing cold-restore block handles its own bookkeeping (and the replacement shell will run through handleStartShell, which already needs its own readiness plumbing — addressed separately). FORK NOTE: upstream regression. Revisit when upstream ships a coherent retry / cache-reset contract.
Codex review (PR#174): the previous unconditional markSessionReady() call in handleRetryConnection.onSuccess broke the PR #167 invariant that the cache must not flip to streamReady=true before a real backend session exists. Cold-restore reconnect responses come back with isColdRestore=true and no backend; setting the cache ready in that state lets a subsequent tab-switch remount take the isReattach fast-path and silently drop user keystrokes — exactly the bug PR #167 was meant to fix. Restore the !result.isColdRestore gate. The cold-restore reconnect path is still wired end-to-end because handleStartShell.onSuccess (in main, after PR #167) calls markSessionReady once the replacement shell spawns.
MocA-Love
force-pushed
the
fix/v1-terminal-retry-reconnect-stream
branch
from
e1e8d78 to
b013578
Compare
1 participant
症状
接続エラー発生後の自動リトライで、見た目は `[Reconnected]` と表示されて成功するのに、その後の stdout / exit / error イベントが 一切届かない 状態になる。
ユーザーから見ると:
ターミナル自体はアクティブで入力もできるが、返事が一切来ない。pane を閉じて開き直すまで復旧しない。
原因
`v1-terminal-cache.ts:231` で subscription がエラー終了すると、cache は subscription=null + streamReady=false にリセットされる:
```ts
entry.subscription = electronTrpcClient.terminal.stream.subscribe(paneId, {
// ...
onError: () => {
entry.subscription = null;
entry.streamReady = false;
// renderer 側に errorHandler 経由で通知
},
});
```
その後 `Terminal.tsx:287` の auto-retry `useEffect` が `handleRetryConnection` を呼ぶ。しかしこの関数は initial attach の `useTerminalLifecycle` と違って、`startStream` / `setStreamReady` / `markTerminalSessionReady` を一切呼ばない。
結果:
コード比較
initial attach (`useTerminalLifecycle.ts` onSuccess):
```ts
v1TerminalCache.startStream(paneId); // ← subscription 再生成
v1TerminalCache.setStreamReady(paneId);
markTerminalSessionReady(paneId);
```
retry reconnect (`useTerminalColdRestore.ts` handleRetryConnection.onSuccess):
```ts
setConnectionError(null);
currentXterm.writeln("[Reconnected]");
// ❌ startStream が呼ばれていない
```
再現手順
UX 影響
修正内容
`handleRetryConnection.onSuccess` で initial attach と同じ 3 ステップ を呼ぶ。ただし cold-restore 経路は独自のフローを持つので gate する。
```diff
setConnectionError(null);
currentXterm.writeln("\x1b[90m[Reconnected]\x1b[0m");
+if (!result.isColdRestore) {
+}
if (result.isColdRestore) {
// ... 既存の cold restore 分岐 ...
}
```
既存機能への影響
退化なし
新たなリスク
関連問題
PR#173 (cold restore snapshot の早期破棄) とは独立。両方マージされても衝突しない (変更箇所が `handleRetryConnection` と `handleStartShell` で別関数)。
検証
FORK NOTE
upstream (superset-sh/superset) superset-sh#3348 由来の regression。upstream 側の修正が出たら合わせる想定。
Summary by CodeRabbit
リリースノート