fix: comprehensive audit — fix 14 critical and 8 high-severity bugs#99
fix: comprehensive audit — fix 14 critical and 8 high-severity bugs#99justrach wants to merge 37 commits into
Conversation
… GC interval jsonValue previously broke out of the loop on unterminated strings then accessed json[start..i+1] where i could equal json.len — one byte past the buffer. In ReleaseFast (no bounds checks) this is UB/segfault. Now returns early on closing quote and returns null on unterminated input. Also lowers MVCC GC_INTERVAL from 10,000 to 500 to prevent OOM during large bulk ingests — version chains now get cleaned up 20x more often. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
handleConn only had a read loop for bulk bodies > 64KB (big_buf path). For bodies <= 64KB it did a single read() and assumed TCP delivered everything — but TCP can fragment data across packets, so most of the NDJSON body was never read. This caused ~85% of bulk insert lines to be silently skipped (no key found → continue). Now small bulk requests also loop until Content-Length bytes are received, using the existing req buffer without extra allocation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eader
The old parser only handled C/c for the first character but used a
case-sensitive match for the rest ("ontent-length: "). curl and most
HTTP clients send "Content-Length" (capital L), which didn't match
"content-length" (lowercase l). This meant the bulk read loop never
triggered for bodies > 64KB — the server only processed the first
read() chunk and silently dropped the rest of the NDJSON payload.
Now matches each character of "Content-Length:" independently.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dropCollectionForTenant removed the collection from the in-memory hashmap but left the .pages file on disk. The next access to the same collection name re-opened the old file and all data reappeared, making DROP appear to do nothing. Now reconstructs the page file path and unlinks it after closing the collection, so data is actually gone. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… GC interval Cap query limits to prevent client-driven OOM: scan max 1000, search max 500, context max 100. Add buffer overflow guards in scan/search response serialization — stop writing docs when the 64KB body buffer is nearly full instead of silently truncating JSON. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Cap query limits: scan max 1000, search max 500, context max 100 - Add buffer overflow guards in scan/search/context response loops — stop serializing when the 64KB body buffer is nearly full instead of silently truncating JSON - Bulk insert now checks tenant storage quota before processing (was bypassing ensureTenantStorageAvailable entirely) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Trigram index: cap posting lists at 10K entries per trigram. Trigrams
appearing in more docs than this are too common to be discriminative
for search — skip them to prevent unbounded memory growth on large
collections (13K+ files could otherwise consume hundreds of MB).
Word index: cap hits per word at 5K. Common words ("the", "var", "if")
accumulate thousands of hits across a codebase — beyond this limit they
waste memory for negligible search value.
WAL: truncate the file after checkpoint. All data is already durable
in page files at checkpoint time, so the WAL can be safely cleared to
reclaim disk space. Prevents unbounded WAL growth on long-running
instances.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Throughput and stability under high concurrent load: - WAL: add 8MB write buffer backpressure — writers yield when buffer is full instead of growing unboundedly toward OOM - Thread stack: 256KB explicit stack size (was OS default ~2MB). 512 threads × 2MB = 1GB just for stacks; now 512 × 256KB = 128MB - Connection rejection: return HTTP 503 with JSON body instead of silent TCP RST when at MAX_CONNECTIONS - Stripe locks: 1024 → 4096 to reduce contention under 100+ concurrent writers (birthday paradox collision rate drops 4x) - Index queue: 32K → 128K capacity to absorb write bursts before falling back to synchronous indexing - Second index worker thread spawned to double indexing throughput Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clients can upgrade any HTTP connection to WebSocket by sending a
standard Upgrade: websocket request. After the 101 handshake, each
WS text message is dispatched as a request using the format:
METHOD /path\n{body}
e.g. "POST /db/mycol\n{\"key\":\"k\",\"value\":{}}"
The response body (without HTTP headers) is sent back as a WS text
frame. This enables:
- Streaming bulk inserts over one connection (bypasses Cloudflare
Workers 1000 subrequest limit)
- Lower latency (no HTTP overhead per request)
- Persistent connections for real-time operations
Implementation: SHA1+Base64 handshake, RFC 6455 frame parsing with
masking, ping/pong keepalive, 16MB max message (heap-allocated).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… GC interval Four correctness and safety fixes: CDC: workerMain captured subscriptions slice pointer then released the lock — registerWebhook could reallocate the ArrayList, dangling the pointer. Now iterates by index, re-reading under lock each iteration. WAL: backpressure yield-loop gave up after 100 retries and let writes proceed anyway, defeating the purpose. Now uses cond.wait() so writers truly block until the flusher drains below threshold. doc.zig: decode() now rejects val_len > 64MB as corrupt before using it in size arithmetic, preventing OOB slice creation from corrupted page data. Moved key_len check before the total size calculation. branch.zig: write() freed old entry before allocating new copies — if the allocation failed, the hashmap entry had dangling pointers. Now allocates first, frees second, with proper errdefer on both copies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…threads WordIndex had no synchronization. With two background index worker threads both calling words.indexFile() concurrently, the unsynchronized StringHashMap operations (getOrPut, append, put) corrupted internal state during resize — causing a panic or segfault in ReleaseSafe. This was the root cause of the vitess ingest crash (msg #41): the second index worker (spawned in commit 5190777) raced with the first on the shared WordIndex, corrupting the hashmap. Added mu: Mutex to WordIndex, acquired in indexFile() and search(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Critical fixes: - server.zig: fix overlapping memcpy in handleGet corrupting response bodies (copyForwards → copyBackwards for left-shift of overlapping regions) - server.zig: fix WebSocket ping handler not consuming payload (stream desync) - server.zig: add jsonEscape helper to prevent JSON injection in responses - wire.zig: fix read buffer overflow (rp could exceed RD_BUF after read) - wire.zig: remove cached *Collection pointer (use-after-free on drop) - codeindex.zig: fix WordIndex.search() returning dangling internal slice (now returns owned copy, searchDeduped holds lock during iteration) - collection.zig: add shared_mu protecting hash_idx/key_doc_ids/versions from concurrent access across stripe locks - codeindex.zig: cap trigram posting lists (MAX_FILES_PER_TRIGRAM=10K), per-file trigrams (MAX_TRIGRAMS_PER_FILE=8K), skip files >256KB High fixes: - hot_cache.zig: add mutex for thread-safe lookup/insert/invalidate - page.zig: add leaf_mu serializing leafAppend read-modify-write - storage/mmap.zig: add bounds assertions in at()/slice() - storage/wal.zig: hold mutex through entire checkpoint (prevent WAL corruption from concurrent writers) - storage/wal.zig: stop swallowing I/O errors in flushPending/checkpoint (add io_err_flag, log errors, don't advance synced_lsn on failure) Tests: 9 new tests for concurrency, memory caps, and trigram bounds. Issues: #50-#98 created on GitHub for full audit findings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b3f7f8bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (masked) wsReadExact(conn, &ping_mask) catch return; | ||
| // Read the ping payload so the stream stays synchronised. | ||
| const pl: usize = @intCast(ping_len); | ||
| if (pl > 0 and pl <= frame_buf.len) { | ||
| wsReadExact(conn, frame_buf[0..pl]) catch return; |
There was a problem hiding this comment.
The ping handler converts ping_len to usize before any size validation, so a malformed frame with an extended 64-bit payload length can trigger a runtime integer-cast panic (and in Zig safe builds, abort the process). This path is reachable before auth because upgrade handling runs before dispatch(), so an unauthenticated client can send a crafted ping and crash the server; validate/control-frame length (or at least cap to MAX_BULK) before @intCast and close on invalid sizes.
Useful? React with 👍 / 👎.
| if (content.len > TrigramIndex.MAX_INDEX_FILE_SIZE) return; | ||
| self.mu.lock(); | ||
| defer self.mu.unlock(); | ||
| // Clean up old entries first | ||
| self.removeFile(path); |
There was a problem hiding this comment.
The new size guard returns before removeFile(path), so reindexing an existing path that grows beyond MAX_INDEX_FILE_SIZE leaves its old word postings in place. After one normal index followed by an oversized update, search() can still return stale hits for words that are no longer in the document, which corrupts query correctness.
Useful? React with 👍 / 👎.
| if (content.len < 3 or content.len > MAX_INDEX_FILE_SIZE) { | ||
| batch[di].tris = &.{}; | ||
| continue; |
There was a problem hiding this comment.
In batch indexing, oversized documents are assigned tris = &.{} and then skipped later, but no existing postings are removed for that path. If a file was previously indexed and later exceeds MAX_INDEX_FILE_SIZE, its old trigram postings remain in file_trigrams/index, so candidate generation keeps using stale data and search behavior no longer matches the current indexing policy.
Useful? React with 👍 / 👎.
Storage layer (4 fixes): - parallel_wal: use writeAll instead of write to handle short writes - parallel_wal: add completed counter to prevent torn writes (flusher only flushes fully-written entries) - parallel_wal: remove fetchSub rollback on segment-full (eliminates race) - page: leafRead returns null instead of silently truncating Indexing subsystem (10 fixes): - codeindex: indexBatch caps at 64 entries (prevents stack overflow) - codeindex: indexBatch errdefer frees trigram slices on error - codeindex: SparseNgramIndex now dupes path strings (was use-after-free) - codeindex: SparseNgramIndex now has mutex for thread safety - codeindex: removeFileById cleans up path_to_id and id_to_path - fast_index: add mutex to FastTrigramIndex - trigram: change tombstone from 0 to maxInt(u64) (0 is valid doc_id) - disk_index: cap extractSparseNgrams loop at weights[1024] (OOB fix) - disk_index: validate mmap size in DiskIndex.open (corrupt file safety) - disk_index: add errdefer for mmap/fd cleanup on partial open failure Core features (6 fixes): - mvcc: GC rolls back on OOM instead of losing version entries - mvcc: beginRead uses cmpxchgWeak loop for atomic min_active_epoch update - auth: enabled flag is now atomic (eliminates auth bypass race window) - auth: addKey returns null when MAX_KEYS reached (was silent failure) - branch: use StringHashMap with actual key (was FNV-1a hash — collisions caused data loss) - collection: round-robin index queue assignment (prevents double-processing) CDC/query/errors (6 fixes): - server: scan/search adds "truncated":true when results exceed buffer - cdc: add events_dropped atomic counter (was silent event loss) - cdc: increase event buffers + add truncated flag - cdc: escape JSON in webhook payloads (was injection vector) - query: increase field name buffer from 256 to 1024 bytes - errors: escape detail string in JSON error responses Advanced data structures (6 fixes): - lsm: only drop tombstones at max level (prevents data resurrection) - lsm: add RwLock for flush/get synchronization (prevents torn reads) - compression: LZ4 hash table u16→u32 (fixes corruption >64KB) - art: hold write lock through prefix mismatch (closes TOCTOU window) - bwtree: epoch-based reclamation with 2-epoch grace period - turboquant: dequantize handles FWHT mode (was crash on empty slice) Replication (7 fixes): - integration: replace globals with per-instance context - integration: heap-allocate executor (was dangling ptr to optional field) - integration: bounds check in packTxnData (was stack overflow) - integration: free allocations on submit failure (was memory leak) - shard: registerPartition also updates hash ring - router: parseU16 uses checked arithmetic (was wrapping to wrong partition) - sequencer: owns_transactions flag prevents freeing stack memory Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The 256 KiB thread stack caused SIGILL crashes on macOS ARM64 in Debug/ReleaseSafe builds. Zig's stack probe instrumentation needs headroom for the deep Collection.open → PageFile.open → MmapFile.open call chain. Production builds use ReleaseFast (no probes) but dev builds need the larger stack. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42f536689d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (text_results.docs.len > out_len) { | ||
| // We can't easily shrink, but the caller will respect .docs.len | ||
| // Just return the full text_results (already re-ranked in-place) | ||
| text_results.docs = text_results.docs[0..out_len]; |
There was a problem hiding this comment.
searchText() returns an owned docs allocation that is later freed by TextSearchResult.deinit via allocator.free(self.docs). Re-slicing that owned slice to docs[0..out_len] changes its length metadata, so when hybrid search returns more than limit hits, deinit frees with the wrong size and can trip allocator safety checks (or corrupt memory in non-debug allocators). Keep ownership metadata intact and represent truncation without mutating the owned slice length.
Useful? React with 👍 / 👎.
| while (self.write_buf.items.len >= MAX_WRITE_BUF) { | ||
| // Release lock and wait for flusher to signal. | ||
| self.cond.wait(&self.mu); |
There was a problem hiding this comment.
The new backpressure loop blocks in write() while write_buf is above MAX_WRITE_BUF, but on flushPending() write errors the buffer is re-queued and io_err_flag is set without any path in write() that checks it. Under persistent I/O failure (e.g., disk full), writers can wake, re-check the same full buffer, and wait forever, causing request hangs instead of returning an error.
Useful? React with 👍 / 👎.
| self.flushing = true; | ||
| var to_write = self.write_buf; | ||
| self.write_buf = .empty; |
There was a problem hiding this comment.
checkpoint() acquires mu and immediately starts its own flush/truncate sequence without waiting for flushing to become false. commit()/flushPending() perform file I/O after releasing mu, so a checkpoint can run concurrently with an already-active flusher and truncate the WAL while that flusher is still writing, risking dropped or reordered WAL records around checkpoint boundaries.
Useful? React with 👍 / 👎.
With 20 collections: 14 MB → 6 MB RSS. - IndexQueue CAPACITY: 131K → 16K entries (saves ~3.7 MB per collection) 16K pending docs still handles bursty concurrent writes - HotCache: 16K → 4K entries (saves 192 KB per collection) CLOCK eviction keeps hot keys effective at smaller size - STRIPE_COUNT: 4096 → 256 (saves 15 KB per collection) 256 stripes is still very fine-grained concurrency - GROW_CHUNK: 256 MB → 64 MB (less pre-allocated file space) Still grows on demand, just starts smaller - MAX_RESP: 128 KB → 64 KB (saves 64 KB per connection) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reduces virtual address space pressure per collection. 1 GiB is sufficient for most workloads and reduces VSZ reporting by 75%. Physical memory (RSS) is unchanged — VA reservations cost nothing until pages are actually written. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bufPrint on the 8192-byte Delivery.payload buffer silently fell back to
"{}" on overflow, producing a 2-byte payload with an HMAC signature
computed over "{}" instead of the real event. Webhook consumers would
see corrupted payloads with invalid signatures.
Now retries without the value field (adding "truncated":true) so the
event metadata (seq, tenant, collection, op, key, doc_id) is still
delivered correctly. Signature is computed over the actual payload.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc0ba24013
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pos + 2 > buf.len) return buf[0..pos]; // truncate safely | ||
| buf[pos] = '\\'; | ||
| pos += 1; | ||
| } else { | ||
| if (pos + 1 > buf.len) return buf[0..pos]; // truncate safely |
There was a problem hiding this comment.
jsonEscape silently returns a truncated slice when the 1 KiB scratch buffer fills, so endpoints that interpolate escaped user fields (for example key/tenant/collection in insert/scan/search responses) can emit incomplete identifiers whenever those inputs are long and contain " or \. This is a correctness regression: clients can receive mismatched metadata even though the underlying write/read succeeded.
Useful? React with 👍 / 👎.
| const offset = self.pos.fetchAdd(total, .monotonic); | ||
| if (offset + total > BUF_SIZE) { | ||
| // Roll back so other threads see accurate remaining space. | ||
| _ = self.pos.fetchSub(total, .monotonic); | ||
| // Don't roll back — the segment is full. Other threads that already | ||
| // reserved valid offsets can still complete. A rollback via fetchSub | ||
| // races with concurrent fetchAdd and can corrupt offsets. |
There was a problem hiding this comment.
When append detects offset + total > BUF_SIZE, it now returns error.SegmentFull without restoring pos. A single oversized write near the end of the segment permanently advances the reservation pointer past valid space, so subsequent smaller appends fail even if they would have fit in the remaining bytes, and pos can no longer represent contiguous written data. This behavior was introduced by removing the rollback path.
Useful? React with 👍 / 👎.
justrach
force-pushed
the
fix/audit-critical-bugs
branch
from
3c7a50f to
113af7c
Compare
…rent crash Performance: - leafAppend: replace global leaf_mu with 64-way sharded locks keyed on page number. Concurrent inserts to different pages no longer serialize through a single mutex. - containsInsensitive: replace per-byte branch with compile-time 256-entry lowercase LUT. Pre-lowercase needle once, first-byte fast skip before full compare. - insert: write documents directly into page memory via leafReserve instead of encoding into a stack buffer then memcpy'ing to the page. Bug fix: - Collection.open: zero-initialize the allocated struct with @Memset before field assignment. GPA fills allocations with 0xAA in debug mode, which macOS os_unfair_lock detects as corrupted lock state, causing _os_unfair_lock_corruption_abort on concurrent access. This was the root cause of the intermittent SIGILL crash on first collection creation under concurrent load. - CDC fallback format string: fix unescaped quotes in truncated payload JSON. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
justrach
force-pushed
the
fix/audit-critical-bugs
branch
from
113af7c to
6277cfa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6277cfa367
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var lower_needle: [256]u8 = undefined; | ||
| const nlen = @min(needle.len, 256); | ||
| for (needle[0..nlen], 0..) |c, k| lower_needle[k] = lower_lut[c]; | ||
| if (needle.len > 256) return false; // needle > 256 chars unsupported |
There was a problem hiding this comment.
The new fast-path in containsInsensitive now returns false whenever needle.len > 256, which makes any long query term impossible to match even when the document actually contains it. This is a correctness regression introduced by the optimization: endpoints that rely on this helper (text search and multi-term filtering paths) will silently return missing results for long user queries instead of performing a full comparison.
Useful? React with 👍 / 👎.
…ectness - Trigger version chain GC from update/delete (not just insert) to prevent unbounded dead-version accumulation in update/delete-heavy workloads - Store current_epoch after every appendVersion so ReadTxn.end() can advance min_active_epoch and unblock GC - CDC: batch-drain pending events to eliminate O(n) orderedRemove(0) per event - Epoch timeline: cap at 100K entries (~1.6 MiB), drop oldest half when full - JSON escape: handle \r, \t, control chars, escaped quotes in jsonStr parser - Wire: close connection on oversized frame instead of silent buffer wrap - HTTP: read full Content-Length body for all requests, not just /bulk - Index worker: 1ms sleep instead of busy-spin yield on empty queue Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddb6727a68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _ = self.path_to_id.remove(path); | ||
| self.allocator.free(path); | ||
| self.id_to_path.items[file_id] = &.{}; |
There was a problem hiding this comment.
removeFileById now clears path_to_id and id_to_path, but indexFile/indexBatch call it as part of normal reindex of an existing file. That leaves fresh postings pointing at a file_id whose path mapping was erased, so candidate translation returns empty paths and later reindexes allocate new ids without cleaning old postings. In practice, updating/reinserting the same path can make search results disappear or become stale even though indexing appears to succeed.
Useful? React with 👍 / 👎.
| _ = self.key_doc_ids.remove(key_hash); | ||
| _ = self.key_epochs.remove(key_hash); |
There was a problem hiding this comment.
Removing key_doc_ids on delete breaks historical lookup paths: getAsOfEpoch and scanAsOfEpoch resolve document history by first reading key_doc_ids, so once this entry is deleted, older versions become unreachable even for epochs before the tombstone. This regresses time-travel semantics (e.g., “get survives delete”) and causes false null results for valid historical reads.
Useful? React with 👍 / 👎.
- Add spinLoopHint() to all bare CAS retry loops (bwtree insert/delete, IndexQueue push, MpscRing push, MVCC beginRead) to reduce CPU burn under contention - WAL checkpoint: release mutex before fsync/writeAll, reacquire for truncate — writers no longer block for 10s+ of ms during checkpoint - CDC: add MAX_SUBSCRIPTIONS cap (1024) and removeWebhook() to prevent unbounded subscription list growth - Trigram: compact posting lists in-place on removeDoc instead of tombstoning — eliminates gradual memory leak from dead entries Tested with 12,410 openclaw source files (70 MB): Insert: 138 docs/s, search: 23-43ms, idle CPU: 1.8% Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c894d48613
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const MAX_REQ = 65536; // 64 KiB (initial read) | ||
| const MAX_RESP = 131072; // 128 KiB | ||
| const MAX_RESP = 65536; // 64 KiB |
There was a problem hiding this comment.
Reducing MAX_RESP to exactly MAX_BODY leaves no space for HTTP headers in respond(), which writes headers and body into the same fixed buffer. Any response body near 64 KiB (e.g., large scan/search payloads) will overflow the fixed buffer, so the write is truncated while Content-Length still advertises the full body size, causing malformed/truncated responses for clients. Keep MAX_RESP larger than MAX_BODY (or cap body bytes to account for header overhead).
Useful? React with 👍 / 👎.
…ckoff - WAL txn_id race: stop pre-reading next_lsn before write() — concurrent inserts could share the same txn_id, breaking per-key isolation on recovery - scan(): hold shared_mu during page iteration to prevent reading partially written documents from concurrent inserts - Pre-allocate hash_idx/key_doc_ids/key_epochs to 4096 capacity — avoids O(n) rehash under shared_mu during insert bursts - Queue push: reduce retry cap from 1000→64 and use spinLoopHint() instead of yield() — faster fallback to sync indexing when queue is full Bench (12K openclaw files, persistent HTTP conn): Insert: 6,296 docs/s | Search: 0.5-6ms | Scan: 0.3ms Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c366d937
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ph.used_bytes += @intCast(len); | ||
| ph.doc_count += 1; | ||
| return .{ .buf = dest[used..][0..len], .off = used }; |
There was a problem hiding this comment.
leafReserve increments used_bytes and returns before the caller writes the document into buf, so concurrent readers that trust used_bytes (for example Collection.scan) can parse partially-written bytes and stop early on decode failure, yielding missing/incorrect scan results under insert+scan concurrency. The reservation should not become visible until the payload is fully encoded (or readers must synchronize on the same page-write lock).
Useful? React with 👍 / 👎.
| var pno: u32 = 0; | ||
| // Snapshot used_bytes per page under shared_mu to avoid reading | ||
| // partially-written documents from concurrent inserts. | ||
| self.shared_mu.lock(); |
There was a problem hiding this comment.
scan acquires shared_mu without defer and then executes fallible work inside the locked region; if any try in the loop returns (notably results.append on allocation failure), the function exits before reaching the manual unlock, leaving shared_mu permanently locked and blocking subsequent collection operations.
Useful? React with 👍 / 👎.
Add Collection.insertBatch() + flushBatch() for bulk inserts: - Phase 1: encode + write pages without shared_mu (page-level locking) - Phase 2: take shared_mu ONCE, insert all entries into BTree + HashMaps - Pre-grow HashMaps before locking to avoid rehash under contention - Push all docs to async trigram queue (skip sync fallback) Server bulk handler now parses NDJSON into slices, then calls insertBatch instead of per-line insert(). Eliminates N lock/unlock cycles per request. Bench (13,340 openclaw files, persistent HTTP conn): Before: 6,296 docs/s | After: 7,403 docs/s (+18%) Search: 1-15ms | Scan: 0.4ms Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcd8047089
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (i < count) : (i += 1) { | ||
| if (batch_n >= MaxBatch) { | ||
| // Flush accumulated batch to indexes. | ||
| self.flushBatch(entries[0..batch_n], doc_ids[0..batch_n], hashes[0..batch_n], keys, values, i - batch_n); |
There was a problem hiding this comment.
insertBatch derives key_offset as i - batch_n (and later count - batch_n), which only works if every processed row became a successful entries[] item. As soon as one row is skipped (invalid key/value, WAL/page encode failure), entries/doc_ids and keys/values are misaligned, so flushBatch indexes and emits CDC with the wrong key/value pairs for persisted documents (flushBatch uses key_offset + j for async indexing and emitChange). This can produce incorrect search index entries and incorrect CDC payloads for bulk inserts containing mixed valid/invalid rows.
Useful? React with 👍 / 👎.
| const needed = entries.len + self.hash_idx.count(); | ||
| self.hash_idx.ensureTotalCapacity(@intCast(needed)) catch {}; | ||
| self.key_doc_ids.ensureTotalCapacity(@intCast(needed)) catch {}; | ||
| self.key_epochs.ensureTotalCapacity(@intCast(needed)) catch {}; |
There was a problem hiding this comment.
flushBatch mutates hash_idx, key_doc_ids, and key_epochs via ensureTotalCapacity before taking shared_mu, but other collection paths rely on shared_mu to serialize access to these same maps. A concurrent get/insert/update can therefore race with this pre-growth rehash and observe/mutate unstable map internals, leading to crashes or corruption under concurrent bulk-insert traffic. Capacity growth should happen under the same mutex that protects the maps.
Useful? React with 👍 / 👎.
Eliminate double-encode (write directly to page, WAL reads from page memory), batch WAL writes under one lock, cache current leaf page to skip per-doc findOrAllocLeaf scans, and bump MaxBatch to 4096 to match server-side MaxLines. ~2.1x throughput improvement over prior commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b35cf77cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (headerContains(initial, "upgrade", "websocket")) { | ||
| handleWebSocket(srv, conn, initial) catch {}; | ||
| return; // WS handler owns the connection until close |
There was a problem hiding this comment.
This branch upgrades immediately using the already-read initial buffer but does not preserve any bytes after the HTTP headers. When a client sends the first WebSocket frame in the same TCP read as the upgrade request, those frame bytes are consumed and dropped before handleWebSocket starts wsReadExact, so the first message is lost and the stream can desynchronize.
Useful? React with 👍 / 👎.
| self.wal_log.writeBatch(0, .doc_insert, 0, 0, wal_payloads[0..batch_n]) catch {}; | ||
| self.flushBatch(entries[0..batch_n], doc_ids[0..batch_n], hashes[0..batch_n], keys, values, i - batch_n); |
There was a problem hiding this comment.
The bulk path swallows writeBatch errors and still runs flushBatch and increments inserted, so callers can receive success for writes that were never appended to WAL. Under allocator failure during WAL buffering, this acknowledges data without durable recovery records, which can be lost after a crash/restart.
Useful? React with 👍 / 👎.
| const sub_count = self.subscriptions.items.len; | ||
| self.mu.unlock(); | ||
|
|
||
| for (subs) |sub| { | ||
| if (!matches(sub, ev)) continue; | ||
| const delivery = makeDelivery(sub, ev); | ||
| self.mu.lock(); | ||
| self.deliveries.append(self.allocator, delivery) catch {}; | ||
| if (self.deliveries.items.len > 4096) { | ||
| _ = self.deliveries.orderedRemove(0); | ||
| for (batch.items) |ev| { | ||
| var si: usize = 0; |
There was a problem hiding this comment.
The worker captures sub_count once and then walks self.subscriptions by index while removals can happen via swapRemove. If a lower index is removed concurrently, another active subscription can move to an already-passed index and be skipped for the current batch (or iteration can break early), silently dropping deliveries during webhook churn.
Useful? React with 👍 / 👎.
Add simdFindNewline() using @vector(16, u8) to scan 16 bytes per iteration for newline delimiters (SSE2 on x86_64, NEON on aarch64). Replaces scalar std.mem.indexOfScalarPos in handleBulkInsert. Local benchmark: 23K docs/s on 500-doc batch, 17.6K on 5K-doc batch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdb6663e65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /// Index a file's content — tokenizes into words and records hits. | ||
| pub fn indexFile(self: *WordIndex, path: []const u8, content: []const u8) !void { | ||
| if (content.len > TrigramIndex.MAX_INDEX_FILE_SIZE) return; |
There was a problem hiding this comment.
WordIndex.indexFile now returns immediately for files above MAX_INDEX_FILE_SIZE, but that return happens before removeFile(path) runs. When an already-indexed file later exceeds the size cap, its old word hits remain in index/file_words, so word lookups can return stale matches for content that should have been de-indexed under the new policy.
Useful? React with 👍 / 👎.
| if (!q.push(owned_key, owned_val)) { | ||
| // Queue full — just skip async indexing for this doc. | ||
| // The data is still in the page store and searchable via scan. | ||
| self.alloc.free(owned_key); |
There was a problem hiding this comment.
In flushBatch, a full indexing queue causes the document’s key/value to be freed and indexing to be skipped entirely. Under bulk ingest pressure, writes are still persisted and counted as inserted, but trigram/word indexes silently fall behind and queries miss newly inserted documents. This path should retry or fall back to synchronous indexing (as single inserts do) instead of dropping index updates.
Useful? React with 👍 / 👎.
Add a streaming ingest mode over WebSocket that eliminates per-batch HTTP round-trip latency. Client sends "STREAM /db/:col/bulk", then fires NDJSON frames continuously without waiting for responses. Server processes each frame via insertBatch, sends one final summary on close. Server changes (server.zig): - Extract parseAndInsertNdjson() from handleBulkInsert for reuse - Add handleWsStream() — streaming frame loop with deferred billing - Add extractParam() helper for init message parsing - Detect "STREAM " prefix in handleWebSocket text frame handler Python client (python/turbodb/stream.py): - StreamInserter class with add()/flush()/send_ndjson() API - Context manager support for clean open/close Over high-latency connections (SSH tunnel, ~200ms RTT), this turns 7 sequential round-trips into 1 connection + fire-and-forget frames. Expected 10-20x throughput improvement for remote ingestion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeaf44e7d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const old_map_key = self.writes.getKey(key).?; | ||
| self.allocator.free(@constCast(old_map_key)); | ||
| // Remove old entry so we can insert with new owned key. | ||
| _ = self.writes.remove(key); |
There was a problem hiding this comment.
When updating an existing branch key, this code frees old_map_key and then calls self.writes.remove(key). StringHashMap.remove may still read stored keys while probing/comparing entries, so removing after freeing can read freed memory and trigger allocator corruption or crashes during repeated writes/deletes to the same branch key. Remove/fetch the map entry first, then free the returned owned key/value buffers.
Useful? React with 👍 / 👎.
| if (opcode == 0x1 or opcode == 0x2) { | ||
| srv.db.recordTenantOperation(tenant_id) catch break; | ||
| const r = parseAndInsertNdjson(col, payload); |
There was a problem hiding this comment.
This STREAM path inserts NDJSON frames directly (parseAndInsertNdjson) after only tenant-op accounting, but it never verifies an API key/auth context. Unlike HTTP routes (which pass through dispatch()'s auth gate), a client that upgrades to WebSocket can write data here even when auth is enabled, and can also choose tenant= from the init message. Add explicit auth validation (or carry authenticated handshake context) before accepting stream inserts.
Useful? React with 👍 / 👎.
Add Bulk Ingestion section with NDJSON curl examples, WebSocket streaming Python usage, and a throughput comparison table. Add bulk insert and WS streaming rows to the feature status table. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81dcab9023
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const old_cap = cur_cap; | ||
| const new_cap = alignUp(needed_len + GROW_CHUNK, OS_PAGE_ALIGN); | ||
|
|
||
| std.debug.assert(new_cap <= MAX_VA_SIZE); |
There was a problem hiding this comment.
grow() only guards the 1 GiB reservation with std.debug.assert(new_cap <= MAX_VA_SIZE), which is compiled out in ReleaseFast/ReleaseSmall. When a database grows beyond that limit (or opens already larger), the subsequent MAP_FIXED remap can extend outside the reserved VA window and overwrite unrelated mappings, causing process corruption/crashes in production. This should be a runtime error path before ftruncate/mmap, not a debug-only assert.
Useful? React with 👍 / 👎.
| if isinstance(value, (dict, list)): | ||
| value = json.dumps(value) | ||
| self._buf.append(json.dumps({"key": key, "value": value})) |
There was a problem hiding this comment.
When value is a dict/list, add() serializes it first and then serializes the outer document again, producing NDJSON like "value":"{...}" instead of "value":{...}. On the server side this gets stored as a JSON string literal rather than the original object/array, so streamed inserts have different semantics than normal client inserts for the same Python input.
Useful? React with 👍 / 👎.
| wsWriteText(conn, summary) catch {}; | ||
| wsWriteClose(conn, 1000); |
There was a problem hiding this comment.
In handleWsStream, the loop breaks on opcode 0x8 (client close) but then still sends a text summary frame before sending close. Sending data frames after a close frame is a protocol violation for many WebSocket clients, so the final summary can be dropped or treated as an error exactly when clients terminate normally with close.
Useful? React with 👍 / 👎.
The bulk insert call chain (parseAndInsertNdjson → insertBatch) was stacking ~360KB of arrays per request. With 2MB thread stacks and concurrent bulk inserts, this risks stack overflow. Reducing the outer batch to 1024 cuts stack usage to ~130KB; insertBatch handles its own internal chunking so throughput is unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b75ce01aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| pub fn deinit(self: *ReplicatedDB) void { | ||
| if (self.receiver) |*r| r.stop(); | ||
| if (self.executor_ptr) |ptr| self.alloc.destroy(ptr); |
There was a problem hiding this comment.
deinit() destroys executor_ptr immediately after r.stop(), but the replica receiver is spawned as a detached thread and PeerReceiver.stop() only flips a flag; it does not join/await in-flight handlePeerConn workers. During shutdown with active replication traffic, those workers can still execute batches through self.executor, turning this free into a use-after-free crash/corruption path.
Useful? React with 👍 / 👎.
| // Reject if at connection limit to prevent OOM from thread exhaustion. | ||
| if (self.active_conns.load(.monotonic) >= MAX_CONNECTIONS) { | ||
| // Return HTTP 503 instead of silent RST so clients can retry. | ||
| const reject = "HTTP/1.1 503 Service Unavailable\r\nContent-Length: 31\r\nConnection: close\r\n\r\n{\"error\":\"too many connections\"}"; |
There was a problem hiding this comment.
The hard-coded rejection response advertises Content-Length: 31 but the JSON body {"error":"too many connections"} is 32 bytes. Strict HTTP clients may read a truncated body (or leave one extra byte in the stream), causing malformed parsing/retry behavior under connection-limit pressure. The same literal is duplicated in both TCP and Unix accept loops.
Useful? React with 👍 / 👎.
…ency Two fixes inspired by PostgreSQL's concurrent write patterns: 1. Move ensureTotalCapacity inside shared_mu — concurrent resizes on the same HashMap from multiple threads was a data race that could corrupt internal arrays and crash the server. 2. Chunk the insertion loop into sub-batches of 64 docs, releasing shared_mu between chunks. Previously the lock was held for up to 4096 docs × 7 operations = 28K ops, completely serializing parallel writers. Now each critical section is 64 × 7 = 448 ops, letting parallel workers interleave (PostgreSQL's small-critical-section pattern). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f628ede927
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| srv.db.recordTenantOperation(tenant_id) catch break; | ||
| const r = parseAndInsertNdjson(col, payload); |
There was a problem hiding this comment.
In handleWsStream, streamed NDJSON frames are inserted after only recordTenantOperation, but there is no ensureTenantStorageAvailable gate before parseAndInsertNdjson. This creates a quota bypass for clients using STREAM over WebSocket: HTTP bulk insert enforces storage limits, while WS streaming can keep writing indefinitely and exceed tenant storage caps.
Useful? React with 👍 / 👎.
| // WebSocket frame loop — heap-allocate frame buffer (too large for stack). | ||
| const frame_buf = std.heap.page_allocator.alloc(u8, MAX_BULK) catch return error.OutOfMemory; | ||
| defer std.heap.page_allocator.free(frame_buf); |
There was a problem hiding this comment.
handleWebSocket allocates frame_buf with size MAX_BULK immediately after upgrade, and MAX_BULK is now 256 MiB. That means each WebSocket connection reserves a quarter-gigabyte even for tiny frames, so a small number of concurrent WS clients can exhaust memory and force connection failures or OOM under load.
Useful? React with 👍 / 👎.
1 participant
Summary
Full codebase security and reliability audit identified 77 bugs across the entire TurboDB codebase. This PR fixes the 22 most critical and high-severity issues across 9 files.
Critical fixes (14)
memcpycorrupting every GET response body >166 bytesjsonEscapehelper)rpexceedingRD_BUFafter read)*Collectionpointer after dropWordIndex.search()returning dangling internal ArrayList slicesearchDeduped()iterating dangling slice after lock releaseshared_muprotectinghash_idx/key_doc_ids/versionsfrom concurrent access across stripe locks (root cause of ingest crashes)MAX_FILES_PER_TRIGRAM,MAX_TRIGRAMS_PER_FILE,MAX_INDEX_FILE_SIZE)High fixes (8)
leaf_muserializingleafAppendread-modify-writeat()/slice()flushPending/checkpointRemaining issues
49 GitHub issues created (#50-#98) covering the full audit findings including medium/low bugs in replication, LSM, compression, ART, BwTree, CDC, auth, branching, and more.
Test plan
zig build test)🤖 Generated with Claude Code