fix(debug): recover from failed terminal debug sessions #4687
Conversation
走查（Walkthrough）该PR针对调试会话的启动和终端处理流程进行了改进。在调试会话启动失败时添加了错误捕获和清理机制，防止阻塞会话残留。同时，对终端进程ID的验证逻辑进行了优化，增加了鲁棒的错误处理和清理流程。 变更（Changes）
预计代码审查工作量🎯 3 (中等) | ⏱️ ~20 分钟 可能相关的PR
建议标签
建议审查者
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/debug/src/browser/debug-session.ts (1)
341-377: 新创建的终端无法获取有效 processId 时应抛出错误，而非返回 undefined当终端创建成功但
getValidTerminalProcessId返回undefined时（第 368 行），函数会移除该终端但仍返回{ processId: undefined }。虽然 Debug Adapter Protocol 允许processId为可选项，但当终端创建后无法获取其进程 ID 时，应该抛出错误以表示操作失败，而不是静默返回undefined。这会造成与已激活终端处理逻辑的不对称（行 350-353 处，如果无法获取 processId 会创建新终端，而不是继续使用旧终端）。建议在第 368 行之后添加检查：如果processId为undefined，应抛出错误，这样可以防止调试器使用无效的进程 ID。
🧹 Nitpick comments (2)
packages/debug/src/browser/debug-session-manager.ts (1)
466-472: 考虑记录启动失败的错误错误处理逻辑正确地销毁了失败的会话，但错误信息被静默吞没。建议至少记录错误以便调试，或者在销毁前传递错误上下文。
🔎 建议的改进
session .start() .then(() => this.onDidStartDebugSessionEmitter.fire(session)) - .catch(() => { + .catch((error) => { // 启动失败的时候清空，避免留下阻塞的session + // 注: session.start() 内部已处理错误消息显示 this.destroy(session.id); });packages/debug/src/browser/debug-session.ts (1)
360-374: 简化清理逻辑在 catch 块中使用
getValidTerminalProcessId(createdTerminalId, true)仅为触发终端移除有些间接。可以直接调用removeTerm。🔎 建议的简化
} catch (error) { if (createdTerminalId) { - await this.getValidTerminalProcessId(createdTerminalId, true); + this.terminalService.removeTerm(createdTerminalId); } throw error; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/debug/__tests__/browser/debug-session-manager.test.tspackages/debug/__tests__/browser/debug-session-terminal.test.tspackages/debug/src/browser/debug-session-manager.tspackages/debug/src/browser/debug-session.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/debug/src/browser/debug-session-manager.ts (1)
packages/debug/src/browser/view/debug-view-model.ts (1)
session(71-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (ubuntu-latest, 20.x)
- GitHub Check: build-windows
- GitHub Check: build (macos-latest, 20.x)
- GitHub Check: ubuntu-latest, Node.js 20.x
- GitHub Check: unittest (macos-latest, 18.x, jsdom)
- GitHub Check: unittest (ubuntu-latest, 18.x, jsdom)
- GitHub Check: unittest (macos-latest, 18.x, node)
- GitHub Check: unittest (ubuntu-latest, 18.x, node)
🔇 Additional comments (5)
packages/debug/src/browser/debug-session.ts (1)
323-339: LGTM! 辅助方法设计合理
getValidTerminalProcessId方法职责清晰，正确处理了进程 ID 验证和可选的清理逻辑。错误处理也很完善。packages/debug/__tests__/browser/debug-session-manager.test.ts (2)
171-209: LGTM! 启动失败场景测试完善测试正确地模拟了会话启动失败的场景，并验证了
terminateDebugSession被调用以清理资源。使用setImmediate来等待异步操作完成是合适的做法。
211-228: LGTM! 测试改进提升了可读性将基于回调的测试重构为使用
async/await和 Promise，使代码更清晰易读。packages/debug/__tests__/browser/debug-session-terminal.test.ts (2)
7-11: LGTM! 测试类设计合理通过继承
DebugSession并暴露runInTerminalForTest方法来测试受保护的doRunInTerminal方法是一种合适的测试策略。
50-86: LGTM! 终端处理测试覆盖全面两个测试场景都很重要：
- 替换无效的活动终端 - 验证了清理旧终端并创建新终端的逻辑
- 移除新创建的无效终端 - 验证了创建失败时的清理逻辑
模拟和断言都很准确，很好地覆盖了 PR 中描述的终端清理场景。
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4687 +/- ##
==========================================
+ Coverage 52.66% 52.76% +0.10%
==========================================
Files 1686 1686
Lines 104696 104718 +22
Branches 22749 22756 +7
==========================================
+ Hits 55135 55257 +122
+ Misses 41178 41091 -87
+ Partials 8383 8370 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/next |
|
🎉 PR Next publish successful! 3.9.1-next-1766735800.0 |
Types
Background
cwdleaves a broken integrated terminal and a stale debug session. Subsequent launches reuse the bad terminal/session and fail until the page refreshes.Solution
Changelog
Summary by CodeRabbit
发布说明
Bug 修复
测试
✏️ Tip: You can customize this high-level summary in your review settings.