failed startVat won't fire criticalVat panic

[warner]

While working on #8980, I noticed a problem in the code which is supposed to panic the kernel if a "critical" vat gets terminated. If such a vat gets terminated super-early, during buildRootObject(), then it won't trigger the panic.

This can only happen when a dynamic vat is marked as critical (requiring criticalVatKey) and fails its initial launch, perhaps because its buildRootObject() throws an Error.

The problem is in how we manage state. When a vat triggers a fatal termination-worthy error, it sets both crankResults.abort and crankResults.terminate. The .abort causes us to unwind the state changes back to the point where we've popped the message off the run-queue (in this case a create-vat message):

agoric-sdk/packages/SwingSet/src/kernel/kernel.js

Lines 1265 to 1275 in 8e87d58

	if (crankResults.abort) {
	// Errors unwind any changes the vat made, by rolling back to the
	// "deliver" savepoint In addition, the crankResults will either ask for
	// the message to be consumed (without redelivery), or they'll ask for it
	// to be attempted again (so it can go "splat" against a terminated vat,
	// and give the sender the right error). In the latter case, we roll back
	// to the "start" savepoint, established by `run()` or `step()` before the
	// delivery was pulled off the run-queue, undoing the dequeueing.
	kernelKeeper.rollbackCrank(
	crankResults.consumeMessage ? 'deliver' : 'start',
	);

then a moment later, the .terminate invokes terminateVat:

agoric-sdk/packages/SwingSet/src/kernel/kernel.js

Lines 1265 to 1275 in 8e87d58

	if (crankResults.abort) {
	// Errors unwind any changes the vat made, by rolling back to the
	// "deliver" savepoint In addition, the crankResults will either ask for
	// the message to be consumed (without redelivery), or they'll ask for it
	// to be attempted again (so it can go "splat" against a terminated vat,
	// and give the sender the right error). In the latter case, we roll back
	// to the "start" savepoint, established by `run()` or `step()` before the
	// delivery was pulled off the run-queue, undoing the dequeueing.
	kernelKeeper.rollbackCrank(
	crankResults.consumeMessage ? 'deliver' : 'start',
	);

which needs to retrieve the critical flag:

agoric-sdk/packages/SwingSet/src/kernel/kernel.js

Lines 256 to 258 in 8e87d58

	async function terminateVat(vatID, shouldReject, info) {
	const vatKeeper = kernelKeeper.provideVatKeeper(vatID);
	const critical = vatKeeper.getOptions().critical;

then deletes state (which only matter for later deliveries, after the vat has had a chance to create any promises or state):

agoric-sdk/packages/SwingSet/src/kernel/kernel.js

Lines 268 to 276 in 8e87d58

	if (kernelKeeper.vatIsAlive(vatID)) {
	// Reject all promises decided by the vat, making sure to capture the list
	// of kpids before that data is deleted.
	const deadPromises = [...kernelKeeper.enumeratePromisesByDecider(vatID)];
	kernelKeeper.cleanupAfterTerminatedVat(vatID);
	for (const kpid of deadPromises) {
	resolveToError(kpid, makeError('vat terminated'), vatID);
	}
	}

then uses critical to decide whether to panic the kernel or not :

agoric-sdk/packages/SwingSet/src/kernel/kernel.js

Lines 277 to 288 in 8e87d58

	if (critical) {
	// The following error construction is a bit awkward, but (1) it saves us
	// from doing unmarshaling while in the kernel, while (2) it protects
	// against the info not actually encoding an error, but (3) it still
	// provides some diagnostic information back to the host, and (4) this
	// should happen rarely enough that if you have to do a little bit of
	// extra interpretive work on the receiving end to diagnose the problem,
	// it's going to be a small cost compared to the trouble you're probably
	// already in anyway if this happens.
	panic(`critical vat ${vatID} failed`, Error(info.body));
	return;
	}

The problem is that the critical flag is stored in vNN.options, which is part of the state that got unwound by the .abort flag. So when terminateVat calls vatKeeper.getOptions() to read out the .critical flag, we end up relying upon a fallback inside vatKeeper.getOptions that returns an empty options if the key was missing:

agoric-sdk/packages/SwingSet/src/kernel/state/vatKeeper.js

Lines 126 to 130 in 8e87d58

	function getOptions() {
	/** @type { RecordedVatOptions } */
	const options = JSON.parse(kvStore.get(`${vatID}.options`) || '{}');
	return harden(options);
	}

So, in this case, we always get an empty options, which means .critical is undefined, which is falsy, which inhibits the panic.

Fixes

My vague plan is to add .criticalVat to the crankResults. At the beginning of createVat, we can populate this from the options that are getting stored by optionRecorder.recordDynamic(). At the beginning of other deliveries, we can populated it from a vatKeeper.getOptions().critical. Then the post-delivery code can refer to crankResults.criticalVat instead of querying the vatKeeper (whose backing state might no longer be present).

I'm not entirely happy with this approach, though, because it means an extra DB call for every delivery. We don't usually need to know whether a vat is critical or not, and the critical status doesn't change for the lifetime of the vat. So either a lazy lookup or something that gets cached (maybe in the vatKeeper, maybe in vat-warehouse) would be more satisfying.

[warner]

Ah, I think I can avoid the unnecessary DB call. Most crankResults are built by the deliveryCrankResults() function: I'll have it only query getOptions() in the case that it is returning a truthy .terminate, and since it is called after delivery (but before anything gets unwound), it will get the right value even if the vat state is being deleted or unwound. Then, for processCreateVat (which builds its own crankResults for the creation phase), it can copy the .critical flag from the dynamicOptions that it recorded.

This means that deliveryCrankResults() will need to call provideVatKeeper(), but that should be cheap because all the callers will have already created a vatkeeper (to perform their deliveries).