fix: inactive IDB tx in SenderTaggingStore#21918
fix: inactive IDB tx in SenderTaggingStore#21918mverzilli wants to merge 2 commits intomerge-train/fairiesfrom
Conversation
Thunkar
left a comment
Thunkar
left a comment
There was a problem hiding this comment.
IDB is cursed, thanks for this
…packages into martin/fix-idb-tx-timeout-v2
|
I broke a thing again 😭😭😭😭 |
benesjan
left a comment
benesjan
left a comment
There was a problem hiding this comment.
Did not approve yet as I don't fully understand the bug.
Once I'll grok it will be happy to approve.
Thanks for cleaning my mess!
| // Read and process within a single transactionAsync so the IDB transaction stays alive across | ||
| // the read → process → staged-write sequence. Before this fix, #getSecretsWithPendingData opened | ||
| // and closed its own transaction, then the writes happened outside any transaction — allowing the | ||
| // browser to auto-commit the IDB transaction between the two phases. |
There was a problem hiding this comment.
I still don't follow why exactly was this an issue when the staged write doesn't write into the db (it just stores the info in memory).
So why do we need to keep the tx alive given that the write is not a db write?
There was a problem hiding this comment.
No, you're right. I scrambled to open this PR because it was late and I was hoping to get it into a nightly if CI was green, but I've been testing more this morning and I can still reproduce the issue
| continue; | ||
| } | ||
| // Read and process within a single transactionAsync so the IDB transaction stays alive across | ||
| // the read → process → staged-write sequence. Before this fix, #getSecretsWithPendingData opened |
There was a problem hiding this comment.
Before this fix, #getSecretsWithPendingData opened...
Ideally comments should not assume knowledge of history of the codebase so would not mention "before this fix" here.
That part of comment most likely should have been just a github comment.
It's not even clear that you did anymore. I've reproduced something that look like the bug with the patch applied earlier today. Now I rolled it back and I can't reproduce. So it's a really annoying one |
3 participants
Fixes a subtle stale IDB tx issue we introduced in #20817.
Unfortunately this is tricky to reproduce in browser env, we do have kv-store tests that run on playwright but nothing similar for PXE's actual stores. For now it's important for this fix to be quickly added, I'll try to play with adding some playwright regression test in the next days.