From 2b0059d7406ea9b7ff7d90d024afdccb822c558c Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Mon, 15 Jun 2026 23:22:33 -0400 Subject: [PATCH 1/3] initial commit to begin surfacing txn issues --- .../java/io/dgraph/TxnConflictException.java | 46 ++++++ .../java/io/dgraph/AbortReasonLiveTest.java | 95 +++++++++++++ src/test/java/io/dgraph/AbortReasonTest.java | 133 ++++++++++++++++++ 3 files changed, 274 insertions(+) create mode 100644 src/test/java/io/dgraph/AbortReasonLiveTest.java create mode 100644 src/test/java/io/dgraph/AbortReasonTest.java diff --git a/src/main/java/io/dgraph/TxnConflictException.java b/src/main/java/io/dgraph/TxnConflictException.java index f6a4e66..b963ba6 100644 --- a/src/main/java/io/dgraph/TxnConflictException.java +++ b/src/main/java/io/dgraph/TxnConflictException.java @@ -15,6 +15,27 @@ public class TxnConflictException extends TxnException { private static final long serialVersionUID = 1L; + /** + * The category of a transaction abort, as reported by the Dgraph server. + * + * + */ + public enum AbortReason { + CONFLICT, + PREDICATE_MOVE, + STALE_STARTTS, + UNKNOWN + } + public TxnConflictException(String msg) { super(Status.ABORTED.withDescription(msg), null); } @@ -23,6 +44,31 @@ public TxnConflictException(String msg) { super(status, trailers); } + /** + * Returns the category of this abort. The server encodes the reason as a {@code ": "} + * prefix on the gRPC status description; this method parses that prefix. Against a server that does + * not report a reason (older versions), this returns {@link AbortReason#UNKNOWN}. The full + * human-readable description remains available via {@link #getMessage()}. + */ + public AbortReason getReason() { + String desc = getStatus().getDescription(); + if (desc == null) { + return AbortReason.UNKNOWN; + } + int colon = desc.indexOf(':'); + String code = (colon >= 0 ? desc.substring(0, colon) : desc).trim().toLowerCase(); + switch (code) { + case "conflict": + return AbortReason.CONFLICT; + case "predicate-move": + return AbortReason.PREDICATE_MOVE; + case "stale-startts": + return AbortReason.STALE_STARTTS; + default: + return AbortReason.UNKNOWN; + } + } + @Override public boolean isRetryable() { return true; diff --git a/src/test/java/io/dgraph/AbortReasonLiveTest.java b/src/test/java/io/dgraph/AbortReasonLiveTest.java new file mode 100644 index 0000000..65e86aa --- /dev/null +++ b/src/test/java/io/dgraph/AbortReasonLiveTest.java @@ -0,0 +1,95 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.dgraph; + +import static org.testng.Assert.*; + +import com.google.protobuf.ByteString; +import io.dgraph.DgraphProto.Mutation; +import io.dgraph.DgraphProto.Operation; +import io.dgraph.DgraphProto.Response; +import io.dgraph.TxnConflictException.AbortReason; +import io.grpc.ManagedChannel; +import io.grpc.ManagedChannelBuilder; +import java.util.concurrent.TimeUnit; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +/** + * Live cross-language end-to-end proof for the transaction-abort reason. Unlike {@link + * AbortReasonTest}, which feeds synthetic gRPC statuses into the parser, this test drives a real + * (locally patched) Dgraph server: it forces a genuine write-write conflict between two + * transactions and asserts that the abort propagates all the way to {@link + * TxnConflictException#getReason()} as {@link AbortReason#CONFLICT}. This closes the loop the unit + * tests cannot — proving the server actually emits the categorized reason on the wire and the + * client parses it. + * + *

Run against a non-ACL alpha listening on localhost:9180 (e.g. {@code dgraph alpha -o 100}). + * It is intentionally standalone (does not extend {@link DgraphIntegrationTest}) so it needs only a + * single alpha and no ACL login. Excluded from the default unit run unless selected explicitly via + * {@code --tests io.dgraph.AbortReasonLiveTest}. + */ +public class AbortReasonLiveTest { + private static final String HOST = "localhost"; + private static final int PORT = 9180; + + private static ManagedChannel channel; + private static DgraphClient client; + + @BeforeClass + public static void before() { + channel = ManagedChannelBuilder.forAddress(HOST, PORT).usePlaintext().build(); + client = new DgraphClient(DgraphGrpc.newStub(channel)); + client.alter(Operation.newBuilder().setDropAll(true).build()); + } + + @AfterClass + public static void after() throws InterruptedException { + if (channel != null) { + channel.shutdown().awaitTermination(5, TimeUnit.SECONDS); + } + } + + @Test + public void liveConflictReportsConflictReason() { + // txn1 creates a node with a name. + Transaction txn1 = client.newTransaction(); + Mutation mu1 = + Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"Manish\"}")).build(); + Response assigned = txn1.mutate(mu1); + assertEquals(assigned.getUidsMap().size(), 1, "expected exactly one assigned uid"); + String uid = assigned.getUidsMap().values().iterator().next(); + + // txn2 writes the same predicate on the same uid -> conflicts. + Transaction txn2 = client.newTransaction(); + Mutation mu2 = + Mutation.newBuilder() + .setSetJson(ByteString.copyFromUtf8("{\"uid\": \"" + uid + "\", \"name\": \"Manish\"}")) + .build(); + txn2.mutate(mu2); + + // First commit wins; its commitTs is now greater than txn2's startTs. + txn1.commit(); + + // Second commit must abort. The reason must reach the client end-to-end: a + // TxnConflictException whose parsed reason is CONFLICT, still retryable, with the + // full "conflict: ..." server message preserved. + try { + txn2.commit(); + fail("expected the conflicting second commit to throw TxnConflictException"); + } catch (TxnConflictException e) { + assertEquals( + e.getReason(), + AbortReason.CONFLICT, + "server-reported reason should parse to CONFLICT; full message: " + e.getMessage()); + assertTrue(e.isRetryable(), "conflict aborts are retryable"); + assertTrue( + e.getMessage().contains("conflict:"), + "full categorized server message should be preserved; got: " + e.getMessage()); + } + } +} diff --git a/src/test/java/io/dgraph/AbortReasonTest.java b/src/test/java/io/dgraph/AbortReasonTest.java new file mode 100644 index 0000000..8159d8d --- /dev/null +++ b/src/test/java/io/dgraph/AbortReasonTest.java @@ -0,0 +1,133 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.dgraph; + +import static org.testng.Assert.*; + +import io.dgraph.TxnConflictException.AbortReason; +import io.grpc.Status; +import io.grpc.StatusRuntimeException; +import org.testng.annotations.Test; + +/** + * Unit tests for surfacing the transaction-abort reason to the client. The Dgraph server encodes the + * abort category as a {@code ": "} prefix on the gRPC ABORTED status; these tests + * verify that {@link TxnConflictException#getReason()} parses it, that the mapping from a raw gRPC + * error goes through {@link Exceptions#translate}, and that behavior degrades gracefully against a + * server that reports no reason. + */ +public class AbortReasonTest { + + private StatusRuntimeException aborted(String description) { + return Status.Code.ABORTED.toStatus().withDescription(description).asRuntimeException(); + } + + private TxnConflictException conflictExceptionFor(String description) { + DgraphException ex = Exceptions.translate(aborted(description)); + assertTrue( + ex instanceof TxnConflictException, + "ABORTED status should map to TxnConflictException, got " + ex.getClass()); + return (TxnConflictException) ex; + } + + // --- Reason categorization (the three server-reported categories) --- + + @Test + public void testConflictReason() { + TxnConflictException ex = + conflictExceptionFor("conflict: Transaction has been aborted. Please retry"); + assertEquals(ex.getReason(), AbortReason.CONFLICT); + assertTrue(ex.isRetryable()); + } + + @Test + public void testPredicateMoveReason() { + TxnConflictException ex = + conflictExceptionFor( + "predicate-move: Commits on predicate name are blocked due to predicate move"); + assertEquals(ex.getReason(), AbortReason.PREDICATE_MOVE); + assertTrue(ex.isRetryable()); + } + + @Test + public void testStaleStartTsReason() { + TxnConflictException ex = + conflictExceptionFor( + "stale-startts: Transaction has been aborted due to a leader change. Please retry"); + assertEquals(ex.getReason(), AbortReason.STALE_STARTTS); + assertTrue(ex.isRetryable()); + } + + // --- Full message preserved alongside the parsed reason (backward compatibility) --- + + @Test + public void testFullMessageIsPreserved() { + String desc = "conflict: Transaction has been aborted. Please retry"; + TxnConflictException ex = conflictExceptionFor(desc); + // getMessage() still exposes the complete human-readable description. + assertTrue(ex.getMessage().contains(desc)); + assertEquals(ex.getStatus().getDescription(), desc); + } + + // --- Graceful degradation against an older server (no reason prefix) --- + + @Test + public void testLegacyMessageDegradesToUnknown() { + // Pre-feature servers emit the bare static string with no category prefix. + TxnConflictException ex = conflictExceptionFor("Transaction has been aborted. Please retry"); + assertEquals(ex.getReason(), AbortReason.UNKNOWN); + assertTrue(ex.isRetryable()); + } + + @Test + public void testUnrecognizedPrefixDegradesToUnknown() { + TxnConflictException ex = conflictExceptionFor("something-else: not a known category"); + assertEquals(ex.getReason(), AbortReason.UNKNOWN); + } + + @Test + public void testNullDescriptionIsUnknown() { + DgraphException ex = Exceptions.translate(Status.ABORTED.asRuntimeException()); + assertTrue(ex instanceof TxnConflictException); + assertEquals(((TxnConflictException) ex).getReason(), AbortReason.UNKNOWN); + } + + // --- Parsing robustness --- + + @Test + public void testReasonIsCaseInsensitiveAndTrimmed() { + assertEquals(conflictExceptionFor("CONFLICT: x").getReason(), AbortReason.CONFLICT); + assertEquals(conflictExceptionFor(" predicate-move : y").getReason(), AbortReason.PREDICATE_MOVE); + } + + @Test + public void testReasonWithoutDetailStillParses() { + // A bare code with no ": detail" suffix should still categorize. + assertEquals(conflictExceptionFor("conflict").getReason(), AbortReason.CONFLICT); + } + + // --- The constructor used elsewhere in the client still works --- + + @Test + public void testStringConstructorReason() { + TxnConflictException ex = new TxnConflictException("conflict: manual"); + assertEquals(ex.getReason(), AbortReason.CONFLICT); + assertTrue(ex.isRetryable()); + } + + @Test + public void testFailedPreconditionAlsoCarriesReason() { + // FAILED_PRECONDITION also maps to TxnConflictException; reason parsing applies there too. + DgraphException ex = + Exceptions.translate( + Status.Code.FAILED_PRECONDITION + .toStatus() + .withDescription("conflict: Transaction conflict") + .asRuntimeException()); + assertTrue(ex instanceof TxnConflictException); + assertEquals(((TxnConflictException) ex).getReason(), AbortReason.CONFLICT); + } +} From 89e6ca860f0a29108f198d449cc82441253e8927 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Wed, 17 Jun 2026 01:47:13 -0400 Subject: [PATCH 2/3] surfacing transaction errors to clients --- docker-compose.abort-reason.yml | 41 ++++ .../java/io/dgraph/AbortReasonLiveTest.java | 203 ++++++++++++++++-- 2 files changed, 231 insertions(+), 13 deletions(-) create mode 100644 docker-compose.abort-reason.yml diff --git a/docker-compose.abort-reason.yml b/docker-compose.abort-reason.yml new file mode 100644 index 0000000..010437a --- /dev/null +++ b/docker-compose.abort-reason.yml @@ -0,0 +1,41 @@ +# Multi-group, no-ACL cluster for the live transaction-abort-reason tests +# (io.dgraph.AbortReasonLiveTest). Two alpha groups (replicas=1) enable the +# predicate-move case; the single zero is restartable for the stale-startts case. +# +# Usage: +# make local-image # build dgraph/dgraph:local from this branch +# docker compose -f docker-compose.abort-reason.yml up -d +# TEST_GRPC_PORT=9180 \ +# TEST_ZERO_HTTP=localhost:6180 \ +# TEST_ZERO_RESTART_CMD="docker compose -f docker-compose.abort-reason.yml restart zero1" \ +# ./gradlew test --tests io.dgraph.AbortReasonLiveTest +# docker compose -f docker-compose.abort-reason.yml down +version: "3.5" +services: + zero1: + image: dgraph/dgraph:local + container_name: ar_zero1 + ports: + - 5180:5180 + - 6180:6180 + command: dgraph zero -o 100 --my=zero1:5180 --replicas=1 --logtostderr -v=2 --bindall + + alpha1: + image: dgraph/dgraph:local + container_name: ar_alpha1 + ports: + - 8180:8180 + - 9180:9180 + command: + dgraph alpha -o 100 --my=alpha1:7180 --zero=zero1:5180 --logtostderr -v=2 --raft "idx=1; + group=1" --security "whitelist=0.0.0.0/0;" + + alpha2: + image: dgraph/dgraph:local + container_name: ar_alpha2 + ports: + - 8182:8182 + - 9182:9182 + command: + dgraph alpha -o 102 --my=alpha2:7182 --zero=zero1:5180 --logtostderr -v=2 --raft "idx=2; + group=2" --security "whitelist=0.0.0.0/0;" diff --git a/src/test/java/io/dgraph/AbortReasonLiveTest.java b/src/test/java/io/dgraph/AbortReasonLiveTest.java index 65e86aa..2a85334 100644 --- a/src/test/java/io/dgraph/AbortReasonLiveTest.java +++ b/src/test/java/io/dgraph/AbortReasonLiveTest.java @@ -7,6 +7,7 @@ import static org.testng.Assert.*; +import com.google.gson.Gson; import com.google.protobuf.ByteString; import io.dgraph.DgraphProto.Mutation; import io.dgraph.DgraphProto.Operation; @@ -14,7 +15,13 @@ import io.dgraph.TxnConflictException.AbortReason; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; +import java.io.BufferedReader; +import java.io.InputStreamReader; +import java.net.HttpURLConnection; +import java.net.URL; +import java.util.Map; import java.util.concurrent.TimeUnit; +import org.testng.SkipException; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; @@ -22,24 +29,43 @@ /** * Live cross-language end-to-end proof for the transaction-abort reason. Unlike {@link * AbortReasonTest}, which feeds synthetic gRPC statuses into the parser, this test drives a real - * (locally patched) Dgraph server: it forces a genuine write-write conflict between two - * transactions and asserts that the abort propagates all the way to {@link - * TxnConflictException#getReason()} as {@link AbortReason#CONFLICT}. This closes the loop the unit - * tests cannot — proving the server actually emits the categorized reason on the wire and the - * client parses it. + * (locally patched) Dgraph server and asserts that each abort category propagates all the way to + * {@link TxnConflictException#getReason()}. This closes the loop the unit tests cannot — proving the + * server actually emits the categorized reason on the wire and the client parses it. * - *

Run against a non-ACL alpha listening on localhost:9180 (e.g. {@code dgraph alpha -o 100}). - * It is intentionally standalone (does not extend {@link DgraphIntegrationTest}) so it needs only a - * single alpha and no ACL login. Excluded from the default unit run unless selected explicitly via - * {@code --tests io.dgraph.AbortReasonLiveTest}. + *

Configuration (system properties or environment variables): + * + *

    + *
  • {@code dgraph.test.host} / {@code TEST_HOSTNAME} — alpha host (default {@code localhost}) + *
  • {@code dgraph.test.port} / {@code TEST_GRPC_PORT} — alpha gRPC port (default {@code 9180}) + *
  • {@code dgraph.test.zeroHttp} / {@code TEST_ZERO_HTTP} — zero HTTP admin (e.g. {@code + * localhost:6180}); enables the predicate-move test (needs a multi-group cluster) + *
  • {@code dgraph.test.zeroRestartCmd} / {@code TEST_ZERO_RESTART_CMD} — shell command that + * restarts Zero; enables the stale-startts test + *
+ * + * Tests whose infrastructure is not configured are skipped (not failed), so the file is safe in the + * default run. Run explicitly with {@code --tests io.dgraph.AbortReasonLiveTest}. */ public class AbortReasonLiveTest { - private static final String HOST = "localhost"; - private static final int PORT = 9180; + private static final String HOST = conf("dgraph.test.host", "TEST_HOSTNAME", "localhost"); + private static final int PORT = + Integer.parseInt(conf("dgraph.test.port", "TEST_GRPC_PORT", "9180")); + private static final String ZERO_HTTP = conf("dgraph.test.zeroHttp", "TEST_ZERO_HTTP", null); + private static final String ZERO_RESTART_CMD = + conf("dgraph.test.zeroRestartCmd", "TEST_ZERO_RESTART_CMD", null); private static ManagedChannel channel; private static DgraphClient client; + private static String conf(String prop, String env, String dflt) { + String v = System.getProperty(prop); + if (v == null || v.isEmpty()) { + v = System.getenv(env); + } + return (v == null || v.isEmpty()) ? dflt : v; + } + @BeforeClass public static void before() { channel = ManagedChannelBuilder.forAddress(HOST, PORT).usePlaintext().build(); @@ -75,8 +101,7 @@ public void liveConflictReportsConflictReason() { // First commit wins; its commitTs is now greater than txn2's startTs. txn1.commit(); - // Second commit must abort. The reason must reach the client end-to-end: a - // TxnConflictException whose parsed reason is CONFLICT, still retryable, with the + // Second commit must abort with the CONFLICT category, still retryable, with the // full "conflict: ..." server message preserved. try { txn2.commit(); @@ -92,4 +117,156 @@ public void liveConflictReportsConflictReason() { "full categorized server message should be preserved; got: " + e.getMessage()); } } + + /** + * A transaction's start ts becomes "stale" when it predates the current Zero leader's lease — i.e. + * after a leader change. We force that by opening a transaction and then restarting Zero (via the + * configured command): on restart Zero renews its lease and advances startTxnTs past every + * previously-leased start ts, so committing the now-old txn aborts with STALE_STARTTS. + */ + @Test + public void liveStaleStartTsReportsStaleReason() throws Exception { + if (ZERO_RESTART_CMD == null) { + throw new SkipException( + "set dgraph.test.zeroRestartCmd / TEST_ZERO_RESTART_CMD to restart Zero"); + } + + // Open a transaction so it gets a start ts that the restart will invalidate. + Transaction txn = client.newTransaction(); + txn.mutate( + Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"Manish\"}")).build()); + + // Restart Zero; sleeps give the leader time to re-establish (lease renewal, hence the + // startTxnTs bump, runs on becoming leader). + runShell(ZERO_RESTART_CMD); + Thread.sleep(8000); + + try { + txn.commit(); + fail("expected the stale commit to throw TxnConflictException"); + } catch (TxnConflictException e) { + assertEquals( + e.getReason(), + AbortReason.STALE_STARTTS, + "server-reported reason should parse to STALE_STARTTS; full message: " + e.getMessage()); + assertTrue( + e.getMessage().contains("stale-startts:"), + "full categorized server message should be preserved; got: " + e.getMessage()); + } + } + + /** + * Moving a predicate's tablet to another group rejects commits that mutated it on the old group. + * We mutate "name" while its tablet is on the source group, move the tablet, then commit: the + * commit's predicate keys reference the old group, so Zero's checkPreds rejects it with the + * PREDICATE_MOVE category. + */ + @Test + public void livePredicateMoveReportsPredicateMoveReason() throws Exception { + if (ZERO_HTTP == null) { + throw new SkipException( + "set dgraph.test.zeroHttp / TEST_ZERO_HTTP and run a multi-group cluster"); + } + + client.alter(Operation.newBuilder().setSchema("name: string @index(exact) .").build()); + + // Seed so the "name" tablet exists and settles on some group. + Transaction seed = client.newTransaction(); + seed.mutate( + Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"seed\"}")).build()); + seed.commit(); + Thread.sleep(1000); + + String src = groupOf("name"); + if (src == null || groupCount() < 2) { + throw new SkipException("need a multi-group cluster serving predicate 'name'"); + } + String dst = src.equals("1") ? "2" : "1"; + + // Mutate "name" while it is on `src` (the txn's predicate keys reference `src`), don't commit. + Transaction txn = client.newTransaction(); + txn.mutate( + Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"Manish\"}")).build()); + + // Move the tablet and wait for the move to complete. + httpGet("http://" + ZERO_HTTP + "/moveTablet?tablet=name&group=" + dst); + long deadline = System.currentTimeMillis() + 60_000; + while (System.currentTimeMillis() < deadline && !dst.equals(groupOf("name"))) { + Thread.sleep(1000); + } + assertEquals(groupOf("name"), dst, "tablet move did not complete"); + + try { + txn.commit(); + fail("expected the post-move commit to throw TxnConflictException"); + } catch (TxnConflictException e) { + assertEquals( + e.getReason(), + AbortReason.PREDICATE_MOVE, + "server-reported reason should parse to PREDICATE_MOVE; full message: " + e.getMessage()); + assertTrue( + e.getMessage().contains("predicate-move:"), + "full categorized server message should be preserved; got: " + e.getMessage()); + } + } + + // --- helpers --- + + @SuppressWarnings("unchecked") + private static Map zeroState() throws Exception { + String body = httpGet("http://" + ZERO_HTTP + "/state"); + return new Gson().fromJson(body, Map.class); + } + + /** Returns the group id serving the given predicate (matching the namespaced tablet key). */ + @SuppressWarnings("unchecked") + private static String groupOf(String pred) throws Exception { + Map groups = (Map) zeroState().get("groups"); + if (groups == null) { + return null; + } + for (Map.Entry e : groups.entrySet()) { + Object tabletsObj = ((Map) e.getValue()).get("tablets"); + if (tabletsObj instanceof Map) { + for (String tablet : ((Map) tabletsObj).keySet()) { + if (tablet.equals(pred) || tablet.endsWith("-" + pred)) { + return e.getKey(); + } + } + } + } + return null; + } + + @SuppressWarnings("unchecked") + private static int groupCount() throws Exception { + Map groups = (Map) zeroState().get("groups"); + return groups == null ? 0 : groups.size(); + } + + private static String httpGet(String urlStr) throws Exception { + HttpURLConnection conn = (HttpURLConnection) new URL(urlStr).openConnection(); + conn.setRequestMethod("GET"); + try (BufferedReader rd = new BufferedReader(new InputStreamReader(conn.getInputStream()))) { + StringBuilder sb = new StringBuilder(); + String line; + while ((line = rd.readLine()) != null) { + sb.append(line); + } + return sb.toString(); + } finally { + conn.disconnect(); + } + } + + private static void runShell(String cmd) throws Exception { + Process p = new ProcessBuilder("bash", "-c", cmd).inheritIO().start(); + if (!p.waitFor(60, TimeUnit.SECONDS)) { + p.destroyForcibly(); + throw new RuntimeException("zero restart command timed out: " + cmd); + } + if (p.exitValue() != 0) { + throw new RuntimeException("zero restart command failed (" + p.exitValue() + "): " + cmd); + } + } } From 0ac6f973800839cc767c212b330ca22809a0fc42 Mon Sep 17 00:00:00 2001 From: Ryan Hendrickson Date: Fri, 31 Jul 2026 01:29:15 -0400 Subject: [PATCH 3/3] test: track the server's abort messages after the dgraph-side changes No behaviour change is needed - no category was added or renamed, and the prefix parser already handles every message the server now sends. But three fixtures quoted text no server emits anymore, and the STALE_STARTTS javadoc described one of its two causes. Fixtures now hold the verbatim server strings, declared as constants so the wire format is recorded in one place: - the conflict detail gained an explanation of which key kinds could have collided, and now contains a colon of its own after the category prefix - stale-startts no longer claims "a leader change"; Zero also raises its startTxnTs floor when trimming the conflict map at a snapshot, which is not a leader change, so the message names both and so does the javadoc - new: aborts the server declines to categorize, sending the detail with no prefix rather than implying a wrong remedy Adds coverage for two ways that could go wrong. Both the conflict and the completed-move messages carry embedded colons, so only the first may delimit the category. And the out-of-band abort opens with the exact sentence a pre-feature server sent for every abort - a false CONFLICT there would tell a caller to retry something that cannot succeed. AbortReasonTest 15/15 pass on JDK 25 via standalone Gradle 9.1.0. The other suites fail in beforeClass ("Unable to perform the DropAll operation") for want of a running cluster, unrelated to this change. Co-Authored-By: Claude Opus 5 --- .../java/io/dgraph/TxnConflictException.java | 27 ++++- .../java/io/dgraph/AbortReasonLiveTest.java | 13 +- src/test/java/io/dgraph/AbortReasonTest.java | 114 ++++++++++++++++-- 3 files changed, 131 insertions(+), 23 deletions(-) diff --git a/src/main/java/io/dgraph/TxnConflictException.java b/src/main/java/io/dgraph/TxnConflictException.java index b963ba6..62d1593 100644 --- a/src/main/java/io/dgraph/TxnConflictException.java +++ b/src/main/java/io/dgraph/TxnConflictException.java @@ -20,14 +20,29 @@ public class TxnConflictException extends TxnException { * *
    *
  • {@link #CONFLICT} — a write-write conflict with another concurrent transaction; retrying - * with a fresh transaction is the expected response. + * with a fresh transaction is the expected response. The server cannot say which + * key collided: conflict keys are one-way fingerprints by the time they are compared, so + * the culprit may be the data key written directly, or an index or count key derived from + * it. On an {@code @upsert} predicate the uid is excluded from the comparison, so any two + * transactions writing the same value conflict. *
  • {@link #PREDICATE_MOVE} — a predicate is being moved between groups and commits on it are - * temporarily blocked; back off and retry once the move completes. - *
  • {@link #STALE_STARTTS} — the transaction's start timestamp predates the current Zero - * leader (a leader change); retry with a fresh transaction. - *
  • {@link #UNKNOWN} — no reason was reported. Returned for aborts from older servers that do - * not yet categorize the reason, so callers degrade gracefully. + * temporarily blocked, or it finished moving while the transaction was open; back off and + * retry once the move completes. + *
  • {@link #STALE_STARTTS} — the transaction's start timestamp is older than the oldest + * timestamp the server can still validate against. That happens on a Zero leader change, + * and also when Zero trims its conflict map at a snapshot, which is not a leader change at + * all. Retry with a fresh transaction. + *
  • {@link #UNKNOWN} — no category was reported. This covers aborts from older servers that + * do not categorize at all, and aborts a current server declines to categorize because no + * published category fits — for example a transaction already aborted out of band by a + * schema change or the idle-transaction reaper, a cancelled context, or a predicate no + * group currently serves. The description still explains what happened; only the machine + * readable category is absent, so callers degrade gracefully. *
+ * + *

Categories are matched on the description prefix, and an unrecognized prefix degrades to + * {@link #UNKNOWN}. A newer server may therefore introduce categories this enum does not name + * without breaking this client. */ public enum AbortReason { CONFLICT, diff --git a/src/test/java/io/dgraph/AbortReasonLiveTest.java b/src/test/java/io/dgraph/AbortReasonLiveTest.java index 2a85334..98d1cb3 100644 --- a/src/test/java/io/dgraph/AbortReasonLiveTest.java +++ b/src/test/java/io/dgraph/AbortReasonLiveTest.java @@ -119,10 +119,15 @@ public void liveConflictReportsConflictReason() { } /** - * A transaction's start ts becomes "stale" when it predates the current Zero leader's lease — i.e. - * after a leader change. We force that by opening a transaction and then restarting Zero (via the - * configured command): on restart Zero renews its lease and advances startTxnTs past every - * previously-leased start ts, so committing the now-old txn aborts with STALE_STARTTS. + * A transaction's start timestamp becomes "stale" when it falls below the oldest timestamp Zero + * can still validate against (its startTxnTs floor). Zero raises that floor on a leader change, + * and also when it trims its conflict map at a snapshot — the second is not a leader change at + * all, which is why the server message names both causes. + * + *

A leader change is simply the one that can be forced deterministically: open a transaction, + * then restart Zero (via the configured command). On restart Zero renews its lease and advances + * startTxnTs past every previously-leased start timestamp, so committing the now-old transaction + * aborts with STALE_STARTTS. */ @Test public void liveStaleStartTsReportsStaleReason() throws Exception { diff --git a/src/test/java/io/dgraph/AbortReasonTest.java b/src/test/java/io/dgraph/AbortReasonTest.java index 8159d8d..876f481 100644 --- a/src/test/java/io/dgraph/AbortReasonTest.java +++ b/src/test/java/io/dgraph/AbortReasonTest.java @@ -33,43 +33,92 @@ private TxnConflictException conflictExceptionFor(String description) { return (TxnConflictException) ex; } + // Verbatim descriptions the server sends, from the abort-detail constants in + // dgraph/cmd/zero/oracle.go. Kept literal rather than abbreviated so these fixtures stay a + // faithful record of the wire format: several now contain colons of their own after the category + // prefix, which is exactly the case the prefix parser has to get right. + private static final String SERVER_CONFLICT = + "conflict: 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"; + private static final String SERVER_STALE_STARTTS = + "stale-startts: 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"; + private static final String SERVER_MOVE_IN_FLIGHT = + "predicate-move: Commits on predicate name are blocked due to predicate move"; + private static final String SERVER_MOVE_COMPLETED = + "predicate-move: Mutation done in group: 1. Predicate name assigned to 2"; + + // Causes the server deliberately leaves uncategorized: it says what happened, but no published + // category fits, so it sends the detail with no prefix rather than implying a wrong remedy. + private static final String SERVER_PRE_ABORTED = + "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\""; + private static final String SERVER_TABLET_NIL = "Tablet for name is nil"; + private static final String SERVER_MALFORMED_KEY = "Unable to find group id in 1name"; + private static final String SERVER_BAD_GROUP_ID = + "unable to parse group id from xname: strconv.Atoi: parsing \"x\": invalid syntax"; + private static final String SERVER_CTX_CANCELLED = "context canceled"; + // --- Reason categorization (the three server-reported categories) --- @Test public void testConflictReason() { - TxnConflictException ex = - conflictExceptionFor("conflict: Transaction has been aborted. Please retry"); + TxnConflictException ex = conflictExceptionFor(SERVER_CONFLICT); assertEquals(ex.getReason(), AbortReason.CONFLICT); assertTrue(ex.isRetryable()); } @Test public void testPredicateMoveReason() { - TxnConflictException ex = - conflictExceptionFor( - "predicate-move: Commits on predicate name are blocked due to predicate move"); + TxnConflictException ex = conflictExceptionFor(SERVER_MOVE_IN_FLIGHT); assertEquals(ex.getReason(), AbortReason.PREDICATE_MOVE); assertTrue(ex.isRetryable()); } + /** + * The completed-move message contains a colon of its own ("group: 1"). Only the first colon + * delimits the category, so this must still parse as PREDICATE_MOVE rather than being confused by + * the second one. + */ + @Test + public void testPredicateMoveCompletedReasonWithEmbeddedColon() { + TxnConflictException ex = conflictExceptionFor(SERVER_MOVE_COMPLETED); + assertEquals(ex.getReason(), AbortReason.PREDICATE_MOVE); + } + @Test public void testStaleStartTsReason() { - TxnConflictException ex = - conflictExceptionFor( - "stale-startts: Transaction has been aborted due to a leader change. Please retry"); + TxnConflictException ex = conflictExceptionFor(SERVER_STALE_STARTTS); assertEquals(ex.getReason(), AbortReason.STALE_STARTTS); assertTrue(ex.isRetryable()); } + /** + * The conflict detail also contains its own colon ("cannot be identified: it may be"). Same + * requirement as the completed-move case: the category comes from the first colon only. + */ + @Test + public void testConflictReasonWithEmbeddedColon() { + assertTrue( + SERVER_CONFLICT.indexOf(':') != SERVER_CONFLICT.lastIndexOf(':'), + "fixture should contain more than one colon, otherwise this test proves nothing"); + assertEquals(conflictExceptionFor(SERVER_CONFLICT).getReason(), AbortReason.CONFLICT); + } + // --- Full message preserved alongside the parsed reason (backward compatibility) --- @Test public void testFullMessageIsPreserved() { - String desc = "conflict: Transaction has been aborted. Please retry"; - TxnConflictException ex = conflictExceptionFor(desc); - // getMessage() still exposes the complete human-readable description. - assertTrue(ex.getMessage().contains(desc)); - assertEquals(ex.getStatus().getDescription(), desc); + TxnConflictException ex = conflictExceptionFor(SERVER_CONFLICT); + // getMessage() still exposes the complete human-readable description, including the detail + // explaining which kinds of key could have collided. + assertTrue(ex.getMessage().contains(SERVER_CONFLICT)); + assertEquals(ex.getStatus().getDescription(), SERVER_CONFLICT); } // --- Graceful degradation against an older server (no reason prefix) --- @@ -88,6 +137,45 @@ public void testUnrecognizedPrefixDegradesToUnknown() { assertEquals(ex.getReason(), AbortReason.UNKNOWN); } + /** + * A current server also sends aborts with no category, for causes no published category fits. It + * still explains what happened in the description — only the machine-readable code is absent — + * and the client must report UNKNOWN rather than misreading the detail as a category. These are + * the real messages, not invented ones. + */ + @Test + public void testUncategorizedServerCausesDegradeToUnknown() { + String[] uncategorized = { + SERVER_PRE_ABORTED, SERVER_TABLET_NIL, SERVER_MALFORMED_KEY, + SERVER_BAD_GROUP_ID, SERVER_CTX_CANCELLED, + }; + for (String desc : uncategorized) { + TxnConflictException ex = conflictExceptionFor(desc); + assertEquals( + ex.getReason(), + AbortReason.UNKNOWN, + "uncategorized server message should not parse as a category: " + desc); + assertEquals( + ex.getStatus().getDescription(), desc, "the explanation must survive intact: " + desc); + } + } + + /** + * The out-of-band abort opens with the same sentence a pre-feature server sent for every abort, + * and the malformed-group-id message carries a colon of its own. Neither may be mistaken for a + * category — a false CONFLICT here would tell a caller to retry something that cannot succeed. + */ + @Test + public void testUncategorizedLookalikesAreNotMisparsed() { + assertTrue( + SERVER_PRE_ABORTED.startsWith("Transaction has been aborted. Please retry"), + "fixture should start with the legacy sentence, otherwise this test proves nothing"); + assertEquals(conflictExceptionFor(SERVER_PRE_ABORTED).getReason(), AbortReason.UNKNOWN); + + assertTrue(SERVER_BAD_GROUP_ID.contains(":"), "fixture should contain a colon"); + assertEquals(conflictExceptionFor(SERVER_BAD_GROUP_ID).getReason(), AbortReason.UNKNOWN); + } + @Test public void testNullDescriptionIsUnknown() { DgraphException ex = Exceptions.translate(Status.ABORTED.asRuntimeException());