diff --git a/CHANGELOG.md b/CHANGELOG.md index 6820bb0..5f9614f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,17 +27,55 @@ Recency did not merely participate in the ranking, it annihilated every boost, a now negated age in hours (newer = closer to 0 = higher), which preserves "newest first" on a scale comparable to a boost sum. -Ordering changes wherever entity IDs were not monotonic with creation time. Three -built-in profiles set `Sort::New` — `new`, `recent_uploads` and the `brief` extra — -and any custom profile using it is affected. Items with no `created_at` available -score 0.0 (treated as brand new) and tie, rather than ordering by an ID that -carries no recency meaning. +Ordering changes wherever entity IDs were not monotonic with creation time. The +age-derived sorts are set by **seven** built-in profiles, four `Hot` and three +`New`, plus any custom profile using either: + +| Profile | Sort | Defined at | Diversity block | Loaded item metadata *before* this change? | +|---------|------|-----------|-----------------|--------------------------------------------| +| `hot` | `Hot { gravity: 1.8 }` | `builtins.rs:188` | yes (`:197-200`) | yes | +| `for_you` | `Hot { gravity: 1.5 }` | `builtins.rs:312` | yes (`:333-336`) | yes | +| `related` | `Hot { gravity: 1.2 }` | `builtins.rs:381` | yes (`:396-399`) | yes | +| `brief` | `Hot { gravity: 1.5 }` | `builtins/extras.rs:39` | yes (`:74-77`) | yes | +| `following` | `New` | `builtins.rs:349` | yes (`:356-359`) | yes | +| `new` | `New` | `builtins.rs:206` | none | no | +| `chronological` | `New` | `builtins/extras.rs:91` | none | no | + +The last column matters for rollout. The pre-change metadata pre-load was gated on +`session_context.is_some() || needs_metadata_for_creator_grouping` +(`query::executor::mod.rs:472-478`), and a diversity block satisfies the second +arm. So the five profiles carrying one were **already** loading the +`EntityId -> metadata` map and start scoring by real `created_at` the instant the +image rolls, with no configuration change. `new` and `chronological` carry no +diversity block and no session; they are newly enabled by the widened +`needs_metadata_for_sort` arm described under *Fixed* below. + +Items with no usable `created_at` are handled by cause, not lumped together. When +the metadata map was never loaded at all, every candidate scores `0.0` and the set +ties. When the map *is* loaded and this specific entity has no parseable +`created_at`, it scores `f64::NEG_INFINITY` and sorts last — an undated row is not +silently promoted to "brand new" alongside genuinely fresh ones. + +**Wire consequence of a tie, stated plainly:** for a corpus where *no* row carries +`created_at`, every candidate holds the same `Sort::New` score, so the result order +falls through to `finalize`'s tie-break 2, **ascending entity id** +(`ranking/executor/mod.rs:942-943`). That is the exact reverse of the pre-commit +output, which ranked on `entity_id as f64` descending. A caller reading an undated +corpus sees its page flip end to end. This is a correction, not a regression: +`docs/specs/09-ranking-scoring.md:1214` specifies `Sort::New` as `created_at DESC`, +"Pure chronological, no scoring", and `docs/specs/11-schema.md:1409` specifies the +`following` profile's sort semantics as "`created_at DESC` (pure chronological)". +Neither spec licenses entity id as a recency proxy; the implementation now conforms +to both. Under `ReducedCandidates` load the candidate cap now selects survivors via the `created_at` index read newest-first, not by descending entity ID. The old key was only correct while `Sort::New` itself ranked by ID; left alone it would have discarded the genuinely newest items *before* scoring, leaving the ranking wrong -only when degraded. `RangeIndex::top_n_descending` is new and public for this. +only when degraded. Candidates the index oversample does not cover fall back to +**descending entity id** rather than to scan order, so the degraded path keeps a +deterministic, recency-shaped tail. `RangeIndex::top_n_descending` is new and +public for this. ### Fixed @@ -47,8 +85,9 @@ only when degraded. `RangeIndex::top_n_descending` is new and public for this. `log10(max(views,1)) / (age_hours + 2)^gravity` was constant across the candidate set and `Sort::Hot` reduced **exactly** to `log10(max(views, 1))` — a view-count ranking wearing a recency sort's name. Anyone tuning `gravity` was tuning a no-op. -All four built-in `Hot` profiles were affected: `hot`, `for_you`, `following`, and -the `brief` extra. +All four built-in `Hot` profiles were affected: `hot` (gravity 1.8), `for_you` +(1.5), `related` (1.2) and the `brief` extra (1.5). `following` sets `Sort::New`, +not `Sort::Hot`; see the profile table under *Breaking* for the full set. The in-code comment justified this by saying a per-entity `created_at` lookup would need an `EntityId -> created_at_ns` reverse map that "is not built". That was stale: @@ -63,6 +102,13 @@ Measured on a real server, 10 items with equal view counts and ages spanning normalizer's neutral midpoint) and the feed returned oldest-first forever; after, scores run `1.0 → 0.0` strictly descending, newest first. +Two distinct "no age available" cases are kept distinct here too. If the metadata +map was never loaded, `score_hot` falls back to the documented +`DEFAULT_HOT_AGE_HOURS` constant, so the whole set shares one divisor and ranks by +view count as before. If the map *is* loaded and this entity has no parseable +`created_at`, the score returns Hot's floor of `0.0` instead — an undated row is +not handed the freshest possible divisor and floated to the top of a dated corpus. + **LIMIT, deliberately not papered over:** `hot`'s numerator is `log10(max(views, 1))`, which is exactly `0.0` for 0 **or** 1 views, so the age divisor cannot differentiate a zero-signal corpus — every candidate still scores @@ -73,6 +119,33 @@ a separate decision and is not folded in here. `sort_hot_zero_view_corpus_still_ ties_regardless_of_age` pins the current behaviour so the limit cannot be rediscovered by accident. +**A wrong-unit `created_at` is now visible instead of silent** + +`Items::metadata_with_created_at` defaults `created_at` only when the key is +absent or unparseable as `u64`. A **seconds**-unit value parses fine and is stored +verbatim, and `read_age_hours` divides by nanoseconds-per-hour — so +`"1700000000"` reads as **496,731 hours (~56 years)** old. Until this release both +age sorts ignored the value and the mistake was inert. Now it is live and severe: +`Sort::New` scores `-496_731` against boost sums in single digits, re-entering +through *data* the exact "recency annihilates every boost" defect the entity-id fix +above removed, and `Sort::Hot` at 10 views and gravity 1.8 falls from `2.838e-3` +(a correctly-dated 24-hour-old item) to `5.584e-11`, burying the item permanently. +The repo's own `tidal-server` fixtures wrote seconds, which is the evidence that +this is the natural caller mistake rather than a hypothetical; they now write +nanoseconds. + +A write whose `created_at` parses but is below `6e17` ns (1989-01-05) now logs a +`WARN` naming the entity id, the offending value, and the resulting age in hours +and years. The threshold works because the plausible unit ranges cannot overlap: a +nanosecond timestamp for any date after 1990 is at least `6.31e17`, while seconds, +millis and micros values stay below `~1e16` even for dates centuries out. + +The value is **not** rewritten and the write is **not** rejected. Guessing a unit +would corrupt a number the `created_at` range index already reads as nanoseconds, +and rejecting would break an API that accepts it today. `ItemRequest::metadata` now +documents the nanosecond unit, what it drives, and this failure mode, and the +description propagates into the OpenAPI schema. + **Every metadata-based sort was dead on the SEARCH path** `alphabetical_asc`, `alphabetical_desc`, `shortest` and `longest` silently tied on diff --git a/k8s/discover/schema.yaml b/k8s/discover/schema.yaml index 5a9812c..ba2e429 100644 --- a/k8s/discover/schema.yaml +++ b/k8s/discover/schema.yaml @@ -88,31 +88,63 @@ profiles: # caller requests limit=200 rather than a page-size limit: a 2,000-wide pool # over a ~1,821-post corpus. See the endpoint's DISCOVER_TIDAL_FETCH const. candidate_strategy: scan - # NO `sort:` — VERIFIED DELIBERATE. Neither built-in sort is usable here, and - # the score is `sort_base + boost_sum` then min-max normalized - # (tidal/src/ranking/executor/{mod,helpers}.rs), so a sort's magnitude decides - # whether signals matter at all: + # NO `sort:` — DECISION UNCHANGED, RATIONALE REWRITTEN. The score is + # `sort_base + boost_sum`, then min-max normalized + # (tidal/src/ranking/executor/{mod,helpers}.rs), so what decides whether + # signals matter is the SPREAD of sort_base across the candidate set + # relative to the spread of boost_sum (single digits here). # - # hot: NOT age-aware. `score_hot` treats EVERY candidate as exactly 24 - # hours old and ranks by `view` COUNT - # (tidal/src/ranking/executor/scoring.rs:263-271, which says so in a - # comment). It reads no created_at, so it adds a term the `view` boost - # below already covers, and contributes no recency whatsoever. - # new: base score is `entity_id as f64` - # (tidal/src/ranking/executor/scoring.rs:122-133), i.e. ~1_821 on this - # corpus, against a boost_sum of single digits. Recency would dominate - # by three orders of magnitude and the signals would be decorative. + # The two bullets below previously argued from the PRE-CHANGE formulas and + # cited `tidal/src/query/executor/scoring.rs`, a path that does not exist + # (the real file is `tidal/src/ranking/executor/scoring.rs`). Both sorts are + # now age-aware; here is what they actually do: + # + # hot: IS age-aware now. `score_hot` reads this item's real `created_at` + # out of the item-metadata map and scores + # `log10(max(views,1)) / (age_hours + 2)^gravity` + # (`score_hot`, tidal/src/ranking/executor/scoring.rs). The old claim + # that it "treats EVERY candidate as exactly 24 hours old" and + # "contributes no recency whatsoever" is VOID. + # What still holds: the numerator is exactly 0.0 at 0 OR 1 views, so + # on the zero-signal rows that make up most of this corpus the age + # divisor has nothing to divide — every such candidate scores 0.0 and + # ties. `hot` would add recency only from an item's second view on. + # new: base is negated age in HOURS now, not `entity_id as f64`. The old + # "~1_821 on this corpus" figure is VOID. + # It is NOT automatically boost-comparable, though: the spread of + # `-age_hours` equals the corpus's age span expressed in hours, so a + # corpus accumulated over a year spans ~8_766 — the same order of + # magnitude as the entity-id spread it replaced, and still ~3 orders + # above a single-digit boost_sum. The domination concern SURVIVES the + # change; only its derivation moved from item COUNT to corpus AGE + # SPAN. On a corpus spanning under ~a day it would genuinely be + # boost-comparable, which is exactly why this needs measuring rather + # than asserting. # # With no sort the ranking is boost_sum alone — measured: 5 likes outranks 3 - # outranks 1, with the remainder in scan order. That IS the intent ("ranks on - # signals alone"); do not add a sort back without re-measuring both effects. + # outranks 1, with the remainder in scan order. That IS the intent ("ranks + # on signals alone"). # - # Consequence, accepted and handled UPSTREAM: on a zero-signal corpus every - # score ties at the normalizer's neutral 0.5 and the order is scan order, - # i.e. OLDEST-FIRST (the sweep allocates ids oldest-first). That is why the - # endpoint routes a viewer below the impression threshold to its `ranked` - # arm instead of reading this profile unpersonalized — an arbitrary order is - # strictly worse than the recency ordering that already exists. + # STATUS: PENDING RE-MEASUREMENT. Keeping `NO sort:` is the safe hold, but + # it is no longer JUSTIFIED by the reasoning above — the reasoning it rested + # on was about the old formulas. Before adding a sort back, measure on a + # production snapshot: + # 1. corpus age span in hours (max created_at - min created_at). That is + # the `new` sort_base spread. + # 2. observed boost_sum spread (p1..p99) over the same candidate pool. + # 3. fraction of the pool with >= 2 `view` signals. That is the fraction + # `hot` can differentiate at all. + # Add `new` only if (1) is within roughly one order of magnitude of (2); + # add `hot` only if (3) is a clear majority. Otherwise the sort still either + # annihilates the signals or ties the set. + # + # Consequence, accepted and handled UPSTREAM — UNCHANGED by the age-aware + # work, because with no sort no age term is read at all: on a zero-signal + # corpus every score ties at the normalizer's neutral 0.5 and the order is + # scan order, i.e. OLDEST-FIRST (the sweep allocates ids oldest-first). That + # is why the endpoint routes a viewer below the impression threshold to its + # `ranked` arm instead of reading this profile unpersonalized — an arbitrary + # order is strictly worse than the recency ordering that already exists. # NOTE: `window` is INERT for `agg: decay_score`. The executor calls # read_decay_score_at(entity_id, signal, 0, now) -- window index hard-coded # to 0 (tidal/src/query/executor/helpers.rs:112-114). So `all_time` below is diff --git a/site/content/blog/one-query-six-systems.mdx b/site/content/blog/one-query-six-systems.mdx index 0b29421..c7a2c3e 100644 --- a/site/content/blog/one-query-six-systems.mdx +++ b/site/content/blog/one-query-six-systems.mdx @@ -173,7 +173,7 @@ tidalDB ships 15 built-in profiles. Each is a standard `RankingProfile` struct - |---------|-----------|---------| | `trending` | `Trending` | Share velocity (24h, 2x weight) + view velocity (24h) | | `hot` | `Hot { gravity: 1.8 }` | `log10(max(views, 1)) / (age_hours + 2)^1.8` | -| `new` | `New` | Entity recency (higher ID = newer) | +| `new` | `New` | `-age_hours` -- negated age from `created_at`, newest first | | `top_week` | `TopWindow { SevenDays }` | `views * 0.3 + likes * 0.3 + shares * 0.2 + completion * views * 0.1` within 7-day window | | `top_month` | `TopWindow { ThirtyDays }` | Same multi-signal formula within 30-day window | | `top_all_time` | `TopWindow { AllTime }` | Same multi-signal formula, all-time window | @@ -198,7 +198,7 @@ fn score_by_sort(&self, entity_id: EntityId, sort: Option<&Sort>, now: Timestamp Some(Sort::Trending) => self.score_trending(entity_id), Some(Sort::Controversial) => self.score_controversial(entity_id), Some(Sort::HiddenGems) => self.score_hidden_gems(entity_id), - Some(Sort::New) => { /* entity recency */ }, + Some(Sort::New) => { /* negated age in hours from created_at */ }, Some(Sort::TopWindow { window }) => self.score_top_window(entity_id, *window), Some(Sort::MostViewed { window }) => read_agg(entity_id, "view", ...), Some(Sort::MostLiked { window }) => read_agg(entity_id, "like", ...), @@ -282,7 +282,7 @@ The acceptance test writes 1,000 items with metadata (category, format, creator_ 1. **Trending with diversity** -- 50 results, scores descending, max 1 per creator. 2. **Hot filtered by category** -- only jazz items, scores descending. -3. **New** -- entity recency descending. +3. **New** -- `created_at` recency descending (newest first). 4. **Top week** -- multi-signal weighted score within 7-day window. 5. **Hidden gems** -- quality/reach ratio ordering. 6. **Controversial** -- dual-signal ranking. diff --git a/site/content/blog/ranking-profiles-are-data.mdx b/site/content/blog/ranking-profiles-are-data.mdx index ea4439b..b205bdd 100644 --- a/site/content/blog/ranking-profiles-are-data.mdx +++ b/site/content/blog/ranking-profiles-are-data.mdx @@ -279,7 +279,7 @@ fn score_by_sort(&self, entity_id: EntityId, sort: Option<&Sort>, now: Timestamp Some(Sort::Controversial) => self.score_controversial(entity_id), Some(Sort::HiddenGems) => self.score_hidden_gems(entity_id), Some(Sort::Shuffle) => shuffle_score(entity_id.as_u64()), - Some(Sort::New) => { /* entity recency */ }, + Some(Sort::New) => { /* negated age in hours from created_at */ }, Some(Sort::TopWindow { window }) => self.score_top_window(entity_id, *window), // ... None => 0.0, diff --git a/tests/e2e/app/fixture-contract.ts b/tests/e2e/app/fixture-contract.ts index 9cb0f4a..2925451 100644 --- a/tests/e2e/app/fixture-contract.ts +++ b/tests/e2e/app/fixture-contract.ts @@ -129,14 +129,24 @@ const TITLES: Record = { * * Insertion order is category-grouped, and the feed's initial order is * id-ascending, so the first page shows one category. That is not a layout bug: - * a freshly seeded corpus has no signals, every item ties on score, and - * `for_you`'s tie-break is the entity id. Interleaving was tried and reverted — - * the `for_you` candidate scan declares `sort_field: "created_at"` - * (`tidal/src/ranking/builtins.rs:49`) but ignores a `created_at` metadata value - * entirely (measured: an order matching neither id-ascending nor - * created_at-descending came back strictly id-ascending), so insertion order and - * timestamps have no observable effect. The page stops looking uniform the - * instant a signal is written, which is the point. + * a freshly seeded corpus has no signals, every item ties on score, and the + * executor's final tie-break is ascending entity id + * (`finalize`, `tidal/src/ranking/executor/mod.rs:942-943`). Interleaving was + * tried and reverted, and the measured result (an order matching neither + * id-ascending nor created_at-descending came back strictly id-ascending) still + * stands — but the reason is NOT that `created_at` is ignored. + * + * `for_you` uses `CandidateStrategy::Ann` (`tidal/src/ranking/builtins.rs:305-311`), + * not the `Scan { sort_field: "created_at" }` that `default_strategy()` + * (`builtins.rs:47-51`) hands every skeleton, and as of the age-aware ranking + * change its `Sort::Hot { gravity: 1.5 }` (`builtins.rs:312`) reads each item's + * real `created_at` out of item metadata. The tie survives anyway because Hot's + * numerator is `log10(max(views, 1))`, which is exactly 0.0 at 0 OR 1 views: with + * no signals written there is nothing for the age divisor to scale, so every + * candidate scores 0.0 regardless of its timestamp. Seed timestamps therefore + * still have no observable effect on the initial page — but they WILL once any + * item passes its second view. The page stops looking uniform the instant a + * signal is written, which is the point. */ export const CATALOG: readonly CatalogItem[] = CATEGORIES.flatMap(({ name, idBase }) => (TITLES[name] ?? []).map((title, offset) => ({ diff --git a/tidal-server/src/dto.rs b/tidal-server/src/dto.rs index 994bcf2..5a29662 100644 --- a/tidal-server/src/dto.rs +++ b/tidal-server/src/dto.rs @@ -29,6 +29,22 @@ pub struct ItemRequest { #[schema(example = 1)] pub entity_id: u64, /// Arbitrary string→string metadata (e.g. `title`, `category`, `created_at`). + /// + /// `created_at` is special: it MUST be **nanoseconds since the Unix epoch**, + /// as a decimal string (e.g. `"1700000000000000000"`). It drives the recency + /// sorts (`Sort::New`, `Sort::Hot`) and backs the `created_at` range index. + /// Omit it and the server materializes the current time; supply it in the + /// wrong unit and it is stored verbatim, because rewriting it would corrupt + /// a value the range index already reads as nanoseconds. A seconds-unit + /// value such as `"1700000000"` parses cleanly and reads as roughly 56 years + /// old, which silently buries the item in every recency-ranked feed. The + /// server logs a warning naming the entity when a `created_at` is too small + /// to be nanoseconds. + #[schema(example = json!({ + "title": "Blue Train", + "category": "jazz", + "created_at": "1700000000000000000" + }))] pub metadata: HashMap, } diff --git a/tidal-server/tests/standalone.rs b/tidal-server/tests/standalone.rs index 96d1028..d742860 100644 --- a/tidal-server/tests/standalone.rs +++ b/tidal-server/tests/standalone.rs @@ -87,13 +87,17 @@ async fn items_signals_feed_search_round_trip() { let app = make_app(); // Two items with searchable titles + a created_at for the Hot sort age. + // `created_at` is NANOSECONDS since the Unix epoch; 1_700_000_000_000_000_000 + // is 2023-11-14T22:13:20Z. A seconds-unit value here would parse fine, read as + // ~56 years old (measured 496_731 h on 2026-09-01), and quietly floor the + // Hot score. for (id, title) in [(1u64, "jazz piano nocturne"), (2u64, "ambient jazz drift")] { let status = post_json( &app, "/items", serde_json::json!({ "entity_id": id, - "metadata": { "created_at": "1700000000", "title": title, "category": "music" } + "metadata": { "created_at": "1700000000000000000", "title": title, "category": "music" } }), ) .await; @@ -236,7 +240,9 @@ async fn feed_omits_signals_when_empty() { "/items", serde_json::json!({ "entity_id": 42, - "metadata": { "created_at": "1700000000", "title": "no signals here" } + // Nanoseconds since the Unix epoch (2023-11-14T22:13:20Z), the unit + // `Sort::New` and the `created_at` range index both read. + "metadata": { "created_at": "1700000000000000000", "title": "no signals here" } }), ) .await; diff --git a/tidal-server/tests/standalone_offload.rs b/tidal-server/tests/standalone_offload.rs index c68102c..a238564 100644 --- a/tidal-server/tests/standalone_offload.rs +++ b/tidal-server/tests/standalone_offload.rs @@ -81,13 +81,15 @@ async fn feed_returns_ranked_items_through_offload() { let app = make_app(); // Write three items with a created_at so the `for_you` Hot sort has an age. + // NANOSECONDS since the Unix epoch (2023-11-14T22:13:20Z) — the seconds-unit + // form parses but reads as ~56 years old and floors the Hot score. for id in 1u64..=3 { let status = post_json( &app, "/items", serde_json::json!({ "entity_id": id, - "metadata": { "created_at": "1700000000", "category": "tech" } + "metadata": { "created_at": "1700000000000000000", "category": "tech" } }), ) .await; diff --git a/tidal/src/db/items.rs b/tidal/src/db/items.rs index efb497d..36adbd5 100644 --- a/tidal/src/db/items.rs +++ b/tidal/src/db/items.rs @@ -592,6 +592,31 @@ impl TidalDb { Ok(()) } + /// Smallest `created_at` value this crate is willing to believe is really + /// nanoseconds since the Unix epoch: 6e17 ns, i.e. 1989-01-05. + /// + /// The point of the threshold is that the plausible ranges of the four + /// candidate units cannot overlap, so a value below it is unambiguously the + /// wrong unit rather than merely an old item. A NANOSECOND timestamp for any + /// date after ~1990 is at least ~6.3e17 (1990-01-01 = 6.31152e17). A + /// SECONDS, MILLIS or MICROS value for any date a caller could plausibly + /// mean stays far below that: seconds top out near 1e10, millis near 1e13, + /// and micros near 1e16 even for dates centuries out. Two orders of + /// magnitude of empty space separate the largest wrong-unit value from the + /// smallest right-unit one, so the check cannot fire on a real timestamp and + /// cannot miss a unit mistake for any date this century. + const MIN_PLAUSIBLE_CREATED_AT_NS: u64 = 600_000_000_000_000_000; + + /// Nanoseconds per hour, for reporting an implausible `created_at`'s age in + /// the same unit `Sort::New` and `Sort::Hot` score in. Integer division — + /// the log wants a legible magnitude, not sub-hour precision, and integer + /// math keeps the diagnostic free of a lossy `u64 -> f64` cast. + const NANOS_PER_HOUR: u64 = 3_600_000_000_000; + + /// Hours in a Julian year (365.25 days), for the human-scale second field + /// on the same log line. + const HOURS_PER_YEAR: u64 = 8_766; + /// Materialize a defaulted `created_at` INTO the persisted metadata when /// absent or unparseable, then both persist and index from this same map. /// Persisting the default (rather than only inserting it into the live @@ -599,30 +624,63 @@ impl TidalDb { /// recency ordering: the rebuild reads `created_at` from metadata and /// there is no second, divergent code path. `Cow` avoids cloning the /// common case where `created_at` is already a valid value. + /// + /// A value that parses but is implausibly small for nanoseconds is logged + /// and **stored verbatim**. Neither alternative is safe: rewriting it would + /// mean guessing a unit and corrupting a number the `created_at` range index + /// already interprets as nanoseconds, and rejecting the write would break an + /// API that accepts it today. The consequence is real and is why this warns + /// rather than staying silent — `Sort::New` scores `-age_hours`, so the + /// seconds-unit value `1700000000` reads as **496_731 hours (~56 years)** + /// old, measured 2026-09-01, and scores `-496_731` against boost sums in + /// single digits. `Sort::Hot` at 10 views and the builtin gravity 1.8 scores + /// `2.838e-3` for a correctly-dated 24-hour-old item and `5.584e-11` for the + /// same item carrying the seconds value — a factor of 5.1e7. Both sorts bury + /// the item permanently. fn metadata_with_created_at( id: EntityId, metadata: &HashMap, ) -> std::borrow::Cow<'_, HashMap> { - let needs_default = metadata + let parsed = metadata .get("created_at") - .is_none_or(|v| v.parse::().is_err()); - if needs_default { - let mut m = metadata.clone(); - if let Some(bad) = m.get("created_at") { + .and_then(|v| v.parse::().ok()); + if let Some(ns) = parsed { + if ns < Self::MIN_PLAUSIBLE_CREATED_AT_NS { + let age_hours = + Timestamp::now().as_nanos().saturating_sub(ns) / Self::NANOS_PER_HOUR; tracing::warn!( entity_id = id.as_u64(), - value = %bad, - "unparseable 'created_at' metadata; defaulting to current time" + created_at = ns, + age_hours, + age_years = age_hours / Self::HOURS_PER_YEAR, + min_plausible_ns = Self::MIN_PLAUSIBLE_CREATED_AT_NS, + "'created_at' is too small to be nanoseconds since the Unix \ + epoch; it looks like seconds, millis or micros. Stored \ + verbatim -- the value is NOT rewritten, because guessing the \ + unit would corrupt what the created_at range index already \ + reads as nanoseconds. Recency sorts will treat this item as \ + the stated age and bury it. Rewrite the item with a \ + nanosecond value to fix." ); } - m.insert( - "created_at".to_string(), - Timestamp::now().as_nanos().to_string(), - ); - std::borrow::Cow::Owned(m) - } else { - std::borrow::Cow::Borrowed(metadata) + return std::borrow::Cow::Borrowed(metadata); } + // Absent, or present but unparseable as u64: materialize the default so the + // restart rebuild reproduces the identical recency ordering from metadata + // alone, with no second divergent code path. + let mut m = metadata.clone(); + if let Some(bad) = m.get("created_at") { + tracing::warn!( + entity_id = id.as_u64(), + value = %bad, + "unparseable 'created_at' metadata; defaulting to current time" + ); + } + m.insert( + "created_at".to_string(), + Timestamp::now().as_nanos().to_string(), + ); + std::borrow::Cow::Owned(m) } /// Persist validated, `created_at`-materialized metadata and update every diff --git a/tidal/src/query/executor/pipeline.rs b/tidal/src/query/executor/pipeline.rs index 3533d56..428f079 100644 --- a/tidal/src/query/executor/pipeline.rs +++ b/tidal/src/query/executor/pipeline.rs @@ -6,6 +6,7 @@ #![allow(clippy::too_many_lines)] use std::{ + cmp::Reverse, collections::{HashMap, HashSet}, time::Instant, }; @@ -42,34 +43,55 @@ impl RetrieveExecutor<'_> { /// /// Keyed off the `created_at` range index, read newest-first, then intersected /// with the live candidate set — the index covers the whole universe while - /// `candidates` may already be narrowed, so an id from the index is kept only - /// if it is actually a candidate. Anything the index does not cover keeps its - /// relative order behind the ranked prefix. + /// `candidates` is only ever a prefix of it. /// - /// Falls back to the historical descending-entity-ID partition when no - /// `created_at` index is wired: at this point in the pipeline item metadata has - /// not been loaded, so there is no other recency key available, and an - /// approximate survivor set beats discarding the newest items outright. + /// That asymmetry is the whole difficulty, and getting it wrong INVERTS this + /// function. `scan_candidates` iterates the universe bitmap in ASCENDING id + /// order and breaks at `(limit * multiplier).max(200)`, so `candidates` is the + /// LOW-id prefix, while `top_n_descending` returns the globally newest ids — + /// which on a catalog whose ids are assigned in creation order (what + /// `metadata_with_created_at`'s `Timestamp::now()` default produces on every + /// write) are the HIGH ids. Above roughly `max_candidates + 4*cap` items the + /// two sets stop intersecting entirely. A rank-only key would then leave every + /// candidate tied at `usize::MAX`, the sort would be a no-op, and the caller's + /// `truncate(cap)` would keep the OLDEST candidates — strictly worse than the + /// descending-id partition this replaced. + /// + /// So the key is composite: true recency rank first, then DESCENDING id as the + /// tie-break for everything the oversample did not cover. Where the index and + /// the candidate window overlap the survivors are genuinely the newest; where + /// they do not, the behaviour degrades to exactly the documented pre-existing + /// approximation instead of inverting. + /// + /// Falls back to that same descending-id partition when no `created_at` index + /// is wired: item metadata is not loaded until Stage 3, so no other recency + /// key exists at this point in the pipeline. + /// + /// `select_nth_unstable_by_key`, not `sort_by_key`: this runs ONLY under + /// `reduces_candidates()`, the path whose entire purpose is to cut work on an + /// already-overloaded node, so it must stay a linear partition. Only membership + /// of `[0, cap)` matters — Stage 3 re-orders the survivors by real score. fn truncate_to_newest(&self, candidates: &mut [EntityId], cap: usize) { + // `cap` is `(limit * 4).max(100)` and `Retrieve::validate` rejects + // `limit > 500`, so `cap` is in `[100, 2000]`: `cap - 1` cannot underflow + // and `cap * 4` cannot overflow. + debug_assert!(cap > 0, "cap is floored at 100 by the caller"); let Some(index) = self.created_at_index else { - // No recency key available — preserve the documented approximation. - candidates.select_nth_unstable_by(cap - 1, |a, b| b.as_u64().cmp(&a.as_u64())); + candidates.select_nth_unstable_by_key(cap - 1, |eid| Reverse(eid.as_u64())); return; }; - // Pull more than `cap` because the index spans the full universe and some - // of its newest entities may have been filtered out of `candidates` - // already. Bounded at 4x so a heavily-filtered query cannot walk the whole - // tree; if the oversample still comes up short the remainder is filled - // from the untouched tail below, which is the same set a blind truncate - // would have kept. + // Oversample: the index spans the full universe, so some of its newest + // entities may already have been filtered out of `candidates`. Bounded at + // 4x so a heavily-filtered query cannot walk the whole tree. let newest = index.top_n_descending(cap.saturating_mul(4)); let mut rank: HashMap = HashMap::with_capacity(newest.len()); for (pos, id) in newest.iter().enumerate() { rank.entry(u64::from(*id)).or_insert(pos); } - // Stable partition: ranked entities first in recency order, everything the - // index did not cover after them in its existing order. - candidates.sort_by_key(|eid| rank.get(&eid.as_u64()).copied().unwrap_or(usize::MAX)); + candidates.select_nth_unstable_by_key(cap - 1, |eid| { + let id = eid.as_u64(); + (rank.get(&id).copied().unwrap_or(usize::MAX), Reverse(id)) + }); } /// Execute a RETRIEVE query through the 6-stage pipeline. diff --git a/tidal/src/query/executor/tests.rs b/tidal/src/query/executor/tests.rs index e460fdb..f768a30 100644 --- a/tidal/src/query/executor/tests.rs +++ b/tidal/src/query/executor/tests.rs @@ -62,10 +62,24 @@ fn scan_returns_items_ranked_by_new() { let ts: RangeIndex = RangeIndex::new("created_at"); let mut universe_bm = RoaringBitmap::new(); - // Ages OPPOSE entity id: id 1 is the newest (1h), id 10 the oldest (100h). An - // ascending fixture would be satisfied by both the real `created_at` ordering - // and the old entity-id proxy, so it would pin nothing. - let ages: Vec<(u64, u64)> = (1..=10u64).map(|i| (i, i * 10)).collect(); + // Ages are NON-MONOTONIC in entity id, so newest-first disagrees with BOTH + // wrong answers: descending id (the old `entity_id as f64` recency proxy) and + // ascending id (`ProfileExecutor::finalize`'s tie-break 2, which is exactly + // where an all-tied set lands when the Stage-1 metadata point-read never + // happens). A fixture monotonic in either direction is satisfied by one of + // those two mutations and pins nothing. + let ages: Vec<(u64, u64)> = vec![ + (1, 70), + (2, 20), + (3, 90), + (4, 10), + (5, 50), + (6, 100), + (7, 30), + (8, 80), + (9, 40), + (10, 60), + ]; for &(id, age_h) in &ages { add_item_aged( &cat, @@ -99,17 +113,20 @@ fn scan_returns_items_ranked_by_new() { assert_eq!(results.items.len(), 5); assert_eq!(results.total_candidates, 10); // Was: "highest IDs first", pinning the entity-id proxy. `new` now ranks by - // real `created_at`, so the newest item (id 1) leads. This also covers the - // widened `needs_metadata_for_sort` arm end to end: without it the metadata - // point-read never runs, every candidate scores 0.0 and the set ties. + // real `created_at`, so the newest item (id 4, at 10h) leads and the oldest + // (id 6, at 100h) is last. This also covers the widened + // `needs_metadata_for_sort` arm end to end: without it the Stage-1 metadata + // point-read never runs, every candidate scores 0.0, the set ties and + // `finalize` falls back to ascending id -- which this vector is not. assert_eq!( results .items .iter() .map(|i| i.entity_id.as_u64()) .collect::>(), - vec![1, 2, 3, 4, 5], - "newest-first by created_at, not descending entity id" + vec![4, 2, 7, 9, 5], + "newest-first by created_at; matches neither ascending nor descending \ + entity id" ); assert_eq!(results.items[0].rank, 1); } @@ -525,6 +542,109 @@ fn load_degradation_truncation_keeps_top_ranked_new_items() { ); } +/// The NON-INTERSECTING case of `truncate_to_newest`, which the 150-item fixture +/// above cannot reach. +/// +/// `scan_candidates` walks the universe bitmap in ASCENDING id order and breaks at +/// `(limit * multiplier).max(200)`, so `candidates` is the LOW-id prefix of the +/// universe. `RangeIndex::top_n_descending` returns the globally newest ids, which +/// on a time-ordered catalog are the HIGH ids. Above roughly +/// `max_candidates + 4 * cap` items the two sets stop intersecting ENTIRELY: every +/// candidate is then unranked, and a rank-only sort key made the sort a no-op, so +/// the caller's `truncate(cap)` kept the OLDEST candidates -- strictly worse than +/// the descending-id partition it replaced. +/// +/// `load_degradation_truncation_keeps_top_ranked_new_items` above is blind to this: +/// its 150-item universe is fully covered by the oversample AND its newest items +/// sit at the LOW ids, the one configuration in which the defect is invisible. +/// +/// Arithmetic settled on, all of which must hold simultaneously: +/// - no user context, so `scan_candidates`' multiplier is 4; +/// - `limit = 25` -> `max_candidates = (25 * 4).max(200) = 200`, so +/// `candidates` is ids 1..=200; +/// - `cap = (25 * 4).max(100) = 100`, and `200 > 100`, so the truncation +/// actually runs (note `limit = 50` would give `max_candidates == cap == 200` +/// and skip it entirely); +/// - oversample `= top_n_descending(cap * 4) = 400` -> the newest 400 ids, i.e. +/// 2601..=3000 on a 3,000-item catalog, which is DISJOINT from 1..=200. +/// +/// So every candidate is unranked. The composite `(rank, Reverse(id))` key then +/// partitions the 100 HIGHEST candidate ids (101..=200) into `[0, cap)`, and the +/// newest of those, id 200, tops the scored result. A rank-only key leaves ids +/// 1..=100 and yields id 100. +#[test] +fn load_degradation_truncation_survives_a_non_intersecting_oversample() { + // A TIME-ORDERED catalog: age DESCENDS with id, so id 1 is the oldest and id + // 3000 the newest. That is what `Items::metadata_with_created_at`'s + // `Timestamp::now()` default produces on a real write path, and it is the + // arrangement that makes the candidate window and the oversample disjoint. + const CATALOG: u64 = 3_000; + + let schema = test_schema(); + let ledger = SignalLedger::new(schema, Box::new(NoopWalWriter)); + let profile_reg = setup_registry(); + let cat = BitmapIndex::new("category"); + let fmt = BitmapIndex::new("format"); + let creator_idx = BitmapIndex::new("creator"); + let tag = BitmapIndex::new("tags"); + let dur: RangeIndex = RangeIndex::new("duration"); + let ts: RangeIndex = RangeIndex::new("created_at"); + let mut universe_bm = RoaringBitmap::new(); + let ages: Vec<(u64, u64)> = (1..=CATALOG).map(|id| (id, CATALOG + 1 - id)).collect(); + for &(id, age_h) in &ages { + add_item_aged( + &cat, + &fmt, + &creator_idx, + &dur, + &ts, + &mut universe_bm, + id, + age_h, + ); + } + + let universe = RwLock::new(universe_bm); + let storage = storage_with_ages(&ages); + let exec = make_executor( + &ledger, + &profile_reg, + &cat, + &fmt, + &creator_idx, + &tag, + &dur, + &ts, + &universe, + ) + .with_items_storage(&storage) + .with_degradation_level(crate::load::DegradationLevel::ReducedCandidates); + + let query = Retrieve::builder() + .profile("new") + .limit(25) + .build() + .unwrap(); + let results = exec.execute(&query).unwrap(); + + // Pin the arithmetic itself: if either cap changes, the fixture stops covering + // the non-intersecting case and this test silently becomes the one above. + assert_eq!( + results.total_candidates, 100, + "the degradation cap must have truncated 200 candidates down to 100; \ + otherwise this fixture no longer exercises the truncation path" + ); + assert_eq!(results.items.len(), 25); + assert_eq!( + results.items[0].entity_id, + EntityId::new(200), + "with the oversample disjoint from the candidate window, truncation must \ + fall back to DESCENDING id and keep ids 101..=200, whose newest member is \ + id 200; a rank-only key ties every candidate, no-ops the sort and keeps \ + ids 1..=100, topping out at id 100" + ); +} + /// §6.3: the cost of the Stage-1 metadata point-read that the widened /// `needs_metadata_for_sort` arm introduces for a `hot` profile with no diversity /// and no session -- the exact profile shape that previously loaded nothing. @@ -532,8 +652,11 @@ fn load_degradation_truncation_keeps_top_ranked_new_items() { /// Varies exactly ONE thing: whether `items_storage` is wired. Same profile, same /// candidates, same indexes, so the delta is the metadata load and nothing else. /// -/// 2,000 candidates is a realistic upper bound: `scan_candidates` caps at -/// `max(limit * 10, 200)` with user context and the engine rejects `limit > 500`. +/// 2,000 candidates is both a realistic upper bound and EXACTLY what this query +/// scans: with no user context `scan_candidates` uses multiplier 4 and caps at +/// `(limit * 4).max(200)`, so `limit = 500` -- the largest `Retrieve::validate` +/// accepts -- yields exactly 2,000. The `(limit * 10).max(200)` with-user-context +/// path is NOT the one taken here. /// /// This is a pathology guard, not a latency SLO: it fails only if the read stops /// being roughly linear-and-cheap, which is the signal that the documented @@ -607,6 +730,15 @@ fn hot_metadata_point_read_cost_is_measured_not_assumed() { let t = std::time::Instant::now(); let r = exec.execute(&query).unwrap(); assert_eq!(r.items.len(), 500); + // `per_candidate` below divides by N, so the scan must actually have + // covered N candidates or the figure is a fiction. Asserted, not + // assumed: the equality above is arithmetic, not a guarantee. + assert_eq!( + u64::try_from(r.total_candidates).unwrap(), + N, + "the measured scan must cover all {N} candidates for the \ + per-candidate cost to mean anything" + ); best = best.min(t.elapsed().as_secs_f64() * 1000.0); } best diff --git a/tidal/src/query/search/executor/pipeline.rs b/tidal/src/query/search/executor/pipeline.rs index c018173..e490895 100644 --- a/tidal/src/query/search/executor/pipeline.rs +++ b/tidal/src/query/search/executor/pipeline.rs @@ -1245,8 +1245,16 @@ mod tests { /// as the keyword-hint argument. So `Shortest` scored `NEG_INFINITY` for every /// candidate and the whole set tied, silently. /// - /// Item 2 is shorter, so it must rank first regardless of retrieval order (both - /// candidates sit on the identical vector, so relevance cannot break the tie). + /// Item 1 is the SHORTER (30s vs 600s) but the LESS relevant of the two: the + /// `ranked_registry` vectors are distinct and the query vector is item 2's exact + /// match, so relevance seeds item 2 first. `[1, 2]` is therefore reachable only + /// if the duration metadata actually reached the scorer. + /// + /// The ORDER assertion alone is still not sufficient, which is why the strict + /// score assertion below is the real discriminator: with the map unwired both + /// candidates score `NEG_INFINITY`, `normalize` folds both to 0.0, and + /// `finalize`'s ascending-id tie-break also yields `[1, 2]`. Only a strict + /// score inequality separates "the sort ran" from "the whole set tied". #[test] fn search_metadata_sort_reads_metadata_without_a_session() { let schema = test_schema(); diff --git a/tidal/src/ranking/executor/scoring.rs b/tidal/src/ranking/executor/scoring.rs index 675dfb8..cadf44d 100644 --- a/tidal/src/ranking/executor/scoring.rs +++ b/tidal/src/ranking/executor/scoring.rs @@ -144,13 +144,36 @@ impl ProfileExecutor<'_> { // (`normalize` runs on the final sum and cannot rescue the ratio // between the two terms). // - // Missing metadata scores 0.0 -- treated as brand new, matching the - // clamp direction in `read_age_hours` -- so a candidate set with no - // metadata loaded ties and normalizes to the neutral midpoint - // instead of ordering by an id that means nothing. - let score = self - .read_age_hours(entity_id, now_ns) - .map_or(0.0, |age_hours| -age_hours); + // The two `None` causes are NOT the same and must not share a + // default. Every dated candidate scores `-age_hours`, i.e. <= 0.0, + // so 0.0 is the MAXIMUM of this scale, not a neutral value. + // + // `normalize` already has a regression test for exactly this + // anomaly on exactly this shape -- + // `normalize_neg_inf_sentinel_on_negated_scale_folds_to_bottom` + // (helpers.rs) -- because `Shortest` hit it first: a pre-clamped + // 0.0 floated above every `-duration` and the LAST-ranked item + // reported the highest score 1.0. Defaulting a missing age to 0.0 + // reintroduces it, and the reachable data classes are real: a + // legacy row written before `created_at` was materialized + // (state_rebuild.rs documents the class), an empty map from + // `deserialize_metadata` on a short or corrupt row, or an entity + // dropped by the `.ok().flatten()` in the metadata map builder -- + // which means a TRANSIENT STORAGE READ ERROR would promote an item + // to the top of the feed. + let score = match self.read_age_hours(entity_id, now_ns) { + Some(age_hours) => -age_hours, + // The map was never loaded for this query, so NO candidate has + // an age: the whole set ties and normalizes to the neutral + // midpoint rather than ordering by an id that means nothing. + None if self.item_metadata.is_none() => 0.0, + // The map IS loaded and this one entity has no usable + // `created_at`. It carries no recency claim, so it sorts last + // via the sentinel the metadata sorts already use -- unlike a + // FUTURE `created_at`, which is a real claim that + // legitimately means "newest" and is clamped, not sentinelled. + None => f64::NEG_INFINITY, + }; Ok((score, smallvec![])) } Some(Sort::TopWindow { window }) => { @@ -290,9 +313,23 @@ impl ProfileExecutor<'_> { // the replication record, so replicas agree). No reverse index is needed // or wanted: it would be a second source of truth for a value already // persisted, replicated and rebuilt on this path. - let age_hours = self - .read_age_hours(entity_id, now.as_nanos()) - .unwrap_or(DEFAULT_HOT_AGE_HOURS); + let age_hours = match self.read_age_hours(entity_id, now.as_nanos()) { + Some(age) => age, + // Map never loaded for this query: uniform degradation across the + // whole candidate set, bit-for-bit the pre-fix constant. + None if self.item_metadata.is_none() => DEFAULT_HOT_AGE_HOURS, + // Map loaded but THIS entity has no usable `created_at`. Handing it + // `DEFAULT_HOT_AGE_HOURS` would award a single undated row the + // freshest divisor in the set: against a one-year-old cohort at the + // builtin gravity 1.8 the ratio `(8762/26)^1.8` is ~3e4, so a legacy + // or corrupt row with two views would outrank correctly-dated items + // with tens of thousands. Score it at Hot's floor instead -- every + // term of `hot_score` is non-negative, so 0.0 is the bottom of the + // range and promotes nothing. Now that `Sort::Hot` reports + // `needs_item_metadata`, the map is loaded on essentially every Hot + // query, which makes THIS the live branch rather than a corner. + None => return Ok((0.0, smallvec![(SignalKey::Static("view"), views)])), + }; Ok(( hot_score(views, age_hours, gravity), smallvec![(SignalKey::Static("view"), views)], diff --git a/tidal/src/ranking/executor/tests.rs b/tidal/src/ranking/executor/tests.rs index 0fd042b..6446f98 100644 --- a/tidal/src/ranking/executor/tests.rs +++ b/tidal/src/ranking/executor/tests.rs @@ -166,9 +166,10 @@ fn score_new_ranks_by_created_at_not_entity_id() { // pinned the defect: `Sort::New` used the raw id as a recency PROXY, which is // only right if ids are assigned monotonically and, worse, contributed the id's // magnitude (~N on a catalog of N) against a boost sum in the single digits. - // `Sort::New` now reads the real `created_at`, so the fixture makes id order - // and creation order DISAGREE -- id 10 is the OLDEST -- and the expected result - // is the exact reverse of what this test used to assert. + // `Sort::New` now reads the real `created_at`, so creation order is made + // NON-MONOTONIC in id: the expected vector matches neither descending id (the + // old proxy) nor ascending id (`finalize`'s tie-break 2, which is where an + // all-tied set lands if the metadata map never reaches the scorer). const NOW_NS: u64 = 1_708_000_000_000_000_000; const HOUR_NS: u64 = 3_600_000_000_000; @@ -177,7 +178,7 @@ fn score_new_ranks_by_created_at_not_entity_id() { register_builtins(&mut registry).unwrap(); let profile = registry.get("new").unwrap().clone(); let meta: std::collections::HashMap> = - [(1_u64, 1_u64), (5, 50), (10, 500)] + [(1_u64, 50_u64), (5, 1), (10, 500)] .into_iter() .map(|(id, age_h)| { ( @@ -201,8 +202,9 @@ fn score_new_ranks_by_created_at_not_entity_id() { .iter() .map(|c| c.entity_id.as_u64()) .collect::>(), - vec![1, 5, 10], - "newest first by created_at (1h, 50h, 500h), NOT by descending entity id" + vec![5, 1, 10], + "newest first by created_at (id 5 at 1h, id 1 at 50h, id 10 at 500h) -- \ + neither ascending nor descending entity id" ); } diff --git a/tidal/src/ranking/executor/tests/sort_tests.rs b/tidal/src/ranking/executor/tests/sort_tests.rs index a01f365..d591805 100644 --- a/tidal/src/ranking/executor/tests/sort_tests.rs +++ b/tidal/src/ranking/executor/tests/sort_tests.rs @@ -689,6 +689,92 @@ fn sort_hot_without_metadata_ties_at_the_uniform_fallback_age() { ); } +/// Map LOADED but an individual row undated: `score_hot` must floor it, NOT hand +/// it the `DEFAULT_HOT_AGE_HOURS` map-absent default. +/// +/// The two `None` causes from `read_age_hours` used to collapse into one +/// `unwrap_or(DEFAULT_HOT_AGE_HOURS)`. Because `Sort::Hot` now reports +/// `needs_item_metadata`, the map is loaded on essentially every Hot query, so the +/// per-entity branch is the LIVE one -- and there a 24h age against a year-old +/// cohort is a `(8762/26)^1.5` ~ 6e3 score MULTIPLIER, not a degradation. Every +/// `hot_score` term is non-negative, so 0.0 is the true floor and promotes nothing. +/// +/// Both reachable undated classes are covered: entity 1 has an EMPTY metadata +/// entry (a legacy row written before `created_at` was materialized, or a short / +/// corrupt row that `deserialize_metadata` yields nothing from), and entity 4 is +/// ABSENT from the map entirely (dropped by the `.ok().flatten()` in the metadata +/// map builder -- i.e. a transient storage read error, which under the old default +/// could promote an item to the top of the feed). +#[test] +fn sort_hot_undated_row_is_not_promoted_over_dated_cohort() { + use crate::ranking::executor::formulas::hot_score; + + // Equal views, so the age divisor is the ONLY thing that can order the set. + let ledger = ledger_with_views(&[(1, 10), (2, 10), (3, 10), (4, 10)]); + let mut meta: HashMap> = HashMap::new(); + meta.insert(1, HashMap::new()); // present, no `created_at` + for id in [2_u64, 3] { + meta.insert( + id, + [( + "created_at".to_string(), + (NOW_NS - 8_760 * H).to_string(), // one year old + )] + .into(), + ); + } + // entity 4: deliberately NOT inserted. + + let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); + let candidates: Vec = (1..=4).map(EntityId::new).collect(); + let profile = make_profile(Sort::Hot { gravity: 1.5 }); + + let result = executor + .score(&candidates, &profile, Timestamp::from_nanos(NOW_NS)) + .unwrap(); + + let rank = |id: u64| { + result + .iter() + .position(|c| c.entity_id == EntityId::new(id)) + .unwrap() + }; + for undated in [1_u64, 4] { + for dated in [2_u64, 3] { + assert!( + rank(undated) > rank(dated), + "undated entity {undated} must rank below the year-old dated \ + entity {dated}; got {:?}", + result + .iter() + .map(|c| (c.entity_id.as_u64(), c.score)) + .collect::>() + ); + } + } + + // ...and it must sit at Hot's FLOOR, not merely below this particular cohort: + // an as-if-24h-old score would beat any cohort older than 24h. + let (raw_undated, _) = executor + .score_by_sort( + EntityId::new(1), + Some(&Sort::Hot { gravity: 1.5 }), + Timestamp::from_nanos(NOW_NS), + None, + ) + .unwrap(); + assert!( + (raw_undated - 0.0).abs() < f64::EPSILON, + "an undated row must score Hot's floor 0.0, got {raw_undated}" + ); + assert!( + raw_undated < hot_score(10.0, 24.0, 1.5), + "an undated row must NOT be scored as if it were 24h old \ + (that value is {}), got {raw_undated}", + hot_score(10.0, 24.0, 1.5) + ); +} + /// The raw fallback score is byte-identical to the pre-change formula: the age /// term is the named 24h constant, not merely "some constant". #[test] @@ -744,17 +830,25 @@ fn sort_hot_malformed_created_at_stays_finite() { c.score ); } - // The future-dated item clamps to age 0, so it beats the 1h-old item rather - // than wrapping to the bottom of the set. - let rank = |id: u64| { + // The future-dated item clamps to age 0, so it must score STRICTLY MORE than + // the 1h-old item. Asserting on the SCORE rather than on the rank matters: + // `finalize`'s tie-break 2 is ASCENDING entity id, so `rank(3) < rank(4)` is + // also satisfied by any mutation that merely ties the whole set (the metadata + // map never reaching the scorer, say). A strict score comparison is reachable + // only if entity 3's age really did resolve smaller than entity 4's. + let score_of = |id: u64| { result .iter() - .position(|c| c.entity_id == EntityId::new(id)) + .find(|c| c.entity_id == EntityId::new(id)) .unwrap() + .score }; assert!( - rank(3) < rank(4), - "a future `created_at` must clamp to maximally fresh, not wrap to oldest" + score_of(3) > score_of(4), + "a future `created_at` must clamp to maximally fresh (age 0) and outscore \ + the 1h-old item, not wrap to oldest: 3 -> {}, 4 -> {}", + score_of(3), + score_of(4) ); } @@ -797,22 +891,44 @@ fn sort_hot_zero_view_corpus_still_ties_regardless_of_age() { raw.iter().all(|&s| s == 0.0), "log10(max(0,1)) == 0 zeroes the numerator, so age cannot differentiate: {raw:?}" ); - let _ = executor.score(&candidates, &profile, Timestamp::from_nanos(NOW_NS)); + // And the all-equal raw set must land on `normalize`'s neutral midpoint + // (`range < f64::EPSILON` branch), not on the maximally-confident 1.0. + let normalized = executor + .score(&candidates, &profile, Timestamp::from_nanos(NOW_NS)) + .unwrap(); + assert!( + normalized.iter().all(|c| c.score == 0.5), + "an all-equal set must fold to the neutral midpoint 0.5, got {:?}", + normalized.iter().map(|c| c.score).collect::>() + ); } /// `Sort::New` must order by real creation time, NOT by entity id. /// -/// The fixture makes the two orders DISAGREE: id 101 is the NEWEST and id 110 the -/// OLDEST. Real-age ordering therefore returns ascending id (101..110) while the -/// old `entity_id as f64` proxy returns descending id (110..101) -- opposite, so -/// the assertion discriminates. An earlier version of this test had age ascending -/// WITH id, which both implementations satisfy: it passed against the defect and -/// pinned nothing. Mutation-testing caught it. +/// The fixture makes creation order NON-MONOTONIC in entity id, so the expected +/// vector disagrees with BOTH wrong answers: descending id (what the old +/// `entity_id as f64` proxy returned) and ascending id (`finalize`'s tie-break 2, +/// which is what an all-tied set collapses to when the metadata map never reaches +/// the scorer). Two earlier versions of this fixture were monotonic in id -- one +/// ascending, one descending -- and each was satisfied by one of those two +/// mutations while pinning nothing; mutation-testing caught both. #[test] fn sort_new_orders_by_creation_time_not_entity_id() { let ledger = ledger_for(&["view"]); - // id 101 newest (6h) ... id 110 oldest (480h / 20 days). - let ages: Vec<(u64, u64)> = (0..10).map(|i| (101 + i, 6 + i * 52)).collect(); + // Ages in hours, shuffled against id: id 104 is the newest (10h) and id 106 + // the oldest (100h). + let ages: Vec<(u64, u64)> = vec![ + (101, 70), + (102, 20), + (103, 90), + (104, 10), + (105, 50), + (106, 100), + (107, 30), + (108, 80), + (109, 40), + (110, 60), + ]; let meta = meta_aged(&ages); let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); let candidates: Vec = (101..=110).map(EntityId::new).collect(); @@ -827,9 +943,9 @@ fn sort_new_orders_by_creation_time_not_entity_id() { .iter() .map(|c| c.entity_id.as_u64()) .collect::>(), - (101..=110).collect::>(), - "New must return newest-first by created_at (101..110 here), \ - not by descending entity id (110..101)" + vec![104, 102, 107, 109, 105, 110, 101, 108, 103, 106], + "New must return newest-first by created_at; this order matches neither \ + ascending id (101..110) nor descending id (110..101)" ); } @@ -886,6 +1002,65 @@ fn sort_new_without_metadata_ties_instead_of_ordering_by_id() { } } +/// Map LOADED but an individual row undated: `Sort::New` must sentinel it to the +/// BOTTOM, not to the top of its own scale. +/// +/// Every dated candidate scores `-age_hours`, which is `<= 0.0`, so the old +/// collapsed `map_or(0.0, ..)` default was the MAXIMUM of that scale: an undated +/// row ranked #1 and `normalize` reported its score as 1.0. `helpers.rs`'s +/// `normalize_neg_inf_sentinel_on_negated_scale_folds_to_bottom` exists because +/// `Shortest` hit this exact anomaly on the same negated scale first. +/// +/// The fixture is non-monotonic in id, and both undated rows sit at LOW ids, so +/// the expected order matches neither ascending id (`finalize`'s tie-break 2, +/// where an all-tied set lands) nor descending id. Entity 1 is undated with an +/// EMPTY metadata entry (legacy / corrupt row); entity 5 is ABSENT from the map +/// (dropped by the map builder's `.ok().flatten()` on a storage read error). +#[test] +fn sort_new_undated_row_sorts_last_not_first() { + let ledger = ledger_for(&["view"]); + let mut meta = meta_aged(&[(2, 1), (3, 400), (4, 200)]); + meta.insert(1, HashMap::new()); // present, no `created_at` + // entity 5: deliberately NOT inserted. + + let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); + let candidates: Vec = (1..=5).map(EntityId::new).collect(); + let profile = make_profile(Sort::New); + + let result = executor + .score(&candidates, &profile, Timestamp::from_nanos(NOW_NS)) + .unwrap(); + + assert_eq!( + result + .iter() + .map(|c| c.entity_id.as_u64()) + .collect::>(), + vec![2, 4, 3, 1, 5], + "dated rows newest-first (2 at 1h, 4 at 200h, 3 at 400h) then both undated \ + rows last; got {:?}", + result + .iter() + .map(|c| (c.entity_id.as_u64(), c.score)) + .collect::>() + ); + + // The REPORTED score must be monotonic with the rank too: the old default made + // the undated row normalize to 1.0, the top of the output range. + let undated_score = result[3].score; + assert!( + result.iter().all(|c| c.score >= undated_score), + "an undated row must report the LOWEST normalized score: {:?}", + result.iter().map(|c| c.score).collect::>() + ); + assert!( + undated_score < result[0].score, + "an undated row must not report the top score (1.0 under the old \ + collapsed default); got {undated_score} vs {}", + result[0].score + ); +} + /// Guards the single source of truth: every sort whose scorer reads the item /// metadata map must declare it, and every sort that does not must not. A new /// metadata-reading variant that forgets to opt in reproduces the original diff --git a/tidal/src/storage/indexes/range.rs b/tidal/src/storage/indexes/range.rs index 0f15b76..55bd8d4 100644 --- a/tidal/src/storage/indexes/range.rs +++ b/tidal/src/storage/indexes/range.rs @@ -494,12 +494,15 @@ mod tests { #[test] fn top_n_descending_saturates_and_handles_zero() { let index: RangeIndex = RangeIndex::new("created_at"); - for id in 1..=3u32 { - index.insert(id, u64::from(id)); + // Values are NON-MONOTONIC in entity id, so the expected order matches + // neither ascending nor descending id: a value-blind enumeration in either + // direction fails, and only a genuine by-value walk produces [2, 1, 3]. + for (id, value) in [(1_u32, 2_u64), (2, 3), (3, 1)] { + index.insert(id, value); } // n == 0 short-circuits; n beyond the population returns everything. assert!(index.top_n_descending(0).is_empty()); - assert_eq!(index.top_n_descending(99), vec![3, 2, 1]); + assert_eq!(index.top_n_descending(99), vec![2, 1, 3]); // An empty index yields nothing rather than panicking. let empty: RangeIndex = RangeIndex::new("created_at"); assert!(empty.top_n_descending(5).is_empty());