-
-
Notifications
You must be signed in to change notification settings - Fork 128
A few fixes for search. #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+399
−26
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
856925b
A few fixes for search.
jamwt dcdc87e
Uh swap these labels back thx.
jamwt 476cf49
Lint.
jamwt b5ee488
Start to move toward action-based aggregations that bundle up larger …
jamwt 69df9a7
Merge remote-tracking branch 'origin/main' into fix_search
jamwt f97e1ec
Use index for scanning stats records.
jamwt 09cc26b
Re-add cron job.
jamwt 7417471
Merge branch 'main' into fix_search
jamwt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Events with the same
_creationTimecan be lost due to cursor update logic. The code updates the working cursor to the last event's creation time, but when events share the same creation time timestamp, the next run will skip them because the query uses.gt()(strictly greater than) instead of inclusive comparison.View Details
📝 Patch Details
Analysis
Cursor-based pagination loses events with duplicate _creationTime timestamps
What fails: The
processSkillStatEventsActionfunction inconvex/skillStatEvents.tscan permanently lose unprocessed skill stat events when multiple events share the same_creationTimevalue and batch processing is interrupted by the skill limit.How to reproduce:
_creationTimetimestamp (possible in Convex since events can be created within the same millisecond)MAX_SKILLS_PER_RUNsmall enough that processing stops mid-batch (e.g., 2-3 unique skills)_creationTimeseen.gt()(strictly greater than) to fetch the next batch_creationTimeequal to the saved cursor are permanently skippedResult: Events that were never processed are lost, resulting in permanently missing stat updates for those events.
Expected: All events should be processed exactly once, even when they share the same
_creationTimetimestamp.Root cause: The
getUnprocessedEventBatchquery at line 307 uses.gt('_creationTime', cursor)which performs a strictly greater-than comparison. When events are batched and the batch boundary falls in the middle of a group of events with the same_creationTime, subsequent queries will skip those boundary events.Fix applied: Changed the query operator from
.gt()to.gte()(greater than or equal) in thegetUnprocessedEventBatchquery function. This ensures events at the cursor timestamp are included in subsequent batches. The aggregation logic is idempotent (processing the same skill's events twice produces the same result), so minor reprocessing of boundary events is safe.See: Convex index documentation on comparison operators -
.gte()performs greater-than-or-equal comparison as opposed to.gt()'s strictly greater-than behavior.