Skip to content

fix(migration): persist canonical classic structures so v5 workers don't fork (#1453) - #1455

Merged
kriszyp merged 5 commits into
mainfrom
kris/migration-struct-fork
Jun 24, 2026
Merged

fix(migration): persist canonical classic structures so v5 workers don't fork (#1453)#1455
kriszyp merged 5 commits into
mainfrom
kris/migration-struct-fork

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 23, 2026

Copy link
Copy Markdown
Member

Summary

copyDbToRocks now persists a canonical v5 classic shared-structures dictionary at the end of migrating each primary table, built by a separate shared-mode observer encoder. The migration's own/inline encoder is unchanged, so migrated records stay self-describing.

Purpose (fixes #1453)

On a v4→v5 in-place upgrade, the migration re-encodes records with an own/inline encoder and persists no v5 classic structurescopyStructures only copies v4's verbatim (random-access/typed) buffer. So after migration the durable classic dictionary is empty, and every v5 worker mints the record structures from scratch, concurrently, assigning the same structure-id to different structures. A read-heavy worker then strands its divergent in-memory dict (it never re-mints, so msgpackr's missing-id reload never fires) and every read decodes against the wrong structure → Data read, but end of buffer not reached / null records. Confirmed by byte-level trace on the Akamai v4→v5 stage cluster (first-upgraded node lost 23 HttpCache records; see #1453 for the full investigation). The fix makes every worker adopt one agreed dictionary instead of minting divergent ones.

Where to look / lower-confidence areas

  • Integration gap (most important): the unit tests verify the migration doesn't regress (records still decode own/inline) and that the observer captures the canonical dict in-process — but cross-process worker adoption of the persisted dictionary is not unit-verified. It needs the runtime's multi-column-family open + per-worker handleLocalTimeForGets encoder wiring, which the single-handle test harness can't replicate. The persist uses the same composite-key mechanism copyStructures already uses (which the runtime reads in prod, per harper-pro#362), so I'm confident — but this should be confirmed end-to-end via the cluster repro before merge.
  • Dropping v4 typed structs: the persisted dict is classic-only (named array). I deliberately drop the vestigial v4 typed structs because keeping them makes a classic encoder's saveStructures CAS reject on a typed-length mismatch (the reload/re-mint churn behind the fork). Worth confirming this is safe vs the harper-pro#362 replication-delivered-structure path.
  • Per-record observerEncoder.encode(value) adds one extra (discarded) encode per record during migration — guarded so a structure-building failure never fails the record. Acceptable for a one-time migration, but flagging the cost.
  • Out of scope: the Campaign value-width structure proliferation (ctr/cvr float32↔float64 minting many variants) is a separate amplifier this per-shape observer does not fully address.

Cross-model review: Codex completed (its findings are addressed in the commits). The Gemini CLI was unavailable headless (interactive auth), so additional cross-model coverage relies on the GitHub bot reviewers.

🤖 Generated by an LLM (Claude Opus 4.8). Investigation + fix in collaboration with @kriszyp.

kriszyp and others added 3 commits June 22, 2026 21:52
…ecovery

Adds a real-rocksdb regression test for the structure-id fork behind the
Akamai v4→v5 "Data read, but end of buffer not reached" record loss
(GH #1453): a record that decodes correctly against the canonical durable
structures returns null when the worker's in-memory dict is forked (same
structure minted in a different id order), and reloading the durable
structures heals the read.

Confirmed by byte-level trace of a live failing record on nl-ams-1.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n't fork

The v4→v5 migration re-encodes records with an own/inline (self-describing)
encoder and never persists any v5 classic shared-structures dictionary —
copyStructures only copies v4's verbatim (typed) buffer. After migration the
durable classic dictionary is therefore empty, so every v5 worker mints the
record structures from scratch, concurrently, and assigns the same structure-id
to different structures. A read-heavy worker strands its divergent in-memory
dict (it never re-mints, so msgpackr's missing-id reload never fires) and every
read decodes against the wrong structure → "Data read, but end of buffer not
reached" / null records (#1453, confirmed on the Akamai v4→v5
stage cluster).

Fix: a separate shared-mode observer encoder accumulates the canonical classic
structures while re-encoding (the migration encoder stays own/inline, so the
migrated records remain self-describing and unchanged), captured from
saveStructures and persisted once after the loop to the composite structures
key. v5 workers then adopt one agreed dictionary on startup instead of minting
divergent ones — restoring the "structures only grow, never diverge" invariant.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kriszyp
kriszyp requested review from cb1kenobi and ldt1996 June 23, 2026 12:05

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request resolves a structure-id fork issue during the v4 to v5 migration to RocksDB by introducing an observer encoder in bin/copyDb.ts to build and persist canonical classic structures. It also adds regression and reproduction tests in unitTests/bin/migrationCanonicalStructures.test.js and unitTests/resources/structureForkOnMigration.test.js. Additionally, an accidental file named node_modules containing a local path was committed. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

kriszyp and others added 2 commits June 23, 2026 06:13
Codex review (P2): the observer's encode of file-backed Blob values ran outside
encodeBlobsWithFilePath, so the blob packer would readFileSync the full blob into
memory just to build the structure dictionary — risking stalls/OOM on blob-heavy
migrations. A classic/named structure depends only on the keys, so feed the observer
a shape skeleton (objects/arrays recursed, all leaves incl. Blobs stubbed to a
primitive). Same canonical structures, no blob reads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

Root cause: v4→v5 migration structure-id fork silently nulls records (HttpCache/Campaign/WarmupJob)

2 participants