Close two edges left by the re-review: unloaded cache files, sortless ordering

1. reload() only loads countsfile and statsfile under stats.saving, but
   stats_fresh was read off the filenames alone.  So on a table without
   saving, both files were accepted, neither was loaded or swapped, and
   meta_tables.stats_valid was still set true -- leaving the live cache rows,
   which describe the data being replaced, eligible to be served.  Reachable
   by toggling saving off before a reload, or simply by reloading from a
   process using the base saving=False setting after counts were recorded.

   Freshness now asks whether both companions are in the swap as well as
   whether both files were named, which is what actually decides what goes
   live.  The partial-file and manual reload_final_swap cases stay
   conservative as before.  A reload handed cache files it will not load now
   says so rather than dropping them silently; it is left as a warning rather
   than an error, since rejecting outright would change an accepted call into
   a failure mid-release-candidate.

2. _metafile_order_state maps a SQL-null sort to None, and _generate_sorted_ids
   reads sort=None as "use the configured sort".  A metafile with
   id_ordered=true, out_of_order=false and sort=NULL therefore numbered the
   _tmp relation by the sort it was replacing, after which reload_meta
   installed no sort and _set_ordered recorded an ordered table: exactly the
   id_ordered-with-no-sort state the audit reports as an error.

   reload now refuses that combination, with a message saying what to do
   instead, before anything is rebuilt or swapped -- so the live table is
   untouched and no _tmp or _resort relation is left behind.  The check reads
   the metafile whether or not this reload renumbers, since the non-resorting
   path installs the same impossible row through reload_meta.  Only the
   incoherent combination is refused: a metafile that drops the sort and the
   id_ordered claim together still loads.

Regression tests for both, plus the positive controls (a partial cache pair
is still not trusted; a sortless metafile that also clears id_ordered still
loads).  Each new test was checked to fail against a39070f.  Full suite green
(1405 passed) over three clean-database runs, ruff clean, docs clean under -W.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: roed314

CHANGELOG.md

@@ -448,11 +448,22 @@ hardening standalone use; the highlights:
   beside changed data) now mark the table invalid, in the same transaction as
   the swap. A `metafile` no longer installs a `stats_valid = true` describing
   the database it was exported from. Without this a count cached before such a
-  write was still served afterwards, defeating the enforcement above. The rule
-  is deliberately conservative in one place: `resort()` leaves the rows
-  unchanged, so the caches would still be accurate, but on a table without
-  `saving` nothing rebuilds them and it ends invalid. *Migration:* run
+  write was still served afterwards, defeating the enforcement above. A
+  `countsfile`/`statsfile` pair counts as fresh only when it was actually
+  loaded and swapped in, which needs `stats.saving`; a reload that is handed
+  cache files it will not load now says so rather than dropping them silently.
+  The rule is deliberately conservative in one place: `resort()` leaves the
+  rows unchanged, so the caches would still be accurate, but on a table
+  without `saving` nothing rebuilds them and it ends invalid. *Migration:* run
   `refresh_stats()` after a `resort()` on such a table.
+- **A `metafile` claiming to be ordered must install a sort.** `reload` now
+  refuses one carrying `id_ordered = true`, `out_of_order = false` and no
+  sort: there is nothing for ascending `id` to follow, so the flags assert an
+  invariant that cannot hold. Previously the rows were numbered by the sort
+  the file was *replacing* and the table went live in exactly the state the
+  audit reports as an error. The check runs before anything is rebuilt or
+  swapped, so a rejected reload leaves the live table untouched. *Migration:*
+  give such a metafile a sort, or let it record `out_of_order = true`.
 - **`scripts/audit_id_order.py`**, a read-only check that streams each
   `id_ordered` table in sort order and reports whether its ids actually
   increase, since the flag can drift. Each table is audited in a read

DataManagement.md

@@ -142,12 +142,15 @@ and the staged swap — decide the flag in one place, in the same transaction as
 the swap.  The table ends up valid only when the counts and stats that go live
 were built from the data that goes live with them: either recomputed here
 (`restat`, on a table with `saving` on) or supplied whole by the export
-(`reload` given **both** `countsfile` and `statsfile`).  Anything else — a
-`restat=False` write, a partial cache set, companions that were merely cloned
-or left in place beside replaced rows — is marked invalid.  A `metafile`
-carries a `stats_valid` of its own describing the database it was exported
-from; it does not vouch for caches the reload did not install, and is
-overridden to false with a message when it disagrees.
+(`reload` given **both** `countsfile` and `statsfile`, and loading them —
+which also needs `saving`, since that is what puts the companions in the
+swap; a reload handed cache files it will not load says so rather than
+dropping them silently).  Anything else — a `restat=False` write, a partial
+cache set, companions that were merely cloned or left in place beside
+replaced rows — is marked invalid.  A `metafile` carries a `stats_valid` of
+its own describing the database it was exported from; it does not vouch for
+caches the reload did not install, and is overridden to false with a message
+when it disagrees.
 
 Bulk paths also run PostgreSQL's own `ANALYZE`, which is a different thing from
 psycodict's statistics: a freshly loaded relation has no planner statistics
@@ -197,6 +200,15 @@ tell that it did not move a sort key.  A staged write is not allowed to rebuild
 inside the staged context, but it carries that fact through the commit, so the
 swapped-in table is marked out of order and `resort()` afterwards puts it right.
 
+A `metafile` replaces the order flags and the sort together, so a reload
+carrying one numbers the rows by the sort *that file* installs, not the one it
+is replacing.  A metafile asking for `id_ordered = true` with
+`out_of_order = false` and no sort is refused: there would be nothing for
+ascending `id` to follow, so the flags would assert an invariant that cannot
+hold.  The check runs before anything is rebuilt or swapped, so the live table
+is left untouched.  To remove a sort, let the metafile record
+`id_ordered = false` as well.
+
 `scripts/audit_id_order.py` checks the flag against reality: it streams each
 `id_ordered` table in sort order and reports `OK`, `MISMATCH` or an error
 without modifying anything.  Each table is audited in a read transaction of its

psycodict/table.py

@@ -2412,15 +2412,20 @@ def reload(
 
         - ``searchfile`` -- a string, the file with data for the search table
         - ``countsfile`` -- a string (optional), giving a file containing counts
-            information for the table.
+            information for the table.  Loaded only when the table saves
+            statistics; otherwise it is reported and ignored.
         - ``statsfile`` -- a string (optional), giving a file containing stats
-            information for the table.
+            information for the table.  Same condition as ``countsfile``.
         - ``indexesfile`` -- a string (optional), giving a file containing index
             information for the table.
         - ``constraintsfile`` -- a string (optional), giving a file containing constraint
             information for the table.
         - ``metafile`` -- a string (optional), giving a file containing the meta
-            information for the table.
+            information for the table.  It replaces the sort along with the
+            order flags, so the ids are numbered by the sort it installs; one
+            claiming ``id_ordered`` and not ``out_of_order`` while installing
+            no sort is refused, since ascending id would have nothing to
+            follow.
         - ``resort`` -- whether to sort the ids after copying in the data.
             Only relevant for tables that are id_ordered.  Defaults to sorting
             when the searchfile does not contain ids.
@@ -2471,6 +2476,18 @@ def reload(
                     (self.stats.counts, _counts_cols, False, countsfile),
                     (self.stats.stats, _stats_cols, False, statsfile),
                 ])
+            elif countsfile is not None or statsfile is not None:
+                # Only a saving table loads its companions, so these are
+                # dropped on the floor.  Say so: silently ignoring a file the
+                # caller named looks like it worked, and the table keeps
+                # whatever counts it already had.
+                print(
+                    "Warning: %s does not save statistics (stats.saving is "
+                    "False), so the counts/stats files given are not loaded "
+                    "and the existing cached statistics are left in place; "
+                    "they are marked invalid, since they describe the data "
+                    "being replaced." % (self.search_table,)
+                )
             addedid = None
             with DelayCommit(self, silence=True):
                 for table, cols, header, filename in tabledata:
