Skip to content

Commit f28b02e

Browse files
committed
fix(core): keep BLOCKED tasks under reconciliation, and stop the cache guard no-op'ing (#1032)
CI review on #1079. The reconciliation sweep selected tasks whose batch result is None or RUNNING. Once a task's first attempt reported BLOCKED, that entry is neither — and nothing clears it mid-batch — so the task was excluded from every later sweep. That silently disabled BOTH the pre-existing blocker-resolved requeue and the blocked-task GitHub check added two commits ago: the code ran, nothing ever reached it. A blocked task is precisely the one whose state changes from outside, whether by a human answering or by the linked issue being closed. BLOCKED now joins None/RUNNING; COMPLETED and FAILED stay excluded. Cache runaway guard: clearing only _open_until does nothing once _closed alone reaches the cap, so past that point the guard fired on every lookup and freed nothing. Now clears _closed too when it is the half at the cap. Both pinned by tests, including one asserting a COMPLETED task is still excluded from sweeps.
1 parent 35f0c3e commit f28b02e

3 files changed

Lines changed: 83 additions & 3 deletions

File tree

codeframe/core/conductor.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2209,10 +2209,16 @@ def _pass() -> None:
22092209
"""One reconciliation sweep. Raises nothing the caller must handle."""
22102210
try:
22112211
# Get currently active task IDs from the batch
2212+
# BLOCKED belongs here, not just None/RUNNING (#1032). A blocked
2213+
# task is precisely the one whose state can change from outside —
2214+
# a human answering the blocker, or closing the linked GitHub
2215+
# issue. Excluding it meant both the blocker-resolved requeue and
2216+
# the GitHub check were unreachable the moment a task's first
2217+
# attempt reported BLOCKED, since nothing clears that entry
2218+
# mid-batch. COMPLETED and FAILED stay excluded: finished work.
22122219
active_ids = [
22132220
tid for tid in batch.task_ids
2214-
if batch.results.get(tid) is None
2215-
or batch.results.get(tid) == "RUNNING"
2221+
if batch.results.get(tid) in (None, "RUNNING", "BLOCKED")
22162222
]
22172223
if not active_ids:
22182224
return

codeframe/core/reconciliation.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,14 @@ def is_closed(self, task) -> bool:
176176
self._disable(f"GitHub issue lookup failed ({exc})")
177177
return False
178178

179+
# Runaway guard. Drop the open half first — it is the cheap one to
180+
# rebuild — and only clear the closed set if that alone is at the cap,
181+
# otherwise the guard would keep firing as a no-op once _closed filled
182+
# it (CI review).
179183
if len(self._closed) + len(self._open_until) >= _ISSUE_STATE_CACHE_MAX:
180-
self._open_until.clear() # the cheap half to rebuild
184+
self._open_until.clear()
185+
if len(self._closed) >= _ISSUE_STATE_CACHE_MAX:
186+
self._closed.clear()
181187

182188
if str(state).lower() == "closed":
183189
self._closed.add(key)

tests/core/test_external_completion_honored_1032.py

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,74 @@ def apply_changes(self, result, batch, procs):
346346
assert batch.results[target] == "COMPLETED"
347347

348348

349+
class TestBlockedTasksStayUnderReconciliation:
350+
"""A task recorded BLOCKED must keep being re-checked.
351+
352+
``_pass`` selected tasks whose result is None or RUNNING. Once a task's
353+
first attempt returned BLOCKED, that entry is neither — so the task was
354+
excluded from every later sweep for the rest of the batch. That silently
355+
disabled both the pre-existing blocker-resolved requeue and this branch's
356+
GitHub check for blocked tasks: the code ran, but nothing ever reached it.
357+
"""
358+
359+
def test_a_blocked_task_is_still_swept(self, workspace, three_tasks, monkeypatch):
360+
swept = []
361+
362+
class StubEngine:
363+
def __init__(self, workspace):
364+
pass
365+
366+
def check_all_active(self, ids):
367+
from codeframe.core.reconciliation import ReconciliationResult
368+
369+
swept.append(list(ids))
370+
return ReconciliationResult()
371+
372+
def apply_changes(self, result, batch, procs):
373+
pass
374+
375+
monkeypatch.setattr(
376+
"codeframe.core.reconciliation.ReconciliationEngine", StubEngine
377+
)
378+
379+
blocked = three_tasks[0]
380+
batch = _make_batch(workspace, three_tasks, {blocked: "BLOCKED"})
381+
conductor._start_reconciliation_thread(workspace, batch, interval_seconds=3600)
382+
383+
assert swept, "no sweep ran"
384+
assert blocked in swept[0], (
385+
"a BLOCKED task is excluded from reconciliation for the rest of "
386+
"the batch, so it can never be unblocked or completed externally"
387+
)
388+
389+
def test_a_completed_task_is_not_swept(self, workspace, three_tasks, monkeypatch):
390+
"""The exclusion is still right for genuinely finished work."""
391+
swept = []
392+
393+
class StubEngine:
394+
def __init__(self, workspace):
395+
pass
396+
397+
def check_all_active(self, ids):
398+
from codeframe.core.reconciliation import ReconciliationResult
399+
400+
swept.append(list(ids))
401+
return ReconciliationResult()
402+
403+
def apply_changes(self, result, batch, procs):
404+
pass
405+
406+
monkeypatch.setattr(
407+
"codeframe.core.reconciliation.ReconciliationEngine", StubEngine
408+
)
409+
410+
done = three_tasks[0]
411+
batch = _make_batch(workspace, three_tasks, {done: "COMPLETED"})
412+
conductor._start_reconciliation_thread(workspace, batch, interval_seconds=3600)
413+
414+
assert done not in swept[0]
415+
416+
349417
class TestEveryWriteSiteUsesTheGuard:
350418
def test_no_executor_writes_batch_results_directly(self):
351419
"""A guard that four of five call sites bypass guards nothing.

0 commit comments

Comments
 (0)