Skip to content

fix: catch unhandled promise rejections in logging middleware#291

Merged
yash-pouranik merged 2 commits into
geturbackend:mainfrom
Kirtan-pc:fix-api-usage-unhandled-rejection
Jun 11, 2026
Merged

fix: catch unhandled promise rejections in logging middleware#291
yash-pouranik merged 2 commits into
geturbackend:mainfrom
Kirtan-pc:fix-api-usage-unhandled-rejection

Conversation

@Kirtan-pc

@Kirtan-pc Kirtan-pc commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

🚀 Pull Request Description

Fixes the issue where MongoDB/Mongoose logging write failures caused UnhandledPromiseRejection crashes.

  • Added a .catch() error handler to the fire-and-forget Log.create(...) promise call in api_usage.js. This ensures that any asynchronous rejections during request logging (such as MongoDB connection drops or schema validation errors) are captured gracefully and do not crash the Node.js process.
  • Improved the error logging for ApiAnalytics.create(...) inside setImmediate to log err.message || err instead of the broad error object, reducing verbose log dumps.
  • Created unit tests in api_usage.test.js to assert correct behavior during database write successes, failures, and route logging exclusions.

🛠️ Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📝 Documentation update
  • 🎨 UI/UX improvement (Frontend only)
  • ⚙️ Refactor / Chore

🧪 Testing & Validation

Backend Verification:

  • I have run npm test (via workspace test suites) and all tests passed.
  • I have verified the API endpoints using Postman/Thunder Client.
  • New unit tests have been added (if applicable).

Frontend Verification:

  • I have run npm run lint in the frontend/ directory.
  • Verified the UI changes on different screen sizes (Responsive).
  • Checked for any console errors in the browser dev tools.

📸 Screenshots / Recordings (Optional)

N/A

✅ Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings or errors.
  • I have updated the documentation (README/Docs) accordingly.

Built with ❤️ for urBackend.

Summary by CodeRabbit

  • Tests

    • Added comprehensive tests for API usage and analytics handling to ensure tracking runs correctly and failures are safely handled.
  • Bug Fixes

    • Improved error logging for clearer diagnostic messages and hardened middleware so analytics/logging failures do not crash request processing.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 22d6c6c9-97e1-4b02-a426-a9d65b28c511

📥 Commits

Reviewing files that changed from the base of the PR and between d71e23f and 98e53fe.

📒 Files selected for processing (1)
  • apps/public-api/src/__tests__/api_usage.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/public-api/src/tests/api_usage.test.js

📝 Walkthrough

Walkthrough

Adds a Jest test suite for the api_usage middleware that mocks logging, analytics, and Redis helpers; verifies listener registration, successful analytics capture, and error resilience. Also adjusts middleware error-logging to print error messages instead of raw error objects.

Changes

API Usage Middleware Testing

