[tonyandrewmeyer]

State.deferred only picks up a notice if its event's handle path matches ops' event regex, which needs an on/. A deferred custom event from an ObjectEvents created with a key (MyCharm/on[key]/event[n]), or from an EventSource directly on the charm (MyCharm/event[n]), is silently dropped, so its handler never runs again, although ops itself re-emits every notice whatever the path. The same snapshot also shows up in State.stored_states as a bogus StoredState.

This reads every notice instead, as ops does, and leaves anything with a notice out of the stored states.

Two more things came out of looking at this, and they change what tests see, so they're worth calling out for the release notes:

• State.deferred is now in the order the events were deferred, which is the order ops re-emits them in. It was in handle path string order (list_snapshots() has no ORDER BY), so a test that unpacked out.deferred into named variables may now get them the other way around. One of our own tests did, and its comment already said the new order.
• A StoredState with no owner_path is the framework's own stored state, and it is now written to the key ops reads (StoredStateData[_stored] rather than /StoredStateData[_stored]). That fixes a duplicate _stored entry with owner_path='' appearing after a State round trip. It also means a StoredState that's meant to be the charm's but has owner_path left out would overwrite the framework's state, so the consistency checker rejects that with a message saying what to set.

Hyrum has one newly failing test and none newly passing. Valkey's test_start_primary defers unit_fully_started (an EventSource on its BaseEvents object) in one start run and asserts the status after the next. With this change the deferred event is re-emitted before that start, as it would be in Juju, so the status differs. I think that's the test relying on the bug rather than a reason to keep it, so I think we should make this change, but open a PR to fix things there.

It turned up in a multi-charm test of Valkey, where the dropped event meant its start lock was never released.

[hpidcock]

@chapman-von-monty review this PR.

[chapman-von-monty]

Here is my review of PR #2811:

Executive Summary & Verdict

Verdict: Request Changes

Reconstructing deferred events directly from the storage notice stream (db.notices()) is a very clean and sound fix for preserving custom events without on/ in their handle paths, as well as maintaining proper notice-emission order. The handle-path fix for framework stored state to prevent duplicate _stored entries on round-trips is also well-targeted.

There is one blocking validation issue in the newly introduced consistency checker rule:

---

Blocking Issue

Validate ownerless framework _stored payload schema/type, not just key presence

• Location: testing/src/scenario/_consistency_checker.py (around lines 801–808 in the PR diff).
• Issue: The check treats an ownerless StoredState(name='_stored', ...) as framework state merely by checking 'event_count' in ss.content. However, event_count is the framework's internal event sequence counter. Values like {'event_count': None} or {'event_count': '1'} pass consistency checking and only blow up later during framework initialization or event dispatch. The goal of this consistency checker rule is to catch malformed or ambiguous ownerless charm state early with clear, actionable diagnostics.
• Recommendation: Validate that event_count is present and is a non-negative integer (e.g. type(event_count) is int and event_count >= 0, disallowing bool), and ensure ownerless StoredState objects whose name is not '_stored' are cleanly rejected with guidance to set owner_path:

  if ss.owner_path is None:
      if ss.name != '_stored':
          errors.append(
              f"The StoredState object {ss.name!r} has no owner_path. "
              "Set owner_path to the object that owns it."
          )
          continue
  
      event_count = ss.content.get('event_count')
      if type(event_count) is not int or event_count < 0:
          errors.append(
              "The framework StoredState object must contain a "
              "non-negative integer 'event_count'."
          )

  Please add parameterized tests in testing/tests/test_consistency_checker.py covering invalid values (e.g. None, string, boolean, negative integer) and verifying that the diagnostic is raised.

---

Non-blocking Suggestions & Observations

1. Release notes / Changelog: Because State.deferred ordering now matches deferral order (which may affect tests unpacking out.deferred positionally) and previously dropped custom deferred events are now preserved, consider including a note in the release notes / changelog highlighting this change for charm authors.
2. Empty owner_path semantics: Currently StoredState.owner_path allows str | None, and _handle_path evaluates self.owner_path or ''. As a follow-up, consider clarifying whether owner_path='' should be explicitly rejected or treated identically to None.
3. Downstream coordination: As noted in the description with Valkey's test_start_primary, downstream charms/tests that inadvertently relied on custom events being dropped may need adjustments once this merges.

[tonyandrewmeyer]

a very clean and sound fix

This is a bit sycophantic for my taste, but if it doesn't always sound like that, then ignore this.

There is one blocking validation issue in the newly introduced consistency checker rule:

Sure, {'event_count': None} or '1' would pass the checker and then fail with a TypeError on the first emit. However, the details in the example are wrong (True and negative integers don't break anything. True + 1 == 2, and a negative counter only gives odd handle keys). I feel like "non-negative, not bool" adds rules the framework doesn't actually have.

I definitely would not consider this blocking. It's extremely unrealistic: the framework's _stored gets into State by round-tripping output, which always holds an int. Writing event_count by hand as a string or None is very unlikely. Nothing else in the consistency checker validates framework internals this closely. Note also that this is test code, not something where unexpected data could come from a user or Juju or some other external system.

For the suggestion to reject ownerless StoredStates with other names, I would argue it's out of scope. Before this PR, StoredState(name='my_stored_state') with no owner was written to /StoredStateData[...], which ops never reads. Now it goes to StoredStateData[...], which ops also never reads. Nothing about it changed here. I'm also not certain we would want to hard-code a framework internal like that into Scenario.

The goal of this consistency checker rule is to catch malformed or ambiguous ownerless charm state early with clear, actionable diagnostics.

I wouldn't define the goal in this way.

I'm fine adding an isinstance(event_count, int) check, but definitely feel blocker is wrong.

1. Release notes / Changelog: Because State.deferred ordering now matches deferral order (which may affect tests unpacking out.deferred positionally) and previously dropped custom deferred events are now preserved, consider including a note in the release notes / changelog highlighting this change for charm authors.

This is not how changelogs/release notes are handled in any Charm Tech repos.

2. Empty owner_path semantics: Currently StoredState.owner_path allows str | None, and _handle_path evaluates self.owner_path or ''. As a follow-up, consider clarifying whether owner_path='' should be explicitly rejected or treated identically to None.

This is the best point in the review, but the bot missed the bug. _handle_path uses if self.owner_path:, so '' is treated like None (since it's false-y) and gets written to StoredStateData[_stored]. The new checker uses ss.owner_path is None, so StoredState(owner_path='', content={'foo': 'bar'}) gets past the check and overwrites the framework's state.

3. Downstream coordination: As noted in the description with Valkey's test_start_primary, downstream charms/tests that inadvertently relied on custom events being dropped may need adjustments once this merges.

The description says this and there's a linked PR, so it seems like the bot should have been able to figure out this was already handled.

Hard to me to know if the bot missed anything, so would be worth checking for that after James and Nhan review.