Repository navigation
fix(api): answer bulk load only after saving - #662
Merged
Merged
Conversation
Bulk load answered 200 before the entries were written, so the crawler never saw a database failure and its run looked successful. The handler now waits for the save, and a failure goes to the existing catch: it is logged, emailed when recipients are set, and answered with 500, which makes the crawler exit 1.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation correctly propagates persistence failures and the focused tests cover the regression.
0 open findings
What changed in this PR
Ensures bulk-load requests report success only after database persistence completes.
Changes:
- Awaits bulk insert/update operations and routes failures through existing error handling.
- Removes obsolete fire-and-forget error-email helper.
- Adds tests for save failures and delayed saves.
| File | Description |
|---|---|
api/src/services/pub-code-service.js |
Awaits persistence before responding. |
api/test/bulk-load.test.js |
Verifies failure and response-order behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
POST /api/pub-code/bulk-loadstarted the database writes and sent 200 straight away. If saving failed, the crawler still saw success, so the scheduled crawl went green even when nothing was stored.Changes
bulkLoadnow waits forinsertOrUpdateand answers 200 only after the entries are saved.A save failure goes to the handler's existing catch: it's logged, emailed when
EMAIL_RECIPIENTSis set, and answered with500 { message: "Internal server error" }. The crawler already exits 1 on an error response, so a failed load now fails the scheduled run.The
sendEmailForErrorhelper was only used by the old fire-and-forget path, so it's removed.New tests in
api/test/bulk-load.test.js:Both fail on the old code.
Tests and CI
The new tests need no database. They run in CI's
API Testsjob (added in #658, now on main). All 8 API tests pass there, including #658's MongoDB integration tests, which poll for the save and so work with or without this change.Because the response now waits for the save, a very large payload could take longer to answer. Sequential saves of a few hundred entries should finish well within the route timeout.
Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in: