Activity update clobbers role

Medium Severity

Concurrent updateUserActivity and updateUserRole both read-modify-write the full presence JSON in Redis. If activity was read before a role refresh but written after, the stale snapshot can overwrite a downgraded or refreshed role and roleCheckedAt, so mutating ops may skip revalidation until the TTL expires.

 

^{Reviewed by Cursor Bugbot for commit 5c9d711. Configure here.}

Not a race here — both updateUserActivity and updateUserRole run as EVALSHA Lua scripts, and Redis executes Lua atomically (script execution is serialized; nothing interleaves between a script's HGET and HSET).

UPDATE_ACTIVITY_SCRIPT re-reads the presence blob inside its own atomic block (HGET → cjson.decode → set only cursor/selection/lastActivity → HSET cjson.encode), so it always preserves whatever role/roleCheckedAt are current at that instant. A concurrent UPDATE_ROLE_SCRIPT can't run between activity's read and write, so it can't be clobbered by a stale snapshot.

The only staleness window is the intended ≤15s role-revalidation TTL (and the push reconciliation makes full revocation immediate). Leaving the scripts as-is.

Missing fetch timeout blocks the caller

notifyWorkspaceAccessChanged is awaited synchronously in both the PATCH /api/workspaces/[id]/permissions handler and the DELETE /api/workspaces/members/[id] handler. If the realtime server accepts the TCP connection but never sends a response (e.g., it is under load, mid-restart, or hanging), fetch has no built-in deadline and the permission-change HTTP response will hang indefinitely — contradicting the "failures are logged but never block the permission mutation" promise in the JSDoc.

Add an AbortSignal.timeout() to bound the worst case: signal: AbortSignal.timeout(3000)

Redundant per-workflow DB queries for workspace-level permission

verifyWorkflowAccess issues two DB queries per call: one to look up the workflow row and one to check the workspace permission via authorizeWorkflowByWorkspacePermission. Because workspace permission is a single entry in the permissions table that applies uniformly to every workflow in the workspace, calling it inside the per-workflow loop means the same permission row is fetched N times — once for each active room the user is present in. For a user simultaneously in 10+ workflow rooms, reconciliation makes 20+ sequential DB round-trips when 2 would suffice.

A targeted optimization would check the workspace-level access once before the loop and skip the per-workflow permission query, using only the workflow-existence check inside the loop.

Empty strings bypass the type check and reach the room manager

typeof workspaceId !== 'string' accepts "" as valid, which would then call handleWorkspaceAccessChange('', ''). The DB query filters by workspaceId, so it returns 0 rows and the call is harmless today — but an empty-string guard costs nothing and makes the contract explicit.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Thundering herd of DB queries at each TTL boundary

When a socket is actively sending events (e.g., a large paste generates tens of workflow-operation frames in rapid succession), all inflight handlers snapshot presence.roleCheckedAt before any of them can write it back. Every handler sees isStale = true and concurrently calls verifyWorkflowAccess (two DB round-trips each). The first response to return then updates the room-manager timestamp, but the remaining in-flight queries are already underway.

Because TTL expiry is aligned to wall-clock time and batched edits are common in collaborative editors, this burst fires on every 15-second boundary for active sockets. The side-effect is harmless but wastes DB connections. A per-socket in-memory flag (e.g., a Set keyed by socketId) that is cleared on completion could collapse concurrent revalidations to one query.