[Adelagric]

Fixes #1155.

Problem

Simulation.clone() promises to "copy the simulation just enough to be able to run the copy without modifying the original simulation", but the clone actually shares most of its mutable state with the original. Four distinct defects compound:

1. Holder.clone shares storage by reference: the cloned holder reuses the original's InMemoryStorage and OnDiskStorage objects. Any set_input, put_in_cache or delete_arrays through one simulation silently alters the other. Worse, the original can end up internally inconsistent: change salary through a clone and the original now returns the new salary but keeps its stale cached income_tax.
2. GroupPopulation.clone binds cloned holders to the original population: holder.clone(self) is called instead of holder.clone(result), so the clone's group-entity holders have population and simulation pointing at the original simulation.
3. GroupPopulation.clone keeps the original persons as members — this is the mechanism @nikhilwoodruff suspected in Possible bug in clone logic #1155: clone.household.members is original.persons.
4. The clone shares _data_storage_dir: when both simulations lazily create a holder for the same variable (with a memory_config active), their two distinct OnDiskStorage objects write {period}.npy files into the same directory. One simulation then reads the other's values, and garbage-collecting either simulation rmtrees the directory, physically destroying the other's data.

Minimal reproduction of defect 1 (before this fix):

simulation = SimulationBuilder().build_from_entities(tbs, situation)  # salary = 2000
simulation.calculate("income_tax", "2026-01")   # 300, cached
clone = simulation.clone()
clone.delete_arrays("salary", "2026-01")
clone.set_input("salary", "2026-01", [4000])
simulation.calculate("salary", "2026-01")       # 4000 — the original was modified
simulation.calculate("income_tax", "2026-01")   # 300 — stale, inconsistent with salary

Fix

• InMemoryStorage.clone() / OnDiskStorage.clone(storage_dir): new methods returning independent copies. The in-memory clone copies the period-to-array mapping only (arrays stay shared, as the engine treats them as immutable). The on-disk clone copies the .npy files into the new directory and re-registers the enum mapping under the new directory key, so EnumArray values survive the copy.
• Holder.clone uses them instead of sharing references.
• GroupPopulation.clone binds holders to the cloned population and takes the cloned persons (simulation.persons) as members.
• Simulation.clone resets _data_storage_dir (the clone lazily gets its own temporary directory) and copies invalidated_caches.

Trade-offs

• Cloning a simulation with on-disk storage now copies the stored files (I/O cost proportional to spilled data). The alternative — sharing files read-only — was rejected because either simulation's cleanup deletes the other's files.
• Arrays themselves are still shared between original and clone, consistently with how they are already shared within a single simulation's cache.
• Code that relied on the sharing behaviour (e.g. writing into a clone to warm the original's cache) will observe a change; that behaviour contradicted the documented contract of clone.

Test plan

• 6 new tests in tests/core/test_simulations.py, each failing on master (except the enum guard, which protects the new copy logic): holder-storage isolation, group holders bound to the clone, cloned members, invalidated_caches copy, on-disk isolation incl. garbage-collection of the clone, enum preservation on disk clone.
• Full test suite and doctests pass; isort/black/flake8/pylint clean on the changed files.

[Adelagric]

@nikhilwoodruff since you reported #1155 and have been living with this at PolicyEngine — if you have a chance to run this branch against your workloads, that would be a much stronger validation than my regression tests alone. Happy to adjust anything that surfaces.

[guillett]

That looks good to me. Many thanks @Adelagric

[Adelagric]

Thanks @guillett! One thing: the CI workflows haven't run on this PR yet — as a first-time contributor they need a maintainer to approve the run. Happy to rebase if the version bump conflicts with anything landing before this.

While you're at it: #1386 (spiral warning, #956) and #1387 (restore_simulation counts) are the two sibling PRs from the same pass, also awaiting a first look.

[guillett]

Why are warnings ignored here?

[Adelagric]

Good catch — no good reason. The only warning raised there is MemoryConfigWarning (the "experimental feature" notice), and test_holders.py already uses MemoryConfig(max_memory_occupation=0) without filtering anything. Removed in the latest commit.

[guillett]

Did you try to run tests directly on your own machine?

[guillett]

I'm trying to figure out, I much AI assisted you are.

[Adelagric]

Fair question, and I'd rather answer it plainly than have it hang over the PR.

Yes, the tests were run locally several ways (pytest, make test-core, full suite), all green with the project config. The filterwarnings was a defensive reflex from a run with -W error, where MemoryConfigWarning fails the disk tests; since test_holders.py uses MemoryConfig without filtering, I removed it to follow the existing convention.

On AI: I use AI tooling in my workflow, the way I'd use an IDE — but the analysis is mine, and I can defend every line. The five compounding defects behind #1155 were each reproduced by hand before a line of fix was written; the subtle one is the shared _data_storage_dir, where fixing the holder storage alone just moves the bug to the filesystem. Ask me anything about it.

To be clear about where I stand: I'm here because these are real silent-data-loss bugs in a codebase people rely on for policy decisions, not for the merge count. If the project would rather not take contributions from people who use these tools, say so and I'll respect it — no hard feelings either way. If the concern is maintainability, I'm happy to stay reachable on this code after merge.

[Adelagric]

Fair question. Yes — the tests were run locally several ways (pytest, make test-core, full suite), all green with the project config. The filterwarnings came from a run with -W error, where MemoryConfigWarning fails the disk tests; since test_holders.py uses MemoryConfig without filtering, I removed it to follow the convention.

On AI: I use AI tooling in my workflow, the way I'd use an IDE. The analysis is mine and I can defend every line — the five compounding defects behind #1155 were each reproduced by hand before any fix was written, and the subtle one (the shared _data_storage_dir, where fixing the holder storage alone just moves the bug to the filesystem) is the kind of thing you only find by running it. Ask me anything about it.

I'm here because these are real silent-data-loss bugs in code people rely on for policy decisions. If the project prefers not to take contributions from people who use these tools, just say so — I'll respect it. Otherwise I'm happy to stay reachable on this code after merge.

[guillett]

Shoudn't these cloned? I guess they are array references, no?

[Adelagric]

Array references, yes — this block predates the PR. Nothing in core mutates them in place (every write path reassigns a new array; the indexed write in members_position is on a freshly built one), so sharing them was safe under the same immutability policy as holder values. It's a convention, not a guarantee, and copying costs nothing: done in e14e0d6, with a test.

[MattiSG]

Thanks for the clarifications @Adelagric.

As you said, this is “code people rely on for policy decisions”, and unfortunately that means we have to deal with real risks of reputation farming, privilege escalation, or even exploit planting (prepositioning). Core was never, until LLM assistance, the entry point for new contributors due to the complexity of the codebase and the potential global impact of changes.
It is super exciting to have new contributors, and we're glad to have you come along as someone who seems to be able to participate in discussions beyond dumping an LLM-generated changeset 🙂 However, we also need to be very careful about supply chain vulnerabilities, and about the work of volunteers who have to handle the consequences of a bug in a new deployed version (aka “I'm not super keen on another sleepless night of emergency rollbacks across deployment sites and PR management on behalf of an anonymous third party” 😄).

We are slowly learning to deal with the size and complexity of AI-generated contributions, but this is an area under active definition, and I'm sorry that everything is not super clear up front. Please bear with us!

In particular, the OpenFisca community is trying to define its approach with regards to AI-assisted contributions. You can find of our current line of thinking in #1372. In particular, defining the level of assistance and model(s) used would be helpful, as @guillett mentioned in #1385 (comment) 🙂

It would also make a significant difference in the trust and effort we can allocate to such changesets if we understood a bit more of your context for using OpenFisca and contributing to it 🙂 I could gather from your public website that you claim to be based in France, but a little hi and an intro in our community chat would be great as well! I sent you an invite to your publicly listed email. Feel free to join there or drop a line on contact@openfisca.org. In particular, understanding which production uses you have of OpenFisca, what's your experience with it, and what motivates you to contribute to Core would be very helpful.

Note that this is not an official precondition as we have not made this into a strict prerequisite policy, and I'll let @guillett and other Core maintainers judge the relevance of the changeset itself. I just hope to bring some guidance on what can make the integration of your work into the codebase smoother 🙂 OpenFisca is not merely an open source project, it is a digital common and that means governance and the human connections that make the codebase evolve are an integral part of it.

