tidaldb/tidal/docs/reviews/CODE_REVIEW_m0-m10.md

Code Review: tidalDB — Milestones M0–M10

Scope: entire implementation (git diff 413b712..HEAD) — 1060 files, ~317k insertions; ~110k lines of Rust across tidal/, tidal-net/, tidal-server/, tidalctl/.

Method: seven-dimension code-reviewer protocol (Completeness, Accuracy, Tech Debt, Maintainability, Extensibility, DRY, CLEAN) run as 24 parallel subsystem reviews, then every BLOCKER/CRITICAL adversarially re-verified against the cited source by an independent skeptic agent (60 agents total).

Verdict: REQUEST_CHANGES

Severity	Raw	After verification
Blocker	9	7
Critical	27	10
Warning	62	74
Suggestion	47	51
Praise	27	27

Verification dropped 3 false positives, downgraded 16 findings (real but on derived/recoverable state or behind unwired paths), and confirmed all 7 BLOCKERs + 10 CRITICALs at high confidence.

---

Remediation status: RESOLVED

All 142 findings (7 BLOCKER · 10 CRITICAL · 74 WARNING · 51 SUGGESTION) have been remediated.

Gate (workspace):

• cargo fmt --all — applied
• cargo clippy --workspace --all-targets -- -D warnings — exit 0 (clean)
• cargo test — 0 failures: tidaldb lib 1583 + the full integration suite (incl. the 6 new m0m10_durability crash tests, m0m10_recovery text/vector/purge recovery, m8p2_replication_durability, m8p3_reconcile_production, session_durability, and the M7 crash-property/load/scale + vector_usearch recall suites); tidal-net, tidal-server (incl. new standalone), tidalctl all green.

How the systemic root cause was closed. The "signal WAL record carries no per-user / per-contributor identity" gap was fixed with localized durable persistence + restore-on-open + periodic checkpoint (matching the existing CoEngagementIndex/CommunityLedger patterns) rather than a hot-WAL wire-format change: hard-negatives, the interaction ledger, completion/save state, preference vectors, co-engagement, and community contributions are now crash-recoverable (write-through Tag-keyed rows or zero-loss synchronous checkpoint, plus the periodic checkpoint thread), each proven by a hard-crash test (tidal/tests/m0m10_durability.rs).

Items resolved as documented decisions (per each finding's own option list / the review's "do it incrementally, don't force a risky split" guidance), not a code rewrite:

• db/mod.rs TidalDb god-struct (Maintainability-W): the two constructors' ~18 duplicated initializers were DRY'd into SharedDefaults, and the four-cluster field-extraction is documented inline as the prescribed incremental next step (each cluster is a whole-crate change).
• db/backup.rs flush-before-copy (Accuracy-S): documented + test-proven that every acked signal survives backup→reopen→replay; a guaranteed drain of un-acked enqueued events would need a new reply-after-fsync WalCommand::Flush.
• File-size smells (cluster.rs, scatter_gather.rs, testing/cluster.rs) carry tracking notes; tidalctl/main.rs was actually split into commands/ + json/wal_state modules.
• usearch_index.rs total_slots documented best-effort (vs a write-lock guard); the tidal-net receiver-thread leak is fixed at the root (cancellable recv_segment + shutdown_receivers), with the server-side join wiring noted.

---

Systemic root cause (read this first)

The single highest-leverage finding is not any one bug — it is a missing abstraction that produces at least 5 of the 7 BLOCKERs and several CRITICALs:

The signal WAL record carries no per-user / per-contributor identity. EventRecord (wal/format/batch.rs) and SignalEvent (wal/mod.rs) hold only {entity_id, signal_type, weight, timestamp, scope, writer_agent} — there is no user / for_user / creator_id field. But the per-user side-effects of signal_with_context (db/signals.rs) — hard-negatives, interaction ledger, preference vectors, co-engagement edges, completion/save state, community contributor identity — are keyed on exactly that missing identity. Consequently those indexes are in-memory only, are not replayable from the WAL, and are made durable only by a checkpoint on graceful shutdown. A hard crash (panic/SIGKILL/power loss) silently loses everything written since the last clean shutdown, with no error and no metric — a direct violation of CODING_GUIDELINES §2 ("WAL is the source of truth; derived state can be rebuilt from the WAL") and §8 ("no lost events").

Fixing the individual ledgers one-by-one will paper over the symptom. The proper fix is structural: extend the scoped-signal WAL record to carry the contributor identity (user id + interned writer-agent + epoch) for the side-effect-bearing signal types, and replay those side-effects on open exactly as the global SignalLedger already does. That one change makes hard-negatives (BLOCKER 2), the interaction ledger (BLOCKER 3), community contributors (BLOCKER 1, CRITICAL 12), agent-purge tombstones (BLOCKER 4), preference/co-engagement (CRITICAL 11), and completion/save (CRITICAL 10) all crash-recoverable, and lets you delete the shutdown-only checkpoint dependency. Until then, every one of those subsystems is a 3am silent-data-loss incident.

Two more cross-cutting themes:

• Silent wrong-answers in the query/ranking path (BLOCKER 5: OR-of-deferred-filters degrades to AND; CRITICAL 13: windowed/velocity counts frozen at read time; CRITICAL 10: InProgress/DateSaved dead in prod; CRITICAL 8: cohort cache never invalidated). For a database whose entire job is "what should the user see, in what order," a well-formed query that succeeds while returning the wrong set is the worst failure mode.
• u64 EntityId → u32 truncation at the in-memory index boundary recurs across items, entities, hides, creators, and retrieve (downgraded to SUGGESTION/WARNING because it is currently bounded, but it is one missing enforced rejection away from silent cross-entity collisions; fix it once, centrally).

---

BLOCKERS (7) — must fix before merge

1. [BLOCKER] Community contributions are lost on crash — WAL cannot rebuild the in-memory aggregate

Subsystem: db-entities-ops · Location: tidal/src/db/communities.rs:284 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: community_contribute writes a durable WAL event via record_signal_scoped (which succeeds, line 275) and then records per-contributor state into the in-memory community_ledger (lines 288-299). That in-memory ledger is the ONLY home for per-contributor identity: the WAL community events deliberately carry no contributor identity (confirmed at db/mod.rs:460-461: "it cannot rebuild from the WAL, whose community events carry no contributor identity"), and CommunityLedger::restore() reads exclusively the Tag::Community checkpoint (community_ledger.rs:440-466). That checkpoint is flushed only on the graceful-shutdown lifecycle path (db/lifecycle.rs:188). Therefore any hard crash (panic, SIGKILL, power loss) loses every community contribution recorded since the last checkpoint, along with its provenance — and there is no recovery path that can reconstruct it.

Why it matters: This violates the load-bearing invariant in CODING_GUIDELINES §2: "WAL is the source of truth. The entity store, signal aggregates... are derived state. If they are lost, they can be rebuilt from the WAL." Community aggregates are derived state that CANNOT be rebuilt, so a crash silently corrupts community ranking results (read_community_decay_score / read_community_windowed_count return understated values) and silently drops contributions a user believes were durably accepted (signal_for_community returned Ok(true)). At 3am this looks like 'community trending is wrong after the restart' with no error logged anywhere.

Fix: Make the community aggregate rebuildable from durable state. Either (a) extend the community WAL event to carry contributor identity (user, writer_agent, epoch) so CommunityLedger can replay it on open exactly like the local ledger, then drop the lifecycle-only checkpoint dependency; or (b) checkpoint the affected community cell synchronously (or via a bounded write-behind that fsyncs before community_contribute returns Ok(true)) so the durable Tag::Community row is never behind an accepted contribution. Option (a) is the correct structural fix and matches how the local ledger already replays from the WAL.

Verification

Every claim in the finding is verified against the actual code.

1. WAL carries no contributor identity. communities.rs:275-283 calls record_signal_scoped, which in signals/ledger/core.rs:169-178 calls wal.append_signal_scoped(...) passing crate::governance::WRITER_DIRECT (constant = 0, per provenance.rs:17) — not the real contributor. The wire record EventRecord (wal/format/batch.rs:106-119) has fields entity_id, signal_type, weight, timestamp, scope, writer_agent(u16), share_policy_version, membership_epoch — and NO user field at all. But the community ledger's per-contributor key is the tuple (user, writer_agent, epoch) (communities.rs:289-298; community_ledger.rs:461-462). The user dimension is simply not in the WAL, and for direct writes writer_agent=0, so the WAL cannot reconstruct per-contributor state. db/mod.rs:460-461 states this explicitly in a comment.

2. restore() reads only the checkpoint. CommunityLedger::restore (community_ledger.rs:440-466) scans exclusively Tag::Community prefix rows. db/mod.rs:462-464 is the only call site on open.

3. The community checkpoint is written on graceful shutdown ONLY. self.community_ledger.checkpoint(...) appears in production exactly once: lifecycle.rs:188 (shutdown_inner). It is absent from the backup path (backup.rs:199-225 checkpoints ledger, cohort_ledger, co_engagement but NOT community_ledger) and absent from the periodic checkpoint thread (state_rebuild.rs run_checkpoint_thread, lines ~418-481, checkpoints the signal ledger and cohort_ledger only). grep community state_rebuild.rs returns nothing — the crash-recovery rebuild path has zero community handling.

4. The periodic thread makes it worse: after its checkpoint it runs compact_wal_online(dir, seq) (state_rebuild.rs:~445) deleting WAL segments covered by seq — segments that hold the community-scoped events — while never persisting the community ledger. So recovery cannot even partially reconstruct from a compacted WAL.

5. Reads are served from the in-memory ledger only: read_community_windowed_count and read_community_decay_score (communities.rs:362,379) delegate to self.community_ledger. After a hard crash they return understated values with no error.

6. The durability contract is explicit. signal_for_community rustdoc (communities.rs:182-184) promises Ok(true) means "durably logged as a community-scoped WAL event" — a durability promise the system cannot honor for the per-contributor aggregate.

This is a real, reachable bug: any panic/SIGKILL/power-loss between the last graceful shutdown and the crash loses every community contribution and its provenance, silently corrupting community ranking with nothing logged — exactly the CODING_GUIDELINES "WAL is source of truth / derived state rebuildable from WAL" invariant being violated. BLOCKER is appropriate given silent data loss plus a broken explicit durability contract on accepted writes.

2. [BLOCKER] Hard negatives from skip/dislike signals are silently lost on every restart

Subsystem: entities · Location: tidal/src/entities/hard_neg.rs:7 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: The module doc and struct doc state the index is "populated from signal events during startup and updated in real-time" (hard_neg.rs:7, 19-20). No such startup rebuild exists. In production HardNegIndex is written only in-memory by signal_with_context (db/signals.rs:327) for the signal types skip/hide/dislike/block, and is constructed empty on open (db/open.rs:135,241) with no restore call and no checkpoint. WAL replay on open only re-applies aggregate signal events to SignalLedger (db/open.rs:181-187); it never re-runs the per-user hard-neg side-effect. A 'skip' or 'dislike' signal therefore creates a hard-negative that vanishes the moment the process restarts.

Why it matters: CODING_GUIDELINES §6 makes hard-negatives a first-class, permanent correctness primitive: 'A hide creates a permanent hard-negative... the ranking function treats these as first-class inputs', and §8 lists 'Blocked/hidden items never appear in query results' as a property-tested invariant. After a restart, content the user explicitly rejected via skip/dislike resurfaces in their feed. This is exactly the 3am-incident class of bug: silent, data-losing, and contradicts a documented guarantee. Hides routed through write_relationship(Hide) survive (via user_state), but skip/dislike hard-negs do not — an inconsistent and undocumented split.

Fix: Make HardNegIndex recoverable. Either (a) durably persist each hard-neg as a Tag-keyed row (like CoEngagementIndex::checkpoint/restore) and call restore() in db/open.rs, or (b) preferred for WAL-source-of-truth: drive hard-neg reconstruction from the signal WAL by replaying signal_with_context's hard-neg side-effect during recovery (resolve signal_type, for_user, and re-run HardNegIndex::add). Add a crash-recovery test asserting a skip-recorded item is still excluded after reopen. Until fixed, the doc comment claiming startup rebuild must be corrected so it does not misrepresent the guarantee.

Verification

Verified the full chain in source. (1) Write path is in-memory only: db/signals.rs:326-328 calls self.hard_negatives.add(user_id, item_u32) for skip/hide/dislike/block; HardNegIndex::add (hard_neg.rs:49-51) only mutates an in-memory DashMap<u64, RoaringBitmap> — no durable write. (2) Constructed empty with no restore: open.rs:135 and open.rs:241 both build hard_negatives: HardNegIndex::new(); HardNegIndex has no checkpoint/restore methods at all (grep found none, unlike CoEngagementIndex which has checkpoint/restore at co_engagement.rs:203,239), and open.rs never calls a hard-neg restore. (3) WAL replay does not reconstruct hard-negs: open.rs:181-187 replays events only into ledger.apply_wal_event (aggregate SignalLedger state); the per-user for_user side-effect from signal_with_context is never re-run, and the WAL event carries no for_user field. (4) rebuild_entity_state (state_rebuild.rs:73-147) reconstructs user_state only from durable RelationshipType edges (Blocks/Hide/Follows/InteractionWeight) and never touches hard_negatives. Critically, RelationshipType (relationship.rs:11-15) has only Follows/Blocks/InteractionWeight/Hide/Mute — there is NO Skip or Dislike relationship, so skip/dislike signals produce no durable edge and cannot be rebuilt even in principle. (5) The query exclusion path reads the volatile in-memory bitmap (query_ops.rs:80 -> executor/pipeline.rs:330-331 hard_neg.bitmap(user_id)), so after restart the bitmap is empty and skip/dislike-rejected items resurface. (6) The doc comments at hard_neg.rs:7 ("rebuilt from signals on startup") and lines 19-20 ("populated from signal events during startup") describe a startup rebuild that does not exist. This violates the documented hard-negative permanence guarantee (CODING_GUIDELINES §6/§8). Hides routed through write_relationship(Hide) at least persist a durable edge (relationships.rs:50-52) and rebuild into user_state.seen, but skip/dislike hard-negs are lost — exactly the inconsistent, undocumented split the finding describes. BLOCKER severity is appropriate: silent data loss of an explicit user-correctness primitive that contradicts a property-tested invariant. One minor caveat: the struct-level "Replication" doc (lines 25-35) is itself accurate (it describes the cluster reconciliation/LWWRegister path), so only the module-level and field-level "rebuilt on startup" claims are wrong — this does not change the verdict.

3. [BLOCKER] InteractionLedger built from signals is not persisted and not WAL-recoverable — resets to zero on restart

Subsystem: entities · Location: tidal/src/entities/interaction.rs:115 · Dimension: Completeness · Verified: CONFIRMED/high

Issue: signal_with_context records interaction strength into the in-memory InteractionLedger (db/signals.rs:336-337) but never writes a durable RelationshipType::InteractionWeight edge. rebuild_entity_state only reconstructs the ledger from InteractionWeight edges (db/state_rebuild.rs:124-130), and the only writer of those edges is an explicit embedder call to write_relationship(.., InteractionWeight, ..). WAL replay (db/open.rs:181-187) does not re-run the side-effect. So the entire (user, creator) interaction graph accumulated through normal signal traffic is lost on restart.

Why it matters: The for_you and following ranking profiles depend on InteractionLedger::top_creators/score (per the module doc, interaction.rs:7-9). After any restart every user's interaction strength silently snaps to zero, degrading personalized ranking until traffic re-accumulates — with no error, log, or metric. This violates CODING_GUIDELINES §2 ('derived state... can be rebuilt from the WAL') for a hot-path personalization input and is invisible to operators.

Fix: Persist interaction contributions on the signal path so they are recoverable: either write/merge an InteractionWeight edge inside signal_with_context (carrying the decayed running score + last_update_ns so rebuild reproduces state exactly), or replay the interaction side-effect from the signal WAL during recovery. Add a crash-recovery test that records interactions via signals, reopens, and asserts top_creators is preserved within decay tolerance.

Verification

Verified the full chain in production (non-test) code:

WRITE PATH: signal_with_context (db/signals.rs:335-338) records (user, creator) strength into the in-memory self.interaction_ledger via .record(...). It writes NO durable edge. grep of InteractionWeight across the repo returns only: state_rebuild.rs (the READER, lines 124-130), relationship.rs (enum def + serialize helpers), and interaction.rs (the unrelated InteractionWeights constants struct). There is no writer of an InteractionWeight edge anywhere — the finding's claim of an "explicit embedder call" overstates reality; in fact the durable InteractionWeight keyspace is never written by anyone. The only production write_relationship callers (applications/iknowyou/engine/src/lib.rs:325-366) write Hide, Mute, and Blocks — never InteractionWeight.

REBUILD PATH: rebuild_entity_state (db/state_rebuild.rs:124-130) reconstructs the ledger ONLY from InteractionWeight edges (interaction_ledger.record on line 127), which are never persisted. So rebuild yields an empty ledger.

WAL REPLAY: db/open.rs:181-187 replays raw signal events into the SIGNAL ledger (ledger.apply_wal_event) but does not re-run signal_with_context side effects, so the interaction ledger is not reconstructed from the WAL either.

NO OWN PERSISTENCE: InteractionLedger (entities/interaction.rs) has no checkpoint/restore/snapshot/serialize method (confirmed by grep — those exist on relationship/user/creator/collection/co_engagement, not interaction). On restart it is freshly InteractionLedger::new() (open.rs:136, 196).

CONSUMER IMPACT: build_user_context (query/executor/personalization.rs:28-29) calls interaction_ledger.top_creators(user_id, 50, now_ns) to populate creator_interaction_boosts used by score_personalized. After restart this returns empty, so per-creator interaction boosts collapse to zero for every user until live traffic re-accumulates.

MISLEADING DOC: db/signals.rs:294-303 explicitly asserts the user-context side effects (including interaction weights) "are reconstructed from durable storage on restart via rebuild_entity_state" — this is affirmatively false for interaction weights.

UNTESTED: the crash-invariant suite (m7_crash_invariant.rs, m7_crash_m6.rs, m6_crash_surfaces.rs) has zero references to interaction recovery / top_creators, so the gap is silent.

This is a real Completeness/Durability defect: a hot-path personalization input silently and permanently resets to zero on every restart, no error/log/metric, and a doc comment that lies about recoverability — a direct CODING_GUIDELINES §2 violation. Severity nuance: degradation is graceful (boosts go to zero, not corrupt), the ledger re-accumulates from live traffic, and the embedding-based preference_vectors personalization IS durably recoverable, so personalization is degraded not broken. Even so, silent recurring data loss on a ranking input plus a false durability doc is BLOCKER-appropriate for a database whose stated bar is "trust at 3am during a production incident." Severity stands.

4. [BLOCKER] Agent-scope purge resurrects purged contributions after a crash (no durable tombstone)

Subsystem: governance · Location: tidal/src/governance/community_ledger.rs:311 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: purge_writer() (the CommunityAgent removal path) only mutates the in-memory ledger and sets an in-memory watermark. Tracing the caller in db/remove_scope.rs:42-56 confirms it writes NO durable PurgeTombstone and does NOT re-checkpoint the community ledger. The ledger is checkpointed only on graceful shutdown (db/lifecycle.rs:188), and the on-open replay (db/mod.rs:482-487) only re-applies Tag::PurgeTombstone rows — which agent purges never write. So after remove_from_personalization(CommunityAgent{..}) succeeds and returns a receipt, a crash before the next clean shutdown restores the PRE-purge Tag::Community snapshot with the revoked agent's contributions intact. This is exactly the resurrection failure the user-purge tombstone path (purge_contributor + persist_tombstone + apply_purge_tombstone) was built to prevent — the agent path is missing that entire half.

Why it matters: Removing a revoked/abusive agent's influence is a privacy and trust guarantee. Silently resurrecting it on the next crash means the documented 'removed state survives restart' contract (db/remove_scope.rs:27-29) is false for agent scope. A user who was told their data is gone, or an operator who revoked a rogue agent, gets it back at 3am after an OOM kill.

Fix: Make agent purge crash-recoverable the same way user purge is. Either (a) introduce a writer/agent-scoped tombstone variant (e.g. extend PurgeTombstone or add an AgentPurgeTombstone with community_id + writer_agent + watermark_ns) persisted under a Tag before the in-memory purge_writer, and replay it in db/mod.rs alongside the user tombstones; or (b) force a community_ledger.checkpoint() immediately after every purge_writer so the durable snapshot already reflects the removal. Option (a) is preferred — it matches the existing durability-before-mutation pattern and keeps an audit trail of agent removals.

Verification

The finding is accurate on every traced claim. The agent-purge path in db/remove_scope.rs:42-56 (RemoveScope::CommunityAgent arm) calls ONLY community_ledger.purge_writer(community, &writer_agent) (in-memory mutation, community_ledger.rs:311-313) and set_purge_watermark (in-memory only). It writes no durable tombstone and forces no checkpoint.

Contrast with the user-purge path purge_prior_contributions (db/communities.rs:411-440), which does durability-before-mutation: it persist_tombstone(&tombstone)? under Tag::PurgeTombstone (communities.rs:428,450-460) BEFORE calling purge_contributor. On reopen, db/mod.rs:482-487 scans every Tag::PurgeTombstone row and replays it via community_ledger.apply_purge_tombstone, closing the crash window. The codebase even ships a regression test for exactly this window (tombstone_replay_recovers_purge_lost_to_a_crash, community_ledger.rs:1017-1060).

The replay machinery cannot cover the agent case: PurgeTombstone (governance/tombstone.rs:21-34) carries user_id, not writer_agent, and apply_purge_tombstone (community_ledger.rs:481-484) re-purges by user. storage/keys.rs defines only PurgeTombstone=0x0F — there is no agent-scoped tombstone tag.

The Tag::Community checkpoint is the SOLE durable copy of per-contributor state (it 'cannot be rebuilt from the WAL', per community_ledger.rs:390 and db/mod.rs:461), and community_ledger.checkpoint() has exactly ONE call site — db/lifecycle.rs:188, graceful shutdown only. So after remove_from_personalization(CommunityAgent{..}) returns a receipt, any crash/OOM before clean shutdown means db/mod.rs:464 restore() reloads the PRE-purge snapshot with the revoked agent's contributions intact. The watermark is lost too: purge_watermarks is a plain in-memory DashMap (community_ledger.rs:76) never serialized in checkpoint() (the checkpoint loop at 425-433 persists only cell contributors). This breaks the documented 'removed state survives restart' contract at remove_scope.rs:27-29 for agent scope. Reachable, privacy/trust-impacting, no compensating mechanism found. BLOCKER stands.

5. [BLOCKER] OR over non-threshold deferred filters silently degrades to AND, returning the wrong result set

Subsystem: query-executor · Location: tidal/src/query/executor/pipeline.rs:286-326 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: Deferred filters (Saved/Liked/InProgress/InCollection/NearLocation/SocialGraph) resolve to the full universe at the FilterEvaluator level (evaluator.rs:86-98), so an Or([Saved(u), CategoryEq("jazz")]) evaluates in Stage 2 to universe∪jazz = universe (everything retained). Stage 2.5 then calls extract_user_state_filters, which recurses INTO the Or (user_filter.rs:107-111), pulls out Saved(u), and applies it as candidates.retain(...) — an intersection. The query 'saved OR jazz' silently becomes 'saved'. The same happens for InCollection (post_filter.rs:105-116 does retain/clear unconditionally) and NearLocation/SocialGraph. The codebase already recognized this exact trap for thresholds and added reject_or_of_signal_thresholds (user_filter.rs:172), but left every other deferred variant unguarded. This is reachable from the public Retrieve API in the fully-wired DB, and returns a silently-wrong set with no error and nothing logged — the worst failure mode for a ranking DB.

Why it matters: A well-formed query returns the inverse/wrong items with no signal of failure. At 3am you cannot tell the ranking is wrong because the query 'succeeded'. It violates the module's own stated contract ('failing loudly beats returning the inverse set') and the §6 graceful-degradation rule.

Fix: Generalize the guard: add reject_or_of_deferred_filters(expr) that errors on any Or subtree containing ANY is_deferred_post_filter variant (reuse the existing is_deferred_post_filter predicate and the subtree-walk structure of reject_or_of_signal_thresholds), and call it alongside the existing two rejections at pipeline.rs:54-57. Thresholds already have a dedicated message; broaden it to cover all deferred variants, or keep both and make the threshold one a special case. Add tests mirroring reject_or_of_thresholds_errors for Saved/InCollection/NearLocation under Or.

Verification

Verified every link in the chain by reading the actual code:

1. FilterEvaluator resolves deferred variants to the full universe: evaluator.rs:86-98 maps Saved/Liked/InProgress/SocialGraph/MinSignal/MaxSignal/NearLocation/InCollection to self.universe.clone(). evaluate_or (evaluator.rs:124-133) unions children, so Or([Saved(u), CategoryEq("jazz")]) = universe ∪ jazz = universe — every candidate retained in Stage 2 (pipeline.rs:224-233).

2. Stage 2.5 then over-constrains: pipeline.rs:286-326 calls extract_user_state_filters, whose collector recurses INTO Or (user_filter.rs:107-111) and pushes Saved/Liked/InProgress/SocialGraph. The pipeline then does candidates.retain(|id| saved.contains(...)) unconditionally (pipeline.rs:290-293). Net effect: saved OR jazz silently becomes saved (an intersection, the inverse of the requested union).

3. Same defect for the other deferred variants: InCollection — extract_collection_filters recurses into Or (user_filter.rs:257-261), apply_in_collection does retain/clear unconditionally (post_filter.rs:105-116); NearLocation — extract_geo_filters recurses into Or (user_filter.rs:227-231), apply_near_location retains unconditionally (post_filter.rs:194-220); SocialGraph — unconditional retain at pipeline.rs:306-321.

4. The guard gap is real and the maintainers already knew the pattern is wrong: only reject_negated_deferred_filters and reject_or_of_signal_thresholds are called (pipeline.rs:54-57). The latter (user_filter.rs:172-209) rejects Or-of-MinSignal/MaxSignal precisely because "thresholds are applied conjunctively (AND), so an OR would be silently evaluated as AND" — the identical trap, yet Saved/Liked/InProgress/InCollection/NearLocation/SocialGraph under Or are left unguarded.

