fix: the health probe gets a third answer, and stops holding the lock - #441
Merged
Merged
Conversation
Closes #437. #440 bounded the COM call; this is the rest. Engine.Running returns (bool, error) ------------------------------------ Both implementations used to collapse a failed probe into false, and the reconciler's response to false is Engine.Start. So a wedged wslservice, a WSL update mid-flight or any transient probe failure read as "the engine is down" and provoked a start of an engine that was almost certainly running. That is also why the bound could not simply be added to tick: bounding a two-valued Running converts "slow" into "down", which is the same bug with a timer attached. The third answer had to come first. (false, nil) now means "definitely not running". Anything the probe could not determine is an error, and tick does nothing at all that round rather than guess -- lastUp keeps the last reading worth trusting, and the next tick asks again. Provisioner keeps both shapes. EngineRunning stays for the waitFor polls, where "cannot tell" and "not yet" both correctly mean keep waiting; EngineRunningErr is for the caller whose next move is to start something. Same probe, the error no longer discarded on the path that needs it. The probe runs outside s.mu --------------------------- tick held the mutex across Engine.Running with the supervisor's process-lifetime context, so a service that stopped answering parked the reconciler INSIDE the lock: every Demand() blocked, meaning every docker connection hung rather than failing; LifecycleSnapshot froze, so the tray could not even show the supervisor was stuck; and the poke loop never ran again. Now: probe outside, decide inside, with a 60s ceiling on the probe itself. Generous on purpose -- a cold `wsl.exe --list` on a loaded machine is not fast, and this is a ceiling for a probe that has stopped answering, not a latency target. Moving it out opens a window where a Demand can cold-start the engine between the probe and the decision, leaving tick holding a reading that no longer describes the machine. startGen closes it: tick captures the counter before probing and gives up the round if it moved. Start is idempotent so the stale case was survivable anyway, but "engine is down; starting it" logged about an engine somebody just started is how an operator stops believing the log. Verification ------------ Three negative controls, each run both ways: probe error ignored -> Start called 41 times in 400ms against a RUNNING engine probe back inside mu -> "Demand blocked behind a probe in flight" (from #440) bound gone -> the COM test times out Plus TestDefiniteDownStillStartsTheEngine, so the fix cannot have bought safety by quietly disabling the supervisor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #437. #440 bounded the COM call; this is the rest.
Engine.Runningreturns(bool, error)Both implementations collapsed a failed probe into
false— and the reconciler's response tofalseisEngine.Start. So a wedgedwslservice, a WSL update mid-flight, or any transient probe failure read as "the engine is down" and provoked a start of an engine that was almost certainly running.That is also why the bound could not simply be added to
tick. Bounding a two-valuedRunningconverts "slow" into "down" — the same bug with a timer attached. The third answer had to come first.(false, nil)now means definitely not running. Anything the probe could not determine is an error, andtickdoes nothing that round rather than guess.Provisionerkeeps both shapes:EngineRunningfor thewaitForpolls, where "cannot tell" and "not yet" both correctly mean keep waiting;EngineRunningErrfor the caller whose next move is to start something.The probe runs outside
s.mutickheld the mutex acrossEngine.Runningwith the supervisor's process-lifetime context, so a service that stopped answering parked the reconciler inside the lock: everyDemand()blocked (docker commands hung rather than failing),LifecycleSnapshotfroze so the tray could not show the supervisor was stuck, and the poke loop never ran again.Now: probe outside, decide inside, 60 s ceiling on the probe. Generous deliberately — a cold
wsl.exe --liston a loaded machine is not fast, and this is a ceiling for a probe that has stopped answering, not a latency target.Moving it out opens a window where
Demandcan cold-start the engine between the probe and the decision.startGencloses it: tick captures the counter before probing and gives up the round if it moved.Startis idempotent so the stale case was survivable anyway — but "engine is down; starting it" logged about an engine somebody just started is how an operator stops believing the log.Verification
Three negative controls, each run both ways:
Startcalled 41 times in 400 ms against a running enginemuDemand blocked behind a probe in flightPlus
TestDefiniteDownStillStartsTheEngine, so this cannot have bought safety by quietly disabling the supervisor.