From d140d1033e5b5f3d525f4e66cdc3a097bf4e8b95 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 09:13:13 -0400 Subject: [PATCH 01/11] fix(kbfs): don't crash on service notifications that arrive before KBFSOps is set KBFS init connects to the service (and registers its notification handlers) before it calls SetKBFSOps. A Reachability notification in that window called PushConnectionStatusChange on a nil KBFSOps, crashing the mobile app at launch with a nil dereference at addr 0x140. Guard the notification handlers and serviceLoggedIn so they skip KBFSOps work until init has set it. --- go/kbfs/libkbfs/keybase_daemon_rpc.go | 6 ++-- go/kbfs/libkbfs/keybase_daemon_rpc_test.go | 26 +++++++++++++++ go/kbfs/libkbfs/keybase_service_base.go | 38 +++++++++++++--------- go/kbfs/libkbfs/keybase_service_util.go | 11 ++++--- 4 files changed, 60 insertions(+), 21 deletions(-) diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc.go b/go/kbfs/libkbfs/keybase_daemon_rpc.go index 870765f11274..f9c9766e5aef 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc.go @@ -480,8 +480,10 @@ func (k *KeybaseDaemonRPC) FavoritesChanged(ctx context.Context, uid keybase1.UID, ) error { k.log.Debug("Received FavoritesChanged RPC.") - k.config.KBFSOps().RefreshCachedFavorites(ctx, - FavoritesRefreshModeInMainFavoritesLoop) + if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { + kbfsOps.RefreshCachedFavorites(ctx, + FavoritesRefreshModeInMainFavoritesLoop) + } return nil } diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go index 552e69a49e80..946bec053a27 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go @@ -55,6 +55,32 @@ func TestKeybaseDaemonRPCGetCurrentSessionCanceled(t *testing.T) { testRPCWithCanceledContext(t, serverConn, f) } +// Service notifications can arrive before init has called SetKBFSOps. +func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { + config := MakeTestConfigOrBust(t, "testuser") + kbfsOps := config.KBFSOps() + config.SetKBFSOps(nil) + defer func() { + config.SetKBFSOps(kbfsOps) + CheckConfigAndShutdown(context.Background(), t, config) + }() + + daemon := newKeybaseDaemonRPC(config, nil, logger.NewTestLogger(t)) + ctx := context.Background() + for _, r := range []keybase1.Reachable{ + keybase1.Reachable_YES, keybase1.Reachable_NO, + } { + require.NoError(t, daemon.ReachabilityChanged( + ctx, keybase1.Reachability{Reachable: r})) + } + require.NoError(t, daemon.FavoritesChanged(ctx, keybase1.UID(""))) + require.NoError(t, daemon.PaperKeyCached(ctx, keybase1.PaperKeyCachedArg{})) + require.NoError(t, daemon.TeamChangedByID(ctx, keybase1.TeamChangedByIDArg{ + Changes: keybase1.TeamChangeSet{Renamed: true}, + })) + require.NoError(t, daemon.TeamAbandoned(ctx, keybase1.TeamID(""))) +} + // TODO: Add tests for Favorite* methods, too. type fakeKeybaseClient struct { diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index bf7c71dee74e..d0629d24741d 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -417,11 +417,15 @@ func (k *KeybaseServiceBase) ReachabilityChanged(ctx context.Context, reachability keybase1.Reachability, ) error { k.log.CDebugf(ctx, "CheckReachability invoked: %v", reachability) - if reachability.Reachable == keybase1.Reachable_YES { - k.config.KBFSOps().PushConnectionStatusChange(GregorServiceName, nil) - } else { - k.config.KBFSOps().PushConnectionStatusChange( - GregorServiceName, errDisconnected{}) + // The service connection delivers notifications before init has called + // SetKBFSOps, so KBFSOps can still be nil here. + if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { + if reachability.Reachable == keybase1.Reachable_YES { + kbfsOps.PushConnectionStatusChange(GregorServiceName, nil) + } else { + kbfsOps.PushConnectionStatusChange( + GregorServiceName, errDisconnected{}) + } } mdServer := k.config.MDServer() if mdServer != nil { @@ -452,13 +456,15 @@ func (k *KeybaseServiceBase) PaperKeyCached(ctx context.Context, k.log.CDebugf(ctx, "Paper key for %s cached", arg.Uid) if k.getCachedCurrentSession().UID == arg.Uid { - err := k.config.KBFSOps().KickoffAllOutstandingRekeys() - if err != nil { - // Ignore and log errors here. For now the only way it could error - // is when the method is called on a folderBranchOps which is a - // developer mistake and not recoverable from code. - k.log.CDebugf(ctx, - "Calling KickoffAllOutstandingRekeys error: %s", err) + if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { + err := kbfsOps.KickoffAllOutstandingRekeys() + if err != nil { + // Ignore and log errors here. For now the only way it could error + // is when the method is called on a folderBranchOps which is a + // developer mistake and not recoverable from code. + k.log.CDebugf(ctx, + "Calling KickoffAllOutstandingRekeys error: %s", err) + } } // Ignore any errors for now, we don't want to block this // notification and it's not worth spawning a goroutine for. @@ -1403,8 +1409,8 @@ func (k *KeybaseServiceBase) TeamChangedByID(ctx context.Context, arg.Changes.KeyRotated, arg.Changes.Renamed) k.setCachedTeamInfo(arg.TeamID, idutil.TeamInfo{}) - if arg.Changes.Renamed { - k.config.KBFSOps().TeamNameChanged(ctx, arg.TeamID) + if kbfsOps := k.config.KBFSOps(); arg.Changes.Renamed && kbfsOps != nil { + kbfsOps.TeamNameChanged(ctx, arg.TeamID) } return nil } @@ -1454,7 +1460,9 @@ func (k *KeybaseDaemonRPC) TeamAbandoned( ) error { k.log.CDebugf(ctx, "Implicit team %s abandoned", tid) k.setCachedTeamInfo(tid, idutil.TeamInfo{}) - k.config.KBFSOps().TeamAbandoned(ctx, tid) + if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { + kbfsOps.TeamAbandoned(ctx, tid) + } return nil } diff --git a/go/kbfs/libkbfs/keybase_service_util.go b/go/kbfs/libkbfs/keybase_service_util.go index 818714f4a9ea..5e7552592fa4 100644 --- a/go/kbfs/libkbfs/keybase_service_util.go +++ b/go/kbfs/libkbfs/keybase_service_util.go @@ -125,11 +125,14 @@ func serviceLoggedIn(ctx context.Context, config Config, session idutil.SessionI go bServer.RefreshAuthToken(context.Background()) } - if config.Mode().DoRefreshFavoritesOnInit() { - config.KBFSOps().RefreshCachedFavorites( - ctx, FavoritesRefreshModeInMainFavoritesLoop) + // CurrentSession can land here before init has called SetKBFSOps. + if kbfsOps := config.KBFSOps(); kbfsOps != nil { + if config.Mode().DoRefreshFavoritesOnInit() { + kbfsOps.RefreshCachedFavorites( + ctx, FavoritesRefreshModeInMainFavoritesLoop) + } + kbfsOps.PushStatusChange() } - config.KBFSOps().PushStatusChange() config.ResetForLogin(ctx, session.Name) From eb558655bb42ecaaf1745630bd8663cba1679d4d Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 09:29:19 -0400 Subject: [PATCH 02/11] fix(kbfs): defer the login/logout flows until KBFSOps is set serviceLoggedOut and serviceLoggedIn (via CurrentSession, including its async setHomeTlfIdsForDbcAndFavorites) also dereference KBFSOps, and logout touches Chat, which is set late too. Skip logout when KBFSOps is unset (nothing is cached yet), and for login forget the session instead so the first lookup after init runs the logged-in flow. --- go/kbfs/libkbfs/keybase_daemon_rpc_test.go | 18 ++++++++++++++++++ go/kbfs/libkbfs/keybase_service_base.go | 10 ++++++++-- go/kbfs/libkbfs/keybase_service_util.go | 16 +++++++++------- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go index 946bec053a27..9f8b0c78e3ae 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go @@ -65,7 +65,16 @@ func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { CheckConfigAndShutdown(context.Background(), t, config) }() + name := kbname.NormalizedUsername("fake username") + session := idutil.SessionInfo{ + Name: name, + UID: keybase1.MakeTestUID(1), + CryptPublicKey: idutil.MakeLocalUserCryptPublicKeyOrBust(name), + VerifyingKey: idutil.MakeLocalUserVerifyingKeyOrBust(name), + } + client := &fakeKeybaseClient{session: session} daemon := newKeybaseDaemonRPC(config, nil, logger.NewTestLogger(t)) + daemon.fillClients(client) ctx := context.Background() for _, r := range []keybase1.Reachable{ keybase1.Reachable_YES, keybase1.Reachable_NO, @@ -79,6 +88,15 @@ func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { Changes: keybase1.TeamChangeSet{Renamed: true}, })) require.NoError(t, daemon.TeamAbandoned(ctx, keybase1.TeamID(""))) + require.NoError(t, daemon.LoggedOut(ctx)) + + // The logged-in flow is deferred, not dropped: the session stays + // uncached until KBFSOps is set, and the next lookup runs it. + testCurrentSession(t, client, daemon, session, expectCall) + testCurrentSession(t, client, daemon, session, expectCall) + config.SetKBFSOps(kbfsOps) + testCurrentSession(t, client, daemon, session, expectCall) + testCurrentSession(t, client, daemon, session, expectCached) } // TODO: Add tests for Favorite* methods, too. diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index d0629d24741d..2d26055b1f6d 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -1190,8 +1190,14 @@ func (k *KeybaseServiceBase) CurrentSession( } if newSession && k.config != nil { - // Don't hold the lock while calling `serviceLoggedIn`. - _ = serviceLoggedIn(ctx, k.config, s, TLFJournalBackgroundWorkEnabled) + if k.config.KBFSOps() == nil { + // Init hasn't called SetKBFSOps yet, and the logged-in flow needs + // it. Forget the session so the first lookup after init runs it. + k.setCachedCurrentSession(idutil.SessionInfo{}) + } else { + // Don't hold the lock while calling `serviceLoggedIn`. + _ = serviceLoggedIn(ctx, k.config, s, TLFJournalBackgroundWorkEnabled) + } } return s, nil diff --git a/go/kbfs/libkbfs/keybase_service_util.go b/go/kbfs/libkbfs/keybase_service_util.go index 5e7552592fa4..99acb5eea7f9 100644 --- a/go/kbfs/libkbfs/keybase_service_util.go +++ b/go/kbfs/libkbfs/keybase_service_util.go @@ -125,14 +125,11 @@ func serviceLoggedIn(ctx context.Context, config Config, session idutil.SessionI go bServer.RefreshAuthToken(context.Background()) } - // CurrentSession can land here before init has called SetKBFSOps. - if kbfsOps := config.KBFSOps(); kbfsOps != nil { - if config.Mode().DoRefreshFavoritesOnInit() { - kbfsOps.RefreshCachedFavorites( - ctx, FavoritesRefreshModeInMainFavoritesLoop) - } - kbfsOps.PushStatusChange() + if config.Mode().DoRefreshFavoritesOnInit() { + config.KBFSOps().RefreshCachedFavorites( + ctx, FavoritesRefreshModeInMainFavoritesLoop) } + config.KBFSOps().PushStatusChange() config.ResetForLogin(ctx, session.Name) @@ -141,6 +138,11 @@ func serviceLoggedIn(ctx context.Context, config Config, session idutil.SessionI // serviceLoggedOut should be called when the current user logs out. func serviceLoggedOut(ctx context.Context, config Config) { + // A logout can arrive before init has called SetKBFSOps. Nothing has been + // cached yet then, and Chat may still be unset. + if config.KBFSOps() == nil { + return + } if jManager, err := GetJournalManager(config); err == nil { jManager.shutdownExistingJournals(ctx) } From 615957e71f53327942fcccb271d73a0e7a8b9271 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 09:34:58 -0400 Subject: [PATCH 03/11] fix(kbfs): error out of service requests before init, and cache the session exactly once The TlfKeys, FSRequest and ImplicitTeamMigration handlers share the service connection and dereference KBFSOps/MDOps too; return errKBFSNotInitialized until init has set them. Replace the after-the-fact session-cache clear with not caching the session while KBFSOps is unset. A lookup now reports a new session only if it cached it, so serviceLoggedIn runs exactly once per login even if init finishes mid-lookup. --- go/kbfs/libkbfs/keybase_daemon_rpc_test.go | 13 ++++++ go/kbfs/libkbfs/keybase_service_base.go | 49 ++++++++++++++++++---- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go index 9f8b0c78e3ae..d074e6eb3675 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go @@ -90,6 +90,19 @@ func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { require.NoError(t, daemon.TeamAbandoned(ctx, keybase1.TeamID(""))) require.NoError(t, daemon.LoggedOut(ctx)) + // Service-initiated requests get an error instead of a nil dereference. + query := keybase1.TLFQuery{TlfName: "testuser"} + _, err := daemon.GetTLFCryptKeys(ctx, query) + require.Equal(t, errKBFSNotInitialized{}, err) + _, err = daemon.GetPublicCanonicalTLFNameAndID(ctx, query) + require.Equal(t, errKBFSNotInitialized{}, err) + require.Equal(t, errKBFSNotInitialized{}, + daemon.FSEditListRequest(ctx, keybase1.FSEditListRequest{})) + require.Equal(t, errKBFSNotInitialized{}, + daemon.StartMigration(ctx, keybase1.Folder{})) + require.Equal(t, errKBFSNotInitialized{}, + daemon.FinalizeMigration(ctx, keybase1.Folder{})) + // The logged-in flow is deferred, not dropped: the session stays // uncached until KBFSOps is set, and the next lookup runs it. testCurrentSession(t, client, daemon, session, expectCall) diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index 2d26055b1f6d..cd2c2da7bea6 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -434,6 +434,18 @@ func (k *KeybaseServiceBase) ReachabilityChanged(ctx context.Context, return nil } +// errKBFSNotInitialized is returned from service-initiated requests that +// arrive before init has set up KBFSOps and MDOps. +type errKBFSNotInitialized struct{} + +func (errKBFSNotInitialized) Error() string { return "KBFS is not initialized yet" } + +// kbfsInitialized reports whether init has set the KBFSOps and MDOps that +// service-initiated requests use. The service connection is live first. +func (k *KeybaseServiceBase) kbfsInitialized() bool { + return k.config.KBFSOps() != nil && k.config.MDOps() != nil +} + // StartReachability implements keybase1.ReachabilityInterface. func (k *KeybaseServiceBase) StartReachability(ctx context.Context) (res keybase1.Reachability, err error) { return k.CheckReachability(ctx) @@ -1148,11 +1160,14 @@ func (k *KeybaseServiceBase) getCurrentSession( } var s idutil.SessionInfo + cache := true // Close and clear the in-progress channel, even on an error. defer func() { k.sessionCacheLock.Lock() defer k.sessionCacheLock.Unlock() - k.cachedCurrentSession = s + if cache { + k.cachedCurrentSession = s + } close(k.sessionInProgressCh) k.sessionInProgressCh = nil }() @@ -1173,6 +1188,13 @@ func (k *KeybaseServiceBase) getCurrentSession( k.log.CDebugf( ctx, "new session with username %s, uid %s, crypt public key %s, and verifying key %s", s.Name, s.UID, s.CryptPublicKey, s.VerifyingKey) + // The logged-in flow needs KBFSOps, which init sets after the service + // connection is live. Until then leave the session uncached, so the first + // lookup after init is the new session that runs it. + if k.config != nil && k.config.KBFSOps() == nil { + cache = false + return s, false, nil + } return s, true, nil } @@ -1190,14 +1212,8 @@ func (k *KeybaseServiceBase) CurrentSession( } if newSession && k.config != nil { - if k.config.KBFSOps() == nil { - // Init hasn't called SetKBFSOps yet, and the logged-in flow needs - // it. Forget the session so the first lookup after init runs it. - k.setCachedCurrentSession(idutil.SessionInfo{}) - } else { - // Don't hold the lock while calling `serviceLoggedIn`. - _ = serviceLoggedIn(ctx, k.config, s, TLFJournalBackgroundWorkEnabled) - } + // Don't hold the lock while calling `serviceLoggedIn`. + _ = serviceLoggedIn(ctx, k.config, s, TLFJournalBackgroundWorkEnabled) } return s, nil @@ -1352,6 +1368,9 @@ func (k *KeybaseServiceBase) FSEditListRequest(ctx context.Context, k.log) k.log.CDebugf(ctx, "Edit list request for %s (public: %t)", req.Folder.Name, !req.Folder.Private) + if !k.kbfsInitialized() { + return errKBFSNotInitialized{} + } tlfHandle, err := getHandleFromFolderName( ctx, k.config.KBPKI(), k.config.MDOps(), k.config, req.Folder.Name, !req.Folder.Private) @@ -1502,6 +1521,9 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, if mdServer == nil { return errors.New("no mdserver") } + if !k.kbfsInitialized() { + return errKBFSNotInitialized{} + } // Making a favorite here to reuse the code that converts from // `keybase1.FolderType` into `tlf.Type`. fav := favorites.NewFolderFromProtocol(folder) @@ -1526,6 +1548,9 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, func (k *KeybaseServiceBase) FinalizeMigration(ctx context.Context, folder keybase1.Folder, ) (err error) { + if !k.kbfsInitialized() { + return errKBFSNotInitialized{} + } fav := favorites.NewFolderFromProtocol(folder) handle, err := GetHandleFromFolderNameAndType( ctx, k.config.KBPKI(), k.config.MDOps(), k.config, fav.Name, fav.Type) @@ -1563,6 +1588,9 @@ func (k *KeybaseServiceBase) GetTLFCryptKeys(ctx context.Context, return keybase1.GetTLFCryptKeysRes{}, err } + if !k.kbfsInitialized() { + return res, errKBFSNotInitialized{} + } tlfHandle, err := getHandleFromFolderName( ctx, k.config.KBPKI(), k.config.MDOps(), k.config, query.TlfName, false) if err != nil { @@ -1607,6 +1635,9 @@ func (k *KeybaseServiceBase) GetPublicCanonicalTLFNameAndID( return keybase1.CanonicalTLFNameAndIDWithBreaks{}, err } + if !k.kbfsInitialized() { + return res, errKBFSNotInitialized{} + } tlfHandle, err := getHandleFromFolderName( ctx, k.config.KBPKI(), k.config.MDOps(), k.config, query.TlfName, true /* public */) From 9fba71aa0c1aacc9213dcca927c78cc646d6bb90 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 09:59:09 -0400 Subject: [PATCH 04/11] fix(kbfs): gate SimpleFS/git/fs requests on init, and share one readiness check SimpleFS, KBFSGit and Fs are registered on the same service connection as the KBFS handlers, so their requests can also arrive before init has set KBFSOps, MDOps, Notifier and the servers. Wrap every method of those additional protocols so it waits until init has set them, bounded by the caller's context (as SimpleFS's getKBPKI already does for KBPKI). Requests run on their own goroutines, so waiting doesn't block the connection. Replace the per-site checks with kbfsOpsReady (KBFSOps and MDOps) for the login/logout flows, since serviceLoggedIn reaches MDOps and init sets it just after KBFSOps, and kbfsServersReady (plus the key and block servers) for service-initiated requests that fetch keys or blocks. --- go/kbfs/libkbfs/keybase_daemon_rpc.go | 48 +++++++++++++++++++- go/kbfs/libkbfs/keybase_daemon_rpc_test.go | 53 +++++++++++++++++++++- go/kbfs/libkbfs/keybase_service_base.go | 35 ++++++++------ go/kbfs/libkbfs/keybase_service_util.go | 6 +-- 4 files changed, 122 insertions(+), 20 deletions(-) diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc.go b/go/kbfs/libkbfs/keybase_daemon_rpc.go index f9c9766e5aef..d06faaf2394d 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc.go @@ -81,6 +81,52 @@ func (k *KeybaseDaemonRPC) addKBFSProtocols() { k.AddProtocols(protocols) } +const kbfsInitPollInterval = 100 * time.Millisecond + +// waitForKBFSInit wraps every method of the given protocols (SimpleFS, git, +// fs) so each request waits until init has set up KBFSOps, MDOps and the +// servers. The service connection, and so these handlers, is live before +// init sets them. Each request is served on its own goroutine, so waiting +// doesn't block the connection; the caller's context bounds the wait. +func waitForKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { + if len(protocols) == 0 { + return protocols + } + wrapped := make([]rpc.Protocol, 0, len(protocols)) + for _, p := range protocols { + methods := make(map[string]rpc.ServeHandlerDescription, len(p.Methods)) + for name, m := range p.Methods { + handler := m.Handler + m.Handler = func(ctx context.Context, arg any) (any, error) { + if err := waitForKBFSServersReady(ctx, config); err != nil { + return nil, err + } + return handler(ctx, arg) + } + methods[name] = m + } + p.Methods = methods + wrapped = append(wrapped, p) + } + return wrapped +} + +func waitForKBFSServersReady(ctx context.Context, config Config) error { + if kbfsServersReady(config) { + return nil + } + ticker := time.NewTicker(kbfsInitPollInterval) + defer ticker.Stop() + for !kbfsServersReady(config) { + select { + case <-ticker.C: + case <-ctx.Done(): + return ctx.Err() + } + } + return nil +} + // NewKeybaseDaemonRPC makes a new KeybaseDaemonRPC that makes RPC // calls using the socket of the given Keybase context. func NewKeybaseDaemonRPC(config Config, kbCtx Context, log logger.Logger, @@ -104,7 +150,7 @@ func NewKeybaseDaemonRPC(config Config, kbCtx Context, log logger.Logger, k.notifyService = newNotifyServiceHandler(config, log) k.addKBFSProtocols() - k.AddProtocols(additionalProtocols) + k.AddProtocols(waitForKBFSInit(config, additionalProtocols)) return k } diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go index d074e6eb3675..280bb3c73cec 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go @@ -58,10 +58,11 @@ func TestKeybaseDaemonRPCGetCurrentSessionCanceled(t *testing.T) { // Service notifications can arrive before init has called SetKBFSOps. func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { config := MakeTestConfigOrBust(t, "testuser") - kbfsOps := config.KBFSOps() + kbfsOps, mdOps := config.KBFSOps(), config.MDOps() config.SetKBFSOps(nil) defer func() { config.SetKBFSOps(kbfsOps) + config.SetMDOps(mdOps) CheckConfigAndShutdown(context.Background(), t, config) }() @@ -83,7 +84,10 @@ func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { ctx, keybase1.Reachability{Reachable: r})) } require.NoError(t, daemon.FavoritesChanged(ctx, keybase1.UID(""))) - require.NoError(t, daemon.PaperKeyCached(ctx, keybase1.PaperKeyCachedArg{})) + // PaperKeyCached only acts for the current session's user. + daemon.setCachedCurrentSession(session) + require.NoError(t, daemon.PaperKeyCached( + ctx, keybase1.PaperKeyCachedArg{Uid: session.UID})) require.NoError(t, daemon.TeamChangedByID(ctx, keybase1.TeamChangedByIDArg{ Changes: keybase1.TeamChangeSet{Renamed: true}, })) @@ -107,11 +111,56 @@ func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { // uncached until KBFSOps is set, and the next lookup runs it. testCurrentSession(t, client, daemon, session, expectCall) testCurrentSession(t, client, daemon, session, expectCall) + // KBFSOps alone isn't enough: init sets MDOps just after it. + config.SetMDOps(nil) config.SetKBFSOps(kbfsOps) testCurrentSession(t, client, daemon, session, expectCall) + config.SetMDOps(mdOps) + testCurrentSession(t, client, daemon, session, expectCall) testCurrentSession(t, client, daemon, session, expectCached) } +// The SimpleFS/git/fs protocols share the service connection, so their +// requests can also arrive before init has set up KBFS. +func TestWaitForKBFSInit(t *testing.T) { + config := MakeTestConfigOrBust(t, "testuser") + kbfsOps := config.KBFSOps() + config.SetKBFSOps(nil) + defer func() { + config.SetKBFSOps(kbfsOps) + CheckConfigAndShutdown(context.Background(), t, config) + }() + + called := make(chan struct{}, 1) + protocols := waitForKBFSInit(config, []rpc.Protocol{{ + Name: "test", + Methods: map[string]rpc.ServeHandlerDescription{ + "method": {Handler: func(context.Context, any) (any, error) { + called <- struct{}{} + return nil, nil + }}, + }, + }}) + handler := protocols[0].Methods["method"].Handler + + // Not ready: the request waits until the caller gives up, and the + // handler never runs. + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + _, err := handler(ctx, nil) + require.ErrorIs(t, err, context.DeadlineExceeded) + require.Empty(t, called) + + // Becoming ready mid-wait lets the request through. + go func() { + time.Sleep(50 * time.Millisecond) + config.SetKBFSOps(kbfsOps) + }() + _, err = handler(context.Background(), nil) + require.NoError(t, err) + require.Len(t, called, 1) +} + // TODO: Add tests for Favorite* methods, too. type fakeKeybaseClient struct { diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index cd2c2da7bea6..64d943117775 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -435,15 +435,22 @@ func (k *KeybaseServiceBase) ReachabilityChanged(ctx context.Context, } // errKBFSNotInitialized is returned from service-initiated requests that -// arrive before init has set up KBFSOps and MDOps. +// arrive before init has set up the parts of config they use. type errKBFSNotInitialized struct{} func (errKBFSNotInitialized) Error() string { return "KBFS is not initialized yet" } -// kbfsInitialized reports whether init has set the KBFSOps and MDOps that -// service-initiated requests use. The service connection is live first. -func (k *KeybaseServiceBase) kbfsInitialized() bool { - return k.config.KBFSOps() != nil && k.config.MDOps() != nil +// kbfsOpsReady reports whether init has set KBFSOps and MDOps. The service +// connection, and so every handler on it, is live before init sets them. +func kbfsOpsReady(config Config) bool { + return config.KBFSOps() != nil && config.MDOps() != nil +} + +// kbfsServersReady also requires the key and block servers, which init sets +// last. Requests that fetch keys or blocks need them. +func kbfsServersReady(config Config) bool { + return kbfsOpsReady(config) && + config.KeyServer() != nil && config.BlockServer() != nil } // StartReachability implements keybase1.ReachabilityInterface. @@ -1188,10 +1195,10 @@ func (k *KeybaseServiceBase) getCurrentSession( k.log.CDebugf( ctx, "new session with username %s, uid %s, crypt public key %s, and verifying key %s", s.Name, s.UID, s.CryptPublicKey, s.VerifyingKey) - // The logged-in flow needs KBFSOps, which init sets after the service - // connection is live. Until then leave the session uncached, so the first - // lookup after init is the new session that runs it. - if k.config != nil && k.config.KBFSOps() == nil { + // The logged-in flow needs KBFSOps and MDOps, which init sets after the + // service connection is live. Until then leave the session uncached, so + // the first lookup once they're set is the new session that runs it. + if k.config != nil && !kbfsOpsReady(k.config) { cache = false return s, false, nil } @@ -1368,7 +1375,7 @@ func (k *KeybaseServiceBase) FSEditListRequest(ctx context.Context, k.log) k.log.CDebugf(ctx, "Edit list request for %s (public: %t)", req.Folder.Name, !req.Folder.Private) - if !k.kbfsInitialized() { + if !kbfsServersReady(k.config) { return errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( @@ -1521,7 +1528,7 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, if mdServer == nil { return errors.New("no mdserver") } - if !k.kbfsInitialized() { + if !kbfsServersReady(k.config) { return errKBFSNotInitialized{} } // Making a favorite here to reuse the code that converts from @@ -1548,7 +1555,7 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, func (k *KeybaseServiceBase) FinalizeMigration(ctx context.Context, folder keybase1.Folder, ) (err error) { - if !k.kbfsInitialized() { + if !kbfsServersReady(k.config) { return errKBFSNotInitialized{} } fav := favorites.NewFolderFromProtocol(folder) @@ -1588,7 +1595,7 @@ func (k *KeybaseServiceBase) GetTLFCryptKeys(ctx context.Context, return keybase1.GetTLFCryptKeysRes{}, err } - if !k.kbfsInitialized() { + if !kbfsServersReady(k.config) { return res, errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( @@ -1635,7 +1642,7 @@ func (k *KeybaseServiceBase) GetPublicCanonicalTLFNameAndID( return keybase1.CanonicalTLFNameAndIDWithBreaks{}, err } - if !k.kbfsInitialized() { + if !kbfsServersReady(k.config) { return res, errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( diff --git a/go/kbfs/libkbfs/keybase_service_util.go b/go/kbfs/libkbfs/keybase_service_util.go index 99acb5eea7f9..abe632f126f5 100644 --- a/go/kbfs/libkbfs/keybase_service_util.go +++ b/go/kbfs/libkbfs/keybase_service_util.go @@ -138,9 +138,9 @@ func serviceLoggedIn(ctx context.Context, config Config, session idutil.SessionI // serviceLoggedOut should be called when the current user logs out. func serviceLoggedOut(ctx context.Context, config Config) { - // A logout can arrive before init has called SetKBFSOps. Nothing has been - // cached yet then, and Chat may still be unset. - if config.KBFSOps() == nil { + // A logout can arrive before init has set KBFSOps and MDOps. Nothing has + // been cached yet then, and Chat may still be unset. + if !kbfsOpsReady(config) { return } if jManager, err := GetJournalManager(config); err == nil { From d2881f4c83cbf631bdcf7877de554cfb511d571b Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 10:31:15 -0400 Subject: [PATCH 05/11] fix(kbfs): set up KBFSOps before the service connection instead of guarding handlers The crashes came from init creating the service connection, which registers KBFS's handlers, before KBFSOps and MDOps existed. KBPKI, KBFSOps, KeyManager and MDOps only store config at construction, and the test config already builds them before the service, so create them first and drop the per-handler nil guards and the session-cache gate. What's still set after the service connection is live: - Chat: serviceLoggedOut nil-checks it. - The key and block servers: service-initiated requests return errKBFSNotInitialized, and SimpleFS/git/fs requests wait for them. If init fails after the service is set, shut the service connection down so the service stops routing to a half-initialized KBFS; that also cancels requests waiting for the servers. TestInitSetsUpKBFSBeforeService runs the real doInit with a fake service that delivers notifications, a login and a logout mid-init, and checks the connection is shut down when init fails. --- go/kbfs/libkbfs/init.go | 41 ++++-- go/kbfs/libkbfs/init_test.go | 151 +++++++++++++++++++++ go/kbfs/libkbfs/keybase_daemon_rpc.go | 15 +- go/kbfs/libkbfs/keybase_daemon_rpc_test.go | 66 ++------- go/kbfs/libkbfs/keybase_service_base.go | 64 +++------ go/kbfs/libkbfs/keybase_service_util.go | 11 +- 6 files changed, 225 insertions(+), 123 deletions(-) create mode 100644 go/kbfs/libkbfs/init_test.go diff --git a/go/kbfs/libkbfs/init.go b/go/kbfs/libkbfs/init.go index 3457d1b217d8..89421d1ea539 100644 --- a/go/kbfs/libkbfs/init.go +++ b/go/kbfs/libkbfs/init.go @@ -797,8 +797,23 @@ func doInit( kbfsLog := config.MakeLogger("") - // Initialize Keybase service connection. This needs to happen before - // KBPKI client. + // Initialize KBPKI client (needed for KBFSOps, MD Server, and Chat). It + // reaches the service through config, so it doesn't need it yet. + k := NewKBPKIClient(config, kbfsLog) + config.SetKBPKI(k) + + // Set up KBFSOps and MDOps before the service connection. Creating the + // connection registers the KBFS handlers, and the service can call them + // right away. None of these use the service until they're called. + initDoneCh := make(chan struct{}) + kbfsOps := NewKBFSOpsStandard(kbCtx, config, initDoneCh) + defer close(initDoneCh) + config.SetKBFSOps(kbfsOps) + config.SetNotifier(kbfsOps) + config.SetKeyManager(NewKeyManagerStandard(config)) + config.SetMDOps(NewMDOpsStandard(config)) + + // Initialize Keybase service connection. if keybaseServiceCn == nil { keybaseServiceCn = keybaseDaemon{} } @@ -811,10 +826,15 @@ func doInit( service = NewKeybaseServiceMeasured(service, registry) } config.SetKeybaseService(service) - - // Initialize KBPKI client (needed for KBFSOps, MD Server, and Chat). - k := NewKBPKIClient(config, kbfsLog) - config.SetKBPKI(k) + // If init fails from here on, close the connection so the service stops + // routing to this half-initialized KBFS. That also cancels requests + // waiting in waitForKBFSInit. + initSucceeded := false + defer func() { + if !initSucceeded { + service.Shutdown() + } + }() // Initialize Chat client (for file edit notifications). chat, err := keybaseServiceCn.NewChat(config, params, kbCtx, kbfsLog) @@ -823,14 +843,6 @@ func doInit( } config.SetChat(chat) - initDoneCh := make(chan struct{}) - kbfsOps := NewKBFSOpsStandard(kbCtx, config, initDoneCh) - defer close(initDoneCh) - config.SetKBFSOps(kbfsOps) - config.SetNotifier(kbfsOps) - config.SetKeyManager(NewKeyManagerStandard(config)) - config.SetMDOps(NewMDOpsStandard(config)) - config.SetDiskBlockCacheFraction(getCacheFrac( ctx, kbCtx, params.DiskBlockCacheFraction, defaultDiskBlockCacheFraction, configBlockCacheDiskMaxFracStr, log)) @@ -995,6 +1007,7 @@ func doInit( go cleanOldTempStorageRoots(config) } + initSucceeded = true return config, nil } diff --git a/go/kbfs/libkbfs/init_test.go b/go/kbfs/libkbfs/init_test.go new file mode 100644 index 000000000000..4ec4c63500fc --- /dev/null +++ b/go/kbfs/libkbfs/init_test.go @@ -0,0 +1,151 @@ +// Copyright 2026 Keybase Inc. All rights reserved. +// Use of this source code is governed by a BSD +// license that can be found in the LICENSE file. + +package libkbfs + +import ( + "context" + "errors" + "net" + "testing" + + "github.com/keybase/client/go/kbconst" + "github.com/keybase/client/go/kbfs/env" + "github.com/keybase/client/go/kbfs/idutil" + kbname "github.com/keybase/client/go/kbun" + "github.com/keybase/client/go/libkb" + "github.com/keybase/client/go/logger" + "github.com/keybase/client/go/protocol/keybase1" + "github.com/keybase/go-framed-msgpack-rpc/rpc" + "github.com/stretchr/testify/require" +) + +// initTestContext is the minimal Context doInit needs up to the point where +// initOrderCn fails it. +type initTestContext struct { + env.EmptyAppStateUpdater + env *libkb.Env + dataDir string +} + +var _ Context = (*initTestContext)(nil) + +var errNoSocket = errors.New("no socket in test") + +func (c *initTestContext) GetRunMode() kbconst.RunMode { return kbconst.DevelRunMode } +func (c *initTestContext) GetLogDir() string { return c.dataDir } +func (c *initTestContext) GetDataDir() string { return c.dataDir } +func (c *initTestContext) GetEnv() *libkb.Env { return c.env } + +func (c *initTestContext) GetMountDir() (string, error) { + return "", errors.New("no mount dir in test") +} + +func (c *initTestContext) ConfigureSocketInfo() error { return nil } +func (c *initTestContext) CheckService() error { return nil } + +func (c *initTestContext) GetSocket(bool) (net.Conn, rpc.Transporter, bool, error) { + return nil, nil, false, errNoSocket +} + +func (c *initTestContext) NewRPCLogFactory() rpc.LogFactory { return nil } + +func (c *initTestContext) NewNetworkInstrumenter( + keybase1.NetworkSource, +) rpc.NetworkInstrumenterStorage { + return nil +} + +func (c *initTestContext) GetKBFSSocket(bool) (net.Conn, rpc.Transporter, bool, error) { + return nil, nil, false, errNoSocket +} + +func (c *initTestContext) BindToKBFSSocket() (net.Listener, error) { + return nil, errNoSocket +} + +func (c *initTestContext) GetVDebugSetting() string { return "" } +func (c *initTestContext) GetPerfLog() logger.Logger { return logger.NewNull() } + +type shutdownRecorder struct { + KeybaseService + shutdown chan struct{} +} + +func (s shutdownRecorder) Shutdown() { + close(s.shutdown) + s.KeybaseService.Shutdown() +} + +var errInitTestCrypto = errors.New("crypto unavailable in test") + +// initOrderCn stands in for the service. NewChat runs right after init sets +// the service, so it delivers what the live service can send at that point +// while the rest of init is still to come. NewCrypto then fails init. +type initOrderCn struct { + t *testing.T + daemon *KeybaseDaemonRPC + shutdown chan struct{} +} + +func (c *initOrderCn) NewKeybaseService( + config Config, _ InitParams, _ Context, log logger.Logger, +) (KeybaseService, error) { + name := kbname.NormalizedUsername("fake username") + c.daemon = newKeybaseDaemonRPC(config, nil, log) + c.daemon.fillClients(&fakeKeybaseClient{session: idutil.SessionInfo{ + Name: name, + UID: keybase1.MakeTestUID(1), + CryptPublicKey: idutil.MakeLocalUserCryptPublicKeyOrBust(name), + VerifyingKey: idutil.MakeLocalUserVerifyingKeyOrBust(name), + }}) + return shutdownRecorder{c.daemon, c.shutdown}, nil +} + +func (c *initOrderCn) NewChat( + config Config, _ InitParams, _ Context, _ logger.Logger, +) (Chat, error) { + ctx := context.Background() + require.NoError(c.t, c.daemon.ReachabilityChanged( + ctx, keybase1.Reachability{Reachable: keybase1.Reachable_NO})) + require.NoError(c.t, c.daemon.FavoritesChanged(ctx, keybase1.UID(""))) + _, err := c.daemon.CurrentSession(ctx, 0) + require.NoError(c.t, err) + require.NoError(c.t, c.daemon.LoggedOut(ctx)) + return newChatLocal(config), nil +} + +func (c *initOrderCn) NewCrypto( + Config, InitParams, Context, logger.Logger, +) (Crypto, error) { + return nil, errInitTestCrypto +} + +// The service can call KBFS's handlers as soon as its connection is up, which +// is before init finishes. KBFSOps and MDOps must already be set by then, and +// a failed init must shut the connection down. +func TestInitSetsUpKBFSBeforeService(t *testing.T) { + dataDir := t.TempDir() + kbCtx := &initTestContext{ + env: libkb.NewEnv(nil, nil, func() logger.Logger { + return logger.NewNull() + }), + dataDir: dataDir, + } + params := DefaultInitParams(kbCtx) + params.StorageRoot = dataDir + params.DiskCacheMode = DiskCacheModeOff + params.EnableJournal = false + + cn := &initOrderCn{t: t, shutdown: make(chan struct{})} + _, err := doInit( + context.Background(), kbCtx, params, cn, logger.NewTestLogger(t), "test") + require.ErrorContains(t, err, errInitTestCrypto.Error()) + + select { + case <-cn.shutdown: + default: + t.Fatal("init failed without shutting down the service connection") + } +} diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc.go b/go/kbfs/libkbfs/keybase_daemon_rpc.go index d06faaf2394d..f6a5ceebcf13 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc.go @@ -84,10 +84,11 @@ func (k *KeybaseDaemonRPC) addKBFSProtocols() { const kbfsInitPollInterval = 100 * time.Millisecond // waitForKBFSInit wraps every method of the given protocols (SimpleFS, git, -// fs) so each request waits until init has set up KBFSOps, MDOps and the -// servers. The service connection, and so these handlers, is live before -// init sets them. Each request is served on its own goroutine, so waiting -// doesn't block the connection; the caller's context bounds the wait. +// fs) so each request waits until init has set the key and block servers, +// which it does after the service connection is live. Each request is served +// on its own goroutine, so waiting doesn't block the connection. The wait +// ends with the caller's context, or when a failed init shuts the +// connection down. func waitForKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { if len(protocols) == 0 { return protocols @@ -526,10 +527,8 @@ func (k *KeybaseDaemonRPC) FavoritesChanged(ctx context.Context, uid keybase1.UID, ) error { k.log.Debug("Received FavoritesChanged RPC.") - if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { - kbfsOps.RefreshCachedFavorites(ctx, - FavoritesRefreshModeInMainFavoritesLoop) - } + k.config.KBFSOps().RefreshCachedFavorites(ctx, + FavoritesRefreshModeInMainFavoritesLoop) return nil } diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go index 280bb3c73cec..ca1819e081ab 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go @@ -55,46 +55,19 @@ func TestKeybaseDaemonRPCGetCurrentSessionCanceled(t *testing.T) { testRPCWithCanceledContext(t, serverConn, f) } -// Service notifications can arrive before init has called SetKBFSOps. -func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { +// Init sets the key and block servers after the service connection is live, +// so service-initiated requests can arrive before them. +func TestKeybaseDaemonRPCRequestsBeforeServers(t *testing.T) { config := MakeTestConfigOrBust(t, "testuser") - kbfsOps, mdOps := config.KBFSOps(), config.MDOps() - config.SetKBFSOps(nil) + keyServer := config.KeyServer() + config.SetKeyServer(nil) defer func() { - config.SetKBFSOps(kbfsOps) - config.SetMDOps(mdOps) + config.SetKeyServer(keyServer) CheckConfigAndShutdown(context.Background(), t, config) }() - name := kbname.NormalizedUsername("fake username") - session := idutil.SessionInfo{ - Name: name, - UID: keybase1.MakeTestUID(1), - CryptPublicKey: idutil.MakeLocalUserCryptPublicKeyOrBust(name), - VerifyingKey: idutil.MakeLocalUserVerifyingKeyOrBust(name), - } - client := &fakeKeybaseClient{session: session} daemon := newKeybaseDaemonRPC(config, nil, logger.NewTestLogger(t)) - daemon.fillClients(client) ctx := context.Background() - for _, r := range []keybase1.Reachable{ - keybase1.Reachable_YES, keybase1.Reachable_NO, - } { - require.NoError(t, daemon.ReachabilityChanged( - ctx, keybase1.Reachability{Reachable: r})) - } - require.NoError(t, daemon.FavoritesChanged(ctx, keybase1.UID(""))) - // PaperKeyCached only acts for the current session's user. - daemon.setCachedCurrentSession(session) - require.NoError(t, daemon.PaperKeyCached( - ctx, keybase1.PaperKeyCachedArg{Uid: session.UID})) - require.NoError(t, daemon.TeamChangedByID(ctx, keybase1.TeamChangedByIDArg{ - Changes: keybase1.TeamChangeSet{Renamed: true}, - })) - require.NoError(t, daemon.TeamAbandoned(ctx, keybase1.TeamID(""))) - require.NoError(t, daemon.LoggedOut(ctx)) - - // Service-initiated requests get an error instead of a nil dereference. query := keybase1.TLFQuery{TlfName: "testuser"} _, err := daemon.GetTLFCryptKeys(ctx, query) require.Equal(t, errKBFSNotInitialized{}, err) @@ -106,28 +79,16 @@ func TestKeybaseDaemonRPCNotificationsBeforeKBFSOps(t *testing.T) { daemon.StartMigration(ctx, keybase1.Folder{})) require.Equal(t, errKBFSNotInitialized{}, daemon.FinalizeMigration(ctx, keybase1.Folder{})) - - // The logged-in flow is deferred, not dropped: the session stays - // uncached until KBFSOps is set, and the next lookup runs it. - testCurrentSession(t, client, daemon, session, expectCall) - testCurrentSession(t, client, daemon, session, expectCall) - // KBFSOps alone isn't enough: init sets MDOps just after it. - config.SetMDOps(nil) - config.SetKBFSOps(kbfsOps) - testCurrentSession(t, client, daemon, session, expectCall) - config.SetMDOps(mdOps) - testCurrentSession(t, client, daemon, session, expectCall) - testCurrentSession(t, client, daemon, session, expectCached) } // The SimpleFS/git/fs protocols share the service connection, so their -// requests can also arrive before init has set up KBFS. +// requests can also arrive before init has set the servers. func TestWaitForKBFSInit(t *testing.T) { config := MakeTestConfigOrBust(t, "testuser") - kbfsOps := config.KBFSOps() - config.SetKBFSOps(nil) + blockServer := config.BlockServer() + config.SetBlockServer(nil) defer func() { - config.SetKBFSOps(kbfsOps) + config.SetBlockServer(blockServer) CheckConfigAndShutdown(context.Background(), t, config) }() @@ -154,9 +115,12 @@ func TestWaitForKBFSInit(t *testing.T) { // Becoming ready mid-wait lets the request through. go func() { time.Sleep(50 * time.Millisecond) - config.SetKBFSOps(kbfsOps) + config.SetBlockServer(blockServer) }() - _, err = handler(context.Background(), nil) + readyCtx, readyCancel := context.WithTimeout( + context.Background(), 5*time.Second) + defer readyCancel() + _, err = handler(readyCtx, nil) require.NoError(t, err) require.Len(t, called, 1) } diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index 64d943117775..8a08348e0e05 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -417,15 +417,11 @@ func (k *KeybaseServiceBase) ReachabilityChanged(ctx context.Context, reachability keybase1.Reachability, ) error { k.log.CDebugf(ctx, "CheckReachability invoked: %v", reachability) - // The service connection delivers notifications before init has called - // SetKBFSOps, so KBFSOps can still be nil here. - if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { - if reachability.Reachable == keybase1.Reachable_YES { - kbfsOps.PushConnectionStatusChange(GregorServiceName, nil) - } else { - kbfsOps.PushConnectionStatusChange( - GregorServiceName, errDisconnected{}) - } + if reachability.Reachable == keybase1.Reachable_YES { + k.config.KBFSOps().PushConnectionStatusChange(GregorServiceName, nil) + } else { + k.config.KBFSOps().PushConnectionStatusChange( + GregorServiceName, errDisconnected{}) } mdServer := k.config.MDServer() if mdServer != nil { @@ -440,17 +436,11 @@ type errKBFSNotInitialized struct{} func (errKBFSNotInitialized) Error() string { return "KBFS is not initialized yet" } -// kbfsOpsReady reports whether init has set KBFSOps and MDOps. The service -// connection, and so every handler on it, is live before init sets them. -func kbfsOpsReady(config Config) bool { - return config.KBFSOps() != nil && config.MDOps() != nil -} - -// kbfsServersReady also requires the key and block servers, which init sets -// last. Requests that fetch keys or blocks need them. +// kbfsServersReady reports whether init has set the key and block servers. +// Init sets them after the service connection is live, and requests that +// fetch keys or blocks need them. func kbfsServersReady(config Config) bool { - return kbfsOpsReady(config) && - config.KeyServer() != nil && config.BlockServer() != nil + return config.KeyServer() != nil && config.BlockServer() != nil } // StartReachability implements keybase1.ReachabilityInterface. @@ -475,15 +465,13 @@ func (k *KeybaseServiceBase) PaperKeyCached(ctx context.Context, k.log.CDebugf(ctx, "Paper key for %s cached", arg.Uid) if k.getCachedCurrentSession().UID == arg.Uid { - if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { - err := kbfsOps.KickoffAllOutstandingRekeys() - if err != nil { - // Ignore and log errors here. For now the only way it could error - // is when the method is called on a folderBranchOps which is a - // developer mistake and not recoverable from code. - k.log.CDebugf(ctx, - "Calling KickoffAllOutstandingRekeys error: %s", err) - } + err := k.config.KBFSOps().KickoffAllOutstandingRekeys() + if err != nil { + // Ignore and log errors here. For now the only way it could error + // is when the method is called on a folderBranchOps which is a + // developer mistake and not recoverable from code. + k.log.CDebugf(ctx, + "Calling KickoffAllOutstandingRekeys error: %s", err) } // Ignore any errors for now, we don't want to block this // notification and it's not worth spawning a goroutine for. @@ -1167,14 +1155,11 @@ func (k *KeybaseServiceBase) getCurrentSession( } var s idutil.SessionInfo - cache := true // Close and clear the in-progress channel, even on an error. defer func() { k.sessionCacheLock.Lock() defer k.sessionCacheLock.Unlock() - if cache { - k.cachedCurrentSession = s - } + k.cachedCurrentSession = s close(k.sessionInProgressCh) k.sessionInProgressCh = nil }() @@ -1195,13 +1180,6 @@ func (k *KeybaseServiceBase) getCurrentSession( k.log.CDebugf( ctx, "new session with username %s, uid %s, crypt public key %s, and verifying key %s", s.Name, s.UID, s.CryptPublicKey, s.VerifyingKey) - // The logged-in flow needs KBFSOps and MDOps, which init sets after the - // service connection is live. Until then leave the session uncached, so - // the first lookup once they're set is the new session that runs it. - if k.config != nil && !kbfsOpsReady(k.config) { - cache = false - return s, false, nil - } return s, true, nil } @@ -1441,8 +1419,8 @@ func (k *KeybaseServiceBase) TeamChangedByID(ctx context.Context, arg.Changes.KeyRotated, arg.Changes.Renamed) k.setCachedTeamInfo(arg.TeamID, idutil.TeamInfo{}) - if kbfsOps := k.config.KBFSOps(); arg.Changes.Renamed && kbfsOps != nil { - kbfsOps.TeamNameChanged(ctx, arg.TeamID) + if arg.Changes.Renamed { + k.config.KBFSOps().TeamNameChanged(ctx, arg.TeamID) } return nil } @@ -1492,9 +1470,7 @@ func (k *KeybaseDaemonRPC) TeamAbandoned( ) error { k.log.CDebugf(ctx, "Implicit team %s abandoned", tid) k.setCachedTeamInfo(tid, idutil.TeamInfo{}) - if kbfsOps := k.config.KBFSOps(); kbfsOps != nil { - kbfsOps.TeamAbandoned(ctx, tid) - } + k.config.KBFSOps().TeamAbandoned(ctx, tid) return nil } diff --git a/go/kbfs/libkbfs/keybase_service_util.go b/go/kbfs/libkbfs/keybase_service_util.go index abe632f126f5..2a55088ae6fa 100644 --- a/go/kbfs/libkbfs/keybase_service_util.go +++ b/go/kbfs/libkbfs/keybase_service_util.go @@ -138,17 +138,16 @@ func serviceLoggedIn(ctx context.Context, config Config, session idutil.SessionI // serviceLoggedOut should be called when the current user logs out. func serviceLoggedOut(ctx context.Context, config Config) { - // A logout can arrive before init has set KBFSOps and MDOps. Nothing has - // been cached yet then, and Chat may still be unset. - if !kbfsOpsReady(config) { - return - } if jManager, err := GetJournalManager(config); err == nil { jManager.shutdownExistingJournals(ctx) } config.ResetCaches() config.UserHistory().Clear() - config.Chat().ClearCache() + // Init sets Chat after the service connection is live, so a logout can + // arrive before it exists. + if chat := config.Chat(); chat != nil { + chat.ClearCache() + } mdServer := config.MDServer() if mdServer != nil { mdServer.RefreshAuthToken(ctx) From 242081680357d9266c1e66e02bca2b5e8257c84a Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 11:22:14 -0400 Subject: [PATCH 06/11] fix(kbfs): gate early service calls on init finishing, not on shutting the connection down The failed-init service.Shutdown() didn't disconnect anything: the shared transport's Close is a no-op, so the handlers stayed registered and waiting requests weren't cancelled. For passthrough child inits (libgit, search) it also stopped the parent service's keepalive. Drop it. Instead, KBFSOpsStandard records how init ended: initDoneCh is now closed only on success, and a new initFailedCh on failure. - SimpleFS/git/fs requests wait for that (bounded by the caller's context) instead of polling for the key and block servers, so they also wait out journaling setup, and fail fast if init failed. - Service-initiated KBFS requests check it without blocking. - The edit-history and synced-TLF goroutines return on failure instead of running against a half-built config. Also set up the disk limiter before the service connection: a login it delivers can create the disk block cache, which expects the limiter. TestInitSetsUpKBFSBeforeService now calls into KBFS from inside the fake NewKeybaseService (the earliest point) as well as after the service is set, checks that requests wait during init and fail once init fails, and shuts KBFSOps down before its TempDir is removed. TestKeybaseDaemonRPCGatesAdditionalProtocolsOnInit checks the real NewKeybaseDaemonRPC wiring. --- go/kbfs/libkbfs/init.go | 47 ++++---- go/kbfs/libkbfs/init_test.go | 134 ++++++++++++++------- go/kbfs/libkbfs/kbfs_ops.go | 39 +++++- go/kbfs/libkbfs/keybase_daemon_rpc.go | 35 ++---- go/kbfs/libkbfs/keybase_daemon_rpc_test.go | 78 +++++------- go/kbfs/libkbfs/keybase_service_base.go | 41 +++++-- 6 files changed, 222 insertions(+), 152 deletions(-) diff --git a/go/kbfs/libkbfs/init.go b/go/kbfs/libkbfs/init.go index 89421d1ea539..760eeb957534 100644 --- a/go/kbfs/libkbfs/init.go +++ b/go/kbfs/libkbfs/init.go @@ -807,12 +807,36 @@ func doInit( // right away. None of these use the service until they're called. initDoneCh := make(chan struct{}) kbfsOps := NewKBFSOpsStandard(kbCtx, config, initDoneCh) - defer close(initDoneCh) + // Handlers on the service connection wait for init to finish (see + // waitForKBFSInit), so tell them how it ended. + initSucceeded := false + defer func() { + if initSucceeded { + close(initDoneCh) + } else { + kbfsOps.initFailed() + } + }() config.SetKBFSOps(kbfsOps) config.SetNotifier(kbfsOps) config.SetKeyManager(NewKeyManagerStandard(config)) config.SetMDOps(NewMDOpsStandard(config)) + // Also before the service connection: a login it delivers can create the + // disk block cache, which expects the disk limiter. The limiter only reads + // local config. + config.SetDiskBlockCacheFraction(getCacheFrac( + ctx, kbCtx, params.DiskBlockCacheFraction, + defaultDiskBlockCacheFraction, configBlockCacheDiskMaxFracStr, log)) + config.SetSyncBlockCacheFraction(getCacheFrac( + ctx, kbCtx, params.SyncBlockCacheFraction, + defaultSyncBlockCacheFraction, configBlockCacheSyncMaxFracStr, log)) + err = config.EnableDiskLimiter(params.StorageRoot) + if err != nil { + log.CWarningf(ctx, "Could not enable disk limiter: %+v", err) + return nil, err + } + // Initialize Keybase service connection. if keybaseServiceCn == nil { keybaseServiceCn = keybaseDaemon{} @@ -826,15 +850,6 @@ func doInit( service = NewKeybaseServiceMeasured(service, registry) } config.SetKeybaseService(service) - // If init fails from here on, close the connection so the service stops - // routing to this half-initialized KBFS. That also cancels requests - // waiting in waitForKBFSInit. - initSucceeded := false - defer func() { - if !initSucceeded { - service.Shutdown() - } - }() // Initialize Chat client (for file edit notifications). chat, err := keybaseServiceCn.NewChat(config, params, kbCtx, kbfsLog) @@ -843,18 +858,6 @@ func doInit( } config.SetChat(chat) - config.SetDiskBlockCacheFraction(getCacheFrac( - ctx, kbCtx, params.DiskBlockCacheFraction, - defaultDiskBlockCacheFraction, configBlockCacheDiskMaxFracStr, log)) - config.SetSyncBlockCacheFraction(getCacheFrac( - ctx, kbCtx, params.SyncBlockCacheFraction, - defaultSyncBlockCacheFraction, configBlockCacheSyncMaxFracStr, log)) - err = config.EnableDiskLimiter(params.StorageRoot) - if err != nil { - log.CWarningf(ctx, "Could not enable disk limiter: %+v", err) - return nil, err - } - kbfsOps.favs.Initialize(ctx) config.SetReporter(NewReporterKBPKI(config, 10, 1000)) diff --git a/go/kbfs/libkbfs/init_test.go b/go/kbfs/libkbfs/init_test.go index 4ec4c63500fc..a76614587c58 100644 --- a/go/kbfs/libkbfs/init_test.go +++ b/go/kbfs/libkbfs/init_test.go @@ -9,6 +9,7 @@ import ( "errors" "net" "testing" + "time" "github.com/keybase/client/go/kbconst" "github.com/keybase/client/go/kbfs/env" @@ -21,8 +22,8 @@ import ( "github.com/stretchr/testify/require" ) -// initTestContext is the minimal Context doInit needs up to the point where -// initOrderCn fails it. +// initTestContext is the minimal Context doInit and NewKeybaseDaemonRPC need +// in tests. It has no sockets, so connections just keep failing to dial. type initTestContext struct { env.EmptyAppStateUpdater env *libkb.Env @@ -31,6 +32,15 @@ type initTestContext struct { var _ Context = (*initTestContext)(nil) +func newInitTestContext(t *testing.T) *initTestContext { + return &initTestContext{ + env: libkb.NewEnv(nil, nil, func() logger.Logger { + return logger.NewNull() + }), + dataDir: t.TempDir(), + } +} + var errNoSocket = errors.New("no socket in test") func (c *initTestContext) GetRunMode() kbconst.RunMode { return kbconst.DevelRunMode } @@ -68,30 +78,37 @@ func (c *initTestContext) BindToKBFSSocket() (net.Listener, error) { func (c *initTestContext) GetVDebugSetting() string { return "" } func (c *initTestContext) GetPerfLog() logger.Logger { return logger.NewNull() } -type shutdownRecorder struct { - KeybaseService - shutdown chan struct{} -} - -func (s shutdownRecorder) Shutdown() { - close(s.shutdown) - s.KeybaseService.Shutdown() +// newGatedTestProtocol returns a one-method protocol, standing in for +// SimpleFS/git/fs, that counts the calls that reach its handler. +func newGatedTestProtocol(calls *int) rpc.Protocol { + return rpc.Protocol{ + Name: "gatedTest", + Methods: map[string]rpc.ServeHandlerDescription{ + "method": {Handler: func(context.Context, any) (any, error) { + *calls++ + return nil, nil + }}, + }, + } } var errInitTestCrypto = errors.New("crypto unavailable in test") -// initOrderCn stands in for the service. NewChat runs right after init sets -// the service, so it delivers what the live service can send at that point -// while the rest of init is still to come. NewCrypto then fails init. +// initOrderCn stands in for the service. Both while its connection is being +// built and once init has set it, it calls into KBFS the way the live service +// can while init is still running. NewCrypto then fails init. type initOrderCn struct { - t *testing.T - daemon *KeybaseDaemonRPC - shutdown chan struct{} + t *testing.T + config Config + daemon *KeybaseDaemonRPC + gated func(context.Context, any) (any, error) + calls int } func (c *initOrderCn) NewKeybaseService( config Config, _ InitParams, _ Context, log logger.Logger, ) (KeybaseService, error) { + c.config = config name := kbname.NormalizedUsername("fake username") c.daemon = newKeybaseDaemonRPC(config, nil, log) c.daemon.fillClients(&fakeKeybaseClient{session: idutil.SessionInfo{ @@ -100,19 +117,16 @@ func (c *initOrderCn) NewKeybaseService( CryptPublicKey: idutil.MakeLocalUserCryptPublicKeyOrBust(name), VerifyingKey: idutil.MakeLocalUserVerifyingKeyOrBust(name), }}) - return shutdownRecorder{c.daemon, c.shutdown}, nil + gated := gateOnKBFSInit(config, []rpc.Protocol{newGatedTestProtocol(&c.calls)}) + c.gated = gated[0].Methods["method"].Handler + c.callIntoKBFS() + return c.daemon, nil } func (c *initOrderCn) NewChat( config Config, _ InitParams, _ Context, _ logger.Logger, ) (Chat, error) { - ctx := context.Background() - require.NoError(c.t, c.daemon.ReachabilityChanged( - ctx, keybase1.Reachability{Reachable: keybase1.Reachable_NO})) - require.NoError(c.t, c.daemon.FavoritesChanged(ctx, keybase1.UID(""))) - _, err := c.daemon.CurrentSession(ctx, 0) - require.NoError(c.t, err) - require.NoError(c.t, c.daemon.LoggedOut(ctx)) + c.callIntoKBFS() return newChatLocal(config), nil } @@ -122,30 +136,68 @@ func (c *initOrderCn) NewCrypto( return nil, errInitTestCrypto } -// The service can call KBFS's handlers as soon as its connection is up, which -// is before init finishes. KBFSOps and MDOps must already be set by then, and -// a failed init must shut the connection down. -func TestInitSetsUpKBFSBeforeService(t *testing.T) { - dataDir := t.TempDir() - kbCtx := &initTestContext{ - env: libkb.NewEnv(nil, nil, func() logger.Logger { - return logger.NewNull() - }), - dataDir: dataDir, +func (c *initOrderCn) callIntoKBFS() { + t := c.t + ctx := context.Background() + for _, r := range []keybase1.Reachable{ + keybase1.Reachable_YES, keybase1.Reachable_NO, + } { + require.NoError(t, c.daemon.ReachabilityChanged( + ctx, keybase1.Reachability{Reachable: r})) } + require.NoError(t, c.daemon.FavoritesChanged(ctx, keybase1.UID(""))) + require.NoError(t, c.daemon.TeamChangedByID(ctx, keybase1.TeamChangedByIDArg{ + Changes: keybase1.TeamChangeSet{Renamed: true}, + })) + require.NoError(t, c.daemon.TeamAbandoned(ctx, keybase1.TeamID(""))) + session, err := c.daemon.CurrentSession(ctx, 0) + require.NoError(t, err) + require.NoError(t, c.daemon.PaperKeyCached( + ctx, keybase1.PaperKeyCachedArg{Uid: session.UID})) + require.NoError(t, c.daemon.LoggedOut(ctx)) + + // Requests get an error, or wait, until init finishes. + _, err = c.daemon.GetTLFCryptKeys(ctx, keybase1.TLFQuery{TlfName: "testuser"}) + require.Equal(t, errKBFSNotInitialized{}, err) + waitCtx, cancel := context.WithTimeout(ctx, 20*time.Millisecond) + defer cancel() + _, err = c.gated(waitCtx, nil) + require.ErrorIs(t, err, context.DeadlineExceeded) +} + +// The service can call into KBFS as soon as its connection is up, which is +// before init finishes. KBFSOps and MDOps must already be set by then, and +// once init fails, requests must fail instead of waiting. +func TestInitSetsUpKBFSBeforeService(t *testing.T) { + cn := &initOrderCn{t: t} + kbCtx := newInitTestContext(t) + initReturned := false + // Registered after TempDir, so it runs first and closes the favorites + // db before the directory is removed. Skipped if doInit stopped partway + // (a failed assertion), since the favorites Shutdown can then block. + t.Cleanup(func() { + if !initReturned { + return + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + require.NoError(t, cn.config.KBFSOps().Shutdown(ctx)) + }) params := DefaultInitParams(kbCtx) - params.StorageRoot = dataDir + params.StorageRoot = kbCtx.dataDir params.DiskCacheMode = DiskCacheModeOff params.EnableJournal = false - cn := &initOrderCn{t: t, shutdown: make(chan struct{})} _, err := doInit( context.Background(), kbCtx, params, cn, logger.NewTestLogger(t), "test") + initReturned = true require.ErrorContains(t, err, errInitTestCrypto.Error()) - select { - case <-cn.shutdown: - default: - t.Fatal("init failed without shutting down the service connection") - } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err = cn.gated(ctx, nil) + require.Equal(t, errKBFSNotInitialized{}, err) + _, err = cn.daemon.GetTLFCryptKeys(ctx, keybase1.TLFQuery{TlfName: "testuser"}) + require.Equal(t, errKBFSNotInitialized{}, err) + require.Zero(t, cn.calls) } diff --git a/go/kbfs/libkbfs/kbfs_ops.go b/go/kbfs/libkbfs/kbfs_ops.go index 549f70d2cd99..33cc037e9d7f 100644 --- a/go/kbfs/libkbfs/kbfs_ops.go +++ b/go/kbfs/libkbfs/kbfs_ops.go @@ -51,6 +51,8 @@ type KBFSOpsStandard struct { // watcher. reIdentifyControlChan chan chan<- struct{} initDoneCh <-chan struct{} + // initFailedCh is closed instead of initDoneCh if init fails. + initFailedCh chan struct{} favs *Favorites @@ -82,7 +84,8 @@ const ctxKBFSOpsSkipEditHistoryBlock ctxKBFSOpsSkipEditHistoryBlockType = 1 // NewKBFSOpsStandard constructs a new KBFSOpsStandard object. // `initDone` should be closed when the rest of initialization (such -// as journal initialization) has completed. +// as journal initialization) has completed. If it fails instead, call +// initFailed. func NewKBFSOpsStandard( appStateUpdater env.AppStateUpdater, config Config, initDoneCh <-chan struct{}, @@ -97,6 +100,7 @@ func NewKBFSOpsStandard( opsByFav: make(map[favorites.Folder]*folderBranchOps), reIdentifyControlChan: make(chan chan<- struct{}), initDoneCh: initDoneCh, + initFailedCh: make(chan struct{}), favs: NewFavorites(config), syncedTlfObservers: newSyncedTlfObserverList(), longOperationDebugDumper: NewImpatientDebugDumper( @@ -108,6 +112,35 @@ func NewKBFSOpsStandard( return kops } +// initFailed tells anything in waitForInit that init won't finish. +func (fs *KBFSOpsStandard) initFailed() { + close(fs.initFailedCh) +} + +// waitForInit blocks until init has finished, and returns +// errKBFSNotInitialized if it failed. A nil initDoneCh (KBFSOps built +// outside init, as in tests) counts as finished. +func (fs *KBFSOpsStandard) waitForInit(ctx context.Context) error { + if fs.initDoneCh == nil { + return nil + } + // Check for success first, so an already-canceled ctx can't win the + // select below when init has finished. + select { + case <-fs.initDoneCh: + return nil + default: + } + select { + case <-fs.initDoneCh: + return nil + case <-fs.initFailedCh: + return errKBFSNotInitialized{} + case <-ctx.Done(): + return ctx.Err() + } +} + func (fs *KBFSOpsStandard) markForReIdentifyIfNeededLoop() { maxValid := fs.config.TLFValidDuration() // Tests and some users fail to set this properly. @@ -2180,6 +2213,8 @@ func (fs *KBFSOpsStandard) initTlfsForEditHistories() { select { case <-fs.initDoneCh: + case <-fs.initFailedCh: + return case <-ctx.Done(): return } @@ -2267,6 +2302,8 @@ func (fs *KBFSOpsStandard) initSyncedTlfs() { select { case <-fs.initDoneCh: + case <-fs.initFailedCh: + return case <-ctx.Done(): return } diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc.go b/go/kbfs/libkbfs/keybase_daemon_rpc.go index f6a5ceebcf13..61d666c5cc67 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc.go @@ -81,15 +81,12 @@ func (k *KeybaseDaemonRPC) addKBFSProtocols() { k.AddProtocols(protocols) } -const kbfsInitPollInterval = 100 * time.Millisecond - -// waitForKBFSInit wraps every method of the given protocols (SimpleFS, git, -// fs) so each request waits until init has set the key and block servers, -// which it does after the service connection is live. Each request is served -// on its own goroutine, so waiting doesn't block the connection. The wait -// ends with the caller's context, or when a failed init shuts the -// connection down. -func waitForKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { +// gateOnKBFSInit wraps every method of the given protocols (SimpleFS, git, +// fs) so each request waits for init to finish, which is after the service +// connection is live. Each request is served on its own goroutine, so +// waiting doesn't block the connection. A request fails with +// errKBFSNotInitialized if init failed, or ends with the caller's context. +func gateOnKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { if len(protocols) == 0 { return protocols } @@ -99,7 +96,7 @@ func waitForKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { for name, m := range p.Methods { handler := m.Handler m.Handler = func(ctx context.Context, arg any) (any, error) { - if err := waitForKBFSServersReady(ctx, config); err != nil { + if err := waitForKBFSInit(ctx, config); err != nil { return nil, err } return handler(ctx, arg) @@ -112,22 +109,6 @@ func waitForKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { return wrapped } -func waitForKBFSServersReady(ctx context.Context, config Config) error { - if kbfsServersReady(config) { - return nil - } - ticker := time.NewTicker(kbfsInitPollInterval) - defer ticker.Stop() - for !kbfsServersReady(config) { - select { - case <-ticker.C: - case <-ctx.Done(): - return ctx.Err() - } - } - return nil -} - // NewKeybaseDaemonRPC makes a new KeybaseDaemonRPC that makes RPC // calls using the socket of the given Keybase context. func NewKeybaseDaemonRPC(config Config, kbCtx Context, log logger.Logger, @@ -151,7 +132,7 @@ func NewKeybaseDaemonRPC(config Config, kbCtx Context, log logger.Logger, k.notifyService = newNotifyServiceHandler(config, log) k.addKBFSProtocols() - k.AddProtocols(waitForKBFSInit(config, additionalProtocols)) + k.AddProtocols(gateOnKBFSInit(config, additionalProtocols)) return k } diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go index ca1819e081ab..57ba497b3ecd 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/golang/mock/gomock" + "github.com/keybase/client/go/kbfs/env" "github.com/keybase/client/go/kbfs/idutil" "github.com/keybase/client/go/kbfs/kbfscrypto" "github.com/keybase/client/go/kbfs/test/clocktest" @@ -55,74 +56,53 @@ func TestKeybaseDaemonRPCGetCurrentSessionCanceled(t *testing.T) { testRPCWithCanceledContext(t, serverConn, f) } -// Init sets the key and block servers after the service connection is live, -// so service-initiated requests can arrive before them. -func TestKeybaseDaemonRPCRequestsBeforeServers(t *testing.T) { +// NewKeybaseDaemonRPC gates the additional protocols (SimpleFS/git/fs) on +// init, since the service can call them as soon as the connection is up. +func TestKeybaseDaemonRPCGatesAdditionalProtocolsOnInit(t *testing.T) { config := MakeTestConfigOrBust(t, "testuser") - keyServer := config.KeyServer() - config.SetKeyServer(nil) + origKBFSOps := config.KBFSOps() + initDoneCh := make(chan struct{}) + kbfsOps := NewKBFSOpsStandard(env.EmptyAppStateUpdater{}, config, initDoneCh) + config.SetKBFSOps(kbfsOps) defer func() { - config.SetKeyServer(keyServer) + config.SetKBFSOps(origKBFSOps) + require.NoError(t, kbfsOps.Shutdown(context.Background())) CheckConfigAndShutdown(context.Background(), t, config) }() - daemon := newKeybaseDaemonRPC(config, nil, logger.NewTestLogger(t)) - ctx := context.Background() - query := keybase1.TLFQuery{TlfName: "testuser"} - _, err := daemon.GetTLFCryptKeys(ctx, query) - require.Equal(t, errKBFSNotInitialized{}, err) - _, err = daemon.GetPublicCanonicalTLFNameAndID(ctx, query) - require.Equal(t, errKBFSNotInitialized{}, err) - require.Equal(t, errKBFSNotInitialized{}, - daemon.FSEditListRequest(ctx, keybase1.FSEditListRequest{})) - require.Equal(t, errKBFSNotInitialized{}, - daemon.StartMigration(ctx, keybase1.Folder{})) - require.Equal(t, errKBFSNotInitialized{}, - daemon.FinalizeMigration(ctx, keybase1.Folder{})) -} - -// The SimpleFS/git/fs protocols share the service connection, so their -// requests can also arrive before init has set the servers. -func TestWaitForKBFSInit(t *testing.T) { - config := MakeTestConfigOrBust(t, "testuser") - blockServer := config.BlockServer() - config.SetBlockServer(nil) - defer func() { - config.SetBlockServer(blockServer) - CheckConfigAndShutdown(context.Background(), t, config) - }() - - called := make(chan struct{}, 1) - protocols := waitForKBFSInit(config, []rpc.Protocol{{ - Name: "test", - Methods: map[string]rpc.ServeHandlerDescription{ - "method": {Handler: func(context.Context, any) (any, error) { - called <- struct{}{} - return nil, nil - }}, - }, - }}) - handler := protocols[0].Methods["method"].Handler + var calls int + daemon := NewKeybaseDaemonRPC( + config, newInitTestContext(t), logger.NewTestLogger(t), false, + []rpc.Protocol{newGatedTestProtocol(&calls)}) + defer daemon.Shutdown() + var handler func(context.Context, any) (any, error) + daemon.lock.Lock() + for _, p := range daemon.protocols { + if p.Name == "gatedTest" { + handler = p.Methods["method"].Handler + } + } + daemon.lock.Unlock() + require.NotNil(t, handler) - // Not ready: the request waits until the caller gives up, and the - // handler never runs. + // Init still running: the request waits, and the handler doesn't run. ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) defer cancel() _, err := handler(ctx, nil) require.ErrorIs(t, err, context.DeadlineExceeded) - require.Empty(t, called) + require.Zero(t, calls) - // Becoming ready mid-wait lets the request through. + // Init finishing mid-wait lets the request through. go func() { time.Sleep(50 * time.Millisecond) - config.SetBlockServer(blockServer) + close(initDoneCh) }() readyCtx, readyCancel := context.WithTimeout( context.Background(), 5*time.Second) defer readyCancel() _, err = handler(readyCtx, nil) require.NoError(t, err) - require.Len(t, called, 1) + require.Equal(t, 1, calls) } // TODO: Add tests for Favorite* methods, too. diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index 8a08348e0e05..95e153f1b481 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -430,17 +430,34 @@ func (k *KeybaseServiceBase) ReachabilityChanged(ctx context.Context, return nil } -// errKBFSNotInitialized is returned from service-initiated requests that -// arrive before init has set up the parts of config they use. +// errKBFSNotInitialized is returned to the service for requests that arrive +// before init has finished, or after it failed. type errKBFSNotInitialized struct{} func (errKBFSNotInitialized) Error() string { return "KBFS is not initialized yet" } -// kbfsServersReady reports whether init has set the key and block servers. -// Init sets them after the service connection is live, and requests that -// fetch keys or blocks need them. -func kbfsServersReady(config Config) bool { - return config.KeyServer() != nil && config.BlockServer() != nil +// kbfsInitWaiter is implemented by KBFSOpsStandard. Init sets up the service +// connection before it finishes, so handlers on that connection use it. +type kbfsInitWaiter interface { + waitForInit(ctx context.Context) error +} + +// waitForKBFSInit blocks until init has finished, and returns +// errKBFSNotInitialized if it failed. +func waitForKBFSInit(ctx context.Context, config Config) error { + if w, ok := config.KBFSOps().(kbfsInitWaiter); ok { + return w.waitForInit(ctx) + } + return nil +} + +// kbfsInitDone reports, without blocking, whether init has finished. +// Service-initiated requests check this rather than waiting, so none of them +// can block on an init that is itself waiting on the service. +func kbfsInitDone(config Config) bool { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + return waitForKBFSInit(ctx, config) == nil } // StartReachability implements keybase1.ReachabilityInterface. @@ -1353,7 +1370,7 @@ func (k *KeybaseServiceBase) FSEditListRequest(ctx context.Context, k.log) k.log.CDebugf(ctx, "Edit list request for %s (public: %t)", req.Folder.Name, !req.Folder.Private) - if !kbfsServersReady(k.config) { + if !kbfsInitDone(k.config) { return errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( @@ -1504,7 +1521,7 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, if mdServer == nil { return errors.New("no mdserver") } - if !kbfsServersReady(k.config) { + if !kbfsInitDone(k.config) { return errKBFSNotInitialized{} } // Making a favorite here to reuse the code that converts from @@ -1531,7 +1548,7 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, func (k *KeybaseServiceBase) FinalizeMigration(ctx context.Context, folder keybase1.Folder, ) (err error) { - if !kbfsServersReady(k.config) { + if !kbfsInitDone(k.config) { return errKBFSNotInitialized{} } fav := favorites.NewFolderFromProtocol(folder) @@ -1571,7 +1588,7 @@ func (k *KeybaseServiceBase) GetTLFCryptKeys(ctx context.Context, return keybase1.GetTLFCryptKeysRes{}, err } - if !kbfsServersReady(k.config) { + if !kbfsInitDone(k.config) { return res, errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( @@ -1618,7 +1635,7 @@ func (k *KeybaseServiceBase) GetPublicCanonicalTLFNameAndID( return keybase1.CanonicalTLFNameAndIDWithBreaks{}, err } - if !kbfsServersReady(k.config) { + if !kbfsInitDone(k.config) { return res, errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( From 6f8a52b3c15206999044f5bcfdfa4b8a2783b962 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 11:42:45 -0400 Subject: [PATCH 07/11] fix(kbfs): let early service calls through once KBFS is ready, before journaling Gating on all of doInit held SimpleFS/git/fs requests through EnableJournaling, whose journal FBO setup isn't bounded by its 60s context, so early GUI calls (online status, subscriptions, badge) could exceed the service's 60s SimpleFS timeout where master served them. Service-initiated requests (chat's TLF key lookups) also failed for that whole tail. Add a separate ready signal: doInit calls kbfsOps.initReady() once Crypto, the servers, the caches and the KBFS service are set, just before journaling. Requests wait for (or check) that instead of full init; a failed init still makes them fail fast. The edit-history and synced-TLF goroutines keep waiting for full init. Also assert at compile time that KBFSOpsStandard implements kbfsInitWaiter, so renaming its method can't silently open the gate. --- go/kbfs/libkbfs/init.go | 8 +++-- go/kbfs/libkbfs/init_test.go | 4 +-- go/kbfs/libkbfs/kbfs_ops.go | 40 +++++++++++++++++----- go/kbfs/libkbfs/keybase_daemon_rpc.go | 14 ++++---- go/kbfs/libkbfs/keybase_daemon_rpc_test.go | 5 +-- go/kbfs/libkbfs/keybase_service_base.go | 35 ++++++++++--------- 6 files changed, 67 insertions(+), 39 deletions(-) diff --git a/go/kbfs/libkbfs/init.go b/go/kbfs/libkbfs/init.go index 760eeb957534..3a9340ba4291 100644 --- a/go/kbfs/libkbfs/init.go +++ b/go/kbfs/libkbfs/init.go @@ -807,8 +807,8 @@ func doInit( // right away. None of these use the service until they're called. initDoneCh := make(chan struct{}) kbfsOps := NewKBFSOpsStandard(kbCtx, config, initDoneCh) - // Handlers on the service connection wait for init to finish (see - // waitForKBFSInit), so tell them how it ended. + // Handlers on the service connection wait for init (see + // waitForKBFSReady), so tell them how it ended. initSucceeded := false defer func() { if initSucceeded { @@ -984,6 +984,10 @@ func doInit( } } + // Requests on the service connection have what they need from here on. + // Don't hold them for journaling, which can take a while. + kbfsOps.initReady() + ctx60s, cancel := context.WithTimeout(ctx, 60*time.Second) defer cancel() // TODO: Don't turn on journaling if either -bserver or diff --git a/go/kbfs/libkbfs/init_test.go b/go/kbfs/libkbfs/init_test.go index a76614587c58..c5be3fdaf94c 100644 --- a/go/kbfs/libkbfs/init_test.go +++ b/go/kbfs/libkbfs/init_test.go @@ -117,7 +117,7 @@ func (c *initOrderCn) NewKeybaseService( CryptPublicKey: idutil.MakeLocalUserCryptPublicKeyOrBust(name), VerifyingKey: idutil.MakeLocalUserVerifyingKeyOrBust(name), }}) - gated := gateOnKBFSInit(config, []rpc.Protocol{newGatedTestProtocol(&c.calls)}) + gated := gateOnKBFSReady(config, []rpc.Protocol{newGatedTestProtocol(&c.calls)}) c.gated = gated[0].Methods["method"].Handler c.callIntoKBFS() return c.daemon, nil @@ -156,7 +156,7 @@ func (c *initOrderCn) callIntoKBFS() { ctx, keybase1.PaperKeyCachedArg{Uid: session.UID})) require.NoError(t, c.daemon.LoggedOut(ctx)) - // Requests get an error, or wait, until init finishes. + // Until init is ready, requests get an error or wait. _, err = c.daemon.GetTLFCryptKeys(ctx, keybase1.TLFQuery{TlfName: "testuser"}) require.Equal(t, errKBFSNotInitialized{}, err) waitCtx, cancel := context.WithTimeout(ctx, 20*time.Millisecond) diff --git a/go/kbfs/libkbfs/kbfs_ops.go b/go/kbfs/libkbfs/kbfs_ops.go index 33cc037e9d7f..45e4a91f9407 100644 --- a/go/kbfs/libkbfs/kbfs_ops.go +++ b/go/kbfs/libkbfs/kbfs_ops.go @@ -53,6 +53,9 @@ type KBFSOpsStandard struct { initDoneCh <-chan struct{} // initFailedCh is closed instead of initDoneCh if init fails. initFailedCh chan struct{} + // initReadyCh is closed once config has what requests need, ahead of + // the slow tail of init (journaling). + initReadyCh chan struct{} favs *Favorites @@ -72,7 +75,10 @@ type KBFSOpsStandard struct { initSyncCancel context.CancelFunc } -var _ KBFSOps = (*KBFSOpsStandard)(nil) +var ( + _ KBFSOps = (*KBFSOpsStandard)(nil) + _ kbfsInitWaiter = (*KBFSOpsStandard)(nil) +) const longOperationDebugDumpDuration = time.Minute @@ -85,7 +91,7 @@ const ctxKBFSOpsSkipEditHistoryBlock ctxKBFSOpsSkipEditHistoryBlockType = 1 // NewKBFSOpsStandard constructs a new KBFSOpsStandard object. // `initDone` should be closed when the rest of initialization (such // as journal initialization) has completed. If it fails instead, call -// initFailed. +// initFailed. Call initReady as soon as config has what requests need. func NewKBFSOpsStandard( appStateUpdater env.AppStateUpdater, config Config, initDoneCh <-chan struct{}, @@ -101,6 +107,7 @@ func NewKBFSOpsStandard( reIdentifyControlChan: make(chan chan<- struct{}), initDoneCh: initDoneCh, initFailedCh: make(chan struct{}), + initReadyCh: make(chan struct{}), favs: NewFavorites(config), syncedTlfObservers: newSyncedTlfObserverList(), longOperationDebugDumper: NewImpatientDebugDumper( @@ -112,26 +119,41 @@ func NewKBFSOpsStandard( return kops } -// initFailed tells anything in waitForInit that init won't finish. +// initReady tells anything in waitForReady that config now has what requests +// need, ahead of the rest of init. +func (fs *KBFSOpsStandard) initReady() { + close(fs.initReadyCh) +} + +// initFailed tells anything waiting on init that it won't finish. func (fs *KBFSOpsStandard) initFailed() { close(fs.initFailedCh) } -// waitForInit blocks until init has finished, and returns -// errKBFSNotInitialized if it failed. A nil initDoneCh (KBFSOps built -// outside init, as in tests) counts as finished. -func (fs *KBFSOpsStandard) waitForInit(ctx context.Context) error { +// waitForReady blocks until init has set up what requests need (initReady, or +// init finishing), and returns errKBFSNotInitialized if init failed. A nil +// initDoneCh (KBFSOps built outside init, as in tests) counts as ready. +func (fs *KBFSOpsStandard) waitForReady(ctx context.Context) error { if fs.initDoneCh == nil { return nil } - // Check for success first, so an already-canceled ctx can't win the - // select below when init has finished. + // Check in priority order first, so a failed init always errors and an + // already-canceled ctx can't win the select below. + select { + case <-fs.initFailedCh: + return errKBFSNotInitialized{} + default: + } select { + case <-fs.initReadyCh: + return nil case <-fs.initDoneCh: return nil default: } select { + case <-fs.initReadyCh: + return nil case <-fs.initDoneCh: return nil case <-fs.initFailedCh: diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc.go b/go/kbfs/libkbfs/keybase_daemon_rpc.go index 61d666c5cc67..32f3b2c33dc4 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc.go @@ -81,12 +81,12 @@ func (k *KeybaseDaemonRPC) addKBFSProtocols() { k.AddProtocols(protocols) } -// gateOnKBFSInit wraps every method of the given protocols (SimpleFS, git, -// fs) so each request waits for init to finish, which is after the service -// connection is live. Each request is served on its own goroutine, so -// waiting doesn't block the connection. A request fails with +// gateOnKBFSReady wraps every method of the given protocols (SimpleFS, git, +// fs) so each request waits until init has set up what requests need, which +// is after the service connection is live. Each request is served on its own +// goroutine, so waiting doesn't block the connection. A request fails with // errKBFSNotInitialized if init failed, or ends with the caller's context. -func gateOnKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { +func gateOnKBFSReady(config Config, protocols []rpc.Protocol) []rpc.Protocol { if len(protocols) == 0 { return protocols } @@ -96,7 +96,7 @@ func gateOnKBFSInit(config Config, protocols []rpc.Protocol) []rpc.Protocol { for name, m := range p.Methods { handler := m.Handler m.Handler = func(ctx context.Context, arg any) (any, error) { - if err := waitForKBFSInit(ctx, config); err != nil { + if err := waitForKBFSReady(ctx, config); err != nil { return nil, err } return handler(ctx, arg) @@ -132,7 +132,7 @@ func NewKeybaseDaemonRPC(config Config, kbCtx Context, log logger.Logger, k.notifyService = newNotifyServiceHandler(config, log) k.addKBFSProtocols() - k.AddProtocols(gateOnKBFSInit(config, additionalProtocols)) + k.AddProtocols(gateOnKBFSReady(config, additionalProtocols)) return k } diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go index 57ba497b3ecd..0dc390e5e60b 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc_test.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc_test.go @@ -92,10 +92,11 @@ func TestKeybaseDaemonRPCGatesAdditionalProtocolsOnInit(t *testing.T) { require.ErrorIs(t, err, context.DeadlineExceeded) require.Zero(t, calls) - // Init finishing mid-wait lets the request through. + // Init becoming ready mid-wait lets the request through, without the + // rest of init having finished. go func() { time.Sleep(50 * time.Millisecond) - close(initDoneCh) + kbfsOps.initReady() }() readyCtx, readyCancel := context.WithTimeout( context.Background(), 5*time.Second) diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index 95e153f1b481..9fa035e59b94 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -431,33 +431,34 @@ func (k *KeybaseServiceBase) ReachabilityChanged(ctx context.Context, } // errKBFSNotInitialized is returned to the service for requests that arrive -// before init has finished, or after it failed. +// before init has set up what they need, or after init failed. type errKBFSNotInitialized struct{} func (errKBFSNotInitialized) Error() string { return "KBFS is not initialized yet" } // kbfsInitWaiter is implemented by KBFSOpsStandard. Init sets up the service -// connection before it finishes, so handlers on that connection use it. +// connection before it has set up everything requests need, so handlers on +// that connection use it. type kbfsInitWaiter interface { - waitForInit(ctx context.Context) error + waitForReady(ctx context.Context) error } -// waitForKBFSInit blocks until init has finished, and returns -// errKBFSNotInitialized if it failed. -func waitForKBFSInit(ctx context.Context, config Config) error { +// waitForKBFSReady blocks until init has set up what requests need, and +// returns errKBFSNotInitialized if init failed. +func waitForKBFSReady(ctx context.Context, config Config) error { if w, ok := config.KBFSOps().(kbfsInitWaiter); ok { - return w.waitForInit(ctx) + return w.waitForReady(ctx) } return nil } -// kbfsInitDone reports, without blocking, whether init has finished. -// Service-initiated requests check this rather than waiting, so none of them -// can block on an init that is itself waiting on the service. -func kbfsInitDone(config Config) bool { +// kbfsReady reports, without blocking, whether init has set up what requests +// need. Service-initiated requests check this rather than waiting, so none of +// them can block on an init that is itself waiting on the service. +func kbfsReady(config Config) bool { ctx, cancel := context.WithCancel(context.Background()) cancel() - return waitForKBFSInit(ctx, config) == nil + return waitForKBFSReady(ctx, config) == nil } // StartReachability implements keybase1.ReachabilityInterface. @@ -1370,7 +1371,7 @@ func (k *KeybaseServiceBase) FSEditListRequest(ctx context.Context, k.log) k.log.CDebugf(ctx, "Edit list request for %s (public: %t)", req.Folder.Name, !req.Folder.Private) - if !kbfsInitDone(k.config) { + if !kbfsReady(k.config) { return errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( @@ -1521,7 +1522,7 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, if mdServer == nil { return errors.New("no mdserver") } - if !kbfsInitDone(k.config) { + if !kbfsReady(k.config) { return errKBFSNotInitialized{} } // Making a favorite here to reuse the code that converts from @@ -1548,7 +1549,7 @@ func (k *KeybaseServiceBase) StartMigration(ctx context.Context, func (k *KeybaseServiceBase) FinalizeMigration(ctx context.Context, folder keybase1.Folder, ) (err error) { - if !kbfsInitDone(k.config) { + if !kbfsReady(k.config) { return errKBFSNotInitialized{} } fav := favorites.NewFolderFromProtocol(folder) @@ -1588,7 +1589,7 @@ func (k *KeybaseServiceBase) GetTLFCryptKeys(ctx context.Context, return keybase1.GetTLFCryptKeysRes{}, err } - if !kbfsInitDone(k.config) { + if !kbfsReady(k.config) { return res, errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( @@ -1635,7 +1636,7 @@ func (k *KeybaseServiceBase) GetPublicCanonicalTLFNameAndID( return keybase1.CanonicalTLFNameAndIDWithBreaks{}, err } - if !kbfsInitDone(k.config) { + if !kbfsReady(k.config) { return res, errKBFSNotInitialized{} } tlfHandle, err := getHandleFromFolderName( From 2ad4612a28ba3f9a8827c0cf74dd0f4767ab66d4 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 12:54:02 -0400 Subject: [PATCH 08/11] fix(desktop): allow the Vite dev origin to read KBFS http previews in hot dev The hot-dev renderer loads from http://localhost:4000, so text preview XHRs to the KBFS http server are cross-origin and blocked by CORS. Add the allow-origin header to those responses in hot dev only. --- shared/desktop/app/html-root.desktop.tsx | 2 +- shared/desktop/app/main-window.desktop.tsx | 13 ++++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/shared/desktop/app/html-root.desktop.tsx b/shared/desktop/app/html-root.desktop.tsx index 1f27d541afea..11674798adae 100644 --- a/shared/desktop/app/html-root.desktop.tsx +++ b/shared/desktop/app/html-root.desktop.tsx @@ -11,7 +11,7 @@ const fileRoot = `${htmlPrefix}${(`${distRoot}/`).replaceAll(path.sep, '/')}` // be loaded from file://). Cold dev + prod load the built html from file://. // Vite keeps the shells at their source paths in both cases, so the same // relative path is used for the http origin and the dist file root. -const devServerOrigin = 'http://localhost:4000' +export const devServerOrigin = 'http://localhost:4000' const htmlRelPath: Record = { main: 'desktop/renderer/main.html', remote: 'desktop/remote/remote.html', diff --git a/shared/desktop/app/main-window.desktop.tsx b/shared/desktop/app/main-window.desktop.tsx index 379dcaa229d3..6e30dafa0c67 100644 --- a/shared/desktop/app/main-window.desktop.tsx +++ b/shared/desktop/app/main-window.desktop.tsx @@ -7,7 +7,7 @@ import {showDevTools} from '@/local-debug' import {guiConfigFilename, isDarwin, isWindows, defaultUseNativeFrame} from '@/constants/platform' import logger from '@/logger' import debounce from 'lodash/debounce' -import {htmlURL, preloadPath} from './html-root.desktop' +import {devServerOrigin, htmlURL, preloadPath} from './html-root.desktop' import KB2 from '@/util/electron' const {env} = KB2.constants @@ -47,6 +47,17 @@ const setupDefaultSession = () => { } return callback(false) }) + + // In hot dev the renderer lives on the Vite http origin, so its XHRs to the + // KBFS http server (text previews) are cross-origin, and that server sends no + // CORS headers. Packaged builds load from file:// and don't need this. + if (__HOT__) { + ds.webRequest.onHeadersReceived({urls: ['http://127.0.0.1:*/files/*']}, (details, callback) => { + callback({ + responseHeaders: {...details.responseHeaders, 'Access-Control-Allow-Origin': [devServerOrigin]}, + }) + }) + } } const defaultWindowState = { From fc0add4b5df9a1bcac71fe774321429e67cc6402 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Thu, 10 Sep 2026 21:47:23 -0400 Subject: [PATCH 09/11] fix(kbfs): open the readiness gate after the last init check, and check it first in StartMigration The sync batch size validation ran after initReady(), so an invalid value released waiting requests and then failed init; released requests can't be recalled. Validate and set it before initReady(). StartMigration checked MDServer before readiness. MDServer is nil until init is ready, so it returned "no mdserver" instead of the transient errKBFSNotInitialized. Check readiness first, and cover it in TestInitSetsUpKBFSBeforeService. --- go/kbfs/libkbfs/init.go | 19 ++++++++++--------- go/kbfs/libkbfs/init_test.go | 2 ++ go/kbfs/libkbfs/keybase_service_base.go | 7 ++++--- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/go/kbfs/libkbfs/init.go b/go/kbfs/libkbfs/init.go index 3a9340ba4291..a4e7d1310fb2 100644 --- a/go/kbfs/libkbfs/init.go +++ b/go/kbfs/libkbfs/init.go @@ -984,8 +984,17 @@ func doInit( } } + if params.BGFlushDirOpBatchSize < 1 { + return nil, fmt.Errorf( + "Illegal sync batch size: %d", params.BGFlushDirOpBatchSize) + } + log.CDebugf(ctx, "Enabling a dir op batch size of %d", + params.BGFlushDirOpBatchSize) + config.SetBGFlushDirOpBatchSize(params.BGFlushDirOpBatchSize) + // Requests on the service connection have what they need from here on. - // Don't hold them for journaling, which can take a while. + // Don't hold them for journaling, which can take a while. Nothing after + // this point may fail init, since released requests can't be recalled. kbfsOps.initReady() ctx60s, cancel := context.WithTimeout(ctx, 60*time.Second) @@ -1002,14 +1011,6 @@ func doInit( log.CDebugf(ctx, "Journaling enabled") } - if params.BGFlushDirOpBatchSize < 1 { - return nil, fmt.Errorf( - "Illegal sync batch size: %d", params.BGFlushDirOpBatchSize) - } - log.CDebugf(ctx, "Enabling a dir op batch size of %d", - params.BGFlushDirOpBatchSize) - config.SetBGFlushDirOpBatchSize(params.BGFlushDirOpBatchSize) - if config.Mode().OldStorageRootCleaningEnabled() { go cleanOldTempStorageRoots(config) } diff --git a/go/kbfs/libkbfs/init_test.go b/go/kbfs/libkbfs/init_test.go index c5be3fdaf94c..a9333d6437de 100644 --- a/go/kbfs/libkbfs/init_test.go +++ b/go/kbfs/libkbfs/init_test.go @@ -159,6 +159,8 @@ func (c *initOrderCn) callIntoKBFS() { // Until init is ready, requests get an error or wait. _, err = c.daemon.GetTLFCryptKeys(ctx, keybase1.TLFQuery{TlfName: "testuser"}) require.Equal(t, errKBFSNotInitialized{}, err) + err = c.daemon.StartMigration(ctx, keybase1.Folder{Name: "testuser"}) + require.Equal(t, errKBFSNotInitialized{}, err) waitCtx, cancel := context.WithTimeout(ctx, 20*time.Millisecond) defer cancel() _, err = c.gated(waitCtx, nil) diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index 9fa035e59b94..1f16a06a3663 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -1518,13 +1518,14 @@ func (k *KeybaseDaemonRPC) TeamTreeMembershipsDone(context.Context, func (k *KeybaseServiceBase) StartMigration(ctx context.Context, folder keybase1.Folder, ) (err error) { + // Before init is ready, MDServer is nil too; report the transient error. + if !kbfsReady(k.config) { + return errKBFSNotInitialized{} + } mdServer := k.config.MDServer() if mdServer == nil { return errors.New("no mdserver") } - if !kbfsReady(k.config) { - return errKBFSNotInitialized{} - } // Making a favorite here to reuse the code that converts from // `keybase1.FolderType` into `tlf.Type`. fav := favorites.NewFolderFromProtocol(folder) From ae8d6bc6ab7d3eb816b9a0819a70ca0db020b0a5 Mon Sep 17 00:00:00 2001 From: Joshua Blum Date: Fri, 11 Sep 2026 12:42:47 -0400 Subject: [PATCH 10/11] fixes --- go/kbfs/libkbfs/favorites.go | 101 +++++++++++------------- go/kbfs/libkbfs/init_test.go | 3 +- go/kbfs/libkbfs/kbfs_ops.go | 63 +++++++++------ go/kbfs/libkbfs/kbpki_client.go | 44 +++++++++-- go/kbfs/libkbfs/keybase_daemon_rpc.go | 3 + go/kbfs/libkbfs/keybase_service_base.go | 8 +- 6 files changed, 133 insertions(+), 89 deletions(-) diff --git a/go/kbfs/libkbfs/favorites.go b/go/kbfs/libkbfs/favorites.go index 240a01f33289..8d98341c02b8 100644 --- a/go/kbfs/libkbfs/favorites.go +++ b/go/kbfs/libkbfs/favorites.go @@ -70,6 +70,7 @@ type favReq struct { favs chan<- []favorites.Folder favsAll chan<- keybase1.FavoritesResult homeTLFInfo *homeTLFInfo + loadDisk bool // For asynchronous refreshes, pass in the Favorites from the server here favResult *keybase1.FavoritesResult @@ -130,6 +131,7 @@ type Favorites struct { shutdownChan chan struct{} muShutdown sync.RWMutex shutdown bool + loopOnce sync.Once } func newFavoritesWithChan(config Config, reqChan chan *favReq) *Favorites { @@ -157,10 +159,19 @@ func newFavoritesWithChan(config Config, reqChan chan *favReq) *Favorites { bufferedInterval: defaultFavoritesBufferedReqInterval, shutdownChan: make(chan struct{}), } - return f } +func (f *Favorites) startLoop() { + if f.disabled { + return + } + f.loopOnce.Do(func() { + f.loopWG.Add(1) + go f.loop() + }) +} + // NewFavorites constructs a new Favorites instance. func NewFavorites(config Config) *Favorites { return newFavoritesWithChan(config, make(chan *favReq, 100)) @@ -290,27 +301,25 @@ func (f *Favorites) writeCacheToDisk(ctx context.Context) error { // InitForTest starts the Favorites cache's internal processing loop without // loading cached favorites from disk. func (f *Favorites) InitForTest() { - if f.disabled { - return - } - go f.loop() + f.startLoop() } -// Initialize loads the favorites cache from disk and starts listening for -// requests asynchronously. +// Initialize starts the processing loop and loads the favorites cache from +// disk. Other methods start the loop on first use so notifications that +// arrive before this can still be processed. func (f *Favorites) Initialize(ctx context.Context) { if f.disabled { return } - // load cache from disk - err := f.readCacheFromDisk(ctx) - if err != nil { + req := &favReq{ + ctx: ctx, + loadDisk: true, + done: make(chan struct{}), + } + if err := f.sendReq(ctx, req); err != nil { f.log.CWarningf( ctx, "Failed to read cached favorites from disk: %v", err) } - - // launch background loop - go f.loop() } func (f *Favorites) closeReq(req *favReq, err error) { @@ -441,6 +450,10 @@ func (f *Favorites) handleReq(req *favReq) (err error) { } }() + if req.loadDisk { + return f.readCacheFromDisk(req.ctx) + } + if req.refresh && !req.buffered { <-f.refreshWaiting } @@ -674,7 +687,6 @@ func (f *Favorites) handleReq(req *favReq) (err error) { } func (f *Favorites) loop() { - f.loopWG.Add(1) defer f.loopWG.Done() bufferedTicker := time.NewTicker(f.bufferedInterval) defer bufferedTicker.Stop() @@ -759,13 +771,20 @@ func (f *Favorites) waitOnReq(ctx context.Context, } } -func (f *Favorites) sendReq(ctx context.Context, req *favReq) error { +func (f *Favorites) enqueue(ctx context.Context, req *favReq) error { + f.startLoop() f.wg.Add(1) select { case f.reqChan <- req: + return nil case <-ctx.Done(): f.wg.Done() - err := ctx.Err() + return ctx.Err() + } +} + +func (f *Favorites) sendReq(ctx context.Context, req *favReq) error { + if err := f.enqueue(ctx, req); err != nil { f.closeReq(req, err) return err } @@ -835,14 +854,8 @@ func (f *Favorites) AddAsync(ctx context.Context, fav favorites.ToAdd) { // if the original context is canceled. req, doSend := f.startOrJoinAddReq(context.Background(), fav) if doSend { - f.wg.Add(1) - select { - case f.reqChan <- req: - case <-ctx.Done(): - f.wg.Done() - err := ctx.Err() + if err := f.enqueue(ctx, req); err != nil { f.closeReq(req, err) - return } } } @@ -914,36 +927,27 @@ func (f *Favorites) RefreshCache(ctx context.Context, mode FavoritesRefreshMode) done: make(chan struct{}), ctx: context.Background(), } - f.wg.Add(1) if mode == FavoritesRefreshModeBlocking { favResult, err := f.config.KBPKI().FavoriteList(ctx) if err != nil { f.log.CDebugf(ctx, "Failed to refresh cached Favorites: %+v", err) - // Because the request will not make it to the main processing - // loop, mark it as done and clear the refresh channel here. - f.wg.Done() <-f.refreshWaiting return } req.favResult = &favResult } - select { - case f.reqChan <- req: - go func() { - <-req.done - if req.err != nil { - f.log.CDebugf(ctx, "Failed to refresh cached Favorites ("+ - "error in main loop): %+v", req.err) - } - }() - case <-ctx.Done(): - // Because the request will not make it to the main processing - // loop, mark it as done and clear the refresh channel here. - f.wg.Done() + if err := f.enqueue(ctx, req); err != nil { <-f.refreshWaiting return } + go func() { + <-req.done + if req.err != nil { + f.log.CDebugf(ctx, "Failed to refresh cached Favorites ("+ + "error in main loop): %+v", req.err) + } + }() } // RefreshCacheWhenMTimeChanged refreshes the cached favorites, but @@ -960,6 +964,7 @@ func (f *Favorites) RefreshCacheWhenMTimeChanged( if f.disabled || f.shutdown { return } + f.startLoop() req := &favReq{ refresh: true, @@ -1003,13 +1008,7 @@ func (f *Favorites) ClearCache(ctx context.Context) { done: make(chan struct{}), ctx: context.Background(), } - f.wg.Add(1) - select { - case f.reqChan <- req: - case <-ctx.Done(): - f.wg.Done() - return - } + _ = f.enqueue(ctx, req) } // GetFolderWithFavFlags returns the a FolderWithFavFlags for give folder, if found. @@ -1086,13 +1085,7 @@ func (f *Favorites) setHomeTLFInfo(ctx context.Context, info homeTLFInfo) { done: make(chan struct{}), ctx: context.Background(), } - f.wg.Add(1) - select { - case f.reqChan <- req: - case <-ctx.Done(): - f.wg.Done() - return - } + _ = f.enqueue(ctx, req) } // GetAll returns the logged-in user's list of favorite, new, and ignored TLFs. diff --git a/go/kbfs/libkbfs/init_test.go b/go/kbfs/libkbfs/init_test.go index a9333d6437de..95486431d326 100644 --- a/go/kbfs/libkbfs/init_test.go +++ b/go/kbfs/libkbfs/init_test.go @@ -111,6 +111,7 @@ func (c *initOrderCn) NewKeybaseService( c.config = config name := kbname.NormalizedUsername("fake username") c.daemon = newKeybaseDaemonRPC(config, nil, log) + config.SetKeybaseService(c.daemon) c.daemon.fillClients(&fakeKeybaseClient{session: idutil.SessionInfo{ Name: name, UID: keybase1.MakeTestUID(1), @@ -187,7 +188,7 @@ func TestInitSetsUpKBFSBeforeService(t *testing.T) { }) params := DefaultInitParams(kbCtx) params.StorageRoot = kbCtx.dataDir - params.DiskCacheMode = DiskCacheModeOff + params.DiskCacheMode = DiskCacheModeLocal params.EnableJournal = false _, err := doInit( diff --git a/go/kbfs/libkbfs/kbfs_ops.go b/go/kbfs/libkbfs/kbfs_ops.go index 45e4a91f9407..16b94b1f1f95 100644 --- a/go/kbfs/libkbfs/kbfs_ops.go +++ b/go/kbfs/libkbfs/kbfs_ops.go @@ -50,12 +50,13 @@ type KBFSOpsStandard struct { // Closing this channel will shutdown the reidentification // watcher. reIdentifyControlChan chan chan<- struct{} - initDoneCh <-chan struct{} - // initFailedCh is closed instead of initDoneCh if init fails. + initDoneCh <-chan struct{} + // Closed once, under initMu: ready if requests can run, failed if + // init failed before that. initFailed is a no-op after ready. + initReadyCh chan struct{} initFailedCh chan struct{} - // initReadyCh is closed once config has what requests need, ahead of - // the slow tail of init (journaling). - initReadyCh chan struct{} + initMu sync.Mutex + initSignaled bool favs *Favorites @@ -106,8 +107,8 @@ func NewKBFSOpsStandard( opsByFav: make(map[favorites.Folder]*folderBranchOps), reIdentifyControlChan: make(chan chan<- struct{}), initDoneCh: initDoneCh, - initFailedCh: make(chan struct{}), initReadyCh: make(chan struct{}), + initFailedCh: make(chan struct{}), favs: NewFavorites(config), syncedTlfObservers: newSyncedTlfObserverList(), longOperationDebugDumper: NewImpatientDebugDumper( @@ -119,43 +120,55 @@ func NewKBFSOpsStandard( return kops } +func (fs *KBFSOpsStandard) signalInit(ready bool) { + fs.initMu.Lock() + defer fs.initMu.Unlock() + if fs.initSignaled { + return + } + fs.initSignaled = true + if ready { + close(fs.initReadyCh) + } else { + close(fs.initFailedCh) + } +} + // initReady tells anything in waitForReady that config now has what requests // need, ahead of the rest of init. func (fs *KBFSOpsStandard) initReady() { - close(fs.initReadyCh) + fs.signalInit(true) } -// initFailed tells anything waiting on init that it won't finish. +// initFailed tells anything waiting on init that it won't finish. No-op if +// initReady already ran: in-flight requests cannot be recalled. func (fs *KBFSOpsStandard) initFailed() { - close(fs.initFailedCh) + fs.signalInit(false) } -// waitForReady blocks until init has set up what requests need (initReady, or -// init finishing), and returns errKBFSNotInitialized if init failed. A nil -// initDoneCh (KBFSOps built outside init, as in tests) counts as ready. -func (fs *KBFSOpsStandard) waitForReady(ctx context.Context) error { +// ready reports, without blocking, whether init has set up what requests need. +func (fs *KBFSOpsStandard) ready() bool { if fs.initDoneCh == nil { - return nil + return true } - // Check in priority order first, so a failed init always errors and an - // already-canceled ctx can't win the select below. select { - case <-fs.initFailedCh: - return errKBFSNotInitialized{} + case <-fs.initReadyCh: + return true default: + return false } - select { - case <-fs.initReadyCh: - return nil - case <-fs.initDoneCh: +} + +// waitForReady blocks until init has set up what requests need, and returns +// errKBFSNotInitialized if init failed. A nil initDoneCh (KBFSOps built +// outside init, as in tests) counts as ready. +func (fs *KBFSOpsStandard) waitForReady(ctx context.Context) error { + if fs.initDoneCh == nil { return nil - default: } select { case <-fs.initReadyCh: return nil - case <-fs.initDoneCh: - return nil case <-fs.initFailedCh: return errKBFSNotInitialized{} case <-ctx.Done(): diff --git a/go/kbfs/libkbfs/kbpki_client.go b/go/kbfs/libkbfs/kbpki_client.go index 80ea4d41c5f5..6cc40f823d55 100644 --- a/go/kbfs/libkbfs/kbpki_client.go +++ b/go/kbfs/libkbfs/kbpki_client.go @@ -53,12 +53,24 @@ func NewKBPKIClient( return &KBPKIClient{serviceOwner, log, cache} } +func (k *KBPKIClient) service() (KeybaseService, error) { + s := k.serviceOwner.KeybaseService() + if s == nil { + return nil, errKBFSNotInitialized{} + } + return s, nil +} + // GetCurrentSession implements the KBPKI interface for KBPKIClient. func (k *KBPKIClient) GetCurrentSession(ctx context.Context) ( idutil.SessionInfo, error, ) { + s, err := k.service() + if err != nil { + return idutil.SessionInfo{}, err + } const sessionID = 0 - return k.serviceOwner.KeybaseService().CurrentSession(ctx, sessionID) + return s.CurrentSession(ctx, sessionID) } // Resolve implements the KBPKI interface for KBPKIClient. @@ -67,7 +79,11 @@ func (k *KBPKIClient) Resolve( offline keybase1.OfflineAvailability) ( kbname.NormalizedUsername, keybase1.UserOrTeamID, error, ) { - return k.serviceOwner.KeybaseService().Resolve(ctx, assertion, offline) + s, err := k.service() + if err != nil { + return kbname.NormalizedUsername(""), keybase1.UserOrTeamID(""), err + } + return s.Resolve(ctx, assertion, offline) } // Identify implements the KBPKI interface for KBPKIClient. @@ -76,7 +92,11 @@ func (k *KBPKIClient) Identify( offline keybase1.OfflineAvailability) ( kbname.NormalizedUsername, keybase1.UserOrTeamID, error, ) { - return k.serviceOwner.KeybaseService().Identify( + s, err := k.service() + if err != nil { + return kbname.NormalizedUsername(""), keybase1.UserOrTeamID(""), err + } + return s.Identify( ctx, assertion, reason, offline) } @@ -432,20 +452,32 @@ func (k *KBPKIClient) CreateTeamTLF( // FavoriteAdd implements the KBPKI interface for KBPKIClient. func (k *KBPKIClient) FavoriteAdd(ctx context.Context, folder keybase1.FolderHandle) error { - return k.serviceOwner.KeybaseService().FavoriteAdd(ctx, folder) + s, err := k.service() + if err != nil { + return err + } + return s.FavoriteAdd(ctx, folder) } // FavoriteDelete implements the KBPKI interface for KBPKIClient. func (k *KBPKIClient) FavoriteDelete(ctx context.Context, folder keybase1.FolderHandle) error { - return k.serviceOwner.KeybaseService().FavoriteDelete(ctx, folder) + s, err := k.service() + if err != nil { + return err + } + return s.FavoriteDelete(ctx, folder) } // FavoriteList implements the KBPKI interface for KBPKIClient. func (k *KBPKIClient) FavoriteList(ctx context.Context) ( keybase1.FavoritesResult, error, ) { + s, err := k.service() + if err != nil { + return keybase1.FavoritesResult{}, err + } const sessionID = 0 - return k.serviceOwner.KeybaseService().FavoriteList(ctx, sessionID) + return s.FavoriteList(ctx, sessionID) } // Notify implements the KBPKI interface for KBPKIClient. diff --git a/go/kbfs/libkbfs/keybase_daemon_rpc.go b/go/kbfs/libkbfs/keybase_daemon_rpc.go index 32f3b2c33dc4..f03592688725 100644 --- a/go/kbfs/libkbfs/keybase_daemon_rpc.go +++ b/go/kbfs/libkbfs/keybase_daemon_rpc.go @@ -120,6 +120,9 @@ func NewKeybaseDaemonRPC(config Config, kbCtx Context, log logger.Logger, if debug { k.daemonLog.Configure("", true, "") } + // Handlers in OnConnect can run before this constructor returns, and + // KBPKI reaches the daemon through config. + config.SetKeybaseService(k) conn := NewSharedKeybaseConnection(kbCtx, config, k) k.fillClients(conn.GetClient()) k.shutdownFn = conn.Shutdown diff --git a/go/kbfs/libkbfs/keybase_service_base.go b/go/kbfs/libkbfs/keybase_service_base.go index 1f16a06a3663..b60a05f45104 100644 --- a/go/kbfs/libkbfs/keybase_service_base.go +++ b/go/kbfs/libkbfs/keybase_service_base.go @@ -441,6 +441,7 @@ func (errKBFSNotInitialized) Error() string { return "KBFS is not initialized ye // that connection use it. type kbfsInitWaiter interface { waitForReady(ctx context.Context) error + ready() bool } // waitForKBFSReady blocks until init has set up what requests need, and @@ -456,9 +457,10 @@ func waitForKBFSReady(ctx context.Context, config Config) error { // need. Service-initiated requests check this rather than waiting, so none of // them can block on an init that is itself waiting on the service. func kbfsReady(config Config) bool { - ctx, cancel := context.WithCancel(context.Background()) - cancel() - return waitForKBFSReady(ctx, config) == nil + if w, ok := config.KBFSOps().(kbfsInitWaiter); ok { + return w.ready() + } + return true } // StartReachability implements keybase1.ReachabilityInterface. From a821fbbc8d6c154c2370ca74ac1c14c7a32aa7a6 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 14 Sep 2026 14:36:44 -0400 Subject: [PATCH 11/11] style(kbfs): gofmt kbfs_ops.go --- go/kbfs/libkbfs/kbfs_ops.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/kbfs/libkbfs/kbfs_ops.go b/go/kbfs/libkbfs/kbfs_ops.go index 16b94b1f1f95..7e2d18f97f4f 100644 --- a/go/kbfs/libkbfs/kbfs_ops.go +++ b/go/kbfs/libkbfs/kbfs_ops.go @@ -50,7 +50,7 @@ type KBFSOpsStandard struct { // Closing this channel will shutdown the reidentification // watcher. reIdentifyControlChan chan chan<- struct{} - initDoneCh <-chan struct{} + initDoneCh <-chan struct{} // Closed once, under initMu: ready if requests can run, failed if // init failed before that. initFailed is a no-op after ready. initReadyCh chan struct{}