ipn/ipnlocal: move resetDialPlan to after cc.Logout() returnsnickkhyl/fix-dialplan-resets

We shouldn't be resetting the current dial plan until cc.Logout() has returned,
so we move the resetDialPlan() call accordingly.

We also update a comment that become inaccurate sometime since 2020.

Finally, we add a resetDialPlan() call when the current profile is deleted.
It results in a switch to a new, empty profile and resets the state machine,
so the dial plan should be reset as well.

Updates #cleanup
Updates #11938

Signed-off-by: Nick Khyl <nickk@tailscale.com>

1 files changed, 11 insertions, 8 deletions

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index ef5ec267f..ee5a99edf 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -5962,17 +5962,14 @@ func (b *LocalBackend) Logout(ctx context.Context) error {
 	}
 	// b.mu is now unlocked, after editPrefsLockedOnEntry.
 
-	// Clear any previous dial plan(s), if set.
-	b.resetDialPlan()
-
 	if cc == nil {
-		// Double Logout can happen via repeated IPN
-		// connections to ipnserver making it repeatedly
-		// transition from 1->0 total connections, which on
-		// Windows by default ("client mode") causes a Logout
-		// on the transition to zero.
 		// Previously this crashed when we asserted that c was non-nil
 		// here.
+		// It can be nil due to atypical locking patterns in LocalBackend.
+		// For example, the old cc can be set to nil while holding b.mu,
+		// then b.mu is unlocked before calling [LocalBackend.Start],
+		// which re-acquires the lock and sets a new cc.
+		// If Logout is called in between, cc can be nil.
 		return errors.New("no controlclient")
 	}
 
@@ -5983,6 +5980,9 @@ func (b *LocalBackend) Logout(ctx context.Context) error {
 	unlock = b.lockAndGetUnlock()
 	defer unlock()
 
+	// Clear any previous dial plan(s), if set.
+	b.resetDialPlan()
+
 	if err := b.pm.DeleteProfile(profile.ID()); err != nil {
 		b.logf("error deleting profile: %v", err)
 		return err
@@ -7334,6 +7334,9 @@ func (b *LocalBackend) DeleteProfile(p ipn.ProfileID) error {
 	if !needToRestart {
 		return nil
 	}
+	// Deleting the current profile switches to a new, empty one.
+	// Its ControlURL hasn't been set yet, so we conservatively reset the dialPlan.
+	b.resetDialPlan()
 	return b.resetForProfileChangeLockedOnEntry(unlock)
 }