[hawkw]

There's currently a bug in sled-agent's InstanceRunner where a request to stop a VMM that has not yet started will fall through into code that asserts there is a running Propolis zone, and panic. This was pointed out by @jgallagher in a review of #11380. The bug existed long before that change, but we've apparently never managed to trigger it (?). However, a race here may become likelier after #11380 merges, since the control plane will request that starting VMMs stop.

This commit adds a regression test for this, and then fixes the bug by returning early in this case rather than falling through into the panic.

Fixes #11536

[hawkw]

@claude review

[jgallagher]

The bug existed long before that change, but we've apparently never managed to trigger it (?).

I don't think that's true! @jmpesp pointed me to #10369 and #10401

[claude]

LGTM. This is a small, correct fix for the panic when a VMM is stopped before it has started.

What was reviewed:

• The Stopped arm of put_state. Before this change, (None, None) fell through to assert!(self.running_state.is_some()) and panicked. The only steps the early return now skips are that assert and two no-ops (propolis_request/next_published were both None).
• terminate() still runs before the return, so the zone is removed and terminator.completed is set. That is the same pattern as the existing fail_vmm_and_terminate + return Ok(self.state.sled_instance_state()) path.
• The new test_stop_before_start checks four things: the reply state is Destroyed, the runner requests removal, the published state is InstancePut/Destroyed, and the runner task exits cleanly.

Extended reasoning...

The change touches only sled-agent/src/instance.rs. It is a three-line control-flow change in InstanceRunner::put_state plus a new regression test. It touches no auth, crypto, or data-exposure surface. In the no-Propolis case, the old code could only reach an assert that always failed. The early return matches an existing pattern in the same function, the new test covers the case, and no CODEOWNERS file covers the path.

[hawkw]

The bug existed long before that change, but we've apparently never managed to trigger it (?).

I don't think that's true! @jmpesp pointed me to #10369 and #10401

Oh, sigh. I'll take a look at James' prior change here.

[hawkw]

Looking slightly closer at this, I like the way @jmpesp's change just removes the assert, since there's already an unwrap in propolis_state_put, it feels a bit simpler that way. But, I kinda want to keep the regression test I added. I think what I'm going to do is pick up @jmpesp's commit on top of this branch so we can have both?

[hawkw]

or i guess we could just merge James' PR and then this one could just add the regression test

[davepacheco]

In the watercooler call I believe it was observed that James's PR eliminates this assertion failure but not a subsequent expect() that might fail. I gather this PR eliminates these cases and makes the assertion actually valid?

[hawkw]

In the watercooler call I believe it was observed that James's PR eliminates this assertion failure but not a subsequent expect() that might fail. I gather this PR eliminates these cases and makes the assertion actually valid?

The expect() call only occurs inside of the if let Some(p) = propolis_request block here: https://github.com/jmpesp/omicron/blob/86350a9921a4516d178c2e73e8bfff6425c5030a/sled-agent/src/instance.rs#L2420

and propolis_request is only set to Some if there is a propolis. So, both PRs should behave identically, my branch just has more convoluted control flow than James's, since we still make the assertion even when propolis_request is None, and use an early return to avoid hitting the assert in the one branch that can hit it. In James' branch, the control flow here is IMO much less convoluted, we just say that propolis_request is None and fall through the if let, skipping the expect(), and the assertion (which should really only be made in the case where the match suggests that we do need to send a request to propolis) is just removed completely, because it's duplicative of the expect() anyway. I think my change is strictly worse than James', since a future change to the match that adds an arm where propolis_request is None but doesn't early-return can hit the incorrect assertion once again.

I do want to land the test regression test I added, though, since I think it's worth keeping that part.