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

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?

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.

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.

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.

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?

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

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.

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.

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

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?

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.

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.

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?

      // 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?

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.

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.

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.

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?

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

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.

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.

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.

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

Obviously ctx.signal.throwIfAborted() is better choice here.

However beware that this only works synchronously, when dealing with asynchronous operations, you either want to propagate that signal downwards, so that all asynchronous loop steps into a check of the signal property. Sometimes that is not available, so you have to integrate it into the chunked loop step.

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?

I don't understand this, js-rpc must take an input JSON object. There's no choice around here. The ctx is part of the meta-protocol that's why it's not part of the input JSON object message.

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?

You should make it explicit. Sometimes people are lazy. Don't propagate laziness.

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.

1. See Add cancellation to RPC handlers #846 (comment)
2. See Add cancellation to RPC handlers #846 (comment)
3. Yes see the tests in js-contexts. Also update the readme of js-contexts with this understanding.
4. Why would you do that, just make a proper context object, go to js-contexts tests to see how to do that.
5. Yes generally tran is outside the input object. However ctx should always be the last parameter. There should be examples of doing this in other parts of the codebase.
6. Becareful with stream IO manipulation, WebStreams have their own sets of logic that you need to understand. Both js-ws and js-quic make use of webstreams to standardise stream logic. You have to prototype the code here and show different examples to really understand.

I think this PR is lacking consideration/discussion of the timeouts/timers. Beware of how we expect timeouts to work.

You must read the README.md of js-rpc on the timeouts section and do some reflection there.

If MatrixAI/Polykey-CLI#264 (comment) is not fixed by this PR, then this PR is not sufficient. You must target the fixing of MatrixAI/Polykey-CLI#264 (comment).

If MatrixAI/Polykey-CLI#264 (comment) is not fixed by this PR, then this PR is not sufficient. You must target the fixing of MatrixAI/Polykey-CLI#264 (comment).

This specific PR lays down the groundwork to allow this. However, adding RPC progress updates is out of scope for this particular PR. I can work on that in a new PR after I close this one.

I have noticed inside VaultsSecretsStat, we are returning information like uid, gid, mode, etc. from the handler. Things like file permissions don't actually exist in the vault, so why are we returning redundant information to the user? Shouldn't we ignore this information completely? This would also reduce noise on the output and show only the relevant information to the user.