Layer / File(s) Summary
Test infrastructure and fixtures
apps/public-api/src/__tests__/api_usage.test.js
Jest mocks for @urbackend/common (Log.create, ApiAnalytics.create, Redis helpers), middleware import, and shared req/res/next fixtures with captured finish callback and mock reset in beforeEach.
Middleware listener and routing behavior
apps/public-api/src/__tests__/api_usage.test.js
Tests verify logger registers a finish listener and calls next() for analytics routes, and skips registering the listener for non-analytics routes.
Analytics capture success path
apps/public-api/src/__tests__/api_usage.test.js
Test triggers the captured finish callback and asserts Log.create, Redis incrWithTtlAtomic, and ApiAnalytics.create calls with expected metadata and that console.log contains the request line.
Error resilience and logging improvements
apps/public-api/src/__tests__/api_usage.test.js, apps/public-api/src/middlewares/api_usage.js
Tests confirm the middleware gracefully handles rejections from Log.create and ApiAnalytics.create without crashing. Middleware logging was changed to use e.message and `err.message

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

level:intermediate

Suggested reviewers

  • yash-pouranik
  • Nitin-kumar-yadav1307

Poem

🐰 I hopped through mocks and finish calls today,
Logging each route and timing the relay,
When saves fail, I chuckle — errors I display,
Messages now clear, no raw objects in the fray,
Tests pass, analytics counted, hip-hip-hooray!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: catch unhandled promise rejections in logging middleware' is directly related to the main change—adding .catch() handlers to prevent unhandled promise rejections in the api_usage middleware, and includes improved error logging.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
apps/public-api/src/__tests__/api_usage.test.js (1)

81-81: ⚡ Quick win

Consider verifying the Redis key pattern and TTL arguments.

The test asserts that incrWithTtlAtomic is called but doesn't verify the key pattern or TTL. Adding argument verification would catch potential regressions in the Redis key construction or TTL handling.

Enhanced assertion
-        expect(mockIncrWithTtlAtomic).toHaveBeenCalled();
+        expect(mockIncrWithTtlAtomic).toHaveBeenCalledWith(
+            expect.anything(), // redis client
+            'project:usage:req:count:test_project_id:2026-06-08',
+            86400, // DEFAULT_DAILY_TTL_SECONDS
+            undefined // amount defaults to 1
+        );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/public-api/src/__tests__/api_usage.test.js` at line 81, The test
currently only checks that mockIncrWithTtlAtomic was called; update the
assertion to verify the Redis key pattern and TTL arguments for
incrWithTtlAtomic (e.g., assert mockIncrWithTtlAtomic was called with a key
string matching the expected pattern using expect.stringMatching(/.../) and the
expected TTL value), referencing the mockIncrWithTtlAtomic call in the test so
it fails if the key format or TTL changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@apps/public-api/src/__tests__/api_usage.test.js`:
- Line 81: The test currently only checks that mockIncrWithTtlAtomic was called;
update the assertion to verify the Redis key pattern and TTL arguments for
incrWithTtlAtomic (e.g., assert mockIncrWithTtlAtomic was called with a key
string matching the expected pattern using expect.stringMatching(/.../) and the
expected TTL value), referencing the mockIncrWithTtlAtomic call in the test so
it fails if the key format or TTL changes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2f3e98b5-6ef1-4dc0-b3a7-b986070c2225

📥 Commits

Reviewing files that changed from the base of the PR and between 23c590b and d71e23f.

📒 Files selected for processing (2)
  • apps/public-api/src/__tests__/api_usage.test.js
  • apps/public-api/src/middlewares/api_usage.js

@yash-pouranik

Copy link
Copy Markdown
Collaborator

Please mention issue, what it solves.
@Kirtan-pc

@Kirtan-pc

Copy link
Copy Markdown
Contributor Author

Hey @yash-pouranik ,
It solves issue #283.
Thank you

@yash-pouranik

Copy link
Copy Markdown
Collaborator

ok to merge?
@coderabbitai

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details and status
gh pr view 291 --repo geturbackend/urBackend --json title,state,reviews,statusCheckRollup,mergeable,mergeStateStatus,body

echo "---REVIEW_COMMENTS---"
gh pr reviews 291 --repo geturbackend/urBackend

echo "---COMMENTS---"
gh pr view 291 --repo geturbackend/urBackend --json comments --jq '.comments[] | {author: .author.login, body: .body}'

echo "---CHECKS---"
gh pr checks 291 --repo geturbackend/urBackend

echo "---DIFF---"
gh pr diff 291 --repo geturbackend/urBackend

Length of output: 30906



@yash-pouranik: Here's a quick summary of the PR's status:

✅ All CI checks pass — frontend-lint, public-api-tests, dashboard-api-tests, sdk-tests, python-sdk-tests, and CodeQL all green.

✅ Merge state is CLEAN and the branch has no conflicts.

✅ The fix is correct and minimal:

  • Adds a .catch() handler to the fire-and-forget Log.create(...) call, preventing unhandled promise rejection crashes.
  • Improves error log readability (err.message || err instead of the raw object).
  • 5 new unit tests covering success path, DB failure for Log.create, DB failure for ApiAnalytics.create, and route exclusions.

⚠️ One open nitpick (non-blocking): My review flagged that the incrWithTtlAtomic assertion in the test only checks that the function was called, not the Redis key pattern or TTL value. This is a nice-to-have improvement, not a blocker.

Verdict: The PR is good to merge. The core bug fix is solid and well-tested. The nitpick can be addressed in a follow-up if desired.

@yash-pouranik

Copy link
Copy Markdown
Collaborator

fix these 2 things.

@yash-pouranik

Copy link
Copy Markdown
Collaborator

@Kirtan-pc

@Kirtan-pc

Copy link
Copy Markdown
Contributor Author

Hey @yash-pouranik,
I have fixed the 2 things you have suggested , please review it once.
Thank you

@yash-pouranik yash-pouranik merged commit 2d42729 into geturbackend:main Jun 11, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants