Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sqlite-backed DOs: Allow calling deleteAll() inside an alarm handler #3018

Merged
merged 3 commits into from
Nov 1, 2024
Merged

Sqlite-backed DOs: Allow calling deleteAll() inside an alarm handler #3018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c61da9
SRS actors: Ensure that deleteAll() always has a commit scheduled dur…
jclee Oct 25, 2024
01e81ee
SRS actors: Add a little more test coverage for repeat deleteAll() in…
jclee Oct 25, 2024
25fa631
SRS actors: Don't attempt alarm deletion at end of handler if the out…
jclee Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 135 additions & 22 deletions src/workerd/io/actor-sqlite-test.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,13 @@ struct ActorSqliteTest final {
db(vfs, kj::Path({"foo"}), kj::WriteMode::CREATE | kj::WriteMode::MODIFY),
actor(kj::attachRef(db), gate, KJ_BIND_METHOD(*this, commitCallback), hooks),
gateBrokenPromise(options.monitorOutputGate ? eagerlyReportExceptions(gate.onBroken())
: kj::Promise<void>(kj::READY_NOW)) {}
: kj::Promise<void>(kj::READY_NOW)) {
db.afterReset([this](SqliteDatabase& cbDb) {
KJ_ASSERT(&db == &cbDb);
KJ_ASSERT(actor.isCommitScheduled(),
"actor should have a commit scheduled during afterReset() callback");
});
}

~ActorSqliteTest() noexcept(false) {
if (!unwindDetector.isUnwinding()) {
Expand Down Expand Up @@ -625,6 +631,52 @@ KJ_TEST("canceling deferred alarm deletion is idempotent") {
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
}

KJ_TEST("alarm handler cleanup succeeds when output gate is broken") {
auto runWithSetup = [](auto testFunc) {
ActorSqliteTest test({.monitorOutputGate = false});
auto promise = test.gate.onBroken();

// Initialize alarm state to 1ms.
test.setAlarm(oneMs);
test.pollAndExpectCalls({"scheduleRun(1ms)"})[0]->fulfill();
test.pollAndExpectCalls({"commit"})[0]->fulfill();
test.pollAndExpectCalls({});
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

auto armResult = test.actor.armAlarmHandler(oneMs, false);
KJ_ASSERT(armResult.is<ActorSqlite::RunAlarmHandler>());
auto deferredDelete = kj::mv(armResult.get<ActorSqlite::RunAlarmHandler>().deferredDelete);

// Break gate
test.put("foo", "bar");
test.pollAndExpectCalls({"commit"})[0]->reject(KJ_EXCEPTION(FAILED, "a_rejected_commit"));
KJ_EXPECT_THROW_MESSAGE("a_rejected_commit", promise.wait(test.ws));
// Ensure taskFailed handler runs and notices brokenness:
test.ws.poll();

testFunc(test, kj::mv(deferredDelete));
};

// Here, we test that the deferred deleter destructor doesn't throw, both in the case when the
// caller cancels deletion and when it does not cancel it:

runWithSetup([](ActorSqliteTest& test, kj::Own<void> deferredDelete) {
// In the case where the handler fails, we assume the caller will explicitly cancel deferred
// alarm deletion:
test.actor.cancelDeferredAlarmDeletion();

// Dropping the DeferredAlarmDeleter should succeed -- that is, it should not throw here:
{ auto drop = kj::mv(deferredDelete); }
});

runWithSetup([](ActorSqliteTest& test, kj::Own<void> deferredDelete) {
// In the case where the handler succeeds, the caller will not cancel deferred deletion before
// dropping the DeferredAlarmDeleter. Dropping the DeferredAlarmDeleter should still succeed,
// even if the output gate happens to already be broken:
{ auto drop = kj::mv(deferredDelete); }
});
}

KJ_TEST("handler alarm is not deleted when commit fails") {
ActorSqliteTest test({.monitorOutputGate = false});

Expand Down Expand Up @@ -924,19 +976,48 @@ KJ_TEST("calling deleteAll() preserves alarm state if alarm is set") {
test.pollAndExpectCalls({});
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

ActorCache::DeleteAllResults results = test.actor.deleteAll({});
KJ_ASSERT(results.backpressure == kj::none);
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
{
KJ_ASSERT(!test.actor.isCommitScheduled());
ActorCache::DeleteAllResults results = test.actor.deleteAll({});
KJ_ASSERT(test.actor.isCommitScheduled());
KJ_ASSERT(results.backpressure == kj::none);
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
KJ_ASSERT(results.count.wait(test.ws) == 0);
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own education, we only expect to see scheduleRun(1ms) when setAlarm is called, and not on subsequent resets? Presumably, that's because the scheduled run gets scheduled outside of this deletion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much, yeah. The scheduleRun() hook is called by ActorSqlite as part of the committing logic -- implemented in startPrecommitAlarmScheduling() and commitImpl() -- to request a change to the alarm manager's scheduled alarm notification time. If ActorSqlite knows the currently scheduled alarm doesn't need to be updated, it doesn't call scheduleRun().

But thinking about this a bit more, I guess the point of confusion might be why we don't see a scheduleRun(kj::none) in the test corresponding to the db.reset() and a subsequent scheduleRun(1ms) corresponding to the alarm state reset? That's because we don't run the committing logic for the db.reset(), only for the state reset. And because we're restoring the original state, the committing logic sees no change, so scheduleRun() is not called.

I haven't yet implemented Kenton's suggestion of simplifying the deleteAll() logic by performing a redundant commit, but I'm pretty sure this won't change the situation, since the promise that deleteAll() queues calls commitCallback() directly, rather than through the alarm commit logic. And I'm pretty sure that's OK? But might need to think about it a bit more to be sure.

KJ_ASSERT(results.count.wait(test.ws) == 0);
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

commitFulfiller->fulfill();
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
commitFulfiller->fulfill();
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

test.pollAndExpectCalls({});
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
test.pollAndExpectCalls({});
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
}

{
// Should be fine to call deleteAll() a few times in succession, too:
KJ_ASSERT(!test.actor.isCommitScheduled());
ActorCache::DeleteAllResults results1 = test.actor.deleteAll({});
ActorCache::DeleteAllResults results2 = test.actor.deleteAll({});
KJ_ASSERT(test.actor.isCommitScheduled());
KJ_ASSERT(results1.backpressure == kj::none);
KJ_ASSERT(results2.backpressure == kj::none);
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

// Presumably fine to be performing the alarm state restoration after each db reset:
auto commitFulfillers = test.pollAndExpectCalls({"commit", "commit"});
KJ_ASSERT(results1.count.wait(test.ws) == 0);
KJ_ASSERT(results2.count.wait(test.ws) == 0);
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

commitFulfillers[0]->fulfill();
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
commitFulfillers[1]->fulfill();
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

test.pollAndExpectCalls({});
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);
}
}

KJ_TEST("calling deleteAll() preserves alarm state if alarm is not set") {
Expand All @@ -954,29 +1035,61 @@ KJ_TEST("calling deleteAll() preserves alarm state if alarm is not set") {
test.pollAndExpectCalls({});
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);

ActorCache::DeleteAllResults results = test.actor.deleteAll({});
KJ_ASSERT(results.backpressure == kj::none);
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);
{
KJ_ASSERT(!test.actor.isCommitScheduled());
ActorCache::DeleteAllResults results = test.actor.deleteAll({});
KJ_ASSERT(test.actor.isCommitScheduled());
KJ_ASSERT(results.backpressure == kj::none);
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);

auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
KJ_ASSERT(results.count.wait(test.ws) == 0);
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);
auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
KJ_ASSERT(results.count.wait(test.ws) == 0);
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);

commitFulfiller->fulfill();
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);
commitFulfiller->fulfill();
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);

// We can also assert that we leave the database empty, in case that turns out to be useful later:
auto q =
test.db.run("SELECT name FROM sqlite_master WHERE type='table' AND name='_cf_METADATA'");
KJ_ASSERT(q.isDone());
}