5. Reachability from the public API confirmed: FilterExpr::Or is a public enum variant (expr.rs:47, no #[non_exhaustive]); RetrieveBuilder::filter (types.rs:263) pushes any FilterExpr verbatim into self.filters with no rewriting; combined_filter (types.rs:128-134) and validate (types.rs:149) never inspect filter-tree shape. A caller can construct Retrieve::builder().filter(FilterExpr::Or(vec![FilterExpr::Saved(u), FilterExpr::CategoryEq("jazz".into())])) and the query silently returns the wrong set.

6. Silent failure: no error returned, nothing logged at warn/error. This directly violates the module's own stated contract at user_filter.rs:25 ("Failing loudly beats returning the inverse set"). For a ranking DB, a well-formed query that returns a silently-wrong result set with a success status is the worst failure mode — undetectable at 3am.

Not test/bench code, not handled elsewhere, not intended behavior. BLOCKER is appropriate: the maintainers themselves treated the structurally-identical threshold case as worth a hard query rejection, and the proposed fix (generalize the guard via the existing is_deferred_post_filter predicate) is exactly consistent with the existing design.

6. [BLOCKER] Follower replicated state is never made durable — a follower crash silently loses all replicated data

Subsystem: replication-core · Location: tidal/src/replication/receiver.rs:194 · Dimension: Completeness · Verified: CONFIRMED/high

Issue: apply_payload applies each replicated event via ledger.apply_wal_event, which by its own contract (signals/ledger/core.rs:300-309) 'bypasses the WAL write' — it mutates only in-memory DashMap state. The received segment bytes are never written to the follower's own WAL directory, and the ReplicationState high-water-mark advanced at receiver.rs:198 is never persisted: to_checkpoint_bytes/from_checkpoint_bytes (state.rs:116,129) are dead outside tests, and db/mod.rs:622-625 constructs ReplicationState::new with every shard pinned at 0 on every startup. Therefore a follower that crashes loses every replicated event since process start, and on restart re-applies from seqno 0. Spec 01-storage-engine.md:754 ('followers replay segments locally') and 14-scale-architecture.md:834 ('promote the follower with the highest replayed seqno') both assume follower-side durability that does not exist.

Why it matters: This defeats the purpose of replication: a follower is supposed to be a durable replica that survives leader loss. As implemented, a follower restart is indistinguishable from a fresh node — it has no persisted record of what it applied, so failover promotion 'by highest replayed seqno' is meaningless across a restart, and lost-write/divergence is guaranteed on any follower crash. This is a 3am data-loss incident waiting to happen, and it is not documented as a known gap.

Fix: On the receive path, before applying events in-memory, durably persist each accepted segment to the follower's own WAL (write the segment bytes to the local WAL segment directory and fsync) OR route the events through the normal WAL-first write path so they are recoverable. Persist ReplicationState on each checkpoint (to_checkpoint_bytes is already written — call it from the checkpoint thread and store it with the fjall checkpoint) and load it via from_checkpoint_bytes in db/mod.rs at startup so applied_seqno survives restart. Add a crash-recovery integration test: write to leader, ship+apply to follower, kill+restart follower, assert the replicated entities and applied_seqno are intact and that re-shipped segments are idempotent no-ops.

Verification

All claims verified against the actual code, and the production reachability holds (not test-only).

1. WAL-bypass apply: receiver.rs:194 calls ledger.apply_wal_event(...). Its rustdoc at core.rs:300-303 says it applies "directly to in-memory state, bypassing the WAL write" and "Does not call wal.append_signal()." Confirmed.

2. No follower-side durability on the receive path: apply_payload (receiver.rs:133-211) only decode_batch -> ledger.apply_wal_event (in-memory DashMap) -> state.advance. There is no write of the received segment bytes to a local WAL, no fsync, no WalWriter/append on the receive path. The only WalWriter references in receiver.rs are NoopWalWriter inside #[cfg(test)] (lines 222-628).

3. Applied high-water-mark is never persisted: to_checkpoint_bytes/from_checkpoint_bytes (state.rs:116,129) have zero production callers — grep across tidal/src and tidal-server/src shows usage only inside state.rs's own tests.

4. Startup always pins shards to 0: db/mod.rs:622-626 always builds ReplicationState::new(&shards); the ctor at state.rs:56-62 sets every AtomicU64::new(0). from_checkpoint_bytes is never called at open. So a restarted follower starts at applied_seqno 0 with an empty in-memory ledger.

5. This is a real production path, not test scaffolding: db/builder.rs:402-417 calls db.start_replication(transport) for NodeRole::Follower during normal open(); replication_ops.rs:23-40 wires the live spawn_receiver. tidal-server uses this for real gRPC followers (cluster.rs references the gRPC segment-receiver thread).

6. Spec contradiction is real: 01-storage-engine.md:754 ("the leader ships sealed segments to followers, who replay them locally") and 14-scale-architecture.md:834 ("Loss of the leader promotes a follower (the one with the highest replayed seqno)") both presume follower-side durability that the implementation lacks.

7. Not documented as a known gap: CHANGELOG only references "M8 gap G1" (gRPC serialization, closed). No documented limitation about follower durability or non-persisted applied seqno.

Consequence: on any follower crash, all replicated signal-ledger state and the applied high-water-mark are lost (in-memory only), and on restart the follower is indistinguishable from a fresh node (applied_seqno=0). Failover "promote highest replayed seqno" is meaningless across a restart. With a live leader the follower could partially re-converge from re-shipped segments, but that does not cover the failover (leader-gone) case and does not guarantee re-shipping from seqno 0 — the lost-write/divergence and meaningless-failover-seqno consequences stand. This is genuine, undocumented, silent follower data-loss for a feature whose entire purpose is durable replication. BLOCKER severity (Completeness) is appropriate.

7. [BLOCKER] Text-index crash recovery is non-functional: durably-written items silently vanish from search after a crash

Subsystem: text · Location: tidal/src/db/items.rs:161 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: The whole point of writer.rs's two-phase commit-with-payload (commit() stores last_seq; last_committed_seq() reads it back, documented as 'used on startup for crash recovery to determine the WAL replay point') is to let the text index detect, on restart, that it lags the durable entity store. But the production write path sends every PendingWrite with seq: 0 (literally commented // seq tracking is best-effort for now), so the Tantivy commit payload is always 0. Worse, last_committed_seq() is never called anywhere outside unit tests — there is NO startup reconciliation comparing the Tantivy payload to the WAL/entity-store seq, and unlike the vector index (db/mod.rs:586 rebuild_from_store) the text index is NOT unconditionally rebuilt at open (spawn_text_syncer just opens the on-disk Tantivy index as-is). The syncer commits asynchronously and best-effort into an unbounded channel. So the failure mode: an item is durably persisted to the entity store (persist_item_metadata) and enqueued for the text syncer; the process crashes before the syncer's next batch commit; on restart the entity store has the item but Tantivy does not; the syncer was healthy when the process died, so the unhealthy-triggered rebuild (poll_text_index_health / flush_text_index) never fires. The item is missing from BM25 text search permanently, with no error and no observability. This is a silent lost-write from the search surface's perspective for any item written within commit_every_secs (default 2s) of a crash.

Why it matters: At 3am during an incident, search silently returns fewer results than the entity store contains, with the health check reporting GREEN. There is no log line, no degraded flag, nothing to point at. The recovery machinery that was built to prevent exactly this (seq payload) is present in the code, giving false confidence that the case is handled.

Fix: Two correct options, pick one and wire it end-to-end: (A) Plumb the real WAL sequence number into PendingWrite.seq at db/items.rs:157-162 (and the creator path in db/creators.rs) so the Tantivy commit payload records true progress; then at open, after spawn_text_syncer, read TextIndex last_committed_seq() and if it lags last_wal_seq, call rebuild_item_text_index() (already implemented) unconditionally before serving search. (B) Simpler and matching the vector index's proven pattern: at open, always rebuild the item/creator text index from the durable store via rebuild_text_index_from_engine() (the entity store is the source of truth), and delete the now-purposeless seq-payload code in writer.rs. Either way, add an integration test that writes items, drops the DB without an explicit flush, reopens, and asserts a SEARCH over those titles returns them.

Verification

Every load-bearing claim is confirmed by the actual code.

(1) Production write path: tidal/src/db/items.rs:157-162 sends PendingWrite { ..., seq: 0, ... } with the literal comment // seq tracking is best-effort for now. The Tantivy commit payload at tidal/src/text/writer.rs:133 (prepared.set_payload(&last_seq.to_string())) is therefore always 0, so the two-phase seq-payload recovery machinery records no real progress.

(2) last_committed_seq() (writer.rs:161) is never called outside #[cfg(test)] modules: every caller (writer.rs:341/348, syncer.rs:427/477/526/599) is inside a mod tests. There is no startup code comparing the Tantivy payload to the WAL/entity-store seq.

(3) The text index is NOT rebuilt at open. spawn_text_syncer (db/text_syncer.rs:47-50) just calls TextIndex::open on the on-disk index. By contrast the vector index IS unconditionally rebuilt at open (db/mod.rs:578-607, "Without this rebuild, vector SEARCH silently returns nothing after every reopen"), and the bitmap/range indexes are rebuilt via rebuild_item_indexes (state_rebuild.rs:274-328) — that function deliberately excludes the text/Tantivy index. The text index is the only derived index opened as-is.

(4) The only triggers for rebuild_item_text_index/rebuild_text_index_from_engine are flush_text_index (called only from backup.rs:230, the backup path — not at open) and poll_text_index_health (state_rebuild.rs:532), which fires only when !is_healthy(). is_healthy flips false only after COMMIT_FAILURES_BEFORE_UNHEALTHY = 3 (syncer.rs:25) consecutive commit FAILURES in a running syncer (syncer.rs:325-329). On reopen the health flag is freshly AtomicBool::new(true) (index.rs:186). So after a hard crash of a previously-healthy syncer, health is GREEN on restart and no rebuild ever fires.

(5) The lost-write window is real and reachable: items are durably persisted, then enqueued to an unbounded channel; the syncer commits time-based at commit_every_secs: 2 default (index.rs:97) or every 1000 docs (index.rs:96). A hard crash within that window leaves the durable entity store ahead of Tantivy permanently and silently. The graceful-shutdown path (syncer.rs:254-259 Disconnected -> final_flush) does flush, so the window is specifically hard crashes/SIGKILL/power-loss — which is exactly the crash-recovery case the system claims to handle.

(6) No test covers it: tidal/tests/m0m10_recovery.rs explicitly tests the analogous VECTOR reopen case ("vector SEARCH must return a result after reopen") and m6_crash_surfaces.rs covers cohort ledger, collections, co-engagement, and checkpoints — none writes items, drops without flush, reopens, and asserts a text SEARCH returns them.

Severity BLOCKER is appropriate: a silent lost-write on the primary BM25 search surface for the default 2s pre-crash window, health reporting GREEN, no log line, and dead recovery machinery (the seq payload) giving false confidence the case is handled — squarely the Accuracy/3am-incident bar this project sets for itself.

---

CRITICAL (10) — should fix before merge

1. [CRITICAL] Resolver membership cache is never invalidated when a cohort is defined at runtime, silently dropping attribution

Subsystem: cohort · Location: tidal/src/cohort/resolver.rs:74 (and db/cohorts.rs:39) · Dimension: Completeness · Verified: CONFIRMED/high

Issue: CohortResolver caches per-user memberships keyed by user_id. The cache is only invalidated on user-metadata change (db/users.rs:114 -> resolver.invalidate). define_cohort (db/cohorts.rs:19-40) is a runtime operation on a live, writeable DB and adds a new cohort to the registry without touching the resolver cache. Any user whose membership was already resolved and cached (membership computed against the OLD cohort set) will keep returning the stale set: signal -> try_cohort_attribution -> resolver.resolve returns cached memberships that omit the newly-defined cohort. The new cohort therefore silently accumulates no signals for those users until they happen to be evicted from the 100k-entry LRU or their metadata changes. No test exercises define-after-resolve ordering (all m6_cohort tests define cohorts before signaling).

Why it matters: Cohort attribution is best-effort but still a correctness contract: a defined cohort is expected to receive matching signals. A user-visible cohort can be created and then receive zero trending/top signal for an unbounded set of active users, with no error and no log — exactly the kind of silent wrong-answer that is impossible to diagnose at 3am because nothing failed.

Fix: Add a clear()/invalidate_all() method to CohortResolver (cache.lock().clear()) and call it from TidalDb::define_cohort after cohort_registry.define succeeds. A full clear is correct and cheap (the cache is a pure accelerator; every entry recomputes on next resolve). Add an integration test: define cohort A, signal user U (caching U), define cohort B that U matches, signal U again, assert B received U's signal.

Verification

Verified the full path. CohortResolver.resolve (resolver.rs:74-98) returns a cached Vec verbatim on hit (lines 78-80) and on miss caches the computed memberships unconditionally — including an empty Vec — via cache.put (line 96). The only cache invalidation is per-user, triggered solely from write_user (users.rs:114 self.cohort_resolver.invalidate(id.as_u64())). define_cohort (cohorts.rs:19-40) is a public runtime method on a live writeable TidalDb (guarded only by require_writeable) that calls self.cohort_registry.define(def) and never touches cohort_resolver — confirmed by grep: cohort_resolver does not appear in cohorts.rs at all. The sole read path for attribution is try_cohort_attribution (signals.rs:369-402), which calls self.cohort_resolver.resolve(user_id, &metadata) (line 394) and records the signal into each returned cohort's ledger. So the reported sequence is reachable: signal user U (resolve caches U's membership against the current cohort set), then define_cohort(B) where U matches B (registry updated, resolver cache untouched), then signal U again -> resolve returns the stale cached set omitting B -> B's ledger silently receives nothing. The stale state persists until U is LRU-evicted (100k-entry cache, DEFAULT_RESOLVER_CACHE_CAPACITY=24) or U's metadata is rewritten (which invalidates). The test-masking claim also checks out: every m6_cohort test defines cohorts before write_user/signal (m6_cohort.rs:91-169, 255-271, 364-373, etc.), and write_user invalidates the cache, so define-after-resolve ordering is never exercised. This is a real, silent, no-log, no-error wrong-answer on a documented public API (define_cohort) with an unbounded-duration window — the failure is undetectable without reading code. Severity CRITICAL is appropriate: although cohort attribution is documented best-effort (signals.rs:367) and self-heals on the next metadata write or eviction, the data-loss window is unbounded and produces zero observable signal, exactly the 3am-undiagnosable class. The reviewer's proposed fix (add invalidate_all/clear to CohortResolver and call it from define_cohort after registry.define succeeds) is correct and cheap since the cache is a pure accelerator that recomputes on miss.

2. [CRITICAL] Checkpoint restore's fail-loud corruption contract is silently swallowed at its only call site

Subsystem: cohort · Location: tidal/src/db/mod.rs:543 (contract at tidal/src/cohort/checkpoint.rs:94-106) · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: CohortSignalLedger::restore documents a deliberate, strongly-worded corruption policy: any structurally-invalid Tag::Cohort entry (malformed suffix OR un-deserializable value) is treated as fatal and aborts the whole restore with TidalError::Internal, because the cohort aggregate cannot be rebuilt from the WAL and a partial restore would 'silently resurrect a wrong aggregate' — closing with 'don't ship what you wouldn't trust at 3am.' But the sole caller, TidalDb::open, matches Err(e) => tracing::warn!(...; 'starting from empty state') and continues. The fail-loud Internal error is downgraded to a warn-and-drop-all-cohort-state, which is precisely the silent degradation the contract was written to prevent.

Why it matters: The code carries an explicit, reviewed argument that corruption must abort loudly, yet the system actually discards all durable cohort aggregate state on any corrupt byte and serves empty cohort rankings with only a warn-level log. The doc comment is now actively misleading about real behavior, and an operator gets no escalation when durable cohort state is lost. Either the contract is wrong or the caller is wrong — they contradict each other.

Fix: Decide the policy once and make code and docs agree. If start-empty is genuinely acceptable for this derived aggregate, change restore to return Ok(None) on corruption (logging an error! with the corrupt key) and rewrite the checkpoint.rs corruption-policy section to say so. If the fail-loud contract is intended, propagate the Err out of TidalDb::open (or gate the warn-and-continue behind an explicit recovery flag) so a corrupt cohort checkpoint surfaces at open time rather than being silently dropped. Add a test asserting the chosen open-time behavior.

Verification

The finding is accurate at every level I verified.

CONTRACT (tidal/src/cohort/checkpoint.rs:94-111): CohortSignalLedger::restore documents a deliberate fail-loud corruption policy — "we therefore treat any structurally invalid Tag::Cohort entry as fatal corruption and abort the whole restore with TidalError::Internal ... Failing loudly on corruption beats serving a silently-truncated aggregate — 'don't ship what you wouldn't trust at 3am.'" The implementation honors this: a malformed suffix returns Err(TidalError::internal(...)) (lines 144-154) and an un-deserializable value returns Err via deserialize_entry(...).map_err(...) (lines 156-161). Two unit tests lock it in: restore_aborts_on_malformed_suffix (450-481) and restore_aborts_on_corrupt_value (483-513), both asserting matches!(err, TidalError::Internal(_)).

CALL SITE (tidal/src/db/mod.rs:531-550): The sole caller does match cohort_ledger.restore(sb.items_engine()) and on Err(e) => runs tracing::warn!(error = %e, "cohort signal ledger restore failed; starting from empty state") then falls through and continues. The Internal error is downgraded to a warn-and-drop-all-cohort-state — exactly the silent degradation the contract forbids.

STRUCTURALLY UNRECOVERABLE: The enclosing function is from_parts (tidal/src/db/mod.rs:380), which returns Self (infallible) — so the swallowed error cannot bubble. The public TidalDbBuilder::open (tidal/src/db/builder.rs:317) does return crate::Result<TidalDb> and calls from_parts at line 370, so propagation is possible in principle, but from_parts itself has no channel to surface it. So the corruption is guaranteed swallowed on every code path.

INTERNAL INCONSISTENCY: The sibling CommunityLedger::restore (tidal/src/governance/community_ledger.rs:440-466), which db/mod.rs:459-461 explicitly notes has the SAME "cannot rebuild from the WAL" property, deliberately uses a skip-corrupt-rows policy (continue on torn rows) and returns (). So the project applies opposite corruption policies to two structurally-identical derived aggregates, and the cohort one's loud-failure doc comment is now actively misleading about real behavior — an operator gets no escalation when durable cohort aggregate state is silently discarded and empty cohort rankings are served.

This is reachable on any single corrupt cohort checkpoint byte at open time, and it silently loses durable derived state with only a warn log. CRITICAL is the right severity (not BLOCKER): cohort signal state is a derived aggregate, not user-source data, and the database stays operational and serves other state; but it is a genuine data-integrity/accuracy defect with a documented-vs-actual contradiction, so it is more than a WARNING.

3. [CRITICAL] completion / saved / liked state in UserStateIndex has no production writer — InProgress filter and DateSaved sort are dead in prod

Subsystem: entities · Location: tidal/src/entities/user_state.rs:329 · Dimension: Completeness · Verified: CONFIRMED/high

Issue: record_completion, add_save, add_save_timestamped, and add_like are invoked only from test modules; no caller exists under db/ or load/ (verified by crate-wide grep). Yet the reader side is wired into live query/ranking: pipeline.rs:303 and search/executor/pipeline.rs:523 call is_in_progress for FilterExpr::InProgress, and ranking scoring.rs:431 calls get_save_timestamp for DateSaved sort. signal_with_context (db/signals.rs:322-358) handles seen/hard-neg/interaction/preference/co-engagement/user-signal/cohort but never completion or save.

Why it matters: A user issuing a 'completion' or 'save' signal updates seen/interaction/preference but never the completion or saved map, so the InProgress filter always matches nothing and DateSaved sort always sees timestamp=None in production. An entire query/sort capability is silently inert — it passes its own unit tests but does nothing for real users. This is the 'complete the loop' failure: a field that does not flow through every layer.

Fix: Wire the writers into signal_with_context: on a 'completion'-type signal with a ratio, call user_state.record_completion(user_id, entity_id, ratio); on a 'save' signal, call add_save_timestamped(user_id, item, ts). Then add the same recovery path as the other derived indexes (these maps are also in-memory-only and would need rebuild-from-signal or persistence). If InProgress/DateSaved are intentionally deferred, gate or remove the reader wiring and the public mutators so the surface does not advertise an unimplemented feature.

Verification

Verified the writer/reader split is real and the loop is broken in production.

WRITERS HAVE NO PRODUCTION CALLER. Crate-wide grep shows every production call to a UserStateIndex mutator is add_block_creator / add_hide / add_follow / add_creator_follower, all routed through db/relationships.rs write_relationship (lines 46-59) and db/state_rebuild.rs (lines 110-121). record_completion, add_save, add_save_timestamped, and add_like appear ONLY inside #[cfg(test)] modules and integration tests (user_state.rs tests, query/executor/tests_part2.rs, ranking/executor/tests/sort_tests.rs, tests/m6_uat.rs, tests/m6p4_collections.rs). RelationshipType (entities/relationship.rs:10-16) has only Follows/Blocks/InteractionWeight/Hide/Mute — no Save/Like/Completion variant — and write_relationship's match has a bare _ => {} arm, so nothing else can route to those writers.

SIGNAL PATH NEVER WRITES THEM. db/signals.rs signal_with_context (lines 308-362) dispatches on signal type/context for hard-negatives, seen, interaction weight, preference vector + co-engagement, per-user signal index, and cohort attribution — but never calls record_completion, add_save, or add_save_timestamped. Yet completion is a real user-sent signal type (db/metadata.rs:101 classifies it as positive engagement; builtins.rs:307 ranks on it). So a user issuing a completion or save-type signal updates seen/preference/cohort but leaves the completion and save_timestamps maps empty.

READERS ARE LIVE. ranking/executor/scoring.rs:431 calls get_save_timestamp for the DateSaved sort; query/executor/pipeline.rs:303 and query/search/executor/pipeline.rs:523 call is_in_progress for FilterExpr::InProgress. Both surfaces are PUBLIC and ADVERTISED: FilterExpr and Sort are pub enums, and CHANGELOG.md lists InProgress (line 58) and DateSaved (line 62) as shipped features. With the maps never populated, in production the InProgress filter matches nothing and DateSaved sorts every item to NEG_INFINITY.

SMOKING GUN: the UAT itself reaches around the missing wiring. tests/m6_uat.rs:402-403 calls db.user_state().record_completion(1001, 50, 0.5) directly via the public accessor (db/relationships.rs:145) to seed the index before issuing the InProgress filter — it does NOT send a completion signal and expect the filter to reflect it, because no such path exists. The feature passes its own tests only because the tests poke the in-memory index that no production code writes.

This is a genuine 'complete the loop' Completeness failure: a documented, public query/sort capability is silently inert for real users. CRITICAL is appropriate — not a crash or corruption (so not BLOCKER), but a shipped, advertised feature that does nothing in production while passing tests. The finding's two load-bearing claims (completion→InProgress, save→DateSaved) are fully substantiated.

4. [CRITICAL] PreferenceVectors and CoEngagement only survive crashes up to the last checkpoint — no WAL replay backstop

Subsystem: entities · Location: tidal/src/entities/preference.rs:100 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: PreferenceVectors has no checkpoint/restore and is rebuilt only by replaying signals into try_update_preference_vector during state rebuild paths; CoEngagementIndex persists via checkpoint() called on shutdown/periodic/backup (db/lifecycle.rs:181, backup.rs:222) and restores on open. Neither is fed by the signal WAL on recovery (db/open.rs:181-187 only re-applies SignalLedger aggregates). A hard crash between checkpoints loses all preference-vector updates and co-engagement edges accumulated since the last checkpoint, and update_counts (preference.rs:35) are explicitly never persisted.

Why it matters: CODING_GUIDELINES §8 requires crash-recovery tests proving 'No lost events' at every write-path point, including 'After signal aggregation'. Here a crash silently rolls back personalization state to the last checkpoint with no replay to close the gap. For a database whose entire value proposition is durable personalized ranking, post-crash preference drift is a correctness regression, not just a cache miss.

Fix: Either replay the signal WAL through the per-user side-effect functions (try_update_preference_vector, co_engagement.record_positive) during recovery so post-checkpoint events are reapplied, or document explicitly and prominently that these indexes are checkpoint-only with a bounded post-crash loss window and add the corresponding crash-recovery test asserting the bound. The current state — a doc-implied 'rebuilt from signals' with no replay — is the dangerous middle.

Verification

The finding is accurate on every load-bearing claim, verified against the actual code.

1. PreferenceVectors has NO durability at all. tidal/src/entities/preference.rs defines only new/with_learning_rate/set/get/update/clear — no checkpoint or restore method exists (grep confirmed). It is constructed empty in both open paths (open.rs:137 ephemeral, open.rs:243 persistent; from_parts at mod.rs:748) and is never fed on recovery. state_rebuild.rs rebuilds entity state, item indexes, suggestion index, and text index — but never preference vectors. try_update_preference_vector and preference_vectors.update are called ONLY from the live signal write path (signals.rs:344, 495), never during open/recovery. So preference vectors start empty after EVERY restart (clean or crash), which is actually a wider gap than the finding's "rebuilt from signals" framing implies.

2. The WAL physically cannot replay these side effects. The WAL SignalEvent record (wal/mod.rs:56-61) carries only {entity_id, signal_type, weight, timestamp_nanos} — there is no user_id/for_user or creator_id. The per-user attribution that drives try_update_preference_vector(user_id,...) and co_engagement.record_positive(user_id,...) lives only in the transient signal_with_context call (signals.rs:308-346) and is never persisted to the WAL. open.rs:181-187 replays events via ledger.apply_wal_event (ledger aggregates only), so the recovery path structurally cannot reconstruct these indexes. This confirms 'only re-applies SignalLedger aggregates.'

3. CoEngagementIndex is checkpoint-only. checkpoint() is called from shutdown (lifecycle.rs:181), backup (backup.rs:222), and the 30s periodic thread; restore() runs on open (mod.rs:414). Edges accumulated since the last checkpoint have no WAL backstop.

4. The one crash test that touches co-engagement (m6_crash_surfaces.rs:302) only passes because the injected panic is caught by catch_unwind in run_with_crash (crash_injector.rs:198) and the owned db is dropped during unwind, so Drop -> shutdown_inner() -> co_engagement.checkpoint() runs (lifecycle.rs:181). This is a graceful shutdown checkpoint, NOT a hard crash. m7_crash_property.rs:220 explicitly documents this Drop-during-unwind checkpoint reliance, and every property invariant there asserts durability only for the WAL-backed signal ledger (recovered_count >= acked_count), never for preference vectors or co-engagement. No test proves these indexes survive a hard crash between checkpoints.

5. update_counts is explicitly documented as never persisted (preference.rs:28-34), matching the finding.

