Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: prevent race condition when updating job results #1867

Merged
merged 2 commits into from Mar 19, 2024
Merged

fix: prevent race condition when updating job results #1867

merged 2 commits into from Mar 19, 2024

Conversation

TBonnin
Copy link
Collaborator

@TBonnin TBonnin commented Mar 18, 2024

locking the job row to prevent concurrent call to UpdateSyncJobResult to overwrite each other

Issue ticket number and link

https://linear.app/nango/issue/NAN-577/persist-handle-concurrent-access

Checklist before requesting a review (skip if just adding/editing APIs & templates)

  • I added tests, otherwise the reason is:
  • I added observability, otherwise the reason is:
  • I added analytics, otherwise the reason is:

@linear Linear
Copy link

linear bot commented Mar 18, 2024

NAN-577 Persist: handle concurrent access

TBonnin
TBonnin commented Mar 18, 2024
View reviewed changes
packages/shared/lib/services/sync/job.service.ts Outdated
}
};
return db.knex.transaction(async (trx) => {
const { result: existingResult } = await schema().from<SyncJob>(SYNC_JOB_TABLE).select('result').where({ id }).first().forUpdate();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using transaction and select for update to lock the row.

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 18, 2024
View reviewed changes
packages/shared/lib/services/sync/job.service.ts
finalResult[model].deleted = deletedValue + incomingDeletedValue;
}
if (deletedValue !== 0 || incomingDeletedValue !== 0) {
finalResult[model].deleted = deletedValue + incomingDeletedValue;

Check warning

Code scanning / CodeQL

Prototype-polluting assignment Medium

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
user controlled input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
user controlled input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
user controlled input
.
@TBonnin TBonnin force-pushed the tbonnin/NAN-577/update-job-results-lock branch 3 times, most recently from 979d6b4 to 06c4161 Compare March 18, 2024 14:37
bodinsamuel
bodinsamuel reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👌🏻
You have a failing test, I'm not sure it's related but let me know ☺️

@TBonnin TBonnin force-pushed the tbonnin/NAN-577/update-job-results-lock branch 2 times, most recently from e5f4cde to 5fac4c9 Compare March 18, 2024 16:50
@TBonnin TBonnin force-pushed the tbonnin/NAN-577/update-job-results-lock branch from 5fac4c9 to 50d9b52 Compare March 18, 2024 17:25
@TBonnin
Copy link
Collaborator Author

TBonnin commented Mar 18, 2024 •
edited

Looks good 👌🏻 You have a failing test, I'm not sure it's related but let me know ☺️

Not sure what's going on with the test. The select times out now it is in a transaction

@TBonnin
Copy link
Collaborator Author

TBonnin commented Mar 18, 2024 •
edited

Looks good 👌🏻 You have a failing test, I'm not sure it's related but let me know ☺️

Not sure what's going on with the test. The select times out now it is in a transaction.

My bad. I hadn't followed the 'DO NOT mock the database' rule and I got bitten. Removed the db mocking in 2018920

@TBonnin TBonnin force-pushed the tbonnin/NAN-577/update-job-results-lock branch from 8e222b9 to 2018920 Compare March 18, 2024 20:34
bodinsamuel
bodinsamuel approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good ✅
Took the opportunity to express some concerns but not related to this PR ☺️

packages/persist/lib/server.integration.test.ts
} from '@nangohq/shared';

describe('Persist API', () => {
const port = 3096;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be cool to use https://www.npmjs.com/package/get-port

packages/persist/lib/server.integration.test.ts Outdated
);
expect(response.status).toEqual(400);
const respBody = (await response.json()) as any[];
expect(respBody[0]['errors']['issues'][0]['path'][0]).toEqual('records');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to this pr: Could be interesting to test the full body with toMatchSnapshot or toMatchObject since we don't have a lot of tests regarding this

packages/persist/lib/server.integration.test.ts
}
}
);
expect(response.status).toEqual(201);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to this PR: this test do not actually check if the records were deleted

packages/persist/lib/server.integration.test.ts
}
}
);
expect(response.status).toEqual(201);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment

packages/persist/lib/server.integration.test.ts
const connection = (await connectionService.getConnectionById(connectionId)) as Connection;
if (!connection) throw new Error('Connection not found');

const sync = await createSync(connectionId, 'sync-test');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to this pr: the fact that we try/catch in models or don't throw makes those boilerplate so unnecessary huge 😩

packages/persist/lib/server.integration.test.ts
};

const clearDb = async () => {
await db.knex.raw(`DROP SCHEMA nango CASCADE`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should invest in tests helpers to avoid repeating those stuff

packages/shared/lib/services/sync/job.service.ts
}
};
return db.knex.transaction(async (trx) => {
const { result: existingResult } = await trx.from<SyncJob>(SYNC_JOB_TABLE).select('result').forUpdate().where({ id }).first();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love that the fix is so small <3

@TBonnin TBonnin force-pushed the tbonnin/NAN-577/update-job-results-lock branch from 2018920 to f9fee65 Compare March 19, 2024 14:27
@TBonnin
fix: prevent race condition when updating job results
c562c23 
locking the job row to prevent concurrent call to UpdateSyncJobResult to
overwrite each other
@TBonnin
one shouldn't mock the database
6a78eb0
@TBonnin TBonnin force-pushed the tbonnin/NAN-577/update-job-results-lock branch from f9fee65 to 6a78eb0 Compare March 19, 2024 15:09
@TBonnin
Copy link
Collaborator Author

TBonnin commented Mar 19, 2024

thank you @bodinsamuel. All your points are valid (I have updated the error matching test) and I will improve the tests as part of the track_delete and records migration

@TBonnin TBonnin merged commit 3c02fa9 into master Mar 19, 2024
15 of 16 checks passed
@TBonnin TBonnin deleted the tbonnin/NAN-577/update-job-results-lock branch March 19, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bodinsamuel bodinsamuel bodinsamuel approved these changes

@khaliqgant khaliqgant khaliqgant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@TBonnin @bodinsamuel @khaliqgant