Production incident

The report that reordered itself

45 minintermediate212 yrs

A real incident: symptom first, cause hidden, tradeoff at the end.

The incident

A settlement report that has been quietly wrong for months. The ticket that finally got it looked at says only "statement rows out of order for one customer", and the exception in the logs is unrelated to it. What the review turned up: 1. The statement comparator subtracts two longs and casts the result to int. It has never thrown. 2. A banding comparator treats amounts within one rupee as equal. This is the one that throws, and it is the only defect anyone knew about. 3. De-duplication runs through a TreeSet ordered by customer, and has been discarding three quarters of the rows. 4. The summary box sorts all hundred thousand orders to display ten. Four defects: two silent wrong answers, one silent data loss, one waste. The one that throws is the least damaging of the four. Fix all of them without weakening the checks.

What this teaches

  • Subtraction in a comparator overflows, and the failure is a wrong order rather than an exception
  • A tolerance is not a total order; band the value instead, so transitivity holds by construction
  • TreeSet membership is decided by the comparator, not by equals, so a partial comparator deletes rows
  • sorted().limit(k) is a full sort; a bounded heap is O(n log k)
  • The defect that throws is not automatically the worst one

Starter

Starter.java
import java.util.*;
import java.util.stream.*;

/**
 * Production: the settlement report that reordered itself.
 *
 * Four defects, all in ordering code, none of which throws in the test suite.
 * Run this. Four checks fail. Fix the report so all four pass, without
 * weakening the checks.
 */
public class Starter {

    record Order(long id, int amountMinor, String customer) {}

    /* ── the report under review ────────────────────────────────────────── */

    /** Statement lines, oldest first. Ids come from a 64-bit sequence. */
    static List<Order> byId(List<Order> orders, Comparator<Order> counted) {
        List<Order> copy = new ArrayList<>(orders);
        copy.sort(counted);
        return copy;
    }

    /** The comparator the statement uses. */
    static final Comparator<Order> BY_ID = (a, b) -> (int) (a.id() - b.id());

    /**
     * "Amounts within one rupee are the same for banding purposes."
     * Used to sort orders into display bands.
     */
    static final Comparator<Order> BY_BAND =
        (a, b) -> Math.abs(a.amountMinor() - b.amountMinor()) <= 100
                ? 0 : Integer.compare(a.amountMinor(), b.amountMinor());

    /** De-duplicates orders before the report renders them. */
    static List<Order> deduplicate(List<Order> orders) {
        Set<Order> unique = new TreeSet<>(Comparator.comparing(Order::customer));
        unique.addAll(orders);
        return new ArrayList<>(unique);
    }

    /** The ten largest orders, for the summary box at the top of the report. */
    static List<Order> topTen(List<Order> orders, Comparator<Order> counted) {
        return orders.stream().sorted(counted).limit(10).toList();
    }

    /* ── checks ─────────────────────────────────────────────────────────── */

    static <T> Comparator<T> counting(Comparator<T> inner, long[] calls) {
        return (a, b) -> { calls[0]++; return inner.compare(a, b); };
    }

    static List<Order> sample(int n, long seed) {
        Random r = new Random(seed);
        List<Order> out = new ArrayList<>(n);
        for (int i = 0; i < n; i++)
            out.add(new Order(r.nextLong(), r.nextInt(1_000_000), "cust-" + r.nextInt(50)));
        return out;
    }

