mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(sequencing): reachability-aware claim bar + sequence_held surfacing (#681)
Three coupled claim-path bugs from the 2026-07-24 live incident, fixed at the shared root: - The edge-agnostic sequence bar phantom-held a task behind an unrelated, never-connected same-parent sibling that coincidentally shared a lower raw sequence (stamp_wave_sequence stamps from a partial per-task view). _claim_blocked_by_sequence now branches on is_batch_root_subtask: a MegaTask root-subtask (globally-computed Kahn wave, a deliberate staged-release barrier) keeps the strict rule unchanged; every other same-parent context routes through the pure sequence_blocker_id, which only blocks on a real transitive predecessor via dependency_ids UNIONED with completed_dependency_ids. A task with no same-parent dependency edge at all falls back to the raw bar unchanged (#452 preserved). - The hold surfaced as claim()'s bare None and was misdiagnosed by the verb runner as a concurrent-transition invalid_state. New sequence_hold_reason + a proactive _sequencing_claim_guard return a dedicated Envelope.sequence_held naming the blocker, on both the PENDING and NEEDS_REVISION reclaim paths. - give_me_work offered tasks the claim gate then rejected: both offer paths (list_pending_for_agent, _drop_dependency_held) now consult the bar via the exact claim predicate (is_pending_claim_blocked, extended to NEEDS_REVISION). Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -117,7 +117,7 @@ def test_sibling_sequence_blocks_claim_until_earlier_sibling_terminal(
|
||||
# No wire_dependency() call anywhere in this test — sequence alone must
|
||||
# hold the order; seq0 stays PENDING (open, non-terminal).
|
||||
|
||||
expect_error(
|
||||
env = expect_error(
|
||||
main_pm.flow(
|
||||
"i_will_plan",
|
||||
task_id=str(seq1_id),
|
||||
@@ -125,9 +125,10 @@ def test_sibling_sequence_blocks_claim_until_earlier_sibling_terminal(
|
||||
approach=_APPROACH,
|
||||
sub_tasks=_SUB_TASKS,
|
||||
),
|
||||
"invalid_state",
|
||||
"sequence_held",
|
||||
"main_pm i_will_plan seq-1 while seq-0 open (no dependency edge)",
|
||||
)
|
||||
assert "Revision 0" in (env.get("message") or "")
|
||||
assert task_state(stack, seq1_id)["status"] == "pending"
|
||||
|
||||
_cancel(stack, seq0_id)
|
||||
|
||||
@@ -1737,11 +1737,17 @@ async def test_is_pending_claim_blocked_false_for_missing_task(
|
||||
async def test_claim_batch_wave_blocked_by_all_wave0_siblings_no_edges(
|
||||
task_setup: dict, db_session: AsyncSession
|
||||
) -> None:
|
||||
"""STRICTER than dependency edges where both exist: a wave-1 root-subtask
|
||||
waits for EVERY wave-0 sibling, not just the one edge target the
|
||||
collision analyzer happened to wire — no edges-exist exemption."""
|
||||
"""STRICTER than dependency edges where both exist: a MegaTask wave-1
|
||||
root-subtask waits for EVERY wave-0 root-subtask, not just the one edge
|
||||
target the collision analyzer happened to wire — no edges-exist
|
||||
exemption. Scoped to `batch_id`-bearing root-subtasks specifically
|
||||
(`_build_confirm_batch` stamps `sequence` as a one-shot, globally
|
||||
computed Kahn wave index — a deliberate staged-release barrier); a
|
||||
plain (non-batch) same-parent sibling instead needs a real dependency
|
||||
path — see `test_claim_not_blocked_by_unconnected_sibling_in_different_stream`."""
|
||||
svc = task_setup["svc"]
|
||||
umbrella = await svc.create(_req(task_setup, title="umbrella"))
|
||||
batch_id = uuid4()
|
||||
wave0_a = await svc.create(
|
||||
_req(task_setup, title="wave0-a", parent_task_id=umbrella.id, sequence=0)
|
||||
)
|
||||
@@ -1756,6 +1762,12 @@ async def test_claim_batch_wave_blocked_by_all_wave0_siblings_no_edges(
|
||||
# never wires an edge to it.
|
||||
await svc.add_dependency(wave1.id, wave0_a.id)
|
||||
wave0_a.status = TaskStatus.COMPLETED
|
||||
# Direct ORM stamp (mirrors `_build_confirm_batch`'s BatchPlacement,
|
||||
# bypassing create-time batch-shape validation the same way the rest of
|
||||
# this test pokes `.status` directly).
|
||||
wave0_a.batch_id = batch_id
|
||||
wave0_b.batch_id = batch_id
|
||||
wave1.batch_id = batch_id
|
||||
await db_session.flush()
|
||||
|
||||
# The edge is satisfied, but wave0_b is still open with a lower sequence.
|
||||
@@ -1770,6 +1782,94 @@ async def test_claim_batch_wave_blocked_by_all_wave0_siblings_no_edges(
|
||||
assert claimed.status == TaskStatus.CLAIMED
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_claim_not_blocked_by_unconnected_sibling_in_different_stream(
|
||||
task_setup: dict, db_session: AsyncSession
|
||||
) -> None:
|
||||
"""The 2026-07-24 live incident: a frontend cell with 4 independent
|
||||
dev-task streams — stream1-b (stamped wave 1, a real dependency on
|
||||
stream1-a) must not be phantom-held by stream4-b (wave 0, in_progress)
|
||||
just because they share a same parent and a lower raw sequence number.
|
||||
Unlike the MegaTask-batch case above (no `batch_id` here), a plain
|
||||
same-parent sibling only blocks via a real dependency path."""
|
||||
svc = task_setup["svc"]
|
||||
cell = await svc.create(_req(task_setup, title="cell"))
|
||||
stream1_a = await svc.create(
|
||||
_req(task_setup, title="stream1-a", parent_task_id=cell.id)
|
||||
)
|
||||
stream1_b = await svc.create(
|
||||
_req(task_setup, title="stream1-b", parent_task_id=cell.id)
|
||||
)
|
||||
stream4_b = await svc.create(
|
||||
_req(task_setup, title="stream4-b", parent_task_id=cell.id)
|
||||
)
|
||||
await svc.add_dependency(stream1_b.id, stream1_a.id)
|
||||
await svc.stamp_wave_sequence(stream1_a.id)
|
||||
await svc.stamp_wave_sequence(stream1_b.id)
|
||||
await svc.stamp_wave_sequence(stream4_b.id)
|
||||
assert (stream1_a.sequence, stream1_b.sequence, stream4_b.sequence) == (0, 1, 0)
|
||||
|
||||
# stream1-a (the REAL predecessor) is done; stream4-b (an unconnected
|
||||
# sibling in a different stream) is still open with a lower sequence.
|
||||
stream1_a.status = TaskStatus.COMPLETED
|
||||
stream4_b.status = TaskStatus.IN_PROGRESS
|
||||
stream1_b.branch_name = "feature/frontend/aaaa9999"
|
||||
await db_session.flush()
|
||||
|
||||
claimed = await svc.claim(stream1_b.id, task_setup["agent_id"])
|
||||
assert claimed is not None, (
|
||||
"an unconnected sibling in a different stream must not phantom-hold"
|
||||
)
|
||||
assert claimed.status == TaskStatus.CLAIMED
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_claim_not_blocked_after_real_dependency_pruned_on_completion(
|
||||
task_setup: dict, db_session: AsyncSession
|
||||
) -> None:
|
||||
"""The precise drift mechanic: `_unblock_dependents` strips a completed
|
||||
dependency from the live `dependency_ids` (moving it to
|
||||
`completed_dependency_ids`) the moment it completes — almost always
|
||||
BEFORE the dependent is ever claimed. The sequence claim-gate must
|
||||
still recognize the pruned edge as real graph info (via
|
||||
`completed_dependency_ids`) rather than treating the now-edge-less task
|
||||
as a manually-sequenced, edge-less chain and reviving the raw
|
||||
strictly-lower-sequence bar against an unrelated sibling."""
|
||||
svc = task_setup["svc"]
|
||||
cell = await svc.create(_req(task_setup, title="cell"))
|
||||
real_predecessor = await svc.create(
|
||||
_req(task_setup, title="real predecessor", parent_task_id=cell.id)
|
||||
)
|
||||
dependent = await svc.create(
|
||||
_req(task_setup, title="dependent", parent_task_id=cell.id)
|
||||
)
|
||||
unrelated = await svc.create(
|
||||
_req(task_setup, title="unrelated stream", parent_task_id=cell.id)
|
||||
)
|
||||
await svc.add_dependency(dependent.id, real_predecessor.id)
|
||||
await svc.stamp_wave_sequence(real_predecessor.id)
|
||||
await svc.stamp_wave_sequence(dependent.id)
|
||||
await svc.stamp_wave_sequence(unrelated.id)
|
||||
assert dependent.sequence == 1
|
||||
|
||||
# Simulate `_unblock_dependents`'s exact effect: the completed
|
||||
# predecessor's edge is pruned from the live column and moved to the
|
||||
# completed ledger.
|
||||
real_predecessor.status = TaskStatus.COMPLETED
|
||||
dependent.dependency_ids = []
|
||||
dependent.completed_dependency_ids = [real_predecessor.id]
|
||||
unrelated.status = TaskStatus.IN_PROGRESS
|
||||
dependent.branch_name = "feature/frontend/bbbb8888"
|
||||
await db_session.flush()
|
||||
|
||||
claimed = await svc.claim(dependent.id, task_setup["agent_id"])
|
||||
assert claimed is not None, (
|
||||
"a pruned-but-once-real edge must still count as graph info, not "
|
||||
"revert to the raw edge-less sequence bar"
|
||||
)
|
||||
assert claimed.status == TaskStatus.CLAIMED
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_needs_revision_reclaim_blocked_by_lower_sequence_sibling(
|
||||
task_setup: dict, db_session: AsyncSession
|
||||
|
||||
@@ -22,6 +22,7 @@ from roboco.services.sequencing import (
|
||||
by_osmosis_tail_dev_tasks,
|
||||
cell_task_wave_chain_depends_on,
|
||||
dev_task_collision_edges,
|
||||
sequence_blocker_id,
|
||||
)
|
||||
|
||||
|
||||
@@ -594,3 +595,75 @@ def test_declared_cycle_rejected() -> None:
|
||||
]
|
||||
with pytest.raises(SequencingError):
|
||||
SequencingService().analyze(s, _backend, {"backend": 2})
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# sequence_blocker_id — the claim-gate's non-batch reachability decision
|
||||
# (the 2026-07-24 phantom cross-stream serialization fix).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_sequence_blocker_no_graph_info_falls_back_to_raw_first_candidate() -> None:
|
||||
"""No dependency edge onto ANY same-parent sibling at all — the #452
|
||||
manually-sequenced, edge-less scenario — keeps the strict raw bar: the
|
||||
first (lowest-sequence) candidate blocks unconditionally."""
|
||||
a, b = uuid4(), uuid4()
|
||||
assert (
|
||||
sequence_blocker_id(
|
||||
task_dependency_ids=[],
|
||||
candidate_ids=[a, b],
|
||||
sibling_dependency_ids={a: [], b: []},
|
||||
)
|
||||
== a
|
||||
)
|
||||
|
||||
|
||||
def test_sequence_blocker_ignores_unconnected_sibling() -> None:
|
||||
"""A same-parent sibling reachable via NO edge (a different stream) must
|
||||
not block once real graph info exists elsewhere."""
|
||||
real_predecessor, unrelated = uuid4(), uuid4()
|
||||
assert (
|
||||
sequence_blocker_id(
|
||||
task_dependency_ids=[real_predecessor],
|
||||
candidate_ids=[unrelated],
|
||||
sibling_dependency_ids={real_predecessor: [], unrelated: []},
|
||||
)
|
||||
is None
|
||||
)
|
||||
|
||||
|
||||
def test_sequence_blocker_finds_direct_predecessor() -> None:
|
||||
predecessor = uuid4()
|
||||
assert (
|
||||
sequence_blocker_id(
|
||||
task_dependency_ids=[predecessor],
|
||||
candidate_ids=[predecessor],
|
||||
sibling_dependency_ids={predecessor: []},
|
||||
)
|
||||
== predecessor
|
||||
)
|
||||
|
||||
|
||||
def test_sequence_blocker_transitive_two_hop() -> None:
|
||||
"""A candidate two hops away (through an already-terminal, non-candidate
|
||||
intermediate) is still found — a genuine ordering must still hold."""
|
||||
intermediate, root_blocker = uuid4(), uuid4()
|
||||
assert (
|
||||
sequence_blocker_id(
|
||||
task_dependency_ids=[intermediate],
|
||||
candidate_ids=[root_blocker],
|
||||
sibling_dependency_ids={intermediate: [root_blocker], root_blocker: []},
|
||||
)
|
||||
== root_blocker
|
||||
)
|
||||
|
||||
|
||||
def test_sequence_blocker_no_candidates_is_none() -> None:
|
||||
assert (
|
||||
sequence_blocker_id(
|
||||
task_dependency_ids=[uuid4()],
|
||||
candidate_ids=[],
|
||||
sibling_dependency_ids={},
|
||||
)
|
||||
is None
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user