I'm a bit worried about the complexity of what's going on here (muddy colored functions). We're inside a routine belonging to the api conn manager, but within it starting a routine that's to be tracked on a higher level.

This is definitely due for a refactor, but it's probably fine to keep it like this for now.

Maybe we should separate out all the script stuff into its own service/loop that performs the appropriate execution based on agent events.

Yeah, I think separating the script stuff out along with the logging into a neat subcomponent would be useful. Part of the problem is the "Manifest" is a grab bag of different things, so it's harder to make it well-encapsulated sub-components.

Not for this PR though. I want to finish off the v2 Agent API before we contemplate a v2.1!

I feel like this context may be problematic duringClose. If we close the agent before we've established a connection, then the connection can't establish and we have some connection dependent things inClose.

Why should we limit tailnet to anything except "hardCtx"?

This function is also pretty confusing since it takesctx as an argument, but uses graceful here.

Yeah, this whole thing needs a bigger refactor like you suggested above. This routine doesn't directly use thedrpc.Conn at all, so could arguably be outside theapiConnRoutineManager --- but it does rely on the manifest, and several other routines block on the network being available. Fixing that is more than I wanted to bite off.

In terms of the context, I don't really think it matters whether we use thegracefulCtx or thehardCtx. As soon as we cancel thegracefulCtx, then we send disconnect to the coordinator and clients will start disconnecting their tunnels.

If we close before we'veever established a tailnet, the I don't think anything inClose() can block on it. We'd never have any SSH sessions, and would never have connected to the Coordinator.

An option to usinghardCtx here would be to closea.lifeCycleReported once the loop exits. (As closinghardCtx would signal the loop to close.) This ensures we don't leave the routine behind.

I guess it would be fine to use the channel close to indicate a timeout, but I don't want to wait for the channel to close in the success case, since that's a chicken-and-egg problem (we wait until the final state is reported to close thehardCtx in the success case).

Since it only enforces that the goroutine exits in the failure case, I don't think it's a worthwhile refactor.

While always checking the context is a good way to ensure we actually do exit, it creates a lot of paths that short-circuit execution, potentially leaving things behind. The state is also harder to reason about as there are multiple branches.

That is to say, I think it would be OK to skiphardCtx here? I guess the only danger is ifcoordination.Close() hangs indefinitely or is very slow.

This change might also suggest it'd be sensible to handlehardCtx inrunCoodinator (instead of justreturn <-errCh).

The that arm of the select is mostly about logging when we hit a timeout.

I take your point from earlier discussions that relying solely on context to tear down goroutines can cause us to exit before finalizer actions have run. Here we are explicitly checking the status of a finalizer.

We're definitely not doing a complete job of tracking goroutines in the agent, but I'm not convinced that should be a goal unto itself. Tracking and waiting for errant goroutines doesn't actuallyprevent leaks. If there is a leak, in testing, it causes the test to hang, whereas goleak fails much faster. In production, it will cause shutdown to hang so the cluster manager has to force-stop the agent.

Fair 👍🏻