Skip to content

Commit

Permalink
fix: move triggering actions out of transations (#3215)
Browse files Browse the repository at this point in the history
When removing failing connection in SlackService, we used to trigger
actions in the middle of a db transaction that is locking rows. We also
do all sort of async operations like logging. If the actions for
instance are experiencing any kind of issue, the transaction is kept
open and waiting until the actions times out or the db cancels the
transaction, holding the db connection during all this time.
This commits moves the actual sending of the Slack notification after
the db has been updated. We run into the risk of updating the db and not
sending the notification but we cannot currently guarantee those
operations to be fully atomic without potentially locking the db for a
very long time.
There might be a possibility to now reduce the transaction into a single
atomic query but I didn't want this change to be too heavy right before
the holidays
  • Loading branch information
TBonnin authored Dec 22, 2024
1 parent 84e8349 commit 088a5f6
Showing 1 changed file with 13 additions and 5 deletions.
18 changes: 13 additions & 5 deletions packages/shared/lib/services/notification/slack.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ export class SlackService {
environment_id: number;
provider: string;
}): Promise<void> {
await db.knex.transaction(async (trx) => {
const update = await db.knex.transaction(async (trx) => {
const slackNotificationsEnabled = await environmentService.getSlackNotificationsEnabled(nangoConnection.environment_id);
if (!slackNotificationsEnabled) {
return;
Expand Down Expand Up @@ -638,7 +638,15 @@ export class SlackService {
connection_list,
updated_at: new Date()
});
return {
id,
slackTimestamp: slack_timestamp,
adminSlackTimestamp: admin_slack_timestamp,
connectionCount: connection_list.length
};
});

if (update) {
// we report resolution to the slack channel which could be either
// 1) The slack notification is resolved, connection_list === 0
// 2) The list of failing connections has been decremented
Expand All @@ -649,11 +657,11 @@ export class SlackService {
originalActivityLogId,
environment_id,
provider,
slack_timestamp as string,
admin_slack_timestamp as string,
connection_list.length
update.slackTimestamp as string,
update.adminSlackTimestamp as string,
update.connectionCount
);
});
}
}

async closeAllOpenNotificationsForEnv(environment_id: number): Promise<void> {
Expand Down

0 comments on commit 088a5f6

Please sign in to comment.