fix: guard spurious rollback, harden createRole error path, validate before DB calls

- Add migrationRan flag to prevent rollback of user migration that never ran
- Return generic message on 500 in createRoleHandler, specific only for 409
- Move description validation before DB queries in updateRoleHandler
- Return existing role early when update body has no changes
- Wrap cache.set in createRoleByName with try/catch to prevent masking DB success
- Add JSDoc on 11000 catch explaining compound unique index
- Add tests: spurious rollback guard, empty update body, description validation
  ordering, listUsersByRole pagination

packages/api/src/admin/roles.spec.ts

@@ -247,7 +247,7 @@ describe('createAdminRolesHandlers', () => {
       await handlers.createRole(req, res);
 
       expect(status).toHaveBeenCalledWith(500);
-      expect(json).toHaveBeenCalledWith({ error: 'db crash' });
+      expect(json).toHaveBeenCalledWith({ error: 'Failed to create role' });
     });
 
     it('does not classify unrelated errors as 409', async () => {
@@ -557,6 +557,58 @@ describe('createAdminRolesHandlers', () => {
       expect(status).toHaveBeenCalledWith(500);
       expect(json).toHaveBeenCalledWith({ error: 'Failed to update role' });
     });
+
+    it('does not roll back when error occurs before user migration', async () => {
+      const deps = createDeps({
+        getRoleByName: jest
+          .fn()
+          .mockResolvedValueOnce(mockRole())
+          .mockRejectedValueOnce(new Error('db crash')),
+      });
+      const handlers = createAdminRolesHandlers(deps);
+      const { req, res, status } = createReqRes({
+        params: { name: 'editor' },
+        body: { name: 'new-name' },
+      });
+
+      await handlers.updateRole(req, res);
+
+      expect(status).toHaveBeenCalledWith(500);
+      expect(deps.updateUsersByRole).not.toHaveBeenCalled();
+    });
+
+    it('returns existing role early when update body has no changes', async () => {
+      const role = mockRole();
+      const deps = createDeps({
+        getRoleByName: jest.fn().mockResolvedValue(role),
+      });
+      const handlers = createAdminRolesHandlers(deps);
+      const { req, res, status, json } = createReqRes({
+        params: { name: 'editor' },
+        body: {},
+      });
+
+      await handlers.updateRole(req, res);
+
+      expect(status).toHaveBeenCalledWith(200);
+      expect(json).toHaveBeenCalledWith({ role });
+      expect(deps.updateRoleByName).not.toHaveBeenCalled();
+    });
+
+    it('rejects invalid description before making DB calls', async () => {
+      const deps = createDeps();
+      const handlers = createAdminRolesHandlers(deps);
+      const { req, res, status, json } = createReqRes({
+        params: { name: 'editor' },
+        body: { description: 123 },
+      });
+
+      await handlers.updateRole(req, res);
+
+      expect(status).toHaveBeenCalledWith(400);
+      expect(json).toHaveBeenCalledWith({ error: 'description must be a string' });
+      expect(deps.getRoleByName).not.toHaveBeenCalled();
+    });
   });
 
   describe('updateRolePermissions', () => {

packages/api/src/admin/roles.ts

@@ -109,12 +109,12 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
       });
       return res.status(201).json({ role });
     } catch (error) {
-      const message = error instanceof Error ? error.message : 'Failed to create role';
       logger.error('[adminRoles] createRole error:', error);
       const is409 =
         error instanceof Error &&
         (error.message.startsWith('Role "') ||
           error.message.startsWith('Cannot create role with reserved'));
+      const message = is409 && error instanceof Error ? error.message : 'Failed to create role';
       return res.status(is409 ? 409 : 500).json({ error: message });
     }
   }
@@ -124,6 +124,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
     const body = req.body as Partial<IRole>;
     let isRename = false;
     let trimmedName = '';
+    let migrationRan = false;
     try {
       if (
         body.name !== undefined &&
@@ -136,6 +137,14 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
           .status(400)
           .json({ error: `name must not exceed ${MAX_NAME_LENGTH} characters` });
       }
+      if (body.description !== undefined && typeof body.description !== 'string') {
+        return res.status(400).json({ error: 'description must be a string' });
+      }
+      if (body.description && body.description.length > MAX_DESCRIPTION_LENGTH) {
+        return res
+          .status(400)
+          .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` });
+      }
 
       trimmedName = body.name?.trim() ?? '';
       isRename = trimmedName !== '' && trimmedName !== name;
@@ -159,15 +168,6 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
         }
       }
 
-      if (body.description !== undefined && typeof body.description !== 'string') {
-        return res.status(400).json({ error: 'description must be a string' });
-      }
-      if (body.description && body.description.length > MAX_DESCRIPTION_LENGTH) {
-        return res
-          .status(400)
-          .json({ error: `description must not exceed ${MAX_DESCRIPTION_LENGTH} characters` });
-      }
-
       const updates: Partial<IRole> = {};
       if (isRename) {
         updates.name = trimmedName;
@@ -176,13 +176,18 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
         updates.description = body.description;
       }
 
+      if (Object.keys(updates).length === 0) {
+        return res.status(200).json({ role: existing });
+      }
+
       if (isRename) {
         await updateUsersByRole(name, trimmedName);
+        migrationRan = true;
       }
 
       const role = await updateRoleByName(name, updates);
       if (!role) {
-        if (isRename) {
+        if (migrationRan) {
           await updateUsersByRole(trimmedName, name);
         }
         return res.status(404).json({ error: 'Role not found' });
@@ -190,7 +195,7 @@ export function createAdminRolesHandlers(deps: AdminRolesDeps) {
 
       return res.status(200).json({ role });
     } catch (error) {
-      if (isRename) {
+      if (migrationRan) {
         try {
           await updateUsersByRole(trimmedName, name);
         } catch (rollbackError) {

packages/data-schemas/src/methods/role.methods.spec.ts

@@ -655,6 +655,27 @@ describe('listUsersByRole', () => {
     expect(users).toEqual([]);
   });
 
+  it('respects limit and offset for pagination', async () => {
+    await User.create([
+      { name: 'Alice', email: 'a@test.com', role: 'editor', username: 'a' },
+      { name: 'Bob', email: 'b@test.com', role: 'editor', username: 'b' },
+      { name: 'Carol', email: 'c@test.com', role: 'editor', username: 'c' },
+      { name: 'Dave', email: 'd@test.com', role: 'editor', username: 'd' },
+      { name: 'Eve', email: 'e@test.com', role: 'editor', username: 'e' },
+    ]);
+
+    const page1 = await listUsersByRole('editor', { limit: 2, offset: 0 });
+    const page2 = await listUsersByRole('editor', { limit: 2, offset: 2 });
+    const page3 = await listUsersByRole('editor', { limit: 2, offset: 4 });
+
+    expect(page1).toHaveLength(2);
+    expect(page2).toHaveLength(2);
+    expect(page3).toHaveLength(1);
+
+    const allIds = [...page1, ...page2, ...page3].map((u) => u._id!.toString());
+    expect(new Set(allIds).size).toBe(5);
+  });
+
   it('selects only expected fields', async () => {
     await User.create({
       name: 'Alice',

packages/data-schemas/src/methods/role.ts

@@ -364,14 +364,24 @@ export function createRoleMethods(mongoose: typeof import('mongoose'), deps: Rol
         name: name.trim(),
       }).save();
     } catch (err) {
+      /**
+       * The compound unique index `{ name: 1, tenantId: 1 }` on the role schema
+       * (roleSchema.index in schema/role.ts) triggers error 11000 when a concurrent
+       * request races past the findOne check above. This catch converts it into
+       * the same user-facing message as the application-level duplicate check.
+       */
       if (err && typeof err === 'object' && 'code' in err && err.code === 11000) {
         throw new Error(`Role "${name.trim()}" already exists`);
       }
       throw err;
     }
-    const cache = deps.getCache?.(CacheKeys.ROLES);
-    if (cache) {
-      await cache.set(role.name, role.toObject());
+    try {
+      const cache = deps.getCache?.(CacheKeys.ROLES);
+      if (cache) {
+        await cache.set(role.name, role.toObject());
+      }
+    } catch (cacheError) {
+      logger.error(`[createRoleByName] cache set failed for "${role.name}":`, cacheError);
     }
     return role.toObject() as IRole;
   }