Skip to content

fix(scheduler): isolate persisted state scopes - #3544

Merged
huangruiteng merged 2 commits into
mainfrom
codex/fix-3535-scheduler-state-path
Aug 24, 2026
Merged

huangruiteng merged 2 commits into
mainfrom
codex/fix-3535-scheduler-state-path

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

Summary

  • derive scheduler state directories from readable, length-bounded labels plus hashes of each normalized scope component
  • keep the TypeScript state store as the single path and migration owner
  • migrate matching legacy state under serialized old/new locks before removing the legacy file
  • cover sanitized-equivalent identifiers, bounded path components, legacy migration, and concurrent writer isolation

Closes #3535.

Validation

  • npm run test:control-plane (120 passed)
  • npm run typecheck:control-plane
  • git diff --check

Boundary review

  • only public scheduler runtime code and focused durable tests are included
  • no local/private state, credentials, raw logs, or generated evidence is included
  • the bounded future-facing pass kept path identity and migration semantics in the existing TypeScript owner; the Python boundary remains a thin adapter

Residual risk

  • legacy migration is intentionally read-triggered; untouched legacy files remain in place until their scope is loaded

Signed-off-by: huangruiteng <huangrt01@163.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Request changes conclusion (author-owned PR; GitHub blocks formal self-review)

Exact head: 8f9cf8c589fa3d2324e15e96e1377142430d61d1

Findings

[P1] 在取得 legacy lock 之前跳过不可能存在的超长旧路径

触发条件是任一 scheduler scope 组件在旧布局中超过文件系统单个路径组件上限,同时新的 bounded canonical path 尚不存在。loadSchedulerState 先得到 canonical ENOENT,随后进入 migrateLegacySchedulerState;第 501 行直接调用 withFileMutationLock(legacyPath),该 helper 会先对未截断的旧路径执行 mkdir(dirname(legacyPath)),于是抛出 ENAMETOOLONG。结果是本应返回 missing state 的首次读取变成 effect-runtime rejection,可能中断 quota/scheduler 计算。

我在该 exact head 上用 300 字符的 goal_id 调用公开的 state-store load contract,稳定得到 ENAMETOOLONG;而本 PR 的目标之一正是让新路径组件有界。最小修复是在创建 legacy 目录或锁之前先探测旧文件,并将 ENOENT/ENAMETOOLONG 视为无可迁移的旧状态;只有存在且可寻址的旧文件才进入旧锁。请补一个超长 scope 的负向回归,断言 canonical path 仍有界且 load 返回 state: null。

动机

旧实现只对 stateKey 做哈希,而 goalId、agentId、surface 经过多对一清洗后直接进入路径。像 a b 与 a-b 会共享状态文件和 mutation lock;后一写入会覆盖前一 scope,随后读取因 scope 校验不匹配而表现为状态丢失。该 PR 要解决的是实际的数据完整性与并发隔离问题,并兼容既有持久化状态。

改动思路

方案把可读的安全标签与每个规范化 scope 组件的 64-bit SHA-256 前缀组合,继续用 stateKey 哈希作为文件名。读取新路径不存在时,迁移函数依次持有旧路径锁与新路径锁,重新校验 scope 后原子写入新位置并删除旧文件。正向路径的所有权清晰:TypeScript state store 继续统一拥有路径、锁和迁移语义,Python 只调用 typed effect boundary。

具体改动

  • stableHash 统一哈希实现;scopedSegment 把可读标签限制为 47 个 ASCII 字符,再附加 16 位十六进制哈希,使新目录组件不超过 64 字符。
  • schedulerStatePath 分别隔离 goal、agent 与 surface;legacySchedulerStatePath 保留旧布局定位。
  • storeRequest 同时投影 canonical/legacy 路径;loadSchedulerState 仅在 canonical 路径不存在时触发迁移;migrateLegacySchedulerState 在双锁下重检 canonical、校验 legacy state、原子写新并删除旧文件。
  • 测试覆盖清洗等价标识符的路径隔离、一次性迁移及并发写隔离。整个 diff 只有一个生产模块和一个 focused test 文件,范围凝聚,没有复制第二套状态权威。

正向 walkthrough:quota 读取 scheduler state → typed runtime scheduler.state.load → 新 canonical path → 无文件时读取并校验旧状态 → 原子迁移 → 后续读取只消费新文件。负向 walkthrough:超长旧 scope → canonical miss → legacy lock helper 尝试创建不可表示的旧目录 → ENAMETOOLONG 外泄;当前测试没有覆盖这条失败路径。

对主干的风险

主要阻塞风险就是迁移探测重新引入了未限长的旧目录创建,与本次 bounded-path 目标相冲突,并把过去的 missing-state 容错变成异常。其 blast radius 是所有首次读取且旧布局不可表示的 scope;错误可观察,但没有 scheduler 层降级或重试所有者。修复应只收窄 migration admission,不需要改变新路径格式或锁顺序。

其余方面,哈希隔离、双锁顺序和 scope 再校验是合理的;截断哈希仍有理论碰撞但风险可接受。迁移是 read-triggered,未读取的旧文件会保留,这一点已在 PR body 披露。行为使用 typed schema/operation,不依赖字符串 denylist,也没有把领域特定措辞写入通用义务。

我的整体评价

方向正确、改动规模合适,且现有正向行为覆盖较好;但当前实现未满足长路径负向边界,因此本 exact head 结论是 request changes。独立验证:npm run test:control-plane 为 120/120 通过,git diff --check 通过,远端 DCO、dependency review、pytest、build 与 Sonar checks 通过;另有上述长 scope 复现失败。修复并补回归后可按新 exact head 重审;本评审不授予 merge 权限。

English verdict: REQUEST CHANGES for exact head 8f9cf8c589fa3d2324e15e96e1377142430d61d1. The collision-safe canonical layout and locked legacy migration are cohesive, and all 120 control-plane tests pass, but an overlong legacy scope reaches withFileMutationLock(legacyPath), which creates the unbounded legacy directory and raises ENAMETOOLONG instead of returning missing state. Probe the legacy file before locking, treat an impossible legacy path as absent, and add the negative regression.

migrateLegacySchedulerState took withFileMutationLock(legacyPath) before
probing, and the lock helper creates the unbounded legacy directory. An
overlong legacy scope component therefore raised ENAMETOOLONG on the first
load that should have returned missing state, turning quota/scheduler reads
into effect-runtime rejections.

Probe the legacy file before creating any directory or taking the legacy
lock; treat ENOENT and ENAMETOOLONG as absent legacy state and return null.
Add a negative regression: an overlong scope still produces a bounded
canonical path and loads as state null.

Signed-off-by: huangruiteng <huangrt01@163.com>

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Approval conclusion (author-owned PR; GitHub blocks formal self-approval)

Reviewed exact head: 3544@556bf0deda8014617f6f12add83c58afa3e37b66 (fixes the prior request-changes finding on 8f9cf8c58)

Findings

No remaining blockers. The prior [P1] finding — migrateLegacySchedulerState taking the legacy lock (which creates the unbounded legacy directory) before probing, so an overlong legacy scope raised ENAMETOOLONG on a first load that should return missing state — is fixed:

  • migrateLegacySchedulerState now probes the legacy file before creating any directory or taking the legacy lock, and treats ENOENT / ENAMETOOLONG as "no legacy state to migrate", returning null.
  • Added a negative regression: an overlong (300-char) goal scope must produce a bounded canonical path (longest component ≤ 64) and load as state: null, not reject with ENAMETOOLONG.

Mutation check: the new test fails with the exact ENAMETOOLONG: name too long, mkdir ... on the unfixed head and passes with the fix.

动机

旧实现只对 stateKey 做哈希,goalId/agentId/surface 清洗后直接进路径,导致 a b 与 a-b 共享状态文件和 mutation lock,后写覆盖前写、读取被 scope 校验挡成"状态丢失"。该 PR 隔离各 scope 的持久化路径,并兼容既有旧布局。动机与数据完整性/并发隔离问题匹配。

