mpackdb

15.5 KB

1. # BUG: `refresh()` clobbers un-persisted in-memory meta during a concurrent write → lost deletes/inserts
3. **Severity:** High — silent data corruption (deleted rows resurrect; inserts can vanish) under
4. concurrent reads + writes in the *same process*. No error is thrown.
6. **Status: FIXED (2026-07-02).** Reproduced first with `tests/concurrent-delete-during-scan.test.js`
7. (failed on the old code: deleted record resurrected in full scan), then fixed:
9. - `meta.json` now carries a monotonic `version`, bumped by every `persistMeta()` (written
10. atomically via tmp+rename). `refresh()` only adopts the on-disk meta when its version is
11. NEWER than the in-memory one — i.e. another process persisted under the file lock.
12. In-memory meta stays authoritative for this process, so the clobber below cannot happen.
13. - Writers refresh right after acquiring the file lock, so cross-process `nextId`/tombstones
14. are current before mutating.
15. - Bonus fix found during the work: the in-process lock was *temporal*, not causal — any
16. operation that merely STARTED while another held the lock walked straight into the critical
17. section (`_lockDepth++`). Locking is now based on `AsyncLocalStorage` call-chain ownership
18. plus an in-process FIFO queue (`tests/causal-lock.test.js`).
20. Verified by `tests/concurrent-delete-during-scan.test.js` (40 rounds, 5x repeat, stable),
21. `tests/causal-lock.test.js`, `tests/multi-process-write.test.js`, and the full suite.
22. The original analysis is kept below for reference.
24. ---
26. ## Summary
28. Read scans (`recordGenerator` / `find`) do **not** take the instance lock, and every scan begins
29. with `await this.refresh()`, which **unconditionally overwrites `this._meta` with the on-disk
30. copy**. A concurrent `delete()` (or `insert()`) holds the lock, mutates `this._meta` in memory,
31. and only calls `persistMeta()` **after an `await`**. If an unlocked scan's `refresh()` runs during
32. that await window, it replaces `this._meta` with the *stale* on-disk version — discarding the
33. in-memory mutation. The write then persists the rolled-back meta, so the change is silently lost.
35. For a delete, the lost mutation is a tombstone offset → **the deleted record is resurrected** on
36. subsequent scans. For an insert, `nextId`/append bookkeeping can be lost.
38. ## Root cause (code)
40. `src/MPackDB.js`:
42. 1. **`refresh()`** — unconditional wholesale reload:
43. ```js
44. async refresh() {
45. try { this._meta = JSON.parse(await readFile(this._metaPath)); } catch (e) {}
46. }
47. ```
49. 2. **`recordGenerator()`** (backs `find()`) — calls `refresh()` with **no lock held**:
50. ```js
51. async *recordGenerator(queryFn = null, { ... } = {}) {
52. await this.init();
53. await this.refresh(); // <-- unlocked; clobbers in-memory meta
54. ...
55. }
56. ```
58. 3. **`delete()`** — holds the lock, mutates meta in memory, `await`s, THEN persists:
59. ```js
60. async delete(mixed, { ... } = {}) {
61. await this._acquireLock('delete', mixed);
62. try {
63. for await (const [record, offset] of this.find(mixed, ...)) {
64. this._meta.deleted.push(offset); // (A) in-memory mutation, NOT persisted
65. if (this._indexManager) await this._indexManager.remove(...); // (B) await → yields event loop
66. }
67. await this.persistMeta(); // (C) writes this._meta to disk
68. } finally { await this._releaseLock(); }
69. }
70. ```
72. ### The interleaving that corrupts data (single process)
74. ```
75. delete(): acquire lock
76. delete(): this._meta.deleted.push(offset) // (A) tombstone in memory only
77. delete(): await indexManager.remove(...) // (B) event loop yields here
78. find(): recordGenerator -> await refresh() // NO lock; reloads on-disk meta
79. find(): this._meta = <on-disk copy> // ← DROPS the pushed tombstone
80. delete(): await persistMeta() // (C) persists meta WITHOUT the tombstone
81. => the "deleted" row is not tombstoned; future scans return it (resurrected).
82. ```
84. Note `delete()` itself calls `find()` internally (the `for await ... of this.find(...)`), so the
85. scan and the delete are routinely in flight together, and any other concurrent `find()` on the
86. same instance (e.g. a background query) triggers the same clobber.
88. ## Impact / how it was found
90. Found in a multi-lane blockchain (adryllium-2026-3) built on mpackdb. The UTXO store gets constant
91. concurrent access: background scans (`find`), point lookups, and a state-fingerprint reader run
92. concurrently with `delete()` (spend) and `insert()` (create). Spent UTXOs kept **reappearing** on
93. some nodes (and occasionally inserts were lost), causing derived-state divergence across nodes that
94. should be byte-identical. Every application-level guard failed because the corruption is **below**
95. the application — mpackdb was losing writes. It also caused a downstream "post-write invariant
96. check" to see a just-deleted row as live and trigger expensive, unnecessary rebuilds.
98. The bug is independent of any external/second writer — it happens with a **single process** doing
99. concurrent reads and writes on one `MPackDB` instance.
101. ## Reproduction sketch
103. ```js
104. const db = new MPackDB('/tmp/racetest', { primaryKey: 'id' });
105. await db.insert({ id: 'x', v: 1 });
106. // Concurrently: delete 'x' while hammering find() so a scan's refresh() lands in delete()'s await window.
107. await Promise.all([
108. db.delete('x'),
109. (async () => { for (let i = 0; i < 50; i++) { for await (const _ of db.find(() => true)) {} } })(),
110. ]);
111. // BUG: (await db.find('x')).length is sometimes 1 — 'x' resurrected. Should always be 0.
112. ```
113. (Timing-dependent; loop it or add a small await inside `indexManager.remove` to widen the window.)
115. ## Suggested fix
117. `refresh()` must not reload when nothing changed externally, because **in-memory meta is
118. authoritative for this process** and may contain un-persisted mutations. Only a genuine external
119. change (another process wrote the meta file under the file lock) should trigger a reload.
121. **Do NOT** simply make `refresh()` acquire the lock — `delete()`/`insert()` call `find()` (→
122. `refresh()`) *while already holding the lock*, so locking `refresh()` would deadlock (non-reentrant
123. lock).
125. Recommended: guard the reload on the meta file's identity since our last read/write.
127. - Track `this._metaStat = { mtimeMs, size }` whenever we read the meta (in `init()`) or write it
128. (end of `persistMeta()`, via `await stat(this._metaPath)`).
129. - In `refresh()`: `stat()` the file; if `mtimeMs` and `size` are unchanged from `this._metaStat`,
130. return without reloading; otherwise reload and update `this._metaStat`.
132. This makes `refresh()` a no-op within a single writer (in-memory stays authoritative, so the
133. concurrent-delete clobber can't happen) while still reloading after a genuine external write.
135. Caveat: `mtimeMs`+`size` can collide on coarse-mtime filesystems or same-size rewrites. If robust
136. multi-writer support is required, prefer a **monotonic version counter persisted inside the meta**
137. (bump on every `persistMeta`; `refresh()` reloads only if the on-disk version > in-memory version).
138. The stat-based guard is sufficient for single-writer-per-file usage (the common case here).
140. ## Suggested test
142. Add a `tests/concurrent-delete-during-scan.test.js` that runs many `find()` scans concurrently with
143. `delete()`/`insert()` and asserts deleted keys never reappear and inserted keys never vanish (the
144. repro above, looped). The existing `concurrent-find-insert.test.js` / `concurrent-lock.test.js` do
145. not cover the delete-tombstone-loss case.
147. ## Files
149. - `src/MPackDB.js`: `refresh()`, `recordGenerator()`, `delete()`, `insert()`, `persistMeta()`,
150. `init()` (meta load).
152. ---
154. # BUG: index lookup leaks `ERR_INVALID_ARG_TYPE` when `_indexPaths[field]` is undefined
156. **Severity:** Medium — opaque rejected operation which can terminate a process when uncaught; no
157. data corruption. Reported against package 1.0.7 before publication.
159. **Status: FIXED IN SOURCE (2026-07-20), consumer confirmation pending.**
161. ## Resolution / maintainer findings
163. - All production access to `_indexPaths` now goes through `IndexManager._getIndexPath(field)`.
164. Paths for declared indexes are deterministic, populated synchronously in the constructor and
165. reconstructed if a slot is unexpectedly absent. This prevents `readFile(undefined)` for the
166. reported declared-index condition.
167. - Truly undeclared hints now reject with a clear `INDEX_NOT_FOUND` error carrying `field` and the
168. configured `indexes`. Returning `[]` was rejected because that would silently create false
169. negatives and could break find-then-insert uniqueness logic.
170. - The report's exact/range asymmetry was incorrect: both `get()` and `entries()` use
171. `_readIndexEntries()`, and both previously threw the same raw TypeError for an undeclared field.
172. - The proposed `resetAfterReplace()` race was not supported by the implementation or runtime test:
173. reset never clears `_indexPaths`. An external compaction/reopen preserved every path and the
174. subsequent declared exact lookup succeeded. The original observation most likely involved an
175. instance whose effective index configuration did not contain the requested field; the new error
176. will expose that configuration directly if it recurs.
177. - Regression coverage: `tests/index-path-guard.test.js` forces a declared path slot missing, checks
178. exact and range recovery, checks both unknown-hint errors, and performs an external replacement.
179. The targeted test, all 20 test files, and a clean packed-artifact consumer test pass.
181. The original report is retained below for incident context; its range-immunity and reset-race
182. hypotheses are historical and disproven.
184. ---
186. ## Original report (historical)
188. ## Summary
190. An exact-value indexed lookup — `db.find(fn, { index: { field, value } })`, which routes through
191. `IndexManager.get()` → `_getOnDisk()` → `_readIndexEntries()` — crashed with:
193. ```
194. TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received undefined
195. at readFile (node:internal/fs/promises:1279:20)
196. at IndexManager._readIndexEntries (src/IndexManager.js:393:39) // readFile(this._indexPaths[field], 'utf-8')
197. at IndexManager._getOnDisk (src/IndexManager.js:573:34)
198. at IndexManager.get (src/IndexManager.js:545:36)
199. at MPackDB._indexedStream (src/MPackDB.js:703:46) // hint.value !== undefined branch
200. at MPackDB.recordGenerator (src/MPackDB.js:529:16)
201. at async Cursor.toArray (src/Cursor.js:60:20)
202. ```
204. `_readIndexEntries(field)` does `readFile(this._indexPaths[field], 'utf-8')`. When
205. `this._indexPaths[field]` is `undefined`, `readFile(undefined)` throws `ERR_INVALID_ARG_TYPE`. The
206. surrounding `try/catch` only swallows `ENOENT`, so this error propagates and (in our case) became an
207. uncaught exception that killed the process.
209. ## Two independent problems
211. 1. **The robustness gap (definitely real, trivially confirmable by reading the code):**
212. `_indexPaths[field]` is populated *only* in `IndexManager.init()` (src/IndexManager.js:62,
213. iterating the declared `_indexes`) and is never re-cleared. If `get()` is ever reached with a
214. `field` whose path slot is `undefined` — a field not in the declared index set, or a `get()`
215. that races `init()`/`resetAfterReplace()` before the path map is (re)populated — the code does
216. `readFile(undefined)` and throws an opaque `ERR_INVALID_ARG_TYPE` instead of returning `[]` or a
217. clear "no such index" error.
219. **Asymmetry:** the *range-scan* path (`entries()`, used by `boundingBox()` and `from/to` hints)
220. tolerates a missing/empty index gracefully (returns nothing) — only the *exact-value* path
221. (`get()`/`_getOnDisk()`/`_readIndexEntries()`) faults. In our migration, `boundingBox()` over an
222. empty `units` collection returned `[]` cleanly in the same run where an exact-value `url` lookup
223. crashed.
225. 2. **The trigger (observed, NOT yet isolated):** how `_indexPaths[field]` became `undefined` for a
226. *declared* index. It happened during a find-then-insert (`uniqueUrl` doing
227. `find(r => r.url === url, { index: { field: 'url', value: url } })`) on a collection declared with
228. `indexes: ['name','url','email']`, in a **second process** (a one-off Node script) attached to
229. files that a **first process** (a long-running server, `compact:false`) also held open, shortly
230. after a `drop()` (our `delete(() => true)` + `compact()`) had rewritten/replaced those files —
231. i.e. the external-compaction `refresh()` → `_reopenAfterExternalReplace()` →
232. `resetAfterReplace()` path was in play, and the server's 30Hz tick was doing concurrent index
233. ops throughout. This points at an init/reset-ordering race around `_indexPaths`, not a plain
234. empty-collection case.
236. ## Reproduction attempts (could NOT reproduce standalone)
238. All of the following returned cleanly (`0` rows) and created a 0-byte `<name>.<field>.txt`:
239. - fresh collection, no inserts, then `find(fn, {index:{field:'url', value:'x'}})`;
240. - `insert` → `delete(() => true)` → `compact()` → exact-value lookup;
241. - two `MPackDB` instances on the same files, second does the exact-value lookup;
242. - `drop()` of a never-written collection, then a second instance + `refresh()` + exact-value lookup.
244. So a single-process, quiescent reproduction does **not** trigger it. Reproducing it likely needs the
245. concurrent multi-process timing above (long-running writer doing continuous index ops + external
246. compaction + a second reader issuing an exact-value lookup during the reopen/reset window). Sharing
247. this now so the maintainer with the internals in hand can pin the exact interleaving.
249. ## Suggested fix
251. - **Make the failure safe and clear (fixes the crash regardless of trigger).** In
252. `_readIndexEntries()` (and anywhere `_indexPaths[field]` is read), guard the path:
253. ```js
254. async _readIndexEntries(field) {
255. const path = this._indexPaths[field];
256. if (!path) return []; // not-yet-initialized / unknown field — behave like the range-scan path
257. try {
258. return this._parseIndexLines(await readFile(path, 'utf-8'), field);
259. } catch (e) {
260. if (e.code === 'ENOENT') return [];
261. throw e;
262. }
263. }
264. ```
265. (Or throw an explicit `Error(`No index for field "${field}"`)` when the field isn't declared, so a
266. genuine misuse is obvious instead of an opaque fs TypeError.) This aligns the exact-value path
267. with the already-tolerant range-scan path.
268. - **Investigate the trigger:** ensure `get()`/`_getOnDisk()` can never run against a half-populated
269. `_indexPaths` — e.g. confirm `init()`/`resetAfterReplace()` fully repopulate `_indexPaths` before
270. any query can observe the reset, under the causal lock, during a cross-process external-compaction
271. reopen.
273. ## Suggested test
275. - Unit: call the exact-value lookup path for a field that is (a) declared but whose index file does
276. not yet exist, and (b) not declared at all — assert `[]` (a) / clear error (b), never a raw
277. `ERR_INVALID_ARG_TYPE`.
278. - Concurrency: a long-lived writer doing continuous indexed `update`/`find` while a second process
279. triggers `drop()`+`compact()` (external replace), and a third issues exact-value lookups across
280. the reopen window — assert no `ERR_INVALID_ARG_TYPE` escapes.
282. ## Consumer workaround (in use)
284. Dropped exact-value `index` hints on lookups against collections that may be fresh/just-dropped
285. (`url`-uniqueness checks, `owner` counts) and used predicate full scans there instead. Range-scan
286. `boundingBox()` hints were kept (that path is immune). Small collections, so scans are cheap.
288. ## Files
290. - `src/IndexManager.js`: `_readIndexEntries()` (:393), `_getOnDisk()` (:573), `get()` (:545),
291. `init()` (:62, `_indexPaths` population), `resetAfterReplace()` (:158).
292. - `src/MPackDB.js`: `_indexedStream()` (:703, exact-value branch), `refresh()` /
293. `_reopenAfterExternalReplace()` (:772/:798).