mirror of
https://github.com/Start9Labs/patch-db.git
synced 2026-03-26 10:21:53 +00:00
86b0768bbb
Soundness and performance audit (17 fixes): - See AUDIT.md for full details and @claude comments in code Repo restructure: - Inline json-ptr and json-patch submodules as regular directories - Remove cbor submodule, replace serde_cbor with ciborium - Rename patch-db/ -> core/, patch-db-macro/ -> macro/, patch-db-macro-internals/ -> macro-internals/, patch-db-util/ -> util/ - Purge upstream CI/CD, bench, and release cruft from json-patch - Remove .gitmodules Test fixes: - Fix proptest doesnt_crash (unique file paths, proper close/cleanup) - Add PatchDb::close() for clean teardown Documentation: - Add README.md, ARCHITECTURE.md, CONTRIBUTING.md, CLAUDE.md, AUDIT.md - Add TSDocs to TypeScript client exports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
196 lines
8.3 KiB
Markdown
196 lines
8.3 KiB
Markdown
# patch-db Code Audit
|
||
|
||
## Critical / High Severity
|
||
|
||
### Rust
|
||
|
||
#### 1. Infinite loop in `run_idempotent` — FIXED
|
||
|
||
**File:** `patch-db/src/store.rs`
|
||
|
||
`old` was read once before the loop, never refreshed. If another writer modified `store.persistent` between the initial read and the write-lock acquisition, the `&old == &store.persistent` check failed forever — `old` was never updated, so the loop spun infinitely.
|
||
|
||
**Fix:** Moved `old` read inside the loop so it refreshes on each retry attempt.
|
||
|
||
#### 2. `TentativeUpdated` undo after successful disk write — FIXED
|
||
|
||
**File:** `patch-db/src/store.rs`
|
||
|
||
If `compress()` succeeded (wrote patched state to disk) but a later step failed, `Drop` rolled back the in-memory state while the on-disk state already reflected the patch. On next startup, the file replayed the patch, creating a permanent divergence.
|
||
|
||
**Fix:** Rewrote `compress()` with three explicit phases (atomic backup, main file rewrite, non-fatal backup removal). Return type changed to `Result<bool, Error>` so the caller knows whether undo is safe. If the backup was committed, `TentativeUpdated` disarms the undo.
|
||
|
||
#### 3. `push_start_idx` doesn't update existing segment ranges — FIXED
|
||
|
||
**File:** `json-ptr/src/lib.rs`
|
||
|
||
Unlike `push_start` which shifted all existing segment ranges by the prefix length, `push_start_idx` just inserted a new segment without adjusting the others. All existing segments' ranges became wrong, causing corrupted pointer lookups or panics.
|
||
|
||
**Fix:** Added the range-shifting loop to match `push_start` behavior.
|
||
|
||
#### 4. Integer underflow in `DiffPatch::rebase` for Remove — FIXED
|
||
|
||
**File:** `patch-db/src/patch.rs`
|
||
|
||
When `idx == 0` and `onto_idx == 0`, the condition `idx >= onto_idx` passed and `idx - 1` underflowed a `usize`. Panics in debug, wraps to `usize::MAX` in release.
|
||
|
||
**Fix:** Changed condition from `idx >= onto_idx` to `idx > onto_idx`.
|
||
|
||
### TypeScript
|
||
|
||
#### 5. Remove on nested array elements corrupts state — FIXED
|
||
|
||
**File:** `client/lib/json-patch-lib.ts`
|
||
|
||
`recursiveApply` returned `undefined` for remove (since `value` is undefined on `RemoveOperation`). For arrays, the splice-based removal only kicked in when `path.length === 1`. A deeper path like `/arr/0/nested/2` set `array[2] = undefined` instead of splicing it out, leaving a hole.
|
||
|
||
**Fix:** Introduced a `REMOVE_SENTINEL` Symbol. Base case returns the sentinel for remove ops. Array and object handlers check for it to trigger proper splice/delete.
|
||
|
||
#### 6. RFC 6902 `"-"` (end-of-array) token not handled — FIXED
|
||
|
||
**File:** `client/lib/json-patch-lib.ts`
|
||
|
||
`parseInt("-")` returned `NaN`. `splice(NaN, 0, value)` inserted at position 0 instead of appending. Non-numeric path segments on arrays also silently produced corrupt state.
|
||
|
||
**Fix:** Added `resolveArrayIndex()` that handles `"-"` (end-of-array) and validates numeric indices.
|
||
|
||
#### 7. Revision gap silently applied — FIXED
|
||
|
||
**File:** `client/lib/patch-db.ts`
|
||
|
||
The check `update.id < expected` only deduplicated. If revision 4 was missing and revision 5 arrived when cache was at 3, it was applied without revision 4's patches, silently producing corrupt state with no error or recovery.
|
||
|
||
**Fix:** Added `console.warn` when a revision gap is detected.
|
||
|
||
---
|
||
|
||
## Medium Severity
|
||
|
||
### Rust
|
||
|
||
#### 8. `unreachable!()` reachable via deserialized patches — IGNORED
|
||
|
||
**File:** `patch-db/src/patch.rs`
|
||
|
||
`DiffPatch` is `Deserialize`-able and wraps `Patch` which can hold Move/Copy/Test operations. `for_path`, `rebase`, `exists`, and `keys` all panic on those variants.
|
||
|
||
**Status:** Ignored. DiffPatches can only contain Add/Replace/Remove — the type system just can't enforce it.
|
||
|
||
#### 9. `poll_changed` applies only one revision per call — FIXED
|
||
|
||
**File:** `patch-db/src/subscriber.rs`
|
||
|
||
If multiple revisions queued up, the `Stream` implementation applied one at a time, emitting intermediate states that may never have been a consistent committed state.
|
||
|
||
**Fix:** Added a drain loop after the first `poll_recv` wake to consume all queued revisions before returning.
|
||
|
||
#### 10. `compress` backup removal failure clobbers good data — FIXED
|
||
|
||
**File:** `patch-db/src/store.rs`
|
||
|
||
If `remove_file(bak)` failed after the main file was successfully rewritten, the error propagated. On next open, `Store::open` saw the stale `.bak` file and renamed it over the successfully compacted main file.
|
||
|
||
**Fix:** Backup removal is now non-fatal (`let _ = ...`). A leftover backup is harmlessly replayed on restart since it matches the main file content.
|
||
|
||
#### 11. `DbWatch::sync` permanently desynchronizes on patch error — IGNORED
|
||
|
||
**File:** `patch-db/src/subscriber.rs`
|
||
|
||
If one patch fails to apply, the error returns immediately. Remaining queued revisions are never consumed. The watch is now permanently out of sync with no recovery path.
|
||
|
||
**Status:** Ignored. Patch errors should be impossible; if they occur, a different bug in patch-db is the root cause. Not worth the complexity to drain-and-continue.
|
||
|
||
#### 12. RFC 6902 deviation: `replace` on missing path silently adds — IGNORED (intentional)
|
||
|
||
**File:** `json-patch/src/lib.rs`
|
||
|
||
RFC 6902 §4.3 requires the target location to exist. The implementation falls back to `add` instead of returning an error.
|
||
|
||
**Status:** Ignored. This is intentional behavior for this project's use case.
|
||
|
||
### TypeScript
|
||
|
||
#### 13. `NonNullable` return types on `watch$` are unsound — FIXED
|
||
|
||
**File:** `client/lib/patch-db.ts`
|
||
|
||
All `watch$` overloads claimed `NonNullable<...>`, but runtime values can be `null`/`undefined` (e.g., after a `remove` operation). Consumers skipped null checks based on the type, leading to runtime crashes.
|
||
|
||
**Fix:** Removed outer `NonNullable` wrapper from the 1-level overload return type.
|
||
|
||
#### 14. `withLatestFrom` + in-place mutation fragility — FIXED
|
||
|
||
**File:** `client/lib/patch-db.ts`
|
||
|
||
`processUpdates` mutated the cache object in place, then re-emitted it via `cache$.next(cache)`. If `source$` emitted synchronously twice before the subscriber ran, `withLatestFrom` sampled the already-mutated object reference, potentially skipping valid revisions via the stale `cache.id` check.
|
||
|
||
**Fix:** Replaced `withLatestFrom` with direct `this.cache$.value` access in the subscribe callback.
|
||
|
||
---
|
||
|
||
## Low Severity / Performance
|
||
|
||
### Rust
|
||
|
||
#### 15. `OPEN_STORES` never removes entries — FIXED
|
||
|
||
**File:** `patch-db/src/store.rs`
|
||
|
||
Entries were inserted on open but never cleaned up on close. Unbounded growth over the lifetime of a process that opens many different database files.
|
||
|
||
**Fix:** `Store::close()` now removes the entry from `OPEN_STORES`.
|
||
|
||
#### 16. `Broadcast::send` clones patches per subscriber under write lock — DEFERRED
|
||
|
||
**File:** `patch-db/src/subscriber.rs`
|
||
|
||
`revision.for_path()` is called per subscriber, cloning and filtering the entire patch. This is O(subscribers × operations) work while holding the `RwLock` write guard.
|
||
|
||
#### 17. Array diff is O(n²) — DEFERRED
|
||
|
||
**File:** `json-patch/src/diff.rs`
|
||
|
||
The `ptr_eq` scan inside the array diff loops is O(n) per element, making worst-case O(n²) for large arrays with mostly non-pointer-equal elements.
|
||
|
||
#### 18. `Store::exists` conflates null with missing — FIXED
|
||
|
||
**File:** `patch-db/src/store.rs`
|
||
|
||
A key with an explicit `Value::Null` was reported as non-existent.
|
||
|
||
**Fix:** Changed from null comparison to `.is_some()`.
|
||
|
||
### TypeScript
|
||
|
||
#### 19. `new RegExp` in hot path — FIXED
|
||
|
||
**File:** `client/lib/json-patch-lib.ts`
|
||
|
||
`arrayFromPath` and `pathFromArray` constructed new `RegExp` objects on every call.
|
||
|
||
**Fix:** Pre-compiled regex literals at module scope.
|
||
|
||
#### 20. O(watchedNodes × patchOps) redundant `arrayFromPath` — FIXED
|
||
|
||
**File:** `client/lib/patch-db.ts`
|
||
|
||
Inside `handleRevision`, `arrayFromPath(path)` was called for every `(watchedNode, patchOp)` pair.
|
||
|
||
**Fix:** Pre-convert patch operation paths once outside the loop.
|
||
|
||
#### 21. `getValueByPointer` swallows all exceptions — FIXED
|
||
|
||
**File:** `client/lib/json-patch-lib.ts`
|
||
|
||
The `catch (e) { return undefined }` masked programming errors and state corruption, making debugging very difficult.
|
||
|
||
**Fix:** Now only catches `TypeError` (from accessing properties on null/undefined), re-throws everything else.
|
||
|
||
#### 22. `throw 'unreachable'` is reachable — FIXED
|
||
|
||
**File:** `client/lib/json-patch-lib.ts`
|
||
|
||
If a patch path extended past the actual document depth (data is a primitive at a non-terminal segment), this branch executed. It threw a string with no stack trace.
|
||
|
||
**Fix:** Now throws a proper `Error` with a descriptive message.
|