Repository navigation
fix(OMN-14498): close non-outbox result-applier publish seam at default flag state - #2672
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Limit details: You’ve used all 1 included review currently available under your plan. You completed 114 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe runtime now routes non-outbox result-applier publish failures to the DLQ without relying on a flag. It raises when durable DLQ persistence fails, preserves the consume offset, and keeps outbox propagation unchanged. Two migrations update tenant IDs and RLS behavior. CI checkout credentials are persisted. ChangesPublish Failure DLQ Handling
Database Migration Updates
CI Checkout Credentials
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ResultApplier as result_applier.apply()
participant EventCallback as event callback
participant DLQ as DLQ publisher
participant ConsumeOffset as consume offset
ResultApplier->>EventCallback: publish exception
EventCallback->>DLQ: persist original message and failure metadata
DLQ-->>EventCallback: persistence result
alt DLQ persistence succeeds
EventCallback->>ConsumeOffset: acknowledge consumed message
else DLQ persistence fails
EventCallback-->>ResultApplier: BoundaryApplyPublishError
EventCallback->>ConsumeOffset: leave offset unacknowledged
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
❓ Hostile Reviewer — UNKNOWNBlocking findings (critical): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/omnibase_infra/runtime/auto_wiring/handler_wiring.py`:
- Around line 4560-4561: Propagate the resolved correlation_id from callback()
through _dispatch_with_bounded_retry into _route_apply_publish_failure,
replacing the envelope.correlation_id or uuid4() fallback when constructing the
DLQ record. Add a regression test covering a header-only correlation ID and
assert that the DLQ call receives that resolved value.
- Around line 4331-4385: Refactor _route_apply_publish_failure to remove its
event_bus parameter and all direct access to _publish_raw_to_dlq. Inject and use
a container-provided DLQ protocol service or typed effect executor for
persistence, while preserving the existing unconditional routing and
BoundaryApplyPublishError behavior when persistence is unavailable or fails.
Update callers such as _make_event_bus_callback.callback to pass the new
dependency without exposing the event-bus object.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fd27bef-e58e-47c6-b205-b7c2c89b976a
📒 Files selected for processing (3)
.github/workflows/ci.ymlsrc/omnibase_infra/runtime/auto_wiring/handler_wiring.pytests/unit/runtime/auto_wiring/test_result_applier_publish_failure_seam_omn14498.py
| message=message, | ||
| correlation_id=envelope.correlation_id or uuid4(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate the resolved ingress correlation ID to the DLQ.
Line 4561 uses envelope.correlation_id or uuid4(). When an input has a correlation ID only in transport headers, callback() already resolves that ID at Line 4793, but this route creates a new ID. The DLQ record then loses ingress lineage.
Pass the callback’s resolved correlation_id into _dispatch_with_bounded_retry and then into _route_apply_publish_failure. Add a regression test with a header-only correlation ID and assert that the DLQ call receives it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/omnibase_infra/runtime/auto_wiring/handler_wiring.py` around lines 4560 -
4561, Propagate the resolved correlation_id from callback() through
_dispatch_with_bounded_retry into _route_apply_publish_failure, replacing the
envelope.correlation_id or uuid4() fallback when constructing the DLQ record.
Add a regression test covering a header-only correlation ID and assert that the
DLQ call receives that resolved value.
Source: Coding guidelines
|
Temporarily closed to relieve CI runner-fleet congestion (merge-sweep 2026-08-08, operator-approved — see ROLLING_WORK_LEDGER [mergesweep-0808-close]). Nothing is lost: branch, commits, and review state remain; this PR will be reopened in batches once the queue drains (reopen order recorded in the ledger). |
Pull request was closed
|
Reopened and rebased onto |
2712937 to
be65d9c
Compare
…lt flag state Adversarial verify (Linear comment 3c6da9a0-7d29-4bbc-9342-585aea0e8b5d) proved the prior proof-rerun PASS was vacuous: the two existing OMN-14498 seam tests either drive the unrelated sync-publisher leg or force ONEX_BOUNDARY_DLQ_ENABLED=1, a configuration the runtime does not ship with. At the real production default (flag unset), a forced result_applier.apply() publish failure on a non-outbox (propagate_publish_failures=False) contract still returned normally from _make_event_bus_callback -- an ACK -- with _publish_raw_to_dlq never awaited: a silent drop. Fix: _dispatch_with_bounded_retry's apply() except-clause now routes a non-outbox publish failure through a new _route_apply_publish_failure helper that attempts the DLQ write UNCONDITIONALLY (mirroring the unconditional idiom #2436 already uses for the sync-publisher leg, instead of staging it behind ONEX_BOUNDARY_DLQ_ENABLED) -- the dispatch already succeeded here, only the downstream publish of its known-good result failed, so this is not the doubtful/unvalidated payload the staged rollout exists to hold back. If the DLQ write persists, the callback returns normally (safe ACK). If it cannot persist (no DLQ-capable bus, a False non-persistence return, or the DLQ publish itself raising), a new BoundaryApplyPublishError propagates out of the callback unconditionally -- never re-absorbed by the flag-gated _route_swallowed_exception -- so the offset is withheld instead. Untouched: a raw handler/dispatch exception that never reaches apply() still keeps the historical staged-rollout swallow at the default flag state (test_boundary_dlq_omn14507.py::TestBoundaryDlqFlagOff, rerun green, unmodified). The outbox/state_io path (propagate_publish_failures=True) is unaffected -- still propagates the original cause directly. RED-before/GREEN-after: the new test file failed on collection (ImportError: BoundaryApplyPublishError) against unfixed dev HEAD, proving the fix was previously absent; all 5 new tests plus the full 26-test OMN-14498/14507 family plus the 467-test auto_wiring directory plus the 5445-test unit/runtime directory pass post-fix. OMN-14498
[mergesweep-0809-infraunblock] OCC#6266 merged 2026-08-09T18:10:57Z (supersedes the closed regenerate-class OCC#6181). This gate polls a frozen head at trigger time; retriggering to pick up the new companion state.
`test_non_outbox_boundary_still_swallows_unchanged` asserted that a non-outbox contract's `result_applier.apply()` publish failure was STILL log-and-discarded (ACK) at the default `ONEX_BOUNDARY_DLQ_ENABLED` state. That was never a claim that swallowing is correct: the test's own docstring scoped it as "P3b must not broaden the un-swallow to every auto-wired contract (the platform-wide hole is OMN-14498/OMN-14507's, not this ticket's)" -- a deliberate scope fence naming THIS ticket as the owner of closing the hole. This is that ticket, and the hole is closed, so the fence is retired rather than kept green. Replaced with two tests that pin the new contract and preserve the fence's surviving invariant: - `test_non_outbox_boundary_nacks_when_no_durable_dlq_route`: with no DLQ-capable bus, the boundary must raise `BoundaryApplyPublishError` (offset withheld) instead of returning normally. Verified RED against the pre-fix implementation, which returns None -- the silent drop itself. - `test_non_outbox_boundary_acks_when_dlq_durably_captures`: when the unconditional DLQ route durably takes the record, a non-outbox contract still ACKs. This is the fence's real invariant -- "do not broaden the un-swallow into blanket propagation" -- restated for the closed seam. The discriminator moved from "outbox vs non-outbox" to "was the record durably captured"; nothing is silently dropped in either arm. Also verified RED pre-fix (zero DLQ writes attempted). No production code changed by this commit.
8b2f66c to
8d8cf11
Compare
1 participant
Summary
Closes the residual seam adversarial verify proved still open on OMN-14498
after PRs #2436 (sync-publisher DLQ route) and #2604 (DLQ correlation
lineage / unpersisted-DLQ NACK): at the production default
(
ONEX_BOUNDARY_DLQ_ENABLEDunset), a forcedresult_applier.apply()publish failure on a non-outbox (
propagate_publish_failures=False,i.e. non-
state_io) contract dispatched through the real_make_event_bus_callbackstill returned normally (ACK) with_publish_raw_to_dlqnever awaited — a silent drop, exactly what theticket's acceptance criterion forbids.
Full refutation, including the live counter-probe against the real
production callback at default flag state, is in Linear comment
3c6da9a0-7d29-4bbc-9342-585aea0e8b5don OMN-14498.
Root cause
_dispatch_with_bounded_retry'sresult_applier.apply()except-clause hadtwo arms:
propagate_publish_failures=True(state_io/outbox contracts): wraps inBoundaryPublishError, propagates — correctly satisfies "offset notcommitted."
propagate_publish_failures=False(the general/non-outbox case): bareraise, which rides the generic retry arm and, on exhaustion, falls intothe flag-gated
_route_swallowed_exception. At the default (unset)flag state that function finds no DLQ target, logs one line, and
returns normally — an ACK.
handler_wiring.py:7270wirespropagate_publish_failures=_contract_declares_state_io(contract)— every non-
state_iocontract (the overwhelming majority) takes thesilent-swallow arm at default config.
Fix
Mirrors the same unconditional DLQ idiom PR #2436 already uses for the
sync-publisher leg, applied to this seam specifically (not to the whole
flag-gated raw-handler-exception path):
_route_apply_publish_failurehelper: on a non-outboxresult_applier.apply()failure, attempts_publish_raw_to_dlqunconditionally — not staged behind
ONEX_BOUNDARY_DLQ_ENABLED. Thedispatch already succeeded here; only the downstream publish of its
known-good result failed, so this is not the doubtful/unvalidated
payload the staged rollout exists to hold back.
Falsenon-persistence return, or theDLQ publish itself raising → raises new
BoundaryApplyPublishError.BoundaryApplyPublishErrorexception: added to_dispatch_with_bounded_retry's non-retryable tuple (the handler alreadyran once; retrying can't fix a downstream publish problem) and given a
dedicated
exceptclause incallback()so it is never re-absorbedby the flag-gated
_route_swallowed_exception— it propagatesunconditionally out of the boundary, withholding the offset.
_dispatch_with_bounded_retrynow takes the rawmessage(threaded fromcallback()) so the DLQ write preserves the same ingress fidelity_route_swallowed_exceptionalready gets (headers/offset/partition), nota reconstructed stand-in.
Untouched (staged-rollout semantics preserved for every other seam): a
raw handler/dispatch exception that never reaches
apply()still keeps thehistorical default-flag-off swallow — reproved by rerunning
test_boundary_dlq_omn14507.py::TestBoundaryDlqFlagOffunmodified. Theoutbox/
state_iopath (propagate_publish_failures=True) is unaffected —still propagates the original cause directly, unwrapped.
Seam definition (interacting-work deliverable)
BoundaryApplyPublishError(topic: str, correlation_id: object, cause: Exception)— moduleomnibase_infra.runtime.auto_wiring.handler_wiring_route_apply_publish_failure(exc, *, event_bus, topic, message, correlation_id) -> None(raisesBoundaryApplyPublishErroron non-persistence, else returns)_dispatch_with_bounded_retry(envelope, message)— added positionalmessage: object(wasenvelopeonly)failure_type)original_topic,raw_msg=message,error,correlation_id,failure_type="apply_publish_failed",consumer_group="auto-wiring",dlq_topicONEX_BOUNDARY_DLQ_ENABLEDis not read anywhere in the new code pathBoundaryApplyPublishErroradded alongsideBoundaryPublishErrorin_dispatch_with_bounded_retry's except tupleRED-before / GREEN-after proof
New file
tests/unit/runtime/auto_wiring/test_result_applier_publish_failure_seam_omn14498.pydrives the real
_make_event_bus_callback(not a stub), non-outboxdefaults,
ONEX_BOUNDARY_DLQ_ENABLEDexplicitly unset(
monkeypatch.delenv).RED (against pre-fix
devHEADa853500ab3):GREEN (post-fix):
Regression scope run post-fix:
seam files +
test_boundary_dlq_omn14507.pyunmodified) — 26 passedtests/unit/runtime/auto_wiring/— 467 passedtests/unit/runtime/— 5445 passed, 5 skippedGates
uv run ruff format/ruff check --fix— cleanuv run mypy src/omnibase_infra/runtime/auto_wiring/handler_wiring.py --strict— cleanuv run mypy tests/unit/runtime/auto_wiring/test_result_applier_publish_failure_seam_omn14498.py --strict— cleanpre-commit run --files <changed files>— all hooks passedselector — escalated to the full local suite for this shared-module diff,
23173 passed, deploy-scope DoD parity) — all passedOMN-14498
Evidence-Source: OCC#6266
Evidence-Ticket: OMN-14498
Evidence-Head: 53c05db39267693c8821aa46d67e4f831b33416e
Summary by CodeRabbit
Bug Fixes
Tests