Push notifications - #231
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Greptile SummaryThis PR adds browser Web Push notifications to the existing notifications service, including a new
Confidence Score: 3/5Not safe to merge as-is — the notification worker now permanently swallows transient email failures with no retry path, and the push subscription endpoint has an upsert that allows one user to silently steal another user's subscription. The DB-first notification ordering combined with swallowed email errors means a temporary SMTP failure permanently drops the email — the user gets an in-app record but no email, and the deduplication check blocks any future retry attempt. The push subscription upsert (ON CONFLICT endpoint DO UPDATE SET user_id) allows any authenticated user to claim an endpoint already registered to another user, silently redirecting or cutting off that user's notifications. services/workers/src/workers/notifications.ts (email retry regression) and services/api/src/routes/push.ts (ON CONFLICT upsert cross-user hijacking, SSRF TOCTOU — both called out in prior review threads and still unaddressed) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Browser
participant API
participant DB
participant Worker
participant PushService as Push Service (VAPID)
participant SMTP
Browser->>API: POST /api/v1/users/me/push-subscriptions
API->>API: validatePushEndpoint (HTTPS + DNS check)
API->>DB: advisory lock (userId)
API->>DB: check existing / count limit
API->>DB: INSERT push_subscriptions (ON CONFLICT upsert)
API-->>Browser: 201 Created / 200 OK
Note over Worker,DB: Notification job consumed from pg-boss
Worker->>DB: SELECT ens_names
Worker->>DB: SELECT users (email_verified check)
Worker->>DB: INSERT notifications RETURNING id
Worker->>SMTP: sendEmail (try/catch, no retry on failure)
Worker->>DB: SELECT push_subscriptions WHERE user_id AND enabled
Worker->>PushService: webPush.sendNotification per subscription
PushService-->>Worker: 410 Gone, DELETE stale subscription
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Browser
participant API
participant DB
participant Worker
participant PushService as Push Service (VAPID)
participant SMTP
Browser->>API: POST /api/v1/users/me/push-subscriptions
API->>API: validatePushEndpoint (HTTPS + DNS check)
API->>DB: advisory lock (userId)
API->>DB: check existing / count limit
API->>DB: INSERT push_subscriptions (ON CONFLICT upsert)
API-->>Browser: 201 Created / 200 OK
Note over Worker,DB: Notification job consumed from pg-boss
Worker->>DB: SELECT ens_names
Worker->>DB: SELECT users (email_verified check)
Worker->>DB: INSERT notifications RETURNING id
Worker->>SMTP: sendEmail (try/catch, no retry on failure)
Worker->>DB: SELECT push_subscriptions WHERE user_id AND enabled
Worker->>PushService: webPush.sendNotification per subscription
PushService-->>Worker: 410 Gone, DELETE stale subscription
Reviews (4): Last reviewed commit: "Add prettier config, reformat documents" | Re-trigger Greptile |
| if (notificationJobs.length > 0) { | ||
| await Promise.all( | ||
| notificationJobs.map((notificationJob) => boss.send(QUEUE_NAMES.SEND_NOTIFICATION, notificationJob)) | ||
| ); | ||
| logger.info( | ||
| { count: notificationJobs.length, ensNameId, ensName }, | ||
| 'Notification jobs queued for unfunded listings' | ||
| ); |
There was a problem hiding this comment.
Notification jobs dispatched after
COMMIT — non-atomic with DB changes
boss.send() is called after client.query('COMMIT'), so if the queue insertion fails (network blip, pg-boss restart) the transaction is permanently committed (ownership updated, listings unfunded) but the seller notifications are silently dropped. The catch block then calls ROLLBACK against an already-closed transaction, which is a no-op and swallows the queue error behind a misleading log.
The existing validation jobs at line 126 use boss.insert(validationJobs) inside the transaction (before COMMIT), which is the correct pattern — pg-boss supports transactional job insertion through a shared pg client. The new notification jobs should follow the same approach: build notificationJobs as before, then call await boss.insert(notificationJobs.map(j => ({ name: QUEUE_NAMES.SEND_NOTIFICATION, data: j }))) before COMMIT, so notification dispatch is atomic with the DB update.
| // Log notification in database as the canonical in-app notification | ||
| let notificationId: number | undefined; | ||
| if (userId) { | ||
| await pool.query( | ||
| const insertedNotification = await pool.query<InsertedNotificationRow>( | ||
| `INSERT INTO notifications (user_id, type, ens_name_id, metadata, sent_at) | ||
| VALUES ($1, $2, $3, $4, NOW())`, | ||
| [userId, type, ensNameId, JSON.stringify(metadata || {})] | ||
| VALUES ($1, $2, $3, $4, NOW()) | ||
| RETURNING id`, | ||
| [userId, type, ensNameId, JSON.stringify(metadata || {})], | ||
| ); | ||
| notificationId = insertedNotification.rows[0].id; | ||
| } | ||
|
|
||
| if (recipientEmail && emailTemplate) { | ||
| try { | ||
| await sendEmail(recipientEmail, emailTemplate); | ||
| } catch (emailError) { | ||
| logger.warn( | ||
| { | ||
| error: emailError, | ||
| userId, | ||
| type, | ||
| ensNameId, | ||
| email: recipientEmail, | ||
| }, | ||
| 'Email notification delivery failed after canonical notification was logged', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (userId && notificationId !== undefined) { | ||
| try { | ||
| await sendPushNotifications({ | ||
| userId, | ||
| type, | ||
| ensName, | ||
| notificationId, | ||
| metadata, | ||
| }); | ||
| } catch (pushError) { | ||
| logger.warn( | ||
| { error: pushError, userId, type, ensNameId }, | ||
| 'Push notification delivery failed after canonical notification was logged', | ||
| ); | ||
| } | ||
|
|
||
| logger.info({ userId, type, ensNameId, email: recipientEmail }, 'Notification sent and logged'); |
There was a problem hiding this comment.
Transient email failures are now permanently unrecoverable
The PR moves the INSERT INTO notifications before email/push delivery (to obtain notificationId), and wraps email sending in a try/catch that swallows failures. This breaks pg-boss retry semantics for email: when email fails transiently (SMTP timeout, rate limit, etc.), the job completes successfully with a DB record in place. Any subsequent retry by pg-boss will find that record in the deduplication check at line 126 and skip the notification entirely — the user never receives the email.
In the previous code, email failure propagated to the outer catch block (line 337), which rethrew and triggered pg-boss retry. At retry time, no DB record existed yet, so the full flow would run again. That safety net is gone.
Push notifications added to the existing notifications service, which will only send push notifications for users with verifired emails set.