Repository navigation
diagnose(shell): settle the owned desktop before reporting a leak (#160) - #199
Conversation
#160 Close keeps its success contract; on failure it retries desktop_gone for up to 500 ms and records the attempts, so an artifact shows whether the desktop was still held by exiting children or genuinely left behind.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46c49eab1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| # one; settle briefly and keep the attempts so a failure shows whether the | ||
| # desktop was still held or never closed. Nothing is added on success. | ||
| attempts = [{"elapsed_ms": 0, "gone": False}] | ||
| began = time.monotonic() |
There was a problem hiding this comment.
When the first desktop_gone() check returns false, this call raises NameError: name 'time' is not defined because the module never imports time. This prevents all retries and leaves desktop_close_settle unwritten—the exact transient-desktop scenario this change is intended to diagnose still fails, now without the intended evidence. In the cleanup-only call site, the exception also bypasses the except OSError handler and can prevent sample.json from being saved. This is reproducible by mocking desktop_gone to return false and calling close({}).
Useful? React with 👍 / 👎.
内容
针对 #160 的"
owned desktop remained"，在夹具的 desktop 关闭路径上增加仅失败时的沉降观测：OwnedTestDesktop.close(record)：CloseDesktop成功后若第一轮desktop_gone仍为 false，则以 50 ms 间隔最多重试 10 次（≤500 ms）；elapsed_ms / gone写入record["desktop_close_settle"]；成功路径不写任何新字段，不影响通过记录的既有结构；现场依据
新样本（run 40932322420 的
shell测试）：{"timed_out":true,"job_at_timeout":{"active":5,"total":6,"terminated":0}, "desktop_gone":false,"error":"owned desktop remained", "job_after":{"active":0,"total":6,"terminated":0}}超时瞬间 Job 仍有 5 个活动进程且
terminated:0；清理后 Job 归零但 desktop 仍可打开。一个合理解释是 关闭句柄时子进程尚未完全退出、仍持有 desktop 句柄，因此需要"沉降后再判定"，而不是立即判定为泄漏。本地验证
边界
Refs #160