users: never cache a deleted user's base row

redisstore.userBaseValue has no Deleted/DeletedAt/Status field, so caching a
deleted user silently reset Deleted back to false (and Status to the zero
UserStatusUnknown) on every round trip. That never self-healed: each later
cache miss reloaded the correctly tombstoned DB row and immediately
re-corrupted it on write, so once anyone looked a deleted account up, it kept
showing a blank name with "last seen recently" instead of "Deleted Account".

Keep deleted users off the base cache entirely so lookups always hit the
authoritative store, and stop presence overlay from touching a Deleted user's
Status at all as defense in depth.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Astra 2026-09-13 22:08:06 +01:00
parent f823b2cb74
commit 4ca35d2000
3 changed files with 80 additions and 1 deletions

View file

@ -906,7 +906,16 @@ func (s *Service) putCachedUsers(ctx context.Context, users ...domain.User) {
// Collectible ownership may change inside the star-gift aggregate. Keep // Collectible ownership may change inside the star-gift aggregate. Keep
// these uncommon users on the authoritative store path so the database // these uncommon users on the authoritative store path so the database
// lifecycle trigger can never be masked by a stale base-user cache entry. // lifecycle trigger can never be masked by a stale base-user cache entry.
if user.ID != 0 && user.EmojiStatusCollectible.Empty() { //
// redisstore.userBaseValue has no field for Deleted/DeletedAt/Status: caching
// a deleted user silently resets Deleted to false (and Status to the zero
// UserStatusUnknown) on every round trip, which never self-heals -- each
// subsequent miss reloads the correctly tombstoned DB row and immediately
// re-corrupts it on write. That regressed a deleted account back to looking
// live (blank name, but "last seen recently" instead of "Deleted Account").
// Keep deleted users off the cache so lookups always hit the authoritative
// store, which encodes the tombstone correctly.
if user.ID != 0 && user.EmojiStatusCollectible.Empty() && !user.Deleted {
cacheable = append(cacheable, user) cacheable = append(cacheable, user)
} }
} }

View file

@ -555,6 +555,69 @@ func TestServiceUpdateLastSeenBatchIsMonotonicAndInvalidatesOnce(t *testing.T) {
} }
} }
// deletedOverrideUserStore reports one user id as an already-tombstoned
// domain.User regardless of what the underlying memory store holds, so the
// test doesn't need a memory.UserStore deletion helper to exercise the
// base-user cache's handling of deleted users.
type deletedOverrideUserStore struct {
*memory.UserStore
deletedID int64
byIDsCalls int
}
func (s *deletedOverrideUserStore) ByIDs(ctx context.Context, ids []int64) ([]domain.User, error) {
s.byIDsCalls++
users, err := s.UserStore.ByIDs(ctx, ids)
if err != nil {
return nil, err
}
for i, u := range users {
if u.ID == s.deletedID {
users[i] = domain.User{ID: u.ID, Deleted: true, Status: domain.UserStatus{Kind: domain.UserStatusEmpty}}
}
}
return users, nil
}
// TestServiceNeverCachesDeletedUser guards the redisstore.UserCache cache
// schema gap: userBaseValue carries no Deleted/DeletedAt/Status field, so
// caching a deleted user silently resets Deleted to false (and Status to the
// zero UserStatusUnknown) on every round trip -- a bug that never self-heals,
// since each subsequent cache miss reloads the correctly tombstoned row and
// immediately re-corrupts it on write. It regressed a deleted account back to
// looking live: blank name (still blank, that part survives), but "last seen
// recently" instead of "Deleted Account". The service must keep deleted users
// off the base cache entirely so every lookup hits the authoritative store.
func TestServiceNeverCachesDeletedUser(t *testing.T) {
ctx := context.Background()
base := memory.NewUserStore()
owner, err := base.Create(ctx, domain.User{AccessHash: 1, Phone: "15550000031", FirstName: "Owner"})
if err != nil {
t.Fatalf("create owner: %v", err)
}
target, err := base.Create(ctx, domain.User{AccessHash: 2, Phone: "15550000032", FirstName: "Target"})
if err != nil {
t.Fatalf("create target: %v", err)
}
store := &deletedOverrideUserStore{UserStore: base, deletedID: target.ID}
cache := newMemoryBaseUserCache()
svc := NewService(store, WithBaseUserCache(cache))
got, found, err := svc.ByID(ctx, owner.ID, target.ID)
if err != nil || !found || !got.Deleted {
t.Fatalf("ByID = %+v found=%v err=%v, want a deleted user", got, found, err)
}
if _, cached := cache.users[target.ID]; cached {
t.Fatalf("cache holds deleted user %d, want it kept off the cache entirely", target.ID)
}
if _, found, err := svc.ByID(ctx, owner.ID, target.ID); err != nil || !found {
t.Fatalf("second ByID found=%v err=%v", found, err)
}
if store.byIDsCalls != 2 {
t.Fatalf("store ByIDs calls = %d, want 2 (deleted user must never be served from cache)", store.byIDsCalls)
}
}
func TestServiceRefreshesBaseCacheAfterProfileUpdate(t *testing.T) { func TestServiceRefreshesBaseCacheAfterProfileUpdate(t *testing.T) {
ctx := context.Background() ctx := context.Background()
base := memory.NewUserStore() base := memory.NewUserStore()

View file

@ -611,6 +611,13 @@ func (r *Router) withUserPresence(u domain.User) domain.User {
if u.Bot { if u.Bot {
return u return u
} }
// 已注销账号同理不参与 presencetgUser/tgSelfUser 对 Deleted 用户直接短路输出
// 精简 tombstone从不读取 Status这里覆盖与否本应无观测差异——但只靠那一层
// 短路里应外合:任何上游把 Deleted 弄丢的 bug例如缓存往返丢字段都会让这里
// 覆盖出的 Status 变成可见的“最近上线”,掩盖真正的 bug 而不是让它更早炸出来。
if u.Deleted {
return u
}
u.Status = r.userPresenceStatusForUser(u) u.Status = r.userPresenceStatusForUser(u)
return u return u
} }