When removing an account, wait for the last account reference has gone away before removing the files.

The intent to remove the account is stored in the database. At startup, if
there are any such referenes, they are applied by removing the account
directories and the entry in the database. This ensures the account directory
is properly removed, even on incomplete shutdown.

Don't add an account when its directory already exits.

admin/admin.go

@@ -10,6 +10,7 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
+	"io/fs"
 	"log/slog"
 	"os"
 	"path/filepath"
@@ -661,6 +662,15 @@ func AccountAdd(ctx context.Context, account, address string) (rerr error) {
 		return fmt.Errorf("%w: account already present", ErrRequest)
 	}
 
+	// Ensure the directory does not exist, e.g. due to pending account removal, or an
+	// otherwise failed cleanup.
+	accountDir := filepath.Join(mox.DataDirPath("accounts"), account)
+	if _, err := os.Stat(accountDir); err == nil {
+		return fmt.Errorf("%w: account directory %q already/still exists", ErrRequest, accountDir)
+	} else if !errors.Is(err, fs.ErrNotExist) {
+		return fmt.Errorf(`%w: stat account directory %q, expected "does not exist": %v`, ErrRequest, accountDir, err)
+	}
+
 	if err := checkAddressAvailable(addr); err != nil {
 		return fmt.Errorf("%w: address not available: %v", ErrRequest, err)
 	}
@@ -690,12 +700,20 @@ func AccountRemove(ctx context.Context, account string) (rerr error) {
 		}
 	}()
 
+	// Open account now. The deferred Close MUST happen after the dynamic unlock,
+	// because during tests the consistency checker takes the same lock.
+	acc, err := store.OpenAccount(log, account, false)
+	if err != nil {
+		return fmt.Errorf("%w: open account: %v", ErrRequest, err)
+	}
+	defer func() {
+		err := acc.Close()
+		log.Check(err, "closing account after error")
+	}()
+
 	defer mox.Conf.DynamicLockUnlock()()
 
 	c := mox.Conf.Dynamic
-	if _, ok := c.Accounts[account]; !ok {
-		return fmt.Errorf("%w: account does not exist", ErrRequest)
-	}
 
 	// Compose new config without modifying existing data structures. If we fail, we
 	// leave no trace.
@@ -711,28 +729,11 @@ func AccountRemove(ctx context.Context, account string) (rerr error) {
 		return fmt.Errorf("writing domains.conf: %w", err)
 	}
 
-	odir := filepath.Join(mox.DataDirPath("accounts"), account)
-	tmpdir := filepath.Join(mox.DataDirPath("tmp"), "oldaccount-"+account)
-	if err := os.Rename(odir, tmpdir); err != nil {
-		log.Errorx("moving old account data directory out of the way", err, slog.String("account", account))
-		return fmt.Errorf("account removed, but account data directory %q could not be moved out of the way: %v", odir, err)
-	}
-	if err := os.RemoveAll(tmpdir); err != nil {
-		log.Errorx("removing old account data directory", err, slog.String("account", account))
-		return fmt.Errorf("account removed, its data directory moved to %q, but removing failed: %v", odir, err)
+	if err := acc.Remove(context.Background()); err != nil {
+		return fmt.Errorf("account removed from configuration file, but scheduling account directory for removal failed: %v", err)
 	}
 
-	if err := store.TLSPublicKeyRemoveForAccount(context.Background(), account); err != nil {
-		log.Errorx("removing tls public keys for removed account", err)
-		return fmt.Errorf("account removed, but removing tls public keys failed: %v", err)
-	}
-
-	if err := store.LoginAttemptRemoveAccount(context.Background(), account); err != nil {
-		log.Errorx("removing historic login attempts for removed account", err)
-		return fmt.Errorf("account removed, but removing historic login attempts failed: %v", err)
-	}
-
-	log.Info("account removed", slog.String("account", account))
+	log.Info("account marked for removal", slog.String("account", account))
 	return nil
 }
 

store/account.go

