Allow vault efs resource acquisition to operate on multiple vaults in parallel

[aryanjassal]

Description

For handlers like VaultsSecretsCopy and VaultsSecretsMove, we need to acquire a transaction across multiple vaults. This allows for all the given operations to be completed in a single transaction, avoiding polluting the commit space.

This PR creates such pattern, to acquire the resources from a vault dynamically in a way which works across multiple vaults at the same time.

Issues Fixed

• Fixes Manage multiple transactions dynamically in the vault domain #821 (FIX ENG-423)
• Fixes VaultsSecretsRemove incorrectly reorders directories to be removed #837 (FIX ENG-450)

Tasks

• 1. Add VaultInternal.acquireRead
• 2. Add VaultInternal.acquireWrite
• 3. Add tests
• 4. Update VaultsSecretsRemove to use this instead of grouping paths by vault name.

Final checklist

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

[linear]

ENG-423 Manage multiple transactions dynamically in the vault domain

[linear]

ENG-450 `VaultsSecretsRemove` incorrectly reorders directories to be removed

[aryanjassal]

While trying to use the new resource acquisition pattern to update VaultsSecretsMkdir, I ran into an issue.

I need to send over the vault names first, before sending any data. To do this, I send a header message before sending the data. This is reflected as the type SecretsRemoveHeaderMessage | SecretIdentifierMessage. However, similar to the issue with SuccessOrErrorMessage, when the type is wrapped inside ClientRPCResponseResult or ClientRPCRequestParams, the type narrowing doesn't work properly, and only the common types are parsed into the resultant type. In this case, only the metadata and the type fields will exist on the wrapped type. When trying to pass a value into the writer, it complains of invalid type.

This was handled before using the as keyword to help typescript with type narrowing. Here, we can define a variable with the relevant type, like SecretIdentifierMessage, and pass that into the writer. That works, but is far from ideal and is unmaintainable. The type wrapper is used only to inject a metadata field into the input and the output. If we remove the type wrapper, everything works perfectly and typescript can properly narrow the types down, too.

This is a major issue, as in the future when we implement progress packets for long running async calls, then we will run into this issue, too. Previously, as this issue had limited impact, workarounds were used, but now that this issue can potentially impact a major part of the codebase, this approach needs to be reevaluated.

After having a discussion with @tegefaulkes, we happened upon some potential solutions.

A solution is to remove the wrapper and just pass in the types as-is. This will ensure the type narrowing works perfectly, but might need some additional work and casting or using the as keyword if we need to access the metadata field, and as Polykey CLI relies on the metadata to pass in the authentication information, that might break, too. This would also mean a refactor of all RPC handlers to align with this in both Polykey and Polykey CLI (where relevant). This will be a very annoying and lengthy change.

Another solution could be to update the way we define the utility wrapper. For example, instead of injecting the metadata field, we can do a type union between whatever type is expected and a type which provides the metadata field. This would help us retain the current behaviour and potentially fix the issues we are having with type narrowing.

What do you propose we do about this, @CMCDragonkai?

[CMCDragonkai]

Describe the entire problem in isolation - like give an self contained example of this problem, and there should be some type assertions that can solve this.

[aryanjassal]

The first line in the reviewed code is basically self-contained example of this problem.

type SecretIdentifierMessageTagged = {
  type: 'SecretIdentifierMessage';
  secretName: string;
}

type VaultNamesHeaderMessage = {
  type: 'VaultNamesHeaderMessage';
  vaultNames: Array<string>;
};

// This will fail because the only fields that exist on
// the new type is `metadata` and `type`, which are
// common to both types. The field `vaultNames` is
// invalid from this perspective.
let variable1: ClientRPCRequestParams<
  | SecretsRemoveHeaderMessage
  | SecretIdentifierMessage
> = {
  type: 'VaultNamesHeaderMesage', 
  vaultNames: ['invalid']
};

// This will succeed. The types are correctly formed.
let variable2:
  | SecretsRemoveHeaderMessage
  | SecretIdentifierMessage
= {
  type: 'VaultNamesHeaderMesage', 
  vaultNames: ['invalid']
};

// So, writing the valid JSON object via the writer 
// will complain, but writing the second variable 
// won't. This means that we need to update a variable 
// and pass in that variable to the writer instead.
writer.write({
  type: 'VaultNamesHeaderMesage',
  vaultNames: ['invalid'],
}); // Complains of incorrect types
writer.write(variable2); // Works

[aryanjassal]

Approaches

Removing ClientRPC* wrapper

The ClientRPC* type is a utility type which injects a metadata field into the request and response. This solution proposes removal of that wrapper to directly return the required type without wrapping. This would clean up the type and resolve the type narrowing issue.

However, doing so would see a complete inability to pass or obtain the metadata. Polykey CLI relies on this metadata to pass on authorisation data, and this would basically brick the commands, unless there is an alternative way to provide authentication. It would also involve massive refactor of the entire code base for all RPC handlers, which also seems kind of excessive for the benefits provided by this approach.

Type Unions