This violates CODING_GUIDELINES §8 ('WAL replay produces identical state to uninterrupted execution'; crash-recovery 'No lost events' at 'After signal aggregation'). CRITICAL (not BLOCKER) is the correct severity: the primary ranking substrate (signal ledger) IS WAL-durable, and the affected state is derived personalization that self-heals — preference vectors re-converge via subsequent EMA blends and missing co-engagement edges degrade related-recs gracefully rather than producing corrupt/wrong answers. It is bounded post-crash personalization drift against an explicit durability guideline, not loss of acked primary writes. Confirmed at CRITICAL.

Relevant files: /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/entities/preference.rs, /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/entities/co_engagement.rs, /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/open.rs, /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/signals.rs, /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/state_rebuild.rs, /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/wal/mod.rs, /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/tests/m6_crash_surfaces.rs, /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/tests/m7_crash_property.rs.

5. [CRITICAL] Community contributions since the last checkpoint are unrecoverable on crash

Subsystem: governance · Location: tidal/src/governance/community_ledger.rs:403 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: The per-contributor CommunityLedger is durable ONLY via checkpoint(), which db/lifecycle.rs:188 invokes solely on graceful shutdown. The shippable WAL event written on the contribution path (db/communities.rs:275 -> record_signal_scoped, signals/ledger/core.rs:169) stamps writer_agent = WRITER_DIRECT and carries no UserId, so as the module docs note (lines 388-391) the community aggregate 'cannot be rebuilt from the WAL'. Consequence: every community contribution recorded after the most recent clean shutdown is permanently lost on a crash, because there is no WAL replay path and no incremental checkpoint.

Why it matters: CODING_GUIDELINES §2 makes the WAL the source of truth and requires derived state be rebuildable from it; §8 requires crash recovery with 'no lost events'. Here the community aggregate is durable state with no WAL backing and a shutdown-only checkpoint — a hard recovery gap for the one ledger that explicitly can't replay. A node that crashes loses real user-shared community signal.

Fix: Give community contributions a WAL-replayable identity: extend the scoped signal WAL record to carry the contributor (user id + writer-agent interned id + epoch) for community-scoped events, and rebuild the CommunityLedger by replaying those events on open (mirroring how the global ledger rebuilds). If that is deferred, document it as a known recovery limitation in CHANGELOG and at minimum checkpoint the community ledger periodically (not only at shutdown) to bound the loss window. Today the window is unbounded.

Verification

Every link in the finding's causal chain is confirmed by the actual code.

1. WAL event lacks contributor identity. db/communities.rs:275 (inside the public signal_for_community, communities.rs:198) calls record_signal_scoped, which at signals/ledger/core.rs:169-178 writes wal.append_signal_scoped(type_id, entity_id, weight, timestamp, scope.discriminant(), crate::governance::WRITER_DIRECT, share_policy_version, membership_epoch) — it passes WRITER_DIRECT and NO UserId / per-contributor writer_agent. The in-memory community aggregate is then updated separately at communities.rs:288-299 with the real user, writer_agent, epoch.

2. Community ledger is durable ONLY via checkpoint(), shutdown-only. The sole production call to community_ledger.checkpoint() is db/lifecycle.rs:188, which lives inside shutdown_inner (the graceful-shutdown path). The module docs at community_ledger.rs:388-391 and 404-413 state this is "the SOLE durable copy of per-contributor state — it is not rebuildable from the WAL."

3. The periodic checkpoint thread does NOT cover the community ledger. run_checkpoint_thread (state_rebuild.rs:348-357) takes only ledger: Arc<SignalLedger> and cohort_ledger: Arc<CohortSignalLedger>; its 30s loop checkpoints those two (lines 420, 477-481) and never the community ledger. The spawn site (mod.rs:693-702) passes only ledger_clone and cohort_clone. So there is no incremental/periodic durability — the loss window is unbounded between clean shutdowns.

4. No WAL replay path rebuilds the community ledger. On open, db/mod.rs:464 calls community_ledger.restore(sb.items_engine()), which reads ONLY the Tag::Community checkpoint (community_ledger.rs:440-466). The code's own comment at mod.rs:459-461 says the WAL "carry[s] no contributor identity" so it "cannot rebuild from the WAL." No community_ledger.record call exists anywhere in the open/replay path — only on the live contribution path.

5. Not documented as a known gap: grep of tidal/CHANGELOG.md returns 0 mentions of community/M9 and no recovery-limitation note.

Net effect: every community contribution recorded after the most recent clean shutdown is permanently lost on a crash. This violates CODING_GUIDELINES §2 (WAL is source of truth, derived state rebuildable from it) and §8 (crash recovery with no lost events). CRITICAL is the appropriate severity — it is silent, unbounded data loss of real user-shared signal on a reachable public API (signal_for_community), but it is scoped to the community-aggregation subsystem rather than corrupting the whole DB or the local user profile (record_signal_scoped only updates the local aggregate for SignalScope::Local, core.rs:188, so a crash does not corrupt unrelated state). The finding is real, the location and mechanism are precisely correct, and the severity is justified.

6. [CRITICAL] Windowed/velocity counts are frozen at read time — expired buckets never rotate out without a new write

Subsystem: signals · Location: tidal/src/signals/warm.rs:176 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: windowed_count() (and sum_current_hour/day, current_minute_count) read the bucket ring directly and never call maybe_rotate(now). Rotation only fires inside increment()/increment_by()/reconcile_to_count(). An entity that received N events in an hour and then goes quiet keeps reporting those N in its OneHour window forever — the minute buckets that should have aged out are never zeroed because no write triggers rotation. This is consumed live: ranking/executor/helpers.rs:56 and query/search/scope.rs:141,200 read windowed_count(window) to drive the Trending scope and velocity ranking. So a viral item that stops getting views stays 'trending' indefinitely, and a 1h-velocity term is computed against stale counts. read_velocity inherits the same staleness. Tests only ever read immediately after writing, so they never observe the freeze.

Why it matters: Trending/velocity is the product's headline retrieval surface ('what content should they see, in what order?'). A windowed count that does not decay between writes silently corrupts ranking for exactly the bursty, then-quiet items velocity is meant to demote. CODING_GUIDELINES §8 lists 'windowed aggregates equal the sum of events within the window' as a required invariant — it does not hold at read time.

Fix: Add a read-time rotation. windowed_count should take a now_ns and call self.maybe_rotate(now_ns) before summing (maybe_rotate is already CAS-guarded and idempotent for sub-minute calls, so a read-heavy load only rotates when a boundary is actually crossed). Thread now_ns from read_windowed_count/read_velocity (which already have a clock via Timestamp::now()) down through windowed_count, and update the scope.rs/helpers.rs call sites to pass the query timestamp. Add a property test: record events, advance the clock past the window with NO further writes, assert the windowed count drops to 0.

Verification

The finding's mechanism is accurate and reachable on production retrieval paths.

1. windowed_count(window) (warm.rs:176) takes only a Window — no now_ns. It dispatches to sum_last_n_minutes/sum_last_n_hours/sum_last_n_days, which read the bucket ring directly. maybe_rotate(now_ns) is only ever called from increment (warm.rs:135), increment_by (143), and reconcile_to_count (221) — all WRITE paths. There is no background rotation thread (warm.rs docs explicitly say "No background thread is required"; my grep found none in signals/).

2. The read path confirms no read-time rotation: read_windowed_count (core.rs:259) does Ok(entry.warm.windowed_count(window)) with no now/maybe_rotate. read_velocity (core.rs:278) derives count via read_windowed_count then divides by a fixed window duration, so it inherits the freeze.

3. The contrast with the hot tier is decisive and shows the asymmetry is real, not intended parity: read_decay_score (core.rs:217-240) deliberately reads Timestamp::now() and applies lazy decay forward to the current time via entry.hot.current_score(idx, now_ns, lambda). The warm tier has the clock available at the same call sites but never threads it down. So a quiet entity's decay score ages correctly while its windowed/velocity counts stay frozen.

4. Live consumption confirmed on the headline retrieval surface: query/search/scope.rs:141 (resolve_trending) and :200 (resolve_cohort_trending) call entry.value().warm.windowed_count(window) directly to compute per-entity velocity for the Trending/CohortTrending scopes; ranking/executor/helpers.rs:56 and :59 read read_windowed_count/read_velocity for SignalAgg::Value and Velocity. A viral item that stops getting views keeps reporting its peak windowed count and velocity until the next write to that exact entity-signal pair triggers maybe_rotate — i.e. indefinitely if it stays quiet.

5. The invariant claim checks out: CODING_GUIDELINES.md:217 lists "Windowed aggregates equal the sum of events within the window" as a required property-test invariant. It does not hold at read time post-quiet. The module's approximate-by-design disclaimer (warm.rs:28-30) only covers ±60s scheduling jitter, not indefinite freezing, so it does not excuse this.

6. Tests confirm the freeze is unobserved: every test in warm.rs and core_tests.rs reads immediately after a write, and even events_outside_1h_window_not_counted (warm.rs:627) drives rotation via increments at minutes 1..70 rather than a pure read after a time gap. No test records, advances the clock with no further writes, then reads.

The proposed fix is sound: maybe_rotate is CAS-guarded and returns early when now_ns < last_min + NS_PER_MIN (warm.rs:353-356), so threading now_ns into windowed_count and calling maybe_rotate before summing is cheap and idempotent under read-heavy load.

Severity CRITICAL is appropriate, not BLOCKER: this silently corrupts ranking accuracy on the product's core surface (Trending/velocity is "what content should they see, in what order?") and violates a documented invariant, but it does not crash, lose durable data, or corrupt the WAL (all_time_count and the underlying events remain correct; only the windowed read is stale, and it self-heals on the next write to that pair).

7. [CRITICAL] Follower backpressure (accepted=false) trips the circuit breaker, converting transient flow-control into a 30s replication stall

Subsystem: tidal-net · Location: tidal-net/src/client.rs:136 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: When a healthy follower's inbound channel is full it replies accepted=false (server.rs:68). The client treats this identically to a hard failure: peer.circuit_breaker.record_failure(). After circuit_breaker_threshold (default 5) consecutive backpressure replies — easy to hit when a follower is merely slow to drain its channel — the breaker OPENS. For the full circuit_breaker_reset (default 30s) every send to that peer short-circuits to CircuitOpen → TransportError::Closed, so the shipper stops shipping to a follower that is up, reachable, and just briefly behind. Backpressure is normal flow-control; the breaker is meant for connection failure. Conflating them turns expected congestion into a self-inflicted 30s outage and amplifies follower lag.

Why it matters: Replication lag is the exact failure mode an operator is paged for. A follower under a write burst is precisely when you must keep shipping (so it can catch up), not stop for 30s. This makes lag spikes worse under load and is non-obvious from logs (it looks like the peer went down).

Fix: Do not feed SegmentRejected/backpressure into the circuit breaker as a failure. In send_to, on accepted == false return GrpcTransportError::SegmentRejected WITHOUT calling record_failure (leave the breaker state untouched), or add a distinct CircuitBreaker::record_backpressure() that is a no-op for breaker state. Only genuine transport/Grpc errors (the Err(status) arm) should count toward opening the breaker. Add a regression test: N consecutive accepted=false replies must NOT open the breaker.

Verification

The mechanical claim is confirmed by direct reads of the cited code. In tidal-net/src/client.rs:126-144, send_to treats a accepted=false reply identically to a hard transport error: the else branch at line 136 calls peer.circuit_breaker.record_failure() and returns SegmentRejected, exactly as the Err(status) arm at line 141 calls record_failure(). The server produces accepted=false as normal backpressure when its inbound mpsc stays full (server.rs:61-69; the comment there says "The WAL is durable on the leader, so the follower will catch up when capacity frees" — i.e. it is explicitly flow control, not failure). circuit_breaker.rs:85-103 opens after circuit_breaker_threshold consecutive failures (default 5 per config.rs:60) and stays Open for circuit_breaker_reset (default 30s, config.rs:61). While open, client.rs:117-119 short-circuits to CircuitOpen(shard), which error.rs:127 maps to TransportError::Closed, which the shipper (shipper.rs:215-226) handles by breaking that peer's run for the poll — so for the full 30s the leader stops shipping to a peer that is up and reachable.

Reachability is real but gated: record_success() (circuit_breaker.rs:74-82) resets the consecutive-failure count to 0, and is called on every accepted segment (client.rs:133). The shipper breaks on the first rejected segment per poll (shipper.rs:225), so a peer gets one send_to attempt that ends in backpressure per poll (default poll_interval 2s, shipper.rs:57). Opening the breaker therefore requires ~5 consecutive polls (~10s) where the follower's 1024-deep inbound channel stays continuously saturated with no segment accepted in between. That is exactly what happens to a follower whose recv_segment/apply consumer falls behind during a sustained write burst — a normal, expected condition, not a contrived one.

The defect is genuine: conflating expected flow-control backpressure with connection failure converts transient congestion into a self-inflicted 30s replication stall that amplifies follower lag precisely when the leader must keep shipping for the follower to catch up. The codebase's own taxonomy already classifies SegmentRejected as transient/non-permanent (error.rs:85-90, test circuit_and_backpressure_are_transient), and the design intent (per the server comment) is that backpressure is recoverable — yet feeding it into record_failure lets it OPEN the breaker, contradicting that intent. No test covers "N consecutive accepted=false must not open the breaker" (the only breaker/backpressure test, reconnection.rs, exercises a downed peer, not backpressure). The proposed fix (do not call record_failure on accepted=false; only the Err(status) arm should count toward the breaker) is correct and consistent with the existing classification. This is not test/bench code and is not handled elsewhere.

Severity CRITICAL is appropriate: it is an accuracy/availability regression in the replication path that worsens the exact failure mode (follower lag) an operator is paged for, is non-obvious from logs (looks like the peer went down), and is self-inflicted under load. It is not a BLOCKER only because no data is lost (the WAL is durable on the leader and the breaker self-heals after 30s via the half-open probe), but a 30s stall that amplifies lag during a write burst is a serious, operator-impacting defect.

8. [CRITICAL] Standalone feed/search run blocking CPU-bound engine queries directly on the async reactor

Subsystem: tidal-server · Location: tidal-server/src/router.rs:235 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: feed (router.rs:235) and search (router.rs:256-260) call state.retrieve(...), state.reload_text_index(...), and state.search(...) directly inside the async handler. TidalDb::retrieve/search are synchronous, CPU-bound calls (candidate scan up to 500 candidates + scoring + diversity; tantivy reload). The cluster path deliberately offloads the identical work via offload_cluster_read/spawn_blocking (cluster.rs:751,786) and documents exactly why. The standalone path does not, so each in-flight feed/search occupies a tokio worker thread for the whole query.

Why it matters: With the default multi-thread runtime (worker count = CPU cores) and a 100-slot ConcurrencyLimitLayer, a burst of feed/search requests can pin every worker thread in synchronous scoring, starving the reactor: health probes are outside the timeout layer but still share the runtime, and other endpoints' futures stop making progress. This is the classic 'blocking call on async runtime' footgun the guidelines call out, and the cluster path already proves the team knows the fix — the standalone path was simply not given the same treatment.

Fix: Wrap the blocking engine calls in tokio::task::spawn_blocking exactly as the cluster handlers do. Extract a shared offload_read helper (it has no gRPC dependency, so plain spawn_blocking suffices) into a common module and use it from both router.rs and cluster.rs so the two surfaces cannot drift again. The write handlers (signal/item/embedding) enqueue to the WAL channel and are not fsync-blocking, so they can stay inline, but feed/search/reload_text_index must offload.

Verification

The finding is accurate on every checked point.

STANDALONE PATH (router.rs): The feed handler (router.rs:222-244) calls state.retrieve(...) inline at line 235; the search handler (router.rs:246-267) calls state.reload_text_index(...) at line 256 and state.search(...) at line 258 inline. There is no spawn_blocking/block_in_place anywhere in router.rs or state.rs (grep returned nothing).

THE CALLS ARE SYNCHRONOUS CPU-BOUND WORK: ServerState::retrieve/search/reload_text_index are plain pub fn (state.rs:132,152,163), each delegating to self.db.<op>(). TidalDb::retrieve (query_ops.rs:31) builds a RetrieveExecutor wired with category/format/creator/tag/duration/created_at indexes, co-engagement, diversity, etc., then runs executor.execute(query) synchronously. TidalDb::search (query_ops.rs:155) runs an 8-stage BM25 + ANN + RRF-fusion + filter + profile-scoring + diversity pipeline. Candidate generation does real CPU work — scan plus select_nth_unstable_by/truncate over up to (limit*4).max(200) candidates (candidate_gen.rs:27,51,79-81). None of this yields to the reactor.

CLUSTER PATH PROVES THE INTENT: The cluster feed/search handlers (cluster.rs:729-804) deliberately wrap the identical work in offload_cluster_read (defined cluster.rs:1000-1013 as tokio::task::spawn_blocking), with a doc comment (cluster.rs:747-749, 781-784, 994-999) stating that running retrieve/search on the axum reactor "would stall every other in-flight request behind one slow shard." The standalone surface simply was not given the same treatment — exactly the drift the finding describes.

RUNTIME CONTEXT: #[tokio::main] with no flavor (main.rs:71) = default multi-thread runtime, worker_threads = CPU cores. router.rs:117 installs ConcurrencyLimitLayer::new(100) and router.rs:82-83 deliberately keeps /health outside the timeout/concurrency layers — but those probes still share the same runtime worker threads, so a burst of feed/search that pins every worker in synchronous scoring delays the reactor's progress on probe and other futures, as claimed.

This is a genuine, reachable 'blocking on the async runtime' defect in the default/primary deployment mode, with a fix the team already ships in the cluster path. Severity CRITICAL is appropriate: it is an availability/latency-under-load degradation (not a crash or data loss, and individual queries are bounded by candidate caps and the 30s timeout — which is why it is CRITICAL rather than BLOCKER), but the codebase's own cluster code treats it as serious enough to engineer around, and the project's explicit '3am production incident' bar makes inline reactor-blocking a must-fix. The proposed fix (extract a shared offload_read plain-spawn_blocking helper used by both surfaces) is correct and matches the existing cluster pattern.

9. [CRITICAL] Cluster write/heal handlers spawn an unbounded fresh OS thread per request

Subsystem: tidal-server · Location: tidal-server/src/cluster.rs:1030 · Dimension: Tech Debt · Verified: CONFIRMED/high

Issue: offload_cluster_write (cluster.rs:1024-1047) creates a brand-new std::thread for every /signals and /cluster/heal request. There is no thread pool, no bound, and no backpressure on thread count — the only limit is the route-level ConcurrencyLimitLayer of 100, but that gates concurrency for the whole protected router, not thread creation per se, and heal/other requests share that budget.

Why it matters: A sustained write burst (the exact 3am-incident scenario for a ranking ingest path) spawns one OS thread per signal write, each doing a blocking gRPC ship to followers. Thread spawn is not free (~stack reservation, scheduler pressure), and dispatch_shards already handles Err from thread::Builder::spawn defensively because it knows thread exhaustion is real — but offload_cluster_write turns a spawn failure into a 500 for the whole write rather than degrading. Under load this is unbounded resource growth on the hottest path.

Fix: Replace per-request thread creation with a small dedicated runtime-free worker pool (a fixed set of OS threads draining an mpsc queue of boxed closures), sized from config, so the gRPC-ship work is bounded and reused. The pool must remain runtime-free (the reason spawn_blocking cannot be used is correctly documented at cluster.rs:1015-1023). Surface queue-full as TidalError::Backpressure→429, consistent with the engine's own backpressure semantics, instead of a hard 500.

Verification

Read cluster.rs:1024-1047: offload_cluster_write spawns a fresh std::thread::Builder::new().name("cluster-write").spawn(...) on every call (line 1030), one OS thread per request, with the result bridged back via a oneshot. On spawn failure it returns ClusterAppError(ServerError::Cluster("spawn cluster write worker: {e}")) — a hard 500, not a degrade or 429. It is invoked from write_signal (POST /signals, line 720) and cluster_heal (POST /cluster/heal, line 675), both hot/operational paths.

The finding is not only confirmed but understated: it claims the per-request thread spawn is bounded only by a route-level ConcurrencyLimitLayer of 100. I read build_cluster_router (cluster.rs:519-554) and it applies NO ConcurrencyLimitLayer and NO TimeoutLayer — only DefaultBodyLimit and optional bearer auth. The ConcurrencyLimitLayer(100) lives solely on the standalone build_router (router.rs:111-118), and main.rs:195 wires cluster mode to build_cluster_router while main.rs:276 uses build_router only for standalone. So on the cluster write path there is genuinely zero in-flight concurrency bound, making per-request OS-thread creation truly unbounded.

The codebase's own author already treats thread-spawn failure as reachable: dispatch_shards (scatter_gather.rs:251-268) checks the spawn Result and on Err logs and marks the shard degraded rather than failing the request — exactly the defensive handling the finding references, and a sharper contrast to offload_cluster_write's hard-500.

The proposed fix is grounded in existing infrastructure: TidalError::Backpressure exists (schema/error.rs:137, used in db/signals.rs), and status_from_error (router.rs:338-339) already maps Backpressure/RateLimited to 429 TOO_MANY_REQUESTS. The runtime-free-thread constraint is real and documented at cluster.rs:1015-1023 (GrpcTransport::send_segment asserts Handle::try_current().is_err(), so spawn_blocking cannot be used), so a runtime-free worker pool draining an mpsc queue is the correct remedy. CRITICAL severity on the Tech Debt dimension is appropriate given the unbounded per-request thread spawn on the hottest cluster write path with no concurrency ceiling whatsoever.

10. [CRITICAL] checkpoint() and flush_batch() panic on a pre-Unix-epoch system clock

Subsystem: wal-core · Location: tidal/src/wal/writer.rs:198 · Dimension: Accuracy · Verified: CONFIRMED/high

Issue: flush_batch computes the batch timestamp with SystemTime::now().duration_since(UNIX_EPOCH).expect("system clock is before Unix epoch") (writer.rs:198-201), and WalHandle::checkpoint does the same (mod.rs:392-396). duration_since returns Err whenever the wall clock is before 1970 — reachable in production via an NTP step backwards, a dead/uninitialized RTC at boot (embedded/container-in-a-fresh-namespace), or a misconfigured host. The expect() turns that into a panic. flush_batch runs on the dedicated writer thread, so the panic poisons nothing but kills the writer thread; every subsequent append then blocks-then-fails as the command channel is dropped — a permanent write outage from a transient clock glitch, the exact failure mode the steady-state-resilience design (writer.rs:309-316, 435-452) goes to great lengths to avoid for I/O faults.

Why it matters: A panic on a reachable path in the durability writer converts a recoverable environmental hiccup into a hard write outage (or a crash for the checkpoint path). 'I don't ship what I wouldn't trust at 3am' — a backwards clock step at 3am should not wedge the WAL.

Fix: Replace the expect() with a graceful fallback: on duration_since Err, use a saturating timestamp of 0 (or the last-known batch_ts) and log a warning, rather than panicking. The batch timestamp is informational metadata, not a correctness input (sequence numbers, not timestamps, order the WAL), so a clamped value is safe. Remove the '# Panics' clauses once the panic is gone.

Verification

Verified both cited sites exist verbatim. tidal/src/wal/writer.rs:198-201 (flush_batch) and tidal/src/wal/mod.rs:393-396 (checkpoint) both call SystemTime::now().duration_since(UNIX_EPOCH).expect("system clock is before Unix epoch"). duration_since returns Err whenever the wall clock is before 1970, which is reachable in production (NTP step-back, dead/uninitialized RTC at boot in containers/embedded, misconfigured host), so .expect() panics.

The decisive evidence is that the codebase ALREADY solved this exact problem the right way and these two sites violate it. tidal/src/schema/timestamp.rs:29-41 Timestamp::now() handles the identical SystemTime::now().duration_since(UNIX_EPOCH) Err case by saturating to Timestamp(0) and logging tracing::warn!, with a doc comment (lines 20-27) stating 'it must never panic on a clock anomaly: a pre-epoch system clock corrupts no state and should not take down the database', plus a regression test now_never_panics() at line 103. The two flagged call sites reimplement the raw .expect() instead of calling this canonical helper. Their '# Panics' doc comments (writer.rs:188, 328) even claim 'same as Timestamp::now()' — which is factually false, since Timestamp::now() does not panic.

The panic is a permanent outage, not a transient one. flush_batch runs on the spawned writer thread (mod.rs:257-260 std:🧵:Builder spawn run_writer). There is no catch_unwind and no respawn anywhere in tidal/src/wal/. When run_writer panics the thread dies, rx drops, and every subsequent append/flush/checkpoint returns WalError::SendFailed (mod.rs:110/132/302); error.rs:30 documents SendFailed as 'writer thread panicked or exited'. This directly defeats the writer's own resilience design: the run_writer 'Resilience' doc (writer.rs:309-316) and the Err arm (writer.rs:435-452) go to great lengths to keep the writer alive through transient I/O faults (ENOSPC/EINTR/NFS blip) precisely so a transient fault never becomes a permanent write outage — yet a transient clock glitch panics straight through that design. checkpoint() runs synchronously on the caller's thread (the signal materializer), so its panic propagates there.

The batch timestamp is non-load-bearing: WAL ordering is by sequence number (batch_seq/next_seq), and batch_ts is passed only as informational metadata into format::encode_batch_with_shard (writer.rs:213-219), so the proposed saturate-to-0 fix is safe and matches the existing Timestamp::now() precedent. CRITICAL is the right severity: a durability subsystem turning a transient environmental hiccup into a permanent write outage is exactly the 3am-incident failure the project philosophy (CLAUDE.md: 'I don't ship what I wouldn't trust at 3am') forbids.

---

WARNINGS (74)

cohort

• tidal/src/cohort/types.rs:156-161 — [CLEAN] serialize_cohort_def converts an (impossible) serde error into an empty Vec that becomes silent data loss on restart
• tidal/src/cohort/ledger.rs:81 (and read paths ledger.rs:123, ledger.rs:147) — [Tech Debt] Per-signal String key allocation on the cohort attribution fan-out path

db-core

• tidal/src/db/mod.rs:290 — [Accuracy] from_config builds two divergent ReplicationState Arcs; control-plane lag gauge and stored field disagree (was CRITICAL)
• tidal/src/db/mod.rs:297 — [DRY] from_config and from_parts duplicate ~50 verbatim field initializers
• tidal/src/db/mod.rs:81 — [Maintainability] TidalDb god-struct and 821-line mod.rs exceed the §9 file-size guideline

db-entities-ops

• tidal/src/db/sessions.rs:515 — [Accuracy] Cross-session preference aggregation hardcodes EMB:content instead of the schema-resolved item slot
• tidal/src/db/items.rs:114 — [Accuracy] u64→u32 entity-ID truncation silently collides large IDs across every in-memory index
• tidal/src/db/items.rs:161 — [Tech Debt] Tantivy commit-sequence checkpoint stubbed to seq:0 with no tracking issue

db-infra

• tidal/src/db/export.rs:261 — [Accuracy] export_signals materializes the entire WAL into memory before applying the limit (was CRITICAL)
• tidal/src/db/replication_ops.rs:96 — [Completeness] signal_for_tenant silently drops remote-shard writes but its rustdoc claims dual-write
• tidal/src/db/text_syncer.rs:54 — [Completeness] Text index open failure is swallowed with no log, silently disabling text search
• tidal/src/db/state_rebuild.rs:629 — [Tech Debt] wal_lag_bytes gauge sums ALL segments but is documented as 'not yet compacted'

entities

• tidal/src/entities/user_state.rs:93 — [Accuracy] Hide item ID truncated to u32 without the warning the item-write path emits
• tidal/src/entities/relationship.rs:15 — [Maintainability] RelationshipType::Mute silently no-ops in both write and rebuild paths

governance

• tidal/src/governance/capability.rs:308 — [Accuracy] Capability eviction can drop a live token, silently denying a valid capability until restart (was CRITICAL)
• tidal/src/governance/capability.rs:28 — [Extensibility] ScopeClass duplicates SignalScope's variant set and discriminant ordering by hand

load-testing

• tidal/src/load/rate_limiter.rs:107 — [Accuracy] Rate limiter accepts zero/negative rate, permanently locking out agents and emitting u64::MAX retry (was CRITICAL)
• tidal/src/load/rate_limiter.rs:172 — [CLEAN] Unlimited-mode rate-limiter check allocates a String and touches DashMap on every session write
• tidal/src/testing/cluster.rs:347 — [Accuracy] write_signal commits to the leader before a panicking encode step, leaving leader ahead of an un-shipped batch

query-executor

• tidal/src/query/executor/pipeline.rs:88-98 — [Accuracy] for_creator scope silently lost when creator_items index is absent
• tidal/src/query/executor/mod.rs:499-505 — [Extensibility] Social-graph trending scope resolution only inspects top-level And, diverging from the filter that recurses into Or
• tidal/src/query/executor/tests_part2.rs:25-96 — [DRY] Test scaffolding duplicated verbatim across executor test modules

query-retrieve-fusion

• tidal/src/query/retrieve/types.rs:147-149 — [Tech Debt] Stale #[allow(dead_code)] comment on validate() actively misleads — the executor calls it
• tidal/src/query/retrieve/cursor.rs:42-50 — [Accuracy] Cursor offset truncates on 32-bit targets via usize<->u64 casts
• tidal/src/query/retrieve/types.rs:373-378 — [Accuracy] for_creator() silently truncates creator IDs above u32::MAX

query-search

• tidal/src/query/search/executor/pipeline.rs:393 — [Accuracy] Deferred filter AND-combined with an empty universe drops every candidate
• tidal/src/query/search/executor/pipeline.rs:95 — [DRY] combined_filter() recomputed and cloned five times per query in the hot path
• tidal/src/query/search/executor/pipeline.rs:249 — [Completeness] Silent empty-result degradation on poisoned universe lock with no warning

ranking

• tidal/src/ranking/executor/helpers.rs:199 — [Accuracy] Shortest sort: bottom-ranked missing-duration item is rewritten to the maximum score 1.0 by normalize()
• tidal/src/ranking/executor/scoring.rs:121 — [Maintainability] now: Timestamp is threaded through the entire scoring path but is dead; all time-sensitive reads use Timestamp::now() internally

replication-core

• tidal/src/replication/receiver.rs:189 — [Accuracy] Mid-payload corruption commits earlier batches then halts with no durable recovery point (was CRITICAL)
• tidal/src/replication/receiver.rs:175 — [Accuracy] Lag gauge under-reports leader seqno when community-overlay drops an all-local segment
• tidal/src/replication/../db/replication_ops.rs:96 — [Tech Debt] Dual-write migration silently drops remote-shard signals as a logged no-op
• tidal/src/replication/shipper.rs:115 — [Extensibility] Quarantine and high-water-mark state live only in the shipper thread's stack — no operator visibility or reset without restart

replication-crdt-tenant

• tidal/src/replication/reconcile.rs (engine) + tidal/src/db/replication_ops.rs (no caller) — [Completeness] CRDT reconciliation engine is never invoked in production — partitions never heal (was CRITICAL)
• tidal/src/db/replication_ops.rs:96 — [Tech Debt] "Task 5 not yet wired" stub is a TODO without a tracking issue
• tidal/src/replication/crdt/signal_state.rs:98 — [Completeness] from_node_contribution windowed-count round-trip is only ever exercised with bucket_count=0

schema

• tidal/src/schema/validation/mod.rs:154 — [DRY] EntityKind->fingerprint-byte mapping duplicated in three places, aligned only by comment
• tidal/src/schema/validation/builders.rs:251 — [Completeness] Agent-policy validation branch has zero test coverage

session

• tidal/src/db/session_restore.rs:173 — [Accuracy] WAL replay collapses all session signals into one minute bucket, corrupting restored window counts (was CRITICAL)
• tidal/src/db/sessions.rs:439 — [Accuracy] f64 session signal weight is narrowed to f32 on the WAL path, so replayed scores lose precision
• tidal/src/wal/mod.rs:286 — [Maintainability] Session-journal append doc-comment claims a durability guarantee the caller never waits for

signals

• tidal/src/signals/ledger/core.rs:226 — [Accuracy] read_decay_score silently returns an undecayed score for an out-of-range decay_rate_idx
• tidal/src/signals/ledger/core.rs:105 — [DRY] Get-or-create EntitySignalEntry block duplicated across four ledger methods
• tidal/src/signals/checkpoint/format.rs:75 — [Maintainability] Serialization byte offsets are hand-encoded magic numbers repeated in three independent places

storage-engine-indexes

• tidal/src/storage/fjall.rs:52 — [Accuracy] All fjall errors flattened to StorageError::Corruption, mislabeling transient and lock failures (was CRITICAL)
• tidal/src/storage/fjall.rs:256 — [Accuracy] flush_all performs four full-database fsyncs where one is required (was CRITICAL)
• tidal/src/storage/indexes/filter/evaluator.rs:109 — [CLEAN] evaluate_and double-evaluates compound children (selectivity recursion then bitmap evaluation)
• tidal/src/storage/fjall.rs:77 — [Accuracy] scan_prefix eagerly materializes the entire result set into a Vec
• tidal/src/storage/indexes/filter/result.rs:79 — [Tech Debt] FilterResult::cardinality and is_empty silently lie for the Predicate variant

storage-vector

• tidal/src/storage/vector/lifecycle/ops.rs:146 — [Accuracy] Soft-delete leaves a durable key that rebuild_from_store resurrects on restart (was CRITICAL)
• tidal/src/storage/vector/usearch_index.rs:127 — [Accuracy] total_slots counter is incremented via a non-atomic contains/add/fetch_add sequence
• tidal/src/storage/vector/registry.rs:156 — [CLEAN] Slot lookup allocates a String on every search/insert hot-path call
• tidal/src/storage/vector/registry.rs:203 — [Tech Debt] index_stats multiplies countdimbytes without overflow guards; panics in debug

text

• tidal/src/text/collectors.rs:51 — [DRY] AllScoresCollector.entity_id_field is set by every caller but ignored; for_segment hardcodes the "entity_id" string
• tidal/src/text/collectors.rs:52 — [Accuracy] first_or_default_col(0) can emit EntityId(0), which aliases a real entity, rather than signalling a missing id

tidal-net

• tidal-net/src/circuit_breaker.rs:60 — [Accuracy] Circuit breaker half-open does not enforce the documented single-probe invariant
• tidal/src/replication/transport.rs:72 — [Completeness] Transport trait contract says send_segment is "non-blocking"; gRPC impl blocks the shipper thread up to request_timeout
• tidal-net/src/transport.rs:146 — [Tech Debt] Segment-receiver thread parks forever in recv_segment and is only reclaimed at process exit (leaked thread on clean shutdown)

tidal-server

• tidal-server/src/scatter_gather.rs:575 — [Accuracy] Coordinator-level max_per_creator resolves creators only from the leader replica, silently under-enforcing in entity-sharded mode
• tidal-server/src/cluster.rs:558 — [DRY] health_startup and health_live are byte-identical duplicates across the two routers
• tidal-server/src/cluster.rs:442 — [CLEAN] cluster_ref()/cluster_arc() rely on expect() guarded only by an external ordering invariant
• tidal-server/tests/middleware.rs:1 — [Completeness] Standalone data handlers have no integration coverage; only middleware/auth are tested

tidalctl

• tidalctl/src/main.rs:565 — [Accuracy] Text-index open failure is silently reported as an empty index with no exit-code signal
• tidalctl/src/main.rs:573 — [Accuracy] checkpoint_age_seconds emits a fabricated 0 in JSON when no checkpoint exists
• tidalctl/src/main.rs:203 — [Accuracy] status exits 0 on a corrupt/unreadable WAL while diagnostics exits 2

wal-core

• tidal/src/wal/writer.rs:369 — [Accuracy] truncate_before can unlink the active segment the live writer is appending to (silent post-checkpoint write loss) (was CRITICAL)
• tidal/src/wal/segment.rs:208 — [Accuracy] SegmentWriter::sync() doc claims fsync-level durability but only fdatasyncs the data file, not the directory
• tidal/src/wal/diagnostics.rs:132 — [DRY] Segment byte-scan loop duplicated between reader and diagnostics, already drifted once (>= vs >)
• tidal/src/wal/writer.rs:362 — [DRY] WalCommand dispatch match duplicated across three sites in run_writer

wal-format

• tidal/src/wal/session_journal.rs:78 — [Completeness] Session-journal corruption silently truncates recovery with zero observability (was CRITICAL)
• tidal/src/wal/format/session.rs:410 — [Accuracy] from_utf8_lossy silently mutates string fields on replay instead of flagging corruption
• tidal/src/wal/format/session.rs:363 — [Accuracy] Unknown session record type is skipped, masking a class of mid-stream corruption
• tidal/src/wal/format/session.rs:280 — [Tech Debt] Magic offsets in v2 frame layout and tests lack named constants

WARNING details (issue + fix)

serialize_cohort_def converts an (impossible) serde error into an empty Vec that becomes silent data loss on restart (cohort)

tidal/src/cohort/types.rs:156-161 · CLEAN

• Issue: serialize_cohort_def returns serde_json::to_vec(def).unwrap_or_default(). On the (acknowledged-impossible) serialization failure it returns an empty Vec. That empty Vec is what define_cohort persists to Tag::CohortDef (db/cohorts.rs:30-31). On the next open, the restore loop calls deserialize_cohort_def(b"") which returns None (verified: types.rs:167-169 + test at types.rs:372), so the cohort definition silently vanishes after restart while the in-memory registry for the current process still has it.
• Fix: Change serialize_cohort_def to return crate::Result<Vec> (or Result<Vec, TidalError>) and propagate via ? into define_cohort, mapping a serde failure to TidalError::Internal. Since to_vec genuinely cannot fail for these types, this is a zero-runtime-cost change that removes the silent-empty-write footgun and makes the impossible case loud if it ever becomes possible (e.g. a future Predicate variant).

Per-signal String key allocation on the cohort attribution fan-out path (cohort)

tidal/src/cohort/ledger.rs:81 (and read paths ledger.rs:123, ledger.rs:147) · Tech Debt

• Issue: CohortSignalLedger keys entries by (String, EntityId, SignalTypeId) and record() builds the key with cohort.to_owned() on every call. A single signal_with_context fans out to every matching cohort (db/signals.rs:398-401), so one signal write can allocate N Strings (N = matched cohorts). The read paths also allocate a fresh String per query. The doc (ledger.rs:22-26) acknowledges this as a deliberate lifetime-avoidance tradeoff, but it runs against CODING_GUIDELINES §1 ('Avoid String in hot-path structs — use interned IDs or u64 hashes') on the signal write path.
• Fix: Intern cohort names to a u16/u32 cohort-id at define time (the registry already has a natural id space) and key the ledger DashMap by (CohortId, EntityId, SignalTypeId) — a Copy tuple with zero per-write allocation. The checkpoint suffix can then encode the compact id and resolve the name via the registry, also shrinking the on-disk suffix. If interning is deferred, at minimum store Arc keys and clone the Arc (refcount bump) instead of to_owned().

from_config builds two divergent ReplicationState Arcs; control-plane lag gauge and stored field disagree (db-core)

tidal/src/db/mod.rs:290 · Accuracy

• Issue: In from_config, build_control_plane is fed rep_state created at line 290 (ReplicationState::single()), but the struct's replication_state field is initialized with a SEPARATE ReplicationState::single() at line 354. These are two distinct Arcs. The control-plane lag gauge (built inside build_control_plane) holds the line-290 Arc, while start_replication (replication_ops.rs:28) clones the line-354 field Arc and hands it to the receiver. A receiver would then advance the field Arc while the lag gauge reads the orphaned line-290 Arc — replication lag would be permanently stuck. This is exactly the obs-REPL-1 class of bug that from_parts (mod.rs:622-631) was fixed to avoid by sharing one Arc; from_config was not given the same fix.
• Fix: Construct one ReplicationState Arc in from_config and use it for both build_control_plane and the field, mirroring from_parts. Delete the second ReplicationState::single() at line 354 and initialize the field with Arc::clone(&rep_state) (rename the line-290 binding to match). Add a debug_assert or test that Arc::ptr_eq(db.replication_state(), &lag-gauge-state) in both constructors so the invariant cannot silently regress.

from_config and from_parts duplicate ~50 verbatim field initializers (db-core)

tidal/src/db/mod.rs:297 · DRY

• Issue: from_config (mod.rs:297-365) and from_parts (mod.rs:721-789) each contain a ~50-field struct literal where the majority of fields are identical default initializers: closed, text_tx/None, sessions: DashMap::new(), next_session_id: AtomicU64::new(1), load_detector, backpressure_config, rate_limiter, shutdown_sweeper, sweeper_thread, backup_in_progress, receiver_handle, shipper_handle: None, session_bridge, lock_file: None, membership/community/governance/capability registries, etc. The two literals must be kept in lock-step by hand.
• Fix: Extract the shared defaults into a TidalDb::common_defaults() builder-style helper (or a Defaults struct spread via ..), so each constructor only specifies the fields that genuinely differ (storage/ledger/wal/indexes vs None, and the control-plane/replication wiring). The control-plane and session-bridge helpers were already extracted this way — finish the job for the rest of the literal.

TidalDb god-struct and 821-line mod.rs exceed the §9 file-size guideline (db-core)

tidal/src/db/mod.rs:81 · Maintainability

• Issue: TidalDb carries ~60 fields spanning M0–M10 concerns (storage, WAL, checkpoint thread, two text syncers, sessions, co-engagement, cohorts, collections, suggestions, notifications, load/rate-limit, replication, tenancy, governance, capabilities) on one struct, and mod.rs is 821 lines. CODING_GUIDELINES §9 caps source-file size and the prompt flags large files as a known smell; three #[allow(clippy::too_many_lines)] in this file acknowledge it.
• Fix: Group cohesive field clusters into sub-structs held by TidalDb: e.g. replication: ReplicationParts, governance: GovernanceParts, text: TextParts, lifecycle: LifecycleThreads (checkpoint/sweeper handles + shutdown flags). This shrinks both constructors, makes Drop ordering explicit per cluster, and confines future milestone fields to their cluster rather than the root. Do it incrementally, one cluster per change, to keep diffs reviewable.

Cross-session preference aggregation hardcodes EMB:content instead of the schema-resolved item slot (db-entities-ops)

tidal/src/db/sessions.rs:515 · Accuracy

• Issue: apply_session_preference_update builds the embedding key with encode_key(EntityId::new(entity_id), Tag::Meta, b"EMB:content") — a hardcoded slot suffix. Every other path that reads an item embedding resolves the slot from schema via item_embedding_slot() (e.g. read_item_embedding at items.rs:337 and try_update_preference_vector at signals.rs:460, both via embedding_store_key(entity_id, self.item_embedding_slot())). item_embedding_slot() returns the schema's first Item embedding slot name and only falls back to "content" when no schema declares one (signals.rs:431-441).
• Fix: Replace the hardcoded key with the resolver used everywhere else: let key = crate::storage::vector::embedding_store_key(EntityId::new(entity_id), self.item_embedding_slot()); — identical to try_update_preference_vector. This collapses the two preference-update read paths onto the single slot resolver and removes the divergence.

u64→u32 entity-ID truncation silently collides large IDs across every in-memory index (db-entities-ops)

tidal/src/db/items.rs:114 · Accuracy

• Issue: write_item_with_metadata truncates the entity ID with let id_u32 = id.as_u64() as u32; and only emits a tracing::warn! when id > u32::MAX (lines 115-120) before inserting the truncated ID into the universe bitmap and all bitmap/range indexes. The same as u32 truncation is repeated in signals.rs:323 (hard-negatives, seen, co-engagement), relationships.rs:51/99 (hide), and collections.rs:59/95. None of these reject the oversized ID; they truncate and proceed.
• Fix: Enforce the documented universe limit at the write boundary: in write_item_with_metadata (and write paths that key bitmaps on truncated IDs), return TidalError::invalid_input when id.as_u64() > u64::from(u32::MAX) instead of warning-and-truncating. Document the u32 item-universe limit on the public write_item/signal_with_context rustdoc so embedders allocate dense IDs. If a larger universe is required later, widen the bitmap key type behind the index abstraction.

Tantivy commit-sequence checkpoint stubbed to seq:0 with no tracking issue (db-entities-ops)

tidal/src/db/items.rs:161 · Tech Debt

• Issue: write_item_with_metadata enqueues every PendingWrite with seq: 0 and the comment "seq tracking is best-effort for now" (also creators.rs:39 with seq:0). The syncer commits update.seq as the Tantivy commit payload (text/syncer.rs:168,216) which CODING_GUIDELINES §5 specifies is the last-processed sequence number used to "replay from that sequence number" on crash recovery. With seq always 0, that incremental-replay checkpoint is defeated; recovery falls back to a full rebuild_from over the durable item store (state_rebuild.rs:577), which is correct but O(all-items) on every open.
• Fix: Thread the real WAL sequence number into PendingWrite.seq (the same last_wal_seq the rebuild uses) so the syncer commits a meaningful checkpoint, and have open-time recovery resume from the committed seq rather than always full-rebuilding. If deferring, replace the inline comment with a tracked issue reference per §12.

export_signals materializes the entire WAL into memory before applying the limit (db-infra)

tidal/src/db/export.rs:261 · Accuracy

• Issue: read_all_events(&wal_dir) (wal/reader.rs:122) reads every event from every segment into one unbounded Vec, then export_signals filters and only truncates to effective_limit at line 308. The MAX_EXPORT_LIMIT (500K) and the per-request limit bound the OUTPUT, not the working set. Between checkpoints the WAL can hold tens of millions of events (compaction only deletes segments fully covered by a checkpoint, which runs every 30s but can fall behind under load). So export_signals(&ExportRequest { limit: Some(10), .. }) can still allocate a Vec of every event in the WAL.
• Fix: Push the time-range and limit down into the WAL scan: stream segments newest-relevant-first (segments are seq-ordered; since/until let you skip whole segments by their min/max timestamp) and stop once effective_limit matching events are collected. Add a streaming variant read_events_filtered(dir, since, until, type_filter, limit) that short-circuits, or at minimum cap the in-memory Vec at effective_limit during the scan loop and document that exports are most-recent-N within the range. Add a test with a WAL far larger than the limit asserting bounded allocation / early termination.

signal_for_tenant silently drops remote-shard writes but its rustdoc claims dual-write (db-infra)

tidal/src/db/replication_ops.rs:96 · Completeness

• Issue: The method rustdoc (lines 65-67) states 'During a dual-write migration, the signal is written to both the source and target shard.' But the remote-shard branch (lines 94-105) only emits tracing::debug! and returns Ok(()) — the write to the target shard never happens. signal_for_tenant returns Ok overall, so the caller believes the dual-write succeeded.
• Fix: Make the doc match reality: state that remote-shard dispatch is not yet wired and that calling this against a shard not local to this node is currently a no-op for the remote target. Better, return a typed error (e.g. TidalError::Internal or a dedicated NotImplemented) when an assignment resolves to a non-local shard and no transport is configured, so a migration cannot silently lose writes. Track the 'Task 5' wiring with a real issue link per CODING_GUIDELINES §12.

Text index open failure is swallowed with no log, silently disabling text search (db-infra)

tidal/src/db/text_syncer.rs:54 · Completeness

• Issue: spawn_text_syncer matches TextIndex::open/ephemeral and on Err(_) returns a bundle with index: None, write_tx: None — discarding the error entirely. No tracing::warn!/error!. The caller cannot distinguish 'no text fields configured' from 'text index failed to open' (e.g. corrupted Tantivy dir, permission error, disk full).
• Fix: Log the discarded error before returning the empty bundle: Err(e) => { tracing::error!(index = index_name, error = %e, "text index open failed; text search disabled for this index"); return TextSyncerBundle::empty(); }. Likewise replace the .spawn(...).ok() with an explicit match that logs on Err. Consider surfacing it through the text_unhealthy latch so /health reflects the disabled index.

wal_lag_bytes gauge sums ALL segments but is documented as 'not yet compacted' (db-infra)

tidal/src/db/state_rebuild.rs:629 · Tech Debt

• Issue: compute_wal_lag_bytes sums the file sizes of every segment returned by list_segments(wal_dir). It is stored into metrics.wal_lag_bytes, whose field doc (metrics/mod.rs:67) and Prometheus HELP (mod.rs:311) both say 'Bytes of WAL segments not yet compacted'. After compact_wal_online deletes covered segments the remaining sum does approximate the uncompacted tail, but it also includes the active segment the writer is appending to and any segment retained for reasons other than lag, so the number is not the 'lag behind checkpoint' the name implies.
• Fix: Either rename/redocument the metric to 'total on-disk WAL bytes' (honest about what it measures), or compute true lag by summing only segments with seq > last-checkpointed seq (the checkpoint already knows last_wal_seq). Given compact_wal_online runs right before this, the simplest correct fix is to document it as total retained WAL bytes and add a separate uncompacted-since-checkpoint gauge if true lag is needed.

Hide item ID truncated to u32 without the warning the item-write path emits (entities)

tidal/src/entities/user_state.rs:93 · Accuracy

• Issue: Item-side IDs are truncated u64->u32 throughout (seen/hidden/saved/liked bitmaps), which is a documented ~4B-item contract — but db/items.rs:115-120 logs a warn when an item ID exceeds u32::MAX so collisions are observable. The Hide path does not: db/relationships.rs:51 and db/state_rebuild.rs:114 do to.as_u64() as u32 silently. add_hide/remove_hide (user_state.rs:93,190) take u32 with no guard.
• Fix: Centralize the u64->u32 item-slot narrowing in one helper that emits the same tracing::warn on overflow, and route add_hide / the Hide relationship path through it. This keeps the documented universe limit but makes every truncation observable and consistent.

RelationshipType::Mute silently no-ops in both write and rebuild paths (entities)

tidal/src/entities/relationship.rs:15 · Maintainability

• Issue: Mute is a defined RelationshipType (relationship.rs:15) but write_relationship (db/relationships.rs:60) and delete_relationship (relationships.rs:106) handle it via a catch-all _ => {} arm, and rebuild_entity_state (db/state_rebuild.rs:131-134) just increments a counter with the comment 'Mute edges do not have in-memory state (yet)'. The edge is persisted but has no effect anywhere.
• Fix: Either implement Mute (add a muted set to UserStateIndex and apply it in the unblocked/unseen predicates, plus rebuild) or remove the variant from the public enum until it is implemented. If deferral is intentional, replace the catch-all arms with explicit RelationshipType::Mute => {} arms carrying a tracking-issue TODO per §12 so the no-op is deliberate and greppable rather than absorbed by _.

Capability eviction can drop a live token, silently denying a valid capability until restart (governance)

tidal/src/governance/capability.rs:308 · Accuracy

• Issue: When the registry is at cap and no dead token exists, evict_one() removes the oldest LIVE token (line 321-327). check() (line 361) consults only the in-memory tokens/by_agent maps; it never consults the durable Tag::Capability row. So once a still-valid, non-expired token is evicted, every check() for that agent/scope returns InsufficientCapability even though the grant is durably valid and unexpired. The durable row only re-populates the registry on the next open() (db/mod.rs:512-523), so the false denial persists for the life of the process.
• Fix: On a check() miss for an agent that has durable tokens, fall back to loading from Tag::Capability (lazy rehydrate) before returning a denial; or make eviction of a live token also load-shed from durable storage on demand. At minimum, emit tracing::warn! when evict_one() drops a live (non-revoked, non-expired) token so the condition is observable, and consider making the cap a soft target that grows rather than evicting live grants. The current silent-deny-on-eviction is the worst of the options.

ScopeClass duplicates SignalScope's variant set and discriminant ordering by hand (governance)

tidal/src/governance/capability.rs:28 · Extensibility

• Issue: ScopeClass (Local/Community/Session/Agent with explicit repr(u8) values) re-encodes the same four-way taxonomy and ordering already defined by SignalScope (scope.rs:62) and its discriminant() (scope.rs:77). ScopeClass::of (capability.rs:42) bridges them. Adding or reordering a scope requires editing SignalScope, SignalScope::discriminant, SignalScope::from_discriminant, AND ScopeClass plus its repr values, with no compile-time link guaranteeing the two stay aligned.
• Fix: Add a const assertion or a test that pins ScopeClass repr values equal to SignalScope::discriminant for each variant (e.g. assert_eq!(ScopeClass::Community as u8, SignalScope::Community(CommunityId::NONE).discriminant())). Better, derive ScopeClass purely from SignalScope and drop the separate repr, or generate both from one macro so a new variant forces both sides to update.

Rate limiter accepts zero/negative rate, permanently locking out agents and emitting u64::MAX retry (load-testing)

tidal/src/load/rate_limiter.rs:107 · Accuracy

• Issue: RateLimiterConfig::limited and the struct literal path perform no validation. If signals_per_second (the bucket refill_rate) is 0.0, refill() never adds tokens, so once burst_capacity is drained the agent is locked out forever. retry_after_ms() then computes deficit / 0.0 == f64::INFINITY, and (INFINITY * 1000.0).ceil() as u64 saturates to u64::MAX (rate_limiter.rs:67-72), surfacing a ~584-million-year retry_after_ms in TidalError::RateLimited. A negative rate produces a negative added (tokens decrease over time) — equally broken. This is production code (pub mod load, not cfg-gated), so a misconfigured embedder silently bricks all session writes for an agent with no diagnostic.
• Fix: Make RateLimiterConfig::limited fallible (return Result<Self, String>) or validate in RateLimiter::new / TidalDbBuilder::with_rate_limiter_config: reject signals_per_second that is NaN, <= 0.0 (when not INFINITY), and burst_capacity < 1.0. Mirror the pattern already used by DegradationThresholds::new (detector.rs:98). Add a unit + proptest asserting a 0.0 rate is rejected at construction, not at first write.

