Add cancellation to RPC handlers for the vaults domain

[aryanjassal]

Description

Without proper abortion or cancellation for a RPC, it will keep running and conclude first. This ends up adding a significant amount of time when closing the agent, as all outstanding RPC calls must resolve beforehand.

Issues Fixed

• Fixes Review and refactor RPC handlers to handle cancellation properly #832 (FIX ENG-432)

Tasks

• Add context and cancellation to VaultInternal.ts
• Add context and cancellation to VaultManager.ts
• Add cancellation to vault RPC handlers
  • VaultsVersion
  • VaultsClone
  • VaultsPull
  • VaultsCreate
  • VaultsDelete
  • VaultsList
  • VaultsLog
  • VaultsRename
  • VaultsScan
  • VaultsPermissionsGet
  • VaultsPermissionsSet
  • VaultsSecretsCat
  • VaultsSecretsEnv
  • VaultsSecretsGet
  • VaultsSecretsList
  • VaultsSecretsMkdir
  • VaultsSecretsNew
  • VaultsSecretsNewDir
  • VaultsSecretsRemove
  • VaultsSecretsRename
  • VaultsSecretsStat
  • VaultsSecretsWriteFile
• Add tests for cancellation
  • Fast-check cancellation at random times
• Change the SuccessOrErrorMessage tag in Polykey CLI

Old tasks list

• 1. Add cancellation to Client RPC
  • AgentLockAll
  • AgentStatus uncancellable unary command
  • AgentStop uncancellable unary command
  • AgentUnlock cant cancel unary noop command
  • AuditEventsGet
  • AuditMetricGet
  • GestaltsActionsGetByIdentity
  • GestaltsActionsGetByNode
  • GestaltsActionsSetByIdentity
  • GestaltsActionsSetByNode
  • GestaltsActionsUnsetByIdentity
  • GestaltsActionsUnsetByNode
  • GestaltsDiscoveryByIdentity
  • GestaltsDiscoveryByNode
  • GestaltsDiscoveryQueue
  • GestaltsGestaltsGetByIdentity
  • GestaltsGestaltsGetByNode
  • GestaltsGestaltsList
  • GestaltsGestaltsTrustByIdentity
  • GestaltsGestaltsTrustByNode
  • IdentitiesAuthenticate
  • IdentitiesAuthenticatedGet
  • IdentitiesClaim
  • IdentitiesInfoConnectedGet
  • IdentitiesInfoGet
  • IdentitiesInvite
  • IdentitiesProvidersList
  • IdentitiesTokenDelete
  • IdentitiesTokenGet
  • IdentitiesTokenPut
  • KeysCertsChainGet
  • KeysCertsGet
  • KeysDecrypt
  • KeysEncrypt
  • KeysKeyPair
  • KeysKeyPairRenew
  • KeysKeyPairReset
  • KeysPasswordChange
  • KeysPublicKey
  • KeysSign
  • KeysVerify
  • NodesAdd
  • NodesClaim
  • NodesFind
  • NodesGetAll
  • NodesListConnections
  • NodesPing
  • NotificationsInboxClear
  • NotificationsInboxRead
  • NotificationsInboxRemove
  • NotificationsOutboxClear
  • NotificationsOutboxClear
  • NotificationsOutboxRead
  • NotificationsOutboxRemove
  • NotificationsSend
  • VaultsClone
  • VaultsCreate
  • VaultsDelete
  • VaultsList
  • VaultsLog
  • VaultsPermissionGet
  • VaultsPermissionSet
  • VaultsPermissionUnset
  • VaultsPull
  • VaultsRename
  • VaultsScan
  • VaultsSecretsCat
  • VaultsSecretsEnv
  • VaultsSecretsGet
  • VautlsSecretsList
  • VaultsSecretsMkdir
  • VaultsSecretsNew
  • VaultsSecretsNewDir
  • VaultsSecretsRemove
  • VaultsSecretsRename
  • VaultsSecretsStat
  • VaultsSecretsWriteFile
  • VaultsVersion

