Step 5: Implementation

You have a proven root cause and a confirmed mechanism. Now write the real fix. Not the throwaway one-liner from Step 4 — the version a maintainer will merge. The difference between those two is almost entirely discipline: minimum diff, right conventions, thread-safety, backward compatibility, no scope creep.

The rule that governs this step: every line in your diff must be justifiable in one sentence. If you cannot say why a line changed without saying "while I was in there," delete it.

Weigh the Fix Directions

You named three fix directions in Step 4. Now choose one, on the record, with reasons. Use the aggregation min_doc_count: 0 bug as the worked example.

Direction B wins because it has the smallest blast radius that actually fixes the bug, and it leaves the hot reduce path untouched. Write this table into your PR description — reviewers want to see you considered and rejected the obvious alternatives, not that you found the first thing that worked.

Find the Real Fix Site

The throwaway patch told you roughly where. The real fix goes where the abstraction says it belongs, which may be a layer up or down from where you proved the mechanism. Re-grep with the chosen direction in mind:

# Where does the empty-segment fast path actually live for terms?
grep -rn "getMinDocCount\|minDocCount" \
  server/src/main/java/org/opensearch/search/aggregations/bucket/terms/

# Confirm the field that carries min_doc_count through the aggregator.
grep -rn "bucketCountThresholds" \
  server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregator.java

Read the surrounding method end to end before you touch it. Understand the fast path's original purpose so your guard preserves it for every case except the one you are fixing.

The SPDX Header and Spotless

