feat(kernel): Add VatWorkerService terminateAll method.

[grypez]

Closes: #99 #170

Changes

@ocap/extension

• Updates VatWorkerServer and VatWorkerClient to implement the new terminateAll method.

@ocap/kernel

• Renames initWorker resp. deleteWorker vat worker service methods to launch resp. terminate.
• Adds a new terminateAll method to the vat worker service interface.
• Adopts @ocap/errors for VatAlreadyExistsError and VatDeletedError.
• Implements a generic IdentifiedMessageKit in messages/message-kit.ts which abstracts the { id, payload } form used by both VatCommand and VatWorkerServiceCommand to handle messages by id rather than method.

Comments

It would also be nice to use either the PostMessageStream or MessagePortStream from @ocap/streams instead of the ad hoc approach I have taken here; but because the transfer argument is a peer to, not a child of, message, some workaround is necessary at the interface level and the type guard level to permit this use case.

[sirtimid]

on the other hand we could extract this to a isValidVatCommandId function to show intent better

[grypez]

Since the id guard is only used internally to the kit I do not see the need to give it a name.

I could have makeIdentifiedMessageKit accept an object, so that this call would look like:

const vatMessageKit = makeIdentifiedMessageKit({
  source: vatCommand,
  isMessageId: (value: unknown): value is `${VatId}:${number}` =>
    typeof value === 'string' &&
    /^\w+:\d+$/u.test(value) &&
    isVatId(value.split(':')[0]),
});

Would that suffice?

[grypez]

Done 8c3f217

[rekmarks]

First, a word of caution:

Naively, given a method foo, implementing fooAll should amount to something like:

• Add a field fooMap: Map<FooId, Foo> = new Map();
• for (const id of fooMap) { this.terminate(id); }

That we require a PR of this magnitude to accomplish the above is a sign that our abstractions are too numerous and don't have the right responsibilities. Not much to do here, but something to keep in mind.

Second, I think we can potentially make a couple of modifications to our approach here:

Partially adopts @ocap/errors standards for VatAlreadyExistsError and VatDeletedError.

I believe this refers to where our well-known error codes occur as strings. A partial adoption of something is a potential source of bugs and confusion, so we may want to just give up on this until #170 can be addressed.

Implements a generic IdentifiedMessageKit in messages/message-kit.ts which abstracts the { id, payload } form used by both VatCommand and VatWorkerServiceCommand to handle messages by id rather than method.

Per @sirtimid's comment, I'm not sure that this is different enough to warrant its own abstraction.

[rekmarks]

Why is terminateAll not tested here?

[grypez]

It should have been. a60ec03

[rekmarks]

I didn't know you could do conditional type guards, and now that I do, I'm not sure how I feel about it.

This particular one has a number of issues.

In the first place, its highly specialized nature means that it should probably make its home in vat-worker-service.ts.

Second, it is probably better to split it into two separate functions, one for each "mode". However, it may be possible to elide the the "optional" mode completely, since we can assert that an arbitrary object has optional properties without type guards.

Third, in its actual usage, we seem to be looking for specific error codes, but the actual type we get out of the deal doesn't retain this information. Generally, we shouldn't have to care about which specific error is attached to some object; if it's an error code we know, we know how to handle it. Ensuring that the correct errors are thrown is the responsibility of the unit tests of the code that throws those errors, not a concern of our messaging layer.

In sum, we can probably just let the recipient figure out the kind of error that it is.

[grypez]

In the first place, its highly specialized nature means that it should probably make its home in vat-worker-service.ts.

Okay 👍

Second, it is probably better to split it into two separate functions, one for each "mode".

I don't mind doing this, but, given that the split results in more and duplicated code, and the usage

hasMarshaledError('optional', reply, ErrorCode.VatAlreadyExists)

has comparable verbosity and intellisense support to

hasOptionalMarshaledError(reply, ErrorCode.VatAlreadyExists)

I would like to know why you think it is probably better to split up.

However, it may be possible to elide the the "optional" mode completely, since we can assert that an arbitrary object has optional properties without type guards.

The optional type guard asserts that if the value exists then it has the right type. This is not equivalent to not checking, and is the correct way to check for a ? typed property.

Third, in its actual usage, we seem to be looking for specific error codes, but the actual type we get out of the deal doesn't retain this information.

I support your concern for the misalignment of the guarded and declared type. I note that since the guard implementation is narrower than its guarded type the resulting code should remain type safe under our usage conventions if (!isX(value)) { throw new Error } and if (isX(value)) { useX(value) } which I understand to be in place to support this kind of weak assertion.

Generally, we shouldn't have to care about which specific error is attached to some object; if it's an error code we know, we know how to handle it. Ensuring that the correct errors are thrown is the responsibility of the unit tests of the code that throws those errors, not a concern of our messaging layer.

This convention is already in use in @metamask/snaps so I am not keen to argue against it for very long. That said,

I have typically conceived of errors as a possible return type of the throwing routine, and therefore they would be a valid concern of the message types that transmit return values. I think 'if we know the error code, we know how to handle it' introduces implicit dependency between e.g. client and server which would be explicit if the error codes were included in the message reply types. A simple example of that implicit dependency going awry is a client that devotes code to handling error messages the server never sends.

The presence at all of the messaging layer in this case arises as an artifact of the extension implementation; a node implementation probably doesn't need it at all. I think this indicates the message types here should be declared in @ocap/extension, however that requires me to export some things I don't wish to export.

[grypez]

With the marshaling working the point is moot, check b063c07

[rekmarks]

@grypez I'm getting a lot of type errors locally in the vat worker service tests files. Are they there for you?

See also: #164 (comment)

[sirtimid]

@grypez I'm getting a lot of type errors locally in the vat worker service tests files. Are they there for you?

See also: #164 (comment)

@grypez @rekmarks The new yarn test:ts command shows them as well

[grypez]

Types are once again quasi-coherent 7b67fa8. Note the one as unknown as is guarded by a runtime assertion.

[rekmarks]

LGTM.