fix: deep-copy layer objects during Scenario plan rendering#2380
fix: deep-copy layer objects during Scenario plan rendering#2380tonyandrewmeyer wants to merge 13 commits intocanonical:mainfrom
Conversation
When rendering services, checks, and log targets in Container's plan property, the else branch stored direct references to the original Layer's objects. A subsequent merge call on those references would mutate the original Layer in place, causing list fields (after, before, requires, services) to accumulate duplicates each time .plan was accessed. Use copy.deepcopy() in the else branch of _render_services(), _render_checks(), and _render_log_targets() to prevent mutation of the original layer objects. This is the same class of bug that was previously fixed in Harness (commit 3dda5b5). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tonyandrewmeyer
force-pushed
the
fix/scenario-render-services-deepcopy
branch
from
8aa7b17 to
7242367
Compare
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
I ran the new test with testing/src/scenario/state.py unmodified, and the test passed, but I was expecting it to fail. So it looks like something isn't quite right (perhaps my understanding is off!)
| assert plan1.checks['chk-a'].level == plan2.checks['chk-a'].level | ||
|
|
||
| # Log target list fields must not accumulate duplicates. | ||
| assert plan1.log_targets['lt-a'].services == plan2.log_targets['lt-a'].services |
There was a problem hiding this comment.
Would there be a way to do a "deep equality" check between plan1 and plan2? The standard library doesn't provide anything for that does it? In which case this seems fine
There was a problem hiding this comment.
We could do a deep equality check with plan1.to_dict() == plan2.to_dict(). But I don't think this test, as written, is actually exercising anything, since that just means container.plan.to_dict() == container.plan.to_dict().
In order to actually test what this PR is trying to fix, the test should instead be mutating layer1 and ensuring the changes aren't reflected here in the plan, or taking a deep copy of layer1 before passing it into Container and then asserting that layer1 wasn't mutated.
For example:
layer1_snapshot = copy.deepcopy(layer1.to_dict()) # idk if the copy is actually needed
container = Container(
'my-container',
can_connect=True,
layers={'base': layer1, 'override': layer2},
)
assert layer1.to_dict() == layer1_snapshotThere was a problem hiding this comment.
I hadn't understood that plan is a property that computes the plan on the fly.
Maybe assert container.plan.to_dict() == container.plan.to_dict() would do what we want here?
| # Service list fields must not accumulate duplicates. | ||
| assert plan1.services['svc-a'].after == plan2.services['svc-a'].after | ||
| assert plan1.services['svc-a'].before == plan2.services['svc-a'].before | ||
| assert plan1.services['svc-a'].requires == plan2.services['svc-a'].requires |
There was a problem hiding this comment.
This tests implementation but not the API.
Imagine one day we'd update runtime pebble code to extend these fields omitting duplicates (instead of plain extend as it is today).
Suddenly this test doesn't actually test anything any more.
A better test could be something like:
initial_state = ...
state_a = ctx.run(on_smth_that_merges_plan, initial_state)
assert "a" in state_a.get_container(...).foo
state_b = ctx.run(on_smthn_else_that_merges_plan, initial_state)
assert "a" not in state_b.get_container(...).foo`_MockModelBackend.secret_get()` returned `secret.latest_content` and `secret.tracked_content` directly, and `action_get()` returned `action.params` directly. This meant that mutating the returned dict would silently corrupt the Scenario internal state, a divergence from production behaviour where fresh data is returned each time. Apply the same fix as commit be09012 (which addressed the identical issue in `relation_get`): return `.copy()` / `dict()` so callers get independent copies. Includes regression tests that verify mutating the returned dict does not affect subsequent calls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Juju requires secrets to have at least one key with string keys and values. Validate this in Secret.__post_init__ so charmers get a clear error at construction time rather than a confusing failure during .run(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rather than raising on the first non-string key/value, collect all bad entries and report them together. Also use lowercase dict[str, str] in the error message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move _validate_content below __post_init__ in Secret - Use copy.deepcopy for action_get (params can be nested) - Dedent test assertions outside context manager scope Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Your understanding is fine, the test was off. It should be fixed now (and fails for me if I remove the fixes). Sorry about that - I should have done that check myself. |
5 participants
Summary
Container._render_services(),_render_checks(), and_render_log_targets()stored direct references to original Layer objects in their result dictionaries. When a subsequent layer merged into those references, the_merge()call mutated the original Layer's objects in place. This meant that accessing.plan(or.services) multiple times caused list fields (after,before,requires,services) to accumulate duplicates with each access.copy.deepcopy()when storing objects in theelsebranch (i.e. when a service/check/log_target is first encountered or usesreplaceoverride), so that merge operations never mutate the original Layer data.3dda5b5f("Pebble layer merging in Harness mutated the original layer objects").🤖 Generated with Claude Code but owned by me.