Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zoe will stop tracking contract vat termination when vat-zoe is upgraded #8387

Closed
warner opened this issue Sep 25, 2023 · 1 comment · Fixed by #8453
Closed

zoe will stop tracking contract vat termination when vat-zoe is upgraded #8387

warner opened this issue Sep 25, 2023 · 1 comment · Fixed by #8453
Assignees
Labels
bug Something isn't working Zoe package: Zoe

Comments

@warner
Copy link
Member

warner commented Sep 25, 2023

As mentioned in #8263 (comment) , while investigating outstanding mainnet promises, we concluded that Zoe will fail to re-subscribe to contract-vat done() promises if/when vat-zoe is upgraded.

Zoe uses vatAdminSvc to create a new vat for each contract instance, which returns a per-vat adminNode that serves as a control facet. Zoe can use this to terminate the vat or trigger an upgrade. In addition, calling E(adminNode).done() will return a Promise that doesn't fire until the vat is terminated. The done() promise fulfills if the vat exits "happy" (if it self-terminates with vatPowers.exitVat()), and rejects if it exits "sad" (vatPowers.exitVatWithFailure(), adminNode.terminateWithFailure(), or the vat exceeds its meter or has some fatal error).

Zoe currently uses an ephemeral E.when() inside startInstance to attach a call to exitAllSeats to this promise:

E.when(
E(adminNode).done(),
completion => {
instanceAdmin.exitAllSeats(completion);
},
reason => instanceAdmin.failAllSeats(reason),
);

which means that if/when vat-zoe is restarted or upgraded, the callback will be lost. The zoe vat will still be subscribed to the promise (the kernel will deliver a dispatch.notify), but the new zoe incarnation will not have any callbacks registered to fire, so it will ignore the resolution.

As a result, when vat-zoe is upgraded, it will stop tracking that done() promise. So if any contract vats terminate, zoe will not clean up after them. Depending upon the exit conditions on each offer, that might make it difficult to close out a seat.

One possible fix is for Zoe to use globalThis.VatData.providePromiseWatcher() to establish a durable watcher on the done Promise. That would remove the need to re-fetch a done promise when zoe is upgraded (however we would still need the isUpgradedDisconnection() check to guard against vat-vat-admin being upgraded). However, that won't help with the existing done() promises currently tracked by the deployed zoe.

Another fix, which would help with the current deployment, would be for Zoe's buildRootObject to scan its durable storage for the adminNodes of all active contract vats, and call done() on each of them, to re-establish the callbacks.

If we wanted to combine the two techniques, zoe's per-contract-vat table should have a flag to track whether we have a durable promise watcher registered or not. At startup (buildRootObject), walk the table, and for every vat that lacks the flag, use providePromiseWatcher(), and set the flag. In startInstance, when starting a new contract vat, use providePromiseWatcher() and also set the flag.

@warner warner added bug Something isn't working Zoe package: Zoe labels Sep 25, 2023
@Chris-Hibbert
Copy link
Contributor

Chris-Hibbert commented Oct 12, 2023

Another fix, which would help with the current deployment, would be for Zoe's buildRootObject to scan its durable storage for the adminNodes of all active contract vats

I don't think Zoe has any strong maps of instances. Can we add the ability for an exo class to scan its members?

The closest thing is instanceToInstanceAdmin in instanceAdminStorage, but it's a weak map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Zoe package: Zoe
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants