From 8ff9957b9a4a50ca08fe86fc3a4cba5d4a69562f Mon Sep 17 00:00:00 2001
From: Googler <jhorvitz@google.com>
Date: Mon, 5 Feb 2024 18:24:31 -0800
Subject: [PATCH] Make attempting to rewind a source artifact a hard crash.

This simplifies adding support for rewinding of lost top-level outputs, for which there is no failed `Action`.

Given that we never expect this to happen, a less detailed error message is acceptable.

PiperOrigin-RevId: 604494510
Change-Id: I7088218b12ccee71ee1485a9cf491679a15840c1
---
 .../rewinding/ActionRewindStrategy.java       | 38 ++++---------------
 src/main/protobuf/failure_details.proto       |  4 +-
 2 files changed, 10 insertions(+), 32 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java
index f68268fe34d539..5f25a12d423f68 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/rewinding/ActionRewindStrategy.java
@@ -145,8 +145,7 @@ public RewindPlan getRewindPlan(
                 .addAll(failedActionDeps)
                 .addAll(Artifact.keys(nestedSetKeys.keySet()))
                 .build(),
-            failedAction,
-            lostInputsException);
+            failedAction);
 
     for (DerivedArtifact lostArtifact : lostArtifacts) {
       SkyKey artifactKey = Artifact.key(lostArtifact);
@@ -289,17 +288,14 @@ private Set<DerivedArtifact> getLostInputOwningDirectDeps(
       ImmutableList<ActionInput> lostInputs,
       ActionInputDepOwners inputDepOwners,
       Set<SkyKey> failedActionDeps,
-      Action failedAction,
-      LostInputsActionExecutionException lostInputsException)
-      throws ActionExecutionException {
-
+      Action failedAction) {
     Set<DerivedArtifact> lostInputOwningDirectDeps = new HashSet<>();
     for (ActionInput lostInput : lostInputs) {
       boolean foundLostInputDepOwner = false;
 
       Collection<Artifact> owners = inputDepOwners.getDepOwners(lostInput);
       for (Artifact owner : owners) {
-        checkDerived(/*lostInputQualifier=*/ " owner", owner, failedAction, lostInputsException);
+        checkDerived(owner);
 
         // Rewinding must invalidate all Skyframe paths from the failed action to the action which
         // generates the lost input. Intermediate nodes not on the shortest path to that action may
@@ -309,11 +305,7 @@ private Set<DerivedArtifact> getLostInputOwningDirectDeps(
 
         Collection<Artifact> transitiveOwners = inputDepOwners.getDepOwners(owner);
         for (Artifact transitiveOwner : transitiveOwners) {
-          checkDerived(
-              /*lostInputQualifier=*/ " transitive owner",
-              transitiveOwner,
-              failedAction,
-              lostInputsException);
+          checkDerived(transitiveOwner);
 
           if (failedActionDeps.contains(Artifact.key(transitiveOwner))) {
             // The lost input is included in an aggregation artifact (e.g. a tree artifact or
@@ -334,8 +326,7 @@ private Set<DerivedArtifact> getLostInputOwningDirectDeps(
 
       if (lostInput instanceof Artifact
           && failedActionDeps.contains(Artifact.key((Artifact) lostInput))) {
-        checkDerived(
-            /* lostInputQualifier= */ "", (Artifact) lostInput, failedAction, lostInputsException);
+        checkDerived((Artifact) lostInput);
 
         lostInputOwningDirectDeps.add((DerivedArtifact) lostInput);
         foundLostInputDepOwner = true;
@@ -357,23 +348,8 @@ private Set<DerivedArtifact> getLostInputOwningDirectDeps(
     return lostInputOwningDirectDeps;
   }
 
-  private void checkDerived(
-      String lostInputQualifier,
-      Artifact expectedDerived,
-      Action failedAction,
-      LostInputsActionExecutionException lostInputsException)
-      throws ActionExecutionException {
-    if (!expectedDerived.isSourceArtifact()) {
-      return;
-    }
-    // TODO(b/19539699): tighten signatures for ActionInputDepOwnerMap to make this impossible.
-    String message =
-        String.format(
-            "Unexpected source artifact as lost input%s: %s %s",
-            lostInputQualifier, expectedDerived, failedAction);
-    bugReporter.sendBugReport(new IllegalStateException(message));
-    throw createActionExecutionException(
-        lostInputsException, failedAction, message, Code.LOST_INPUT_IS_SOURCE);
+  private static void checkDerived(Artifact artifact) {
+    checkState(!artifact.isSourceArtifact(), "Unexpected source artifact: %s", artifact);
   }
 
   /**
diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto
index 389ceb49fce583..a2eb6a5d584d47 100644
--- a/src/main/protobuf/failure_details.proto
+++ b/src/main/protobuf/failure_details.proto
@@ -1035,8 +1035,10 @@ message ActionRewinding {
   enum Code {
     ACTION_REWINDING_UNKNOWN = 0 [(metadata) = { exit_code: 37 }];
     LOST_INPUT_TOO_MANY_TIMES = 1 [(metadata) = { exit_code: 1 }];
-    LOST_INPUT_IS_SOURCE = 2 [(metadata) = { exit_code: 1 }];
     REWIND_LOST_INPUTS_PREREQ_UNMET = 3 [(metadata) = { exit_code: 2 }];
+    // Deprecated: attempting to rewind a source artifact is now a hard crash.
+    DEPRECATED_LOST_INPUT_IS_SOURCE = 2
+        [(metadata) = { exit_code: 1 }, deprecated = true];
   }
 
   Code code = 1;