Repository navigation
Conversation
matthewmcneely
force-pushed
the
txn-abort-reason-surface-phase-1
branch
from
June 18, 2026 17:33
6eff47d to
1042bb1
Compare
shiva-istari
reviewed
Jul 6, 2026
shiva-istari
left a comment
shiva-istari
left a comment
Contributor
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>
Author
|
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>
Author
|
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:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…not truncate CodeQL flagged the strconv.Atoi -> uint32(gid) conversion in checkPreds (go/incorrect-integer-conversion): Atoi returns an arch-dependent int, so a value outside uint32 range — or a negative, which Atoi accepts — silently wrapped in the comparison against tablet.GroupId. ParseUint with bitSize 32 bounds the value to the type it is compared against and rejects negatives at parse time, turning them into the existing "unable to parse group id" abort. The pattern is pre-existing: main has the identical Atoi + uint32(gid) code today inside the checkPreds closure in commit(); the alert fired on this PR only because the lines moved. No legitimate value is affected — Alpha builds these entries from a uint32 group id — so only hand-crafted TxnContext values ever hit the new parse rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XQTSiZLjtkA3QcAPrACvoQ
Author
|
Dropping a polite ping for a review or an estimate for when it can be reviewed. Thanks |
shiva-istari
reviewed
Sep 1, 2026
shiva-istari
left a comment
shiva-istari
left a comment
Contributor
There was a problem hiding this comment.
Latest changes look good to me. One ask before this merges: since centralising the reason strings across the server, client libraries (dgraph4j, pydgraph), and docs is deferred to phases C and D, please file tracking issues for those phases and link them in the PR description so the follow-up work outlives this thread. Otherwise, LGTM!
matthewmcneely
requested changes
Sep 1, 2026
2 of 3 tasks
dgraph-io/dgo#288 makes Txn return an *AbortedError that wraps ErrAborted, carrying the server's abort reason. A direct == comparison against the sentinel stops matching once dgraph moves to that dgo, so the live loader would print aborts as "Error while mutating" and the mutations systests would treat an abort as a failure. errors.Is matches both the current sentinel and the wrapped error, so this is safe ahead of the dgo bump. The edgraph comparisons are left as is: they check worker.CommitOverNetwork's return, not a dgo client error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Abort details from checkPreds name the predicate as the user wrote it. Preds entries carry the namespaced attribute, so clients were about to receive "0-name" in a wire message that has never been published before. displayPred strips the namespace with a guarded strings.Cut, since Preds is client-supplied and x.ParseAttr would panic on an entry without a separator. - CommitOverNetwork emits its "Commit status" span event for server-decided aborts too, now with the reason, so the aborts most worth tracing don't go dark. - The pre-aborted detail lists a concurrent client Discard as a fourth cause and stops reading as exhaustive. proposeTxn's race comment no longer claims a predicate move aborts pending transactions: a move blocks commits in checkPreds, and the move path waits with WaitForTs rather than calling TryAbort. - Comments that described dgraph4j and pydgraph behavior in the present tense now name the open client PRs, including dgraph-io/dgo#288. - TestStaleStartTsAbortReason polls the commit itself until it returns codes.Aborted. A Zero leader existing doesn't mean the Alpha has a connection to it yet, and in that window the commit fails with codes.Unknown. - Copyright headers on the new test files use 2017-2026. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Before abort reasons, every abort a client received read "Transaction has been aborted. Please retry". The predicate-move, stale-startts and uncategorized details dropped that sentence, which breaks clients that recognise aborts by their text. dgraph-js-http, for one, treats an HTTP error as an abort only when its message contains "abort" and "retry" and rethrows anything else, so a predicate-move abort, the one that most needs a retry, became a hard failure. abortReason now frames every detail as "<category>: Transaction has been aborted. Please retry. <detail>", and the details drop their own copy of the sentence. Conflict and pre-aborted messages are byte-identical on the wire. Clients still parse the category from the text before the first colon. Also, the late abort path re-read startTxnTs after Oracle.commit released its lock, so a snapshot purge or lease renewal in between could label a real conflict stale-startts, or the reverse. Oracle.commit now returns errStaleStartTs for that case, decided under its own lock, and the classification moves into lateAbortReason so it is unit-tested directly. The previous test exercised only helpers commit() never called. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Abort messages can now name a predicate, so a predicate-move abort on a predicate called e.g. x509cert would hit the x509 case in handleError and make the live loader exit. The abort check now comes first and also matches dgo.ErrAborted via errors.Is, since dgo hands the loader that rather than the raw status. TestTxnDiscardBeforeCommit asserted NotEqual(err, dgo.ErrAborted), which passes vacuously once dgo returns a wrapping error; it now requires no error. Test comments no longer state that dgo always discards the abort reason, since dgraph-io/dgo#288 keeps it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 8 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the primary change: exposing categorized transaction abort reasons to clients. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 8 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
This was referenced Oct 22, 2026
Contributor
|
@shiva-istari tracking issues for the later phases are up and linked from the description: #9866 (structured @rahst12 thanks for all the work here. I've picked this up to get it over the line and pushed a few commits on top of your branch. Besides the review threads, two changes affect the wire format and the client PRs:
There's also a Go client PR now: dgraph-io/dgo#288. |
go.mod has required go 1.27.3 since dgraph-io#9864, but trunk still ran the go 1.27.0 runtime with golangci-lint 2.13.1. CI auto-switches to the 1.27.3 toolchain, which writes export data version 5, and the x/tools v0.49.0 importer inside golangci-lint 2.13.1 only reads up to version 4. Package x then fails to load, so every promoted x.SafeMutex method reports as undefined, and trunk flags whichever of those land in a changed hunk: s.orc.RLock undefined (type *Oracle has no field or method RLock) golangci-lint 2.15.0 is built on x/tools v0.52.0 and reads the new export data; locally it reports no new issues on this branch under the default, integration and integration2 build tags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 tasks done
matthewmcneely
approved these changes
Oct 22, 2026
matthewmcneely
added a commit
that referenced
this pull request
Oct 23, 2026
**Description** This PR bumps trunk's Go runtime to `go@1.27.3` and golangci-lint to `2.15.0`, so `trunk check` can lint the module again. #9864 moved `go.mod` to `go 1.27.3`, but trunk still pins `go@1.27.0` and golangci-lint `2.13.1`. CI switches to the 1.27.3 toolchain automatically, and 1.27.3 writes export data version 5. The `x/tools` v0.49.0 importer inside golangci-lint 2.13.1 only reads up to version 4: ``` could not import internal/cpu (... export data version 5 is greater than maximum supported version 4) ``` So package `x` fails to load, every promoted `x.SafeMutex` method reports as undefined, and trunk flags whichever of those land in a changed hunk. #9747 hit it first: ``` dgraph/cmd/zero/oracle.go:602:8 s.orc.RLock undefined (type *Oracle has no field or method RLock) golangci-lint2/typecheck ``` #9864 itself passed because it changed no Go code. Any PR that touches Go files using `SafeMutex` (or anything else in `x`) will fail the same way until this lands. golangci-lint 2.15.0 is built on `x/tools` v0.52.0 and reads the new export data. I reproduced the failure locally with 2.13.1, then ran 2.15.0 under Go 1.27.3 with the repo config: no new issues on #9747 under the default, `integration` and `integration2` build tags. **Checklist** - [x] The PR title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) syntax, leading with `fix:`, `feat:`, `chore:`, `ci:`, etc. - [x] Code compiles correctly and linting (via trunk) passes locally <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/dgraph-io/codesmith/dgraph/pr/9868?autoLogin=true&ref=codesmith_pr_footer"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3-3x.png" width="162" height="28"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1795291376&installation_model_id=8575&pr_number=9868&ref=codesmith_pr_footer&repository=dgraph-io%2Fdgraph&return_to=https%3A%2F%2Fgithub.com%2Fdgraph-io%2Fdgraph%2Fpull%2F9868&signature=c8242b1e55f9f0f9dacaa280a2232c48647eb428de7b10ecd5bfc62d42f4cbac"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark-3x.png" width="190" height="28"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Chores** * Updated the versions of the Go runtime and Go linting tools used in the project. These are internal tooling updates and do not introduce user-facing changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
![https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source](https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v3.svg"><source){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img](https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v3.svg"><img){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source](https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source){kind=link}
![https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img](https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img){kind=link}
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.
Problem
When Dgraph aborts a transaction, Zero knows why, but the client never finds out.
Server.commit()(dgraph/cmd/zero/oracle.go) collapses every cause intosrc.Aborted = true, and the caller gets one opaque string:The
checkPredspath even builds specific messages for predicate moves and then throws them away. Without a discriminator, a client can't make the one decision the category enables: retry now (conflict) or back off (a predicate is moving).The server can't name the conflicting predicate.
GetConflictKey()(posting/list.go) reduces every mutated edge tofarm.Fingerprint64(key) ^ uid, and Zero only ever matches fingerprints. Naming the predicate would need a per-transaction reverse map on the Alpha, which is follow-up work. This PR delivers the category.Wire format
There's no proto change. The reason rides on the existing
codes.Abortedstatus:conflict@upsert)stale-starttsisBlocked)predicate-moveCommits on predicate <name> are blocked due to predicate movepredicate-moveMutation done in group: 1. Predicate <name> assigned to 2Predsentry, cancelled or timed-out context, or already aborted out of bandSome details on the format:
conflict:prefix and the appended explanation.UNKNOWN, just like every abort from an older server.name, not the internal0-name).Changes
dgraph/cmd/zero/oracle.gocommit()tags each abort with its category.checkPredsreturns its category alongside the error, and checksisBlockedbefore the tablet lookup so an in-flight move is always reported as a move.Oracle.commitreturnserrStaleStartTsso the late (post-lease) path can tell stale from conflict under the oracle lock.txn-abort-after, or a concurrent Discard) used to fall through as a baredgo.ErrAborted. They're reported now.worker/mutation.go:CommitOverNetworkforwards the categorized status, records the abort metric, and emits theCommit statusspan event with the reason.edgraph/server.go:doMutate(CommitNow) andCommitOrAbortpass the status through unchanged.dgraph/cmd/live/batch.go: checks for an abort before thex509match, so an abort that names a predicate likex509certcan't make the loader exit. It also useserrors.Isfordgo.ErrAborted.systest/mutations-and-queries:errors.Isinstead of==againstdgo.ErrAborted, ahead of the dgo change below.Backward compatible: the status code stays
codes.Aborted, and every abort message still contains the legacy sentence.Clients
dgo#288 returns an
*AbortedErrorthat wrapsdgo.ErrAborted, soerrors.Iskeeps working but a direct==comparison doesn't. That's why this PR switches the in-repo comparisons toerrors.Isfirst.Testing
dgraph/cmd/zero/oracle_reason_test.go: the full vocabulary and its framing,checkPredscategory selection (including namespaced predicates),displayPredon client-supplied input, and late-path classification (lateAbortReason) including context-vs-oracle precedence.dgraph/cmd/alpha/txn_test.go: conflict and group-mismatch aborts against a live cluster through the raw gRPC stub.systest/integration2/txn_abort_reason_test.go: stale-startts end to end, restarting Zero and polling the commit until the Alpha can reach it.AbortReasonConflictwith the server's full message.Follow-up
ErrorInfo): feat(txn): carry abort reasons as structured gRPC error details #9866.TxnContext: feat(txn): add an abort reason field to TxnContext #9867.reasonlabel on theTxnAbortsmetric.Predsis client-supplied and isn't checked against the caller's namespace. A tenant can already probe another namespace's predicate placement by trying group ids; the new details make that one call instead of a few.Checklist
fix:,feat:,chore:,ci:, etc.Summary by CodeRabbit