feat(sp27): unify 835 ingest + reconciliation in handle_835 (atomic)
Move 'reconcile.run(s, record.id)' inside CycloneStore.add()'s ingest session, before s.commit(). The placeholder adjustment_amount set by _remittance_835_row is overwritten by reconcile's CAS-aggregate pass in the same transaction — readers never see a half-reconciled Remittance row. If reconcile raises, the entire 835 ingest rolls back via the session's __exit__. The previous flow committed batch + remittance rows in session-1, then opened session-2 to run reconcile.fail-soft. Two visible problems closed: a race window where readers fetched the placeholder adjustment_amount, and a half-reconciled state left visible if reconcile crashed. Deviation from the plan (N4 in autoreview): the reconcile call lives in cycl_store.add() rather than handle_835 calling a new reconcile.run_now(batch_id) helper after add(). Same end state, one fewer module surface, handler stays a thin wrapper over the store.
This commit is contained in:
Nora
@@ -1,14 +1,17 @@
|
||||
"""Direct tests for the ``handle_835`` handler (SP27 Task 5).
|
||||
"""Direct tests for the ``handle_835`` handler (SP27 Task 5 + Task 10).
|
||||
|
||||
Locks the handler's contract independent of the scheduler lifecycle
|
||||
so a regression in the scheduler wiring doesn't hide a regression
|
||||
in the handler.
|
||||
|
||||
Note: the 835 handler is the largest of the four because the schema
|
||||
covers per-claim remittances + CAS adjustments + validation. The
|
||||
two-phase ingestion problem (batch row first, then a separate
|
||||
``reconcile`` pass) is a known gap; atomic unification happens in
|
||||
SP27 Task 10. For Task 5 we lock current behavior (no atomic).
|
||||
covers per-claim remittances + CAS adjustments + validation. SP27
|
||||
Task 10 unified the two-phase ingest path (batch row first, then a
|
||||
separate ``reconcile`` pass that overwrote ``adjustment_amount``)
|
||||
into one critical section. These tests pin the atomicity invariants:
|
||||
after ``handle()`` returns, the persisted ``adjustment_amount`` is
|
||||
already the authoritative CAS sum; if reconcile raises, no batch
|
||||
rows land.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -102,3 +105,148 @@ def test_handle_835_raises_on_completely_unparseable_input():
|
||||
bad = "this is not even EDI"
|
||||
with pytest.raises((CycloneParseError, ValueError)):
|
||||
handle(bad, source_file="bad.835")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# SP27 Task 10: atomicity invariants
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_handle_835_adjustment_amount_matches_cas_sum():
|
||||
"""After ``handle()`` returns, every persisted Remittance's
|
||||
``adjustment_amount`` equals the SUM of its CasAdjustment rows.
|
||||
|
||||
Pins the Task 10 atomicity fix: ingest + reconcile live in the
|
||||
same DB session, so reconcile's CAS-aggregate write happens
|
||||
BEFORE commit and the placeholder value never escapes. Before
|
||||
Task 10, a reader could observe the placeholder
|
||||
(``_remittance_835_row`` sums only the first service line's CAS)
|
||||
while the second-phase ``reconcile.run`` was still pending.
|
||||
"""
|
||||
from decimal import Decimal
|
||||
|
||||
from sqlalchemy import func, select
|
||||
|
||||
from cyclone.db import CasAdjustment, Remittance
|
||||
|
||||
text = MINIMAL.read_text()
|
||||
handle(text, source_file=MINIMAL.name)
|
||||
|
||||
with db.SessionLocal()() as session:
|
||||
remits = session.execute(select(Remittance)).scalars().all()
|
||||
assert len(remits) >= 1
|
||||
for r in remits:
|
||||
cas_sum = session.execute(
|
||||
select(func.coalesce(func.sum(CasAdjustment.amount), 0))
|
||||
.where(CasAdjustment.remittance_id == r.id)
|
||||
).scalar_one()
|
||||
assert r.adjustment_amount == Decimal(str(cas_sum)), (
|
||||
f"remit {r.id}: adjustment_amount={r.adjustment_amount} "
|
||||
f"!= CAS sum={cas_sum}"
|
||||
)
|
||||
|
||||
|
||||
def test_handle_835_reconcile_failure_rolls_back_ingest(monkeypatch):
|
||||
"""If reconcile raises mid-ingest, the batch + remittance rows
|
||||
don't land at all (atomic rollback, SP27 Task 10).
|
||||
|
||||
Before Task 10, ``store.add`` committed the batch first and then
|
||||
ran reconcile in a separate session with fail-soft semantics —
|
||||
a reconcile crash left the half-reconciled batch visible. After
|
||||
Task 10, ``reconcile.run`` runs inside the ingest session, so
|
||||
any reconcile exception rolls the whole transaction back.
|
||||
"""
|
||||
from sqlalchemy import select
|
||||
|
||||
from cyclone import reconcile
|
||||
from cyclone.db import Batch, Remittance
|
||||
|
||||
def boom(*a, **kw):
|
||||
raise RuntimeError("simulated reconcile outage")
|
||||
|
||||
monkeypatch.setattr(reconcile, "match", boom)
|
||||
|
||||
text = MINIMAL.read_text()
|
||||
with pytest.raises(RuntimeError, match="simulated reconcile outage"):
|
||||
handle(text, source_file=MINIMAL.name)
|
||||
|
||||
with db.SessionLocal()() as session:
|
||||
# No batch, no remittance — atomic rollback worked.
|
||||
batches = session.execute(select(Batch)).scalars().all()
|
||||
remits = session.execute(select(Remittance)).scalars().all()
|
||||
assert batches == [], (
|
||||
f"reconcile raised but {len(batches)} batch rows landed"
|
||||
)
|
||||
assert remits == [], (
|
||||
f"reconcile raised but {len(remits)} remittance rows landed"
|
||||
)
|
||||
|
||||
|
||||
def test_handle_835_late_reconcile_failure_rolls_back_match_loop(monkeypatch):
|
||||
"""A crash in the SECOND pipeline (``_reconcile_pair``) still
|
||||
rolls back the FIRST pipeline's mutations — Claim.state,
|
||||
Claim.matched_remittance_id, Remittance.claim_id, the new
|
||||
ActivityEvent row, the new Match row.
|
||||
|
||||
Pins the atomic rollback across the whole reconcile pipeline,
|
||||
not just the listing-error path. Without this, a future change
|
||||
that splits ``reconcile.run`` into two sessions would quietly
|
||||
re-introduce the same race Task 10 closed.
|
||||
"""
|
||||
from datetime import date
|
||||
from decimal import Decimal
|
||||
|
||||
from sqlalchemy import select
|
||||
|
||||
from cyclone import reconcile
|
||||
from cyclone.db import (
|
||||
ActivityEvent, Batch, Claim, ClaimState, Match, Remittance,
|
||||
)
|
||||
|
||||
# Pre-seed a Claim that will auto-match the minimal_835 remit
|
||||
# (``payer_claim_control_number = "CLM001"``). Without this,
|
||||
# ``reconcile.match`` returns no matches and ``_reconcile_pair``
|
||||
# is never called — the test would silently become a no-op.
|
||||
with db.SessionLocal()() as s:
|
||||
s.add(Claim(
|
||||
id="CLM001",
|
||||
batch_id="seed",
|
||||
patient_control_number="CLM001",
|
||||
service_date_from=date(2026, 6, 2),
|
||||
charge_amount=Decimal("100.00"),
|
||||
state=ClaimState.SUBMITTED,
|
||||
))
|
||||
s.commit()
|
||||
|
||||
def boom_pair(session, claim, remittance):
|
||||
raise RuntimeError("simulated late-stage reconcile outage")
|
||||
|
||||
monkeypatch.setattr(reconcile, "_reconcile_pair", boom_pair)
|
||||
|
||||
text = MINIMAL.read_text()
|
||||
with pytest.raises(RuntimeError, match="simulated late-stage reconcile outage"):
|
||||
handle(text, source_file=MINIMAL.name)
|
||||
|
||||
with db.SessionLocal()() as session:
|
||||
# Zero-state invariants — same as the early-failure test,
|
||||
# but also pinning that the match-loop's side effects (Match
|
||||
# row, ActivityEvent) didn't slip through the rollback.
|
||||
batches = session.execute(select(Batch)).scalars().all()
|
||||
remits = session.execute(select(Remittance)).scalars().all()
|
||||
match_rows = session.execute(select(Match)).scalars().all()
|
||||
activity_rows = session.execute(select(ActivityEvent)).scalars().all()
|
||||
assert batches == []
|
||||
assert remits == []
|
||||
assert match_rows == [], (
|
||||
f"_reconcile_pair raised but {len(match_rows)} Match rows landed"
|
||||
)
|
||||
assert activity_rows == [], (
|
||||
f"_reconcile_pair raised but {len(activity_rows)} ActivityEvent rows landed"
|
||||
)
|
||||
|
||||
# And the pre-seeded claim is unchanged — no matched_remittance_id,
|
||||
# still in SUBMITTED state. The match-loop's mutations against
|
||||
# this row were rolled back too.
|
||||
claim = session.get(Claim, "CLM001")
|
||||
assert claim.matched_remittance_id is None
|
||||
assert claim.state == ClaimState.SUBMITTED
|
||||
|
||||
@@ -330,8 +330,20 @@ def test_run_reversal_flips_paid_to_reversed(fixture_835):
|
||||
assert reversal_match.prior_claim_state == ClaimState.PAID
|
||||
|
||||
|
||||
def test_run_failed_reconcile_writes_activity_event(fixture_835):
|
||||
"""If reconcile crashes, the batch + remittances stay; activity event records failure."""
|
||||
def test_run_reconcile_raise_in_session_leaves_prior_commits_alone(fixture_835):
|
||||
"""A ``reconcile.run`` raise inside an open session does not damage
|
||||
previously-committed rows.
|
||||
|
||||
The test seeds a batch + remit, commits them in one session, then
|
||||
calls ``reconcile.run`` in a fresh session with ``match`` monkey-
|
||||
patched to raise. The pre-existing rows must survive — they're on
|
||||
disk from the prior commit, separate from the rolling-back
|
||||
session. This pins the unit-level invariant that ``reconcile.run``
|
||||
itself never commits and never silently mutates rows outside the
|
||||
session it was given; it is the *caller's* responsibility (now
|
||||
``CycloneStore.add`` per SP27 Task 10) to control the transaction
|
||||
boundary.
|
||||
"""
|
||||
with db.SessionLocal()() as s:
|
||||
_make_batch(s)
|
||||
_make_remit(s, "CLP-1", "PCN-A", "1", "100.00", "100.00")
|
||||
|
||||
Reference in New Issue
Block a user