Skip to content

test(remote-agent): groupRelay 用例清理临时目录时重试,避免与后台写入竞态 - #5776

Open
FicoHub wants to merge 4 commits into
makecindy:mainfrom
FicoHub:fix/group-relay-test-cleanup
Open

FicoHub wants to merge 4 commits into
makecindy:mainfrom
FicoHub:fix/group-relay-test-cleanup

Conversation

@FicoHub

@FicoHub FicoHub commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

这次改了什么

摘要

apps/desktop/src/main/remote-agent/__tests__/groupRelay.test.ts 的 afterEach 用 fs.rmSync(root, { recursive: true, force: true }) 清理临时目录。用例里的 host 会异步向 runsRoot 写入运行目录和 guest-sessions.json(tmp + rename)。用例结束时还有写入在落盘,rmdir 就会报 ENOTEMPTY,让一个断言本已全部通过的用例失败。

本 PR 只改这个 afterEach,不改任何断言,也不改产品代码:

  • 改为异步删除(fs.promises.rm,带少量重试),删除期间不阻塞事件循环,在途的后台写入可以完成;
  • 单次删除失败(rm 用尽内部重试后 reject)不会让 hook 失败,交给外层循环继续重试;
  • 删除后等待 50ms,连续两个窗口内 root 都不存在才结束;被重建就再删一次,最多 40 次;
  • 仍被重建就直接报错,不会悄悄把目录留在 tmpdir。
  • 这仍是测试侧的启发式等待;彻底解决需要给 host 加 flush/dispose,见下方「明确不包含」。

第一版只给 rmSync 加了重试。review 指出(PRRT_kwDOTgdRUs6rJOW5):同步删除若抢在写入之前完成,写入落地后会重建目录并遗留。本机核实属实:$TMPDIR 里留有 9 个旧版本跑出的 cindy-group-relay-* 目录;新版连续跑 4 次没有新增遗留。

变更类型

  • test 测试

范围

  • 关联 Issue / 需求:无单独 issue。CI 证据如下,同一失败已在两个无关 PR 上出现:
  • 本 PR 包含:上述 afterEach 改为异步删除,并确认目录不再被重建。
  • 明确不包含:给 host 加 flush/dispose 之类的等待接口。runHost.ts 里写 runsRoot 的后台链路约 20 处(recordGuestSession、startRun 的影子工作区与附件目录、finishRun/disposeRun、retryPendingForgets 等),要逐一跟踪才能真正 flush,属于供应商组 host 生命周期的产品改动,留给维护者决定(review 线程 PRRT_kwDOTgdRUs6rJizN)。
  • 用户可见变化:无。
  • 是否存在 breaking change:无

UI 变化

不涉及

  • 引用的设计规范:不涉及

怎么验证的

自动验证

apps/desktop: npx vitest run src/main/remote-agent/__tests__/groupRelay.test.ts(4 进程并行 + 串行 3 次)→ 每次 22 passed,跑后 $TMPDIR 无新增 cindy-group-relay-* 目录
apps/desktop: tsc --noEmit -p tsconfig.json → 0 错误
eslint(改动文件)→ 无错误;pnpm check:dco → passed

手工验证

不涉及

未执行的验证

本机无法稳定复现这个竞态(之前单独跑 6 次都通过,失败只出现在负载较高的 CI 上),所以没有「修复前必失败」的本地反证;依据是 CI 日志与 Node rmSync 重试语义。

风险

风险分类

  • 无已知风险

影响与回滚

  • 影响范围:仅这一个测试文件的清理步骤。
  • 回滚 / 降级方式:revert 本 commit。

提交前检查

  • 已 review 完整 diff
  • 每个 commit 都带 DCO 签名(git commit -s,见 DCO)
  • UI 改动已在「UI 变化」注明引用的设计规范章节(不涉及 UI 则跳过)
  • 未提交凭证、令牌或授权文件
  • 已核对受影响的文档,行为变化涉及的旧结论已同步修订(不涉及则无需修改文档)
  • 已确认测试结果或说明未执行原因

供应商组中转测试的 host 会异步写入 runsRoot(运行目录与 guest-sessions.json 的
tmp+rename),用例结束时仍在落盘的写入会让 afterEach 的 rmSync 报 ENOTEMPTY,
使本已通过的用例失败(CI 中已在两个无关 PR 上复现)。清理改用 rmSync 自带的
重试(maxRetries/retryDelay),不改任何断言。

Signed-off-by: ficowang <fico@xd.com>
@FicoHub
FicoHub requested a review from a team as a code owner October 11, 2026 01:05
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-11T06:05:24.427381Z 58b0c48 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@greptile-apps

greptile-apps Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Low impact] 本 PR 可以合并,未发现新增的行为错误或规则违反。

Summary

本 PR 仅为 groupRelay.test.ts 的临时目录清理增加有限次数的重试,以减少后台写入导致的偶发失败。

  • Group relay tests retry temporary-folder cleanup after write collisions.

Reviews (1) · Last reviewed commit: "test(remote-agent): groupRelay 用例清理临时目录时..." · Reviewed by Greptile

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: beff558028

ℹ️ 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".

Comment thread apps/desktop/src/main/remote-agent/__tests__/groupRelay.test.ts Outdated
rmSync 带重试只能避开 ENOTEMPTY,却会阻塞事件循环;删除若抢在后台
写入之前完成,写入落地后又会重建临时目录并遗留在 tmpdir。改为异步
删除、让在途写入完成,确认目录在静默窗口内不再出现才结束;反复被
重建则直接报错,不再悄悄遗留。

Signed-off-by: ficowang <fico@xd.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6f9aaaec0

ℹ️ 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".

Comment on lines +216 to +218
await fs.promises.rm(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 20 });
await delay(50);
if (!fs.existsSync(root)) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 等待 host 落盘完成再判定目录已清空

在高负载 CI 或线程池拥塞时,runHost.ts 通过 void recordGuestSession(...) 启动的链可能在 readFile/mkdir 上停留超过 50 ms,所以一次 existsSync 为 false 并不能证明它不会随后重建 root。相较已有评论所针对的同步版本,这次修订出现的新证据是退出条件仍只是固定 50 ms 的静默窗口,且这里的 rm 若耗尽 3 次内部重试会直接 reject,20 次外层循环也不会继续;因此同一场景仍会静默遗留目录或使 hook 失败。请暴露并 await host 的 flush/异步 dispose,再做最终删除与断言。

AGENTS.md reference: AGENTS.md:L40-L41

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

两点都核实了。

已在 edf3131 修正:

  • rm 用尽内部重试后会 reject,导致 hook 直接失败、外层循环不再继续。现在单次失败会被吞掉,交给外层循环重试。
  • 退出条件从「一次检查不存在」改为「连续两个 50ms 窗口内都不存在」,最多 40 次;仍被重建就报错。

仍是启发式,这点我同意。 慢速写入如果在判定之后才落地,理论上仍可能遗留目录。

要彻底解决,需要给 host 加 flush 或异步 dispose。不过 runHost.ts 里写 runsRoot 的后台链路不止 recordGuestSession,还有:

  • startRun 的影子工作区与附件目录;
  • finishRun / disposeRun;
  • retryPendingForgets 等约 20 处 void 调用。

真正的 flush 要逐一跟踪这些链路,属于供应商组 host 生命周期的产品代码改动,超出这个测试清理 PR 的范围。所以本 PR 只在测试侧把窗口收紧。是否给 host 加 whenIdle() 之类的接口,留给维护者决定;需要的话我另开 PR。这条先不 resolve。

本机验证:4 个进程并行跑、再串行跑 3 次,每次 22 个用例全部通过,$TMPDIR 没有新增遗留;eslint 通过。

rm 用尽内部重试后 reject 会直接让 hook 失败、外层循环不再继续;改为
吞掉单次失败交给外层循环重试。退出条件从一次检查改为连续两个 50ms
窗口内目录都不存在,降低慢速写入在判定之后才重建目录的概率。

Signed-off-by: ficowang <fico@xd.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: edf31311af

ℹ️ 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".

Comment on lines +217 to +220
for (let attempt = 0; attempt < 40 && absentChecks < 2; attempt += 1) {
await fs.promises.rm(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 20 }).catch(() => undefined);
await delay(50);
absentChecks = fs.existsSync(root) ? 0 : absentChecks + 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 在判定清理完成前排空后台写入

在高负载 CI 让 guest-index I/O 超过 100 ms 时,这里仍能在两个 50 ms 采样后返回:runHost.ts 的 startRelay() 会先发布 started、再等待 recordGuestSession(),而 open 路径又以 void ...then(startRelay) 启动它,因此测试和 client.close() 都可能先完成,后台链随后仍会重建 root。相较已有评论检查的版本,最终 diff 的新证据是退出条件虽然改成了 absentChecks < 2,却仍完全不观察 host/promise 生命周期;hook 成功后泄漏的目录会在下一次 beforeEach 覆盖 root 后失去清理机会。请先 await host 的 flush/异步 dispose,再执行最终删除。

AGENTS.md reference: AGENTS.md:L40-L41

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同意:只要清理不观察 host 的 promise 生命周期,它就仍是启发式的;你举的 startRelay() 先发布 started、再等 recordGuestSession() 的路径也属实。

这和 PRRT_kwDOTgdRUs6rJizN 是同一个取舍,我不再继续往测试侧叠等待逻辑:

  • 要真正排空,需要在 runHost.ts 里给后台链路(startRelay/recordGuestSession、影子工作区与附件目录的写入、disposeRun 等)建立统一跟踪,并暴露 whenIdle() 或异步 dispose();
  • 这是供应商组 host 的生命周期设计,这个模块今天还在被维护者频繁改动;
  • 我会等维护者决定:是在本 PR 里加这个接口,还是本 PR 只保留测试侧的缓解、另开 issue 跟进。

在此之前这条线程保持 open。

另:本 head(edf31311a)的 Linux/Windows (2/2) 失败是 remoteAgentRelocationWiring.test.ts「改道的行带上所属电脑」。main 自 7b07864 起同样失败(main client-ci 38107453839),与本 PR 无关;修复在 #5781。

@MagicLizi MagicLizi added the status:ci-failed CI 失败(review-pr 自动维护,仅展示) label Oct 11, 2026
Signed-off-by: ficowang <fico@xd.com>
@FicoHub

FicoHub commented Oct 11, 2026

Copy link
Copy Markdown
Contributor Author

已同步 main(含 #5781),head 为 58b0c48de。上一版 CI 的 Linux/Windows (2/2) 失败是 main 的 remoteAgentRelocationWiring.test.ts,#5781 修好后这次应能消除。

补充一条新证据:#5781 合入后,main 自身的 client-ci(run 38113636656,head 9fb768f)在 Linux unit tests (2/2) 中也失败了,失败的正是这个竞态:

  • 用例:groupRelay.test.ts › provider group relay for shared users › asks the computer to forget a removed shared user, and the computer clears only that user;
  • 错误:ENOTEMPTY: directory not empty, rmdir '/tmp/cindy-group-relay-s2vvzX/mini'。

这是继 #4264、#4263 之后的第三次,这次换了一个用例,并且发生在 main 上。main 目前只剩这一个失败,本 PR 修的正是它。

@MagicLizi MagicLizi removed the status:ci-failed CI 失败(review-pr 自动维护,仅展示) label Oct 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants