From 11ff9ecde0be2a113a9078dbef87c7394a76d199 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Mon, 15 Jun 2026 21:02:45 -0400 Subject: [PATCH 1/6] first draft of surfacing the reason transaction abort/error occured --- dgraph/cmd/alpha/txn_test.go | 63 ++++++++++++++++++++++++++++++++++++ dgraph/cmd/zero/oracle.go | 62 +++++++++++++++++++++++++++++++---- edgraph/server.go | 10 ++++++ worker/mutation.go | 12 +++++++ 4 files changed, 140 insertions(+), 7 deletions(-) diff --git a/dgraph/cmd/alpha/txn_test.go b/dgraph/cmd/alpha/txn_test.go index 0e20c8d8271..54f78682a1d 100644 --- a/dgraph/cmd/alpha/txn_test.go +++ b/dgraph/cmd/alpha/txn_test.go @@ -21,6 +21,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" "github.com/dgraph-io/dgo/v250" "github.com/dgraph-io/dgo/v250/protos/api" @@ -248,6 +253,64 @@ func TestConflict(t *testing.T) { require.True(t, bytes.Equal(resp.Json, []byte("{\"me\":[{\"name\":\"Manish\"}]}"))) } +// TestConflictAbortReason proves the server emits the categorized abort reason on the +// gRPC status of a write-write conflict: the code stays codes.Aborted (so existing +// clients keep retrying) and the message is prefixed with "conflict: ". This is exactly +// the unflattened status a gRPC client such as dgraph4j receives and parses into +// TxnConflictException.AbortReason. +// +// It must inspect the raw CommitOrAbort response rather than dgo's high-level +// Txn.Commit, because dgo intentionally replaces any codes.Aborted error with the +// static dgo.ErrAborted (txn.go), discarding the reason for the Go client. +func TestConflictAbortReason(t *testing.T) { + op := &api.Operation{} + op.DropAll = true + require.NoError(t, dg.Alter(context.Background(), op)) + + // First transaction creates a node with a name. + txn := dg.NewTxn() + mu := &api.Mutation{} + mu.SetJson = []byte(`{"name": "Manish"}`) + assigned, err := txn.Mutate(context.Background(), mu) + require.NoError(t, err) + require.Len(t, assigned.Uids, 1) + var uid string + for _, u := range assigned.Uids { + uid = u + } + + // Second transaction writes the same predicate on the same uid -> conflicts. + txn2 := dg.NewTxn() + mu = &api.Mutation{} + mu.SetJson = []byte(fmt.Sprintf(`{"uid": %q, "name": "Manish"}`, uid)) + resp2, err := txn2.Mutate(context.Background(), mu) + require.NoError(t, err) + require.NotEmpty(t, resp2.GetTxn().GetKeys(), "mutation response must carry conflict keys") + + // First commit wins; its commitTs is now greater than txn2's startTs. + require.NoError(t, txn.Commit(context.Background())) + + // Commit the loser via the raw gRPC stub so we observe the unflattened status the + // server sends (dgo's Txn.Commit would replace it with the reasonless ErrAborted). + conn, err := grpc.NewClient(alphaSockAdd, grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + defer func() { _ = conn.Close() }() + raw := api.NewDgraphClient(conn) + + // TestMain enables ACL, so the raw stub (unlike the logged-in dg client) must + // carry the access JWT; CommitOrAbort goes through the same auth interceptor. + ctx := metadata.NewOutgoingContext(context.Background(), + metadata.Pairs("accessJwt", hc.AccessJwt)) + _, err = raw.CommitOrAbort(ctx, resp2.GetTxn()) + require.Error(t, err) + + st := status.Convert(err) + require.Equal(t, codes.Aborted, st.Code(), + "abort must keep codes.Aborted so existing clients still retry") + require.True(t, strings.HasPrefix(st.Message(), "conflict: "), + "abort reason should be categorized as conflict; got %q", st.Message()) +} + func TestConflictTimeout(t *testing.T) { var uid string txn := dg.NewTxn() diff --git a/dgraph/cmd/zero/oracle.go b/dgraph/cmd/zero/oracle.go index cee15fae72f..900907d35ae 100644 --- a/dgraph/cmd/zero/oracle.go +++ b/dgraph/cmd/zero/oracle.go @@ -18,6 +18,8 @@ import ( "go.opentelemetry.io/otel" attribute "go.opentelemetry.io/otel/attribute" trace "go.opentelemetry.io/otel/trace" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/dgraph-io/badger/v4/y" "github.com/dgraph-io/dgo/v250/protos/api" @@ -338,21 +340,56 @@ func (s *Server) proposeTxn(ctx context.Context, src *api.TxnContext) error { return nil } +// Abort-reason codes. When Zero decides to abort a transaction it surfaces the category to the +// client as the prefix of a codes.Aborted gRPC status message, formatted as ": ". +// api.TxnContext (external dgo module) has no field for the reason, so it rides on the error; +// the status code stays codes.Aborted, so existing abort handling is unaffected. The dgraph4j +// client parses these prefixes into TxnConflictException.AbortReason — keep the two in sync. +const ( + abortReasonConflict = "conflict" + abortReasonStaleStartTs = "stale-startts" + abortReasonPredicateMove = "predicate-move" +) + +// abortReason builds the wire string the client parses: ": ". +func abortReason(code, detail string) string { + return code + ": " + detail +} + func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { span := trace.SpanFromContext(ctx) span.SetAttributes(attribute.Int64("startTs", int64(src.StartTs))) if src.Aborted { + // Client-initiated discard (txn.Discard); not a server-decided abort, so no reason. return s.proposeTxn(ctx, src) } + // abortWithReason marks the txn aborted, proposes it (advancing the watermark exactly as the + // previous code did), and then surfaces the categorized reason to the caller as a gRPC + // Aborted status. proposeTxn still runs identically — only the post-propose return changes. + abortWithReason := func(reason string) error { + span.SetAttributes(attribute.Bool("abort", true)) + src.Aborted = true + if err := s.proposeTxn(ctx, src); err != nil { + return err + } + return status.Error(codes.Aborted, reason) + } + // Use the start timestamp to check if we have a conflict, before we need to assign a commit ts. s.orc.RLock() conflict := s.orc.hasConflict(src) + // A txn whose startTs predates this leader's lease is aborted by hasConflict, but it's a + // leader change rather than a write-write conflict — report it distinctly. + stale := src.StartTs < s.orc.startTxnTs s.orc.RUnlock() if conflict { - span.SetAttributes(attribute.Bool("abort", true)) - src.Aborted = true - return s.proposeTxn(ctx, src) + if stale { + return abortWithReason(abortReason(abortReasonStaleStartTs, + "Transaction has been aborted due to a leader change. Please retry")) + } + return abortWithReason(abortReason(abortReasonConflict, + "Transaction has been aborted. Please retry")) } checkPreds := func() error { @@ -385,9 +422,9 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { return nil } if err := checkPreds(); err != nil { - span.SetAttributes(attribute.Bool("abort", true)) - src.Aborted = true - return s.proposeTxn(ctx, src) + // checkPreds builds rich messages, e.g. "Commits on predicate %s are blocked due to + // predicate move" — forward them instead of swallowing the reason. + return abortWithReason(abortReason(abortReasonPredicateMove, err.Error())) } num := pb.Num{Val: 1, Type: pb.Num_TXN_TS} @@ -402,16 +439,27 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { span.SetAttributes(attribute.Int64("nodeId", int64(s.Node.Id))) span.AddEvent(fmt.Sprintf("TXN Context: %+v", src)) + aborted := false if err := s.orc.commit(src); err != nil { span.SetAttributes(attribute.Bool("abort", true)) src.Aborted = true + aborted = true } if err := ctx.Err(); err != nil { span.SetAttributes(attribute.Bool("abort", true)) src.Aborted = true + aborted = true } // Propose txn should be used to set watermark as done. - return s.proposeTxn(ctx, src) + if err := s.proposeTxn(ctx, src); err != nil { + return err + } + if aborted { + // A late write-write conflict detected at commit time (keyCommit), or a cancelled ctx. + return status.Error(codes.Aborted, abortReason(abortReasonConflict, + "Transaction has been aborted. Please retry")) + } + return nil } // CommitOrAbort either commits a transaction or aborts it. diff --git a/edgraph/server.go b/edgraph/server.go index 0da2fd32c62..26744ee3818 100644 --- a/edgraph/server.go +++ b/edgraph/server.go @@ -646,6 +646,10 @@ func (s *Server) doMutate(ctx context.Context, qc *queryContext, resp *api.Respo if err == dgo.ErrAborted { err = status.Error(codes.Aborted, err.Error()) resp.Txn.Aborted = true + } else if status.Code(err) == codes.Aborted { + // Server-decided abort carrying a categorized reason; err is already a codes.Aborted + // status (e.g. "conflict: ...", "predicate-move: ...") — surface it unchanged. + resp.Txn.Aborted = true } return err @@ -2085,6 +2089,12 @@ func (s *Server) CommitOrAbort(ctx context.Context, tc *api.TxnContext) (*api.Tx return tctx, status.Error(codes.Aborted, err.Error()) } + if status.Code(err) == codes.Aborted { + // Server-decided abort carrying a categorized reason; err is already a codes.Aborted + // status (e.g. "conflict: ...", "predicate-move: ...") — surface it unchanged. + tctx.Aborted = true + return tctx, err + } tctx.StartTs = tc.StartTs tctx.CommitTs = commitTs return tctx, err diff --git a/worker/mutation.go b/worker/mutation.go index fdac2a41c1b..1f0298bed14 100644 --- a/worker/mutation.go +++ b/worker/mutation.go @@ -20,7 +20,9 @@ import ( "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "github.com/dgraph-io/badger/v4" @@ -908,6 +910,16 @@ func CommitOverNetwork(ctx context.Context, tc *api.TxnContext) (uint64, error) tctx, err := zc.CommitOrAbort(ctx, tc) if err != nil { + // Zero signals a server-decided abort as a codes.Aborted status carrying a categorized + // reason (e.g. "conflict: ...", "predicate-move: ..."). Record the abort metric and + // forward the reason rather than treating it as a generic transport error or flattening + // it to the reasonless dgo.ErrAborted. + if status.Code(err) == codes.Aborted { + if !clientDiscard { + ostats.Record(ctx, x.TxnAborts.M(1)) + } + return 0, err + } span.AddEvent("Error in CommitOrAbort", trace.WithAttributes( attribute.String("error", err.Error()))) return 0, err From 1042bb19b9dbb633254f997e40a4e3378ca9d3d1 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Wed, 17 Jun 2026 01:25:36 -0400 Subject: [PATCH 2/6] adding test --- dgraph/cmd/alpha/txn_test.go | 53 ++++++++++++ dgraph/cmd/zero/oracle.go | 26 ++++-- dgraph/cmd/zero/oracle_reason_test.go | 63 ++++++++++++++ systest/integration2/txn_abort_reason_test.go | 86 +++++++++++++++++++ 4 files changed, 220 insertions(+), 8 deletions(-) create mode 100644 dgraph/cmd/zero/oracle_reason_test.go create mode 100644 systest/integration2/txn_abort_reason_test.go diff --git a/dgraph/cmd/alpha/txn_test.go b/dgraph/cmd/alpha/txn_test.go index 54f78682a1d..aac226b6e9e 100644 --- a/dgraph/cmd/alpha/txn_test.go +++ b/dgraph/cmd/alpha/txn_test.go @@ -311,6 +311,59 @@ func TestConflictAbortReason(t *testing.T) { "abort reason should be categorized as conflict; got %q", st.Message()) } +// TestPredicateMoveAbortReason proves the "predicate-move" category is surfaced on the +// gRPC status. Zero's checkPreds aborts a commit whose predicate keys don't match the +// tablet's serving group (the same code path that rejects commits during a predicate +// move). We reach it deterministically by committing a real txn context whose Preds have +// been rewritten to claim a group that doesn't serve the predicate, with conflict Keys +// cleared so hasConflict passes and checkPreds runs. +// +// As with TestConflictAbortReason, it uses the raw CommitOrAbort stub to observe the +// unflattened status the server sends. +func TestPredicateMoveAbortReason(t *testing.T) { + op := &api.Operation{} + op.DropAll = true + require.NoError(t, dg.Alter(context.Background(), op)) + + // A normal mutation gives us a valid, fresh TxnContext (real StartTs and Preds). + txn := dg.NewTxn() + mu := &api.Mutation{SetJson: []byte(`{"name": "Alice"}`)} + resp, err := txn.Mutate(context.Background(), mu) + require.NoError(t, err) + + tc := resp.GetTxn() + require.NotEmpty(t, tc.GetPreds(), "mutation response must carry predicate keys") + + // Rewrite each predicate key's group id to a group that does not serve it, and drop + // the conflict keys so hasConflict is false and the commit reaches checkPreds. + doctored := make([]string, 0, len(tc.GetPreds())) + for _, p := range tc.GetPreds() { + // Preds look like "-"; claim a nonexistent group 100. + if idx := strings.IndexByte(p, '-'); idx >= 0 { + doctored = append(doctored, "100"+p[idx:]) + } + } + require.NotEmpty(t, doctored, "expected parseable predicate keys") + tc.Preds = doctored + tc.Keys = nil + + conn, err := grpc.NewClient(alphaSockAdd, grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + defer func() { _ = conn.Close() }() + raw := api.NewDgraphClient(conn) + + ctx := metadata.NewOutgoingContext(context.Background(), + metadata.Pairs("accessJwt", hc.AccessJwt)) + _, err = raw.CommitOrAbort(ctx, tc) + require.Error(t, err) + + st := status.Convert(err) + require.Equal(t, codes.Aborted, st.Code(), + "abort must keep codes.Aborted so existing clients still retry") + require.True(t, strings.HasPrefix(st.Message(), "predicate-move: "), + "abort reason should be categorized as predicate-move; got %q", st.Message()) +} + func TestConflictTimeout(t *testing.T) { var uid string txn := dg.NewTxn() diff --git a/dgraph/cmd/zero/oracle.go b/dgraph/cmd/zero/oracle.go index 900907d35ae..d29207ca814 100644 --- a/dgraph/cmd/zero/oracle.go +++ b/dgraph/cmd/zero/oracle.go @@ -351,11 +351,27 @@ const ( abortReasonPredicateMove = "predicate-move" ) +// Human-readable details paired with the conflict abort codes. +const ( + abortDetailConflict = "Transaction has been aborted. Please retry" + abortDetailStaleStartTs = "Transaction has been aborted due to a leader change. Please retry" +) + // abortReason builds the wire string the client parses: ": ". func abortReason(code, detail string) string { return code + ": " + detail } +// conflictAbortReason returns the wire reason for a hasConflict abort, distinguishing a +// write-write conflict from a stale start timestamp (a txn that predates the current +// leader's lease, i.e. a leader change rather than a real conflict). +func conflictAbortReason(stale bool) string { + if stale { + return abortReason(abortReasonStaleStartTs, abortDetailStaleStartTs) + } + return abortReason(abortReasonConflict, abortDetailConflict) +} + func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { span := trace.SpanFromContext(ctx) span.SetAttributes(attribute.Int64("startTs", int64(src.StartTs))) @@ -384,12 +400,7 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { stale := src.StartTs < s.orc.startTxnTs s.orc.RUnlock() if conflict { - if stale { - return abortWithReason(abortReason(abortReasonStaleStartTs, - "Transaction has been aborted due to a leader change. Please retry")) - } - return abortWithReason(abortReason(abortReasonConflict, - "Transaction has been aborted. Please retry")) + return abortWithReason(conflictAbortReason(stale)) } checkPreds := func() error { @@ -456,8 +467,7 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { } if aborted { // A late write-write conflict detected at commit time (keyCommit), or a cancelled ctx. - return status.Error(codes.Aborted, abortReason(abortReasonConflict, - "Transaction has been aborted. Please retry")) + return status.Error(codes.Aborted, conflictAbortReason(false)) } return nil } diff --git a/dgraph/cmd/zero/oracle_reason_test.go b/dgraph/cmd/zero/oracle_reason_test.go new file mode 100644 index 00000000000..2e2d982c151 --- /dev/null +++ b/dgraph/cmd/zero/oracle_reason_test.go @@ -0,0 +1,63 @@ +/* + * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package zero + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/dgraph-io/dgo/v250/protos/api" +) + +// The abort-reason wire format is a contract with gRPC clients (e.g. dgraph4j parses the +// ": " prefix into TxnConflictException.AbortReason). These unit tests pin the +// category prefixes and the logic that selects between them, so the contract can't drift +// silently without an integration cluster. + +func TestAbortReasonFormat(t *testing.T) { + require.Equal(t, "conflict: boom", abortReason(abortReasonConflict, "boom")) + require.Equal(t, "stale-startts: x", abortReason(abortReasonStaleStartTs, "x")) + require.Equal(t, "predicate-move: y", abortReason(abortReasonPredicateMove, "y")) +} + +func TestConflictAbortReason(t *testing.T) { + // Write-write conflict. + r := conflictAbortReason(false) + require.True(t, strings.HasPrefix(r, abortReasonConflict+": "), + "want conflict prefix, got %q", r) + require.Equal(t, abortReason(abortReasonConflict, abortDetailConflict), r) + + // Stale start timestamp (leader change). + r = conflictAbortReason(true) + require.True(t, strings.HasPrefix(r, abortReasonStaleStartTs+": "), + "want stale-startts prefix, got %q", r) + require.Equal(t, abortReason(abortReasonStaleStartTs, abortDetailStaleStartTs), r) + require.Contains(t, r, "leader change") +} + +// TestHasConflictStaleStartTs pins the exact discriminator commit() uses to choose the +// stale-startts reason: a txn whose startTs predates the leader's startTxnTs lease is a +// conflict, and is flagged stale; a fresh startTs with no conflicting keys is neither. +func TestHasConflictStaleStartTs(t *testing.T) { + o := &Oracle{} + o.Init() + defer o.close() + + o.updateStartTxnTs(100) + + // startTs below the lease floor: hasConflict true, and the stale discriminator true. + stale := &api.TxnContext{StartTs: 42} + require.True(t, o.hasConflict(stale), "txn below startTxnTs must conflict") + require.True(t, stale.StartTs < o.startTxnTs, "must be flagged stale") + require.Equal(t, conflictAbortReason(true), conflictAbortReason(stale.StartTs < o.startTxnTs)) + + // startTs at/above the lease floor with no keys: not a conflict, not stale. + fresh := &api.TxnContext{StartTs: 100} + require.False(t, o.hasConflict(fresh), "fresh txn with no keys must not conflict") + require.False(t, fresh.StartTs < o.startTxnTs, "must not be flagged stale") +} diff --git a/systest/integration2/txn_abort_reason_test.go b/systest/integration2/txn_abort_reason_test.go new file mode 100644 index 00000000000..acf87f21167 --- /dev/null +++ b/systest/integration2/txn_abort_reason_test.go @@ -0,0 +1,86 @@ +//go:build integration2 + +/* + * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package main + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/status" + + "github.com/dgraph-io/dgo/v250/protos/api" + "github.com/dgraph-io/dgraph/v25/dgraphtest" +) + +// TestStaleStartTsAbortReason proves the "stale-startts" abort category end-to-end against a +// real cluster. A transaction's start timestamp becomes "stale" when it predates the current +// Zero leader's lease — i.e. after a leader change. We reproduce that deterministically by +// opening a transaction, then restarting the (single) Zero: on restart Zero renews its lease and +// advances startTxnTs past every previously-leased start ts, so committing the now-old txn aborts +// with the stale-startts reason rather than a plain write-write conflict. +// +// As in the alpha-level reason tests, the commit goes through the raw CommitOrAbort stub so we +// observe the unflattened codes.Aborted status (dgo's Txn.Commit would replace it with the +// reasonless ErrAborted). +func TestStaleStartTsAbortReason(t *testing.T) { + conf := dgraphtest.NewClusterConfig().WithNumAlphas(1).WithNumZeros(1).WithReplicas(1) + c, err := dgraphtest.NewLocalCluster(conf) + require.NoError(t, err) + t.Cleanup(func() { c.Cleanup(t.Failed()) }) + require.NoError(t, c.Start()) + + gc, cleanup, err := c.Client() + require.NoError(t, err) + defer cleanup() + + ctx := context.Background() + require.NoError(t, gc.Alter(ctx, &api.Operation{DropAll: true})) + + // Open a transaction and mutate so it gets a real (soon-to-be-stale) start ts and keys. + txn := gc.NewTxn() + resp, err := txn.Mutate(ctx, &api.Mutation{SetJson: []byte(`{"name": "Manish"}`)}) + require.NoError(t, err) + tc := resp.GetTxn() + require.NotZero(t, tc.GetStartTs(), "mutation must yield a start ts") + + // Restart Zero. On coming back up it renews its lease and sets startTxnTs to MaxTxnTs+1, + // which is strictly greater than the start ts leased above — making our open txn stale. + require.NoError(t, c.StopZero(0)) + require.NoError(t, c.StartZero(0)) + require.NoError(t, c.HealthCheck(false)) + + // Wait until a Zero leader is established again (lease renewal, hence startTxnTs bump, runs + // when a Zero becomes leader). Avoids racing the commit against the leaderless window. + require.Eventually(t, func() bool { + _, err := c.GetZeroLeader(0) + return err == nil + }, 60*time.Second, time.Second, "zero leader did not re-establish after restart") + + // Commit the stale txn via the raw stub to observe the categorized status. + port, err := c.GetAlphaGrpcPublicPort(0) + require.NoError(t, err) + conn, err := grpc.NewClient("0.0.0.0:"+port, grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + defer func() { _ = conn.Close() }() + raw := api.NewDgraphClient(conn) + + _, err = raw.CommitOrAbort(ctx, tc) + require.Error(t, err, "committing a txn whose start ts predates the new leader must abort") + + st := status.Convert(err) + require.Equal(t, codes.Aborted, st.Code(), + "abort must keep codes.Aborted so existing clients still retry") + require.True(t, strings.HasPrefix(st.Message(), "stale-startts: "), + "abort reason should be categorized as stale-startts; got %q", st.Message()) +} From 59855c9fbb4e9a30a5dae18385cc22a5e4615f05 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Thu, 30 Jul 2026 01:51:49 -0400 Subject: [PATCH 3/6] fix(txn): report the cause an abort actually had, not the one nearby 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 --- dgraph/cmd/alpha/txn_test.go | 2 +- dgraph/cmd/zero/oracle.go | 180 +++++++++++++----- dgraph/cmd/zero/oracle_reason_test.go | 154 ++++++++++++++- systest/integration2/txn_abort_reason_test.go | 10 +- 4 files changed, 282 insertions(+), 64 deletions(-) diff --git a/dgraph/cmd/alpha/txn_test.go b/dgraph/cmd/alpha/txn_test.go index 813eb17a552..c95372e7a58 100644 --- a/dgraph/cmd/alpha/txn_test.go +++ b/dgraph/cmd/alpha/txn_test.go @@ -314,7 +314,7 @@ func TestConflictAbortReason(t *testing.T) { // TestPredicateMoveAbortReason proves the "predicate-move" category is surfaced on the // gRPC status. Zero's checkPreds aborts a commit whose predicate keys don't match the // tablet's serving group (the same code path that rejects commits during a predicate -// move). We reach it deterministically by committing a real txn context whose Preds have +// move). We reach it deterministically by committing a real transaction context whose Preds have // been rewritten to claim a group that doesn't serve the predicate, with conflict Keys // cleared so hasConflict passes and checkPreds runs. // diff --git a/dgraph/cmd/zero/oracle.go b/dgraph/cmd/zero/oracle.go index dcfc2d70b7c..b3993b6338f 100644 --- a/dgraph/cmd/zero/oracle.go +++ b/dgraph/cmd/zero/oracle.go @@ -342,10 +342,22 @@ func (s *Server) proposeTxn(ctx context.Context, src *api.TxnContext) error { // Abort-reason codes. When Zero decides to abort a transaction it surfaces the category to the // client as the prefix of a codes.Aborted gRPC status message, formatted as ": ". -// api.TxnContext (external dgo module) has no field for the reason, so it rides on the error; -// the status code stays codes.Aborted, so existing abort handling is unaffected. The dgraph4j -// client parses these prefixes into TxnConflictException.AbortReason — keep the two in sync. +// api.TxnContext (external dgo module) has no field for the reason, so the reason rides on the +// error instead; the status code stays codes.Aborted, so existing abort handling is unaffected. +// The dgraph4j and pydgraph clients parse these prefixes into their AbortReason enums — keep all +// three in sync. +// +// These three codes are the entire published vocabulary. A cause that cannot be mapped onto one of +// them emits the detail with *no* prefix (abortReasonUncategorized) rather than borrowing a code +// that would imply the wrong remedy. Both dgraph4j and pydgraph already degrade an absent or +// unrecognized prefix to UNKNOWN, so "no category" is a supported and truthful answer. Withholding +// also keeps a later phase purely additive: naming a new category then only adds precision to +// something that was already UNKNOWN, and never reclassifies a code clients have shipped against. const ( + // abortReasonUncategorized means the server knows what happened and says so in the detail, + // but the cause does not correspond to any published category. It is not "the server does not + // know" — it is "there is no code for this yet". + abortReasonUncategorized = "" abortReasonConflict = "conflict" abortReasonStaleStartTs = "stale-startts" abortReasonPredicateMove = "predicate-move" @@ -353,18 +365,29 @@ const ( // Human-readable details paired with the conflict abort codes. const ( - abortDetailConflict = "Transaction has been aborted. Please retry" - abortDetailStaleStartTs = "Transaction has been aborted due to a leader change. Please retry" + abortDetailConflict = "Transaction has been aborted. Please retry" + // A start timestamp goes stale for two different reasons, so this detail deliberately names + // both rather than asserting one. Zero raises startTxnTs either when a Zero becomes leader and + // renews its leases (updateLeases, assign.go) or when it trims its conflict map while applying + // a snapshot (purgeBelow via applySnapshot, raft.go) — the second is not a leader change at + // all. Both mean the same thing to a caller: this transaction's start timestamp is older than + // the oldest timestamp Zero can still validate against, so retry with a fresh transaction. + abortDetailStaleStartTs = "Transaction start timestamp is older than the oldest timestamp " + + "Zero can still validate (Zero leader change, or its conflict map was trimmed at a " + + "snapshot). Please retry" ) -// abortReason builds the wire string the client parses: ": ". +// abortReason builds the wire string the client parses: ": ". An empty code yields +// the bare detail, which is exactly what a client receives from a server without this feature. func abortReason(code, detail string) string { + if code == abortReasonUncategorized { + return detail + } return code + ": " + detail } -// conflictAbortReason returns the wire reason for a hasConflict abort, distinguishing a -// write-write conflict from a stale start timestamp (a txn that predates the current -// leader's lease, i.e. a leader change rather than a real conflict). +// conflictAbortReason returns the wire reason for an abort decided by hasConflict, distinguishing a +// write-write conflict from a stale start timestamp. See isStaleStartTs for what "stale" means. func conflictAbortReason(stale bool) string { if stale { return abortReason(abortReasonStaleStartTs, abortDetailStaleStartTs) @@ -372,16 +395,74 @@ func conflictAbortReason(stale bool) string { return abortReason(abortReasonConflict, abortDetailConflict) } +// isStaleStartTs reports whether this transaction's start timestamp (src.StartTs) is below the +// floor Zero can still validate against (startTxnTs). hasConflict aborts such a transaction, but +// the cause is not a write-write conflict, so it is reported under its own category. See +// abortDetailStaleStartTs for the two ways that floor rises. Callers must hold at least a read +// lock on the oracle. +func (o *Oracle) isStaleStartTs(src *api.TxnContext) bool { + return src.StartTs < o.startTxnTs +} + +// checkPreds reports whether any predicate this transaction touched is unusable, in which case the +// transaction must be aborted. It returns the abort category alongside the error because its exits +// are unrelated causes, not degrees of one cause: only a group mismatch and a blocked tablet are +// predicate moves. A malformed entry in preds, or a predicate that no group serves, are different +// failures needing different remedies — retrying a malformed predicate key can never succeed — so +// those return abortReasonUncategorized rather than claiming a move that did not happen. Every exit +// still aborts the transaction, exactly as before; only the reported category changes. +// +// Each entry in preds is a "-" string built by the Alpha. +func (s *Server) checkPreds(preds []string) (string, error) { + for _, pkey := range preds { + splits := strings.SplitN(pkey, "-", 2) + if len(splits) < 2 { + return abortReasonUncategorized, errors.Errorf("Unable to find group id in %s", pkey) + } + gid, err := strconv.Atoi(splits[0]) + if err != nil { + return abortReasonUncategorized, errors.Wrapf(err, + "unable to parse group id from %s", pkey) + } + pred := splits[1] + if strings.Contains(pred, hnsw.VecKeyword) { + pred = pred[0:strings.Index(pred, hnsw.VecKeyword)] + } + // Checked before the tablet lookup because blockTablet holds for the entire duration of a + // move: movePredicate defers its unblock past the tablet reassignment proposal, so this is + // the authoritative signal that a move is in flight. Checking it first means an in-flight + // move can never be reported as anything else, and it lets the tablet == nil case below mean + // only "no group serves this predicate" and never "it is moving". + if s.isBlocked(pred) { + return abortReasonPredicateMove, errors.Errorf( + "Commits on predicate %s are blocked due to predicate move", pred) + } + tablet := s.ServingTablet(pred) + if tablet == nil { + return abortReasonUncategorized, errors.Errorf("Tablet for %s is nil", pred) + } + // The predicate finished moving to another group while this transaction was open: the + // mutation was written against group gid, but the predicate now belongs elsewhere. + if tablet.GroupId != uint32(gid) { + return abortReasonPredicateMove, errors.Errorf( + "Mutation done in group: %d. Predicate %s assigned to %d", + gid, pred, tablet.GroupId) + } + } + return abortReasonUncategorized, nil +} + func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { span := trace.SpanFromContext(ctx) span.SetAttributes(attribute.Int64("startTs", int64(src.StartTs))) if src.Aborted { - // Client-initiated discard (txn.Discard); not a server-decided abort, so no reason. + // The client discarded this transaction itself (a Discard call), so the server decided + // nothing and there is no reason to report. return s.proposeTxn(ctx, src) } - // abortWithReason marks the txn aborted, proposes it (advancing the watermark exactly as the - // previous code did), and then surfaces the categorized reason to the caller as a gRPC + // abortWithReason marks the transaction aborted, proposes it (advancing the watermark exactly as + // the previous code did), and then surfaces the categorized reason to the caller as a gRPC // Aborted status. proposeTxn still runs identically — only the post-propose return changes. abortWithReason := func(reason string) error { span.SetAttributes(attribute.Bool("abort", true)) @@ -395,47 +476,18 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { // Use the start timestamp to check if we have a conflict, before we need to assign a commit ts. s.orc.RLock() conflict := s.orc.hasConflict(src) - // A txn whose startTs predates this leader's lease is aborted by hasConflict, but it's a - // leader change rather than a write-write conflict — report it distinctly. - stale := src.StartTs < s.orc.startTxnTs + // hasConflict also aborts a transaction whose start timestamp is too old to validate, which is + // not a write-write conflict, so it is reported under its own category. See isStaleStartTs. + stale := s.orc.isStaleStartTs(src) s.orc.RUnlock() if conflict { return abortWithReason(conflictAbortReason(stale)) } - checkPreds := func() error { - // Check if any of these tablets is being moved. If so, abort the transaction. - for _, pkey := range src.Preds { - splits := strings.SplitN(pkey, "-", 2) - if len(splits) < 2 { - return errors.Errorf("Unable to find group id in %s", pkey) - } - gid, err := strconv.Atoi(splits[0]) - if err != nil { - return errors.Wrapf(err, "unable to parse group id from %s", pkey) - } - pred := splits[1] - if strings.Contains(pred, hnsw.VecKeyword) { - pred = pred[0:strings.Index(pred, hnsw.VecKeyword)] - } - tablet := s.ServingTablet(pred) - if tablet == nil { - return errors.Errorf("Tablet for %s is nil", pred) - } - if tablet.GroupId != uint32(gid) { - return errors.Errorf("Mutation done in group: %d. Predicate %s assigned to %d", - gid, pred, tablet.GroupId) - } - if s.isBlocked(pred) { - return errors.Errorf("Commits on predicate %s are blocked due to predicate move", pred) - } - } - return nil - } - if err := checkPreds(); err != nil { - // checkPreds builds rich messages, e.g. "Commits on predicate %s are blocked due to - // predicate move" — forward them instead of swallowing the reason. - return abortWithReason(abortReason(abortReasonPredicateMove, err.Error())) + if reason, err := s.checkPreds(src.Preds); err != nil { + // checkPreds already builds a specific, human-readable message for each cause. Forward it + // instead of discarding it, tagged with whichever category that cause justifies. + return abortWithReason(abortReason(reason, err.Error())) } num := pb.Num{Val: 1, Type: pb.Num_TXN_TS} @@ -450,15 +502,44 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { span.SetAttributes(attribute.Int64("nodeId", int64(s.Node.Id))) span.AddEvent(fmt.Sprintf("TXN Context: %+v", src)) + // Past this point the transaction already holds a commit timestamp, so an abort here is a "late" + // abort. Two unrelated causes can trigger one, tracked separately because they report + // differently. A conflict found here takes precedence: if the oracle rejected the transaction it + // would have aborted whatever the context said, so that is the honest cause to report. aborted := false + lateReason := abortReasonUncategorized + lateDetail := abortDetailConflict if err := s.orc.commit(src); err != nil { span.SetAttributes(attribute.Bool("abort", true)) src.Aborted = true aborted = true + // Oracle.commit re-runs hasConflict, which aborts a stale start timestamp just as readily as + // a genuine write-write conflict found in the keyCommit tree, and collapses both into + // x.ErrConflict. Re-check staleness here so this path can report stale-startts, instead of + // labelling every late abort a conflict. The early check at the top of commit() already + // draws that distinction; without this, the stale-startts category was only ever reachable + // from the early path. + s.orc.RLock() + stale := s.orc.isStaleStartTs(src) + s.orc.RUnlock() + lateReason = abortReasonConflict + if stale { + lateReason = abortReasonStaleStartTs + lateDetail = abortDetailStaleStartTs + } } if err := ctx.Err(); err != nil { span.SetAttributes(attribute.Bool("abort", true)) src.Aborted = true + if !aborted { + // The context was cancelled or timed out; no conflict occurred. No published category + // describes this, so report the context's own message with no category prefix rather + // than claiming a conflict that did not happen — the client degrades that to UNKNOWN. + // The gRPC status code stays codes.Aborted because the transaction genuinely did abort; + // switching to Canceled or DeadlineExceeded would change retry behaviour for clients + // that already treat Aborted as retryable. + lateDetail = err.Error() + } aborted = true } // Propose txn should be used to set watermark as done. @@ -466,8 +547,7 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { return err } if aborted { - // A late write-write conflict detected at commit time (keyCommit), or a cancelled ctx. - return status.Error(codes.Aborted, conflictAbortReason(false)) + return status.Error(codes.Aborted, abortReason(lateReason, lateDetail)) } return nil } diff --git a/dgraph/cmd/zero/oracle_reason_test.go b/dgraph/cmd/zero/oracle_reason_test.go index 2e2d982c151..114981d114c 100644 --- a/dgraph/cmd/zero/oracle_reason_test.go +++ b/dgraph/cmd/zero/oracle_reason_test.go @@ -7,11 +7,13 @@ package zero import ( "strings" + "sync" "testing" "github.com/stretchr/testify/require" "github.com/dgraph-io/dgo/v250/protos/api" + "github.com/dgraph-io/dgraph/v25/protos/pb" ) // The abort-reason wire format is a contract with gRPC clients (e.g. dgraph4j parses the @@ -25,6 +27,38 @@ func TestAbortReasonFormat(t *testing.T) { require.Equal(t, "predicate-move: y", abortReason(abortReasonPredicateMove, "y")) } +// TestAbortReasonUncategorized pins the withholding contract: when the server cannot substantiate +// one of the published categories it emits the bare detail with no prefix, rather than borrowing a +// category that would imply the wrong remedy. That is byte-identical to what a pre-feature server +// sends, and both dgraph4j and pydgraph already degrade a prefix-less message to UNKNOWN. The +// absence of a colon-delimited prefix is the whole contract, so it must not regress. +func TestAbortReasonUncategorized(t *testing.T) { + require.Equal(t, "Tablet for foo is nil", + abortReason(abortReasonUncategorized, "Tablet for foo is nil")) + require.Equal(t, "context canceled", abortReason(abortReasonUncategorized, "context canceled")) + + // No withheld detail may begin with a token a client would parse as a real category, otherwise + // withholding would be silently reinterpreted as a category. + for _, detail := range []string{ + "Unable to find group id in 1foo", + "Tablet for foo is nil", + "context canceled", + "context deadline exceeded", + } { + got := abortReason(abortReasonUncategorized, detail) + prefix := got + if i := strings.Index(got, ":"); i >= 0 { + prefix = got[:i] + } + for _, code := range []string{ + abortReasonConflict, abortReasonStaleStartTs, abortReasonPredicateMove, + } { + require.NotEqual(t, code, strings.ToLower(strings.TrimSpace(prefix)), + "withheld detail %q would be parsed as category %q", detail, code) + } + } +} + func TestConflictAbortReason(t *testing.T) { // Write-write conflict. r := conflictAbortReason(false) @@ -32,16 +66,90 @@ func TestConflictAbortReason(t *testing.T) { "want conflict prefix, got %q", r) require.Equal(t, abortReason(abortReasonConflict, abortDetailConflict), r) - // Stale start timestamp (leader change). + // Stale start timestamp. The detail must name both ways startTxnTs rises — a Zero leader change + // and a conflict-map trim at a snapshot — because asserting only the first is wrong on the + // second path (purgeBelow via applySnapshot, which involves no leader change at all). r = conflictAbortReason(true) require.True(t, strings.HasPrefix(r, abortReasonStaleStartTs+": "), "want stale-startts prefix, got %q", r) require.Equal(t, abortReason(abortReasonStaleStartTs, abortDetailStaleStartTs), r) require.Contains(t, r, "leader change") + require.Contains(t, r, "snapshot") +} + +// TestCheckPredsCategories is the core of the abort-category fix. checkPreds has five exits and only +// two of them are predicate moves; the other three are unrelated failures with different remedies, +// so they must not be reported as moves. Every case still returns an error (the transaction aborts +// regardless) — this pins only which category each cause claims. +func TestCheckPredsCategories(t *testing.T) { + const pred = "friend" + servingIn := func(gid uint32) *Server { + s := &Server{} + s.blockCommitsOn = new(sync.Map) + s.state = &pb.MembershipState{Groups: map[uint32]*pb.Group{ + gid: {Tablets: map[string]*pb.Tablet{pred: {GroupId: gid, Predicate: pred}}}, + }} + return s + } + + t.Run("in-flight move is reported as a move", func(t *testing.T) { + // The authoritative signal, and it must win even though the tablet still resolves cleanly. + s := servingIn(1) + s.blockCommitsOn.Store(pred, struct{}{}) + reason, err := s.checkPreds([]string{"1-" + pred}) + require.Error(t, err) + require.Equal(t, abortReasonPredicateMove, reason) + require.Contains(t, err.Error(), "blocked due to predicate move") + }) + + t.Run("in-flight move wins over an absent tablet", func(t *testing.T) { + // Regression guard for the check ordering. blockTablet is held for the whole move, so if the + // tablet lookup ran first a genuine in-flight move could be reported as uncategorized. + s := &Server{state: &pb.MembershipState{Groups: map[uint32]*pb.Group{}}} + s.blockCommitsOn = new(sync.Map) + s.blockCommitsOn.Store(pred, struct{}{}) + reason, err := s.checkPreds([]string{"1-" + pred}) + require.Error(t, err) + require.Equal(t, abortReasonPredicateMove, reason, + "isBlocked must be consulted before the tablet lookup") + }) + + t.Run("completed move is reported as a move", func(t *testing.T) { + // Written against group 1, but the predicate now belongs to group 2. + reason, err := servingIn(2).checkPreds([]string{"1-" + pred}) + require.Error(t, err) + require.Equal(t, abortReasonPredicateMove, reason) + require.Contains(t, err.Error(), "assigned to 2") + }) + + t.Run("predicate served by no group is not a move", func(t *testing.T) { + s := &Server{state: &pb.MembershipState{Groups: map[uint32]*pb.Group{}}} + s.blockCommitsOn = new(sync.Map) + reason, err := s.checkPreds([]string{"1-" + pred}) + require.Error(t, err) + require.Equal(t, abortReasonUncategorized, reason, + "an unserved predicate is not a move; retrying may never succeed") + }) + + t.Run("malformed predicate key is not a move", func(t *testing.T) { + s := servingIn(1) + for _, pkey := range []string{pred, "x-" + pred} { + reason, err := s.checkPreds([]string{pkey}) + require.Error(t, err, "pkey %q must abort", pkey) + require.Equal(t, abortReasonUncategorized, reason, + "a malformed predicate key %q is not a move and can never succeed on retry", pkey) + } + }) + + t.Run("healthy predicate does not abort", func(t *testing.T) { + reason, err := servingIn(1).checkPreds([]string{"1-" + pred}) + require.NoError(t, err) + require.Equal(t, abortReasonUncategorized, reason) + }) } // TestHasConflictStaleStartTs pins the exact discriminator commit() uses to choose the -// stale-startts reason: a txn whose startTs predates the leader's startTxnTs lease is a +// stale-startts reason: a transaction whose startTs is below the leader's startTxnTs floor is a // conflict, and is flagged stale; a fresh startTs with no conflicting keys is neither. func TestHasConflictStaleStartTs(t *testing.T) { o := &Oracle{} @@ -50,14 +158,42 @@ func TestHasConflictStaleStartTs(t *testing.T) { o.updateStartTxnTs(100) - // startTs below the lease floor: hasConflict true, and the stale discriminator true. + // startTs below the floor: hasConflict true, and the stale discriminator true. stale := &api.TxnContext{StartTs: 42} - require.True(t, o.hasConflict(stale), "txn below startTxnTs must conflict") - require.True(t, stale.StartTs < o.startTxnTs, "must be flagged stale") - require.Equal(t, conflictAbortReason(true), conflictAbortReason(stale.StartTs < o.startTxnTs)) + require.True(t, o.hasConflict(stale), "transaction below startTxnTs must conflict") + require.True(t, o.isStaleStartTs(stale), "must be flagged stale") + require.Equal(t, conflictAbortReason(true), conflictAbortReason(o.isStaleStartTs(stale))) - // startTs at/above the lease floor with no keys: not a conflict, not stale. + // startTs at/above the floor with no keys: not a conflict, not stale. fresh := &api.TxnContext{StartTs: 100} - require.False(t, o.hasConflict(fresh), "fresh txn with no keys must not conflict") - require.False(t, fresh.StartTs < o.startTxnTs, "must not be flagged stale") + require.False(t, o.hasConflict(fresh), "fresh transaction with no keys must not conflict") + require.False(t, o.isStaleStartTs(fresh), "must not be flagged stale") +} + +// TestLateAbortStaleStartTs guards the late-abort path. Oracle.commit re-runs hasConflict and +// collapses a stale start timestamp and a genuine keyCommit conflict into the same x.ErrConflict, so +// before this fix every late abort was reported as "conflict" and the stale-startts category was +// reachable only from the early check. Both causes are exercised here through the same discriminator +// commit() now uses. +func TestLateAbortStaleStartTs(t *testing.T) { + o := &Oracle{} + o.Init() + defer o.close() + o.updateStartTxnTs(100) + + // Stale: Oracle.commit rejects it, and the late path must report stale-startts, not conflict. + stale := &api.TxnContext{StartTs: 42, CommitTs: 200} + require.Error(t, o.commit(stale), "a stale start timestamp must be rejected by Oracle.commit") + require.True(t, o.isStaleStartTs(stale)) + require.True(t, strings.HasPrefix(conflictAbortReason(o.isStaleStartTs(stale)), + abortReasonStaleStartTs+": "), "late stale abort must not be labelled a conflict") + + // A genuine write-write conflict above the floor still reports conflict. + first := &api.TxnContext{StartTs: 100, CommitTs: 150, Keys: []string{"a"}} + require.NoError(t, o.commit(first)) + second := &api.TxnContext{StartTs: 120, CommitTs: 200, Keys: []string{"a"}} + require.Error(t, o.commit(second), "second writer of key a must conflict") + require.False(t, o.isStaleStartTs(second), "above the floor, so not stale") + require.True(t, strings.HasPrefix(conflictAbortReason(o.isStaleStartTs(second)), + abortReasonConflict+": "), "a real write-write conflict must stay a conflict") } diff --git a/systest/integration2/txn_abort_reason_test.go b/systest/integration2/txn_abort_reason_test.go index acf87f21167..342577c444c 100644 --- a/systest/integration2/txn_abort_reason_test.go +++ b/systest/integration2/txn_abort_reason_test.go @@ -27,7 +27,8 @@ import ( // real cluster. A transaction's start timestamp becomes "stale" when it predates the current // Zero leader's lease — i.e. after a leader change. We reproduce that deterministically by // opening a transaction, then restarting the (single) Zero: on restart Zero renews its lease and -// advances startTxnTs past every previously-leased start ts, so committing the now-old txn aborts +// advances startTxnTs past every previously-leased start timestamp, so committing the now-old +// transaction aborts // with the stale-startts reason rather than a plain write-write conflict. // // As in the alpha-level reason tests, the commit goes through the raw CommitOrAbort stub so we @@ -47,7 +48,7 @@ func TestStaleStartTsAbortReason(t *testing.T) { ctx := context.Background() require.NoError(t, gc.Alter(ctx, &api.Operation{DropAll: true})) - // Open a transaction and mutate so it gets a real (soon-to-be-stale) start ts and keys. + // Open a transaction and mutate so it gets a real (soon-to-be-stale) start timestamp and keys. txn := gc.NewTxn() resp, err := txn.Mutate(ctx, &api.Mutation{SetJson: []byte(`{"name": "Manish"}`)}) require.NoError(t, err) @@ -55,7 +56,8 @@ func TestStaleStartTsAbortReason(t *testing.T) { require.NotZero(t, tc.GetStartTs(), "mutation must yield a start ts") // Restart Zero. On coming back up it renews its lease and sets startTxnTs to MaxTxnTs+1, - // which is strictly greater than the start ts leased above — making our open txn stale. + // which is strictly greater than the start timestamp leased above — making our open + // transaction stale. require.NoError(t, c.StopZero(0)) require.NoError(t, c.StartZero(0)) require.NoError(t, c.HealthCheck(false)) @@ -67,7 +69,7 @@ func TestStaleStartTsAbortReason(t *testing.T) { return err == nil }, 60*time.Second, time.Second, "zero leader did not re-establish after restart") - // Commit the stale txn via the raw stub to observe the categorized status. + // Commit the stale transaction via the raw stub to observe the categorized status. port, err := c.GetAlphaGrpcPublicPort(0) require.NoError(t, err) conn, err := grpc.NewClient("0.0.0.0:"+port, grpc.WithTransportCredentials(insecure.NewCredentials())) From 29d86d082840706317b2b2b9f89a5c81fb71a769 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Thu, 30 Jul 2026 18:34:49 -0400 Subject: [PATCH 4/6] refactor(txn): declare every abort detail in one block 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 --- dgraph/cmd/zero/oracle.go | 37 ++++++++++++----- dgraph/cmd/zero/oracle_reason_test.go | 57 +++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/dgraph/cmd/zero/oracle.go b/dgraph/cmd/zero/oracle.go index b3993b6338f..70e67c8e887 100644 --- a/dgraph/cmd/zero/oracle.go +++ b/dgraph/cmd/zero/oracle.go @@ -363,7 +363,20 @@ const ( abortReasonPredicateMove = "predicate-move" ) -// Human-readable details paired with the conflict abort codes. +// Human-readable details, one per cause. Every message a client can receive for a server-decided +// abort is declared here rather than at its call site, so the published surface is a single block: +// adding a new abort means adding a constant next to the existing vocabulary, where the category it +// should pair with is visible. Entries ending in Fmt are format strings and must be given their +// arguments; the other two are complete messages. +// +// category detail +// ─────────────── ────────────────────────────────────────────────────────── +// conflict abortDetailConflict +// stale-startts abortDetailStaleStartTs +// predicate-move abortDetailPredicateBlockedFmt, abortDetailGroupMismatchFmt +// (withheld) abortDetailMissingGroupIDFmt, abortDetailBadGroupIDFmt, +// abortDetailTabletNilFmt, and ctx.Err() — the last supplied +// by the Go runtime, so it is not declared here const ( abortDetailConflict = "Transaction has been aborted. Please retry" // A start timestamp goes stale for two different reasons, so this detail deliberately names @@ -375,6 +388,14 @@ const ( abortDetailStaleStartTs = "Transaction start timestamp is older than the oldest timestamp " + "Zero can still validate (Zero leader change, or its conflict map was trimmed at a " + "snapshot). Please retry" + + // Details produced by checkPreds. Wording is unchanged from before the abort-reason work; only + // the declaration moved here, so existing log-scrapers and docs still match. + abortDetailMissingGroupIDFmt = "Unable to find group id in %s" + abortDetailBadGroupIDFmt = "unable to parse group id from %s" + abortDetailTabletNilFmt = "Tablet for %s is nil" + abortDetailPredicateBlockedFmt = "Commits on predicate %s are blocked due to predicate move" + abortDetailGroupMismatchFmt = "Mutation done in group: %d. Predicate %s assigned to %d" ) // abortReason builds the wire string the client parses: ": ". An empty code yields @@ -417,12 +438,11 @@ func (s *Server) checkPreds(preds []string) (string, error) { for _, pkey := range preds { splits := strings.SplitN(pkey, "-", 2) if len(splits) < 2 { - return abortReasonUncategorized, errors.Errorf("Unable to find group id in %s", pkey) + return abortReasonUncategorized, errors.Errorf(abortDetailMissingGroupIDFmt, pkey) } gid, err := strconv.Atoi(splits[0]) if err != nil { - return abortReasonUncategorized, errors.Wrapf(err, - "unable to parse group id from %s", pkey) + return abortReasonUncategorized, errors.Wrapf(err, abortDetailBadGroupIDFmt, pkey) } pred := splits[1] if strings.Contains(pred, hnsw.VecKeyword) { @@ -434,20 +454,19 @@ func (s *Server) checkPreds(preds []string) (string, error) { // move can never be reported as anything else, and it lets the tablet == nil case below mean // only "no group serves this predicate" and never "it is moving". if s.isBlocked(pred) { - return abortReasonPredicateMove, errors.Errorf( - "Commits on predicate %s are blocked due to predicate move", pred) + return abortReasonPredicateMove, errors.Errorf(abortDetailPredicateBlockedFmt, pred) } tablet := s.ServingTablet(pred) if tablet == nil { - return abortReasonUncategorized, errors.Errorf("Tablet for %s is nil", pred) + return abortReasonUncategorized, errors.Errorf(abortDetailTabletNilFmt, pred) } // The predicate finished moving to another group while this transaction was open: the // mutation was written against group gid, but the predicate now belongs elsewhere. if tablet.GroupId != uint32(gid) { - return abortReasonPredicateMove, errors.Errorf( - "Mutation done in group: %d. Predicate %s assigned to %d", + return abortReasonPredicateMove, errors.Errorf(abortDetailGroupMismatchFmt, gid, pred, tablet.GroupId) } + } return abortReasonUncategorized, nil } diff --git a/dgraph/cmd/zero/oracle_reason_test.go b/dgraph/cmd/zero/oracle_reason_test.go index 114981d114c..ad1814251ac 100644 --- a/dgraph/cmd/zero/oracle_reason_test.go +++ b/dgraph/cmd/zero/oracle_reason_test.go @@ -6,6 +6,7 @@ package zero import ( + "fmt" "strings" "sync" "testing" @@ -27,6 +28,62 @@ func TestAbortReasonFormat(t *testing.T) { require.Equal(t, "predicate-move: y", abortReason(abortReasonPredicateMove, "y")) } +// TestAbortVocabulary pins the entire published surface: every category, and every detail a client +// can receive for a server-decided abort, with the category each detail pairs with. Because the +// details are declared in one block in oracle.go, adding a new abort message without deciding its +// category — or quietly rewording an existing one that dgraph4j, pydgraph or the docs depend on — +// fails here. The wording assertions are the pre-abort-reason text verbatim, so hoisting these to +// constants is provably a move and not an edit. +func TestAbortVocabulary(t *testing.T) { + // The three published codes, plus the withheld sentinel. Clients switch on exactly these. + require.Equal(t, "", abortReasonUncategorized) + require.Equal(t, "conflict", abortReasonConflict) + require.Equal(t, "stale-startts", abortReasonStaleStartTs) + require.Equal(t, "predicate-move", abortReasonPredicateMove) + + for _, tc := range []struct { + name string + category string + rendered string + }{ + {"write-write conflict", abortReasonConflict, + abortDetailConflict}, + {"stale start timestamp", abortReasonStaleStartTs, + abortDetailStaleStartTs}, + {"move in flight", abortReasonPredicateMove, + fmt.Sprintf(abortDetailPredicateBlockedFmt, "friend")}, + {"move completed", abortReasonPredicateMove, + fmt.Sprintf(abortDetailGroupMismatchFmt, 1, "friend", 2)}, + {"predicate served by no group", abortReasonUncategorized, + fmt.Sprintf(abortDetailTabletNilFmt, "friend")}, + {"malformed key, no separator", abortReasonUncategorized, + fmt.Sprintf(abortDetailMissingGroupIDFmt, "1foo")}, + {"malformed key, bad group id", abortReasonUncategorized, + fmt.Sprintf(abortDetailBadGroupIDFmt, "xfoo")}, + } { + t.Run(tc.name, func(t *testing.T) { + wire := abortReason(tc.category, tc.rendered) + if tc.category == abortReasonUncategorized { + require.Equal(t, tc.rendered, wire, "a withheld cause must carry no prefix") + return + } + require.Equal(t, tc.category+": "+tc.rendered, wire) + }) + } + + // Wording of the checkPreds details, verbatim from before the abort-reason work. + require.Equal(t, "Unable to find group id in 1foo", + fmt.Sprintf(abortDetailMissingGroupIDFmt, "1foo")) + require.Equal(t, "unable to parse group id from xfoo", + fmt.Sprintf(abortDetailBadGroupIDFmt, "xfoo")) + require.Equal(t, "Tablet for friend is nil", + fmt.Sprintf(abortDetailTabletNilFmt, "friend")) + require.Equal(t, "Commits on predicate friend are blocked due to predicate move", + fmt.Sprintf(abortDetailPredicateBlockedFmt, "friend")) + require.Equal(t, "Mutation done in group: 1. Predicate friend assigned to 2", + fmt.Sprintf(abortDetailGroupMismatchFmt, 1, "friend", 2)) +} + // TestAbortReasonUncategorized pins the withholding contract: when the server cannot substantiate // one of the published categories it emits the bare detail with no prefix, rather than borrowing a // category that would imply the wrong remedy. That is byte-identical to what a pre-feature server From b3d40ad9bcc0949f949763f11c29df2181250d3e Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Thu, 30 Jul 2026 18:54:32 -0400 Subject: [PATCH 5/6] feat(txn): name the key kinds a conflict abort cannot distinguish "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 --- dgraph/cmd/zero/oracle.go | 28 ++++++++++++++++++++++++++- dgraph/cmd/zero/oracle_reason_test.go | 17 ++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/dgraph/cmd/zero/oracle.go b/dgraph/cmd/zero/oracle.go index 70e67c8e887..0c5970e1e09 100644 --- a/dgraph/cmd/zero/oracle.go +++ b/dgraph/cmd/zero/oracle.go @@ -378,7 +378,33 @@ const ( // abortDetailTabletNilFmt, and ctx.Err() — the last supplied // by the Go runtime, so it is not declared here const ( - abortDetailConflict = "Transaction has been aborted. Please retry" + // A conflict is the one category the server cannot narrow. Every conflict key reaching Zero is a + // farm.Fingerprint64(key)^uid produced by GetConflictKey (posting/list.go), so by the time + // hasConflict matches one, which key produced it is already unrecoverable. Naming the + // possibilities is therefore the most this category can honestly say, and it is worth saying: a + // caller who set a single scalar value has no reason to expect that the index and count keys + // *derived* from it can conflict too. + // + // Exactly three kinds of key carry a conflict — data, index and count. Reverse keys carry none: + // IsReverse is a distinct ByteType, so a reverse key matches no case in GetConflictKey's switch, + // falls to default, and addConflictKey drops the resulting zero. Do not add "reverse" here. + // + // @upsert is named separately because it is a *rule*, not a fourth kind of key. HasUpsert is + // tested ahead of every IsData/IsIndex case, so on an @upsert predicate the existing data and + // index keys switch to getKey(key, 0) — the uid drops out and any two transactions writing the + // same value collide on the shared index key. That is the uniqueness mechanism, and in + // upsert-heavy ingest it is the most likely cause of this abort, which is why it is worth the + // extra sentence. + // + // The leading sentence is load-bearing and must stay verbatim at the front. Three tests in this + // repo substring-match it against the live server message — including the retry loop at + // dgraph/cmd/alpha/upsert_test.go, which would exit immediately and fail if it stopped matching + // — and it is also the text of dgo.ErrAborted, so user log-scrapers key off it. Extend this + // detail only by appending. + abortDetailConflict = "Transaction has been aborted. Please retry. Another transaction " + + "committed to one of the same keys. The conflicting key cannot be identified: it may be " + + "the data key written directly, or an index or count key derived from it. On an @upsert " + + "predicate the uid is excluded, so any two transactions writing the same value conflict" // A start timestamp goes stale for two different reasons, so this detail deliberately names // both rather than asserting one. Zero raises startTxnTs either when a Zero becomes leader and // renews its leases (updateLeases, assign.go) or when it trims its conflict map while applying diff --git a/dgraph/cmd/zero/oracle_reason_test.go b/dgraph/cmd/zero/oracle_reason_test.go index ad1814251ac..88352fcf8ae 100644 --- a/dgraph/cmd/zero/oracle_reason_test.go +++ b/dgraph/cmd/zero/oracle_reason_test.go @@ -71,6 +71,23 @@ func TestAbortVocabulary(t *testing.T) { }) } + // The conflict detail names what it cannot narrow — a conflict key is a one-way fingerprint by + // the time Zero matches it, so data, index, count and @upsert conflicts are indistinguishable. + // Callers who set one scalar value do not expect the keys derived from it to conflict, so the + // possibilities are spelled out. + for _, want := range []string{"index", "count", "@upsert"} { + require.Contains(t, abortDetailConflict, want, + "conflict detail should name the key kinds it cannot distinguish between") + } + + // Backwards-compatibility guard. This exact sentence must remain a prefix of the conflict + // detail: dgraph/cmd/alpha/upsert_test.go loops while the live server error contains it (so a + // change would exit the loop immediately and fail), query/query4_test.go and + // systest/unique_test.go assert Contains on it, and it is verbatim the text of dgo.ErrAborted, + // which user log-scrapers key off. Extend abortDetailConflict by appending only. + require.True(t, strings.HasPrefix(abortDetailConflict, "Transaction has been aborted. Please retry"), + "conflict detail must keep the legacy sentence as its prefix; got %q", abortDetailConflict) + // Wording of the checkPreds details, verbatim from before the abort-reason work. require.Equal(t, "Unable to find group id in 1foo", fmt.Sprintf(abortDetailMissingGroupIDFmt, "1foo")) From d1dddc75d216f935e79d3e6292799159bfa08bdc Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Thu, 30 Jul 2026 23:37:58 -0400 Subject: [PATCH 6/6] fix(txn): report aborts that happened before the commit was decided 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 --- dgraph/cmd/zero/oracle.go | 36 +++++++++++++++++++++++++-- dgraph/cmd/zero/oracle_reason_test.go | 13 ++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/dgraph/cmd/zero/oracle.go b/dgraph/cmd/zero/oracle.go index 0c5970e1e09..d169c0560a1 100644 --- a/dgraph/cmd/zero/oracle.go +++ b/dgraph/cmd/zero/oracle.go @@ -375,8 +375,9 @@ const ( // stale-startts abortDetailStaleStartTs // predicate-move abortDetailPredicateBlockedFmt, abortDetailGroupMismatchFmt // (withheld) abortDetailMissingGroupIDFmt, abortDetailBadGroupIDFmt, -// abortDetailTabletNilFmt, and ctx.Err() — the last supplied -// by the Go runtime, so it is not declared here +// abortDetailTabletNilFmt, abortDetailPreAborted, and +// ctx.Err() — the last supplied by the Go runtime, so it is +// not declared here const ( // A conflict is the one category the server cannot narrow. Every conflict key reaching Zero is a // farm.Fingerprint64(key)^uid produced by GetConflictKey (posting/list.go), so by the time @@ -415,6 +416,26 @@ const ( "Zero can still validate (Zero leader change, or its conflict map was trimmed at a " + "snapshot). Please retry" + // Reported when proposeTxn comes back with the transaction already aborted while this commit + // decided nothing itself — i.e. something aborted it out of band, before the commit was + // arbitrated. Zero records no cause when that happens (TryAbort carries only timestamps), so the + // category is withheld and the detail names the possibilities instead of guessing. All three + // reach Zero through the same TryAbort RPC, and none of them is a predicate *move*, so + // predicate-move would be the wrong label: + // + // - a schema or type update on a predicate the transaction touched, which cancels pending + // transactions so the index can be rebuilt (detectPendingTxns, worker/draft.go) + // - a drop-predicate (S * * delete) on a predicate the transaction touched (same function) + // - the Alpha leader ageing out transactions idle longer than --limit "txn-abort-after" + // (abortOldTransactions, worker/draft.go) + // + // Before this was handled, commit() returned nil here and the abort reached the client as a + // bare dgo.ErrAborted with no reason at all. + abortDetailPreAborted = "Transaction has been aborted. Please retry. It was already aborted " + + "before this commit was decided, which happens when a schema update or a drop-predicate " + + "cancels pending transactions on a predicate it touched, or when the server ages out " + + "transactions idle for longer than --limit \"txn-abort-after\"" + // Details produced by checkPreds. Wording is unchanged from before the abort-reason work; only // the declaration moved here, so existing log-scrapers and docs still match. abortDetailMissingGroupIDFmt = "Unable to find group id in %s" @@ -594,6 +615,17 @@ func (s *Server) commit(ctx context.Context, src *api.TxnContext) error { if aborted { return status.Error(codes.Aborted, abortReason(lateReason, lateDetail)) } + // proposeTxn sets src.Aborted when it finds no commit timestamp for this transaction, meaning + // something aborted it out of band while this commit was in flight — a TryAbort from a schema + // update, a drop-predicate, or the idle-transaction reaper. Nothing above set `aborted`, because + // this commit itself decided to proceed. Returning nil here (as this did previously) loses the + // abort entirely: CommitOverNetwork then sees tctx.Aborted with no error and falls through to a + // bare dgo.ErrAborted, so the caller learns nothing. Report it instead. + if src.Aborted { + span.SetAttributes(attribute.Bool("abort", true)) + return status.Error(codes.Aborted, + abortReason(abortReasonUncategorized, abortDetailPreAborted)) + } return nil } diff --git a/dgraph/cmd/zero/oracle_reason_test.go b/dgraph/cmd/zero/oracle_reason_test.go index 88352fcf8ae..1ad057b8774 100644 --- a/dgraph/cmd/zero/oracle_reason_test.go +++ b/dgraph/cmd/zero/oracle_reason_test.go @@ -60,6 +60,8 @@ func TestAbortVocabulary(t *testing.T) { fmt.Sprintf(abortDetailMissingGroupIDFmt, "1foo")}, {"malformed key, bad group id", abortReasonUncategorized, fmt.Sprintf(abortDetailBadGroupIDFmt, "xfoo")}, + {"aborted out of band before commit", abortReasonUncategorized, + abortDetailPreAborted}, } { t.Run(tc.name, func(t *testing.T) { wire := abortReason(tc.category, tc.rendered) @@ -80,6 +82,17 @@ func TestAbortVocabulary(t *testing.T) { "conflict detail should name the key kinds it cannot distinguish between") } + // The out-of-band abort names its three causes rather than guessing a category. None of them is + // a predicate move — they all arrive via TryAbort, which records no cause — so labelling this + // predicate-move would be wrong. Before it was handled, commit() returned nil and the abort + // reached the client as a bare dgo.ErrAborted with no reason at all. + for _, want := range []string{"schema update", "drop-predicate", "txn-abort-after"} { + require.Contains(t, abortDetailPreAborted, want, + "pre-abort detail should name the causes it cannot distinguish between") + } + require.True(t, strings.HasPrefix(abortDetailPreAborted, "Transaction has been aborted. Please retry"), + "pre-abort detail must keep the legacy sentence as its prefix; got %q", abortDetailPreAborted) + // Backwards-compatibility guard. This exact sentence must remain a prefix of the conflict // detail: dgraph/cmd/alpha/upsert_test.go loops while the live server error contains it (so a // change would exit the loop immediately and fail), query/query4_test.go and