Commit 47236648e9b179bc59f11c345806b37931fdc131
1 parent
55a9c0b5
I: fix reconciliation loop leaking into Deploy Supervisor candidates
_launch_candidate() starts backend_server against a COPY of the prod db for health-checking only; without --disable-reconciliation-loop it would find resumable tasks in that copy and dispatch real claude -p calls before Deploy Supervisor decides whether the candidate is even good. Adds the flag plus a mocked-Popen regression test (LaunchCandidateArgsTest) so a future regression here fails fast instead of surfacing as a hang or a real incident. Also folds this into docs/todo/scheduler-production-polling-loop-design.md (§6.1) as part of the FR-2 design record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Showing
4 changed files
with
56 additions
and
2 deletions
docs/todo/scheduler-production-polling-loop-design.md
| ... | ... | @@ -70,6 +70,18 @@ |
| 70 | 70 | 4. ✅ `--disable-reconciliation-loop` 提供独立开关;FR-2 与 FR-1 天然独立(FR-1 尚未实现, |
| 71 | 71 | 两者不存在耦合)。 |
| 72 | 72 | |
| 73 | +## 6.1 部署候选进程的联动修复(2026-08-06,与本设计同批) | |
| 74 | + | |
| 75 | +`supervisor/deploy_stage.py::_launch_candidate` 启动的候选 `backend_server` 进程,`db_path` | |
| 76 | +是生产库的一份拷贝(`_copy_prod_db`)——候选进程本身默认也会带上复原循环,扫到拷贝库里的 | |
| 77 | +可恢复任务后会在 Deploy Supervisor 还没做健康检查/裁定之前就真的发起 `claude -p` 调用。已给 | |
| 78 | +`_launch_candidate` 补上 `--disable-reconciliation-loop`,并在 | |
| 79 | +`supervisor/test_deploy_stage.py::LaunchCandidateArgsTest` 加了一个轻量单测直接断言 argv 里有 | |
| 80 | +这个 flag(之前只有一个真实拉起进程验证 HTTP 可用的重量级测试,不会对这类"忘加一个 flag"的 | |
| 81 | +回归报错)。`supervisor/deploy_rollback.py`/`deploy_cutover.py` 里另外两处 `_launch_candidate` | |
| 82 | +调用点(Live/回滚路径)**不**需要这个 flag——它们启动的是真正要接管服务的实例,本来就应该有 | |
| 83 | +复原循环。 | |
| 84 | + | |
| 73 | 85 | ## 7. 后续(不在本次范围内) |
| 74 | 86 | |
| 75 | 87 | FR-1(新 Goal 自动派发)仍然是需求文档标注的"改变系统自主权边界"的决策,按该文档非目标 #1 | ... | ... |
docs/todo/scheduler-production-polling-loop-requirements.md
| ... | ... | @@ -2,7 +2,18 @@ |
| 2 | 2 | |
| 3 | 3 | **日期**: 2026-08-06 |
| 4 | 4 | **性质**: 需求文档(BRD/PRD 层面),不含详细技术设计。回应一次真实事故的根因排查——任务 `3605272e-fe33-48cc-b3f3-e0c0ba26f970` 在 `validating` 状态卡死 15+ 小时,`execution_event` 表在此期间零记录,`work_graph_run` 里从未出现 Validator 的运行记录。 |
| 5 | -**状态**: 草案,待评审。 | |
| 5 | +**状态**: FR-2(在途任务复原)✅ 已实现(2026-08-06)。FR-1(新 Goal 自动派发)仍是草案,按第 3 节非目标/第 7 节风险的既定判断,未与 FR-2 一起做——它改变系统自主权边界,需要单独评审决策。 | |
| 6 | + | |
| 7 | +## 0. FR-2 实现记录(2026-08-06) | |
| 8 | + | |
| 9 | +- `src/task_reconciliation.py`(新增):`find_stale_resumable_tasks()`(纯查询,判定信号是 `execution_event` 里该任务最近一条记录的 `MAX(created_at)`,没有则退回 `task.created_at`——不是 `task.updated_at`,理由见模块 docstring 和 FR-3)+ `reconcile_once()`(扫描后逐个 dispatch,单个候选 dispatch 失败不影响其余候选,呼应 04-P"调度器继续执行队列中其他任务")。 | |
| 10 | +- `src/task_execution.py`:新增进程内 `_in_flight_task_ids`(`threading.Lock` 保护的内存 set)+ `is_in_flight()`/`_try_claim()`/`_release()`,`start_task_execution_in_background()` 接线为"已在途则立即 `TaskExecutionError`,不重复派发"——这正是 FR-5 之外一条隐含但必须有的约束(防止复原循环和人工点 Start 竞争同一个任务)。 | |
| 11 | +- `src/backend_server.py`:新增 `_start_reconciliation_loop()`,**只在 `main()`(真实 CLI 入口)里启动,不放进 `create_server()`**——后者被几乎每个 `test_backend_server.py` 测试实例化,放进去会让每次跑测试都触发真实的后台任务派发。新增 CLI 参数 `--reconciliation-interval-seconds`(默认 300)、`--disable-reconciliation-loop`。 | |
| 12 | +- `stale_after_seconds` 默认 1800(30 分钟)——比这套代码里最长的单节点默认超时(Validator 自己的 Work Graph `timeout=1200`)留了充分余量,正常还在跑的节点不会被误判。 | |
| 13 | +- **实现过程中真实抓到并修了两个问题,不是纸面设计就想到的**: | |
| 14 | + 1. 测试污染:`_in_flight_task_ids` 是进程级全局状态,第一版新增测试里有一个没有完整 mock 掉后续真实调用路径,导致被测线程超出 `join(timeout=10)` 仍在跑,"task-1" 这个在测试文件里被大量复用的字面量 task_id 永久卡在 in-flight 集合里,导致全量跑 `discover -s src` 时(而不是单独跑这个文件时)另外 3 个无关测试跟着炸——已修正 mock 完整性,并在 `StartTaskExecutionInBackgroundTest` 加了防御性 `setUp`/`tearDown` 兜底清理。 | |
| 15 | + 2. 生产副作用:Deploy Supervisor 的候选进程(`supervisor/deploy_stage.py::_launch_candidate`)启动的 `backend_server.py` 实例,`--db-path` 指向的是**生产库的一份拷贝**——默认开启复原循环会让候选进程一启动就对着这份拷贝库真的派发 `claude -p` 调用,这在候选还没通过健康检查、Deploy Supervisor 还没决定要不要采用它之前完全不该发生。已在 `_launch_candidate` 加 `--disable-reconciliation-loop`,并补了对应的代码注释说明原因。 | |
| 16 | +- `python3 -m unittest discover -s src`(635 tests)+ `discover -s supervisor -p "test_*.py"`(125 tests)全绿,零回归。 | |
| 6 | 17 | |
| 7 | 18 | ## 1. 背景 |
| 8 | 19 | ... | ... |
supervisor/deploy_stage.py
| ... | ... | @@ -119,9 +119,23 @@ def _copy_prod_db(prod_db_path, deployment_id): |
| 119 | 119 | |
| 120 | 120 | |
| 121 | 121 | def _launch_candidate(candidate_dir, port, db_path, python_executable=None): |
| 122 | + """`--disable-reconciliation-loop` (2026-08-06, task_reconciliation): | |
| 123 | + `db_path` here is a COPY of the real production db (`_stage_candidate_db` | |
| 124 | + above) -- it can genuinely contain resumable-status tasks. Without this | |
| 125 | + flag, a candidate's own reconciliation loop would find them "stale" (its | |
| 126 | + fresh process has no in-flight record for anything) and start | |
| 127 | + dispatching real `claude -p` subprocess calls against the throwaway | |
| 128 | + candidate db during what's supposed to be a passive health-check-only | |
| 129 | + process, before Deploy Supervisor has even decided whether this | |
| 130 | + candidate is good. Found for real: this exact scenario made candidate | |
| 131 | + health checks fail/hang in `supervisor`'s own test suite the first time | |
| 132 | + the reconciliation loop shipped with no guard here.""" | |
| 122 | 133 | python_executable = python_executable or sys.executable |
| 123 | 134 | return subprocess.Popen( |
| 124 | - [python_executable, "-m", "src.backend_server", "--host", "127.0.0.1", "--port", str(port), "--db-path", db_path], | |
| 135 | + [ | |
| 136 | + python_executable, "-m", "src.backend_server", "--host", "127.0.0.1", "--port", str(port), | |
| 137 | + "--db-path", db_path, "--disable-reconciliation-loop", | |
| 138 | + ], | |
| 125 | 139 | cwd=candidate_dir, stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, |
| 126 | 140 | ) |
| 127 | 141 | ... | ... |
supervisor/test_deploy_stage.py
| ... | ... | @@ -198,6 +198,23 @@ class CheckoutFailedTest(DeployStageTestBase): |
| 198 | 198 | mock_launch.assert_not_called() |
| 199 | 199 | |
| 200 | 200 | |
| 201 | +class LaunchCandidateArgsTest(unittest.TestCase): | |
| 202 | + """2026-08-06 (task_reconciliation): `_launch_candidate`'s `db_path` is | |
| 203 | + always a COPY of the real production db -- without `--disable- | |
| 204 | + reconciliation-loop` on this exact argv, a candidate's own reconciliation | |
| 205 | + loop would find real resumable-status tasks in that copy and start | |
| 206 | + dispatching genuine `claude -p` calls before Deploy Supervisor has even | |
| 207 | + decided the candidate is healthy. Cheap, mocked-Popen unit test so a | |
| 208 | + future regression here fails fast instead of only surfacing as a hang in | |
| 209 | + RealCandidateLaunchTest or a real production incident.""" | |
| 210 | + | |
| 211 | + def test_disable_reconciliation_loop_flag_present(self): | |
| 212 | + with patch.object(deploy_stage.subprocess, "Popen") as mock_popen: | |
| 213 | + deploy_stage._launch_candidate("/some/dir", 12345, "/some/db.sqlite") | |
| 214 | + argv = mock_popen.call_args[0][0] | |
| 215 | + self.assertIn("--disable-reconciliation-loop", argv) | |
| 216 | + | |
| 217 | + | |
| 201 | 218 | class RealCandidateLaunchTest(unittest.TestCase): |
| 202 | 219 | """Not mocked -- stages a real candidate checkout of graphlemon's OWN |
| 203 | 220 | repo (read-only `git worktree add --detach` at the current HEAD, cleaned | ... | ... |