test(core): wait for cron lock probe takeover

[tt-a1i]

Fixes #5534

Summary

• wait for the cron lock-probe takeover behavior before asserting that a durable job fires
• keep the lock-file assertion, but avoid treating the visible lock write as proof that the scheduler's owner state has settled
• assert that the probe job fires exactly once after takeover

Test plan

• npm --workspace @qwen-code/qwen-code-core test -- src/services/cronScheduler.test.ts
• repeated the same target test 5 times locally
• npx prettier --check packages/core/src/services/cronScheduler.test.ts
• git diff --check

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Hi @tt-a1i — thanks for digging into this flaky test! The fix itself looks like a reasonable approach, but the PR body doesn't follow our required template.

The current body uses ## Summary / ## Test plan / ## AI Assistance Disclosure, but the template requires these sections:

• ## What this PR does
• ## Why it's needed
• ## Reviewer Test Plan (with ### How to verify, ### Evidence (Before & After), ### Tested on)
• ## Risk & Scope
• ## Linked Issues

Could you reformat the body to match? The content is all there — it just needs to be under the right headings so reviewers can find it. Happy to re-run triage once it's updated.

中文说明

@tt-a1i 你好，感谢修复这个不稳定的测试！修复方案本身看起来合理，但 PR 正文没有按照我们的必填模板填写。

当前使用的是 ## Summary / ## Test plan / ## AI Assistance Disclosure，但模板要求以下章节：

• ## What this PR does
• ## Why it's needed
• ## Reviewer Test Plan（包含 ### How to verify、### Evidence (Before & After)、### Tested on）
• ## Risk & Scope
• ## Linked Issues

请把正文调整到对应的标题下，内容都在，只是需要放到正确的位置方便审查。更新后可以重新触发 triage。

— Qwen Code · qwen3.7-max

[wenshao]

Verified — a clean, well-scoped flaky-test fix. The waitFor-wrapped tick correctly handles the race where the lock file write is visible before the probe's .then flips isOwner, and the exact .toEqual(['probe-job']) is safe because the scheduler's lastFiredAt dedup (skips slots ≤ lastFiredAt) prevents the repeated ticks from double-firing. Ran the target test 5× (all green, no flakiness) and the full cronScheduler.test.ts (89/89); CI is green. LGTM! ✅

— claude-opus-4-8 via Qwen Code /qreview

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Re-running triage per maintainer request.

