Repository navigation
fix(edi): store an interchange completely or not at all - #3833
PetrefiedThunder wants to merge 2 commits into
Conversation
If one line of an interchange failed during ingest, the others stayed stored and the claim stayed held, so a resend got a 409 for seven days and the failed lines were never stored. Releasing the claim is not safe: event keys carry line numbers, so a corrected resend would store surviving lines twice. Add all_or_nothing to ingest_events and use it on both EDI routes. Any rejected event undoes the batch in both stores, nothing reaches graph sync, and the claim is released because nothing was stored. The ingestion run stays as the record of the attempt. Other callers keep per-event acceptance. Found by a Codex review of #3818. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review: 1
Please check the settings in the CodeRabbit UI or the
 |
PR Blocker ChecklistHead SHA: Status: Ready
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0075a3266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The deferred graph publish ran after the commit, so a Redis error escaped ingest_events and the route released the claim on stored events. A resend could then store them again. Log the failure and return the stored result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
If one line of an EDI interchange failed during ingest, the other lines stayed stored and the ISA13 claim stayed held. A resend got a 409 for seven days, so the failed lines were never stored. Codex flagged this as P1 while reviewing #3818.
#3811 explains why releasing the claim is unsafe: event keys carry line numbers, so a corrected resend would store surviving lines twice.
Fix
An interchange now stores completely or not at all, so the key problem never arises.
ingest_eventsgainsall_or_nothing, default off. Both EDI routes turn it on.Not stored: another event in this batch was rejected.acceptedis 0, so the claim is released and the corrected document can reuse its ISA13.Other
ingest_eventscallers keep per-event acceptance.Merge order
Based on #3811 (
claude/edi-isa13-false-409). Retarget tomainonce it lands.ingest_eventscalls, because fix(edi): store valid 856s through the required-KDE gate #3818 addssource_file=there, and in the test stubs. Keep both arguments.partialstatus can no longer happen on EDI once this lands. An interchange that fails during ingest reportsrejected.Verification
test_edi_atomic_interchange.pyruns the realingest_eventsand gate, against a journal that savepoints actually undo.accepted: 0, rejected: 2, with nothing infsma.cte_eventsorfsma.traceability_events, and no graph publish.ingest_eventsdirectly. The route-level path needs fix(edi): store valid 856s through the required-KDE gate #3818 to store an 856, so it hasn't been driven end to end on this branch.🤖 Generated with Claude Code
Note
Medium Risk
Changes core EDI ingest persistence and ISA13 dedup semantics for multi-line interchanges, but behavior is scoped to EDI routes with savepoint rollback and dedicated test coverage.
Overview
EDI interchanges are now atomic at persistence: if any event in the batch fails validation or storage, the whole interchange rolls back in CTE and canonical stores, the API reports every line as rejected (including a "Not stored: another event in this batch was rejected" message for lines that looked accepted mid-flight), and ISA13 is released so partners can resend the corrected document under the same control number.
Both
POST /api/v1/ingest/ediand/documentpassall_or_nothing=Trueintoingest_events. That helper gains an opt-inall_or_nothingflag (default off for other callers): a nested savepoint wraps the event loop, graph sync is deferred until after DB commit, and a post-commit graph failure is logged without failing the request.Docs in
TRACEABILITY_FILE_INTAKE.mdnow describe this all-or-nothing interchange rule instead of partly stored interchanges holding a duplicate 409. New tests intest_edi_atomic_interchange.pyexercise rollback, successful commit + sync, default per-event behavior, and graph outage tolerance; existing EDI test stubs accept the new keyword.Reviewed by Cursor Bugbot for commit 738f368. Bugbot is set up for automated code reviews on this repo. Configure here.