Sync fails with IndexedDB due to weird Transaction semantics

[Slug-Man]

When calling replica.sync() with IndexedDbStorage in a browser, sync fails with (in Chrome): InvalidStateError: Failed to execute 'objectStore' on 'IDBTransaction': The transaction has finished.

Root Cause:
The sync code in taskdb/sync.rs opens a single transaction and holds it open while:

1. Reading from storage (IDB operations)
2. Making HTTP calls to the sync server (network I/O)
3. Writing back to storage (IDB operations)

IndexedDB transactions auto-commit when there are no pending IDB operations. During HTTP calls, the browser sees no pending IDB work and closes the transaction. When sync tries to write afterward, it fails.

[djmitche]

Thanks! I did miss that important detail in implementing this functionality, and it's a pretty fundamental flaw in the WASM support.

I suppose we have two options:

• Somehow make the Txn implementation satisfy the requirements of that trait -- perhaps by artificially holding the transaction open with repeated operations, or by layering on some kind of in-database locking like an is_locked table.
• Re-writing the sync algorithm so that it does not require the transaction to be open when contacting the remote server.

The first is impractical: busy-looping with repeated operations would waste a lot of time and energy, and a simple lock doesn't accomplish the not-visible-until-committed requirement.

The second might be possible. There are two remote calls involved in sync: GetChildVersion and AddVersion. In both cases, we are already able to "crash" either before or after the server receives the request without data corruption. This implies that DB changes may occur while the API call proceeds: API call begins, process crashes, another process starts and makes changes. So, the questions is, do we rely on transaction rollback at any point -- that is, is there a point where something has been modified in the DB that must be rolled back if the response from the server is not as expected.

GetChildVersion is called having written nothing, so this is pretty easy. It depends on base_version, and could easily re-validate that value after the API call.

AddVersion takes a version, which we construct from the DB, but nothing is written to the DB before the API call. However, it does depend on the operations in that version not changing during the API call. We change operations by marking them as sync'd -- which would be OK, as it would result in duplicating operations on the server and this is already possible and handled properly.

In summary, it should be possible to re-write the sync process such that it works with shorter transactions that do not span over API calls.

[djmitche]

I can reproduce the issue with tests now.

The batching and storing transformed_server_ops make the fix a bit harder than I expected!

[djmitche]

I spent some time hacking away at this, and I'm not sure it's going to work, although I see @Slug-Man has an interesting solution developed with Claude.

There are a few issues. The big one is concurrent modification of operations while a sync is in progress, such as by task undo or, worse, a concurrent task sync. If we have included an operation in a batch, and that operation is subsequently removed or modified, how do we detect that and how do we handle it? One idea I had was to apply a version immediately after uploading it to the server. If the operations haven't changed, then there's no problem thanks to the magic of OT. If they have changed, then this updates the local state to represent the original operation and the modified operation both having occurred.

The problem here is the task history. The result of applying the same operation locally and from the server is no change -- the local and remote state are already the same so no further operations are required. However, we retain sync'd operations to generate the task history (for task info), and this idea of applying the version locally essentially removes all of those operations, leaving no history. I think we accepted that corner-cases like this might happen in failure scenarios (e.g., if the client crashes after uploading a version but before committing the DB transaction), but if it happens 100% of the time that's pretty bad.

#686 is also an issue here -- if a batch of operations is only a prefix of the un-synced operations, then we need some way to update some operations as sync'd and some as un-sync'd but modified. That's a lot harder to do if that set of operations may have changed while communicating with the server!

I am curious about the sync_id approach in Slug-Man@556b2b5. It looks like as-written that applies sync_id to all un-sync'd operations at the beginning of the run, rather than just the batched items. I think this idea could work, but I wonder what happens when a replica crashes after having labeled a batch of operations with a sync_id. When another replica tries to sync, does it skip those operations? re-sync them? how does it know the other replica has crashed and is not running concurrently?

I'm tempted to abandon IndexedDB and look for another solution. Are there transactional overlays on LocalStorage that we could use, perhaps even an SQLite backend that uses LocalStorage for block storage? Should we look more closely at the locking idea (some kind of is_locked table with a timeout)?

[Slug-Man]

Yeah, I've been trying to get to grips with the sync model and that was an early attempt to see how a fix would work. Didn't mean to ping this repo with that before I'd thought about it some more!

I hadn't considered batched operations at that point. I think there were other issues I wasn't fond of with that approach. Mainly around failure recovery and concurrent syncing on the same storage.

Rather than applying the version immediately after change, I thought a solution could be to keep track of the version and latest op_id and only commit the version and ops at the end once applied. If our saved op_id no longer exists or the version is different, we hit an undo or sync race condition and can abort.

I took a stab at that solution with Claude just to make it more concrete. I'm not used to working on forked repos in GitHub so I managed to open a PR of it: #703. Added bonus that I believe it also solves #686. A word of caution that it's probably better skimmed and thought of as a throwaway illustration rather than a serious pull request. I probably can't defend any of the Rust-specific implementation details. :)

I'm not convinced I handled all of the edge cases and I've also been leaning towards this kind of change not being worth it for IDB support alone. For the locking idea, I think the Web Locks API would work well and removes the need for any special timeout handling on the lock. Concurrency at this level is all new to me so I could be very off!

[djmitche]

No worries regarding pinging the issue -- it was neat to learn you were working on it, and a helpful idea to think about.

The SyncPoint abstraction sounds cool, but it has a similar issue to the sync_id -- we would need to prevent any operations before an outstanding SyncPoint from being modified, but what happens if the sync operation fails and the SyncPoint remains? You're right that this is some kind of race but need to ensure that it's handled cleanly and without data loss.

Web Locks sounds fantastic, and would accomplish the serialization aspect of transactions. The concern with using a lock is that we also need to ensure that any partially-applied modifications aren't visible if the transaction is aborted. However, maybe we could accomplish that by just queueing the modifications up and applying them in one IDB transaction in txn.commit(). That will be complicated (need to ensure that modifications queued in memory are still visible within the transaction) but maybe not impossible.

I won't be able to work on this for the next week or so, so if you're interested feel free to keep hacking away at this. If not, that's fine too -- I'm not your boss :)

[djmitche]

I still haven't had time for this, but the lock + queued modifications thing might be the best bet for solving this.