Skip to content

fix(OMN-13118): correlator resolves matched terminal in place for late awaiter - #1975

Merged
jonahgabriel merged 1 commit into
devfrom
jonah/omn-13118-correlator-handoff-fix
Jun 14, 2026
Merged

jonahgabriel merged 1 commit into
devfrom
jonah/omn-13118-correlator-handoff-fix

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Jun 14, 2026 •

Copy link
Copy Markdown
Collaborator

OMN-13118 — correlator resolves matched terminal in place for the late awaiter (P_RESOLVE_BEFORE_AWAIT)

Ticket: OMN-13118 (Tier B consume-leg redesign; epic OMN-12525)

The race (P_RESOLVE_BEFORE_AWAIT)

LongLivedTerminalCorrelator runs ONE always-on poll loop over a single
runtime-lifetime consumer. A trial register(cid)s its future BEFORE publishing
its command (subscribe-before-publish at the correlation level), publishes, then
blocks on wait(cid).

Because the poll loop runs continuously and the publish -> generation -> wait
latency is large (~28s observed live), the loop typically consumes and matches
the correlated terminal BEFORE the runner's wait() attaches
. With the old
_deliver that popped the future on match, the resolved future was removed
from the registry the instant it was matched. The late _wait then found
nothing in _pending, created a fresh (unresolved) future, and timed out at the
120s trial cap -> None -> degenerate generation-failure row -> zero rows.

This is the live "strike-seven" wedge: the COMPLETED terminal was published and
its HWM climbed every trial, but the delivered-and-discarded match meant the
runner saw no terminal.

The fix (this PR — clean, no instrumentation)

  • _deliver resolves the future IN PLACE: self._pending.get(cid) +
    future.set_result(body) — no pop on match. A future resolved before the
    awaiter attaches now survives in the registry until the awaiter reads it.
  • _wait owns removal: pops the cid in a finally on BOTH success and
    timeout, so a resolved-and-read future does not linger.
  • Idempotent under duplicate delivery via the existing future.done() guard;
    register() (pre-publish) guarantees the future exists when _deliver matches.

Two-line load-bearing change in service_terminal_event_consumer.py:
pop -> get in _deliver, and move the registry removal into _wait's
finally. No DIAG13118 instrumentation is included in this PR — this is the
clean fix only.

Regression test

New tests/unit/runtime/test_terminal_correlator_resolve_before_await.py drives
the real LongLivedTerminalCorrelator and pins the exact ordering: register(cid)
-> append terminal -> block until the registry future is done() (proving
_deliver resolved it IN PLACE, i.e. delivery provably beat the awaiter) ->
THEN wait(cid) and assert it returns the body. Verified RED->GREEN: with
_deliver reverted to pop-on-match the test FAILS (the resolved future is gone
before the awaiter attaches); with the resolve-in-place fix it PASSES and the
awaiter also pops the cid on exit. A sibling test asserts the in-window
(arrive-during-await) path is not regressed.

Acceptance — LIVE K>=10 gate (PASSED on rebuild-10)

The acceptance gate for this defect class is NOT a unit test; it is the LIVE
K>=10 multi-cell battery
on the stability-test lane. It PASSED on
reprobe-K10-rebuild10 (2026-06-13T23:39Z, omnibase-infra-stability-test
:18085/:18086, projection-api :13002), VERDICT PASS:

  • 40/40 trials' primary wait() logged RESOLVED (rebuild-9 had RESOLVED=0,
    3 no terminal event received warnings; rebuild-10 had 0).
  • Terminal context-roi-run-completed.v1 HWM advanced 22 -> 24.
  • Projection total_rows 448 -> 488 (+40); rows_for_fresh_run_id = 40 via
    projection API (20 bool_flag + 20 address_norm, 40 distinct cids, sample
    final_success=True first_pass_success=True attempt_count=1).
  • Aggregate trace: MATCHED=80, UNMATCHED=0, RESOLVED=40, trial-terminal
    TIMEOUT=0. (Residual TIMEOUT=39 / NO_PRE_REGISTER=40 are the benign
    second-redundant-waiter from the runner opening two sessions per trial sharing
    one cid — reported, not suppressed; candidate cleanup is register/await once
    per trial.)

