Keep branch period reads, dumps, and disk deletion consistent

changelog.d/fix-uprating-branch-visibility.fixed.md

@@ -0,0 +1 @@
+Scope known periods and dumps to the branch's readable values while preserving derived marks, leave root uprating and carry-over fallback defaults uncached through period conversions and even after branch mutations converge so later inputs take effect, return inputs supplied during conversion on the first call, parse underscore-containing disk branch names safely, delete contained disk periods consistently with memory, reject ambiguous or unparseable storage keys before writing, and warn and skip malformed restored filenames.

docs/python_api/data_storage.md

@@ -31,6 +31,20 @@ writing when its existing values must remain isolated.
     :show-inheritance:
 ```
 
+## Storage keys
+
+Both storage backends reject branch names containing `:` and month or year
+periods anchored after the first day of a month. They also reject periods whose
+string form cannot be parsed back, such as the year `0999`, before storing a
+value. Existing aliases, including twelve-month periods serialized as one year,
+remain supported. Disk restoration warns and skips filenames without a branch
+separator and a canonical period string.
+
+```{eval-rst}
+.. automodule:: policyengine_core.data_storage.storage_keys
+    :members:
+```
+
 ## TemporaryStorageDirectory
 
 A caller-supplied `simulation._data_storage_dir` is created on first use if it

docs/usage/branches.md

@@ -237,7 +237,15 @@ depend on the input is calculated again) but not less, within these limits:
   input. Input helpers still remove overlapping calculated values of the
   variable they write. A formula's input for its own period is still returned
   as described above, and results calculated while an input is being set do not
-  enter the fast cache.
+  enter the fast cache. When uprating or automatic carry-over finds no source
+  input, the root returns the fallback default without caching it, including
+  when repeated branch mutations converge to a fixed point. Period conversion
+  preserves this rule: a divided, summed or stock-selected value containing
+  such a fallback also stays uncached. If a formula supplies an input for the
+  requested period during conversion, that input is returned on the first
+  call, before deciding whether to cache the converted result. Setting an
+  earlier input can therefore change the next calculation of the requested period.
+  Branches can cache these defaults because later inputs invalidate them.
 - **Existing child branches keep their values.** An input set on a branch does
   not reach branches already created from it.
 - **Values kept elsewhere stay.** A value one simulation calculated from
@@ -276,9 +284,9 @@ say what each value was calculated from, nor what was read without being kept,
 so the restored simulation records that values from that number on may depend
 on anything: an input set for any variable on a branch of it drops all of them.
 A dump written before calculated values were recorded is restored with every
-value as an input, as before, so such values never drop. With disk storage, a branch whose
-name contains `_` cannot be dumped yet: `OnDiskStorage.get_known_periods`
-splits its keys on every `_` (as on master).
+value as an input, as before, so such values never drop. Disk storage splits
+keys at the last underscore, so branch names containing `_` also round-trip
+through dumps and restoration.
 
 Two related behaviours: a branch stops reading macro-cache files once an input
 is set on it, even one it already reads, since they are keyed by branch name and

policyengine_core/data_storage/in_memory_storage.py

@@ -4,6 +4,7 @@
 from numpy.typing import ArrayLike
 
 from policyengine_core import periods
+from policyengine_core.data_storage.storage_keys import check_storable
 from policyengine_core.data_storage.store_history import (
     _period_from_storage_key,
     advance_sequence_past,
@@ -289,20 +290,8 @@ def put(
         period = periods.period(period)
 
         # Keys embed f"{branch_name}:{period}", so reject inputs whose key
-        # would be ambiguous to parse back: branch names containing the
-        # separator, and month/year periods anchored mid-month (their string
-        # form drops the day). See policyengine-core#526 for the structured-
-        # key refactor that will lift these restrictions.
-        if ":" in branch_name:
-            raise ValueError(
-                f"Branch name {branch_name!r} may not contain ':' "
-                "(see policyengine-core#526)."
-            )
-        if period.unit in (periods.YEAR, periods.MONTH) and period.start.day != 1:
-            raise ValueError(
-                f"Cannot cache period {period} anchored mid-month: its "
-                "string form is lossy (see policyengine-core#526)."
-            )
+        # would be ambiguous to parse back, as ``OnDiskStorage.put`` does.
+        check_storable(branch_name, period)
         key = f"{branch_name}:{period}"
         self._arrays[key] = value
         self._stop_sharing(key)

policyengine_core/data_storage/on_disk_storage.py

@@ -1,6 +1,7 @@
 import os
 import shutil
 import tempfile
+import warnings
 import weakref
 from typing import Dict, List, Optional, Set, Tuple
 
@@ -17,6 +18,7 @@
     directory_containing,
     path_key,
 )
+from policyengine_core.data_storage.storage_keys import check_storable
 from policyengine_core.enums import EnumArray
 from policyengine_core.periods import Period
 
@@ -54,6 +56,41 @@ def _epoch() -> tuple:
     return os.getpid(), _forks
 
 
+def _split_key(key: str) -> tuple:
+    """Split a ``f"{branch_name}_{period}"`` file key into its two parts.
+
+    Branch names often contain ``_`` (policyengine-us uses ``no_salt`` and
+    ``mtr_for_adult_1``) but a period's string form never does, so the key
+    splits on its last ``_``.
+    """
+    branch_name, period = key.rsplit("_", 1)
+    return branch_name, period
+
+
+def _is_key(key: str) -> bool:
+    """Whether ``key`` ends with a canonical period after the branch separator.
+
+    Legacy branch names and eternal-storage files retain their restore
+    behavior; this check only validates the period's spelling.
+    """
+    _, separator, period = key.rpartition("_")
+    if not separator:
+        return False
+    try:
+        return str(periods.period(period)) == period
+    except ValueError:
+        return False
+
+
+def _is_within(key: str, branch_name: str, period: Period) -> bool:
+    """Whether file key ``key`` stores a value of ``branch_name`` for a
+    period that ``period`` contains."""
+    key_branch_name, key_period = _split_key(key)
+    return key_branch_name == branch_name and period.contains(
+        periods.period(key_period)
+    )
+
+
 class OnDiskStorage:
     """
     Low-level class responsible for storing and retrieving calculated vectors on disk
@@ -303,6 +340,11 @@ def put(
         if self.is_eternal:
             period = periods.period(periods.ETERNITY)
         period = periods.period(period)
+        # The key must split back into this branch name and period (see
+        # ``get_known_branch_periods`` and ``delete``): reject what
+        # ``InMemoryStorage.put`` rejects. A month anchored mid-month, say,
+        # would be stored under the key of the calendar month.
+        check_storable(branch_name, period)
 
         filename = f"{branch_name}_{period}"
         path = self._path_to_write(filename)
@@ -417,12 +459,13 @@ def delete(self, period: Period = None, branch_name: str = "default") -> None:
         if period is None:
             # Only wipe files belonging to the requested branch (previously
             # this wiped every branch regardless of ``branch_name`` — same
-            # class of bug as C2 in InMemoryStorage).
-            branch_prefix = f"{branch_name}_"
+            # class of bug as C2 in InMemoryStorage). Compare the parsed
+            # branch name, not a prefix: deleting ``pre_tcja`` must not
+            # also wipe ``pre_tcja_ctc``.
             self._files = {
                 period_item: value
                 for period_item, value in self._files.items()
-                if not period_item.startswith(branch_prefix)
+                if _split_key(period_item)[0] != branch_name
             }
             self._forget_dropped_keys()
             return
@@ -431,27 +474,34 @@ def delete(self, period: Period = None, branch_name: str = "default") -> None:
             period = periods.period(periods.ETERNITY)
         period = periods.period(period)
 
+        # Delete every period of the branch that ``period`` contains, as
+        # ``InMemoryStorage.delete`` does and ``Holder.delete_arrays``
+        # documents: deleting ``2025`` also deletes ``2025-01``. Previously
+        # only the file keyed exactly ``period`` was deleted.
         if period is not None:
             self._files = {
                 period_item: value
                 for period_item, value in self._files.items()
-                if not period_item == f"{branch_name}_{period}"
+                if not _is_within(period_item, branch_name, period)
             }
             self._forget_dropped_keys()
 
     def get_known_periods(self) -> list:
-        return list([periods.period(x.split("_")[1]) for x in self._files.keys()])
+        return [period for _, period in self.get_known_branch_periods()]
 
     def get_known_branch_periods(self) -> list:
         return [
             (branch_name, periods.period(period))
-            for branch_name, period in map(lambda x: x.split("_"), self._files.keys())
+            for branch_name, period in map(_split_key, self._files.keys())
         ]
 
     def restore(self) -> None:
         """Read back the values stored in this storage's directory: for each
         key, the file named for it (not those in ``REPLACEMENTS_DIR``).
 
+        A ``.npy`` file whose name lacks a branch separator and a canonical
+        period string is left out, with a warning.
+
         This storage writes over portable files, as over those it wrote, until
         it is cloned or copied, or this process forks: from then on it writes
         a new file instead, since the clone, copy or forked process reads
@@ -471,6 +521,7 @@ def restore(self) -> None:
         self._files = files = {}
         # Files read back from a directory carry no derived marks or numbers.
         self._derived = set()
+        not_keys = []
         self._sequence_numbers = {}
         # Restore self._files from content of storage_dir.
         for filename in os.listdir(self.storage_dir):
@@ -482,12 +533,25 @@ def restore(self) -> None:
             key = filename_core
             if separator:
                 key = f"{branch}{separator}{period_string.replace(';', ':')}"
+            if not _is_key(key):
+                # Reading it back would put a key in ``_files`` that names no
+                # branch and period, which ``delete`` and
+                # ``get_known_periods`` could not parse.
+                not_keys.append(filename)
+                continue
             # Updating a legacy value may leave its old colon filename
             # beside the portable file. Prefer the latter, independent of
-            # directory listing order, without parsing arbitrary basenames.
+            # directory listing order.
             if key not in files or key != filename_core:
                 files[key] = path
         self._restored_paths.update(files.values())
+        if not_keys:
+            warnings.warn(
+                f"Not restoring {', '.join(sorted(not_keys))} in "
+                f"{self.storage_dir}: a stored value's file is named for a "
+                "branch name, '_' and a period.",
+                stacklevel=2,
+            )
 
     def __del__(self, _rmtree=shutil.rmtree, _getpid=os.getpid) -> None:
         # (The defaults keep the two functions reachable while the

policyengine_core/data_storage/storage_keys.py

@@ -0,0 +1,43 @@
+"""What a storage key can hold.
+
+Both storages key a value by its branch name and its period's string form:
+``f"{branch_name}:{period}"`` in memory, ``f"{branch_name}_{period}"`` on disk
+(the name of the value's file). ``check_storable`` enforces the same branch
+and period serialization restrictions in both storages (``Holder._set``
+chooses between them by how much memory is in use).
+"""
+
+from policyengine_core import periods
+from policyengine_core.periods import Period
+
+
+def check_storable(branch_name: str, period: Period) -> None:
+    """Raise ``ValueError`` for an ambiguous branch or period serialization.
+
+    Reject a branch name containing ``:``, which separates the parts of an
+    in-memory key, and a year or month anchored mid-month, whose string form
+    drops the day (``month:2025-03-15`` is ``2025-03``), so that its key would
+    be the key of another period. Reject unparseable period strings too,
+    including years below 1000 whose string form loses leading zeroes.
+    Supported aliases, such as twelve months serialized as one year, retain
+    their existing meaning. See policyengine-core#526 for the structured-key
+    refactor.
+    """
+    if ":" in branch_name:
+        raise ValueError(
+            f"Branch name {branch_name!r} may not contain ':' "
+            "(see policyengine-core#526)."
+        )
+    if period.unit in (periods.YEAR, periods.MONTH) and period.start.day != 1:
+        raise ValueError(
+            f"Cannot cache period {period} anchored mid-month (it starts on "
+            f"{period.start}): its string form is lossy (see "
+            "policyengine-core#526)."
+        )
+    try:
+        periods.period(str(period))
+    except ValueError as error:
+        raise ValueError(
+            f"Cannot cache period {period!r}: its string form cannot be parsed "
+            "(see policyengine-core#526)."
+        ) from error

policyengine_core/holders/holder.py

@@ -538,11 +538,25 @@ def get_memory_usage(self) -> dict:
 
         return usage
 
-    def get_known_periods(self) -> List[Period]:
+    def get_known_periods(self, branch_name: str = None) -> List[Period]:
         """
         Get the list of periods the variable value is known for.
-        """
 
+        With ``branch_name``, list each period ``get_array(period,
+        branch_name)`` reads a stored value for, once: those stored under
+        that branch, its ``parent_branch`` ancestors or ``default`` (see
+        ``_readable_branches``). Without it, list the period of every value
+        stored, under any branch; that branch may not read some of them.
+        """
+        if branch_name is not None:
+            readable = set(self._readable_branches(branch_name))
+            return list(
+                dict.fromkeys(
+                    period
+                    for stored_branch, period in self.get_known_branch_periods()
+                    if stored_branch in readable
+                )
+            )
         return list(self._memory_storage.get_known_periods()) + list(
             (self._disk_storage.get_known_periods() if self._disk_storage else [])
         )