2. Add cancellation to node RPC
   • NodesClaimNetworkSign
   • NodesClaimNetworkVerify
   • NodesClaimsGet
   • NodesClosestActiveConnectionsGet
   • NodesClosestLocalNodesGet
   • NodesConnectionSignalFinal
   • NodesConnectionSignalInitial
   • NodesCrossSignClaim
   • NotificationsSend
   • VaultsGitInfoGet
   • VaultsGitPackGet
   • VaultsScan

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[linear]

ENG-432 Review and refactor RPC handlers to handle cancellation properly

[aryanjassal]

For each RPC handler, we need to provide a cancel, meta, and ctx parameter. The type of cancel is (reason?: any) => void, meta is Record<string, JSONValue> | undefined, and ctx is ContextTimed. The type for ctx is easy to import and use, but it is long and error-prone to have to manually enter the type for cancel and meta.

Should I make a commit into js-rpc staging which exports the types properly, or should I just define it inside Polykey itself?

[tegefaulkes]

Don't bother defining the type for it unless you're actually using it. So you can just name the import parameter as _import it indicate it's not being used. I think I had the types of these parameters being inferred properly at some point. There's some oddity with abstract classes that make the types not work properly here.

[aryanjassal]

Don't bother defining the type for it unless you're actually using it. So you can just name the import parameter as _import it indicate it's not being used. I think I had the types of these parameters being inferred properly at some point. There's some oddity with abstract classes that make the types not work properly here.

I just did this to mute the warnings for type hints, as TS assumes it is any by default, and warns me about it. For unused methods, I do indeed prefix it with an underscore to avoid the usage warnings.

[CMCDragonkai]

Please make sure to review the actual situation where this can cause a big problem: MatrixAI/Polykey-CLI#264 (comment).

If you're fixing it, a test must be created to prevent regression of that specific problem.

[aryanjassal]

Please make sure to review the actual situation where this can cause a big problem: MatrixAI/Polykey-CLI#264 (comment).

If you're fixing it, a test must be created to prevent regression of that specific problem.

All this does is add support for cancelling RPC which are in the middle of something. This would ensure that a RPC won't keep the entire agent alive in a deadlock state.

I'm not sure how this relates to adding support for streaming progress updates back. They can still be done just fine after this PR is implemented, no?

[aryanjassal]

Polykey/src/client/handlers/VaultsClone.ts

Lines 28 to 42 in 19e5278

	const {
	nodeId,
	}: {
	nodeId: NodeId;
	} = validateSync(
	(keyPath, value) => {
	return matchSync(keyPath)(
	[['nodeId'], () => ids.parseNodeId(value)],
	() => value,
	);
	},
	{
	nodeId: input.nodeIdEncoded,
	},
	);

This whole section of parsing nodeId can be shrunk down into this one-line solution, but we aren't using this solution. I asked Brian and he told me that using validateSync and matchSync is just a standard for parsing in handlers, but I can't help but feel that it is a huge waste.

const nodeId = ids.parseNodeId(input.nodeIdEncoded);

This single line is more concise and performant than the validateSync and matchSync block. There have been cases where I had to use the validateSync block to actually do parsing. This has mostly been a case with RawHandler RPC handlers. Using this solution everywhere seems like overkill, so shouldn't we try to simplify the code where we can?

@CMCDragonkai

[aryanjassal]

isomorphic-git/isomorphic-git#1867

isomorphic-git does not support cancellation and abort signals. This means that there is no native way to cancel a long-running operation, like git clone.

My idea is to use a deconstructed promise and reject the promise when the context sends an abort signal, but I'm not sure how this would affect other things, especially the underlying code in isomorphic-git.

[CMCDragonkai]

This requires fast check model checking applied. I don't trust it until sufficient variations of side effects are tested and the right defaults are found. Along with benchmarking.

[aryanjassal]

We check for abortion in multiple ways in Polykey. In some places, it is ctx.signal.throwIfAborted() and in others, it is if (ctx.signal.aborted) throw ctx.signal.reason;

Which one should we follow consistently throughout the repo?
@tegefaulkes

[aryanjassal]

await vaultManager.pullVault(
  {
    vaultId: vaultId,
    pullNodeId: nodeId,
    pullVaultNameOrId: pullVaultId,
    tran: tran,
  },
  ctx,
);

Here, the parameters are being sent as a JSON object for some reason. I want to know when a JSON object should be used for parameters and when should they be passed as-is.

Before, the ctx field did not exist here, so it looked fine. But after adding the context, having the JSON object there just adds unnecessary complications and it can be omitted.

await vaultManager.pullVault({
  vaultId: vaultId,
  pullNodeId: nodeId,
  pullVaultNameOrId: pullVaultId,
  tran: tran,
});

The one benefit I see is that the parameters don't need to be ordered and can be set via setting key-value pairs like in Python. However, I can't do the same with the context, as it needs a special decorator which doesn't work well with JSON objects.

@timedCancellable(true)
public async pullVault(
  {
    vaultId,
    pullNodeId,
    pullVaultNameOrId,
    tran,
  }: {
    vaultId: VaultId;
    pullNodeId?: NodeId;
    pullVaultNameOrId?: VaultId | VaultName;
    tran?: DBTransaction;
  },
  @context ctx: ContextTimed, // The decorator can't go in JSON object
): Promise<void>;

How should I handle these cases, @tegefaulkes?

[aryanjassal]

I went over a couple of things with Brian in a meeting today, and this is the key takeaways.

1. There is no real easy way to cancel git operations. Instead of racing promises, he suggested an alternate method. I need to proxy the efs which takes in a context, and if the context is aborted, the efs will throw back aborted errors and cancel the git operation. This is not a final solution as it's quite hacky. I think the best solution would be to implement signal support upstream.
2. Abortion is handled inconsistently throughout the codebase. Some places use if (ctx.signal.aborted) throw ctx.signal.reason); and others use ctx.signal.throwIfAborted(). The throwIfAborted method is a newer addition, and as such, not everything has been updated yet. So, update all abortion to use throwIfAborted.
3. Wherever I have defined contexts, they are defined as ctx?: Partial<ContextTimed>. This is not quite correct. The correct way to do this is ctx?: Partial<ContextTimedInput>. This will allow passing numbers to convert to a timeout automatically with the decorator. In places, I also was using ContextCancellable thinking ContextTimed is just a timeout, but I later realised that ContextTimed is an intersection with ContextCancellable, so I need to use ContextTimed everywhere instead.
4. In tests, I did {} as ContextCancellable to create an empty object. Some tests failed as they couldn't access the signal property of the context, but some tests never used the context at all, so all the tests passed even with this. This all needs to change to const ctx = { signal: abortController.signal } as ContextTimed;. The as keyword is used because ContextTimed expects a timer field, too, but its a hassle to set up and is not even used.
5. In places within the Vaults domain, we have used a JSON object to pass in parameters. This is to avoid spamming a bunch of undefined if a parameter won't be defined. However, some objects have the database transaction also bundled up inside this object. The transaction and the context must be defined outside like function abc(..., ctx, tran); so this will need to change in the code, too.
6. During a git.clone operation, when I try to cancel the gitPackGet stream, it throws an error along the lines of "Cannot execute readableCancel". I need to look into why this is happening and if this is a js-quic bug or not.

[aryanjassal]

I am kind of nitpicking here, but I noticed this minor point I want to bring up and get some clarification on.

We rely on getting the class names for logger messages. This is done differently for static and instance methods for a class. In static methods, this.name is required to get the class name. In instance methods, this.constructor.name is required. This inconsistency is difficult to know and can bite unknowing newcomers in their posteriors.

Can't we do something like a getter that returns the class name reliably in all contexts?

class VaultInternal {
  static get className(): string {
      return this.name;
  }

  get className(): string {
      return this.constructor.className;
  }

  // Now the class name can be obtained in any context using
  // this.className
}

Note that this code was generated by ChatGPT and has not been tested yet. This probably won't be the final implementation, but I am just asking if this idea can be implemented.

[aryanjassal]

We are also inconsistently using async function abc() and async function abc(): Promise<void>. Should a method be decorated with the return type if it is the default return type, or should the code detect that implicitly?

I think we should be explicit about this. Thoughts, @tegefaulkes?

[aryanjassal]

      // This should really be an internal property
      // get whether this is remote, and the remote address
      // if it is, we consider this repo an "attached repo"
      // this vault is a "mirrored" vault
      if (
        (await tran.get([
          ...this.vaultMetadataDbPath,
          VaultInternal.remoteKey,
        ])) != null
      ) {
        // Mirrored vaults are immutable
        throw new vaultsErrors.ErrorVaultRemoteDefined();
      }

