Skip to content

Commit

Permalink
SQLite: Make foreign key errors on txn commit visible to app.
Browse files Browse the repository at this point in the history
I noticed the test was testing for an "internal error", which is always a bug, and implies that in prod an error would have been logged to Sentry. We can make this throw an application-visible error instead. This also makes the test output look less bad.
  • Loading branch information
kentonv committed Oct 15, 2024
1 parent c3b0c07 commit 8eea488
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/workerd/api/sql-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1379,7 +1379,7 @@ export default {
// Test defer_foreign_keys (explodes the DO)
await assert.rejects(async () => {
await doReq('sql-test-foreign-keys');
}, /Error: internal error/);
}, /constraints were violated: FOREIGN KEY constraint failed: SQLITE_CONSTRAINT/);

// Some increments.
assert.equal(await doReq('increment'), 1);
Expand Down
3 changes: 3 additions & 0 deletions src/workerd/io/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ wd_cc_library(
"@capnp-cpp//src/capnp:capnp-rpc",
"@capnp-cpp//src/kj:kj-async",
],
implementation_deps = [
"@sqlite3",
],
)

wd_cc_library(
Expand Down
16 changes: 16 additions & 0 deletions src/workerd/io/actor-sqlite.c++
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <workerd/util/sentry.h>

#include <algorithm>
#include <sqlite3.h>

namespace workerd {

Expand Down Expand Up @@ -648,6 +649,21 @@ kj::Maybe<kj::Promise<void>> ActorSqlite::onNoPendingFlush() {
return outputGate.wait();
}

void ActorSqlite::TxnCommitRegulator::onError(
kj::Maybe<int> sqliteErrorCode, kj::StringPtr message) const {
KJ_IF_SOME(c, sqliteErrorCode) {
if (c == SQLITE_CONSTRAINT) {
JSG_ASSERT(false, Error,
"Durable Object was reset and rolled back to its last known good state because the "
"application left the database in a state where constraints were violated: ",
message);
}
}

// For any other type of error, fall back to the default behavior (throwing a non-JSG exception)
// as we don't know for sure that the problem is the application's fault.
}

const ActorSqlite::Hooks ActorSqlite::Hooks::DEFAULT = ActorSqlite::Hooks{};

kj::Promise<void> ActorSqlite::Hooks::scheduleRun(kj::Maybe<kj::Date> newAlarmTime) {
Expand Down
10 changes: 9 additions & 1 deletion src/workerd/io/actor-sqlite.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,16 @@ class ActorSqlite final: public ActorCacheInterface, private kj::TaskSet::ErrorH
SqliteKv kv;
SqliteMetadata metadata;

// Define a SqliteDatabase::Regulator that is similar to TRUSTED but turns certain SQLite errors
// into application errors as appropriate when committing an implicit transaction.
class TxnCommitRegulator: public SqliteDatabase::Regulator {
public:
void onError(kj::Maybe<int> sqliteErrorCode, kj::StringPtr message) const;
};
static constexpr TxnCommitRegulator TRUSTED_TXN_COMMIT;

SqliteDatabase::Statement beginTxn = db->prepare("BEGIN TRANSACTION");
SqliteDatabase::Statement commitTxn = db->prepare("COMMIT TRANSACTION");
SqliteDatabase::Statement commitTxn = db->prepare(TRUSTED_TXN_COMMIT, "COMMIT TRANSACTION");

kj::Maybe<kj::Exception> broken;

Expand Down

0 comments on commit 8eea488

Please sign in to comment.