Commands that fail to connect to peer agents throw `ErrorRPCTimedOut`

[tegefaulkes]

Describe the bug

Any commands that attempt a connection such as nodes add or vaults clone could take a long time to complete, especially if we fail to connect. The RPC timeout is 15 seconds with the connection timeout being the same 15 seconds but could take multiple 15 second attempts before giving up. Worst case is about 20/3 attempts to fail a nodesFind. This will cause a RPC timeout before we can respond with the actual error.

To Reproduce

1. run polykey nodes find for a node you know you can't connect to. This will timeout with a ErrorRPCTimedOut error.

Expected behaviour

A command such as nodes add or vaults clone or any command that relies on a connection to an peer agent needs to take as long as it needs to attempt the connection and then timeout the connection with a descriptive error. The RPC should not timeout during this process.

Screenshots

Platform (please complete the following information)

• Device: NAS running Ubuntu in a VM
• OS: Ubuntu 22.04

[tegefaulkes]

The nodes ping command has a similar problem where a fail to ping results in a RPC timeout error and not the desired ping failure.

Anything that takes the full timeout provided by the RPC system will encounter this problem. This can be fixed by giving the client timeout a second or two more time than the server's timeout time. The timeout middleware however will set the server's timeout to be equal to the client's timeout. Maybe we can add the extra second or two there?

Before applying a fix we'll need to fully spec out the behaviour we want.

[CMCDragonkai]

Timeouts should not be ad-hoc manipulated like this, this would just cause confusion and all sorts of bugs down the line.

A timeout from the RPC client is the maximum amount of time the client wants to wait for a response. That's the an ironclad rule and must not change. For streams, it is the time until the first response message. And this does not affect the idleness of the underlying connection which is a transport concern.

When you do a nodes ping on the CLI. The CLI is supposed to configure what the timeout should be. You can default it to something appropriate, but then allow the user to modify it on the CLI nodes ping --client-timeout=... as an example - I don't think we have considered a full spec of the CLI special options for now.

Afterwards on the server side, the handler should take the client provided timeout, and appropriately set it to govern all of its operations (according to the timeout spec, you must then consider heterogenous timeouts or homogenous timeout composition). Thus the RPC client provided timeout would be ultimately the timeout of all operations internally.

The handler can have its own timeout too. The 2 timeouts have to be negotiated. Several strategies exist here:

• Choose the max timeout between the 2
• Choose the min timeout between the 2
• Prefer the client timeout
• Prefer the server timeout

Different behaviours depending on whether it is null or undefined. Where null means Infinity while undefined means does not exist. I forgot which one it is right now, is this documented somewhere @amydevs.

Ok so let's say the server handler takes some resolved timeout, and applies it to its own async operation. Suppose that operation is an underlying ping operation. What you want to see is that the ping operation timeout, and then throw an error back to the client side.

The client side will then say that the operation has timedout due to the ping operation.

So the challenge here is that if the RPC client timeout is "mandatory" that would imply that the client has to error out with a timeout error if no response was provided by the server side. Adjusting the server timeout by 1 second or whatever is not a good solution to this problem.

I think stepping back from this problem. It appears nodes ping command should actually use null timeout to indicate the client command will wait forever.

This is because the underlying operation does not "naturally" have a timeout. The timeout is totally arbitrary, a connection can wait forever to try to connect. Whenever I hit problems like this, I bubble up the decision making upwards.

[CMCDragonkai]

Thus I would imagine nodes ping have either undefined or null for its timeout going all the way to attempting to make a connection forever. A caller can however provide a timeout to cancel the operation.

But if this is the case, what about adding a node, what about cloning a vault? All of this seems arbitrary.

From a UX perspective would we just wait forever to clone a vault.

[CMCDragonkai]

Need to review the whole CLI spec to consider.

[tegefaulkes]

I think this has been fixed by MatrixAI/js-rpc#59. I'll do some testing to confirm it.

[tegefaulkes]

Yeah, this was already addressed by previous fixes. Just hasn't been resolved yet.

I'm adding 1 test to check for regression for vaults clone but otherwise I'll just resolve with a commit.