fix(sdk): Ensure a gap has been inserted before removing it #4490
Conversation
This patch fixes a bug where the code assumes a gap has been inserted, and thus, is always present. But this isn't the case. If `prev_batch` is `None`, a gap is not inserted, and so we cannot remove it. This patch checks that `prev_batch` is `Some(_)`, which means the invariant is correct, and the code can remove the gap.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4490 +/- ##
==========================================
+ Coverage 85.31% 85.32% +0.01%
==========================================
Files 284 284
Lines 31741 31745 +4
==========================================
+ Hits 27080 27087 +7
+ Misses 4661 4658 -3 ☔ View full report in Codecov by Sentry. |
|
Should fix this then element-hq/element-x-ios#3648? |
To wit: usually those issues are marked as fixed when the app upstreams the SDK, so we don't have the SDK close EX apps issues. |
| let gap = as_variant::as_variant!(c.content(), ChunkContent::Gap)?; | ||
| (gap.prev_token == *prev_token).then_some(c.identifier()) |
There was a problem hiding this comment.
Since we're touching this code, I also made the check more precise: instead of "give me the previous gap, whatever it is", this is now "give me the previous gap with the matching prev-batch token". This would have made the bug much more frequent.
There was a problem hiding this comment.
I would prefer a named function for the code inside find_map because it took me quite a while to figure out what this was doing, but I won't insist on it, and I'm happy for the change to come later if you like the idea.
There was a problem hiding this comment.
Thanks! added a small code comment above this line, expliciting what it does 👍
bnjbvr
requested review from
a team and
andybalaam
and removed request for
a team
| let gap = as_variant::as_variant!(c.content(), ChunkContent::Gap)?; | ||
| (gap.prev_token == *prev_token).then_some(c.identifier()) |
There was a problem hiding this comment.
I would prefer a named function for the code inside find_map because it took me quite a while to figure out what this was doing, but I won't insist on it, and I'm happy for the change to come later if you like the idea.
bnjbvr
force-pushed
the
fix-sdk-event-cache-panic-with-gaps
branch
from
d29bf7c to
e0c9b7a
Compare
This patch fixes a bug where the code assumes a gap has been inserted, and thus, is always present. But this isn't the case. If
prev_batchisNone, a gap is not inserted, and so we cannot remove it. This patch checks thatprev_batchisSome(_), which means the invariant is correct, and the code can remove the gap.