From 5c692c1ff0716c06eead57b7075f6671cc1a3f8a Mon Sep 17 00:00:00 2001 From: Brian Warner Date: Wed, 27 Mar 2024 10:32:31 -0700 Subject: [PATCH] refactor(swingset): create vat state non-lazily Previously, vat state initialization (e.g. setting counters to zero) happened lazily, the first time that `provideVatKeeper()` was called. When creating a new vat, the pattern was: vk = kernelKeeper.provideVatKeeper(); vk.setSourceAndOptions(source, options); Now, we initialize both counters and source/options explicitly, in `recordVatOptions`, when the static/dynamic vat is first defined: kernelKeeper.createVatState(vatID, source, options); In the future, this will make it easier for `provideVatKeeper` to rely upon the presence of all vat state keys, especially `options`. Previously, vatKeeper.getOptions() would tolerate a missing key by returning empty options. The one place where this was needed (terminating a barely-created vat because startVat() failed, using getOptions().critical) was changed, so this tolerance is no longer needed, and was removed. The tolerance caused bug #9157 (kernel doesn't panic when it should), which continues to be present, but at least won't cause a crash. refs #8980 --- packages/SwingSet/src/kernel/kernel.js | 11 +++++-- .../SwingSet/src/kernel/state/kernelKeeper.js | 9 ++++-- .../SwingSet/src/kernel/state/vatKeeper.js | 30 +++++++++++++------ packages/SwingSet/src/lib/recordVatOptions.js | 4 +-- packages/SwingSet/test/clist.test.js | 8 +++++ packages/SwingSet/test/state.test.js | 20 ++++++------- 6 files changed, 55 insertions(+), 27 deletions(-) diff --git a/packages/SwingSet/src/kernel/kernel.js b/packages/SwingSet/src/kernel/kernel.js index df5dfd1fba19..4dd202f4771b 100644 --- a/packages/SwingSet/src/kernel/kernel.js +++ b/packages/SwingSet/src/kernel/kernel.js @@ -252,8 +252,7 @@ export default function buildKernel( */ async function terminateVat(vatID, shouldReject, info) { console.log(`kernel terminating vat ${vatID} (failure=${shouldReject})`); - const vatKeeper = kernelKeeper.provideVatKeeper(vatID); - const critical = vatKeeper.getOptions().critical; + let critical = false; insistCapData(info); // ISSUE: terminate stuff in its own crank like creation? // TODO: if a static vat terminates, panic the kernel? @@ -264,6 +263,14 @@ export default function buildKernel( // check will report 'false'. That's fine, there's no state to // clean up. if (kernelKeeper.vatIsAlive(vatID)) { + // If there was no vat state, we can't make a vatKeeper to ask for + // options. For now, pretend the vat was non-critical. This will fail + // to panic the kernel for startVat failures in critical vats + // (#9157). The fix will add .critical to CrankResults, populated by a + // getOptions query in deliveryCrankResults() or copied from + // dynamicOptions in processCreateVat. + critical = kernelKeeper.provideVatKeeper(vatID).getOptions().critical; + // 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)]; diff --git a/packages/SwingSet/src/kernel/state/kernelKeeper.js b/packages/SwingSet/src/kernel/state/kernelKeeper.js index 4295b0ccf963..7b24e6c91967 100644 --- a/packages/SwingSet/src/kernel/state/kernelKeeper.js +++ b/packages/SwingSet/src/kernel/state/kernelKeeper.js @@ -1288,15 +1288,17 @@ export default function makeKernelKeeper(kernelStorage, kernelSlog) { maybeFreeKrefs.clear(); } + function createVatState(vatID, source, options) { + initializeVatState(kvStore, transcriptStore, vatID, source, options); + } + function provideVatKeeper(vatID) { insistVatID(vatID); const found = ephemeral.vatKeepers.get(vatID); if (found !== undefined) { return found; } - if (!kvStore.has(`${vatID}.o.nextID`)) { - initializeVatState(kvStore, transcriptStore, vatID); - } + assert(kvStore.has(`${vatID}.o.nextID`), `${vatID} was not initialized`); const vk = makeVatKeeper( kvStore, transcriptStore, @@ -1610,6 +1612,7 @@ export default function makeKernelKeeper(kernelStorage, kernelSlog) { getVatIDForName, allocateVatIDForNameIfNeeded, allocateUnusedVatID, + createVatState, provideVatKeeper, vatIsAlive, evictVatKeeper, diff --git a/packages/SwingSet/src/kernel/state/vatKeeper.js b/packages/SwingSet/src/kernel/state/vatKeeper.js index 11b2e776ce61..33769b873004 100644 --- a/packages/SwingSet/src/kernel/state/vatKeeper.js +++ b/packages/SwingSet/src/kernel/state/vatKeeper.js @@ -39,11 +39,30 @@ const FIRST_DEVICE_ID = 70n; * @param {*} transcriptStore Accompanying transcript store * @param {string} vatID The vat ID string of the vat in question * TODO: consider making this part of makeVatKeeper + * @param {SourceOfBundle} source + * @param {RecordedVatOptions} options */ -export function initializeVatState(kvStore, transcriptStore, vatID) { +export function initializeVatState( + kvStore, + transcriptStore, + vatID, + source, + options, +) { + assert(options.workerOptions, `vat ${vatID} options missing workerOptions`); + assert(source); + assert('bundle' in source || 'bundleName' in source || 'bundleID' in source); + assert.typeof(options, 'object'); + const count = options.reapInterval; + assert(count === 'never' || isNat(count), `bad reapCountdown ${count}`); + kvStore.set(`${vatID}.o.nextID`, `${FIRST_OBJECT_ID}`); kvStore.set(`${vatID}.p.nextID`, `${FIRST_PROMISE_ID}`); kvStore.set(`${vatID}.d.nextID`, `${FIRST_DEVICE_ID}`); + kvStore.set(`${vatID}.source`, JSON.stringify(source)); + kvStore.set(`${vatID}.options`, JSON.stringify(options)); + kvStore.set(`${vatID}.reapInterval`, `${count}`); + kvStore.set(`${vatID}.reapCountdown`, `${count}`); transcriptStore.initTranscript(vatID); } @@ -125,16 +144,10 @@ export function makeVatKeeper( function getOptions() { /** @type { RecordedVatOptions } */ - const options = JSON.parse(kvStore.get(`${vatID}.options`) || '{}'); + const options = JSON.parse(getRequired(`${vatID}.options`)); return harden(options); } - function initializeReapCountdown(count) { - count === 'never' || isNat(count) || Fail`bad reapCountdown ${count}`; - kvStore.set(`${vatID}.reapInterval`, `${count}`); - kvStore.set(`${vatID}.reapCountdown`, `${count}`); - } - function updateReapInterval(reapInterval) { reapInterval === 'never' || isNat(reapInterval) || @@ -656,7 +669,6 @@ export function makeVatKeeper( setSourceAndOptions, getSourceAndOptions, getOptions, - initializeReapCountdown, countdownToReap, updateReapInterval, nextDeliveryNum, diff --git a/packages/SwingSet/src/lib/recordVatOptions.js b/packages/SwingSet/src/lib/recordVatOptions.js index 81330a8a26b6..9e87fca1200e 100644 --- a/packages/SwingSet/src/lib/recordVatOptions.js +++ b/packages/SwingSet/src/lib/recordVatOptions.js @@ -39,9 +39,7 @@ export const makeVatOptionRecorder = (kernelKeeper, bundleHandler) => { critical, meterID, }); - const vatKeeper = kernelKeeper.provideVatKeeper(vatID); - vatKeeper.setSourceAndOptions(source, vatOptions); - vatKeeper.initializeReapCountdown(vatOptions.reapInterval); + kernelKeeper.createVatState(vatID, source, vatOptions); }; /** diff --git a/packages/SwingSet/test/clist.test.js b/packages/SwingSet/test/clist.test.js index 2b3831da74c3..10237c763229 100644 --- a/packages/SwingSet/test/clist.test.js +++ b/packages/SwingSet/test/clist.test.js @@ -13,6 +13,9 @@ test(`clist reachability`, async t => { const s = kk.kvStore; kk.createStartingKernelState({ defaultManagerType: 'local' }); const vatID = kk.allocateUnusedVatID(); + const source = { bundleID: 'foo' }; + const options = { workerOptions: 'foo', reapInterval: 1 }; + kk.createVatState(vatID, source, options); const vk = kk.provideVatKeeper(vatID); const ko1 = kk.addKernelObject('v1', 1); @@ -98,12 +101,17 @@ test('getImporters', async t => { kk.createStartingKernelState({ defaultManagerType: 'local' }); const vatID1 = kk.allocateUnusedVatID(); + const source = { bundleID: 'foo' }; + const options = { workerOptions: 'foo', reapInterval: 1 }; + kk.createVatState(vatID1, source, options); kk.addDynamicVatID(vatID1); const vk1 = kk.provideVatKeeper(vatID1); const vatID2 = kk.allocateUnusedVatID(); + kk.createVatState(vatID2, source, options); kk.addDynamicVatID(vatID2); const vk2 = kk.provideVatKeeper(vatID2); const vatID3 = kk.allocateUnusedVatID(); + kk.createVatState(vatID3, source, options); kk.addDynamicVatID(vatID3); const vk3 = kk.provideVatKeeper(vatID3); diff --git a/packages/SwingSet/test/state.test.js b/packages/SwingSet/test/state.test.js index bdabd07f02a0..d1ab7b83d50a 100644 --- a/packages/SwingSet/test/state.test.js +++ b/packages/SwingSet/test/state.test.js @@ -511,8 +511,11 @@ test('vatKeeper', async t => { const store = buildKeeperStorageInMemory(); const k = makeKernelKeeper(store, null); k.createStartingKernelState({ defaultManagerType: 'local' }); - const v1 = k.allocateVatIDForNameIfNeeded('name1'); + const source = { bundleID: 'foo' }; + const options = { workerOptions: 'foo', reapInterval: 1 }; + k.createVatState(v1, source, options); + const vk = k.provideVatKeeper(v1); // TODO: confirm that this level of caching is part of the API t.is(vk, k.provideVatKeeper(v1)); @@ -547,18 +550,15 @@ test('vatKeeper.getOptions', async t => { const store = buildKeeperStorageInMemory(); const k = makeKernelKeeper(store, null); k.createStartingKernelState({ defaultManagerType: 'local' }); - const v1 = k.allocateVatIDForNameIfNeeded('name1'); - const vk = k.provideVatKeeper(v1); const bundleID = 'b1-00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000'; - vk.setSourceAndOptions( - { bundleID }, - { - workerOptions: { type: 'local' }, - name: 'fred', - }, - ); + const source = { bundleID }; + const workerOptions = { type: 'local' }; + const options = { workerOptions, name: 'fred', reapInterval: 1 }; + k.createVatState(v1, source, options); + + const vk = k.provideVatKeeper(v1); const { name } = vk.getOptions(); t.is(name, 'fred'); });