Skip to content

Give each disk-backed holder storage its own directory - #558

Draft
MaxGhenis wants to merge 8 commits into
masterfrom
fix-disk-storage-branch-ownership
Draft

MaxGhenis wants to merge 8 commits into
masterfrom
fix-disk-storage-branch-ownership

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Problem

With disk-backed holder storage (simulation.memory_config = MemoryConfig(...)), every simulation in a family (the root, its branches and its clone()s) wrote into one shared directory per variable, <data_storage_dir>/<variable>. Simulation.clone copies _data_storage_dir, and Holder.create_disk_storage made OnDiskStorage(<data_storage_dir>/<variable>, preserve_storage_dir=False), so the storage that created it owned it and removed it in __del__. OnDiskStorage.clone passed ownership along, but a holder a branch created itself (population.get_holder for a variable the parent had no holder for) made a second owner of the same path.

Confirmed on master (7950c01):

  1. Deleting a branch deletes the parent's values. The branch's holder created <dir>/income_tax, the root's holder then wrote <dir>/income_tax/default_2017-01.npy, and when the branch was garbage-collected (formulas routinely del simulation.branches[name]) its __del__ ran shutil.rmtree on the shared directory. The root then raised FileNotFoundError reading its own value.
  2. A clone overwrote its source's inputs. Simulation.clone() keeps branch_name "default", so clone.set_input("salary", "2017-01", [5]) overwrote the root's default_2017-01.npy: the root read 5 instead of 1,000. No error was raised. Simulation.derivative perturbs a clone()'s input this way, so with disk storage sim.derivative("income_tax", "salary", "2017-01", delta=100) returned 0 instead of 0.15 and left the simulation's own salary at 3,100 instead of 3,000.
  3. Same-named branches under different parents collided. sim.get_branch("first").get_branch("nested") and sim.get_branch("second").get_branch("nested") both wrote nested_2017-01.npy in the same directory, so the first read the second's salary (2 instead of 1).
  4. A branch saw its parent's later writes on disk, unlike in memory. In memory, a branch keeps the values its parent had when it was created. On disk, the parent overwriting a key in place changed what the branch read (7 instead of 1,000).
  5. Teardown errors and a missing parent directory. Two owners removing the same directory printed Exception ignored while calling deallocator ... FileNotFoundError. When the last storage in data_storage_dir went, its __del__ also removed that directory, so a simulation that still used it failed in os.mkdir when it next created a holder.

The new property test, run against master, shrinks to two minimal failing sequences: setting salary and then replacing the holders (FileNotFoundError), and calculating salary and then cloning (12 FileNotFoundErrors raised in finalizers).

Fix

  • Each storage writes only into a directory of its own. Holder.create_disk_storage() (no explicit directory) returns OnDiskStorage.temporary(variable, ...). That storage creates a uniquely named directory inside the simulation's data_storage_dir on its first write. OnDiskStorage.clone() returns a storage that reads the source's files through copied mappings and writes into its own new directory.

  • Readers are tracked per file. Each directory has one StorageDirectory handle per process. For every file, the handle records (in a WeakSet) the other live storages that may read it: clones, copies, and storages that restore()d the directory.

  • When a storage overwrites in place. Only when all of these hold:

    • it created the file itself, since the last fork (an os.register_at_fork counter; a file from before a fork may be read by the other process);
    • the file is the newest it created for that key;
    • no live reader is registered for the file;
    • outside its own temporary directory, the file is still the key's highest-numbered one.

    Otherwise it creates a new file with O_EXCL: {key}.npy if the key has no file yet, else {key}.{n}.npy numbered above every file for the key. So a storage never overwrites a file it did not create, and restore(), which maps each key to its highest-numbered file, finds the latest value. A simulation that never branches overwrites in place as before. A key whose file a live clone reads adds at most one more file, and once the clone is gone the storage overwrites in place again.

  • Directories are reference-counted. A StorageDirectory removes its directory in a weakref.finalize (with ignore_errors=True) once nothing references it. Each storage holds its own handle and those of every directory its mappings may point into, so a directory outlives every storage that can read it.

    • The simulation family shares the data_storage_dir handle (Simulation.clone copies it). Every storage inside it holds it as parent, including create_disk_storage(preserve=True)'s.
    • That directory is removed only once the family and all its storages are gone, and never while a simulation could still create a storage in it.
    • A data_storage_dir the user set is never removed; only the storages' own directories inside it are.
  • Preserving. preserve_storage_dir can still be set after construction. Preserving a directory also preserves the directories it is in, so a preserved storage survives its simulation family.

  • Forks. A finalizer removes a directory only in the process that created it. After a fork, neither process overwrites a file created before the fork. Readers in another process are not tracked otherwise.

  • Copies. copy and deepcopy of a storage share its directory handles, and unpickling in the same process finds the live handle, so a copy keeps the directory while it reads from it. A copy is registered as a reader and never overwrites the original's files. Storages copy and pickle on Python 3.14, which dropped pickling for itertools.count.

