From 47236648e9b179bc59f11c345806b37931fdc131 Mon Sep 17 00:00:00 2001 From: bboonnee Date: Thu, 6 Aug 2026 17:38:55 +0800 Subject: [PATCH] I: fix reconciliation loop leaking into Deploy Supervisor candidates --- docs/todo/scheduler-production-polling-loop-design.md | 12 ++++++++++++ docs/todo/scheduler-production-polling-loop-requirements.md | 13 ++++++++++++- supervisor/deploy_stage.py | 16 +++++++++++++++- supervisor/test_deploy_stage.py | 17 +++++++++++++++++ 4 files changed, 56 insertions(+), 2 deletions(-) diff --git a/docs/todo/scheduler-production-polling-loop-design.md b/docs/todo/scheduler-production-polling-loop-design.md index fcf2cc2..fe3b9f4 100644 --- a/docs/todo/scheduler-production-polling-loop-design.md +++ b/docs/todo/scheduler-production-polling-loop-design.md @@ -70,6 +70,18 @@ 4. ✅ `--disable-reconciliation-loop` 提供独立开关；FR-2 与 FR-1 天然独立（FR-1 尚未实现， 两者不存在耦合）。 +## 6.1 部署候选进程的联动修复（2026-08-06，与本设计同批） + +`supervisor/deploy_stage.py::_launch_candidate` 启动的候选 `backend_server` 进程，`db_path` +是生产库的一份拷贝（`_copy_prod_db`）——候选进程本身默认也会带上复原循环，扫到拷贝库里的 +可恢复任务后会在 Deploy Supervisor 还没做健康检查/裁定之前就真的发起 `claude -p` 调用。已给 +`_launch_candidate` 补上 `--disable-reconciliation-loop`，并在 +`supervisor/test_deploy_stage.py::LaunchCandidateArgsTest` 加了一个轻量单测直接断言 argv 里有 +这个 flag（之前只有一个真实拉起进程验证 HTTP 可用的重量级测试，不会对这类"忘加一个 flag"的 +回归报错）。`supervisor/deploy_rollback.py`/`deploy_cutover.py` 里另外两处 `_launch_candidate` +调用点（Live/回滚路径）**不**需要这个 flag——它们启动的是真正要接管服务的实例，本来就应该有 +复原循环。 + ## 7. 后续（不在本次范围内） FR-1（新 Goal 自动派发）仍然是需求文档标注的"改变系统自主权边界"的决策，按该文档非目标 #1 diff --git a/docs/todo/scheduler-production-polling-loop-requirements.md b/docs/todo/scheduler-production-polling-loop-requirements.md index d464f83..b7b10e5 100644 --- a/docs/todo/scheduler-production-polling-loop-requirements.md +++ b/docs/todo/scheduler-production-polling-loop-requirements.md @@ -2,7 +2,18 @@ **日期**: 2026-08-06 **性质**: 需求文档（BRD/PRD 层面），不含详细技术设计。回应一次真实事故的根因排查——任务 `3605272e-fe33-48cc-b3f3-e0c0ba26f970` 在 `validating` 状态卡死 15+ 小时，`execution_event` 表在此期间零记录，`work_graph_run` 里从未出现 Validator 的运行记录。 -**状态**: 草案，待评审。 +**状态**: FR-2（在途任务复原）✅ 已实现（2026-08-06）。FR-1（新 Goal 自动派发）仍是草案，按第 3 节非目标/第 7 节风险的既定判断，未与 FR-2 一起做——它改变系统自主权边界，需要单独评审决策。 + +## 0. FR-2 实现记录（2026-08-06） + +- `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"调度器继续执行队列中其他任务"）。 +- `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 竞争同一个任务）。 +- `src/backend_server.py`：新增 `_start_reconciliation_loop()`，**只在 `main()`（真实 CLI 入口）里启动，不放进 `create_server()`**——后者被几乎每个 `test_backend_server.py` 测试实例化，放进去会让每次跑测试都触发真实的后台任务派发。新增 CLI 参数 `--reconciliation-interval-seconds`（默认 300）、`--disable-reconciliation-loop`。 +- `stale_after_seconds` 默认 1800（30 分钟）——比这套代码里最长的单节点默认超时（Validator 自己的 Work Graph `timeout=1200`）留了充分余量，正常还在跑的节点不会被误判。 +- **实现过程中真实抓到并修了两个问题，不是纸面设计就想到的**： + 1. 测试污染：`_in_flight_task_ids` 是进程级全局状态，第一版新增测试里有一个没有完整 mock 掉后续真实调用路径，导致被测线程超出 `join(timeout=10)` 仍在跑，"task-1" 这个在测试文件里被大量复用的字面量 task_id 永久卡在 in-flight 集合里，导致全量跑 `discover -s src` 时（而不是单独跑这个文件时）另外 3 个无关测试跟着炸——已修正 mock 完整性，并在 `StartTaskExecutionInBackgroundTest` 加了防御性 `setUp`/`tearDown` 兜底清理。 + 2. 生产副作用：Deploy Supervisor 的候选进程（`supervisor/deploy_stage.py::_launch_candidate`）启动的 `backend_server.py` 实例，`--db-path` 指向的是**生产库的一份拷贝**——默认开启复原循环会让候选进程一启动就对着这份拷贝库真的派发 `claude -p` 调用，这在候选还没通过健康检查、Deploy Supervisor 还没决定要不要采用它之前完全不该发生。已在 `_launch_candidate` 加 `--disable-reconciliation-loop`，并补了对应的代码注释说明原因。 +- `python3 -m unittest discover -s src`（635 tests）+ `discover -s supervisor -p "test_*.py"`（125 tests）全绿，零回归。 ## 1. 背景 diff --git a/supervisor/deploy_stage.py b/supervisor/deploy_stage.py index 1fb7045..661cfa8 100644 --- a/supervisor/deploy_stage.py +++ b/supervisor/deploy_stage.py @@ -119,9 +119,23 @@ def _copy_prod_db(prod_db_path, deployment_id): def _launch_candidate(candidate_dir, port, db_path, python_executable=None): + """`--disable-reconciliation-loop` (2026-08-06, task_reconciliation): + `db_path` here is a COPY of the real production db (`_stage_candidate_db` + above) -- it can genuinely contain resumable-status tasks. Without this + flag, a candidate's own reconciliation loop would find them "stale" (its + fresh process has no in-flight record for anything) and start + dispatching real `claude -p` subprocess calls against the throwaway + candidate db during what's supposed to be a passive health-check-only + process, before Deploy Supervisor has even decided whether this + candidate is good. Found for real: this exact scenario made candidate + health checks fail/hang in `supervisor`'s own test suite the first time + the reconciliation loop shipped with no guard here.""" python_executable = python_executable or sys.executable return subprocess.Popen( - [python_executable, "-m", "src.backend_server", "--host", "127.0.0.1", "--port", str(port), "--db-path", db_path], + [ + python_executable, "-m", "src.backend_server", "--host", "127.0.0.1", "--port", str(port), + "--db-path", db_path, "--disable-reconciliation-loop", + ], cwd=candidate_dir, stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) diff --git a/supervisor/test_deploy_stage.py b/supervisor/test_deploy_stage.py index 637f2e2..da5ee71 100644 --- a/supervisor/test_deploy_stage.py +++ b/supervisor/test_deploy_stage.py @@ -198,6 +198,23 @@ class CheckoutFailedTest(DeployStageTestBase): mock_launch.assert_not_called() +class LaunchCandidateArgsTest(unittest.TestCase): + """2026-08-06 (task_reconciliation): `_launch_candidate`'s `db_path` is + always a COPY of the real production db -- without `--disable- + reconciliation-loop` on this exact argv, a candidate's own reconciliation + loop would find real resumable-status tasks in that copy and start + dispatching genuine `claude -p` calls before Deploy Supervisor has even + decided the candidate is healthy. Cheap, mocked-Popen unit test so a + future regression here fails fast instead of only surfacing as a hang in + RealCandidateLaunchTest or a real production incident.""" + + def test_disable_reconciliation_loop_flag_present(self): + with patch.object(deploy_stage.subprocess, "Popen") as mock_popen: + deploy_stage._launch_candidate("/some/dir", 12345, "/some/db.sqlite") + argv = mock_popen.call_args[0][0] + self.assertIn("--disable-reconciliation-loop", argv) + + class RealCandidateLaunchTest(unittest.TestCase): """Not mocked -- stages a real candidate checkout of graphlemon's OWN repo (read-only `git worktree add --detach` at the current HEAD, cleaned -- libgit2 0.21.4