Connection dropped/timed out when connecting to deployed agent #414

Activity

1. emmacasolin

   added

   bugSomething isn't workingSomething isn't working

   on Jul 13, 2022
2. emmacasolin commented on Jul 14, 2022

   

   emmacasolin

   on Jul 14, 2022

   ContributorAuthor

   More actions

   Interestingly, I'm having a lot less difficulty connecting to deployed agents on my home wifi compared to uni wifi, and I haven't had the connection dropped like it was yesterday. This may just be our agents not being able to punch through stronger NATs, so this issue may be relevant when it comes to working on #383
3. CMCDragonkai commented on Jul 14, 2022

   

   CMCDragonkai

   on Jul 14, 2022

   Member

   More actions

   The testnet isn't behind any NAT. It has public IP addresses, when connecting from local agent to the testnet, there's isn't any punch through. As soon as your local agent initiates a message, your local NAT would allow reverse packets from the same IP+port.
4. emmacasolin commented on Jul 14, 2022

   

   emmacasolin

   on Jul 14, 2022

   Last edited by emmacasolin

   ContributorAuthor

   More actions

   The testnet isn't behind any NAT. It has public IP addresses, when connecting from local agent to the testnet, there's isn't any punch through. As soon as your local agent initiates a message, your local NAT would allow reverse packets from the same IP+port.

   Yes, I meant in the case of the testnet trying to connect back to me. If the conntrack entry has expired then the testnet would not be able to reach me, if that's indeed what it's attempting to do. These connection re-attempts (coming from the testnet) are continuing for 20+ hours after my local agent was killed, probably longer, but that's the longest any of the agents showing this behaviour have been active.
5. CMCDragonkai commented on Jul 14, 2022

   

   CMCDragonkai

   on Jul 14, 2022

   Last edited by CMCDragonkai

   Member

   More actions

   Ah yes, that can happen.

   Now that I think about it. What is the reason for the testnet attempting to connect back? It has something to do with DHT queries I believe, but in that case, one must expect failures to reconnect in case your local node goes offline. Is the connection attempt solely at the proxy level (proxy connection), or at the grpc connection level (node connection)?

   Are there are other reasons? @tegefaulkes

   For punching back to your local node, you have to have #383 done too. Plus your node is pretty transient since it's both mobile on the Uni wifi network, and then the Uni network itself would have a NAT. Note that #383 isn't solely about 2 levels of NATs, but really any number of levels of NATs.

   Tailscale/tailnet is good source of inspiration for how to address some of this.
6. CMCDragonkai commented on Jul 14, 2022

   

   CMCDragonkai

   on Jul 14, 2022

   Member

   More actions

   Furthermore if reconnection attempts fail for any arbitrary node on the network, these may make very noisy logs, because there would lots of failed network connection attempts in the future as the network scales. I reckon structured logging will need to be made use of to help filter out these logs.
7. emmacasolin commented on Jul 14, 2022

   

   emmacasolin

   on Jul 14, 2022

   ContributorAuthor

   More actions

   Now that I think about it. What is the reason for the testnet attempting to connect back? It has something to do with DHT queries I believe, but in that case, one must expect failures to reconnect in case your local node goes offline. Is the connection attempt solely at the proxy level (proxy connection), or at the grpc connection level (node connection)?

   While my agent is online the connection attempts are at the grpc connection/node connection level, but once it goes offline I believe it's only at the proxy level (since the only logs are coming from ConnectionForward).
8. tegefaulkes commented on Jul 14, 2022

   

   tegefaulkes

   on Jul 14, 2022

   Contributor

   More actions

   AFAIK the only times a node will automatically try to connect to another node is

   • initial start up discovery with syncNodeGraph.
   • discovery queue doing it's thing
   • refresh buckets queue, initially right after startup and for every bucket every hour. This will generate a lot of noise since it will do a find node operation of 255 buckets every hour. In a small network we will only find the same nodes for each operation. We still need to add some way to cancel refresh bucket operations if we're not going to find any new nodes.
   • ping node queue, should only happen in the background if a bucket is full. shouldn't be doing anything here. and I think pingNode only opens a proxy connection and not a full nodeConnection.

   If the nodes are continuously trying to connect over and over again then I'd check

   1. The queues are not the culprit. I don't know if all of then avoid duplicating when adding to the queue. should be easy to check if you add some debug logging on the agents for the queues.
   2. connections that are failing to connect are actually timing out and giving up.
9. CMCDragonkai commented on Sep 14, 2022

   

   CMCDragonkai

   on Sep 14, 2022

   Member

   More actions

   This problem is similar to #418. This may also be an unhandled promise rejection.

   I believe our PK agent should have a unhandled promise rejection handler registered at the top level. And if it receives it, should report it appropriately and also exit the program. We should have this done inside bin/polykey-agent.ts.

   At the moment there is no unhandled promise rejection handler.

   We can add this in to aid in our debugging.

   At the same time, we could also address: #307.

   See: https://nodejs.org/api/process.html#event-unhandledrejection

   We should have 2 new exceptions at the top level:

   ErrorPolykeyUnhandledRejection
   ErrorPolykeyAsynchronousDeadlock
   

   These 2 indicate software bugs, so they should use sysexits.SOFTWARE as the sysexit code.

   Actually I just remembered bin/utils/ExitHandlers. This is actually being registered for signals, but not for those problems. So the implementation of these handlers should be done in ExitHandlers.