This was a comment made in VaultInternal.ts. I don't think I should get this done as a part of this PR. Should I just make a new issue to track this?

[CMCDragonkai]

I am kind of nitpicking here, but I noticed this minor point I want to bring up and get some clarification on.

We rely on getting the class names for logger messages. This is done differently for static and instance methods for a class. In static methods, this.name is required to get the class name. In instance methods, this.constructor.name is required. This inconsistency is difficult to know and can bite unknowing newcomers in their posteriors.

Can't we do something like a getter that returns the class name reliably in all contexts?

class VaultInternal {
  static get className(): string {
      return this.name;
  }

  get className(): string {
      return this.constructor.className;
  }

  // Now the class name can be obtained in any context using
  // this.className
}

Note that this code was generated by ChatGPT and has not been tested yet. This probably won't be the final implementation, but I am just asking if this idea can be implemented.

Probably doesn't work that's why it's different.

[aryanjassal]

Probably doesn't work that's why it's different.

But is this a good thing to invest some time and effort in? This can probably be a quick fix as a part of this PR itself, or I can make an issue which explores this more in depth and maybe explores a way to get this in other repos without defining it in every class.

[aryanjassal]

src/vaults/fileTree.ts has been sitting in the vaults domain without usage. Now that we have basically confirmed glob pattern matching will not be used, should the file be removed? It would still exist in git history and we will be able to recover it if needs be.

Another option would be to repurpose the file to assist with other issues like #822. We shouldn't be keeping dead code unless there's a good reason.

[aryanjassal]

Polykey/src/vaults/utils.ts

Lines 54 to 68 in 72c7dfb

	async function* readDirRecursively(
	fs,
	dir = '.',
	): AsyncGenerator<string, void, void> {
	const dirEntries = await fs.promises.readdir(dir);
	for (const dirEntry of dirEntries) {
	const res = path.join(dir, dirEntry.toString());
	const stat = await fs.promises.stat(res);
	if (stat.isDirectory()) {
	yield* readDirRecursively(fs, res);
	} else if (stat.isFile()) {
	yield res;
	}
	}
	}

The type for fs parameter is any, but it's usage is discouraged. I skimmed the js-encryptedfs code but couldn't find any reference to a common base for both fs and efs. Is there a type we can use as a common type for both instead of any?

[CMCDragonkai]

Did you review this: https://github.com/MatrixAI/js-rpc?tab=readme-ov-file#specifications?

[CMCDragonkai]

Polykey/src/client/handlers/VaultsClone.ts

Lines 28 to 42 in 19e5278

	const {
	nodeId,
	}: {
	nodeId: NodeId;
	} = validateSync(
	(keyPath, value) => {
	return matchSync(keyPath)(
	[['nodeId'], () => ids.parseNodeId(value)],
	() => value,
	);
	},
	{
	nodeId: input.nodeIdEncoded,
	},
	);

This whole section of parsing nodeId can be shrunk down into this one-line solution, but we aren't using this solution. I asked Brian and he told me that using validateSync and matchSync is just a standard for parsing in handlers, but I can't help but feel that it is a huge waste.

const nodeId = ids.parseNodeId(input.nodeIdEncoded);

This single line is more concise and performant than the validateSync and matchSync block. There have been cases where I had to use the validateSync block to actually do parsing. This has mostly been a case with RawHandler RPC handlers. Using this solution everywhere seems like overkill, so shouldn't we try to simplify the code where we can?

@CMCDragonkai

No it is not equivalent. There are certain exceptions we expect from the validator and matcher.

[CMCDragonkai]

Please make sure to review the actual situation where this can cause a big problem: MatrixAI/Polykey-CLI#264 (comment).
If you're fixing it, a test must be created to prevent regression of that specific problem.

All this does is add support for cancelling RPC which are in the middle of something. This would ensure that a RPC won't keep the entire agent alive in a deadlock state.

I'm not sure how this relates to adding support for streaming progress updates back. They can still be done just fine after this PR is implemented, no?

Yes so refer to my comment here: #846 (comment).

