Swartzn/fix/remote node connection handler

[swartzn]

This patch does solidifies the worker connect/connection handler behavior. The connection handler should only stop trying to reconnect to the worker when remote itself is shutting down. So I propose two alternatives with a preference for the second,

1. Redefine retry in the grpcClientHandler interface's connect function to isErrTransient. This keeps the beesync connection code the same but realigns the interface with the expected behavior. This allows us to cleanly adjust remote's worker connection handler so that there are two error types: 1) transient errors (rpc/network) and 2) non-transient errors (version/config). Keeping these error types separate allows for a fixed retry delay for the non-transient errors.

   • To see this implementation, just look at the first commit.

2. Remove retry/isErrTransient altogether from the grpcClientHandler interface's connect function. This unifies the errors so that they are all retried under the same exponential backoff mechanism.

   • To see this implementation, look at all the changes together.

What does this PR do / why do we need it?

Required for all PRs.

• Fixes the remote worker node shutdown.
• Aligns grpcClientHandler connect semantics with the expected behavior.

Related Issue(s)

Required when applicable.

Where should the reviewer(s) start reviewing this?

Only required for larger PRs when this may not be immediately obvious.

Are there any specific topics we should discuss before merging?

Not required.

What are the next steps after this PR?

Not required.

Checklist before merging:

Required for all PRs.

When creating a PR these are items to keep in mind that cannot be checked by GitHub actions:

• Documentation:
  • Does developer documentation (code comments, readme, etc.) need to be added or updated?
  • Does the user documentation need to be expanded or updated for this change?
• Testing:
  • Does this functionality require changing or adding new unit tests?
  • Does this functionality require changing or adding new integration tests?
• Git Hygiene:
  • Do commits adhere to Conventional Commits?
  • Is the commit history squashed down reasonably?

For more details refer to the Go coding standards and the pull request process.

[iamjoemccormick]

Overall LGTM, this is also much easier to follow than what I wrote originally! Only one blocker, which maybe you already considered and isn't worth addressing.

[iamjoemccormick]

todo: initially Claude flagged this as "gRPC ClientConn leaked on every failed reconnect attempt" but I pushed back because I'd never observed this happen before, and I'd expect the documentation on grpc.NewClient to explicitly call out the need to disconnect if it didn't somehow handle cleaning up connections. Below is Claude's revised findings. I still think this is worth addressing, but unless someone drops the MaxReconnectBackOff we're unlikely to accrue a meaningful number of "live orphaned connections".

gRPC ClientConn leaked on every failed reconnect attempt beegfs-go/rst/remote/internal/worker/worker.go:206 (with beesync.go:45)

BeeSyncNode.connect assigns a live *grpc.ClientConn to n.conn at beesync.go:45, then can still fail at a later step — address-discovery heartbeat (:69/:73), capability check (:88/:90), config rejected (:109), or bulk-update rejected (:120). The new waitUntilConnected retries connect() in a tight loop at worker.go:206 and never calls disconnect() between attempts — it only returns on success or context-cancel, so Handle's setOffline()/disconnect() at the bottom of the outer loop is never reached during a persistent-rejection cycle. The next iteration overwrites n.conn via beegrpc.NewClientConn, orphaning the prior connection plus its resolver/balancer goroutines and socket.

This is a genuine regression for the previously-"fatal" paths: in the old code, connect returned retry=false for config-rejected / bulk-rejected / address-discovery failures, which caused connectLoop to return and Handle to run n.disconnect() before the next attempt. (The transient RPC-error paths already leaked in the old code via continue-without-disconnect; this diff extends the leak to the previously-fatal paths.)

Failure scenario: a sync node is dial-reachable but persistently rejects config or fails capability negotiation (version skew / misconfiguration). Each retry leaks one ClientConn, accumulating unbounded — roughly one per MaxReconnectBackOff seconds (default 60s) once backoff saturates — for as long as the node stays misconfigured.

Minimal fix — disconnect before backing off in the error branch of waitUntilConnected:

if err := n.connect(config, wrUpdates, requiredFeatures); err != nil {
    if derr := n.disconnect(); derr != nil {
        n.log.Debug("error disconnecting before reconnect", zap.Error(derr))
    }
    // ...existing backoff...
}

(disconnect() is already nil-safe on n.conn — see beesync.go:132.)

Assisted-by: Claude:claude-opus-4-8

[iamjoemccormick]

nitpick: there are some stale comments referencing "fatal" errors we could cleanup.