fix: forward isCompatible from _saveTypedStructures to saveStructures#6
fix: forward isCompatible from _saveTypedStructures to saveStructures#6kriszyp wants to merge 1 commit into
Conversation
_saveTypedStructures called this.saveStructures(structures) without forwarding the isCompatible function that prepareStructures attaches to the returned structures. A saveStructures implementation that runs an optimistic CAS on the second parameter (e.g. Harper's RocksDB override) therefore saw `undefined`, so a concurrent same-length typed-struct save could silently clobber the previously persisted struct. Forward it as the second arg, matching msgpackr's own pack call site (saveStructures(newSharedData, newSharedData.isCompatible)). See HarperFast/harper#1441 / #1442. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates _saveTypedStructures in index.js to forward structures.isCompatible as the second argument to this.saveStructures, matching msgpackr's call site behavior and preventing potential concurrency issues where same-length saves could clobber each other. A corresponding regression test has been added in tests/test.js to verify this behavior. There are no review comments, and I have no additional 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.
2 participants
Summary
_saveTypedStructuresnow forwardsisCompatibleas the second argument tosaveStructures, matching msgpackr's own pack call site (saveStructures(newSharedData, newSharedData.isCompatible)).Why
prepareStructuresattachesisCompatibleto the returned structures object, but_saveTypedStructurescalledthis.saveStructures(structures)and dropped it. AsaveStructuresimplementation that runs an optimistic CAS on the parameter (e.g. Harper's RocksDBRecordEncoderoverride) then seesundefinedand a concurrent same-length typed-struct save can silently clobber the previously persisted struct. See HarperFast/harper#1441 / #1442.Scope / where to look
One-line fix plus a regression test. The test exercises
_saveTypedStructuresdirectly because the fast path (msgpackr ≥ 2 with struct hooks) saves structures via the base class's own pack call site, which already passesisCompatible— so the bug only bites consumers on the standalone path (a base withoutSUPPORTS_STRUCT_HOOKS, e.g. cbor-x) or anysaveStructuresthat CASes on the parameter. The test asserts the forwarded arg is theisCompatiblethatprepareStructuresattached, and fails without the fix.🤖 Generated by an LLM (Claude Opus 4.8), defense-in-depth follow-up to the harper#1453 investigation with @kriszyp.