fix: isolate EventListener failures from protected calls and lanes - #108
Merged
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Listener hooks were invoked directly at every call site, so a bug in a user's listener became a new failure source: - it could replace a successful protected result with an observability exception, or mask the dependency's own error; - worse, on a coordinated breaker `poll_once` calls `_accept` outside its `try` (interlock/_coordination.py), so a raising `on_state_change` propagated out of the lane tick and terminated the background lane for good — the breaker then never polled the shared view or flushed queued writes again, with nothing surfaced anywhere; - inside `execute_op` the same exception was caught by `except Exception` and routed to `_degrade`, reporting a listener bug to the user as a *storage* failure via `on_storage_degraded`. Route every hook through one shared dispatcher, `interlock/_notify.py`: dispatch by name via `getattr` (so every hook stays optional and pre-1.2 / pre-2.0 listeners keep working), catch `Exception`, log it to the `interlock` logger at ERROR with hook and breaker context, and continue. `BaseException` is not caught — cancellation and shutdown propagate as everywhere else. The policy covers core, storage and pipeline hooks. User-supplied *policy* callbacks are unchanged and still raise: a `FailureClassifier`, a fallback function, a tenacity `before_sleep`. `_NoopListener` and the two ad-hoc `_notify*` helpers are gone; the engine now holds `EventListener | None` and the no-listener path skips dispatch entirely. Closes #83.
bagowix
force-pushed
the
fix/isolate-listener-failures
branch
from
August 1, 2026 13:58
a59c01a to
2ae8cb0
Compare
6 tasks
bagowix
added a commit
that referenced
this pull request
Aug 1, 2026
…on (#110) ## Summary Two gaps in `AGENTS.md` that have each cost rework already. **1. The CHANGELOG and docs obligations lived only in the PR checklist.** They surface at review time rather than while the work happens, so a change could arrive at a PR complete in every respect except the two things a user actually reads. `#108` shipped its `CHANGELOG` entry only because the template got opened at the end. Adds a **Definition of done** section naming them explicitly, together with the `llms-full.txt` regeneration step (previously documented only in `docs/CLAUDE.md`, which an agent working in `interlock/` never reads) and the tests-first expectation. The PR template is called out as the last gate, not the first reminder. **2. The "no silent exceptions" hard rule now has a deliberate, shipped counter-example.** `interlock/_notify.py` logs a raising `EventListener` hook with its traceback and swallows it, so observability cannot replace a protected result, mask a dependency's exception, or kill a coordinator lane (#83). Without that written down, the next reader — human or agent — sees a blind `except Exception` in a codebase whose style guide forbids exactly that, and "fixes" it. The note records the exception, states why it is not actually silent (`BaseException` still propagates, the traceback is logged), and bounds it so it is not read as licence to swallow anywhere else. ## Checklist - [ ] Tests added or updated (suite stays at 100% coverage) — n/a, documentation only - [x] `uv run ruff format --check` and `uv run ruff check` pass - [x] `uv run mypy` and `uv run pyright` pass - [x] Docs updated (`docs/`) for user-facing changes — n/a, `AGENTS.md` is contributor-facing - [ ] `CHANGELOG.md` `[Unreleased]` updated — n/a, no user-visible behaviour change - [x] Commits follow Conventional Commits ## Related issues None — follow-up housekeeping after #108 and #109.
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
An EventListener that raises could damage the breaker it observes. Hooks were invoked directly at every call site, so a bug in optional observability became a new failure source in a fault-tolerance library.
Three distinct consequences of one root cause, all confirmed in the code:
The protected call.
on_callraising inEngine._settlereplaced a successful result with an observability exception, or masked the dependency's own error.Silent death of the coordinator lane.
poll_oncecalls_acceptoutside itstry(interlock/_coordination.py:248, 417), and_accept → Engine._on_shared_view → _emit_transitions → on_state_change. A raising listener propagated out of_sync_lane_tick/_async_lane_tickand terminated the daemon thread / asyncio task permanently. From then on the breaker never refreshed the shared view and never flushed queued writes again — with nothing surfaced anywhere.False diagnosis. Inside
execute_opthe same exception was caught by the genericexcept Exceptionand routed to_degrade, so a listener bug was reported to the application as a storage failure viaon_storage_degraded.The fix. A new
interlock/_notify.pyholdsnotify()— now the only place an EventListener hook is ever invoked:getattr, so every hook is optional and pre-1.2 / pre-2.0 listeners keep working;Exceptionis logged to the interlock logger atERRORwith the traceback and hook/breaker context, then ignored;BaseException(KeyboardInterrupt,asyncio.CancelledError) is not caught — cancellation propagates as everywhere else in interlock.Applied to core, storage and pipeline hooks (
_engine.py,pipeline.py,integrations/tenacity.py). User-supplied policy callbacks deliberately keep their previous semantics and still raise — aFailureClassifier, a pipeline fallback function, a tenacitybefore_sleephook — because they shape the call's behaviour rather than observe it.Incidentally removes
_NoopListener,_NOOP_LISTENERand two ad-hoc_notify*helpers: the engine now holdsEventListener | None, and the no-listener path skips dispatch entirely instead of calling a no-op method.No public API change. The EventListener protocol is unchanged; the guarantees around it got stronger. One behavioural nuance worth review: a hook missing from a listener is now skipped instead of raising
AttributeError, which makes the whole protocol uniformly optional — consistent with how the storage and pipeline hooks already behaved, and static checkers still catch a missing core hook at the call site.Checklist
tests/test_notify.py(19 tests) +ExplodingListenerfixture inconftest.py, 3 tests intest_pipeline.py, 2 intest_tenacity.py. Written test-first: 5 failed withRuntimeError: listener ... explodedbefore the fix. Total 517 passed / 2 skipped, coverage 100.00%.uv run ruff format --checkanduv run ruff checkpass.uv run mypyanduv run pyrightpass (both strict).docs/) — new "Listener failures are isolated" section indocs/guides/observability.md, updatedpipeline.md, refreshed EventListener docstrings,llms-full.txtregenerated.CHANGELOG.md[Unreleased]updated.Standalone invariant holds: the entire v1 suite passes unmodified — in existing test files only import lines changed, not a single assertion. Benchmarks (
benchmarks/, 21 tests) still pass; the no-listener hot path got marginally cheaper.Related issues
Closes #83.