69ae6e64a1
- Extract shared deferred post-filter (InCollection/MinSignal/MaxSignal/ NearLocation/SocialGraph) into query/executor/post_filter.rs so RETRIEVE and SEARCH apply identical predicates; SEARCH previously ignored four variants - Harden WAL writer/compaction/dedup paths - Brute-force vector store fixes + tests - Governance community ledger updates - Pin docker builders to bookworm (glibc/libmvec match) and add protobuf-compiler for tidal-net's tonic-build - Add M0-M10 code-review document
1755 lines
233 KiB
Markdown
1755 lines
233 KiB
Markdown
# tidalDB — M0–M10 Seven-Dimension Code Review
|
||
|
||
> **Scope:** the full committed engine across milestones M0–M10 — `tidal/`, `tidal-net/`, `tidal-server/` (~107k LOC, 318 source files).
|
||
> **Date:** 2026-06-07 · **Method:** 26 parallel zone reviewers, each applying all seven dimensions, every non-trivial finding then checked by an independent *default-refute* verifier that re-read the actual current code.
|
||
|
||
## How to read this
|
||
|
||
This is a **post-remediation re-review**: an earlier review + remediation pass already landed (shared decay kernel, atomic write batches, recovery rebuild passes, `serde_json` boundaries). Everything below was found in the **current committed code** (`git HEAD`) and survived adversarial verification — refuted/false-positive items are listed at the end for transparency.
|
||
|
||
### Seven dimensions
|
||
|
||
`Correctness & Logic` · `Concurrency & Async Safety` · `Durability & Crash Recovery` · `Error Handling & Resilience` · `Performance & Resource Management` · `Security & Input Validation` · `API Design & Maintainability`
|
||
|
||
### Severity ladder
|
||
|
||
| Severity | Meaning |
|
||
|---|---|
|
||
| **BLOCKER** | Data loss / corruption, crash on a normal path, or security hole. Fix before shipping. |
|
||
| **CRITICAL** | Serious bug hit in realistic use — wrong ranking results, a recovery gap, or resource exhaustion under normal load. |
|
||
| **WARNING** | A real issue with lower frequency/impact, a correctness smell, or a maintainability risk that bites during an incident. |
|
||
| **SUGGESTION** | A genuine improvement that is not a bug. |
|
||
|
||
## Headline
|
||
|
||
- **88** verified findings survived (of 102 non-trivial raised; **14** refuted).
|
||
- **5 BLOCKER · 20 CRITICAL · 40 WARNING · 23 downgraded-to-suggestion**, plus **31** standalone suggestions.
|
||
- Verifier verdicts on survivors: 64 CONFIRMED at the reviewer's severity, 24 DOWNGRADE (severity tightened), 0 UPGRADE.
|
||
|
||
**Findings by dimension (survivors):** Correctness 30, Durability 22, Performance 13, Concurrency 10, Error Handling 5, Api Design 5, Security 3.
|
||
|
||
## Fix-first queue
|
||
|
||
1. **[BLOCKER]** `tidal/src/governance/community_ledger.rs:403-426` — CommunityLedger::checkpoint is non-atomic (delete-then-put outside a WriteBatch) and corrupts the only durable copy of contributor state on a crash
|
||
2. **[BLOCKER]** `tidal/src/query/search/executor/pipeline.rs:161-167, 441-506` — SEARCH silently ignores MinSignal/MaxSignal/NearLocation/InCollection/SocialGraph deferred filters
|
||
3. **[BLOCKER]** `tidal/src/storage/vector/brute/mod.rs:272-298` — BruteForceIndex::deserialize panics / OOMs on corrupt or hostile index file (integer overflow + unchecked allocation)
|
||
4. **[BLOCKER]** `tidal/src/wal/writer.rs:404-411` — A single transient flush/fsync error permanently kills the writer thread and wedges all future writes
|
||
5. **[BLOCKER]** `tidal/src/wal/compaction.rs:56-80` — Online compaction deletes the actively-written WAL segment out from under the writer thread
|
||
6. **[CRITICAL]** `tidal/src/db/collections.rs:53-82` — add_to_collection / remove_from_collection mutate in-memory index BEFORE persisting, inverting the durable-first ordering used everywhere else
|
||
7. **[CRITICAL]** `tidal/src/wal/writer.rs:260-281, 402-406` — Dedup window is mutated before the batch is durably persisted, so a failed/lost batch is suppressed on retry
|
||
8. **[CRITICAL]** `tidal/src/query/executor/pipeline.rs:242-346` — OR-of-deferred-filters silently returns wrong results (no guard like signal thresholds have)
|
||
9. **[CRITICAL]** `tidal/src/query/executor/helpers.rs:138-193` — Cohort rescore ignores the profile's sort mode, silently changing result ordering
|
||
10. **[CRITICAL]** `tidal/src/query/executor/helpers.rs:145-178` — Cohort rescore silently swallows unknown-signal errors and scores 0.0
|
||
11. **[CRITICAL]** `tidal/src/query/search/executor/pipeline.rs:469-491` — OR of user-state inclusion filters (Saved/Liked/InProgress) is silently evaluated as AND
|
||
12. **[CRITICAL]** `tidal/src/ranking/executor/scoring.rs:35-115, 239-263` — read_decay_score reads Timestamp::now() instead of the query's `now`, making decay-based scoring non-reproducible and ignoring the threaded clock
|
||
13. **[CRITICAL]** `tidal/src/replication/in_process.rs:115-127` — InProcessTransport::send_segment blocks on a full bounded channel, violating the non-blocking contract and stalling the shipper
|
||
14. **[CRITICAL]** `tidal/src/db/open.rs:151-163` — Custom .wal_dir() override is validated but silently ignored for WAL placement; leader shipper then ships an empty directory
|
||
15. **[CRITICAL]** `tidal/src/db/mod.rs:452-523` — Restore loops swallow storage read errors and corrupt rows for capability tokens, governance policies, and memberships with no log or counter
|
||
16. **[CRITICAL]** `tidal/src/db/session_restore.rs:142-158` — Restored sessions get a fresh TTL lease on every restart — session expiry is not honored across crashes
|
||
17. **[CRITICAL]** `tidal/src/db/text_syncer.rs:47-62` — Text index open failure is swallowed — SEARCH silently disabled with zero operator signal
|
||
18. **[CRITICAL]** `tidal/src/db/metrics/mod.rs:241-259` — Persistent checkpoint failure from open reports healthy forever — durability blind spot
|
||
19. **[CRITICAL]** `tidal/src/signals/warm.rs:182-185, 440-445, 488-490` — windowed_count(TwentyFourHours / SevenDays) double-counts after an hour boundary that does not fire 60 minute rotations
|
||
20. **[CRITICAL]** `tidal/src/replication/crdt/hlc.rs:288-314` — Hlc::update can return a timestamp NOT strictly greater than remote, violating its documented causality guarantee
|
||
21. **[CRITICAL]** `tidal-net/src/transport.rs:146-158` — Server-loop death is indistinguishable from clean shutdown at recv_segment → follower silently stops replicating and reports success
|
||
22. **[CRITICAL]** `tidal-server/src/cluster.rs:519-554` — Cluster router has no concurrency limit or timeout — unbounded thread spawning under load
|
||
23. **[CRITICAL]** `tidal/src/signals/warm.rs:189, 460-481, 500-502` — windowed_count(ThirtyDays) double-counts after a day boundary because day rollup does not clear the hour tier
|
||
24. **[CRITICAL]** `tidal-server/src/scatter_gather.rs:458-495, 574-577, 701-704` — Coordinator max_per_creator silently under-enforces in entity-sharded topology (reads all metadata from one node)
|
||
25. **[CRITICAL]** `tidal/src/query/executor/candidate_gen.rs:46-86` — SignalRanked candidate generation does a full O(N·S) scan of the entire signal ledger per query
|
||
|
||
|
||
---
|
||
|
||
## 🟥 BLOCKERS (5)
|
||
|
||
### B1. CommunityLedger::checkpoint is non-atomic (delete-then-put outside a WriteBatch) and corrupts the only durable copy of contributor state on a crash
|
||
|
||
**governance · DURABILITY & CRASH RECOVERY** · `tidal/src/governance/community_ledger.rs:403-426` · verifier **CONFIRMED** (98)
|
||
|
||
The community ledger explicitly cannot be rebuilt from the WAL: community WAL events do not carry per-contributor identity (signals/ledger/types.rs:58 ignores _writer_agent), and CommunityLedger is absent from the WAL-replay rebuild path (state_rebuild.rs / open.rs). The Tag::Community checkpoint is therefore the SOLE durability mechanism for per-(user, writer, epoch) contributions. But checkpoint() first scans and individually `storage.delete()`s every existing row, then individually `storage.put()`s every current cell — none of it routed through the atomic `write_batch`. A crash (or write error) after some deletes complete but before/while the puts run leaves the durable snapshot partially erased: on reopen, restore() reads a snapshot missing whole contributors/cells, and those contributions are gone forever (the tombstone replay at db/mod.rs:484 only re-applies *purges*, it cannot restore lost contributions). This is silent community-ranking data loss on a normal shutdown-checkpoint that hits an I/O hiccup. The two sibling checkpoints do this correctly: cohort/checkpoint.rs commits 'a single atomic WriteBatch' and entities/co_engagement.rs:214-226 commits one `write_batch` precisely so 'a [crash] leaves it consistent'. CommunityLedger is the lone deviation.
|
||
|
||
```rust
|
||
let stale: Vec<_> = storage.scan_prefix(&prefix).flatten().map(|(k, _)| k).collect();
|
||
for k in stale {
|
||
storage.delete(&k).map_err(TidalError::from)?;
|
||
}
|
||
for cell in &self.cells {
|
||
...
|
||
storage.put(&key, &serialize_entry(entity, type_id, entry)).map_err(TidalError::from)?;
|
||
}
|
||
```
|
||
|
||
**Fix.** Build a single `WriteBatch`: stage all stale-key `batch.delete(k)` ops and all current-cell `batch.put(key, value)` ops, then commit with `storage.write_batch(batch)` so the swap from old snapshot to new is all-or-nothing — matching cohort/checkpoint.rs and co_engagement.rs. (Bonus: it is also one storage round-trip instead of N.)
|
||
|
||
> **Verification.** The cited code exists exactly as described at community_ledger.rs:403-426. The `checkpoint()` function: (1) collects stale keys via `scan_prefix` then calls `storage.delete(&k)` in a loop (lines 411-413), then calls `storage.put(...)` in a separate loop (lines 420-422) — no WriteBatch anywhere. The file imports `StorageEngine, Tag, encode_key, entity_tag_prefix` from storage (line 29) but deliberately omits `WriteBatch`. The `StorageEngine` trait does expose `write_batch` (engine.rs:45). Both sibling checkpoints use it atomically: `cohort/checkpoint.rs:81` calls `storage.write_batch(batch)` after building a single `WriteBatch`, and …
|
||
|
||
### B2. SEARCH silently ignores MinSignal/MaxSignal/NearLocation/InCollection/SocialGraph deferred filters
|
||
|
||
**query-search · CORRECTNESS & LOGIC** · `tidal/src/query/search/executor/pipeline.rs:161-167, 441-506` · verifier **CONFIRMED** (98)
|
||
|
||
The deferred filter variants MinSignal, MaxSignal, NearLocation, InCollection, and SocialGraph all evaluate to 'full universe' at the FilterEvaluator level (storage/indexes/filter/expr.rs lines 50-94 document them as no-ops there, to be applied as post-filters in later stages). The RETRIEVE pipeline applies all of them: Stage 2.2 InCollection, Stage 2.3 MinSignal/MaxSignal, Stage 2.4 NearLocation, Stage 2.5 SocialGraph (query/executor/pipeline.rs lines 242-409). The SEARCH pipeline's `execute` runs only `apply_metadata_filter` (Stage 2, where these are no-ops), `apply_creator_metadata_filter`, and `apply_user_context_filter`. `apply_user_context_filter` (lines 469-491) matches ONLY Saved/Liked/InProgress and has `_ => {}` for everything else. There is NO Stage 2.2/2.3/2.4 in SEARCH and no SocialGraph arm. Result: a well-formed `Search` with `FILTER min_signal("like", 100)`, `near_location(...)`, `in_collection(id)`, or `social_graph(u, 1)` returns items that violate the filter — over-inclusive results — with no error and no warning. The node-count guard (lines 161-167) and the negated/OR rejecters (executor pipeline lines 96-99) all pass these filters through. This is a wrong-results data leak from a normal query path; a user constraining 'items in this collection' or 'within 5km' gets unconstrained results at 3am with nothing in the logs to explain it.
|
||
|
||
```rust
|
||
// pipeline.rs execute(): Stage 2 then 2.5 only
|
||
self.apply_metadata_filter(query, &mut candidates);
|
||
self.apply_creator_metadata_filter(query, &mut candidates);
|
||
self.apply_user_context_filter(query, &mut candidates);
|
||
...
|
||
// apply_user_context_filter, the only place user-state filters run:
|
||
for uf in extract_user_state_filters(&filter_expr) {
|
||
match uf {
|
||
FilterExpr::Saved(uid) => { ... }
|
||
FilterExpr::Liked(uid) => { ... }
|
||
FilterExpr::InProgress { user_id: uid, threshold } => { ... }
|
||
_ => {} // SocialGraph (and any other) silently dropped
|
||
…
|
||
```
|
||
|
||
**Fix.** Port RETRIEVE's Stage 2.2/2.3/2.4 and the SocialGraph arm into the SEARCH pipeline (or extract a shared post-filter applier used by both executors). Wire the collection index for InCollection (query_ops.rs only wires it for WithinScope::Collection), the ledger for MinSignal/MaxSignal, items storage for NearLocation, and creator_items for SocialGraph. Until then, at minimum detect any of these variants in `combined_filter()` during SEARCH validation and return QueryError::InvalidFilter so a constraint is never silently dropped.
|
||
|
||
> **Verification.** The finding is real. In /tidal/src/query/search/executor/pipeline.rs, the SEARCH pipeline's execute() (lines 161-166) calls only apply_metadata_filter, apply_creator_metadata_filter, and apply_user_context_filter. There are no Stage 2.2 (InCollection), Stage 2.3 (MinSignal/MaxSignal), or Stage 2.4 (NearLocation) blocks. A grep of the entire tidal/src/query/search/ tree for InCollection, MinSignal, MaxSignal, NearLocation, SocialGraph, extract_collection_filters, extract_signal_threshold_filters, extract_geo_filters, and social_graph_bitmap returns zero matches — none of these deferred filter variants are handled anywhere in the SEARCH …
|
||
|
||
### B3. BruteForceIndex::deserialize panics / OOMs on corrupt or hostile index file (integer overflow + unchecked allocation)
|
||
|
||
**storage-vector · DURABILITY & CRASH RECOVERY** · `tidal/src/storage/vector/brute/mod.rs:272-298` · verifier **CONFIRMED** (97)
|
||
|
||
`count` is read directly from the file header as a u64 (line 272) and is entirely controlled by the on-disk bytes — a corrupted or torn index file, or a malicious one. Line 276 computes `expected_size = HEADER_SIZE + count * bytes_per_vector` with no overflow check. In release builds `count * bytes_per_vector` wraps, so a huge `count` can yield a tiny `expected_size` that passes the `data.len() < expected_size` truncation guard on line 277. The loop on line 287 then iterates `count` (enormous) times calling `read_u64_le`/`read_f32_le`, which index `data[offset..]` and panic with an out-of-bounds slice access as soon as `offset` passes the real buffer length. Separately, `HashMap::with_capacity(count)` on line 285 requests a multi-exabyte allocation and aborts the process. Either way, opening a damaged index file crashes the database on a recovery path — exactly the 3am scenario. The vectors are rebuildable from the entity store, but this code path turns 'derived index is corrupt' (recoverable) into 'process panics/aborts on open' (not recoverable without manual file deletion).
|
||
|
||
```rust
|
||
let count = read_u64_le(data, 9) as usize;
|
||
// Validate total size
|
||
let bytes_per_vector = 8 + dims * 4; // id (8) + floats (dims * 4)
|
||
let expected_size = HEADER_SIZE + count * bytes_per_vector;
|
||
if data.len() < expected_size { ... }
|
||
...
|
||
let mut vectors = HashMap::with_capacity(count);
|
||
```
|
||
|
||
**Fix.** Validate count against the actual buffer before allocating or looping. Use checked arithmetic: `let body = data.len().checked_sub(HEADER_SIZE)?; let bytes_per_vector = 8 + dims.checked_mul(4)?; let max_count = body / bytes_per_vector; if count > max_count { return Err(VectorError::CorruptedIndex(...)); }`. Compute `expected_size` with `checked_mul`/`checked_add` and return `CorruptedIndex` on overflow. Pass the validated count to `with_capacity` (now bounded by file size).
|
||
|
||
> **Verification.** The code at lines 272-298 of `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/storage/vector/brute/mod.rs` matches the finding exactly. `count` is read as `read_u64_le(data, 9) as usize` (line 272) with no upper-bound validation against the actual buffer length. Line 275 computes `bytes_per_vector = 8 + dims * 4` (where `dims` is validated against config but `count` is not), and line 276 computes `expected_size = HEADER_SIZE + count * bytes_per_vector` using plain `usize` arithmetic — no `checked_mul` or `checked_add`. On a 64-bit target in release mode, a large `count` wraps around and yields a small `expected_size` that passes …
|
||
|
||
### B4. Online compaction deletes the actively-written WAL segment out from under the writer thread
|
||
|
||
**wal · DURABILITY & CRASH RECOVERY** · `tidal/src/wal/compaction.rs:56-80` · verifier **CONFIRMED** (92)
|
||
|
||
compact_wal() deletes every segment whose filename first_seq is strictly less than checkpoint_seq. It is invoked from the periodic checkpoint task (src/db/state_rebuild.rs:449) and from shutdown, while the WAL writer thread is still running and appending to its current segment. The writer holds ONE active segment open and only rotates when it reaches 16 MB (segment.needs_rotation()); between rotations it appends many batches whose sequence numbers climb well past the segment's filename first_seq. As soon as the materialized checkpoint advances past that first_seq (the common case after any burst, since the checkpoint covers events that physically live in the active segment), compact_wal sees active.first_seq < checkpoint_seq and remove_file()s the live segment. On Linux the writer's open FD keeps appending to a now-unlinked inode: every post-checkpoint event written after the unlink is lost on the next open (the directory entry is gone, so reader::recover never sees it) — silent data loss of acknowledged, fsync'd writes. On Windows remove_file on an open handle errors, surfacing as a (logged, non-fatal) compaction failure and unbounded WAL growth. The compaction doc comment only special-cases first_seq == checkpoint_seq; it never accounts for a segment that STARTS before the checkpoint but still CONTAINS uncheckpointed tail events, which is exactly the active segment.
|
||
|
||
```rust
|
||
if *seg_first_seq < checkpoint_seq {
|
||
let file_size = std::fs::metadata(seg_path).map(|m| m.len()).unwrap_or(0);
|
||
std::fs::remove_file(seg_path)?; // deletes the segment the writer thread still holds open
|
||
```
|
||
|
||
**Fix.** Never delete the segment the writer is currently appending to. Track the active segment's first_seq (the writer owns it) and have compaction stop at the active segment: delete only segments strictly older than the active one (i.e. first_seq < min(checkpoint_seq, active_segment_first_seq)). Equivalently, force a rotation at/after the checkpoint and only compact segments whose entire [first_seq, last_seq] range is below checkpoint_seq. The shutdown-path compaction is already correct because it runs after wal.shutdown(); the online path in state_rebuild.rs must gain the same active-segment exclusion, ideally by routing compaction through the writer thread (like TruncateBefore) so it cannot race the live FD.
|
||
|
||
> **Verification.** The finding is real and present in the current code.
|
||
|
||
**The vulnerable path:** `state_rebuild.rs:449` calls `crate::wal::compaction::compact_wal(dir, seq)` directly — not through the writer thread's `TruncateBefore` command channel. `compact_wal` (compaction.rs:60) deletes any segment where `*seg_first_seq < checkpoint_seq`.
|
||
|
||
**The race mechanism:** The active `SegmentWriter` in the writer thread holds an open `File` handle to the segment with the smallest `first_seq` that is still being appended to. After a segment rotation (which fires when `needs_rotation()` is true at 16MB), the segment's `first_seq` is set to the batch seq at rotation …
|
||
|
||
### B5. A single transient flush/fsync error permanently kills the writer thread and wedges all future writes
|
||
|
||
**wal · ERROR HANDLING & RESILIENCE** · `tidal/src/wal/writer.rs:404-411` · verifier **CONFIRMED** (95)
|
||
|
||
In the steady-state loop, flush_batch is invoked with `?`: any error it returns propagates straight out of run_writer, terminating the writer thread for good. There is no respawn/restart anywhere (confirmed: open() spawns run_writer exactly once and nothing re-spawns it). After the thread dies, the command channel's receiver is dropped, so every subsequent WalHandle::append / WalSender::append returns WalError::SendFailed forever — and because the WAL is the source of truth for every entity/signal/relationship write, the entire database stops accepting writes permanently. The triggering error need not be permanent: a brief ENOSPC during fsync (disk fills, then operator frees space), an EINTR, or a transient NFS/network-storage blip all return Err from segment.sync()/write_batch_bytes and kill the writer. CODING_GUIDELINES section 7 classifies Storage/Durability errors as 'retry may succeed' / 'retry required', but this path converts a retryable I/O fault into a permanent outage. Worse, the events in the failed batch were already inserted into the dedup window by partition_dedup before flush_batch ran (see wal-3), so even a manual restart could silently suppress them as duplicates.
|
||
|
||
```rust
|
||
if !kept_events.is_empty() {
|
||
next_seq = flush_batch(&mut segment, config, next_seq, &kept_events, kept_replies)?;
|
||
}
|
||
|
||
if shutdown_requested {
|
||
break;
|
||
}
|
||
```
|
||
|
||
**Fix.** Do not let a flush error tear down the writer loop. flush_batch already notifies every waiting caller with the error before returning, so the loop can log the failure (tracing::error!), increment a durability-failure metric, and continue serving — letting callers retry — instead of `?`-propagating out of run_writer. If the failure is genuinely fatal (e.g. corruption), enter an explicit 'degraded/closed' state that returns a typed error to callers rather than a dropped channel, and document it. Either way the writer thread must not silently disappear on the first I/O hiccup.
|
||
|
||
> **Verification.** The code at writer.rs line 405 is exactly as quoted: `next_seq = flush_batch(&mut segment, config, next_seq, &kept_events, kept_replies)?;`. The `?` operator propagates any `WalError` returned by `flush_batch` straight out of `run_writer`, terminating the writer thread.
|
||
|
||
`flush_batch` (lines 230-252) does correctly notify all callers in the *current* batch with the error before returning it (lines 247-249), so in-flight callers are not silently abandoned. But `run_writer` exits immediately afterward — there is no retry, no loop continuation, and no respawn.
|
||
|
||
The thread is spawned exactly once in `mod.rs` line 257-259: …
|
||
|
||
|
||
---
|
||
|
||
## 🟧 CRITICAL (20)
|
||
|
||
### C1. add_to_collection / remove_from_collection mutate in-memory index BEFORE persisting, inverting the durable-first ordering used everywhere else
|
||
|
||
**db-entities-ops · DURABILITY & CRASH RECOVERY** · `tidal/src/db/collections.rs:53-82` · verifier **CONFIRMED** (98)
|
||
|
||
`add_to_collection` calls `self.collection_index.add_item(...)` to mutate the live in-memory bitmap/metadata FIRST, then reads it back and calls `persist_collection(...)`. If the storage write fails (or the process crashes after the in-memory mutation but before the durable put), the in-memory index now contains an item the durable record does not. On the next restart `rebuild_collections` reads only the durable record, so the membership is silently lost — the running process and the recovered process disagree about collection contents. This directly contradicts the sibling `create_collection` (lines 28-45), `join_community`, `define_community_policy`, and `define_cohort` paths, which all persist BEFORE registering in memory specifically so 'a crash between the two leaves the definition on disk.' `remove_from_collection` (lines 89-107) has the mirror-image bug: it removes from memory first, so a failed persist leaves the durable record still containing a removed item.
|
||
|
||
```rust
|
||
// Update in-memory index.
|
||
self.collection_index.add_item(collection_id, item_id_u32);
|
||
|
||
// Re-persist the updated collection.
|
||
let collection = self
|
||
.collection_index
|
||
.get_meta(collection_id)
|
||
.ok_or_else(|| { ... })?;
|
||
self.persist_collection(&collection)?;
|
||
```
|
||
|
||
**Fix.** Build the updated `Collection` (clone meta, push/retain item_id) and call `persist_collection(&updated)` FIRST; only mutate `self.collection_index` after the durable put returns Ok — matching the persist-before-register order in create_collection/join_community. Then the failure mode is 'durable ahead of memory' (recoverable on restart) rather than 'memory ahead of durable' (silent loss).
|
||
|
||
> **Verification.** The cited code exists at exactly the stated lines in /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/collections.rs. `add_to_collection` (line 62) calls `self.collection_index.add_item(collection_id, item_id_u32)` before calling `self.persist_collection(&collection)?` (line 74). `remove_from_collection` (lines 98-99) calls `self.collection_index.remove_item(collection_id, item_id_u32)` before `self.persist_collection` (line 103). The sibling `create_collection` (lines 40-43) does the opposite: `self.persist_collection(&collection)?` on line 40 then `self.collection_index.create(collection)` on line 43. `persist_to_items` …
|
||
|
||
### C2. Custom .wal_dir() override is validated but silently ignored for WAL placement; leader shipper then ships an empty directory
|
||
|
||
**db-lifecycle · CORRECTNESS & LOGIC** · `tidal/src/db/open.rs:151-163` · verifier **CONFIRMED** (97)
|
||
|
||
The builder exposes `.wal_dir(dir)` (builder.rs:172-176), validates that the directory exists and is writable (builder.rs:215-239), and the leader WAL shipper reads `db.config.wal_dir` to decide what to ship (builder.rs:420-424). But `open_with_schema` constructs the WAL with `WalConfig { dir: data_dir.clone(), .. }`, and `WalConfig::wal_dir()` appends a fixed `wal/` subdir (wal/config.rs:51-52), so the WAL is ALWAYS written to `{data_dir}/wal` regardless of the override. Two real failures: (a) a single-node operator who puts the WAL on a fast disk via `.wal_dir("/ssd/wal")` silently gets WAL written to `{data_dir}/wal` instead — wrong data placement, no error; (b) in Leader role the shipper points at the custom `config.wal_dir` (which the WAL never writes to) while the WAL actually lives in `{data_dir}/wal`, so the shipper ships an empty directory and replication silently transfers nothing. In the default (no-override) case `resolve_defaults` sets `wal_dir = {data_dir}/wal` so it accidentally matches; the bug only bites when the documented override is used.
|
||
|
||
```rust
|
||
let wal_config = WalConfig {
|
||
dir: data_dir.clone(),
|
||
..WalConfig::default()
|
||
};
|
||
|
||
let (wal, replayed_events, session_events) =
|
||
WalHandle::open(wal_config).map_err(...)?;
|
||
```
|
||
|
||
**Fix.** Thread the resolved `config.wal_dir` into `open_with_schema` and build `WalConfig` from it (e.g. set `dir` to the parent of the configured wal dir, or change WalConfig to take the wal directory directly), so the actual WAL location, the validated path, and the shipper's source path are one and the same. Alternatively, reject `.wal_dir()` overrides that are not `{data_dir}/wal` until they are honored, rather than silently dropping them.
|
||
|
||
> **Verification.** The bug is real and exactly as described. Three code locations confirm it:
|
||
|
||
1. `/tidal/src/db/builder.rs:173-176` — `.wal_dir(dir)` stores `dir` into `config.wal_dir`.
|
||
|
||
2. `/tidal/src/db/open.rs:153-156` — `open_with_schema` builds `WalConfig { dir: data_dir.clone(), ..WalConfig::default() }`, completely ignoring `config.wal_dir`. The WAL always uses `data_dir` as its parent.
|
||
|
||
3. `/tidal/src/wal/config.rs:51-52` — `WalConfig::wal_dir()` returns `self.dir.join("wal")`, so the actual WAL directory is always `{data_dir}/wal`.
|
||
|
||
4. `/tidal/src/db/builder.rs:420-424` — The Leader WAL shipper is constructed with `wal_dir: wal_dir.clone()` where …
|
||
|
||
### C3. Restore loops swallow storage read errors and corrupt rows for capability tokens, governance policies, and memberships with no log or counter
|
||
|
||
**db-lifecycle · DURABILITY & CRASH RECOVERY** · `tidal/src/db/mod.rs:452-523` · verifier **CONFIRMED** (97)
|
||
|
||
`scan_prefix` yields `Result<(Vec<u8>, Vec<u8>), StorageError>` (storage/iterator.rs:7-8). The membership, governance-policy, and capability-token restore loops all use `.flatten()` then `if let Some(x) = X::from_bytes(...)`, which (a) silently discards any `Err(StorageError)` item — an I/O error reading a row during recovery — and (b) silently drops any row whose deserialization returns `None`. For capability tokens and governance policies this is authorization state: a single unreadable or corrupt row means a revoked/granted capability or an active governance policy silently vanishes on restart, changing who can write to a community, with zero observability. Contrast the adjacent purge-tombstone replay (lines 481-493) which counts and logs. At the 3am-incident bar, a recovery path that silently drops auth state on a disk read error is exactly the failure that must be observable.
|
||
|
||
```rust
|
||
for (_, value) in sb.items_engine().scan_prefix(&prefix).flatten() {
|
||
if let Some(t) = crate::governance::CapabilityToken::from_bytes(&value) {
|
||
capability_registry.insert(t);
|
||
}
|
||
}
|
||
```
|
||
|
||
**Fix.** Iterate without `.flatten()`: match each item, and on `Err(e)` log `tracing::error!(error=%e, "capability restore read failed")` (and ideally abort the open or latch degraded for auth-critical tags), and on a `None` parse log a warning and increment a dropped-row counter. At minimum emit a count of successfully restored vs skipped rows like the tombstone loop does, so a silent loss of auth/governance state is visible.
|
||
|
||
> **Verification.** The cited code exists verbatim. `PrefixIterator` at `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/storage/iterator.rs:7-8` is `Box<dyn Iterator<Item = Result<(Vec<u8>, Vec<u8>), StorageError>>>`. All three auth-critical restore loops use `.flatten()` which converts `Err` items to `None` and drops them silently: membership at line 453, governance policies at line 503, capability tokens at line 518. None of the three emit any log on `Err` or on a `None` parse result. The adjacent purge-tombstone loop (lines 481-487) uses the identical `.flatten()` pattern but at least counts successfully replayed rows and emits a …
|
||
|
||
### C4. Persistent checkpoint failure from open reports healthy forever — durability blind spot
|
||
|
||
**db-metrics · DURABILITY & CRASH RECOVERY** · `tidal/src/db/metrics/mod.rs:241-259` · verifier **CONFIRMED** (97)
|
||
|
||
is_degraded() is the single source of truth for /healthz, tidaldb_health_ok, and health_check(). It folds in health_ok, checkpoint_thread_died, and checkpoint staleness — but staleness is gated on `last_ok > 0`. The periodic checkpoint loop (state_rebuild.rs:420-424) logs the error, increments checkpoint_failures_total, and CONTINUES looping; it never panics and never sets checkpoint_thread_died, so health_check()'s JoinHandle::is_finished() latch (diagnostics.rs:60-70) never fires either. If a database opens against a disk that is full / read-only / out of inodes, the FIRST checkpoint fails and every subsequent one fails, so last_checkpoint_ns stays 0 forever. is_degraded() therefore returns false, tidaldb_health_ok reads 1, and /healthz reports "ok" — indefinitely — while signal durability is completely broken and no checkpoint has ever been written. checkpoint_failures_total climbs but is rendered only, never consulted by is_degraded(). This is precisely the 'recovery that silently drops state' / 'signal durability has silently stopped' scenario the staleness machinery was built to catch, with a hole for the never-succeeded-once case.
|
||
|
||
```rust
|
||
#[cfg(feature = "metrics")]
|
||
{
|
||
let last_ok = self.last_checkpoint_ns.load(Ordering::Acquire);
|
||
if last_ok > 0 {
|
||
let age_ns = now_unix_nanos().saturating_sub(last_ok);
|
||
if age_ns > Self::CHECKPOINT_STALENESS_LIMIT_NS {
|
||
return true;
|
||
}
|
||
}
|
||
}
|
||
false
|
||
```
|
||
|
||
**Fix.** Fold the failure counter into is_degraded(): treat the never-checkpointed-but-failing case as degraded. e.g. track a checkpoint_failures threshold or an `attempted but never succeeded` condition — if checkpoint_failures_total > 0 AND last_checkpoint_ns == 0 (no checkpoint ever succeeded), report degraded. Alternatively, record the database-open time as an implicit `last_ok` floor so staleness is measured from open even before the first checkpoint, so a DB that has been up longer than CHECKPOINT_STALENESS_LIMIT_NS with zero successful checkpoints flips to degraded. The failure counter currently has zero effect on health — that must change.
|
||
|
||
> **Verification.** The code confirms every step of the claimed mechanism:
|
||
|
||
1. `MetricsState::is_degraded()` (metrics/mod.rs:241-259): the staleness check at line 251 is `if last_ok > 0 { ... }`. When `last_checkpoint_ns` is 0 (never succeeded), the entire staleness branch is skipped and the function returns `false`.
|
||
|
||
2. `MetricsState::new()` (metrics/mod.rs:161): `last_checkpoint_ns: AtomicU64::new(0)` — starts at zero with no floor.
|
||
|
||
3. The checkpoint loop (state_rebuild.rs:420-441): on `Err(e)`, it logs and does `metrics.checkpoint_failures_total.fetch_add(1, Ordering::Relaxed)` only. The `last_checkpoint_ns` store at line 440 is inside the `else` branch — …
|
||
|
||
### C5. Text index open failure is swallowed — SEARCH silently disabled with zero operator signal
|
||
|
||
**db-query-signals · ERROR HANDLING & RESILIENCE** · `tidal/src/db/text_syncer.rs:47-62` · verifier **CONFIRMED** (97)
|
||
|
||
When `TextIndex::open(...)` fails on a persistent index (corrupted index dir, permission error, disk full, a stale Tantivy lock from a prior crash), the error `e` is discarded by the bare `Err(_) =>` arm and a bundle with `index: None` is returned. The database then opens successfully, but full-text SEARCH permanently returns nothing for the lifetime of the process — and there is no `tracing::warn!`/`error!`, no propagation, nothing. CODING_GUIDELINES §11 says 'Use tracing::error! ... Never swallow errors' and §4 forbids swallowed errors via `.ok()`/discarded `Result`. For a node where Tantivy is a derived view that should be rebuildable, the operator must at minimum be told it failed to open. At 3am during an incident, 'search returns empty' with no log is exactly the failure this guideline exists to prevent.
|
||
|
||
```rust
|
||
let index = match index {
|
||
Ok(idx) => Arc::new(idx),
|
||
Err(_) => {
|
||
return TextSyncerBundle {
|
||
index: None,
|
||
write_tx: None,
|
||
flush_tx: None,
|
||
thread: std::sync::Mutex::new(None),
|
||
};
|
||
}
|
||
};
|
||
```
|
||
|
||
**Fix.** Log the dropped error loudly before degrading: `Err(e) => { tracing::error!(error = %e, index = index_name, "text index failed to open; full-text SEARCH disabled for this process"); return TextSyncerBundle { index: None, .. }; }`. Given Tantivy is rebuildable from the entity store, prefer surfacing the failure (or attempting a one-shot rebuild) rather than silently running with text search off.
|
||
|
||
> **Verification.** The code at lines 52-62 of tidal/src/db/text_syncer.rs contains exactly the pattern quoted: `Err(_) => { return TextSyncerBundle { index: None, write_tx: None, flush_tx: None, thread: std::sync::Mutex::new(None) }; }`. The error is bound to `_` and completely discarded — no tracing call of any level precedes the return. CODING_GUIDELINES line 345 states "Use `tracing::error!` for `TidalError::Internal` (a bug occurred), `tracing::warn!` for recoverable degradation". An open failure on a persistent Tantivy index (stale lock from prior crash, permission error, disk full) falls squarely in the recoverable-degradation category and must log at …
|
||
|
||
### C6. Restored sessions get a fresh TTL lease on every restart — session expiry is not honored across crashes
|
||
|
||
**db-recovery · CORRECTNESS & LOGIC** · `tidal/src/db/session_restore.rs:142-158` · verifier **CONFIRMED** (97)
|
||
|
||
When an active session is restored from the WAL, its `started_at` is set to `Instant::now()` with the comment 'We lost the exact monotonic Instant -- approximate with now'. But both the TTL sweeper (`sweep_expired_sessions`, sweeper.rs:26 `now.duration_since(state.started_at)`) and the per-signal policy check (`PolicyEvaluator::check`, policy.rs:75) compute elapsed session age exclusively from this monotonic `started_at`. A session that was open for 23h under a 24h `max_session_duration` is handed a fresh full 24h lease after every restart. A routine restart cycle (or a crash loop) lets a session live indefinitely and never be reaped, defeating `max_session_duration` entirely. The wall-clock anchor `started_at_ns` IS preserved on the restored state (line 144) and is exactly what expiry should use for restored sessions, but it is never consulted by the expiry paths.
|
||
|
||
```rust
|
||
// We lost the exact monotonic Instant -- approximate with "now".
|
||
started_at: std::time::Instant::now(),
|
||
started_at_ns,
|
||
```
|
||
|
||
**Fix.** Make session expiry use the durable wall-clock anchor for restored sessions. Either (a) in `sweep_expired_sessions`/`PolicyEvaluator::check`, when a session was restored, compute elapsed from `started_at_ns` against `Timestamp::now().as_nanos()` instead of the monotonic Instant; or (b) on restore, back-date `started_at` by `now_ns.saturating_sub(started_at_ns)` (i.e. `Instant::now() - elapsed_since_start_ns`) so the existing monotonic math reflects the true age. Option (b) keeps one expiry code path.
|
||
|
||
> **Verification.** The cited code is exactly as described. In `session_restore.rs` lines 147-149, restored sessions get `started_at: std::time::Instant::now()` with the comment "We lost the exact monotonic Instant -- approximate with 'now'", while `started_at_ns` (the durable wall-clock anchor) is preserved in the struct.
|
||
|
||
Both expiry paths use only `started_at` (the reset Instant):
|
||
- `sweeper.rs` line 20: `let elapsed = now.duration_since(state.started_at);` — the sweeper's own comment (lines 93-95) says it reaps "sessions that expired while the process was down" at startup, but this logic is defeated because `started_at` was just set to now on restore, so …
|
||
|
||
### C7. Server-loop death is indistinguishable from clean shutdown at recv_segment → follower silently stops replicating and reports success
|
||
|
||
**net · DURABILITY & CRASH RECOVERY** · `tidal-net/src/transport.rs:146-158` · verifier **CONFIRMED** (95)
|
||
|
||
recv_segment does block_on(rx.recv()). The ONLY mpsc::Sender (inbound_tx) is owned by the spawned serve task (start_server moves it into WalShippingService). If that serve loop terminates on its own — listener death, TLS handshake task panic, reactor teardown — the sender is dropped and rx.recv() returns None for the rest of the process lifetime. The follower-side receiver thread (spawn_receiver in receiver.rs:82-85) treats None as 'transport closed / shutdown' and returns Ok(()), exiting cleanly. The result: the gRPC receive side dies, every subsequent segment from the leader is silently dropped, the follower's SegmentReceiverHandle::join() reports success, and the replica diverges with no error surfaced. server_terminated() exists to observe this (transport.rs:113) but nothing on the recv path consults it, so None collapses 'we asked to stop' and 'the server crashed under us' into the same benign signal. At 3am this is the worst kind of failure: a quiet replica that thinks it is healthy.
|
||
|
||
```rust
|
||
let mut rx = match self.inbound_rx.lock() { ... };
|
||
self.runtime().block_on(rx.recv()) // None on BOTH shutdown and server-task death
|
||
```
|
||
|
||
**Fix.** Distinguish the two cases. Hold a sentinel that is only dropped on intentional Drop (e.g. keep a clone of inbound_tx in the GrpcTransport that is dropped in Drop, or check server_handle.is_finished() when recv returns None). When recv() returns None AND the transport was not asked to shut down, return a distinct error/condition so spawn_receiver surfaces WalError::Corruption-or-equivalent and the operator sees a halted (not cleanly-stopped) replica, rather than Ok(()).
|
||
|
||
> **Verification.** The mechanism is real and confirmed by reading the actual code:
|
||
|
||
1. `transport.rs:78` creates the channel: `let (inbound_tx, inbound_rx) = mpsc::channel(config.channel_capacity);`
|
||
2. `transport.rs:83` passes `inbound_tx` into `server::start_server(&config, inbound_tx)?` — this is the only move; `GrpcTransport` retains **no clone** of `inbound_tx`. The struct definition at line 31–40 has no `inbound_tx` field.
|
||
3. `server.rs:138` moves `inbound_tx` into `WalShippingService::new(inbound_tx, ...)`, which is placed inside a tokio task spawned at line 169. When that task exits for any reason, `WalShippingService` drops and with it `inbound_tx`.
|
||
4. …
|
||
|
||
### C8. OR-of-deferred-filters silently returns wrong results (no guard like signal thresholds have)
|
||
|
||
**query-executor · CORRECTNESS & LOGIC** · `tidal/src/query/executor/pipeline.rs:242-346` · verifier **CONFIRMED** (97)
|
||
|
||
Deferred post-filters (InCollection, NearLocation, Saved, Liked, InProgress, SocialGraph) are evaluated in two phases: Stage 2's FilterEvaluator returns the FULL UNIVERSE for them (evaluator.rs:86-98), and a Stage 2.x post-filter (`retain`) applies the real predicate. The post-filter extractors (`extract_collection_filters`, `extract_geo_filters`, `extract_user_state_filters`) recurse into BOTH `And` AND `Or` nodes and then apply every collected predicate conjunctively via sequential `retain`. This is the EXACT bug that `reject_or_of_signal_thresholds` was added to prevent for MinSignal/MaxSignal — but there is NO equivalent guard for InCollection/geo/Saved/Liked/InProgress/SocialGraph. Concrete failure: `Or(InCollection(a), CategoryEq("jazz"))` — Stage 2 computes `universe ∪ jazz = universe` (keeps everything), then Stage 2.2 retains only collection-a members. The jazz branch is silently dropped; the user gets only collection-a items with no warning. Likewise `Or(InCollection(a), InCollection(b))` is applied as intersection (items in BOTH) instead of union (EITHER). Silent wrong results on a well-formed query.
|
||
|
||
```rust
|
||
// pipeline.rs Stage 2.2
|
||
let coll_filters = user_filter::extract_collection_filters(filter_expr);
|
||
...
|
||
for cf in &coll_filters {
|
||
if let FilterExpr::InCollection(cid) = cf {
|
||
if let Some(bm) = coll_idx.bitmap(*cid) {
|
||
candidates.retain(|id| bm.contains(id.as_u64() as u32));
|
||
|
||
// user_filter.rs collect_collection_filters recurses into Or:
|
||
FilterExpr::And(children) | FilterExpr::Or(children) => {
|
||
for child in children { collect_collection_filters(child, out); }
|
||
}
|
||
```
|
||
|
||
**Fix.** Extend the up-front rejection to all deferred post-filters under OR, not just signal thresholds. Generalize `reject_or_of_signal_thresholds` into `reject_or_of_deferred_filters` that errors on any `Or` subtree containing a variant for which `is_deferred_post_filter` is true (InCollection, NearLocation, Saved, Liked, InProgress, SocialGraph included), and call it alongside the existing rejections at pipeline.rs:58-61. Until polarity/OR-aware deferred evaluation exists, failing loud is the only correct option — the same rationale already documented for thresholds.
|
||
|
||
> **Verification.** The mechanism is exactly as described. In `evaluator.rs` lines 86-98, all deferred post-filters (InCollection, NearLocation, Saved, Liked, InProgress, SocialGraph, MinSignal, MaxSignal) return `self.universe.clone()` in Stage 2. In `pipeline.rs` lines 58-61, the only up-front rejection guards are `reject_negated_deferred_filters` and `reject_or_of_signal_thresholds` — no equivalent guard exists for InCollection, NearLocation, Saved, Liked, InProgress, or SocialGraph under OR. In `user_filter.rs` `collect_collection_filters` (line 257), the extractor recurses into `FilterExpr::And(children) | FilterExpr::Or(children)` identically, flattening …
|
||
|
||
### C9. Cohort rescore ignores the profile's sort mode, silently changing result ordering
|
||
|
||
**query-executor · CORRECTNESS & LOGIC** · `tidal/src/query/executor/helpers.rs:138-193` · verifier **CONFIRMED** (97)
|
||
|
||
`rescore_with_cohort` recomputes each candidate's score purely as a sum of `boost.weight * cohort_value` over `profile.boosts`, overwrites `candidate.score`, then re-sorts strictly by descending score. It never consults `profile.sort`. The main scorer (`ranking/executor/scoring.rs:42-112`) maps each `Sort` variant to a dedicated scoring function — `Sort::AlphabeticalAsc`, `Sort::DateSaved`, `Sort::New`, `Sort::Hot`, `Sort::Shortest`, etc. So for any cohort query whose profile declares an explicit non-boost sort, activating a cohort silently replaces the declared ordering (e.g. alphabetical, date-saved, newest) with boost-sum-descending. This directly contradicts the documented contract in mod.rs:422-424: "The profile's boosts and sort mode are the same, but signal values come from cohort-specific state." The sort mode is NOT the same. It also ignores `profile.gates`/excludes against cohort values, but the sort divergence is the concrete wrong-ordering bug a user will observe.
|
||
|
||
```rust
|
||
for candidate in scored.iter_mut() {
|
||
let mut cohort_score = 0.0;
|
||
for boost in boosts { ... cohort_score += value * boost.weight; }
|
||
candidate.score = cohort_score; // profile.sort never consulted
|
||
}
|
||
...
|
||
scored.sort_by(|a, b| b.score.partial_cmp(&a.score).unwrap_or(std::cmp::Ordering::Equal));
|
||
```
|
||
|
||
**Fix.** Honor `profile.sort` in the cohort path: when `profile.sort` is `Some(non-boost variant)`, the cohort rescore must reproduce that sort's scoring (or skip rescoring and only substitute cohort signal values into the same scoring function). At minimum, gate cohort rescoring to boost-driven profiles (sort == None or a boost-compatible sort) and return a `QueryError`/warning when a cohort is combined with an incompatible explicit sort, rather than silently reordering. Align the mod.rs:422-424 comment with whatever behavior is chosen.
|
||
|
||
> **Verification.** The bug is real and exactly as described. In `/tidal/src/query/executor/helpers.rs` lines 138-193, `rescore_with_cohort` accepts only `boosts: &[crate::ranking::profile::Boost]` — `profile.sort` is never passed and never consulted. For every candidate it computes `cohort_score = sum(value * boost.weight)`, overwrites `candidate.score = cohort_score`, then re-sorts strictly by descending score (lines 188-192). The call site at `mod.rs:462` is `self.rescore_with_cohort(&mut scored, cohort_name, cohort_ledger, &profile.boosts)` — only boosts are forwarded, not the sort. The main scorer in `ranking/executor/scoring.rs:35-113` dispatches to …
|
||
|
||
### C10. SignalRanked candidate generation does a full O(N·S) scan of the entire signal ledger per query
|
||
|
||
**query-executor · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/query/executor/candidate_gen.rs:46-86` · verifier **DOWNGRADE** (82) · _severity downgraded from CRITICAL by verifier_
|
||
|
||
`signal_ranked_candidates` iterates `ledger.entries()` — which returns a reference to the full `DashMap<(EntityId, SignalTypeId), EntitySignalEntry>` covering every entity and every signal type in the database (ledger/core.rs:29,416) — for every signal-ranked RETRIEVE. For each matching entry it calls `current_score` (an `exp()` decay read) and pushes onto an unbounded `Vec`. This is an O(N·S) full scan on the candidate-generation hot path, executed before any limit/degradation cap takes effect (the `reduces_candidates` truncation in pipeline.rs:169-172 runs in Stage 1 only on the returned vec, after the full scan already happened). CODING_GUIDELINES §1 and §6 explicitly forbid full scans on hot paths and require graceful degradation under load. SignalRanked is the strategy behind trending/popular profiles — the highest-traffic queries — so this is the worst place for an O(N) scan. At 10M items (the stated single-node target) this scans 10M+ DashMap entries per query.
|
||
|
||
```rust
|
||
for entry in ledger.entries() {
|
||
let (entity_id, signal_type_id) = entry.key();
|
||
if *signal_type_id == type_id {
|
||
let score = entry.value().hot.current_score(0, now_ns, 0.0);
|
||
scored.push((*entity_id, score));
|
||
}
|
||
}
|
||
```
|
||
|
||
**Fix.** Maintain a per-signal-type top-K index (e.g. a periodically-refreshed sorted structure, or a per-signal max-heap kept on signal write) so candidate generation reads the top `max_candidates` without scanning every entity. At minimum, respect the degradation level here (skip/coarsen under load) and bound the working `Vec` so a single signal-ranked query can't allocate one entry per entity in the database.
|
||
|
||
> **Verification.** The O(N·S) full scan claim is real and confirmed. `signal_ranked_candidates` at candidate_gen.rs:59-69 calls `ledger.entries()` which returns `&DashMap<(EntityId, SignalTypeId), EntitySignalEntry>` (ledger/core.rs:416-417 — `pub const fn entries(&self) -> &DashMap<(EntityId, SignalTypeId), EntitySignalEntry>`), iterates every entry, and filters by `signal_type_id` in the hot path. There is no per-signal-type secondary index. The pipeline.rs:169-172 `reduces_candidates` truncation fires after `signal_ranked_candidates` returns, not inside it — so it does not bound the scan.
|
||
|
||
However, the reviewer's claim that "a single signal-ranked query …
|
||
|
||
### C11. Cohort rescore silently swallows unknown-signal errors and scores 0.0
|
||
|
||
**query-retrieve · CORRECTNESS & LOGIC** · `tidal/src/query/executor/helpers.rs:145-178` · verifier **CONFIRMED** (97)
|
||
|
||
When a named/ad-hoc cohort is active, rescore_with_cohort recomputes each candidate's score from the profile's boosts using the cohort ledger. Every cohort read is collapsed with `.unwrap_or(...)`: `read_windowed_count(...).unwrap_or(0)`, `read_velocity(...).unwrap_or(0.0)`, and `read_decay_score(...).unwrap_or(None).unwrap_or(0.0)`. All three cohort-ledger functions return `Err(TidalError::Schema(UnknownSignalType))` when the signal name is not in the cohort ledger's schema (see tidal/src/cohort/ledger.rs:139-188). That error is discarded and the term contributes 0.0. This directly contradicts the project's stated invariant (CODING_GUIDELINES and ranking/executor/helpers.rs read_agg: 'a profile that slipped a typo'd signal name past registration fails loud instead of silently scoring 0.0'). The non-cohort path (Stage 2.3 signal thresholds at pipeline.rs:280-292) correctly maps read_agg errors to QueryError; the cohort path does not. Result: a cohort query using a profile whose boost references a signal absent from the cohort ledger returns a plausible-but-wrong ranking (all candidates rescored to a constant, then normalized to all-1.0) with no error or warning — exactly the silent-wrong-results failure the guidelines forbid.
|
||
|
||
```rust
|
||
crate::ranking::profile::SignalAgg::DecayScore => cohort_ledger
|
||
.read_decay_score(cohort_name, candidate.entity_id, &boost.signal, 0)
|
||
.unwrap_or(None)
|
||
.unwrap_or(0.0),
|
||
_ => 0.0,
|
||
...
|
||
candidate.score = cohort_score;
|
||
```
|
||
|
||
**Fix.** Make rescore_with_cohort fallible: return Result<(), QueryError> and propagate the schema error (map UnknownSignalType to QueryError::InvalidFilter naming the cohort + signal), matching the Stage 2.3 / read_agg contract. At minimum, distinguish Err (unknown signal -> fail or warn loudly) from Ok(None)/missing-data (-> 0.0) instead of folding both into 0.0.
|
||
|
||
> **Verification.** The cited code at helpers.rs:138-193 (`rescore_with_cohort`) is real and exactly matches the claim. The function signature is `pub(super) fn rescore_with_cohort(...) { ... }` — infallible, returns `()`. All three cohort ledger calls use `unwrap_or` to swallow errors:
|
||
|
||
- Line 151-158: `cohort_ledger.read_windowed_count(...).unwrap_or(0) as f64` — `read_windowed_count` returns `crate::Result<u64>`, and `Err(TidalError::Schema(UnknownSignalType))` is silently mapped to 0.
|
||
- Lines 161-168: `cohort_ledger.read_velocity(...).unwrap_or(0.0)` — same pattern, `crate::Result<f64>`, error → 0.0.
|
||
- Lines 169-172: …
|
||
|
||
### C12. OR of user-state inclusion filters (Saved/Liked/InProgress) is silently evaluated as AND
|
||
|
||
**query-search · CORRECTNESS & LOGIC** · `tidal/src/query/search/executor/pipeline.rs:469-491` · verifier **CONFIRMED** (97)
|
||
|
||
`extract_user_state_filters` (executor_helpers.rs lines 19-42) recurses into BOTH And and Or children identically, flattening every Saved/Liked/InProgress node into one list. `apply_user_context_filter` then applies each via a sequential `Self::retain_included` / `candidates.retain(...)`, which composes them conjunctively (intersection). So `Or([Saved(u), Liked(u)])` returns items that are saved AND liked, not saved OR liked — the opposite of the requested set, silently and with no warning. This is exactly the failure mode that `reject_or_of_signal_thresholds` (user_filter.rs lines 172-209) was written to prevent for MinSignal/MaxSignal, but there is no analogous guard for user-state inclusion filters in the SEARCH path (nor RETRIEVE). A FILTER expressing 'saved or liked' quietly under-returns.
|
||
|
||
```rust
|
||
// executor_helpers.rs collect_user_state_filters: Or treated like And
|
||
FilterExpr::And(children) | FilterExpr::Or(children) => {
|
||
for child in children { collect_user_state_filters(child, out); }
|
||
}
|
||
// pipeline.rs apply_user_context_filter: each applied as a separate retain (AND)
|
||
for uf in extract_user_state_filters(&filter_expr) {
|
||
match uf {
|
||
FilterExpr::Saved(uid) => { let saved = ...; Self::retain_included(candidates, &saved); }
|
||
FilterExpr::Liked(uid) => { let liked = ...; Self::retain_included(candidates, &liked); }
|
||
```
|
||
|
||
**Fix.** Add a `reject_or_of_user_state_filters` guard mirroring `reject_or_of_signal_thresholds`, called alongside the other rejecters at the top of `execute` (lines 96-99), so an OR subtree containing a Saved/Liked/InProgress/SocialGraph node returns QueryError::InvalidFilter instead of silently collapsing to AND. (Or implement true OR composition via union of the per-variant bitmaps.)
|
||
|
||
> **Verification.** All three parts of the mechanism are present in the current code exactly as described:
|
||
|
||
1. `collect_user_state_filters` in `/tidal/src/query/search/executor_helpers.rs` line 24: `FilterExpr::And(children) | FilterExpr::Or(children) => { for child in children { collect_user_state_filters(child, out); } }` — Or children are recursed identically to And children, so all Saved/Liked/InProgress nodes are flattened into a flat list regardless of whether they were under Or or And.
|
||
|
||
2. `apply_user_context_filter` in `/tidal/src/query/search/executor/pipeline.rs` lines 469-491: the loop `for uf in extract_user_state_filters(&filter_expr)` applies each …
|
||
|
||
### C13. read_decay_score reads Timestamp::now() instead of the query's `now`, making decay-based scoring non-reproducible and ignoring the threaded clock
|
||
|
||
**ranking-core · CONCURRENCY & ASYNC SAFETY** · `tidal/src/ranking/executor/scoring.rs:35-115, 239-263` · verifier **CONFIRMED** (97)
|
||
|
||
The entire scoring pipeline threads a `now: Timestamp` argument for decay calculations (score(), compute_raw_score(), score_by_sort() all carry it). But every `DecayScore` read bottoms out in `SignalLedger::read_decay_score`, which internally calls `Timestamp::now()` (core.rs:224) and ignores the caller's `now`. So `score_hidden_gems` (completion DecayScore), `for_you`/`related`/`search` boosts (view/like DecayScore), `LiveViewerCount`, and every `ProfileDecay` factor age the running score to wall-clock-at-read, NOT to the query timestamp the rest of the pipeline uses. Two consequences at 3am: (a) ranking is non-deterministic for a fixed input — replaying the same query a millisecond later yields different decay scores, breaking the spec's reproducibility contract and any property test that scores against a fixed `now`; (b) windowed counts (read_windowed_count) and decay scores are computed against DIFFERENT clocks within a single candidate's score, so a `for_you` candidate's `view DecayScore` and `view Velocity` boosts are internally inconsistent in time. The `now` parameter threaded through scoring is a lie for the decay path.
|
||
|
||
```rust
|
||
scoring.rs score_by_sort signature: `sort: Option<&Sort>, now: Timestamp` — but read_decay_score is called with no clock; in core.rs:224 `let now_ns = Timestamp::now().as_nanos();` ... `entry.hot.current_score(decay_rate_idx, now_ns, lambda)`. The executor's `now` never reaches it.
|
||
```
|
||
|
||
**Fix.** Plumb the query `now` into a clock-parameterized decay read (e.g. `read_decay_score_at(entity, signal, idx, now)`), and have `read_agg`/`read_agg_for_sort` pass `now` through for the `DecayScore` arm. The wall-clock read should exist only as a thin convenience wrapper, not the scoring path. Until then, decay-based ranking cannot be deterministically tested or reasoned about.
|
||
|
||
> **Verification.** The finding is accurate. In `/tidal/src/signals/ledger/core.rs` at line 224, `read_decay_score` (signature: `fn read_decay_score(&self, entity_id, signal_type_name, decay_rate_idx)`) has no `now` parameter and internally calls `let now_ns = Timestamp::now().as_nanos();`. This is the only clock used for decay aging (`entry.hot.current_score(decay_rate_idx, now_ns, lambda)` at line 235-239).
|
||
|
||
The chain is: `helpers.rs:69-71` — `SignalAgg::DecayScore => Ok(ledger.read_decay_score(entity_id, signal, 0)?.unwrap_or(0.0))` — no `now` is passed. This `read_agg` function is called by `read_agg_for_sort`, which in turn is called from:
|
||
|
||
1. …
|
||
|
||
### C14. InProcessTransport::send_segment blocks on a full bounded channel, violating the non-blocking contract and stalling the shipper
|
||
|
||
**replication-core · CONCURRENCY & ASYNC SAFETY** · `tidal/src/replication/in_process.rs:115-127` · verifier **CONFIRMED** (97)
|
||
|
||
The Transport trait documents send_segment as non-blocking best-effort delivery (transport.rs:79 "send_segment is non-blocking (best-effort delivery)"), and the shipper depends on this: it treats TransportError::Closed as transient follower backpressure and retries the same seqno next poll (shipper.rs:215-226, comment at 217 explicitly says "follower backpressure"). But InProcessTransport uses crossbeam's BLOCKING bounded send. When a receiver lags and the 64-slot channel (in_process.rs:89) fills, sender.send(payload) blocks the shipper poll thread indefinitely instead of returning an error. The sibling SessionShardTransport for the identical channel wiring correctly uses try_send (session_bridge.rs:189). This is an inconsistent, contract-violating implementation that can deadlock replication under backpressure and defeats the shipper's whole gap-free retry design.
|
||
|
||
```rust
|
||
sender.send(payload).map_err(|_| TransportError::Closed)
|
||
```
|
||
|
||
**Fix.** Use the non-blocking variant to honor the documented contract: `sender.try_send(payload).map_err(|e| match e { crossbeam::channel::TrySendError::Full(_) => TransportError::Closed, crossbeam::channel::TrySendError::Disconnected(_) => TransportError::Closed })` (or a dedicated backpressure variant). Mirror the pattern already used in session_bridge.rs:189 so a full follower buffer surfaces as transient backpressure the shipper re-ships next poll, never a blocked thread.
|
||
|
||
> **Verification.** The cited code is present and the bug is real.
|
||
|
||
`/tidal/src/replication/in_process.rs` line 126: `sender.send(payload).map_err(|_| TransportError::Closed)` — `crossbeam::channel::Sender::send()` on a bounded channel BLOCKS the calling thread when the channel is full; it only returns `Err` when the channel is **disconnected** (receiver dropped), not when full.
|
||
|
||
The `Transport` trait at `/tidal/src/replication/transport.rs` lines 72-73 documents the contract explicitly: `send_segment is non-blocking (best-effort delivery)`. The `InProcessTransport` violates this.
|
||
|
||
The shipper at `/tidal/src/replication/shipper.rs` lines 215-226 depends on the …
|
||
|
||
### C15. Hlc::update can return a timestamp NOT strictly greater than remote, violating its documented causality guarantee
|
||
|
||
**replication-crdt · CORRECTNESS & LOGIC** · `tidal/src/replication/crdt/hlc.rs:288-314` · verifier **CONFIRMED** (95)
|
||
|
||
The doc comment (lines 266-272) promises: "Advances the local clock to be causally after `remote` ... any subsequent `now()` call produces a timestamp strictly greater than `remote`. Returns the new local timestamp (which is strictly > `remote`)." That guarantee is false when the remote logical counter is at or above LOGICAL_MAX. Walk the `pt == remote_ms` branch (line 295): `new_logical = ((remote_logical + 1).min(LOGICAL_MAX))`. If `remote.logical >= 65535`, then `remote_logical = 65535 = LOGICAL_MAX` (truncated at line 278), so `new_logical = (65536).min(65535) = LOGICAL_MAX`. The returned timestamp is `(remote_ms_ns, LOGICAL_MAX, self.node_id)`. Compared against remote `(remote_ms_ns, LOGICAL_MAX, remote.node_id)` via HlcTimestamp::cmp (wall, then logical, then node_id), if `self.node_id < remote.node_id` the result is `updated < remote` — strictly LESS than remote. Unlike `now()`, which carries the overflow into the wall component (lines 226-231) to stay strictly monotonic, `update()` has NO overflow-carry handling; it silently clamps. This breaks the happens-before relation the HLC exists to preserve: a node that ingests a remote event then issues a local write can produce a timestamp that orders before the event it causally follows, corrupting LWW conflict resolution (the very thing the HLC feeds).
|
||
|
||
```rust
|
||
} else if pt == remote_ms {
|
||
// Remote wall is highest -- advance past remote logical.
|
||
((u64::from(remote_logical) + 1).min(LOGICAL_MAX)) as u16
|
||
}
|
||
// ...
|
||
return HlcTimestamp { wall_ns: wall_ms_to_ns(pt), logical: u32::from(new_logical), node_id: self.node_id };
|
||
```
|
||
|
||
**Fix.** Mirror the overflow-carry logic from `now()`: when the computed `new_logical` would exceed LOGICAL_MAX (i.e. the +1 saturates), advance `pt` by one millisecond and reset `new_logical` to 0 instead of clamping. That keeps the returned timestamp strictly greater than both the local state and `remote` for all inputs, preserving causality. Add a property test asserting `clock.update(remote) > remote` for arbitrary remotes including `logical == u32::MAX`.
|
||
|
||
> **Verification.** The bug is real. In `Hlc::update` (lines 273-317), when `pt == remote_ms` (remote wall is the highest), line 297 computes `new_logical = ((u64::from(remote_logical) + 1).min(LOGICAL_MAX)) as u16`. At line 278, `remote_logical` is already truncated to `u16::MAX = 65535 = LOGICAL_MAX`. If `remote.logical >= 65535`, then `remote_logical = 65535`, so `(65535u64 + 1).min(65535) = 65535 = LOGICAL_MAX`. The returned `HlcTimestamp` is `{wall_ns: wall_ms_to_ns(remote_ms), logical: 65535, node_id: self.node_id}`. The remote is `{wall_ns: wall_ms_to_ns(remote_ms), logical: 65535, node_id: remote.node_id}`. Since `HlcTimestamp::cmp` (line 137-143) …
|
||
|
||
### C16. Cluster router has no concurrency limit or timeout — unbounded thread spawning under load
|
||
|
||
**server-cluster · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal-server/src/cluster.rs:519-554` · verifier **CONFIRMED** (95)
|
||
|
||
The standalone router (`build_router` in router.rs:111-118) wraps protected routes in a `TimeoutLayer` + `ConcurrencyLimitLayer(MAX_CONCURRENCY=100)` so in-flight requests are bounded. `build_cluster_router` applies NEITHER. Cluster mode is therefore the path with the LEAST admission control, yet it is the one that, per request, spawns a fresh unbounded `std::thread` for every write (`offload_cluster_write`, line 1030) and one detached OS thread per shard for every sharded query (`dispatch_shards`, scatter_gather.rs:251). With no upstream backpressure, a burst of write/sharded traffic spawns threads without limit until the OS refuses — `dispatch_shards` even comments 'OS thread exhaustion: degrade this shard' (scatter_gather.rs:261-267), acknowledging the failure mode while doing nothing to prevent it. At 3am this is the classic cascade: load spikes, threads explode, the whole process (all regions, since they share it) falls over.
|
||
|
||
```rust
|
||
let protected = Router::new()
|
||
.route("/items", post(create_item))
|
||
...
|
||
.route("/sharded/search", get(sharded_search))
|
||
.layer(axum::extract::DefaultBodyLimit::max(2 * 1024 * 1024))
|
||
.with_state(state);
|
||
// (no TimeoutLayer, no ConcurrencyLimitLayer — cf. router.rs:111-118)
|
||
```
|
||
|
||
**Fix.** Apply the same `ServiceBuilder::new().layer(TimeoutLayer::...).layer(ConcurrencyLimitLayer::new(MAX_CONCURRENCY))` stack to the cluster router's protected routes (ideally factor the standalone middleware stack into a shared helper so the two routers can never drift). A bounded in-flight count is what caps the number of concurrently-spawned write/shard threads.
|
||
|
||
> **Verification.** The code matches the claim exactly at every cited location.
|
||
|
||
`build_cluster_router` (cluster.rs:519-554) builds its `protected` sub-router with only `DefaultBodyLimit::max(2 * 1024 * 1024)` and optional bearer-auth middleware. There is no `TimeoutLayer` and no `ConcurrencyLimitLayer`. The router is handed directly to `axum::serve` in `serve_cluster` (main.rs:195) with no additional wrapper.
|
||
|
||
By contrast, `build_router` (router.rs:111-118) explicitly wraps its protected routes with `ServiceBuilder::new().layer(TimeoutLayer::with_status_code(..., Duration::from_secs(30))).layer(ConcurrencyLimitLayer::new(100))`.
|
||
|
||
`offload_cluster_write` …
|
||
|
||
### C17. Coordinator max_per_creator silently under-enforces in entity-sharded topology (reads all metadata from one node)
|
||
|
||
**server-cluster · CORRECTNESS & LOGIC** · `tidal-server/src/scatter_gather.rs:458-495, 574-577, 701-704` · verifier **CONFIRMED** (88)
|
||
|
||
`enforce_max_per_creator` resolves every item's `creator_id` by calling `db.get_item_metadata` on a SINGLE node — `cluster.node(read_region).db` — and both call sites pass `read_region = cluster.leader_region()`. `db.get_item_metadata` (tidal/src/db/items.rs:190-201) reads only that one node's local item store. In an entity-sharded topology (entities written via `sharded_write_item`, which writes directly to one node and is NOT replicated), the leader node holds only its own slice. Items owned by other shards return `None`, are classified as 'unattributed', and are NEVER capped (lines 481-491). So the coordinator-level diversity cap — the entire reason this function exists ('each shard only enforces over its own slice') — silently fails to enforce for exactly the sharded case it claims to cover. The function's own doc claims 'an entity-sharded coordinator reads each item from whichever live shard returned it' (lines 451-453), but the code does not do that; it reads everything from `read_region`. A prolific creator can blow past the cap in a sharded feed with no warning.
|
||
|
||
```rust
|
||
let read_region = cluster.leader_region();
|
||
let dropped = enforce_max_per_creator(cluster, read_region, &mut all_items, max_per_creator);
|
||
// inside:
|
||
let db = &cluster.node(read_region).db;
|
||
...
|
||
let creator = db.get_item_metadata(item.entity_id()).ok().flatten()
|
||
.and_then(|meta| meta.get("creator_id").and_then(|c| c.parse::<u64>().ok()));
|
||
```
|
||
|
||
**Fix.** Resolve each item's creator from a node known to hold it. For replicated mode, any healthy region works; for entity-sharded mode, compute the owning shard via `entity_shard(item.entity_id(), shards)` and read metadata from THAT node (skipping partitioned shards). Failing that, query the metadata on the same shard that returned the item. Reading all items from a single leader node is only correct for fully-replicated topologies.
|
||
|
||
> **Verification.** The code at scatter_gather.rs lines 470, 575, and 702 confirms the reviewer's mechanism. `enforce_max_per_creator` (line 458) binds `let db = &cluster.node(read_region).db` (line 470) and reads ALL item metadata from that single node. Both call sites pass `cluster.leader_region()` as `read_region` (lines 575, 702). `sharded_write_item` (line 93–105) writes metadata via `cluster.node(shard).db.write_item_with_metadata`, where `shard = entity_shard(entity_id, shards)` — this writes to exactly one non-replicated shard. The leader node has no metadata for items owned by other shards. Consequently `db.get_item_metadata` returns `None` for those …
|
||
|
||
### C18. windowed_count(TwentyFourHours / SevenDays) double-counts after an hour boundary that does not fire 60 minute rotations
|
||
|
||
**signals · CORRECTNESS & LOGIC** · `tidal/src/signals/warm.rs:182-185, 440-445, 488-490` · verifier **CONFIRMED** (95)
|
||
|
||
An hour rotation rolls the minute aggregate up into a NEW hour bucket via `rotate_hour(hour_agg)`, but it does NOT clear the source minute buckets. The minute buckets are only cleared by `rotate_minute`, and `maybe_rotate` runs only `minutes_elapsed.min(60)` minute rotations — a count derived independently from `hours_elapsed`. When an hour boundary is crossed while fewer than 60 minutes have elapsed since the last minute rotation (the normal steady-state case: an event every few minutes), the minute buckets still hold the events that were just rolled up into the hour bucket. `windowed_count(TwentyFourHours)` = `sum_current_hour()` (reads all 60 minute buckets) + `sum_last_n_hours(23)` (reads the hour bucket that now ALSO holds that data), so those events are counted twice. SevenDays is identical (`sum_current_hour() + sum_last_n_hours(167)`). I confirmed this by running the real code: 4 events placed at minute 59 then one event at minute 60 (crossing into hour 1 with a single minute rotation) yields `windowed_count(TwentyFourHours) == 7` against an all-time count of 4. The existing regression test `twenty_four_hour_window_no_double_count_across_hour_boundary` (line 754) hides the bug because it crosses a FULL hour (`increment(NS_PER_HOUR)`), which fires 60 minute rotations and happens to clear the minute tier. This inflates every 24h/7d windowed count (and the velocity …
|
||
|
||
```rust
|
||
Window::TwentyFourHours => self.sum_current_hour() + self.sum_last_n_hours(23),
|
||
Window::SevenDays => self.sum_current_hour() + self.sum_last_n_hours(HOUR_BUCKETS - 1),
|
||
// ... in maybe_rotate:
|
||
let steps = minutes_elapsed.min(MINUTE_BUCKETS);
|
||
for _ in 0..steps { self.rotate_minute(); } // only minutes_elapsed steps clear minute buckets
|
||
// ... hour rollup uses an INDEPENDENT count:
|
||
let h_steps = hours_elapsed.min(HOUR_BUCKETS);
|
||
for i in 0..h_steps { let bucket_val = if i == 0 { hour_agg } else { 0 }; self.rotate_hour(bucket_val); }
|
||
```
|
||
|
||
**Fix.** Make the cascade clear the source tier it just rolled up. When an hour rotation fires, the minute buckets whose data was folded into the new hour bucket must be zeroed (e.g. clear all 60 minute slots after capturing `hour_agg`, or have the window read only the minutes accumulated since the last hour rotation). Equivalently, change `sum_current_hour` to sum only minute buckets newer than `last_hour_rotation_ns` so it never overlaps a completed hour bucket. Add a regression test that crosses an hour boundary with a SINGLE minute rotation (events at minute 59 then minute 60) and asserts `windowed_count(TwentyFourHours) == all_time_count()`.
|
||
|
||
> **Verification.** The bug is real. The mechanism in `maybe_rotate` (lines 352–449) is:
|
||
1. `hour_agg` is captured as the sum of all 60 minute buckets (lines 381–388).
|
||
2. `steps = minutes_elapsed.min(MINUTE_BUCKETS)` (line 391) — only `minutes_elapsed` minute rotations fire, NOT necessarily 60.
|
||
3. `rotate_hour(hour_agg)` stores that aggregate into the new hour slot (lines 440–445).
|
||
|
||
`rotate_minute` (line 252) advances `current_minute` to the next slot and zeroes that next slot only — it does NOT zero the slot it just left. So if `minutes_elapsed < 60`, the minute buckets holding the folded data are NOT cleared.
|
||
|
||
`sum_current_hour` (line 488) calls …
|
||
|
||
### C19. windowed_count(ThirtyDays) double-counts after a day boundary because day rollup does not clear the hour tier
|
||
|
||
**signals · CORRECTNESS & LOGIC** · `tidal/src/signals/warm.rs:189, 460-481, 500-502` · verifier **CONFIRMED** (92)
|
||
|
||
Same defect one tier up. `rotate_days` rolls `day_agg` (the last 24 hour buckets) into a NEW day bucket but does not clear the source hour buckets. Hour buckets are only overwritten as the 168-slot hour ring wraps (~7 days later), so a day's rolled-up data persists in the hour ring long after it has also been written into a day bucket. `windowed_count(ThirtyDays)` = `sum_current_day()` (which reads the hour tier via `sum_last_n_hours(23)` + minute buckets) + `sum_last_n_days(30)` (which reads the day buckets that ALSO hold the same data). I confirmed against the real code: 13 events clustered in hours 20-23 of day 0, then one event just into day 1 (triggering a day rollup), yields `windowed_count(ThirtyDays) == 25` against an all-time count of 13 (day_buckets=12, sum_current_day=13, summed = 25). The existing test `thirty_day_window_aggregates_across_days` (line 689) misses this because it steps a FULL day per event, firing 24 hour rotations that push the prior day's hour data out of the last-24h read window. Any ranking profile using a 30-day window count gets up to ~2x inflated counts for ~24h after each day boundary.
|
||
|
||
```rust
|
||
Window::ThirtyDays => self.sum_current_day() + self.sum_last_n_days(30),
|
||
// sum_current_day reads the hour tier that still holds the rolled-up data:
|
||
fn sum_current_day(&self) -> u64 { self.sum_current_hour() + self.sum_last_n_hours(23) }
|
||
// rotate_days writes day_agg into a NEW day bucket but never clears the hour buckets it summed:
|
||
let d_steps = days_elapsed.min(DAY_BUCKETS);
|
||
for i in 0..d_steps { let bucket_val = if i == 0 { day_agg } else { 0 }; self.rotate_day(bucket_val); }
|
||
```
|
||
|
||
**Fix.** Mirror the signals-1 fix at the day tier: after capturing `day_agg`, clear (or exclude from the day-window read) the hour buckets that were folded into the new day bucket, so `sum_current_day()` covers only data not yet rolled up into a day bucket. Add a regression test that crosses a day boundary with the prior day's hour data still inside the last-24h window and asserts `windowed_count(ThirtyDays) <= all_time_count()`.
|
||
|
||
> **Verification.** The double-counting mechanism is real and present in the current code.
|
||
|
||
`windowed_count(ThirtyDays)` at line 189 returns `self.sum_current_day() + self.sum_last_n_days(30)`.
|
||
|
||
`sum_current_day()` (lines 500-502) returns `self.sum_current_hour() + self.sum_last_n_hours(23)`, which reads the 23 most recently completed hour buckets.
|
||
|
||
In `maybe_rotate` (lines 420-449), when a day boundary is crossed, `day_agg` is captured as the sum of `hour_agg` (the in-progress hour being rolled) plus the 23 most recent already-stored hour buckets (lines 430-435). Then `rotate_hour(hour_agg)` advances `current_hour` by 1, writing `hour_agg` to the next …
|
||
|
||
### C20. Dedup window is mutated before the batch is durably persisted, so a failed/lost batch is suppressed on retry
|
||
|
||
**wal · DURABILITY & CRASH RECOVERY** · `tidal/src/wal/writer.rs:260-281, 402-406` · verifier **CONFIRMED** (97)
|
||
|
||
partition_dedup calls dedup.is_duplicate(&event) for every queued append, which INSERTS the event's content hash into the dedup window on a miss — this happens BEFORE flush_batch writes and fsyncs the batch. The ordering is 'mark as seen, then try to persist'. If flush_batch then fails (disk error) the events are already recorded as seen in the in-memory dedup set, yet were never written to disk. A client that observes the error and retries the identical event will have it silently swallowed as a duplicate (seq=0) without ever being persisted — a lost acknowledged-intent write with no durable record. This is the classic record-then-commit inversion: dedup state should only reflect events that actually reached stable storage.
|
||
|
||
```rust
|
||
for (event, reply) in batch {
|
||
if dedup.is_duplicate(&event) { // inserts hash into `current` on miss
|
||
let _ = reply.send(Ok(0));
|
||
} else {
|
||
kept_events.push(event);
|
||
kept_replies.push(reply);
|
||
}
|
||
}
|
||
// ... later, only AFTER this returns:
|
||
next_seq = flush_batch(&mut segment, config, next_seq, &kept_events, kept_replies)?;
|
||
```
|
||
|
||
**Fix.** Split duplicate-detection from dedup-state insertion: in partition_dedup, only CHECK membership (contains) to route duplicates; insert the kept events' hashes into the dedup window only inside flush_batch AFTER segment.sync() succeeds. On flush failure, do not record the events as seen, so a retry is accepted and durably written.
|
||
|
||
> **Verification.** The finding is real. In `/tidal/src/wal/dedup.rs` at `is_duplicate` (lines 74-82), the method inserts the content hash into `self.current` on every non-duplicate call: it checks `current.contains` and `previous.contains`, then unconditionally calls `self.current.insert(hash)` before returning `false`. In `/tidal/src/wal/writer.rs`, `partition_dedup` (lines 260-281) calls `dedup.is_duplicate(&event)` for every event in the batch drain — this inserts each new event's hash into the dedup window's `current` set right then. Only after the loop does the caller at line 402-406 invoke `flush_batch`. If `flush_batch` fails (the error path at lines …
|
||
|
||
|
||
---
|
||
|
||
## 🟨 WARNING (40)
|
||
|
||
### W1. Resolver membership cache is never invalidated when a new cohort is defined — users keep stale (incomplete) memberships
|
||
|
||
**cohort · CORRECTNESS & LOGIC** · `tidal/src/cohort/resolver.rs:33-36, 100-111` · verifier **CONFIRMED** (97)
|
||
|
||
The resolver caches per-user cohort memberships keyed only by `user_id`, with the cache invalidated solely on user-metadata change (`invalidate(user_id)`, called from db/users.rs:114). Defining a NEW cohort changes the correct membership of EVERY already-resolved user, but there is no `clear()`/`invalidate_all()` primitive on the resolver and `define_cohort` does not flush the cache. Result: any user resolved before a `define_cohort` call keeps a stale membership list (missing the new cohort) until their metadata changes or they are LRU-evicted — for an active user that can be effectively never. Cohort-scoped trending/top for the new cohort then silently under-attributes signals. The doc comment on `resolve` (lines 62-72) only promises per-user invalidation, so this is a real correctness gap against the resolver's own contract once cohorts can be added at runtime.
|
||
|
||
```rust
|
||
cache: Mutex<LruCache<u64, Vec<CohortName>>>,
|
||
...
|
||
/// Call this when user metadata is updated so the next `resolve()` call
|
||
/// re-evaluates all predicates.
|
||
pub fn invalidate(&self, user_id: u64) {
|
||
let mut cache = self.cache.lock().expect("cohort resolver cache poisoned");
|
||
cache.pop(&user_id);
|
||
}
|
||
```
|
||
|
||
**Fix.** Add an `invalidate_all(&self)` (clears the whole LRU) or a registry-version stamp checked on each `resolve` hit, and call it from `TidalDb::define_cohort` (and any cohort redefinition path). A monotonically-increasing registry generation counter compared on cache lookup is the cheapest correct fix — bump it in `CohortRegistry::define` and store it alongside each cached entry; treat a generation mismatch as a miss.
|
||
|
||
> **Verification.** The code is exactly as described. In `/tidal/src/cohort/resolver.rs` lines 33-35, the struct holds `cache: Mutex<LruCache<u64, Vec<CohortName>>>` with no generation counter. The only invalidation primitive is `invalidate(user_id)` at lines 108-111, which pops a single key. In `/tidal/src/db/cohorts.rs` lines 19-41, `TidalDb::define_cohort` calls `self.cohort_registry.define(def)` (line 39) and does nothing with `self.cohort_resolver`. There is no `invalidate_all`, no generation stamp, and no cache flush anywhere in the define path. The reviewer's claim is accurate: any user resolved before a `define_cohort` call retains a stale (incomplete) …
|
||
|
||
### W2. serialize_cohort_def swallows serialization failure into an empty Vec, which deserializes back to None (silent cohort loss)
|
||
|
||
**cohort · ERROR HANDLING & RESILIENCE** · `tidal/src/cohort/types.rs:155-161` · verifier **CONFIRMED** (88)
|
||
|
||
`serialize_cohort_def` returns `serde_json::to_vec(def).unwrap_or_default()`, i.e. an empty `Vec<u8>` if serialization ever fails. `define_cohort` (db/cohorts.rs) persists this value under `Tag::CohortDef`, and on reopen `deserialize_cohort_def(b"")` returns `None` (types.rs:167-169) — so the cohort silently vanishes after restart with no error anywhere. The comment asserts this 'should never happen' for the current types, but the sibling pattern in replication/state.rs:120 uses `.expect(...)` for an equally-infallible serialization, and CODING_GUIDELINES §7 requires fallible operations to surface a typed `Result`. Returning an empty buffer converts a future serialization bug into silent durable data loss rather than a loud failure.
|
||
|
||
```rust
|
||
pub fn serialize_cohort_def(def: &CohortDef) -> Vec<u8> {
|
||
// serde_json::to_vec will not fail for these types ...
|
||
serde_json::to_vec(def).unwrap_or_default()
|
||
}
|
||
```
|
||
|
||
**Fix.** Return `crate::Result<Vec<u8>>` and propagate the serde error (or, matching replication/state.rs, `.expect("CohortDef serialization is infallible")` so a regression panics loudly at the source instead of writing a poison-pill empty value that disappears on restore). Never persist a zero-length value that round-trips to `None`.
|
||
|
||
> **Verification.** The code at `tidal/src/cohort/types.rs:156-161` is exactly as quoted: `serde_json::to_vec(def).unwrap_or_default()` returning an empty `Vec<u8>` on failure. `db/cohorts.rs:30` calls `serialize_cohort_def(&def)` and persists the result with no ability to detect a zero-length buffer. `deserialize_cohort_def(b"")` returns `None` per the test at types.rs:372. The sibling at `replication/state.rs:120` uses `.expect("ReplicationState serialization is infallible")`, which would loudly panic at the source rather than writing a poison-pill value. CODING_GUIDELINES §184 ("Every fallible operation returns `Result`. No `unwrap()`, no `expect()` outside …
|
||
|
||
### W3. flush_text_index and flush_creator_text_index swallow the flush send/ack failures, reporting success against a possibly-uncommitted index
|
||
|
||
**db-entities-ops · DURABILITY & CRASH RECOVERY** · `tidal/src/db/items.rs:425-505` · verifier **CONFIRMED** (95)
|
||
|
||
Both flush methods do `let _ = flush_tx.send(ack_tx)` and `let _ = ack_rx.recv_timeout(Duration::from_secs(10))`, discarding both the send error (syncer thread dead/channel dropped) and the recv_timeout result (timeout vs ack). flush_text_index partially compensates afterward by consulting `idx.is_healthy()` and attempting a rebuild, so an unhealthy syncer is caught. But flush_creator_text_index (498-505) has NO such health check — it ignores the ack entirely and returns `self.reload_creator_text_index()`, so if the creator text syncer never committed (timed out or died), the caller is told the flush succeeded and search runs against a stale creator index. For a 10s timeout that silently elapses, the method returns Ok with no warning logged.
|
||
|
||
```rust
|
||
pub fn flush_creator_text_index(&self) -> crate::Result<()> {
|
||
if let Some(ref flush_tx) = self.creator_text_flush_tx {
|
||
let (ack_tx, ack_rx) = crossbeam::channel::bounded(1);
|
||
let _ = flush_tx.send(ack_tx);
|
||
let _ = ack_rx.recv_timeout(std::time::Duration::from_secs(10));
|
||
}
|
||
self.reload_creator_text_index()
|
||
}
|
||
```
|
||
|
||
**Fix.** Mirror the flush_text_index pattern in flush_creator_text_index: match on recv_timeout, and after reload check `creator_text_index.is_healthy()`, attempting a rebuild_from durable storage (and returning a degraded TidalError) when the syncer is unhealthy, so callers never trust a stale creator search index. At minimum, log a warn! when the ack times out instead of discarding it.
|
||
|
||
> **Verification.** The code at `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/items.rs` lines 498-505 is exactly as quoted. `flush_creator_text_index` does `let _ = flush_tx.send(ack_tx)` and `let _ = ack_rx.recv_timeout(Duration::from_secs(10))`, discards both results with no logging, then unconditionally calls `self.reload_creator_text_index()` and returns its result as Ok.
|
||
|
||
By contrast, `flush_text_index` (lines 425-456) has an explicit post-flush health check: `if let Some(idx) = self.text_index.as_ref() && !idx.is_healthy()` — it logs a warning, calls `rebuild_item_text_index()`, sets a `text_index_unhealthy` atomic flag, and returns …
|
||
|
||
### W4. Histogram metrics omit the partition_id label every other metric carries
|
||
|
||
**db-metrics · API DESIGN & MAINTAINABILITY** · `tidal/src/db/metrics/histogram.rs:87-96` · verifier **CONFIRMED** (88)
|
||
|
||
Every scalar gauge/counter in render_prometheus() is emitted with a `{partition_id="0"}` label (mod.rs:292-298) or via write_metric_line, but the LatencyHistogram bucket/sum/count lines carry only the `le` label (or none). The three histograms — tidaldb_signal_write_latency_us, tidaldb_retrieve_latency_us, tidaldb_search_latency_us — and tidaldb_checkpoint_failures_total (mod.rs:466-471, no label) are thus label-inconsistent with the rest of the exposition. partition_id is hardcoded "0" today so this is cosmetic now, but the moment partitions become real, histogram and counter series cannot be disambiguated by partition while gauges can, and any recording rule / dashboard that joins on partition_id will silently drop the histogram series. The exposition contract should be uniform.
|
||
|
||
```rust
|
||
for (i, &bound) in self.bounds.iter().enumerate() {
|
||
let count = self.buckets[i].load(Ordering::Relaxed);
|
||
let _ = writeln!(out, "{name}_bucket{{le=\"{bound}\"}} {count}");
|
||
}
|
||
...
|
||
let _ = writeln!(out, "{name}_bucket{{le=\"+Inf\"}} {total_count}");
|
||
let _ = writeln!(out, "{name}_sum {total_sum}");
|
||
let _ = writeln!(out, "{name}_count {total_count}");
|
||
```
|
||
|
||
**Fix.** Thread the partition_id (or a generic label set) through render_prometheus / write_metric_line and the histogram render, so histogram lines emit `{le="..",partition_id="0"}` and _sum/_count emit `{partition_id="0"}`, matching the scalar metrics. Add tidaldb_checkpoint_failures_total's partition_id label at mod.rs:470 too.
|
||
|
||
> **Verification.** The code matches the claim. In histogram.rs lines 87-96, `render_prometheus` emits: `{name}_bucket{{le="{bound}"}} {count}` (only `le` label), `{name}_bucket{{le="+Inf"}} {total_count}` (only `le`), `{name}_sum {total_sum}` (no labels), `{name}_count {total_count}` (no labels). In mod.rs lines 292-298, the three initial scalar gauges use `{partition_id="0"}` in their format strings. In mod.rs lines 466-471, `tidaldb_checkpoint_failures_total {failures}` is emitted with no label. The inconsistency is real: histogram and checkpoint-failure series carry no `partition_id` while the uptime/health/info gauges do. The reviewer's claim that "every …
|
||
|
||
### W5. `dislike`/`skip` hard-negatives are in-memory only and lost on restart, contradicting the method's durability doc and the negative-signal invariant
|
||
|
||
**db-query-signals · CORRECTNESS & LOGIC** · `tidal/src/db/signals.rs:325-328` · verifier **CONFIRMED** (97)
|
||
|
||
In `signal_with_context`, any signal where `HardNegIndex::is_hard_neg_signal` is true (`skip`, `hide`, `dislike`, `block` per hard_neg.rs:94) adds an in-memory hard-negative via `self.hard_negatives.add(...)`. The method docstring (lines 299-303) asserts 'Hard negatives (hide/block) are durably written via write_relationship()'. But for a `dislike` or `skip` recorded only through `signal_with_context`, nothing is written as a relationship — only the base signal goes to the WAL, and state_rebuild.rs reconstructs hard-negatives exclusively from `RelationshipType::Hide`/`Block` edges (state_rebuild.rs:114). So a `dislike`-derived hard-negative is honored while the process lives (the item is suppressed) but silently vanishes after a crash/restart, re-surfacing disliked content. CODING_GUIDELINES §6 states negative signals 'carry the same weight, precision, and update immediacy as likes' and 'A hide creates a permanent hard-negative.' The doc comment overstates durability for the dislike/skip case.
|
||
|
||
```rust
|
||
// 1. Hard negatives.
|
||
if HardNegIndex::is_hard_neg_signal(signal_type) {
|
||
self.hard_negatives.add(user_id, item_u32);
|
||
}
|
||
```
|
||
|
||
**Fix.** Either persist dislike/skip-derived hard-negatives durably (write a relationship edge or replay them from the signal WAL in state_rebuild for hard-neg signal types), or correct the docstring to state explicitly that only relationship-based Hide/Block survive restart and that signal-derived hard-negatives (dislike/skip) are intentionally ephemeral — so callers relying on a 'permanent hard-negative' for a dislike aren't surprised.
|
||
|
||
> **Verification.** The bug is real and verified across three files:
|
||
|
||
1. `/tidal/src/db/signals.rs` lines 325-328: `signal_with_context` calls `self.hard_negatives.add(user_id, item_u32)` in-memory for all four `HARD_NEG_SIGNALS` (`skip`, `hide`, `dislike`, `block`) but never calls `write_relationship()` — no durable edge is written from this code path.
|
||
|
||
2. `/tidal/src/db/open.rs` line 241: On `Persistent` mode open, `hard_negatives: HardNegIndex::new()` is constructed empty. There is no scan of the signal WAL or the signal ledger to repopulate hard negatives from previously-recorded `skip`/`hide`/`dislike`/`block` signals. Only `rebuild_entity_state` is called …
|
||
|
||
### W6. session_signal mutates in-memory aggregates and consumes a seqno before the WAL append, so a WAL failure leaves live state ahead of the journal
|
||
|
||
**db-query-signals · DURABILITY & CRASH RECOVERY** · `tidal/src/db/sessions.rs:395-448` · verifier **CONFIRMED** (97)
|
||
|
||
session_signal applies the aggregation (`sig_entry.on_signal`), records the signaled entity, pushes the annotation, and increments `signals_written` (deriving the idempotency seqno from it) all BEFORE attempting the WAL append. If `wal.session_signal(...)` returns `Err` it is only logged via `tracing::warn!` and the call still returns `Ok(())`. The in-memory session therefore counts and decays a signal that the journal never recorded, and the consumed seqno leaves a gap that idempotent replay will not fill. This inverts the WAL-first ordering that CODING_GUIDELINES §2/§3 require ('Every write goes through the WAL before any processing'; 'Signal events are durably logged before aggregation occurs') and which the global `record_signal` path (ledger/core.rs:89) correctly follows. Session signals are explicitly ephemeral/in-memory-first, so impact is bounded, but the seqno-consumed-on-failure plus aggregate-ahead-of-journal is a real recovery skew the comment acknowledges only as 'silently lost on recovery.'
|
||
|
||
```rust
|
||
sig_entry.on_signal(weight, ts_ns);
|
||
drop(sig_entry);
|
||
...
|
||
let seqno_raw = state.signals_written.fetch_add(1, Ordering::Relaxed) + 1;
|
||
...
|
||
&& let Err(e) = wal.session_signal( ... Some(seqno_raw), Some(ikey.0))
|
||
{
|
||
tracing::warn!(error = %e, session_id = %handle.id, "session journal append failed");
|
||
}
|
||
```
|
||
|
||
**Fix.** Append to the session journal first; only on `Ok` apply `on_signal`, `track_signaled_entity`, the annotation, and the `signals_written` increment — mirroring the WAL-first ordering in `SignalLedger::record_signal`. If full WAL-first is intentionally not desired for session hot-state, derive the seqno from a counter that is only advanced after a successful append so replay sees no gap.
|
||
|
||
> **Verification.** The code at sessions.rs lines 395-448 is exactly as described. The sequence is:
|
||
1. Line 401: `sig_entry.on_signal(weight, ts_ns)` — in-memory aggregate mutated
|
||
2. Line 407: `state.track_signaled_entity(entity_id.as_u64())` — entity recorded
|
||
3. Lines 411-416: annotation pushed
|
||
4. Line 420: `state.signals_written.fetch_add(1, Ordering::Relaxed) + 1` — seqno consumed
|
||
5. Lines 434-448: WAL append attempted last, errors only logged via `tracing::warn!`, function returns `Ok(())`
|
||
|
||
The code even comments on this explicitly at lines 431-433: "In-memory state was already mutated (signals_written incremented), so a failed append means this signal is …
|
||
|
||
### W7. export_signals loads the entire WAL into memory regardless of the requested limit
|
||
|
||
**db-recovery · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/db/export.rs:261-298, 304-308` · verifier **CONFIRMED** (97)
|
||
|
||
`export_signals` calls `crate::wal::reader::read_all_events(&wal_dir)` which materializes every batch WAL event into a `Vec<EventRecord>` (reader.rs:122), then filters, pushes survivors into `results`, merges session-journal signals, sorts, and only THEN applies `effective_limit` via `results.truncate(...)`. The limit (default 100K, max 500K) is applied at the very end, so a multi-gigabyte WAL produces a multi-gigabyte intermediate allocation even for a `limit: Some(1)` export. The time-range/type filters also run after the full read rather than short-circuiting the reader. On a large database this is a memory spike and a long stall on what reads as a cheap, bounded API. It is bounded by WAL-on-disk size (not unbounded network input), hence WARNING not BLOCKER.
|
||
|
||
```rust
|
||
let all_events = crate::wal::reader::read_all_events(&wal_dir)
|
||
.map_err(|e| TidalError::internal("export_signals", e.to_string()))?;
|
||
for event in all_events {
|
||
```
|
||
|
||
**Fix.** Stream WAL events (push filters — since/until/type/limit — into a streaming reader or iterate segment-by-segment) and stop once `effective_limit` survivors are collected, rather than reading all events then truncating. If a global timestamp sort across batch + session sources is required for determinism, bound the in-flight buffer (e.g. a min-heap of size `effective_limit`) instead of holding the whole WAL.
|
||
|
||
> **Verification.** The cited code exists exactly as described. In `/tidal/src/db/export.rs` lines 261-264, `export_signals` calls `crate::wal::reader::read_all_events(&wal_dir)` which — per reader.rs line 122-131 — iterates every segment file and extends a single `Vec<EventRecord>` with all events before returning. Back in export.rs, the for-loop at line 264 then applies time-range and type filters, pushes survivors into `results`, merges session journal signals (line 301-302), sorts (line 305), and only then calls `results.truncate(effective_limit)` at line 308. The limit (default 100K, max 500K per the constants at lines 37-39) is enforced solely via …
|
||
|
||
### W8. record_positive holds a DashMap shard write-guard while taking write-guards on a second DashMap
|
||
|
||
**entities · CONCURRENCY & ASYNC SAFETY** · `tidal/src/entities/co_engagement.rs:84-97` · verifier **CONFIRMED** (78)
|
||
|
||
The user_recent entry guard (a write lock on one shard of user_recent) is held across the loop that calls self.edges.entry(...).or_insert() repeatedly, i.e. a guard on map A is held while acquiring guards on map B. Today this is sound because user_recent and edges are distinct DashMaps (independent shard arrays) and no other code path locks edges before user_recent, so no lock-order inversion exists. It is flagged because it is a fragile pattern in a hot write path: any future code that does the reverse (hold an edges entry, then touch user_recent for the same user) introduces a classic two-lock deadlock that is invisible at this call site. The guard is held longer than necessary — only item_u64 is needed for the edge loop; the queue push could be deferred until after edges are updated, shrinking the critical section.
|
||
|
||
```rust
|
||
let mut user_rec = self.user_recent.entry(user_id).or_default();
|
||
for &prev_item in user_rec.iter() {
|
||
if prev_item == item_u64 { continue; }
|
||
*self.edges.entry((item_u64, prev_item)).or_insert(0.0) += 1.0;
|
||
}
|
||
user_rec.push_back(item_u64);
|
||
...
|
||
drop(user_rec);
|
||
```
|
||
|
||
**Fix.** Snapshot the recent items needed (or just iterate into a small Vec/SmallVec) and drop the user_recent guard before touching self.edges, so no cross-map guard nesting exists. At minimum add a comment establishing the lock-ordering rule (user_recent before edges, never the reverse) so future edits do not invert it.
|
||
|
||
> **Verification.** The cited code exists verbatim at lines 84-97 of /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/entities/co_engagement.rs. In `record_positive`, line 84 acquires a DashMap shard write-guard via `self.user_recent.entry(user_id).or_default()` and holds it through the entire loop (lines 85-90) that calls `self.edges.entry((item_u64, prev_item)).or_insert(0.0)` — acquiring shard write-guards on a second, distinct DashMap while the first is still held. The explicit `drop(user_rec)` is at line 97, after both maps have been touched. The reviewer's characterization is accurate: the user_recent guard is held while edges guards are …
|
||
|
||
### W9. CapabilityRegistry::insert cap enforcement is a racy check-then-act; the documented hard ceiling can be transiently exceeded under concurrent inserts
|
||
|
||
**governance · CONCURRENCY & ASYNC SAFETY** · `tidal/src/governance/capability.rs:285-299` · verifier **CONFIRMED** (88)
|
||
|
||
The type docs and test (`registry_is_bounded_and_never_exceeds_cap`, 'registry must never exceed its cap') present `cap` as a hard ceiling. The implementation enforces it with `if self.tokens.len() >= self.cap { evict_one }` then `self.tokens.insert(...)` — a non-atomic check-then-act across a concurrent DashMap. Two threads inserting at the boundary can both observe `len < cap` (or both evict the same victim, since evict_one selects then removes without holding a global lock), then both insert, leaving the map above `cap`. The bound is therefore best-effort, not the guaranteed ceiling the docs claim. Memory stays loosely bounded so this is not corruption, but the stated invariant is false under concurrency and the single-threaded test masks it.
|
||
|
||
```rust
|
||
if self.tokens.len() >= self.cap {
|
||
let now_ns = crate::schema::Timestamp::now().as_nanos();
|
||
self.evict_one(now_ns);
|
||
}
|
||
let token_id = token.token_id.clone();
|
||
...
|
||
self.tokens.insert(token_id.clone(), std::sync::Arc::new(token));
|
||
```
|
||
|
||
**Fix.** Either soften the docs/test to state the cap is a best-effort soft ceiling that may be transiently exceeded under concurrent inserts, or serialize the evict+insert critical section (e.g. a short Mutex around the size-check/evict/insert sequence, or an eviction loop `while len > cap { evict_one }` after insert) so the post-insert size is genuinely bounded.
|
||
|
||
> **Verification.** The cited code at lines 285-299 of `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/governance/capability.rs` is exactly as described. The `insert` method performs a non-atomic check-then-act:
|
||
|
||
```rust
|
||
if self.tokens.len() >= self.cap { // DashMap .len() — one op
|
||
let now_ns = ...;
|
||
self.evict_one(now_ns); // separate DashMap iteration + remove
|
||
}
|
||
// ...
|
||
self.tokens.insert(token_id.clone(), ...); // another separate DashMap op
|
||
```
|
||
|
||
`DashMap` provides per-shard locking for individual operations but provides no global lock across multiple operations. Two concurrent callers can both observe `len < cap`, …
|
||
|
||
### W10. No concurrency / max-connections / per-RPC server-side timeout on the network-facing gRPC server (DoS surface)
|
||
|
||
**net · SECURITY & INPUT VALIDATION** · `tidal-net/src/server.rs:141-167` · verifier **CONFIRMED** (92)
|
||
|
||
The server builder sets only payload-size limits. It sets no concurrency_limit_per_connection, no http2_max_concurrent_streams, and (unlike the client, which sets .timeout(request_timeout)) no per-request timeout on inbound handlers. A peer that has passed mTLS (or any peer at all when insecure=true) can open many connections / streams and hold them, and a slow inbound stream has no server-imposed deadline. For a replication transport that is explicitly distributed- and security-critical, an untrusted-or-compromised peer can exhaust server resources. The inbound channel (channel_capacity) bounds queued segments but not concurrent connections/streams or per-call wall time.
|
||
|
||
```rust
|
||
let mut server_builder = tonic::transport::Server::builder();
|
||
// ... only .tls_config(...) and per-service max_*_message_size; no
|
||
// .concurrency_limit_per_connection(...), .max_concurrent_streams(...),
|
||
// or .timeout(...) is ever applied.
|
||
```
|
||
|
||
**Fix.** Set Server::builder().timeout(config.request_timeout), .concurrency_limit_per_connection(N), and .max_concurrent_streams(M) (or tcp keepalive / http2 keepalive on the server) so a stalled or abusive peer cannot pin server resources indefinitely. Drive the values from GrpcTransportConfig.
|
||
|
||
> **Verification.** In `start_server` (server.rs lines 141–176), `tonic::transport::Server::builder()` is constructed and only receives `.tls_config(...)` conditionally. The `WalShippingServer` service gets `.max_decoding_message_size` and `.max_encoding_message_size` (lines 165–167), but the server builder itself never has `.timeout(...)`, `.concurrency_limit_per_connection(...)`, `.max_concurrent_streams(...)`, or any HTTP/2 keep-alive applied. `GrpcTransportConfig` does define `request_timeout` (line 39), `keep_alive_interval` (line 43), and `keep_alive_timeout` (line 46) with defaults at lines 63–65, but none of these are wired into the server builder — …
|
||
|
||
### W11. Partial mTLS client config (cert without key, or key without cert) is silently ignored instead of erroring
|
||
|
||
**net · SECURITY & INPUT VALIDATION** · `tidal-net/src/tls.rs:42-48` · verifier **CONFIRMED** (97)
|
||
|
||
client_tls_config only installs a client identity when BOTH client_cert and client_key are Some. If an operator sets exactly one (a realistic config typo — they wrote client_cert but the key path env var was unset), the code silently builds a one-way-TLS client with no identity. Against a server that requires client_ca_root (server_tls_config always calls .client_ca_root(ca), so the server always demands mTLS), the handshake fails later as an opaque transport error, and the operator's actual misconfiguration (half-set client identity) is never reported. Silent omission of half-provided security material is exactly the kind of config gap that turns into an unexplained replication outage.
|
||
|
||
```rust
|
||
if let (Some(cert_path), Some(key_path)) = (&tls.client_cert, &tls.client_key) {
|
||
// identity installed
|
||
}
|
||
// else: one of the two set, the other not -> silently no client identity
|
||
```
|
||
|
||
**Fix.** Treat exactly-one-of (client_cert, client_key) as a configuration error: return GrpcTransportError::TlsConfig("client_cert and client_key must be provided together") so the misconfiguration is loud and actionable instead of degrading to anonymous TLS.
|
||
|
||
> **Verification.** In `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal-net/src/tls.rs`, `client_tls_config` (lines 35-51) reads the CA cert unconditionally, then at line 42 uses `if let (Some(cert_path), Some(key_path)) = (&tls.client_cert, &tls.client_key)` with no else branch. `TlsConfig` (config.rs lines 72-83) declares `client_cert: Option<PathBuf>` and `client_key: Option<PathBuf>` as independent optionals with no struct-level invariant enforcing they are either both set or both unset. There is no other validation site. If an operator sets exactly one field, the function returns `Ok(config)` with no identity installed and no diagnostic. The server …
|
||
|
||
### W12. Stage 2.5 blocked-creator bitmap is rebuilt per query by OR-ing each blocked creator's full item set
|
||
|
||
**query-executor · CORRECTNESS & LOGIC** · `tidal/src/query/executor/pipeline.rs:362-373` · verifier **CONFIRMED** (82)
|
||
|
||
For each FOR USER query, the blocked-creator exclusion clones and unions every blocked creator's entire item bitmap into a fresh `blocked_items` RoaringBitmap, then retains candidates against it. `creator_items.get(cid)` returns a clone (`|= &bm` over an owned `bm`), so a user who blocks a few prolific creators forces a per-query union over potentially very large item sets even though only the comparatively tiny candidate set needs checking. This is O(total items of all blocked creators) per query rather than O(candidates · lookup). Under a viral/prolific-creator block list this is wasted hot-path work and allocation.
|
||
|
||
```rust
|
||
let mut blocked_items = RoaringBitmap::new();
|
||
for &cid in &blocked_creators {
|
||
if let Some(bm) = creator_items.get(cid) {
|
||
blocked_items |= &bm;
|
||
}
|
||
}
|
||
candidates.retain(|id| !blocked_items.contains(id.as_u64() as u32));
|
||
```
|
||
|
||
**Fix.** Invert the loop: for each candidate, check whether its creator is in `blocked_creators` (resolve creator via the already-loaded metadata / a candidate→creator lookup), or intersect each blocked creator's bitmap with the current candidate bitmap rather than materializing the full union. This bounds the work by the candidate count, not by the blocked creators' total catalog size.
|
||
|
||
> **Verification.** The code at pipeline.rs:362-373 exists exactly as quoted. `creator_items.get(cid)` (entities/mod.rs:67-68) returns `r.clone()` — a full RoaringBitmap clone per blocked creator — then ORs each clone into `blocked_items`. This materializes the full union of all blocked-creator item sets regardless of the candidate set size before the single `candidates.retain` call. The `CreatorItemsBitmap` struct already provides `get_ref()` (line 76) returning a `dashmap::Ref` without cloning, and `union_for()` (line 85) which uses `get_ref` internally to avoid per-creator clones — but neither is used in the blocked-creator path. A user blocking a prolific …
|
||
|
||
### W13. signal_ranked candidate generation scans the entire signal ledger per query
|
||
|
||
**query-retrieve · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/query/executor/candidate_gen.rs:46-86` · verifier **CONFIRMED** (95)
|
||
|
||
signal_ranked_candidates iterates `ledger.entries()` — every (entity_id, signal_type_id) pair in the whole database — on each RETRIEVE that uses a SignalRanked candidate strategy (e.g. trending/hot profiles, the common case for a content feed). It builds a Vec of every entry matching the target signal type, then top-K selects. This is O(total_signal_entries) per query on the hottest read path, scaling with catalog size × signal types, not with `limit`. CODING_GUIDELINES §1/§6 explicitly call out avoiding full scans on hot paths and budget a <50ms end-to-end RETRIEVE; a multi-million-entity ledger blows that. The inline comment acknowledges it is a 'simplified ranking for candidate generation' but there is no index (e.g. a maintained top-K-by-signal structure) backing it.
|
||
|
||
```rust
|
||
for entry in ledger.entries() {
|
||
let (entity_id, signal_type_id) = entry.key();
|
||
if *signal_type_id == type_id {
|
||
let score = entry.value().hot.current_score(0, now_ns, 0.0);
|
||
scored.push((*entity_id, score));
|
||
}
|
||
}
|
||
```
|
||
|
||
**Fix.** Back SignalRanked candidate generation with a per-signal maintained top-K / sorted structure (or a bounded heap fed incrementally on writes) so candidate gen is O(K) not O(N). If a full scan is genuinely interim, gate it behind a documented small-catalog assumption and a tracking issue, and surface a degradation warning above a size threshold.
|
||
|
||
> **Verification.** The cited code exists exactly as quoted in `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/query/executor/candidate_gen.rs` at lines 59-68. `signal_ranked_candidates` calls `ledger.entries()` (line 59), which returns the raw `DashMap<(EntityId, SignalTypeId), EntitySignalEntry>` (confirmed in `signals/ledger/core.rs:416`), then iterates every key in the map, filtering by `*signal_type_id == type_id`. There is no secondary per-signal index, no sorted structure, and no size gate. The `DashMap` itself has no iteration shortcut — every shard is walked. The CODING_GUIDELINES (`docs/CODING_GUIDELINES.md:242`) explicitly state `<50ms` …
|
||
|
||
### W14. Trending/CohortTrending scope resolution scans the full ledger every search
|
||
|
||
**query-retrieve · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/query/search/scope.rs:119-214` · verifier **CONFIRMED** (97)
|
||
|
||
resolve_trending and resolve_cohort_trending both iterate every ledger entry (`self.ledger.entries()` / `cohort_ledger.iter_entries()`) to accumulate per-entity windowed counts, then percentile-filter. This runs in Stage 0 of every SEARCH whose query sets `within: Trending{..}` or `CohortTrending{..}` — a primary discovery surface. Like query-retrieve-2 this is O(total_ledger_entries) per query and unrelated to `limit`. For CohortTrending it is worse: it scans the entire cohort ledger (all cohorts) and filters by string compare `entry_cohort != cohort` per entry, rather than keying directly into the requested cohort. The <10ms BM25 / <50ms end-to-end budgets in CODING_GUIDELINES §8 cannot hold at scale.
|
||
|
||
```rust
|
||
for entry in cohort_ledger.iter_entries() {
|
||
let (ref entry_cohort, entity_id, type_id) = *entry.key();
|
||
if entry_cohort != cohort { continue; }
|
||
...
|
||
let count = entry.value().warm.windowed_count(window);
|
||
*entity_velocity.entry(entity_id.as_u64()).or_insert(0) += count;
|
||
}
|
||
```
|
||
|
||
**Fix.** Maintain a velocity/trending index (per-window top-K by view+share) updated incrementally on signal writes, or at least a cohort-prefixed secondary index so CohortTrending probes only the requested cohort's entries instead of scanning all cohorts and string-comparing each key.
|
||
|
||
> **Verification.** The cited code exists verbatim in the current file. In `/tidal/src/query/search/scope.rs`:
|
||
|
||
- `resolve_trending` (line 119): iterates `self.ledger.entries()` — the full global SignalLedger DashMap — filtering by signal type in the loop body. No pre-computed index.
|
||
- `resolve_cohort_trending` (line 157): iterates `cohort_ledger.iter_entries()` — confirmed in `/tidal/src/cohort/ledger.rs` line 191-201 to be a plain `self.entries.iter()` over the entire `DashMap<(String, EntityId, SignalTypeId), EntitySignalEntry>`. The cohort name is only the first field of a composite key with no secondary index. The skip `if entry_cohort != cohort { …
|
||
|
||
### W15. SEARCH pipeline re-clones the combined filter 3-4 times per query
|
||
|
||
**query-retrieve · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/query/search/executor/pipeline.rs:96-99, 162-163, 166, 362, 411, 469` · verifier **CONFIRMED** (97)
|
||
|
||
Search::combined_filter() (types.rs:104-110) clones the entire `filters` Vec into a fresh And node on every call. The SEARCH execute path calls it independently in the reject-validators block (line 96), apply_metadata_filter (line 362), apply_creator_metadata_filter (line 411), and apply_user_context_filter (line 469) — at least 3-4 full filter-tree clones per query. The RETRIEVE pipeline deliberately computes `combined_filter` once and threads it (pipeline.rs:50-53, with an explicit comment that it 'clones the entire filters Vec ... and the pipeline consults it at 6+ points'). The SEARCH path lost that optimization, so the two surfaces handle the identical concern inconsistently and SEARCH pays repeated allocation on every request.
|
||
|
||
```rust
|
||
if let Some(ref filter_expr) = query.combined_filter() { ... } // line 96
|
||
self.apply_metadata_filter(query, &mut candidates); // calls combined_filter() again, line 362
|
||
self.apply_creator_metadata_filter(query, &mut candidates); // and again, line 411
|
||
self.apply_user_context_filter(query, &mut candidates); // and again, line 469
|
||
```
|
||
|
||
**Fix.** Compute `let combined = query.combined_filter();` once in execute() and pass `combined.as_ref()` into the stage helpers (mirroring RetrieveExecutor::execute), so the filter tree is cloned at most once per query.
|
||
|
||
> **Verification.** The finding is accurate in every detail. At `tidal/src/query/search/executor/pipeline.rs`, `query.combined_filter()` is called independently at line 96 (reject-validators block), line 362 (inside `apply_metadata_filter`), line 411 (inside `apply_creator_metadata_filter`), and line 469 (inside `apply_user_context_filter`) — 4 separate calls per query. `Search::combined_filter()` at `types.rs:104–110` clones on every call: `self.filters[0].clone()` for the single-filter case and `FilterExpr::And(self.filters.clone())` for the multi-filter case.
|
||
|
||
The RETRIEVE pipeline at `tidal/src/query/executor/pipeline.rs` explicitly avoids this: line 51 has …
|
||
|
||
### W16. build_user_context duplicated across RETRIEVE and SEARCH with divergent arg order
|
||
|
||
**query-retrieve · API DESIGN & MAINTAINABILITY** · `tidal/src/query/search/executor_helpers.rs:47-89` · verifier **CONFIRMED** (95)
|
||
|
||
build_user_context is implemented twice — personalization.rs:19-66 (RETRIEVE) and executor_helpers.rs:47-89 (SEARCH) — with byte-for-byte identical logic (top_creators -> creator_items expansion -> max-normalize boosts) but a different parameter order (RETRIEVE: user_id, now, ledger, items; SEARCH: ledger, items, user_id, now). Same for the user-state filter extractor (user_filter.rs::collect_user_state_filters vs executor_helpers.rs::collect_user_state_filters), which additionally differ in whether they extract SocialGraph — the SEARCH copy omits the SocialGraph arm entirely. Duplicated personalization/boost logic is exactly the kind of shared code CODING_GUIDELINES §6 warns about (it cites decay as the canonical example); two copies will drift (the arg-order swap and the SocialGraph divergence already show drift) and a fix to one will silently miss the other.
|
||
|
||
```rust
|
||
// search/executor_helpers.rs
|
||
pub(super) fn build_user_context(interaction_ledger: Option<&InteractionLedger>, creator_items: Option<&CreatorItemsBitmap>, user_id: u64, now: Timestamp) -> UserContext
|
||
// vs query/executor/personalization.rs
|
||
pub(crate) fn build_user_context(user_id: u64, now: Timestamp, interaction_ledger: Option<&InteractionLedger>, creator_items: Option<&CreatorItemsBitmap>) -> UserContext
|
||
```
|
||
|
||
**Fix.** Promote a single build_user_context (and a single user-state filter extractor) to a shared query module and have both executors call it, so personalization stays identical across RETRIEVE and SEARCH.
|
||
|
||
> **Verification.** Both claims are confirmed by the actual code.
|
||
|
||
1. `build_user_context` duplication with swapped arg order: `executor_helpers.rs:47-52` defines `pub(super) fn build_user_context(interaction_ledger, creator_items, user_id, now)` while `personalization.rs:19-24` defines `pub(crate) fn build_user_context(user_id, now, interaction_ledger, creator_items)`. The bodies are byte-for-byte identical (top_creators -> creator_interaction_boosts expansion -> max-normalize -> UserContext construction with empty preference_boosts).
|
||
|
||
2. SocialGraph omission in SEARCH: `executor_helpers.rs:19-36` (`collect_user_state_filters`) matches only `Saved | Liked | …
|
||
|
||
### W17. percentile_bitmap truncates u64 entity id to u32 with only a debug_assert guard
|
||
|
||
**query-search · CORRECTNESS & LOGIC** · `tidal/src/query/search/scope.rs:315-324` · verifier **CONFIRMED** (92)
|
||
|
||
Trending/CohortTrending scope resolution inserts entity ids into a RoaringBitmap via `bitmap.insert(id as u32)`, guarded only by `debug_assert!`. In a release build (the production target) the debug_assert is compiled out, so an entity id above u32::MAX silently truncates and ALIASES onto a low id — exactly the truncation hazard the rest of this zone is careful to avoid via `entity_as_u32`/`u32::try_from` (pipeline.rs lines 67-69, 346-351, 511-523). A truncated id would inject the wrong item into the trending scope pre-filter (or shadow a legitimate one), corrupting the candidate set for both BM25 and ANN. The id originates from the signal ledger, not directly from network input, but the codebase elsewhere treats >u32 ids as a real possibility and handles them by exclusion.
|
||
|
||
```rust
|
||
for &(id, vel) in &sorted {
|
||
if vel >= cutoff_velocity && vel > 0 {
|
||
debug_assert!(
|
||
u32::try_from(id).is_ok(),
|
||
"entity ID {id} exceeds u32::MAX; RoaringBitmap key truncation would produce incorrect scope results"
|
||
);
|
||
bitmap.insert(id as u32);
|
||
}
|
||
}
|
||
```
|
||
|
||
**Fix.** Replace the debug_assert + `id as u32` with the same fallible conversion used everywhere else in the zone: `if let Ok(i) = u32::try_from(id) { bitmap.insert(i); }` (optionally `tracing::warn!` on the dropped id). This keeps the scope bitmap correct in release builds instead of relying on a guard that is stripped from the production binary.
|
||
|
||
> **Verification.** The cited code exists verbatim at /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/query/search/scope.rs lines 315-324. The `percentile_bitmap` function (visible at lines 315-326) inserts entity ids via `bitmap.insert(id as u32)` with only a `debug_assert!(u32::try_from(id).is_ok(), ...)` guard. In a release build that assert is compiled out, leaving a bare truncating cast. The inline comment at lines 312-314 explicitly acknowledges the u32 constraint and states "The debug_assert catches violations in testing" — confirming the guard is intentionally debug-only, and therefore absent in production. The rest of the zone (pipeline.rs …
|
||
|
||
### W18. combined_filter() deep-clones the entire filter Vec on every call, several times per query
|
||
|
||
**query-search · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/query/search/types.rs:104-110` · verifier **CONFIRMED** (95)
|
||
|
||
`combined_filter()` clones either a single FilterExpr or the whole `filters` Vec (wrapped in And) on every invocation. Within a single SEARCH it is called at least four times on the hot path: pipeline.rs line 96 (validation), line 362 (`apply_metadata_filter`), line 411 (`apply_creator_metadata_filter`), line 469 (`apply_user_context_filter`), plus query_ops.rs line 162 (node-count guard). Each call re-clones the full expression tree (FilterExpr is recursive and may carry many String-valued nodes). On a query path the guidelines explicitly want allocation-disciplined (CODING_GUIDELINES §1), this is avoidable repeated allocation of the same data.
|
||
|
||
```rust
|
||
pub fn combined_filter(&self) -> Option<FilterExpr> {
|
||
match self.filters.len() {
|
||
0 => None,
|
||
1 => Some(self.filters[0].clone()),
|
||
_ => Some(FilterExpr::And(self.filters.clone())),
|
||
}
|
||
}
|
||
```
|
||
|
||
**Fix.** Compute the combined filter once at the top of `execute` (e.g. `let combined = query.combined_filter();`) and pass `Option<&FilterExpr>` into `apply_metadata_filter`, `apply_creator_metadata_filter`, and `apply_user_context_filter`, matching how RETRIEVE's pipeline binds `combined_filter` once and reuses `&filter_expr`.
|
||
|
||
> **Verification.** The cited code at `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/query/search/types.rs:104-110` is real and clones on every call. The SEARCH hot path calls it at least 5 times without binding the result:
|
||
|
||
1. `tidal/src/db/query_ops.rs:162` — node-count guard (clones)
|
||
2. `tidal/src/query/search/executor/pipeline.rs:96` — `if let Some(ref filter_expr) = query.combined_filter()` (clones)
|
||
3. `pipeline.rs:362` — `let Some(filter_expr) = query.combined_filter() else { return; }` inside `apply_metadata_filter` (clones)
|
||
4. `pipeline.rs:411` — `let Some(combined) = query.combined_filter() else { return; }` inside …
|
||
|
||
### W19. session_boost double-counts multi-word keyword hints and over-weights single hits
|
||
|
||
**ranking-core · CORRECTNESS & LOGIC** · `tidal/src/ranking/executor/mod.rs:376-405` · verifier **CONFIRMED** (97)
|
||
|
||
`session_boost` splits each keyword hint on whitespace and counts every token in both `total_kw` and the `hits` filter, but the `hits` filter tests each token independently with `metadata.values().any(|v| v.contains(kw))`. A single annotation hint like 'deep learning' contributes 2 to total_kw; if metadata contains the substring 'deep' but not 'learning', hits=1, hint_score=0.5 — an item that only partially matches one phrase gets half the full-match boost. Conversely a metadata value containing both tokens via unrelated fields double-counts. More importantly the documented contract in the rustdoc says `hint_score = hits / total_keywords * 0.3 (capped via the [0,1] fraction)` but the code computes `hint_score` as the raw fraction and multiplies by 0.3 only in the final return (`hint_score * 0.3 + vel_norm * 0.2`) — the doc and code disagree on what `hint_score` names, and the per-token (not per-hint) counting means the boost is not the intuitive 'fraction of hints matched'. This skews session-personalized ordering in a way that's hard to reason about during an incident.
|
||
|
||
```rust
|
||
let total_kw: usize = ctx.keywords.iter().flat_map(|h| h.split_whitespace()).count(); let hits = ... .flat_map(|h| h.split_whitespace()).filter(|kw| ...).count(); ... hint_score * 0.3 + vel_norm * 0.2
|
||
```
|
||
|
||
**Fix.** Decide on the unit: count matched HINTS (a hint matches if all/any of its tokens appear) rather than tokens, or document that scoring is token-granular. Align the rustdoc formula with the code (the `* 0.3` lives in the return, not in hint_score).
|
||
|
||
> **Verification.** The code at `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/ranking/executor/mod.rs` lines 376-405 is exactly as quoted by the reviewer.
|
||
|
||
Sub-claim 1 — token-level counting: `total_kw` is computed via `ctx.keywords.iter().flat_map(|h| h.split_whitespace()).count()` (line 381-385) and `hits` uses the identical `flat_map` + `filter` pattern (lines 386-396). A two-word hint like "deep learning" contributes 2 to `total_kw` and independently matches 0, 1, or 2 tokens. Confirmed real.
|
||
|
||
Sub-claim 2 — doc/code naming mismatch: The rustdoc at line 374 states `hint_score = hits / total_keywords * 0.3`. The code at line 401 assigns …
|
||
|
||
### W20. ProfileDecay weight outside [0,1] can negate the composite score and invert ranking
|
||
|
||
**ranking-executor · CORRECTNESS & LOGIC** · `tidal/src/ranking/executor/mod.rs:535-552` · verifier **CONFIRMED** (97)
|
||
|
||
compute_raw_score builds the multiplicative recency factor as `factor = decay.weight.mul_add(recency, 1.0 - decay.weight)`. `recency` is clamped to [0,1] (line 544) but `decay.weight` is NOT clamped or validated. The doc comment (lines 530-534) asserts "`weight` in `[0, 1]`", yet ProfileRegistry::validate_profile (registry.rs:128-174) validates only exploration and gate thresholds — never ProfileDecay.weight. A profile registered with weight=2.0 produces factor = 2*recency - 1, which is NEGATIVE for any item whose decay signal recency < 0.5 (e.g. recency=0 -> factor=-1.0). That multiplies the whole additive composite by a negative number, flipping the sign of the score and silently inverting the relative ranking of aged content. A profile that registers cleanly should never be able to corrupt rank order at 3am.
|
||
|
||
```rust
|
||
let factor = decay.weight.mul_add(recency, 1.0 - decay.weight);
|
||
...
|
||
additive * factor
|
||
```
|
||
|
||
**Fix.** Either clamp at read time — `let w = decay.weight.clamp(0.0, 1.0); let factor = w.mul_add(recency, 1.0 - w);` — or, better, reject `ProfileDecay.weight` outside [0,1] in ProfileRegistry::validate_profile so the misconfiguration fails loudly at registration rather than corrupting rankings at query time.
|
||
|
||
> **Verification.** The code is exactly as the reviewer describes. At `/tidal/src/ranking/executor/mod.rs` line 545, `let factor = decay.weight.mul_add(recency, 1.0 - decay.weight)` computes the recency multiplier. `recency` is clamped to [0,1] on line 544, but `decay.weight` is an unclamped `f64` field on `ProfileDecay` (`/tidal/src/ranking/profile.rs` lines 178-182: `pub weight: f64`). The `ProfileRegistry::validate_profile` function at `/tidal/src/ranking/registry.rs` lines 128-174 validates exploration (line 133), gate thresholds (lines 141-147), and aggregation support (lines 155-171), but has no check for `ProfileDecay.weight`. The `profile_signal_terms` …
|
||
|
||
### W21. Alphabetical tie-break re-reads + lowercases the title inside the sort comparator on every comparison
|
||
|
||
**ranking-executor · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/ranking/executor/mod.rs:596-614` · verifier **CONFIRMED** (92)
|
||
|
||
For AlphabeticalAsc/Desc, the finalize() comparator calls self.read_title(a) and self.read_title(b) for each pairwise comparison. read_title (scoring.rs:391-399) does a HashMap lookup AND `title.to_lowercase()` — a fresh String allocation — every call. sort_unstable_by invokes the comparator O(n log n) times, so an N-candidate alphabetical sort performs ~2·N·log N HashMap lookups and heap allocations purely to break prefix ties. CODING_GUIDELINES §1 ("Ranking queries should not allocate per-candidate", "Reuse Vec capacity") makes this a hot-path allocation regression: a 1,000-candidate alphabetical page allocates ~20k transient Strings inside the comparator.
|
||
|
||
```rust
|
||
if let Some(ascending) = alpha_dir {
|
||
let ta = self.read_title(a.entity_id);
|
||
let tb = self.read_title(b.entity_id);
|
||
let by_title = if ascending { ta.cmp(&tb) } else { tb.cmp(&ta) };
|
||
```
|
||
|
||
**Fix.** Precompute a key array once before sorting: build `Vec<(usize, Option<String>)>` of lowercased titles keyed by candidate index (or decorate-sort-undecorate), then compare the cached keys in the comparator. This drops title resolution from O(n log n) to O(n) lookups/allocations.
|
||
|
||
> **Verification.** The cited code exists exactly as described. In `finalize()` at mod.rs:596-614, the `sort_unstable_by` comparator calls `self.read_title(a.entity_id)` and `self.read_title(b.entity_id)` on lines 605-606 whenever `by_score == Equal`. `read_title` at scoring.rs:391-399 performs a `HashMap::get(&entity_id.as_u64())` lookup followed by `title.to_lowercase()` — a fresh heap-allocated `String` — on every invocation.
|
||
|
||
The short-circuit at line 599 does NOT save the alphabetical sort case: for `AlphabeticalAsc`/`AlphabeticalDesc`, the f64 score IS derived from the alphabetical prefix (8 bytes of title packed into a float), so many candidates will …
|
||
|
||
### W22. TopWindow score includes a quadratic-in-views `completion*views*0.1` term inconsistent with its own snapshot/explainability
|
||
|
||
**ranking-executor · CORRECTNESS & LOGIC** · `tidal/src/ranking/executor/scoring.rs:302-313` · verifier **CONFIRMED** (90)
|
||
|
||
score_top_window computes `views*0.3 + likes*0.3 + shares*0.2 + completion*views*0.1`. The final term multiplies the completion count by the view count, so the completion contribution scales QUADRATICALLY with views — a high-view item gets a completion bonus that grows with its view count, which contradicts the intent of a weighted engagement sum (every other term is linear). It also diverges from the explainability snapshot, which records the raw `completion` value (line 311) not the `completion*views` product actually summed, so the snapshot cannot reconstruct the score. There is no spec formula for TopWindow (docs/specs/09 does not define it), so this is an undocumented implementation choice that is almost certainly a typo for `completion*0.1`.
|
||
|
||
```rust
|
||
views.mul_add(
|
||
0.3,
|
||
likes.mul_add(0.3, shares.mul_add(0.2, completion * views * 0.1)),
|
||
),
|
||
```
|
||
|
||
**Fix.** If a linear weighted sum was intended, change `completion * views * 0.1` to `completion * 0.1`. If the quadratic interaction is genuinely intended, document it and push the actual `completion*views` product into the snapshot so explainability matches the score.
|
||
|
||
> **Verification.** In `score_top_window` (lines 265–313 of `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/ranking/executor/scoring.rs`), `completion` is read with `SignalAgg::Value` (a raw event count, not a rate), and the score term at line 305 is `completion * views * 0.1` — two raw counts multiplied together, making the contribution quadratic in scale. The spec at `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/docs/specs/09-ranking-scoring.md` lines 1188–1192 specifies `completion_rate(w) * view.count(w) * 0.1` — a rate (0–1) times a count, which is a dimensionally sensible cross-term. The implementation substitutes a raw completion …
|
||
|
||
### W23. RollingUpgradeCoordinator uses three different poison-handling strategies for the same lock; is_drained silently reports a drained shard as serving
|
||
|
||
**replication-core · CONCURRENCY & ASYNC SAFETY** · `tidal/src/replication/upgrade.rs:118-139` · verifier **CONFIRMED** (90)
|
||
|
||
The single UpgradeState lock is handled three inconsistent ways: drain/rejoin return TidalError::internal on poison (lines 69, 103), current_phase panics via .expect (line 137), and is_drained swallows poison with .unwrap_or(false) (line 124). is_drained is the dangerous one: on a poisoned lock it reports a shard as NOT drained. During a rolling upgrade an operator/router consulting is_drained could then route writes to a node that is actually draining/upgrading, exactly the failure the coordinator exists to prevent. A poisoned upgrade lock means a prior panic left state possibly inconsistent; defaulting to 'not drained' is the unsafe default.
|
||
|
||
```rust
|
||
pub fn is_drained(&self, shard_id: ShardId) -> bool {
|
||
self.state
|
||
.read()
|
||
.map(|s| s.drained.contains(&shard_id))
|
||
.unwrap_or(false)
|
||
}
|
||
```
|
||
|
||
**Fix.** Make poison handling uniform and fail-safe. For is_drained, recover the guard rather than defaulting to the unsafe answer: `self.state.read().map(|s| s.drained.contains(&shard_id)).unwrap_or_else(|e| e.into_inner().drained.contains(&shard_id))`, or return Result like drain/rejoin. At minimum, a poisoned upgrade lock should fail safe toward 'treat as drained' so writes are never routed to an upgrading node.
|
||
|
||
> **Verification.** The cited code exists exactly as described in /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/replication/upgrade.rs. All four inconsistencies are present:
|
||
|
||
1. `drain` (line 69): `.map_err(|_| TidalError::internal("upgrade", "state lock poisoned"))` — propagates error
|
||
2. `rejoin` (line 103): same pattern — propagates error
|
||
3. `current_phase` (lines 134-139): `.expect("state lock poisoned")` — panics; this is even documented in the docstring (lines 129-131 "Panics if the internal state lock is poisoned")
|
||
4. `is_drained` (lines 121-124): `.unwrap_or(false)` — silently swallows poison and returns false
|
||
|
||
The dangerous default is …
|
||
|
||
### W24. Tenant migration state and the routing cutover it performs are in-memory only; a crash mid-migration loses progress with no documented recovery
|
||
|
||
**replication-core · DURABILITY & CRASH RECOVERY** · `tidal/src/replication/migration.rs:101-122` · verifier **CONFIRMED** (97)
|
||
|
||
finalize() calls tenant_router.finalize_migration (which flips routing to the target shard) and then sets state to Finalizing, but MigrationState lives only in a Mutex (line 27) and the TenantRouter routing override is also in-memory (tenant.rs set_dual_write/finalize_migration mutate in-process maps with no WAL/checkpoint). If the process crashes after finalize_migration switches routing but before any durable record exists, restart loses both the migration state machine position AND the routing cutover, silently reverting reads/writes to the source shard. The doc comments make no mention of this non-durability or any recovery path, so an operator cannot reason about what happens to an in-flight migration across a restart. This violates the project rule that entity/migration lifecycle state should live durably and that every transition define what happens if it never completes.
|
||
|
||
```rust
|
||
self.tenant_router
|
||
.finalize_migration(self.tenant_id, self.target_shard);
|
||
let switched_at_ns = crate::replication::now_ns();
|
||
*state = MigrationState::Finalizing { switched_at_ns };
|
||
```
|
||
|
||
**Fix.** Either persist MigrationState transitions (and the routing override) to the WAL/checkpoint so a restart resumes the migration deterministically, or explicitly document on TenantMigration that migration state is process-local, non-durable, and that an in-flight migration must be restarted from Idle after any node restart. Order the durable write before the routing flip so a torn write never leaves routing ahead of recorded state.
|
||
|
||
> **Verification.** The cited code exists exactly as described. In `/tidal/src/replication/migration.rs` line 27, `MigrationState` lives in `Mutex<MigrationState>` with no persistence. The `finalize()` function at lines 101-122 calls `self.tenant_router.finalize_migration(self.tenant_id, self.target_shard)` (lines 116-117) before setting `*state = MigrationState::Finalizing { switched_at_ns }` (line 119) — both purely in-memory. In `/tidal/src/replication/tenant.rs`, `TenantRouter` holds `dual_write_map: DashMap<TenantId, (ShardId, ShardId)>` (line 233) and `shard_pin_map: DashMap<TenantId, ShardId>` (line 236) — both in-process `DashMap`s. …
|
||
|
||
### W25. Hlc::update can return a duplicate (non-unique) timestamp when the local logical counter is already saturated
|
||
|
||
**replication-crdt · CORRECTNESS & LOGIC** · `tidal/src/replication/crdt/hlc.rs:288-294` · verifier **CONFIRMED** (97)
|
||
|
||
In the `pt == cur_wall` branches (lines 288-294), if the current packed logical counter is already at LOGICAL_MAX and `pt == cur_wall`, then `new_logical = ((cur_logical + 1).min(LOGICAL_MAX)) = LOGICAL_MAX`, so `new_packed == cur`. The `compare_exchange(cur, new_packed, ...)` SUCCEEDS (the value is unchanged but CAS only requires `current == cur`), and the function returns `(cur_wall_ns, LOGICAL_MAX, node_id)` — byte-identical to the timestamp the prior caller already observed. `now()` explicitly guards against exactly this (the `overflowed` branch, lines 226-231, carries into the wall to stay unique) and there is even a dedicated test `now_strictly_advances_when_logical_saturated`. `update()` has the matching saturation path but no carry, so it can hand out duplicate timestamps from the same node, silently breaking the per-node uniqueness invariant LWW depends on. Lower severity than crdt-1 only because saturating the local logical within one ms requires 65536 update() calls in a single millisecond; but it is the same class of bug and the same fix covers both.
|
||
|
||
```rust
|
||
let new_logical = if pt == cur_wall && pt == remote_ms {
|
||
((u64::from(cur_logical.max(remote_logical)) + 1).min(LOGICAL_MAX)) as u16
|
||
} else if pt == cur_wall {
|
||
((u64::from(cur_logical) + 1).min(LOGICAL_MAX)) as u16
|
||
```
|
||
|
||
**Fix.** Apply the same overflow-carry fix as crdt-1: detect when the +1 would exceed LOGICAL_MAX and instead bump `pt` by 1 ms with `new_logical = 0`, so the CAS always writes a value strictly greater than `cur` and the returned timestamp is never a duplicate. A single shared carry helper for both `now()` and `update()` removes the divergence.
|
||
|
||
> **Verification.** The cited code is real and the mechanism is exactly as claimed. In `update()` at lines 288-294, the two saturation paths are:
|
||
|
||
Line 291: `((u64::from(cur_logical.max(remote_logical)) + 1).min(LOGICAL_MAX)) as u16`
|
||
Line 294: `((u64::from(cur_logical) + 1).min(LOGICAL_MAX)) as u16`
|
||
|
||
When `cur_logical == LOGICAL_MAX` (65535u16), the +1 gives 65536u64, `.min(LOGICAL_MAX)` clamps it back to 65535u64, cast to u16 gives 65535u16. So `new_logical == cur_logical == LOGICAL_MAX`. Combined with `pt == cur_wall`, `new_packed = pack(pt, LOGICAL_MAX) = pack(cur_wall, LOGICAL_MAX) = cur` (the unchanged old value). The CAS `compare_exchange(cur, new_packed, …
|
||
|
||
### W26. Session bridge silently drops corrupt payloads while WAL receiver halts — asymmetric, unobservable session-event loss
|
||
|
||
**replication-data · DURABILITY & CRASH RECOVERY** · `tidal/src/replication/session_bridge.rs:313-326` · verifier **CONFIRMED** (97)
|
||
|
||
`recv_and_apply` treats a checksum/decode failure as a non-event: it logs a `warn!` and returns `Some(0)`, then the caller loops and keeps consuming. This is the exact opposite of the WAL `spawn_receiver`, which HALTS the thread on `WalError::Corruption` so the operator sees a stalled replica (receiver.rs:95-107). The two replication streams therefore have opposite failure semantics. For session journal data — which is soft-state used to restore active sessions — graceful skip is a defensible choice, but the loss is observable only as a `warn` line: there is no corruption counter, no metric, and no way for an operator to answer 'did this follower silently drop session events?' at 3am. The session decode path (`decode_session_events`) also silently truncates a checksum-valid-but-structurally-malformed payload, compounding the silent-loss surface.
|
||
|
||
```rust
|
||
let events = match payload.decode_and_verify() {
|
||
Ok(e) => e,
|
||
Err(e) => {
|
||
// Log and skip corrupted payloads.
|
||
tracing::warn!(
|
||
"session_bridge: corrupted payload from {}: {e}",
|
||
payload.source_shard
|
||
);
|
||
return Some(0);
|
||
}
|
||
};
|
||
```
|
||
|
||
**Fix.** Keep the skip-and-continue policy (correct for soft session state, unlike the WAL gap-must-halt rule), but make the loss observable: increment a `corrupted_payloads` counter on the bridge (an `AtomicU64`) and surface it the way `decode_session_events_with_diagnostics` already surfaces `corruption_count`, so a stuck/lossy follower is detectable without reading logs. Document the deliberate asymmetry with the WAL receiver in the `recv_and_apply` doc comment.
|
||
|
||
> **Verification.** The cited code exists verbatim at lines 313-326 of `/tidal/src/replication/session_bridge.rs`. `recv_and_apply` calls `payload.decode_and_verify()`, catches the `Err`, emits `tracing::warn!`, and returns `Some(0)` — no counter, no metric, no halt. `SessionReplicationBridge` struct (lines 227-234) has no `AtomicU64` or any corruption-counter field. The WAL segment receiver at `receiver.rs:95-107` explicitly halts and returns `Err(e)` on corruption, confirming the asymmetry. `decode_session_events_with_diagnostics` in `wal/format/session.rs:256` already returns a `SessionDecodeOutcome` with a `corruption_count: u64` field (lines 242-246), but …
|
||
|
||
### W27. Ship-side HWM is in-memory only; a leader restart re-ships every sealed segment / session batch to every peer
|
||
|
||
**replication-data · DURABILITY & CRASH RECOVERY** · `tidal/src/replication/shipper.rs:100-115` · verifier **CONFIRMED** (82)
|
||
|
||
Both ship-side high-water-marks live only in process memory: the WAL shipper's `peer_hwm`/`quarantined` are local `HashMap`/`HashSet` re-initialized to 0 each `spawn_shipper` call (shipper.rs:105-115), and the session bridge's `ship_hwm` is a `DashMap` with no persistence and an explicit doc note that it resets 'after crash recovery' (session_bridge.rs:218-220, 233). After a leader restart, the shipper re-ships ALL sealed segments from seqno 0 to every peer, and the session bridge re-ships all journal entries. This is correct (the receiver's `ReplicationState` HWM and the `SessionSeqNoTracker` HWM make the replay idempotent), but it means a leader bounce in a large cluster triggers a full re-ship storm to every follower on every restart — a real resource/latency event under normal operations, not a corruption bug. The quarantine set resetting on restart also means a permanently-broken peer gets re-attempted once after every leader restart.
|
||
|
||
```rust
|
||
let mut peer_hwm: HashMap<ShardId, u64> =
|
||
config.peer_shards.iter().map(|&p| (p, 0)).collect();
|
||
...
|
||
let mut quarantined: HashSet<ShardId> = HashSet::new();
|
||
```
|
||
|
||
**Fix.** Persist the per-peer ship HWM (and ideally the quarantine set) alongside the WAL/checkpoint so a restarted leader resumes from the last acknowledged seqno per peer instead of re-shipping from 0. At minimum, document the restart re-ship storm in the cluster runbook so operators expect the post-restart bandwidth spike and the one re-attempt of quarantined peers.
|
||
|
||
> **Verification.** The cited code exists exactly as described. In `/tidal/src/replication/shipper.rs` lines 105-115, `peer_hwm: HashMap<ShardId, u64>` and `quarantined: HashSet<ShardId>` are initialized fresh inside the `spawn_shipper` closure — pure process-local state that resets to zero/empty on every shipper spawn. In `/tidal/src/replication/session_bridge.rs` lines 233 and 249, `ship_hwm: DashMap<(u64, ShardId), SessionSeqNo>` is initialized to `DashMap::new()`, and the doc comment at lines 217-220 explicitly acknowledges "the same events are never re-shipped unless the bridge is reconstructed (e.g., after crash recovery)." The receiver-side …
|
||
|
||
### W28. Embedding-slot names are unvalidated for charset, so the schema fingerprint has a canonicalization ambiguity
|
||
|
||
**schema · DURABILITY & CRASH RECOVERY** · `tidal/src/schema/validation/builders.rs:288-308` · verifier **CONFIRMED** (82)
|
||
|
||
embedding_slot() accepts any string for the slot name; build() only rejects empty names and duplicate (name, kind) pairs. There is no charset restriction (unlike signal names, which must be lowercase-ascii). The fingerprint consumer hashes slot names by concatenating raw `name.as_bytes()` then a 1-byte kind (0/1/2) then dims as u64-LE with no length prefix or delimiter (db/schema_fingerprint.rs:70-78). Because slot names may contain NUL/0x01/0x02/digits, two distinct slot configurations can produce the same byte stream and therefore the same fingerprint — e.g. a name ending in a byte that overlaps the following kind/dims bytes. A fingerprint collision lets a genuinely-changed embedding schema (different dims or kind) be silently accepted on reopen, exactly the silent-vector-corruption case the fingerprint exists to prevent. Likelihood is low (names are normally clean identifiers) but the schema layer is what should make it impossible.
|
||
|
||
```rust
|
||
// builders.rs — only empty + duplicate checks, no charset validation:
|
||
for slot in &self.embedding_slots {
|
||
if slot.name.is_empty() { return Err(SchemaError::DuplicateEmbeddingSlot { .. }); }
|
||
if slot.dimensions == 0 { return Err(SchemaError::InvalidEmbeddingDimensions { .. }); }
|
||
if !seen_slots.insert((slot.name.as_str(), slot.entity_kind)) { return Err(..); }
|
||
}
|
||
// schema_fingerprint.rs — no delimiter/length prefix:
|
||
hasher.update(name.as_bytes());
|
||
hasher.update(&[kind_byte]);
|
||
hasher.update(&(dims as u64).to_le_bytes());
|
||
```
|
||
|
||
**Fix.** Validate embedding-slot names with the same is_valid_signal_name charset rule (or a dedicated identifier check) at build() and add an InvalidEmbeddingSlotName error. Independently, make the fingerprint encoding unambiguous by length-prefixing every variable-length field (write name.len() as u32-LE before name.as_bytes()), so canonicalization no longer depends on the name charset.
|
||
|
||
> **Verification.** Both code claims are exactly correct in the current committed code.
|
||
|
||
1. builders.rs lines 288-308: The embedding slot validation loop checks only `slot.name.is_empty()`, `slot.dimensions == 0`, and duplicate `(name, entity_kind)` pairs. There is no charset restriction. By contrast, signal names go through `is_valid_signal_name()` (mod.rs line 167) which enforces ASCII lowercase alphanumeric + underscore starting with a letter.
|
||
|
||
2. schema_fingerprint.rs lines 70-78: The loop writes `hasher.update(name.as_bytes())`, then `hasher.update(&[kind_byte])` (values 0, 1, or 2), then `hasher.update(&(dims as u64).to_le_bytes())` — no length prefix or …
|
||
|
||
### W29. Signal windows and velocity flag are excluded from the schema fingerprint
|
||
|
||
**schema · DURABILITY & CRASH RECOVERY** · `tidal/src/schema/signal.rs:16-32` · verifier **CONFIRMED** (97)
|
||
|
||
The schema fingerprint (db/schema_fingerprint.rs) hashes signal name, target, and decay model parameters, but NOT the WindowSet or velocity_enabled flag carried by SignalTypeDef. Window labels are the suffix of persisted aggregate keys (Window::label gives '1h','24h','7d','30d','all'; CODING_GUIDELINES §2 key encoding is `SIG:view:24h`). Reopening a persisted DB with a signal's window set changed (e.g. dropping SevenDays, adding ThirtyDays) is therefore silently accepted: previously-materialized aggregates for now-undeclared windows become orphaned key-space, and newly-declared windows start empty rather than being flagged as a schema change. The doc comment on `target` (lines 20-26) explicitly claims target is fingerprinted 'load-bearing for durability', which highlights by omission that windows — which actually key on-disk state — are not. Aggregates are recomputable from the event log per §3, so this is recoverable rather than corrupting, hence WARNING not BLOCKER, but a window change should at minimum be a deliberate, detected event.
|
||
|
||
```rust
|
||
// signal.rs SignalTypeDef carries windows + velocity:
|
||
windows: WindowSet,
|
||
velocity_enabled: bool,
|
||
// schema_fingerprint.rs hashes only name, target (kind), decay — never windows/velocity:
|
||
hasher.update(sig.name().as_bytes());
|
||
hasher.update(&[kind_byte]);
|
||
match sig.decay() { Exponential { half_life, .. } => { .. } Linear { .. } => { .. } Permanent => { .. } }
|
||
```
|
||
|
||
**Fix.** Add a stable accessor on SignalTypeDef exposing the sorted window labels and velocity flag, and fold them into compute_schema_fingerprint so a window-set or velocity change is surfaced as SchemaMismatch on reopen (or explicitly documented as a migration-safe no-op with the orphaned-key cleanup that implies).
|
||
|
||
> **Verification.** Reading /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/schema_fingerprint.rs confirms the claim precisely. The `compute_schema_fingerprint` function (lines 38-63) iterates over signals and hashes: `sig.name().as_bytes()`, an entity kind byte from `sig.target()`, and decay model discriminant+parameters. The `windows()` accessor (signal.rs line 82) and `velocity_enabled()` accessor (signal.rs line 88) are never called inside the fingerprint function — no other code path in schema_fingerprint.rs calls them either. The `Window::label()` method (signal.rs lines 216-224) returns `"1h"`, `"24h"`, `"7d"`, `"30d"`, `"all"` — these are …
|
||
|
||
### W30. Empty embedding-slot name reports the misleading DuplicateEmbeddingSlot error
|
||
|
||
**schema · API DESIGN & MAINTAINABILITY** · `tidal/src/schema/validation/builders.rs:289-295` · verifier **CONFIRMED** (95)
|
||
|
||
When an embedding slot is declared with an empty name, build() returns SchemaError::DuplicateEmbeddingSlot — a duplicate error for what is actually an empty-name error. The SchemaError doc comment (error.rs:258-267) even documents DuplicateEmbeddingSlot as covering 'declared an empty name, or two slots share the same (name, entity_kind) pair', folding two distinct developer mistakes into one variant. At 3am a developer who typo'd an empty slot name gets told they have a duplicate, sending them to hunt for a collision that doesn't exist. The test rejects_empty_embedding_slot_name (lines 651-661) locks in this misleading behavior.
|
||
|
||
```rust
|
||
if slot.name.is_empty() {
|
||
return Err(SchemaError::DuplicateEmbeddingSlot {
|
||
slot: slot.name.clone(),
|
||
entity_kind: slot.entity_kind,
|
||
});
|
||
}
|
||
```
|
||
|
||
**Fix.** Add a dedicated SchemaError::InvalidEmbeddingSlotName(String) (mirroring InvalidTextField) and return it for the empty-name case; reserve DuplicateEmbeddingSlot for genuine (name, kind) collisions.
|
||
|
||
> **Verification.** The code at builders.rs:290-294 is exactly as quoted: the empty-name check returns `SchemaError::DuplicateEmbeddingSlot { slot: slot.name.clone(), entity_kind: slot.entity_kind }` rather than any distinct "empty name" error. The doc comment at error.rs:258-259 explicitly codifies this conflation ("An embedding slot declared an empty name, or two slots share the same `(name, entity_kind)` pair"), folding two distinct mistakes into one variant. The test at lines 651-661 asserts `matches!(result, Err(SchemaError::DuplicateEmbeddingSlot { ref slot, .. }) if slot.is_empty())`, locking in the misleading behavior. The error message at runtime would …
|
||
|
||
### W31. shard_deadline_ms is reported to clients but never enforced on the shard query
|
||
|
||
**server-cluster · API DESIGN & MAINTAINABILITY** · `tidal-server/src/scatter_gather.rs:533-535, 550-558, 596-602` · verifier **CONFIRMED** (97)
|
||
|
||
`shard_deadline_ms` is computed (`deadline_ms - NETWORK_OVERHEAD_MS`) and surfaced in `ScatterGatherMeta` / the HTTP response as 'Per-shard deadline that was propagated'. But it is never propagated to the shard: the per-shard closure calls `cluster.retrieve(shard, &shared_query)` / `cluster.search(...)`, and `db.retrieve`/`db.search` take no deadline argument (tidal/src/testing/cluster.rs:554-569). The only deadline actually enforced is the coordinator's `total_deadline` at `recv_timeout` (dispatch_shards line 280-284), which abandons (leaks) a slow shard's still-running thread rather than canceling its query. So the metadata claims a per-hop budget the shard never receives, and an operator reading `shard_deadline_ms` to reason about per-shard timeouts is misled. The doc comment 'Per-shard deadline that was propagated' (line 49) is stale/aspirational.
|
||
|
||
```rust
|
||
let shard_deadline_ms = deadline_ms.unwrap_or(DEFAULT_DEADLINE_MS).saturating_sub(NETWORK_OVERHEAD_MS);
|
||
// ... but the shard closure has no deadline:
|
||
let result = cluster.retrieve(shard, &shared_query).map_err(ServerError::from)?;
|
||
```
|
||
|
||
**Fix.** Either thread a real deadline into `db.retrieve`/`db.search` so the shard query can abort early, or rename/re-document the field to make clear it is only the coordinator's total budget minus a fixed overhead estimate and is NOT enforced per shard. Reporting a budget that is never applied invites incorrect operator conclusions during an incident.
|
||
|
||
> **Verification.** The finding is fully confirmed by the current code.
|
||
|
||
1. `scatter_gather.rs` lines 533-535: `shard_deadline_ms` is computed as `deadline_ms.unwrap_or(DEFAULT_DEADLINE_MS).saturating_sub(NETWORK_OVERHEAD_MS)` in both `scatter_gather_retrieve` and `scatter_gather_search` (same pattern at lines 656-658).
|
||
|
||
2. `ScatterGatherMeta.shard_deadline_ms` (line 49) has the doc comment "Per-shard deadline that was propagated." — the word "propagated" is aspirational, not factual.
|
||
|
||
3. The shard closure passed to `dispatch_shards` (lines 550-558 for retrieve, 673-685 for search) calls `cluster.retrieve(shard, &shared_query)` / `cluster.search(shard, …
|
||
|
||
### W32. Slow/hung shard threads are leaked indefinitely; no cap on simultaneously-leaked workers
|
||
|
||
**server-cluster · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal-server/src/scatter_gather.rs:197-328` · verifier **CONFIRMED** (82)
|
||
|
||
By design `dispatch_shards` detaches shard workers and returns the partial result the instant the budget expires without joining (documented, lines 181-196). A worker whose blocking `db.retrieve` never completes is leaked for the life of the process. Combined with finding server-cluster-1 (no concurrency limit on the cluster router), a sustained pattern of one genuinely-hung shard means every sharded request leaks one more permanently-parked OS thread, with no upper bound and no metric. The test `dispatch_shards_slow_shard_does_not_block_deadline` only sleeps 2s, so it never exercises a truly non-terminating shard. This is acceptable as a deliberate tradeoff only if there is an admission cap upstream; with no cap (server-cluster-1) the leaks accumulate unbounded under a stuck-shard incident.
|
||
|
||
```rust
|
||
// detached, never joined:
|
||
std::thread::Builder::new().name(format!("scatter-shard-{}", shard.0)).spawn(move || {
|
||
let outcome = query_one(&cluster, shard);
|
||
let _ = tx.send((shard, outcome));
|
||
});
|
||
match spawned { Ok(_handle) => dispatched.push(shard), ... }
|
||
```
|
||
|
||
**Fix.** Cap the number of concurrently in-flight (leaked) shard workers — e.g. a process-wide bounded thread pool or semaphore for scatter-gather workers — and emit a gauge/metric for currently-parked workers so a stuck shard is observable. At minimum, fixing server-cluster-1 (admission control) bounds the leak rate; ideally bound it directly here.
|
||
|
||
> **Verification.** The detached-thread leak mechanism is real and exactly as described. In `dispatch_shards` (lines 251-258), `std::thread::Builder::new().spawn(...)` returns a `JoinHandle` that is matched as `Ok(_handle)` — the handle is immediately dropped (not stored), detaching the thread. The code's own doc-comment at lines 181-196 explicitly acknowledges this: "the threads must be detached, not scoped, so the coordinator can return its partial result the instant the budget expires without joining a shard whose blocking query is still in flight." There is no semaphore, thread pool, or cap on how many such detached threads can exist simultaneously. If …
|
||
|
||
### W33. Standalone shutdown relies on Drop and cannot observe final-flush failures
|
||
|
||
**server-core · DURABILITY & CRASH RECOVERY** · `tidal-server/src/main.rs:264-280` · verifier **CONFIRMED** (95)
|
||
|
||
The cluster path (serve_cluster) explicitly reclaims ownership and calls owned.shutdown() (main.rs:204-205), which RETURNS and surfaces any final checkpoint/WAL-fsync failure. The standalone serve() does no such thing: after axum::serve(...).await? returns, it just returns Ok(()), so the Arc<ServerState> (holding Arc<TidalDb>) is dropped and durability falls entirely to TidalDb::Drop. Per tidal/src/db/lifecycle.rs:246-261, Drop runs shutdown_inner() but can only log a failed final flush at error level — it cannot return it. The result: if the standalone server's final checkpoint/WAL marker fails on shutdown, the process still exits 0 and the operator gets no error signal. For a database whose bar is '3am production incident', the standalone graceful-shutdown path silently downgrades a durability failure to a log line, while the experimental cluster path treats it as fatal. The asymmetry is backwards — standalone is the production path.
|
||
|
||
```rust
|
||
axum::serve(listener, build_router(state, api_key))
|
||
.with_graceful_shutdown(shutdown_signal(shutdown_state))
|
||
.await?;
|
||
Ok(())
|
||
}
|
||
```
|
||
|
||
**Fix.** Mirror serve_cluster: after axum::serve returns, Arc::try_unwrap the ServerState, take ownership of the inner TidalDb, and call db.close() (the returning variant) so a failed final flush propagates as a non-zero process exit. If the Arc is still shared, log a warn and fall back to Drop as the cluster path does. This requires ServerState to expose a method that yields the owned TidalDb (or a close() that returns Result).
|
||
|
||
> **Verification.** The code at main.rs:264-280 (`serve()`) returns `Ok(())` immediately after `axum::serve(...).await?`, dropping the `Arc<ServerState>` (which holds `Arc<TidalDb>`) via Drop. In contrast, `serve_cluster()` (lines 204-212) explicitly calls `Arc::try_unwrap(shutdown_state)` followed by `owned.shutdown()`, which calls `close()` → `shutdown_inner()`, a path that returns `Result`. The `lifecycle.rs:246-261` Drop impl confirms the asymmetry: it calls `shutdown_inner()` but can only log the error (`tracing::error!`), and even has an explicit comment at line 241: "Drop logs it instead — see the Drop impl below." The `shutdown_inner()` at line 242 does …
|
||
|
||
### W34. Public readiness probe GET /health returns 400 when given ?region=
|
||
|
||
**server-core · API DESIGN & MAINTAINABILITY** · `tidal-server/src/router.rs:280-307` · verifier **CONFIRMED** (97)
|
||
|
||
The readiness handler reads ?region= from the query string and passes it to state.item_count(region), which calls ensure_standalone() (state.rs:178-181, 22-29) and returns ServerError::BadRequest for any non-None region. /health is the public, auth-exempt liveness/readiness route. A probe or operator hitting /health?region=us-east gets a 400 from a route that should answer only 'are you ready'. Mixing a data-plane concept (region routing) into the readiness probe is a layering smell: readiness should not depend on, or 400 on, a routing parameter. It also means a misconfigured load balancer that appends a region query param would flap the standalone server out of the ready pool.
|
||
|
||
```rust
|
||
let region = query.get("region").map(std::string::String::as_str);
|
||
let items = state.item_count(region).map_err(AppError)?;
|
||
```
|
||
|
||
**Fix.** Do not thread region through the readiness probe. Call item_count without a region in standalone (state.item_count(None)) or drop the items field from the readiness body entirely — readiness should answer only is_shutting_down plus a cheap liveness signal, never reject on a query parameter.
|
||
|
||
> **Verification.** router.rs lines 280-307: the `health()` handler extracts `query.get("region")` at line 295 and passes it directly to `state.item_count(region)` at line 296. state.rs lines 178-181 show `item_count` calls `ensure_standalone(region_name)`, and lines 22-29 show `ensure_standalone` returns `Err(ServerError::BadRequest(...))` for any non-None region. A request to `GET /health?region=us-east` will therefore return 400 from the readiness probe. The mechanism is real, the cited lines exist, and no guard elsewhere overrides this path. The severity (WARNING / API DESIGN & MAINTAINABILITY) is appropriate — it's a layering smell with a concrete …
|
||
|
||
### W35. saved_search deserialization uses from_utf8_lossy, silently masking on-disk corruption
|
||
|
||
**session · DURABILITY & CRASH RECOVERY** · `tidal/src/session/saved_search.rs:107,120,141` · verifier **CONFIRMED** (98)
|
||
|
||
deserialize_saved_search decodes the persisted name, query_text, and filter_expr_json with String::from_utf8_lossy. A torn or corrupt fjall record (or a length field pointing into adjacent bytes) is silently hydrated with U+FFFD replacement characters instead of being rejected. This directly contradicts the sibling serde module (session/serde/mod.rs:54-59, read_utf8), whose own doc says the OLD from_utf8_lossy 'masked corruption behind U+FFFD replacement characters' and which deliberately returns None on non-UTF-8 so 'corrupt text in a stored record is rejected, not silently replaced.' The saved-search codec was not migrated to that standard. Worse, the corrupted filter_expr_json is later fed to a JSON parser on retrieve_saved_search: a U+FFFD-mangled filter either parses to a wrong filter (silently wrong results) or fails far from the corruption site, with no signal that the stored record was damaged. At 3am this looks like a query bug, not the storage corruption it actually is.
|
||
|
||
```rust
|
||
let name = String::from_utf8_lossy(&bytes[pos..pos + name_len]).to_string();
|
||
...
|
||
let query_text = String::from_utf8_lossy(&bytes[pos..pos + query_len]).to_string();
|
||
...
|
||
let json = String::from_utf8_lossy(&bytes[pos..pos + filter_len]).to_string();
|
||
```
|
||
|
||
**Fix.** Reject non-UTF-8 by returning None instead of lossily replacing. Replace each `String::from_utf8_lossy(slice).to_string()` with `std::str::from_utf8(slice).ok()?.to_owned()` (the exact pattern in serde/mod.rs::read_utf8). Better, route serialize/deserialize_saved_search through the shared read_u32/read_len_prefixed_utf8 cursor helpers in session/serde/mod.rs so the saved-search wire format cannot drift from the rest of the session codecs and a corrupt record surfaces as a skipped/logged record rather than a silently mangled query.
|
||
|
||
> **Verification.** Both files were read in full. In saved_search.rs, `deserialize_saved_search` at lines 107, 120, and 141 uses `String::from_utf8_lossy(...).to_string()` for the name, query_text, and filter_expr_json fields respectively. In serde/mod.rs, the `read_utf8` helper (lines 54-59) uses `std::str::from_utf8(slice).ok()?.to_owned()` and its own doc comment at lines 51-52 explicitly calls out that "the old `from_utf8_lossy` masked corruption behind U+FFFD replacement characters" — making clear the codebase already identified and corrected this pattern in the shared helpers. The `saved_search.rs` codec was never migrated to those helpers. The …
|
||
|
||
### W36. Snapshot building treats a poisoned annotations/audit mutex as empty data
|
||
|
||
**session · ERROR HANDLING & RESILIENCE** · `tidal/src/session/snapshot.rs:106-110,134-138,173-183` · verifier **CONFIRMED** (95)
|
||
|
||
build_snapshot and build_frozen_snapshot recover from a poisoned Mutex by silently substituting empty data: `state.annotations.lock().map(|g| g.clone()).unwrap_or_default()` and the audit-log equivalent. A poisoned lock means a prior thread panicked while holding it — exactly the moment you want a loud signal. Instead, on close_session the frozen snapshot would archive empty annotations and an empty audit log with audit_truncated=false, permanently discarding the policy-decision trail and preference hints for that session and recording a false 'not truncated' state. This is the durable archive path (build_frozen_snapshot feeds serialize_snapshot to storage), so the loss is permanent, not transient. CODING_GUIDELINES §7 says Internal/invariant failures must be logged loudly; here a panic-induced poison is swallowed with no warn/error event.
|
||
|
||
```rust
|
||
let annotations = state
|
||
.annotations
|
||
.lock()
|
||
.map(|guard| guard.clone())
|
||
.unwrap_or_default();
|
||
...
|
||
let (audit_entries, audit_truncated) = state
|
||
.audit_log
|
||
.lock()
|
||
.map(|guard| (guard.entries(), guard.truncated))
|
||
.unwrap_or_default();
|
||
```
|
||
|
||
**Fix.** On a poisoned lock, recover the guard via `err.into_inner()` (the data is still readable and is what you want to archive) rather than discarding it, and emit `tracing::error!` so the prior panic is observable. At minimum, log a `tracing::warn!` before falling back to empty so a silently-truncated archive is diagnosable instead of looking like a session that genuinely had no annotations or audit entries.
|
||
|
||
> **Verification.** The cited code exists verbatim. In `build_frozen_snapshot` (lines 173-183 of `/tidal/src/session/snapshot.rs`):
|
||
|
||
```rust
|
||
let annotations = state
|
||
.annotations
|
||
.lock()
|
||
.map(|guard| guard.clone())
|
||
.unwrap_or_default(); // line 177: substitutes [] silently on poison
|
||
|
||
let (audit_entries, audit_truncated) = state
|
||
.audit_log
|
||
.lock()
|
||
.map(|guard| (guard.entries(), guard.truncated))
|
||
.unwrap_or_default(); // line 183: substitutes ([], false) silently on poison
|
||
```
|
||
|
||
Both `annotations` and `audit_log` are `std::sync::Mutex` fields in `SessionState` (state.rs lines 44, 50). `Mutex::lock()` returns `Err(PoisonError)` …
|
||
|
||
### W37. Non-atomic check-then-act on total_slots in UsearchIndex::insert (TOCTOU)
|
||
|
||
**storage-core · CONCURRENCY & ASYNC SAFETY** · `tidal/src/storage/vector/usearch_index.rs:126-136` · verifier **CONFIRMED** (82)
|
||
|
||
UsearchIndex is documented as the production HNSW backend and the trait requires `Send + Sync` because 'Concurrent inserts and searches are expected in production'. insert() reads `self.inner.contains(id)` into `is_new`, then unconditionally `add`s, then conditionally `fetch_add(1)`s total_slots. The contains()/add()/fetch_add() trio is a non-atomic read-modify-write. Two threads inserting the SAME new key both observe is_new=true and both increment, over-counting total_slots; conversely interleavings can under-count. total_slots drives len(), tombstone_ratio(), and registry index_stats(), so the corruption feeds compaction-trigger decisions and footprint reporting (not search results, hence WARNING not CRITICAL). It is silently wrong with no detection.
|
||
|
||
```rust
|
||
let is_new = !self.inner.contains(id);
|
||
|
||
self.inner
|
||
.add(id, embedding)
|
||
.map_err(|e| VectorError::Backend(format!("USearch insert failed: {e}")))?;
|
||
|
||
if is_new {
|
||
self.total_slots.fetch_add(1, Ordering::Relaxed);
|
||
}
|
||
```
|
||
|
||
**Fix.** Derive the total-slot count from a source that cannot race, or serialize the insert critical section. Simplest correct option: drop the separate counter and compute len() as `inner.size() + tombstone_count`, tracking tombstones with a single fetch_add inside delete() (which is already a true post-contains() remove). Alternatively guard insert with a short Mutex so contains+add+increment is atomic. At minimum, document that total_slots is unreliable under concurrent same-key inserts so compaction logic does not trust it.
|
||
|
||
> **Verification.** The cited code exists at lines 122-138 of `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/storage/vector/usearch_index.rs` and matches the quoted evidence exactly. The TOCTOU mechanism is real: `insert()` calls `self.inner.contains(id)` into `is_new` (line 127), then `self.inner.add(id, embedding)` (lines 129-131), then conditionally `self.total_slots.fetch_add(1, Ordering::Relaxed)` (line 134). Two concurrent threads inserting the same new key both observe `is_new=true` before either's `add()` completes, and both increment `total_slots`, overcounting by 1. `len()` (line 311) returns `self.total_slots.load(Ordering::Relaxed)`, …
|
||
|
||
### W38. FilterResult::into_predicate silently truncates u64 entity IDs to u32 in release builds
|
||
|
||
**storage-core · CORRECTNESS & LOGIC** · `tidal/src/storage/indexes/filter/result.rs:64-72` · verifier **CONFIRMED** (97)
|
||
|
||
into_predicate() builds a closure `|id: u64| bitmap.contains(id as u32)` guarded only by a `debug_assert!`. In release builds (production), an EntityId above u32::MAX wraps via `id as u32` and aliases onto a low ID, so the predicate returns a WRONG membership answer for that candidate — a blocked/hidden/saved item could be admitted or excluded incorrectly. The write path in db/items.rs at least emits a tracing::warn! when an ID exceeds u32::MAX; this read/query path silently aliases with no log. roaring bitmaps are inherently u32, so this is a documented single-node universe limit (~4B), but the read side should fail loud or skip rather than alias.
|
||
|
||
```rust
|
||
Self::Bitmap(bitmap) => Box::new(move |id: u64| {
|
||
debug_assert!(u32::try_from(id).is_ok(), "EntityId out of u32 range");
|
||
bitmap.contains(id as u32)
|
||
}),
|
||
```
|
||
|
||
**Fix.** Replace `id as u32` with a checked conversion: `u32::try_from(id).is_ok_and(|n| bitmap.contains(n))`. An out-of-range id then correctly returns false (not in any u32 bitmap) instead of aliasing onto an unrelated low ID. Apply the same checked pattern to the cardinality/contains casts on every read path that consumes EntityId against a roaring bitmap.
|
||
|
||
> **Verification.** The code at result.rs lines 64-72 exists exactly as cited. `into_predicate()` builds a closure with `bitmap.contains(id as u32)` guarded only by `debug_assert!(u32::try_from(id).is_ok(), ...)`, which is a no-op in release builds. An id above u32::MAX will be truncated via the as-cast, aliasing onto a low ID and producing a wrong membership answer with no log. The asymmetry is confirmed by db/items.rs lines 114-118, which does `let id_u32 = id.as_u64() as u32; if id.as_u64() > u64::from(u32::MAX) { tracing::warn!(..., "entity ID exceeds u32::MAX; universe bitmap entry will collide with a lower ID") }` — the write side warns loudly, the …
|
||
|
||
### W39. FjallBackend::scan_prefix eagerly materializes the entire prefix range into a Vec
|
||
|
||
**storage-core · PERFORMANCE & RESOURCE MANAGEMENT** · `tidal/src/storage/fjall.rs:77-89` · verifier **CONFIRMED** (93)
|
||
|
||
scan_prefix collects every matching (key,value) pair into a Vec<(Vec<u8>,Vec<u8>)> before returning the iterator, to avoid holding a fjall snapshot across the iteration boundary. For per-entity prefix scans this is bounded, but the codebase repeatedly calls scan_prefix(&[]) — a FULL keyspace scan — during recovery rebuilds (registry.rs:260 rebuild_from_store in this zone, plus signals/checkpoint, cohort/checkpoint, db/users, db/session_restore). On restart this loads the entire items/users/creators keyspace (target 10M items, each with metadata + embeddings) into a single in-memory Vec at once, with no streaming and no capacity hint, spiking peak RSS exactly when the process is most fragile (recovery). A 3am restart on a large dataset can OOM.
|
||
|
||
```rust
|
||
let entries: Vec<_> = self
|
||
.keyspace
|
||
.prefix(prefix)
|
||
.map(|guard| {
|
||
let (k, v) = guard.into_inner().map_err(|e| map_fjall_err(&e))?;
|
||
Ok((k.to_vec(), v.to_vec()))
|
||
})
|
||
.collect();
|
||
|
||
Box::new(entries.into_iter())
|
||
```
|
||
|
||
**Fix.** Stream the scan instead of buffering: own the fjall prefix iterator inside a custom Iterator adapter (move the keyspace Arc clone into it) so entries are yielded lazily and the snapshot lives for the iterator's lifetime, eliminating the full-keyspace materialization. If a true streaming snapshot is hard with the fjall API, at minimum batch the rebuild scans (range-paginate by key) so peak memory is bounded rather than O(keyspace).
|
||
|
||
> **Verification.** The code at fjall.rs:77-88 is exactly as quoted: `scan_prefix` calls `self.keyspace.prefix(prefix).map(...).collect()` into a `Vec<_>` before boxing it as an iterator. The full materialization is real and unguarded.
|
||
|
||
Multiple recovery paths call `items_engine().scan_prefix(&[])`:
|
||
- `state_rebuild.rs:159` (`rebuild_suggestion_index`) and `state_rebuild.rs:284` (`rebuild_item_indexes`) — both want only non-EMB `Tag::Meta` rows, but because materialization happens first, ALL rows including embedding rows (`Tag::Meta, "EMB:slot_name"` keys per `vector/lifecycle/serde.rs`) are loaded into the Vec before any filtering occurs.
|
||
- …
|
||
|
||
### W40. Hashtag preprocessing rewrites '#' inside quoted phrases and field-scoped terms, altering query semantics
|
||
|
||
**text · CORRECTNESS & LOGIC** · `tidal/src/text/query.rs:75-86` · verifier **CONFIRMED** (82)
|
||
|
||
`preprocess_query` is a flat character scan with no awareness of Tantivy syntax. Any `#` immediately followed by an ASCII alphanumeric is stripped — including inside a quoted phrase. A user phrase query `"#nofilter wins"` becomes `"nofilter wins"`, and `c#tutorial` becomes `ctutorial`; the stripped `#` silently changes phrase/term semantics. For a search surface that advertises exact-phrase support (`"exact phrase"` in the doc comment), mutating the contents of a quoted phrase is a real correctness smell that produces wrong matches for legitimate text containing '#'.
|
||
|
||
```rust
|
||
while let Some(ch) = chars.next() {
|
||
if ch == '#' && chars.peek().is_some_and(char::is_ascii_alphanumeric) {
|
||
// Valid hashtag prefix -- skip the '#', let the word through.
|
||
} else {
|
||
result.push(ch);
|
||
}
|
||
}
|
||
```
|
||
|
||
**Fix.** Track quote state (and ideally only strip a leading '#' at a token boundary, i.e. start-of-string or after whitespace) so hashtag normalization never touches characters inside a `"..."` phrase or after a `field:` prefix. A `#` that is not at a word boundary should be left untouched.
|
||
|
||
> **Verification.** The code at `/Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/text/query.rs` lines 75-86 matches the cited evidence exactly. The `preprocess_query` function is a flat character scan: `if ch == '#' && chars.peek().is_some_and(char::is_ascii_alphanumeric)` at line 79 strips any `#` followed by an alphanumeric, with no guard for quote state or word-boundary position. This means:
|
||
|
||
1. `"#nofilter wins"` → `"nofilter wins"`: the `#` inside a quoted phrase is stripped. In practice, Tantivy's tokenizer would discard `#` from phrase tokens during indexing anyway, so the phrase match result is likely identical — this partially mitigates the …
|
||
|
||
|
||
---
|
||
|
||
## 🟦 Suggestions
|
||
|
||
_31 standalone suggestions + 23 findings the verifier downgraded to suggestion-level._
|
||
|
||
- **S1.** `tidal/src/db/communities.rs:108-126` (db-entities-ops) — leave_community / rejoin_community perform a non-atomic read-modify-write across with_row + put on the membership row — `with_row` takes a DashMap read guard, runs the closure, and drops the guard before returning the snapshot. The row is then re-inserted via `membership_registry.put(updated)` in a separate, unsynchronized step. Between the `with_row` guard … _(was WARNING; DOWNGRADE 72)_
|
||
- **S2.** `tidal/src/db/relationships.rs:50-52` (db-entities-ops) — Hide relationship truncates the item EntityId to u32 with no overflow guard, so item IDs > u32::MAX hide the wrong item — For `RelationshipType::Hide`, the target item id is truncated `to.as_u64() as u32` before insertion into the hidden-items bitmap (and again in delete_relationship line 99, and in the rebuild at state_rebuild.rs:114). Hides are … _(was WARNING; DOWNGRADE 90)_
|
||
- **S3.** `tidal/src/db/capabilities.rs:36` (db-lifecycle) — Duration-to-u64-nanos cast truncates instead of saturating, so a very large capability TTL can expire far earlier than requested — `Duration::as_nanos()` returns `u128`; `d.as_nanos() as u64` truncates the high 64 bits rather than saturating. For any TTL exceeding ~584 years (u64::MAX nanoseconds) the value wraps to a small number, so `now_ns.saturating_add(small)` … _(was WARNING; DOWNGRADE 85)_
|
||
- **S4.** `tidal/src/db/metrics/mod.rs:34-39` (db-metrics) — now_unix_nanos saturates but the writer of last_checkpoint_ns wraps — split casting policy — now_unix_nanos() was deliberately created (per its own doc comment, lines 23-33) as the one place the 'now in nanos' pattern lives, and it correctly saturates with `u64::try_from(d.as_nanos()).unwrap_or(u64::MAX)`. But the value it is … _(was WARNING; DOWNGRADE 88)_
|
||
- **S5.** `tidal/src/db/text_syncer.rs:71-85` (db-query-signals) — Syncer thread spawn failure swallowed by `.ok()` — writes accepted but never committed to index — `std::thread::Builder::spawn(...).ok()` discards a thread-spawn failure (e.g. EAGAIN / resource exhaustion). On failure `handle` is `None`, but the bundle is still returned with a live `write_tx` (`Some(tx)`) and an `Arc<TextIndex>`. The … _(was WARNING; DOWNGRADE 88)_
|
||
- **S6.** `tidal/src/db/state_rebuild.rs:472-477` (db-recovery) — Cohort signal state is only durable via the 30s checkpoint thread; a crash loses all cohort signals since the last checkpoint — The periodic checkpoint thread is the sole durability mechanism for cohort signal state. Unlike the global signal ledger (which is WAL-logged at write time, restored from checkpoint, and then has post-checkpoint WAL events replayed via … _(was WARNING; DOWNGRADE 88)_
|
||
- **S7.** `tidal/src/db/state_rebuild.rs:528-533, 555-575` (db-recovery) — Text-index resync stamps the signal-stream WAL seq as the text index high-water mark — `poll_text_index_health` rebuilds the item text index by calling `rebuild_text_index_from_engine(idx, items_engine, last_wal_seq.load(Acquire))`, which forwards that value to `TextIndex::rebuild_from(.., last_seq)` and commits it as the … _(was WARNING; DOWNGRADE 82)_
|
||
- **S8.** `tidal/src/entities/co_engagement.rs:108-136` (entities) — Co-engagement capacity bound is not enforced atomically under concurrent writers — record_positive reads self.edges.len(), computes over = len - capacity, snapshots all edges into a Vec, partitions, then removes the weakest `over` keys. The len() read, the snapshot, and the removals are three separate operations on a … _(was WARNING; DOWNGRADE 72)_
|
||
- **S9.** `tidal/src/entities/user_state.rs:352-361` (entities) — in_progress_items truncates u64 item IDs to u32, can fabricate phantom in-progress matches — The completion map is keyed by full u64 item IDs (record_completion/get_completion take item_id: u64), but in_progress_items casts each key to u32 via `iid as u32` when inserting into the result RoaringBitmap. Any item ID >= 2^32 collapses … _(was WARNING; DOWNGRADE 85)_
|
||
- **S10.** `tidal/src/query/executor/pipeline.rs:229-233` (query-executor) — Executor relies on a silent u64→u32 EntityId truncation that the write path does not validate — Every bitmap membership check in the executor casts `id.as_u64() as u32` (pipeline.rs:232,251,355,359,371,384,388,408,422; candidate_gen.rs:34,99; personalization.rs:112). The inline comment asserts this is "intentional and correct … _(was WARNING; DOWNGRADE 82)_
|
||
- **S11.** `tidal/src/query/executor/pipeline.rs:306-346` (query-executor) — Stage 2.4 geo post-filter re-reads + deserializes item metadata from storage per candidate, separately from Stage 3 — The Stage 2.4 geographic post-filter does a `storage.get(encode_key(eid, Tag::Meta, ...))` plus full `deserialize_item_metadata` for every surviving candidate, purely to read latitude/longitude. Stage 3 (`stage3_score`, mod.rs:337-356) … _(was WARNING; DOWNGRADE 82)_
|
||
- **S12.** `tidal/src/ranking/diversity/selector.rs:107-142` (ranking-core) — DiversitySelector relaxation stages clone the full remaining candidate Vec twice on the hot path — Stage 1 and Stage 2 each build a `remaining: Vec<_>` by filtering and `.cloned()`-ing every not-yet-accepted ScoredCandidate, then `greedy_select` clones each accepted candidate again into its output. ScoredCandidate owns a … _(was WARNING; DOWNGRADE 80)_
|
||
- **S13.** `tidal/src/ranking/executor/mod.rs:522-552` (ranking-core) — ProfileDecay multiplicative factor silently flips sign of negative composite scores — The composite `additive = base + boost_sum + co_eng_boost - penalty_sum` can be negative (heavy penalties, or an `AlphabeticalAsc`/`DateSaved` sort_base of -1.0 / NEG_INFINITY with no boosts). ProfileDecay then multiplies it by `factor = … _(was WARNING; DOWNGRADE 72)_
|
||
- **S14.** `tidal/src/ranking/executor/helpers.rs:195-221` (ranking-core) — normalize() collapses all scores to 1.0 when the spread is sub-epsilon, masking real (tiny) ranking differences — normalize uses `range < f64::EPSILON` to detect 'all equal' and sets every score to 1.0. f64::EPSILON is ~2.2e-16, but it's compared against an absolute `range = max - min` whose magnitude depends on the score scale. For large scores (e.g. … _(was WARNING; DOWNGRADE 78)_
|
||
- **S15.** `tidal/src/replication/control.rs:23-23` (replication-core) — ShardStats.replication_lag unit (ns) is undocumented and silently coupled to DEGRADED_LAG_NS; a caller populating segment-lag would mis-classify region health — recompute_region_health compares each value in stats.replication_lag against DEGRADED_LAG_NS = 5_000_000_000 (5 seconds in nanoseconds, control.rs:59,187-191), so replication_lag values MUST be nanoseconds. But the field `replication_lag: … _(was WARNING; DOWNGRADE 88)_
|
||
- **S16.** `tidal/src/replication/crdt/signal_state.rs:187-212` (replication-crdt) — CrdtSignalState::merge silently keeps self.lambda and ignores other.lambda, so merging states with different decay rates is order-dependent (non-commutative) — `merge` never touches `self.lambda`, so `merge(a,b)` keeps `a.lambda` and `merge(b,a)` keeps `b.lambda`. The struct derives `PartialEq` over ALL four fields including `lambda` (line 46), and the merged per-node scores are decayed using the … _(was WARNING; DOWNGRADE 80)_
|
||
- **S17.** `tidal-server/src/router.rs:254-260` (server-core) — Every /search request forces a synchronous Tantivy reader reload on the hot path — The search handler unconditionally calls state.reload_text_index(...) before every search, on every request. The engine reload (tidal/src/db/items.rs:360-364) calls idx.reload_reader() synchronously. The state.rs doc (state.rs:142-146) … _(was WARNING; DOWNGRADE 78)_
|
||
- **S18.** `tidal/src/storage/vector/usearch_index.rs:122-138` (storage-vector) — UsearchIndex::insert has a non-atomic check-then-act on total_slots, corrupting the tombstone ratio under concurrent inserts — `insert` reads `self.inner.contains(id)` (line 127), then calls `self.inner.add` (line 129), then conditionally `total_slots.fetch_add(1)` (line 133). The three steps are not atomic. The trait doc (mod.rs:175) explicitly states concurrent … _(was CRITICAL; DOWNGRADE 88)_
|
||
- **S19.** `tidal/src/storage/vector/brute/mod.rs:185-211` (storage-vector) — BruteForceIndex::save flushes the userspace buffer but never fsyncs the file — `save` writes the header and vectors, then calls `file.flush()` (line 210), which only drains the Rust BufWriter/userspace buffer into the OS — it does not call `sync_all()`/fsync, so the data may still sit in the page cache and be lost on … _(was WARNING; DOWNGRADE 85)_
|
||
- **S20.** `tidal/src/text/collectors.rs:45-57, 79-82` (text) — AllScoresCollector resolves entity_id by hardcoded name and silently defaults missing fast field to entity 0 — `for_segment` ignores the `entity_id_field: Field` handle stored on the collector and instead resolves the fast field by the hardcoded string `"entity_id"`, then wraps it with `first_or_default_col(0)`. If any segment lacks a populated … _(was CRITICAL; DOWNGRADE 82)_
|
||
- **S21.** `tidal/src/text/writer.rs:93-138, 155-168` (text) — last_seq commit-payload recovery machinery is fed a constant 0 by the live write path, making last_committed_seq() effectively dead — `commit()` persists `last_seq` into the Tantivy payload and `last_committed_seq()` is documented as the WAL-replay point for crash recovery ('This is used on startup for crash recovery to determine the WAL replay point'). But the only … _(was WARNING; DOWNGRADE 88)_
|
||
- **S22.** `tidal/src/wal/segment.rs:201-206` (wal) — write_batch_bytes is not atomic vs. a torn write_all, and current_size desyncs from the file on partial write — write_batch_bytes does file.write_all(bytes)? then current_size += bytes.len(). If write_all writes part of the buffer and then errors (short write turned error, EINTR-then-fail, ENOSPC mid-write), bytes are already on disk but … _(was WARNING; DOWNGRADE 82)_
|
||
- **S23.** `tidal/src/wal/reader.rs:188-205` (wal) — Recovery/diagnostic scanners trust an unvalidated 32-bit payload_len before bounds-checking it — scan_segment_inner reads payload_len directly from on-disk header bytes (offset+24..28), casts u32->usize, then computes batch_end = offset + HEADER_SIZE + payload_len and checks batch_end > data.len(). On a 64-bit target the add cannot … _(was WARNING; DOWNGRADE 82)_
|
||
- **S24.** `tidal/src/cohort/types.rs:113-117` (cohort) — CohortRegistry::get allocates a fresh Arc<str> per lookup; resolver slow path pays it once per cohort per cache miss — `CohortRegistry::get(&str)` builds a new `Arc<str>` key (`let key: CohortName = Arc::from(name)` — a heap allocation plus byte copy) on every call just to probe the `DashMap`. In `CohortResolver::resolve`'s slow path (resolver.rs:84-91) …
|
||
- **S25.** `tidal/src/db/collections.rs:59-60, 95-96, 284-289` (db-entities-ops) — add_to_collection / remove_from_collection / save_search truncate item and user EntityIds to u32/key bytes with no guard, and collection keys use little-endian, breaking lexicographic scan ordering — `add_to_collection` and `remove_from_collection` truncate `item_id.as_u64() as u32` with no overflow check (same class as finding 3, lower impact since collection membership is a softer signal). Separately, persist_collection encodes the …
|
||
- **S26.** `tidal/src/db/builder.rs:297-299` (db-lifecycle) — Builder doc claims directories are created via Paths::ensure_all during initialization, but ensure_all is never called on the open path — The `open` doc comment states: 'Resolved defaults (e.g., {data_dir}/wal) are populated after validation -- the storage engine will create them via [Paths::ensure_all] during initialization.' In reality `Paths::ensure_all` is never invoked …
|
||
- **S27.** `tidal/src/db/metrics/histogram.rs:87-96` (db-metrics) — Histogram render takes a non-atomic snapshot — +Inf/buckets/sum can be mutually inconsistent under concurrent observe — render_prometheus() reads each bucket, then count, then sum, with independent Relaxed loads while observe() may be concurrently incrementing them in a non-atomic group (the per-bucket fetch_add loop in observe is not a single transaction). …
|
||
- **S28.** `tidal/src/db/query_ops.rs:34-41` (db-query-signals) — Filter-complexity guard deep-clones the entire filter tree on every query just to count nodes — `query.combined_filter()` (retrieve/types.rs:128) clones all filters — `Some(FilterExpr::And(self.filters.clone()))` for the multi-filter case — and the only use of the returned value here is `filter.node_count()`, after which the clone is …
|
||
- **S29.** `tidal/src/entities/co_engagement.rs:151-166` (entities) — top_candidates does a full O(N) scan of the entire edge map per call — top_candidates filters the whole edges DashMap (up to DEFAULT_CO_ENGAGEMENT_CAPACITY = 50K entries) by key().0 == seed_id, collects matches into a Vec, then sort+truncate. There is no per-seed adjacency index, so every related-profile …
|
||
- **S30.** `tidal/src/entities/relationship.rs:8-16` (entities) — Public extensible enums lack #[non_exhaustive] — RelationshipType (and Visibility in collection.rs:32-37) are pub enums with byte discriminants that are clearly intended to grow (Follows/Blocks/InteractionWeight/Hide/Mute today; the from_byte/as_byte codec is built for adding variants). …
|
||
- **S31.** `tidal/src/governance/community_ledger.rs:219-227` (governance) — decay_score_at / explain / aggregate_digest do a redundant second HashMap lookup per contributor after sorting keys — After collecting `cell.contributors.keys()` into a Vec and sorting, the fold re-looks-up each key with `cell.contributors.get(k)` to fetch the value it already had a reference to. Since the keys come from the same map under the same read …
|
||
- **S32.** `tidal-net/src/circuit_breaker.rs:55-59` (net) — Circuit breaker fails OPEN on poisoned lock, which can mask a peer that should be quarantined — check() returns Ok(()) (allow the request) when the state Mutex is poisoned. A poisoned lock here only happens if a prior holder panicked while mutating CircuitState — which for this tiny critical section is essentially impossible, so the …
|
||
- **S33.** `tidal-net/src/error.rs:6-37` (net) — Public error enum GrpcTransportError lacks #[non_exhaustive] — GrpcTransportError is a pub enum returned across the crate boundary (re-exported from lib.rs:40). Adding a new variant later is a breaking change for any external matcher. This is a known-deferred low-severity item per the review brief; …
|
||
- **S34.** `tidal/src/query/executor/user_filter.rs:99-266` (query-executor) — Four near-identical deferred-filter extractor functions duplicate the same recursion skeleton — `collect_user_state_filters`, `collect_signal_threshold_filters`, `collect_geo_filters`, and `collect_collection_filters` are byte-for-byte identical except for which leaf variants they push: each matches its target variants, recurses into …
|
||
- **S35.** `tidal/src/query/retrieve/types.rs:372-378` (query-retrieve) — for_creator truncates creator IDs to u32 inside an infallible builder — RetrieveBuilder::for_creator pushes `FilterExpr::CreatorEq(creator_id.as_u64() as u32)`, silently truncating any creator ID above u32::MAX. The doc comment honestly flags this as a known systemic bitmap-index limitation, so it is a …
|
||
- **S36.** `tidal/src/query/search/executor_helpers.rs:19-42` (query-search) — Duplicated user-state filter extraction diverged between SEARCH and RETRIEVE copies — `extract_user_state_filters`/`collect_user_state_filters` are duplicated verbatim-in-spirit between this file and query/executor/user_filter.rs lines 93-120. They have already drifted: the user_filter.rs copy collects `SocialGraph` (line …
|
||
- **S37.** `tidal/src/ranking/profile.rs:47-100, 105-113, 124-131` (ranking-core) — Public ranking enums (Sort, CandidateStrategy, SignalAgg, ProfileError) lack #[non_exhaustive] — Sort, CandidateStrategy, and SignalAgg are re-exported from the crate root (mod.rs:12-16) and are serde-(de)serialized from stored schema/API payloads. They are explicitly designed to grow (Ratio/RelativeVelocity reserved for M3; new sort …
|
||
- **S38.** `tidal/src/ranking/executor/scoring.rs:47-59, 426-434` (ranking-core) — DateSaved/New timestamp & id-as-f64 casts lose precision past 2^53 without a guard — `Sort::New` casts `entity_id.as_u64() as f64` and `Sort::DateSaved` casts a u64 nanosecond save-timestamp `ts as f64`. Both lose integer precision above 2^53 (~9.0e15). For DateSaved, 2^53 ns is only ~104 days of wall-clock nanos beyond …
|
||
- **S39.** `tidal/src/ranking/executor/scoring.rs:333-337` (ranking-executor) — Rising ratio can explode when long-window velocity is tiny but above f64::EPSILON — score_rising guards only `long < f64::EPSILON` (~2.2e-16) before dividing `short / long`. A long-window velocity that is small but above epsilon (e.g. a single event smeared across the 24h window producing a value like 1e-6) makes `short / …
|
||
- **S40.** `tidal/src/ranking/executor/scoring.rs:426-434` (ranking-executor) — score_date_saved silently ignores user_id mismatch and discards timestamp precision via u64->f64 — score_date_saved casts a u64 nanosecond save timestamp (~1.7e18) to f64 for use as the sort key. f64 has 52 bits of mantissa, so timestamps that differ by less than ~512ns at this magnitude collapse to the same f64 and become ties …
|
||
- **S41.** `tidal/src/replication/crdt/hlc.rs:89-111` (replication-crdt) — HlcTimestamp is a public struct with all-public fields, allowing construction of timestamps that violate the ms-granularity / supported-range invariants the module documents — `HlcTimestamp` is `pub` with `pub wall_ns`, `pub logical`, `pub node_id`. The module documents two invariants on `wall_ns`: it is always a multiple of 1_000_000 (ms granularity, lines 92-95) and it must stay <= MAX_WALL_MS*1e9 for total …
|
||
- **S42.** `tidal/src/replication/shipper.rs:198-203` (replication-data) — Per-peer segment bytes cloned on every send instead of shared via Arc — Each `send_segment` call clones the full segment byte buffer into a fresh `WalSegmentPayload.bytes: Vec<u8>` (line 201, `segment_bytes.clone()`). The `bytes_cache` already deduplicates the disk read across peers at the same lag, but the …
|
||
- **S43.** `tidal/src/schema/signal.rs:112-130` (schema) — Public extensible enums lack #[non_exhaustive] — Known-deferred maintainability item, confirmed present. The public, plausibly-growing enums DecayModel (signal.rs:113), Window (signal.rs:177), EntityKind (entity.rs:50), TextFieldType (validation/text.rs:17), and DecaySpec …
|
||
- **S44.** `tidal/src/schema/validation/policies.rs:24-35` (schema) — AgentPolicy is a fully-public struct with unvalidated numeric limits and a magic-zero sentinel — AgentPolicy exposes all fields pub with no constructor, so callers build it struct-literal style. build() validates only the referenced signal names — it never validates max_session_duration or max_signals_per_session. A zero Duration …
|
||
- **S45.** `tidal-server/src/scatter_gather.rs:589-593, 715-719, 715` (server-cluster) — QueryStats.candidates_after_diversity can overstate the actual returned count — In `scatter_gather_retrieve`, `limit = query.limit.min(all_items.len())` is computed AFTER `enforce_max_per_creator` has already shrunk `all_items`, and `candidates_after_diversity` is set to `limit` (line 619 region). In …
|
||
- **S46.** `tidal-server/src/error.rs:7-28` (server-core) — Public ServerError enum is not #[non_exhaustive] — ServerError is a pub enum re-exported through the lib crate (lib.rs exposes error as a pub module) and is matched on by status_from_error in router.rs. Without #[non_exhaustive], any future variant is a breaking change for downstream …
|
||
- **S47.** `tidal-server/src/error.rs:9-21` (server-core) — Two ServerError variants both wrap std::io::Error with divergent Display — ServerError has both Io { path, source } (constructed only via ServerError::io for config reads) and Http(#[from] std::io::Error). Because Http carries the #[from], every bare `?` on an io::Error (e.g. TcpListener::bind, axum::serve in …
|
||
- **S48.** `tidal/src/session/snapshot.rs:167,205-224` (session) — build_frozen_snapshot derives closed_at_ns from a fresh clock independent of duration_ms — build_frozen_snapshot receives duration_ms as a precomputed argument but computes closed_at_ns independently via crate::schema::Timestamp::now().as_nanos(). The caller (db/sessions.rs:167) computes duration_ms from started_at.elapsed() at …
|
||
- **S49.** `tidal/src/schema/signal.rs:176-183` (signals) — Public enum Window and several pub fns lack #[non_exhaustive] / are broader than needed (known-deferred) — Confirming the known-deferred API-surface item for completeness: `Window` is a `pub enum` consumed across the signals warm tier and exposed to embedders, but is not `#[non_exhaustive]`, so adding a future window variant is a breaking …
|
||
- **S50.** `tidal/src/storage/vector/brute/mod.rs:185-211` (storage-core) — BruteForceIndex::save flushes but does not fsync; torn-write on crash leaves a corrupt .bfvi — save() writes header + vectors and calls file.flush() (userspace buffer flush) but never file.sync_all(). A crash mid-save or before the OS page cache is persisted leaves a truncated/torn index file whose declared count exceeds the bytes …
|
||
- **S51.** `tidal/src/storage/vector/brute/mod.rs:113-128` (storage-vector) — BruteForceIndex::search materializes and sorts all N results instead of a bounded top-k heap — Both `search` and `filtered_search` collect a `VectorSearchResult` for every stored vector into a Vec, full-`sort_by` the whole thing O(N log N), then `truncate(k)`. For the documented use (correctness baseline, small datasets) this is …
|
||
- **S52.** `tidal/src/text/collectors.rs:24-27` (text) — AllScoresCollector.entity_id_field is a public, never-read field — `entity_id_field: Field` is `pub` and is set by every caller (pipeline.rs:292, all tests) but is never read anywhere in the implementation — `for_segment` resolves the column by string instead (see text-1). It is misleading public API: …
|
||
- **S53.** `tidal/src/db/text_syncer.rs:71-86` (text) — Background syncer spawn failure is silently swallowed, yielding a live channel with no consumer — Outside the strict text/ zone but directly governing the text syncer's liveness: `spawn(...).ok()` discards a thread-spawn failure, then the bundle still returns `write_tx: Some(tx)`. If the OS refuses the thread, the producer keeps …
|
||
- **S54.** `tidal/src/wal/mod.rs:396-397` (wal) — Checkpoint timestamp truncates u128 nanos to u64 via `as`, wrapping in year 2554 — ts is Duration::as_nanos() (u128); `let ts_u64 = ts as u64` silently truncates. The checkpoint ts field is informational (not used for replay correctness, only diagnostics/observability), so impact is low, but the silent `as` truncation is …
|
||
|
||
|
||
---
|
||
|
||
## Refuted / not actioned (14)
|
||
|
||
_Raised by a reviewer, then dismissed by the verifier after re-reading the code. Listed so reviewers can see what was considered._
|
||
|
||
- ~~WAL backpressure check is silently skipped when the WAL mutex is poisoned, removing the only overload guard on the signal write path~~ `tidal/src/db/signals.rs:107-121` (db-query-signals, was WARNING) — The code at signals.rs lines 107-121 and 185-194 exactly matches what the reviewer cited: `if let Ok(guard) = self.wal.lock() && let Some(wal) = guard.as_ref()` — so the pattern is real. However, the threat model is wrong. Mutex poisoning requires a thread to …
|
||
- ~~deserialize_entity length math can wrap on 32-bit targets, defeating the bounds check~~ `tidal/src/entities/mod.rs:172-180` (entities, was WARNING) — The code at lines 172-180 of /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/entities/mod.rs is exactly as cited: `let emb_len = u32::from_le_bytes([...]) as usize; pos += 4; let byte_len = emb_len * 4; if pos + byte_len > bytes.len() { return …
|
||
- ~~Server trusts wire-supplied event_count without cross-checking the segment bytes~~ `tidal-net/src/server.rs:43-56` (net, was WARNING) — The finding is a false positive. The receiver (`tidal/src/replication/receiver.rs`) calls `apply_payload(&payload.bytes, shard_id, ...)` — it passes only the raw bytes, not the `WalSegmentPayload.event_count` field. Inside `apply_payload` (lines 133-211), …
|
||
- ~~Cohort rescore discards gates and base/personalization/relevance contributions~~ `tidal/src/query/executor/helpers.rs:138-193` (query-retrieve, was WARNING) — The central claim — "a candidate that the profile's gate would have excluded can resurface in cohort mode" — is false. Gates and excludes are applied inside `score_personalized` (ranking/executor/mod.rs lines 263-269), which calls `passes_excludes` and …
|
||
- ~~score_personalized truncates u64 entity IDs to u32 for boost lookup — silent collisions / missed boosts above u32::MAX~~ `tidal/src/ranking/executor/mod.rs:281-299` (ranking-core, was WARNING) — The `as u32` truncation at `ranking/executor/mod.rs:281` (`let item_id = entity_id.as_u64() as u32`) is consistent with a system-wide constraint, not a ranking-specific bug. Three pieces of evidence:
|
||
|
||
1. `creator_interaction_boosts` and `preference_boosts` …
|
||
- ~~Two-layer dedup is order-fragile and Layer 2 is effectively dead given the ship path~~ `tidal/src/replication/session_bridge.rs:329-358` (replication-data, was WARNING) — The reviewer's core claim — "Layer 2 can never independently reject anything the current ship path produces" — is wrong because it ignores legacy events (session_seqno: None).
|
||
|
||
In ship() at lines 279-286, the filter is `seqno.is_none_or(|s| s > current_hwm)`. …
|
||
- ~~total_candidates reconciliation infers topology from result overlap — misclassifies replicated clusters as disjoint~~ `tidal-server/src/scatter_gather.rs:423-436, 562, 585-586` (server-cluster, was WARNING) — The cited code at lines 423-436 and 562, 585-586 exists exactly as described, but the claim that `overlap_detected` is an unreliable proxy for topology does not hold in the current implementation.
|
||
|
||
`SimulatedCluster.write_item_with_metadata` …
|
||
- ~~maybe_rotate is multi-CAS and non-atomic: an interleaved writer can drop a minute/hour/day rollup~~ `tidal/src/signals/warm.rs:352-450, 460-481` (signals, was WARNING) — The reviewer's race scenario is mechanically impossible given the code structure. The hour CAS at lines 407-418 is only reachable by a thread that has already won the minute CAS at lines 365-371. Both CAS operations share the same `last_min` value read at …
|
||
- ~~Checkpoint snapshots DashMap shard-by-shard with no global lock; concurrent writes to already-snapshotted shards are silently dropped from the checkpoint~~ `tidal/src/signals/checkpoint/mod.rs:62-90` (signals, was WARNING) — The reviewer's core claim is that `state_rebuild.rs` reads `last_wal_seq` AFTER the "trim/iterate region," meaning entries already snapshotted but written after the sequence capture would be silently lost. In the actual code at `run_checkpoint_thread` (lines …
|
||
- ~~apply_crdt_state force-sets the SAME merged score into every lambda index; the per-rate machinery is latently incorrect for multi-rate signals~~ `tidal/src/signals/ledger/core.rs:392-402` (signals, was WARNING) — The cited code at core.rs:396-402 exists exactly as quoted and the comment does acknowledge the CRDT only tracks the primary rate. However the finding's harm scenario — writing the rate-0 merged score into rate-1/rate-2 slots — is currently impossible. …
|
||
- ~~Recovery rebuild re-normalizes nothing but silently trusts un-normalized stored vectors, breaking the L2==cosine invariant on reopen~~ `tidal/src/storage/vector/registry.rs:285-325` (storage-vector, was WARNING) — The finding's central empirical claim — that the test at registry.rs:597-606 "deliberately stores NON-normalized vectors" — is factually wrong. The actual test vectors are `[1.0,0.0,0.0,0.0]`, `[0.0,1.0,0.0,0.0]`, `[0.0,0.0,1.0,0.0]`: these are standard basis …
|
||
- ~~filtered_search accepts ef_search but silently ignores it without the warning that search() emits~~ `tidal/src/storage/vector/usearch_index.rs:192-216` (storage-vector, was WARNING) — Reading /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/storage/vector/usearch_index.rs lines 192–224 shows that `filtered_search` at line 201-207 contains exactly the same `if ef_search != self.config.ef_search { tracing::warn!(...) }` guard that …
|
||
- ~~Unbounded outbox channel lets the text syncer backlog grow without bound while commits are stalled~~ `tidal/src/text/syncer.rs:45-74, 213-264` (text, was WARNING) — The finding's central mechanism is wrong. In `syncer.rs:213-232`, when a commit fails, `retry_not_before` is set but the loop does NOT block — it continues calling `self.rx.recv_timeout(poll)` (poll is capped at 100ms, line 208), receives each `PendingWrite`, …
|
||
- ~~Checkpoint is written from the caller thread while the writer thread mutates segments — no ordering barrier~~ `tidal/src/wal/mod.rs:392-399` (wal, was WARNING) — The reviewer's claimed vulnerability — that checkpoint() can be called with a seq whose batch is still buffered and not fsynced — does not exist in the current code because the invariant is structurally enforced, not merely convention.
|
||
|
||
Key evidence:
|
||
|
||
1. …
|
||
|
||
|
||
---
|
||
|
||
## Praise (130)
|
||
|
||
- _wal_ — Batch wire format is genuinely robust: 64-byte cache-line header, BLAKE3 over header[0..32] || payload (covers shard/region/governance bytes), and two-phase decode (magic+bounds, then checksum) — the proptest corrupt_any_payload_byte_fails proves every payload byte is checksum-protected.
|
||
- _wal_ — Crash-recovery torn-tail handling is well-separated by intent: scan_segment_readonly never mutates (safe for concurrent export/diagnostics) while recover_segment truncates-and-fsyncs, and is explicitly documented as single-threaded-at-open-only.
|
||
- _wal_ — Sequence arithmetic during recovery uses checked_add (surfacing a forged first_seq near u64::MAX as Corruption, not a wrap/panic) and saturating_add for the next_seq floor — backed by the recover_sequence_overflow_is_corruption_not_panic test.
|
||
- _wal_ — The shared flush_batch / partition_dedup routines deliberately funnel both the steady-state loop and the shutdown drain through one path so a write error can never drop a caller's reply channel on one path but not the other, with targeted fault-injection tests (flush_batch_error_notifies_callers, shutdown_drain_notifies_callers_on_write_error) proving it.
|
||
- _wal_ — Durability primitives are correct where it counts: parent-directory fsync after segment creation/rotation and after checkpoint rename, write-temp-then-rename for the checkpoint, and per-record fsync + per-record BLAKE3 trailer on the session journal with a clean distinction between a torn tail (normal) and a checksum mismatch (counted corruption).
|
||
- _storage-core_ — fjall.rs write_batch routes every op through a single OwnedWriteBatch with an excellent comment explaining why per-op insert would tear, plus a thread-local fault-injection hook and a catch_unwind test that proves all-or-nothing on real crash — exactly the 3am-incident durability rigor the project demands.
|
||
- _storage-core_ — bitmap.rs length-prefixes the field name in the persisted key (BMP: + u16 len + field + value) and has a regression test proving field='a:b'/value='c' no longer collides with field='a'/value='b:c' — a subtle key-encoding ambiguity caught and closed correctly.
|
||
- _storage-core_ — range.rs is_empty_interval() pre-checks inverted/excluded-equal bounds before calling BTreeMap::range, turning a reachable panic from the public filter API into a clean empty result, with a proptest that hammers all 9 Bound-kind combinations for no-panic.
|
||
- _storage-core_ — registry.rs rebuild_from_store reconstructs the derived ANN index from the durable source-of-truth embeddings on restart, reads dimensionality from the stored header, cross-checks schema-declared dims, and skips non-UTF-8 slot names without aborting the whole rebuild — a complete, idempotent recovery path.
|
||
- _storage-core_ — FilterResult exposes both try_into_bitmap (typed error) and into_bitmap (logged degrade) for the Predicate variant instead of panicking, correctly reasoning that both variants are publicly constructible so a panic would be reachable from outside the crate.
|
||
- _storage-vector_ — The L2==cosine equivalence is documented and enforced at exactly one place (normalize.rs) and exercised by idempotence + direction-preservation proptests — the invariant is real, not aspirational.
|
||
- _storage-vector_ — rebuild_from_store correctly treats the ANN index as derived state and reconstructs it from the durable entity-store embeddings on reopen, with a proptest-backed search-after-rebuild assertion — the recovery gap it fixes is well understood and documented.
|
||
- _storage-vector_ — Shared encoding/validation helpers (validate_dimensions, read_u32_le/u64/f32) live in one place in mod.rs and are reused by both index impls and the serde layer, eliminating the dimension-check and LE-decode duplication.
|
||
- _storage-vector_ — tombstone_ratio / len vs len_live cleanly separates USearch's live size() from total occupied slots, giving an honest compaction signal instead of pretending removes reclaim space.
|
||
- _storage-vector_ — validate_dimensions is checked up front on every insert/search hot path before any distance work, so a malformed-dimension query fails fast with a typed DimensionMismatch rather than producing garbage distances.
|
||
- _signals_ — decay.rs is exemplary: a single pure `forward_decay_step` kernel with mul_add fusion, an explicit out-of-order pre-decay branch that does not regress the timestamp, and a proptest proving order-independence — exactly the 'decay is a type, not a formula you call' mandate, with the prior five-copy drift retired.
|
||
- _signals_ — HotSignalState is a genuinely correct lock-free design: 64-byte cache-line layout enforced by const assertions, per-rate CAS loops, and a carefully documented Acquire/Release/AcqRel ordering scheme with a restore() that stores scores before the timestamp so readers never see a fresh timestamp with stale scores.
|
||
- _signals_ — Checkpoint integrity is real: BLAKE3 over length-prefixed payloads hashed in sorted-key order (matching scan order on restore), verified before any DashMap insert, with a clean fall-back to full WAL replay on mismatch — and a clone-free `hash_checkpoint_payload_iter` proven byte-identical to the owned variant.
|
||
- _signals_ — The V1->V2 checkpoint format evolution is handled without corruption: version-byte dispatch, strict length validation per version, and V1 records reconstructing an empty day tier rather than misreading bytes — with explicit backward-compat tests.
|
||
- _signals_ — record_signal_scoped enforces the local-profile-intact invariant precisely: every scope is durably WAL-logged, but only SignalScope::Local touches the in-memory aggregate, so community/session/agent writes can never perturb a user's own ranking state.
|
||
- _entities_ — Out-of-order signal handling is genuinely correct: InteractionLedger::record routes through the shared forward_decay_step kernel and only advances last_update_ns on in-order events (interaction.rs:115-135), with a dedicated regression test proving late events are pre-decayed instead of full-weighted — exactly the kernel discipline CODING_GUIDELINES section 3 demands.
|
||
- _entities_ — NaN is defended in depth in the co-engagement path: weights are neutralized to 0.0 at the restore boundary (co_engagement.rs:266-271) so corrupt bytes never enter the map, and every sort uses total_cmp for a deterministic total order, with both a determinism test and a restore-neutralization test.
|
||
- _entities_ — CoEngagementIndex::checkpoint accumulates all edges into a single atomic WriteBatch keyed under a sentinel entity prefix (co_engagement.rs:203-228), so a crash mid-checkpoint can never leave a partial edge set and restore is a targeted prefix scan rather than a full table scan.
|
||
- _entities_ — The creator_followers reverse index correctly uses RoaringTreemap to preserve full u64 user IDs (user_state.rs:36-47) with an explicit regression test for the prior u32 aliasing bug — the right call, since user IDs are sparse/unbounded unlike dense item IDs.
|
||
- _entities_ — remove_hide deliberately clears only the hidden flag and leaves seen-state intact (user_state.rs:182-194), with a regression test ensuring an un-hidden, already-seen item does not resurface as unseen — a subtle correctness boundary handled and documented well.
|
||
- _governance_ — community_ledger.rs:548 — the checkpoint key codec deliberately includes (entity, signal_type) in the storage suffix, with a regression test (checkpoint_restore_preserves_multi_entity_multi_signal_contributor) proving the prior collision-BLOCKER stays fixed.
|
||
- _governance_ — membership.rs:152-165 & capability.rs:142-169 — the <1s stop-forward / revocation guarantee is enforced by a single Acquire atomic load on the synchronous write/check path and an idempotent CAS (first-non-zero-wins, now_ns.max(1)), never routing through the 60s sweeper.
|
||
- _governance_ — serde_util.rs — factoring the AtomicU64 serde adapter into one shared module so Membership.stop_forward_at_ns and CapabilityToken.revoked_at_ns encode identically and cannot drift; reading at Ordering::Acquire to avoid a torn snapshot is the right call.
|
||
- _governance_ — membership.rs:282 / capability.rs:183 / policy.rs:127 / tombstone.rs:86 — serialize_row/to_bytes propagate serde failure as TidalError::Internal instead of swallowing into empty bytes, with a precise doc-comment on why an empty value would silently vanish a gate/epoch/purge on restart.
|
||
- _governance_ — community_ledger.rs:218 & 479 — the aggregate is folded in a fixed sorted (user, writer, epoch) order and digested order-independently, making purge + rematerialization bit-reproducible across replay exactly as the M9p3 spec requires.
|
||
- _query-executor_ — Stage 3 scoring errors are surfaced as `QueryError::InvalidFilter` rather than swallowed into an empty result set (mod.rs:412-418) — a misconfigured profile fails loud instead of silently returning nothing.
|
||
- _query-executor_ — `reject_negated_deferred_filters` correctly tracks Not polarity (even/odd) and refuses to silently return the inverse set for filters that have no sound bitmap-layer complement — exactly the kind of correctness gate a DB needs (user_filter.rs:63-85).
|
||
- _query-executor_ — Cursor-offset overflow is handled defensively with `saturating_add(...).min(len)` plus an explicit `offset < len` guard, so an attacker-supplied cursor near usize::MAX can't wrap into an out-of-bounds slice (pipeline.rs:526-538).
|
||
- _query-executor_ — The notification-cap recording path routes through `NotificationTracker::check_and_record`, closing the check-then-record TOCTOU under concurrency with a single lock, and recovers a poisoned lock instead of panicking into the query path (helpers.rs:244-259, notification_tracker.rs:88-122).
|
||
- _query-executor_ — `signal_ranked_candidates` uses `select_nth_unstable_by` for an O(N) top-K partition before the O(K log K) sort, with a deterministic entity-ID tiebreak on NaN scores — the right algorithm and a stable, reproducible ordering (candidate_gen.rs:71-85).
|
||
- _query-retrieve_ — The u64->u32 truncation hazard is handled with real rigor on the SEARCH path: entity_as_u32 returns Option and retain_excluded/retain_included document opposite polarities (keep on overflow for exclusion, drop for inclusion) so a high ID can never alias onto a suppressed low ID — exactly the kind of subtle bitmap aliasing bug that bites at 3am.
|
||
- _query-retrieve_ — Negated deferred post-filters and OR-of-signal-thresholds are rejected up front with clear errors (user_filter.rs) instead of silently returning the inverse set or AND-instead-of-OR — failing loud on semantics that have no sound interpretation is the right call for a ranking DB.
|
||
- _query-retrieve_ — Cursor-offset overflow is correctly guarded in both pipelines via saturating_add(...).min(len) with an explicit empty-page branch, and there is a regression test (huge_cursor_offset_does_not_overflow) proving an attacker-supplied near-usize::MAX offset cannot panic or wrap.
|
||
- _query-retrieve_ — signal_ranked_candidates uses select_nth_unstable_by for O(N) top-K partition with a deterministic NaN-safe comparator (falls back to entity-ID order) — correct, fast, and reproducible.
|
||
- _query-retrieve_ — The SEARCH relevance anchor is well-defended: fused scores are min-max normalized to a common [0,1] range before seeding the scorer, with a CRITICAL regression test (top_result_is_most_relevant_not_most_liked) ensuring profile boosts only break near-ties rather than letting signal popularity flip the exact vector match out of first place.
|
||
- _query-search_ — The 64-bit-to-32-bit id conversion is handled with correct polarity throughout the suppression/inclusion path: `entity_as_u32` returns None on overflow, and `retain_excluded` keeps overflow ids while `retain_included` drops them (pipeline.rs 67-69, 511-523) — the only sound choice for a 32-bit bitmap.
|
||
- _query-search_ — Cursor-offset overflow is defended explicitly: `offset.saturating_add(limit).min(len)` plus the `offset < len` guard prevents a panic from an attacker-supplied near-usize::MAX cursor, and there is a dedicated regression test (pipeline.rs 612-622, 897-917).
|
||
- _query-search_ — Profile scoring errors are surfaced as QueryError rather than collapsing to a silently-empty result set, with a comment explaining why (pipeline.rs 588-594) — the right call for a ranking system where an empty page is indistinguishable from 'no matches'.
|
||
- _query-search_ — The ANN slot-name resolution bug (read used a hard-coded "content" literal while writes used the schema slot) is fixed and pinned with three regression tests covering threaded slot, default mismatch warning, and relevance-over-popularity ordering (pipeline.rs 781-893).
|
||
- _query-search_ — Degradation is threaded cleanly end-to-end: ReducedCandidates trims BM25/ANN caps via named constants, NoDiversity pushes a warning instead of erroring, and the level is reported in stats and SearchResults — observable graceful degradation per CODING_GUIDELINES §6.
|
||
- _ranking-core_ — finite_score() + total_cmp() pairing (executor/mod.rs:53-56, 596) correctly neutralizes NaN before the sort while deliberately preserving +/-Inf sentinels, with an excellent rationale comment — this is exactly the 3am-proof handling of the classic partial_cmp ranking-scramble bug.
|
||
- _ranking-core_ — DiversitySelector's accepted-ID HashSet + single final emit pass over the score-sorted candidate list (selector.rs:154-159) cleanly guarantees global score order (INV-RANK-6) across relaxation stages instead of the naive per-group approach, and the fixed `denominator` for format-mix is a subtle correctness fix that's well-commented and regression-tested.
|
||
- _ranking-core_ — The registry rejecting Ratio/RelativeVelocity aggregations at registration (registry.rs:155-161) so a misconfigured profile fails loudly up front rather than silently scoring 0.0 at query time — fail-fast over silent-wrong, with read_agg treating the same arm as an Internal invariant violation as defense in depth.
|
||
- _ranking-core_ — TWO_POW_64 named constant (scoring.rs:16-21) instead of dividing by u64::MAX, with a precise comment about the ULP off-by-one — the kind of numeric care that prevents a whole class of normalization bugs.
|
||
- _ranking-core_ — read_agg_for_sort vs read_agg split (helpers.rs:36-116): generic sort-mode signals a schema may omit degrade to 0.0, while user-declared profile terms fail loud — the distinction is principled, documented, and matches the registry's builtin-exemption logic exactly.
|
||
- _ranking-executor_ — finite_score (mod.rs:53-56) + total_cmp sort (mod.rs:596-614) is exactly right: NaN is neutralized at construction so it can never scramble the descending sort, while ±Inf sentinels are deliberately preserved because total_cmp orders them correctly — the rationale is documented and backed by the nan_interaction_boost_does_not_scramble_ranking end-to-end test.
|
||
- _ranking-executor_ — The exclude/gate hard filters always read at DegradationLevel::Full (helpers.rs:130-188) so correctness-critical filtering never trades window precision for load shedding, while only soft scoring reads coarsen — a clean, well-reasoned split.
|
||
- _ranking-executor_ — read_agg vs read_agg_for_sort (helpers.rs:36-116) draws a precise line: user-declared profile terms fail loud on an unknown signal (caught at registration), while the fixed generic sort vocabulary degrades a schema-omitted signal to 0.0 — and the Ratio/RelativeVelocity arm surfaces an Internal invariant violation rather than silently scoring 0.0.
|
||
- _ranking-executor_ — Pure formulas live in their own I/O-free module (formulas.rs) with guarded denominators (controversial_score returns 0.0 not NaN when pos+neg==0) and the TWO_POW_64 constant correctly avoids the u64::MAX-rounds-to-2^64 ULP trap (scoring.rs:16-21).
|
||
- _ranking-executor_ — The deterministic alphabetical tie-break (full-title fallback past the 8-byte f64 prefix, then ascending entity-id) gives a total, reproducible order and is proven by sort_alphabetical_8char_prefix_collision_is_deterministic running with shuffled input.
|
||
- _replication-core_ — state.rs advance_atomic_max centralizes the lock-free max-into-atomic with carefully chosen AcqRel/Acquire ordering and is correctly shared by both ReplicationState and the lag gauge, preventing the two high-water-marks from drifting in concurrency semantics.
|
||
- _replication-core_ — reconcile.rs apply() guards the EntityId(u64) -> RoaringBitmap(u32) narrowing with u32::try_from and returns a typed Internal error instead of an as-cast that would silently truncate item IDs.
|
||
- _replication-core_ — The two-layer session dedup (seqno HWM as primary defense + bounded-LRU idempotency key as secondary, idempotency.rs:48-51) is sound: the documented eviction-returns-true behavior is correctly compensated by the seqno high-water-mark so evicted keys never cause double-apply.
|
||
- _replication-core_ — TransportError cleanly separates Closed (transient, re-ship same seqno) from Permanent (quarantine the peer), and the shipper acts on the distinction (shipper.rs:215-248) instead of retrying a doomed segment forever — exactly the right 3am-incident behavior.
|
||
- _replication-core_ — LWWRegister merge is funneled through the single lww_other_wins helper with a total, node_id-folded HlcTimestamp ordering, giving deterministic, commutative/associative/idempotent hard-negative reconciliation verified end-to-end in reconcile_tests.rs.
|
||
- _replication-data_ — receiver.rs:155-168 correctly rejects a `first_seq + event_count` that would wrap u64 as Corruption instead of silently producing a tiny bogus high-water-mark — and has a dedicated test (`apply_payload_seq_overflow_is_corruption`) proving the HWM does not advance on the overflowing batch.
|
||
- _replication-data_ — shipper.rs `filter_segment_drop_local` re-stamps `first_seq` so the filtered batch's derived last-seq equals the original's, preserving the idempotency/lag boundary even when local events are dropped — the WAL-position-preservation math is documented in detail and covered by both an explicit case and a 128-case proptest in receiver.rs.
|
||
- _replication-data_ — shipper.rs:227-248 distinguishes `TransportError::Permanent` from transient `Closed` and quarantines permanently-failing peers instead of an unbounded silent retry of seqno 1, trading a stall for a loud actionable operator error — exactly the 3am-incident-friendly failure mode, with three tests pinning the behavior.
|
||
- _replication-data_ — tenant.rs `TenantRateLimiter::try_acquire` decouples refill-claim (gated on a CAS that advances `last_refill_ns`) from token consumption so the elapsed-window refill is granted exactly once under contention; the no-over-grant property is verified with an 8-thread hammer test.
|
||
- _replication-data_ — tenant.rs `find_shard_assignment` makes a pin / dual-write target that is missing from the topology a typed error rather than fabricating a `RegionId::SINGLE` placeholder, preventing silent mis-routing of writes to an unconfigured region — covered by `route_pinned_to_absent_shard_errors` and `write_assignments_dual_write_absent_target_errors`.
|
||
- _replication-crdt_ — The HLC logical-counter overflow handling in `now()` (carry into wall + tracing::warn! + dedicated `now_strictly_advances_when_logical_saturated` test) is exactly the kind of 3am-incident defensiveness the bar demands — a saturating bump would have silently re-emitted a duplicate timestamp and corrupted LWW.
|
||
- _replication-crdt_ — Packing `(wall_ms, logical)` into a single AtomicU64 and advancing both via one compare_exchange is the right lock-free design — it eliminates the read-modify-write race that two separate atomics would have, and the proptest over the pack/unpack identity guards the bit-twiddling.
|
||
- _replication-crdt_ — CrdtSignalState::merge correctly replaced additive per-node merge with LWW-per-node versioned by last_update_ns, making segment replay idempotent (`merge(merge(a,b),b) == merge(a,b)`) — the doc comment honestly explains why addition double-counts on retry, and the property tests cover the shared-node LWW path, not just disjoint nodes.
|
||
- _replication-crdt_ — Routing both LWW call sites through the single `lww_other_wins` helper (with the `>` strict-greater / equality-keeps-current rule documented) is the right DRY move — it makes equal-key idempotency and commutativity provable in one place instead of two that could drift.
|
||
- _replication-crdt_ — decay_score does a key-aligned `node_last_update_ns.get(&node)` lookup per node rather than zipping two HashMaps, with a comment calling out that `.zip()` does NOT guarantee key alignment — this is a real, previously-shipped bug class that the code now structurally prevents.
|
||
- _schema_ — Score (score.rs) rejects NaN/inf at construction and implements Ord via f64::total_cmp, giving a sound total order for ranking — exactly the invariant a comparator-stability bug would otherwise violate.
|
||
- _schema_ — Timestamp::now() degrades a pre-epoch clock anomaly to Timestamp(0) with a warning instead of panicking, and seconds_since/elapsed_since both saturating-subtract, so decay math never sees a negative or wrapped dt on the write hot path.
|
||
- _schema_ — DecayModel::Exponential pre-computes and stores lambda = ln(2)/half_life at schema-build time, keeping the per-write/per-read hot path division-free exactly as CODING_GUIDELINES §3 mandates, with property tests asserting lambda*half_life == ln(2).
|
||
- _schema_ — SchemaError documents the real failure mode behind each text-field/embedding rule (e.g. reserved entity_id key collides inside Tantivy's SchemaBuilder::build()) and the builder catches them at define-time, converting would-be panics into typed errors.
|
||
- _schema_ — embedding_slot_fingerprint() sorts by (name, entity_kind discriminant) via an explicit entity_kind_ord rather than relying on a derived Ord (EntityKind has none), giving a reproducible cross-platform fingerprint ordering.
|
||
- _session_ — The session decay tier (signal_state.rs) correctly delegates all two-branch decay arithmetic to the canonical forward_decay_step kernel and asserts advance_timestamp invariance across CAS retries, retiring the documented over-crediting bug where late events were added at full weight.
|
||
- _session_ — serde/mod.rs is exemplary hostile-input handling: every length/count read is bounds-checked, capped_capacity clamps allocations against remaining bytes so a corrupt u32::MAX count cannot OOM, and non-UTF-8 is rejected rather than lossily replaced — with property-style tests proving each.
|
||
- _session_ — track_signaled_entity correctly avoids a DashMap self-deadlock by checking the len() cap BEFORE taking the per-shard entry lock, and documents precisely why the benign over-count race is acceptable for a structural safety bound.
|
||
- _session_ — AuditLog uses a VecDeque for O(1) front-pop eviction with an explicit note that the prior Vec::remove(0) was O(n) per push past the cap, and a test (audit_log_eviction_preserves_fifo_order) proves FIFO semantics survive overflow.
|
||
- _session_ — The snapshot wire format is explicitly versioned (0x01/0x02/0x03) with forward-compatible deserialization that defaults absent fields, and write_audit_entry is shared between the snapshot and standalone audit-log codecs so the two formats cannot drift apart.
|
||
- _text_ — Syncer resilience is genuinely production-grade: a failed Tantivy commit retains the batch, retries with exponential backoff capped at 30s, flips a shared Acquire/Release health flag after N failures, and never lets a single commit error kill the thread — exactly the 3am failure mode that silently loses all future writes, with a CRITICAL regression test pinning it.
|
||
- _text_ — The Acquire/Release ordering on the `healthy` AtomicBool is correctly reasoned and documented at both the load (is_healthy) and every store site, so a reader observing `false` also observes the syncer's pre-failure writes.
|
||
- _text_ — Two-phase Tantivy commit (prepare_commit -> set_payload -> commit) is used correctly to attach the sequence payload, and index_item does delete-then-add in one batch so updates are atomic from the reader's perspective.
|
||
- _text_ — `steady_state_merge_policy()` was factored into a single shared builder so the on-disk and ephemeral paths cannot drift in merge behavior, with the three tuning constants documented at module scope.
|
||
- _text_ — Config thresholds are defensively clamped to >=1 (commit_every_n and commit_every_secs) with a clear rationale that 0 is always misconfiguration, preventing a per-write fsync spin or an idle-tick commit storm — and both clamps are regression-tested.
|
||
- _cohort_ — cohort/types.rs: define() uses the DashMap Entry API to make check-and-insert atomic, with a dedicated 16-thread barrier test (concurrent_define_same_name_yields_exactly_one_winner) proving exactly one winner — the TOCTOU window is genuinely closed, not hand-waved.
|
||
- _cohort_ — cohort/checkpoint.rs: restore() treats ANY structurally-invalid Tag::Cohort row (malformed suffix OR un-deserializable value) as fatal corruption and aborts, with an explicit rationale that the checkpoint is the only durability source for cohort aggregates — exactly the 'fail loud, never serve a silently-truncated aggregate' posture the bar demands.
|
||
- _cohort_ — cohort/types.rs + checkpoint.rs: the MAX_COHORT_NAME_LEN (u16) bound is validated at BOTH define time and checkpoint time (defense in depth), with the u16 length-prefix truncation risk documented and tested at the exact boundary (at-limit succeeds, +1 rejected).
|
||
- _cohort_ — load/detector.rs: InFlightGuard is a clean RAII decrement, and guard_drop_on_panic_path plus the in_flight_never_negative proptest prove the counter cannot leak even on a panicking query executor — correct degradation accounting under failure.
|
||
- _cohort_ — load/rate_limiter.rs: the lazy-refill TokenBucket with retry_after_ms ceil-to-next-ms (never suggests a 0ms retry) is a tidy, syscall-light design, and check() scopes the DashMap entry guard in a block to release the shard lock before returning the error.
|
||
- _db-lifecycle_ — from_parts orders the control-plane install before any metrics Arc clone and loudly logs the unreachable get_mut==None case (mod.rs:609-643), turning a previously-silent un-wired-replication-lag bug into an observable error.
|
||
- _db-lifecycle_ — shutdown_inner captures the FIRST durable-flush error across ledger/cohort/co-engagement/community/storage/WAL-marker steps and still attempts every remaining flush, so close() cannot return Ok on lost durable state while leaving the disk as consistent as possible (lifecycle.rs:159-242).
|
||
- _db-lifecycle_ — The checkpoint thread is wrapped in catch_unwind with a supervisor that latches checkpoint_thread_died and folds it into health_check, so a panicked durability thread flips /healthz to degraded instead of reporting ok against a dead thread (mod.rs:683-715, diagnostics.rs:56-77).
|
||
- _db-lifecycle_ — bump_last_seq uses a Relaxed-load + Release-CAS high-water-mark loop with a documented happens-before rationale for weakly-ordered architectures, preventing a checkpoint marker that references a stale WAL seq and the resulting spurious replay (wal_bridge.rs:121-149).
|
||
- _db-lifecycle_ — The data-directory advisory flock (builder.rs:326-349) plus the dual-open test prevents two processes from silently corrupting the same fjall directory, with the lock held for the instance lifetime and released on Drop.
|
||
- _db-recovery_ — metadata.rs `deserialize_metadata` uses `checked_add` on every `pos + len` bound with a clear comment about 32-bit wrap, and `String::from_utf8_lossy` so a corrupt/truncated metadata record never panics or slices OOB — exactly the right posture for derived-state reads on the recovery path (lines 50-89).
|
||
- _db-recovery_ — export.rs routes JSON-Lines rendering through a `serde::Serialize` wire struct so serde_json owns all control-char/Unicode/quote/backslash escaping (lines 91-143); the regression tests in export_tests.rs prove embedded `\n`/`\0`/`\t` stay one-record-per-line and round-trip, closing a real stream-corruption hole.
|
||
- _db-recovery_ — schema_fingerprint.rs hashes `Duration::as_nanos()` (u128) rather than the derived f64 lambda to avoid IEEE-754 cross-platform instability, and correctly folds embedding-slot dimensions into the fingerprint so a dimension change is rejected on reopen instead of silently corrupting ANN search (lines 25-78).
|
||
- _db-recovery_ — export_signals surfaces the >256 signal-type case as a typed `invalid_input` error via `u8::try_from` instead of the prior debug_assert+silent-truncation that aliased signal type #256 onto #0 and mislabeled every record (lines 208-226).
|
||
- _db-recovery_ — state_rebuild.rs `ItemIndexes::index` is the single shared indexer used by both the live write path and the restart-rebuild path, with the divergence history documented — this structurally prevents the recency-ordering / creator-following drift across restarts that an independent rebuild parser had already caused (lines 178-258).
|
||
- _db-entities-ops_ — items.rs write_item_with_metadata routes indexing through the SAME state_rebuild::ItemIndexes path the restart rebuild uses, and persists a defaulted created_at into metadata so the live index and the rebuild can never diverge on recency ordering (items.rs:79-133) — exactly the right way to kill write/recover drift.
|
||
- _db-entities-ops_ — Poisoned-lock recovery on the universe bitmap is handled correctly: after a successful durable write it logs, recovers via into_inner().insert(), and clear_poison() rather than silently dropping the item from query candidate generation (items.rs:138-151).
|
||
- _db-entities-ops_ — all_item_metadata explicitly skips EMB: embedding rows that share Tag::Meta, preventing serialized vectors from being deserialized as metadata and reaching the text-rebuild/embedder-mapping callers as garbage (items.rs:226-229).
|
||
- _db-entities-ops_ — community governance writes follow durability-before-mutation rigorously: purge_prior_contributions persists the PurgeTombstone before mutating the aggregate (communities.rs:427-434), and define_community_policy persists before registering, with clear comments on the crash-recovery rationale.
|
||
- _db-entities-ops_ — rebuild_collections scopes its open-time scan to a single reserved COLLECTION_NAMESPACE prefix instead of an O(all-keys) walk, with a defensive parse+tag re-check guarding against future key-layout drift (collections.rs:284-339).
|
||
- _db-query-signals_ — notification_tracker.rs: the check-and-record TOCTOU fix is exemplary — the per-creator and per-day caps are evaluated and incremented under a single `record_lock`, poisoned-lock recovery uses `PoisonError::into_inner`, and two concurrency proptests/threaded tests prove no overshoot for both the per-creator and per-day binding caps.
|
||
- _db-query-signals_ — http.rs: the metrics endpoint is genuinely hardened against untrusted input — bounded request-line read via `take(MAX_REQUEST_LINE_BYTES)`, bounded+counted header drain, read timeout, non-loopback bind warning, and tests that actually drive a real socket with oversize and header-flood payloads.
|
||
- _db-query-signals_ — signals.rs `signal()`: finite-weight validation, an Acquire-load backup-in-progress guard paired to create_backup's Release store, and O(1) channel-depth backpressure before enqueue form a clean, well-reasoned write-admission path.
|
||
- _db-query-signals_ — sessions.rs `close_session`: the frozen-snapshot persist and start-record delete are done in a single `WriteBatch` (atomic), and the bounded `closed_sessions` eviction sorts by `closed_at_ns` and removes a fixed batch — no unbounded in-memory growth.
|
||
- _db-query-signals_ — query_ops.rs: SEARCH degrades gracefully on a missing FOR SESSION (warn + execute without session boost) while RETRIEVE fails closed, a deliberate and well-documented asymmetry that matches the 'graceful degradation, never failure' guideline for the search path.
|
||
- _db-metrics_ — now_unix_nanos() saturates to u64::MAX instead of panicking on a pre-epoch or absurd clock, and is feature-gated to match its only consumers — a clean, panic-free time helper.
|
||
- _db-metrics_ — observe() uses partition_point (O(log n)) to find the cumulative-bucket suffix on the signal-write hot path, with a proptest (observe_matches_naive_cumulative_rule) proving it equals the naive value<=bound rule for every input.
|
||
- _db-metrics_ — is_degraded() correctly does NOT trip staleness on a freshly-opened DB (last_checkpoint_ns==0 guard) while enforcing it once any checkpoint succeeds — the right default for the cold-start window.
|
||
- _db-metrics_ — set_control_plane via Arc::get_mut before the Arc is shared, with a loud tracing::error! on the unreachable already-shared branch instead of silently leaving health/lag readers un-wired (mod.rs:632-643).
|
||
- _db-metrics_ — render_prometheus_parses_as_well_formed_exposition (tests.rs:44-132) actually parses the output and validates HELP/TYPE/sample-line structure and metric-name grammar, instead of the usual substring-only assertions that would pass on malformed exposition.
|
||
- _net_ — error.rs is/permanent classification (is_permanent + From<GrpcTransportError> for TransportError, lines 45-128) is exactly right: it splits TLS/auth/malformed/unimplemented from transient codes so the shipper quarantines instead of silently retrying a doomed seqno forever — and it is matched by real shipper tests (permanent_failure_quarantines_peer_no_infinite_retry).
|
||
- _net_ — Both ends raise tonic's 4 MiB codec ceiling to max_payload_bytes on encode AND decode (client.rs:75-77, server.rs:164-167) with a precise comment on why a full-size WAL segment would otherwise trip the encoder before bytes leave the process and be retried forever — a subtle, production-grade detail.
|
||
- _net_ — client.rs wires connect_timeout + per-request .timeout() + HTTP/2 keepalive on the lazy endpoint (lines 49-56) so a blackholed peer fails fast into the circuit breaker instead of hanging the single-threaded block_on shipper — the right defensive posture for a sync/async bridge.
|
||
- _net_ — Drop uses server_handle.abort() then runtime.shutdown_background() (transport.rs:165-180) with a clear rationale, correctly avoiding the 'cannot drop a runtime in an async context' panic when a GrpcTransport is dropped on a reactor thread during cluster shutdown.
|
||
- _net_ — convert.rs validates externally-supplied region_id/shard_id down to u16 with explicit try_into and rejects missing ids (lines 30-45), with overflow + missing-id tests — untrusted proto input is bounded before it becomes a domain type.
|
||
- _server-core_ — bearer_auth uses subtle::ConstantTimeEq for the token comparison and short-circuits only on length (which leaks nothing), correctly avoiding a timing oracle — and the s[..7]/s[7..] slicing is sound because HeaderValue::to_str only yields visible ASCII, so every byte is a char boundary (router.rs:152-184).
|
||
- _server-core_ — Timeout and concurrency layers are applied to protected routes only, keeping /health, /health/live, /health/startup outside the timeout/concurrency budget so probes never flap under saturation — the doc comment explicitly justifies this (router.rs:83-118).
|
||
- _server-core_ — Cluster startup is honestly gated behind ensure_experimental_enabled with a loud WARN, and ClusterState is built on a dedicated thread because GrpcTransport::new blocks on its own runtime and must not run inside the axum reactor — a correct and well-explained async-safety call (main.rs:140-181).
|
||
- _server-core_ — The decay/sort/agg/window YAML parsing in config.rs validates every enum string and rejects unknown variants with actionable SchemaConfig errors, and validate_profile_signals cross-checks every boost/gate/penalty/exclude/decay signal reference against declared signals before the profile is accepted (config.rs:443-475, 609-637).
|
||
- _server-core_ — dto.rs is a deliberate single source of truth for the request/response shapes and engine-result -> JSON mapping shared by the standalone and cluster routers, with a docstring explaining that prior duplication silently drifted the two modes' APIs — exactly the right de-duplication for a contract surface.
|
||
- _server-cluster_ — dispatch_shards correctly degrades rather than drops: every timed-out, errored, or unspawnable shard is recorded by name in unavailable_shards, and the post-deadline try_recv drain (lines 298-305) catches stragglers so a just-in-time shard is not mis-reported as a timeout.
|
||
- _server-cluster_ — entity_shard delegates to the engine's ShardRouter::hash (FNV-1a) instead of a private hash, with a dedicated test (entity_shard_matches_engine_router) proving write/read routing can never diverge from the engine — exactly the bug the comment says it replaced.
|
||
- _server-cluster_ — The offload split is principled and well-documented: spawn_blocking for read queries vs a runtime-free std::thread for gRPC-shipping writes (because GrpcTransport asserts no ambient runtime), with the reasoning captured inline so a future maintainer won't 'simplify' it back into a panic.
|
||
- _server-cluster_ — reconcile_total_candidates floors the reconciled total at deduped_len (line 435), so the reported candidate count can never be smaller than the items actually returned — a thoughtful defense against stale per-shard totals.
|
||
- _server-cluster_ — Cluster mode is gated behind an explicit operator opt-in (ensure_experimental_enabled) with a loud WARN that spells out precisely that all regions share one process and this is NOT production HA — honest about its limitations rather than overselling.
|
||
|
||
|
||
---
|
||
|
||
## Appendix — coverage
|
||
|
||
26 zones, 318 file-reads recorded. Every zone's files were read in full (not skimmed) before findings were raised.
|
||
|
||
| Zone | Files reviewed |
|
||
|---|---|
|
||
| `wal` | 19 |
|
||
| `storage-core` | 26 |
|
||
| `storage-vector` | 10 |
|
||
| `signals` | 14 |
|
||
| `entities` | 12 |
|
||
| `governance` | 14 |
|
||
| `query-executor` | 9 |
|
||
| `query-retrieve` | 26 |
|
||
| `query-search` | 12 |
|
||
| `ranking-core` | 15 |
|
||
| `ranking-executor` | 8 |
|
||
| `replication-core` | 14 |
|
||
| `replication-data` | 12 |
|
||
| `replication-crdt` | 8 |
|
||
| `schema` | 12 |
|
||
| `session` | 10 |
|
||
| `text` | 6 |
|
||
| `cohort` | 8 |
|
||
| `db-lifecycle` | 13 |
|
||
| `db-recovery` | 14 |
|
||
| `db-entities-ops` | 15 |
|
||
| `db-query-signals` | 9 |
|
||
| `db-metrics` | 3 |
|
||
| `net` | 13 |
|
||
| `server-core` | 8 |
|
||
| `server-cluster` | 8 |
|
||
|
||
_Generated from workflow `wv2vxper8` (128 agents, 5.78M tokens). Reviewer model: Opus 4.8; verifier model: Sonnet 4.6 (independent, default-refute)._
|