[#953] Refuse a server-error-result-code which does not report a failure

opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/GlobalConfiguration.xml

@@ -14,6 +14,7 @@
 
   Copyright 2007-2010 Sun Microsystems, Inc.
   Portions Copyright 2011-2016 ForgeRock AS.
+  Portions Copyright 2026 3A Systems, LLC.
   ! -->
 <adm:managed-object name="global" plural-name="globals"
   package="org.forgerock.opendj.server.config"
@@ -172,6 +173,13 @@
     <adm:synopsis>
       Specifies the numeric value of the result code when request
       processing fails due to an internal server error.
+      The value must be a result code which reports a failure. The five codes
+      which report a success - 0 (success), 5 (compare false), 6 (compare true),
+      14 (SASL bind in progress) and 16654 (no operation) - are refused: the
+      server would then report a request it could not process with a code that
+      says it succeeded, and a replication domain reading that code records a
+      change it never applied as replayed. A code the server does not know
+      reports a failure and is accepted.
     </adm:synopsis>
     <adm:default-behavior>
       <adm:defined>

opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java

@@ -30,6 +30,7 @@
 import org.forgerock.opendj.server.config.meta.GlobalCfgDefn.InvalidAttributeSyntaxBehavior;
 import org.forgerock.opendj.server.config.meta.GlobalCfgDefn.SingleStructuralObjectclassBehavior;
 import org.forgerock.opendj.server.config.server.GlobalCfg;
+import org.forgerock.util.annotations.VisibleForTesting;
 import org.opends.server.api.AuthenticationPolicy;
 import org.opends.server.api.LocalBackend;
 import org.opends.server.loggers.CommonAudit;
@@ -196,7 +197,7 @@ private void applyGlobalConfiguration(final GlobalCfg globalConfig, final CoreAt
     core.addMissingRDNAttributes = globalConfig.isAddMissingRDNAttributes();
     core.allowAttributeNameExceptions = globalConfig.isAllowAttributeNameExceptions();
     core.syntaxEnforcementPolicy = convert(globalConfig.getInvalidAttributeSyntaxBehavior());
-    core.serverErrorResultCode = ResultCode.valueOf(globalConfig.getServerErrorResultCode());
+    core.serverErrorResultCode = serverErrorResultCode(globalConfig.getServerErrorResultCode());
     core.singleStructuralClassPolicy = convert(globalConfig.getSingleStructuralObjectclassBehavior());
 
     core.notifyAbandonedOperations = globalConfig.isNotifyAbandonedOperations();
@@ -423,6 +424,11 @@ public boolean isConfigurationChangeAcceptable(GlobalCfg configuration,
       configAcceptable = false;
     }
 
+    if (!isServerErrorResultCodeAcceptable(configuration, unacceptableReasons))
+    {
+      configAcceptable = false;
+    }
+
     if (!isSubordinateDNsAcceptable(configuration, unacceptableReasons))
     {
       configAcceptable = false;
@@ -431,6 +437,80 @@ public boolean isConfigurationChangeAcceptable(GlobalCfg configuration,
     return configAcceptable;
   }
 
+  /**
+   * Returns the result code to put on an operation an internal error prevented this
+   * server from processing, reading the configured value and falling back on
+   * {@link ResultCode#OTHER} - the default of the setting - when it does not report a
+   * failure.
+   * <p>
+   * {@link #isConfigurationChangeAcceptable} refuses such a value, so the fallback is
+   * what a configuration written before that - or edited outside the server - runs into:
+   * the server starts on the code its own default names rather than refusing to start,
+   * and says which value it ignored. The core configuration is applied before the error
+   * loggers are configured, so at start-up the warning goes where every start-up message
+   * goes - the standard output of the server, {@code logs/server.out} when it was started
+   * by {@code start-ds} - rather than into {@code logs/errors}.
+   * <p>
+   * Package private for the tests: a value the fallback is for never gets past
+   * {@link #isConfigurationChangeAcceptable}, so no change to a running server can reach
+   * it, and the tests pin it directly rather than through a start-up.
+   *
+   * @param configured the configured numeric result code
+   * @return the result code to put on an internal error
+   */
+  @VisibleForTesting
+  static ResultCode serverErrorResultCode(int configured)
+  {
+    final ResultCode resultCode = ResultCode.valueOf(configured);
+    if (resultCode.isExceptional())
+    {
+      return resultCode;
+    }
+    logger.warn(WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE, configured, ResultCode.OTHER);
+    return ResultCode.OTHER;
+  }
+
+  /**
+   * Returns whether the configured result code reports a failure, which the code this
+   * server puts on an internal error has to.
+   * <p>
+   * The setting is a plain integer and used to accept any of them, including the five
+   * codes {@code ResultCode} registers as reporting a success. Each of those means
+   * something of its own to whoever reads a result code, and the reader then acts on that
+   * meaning while the operation it came from failed: the replay of a replication domain
+   * reads {@code NO_OPERATION} as "conflict resolution found the change already applied"
+   * and records a change which never reached the backend as replayed (issue #953), and
+   * {@code SUCCESS} has {@code LDAPReplicationDomain.synchronize()} both record it and
+   * publish the operation which failed to every other server of the topology. The
+   * configuration itself is a third reader: {@link #applyConfigurationChange} puts this
+   * code on a change to {@code cn=config} which failed to apply and keeps the new core
+   * attributes only when the result is {@code SUCCESS}, so a code of 0 reported that failure
+   * as a success and applied the change all the same. No reader can tell the two meanings
+   * apart once they are the same integer, which is why the value is refused here rather
+   * than worked around at each of them.
+   * <p>
+   * A code {@code ResultCode} does not know reports a failure - {@code valueOf()} answers
+   * an unknown code which does - so an administrator keeps the freedom to put a private
+   * code on an internal error.
+   *
+   * @param configuration the configuration to check
+   * @param unacceptableReasons where the reason is reported when the value is refused
+   * @return whether the configured result code is acceptable
+   */
+  private static boolean isServerErrorResultCodeAcceptable(
+      GlobalCfg configuration, List<LocalizableMessage> unacceptableReasons)
+  {
+    final int configured = configuration.getServerErrorResultCode();
+    final ResultCode resultCode = ResultCode.valueOf(configured);
+    if (resultCode.isExceptional())
+    {
+      return true;
+    }
+    unacceptableReasons.add(
+        ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(configured, resultCode));
+    return false;
+  }
+
   private boolean isSubordinateDNsAcceptable(GlobalCfg configuration, List<LocalizableMessage> unacceptableReasons)
   {
     try

opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java

@@ -459,9 +459,10 @@ && getBackend().getBackendID().equals(backend.getBackendID())) {
       new AtomicLong(UNREPLAYED_CHANGE_ALERT_NEVER_SENT);
   /**
    * The result codes conflict resolution knows how to solve. The result code the server
-   * puts on an internal error is configurable and is not validated as a result code, so
-   * it could be set to one of these: it must never take a change away from
-   * {@code solveNamingConflict()}, which is the only thing which can solve them.
+   * puts on an internal error is configurable, and every one of these reports a failure -
+   * which is all the configuration asks of it - so it can be set to one of them: it must
+   * never take a change away from {@code solveNamingConflict()}, which is the only thing
+   * which can solve them.
    */
   private static final Set<ResultCode> CONFLICT_RESULT_CODES = Collections.unmodifiableSet(
       newHashSet(
@@ -470,6 +471,23 @@ && getBackend().getBackendID().equals(backend.getBackendID())) {
           // solveNamingConflict(ModifyDNOperation) solves these two as well
           ResultCode.UNWILLING_TO_PERFORM, ResultCode.OBJECTCLASS_VIOLATION));
 
+  /**
+   * The attachment which says that conflict resolution turned this operation into a
+   * no-op, so that {@link #replay} reads that decision rather than the result code which
+   * reports it.
+   * <p>
+   * The code conflict resolution reports for a no-op is {@code NO_OPERATION}, and the
+   * code this server puts on an internal error is a configuration knob: while nothing
+   * validated it, the two could be the same code, and every change an internal error kept
+   * out of the backend was then read as a change conflict resolution had found already
+   * applied and recorded in the ServerState - the silent divergence of issue #889, one
+   * branch earlier (issue #953). The configuration refuses a code which does not report a
+   * failure now, so they can not be the same code anymore; the decision travels on the
+   * operation all the same, so that what the replay acts on is what conflict resolution
+   * decided rather than a value an administrator owns.
+   */
+  private static final String CONFLICT_RESOLUTION_NO_OP = "replicationConflictResolutionNoOp";
+
   private final PersistentServerState state;
   private volatile boolean generationIdSavedStatus;
 
@@ -1914,8 +1932,7 @@ SynchronizationProviderResult handleConflictResolution(
       }
       if (replayedEntryDN != null)
       {
-        return new SynchronizationProviderResult.StopProcessing(
-            ResultCode.NO_OPERATION, null);
+        return conflictResolutionFoundNothingToDo(addOperation);
       }
 
       /* The parent entry may have been renamed here since the change was done
@@ -2111,8 +2128,7 @@ SynchronizationProviderResult handleConflictResolution(
           modifyDNOperation.getOriginalEntry());
       if (hist.addedOrRenamedAfter(ctx.getCSN()))
       {
-        return new SynchronizationProviderResult.StopProcessing(
-            ResultCode.NO_OPERATION, null);
+        return conflictResolutionFoundNothingToDo(modifyDNOperation);
       }
     }
     else
@@ -2167,8 +2183,7 @@ SynchronizationProviderResult handleConflictResolution(
         {
           // Every modifications filtered in this operation: the operation
           // becomes a no-op
-          return new SynchronizationProviderResult.StopProcessing(
-            ResultCode.NO_OPERATION, null);
+          return conflictResolutionFoundNothingToDo(modifyOperation);
         }
       }
       else
@@ -2929,7 +2944,7 @@ private void replayChangeAndTheChangesWaitingForIt(
 
                 if (result != ResultCode.SUCCESS)
                 {
-                  if (result == ResultCode.NO_OPERATION)
+                  if (isConflictResolutionNoOp(op))
                   {
                     // Pre-operation conflict resolution detected that the operation
                     // was a no-op. For example, an add which has already been
@@ -3452,6 +3467,37 @@ private boolean updateError(CSN csn)
     }
   }
 
+  /**
+   * Stops an operation conflict resolution found nothing left to do for, and marks it so
+   * that the replay reads that decision off the operation rather than off the result code
+   * this answer carries.
+   *
+   * @param op the operation conflict resolution turned into a no-op
+   * @return the answer which stops the operation
+   */
+  private static SynchronizationProviderResult conflictResolutionFoundNothingToDo(PluginOperation op)
+  {
+    op.setAttachment(CONFLICT_RESOLUTION_NO_OP, Boolean.TRUE);
+    return new SynchronizationProviderResult.StopProcessing(ResultCode.NO_OPERATION, null);
+  }
+
+  /**
+   * Returns whether conflict resolution turned the replayed operation into a no-op, which
+   * says that the change it carries is in the data and can be recorded as replayed.
+   * <p>
+   * Only {@link #conflictResolutionFoundNothingToDo} answers {@code true} here. The
+   * result code that answer carries says the same thing, but it is a code the
+   * configuration can name as well - see {@link #CONFLICT_RESOLUTION_NO_OP} - and a
+   * change which failed must never be read as one which was already applied.
+   *
+   * @param op the operation which was replayed
+   * @return {@code true} if conflict resolution found nothing left to do for the change
+   */
+  private static boolean isConflictResolutionNoOp(Operation op)
+  {
+    return Boolean.TRUE.equals(op.getAttachment(CONFLICT_RESOLUTION_NO_OP));
+  }
+
   /**
    * Returns whether the provided result code reports a failure of this server rather
    * than a change which can not be applied: the backend being offline or rebuilt
@@ -3468,10 +3514,10 @@ private boolean updateError(CSN csn)
   static boolean isServerFailure(ResultCode result, ResultCode serverErrorResultCode)
   {
     /*
-     * The result code the server puts on an internal error is configurable and is not
-     * validated as a result code, so it may well be one conflict resolution knows how to
-     * solve: such a setting must not take a change away from solveNamingConflict(), which
-     * is the only thing which can solve them. A change it could not solve either is a
+     * The result code the server puts on an internal error is configurable and only has
+     * to report a failure, so it may well be one conflict resolution knows how to solve:
+     * such a setting must not take a change away from solveNamingConflict(), which is
+     * the only thing which can solve them. A change it could not solve either is a
      * failure of the server all the same, which replay() acts on once conflict resolution
      * has reported it.
      */

opendj-server-legacy/src/messages/org/opends/messages/config.properties

@@ -12,6 +12,7 @@
 #
 # Copyright 2006-2010 Sun Microsystems, Inc.
 # Portions Copyright 2013-2016 ForgeRock AS.
+# Portions Copyright 2026 3A Systems, LLC.
 
 
 
@@ -872,3 +873,11 @@ ERR_CONFIG_FILE_MODIFY_REJECTED_DUE_TO_EVALUATION_FAILURE_766=Entry '%s' cannot
   contained an expression '%s' that could not be evaluated: %s
 ERR_CONFIG_FILE_READ_FAILED_DUE_TO_EVALUATION_FAILURE_767=Entry '%s' cannot be read because attribute '%s' \
   contained an expression '%s' that could not be evaluated: %s
+ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE_768=The value '%s' is not acceptable for attribute \
+  ds-cfg-server-error-result-code because result code '%s' does not report a failure. This server puts that code \
+  on the operations an internal error prevents it from processing, so a code which reports a success leaves a \
+  failed operation indistinguishable from one which succeeded: a replication domain would record a change it \
+  never applied as replayed, or publish an operation which failed to the whole topology
+WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE_769=The value '%s' configured in attribute \
+  ds-cfg-server-error-result-code does not report a failure and is ignored: result code '%s' is used instead for \
+  the operations an internal error prevents this server from processing