Any progress update system SHOULD not be part of the regular RPC call, that over complicates our API and makes it harder to integrate. Remember even a unary call can trigger a long running operation on the agent side (and downstream effects magnify this due to "Call Amplification").

All progress updates depend on the measurement point. A client may measure between calls it makes by chunking up its calls. An example is like MULTIPLE RPC call or a client stream that streams chunks at a time expecting a synchronous response. This doesn't require the server side to necessarily have a separate protocol to give progress updates, it's implicit to the algorithm which makes it elegant.

On the other hand, let's say you want to "break up" a long running async operation to get progress updates, rather than building a separate protocol for that, you could try to just do chunking of the request.

On the other hand, in some cases it really doesn't make sense to chunk the request, since the request is spatially/structurally small on the client side, it just takes a long time on the server side. You don't necessarily want to change your unary call into a server streaming call cause that changes the API and makes it needlessly complex for users who just want to do a regular call.

So in those scenarios, you introduce out-of-band RPC calls to get this information. This is where #444 comes into play. We can make use of a generalized push outlet, and stream information from there, that allows clients at their discretion to get extra progress information from a separate system.

Another way is to introduce a streaming variant of the same call, this enables clients again to use a more complex API if they want the progress update version of the same call. This is common in functional programming where one creates f and f' as part of their API.

[CMCDragonkai]

isomorphic-git/isomorphic-git#1867

isomorphic-git does not support cancellation and abort signals. This means that there is no native way to cancel a long-running operation, like git clone.

My idea is to use a deconstructed promise and reject the promise when the context sends an abort signal, but I'm not sure how this would affect other things, especially the underlying code in isomorphic-git.

So isogit is a third party library. We often come across third party library problems, if it is structurally a problem, we fork them and implement it ourselves - we can do so by creating js-virtualgit.

However I remember that in the case of isogit, we actually hacked into the HTTP stream itself, where the HTTP stream is virtualised ontop of our RPC stream protocol.

That would mean, you could implement a sort of abortion at the raw RPC stream level, by aborting it at the stream.

This is because when we use isogit's features, we are actually wrapping its entire IO, so by being able to control the IO we can inject an abortion at the stream rather than relying on the internal mechanism of isogit to do so. This is absolutely possible but you need to diagram out a block diagram of the wrapped structure to demonstrate.

[aryanjassal]

While trying to implement cancellation checks, I tried to do the following. This, however, does not work. I get the following error.

test('should fail when cancelled', async () => {
  const response = await rpcClient.methods.vaultsSecretsRemove();
  response.cancel('reason');

  const consumeP = async () => {
    for await (const _ of response.readable);
  };

  await expect(consumeP()).rejects.toThrow('reason');
});

this.readableCancel is not a function
TypeError: this.readableCancel is not a function
    at Object.cancel (/home/aryanj/Projects/polykey/node_modules/@matrixai/ws/src/WebSocketStream.ts:336:10)
    at Object.cancel (/home/aryanj/Projects/polykey/tests/client/handlers/vaults.test.ts:2474:14)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Then I started investigating this by going in deeper into js-ws code. The error pointed me to a line where the this.cancel() was defined. There, it tried to execute this.readableCancel() and this.writableAbort(). If I threw the two lines in a try/catch and ignored the error, then the program fails later down the line, which would be kinda expected.

I then printed out the contents of this, and I got interesting results. The first this doesn't have the data that it should and doesn't even look like a websocket object.

{
  readable: ReadableStream { locked: false, state: 'readable', supportsBYOB: false },
  writable: WritableStream { locked: false, state: 'writable' },
  cancel: [Function: cancel],
  meta: {
    localHost: '127.0.0.1',
    localPort: 40916,
    remoteHost: '127.0.0.1',
    remotePort: 45769,
    localCACertsChain: [],
    localCertsChain: [],
    remoteCertsChain: [
      <Buffer 30 82 02 ca 30 82 02 7c a0 03 02 01 02 02 10 06 78 0d 22 8f ef 70 00 ba aa e2 75 31 93 1e 17 30 05 06 03 2b 65 70 30 40 31 3e 30 3c 06 03 55 04 03 13 ... 668 more bytes>
    ],
    command: 'vaultsSecretsRemove'
  }
}