    public static void main(String[] args) {
        List<String> failures = new ArrayList<>();

        // 1. Ordering must be correct for ids anywhere in the 64-bit range.
        List<Order> wide = List.of(
            new Order(Long.MAX_VALUE, 10, "a"),
            new Order(-5L, 20, "b"),
            new Order(4_294_967_296L, 30, "c"),      // 2^32 — differs from 0 by exactly 2^32
            new Order(0L, 40, "d"));
        List<Order> sorted = byId(wide, counting(BY_ID, new long[1]));
        for (int i = 1; i < sorted.size(); i++) {
            if (sorted.get(i - 1).id() > sorted.get(i).id()) {
                failures.add("1. statement is not in id order: "
                    + sorted.stream().map(o -> String.valueOf(o.id())).collect(Collectors.joining(", ")));
                break;
            }
        }

        // 2. Every comparator the report uses must be a real total order.
        //    Amounts here sit inside a narrow band on purpose — that is where
        //    a tolerance comparator stops being transitive.
        List<Order> triples = new ArrayList<>();
        Random rb = new Random(7);
        for (int i = 0; i < 40; i++) triples.add(new Order(i, rb.nextInt(500), "cust"));
        outer:
        for (Order a : triples) for (Order b : triples) {
            if (Integer.signum(BY_BAND.compare(a, b)) != -Integer.signum(BY_BAND.compare(b, a))) {
                failures.add("2. BY_BAND is not antisymmetric");
                break outer;
            }
            for (Order c : triples) {
                if (BY_BAND.compare(a, b) <= 0 && BY_BAND.compare(b, c) <= 0 && BY_BAND.compare(a, c) > 0) {
                    failures.add("2. BY_BAND is not transitive: it puts " + a.amountMinor()
                        + " at or before " + b.amountMinor() + ", and " + b.amountMinor()
                        + " at or before " + c.amountMinor() + ", but " + a.amountMinor()
                        + " after " + c.amountMinor());
                    break outer;
                }
            }
        }

        // 3. De-duplication must remove duplicates and nothing else.
        List<Order> withDupes = new ArrayList<>(sample(200, 11));
        withDupes.add(withDupes.get(0));                       // one genuine duplicate
        int expected = (int) withDupes.stream().distinct().count();
        int actual = deduplicate(withDupes).size();
        if (actual != expected)
            failures.add("3. deduplicate kept " + actual + " of " + expected + " distinct orders — "
                       + (expected - actual) + " rows silently dropped");

        // 4. Top ten of 100,000 must not cost a full sort.
        List<Order> many = sample(100_000, 13);
        long[] calls = new long[1];
        // Tiebroken on id so there is exactly one correct answer, whatever
        // algorithm produces it. A partial order would make this check flaky.
        Comparator<Order> largestFirst = Comparator.comparingInt(Order::amountMinor)
                                                   .reversed()
                                                   .thenComparingLong(Order::id);
        List<Order> top = topTen(many, counting(largestFirst, calls));
        List<Order> reference = many.stream().sorted(largestFirst).limit(10).toList();
        if (!top.equals(reference))
            failures.add("4. top ten is wrong");
        else if (calls[0] > 500_000)
            failures.add("4. top ten cost " + calls[0] + " comparisons for 10 rows out of "
                       + many.size() + " — budget is 500000");

        /* ── report ─────────────────────────────────────────────────────── */
        if (failures.isEmpty()) {
            System.out.println("PASS");
        } else {
            failures.forEach(f -> System.out.println("  " + f));
            System.out.println("FAIL");
        }
    }
}

Run it locally:

cd exercises/java/searching-and-sorting/arrays-sort/03-production
javac Starter.java -d /tmp/out && java -cp /tmp/out Starter

Hints

  1. Hint 1

    Start with the check that fails most quietly. Sort four ids spanning the long range and read the output — no exception, wrong answer.

  2. Hint 2

    For the banding comparator, write down three amounts where a is within tolerance of b, b of c, and a is not of c. Then ask what function of a single amount would give the same grouping without that problem.

  3. Hint 3

    The TreeSet is not deduplicating. Ask what it uses to decide two elements are the same, and whether that is the same question equals asks.

  4. Hint 4

    The top-ten check counts comparator calls. Work out what k-sized structure lets you reject most elements after exactly one comparison.

Done when

  • Statement ordering is correct for ids anywhere in the 64-bit range
  • The banding comparator passes a brute-force transitivity check
  • De-duplication removes only genuine duplicates
  • Top ten costs well under 500,000 comparisons and matches the full sort exactly
  • You can say which of the four would have been caught by an exception, and which three would not

Solution

Show the solution — try it yourself first
Solution.java
import java.util.*;
import java.util.stream.*;

/**
 * Solution: the settlement report that reordered itself.
 *
 * Four independent defects. Only the second one is the kind that ever throws,
 * and it is the one the team already knew about — the other three had been
 * producing wrong reports quietly.
 */
public class Solution {

    record Order(long id, int amountMinor, String customer) {}

    /* ── the report, fixed ──────────────────────────────────────────────── */

    static List<Order> byId(List<Order> orders, Comparator<Order> counted) {
        List<Order> copy = new ArrayList<>(orders);
        copy.sort(counted);
        return copy;
    }

    /*
     * Defect 1. The original was `(int) (a.id() - b.id())`, which is wrong
     * twice over: the subtraction of two longs can overflow, and the cast to
     * int throws away the top 32 bits of whatever survives — so ids differing
     * by exactly 2^32 compared equal. Neither failure throws.
     *
     * comparingLong never subtracts. It is also the shorter code.
     */
    static final Comparator<Order> BY_ID = Comparator.comparingLong(Order::id);

    /*
     * Defect 2. "Within one rupee counts as the same band" is not transitive:
     * 236 and 164 are within 100, 164 and 76 are within 100, and 236 and 76
     * are not. TimSort noticed and threw the general-contract exception.
     *
     * The requirement is real; expressing it as a tolerance is what breaks.
     * Assign each amount to a fixed band, compare bands, and use the amount
     * itself as a tiebreak. Banding is a function of one element, so the
     * result is transitive by construction.
     */
    static final int BAND_MINOR = 100;
    static final Comparator<Order> BY_BAND =
        Comparator.comparingInt((Order o) -> o.amountMinor() / BAND_MINOR)
                  .thenComparingInt(Order::amountMinor);

    /*
     * Defect 3. A TreeSet decides membership with its comparator, not with
     * equals. Comparing only on customer meant every order after the first for
     * a given customer was treated as a duplicate and dropped — 150 of 200
     * rows, silently, with no exception and no log line.
     *
     * De-duplication is an equals question, so use an equals-based Set.
     * LinkedHashSet also preserves encounter order, which a HashSet would not.
     */
    static List<Order> deduplicate(List<Order> orders) {
        return new ArrayList<>(new LinkedHashSet<>(orders));
    }

