Skip to content

Commit 4e91dc5

Browse files
author
wenshao
committed
test(runtime-broker): close self-audit round 4 on the 13183 branch
Round 4 found no Critical: an adversarial pass over the tests and an undirected pass over the whole diff returned eleven Suggestions between them, all addressed here. Two of the newest tests described a mechanism that is not what happens. The release-grace harness claimed its exit kills the daemon escalation thread, but System.exit runs the shutdown hooks first, so during the hook's own grace wait that thread is alive and destroys the worker at about the same moment; what the hook's snapshot actually provides is keeping the JVM alive until the reclaim completes and issuing the destroy itself. The test-side comment claimed its poll loop had to outlast that grace window, when the loop only starts after the harness has exited and halted, so what it waits out is pid reaping. The guard assertion that only makes sense where destroy() is a signal became its own test guarded by an assumption, so a platform that cannot reach the guard reports skipped instead of passing without exercising it. The shared race fixture moved into a helper, and the platform-independent half - no child process outlives the provisioner - still runs everywhere. Both forked tests cleaned up by killing a bare pid, which a recycled pid would turn into killing an unrelated process. They now require that pid to still name a node worker, and the liveness poll applies the same identity check so a recycled pid cannot read as a false failure. The clock the cooldown tests advance is volatile now, since the service reads it from the HTTP handler and coordination threads while the test thread writes it. The provisioner's close() also clears the tracking set, whose entry a discarded escalation task would otherwise never remove. The synchronized block in stop() cannot be pinned from outside the lock, so its comment now says the guarantee is structural, the way the spawn-and-register javadoc already does. The tracking set's field comment names both of its membership windows instead of only the pre-issuance one. The backoff sweep no longer ties its range to the default window, which has a floor but no ceiling. Documentation: the design docs still described a 200-round stress when the code runs 600, and understated which outcomes the release primitive's contract test covers; the deployment README's guidance for opting into a non-loopback bind was weaker than the server's own refusal message, which asks for a TLS-terminating layer that authorizes callers rather than a restricted network, on a face that serves one global bearer token in plaintext; and the release transaction gains a comment recording that its statement order is load-bearing under InnoDB repeatable read, where a read moved ahead of the locking read would silently reopen the race without H2 showing it. Suites green: runtime-broker 632 tests / 0 failures / 2 skipped with checkstyle and spotbugs clean, managed-agent-server fix-adjacent 19/19 against the reinstalled jar.
1 parent 56c2096 commit 4e91dc5

8 files changed

Lines changed: 111 additions & 59 deletions

File tree