Then, after this, another console.log statement is returned. This happens due to the post-test hook closing the websocket client. I also put console.log statements throughout the actual this.readerClose() and confirmed that the whole thing was working just fine and executed end-to-end.

<ref *1> WebSocketStream {
  initiated: 'local',
  streamId: 0n,
  encodedStreamId: <Buffer 00>,
  readable: ReadableStream { locked: true, state: 'readable', supportsBYOB: false },
  writable: WritableStream { locked: true, state: 'writable' },
  logger: Logger {
    key: 'WebSocketStream 0',
    level: 0,
    filter: undefined,
    keys: 'vaultsSecretsRemove test.WebSocketClient.WebSocketConnection 0.WebSocketStream 0',
    handlers: Set(0) {},
    parent: Logger {
      key: 'WebSocketConnection 0',
      level: 0,
      filter: undefined,
      keys: 'vaultsSecretsRemove test.WebSocketClient.WebSocketConnection 0',
      handlers: Set(0) {},
      parent: [Logger]
    }
  },
  connection: WebSocketConnection {
    type: 'client',
    connectionId: 0,
    streamMap: Map(1) { 0n => [Circular *1] },
    usedIdSet: Set(0) {},
    logger: Logger {
      key: 'WebSocketConnection 0',
      level: 0,
      filter: undefined,
      keys: 'vaultsSecretsRemove test.WebSocketClient.WebSocketConnection 0',
      handlers: Set(0) {},
      parent: [Logger]
    },

[......]

After going through the entire object, I can confirm that this.readableCancel and this.writableAbort does not exist on this object. I might need to have a chat about this with Brian as I am unfamiliar with the web code.

[CMCDragonkai]

It looks like the this context might be lost. You may need to find where to do an explicit binding.

[CMCDragonkai]

@tegefaulkes have you reviewed this?

[CMCDragonkai]

BTW make sure you don't leave any vestigial comments like TODOs.

[aryanjassal]

There are some observations that I have made. We are not consistent about parameter order. In some places ctx is the last parameter and a callback is the first, in other places its the opposite. This is apparent in VaultInternal.writeF and VaultInternal.writeG where the callback is the first parameter and the other optional parameters are lower down. The same is applicable for VaultManager.withVaults, where the callback comes first, then optional parameters. However, in the case of withF pattern for resource usage, all the parameters come before the callback as it is easier to miss additional parameters after a callback. On the other hand, putting optional parameters after the callback ensures better backwards compatibility with the fact that adding a new parameter won't break the usage of the callback everywhere else.

After a discussion with Brian, this is what we want to do - keep the callback as the last parameter. However, our code uses both the approaches in a mixed manner. Should we be concerned about it, or leave it as-is? A change would involve a large amount of minor modifications of the codebase for both Polykey and Polykey CLI.

What do you think, @CMCDragonkai?

[aryanjassal]

This PR is finally ready for merging. @tegefaulkes, can you please review this PR once again, as a big amount of changes were made after your previous review.

[CMCDragonkai]

There are some observations that I have made. We are not consistent about parameter order. In some places ctx is the last parameter and a callback is the first, in other places its the opposite. This is apparent in VaultInternal.writeF and VaultInternal.writeG where the callback is the first parameter and the other optional parameters are lower down. The same is applicable for VaultManager.withVaults, where the callback comes first, then optional parameters. However, in the case of withF pattern for resource usage, all the parameters come before the callback as it is easier to miss additional parameters after a callback. On the other hand, putting optional parameters after the callback ensures better backwards compatibility with the fact that adding a new parameter won't break the usage of the callback everywhere else.

After a discussion with Brian, this is what we want to do - keep the callback as the last parameter. However, our code uses both the approaches in a mixed manner. Should we be concerned about it, or leave it as-is? A change would involve a large amount of minor modifications of the codebase for both Polykey and Polykey CLI.

What do you think, @CMCDragonkai?

You should write a spec in graph.matrix.ai.

[tegefaulkes]

Some small changes

[aryanjassal]

All the review points from @tegefaulkes have been addressed. Once he has approved changes in both Polykey and Polykey CLI, the merge can happen in both places at the same time.