// We can also assert that we leave the database empty, in case that turns out to be useful later:
auto q = test.db.run("SELECT name FROM sqlite_master WHERE type='table' AND name='_cf_METADATA'");
KJ_ASSERT(q.isDone());
{
// Should be fine to call deleteAll() a few times in succession, too:
KJ_ASSERT(!test.actor.isCommitScheduled());
ActorCache::DeleteAllResults results1 = test.actor.deleteAll({});
ActorCache::DeleteAllResults results2 = test.actor.deleteAll({});
KJ_ASSERT(test.actor.isCommitScheduled());
KJ_ASSERT(results1.backpressure == kj::none);
KJ_ASSERT(results2.backpressure == kj::none);
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);

// Presumably fine that the deletion commits coalesce:
auto commitFulfiller = kj::mv(test.pollAndExpectCalls({"commit"})[0]);
KJ_ASSERT(results1.count.wait(test.ws) == 0);
KJ_ASSERT(results2.count.wait(test.ws) == 0);
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);

commitFulfiller->fulfill();
KJ_ASSERT(expectSync(test.getAlarm()) == kj::none);

auto q =
test.db.run("SELECT name FROM sqlite_master WHERE type='table' AND name='_cf_METADATA'");
KJ_ASSERT(q.isDone());
}
}

KJ_TEST("calling deleteAll() during an implicit transaction preserves alarm state") {
ActorSqliteTest test;

KJ_ASSERT(!test.actor.isCommitScheduled());

// Initialize alarm state to 1ms.
test.setAlarm(oneMs);

ActorCache::DeleteAllResults results = test.actor.deleteAll({});
KJ_ASSERT(test.actor.isCommitScheduled());
KJ_ASSERT(results.backpressure == kj::none);
KJ_ASSERT(expectSync(test.getAlarm()) == oneMs);

Expand Down
27 changes: 22 additions & 5 deletions src/workerd/io/actor-sqlite.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,17 @@ void ActorSqlite::maybeDeleteDeferredAlarm() {
inAlarmHandler = false;

if (haveDeferredDelete) {
metadata.setAlarm(kj::none);
// If we have reached this point, the client is destroying its DeferredAlarmDeleter at the end
// of an alarm handler run, and deletion hasn't been cancelled, indicating that the handler
// returned success.
//
// If the output gate has somehow broken in the interim, attempting to write the deletion here
// will cause the DeferredAlarmDeleter destructor to throw, which the caller probably isn't
// expecting. So we'll skip the deletion attempt, and let the caller detect the gate
// brokenness through other means.
if (broken == kj::none) {
metadata.setAlarm(kj::none);
}
haveDeferredDelete = false;
}
}
Expand Down Expand Up @@ -513,15 +523,22 @@ ActorCacheInterface::DeleteAllResults ActorSqlite::deleteAll(WriteOptions option
}
}

if (localAlarmState == kj::none && !deleteAllCommitScheduled) {
// If we're not going to perform a write to restore alarm state, we'll want to make sure the
// commit callback is called for the deleteAll().
if (!deleteAllCommitScheduled) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it isn't apparent: the main fix here is removal of the localAlarmState == kj::none check, to ensure deleteAllCommitScheduled is always true by the time we reach db.clear(), which in turn ensures that isCommitScheduled() is true, since it's used to check an invariant in a storage relay callback.

The localAlarmState == kj::none check was originally put in place to avoid having a redundant commit, since the write to restore the alarmState also adds a commit. But of course there's no way for the storage relay callback to know via the current isCommitScheduled() implementation that we're planning to add the commit after calling db.clear() (and maybe it isn't even correct to rely solely on committing after db.clear(), if it's important that the output lock is synchronously started before calling db.clear()?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think you're overestimating the harm of a "redundant commit". StorageRelayClient::commit() already detects if there's nothing to commit (if (!needCommit at the beginning), so it's not that big a deal to schedule an extra one here.

And deleteAll() is not commonly called so I don't think it's worth optimizing away some promises.

Let's instead simplify this code to just always schedule (and apply) the deleteAll commit.

Copy link
Contributor Author

@jclee jclee Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Sure, makes sense... I'll try to simplify it.

Copy link
Contributor Author

@jclee jclee Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Thinking about it a bit more while responding to Justin's comment... Is there a potential problem if we queue a commit task for the db.reset() and another (via onWrite() -> commitImpl()) for the alarm state reset? If the first commit task succeeds but the second somehow fails, wouldn't that leave the db alarm state empty, when the current expectation may be that the committed alarm state remains in place regardless of success or failure of the deleteAll() operation?

Or is it perhaps OK to assume the alarm state reset isn't crucial, if deleteAll() is being called?

Or, I suppose, is some small window of alarm state loss unavoidable, due to the nature of deleting and reinstating the sqlite database?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what you're getting at is that the reset commit task doesn't actually do the same set of work as a regular write commit -- it doesn't actually commit a transaction and whatnot. Yeah, I agree it would be a problem if we committed the reset without actually committing the transaction to restore the alarm time.

So perhaps the reset commit task needs to actually skip calling commitCallback in the case that it knows that a further commit is coming for the metadata. Perhaps it can just be like:

    commitTasks.add(outputGate.lockWhile(kj::evalLater([this]() mutable -> kj::Promise<void> {
      // Don't commit if shutdown() has been called.
      requireNotBroken();

      deleteAllCommitScheduled = false;
      if (currentTxn.is<ImplicitTxn>()) {
        // An implicit transaction is already scheduled, so we'll count on it to perform a commit when it's
        // done. This is particularly important for the case where deleteAll() was called while an alarm
        // is outstanding; we'll immediately begin an implicit transaction that restores the alarm. We don't
        // want to commit the deletion without that transaction.
        return kj::READY_NOW;
      } else {
        return commitCallback();
      }
    })));
    deleteAllCommitScheduled = true;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another, maybe cleaner approach would be to unify the two commit tasks into a single one which checks both currentTxn.is<ImplicitTxn>() and deleteAllCommitScheduled to decide what actual work needs to be performed before committing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also curious... I assume it's OK/moot that we call requireNotBroken() prior to clearing deleteAllCommitScheduled, since once the output gate is broken, we expect all subsequent user operations to fail and continue failing, such that the return value of isCommitScheduled() no longer matters?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah nothing matters once broken.

Copy link
Contributor Author

@jclee jclee Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried combining the deleteAll() and onWrite() commit tasks, but it was turning out to be a more complicated change than your suggestion of just checking if (currentTxn.is<ImplicitTxn*>()) {...} in deleteAll(), so I went with that.

One thing I wasn't quite sure of, though, is the case involving explicit transactions:

  • user calls deleteAll() while alarm state is present.
  • deleteAll() adds its kj::evalLater() commit task to the event queue.
  • deleteAll() calls db.reset() to clear the database.
  • deleteAll() calls setAlarm() to restore the post-delete alarm state, which fires onWrite(), which sets up the implicit transaction and adds its kj::evalLater() commit task to the event queue to run immediately after.
  • before the event queue processes either commit task, it runs an event where user code starts an explicit transaction.
  • the explicit transaction constructor runs the implicit transaction's commit(), then updates currentTxn to point to the explicit transaction.
  • the user code maybe makes a few additional db changes within the explicit transaction.

When the deleteAll() commit task runs, it will see that currentTxn is no longer an implicit transaction, so it's going to just run commitCallback(). Is that OK?

I assume the commitCallback() would end up persisting the interim state of the explicit transaction. But presumably that's OK, because if the system goes down at that point, we recover from that by rolling back any explicit transaction savepoints we see in the persisted DB when reloading? (If not, I think a similar sequence of events is already possible just using implicit and explicit transactions together, without calling deleteAll().)

And in this case, we'd also be running a commitCallback() that effectively updates alarm state without also running the alarm rescheduling commit code, but maybe that's OK, since the very next event queue item should be the implicit transaction's commit task, which would run it?

// Make sure a commit callback is queued for the deleteAll().
commitTasks.add(outputGate.lockWhile(kj::evalLater([this]() mutable -> kj::Promise<void> {
// Don't commit if shutdown() has been called.
requireNotBroken();

deleteAllCommitScheduled = false;
return commitCallback();
if (currentTxn.is<ImplicitTxn*>()) {
// An implicit transaction is already scheduled, so we'll count on it to perform a commit when it's
// done. This is particularly important for the case where deleteAll() was called while an alarm
// is outstanding; resetting the alarm state (below) starts an implicit transaction.
// We don't want to commit the deletion without that transaction.
return kj::READY_NOW;
} else {
return commitCallback();
}
})));
deleteAllCommitScheduled = true;
}
Expand Down