Skip to content

fix: preserve cold storage during sync and backup#1484

Open
SameDesu123 wants to merge 6 commits into
kwaroran:mainfrom
SameDesu123:fix/issue-1482
Open

fix: preserve cold storage during sync and backup#1484
SameDesu123 wants to merge 6 commits into
kwaroran:mainfrom
SameDesu123:fix/issue-1482

Conversation

@SameDesu123

@SameDesu123 SameDesu123 commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

PR Checklist

  • Required Checks
    • Have you added type definitions?
    • Have you tested your changes?
    • Have you checked that it won't break any existing features?
  • If your PR uses models1, check the following:
    • Have you checked if it works normally in all models?
    • Have you checked if it works normally in all web, local, and node-hosted versions? If it doesn't, have you blocked it in those versions?
  • If your PR is highly AI generated2, check the following:
    • Have you understood what the code does?
    • Have you cleaned up any unnecessary or redundant code?
    • Is it not a huge change?
      • We currently do not accept highly AI generated PRs that are large changes.

Summary

Fixes a cold storage sync/backup gap that can leave the database with cold storage pointers but no corresponding cold storage payloads.

This prevents bootstrap and character-switch paths from throwing when an existing cold storage item is missing, and ensures future account sync and backup flows carry the cold storage payloads alongside the database pointers.

Related Issues

Fixes #1482 and #1425

Changes

  • Guard cold storage reads before dereferencing character, so missing payloads do not crash bootstrap or character selection.
  • Add shared cold storage helpers for collecting referenced cold storage keys and recognizing backup payload names.
  • During account sync migration, copy referenced cold storage items into the account cold storage endpoint before writing the migrated database.
  • Include cold storage payloads in Google Drive backup uploads and restore them before applying a loaded Drive database.
  • Include cold storage payloads in local and partial local backups even when the current storage backend is account sync, and restore them through the normal cold storage writer.
  • Add focused tests for cold storage key collection and backup payload recognition.

Impact

Users with already-missing cold storage payloads should no longer hit a Cannot read properties of null (reading 'character') crash during bootstrap. Those missing payloads are still unrecoverable unless a valid backup exists, but the app can continue loading and future sync/backup operations should no longer create pointer-only cold storage states.

Validation performed:

  • pnpm vitest run src/ts/process/coldstorageData.test.ts
  • pnpm check
  • git diff --check

Additional Notes

Existing corrupted account data may still require restoring from a backup that contains the missing cold storage payloads.

Footnotes

  1. Modifies the behavior of prompting, requesting, or handling responses from AI models.

  2. Over 80% of the code is AI generated.

@SameDesu123 SameDesu123 marked this pull request as ready for review June 13, 2026 10:21

@cubicj cubicj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The cold storage preservation direction looks right, but I think this also makes the existing pointer-only data case more important to handle carefully.

One path I am worried about is when the database already references cold storage keys but the payload only exists locally, or no longer exists at all. backupDrive() currently only logs and skips missing cold storage payloads before uploading the database backup, restoreColdStorageFromDrive() also continues when a referenced payload is missing from Drive, and local/Drive restore calls setColdStorageItem() without checking whether the write actually succeeded. In those cases, the app can still end up with a restored or newly uploaded database that contains cold storage pointers without the matching payloads.

Could you take another look at whether these partial backup/restore cases should be surfaced to the user before the database is applied or uploaded?

@SameDesu123

Copy link
Copy Markdown
Contributor Author

The cold storage preservation direction looks right, but I think this also makes the existing pointer-only data case more important to handle carefully.

One path I am worried about is when the database already references cold storage keys but the payload only exists locally, or no longer exists at all. backupDrive() currently only logs and skips missing cold storage payloads before uploading the database backup, restoreColdStorageFromDrive() also continues when a referenced payload is missing from Drive, and local/Drive restore calls setColdStorageItem() without checking whether the write actually succeeded. In those cases, the app can still end up with a restored or newly uploaded database that contains cold storage pointers without the matching payloads.