Invariants (each covered by tests)

  • Readable files exist. Every path in a live storage's _files exists on disk.
  • Isolation. A write through one storage never changes a value another live storage in the process reads. That covers clones, copies and storages that restored the same directory.
  • Disk matches memory. For the operations the property test generates, calculations return exactly what they return with values held in memory. Known pre-existing differences, outside this PR, are listed below.
  • Latest restore. restore() returns each key's latest value, given that every storage writing the directory goes through these rules.
  • Cleanup. Once a family and its storages are gone, its data_storage_dir is removed (unless something in it is preserved), and no finalizer raises.

New tests (helpers in tests/fixtures/disk_storage.py):

  • tests/core/test_disk_storage_ownership.py: 31 example tests.
    • Each failure above, including derivative, and derivative of a carried-over YEAR input (from the carry-over-order review's repro).
    • Overwriting in place until a clone reads the file, and again once the clone is gone.
    • Reforms after a derivative reusing files.
    • A live branch adding at most one file per key.
    • A clone keeping the directories it reads.
    • restore picking the latest version, including a branch name with ..
    • Writes after a restore leaving older readers and being restored next, by one or two writers.
    • A storage removing only its own directory.
    • Preserve, both explicit and inside the simulation's directory.
    • Enum; copy, deepcopy and pickle in either release order.
    • Forks: directories, and values in both directions.
    • A user-set data_storage_dir.
  • tests/core/test_disk_storage_differential.py: a Hypothesis property over random sequences of branch, clone, set_input (including an ETERNITY variable and an enum), calculate, derivative, apply_reform, delete_arrays, holder replacement and branch/clone drop.
    • It runs each sequence on a disk-backed and a memory-backed family.
    • After every step it compares the results and checks that every mapped file exists.
    • At the end it releases both families and checks that the directory is gone and no finalizer raised.
    • The module starts with pytest.importorskip("hypothesis"), because the "Test Core and country packages" smoke job installs no dev dependencies.

Against master, 26 of the 32 new tests fail. 18 of them fail on the behavior above, and 8 only because they use the new OnDiskStorage.temporary constructor. The 6 that pass on master are:

  • the memory-storage halves of the two parity tests;
  • the two original preserve tests;
  • the two new preserve tests, which guard against the regression the round-3 review found in an earlier version of this branch.

Against this branch's previous head (c7a0369), 10 of the new tests fail, one per round-3 finding. Two tests in test_branch_scoped_delete_arrays.py pinned the old internals (clone.storage_dir == storage.storage_dir, clone.preserve_storage_dir is True, _storage_dir_owner); I rewrote them for the new contract.

Not in this PR

Pre-existing, and the same on master:

  • Branch names with _. OnDiskStorage.get_known_periods / get_known_branch_periods split keys on every _, which breaks for names like no_salt. Read only periods the current branch can see when uprating or carrying over #552 already fixes that (_split_key), so the property test uses names without _.
  • Deleting a period. delete_arrays(var, "2017") removes the months in memory but only the exact key on disk. Another open PR, branch fix-delete-arrays-fast-cache, makes OnDiskStorage.delete remove contained periods.
  • Writing into a returned array. In memory this mutates the cached value; on disk it does not.
  • Branch names with :. InMemoryStorage raises for these; disk storage works.
  • Deleted keys keep their files on disk. delete() and _invalidate_all_caches forget mappings but leave files until the directory is removed, since another storage may read them.

Overlap with other PRs

Tests

  • Before merging master: uv run pytest tests gave 1115 passed, 4 skipped, 1 xfailed (Python 3.13, with -W error::pytest.PytestUnraisableExceptionWarning).
  • On this head: the disk-storage, branch, shared-array, holder, apply-reform, cache-invalidation and dump/restore test files pass locally (127 tests). The round-3 review's repro (repro.py) and its earlier 16-scenario attack script pass, apart from the four pre-existing differences listed above. The local full-suite run stalled inside a test's gc.collect() while the machine was swapping, so on this head the full suite is CI's.
  • On this head: the new test files plus scoped-delete and dump/restore pass on Python 3.11 (39 passed) and 3.14 (38 passed; the property module was skipped in that environment, which has no Hypothesis).
  • The property test with 2,000 examples and up to 40 operations passed on an earlier head. The round-3 review ran 300 examples with extended operations on c7a0369.
  • Smoke collection (pytest -m smoke) in an environment without Hypothesis: collects cleanly; the property module is skipped.
  • ruff format --check . and ruff check . pass.
  • Not run locally: Windows (CI covers it; the fork test is skipped there) and make documentation.

axiom: n/a: core storage infrastructure, no policy change

🤖 Generated with Claude Code

MaxGhenis and others added 4 commits October 1, 2026 15:48
A holder created lazily in a branch made an OnDiskStorage that owned the
family's shared <data_storage_dir>/<variable> directory, so garbage-collecting
the branch deleted the parent's files. The shared directory also let a
Simulation.clone() or a same-named branch under another parent overwrite the
parent's .npy files, and let a parent's later write change what its branch
read.

Each storage now writes only into a directory of its own (created on first
write inside the simulation's data_storage_dir), a clone keeps alive every
directory it reads from, and a file a clone reads is never overwritten.
Directories are removed by finalizers on reference-counted StorageDirectory
handles, only in the process that created them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ression test

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Simulation.derivative perturbs a clone's input; with the shared directory it
overwrote the simulation's own input file and returned 0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ypothesis

The country-package smoke job installs no dev dependencies, so a module-level
hypothesis import failed its collection. Shared helpers move to
tests/fixtures/disk_storage.py.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis and others added 2 commits October 2, 2026 01:58
…branch-ownership

# Conflicts:
#	policyengine_core/simulations/simulation.py
Another storage, possibly in another process, may have restored the same
directory and read those files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… 3.14

Python 3.14 removed pickle/copy support from itertools objects, so
itertools.count made copy.deepcopy and pickle of an OnDiskStorage raise.
Found by the independent review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arents

Round-3 review (REQUEST_CHANGES) found that a preserved storage inside the
simulation's directory was removed with it, and that writing after restore()
could overwrite an older clone's file or make a later restore read a stale
value. The earlier attack script also showed permanent "shared" marks made
every re-store after any clone (such as derivative's) add a file.

- StorageDirectory keeps, per file, a WeakSet of the storages that may read
  it besides its creator (clones, copies, restorers); one handle per path per
  process.
- A storage overwrites only the newest file it created itself, since the
  last fork, that no live reader may read (and, outside its own temporary
  directory, that is still the key's highest-numbered file). Otherwise it
  creates a new file with O_EXCL, numbered above every file for the key.
- Preserving a directory preserves its parents; create_disk_storage(preserve=
  True) inside data_storage_dir takes the simulation's handle as parent.
- copy/deepcopy return the same directory handle and pickles find the live
  one, so copies keep the directory; copies never overwrite the original's
  files.

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant