5d0a4b8db4ac0a08dbb477dd04245687322c5ec8
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Charles Bochet
|
1c3ae92c04 |
fix(server): prevent SSE stream teardown errors from crashing all pods (#21395)
## Context
In prod-eu, **all `twenty-server` API pods crash simultaneously**
several times per hour (then restart in lockstep) since ~Jun 2. Each
crash is an **unhandled promise rejection** in SSE event-stream teardown
— `exitCode=1`, identical stack on every one of the 7 pods:
```
Error: Failed to acquire lock for key: workspace:<id>:activeStreams
at CacheLockService.withLock (cache-lock.service.ts:53)
at async EventStreamService.destroyEventStream (event-stream.service.ts:75)
at async cleanup (wrap-async-iterator-with-lifecycle.ts)
at async Object.return (wrap-async-iterator-with-lifecycle.ts)
at async Object.cancel (graphql-yoga/.../result-processor/sse.js:68)
```
### Mechanism
1. A busy workspace's SSE clients reconnect (no client backoff), so
connect/disconnect contend on a **single per-workspace Redis lock**
`workspace:<id>:activeStreams`.
2. Under contention `CacheLockService.withLock` exhausts its retries and
**throws**.
3. In the `destroyEventStream` teardown path the throw escapes
`wrapAsyncIteratorWithLifecycle`'s `cleanup()` — `return()` does `try {
await cleanup() } finally { … }` and does **not** catch a cleanup throw.
4. graphql-yoga invokes this from `cancel()` as a **fire-and-forget**
`Promise.all` on connection abort. With **no global `unhandledRejection`
handler**, Node's default policy terminates the process with **exit code
1**.
5. The crash drops all that pod's SSE clients → they reconnect to
surviving pods → contention moves there → the whole fleet crashes
together → restarts → reconnect storm → repeats (~14 min period,
matching the metrics).
## Changes
Crash-stopping hotfix (defense in depth). Does **not** change the
locking design or client reconnect behavior — see follow-ups.
- **`wrapAsyncIteratorWithLifecycle`**: `onCleanup()` is now best-effort
— wrapped in try/catch so teardown can never reject out of
`next()`/`return()`/`throw()`. The original iterator error is still
rethrown unchanged. Adds an `onCleanupError` hook so the swallowed error
is still reported.
- **`EventStreamResolver`**: wires `onCleanupError` to
`ExceptionHandlerService.captureExceptions` (→ Sentry) with workspace +
channel context, so these failures stay visible.
- **`main.ts`**: registers a global `process.on('unhandledRejection')`
that reports via `ExceptionHandlerService` (Sentry) instead of letting
Node terminate. Registering the listener also suppresses Node's default
process-termination. Non-`Error` reasons are formatted with
`util.inspect` (per Copilot review) so Sentry gets a readable message
rather than `[object Object]`.
## Verification
The fix was checked against the exact crash path — a wrapped iterator
whose `onCleanup` rejects:
- `return()` (graphql-yoga's `cancel()` path) **resolves** instead of
rejecting → no unhandled rejection.
- `next()` on a completed stream **resolves** despite a rejecting
cleanup.
- A genuine iterator error still surfaces on `next()` (cleanup failure
doesn't mask it).
- `onCleanupError` receives the original `Error`.
All four pass. `nx lint twenty-server` + `oxfmt` clean; no new `tsc`
errors in the changed files.
## Follow-ups (not in this PR)
- Remove the unnecessary `withLock` around the already-atomic Redis
`SADD`/`SREM` in `event-stream.service.ts` (the contention source).
- Restore exponential backoff + jitter on the frontend SSE reconnect
(regressed in #21061) to stop the thundering herd.
- Infra: make `/healthz` a meaningful liveness signal and add
`maxUnavailable` + a PodDisruptionBudget so pods can't all die together.
|
||
|
Thomas Trompette
|
f4380f89a8 |
fix: SSE event stream reconnection after idle connection death (#21061)
The SSE event stream could silently die from network partitions, NAT
table flushes, browser tab throttling, or server restarts. When this
happened:
1. The `error` callback only called `captureException` — no reconnection
was triggered
2. The `complete` callback was `() => {}` — a cleanly terminated stream
left the client permanently broken
3. No mechanism existed to detect a silently dead connection where no
FIN/RST was received
## Summary
- **Fix `error`/`complete` callbacks**: The `graphql-sse` subscription's
`error` callback only reported to Sentry, and `complete` was a no-op.
Both now set `shouldDestroyEventStreamState = true` to trigger the
destroy-recreate lifecycle, ensuring detected transport failures and
clean stream terminations lead to automatic reconnection.
- **Add server-side keepalive**: The existing heartbeat timer now runs
every 30s (instead of 6min) and publishes empty events through the Redis
pub/sub channel in addition to refreshing the Redis TTL (throttled to
~6min). Unlike GraphQL Yoga's opaque SSE comment pings, these are real
subscription events that flow through the client's `next`/`message`
handlers.
- **Add client-side keepalive monitor (`SSEKeepAliveEffect`)**: Tracks
the timestamp of the last received event. If no event arrives within 90
seconds (3x the keepalive interval), it clears query listeners and
triggers a stream destroy-recreate cycle.
## Test plan
- [x] Start the app, verify SSE events flow normally (workflow runs
update in real-time)
- [x] Leave the app idle for >90 seconds, then trigger a workflow run —
verify the stream auto-reconnects and events are delivered
- [x] Kill the server, restart it, verify the frontend recovers its
event stream
- [x] Verify keepalive events (empty
`objectRecordEventsWithQueryIds`/`metadataEvents`) appear in browser
network tab every ~30s
- [x] Verify no regressions in SSE-dependent features (record updates,
metadata changes, workflow run visualization)
|
||
|
Thomas Trompette
|
95a34d8517 |
Return false instead of throwing when event stream does not exist (#20165)
## Summary - When an event stream expires (TTL), `addQueryToEventStream` and `removeQueryFromEventStream` now return `false` instead of throwing `EVENT_STREAM_DOES_NOT_EXIST` as an `InternalServerError` - Frontend checks the mutation return value and triggers the destroy/recreate cycle, same recovery behavior without the error path - Removes `EVENT_STREAM_DOES_NOT_EXIST` from exception code, exception filter, and frontend graceful error check since it's no longer thrown ## Test plan - [x] Verify that when an event stream TTL expires, the frontend silently recreates the stream without error noise in logs/Sentry - [x] Verify that `NOT_AUTHORIZED` errors still throw correctly on both mutations - [ ] Verify that the subscription `onEventSubscription` still works end-to-end with stream creation, query registration, and heartbeat TTL refresh Made with [Cursor](https://cursor.com) |
||
|
Thomas Trompette
|
d887fdc532 |
Stop throwing for event stream does not exists (#20008)
Fixes https://twenty-v7.sentry.io/issues/7351816489/?environment=prod&environment=prod-eu&project=4507072499810304&query=is%3Aunresolved%20%21issue.type%3A%5Bperformance_consecutive_db_queries%2Cperformance_consecutive_http%2Cperformance_file_io_main_thread%2Cperformance_db_main_thread%2Cperformance_n_plus_one_db_queries%2Cperformance_n_plus_one_api_calls%2Cperformance_p95_endpoint_regression%2Cperformance_slow_db_query%2Cperformance_render_blocking_asset_span%2Cperformance_uncompressed_assets%2Cperformance_http_overhead%2Cperformance_large_http_payload%5D%20timesSeen%3A%3E10&referrer=issue-stream&sort=date - When an SSE event stream already exists in Redis during a reconnection, the server now checks if the caller is the rightful owner (via isAuthorized) and destroys the stale stream before creating a fresh one, instead of throwing an EVENT_STREAM_ALREADY_EXISTS error - Unauthorized callers still receive the error, preserving the security guard against hijacking |
||
|
Charles Bochet
|
9cb21e71fa |
feat: secure and user-scope metadata resolvers for messaging infrastructure (#18787)
## Summary Builds on the messaging infrastructure migration (#18784) by securing and user-scoping all 4 metadata resolvers: ### DTOs secured - **ConnectedAccountDTO**: `@HideField()` on `accessToken`, `refreshToken`, `connectionParameters`, `oidcTokenClaims` - **MessageChannelDTO / CalendarChannelDTO**: `@HideField()` on `syncCursor` - **MessageFolderDTO**: `@HideField()` on `syncCursor`, `externalId` - **UpdateMessageFolderInputUpdates**: stripped to only `isSynced` (removed `name`, `syncCursor`, `pendingSyncAction`) ### Resolvers user-scoped via `@AuthUserWorkspaceId()` - `myConnectedAccounts` — returns only the calling user's accounts (no permission guard) - `myMessageChannels(connectedAccountId?)` — returns channels for the user's connected accounts - `myCalendarChannels(connectedAccountId?)` — same pattern - `myMessageFolders(messageChannelId?)` — returns folders through the ownership chain ### Admin-only listing with permission guard - `connectedAccounts` query retained with `SettingsPermissionGuard(CONNECTED_ACCOUNTS)` for admin listing of all workspace accounts ### Unsafe mutations removed - Removed `createConnectedAccount`, `updateConnectedAccount` (OAuth/IMAP flows create/refresh tokens server-side) - Removed `create*`/`delete*` mutations from MessageChannel, CalendarChannel, MessageFolder (managed by sync engine) ### Update mutations restricted with ownership verification - `deleteConnectedAccount(id)` — verifies `entity.userWorkspaceId === currentUserWorkspaceId` - `updateMessageChannel` / `updateCalendarChannel` / `updateMessageFolder` — verify ownership through connected account chain - New `OWNERSHIP_VIOLATION` exception codes map to `ForbiddenError` in GraphQL ### `@AuthUserWorkspaceId` decorator hardened - Added `allowUndefined` option (default: `false`) — throws `ForbiddenException` if `userWorkspaceId` is undefined (e.g. API key auth) - Existing callers updated to `@AuthUserWorkspaceId({ allowUndefined: true })` where needed - New user-scoped resolvers enforce non-undefined `userWorkspaceId` at decorator level ### Exception handler chaining - `MessageFolderGraphqlApiExceptionInterceptor`, `MessageChannelGraphqlApiExceptionInterceptor`, `CalendarChannelGraphqlApiExceptionInterceptor` chain upstream exception handling (ConnectedAccountException, MessageChannelException) for correct `ForbiddenError` propagation ### Metadata services enhanced - `findByUserWorkspaceId()`, `getUserConnectedAccountIds()`, `findByConnectedAccountIds()`, `findByMessageChannelIds()` - `findBy*ForUser()` methods encapsulate ownership checks before querying - `verifyOwnership()` on all 4 services with proper chain validation - Named parameters throughout for clarity ### Dev seeds for both schemas - Added JANE to connected account, message channel, calendar channel workspace seeds - Created message folder workspace seeds (TIM, JONY, JANE) - New `seed-metadata-entities.util.ts` seeds core schema tables (connectedAccount, messageChannel, calendarChannel, messageFolder) with same IDs as workspace seeds, mapping `accountOwnerId` → `userWorkspaceId` ### Integration tests (using seeds, not raw SQL) - 4 test suites (`connected-account`, `message-channel`, `calendar-channel`, `message-folder`) - Tests use seeded data IDs from seed constants — no raw SQL inserts/deletes - Tests read via GraphQL resolvers - Tests cover: user scoping, admin permission checks, sensitive field exclusion, ownership enforcement on mutations ### Frontend migration - Feature-flag-gated hooks (`useMyConnectedAccounts`, `useMyMessageChannels`, `useMyCalendarChannels`, `useMyMessageFolders`) - When `IS_CONNECTED_ACCOUNT_MIGRATED` is on: hooks use metadata API (`POST /metadata`) - When flag is off: hooks use existing workspace API (`POST /graphql`, current behavior) - Settings account pages updated to use new hooks - `useEffect` extracted to `SettingsAccountsSelectedMessageChannelEffect` component per project conventions - Error messages translated with Lingui ## Test plan - [x] Server typecheck passes - [x] Server lint passes - [x] Server unit tests pass (477 suites, 4269 tests) - [x] Frontend typecheck passes - [x] Frontend lint passes - [x] Integration tests verify user-scoping, ownership enforcement, hidden fields - [ ] CI green --------- Co-authored-by: neo773 <neo773@protonmail.com> |
||
|
Thomas Trompette
|
5526d2e5d0 |
Separate metadata and object record publisher (#18740)
- Separate both publishers - Renaming - Removal of the old SSE endpoint --------- Co-authored-by: Charles Bochet <charles@twenty.com> |