Skip to content

Conversation

@mgoldenberg
Copy link

The details below are mostly copied directly from Alorel#72, which is a pull request attempting to upstream these changes.

Background

Recently, I created an issue in this repository outlining a change that prevented certain kinds of database upgrades (for details, see Alorel#66).

To summarize, the old upgrade callback was able to access the active version change transaction and, for instance, add an index to an existing object store. In more recent versions of this library, this is no longer possible as the values provided to the callback cannot be used to get access to the active transaction.

Changes

In order to support the behavior described above, I have made a number of changes - many of which are breaking changes.

The overarching change is that the active version change transaction is explicitly provided to the callback and the database is not provided to the callback. This seemed like a reasonable change, as Transaction::db provides a reference to the underlying Database, so existing codebases could relatively easily make their callbacks compatible - with minor exceptions detailed below.

Details on the changes are below.

Synchronous Upgrade Needed Callback

The signature of the callback provided to OpenDbRequestBuilder::with_on_upgrade_needed has changed to accept a reference to the active version change Transaction rather than the Database.

// original callback constraint
F: FnOnce(VersionChangeEvent, Database) -> crate::Result<()>

// modified callback constraint
F: FnOnce(VersionChangeEvent, &Transaction<'_>) -> crate::Result<()>

Note that the function that invokes the callback - OpenDbListener::listener - constructs the Transaction from the VersionChangeEvent with a newly added function, Transaction::from_raw_version_change_event.

Configuring Transaction behavior on Drop

Given that the callback above is synchronous, it is unable to commit the Transaction as Transaction::commit is asynchronous. Furthermore, the function which invokes the callback - OpenDbListener::listener - is also synchronous and has the same shortcoming. The consequence is that when the Transaction is Dropped at the end of OpenDbListener::listener, it is automatically aborted, and any changes made to the Database in the callback are lost.

In order to mitigate this issue, I have updated Transaction so that the Drop behavior is configurable. That is, one can set a field in the Transaction so that it is either committed or aborted when it is Dropped. By default, it is aborted, making this change backward compatible.

Asynchronous Upgrade Needed Callback

The signature of the callback provided to OpenDbRequestBuilder::with_on_upgrade_needed_fut has also changed to accept a reference to the active version change Transaction rather than the Database. And additionally, it uses the recently stabilized AsynFnOnce trait.

// original callback constraint
F: FnOnce(VersionChangeEvent, Database) -> Fut,
Fut: Future<Output = crate::Result<()>>

// modified callback constraint
F: AsyncFnOnce(VersionChangeEvent, &Transaction<'_>) -> crate::Result<()>

Note that using AsyncFnOnce requires updating the minimum supported Rust version to 1.85.0. I'm not entirely sure that using AsyncFnOnce is absolutely necessary, but I believe that without it, I was bumping up against the lifetime-related limitations detailed in rust-lang/rust#70263.

Caveat

The original callbacks provided to OpenDbRequestBuilder::with_on_upgrade_needed and OpenDbRequestBuilder::with_on_upgrade_needed_fut had access to an owned Database, while the updated callbacks only have access to a reference. This precludes calling the functions below, which consume the Database.

However, both of these functions seemed to me like operations that perhaps shouldn't occur in a version upgrade. And in the case of deleting a database, this seems to be in alignment with the official IndexedDB documentation, where deleting a database is an operation exposed through the IDBFactory and not the IDBDatabase.


UPDATE (2025/09/07): In order to get everything to pass the C.I. tests, I had to update the toolchain used to generate documentation, update the examples, and make some minimal changes to the code in order to accommodate new lint rules. Also, I force pushed some new commit messages so that they conform to the semantic pull request guidelines outlined in .github/semantic.yml.

…ded callback

BREAKING CHANGE: The callback provided to
`OpenDbRequestBuilder::with_on_upgrade_needed` must now accept a
reference to a `Transaction` rather than a `Database`. For most cases,
existing code can be made compatible by calling `Transaction::database`
in order to get access to the underlying database. The caveat is that
the `Transaction::database` provides a reference, rather than ownership
of the `Database`.
BREAKING CHANGE: `AsyncFnOnce` was stabilized in version 1.85.0 of Rust,
so the MSRV had to be bumped to accommodate this. Additionally,
synchronous closures that use `async move` blocks are not drop-in
replacements for `async` closures, so users of
`OpenDbRequestBuilder::with_on_upgrade_needed_fut` will have to make minor
changes to their invocations.
…de needed callback

BREAKING CHANGE: The callback provided to
`OpenDbRequestBuilder::with_on_upgrade_needed_fut` must now accept a
reference to a `Transaction` rather than a `Database`. For most cases,
existing code can be made compatible by calling `Transaction::database`
in order to get access to the underlying database. The caveat is that
the `Transaction::database` provides a reference, rather than ownership
of the `Database`.
For some reason, calling `TransactionSys::do_commit` in the `Drop`
implementation of `Transaction` causes tests in a headless Chrome
browser to hang, even though they pass in a non-headless context.
Ultimately, the default behavior on the JavaScript end is for the
transaction to be committed, so it is safe to exclude this action.
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. I believe all this makes a lot of sense. Thanks! And thank for the clear commit, it's a pleasure to review.

I've left a bit of feedback. Can you address them so that I can merge this PR please?

@mgoldenberg mgoldenberg force-pushed the expose-version-change-transaction branch from e48ec89 to 6ec78c3 Compare September 30, 2025 14:22
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@Hywan Hywan merged commit 201caac into matrix-org:master Oct 1, 2025
Hywan pushed a commit to matrix-org/matrix-rust-sdk that referenced this pull request Oct 3, 2025
**NOTE:** _this should not be merged until matrix-org/rust-indexed-db#1
is merged! The `[patch]` in this branch should point to the official
`matrix-org` fork of `rust-indexed_db`, but is currently pointed at my
personal fork._

## Background

This pull request makes updates
[`indexed_db_futures`](https://docs.rs/indexed_db_futures/latest/indexed_db_futures/index.html)
in the `matrix-sdk-indexeddb` crate. The reason we'd like to update this
dependency is because the version currently used does not fully support
the Chrome browser (see #5420).

The latest version of `indexed_db_futures` has significant changes. Many
of these changes can be integrated without issue. There is, however, a
single change which is incompatible with the `matrix-sdk-indexeddb`
crate. Namely, one cannot access the active transaction in the callback
to update the database (for details, see Alorel/rust-indexed-db#66).

### An Updated Proposal

Originally, new migrations were implemented in order to work around this
issue (see #5467). However, the proposal was ultimately rejected (see
@andybalaam's
[comment](#5467 (comment))).

For this reason, the dependency has instead been `[patch]`ed in the
top-level `Cargo.toml` with a modified version of `indexed_db_futures`
(see matrix-org/rust-indexed-db#1). Furthermore, these changes have been
proposed to the maintainer and are awaiting feedback (see
Alorel/rust-indexed-db#72).

### Why do we need the active transaction in our migrations?

The `crypto_store` module provides access to the active transaction to
its migrations (see
[here](https://github.com/matrix-org/matrix-rust-sdk/blob/ca89700dfe9f29dcd823bb10861807f9d75e0634/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs#L211)).
Furthermore, there is a single migration (`v11_to_v12`) in the
`crypto_store` module which actually makes use of the active transaction
(see
[here](https://github.com/matrix-org/matrix-rust-sdk/blob/ca89700dfe9f29dcd823bb10861807f9d75e0634/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/v11_to_v12.rs#L23)).

For clarity, the reason `v11_to_v12` is problematic in the latest
versions of `indexed_db_futures` is because it is simply adding an index
to an object store which was created in a different migration and this
requires access to the active transaction. All the other migrations
create object stores and indices in the same migration, which does not
suffer from the same issue.

## Changes

- Move `indexed_db_futures` to the workspace `Cargo.toml` and add a
`[patch]` so that it points to a modified version.
- Add `GenericError` type and conversions in order to more easily map
`indexed_db_futures` errors into `matrix-sdk-*` errors.
- Update all IndexedDB interactions so that they use the upgraded
interface provided by `indexed_db_futures`
- Add functionality for running `wasm-pack` tests against Chrome


---
Closes #5420.

---

- [ ] Public API changes documented in changelogs (optional)


Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>

---------

Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants