feat(core): add StallDetector and StallAction (#400) - #424
Conversation
Standalone synchronous stall detector that tracks time since last agent activity. Complements the threaded StallMonitor with a simpler API for synchronous checks. StallAction enum defines recovery strategies (retry, blocker, fail).
WalkthroughAdds a standalone stall detection utility: a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/test_stall_detector.py (1)
93-96: Consider increasing sleep margin for CI reliability.While 10ms sleep vs 1ms timeout should work, CI environments under load can experience scheduling delays. A slightly larger margin could prevent intermittent failures.
♻️ Optional: Increase sleep margin
def test_very_short_timeout_stalls_quickly(self, short_detector): # With 1ms timeout, should be stalled almost immediately - time.sleep(0.01) + time.sleep(0.05) # 50ms to ensure CI reliability assert short_detector.is_stalled() is TrueAlternatively, this test could use the backdating pattern like the other time-sensitive tests for full determinism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_stall_detector.py` around lines 93 - 96, The test test_very_short_timeout_stalls_quickly is flaky on CI because it sleeps only 0.01s against a 0.001s timeout; increase the sleep margin (e.g., change time.sleep(0.01) to time.sleep(0.05)) to reduce scheduling-related failures, or alternatively adopt the deterministic backdating pattern used in other tests by adjusting the detector's timestamp before calling short_detector.is_stalled(); locate the test function name test_very_short_timeout_stalls_quickly and the short_detector fixture/is_stalled() call to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/test_stall_detector.py`:
- Around line 93-96: The test test_very_short_timeout_stalls_quickly is flaky on
CI because it sleeps only 0.01s against a 0.001s timeout; increase the sleep
margin (e.g., change time.sleep(0.01) to time.sleep(0.05)) to reduce
scheduling-related failures, or alternatively adopt the deterministic backdating
pattern used in other tests by adjusting the detector's timestamp before calling
short_detector.is_stalled(); locate the test function name
test_very_short_timeout_stalls_quickly and the short_detector
fixture/is_stalled() call to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 273f9671-45c3-4349-bacf-d1fe599830bc
📒 Files selected for processing (3)
codeframe/core/__init__.pycodeframe/core/stall_detector.pytests/core/test_stall_detector.py
|
Review: StallDetector + StallAction (closes #400) Overall this is a clean, well-scoped primitive - minimal deps, good docstrings, correct use of time.monotonic(). A few things worth addressing before merge. Issues Issue 1: StallMonitor doc claims it wraps StallDetector but it does not stall_detector.py module docstring says: "Use StallMonitor (in stall_monitor.py) for the threaded watchdog that wraps this logic." But StallMonitor does not use StallDetector at all - it has its own parallel datetime.now(timezone.utc) tracking. This leaves two divergent time-tracking approaches (time.monotonic() here vs datetime in the monitor). Either update the docstring to say these are independent implementations, or refactor StallMonitor._watch_loop to delegate to StallDetector internally (preferred, since StallDetector uses the safer monotonic clock). Issue 2: StallAction is exported but not wired anywhere StallAction is now a public export from codeframe.core, but no existing code - not StallMonitor, ReactAgent, or runtime.py - references it. There is no recovery path that dispatches on RETRY / BLOCKER / FAIL. As a building block this is fine, but it should either be wired in a follow-up immediately or noted explicitly in the PR description as a placeholder. Issue 3: One test uses time.sleep - inconsistent with the rest of the suite The test test_very_short_timeout_stalls_quickly uses time.sleep(0.01) while all other time-sensitive tests in this file correctly backdate _last_activity to avoid real sleeps. This can be flaky on slow CI. The recommended fix is: Minor observations
What is good
Summary: Two items to fix before merge - the misleading "wraps this logic" doc and the time.sleep in one test. Track StallAction wiring as a follow-up issue if not addressed here. |
|
Follow-up review (refs my earlier comment) Fixed: The time.sleep in test_very_short_timeout_stalls_quickly is now using the deterministic backdating pattern (_last_activity = time.monotonic() - 1.0) — consistent with all other time-sensitive tests in the suite. Good fix. Still open from previous review:
Nothing blocking merge on the test side now that the sleep issue is resolved, but the module docstring correction is low-effort and worth landing before this ships. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/test_stall_detector.py`:
- Around line 58-65: Add a test that asserts the exact-threshold behavior by
setting detector._last_activity to time.monotonic() - 300 and asserting
detector.is_stalled() is False (or True depending on spec) to catch regressions
from > to >=; implement this as a new test function (e.g.,
test_stalled_at_timeout) alongside test_stalled_after_timeout and
test_not_stalled_before_timeout and, if needed, use a fixed/mockable clock or
capture a baseline monotonic value in the test to avoid flakiness when computing
the exact 300-second offset before calling is_stalled().
- Around line 40-53: Tests and fixtures currently assert a seconds-based API
(timeout_s) but the public API should be millisecond-based (stall_timeout_ms)
with default 300_000; update the fixtures and assertions to use
StallDetector(stall_timeout_ms=...) and check .stall_timeout_ms instead of
.timeout_s (change the detector and short_detector fixtures, plus
test_default_timeout, test_custom_timeout and the other tests around lines
83-96) and set short_detector to a small millisecond value (e.g., 1) to preserve
the quick timeout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa637e94-cb27-49f2-b685-7c8f32c8df0a
📒 Files selected for processing (1)
tests/core/test_stall_detector.py
| @pytest.fixture | ||
| def detector(self): | ||
| return StallDetector() | ||
|
|
||
| @pytest.fixture | ||
| def short_detector(self): | ||
| return StallDetector(timeout_s=0.001) | ||
|
|
||
| def test_default_timeout(self, detector): | ||
| assert detector.timeout_s == 300 | ||
|
|
||
| def test_custom_timeout(self): | ||
| d = StallDetector(timeout_s=600) | ||
| assert d.timeout_s == 600 |
There was a problem hiding this comment.
These tests are locking in the wrong timeout API.
Line 46, Line 49, Line 52, Line 84, and Line 89 all encode a seconds-based timeout_s contract, but issue #400 calls for a millisecond-based stall_timeout_ms public API with a default of 300_000. If this lands as-is, the test suite will bless the wrong interface and make the issue-compliant implementation look like a regression.
🧪 Align the tests with the issue contract
`@pytest.fixture`
def short_detector(self):
- return StallDetector(timeout_s=0.001)
+ return StallDetector(stall_timeout_ms=1)
def test_default_timeout(self, detector):
- assert detector.timeout_s == 300
+ assert detector.stall_timeout_ms == 300_000
def test_custom_timeout(self):
- d = StallDetector(timeout_s=600)
- assert d.timeout_s == 600
+ d = StallDetector(stall_timeout_ms=600_000)
+ assert d.stall_timeout_ms == 600_000
def test_disabled_when_zero(self):
- d = StallDetector(timeout_s=0)
+ d = StallDetector(stall_timeout_ms=0)
def test_disabled_when_negative(self):
- d = StallDetector(timeout_s=-1)
+ d = StallDetector(stall_timeout_ms=-1)Also applies to: 83-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_stall_detector.py` around lines 40 - 53, Tests and fixtures
currently assert a seconds-based API (timeout_s) but the public API should be
millisecond-based (stall_timeout_ms) with default 300_000; update the fixtures
and assertions to use StallDetector(stall_timeout_ms=...) and check
.stall_timeout_ms instead of .timeout_s (change the detector and short_detector
fixtures, plus test_default_timeout, test_custom_timeout and the other tests
around lines 83-96) and set short_detector to a small millisecond value (e.g.,
1) to preserve the quick timeout behavior.
| def test_stalled_after_timeout(self, detector): | ||
| # Simulate time passing by backdating _last_activity | ||
| detector._last_activity = time.monotonic() - 301 | ||
| assert detector.is_stalled() is True | ||
|
|
||
| def test_not_stalled_before_timeout(self, detector): | ||
| detector._last_activity = time.monotonic() - 299 | ||
| assert detector.is_stalled() is False |
There was a problem hiding this comment.
Please add the exact-threshold case.
Line 60 and Line 64 only verify “before” and “after” the cutoff. They would not catch a regression from > to >=, which is the key threshold behavior this API is supposed to define. One equality-boundary assertion with a fixed clock would close that gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/test_stall_detector.py` around lines 58 - 65, Add a test that
asserts the exact-threshold behavior by setting detector._last_activity to
time.monotonic() - 300 and asserting detector.is_stalled() is False (or True
depending on spec) to catch regressions from > to >=; implement this as a new
test function (e.g., test_stalled_at_timeout) alongside
test_stalled_after_timeout and test_not_stalled_before_timeout and, if needed,
use a fixed/mockable clock or capture a baseline monotonic value in the test to
avoid flakiness when computing the exact 300-second offset before calling
is_stalled().
1 participant
Summary
StallDetectorclass — synchronous, non-threaded stall detection primitive withrecord_activity(),is_stalled(), andelapsed_since_activity_ms()StallActionenum — recovery action options (RETRY, BLOCKER, FAIL)codeframe.coreComplements the existing threaded
StallMonitor(from #399) with a simpler synchronous API.Closes #400
Test plan
Summary by CodeRabbit
New Features
Tests