Repository navigation
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41b5ff9d2c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
41b5ff9 to
c7d402c
Compare
gdesmott
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7d402cf89
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
c7d402c to
1c3c89b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c3c89b89e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
9799796 to
22b1e8b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22b1e8bdd4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
22b1e8b to
e966b42
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e966b42ff8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11940 +/- ##
==========================================
- Coverage 83.77% 83.76% -0.02%
==========================================
Files 682 683 +1
Lines 88675 88774 +99
Branches 11471 11480 +9
==========================================
+ Hits 74291 74361 +70
- Misses 10740 10777 +37
+ Partials 3644 3636 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e966b42 to
ab2a8ed
Compare
gdesmott
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2a8ed1c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ff76621 to
8216430
Compare
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8216430c11
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
8216430 to
9f61b4d
Compare
gdesmott
changed the title
Revalidation could publish an aggregate that is not a consensus the intersection yielded. A child error mid-consensus can leave the children on different documents, or leave the result on a document that failed the proximity check; an error during the first read can leave children positioned with nothing yielded. And when no child moved, the aggregate's child entries are stale either way: revalidating a child borrows it mutably, which invalidates the entries the aggregate borrowed from it (undefined behavior under Miri when the result is read afterwards). When every child still sits on the document last yielded, rebuild the aggregate's entries for it in place, keeping its metrics; a child may report a move while keeping its position, so this does not depend on whether one did. Otherwise re-seek from the furthest child, as a read would; re-seeking from the furthest moved child alone could seek a child that stayed ahead backwards. A result swapped in through current() that cannot be rebuilt in place aborts. Children may only be pushed before iteration: a child added later sits behind the documents already yielded, which no re-seek can reconcile.
…hing When no child changed, revalidate published the aggregate as it stood, but revalidating each child borrows it mutably, which invalidates the entries the aggregate borrowed from it: reading the result after an Ok revalidation is undefined behavior under Miri. Rebuild the entries on that exit with the same helper resume uses on its own no-change exit, so the two paths offer children in the same order and keep the same quick-mode backer, metrics and doc ID. As in resume, an unbacked rebuild aborts, and a result swapped in through current() that is not a borrowed union aggregate aborts rather than being rebuilt or settled in place.
9f61b4d to
81b161e
Compare
Describe the changes in the pull request
Intersection,UnionFlatandUnionHeapcan publish an aggregate afterrevalidatewhose borrowed child entries were invalidated by the children's own revalidation: reading the result after anOkrevalidation is undefined behavior under Miri. The intersection can also publish a document that is not a consensus it yielded after a child error (children on different documents, a document that failed the slop / in-order check, or an error during the first read), and its re-seek could seek a child that stayed ahead backwards.resumepath already performs, so both paths behave identically. Otherwise the intersection re-seeks from the furthest child, as a read would, and reportsMoved. A result swapped in throughcurrent()that cannot be rebuilt in place aborts. Children may now only be added to an intersection before iteration (debug-asserted; the query optimizer already adds them at plan time).Okrevalidation, and after a child error the intersection continues from the next consensus instead of republishing a stale document or skipping one. None of these cases can currently be triggered from a test:Readhas rebuilt them, except under a union, where the pointers still reach unchanged results. Miri flags this, but the data read is correct.current(), and the query optimizer only adds intersection children before iteration.ON_TIMEOUT RETURN. That can only happen on a real deadline; no debug hook triggers it deterministically.Which additional issues this PR fixes
N/A
Main objects this PR modified
Intersection::revalidate(rqe_iterators): refreshes the aggregate when it is still backed, otherwise re-seeks from the furthest child.UnionFlat::revalidate/UnionHeap::revalidate: rebuild the aggregate's entries when no child changed, sharingresume's rebuild.Mark if applicable
Release Notes
If a release note is required (bug fix / new feature / enhancement), describe the user impact of this PR in the title.