Unlimited-mode rate-limiter check allocates a String and touches DashMap on every session write (load-testing)

tidal/src/load/rate_limiter.rs:172 · CLEAN

• Issue: RateLimiter::check always does let key = (agent_id.to_owned(), session_id) then buckets.entry(key).or_insert_with(...) — even in the default Unlimited config (signals_per_second = INFINITY, burst = INFINITY). The default is unlimited (the documented common case), yet every session_signal() (sessions.rs:308) pays one heap allocation for the agent_id String plus a DashMap shard lock + entry creation, purely to acquire a token from an infinite bucket that can never deny. CODING_GUIDELINES §1 explicitly forbids per-write String allocation on the hot path and prefers interned/u64 keys.
• Fix: Add a fast path: if self.config.signals_per_second.is_infinite() && self.config.burst_capacity.is_infinite() return Ok(()) before constructing the key — no allocation, no map touch. Alternatively store an enabled: bool on RateLimiter computed once at construction. Keep the existing keyed path only for the limited configuration.

write_signal commits to the leader before a panicking encode step, leaving leader ahead of an un-shipped batch (load-testing)

tidal/src/testing/cluster.rs:347 · Accuracy

• Issue: In SimulatedCluster::write_signal the leader db.signal() write (cluster.rs:347-349) is applied first; only afterward is the WAL batch encoded with encode_batch(...).expect("WAL batch encoding must not fail") (cluster.rs:376-377) and the seqno bumped. If encode_batch ever returns Err (e.g. a future event-count or size limit), the .expect() panics AFTER the leader already mutated its ledger and AFTER total_signals was not yet incremented, leaving the harness in an inconsistent leader-ahead state mid-test and aborting via panic rather than surfacing crate::Result. The function already returns crate::Result, so the error channel exists.
• Fix: Encode the batch and resolve type_id BEFORE writing to the leader, and propagate errors with ? instead of .expect(): compute let bytes = encode_batch(...).map_err(|e| TidalError::internal("write_signal", e.to_string()))?; first, then perform the leader db.signal() write, then ship. This keeps the leader state and the shipped batch atomic with respect to encode failures.

for_creator scope silently lost when creator_items index is absent (query-executor)

tidal/src/query/executor/pipeline.rs:88-98 · Accuracy

• Issue: The Stage 1 branch is if let Some(creator_id) = query.for_creator && let Some(creator_items) = self.creator_items. When for_creator is set but creator_items is None, the entire condition is false and execution falls into the else block, which runs the profile's normal candidate strategy (typically a full-universe scan). The for_creator restriction is silently dropped: a query for one creator's items returns items from all creators.
• Fix: When query.for_creator.is_some() but self.creator_items.is_none(), push an explicit warning and return an empty candidate set (the for_creator scope cannot be satisfied), or return a QueryError. Do not fall through to a full scan that ignores the requested scope. Mirror the warning-on-missing-index pattern used in Stage 2.

Social-graph trending scope resolution only inspects top-level And, diverging from the filter that recurses into Or (query-executor)

tidal/src/query/executor/mod.rs:499-505 · Extensibility

• Issue: extract_social_graph_params walks only SocialGraph and And(exprs); it does not look inside Or or Not. But the Stage 2.5 SocialGraph filter is extracted via extract_user_state_filters, which recurses into Or (user_filter.rs:107-111). So for an Or-nested SocialGraph, the inclusion filter fires but the trending-scope resolution (with_social_graph_users) does not — the scoring scope and the filtering scope disagree about which social graph is active.
• Fix: Make extract_social_graph_params share the same traversal discipline as the filter extractors (the And/Or walk, deliberately skipping Not), or better, have stage3 read the SocialGraph params from the same extract_user_state_filters output the Stage 2.5 filter uses, so a single extraction drives both filtering and scope resolution.

Test scaffolding duplicated verbatim across executor test modules (query-executor)

tidal/src/query/executor/tests_part2.rs:25-96 · DRY

• Issue: test_schema, setup_registry, add_item, and make_executor are copy-pasted byte-for-byte from tests.rs:20-91. The inject_exploration tests are also duplicated between candidate_gen.rs:165-251 and tests_part2.rs:366-453. The part2 docstring attributes the split to the ≤600-line file cap (§9).
• Fix: Extract a mod test_support (or pub(super) mod fixtures) under executor/ holding test_schema/setup_registry/add_item/make_executor, and have both tests.rs and tests_part2.rs use super::test_support::*. Delete the duplicated exploration tests from tests_part2.rs since candidate_gen.rs already owns them. This also lets the two test files shrink back under the cap without copy-paste.

Stale #[allow(dead_code)] comment on validate() actively misleads — the executor calls it (query-retrieve-fusion)

tidal/src/query/retrieve/types.rs:147-149 · Tech Debt

• Issue: The comment reads // Used by the query executor in Task 02. Until then, only tests reference it. followed by #[allow(dead_code)]. validate() is in fact called on the live read path at query/executor/pipeline.rs:44 (query.validate(self.profile_registry)?;). The allow attribute is now suppressing nothing and the comment asserts the opposite of reality.
• Fix: Remove the #[allow(dead_code)] and replace the comment with the current truth, e.g. // Called by RetrieveExecutor::execute before planning (query/executor/pipeline.rs). If the attribute is still needed for some build configuration, document the real reason; otherwise delete it.

Cursor offset truncates on 32-bit targets via usize<->u64 casts (query-retrieve-fusion)

tidal/src/query/retrieve/cursor.rs:42-50 · Accuracy

• Issue: from_offset stores offset as u64 and offset() returns self.last_id.as_u64() as usize. On a 32-bit target an offset (or a decoded u64 that exceeds u32::MAX from a crafted cursor) is truncated when cast back to usize, silently changing the page position. The decode path accepts any 8-byte big-endian value as last_id, so a hostile or corrupted cursor can carry a value > u32::MAX.
• Fix: Validate the decoded offset fits in usize before use (return QueryError::InvalidCursor on overflow), or store the offset width-explicitly and clamp with try_into().map_err(...) rather than as usize. The executor already saturating_add-clamps end, but the offset itself is read before that, so the guard belongs in offset()/decode().

for_creator() silently truncates creator IDs above u32::MAX (query-retrieve-fusion)

tidal/src/query/retrieve/types.rs:373-378 · Accuracy

• Issue: self.filters.push(FilterExpr::CreatorEq(creator_id.as_u64() as u32)) truncates any creator EntityId > u32::MAX to its low 32 bits. The docstring documents this as a 'Known Limitation', but the builder still accepts the oversized ID, applies a wrong filter, and ALSO stores the full 64-bit id in self.for_creator — so the bitmap filter and the for_creator restriction can disagree for the same query.
• Fix: Make for_creator fallible (return Result) or validate at build()/validate() time: reject creator_id > u32::MAX with QueryError::InvalidFilter until the bitmap index is upgraded to 64-bit keys. A silent narrowing cast on a user-supplied entity ID should never produce results — it should error.

Deferred filter AND-combined with an empty universe drops every candidate (query-search)

tidal/src/query/search/executor/pipeline.rs:393 · Accuracy

• Issue: In apply_metadata_filter, FilterEvaluator returns self.universe.clone() for deferred variants (MinSignal/MaxSignal/NearLocation/InCollection — see filter/evaluator.rs:92-97), and evaluate_and intersects children against the universe (evaluator.rs:102-105). When the executor's universe is None or an empty RoaringBitmap, the resulting FilterResult::Bitmap is empty and candidates.retain drops ALL candidates — including ones that should pass — before the Stage 2.2/2.5 post-filters ever run. This is currently only masked because production always wires a populated self.universe (query_ops.rs:225) and maintains it on writes; the test harness documents the trap explicitly at pipeline.rs:836-840 ('would intersect against an empty universe and drop everything'). A poisoned universe lock (pipeline.rs:387 falls back to &empty_universe) reintroduces the bug at runtime.
• Fix: Decouple deferred-filter handling from the universe sentinel. In apply_metadata_filter, evaluate only the index-resolvable sub-expression against the real indexes and treat deferred variants as pass-through (return the current candidate set unchanged) rather than intersecting with the universe — the actual deferred filtering already happens in apply_deferred_post_filters. Alternatively, when self.universe is None or the read lock is poisoned, skip the bitmap intersection entirely (retain all) and rely on the post-filter stage, logging a warning. Add a test: deferred MinSignal filter with no universe wired must still return the candidates and let the post-filter trim them.

combined_filter() recomputed and cloned five times per query in the hot path (query-search)

tidal/src/query/search/executor/pipeline.rs:95 · DRY

• Issue: Search::combined_filter() (types.rs:104-110) clones self.filters and, for >1 filter, allocates a new FilterExpr::And(self.filters.clone()) — a full deep clone of every FilterExpr node. execute() calls it at pipeline.rs:95, then apply_metadata_filter (364), apply_creator_metadata_filter (413), apply_deferred_post_filters (459), and apply_user_context_filter (505) each call it again. That is up to five independent deep clones of the entire filter tree on every single search.
• Fix: Compute the combined filter once at the top of execute() (let combined = query.combined_filter();) and pass combined.as_ref() (an &FilterExpr) down to each stage helper, changing their signatures to take Option<&FilterExpr>. The deferred-filter and user-context stages already only read it, so a borrow suffices — no clone needed.

Silent empty-result degradation on poisoned universe lock with no warning (query-search)

tidal/src/query/search/executor/pipeline.rs:249 · Completeness

• Issue: resolve_scope reads the universe via u.read().map(|g| g.clone()).unwrap_or_default(); on a poisoned lock this yields an empty RoaringBitmap. For WithinScope::Trending with no velocity data, resolve_trending returns self.universe.clone() (scope.rs:128-129,150-151) — an empty bitmap — so the scope pre-filter silently restricts retrieval to nothing and the query returns zero results with no warning pushed. apply_metadata_filter (pipeline.rs:387) similarly falls back to &empty_universe on poison.
• Fix: On a poisoned universe read, push a warning into the warnings vec ('universe lock poisoned; scope/filter pre-filter degraded') so it surfaces in SearchResults.warnings, or recover the guard via PoisonError::into_inner() (as suggest.rs already does at suggest.rs:113-114,172-173) so the last-known universe is used instead of silently empty. Be consistent with the suggest module's poison-recovery pattern.

Shortest sort: bottom-ranked missing-duration item is rewritten to the maximum score 1.0 by normalize() (ranking)

tidal/src/ranking/executor/helpers.rs:199 · Accuracy

• Issue: score_shortest returns -duration (a negative score for any present item) and f64::NEG_INFINITY for a missing duration. finalize() sorts first (total_cmp ranks NEG_INFINITY last, correct order), then calls normalize(), which clamps NEG_INFINITY to 0.0 and min-max normalizes. Because every present item has a NEGATIVE score, the clamped 0.0 of the missing item becomes the MAX, so it normalizes to 1.0 while every present (shorter) item normalizes below it. normalize does not re-sort, so the order stays correct, but the reported score of the last-ranked item is the highest in the set. The existing test sort_missing_duration_last only asserts position (result[2].entity_id), so it passes despite the anomaly.
• Fix: Stop encoding 'sort last' as NEG_INFINITY on a negated scale. In score_shortest, return -duration for present items and a missing-sentinel that is strictly below the minimum real score after collection (or carry a 'missing' flag honored by both the comparator and normalize), so normalization maps the missing item to 0.0 and keeps score monotonic with rank. Add a test asserting result.last().score <= result.first().score for Shortest with a missing duration.

now: Timestamp is threaded through the entire scoring path but is dead; all time-sensitive reads use Timestamp::now() internally (ranking)

tidal/src/ranking/executor/scoring.rs:121 · Maintainability

• Issue: score() / score_personalized() / score_with_session() all take now: Timestamp and pass it to score_by_sort(), which passes it to score_hot() as _now (unused). Every actually time-dependent read — DecayScore via SignalLedger::read_decay_score (core.rs:224 calls Timestamp::now()) and ProfileDecay's recency read — ignores the threaded now and re-reads wall-clock time. So the now parameter influences nothing in the current scoring implementation.
• Fix: Decide the clock contract and enforce it end to end. Preferred: plumb now down — add read_decay_score_at(entity, signal, idx, now_ns) to SignalLedger and have read_agg/compute_raw_score pass the executor's now so scoring is a pure function of (ledger snapshot, now). Alternatively, if wall-clock-at-read is intentional, delete the now parameter from the executor surface and the score_hot _now, and document that decay is evaluated at call time.

Mid-payload corruption commits earlier batches then halts with no durable recovery point (replication-core)

tidal/src/replication/receiver.rs:189 · Accuracy

• Issue: apply_payload iterates batches in a payload and, for each non-corrupt batch, applies its events to the ledger (line 189-195) and advances applied_seqno (line 198) immediately. If a later batch in the same payload fails decode, the function returns Err at line 204 — but the earlier batches' mutations and HWM advances have already happened. The receiver thread then halts (receiver.rs:106). Because no follower state is persisted (see the BLOCKER), there is no clean point to resume from: the in-memory ledger holds a partial payload, applied_seqno reflects the last good batch, and the only recovery is a full restart that replays from 0.
• Fix: Make payload application atomic: decode and validate ALL batches in the payload first (collect (header, events, batch_last_seq)), and only after the whole payload validates, apply events and advance applied_seqno to the payload's final boundary in one step. If any batch is corrupt, return Err before mutating any state, so the follower's HWM and ledger never reflect a half-applied payload. This also lets the leader cleanly re-ship the same un-acked segment.

Lag gauge under-reports leader seqno when community-overlay drops an all-local segment (replication-core)

tidal/src/replication/receiver.rs:175 · Accuracy

• Issue: The lag gauge's leader_seqno is advanced only from batch_last_seq of batches actually decoded in apply_payload (line 175-177). In community_share_only mode, filter_segment_drop_local (shipper.rs:348) drops batches whose events are entirely local, and a segment with no shippable events ships as empty bytes; apply_payload over empty bytes decodes zero batches and never calls update_leader_seqno for that segment's seqno range. The shipper's peer_hwm still advances (a successful empty send), so replication progress is fine, but the receiver's view of the leader high-water-mark skips those seqnos.
• Fix: Carry the segment's true last WAL seqno in WalSegmentPayload (the leader knows it before filtering) and feed update_leader_seqno from that authoritative value on the receive path, independent of how many batches survived the filter. Alternatively, in community mode emit a zero-event boundary batch that preserves the seqno range so the receiver can still observe it. Add a test asserting the gauge tracks the leader HWM across an all-local (empty) segment.

Dual-write migration silently drops remote-shard signals as a logged no-op (replication-core)

tidal/src/replication/../db/replication_ops.rs:96 · Tech Debt

• Issue: signal_for_tenant resolves write_assignments (1 in normal mode, 2 during dual-write migration) and, for any assignment that is not the local shard, only emits a tracing::debug! 'pending remote-shard dispatch (transport not yet wired)' (replication_ops.rs:96-105). The signal to the remote shard is silently dropped. The comment says this will be wired 'once the shipper is integrated (Task 5)'.
• Fix: Either wire the remote-shard write through the transport now, or fail closed: return TidalError::Internal/Unsupported when an assignment targets a non-local shard and no transport is configured, so a misconfigured dual-write is loud rather than silently lossy. At minimum raise the log to warn! and gate dual-write migration behind an explicit capability check. Track the deferral with an issue link per CODING_GUIDELINES §12 ('No TODO without an issue').

Quarantine and high-water-mark state live only in the shipper thread's stack — no operator visibility or reset without restart (replication-core)

tidal/src/replication/shipper.rs:115 · Extensibility

• Issue: peer_hwm and quarantined (shipper.rs:105,115) are local variables inside the spawned thread closure. The only way to clear a quarantine is to drop the WalShipperHandle and re-spawn the whole shipper (documented intent). There is no API to query which peers are quarantined, what each peer's HWM is, or to clear a single peer's quarantine after an operator fixes its config.
• Fix: Hoist peer_hwm and quarantined into a shared, lock-protected (or DashMap) structure owned by WalShipperHandle, expose read accessors (quarantined peers, per-peer HWM) for the control plane / metrics, and add a clear_quarantine(peer) method that the operator endpoint can call. This also lets the HWM be checkpointed alongside ReplicationState.

CRDT reconciliation engine is never invoked in production — partitions never heal (replication-crdt-tenant)

tidal/src/replication/reconcile.rs (engine) + tidal/src/db/replication_ops.rs (no caller) · Completeness

• Issue: ReconciliationEngine::new, StateSnapshot population, and CrdtSignalState::merge are referenced only from test files (m8p3_crdt.rs, m8_uat.rs, reconcile_tests.rs). No production code under tidal/src/db/ ever builds a StateSnapshot from the live signal ledger or calls engine.plan()/apply(). The entire crdt/ module — the subsystem's reason for existing ('deterministic reconciliation after network partitions') — is dead weight at runtime: after a real partition heals, nothing merges the diverged per-node decay scores or hard-negative registers.
• Fix: Add a production TidalDb::take_crdt_snapshot() that walks the signal ledger and hard-neg index into a StateSnapshot (populating windowed bucket counts via from_node_contribution(..., bucket_count, ...)), and a reconcile_with(remote_snapshot) entry point that instantiates ReconciliationEngine, computes the plan, and applies it through apply_crdt_state. Wire it into the reconnection/anti-entropy flow and cover it with a multi-node integration test that partitions, writes divergently on both sides, heals, and asserts merged scores.

"Task 5 not yet wired" stub is a TODO without a tracking issue (replication-crdt-tenant)

tidal/src/db/replication_ops.rs:96 · Tech Debt

• Issue: The remote-dispatch gap is documented inline as 'will be wired here once the shipper is integrated (Task 5)' with no issue link. CODING_GUIDELINES §12 forbids TODOs without a tracking issue ('Orphan TODOs rot').
• Fix: File a tracking issue for the shipper-integrated remote dispatch, reference it in the comment, and (per the BLOCKER fix) make the path fail loudly until it is closed so the debt cannot be silently relied upon.

from_node_contribution windowed-count round-trip is only ever exercised with bucket_count=0 (replication-crdt-tenant)

tidal/src/replication/crdt/signal_state.rs:98 · Completeness