@@ -939,8 +939,9 @@ type Account struct {
 
 	// Reference count, while >0, this account is alive and shared. Protected by
 	// openAccounts, not by account wlock.
-	nused  int
-	closed chan struct{} // Closed when last reference is gone.
+	nused   int
+	removed bool          // Marked for removal. Last close removes the account directory.
+	closed  chan struct{} // Closed when last reference is gone.
 }
 
 type Upgrade struct {
@@ -972,20 +973,34 @@ var openAccounts = struct {
 }
 
 func closeAccount(acc *Account) (rerr error) {
+	// If we need to remove the account files, we do so without the accounts lock.
+	remove := false
+	defer func() {
+		if remove {
+			log := mlog.New("store", nil)
+			err := removeAccount(log, acc.Name)
+			if rerr == nil {
+				rerr = err
+			}
+			close(acc.closed)
+		}
+	}()
+
 	openAccounts.Lock()
 	defer openAccounts.Unlock()
 	acc.nused--
 	if acc.nused > 0 {
 		return
 	}
-
-	// threadsCompleted must be closed now because it increased nused.
+	remove = acc.removed
 
 	defer func() {
 		err := acc.DB.Close()
 		acc.DB = nil
 		delete(openAccounts.names, acc.Name)
-		close(acc.closed)
+		if !remove {
+			close(acc.closed)
+		}
 
 		if rerr == nil {
 			rerr = err
@@ -999,9 +1014,51 @@ func closeAccount(acc *Account) (rerr error) {
 	} else if len(l) > 0 {
 		return fmt.Errorf("messageerase records still present after last account reference is gone: %v", l)
 	}
+
 	return nil
 }
 
+// removeAccount moves the account directory for an account away and removes
+// all files, and removes the AccountRemove struct from the database.
+func removeAccount(log mlog.Log, accountName string) error {
+	log = log.With(slog.String("account", accountName))
+	log.Info("removing account directory and files")
+
+	// First move the account directory away.
+	odir := filepath.Join(mox.DataDirPath("accounts"), accountName)
+	tmpdir := filepath.Join(mox.DataDirPath("tmp"), "oldaccount-"+accountName)
+	if err := os.Rename(odir, tmpdir); err != nil {
+		return fmt.Errorf("moving account data directory %q out of the way to %q (account not removed): %v", odir, tmpdir, err)
+	}
+
+	var errs []error
+
+	// Commit removal to database.
+	err := AuthDB.Write(context.Background(), func(tx *bstore.Tx) error {
+		if err := tx.Delete(&AccountRemove{accountName}); err != nil {
+			return fmt.Errorf("deleting account removal request: %v", err)
+		}
+		if err := tlsPublicKeyRemoveForAccount(tx, accountName); err != nil {
+			return fmt.Errorf("removing tls public keys for account: %v", err)
+		}
+
+		if err := loginAttemptRemoveAccount(tx, accountName); err != nil {
+			return fmt.Errorf("removing historic login attempts for account: %v", err)
+		}
+		return nil
+	})
+	if err != nil {
+		errs = append(errs, fmt.Errorf("remove account from database: %w", err))
+	}
+
+	// Remove the account directory and its message and other files.
+	if err := os.RemoveAll(tmpdir); err != nil {
+		errs = append(errs, fmt.Errorf("removing account data directory %q that was moved to %q: %v", odir, tmpdir, err))
+	}
+
+	return errors.Join(errs...)
+}
+
 // OpenAccount opens an account by name.
 //
 // No additional data path prefix or ".db" suffix should be added to the name.
@@ -1010,6 +1067,10 @@ func OpenAccount(log mlog.Log, name string, checkLoginDisabled bool) (*Account,
 	openAccounts.Lock()
 	defer openAccounts.Unlock()
 	if acc, ok := openAccounts.names[name]; ok {
+		if acc.removed {
+			return nil, fmt.Errorf("account has been removed")
+		}
+
 		acc.nused++
 		return acc, nil
 	}
@@ -1077,6 +1138,7 @@ func OpenAccountDB(log mlog.Log, accountDir, accountName string) (a *Account, re
 		if err := initAccount(db); err != nil {
 			return nil, fmt.Errorf("initializing account: %v", err)
 		}
+
 		close(acc.threadsCompleted)
 		return acc, nil
 	}
@@ -1576,6 +1638,20 @@ func initAccount(db *bstore.DB) error {
 	})
 }
 
+// Remove schedules an account for removal. New opens will fail. When the last
+// reference is closed, the account files are removed.
+func (a *Account) Remove(ctx context.Context) error {
+	openAccounts.Lock()
+	defer openAccounts.Unlock()
+
+	if err := AuthDB.Insert(ctx, &AccountRemove{AccountName: a.Name}); err != nil {
+		return fmt.Errorf("inserting account removal: %w", err)
+	}
+	a.removed = true
+
+	return nil
+}
+
 // WaitClosed waits until the last reference to this account is gone and the
 // account is closed. Used during tests, to ensure the consistency checks run after
 // expunged messages have been erased.

store/account_test.go

@@ -2,6 +2,8 @@ package store
 
 import (
 	"context"
+	"errors"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -411,3 +413,72 @@ func TestNextMessageID(t *testing.T) {
 	tcheck(t, err, "closing account")
 	acc.WaitClosed()
 }
+
+func TestRemove(t *testing.T) {
+	log := mlog.New("store", nil)
+	os.RemoveAll("../testdata/store/data")
+	mox.ConfigStaticPath = filepath.FromSlash("../testdata/store/mox.conf")
+	mox.MustLoadConfig(true, false)
+	err := Init(ctxbg)
+	tcheck(t, err, "init")
+	defer func() {
+		err := Close()
+		tcheck(t, err, "close")
+	}()
+	defer Switchboard()()
+
+	// Note: we are not removing the account from the config file. Nothing currently
+	// has a problem with that.
+
+	// Ensure account exists.
+	acc, err := OpenAccount(log, "mjl", false)
+	tcheck(t, err, "open account")
+
+	// Mark account removed. It will only be removed when we close the account.
+	err = acc.Remove(context.Background())
+	tcheck(t, err, "remove account")
+
+	p := filepath.Join(mox.DataDirPath("accounts"), "mjl")
+	_, err = os.Stat(p)
+	tcheck(t, err, "stat account dir")
+
+	err = acc.Close()
+	tcheck(t, err, "closing account")
+	acc.WaitClosed()
+	acc = nil
+
+	if _, err := os.Stat(p); err == nil || !errors.Is(err, fs.ErrNotExist) {
+		t.Fatalf(`got stat err %v for account directory, expected "does not exist"`, err)
+	}
+
+	// Recreate files and directories. We will reinitialize store/ without closing our
+	// account reference. This will apply the account removal. We only drop our (now
+	// broken) account reference when done.
+	acc, err = OpenAccount(log, "mjl", false)
+	tcheck(t, err, "open account")
+	defer func() {
+		acc.Close() // Ignore errors.
+		acc.WaitClosed()
+		CheckConsistencyOnClose = true
+	}()
+
+	// Init below will remove the directory, we are no longer consistent.
+	CheckConsistencyOnClose = false
+
+	err = acc.Remove(context.Background())
+	tcheck(t, err, "remove account")
+
+	_, err = os.Stat(p)
+	tcheck(t, err, "stat account dir")
+
+	err = Close()
+	tcheck(t, err, "close store")
+	err = Init(ctxbg)
+	tcheck(t, err, "init store")
+	if _, err := os.Stat(p); err == nil || !errors.Is(err, fs.ErrNotExist) {
+		t.Fatalf(`got stat err %v for account directory, expected "does not exist"`, err)
+	}
+	exists, err := bstore.QueryDB[AccountRemove](ctxbg, AuthDB).Exists()
+	tcheck(t, err, "checking for account removals")
+	tcompare(t, exists, false)
+}

store/init.go

@@ -17,9 +17,15 @@ import (
 	"github.com/mjl-/mox/moxvar"
 )
 
+// AccountRemove represents the scheduled removal of an account, when its last
+// reference goes away.
+type AccountRemove struct {
+	AccountName string
+}
+
 // AuthDB and AuthDBTypes are exported for ../backup.go.
 var AuthDB *bstore.DB
-var AuthDBTypes = []any{TLSPublicKey{}, LoginAttempt{}, LoginAttemptState{}}
+var AuthDBTypes = []any{TLSPublicKey{}, LoginAttempt{}, LoginAttemptState{}, AccountRemove{}}
 
 var loginAttemptCleanerStop chan chan struct{}
 
@@ -38,6 +44,18 @@ func Init(ctx context.Context) error {
 		return err
 	}
 
+	// List pending account removals, and process them one by one, committing each
+	// individually.
+	removals, err := bstore.QueryDB[AccountRemove](ctx, AuthDB).List()
+	if err != nil {
+		return fmt.Errorf("listing scheduled account removals: %v", err)
+	}
+	for _, removal := range removals {
+		if err := removeAccount(pkglog, removal.AccountName); err != nil {
+			pkglog.Errorx("removing old account", err, slog.String("account", removal.AccountName))
+		}
+	}
+
 	startLoginAttemptWriter()
 	loginAttemptCleanerStop = make(chan chan struct{})
 

store/loginattempt.go

@@ -313,15 +313,13 @@ func LoginAttemptCleanup(ctx context.Context) error {
 	})
 }
 
-// LoginAttemptRemoveAccount removes all LoginAttempt records for an account
+// loginAttemptRemoveAccount removes all LoginAttempt records for an account
 // (value must be non-empty).
-func LoginAttemptRemoveAccount(ctx context.Context, accountName string) error {
-	return AuthDB.Write(ctx, func(tx *bstore.Tx) error {
-		q := bstore.QueryTx[LoginAttempt](tx)
-		q.FilterNonzero(LoginAttempt{AccountName: accountName})
-		_, err := q.Delete()
-		return err
-	})
+func loginAttemptRemoveAccount(tx *bstore.Tx, accountName string) error {
+	q := bstore.QueryTx[LoginAttempt](tx)
+	q.FilterNonzero(LoginAttempt{AccountName: accountName})
+	_, err := q.Delete()
+	return err
 }
 
 // LoginAttemptList returns LoginAttempt records for the accountName. If

store/loginattempt_test.go

@@ -8,6 +8,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/mjl-/bstore"
+
 	"github.com/mjl-/mox/mox-"
 )
 
@@ -87,7 +89,9 @@ func TestLoginAttempt(t *testing.T) {
 	tcompare(t, len(l), 2)
 
 	// Removing account will keep last entry for mjl2.
-	err = LoginAttemptRemoveAccount(ctxbg, "mjl1")
+	err = AuthDB.Write(ctxbg, func(tx *bstore.Tx) error {
+		return loginAttemptRemoveAccount(tx, "mjl1")
+	})
 	tcheck(t, err, "remove login attempt account")
 	l, err = LoginAttemptList(ctxbg, "", 0)
 	tcheck(t, err, "list login attempts")

store/tlspubkey.go

@@ -127,9 +127,9 @@ func TLSPublicKeyRemove(ctx context.Context, fingerprint string) error {
 	return AuthDB.Delete(ctx, &k)
 }
 
-// TLSPublicKeyRemoveForAccount removes all tls public keys for an account.
-func TLSPublicKeyRemoveForAccount(ctx context.Context, account string) error {
-	q := bstore.QueryDB[TLSPublicKey](ctx, AuthDB)
+// tlsPublicKeyRemoveForAccount removes all tls public keys for an account.
+func tlsPublicKeyRemoveForAccount(tx *bstore.Tx, account string) error {
+	q := bstore.QueryTx[TLSPublicKey](tx)
 	q.FilterNonzero(TLSPublicKey{Account: account})
 	_, err := q.Delete()
 	return err