@@ -2498,19 +2515,37 @@ def reload(
                 # Renumber ids in sort order before the keys are built: the
                 # renumber replaces the _tmp table, so it must run while that
                 # table still has no primary key or indexes to rebuild.
+                #
+                # A metafile replaces the order flags *and* the sort, so what it
+                # will install decides both whether to renumber and what to
+                # renumber by -- resort_sort, not the sort currently configured,
+                # since numbering the rows by a sort the table is about to stop
+                # having would install ids that satisfy nothing while claiming
+                # to be ordered.  Read whether or not this reload renumbers, so
+                # that the check below also covers a metafile installed without
+                # one.
                 resort_sort = None
-                if resort:
-                    if metafile:
-                        id_ordered, out_of_order, resort_sort = self._metafile_order_state(
-                            metafile, sep=sep
+                if metafile:
+                    meta_ordered, meta_out_of_order, resort_sort = self._metafile_order_state(
+                        metafile, sep=sep
+                    )
+                    if meta_ordered and not meta_out_of_order and not resort_sort:
+                        # Ascending id cannot follow a sort the table does not
+                        # have.  Left alone this numbers the rows by the sort
+                        # being replaced and then installs the combination the
+                        # id-order audit reports as an error.  Raised inside
+                        # the DelayCommit, before anything is rebuilt or
+                        # swapped, so the live table is untouched.
+                        raise ValueError(
+                            "%s claims id_ordered = true and out_of_order = false "
+                            "but installs no sort, so there is nothing for "
+                            "ascending id to follow.  Give the metafile a sort, "
+                            "or let it record out_of_order = true." % (metafile,)
                         )
-                        resort = id_ordered and not out_of_order
-                        # resort_sort, not the sort currently configured: the
-                        # metafile replaces that one, and numbering the rows by
-                        # a sort the table is about to stop having would install
-                        # ids that satisfy nothing while claiming to be ordered.
-                    elif not self._id_ordered:  # this table doesn't need to be sorted
-                        resort = False
+                    resort = bool(resort) and meta_ordered and not meta_out_of_order
+                elif resort and not self._id_ordered:
+                    # this table doesn't need to be sorted
+                    resort = False
                 if resort:
                     self._generate_sorted_ids(suffix=suffix, sort=resort_sort)
                     ordered = True
@@ -2549,14 +2584,26 @@ def reload(
 
                 # The replacement's caches agree with its data in exactly two
                 # cases: the export supplied both of them alongside the search
-                # file, or they were recomputed here from what was loaded.
-                # Otherwise the counts and stats that go live are the ones
-                # already there (untouched, since they are not in ``tables``)
-                # or an empty or reused ``_tmp`` clone -- in neither case a
-                # description of the rows now in the table.
-                stats_fresh = (countsfile is not None and statsfile is not None) or (
-                    self.stats.saving and restat
+                # file *and* they were loaded and are being swapped in with it,
+                # or they were recomputed here from what was loaded.  Otherwise
+                # the counts and stats that go live are the ones already there
+                # (untouched, since they are not in ``tables``) or an empty or
+                # reused ``_tmp`` clone -- in neither case a description of the
+                # rows now in the table.
+                #
+                # Asking for membership of ``tables`` rather than just for the
+                # filenames is the point: the companions are only loaded under
+                # ``stats.saving``, so on a table without it both files are
+                # accepted and silently ignored, and trusting the filenames
+                # alone marked the live caches -- which still describe the old
+                # data -- as agreeing with the new.
+                loaded_both_caches = (
+                    countsfile is not None
+                    and statsfile is not None
+                    and self.stats.counts in tables
+                    and self.stats.stats in tables
                 )
+                stats_fresh = loaded_both_caches or (self.stats.saving and restat)
                 if final_swap:
                     self.reload_final_swap(tables=tables,
                                            metafile=metafile,

tests/test_resort.py

@@ -379,3 +379,113 @@ def test_a_metafile_sort_may_be_a_direction_pair(db, two_col_table, tmp_path):
     from test_id_order_audit import audit
 
     assert audit.audit_table(db, name)[0] == "OK"
+
+
+def _leftover_relations(table):
+    """
+    Any _tmp or _resort relation belonging to this table.
+    """
+    name = table.search_table
+    return [
+        t for t in table._all_tablenames()
+        if t.startswith(name) and (t.endswith("_tmp") or t.endswith("_resort"))
+    ]
+
+
+def test_a_metafile_may_not_claim_order_without_a_sort(db, two_col_table, tmp_path):
+    """
+    ``id_ordered = true`` with ``out_of_order = false`` and no sort describes
+    an invariant nothing can satisfy: there is no sort for ascending id to
+    follow.  Left alone the reload numbers the rows by the sort the metafile is
+    *replacing* and then installs the combination the id-order audit reports as
+    an error, so refuse it up front.
+    """
+    name = two_col_table.search_table
+    before = list(two_col_table.search({}, projection=["n", "label"], sort=[["n", 1]]))
+    was_out_of_order = two_col_table._out_of_order
+    path = _reload_file(
+        tmp_path / "d.txt",
+        ["n", "label"],
+        ["integer", "text"],
+        [[str(i), "l%d" % i] for i in ROW_ORDER],
+    )
+    metafile = _metafile_with_sort(two_col_table, tmp_path, "\\N")
+
+    with pytest.raises(ValueError, match="no sort"):
+        two_col_table.reload(path, metafile=metafile, resort=True)
+
+    # nothing was installed, rebuilt or left half-built
+    table = db[name]
+    assert table._sort_orig == ["n"]
+    assert table._id_ordered is True
+    assert table._out_of_order is was_out_of_order
+    assert list(table.search({}, projection=["n", "label"], sort=[["n", 1]])) == before
+    assert _leftover_relations(table) == []
+
+
+def test_the_refusal_does_not_depend_on_resorting(db, two_col_table, tmp_path):
+    """
+    The renumbering is only half of it: without one, reload_meta installs the
+    same impossible row.  A search file carrying ids resorts nothing, and the
+    metafile must still be refused.
+    """
+    name = two_col_table.search_table
+    path = str(tmp_path / "d.txt")
+    two_col_table.copy_to(path)
+    metafile = _metafile_with_sort(two_col_table, tmp_path, "\\N")
+
+    with pytest.raises(ValueError, match="no sort"):
+        two_col_table.reload(path, metafile=metafile, resort=False)
+
+    table = db[name]
+    assert table._sort_orig == ["n"]
+    assert _leftover_relations(table) == []
+
+
+def test_a_metafile_may_drop_the_sort_when_it_drops_the_ordering(db, two_col_table, tmp_path):
+    """
+    The coherent way to remove a sort: stop claiming to be id_ordered.  Only
+    the impossible combination is refused, not every sortless metafile.
+    """
+    from psycodict.base import _meta_tables_cols
+
+    name = two_col_table.search_table
+    path = str(tmp_path / "d.txt")
+    two_col_table.copy_to(path)
+    metafile = tmp_path / "meta.txt"
+    two_col_table.copy_to_meta(str(metafile))
+    fields = open(metafile).read().rstrip("\n").split("|")
+    fields[_meta_tables_cols.index("sort")] = "\\N"
+    fields[_meta_tables_cols.index("id_ordered")] = "f"
+    fields[_meta_tables_cols.index("out_of_order")] = "f"
+    with open(metafile, "w") as F:
+        F.write("|".join(fields) + "\n")
+
+    two_col_table.reload(path, metafile=str(metafile), resort=True)
+
+    table = db[name]
+    assert table._sort_orig is None
+    assert table._id_ordered is False
+    assert table.count() == len(ROW_ORDER)
+
+
+def test_reload_never_installs_a_state_the_audit_calls_an_error(db, two_col_table, tmp_path):
+    """
+    The invariant behind the refusal, stated in the audit's own terms.
+    """
+    from test_id_order_audit import audit
+
+    two_col_table.resort()
+    name = two_col_table.search_table
+    assert audit.audit_table(db, name)[0] == "OK"
+
+    table = db[name]
+    path = str(tmp_path / "d.txt")
+    table.copy_to(path)
+    metafile = _metafile_with_sort(table, tmp_path, "\\N")
+    with pytest.raises(ValueError):
+        table.reload(path, metafile=metafile, resort=True)
+
+    # Had the metafile gone in, the table would be id_ordered with no sort,
+    # which the audit reports as an error rather than as an ordering.
+    assert audit.audit_table(db, name)[0] == "OK"

tests/test_stats_validity.py

@@ -548,3 +548,57 @@ def test_resort_of_a_saving_table_stays_valid(db, counted):
     assert stats_valid_in_meta(table) is True
     assert table.stats.count(QUERY, record=True) == 67
     assert table.stats.quick_count(QUERY) == 67
+
+
+def test_cache_files_are_not_trusted_when_they_are_not_loaded(db, counted, tmp_path):
+    """
+    Only a table with ``saving`` on loads its counts and stats companions, so
+    on one without it both files are accepted and quietly ignored: the live
+    caches stay exactly as they were, describing the data being replaced.
+    Reading freshness off the filenames alone therefore marked caches for the
+    old rows as agreeing with the new ones.
+    """
+    name = counted.search_table
+    searchfile = tmp_path / "data.txt"
+    countsfile = tmp_path / "counts.txt"
+    statsfile = tmp_path / "stats.txt"
+    counted.copy_to(
+        str(searchfile), countsfile=str(countsfile), statsfile=str(statsfile)
+    )
+    # Keep the three header lines and five rows, so the cached 67 is no
+    # longer the answer to anything.
+    lines = open(searchfile).read().split("\n")
+    with open(searchfile, "w") as F:
+        F.write("\n".join(lines[:3] + lines[3:8]) + "\n")
+
+    counted.stats.saving = False
+    counted.reload(
+        str(searchfile),
+        countsfile=str(countsfile),
+        statsfile=str(statsfile),
+        restat=False,
+    )
+
+    table = db[name]
+    assert table.count() == 5
+    truth = table._execute(
+        SQL("SELECT count(*) FROM {0} WHERE flag").format(Identifier(name))
+    ).fetchone()[0]
+    assert truth != 67
+    assert stats_valid_in_meta(table) is False
+    assert table.stats.quick_count(QUERY) is None
+    assert table.count(QUERY) == truth
+
+
+def test_a_partial_cache_pair_is_still_not_trusted(db, counted, tmp_path):
+    """
+    The rule needs *both* companions: one file leaves the other an empty
+    clone, which is not the cache the data was exported with.
+    """
+    name = counted.search_table
+    searchfile = tmp_path / "data.txt"
+    countsfile = tmp_path / "counts.txt"
+    counted.copy_to(str(searchfile), countsfile=str(countsfile))
+
+    counted.reload(str(searchfile), countsfile=str(countsfile), restat=False)
+    assert stats_valid_in_meta(db[name]) is False