refactor(gregor): connection identity, own ctx, and app-state gating

go/chat/deliverer.go

@@ -355,9 +355,7 @@ func (s *Deliverer) doNotRetryFailure(ctx context.Context, obr chat1.OutboxRecor
 		return 0, err, false
 	case net.Error:
 		s.Debug(ctx, "doNotRetryFailure: generic net error, reconnecting to the server: %s(%T)", berr, berr)
-		if _, rerr := s.serverConn.Reconnect(ctx); rerr != nil {
-			s.Debug(ctx, "doNotRetryFailure: failed to reconnect: %s", rerr)
-		}
+		s.serverConn.Reconnect(ctx)
 		return chat1.OutboxErrorType_OFFLINE, err, !berr.Temporary() //nolint
 	}
 	if errors.Is(err, ErrChatServerTimeout) || errors.Is(err, ErrDuplicateConnection) ||

go/chat/server.go

@@ -133,11 +133,7 @@ func (h *Server) handleOfflineError(ctx context.Context, err error,
 		case OfflineErrorKindOfflineReconnect:
 			// Reconnect Gregor if we think we are offline (and told to reconnect)
 			h.Debug(ctx, "handleOfflineError: reconnecting to gregor")
-			if _, err := h.serverConn.Reconnect(ctx); err != nil {
-				h.Debug(ctx, "handleOfflineError: error reconnecting: %s", err)
-			} else {
-				h.Debug(ctx, "handleOfflineError: success reconnecting")
-			}
+			h.serverConn.Reconnect(ctx)
 		default:
 			// Nothing to do for other errors.
 		}

go/chat/server_test.go

@@ -106,9 +106,7 @@ func (g *gregorTestConnection) GetClient() chat1.RemoteInterface {
 	return chat1.RemoteClient{Cli: g.cli}
 }
 
-func (g *gregorTestConnection) Reconnect(ctx context.Context) (bool, error) {
-	return false, nil
-}
+func (g *gregorTestConnection) Reconnect(ctx context.Context) {}
 
 func (g *gregorTestConnection) OnConnect(ctx context.Context, _ *rpc.Connection,
 	cli rpc.GenericClient, srv *rpc.Server,

go/chat/sync.go

@@ -177,6 +177,13 @@ func (s *Syncer) Connected(ctx context.Context, cli chat1.RemoteInterface, uid g
 	ctx = globals.CtxAddLogTags(ctx, s.G())
 	defer s.Trace(ctx, &err, "Connected")()
 	s.Lock()
+	// The caller cancels ctx when the connection it was made for shuts
+	// down, before it calls Disconnected, so a Connected that sees the cancel
+	// here must not mark the syncer connected after that Disconnected.
+	if err := ctx.Err(); err != nil {
+		s.Unlock()
+		return err
+	}
 	s.isConnected = true
 	// Let the Offlinables know that we are back online
 	for _, o := range s.offlinables {

go/chat/sync_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/keybase/client/go/chat/storage"
 	"github.com/keybase/client/go/chat/types"
 	"github.com/keybase/client/go/chat/utils"
+	"github.com/keybase/client/go/externalstest"
 	"github.com/keybase/client/go/kbtest"
 	"github.com/keybase/client/go/libkb"
 	"github.com/keybase/client/go/protocol/chat1"
@@ -438,6 +439,20 @@ func TestSyncerMembersTypeChanged(t *testing.T) {
 	}
 }
 
+// Connected with a ctx its connection's Shutdown has already cancelled must
+// not mark the syncer connected: the Disconnected that follows the cancel may
+// already have run.
+func TestSyncerConnectedAfterCancelIsIgnored(t *testing.T) {
+	tc := externalstest.SetupTest(t, "syncer-connected-cancel", 0)
+	defer tc.Cleanup()
+	syncer := NewSyncer(globals.NewContext(tc.G, &globals.ChatContext{}))
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+	err := syncer.Connected(ctx, nil, gregor1.UID(make([]byte, 16)), &chat1.SyncChatRes{})
+	require.False(t, syncer.IsConnected(context.Background()))
+	require.ErrorIs(t, err, context.Canceled)
+}
+
 func TestSyncerAppState(t *testing.T) {
 	ctx, world, ri2, _, sender, list := setupTest(t, 1)
 	defer world.Cleanup()

go/chat/types/interfaces.go

@@ -697,7 +697,8 @@ type (
 )
 
 type ServerConnection interface {
-	Reconnect(context.Context) (bool, error)
+	// Reconnect reconnects to the server without waiting for it.
+	Reconnect(context.Context)
 	GetClient() chat1.RemoteInterface
 }
 

go/kbhttp/manager/manager.go

@@ -118,10 +118,13 @@ func (r *Srv) monitorAppState() {
 	for {
 		<-r.G().MobileAppState.NextUpdate(state)
 		state = r.G().MobileAppState.State()
+		// Only BACKGROUND stops the server. INACTIVE is transient (control
+		// center, the app switcher, an incoming call), as gregor also treats it.
 		switch state {
-		case keybase1.MobileAppState_FOREGROUND, keybase1.MobileAppState_BACKGROUNDACTIVE:
+		case keybase1.MobileAppState_FOREGROUND, keybase1.MobileAppState_BACKGROUNDACTIVE,
+			keybase1.MobileAppState_INACTIVE:
 			r.startHTTPSrv()
-		case keybase1.MobileAppState_BACKGROUND, keybase1.MobileAppState_INACTIVE:
+		case keybase1.MobileAppState_BACKGROUND:
 			r.httpSrv.Stop()
 		}
 	}

go/kbhttp/manager/manager_test.go

@@ -0,0 +1,31 @@
+package manager
+
+import (
+	"testing"
+	"time"
+
+	"github.com/keybase/client/go/libkb"
+	"github.com/keybase/client/go/protocol/keybase1"
+	"github.com/stretchr/testify/require"
+)
+
+// Only BACKGROUND stops the server; INACTIVE keeps it up, and starts it when
+// coming back from BACKGROUND.
+func TestSrvAppState(t *testing.T) {
+	tc := libkb.SetupTest(t, "httpsrv", 1)
+	defer tc.Cleanup()
+	srv := NewSrv(tc.G)
+	require.True(t, srv.Active())
+
+	tc.G.MobileAppState.Update(keybase1.MobileAppState_INACTIVE)
+	require.Never(t, func() bool { return !srv.Active() }, 200*time.Millisecond, time.Millisecond,
+		"INACTIVE stopped the server")
+
+	tc.G.MobileAppState.Update(keybase1.MobileAppState_BACKGROUND)
+	require.Eventually(t, func() bool { return !srv.Active() }, 10*time.Second, time.Millisecond,
+		"BACKGROUND did not stop the server")
+
+	tc.G.MobileAppState.Update(keybase1.MobileAppState_INACTIVE)
+	require.Eventually(t, srv.Active, 10*time.Second, time.Millisecond,
+		"INACTIVE after BACKGROUND did not start the server")
+}

go/kbtest/chat.go

@@ -398,9 +398,7 @@ func (m ChatRemoteMockServerConnection) GetClient() chat1.RemoteInterface {
 	return m.mock
 }
 
-func (m ChatRemoteMockServerConnection) Reconnect(ctx context.Context) (bool, error) {
-	return false, nil
-}
+func (m ChatRemoteMockServerConnection) Reconnect(ctx context.Context) {}
 
 type ChatRemoteMock struct {
 	world     *ChatMockWorld