Merge pull request #2297 from cloudflare/jlee/actor-exception-autogat…

…e-cleanup

Actor exception autogate cleanup

src/workerd/api/streams/standard-test.c++

@@ -3,7 +3,6 @@
 #include <workerd/jsg/jsg.h>
 #include <workerd/jsg/jsg-test.h>
 #include <workerd/jsg/observer.h>
-#include <workerd/util/autogate.h>
 
 namespace workerd::api {
 namespace {
@@ -16,7 +15,6 @@ struct RsContext: public jsg::Object, public jsg::ContextGlobal {
 JSG_DECLARE_ISOLATE_TYPE(RsIsolate, RsContext, ReadResult);
 
 void preamble(auto callback) {
-  util::Autogate::initEmptyAutogateForTesting();
   RsIsolate isolate(v8System, kj::heap<jsg::IsolateObserver>());
   isolate.runInLockScope([&](RsIsolate::Lock& lock) {
     JSG_WITHIN_CONTEXT_SCOPE(lock,

src/workerd/io/actor-cache-test.c++

@@ -10,7 +10,6 @@
 #include "io-gate.h"
 #include <kj/thread.h>
 #include <kj/source-location.h>
-#include <workerd/util/autogate.h>
 #include <workerd/util/capnp-mock.h>
 #include <workerd/util/test.h>
 
@@ -199,7 +198,6 @@ struct ActorCacheTestOptions {
   size_t maxKeysPerRpc = 128;
   bool noCache = false;
   bool neverFlush = false;
-  bool withUpdatedExceptionTypes = false;
 };
 
 struct ActorCacheTest: public ActorCacheConvenienceWrappers {
@@ -228,21 +226,7 @@ struct ActorCacheTest: public ActorCacheConvenienceWrappers {
         cache(kj::mv(mockPair.client), lru, gate),
         gateBrokenPromise(options.monitorOutputGate
             ? eagerlyReportExceptions(gate.onBroken())
-            : kj::Promise<void>(kj::READY_NOW)) {
-    // Set up autogate
-    capnp::MallocMessageBuilder message;
-    auto orphanage = message.getOrphanage();
-    kj::Vector<kj::StringPtr> strs;
-    if (options.withUpdatedExceptionTypes) {
-      strs.add("workerd-autogate-updated-actor-exception-types"_kj);
-    }
-    auto gatesOrphan = orphanage.newOrphan<capnp::List<capnp::Text>>(strs.size());
-    auto gates = gatesOrphan.get();
-    for (auto i: kj::indices(strs)) {
-      gates.set(i, strs[i]);
-    }
-    util::Autogate::initAutogate(gates.asReader());
-  }
+            : kj::Promise<void>(kj::READY_NOW)) {}
 
   ~ActorCacheTest() noexcept(false) {
     // Make sure if the output gate has been broken, the exception was reported. This is important
@@ -5266,13 +5250,9 @@ KJ_TEST("ActorCache can shutdown") {
 
   struct VerifyOptions {
     kj::Maybe<const kj::Exception&> maybeError;
-    bool withUpdatedExceptionTypes = false;
   };
   auto verifyWithOptions = [&](auto&& beforeShutdown, auto&& afterShutdown, VerifyOptions options) {
-    auto test = ActorCacheTest({
-      .monitorOutputGate = false,
-      .withUpdatedExceptionTypes = options.withUpdatedExceptionTypes,
-    });
+    auto test = ActorCacheTest({.monitorOutputGate = false});
     auto& ws = test.ws;
 
     BeforeShutdownResult res = beforeShutdown(test);
@@ -5283,14 +5263,9 @@ KJ_TEST("ActorCache can shutdown") {
 
     afterShutdown(test, kj::mv(res.maybeReq));
 
-    KJ_ASSERT(options.withUpdatedExceptionTypes ==
-        util::Autogate::isEnabled(util::AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES));
-    auto defaultError = (options.withUpdatedExceptionTypes
-            ? KJ_EXCEPTION(DISCONNECTED, kj::str(ActorCache::SHUTDOWN_ERROR_MESSAGE))
-            : KJ_EXCEPTION(OVERLOADED, kj::str(ActorCache::SHUTDOWN_ERROR_MESSAGE)));
     auto error = options.maybeError.map([](const kj::Exception& e) {
       return kj::cp(e);
-    }).orDefault(defaultError);
+    }).orDefault(KJ_EXCEPTION(DISCONNECTED, kj::str(ActorCache::SHUTDOWN_ERROR_MESSAGE)));
 
     if (res.shouldBreakOutputGate) {
       // We expected the output gate to break async after shutdown.
@@ -5322,14 +5297,8 @@ KJ_TEST("ActorCache can shutdown") {
   };
 
   auto verify = [&](auto&& beforeShutdown, auto&& afterShutdown) {
-    verifyWithOptions(beforeShutdown, afterShutdown,
-        { .maybeError = kj::none, .withUpdatedExceptionTypes = false });
-    verifyWithOptions(beforeShutdown, afterShutdown,
-        { .maybeError = kj::none, .withUpdatedExceptionTypes = true });
-    verifyWithOptions(beforeShutdown, afterShutdown,
-        { .maybeError = KJ_EXCEPTION(FAILED, "Nope."), .withUpdatedExceptionTypes = false });
-    verifyWithOptions(beforeShutdown, afterShutdown,
-        { .maybeError = KJ_EXCEPTION(FAILED, "Nope."), .withUpdatedExceptionTypes = true });
+    verifyWithOptions(beforeShutdown, afterShutdown, {.maybeError = kj::none});
+    verifyWithOptions(beforeShutdown, afterShutdown, {.maybeError = KJ_EXCEPTION(FAILED, "Nope.")});
   };
 
   verify([](ActorCacheTest& test){

src/workerd/io/actor-cache.c++

@@ -9,7 +9,6 @@
 
 #include <workerd/jsg/jsg.h>
 #include <workerd/io/io-gate.h>
-#include <workerd/util/autogate.h>
 #include <workerd/util/sentry.h>
 #include <workerd/util/duration-exceeded-logger.h>
 
@@ -2225,11 +2224,8 @@ void ActorCache::shutdown(kj::Maybe<const kj::Exception&> maybeException) {
       }
 
       // Use the direct constructor so that we can reuse the constexpr message variable for testing.
-      auto excType = (util::Autogate::isEnabled(util::AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES)
-              ? kj::Exception::Type::DISCONNECTED
-              : kj::Exception::Type::OVERLOADED);
-      auto exception =
-          kj::Exception(excType, __FILE__, __LINE__, kj::heapString(SHUTDOWN_ERROR_MESSAGE));
+      auto exception = kj::Exception(kj::Exception::Type::DISCONNECTED, __FILE__, __LINE__,
+          kj::heapString(SHUTDOWN_ERROR_MESSAGE));
 
       // Add trace info sufficient to tell us which operation caused the failure.
       exception.addTraceHere();
@@ -2630,15 +2626,10 @@ kj::Promise<void> ActorCache::flushImpl(uint retryCount) {
       } else {
         LOG_NOSENTRY(ERROR, "actor cache flush failed", e);
       }
-      if (util::Autogate::isEnabled(util::AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES)) {
-        // Pass through exception type to convey appropriate retry behavior.
-        return kj::Exception(e.getType(), __FILE__, __LINE__,
-            kj::str("broken.outputGateBroken; jsg.Error: Internal error in Durable "
-                "Object storage write caused object to be reset."));
-      } else {
-        return KJ_EXCEPTION(FAILED, "broken.outputGateBroken; jsg.Error: Internal error in Durable "
-            "Object storage write caused object to be reset.");
-      }
+      // Pass through exception type to convey appropriate retry behavior.
+      return kj::Exception(e.getType(), __FILE__, __LINE__,
+          kj::str("broken.outputGateBroken; jsg.Error: Internal error in Durable "
+              "Object storage write caused object to be reset."));
     }
   });
 }
@@ -2997,15 +2988,9 @@ kj::Promise<void> ActorCache::flushImplDeleteAll(uint retryCount) {
       return kj::mv(e);
     } else {
       LOG_EXCEPTION("actorCacheDeleteAll", e);
-      if (util::Autogate::isEnabled(util::AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES)) {
-        // Pass through exception type to convey appropriate retry behavior.
-        return kj::Exception(e.getType(), __FILE__, __LINE__,
-            kj::str("broken.outputGateBroken; jsg.Error: Internal error in Durable Object storage deleteAll() caused object to be reset."));
-      } else {
-        return KJ_EXCEPTION(FAILED,
-            "broken.outputGateBroken; jsg.Error: Internal error in Durable Object storage deleteAll() caused object to be "
-            "reset.");
-      }
+      // Pass through exception type to convey appropriate retry behavior.
+      return kj::Exception(e.getType(), __FILE__, __LINE__,
+          kj::str("broken.outputGateBroken; jsg.Error: Internal error in Durable Object storage deleteAll() caused object to be reset."));
     }
   });
 }

src/workerd/io/actor-sqlite.c++

@@ -5,7 +5,6 @@
 #include "actor-sqlite.h"
 #include <algorithm>
 #include <workerd/jsg/jsg.h>
-#include <workerd/util/autogate.h>
 #include "io-gate.h"
 
 namespace workerd {
@@ -315,11 +314,8 @@ void ActorSqlite::shutdown(kj::Maybe<const kj::Exception&> maybeException) {
       }
 
       // Use the direct constructor so that we can reuse the constexpr message variable for testing.
-      auto excType = (util::Autogate::isEnabled(util::AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES)
-              ? kj::Exception::Type::DISCONNECTED
-              : kj::Exception::Type::OVERLOADED);
-      auto exception = kj::Exception(
-          excType, __FILE__, __LINE__, kj::heapString(ActorCache::SHUTDOWN_ERROR_MESSAGE));
+      auto exception = kj::Exception(kj::Exception::Type::DISCONNECTED, __FILE__, __LINE__,
+          kj::heapString(ActorCache::SHUTDOWN_ERROR_MESSAGE));
 
       // Add trace info sufficient to tell us which operation caused the failure.
       exception.addTraceHere();

src/workerd/io/worker.c++

@@ -6,7 +6,6 @@
 #include <workerd/io/worker.h>
 #include <workerd/io/promise-wrapper.h>
 #include "actor-cache.h"
-#include <workerd/util/autogate.h>
 #include <workerd/util/batch-queue.h>
 #include <workerd/util/color-util.h>
 #include <workerd/util/mimetype.h>
@@ -2986,15 +2985,9 @@ struct Worker::Actor::Impl {
       auto timeout = 30 * kj::SECONDS;
       co_await timerChannel.afterLimitTimeout(timeout);
 
-      if (util::Autogate::isEnabled(util::AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES)) {
-        kj::throwFatalException(KJ_EXCEPTION(OVERLOADED,
-              "broken.outputGateBroken; jsg.Error: Durable Object storage operation exceeded "
-              "timeout which caused object to be reset."));
-      } else {
-        kj::throwFatalException(KJ_EXCEPTION(FAILED,
-              "broken.outputGateBroken; jsg.Error: Durable Object storage operation exceeded "
-              "timeout which caused object to be reset."));
-      }
+      kj::throwFatalException(KJ_EXCEPTION(OVERLOADED,
+            "broken.outputGateBroken; jsg.Error: Durable Object storage operation exceeded "
+            "timeout which caused object to be reset."));
     }
 
     // Implements OutputGate::Hooks.

src/workerd/jsg/BUILD.bazel

@@ -58,7 +58,6 @@ wd_cc_library(
         ":observer",
         ":url",
         "//src/workerd/util",
-        "//src/workerd/util:autogate",
         "//src/workerd/util:sentry",
         "//src/workerd/util:thread-scopes",
         "@capnp-cpp//src/kj",

src/workerd/jsg/jsg-test.h

@@ -8,7 +8,6 @@
 #include "jsg.h"
 #include "setup.h"
 #include <kj/test.h>
-#include <workerd/util/autogate.h>
 
 namespace workerd::jsg::test {
 
@@ -27,9 +26,7 @@ class Evaluator {
   //   in cases that the isolate includes types that require configuration, but currently the
   //   type is always default-constructed. What if you want to specify a test config?
 public:
-  explicit Evaluator(V8System& v8System) : v8System(v8System) {
-    util::Autogate::initEmptyAutogateForTesting();
-  }
+  explicit Evaluator(V8System& v8System) : v8System(v8System) {}
 
   IsolateType& getIsolate() {
     // Slightly more efficient to only instantiate each isolate type once (17s vs. 20s):

src/workerd/jsg/modules-new-test.c++

@@ -5,7 +5,6 @@
 #include <workerd/jsg/setup.h>
 #include <workerd/jsg/modules-new.h>
 #include <workerd/jsg/modules.capnp.h>
-#include <workerd/util/autogate.h>
 #include "observer.h"
 #include "url.h"
 #include <kj/async-io.h>
@@ -109,7 +108,6 @@ struct TestContext: public Object, public ContextGlobal {
 JSG_DECLARE_ISOLATE_TYPE(TestIsolate, TestContext, TestType);
 
 #define PREAMBLE(fn)                                                                     \
-  util::Autogate::initEmptyAutogateForTesting();                                         \
   TestIsolate isolate(v8System, 123, kj::heap<IsolateObserver>());                       \
   runInV8Stack([&](auto& stackScope) {                                                   \
     TestIsolate::Lock lock(isolate, stackScope);                                         \

src/workerd/jsg/util.c++

@@ -225,12 +225,10 @@ DecodedException decodeTunneledException(v8::Isolate* isolate,
     setRemoteError(isolate, result.handle);
   }
 
-  if (util::Autogate::isEnabled(util::AutogateKey::ACTOR_EXCEPTION_PROPERTIES)) {
-    if (excType == kj::Exception::Type::DISCONNECTED) {
-      setRetryableError(isolate, result.handle);
-    } else if (excType == kj::Exception::Type::OVERLOADED) {
-      setOverloadedError(isolate, result.handle);
-    }
+  if (excType == kj::Exception::Type::DISCONNECTED) {
+    setRetryableError(isolate, result.handle);
+  } else if (excType == kj::Exception::Type::OVERLOADED) {
+    setOverloadedError(isolate, result.handle);
   }
 
   if (result.isDurableObjectReset) {
@@ -286,10 +284,8 @@ v8::Local<v8::Value> makeInternalError(v8::Isolate* isolate, kj::Exception&& exc
         setRemoteError(isolate, exception);
       }
 
-      if (util::Autogate::isEnabled(util::AutogateKey::ACTOR_EXCEPTION_PROPERTIES)) {
-        // DISCONNECTED exceptions are considered retryable
-        setRetryableError(isolate, exception);
-      }
+      // DISCONNECTED exceptions are considered retryable
+      setRetryableError(isolate, exception);
 
       if (tunneledException.isDurableObjectReset) {
         setDurableObjectResetError(isolate, exception);

src/workerd/jsg/value.h

@@ -14,7 +14,6 @@
 #include <kj/time.h>
 #include <kj/debug.h>
 #include <kj/one-of.h>
-#include <workerd/util/autogate.h>
 
 namespace workerd::jsg {
 
@@ -1295,17 +1294,15 @@ class ExceptionWrapper {
     kj::Exception result = [&]() {
 
       kj::Exception::Type excType = [&]() {
-        if (util::Autogate::isEnabled(util::AutogateKey::ACTOR_EXCEPTION_PROPERTIES)) {
-          // Use .retryable and .overloaded properties as hints for what kj exception type to use.
-          if (handle->IsObject()) {
-            auto object = handle.As<v8::Object>();
+        // Use .retryable and .overloaded properties as hints for what kj exception type to use.
+        if (handle->IsObject()) {
+          auto object = handle.As<v8::Object>();
 
-            if (js.toBool(check(object->Get(context, v8StrIntern(js.v8Isolate, "overloaded"_kj))))) {
-              return kj::Exception::Type::OVERLOADED;
-            }
-            if (js.toBool(check(object->Get(context, v8StrIntern(js.v8Isolate, "retryable"_kj))))) {
-              return kj::Exception::Type::DISCONNECTED;
-            }
+          if (js.toBool(check(object->Get(context, v8StrIntern(js.v8Isolate, "overloaded"_kj))))) {
+            return kj::Exception::Type::OVERLOADED;
+          }
+          if (js.toBool(check(object->Get(context, v8StrIntern(js.v8Isolate, "retryable"_kj))))) {
+            return kj::Exception::Type::DISCONNECTED;
           }
         }
         return kj::Exception::Type::FAILED;

src/workerd/util/autogate.c++

@@ -4,7 +4,6 @@
 #include "autogate.h"
 #include <workerd/util/sentry.h>
 #include <kj/common.h>
-#include <capnp/message.h>
 
 namespace workerd::util {
 
@@ -14,10 +13,6 @@ kj::StringPtr KJ_STRINGIFY(AutogateKey key) {
   switch (key) {
     case AutogateKey::TEST_WORKERD:
       return "test-workerd"_kj;
-    case AutogateKey::UPDATED_ACTOR_EXCEPTION_TYPES:
-      return "updated-actor-exception-types";
-    case AutogateKey::ACTOR_EXCEPTION_PROPERTIES:
-      return "actor-exception-properties";
     case AutogateKey::NumOfKeys:
       KJ_FAIL_ASSERT("NumOfKeys should not be used in getName");
   }
@@ -59,14 +54,6 @@ void Autogate::initAutogate(capnp::List<capnp::Text>::Reader gates) {
   globalAutogate = Autogate(gates);
 }
 
-void Autogate::initEmptyAutogateForTesting() {
-  capnp::MallocMessageBuilder message;
-  auto orphanage = message.getOrphanage();
-  auto gatesOrphan = orphanage.newOrphan<capnp::List<capnp::Text>>(0);
-  auto gates = gatesOrphan.get();
-  initAutogate(gates.asReader());
-}
-
 void Autogate::deinitAutogate() { globalAutogate = kj::none; }
 
 }  // namespace workerd::server

src/workerd/util/autogate.h

@@ -13,8 +13,6 @@ namespace workerd::util {
 // Workerd-specific list of autogate keys (can also be used in internal repo).
 enum class AutogateKey {
   TEST_WORKERD,
-  UPDATED_ACTOR_EXCEPTION_TYPES, // updates exception types to better match retriability
-  ACTOR_EXCEPTION_PROPERTIES, // adds .retryable and .overloaded properties to tunneled exceptions
   NumOfKeys // Reserved for iteration.
 };
 
@@ -42,9 +40,6 @@ class Autogate {
   static void initAutogate(
       capnp::List<capnp::Text>::Reader autogates);
 
-  // Convenience method for tests to use to invoke initAutogate()
-  static void initEmptyAutogateForTesting();
-
   // Destroys an initialised global Autogate instance. Used only for testing.
   static void deinitAutogate();
 private: