Skip to content

fix: defer reading of database configuration until needed to fix race#1316

Merged
jescalada merged 18 commits intofinos:mainfrom
kriswest:1313-config-race-condition-b
Dec 13, 2025
Merged

fix: defer reading of database configuration until needed to fix race#1316
jescalada merged 18 commits intofinos:mainfrom
kriswest:1313-config-race-condition-b

Conversation

@kriswest
Copy link
Contributor

@kriswest kriswest commented Dec 9, 2025

resolves #1313
supercedes #1314

By deferring the setup of the database adaptors until they are first used, we can defer the retrieval of the DB config allowing the user configuration to be loaded before we attempt to use it.

@kriswest kriswest requested a review from coopernetes December 9, 2025 15:47
@netlify
Copy link

netlify bot commented Dec 9, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit bb8d97a
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/693d681d10cdfb000872a97c

@kriswest kriswest requested a review from jescalada December 9, 2025 15:47
@github-actions github-actions bot added the fix label Dec 9, 2025
@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

❌ Patch coverage is 84.81013% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.69%. Comparing base (5d1a727) to head (bb8d97a).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
src/db/mongo/helper.ts 0.00% 6 Missing ⚠️
src/db/index.ts 91.37% 5 Missing ⚠️
src/db/file/helper.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1316      +/-   ##
==========================================
- Coverage   80.88%   80.69%   -0.20%     
==========================================
  Files          65       65              
  Lines        4546     4568      +22     
  Branches      777      776       -1     
==========================================
+ Hits         3677     3686       +9     
- Misses        854      867      +13     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kriswest kriswest marked this pull request as ready for review December 9, 2025 17:34
@kriswest
Copy link
Contributor Author

kriswest commented Dec 9, 2025

@jescalada I ended up fixing a bunch of tests in this PR that seemed to be missing the Commit type (in favour of one of the CommitData types).

Copy link
Contributor

@coopernetes coopernetes left a comment

Choose a reason for hiding this comment

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

A few minor suggested changes. The bulk of these changes LGTM.

@kriswest kriswest changed the title fix: defer reading of database configuration until need to fix race fix: defer reading of database configuration until needed to fix race Dec 10, 2025
Removes a duplicated type export and an unnecessary start() call

Co-authored-by: Thomas Cooper <57812123+coopernetes@users.noreply.github.com>
Signed-off-by: Kris West <kristopher.west@natwest.com>
Co-authored-by: Thomas Cooper <57812123+coopernetes@users.noreply.github.com>
Signed-off-by: Kris West <kristopher.west@natwest.com>
Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

I did some testing with the skipped tests just in case, but I'm still getting some mysterious errors with the testPush.test.ts file:

Details
 FAIL  test/testPush.test.ts > Push API > test push API > should allow an authorizer to approve a push
AssertionError: expected 401 to be 200 // Object.is equality

- Expected
+ Received

- 200
+ 401

 ❯ test/testPush.test.ts:158:26
    156|           },
    157|         });
    158|       expect(res.status).toBe(200);
       |                          ^
    159|     });
    160| 

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/3]⎯

 FAIL  test/testPush.test.ts > Push API > should allow an authorizer to reject a push
AssertionError: expected 401 to be 200 // Object.is equality

- Expected
+ Received

- 200
+ 401

 ❯ test/testPush.test.ts:272:24
    270|       .post(`/api/v1/push/${TEST_PUSH.id}/reject`)
    271|       .set('Cookie', `${cookie}`);
    272|     expect(res.status).toBe(200);
       |                        ^
    273|   });
    274| 

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/3]⎯

 FAIL  test/testPush.test.ts > Push API > should allow a committer to cancel a push
AssertionError: expected 401 to be 200 // Object.is equality

- Expected
+ Received

- 200
+ 401

 ❯ test/testPush.test.ts:316:24
    314|       .post(`/api/v1/push/${TEST_PUSH.id}/cancel`)
    315|       .set('Cookie', `${cookie}`);
    316|     expect(res.status).toBe(200);
       |                        ^
    317| 
    318|     const pushes = await request(app).get('/api/v1/push').set('Cookie', `${cookie…

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/3]⎯


 Test Files  1 failed | 45 passed (46)
      Tests  3 failed | 536 passed (539)

Would be great to have some more clues on this!

Co-authored-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Signed-off-by: Kris West <kristopher.west@natwest.com>
@jescalada jescalada merged commit f31592b into finos:main Dec 13, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in config loading prevents connection to mongoDB

3 participants