Evidence bundle: docs/evidence/2026-06-12-weekend-pass/experiments/probe4-stability/reprobe-K10-rebuild10/
(FINDINGS.md, build-provenance.json, projection-check.txt, watermarks-before/after.txt, diag13118-effects.log).

Honest pairing note: the live-proven deploy commit 0d958ea45 carried the
DIAG13118 instrumentation alongside this same logical fix (get-not-pop in
_deliver, _wait pops on exit). This PR ships the identical load-bearing
change WITHOUT the instrumentation.
The runner-side register(cid)-before-publish
half of the pairing (omnimarket cc1f658d, OMN-13118 runner-register-order) is a
separate PR.

Local verification

  • uv run ruff format / uv run ruff check — clean on both changed files.
  • uv run mypy src/ --strict — Success, no issues in 2442 source files.
  • uv run pytest tests/unit/runtime/auto_wiring/test_event_consumer_injection.py tests/integration/test_terminal_consumer_longlived_correlator.py tests/unit/runtime/test_terminal_correlator_resolve_before_await.py -q — 7 passed.
  • pre-commit run --files ... — all hooks pass.

Evidence-Source (OCC pairing)

The OCC evidence PR for the reprobe-K10-rebuild10 acceptance bundle is OCC#2616
(onex_change_control, base dev). It carries the OMN-13118 contract + the four
receipts (dod-omnimarket-runner-pr, dod-omnibase-infra-correlator-pr,
dod-live-gate, dod-deploy-assessment); the decisive evidence is the live
K>=10 reprobe-K10-rebuild10 PASS (dod-live-gate). Receipt-Gate + deploy-gate
resolve against OCC#2616.

Evidence-Source: OCC#2616
Evidence-Ticket: OMN-13118

@coderabbitai

coderabbitai Bot commented Jun 14, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

LongLivedTerminalCorrelator._deliver is changed from pop to get so it resolves the registered future in place without removing it from _pending. _wait is updated to move registry cleanup into a finally block. A new regression test file adds in-memory Kafka fakes and two tests covering both delivery orderings of the race.

Changes

LongLivedTerminalCorrelator resolve-before-await race fix and regression tests

Layer / File(s) Summary
_deliver and _wait registry lifecycle fix
src/omnibase_infra/runtime/service_terminal_event_consumer.py
_deliver uses get instead of pop so the resolved future stays in _pending for late awaiters. _wait moves _pending.pop into a finally block to guarantee cleanup on both success and timeout.
In-memory Kafka fakes and test helpers
tests/unit/runtime/test_terminal_correlator_resolve_before_await.py
Defines _PartitionLog, _FakeClient, _FakeConsumer, _KafkaLikeBus, and helper functions (_terminal_envelope_json, _await_future_resolved, _pending_contains) used by both regression tests.
Regression tests for both delivery orderings
tests/unit/runtime/test_terminal_correlator_resolve_before_await.py
test_wait_returns_body_when_terminal_resolved_before_await confirms wait() returns the correct body from an already-resolved future and that the registry entry is removed. test_wait_still_returns_body_when_terminal_arrives_during_await confirms wait() returns the correct body when the terminal is published on a background thread after wait() starts.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 Hippity-hop, the future stays near,
No early pop to make it disappear!
finally cleans up when waiting is done,
Two races covered, both properly won.
The registry's tidy, the test passes true —
A correlating rabbit says: "Carrots to you!" 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically identifies the main fix: the correlator now resolves terminal matches in place rather than removing them prematurely, allowing late awaiters to retrieve them.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jonah/omn-13118-correlator-handoff-fix

Comment @coderabbitai help to get the list of available commands and usage tips.

@jonahgabriel
jonahgabriel added this pull request to the merge queue Jun 14, 2026
Merged via the queue into dev with commit d703ca7 Jun 14, 2026
172 of 178 checks passed
@jonahgabriel
jonahgabriel deleted the jonah/omn-13118-correlator-handoff-fix branch June 14, 2026 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant