fix(viewer): roll back a payload that fails partway through ingest 🤖🤖🤖 - #274
Open
sushant-mishra-dtu wants to merge 1 commit into
Open
Conversation
_ingest_one() writes as it goes and leaves the commit to its caller
(otlp_store.py:571-755). Nothing rolls back when it raises, so the rows it
already wrote stay in the connection's open transaction and are committed by
the next commit on that connection. There is no rollback() anywhere in the
module.
Both batch callers catch the exception and log "Failed to process one payload,
skipping" (:837, :872), and ingest_batch_write()'s docstring promises that "one
malformed payload cannot prevent the rest of the batch from being committed".
The payload is not skipped: its partial write is committed with the rest.
_ingest_one() updates the sessions row before it inserts the spans, so the
common shape is a session whose span_count counts spans that were never
written. Reproduced against the public API -- one good payload, then one
malformed payload for the same session:
sessions.span_count = 2
rows in spans = 1
list_sessions() reports span_count = 2
get_session_spans() returns = 1 spans
The viewer's own two endpoints disagree, and the row survives a fresh
connection.
ingest() is worse, because it has no catch at all: it raises, leaves the
partial write in the open transaction, and the next unrelated ingest() commits
it. A rejected request for one session is persisted on the back of a later,
successful request for a different one:
request 1 (malformed, sess-A): raised, in_transaction=True
request 2 (valid, sess-B): succeeded
-> sess-A: span_count=2, actual spans=1 (committed by request 2)
Wrap _ingest_one() in a SAVEPOINT and roll back to it on failure. A savepoint
rather than rollback() because the batch path must keep the payloads that
already succeeded -- rolling back the whole transaction would break the
batching behaviour the docstring documents.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: sushant-mishra-dtu <sushant.arh@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe change isolates each OTLP payload with a SQLite savepoint. Direct and batch ingestion paths use the atomic wrapper. Regression coverage verifies that failed payloads do not retain partial session updates. ChangesOTLP atomic ingestion
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to OTLP payload ingestion now discards partial writes from failed payloads while preserving successful payloads in the surrounding batch, preventing inconsistent session and span counts. The change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_ingest_one() writes as it goes and leaves the commit to its caller (otlp_store.py:571-755). Nothing rolls back when it raises, so the rows it already wrote stay in the connection's open transaction and are committed by the next commit on that connection. There is no rollback() anywhere in the module.
Both batch callers catch the exception and log "Failed to process one payload, skipping" (:837, :872), and ingest_batch_write()'s docstring promises that "one malformed payload cannot prevent the rest of the batch from being committed". The payload is not skipped: its partial write is committed with the rest.
_ingest_one() updates the sessions row before it inserts the spans, so the common shape is a session whose span_count counts spans that were never written. Reproduced against the public API -- one good payload, then one malformed payload for the same session:
The viewer's own two endpoints disagree, and the row survives a fresh connection.
ingest() is worse, because it has no catch at all: it raises, leaves the partial write in the open transaction, and the next unrelated ingest() commits it. A rejected request for one session is persisted on the back of a later, successful request for a different one:
Wrap _ingest_one() in a SAVEPOINT and roll back to it on failure. A savepoint rather than rollback() because the batch path must keep the payloads that already succeeded -- rolling back the whole transaction would break the batching behaviour the docstring documents.
What does this PR do?
Related issues
Checklist
uv run ruff check .anduv run ruff format --check .pass)uv run pytest)_ingest_one_atomic()is a private helper, the three public entry points keep their signatures and their documented behaviour (the batch docstring's promise is what this PR makes true), anddocs/does not documentotlp_store's API —docs/concepts/tracing.mddescribes the viewer only as a process to start.git diff --name-statusreportsM,M).scripts/hooks/check_spdx.pyexits 0 on this branch andscripts/check_license_headers.pyreports "all 929 source Python files carry an SPDX header".Re-verified on
ec2eb12(Windows 11 / CPython 3.13.6):uv run ruff check .→All checks passed!,uv run ruff format --check .→926 files already formatted, andtests/viewer→ 154 passed, 2 deselected against 153 passed, 2 deselected onmain. The added test is non-vacuous by fault injection: pointingest()back at_ingest_one()and it fails withassert 2 == 1— exactly thespan_count2 / 1 stored span mismatch described above. A fulluv run pytestdoes not complete on this machine (src/nooa/storage/sqlite.py:10importsfcntl), so the run is scoped totests/viewer.Separate, single-claim fix from #270/#271 and independent of the connection-lifecycle fix on
fix/viewer-close-trace-db, opened since as #273 — please review on its own.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests