refactor(scheduler): attempt 阶段机显式化——统一转移入口 + 单一出口清单 - #1060
Conversation
做 makecindy#1016:PR makecindy#944 review 里「某条出口分支漏做收口动作」同型缺陷出现四次, 根因是 attempt 生命周期靠多个隐式出口各自手工记得收口。三件事: - 转移表显式化(attemptLifecycle.ts,纯逻辑):由现网全部 7 处 phase 写点 穷举推导;所有写入统一走 transitionAttempt,非法转移抛错(「静默少做一 件事」不再可能静默),幂等重入(强制收口与迟到 settle 各置一次 finalizing)按 no-op 放行。 - 单一出口清单:finishInflightAttempt 删除 attempt 时矫正性清扫所有仍指向 它的登记(controller / per-schedule 索引 / session 双向映射 / 静默标记) 并响亮告警——残留即某条出口漏了收口,在日志与测试里直接可见。 abandonedRuns 刻意不碰(设计为跨生命周期由迟到 settle 消费)。 - 登记一致性不变量:begin(注册面唯一扩张点)断言全部按 runId 键控的登记 必须指向在账 attempt,违反抛错。 刻意不做的两件事(与 issue 建议的偏差,均有依据):终态落库/重排/补通知 仍留在各自路径——fireOne 重排而 runNow 不重排、defer 撤销语义等是逐条 review 钉下的有意差异,收进单函数会改变已评审语义;「slotsInUse 永不超过 maxConcurrentRuns」不作断言——runNow 有意绕过闸门挤压槽位(代码注释明示), 该不变量对手动触发不成立,改为断言登记一致性。 行为等价验证:既有 186 测全过未改一行;新增穷举矩阵 + 排队往返/排队中 interrupt 两条端到端链路(出口零残留告警)。 Signed-off-by: ficowang <fico@xd.com>
|
| Filename | Overview |
|---|---|
| packages/maker-scheduler/src/engine/attemptLifecycle.ts | 新增 attempt 阶段合法转移表及统一判定函数,转移关系与说明一致。 |
| packages/maker-scheduler/src/engine/scheduler.ts | 接入统一阶段转移、集中登记清扫和一致性断言,并完整修复迟到排队回调产生伪错误的问题。 |
| packages/maker-scheduler/src/tests/attemptLifecycle.test.ts | 穷举验证全部阶段组合,并单独锁定关键生命周期边。 |
| packages/maker-scheduler/src/tests/scheduler.test.ts | 新增正常排队、强制收口、停机竞态及迟到回调场景的回归覆盖。 |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
C[claiming] --> P[persisting]
L[loading] --> P
P --> R[running]
R --> Q[queued]
Q --> R
Q --> X[cancelling]
P --> F[finalizing]
R --> F
Q --> F
X --> F
F --> F
Reviews (7): Last reviewed commit: "test(maker-scheduler): 修 stop 竞态用例的 mock..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fb7ad8e3c
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR refactors the maker-scheduler in-flight attempt lifecycle to be an explicit phase state machine with a single transition write-entry and a single cleanup exit, aiming to prevent “silent missing cleanup” bugs that previously appeared across multiple exit paths.
Changes:
- Introduces an explicit legal phase-transition table (
attemptLifecycle) and routes all phase writes throughScheduler.transitionAttempt(illegal transitions throw). - Adds a single-exit residual cleanup list (
reapAttemptResiduals) and a begin-time registry invariant check (assertAttemptRegistryInvariants). - Adds tests for the transition matrix and key end-to-end lifecycle paths (including queued ↔ running and queued → finalizing).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/maker-scheduler/src/engine/scheduler.ts | Centralizes attempt phase transitions, adds residual cleanup on exit, and asserts registry invariants. |
| packages/maker-scheduler/src/engine/attemptLifecycle.ts | Defines the explicit legal transition table and helper for validating transitions. |
| packages/maker-scheduler/src/tests/scheduler.test.ts | Adds lifecycle integration tests ensuring no illegal transitions and no residual-cleanup warnings. |
| packages/maker-scheduler/src/tests/attemptLifecycle.test.ts | Adds exhaustive matrix tests to lock down the transition table semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
review 反馈:强制收口把 attempt 置 finalizing 后,runner 的异步 continuation 仍可能调 onQueueWaitStart,原实现会抛非法转移错误——竞态 是预期而非状态机缺陷。与 endQueueWait 同款:attempt 已 finalizing 时 安静返回。新增用例:强制收口完成后迟到调用两个排队回调均不抛。 Signed-off-by: ficowang <fico@xd.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/maker-scheduler/src/engine/scheduler.ts:1708
assertAttemptRegistryInvariants()的注释写的是“所有按 runId 键控的登记必须指向仍在账的 attempt”,但当前实现只校验了inflightControllers/inflightByschedule/runIdToSessionId,未覆盖同样以 runId 为 key 的runIdToBoundSessionId与silencedRuns(两者也在reapAttemptResiduals()里被清理)。这会让一部分“悬挂登记”无法在 beginInflightAttempt 的断言点被响亮暴露,且与注释语义不一致。
private assertAttemptRegistryInvariants(): void {
for (const runId of this.inflightControllers.keys()) {
if (!this.inflightAttempts.has(runId)) {
throw new Error(`scheduler invariant violated: controller without attempt (runId=${runId})`);
}
|
@fico-hub 👋 这个 PR 还有 3 条 review conversation 没 resolve(packages/maker-scheduler/src/engine/scheduler.ts),auto-review 因此暂时跳过、没法继续审查 / 合并。 如果你已经按评论改完或回应了,请到对应 thread 上点 Resolve conversation;全部 resolve 后,下一轮 auto-review 会自动重新审查这个 PR。 |
review 反馈第二轮: - codex P1: 强制收口删除 attempt 后, runner continuation 迟到上报 onTurnActive 会往 session 映射写悬挂登记, 下一次 beginInflightAttempt 的不变量断言将响亮抛错。补与 onQueueWaitStart 同款的迟到守卫 (attempt 不在账或已 finalizing → 安静 no-op)。 - onSessionBound 同类迟到写入 runIdToBoundSessionId 一并守卫, 否则 下述不变量扩展会被合法竞态触发。 - copilot: assertAttemptRegistryInvariants 补齐 runIdToBoundSessionId 与 silencedRuns 两类 runId 键控登记的覆盖。 - copilot: 孤立 JSDoc 挪回 beginInflightAttempt 头上。 新增迟到 onTurnActive/onSessionBound 用例: 强制收口后调用不留悬挂 登记, 后续 fire 的 begin 断言不抛。 Signed-off-by: ficowang <fico@xd.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96c947252e
ℹ️ 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".
|
@fico-hub 👋 这个 PR 还有 1 条 review conversation 没 resolve(packages/maker-scheduler/src/engine/scheduler.ts),auto-review 因此暂时跳过、没法继续审查 / 合并。 如果你已经按评论改完或回应了,请到对应 thread 上点 Resolve conversation;全部 resolve 后,下一轮 auto-review 会自动重新审查这个 PR。 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
packages/maker-scheduler/src/tests/scheduler.test.ts:3996
- 为配合上面去掉提前结束的
describe,文件末尾需要补上对应的});来关闭 describe,否则当前结构会变得不对称/难读。
});
packages/maker-scheduler/src/tests/scheduler.test.ts:3928
describe('Scheduler: attempt 生命周期状态机(#1016)'...)在这里提前结束,导致后面的两条用例(强制收口后的迟到回调)变成 describe 之外的顶层it,分组/生命周期钩子会不一致,也容易让后续维护误判测试覆盖范围。
This issue also appears on line 3996 of the same file.
});
packages/maker-scheduler/src/engine/scheduler.ts:1685
- 这里的注释说明“登记数上限受并发闸门约束”与当前实现不一致:
queuedattempt 不计入slotsInUse,因此 in-flight 登记数量在设计上可以显著大于maxConcurrentRuns。建议更新注释,避免对复杂度/运行期开销产生误导。
* 单测与运行期都响亮失败。只在 begin(注册面唯一的扩张点)校验,O(登记数),
* 上限受并发闸门约束,代价可忽略。
CI 的 scheduler 架构守卫(lint)拦下 no-unused-vars:上一条迟到回调用例 只需要 create 的副作用,不需要返回值。 Signed-off-by: ficowang <fico@xd.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/maker-scheduler/src/tests/scheduler.test.ts:3932
- The
describe('Scheduler: attempt 生命周期状态机(#1016)'...)block is closed at line 3928, but the next twoit(...)cases (starting at 3930) are still indented as if they belong to that suite. This makes the test grouping confusing (and would bypass any suite-level hooks/helpers if added later). Consider either moving those twoitcases inside thedescribeby relocating the closing});, or unindenting them to make them clearly top-level.
});
it('强制收口后 runner 迟到调用 onQueueWaitStart → no-op,不抛非法转移(#1016 review)', async () => {
vi.useFakeTimers();
try {
codex review P1:stop() 清 inflightAttempts 与其余 runId 键控映射时漏了 silencedRuns。留着的标记会让同实例后续第一次 beginInflightAttempt 的 不变量断言把它当悬挂登记抛错。语义上与 silenceRun 文档一致 —— 静默 标记丢失的安全方向就是照常通知。 新增用例:静默 run 执行中 stop 后,标记被清、再 runNow 不被断言误杀。 Signed-off-by: ficowang <fico@xd.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/maker-scheduler/src/engine/attemptLifecycle.ts:39
- LEGAL_PHASE_TRANSITIONS 虽然做了 Object.freeze,但这是浅冻结:内部的数组仍可被外部(或测试)意外 mutate,从而在运行期改变 isLegalPhaseTransition 的判定结果。既然该表被导出并作为“显式转移表”基准,建议把每个数组也冻结(或避免导出可变结构),保证转移表在运行期真正不可变。
> = Object.freeze({
claiming: ['persisting'],
loading: ['persisting'],
persisting: ['running', 'finalizing'],
running: ['queued', 'finalizing'],
packages/maker-scheduler/src/tests/scheduler.test.ts:3931
- 这里在 3928 行已经结束了 describe('Scheduler: attempt 生命周期状态机(#1016)'),但后续 3930 起的 3 个 it(...) 仍保持了块内缩进,看起来是同一组 #1016 测试的一部分却意外落在顶层(且缩进不符合本文件其它顶层测试的格式,例如 3859 行)。建议把这三个用例纳入同一个 describe,避免测试分组/报告名称混乱,也避免未来自动格式化/重排时产生误解。
await h.scheduler.stop();
});
});
it('强制收口后 runner 迟到调用 onQueueWaitStart → no-op,不抛非法转移(#1016 review)', async () => {
vi.useFakeTimers();
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2982346522
ℹ️ 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".
codex review P1:stop() 打在 fireOneInner(claimDueFire/insertRun)或 runNowInner(storage.get/insertRun/update)的前置 await 期间时,attempt 被清 但 continuation 还没有 controller、无从 abort;恢复后照常 registerInflight 会留下没有 attempt 的悬挂 controller/索引,此后同实例每次 begin 都被 不变量断言拦下。 两条路径在 registerInflight 前补 attempt 在账检查:fireOne 放弃本轮 (run 行交给下次 start() 僵尸清扫收敛,认领走崩溃恢复既有归一);runNow 按调用方显式动作的契约抛错,不静默吞掉。 新增用例:insertRun 卡住期间 stop(),恢复后 runNow 拒绝并抛错、无悬挂 登记,同实例再 runNow 正常完成。 Signed-off-by: ficowang <fico@xd.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/maker-scheduler/src/tests/scheduler.test.ts:4052
- 如果按上一条建议移除中途的
});,则需要在文件末尾补回describe('…#1016')的闭合;当前文件结尾只有最后一个it的闭合,没有 describe 的闭合。否则会造成括号不匹配或测试块结构不清晰。
await h.scheduler.stop();
});
packages/maker-scheduler/src/tests/scheduler.test.ts:3931
- 后续几个 #1016 用例(从“强制收口后 runner 迟到调用…”开始)目前因为提前的
});被放到了describe('Scheduler: attempt 生命周期状态机(#1016)')之外,导致用例分组/结构不符合注释意图,也会让后续若添加 beforeEach/afterEach 时行为意外。建议移除这里多余的 describe 结束符,让后面的用例仍归属于该 describe。
This issue also appears on line 4050 of the same file.
await h.scheduler.stop();
});
});
it('强制收口后 runner 迟到调用 onQueueWaitStart → no-op,不抛非法转移(#1016 review)', async () => {
vi.useFakeTimers();
|
⏸️ 本 PR 触发 arch 签字门(大规模重构 + 核心路径),已开讨论 issue 等待 roster 确认。如需放行:在 PR 评论或讨论 issue 里回复同意,或摘掉 讨论 issue:#1089 |
MagicLizi
left a comment
There was a problem hiding this comment.
Review: 1 P1 (阻塞合并)
P1: scheduler.test.ts:4026 — 新测试代码通不过 tsc --noEmit
最后一个新测试('stop() 打在前置 await 期间...(#1016 review)', L4018–4051)将 storage.insertRun mock 为返回 Promise<void>(L4026–4030),但 insertRun 的真实签名是 Promise<ScheduleRun>(见 src/interfaces/schedule-storage.ts:22)。
跑 pnpm --filter @cindy/maker-scheduler run build(即 tsc --noEmit)产生两个级联错误(TS2322 @ 4026, TS2345 @ 4029)。vitest 仍然 194/194 全绿是因为 esbuild 跳过了类型检查。
修复:参考同文件 L3777–3780 的正确写法,把 L4028 的 Promise<void> 改成 Promise<ScheduleRun>,L4030 的 resolve() 改成 resolve(realInsertRun(run))(已有一致模式)。
P2(不阻塞,供参考)
- 测试作用域:describe('...#1016...') 在 L3928 就关了,后 4 个
it()块掉到模块顶层(无功能影响,但 PR body 的测试计数对不上)。 - 退出清单未完全统一:
forceReleaseStalledRun、unregisterInflight、stop()三处仍手写重复的注册表清理,未收进reapAttemptResiduals(当前无泄漏,纯冗余)。 assertAttemptRegistryInvariants未独立校验反向sessionIdToRunIdmap(当前由写入对称性保证安全,但未独立断言)。- 无 live-Scheduler 正向测试覆盖
transitionAttempt的 throw 路径和reapAttemptResiduals的 warn-on-residual 路径。
auto-review P1:该用例把 storage.insertRun mock 成 Promise<void>,真实 签名是 Promise<ScheduleRun>,tsc --noEmit(包内 build 脚本)报 TS2322/ TS2345——vitest 经 esbuild 跳过类型检查所以测试仍绿。改为 Promise<ScheduleRun>,resolve 直接透传 realInsertRun 结果。 顺修 P2:describe 在原有用例后提前闭合,后续 4 个追加用例掉到模块顶层 ——闭合挪到文件末尾,用例回到 describe 作用域内。 Signed-off-by: ficowang <fico@xd.com>
|
auto-review 反馈已处理(e57b26c):
验证: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/maker-scheduler/src/engine/attemptLifecycle.ts:39
LEGAL_PHASE_TRANSITIONSusesObject.freeze(...), but that freeze is shallow: the inner arrays remain mutable at runtime (e.g.LEGAL_PHASE_TRANSITIONS.running.push(...)would still work). Since this table is treated as a strict lifecycle contract, it’s safer to freeze the arrays too to prevent accidental mutation in production/tests.
export const LEGAL_PHASE_TRANSITIONS: Readonly<
Record<ScheduleRunPhase, readonly ScheduleRunPhase[]>
> = Object.freeze({
claiming: ['persisting'],
loading: ['persisting'],
persisting: ['running', 'finalizing'],
running: ['queued', 'finalizing'],
queued: ['running', 'cancelling', 'finalizing'],
cancelling: ['finalizing'],
finalizing: [],
});
MagicLizi
left a comment
There was a problem hiding this comment.
Review passed — transition table complete and correct, stop() race guards properly handle async gaps, reapAttemptResiduals safety net validated against false positives, late-callback guards comprehensive.
|
显式阶段机把 scheduler attempt 的隐式状态网格变成了清晰可查表的 transition table,stop() race 条件和残留回收一并理清了,已合入。 |
这次改了什么
摘要
做 #1016:PR #944 的 review 里「某条出口分支漏做收口动作」同型缺陷出现四次,根因是 attempt 生命周期有多个隐式出口、每个出口都靠手工记得做全套收口。本 PR 把生命周期显式化,三件事:
engine/attemptLifecycle.ts,纯逻辑):由现网全部 7 处 phase 写点穷举推导合法边(claiming/loading → persisting → running ⇄ queued → cancelling → finalizing,finalizing 为吸收相);所有 phase 写入统一走Scheduler.transitionAttempt,非法转移抛错(「静默少做一件事」不再可能静默);幂等重入(强制收口与迟到 settle 各置一次 'finalizing' 的既有竞态)按 no-op 放行。finishInflightAttempt(attempt 唯一删除点)删除时矫正性清扫所有仍指向它的登记(controller / per-schedule 索引 / session 双向映射 / 静默标记)并响亮告警——残留即某条出口漏了收口,在日志与测试里直接可见。abandonedRuns刻意不碰(设计为跨 attempt 生命周期存活、由迟到 settle 消费)。beginInflightAttempt(注册面唯一扩张点)断言全部按 runId 键控的登记必须指向在账 attempt,违反抛错。与 issue 建议的两处有据偏差
变更类型
refactor/perf重构或性能优化范围
UI 变化
不涉及。
怎么验证的
自动验证
手工验证
不涉及(调度器行为由假时钟 harness 全量锚定)。
未执行的验证
无。
风险
风险分类
影响与回滚
提交前检查
git commit -s,见 DCO)