fix(pgvector): bound the get_recent scan when the filter is not pushdown-safe

PgVectorCollection.get_recent pushed the ORDER BY into SQL but passed
limit=None to _scroll whenever _requires_local_filter(where) was true, so
a filter like {"$or": [{"wing": "w1"}, {"wing": "w2"}]} dragged the whole
table across the wire to keep `limit` rows. Layer 1 never hits this (it
passes {"wing": ...} or None, both pushdown-safe), but get_recent is
public API and this is the shape a caller reaches for first.

The predicate still cannot ride along, but the ordering can, so instead
of one LIMIT-less scan the local-filter branch now walks the table
newest-first a SQL page at a time and stops at the first page that
completes the answer. On the common shape (a filter most rows match)
that is a single page. Details:

- Page stability. ORDER BY metadata->>field DESC NULLS LAST, id is a
  total order because id is the primary key, so OFFSET paging is well
  defined, matching the guarantee the existing ORDER BY id paging in
  scroll_rows relies on. Rows are deduped by id so a concurrent insert
  that shifts a row across a page boundary cannot return it twice. A
  concurrent delete can still skip one row, which is inherent to OFFSET
  paging and unchanged from the pre-existing paged get.
- Cap. The walk stops after 50,000 rows, so a filter matching almost
  nothing in a huge table returns fewer than `limit` rows rather than
  reading the table. Because the walk is newest-first, what it returns
  is still the newest matching rows within the newest 50,000 records.
  The pushdown branch keeps its unchanged SQL and stays exact.
- Projection. The post-filter reads only metadata, and this branch can
  scan far more rows than it returns, so the document column is
  projected out unless the caller asked for it.

Because the cap makes the non-pushdown path approximate where it used to
be exact, supports_recency_order is now spelled out rather than left to
read as a blanket promise: it covers the filters the backend can
evaluate in storage, which is every filter Layer 1 uses, and a backend
that bounds its non-pushdown walk must document the bound.

Also fixes two things the same diff introduced:

- BaseCollection.get_recent returned padding values for projections the
  caller excluded, so include=["metadatas"] answered with a list of
  empty strings for documents where pgvector answers with []. Worse,
  include=["documents"] meant metadatas never came back, every sort key
  collapsed to the same value and the sort silently did nothing.
  metadatas are now always fetched because the sort reads order_field
  out of them, and only returned when requested.
- The docstrings claimed the inexact branch re-sorted locally after
  filtering, which it never did, and asserted filed_at is always UTC. It
  is not: diary_ingest writes datetime.now(timezone.utc).isoformat() and
  every other writer uses datetime.now().isoformat(), so on a host off
  UTC the two sort against each other skewed by the local offset. That
  predates this change (Layer 1 has compared filed_at as text since
  #1630) and standardising the writers needs its own migration, so the
  docs now name the limitation instead of denying it. The list of places
  where the SQL order and recency_sort_key disagree also now includes
  database collation, which the Python test double cannot emulate.

Tests: the reviewer's exact scenario (800 rows, an $or filter, limit=5)
asserting every scroll carries a SQL LIMIT and the rows requested stay
well under the table; a selective filter that has to page three times,
checking OFFSET advances and the order stays newest-first; the cap
stopping a filter that matches nothing; a row shifted across a page
boundary by a concurrent insert, which returns a duplicate without the
id dedupe; the document projection; and the base-class include
projection.

Author: ATKabli

CHANGELOG.md

@@ -13,7 +13,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 - **`sweep` books a failed `stat` as a failure again.** The non-regular-file gate added in 3.7.1 reads `stat.S_ISREG(f.stat().st_mode)` inside a `try`, and its `except OSError` printed `SKIP` and moved on. A dangling symlink, a symlink loop and a file unlinked between `rglob` and the gate all raise there, and before the gate existed every one of them reached `sweep()` and was booked in `failures` — so `sweep` went from reporting a transcript it could not read to reporting success. A failed probe is now an error, not a benign file type: it is logged, printed as `WARNING`, and appended to `failures`, while a probe that succeeds and says "not regular" still skips silently. (#2221)
 - **`mempalace init` no longer tracebacks on a directory it cannot enter.** `_parse_gradle`'s `is_file()` gate sat in front of the `try` that the parser's own `except OSError` provides, so a manifest under a directory with `r` but no `x` raised `PermissionError` out of a call that used to answer "no manifest name". The gate moved inside that `try`, and `_collect_manifest_names` stats through `os.path.isfile`, which reports rather than raises. (#2221)
 - **`split` no longer blocks on a FIFO at its own output name, nor writes through a broken link.** The type gate in `main` covers the files the glob listed; `split_file` builds its output names itself, so a pre-existing named pipe at one of them wedged `write_text` in the kernel waiting for a reader. The check asks about the link itself rather than its target, because a dangling symlink at an output name reads as "nothing there" and `write_text` would create the target — landing a chunk outside the output directory. Output names that are anything but a regular file are now skipped with a `SKIP` line. (#2221)
-- **Layer 1 wake-up fetches the most recent drawers instead of an arbitrary scan window.** `BaseCollection.get_recent()` returns the newest N by `filed_at`; pgvector overrides it with an `ORDER BY ... DESC LIMIT n` pushdown (capability token `supports_recency_order`), and backends without pushdown keep the previous scan-and-sort behavior. On palaces larger than the 2,000-drawer scan cap, wake-up no longer leads with the oldest backfill. (#1630)
+- **Layer 1 wake-up fetches the most recent drawers instead of an arbitrary scan window.** `BaseCollection.get_recent()` returns the newest N by `filed_at`; pgvector overrides it with an `ORDER BY ... DESC LIMIT n` pushdown (capability token `supports_recency_order`), and backends without pushdown keep the previous scan-and-sort behavior. On palaces larger than the 2,000-drawer scan cap, wake-up no longer leads with the oldest backfill. Ordering is on the stored `filed_at` text, the same comparison Layer 1 has used since #1630. (#1630)
 
 ---
 

mempalace/backends/base.py

@@ -362,6 +362,16 @@ def recency_sort_key(meta: Optional[dict], order_field: str = "filed_at") -> tup
     last instead of raising on a str/None comparison. Backends that implement
     :meth:`BaseCollection.get_recent` with a local sort MUST use this key so
     every backend orders identically.
+
+    This compares the timestamps as *text*, which is chronological only while
+    every value shares one offset representation. It is not a new assumption:
+    Layer 1 has sorted ``filed_at`` as text since #1630 and this key just
+    names the behaviour. It is also not currently true of ``filed_at`` --
+    ``diary_ingest`` writes ``datetime.now(timezone.utc).isoformat()``
+    (``...+00:00``) while every other writer uses ``datetime.now().isoformat()``
+    (naive local), so on a host that is not on UTC the two sort against each
+    other skewed by the local offset. Standardising the writers is a separate
+    change; it needs a migration for palaces that already hold both forms.
     """
     value = (meta or {}).get(order_field)
     if not isinstance(value, str) or not value:
@@ -540,24 +550,48 @@ def get_recent(
 
         ``order_field`` names a metadata key holding an ISO-8601 timestamp
         string (``filed_at`` for drawers). Ordering is descending on that
-        string, which is chronological for ISO-8601. Records whose value is
-        missing, empty, or not a string sort last.
+        string; see :func:`recency_sort_key` for what text ordering promises.
+        Records whose value is missing, empty, or not a string sort last.
 
         The default implementation pages through :meth:`get` in storage order
         up to ``limit`` records and sorts that window locally. That is exact
-        for a collection no larger than ``limit`` and *approximate* above it:
-        the window is whatever the backend hands back first, so the genuinely
-        newest records can fall outside it (issue #1630's known limitation for
-        Layer 1 wake-up). Backends able to push the ordering into storage MUST
-        override this and advertise the ``supports_recency_order`` capability
-        token, which promises exactness at any collection size.
+        when the collection holds no more than ``limit`` records matching
+        ``where``, and *approximate* above that: the window is whatever the
+        backend hands back first, so the genuinely newest records can fall
+        outside it (issue #1630's known limitation for Layer 1 wake-up).
+        Backends able to push the ordering into storage MUST override this and
+        advertise the ``supports_recency_order`` capability token. What that
+        token promises, exactly:
+
+        * The returned records really are the top ``limit`` under the ordering
+          above, at any collection size, **whenever the backend can also
+          evaluate ``where`` in storage** (including ``where=None``).
+        * It says nothing about whether that text ordering matches wall-clock
+          order. That is a property of what the writers store, not of the
+          backend.
+        * A filter the backend cannot push into storage has to be evaluated
+          record by record, so a backend MAY bound how far it walks and return
+          fewer than ``limit`` records rather than read the whole collection.
+          A backend that bounds it MUST document the bound on its override.
+          pgvector does; see :meth:`PgVectorCollection._scroll_recent_local`.
 
         Callers that need the guarantee should check the token rather than
-        assume it; the default is always available so no backend breaks.
+        assume it, and should read it as covering the filters the backend can
+        push down. The default is always available so no backend breaks.
+
+        ``include`` follows the same contract as :meth:`get`: projections the
+        caller did not ask for come back empty. ``metadatas`` is fetched
+        regardless because the sort reads ``order_field`` from it, but it is
+        only *returned* when requested.
         """
         if limit <= 0:
             return GetResult.empty()
         include = ["documents", "metadatas"] if include is None else list(include)
+        want_documents = "documents" in include
+        want_metadatas = "metadatas" in include
+        # The local sort needs order_field, so metadatas always come back from
+        # the backend even when the caller projected them out of the result.
+        fetch_include = include if want_metadatas else [*include, "metadatas"]
 
         ids: list[str] = []
         documents: list[str] = []
@@ -566,7 +600,7 @@ def get_recent(
         fetched = 0
         page_size = min(500, limit)
         while fetched < limit:
-            kwargs: dict = {"include": include, "limit": page_size, "offset": offset}
+            kwargs: dict = {"include": fetch_include, "limit": page_size, "offset": offset}
             if where:
                 kwargs["where"] = where
             batch = self.get(**kwargs)
@@ -591,8 +625,8 @@ def get_recent(
         order = _recency_order(metadatas, order_field)[:limit]
         return GetResult(
             ids=[ids[i] for i in order],
-            documents=[documents[i] for i in order],
-            metadatas=[metadatas[i] for i in order],
+            documents=[documents[i] for i in order] if want_documents else [],
+            metadatas=[metadatas[i] for i in order] if want_metadatas else [],
             embeddings=None,
         )
 

mempalace/backends/pgvector.py

@@ -72,6 +72,16 @@
     {"$eq", "$ne", "$in", "$nin", "$and", "$or", "$contains", "$gt", "$gte", "$lt", "$lte"}
 )
 _PUSHDOWN_OPERATORS = frozenset({"$eq", "$ne", "$in", "$nin", "$and"})
+# Bounds for the local-post-filter branch of ``get_recent``. The pushdown
+# branch needs none — SQL does ORDER BY ... LIMIT n and returns exactly n
+# rows. The post-filter branch cannot push the predicate, so it walks the
+# table newest-first in pages and stops as soon as ``limit`` rows match.
+# The page size trades round trips against rows on the wire; the row cap
+# bounds the pathological case (a filter that matches nothing in a large
+# table) so one call can never walk unboundedly.
+_RECENT_SCAN_PAGE_MIN = 500
+_RECENT_SCAN_PAGE_MAX = 5000
+_RECENT_SCAN_ROW_CAP = 50_000
 
 
 def _utcnow() -> str:
@@ -1211,35 +1221,145 @@ def get(
             embeddings=[row["embedding"] or [] for row in rows] if spec.embeddings else None,
         )
 
+    def _scroll_recent_local(self, *, where, limit, order_field, with_embedding, with_document):
+        """Newest-first paged scan with the filter applied in Python.
+
+        Used when ``where`` is not exactly expressible as ``metadata @> ...``
+        ($or, comparisons, ...). The predicate cannot ride along, but the
+        *ordering* still can, so instead of dragging the whole table across
+        the wire to keep ``limit`` rows we walk it newest-first one SQL page
+        at a time and stop at the first page that completes the answer. On
+        the common shape (a filter most rows match) that is a single page.
+
+        Page stability: ``ORDER BY metadata->>field DESC NULLS LAST, id`` is a
+        total order because ``id`` is the primary key, so OFFSET paging is
+        well defined — the same guarantee the existing ``ORDER BY id`` paging
+        in :meth:`_PgVectorClient.scroll_rows` relies on. Concurrent writes
+        can still shift rows across a page boundary: an insert of a newer row
+        pushes one row down and would hand it back twice, which the ``seen``
+        set drops, and a delete pulls one row up and can skip it. That skip
+        window is inherent to OFFSET paging and is unchanged from the
+        pre-existing paged ``get``; a keyset cursor would close it and is a
+        separate change.
+
+        Bounded, not exhaustive: the walk stops after ``_RECENT_SCAN_ROW_CAP``
+        rows, so a filter that matches almost nothing in a huge table returns
+        fewer than ``limit`` rows rather than reading the table. Because the
+        walk is newest-first, what it does return is still the newest matching
+        rows within the newest ``_RECENT_SCAN_ROW_CAP`` records — a much
+        tighter approximation than the base class's storage-order window, but
+        an approximation, unlike the pushdown branch which is exact at any
+        table size.
+        """
+        # ``limit`` is ``int`` in the contract; the ``None`` the caller's
+        # guard tolerates means "no bound", which on this branch is the cap.
+        target = _RECENT_SCAN_ROW_CAP if limit is None else int(limit)
+        page_size = max(_RECENT_SCAN_PAGE_MIN, min(target, _RECENT_SCAN_PAGE_MAX))
+        matched: list[dict] = []
+        seen: set[str] = set()
+        offset = 0
+        scanned = 0
+        while len(matched) < target and scanned < _RECENT_SCAN_ROW_CAP:
+            want = min(page_size, _RECENT_SCAN_ROW_CAP - scanned)
+            page = self._scroll(
+                where=None,
+                with_embedding=with_embedding,
+                with_document=with_document,
+                limit=want,
+                offset=offset or None,
+                order_field=order_field,
+            )
+            if not page:
+                break
+            scanned += len(page)
+            offset += len(page)
+            for row in page:
+                if row["id"] in seen:
+                    continue
+                seen.add(row["id"])
+                if _matches_where(row["metadata"], where):
+                    matched.append(row)
+                    if len(matched) >= target:
+                        break
+            if len(page) < want:
+                break  # short page — end of table
+        return matched[:target]
+
     def get_recent(self, *, limit, where=None, order_field="filed_at", include=None):
         """Newest-first fetch with the ordering pushed into SQL.
 
         The base implementation scans a window in storage order and sorts it
         locally, so on a collection larger than ``limit`` the genuinely newest
         records can be missing from the window entirely (#1630). Postgres can
         do the whole thing: ``ORDER BY metadata->>'filed_at' DESC ... LIMIT n``
-        is exact at any table size, which is why this backend advertises
-        ``supports_recency_order``.
-
-        Filters that ``metadata @> ...`` cannot express exactly fall back to
-        the local post-filter path (same correctness contract as ``get``), and
-        there the SQL order is still applied before the filter, so the result
-        is the newest ``limit`` rows *that match* only when the pushdown was
-        exact. The inexact case re-sorts locally after filtering.
+        picks the true top ``limit`` under that ordering at any table size,
+        which is why this backend advertises ``supports_recency_order``.
+
+        Filters that ``metadata @> ...`` cannot express exactly ($or,
+        comparisons, ...) keep the local post-filter contract that ``get``
+        uses, but they do *not* fetch the table to do it: the ordering is
+        still pushed into SQL and :meth:`_scroll_recent_local` walks the
+        result newest-first a page at a time, stopping as soon as ``limit``
+        rows match.
+
+        **This is the one case where ``supports_recency_order`` is weaker than
+        it sounds.** That walk is capped, so a non-pushdown filter matching
+        very little in a very large table returns fewer than ``limit`` records
+        rather than reading the table. The token covers the filters this
+        backend can push down, which is every filter Layer 1 uses (``None`` or
+        ``{"wing": ...}``) and everything built from ``$eq``/``$ne``/``$in``/
+        ``$nin``/``$and``. See :meth:`_scroll_recent_local` for the bound and
+        for what the capped answer still guarantees.
+
+        ``limit=None`` is outside the contract (the signature is ``int``). The
+        pushdown branch treats it as unbounded; the local branch cannot, and
+        treats it as the scan cap.
+
+        Ordering is on the JSON *text* of ``order_field``, matching the text
+        ordering :func:`recency_sort_key` already applies in Layer 1. See that
+        function for what text ordering does and does not promise; this method
+        inherits those limits rather than introducing them. At least three
+        places where the SQL order and ``recency_sort_key`` differ, none of
+        them reachable through anything that writes ``filed_at``:
+
+        * a JSON value that is not a string sorts by its text form here but
+          sorts last there;
+        * an empty string sorts above SQL NULL here but ties with a missing
+          key there;
+        * both ``metadata->>%s`` and the ``id`` tiebreak sort under the
+          database collation, while ``recency_sort_key`` sorts by Python
+          codepoint. Under a collation such as ``en_US.UTF-8`` punctuation is
+          weighted differently, so the two can disagree on timestamps that
+          differ only in punctuation (``+00:00`` against ``Z``). The test
+          double emulates the ordering in Python and so cannot catch this.
         """
         if limit is not None and limit <= 0:
             return GetResult.empty()
         _validate_where(where)
         spec = _IncludeSpec.resolve(include, default_distances=False)
-        local_filter = _requires_local_filter(where)
-        rows = self._scroll(
-            where=None if local_filter else where,
-            with_embedding=spec.embeddings,
-            limit=None if local_filter else limit,
-            order_field=order_field,
-        )
-        if local_filter:
-            rows = [row for row in rows if _matches_where(row["metadata"], where)][:limit]
+        if _requires_local_filter(where):
+            rows = self._scroll_recent_local(
+                where=where,
+                limit=limit,
+                order_field=order_field,
+                with_embedding=spec.embeddings,
+                # The post-filter reads only ``metadata``, and this branch can
+                # scan far more rows than it returns, so project the document
+                # text out unless the caller actually asked for it (#1840's
+                # wire-byte win). The pushdown branch below is left alone: it
+                # fetches ``limit`` rows, so the projection is worth little
+                # there and not worth changing that path's SQL for. (It would
+                # still pay off for ``limit=None``, which is outside the
+                # contract; fold it in when that path grows a real caller.)
+                with_document=spec.documents,
+            )
+        else:
+            rows = self._scroll(
+                where=where,
+                with_embedding=spec.embeddings,
+                limit=limit,
+                order_field=order_field,
+            )
         return GetResult(
             ids=[row["id"] for row in rows],
             documents=[row["document"] for row in rows] if spec.documents else [],

tests/test_backends.py

@@ -556,6 +556,58 @@ def test_base_get_recent_default_passes_where_and_zero_limit():
     assert _recent(col, limit=0).ids == []
 
 
+def test_base_get_recent_default_honours_include_projection():
+    """Unrequested projections come back empty, as they do from ``get``.
+
+    ``metadatas`` is fetched from the backend regardless because the local
+    sort reads ``order_field`` out of it, but it is only returned when the
+    caller asked for it. Without that, a backend without recency pushdown
+    would answer ``include=["metadatas"]`` with a list of padding strings
+    while pgvector answers with ``[]``.
+    """
+
+    class _ProjectingCollection:
+        """Honours ``include`` the way the real backends do."""
+
+        def __init__(self):
+            self.calls = []
+
+        def get(self, *, include=None, limit=None, offset=None, **kwargs):
+            self.calls.append(list(include or []))
+            if offset:
+                return GetResult(ids=[], documents=[], metadatas=[])
+            keys = set(include or [])
+            return GetResult(
+                ids=["a", "b"],
+                documents=["older", "newer"] if "documents" in keys else [],
+                metadatas=(
+                    [
+                        {"filed_at": "2024-01-01T00:00:00Z"},
+                        {"filed_at": "2026-01-01T00:00:00Z"},
+                    ]
+                    if "metadatas" in keys
+                    else []
+                ),
+            )
+
+    col = _ProjectingCollection()
+    page = _recent(col, limit=5, include=["metadatas"])
+    assert page.ids == ["b", "a"]
+    assert page.documents == []
+    assert page.metadatas == [
+        {"filed_at": "2026-01-01T00:00:00Z"},
+        {"filed_at": "2024-01-01T00:00:00Z"},
+    ]
+
+    col = _ProjectingCollection()
+    page = _recent(col, limit=5, include=["documents"])
+    # metadatas are fetched anyway so the sort has order_field to read...
+    assert "metadatas" in col.calls[0]
+    # ...which is why the newest document leads, but they are not returned.
+    assert page.documents == ["newer", "older"]
+    assert page.metadatas == []
+
+
 def test_base_get_recent_default_accepts_dict_shaped_get():
     """Collections still returning Chroma-shaped dicts page correctly."""