feat(decisioning): add registry observer removal#861
Merged
Conversation
There was a problem hiding this comment.
LGTM. Lock-protected snapshot copy with dispatch outside the lock is the right shape — re-entrant add/remove from inside an observer can't deadlock, and the "removal during in-flight notification still fires this round" contract is exercised by the self-remove test.
Things I checked
- Snapshot semantics at
src/adcp/decisioning/pg/buyer_agent_registry.py:422-424—tuple(self._mutation_observers)taken under the lock, iteration outside. Observers added mid-flight skip this round; observers removed mid-flight still fire this round. Matches the assertion sequence["self-removing", "persistent", "persistent"]attests/conformance/decisioning/test_pg_buyer_agent_registry.py:387. - Re-entrancy:
threading.Lockis non-reentrant, but thewithblock at_notify_mutationexits before iteration starts, so an observer callingremove_mutation_observerre-acquires cleanly. Self-remove test confirms. remove_mutation_observerreturn contract:list.remove()'sValueErroris caught →False; success →True. Idempotent double-remove asserted attest_pg_buyer_agent_registry.py:350-351.- Conventional-commit prefix:
feat(decisioning):is right — additive public method onPgBuyerAgentRegistry, no signature change, no!needed. - Lock scope is minimal — held only across
append/remove/ tuple-copy. Observer dispatch never runs under the lock.
Follow-ups (non-blocking — file as issues)
with_cachingatsrc/adcp/decisioning/pg/buyer_agent_registry.py:417registers an anonymous lambda observer that cannot be removed by identity. Pre-existing, but now that removal is on the wire it's worth exposing the registered callback (or returning(cache, observer)) so callers can unwire. Out of scope here.
Minor nits (non-blocking)
- Docstring on
add_mutation_observerblurs the add vs. remove semantics.src/adcp/decisioning/pg/buyer_agent_registry.py:359-362says "observers added or removed while a notification is in flight apply to the next mutation." The remove path is the opposite — a removed observer still fires this round (which is exactly what the self-remove test locks in).remove_mutation_observer's docstring at L374-376 gets it right; theadd_mutation_observerblurb should match: "observers added during a notification fire on the next mutation; observers removed during a notification still fire for that in-flight notification but not subsequent ones." - Test could lock the N-registrations contract.
tests/conformance/decisioning/test_pg_buyer_agent_registry.py:342-360verifies double-remove returnsFalsebut doesn't assert the "one registration removed per call" promise atbuyer_agent_registry.py:371-372. Two extra lines (add twice, remove once, mutate, expect one call) would pin that behaviorally. Optional.
A re-roll of #764 to get the repo-owned check suite — fine, but worth noting in the body so the next reviewer doesn't go hunting for the diff between them.
LGTM. Follow-ups noted below.
This was referenced May 26, 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
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
Replaces #764 so the required repo-owned checks can run with the normal base-repo secrets/check suite.
Closes #696.
This adds lifecycle management for
PgBuyerAgentRegistrymutation observers. Observers can now be removed explicitly, and the observer registry is protected by a lock so add/remove operations do not race with mutation notification setup.What changed
remove_mutation_observer(observer) -> bool.threading.Lock.Local validation
PYTHONPATH=src ruff check src/adcp/decisioning/pg/buyer_agent_registry.py tests/conformance/decisioning/test_pg_buyer_agent_registry.pyPYTHONPATH=src pytest tests/conformance/decisioning/test_pg_buyer_agent_registry.py -q(skips locally: no Postgres fixture in this workspace)