10. CMCDragonkai

    assigned

    tegefaulkes

    on Sep 14, 2022
11. CMCDragonkai commented on Sep 14, 2022

    

    CMCDragonkai

    on Sep 14, 2022

    Member

    More actions

    I don't have a solution for this bug yet, @tegefaulkes can add the handlers, and let's see if we reproduce AFTER we have solved the other bugs #418 and #415.
12. tegefaulkes commented on Sep 16, 2022

    

    tegefaulkes

    on Sep 16, 2022

    Contributor

    More actions

    I just looked, src/bin/utils/ExitHandlers.ts:111 already registers handlers for unhandledRejection and uncaughtException. The handler prints the error to stderr and exits the process. So this is already being handled.

    Right now it will set the process exitcode to the error's code if it's an ErrorPolykey. Otherwise the code is set to 255 before exiting. It doesn't non ErrorPolykey errors into anything else. There doesn't seem to be a need for it.

    With #414 (comment) already addressed then I don't know what else to do for this issue.
13. tegefaulkes commented on Sep 16, 2022

    

    tegefaulkes

    on Sep 16, 2022

    Contributor

    More actions

    I could resolve #307 for detecting promise deadlocks.
14. CMCDragonkai commented on Sep 16, 2022

    

    CMCDragonkai

    on Sep 16, 2022

    Member

    More actions

    Yes the ExitHandler has errorHandler property being used for both unhandledRejection and uncaughtException.

    However we want to change this to rethrow a special set of exceptions: #414 (comment)

    So that it is clear to us that these are unrecoverable errors. In fact, the presence of these indicate a software bug.
15. CMCDragonkai commented on Sep 16, 2022

    

    CMCDragonkai

    on Sep 16, 2022

    Member

    More actions

    Since they are all unrecoverable errors, we can use sysexits.SOFTWARE as the software bug code.

    Regardless of whether it is an instance of ErrorPolykey or not, it should be wrapped up.

    ErrorUnhandledRejection
    ErrorUncaughtException
    ErrorAsynchronousDeadlock
    

    We should not use ErrorPolykey prefix here. These are actually not about PK at all. They are nodejs runtime problems.

    I suggest we putting all of these exceptions into the ExitHandlers class itself.
16. tegefaulkes commented on Sep 20, 2022

    

    tegefaulkes

    on Sep 20, 2022

    Contributor

    More actions

    We don't want these errors to extend ErrorPolykey we need a different error to extend. Currently ErrproPolykey is the base class we use to create all of our errors and it's the class that adds the exitCode and description properties.

    I'm going to rename ErrorPolykey to GenericError (or whatever is a better name) and have ErrorPolykey and the other errors above extend that.
17. tegefaulkes commented on Sep 20, 2022

    

    tegefaulkes

    on Sep 20, 2022

    Contributor

    More actions

    I'll hold off on that change for now until I decide the best action.

    1. Would ErrorGeneric be in it's own file separate from ErrorPolykey.ts?
    2. Would ErrorCLI (ErrorBin) extend ErrorPolykey or ErrorGeneric?
18. CMCDragonkai commented on Sep 20, 2022

    

    CMCDragonkai

    on Sep 20, 2022

    Last edited by CMCDragonkai

    Member

    More actions

    Ok well in that case, it should be ErrorBinUnhandledRejection and ErrorBinUncaughtException and ErrorBinAsynchronousDeadlock.

    And have the bin/errors be ErrorBin.
19. CMCDragonkai commented on Sep 20, 2022

    

    CMCDragonkai

    on Sep 20, 2022

    Member

    More actions

    Consider this:

    ErrorBin
    
    ErrorBinUncaughtException extends ErrorBin
    ErrorBinUnhandledRejection extends ErrorBin
    ErrorBinAsynchronousDeadlock extends ErrorBin
    
    ErrorCLI extends ErrorBin
    
    ErrorCLI...
    
20. CMCDragonkai commented on Sep 20, 2022

    

    CMCDragonkai

    on Sep 20, 2022

    Member

    More actions

    Change the 2 ErrorNode* in src/bin/errors to also be ErrorCLI....

    Also in the src/errors.ts. What are these 2:

      class ErrorInvalidId<T> extends ErrorPolykey<T> {}
      
      class ErrorInvalidConfigEnvironment<T> extends ErrorPolykey<T> {}
    
21. CMCDragonkai commented on Sep 20, 2022

    

    CMCDragonkai

    on Sep 20, 2022

    Member

    More actions

    Those last 2 errors, nothing is using those. Must have been forgotten, can be removed.
22. tegefaulkes

    added 3 commits that reference this issue on Sep 20, 2022

    fix: fixes to errors and adding un-recoverable error handlers

    ...

    f6fe013

    fix: fixes to errors and adding un-recoverable error handlers

    ...

    ed47010

    fix: fixes to errors and adding un-recoverable error handlers

    ...

    ca2c966
23. CMCDragonkai

    closed this as completedin #445on Sep 21, 2022
24. CMCDragonkai

    added

    r&d:polykey:core activity 4End to End Networking behind Consumer NAT DevicesEnd to End Networking behind Consumer NAT Devices

    on Jul 10, 2023
25. CMCDragonkai

    added a parent issue on Oct 23, 2025

    • Integration Tests for testnet.polykey.com Polykey-CLI#71