Repository navigation
fix(OMN-18296): terminalise a state_io workflow abandoned by a runtime restart, on a contract-declared bound - #3479
Conversation
…e restart, on a contract-declared bound
|
| Surface | Meaning | Blocks merge? |
|---|---|---|
| Review threads | Per-finding, posted by the reviewer | No (informational) |
Hostile Review Thread Gate |
Deterministic: unresolved hostile-reviewer threads exist | Fails until resolved (not yet a required context) |
degraded verdict |
Fewer than 2 models succeeded (infra) | No |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492)
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 2
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[MAJOR] hostile-reviewer (glm-review)
Bound sweeper bypasses _recovery_lock, enabling concurrent sweeps | _ensure_stale_rows_recovered serializes _recover_outbox_batches and _terminalise_abandoned_rows under _recovery_lock. The new _bound_sweeper_loop calls _terminalise_abandoned_rows with no lock, so a timer-driven sweep can run concurrently with a dispatch-driven sweep. The CAS on finalize makes row state safe, but publish-then-finalize sequences from two sweeps interleave arbitrarily, duplicate publish attempts within the dedupe window are u
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Sweeper task never cancelled; shutdown semantics unspecified | _bound_sweeper_task is created on first dispatch and runs 'forever'. Nothing cancels it on runtime shutdown. A cancelled-mid-publish task can leave a terminal published with the row unfinalized (recoverable, but noisy) or, worse, a partially drained outbox batch. The diff comment celebrates detachment from dispatch traffic but the plan specifies no lifecycle owner for the task. | Evidence: _bound_sweeper_task = asyncio.create_task(_bound_sweeper
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Unchecked sweeper task exception surfaces only as 'never retrieved' warnings | If create_task fails or _bound_sweeper_loop raises outside the per-sweep try (e.g., an error computing sleep, or CancelledError handling after task teardown), nothing awaits or observes the task. The 'if not _bound_sweeper_task.done()' restart path re-creates a task silently, but a dead task is only noticed at the next dispatch, on a lane the ticket itself says has no dispatch traffic left. The design defeats its own motivation:
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
TOCTOU between select and finalize on unbounded row set | select_abandoned_rows has no LIMIT. A backlog of thousands of abandoned rows is selected in one snapshot, then terminalised serially: each row's version is read once and CAS'd potentially minutes later. Long-lived sweeps widen the window in which a legitimate late advance bumps version (CAS fails, row retried, terminal re-published for a workflow that actually progressed), and the unbounded SELECT can materialize a large result set. Additionally, a r
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
_read_completion_bound error handling inconsistent and duplicates file I/O | FileNotFoundError returns None but PermissionError, IsADirectoryError, and yaml.YAMLError propagate raw. Elsewhere in the same wiring, _read_state_io reads the same contract file; the diff re-reads and re-parses it. Malformed top-level YAML then surfaces as a PyYAML exception at wiring rather than the ModelOnexError contract the docstring promises for 'malformed' blocks. | Evidence: except FileNotFoundError: return None; if not isi
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Terminalise loop publishes strictly serially per row | Each abandoned row incurs a full _publish_outbox_batch await plus a finalize await before the next row. Under a backlog (pod restart stranding many rows, the exact incident scenario), terminalisation of row N+1 waits on row N's broker round trip. With a 15-second sweep floor and unbounded rows, drain time is unbounded. | Evidence: for row in rows: ... await _publish_outbox_batch([entry]); await _finalize_outbox_row(...) | Fix: Batch publishes per sweep
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Tests assert timing with sleeps and rely on module-level constant monkeypatching | _fast_sweeps patches module constants and tests await asyncio.sleep(0.4) to let the sweeper fire. This is timing-coupled and can flake under loaded CI; the sweeper captures its interval at loop start, so any future change computing the interval lazily would silently break the harness. Tests are also marked unit while living in tests/integration. | Evidence: await asyncio.sleep(0.4) ... monkeypatch.setattr(..., '_BOUND_SWEEP_M
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
No tests for failure paths: builder exception, built is None, CAS failure | The test file covers the happy path and the no-bound path. The diff's own docstrings enumerate failure modes (codec build raising, builder returning None, crash between publish and finalize) but none are exercised. Per-row isolation, dedupe collapse of re-published terminals, and CAS-failure retry are untested claims. | Evidence: except Exception as exc: # noqa: BLE001 — per-row isolation ... if built is None: continue | Fix: Add t
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
EnumRuntimeRestartPolicy is dead configuration | The field is required in ModelCompletionBound and validated, but no code in the diff reads it; _terminalise_abandoned_rows unconditionally terminalises regardless of the declared policy. The docstring argues a single-member enum is honest, but requiring a value that selects nothing is a contract that lies about its own expressiveness. A future 'resume' member would pass validation and change no behavior. | Evidence: on_runtime_restart: EnumRuntimeRestartPolic
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…ibase_infra#3479 (#9331) * evidence(OMN-18296): author OCC companion for OmniNode-ai/omnibase_infra#3479 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 279b91b22e977b1d9ee88715e6a35aafebfc1999. * evidence(OMN-18296): self-bind OCC#9331 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
The CI Summary umbrella concluded fail-closed at 9s on the first run, while the OCC preflight for this head was still cancelled. Its job cannot be re-run on its own, so a fresh head is the only way to re-evaluate it. No source change.
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 2
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[CRITICAL] hostile-reviewer (glm-review)
Bound declared without event_bus disables all give-up, including legacy | _ensure_stale_rows_recovered routes to _terminalise_abandoned_rows when completion_bound is not None, but that function returns 0 immediately if event_bus is None or the codec lacks build_abandoned_terminal. In both cases the legacy adapter.recover_stale_rows() is also skipped. A contract declaring completion_bound on a wiring configuration without an event_bus (or with a codec lacking the builder) loses the pre-existing row-only FAIL
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Terminal event published for rows that may have concurrently recovered | select_abandoned_rows snapshots rows, then the sweep publishes a terminal before the CAS finalize. If a live leg advances the row between SELECT and publish (pushing updated_at forward and bumping version), the terminal FAILURE is already on the bus and consumed downstream; the finalize fails on version mismatch and is logged as a retry, but the false terminal is unretractable. The gateway will terminalise a workflow that recovered. Th
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Two unsynchronised callers of _terminalise_abandoned_rows can double-publish | The background sweeper task and the dispatch-gated _ensure_stale_rows_recovered both call _terminalise_abandoned_rows. The _recovery_lock guards only the latter. The claim that the uuid5 envelope id 'collapses the duplicate at the consume-path dedupe' treats consumer-side dedupe as a correctness mechanism; between publish and dedupe, downstream consumers receive two terminal events for one workflow, and any consumer not deduping
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Sweeper task never cancelled and silently resurrected | _bound_sweeper_task is created on first dispatch and never cancelled on shutdown; asyncio will also emit 'Task was destroyed but it is pending' noise at loop teardown. Additionally, if the task completes (it should not, given the catch-all, but CancelledError or a BaseException subclass can end it), _ensure_bound_sweeper_started transparently creates a replacement with no log. A silently replaced task hides the reason the original died. | Evidence: if
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
_read_completion_bound treats FileNotFoundError as 'no bound declared' | The docstring states a malformed bound raises because 'a bound the runtime cannot read is worse than no bound at all', then FileNotFoundError returns None, silently downgrading to legacy behaviour. If the contract file is missing or unreadable at the read path (deployment race, wrong mount), enforcement is silently disabled, exactly the failure mode the rationale argues against. yaml.YAMLError and OSError/PermissionError are also uncau
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
select_abandoned_rows has no LIMIT or batching | After an extended outage or partition, the first sweep can select every abandoned row in the table and synchronously publish one terminal per row in a single pass inside the sweeper task. This is a thundering-herd publish burst against Kafka and a long-held pool connection, and per-row processing inside one SELECT result set gives no backpressure. The older recover_stale_rows semantics are not shown to bound this either, but this new path publishes per row, w
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Real SQL predicate untested; fakes re-implement it independently | The tests assert the production wiring against a fake adapter whose select_abandoned_rows duplicates the predicate in Python. The actual SQL (NOT IN on state, in_flight, pending_emissions jsonb_array_length, make_interval against NOW()) has no test, so drift between the fake and the adapter is undetectable. The test docstring's claim of asserting 'the production predicate' is only true of the fake's transcription of it. | Evidence: async def
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Cadence ternary dead branch and first-sweep delay | In _bound_sweeper_loop the 'else _BOUND_SWEEP_MIN_INTERVAL_SECONDS' branch is unreachable because the loop is only started when completion_bound is not None; the conditional is dead code. Separately, the loop sleeps before the first sweep, so a row already past its bound at process start waits a full interval (bound/4) before terminalisation even though the first dispatch would have caught it earlier; the background task and the dispatch-gated sweep theref
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MINOR] hostile-reviewer (glm-review)
on_runtime_restart field is validated but never used | ModelCompletionBound requires on_runtime_restart and EnumRuntimeRestartPolicy has one member, but no code path branches on it. The enum docstring argues this is deliberate YAGNI, yet the field is mandatory in the contract, so every contract author must declare a value with no behavioural effect. A typo'd future member is also rejected at wiring with no migration story. | Evidence: on_runtime_restart: EnumRuntimeRestartPolicy = Field(..., description="Th
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Untested paths: builder exception, publish failure, task death, missing-file bound | The suite covers the happy terminalisation, the no-bound case, and the wiring validation. Untested: codec.build_abandoned_terminal raising (per-row isolation), _publish_outbox_batch failure leaving the row for retry, a sweeper loop exception not killing the task, _read_completion_bound on a deleted file (returns None), and shutdown cancellation of the sweeper mid-publish. The per-row isolation and retry claims in _terminali
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…ibase_infra#3479 (#9331) * evidence(OMN-18296): author OCC companion for OmniNode-ai/omnibase_infra#3479 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 279b91b22e977b1d9ee88715e6a35aafebfc1999. * evidence(OMN-18296): self-bind OCC#9331 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
…e restart, on a contract-declared bound (#3479) * fix(OMN-18296): terminalise a state_io workflow abandoned by a runtime restart, on a contract-declared bound * chore(OMN-18296): re-trigger CI after the OCC companion merged The CI Summary umbrella concluded fail-closed at 9s on the first run, while the OCC preflight for this head was still cancelled. Its job cannot be re-run on its own, so a fresh head is the only way to re-evaluate it. No source change.
Closes OMN-18296 (Urgent, child of OMN-18168).
The defect, measured on the lab
Cloud delegation
16eafedc-199c-44c2-a2cb-b9e535839bb9was submitted to the lab lane at 2026-09-13T09:55:04Z. At 09:57:40Z theomninode-runtime-effectspod was recreated by a concurrent sanctioned re-apply, with that delegation's inference command in flight. It never reached a terminal state, and as of this PR it still has not.Read-only probe of the lab (
onex-devnamespace on the 192.168.86.201 k3s), which establishes where the work was lost and why nothing recovered it:gateway_workflowsrowstatus='published',completed_at NULL,correlation_id=a2fe0848-4b4b-462e-b633-c5f9559afee5delegation_workflow_staterowstate='ROUTED',in_flight=t,version=4,pending_emissionsempty,updated_at 09:55:04.27Zdelegation-inference-request.v1inference-response.v1CURRENT-OFFSET 12,LOG-END-OFFSET 12,TOTAL-LAG 0So the inference call itself was lost with the process, and its consumer offset was already committed, meaning the command was never redelivered. No response event of either kind was ever published, and the FSM has nothing to wait for.
Where it was lost, and why nothing closed it out — two defects, both in this repo:
src/omnibase_infra/runtime/state_io/state_store_adapter.py:383(recover_stale_rows) gives up by writingstate='FAILED'to the row and publishing nothing. Its own docstring has named that as a limitation since OMN-14208. Downstream, a gateway workflow can only leavepublishedwhen a real terminal event is consumed off the bus, so a row-only give-up leaves the customer's delegation non-terminal forever while this repo's state of record says it failed. Two different answers to the same question, one of them invisible.src/omnibase_infra/runtime/auto_wiring/handler_wiring.py:5427(_ensure_stale_rows_recovered) is reachable only from inside a dispatch. A lane whose workflow was abandoned by a restart has, by definition, no traffic left to trigger it. On the lab a delegation completed at 10:01:15Z, six minutes into the abandoned row's life and correctly inside its TTL; nothing has dispatched since, so the sweep that would have closed the row has never run again. The 900s TTL was exceeded by more than twenty minutes with the code that enforces it present, correct, and never called.A third problem sits behind both: the 900s TTL was an environment-variable default here and the client's patience was a hardcoded 300s in omnimarket. Two unrelated numbers, neither of them a contract, so the client gave up before the platform's own recovery would even have fired.
What changed
A typed
completion_boundcontract block.ModelCompletionBound(runtime/state_io/model_completion_bound.py) andEnumRuntimeRestartPolicy(enums/enum_runtime_restart_policy.py), read by_read_completion_boundthe same way_read_state_ioreads its block. A contract that declares no bound keeps the pre-existing behaviour exactly, so this is additive; a contract that declares a malformed one raises at wiring time, because a bound the runtime cannot read is worse than none — a reader would believe one was being enforced.The policy enum has exactly one member. A
resumemember would read as a supported choice while nothing resumes a lost leg: the call is gone with the process, its offset is committed, and there is no persisted step to resume from. Re-publishing a persisted outbox batch, the one thing that is recoverable, already happens unconditionally on every sweep and is not a policy choice.The give-up now travels.
StateStoreAdapter.select_abandoned_rowssurfaces the rows on the same predicaterecover_stale_rowsfails on; the wiring layer, which owns the bus, asks the contract's own codec to build the terminal payload, publishes it through the existing_publish_outbox_batch(deterministic row-derived envelope id, contract-declared topic frompublished_events) and then CAS-finalizes the rowFAILED. Publish precedes finalize for the same reason it does in_recover_outbox_batches: a finalized row whose terminal was never published is indistinguishable from a completed workflow and is unrecoverable, because the predicate that finds it no longer matches.One owner of give-up per contract. Where a bound is declared, the legacy blind-FAIL does not run alongside the new path. This is not tidiness — the first GREEN run of the new test caught the legacy sweep reaching the abandoned row first, flipping it to
FAILED, and leaving the bound sweeper with nothing to emit, which restores the exact silent give-up this change removes.A sweeper that does not need traffic. Started once on the first dispatch (wiring is synchronous, so there is no loop to attach to there) and then running on its own timer for the life of the process. Cadence is derived from the contract's own bound rather than fixed, with a floor so a short bound cannot become a busy loop.
dod_evidence
RED first, non-vacuous.
tests/integration/test_state_io_completion_bound_omn18296.pywas run against the pre-change behaviour with every symbol present — the two call sites reverted, nothing removed — so both failures areAssertionErroron the real defect and not an import error on absence:GREEN. The same four tests pass after the change, and the state_io suite is clean:
The fourth test is the additive guard: a contract with no bound publishes no terminal and still runs the pre-existing row-only sweep, with the sweep count asserted as a positive control so the "no terminal" assertion is about the terminal and not about a harness that never swept.
The fixture rows carry the real correlation id and the real 900s bound; only the sweeper's cadence is collapsed, so the predicate under test is the production one.
ruff format,ruff checkandmypy --strictclean on every file touched. The enum was split intoenums/because the architecture validator refuses a module holding both a model and an enum; that refusal is correct and the gate caught it before the commit.Honest gap. The lab proof asked for by AC1 — delete the runtime pod mid-flight and read back a terminal within the bound — cannot be obtained from this PR alone. It needs both halves live on the lab: this repo's enforcement and omnimarket's contract block plus codec builder. The read-only diagnosis above is the strongest evidence available until both merges have been delivered to the lane.
16eafedcis stillpublishedas of this writing and will not self-heal: closing it out requires the deployed runtime to carry this change.Evidence-Ticket: OMN-18296
Evidence-Source: OCC#9331