Query with syntax error in history causes failed migration to snapshots

[philrz]

Repro is via update from Zui Insiders 1.18.1-insiders.3 to 1.18.1-insiders.12.

This issue was reported by a community zync user. In their own words:

For some reason the latest Zui Insiders has seemed to wipe out all my 20+ saved queries. Do you think there is any way I could recover those?

Here's the baseline steps for a workflow that avoids the problem as shown in the first video below:

1. Start from running a clean install of Zui Insiders 1.18.1-insiders.3
2. Load some sample data and run a simple query (like a search for a keyword, e.g., google).
3. Save the query to some name (e.g. Google)
4. Quit the app and update to 1.18.1-insiders.12
5. When the app relaunches, the Saved Query will still be present

Success.mp4

To trigger the failure, as shown in the video below, right after step 3 in the list above, type a query with bad syntax that causes a parse error. Now after updating to 1.18.1-insiders.12, when the app is relaunched, the Saved Query is gone.

Failure.mp4

[philrz]

Looking in the main.log, we see the root cause seems to be a failure in the migration to snapshots. The error messages shown below appear, which are within the attached appsState.json that I saved at the spot during my "Failure" repro after I'd exited 1.18.1-insiders.3 before updating to 1.18.1-insiders.12.

[2024-11-07 11:28:02.692] [info]  TypeError: Cannot read properties of undefined (reading 'ts')
    at migrateToSnapshots (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112449:34)
    at _Migrations.run (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112524:33)
    at _Migrations.runPending (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112520:17)
    at migrate (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112607:34)
    at /Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112576:64
    at async Object.load (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112576:16)
    at async _MainObject.boot (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:114432:37)
    at async boot (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:114642:17)
    at async main (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:114665:19)
[2024-11-07 11:28:02.699] [error] unable to migrate
[2024-11-07 11:28:02.700] [error] TypeError: Cannot read properties of undefined (reading 'ts')
    at migrateToSnapshots (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112449:34)
    at _Migrations.run (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112524:33)
    at _Migrations.runPending (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112520:17)
    at migrate (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112607:34)
    at /Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112576:64
    at async Object.load (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:112576:16)
    at async _MainObject.boot (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:114432:37)
    at async boot (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:114642:17)
    at async main (/Applications/Zui - Insiders.app/Contents/Resources/app.asar/dist/main.js:114665:19)

@jameskerr traced the line numbers referenced in the stack trace above to line 84 in this code snippet from the migration to snapshots that were added in #3135:

zui/apps/zui/src/js/state/migrations/202409271055_migrateToSnapshots.ts

Lines 78 to 84 in b61d012

	if (!snapshot) {
	/* The Snapshot is missing because it has invalid syntax */
	/* But it is stored in the queryVersions */
	const version = state.queryVersions[queryId].entities[versionId]
	const isNamed = !!queries[queryId]
	snapshot = {
	createdAt: version.ts,

The comment in there about "invalid syntax" is what tipped me off to the repro recipe. A heavy Zui user is almost certainly going to make syntax errors at some point, so if having those left behind in the query history is enough to trigger this bug, it seems very likely that's what happened for this user.

Ultimately it seems we have at least two things to fix here:

1. The migration itself needs to be fixed so entries like these can either be salvaged or skipped over rather than failing the migration.
2. We need to make migrations more robust/defensive so that in situations like this the prior app state is somehow backed up or left unchanged so that way the state (such as saved queries) could be salvaged in a worst case scenario like this where a migration still fails for any reason.

[philrz]

I've also got a shortcut repro in Dev mode at Zui commit b61d012 that uses the attached dev-appState.json that's similar to the one I saved off in the prior comment before updating Zui Insiders. To repro:

1. yarn start Zui commit at b61d012 with no prior app state, then immediately quit
2. Copy the attached dev-appState.json to apps/zui/run/appState.json
3. yarn start Zui again

The failed migration error messages will be visible on the console.

[philrz]

After discussing with @jameskerr, we wanted to confirm if the problem would also surface if, instead of a query that has a syntax error when issued, we instead issue a valid query but then don't save it. As shown in the attached video, it looks like this does not cause the problem, as in this case when I relaunch into 1.18.1-insiders.12 the entry is still present in the Queries tab.

ModifiedQuery.mp4

[philrz]

Verified in Zui Insiders release 1.18.1-insiders.14.

In the first attached video I first verify the specific migration bug has been addressed. I start out by getting into the repro state in 1.18.1-insiders.3 where I've saved a good query called "Google", and then entered a query that triggers a parse error, then quit the app. After I allow the app to upgrade to 1.18.1-insiders.14 and relaunch into this version, now the migration completes successfully, as confirmed by the presence of my saved query.

Verify.mp4

This also happens to show a change in behavior resulting from the changes in #3135. Previously, queries with syntax errors did not show up in the History tab, but now they do. I confirmed with @jameskerr that this an expected change. If users complain about this in the future, we could expose a way to delete entries or a way to filter them out.

In the second attached video I verify the changes in #3166 that will help us avoid a repeat of the kind of data loss that occurred for the user that experienced the failed migration. I start out having already installed 1.18.1-insiders.14 but have replaced the appState.json with one from the repro state shown above, but I've additionally hacked the keys of all the entity objects in the file with garbage values like xyz and abc. When the app launches and the migration starts the first thing that happens is a file with the "before" state of the appState.json is copied into the backups/ folder. Now when the migration fails the user is shown a pop-up message. The appState.json itself is also left untouched, and even in repeated attempts to launch the app, the cycle repeats with yet another backup copy created and the pop-up shown yet again.

Verify-Failed-Migration.mp4

This effectively means the user would be stuck until they:

1. Manually blow away all their saved data on their own (e.g., if they were to delete the Zui Insiders User Data folder). An empty appState.json would be created on relaunch.
2. Contact us on community Slack so we can help understand the nature of the failure and help them patch their appState.json so they can migrate successfully without losing their saved data. This would be the ideal.
3. Figure out on their own how to surgically fix the appState.json. This seems unlikely, but, who knows.

Thanks @jameskerr!