Fix updateUsers Cron Tasks Has Unprotected Promises - #387
Conversation
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (6)packages/matrix-identity-server/src/**/*.ts📄 CodeRabbit inference engine (packages/matrix-identity-server/src/AGENTS.md)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,tsx,jsx}📄 CodeRabbit inference engine (CODING_STYLE.md)
Files:
**/index.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CODING_STYLE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-03-31T07:26:27.898ZApplied to files:
🪛 GitHub Check: CodeQLpackages/matrix-identity-server/src/cron/index.ts[failure] 108-108: Clear-text logging of sensitive information packages/matrix-identity-server/src/cron/updateUsers.ts[failure] 164-164: Clear-text logging of sensitive information 🔇 Additional comments (4)
📝 WalkthroughFundamental flaw fixed
Core changes (systemic data flow + algorithm)
Legacy code removed / replaced
Technical debt (explicitly not addressed)
WalkthroughChangesThe User synchronization
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit aad4b28
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58a8c6fc-959e-4104-9ae7-f5a7d1a46fdf
📒 Files selected for processing (1)
packages/matrix-identity-server/src/cron/updateUsers.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Security / CodeQL
- GitHub Check: Docs / Update Documentation
- GitHub Check: Security / njsscan
- GitHub Check: Build / Build Affected Packages
🧰 Additional context used
📓 Path-based instructions (5)
packages/matrix-identity-server/src/**/*.ts
📄 CodeRabbit inference engine (packages/matrix-identity-server/src/AGENTS.md)
All Matrix Identity Server endpoints must be mounted at
/_matrix/identity/v2/prefix
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{js,ts,jsx,tsx}: Code must follow the philosophy of simplicity over cleverness - junior developers should understand code in 30 seconds, avoid metaprogramming, deep generics, decorator magic, and prefer readable for loops over complex chains like .reduce().flatMap().filter()
Code must follow the philosophy of explicit over implicit - dependencies must be injected not imported globally, errors must be typed not caught-and-rethrown, data flows must be traceable through function signatures
Code comments must explain why, never what - the code itself explains what
Do not use // comments to disable code - delete dead code instead, as Git has history
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Code must follow the philosophy of boundaries over conventions - use module facades enforced by lint rules instead of comments, prefer#privatefields over naming conventions, prefer TypeScript types over JSDoc comments
Do not introduce new any types in TypeScript - warnings are existing tech debt, new ones are blockersEnforce TypeScript strict mode across all packages in ToM-Server
**/*.{ts,tsx}: Use PascalCase for types, interfaces, classes, and enums
Return types must be explicit on all non-trivial functions
Every function must return a meaningful value (void is forbidden)
Use ActionResult type for functions that perform actions with no natural data return
Use Result<T, E> type for functions that produce data, making failure first-class
anytype is forbidden without exception
as unknown as Tdouble casting is forbidden without exception
Useunknownoveranyfor data from external sources (HTTP, JSON.parse, databases)
Prefertypefor unions and intersections,interfacefor object shapes
Avoid TypeScriptenum; use string union types for internal values
Provide type guards and validation helpers for string unions that cross system boundaries
Use Result or ActionResult for expected, domain-meaningful failures (not exceptions)
Use Error.cause when wrapping or rethrowing errors to preserve the original error chain
Type caught errors correctly usinginstanceofchecks (not casting unknown to Error)
Prefer functions and plain objects over classes; use classes only for genuine encapsulation with private mutable state and lifecycle
Do not use static-only classes to group related functions; use named exports instead
Classes must have a single responsibility
@ts-ignoreand@ts-expect-errormust have a written explanation
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ES modules (type: module) throughout the ToM-Server project
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (CODING_STYLE.md)
**/*.{js,ts,tsx,jsx}: Use 2 spaces for indentation (not 4, not tabs)
Opening braces must go on the same line (never on a new line)
Use trailing commas in multi-line structures
Semicolons are required on all statements
Maximum line length is 120 characters (hard limit)
Use camelCase for variables and functions
Use SCREAMING_SNAKE_CASE only for module-level primitive constants that never change
Boolean variables must use is/has/can prefix (e.g., isLoading, hasPermission, canRetry)
Do not abbreviate variable names beyond accepted list (i, j, e, err, ctx, req, res)
Functions must have a single responsibility (no 'and' in function names)
Usefunctiondeclarations for named, standalone, exported units; use arrow functions for callbacks and inline helpers
Maximum 5 function arguments (use options object for more parameters)
Keep functions short (25-40 lines maximum, fit on one screen without scrolling)
Recursion must be tail-call only, or use an iterative loop instead
Maximum 2 levels of nesting (no level 3)
Use early returns to reduce nesting and establish preconditions
Avoidelseafter areturn
Throw exceptions only for invariant violations and programming errors
Catch errors at system boundaries (HTTP handlers, job runners, event listeners), not deep in business logic
Never swallow errors silently (no empty catch blocks)
finallyis for cleanup only, not for conditional logic
Import from specific files, not barrel exports
Organize imports in order: Node built-ins, external packages, internal absolute paths, internal relative paths (with blank lines between groups)
Comments must explain why, not what
Document function contracts in JSDoc, not implementation mechanics
TODO comments must have an owner name and ticket reference
Preferasync/awaitover.then()chains
Run independent async operations in parallel using Promise.all
Never fire-and-forget async operations without a .catch() handler
Use===instead of==(never use loose equality)
Do not use mutabl...
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
🧠 Learnings (1)
📚 Learning: 2026-03-31T07:26:27.898Z
Learnt from: pm-McFly
Repo: linagora/ToM-server PR: 355
File: packages/matrix-identity-server/src/db/index.ts:413-413
Timestamp: 2026-03-31T07:26:27.898Z
Learning: When reviewing TypeScript code in this repo, follow Biome’s `noDoubleEquals` rule: do not use loose equality (`== null` / `!= null`) as a shorthand. For nullish checks, use explicit strict comparisons instead (e.g., `value === null || value === undefined` or `value !== null && value !== undefined`).
Applied to files:
packages/matrix-identity-server/src/cron/updateUsers.ts
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/matrix-identity-server/src/cron/updateUsers.ts (1)
114-132: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftStop routing evaluation output through hidden mutation.
evaluateUserreturnsvoidwhile mutatingstate.tasksandstate.toUpdate. Return a typed evaluation/result and merge it inupdateUsersso the data flow is explicit.As per coding guidelines, “data flows must be traceable through function signatures” and “Every function must return a meaningful value (void is forbidden)”.
Source: Coding guidelines
♻️ Duplicate comments (1)
packages/matrix-identity-server/src/cron/updateUsers.ts (1)
52-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log raw database initialization errors.
Both catch blocks serialize
err; config-backed database errors can include DSNs or password-derived fields. CodeQL already reports this flow at Line 169. Log a fixed event plus a safe error category, unless logger redaction is verified.
packages/matrix-identity-server/src/cron/updateUsers.ts#L52-L54: replace raw Matrix DB error logging with sanitized diagnostics.packages/matrix-identity-server/src/cron/updateUsers.ts#L168-L170: apply the same sanitization to aggregate initialization failures.#!/bin/bash set -euo pipefail rg -n -i -C2 '(redact|password|secret|token|serialize)' \ packages/logger/src packages/matrix-identity-server/srcSource: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65259e69-d917-44ba-8151-1084cb0c0bfb
📒 Files selected for processing (1)
packages/matrix-identity-server/src/cron/updateUsers.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
packages/matrix-identity-server/src/**/*.ts
📄 CodeRabbit inference engine (packages/matrix-identity-server/src/AGENTS.md)
All Matrix Identity Server endpoints must be mounted at
/_matrix/identity/v2/prefix
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{js,ts,jsx,tsx}: Code must follow the philosophy of simplicity over cleverness - junior developers should understand code in 30 seconds, avoid metaprogramming, deep generics, decorator magic, and prefer readable for loops over complex chains like .reduce().flatMap().filter()
Code must follow the philosophy of explicit over implicit - dependencies must be injected not imported globally, errors must be typed not caught-and-rethrown, data flows must be traceable through function signatures
Code comments must explain why, never what - the code itself explains what
Do not use // comments to disable code - delete dead code instead, as Git has history
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Code must follow the philosophy of boundaries over conventions - use module facades enforced by lint rules instead of comments, prefer#privatefields over naming conventions, prefer TypeScript types over JSDoc comments
Do not introduce new any types in TypeScript - warnings are existing tech debt, new ones are blockersEnforce TypeScript strict mode across all packages in ToM-Server
**/*.{ts,tsx}: Use PascalCase for types, interfaces, classes, and enums
Return types must be explicit on all non-trivial functions
Every function must return a meaningful value (void is forbidden)
Use ActionResult type for functions that perform actions with no natural data return
Use Result<T, E> type for functions that produce data, making failure first-class
anytype is forbidden without exception
as unknown as Tdouble casting is forbidden without exception
Useunknownoveranyfor data from external sources (HTTP, JSON.parse, databases)
Prefertypefor unions and intersections,interfacefor object shapes
Avoid TypeScriptenum; use string union types for internal values
Provide type guards and validation helpers for string unions that cross system boundaries
Use Result or ActionResult for expected, domain-meaningful failures (not exceptions)
Use Error.cause when wrapping or rethrowing errors to preserve the original error chain
Type caught errors correctly usinginstanceofchecks (not casting unknown to Error)
Prefer functions and plain objects over classes; use classes only for genuine encapsulation with private mutable state and lifecycle
Do not use static-only classes to group related functions; use named exports instead
Classes must have a single responsibility
@ts-ignoreand@ts-expect-errormust have a written explanation
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ES modules (type: module) throughout the ToM-Server project
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (CODING_STYLE.md)
**/*.{js,ts,tsx,jsx}: Use 2 spaces for indentation (not 4, not tabs)
Opening braces must go on the same line (never on a new line)
Use trailing commas in multi-line structures
Semicolons are required on all statements
Maximum line length is 120 characters (hard limit)
Use camelCase for variables and functions
Use SCREAMING_SNAKE_CASE only for module-level primitive constants that never change
Boolean variables must use is/has/can prefix (e.g., isLoading, hasPermission, canRetry)
Do not abbreviate variable names beyond accepted list (i, j, e, err, ctx, req, res)
Functions must have a single responsibility (no 'and' in function names)
Usefunctiondeclarations for named, standalone, exported units; use arrow functions for callbacks and inline helpers
Maximum 5 function arguments (use options object for more parameters)
Keep functions short (25-40 lines maximum, fit on one screen without scrolling)
Recursion must be tail-call only, or use an iterative loop instead
Maximum 2 levels of nesting (no level 3)
Use early returns to reduce nesting and establish preconditions
Avoidelseafter areturn
Throw exceptions only for invariant violations and programming errors
Catch errors at system boundaries (HTTP handlers, job runners, event listeners), not deep in business logic
Never swallow errors silently (no empty catch blocks)
finallyis for cleanup only, not for conditional logic
Import from specific files, not barrel exports
Organize imports in order: Node built-ins, external packages, internal absolute paths, internal relative paths (with blank lines between groups)
Comments must explain why, not what
Document function contracts in JSDoc, not implementation mechanics
TODO comments must have an owner name and ticket reference
Preferasync/awaitover.then()chains
Run independent async operations in parallel using Promise.all
Never fire-and-forget async operations without a .catch() handler
Use===instead of==(never use loose equality)
Do not use mutabl...
Files:
packages/matrix-identity-server/src/cron/updateUsers.ts
🧠 Learnings (1)
📚 Learning: 2026-03-31T07:26:27.898Z
Learnt from: pm-McFly
Repo: linagora/ToM-server PR: 355
File: packages/matrix-identity-server/src/db/index.ts:413-413
Timestamp: 2026-03-31T07:26:27.898Z
Learning: When reviewing TypeScript code in this repo, follow Biome’s `noDoubleEquals` rule: do not use loose equality (`== null` / `!= null`) as a shorthand. For nullish checks, use explicit strict comparisons instead (e.g., `value === null || value === undefined` or `value !== null && value !== undefined`).
Applied to files:
packages/matrix-identity-server/src/cron/updateUsers.ts
🪛 GitHub Check: CodeQL
packages/matrix-identity-server/src/cron/updateUsers.ts
[failure] 169-169: Clear-text logging of sensitive information
This logs sensitive data returned by an access to DATABASE_PASSWORD as clear text.
This logs sensitive data returned by an access to database_password as clear text.
This logs sensitive data returned by an access to LDAP_PASSWORD as clear text.
This logs sensitive data returned by an access to ldap_password as clear text.
This logs sensitive data returned by an access to MATRIX_DATABASE_PASSWORD as clear text.
This logs sensitive data returned by an access to matrix_database_password as clear text.
This logs sensitive data returned by an access to USERDB_PASSWORD as clear text.
This logs sensitive data returned by an access to userdb_password as clear text.
This logs sensitive data returned by process environment as clear text.
This logs sensitive data returned by an access to sms_api_key as clear text.
This logs sensitive data returned by an access to SMS_API_KEY as clear text.
This logs sensitive data returned by an access to MATRIX_ADMIN_PASSWORD as clear text.
This logs sensitive data returned by an access to DATABASE_PASSWORD as clear text.
This logs sensitive data returned by an access to database_password as clear text.
This logs sensitive data returned by an access to LDAP_PASSWORD as clear text.
This logs sensitive data returned by an access to ldap_password as clear text.
This logs sensitive data returned by an access to MATRIX_DATABASE_PASSWORD as clear text.
This logs sensitive data returned by an access to matrix_database_password as clear text.
This logs sensitive data returned by an access to SMTP_PASSWORD as clear text.
This logs sensitive data returned by an access to smtp_password as clear text.
This logs sensitive data returned by an access to USERDB_PASSWORD as clear text.
This logs sensitive data returned by an access to userdb_password as clear text.
This logs sensitive data returned by an access to sms_api_key as clear text.
This logs sensitive data returned by an access to SMS_API_KEY as clear text.
This logs sensitive data returned by an access to MATRIX_ADMIN_PASSWORD as clear text.
This logs sensitive data returned by an access to DATABASE_PASSWORD as clear text.
This logs sensitive data returned by an access to database_password as clear text.
This logs sensitive data returned by process environment as clear text.
This logs sensitive data returned by an access to LDAP_PASSWORD as clear text.
This logs sensitive data returned by an access to ldap_password as clear text.
This logs sensitive data returned by an access to MATRIX_DATABASE_PASSWORD as clear text.
This logs sensitive data returned by an access to matrix_database_password as clear text.
This logs sensitive data returned by an access to sms_api_key as clear text.
This logs sensitive data returned by an access to SMS_API_KEY as clear text.
This logs sensitive data returned by an access to MATRIX_ADMIN_PASSWORD as clear text.
This logs sensitive data returned by an access to SMTP_PASSWORD as clear text.
This logs sensitive data returned by an access to smtp_password as clear text.
🔇 Additional comments (3)
packages/matrix-identity-server/src/cron/updateUsers.ts (3)
72-76: LGTM!
103-112: LGTM!Also applies to: 136-139
163-167: LGTM!Also applies to: 175-185
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
This PR refactors the
updateUserscron task to improve performance, reliability, and stability. It introduces chunked data processing, migrates from monolithic promise chains to a queued task execution model to prevent connection pool exhaustion.Why
This change addresses a critical stability issue where malformed user records (e.g., empty
uidvalues) were causing unhandled promise rejections that crashed the entire Node.js process. Additionally, the existing implementation suffered from PostgreSQL integer overflow errors on thetimestampfield due to millisecond-precision epochs, and performance issues where simultaneous queries during high-volume syncs exhausted database connection limits.Closes #386
How
The implementation was refactored into modular, single-responsibility functions under 20 lines each.
toMatrixIdSafeand wrapped processing logic in try/catch blocks to ensure malformed entries are logged and skipped rather than triggering process-wide failures.executeTasksto process DB write operations in controlled chunks of 50, preventing connection pool saturation.Math.floor(epoch() / 1000)to conform to the 32-bitINTEGERtype constraint in the PostgreSQL schema.