Skip to content

fix(httpapi): 取消 fan-out 执行前先校验 Run 绑定 - #495

Merged
Qiyuanqiii merged 1 commit into
mainfrom
Qiyuanqiii/fanout-run-boundary
Oct 27, 2026
Merged

Qiyuanqiii merged 1 commit into
mainfrom
Qiyuanqiii/fanout-run-boundary

Conversation

@Qiyuanqiii

Copy link
Copy Markdown
Member

POST /api/v1/runs/{run_id}/fanout-executions/{execution_id}/cancel 先写后判。处理函数在 internal/httpapi/fanout_execution_control.go:156-157 调用 CancelReadOnlyFanoutExecution,服务在 internal/application/readonly_fanout_execution.go:917 起的实现里按 execution 自己的绑定加载 plan、取 plan 所属 Run 的执行租约,然后依次 RecoverReadOnlyFanoutExecution → CancelReadOnlyFanoutExecutionRemainder → FinalizeReadOnlyFanoutExecution;直到这些都 durable 提交之后,处理函数才在 :162 比较 execution.RunID != runID 并回 404 “fan-out execution is not part of the run”。也就是说路径里的 run_id 只决定响应体,不决定要不要执行这次取消:用一个无关的 Run 去寻址,另一个 Run 的执行仍然从 running 变成 cancelled,那个 Run 的在跑 worker 的租约 generation 也被 fence 掉,而调用方收到的是 404——按 404 语义它认为什么都没发生,于是重试或回滚判断都建立在错误前提上。同一条缺失的绑定检查也让 GET 集合可越界:listFanoutExecutions(:57-64)只用 GetRun(runID) 做一次存在性探测,取数完全按 plan_id,所以另一个 Run 的分片历史会以 200 出现在这个 Run 的集合里;FanoutExecutionsListView 与 FanoutExecutionView 都不带 run_id 字段(:32-40、fanoutExecutionFullView),网页侧的 parseFanoutExecutions 只校验 plan_id 与 items 形状,HTTP 层是唯一能拦的地方。

两个路由的文档契约都把它们写成 Run 作用域、以 NotFound 表达不可寻址:cancelRunFanoutExecution 是 Control: true, NotFound: true, Parameters: []openAPIParameter{runID, executionID}(internal/httpapi/openapi.go:1785-1792),listRunFanoutExecutions 是 NotFound: true, Parameters: []openAPIParameter{runID, identityQueryParameter("plan_id", …)}(internal/httpapi/openapi.go:1730-1734);路由确实注册在 runs/{run_id}/fanout-executions 之下(internal/httpapi/handlers.go:543)。所以这里没有需要重新解释的余地:绑定应当在写之前成立。

需要先说清这个缺陷的严重性定位,避免把它当成提权通道。这两个路由都要求 control bearer(fanout_execution_control.go:101-107),而 authorized()(internal/httpapi/server.go:1702-1713)只把 bearer 与单一全局 controlTokenHash 做常数时间比较——持有 control token 的一方本来就能对任意 Run 取消执行。因此问题不是“读到了不该读的、改了不该改的权限域”,而是已经改完了却报告没找到:状态机与租约栅栏被真实推进,响应却是 404,加上集合路由把别的 Run 的历史当成当前 Run 的数据返回。修复方向是把既有绑定提前到副作用之前,不是收紧授权。

实现上把 Run 变成请求身份的一部分,与本仓库既有的“控制器自己认作用域”写法一致(internal/httpapi/controlled_command_proposal_control.go:220-231 的 List(ctx, runID, limit) 就是这种形状):

  • CancelReadOnlyFanoutExecution(ctx, runID, executionID, requestedBy, operationKey):runID 进 ValidAgentID 校验,并在 GetReadOnlyFanoutExecution + GetReadOnlyFanoutPlan 之后、withRunExecutionLease 之前用 execution.RunID != runID || plan.RunID != runID 判 CodeNotFound。plan 与 execution 的 Run 绑定不可变,所以这是前置条件而不是竞态检查;处理函数原有的 :162 判等保留作数据损坏兜底。
  • listFanoutExecutions:对服务返回的每一行核对 execution.RunID != runID,不符则 CodeNotFound。这里刻意不改 ListReadOnlyFanoutExecutions 的签名:plan 不存在时维持现有 200 空投影,只有“plan 属于别的 Run 且真有历史”才 404,避免把 404 与 200 空列表的差变成存在性探针。

两处调用方可见的变化我不回避:

  1. 跨 Run 寻址一个已终态的 execution,旧代码会在绑定检查之前就返回它的存储状态(200 + 别人 Run 的状态),现在与在跑的一样得到 404。幂等语义只对自己的 Run 保留——我把这条写进正向对照,running → cancelled → 重复 cancel 仍 200 三步都锁住。
  2. 跨 Run 的 list 从 200 + 别人的历史 变成 404。自家 Run 的 list 仍是 200 且带同一条 execution。

CancelReadOnlyFanoutExecution 只有一个实现(ReadOnlyFanoutExecutionService,grep 无第二个签名匹配点)和一个 HTTP 调用点,FanoutExecutionController 也没有测试替身(internal/httpapi/openapi_test.go:878 用的就是真实服务),所以签名加宽由编译器覆盖,不需要新增接口方法或包装。internal/store/readonly_fanout_execution_test.go:305/314/359 三处调用改为传 executions[0].RunID——它们测的本来就是这条 execution 自己的 Run,语义未变。

红→绿(go test -count=1 -timeout 480s -run "TestFanoutExecution" ./internal/httpapi/,base 274fefb6):

--- FAIL: TestFanoutExecutionCancelEnforcesRunBoundaryBeforeMutating (0.51s)
    cancellation crossed the documented Run boundary before the identity check: execution
    "fanout-execution-audit-boundary" owned by "run-...6bcd88c35ba8" became "cancelled" while the
    caller addressed "run-..." and received 404
--- FAIL: TestFanoutExecutionListEnforcesRunBoundary (0.50s)
    run-scoped collection served execution history owned by another Run through plan
    "fanout-plan-...ddf05b23791b": 200 {..."items":[{"id":"fanout-execution-audit-boundary","status":"running"...

修复后同一个 -run 通过。第三条 TestFanoutExecutionRoutesServeTheirOwnRun 是正向对照,它在 base 上就通过、修复后仍通过,用来证明这不是把可用路径一起关掉。fixture 用真实的 ReadOnlyFanoutPlanService 建 plan、真实的 CreateReadOnlyFanoutExecution(带租约)建 running execution,并把真实服务接成控制器,两个 Run 除绑定外没有别的差异,所以判定只能由边界检查决定;断言只打在公开 HTTP 入口与 store.GetReadOnlyFanoutExecution 的落库状态上,不复制 SQL。

验证:

  • go test -count=1 ./internal/httpapi/(含新测试文件)全包通过:ok cyberagent-workbench/internal/httpapi 215.386s。
  • go test -count=1 -run "ReadOnlyFanout|Fanout" ./internal/store/ ./internal/application/ 通过:ok …/store 19.438s、ok …/application 0.841s。
  • go vet ./internal/httpapi/ ./internal/application/ ./internal/store/ 通过;git diff --check 干净。gofmt -l 对我改动的四个文件里除 fanout_execution_control.go 外无输出,而 fanout_execution_control.go 在 base 上就已经被 gofmt 标记(const 块对齐、文件末尾多一个空行),我没有顺手重排这两处无关格式,以保持 diff 只包含本缺陷。
  • 我没有跑 go test ./... 全量:本机 internal/store、internal/application 的单包全量都在 10 分钟默认超时上 panic: test timed out(超时时在跑逐历史 schema 与 docker 相关用例,不是断言失败),CI 正是把它们拆成 shard。全量以本 PR 的 CI 为准。
  • OCR delegate 复核(开 PR 前执行):ocr delegate preview --from 274fefb6 --to HEAD → 2 个可复核文件 / 共 4 个改动文件(fanout_run_boundary_test.go、readonly_fanout_execution_test.go 被 default_path 规则排除,已人工复核:只加断言与 runID 实参,不改原测试意图,无跳过、无放宽)。按 **/*.go 规则组逐条核对:Security-Sensitive Boundaries——新增的是绑定校验,两条错误消息不回显 id、路径或 token;Errors/API 契约——CodeNotFound 与两路由声明的 NotFound: true 一致,InvalidArgument 只覆盖本来就该被 validatePathIdentity 拒掉的形态;Nil/Interfaces——接口签名加宽由编译期覆盖唯一实现;Context/锁/资源——没有在租约或锁内新增阻塞 I/O,withRunExecutionLease 与 Recover/Finalize 顺序未变,plan 读取提前只多一次只读查询;数值边界——无。无发现。

无 Surface 变化:DTO、请求/响应形状、路径与参数都没有改动,docs/openapi.json 无需再生成(两个操作现有描述已经承诺 Run 作用域与 NotFound),protocols/registry.json 不变;本 PR 不改变任何门禁、授权、预算或执行判定的强度,只把已经存在的 Run 绑定从“副作用之后判定”提前到“副作用之前判定”,并把集合路由的绑定核验补到返回行上;不含凭据与本地运行数据。

POST /api/v1/runs/{run_id}/fanout-executions/{execution_id}/cancel compared the
cancelled execution's owning Run only after CancelReadOnlyFanoutExecution had
already recovered, cancelled and finalized it, so a cancellation addressed to an
unrelated Run destroyed the other Run's execution and answered 404. GET the
run-scoped collection keyed purely on plan_id and returned another Run's shard
history as a 200 body.

CancelReadOnlyFanoutExecution now takes the addressable Run and rejects a foreign
execution or plan binding before it acquires the lease or writes anything, and
the list route verifies every execution it returns belongs to its own Run.
@Qiyuanqiii
Qiyuanqiii force-pushed the Qiyuanqiii/fanout-run-boundary branch from a71a5e2 to 29c1605 Compare October 27, 2026 17:45
@Qiyuanqiii
Qiyuanqiii merged commit 14888de into main Oct 27, 2026
18 checks passed
@Qiyuanqiii
Qiyuanqiii deleted the Qiyuanqiii/fanout-run-boundary branch October 27, 2026 17:56
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.

2 participants