Have a good day! 😊

[MattiSG]

I've checked the list of changed files: none modify CI. I've checked that the introduced code does not make suspicious new logs (secret extraction) or remote calls: all good.
I've thus approved workflows to run 🙂 CI will report in a bit.

[Adelagric]

Thanks @MattiSG — both for taking the time to write this and for reviewing the diff and approving the workflows. I read #1372, and I appreciate that the policy is being worked out in the open.

On AI usage: under the RFC terminology, I would classify the submitted fix and tests as 🟠 AI-generated: I use an LLM coding assistant under my direction to write code, while the investigation and reproduction work here was mine. Every defect in #1155 was first reproduced independently against the actual code; the fix was then developed test-first, with the tests confirmed red on master before going green with the change. I also run an adversarial review pass before submitting — in this case it caught a disk test that was not actually discriminating.

Everything I submit is reread, executed and verified by me, and I consider myself responsible for it. I can explain every line and maintain the code afterward.

For tooling, I use an agentic coding setup with an LLM assistant. I'm happy to disclose the exact tool/model explicitly once the contribution template has a place for it.

On who I am: I'm Adel Kaleche, based in Occitanie, France. I teach part-time and run a small software and operations-research consultancy on the side (kaleche.dev). I do not use OpenFisca in production, so I want to be explicit about that.

Most of my open-source work has converged on the same kind of problem: silent data loss and silent failures. I have contributed fixes in projects such as rust-bio, Rostam, conda and x402, and I currently have work open in several data/infrastructure projects. The recurring pattern is the same: find a case where the system loses, corrupts or mis-associates data without failing loudly; reproduce it deterministically; then fix it behind a red-then-green regression test.

Why OpenFisca: part of the work I'm building a specialty around sits at the boundary between probabilistic and deterministic systems. On the probabilistic side, I recently wrote a preprint on detecting silent distribution drift: https://arxiv.org/abs/2608.01268. On the deterministic side, I tend to hunt exactly the kind of failures I found in the clone/dump/spiral paths here.

As more code is produced with AI assistance, I think this boundary — systems appearing to work while silently producing the wrong state — is going to matter more, not less.

The second reason is more personal: France took me in, and contributing to a digital common the country relies on felt like a small way to give something back.

On future involvement: I can't promise a regular contribution rate — teaching and client work make that difficult — but I do consider myself responsible for the code I submit. I'll stay available for review questions, regressions, or follow-up work on #1385, #1386 and #1387 after merge.

Beyond that, I'll probably continue following this same thread across other codebases whenever I find another silent failure worth digging into.

I'll say hi in the community chat as well. Thanks for the invitation.

[Adelagric]

Rebased on main after the master→main and ruff migrations: version bumped to 45.0.5, CHANGELOG entry moved on top, import order adjusted for ruff in group_population.py. No functional change, same three commits. Tests and ruff green locally.

I was too optimistic

[guillett]

Could you please refactor this PR moving toward a breaking change that removes clone entirely and rely on a more pythonic way with copy (and deepcopy).

It looks like clone promise could be equivalent to a copy. (clone should not have debug nor trace as parameters, you can remove them as well.)

[Adelagric]

Happy to look at a copy/deepcopy design, but I'd suggest keeping it separate from this PR. This one fixes a verified silent-data-loss bug with the current API, and was approved as such; a breaking removal of clone is an API decision that deserves its own issue (and a 46.0.0 with a migration note for openfisca-france, survey-manager and PolicyEngine, which all call clone). One design point to settle first: the right semantics here is neither copy.copy (shallow — that's the bug) nor copy.deepcopy (would copy the tax-benefit system and every array); it's "fresh holders, shared arrays, shared TBS". That can be a custom __copy__, but then copy.copy(simulation) doesn't mean what people expect. Worth agreeing on before writing it. Shall I open the issue?

[Adelagric]

Rebased on main (46.0.2). The version is now bumped in pyproject.toml and uv.lock (46.0.3). No code change. The test suite passes, and the six new clone tests fail on main.