diff --git a/.gitignore b/.gitignore index c2b15b1..a2b5e85 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,10 @@ # Rust build artifacts target/ +# Agent cargo lane — a separate CARGO_TARGET_DIR so an agent build never +# invalidates or races the human's `target/` (the convention thepeach's +# CLAUDE.md mandates for cross-repo work). +target-agent/ +target-agent-*/ *.prof *.profraw diff --git a/CHANGELOG.md b/CHANGELOG.md index 50228af..6820bb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,90 @@ below. ### Breaking -**`/sharded/*` writes now require `x-tidal-ack: local` (wire-visible)** +**`Sort::New` orders by real `created_at`, not by entity ID (wire-visible)** + +`Sort::New` used `entity_id as f64` as its score. That was wrong twice over. It +assumed IDs are assigned in creation order, which caller-supplied `u64` IDs do not +guarantee; and it used the ID's *magnitude* as the base score, so on a catalog of +N items the sort term contributed ~N against a boost sum in the single digits. +Recency did not merely participate in the ranking, it annihilated every boost, and +`normalize()` runs on the final sum so it could not rescue the ratio. The score is +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. + +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. + +### Fixed + +**`Sort::Hot` is age-aware; it was ranking on view count alone** + +`score_hot` hardcoded `age_hours = 24.0` for every candidate, so the divisor in +`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. + +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: +`created_at` has been materialized **into item metadata** on every write since +`Items::metadata_with_created_at`, the executor has held an `EntityId -> metadata` +map since M6p3, and the WAL record ships the materialized map so replicas agree. No +new index, storage change, schema change or migration was required — the scorer +reads the map it already had, the same way `read_duration` does. + +Measured on a real server, 10 items with equal view counts and ages spanning +2–20 days: before, every item scored `0.5` (an all-equal set folded to the +normalizer's neutral midpoint) and the feed returned oldest-first forever; after, +scores run `1.0 → 0.0` strictly descending, newest first. + +**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 +`0.0` and ties. Age-awareness takes effect from the second view onward. Making a +cold-start corpus rank newest-first requires recency to be **additive** rather than +a pure divisor, which changes ordering for every existing `Hot` consumer, so it is +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. + +**Every metadata-based sort was dead on the SEARCH path** + +`alphabetical_asc`, `alphabetical_desc`, `shortest` and `longest` silently tied on +`GET /search` for any query without a session context. Two independent causes, both +in `query::search::executor::pipeline`: the metadata pre-load was gated on +`session_context.is_some()` alone and never consulted `profile.sort`, and the +`ProfileExecutor` it built never had `with_item_metadata` called at all — the map it +did compute was passed only as the keyword-hint argument, which the sort scorers do +not read. So `shortest`/`longest` scored `NEG_INFINITY` and the alphabetical sorts +scored the missing-title sentinel, for every candidate. + +Both query executors now derive the decision from `Sort::needs_item_metadata`, an +exhaustive match on the enum itself. The knowledge lived in a `matches!` inside one +executor while a second executor had its own, different copy — which is precisely +how a metadata-reading sort came to be omitted from both. A new variant now has one +place to declare this and the compiler forces the author to visit it. + +**`Sort::Hot`/`Sort::New` were missing from `needs_metadata_for_sort`** + +On the RETRIEVE path the metadata point-read is skipped unless the profile needs it. +Both age-derived sorts were absent from that list, so a profile with no session and +no diversity block never loaded the map the fix above depends on. Measured cost of +including them, 2,000 candidates (the realistic ceiling — `scan_candidates` caps at +`max(limit * 10, 200)` and `limit > 500` is rejected): **7.25 ms, 3.6 µs per +candidate**, guarded by a test that fails above 250 ms. + + `POST /sharded/{items,embeddings,signals}` hash-partitions by `entity_id` and applies the write to the **owning region's local store with no WAL append**. It diff --git a/k8s/discover/kustomization.yaml b/k8s/discover/kustomization.yaml new file mode 100644 index 0000000..776773a --- /dev/null +++ b/k8s/discover/kustomization.yaml @@ -0,0 +1,123 @@ +# tidalDB STANDALONE instance backing thepeach's "discover" post feed. +# Apply with: +# kubectl apply -k k8s/discover/ +# +# --------------------------------------------------------------------------- +# STANDALONE, NEVER CLUSTER. A cluster node's POST /signals applies +# signal/entity/weight and never reads user_id/creator_id +# (tidal-server/src/cluster/node.rs:7461-7503) while still answering 204. A +# cluster deployment would therefore accept every behavioural signal, learn +# nothing, and leave no wire evidence. The consumer asserts +# mode:"standalone" from /health at boot for exactly this reason. Do not +# rebase this overlay onto k8s/cluster/. +# --------------------------------------------------------------------------- +# +# WHY THE MANIFESTS ARE VENDORED HERE INSTEAD OF `resources: [..]`: +# kustomize refuses both ways of reaching k8s/'s manifests from a directory +# nested inside k8s/ (verified with kubectl v1.35.0 / kustomize v5.7.1): +# - `resources: [..]` -> "cycle detected: candidate root .../k8s +# contains visited root .../k8s/discover" +# - `resources: [../statefulset.yaml]` (or a symlink to it) +# -> "security; file .../k8s/statefulset.yaml +# is not in or below .../k8s/discover" +# The sibling overlays (cluster-local-kind, cluster-t4-kind) work only because +# k8s/cluster/ is not their parent. So statefulset.yaml, service.yaml and +# poddisruptionbudget.yaml here are BYTE-IDENTICAL copies of the ones in k8s/, +# and every discover-specific difference lives in this file plus +# statefulset-patch.yaml. That keeps the divergence auditable: +# +# for f in statefulset service poddisruptionbudget; do +# diff -u "k8s/$f.yaml" "k8s/discover/$f.yaml" || echo "DRIFT: $f" +# done # must print nothing; refresh with `cp k8s/$f.yaml k8s/discover/` +# +# k8s/namespace.yaml is deliberately NOT vendored: thepeach-staging is created +# and owned by thepeach's own tofu, and kustomize's namespace transformer +# rewrites a vendored Namespace's metadata.name, which would make this overlay +# claim ownership of thepeach's namespace. k8s/schema-configmap.yaml is not +# vendored either — the discover schema below replaces it. +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +# Lives beside thepeach's own workloads so the api and companion-worker pods +# reach it by in-cluster DNS with no cross-namespace NetworkPolicy work. NOT +# tidalDB's own `tidaldb` namespace. +namespace: thepeach-staging + +resources: + - service.yaml + - statefulset.yaml + - poddisruptionbudget.yaml + +# The schema is kept as a plain, reviewable YAML file and turned into a +# ConfigMap here rather than hand-embedded in a ConfigMap literal, so it stays +# diffable against thepeach's copy (thepeach: tidaldb_config/discover-schema.yaml) +# and loadable by a local `tidal-server standalone --schema` run. +# disableNameSuffixHash: the StatefulSet's schema volume and the tofu-side +# operator runbooks name this ConfigMap explicitly; a rolling hash would break +# both. The schema is read once at boot, so roll the StatefulSet to apply edits. +configMapGenerator: + - name: tidaldb-discover-schema + files: + - schema.yaml + options: + disableNameSuffixHash: true + +# The base image `tidaldb:deploy` is the LOCAL kind-loaded tag and would +# ImagePullBackOff on a real cluster. Pinned to the current published amd64 +# PLATFORM-manifest digest (tag m12-poisonfix-20260831), the same image the live +# cluster set runs (k8s/cluster/statefulset.yaml:103). One image serves every +# subcommand; the StatefulSet's `standalone` args select the mode. +images: + - name: tidaldb + newName: registry.threesix.ai/tidal/server + digest: sha256:93a2929d2473c753ed2322aac547dcc5a2d8598fd00aada5104553d7b8632078 + +labels: + # Mirrors the label k8s/kustomization.yaml applies to the base set. + - pairs: + app.kubernetes.io/part-of: tidaldb + includeSelectors: false + # includeSelectors: the base selects purely on app.kubernetes.io/name=tidaldb. + # In a shared namespace that would cross-match any other tidalDB instance + # (e.g. a future companions instance), silently pointing this Service at the + # wrong pods. Pinning `instance: discover` into the Service, PDB and + # StatefulSet selectors plus the pod template makes the match exact. + - pairs: + app.kubernetes.io/instance: discover + includeSelectors: true + +patches: + # Order matters: this patch targets the base name `tidaldb`, so it must run + # before the renames below. + - path: statefulset-patch.yaml + target: + kind: StatefulSet + name: tidaldb + # Rename the base's `tidaldb` triple to `tidaldb-discover`. Done with explicit + # patches rather than `nameSuffix: -discover` because nameSuffix also rewrites + # GENERATED resources, turning the ConfigMap above into + # `tidaldb-discover-schema-discover` (verified). The StatefulSet's + # spec.serviceName is set to match in statefulset-patch.yaml — a patch-driven + # rename is opaque to kustomize's nameReference transformer, so nothing + # rewires it for us. + - target: + kind: Service + name: tidaldb + patch: |- + - op: replace + path: /metadata/name + value: tidaldb-discover + - target: + kind: PodDisruptionBudget + name: tidaldb + patch: |- + - op: replace + path: /metadata/name + value: tidaldb-discover + - target: + kind: StatefulSet + name: tidaldb + patch: |- + - op: replace + path: /metadata/name + value: tidaldb-discover diff --git a/k8s/discover/poddisruptionbudget.yaml b/k8s/discover/poddisruptionbudget.yaml new file mode 100644 index 0000000..cbaa3c8 --- /dev/null +++ b/k8s/discover/poddisruptionbudget.yaml @@ -0,0 +1,20 @@ +# A single-node DB cannot stay available across a voluntary disruption (node +# drain / autoscaler scale-down) — there is no second replica to serve while +# the pod reschedules. This PDB makes that explicit: `maxUnavailable: 0` blocks +# VOLUNTARY evictions so a drain cannot kill tidalDB without an operator first +# scaling it down or accepting the downtime. It does NOT protect against +# involuntary loss (node crash) — for that you need durable PVC storage + the +# recovery path in docs/ops/recovery.md. Zero-downtime requires true +# multi-process HA, which is not yet available (tracked as m8p10). +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: tidaldb + namespace: tidaldb + labels: + app.kubernetes.io/name: tidaldb +spec: + maxUnavailable: 0 + selector: + matchLabels: + app.kubernetes.io/name: tidaldb diff --git a/k8s/discover/schema.yaml b/k8s/discover/schema.yaml new file mode 100644 index 0000000..5a9812c --- /dev/null +++ b/k8s/discover/schema.yaml @@ -0,0 +1,155 @@ +# The redgifs POSTS corpus schema, for the STANDALONE `tidaldb-discover` +# instance. Distinct from k8s/cluster/schema-configmap.yaml, which is the +# companions corpus and MUST NOT be changed by this feature. +# +# Read ONCE at boot: a ConfigMap edit needs a StatefulSet rollout. Only +# backward-compatible changes are safe against a populated data dir +# (docs/ops/recovery.md §4): +# - adding a signal SAFE +# - changing an existing signal's half_life FORBIDDEN (define a new signal) +# - changing an embedding slot's dimensions requires deleting /data +# +# The byte-identical dev copy is thepeach's tidaldb_config/discover-schema.yaml. +# `diff` them whenever either changes; nothing else guards the fork. +signals: + # Impression. Emitted on FOCUS, never on serve -- serving-time emission would + # mark every item in the page `seen` and burn the corpus in one read. Every + # context signal (one carrying a user_id) also marks the item seen for that + # user, which is what advances the feed page to page. + - name: view + entity: item + decay: + exponential: + half_life_seconds: 604800 # 7 days -- matches the companions schema + windows: [one_hour, twenty_four_hours, seven_days] + velocity: true + positive_engagement: true + # The heart of the ranking. Server-side tee off the existing post-reaction + # path, so it cannot be forged by a client and cannot drift from the reaction + # the user actually sees. + - name: like + entity: item + decay: + exponential: + half_life_seconds: 1209600 # 14 days -- matches the companions schema + windows: [twenty_four_hours, seven_days, thirty_days, all_time] + velocity: false + positive_engagement: true + # Graded engagement depth in [0,1]. The signal WEIGHT *is* the ratio -- that + # is the engine's documented contract for this exact signal name + # (tidal/src/db/signals.rs:727-735), not a convention invented here. + # Video: currentTime/duration. Image: dwell/3s, capped at 1.0. + # + # NOT present in any other shipped schema in either repo. Declared here for + # the first time; a POST /signals with this name 400s against a schema that + # omits it, inside a fire-and-forget path where nobody would see the 400. + - name: completion + entity: item + decay: + exponential: + half_life_seconds: 1209600 # 14 days, matching `like` + windows: [twenty_four_hours, seven_days, all_time] + velocity: false + positive_engagement: true +text_fields: + # `title` carries the post caption, `category` the post kind. Declared so the + # metadata the worker writes is indexed rather than inert. Nothing queries + # /search on this instance yet. + - name: title + kind: text + - name: category + kind: keyword +embedding_slots: + # DECLARED, NEVER WRITTEN in v1: 1,819 of 1,821 posts have an empty caption, + # so an embedding would give one companion's 127 posts an identical vector. + # + # Both the NAME and the WIDTH are permanent once data lands, and a future ANN + # profile must name this exact slot -- so both are fixed now to match + # thepeach's embedder (text-embedding-3-small, 1536-D) and the name every + # other peach schema already uses. The tidalDB standalone image bakes a 128-D + # default; this file overrides it. + - name: content_vector + entity: item + dimensions: 1536 +profiles: + # The v1 discover ranking. Deliberately NOT built-in `for_you`: that profile + # hard-codes CandidateStrategy::Ann{slot:"content"} + # (tidal/src/ranking/builtins.rs:305-311), which (a) names a slot this schema + # does not declare -- every peach schema calls it `content_vector` -- and + # (b) with no vectors written would fall through to the scan fallback and push + # a warning on EVERY read, which thepeach's CLAUDE.md:57 forbids. + # + # `scan` reaches the same code path with no warning. + - name: rg_discover + version: 1 + # Takes max(limit * M, 200) ids in ASCENDING ENTITY-ID ORDER, BEFORE + # seen-exclusion, where M is 10 with a user_id and 4 without + # (tidal/src/query/executor/candidate_gen.rs:22-44). That ordering is why the + # 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: + # + # 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. + # + # 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. + # + # 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. + # 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 + # not a claim that each signal declares an all_time window; built-in + # `for_you` pairs view+decay_score+all_time against a schema whose `view` + # has no all_time window, for the same reason. Do NOT "fix" this by adding + # all_time to `view.windows` (a dead index) or by switching agg. + boosts: + - signal: view + agg: decay_score + window: all_time + weight: 1.0 + - signal: like + agg: decay_score + window: all_time + weight: 2.0 + - signal: completion + agg: decay_score + window: all_time + weight: 2.5 + diversity: + # 6, not built-in for_you's 2. One companion (`Nettie`) is 127 of 1,821 + # posts and the top 10 companions are 45% of the corpus, so 2 would + # systematically starve the 1-post long tail; 6 still keeps any one + # companion off two-thirds of a 24-item page. + max_per_creator: 6 + # `format_mix_max_fraction` is DELIBERATELY ABSENT. It caps how much of a + # page one `format` metadata value may occupy, but (a) this corpus is + # 1,727 images to 94 videos (measured), so capping the dominant format + # starves pages rather than diversifying them, and (b) the worker writes + # the post kind to `category`, not `format`, so the key would govern a + # field nothing populates. Two independent reasons; do not add it back + # without changing both. + # Exploration items are APPENDED at score 0.0 + # (tidal/src/query/executor/candidate_gen.rs:215-273) — appended, so they + # rank last, not first. This does not solve cold start (see the no-sort note + # above); it keeps a zero-signal post reachable once the head of the ranking + # is signal-dominated, which is otherwise a closed loop: only signalled posts + # rank, and only ranked posts get signalled. + exploration: 0.1 diff --git a/k8s/discover/service.yaml b/k8s/discover/service.yaml new file mode 100644 index 0000000..aee074b --- /dev/null +++ b/k8s/discover/service.yaml @@ -0,0 +1,24 @@ +# Headless Service: gives the StatefulSet pod a stable DNS name +# (tidaldb-0.tidaldb.tidaldb.svc.cluster.local) and backs in-cluster clients. +# Headless (clusterIP: None) is the right default for a single stateful pod; +# readiness gating still applies, so the pod drops out of DNS A-records while +# draining. Front it with an Ingress/Gateway for external traffic — do NOT +# expose the metrics port (9091) outside the cluster (it is unauthenticated). +apiVersion: v1 +kind: Service +metadata: + name: tidaldb + namespace: tidaldb + labels: + app.kubernetes.io/name: tidaldb +spec: + clusterIP: None + selector: + app.kubernetes.io/name: tidaldb + ports: + - name: http + port: 9400 + targetPort: http + - name: metrics + port: 9091 + targetPort: metrics diff --git a/k8s/discover/statefulset-patch.yaml b/k8s/discover/statefulset-patch.yaml new file mode 100644 index 0000000..5f28255 --- /dev/null +++ b/k8s/discover/statefulset-patch.yaml @@ -0,0 +1,44 @@ +# The entire discover-specific delta to the vendored copy of k8s/statefulset.yaml. +# +# A STRATEGIC-MERGE patch, not JSON 6902: `volumes`, `containers` and `env` all +# carry `name` as their patch merge key, so every field below is addressed by a +# stable identifier. The JSON-6902 equivalent would need positional indices +# (/spec/template/spec/volumes/0/..., /spec/template/spec/containers/0/env/0/...) +# which silently patch the wrong element the moment the base list order changes. +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: tidaldb +spec: + # The base is parked at 0; scripts/restore-fleet.sh is the base's only + # supported scale-up path, so the overlay does the scaling instead of editing + # the base out from under its other consumers. 1 is the ceiling: tidalDB is + # single-node-first and scales vertically. + replicas: 1 + # kustomize's nameReference transformer cannot follow the metadata.name rename + # in kustomization.yaml (patch-driven renames are opaque to it), so the + # headless Service name is restated here. It MUST equal the Service's rendered + # name or the pod gets no stable DNS record. + serviceName: tidaldb-discover + template: + spec: + volumes: + # Replaces the base's 128-dimension `tidaldb-schema` ConfigMap with the + # discover schema (view/like/completion signals, title+category text, + # content_vector at 1536, the rg_discover ranking profile). + - name: schema + configMap: + name: tidaldb-discover-schema + containers: + - name: tidaldb + env: + # Without TIDAL_API_KEY the server leaves every data route + # unauthenticated — an open instance inside the cluster. The Secret + # is provisioned out-of-band through GCP Secret Manager (never + # `kubectl create secret`) and is deliberately not part of this + # kustomization, so no key is ever committed. + - name: TIDAL_API_KEY + valueFrom: + secretKeyRef: + name: tidaldb-discover-api-key + key: api-key diff --git a/k8s/discover/statefulset.yaml b/k8s/discover/statefulset.yaml new file mode 100644 index 0000000..276c273 --- /dev/null +++ b/k8s/discover/statefulset.yaml @@ -0,0 +1,143 @@ +# tidalDB standalone server as a StatefulSet. +# +# WHY A STATEFULSET (not a Deployment): tidalDB is single-node-first and +# embeddable — the server wraps ONE engine instance whose state (WAL + +# checkpoints + indexes) lives on a durable data dir. It scales VERTICALLY +# (bigger node), not by adding active replicas. The parked source state keeps +# `replicas: 0`; `scripts/restore-fleet.sh --standalone` restores cardinality 1. +# For HA, use the DIFFERENT multi-process `cluster` deployment: one process per +# region, quorum-acked writes, and automatic failover. It ships as its own +# StatefulSet under k8s/cluster/ and does not change this manifest. +# See docs/runbooks/kubernetes.md and docs/runbooks/cluster.md. +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: tidaldb + namespace: tidaldb + labels: + app.kubernetes.io/name: tidaldb + app.kubernetes.io/component: server +spec: + serviceName: tidaldb # the headless Service in service.yaml — stable network id + replicas: 0 # parked; scripts/restore-fleet.sh is the only supported scale-up path + selector: + matchLabels: + app.kubernetes.io/name: tidaldb + template: + metadata: + labels: + app.kubernetes.io/name: tidaldb + app.kubernetes.io/component: server + annotations: + # Plain-Prometheus scrape hints (the PodMonitor/ServiceMonitor in + # servicemonitor.yaml is the Operator-native alternative). Metrics are + # unauthenticated — keep :9091 cluster-internal (see ops/monitoring.md). + prometheus.io/scrape: "true" + prometheus.io/port: "9091" + prometheus.io/path: "/metrics" + spec: + # SIGTERM flips readiness to 503 (pod leaves Endpoints), drains in-flight + # requests, then checkpoints + fsyncs the WAL before exit. Give that room. + terminationGracePeriodSeconds: 60 + securityContext: + runAsNonRoot: true + runAsUser: 10001 # the `tidal` user baked into docker/deploy/Dockerfile + runAsGroup: 10001 + fsGroup: 10001 # makes the mounted PVC group-writable by the runtime user + seccompProfile: + type: RuntimeDefault + containers: + - name: tidaldb + # For a real cluster, replace with your registry image pinned by digest + # (e.g. registry.example.com/tidaldb@sha256:...) and set + # imagePullPolicy: IfNotPresent. `tidaldb:deploy` is the local image + # built from docker/deploy/Dockerfile and loaded via `kind load`. + image: tidaldb:deploy + imagePullPolicy: IfNotPresent + # ENTRYPOINT is the bare binary; these args override the image CMD so + # the schema comes from the mounted ConfigMap, not the baked default. + args: + - standalone + - --listen + - 0.0.0.0:9400 + - --schema + - /etc/tidaldb/schema/schema.yaml + - --data-dir + - /data + - --metrics + - 0.0.0.0:9091 + env: + - name: TIDAL_API_KEY + valueFrom: + secretKeyRef: + name: tidaldb-api-key + key: api-key + - name: TIDAL_SERVER_LOG + value: info + ports: + - name: http + containerPort: 9400 + - name: metrics + containerPort: 9091 + # Three distinct probes map to the three health endpoints: + # - /health/startup : always 200 once the HTTP listener is up + # - /health/live : always 200 while the process is alive + # - /health : 200 ready / 503 while draining on SIGTERM + # All are unauthenticated by design, so the probes need no token. + startupProbe: + httpGet: + path: /health/startup + port: http + periodSeconds: 5 + failureThreshold: 60 # up to ~5 min for large-DB WAL replay / index load (capacity-planning.md) + livenessProbe: + httpGet: + path: /health/live + port: http + periodSeconds: 10 + timeoutSeconds: 3 + failureThreshold: 3 + readinessProbe: + httpGet: + path: /health # 503 during drain -> removed from Service Endpoints + port: http + periodSeconds: 10 + timeoutSeconds: 3 + failureThreshold: 3 + resources: + requests: + cpu: "250m" + memory: 256Mi + limits: + cpu: "2" + memory: 2Gi # size from docs/ops/capacity-planning.md for your item/embedding count + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true # the server only writes /data (PVC) and /tmp (emptyDir) + capabilities: + drop: ["ALL"] + volumeMounts: + - name: data + mountPath: /data + - name: schema + mountPath: /etc/tidaldb/schema + readOnly: true + - name: tmp + mountPath: /tmp + volumes: + - name: schema + configMap: + name: tidaldb-schema + - name: tmp + emptyDir: {} + volumeClaimTemplates: + - metadata: + name: data + labels: + app.kubernetes.io/name: tidaldb + spec: + accessModes: ["ReadWriteOnce"] + # storageClassName: "" # uncomment + set to pin a class; omitted = cluster default + resources: + requests: + storage: 10Gi diff --git a/tidal/src/query/executor/mod.rs b/tidal/src/query/executor/mod.rs index b7116f5..d8ca478 100644 --- a/tidal/src/query/executor/mod.rs +++ b/tidal/src/query/executor/mod.rs @@ -461,10 +461,14 @@ impl<'a> RetrieveExecutor<'a> { // Pre-load item metadata for keyword hint matching (session), // metadata-based sorts (alphabetical, duration), or diversity / notification // cap enforcement (both need creator_id from metadata to group candidates). - let needs_metadata_for_sort = matches!( - profile.sort, - Some(Sort::AlphabeticalAsc | Sort::AlphabeticalDesc | Sort::Shortest | Sort::Longest) - ); + // Delegated to `Sort::needs_item_metadata` so this list cannot drift from + // the scorers that read the map, nor from the SEARCH executor's copy of + // the same decision. `Hot` and `New` were missing here: `Hot` fell back to + // a uniform age term, collapsing to a view-count ranking that returned + // oldest-first on any zero-signal corpus, and `New` ordered by raw entity + // id. Both returned HTTP 200 with a plausible-looking page, and nothing in + // logs or metrics distinguished either from working. + let needs_metadata_for_sort = profile.sort.as_ref().is_some_and(Sort::needs_item_metadata); let needs_metadata_for_creator_grouping = profile.diversity.max_per_creator.is_some() || profile.diversity.format_mix_max_fraction.is_some() || query.notification_caps.is_some(); diff --git a/tidal/src/query/executor/pipeline.rs b/tidal/src/query/executor/pipeline.rs index de6000e..3533d56 100644 --- a/tidal/src/query/executor/pipeline.rs +++ b/tidal/src/query/executor/pipeline.rs @@ -5,7 +5,10 @@ #![allow(clippy::too_many_lines)] -use std::{collections::HashSet, time::Instant}; +use std::{ + collections::{HashMap, HashSet}, + time::Instant, +}; use super::{RetrieveExecutor, candidate_gen, post_filter, user_filter}; use crate::{ @@ -35,6 +38,40 @@ const ANN_OVERFETCH: usize = 10; const ANN_CANDIDATE_FLOOR: usize = 200; impl RetrieveExecutor<'_> { + /// Partition `candidates` so the `cap` NEWEST entities occupy `[0, cap)`. + /// + /// 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. + /// + /// 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. + fn truncate_to_newest(&self, candidates: &mut [EntityId], cap: usize) { + 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())); + 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. + 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)); + } + /// Execute a RETRIEVE query through the 6-stage pipeline. /// /// # Errors @@ -286,22 +323,27 @@ impl RetrieveExecutor<'_> { // // A blind `truncate(cap)` keeps the FIRST `cap` candidates in scan order. // `scan_candidates` iterates the universe bitmap in ASCENDING entity-ID - // order, so a blind truncate keeps the LOWEST IDs. For an ID-ordered - // profile that ranks by DESCENDING entity ID (e.g. `New`), the highest IDs - // are exactly the items that should rank at the top — so a blind truncate - // drops the top-ranked items before scoring and corrupts the "new" - // ranking. We therefore truncate by the profile's primary ordering key: - // for `New`, keep the `cap` HIGHEST IDs via an O(N) `select_nth_unstable` - // partition (no full sort; Stage 3 still orders the survivors). Other - // profiles score on signal state rather than scan position, so their - // truncation order is not load-bearing and a plain truncate is retained. + // order, so a blind truncate keeps the LOWEST IDs. For a recency profile + // (`New`) the newest items are exactly the ones that should rank at the + // top, so a blind truncate drops the top-ranked items before scoring ever + // sees them and corrupts the ranking. We therefore truncate by the + // profile's primary ordering key. Other profiles score on signal state + // rather than scan position, so their truncation order is not load-bearing + // and a plain truncate is retained. + // + // The key for `New` is the `created_at` index, read newest-first. It used + // to be the entity ID, which was correct only while `Sort::New` itself + // ranked by descending ID; now that scoring reads the real `created_at`, + // an ID-keyed truncation would silently discard the genuinely newest items + // under load and leave the ranking wrong in a way visible only when + // degraded. With no `created_at` index wired there is no better key + // available at this point in the pipeline — metadata is not loaded until + // Stage 3 — so the ID heuristic remains the documented fallback. if self.degradation_level.reduces_candidates() { let cap = (query.limit * 4).max(100); if candidates.len() > cap { if matches!(profile.sort, Some(Sort::New)) { - // Partition so the `cap` highest IDs occupy [0, cap): the items - // that rank highest under descending-ID order survive. - candidates.select_nth_unstable_by(cap - 1, |a, b| b.as_u64().cmp(&a.as_u64())); + self.truncate_to_newest(&mut candidates, cap); } candidates.truncate(cap); } diff --git a/tidal/src/query/executor/test_support.rs b/tidal/src/query/executor/test_support.rs index 4e59d89..4b1d087 100644 --- a/tidal/src/query/executor/test_support.rs +++ b/tidal/src/query/executor/test_support.rs @@ -70,6 +70,64 @@ pub(super) fn add_item( universe.insert(id_u32); } +/// Add an item whose `created_at` is `age_hours` before [`FIXED_NOW_NS`], so a +/// test can make entity-ID order and creation order disagree. +#[allow(clippy::too_many_arguments)] +pub(super) fn add_item_aged( + category_idx: &BitmapIndex, + format_idx: &BitmapIndex, + creator_idx: &BitmapIndex, + duration_idx: &RangeIndex, + created_at_idx: &RangeIndex, + universe: &mut RoaringBitmap, + id: u64, + age_hours: u64, +) { + let id_u32 = id as u32; + category_idx.insert(id_u32, "jazz"); + format_idx.insert(id_u32, "video"); + creator_idx.insert(id_u32, "1"); + duration_idx.insert(id_u32, 0u32); + created_at_idx.insert(id_u32, FIXED_NOW_NS - age_hours * HOUR_NS); + universe.insert(id_u32); +} + +/// Fixed query clock for age-sensitive fixtures. +pub(super) const FIXED_NOW_NS: u64 = 1_708_000_000_000_000_000; +/// One hour in nanoseconds. +pub(super) const HOUR_NS: u64 = 3_600_000_000_000; + +/// Items-only length-prefixed metadata encoding that `db::deserialize_metadata` +/// reads back, mirroring `db::metadata::serialize_metadata` (not visible here). +pub(super) fn encode_meta(pairs: &[(&str, &str)]) -> Vec { + let mut buf = Vec::new(); + buf.extend_from_slice(&(pairs.len() as u32).to_le_bytes()); + for (k, v) in pairs { + buf.extend_from_slice(&(k.len() as u32).to_le_bytes()); + buf.extend_from_slice(k.as_bytes()); + buf.extend_from_slice(&(v.len() as u32).to_le_bytes()); + buf.extend_from_slice(v.as_bytes()); + } + buf +} + +/// Storage holding a `created_at` metadata row per `(id, age_hours)` pair, which +/// is what the Stage-3 point-read loads for an age-derived sort. +pub(super) fn storage_with_ages(ages: &[(u64, u64)]) -> crate::storage::InMemoryBackend { + use crate::storage::{StorageEngine, Tag, encode_key}; + let storage = crate::storage::InMemoryBackend::new(); + for &(id, age_h) in ages { + let key = encode_key(crate::schema::EntityId::new(id), Tag::Meta, b""); + storage + .put( + &key, + &encode_meta(&[("created_at", &(FIXED_NOW_NS - age_h * HOUR_NS).to_string())]), + ) + .unwrap(); + } + storage +} + /// Build an executor from test indexes. #[allow(clippy::too_many_arguments)] pub(super) fn make_executor<'a>( diff --git a/tidal/src/query/executor/tests.rs b/tidal/src/query/executor/tests.rs index 536de8f..e460fdb 100644 --- a/tidal/src/query/executor/tests.rs +++ b/tidal/src/query/executor/tests.rs @@ -6,7 +6,9 @@ use std::sync::RwLock; use roaring::RoaringBitmap; -use super::test_support::{add_item, make_executor, setup_registry, test_schema}; +use super::test_support::{ + add_item, add_item_aged, make_executor, setup_registry, storage_with_ages, test_schema, +}; use crate::{ entities::{CreatorItemsBitmap, HardNegIndex, InteractionLedger, UserStateIndex}, query::retrieve::{QueryError, Retrieve}, @@ -60,22 +62,25 @@ fn scan_returns_items_ranked_by_new() { let ts: RangeIndex = RangeIndex::new("created_at"); let mut universe_bm = RoaringBitmap::new(); - for i in 1..=10u64 { - add_item( + // 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(); + for &(id, age_h) in &ages { + add_item_aged( &cat, &fmt, &creator_idx, &dur, &ts, &mut universe_bm, - i, - "jazz", - "video", - 1, + id, + age_h, ); } let universe = RwLock::new(universe_bm); + let storage = storage_with_ages(&ages); let exec = make_executor( &ledger, &profile_reg, @@ -86,14 +91,26 @@ fn scan_returns_items_ranked_by_new() { &dur, &ts, &universe, - ); + ) + .with_items_storage(&storage); let query = Retrieve::builder().profile("new").limit(5).build().unwrap(); let results = exec.execute(&query).unwrap(); assert_eq!(results.items.len(), 5); assert_eq!(results.total_candidates, 10); - // "new" sorts by entity_id descending -- highest IDs first. - assert_eq!(results.items[0].entity_id, EntityId::new(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. + 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" + ); assert_eq!(results.items[0].rank, 1); } @@ -444,10 +461,14 @@ fn for_creator_without_creator_items_returns_empty_not_full_scan() { fn load_degradation_truncation_keeps_top_ranked_new_items() { // Regression: under ReducedCandidates load, the candidate cap is // `(limit*4).max(100)`. `scan_candidates` iterates the universe in ASCENDING - // ID order, so a blind `truncate(cap)` would keep the LOWEST IDs. The "new" - // profile sorts by DESCENDING ID, so the highest IDs are the top-ranked - // items — a blind truncate would drop exactly the items that should rank #1. - // The fix truncates by descending ID for `New`, so the highest IDs survive. + // ID order, so a blind `truncate(cap)` keeps the LOWEST IDs — dropping exactly + // the items a recency profile should rank #1 before scoring ever sees them. + // + // The survivor set is chosen by the `created_at` index, read newest-first. It + // used to be chosen by descending entity ID, which was only right while + // `Sort::New` itself ranked by descending ID. This fixture makes the two keys + // DISAGREE — id 1 is the newest and id 150 the oldest — so an ID-keyed + // truncation keeps the 100 OLDEST items and cannot produce the right answer. let schema = test_schema(); let ledger = SignalLedger::new(schema, Box::new(NoopWalWriter)); let profile_reg = setup_registry(); @@ -460,24 +481,23 @@ fn load_degradation_truncation_keeps_top_ranked_new_items() { let mut universe_bm = RoaringBitmap::new(); // 150 items: more than the degradation cap of (1*4).max(100) = 100, so the - // truncation path is exercised. The highest IDs (101..=150) would be dropped - // by a blind ascending-order truncate. - for i in 1..=150u64 { - add_item( + // truncation path is exercised. Age ascends with id, so id 1 is the newest. + let ages: Vec<(u64, u64)> = (1..=150u64).map(|i| (i, i)).collect(); + for &(id, age_h) in &ages { + add_item_aged( &cat, &fmt, &creator_idx, &dur, &ts, &mut universe_bm, - i, - "jazz", - "video", - 1, + id, + age_h, ); } let universe = RwLock::new(universe_bm); + let storage = storage_with_ages(&ages); let exec = make_executor( &ledger, &profile_reg, @@ -489,18 +509,124 @@ fn load_degradation_truncation_keeps_top_ranked_new_items() { &ts, &universe, ) + .with_items_storage(&storage) .with_degradation_level(crate::load::DegradationLevel::ReducedCandidates); let query = Retrieve::builder().profile("new").limit(1).build().unwrap(); let results = exec.execute(&query).unwrap(); - // The #1 "new" item is the highest ID (150). A blind truncate would have - // dropped 101..=150 before scoring and returned 100 instead. assert_eq!(results.items.len(), 1); assert_eq!( results.items[0].entity_id, - EntityId::new(150), - "top 'new' item must be the highest ID, not the highest surviving \ - ascending-order id after truncation" + EntityId::new(1), + "top 'new' item must be the NEWEST by created_at (id 1); an entity-ID \ + keyed truncation keeps ids 51..=150, the 100 OLDEST items, and can only \ + return one of those" + ); +} + +/// §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. +/// +/// 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`. +/// +/// 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 +/// follow-up (narrow the read to the single `created_at` key instead of +/// deserializing the whole metadata map) is actually needed. +#[test] +fn hot_metadata_point_read_cost_is_measured_not_assumed() { + const N: u64 = 2_000; + + let schema = test_schema(); + let ledger = SignalLedger::new(schema, Box::new(NoopWalWriter)); + let mut profile_reg = setup_registry(); + let mut hot = + crate::ranking::test_fixtures::profile_with_sort(crate::ranking::profile::Sort::Hot { + gravity: 1.5, + }); + hot.name = "bench_hot".to_string(); + profile_reg.register(hot).unwrap(); + + 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..=N).map(|i| (i, i % 720)).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 query = Retrieve::builder() + .profile("bench_hot") + .limit(500) + .build() + .unwrap(); + + let measure = |with_storage: bool| -> f64 { + let base = make_executor( + &ledger, + &profile_reg, + &cat, + &fmt, + &creator_idx, + &tag, + &dur, + &ts, + &universe, + ); + let exec = if with_storage { + base.with_items_storage(&storage) + } else { + base + }; + for _ in 0..3 { + exec.execute(&query).unwrap(); + } + let mut best = f64::MAX; + for _ in 0..10 { + let t = std::time::Instant::now(); + let r = exec.execute(&query).unwrap(); + assert_eq!(r.items.len(), 500); + best = best.min(t.elapsed().as_secs_f64() * 1000.0); + } + best + }; + + let with_meta = measure(true); + let without = measure(false); + let delta = with_meta - without; + // N is a small const; the cast cannot lose precision in practice and this is + // a diagnostic print, not a computation the assertion depends on. + #[allow(clippy::cast_precision_loss)] + let per_candidate = delta / N as f64; + eprintln!( + "MEASURED n={N} with_metadata={with_meta:.2}ms without={without:.2}ms \ + delta={delta:.2}ms per_candidate={per_candidate:.5}ms" + ); + + assert!( + delta < 250.0, + "metadata point-read for {N} candidates cost {delta:.2}ms; narrow the \ + read to the single `created_at` key rather than reverting the arm" ); } diff --git a/tidal/src/query/search/executor/pipeline.rs b/tidal/src/query/search/executor/pipeline.rs index 915f4b0..c018173 100644 --- a/tidal/src/query/search/executor/pipeline.rs +++ b/tidal/src/query/search/executor/pipeline.rs @@ -702,9 +702,52 @@ impl SearchExecutor<'_> { retrieval_scores: &HashMap, now: Timestamp, ) -> Result, QueryError> { + // Pre-load item metadata for keyword hint matching (session) or for a sort + // that scores from a metadata field. + // + // The sort clause was absent here, so every metadata-based sort was dead on + // the SEARCH path: `shortest` / `longest` scored `NEG_INFINITY` for every + // candidate and `alphabetical_asc` / `_desc` scored the missing-title + // sentinel, i.e. the whole set tied and the sort contributed nothing. That + // is the same defect class as `Hot`'s hardcoded age, in a second executor + // — which is why the predicate now lives on `Sort` itself rather than being + // restated in each pipeline. + // + // Computed BEFORE the executor is built so the borrow handed to + // `with_item_metadata` outlives every use of the executor below. + let needs_metadata_for_sort = profile + .sort + .as_ref() + .is_some_and(crate::ranking::profile::Sort::needs_item_metadata); + let item_metadata: HashMap> = + if self.session_context.is_some() || needs_metadata_for_sort { + self.items_storage.map_or_else(HashMap::new, |storage| { + candidates + .iter() + .filter_map(|&eid| { + let key = encode_key(eid, Tag::Meta, b""); + storage + .get(&key) + .ok() + .flatten() + .map(|bytes| (eid.as_u64(), deserialize_item_metadata(&bytes))) + }) + .collect() + }) + } else { + HashMap::new() + }; + let mut executor = ProfileExecutor::new(self.ledger).with_degradation_level(self.degradation_level); + // Hand the map to the executor as well as to the scoring call below. The + // scoring argument feeds keyword-hint matching; the builder field is what + // the sort scorers read, and it was never set on this path. + if !item_metadata.is_empty() { + executor = executor.with_item_metadata(&item_metadata); + } + // Wire the Shuffle sort's per-user, per-minute seed (spec §11.6) so SEARCH // shuffle varies per user; without a FOR USER it stays a stable per-minute // global permutation. @@ -714,26 +757,6 @@ impl SearchExecutor<'_> { executor = executor.with_shuffle_user(user_id); } - // Pre-load item metadata for keyword hint matching when session is active. - let item_metadata: HashMap> = if self.session_context.is_some() - { - self.items_storage.map_or_else(HashMap::new, |storage| { - candidates - .iter() - .filter_map(|&eid| { - let key = encode_key(eid, Tag::Meta, b""); - storage - .get(&key) - .ok() - .flatten() - .map(|bytes| (eid.as_u64(), deserialize_item_metadata(&bytes))) - }) - .collect() - }) - } else { - HashMap::new() - }; - #[allow(clippy::option_if_let_else)] let scored = if let Some(user_id) = query.for_user { let mut user_ctx = @@ -1179,6 +1202,140 @@ mod tests { )) } + /// Two items with DISTINCT vectors so relevance is deterministic: against a + /// `[1.0, 0.0]` query, item 2 is an exact match and item 1 is orthogonal. + /// + /// `two_item_registry` puts both candidates on the identical vector, which + /// makes their retrieval order a genuine tie -- and a tie is resolved + /// differently depending on how the suite is scheduled (a sort assertion built + /// on it passed in a 12-test run and failed run alone). Any test that asserts + /// ORDER needs relevance to be unambiguous. + fn ranked_registry() -> RwLock { + RwLock::new(registry_with_slot( + "content", + &[(1, [0.0, 1.0]), (2, [1.0, 0.0])], + )) + } + + /// Result ids in RANKED order. `ids_of` sorts, which cannot observe ordering. + fn ranked_ids(results: &SearchResults) -> Vec { + results.items.iter().map(|r| r.entity_id.as_u64()).collect() + } + + /// A registry holding `setup_registry`'s builtins plus one profile named + /// `metasort` carrying the given metadata-reading [`Sort`]. + fn registry_with_sort(sort: crate::ranking::profile::Sort) -> ProfileRegistry { + let mut reg = setup_registry(); + let mut p = crate::ranking::test_fixtures::profile_with_sort(sort); + p.name = "metasort".to_string(); + reg.register(p).unwrap(); + reg + } + + /// Store `pairs` as item metadata for `id`, the way the write path would. + fn put_meta(storage: &InMemoryBackend, id: u64, pairs: &[(&str, &str)]) { + let key = encode_key(EntityId::new(id), Tag::Meta, b""); + storage.put(&key, &encode_meta(pairs)).unwrap(); + } + + /// Every metadata-based sort was DEAD on the SEARCH path: the pipeline's + /// metadata pre-load was gated on `session_context.is_some()` alone and never + /// consulted `profile.sort`, and the `ProfileExecutor` it built never had + /// `with_item_metadata` called at all -- the map it did compute was passed only + /// 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). + #[test] + fn search_metadata_sort_reads_metadata_without_a_session() { + let schema = test_schema(); + let ledger = SignalLedger::new(schema, Box::new(NoopWalWriter)); + let profile_reg = registry_with_sort(crate::ranking::profile::Sort::Shortest); + // Relevance favours item 2 (exact match); duration favours item 1, so the + // two orderings disagree and the assertion observes the sort. + let registry = ranked_registry(); + let storage = InMemoryBackend::new(); + put_meta(&storage, 1, &[("duration", "30")]); + put_meta(&storage, 2, &[("duration", "600")]); + + let universe = two_item_universe(); + let exec = filtered_executor(&ledger, &profile_reg, ®istry, &universe) + .with_items_storage(&storage); + let query = Search::builder() + .vector(vec![1.0f32, 0.0]) + .using_profile("metasort") + .limit(10) + .build() + .unwrap(); + + let results = exec.execute(&query).unwrap(); + assert_eq!( + ranked_ids(&results), + vec![1, 2], + "SEARCH `shortest` must rank the 30s item (1) above the 600s item (2), \ + overriding the higher relevance seed item 2 receives; an unchanged \ + relevance order means the metadata map never reached the scorer" + ); + assert!( + results.items[0].score > results.items[1].score, + "scores must be STRICTLY ordered: a tie means the sort contributed \ + nothing and the surviving order is just retrieval order ({:?})", + results.items.iter().map(|i| i.score).collect::>() + ); + } + + /// The same gap for the age-derived sorts: SEARCH with `Sort::New` must order + /// by real `created_at`, newest first, with no session context. + #[test] + fn search_new_sort_reads_created_at_without_a_session() { + let schema = test_schema(); + let ledger = SignalLedger::new(schema, Box::new(NoopWalWriter)); + let profile_reg = registry_with_sort(crate::ranking::profile::Sort::New); + let registry = ranked_registry(); + let storage = InMemoryBackend::new(); + // Item 1 is 1 hour old, item 2 is 20 days old. + // + // An ordering assertion only discriminates when the two candidate orderings + // DISAGREE. On the SEARCH path each candidate's base score is seeded with + // its fused relevance score, and with the metadata map unwired that seed is + // the only thing left to order by. `ranked_registry` makes it unambiguous: + // item 2 is the exact vector match, so relevance favours item 2 and recency + // must favour item 1. + let now_ns = 1_708_000_000_000_000_000_u64; + let h = 3_600_000_000_000_u64; + put_meta(&storage, 1, &[("created_at", &(now_ns - h).to_string())]); + put_meta( + &storage, + 2, + &[("created_at", &(now_ns - 480 * h).to_string())], + ); + + let universe = two_item_universe(); + let exec = filtered_executor(&ledger, &profile_reg, ®istry, &universe) + .with_items_storage(&storage); + let query = Search::builder() + .vector(vec![1.0f32, 0.0]) + .using_profile("metasort") + .limit(10) + .build() + .unwrap(); + + let results = exec.execute(&query).unwrap(); + assert_eq!( + ranked_ids(&results), + vec![1, 2], + "SEARCH `new` must rank the 1h-old item (1) above the 20d-old one (2), \ + overriding the higher relevance seed item 2 receives" + ); + assert!( + results.items[0].score > results.items[1].score, + "scores must be STRICTLY ordered; a tie means `created_at` was never \ + read ({:?})", + results.items.iter().map(|i| i.score).collect::>() + ); + } + fn ids_of(results: &SearchResults) -> Vec { let mut v: Vec = results.items.iter().map(|r| r.entity_id.as_u64()).collect(); v.sort_unstable(); diff --git a/tidal/src/ranking/executor/scoring.rs b/tidal/src/ranking/executor/scoring.rs index 84a10f7..675dfb8 100644 --- a/tidal/src/ranking/executor/scoring.rs +++ b/tidal/src/ranking/executor/scoring.rs @@ -54,6 +54,20 @@ fn resolve_sort_signal_type( /// by `u64::MAX`. `2^64` is exactly representable in `f64`. const TWO_POW_64: f64 = 18_446_744_073_709_551_616.0; +/// Age assumed by [`Sort::Hot`] when a candidate's real age is unavailable. +/// +/// A profile may legitimately be scored with no item-metadata map loaded — that +/// is the default for any profile with no session, no metadata sort and no +/// diversity block — so Hot degrades to a *uniform* age term rather than +/// failing. With the divisor constant across the candidate set, `hot_score` +/// collapses to `log10(max(views, 1))`, i.e. a pure view-count cohort ranking. +/// That is a defensible answer, but it is NOT recency: prefer loading metadata +/// (see `needs_metadata_for_sort` in `query::executor`) over relying on this. +const DEFAULT_HOT_AGE_HOURS: f64 = 24.0; + +/// Nanoseconds per hour, the divisor turning a `created_at` delta into hours. +const NANOS_PER_HOUR: f64 = 3_600_000_000_000.0; + impl ProfileExecutor<'_> { /// Returns `true` when the given signal name is suppressed by community read policy. /// @@ -120,16 +134,23 @@ impl ProfileExecutor<'_> { Some(Sort::HiddenGems) => self.score_hidden_gems(entity_id, now_ns, vals), Some(Sort::Shuffle) => Ok((self.score_shuffle(entity_id, now_ns, vals)?, smallvec![])), Some(Sort::New) => { - // M2 limitation: entity metadata (`created_at`) is not accessible from the - // executor. Entity ID is used as a proxy for recency -- ranks higher IDs - // first, which is correct only when IDs are assigned monotonically. - // Caller-specified u64 IDs are not guaranteed monotonic; this is a - // best-effort approximation. Exact creation-time sorting requires - // `db.read_item()` to be plumbed into the executor (deferred to M3+). - // f64 cast cannot overflow (u64::MAX fits in f64 with ~11 bits of - // precision loss for very large IDs, which is acceptable for ranking). - #[allow(clippy::cast_precision_loss)] - let score = entity_id.as_u64() as f64; + // Negated age in hours: newer is closer to 0, so higher, preserving + // "newest first" while keeping the term on a scale comparable to a + // boost sum. The previous implementation used the raw entity id as + // the base score, which was wrong twice over -- it assumed ids are + // assigned monotonically, and on a catalog of N items it contributed + // ~N against a boost sum in the single digits, so recency did not + // merely participate in the ranking, it annihilated every boost + // (`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); Ok((score, smallvec![])) } Some(Sort::TopWindow { window }) => { @@ -234,12 +255,15 @@ impl ProfileExecutor<'_> { } } - /// `now` is the query clock: it ages the all-time view count read consistently - /// across the candidate set (via the ledger's explicit-clock read). It is not - /// yet a per-candidate *age* reference: Hot scoring applies a *uniform* 24h age - /// term because no per-entity `created_at` is plumbed into the executor (see - /// the `age_hours` comment below). When age-aware Hot scoring lands, this `now` - /// becomes the age reference too. + /// `now` is the query clock, used for two things: it ages the all-time view + /// count read consistently across the candidate set (via the ledger's + /// explicit-clock read), and it is the reference against which each + /// candidate's `created_at` becomes an age. Both terms therefore share one + /// clock, so a score is reproducible for a given `now`. + /// + /// When the item-metadata map is not loaded for this query the age term + /// degrades to [`DEFAULT_HOT_AGE_HOURS`], reducing Hot to a view-count cohort + /// ranking; see that constant for why that is a fallback and not a design. fn score_hot( &self, entity_id: EntityId, @@ -260,15 +284,15 @@ impl ProfileExecutor<'_> { now.as_nanos(), vals, )?; - // The scoring loop receives no per-entity `created_at`: the executor reads - // only the signal ledger, and the `created_at` range index is keyed - // `value -> {entity_id}` (built for range filters), so resolving an - // individual candidate's age would require an O(n) tree scan or a separate - // `EntityId -> created_at_ns` reverse map that is not built. Hot scoring - // therefore treats every candidate as 24 hours old, ranking by view count - // with a uniform age term. (A reverse lookup is the prerequisite for - // genuine age-aware hot scoring; it is intentionally not faked here.) - let age_hours = 24.0_f64; + // Real per-candidate age from the item-metadata map, which already carries + // `created_at` because `Items::metadata_with_created_at` materializes it + // into the persisted metadata on every write (and ships that same map in + // 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); Ok(( hot_score(views, age_hours, gravity), smallvec![(SignalKey::Static("view"), views)], @@ -717,6 +741,33 @@ impl ProfileExecutor<'_> { self.read_duration(entity_id).unwrap_or(f64::NEG_INFINITY) } + /// Age in hours derived from the `created_at` metadata key (nanoseconds since + /// the epoch, materialized on write by `Items::metadata_with_created_at`). + /// + /// Returns `None` -- meaning "caller decides the default" -- when the metadata + /// map was not loaded for this query, the entity is absent from it, or the key + /// is absent or unparseable as `u64`. + /// + /// A `created_at` in the future clamps to age 0 (maximally fresh) rather than + /// wrapping: `saturating_sub` on the nanosecond delta. Without the clamp a + /// clock-skewed or deliberately future-dated item would underflow `u64` into + /// an astronomically old age and sort last forever, which is both wrong and + /// invisible. Fresh is the safer wrong answer and matches how a future-dated + /// item behaves in every other recency ordering. + /// + /// A `created_at` expressed in the wrong UNIT (seconds, say) parses fine and + /// yields an enormous age, so the item sorts last. That is deliberate: it is a + /// data error, "sorts last" is a defensible answer for one, and guessing the + /// unit from magnitude would silently rewrite a caller-supplied value that the + /// `created_at` range index already interprets as nanoseconds. + #[allow(clippy::cast_precision_loss)] // ns delta -> hours; f64 mantissa covers ~10^5 years + fn read_age_hours(&self, entity_id: EntityId, now_ns: u64) -> Option { + let meta_map = self.item_metadata?; + let meta = meta_map.get(&entity_id.as_u64())?; + let created_ns = meta.get("created_at")?.parse::().ok()?; + Some(now_ns.saturating_sub(created_ns) as f64 / NANOS_PER_HOUR) + } + /// Read the "duration" metadata field as f64 seconds. Returns `None` if absent /// or unparseable. fn read_duration(&self, entity_id: EntityId) -> Option { diff --git a/tidal/src/ranking/executor/tests.rs b/tidal/src/ranking/executor/tests.rs index c03ffba..0fd042b 100644 --- a/tidal/src/ranking/executor/tests.rs +++ b/tidal/src/ranking/executor/tests.rs @@ -161,20 +161,49 @@ fn score_nan_does_not_propagate() { } #[test] -fn score_new_ranks_higher_ids_first() { +fn score_new_ranks_by_created_at_not_entity_id() { + // Was `score_new_ranks_higher_ids_first`, asserting entity_id-descending. That + // 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. + const NOW_NS: u64 = 1_708_000_000_000_000_000; + const HOUR_NS: u64 = 3_600_000_000_000; + let ledger = test_ledger(); let mut registry = ProfileRegistry::new(); register_builtins(&mut registry).unwrap(); let profile = registry.get("new").unwrap().clone(); - let executor = ProfileExecutor::new(&ledger); - // Sort::New scores by entity_id descending (higher ID = more recent proxy). + let meta: std::collections::HashMap> = + [(1_u64, 1_u64), (5, 50), (10, 500)] + .into_iter() + .map(|(id, age_h)| { + ( + id, + [( + "created_at".to_string(), + (NOW_NS - age_h * HOUR_NS).to_string(), + )] + .into(), + ) + }) + .collect(); + + let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); let candidates = vec![EntityId::new(1), EntityId::new(5), EntityId::new(10)]; - let now = Timestamp::from_nanos(1_708_000_000_000_000_000); + let now = Timestamp::from_nanos(NOW_NS); let result = executor.score(&candidates, &profile, now).unwrap(); assert_eq!(result.len(), 3); - assert_eq!(result[0].entity_id, EntityId::new(10)); - assert_eq!(result[1].entity_id, EntityId::new(5)); - assert_eq!(result[2].entity_id, EntityId::new(1)); + assert_eq!( + result + .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" + ); } // -- Personalized scoring tests ------------------------------------------- diff --git a/tidal/src/ranking/executor/tests/sort_tests.rs b/tidal/src/ranking/executor/tests/sort_tests.rs index f9a6c05..a01f365 100644 --- a/tidal/src/ranking/executor/tests/sort_tests.rs +++ b/tidal/src/ranking/executor/tests/sort_tests.rs @@ -600,3 +600,341 @@ fn sort_date_saved_no_save_sorted_last() { } assert_eq!(result[2].score, 0.0); } + +// -- Age-aware `Hot` / `New` (hot-sort-age-awareness) ───────────────────────── + +/// Nanoseconds per hour, mirroring `scoring::NANOS_PER_HOUR` for fixture math. +const H: u64 = 3_600_000_000_000; +/// Fixed query clock for the age fixtures below. +const NOW_NS: u64 = 1_708_000_000_000_000_000; + +/// Metadata map of `entity_id -> created_at` at the given ages in hours. +fn meta_aged(ages_h: &[(u64, u64)]) -> HashMap> { + ages_h + .iter() + .map(|&(id, age_h)| { + ( + id, + [("created_at".to_string(), (NOW_NS - age_h * H).to_string())].into(), + ) + }) + .collect() +} + +/// A ledger with `view` recorded `n` times for each entity, so `Hot`'s numerator +/// is non-zero and the age term can actually be observed. +fn ledger_with_views(views: &[(u64, usize)]) -> SignalLedger { + let ledger = ledger_for(&["view"]); + let now = Timestamp::from_nanos(NOW_NS); + for &(id, n) in views { + for _ in 0..n { + ledger + .record_signal("view", EntityId::new(id), 1.0, now) + .unwrap(); + } + } + ledger +} + +/// THE defect this work fixes: at IDENTICAL view counts, `Hot` must order by real +/// age, newest first. Before the fix `score_hot` hardcoded `age_hours = 24.0`, so +/// the divisor was constant across the candidate set and all three scored +/// identically -- a view-count ranking wearing a recency sort's name. +#[test] +fn sort_hot_orders_by_real_age_at_equal_view_counts() { + let ledger = ledger_with_views(&[(1, 10), (2, 10), (3, 10)]); + let meta = meta_aged(&[(1, 400), (2, 25), (3, 1)]); + let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); + let candidates: Vec = (1..=3).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(); + + assert_eq!( + result.iter().map(|c| c.entity_id).collect::>(), + vec![EntityId::new(3), EntityId::new(2), EntityId::new(1)], + "equal views must rank newest first; got {:?}", + result + .iter() + .map(|c| (c.entity_id.as_u64(), c.score)) + .collect::>() + ); + assert!( + result[0].score > result[1].score && result[1].score > result[2].score, + "scores must be strictly descending, not merely ordered: {:?}", + result.iter().map(|c| c.score).collect::>() + ); +} + +/// The fallback is EXACTLY today's behaviour: with no metadata map loaded, every +/// candidate is scored at `DEFAULT_HOT_AGE_HOURS`, so a set with equal views ties. +/// This is what makes the change safe for callers that load no metadata. +#[test] +fn sort_hot_without_metadata_ties_at_the_uniform_fallback_age() { + let ledger = ledger_with_views(&[(1, 10), (2, 10), (3, 10)]); + let executor = ProfileExecutor::new(&ledger); // no with_item_metadata + let candidates: Vec = (1..=3).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 scores: Vec = result.iter().map(|c| c.score).collect(); + assert!( + scores.windows(2).all(|w| (w[0] - w[1]).abs() < 1e-12), + "no metadata must degrade to a uniform age term, so equal views tie: {scores:?}" + ); +} + +/// 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] +fn sort_hot_fallback_score_equals_hot_score_at_24h() { + use crate::ranking::executor::formulas::hot_score; + + let ledger = ledger_with_views(&[(1, 10)]); + let executor = ProfileExecutor::new(&ledger); + let (raw, _snapshot) = executor + .score_by_sort( + EntityId::new(1), + Some(&Sort::Hot { gravity: 1.5 }), + Timestamp::from_nanos(NOW_NS), + None, + ) + .unwrap(); + + assert!( + (raw - hot_score(10.0, 24.0, 1.5)).abs() < 1e-12, + "fallback must be exactly hot_score(views, 24.0, gravity); got {raw}" + ); +} + +/// Absent key, unparseable value, and a FUTURE `created_at` must all produce a +/// finite score -- never NaN, never inf, never a wrapped `u64` age. The future +/// case clamps to age 0 (maximally fresh) rather than underflowing into an +/// astronomically old item that sorts last forever. +#[test] +fn sort_hot_malformed_created_at_stays_finite() { + let ledger = ledger_with_views(&[(1, 10), (2, 10), (3, 10), (4, 10)]); + let mut meta: HashMap> = HashMap::new(); + meta.insert(1, HashMap::new()); // key absent -> fallback + meta.insert(2, [("created_at".into(), "not-a-number".into())].into()); // -> fallback + meta.insert( + 3, + [("created_at".into(), (NOW_NS + 9_000 * H).to_string())].into(), + ); // future + meta.insert(4, [("created_at".into(), (NOW_NS - H).to_string())].into()); // 1h old + + 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(); + + for c in &result { + assert!( + c.score.is_finite(), + "entity {} scored non-finite {}", + c.entity_id.as_u64(), + 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| { + result + .iter() + .position(|c| c.entity_id == EntityId::new(id)) + .unwrap() + }; + assert!( + rank(3) < rank(4), + "a future `created_at` must clamp to maximally fresh, not wrap to oldest" + ); +} + +/// MEASURED LIMIT, pinned deliberately: `hot_score`'s numerator is +/// `log10(max(views, 1))`, which is EXACTLY 0.0 for 0 or 1 views, so the age +/// divisor cannot differentiate a corpus with no signals -- every candidate +/// scores 0.0 whatever its age, and `normalize` folds the all-equal set to the +/// neutral midpoint. +/// +/// Age-awareness therefore takes effect from the SECOND view onward. This test +/// exists because the bug report claimed the age fix alone would make a +/// zero-signal corpus rank newest-first; it arithmetically cannot. Fixing that +/// requires making recency ADDITIVE rather than a pure divisor, which changes +/// ordering for every existing `Hot` consumer and all four built-in `Hot` +/// profiles, so it is a separate decision and not folded in here. +#[test] +fn sort_hot_zero_view_corpus_still_ties_regardless_of_age() { + let ledger = ledger_for(&["view"]); // zero signals + let meta = meta_aged(&[(1, 400), (2, 25), (3, 1)]); + let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); + let candidates: Vec = (1..=3).map(EntityId::new).collect(); + let profile = make_profile(Sort::Hot { gravity: 1.5 }); + + let raw: Vec = candidates + .iter() + .map(|&eid| { + executor + .score_by_sort( + eid, + Some(&Sort::Hot { gravity: 1.5 }), + Timestamp::from_nanos(NOW_NS), + None, + ) + .unwrap() + .0 + }) + .collect(); + + assert!( + 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)); +} + +/// `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. +#[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(); + let meta = meta_aged(&ages); + let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); + let candidates: Vec = (101..=110).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::>(), + (101..=110).collect::>(), + "New must return newest-first by created_at (101..110 here), \ + not by descending entity id (110..101)" + ); +} + +/// `Sort::New`'s raw score must be on a scale comparable to a boost sum, not the +/// catalog's id magnitude. The old implementation returned `entity_id as f64`, so +/// on a catalog with 9-digit ids the sort term contributed ~1e9 against boosts in +/// the single digits: recency did not participate in the ranking, it annihilated +/// every boost. `normalize` runs on the final sum and cannot rescue that ratio. +#[test] +fn sort_new_score_is_age_magnitude_not_entity_id_magnitude() { + let ledger = ledger_for(&["view"]); + let big = 1_000_000_000_u64; + let meta = meta_aged(&[(big, 3)]); // 3 hours old + let executor = ProfileExecutor::new(&ledger).with_item_metadata(&meta); + + let (raw, _) = executor + .score_by_sort( + EntityId::new(big), + Some(&Sort::New), + Timestamp::from_nanos(NOW_NS), + None, + ) + .unwrap(); + + assert!( + (raw - -3.0).abs() < 1e-9, + "New must score negated age in hours (-3.0), got {raw}" + ); + assert!( + raw.abs() < 1e3, + "New's magnitude must be boost-comparable, not id-scale: {raw}" + ); +} + +/// Missing metadata makes `New` score 0.0 -- treated as brand new -- so the set +/// ties instead of ordering by an entity id that carries no recency meaning. +#[test] +fn sort_new_without_metadata_ties_instead_of_ordering_by_id() { + let ledger = ledger_for(&["view"]); + let executor = ProfileExecutor::new(&ledger); // no metadata + for id in [1_u64, 999_999_u64] { + let (raw, _) = executor + .score_by_sort( + EntityId::new(id), + Some(&Sort::New), + Timestamp::from_nanos(NOW_NS), + None, + ) + .unwrap(); + assert!( + (raw - 0.0).abs() < f64::EPSILON, + "no metadata must score 0.0 for every id, got {raw} for {id}" + ); + } +} + +/// 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 +/// defect, and the exhaustive match in `needs_item_metadata` is what forces the +/// author to make the choice. +#[test] +fn needs_item_metadata_matches_the_sorts_that_actually_read_it() { + for sort in [ + Sort::Hot { gravity: 1.5 }, + Sort::New, + Sort::AlphabeticalAsc, + Sort::AlphabeticalDesc, + Sort::Shortest, + Sort::Longest, + ] { + assert!( + sort.needs_item_metadata(), + "{sort:?} scores from item metadata and must declare it" + ); + } + for sort in [ + Sort::Trending, + Sort::Controversial, + Sort::HiddenGems, + Sort::Shuffle, + Sort::MostFollowed, + Sort::CreatorEngagementRate, + Sort::Rising, + Sort::LiveViewerCount, + Sort::DateSaved, + Sort::MostViewed { + window: Window::AllTime, + }, + Sort::MostLiked { + window: Window::AllTime, + }, + Sort::MostCommented { + window: Window::AllTime, + }, + Sort::MostShared { + window: Window::AllTime, + }, + Sort::TopWindow { + window: Window::AllTime, + }, + ] { + assert!( + !sort.needs_item_metadata(), + "{sort:?} does not read item metadata and must not force the point-reads" + ); + } +} diff --git a/tidal/src/ranking/profile.rs b/tidal/src/ranking/profile.rs index b238aba..0cd70ba 100644 --- a/tidal/src/ranking/profile.rs +++ b/tidal/src/ranking/profile.rs @@ -99,6 +99,49 @@ pub enum Sort { DateSaved, } +impl Sort { + /// Whether scoring this sort reads the per-candidate item-metadata map. + /// + /// This lives on the enum, beside the variants, because it is a property OF + /// the variant: a query executor cannot know it without being told, and + /// there is more than one query executor. Encoding it as a `matches!` inside + /// one of them is what produced the original defect -- the retrieve path + /// listed four variants, the search path never consulted `sort` at all, and + /// adding a metadata-reading variant silently ordered results wrong on both. + /// A new variant that reads metadata now has exactly one place to declare it, + /// and the compiler's exhaustiveness check forces the author to visit it. + /// + /// `DateSaved` is deliberately absent: it reads the save timestamp from user + /// state, not from item metadata. + #[must_use] + pub const fn needs_item_metadata(&self) -> bool { + match self { + // Age-derived (`created_at`) and field-derived (`title` / `duration`). + Self::Hot { .. } + | Self::New + | Self::AlphabeticalAsc + | Self::AlphabeticalDesc + | Self::Shortest + | Self::Longest => true, + // Ledger-derived, user-state-derived, or constant: no metadata read. + Self::Trending + | Self::Controversial + | Self::HiddenGems + | Self::Shuffle + | Self::TopWindow { .. } + | Self::MostViewed { .. } + | Self::MostLiked { .. } + | Self::MostFollowed + | Self::CreatorEngagementRate + | Self::Rising + | Self::MostCommented { .. } + | Self::MostShared { .. } + | Self::LiveViewerCount + | Self::DateSaved => false, + } + } +} + // ── Candidate strategy ────────────────────────────────────────────────────── /// How candidates are sourced for ranking. diff --git a/tidal/src/storage/indexes/range.rs b/tidal/src/storage/indexes/range.rs index ca7a7dd..0f15b76 100644 --- a/tidal/src/storage/indexes/range.rs +++ b/tidal/src/storage/indexes/range.rs @@ -69,6 +69,46 @@ impl RangeIndex { &self.field_name } + /// The `n` entity ids with the HIGHEST indexed values, highest first. + /// + /// Walks the B-tree backwards and stops as soon as `n` ids are collected, so + /// the cost is `O(n + distinct_values_visited)` rather than a full scan or a + /// sort of the whole index. Ties within one value are yielded in the value + /// bitmap's own ascending-id order; a caller that needs a total order must + /// impose one itself. + /// + /// Exists so a load-shedding candidate cap can truncate by a real ordering key + /// instead of a proxy. `Sort::New`'s truncation used to keep the highest entity + /// IDs, which is only the right survivor set if ids happen to be assigned in + /// creation order; keyed off this index it is right unconditionally. + #[must_use] + pub fn top_n_descending(&self, n: usize) -> Vec { + if n == 0 { + return Vec::new(); + } + let mut out = Vec::with_capacity(n); + // Scoped so the read guard is released before returning, rather than living + // to the end of the function body alongside the value being handed back. + { + let tree = self + .tree + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner); + for bitmap in tree.values().rev() { + for id in bitmap { + out.push(id); + if out.len() == n { + break; + } + } + if out.len() == n { + break; + } + } + } + out + } + /// Add an entity with the given attribute value. pub fn insert(&self, entity_id: u32, value: V) { let mut tree = self @@ -439,6 +479,44 @@ impl RangeIndex { #[cfg(test)] #[allow(clippy::unwrap_used, clippy::float_cmp)] mod tests { + + #[test] + fn top_n_descending_returns_highest_values_first() { + // Values deliberately OPPOSE entity id: entity 1 holds the largest value, + // so a result of [1, 2, 3] proves the walk is by VALUE and not by id. + let index: RangeIndex = RangeIndex::new("created_at"); + for id in 1..=5u32 { + index.insert(id, u64::from(6 - id) * 100); + } + assert_eq!(index.top_n_descending(3), vec![1, 2, 3]); + } + + #[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)); + } + // 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]); + // An empty index yields nothing rather than panicking. + let empty: RangeIndex = RangeIndex::new("created_at"); + assert!(empty.top_n_descending(5).is_empty()); + } + + #[test] + fn top_n_descending_groups_ties_under_one_value() { + // Three entities share the newest value; the next value down follows. + let index: RangeIndex = RangeIndex::new("created_at"); + for id in [7u32, 8, 9] { + index.insert(id, 500); + } + index.insert(1, 400); + let top = index.top_n_descending(4); + assert_eq!(&top[..3], &[7, 8, 9], "the tied newest value comes first"); + assert_eq!(top[3], 1, "then the next value down"); + } use proptest::prelude::*; use super::*;