indexeddb: Attempt to fix export crashes by batching DB operations in get_inbound_group_sessions

[andybalaam]

I saw crashes on export (like #2914 ) in my local dev environment, but unlike the reporters of that problem, I did not see an out of memory error. I saw a crash inside the Indexed DB transaction, which only happened when the number of keys became quite large.

This PR keeps a single transaction, but batches the work we do within it into chunks, each using a separate cursor. I don't know why, but this fixes the problem in my local dev environment, and I think it is part of the work we will need to do to batch our export into genuine separate chunks to prevent memory exhaustion. So I think it's worth merging in its current form, even if it's only a partial step towards the right solution.

I spent some time trying to make the code comprehensible but I'm still not very happy with it, so would welcome suggestions.

I am not sure how much logging is appropriate - I added one debug line per batch, but again, suggestions welcome.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (784c745) 83.47% compared to head (b05a0ec) 83.48%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3012      +/-   ##
==========================================
+ Coverage   83.47%   83.48%   +0.01%     
==========================================
  Files         222      222              
  Lines       23210    23210              
==========================================
+ Hits        19374    19378       +4     
+ Misses       3836     3832       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bnjbvr]

Looks fine by me, thanks! Some questions below.

[bnjbvr]

Since this function already returns a Result, can we avoid the expect() and map to an error that we'd raise with ? here?

[andybalaam]

I tried to do that and couldn't find a good way to construct an IndexeddbCryptoStoreError, and also wasn't sure what error would be right. Would the correct thing be to change the error type, maybe adding an enum value to IndexeddbCryptoStoreError?

A separate but related question/wondering: ideally, we would be able to separate these batches into different transactions. That would mean that, in theory, a key might disappear during an export. What would be the correct behaviour if we "lost our place" during export because the key at the end of the last batch had disappeared? I will ask the crypto team whether we think this will ever happen.

[bnjbvr]

Yes, in general we shouldn't be afraid to add more specialized error types. I am for one guilty of taking shortcuts and reusing errors, but it's very fine to add new errors, and it should be painless in general thanks to the error derives we're using.

That would mean that, in theory, a key might disappear during an export.

And the worst implication would be that we lose track of the cursor?

[andybalaam]

That would mean that, in theory, a key might disappear during an export.

And the worst implication would be that we lose track of the cursor?

Yes, which I thought meant that we would fail mid-export but Rich pointed out to me that in indexeddb, the key range is literally a "key is > other key" thing, meaning if a key did disappear, we would still find the keys after it when we asked, so this would not even abort export.

Keys changing under us while we export is not particularly comfortable, but I am quite confident that any key that exists before the export starts and still exists after it ends will be included in the export, so I think that is a good enough invariant.

All of this discussion is actually about a future change, not this one anyway. This one preserves a single transaction for the whole export.

[andybalaam]

OK, so I looked into this more, and I am fairly certain that this will only fail if we use the wrong type as a key. We will never do this because we always pass in a key that was provided to us by cursor.key(). So I propose to update the comment here (7823dd8) and leave it as an expect() since it should never happen.

Is "panicking when things that should never happen happen" in line with our normal practices @bnjbvr ?

[bnjbvr]

Usually the panics I'm fine with are those that are caused by either one of two things:

• the platform isn't behaving as expected causing an error, and reporting that error would be impractical
• an invariant we just checked happens to not be hold

It seems that we're more or less in the first case here? If so, that looks fine to me 👍

[andybalaam]

Yes, the platform would have to either:

a) not accept a &str-derived JsValue as a valid key, or
b) not return a valid key from cursor.key()

for this to panic, and both of those would be unexpected. I think reporting that error would needlessly complicate our error enum, so I propose to leave it as is if you are happy with it.

[bnjbvr]

lgtm, thanks!