‎docs/design/2026-10-02-runtime-broker-hardening.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ Status: implemented in `packages/sdk-java/runtime-broker` (PR #13214, issue #131
88

99
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.
1010

11-
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 ran a single bounded 100-row recovery pass per phase, so a generation larger than one pass stayed LOST and answered `runtime_broker_runtime_lost` on every later attempt.
11+
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 ran a single bounded 100-row recovery pass per phase, so a generation larger than one pass needed many consecutive reclaim attempts, each answering `runtime_broker_runtime_lost`, before the binding could be reused.
1212

1313
## Decisions
1414

@@ -28,4 +28,4 @@ Credential key rotation (#13202), terminal-row retention (#13203), and InMemory/
2828

2929
## Validation
3030

31-
`Issue13183RegressionTest` and `Issue13183AdversarialTest` encode the issue's scenarios with the fixed expectations: the race interleaving, the stalled renewal, the observation cooldown, the loopback refusal, the wedged-worker escalation, and the whole-generation drain, plus a 200-round cross-process stress and a forked-JVM exit-hook proof. `RuntimeRecoveryContract.verifyBeginSessionRelease` covers the new repository primitive's four outcomes on both repository backends. `mvn clean test` in `packages/sdk-java/runtime-broker` and the managed-agent-server fix-adjacent suites pass; `mvn checkstyle:check` is clean.
31+
`Issue13183RegressionTest` and `Issue13183AdversarialTest` encode the issue's scenarios with the fixed expectations: the race interleaving, the stalled renewal, the observation cooldown, the loopback refusal, the wedged-worker escalation, and the whole-generation drain, plus a 600-round cross-process stress (300 of them a tight race, 300 with the admission committed first) and a forked-JVM exit-hook proof. `RuntimeRecoveryContract.verifyBeginSessionRelease` covers the new repository primitive on both backends: the READY and ACQUIRING transitions, null on a stale snapshot, the passthrough for an already RELEASING or RELEASED session, `runtime_session_busy`, and `runtime_session_not_ready`. `mvn clean test` in `packages/sdk-java/runtime-broker` and the managed-agent-server fix-adjacent suites pass; `mvn checkstyle:check` is clean.

‎docs/design/2026-10-02-runtime-broker-hardening.zh-CN.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

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

11-
同一次变更还修掉两个成本较低的中等缺陷。忽略 SIGTERM 的已释放 worker 从不会被强制销毁，且 ready 握手期间的 JVM 退出会遗弃它。LOST 回收每个阶段也只跑一趟有界的 100 行恢复批次，因此超过一趟容量的代际会一直停在 LOST，之后每次尝试都回答 `runtime_broker_runtime_lost`。
11+
同一次变更还修掉两个成本较低的中等缺陷。忽略 SIGTERM 的已释放 worker 从不会被强制销毁，且 ready 握手期间的 JVM 退出会遗弃它。LOST 回收每个阶段也只跑一趟有界的 100 行恢复批次，因此超过一趟容量的代际需要连续多次 reclaim 才能排空，而每一次都回答 `runtime_broker_runtime_lost`，binding 在此期间无法复用。
1212

1313
## 决策
1414

@@ -28,4 +28,4 @@
2828

2929
## 验证
3030

31-
`Issue13183RegressionTest` 与 `Issue13183AdversarialTest` 以修复后的期望编码了 issue 的场景：竞态交错、卡住的续约、观测冷却、回环拒绝、楔住 worker 的升级强杀、整代际排空，另有 200 轮跨进程对撞与 forked-JVM 退出钩子实证。`RuntimeRecoveryContract.verifyBeginSessionRelease` 在两种仓库后端上覆盖新原语的四种结果。`packages/sdk-java/runtime-broker` 的 `mvn clean test` 与 managed-agent-server 修复邻近套件通过；`mvn checkstyle:check` 干净。
31+
`Issue13183RegressionTest` 与 `Issue13183AdversarialTest` 以修复后的期望编码了 issue 的场景：竞态交错、卡住的续约、观测冷却、回环拒绝、楔住 worker 的升级强杀、整代际排空，另有 600 轮跨进程对撞（其中 300 轮为紧竞态、300 轮由准入先提交）与 forked-JVM 退出钩子实证。`RuntimeRecoveryContract.verifyBeginSessionRelease` 在两种仓库后端上覆盖新原语：READY 与 ACQUIRING 两种转换、快照过期时返回 null、已 RELEASING 或 RELEASED 时原样返回、`runtime_session_busy`，以及 `runtime_session_not_ready`。`packages/sdk-java/runtime-broker` 的 `mvn clean test` 与 managed-agent-server 修复邻近套件通过；`mvn checkstyle:check` 干净。

‎packages/sdk-java/managed-agent-server/README.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -394,9 +394,11 @@ Two optional knobs change how the Broker listens and how long it waits for a
394394
dispatched v3 execution's result:
395395

396396
```bash
397-
# Default false: the Broker refuses to bind a non-loopback address. Set it
398-
# only when the Harness reaches the Broker over a non-loopback interface and
399-
# that interface is already restricted to trusted peers.
397+
# Default false: the Broker refuses to bind a non-loopback address. This face
398+
# is plaintext HTTP with one global bearer token and no per-tenant
399+
# authorization, so set it only behind a layer that terminates TLS and
400+
# authorizes callers — restricting the network alone still puts that token on
401+
# the wire, and whoever reads it owns every execution the Broker admits.
400402
export QWEN_MANAGED_AGENT_RUNTIME_BROKER_ALLOW_NON_LOOPBACK='false'
401403
# Default 30m, minimum 1s: how long the Broker keeps polling the worker for
402404
# a dispatched v3 execution's result. When the window lapses the execution

‎packages/sdk-java/runtime-broker/src/main/java/com/alibaba/qwen/code/runtimebroker/JdbcRuntimeBindingRepository.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,12 @@ public RuntimeSessionRecord beginSessionRelease(RuntimeSessionRepository session
8181
throw new IllegalArgumentException("Release requires the same DataSource");
8282
}
8383
return JdbcRepositorySupport.transaction(dataSource, connection -> {
84+
// Statement order is load-bearing under InnoDB REPEATABLE READ:
85+
// this locking read must stay first, and the plain execution read
86+
// below must stay the transaction's first consistent read, so its
87+
// read view is built after the row lock is held and therefore
88+
// sees an admission that committed before it. A read moved ahead
89+
// of the lock silently reopens the race, and H2 would not show it.
8490
RuntimeSessionRecord current = JdbcRuntimeSessionRepository.selectSession(
8591
connection, expected.getSession().getScope(),
8692
expected.getRuntimeSessionId(), true);

‎packages/sdk-java/runtime-broker/src/main/java/com/alibaba/qwen/code/runtimebroker/LocalProcessRuntimeProvisioner.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,15 @@ public final class LocalProcessRuntimeProvisioner
5858
private final ConcurrentMap<List<Object>, OwnedProcess> owned =
5959
new ConcurrentHashMap<>();
6060
private final Set<List<Object>> issued = ConcurrentHashMap.newKeySet();
61-
// Spawned but not yet issued: the exit hook and close() must see a
62-
// worker from the moment the process exists, not from when its lease
63-
// lands in `owned` — the ready handshake can take READY_TIMEOUT.
61+
// Workers the exit hook and close() must see that `owned` does not
62+
// cover: one still in its ready handshake (spawned, not yet issued —
63+
// that can take READY_TIMEOUT), and one already released but inside its
64+
// grace window (no longer owned, its escalation running on a daemon
65+
// thread that dies with the JVM).
6466
private final Set<OwnedProcess> starting = ConcurrentHashMap.newKeySet();
65-
// Serialize spawn-and-register against the exit snapshot, so a worker
66-
// can never exist unseen by terminateAll.
67+
// Serialize spawn-and-register, and the release-side move out of
68+
// `owned`, against the exit snapshot, so a worker can never exist unseen
69+
// by terminateAll.
6770
private final Object lifecycle = new Object();
6871
private volatile boolean terminated;
6972
private final Thread exitHook;
@@ -353,6 +356,9 @@ void stop(RuntimeLease lease) {
353356
// Move the worker from owned to starting under the same lock
354357
// terminateAll() snapshots with, so an exit can never find it in
355358
// neither set. Once terminated, that snapshot already covered it.
359+
// Like spawn-and-register in start(), this is structural: the
360+
// window it closes is inside the lock, so no test can reach the
361+
// interleaving from outside it.
356362
process = owned.remove(ownershipKey(lease));
357363
if (process != null && !terminated) {
358364
starting.add(process);
@@ -427,6 +433,9 @@ public void close() {
427433
}
428434
owned.clear();
429435
executor.shutdownNow();
436+
// A queued escalation does not run after that shutdown, and its
437+
// finally block was the only thing removing the entry it tracked.
438+
starting.clear();
430439
if (exitHook != null) {
431440
try {
432441
Runtime.getRuntime().removeShutdownHook(exitHook);

‎packages/sdk-java/runtime-broker/src/test/java/com/alibaba/qwen/code/runtimebroker/Issue13183AdversarialTest.java‎

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -172,10 +172,12 @@ void concurrentCrossProcessAdmitAndReleaseNeverContradict()
172172
"admission and release contradicted each other");
173173
assertEquals(0, unexpected.get(),
174174
() -> "unexpected failure: " + surprises);
175-
// Both arms must have run, or the exact error-code checks above only
176-
// covered one of the two outcomes.
175+
// Both outcomes must have occurred, or the exact error-code checks
176+
// above only covered one of them. The committed-admission half is
177+
// guaranteed by the even rounds' latch, so what this really guards
178+
// is that the raced half still produces committed releases.
177179
assertTrue(admitWins.get() > 0 && releaseWins.get() > 0,
178-
"one arm of the stress never ran: admission won "
180+
"one outcome never occurred: admission won "
179181
+ admitWins.get() + " rounds, release won "
180182
+ releaseWins.get() + " of " + rounds);
181183
}
@@ -507,11 +509,14 @@ void exitHookReclaimsAWorkerStillInStartup() throws Exception {
507509
harness.destroyForcibly();
508510
}
509511
// A failed assertion above must not leave the harness's worker
510-
// spinning on the runner: nothing else ever stops it.
511-
if (worker > 0) {
512-
ProcessHandle.of(worker)
513-
.ifPresent(ProcessHandle::destroyForcibly);
514-
}
512+
// spinning on the runner: nothing else ever stops it. Only kill
513+
// that pid while it still names a node worker, so a recycled pid
514+
// cannot take out an unrelated process.
515+
ProcessHandle.of(worker)
516+
.filter(handle -> handle.info().command()
517+
.map(command -> command.contains("node"))
518+
.orElse(false))
519+
.ifPresent(ProcessHandle::destroyForcibly);
515520
Files.deleteIfExists(harnessLog);
516521
Files.deleteIfExists(exitSignal);
517522
}
@@ -550,14 +555,18 @@ void exitHookReclaimsAWorkerInsideItsReleaseGraceWindow() throws Exception {
550555
assertTrue(worker > 0,
551556
"the harness never reported its worker; harness log: "
552557
+ logTail(harnessLog));
553-
// The hook shares one 5s grace window across the processes it
554-
// reclaimed; the loop only has to outlast that.
558+
// The harness has exited, and with it the hook's grace window,
559+
// so this loop only waits out pid reaping. It trusts the pid
560+
// only while that process still names a node worker: the pid
561+
// could be recycled while the loop waits.
555562
long pid = worker;
556563
boolean alive = true;
557564
long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(20);
558565
while (alive && System.nanoTime() < deadline) {
559-
alive = ProcessHandle.of(pid)
560-
.map(ProcessHandle::isAlive).orElse(false);
566+
alive = ProcessHandle.of(pid).map(handle -> handle.isAlive()
567+
&& handle.info().command()
568+
.map(command -> command.contains("node"))
569+
.orElse(true)).orElse(false);
561570
Thread.sleep(100);
562571
}
563572
assertTrue(!alive,
@@ -568,11 +577,13 @@ void exitHookReclaimsAWorkerInsideItsReleaseGraceWindow() throws Exception {
568577
if (harness.isAlive()) {
569578
harness.destroyForcibly();
570579
}
571-
// A failed assertion must not leave the wedged worker behind.
572-
if (worker > 0) {
573-
ProcessHandle.of(worker)
574-
.ifPresent(ProcessHandle::destroyForcibly);
575-
}
580+
// A failed assertion must not leave the wedged worker behind -
581+
// and must not kill an unrelated process holding a recycled pid.
582+
ProcessHandle.of(worker)
583+
.filter(handle -> handle.info().command()
584+
.map(command -> command.contains("node"))
585+
.orElse(false))
586+
.ifPresent(ProcessHandle::destroyForcibly);
576587
Files.deleteIfExists(harnessLog);
577588
}
578589
}
@@ -611,9 +622,11 @@ public static void main(String[] args) throws Exception {
611622
System.out.flush();
612623
provisioner.release(ManagedContextProtocolTest.request(), lease)
613624
.toCompletableFuture().join();
614-
// Exit inside the 5s grace window: the daemon escalation thread
615-
// dies here, so the hook is the only thing left that can reclaim
616-
// a worker ignoring SIGTERM.
625+
// Exit inside the 5s grace window. The hook's snapshot is what
626+
// keeps this JVM alive until the reclaim completes, and what
627+
// issues the forcible destroy: with the worker untracked the JVM
628+
// halts at once, the daemon escalation dies mid-wait, and a
629+
// worker ignoring SIGTERM keeps running.
617630
System.exit(0);
618631
}
619632
}

0 commit comments

Comments
 (0)