Could you take another look at whether these partial backup/restore cases should be surfaced to the user before the database is applied or uploaded?

That makes sense. Since cold storage payloads are required by the database references, we should not treat missing cold storage data the same way as missing image assets.

I’ll update this PR so backup/restore/account migration verifies required cold storage payloads before uploading or applying the database. If any referenced payload is missing, invalid, or fails to write, the flow should fail or surface a clear error instead of completing with a database that contains dangling cold storage pointers.

I’ll keep recovery of already-missing historical payloads out of scope, but this PR should prevent creating new pointer-only backup/sync states.

@SameDesu123 SameDesu123 requested a review from cubicj June 21, 2026 06:14

@cubicj cubicj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the follow-up. The stricter cold storage checks cover the local and Drive restore paths much better, but I still see two ordering cases that can leave partial state behind.

In account sync migration, the normal storage keys are written to AccountStorage before collectColdStorageBackupPayloads() can fail. If a referenced cold payload is missing or cannot be uploaded, the migration returns false, but the remote account can already have database/assets from the failed attempt.

For Drive sync, risu_lastsaved is updated before restoreColdStorageFromDrive() succeeds. If cold storage restore fails, the DB is not applied, but that remote sync can be skipped on the next run because it has already been marked as seen.

Could you check those ordering cases?

@SameDesu123

Copy link
Copy Markdown
Contributor Author

Thanks, good catch. I pushed b78c30b to cover both ordering cases.

For account sync migration, cold storage payloads are now collected and uploaded before the normal account storage keys are copied. I also skip copying database/database.bin in the early key-copy step, so if a referenced cold storage payload is missing or cannot be uploaded, the migration returns before writing the migrated database or other normal account data.

For Drive sync/restore, risu_lastsaved is now updated only after restoreColdStorageFromDrive() succeeds and the database has actually been written locally. So a failed cold storage restore should no longer cause that remote sync version to be marked as already seen.

This still does not try to recover historical pointer-only data, but it should prevent these backup/sync paths from creating or hiding new partial cold storage states.

@SameDesu123

Copy link
Copy Markdown
Contributor Author

And fix merge conflict from main

@cubicj cubicj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry for another follow-up, but I noticed one account migration case that still looks risky.

Character cold storage stores the full character object, but those payloads are uploaded before the replacement map from AccountStorage.setItem() exists. That can leave asset refs inside the cold payload on old local keys while the main DB is rewritten to account-renamed keys.

Could you check whether cold storage payloads should be rewritten with the same replacement map before upload? Also, since the restore path now validates cold storage payload shape, could collectColdStorageBackupPayloads() reject invalid payloads too before backup/account migration accepts them?

@SameDesu123

SameDesu123 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up. I pushed it to cover this account migration case.

The migration now builds the account replacement map before uploading cold storage payloads, then rewrites character cold storage payload resource refs with that same map before sending them to account cold storage. The migrated database is still written last, so a cold storage upload failure should not leave an account database pointing at unavailable cold payload data.

I also tightened collectColdStorageBackupPayloads() so backup and account migration reject invalid cold storage payload shapes up front, matching the restore-side validation instead of accepting any truthy payload.

@SameDesu123 SameDesu123 requested a review from cubicj June 28, 2026 11:04

@cubicj cubicj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. The remaining account migration concern looks covered now: the replacement map is built before cold payload upload, and character cold payload refs are rewritten with the same map before the account DB is written.

I also rechecked the local/Drive backup and restore paths, and missing or invalid cold storage data now fails before a partial database is applied or uploaded. pnpm check, pnpm test, and pnpm build pass locally.

Approving — thanks for the careful follow-ups!

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.

TypeError: Cannot read properties of null (reading 'character') on bootstrap after Google account sync

2 participants