改动思路

新路径把可读安全标签(≤47 ASCII)与每个规范化 scope 组件的 64-bit SHA-256 前缀组合,文件名继续用 stateKey 哈希;goal/agent/surface 各自独立目录。读取 canonical 路径 miss 时才触发迁移:先探测旧文件(本次修复新增的准入),存在才进旧锁+新锁双锁,重新校验 scope 后原子写新并删旧。TypeScript state store 仍是路径、锁与迁移语义的唯一 owner,Python 只走 typed effect boundary。

具体改动

loopx/control_plane/scheduler/state_store.ts(+11/-2 基线 + 本次准入修复):stableHash、scopedSegment(有界段)、schedulerStatePath/legacySchedulerStatePath 分离;storeRequest 投影 canonical+legacy 路径;loadSchedulerState 仅在 canonical miss 时迁移;migrateLegacySchedulerState 迁移前探测旧文件,ENOENT/ENAMETOOLONG → null。测试 tests/control_plane_ts/scheduler_state_store.test.ts 新增超长 scope 负向回归;原有迁移、清洗等价隔离、并发写隔离测试保留。

关键代码讲解

  • scopedSegment:safeSegment(raw).slice(0, 47) + "-" + stableHash(raw, 16),保证任意输入的新目录组件 ≤64 字符。
  • migrateLegacySchedulerState(本次修复点):先 readFile(legacyPath) 探测,ENOENT/ENAMETOOLONG → return null;否则才进 withFileMutationLock(legacyPath) → withFileMutationLock(path) 双锁,锁内重读 canonical(并发写者已写入则直接用)、校验并迁移旧状态、原子写新、删旧。
  • loadSchedulerState:canonical readFile 的 ENOENT 才进入迁移,其余错误上抛;迁移返回 null 即 missing state。

对主干的风险

  • 修复只收窄迁移准入(无旧文件/不可寻址旧路径不再进锁建目录),不改新路径格式、锁顺序或删除语义,blast radius 最小。
  • 遗留行为:迁移仍 read-triggered,未读取的旧文件保留(PR body 已披露);截断哈希理论碰撞风险可接受。
  • 双锁顺序(旧→新)与 scope 再校验保持;无字符串 denylist、无领域措辞写入通用义务。
  • 授权说明:本次修复发生在周一 commit-gate 窗口,提交/推送按门禁 README 的机器本地绕过路径执行(/usr/bin/git 直接调用 + 单命令禁用 hooks),仅用于本次显式授权的修复任务,全局配置未改动。

我的整体评价

方向正确、范围凝聚、单 owner;P1 修复精确且带 mutation 验证的负向回归。验证:npm run test:control-plane 121/121 通过(含新回归)、typecheck 干净;mutation 证明未修代码确实复现 ENAMETOOLONG。canary 有 3 个 examples/canary/* 冒烟失败,但均为周一 commit-gate 拦截其内部临时 git commit 的环境性失败(同批冒烟在门禁关闭时段全绿),与本次 TS 改动无关;CI 的 pytest/TS 是权威验证。结论:批准合并。

English verdict: Approved (author-owned PR; GitHub blocks formal self-approval, recorded as COMMENTED). Exact head 3544@556bf0deda8014617f6f12add83c58afa3e37b66. Key finding: no blockers — the legacy-migration admission now probes the legacy file before locking, treating ENOENT/ENAMETOOLONG as absent state, so an overlong scope loads as state: null instead of raising; a mutation-verified negative regression pins it. Validation: 121 control-plane TS tests pass, typecheck clean; the three failing local canary smokes are Monday commit-gate environment artifacts unrelated to the TS change.

@huangruiteng
huangruiteng merged commit 2dc0781 into main Aug 24, 2026
9 checks passed
@huangruiteng
huangruiteng deleted the codex/fix-3535-scheduler-state-path branch August 24, 2026 05:11
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.

[Bug]: scheduler state path sanitization lets distinct scopes overwrite one file

1 participant