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 during Close. If we close the agent before we've established a connection, then the connection can't establish and we have some connection dependent things in Close.

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

This function is also pretty confusing since it takes ctx 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 the drpc.Conn at all, so could arguably be outside the apiConnRoutineManager --- 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 the gracefulCtx or the hardCtx. As soon as we cancel the gracefulCtx, then we send disconnect to the coordinator and clients will start disconnecting their tunnels.

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

An option to using hardCtx here would be to close a.lifeCycleReported once the loop exits. (As closing hardCtx 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 the hardCtx 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 skip hardCtx here? I guess the only danger is if coordination.Close() hangs indefinitely or is very slow.

This change might also suggest it'd be sensible to handle hardCtx in runCoodinator (instead of just return <-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 actually prevent 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 👍🏻