fix: deep-copy layer objects during Scenario plan rendering#2380
Merged
tonyandrewmeyer merged 13 commits intocanonical:mainfrom Mar 20, 2026
Merged
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
reviewed
Mar 16, 2026
Contributor
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!)
dimaqq
approved these changes
Mar 16, 2026
Comment on lines
+950
to
+953
| # 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 |
Contributor
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>
tonyandrewmeyer
commented
Mar 19, 2026
Collaborator
Author
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. |
james-garner-canonical
approved these changes
Mar 20, 2026
dwilding
approved these changes
Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.