automerge / automerge-classic

A JSON-like data structure (a CRDT) that can be modified concurrently by different users, and merged again automatically.
http://automerge.org/
MIT License
14.76k stars 467 forks source link

1.0.1-preview.5 regression: RangeError: Zero-length runs are not allowed #441

Closed cgauld closed 2 years ago

cgauld commented 2 years ago

Hi!

With 1.0.1-preview.5 I'm seeing the following error after applying a relatively large number of changes to the backend:

RangeError: Zero-length runs are not allowed
    at BooleanDecoder.skipValues (encoding.js:1193)
    at seekWithinBlock (new.js:96)
    at seekToOp (new.js:282)
    at applyOps (new.js:1286)
    at applyChanges (new.js:1546)
    at BackendDoc.applyChanges (new.js:1773)
    at Object.applyLocalChange (backend.js:84)

For me it's happening consistently on the 923rd change applied. The changes are each adding small similarly-sized objects into a map in the doc.

The issue doesn't appear to affect 1.0.1-preview.4 - indeed I've narrowed it down and it looks like the issue is introduced with this commit: 325f9e8f7d4a16ed7a168494161cecf47185aea9 but from the commit message and contents I'm not sure that information necessarily helps 😬.

I've also had a go to see if I can find out what's going on but I'm not familiar with the codebase and haven't have any luck I'm afraid.

It's definitely possible I'm doing something wrong somewhere - but from stepping through my code and looking at the data I'm passing in I think it seems sensible.

Hopefully this is helpful - let me know if there's anything else I can do!

pvh commented 2 years ago

Thanks for the bug report! That was the commit where the new backend became the default. You could set const USE_NEW_BACKEND = true but my hunch is that this regression probably exists in all versions of the backend.

If you're seeing this on all 923rd changes, would you be able to submit a simple script that demonstrates it without other dependencies, or even better, a failing test?

cgauld commented 2 years ago

You're right - setting that on previous commits also triggers the issue. I've been trying to build a minimal test with dummy data for it but not having much luck. I have found that by tweaking which changes I supply to the backend I can influence when the issue appears (i.e. I don't think it's actually tied to a specific number of changes, but instead perhaps it's an edge case somewhere in the binary sync message encoding or decoding?). Will try with my actual data so see if I can narrow it down. If it's in an RLE implementation then I imagine the data itself influences the codepath quite a lot.

cgauld commented 2 years ago

Ok I've managed to produce a minimal-ish test case. It involves an 84kb JSON file so I haven't put a unit test into a fork and pull requested - but happy to do that down the line if it helps.

I've attached the data file here, and a .js file that exhibits the issue. I've sanitized the data in the file (every character replaced with 'a') but my original data has other values. The data structure is a little bit eccentric but the data is well formed and I can't see anything wrong with it or how I'm using the API.

Would be great to get your thoughts! issue-441.zip

Example output:

~/automerge/backend/encoding.js:1193
        if (this.count === 0) throw new RangeError('Zero-length runs are not allowed')
                              ^

RangeError: Zero-length runs are not allowed
    at BooleanDecoder.skipValues (~/automerge/backend/encoding.js:1193:37)
    at seekWithinBlock (~/automerge/backend/new.js:97:11)
    at seekToOp (~/automerge/backend/new.js:283:48)
    at applyOps (~/automerge/backend/new.js:1242:49)
    at applyChanges (~/automerge/backend/new.js:1502:31)
    at BackendDoc.applyChanges (~/automerge/backend/new.js:1729:35)
    at Object.applyLocalChange (~/automerge/backend/backend.js:164:25)
    at Object.<anonymous> (~/automerge/issue.js:26:45)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
pvh commented 2 years ago

This is great, thank you so much for doing the extra work to make this reproducible. We're currently in the throes of final editing for a new paper so it might take a little while for someone to look more closely at this but having a good test case should make it much easier to understand what's going wrong here.

cgauld commented 2 years ago

No worries - we're just prototyping at the moment so can make do with preview.5 for the time being :-) Would it help for me to submit a pull request with this wrapped into text/backend_test.js? Best of luck with the paper!

ept commented 2 years ago

Hi @cgauld, sorry for the delayed reply. Thank you very much for the reproducible example, which made the bug easy to find. I believe I've fixed it in #442 — could you give it a go and let me know if it fixes the problem for you?

cgauld commented 2 years ago

No worries! That looks to have solved the issue for me :-)