Repository navigation
fix: StoredList unwrapping, observer lookup, handle keys and deferred storage events - #2850
Open
tonyandrewmeyer wants to merge 6 commits into
Open
tonyandrewmeyer wants to merge 6 commits into
tonyandrewmeyer wants to merge 6 commits into
Conversation
… storage events A collection of framework edge cases that each surfaced far from their cause: * StoredList.append, insert and extend stored the wrapped value, so putting one stored value inside another only failed in commit(). * observe() recorded the method's __name__ and looked it up again at dispatch, so a handler decorated without functools.wraps was registered but never called, and the deferred notice was silently dropped. The name is now checked when the observer is registered, and a missing method is logged at dispatch. * A Handle key containing "/", "[" or "]" was written into the path but could not be read back by from_path, failing in a later dispatch. It is now rejected when the handle is created. * Handle.__eq__ raised AttributeError when compared with another type. * A deferred event whose type is no longer registered was dropped without any log message. * remove_unreferenced_events skipped event kinds containing digits, such as redis2_pebble_ready. * A deferred StorageEvent whose storage had since been removed raised RuntimeError in restore() on every dispatch from then on. The Storage is now rebuilt from the snapshot so the handler still runs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…thod Raising broke the unit tests of oauth2-proxy-k8s-operator, where a fixture patches a library's observer with a lambda. The event is never emitted in those tests, so the observer not being found doesn't matter to them. A warning still flags the mistake when the observer is registered, and charms with the same pattern keep working. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tonyandrewmeyer
marked this pull request as ready for review
November 8, 2026 23:08
Both look up their own attributes inside __getattr__, so a lookup made before __init__ has run (as copy and pickle do) recursed until it raised RecursionError: copy.copy(charm.on['db']) failed, for example. PrefixedEvents now rejects private names, like the lazy model classes. BoundStoredState can store keys that start with an underscore, so it instead checks whether its data has been set up yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.
A collection of framework edge cases, each of which surfaces a long way from its cause:
StoredList.append,insertandextendstored the wrapped value, so putting one stored value inside another only failed incommit().observe()records the method's__name__and looks it up again at dispatch, so a handler decorated withoutfunctools.wrapswas registered but never called, and the deferred notice was silently dropped. The name is now checked when the observer is registered, with a warning if it doesn't find the method, and a missing method is logged at dispatch rather than ignored.Handlekey containing/,[or]was written into the path but couldn't be read back byfrom_path, failing in a later dispatch. It's now rejected when the handle is created.Handle.__eq__raisedAttributeErrorwhen compared with anything that wasn't aHandle.remove_unreferenced_eventsskipped event kinds containing digits, such asredis2_pebble_ready.PrefixedEventsandBoundStoredStaterecursed in__getattr__when an attribute was looked up before__init__had run, socopy.copy(charm.on['db'])raisedRecursionError.StorageEventwhose storage had since been removed raisedRuntimeErrorinrestore()on every dispatch from then on. TheStorageis now rebuilt from the snapshot (which already has the location) so the handler still runs.One of these newly rejects input that was previously accepted. A handle key with
/,[or]only failed before if an event was emitted through that object (aStoredState-only object with such a key worked, becauseload_snapshottakes aHandlerather than parsing a path). I think that's a bug rather than behaviour anyone relies on, and a Hyrum run found no charms that break. A handler without a matching attribute name still registers, but now logs a warning, because rejecting it broke the unit tests ofoauth2-proxy-k8s-operator.Two collateral changes to existing tests:
test_reemit_ignores_unknown_event_typewas using in-memory storage for both frameworks, so the second one had nothing to re-emit and the test passed vacuously - it now usestmp_path. The removed-storage test resetsmodel.storages._storage_mapdirectly, to simulate a fresh dispatch without a second framework.The last fix is the most significant, and the one I'm least sure about: the alternative would be to drop the notice, but running the handler with the recorded location seems closer to what the charm author would expect. The warning for the dropped deferred event could also reasonably be a debug message, if people think it'll be noisy.