Skip to content

Remove Firestore key migration - #69590

Merged
rosstimothy merged 1 commit into
masterfrom
tross/firestore_cleanup
Aug 12, 2026
Merged

Remove Firestore key migration#69590
rosstimothy merged 1 commit into
masterfrom
tross/firestore_cleanup

Conversation

@rosstimothy

@rosstimothy rosstimothy commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

In preparation for 19.0.0 this removes all references to legacy or broken record keys. The background key conversion will have already converted all legacy or broken keys thus allowing the removal of the migration and the key types.

Manual Test Plan

Test Environment

Local cluster off this branch with a Firestore backend.

Test Cases

  • Upgrade a v18 cluster to v19, ensure the v19 cluster is operational
  • Create a new v19 cluster, ensure the v19 cluster is operational

@rosstimothy rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Aug 12, 2026
@rosstimothy
rosstimothy requested a review from tigrato August 12, 2026 02:11
@rosstimothy
rosstimothy marked this pull request as ready for review August 12, 2026 02:11

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0eac4c527c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +177 to +178
if err := doc.DataTo(&r); err != nil {
return nil, ConvertGRPCError(err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep decoding legacy Firestore records during upgrade

For Firestore clusters that were upgraded through v18 but did not remain there until the background key migration completed, existing documents can still have the old string/array key or string value schema; the removed migration was delayed by jitter and ran in batches, so a normal sequential upgrade can leave such records behind. With this fallback gone, every Get, range read, and watcher path that calls newRecordFromDoc now returns the Firestore decode error for those documents, making existing cluster state unreadable and causing the watcher to keep retrying. Please keep the compatibility decode/migration through the v19 upgrade path or add a synchronous upgrade gate before requiring the bytes schema.

AGENTS.md reference: AGENTS.md:L14-L17

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is technically possible but customers had two major versions to correctly run the migrations. I believe it won't be a big concern

@zmb3

zmb3 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Is this related to #67655?

@public-teleport-github-review-bot
public-teleport-github-review-bot Bot removed the request for review from espadolini August 12, 2026 09:19
@tigrato

tigrato commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Is this related to #67655?

no. I added this migration because firestore mapped the keys differently based on the underlying type of the key ([]byte, Key which was an alias for []byte but ended up mapped to []any) and for a brief period of time we broke firestore backend without realising

#46472

In preparation for 19.0.0 this removes all references
to legacy or broken record keys. The background key
conversion will have already converted all legacy or broken
keys thus allowing the removal of the migration and the key types.
@rosstimothy
rosstimothy force-pushed the tross/firestore_cleanup branch from 0eac4c5 to 704ee2d Compare August 12, 2026 16:32
@rosstimothy
rosstimothy added this pull request to the merge queue Aug 12, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 12, 2026
@rosstimothy
rosstimothy added this pull request to the merge queue Aug 12, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 12, 2026
@rosstimothy
rosstimothy added this pull request to the merge queue Aug 12, 2026
Merged via the queue into master with commit 9c1fc27 Aug 12, 2026
51 of 55 checks passed
@rosstimothy
rosstimothy deleted the tross/firestore_cleanup branch August 12, 2026 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants