Skip to content

Commit 56c2096

Browse files
author
wenshao
committed
fix(runtime-broker): repair the Windows leg and close the release grace window
Round 2 broke windows-latest / Java 21 (612 tests, 2 failures, with ubuntu and macOS green). Both are fixed here without waiving the platform: - The exit-hook test had moved the decision of when the forked harness exits from a sleep inside it to harness.destroy() in the test. destroy() is a SIGTERM only on POSIX; on Windows it is TerminateProcess, which runs no shutdown hooks, so the worker legitimately outlived the harness. The harness now polls a sentinel file and exits itself with System.exit(0) once the test has observed the worker: the window stays test-controlled and the hooks run on every platform. - The provision-race test widens its race window with a --ignore-term worker, which cannot wedge anything on Windows, so close() finishes in milliseconds and the racing loop can miss the terminated guard. The guard assertion now runs only where destroy() is a signal; the platform-independent half - no lingering child processes - still runs everywhere. That code is unchanged from the previous Windows-green commit, so this was a timing flake rather than a new defect. Also close a real orphan window in the release path. stop() removed the worker from owned and then escalated to destroyForcibly on a daemon thread, so for the whole five-second grace the process was tracked by neither set. close() was covered, because shutting the executor down interrupts the escalation and its handler destroys forcibly, but a bare JVM exit was not: the daemon dies with the JVM and the hook's snapshot could not see the worker. A worker that ignores SIGTERM, released and then hit by a Broker SIGTERM inside five seconds, survived. The worker now moves from owned to starting under the same lifecycle lock terminateAll() snapshots with, which also removes the microsecond gap between those two set operations, and leaves it when the escalation finishes. A forked test releases a wedged worker and exits the harness JVM inside the grace window; without the tracking it fails with that worker still alive. Smaller corrections from the same audit round: the cross-process stress rises to 600 rounds so its raced arm keeps the 200 rounds the odd/even split had halved, and its comments now say what the win counters prove - both arms ran - instead of claiming a contradictory interleaving was reached; the v3 backoff test sweeps attempts 0-1000, pinning the inner shift clamp (an unclamped shift wraps at attempt 57 and schedules zero and negative delays, returning the polling to the rate the backoff exists to remove); and the design doc's description of the pre-fix LOST reclaim is corrected in both languages, since at the merge base it ran a single bounded pass per phase and stranded larger generations rather than looping without a budget. A test name and two assertion messages that still carried rationales round 2 retracted in production are corrected, and the exit-hook test's worker cleanup moves into its finally block so a failed assertion cannot leave a spinning node process on the runner. Declined: a production seam inside the release transaction to make the row-lock pin deterministic. The raced arm already detects the missing FOR UPDATE in three runs out of three, with three to five contradictions each, and the transition's in-transaction re-check is pinned deterministically by the delegating-repository release test. Suites green: runtime-broker 631 tests / 0 failures / 2 skipped with checkstyle and spotbugs clean, managed-agent-server fix-adjacent 19/19 against the reinstalled jar.
1 parent 20d8f48 commit 56c2096

6 files changed

Lines changed: 225 additions & 61 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
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 repeated its bounded 100-row recovery passes with no budget, so a generation larger than one pass could spin.
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.
1212

1313
## Decisions
1414

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

Lines changed: 1 addition & 1 deletion
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 行恢复批次，因此超过一批容量的代际可能空转。
11+
同一次变更还修掉两个成本较低的中等缺陷。忽略 SIGTERM 的已释放 worker 从不会被强制销毁，且 ready 握手期间的 JVM 退出会遗弃它。LOST 回收每个阶段也只跑一趟有界的 100 行恢复批次，因此超过一趟容量的代际会一直停在 LOST，之后每次尝试都回答 `runtime_broker_runtime_lost`。
1212

1313
## 决策
1414

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,14 +348,32 @@ public boolean isUsable(RuntimeLease lease) {
348348
}
349349