• Issue: The bucket_count parameter and its round-trip guarantee are well-implemented and unit-tested (from_node_contribution_preserves_windowed_bucket), but every production/integration caller (db/, m8p3_crdt.rs, reconcile_tests.rs) passes 0. Because no production snapshot builder exists (see CRITICAL #1), the windowed-count preservation path is real but unreached, and apply_crdt_state (core.rs:408) folds state.total_count() — which would be 0 — into the warm tier on reconcile.
• Fix: When implementing take_crdt_snapshot (CRITICAL #1 fix), populate bucket_count from the ledger's live windowed count for each node, and add an integration test that reconciles two nodes with non-zero windowed counts and asserts total_count survives end-to-end.

EntityKind->fingerprint-byte mapping duplicated in three places, aligned only by comment (schema)

tidal/src/schema/validation/mod.rs:154 · DRY

• Issue: The Item=0/User=1/Creator=2 byte encoding exists in three independent sites: entity_kind_ord() at validation/mod.rs:154 (used to sort embedding_slot_fingerprint), and two inline match blocks in db/schema_fingerprint.rs:43-47 and :72-76 (used to hash the kind into the fingerprint). They are coupled by nothing but a comment ('Matches the discriminant bytes the schema fingerprint already assigns'). If any site diverges, the slot sort order and the hashed bytes disagree, changing the fingerprint and producing either a spurious SchemaMismatch that bricks reopen of a valid data dir, or mis-sorted slots. Adding a 4th EntityKind variant is compiler-caught (matches are exhaustive) but the byte values are not enforced to stay aligned.
• Fix: Add a single canonical pub(crate) const fn fingerprint_byte(self) -> u8 to EntityKind in schema/entity.rs returning 0/1/2, and call it from all three sites. Delete entity_kind_ord and both inline matches. One source of truth, compiler-enforced exhaustiveness, and the byte values can never drift.

Agent-policy validation branch has zero test coverage (schema)

tidal/src/schema/validation/builders.rs:251 · Completeness

• Issue: build() validates session policies (InvalidPolicyName at :251, DuplicatePolicyName at :254, PolicySignalNotInSchema at :264, PolicySignalConflict at :273), but a grep across tidal/src and tidal/tests finds these four SchemaError variants only in their definition and the build() logic — never in an assertion. Every other build() branch (duplicate signal, invalid name, half-life, lifetime, empty windows, velocity, all embedding-slot rules, all text-field rules) has an explicit reject test in builders.rs; the policy branch is the lone untested one.
• Fix: Add unit tests in builders.rs mirroring the existing reject-test style: one each asserting build() returns InvalidPolicyName (e.g. 'Bad-Name'), DuplicatePolicyName (two session_policy calls with same name), PolicySignalNotInSchema (allowed_signals referencing an undeclared signal), and PolicySignalConflict (a signal in both allowed and denied lists). Also one happy-path test that a valid policy survives build().

WAL replay collapses all session signals into one minute bucket, corrupting restored window counts (session)

tidal/src/db/session_restore.rs:173 · Accuracy

• Issue: On crash recovery, restore_session_wal_events creates each per-signal SessionSignalState with SessionSignalState::new(now_ns, lambda) — seeding the BucketedCounter's rotation timestamps to the restore-time wall clock (warm.rs with_start_time). It then replays each historical event with its original past ts_ns (line 174 ss.on_signal(.., *ts_ns)). BucketedCounter::increment calls maybe_rotate(ts_ns), which early-returns whenever now_ns < last_min + NS_PER_MIN (warm.rs:354). Because every replayed ts_ns is in the past relative to the start time seeded as now_ns, no rotation ever fires and every replayed event lands in the single current minute bucket regardless of its true age. The live path does the opposite: session_signal seeds the counter with the event's own ts_ns (db/sessions.rs:400 SessionSignalState::new(ts_ns, lambda)). The result is that a session restored from the WAL reports a window_1h (and any windowed count) that does not match the pre-crash session, directly violating CODING_GUIDELINES §8 'WAL replay produces identical state to uninterrupted execution.' The hot decay score is unaffected (it is timestamp-exact via forward_decay_step), so the corruption is silent — scores look right while window counts are wrong.
• Fix: Seed the restored counter with the earliest replayed event timestamp, not now_ns, and replay events in timestamp order so rotations fire as they did originally. Concretely: pre-sort each session's signals by ts_ns, create SessionSignalState::new(first_ts_ns, lambda), and feed events in ascending ts_ns so maybe_rotate advances buckets identically to the live path. Add a crash-recovery property test (per §8) asserting that a session built live and the same session rebuilt from its WAL events produce byte-identical SignalSnapEntry.window_1h.

f64 session signal weight is narrowed to f32 on the WAL path, so replayed scores lose precision (session)

tidal/src/db/sessions.rs:439 · Accuracy

• Issue: session_signal accepts weight: f64 and applies it to the in-memory decay score at full f64 precision (sig_entry.on_signal(weight, ts_ns)), but the WAL append narrows it: wal.session_signal(.., weight as f32, ..). On replay, restore_session_wal_events reconstructs the score from the f32 value (session_restore.rs:174 f64::from(*weight)). A session restored from the WAL therefore has a different decay score than the live session had, even ignoring the bucketing bug above. The SessionWalEvent::Signal struct itself stores weight as f32 (replication/session_bridge.rs:405), so the truncation is baked into the wire format.
• Fix: Store the session signal weight as f64 in SessionWalEvent::Signal and the session journal codec (wal/format/session.rs), matching the f64 precision the in-memory path uses. If wire-size is a concern, document the f32 truncation explicitly as an accepted lossy bound on session weights and clamp/round deterministically; do not silently narrow at the call site.

Session-journal append doc-comment claims a durability guarantee the caller never waits for (session)

tidal/src/wal/mod.rs:286 · Maintainability

• Issue: Wal::session_signal/session_start/session_close are fire-and-forget: they send a WalCommand to the writer thread and return Ok(()) immediately (wal/mod.rs:355-366), without a reply channel — unlike Wal::append which blocks on reply_rx for the fsync ack (wal/mod.rs:295-305). The block comment at wal/mod.rs:286-287 above the session methods is inherited from append and states the event 'has been durably fsynced to disk' before return, which is false for the session path. The session-layer callers (db/sessions.rs:430-448) correctly understand this — they log 'silently lost on recovery' on send failure — but the WAL-layer doc is misleading for the next maintainer.
• Fix: Move/rewrite the doc on the three session-journal methods to state explicitly: 'Fire-and-forget. Returns Ok once the command is enqueued to the writer thread; durability (the per-write fsync) happens asynchronously and is NOT awaited. A crash between enqueue and fsync loses the event; in-memory state is the source of truth.' Keep the per-write-fsync description on the writer-thread side where it is accurate.

read_decay_score silently returns an undecayed score for an out-of-range decay_rate_idx (signals)

tidal/src/signals/ledger/core.rs:226 · Accuracy

• Issue: For a decay_rate_idx beyond the signal's lambda vector, the lambda lookup falls back to 0.0 (unwrap_or(0.0)), and current_score (hot.rs:205) saturates the idx to MAX_DECAY_RATES-1. The net effect: an out-of-range index returns stored_score[clamped] with lambda 0.0, i.e. a raw, never-decaying score, with no error. Because Exponential signals only register one lambda (ledger/mod.rs:68 -> vec![*lambda]), any decay_rate_idx >= 1 hits this path and returns 0.0-or-stale silently. A ranking profile that references a non-existent decay rate gets a wrong-but-plausible number instead of a schema error.
• Fix: When signal_lambdas[type_id].get(decay_rate_idx) is None, return TidalError::Schema (e.g. a DecayRateOutOfRange variant) rather than defaulting lambda to 0.0, OR return Ok(None) to signal 'no such decay rate'. At minimum, assert idx < lambdas.len() and surface a distinct result so the caller can distinguish 'no signals recorded' from 'no such decay index'.

Get-or-create EntitySignalEntry block duplicated across four ledger methods (signals)

tidal/src/signals/ledger/core.rs:105 · DRY

• Issue: The identical entries.entry((id, type_id)).or_insert_with(|| EntitySignalEntry { hot: HotSignalState::new(...), warm: BucketedCounter::with_start_time(ts_ns) }) followed by entry.hot.on_signal(...); entry.warm.increment(...); drop(entry) pattern is copy-pasted in record_signal (105), record_signal_scoped (194), apply_wal_event (324), and partially in apply_crdt_state (384). A future change to entry construction or the hot/warm update sequence must be made in four places.
• Fix: Extract a private helper, e.g. fn apply_event_local(&self, entity_id, type_id, weight, ts_ns, lambdas: &[f64]) that does the or_insert_with + on_signal + increment + drop, and have record_signal, record_signal_scoped (local branch), and apply_wal_event delegate to it. apply_crdt_state stays separate (force-set semantics) but can share the or_insert_with construction via a smaller fn get_or_create_entry.

Serialization byte offsets are hand-encoded magic numbers repeated in three independent places (signals)

tidal/src/signals/checkpoint/format.rs:75 · Maintainability

• Issue: The field offsets (1, 9, 11, 13, 21, 29, 37, 45, 46, 47, 55, 63, 71, 311, 983, 984, 992) appear as a doc-comment table, again as implicit sequential extend_from_slice calls in serialize_entry, and a third time as literal slice indices in deserialize_entry (bytes[13..21], bytes[47..55], etc.). serialize writes positionally while deserialize reads by absolute index, so the two are only kept in sync by hand. A single inserted field desyncs them with no compile error — only the roundtrip test would catch it.
• Fix: Define the offsets as named const usize values (OFF_ENTITY_ID, OFF_FLAGS, OFF_SCORE_0, ... OFF_DAY_BUCKETS) once, and use them in BOTH serialize (write at buf[OFF..]) and deserialize (read bytes[OFF..OFF+N]). Alternatively, drive both directions from a single struct with a derive-free manual codec, or add a const-assert that the sequential serialize offsets equal the named constants. This makes a desync a compile error, not a test-only catch.

All fjall errors flattened to StorageError::Corruption, mislabeling transient and lock failures (storage-engine-indexes)

tidal/src/storage/fjall.rs:52 · Accuracy

• Issue: map_fjall_err maps every fjall::Error to StorageError::Corruption { message }. fjall 3.x's Error enum distinguishes Io(std::io::Error), Poisoned, Locked, KeyspaceDeleted, and Unrecoverable from the genuinely corruption-shaped variants (Decompress, InvalidTrailer, InvalidTag, InvalidVersion). StorageError already has a dedicated Io variant and a Closed variant that go unused on this backend. So a disk-full ENOSPC on insert, a poisoned internal lock, or lock contention all surface to callers as 'data corruption'.
• Fix: Pattern-match the fjall error and route it: fjall::Error::Io(e) => StorageError::Io(...) (preserving the underlying io::Error), Poisoned/Locked/KeyspaceDeleted => a new transient/Closed-style variant, and only the decode/trailer/tag/version family => Corruption. Take e by value (not &) so the io::Error can be moved into StorageError::Io rather than stringified.

flush_all performs four full-database fsyncs where one is required (storage-engine-indexes)

tidal/src/storage/fjall.rs:256 · Accuracy

• Issue: FjallStorage holds one shared fjall::Database and three FjallBackends that each clone that same db handle. FjallBackend::flush() (line 122-135) rotates its memtable AND calls self.db.persist(SyncAll) on the shared database. flush_all() then calls items.flush(), users.flush(), creators.flush() — three SyncAll persists of the entire shared db — and follows with a fourth self.db.persist(SyncAll). Each SyncAll is a full fsync of data + metadata for the whole database, so the flush durability barrier fsyncs the same database four times.
• Fix: Separate memtable rotation from the fsync barrier. Either give FjallBackend a rotate_only() that just calls rotate_memtable_and_wait, have flush_all rotate all three keyspaces then call db.persist(SyncAll) exactly once; or keep flush() as the single-keyspace durability primitive and have flush_all rotate the three memtables and persist once. Net: 3 rotations + 1 fsync, not 4 fsyncs.

evaluate_and double-evaluates compound children (selectivity recursion then bitmap evaluation) (storage-engine-indexes)

tidal/src/storage/indexes/filter/evaluator.rs:109 · CLEAN

• Issue: evaluate_and computes (self.selectivity(c), c) for every child to sort by selectivity, then separately calls eval_to_bitmap on each child. For leaf predicates selectivity is a cheap cardinality lookup, but for nested And/Or/Not children, selectivity(c) recurses the whole subtree (evaluator.rs:182-196) and eval_to_bitmap(c) then re-walks and re-materializes that same subtree. A FILTER with nested boolean groups walks each subtree twice.
• Fix: For compound children, estimate ordering cost from a cheap proxy (child node_count, or memoize the selectivity result alongside the child) rather than a full recursive selectivity pass; or evaluate each child to a bitmap once, then sort the already-materialized bitmaps by len() before intersecting. The latter also gives exact, not estimated, ordering for the short-circuit.

scan_prefix eagerly materializes the entire result set into a Vec (storage-engine-indexes)

tidal/src/storage/fjall.rs:77 · Accuracy

• Issue: FjallBackend::scan_prefix collects the full fjall prefix iterator into a Vec<(Vec,Vec)> before returning a boxed iterator over it. The doc comment justifies this as avoiding holding the fjall snapshot across the iteration boundary, but it means an empty-prefix scan (scan_prefix(&[]), used by all_item_metadata in db/items.rs:220 to enumerate every item on restart) loads the entire keyspace — keys and values — into RAM at once. InMemoryBackend (memory.rs:71) does the same.
• Fix: Hold the fjall snapshot/guard inside an iterator adapter so entries stream lazily, mapping each guard.into_inner() to the (Vec,Vec) item on demand. If the borrow/lifetime makes streaming impractical for fjall's API, at minimum bound the eager collection (chunked scans) and document the memory ceiling of scan_prefix(&[]) prominently on the trait method.

FilterResult::cardinality and is_empty silently lie for the Predicate variant (storage-engine-indexes)

tidal/src/storage/indexes/filter/result.rs:79 · Tech Debt

• Issue: cardinality() returns 0 and is_empty() returns false for FilterResult::Predicate, with an inline comment admitting '0 is a safe default but incorrect.' Both variants are publicly constructible. A caller that uses cardinality() to size a buffer or to choose a query plan, or is_empty() to short-circuit, gets answers that are wrong in opposite directions (cardinality understates to 0, is_empty overstates to non-empty).
• Fix: Return Option from cardinality (None for Predicate) and Option from is_empty, forcing callers to handle the unknown case; or, if all current callers only ever hold Bitmap results, make the predicate-returning constructors crate-private and gate the Predicate variant behind try-accessors only. At minimum make is_empty return None/unknown rather than a definitive false.

Soft-delete leaves a durable key that rebuild_from_store resurrects on restart (storage-vector)

tidal/src/storage/vector/lifecycle/ops.rs:146 · Accuracy

• Issue: delete_embedding(hard_delete=false) removes the vector from the ANN index (index.delete) but intentionally keeps the entity-store key (the doc calls this 'archive / soft delete'). On the next process restart, rebuild_from_store (registry.rs:238) scans every EMB: key in the store and unconditionally re-inserts it into the index. The soft-deleted/archived vector therefore reappears in ANN search results after a restart — a silent state divergence between two reboots of the same database. There is no tombstone marker in the store to suppress it. This is currently latent only because delete_embedding has zero call sites outside its own unit tests (verified via grep across src/ and tests/).
• Fix: Make soft-delete durable. Write a tombstone marker for the embedding key (e.g. a zero-length value or a distinct ARCHIVED:{slot} key) when hard_delete is false, and have rebuild_from_store skip any EMB: key that has a corresponding tombstone. Alternatively, require all deletes that must survive restart to be hard deletes and remove the soft-delete code path until the entity-lifecycle layer has a durable archive flag the rebuild can consult. Add an integration test that inserts, soft-deletes, reopens the DB, and asserts the vector is absent from search.

total_slots counter is incremented via a non-atomic contains/add/fetch_add sequence (storage-vector)

tidal/src/storage/vector/usearch_index.rs:127 · Accuracy

• Issue: insert() does let is_new = !self.inner.contains(id); self.inner.add(id, ..); if is_new { total_slots.fetch_add(1, Relaxed) }. Inserts run under only a registry read lock (verified in db/items.rs:302-321), so two threads inserting the same previously-absent id can both observe is_new == true and both fetch_add, double-counting total_slots. Conversely the check-then-act is also racy against a concurrent delete. The struct docstring (usearch_index.rs:39-48) claims total_slots reports 'the true graph size'.
• Fix: Either guard insert with the registry write lock when total_slots must be exact, or stop maintaining the manual counter and derive len() from a source that is consistent under concurrency. If the counter stays best-effort, change the docstring from 'true graph size' to 'approximate occupied-slot count (best-effort under concurrency)' so no future code treats it as authoritative.

Slot lookup allocates a String on every search/insert hot-path call (storage-vector)

tidal/src/storage/vector/registry.rs:156 · CLEAN

• Issue: get() does self.slots.get(&(entity_kind, slot_name.to_owned())) and get_mut() does the same at line 165. The registry key is (EntityKind, String), so every lookup heap-allocates a String just to probe the map, then drops it. These functions are on the per-query and per-write path (every RETRIEVE/SEARCH resolves the slot, every write_*_embedding resolves it twice — items.rs:289,309).
• Fix: Avoid the allocation. Use the hashbrown/std raw-entry or Borrow-based lookup with a (EntityKind, &str) probe key — e.g. wrap the key type so it implements Borrow<(EntityKind, str)>, or restructure as HashMap<EntityKind, HashMap<String, EmbeddingSlotState>> so the inner lookup borrows &str directly. Slot counts are tiny (<=4), so the nested-map form is both faster and zero-alloc.

index_stats multiplies countdimbytes without overflow guards; panics in debug (storage-vector)

tidal/src/storage/vector/registry.rs:203 · Tech Debt

• Issue: total_bytes += count * dim * bytes_per_component uses unchecked multiplication on u64 values that come from index.len() and slot.dimensions. At very large scale (or with a corrupt/huge dimensions value) this overflows; in debug builds the multiply panics, in release it wraps to a wrong-but-silent footprint estimate. The surrounding code is otherwise meticulous about checked arithmetic (see brute/mod.rs:289-322), making this an inconsistency.
• Fix: Use saturating_mul/saturating_add: total_bytes = total_bytes.saturating_add(count.saturating_mul(dim).saturating_mul(bytes_per_component)). Saturation is the right semantic for a 'lower-bound estimate' stat — it can never panic or silently wrap.

AllScoresCollector.entity_id_field is set by every caller but ignored; for_segment hardcodes the "entity_id" string (text)

tidal/src/text/collectors.rs:51 · DRY

• Issue: AllScoresCollector exposes pub entity_id_field: Field and every construction site sets it (pipeline.rs:294, and the tests). But Collector::for_segment never reads self.entity_id_field — it resolves the fast field by the literal string ff.u64("entity_id"). The struct field is pure decoration: it implies the collector is parameterized over which field holds the id, when in reality the field name is hardcoded. If the schema's entity_id field were ever renamed or the struct field set to a different Field, the two would silently disagree and the collector would either fail at runtime (unknown column) or read the wrong column.
• Fix: Use the typed handle: resolve the column from the schema field carried in the struct rather than the magic string — e.g. derive the field name from self.entity_id_field via reader.schema().get_field_name(self.entity_id_field) and pass that to ff.u64(...), or (cleaner) drop the struct field entirely and centralize the column name in a single const ENTITY_ID_FIELD: &str = "entity_id" shared with build_tantivy_schema in index.rs so the name is defined exactly once.

first_or_default_col(0) can emit EntityId(0), which aliases a real entity, rather than signalling a missing id (text)

tidal/src/text/collectors.rs:52 · Accuracy

• Issue: for_segment builds the entity-id column with col.first_or_default_col(0), so any document missing an entity_id fast-field value collects as EntityId::new(0) (collectors.rs:81). EntityId has no NonZero invariant (schema/entity.rs:14 accepts any u64, and 0 is a legal id), so a defaulted 0 is indistinguishable from a genuine entity 0. In normal operation every doc gets an entity_id (writer.rs:69), so this is latent — but it converts a should-be-impossible 'document has no id' into a silently wrong result that points at entity 0.
• Fix: Use a sentinel default that cannot collide with a real id (e.g. u64::MAX) and filter those rows out in collect(), or assert the column is present (first_or_default_col is the lenient API; prefer the strict column accessor and propagate a tantivy error from for_segment if entity_id is absent). At minimum, skip rows whose value equals the chosen sentinel rather than pushing them as results.

Circuit breaker half-open does not enforce the documented single-probe invariant (tidal-net)

tidal-net/src/circuit_breaker.rs:60 · Accuracy

• Issue: The module doc and check() doc state half-open "allows one probe." In reality, once check() transitions Open→HalfOpen, every subsequent check() also matches the Closed { .. } | HalfOpen => Ok(()) arm and returns Ok until a success/failure is recorded. So N concurrent callers in half-open all pass and all probe a peer that just failed. Today the shipper is single-threaded so only one probe is in flight, which masks this — but the invariant the code advertises is not the invariant it implements.
• Fix: Make the state machine enforce it: add a HalfOpenInFlight (or a bool on HalfOpen) so the transition Open→HalfOpen returns Ok exactly once; subsequent check() calls while a probe is outstanding return CircuitOpenError until record_success/record_failure resolves it. Cover with a test asserting the second check() in half-open errors until the probe completes.

Transport trait contract says send_segment is "non-blocking"; gRPC impl blocks the shipper thread up to request_timeout (tidal-net)

tidal/src/replication/transport.rs:72 · Completeness

• Issue: The trait Contract section states send_segment is non-blocking (best-effort delivery). The gRPC implementation (transport.rs:141 self.runtime().block_on(self.pool.send_to(...))) synchronously blocks the calling thread for up to request_timeout (default 10s) per call. The implementation choice is correct and well-documented in tidal-net's own module doc, but the upstream trait contract is now stale and actively misleading: a caller who believes send_segment is non-blocking could call it from a latency-sensitive path and stall for seconds.
• Fix: Update the Transport trait doc in tidal/src/replication/transport.rs to state that send_segment MAY block up to an implementation-defined timeout and must be called from a dedicated shipper thread (never an async/latency-sensitive context). Reference the GrpcTransport block_on bridge. This is a doc fix to match the real, intentional behavior.

Segment-receiver thread parks forever in recv_segment and is only reclaimed at process exit (leaked thread on clean shutdown) (tidal-net)

tidal-net/src/transport.rs:146 · Tech Debt

• Issue: recv_segment blocks on block_on(rx.recv()). The only way it returns None is when ALL inbound_tx senders are dropped — but the inbound_tx lives inside the server task. On GrpcTransport::Drop the server task is aborted and the runtime is shut down via shutdown_background, yet the consumer of recv_segment is a separate detached std::thread (see tidal-server/src/cluster.rs:421 which explicitly says it "parks in recv_segment ... reclaimed when the process exits"). There is no shutdown signal that makes a parked recv_segment return None on demand, so a clean (non-process-exit) teardown leaks that thread and its block_on.
• Fix: Give recv_segment a cancellation path: select over rx.recv() and a shutdown notify (tokio::sync::Notify or a watch channel) that Drop trips before shutdown_background, so a parked recv_segment returns None deterministically and the receiver thread joins. Then have the receiver thread be joined on shutdown rather than detached, and add a test that drops the transport and asserts recv_segment returns None and the thread exits.

Coordinator-level max_per_creator resolves creators only from the leader replica, silently under-enforcing in entity-sharded mode (tidal-server)

tidal-server/src/scatter_gather.rs:575 · Accuracy

• Issue: enforce_max_per_creator is invoked with read_region = cluster.leader_region() (scatter_gather.rs:575 and :703) and reads each item's creator_id via db.get_item_metadata against that single node's local store (scatter_gather.rs:475). In a true entity-sharded topology, items owned by non-leader shards are NOT present on the leader (sharded_write_item writes metadata to one owning shard only — scatter_gather.rs:100-104), so their creator resolves to None and they are treated as uncapped. The doc comment at scatter_gather.rs:449-454 claims the coordinator 'reads each item from whichever live shard returned it,' which the code does not do.
• Fix: Either (a) resolve each item's creator from the shard that actually returned it — carry the source RegionId alongside each merged item through GatherState and look up metadata on that node; or (b) if only the replicated model is supported, change the doc comment to state that and add a debug_assert / config flag rejecting coordinator diversity in entity-sharded mode. Option (a) makes the doc comment true and the constraint actually hold.

health_startup and health_live are byte-identical duplicates across the two routers (tidal-server)

tidal-server/src/cluster.rs:558 · DRY

• Issue: health_startup and health_live in cluster.rs:558-564 are byte-for-byte identical to the same handlers in router.rs:270-277. The dto.rs module exists precisely to prevent this kind of cross-router duplication and its header comment warns that drift 'would silently change one mode's API.'
• Fix: Move health_startup, health_live, and the shared JSON body into a small health helper module (or into dto.rs alongside the other shared surface) and reference it from both routers, mirroring the existing dto.rs pattern.

cluster_ref()/cluster_arc() rely on expect() guarded only by an external ordering invariant (tidal-server)

tidal-server/src/cluster.rs:442 · CLEAN

• Issue: cluster_ref (cluster.rs:439-443) and cluster_arc (cluster.rs:451-457) call .expect("cluster accessed after shutdown") on self.cluster. The safety argument is that shutdown() (which takes the Option) only runs after axum stops accepting requests. That invariant is real today but lives in serve_cluster ordering, not in the type — CODING_GUIDELINES §7 states panics in a database corrupt state and bans expect outside tests/init.
• Fix: Have cluster_ref/cluster_arc return Result<_, ServerError> (e.g. ServerError::Cluster("server shutting down") → 503) and propagate via the handlers' existing ClusterAppError, so a post-shutdown access degrades to a clean 503 instead of unwinding the reactor thread.

Standalone data handlers have no integration coverage; only middleware/auth are tested (tidal-server)

tidal-server/tests/middleware.rs:1 · Completeness

• Issue: middleware.rs covers auth, body limit, and request-ID, and cluster_grpc.rs covers the cluster happy path, but no test drives the standalone POST /items → POST /signals → GET /feed → GET /search round trip through the HTTP surface. The engine-result→DTO mapping in dto.rs (feed_item/search_item, the signals-None-vs-[] logic) and the standalone state.rs region-rejection path are untested end-to-end.
• Fix: Add a tests/standalone.rs using the same oneshot in-process pattern as middleware.rs: write an item + embedding + signal, then assert /feed returns it ranked with the expected DTO shape (signals omitted when empty, present when populated), and assert a ?region= param yields 400. Also add a config.rs unit test for resolve_config_path's error branch (config dir set but file missing).

Text-index open failure is silently reported as an empty index with no exit-code signal (tidalctl)

tidalctl/src/main.rs:565 · Accuracy

• Issue: read_stats_from_dir returns None BOTH when the directory does not exist AND when the index exists but cannot be opened (corruption, partial write, lock). cmd_diagnostics collapses both to (0, 0) via .unwrap_or((0, 0)) and never bumps exit_code. A corrupt Tantivy index therefore reports tantivy_segments: 0, tantivy_indexed_docs: 0 — byte-identical to a healthy empty index — and diagnostics still exits 0 (or 2 only if the WAL also failed).
• Fix: Distinguish 'absent' from 'failed to open'. Have read_stats_from_dir (or a thin wrapper here) return a Result/enum that separates Missing from OpenError, or check dir.exists() first: if the dir exists but read_stats_from_dir returns None, set exit_code = 2 and surface the index as unavailable (null in JSON, 'not readable' in pretty) rather than 0. Apply identically to the creator_text_index at main.rs:570.

checkpoint_age_seconds emits a fabricated 0 in JSON when no checkpoint exists (tidalctl)

tidalctl/src/main.rs:573 · Accuracy

• Issue: checkpoint_age_secs is correctly None when wal.checkpoint_ts == 0 (no checkpoint ever taken). Line 573 collapses it to let checkpoint_age = checkpoint_age_secs.unwrap_or(0); and the JSON DiagnosticsOutput.checkpoint_age_seconds is a plain u64, so a database that has never checkpointed reports checkpoint_age_seconds: 0 — i.e. 'a checkpoint happened 0 seconds ago / just now'. The pretty path is correct (it branches on the Option and prints 'Last checkpoint: none'), so the two surfaces disagree.
• Fix: Type checkpoint_age_seconds as Option in DiagnosticsOutput and pass checkpoint_age_secs directly (serde renders None as null), matching the null-discipline already applied to the runtime-only fields and the pretty output's 'none' branch. Drop the unwrap_or(0) at line 573.

status exits 0 on a corrupt/unreadable WAL while diagnostics exits 2 (tidalctl)

tidalctl/src/main.rs:203 · Accuracy

• Issue: cmd_status maps a WAL gather failure to status: "error" in the JSON body but still returns Ok((rendered, 0)) — process exit code 0. diagnostics, given the identical condition, exits 2 (main.rs:516). So tidalctl status --path X && deploy treats a corrupt WAL as success.
• Fix: Return a non-zero exit code from cmd_status when the WAL state could not be gathered (e.g. Ok((rendered, 2)) in the Err(e) arm of the match at main.rs:203-207), mirroring diagnostics. Document the exit-code contract (0 = ok/empty, 2 = degraded/unreadable) in the //! header and usage() so all commands share one convention.

truncate_before can unlink the active segment the live writer is appending to (silent post-checkpoint write loss) (wal-core)

tidal/src/wal/writer.rs:369 · Accuracy

• Issue: WalCommand::TruncateBefore calls segment::delete_segments_before(&config.dir, before_seq) (segment.rs:280), which removes every segment with first_seq < before_seq — with NO protection for the segment the running writer holds open. After any write burst the active segment's first_seq sits well below the materialized checkpoint, so truncate_before(checkpoint_seq) will remove_file() the very inode the live SegmentWriter still has open and keeps appending to. This is the exact hazard the compaction module documents at length (compaction.rs:29-40, 67-103) and deliberately guards against with compact_wal_online; truncate_before reimplements the unguarded version. On Linux the writer keeps writing to the now-unlinked inode and every post-checkpoint, already-fsync'd, acknowledged write is silently lost on next open. It runs inside the writer thread (so the unlink doesn't race a concurrent write) but that does not prevent unlinking the open FD's inode. The public WalHandle::truncate_before API (mod.rs:411) exposes this to any caller. No production caller exists today, which is the only reason this is CRITICAL and not a shipped BLOCKER.
• Fix: Make truncate_before share the live-safe floor logic. Either route WalCommand::TruncateBefore through compact_wal_online (which clamps the deletion floor to min(before_seq, max_first_seq) so the active segment always survives), or — better, since the writer already knows its live segment — clamp before_seq to segment.first_seq() inside the writer thread before calling delete_segments_before. Add a regression test mirroring compaction::online_never_deletes_active_segment_even_when_fully_checkpointed but driving it through WalHandle::truncate_before with an active sub-checkpoint segment.

SegmentWriter::sync() doc claims fsync-level durability but only fdatasyncs the data file, not the directory (wal-core)

tidal/src/wal/segment.rs:208 · Accuracy

• Issue: sync() uses File::sync_data() (fdatasync) and the rustdoc says it 'maps to fdatasync on Linux and fsync on macOS ... the safe Rust equivalent'. fdatasync flushes file data + size-changing metadata but NOT necessarily other inode metadata, and crucially says nothing about the directory entry. The directory-entry durability is actually handled separately (open()/rotate() fsync the parent dir on creation; compaction fsyncs after unlink) — which is correct — but the sync() doc reads as if a single sync() makes a freshly appended batch fully durable. For an append to an already-existing segment, fdatasync of the data file is genuinely sufficient (no directory change), so behavior is correct; the risk is purely that the comment under-specifies the contract and could mislead a maintainer into dropping one of the directory fsyncs.
• Fix: Tighten the doc: state that sync_data() (fdatasync) makes the batch DATA durable for appends to an existing segment, and that directory-entry durability for newly created/rotated segments is handled by the explicit parent-directory sync_all() in open()/rotate(). Cross-reference those sites so the full durability story is discoverable from one place.

Segment byte-scan loop duplicated between reader and diagnostics, already drifted once (>= vs >) (wal-core)

tidal/src/wal/diagnostics.rs:132 · DRY

• Issue: diagnostics::diagnose_wal (diagnostics.rs:132-201) reimplements the same header-bounds-then-decode segment scan as reader::scan_segment_inner (reader.rs:165-224): identical magic check, identical payload_len read from bytes[offset+24..offset+28], identical batch_end bounds test, identical decode_batch + checked first_seq+i arithmetic. The two copies have ALREADY diverged once on the replay predicate (the >= vs > checkpoint bug the diagnostics tests at lines 360-393 now lock down), proving the copy-paste is fragile. Any future change to the on-disk batch framing must be made in both places or the diagnostic silently misreports recovery state.
• Fix: Extract the per-segment scan into one shared iterator/closure in reader (e.g. for_each_valid_batch(path, |header, events, offset|) returning the SegmentScan struct that already exists) and have diagnose_wal consume it, deriving counts from the same code path that recovery uses. The diagnostic only needs counts/offsets, so it can layer on top of scan_segment_inner's output without re-parsing bytes.

WalCommand dispatch match duplicated across three sites in run_writer (wal-core)

tidal/src/wal/writer.rs:362 · DRY

• Issue: The WalCommand handling (Append-push, TruncateBefore-delete-and-reply, Session*-handle, Shutdown-break) is spelled out three separate times: the blocking recv at lines 363-384, the recv_deadline drain at 388-414, and the final try_recv shutdown drain at 466-490. The three copies differ subtly and intentionally (continue vs break vs set-shutdown-flag), but the TruncateBefore body (delete_segments_before + reply) and the Session* delegation are byte-identical across all three. A future command variant must be threaded through all three arms, and the TruncateBefore active-segment fix above would have to be applied in three places.
• Fix: Factor the side-effecting commands (TruncateBefore, Session*) into a single handle_aux_command(cmd, &config, &mut session_journal) helper that returns an enum {Pushed(event,reply), Handled, Shutdown}. Each of the three loops then matches only on that enum, eliminating the duplicated bodies while preserving each loop's distinct continue/break semantics.

Session-journal corruption silently truncates recovery with zero observability (wal-format)

tidal/src/wal/session_journal.rs:78 · Completeness

• Issue: SessionJournal::recover calls decode_session_events (the infallible front door), not decode_session_events_with_diagnostics. When a mid-file record fails checksum verification, decode_session_events_with_diagnostics breaks at that point (session.rs:289-296), so EVERY session event written after the corrupted record is silently discarded. recover returns only the survivors with no error, no tracing::warn!, and no corruption count. The startup path at wal/mod.rs:217 (let session_events = SessionJournal::recover(...)?) therefore restores a silently-truncated set of active sessions and cannot tell a clean end-of-file from a corruption-induced stop. The diagnostics-bearing variant IS used — but only by the offline inspection command in diagnostics.rs:267, which is not on the recovery path. This is exactly the 3am scenario: a single flipped byte in sessions.log makes an unknown number of live sessions vanish on restart and nothing in the logs says why.
• Fix: Make SessionJournal::recover call decode_session_events_with_diagnostics, and when outcome.corruption_detected is true, emit tracing::warn! (or error!) reporting corruption_count, the journal path, and the count of recovered-vs-suspected-lost events. Return that corruption signal up to wal/mod.rs::open (e.g. via a RecoveryResult-style struct or a logged field) so startup recovery surfaces session-journal corruption the same way segment recovery surfaces batch corruption. Do not change the break-cleanly semantics — just stop discarding the corruption signal on the live path.

from_utf8_lossy silently mutates string fields on replay instead of flagging corruption (wal-format)

tidal/src/wal/format/session.rs:410 · Accuracy

• Issue: decode_start_record and decode_signal_record decode agent_id, policy_name, signal_name, and annotation with String::from_utf8_lossy (session.rs:410, 420, 456, 473). For a v2 frame the BLAKE3 checksum already guarantees the bytes are intact, so lossy decoding can never trigger there — but for a legacy v1 frame there is no checksum, so a single bit-flip inside a string field is silently rewritten to U+FFFD and accepted as a valid event rather than being detected as corruption. The replayed session then carries a mangled agent_id/policy_name that downstream code treats as authoritative.
• Fix: Use str::from_utf8(...).map(str::to_owned) and return None (stop/flag) on invalid UTF-8 instead of from_utf8_lossy, so a non-UTF-8 string field is treated as a malformed record (the existing decode_typed_record false path already counts it as corruption). The v2 path is unaffected (valid records still decode); the legacy v1 path gains corruption detection it currently lacks.

Unknown session record type is skipped, masking a class of mid-stream corruption (wal-format)

tidal/src/wal/format/session.rs:363 · Accuracy

• Issue: decode_typed_record's catch-all arm returns true for any unrecognized record_type, and the caller advances pos to record_end (session.rs:318), so the unknown record is skipped and replay continues. For a v2 frame this is safe (the checksum already passed, so the type byte is intentional and unknown only across versions). For a legacy v1 frame there is no checksum, so a corrupted type byte (e.g. a flipped 0x02->0x42) is indistinguishable from a forward-compatible unknown type: the record is silently dropped and decode keeps going as if nothing happened, with corruption_detected staying false.
• Fix: For legacy (un-checksummed) frames, treat an unknown record type as corruption (return false so it is counted and stops decode). For v2 frames, keep skip-and-continue forward-compat behavior — the checksum makes the type byte trustworthy. Threading a checksummed: bool flag into decode_typed_record cleanly separates the two policies.

Magic offsets in v2 frame layout and tests lack named constants (wal-format)

tidal/src/wal/format/session.rs:280 · Tech Debt

• Issue: The minimum v2 frame length is written as the literal 3 + SESSION_CHECKSUM_LEN (session.rs:280), the marker+version skip is a bare pos += 2 (session.rs:298), the Start fixed-prefix size is 24 (session.rs:396) and the Signal fixed-prefix is 28 (session.rs:435), and the corruption-test flip point is the hand-computed 4 + 3 (batch_tests-adjacent session tests.rs:833). These encode the framing geometry (marker(1)+version(1)+type(1), len-prefix(4)) as scattered integer literals rather than named constants, so a future field added to the frame header requires finding and updating each one by hand.
• Fix: Introduce named constants mirroring batch.rs, e.g. SESSION_FRAME_PREFIX_LEN (marker+version+type = 3), SESSION_LEN_PREFIX_LEN (4), and use them in the bound check, the pos+=2 skip, and the test flip offset. Optionally name the Start/Signal fixed-prefix sizes (24, 28) too.

---

SUGGESTIONS (51)

cohort

• tidal/src/cohort/types.rs:114-117 — [DRY] CohortRegistry::get allocates a fresh Arc on every probe, including the resolver's per-cohort lookups
• tidal/src/cohort/checkpoint.rs:7 and checkpoint.rs:45 — [Maintainability] Stale '983-byte fixed-format record' references in checkpoint docs after format grew to 1116 bytes
• tidal/src/cohort/checkpoint.rs:59 and checkpoint.rs:112 — [Completeness] No tracing instrumentation on cohort checkpoint/restore cold paths

db-core

• tidal/src/db/capabilities.rs:36 — [Accuracy] Capability TTL truncates Duration u128 nanos to u64, expiring very long TTLs early
• tidal/src/db/metadata.rs:24 — [CLEAN] serialize_metadata casts collection lengths to u32 without guarding oversized inputs

db-entities-ops

• tidal/src/db/users.rs:152 — [Maintainability] all_user_metadata omits the EMB-row skip that all_item_metadata has
• tidal/src/db/creators.rs:75 — [DRY] write_item_embedding and write_creator_embedding duplicate a ~40-line registry lock dance

db-infra

• tidal/src/db/backup.rs:251 — [Accuracy] Backup copies the data dir while the WAL writer thread may still flush enqueued events

entities

• tidal/src/entities/preference.rs:117 — [DRY] EMA blend+normalize body duplicated across update and update_with_custom_rate
• tidal/src/entities/user_state.rs:352 — [Tech Debt] in_progress_items scans the entire global completion map per call

governance

• tidal/src/governance/tombstone.rs:86 — [DRY] Four near-identical serialize-or-Internal-error to_bytes() blocks

load-testing

• tidal/src/testing/cluster.rs:1 — [Maintainability] SimulatedCluster file approaching the §9 size smell; cohesive sub-areas could split
• tidal/src/testing/cluster_transport.rs:92 — [Completeness] Partition re-delivery correctness depends on undocumented FIFO + idempotency invariants

query-executor

• tidal/src/query/executor/pipeline.rs:164-168 — [Accuracy] Load-degradation candidate truncation drops the top-ranked items for ID-ordered profiles (was CRITICAL)
• tidal/src/query/executor/candidate_gen.rs:59-86 — [Tech Debt] signal_ranked_candidates performs an O(N) full-ledger scan per query in the candidate-generation hot path
• tidal/src/query/executor/helpers.rs:173 — [Maintainability] Cohort rescore swallows unimplemented aggregations as 0.0 where the main path errors

query-retrieve-fusion

• tidal/src/query/retrieve/cursor.rs:31-50 — [Accuracy] Cursor conflates keyset (score, id) and offset encodings; RETRIEVE executor silently misreads a keyset cursor as an offset (was CRITICAL)
• tidal/src/query/retrieve/cursor.rs:72-94 — [Completeness] Cursor::decode does not reject non-finite (NaN/Inf) scores

query-search

• tidal/src/query/search/executor/pipeline.rs:622 — [Tech Debt] Magic 'content' default slot name duplicated across files
• tidal/src/query/search/executor/pipeline.rs:300 — [CLEAN] BM25 NaN scores ordered arbitrarily by partial_cmp().unwrap_or(Equal)
• tidal/src/query/suggest.rs:149 — [Accuracy] Trending-key cap has a check-then-insert race that can exceed MAX_TRENDING_KEYS

ranking

• tidal/src/ranking/diversity/selector.rs:41 — [Maintainability] DiversitySelector::select doc understates its own ordering guarantee
• tidal/src/ranking/executor/scoring.rs:175 — [Tech Debt] Social-graph trending path bypasses degradation-level window substitution
• tidal/src/ranking/executor/tests.rs:587 — [DRY] Profile/ledger test fixtures duplicated across five ranking test modules

replication-core

• tidal/src/replication/shipper.rs:198 — [CLEAN] count_events_in_segment recomputed per-peer per-poll for a value constant per segment
• tidal/src/replication/shipper.rs:380 — [DRY] Batch-walk decode loop duplicated three times with subtly different bodies

replication-crdt-tenant

• tidal/src/replication/tenant.rs:233 (dual_write_map) and tidal/src/replication/tenant.rs:236 (shard_pin_map); tidal/src/replication/migration.rs:13 (MigrationState) — [Accuracy] Migration routing state is not persisted — a crash reverts a finalized tenant to jump-hash routing (was CRITICAL)
• tidal/src/signals/ledger/core.rs:392 (consumer of CrdtSignalState) — [Accuracy] CrdtSignalState collapses multiple decay rates to a single lambda on reconcile

schema

• tidal/src/schema/error.rs:302 — [Tech Debt] DurabilityError documented as a dead 'Phase 1.2+ stub' but is live across four durability paths
• tidal/src/db/schema_fingerprint.rs:38 — [Accuracy] Signal windows/velocity/positive_engagement are excluded from the schema fingerprint, allowing silent semantic drift on reopen
• tidal/src/schema/validation/builders.rs:196 — [CLEAN] Unreachable !is_finite() defensive check on Duration-derived seconds

session

• tidal/src/db/sessions.rs:197 — [Accuracy] closed_sessions eviction can transiently exceed the cap under concurrent close
• tidal/src/session/snapshot.rs:112 — [CLEAN] Snapshot signaled_entities ordering is non-deterministic (DashMap/HashMap iteration)

signals

• tidal/src/signals/checkpoint/mod.rs:160 — [Tech Debt] Self-flagged TECH DEBT comment in restore() has no tracking issue
• tidal/src/signals/warm.rs:504 — [DRY] Identical circular-buffer reverse-sum idiom repeated three times

storage-engine-indexes

• tidal/src/storage/indexes/filter/result.rs:67 — [Accuracy] u64 EntityId silently truncated to u32 at the index boundary with no enforced rejection (was BLOCKER)
• tidal/src/storage/indexes/range.rs:277 — [Maintainability] Bitmap and range index serialized key prefixes use bare string literals ("BMP:", "RNG:") in multiple spots

storage-vector

• tidal/src/storage/vector/registry.rs:52 — [DRY] HNSW default parameters and their tuning rationale are duplicated across two structs
• tidal/src/storage/vector/brute/mod.rs:123 — [CLEAN] Brute-force search does a full O(n log n) sort when k is small

text

• tidal/src/text/index.rs:286 — [Tech Debt] delete_all() convenience commits with seq 0, clobbering any recorded commit payload
• tidal/src/text/index.rs:373 — [DRY] Segment/doc-count loop duplicated between index_stats() and read_stats_from_dir()
• tidal/src/text/index.rs:118 — [CLEAN] Stale #[allow(dead_code)] 'added in subsequent phases' annotations now that all phases shipped

tidal-net

• tidal-net/src/config.rs:54 — [Tech Debt] Default listen address uses port 59530, outside the documented 59520–59529 band
• tidal-net/src/config.rs:8 — [DRY] MAX_PAYLOAD_BYTES (64 MiB) defined independently in two crates, coupled only by a comment

tidal-server

• tidal-server/src/cluster.rs:1 — [Maintainability] cluster.rs and scatter_gather.rs exceed the 'one concern per file' size guidance
• tidal-server/src/scatter_gather.rs:532 — [Tech Debt] Client-controlled scatter-gather deadline_ms is unbounded

tidalctl

• tidalctl/src/main.rs:515 — [Tech Debt] cmd_diagnostics scans the WAL directory and re-reads the checkpoint twice
• tidalctl/src/main.rs:1 — [Maintainability] main.rs has grown to ~1026 lines spanning arg-parsing, five commands, and all formatters
• tidalctl/tests/cli.rs:861 — [CLEAN] Test .expect() string uses {stdout} as if it were a format macro

wal-core

• tidal/src/wal/reader.rs:55 — [CLEAN] Stale no-op statement and skip-logic comment block in reader::recover
• tidal/src/wal/diagnostics.rs:220 — [Tech Debt] Recovery-time estimate uses undocumented magic throughput constant

---

PRAISE (27) — what's done well

cohort

• Atomic check-and-insert via the DashMap entry API correctly closes the define TOCTOU race — define() uses match self.cohorts.entry(name) with Entry::Occupied -> duplicate error / Entry::Vacant -> insert, holding the bucket lock across check and insert, rather than a contains_key+insert pair. The accompanying comment names the exac

db-core

• Shutdown attempts every flush and returns the first durable error instead of exiting 0 on lost state — shutdown_inner accumulates the first error across the signal-ledger checkpoint, cohort/co-engagement/community checkpoints, storage flush, and WAL checkpoint marker (first_err.get_or_insert), continues attempting every remaining step to lea

db-entities-ops

• Single write-and-index path consolidation eliminates a real divergence class — write_item was previously a direct-to-storage path that bypassed metadata-size validation and in-memory index population; it now routes through write_item_with_metadata so there is exactly one indexing path (signals.rs:18-37), and the rustd

db-infra

• Notification-tracker TOCTOU fix is correctly serialized and concurrency-proptested — check_and_record serializes the read-decide-increment sequence under record_lock so two concurrent RETRIEVE responses for the same user cannot both observe headroom and both record, and it recovers a poisoned lock via into_inner() rather th
• Bounded HTTP request reader defends against slowloris and header floods with real socket tests — read_bounded_request caps the request line at 8 KiB via take(N), bounds each header line at 16 KiB, caps header-line count at 100, and sets a 5s read timeout — then the tests drive an actual loopback socket with an oversize request line (as

entities

• Regression bugs are locked down with precise, well-explained tests — creator_followers preserve full u64 (user_state.rs:546-578), out-of-order interaction pre-decay (interaction.rs:242-271), NaN-deterministic eviction/ranking and capacity==0 partition guard (co_engagement.rs:413-455), and remove_hide-preserv

governance

• Contributor checkpoint codec keys on (entity, signal_type), preventing silent cell collisions — encode_contributor_suffix deliberately includes entity and signal_type in the durable KEY (not just the value), with a focused doc comment and a dedicated regression test (checkpoint_restore_preserves_multi_entity_multi_signal_contributor,

load-testing

• Checked u16→u8 signal-type conversion prevents silent replicated-signal corruption — build() resolves each schema signal type id and uses u8::try_from(id.as_u16()), dropping out-of-range ids from the map (with a tracing::warn) instead of a blind as u8 truncation. The comment explicitly explains that a blind cast would a
• Panic-safe RAII in-flight accounting, verified by a panic-path test and a proptest — InFlightGuard decrements the LoadDetector counter on Drop with Relaxed ordering (correct for a statistical gauge), the level is sampled exactly once at enter() (matching the documented 'do not re-check mid-query' contract), and the guard_dr

query-executor

• Notification-cap check-then-record TOCTOU correctly closed with a single-lock atomic operation — The 'notification' profile routes through NotificationTracker::check_and_record (notification_tracker.rs:88-122), which evaluates both the per-creator and total caps AND increments the counters under one dedicated record_lock mutex, so two

query-retrieve-fusion

• RRF fusion sort is NaN-safe and the rank-score formula is correctly deduplicated — fuse() sorts with b.1.partial_cmp(&a.1).unwrap_or(std::cmp::Ordering::Equal), correctly avoiding the classic unwrap() panic on NaN in a float sort, and the 1/(k+rank) formula is extracted into rrf_term() so fuse() and route_results()'s

query-search

• Entity-id u32/u64 aliasing handled with correct, documented polarity — entity_as_u32 returns Option instead of a truncating id as u32, and the two consumers apply the correct opposite polarity: retain_excluded (pipeline.rs:566-571) keeps overflow ids because they cannot be members of a 32-bit suppression bit

ranking

• NaN is neutralized at construction and the sort uses a true total order — finite_score() maps NaN->0.0 before a candidate ever enters the vector, the sort uses f64::total_cmp (not partial_cmp(...).unwrap_or(Equal)), and infinities are deliberately preserved as 'sort first/last' sentinels because total_cmp orders

replication-core

• Community filter preserves the WAL last-seq boundary via first_seq re-stamping, with overflow guards and property tests — filter_segment_drop_local correctly re-stamps first_seq so that first_seq + kept_count - 1 equals the original batch's last seq, keeping the idempotency boundary and lag-gauge high-water-mark authoritative even when events are dropped (ship

replication-crdt-tenant

• HLC packs both clock components into one atomic to eliminate a real race — The Hlc packs (wall_ms<<16 | logical) into a single AtomicU64 and advances both via one compare_exchange, with the logical-overflow case carrying into the wall component (hlc.rs:226-231) rather than re-emitting a byte-identical timestamp. T
• Forward-decay and LWW logic each live in exactly one place and are delegated to — CrdtSignalState::on_signal/decay_score delegate all decay arithmetic to the canonical forward_decay_step kernel, and both LWW call sites (LWWRegister::merge and CrdtSignalState::merge) route through the shared lww_other_wins helper,

schema

• Timestamp::now degrades instead of panicking on a pre-epoch clock, with the safety argument written down and tested — Timestamp::now (timestamp.rs:29-41) matches on duration_since(UNIX_EPOCH) and, on the Err (clock-before-epoch) branch, logs a warning and saturates to Timestamp(0) instead of unwrapping. The doc (lines 19-27) explains exactly why this is sa

session

• Session record codecs are rigorously hardened against corruption and memory-amplification — The shared cursor helpers (read_u32/read_u64/read_utf8/read_len_prefixed_utf8) are fully bounds-checked and reject non-UTF-8 rather than masking it with from_utf8_lossy; capped_capacity (mod.rs:87) clamps every with_capacity against remaini

signals

• Forward-decay math correctly centralized into one exact, exhaustively-tested kernel — forward_decay_step is a single pure function that both temporal-ordering branches and every tier (HotSignalState, CrdtSignalState, and per the module doc the session/interaction/cohort tiers) delegate to. The module doc explicitly records t

storage-engine-indexes

• write_batch atomicity is correct and proven with mid-batch fault injection — write_batch routes every op through a single fjall OwnedWriteBatch committed under one seqno/checksum envelope, with a precise comment explaining that a naive insert/remove loop would frame each op as its own journal record and durably reco

storage-vector

• Deserialization is hardened against attacker/corruption-controlled count before any allocation — deserialize() reads the on-disk count, then computes bytes_per_vector with checked_mul/checked_add, caps count by what the actual body length can hold (max_count), and only then sizes with_capacity and runs the read loop — every step bounde

text

• Syncer survives transient commit failures instead of dying silently — the right call for an unbounded best-effort channel — run() treats a failed Tantivy commit as recoverable: it retains the buffered batch (Tantivy keeps un-committed docs), retries with exponential backoff capped at 30s, flips a shared health flag after 3 consecutive failures so the DB can surf

tidal-net

• Permanent-vs-transient error classification prevents an infinite silent replication stall — is_permanent() and the From for TransportError mapping correctly separate failures that re-shipping can never fix (bad TLS/CA, Unauthenticated/PermissionDenied, InvalidArgument/OutOfRange, Unimplemented) from transient o

tidal-server

• Scatter-gather correctly bounds latency, never silently drops shards, and reconciles replicated counts — dispatch_shards uses detached (not scoped) workers with a sized sync_channel so no worker blocks on send, drains with recv_timeout against the remaining budget, does a final non-blocking try_recv sweep to catch race-window stragglers, and r

tidalctl

• Refuses to fabricate values it cannot honestly observe offline; null + single-sourced reasons instead — Fields that cannot be derived from a lock-free offline scan (runtime-only session/degradation state, locked-keyspace collection/cohort counts, in-memory-only usearch size) are typed Option and always serialized as JSON null, never 0, w

wal-core

• Dedup is recorded only after a durable flush, preserving acknowledged-write retry correctness — partition_dedup only CHECKS membership (dedup.contains, plus an intra-batch HashSet) and never records; events are committed to the dedup window via dedup.record() exclusively after flush_batch returns Ok (writer.rs:428-433), and on flush e

wal-format

• Batch format checksums the full frame including governance and shard/region bytes, with proptest proof — encode_batch_with_shard computes BLAKE3 over header[0..32] || event_bytes (batch.rs:355-357), which deliberately includes the shard_id/region_id bytes (28-31) and the full 32-byte v3 event records (including the governance envelope at event

---

Verification transparency

False positives dropped (3)

• [CRITICAL db-infra] Text-index resync never clears the syncer health flag, causing a perpetual full re-rebuild every 10s (tidal/src/db/state_rebuild.rs:532)
  • Why dropped: The finding's load-bearing claim — that poll_text_index_health re-runs a full successful scan_prefix + delete_all + reindex every 10s indefinitely, "pinning a core and thrashing disk" — is unreachable because it misreads the writer-lock lifetime.

The syncer's run() (text/syncer.rs:142-143) acquires self.index.writer_guard()? ONCE and holds the MutexGuard for its entire lifetime. The deliberate single-writer design is documented at index.rs:54 ("holds the Tantivy writer lock for its

• [BLOCKER replication-crdt-tenant] Dual-write migration silently drops remote-shard writes (signal loss during migration) (tidal/src/db/replication_ops.rs:94)
  • Why dropped: The claimed BLOCKER ("target shard receives none of the migration-window signals → permanent silent data loss") is not reachable. Two independent facts refute it:

(1) signal_for_tenant has ZERO production callers. grep across tidal/src, tidal-server/src, tidal-net/src, tidalctl/src, and applications/ finds it defined only at replication_ops.rs:75 and invoked only from tests (tidal/tests/m8_uat.rs:483,520 and m8p5_multitenancy.rs). The real server cluster write path uses a separate mechanism — t

• [CRITICAL signals] reconcile_to_count load-then-store on all_time_count can drop a concurrent increment (tidal/src/signals/warm.rs:220)
  • Why dropped: The finding's central premise — "apply() runs over a live ledger with no global lock excluding concurrent record_signal on the same (entity, signal)" — is wrong. The race is not reachable because all callers serialize through the DashMap per-entry shard write lock.

BucketedCounter is stored by value inside EntitySignalEntry, which is stored by value in entries: DashMap<(EntityId, SignalTypeId), EntitySignalEntry> (core.rs:29). It is never wrapped in an Arc that could escape the map, so ev

Severity adjustments (16)

• CRITICAL → WARNING: from_config builds two divergent ReplicationState Arcs; control-plane lag gauge and stored field disagree (db-core)
• CRITICAL → WARNING: export_signals materializes the entire WAL into memory before applying the limit (db-infra)
• CRITICAL → SUGGESTION: Load-degradation candidate truncation drops the top-ranked items for ID-ordered profiles (query-executor)
• CRITICAL → SUGGESTION: Cursor conflates keyset (score, id) and offset encodings; RETRIEVE executor silently misreads a keyset cursor as an offset (query-retrieve-fusion)
• CRITICAL → WARNING: Mid-payload corruption commits earlier batches then halts with no durable recovery point (replication-core)
• CRITICAL → WARNING: CRDT reconciliation engine is never invoked in production — partitions never heal (replication-crdt-tenant)
• CRITICAL → SUGGESTION: Migration routing state is not persisted — a crash reverts a finalized tenant to jump-hash routing (replication-crdt-tenant)
• BLOCKER → SUGGESTION: u64 EntityId silently truncated to u32 at the index boundary with no enforced rejection (storage-engine-indexes)
• CRITICAL → WARNING: All fjall errors flattened to StorageError::Corruption, mislabeling transient and lock failures (storage-engine-indexes)
• CRITICAL → WARNING: flush_all performs four full-database fsyncs where one is required (storage-engine-indexes)
• CRITICAL → WARNING: Soft-delete leaves a durable key that rebuild_from_store resurrects on restart (storage-vector)
• CRITICAL → WARNING: truncate_before can unlink the active segment the live writer is appending to (silent post-checkpoint write loss) (wal-core)
• CRITICAL → WARNING: Session-journal corruption silently truncates recovery with zero observability (wal-format)
• CRITICAL → WARNING: Capability eviction can drop a live token, silently denying a valid capability until restart (governance)
• CRITICAL → WARNING: WAL replay collapses all session signals into one minute bucket, corrupting restored window counts (session)
• CRITICAL → WARNING: Rate limiter accepts zero/negative rate, permanently locking out agents and emitting u64::MAX retry (load-testing)

---

Per-subsystem assessments

db-core — The db-core subsystem (TidalDb handle, builder, open/close lifecycle, config, paths, storage routing, metadata codec, schema fingerprint, diagnostics) is in genuinely strong shape for a database that must survive a 3am incident. Crash recovery is taken seriously: schema-fingerprint verification blocks dimension/decay drift on reopen, embedding/co-engagement/cohort/community/governance/capability state is all rebuilt from durable storage with surfaced (not swallowed) failures, the shutdown path attempts every flush and returns the first durable error rather than exiting 0 on lost state, an advisory directory flock prevents dual-open corruption, and the metadata decoder is hardened against corrupt length prefixes with checked_add. Concurrency hazards that the comments themselves flag (CONCURRENCY-1/2/3, obs-REPL-1, SHUTDOWN-2) have been addressed with care, and the code is unusually well-documented about WHY each ordering matters. The main correctness concern I found is a latent divergent-Arc bug in from_config where the control-plane lag gauge and the replication_state field are built from two different ReplicationState Arcs (currently unreachable because no-schema mode cannot start replication, but a landmine if that ever changes). The dominant structural debt is file/struct size: mod.rs is 821 lines with a 60+-field TidalDb god-struct and two ~120-line constructors that duplicate roughly 50 field initializers verbatim — a real maintainability and DRY tax against CODING_GUIDELINES §9. Remaining items are minor (a u128→u64 truncation on capability TTL, a metadata-len cast). No data-loss or panic-on-reachable-path BLOCKERs.

db-entities-ops — This subsystem is the impl TidalDb write/read surface for items, users, creators, sessions, communities, collections, cohorts, relationships, signals, and queries. The code is mature and unusually well-commented: nearly every method documents its error contract, durability stance, and the reasoning behind ordering decisions. Single-entity write paths correctly fold all writes through one indexing path (the write_item→write_item_with_metadata consolidation is exemplary), validate metadata sizes, guard against non-finite weights, and use WAL-first durability in the underlying ledger. The dominant correctness risk is in the community-contribution path: community contributions are recorded into an in-memory ledger that is only checkpointed on graceful shutdown, while the WAL community events deliberately carry no contributor identity — so a hard crash silently loses every community contribution (and its provenance) since the last checkpoint, violating the project's "WAL is the source of truth, derived state rebuilds from the WAL" invariant. Secondary issues: cross-session preference aggregation hardcodes the EMB:content slot while every sibling path resolves the slot from schema (a silent personalization no-op for non-default slot names); u64→u32 entity-ID truncation across all bitmap indexes silently collides large IDs with only a warning; and the Tantivy commit-sequence checkpoint is stubbed to seq:0. None of the read paths swallow errors improperly, and lock discipline (WAL guard dropped before re-locking in the ledger) is sound.

db-infra — This is well-engineered infrastructure code: the concurrency reasoning is unusually careful (every Acquire/Release pairing is documented and correct, the notification-tracker TOCTOU fix is properly serialized and proptest-covered, the bounded HTTP reader defends against slowloris/flood, and the export JSON-Lines escaping bug was caught and regression-tested). The metrics, histogram, notification-tracker, WAL-bridge, and HTTP modules are essentially production-clean. The real risks cluster in two places. First, poll_text_index_health calls TextIndex::rebuild_from to recover a degraded item index, but that path never resets the syncer's shared healthy flag that is_healthy() reads — so once a syncer gives up, the checkpoint thread re-scans all of durable storage and runs delete_all+reindex every 10 seconds forever (a CPU/IO bug that masquerades as recovery). Second, export_signals materializes the ENTIRE WAL into memory via read_all_events before applying the limit, so a small-limit export on a large pre-compaction WAL can OOM the process — unbounded memory on a database that targets 10M+ entities. signal_for_tenant's dual-write path silently drops remote-shard writes while its own rustdoc claims it writes to both shards. None of these corrupt on-disk state, but the first two are reachable operational hazards and the third is a doc-vs-behavior trap. Fix those three and this subsystem is solid.

query-executor — The RETRIEVE executor is a well-structured, defensively-coded 6-stage pipeline with genuinely strong concurrency hygiene in the hot spots (notification-cap TOCTOU is correctly closed with a single-lock atomic check-and-record; cursor-offset overflow is guarded; poisoned-lock recovery is handled). Error propagation from scoring is loud rather than silent, and the deferred-post-filter sharing between RETRIEVE and SEARCH is a sound anti-drift design. The headline correctness gap is asymmetric handling of OR over deferred filters: the code rigorously rejects OR-of-signal-thresholds and negated deferred filters, but the same silent-AND-instead-of-OR trap applies verbatim to OR over Saved/Liked/InProgress/InCollection/NearLocation/SocialGraph, which are extracted and applied conjunctively with no guard — a well-formed query silently returns the wrong set. Secondary concerns: load-degradation truncation drops the highest-ID candidates before scoring, corrupting the "new" ranking; and a for_creator query silently falls back to a full-universe scan when creator_items is absent. Both latter paths are only reachable through partial construction or under load, but the OR-of-deferred bug is reachable from the public query API in the fully-wired DB.

query-retrieve-fusion — This subsystem (RETRIEVE AST/builder, pagination cursor, query errors, query stats, and RRF hybrid-fusion helpers) is well-constructed, idiomatic Rust with strong test coverage including property tests for the cursor roundtrip, RRF union invariant, and limit-range validation. Error handling is exemplary: no unwrap/expect in non-test code, the cursor decode path documents why its internal unwraps are unreachable, and the fusion sort is NaN-safe via partial_cmp().unwrap_or(Equal). The most material issue is a latent correctness footgun in the Cursor type: it is overloaded to encode BOTH a keyset (score, entity_id) pair AND a bare offset, but the RETRIEVE executor only ever interprets it as an offset — so a caller who constructs a keyset cursor (the type's headline use case, per its own docs) silently gets its entity_id reinterpreted as a page offset with no error. Secondary issues are a stale dead-code comment that actively misleads (validate() IS called by the executor), a 32-bit offset-truncation gap in from_offset/offset, and a known u32 creator-ID truncation that is documented but lets bad data through silently. None are data-loss or crash BLOCKERs; the cursor-semantics ambiguity is the one I would not want to debug at 3am.

query-search — The SEARCH pipeline is well-structured: the 8-stage execution in pipeline.rs is decomposed into focused helpers, the u32/u64 entity-id aliasing hazard around RoaringBitmap is handled deliberately and correctly with documented polarity (retain_excluded keeps overflow ids, retain_included drops them), cursor-offset overflow is guarded with saturating_add, and the team clearly fixed several real correctness regressions (relevance-anchored ranking, schema-resolved embedding slot, deferred post-filters and the SocialGraph arm shared verbatim with RETRIEVE). Tests are strong on the regression surface. The most material concerns are a latent correctness footgun where the deferred-filter path silently drops all candidates if the universe bitmap is empty/poisoned (currently masked only because production always maintains a populated universe), repeated cloning of the filter Vec via combined_filter() called five times per query in the hot path, and a handful of silent-degradation paths under lock poisoning that return empty result sets without surfacing a warning. No data-loss or crash-on-reachable-path BLOCKERs were found in this read-path subsystem.

replication-core — The replication-core subsystem is well-engineered at the concurrency and protocol-detail level: the shipper's per-peer high-water-mark with transient/permanent failure separation, the receiver's strict-by-seqno idempotency with overflow rejection, the lock-free monotonic max-into-atomic shared between ReplicationState and the lag gauge, and the WAL-position-preserving community filter are all correct, carefully reasoned, and backed by property tests. The code is unusually well-documented about WHY decisions were made. However, there is one BLOCKER-class durability gap that undermines the whole point of replication: a follower applies replicated events only to its in-memory ledger (apply_wal_event explicitly bypasses the WAL) and its ReplicationState high-water-mark is never persisted (to_checkpoint_bytes/from_checkpoint_bytes are dead in production; ReplicationState::new always starts every shard at 0). A follower that crashes therefore silently loses all replicated state since process start, and on restart re-applies from seqno 0 — contradicting the spec's promise of self-contained, replayable follower segments and leader-promotion-by-highest-replayed-seqno. A secondary CRITICAL concern: a corrupt mid-payload batch halts the receiver after earlier batches in the same payload already mutated the ledger and advanced applied_seqno, and because nothing is persisted there is no clean recovery point. There is also a real but lower-severity lag-gauge under-reporting bug in community-overlay mode and several maintainability/DRY items. Single-node correctness is solid; multi-node crash recovery is not yet trustworthy at 3am.

replication-crdt-tenant — The CRDT primitives (HLC, PN-Counter, LWW register, CRDT signal state) and the shard router are genuinely excellent: the math is correct and exact, the documentation explains why (forward-decay delegation to the canonical kernel, LWW-per-node vs additive merge for replay-idempotency, single-word atomic HLC packing to avoid the two-atomic race, HLC logical-overflow carry to preserve per-node uniqueness), and property tests assert the CRDT laws (commutative/associative/idempotent) over generated states rather than hand-picked examples. All 74 unit/property tests pass. The serious concerns are not in the algorithms but in integration completeness and durability of the migration path: (1) the entire CRDT reconciliation engine is wired only into tests — no production code ever snapshots the live ledger and calls ReconciliationEngine::plan, so post-partition healing does not actually happen; (2) the production dual-write path (signal_for_tenant) silently drops remote-shard writes during a migration with only a tracing::debug! and a "Task 5 not yet wired" comment, a real signal-loss window on a multi-node migration; (3) migration routing state (shard_pin_map/dual_write_map) and MigrationState are in-memory only and never persisted or recovered, so a crash mid-migration reverts a finalized tenant to jump-hash routing — potentially toward a source shard already GC'd. For a database where crash recovery and no-lost-writes are paramount, these are the findings that matter most. Code-quality, naming, and DRY are strong throughout.

storage-engine-indexes — The storage engine and index layer is well-constructed, correctness-focused code: the key encoding is order-preserving and collision-proof (NUL separator + non-zero tags, length-prefixed bitmap keys), the fjall write_batch correctly routes through a single OwnedWriteBatch for all-or-nothing durability and proves it with mid-batch fault injection, and the index layer is thoroughly property-tested (roundtrips, intersection/union/complement laws, inverted-bound safety). Three real problems stand out for a 3am operator. First, the entire index layer keys on u32 entity IDs while EntityId is u64; the conversion is an unguarded id as u32 cast (result.rs uses a debug-only assert), and the live ingest path that feeds these indexes only logs a warn! on overflow before inserting the truncated, colliding ID — a silent lost-write/wrong-result path. Second, every fjall error is flattened to StorageError::Corruption, so a transient I/O error, poisoned lock, or lock contention all surface as "data corruption" — exactly the wrong signal during an incident. Third, flush_all issues four full-database fsyncs where one suffices because each per-keyspace flush() already persists the shared database with SyncAll. None of these are formatting or style nits; they are durability/observability concerns inherent to a database. The remaining findings are efficiency and footgun-surface improvements.

storage-vector — The vector subsystem is well-structured and unusually disciplined for a v1: a clean VectorIndex trait with three implementations (USearch HNSW, brute-force, mock), a normalize-then-persist-then-index lifecycle that correctly treats the entity store as source of truth and the ANN index as rebuildable derived state, and genuinely hardened deserialization that bounds an attacker-controlled count before any allocation. Error handling is consistent (no unwrap/expect/panic on reachable production paths; lock poisoning maps to typed errors), and crash recovery is sound for the insert path because rebuild_from_store reconstructs the index from durable f32 vectors. The most material issues are (1) a recovery-correctness gap where soft-delete (delete_embedding(hard_delete=false)) removes a vector from the index but leaves the durable key, so rebuild_from_store resurrects it on restart — currently latent because delete_embedding has zero production call sites, but a 3am foot-gun the moment archive is wired; (2) a per-lookup String allocation on the slot hot path; (3) an unchecked multiply in index_stats that panics in debug builds at extreme scale; and (4) a non-atomic total_slots counter in UsearchIndex::insert whose docstring overclaims correctness. None are ship-blockers in the current wiring, but #1 must be closed before soft-delete ships.

wal-core — The WAL core is a strong, durability-first design: batch-oriented group commit with one BLAKE3 checksum + one fsync per batch, two-phase recovery validation, torn-tail truncation confined to the single-threaded recover() path, fsync-of-parent-directory after file creation/rename/unlink, and a carefully reasoned dedup model that records events as "seen" only after a durable flush so a failed-then-retried batch is never silently suppressed. The steady-state writer survives transient flush failures without tearing down the command channel, and both the steady loop and the shutdown drain funnel through one shared flush_batch routine so they cannot diverge on caller notification — these are exactly the 3am-incident properties the project cares about, and they are backed by real fault-injection tests. The most serious concern is a latent data-loss footgun: WalHandle::truncate_before deletes segments via the unprotected delete_segments_before, which can unlink the active segment the live writer is appending to (the precise hazard the compaction module documents at length and guards against with compact_wal_online). It has no production caller today, so it is CRITICAL rather than a shipped BLOCKER, but the public API invites the bug. Secondary issues: a documented panic-on-clock-skew in checkpoint()/flush_batch (reachable via NTP step / bad RTC), a misleading "fsync ensures durability" story in SegmentWriter::sync() that only fdatasyncs the data file (segment data is genuinely durable, but the directory-entry durability story lives elsewhere), and some structural debt around the three duplicated WalCommand dispatch sites and near-duplicate segment-scan loops in reader vs diagnostics.

wal-format — The wal-format subsystem is well-constructed, correctness-conscious code: both wire formats (the fixed-size BLAKE3-checksummed signal batch and the variable-length session journal) are carefully versioned, every decode bound is checked before indexing, and torn-tail-vs-corruption is distinguished cleanly. The signal batch format (batch.rs) is excellent — exhaustive bounds checking, full-frame checksum coverage including governance and shard/region bytes, and strong property-test coverage. The main real gap is observability on the live recovery path: SessionJournal::recover uses the infallible decode_session_events, so a mid-file checksum mismatch silently truncates the journal (dropping every later session event) with no warning, error, or metric — the diagnostics-bearing variant exists but is wired only to an offline inspection command, not to startup recovery. Secondary concerns: silent UTF-8 mutation via from_utf8_lossy on replay, an unknown-record-type path that swallows a class of corruption, and a few magic offsets. None are data-corruption-on-write bugs; the durability path itself (per-record fsync, checksum-on-encode) is sound.

ranking — The ranking subsystem is well-structured, correctness-conscious, and unusually well-documented for a scoring engine: profiles are data (registered/validated/versioned, not code), the executor is stateless over an immutable signal-ledger reference, pure formulas are isolated and unit-tested, and the diversity selector carries explicit invariants (INV-RANK-5/6) backed by property tests. Error handling follows the project rule — no unwrap/expect/panic/unsafe/await in production code; misconfigured profiles fail loud at registration, and the one genuine invariant-violation path (Ratio/RelativeVelocity reaching read_agg) returns an Internal error rather than silently scoring 0.0. The NaN-handling story is solid (finite_score at construction + total_cmp sort + clamp in normalize), and negative signals (penalties/excludes) are first-class as the spec demands. The main real defects are: (1) a score/rank inconsistency for the Shortest sort where a missing-duration item is correctly ranked last but normalize() rewrites its reported score to the maximum 1.0; (2) the now: Timestamp argument is threaded through the entire scoring path but is effectively dead — every time-sensitive read uses Timestamp::now() internally, making scores non-reproducible and the parameter misleading. Both are bounded in blast radius (one sort mode; explainability/testability) and neither corrupts result ordering. The remainder are documentation-accuracy and DRY-in-tests nits. No BLOCKER or CRITICAL data-integrity, concurrency, or recovery issues found in this subsystem.

signals — The signals subsystem is strong, careful code: the forward-decay kernel is centralized into one exact, well-tested pure function (decay.rs), the hot tier is genuinely lock-free and cache-line-aligned with documented memory-ordering rationale, the checkpoint format is versioned with BLAKE3 integrity and clean V1→V2 backward compatibility, and property/crash tests cover the core invariants (decay monotonicity, out-of-order commutativity, roundtrip serialization, CAS no-lost-update). The dominant real problem is read-time staleness in the warm tier: BucketedCounter::windowed_count never rotates, so windowed/velocity counts for an entity that stops receiving signals are frozen at their last-write value and never age out — and this exact value drives the "Trending" retrieval scope and velocity ranking, which is the product's headline surface. A secondary concurrency concern is reconcile_to_count's load-then-store on all_time_count, which can drop a concurrent increment. The remaining findings are quality/maintainability items. No panics or unwraps escape test code; durability ordering (WAL-first, restore-stores-timestamp-last) is correct.

entities — The entities subsystem is the personalization data model: user/creator entity codecs, the relationship-edge key encoding, and a family of in-memory indexes (UserStateIndex, PreferenceVectors, InteractionLedger, HardNegIndex, CoEngagementIndex, UserSignalIndex, CollectionIndex). The code in isolation is clean, well-documented, lock-free on the hot path per the guidelines, and unusually well-tested at the unit/property level — several past correctness bugs (u32 follower truncation, NaN-poisoned eviction ordering, out-of-order interaction decay) carry explicit regression tests, which is excellent. The serious problems are not in these files' internal logic but in their integration contract with the rest of the engine, and they are durability/recovery problems — exactly what matters most for a database. Three of these indexes (HardNegIndex, InteractionLedger, and the completion/saved/liked maps in UserStateIndex) are documented as "rebuilt from signals on startup" but in practice have no working recovery path: the signal write path populates them in memory only, the WAL replay on open does not re-run the per-user side-effects, and no checkpoint/restore exists for them. The net effect is silent data loss on every restart for hard-negatives-from-skip/dislike and interaction strength, and three entirely write-only-from-tests state maps that disable the InProgress filter and DateSaved sort in production. These contradict CODING_GUIDELINES §2 (WAL is the source of truth, derived state is rebuildable) and §6 (a hide/rejection creates a permanent hard-negative), so I assess this as REQUEST_CHANGES despite the high local code quality.

governance — The governance module (M9/M10: signal scope, provenance, share policy, community membership, community ledger, purge tombstones, capability tokens, and governance policy) is unusually high-quality pure-type code: well-documented, correctly concurrency-aware (atomic gates with Acquire/Release at the right boundaries), and thoroughly unit-tested including crash-replay and idempotency cases. The hot-path gates (stop-forward, revocation) are genuinely O(1) atomic loads as advertised. The most serious problems are not inside these files but in the durability contract they participate in, which I verified by tracing the wiring in db/: the CommunityLedger is the SOLE durable copy of per-contributor community state, yet it is checkpointed ONLY on graceful shutdown, and one of its two purge entry points (purge_writer / CommunityAgent) writes no tombstone — so a crash after an agent purge silently resurrects the purged contributions, which is a privacy/data-resurrection bug the tombstone machinery exists specifically to prevent. A secondary durability gap is that community contributions recorded since the last (shutdown-only) checkpoint are unrecoverable on crash because the WAL event carries no contributor identity. Within the assigned files the code is clean; the findings center on the recovery/durability seams these types define.

schema — The schema subsystem is the type/validation foundation of tidalDB: newtype wrappers (EntityId, Timestamp, Score) with order-preserving big-endian encoding, the signal-type/decay/window declarations, the error taxonomy, and the SchemaBuilder validation pipeline. Overall quality is high and clearly written by someone thinking about durability — clock-anomaly handling in Timestamp::now saturates instead of panicking, Score rejects NaN/inf at construction so Ord is sound, the order-preserving encodings are proptest-verified, and the build() validator surfaces every Tantivy/vector-registry footgun as a typed error at define-time rather than as a panic deep in the index. No BLOCKER or CRITICAL issues: there are no reachable panics in non-test code, no swallowed errors, and no concurrency hazards in this (immutable, post-build) layer. The defects that exist are correctness-adjacent and maintainability concerns: (1) the EntityKind->byte mapping used by the schema fingerprint is duplicated in three places coupled only by a comment, (2) the agent-policy validation branch in build() has zero test coverage while every sibling branch is exhaustively tested, (3) DurabilityError is documented as a dead Phase-1.2 stub but is in fact live across four durability paths, and (4) signal windows/velocity/positive_engagement are intentionally excluded from the schema fingerprint, which is safe for on-disk integrity but allows silent semantic drift on reopen. Fixing the DRY fingerprint coupling and adding the missing policy tests are the highest-value follow-ups.

session — The session subsystem (tidal/src/session/) is well-structured, well-documented, and largely correct. Module boundaries follow CODING_GUIDELINES §9 (one concern per file), the decay math correctly delegates to the canonical forward_decay_step kernel (§3), the binary codecs are rigorously bounds-checked against corruption and memory-amplification (a genuine strength), and the audit/closed-session/signaled-entity caps are all enforced. The most material issue is in the WAL replay path (db/session_restore.rs, just outside the assigned files but the session layer's recovery integration): replayed signals reset each BucketedCounter's start time to restore-time wall-clock and then feed past event timestamps, so maybe_rotate never fires and every replayed event collapses into one minute bucket — restored window_1h counts are wrong, diverging from the pre-crash state and violating the §8 invariant that "WAL replay produces identical state." Secondary concerns: f64 session weights are narrowed to f32 in the WAL session-signal codec (lossy on replay), session-journal appends are fire-and-forget (a documented best-effort design, but the doc comment claims a durability guarantee the caller does not actually wait for), and the closed-session eviction has a benign cap race. No data-corruption-on-disk or panic-on-reachable-path bugs were found in the assigned files themselves; the serde layer in particular is defensive and correct.

text — The text subsystem (Tantivy full-text indexing) is well-built at the unit level: the writer's delete-then-add update atomicity, the two-phase commit-with-payload, the resilient syncer (which survives transient commit failures, retries with backoff, and surfaces a health flag), and the AllScoresCollector are all correct and thoroughly unit-tested. The merge policy and config are well-documented with rationale. However, there is one BLOCKER-class durability gap that spans the module boundary: the entire crash-recovery mechanism that this subsystem was designed around — storing the WAL sequence in the Tantivy commit payload (writer.rs commit/last_committed_seq) — is dead. The production write path hardcodes seq=0 (db/items.rs:161), and last_committed_seq() is never called outside tests, so on restart there is no reconciliation between the durable entity store and the Tantivy index. Items durably persisted but not yet asynchronously committed to Tantivy before a crash are silently absent from text search forever (the syncer was "healthy" when the process died, so the unhealthy-triggered rebuild never fires, and unlike the vector index there is no unconditional rebuild-from-store at open). A secondary correctness/maintainability hazard: AllScoresCollector carries an entity_id_field that every caller dutifully sets but for_segment ignores, looking up the fast field by a hardcoded "entity_id" string instead. Accuracy and crash-recovery are where this subsystem needs work; the in-process code quality is otherwise strong.

cohort — The cohort subsystem (named user segments with per-cohort decaying signal state) is well-structured, cleanly separated one-concern-per-file, and shows genuine 3am-grade care in its checkpoint corruption policy and concurrency primitives (atomic entry-API define, parking_lot-backed DashMap with no poison risk, bounded LRU resolver cache). Tests are thorough for the in-isolation behavior of each unit. However, two real correctness gaps undermine the careful work: (1) the resolver membership cache is never invalidated when a cohort is defined at runtime, so users resolved before a new cohort exists silently miss attribution to it until eviction or a metadata change; and (2) the checkpoint restore's deliberately-fail-loud corruption contract is defeated by its sole caller, which catches the fatal error, logs a warn, and silently continues with empty cohort aggregates — the exact silent-degradation the contract's doc comment was written to prevent. Several lower-severity items (impossible-but-swallowed serialization error converted to silent data loss, per-signal String allocations on a fan-out path, stale 983-byte doc references, redundant Arc probe allocations) round out the findings. No data-corruption or panic-on-reachable-path blockers were found.

load-testing — Two distinct concerns share this assignment. tidal/src/load/ is PRODUCTION code (gated by nothing; pub mod load in lib.rs) implementing graceful-degradation (LoadDetector), WAL backpressure config, and a per-session token-bucket rate limiter. tidal/src/testing/ is correctly cfg-gated test infrastructure (#[cfg(any(test, feature = "test-utils"))]) for crash injection, network-partition faults, and a real-fabric SimulatedCluster. The code is generally high quality: the RAII InFlightGuard is panic-safe and proptest-covered, the CrashInjector memory orderings are deliberate and documented, the SimulatedCluster Drop logic correctly splits send/recv channel endpoints to avoid join-deadlock, and the u16→u8 signal-type conversion uses checked truncation to avoid silent wire corruption — all genuinely production-grade. The most material issues are in the production rate limiter: it accepts degenerate configs (signals_per_second == 0.0 or negative) with no validation, which permanently locks out an agent and produces a u64::MAX retry_after_ms; and it allocates a String key plus touches the DashMap on every session_signal() call even in the default unlimited mode, putting an avoidable allocation on the write path. The testing harness has a latent gap: write_signal applies to the leader before shipping batches, but if encode_batch ever failed it would panic (.expect) after the leader write already committed, and the partition-recovery re-delivery relies on FIFO+idempotency invariants that hold today but are undocumented as load-bearing.

tidal-net — tidal-net is a well-built, correctness-focused gRPC implementation of the core crate's Transport trait for WAL segment shipping. The crate boundary is clean (no network types leak into the core engine), the error taxonomy is unusually thoughtful — the permanent-vs-transient classification and its mapping into TransportError::Permanent is exactly the kind of detail that prevents a silent replication stall at 3am — and the codec-size, connect-timeout, keep-alive, and crypto-provider hardening all show real production scar tissue. Security boundaries (untrusted-client-cert rejection, absent-cert rejection, plaintext-rejected-when-not-insecure) are tested as negative paths, not just happy paths. The main correctness concern is that sustained follower backpressure (accepted=false) is fed into the circuit breaker as a failure, so a healthy-but-busy follower will trip its breaker and stall replication for the full reset window — converting normal flow-control into a 30s outage. Secondary concerns: the circuit breaker's documented "one probe" half-open invariant is not actually enforced; the Transport trait's "non-blocking" contract is now stale relative to this blocking-with-timeout implementation; the segment-receiver thread parks forever in recv_segment and is only reclaimed at process exit (a leaked thread on clean shutdown); and heartbeat/stream_segments are stubs (loudly so, and ticketed). No data-loss, lost-write, or panic-on-reachable-path bugs were found; the integration and security tests are genuinely end-to-end over real localhost gRPC and mTLS.

tidal-server — tidal-server is a well-engineered HTTP wrapper over the embedded tidaldb engine, exposing standalone and (explicitly-gated experimental) cluster modes plus scatter-gather fan-out. The code is unusually disciplined for a database server: it forbids unsafe, denies unwrap_used, honors the "Result everywhere" rule, has constant-time bearer auth, body limits, timeout/concurrency layers, request-ID propagation, graceful shutdown with deterministic checkpoint+WAL-fsync on drop, and an honest experimental-cluster opt-in gate with a loud WARN. The scatter-gather coordinator is the strongest piece — its deadline-bounded detached-worker design, replica dedup, candidate-count reconciliation, and coordinator-level diversity re-enforcement are correct and thoroughly unit-tested, with careful documentation of why threads are detached not scoped. The most material gaps are: (1) the standalone router runs blocking, CPU-bound retrieve/search directly on the async reactor while the cluster path correctly offloads identical work — an asymmetry that can starve reactor threads under load; (2) the cluster write/heal path spawns an unbounded fresh OS thread per request; and (3) coordinator-level max_per_creator resolves creators only from the leader replica, which silently under-enforces in a true entity-sharded topology and contradicts its own doc comment. None are data-loss/corruption bugs, and durability is delegated to the engine's WAL. Cluster mode is correctly fenced as non-HA. With the reactor-offload and thread-bounding fixes, this is production-grade for the standalone surface and a faithful (single-process) cluster simulation.

tidalctl — tidalctl is a lock-free, read-only inspector over a tidalDB data directory exposing five commands (status, paths, recover, diagnostics, scope-stats). It builds clean, all 29 integration tests pass, and main.rs itself is clippy-clean under the crate's strict deny posture. The code is unusually disciplined for a CLI: it forbids unsafe, propagates errors via Result everywhere (no unwrap/expect/panic in non-test code), routes all JSON through a single serde_json path (closing the documented hand-rolled-escaping control-char hole, which a dedicated test guards), and — most impressively — refuses to fabricate "0" for fields it cannot honestly derive offline, emitting JSON null plus a single-sourced self-describing reason map instead. The accuracy of its derived metrics (real event counts vs sequence proxies, true uncheckpointed replay-lag bytes, torn-tail completeness indicator) is well-reasoned and directly tested. The findings are about edges, not the core: a handful of error/unavailable conditions collapse to a fabricated 0 in ways that contradict the command's own honesty discipline (text-index open failure → 0 with no exit-code bump; no-checkpoint → checkpoint_age_seconds: 0 in JSON; status exits 0 on a corrupt WAL while diagnostics exits 2), the diagnostics path scans the WAL directory and re-reads the checkpoint twice, and main.rs has grown to ~1026 lines spanning many concerns. None are data-loss or crash bugs — this is a read-only tool — so nothing here is a blocker; they are correctness-of-reporting and organization concerns appropriate to a 3am operator who must trust the numbers.