As the purpose of ClientRPC* is just to attach the metadata field, the request and response type itself can be unified with a type which contains the metadata.

// I'm not sure exactly how this would work. 
// This is just an example.
type ClientRPCRequestParams extends JSONObject = {
  metadata?: JSONObject;
}

type SuccessMessage = {
  success: boolean;
}

type SecretsRemoveMessage = ClientRPCRequestParams & SuccessMessage;

While this would still require the refactor in most of the code base, this seems to be the more permanent and lasting solution as compared to ignoring the metadata altogether. However, I'm not sure if this idea will even work, or how effective it would be.

Modification of the JSONRPC* type

The way js-rpc has structured its JSONRPC* wrapper type disallows for this complex type nesting. That type can be modified to something more functional. This is what I could come up with ChatGPT's help.

type JSONRPCResponseResultNew<
  T extends JSONObject,
  M extends JSONObject,
> = T extends infer U
  ? U extends { metadata?: JSONObject }
    ? {
        metadata?: JSONObject &
          JSONRPCResponseMetadata &
          Omit<U['metadata'] & M, keyof JSONRPCResponseMetadata>;
      } & Omit<U, 'metadata'>
    : U
  : never;

This new and revised type allows for proper type narrowing and can replace the JSONRPCResponseResult from js-rpc without any additional setup. However, this is very untested, and the changes might be catastrophic to some other commands or use cases. More investigation needs to be done for this solution if this is to be used.

However, this solution is the most complete and robust solution, ensuring robust typing for the RPC. This solution also needs minimal changes, as the only change is required in the js-rpc side, making this easy to implement and maintain.

Unions of ClientRPC*

While testing, I realised that if two ClientRPC* types are unioned with a single wrapped type, then type narrowing works perfectly.

// This does not work
let var1: ClientRPCRequestParams<
  | SuccessMessage 
  | ErrorMessage
> = { type: 'success', success: true };

// This works just fine
let var1: 
  | ClientRPCRequestParams<SuccessMessage>
  | ClientRPCRequestParams<ErrorMessage>
> = { type: 'success', success: true };

This solution does not require significant changes and this can be used wherever multiple types are required, making this solution easy to implement and use. This does not require massive changes to the entire code base, and only RPC handlers which take in or return multiple types can use this approach selectively.

However, this solution needs all the wrapped types to not be a union themselves. In this case, the type is simple, but in other cases where the type is more complex and defined in types.ts, this solution will fail to work. In the client domain, we currently have a SuccessOrErrorMessage type which is a union between SuccessMessage and ErrorMessage. That type will not work anymore because the types need to be isolated. This will cause a lot of code repetition, less maintainability, and higher changes of mistakes.

Conclusion

All approaches have their own pros and cons. As a temporary solution, manual unions between ClientRPC* types can be done, but for a permanent, long-term, and robust solution, the ClientRPC* wrapper type itself should be modified to support complex types.

We need to figure out a solution and implement it @tegefaulkes. Which solution do you think I should focus on?

[CMCDragonkai]

In the case of RPC level things where things are not self-contained, it is not a good idea to choose temporary fixes, you must always consider long term robust solutions as these are not self-contained situations, and it becomes very difficult to fix later in the future.

[aryanjassal]

Currently, I have switched to using this type instead of js-rpc provided one.

type JSONRPCResponseResult<
  T extends JSONObject = JSONObject,
  M extends JSONObject = JSONObject,
> = T & {
  metadata?: JSONRPCResponseMetadata &
    M &
    (T extends { metadata: infer U } ? U : {}) &
    JSONObject;
};

Writing it this way solves all the issues and simplifies the code flow. Now, there is no need to do as casting for union types because this version can automatically narrow them down, too. This type is used as follows.

type ClientRPCRequestParams<T extends JSONObject = JSONObject> =
  JSONRPCResponseResult<
    T,
    Partial<{
      authorization: string;
    }>
  >;

type ClientRPCResponseResult<T extends JSONObject = JSONObject> =
  JSONRPCResponseResult<
    T,
    Partial<{
      authorization: string;
    }>
  >;

For reference, this was the old type.

type JSONRPCRequestParams<
  T extends JSONObject = JSONObject,
  M extends JSONObject = JSONObject,
> = {
  metadata?: JSONObject &
    JSONRPCRequestMetadata &
    Omit<T['metadata'] & M, keyof JSONRPCRequestMetadata>;
} & Omit<T, 'metadata'>;

Note that the latest type is work-in-progress, and does not properly handle field conflicts from the message type. This needs to be changed and updated before this change can be merged.

[aryanjassal]

The tasks in this PR have been completed except for deciding upon a proper fix for the RPC wrapper type. For now, I have written a custom type which adds metadata field while allowing for better type narrowing. (see #847 (comment)). However, this is a temporary fix and not final.

I will need to have a discussion if there are any obvious caveats or things I am missing. Once that is no longer an issue, either the type update can be made local to Polykey (not recommended), or made available globally to js-rpc (recommended, as this type might be optionally used by Polykey CLI, and keeping a RPC type inside Polykey doesn't make sense)