350350
void stop(RuntimeLease lease) {
351-
OwnedProcess process = owned.remove(ownershipKey(lease));
351+
OwnedProcess process;
352+
synchronized (lifecycle) {
353+
// Move the worker from owned to starting under the same lock
354+
// terminateAll() snapshots with, so an exit can never find it in
355+
// neither set. Once terminated, that snapshot already covered it.
356+
process = owned.remove(ownershipKey(lease));
357+
if (process != null && !terminated) {
358+
starting.add(process);
359+
}
360+
}
352361
if (process != null) {
353362
process.process.destroy();
354363
// A worker that ignores SIGTERM must not outlive its release;
355364
// escalate after the grace window without blocking the caller.
365+
// That escalation runs on a daemon thread which dies with the
366+
// JVM, so the worker stays in `starting` until it finishes.
356367
try {
357-
executor.execute(() -> forceAfterGrace(process.process));
368+
executor.execute(() -> {
369+
try {
370+
forceAfterGrace(process.process);
371+
} finally {
372+
starting.remove(process);
373+
}
374+
});
358375
} catch (RejectedExecutionException closing) {
376+
starting.remove(process);
359377
process.process.destroyForcibly();
360378
}
361379
}

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

Lines changed: 161 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,21 @@
4040
class Issue13183AdversarialTest {
4141

4242
/**
43-
* N rounds; in each round one thread admits an execution while another
44-
* runs beginSessionRelease from a second repository stack. Half the
45-
* rounds release both from one latch as a tight race; the other half
46-
* hold the release until the admission has committed, so both
47-
* directions are covered rather than whichever one the scheduler
48-
* favours. The two outcomes must never contradict: a
49-
* committed admission forces runtime_session_busy; a committed
50-
* RELEASING transition forces runtime_admission_closed. The
43+
* Rounds of one thread admitting an execution while another runs
44+
* beginSessionRelease from a second repository stack. Odd rounds release
45+
* both from one latch as a tight race: the release wins almost all of
46+
* them, because admission takes the placement-domain lock and does two
47+
* reads first, and a missing Session row lock shows up here as the
5148
* contradictory end state - admission committed AND session RELEASING -
52-
* must never occur, and neither call may hit a lock failure. Both
53-
* interleavings must actually occur, so a one-sided schedule cannot pass
54-
* the exact-error-code checks by never exercising one direction.
49+
* in a few percent of rounds. Even rounds hold the release until the
50+
* admission has committed, the direction a tight race does not reach, so
51+
* the transition's own re-check must answer runtime_session_busy. The
52+
* two outcomes must never contradict: a committed admission forces
53+
* runtime_session_busy; a committed RELEASING transition forces
54+
* runtime_admission_closed. Neither call may hit a lock failure. The win
55+
* counters assert that both arms ran, so the exact-error-code checks
56+
* cover both outcomes; they do not claim a contradictory interleaving
57+
* was reached, which is what the raced arm is for.
5558
*/
5659
@Test
5760
void concurrentCrossProcessAdmitAndReleaseNeverContradict()
@@ -68,7 +71,9 @@ void concurrentCrossProcessAdmitAndReleaseNeverContradict()
6871
JdbcToolExecutionRepository executionsB = new JdbcToolExecutionRepository(
6972
dataSource);
7073

71-
int rounds = 200;
74+
// 300 raced rounds: a missing Session row lock contradicts in a few
75+
// percent of them, so the count sets the odds of catching it.
76+
int rounds = 600;
7277
ExecutorService pool = Executors.newFixedThreadPool(2);
7378
AtomicInteger contradictions = new AtomicInteger();
7479
AtomicInteger unexpected = new AtomicInteger();
@@ -89,11 +94,10 @@ void concurrentCrossProcessAdmitAndReleaseNeverContradict()
8994
fixture.binding.getRequest().getScope(),
9095
fixture.session.getRuntimeSessionId());
9196
CountDownLatch gate = new CountDownLatch(1);
92-
// Half the rounds are a tight race; the other half hold the
93-
// release until the admission has committed. The release
94-
// reaches the session row first in a tight race often enough
95-
// that racing alone would leave the committed-admission
96-
// direction unexercised.
97+
// Odd rounds race; even rounds hold the release until the
98+
// admission has committed. The release wins a tight race
99+
// almost every time, so racing alone would leave the
100+
// committed-admission direction unexercised.
97101
boolean raced = (round & 1) == 1;
98102
CountDownLatch admitDone = new CountDownLatch(raced ? 0 : 1);
99103
AtomicReference<Throwable> admitOutcome =
@@ -168,11 +172,10 @@ void concurrentCrossProcessAdmitAndReleaseNeverContradict()
168172
"admission and release contradicted each other");
169173
assertEquals(0, unexpected.get(),
170174
() -> "unexpected failure: " + surprises);
171-
// Both interleavings must actually have occurred, or the exact
172-
// error-code checks above only covered the direction the scheduler
173-
// happened to favour.
175+
// Both arms must have run, or the exact error-code checks above only
176+
// covered one of the two outcomes.
174177
assertTrue(admitWins.get() > 0 && releaseWins.get() > 0,
175-
"the stress never exercised both interleavings: admission won "
178+
"one arm of the stress never ran: admission won "
176179
+ admitWins.get() + " rounds, release won "
177180
+ releaseWins.get() + " of " + rounds);
178181
}
@@ -440,28 +443,31 @@ public CompletionStage<RuntimeLease> provision(
440443
/**
441444
* The exit hook must reclaim even a worker that is still in its ready
442445
* handshake when the JVM exits: it is registered in {@code starting}
443-
* from spawn. This forks a broker JVM that is signalled mid-provision
444-
* and then checks that the worker did not outlive it. Before the fix,
445-
* the worker survived: it only entered {@code owned} after the
446-
* handshake, which the exit never reached. The forked worker obeys
447-
* SIGTERM, so this covers the hook's {@code destroy()}; the forcible
448-
* fallback on the exit path is exercised by the close() and release()
449-
* escalation tests instead.
446+
* from spawn. This forks a broker JVM that exits on its own mid-provision,
447+
* at the test's signal, and then checks that the worker did not outlive
448+
* it. Before the fix, the worker survived: it only entered {@code owned}
449+
* after the handshake, which the exit never reached. The forked worker
450+
* obeys SIGTERM, so this covers the hook's {@code destroy()}; the
451+
* forcible fallback on the exit path is exercised by the close() and
452+
* release() escalation tests instead.
450453
*/
451454
@Test
452455
void exitHookReclaimsAWorkerStillInStartup() throws Exception {
453456
LocalProcessRuntimeProvisionerTest.requireNode();
454457
String classpath = System.getProperty("java.class.path");
455458
Path harnessLog = Files.createTempFile("exit-harness", ".log");
459+
Path exitSignal = Files.createTempFile("exit-signal", ".flag");
460+
Files.delete(exitSignal);
456461
Process harness = new ProcessBuilder(
457462
Path.of(System.getProperty("java.home"), "bin", "java")
458463
.toString(),
459-
"-cp", classpath, ExitHarnessMain.class.getName())
464+
"-cp", classpath, ExitHarnessMain.class.getName(),
465+
exitSignal.toString())
460466
.redirectErrorStream(true)
461467
.redirectOutput(harnessLog.toFile()).start();
462468
long harnessPid = harness.pid();
469+
long worker = -1;
463470
try {
464-
long worker = -1;
465471
long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(15);
466472
while (System.nanoTime() < deadline) {
467473
java.util.Optional<ProcessHandle> node = ProcessHandle
@@ -480,22 +486,18 @@ void exitHookReclaimsAWorkerStillInStartup() throws Exception {
480486
assertTrue(worker > 0,
481487
"the harness never spawned a worker; harness log: "
482488
+ logTail(harnessLog));
483-
// The test ends the harness JVM rather than waiting for a timer
484-
// inside it: SIGTERM runs the same shutdown hooks a natural exit
485-
// would, and the observation window above no longer has to fit
486-
// inside the harness's own sleep.
487-
harness.destroy();
489+
// The test, not a timer inside the harness, decides when that
490+
// JVM exits, so the observation window above is not a race. The
491+
// harness exits itself rather than being killed from here:
492+
// System.exit runs the shutdown hooks on every platform, while
493+
// destroy() would not run them on Windows.
494+
Files.createFile(exitSignal);
488495
assertTrue(harness.waitFor(20, TimeUnit.SECONDS),
489496
"the harness never exited; harness log: "
490497
+ logTail(harnessLog));
491498
Thread.sleep(1000);
492499
boolean leaked = ProcessHandle.of(worker)
493500
.map(ProcessHandle::isAlive).orElse(false);
494-
if (leaked) {
495-
// Do not leave the proof behind on the machine.
496-
ProcessHandle.of(worker).ifPresent(
497-
process -> process.destroyForcibly());
498-
}
499501
assertTrue(!leaked,
500502
"worker " + worker
501503
+ " survived the broker JVM exit mid-handshake;"
@@ -504,10 +506,118 @@ void exitHookReclaimsAWorkerStillInStartup() throws Exception {
504506
if (harness.isAlive()) {
505507
harness.destroyForcibly();
506508
}
509+
// 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+
}
515+
Files.deleteIfExists(harnessLog);
516+
Files.deleteIfExists(exitSignal);
517+
}
518+
}
519+
520+
/**
521+
* The release grace window is tracked the same way the ready handshake
522+
* is: a worker that ignores SIGTERM is escalated by a daemon thread, and
523+
* that thread dies with the JVM, so an exit inside the window can only
524+
* be saved by the hook finding the worker. POSIX-only, like the other
525+
* escalation tests: on Windows destroy() terminates outright, so there
526+
* is no wedged worker to strand.
527+
*/
528+
@Test
529+
@org.junit.jupiter.api.condition.DisabledOnOs(
530+
org.junit.jupiter.api.condition.OS.WINDOWS)
531+
void exitHookReclaimsAWorkerInsideItsReleaseGraceWindow() throws Exception {
532+
LocalProcessRuntimeProvisionerTest.requireNode();
533+
String classpath = System.getProperty("java.class.path");
534+
Path script = Path.of("src/test/resources/fake-attestation-worker.mjs")
535+
.toAbsolutePath();
536+
Path harnessLog = Files.createTempFile("release-harness", ".log");
537+
Process harness = new ProcessBuilder(
538+
Path.of(System.getProperty("java.home"), "bin", "java")
539+
.toString(),
540+
"-cp", classpath, ReleaseGraceHarnessMain.class.getName(),
541+
script.toString())
542+
.redirectErrorStream(true)
543+
.redirectOutput(harnessLog.toFile()).start();
544+
long worker = -1;
545+
try {
546+
assertTrue(harness.waitFor(60, TimeUnit.SECONDS),
547+
"the harness never exited; harness log: "
548+
+ logTail(harnessLog));
549+
worker = reportedWorker(harnessLog);
550+
assertTrue(worker > 0,
551+
"the harness never reported its worker; harness log: "
552+
+ logTail(harnessLog));
553+
// The hook shares one 5s grace window across the processes it
554+
// reclaimed; the loop only has to outlast that.
555+
long pid = worker;
556+
boolean alive = true;
557+
long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(20);
558+
while (alive && System.nanoTime() < deadline) {
559+
alive = ProcessHandle.of(pid)
560+
.map(ProcessHandle::isAlive).orElse(false);
561+
Thread.sleep(100);
562+
}
563+
assertTrue(!alive,
564+
"worker " + worker
565+
+ " survived a JVM exit inside its release grace"
566+
+ " window; harness log: " + logTail(harnessLog));
567+
} finally {
568+
if (harness.isAlive()) {
569+
harness.destroyForcibly();
570+
}
571+
// A failed assertion must not leave the wedged worker behind.
572+
if (worker > 0) {
573+
ProcessHandle.of(worker)
574+
.ifPresent(ProcessHandle::destroyForcibly);
575+
}
507576
Files.deleteIfExists(harnessLog);
508577
}
509578
}
510579

580+
/** The pid the harness printed, or -1 when it never got that far. */
581+
private static long reportedWorker(Path log) {
582+
try {
583+
return Files.readAllLines(log).stream()
584+
.filter(line -> line.startsWith("worker-pid "))
585+
.map(line -> line.substring("worker-pid ".length()).trim())
586+
.mapToLong(Long::parseLong).findFirst().orElse(-1L);
587+
} catch (Exception unreadable) {
588+
return -1;
589+
}
590+
}
591+
592+
/**
593+
* Harness JVM: releases a worker that ignores SIGTERM and exits at once,
594+
* inside the escalation's grace window, so only the shutdown hook can
595+
* still reclaim it.
596+
*/
597+
public static final class ReleaseGraceHarnessMain {
598+
public static void main(String[] args) throws Exception {
599+
LocalProcessRuntimeProvisioner provisioner =
600+
new LocalProcessRuntimeProvisioner(
601+
List.of("node", args[0], "--ignore-term"),
602+
Path.of(".").toAbsolutePath(),
603+
new HttpRuntimeTransport());
604+
RuntimeLease lease = provisioner
605+
.provision(ManagedContextProtocolTest.request(),
606+
ManagedContextProtocolTest.seed())
607+
.toCompletableFuture().join();
608+
long worker = ProcessHandle.current().children()
609+
.map(ProcessHandle::pid).findFirst().orElse(-1L);
610+
System.out.println("worker-pid " + worker);
611+
System.out.flush();
612+
provisioner.release(ManagedContextProtocolTest.request(), lease)
613+
.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.
617+
System.exit(0);
618+
}
619+
}
620+
511621
private static String logTail(Path log) {
512622
try {
513623
String content = Files.readString(log);
@@ -518,11 +628,13 @@ private static String logTail(Path log) {
518628
}
519629

520630
/**
521-
* Harness JVM: starts provisioning a silent worker, stays in the ready
522-
* handshake until the test ends the JVM, and so exits mid-start.
631+
* Harness JVM: starts provisioning a silent worker, waits for the test
632+
* to signal that it has been observed, and then exits mid-handshake on
633+
* its own so the shutdown hooks run.
523634
*/
524635
public static final class ExitHarnessMain {
525636
public static void main(String[] args) throws Exception {
637+
Path exitSignal = Path.of(args[0]);
526638
LocalProcessRuntimeProvisioner provisioner =
527639
new LocalProcessRuntimeProvisioner(
528640
List.of("node", "-e", "setInterval(() => {}, 1000)"),
@@ -531,9 +643,13 @@ public static void main(String[] args) throws Exception {
531643
provisioner.provision(ManagedContextProtocolTest.request(),
532644
ManagedContextProtocolTest.seed());
533645
// The worker never prints a ready line, so it stays in `starting`
534-
// for as long as this JVM lives; the sleep only bounds an
535-
// orphaned harness.
536-
Thread.sleep(60_000);
646+
// for as long as this JVM lives. The deadline only bounds a
647+
// harness the test abandoned.
648+
Instant giveUp = Instant.now().plusSeconds(60);
649+
while (!Files.exists(exitSignal)
650+
&& Instant.now().isBefore(giveUp)) {
651+
Thread.sleep(50);
652+
}
537653
System.exit(0);
538654
}
539655
}

0 commit comments

Comments
 (0)