Merge pull request #7514 from cloudflare/jolio/sqlite-suppo-ufummb

Sqlite: support adding specific error context to internal exceptions

Author: jtolio

src/workerd/util/sqlite-test.c++

@@ -1093,6 +1093,35 @@ KJ_TEST("SQLite extended error codes in messages") {
   }
 }
 
+KJ_TEST("SQLite error context is appended to internal errors only") {
+  auto dir = kj::newInMemoryDirectory(kj::nullClock());
+  SqliteDatabase::Vfs vfs(*dir);
+  SqliteDatabase db(vfs, kj::Path({"foo"}), kj::WriteMode::CREATE | kj::WriteMode::MODIFY);
+  db.setErrorContext(kj::str("actorId = abc123"));
+
+  db.run("CREATE TABLE things (id INTEGER PRIMARY KEY)");
+  db.run("INSERT INTO things VALUES (1)");
+
+  // Failures while preparing and while stepping both carry the context.
+  KJ_EXPECT_THROW_MESSAGE("no such table: nonexistent: SQLITE_ERROR; actorId = abc123",
+      db.run("SELECT * FROM nonexistent"));
+  KJ_EXPECT_THROW_MESSAGE(
+      "SQLITE_CONSTRAINT_PRIMARYKEY); actorId = abc123", db.run("INSERT INTO things VALUES (1)"));
+
+  // Errors reported by the regulator do not.
+  class ReportingRegulator: public SqliteDatabase::Regulator {
+   public:
+    void onError(kj::Maybe<int> sqliteErrorCode, kj::StringPtr message) const override {
+      kj::throwFatalException(KJ_EXCEPTION(FAILED, "reported", message));
+    }
+  };
+  static ReportingRegulator regulator;
+  auto exception = KJ_ASSERT_NONNULL(kj::runCatchingExceptions(
+      [&]() { db.run({.regulator = regulator}, "SELECT * FROM nonexistent"); }));
+  KJ_EXPECT(exception.getDescription().contains("no such table: nonexistent"), exception);
+  KJ_EXPECT(!exception.getDescription().contains("abc123"), exception);
+}
+
 class MockRollbackCallback {
  public:
   kj::Function<void()> create() {
@@ -1704,10 +1733,11 @@ KJ_TEST("SQLite memory metering tracks allocations correctly") {
       "memory should decrease when running `PRAGMA shrink_memory`");
 }
 
-KJ_TEST("I/O exceptions pass through SQLite") {
+KJ_TEST("I/O exceptions pass through SQLite with the error context") {
   auto dir = kj::atomicRefcounted<ErrorInjectableDirectory>();
   SqliteDatabase::Vfs vfs(*dir);
   SqliteDatabase db(vfs, kj::Path({"db"}), kj::WriteMode::CREATE | kj::WriteMode::MODIFY);
+  db.setErrorContext(kj::str("actorId = abc123"));
 
   db.run({.regulator = SqliteDatabase::TRUSTED}, kj::str(R"(
     CREATE TABLE IF NOT EXISTS things (
@@ -1728,9 +1758,16 @@ KJ_TEST("I/O exceptions pass through SQLite") {
     INSERT INTO things(value) VALUES (456);
   )"));
   }));
-  KJ_EXPECT(exception.getDescription() == "test-vfs-error", exception);
+  KJ_EXPECT(exception.getDescription() == "test-vfs-error; actorId = abc123", exception);
   auto disposition = KJ_ASSERT_NONNULL(exception.getDetail(SENTRY_TAG_DETAIL_ID));
   KJ_EXPECT(disposition.asChars() == "NOSENTRY"_kj, exception);
+
+  // Application-visible exceptions pass through unchanged.
+  KJ_ASSERT_NONNULL(dir->dbFile)->error = KJ_EXCEPTION(FAILED, "jsg.Error: test-vfs-error");
+  auto tunneled = KJ_ASSERT_NONNULL(kj::runCatchingExceptions([&]() {
+    db.run({.regulator = SqliteDatabase::TRUSTED}, "INSERT INTO things(value) VALUES (789)");
+  }));
+  KJ_EXPECT(tunneled.getDescription() == "jsg.Error: test-vfs-error", tunneled);
 }
 
 void testCriticalError(const char* expectedErrorMessage,

src/workerd/util/sqlite.c++

@@ -6,6 +6,7 @@
 
 #include "strings.h"
 
+#include <workerd/jsg/exception.h>
 #include <workerd/util/autogate.h>
 #include <workerd/util/sentry.h>
 
@@ -196,7 +197,19 @@ void tagSentry(kj::Exception& e, kj::StringPtr tag) {
   }
 }
 
-[[noreturn]] void throwSentryException(kj::Exception&& e, kj::StringPtr tag) {
+// Applies the context given to `SqliteDatabase::setErrorContext()`, if any. Tunneled exceptions
+// are left untouched because their description is shown to the application; a VFS callback may
+// throw one (e.g. when a write exceeds a storage limit).
+void appendErrorContext(kj::Exception& e, kj::Maybe<kj::StringPtr> errorContext) {
+  if (jsg::isTunneledException(e.getDescription())) return;
+  KJ_IF_SOME(context, errorContext) {
+    e.setDescription(kj::str(e.getDescription(), "; ", context));
+  }
+}
+
+[[noreturn]] void throwSentryException(
+    kj::Exception&& e, kj::StringPtr tag, kj::Maybe<kj::StringPtr> errorContext) {
+  appendErrorContext(e, errorContext);
   tagSentry(e, tag);
   kj::throwFatalException(kj::mv(e));
 }
@@ -231,8 +244,9 @@ class SqliteCallScope {
     vfsErrorListener = nullptr;
   }
 
-  void rethrowVfsError() {
+  void rethrowVfsError(kj::Maybe<kj::StringPtr> errorContext) {
     KJ_IF_SOME(e, error) {
+      appendErrorContext(e, errorContext);
       // Slight hack: The exception already has a stack trace attached which should include the
       // current stack, but `kj::throwFatalException()` would re-append the current stack trace
       // to the exception. We can avoid that by calling
@@ -257,13 +271,14 @@ class SqliteCallScope {
 
 // Like KJ_REQUIRE() but give the Regulator a chance to report the error. `errorMessage` is either
 // the return value of sqlite3_errmsg() or a string literal containing a similarly
-// application-approriate error message. A reference called `regulator` must be in-scope.
+// application-approriate error message. A reference called `regulator` must be in-scope, as must
+// a `getErrorContext()` method (i.e. this must be used within SqliteDatabase or Query).
 // sqliteErrorCode is a kj::Maybe<int> and represents the error code from sqlite.
 #define SQLITE_REQUIRE_WITH_TAG(condition, sqliteErrorCode, sentryTag, errorMessage, ...)          \
   if (!(condition)) {                                                                              \
     regulator->onError(sqliteErrorCode, errorMessage);                                             \
-    throwSentryException(                                                                          \
-        KJ_EXCEPTION(FAILED, "SQLite failed", errorMessage, ##__VA_ARGS__), sentryTag);            \
+    throwSentryException(KJ_EXCEPTION(FAILED, "SQLite failed", errorMessage, ##__VA_ARGS__),       \
+        sentryTag, getErrorContext());                                                             \
   }
 
 #define SQLITE_REQUIRE(condition, sqliteErrorCode, errorMessage, ...)                              \
@@ -275,12 +290,12 @@ class SqliteCallScope {
   do {                                                                                             \
     SqliteCallScope sqliteCallScope;                                                               \
     int _ec = code;                                                                                \
-    if (_ec != SQLITE_OK) sqliteCallScope.rethrowVfsError();                                       \
+    if (_ec != SQLITE_OK) sqliteCallScope.rethrowVfsError(kj::none);                               \
     if (_ec != SQLITE_OK) {                                                                        \
       throwSentryException(                                                                        \
           KJ_EXCEPTION(                                                                            \
               FAILED, kj::str(sqlite3_errstr(_ec), ": ", namedErrorCode(_ec)), ##__VA_ARGS__),     \
-          "SENTRY_DO"_kj);                                                                         \
+          "SENTRY_DO"_kj, kj::none);                                                               \
     }                                                                                              \
   } while (false)
 
@@ -293,7 +308,7 @@ class SqliteCallScope {
     /* SQLITE_MISUSE doesn't put error info on the database object, so check it separately */      \
     KJ_ASSERT(_ec != SQLITE_MISUSE, "SQLite misused: " #code, ##__VA_ARGS__);                      \
     handleCriticalError(_ec, dbErrorMessage(_ec, db), sqliteCallScope.getException());             \
-    if (_ec == SQLITE_IOERR) sqliteCallScope.rethrowVfsError();                                    \
+    if (_ec == SQLITE_IOERR) sqliteCallScope.rethrowVfsError(getErrorContext());                   \
     SQLITE_REQUIRE(_ec == SQLITE_OK, _ec, dbErrorMessage(_ec, db), ##__VA_ARGS__);                 \
   } while (false)
 
@@ -306,7 +321,7 @@ class SqliteCallScope {
   do {                                                                                             \
     KJ_ASSERT(error != SQLITE_MISUSE, "SQLite misused: " code, ##__VA_ARGS__);                     \
     handleCriticalError(error, dbErrorMessage(error, db), sqliteCallScope.getException());         \
-    if (error == SQLITE_IOERR) sqliteCallScope.rethrowVfsError();                                  \
+    if (error == SQLITE_IOERR) sqliteCallScope.rethrowVfsError(getErrorContext());                 \
     SQLITE_REQUIRE_WITH_TAG(                                                                       \
         error != SQLITE_BUSY, error, "NOSENTRY"_kj, dbErrorMessage(error, db), ##__VA_ARGS__);     \
     SQLITE_REQUIRE(error == SQLITE_OK, error, dbErrorMessage(error, db), ##__VA_ARGS__);           \

src/workerd/util/sqlite.h

@@ -255,6 +255,14 @@ class SqliteDatabase {
     onCriticalErrorCallback = kj::mv(callback);
   }
 
+  // Identifies this database in internal error logs and Sentry reports, e.g. so that a
+  // SQLITE_IOERR, etc. can be traced back to the specific database file that produced
+  // it. `context` (e.g. "id = 1234") is appended as "; <context>" to the description of
+  // exceptions thrown.
+  void setErrorContext(kj::String context) {
+    errorContext = kj::mv(context);
+  }
+
   SqliteMemoryScope enterMemoryScope();
 
   // Returns true if a transaction was automatically rolled due to a critical error.
@@ -389,6 +397,7 @@ class SqliteDatabase {
   kj::Maybe<kj::Function<void(kj::StringPtr errorMessage, kj::Maybe<kj::Exception> maybeException)>>
       onCriticalErrorCallback;
   kj::Maybe<kj::Function<void(SqliteDatabase&)>> afterResetCallback;
+  kj::Maybe<kj::String> errorContext;
 
   kj::List<ResetListener, &ResetListener::link> resetListeners;
 
@@ -433,6 +442,10 @@ class SqliteDatabase {
       kj::StringPtr errorMessage,
       kj::Maybe<const kj::Exception&> exception);
 
+  kj::Maybe<kj::StringPtr> getErrorContext() {
+    return errorContext.map([](kj::String& s) -> kj::StringPtr { return s; });
+  }
+
   enum Multi { SINGLE, MULTI };
 
   // A pair of a compiled statement, and a description of the interesting state changes it applies.
@@ -769,6 +782,10 @@ class SqliteDatabase::Query final: private ResetListener {
     db.handleCriticalError(errorCode, errorMessage, maybeException);
   }
 
+  kj::Maybe<kj::StringPtr> getErrorContext() {
+    return db.getErrorContext();
+  }
+
   // Some reasonable automatic conversions.
 
   inline void bind(uint column, int value) {