Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion resources/RecordEncoder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,28 @@ import { CONFIG_PARAMS } from '../utility/hdbTerms.ts';
import * as envMngr from '../utility/environment/environmentManager.js';

const StructonEncoder = createStructon(Encoder) as typeof Encoder;

// Analytics counter incremented whenever a record cannot be decoded because its shared structure is
// missing on this node (see HarperFast/harper#1163). Surfaces the otherwise-silent condition in
// monitoring; the store name is passed as the metric path.
const MISSING_STRUCTURE_METRIC = 'decode-missing-structure';

// Terminal error messages msgpackr/structon throw when a record references a shared structure that
// is not in this node's structures buffer. Both the typed (random-access) path (structon's
// readStruct) and the classic path (msgpackr's createSecondByteReader) reload the structures from
// durable storage and retry before throwing, so reaching one of these means the structure is
// genuinely absent on this node — not merely stale in memory. Matched by message prefix because
// neither dependency throws a typed error.
const MISSING_TYPED_STRUCTURE_PREFIX = 'Could not find typed structure ';
const MISSING_CLASSIC_STRUCTURE_PREFIX = 'Record id is not defined for ';
Comment on lines +49 to +50

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why the global strings?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you mean vs being inside isMissingStructureError? (its not on the global scope)


export function isMissingStructureError(error: any): boolean {
const message = error?.message;
return (
typeof message === 'string' &&
(message.startsWith(MISSING_TYPED_STRUCTURE_PREFIX) || message.startsWith(MISSING_CLASSIC_STRUCTURE_PREFIX))
);
}
export type Entry = {
key: any;
value: any;
Expand Down Expand Up @@ -384,7 +406,29 @@ export class RecordEncoder extends StructonEncoder {
} // else a normal entry
return options?.valueAsBuffer ? buffer : decodeFromDatabase(() => super.decode(buffer, options), this.rootStore);
} catch (error) {
harperLogger.error('Error decoding record', error, 'data: ' + buffer.slice(0, 40).toString('hex'));
const hexPreview = buffer.slice(0, 40).toString('hex');
if (isMissingStructureError(error)) {
// This record references a shared structure that is genuinely absent on this node — the
// dependency already reloaded from durable storage and retried before throwing (typically a
// replica that received the record but not the structure-buffer update; see
// HarperFast/harper#1163). We still return null so internal reads (e.g. the metadata/__dbis__
// scan during initialization) remain non-fatal, but surface the otherwise-silent condition
// distinctly — a dedicated warning plus an analytics counter — so the dropped record is
// detectable and alertable rather than laundered as legitimate emptiness into query results,
// caches, and downstream consumers.
// this.name is set on the RocksDB encoder; for LMDB fall back to the root store's name so
// the metric/warning still attribute the dropped record to a store.
const storeName = this.name ?? this.rootStore?.name;
recordAction(true, MISSING_STRUCTURE_METRIC, storeName);
harperLogger.warn(
'Record references a shared structure missing on this node; decoded as null (see HarperFast/harper#1163)',
error,
'store: ' + storeName,
'data: ' + hexPreview
);
return null;
}
harperLogger.error('Error decoding record', error, 'data: ' + hexPreview);
return null;
}
}
Expand Down
71 changes: 70 additions & 1 deletion unitTests/resources/recordEncoder.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require('../testUtils');
const assert = require('assert');
const { RecordEncoder } = require('#src/resources/RecordEncoder');
const { RecordEncoder, isMissingStructureError } = require('#src/resources/RecordEncoder');
const harperLogger = require('#src/utility/logging/harper_logger');
const { Encoder } = require('msgpackr');

// Shared structures persisted as an encoded buffer (mirrors how a DBI stores them under
Expand Down Expand Up @@ -83,3 +84,71 @@ describe('RecordEncoder struct-mode gating', () => {
assert.strictEqual(decoded.indexed, record.indexed);
});
});

describe('RecordEncoder missing-structure handling (harper#1163)', () => {
let warnings, errors, restoreWarn, restoreError;
beforeEach(() => {
warnings = [];
errors = [];
restoreWarn = harperLogger.warn;
restoreError = harperLogger.error;
harperLogger.warn = (...args) => warnings.push(args);
harperLogger.error = (...args) => errors.push(args);
});
afterEach(() => {
harperLogger.warn = restoreWarn;
harperLogger.error = restoreError;
});

it('returns null (non-fatal) and warns distinctly when a typed structure is absent on this node', () => {
// A record references a typed (random-access) structure that this node's structures buffer does
// not contain. structon's readStruct reloads from the (still-empty) store and then throws;
// RecordEncoder must keep internal reads non-fatal (return null) while surfacing the dropped
// record distinctly rather than via the generic error path.
const writer = makeEncoder(true, sharedStore());
const bytes = Buffer.from(writer.encode(record));

// Reader on a different node that never received the structure-buffer update.
const reader = makeEncoder(true, sharedStore());
assert.strictEqual(reader.decode(bytes), null, 'missing structure should decode to null, not throw');
assert.strictEqual(warnings.length, 1, 'should emit exactly one distinct warning');
assert.match(warnings[0][0], /shared structure missing/);
assert.strictEqual(errors.length, 0, 'should not use the generic error path for a missing structure');
});

it('recovers (decodes, no throw) once the typed structure is present on this node', () => {
// Writer and reader share the same structures store, so the reader's on-miss reload finds it.
const store = sharedStore();
const writer = makeEncoder(true, store);
const bytes = Buffer.from(writer.encode(record));
const reader = makeEncoder(true, store);
const decoded = reader.decode(bytes);
assert.ok(decoded, 'record should decode when the structure is available');
assert.strictEqual(decoded.name, record.name);
});

it('detects both typed and classic missing-structure errors, and only those', () => {
// classic-structure miss (msgpackr createSecondByteReader) is the relevant variant on 5.1 where
// typed structs are off by default; we cannot easily manufacture a real classic shared-structure
// miss in this harness, so assert the detection contract directly against the dependency's
// terminal error messages.
assert.ok(isMissingStructureError(new Error('Could not find typed structure 1')));
assert.ok(isMissingStructureError(new Error('Record id is not defined for 42')));
assert.ok(!isMissingStructureError(new Error('Data read, but end of buffer not reached 64')));
assert.ok(!isMissingStructureError(new RangeError('Offset is outside the bounds of the DataView')));
assert.ok(!isMissingStructureError(undefined));
});

it('still returns null (tolerant) for a decode failure that is not a missing structure', () => {
// Truncate a valid struct-mode record mid-body: the structure IS present, but the buffer is too
// short, so decoding throws a non-missing-structure error (e.g. out-of-bounds). That genuine
// corruption keeps the existing log-and-null behavior.
const store = sharedStore();
const enc = makeEncoder(true, store);
const bytes = Buffer.from(enc.encode(record));
const truncated = bytes.subarray(0, 2);
assert.strictEqual(enc.decode(truncated), null, 'corrupt (non-structure) decode should still return null');
assert.strictEqual(errors.length, 1, 'genuine corruption should use the generic error path');
assert.strictEqual(warnings.length, 0, 'genuine corruption should not use the missing-structure warning');
});
});
Loading