Repository navigation
Report a non-committed shared-structure save as not-saved (v5.0) - #1157
Conversation
Backport to v5.0 (deployed line). RecordEncoder.saveStructures wrapped rootStore.transactionSync, which returns `undefined` when the structure-save txn was aborted (ERR_ALREADY_ABORTED swallowed); the success path also fell through to `undefined`. msgpackr only re-packs on `=== false`, so a swallowed abort looked like success and the already-encoded record (referencing the new structure id) was written even though the structure was never persisted → "Record id is not defined for N" on a fresh reader (the dominant CDI rt-dev error). Return `true` only on a committed save, `false` otherwise. Bumps msgpackr to 1.11.14, which marks structures uninitialized on save-failure so the re-pack reloads durable structures, rebuilds the trie, and re-mints + re-saves. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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 upgrades msgpackr to version 1.11.14 and fixes an issue in RecordEncoder.ts where aborted transactions in saveStructures returned undefined instead of false, causing msgpackr to write records referencing unsaved structures. The reviewer identified an issue where this.structureUpdate is updated inside the transaction callback before the commit is confirmed. If the transaction aborts, this side-effect remains, which could lead to incorrect metadata flags. The reviewer suggested assigning this.structureUpdate only after a successful commit.
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.
| this.structureUpdate = structures; | ||
| return true; | ||
| }, | ||
| { retryOnBusy: true } | ||
| ); | ||
| return committed === true ? true : false; |
There was a problem hiding this comment.
If the transaction aborts or fails to commit (e.g., due to a conflict), this.structureUpdate is still set to structures because the assignment happens inside the transaction callback before the commit is confirmed. This can lead to incorrect metadata flags (like HAS_STRUCTURE_UPDATE) being written to the audit log for failed/aborted transactions.
To prevent this side-effect, this.structureUpdate should only be assigned if the transaction successfully commits.
\t\t\t\t\t\treturn true;\n\t\t\t\t\t},\n\t\t\t\t\t{ retryOnBusy: true }\n\t\t\t\t);\n\t\t\t\tif (committed === true) {\n\t\t\t\t\tthis.structureUpdate = structures;\n\t\t\t\t\treturn true;\n\t\t\t\t}\n\t\t\t\treturn false;There was a problem hiding this comment.
Good catch — fixed in bc61512. Moved this.structureUpdate = structures out of the transactionSync callback so it's only assigned when committed === true. On an aborted/declined txn it now stays unset, so no spurious HAS_STRUCTURE_UPDATE can be flagged for an unpersisted structure. Also dropped the redundant ternary. — Claude
Move `this.structureUpdate = structures` out of the transactionSync callback so it is only set on a confirmed commit. Setting it inside the callback left it dangling on an aborted txn, which could flag a spurious HAS_STRUCTURE_UPDATE in the audit log for a structure that was never persisted. Addresses review feedback. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Pre-existing format-check failure on the v5.0 line (unformatted code from the RocksDB-config change); not related to the structure-save fix, folded in here to get the Format Check green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
v5.0 backport of the
Record id is not defined for Ndesync fix (main: #1154). This is the deployed line where the error fires on CDI rt-dev.RecordEncoder.saveStructuresreturnedundefinedwhen the structure-save txn aborted (ERR_ALREADY_ABORTEDswallowed bytransactionSync) — and the success path also fell through toundefined. msgpackr only re-packs on=== false, so a swallowed abort looked like success and the already-encoded record (referencing the new structure id) was persisted even though the structure was never saved →Record id is not defined for Non a fresh reader.trueonly on a committed save,falseotherwise (CAS conflict or aborted txn).return falsealone doesn't recover (the re-pack reused the cached transition).Validated end-to-end (
~/dev/scripts/savestructures-abort-repro.cjs). Independent of the typed-structs work — this is the classic shared-structure path. The auto-cherry-pick can't carry this cleanly (main bumps msgpackr to 2.0.3; v5.0 needs 1.11.14), hence a dedicated PR.🤖 Generated by Claude (Opus 4.7).