Repository navigation
fix(runtime-broker): close cross-process release race and harden scheduling #13214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
3df197f
34b3f07
9e4e470
3710ef7
51c1841
af6691c
20d8f48
56c2096
4e91dc5
445964f
6460cd9
608f6d6
51a526d
b81f21d
2538295
5cd1c83
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Mutation-proven: with the fake's default arm wedged ( |
||
| LocalProcessRuntimeProvisionerTest.requireNode(); | ||
| Process worker = start(JSON.writeValueAsBytes(fixtures().get("boot"))); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.