Fix/82 concurrent review lock#166
Open
salman-khan03 wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an integration-level reproduction for issue #82 (concurrent process_review calls for the same profile can overlap) and includes small type-hinting/formatting adjustments in review_service. Despite the PR title, the actual per-profile serialization/locking behavior is not implemented in the service yet.
Changes:
- Added a new integration test module that measures overlap of ingestion windows under concurrent
process_review()calls. - Added a module journal entry describing issue #82, reproduction details, and a proposed fix plan.
- Updated
core/services/review_service.pywithAsyncSessiontype annotations and minor refactors, including atype: ignoreforreview.sections.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tests/integration/test_review_concurrency.py |
Adds a concurrency reproduction test (currently asserting pre-fix behavior) and a parallelism guardrail test for different profiles. |
JOURNAL.md |
Documents issue #82 context, reproduction strategy, and intended locking approach. |
core/services/review_service.py |
Adds AsyncSession annotations and minor adjustments; no actual per-profile lock is introduced. |
Comments suppressed due to low confidence (1)
core/services/review_service.py:92
- The PR title indicates a concurrent-review locking fix, but
process_reviewstill runs ingestion without any per-profile serialization (no lock/advisory lock/row lock). Either implement the per-profile lock around the ingestion+downstream steps here, or adjust the PR scope/title to reflect that this PR only adds a reproduction test.
async def process_review(
db: AsyncSession,
review_id: UUID,
profile_id: UUID,
) -> None:
"""
Background task to process a review.
Steps:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+42
to
+45
| start = asyncio.get_event_loop().time() | ||
| await asyncio.sleep(delay) | ||
| end = asyncio.get_event_loop().time() | ||
| windows.setdefault(str(profile.id), []).append((start, end)) |
Comment on lines
+122
to
+130
| # Today (pre-fix), these two windows overlap because nothing serializes | ||
| # concurrent processing for the same profile. Once a per-profile lock is | ||
| # added, this assertion should be flipped to assert the windows do NOT | ||
| # overlap. | ||
| assert _windows_overlap(*profile_windows), ( | ||
| "Expected the current (unfixed) code to allow overlapping ingestion " | ||
| "windows for the same profile_id -- if this fails, a lock may already " | ||
| "be serializing these calls." | ||
| ) |
Comment on lines
85
to
89
| async def process_review( | ||
| db, | ||
| db: AsyncSession, | ||
| review_id: UUID, | ||
| profile_id: UUID, | ||
| ) -> None: |
Comment on lines
171
to
+172
| review.status = "complete" | ||
| review.sections = [s.model_dump() for s in sections] | ||
| review.sections = [s.model_dump() for s in sections] # type: ignore[assignment] |
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.
Summary
Issue
Closes #
Changes
Testing
make test-unit)make test-integration)make lint)make typecheck)Screenshots / Demo
Notes for Reviewers