Skip to content

runtime-broker: remaining test-oracle gaps and the reconcile=true fan-out decision (deferred from #13214) #13275

Description

@wenshao

runtime-broker: remaining test-oracle gaps and the reconcile=true fan-out decision from PR #13214

Why this issue exists

PR #13214 (issue #13183 hardening) went through three bot review rounds and five self-audit rounds. Under this repo's ~5-review-round rule the last wave landed Critical fixes only, so four items were deferred. None is a known production defect: three are test oracles that cannot currently fail for the property they are named after, and one is a design decision that needs a client-side change.

1. stalledRenewalTickDoesNotDeadlockClose's 5 s bound cannot fail

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

The test measures closeElapsed < 5s, but RuntimeBrokerService.close() never joins: it is closed.compareAndSet → four cancel(false) map sweeps → scheduler.shutdownNow() → renewalScheduler.shutdownNow() → provisioner.close(). awaitTermination has no call site in the module's main sources, so the measured time is bounded by two non-blocking calls no matter where the renewal thread is parked. The fake stall is also interruptible — shutdownNow() interrupts it and the stub's catch only re-sets the flag — so adding a natural renewalScheduler.awaitTermination(...) to close() would leave this test green at ~12 ms. Unlike its two siblings, it also never records which pool the parked thread came from, so nothing ties the stall to the renewal pool its name claims.

Fix: make the park survive interruption (model a wedged JDBC read), and record the thread name, asserting qwen-runtime-broker-lease-renewal.

2. The LOST drain's per-pass claim renewal is not pinned

Issue13183RegressionTest.lostReclaimDrainsAWholeGeneration

The test pins the "loops past three bounded passes" half of its javadoc but not the "under a renewed claim" half. reclaimService(...) builds the service with a 3 s operation lease and the drain is 303 ACQUIRING sessions against LIMIT 100 … FOR UPDATE per pass — four passes costing tens of milliseconds in H2, so hoisting renewRecoveryClaim(claimed) out of the loop leaves the test green. In production (30 s lease, MySQL over the network, 1600 sessions = 16 locked passes) the claim lapses mid-drain and recoverLostDrained throws runtime_provision_fenced: the warm answers 503 and the LOST generation is not reclaimed, which is what the loop exists to prevent.

Fix: wrap the binding repository in the file's own DelegatingBindingRepository, record expected.getOperationLeaseUntil() for each recoverLost call on the LOST binding, and assert the sequence advances (more than one distinct deadline).

3. startResponsesNeverServeTheCooldownCache asserts only the bypass half

RuntimeBrokerHttpServerTest

Nothing pins that the preceding cooled GET actually armed the cooldown, so the test cannot tell "armed and bypassed" from "never armed". Narrowing stampUnknownLookupCooldown to lookups that leave the record UNKNOWN keeps it green, while every plain GET over an executing execution fans through to the worker again — the poll shape #13183 was filed for. The oracle is also time-bounded: a stall longer than the 1 s cooldown between the GET and the :start retry silently disarms it.

Fix: assert the worker status-call count does not grow across the cooled GET (or read the cooldown map through a package-visible accessor), so "never armed" fails independently of the bypass assertion.

4. The observation cooldown cannot reach the shipped repeated-UNKNOWN poller

Decision needed, not just a patch. The only in-tree caller that tolerates repeated UNKNOWN observations is packages/cli/src/serve/hosted-workspace-broker.ts, which builds waitForUnknown ? path + '?reconcile=true' : path and loops every 250 ms. reconcile=true bypasses the cooldown by design — that is what makes an explicit reconcile authoritative — so the fan-out on that path is unchanged. Measured over a real MCP turn with an execution made UNKNOWN for real: ≈1.4 worker status calls/s on both the patched and the pre-patch broker, about 900 status calls per execution over a 630 s window.

Options:

  • apply a shorter cooldown to reconcile=true as well, and pin the trade-off (an explicit reconcile can then answer up to that much stale);
  • back off in the client, which keeps the broker's contract intact;
  • leave it and keep the design doc's statement that the cooldown covers automatic observation only (currently the case).

Context

Found by the bot's round-3 review (R3-10, R3-11, R3-12, R3-3) and confirmed by a three-arm verification rig on PR #13214. Deferred there under the round cap; recorded in the PR thread so nothing is silently dropped.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions