feat(txn): surface transaction abort reasons to clients - #9747
Conversation
6eff47d to
1042bb1
Compare
shiva-istari
left a comment
shiva-istari
left a comment
There was a problem hiding this comment.
Thanks for putting this together @rahst12 surfacing why a transaction aborted is a really useful addition, and I appreciate the care here: keeping codes.Aborted for backward compatibility, and adding tests across all the repos.
I went through it in depth alongside the dgraph4j / pydgraph / docs PRs, and I have a few design-level thoughts I'd like to raise.
A few things I noticed:
No single source of truth for the categories. The reason strings (conflict / stale-startts / predicate-move) are defined in 4 places — server, dgraph4j, pydgraph, docs so they can drift independently over time.
The reason travels inside the message string and is recovered via split(":"). It works, but it's a bit brittle to depend on the message format as a contract.
A few categories may not always match what actually happened e.g. a ctx cancel/timeout currently reports as conflict:, the late (post-lease) abort is always conflict: even when it's really a stale start-ts, and predicate-move: also catches some non-move errors (malformed key / tablet-not-served).
Observability (future scope). The reason is currently invisible in metrics TxnAborts is a bare counter, so we can only see the total, not the breakdown by category. Since the server already computes the reason, it'd be a nice low-effort win down the line to add it as a metric label (e.g. txn_aborts_total{reason="conflict"}) so the split shows up in dashboards. Not needed for this PR more of a follow-up thought.
…ace-phase-1 # Conflicts: # worker/mutation.go
Review feedback: some abort categories did not match what happened. Three distinct defects, all fixed without adding to the published vocabulary (conflict / stale-startts / predicate-move), so no client or docs change is needed and old clients are unaffected. 1. checkPreds labelled all five of its exits "predicate-move", but only two are moves. It now returns the category alongside the error so each exit declares its own cause. A malformed predicate key or a predicate served by no group are different failures with different remedies - retrying a malformed key can never succeed - so they report no category rather than claiming a move. 2. isBlocked is now consulted before the tablet lookup. blockTablet is held for the entire duration of a move (movePredicate defers its unblock past the reassignment proposal), making it the authoritative signal. Checking it first guarantees an in-flight move is always reported as a move, and lets the tablet == nil case mean only "no group serves this predicate". 3. The late (post-lease) abort always reported "conflict". Oracle.commit re-runs hasConflict, which rejects a stale start timestamp as readily as a real write-write conflict and collapses both into x.ErrConflict, so stale-startts was only ever reachable from the early check. Staleness is re-checked on that path. A cancelled or timed-out context is also no longer called a conflict; it reports its own message with no category. The status code stays codes.Aborted, since switching to Canceled/DeadlineExceeded would change retry behaviour for clients that treat Aborted as retryable. Where no published category fits, the detail is emitted with no prefix rather than borrowing a code implying the wrong remedy. dgraph4j and pydgraph already degrade an absent or unrecognized prefix to UNKNOWN, and a test asserts no withheld detail can be misparsed as a category. This also keeps a later phase purely additive: naming a new category then only adds precision to something already UNKNOWN, and never reclassifies a shipped code. Separately, abortDetailStaleStartTs claimed "due to a leader change", but Zero raises startTxnTs from two places - updateLeases on becoming leader, and purgeBelow when trimming its conflict map at a snapshot, which is not a leader change at all. The detail now names both. checkPreds moves from a closure to a method so its category selection is unit testable; behaviour is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the feedback. My goal is to keep this first pass super minimalistic, small in scope, and non-impacting to exisiting users. With these goals, there's some things that don't get addressed, however, I feel like the surfacing of this information is still better than dropping it altogether, recognizing that more work is needed for a comprehensive solution.
Yes this is true and by design. I want to keep the changes very minimal and ensure it's fully backwards compatible.
Agreed, I have some future phase thoughts that would make this better, but would require changes to the proto fields.
Acknowledged and agreed it would be a future change. For this piece of feedback: A few categories may not always match what actually happened e.g. a ctx cancel/timeout currently reports as conflict:, the late (post-lease) abort is always conflict: even when it's really a stale start-ts, and predicate-move: also catches some non-move errors (malformed key / tablet-not-served). I don't think we should assert a category that can't substantiated. There is an UNKNOWN category for things like this. I'm aiming to capture anything unsubstantiated into the UNKNOWN category, then in future PRs introduce greater fidelity. I'm working on disambiguating some things. My thoughts of the categories:
|
The abort messages are a wire contract - dgraph4j, pydgraph and the docs all depend on them - but only two of the seven were constants; the rest were inline errors.Errorf calls spread through checkPreds. Adding a new abort message therefore never forced anyone to look at the existing vocabulary, which is how a category and its detail drift apart. All checkPreds details move next to the two existing constants, with a table in the block comment mapping each detail to the category it pairs with. The *Fmt suffix marks the ones that take arguments. Text is unchanged byte for byte, so existing log-scrapers, client fixtures and docs still match, and TestAbortVocabulary asserts the pre-existing wording verbatim to prove this was a move rather than an edit. That test also pins the whole published surface: the four codes and every detail with its category. Adding an abort message without deciding its category, or quietly rewording one, now fails a test instead of reaching a client. Behaviour is unchanged; ctx.Err() remains the one detail not declared here, since the Go runtime supplies it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"conflict" is the one category the server cannot narrow. GetConflictKey reduces every mutated edge to farm.Fingerprint64(key)^uid, so once hasConflict matches one, whether it came from a data, index, count or @upsert key is unrecoverable - the fingerprint is one-way and Zero never saw the predicate. Naming the possibilities is the most this category can honestly say. It is worth saying, because the derived keys are the non-obvious part. A caller who sets one scalar value has no reason to expect that the index and count keys generated from it can conflict, or that @upsert makes any two transactions touching the same value conflict regardless of uid. The old message gave a remedy with no way to start looking. Appended, not rewritten. The original sentence stays verbatim at the front because three tests match it against the live server message - notably the retry loop in dgraph/cmd/alpha/upsert_test.go, which drives a raw HTTP mutation and so sees this text rather than dgo.ErrAborted; rewording would exit that loop on the first abort and fail the test. It is also verbatim the text of dgo.ErrAborted, so user log-scrapers key off it. A test pins the prefix so a future edit cannot quietly break them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
commit() could return nil while src.Aborted was true. proposeTxn sets that
flag when it finds no commit timestamp, meaning something aborted the
transaction out of band while this commit was in flight. Nothing in commit()
had set the local `aborted` flag, because this commit itself decided to
proceed - so the abort fell through the categorisation entirely.
CommitOverNetwork then saw tctx.Aborted with no error and returned a bare
dgo.ErrAborted, and the caller learned nothing.
This is not a rare path. All three producers reach Zero through TryAbort:
- a schema or type update on a touched predicate, which cancels pending
transactions so the index can be rebuilt (detectPendingTxns)
- a drop-predicate (S * * delete) on a touched predicate (same function)
- the Alpha leader ageing out transactions idle longer than
--limit "txn-abort-after" (abortOldTransactions)
The category is withheld rather than guessed. TryAbort carries only
timestamps, so Zero records no cause, and none of the three is a predicate
*move* - labelling it predicate-move would be wrong. The detail names all
three instead, the same approach used for stale-startts, which also cannot
narrow to one cause.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Added some clarity to the aborts and looked for any lingering ones and performed some organization with the error messages. This PR is backwards compatible with all clients, but now also providing transaction abort details when possible. Attached is the thoughts on the future PRs/phases (attached plan doc)
New table:
|
2 participants
Problem
When Dgraph aborts a transaction, Zero knows why — but the client never finds out.
Zero distinguishes at least three causes internally:
while a move is in flight), and
change, not a real conflict).
All three collapse into
src.Aborted = trueinsideServer.commit()(
dgraph/cmd/zero/oracle.go), and by the time the error reaches the caller it is oneopaque string:
Worse, the
checkPredspath builds specific, human-readable messages for predicatemoves and then throws them away. With no discriminator, a client cannot make the one
decision the category enables: retry immediately (conflict) vs. back off
(predicate is moving).
Why we can't just name the conflicting predicate
For conflicts, you might expect the abort to name the contended predicate/UID. It can't —
not without extra bookkeeping. For every mutated edge, Alpha computes a conflict key in
GetConflictKey()(posting/list.go) as a one-way fingerprint:Only this
uint64is retained (conflicts map[uint64]struct{}inposting/oracle.go);the predicate name and UID are dropped immediately. It is then base-36 encoded into
TxnContext.Keysand shipped to Zero, which detects a conflict purely by matchingfingerprints — it never sees a predicate name. Because the value is a fingerprint XORed
with a UID, it cannot be inverted to recover either input. Naming the predicate would
require remembering what was hashed (a per-transaction reverse map on Alpha), which is
larger scope. So this PR delivers the category, which needs no reverse mapping, and
leaves predicate/UID fidelity to follow-up work.
Fix
Thread a categorized reason out of Zero to the client. No change to conflict detection,
and no proto change — the reason rides on the existing
codes.AbortedgRPC status as"<reason>: <detail>".dgraph/cmd/zero/oracle.go—commit()tags each abort with a category(
conflict,stale-startts,predicate-move); predicate-move now reuses the existingcheckPredsmessages instead of swallowing them.worker/mutation.go—CommitOverNetworkforwards the reason and records theabort metric, instead of flattening it to the reasonless
dgo.ErrAborted.edgraph/server.go— both commit paths (CommitNowand explicitCommitOrAbort)pass the reason through and set
Aborted.Backward compatible: the existing message is preserved and the status code stays
codes.Aborted, so existing client retry logic is unaffected. This phase iscategory-only; naming the contended predicate/UID is planned follow-up.
Future work
This PR is deliberately scoped to the category only — the cheapest, lowest-risk slice
that is immediately useful to clients — because it needs no reverse mapping, no new proto
fields, and no change to conflict detection. All correctness risk stays in what we
report, not what we decide. Richer fidelity is layered, additive work that can land
independently:
Alpha (
fingerprint → {predicate, uid, kind}), populated where the conflict key isalready computed. The aborting transaction's own Alpha still holds this metadata, so
Zero need only echo back which fingerprint matched — no hash inversion. The cost lands
only on the abort path. Target the single-request
CommitNowpath first.gRPC rich-error model (
ErrorInfodetails) so clients get typed fields instead ofparsing a message string. Still no proto change.
TxnContextfield in thedgoproto, giving uniform structured access across alllanguage clients — at the cost of a coordinated cross-repo release.
predicate-movecategory. Today allcheckPredsfailures report aspredicate-move, but some are not moves (e.g. a malformed/internal predicate key, or apredicate not currently served by any group). A later phase can split these into honest
categories (
predicate-unavailable,internal) and revisit retryability — aninternal/malformed abort is not retryable, unlike a move.Checklist
Conventional Commits syntax, leading
with
fix:,feat:,chore:,ci:, etc.