Template was already flagged in the prior review — the maintainer's approval overrides that, so I'm moving past it. Quick note for @tt-a1i: next time, please use the PR template headings (What this PR does, Why it's needed, Reviewer Test Plan, etc.) — it helps reviewers find what they need.

On direction: this is a focused fix for a documented CI flake in the cron scheduler lock probe test. Flaky tests erode CI trust — fixing them is always welcome.

On approach: the change is +6/-4 in a single test file, targeting exactly the race described in the issue. No scope creep, no drive-by refactors. The right size for the problem.

Moving on to code review. 🔍

中文说明

按维护者要求重新跑 triage。

模板问题在上一轮 review 中已经提过了，维护者已批准，所以跳过。@tt-a1i 提醒一下：下次请用 PR 模板 的标题（What this PR does、Why it's needed、Reviewer Test Plan 等），方便审查。

方向：这是针对 已记录的 CI 不稳定问题 的修复，cron scheduler 锁探针测试的竞态条件。不稳定测试会削弱 CI 信任度，修复总是受欢迎的。

方案：改动仅 +6/-4，集中在单个测试文件中，精确针对 issue 描述的竞态条件。没有范围蔓延，没有附带重构。大小合适。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review

The race condition is real and the fix is correct. Here's what's happening:

In enableDurable(), the lock probe's tryAcquireLock() does a file write (lock acquisition), and then in its .then() callback sets this.isOwner = true. The file write can be visible to a concurrent reader before the .then() microtask executes. The old test waited for the lock file to show session-1, then immediately called scheduler.tick() — but isOwner was still false, so tick() skipped the durable job (line 1007: if (job.durable && !this.isOwner) continue).

The fix wraps tick() + the assertion in vi.waitFor(), which retries until the .then() callback has flipped isOwner and the job actually fires. This is safe because tick() on a non-owner scheduler is a no-op for durable jobs — no side effects from repeated calls.

The assertion change from toContain('probe-job') to toEqual(['probe-job']) is strictly better — it catches if the job fires more than once, which would be a bug.

No correctness issues, no security concerns, no AGENTS.md violations. Clean, minimal fix.

Test Results

Ran the specific test 5 times with the PR applied (Linux):

=== Run 1 ===
 ✓ src/services/cronScheduler.test.ts (89 tests | 88 skipped) 74ms
 Test Files  1 passed (1)
      Tests  1 passed | 88 skipped (89)

=== Run 2 ===
 ✓ src/services/cronScheduler.test.ts (89 tests | 88 skipped) 24ms
 Test Files  1 passed (1)
      Tests  1 passed | 88 skipped (89)

=== Run 3 ===
 ✓ src/services/cronScheduler.test.ts (89 tests | 88 skipped) 23ms
 Test Files  1 passed (1)
      Tests  1 passed | 88 skipped (89)

=== Run 4 ===
 ✓ src/services/cronScheduler.test.ts (89 tests | 88 skipped) 22ms
 Test Files  1 passed (1)
      Tests  1 passed | 88 skipped (89)

=== Run 5 ===
 ✓ src/services/cronScheduler.test.ts (89 tests | 88 skipped) 22ms
 Test Files  1 passed (1)
      Tests  1 passed | 88 skipped (89)

All 5 runs pass consistently. The flake is gone.

Note: this is a test-only fix with no user-visible behavior change, so before/after tmux comparison is N/A — the verification is the test itself.

中文说明

代码审查

竞态条件是真实存在的，修复是正确的。问题如下：

在 enableDurable() 中，锁探针的 tryAcquireLock() 先写入文件（获取锁），然后在 .then() 回调中设置 this.isOwner = true。文件写入可能在 .then() 微任务执行之前就可见了。旧测试等到锁文件显示 session-1 后立即调用 scheduler.tick()——但此时 isOwner 仍为 false，所以 tick() 跳过了持久任务（第 1007 行：if (job.durable && !this.isOwner) continue）。

修复方案将 tick() + 断言包装在 vi.waitFor() 中，重试直到 .then() 回调翻转 isOwner 并且任务真正触发。这是安全的，因为对非所有者调度器调用 tick() 对持久任务是空操作——重复调用没有副作用。

断言从 toContain('probe-job') 改为 toEqual(['probe-job']) 更严格——能捕获任务多次触发的情况，那会是一个 bug。

无正确性问题，无安全隐患，无 AGENTS.md 违规。干净、最小化的修复。

测试结果

在 Linux 上用 PR 代码跑了 5 次特定测试，全部通过，不稳定问题已消除。

注：这是纯测试修复，无用户可见行为变化，因此前后对比 N/A——验证就是测试本身。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Straightforward call. The fix targets a well-understood race condition (lock file visible before isOwner microtask settles), the solution is the minimal correct approach (wrap in waitFor), and the test passes 5/5 runs. The assertion upgrade from toContain to toEqual is a nice bonus that catches double-fire bugs.

No reservations. Approving.

中文说明

简单的判断。修复针对一个理解清楚的竞态条件（锁文件在 isOwner 微任务完成前可见），方案是最小且正确的（用 waitFor 包装），测试 5/5 通过。断言从 toContain 升级到 toEqual 是个不错的附加改进，能捕获重复触发的问题。

没有顾虑，批准。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[wenshao]

✅ Local verification — real test runs under tmux (merge reference)

Verdict: verified. The change correctly removes the documented race, is robust under a deterministic reproduction of it, and introduces no collateral. Safe to merge.

I verified this in a clean worktree checked out at the PR head (ad3d483d3) with a real npm ci (no symlinked node_modules), driving vitest in a persistent tmux session.

Root cause recap (issue #5534)

In the lock-probe takeover path (cronScheduler.ts), the probe does:

void tryAcquireLock(...)          // writes the lock file → immediately visible to a reader
  .then((acquired) => {
    if (acquired && !this.isOwner) {
      this.isOwner = true;        // ownership flips only in this .then continuation
      ...
    }
  });

tick() only fires the durable job when isOwner === true. The old test waited for the lock file to show sessionId: 'session-1', then ticked synchronously — so it could tick before the .then continuation flipped isOwner, leaving fired empty. The fix wraps the tick() + assertion in vi.waitFor() so it re-ticks until ownership settles, and tightens toContain('probe-job') → toEqual(['probe-job']) (also pinning exactly one fire).

Results

#	Test variant	Scheduler	Runs	Result
A	NEW (this PR)	pristine	30×	30 / 30 PASS — stable
B	OLD (base)	pristine	100×	100 / 100 PASS — race does not reproduce naturally on Linux
C1	OLD (base)	race widened	5×	0 / 5 PASS (5 FAIL) — expected [] to include 'probe-job'
C2	NEW (this PR)	race widened	5×	5 / 5 PASS — fix is robust
D	full cronScheduler.test.ts (PR head)	pristine	89 tests	89 / 89 PASS — no collateral

Run B confirms the flake is timing/platform-specific (it was reported on macOS CI) and does not surface on this Linux box — which is exactly why a deterministic reproduction was needed for a conclusive A/B.

Decisive A/B — deterministic reproduction of the race

Because the race is too narrow to hit naturally on Linux, I widened only the documented window in the scheduler (the gap between "lock file written" and "isOwner flipped") and ran the same injected scheduler against both test variants. No test assertions were modified.

// packages/core/src/services/cronScheduler.ts — TEMPORARY widening, reverted before merge
// realSetTimeout is captured at module-eval, before any vi.useFakeTimers(), so it is
// immune to the fake clock and the await lands in real-timer territory.
- .then((acquired) => {
+ .then(async (acquired) => {
     if (generation !== this.durableGeneration) { ...; return; }
+    // lock file already written above (visible to the test's reader); defer the
+    // owner flip past the test's immediate tick using a REAL macrotask.
+    await new Promise((r) => realSetTimeout(r, 60));
     if (acquired && !this.isOwner) {
       this.isOwner = true;

• C1 (OLD + widening): fails every time with AssertionError: expected [] to include 'probe-job' at the old expect(...).toContain('probe-job') line — i.e. the exact symptom in issue cronScheduler lock probe test can tick before takeover settles #5534 (lock file visible, owner not yet flipped, empty fired).
• C2 (NEW + same widening): passes every time — vi.waitFor re-ticks until isOwner settles.

This isolates the change: under an identical, deliberately-widened race, the old assertion pattern breaks and the new one holds.

Other checks (PR head, pristine)

• npx prettier --check packages/core/src/services/cronScheduler.test.ts → All matched files use Prettier code style!
• git diff --check (PR commit vs parent) → clean, exit 0
• True PR scope → 1 file, +6 / −4 (test-only; zero production-code impact)

Notes

• The change is confined to the test file — no runtime behavior changes.
• toEqual(['probe-job']) is a nice strengthening over toContain: it also asserts the job fires exactly once across the retried ticks, which holds because tick()'s lastFiredAt double-fire guard makes repeated ticks for the same cron minute idempotent.

Environment

Node v22.22.2 · vitest 3.2.4 · Linux 6.12.63 (Debian 13) x86_64 · worktree @ ad3d483d3

🇨🇳 中文版（点击展开）

✅ 本地验证 —— tmux 下真实测试运行（合并参考）

结论：已验证。 此改动正确消除了所述竞态，在对该竞态的确定性复现下表现稳健，且无任何副作用。可以安全合并。

我在 PR HEAD（ad3d483d3）检出的独立 worktree 中验证，使用真实的 npm ci（非软链 node_modules），并在持久化的 tmux 会话中运行 vitest。

根因回顾（issue #5534）

在锁探测接管路径（cronScheduler.ts）中，探测逻辑为：tryAcquireLock(...) 写入锁文件后立即对读取方可见，但 this.isOwner = true 只在其 .then() 续延回调里翻转；而 tick() 只有在 isOwner === true 时才会触发持久任务。

旧测试先等待锁文件显示 sessionId: 'session-1'，随后同步调用 tick() —— 因此可能在 .then 续延翻转 isOwner 之前就 tick，导致 fired 为空。修复将 tick() 与断言一起包进 vi.waitFor()，使其在所有权落定前反复重试；并把 toContain('probe-job') 收紧为 toEqual(['probe-job'])（同时校验恰好触发一次）。

结果

#	测试版本	调度器	次数	结果
A	新版（本 PR）	原始	30×	30 / 30 通过 —— 稳定
B	旧版（基线）	原始	100×	100 / 100 通过 —— 该竞态在 Linux 上不会自然复现
C1	旧版（基线）	放大竞态	5×	0 / 5 通过（5 失败） —— expected [] to include 'probe-job'
C2	新版（本 PR）	同样放大竞态	5×	5 / 5 通过 —— 修复稳健
D	完整 cronScheduler.test.ts（PR HEAD）	原始	89 个用例	89 / 89 通过 —— 无副作用

Run B 表明该 flaky 是与时序/平台相关的（原报告出现在 macOS CI 上），在本 Linux 机器上不会出现 —— 这正是需要做确定性复现来给出决定性 A/B 对比的原因。

决定性 A/B —— 对竞态的确定性复现

由于该竞态在 Linux 上窗口太窄、无法自然命中，我只放大了所述的那个窗口（“锁文件已写入” 与 “isOwner 翻转” 之间的间隙），并用同一个被注入的调度器分别运行两个测试版本。未修改任何测试断言。

// packages/core/src/services/cronScheduler.ts —— 临时放大，合并前已还原
// realSetTimeout 在模块求值期（任何 vi.useFakeTimers() 之前）捕获，因此不受 fake clock 影响，
// 该 await 落在真实定时器区间。
- .then((acquired) => {
+ .then(async (acquired) => {
     if (generation !== this.durableGeneration) { ...; return; }
+    // 上面 tryAcquireLock 已写入锁文件（对测试的读取方可见）；用一个真实宏任务把
+    // 所有权翻转推迟到测试的同步 tick 之后。
+    await new Promise((r) => realSetTimeout(r, 60));
     if (acquired && !this.isOwner) {
       this.isOwner = true;

• C1（旧版 + 放大）每次都失败：在旧的 expect(...).toContain('probe-job') 处报 AssertionError: expected [] to include 'probe-job' —— 即 issue cronScheduler lock probe test can tick before takeover settles #5534 描述的现象（锁文件可见、所有权尚未翻转、fired 为空）。
• C2（新版 + 同样放大）每次都通过：vi.waitFor 会反复重试 tick，直到 isOwner 落定。

这就把改动的效果隔离出来了：在同一个被刻意放大的竞态下，旧断言写法必然失败，新写法依然成立。

其他检查（PR HEAD，原始状态）

• npx prettier --check packages/core/src/services/cronScheduler.test.ts → All matched files use Prettier code style!
• git diff --check（PR 提交 vs 父提交）→ 干净，退出码 0
• PR 真实改动范围 → 1 个文件，+6 / −4（仅测试；对生产代码零影响）

备注

• 改动仅限测试文件 —— 无任何运行时行为变化。
• toEqual(['probe-job']) 相比 toContain 是不错的加强：它额外断言了在重试 tick 期间任务恰好触发一次；这一点成立是因为 tick() 的 lastFiredAt 防重复触发保护，使同一 cron 分钟内的重复 tick 具有幂等性。

环境

Node v22.22.2 · vitest 3.2.4 · Linux 6.12.63 (Debian 13) x86_64 · worktree @ ad3d483d3

[wenshao]

✅ Local verification report — cron lock-probe takeover flaky-test fix (#5535)

Note: this PR is already merged (8553013ee on main). This is a post-merge confirmation — verified locally with real tooling in tmux. Conclusion: the fix is correct and the merged test is stable. It properly waits for the real ownership takeover (not just the visible lock write) and asserts the durable job fires exactly once.

Environment

Tested	origin/main (106074188, contains the merge) vs pre-fix parent 8be8ef3e2
Toolchain	Linux x86_64, 8 cores · Node v22.22.2 · Vitest 3.2.4

---

1. The PR's stated test plan — reproduced, green

Step	Result
vitest run cronScheduler.test.ts (full file)	✅ 89 / 89 pass
target test repeated (PR did 5×)	✅ ran 300× + 2400× under load — see §2/§3
prettier --check cronScheduler.test.ts	✅ "All matched files use Prettier code style!"
git diff --check	✅ clean

2. The merged (fixed) test is stable

Run	Result
takeover test, 300× in-process	✅ 300 / 300 pass
full cronScheduler.test.ts suite	✅ 89 / 89 pass

---

3. The old race is real but extremely rare (couldn't reproduce naturally)

The flake (#5534, seen on macOS CI) is: the probe writes the lock file (visible on disk) and only afterwards flips isOwner in its .then continuation; the old test ticked once immediately after seeing the lock file, so when isOwner hadn't flipped yet the durable job didn't fire and toContain('probe-job') saw an empty list.

In the takeover path, tryAcquireLock is just writeFile(…, {flag:'wx'}) → return true, so the window between "lock visible" and "isOwner flips" is a single microtask gap. I ran the pre-fix test:

• 150× in-process (idle): all pass
• 2400× (16 concurrent processes × 150) under 8× CPU saturation: all pass

So on Linux the .then microtask essentially always wins — the natural flake is too rare to be a reliable signal here. That's consistent with it being an occasional CI-only flake under heavy load.

4. Deterministic reproduction — the conclusive proof

Since the natural race won't surface on demand, I reproduced the exact race deterministically by wrapping the real tryAcquireLock so its resolve (which is what defers the scheduler's isOwner = true) is delayed by 150 ms — i.e. the lock file is visible immediately but isOwner flips 150 ms later. Faithful: the real acquire still does the real writeFile; only the promise resolution is delayed (timer captured before vi.useFakeTimers()).

injected-race test	result	what it proves
PRE-#5535 — single immediate tick	✅ job does not fire (fired === []), and the exact toContain('probe-job') assertion throws	the flake, reproduced deterministically
POST-#5535 — waitFor-wrapped tick	✅ retries until takeover settles → fires exactly once (['probe-job'])	the fix is robust to that race

This shows the fix is not a no-op: under the race it changes a hard failure into a correct pass.

Scope assessment (why test-only is the right fix)

The change is test-only and does not alter the production race — correctly. In production the brief isOwner=false window is harmless: the scheduler's tick loop / 5 s probe simply fire the durable job on the next tick. The old test was asserting an over-strict timing guarantee that production neither provides nor needs. Tightening toContain → toEqual(['probe-job']) is also a strengthening: it now additionally guards against a duplicate fire (confirmed exactly-once across 300×/2400× and the injected-race run).

Verdict

Confirmed good. The merged test waits for the real takeover instead of treating the visible lock write as proof of settled ownership, asserts a single fire, and is stable across 2700+ local iterations; a deterministic injection of the exact race proves the old assertion would fail and the new one holds. The merge was sound.

🇨🇳 中文版验证报告（点击展开）

✅ 本地验证报告 —— cron lock-probe takeover 不稳定测试修复（#5535）

说明：本 PR 已经合并（main 上的 8553013ee）。这是一份合并后确认报告 —— 在 tmux 中用真实工具链本地验证。结论：修复正确，合并后的测试稳定。 它会等待真正的所有权 takeover 完成（而不是仅凭可见的 lock 写入），并断言 durable job 恰好触发一次。

环境

被测	origin/main（106074188，含此合并） vs 修复前父提交 8be8ef3e2
工具链	Linux x86_64，8 核 · Node v22.22.2 · Vitest 3.2.4

1. PR 自述测试计划 —— 复现，全绿

步骤	结果
vitest run cronScheduler.test.ts（整个文件）	✅ 89 / 89 通过
目标测试重复（PR 做了 5×）	✅ 跑了 300× + 负载下 2400× —— 见 §2/§3
prettier --check cronScheduler.test.ts	✅ 全部符合 Prettier 风格
git diff --check	✅ 干净

2. 合并后（修复后）的测试稳定

运行	结果
takeover 测试，进程内 300×	✅ 300 / 300 通过
完整 cronScheduler.test.ts 套件	✅ 89 / 89 通过

3. 旧的竞态真实存在但极其罕见（无法自然复现）

该不稳定（#5534，macOS CI 上出现）的本质是：probe 先写入 lock 文件（磁盘上可见），之后才在它的 .then 续延里翻转 isOwner；旧测试在看到 lock 文件后立刻 tick 一次，于是当 isOwner 尚未翻转时 durable job 不会触发，toContain('probe-job') 看到空数组。

在 takeover 路径中，tryAcquireLock 只是 writeFile(…, {flag:'wx'}) → return true，因此"lock 可见"到"isOwner 翻转"之间只有一个 microtask 的间隙。我跑了修复前的测试：

• 进程内 150×（空闲）：全部通过
• 2400×（16 个并发进程 × 150）在 8× CPU 饱和下：全部通过

所以在 Linux 上 .then microtask 几乎总是胜出 —— 自然不稳定太罕见，不足以作为可靠信号。这与它仅在重负载下偶发于 CI 的特征一致。

4. 确定性复现 —— 决定性证据

既然自然竞态无法按需出现，我通过包装真实的 tryAcquireLock 来确定性地复现这个竞态：把它的 resolve（也就是推迟 scheduler 的 isOwner = true 的那一步）延迟 150 ms —— 即 lock 文件立即可见，但 isOwner 在 150 ms 后才翻转。忠实于真实路径：真实的 acquire 仍然执行真实的 writeFile，仅延迟 promise 的 resolve（定时器在 vi.useFakeTimers() 之前捕获）。

注入竞态的测试	结果	证明了什么
PRE-#5535 —— 立刻单次 tick	✅ job 不触发（fired === []），且原本的 toContain('probe-job') 断言抛错	确定性地复现了该不稳定
POST-#5535 —— waitFor 包裹的 tick	✅ 重试直到 takeover 完成 → 恰好触发一次（['probe-job']）	修复对该竞态是健壮的

这表明修复并非空操作：在竞态下它把一次硬失败变成了正确的通过。

范围评估（为何"仅测试"是正确的修复）

此改动仅涉及测试，且不改动生产竞态 —— 这是对的。生产中那短暂的 isOwner=false 窗口无害：scheduler 的 tick 循环 / 5 秒 probe 会在下一次 tick 触发该 durable job。旧测试断言了一个生产既不提供也不需要的过严时序保证。把 toContain 收紧为 toEqual(['probe-job']) 也是一种增强：它现在还能防止重复触发（在 300×/2400× 及注入竞态运行中均确认恰好一次）。

结论

确认无误。 合并后的测试等待真正的 takeover，而不是把可见的 lock 写入当作所有权已稳定的证据；断言单次触发；在 2700+ 次本地迭代中稳定；对该竞态的确定性注入证明了旧断言会失败、新断言成立。此次合并是稳妥的。

---

_{Verification method: merged-vs-base worktree; PR test plan reproduced (89/89 + prettier + git diff); merged test 300× in-process; pre-fix test 2400× under 8× CPU + 16× concurrent load (race too rare to surface); deterministic injection of the exact "lock visible before isOwner flips" race showing the old single-tick assertion fails and the new waitFor-tick fires exactly once.}