Repository navigation
fix(runtime-broker): answer a refused provider start as unknown instead of prepared - #13064
Conversation
Since 9cb9dc8 (#12868), the Runtime Broker reconciled an UNKNOWN provider record against the worker and answered 200 with the observed state prepared for a start the worker had refused. Nothing dispatches such a call a second time, so the provider client's startExecution polled every 50 ms without an end. Keep a provider execution the worker still holds as prepared answered as 409 runtime_broker_execution_unknown, as before the change; lost answers keep settling from the worker's retained result. Fixes #13059
github-actions
Bot
added
the
review/self-reported
Verification reportReproduction and fix verification were run on this branch at unit level (the defect site is driven with the worker's
|
|
Independent source verification of this PR against PR head Defect site (verified)
The fix is correct and minimal
The lost-answer path is genuinely unaffectedFor a call the worker did run, the worker answers Test soundness
One nuance worth notingThe fix keys "never started" off the worker's reported Verdict: the change is correct, minimal, and well-tested; the lost-answer and cancellation paths are verified untouched. Endorsed. |
The neverStarted guard serves the :cancel route through the same envelope as :start and the read; cover that combination: an UNKNOWN provider record whose worker still answers prepared is answered 409 runtime_broker_execution_unknown, while the physical cancellation still reaches the worker. Both mutations redden the assertion: removing the guard, and narrowing it to the start route. Refs #13059
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — reviewed at head 835cfad1a407772b0cbab96669e6be4e9186ac9d.
Critical-only scan found no merge-blocking defect. Both files in the diff were read in full, along with the predicate the new guard keys on and every caller of the mapper it changes.
No blocker on record
One finding was ever filed against this PR, at the previous head, and it is severity S: the changed mapper also serves the :cancel route, whose new answer no test pinned. The round that reviewed this head posted zero findings. That round discloses that its verifier never launched and eight findings kept an [unverified] tag, but its ledger published none of them, so nothing stands asserted against this head.
The single Suggestion was also acted on rather than left open. The new test now posts /executions/{id}:cancel while the worker still answers "prepared" and asserts 409 with code == "runtime_broker_execution_unknown", plus assertEquals(1, fixture.transport.executions.get()) — precisely the extension that finding prescribed, including its no-redispatch clause.
The narrowing is scoped exactly as tightly as it needs to be
The guard adds one conjunct, && !neverStarted, to an existing condition, so the branch can only become narrower and record.getState() == UNKNOWN is still required. Everything turns on neverStarted being true only for a provider call the worker genuinely never began, and both halves hold:
ProviderRuntimeProtocol.isReferenceis key-set equality, not a subset test. It isreference != null && reference.keySet().equals(REFERENCE_FIELDS), whereREFERENCE_FIELDSis exactly the seven provider fields (sessionId,promptId,callId,capabilityDigest,policyRevision,invocationId,argsDigest). A raw reference carries five, so it can never satisfyequals, andnullshort-circuits to false. The raw path therefore keeps the previous answer byte for byte — which matters, because a raw deferred start legitimately observespreparedbefore its payload arrives.- The state term is
"prepared"alone. An execution whose start reply was lost after the worker began is observed asexecuting,cancel_requestedorsettled, none of which match, so the response-loss reconciliation this server already performs is untouched. Only a worker that still holds the call as prepared — one that never began it, and that nothing will dispatch a second time — falls through, which is the forever-poll the fix exists to remove.
The fall-through is not a dead end either: runtime_broker_execution_unknown is raised as a non-retryable 409, and the test asserts the physical cancellation still reaches the worker (cancellations.get() == 1), so the refused call is cleaned up rather than orphaned.
All three call sites of the changed mapper are exercised
observedExecutionEnvelope has exactly three callers in RuntimeBrokerHttpServer.java (lines 278, 295, 314) — :start, :cancel and GET /executions/{id}. The new test drives all three: two consecutive :start attempts both answering 409 with the unknown code, the :cancel, and a GET read also answering 409. Repeating the start is what pins idempotence of the new answer, and the trailing executions.get() == 1 is what proves the refusal did not cause a second physical dispatch.
The new test provably ran, and the pinned neighbours still hold
Runtime Broker and Managed Agent MariaDB / Java 21 at this head reports Tests run: 20, Failures: 0, Errors: 0, Skipped: 0 -- in com.alibaba.qwen.code.runtimebroker.RuntimeBrokerHttpServerTest, four BUILD SUCCESS and no failure. The class carried 19 tests before this addition, so the twentieth is the new one and it was neither skipped nor failed.
That same green run is the evidence that the narrowing did not disturb the behaviours the finding warned against widening past: the existing cases pinning that a provider UNKNOWN record observed as cancel_requested still answers 200 with status.state == "cancel_requested", and the response-loss reconciliation case immediately following the new test, both still pass unmodified.
CI
Green at this head: the full Java matrix (ubuntu-latest / Java 11, 17, 21, windows-latest / Java 21, macos-latest / Java 21), Runtime Broker and Managed Agent MariaDB / Java 21, Hosted process fault gates / MySQL 8.4 / Java 21, Real daemon E2E / Java 11, Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and web-shell E2E Smoke. review-pr is pending, which this channel does not treat as a gate.
One honest limit on this review: I read the code and the CI evidence rather than executing the suite myself, so the mutation argument in the earlier Suggestion — that a later edit moving the guard into the :start handler alone would pass silently — is now closed for the cancel route by the assertion above, but I did not run that mutation to confirm it.
|
@qwen-code /triage |
3 participants
What this PR does
The Runtime Broker's observed-execution envelope now keeps a provider execution the worker still holds as
preparedanswered as409 runtime_broker_execution_unknown, instead of reporting the observedpreparedstate with200. A worker that reportspreparedfor a provider call refused the dispatch, and nothing dispatches such a call a second time, so the call stays UNKNOWN for the caller — the same answer the Broker gave before #12868. A witness test pins the refused start answered 409 twice (idempotent), the follow-up read answered 409, and exactly one worker dispatch (no re-dispatch, no status polling).Why it's needed
Since
9cb9dc86e8(#12868), the Broker reconciles an UNKNOWN provider record against the worker and answered200 preparedfor a start the worker had refused (409 managed_runtime_provider_operation_failed). The provider client'sstartExecutionpolls every 50 ms until the state issettled, so a refused call waited without an end and loaded the worker — measured on the real chain: 335statusrequests in 20 s, and 4,549 over 4 min 26 s in a no-limit probe. The lost-answer reconciliation that change was written for is unaffected: a worker that did run the call answersexecutingorsettled, neverprepared, and the existing lost-answer and cancellation tests (provider and v3) keep passing unchanged.Reviewer Test Plan
How to verify
Unit level (JDK 21, Maven): in
packages/sdk-java/runtime-broker,mvn test— the new testproviderStartTheWorkerNeverBeganStaysUnknownForTheCallerfails without the one-condition change in the observed-execution envelope (expected: <409> but was: <200>) and passes with it; the full module suite passes 514 tests, 0 failures. Expected behavior on the refused path:startExecutionis refused in ~0.1 s with409 runtime_broker_execution_unknownand the worker sees a singleexecuterequest; a lost answer still settles from the worker's retained result.Evidence (Before & After)
N/A — non-UI change. Measured evidence on the real chain (built TypeScript provider → Spring-embedded Runtime Broker on MySQL 8.4.7 → bundled worker, boots v1 and v2) is recorded in the linked issue: with this change the refused start is answered in 0.1 s, the worker receives 1
statusrequest instead of 335, and the verification probes pass 21/21, 76/76 and 12/12 (without it: 19/21, 75, 10).Tested on
Environment (optional)
Unit tests run with JDK 21 (
maven.compiler.release21) and Maven 3.8.4. The module's default surefire suite; themysql-integrationprofile and fault-gate group are opt-in and unchanged.Risk & Scope
preparedafter accepting its dispatch would now read as UNKNOWN to the caller. The worker reportsexecuting/settledonce it accepts a dispatch, so that state only ever means "refused, never started"; the real-chain probes above cover this.statusanswer fixed toprepared); the alternative client-side fix (stop polling when a started call readsprepared) was not pursued.Linked Issues
Fixes #13059
中文说明
这个 PR 做了什么
Runtime Broker 的观测执行应答（observed-execution envelope）现在把 worker 仍持有为
prepared的 provider 执行保持应答为409 runtime_broker_execution_unknown，不再以200上报观测到的prepared状态。worker 对某个 provider 调用报prepared意味着它拒绝了这次派发，而这样的调用不会有第二次派发，因此对调用方保持 UNKNOWN——与 #12868 之前 Broker 给出的应答一致。新增见证测试钉住：被拒绝的 start 两次都得到 409（幂等）、随后的读取得到 409、且 worker 只收到一次派发（不二次派发、不轮询状态）。为什么需要
自
9cb9dc86e8（#12868）起，Broker 会对 UNKNOWN 的 provider 记录向 worker 做对账，并对 worker 已拒绝的 start（409 managed_runtime_provider_operation_failed）应答200 prepared。provider 客户端的startExecution每 50 毫秒轮询一次直到状态变为settled，于是被拒绝的调用无止境地等待并持续给 worker 加压——真实链路实测：20 秒内 335 次status请求；不设上限的探针在 4 分 26 秒内发出 4,549 次。该改动原本要解决的应答丢失对账不受影响：真正运行了调用的 worker 会回答executing或settled，绝不会是prepared，现有的应答丢失与取消测试（provider 与 v3）全部原样通过。评审者测试方案
单元级（JDK 21，Maven）：在
packages/sdk-java/runtime-broker中运行mvn test——新增测试providerStartTheWorkerNeverBeganStaysUnknownForTheCaller在去掉观测应答信封里那一个条件时会失败（expected: <409> but was: <200>），保留时通过；模块全量套件 514 个测试通过、0 失败。被拒绝路径的期望行为：startExecution约 0.1 秒内以409 runtime_broker_execution_unknown被拒绝，worker 只看到一次execute请求；应答丢失的情形仍按 worker 保留的结果结算。证据（前后对比）
N/A——非 UI 改动。真实链路（构建出的 TypeScript provider → Spring 内嵌 Runtime Broker，MySQL 8.4.7 → 打包 worker，boot v1 与 v2）上的实测记录在关联 issue 中：加上本改动后被拒绝的 start 在 0.1 秒内得到应答，worker 收到 1 次
status请求而非 335 次，各验证探针为 21/21、76/76、12/12（不加时为 19/21、75、10）。测试平台
macOS ✅；Windows / Linux N/A（由 CI 覆盖）。
环境（可选）
单元测试使用 JDK 21（
maven.compiler.release21）与 Maven 3.8.4。模块默认 surefire 套件；mysql-integrationprofile 与 fault-gate 分组为可选启用，未改动。风险与范围
prepared，调用方现在会读到 UNKNOWN。worker 一旦接受派发即上报executing/settled，因此该状态只意味着「被拒绝、从未启动」；上述真实链路探针覆盖了这一点。status应答固定为prepared，驱动的是同一缺陷点）；未采用另一个客户端侧修法（start 过的调用读到prepared即停止等待）。关联 Issue
Fixes #13059