    /*
     * Defect 4. `stream().sorted(cmp).limit(10)` orders all 100,000 rows to
     * keep ten — sorted() is a full barrier, so limit() saves nothing.
     *
     * A bounded min-heap of size k keeps the k largest seen so far. Each new
     * element costs one comparison against the smallest of those, and only the
     * rare improvement costs a heap operation: O(n log k) time, O(k) memory.
     */
    static List<Order> topTen(List<Order> orders, Comparator<Order> counted) {
        final int k = 10;
        // Smallest-first, so peek() is the weakest member of the current top k.
        PriorityQueue<Order> best = new PriorityQueue<>(k, counted.reversed());
        for (Order o : orders) {
            if (best.size() < k) {
                best.offer(o);
            } else if (counted.compare(o, best.peek()) < 0) {   // strictly better
                best.poll();
                best.offer(o);
            }
        }
        List<Order> out = new ArrayList<>(best);
        out.sort(counted);
        return out;
    }

    /* ── checks (unchanged from the starter) ────────────────────────────── */

    static <T> Comparator<T> counting(Comparator<T> inner, long[] calls) {
        return (a, b) -> { calls[0]++; return inner.compare(a, b); };
    }

    static List<Order> sample(int n, long seed) {
        Random r = new Random(seed);
        List<Order> out = new ArrayList<>(n);
        for (int i = 0; i < n; i++)
            out.add(new Order(r.nextLong(), r.nextInt(1_000_000), "cust-" + r.nextInt(50)));
        return out;
    }

    public static void main(String[] args) {
        List<String> failures = new ArrayList<>();

        List<Order> wide = List.of(
            new Order(Long.MAX_VALUE, 10, "a"),
            new Order(-5L, 20, "b"),
            new Order(4_294_967_296L, 30, "c"),
            new Order(0L, 40, "d"));
        List<Order> sorted = byId(wide, counting(BY_ID, new long[1]));
        for (int i = 1; i < sorted.size(); i++) {
            if (sorted.get(i - 1).id() > sorted.get(i).id()) {
                failures.add("1. statement is not in id order: "
                    + sorted.stream().map(o -> String.valueOf(o.id())).collect(Collectors.joining(", ")));
                break;
            }
        }

        List<Order> triples = new ArrayList<>();
        Random rb = new Random(7);
        for (int i = 0; i < 40; i++) triples.add(new Order(i, rb.nextInt(500), "cust"));
        outer:
        for (Order a : triples) for (Order b : triples) {
            if (Integer.signum(BY_BAND.compare(a, b)) != -Integer.signum(BY_BAND.compare(b, a))) {
                failures.add("2. BY_BAND is not antisymmetric");
                break outer;
            }
            for (Order c : triples) {
                if (BY_BAND.compare(a, b) <= 0 && BY_BAND.compare(b, c) <= 0 && BY_BAND.compare(a, c) > 0) {
                    failures.add("2. BY_BAND is not transitive: it puts " + a.amountMinor()
                        + " at or before " + b.amountMinor() + ", and " + b.amountMinor()
                        + " at or before " + c.amountMinor() + ", but " + a.amountMinor()
                        + " after " + c.amountMinor());
                    break outer;
                }
            }
        }

        List<Order> withDupes = new ArrayList<>(sample(200, 11));
        withDupes.add(withDupes.get(0));
        int expected = (int) withDupes.stream().distinct().count();
        int actual = deduplicate(withDupes).size();
        if (actual != expected)
            failures.add("3. deduplicate kept " + actual + " of " + expected + " distinct orders — "
                       + (expected - actual) + " rows silently dropped");

        List<Order> many = sample(100_000, 13);
        long[] calls = new long[1];
        Comparator<Order> largestFirst = Comparator.comparingInt(Order::amountMinor)
                                                   .reversed()
                                                   .thenComparingLong(Order::id);
        List<Order> top = topTen(many, counting(largestFirst, calls));
        List<Order> reference = many.stream().sorted(largestFirst).limit(10).toList();
        if (!top.equals(reference))
            failures.add("4. top ten is wrong");
        else if (calls[0] > 500_000)
            failures.add("4. top ten cost " + calls[0] + " comparisons for 10 rows out of "
                       + many.size() + " — budget is 500000");

        if (failures.isEmpty()) {
            System.out.println("PASS");
        } else {
            failures.forEach(f -> System.out.println("  " + f));
            System.out.println("FAIL");
        }
    }
}

Stretch

The banding comparator is transitive once amounts are bucketed. Now suppose the requirement is genuinely fuzzy — "group orders that are within one rupee of each other" — where bucketing at fixed boundaries splits 99 and 101 into different bands. Sorting cannot express that at all. Say what can, and why a comparator was the wrong tool from the start.

← Back to Which sort does Arrays.sort use, and why does it depend on the type?