Every OpenSearch source file carries this header. If you create a new file (usually you won't — prefer editing the existing class), it must start with:

/*
 * SPDX-License-Identifier: Apache-2.0
 *
 * The OpenSearch Contributors require contributions made to
 * this file be licensed under the Apache-2.0 license or a
 * compatible open source license.
 */

When editing an existing file, leave its header untouched — do not reformat it, do not update years. precommit enforces header presence; reformatting it is a needless diff line.

Formatting is mechanical and non-negotiable. Run Spotless before you ever look at the diff:

./gradlew spotlessApply        # auto-format the files you changed
./gradlew spotlessJavaCheck    # verify (this is what CI runs)

Warning: Never hand-format to "match the surrounding style." Spotless owns formatting. If spotlessApply changes lines you didn't touch, that means the file was already non-conformant; do not sweep those into your PR — they are scope creep. Run git add -p and stage only your intentional lines, or rebase the unrelated reformat away.

Thread-Safety

OpenSearch is concurrent everywhere. Before you add or move a line, ask: what thread runs this, and what else runs concurrently?

For the engine seqno example, the fix is expanding a lock window, which is a thread-safety change by definition:

// WRONG: version resolved outside the lock, seqno assigned inside.
long version = resolveVersion(op);          // racy read
try (Releasable ignored = versionMap.acquireLock(op.uid().bytes())) {
    ...
}

// RIGHT: the whole read-modify-write is inside one lock acquisition.
try (Releasable ignored = versionMap.acquireLock(op.uid().bytes())) {
    long version = resolveVersion(op);
    ...
}

State the threading argument explicitly in your PR description. Reviewers of engine/coordination code will ask "what thread, what lock" first.

Backward Compatibility

OpenSearch nodes of adjacent versions run in the same cluster during a rolling upgrade, and segments/translog written by an old version are read by a new one. Two BWC tripwires:

1. Serialization changes. If you add or reorder a field on anything that implements Writeable (a request, response, cluster-state piece, or an InternalAggregation), the wire format changes, and an old node must still parse a new node's bytes (and vice versa). Guard the new field with a Version check on both sides:

   // writeTo
   if (out.getVersion().onOrAfter(Version.V_3_1_0)) {
       out.writeOptionalString(newField);
   }
   // the StreamInput constructor
   if (in.getVersion().onOrAfter(Version.V_3_1_0)) {
       this.newField = in.readOptionalString();
   }
   

   Use the next unreleased version constant (check server/src/main/java/org/opensearch/Version.java), never a hardcoded literal. Read the serialization & BWC deep dive before you touch any writeTo/readFrom. Most bug fixes — including all three Capstone examples — should avoid changing serialization. The aggregation fix changes only which buckets are produced, not the bucket wire format, so it has no serialization impact. Say that explicitly: "No serialization change; no Version guard needed."

2. Behavioral compatibility. Even with no wire change, you are changing an observable response (the aggregation now returns a bucket it didn't before). That is the intended fix, but call it out — it's a Fixed CHANGELOG entry, and if anyone depended on the buggy behavior, the PR discussion is where that surfaces. See the compatibility mindset chapter.

Settings Conventions

If your fix introduces a setting (resist this — a bug fix rarely needs a knob), follow the conventions exactly:

public static final Setting<Boolean> SOME_FLAG_SETTING = Setting.boolSetting(
    "search.aggregations.some_flag",
    true,                                   // default preserves prior behavior unless the bug demands otherwise
    Setting.Property.NodeScope,             // or Dynamic + IndexScope as appropriate
    Setting.Property.Dynamic
);

Register it in the relevant settings list (ClusterSettings/IndexScopedSettings wiring lives in SettingsModule / IndexScopedSettings.BUILT_IN_INDEX_SETTINGS), or precommit will fail with "unregistered setting." Default the setting so that existing clusters behave identically after upgrade unless the whole point of the fix is to change the default. For the Capstone bugs, no new setting is needed — the correct behavior is unconditional. Adding a flag to make a bug fix optional is itself a smell a reviewer will challenge.

The Diff

Here is what direction B looks like as a realistic, minimum-surface patch:

--- a/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregator.java
+++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/terms/TermsAggregator.java
@@ class TermsAggregator extends DeferableBucketAggregator {
     @Override
     protected void doPostCollection() throws IOException {
-        // Skip building buckets when this segment produced no ordinals for the field.
-        if (!hasValuesForField()) {
-            return;
-        }
+        // Skip building buckets when this segment produced no ordinals for the field,
+        // *unless* min_doc_count == 0, in which case the synthetic (and `missing`)
+        // bucket must still be emitted per shard so the reduce step can merge it. #NNNN
+        if (!hasValuesForField() && bucketCountThresholds.getMinDocCount() != 0) {
+            return;
+        }
         ...
     }
 }

Properties of a good fix diff, all visible above:

• Tens of lines, not hundreds. This is one guard condition.
• A comment citing the issue (#NNNN) at the non-obvious line — the future reader needs to know why the guard has that extra clause.
• No drive-by changes. The ... regions are untouched. No reformatting, no rename of hasValuesForField, no "I noticed this other thing."
• Behavior for the common case is provably unchanged — when getMinDocCount() != 0, the condition is identical to before.

Generate and read your own diff before you trust it:

git diff origin/main --stat        # which files? should be ONLY the bug's files + tests
git diff origin/main               # read every line; justify each in one sentence

If --stat lists a file unrelated to the bug, that is scope creep. Remove it.

Re-run the Reproducer

The same test that was red in Step 2 must now be green — and only because of your change:

./gradlew :server:test \
  --tests "org.opensearch.search.aggregations.bucket.terms.TermsAggregatorTests.testIssueNNNNRepro"

Green. If it's still red, the fix is wrong or in the wrong place — back to the mechanism. If it's green but you suspect the change is broader than needed, run the surrounding test class to catch collateral breakage early (full validation is Step 7):

./gradlew :server:test --tests "org.opensearch.search.aggregations.bucket.terms.*"

Deliverable for Step 5

• Three fix directions weighed in a table; one chosen with a one-line reason per rejection.
• A minimum-diff production change — tens of lines, every line justifiable.
• ./gradlew spotlessApply run; spotlessJavaCheck green; SPDX headers intact; no unrelated reformatting staged.
• Thread-safety argument stated (what thread, what lock/visibility).
• BWC assessed: either a Version-guarded serialization change or an explicit "no serialization change" note.
• The Step-2 reproducer is now green, and git diff --stat shows only the bug's files.

Validation / Self-check

Before advancing to Step 6:

1. Every line in git diff origin/main is justifiable in one sentence.
2. git diff origin/main --stat lists only files related to the bug (plus tests).
3. ./gradlew spotlessJavaCheck passes with no manual formatting.
4. You did not add a setting, a public-API change, or a serialization change unless the bug genuinely required it — and if you did, it is Version-guarded and registered.
5. You can state which thread runs the changed code and what concurrent access it must tolerate.
6. The Step-2 reproducer passes; behavior for the non-triggering case is provably identical.
7. You wrote the "three directions, one chosen" table for the PR description.

Then go to Step 6: Testing.