Move InvocationPolicy from a startup argument to part of the RunRequest proto.

The user interface is not changing. The policy will still be accepted as a flag passed to the client, as a startup flag (before the command), it will just no longer trigger server restarts and will not be passed on to a bazel server as part of the startup arguments. In batch mode, however, it will still be a startup argument, because the RunRequest proto is not sent, and all invocations restart bazel in batch mode anyway.

Since invocation policy only affects command arguments, and changes in command arguments do not cause server restarts, this is consistent with other server restart behavior.

RELNOTES: Changing --invocation_policy will no longer force a server restart.
PiperOrigin-RevId: 152426207

5 files changed, 55 insertions, 28 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index 7d275a4cb0..f7fadb52ba 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer;
-import com.google.devtools.build.lib.flags.InvocationPolicyParser;
 import com.google.devtools.build.lib.runtime.commands.ProjectFileSupport;
 import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
 import com.google.devtools.build.lib.util.AbruptExitException;
@@ -246,7 +245,12 @@ public class BlazeCommandDispatcher {
    * client process, or throws {@link ShutdownBlazeServerException} to
    * indicate that a command wants to shutdown the Blaze server.
    */
-  int exec(List<String> args, OutErr outErr, LockingMode lockingMode, String clientDescription,
+  int exec(
+      InvocationPolicy invocationPolicy,
+      List<String> args,
+      OutErr outErr,
+      LockingMode lockingMode,
+      String clientDescription,
       long firstContactTime) throws ShutdownBlazeServerException, InterruptedException {
     OriginalCommandLineEvent originalCommandLine = new OriginalCommandLineEvent(args);
     Preconditions.checkNotNull(clientDescription);
@@ -303,8 +307,8 @@ public class BlazeCommandDispatcher {
         outErr.printErrLn("Server shut down " + shutdownReason);
         return ExitCode.LOCAL_ENVIRONMENTAL_ERROR.getNumericExitCode();
       }
-      return execExclusively(
-          originalCommandLine, args, outErr, firstContactTime, commandName, command, waitTimeInMs);
+      return execExclusively(originalCommandLine, invocationPolicy, args, outErr, firstContactTime,
+          commandName, command, waitTimeInMs);
     } catch (ShutdownBlazeServerException e) {
       shutdownReason = "explicitly by client " + currentClientDescription;
       throw e;
@@ -318,6 +322,7 @@ public class BlazeCommandDispatcher {
 
   private int execExclusively(
       OriginalCommandLineEvent originalCommandLine,
+      InvocationPolicy invocationPolicy,
       List<String> args,
       OutErr outErr,
       long firstContactTime,
@@ -395,12 +400,7 @@ public class BlazeCommandDispatcher {
       InvocationPolicy combinedPolicy =
           InvocationPolicy.newBuilder()
               .mergeFrom(runtime.getModuleInvocationPolicy())
-              .mergeFrom(
-                  InvocationPolicyParser.parsePolicy(
-                      getRuntime()
-                          .getStartupOptionsProvider()
-                          .getOptions(BlazeServerStartupOptions.class)
-                          .invocationPolicy))
+              .mergeFrom(invocationPolicy)
               .build();
       InvocationPolicyEnforcer optionsPolicyEnforcer = new InvocationPolicyEnforcer(combinedPolicy);
       optionsPolicyEnforcer.enforce(optionsParser, commandName);
@@ -546,8 +546,8 @@ public class BlazeCommandDispatcher {
   @VisibleForTesting
   public int exec(List<String> args, LockingMode lockingMode, String clientDescription,
       OutErr originalOutErr) throws ShutdownBlazeServerException, InterruptedException {
-    return exec(args, originalOutErr, LockingMode.ERROR_OUT, clientDescription,
-        runtime.getClock().currentTimeMillis());
+    return exec(InvocationPolicy.getDefaultInstance(), args, originalOutErr, LockingMode.ERROR_OUT,
+        clientDescription, runtime.getClock().currentTimeMillis());
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index cf8aff0bad..10812a4c01 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.buildeventstream.PathConverter;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.OutputFilter;
 import com.google.devtools.build.lib.flags.CommandNameCache;
+import com.google.devtools.build.lib.flags.InvocationPolicyParser;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
@@ -747,13 +748,17 @@ public final class BlazeRuntime {
             + commandLineOptions.getStartupArgs());
 
     BlazeRuntime runtime;
+    InvocationPolicy policy;
     try {
       runtime = newRuntime(modules, commandLineOptions.getStartupArgs(), null);
+      policy = InvocationPolicyParser.parsePolicy(
+          runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class)
+              .invocationPolicy);
     } catch (OptionsParsingException e) {
-      OutErr.SYSTEM_OUT_ERR.printErr(e.getMessage());
+      OutErr.SYSTEM_OUT_ERR.printErrLn(e.getMessage());
       return ExitCode.COMMAND_LINE_ERROR.getNumericExitCode();
     } catch (AbruptExitException e) {
-      OutErr.SYSTEM_OUT_ERR.printErr(e.getMessage());
+      OutErr.SYSTEM_OUT_ERR.printErrLn(e.getMessage());
       return e.getExitCode().getNumericExitCode();
     }
 
@@ -761,7 +766,7 @@ public final class BlazeRuntime {
 
     try {
       LOG.info(getRequestLogString(commandLineOptions.getOtherArgs()));
-      return dispatcher.exec(commandLineOptions.getOtherArgs(), OutErr.SYSTEM_OUT_ERR,
+      return dispatcher.exec(policy, commandLineOptions.getOtherArgs(), OutErr.SYSTEM_OUT_ERR,
           LockingMode.ERROR_OUT, "batch client", runtime.getClock().currentTimeMillis());
     } catch (BlazeCommandDispatcher.ShutdownBlazeServerException e) {
       return e.getExitStatus();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
index 6f5b7b6e8b..3b70c8656a 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandExecutor.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.runtime;
 
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
 import com.google.devtools.build.lib.server.ServerCommand;
 import com.google.devtools.build.lib.util.io.OutErr;
 import java.io.PrintWriter;
@@ -39,12 +40,18 @@ public class CommandExecutor implements ServerCommand {
   }
 
   @Override
-  public int exec(List<String> args, OutErr outErr, BlazeCommandDispatcher.LockingMode lockingMode,
-      String clientDescription, long firstContactTime) throws InterruptedException {
+  public int exec(
+      InvocationPolicy invocationPolicy,
+      List<String> args,
+      OutErr outErr,
+      BlazeCommandDispatcher.LockingMode lockingMode,
+      String clientDescription,
+      long firstContactTime) throws InterruptedException {
     LOG.info(BlazeRuntime.getRequestLogString(args));
 
     try {
-      return dispatcher.exec(args, outErr, lockingMode, clientDescription, firstContactTime);
+      return dispatcher.exec(invocationPolicy, args, outErr, lockingMode, clientDescription,
+          firstContactTime);
     } catch (BlazeCommandDispatcher.ShutdownBlazeServerException e) {
       if (e.getCause() != null) {
         StringWriter message = new StringWriter();
diff --git a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
index 19e038f552..676337e8c6 100644
--- a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
@@ -22,8 +22,10 @@ import com.google.common.net.InetAddresses;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import com.google.common.util.concurrent.Uninterruptibles;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.flags.InvocationPolicyParser;
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.LockingMode;
 import com.google.devtools.build.lib.runtime.CommandExecutor;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
 import com.google.devtools.build.lib.server.CommandProtos.CancelRequest;
 import com.google.devtools.build.lib.server.CommandProtos.CancelResponse;
 import com.google.devtools.build.lib.server.CommandProtos.PingRequest;
@@ -38,6 +40,7 @@ import com.google.devtools.build.lib.util.ThreadUtils;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.common.options.OptionsParsingException;
 import com.google.protobuf.ByteString;
 import io.grpc.Server;
 import io.grpc.StatusRuntimeException;
@@ -840,14 +843,20 @@ public class GrpcServerImpl implements RPCServer {
           new RpcOutputStream(command.id, responseCookie, StreamType.STDOUT, sink),
           new RpcOutputStream(command.id, responseCookie, StreamType.STDERR, sink));
 
-      exitCode =
-          commandExecutor.exec(
-              args.build(),
-              rpcOutErr,
-              request.getBlockForLock() ? LockingMode.WAIT : LockingMode.ERROR_OUT,
-              request.getClientDescription(),
-              clock.currentTimeMillis());
-
+      try {
+        InvocationPolicy policy = InvocationPolicyParser.parsePolicy(request.getInvocationPolicy());
+        exitCode =
+            commandExecutor.exec(
+                policy,
+                args.build(),
+                rpcOutErr,
+                request.getBlockForLock() ? LockingMode.WAIT : LockingMode.ERROR_OUT,
+                request.getClientDescription(),
+                clock.currentTimeMillis());
+      } catch (OptionsParsingException e) {
+        rpcOutErr.printErrLn(e.getMessage());
+        exitCode = ExitCode.COMMAND_LINE_ERROR.getNumericExitCode();
+      }
     } catch (InterruptedException e) {
       exitCode = ExitCode.INTERRUPTED.getNumericExitCode();
       commandId = ""; // The default value, the client will ignore it
diff --git a/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java b/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
index 7b0a075f75..ef5f727bdb 100644
--- a/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/server/ServerCommand.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.server;
 
 import com.google.devtools.build.lib.runtime.BlazeCommandDispatcher;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
 import com.google.devtools.build.lib.util.io.OutErr;
 import java.util.List;
 
@@ -27,8 +28,13 @@ public interface ServerCommand {
    * Executes the request, writing any output or error messages into err.
    * Returns 0 on success; any other value or exception indicates an error.
    */
-  int exec(List<String> args, OutErr outErr, BlazeCommandDispatcher.LockingMode lockingMode,
-      String clientDescription, long firstContactTime) throws InterruptedException;
+  int exec(
+      InvocationPolicy policy,
+      List<String> args,
+      OutErr outErr,
+      BlazeCommandDispatcher.LockingMode lockingMode,
+      String clientDescription,
+      long firstContactTime) throws InterruptedException;
 
   /**
    * Whether the server needs to be shut down.