Migrate UserSessionsManager and associated tests to async/await

GitOrigin-RevId: 8b5f3a296798930aa1168738cd3a4c666c7a3028
This commit is contained in:
Simon Gardner
2025-10-28 09:05:42 +00:00
committed by Copybot
parent 9e695c6ab9
commit e3efc885e4
3 changed files with 906 additions and 718 deletions
+389
View File
@@ -0,0 +1,389 @@
---
mode: 'agent'
description: 'Generate a clear code explanation with examples'
---
# Improved Async/Await Migration Instructions
Based on lessons learned from PR #28840 and reviewer feedback, these comprehensive instructions address the original migration requirements while preventing common issues.
## Core Migration Principles
### 1. Function Signature Transformation
- Convert callback-style functions to async/await
- Remove callback parameters from function signatures
- Add `async` keyword to function declarations
- Replace `return callback(err, result)` with `throw err` or `return result`
- Replace `return callback()` with `return` (or `return undefined`)
### 2. Error Handling Patterns
#### OError.tag Usage - CRITICAL UPDATE
**DO NOT** wrap simple operations in try/catch just to tag errors with OError. With async/await, the stack trace is preserved automatically, making OError.tag less necessary for basic error propagation.
```javascript
// OLD (callback style) - OError.tag was needed
callback(err) => {
OError.tag(err, 'description', { context })
return callback(err)
}
// BAD (unnecessary with async/await)
try {
await operation()
} catch (err) {
throw OError.tag(err, 'description', { context })
}
// GOOD (let errors propagate naturally)
await operation()
// ONLY use OError.tag when adding meaningful context or transforming errors
try {
await complexOperation()
} catch (err) {
if (err.code === 'SPECIFIC_ERROR') {
throw OError.tag(err, 'meaningful context about why this failed', {
important_context: value
})
}
throw err // let other errors propagate unchanged
}
```
### 3. Concurrency Considerations - CRITICAL
#### Sequential vs Parallel Operations
**Be extremely cautious when converting from serial to parallel operations.** The original code's choice of sequential processing is often intentional.
```javascript
// OLD - Sequential processing (often intentional)
Async.mapSeries(items, processItem, callback)
// BAD - Unbounded parallel processing
await Promise.all(items.map(processItem))
// BETTER - Keep sequential if unsure about resource limits
for (const item of items) {
await processItem(item)
}
// GOOD - Controlled batch processing for performance
const BATCH_SIZE = 10
for (let i = 0; i < items.length; i += BATCH_SIZE) {
const batch = items.slice(i, i + BATCH_SIZE)
await Promise.all(batch.map(processItem))
}
// IDEAL - Use Redis MGET for multiple key retrieval
// Instead of: Promise.all(keys.map(k => redis.get(k)))
const values = await redis.mget(keys)
```
#### Database/Redis Operation Guidelines
- **Never** send unbounded parallel requests to databases
- **Prefer** sequential processing for database operations unless there's a specific performance need
- **Consider** batch operations (like Redis MGET/MSET) for multiple operations
- **Implement** maximum concurrency limits when parallel processing is necessary
### 4. Background Operations
#### Fire-and-Forget Pattern
When operations were called in the background (with empty callbacks), preserve this behavior:
```javascript
// OLD - Background operation with ignored callback
someOperation(user, function () {}) // errors swallowed
// GOOD - Preserve background behavior
someOperation(user).catch(err => {
logger.error({ err }, 'Failed to run background operation')
})
// Or if truly fire-and-forget:
someOperation(user).catch(() => {}) // explicitly ignore errors
```
### 5. Module Export Patterns
#### Using callbackifyAll for Dual API
```javascript
const { callbackifyAll } = require('@overleaf/promise-utils')
const MyModule = {
async myMethod(param) {
// async implementation
},
}
const moduleExports = {
...callbackifyAll(MyModule), // callback API
promises: MyModule, // promise API
}
module.exports = moduleExports
```
#### Internal Method Stubbing (for testing)
**Only** add method binding patterns when tests need to stub internal method calls:
```javascript
// ONLY if tests need to stub internal calls to _internalMethod
MyModule._internalMethod = (...args) => moduleExports._internalMethod(...args)
```
**Do NOT** expose internal methods at the top level - they should be accessible via `moduleExports.promises._internalMethod`.
### 6. Test Migration Patterns
#### Async Test Conversion
```javascript
// OLD
it('should do something', function (done) {
MyModule.method(param, function (err, result) {
expect(err).to.not.exist
expect(result).to.equal(expected)
done()
})
})
// NEW
it('should do something', async function () {
const result = await MyModule.promises.method(param)
expect(result).to.equal(expected)
})
```
#### Mock/Stub Patterns
```javascript
// For Redis or database mocks, ensure method chaining works
beforeEach(function () {
redis.multi = sinon.stub().returns({
sadd: sinon.stub().returnsThis(),
pexpire: sinon.stub().returnsThis(),
exec: sinon.stub().resolves(),
})
})
```
### 7. Specific Redis Patterns
#### Multi-Transaction Operations
```javascript
// Correct pattern for Redis multi operations
const multi = redis.multi()
multi.sadd(key, value)
multi.pexpire(key, ttl)
await multi.exec()
```
#### Single vs Multiple Key Operations
```javascript
// BAD - Multiple individual operations
const values = await Promise.all(keys.map(k => redis.get(k)))
// GOOD - Use batch operations when available
const values = await redis.mget(keys)
```
## Migration Checklist
### Before Starting
- [ ] Understand the original code's concurrency patterns
- [ ] Identify any background operations that should remain non-blocking
- [ ] Check if Redis batch operations can replace individual operations
- [ ] Look for internal method calls that might need test stubbing
### During Migration
- [ ] Convert function signatures (remove callbacks, add async)
- [ ] Replace callback patterns with await
- [ ] Handle early returns properly
- [ ] Preserve sequential processing unless there's a clear performance benefit
- [ ] Keep background operations non-blocking
- [ ] Avoid unnecessary OError.tag wrapping
- [ ] Update JSDoc comments to remove callback parameters
### After Migration
- [ ] Run comprehensive tests (fix Docker/environment issues if needed)
- [ ] Verify all background operations still work correctly
- [ ] Check that internal method calls can be stubbed if needed
- [ ] Ensure database operations don't overwhelm resources
- [ ] Validate error handling preserves meaningful context
- [ ] **Remove all decaffeinate artifacts from both implementation AND test files**
- [ ] Add explanatory comments for any non-obvious technical patterns
- [ ] Avoid selfRef patterns - use module exports routing instead
### Test Migration
- [ ] **Run tests EARLY and OFTEN during migration process**
- [ ] Convert test functions to async
- [ ] Update assertion patterns
- [ ] Fix mock/stub configurations for chained operations (Redis multi, etc.)
- [ ] Verify all test scenarios still pass
- [ ] Remove duplicate or unnecessary mock setups
- [ ] Clean up decaffeinate comments from test files
- [ ] Ensure internal method stubs work through promises interface
## Critical Lessons from Real Migration Experience
### 1. Testing Environment Issues
**ALWAYS run tests early and often during migration.** Don't wait until the end.
Common test running problems:
- Docker containers may need cleanup: `docker system prune -f`
- Use specific test grep patterns: `MOCHA_GREP="ModuleName" make test_unit_app`
- Mock objects must return proper objects for chaining (e.g., `multi()` must return `{method: stub().returnsThis(), ...}`)
### 2. Method Stubbing for Internal Calls
When methods call other methods internally, tests may need to stub those calls:
```javascript
// If methodA() calls methodB() internally and tests need to verify this:
// DON'T do this - creates unnecessary complexity:
const selfRef = { ... }; // BAD pattern
// DO this - route through the module exports interface:
moduleExports.promises.methodB(params).catch(err => {
logger.error({ err }, 'Failed to run background operation')
})
// Add a brief comment explaining the routing pattern:
// Route through moduleExports so tests can stub this call
```
### 3. Avoid the selfRef Pattern
The `selfRef` pattern should be avoided - it's a code smell that indicates better module structure is needed:
```javascript
// BAD - selfRef pattern
const selfRef = {}
selfRef.methodA = async function() {
await selfRef.methodB() // circular reference
}
// GOOD - route through module exports when stubbing is needed
async methodA() {
await moduleExports.promises.methodB() // testable
}
```
### 4. Complete Decaffeinate Cleanup
**Remove ALL legacy CoffeeScript artifacts** - this is a required part of the migration.
Look for and remove patterns like these (exact format may vary):
```javascript
/* eslint-disable */
// TODO: This file was created by bulk-decaffeinate.
/* eslint-disable
camelcase,
n/handle-callback-err,
max-len,
no-return-assign,
no-unused-vars,
*/
// Fix any style issues and re-enable lint.
/*
* decaffeinate suggestions:
* DS102: Remove unnecessary code created because of implicit returns
* DS207: Consider shorter variations of null checks
* Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
*/
```
**Search patterns to look for:**
- Comments containing "bulk-decaffeinate" or "decaffeinate"
- Large `/* eslint-disable */` blocks at the top of files
- Comments about "Fix any style issues and re-enable lint"
- "decaffeinate suggestions" comment blocks
Check **both implementation AND test files** for these artifacts.
### 5. Add Explanatory Comments for Non-Obvious Code
When you need to write "ugly" code for unavoidable technical reasons, add a brief comment explaining why:
```javascript
// Background operation - preserve fire-and-forget behavior
// Route through moduleExports so tests can stub this call
moduleExports.promises._checkSessions(user).catch(err => {
logger.error({ err }, 'Failed to check sessions in background')
})
```
This prevents future developers from "refactoring" the code and breaking functionality.
### 6. Simplify OError.tag() Usage
**With async/await, many OError.tag() wrappers can be removed.** OError.tag() was primarily used to preserve stack traces across callback boundaries, but async/await handles this automatically.
```javascript
// BEFORE - callback era (needed OError.tag for stack traces)
try {
await redis.multi().sadd(key, value).exec()
} catch (err) {
throw OError.tag(err, 'error adding to redis set', { key })
}
// AFTER - async/await preserves stack traces naturally
await redis.multi().sadd(key, value).exec()
```
**Keep OError.tag() only when:**
- Adding meaningful context that aids debugging
- Transforming low-level errors into domain-specific errors
- The wrapper adds significant value beyond just a descriptive message
**Remove OError.tag() when:**
- The error message doesn't add meaningful context
- The tag message just restates what the code obviously does
- Stack trace preservation was the only benefit
## Common Pitfalls to Avoid
1. **Over-parallelization**: Don't convert all sequential operations to parallel
2. **Unnecessary error wrapping**: Don't wrap every operation in try/catch just for OError.tag
3. **Breaking background operations**: Maintain fire-and-forget behavior where intended
4. **Exposing internal methods incorrectly**: Use the promises interface, not top-level exports
5. **Resource exhaustion**: Be mindful of database connection limits and Redis performance
6. **Test mock complexity**: Keep mocks simple and targeted to what's actually needed
7. **Using selfRef patterns**: Always route through module exports instead
8. **Forgetting decaffeinate cleanup**: Remove all legacy comments and eslint disables
9. **Not running tests early**: Run tests frequently during migration, not just at the end
10. **Missing explanatory comments**: Add brief comments for non-obvious technical patterns
## Success Metrics
- All existing tests pass without modification (except for async conversion)
- No new resource exhaustion issues under load
- Background operations continue to work as intended
- Error messages and logging remain informative
- Internal method stubbing works correctly for testing
- Code is cleaner and more maintainable than before
These instructions should be applied systematically, with careful consideration of the specific context and requirements of each module being migrated.
@@ -1,9 +1,7 @@
const OError = require('@overleaf/o-error')
const Settings = require('@overleaf/settings')
const logger = require('@overleaf/logger')
const Async = require('async')
const _ = require('lodash')
const { promisify } = require('util')
const { callbackifyAll } = require('@overleaf/promise-utils')
const UserSessionsRedis = require('./UserSessionsRedis')
const rclient = UserSessionsRedis.client()
@@ -13,246 +11,157 @@ const UserSessionsManager = {
return `sess:${sessionId}`
},
trackSession(user, sessionId, callback) {
async trackSession(user, sessionId) {
if (!user) {
return callback(null)
return
}
if (!sessionId) {
return callback(null)
return
}
const sessionSetKey = UserSessionsRedis.sessionSetKey(user)
const value = UserSessionsManager._sessionKey(sessionId)
rclient
.multi()
.sadd(sessionSetKey, value)
.pexpire(sessionSetKey, `${Settings.cookieSessionLength}`) // in milliseconds
.exec(function (err, response) {
if (err) {
OError.tag(
err,
'error while adding session key to UserSessions set',
{
user_id: user._id,
sessionSetKey,
}
)
return callback(err)
}
UserSessionsManager._checkSessions(user, function () {})
callback()
})
const multi = rclient.multi()
multi.sadd(sessionSetKey, value)
multi.pexpire(sessionSetKey, `${Settings.cookieSessionLength}`) // in milliseconds
await multi.exec()
UserSessionsManager._checkSessions(user).catch(err => {
logger.error({ err }, 'Failed to check sessions in background')
})
},
untrackSession(user, sessionId, callback) {
if (!callback) {
callback = function () {}
}
async untrackSession(user, sessionId) {
if (!user) {
return callback(null)
return
}
if (!sessionId) {
return callback(null)
return
}
const sessionSetKey = UserSessionsRedis.sessionSetKey(user)
const value = UserSessionsManager._sessionKey(sessionId)
rclient
.multi()
.srem(sessionSetKey, value)
.pexpire(sessionSetKey, `${Settings.cookieSessionLength}`) // in milliseconds
.exec(function (err, response) {
if (err) {
OError.tag(
err,
'error while removing session key from UserSessions set',
{
user_id: user._id,
sessionSetKey,
}
)
return callback(err)
}
UserSessionsManager._checkSessions(user, function () {})
callback()
})
const multi = rclient.multi()
multi.srem(sessionSetKey, value)
multi.pexpire(sessionSetKey, `${Settings.cookieSessionLength}`) // in milliseconds
await multi.exec()
UserSessionsManager._checkSessions(user).catch(err => {
logger.error({ err }, 'Failed to check sessions in background')
})
},
getAllUserSessions(user, exclude, callback) {
async getAllUserSessions(user, exclude) {
exclude = _.map(exclude, UserSessionsManager._sessionKey)
const sessionSetKey = UserSessionsRedis.sessionSetKey(user)
rclient.smembers(sessionSetKey, function (err, sessionKeys) {
if (err) {
OError.tag(err, 'error getting all session keys for user from redis', {
user_id: user._id,
})
return callback(err)
const sessionKeys = await rclient.smembers(sessionSetKey)
const filteredSessionKeys = _.filter(
sessionKeys,
k => !_.includes(exclude, k)
)
if (filteredSessionKeys.length === 0) {
logger.debug({ userId: user._id }, 'no other sessions found, returning')
return []
}
// Use sequential processing to avoid overwhelming Redis
const sessions = []
for (const key of filteredSessionKeys) {
const session = await rclient.get(key)
sessions.push(session)
}
const result = []
for (let session of sessions) {
if (!session) {
continue
}
sessionKeys = _.filter(sessionKeys, k => !_.includes(exclude, k))
if (sessionKeys.length === 0) {
logger.debug({ userId: user._id }, 'no other sessions found, returning')
return callback(null, [])
session = JSON.parse(session)
let sessionUser = session.passport && session.passport.user
if (!sessionUser) {
sessionUser = session.user
}
Async.mapSeries(
sessionKeys,
(k, cb) => rclient.get(k, cb),
function (err, sessions) {
if (err) {
OError.tag(err, 'error getting all sessions for user from redis', {
user_id: user._id,
})
return callback(err)
}
result.push({
ip_address: sessionUser.ip_address,
session_created: sessionUser.session_created,
})
}
const result = []
for (let session of Array.from(sessions)) {
if (!session) {
continue
}
session = JSON.parse(session)
let sessionUser = session.passport && session.passport.user
if (!sessionUser) {
sessionUser = session.user
}
result.push({
ip_address: sessionUser.ip_address,
session_created: sessionUser.session_created,
})
}
callback(null, result)
}
)
})
return result
},
/**
* @param {{_id: string}} user
* @param {string | null | undefined} retainSessionID - the session ID to exclude from deletion
* @param {(err: Error | null, data?: unknown) => void} callback
*/
removeSessionsFromRedis(user, retainSessionID, callback) {
async removeSessionsFromRedis(user, retainSessionID) {
if (!user) {
return callback(
new Error('bug: user not passed to removeSessionsFromRedis')
)
throw new Error('bug: user not passed to removeSessionsFromRedis')
}
const sessionSetKey = UserSessionsRedis.sessionSetKey(user)
rclient.smembers(sessionSetKey, function (err, sessionKeys) {
if (err) {
OError.tag(err, 'error getting contents of UserSessions set', {
user_id: user._id,
sessionSetKey,
})
return callback(err)
}
const keysToDelete = retainSessionID
? _.without(
sessionKeys,
UserSessionsManager._sessionKey(retainSessionID)
)
: sessionKeys
if (keysToDelete.length === 0) {
logger.debug(
{ userId: user._id },
'no sessions in UserSessions set to delete, returning'
)
return callback(null, 0)
}
const sessionKeys = await rclient.smembers(sessionSetKey)
const keysToDelete = retainSessionID
? _.without(sessionKeys, UserSessionsManager._sessionKey(retainSessionID))
: sessionKeys
if (keysToDelete.length === 0) {
logger.debug(
{ userId: user._id, count: keysToDelete.length },
'deleting sessions for user'
{ userId: user._id },
'no sessions in UserSessions set to delete, returning'
)
const deletions = keysToDelete.map(k => cb => rclient.del(k, cb))
Async.series(deletions, function (err, _result) {
if (err) {
OError.tag(err, 'error revoking all sessions for user', {
user_id: user._id,
sessionSetKey,
})
return callback(err)
}
rclient.srem(sessionSetKey, keysToDelete, function (err) {
if (err) {
OError.tag(err, 'error removing session set for user', {
user_id: user._id,
sessionSetKey,
})
return callback(err)
}
callback(null, keysToDelete.length)
})
})
})
},
touch(user, callback) {
if (!user) {
return callback(null)
return 0
}
const sessionSetKey = UserSessionsRedis.sessionSetKey(user)
rclient.pexpire(
sessionSetKey,
`${Settings.cookieSessionLength}`, // in milliseconds
function (err, response) {
if (err) {
OError.tag(err, 'error while updating ttl on UserSessions set', {
user_id: user._id,
})
return callback(err)
}
callback(null)
}
logger.debug(
{ userId: user._id, count: keysToDelete.length },
'deleting sessions for user'
)
// Use sequential processing to avoid overwhelming Redis
for (const key of keysToDelete) {
await rclient.del(key)
}
await rclient.srem(sessionSetKey, keysToDelete)
return keysToDelete.length
},
_checkSessions(user, callback) {
async touch(user) {
if (!user) {
return callback(null)
return
}
const sessionSetKey = UserSessionsRedis.sessionSetKey(user)
rclient.smembers(sessionSetKey, function (err, sessionKeys) {
if (err) {
OError.tag(err, 'error getting contents of UserSessions set', {
user_id: user._id,
sessionSetKey,
})
return callback(err)
await rclient.pexpire(sessionSetKey, `${Settings.cookieSessionLength}`)
},
async _checkSessions(user) {
if (!user) {
return
}
const sessionSetKey = UserSessionsRedis.sessionSetKey(user)
const sessionKeys = await rclient.smembers(sessionSetKey)
// Use sequential processing to avoid overwhelming Redis
for (const key of sessionKeys) {
const val = await rclient.get(key)
if (!val) {
await rclient.srem(sessionSetKey, key)
}
Async.series(
sessionKeys.map(
key => next =>
rclient.get(key, function (err, val) {
if (err) {
return next(err)
}
if (!val) {
rclient.srem(sessionSetKey, key, function (err, result) {
return next(err)
})
} else {
next()
}
})
),
function (err, results) {
callback(err)
}
)
})
}
},
}
UserSessionsManager.promises = {
getAllUserSessions: promisify(UserSessionsManager.getAllUserSessions),
removeSessionsFromRedis: (user, retainSessionID = null) =>
promisify(UserSessionsManager.removeSessionsFromRedis)(
user,
retainSessionID
),
untrackSession: promisify(UserSessionsManager.untrackSession),
module.exports = {
...callbackifyAll(UserSessionsManager),
promises: UserSessionsManager,
}
module.exports = UserSessionsManager
File diff suppressed because it is too large Load Diff