Repository navigation
fix(watchdog): close the edges left from #9's review - #25
abhinavlenka wants to merge 1 commit into
Conversation
…sa-labs-inc#14) Six follow-ups, all in the host-death watchdog: 1. Host resolution ran as a default argument, so it evaluated before the function body's try and anything it threw escaped main() - the plugin failed to start, which is the opposite of the documented "lose the watchdog, keep serving". It now runs inside the guard, and a resolution failure keeps serving rather than halting: not knowing who the host is says nothing about whether it is alive. 2. BOSS_HOST_PID had no pid-reuse protection. A recycled pid yields a live handle for an unrelated process, which the watchdog would then wait on. Where the host also exports BOSS_HOST_START_MS, a pid whose process started at a different time now resolves to orphaned. Inert until the host sets it, and an unreadable start time accepts the pid. 3. The zombie-host gap is documented, in the KDoc and AGENTS.md. 4. A blank BOSS_HOST_PID reads as unset rather than malformed, so the warning for a genuine quoting slip is not buried under one that fires for a launcher no-op. The distinction is a type (DeclaredHostPid) rather than only a log line, because otherwise nothing can test it. 5. The halt KDoc says plainly that plugin-installed shutdown hooks do not run either. 6. Cosmetic: java.util.Optional is gone from the internal signatures (nullable ProcessHandle, with Optional left where the JDK forces it); the watchdog moves to HostDeathWatchdog.kt beside its test, taking 139 lines out of PluginProcessMain.kt; the stale onParentGone in a test message is now onHostGone; and the detached-sleeper tests skip on Windows instead of failing with an unrelated IOException. Adds 8 cases (22 in HostDeathWatchdogTest).
|
Closing in favour of #22, which was opened on 15 September and covers the same six follow-ups. I missed it before opening this - my mistake, and apologies for the noise. Worth saying that #22's pid-reuse check is the better design of the two: it rejects a process that acquired One thing from here that may be worth folding into #22 if you want it: reading Unrelated, found while building this: a clean checkout cannot build right now, because |
1 participant
What this does
Closes #14: the six follow-ups from #9's review. The watchdog works - an orphaned plugin JVM halts instead of redialling a dead kernel forever - these are its edges.
1. Host resolution ran outside the "keep serving" guard.
installHostDeathWatchdog(host = resolveHostHandle())evaluated resolution as a default argument, so it ran before the body'stry. Anything it threw escapedmain()and the plugin failed to start - the opposite of the documented behaviour, and resolution reads the environment and calls into the JDK, so it is not obviously infallible. It now runs inside the guard, asresolveHost: () -> ProcessHandle?.A resolution failure keeps serving rather than halting. Not knowing which process is the host says nothing about whether it is alive, and halting there would turn a resolution bug into a plugin that never starts. That is a different branch from "there is no host", which still halts, and the two are tested apart.
2. Pid reuse. A pid is a reusable integer: between the host exporting
BOSS_HOST_PIDand this runtime reading it, the host can die and the number can be handed to something unrelated, andProcessHandle.ofhands back a live handle for that stranger. We would then wait on a process that may well outlive us - the leak the watchdog exists to close, reached by a different road.BOSS_HOST_START_MScloses it: the host exports its ownstartInstant()as epoch millis beside its pid, and a pid whose process started at a different time resolves to orphaned. Three deliberate limits:BOSS_HOST_PIDinProcessSpawner; happy to send it to BossConsole if you want the pairing.ProcessHandle.info()is best-effort and reports nothing for a foreign process on some platforms; refusing there would stop the runtime working on those platforms entirely.startInstant()and another's reading of it. A pid is not recycled onto a process that started seconds after the one it replaced.3. The zombie host is documented in the KDoc and AGENTS.md, as the issue suggests: a host that has exited but not been reaped stays visible, so
onExit()never fires. Nothing here can tell that from a live host without reading platform process state.4. A blank
BOSS_HOST_PIDreads as unset, not malformed. Both fall back to the OS parent, so the difference is only in what gets logged - which is the point: warning about a launcher's empty export buries the warning that exists for a real quoting slip. Made a type (DeclaredHostPid.Unset / Malformed / Pid) rather than left as a log line, because otherwise nothing can test it - and a mutation that collapsed the two passed every assertion I had until I did.5. The
haltKDoc now says plainly that plugin-installed shutdown hooks do not run either, so a plugin that persists on shutdown loses that write.haltis still right - the alternative can hang forever - but the cost is not paid by this process alone.6. Cosmetic, all four:
java.util.Optionalis gone from the internal signatures (ProcessHandle?), left only where the JDK forces it - theProcessHandleinterface in a test stub.HostDeathWatchdog.ktbesideHostDeathWatchdogTest.kt, taking 139 lines of process-lifetime logic out ofPluginProcessMain.kt, which already carries manifest parsing, reflective loading and state-sync wiring.HostDeathWatchdogTest.kt's staleonParentGoneis nowonHostGone.IOExceptionaboutsh. CI is ubuntu-only, so this only ever fires on a desktop dev machine, where the failure said nothing about what was being tested.Tests
22 cases in
HostDeathWatchdogTest, 8 of them new (262 in the repo, all passing):Unset,abcand12.5asMalformed, and a blank value still falls back to the OS parent.Mutation-checked, each against the test that should catch it:
BOSS_HOST_PIDas malformedThe last two only fail because of the two structural changes above - a type for the pid reading, and
isDeclaredHostbeinginternal. Before those, both mutations passed the whole suite, which is the honest reason they were made.Note for review
./gradlew testneeds-Pipc.version=1.2.0on a clean checkout right now:gradle.propertiespinsipc.version=1.0.0, and the latest BossConsole-Releases publishesboss-ipc-1.2.0.jar, sodownloadDeps404s. Unrelated to this change, and I have left the property alone rather than bump a version floor in a hardening PR - say the word if you would like it in here.