Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test(runtime-broker): address self-audit round 2 on the 13183 branch
An adversarial pass over the tests and an undirected pass over the whole
diff found no Critical and fifteen Suggestions; thirteen are fixed here
and two are declined in the PR thread.

The headline is that the cross-process stress never exercised half of its
own matrix: instrumenting it shows admission won 0 of 200 rounds, because
the release goes straight to the session row lock while admission first
takes the placement-domain lock and does two reads. Half the rounds now
hold the release until the admission has committed, and the test asserts
both directions occurred, so dropping the FOR UPDATE that the whole fix
rests on turns it red with two contradictions instead of passing.

Three more assertions were satisfiable without the code they name:

- The settled-cache replay branch could be deleted without failing its
  test, because the version comparison picked the same record; the stale
  snapshot is now pinned to the settled record's own version.
- The "serve the fresher of two UNKNOWN records" rule was only covered
  with the cache holding the newer record, so collapsing that comparison
  survived the whole suite; a cancel now lands after the cached lookup
  too.
- Deleting the renewal pool's shutdown in close() was invisible, since a
  tick cancels itself while a pool's threads exit only on shutdown; a new
  test tracks the thread IDs an in-flight dispatch starts and asserts
  they are gone after close.

The v3 poll cap moves off the wall clock: the delay computation is
extracted and asserted exactly for attempts 0-7, which tells a 2s cap
from a 2.5s one in 0.2s and drops a 900ms scheduler-jitter budget from
the timing test.

Three tests that cannot fail on Windows now say so, since destroy()
terminates outright there and --ignore-term cannot wedge a worker. The
exit-hook test no longer inherits its observation window from a sleep
inside the forked harness, and the provision-race test's javadoc claims
only the guard it can reach: spawn-and-register atomicity is structural,
inside the lifecycle lock, not observable from outside it. A refused
release is now retried to prove it left the path usable, and the embedded
broker's window validation covers its null and millisecond arms.

Documentation that described behaviour the code does not have: the
deployment README called v3-result-window a result-retention period when
it is the post-dispatch polling deadline whose expiry marks the execution
UNKNOWN; the new repository primitive's contract omitted the
runtime_session_not_ready outcome both implementations throw; the window
floor's two comments justified 1s by a poll schedule that would support
100ms; the HTTP layer claimed a mutation response describes post-mutation
truth, which the pre-existing in-flight lookup merge does not guarantee;
and the design doc's problem section counted three defects while its
decisions listed five.

Declined: a longer lease for the pass-budget test (its drain measures 2.2s
against a 40s deadline, not the 12s the suggestion inferred from a
comment), and rescheduling the one-shot cooldown eviction (a backwards
clock jump retains one small record per affected execution, on the
automatic-observe path only; rescheduling becomes a perpetual timer per
entry under the injected clocks those tests use).

Suites green on top: runtime-broker 630 tests / 0 failures / 2 skipped
with checkstyle and spotbugs clean, managed-agent-server fix-adjacent
19/19 against the reinstalled jar.
  • Loading branch information
