fix: Handle user insert races and add test timeout#1618
Open
edwinjosechittilappilly wants to merge 3 commits into
Open
fix: Handle user insert races and add test timeout#1618edwinjosechittilappilly wants to merge 3 commits into
edwinjosechittilappilly wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
backend
Increase TypeScript integration test timeout to 120s to reduce flakiness during slow CI runs. Enhance user_service.ensure_user_row IntegrityError handling to explicitly handle concurrent-insert races: detect a (oauth_provider, oauth_subject) race and return the existing row, detect an email_lookup_hash race by looking up the email and returning the concurrent identity when it matches, and handle PK collisions by retrying the insert with a new UUID. Add explanatory comments about which collisions are recoverable and when errors should propagate to the caller.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR increases an integration test timeout to 120 seconds and refactors ChangesUser Service Reliability and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
edwinjosechittilappilly
force-pushed
the
fix-integration-tests-ci
branch
from
807c60e to
c3dfd72
Compare
edwinjosechittilappilly
changed the title
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/user_service.py (1)
230-230:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the pipeline failure: use modern union syntax for Optional type.
The Ruff linter (UP045) is failing on this line. While not part of your direct changes, the pipeline must pass for the PR to merge.
Proposed fix
-async def get_effective_agent_config(session: AsyncSession, user_id: str) -> Optional[dict]: +async def get_effective_agent_config(session: AsyncSession, user_id: str) -> dict | None:Also remove the
Optionalimport from line 17 if no longer needed:-from typing import Optional🤖 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 `@src/services/user_service.py` at line 230, The function signature for get_effective_agent_config should use modern union syntax instead of typing.Optional; change the return annotation from Optional[dict] to dict | None and, if Optional is no longer used elsewhere in the file, remove the Optional import (check the import on line with Optional) to fix the UP045 lint error.
🤖 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.
Outside diff comments:
In `@src/services/user_service.py`:
- Line 230: The function signature for get_effective_agent_config should use
modern union syntax instead of typing.Optional; change the return annotation
from Optional[dict] to dict | None and, if Optional is no longer used elsewhere
in the file, remove the Optional import (check the import on line with Optional)
to fix the UP045 lint error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a9b8225-2728-4d7e-89b9-aab7b0acadb4
📒 Files selected for processing (2)
sdks/typescript/tests/integration.test.tssrc/services/user_service.py
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce CI flakiness in TypeScript integration tests and make ensure_user_row more resilient to concurrent user insert races.
Changes:
- Adds explicit
IntegrityErrorhandling comments and recovery paths for OAuth, email hash, and PK collisions. - Adds a 120s timeout override to one TypeScript document ingestion integration test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/services/user_service.py |
Extends concurrent insert recovery logic in ensure_user_row. |
sdks/typescript/tests/integration.test.ts |
Adds a per-test timeout to one ingestion integration test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+120
to
+127
| # Case 2: email_lookup_hash race — same email already inserted (most | ||
| # common for the synthetic anonymous@localhost user in no-auth mode when | ||
| # two concurrent requests both observe an empty table). Look up by email; | ||
| # if the row's (provider, subject) matches ours it's a concurrent insert | ||
| # of the same identity and we can safely return it. | ||
| if user.email: | ||
| by_email = await user_repo.get_by_email(user.email) | ||
| if ( |
| expect(result.status).toBeDefined(); | ||
| expect((result as any).successful_files).toBeGreaterThanOrEqual(0); | ||
| }); | ||
| }, 120_000); |
Replace typing.Optional[dict] with PEP 604 union syntax (dict | None) for get_effective_agent_config and remove the now-unused Optional import. This is a pure type-annotation cleanup (no runtime behavior changes); note it requires Python 3.10+ for the `|` union syntax.
github-actions
Bot
added
bug
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.
Increase TypeScript integration test timeout to 120s to reduce flakiness during slow CI runs.
Enhance user_service.ensure_user_row IntegrityError handling to explicitly handle concurrent-insert races: detect a (oauth_provider, oauth_subject) race and return the existing row, detect an email_lookup_hash race by looking up the email and returning the concurrent identity when it matches, and handle PK collisions by retrying the insert with a new UUID. Add explanatory comments about which collisions are recoverable and when errors should propagate to the caller.
Summary by CodeRabbit