fix: batch of 25 correctness fixes across server and client (#1370)

* chore(workflows): raise subagent effort tiers (sonnet/haiku to xhigh, prove opus to high)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(voice): 6 defect(s) (OC-0098, OC-0004, OC-0005, OC-0006, OC-0007, OC-0020)

* fix(db): 1 defect(s) (OC-0096)

* fix(admin): 1 defect(s) (OC-0097)

* fix(auth): 2 defect(s) (OC-0099, OC-0021)

* fix(voice): 1 defect(s) (OC-0018)

* fix(admin): 1 defect(s) (OC-0045)

* fix(api): 1 defect(s) (OC-0103)

* fix(client): 1 defect(s) (OC-0105)

* fix(client): 1 defect(s) (OC-0107)

* fix(api): 1 defect(s) (OC-0109)

* fix(api): 1 defect(s) (OC-0112)

* test(admin): compare restore bytes with bytes.Equal

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(voice): 2 defect(s) (OC-0095, OC-0014)

OC-0095: createRoom never called setE2EEEnabled(true), so the full ECDH/HKDF/AES-GCM key exchange completed but frames still reached the SFU in plaintext.

OC-0014: token refresh timer was 23h while the server mints LiveKit tokens with a 5-minute TTL, so any reconnect after minute 5 presented an expired token.

* fix(profile): 2 defect(s) (OC-0100, OC-0102)

* fix(service): 1 defect(s) (OC-0022)

Archived channels were only read-only for SendMessage/DeleteMessage. Edit, reaction, pin and purge sinks bypassed the check. Route every write sink through a shared requireChannelWritable gate.

* fix(api): 1 defect(s) (OC-0048)

* chore(workflows): correct stale model labels in bughunt-fix phase details

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(client): 1 defect(s) (OC-0015)

* fix(voice): 1 defect(s) (OC-0002)

* test: fix two CI-only failures in the batch-4 test suite

The delete-account broadcast test now observes member_ban on a second
client's socket: the hub broadcasts and then force-disconnects the target,
so on a slow runner the close could beat the target's own copy of the
frame. The observer is also the party the event exists for.

The voice e2e mock now echoes the real joined channel id on voice_leave
(it hardcoded channel_id 0, which the dispatcher's channel-matched
self-leave teardown correctly ignores), and the rejoin test waits for the
mock's delayed echoes to settle before clicking the row again — clicking
inside the echo window toggled a leave instead of a join.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Server/service/archived_channel_readonly_test.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"errors"
 	"testing"
+
+	"github.com/owncord/server/db"
 )
 
 // `archived` used to be consulted only by the visibility predicate
@@ -63,3 +65,129 @@ func TestSendMessage_AllowedAfterUnarchive(t *testing.T) {
 		t.Fatalf("SendMessage after unarchive: %v", err)
 	}
 }
+
+// OC-0022: SendMessage's archived gate lived only on SendMessage. EditMessage
+// routed its non-DM permission check through checkSendPermission, which
+// carried no archived check at all, so an author who still held a message id
+// could keep injecting arbitrary new text into an archived channel — fanned
+// out to every reader as chat_edited — even though a fresh chat_send was
+// refused. The gate must be shared, not re-implemented per sink.
+func TestEditMessage_RefusedInArchivedChannel(t *testing.T) {
+	svc, database := newTestMessageService(t)
+	ctx := context.Background()
+
+	sent, err := svc.SendMessage(ctx, SendMessageParams{
+		ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: "original",
+	})
+	if err != nil {
+		t.Fatalf("send: %v", err)
+	}
+
+	if _, err := database.ExecContext(ctx,
+		`UPDATE channels SET archived = 1 WHERE id = 10`); err != nil {
+		t.Fatalf("archive channel: %v", err)
+	}
+
+	if _, err := svc.EditMessage(ctx, 1, sent.MessageID, "slipped past the archive"); !errors.Is(err, ErrForbidden) {
+		t.Fatalf("EditMessage in an archived channel: err = %v, want ErrForbidden", err)
+	}
+
+	msg, err := database.GetMessage(ctx, sent.MessageID)
+	if err != nil || msg == nil {
+		t.Fatalf("GetMessage: %v", err)
+	}
+	if msg.Content != "original" {
+		t.Fatalf("content must survive a refused edit against an archived channel, got %q", msg.Content)
+	}
+}
+
+// OC-0022: handleReaction (AddReaction/RemoveReaction) bypasses
+// checkSendPermission entirely, so it needs its own archived gate. A reaction
+// fans a live reaction_update out to every reader just like a send or edit.
+func TestAddReaction_RefusedInArchivedChannel(t *testing.T) {
+	svc, database := newTestMessageService(t)
+	ctx := context.Background()
+
+	sent, err := svc.SendMessage(ctx, SendMessageParams{
+		ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: "react to me",
+	})
+	if err != nil {
+		t.Fatalf("send: %v", err)
+	}
+
+	if _, err := database.ExecContext(ctx,
+		`UPDATE channels SET archived = 1 WHERE id = 10`); err != nil {
+		t.Fatalf("archive channel: %v", err)
+	}
+
+	if _, err := svc.AddReaction(ctx, 1, sent.MessageID, "👍"); !errors.Is(err, ErrForbidden) {
+		t.Fatalf("AddReaction in an archived channel: err = %v, want ErrForbidden", err)
+	}
+
+	reactors, err := database.GetReactionUsers(ctx, sent.MessageID, "👍", db.MaxReactionUsers)
+	if err != nil {
+		t.Fatalf("GetReactionUsers: %v", err)
+	}
+	if len(reactors) != 0 {
+		t.Fatalf("reaction must not persist against an archived channel, got %d reactors", len(reactors))
+	}
+}
+
+// OC-0022: SetMessagePinned also bypasses checkSendPermission, so a
+// MANAGE_MESSAGES holder could still pin/unpin in an archived channel.
+func TestSetMessagePinned_RefusedInArchivedChannel(t *testing.T) {
+	svc, database := newPurgeService(t) // seeds user 2 with MANAGE_MESSAGES on channel 10
+	ctx := context.Background()
+
+	sent, err := svc.SendMessage(ctx, SendMessageParams{
+		ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: "pin me",
+	})
+	if err != nil {
+		t.Fatalf("send: %v", err)
+	}
+
+	if _, err := database.ExecContext(ctx,
+		`UPDATE channels SET archived = 1 WHERE id = 10`); err != nil {
+		t.Fatalf("archive channel: %v", err)
+	}
+
+	if err := svc.SetMessagePinned(ctx, 2, 10, sent.MessageID, true); !errors.Is(err, ErrForbidden) {
+		t.Fatalf("SetMessagePinned in an archived channel: err = %v, want ErrForbidden", err)
+	}
+
+	pinned, err := database.GetPinnedMessages(ctx, 10, 2)
+	if err != nil {
+		t.Fatalf("GetPinnedMessages: %v", err)
+	}
+	if len(pinned) != 0 {
+		t.Fatalf("pin must not persist against an archived channel, got %d pinned", len(pinned))
+	}
+}
+
+// OC-0022: PurgeMessages also bypasses checkSendPermission, so a
+// MANAGE_MESSAGES holder could still bulk-delete an archived channel's
+// history.
+func TestPurgeMessages_RefusedInArchivedChannel(t *testing.T) {
+	svc, database := newPurgeService(t)
+	ctx := context.Background()
+	ids := seedPurgeMessages(t, database, 10, 3)
+
+	if _, err := database.ExecContext(ctx,
+		`UPDATE channels SET archived = 1 WHERE id = 10`); err != nil {
+		t.Fatalf("archive channel: %v", err)
+	}
+
+	if _, err := svc.PurgeMessages(ctx, 2, 10, 3, 0); !errors.Is(err, ErrForbidden) {
+		t.Fatalf("PurgeMessages against an archived channel: err = %v, want ErrForbidden", err)
+	}
+
+	for _, id := range ids {
+		msg, err := database.GetMessage(ctx, id)
+		if err != nil || msg == nil {
+			t.Fatalf("GetMessage(%d): %v", id, err)
+		}
+		if msg.Deleted {
+			t.Fatalf("message %d must survive a purge attempt against an archived channel", id)
+		}
+	}
+}

Server/service/message.go

@@ -204,6 +204,17 @@ func sanitizeToFixpoint(raw string) string {
 	return s
 }
 
+// SanitizeText runs raw through the same unescape-sanitize-fixpoint pipeline
+// as message content and profile free-text fields (see sanitizeToFixpoint):
+// it strips HTML but leaves survivors as typed instead of persisting them as
+// literal '/>/& entities the way a bare bluemonday sanitizer.Sanitize
+// call would. Exported for call sites outside this package that sanitize a
+// single free-text field before storage — e.g. the username field on
+// registration, which must canonicalize identically to how lookups treat it.
+func SanitizeText(raw string) string {
+	return sanitizeToFixpoint(raw)
+}
+
 // sanitizeContent validates and sanitizes message content.
 func sanitizeContent(raw string, allowEmpty bool) (string, error) {
 	if len(raw) > maxMessageLen*4 {

Server/service/message_crud.go

@@ -45,18 +45,10 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
 
 	isDM := ch.Type == "dm"
 
-	// Archived channels are read-only. Until now `archived` was consulted only
-	// by the visibility predicate (VisibleChannelIDs / RefreshChannelVisibility),
-	// so it hid the channel without protecting it: any caller that still held
-	// the id — a custom client, or a stock client racing the channel_delete —
-	// could keep posting into an archive indefinitely. History stays readable;
-	// only writes are refused.
-	if !isDM && ch.Archived {
-		return nil, fmt.Errorf("%w: channel is archived", ErrForbidden)
-	}
-
-	// Permission check.
-	if err := s.checkSendPermission(ctx, p.UserID, p.ChannelID, ch.Type); err != nil {
+	// Permission check. Also refuses a write against an archived channel — see
+	// requireChannelWritable in message_perms.go, the shared gate every
+	// message write sink routes through.
+	if err := s.checkSendPermission(ctx, p.UserID, ch); err != nil {
 		return nil, err
 	}
 
@@ -290,7 +282,7 @@ func (s *MessageService) EditMessage(ctx context.Context, userID, msgID int64, r
 		if blkErr := requireDMNotBlocked(ctx, s.st, userID, msg.ChannelID); blkErr != nil {
 			return nil, blkErr
 		}
-	} else if permErr := s.checkSendPermission(ctx, userID, msg.ChannelID, chanType); permErr != nil {
+	} else if permErr := s.checkSendPermission(ctx, userID, ch); permErr != nil {
 		// An edit injects new text into the channel and is fanned out to every
 		// reader, so it must clear the same gate as a send rather than
 		// SEND_MESSAGES alone: READ_MESSAGES so a role locked out of a private
@@ -379,11 +371,11 @@ func (s *MessageService) DeleteMessage(ctx context.Context, userID, msgID int64)
 	}
 	isDM := ch.Type == "dm"
 
-	// Archived channels are read-only, mirroring SendMessage's gate
-	// (message_crud.go:54): history stays visible, but a member or moderator
-	// must not be able to mutate it by deleting a message out of the archive.
-	if !isDM && ch.Archived {
-		return nil, fmt.Errorf("%w: channel is archived", ErrForbidden)
+	// Archived channels are read-only — see requireChannelWritable in
+	// message_perms.go, the shared gate every message write sink routes
+	// through.
+	if err := requireChannelWritable(ch); err != nil {
+		return nil, err
 	}
 
 	var isMod bool

Server/service/message_perms.go

@@ -59,36 +59,73 @@ func (s *MessageService) CanPost(ctx context.Context, userID, channelID int64) e
 	if err != nil || ch == nil {
 		return fmt.Errorf("%w: channel not found", ErrNotFound)
 	}
-	return s.checkSendPermission(ctx, userID, channelID, ch.Type)
+	return s.checkSendPermission(ctx, userID, ch)
 }
 
-// checkSendPermission validates send permission for a channel of the given
-// type. Announcement channels are readable by anyone with READ_MESSAGES but
-// only postable by users with MANAGE_MESSAGES (posting is restricted to
-// moderators/admins); all other non-DM channels require SEND_MESSAGES.
-func (s *MessageService) checkSendPermission(ctx context.Context, userID, channelID int64, chanType string) error {
-	isDM := chanType == "dm"
+// checkSendPermission validates send permission for ch. Announcement channels
+// are readable by anyone with READ_MESSAGES but only postable by users with
+// MANAGE_MESSAGES (posting is restricted to moderators/admins); all other
+// non-DM channels require SEND_MESSAGES. Also enforces requireChannelWritable,
+// so every caller — SendMessage, EditMessage, CanPost — refuses an archived
+// channel without re-implementing that check itself.
+func (s *MessageService) checkSendPermission(ctx context.Context, userID int64, ch *db.Channel) error {
+	isDM := ch.Type == "dm"
 	if isDM {
-		ok, err := s.st.IsDMParticipant(ctx, userID, channelID)
+		ok, err := s.st.IsDMParticipant(ctx, userID, ch.ID)
 		if err != nil {
 			return fmt.Errorf("%w: failed to check DM participation: %v", ErrInternal, err)
 		}
 		if !ok {
 			return fmt.Errorf("%w: not a participant in this DM", ErrForbidden)
 		}
-		return requireDMNotBlocked(ctx, s.st, userID, channelID)
+		return requireDMNotBlocked(ctx, s.st, userID, ch.ID)
 	}
-	if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages|permissions.SendMessages) {
+	if err := requireChannelWritable(ch); err != nil {
+		return err
+	}
+	if !s.perms.HasChannelPerm(ctx, userID, ch.ID, permissions.ReadMessages|permissions.SendMessages) {
 		return fmt.Errorf("%w: missing SEND_MESSAGES permission", ErrForbidden)
 	}
 	// Announcement channels: posting is restricted to users who can manage
 	// messages, even though everyone with READ_MESSAGES can view them.
-	if chanType == "announcement" && !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ManageMessages) {
+	if ch.Type == "announcement" && !s.perms.HasChannelPerm(ctx, userID, ch.ID, permissions.ManageMessages) {
 		return fmt.Errorf("%w: announcement channels require MANAGE_MESSAGES to post", ErrForbidden)
 	}
 	return nil
 }
 
+// requireChannelWritable refuses a write against an archived non-DM channel.
+// `archived` used to be consulted only by the visibility predicate
+// (VisibleChannelIDs / RefreshChannelVisibility), so it hid a channel without
+// protecting it: any caller that still held the id — a custom client, or a
+// stock client racing the channel_delete that archiving triggers — could keep
+// posting, editing, reacting, pinning, or bulk-deleting in an archive
+// indefinitely. History stays readable; only writes are refused.
+//
+// DMs carry no archive flag/concept and are exempt. ch == nil is treated as
+// "nothing to check" rather than a panic — the caller's own nil handling (a
+// failed channel lookup) decides what happens next.
+//
+// Single shared gate for every write sink: checkSendPermission (so
+// SendMessage, EditMessage and CanPost inherit it), plus DeleteMessage,
+// handleReaction, SetMessagePinned and PurgeMessages, which route their own
+// permission checks and so call it directly instead.
+func requireChannelWritable(ch *db.Channel) error {
+	if ch == nil || ch.Type == "dm" || !ch.Archived {
+		return nil
+	}
+	return fmt.Errorf("%w: channel is archived", ErrForbidden)
+}
+
+// RequireDMNotBlocked is the exported form of requireDMNotBlocked so callers
+// outside the service package (voice join/token-refresh, ws/voice_join.go)
+// can share this single block-check implementation — same group-DM exemption,
+// same "lookup failure is not a block" posture — instead of reimplementing it
+// against the raw DB. st only needs to be a Store; *db.DB satisfies it.
+func RequireDMNotBlocked(ctx context.Context, st Store, userID, channelID int64) error {
+	return requireDMNotBlocked(ctx, st, userID, channelID)
+}
+
 // requireDMNotBlocked reports ErrBlocked when userID and the other participant
 // of DM channelID have blocked each other in either direction.
 //

Server/service/message_purge.go

@@ -52,6 +52,13 @@ func (s *MessageService) PurgeMessages(ctx context.Context, userID, channelID in
 	if ch.Type == "dm" {
 		return nil, fmt.Errorf("%w: bulk delete is not available in direct messages", ErrForbidden)
 	}
+	// Archived channels are read-only. PurgeMessages bypasses
+	// checkSendPermission (it runs its own MANAGE_MESSAGES check below), so it
+	// needs the shared gate directly — see requireChannelWritable in
+	// message_perms.go.
+	if err := requireChannelWritable(ch); err != nil {
+		return nil, err
+	}
 	if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages|permissions.ManageMessages) {
 		return nil, fmt.Errorf("%w: missing MANAGE_MESSAGES permission", ErrForbidden)
 	}

Server/service/message_query.go

@@ -205,6 +205,13 @@ func (s *MessageService) SetMessagePinned(ctx context.Context, userID, channelID
 	if err != nil || ch == nil {
 		return fmt.Errorf("%w: channel not found", ErrNotFound)
 	}
+	// Archived channels are read-only. SetMessagePinned bypasses
+	// checkSendPermission (it runs its own DM/permission branch below), so it
+	// needs the shared gate directly — see requireChannelWritable in
+	// message_perms.go.
+	if err := requireChannelWritable(ch); err != nil {
+		return err
+	}
 	if ch.Type == "dm" {
 		ok, err := s.st.IsDMParticipant(ctx, userID, channelID)
 		if err != nil || !ok {

Server/service/message_reactions.go

@@ -105,6 +105,14 @@ func (s *MessageService) handleReaction(ctx context.Context, userID, msgID int64
 	ch, chErr := s.st.GetChannel(ctx, msg.ChannelID)
 	isDM := chErr == nil && ch != nil && ch.Type == "dm"
 
+	// Archived channels are read-only. handleReaction bypasses
+	// checkSendPermission (it runs its own DM/permission branch below), so it
+	// needs the shared gate directly — see requireChannelWritable in
+	// message_perms.go.
+	if err := requireChannelWritable(ch); err != nil {
+		return nil, err
+	}
+
 	var participantIDs []int64
 	if isDM {
 		ok, dmErr := s.st.IsDMParticipant(ctx, userID, msg.ChannelID)

Server/service/moderation.go

@@ -135,39 +135,45 @@ func (s *ModerationService) BanUser(ctx context.Context, actorID, targetID int64
 // of: the actor must strictly outrank the target, and may not hand out a role
 // positioned at or above their own — otherwise any admin could promote anyone
 // (including themselves via a second account) to Owner.
-func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetID, newRoleID int64) error {
+//
+// It returns the role that was assigned so callers (the member_update
+// broadcast and visibility refresh, in particular) can use it directly
+// instead of re-reading it: a re-read is racing a possible concurrent role
+// delete for no reason, since this call already loaded and validated the
+// exact same row under the same request.
+func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetID, newRoleID int64) (*db.Role, error) {
 	if targetID <= 0 {
-		return fmt.Errorf("%w: user_id must be positive", ErrBadRequest)
+		return nil, fmt.Errorf("%w: user_id must be positive", ErrBadRequest)
 	}
 	if actorID == targetID {
-		return fmt.Errorf("%w: cannot change your own role", ErrBadRequest)
+		return nil, fmt.Errorf("%w: cannot change your own role", ErrBadRequest)
 	}
 
 	// Authorization before existence — see BanUser.
 	actorRole, err := s.requirePerm(ctx, actorID, permissions.ManageRoles)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	target, err := s.st.GetUserByID(ctx, targetID)
 	if err != nil || target == nil {
-		return fmt.Errorf("%w: user not found", ErrNotFound)
+		return nil, fmt.Errorf("%w: user not found", ErrNotFound)
 	}
 	if err := s.requireOutranksRole(ctx, actorRole, targetID); err != nil {
-		return err
+		return nil, err
 	}
 
 	newRole, err := s.st.GetRoleByID(ctx, newRoleID)
 	if err != nil || newRole == nil {
-		return fmt.Errorf("%w: role not found", ErrBadRequest)
+		return nil, fmt.Errorf("%w: role not found", ErrBadRequest)
 	}
 	// Administrator bypasses permission bits, never the hierarchy: the owner
 	// role is above every admin, so only the owner can grant it.
 	if newRole.Position >= actorRole.Position {
-		return fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden)
+		return nil, fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden)
 	}
 
 	if err := s.st.UpdateUserRole(ctx, targetID, newRoleID); err != nil {
-		return fmt.Errorf("%w: failed to update role: %v", ErrInternal, err)
+		return nil, fmt.Errorf("%w: failed to update role: %v", ErrInternal, err)
 	}
 	// Drop the target's cached role immediately: without this a demotion keeps
 	// granting the old bits (and the old rank) for up to permCacheTTL.
@@ -178,7 +184,7 @@ func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetI
 		fmt.Sprintf("changed %s role to %s", target.Username, newRole.Name))
 
 	slog.Info("role changed", "actor_id", actorID, "target_id", targetID, "new_role_id", newRoleID)
-	return nil
+	return newRole, nil
 }
 
 // ForceLogout revokes every session of the target user (the client's "Kick").

Server/service/moderation_test.go

@@ -115,11 +115,11 @@ func TestChangeUserRole_RequiresManageRoles(t *testing.T) {
 	svc, database := newTestRoleService(t)
 
 	// A member without MANAGE_ROLES is refused...
-	if err := svc.ChangeUserRole(context.Background(), 4, 5, 3); !errors.Is(err, ErrForbidden) {
+	if _, err := svc.ChangeUserRole(context.Background(), 4, 5, 3); !errors.Is(err, ErrForbidden) {
 		t.Fatalf("member role change: want ErrForbidden, got %v", err)
 	}
 	// ...and gets Forbidden, not NotFound, for a missing target.
-	if err := svc.ChangeUserRole(context.Background(), 4, 999, 3); !errors.Is(err, ErrForbidden) {
+	if _, err := svc.ChangeUserRole(context.Background(), 4, 999, 3); !errors.Is(err, ErrForbidden) {
 		t.Fatalf("unauthorized probe of missing id: want ErrForbidden, got %v", err)
 	}
 	if got := roleIDOf(t, database, 5); got != 4 {
@@ -131,25 +131,25 @@ func TestChangeUserRole_CannotAssignAtOrAboveOwnRank(t *testing.T) {
 	svc, database := newTestRoleService(t)
 
 	// The hole this closes: an Administrator could promote anyone to Owner.
-	if err := svc.ChangeUserRole(context.Background(), 2, 4, 1); !errors.Is(err, ErrForbidden) {
+	if _, err := svc.ChangeUserRole(context.Background(), 2, 4, 1); !errors.Is(err, ErrForbidden) {
 		t.Fatalf("admin promoting to owner: want ErrForbidden, got %v", err)
 	}
 	// Equal rank is refused too — an admin cannot mint another admin.
-	if err := svc.ChangeUserRole(context.Background(), 2, 4, 2); !errors.Is(err, ErrForbidden) {
+	if _, err := svc.ChangeUserRole(context.Background(), 2, 4, 2); !errors.Is(err, ErrForbidden) {
 		t.Fatalf("admin assigning own rank: want ErrForbidden, got %v", err)
 	}
 	if got := roleIDOf(t, database, 4); got != 4 {
 		t.Fatalf("member role changed to %d despite refusal", got)
 	}
 	// Strictly below own rank is allowed.
-	if err := svc.ChangeUserRole(context.Background(), 2, 4, 3); err != nil {
+	if _, err := svc.ChangeUserRole(context.Background(), 2, 4, 3); err != nil {
 		t.Fatalf("admin promoting to mod: %v", err)
 	}
 	if got := roleIDOf(t, database, 4); got != 3 {
 		t.Fatalf("role after promotion = %d, want 3", got)
 	}
 	// The owner outranks the admin role, so the owner may grant it.
-	if err := svc.ChangeUserRole(context.Background(), 1, 5, 2); err != nil {
+	if _, err := svc.ChangeUserRole(context.Background(), 1, 5, 2); err != nil {
 		t.Fatalf("owner promoting to admin: %v", err)
 	}
 }
@@ -158,26 +158,26 @@ func TestChangeUserRole_HierarchyAndValidation(t *testing.T) {
 	svc, database := newTestRoleService(t)
 
 	// A moderator holding MANAGE_ROLES still cannot touch a higher-ranked user.
-	if err := svc.ChangeUserRole(context.Background(), 3, 2, 4); !errors.Is(err, ErrForbidden) {
+	if _, err := svc.ChangeUserRole(context.Background(), 3, 2, 4); !errors.Is(err, ErrForbidden) {
 		t.Fatalf("mod demoting an admin: want ErrForbidden, got %v", err)
 	}
 	if got := roleIDOf(t, database, 2); got != 2 {
 		t.Fatalf("admin role changed to %d despite refusal", got)
 	}
 	// Nor the owner.
-	if err := svc.ChangeUserRole(context.Background(), 3, 1, 4); !errors.Is(err, ErrForbidden) {
+	if _, err := svc.ChangeUserRole(context.Background(), 3, 1, 4); !errors.Is(err, ErrForbidden) {
 		t.Fatalf("mod demoting the owner: want ErrForbidden, got %v", err)
 	}
 	// Self-service promotion is a bad request regardless of authority.
-	if err := svc.ChangeUserRole(context.Background(), 2, 2, 1); !errors.Is(err, ErrBadRequest) {
+	if _, err := svc.ChangeUserRole(context.Background(), 2, 2, 1); !errors.Is(err, ErrBadRequest) {
 		t.Fatalf("self role change: want ErrBadRequest, got %v", err)
 	}
 	// A nonexistent role is a bad request, not a 500.
-	if err := svc.ChangeUserRole(context.Background(), 1, 4, 9999); !errors.Is(err, ErrBadRequest) {
+	if _, err := svc.ChangeUserRole(context.Background(), 1, 4, 9999); !errors.Is(err, ErrBadRequest) {
 		t.Fatalf("unknown role id: want ErrBadRequest, got %v", err)
 	}
 	// Authorized actor gets a real NotFound for a missing target.
-	if err := svc.ChangeUserRole(context.Background(), 1, 999, 4); !errors.Is(err, ErrNotFound) {
+	if _, err := svc.ChangeUserRole(context.Background(), 1, 999, 4); !errors.Is(err, ErrNotFound) {
 		t.Fatalf("missing target: want ErrNotFound, got %v", err)
 	}
 }
@@ -185,7 +185,7 @@ func TestChangeUserRole_HierarchyAndValidation(t *testing.T) {
 func TestChangeUserRole_AuditWritten(t *testing.T) {
 	svc, database := newTestRoleService(t)
 
-	if err := svc.ChangeUserRole(context.Background(), 1, 4, 3); err != nil {
+	if _, err := svc.ChangeUserRole(context.Background(), 1, 4, 3); err != nil {
 		t.Fatalf("owner role change: %v", err)
 	}
 	entries, err := database.GetAuditLog(context.Background(), 10, 0)

Server/service/profile_fields_test.go

@@ -128,6 +128,48 @@ func TestUpdateProfile_ConcurrentUpdatesSerializePerUser(t *testing.T) {
 	}
 }
 
+// OC-0102: an avatar-only patch must not carry a username at all, so a stale
+// pre-lock snapshot (handleUploadAvatar captures user.Username before the
+// multipart parse / image decode / disk write, well before this function's
+// per-user lock) can never overwrite a rename that commits in the meantime.
+// UpdateProfile's read-merge-write already treats DisplayName/About this
+// way (nil-vs-non-nil pointer); Username needs the same "unspecified means
+// leave it alone" contract via its own zero value, since it is a plain
+// string rather than a pointer.
+func TestUpdateProfile_AvatarOnlyPatchDoesNotOverwriteUsername(t *testing.T) {
+	svc, database := newUserSvc(t)
+	ctx := context.Background()
+
+	// Models a concurrent PATCH /users/me rename that lands and commits
+	// first.
+	if _, err := svc.UpdateProfile(ctx, 1, ProfilePatch{Username: "bob"}); err != nil {
+		t.Fatalf("rename UpdateProfile: %v", err)
+	}
+
+	// An avatar-only caller supplies no username intent at all — Username is
+	// left at its zero value, the way handleUploadAvatar's ProfilePatch must
+	// after the fix (it no longer fills Username from its stale snapshot).
+	avatarURL := "/api/v1/files/abc"
+	u, err := svc.UpdateProfile(ctx, 1, ProfilePatch{Avatar: &avatarURL})
+	if err != nil {
+		t.Fatalf("avatar-only UpdateProfile: %v", err)
+	}
+	if u.Username != "bob" {
+		t.Fatalf("username = %q, want %q — an avatar-only patch must not revert a concurrent rename", u.Username, "bob")
+	}
+	if u.Avatar == nil || *u.Avatar != avatarURL {
+		t.Fatalf("avatar = %v, want %q — the avatar itself must still be applied", u.Avatar, avatarURL)
+	}
+
+	stored, err := database.GetUserByID(ctx, 1)
+	if err != nil {
+		t.Fatalf("GetUserByID: %v", err)
+	}
+	if stored.Username != "bob" {
+		t.Fatalf("stored username = %q, want %q", stored.Username, "bob")
+	}
+}
+
 func TestUpdateProfile_SanitizesAndTrims(t *testing.T) {
 	svc, _ := newUserSvc(t)
 	name := "  <b>Ada</b>  "

Server/service/user.go

@@ -159,6 +159,17 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro
 	if err != nil || current == nil {
 		return nil, fmt.Errorf("%w: user not found", ErrNotFound)
 	}
+	// An empty Username means "unspecified", the same as a nil
+	// DisplayName/About pointer — merged against the current row rather
+	// than written verbatim. This is what lets an avatar-only caller
+	// (handleUploadAvatar) leave username alone without handing over a
+	// snapshot that could be stale by the time this call lands: PATCH
+	// /users/me always validates and rejects an empty username before
+	// calling in, so "" never reaches here as a real rename request.
+	username := patch.Username
+	if username == "" {
+		username = current.Username
+	}
 	avatar := current.Avatar
 	if patch.Avatar != nil {
 		avatar = nullable(*patch.Avatar)
@@ -166,7 +177,7 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro
 	displayName := resolveOptional(patch.DisplayName, current.DisplayName)
 	about := resolveOptional(patch.About, current.About)
 
-	if err := s.st.UpdateUserProfile(ctx, userID, patch.Username, avatar, displayName, about); err != nil {
+	if err := s.st.UpdateUserProfile(ctx, userID, username, avatar, displayName, about); err != nil {
 		if db.IsUniqueConstraintError(err) {
 			return nil, fmt.Errorf("%w: username is already taken", ErrConflict)
 		}
@@ -178,8 +189,8 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro
 	}
 	// Audit rows must survive a request canceled after the write committed.
 	db.WriteAudit(context.WithoutCancel(ctx), s.st, userID, "profile_update", "user", userID,
-		fmt.Sprintf("username=%s", patch.Username))
-	slog.Info("profile updated", "user_id", userID, "username", patch.Username)
+		fmt.Sprintf("username=%s", username))
+	slog.Info("profile updated", "user_id", userID, "username", username)
 	return user, nil
 }