fix: KVStore error handling (#1060)

Guy Bedford · web-flow · commit 95885d88b471 · 2024-12-03T11:05:33.000-08:00
diff --git a/integration-tests/js-compute/fixtures/module-mode/src/kv-store.js b/integration-tests/js-compute/fixtures/module-mode/src/kv-store.js
@@ -8,9 +8,26 @@ import {
 } from './assertions.js';
 import { KVStore } from 'fastly:kv-store';
 import { routes, isRunningLocally } from './routes.js';
+import { sdkVersion } from 'fastly:experimental';
+
+const debug = sdkVersion.endsWith('-debug');
 
 // kvstore e2e tests
 {
+  routes.set('/kv-store/debug-error', async () => {
+    if (!debug) return;
+
+    // special debug function to create a kv entry with an invalid handle
+    const entry = fastly.dump(null, 'invalidkv');
+
+    // we can then test the invalid handle error
+    try {
+      await entry.text();
+    } catch (e) {
+      strictEqual(e.message.includes('Invalid handle'), true);
+    }
+  });
+
   routes.set('/kv-store-e2e/list', async () => {
     const store = new KVStore('example-test-kv-store');
     try {
diff --git a/integration-tests/js-compute/fixtures/module-mode/tests.json b/integration-tests/js-compute/fixtures/module-mode/tests.json
@@ -147,6 +147,7 @@
       "body": "hello world!"
     }
   },
+  "GET /kv-store/debug-error": {},
   "GET /kv-store/delete/called-as-constructor": {},
   "GET /kv-store/delete/called-unbound": {},
   "GET /kv-store/delete/key-parameter-calls-7.1.17-ToString": {},
diff --git a/runtime/fastly/builtins/backend.cpp b/runtime/fastly/builtins/backend.cpp
@@ -28,9 +28,9 @@
 #include "fastly.h"
 
 using builtins::BuiltinImpl;
+using fastly::FastlyGetErrorMessage;
 using fastly::common::parse_and_validate_timeout;
 using fastly::fastly::Fastly;
-using fastly::fastly::FastlyGetErrorMessage;
 using fastly::fetch::RequestOrResponse;
 using fastly::secret_store::SecretStoreEntry;
 
diff --git a/runtime/fastly/builtins/body.cpp b/runtime/fastly/builtins/body.cpp
@@ -15,8 +15,8 @@
 #include "js/Stream.h"
 
 using builtins::web::streams::NativeStreamSource;
+using fastly::FastlyGetErrorMessage;
 using fastly::fastly::convertBodyInit;
-using fastly::fastly::FastlyGetErrorMessage;
 using fastly::fetch::RequestOrResponse;
 
 namespace fastly::body {
diff --git a/runtime/fastly/builtins/cache-override.cpp b/runtime/fastly/builtins/cache-override.cpp
@@ -13,7 +13,7 @@
 #include "host_api.h"
 
 using builtins::BuiltinImpl;
-using fastly::fastly::FastlyGetErrorMessage;
+using fastly::FastlyGetErrorMessage;
 
 namespace fastly::cache_override {
 
diff --git a/runtime/fastly/builtins/cache-simple.cpp b/runtime/fastly/builtins/cache-simple.cpp
@@ -13,8 +13,8 @@
 
 using builtins::BuiltinNoConstructor;
 using builtins::web::streams::NativeStreamSource;
+using fastly::FastlyGetErrorMessage;
 using fastly::fastly::convertBodyInit;
-using fastly::fastly::FastlyGetErrorMessage;
 using fastly::fetch::RequestOrResponse;
 
 namespace fastly::cache_simple {
diff --git a/runtime/fastly/builtins/config-store.cpp b/runtime/fastly/builtins/config-store.cpp
@@ -4,7 +4,7 @@
 #include "fastly.h"
 
 using builtins::BuiltinImpl;
-using fastly::fastly::FastlyGetErrorMessage;
+using fastly::FastlyGetErrorMessage;
 
 namespace fastly::config_store {
 
diff --git a/runtime/fastly/builtins/dictionary.cpp b/runtime/fastly/builtins/dictionary.cpp
@@ -3,7 +3,7 @@
 #include "../host-api/host_api_fastly.h"
 #include "fastly.h"
 
-using fastly::fastly::FastlyGetErrorMessage;
+using fastly::FastlyGetErrorMessage;
 
 namespace fastly::dictionary {
 
diff --git a/runtime/fastly/builtins/fastly.cpp b/runtime/fastly/builtins/fastly.cpp
@@ -11,6 +11,7 @@
 #include "fastly.h"
 #include "js/Conversions.h"
 #include "js/JSON.h"
+#include "kv-store.h"
 #include "logger.h"
 #include <arpa/inet.h>
 
@@ -38,14 +39,6 @@ bool debug_logging_enabled() { return DEBUG_LOGGING_ENABLED; }
 
 namespace fastly::fastly {
 
-const JSErrorFormatString *FastlyGetErrorMessage(void *userRef, unsigned errorNumber) {
-  if (errorNumber > 0 && errorNumber < JSErrNum_Limit) {
-    return &fastly_ErrorFormatString[errorNumber];
-  }
-
-  return nullptr;
-}
-
 JS::PersistentRooted<JSObject *> Fastly::env;
 JS::PersistentRooted<JSObject *> Fastly::baseURL;
 JS::PersistentRooted<JSString *> Fastly::defaultBackend;
@@ -57,6 +50,26 @@ bool Fastly::dump(JSContext *cx, unsigned argc, JS::Value *vp) {
   if (!args.requireAtLeast(cx, __func__, 1))
     return false;
 
+// special debug mode operations
+#ifndef NDEBUG
+  if (args.get(0).isNull() && args.get(1).isString()) {
+    JS::RootedString str(cx, args.get(1).toString());
+    auto str_chars = core::encode(cx, str);
+    if (!str_chars) {
+      return false;
+    }
+    if (!strcmp(str_chars.ptr.get(), "invalidkv")) {
+      host_api::HttpBody body(-1);
+      host_api::HostBytes metadata{};
+      // uint32_t gen = std::get<2>(res.unwrap());
+      JS::RootedObject entry(
+          cx, ::fastly::kv_store::KVStoreEntry::create(cx, body, std::move(metadata)));
+      args.rval().setObject(*entry);
+      return true;
+    }
+  }
+#endif
+
   ENGINE->dump_value(args[0], stdout);
 
   args.rval().setUndefined();
diff --git a/runtime/fastly/builtins/fastly.h b/runtime/fastly/builtins/fastly.h
@@ -26,8 +26,6 @@ class Env : public builtins::BuiltinNoConstructor<Env> {
   static JSObject *create(JSContext *cx);
 };
 
-const JSErrorFormatString *FastlyGetErrorMessage(void *userRef, unsigned errorNumber);
-
 class Fastly : public builtins::BuiltinNoConstructor<Fastly> {
 private:
   static bool log(JSContext *cx, unsigned argc, JS::Value *vp);
diff --git a/runtime/fastly/builtins/fetch/fetch.cpp b/runtime/fastly/builtins/fetch/fetch.cpp
@@ -11,7 +11,7 @@ using fastly::backend::Backend;
 using fastly::fastly::Fastly;
 using fastly::fetch::Request;
 
-using fastly::fastly::FastlyGetErrorMessage;
+using fastly::FastlyGetErrorMessage;
 
 namespace fastly::fetch {
 
diff --git a/runtime/fastly/builtins/fetch/request-response.cpp b/runtime/fastly/builtins/fetch/request-response.cpp
@@ -55,11 +55,11 @@ using builtins::web::streams::TransformStream;
 using builtins::web::url::URL;
 using builtins::web::url::URLSearchParams;
 using builtins::web::worker_location::WorkerLocation;
+using fastly::FastlyGetErrorMessage;
 using fastly::backend::Backend;
 using fastly::cache_core::CacheEntry;
 using fastly::cache_override::CacheOverride;
 using fastly::cache_simple::SimpleCacheEntry;
-using fastly::fastly::FastlyGetErrorMessage;
 using fastly::fetch_event::FetchEvent;
 using fastly::kv_store::KVStoreEntry;
 
diff --git a/runtime/fastly/builtins/kv-store.cpp b/runtime/fastly/builtins/kv-store.cpp
@@ -27,10 +27,10 @@
 #include "kv-store.h"
 
 using builtins::web::streams::NativeStreamSource;
+using fastly::FastlyGetErrorMessage;
 using fastly::common::parse_and_validate_timeout;
 using fastly::common::validate_bytes;
 using fastly::fastly::convertBodyInit;
-using fastly::fastly::FastlyGetErrorMessage;
 using fastly::fetch::RequestOrResponse;
 
 namespace fastly::kv_store {
@@ -248,9 +248,7 @@ bool process_pending_kv_store_list(JSContext *cx, host_api::KVStorePendingList::
 
   auto res = pending_list.wait();
   if (auto *err = res.to_err()) {
-    std::string message = std::move(err->message()).value_or("when attempting to fetch resource.");
-    JS_ReportErrorNumberASCII(cx, FastlyGetErrorMessage, nullptr, JSMSG_KV_STORE_LIST_ERROR,
-                              message.c_str());
+    HANDLE_KV_ERROR(cx, *err, JSMSG_KV_STORE_LIST_ERROR);
     return RejectPromiseWithPendingError(cx, promise);
   }
 
@@ -314,9 +312,7 @@ bool process_pending_kv_store_delete(JSContext *cx, host_api::KVStorePendingDele
 
   auto res = pending_delete.wait();
   if (auto *err = res.to_err()) {
-    std::string message = std::move(err->message()).value_or("when attempting to fetch resource.");
-    JS_ReportErrorNumberASCII(cx, FastlyGetErrorMessage, nullptr, JSMSG_KV_STORE_DELETE_ERROR,
-                              message.c_str());
+    HANDLE_KV_ERROR(cx, *err, JSMSG_KV_STORE_DELETE_ERROR);
     return RejectPromiseWithPendingError(cx, promise);
   }
 
@@ -331,9 +327,7 @@ bool process_pending_kv_store_lookup(JSContext *cx, host_api::KVStorePendingLook
   auto res = pending_lookup.wait();
 
   if (auto *err = res.to_err()) {
-    std::string message = std::move(err->message()).value_or("when attempting to fetch resource.");
-    JS_ReportErrorNumberASCII(cx, FastlyGetErrorMessage, nullptr, JSMSG_KV_STORE_LOOKUP_ERROR,
-                              message.c_str());
+    HANDLE_KV_ERROR(cx, *err, JSMSG_KV_STORE_LOOKUP_ERROR);
     return RejectPromiseWithPendingError(cx, promise);
   }
 
@@ -637,10 +631,7 @@ bool KVStore::put(JSContext *cx, unsigned argc, JS::Value *vp) {
 
     auto res = pending_insert.wait();
     if (auto *err = res.to_err()) {
-      std::string message =
-          std::move(err->message()).value_or("when attempting to fetch resource.");
-      JS_ReportErrorNumberASCII(cx, FastlyGetErrorMessage, nullptr, JSMSG_KV_STORE_LIST_ERROR,
-                                message.c_str());
+      HANDLE_KV_ERROR(cx, *err, JSMSG_KV_STORE_INSERT_ERROR);
       return RejectPromiseWithPendingError(cx, result_promise);
     }
 
diff --git a/runtime/fastly/builtins/kv-store.h b/runtime/fastly/builtins/kv-store.h
@@ -5,6 +5,9 @@
 #include "./fetch/request-response.h"
 #include "builtin.h"
 
+#define HANDLE_KV_ERROR(cx, err, err_type)                                                         \
+  ::host_api::handle_kv_error(cx, err, err_type, __LINE__, __func__)
+
 namespace fastly::kv_store {
 
 class KVStoreEntry final : public builtins::BuiltinImpl<KVStoreEntry> {
diff --git a/runtime/fastly/builtins/secret-store.cpp b/runtime/fastly/builtins/secret-store.cpp
@@ -4,8 +4,8 @@
 #include "../host-api/host_api_fastly.h"
 #include "fastly.h"
 
+using fastly::FastlyGetErrorMessage;
 using fastly::common::validate_bytes;
-using fastly::fastly::FastlyGetErrorMessage;
 
 namespace fastly::secret_store {
 
diff --git a/runtime/fastly/common/validations.cpp b/runtime/fastly/common/validations.cpp
@@ -4,7 +4,7 @@
 #include "../host-api/host_api_fastly.h"
 #include "encode.h"
 
-using fastly::fastly::FastlyGetErrorMessage;
+using fastly::FastlyGetErrorMessage;
 
 namespace fastly::common {
 std::optional<uint32_t> parse_and_validate_timeout(JSContext *cx, JS::HandleValue value,
diff --git a/runtime/fastly/host-api/host_api.cpp b/runtime/fastly/host-api/host_api.cpp
@@ -753,42 +753,45 @@ make_fastly_send_error(fastly::fastly_host_http_send_error_detail &send_error_de
   return res;
 }
 
-FastlyKVError make_fastly_kv_error(fastly::fastly_kv_error kv_error) {
+FastlyKVError make_fastly_kv_error(fastly::fastly_kv_error kv_error,
+                                   fastly::fastly_host_error host_err) {
   FastlyKVError err;
   switch (kv_error) {
   case KV_ERROR_BAD_REQUEST: {
     err.detail = FastlyKVError::detail::bad_request;
-    break;
+    return err;
   }
   case KV_ERROR_INTERNAL_ERROR: {
     err.detail = FastlyKVError::detail::internal_error;
-    break;
+    return err;
   }
   case KV_ERROR_NOT_FOUND: {
     err.detail = FastlyKVError::detail::not_found;
-    break;
+    return err;
   }
   case KV_ERROR_OK: {
     err.detail = FastlyKVError::detail::ok;
-    break;
+    return err;
   }
   case KV_ERROR_PAYLOAD_TOO_LARGE: {
     err.detail = FastlyKVError::detail::payload_too_large;
-    break;
+    return err;
   }
   case KV_ERROR_PRECONDITION_FAILED: {
     err.detail = FastlyKVError::detail::precondition_failed;
-    break;
+    return err;
   }
   case KV_ERROR_TOO_MANY_REQUESTS: {
     err.detail = FastlyKVError::detail::too_many_requests;
-    break;
+    return err;
   }
   case KV_ERROR_UNINITIALIZED: {
     err.detail = FastlyKVError::detail::uninitialized;
-    break;
+    return err;
   }
   }
+  err.detail = FastlyKVError::detail::host_error;
+  err.host_err = host_err;
   return err;
 }
 
@@ -2523,8 +2526,9 @@ const std::optional<std::string> FastlyKVError::message() const {
   /// The kv-error-detail struct has not been populated.
   case uninitialized:
     return "Uninitialized.";
-  /// There was no kv error.
+  /// Host error / no error
   case ok:
+  case host_error:
     return std::nullopt;
   /// Bad request.
   case bad_request:
@@ -3213,11 +3217,11 @@ FastlyResult<HttpBody, FastlyKVError> KVStorePendingList::wait() {
 
   fastly::fastly_host_error err;
   HttpBody body{};
-  fastly::fastly_kv_error kv_err;
+  fastly::fastly_kv_error kv_err = KV_ERROR_UNINITIALIZED;
 
   if (!convert_result(fastly::kv_store_list_wait(this->handle, &body.handle, &kv_err), &err) ||
       kv_err != KV_ERROR_OK || body.handle == INVALID_HANDLE) {
-    res.emplace_err(make_fastly_kv_error(kv_err));
+    res.emplace_err(make_fastly_kv_error(kv_err, err));
   } else {
     res.emplace(body);
   }
@@ -3262,7 +3266,7 @@ KVStorePendingLookup::wait() {
                       &err) ||
       ((kv_err != KV_ERROR_OK || body.handle == INVALID_HANDLE) && kv_err != KV_ERROR_NOT_FOUND)) {
     cabi_free(metadata_buf);
-    res.emplace_err(make_fastly_kv_error(kv_err));
+    res.emplace_err(make_fastly_kv_error(kv_err, err));
   } else if (kv_err == KV_ERROR_NOT_FOUND) {
     cabi_free(metadata_buf);
     res.emplace(std::nullopt);
@@ -3301,10 +3305,10 @@ Result<KVStorePendingDelete::Handle> KVStore::delete_(std::string_view key) {
 FastlyResult<Void, FastlyKVError> KVStorePendingDelete::wait() {
   FastlyResult<Void, FastlyKVError> res;
   fastly::fastly_host_error err;
-  fastly::fastly_kv_error kv_err;
+  fastly::fastly_kv_error kv_err = KV_ERROR_UNINITIALIZED;
   if (!convert_result(fastly::kv_store_delete_wait(this->handle, &kv_err), &err) ||
       kv_err != KV_ERROR_OK) {
-    res.emplace_err(make_fastly_kv_error(kv_err));
+    res.emplace_err(make_fastly_kv_error(kv_err, err));
   }
   return res;
 }
@@ -3371,10 +3375,10 @@ KVStore::insert(std::string_view key, HttpBody body, std::optional<InsertMode> m
 FastlyResult<Void, FastlyKVError> KVStorePendingInsert::wait() {
   FastlyResult<Void, FastlyKVError> res;
   fastly::fastly_host_error err;
-  fastly::fastly_kv_error kv_err;
+  fastly::fastly_kv_error kv_err = KV_ERROR_UNINITIALIZED;
   if (!convert_result(fastly::kv_store_insert_wait(this->handle, &kv_err), &err) ||
       kv_err != KV_ERROR_OK) {
-    res.emplace_err(make_fastly_kv_error(kv_err));
+    res.emplace_err(make_fastly_kv_error(kv_err, err));
   }
   return res;
 }
diff --git a/runtime/fastly/host-api/host_api_fastly.h b/runtime/fastly/host-api/host_api_fastly.h
diff --git a/runtime/fastly/host-api/host_call.cpp b/runtime/fastly/host-api/host_call.cpp

Original file line number	Diff line number	Diff line change
`@@ -147,6 +147,7 @@`
`147`	`147`	`"body": "hello world!"`
`148`	`148`	`}`
`149`	`149`	`},`
	`150`	`+ "GET /kv-store/debug-error": {},`
`150`	`151`	`"GET /kv-store/delete/called-as-constructor": {},`
`151`	`152`	`"GET /kv-store/delete/called-unbound": {},`
`152`	`153`	`"GET /kv-store/delete/key-parameter-calls-7.1.17-ToString": {},`