docs: describe what DISABLED actually stops recording - #164
Conversation
State.DISABLED and disable() were documented as "admit all traffic, record nothing", but only the sliding window ever goes quiet. Every admitted call is still classified, timed and delivered to the EventListener as on_call, so listener-fed dashboards keep reporting for a disabled breaker. The docstrings (State, CircuitBreaker.disable, Engine.disable, StateMachine.disable) and docs/guides/states.md now separate the two observability surfaces: the window, which stops taking outcomes, and the listener, which does not. A new section in the states guide contrasts METRICS_ONLY and DISABLED for rollout switches. Docs only; behaviour is unchanged. The added test pins the semantics the docs now promise.
WalkthroughThe change documents existing ChangesDISABLED observability
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/guides/states.md`:
- Around line 99-101: Update the rollout behavior description in the states
documentation to explicitly state that disable() stops new window data and
threshold evaluation but leaves snapshot() available; clarify that count-based
data remains while time-based data may expire. Apply the same wording to the
corresponding generated section in docs/llms-full.txt so both documents stay
synchronized.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 56404c80-13e3-47cf-8463-b5c5bddf5220
📒 Files selected for processing (8)
CHANGELOG.mddocs/guides/states.mddocs/llms-full.txtinterlock/_engine.pyinterlock/_state_machine.pyinterlock/breaker.pyinterlock/state.pytests/test_observability.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: GitGuardian Security Checks
- GitHub Check: Run benchmarks
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
- GitHub Check: quality (3.14t)
- GitHub Check: quality (3.14)
- GitHub Check: Platform smoke (macos-latest, Python 3.14)
- GitHub Check: Platform smoke (windows-latest, Python 3.14)
- GitHub Check: quality (3.13)
- GitHub Check: Platform smoke (windows-latest, Python 3.11)
- GitHub Check: Platform smoke (macos-latest, Python 3.11)
- GitHub Check: Coverage
- GitHub Check: Analyze (python)
⚠️ CI failures not shown inline (2)
GitHub Actions: Code scanning AI findings on PR #164 / 0_github-advanced-security.txt: Code scanning AI findings on PR #164
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "RUNNER_TEMP=$RUNNER_TEMP"�[0m
�[36;1mfind "$RUNNER_TEMP" -maxdepth 1 -type f -name 'git-credentials-*.config' -print -delete�[0m
�[36;1m�[0m
�[36;1m# Generate a unique token and stop processing workflow commands to prevent the runtime from injecting commands�[0m
�[36;1mSTOP_***REDACTED_SECRET_ASSIGNMENT*** /proc/sys/kernel/random/uuid)�[0m
�[36;1m�[0m
�[36;1m# Use a trap to ensure we always resume command processing and check for�[0m
�[36;1m# fallback error annotations, even if the runtime exits with a non-zero code�[0m
�[36;1m# (which would otherwise cause set -e to abort the shell before we get here).�[0m
�[36;1m# The trap preserves the original exit code.�[0m
�[36;1mcopilot_cleanup() {�[0m
�[36;1m �[0m
�[36;1m if [ -n "${GIT_PROXY_PID:-}" ] && kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "Stopping git-proxy (pid=$GIT_PROXY_PID)..."�[0m
�[36;1m kill "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m for _ in {1..25}; do�[0m
�[36;1m if ! kill -0 "$GIT_PROXY_PID" 2>/dev/null; then break; fi�[0m
�[36;1m sleep 0.2�[0m
�[36;1m done�[0m
�[36;1m if kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "git-proxy did not stop gracefully; forcing termination."�[0m
�[36;1m kill -KILL "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m wait "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m �[0m
�[36;1m echo "::$STOP_***REDACTED_SECRET_ASSIGNMENT***
�[36;1m FALLBACK_FILE="${RUNNER_TEMP}/copilot-fallback-error.txt"�[0m
�[36;1m if [ -f "$FALLBACK_FILE" ]; then�[0m
�[36;1m FALLBACK_MSG=$(head -c 500 "$FALLBACK_FILE" | tr -d '\n\r')�[0m
�[36;1m echo "::error title=Copilot Error::${FALLBACK_MSG}"�[0m
GitHub Actions: Code scanning AI findings on PR #164 / github-advanced-security: Code scanning AI findings on PR #164
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "RUNNER_TEMP=$RUNNER_TEMP"�[0m
�[36;1mfind "$RUNNER_TEMP" -maxdepth 1 -type f -name 'git-credentials-*.config' -print -delete�[0m
�[36;1m�[0m
�[36;1m# Generate a unique token and stop processing workflow commands to prevent the runtime from injecting commands�[0m
�[36;1mSTOP_***REDACTED_SECRET_ASSIGNMENT*** /proc/sys/kernel/random/uuid)�[0m
�[36;1m�[0m
�[36;1m# Use a trap to ensure we always resume command processing and check for�[0m
�[36;1m# fallback error annotations, even if the runtime exits with a non-zero code�[0m
�[36;1m# (which would otherwise cause set -e to abort the shell before we get here).�[0m
�[36;1m# The trap preserves the original exit code.�[0m
�[36;1mcopilot_cleanup() {�[0m
�[36;1m �[0m
�[36;1m if [ -n "${GIT_PROXY_PID:-}" ] && kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "Stopping git-proxy (pid=$GIT_PROXY_PID)..."�[0m
�[36;1m kill "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m for _ in {1..25}; do�[0m
�[36;1m if ! kill -0 "$GIT_PROXY_PID" 2>/dev/null; then break; fi�[0m
�[36;1m sleep 0.2�[0m
�[36;1m done�[0m
�[36;1m if kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "git-proxy did not stop gracefully; forcing termination."�[0m
�[36;1m kill -KILL "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m wait "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m �[0m
�[36;1m echo "::$STOP_***REDACTED_SECRET_ASSIGNMENT***
�[36;1m FALLBACK_FILE="${RUNNER_TEMP}/copilot-fallback-error.txt"�[0m
�[36;1m if [ -f "$FALLBACK_FILE" ]; then�[0m
�[36;1m FALLBACK_MSG=$(head -c 500 "$FALLBACK_FILE" | tr -d '\n\r')�[0m
�[36;1m echo "::error title=Copilot Error::${FALLBACK_MSG}"�[0m
🧰 Additional context used
📓 Path-based instructions (12)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Support Python 3.11 and newer; use Python 3.11+ features where required.
Keep the core zero-dependency and use only the standard library; external dependencies must be isolated behind optional integrations.
Use injectedClockinstances for all time access; do not calltime.monotonic()orsleep()directly in logic.
Implement the core as an I/O-free state machine with a singlethreading.Lockaround the await-free critical section, never held across the protected call.
UseProtocols for extension points:Clock,SlidingWindow,Storage,FailureClassifier, andEventListener; do not inherit from internal classes.
Expose one publicCircuitBreakerclass for sync and async operation, with separate internal paths selected by coroutine detection.
Expose the public API through the package__init__.py; keep helpers underscore-prefixed and hidden.
Use absolute imports, placed at the top of the file, ordered as standard library, third-party, then local imports with blank lines between groups.
Use a maximum line length of 100 characters, single-quoted strings, f-strings, andpathlib.Pathinstead ofos.path.
Annotate every parameter and return value; use modern generic syntax andX | Noneinstead ofOptional[X].
UseStrEnumor module-level constants instead of magic constants.
When a constructor or function has three or more arguments, pass them by keyword.
Keep functions focused on one job, generally no longer than 20–30 lines, with minimal side effects and extracted repeated loop logic.
Useasync/awaitfor I/O-bound work,asyncio.TaskGroupinstead ofasyncio.gather, andasyncio.to_threadorProcessPoolExecutorfor CPU-bound work.
Do not mix sync and async in one function; never await a sync callable or block on an async callable.
Fail fast on invalid input or state by raising immediately; do not continue with partial results or invented defaults.
Catch only expected exceptions, log them with context, and re-raise; do not use ...
Files:
tests/test_observability.pyinterlock/state.pyinterlock/_engine.pyinterlock/_state_machine.pyinterlock/breaker.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Use pytest functions rather than test classes, with names formatted astest__unit_of_work__state_under_test__expected_behavior.
Mirror package layout in test filenames, use Arrange-Act-Assert, and create fixtures for repeated setup.
Use injectedClockinstances for deterministic tests; do not usesleep()in tests.
Usepytest-asyncioand@pytest.mark.asynciofor asynchronous tests, and usepytest-mockto isolate external dependencies.
Use Hypothesis property-based tests for the state machine and cover all transitions and races.
Write the reproducing test before a bug fix and specify the required behavior before implementing a feature.
Files:
tests/test_observability.py
⚙️ CodeRabbit configuration file
pytest functions only, never test classes. Names follow
test__unit_of_work__state_under_test__expected_behaviorin lower case. One behaviour per test, Arrange-Act-Assert. Time is the injected fake Clock — any real sleep or wall-clock read is flakiness, flag it. Async tests use@pytest.mark.asyncio; state-machine work carries hypothesis property tests. Coverage must stay at 100%: point out uncovered branches the diff introduces. Tests run under-n auto, so anything relying on ordering or shared global state is a bug.
Files:
tests/test_observability.py
{interlock/**/*.py,docs/**/*.md,docs/llms-full.txt,docs/llms.txt}
📄 CodeRabbit inference engine (Custom checks)
When a change affects user-facing behaviour through the public API, integrations, or configuration options, update the relevant page under
docs/and regeneratedocs/llms-full.txt; when adding a new documentation page, list it under## Docsindocs/llms.txt.
Files:
interlock/state.pyinterlock/_engine.pyinterlock/_state_machine.pyinterlock/breaker.pydocs/guides/states.mddocs/llms-full.txt
interlock/**/*.py
📄 CodeRabbit inference engine (Custom checks)
Every production behaviour change in
interlock/must be accompanied by a change undertests/; changes limited to docstrings, comments, or type annotations are exempt. Bug fixes must include at least one regression test that fails without the production fix.
Files:
interlock/state.pyinterlock/_engine.pyinterlock/_state_machine.pyinterlock/breaker.py
⚙️ CodeRabbit configuration file
Core rules (AGENTS.md is authoritative): (1) Zero-dependency core — anything under interlock/ except interlock/integrations/ may import stdlib only. Flag every third-party import as a blocking issue. (2) No fallbacks, no silent excepts, no
a or b or cfor required config or data, no hidden retries. Invalid input or state raises immediately. interlock/_notify.py is the one sanctioned swallow (listener hooks are observability, logged with traceback, BaseException still propagates) — do not suggest generalising or "fixing" it. (3) Time comes only from the injected Clock protocol; direct time.monotonic()/time.sleep() in library logic is a bug. (4) Style: 100-char lines, single quotes, f-strings, pathlib, full annotations,X | NoneneverOptional[X], keyword arguments for calls with 3+ arguments, no magic constants (StrEnum or module constants), functions under ~30 lines. (5) Extension points are Protocols (Clock, SlidingWindow, Storage, FailureClassifier, EventListener) — do not propose inheriting internal classes. (6) Sync and async live in one CircuitBreaker with separate internal paths; never propose Sync*/Async* twins and never mix the two paths in one function. (7) Public API is exported from interlock/init.py; everything else is underscore-prefixed. New public symbols need__all__and a docstring. (8) Python 3.11 is the floor — no 3.12+ syntax or stdlib.
Files:
interlock/state.pyinterlock/_engine.pyinterlock/_state_machine.pyinterlock/breaker.py
{interlock/*.py,interlock/!(integrations)/**/*.py,pyproject.toml}
📄 CodeRabbit inference engine (Custom checks)
Keep the core zero-dependency: files under
interlock/outsideinterlock/integrations/may import only the standard library or otherinterlockmodules;[project] dependenciesinpyproject.tomlmust remain empty; andinterlock/__init__.pymust not re-export frominterlock.integrations.
Files:
interlock/state.pyinterlock/_engine.pyinterlock/_state_machine.pyinterlock/breaker.py
interlock/_engine.py
📄 CodeRabbit inference engine (AGENTS.md)
Run mutation testing with
mutmutwhenever_engine.pyis changed.
Files:
interlock/_engine.py
interlock/{_state_machine,_engine,_coordination}.py
⚙️ CodeRabbit configuration file
The critical section. Verify: the state machine stays I/O-free and unaware of sync vs async; the threading.Lock covers only the await-free acquire and record sections and is never held across the protected call (a call under the lock is a deadlock and a throughput bug); HALF_OPEN still bounds concurrent probes. Ask whether
uv run mutmut runwas run — AGENTS.md requires it for these files, because 100% coverage here does not prove the new assertions bite.
Files:
interlock/_engine.pyinterlock/_state_machine.py
interlock/_state_machine.py
📄 CodeRabbit inference engine (AGENTS.md)
Run mutation testing with
mutmutwhenever_state_machine.pyis changed.
Files:
interlock/_state_machine.py
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Document user-facing changes in English Markdown documentation and keep generated documentation mirrors synchronized.
Files:
docs/guides/states.mdCHANGELOG.md
docs/**/*.md
⚙️ CodeRabbit configuration file
User-facing documentation. Check that code samples match the current public API and would actually run. A new page must also be listed in docs/llms.txt under
## Docs. Keep the existing voice: short sentences, no marketing.
Files:
docs/guides/states.md
docs/llms-full.txt
⚙️ CodeRabbit configuration file
Generated artefact — produced by
uv run python scripts/build_llms_full.py. Do not review its content or suggest edits; only confirm it was regenerated together with the docs/ changes in the same PR.
Files:
docs/llms-full.txt
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Add every change to the
[Unreleased]section underAdded,Fixed, orChanged, explaining user impact rather than only symbol movement.
Files:
CHANGELOG.md
⚙️ CodeRabbit configuration file
Keep a Changelog format. New entries go under
## [Unreleased]in Added / Fixed / Changed. An entry describes what a user could not do before and can now, not which symbol moved. Only the release commit dates a section and updates the link references.
Files:
CHANGELOG.md
🧠 Learnings (3)
📚 Learning: 2026-08-07T15:34:32.493Z
Learnt from: CR
Repo: bagowix/interlock PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-07T15:34:32.493Z
Learning: Applies to **/*.py : Expose one public `CircuitBreaker` class for sync and async operation, with separate internal paths selected by coroutine detection.
Applied to files:
interlock/breaker.py
📚 Learning: 2026-08-07T15:34:32.493Z
Learnt from: CR
Repo: bagowix/interlock PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-07T15:34:32.493Z
Learning: Applies to CHANGELOG.md : Add every change to the `[Unreleased]` section under `Added`, `Fixed`, or `Changed`, explaining user impact rather than only symbol movement.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-07T15:34:59.595Z
Learnt from: CR
Repo: bagowix/interlock PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-08-07T15:34:59.595Z
Learning: A changelog entry is required for PRs unless the PR prepares a release, changes only documentation-exempt paths such as `.github/`, `benchmarks/`, `planning/`, tests, non-published developer tooling, or root dot-files. Otherwise, add at least one user-visible bullet under `## [Unreleased]` in `CHANGELOG.md`.
Applied to files:
CHANGELOG.md
🔇 Additional comments (7)
interlock/state.py (1)
16-18: LGTM!interlock/_state_machine.py (1)
21-22: 📐 Maintainability & Code QualityConfirm one mutation run covers both changed critical modules.
Confirm that
uv run mutmut runcovered both changed files.
interlock/_state_machine.py#L21-L22: verify mutation testing for the changed state-machine module.interlock/_state_machine.py#L170-L175: verify mutation testing for the changeddisable()documentation segment.interlock/_engine.py#L268-L272: verify mutation testing for the changed engine module.As per path instructions: both
_state_machine.pyand_engine.pyrequire mutation testing whenever changed.Source: Path instructions
interlock/breaker.py (1)
147-153: LGTM!docs/guides/states.md (1)
67-67: LGTM!Also applies to: 77-98
docs/llms-full.txt (1)
1639-1639: 📐 Maintainability & Code QualityConfirm regeneration from the Markdown source.
Confirm that
uv run python scripts/build_llms_full.pygenerated these lines after thedocs/guides/states.mdchange. Do not edit this file independently.As per path instructions:
docs/llms-full.txtis a generated artefact and must be regenerated with the documentation changes.Also applies to: 1649-1674
Source: Path instructions
CHANGELOG.md (1)
40-49: LGTM!tests/test_observability.py (1)
142-154: LGTM!
| Switching a rollout from `metrics_only()` to `disable()` therefore keeps | ||
| listener-exported dashboards alive, and only stops threshold evaluation and | ||
| `snapshot()`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that snapshot() remains available.
disable() stops new window data and threshold evaluation. It does not stop snapshot(). Count-based data remains, and time-based data can expire.
The generated mirror repeats this wording in docs/llms-full.txt Lines 1671-1673.
Proposed wording
-`snapshot()`.
+further `snapshot()` updates.As per path instructions: user-facing documentation must describe current behavior accurately and generated mirrors must stay synchronized.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Switching a rollout from `metrics_only()` to `disable()` therefore keeps | |
| listener-exported dashboards alive, and only stops threshold evaluation and | |
| `snapshot()`. | |
| Switching a rollout from `metrics_only()` to `disable()` therefore keeps | |
| listener-exported dashboards alive, and only stops threshold evaluation and | |
| further `snapshot()` updates. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/guides/states.md` around lines 99 - 101, Update the rollout behavior
description in the states documentation to explicitly state that disable() stops
new window data and threshold evaluation but leaves snapshot() available;
clarify that count-based data remains while time-based data may expire. Apply
the same wording to the corresponding generated section in docs/llms-full.txt so
both documents stay synchronized.
Source: Path instructions
1 participant
Summary
State.DISABLEDanddisable()were documented as "admit all traffic, record nothing", but only the sliding window ever goes quiet.StateMachine.recordsimply falls through forDISABLED; everything else inEngine._settleruns unconditionally after every admitted call — classifier, duration, outcome andnotify(listener, 'on_call', ...). So aLoggingEventListener, theOTelEventListeneror a Prometheus exporter keeps reporting fully classified outcomes and durations for a disabled breaker, whilesnapshot()gets nothing new and no threshold is evaluated.Two observability surfaces disagreed about whether the breaker is a no-op, and the docstrings described only one of them. This takes the documentation fix from #159 (the alternative — suppressing
on_callinDISABLED— was rejected there: it is a behaviour change for existing listeners, and it makes an operator action look like an outage on every dashboard).Changed:
State.DISABLED,CircuitBreaker.disable(),Engine.disable(),StateMachine.disable()and the_state_machinemodule header;docs/guides/states.md— the operator-override table row, plus a new "What an override does to your metrics" section contrasting the window and the listener, and the observability semantics ofMETRICS_ONLYvsDISABLEDfor a rollout switch.Two points of precision worth calling out, since the issue's own wording glosses over them:
snapshot()does not "freeze". A count-based window keeps its last contents unchanged, but a time-based one drains as its buckets expire (_windows.py—snapshotsums only buckets still inside the window). The docs say "gets nothing new", not "freezes".on_callfires when an admitted call settles. A call interrupted by aBaseExceptiongoes throughEngine._release, not_settle, so it emits nothing.Docs only; behaviour is unchanged. One test is added —
test__disable__admitted_call__still_reports_on_callpins the semantics the docs now promise, so the "docs and code disagree" state cannot come back silently.Checklist
uv run pytest --cov: 772 passed, 2 skipped, total coverage 100.00%uv run ruff format --checkanduv run ruff checkpassuv run mypy,uv run pyrightanduv run pyrefly checkpassdocs/) for user-facing changes —docs/guides/states.md+ regenerateddocs/llms-full.txtCHANGELOG.md[Unreleased]updated (Fixed)Related issues
Closes #159.
Changed
State.DISABLEDandCircuitBreaker.disable(): calls pass, outcomes are not recorded, snapshots receive no data, and thresholds are not evaluated.on_calllisteners.DISABLED,METRICS_ONLY, andFORCED_OPENfor rollout decisions.Engine.disable(),CircuitBreaker.disable(), and state documentation.Added
on_callevents from calls admitted while disabled.