wenshao
wenshao committed Oct 3, 2026
commit 20d8f483f45d7337e033f1876dfc08eaab84692f
4 changes: 3 additions & 1 deletion docs/design/2026-10-02-runtime-broker-hardening.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ Status: implemented in `packages/sdk-java/runtime-broker` (PR #13214, issue #131

A code audit of the Runtime Broker found three high-risk defects. First, the session release decision and the no-active-execution check ran in separate transactions guarded only by a process-local lock: with two Broker processes sharing one database, an execution could be admitted while its session was marked `RELEASED`. Second, every lease renewal ran on one scheduled thread shared with retries, deadline fences, and polling, all with synchronous JDBC: a 1–2 s storage stall queued renewals past their lease and fenced healthy bindings. Third, the broker's HTTP face served one global Bearer token over plaintext HTTP and accepted non-loopback listen addresses.

Two cheaper medium findings are fixed in the same change. A released worker that ignored SIGTERM was never destroyed forcibly, and a JVM exit during the ready handshake could strand one. A LOST reclaim also repeated its bounded 100-row recovery passes with no budget, so a generation larger than one pass could spin.

## Decisions

**One release transaction.** `RuntimeBindingRepository.beginSessionRelease` moves the no-active-execution check into the RELEASING transition's own transaction. The transition locks the Session row `FOR UPDATE` — the same lock `admitExecution` takes — so the two paths serialize per session row across processes. A release with an active execution fails `runtime_session_busy`; an admission that loses the row to a RELEASING session fails `runtime_admission_closed`. The in-process pre-check keeps only the free `hasActiveControl` test; the database round trip it used to pay is gone because the transition's own check answers the same 409 with the same code and message.

**Renewal pool.** Renewals (binding claims and dispatch claims) run on a dedicated two-thread `ScheduledThreadPoolExecutor`; coordination work (retries, fences, polls) keeps the single-thread scheduler. A tick that parks inside its JDBC call occupies one of the pool's two threads and holds its claim's renewal monitor, so it can delay other renewals, but it no longer delays coordination work, and `close()` interrupts both pools without awaiting a parked tick. v3 result polling backs off from 100 ms, doubling to a 2 s cap that bounds how late a finished result is picked up, and its window is configurable (`v3ResultWindow`, minimum 1 s — a suffix-less config value binds as milliseconds, which the constructor now refuses). Automatic observation of an UNKNOWN execution reuses the freshest lookup for a 1 s cooldown instead of fanning every poll through to the worker; a cached lookup whose own record is no longer UNKNOWN is replayed whole, and when both records are still UNKNOWN the higher version wins, so a settled answer is never paired with a stale record. Explicit `reconcile=true` and mutation responses (`:start`, `:cancel`) never serve the cache.
**Renewal pool.** Renewals (binding claims and dispatch claims) run on a dedicated two-thread `ScheduledThreadPoolExecutor`; coordination work (retries, fences, polls) keeps the single-thread scheduler. A tick that parks inside its JDBC call occupies one of the pool's two threads and holds its claim's renewal monitor, so it can delay other renewals, but it no longer delays coordination work, and `close()` interrupts both pools without awaiting a parked tick. v3 result polling backs off from 100 ms, doubling to a 2 s cap that bounds how late a finished result is picked up, and the window it polls within is configurable (`v3ResultWindow`, default 30 m, minimum 1 s — a suffix-less config value binds as milliseconds, which the constructor now refuses); when that window lapses the dispatched execution is marked UNKNOWN rather than polled forever, so it is a polling deadline, not a result-retention period. Automatic observation of an UNKNOWN execution reuses the freshest lookup for a 1 s cooldown instead of fanning every poll through to the worker; a cached lookup whose own record is no longer UNKNOWN is replayed whole, and when both records are still UNKNOWN the higher version wins, so a settled answer is never paired with a stale record. Explicit `reconcile=true` and mutation responses (`:start`, `:cancel`) never serve the cache.

**Loopback by default.** `RuntimeBrokerHttpServer` refuses a non-loopback or unresolved bind address unless the deployment opts in (`allow-non-loopback` / `QWEN_MANAGED_AGENT_RUNTIME_BROKER_ALLOW_NON_LOOPBACK`), because the face has no per-tenant authorization over plaintext HTTP.

Expand Down
4 changes: 3 additions & 1 deletion docs/design/2026-10-02-runtime-broker-hardening.zh-CN.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@

一次代码审计发现 Runtime Broker 的三个高危缺陷。其一,会话释放决策与"无活跃 execution"检查分属两个事务,仅由进程内锁守护:当两个 Broker 进程共享一个数据库时,可能出现 execution 已准入而 session 被标记为 `RELEASED` 的矛盾态。其二,所有租约续约都跑在与重试、截止围栏和轮询共用的单条调度线程上,且均为同步 JDBC:存储抖动 1-2 秒就会让续约排队错过租约,把健康的 binding 围栏。其三,Broker 的 HTTP 面以明文 HTTP 服务单一全局 Bearer token,并接受非回环监听地址。

同一次变更还修掉两个成本较低的中等缺陷。忽略 SIGTERM 的已释放 worker 从不会被强制销毁,且 ready 握手期间的 JVM 退出会遗弃它。LOST 回收也会在没有预算的情况下反复驱动有界的 100 行恢复批次,因此超过一批容量的代际可能空转。

## 决策

**单事务释放。** `RuntimeBindingRepository.beginSessionRelease` 把"无活跃 execution"检查移入 RELEASING 转换自身的事务。该转换持有 Session 行的 `FOR UPDATE` 锁——与 `admitExecution` 获取的是同一把锁——因此两条路径在跨进程场景下按会话行互斥。带活跃 execution 的释放以 `runtime_session_busy` 失败;输给 RELEASING 会话的准入以 `runtime_admission_closed` 失败。进程内预检只保留零成本的 `hasActiveControl` 判断;原先那次数据库往返被取消,因为转换自身的检查以相同的 code 和消息回答同一个 409。

**续约线程池。** 续约(binding claim 与 dispatch claim)运行在独立的双线程 `ScheduledThreadPoolExecutor`;协调工作(重试、围栏、轮询)保留单线程调度器。一次卡在 JDBC 调用里的 tick 会占用池中两个线程之一并持有该 claim 的续约监视器,因此它可能拖慢其它续约,但不再拖慢协调工作,且 `close()` 会中断两个线程池、不等待卡住的 tick。v3 结果轮询从 100ms 起指数退避、2s 封顶(该上限约束了已完成结果被取走的最大延迟),窗口可配置(`v3ResultWindow`,下限 1 秒——无后缀的配置值会被解析为毫秒,构造函数现在拒绝这种值)。对 UNKNOWN 执行的自动观测在 1 秒冷却内复用最近一次查询结果,不再把每次轮询穿透到 worker;自身记录已不再是 UNKNOWN 的缓存查询整体回放,两侧都仍为 UNKNOWN 时取 version 更大的一方,因此绝不会把已结算的答案与过期记录拼配。显式 `reconcile=true` 与 mutation 响应(`:start`、`:cancel`)永远不走缓存。
**续约线程池。** 续约(binding claim 与 dispatch claim)运行在独立的双线程 `ScheduledThreadPoolExecutor`;协调工作(重试、围栏、轮询)保留单线程调度器。一次卡在 JDBC 调用里的 tick 会占用池中两个线程之一并持有该 claim 的续约监视器,因此它可能拖慢其它续约,但不再拖慢协调工作,且 `close()` 会中断两个线程池、不等待卡住的 tick。v3 结果轮询从 100ms 起指数退避、2s 封顶(该上限约束了已完成结果被取走的最大延迟),轮询所处的窗口可配置(`v3ResultWindow`,默认 30 分钟,下限 1 秒——无后缀的配置值会被解析为毫秒,构造函数现在拒绝这种值);窗口到期后,已派发的执行被标记为 UNKNOWN 而不是无限轮询,因此它是轮询截止期,不是结果保留期。对 UNKNOWN 执行的自动观测在 1 秒冷却内复用最近一次查询结果,不再把每次轮询穿透到 worker;自身记录已不再是 UNKNOWN 的缓存查询整体回放,两侧都仍为 UNKNOWN 时取 version 更大的一方,因此绝不会把已结算的答案与过期记录拼配。显式 `reconcile=true` 与 mutation 响应(`:start`、`:cancel`)永远不走缓存。

**默认回环。** `RuntimeBrokerHttpServer` 拒绝非回环或未解析的绑定地址,除非部署方显式开启(`allow-non-loopback` / `QWEN_MANAGED_AGENT_RUNTIME_BROKER_ALLOW_NON_LOOPBACK`),因为该面在明文 HTTP 上没有租户级授权。

Expand Down
10 changes: 7 additions & 3 deletions packages/sdk-java/managed-agent-server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -390,15 +390,19 @@ export QWEN_MANAGED_AGENT_RUNTIME_WORKER_ENTRY='/absolute/path/to/dist/cli.js'
export QWEN_MANAGED_AGENT_CLI_ENTRY='/absolute/path/to/dist/cli.js'
```

Two optional knobs change how the Broker listens and how long it keeps a
finished v3 result readable:
Two optional knobs change how the Broker listens and how long it waits for a
dispatched v3 execution's result:

```bash
# Default false: the Broker refuses to bind a non-loopback address. Set it
# only when the Harness reaches the Broker over a non-loopback interface and
# that interface is already restricted to trusted peers.
export QWEN_MANAGED_AGENT_RUNTIME_BROKER_ALLOW_NON_LOOPBACK='false'
# Default 30m, minimum 1s: how long a finished v3 result stays retrievable.
# Default 30m, minimum 1s: how long the Broker keeps polling the worker for
# a dispatched v3 execution's result. When the window lapses the execution
# is marked UNKNOWN instead of polling on, so a value shorter than your
# longest tool call degrades that call to UNKNOWN. A suffix-less number
# binds as milliseconds, which startup refuses.
export QWEN_MANAGED_AGENT_RUNTIME_BROKER_V3_RESULT_WINDOW='30m'
```

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,16 +215,22 @@ void refusesANonLoopbackListenAddressWithoutTheOptIn() throws Exception {
}

@Test
void refusesANonPositiveV3ResultWindowBeforeAnythingIsBuilt()
throws Exception {
ManagedAgentProperties properties = properties();
properties.getRuntimeBroker().setV3ResultWindow(
java.time.Duration.ZERO);
void refusesAV3ResultWindowBelowThePollFloor() throws Exception {
// An absent value binds as null and a suffix-less one as
// milliseconds; every shape below the floor must be refused here
// rather than degrade each v3 execution later.
for (java.time.Duration window : new java.time.Duration[] {
null, java.time.Duration.ZERO, java.time.Duration.ofMillis(-1),
java.time.Duration.ofMillis(999)}) {
ManagedAgentProperties properties = properties();
properties.getRuntimeBroker().setV3ResultWindow(window);

assertThatThrownBy(() -> broker(mock(ManagedAgentStore.class),
properties))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("v3 result window");
assertThatThrownBy(() -> broker(mock(ManagedAgentStore.class),
properties))
.as("window %s", window)
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("v3 result window");
}
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ RuntimeSessionRecord completeSessionRelease(RuntimeSessionRepository sessions,
* execution check, under the same Session row lock admission takes, so
* two Broker processes cannot interleave an admission into the gap.
* Returns null when the row no longer matches {@code expected}, the
* current record when it is already RELEASING or RELEASED, and throws
* {@code runtime_session_busy} when an execution is still active.
* current record when it is already RELEASING or RELEASED, throws
* {@code runtime_session_busy} when an execution is still active, and
* throws {@code runtime_session_not_ready} when the Session is in any
* other state.
*/
RuntimeSessionRecord beginSessionRelease(RuntimeSessionRepository sessions,
ToolExecutionRepository executions, RuntimeSessionRecord expected);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,8 +388,8 @@ private CompletionStage<ExecutionReconciliation> observe(String harnessSessionId
ExecutionReconciliation.Outcome.IN_FLIGHT, null));
}
// An explicit reconcile asks the Runtime every time, and a mutation's
// own response must describe the post-mutation truth; only pure
// polling reuses a cooled answer.
// own response never serves the cooldown cache; only pure polling
// reuses a cooled answer.
CompletionStage<ExecutionReconciliation> observation =
reconcile || !coolable
Comment thread
This conversation was marked as resolved.
? service.reconcileExecution(harnessSessionId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public final class RuntimeBrokerService implements AutoCloseable {
// inside this window; sequential observations share the one lookup.
static final Duration UNKNOWN_LOOKUP_COOLDOWN =
Duration.ofSeconds(1);
/** The smallest v3 result window that can complete a second poll. */
/** Floor for {@code v3ResultWindow}: rejects a duration bound as milliseconds. */
public static final Duration MIN_V3_RESULT_WINDOW = Duration.ofSeconds(1);
private static final Duration DEFAULT_V3_RESULT_WINDOW =
Duration.ofMinutes(30);
Expand Down Expand Up @@ -188,10 +188,10 @@ private RuntimeBrokerService(HarnessSessionResolver sessionResolver,
this.v3ResultWindow = requireDuration(v3ResultWindow,
"v3ResultWindow");
if (this.v3ResultWindow.compareTo(MIN_V3_RESULT_WINDOW) < 0) {
// Below the first poll tick the window cannot complete even one
// retry; configuration bindings parse a suffix-less number as
// milliseconds, so fail fast rather than silently degrading
// every v3 execution to UNKNOWN.
// A suffix-less duration config binds as milliseconds, so a
// window meant as "30" minutes arrives as 30ms and would
// silently degrade every v3 execution to UNKNOWN. Refuse values
// below a floor no intended configuration lands under.
throw new IllegalArgumentException(
"v3ResultWindow must be at least "
+ MIN_V3_RESULT_WINDOW);
Expand Down Expand Up @@ -2975,6 +2975,14 @@ private CompletionStage<Map<String, Object>> awaitV3Result(SessionContext contex
return result;
}

/**
* The v3 result-poll backoff: 100ms doubling, capped at 2s so a finished
* result is picked up at most one cap late.
*/
static long v3PollDelayMillis(int attempt) {
return Math.min(2_000, 100L << Math.min(attempt, 6));
}

private void pollV3Result(SessionContext context, ToolExecutionRecord original,
Instant deadline, CompletableFuture<Map<String, Object>> answer,
int attempt) {
Expand Down Expand Up @@ -3018,10 +3026,8 @@ private void pollV3Result(SessionContext context, ToolExecutionRecord original,
}
// Each round costs two repository reads and one worker
// call; back off instead of pinning them at 10/s for
// the whole window. The 2s cap bounds how late a
// finished result is picked up.
long delay = Math.min(2_000,
100L << Math.min(attempt, 6));
// the whole window.
long delay = v3PollDelayMillis(attempt);
try {
scheduler.schedule(() -> pollV3Result(context,
original, deadline, answer, attempt + 1),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,15 @@ void ignoreTermSurvivesSigterm() throws Exception {
* The mirror arm: without {@code --ignore-term} the fake worker's
* default SIGTERM handler must let destroy() stop it — otherwise the
* escalation tests prove nothing. The ready line must be read first:
* it is written only after the handler is installed, so destroying
* earlier would kill the worker through SIGTERM's default disposition
* and prove nothing about the handler.
* it is written from the listen callback, so it reaches this process
* only after the top-level handler installation has run, and
* destroying earlier would kill the worker through SIGTERM's default
* disposition and prove nothing about the handler. POSIX-only, like the
* arm it mirrors: on Windows destroy() terminates outright.
*/
@Test
@org.junit.jupiter.api.condition.DisabledOnOs(
org.junit.jupiter.api.condition.OS.WINDOWS)
void defaultWorkerExitsOnSigterm() throws Exception {

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.

Both defects fixed, and the first one was real: the test destroyed the worker without reading its ready line, and the fake installs its SIGTERM handler in top-level script flow while the ready line is written from the listen callback — so destroy() could land before any handler existed and the worker died through SIGTERM's default disposition, proving nothing. It now waits for the ready line, and carries @DisabledOnOs(OS.WINDOWS) like the sibling it mirrors.

Mutation-proven: with the fake's default arm wedged (process.on('SIGTERM', () => {}) unconditionally), the pre-fix body passed 3 runs out of 3 (~0.28 s each) while the fixed body fails with a default worker must exit on SIGTERM.

LocalProcessRuntimeProvisionerTest.requireNode();
Process worker = start(JSON.writeValueAsBytes(fixtures().get("boot")));
Expand Down
Loading
Loading