Skip to content

fix(encoder): heal a diverged shared-structures dictionary on decode (durable-wins reload)#1506

Draft
kriszyp wants to merge 1 commit into
mainfrom
kris/struct-reload-recovery
Draft

fix(encoder): heal a diverged shared-structures dictionary on decode (durable-wins reload)#1506
kriszyp wants to merge 1 commit into
mainfrom
kris/struct-reload-recovery

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 26, 2026

Copy link
Copy Markdown
Member

Status: defense-in-depth (held) — root cause now fixed by #1520

The v4→v5 structure-dictionary fork's root cause is the migration not persisting the canonical seed: copyDb's shapeForStructure stubbed the RecordObjects the migration reads (value.constructor === Object gate), so the observer minted no structure and the seed was skipped — every v5 worker then minted the dictionary from an empty durable and forked it. That's fixed at the source by #1520. With the seed persisted, workers adopt one canonical dictionary and don't diverge.

This PR is therefore no longer the primary fix — it's defense-in-depth:

  • decode-heal (durable-wins reload on a present-but-wrong decode) — recovers any residual in-memory divergence in-process, without a restart: a worker that somehow still strands a divergent dictionary self-heals on the next decode instead of returning nulls until a restart.
  • The isFaithfulExtension saveStructures guard here is not load-bearing — the optimistic-transaction retry + durable-wins merge already converge concurrent appends (thanks @kzyp for pushing on this). It's kept only as cheap belt-and-suspenders; if this PR is promoted it should likely be trimmed to just the decode-heal.

Holding in draft pending evidence it's needed beyond #1520. If residual divergence shows up in the field after #1520 ships — or we want no-restart recovery as insurance — promote (and trim) this; otherwise close it.

Superseded as the primary fix by #1520. Refs #1508, #1453.

— repositioned by KrAIs (Claude Opus 4.8)

…(durable-wins reload)

A v5 worker can strand an in-memory classic shared-structures dictionary whose
id->shape order diverges from the canonical durable order - most often via the
replication-apply / full-copy re-encode path, which mints structures off the
saveStructures CAS. Because msgpackr only auto-reloads on a *missing* id (and its
decode-time merge is in-memory-wins), a "present-but-wrong" decode ("Unexpected
end of MessagePack data" / "Data read, but end of buffer not reached") never
self-heals: every read of the affected ids returns null and the node serves wrong
counts / empty reads until restart. No durable data loss (restart recovers), but
an outage-class per-node wedge under v4->v5 upgrade write load.

Reproduced on a 4-node 4.7.34->5.1.14 in-place rolling upgrade at 100 w/s: the
mid-rolling node accumulates 100k+ such decode errors and fails to converge.

- decode(): on a present-but-wrong (isStructureMismatchError) decode, reload the
  durable dictionary AUTHORITATIVELY (durable-wins, one-arg _mergeStructures) and
  retry once, guarded against recursion. Heals the diverged worker transparently.
- saveStructures(): read the latest committed structures fresh
  (this.rootStore.getBinarySync) instead of the txn start-snapshot, matching
  rocksdb-js's own saveStructures, so the compatibility check sees concurrent peers.
- saveStructures(): enforce the append-only invariant - reject a same-length but
  reordered dictionary (isFaithfulExtension) that the length-only isCompatible
  would otherwise admit, clobbering a peer's committed order.

Validated: the in-place upgrade repro goes green (exact ledger parity on all four
nodes, 100% value-readable, recovery fires and heals, no wedge).

The off-CAS divergence source (replication-apply re-encode not reconciling through
the structures CAS, and #1455's canonical seed persisted in a format the typed-
table runtime discards) is tracked as a follow-up; this change makes the condition
self-healing regardless.

Refs #1453, #1445

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@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 introduces a mechanism to detect and recover from 'present-but-wrong' structure mismatch errors during decoding in RecordEncoder.ts by reloading the canonical durable structures and retrying. It also enforces an append-only invariant on structures to prevent silent corruption. The feedback recommends hardening namedStructures to guarantee it always returns an array to prevent bypassing the append-only check, and improving error handling and logging within the recovery block of decode() to prevent silent failures if _mergeStructures is unavailable or throws an error.

Comment on lines +75 to +80
function namedStructures(dict: any): any[] {
if (!dict) return [];
if (Array.isArray(dict)) return dict;
if (dict instanceof Map) return dict.get('named') ?? [];
return dict.named ?? [];
}

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.

medium

The namedStructures function extracts the shared-structures array from the dictionary. However, if dict.named or dict.get('named') is not an array (e.g., due to corrupted data or an unexpected structure format), it will return that non-array value. This would cause isFaithfulExtension to silently return true because a.length would be undefined, bypassing the append-only check.

We should harden namedStructures to guarantee it always returns an array.

Suggested change
function namedStructures(dict: any): any[] {
if (!dict) return [];
if (Array.isArray(dict)) return dict;
if (dict instanceof Map) return dict.get('named') ?? [];
return dict.named ?? [];
}
function namedStructures(dict: any): any[] {
if (!dict) return [];
if (Array.isArray(dict)) return dict;
if (dict instanceof Map) {
const named = dict.get('named');
return Array.isArray(named) ? named : [];
}
return Array.isArray(dict.named) ? dict.named : [];
}

Comment on lines +505 to +517
if (!this._reloadingStructures && isStructureMismatchError(error)) {
this._reloadingStructures = true;
try {
this._mergeStructures(this.getStructures());
const recovered = this.decode(buffer, options);
recordAction(true, STRUCTURE_RELOAD_RECOVERED_METRIC, this.name ?? this.rootStore?.name);
return recovered;
} catch {
// Reload did not resolve it — fall through to the generic error path below.
} finally {
this._reloadingStructures = false;
}
}

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.

medium

In the recovery block, if this._mergeStructures is not a function (e.g., due to an unexpected runtime environment or dependency version mismatch), or if getStructures throws an error, the error is silently caught and swallowed. This makes troubleshooting extremely difficult because the actual cause of the recovery failure is hidden, and it just falls through to the generic "Error decoding record" log with the original mismatch error.

We should check if _mergeStructures is a function before calling it, and log any unexpected errors thrown during the recovery attempt.

			if (!this._reloadingStructures && isStructureMismatchError(error)) {
				this._reloadingStructures = true;
				try {
					if (typeof this._mergeStructures === 'function') {
						this._mergeStructures(this.getStructures());
						const recovered = this.decode(buffer, options);
						recordAction(true, STRUCTURE_RELOAD_RECOVERED_METRIC, this.name ?? this.rootStore?.name);
						return recovered;
					} else {
						harperLogger.warn('Cannot recover from structure mismatch: _mergeStructures is not a function');
					}
				} catch (recoveryError) {
					harperLogger.warn('Structure reload recovery failed', recoveryError);
				} finally {
					this._reloadingStructures = false;
				}
			}

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp

kriszyp commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

Repositioned as defense-in-depth and holding in draft: the root cause is fixed at the source by #1520 (the migration now persists the canonical seed; shapeForStructure was stubbing the RecordObjects it reads). This PR's decode-heal stays as optional no-restart recovery for any residual divergence; isFaithfulExtension here isn't load-bearing. Will promote (and trim to just the heal) only if the field shows it's needed beyond #1520. — KrAIs

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.

1 participant