Read /account from primary

This commit is contained in:
binwiederhier
2026-06-25 20:24:50 -04:00
parent df6f1f3ee1
commit 5e32f05302
10 changed files with 182 additions and 38 deletions
+9
View File
@@ -90,6 +90,15 @@ func (d *DB) ReadOnly() *sql.DB {
return d.primary.DB return d.primary.DB
} }
// MarkReplicasHealthyForTest immediately marks all configured replicas as healthy, bypassing the
// async health-check loop's initial delay. It exists so tests can deterministically route
// ReadOnly() to a replica without waiting; it is not used in production code.
func (d *DB) MarkReplicasHealthyForTest() {
for _, r := range d.replicas {
r.healthy.Store(true)
}
}
// Close closes the primary database and all replicas, and stops the health-check goroutine. // Close closes the primary database and all replicas, and stops the health-check goroutine.
func (d *DB) Close() error { func (d *DB) Close() error {
d.cancel() d.cancel()
+7
View File
@@ -1983,6 +1983,13 @@ and the [ntfy Android app](https://github.com/binwiederhier/ntfy-android/release
## Not released yet ## Not released yet
### ntfy server v2.26.x (UNRELEASED)
**Bug fixes + maintenance:**
* `GET /account` now reads from the primary database instead of a read replica, so the account view no longer shows stale data right after a change when replicas lag behind
* The web app no longer issues a redundant second `GET /account` after email actions (e.g. setting a primary email); it relies on the sync-topic event for the refresh
### ntfy Android v1.25.x (UNRELEASED) ### ntfy Android v1.25.x (UNRELEASED)
This release makes the "connection lost" alert configurable and turns it off by default. Folks did not like it and many reached out This release makes the "connection lost" alert configurable and turns it off by default. Folks did not like it and many reached out
+4 -8
View File
@@ -180,10 +180,6 @@ func (s *Server) handleAccountGet(w http.ResponseWriter, r *http.Request, v *vis
if err != nil { if err != nil {
return err return err
} }
primaryEmail, err := s.userManager.PrimaryEmail(u.ID)
if err != nil {
return err
}
pendingEmails, err := s.userManager.PendingEmails(u.ID) pendingEmails, err := s.userManager.PendingEmails(u.ID)
if err != nil { if err != nil {
return err return err
@@ -191,7 +187,7 @@ func (s *Server) handleAccountGet(w http.ResponseWriter, r *http.Request, v *vis
// Combine verified (with primary flag) and pending (unverified) into one list // Combine verified (with primary flag) and pending (unverified) into one list
emailInfos := make([]*apiAccountEmailInfo, 0, len(emails)+len(pendingEmails)) emailInfos := make([]*apiAccountEmailInfo, 0, len(emails)+len(pendingEmails))
for _, email := range emails { for _, email := range emails {
emailInfos = append(emailInfos, &apiAccountEmailInfo{Address: email, Primary: email == primaryEmail}) emailInfos = append(emailInfos, &apiAccountEmailInfo{Address: email.Address, Primary: email.Primary})
} }
for _, email := range pendingEmails { for _, email := range pendingEmails {
emailInfos = append(emailInfos, &apiAccountEmailInfo{Address: email, Pending: true}) emailInfos = append(emailInfos, &apiAccountEmailInfo{Address: email, Pending: true})
@@ -667,7 +663,7 @@ func (s *Server) handleAccountEmailAdd(w http.ResponseWriter, r *http.Request, v
emails, err := s.userManager.Emails(u.ID) emails, err := s.userManager.Emails(u.ID)
if err != nil { if err != nil {
return err return err
} else if util.Contains(emails, req.Email) { } else if emails.Contains(req.Email) {
return errHTTPConflictEmailExists return errHTTPConflictEmailExists
} }
// Rate limit (counts against the user's email quota) // Rate limit (counts against the user's email quota)
@@ -888,7 +884,7 @@ func (s *Server) convertEmailAddress(u *user.User, email string) (string, *errHT
if err != nil { if err != nil {
return "", errHTTPInternalError return "", errHTTPInternalError
} else if len(emails) > 0 { } else if len(emails) > 0 {
return emails[0], nil return emails[0].Address, nil
} }
return "", errHTTPBadRequestEmailAddressNotVerified return "", errHTTPBadRequestEmailAddressNotVerified
} }
@@ -903,7 +899,7 @@ func (s *Server) convertEmailAddress(u *user.User, email string) (string, *errHT
emails, err := s.userManager.Emails(u.ID) emails, err := s.userManager.Emails(u.ID)
if err != nil { if err != nil {
return "", errHTTPInternalError return "", errHTTPInternalError
} else if util.Contains(emails, email) { } else if emails.Contains(email) {
return email, nil return email, nil
} }
return "", errHTTPBadRequestEmailAddressNotVerified return "", errHTTPBadRequestEmailAddressNotVerified
+1 -1
View File
@@ -72,7 +72,7 @@ func TestPayments_Checkout_SendsBillingEmailVerification(t *testing.T) {
emails, err := s.userManager.Emails(u.ID) emails, err := s.userManager.Emails(u.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, []string{"billing@example.com"}, emails) require.Equal(t, []string{"billing@example.com"}, emails.Strings())
primary, err := s.userManager.PrimaryEmail(u.ID) primary, err := s.userManager.PrimaryEmail(u.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, "billing@example.com", primary) require.Equal(t, "billing@example.com", primary)
+26 -16
View File
@@ -941,7 +941,9 @@ func (a *Manager) RemoveReservations(username string, topics ...string) error {
// Reservations returns all user-owned topics, and the associated everyone-access // Reservations returns all user-owned topics, and the associated everyone-access
func (a *Manager) Reservations(username string) ([]Reservation, error) { func (a *Manager) Reservations(username string) ([]Reservation, error) {
return a.reservationsTx(a.db.ReadOnly(), username) // Read from the primary, not a replica: this backs GET /account, which the web app refetches
// immediately after a sync event. Replication lag would otherwise show stale data.
return a.reservationsTx(a.db, username)
} }
func (a *Manager) reservationsTx(tx db.Querier, username string) ([]Reservation, error) { func (a *Manager) reservationsTx(tx db.Querier, username string) ([]Reservation, error) {
@@ -1204,7 +1206,8 @@ func (a *Manager) Token(userID, token string) (*Token, error) {
// Tokens returns all existing tokens for the user with the given user ID // Tokens returns all existing tokens for the user with the given user ID
func (a *Manager) Tokens(userID string) ([]*Token, error) { func (a *Manager) Tokens(userID string) ([]*Token, error) {
rows, err := a.db.ReadOnly().Query(a.queries.selectTokens, userID) // Primary read: backs GET /account (read-your-writes after a sync event).
rows, err := a.db.Query(a.queries.selectTokens, userID)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@@ -1417,7 +1420,8 @@ func (a *Manager) readTier(rows *sql.Rows) (*Tier, error) {
// PhoneNumbers returns all phone numbers for the user with the given user ID // PhoneNumbers returns all phone numbers for the user with the given user ID
func (a *Manager) PhoneNumbers(userID string) ([]string, error) { func (a *Manager) PhoneNumbers(userID string) ([]string, error) {
rows, err := a.db.ReadOnly().Query(a.queries.selectPhoneNumbers, userID) // Primary read: backs GET /account (read-your-writes after a sync event).
rows, err := a.db.Query(a.queries.selectPhoneNumbers, userID)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@@ -1465,14 +1469,17 @@ func (a *Manager) readPhoneNumber(rows *sql.Rows) (string, error) {
return phoneNumber, nil return phoneNumber, nil
} }
// Emails returns all verified email addresses for the user with the given user ID // Emails returns all verified email addresses for the user with the given user ID, each carrying
func (a *Manager) Emails(userID string) ([]string, error) { // whether it is the primary (recovery) address. Because the primary flag is included, callers that
rows, err := a.db.ReadOnly().Query(a.queries.selectEmails, userID) // need it (e.g. the account view) do not need a separate PrimaryEmail call.
func (a *Manager) Emails(userID string) (Emails, error) {
// Primary read: backs GET /account (read-your-writes after a sync event).
rows, err := a.db.Query(a.queries.selectEmails, userID)
if err != nil { if err != nil {
return nil, err return nil, err
} }
defer rows.Close() defer rows.Close()
emails := make([]string, 0) emails := make(Emails, 0)
for { for {
email, err := a.readEmail(rows) email, err := a.readEmail(rows)
if errors.Is(err, ErrEmailNotFound) { if errors.Is(err, ErrEmailNotFound) {
@@ -1508,7 +1515,8 @@ func (a *Manager) RemoveEmail(userID, email string) error {
// the user has not designated one. // the user has not designated one.
func (a *Manager) PrimaryEmail(userID string) (string, error) { func (a *Manager) PrimaryEmail(userID string) (string, error) {
var email sql.NullString var email sql.NullString
err := a.db.ReadOnly().QueryRow(a.queries.selectPrimaryEmail, userID).Scan(&email) // Primary read: backs GET /account (read-your-writes after a sync event).
err := a.db.QueryRow(a.queries.selectPrimaryEmail, userID).Scan(&email)
if errors.Is(err, sql.ErrNoRows) { if errors.Is(err, sql.ErrNoRows) {
return "", nil return "", nil
} else if err != nil { } else if err != nil {
@@ -1534,7 +1542,8 @@ func (a *Manager) UserIDByPrimaryEmail(email string) (string, error) {
// PendingEmails returns the user's unverified (pending) email addresses, i.e. addresses with // PendingEmails returns the user's unverified (pending) email addresses, i.e. addresses with
// an outstanding email-verification magic link. // an outstanding email-verification magic link.
func (a *Manager) PendingEmails(userID string) ([]string, error) { func (a *Manager) PendingEmails(userID string) ([]string, error) {
rows, err := a.db.ReadOnly().Query(a.queries.selectPendingEmails, string(MagicLinkKindEmailVerify), userID) // Primary read: backs GET /account (read-your-writes after a sync event).
rows, err := a.db.Query(a.queries.selectPendingEmails, string(MagicLinkKindEmailVerify), userID)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@@ -1756,17 +1765,18 @@ func (a *Manager) deleteExpiredMagicLinks() error {
return err return err
} }
func (a *Manager) readEmail(rows *sql.Rows) (string, error) { func (a *Manager) readEmail(rows *sql.Rows) (*Email, error) {
var email string var address string
var primary bool
if !rows.Next() { if !rows.Next() {
return "", ErrEmailNotFound return nil, ErrEmailNotFound
} }
if err := rows.Scan(&email); err != nil { if err := rows.Scan(&address, &primary); err != nil {
return "", err return nil, err
} else if err := rows.Err(); err != nil { } else if err := rows.Err(); err != nil {
return "", err return nil, err
} }
return email, nil return &Email{Address: address, Primary: primary}, nil
} }
// ChangeBilling updates a user's billing fields // ChangeBilling updates a user's billing fields
+1 -1
View File
@@ -217,7 +217,7 @@ const (
postgresDeletePhoneNumberQuery = `DELETE FROM user_phone WHERE user_id = $1 AND phone_number = $2` postgresDeletePhoneNumberQuery = `DELETE FROM user_phone WHERE user_id = $1 AND phone_number = $2`
// Email queries // Email queries
postgresSelectEmailsQuery = `SELECT email FROM user_email WHERE user_id = $1 ORDER BY email` postgresSelectEmailsQuery = `SELECT email, is_primary FROM user_email WHERE user_id = $1 ORDER BY email`
postgresInsertEmailQuery = `INSERT INTO user_email (user_id, email) VALUES ($1, $2)` postgresInsertEmailQuery = `INSERT INTO user_email (user_id, email) VALUES ($1, $2)`
postgresInsertEmailIgnoreQuery = `INSERT INTO user_email (user_id, email) VALUES ($1, $2) ON CONFLICT (user_id, email) DO NOTHING` postgresInsertEmailIgnoreQuery = `INSERT INTO user_email (user_id, email) VALUES ($1, $2) ON CONFLICT (user_id, email) DO NOTHING`
postgresDeleteEmailQuery = `DELETE FROM user_email WHERE user_id = $1 AND email = $2` postgresDeleteEmailQuery = `DELETE FROM user_email WHERE user_id = $1 AND email = $2`
+1 -1
View File
@@ -214,7 +214,7 @@ const (
sqliteDeletePhoneNumberQuery = `DELETE FROM user_phone WHERE user_id = ? AND phone_number = ?` sqliteDeletePhoneNumberQuery = `DELETE FROM user_phone WHERE user_id = ? AND phone_number = ?`
// Email queries // Email queries
sqliteSelectEmailsQuery = `SELECT email FROM user_email WHERE user_id = ? ORDER BY email` sqliteSelectEmailsQuery = `SELECT email, is_primary FROM user_email WHERE user_id = ? ORDER BY email`
sqliteInsertEmailQuery = `INSERT INTO user_email (user_id, email) VALUES (?, ?)` sqliteInsertEmailQuery = `INSERT INTO user_email (user_id, email) VALUES (?, ?)`
sqliteInsertEmailIgnoreQuery = `INSERT INTO user_email (user_id, email) VALUES (?, ?) ON CONFLICT (user_id, email) DO NOTHING` sqliteInsertEmailIgnoreQuery = `INSERT INTO user_email (user_id, email) VALUES (?, ?) ON CONFLICT (user_id, email) DO NOTHING`
sqliteDeleteEmailQuery = `DELETE FROM user_email WHERE user_id = ? AND email = ?` sqliteDeleteEmailQuery = `DELETE FROM user_email WHERE user_id = ? AND email = ?`
+100 -6
View File
@@ -1157,7 +1157,7 @@ func TestUser_EmailAddListRemove(t *testing.T) {
emails, err := a.Emails(phil.ID) emails, err := a.Emails(phil.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, 1, len(emails)) require.Equal(t, 1, len(emails))
require.Equal(t, "phil@example.com", emails[0]) require.Equal(t, "phil@example.com", emails[0].Address)
require.Nil(t, a.RemoveEmail(phil.ID, "phil@example.com")) require.Nil(t, a.RemoveEmail(phil.ID, "phil@example.com"))
emails, err = a.Emails(phil.ID) emails, err = a.Emails(phil.ID)
@@ -2701,7 +2701,7 @@ func TestStoreEmails(t *testing.T) {
emails, err = manager.Emails(u.ID) emails, err = manager.Emails(u.ID)
require.Nil(t, err) require.Nil(t, err)
require.Len(t, emails, 1) require.Len(t, emails, 1)
require.Equal(t, "phil2@example.com", emails[0]) require.Equal(t, "phil2@example.com", emails[0].Address)
}) })
} }
@@ -2871,7 +2871,7 @@ func TestUser_MagicLink_VerifyEmail_SetsPrimary(t *testing.T) {
emails, err = a.Emails(phil.ID) emails, err = a.Emails(phil.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, []string{"phil@example.com"}, emails) require.Equal(t, []string{"phil@example.com"}, emails.Strings())
primary, err = a.PrimaryEmail(phil.ID) primary, err = a.PrimaryEmail(phil.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, "phil@example.com", primary) require.Equal(t, "phil@example.com", primary)
@@ -2904,7 +2904,7 @@ func TestUser_MagicLink_VerifyEmail_SecondStaysSecondary(t *testing.T) {
// Both verified, but primary is still the first // Both verified, but primary is still the first
emails, err := a.Emails(phil.ID) emails, err := a.Emails(phil.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, []string{"first@example.com", "second@example.com"}, emails) require.Equal(t, []string{"first@example.com", "second@example.com"}, emails.Strings())
primary, err := a.PrimaryEmail(phil.ID) primary, err := a.PrimaryEmail(phil.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, "first@example.com", primary) require.Equal(t, "first@example.com", primary)
@@ -2933,7 +2933,7 @@ func TestUser_MagicLink_PrimaryGlobalUniqueness(t *testing.T) {
require.Nil(t, err) require.Nil(t, err)
emails, err := a.Emails(ben.ID) emails, err := a.Emails(ben.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, []string{"shared@example.com"}, emails) require.Equal(t, []string{"shared@example.com"}, emails.Strings())
primary, err = a.PrimaryEmail(ben.ID) primary, err = a.PrimaryEmail(ben.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, "", primary) require.Equal(t, "", primary)
@@ -3168,7 +3168,7 @@ func TestUser_MagicLink_VerifyEmail_ProvisionedGetsPrimary(t *testing.T) {
emails, err := a.Emails(prov.ID) emails, err := a.Emails(prov.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, []string{"prov@example.com"}, emails) require.Equal(t, []string{"prov@example.com"}, emails.Strings())
primary, err := a.PrimaryEmail(prov.ID) primary, err := a.PrimaryEmail(prov.ID)
require.Nil(t, err) require.Nil(t, err)
require.Equal(t, "prov@example.com", primary) require.Equal(t, "prov@example.com", primary)
@@ -3219,3 +3219,97 @@ func TestUser_MagicLink_UserIDByPrimaryEmail_NotFound(t *testing.T) {
require.ErrorIs(t, err, ErrUserNotFound) require.ErrorIs(t, err, ErrUserNotFound)
}) })
} }
func TestManager_Emails_PrimaryFlagAndHelpers(t *testing.T) {
forEachBackend(t, func(t *testing.T, newManager newManagerFunc) {
a := newTestManager(t, newManager, PermissionDenyAll)
require.Nil(t, a.AddUser("phil", "phil", RoleUser, false))
u, err := a.User("phil")
require.Nil(t, err)
require.Nil(t, a.AddEmail(u.ID, "a@example.com"))
require.Nil(t, a.AddEmail(u.ID, "b@example.com"))
require.Nil(t, a.SetPrimaryEmail(u.ID, "b@example.com"))
// Emails() carries the primary flag, so a separate PrimaryEmail() call is unnecessary.
emails, err := a.Emails(u.ID)
require.Nil(t, err)
require.Len(t, emails, 2)
require.Equal(t, "a@example.com", emails[0].Address) // ORDER BY email
require.False(t, emails[0].Primary)
require.Equal(t, "b@example.com", emails[1].Address)
require.True(t, emails[1].Primary)
// Helper methods for the address-only callers
require.Equal(t, []string{"a@example.com", "b@example.com"}, emails.Strings())
require.True(t, emails.Contains("a@example.com"))
require.False(t, emails.Contains("c@example.com"))
})
}
// openReplicaTestSQLite opens a fresh SQLite database file with the user schema applied.
func openReplicaTestSQLite(t *testing.T, filename string) *sql.DB {
d, err := sql.Open("sqlite3", filename+"?_case_sensitive_like=on")
require.Nil(t, err)
require.Nil(t, setupSQLite(d))
return d
}
// TestManager_AccountReadsUsePrimary verifies that the per-user reads backing the GET /account
// endpoint read from the primary, not from a read replica. The sync event ("something changed")
// is published immediately after a write, so a replica that lags would make the account view
// stale right after the user changes it. These reads must therefore be read-your-writes consistent.
//
// The test wires up a primary and a deliberately-empty replica (simulating replication lag),
// forces the replica healthy so ReadOnly() would route to it, writes everything to the primary,
// and asserts the reads still observe the fresh primary data.
func TestManager_AccountReadsUsePrimary(t *testing.T) {
dir := t.TempDir()
primaryDB := openReplicaTestSQLite(t, filepath.Join(dir, "primary.db"))
replicaDB := openReplicaTestSQLite(t, filepath.Join(dir, "replica.db")) // intentionally left empty
pool := db.New(&db.Host{DB: primaryDB}, []*db.Host{{DB: replicaDB}})
pool.MarkReplicasHealthyForTest() // force ReadOnly() to route to the stale replica
a, err := newManager(pool, sqliteQueries, &Config{BcryptCost: bcrypt.MinCost})
require.Nil(t, err)
t.Cleanup(func() { a.Close() })
// All writes below go to the primary; the replica stays empty.
require.Nil(t, a.AddUser("phil", "phil", RoleUser, false))
u, err := a.User("phil")
require.Nil(t, err)
_, err = a.CreateToken(u.ID, "test token", time.Now().Add(time.Hour), netip.IPv4Unspecified(), false)
require.Nil(t, err)
require.Nil(t, a.AddReservation("phil", "mytopic", PermissionDenyAll, 10))
require.Nil(t, a.AddPhoneNumber(u.ID, "+12223334444"))
require.Nil(t, a.AddEmail(u.ID, "phil@example.com"))
require.Nil(t, a.SetPrimaryEmail(u.ID, "phil@example.com"))
_, err = a.AddMagicLink(MagicLinkKindEmailVerify, u.ID, "pending@example.com", time.Hour)
require.Nil(t, err)
// Each read must observe the just-written primary data, NOT the empty replica.
tokens, err := a.Tokens(u.ID)
require.Nil(t, err)
require.Len(t, tokens, 1)
reservations, err := a.Reservations("phil")
require.Nil(t, err)
require.Len(t, reservations, 1)
phoneNumbers, err := a.PhoneNumbers(u.ID)
require.Nil(t, err)
require.Len(t, phoneNumbers, 1)
emails, err := a.Emails(u.ID)
require.Nil(t, err)
require.Len(t, emails, 1)
require.Equal(t, "phil@example.com", emails[0].Address)
require.True(t, emails[0].Primary)
primaryEmail, err := a.PrimaryEmail(u.ID)
require.Nil(t, err)
require.Equal(t, "phil@example.com", primaryEmail)
pendingEmails, err := a.PendingEmails(u.ID)
require.Nil(t, err)
require.Len(t, pendingEmails, 1)
}
+30
View File
@@ -182,6 +182,36 @@ type Reservation struct {
Everyone Permission Everyone Permission
} }
// Email is a verified email address on a user account, along with whether it is the user's
// designated primary (recovery) address.
type Email struct {
Address string
Primary bool
}
// Emails is a list of verified email addresses for a user.
type Emails []*Email
// Strings returns just the address strings, in the same order. It is a convenience for callers
// that only care about the addresses and not the primary flag.
func (e Emails) Strings() []string {
addresses := make([]string, len(e))
for i, email := range e {
addresses[i] = email.Address
}
return addresses
}
// Contains reports whether the given address is in the list.
func (e Emails) Contains(address string) bool {
for _, email := range e {
if email.Address == address {
return true
}
}
return false
}
// Permission represents a read or write permission to a topic // Permission represents a read or write permission to a topic
type Permission uint8 type Permission uint8
+3 -5
View File
@@ -402,13 +402,12 @@ const Emails = () => {
setSnack(t("account_basics_emails_copied_to_clipboard")); setSnack(t("account_basics_emails_copied_to_clipboard"));
}; };
// runEmailAction wraps an account API call with the shared error handling (redirect on // runEmailAction wraps an email account action with the shared error handling used by the
// unauthorized, surface a message otherwise). On success it refetches the account so the email // delete/set-primary/resend handlers (redirect on unauthorized, surface a message otherwise).
// list reflects the change immediately, rather than waiting for the async sync event. // The account view refreshes via the server's sync event, so there's no explicit refetch here.
const runEmailAction = async (fn, errorMessage) => { const runEmailAction = async (fn, errorMessage) => {
try { try {
await fn(); await fn();
await accountApi.sync();
} catch (e) { } catch (e) {
console.log(`[Account] Email action failed`, e); console.log(`[Account] Email action failed`, e);
if (e instanceof UnauthorizedError) { if (e instanceof UnauthorizedError) {
@@ -569,7 +568,6 @@ const AddEmailDialog = (props) => {
setSending(true); setSending(true);
setError(""); // Clear any error from a previous attempt setError(""); // Clear any error from a previous attempt
await accountApi.startEmailVerification(email); await accountApi.startEmailVerification(email);
await accountApi.sync(); // Refresh so the new "(unverified)" address shows up immediately
setSent(true); setSent(true);
} catch (e) { } catch (e) {
console.log(`[Account] Error starting email verification`, e); console.log(`[Account] Error starting email verification`, e);