Repository navigation
Destroyed agent shuts down remote agent performing background tasks #418
Description
Activity
Seems like an oversight. Any background operations using a connection needs to catch any problems with connections and either handle or ignore them. The
NodeConnectionManageralready handles the following errorsnodesErrors.ErrorNodeConnectionDestroyed,grpcErrors.ErrorGRPC, andagentErrors.ErrorAgentClientDestroyedand cleans up the connection removing it from the connection map. It still throws them up the chain however.With the new
TaskManager, theTaskHandlerfor interacting with node connections in the background should now just eat this exception, and fulfill normally without error.This is because it is expected that a connection may fail because the other node/agent service may go offline due to network problems... etc.
At the same time, if the exception were to bubble up, it would be logged as a warning on the
TaskManager, and then emitted to any task promise.Either we ignore the exception the task handler, OR we handle it in the task promise. The decision depends on whether we actually care to await the task promise. If it's not being awaited for, then we should be eating this exception in the task handler.
I believe in the nodes domain, we never await the task promises. @tegefaulkes says that it's not done for discovery either. So in both cases we just have the task handler consume these exceptions.
In order to prevent regressions for this. We should have an integration test for this. It is an integration test, because we don't want to know how the details of the system work. We just want to say that if another node fails, the first node should not fail. It does not matter what the first node is doing. AT ALL TIMES, the failure of a second node should not trigger a failure of the first node.
This sounds like a
tests/bintest. But this is a "multi node situation". We are talking about multiple agents here.I think we need to group our integration tests together:
tests/bin- single nodetests/integration/multi- multi node teststests/integration/...- all the other integration tests (nat tests, and testnet tests)
An idea is to use fastcheck to run random interactions between the first and second node. Send a SIGKILL to the second node randomly, and then the constraint is that the first node must still be alive and responsive.
It's going to be tricky to write a good test for this. I can have two nodes do random things and kill one of the at a random time. That's not too hard to do. However there are far to may factors here for this to really be useful. To have good coverage whenever we run the test we will need to run a lot of runs to cover a good about of scenarios. This will make it a time consuming test. If the problem depends on timing of operations then it's unlikely to recreate fail scenarios between platforms. lastly the test requires starting a node and killing it over and over again. This makes the test pretty expensive too.
Since by design the tasks system can't crash when a handler throws. I don't think this is a problem for background tasks anymore.
The other places something like this can cause a problem is.
- Any GRPC call could fail for connection reasons. In we should be handling any connection errors when making these calls.
- GRPC handlers should handle this as well. I think the already catch any errors and throw it down the connection. However I'm not sure how they handle a connection failure.
We can force a few scenarios, like when we first startup, and when we perform a call that involves a long interaction between 2 nodes. That should be enough.
Looking at the problem some more. The error here isn't really expected at the scope of the task handlers. The expectation is that
refreshBucketshouldn't throw an error. It's really a bug withfindNodeand by extensiongetClosestGlobalNodesthrowing when it shouldn't.getClosestGlobalNodesSHOULD be catching any connection errors and skipping that node during it's search. As a resultgetClosestGlobalNodesshouldn't throw and returnPromise<NodeAddress | undefined>.Seems like
getClosestGlobalNodesis doing 2 things that should in the future be refactored to be some sort of mutual recursion. That way one can resolve a node ID to node address, while also updating the node graph in the process. The naming can then be changed toupdateClosestGlobalNodeswhich would returnPromise<void>.To check for a connection error we check for
e instanceof nodesErrors.ErrorNodeConnectionDestroyed || e instanceof grpcErrors.ErrorGRPC || e instanceof agentErrors.ErrorAgentClientDestroyed
This is a little clunky, maybe I should make a
isConnectionError()utility to keep the logic of this check in one place.For the name
getClosestGlobalNodes, I don't thinkupdateClosestGlobalNodesquite fits either. Its searching the network for the target node by asking the other nodes in the network. It's adding any nodes it contacts along the way to the node graph.I think a better name would be
searchNetworkForNodeor since it's the 2nd part offindNode,findNodeFromNetwork?- added a commit that references this issue
on Sep 14, 2022 Updated description with tasks, 2-3 hours for this one.
Connection errors like this are only really a problem if they're not caught and handled at some point. The GRPC service handlers by design catch any error and send that through the connection as metadata. So in that case any user-triggered operations shouldn't be able to crash the agent.
That leaves any background tasks and parts of the code supporting normal operation that could cause this.
NodeConnectionManagerfunctions that create connections such asfindNode,pingNode,getClosestGlobalNodes,getRemoteNodeClosestNodesandsyncNodeGraphare likely suspects. I've reviewed and updated them so ensure that they don't throw in the case of a failed connection. Resulting in returning a default such as false or no data.The background tasks shouldn't throw any errors that could crash the agent unless we await the task promise. the existing task handlers don't make any connections within the handler's function but they do call the
NodeConectionManagerfunctions above. So any connection error that reaches the handler should be unexpected.Discoverydomain still needs to be converted and looked at.I gave 1-2 hours to task 2 since It seems like it would take a bit of digging to be sure if we're handling connection errors properly in all cases. but on reviewing I think most connections are triggered via a service handler at some point so they're ultimately handled properly. Any other connections are via the
NodeConnectionManagermethods and I've checked the.As for testing this. We'd need to make a test node that accepts any GRPC calls and immediately kills the connection. It would be best we can mimic a process exiting abruptly without having to keep restarting the GRPC server. That will take some experimenting. We can have variants of this where one refuses connection, times out connecting, times out returning data, mimics a process crash, etc, etc. Then we can start an agent and see if it breaks while doing nothing or attempt specific GRPC calls against it.
Figuring out these tests will be tricky and time consuming. The worst part is coming up with agents that fail the connections in specific ways.
Figuring out these tests will be tricky and time consuming. The worst part is coming up with agents that fail the connections in specific ways.
I'm thinking it's best we just do things randomly without coding specific ways of crashing. Fuzz test the crashing that is. And if we can do it randomly sufficiently enough, it should be enough.
Coding in specific ways of crashing is brittle, and ultimately will not catch things we aren't aware of. That's the whole point of fuzzing, and fast check should help here.
For the name
getClosestGlobalNodes, I don't thinkupdateClosestGlobalNodesquite fits either. Its searching the network for the target node by asking the other nodes in the network. It's adding any nodes it contacts along the way to the node graph.I think a better name would be
searchNetworkForNodeor since it's the 2nd part offindNode,findNodeFromNetwork?Yea I'm not in favour of renaming unless the function itself gets refactored via functional decomposition so that the "mutual recursion" and side-effects is explicit and separated. But that's not a priority atm.
Try with
CommandPingfirst. So basically 2 agents, get agent 1 to ping agent 2. Randomly kill agent 2.If that works, we use
CommandClaim.Use the test that is using 2 nodes for the
syncNodeGraphtest. Then just kill the second node.I created a new issue MatrixAI/Polykey-CLI#8 relating to this.
- added a parent issue
on Oct 23, 2025
Describe the bug
We have several asynchronous background queues, some of which involve establishing grpc connections (node connections) with remote agents. However, if the remote agent we're contacting is destroyed during connection establishment then this will shut down our own agent.
Example:
To Reproduce
The timing is quite finicky, but you just need two agents running and you need to kill one of them at the time when the other is doing something like
Expected behavior
For operations that are occurring in the background, potentially even without the user being aware of them, this should not cause the agent to shut down. While this behaviour makes sense for an operation the user chose to initiate, background tasks that the user has no control over should not be able to kill the agent,
Tasks
findNode,pingNode,getClosestGlobalNodes,getRemoteNodeClosestNodesandsyncNodeGraphcan't throw an error due to a connection error. 1 hour