Skip to content

Commit

Permalink
Log actor id when V8 deserialization fails
Browse files Browse the repository at this point in the history
When V8 deserialization fails, it’s likely due to corruption somewhere
between the Actor in JS and the storage layer.  In order to aid
debugging, log the actor id if we know of one.
  • Loading branch information
justin-mp committed Oct 27, 2023
1 parent 3a33f04 commit 2bdbe17
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 1 deletion.
9 changes: 9 additions & 0 deletions src/workerd/api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,20 @@ wd_cc_capnp_library(
["**/*-test.c++"],
exclude = [
"api-rtti-test.c++",
"actor-state-iocontext-test.c++",
"cf-property-test.c++",
"node/*-test.c++",
],
)]

kj_test(
src = "actor-state-iocontext-test.c++",
deps = [
"//src/workerd/io",
"//src/workerd/tests:test-fixture",
]
)

kj_test(
src = "node/buffer-test.c++",
deps = ["//src/workerd/tests:test-fixture"],
Expand Down
96 changes: 96 additions & 0 deletions src/workerd/api/actor-state-iocontext-test.c++
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// Copyright (c) 2017-2023 Cloudflare, Inc.
// Licensed under the Apache 2.0 license found in the LICENSE file or at:
// https://opensource.org/licenses/Apache-2.0

#include <algorithm>

#include <kj/test.h>
#include <kj/encoding.h>

#include <workerd/api/actor-state.h>
#include <workerd/tests/test-fixture.h>

namespace workerd::api {
namespace {

using workerd::TestFixture;

bool contains(kj::StringPtr haystack, kj::StringPtr needle) {
return std::search(haystack.begin(), haystack.end(), needle.begin(), needle.end())
!= haystack.end();
}

class MockActorId : public ActorIdFactory::ActorId {
public:
MockActorId(kj::String id) : id(kj::mv(id)) {}
kj::String toString() const override {
return kj::str("MockActorId<", id, ">");
}

kj::Maybe<kj::StringPtr> getName() const override {
return kj::none;
}

bool equals(const ActorId& other) const override {
return false;
}

kj::Own<ActorId> clone() const override {
return kj::heap<MockActorId>(kj::heapString(id));
}

virtual ~MockActorId() {};
private:
kj::String id;
};

void runBadDeserialization(jsg::Lock& lock, kj::StringPtr expectedId) {
// FF = kVersion token, 0E = version 15, 06 = an unknown tag value
kj::StringPtr invalidV8Hex = "FF0E06"_kj;
auto invalidV8Value = kj::decodeHex(invalidV8Hex.asArray());
try {
deserializeV8Value(lock, "some-key"_kj, invalidV8Value);
KJ_FAIL_ASSERT("deserializeV8Value should have failed.");
} catch (kj::Exception& ex) {
if (ex.getDescription().startsWith("actor storage deserialization failed")) {
KJ_ASSERT(contains(ex.getDescription(), expectedId));
} else {
throw;
}
}
}

void runBadDeserializationInIoContext(TestFixture& fixture, kj::StringPtr expectedId) {
fixture.runInIoContext(
[expectedId](const workerd::TestFixture::Environment& env) -> kj::Promise<void> {
runBadDeserialization(env.lock, expectedId);
return kj::READY_NOW;
});
}

// TODO(maybe) It would be nice to have a test that tests the case when there's no IoContext,
// but that's a royal pain to set up in this test file we'd basically only test that we don't
// crash, which the actor-state-test.c++ does for us.

KJ_TEST("no actor specified") {
TestFixture fixture;
runBadDeserializationInIoContext(fixture, "actorId = ;"_kj);
}

KJ_TEST("actor specified with string id") {
Worker::Actor::Id id = kj::str("testActorId");
TestFixture fixture(TestFixture::SetupParams{.actorId = kj::mv(id)});
runBadDeserializationInIoContext(fixture, "actorId = testActorId;"_kj);
}

KJ_TEST("actor specified with ActorId object") {
kj::Own<ActorIdFactory::ActorId> mockActorId = kj::heap<MockActorId>(kj::str("testActorId"));
Worker::Actor::Id id = kj::mv(mockActorId);
TestFixture fixture(TestFixture::SetupParams{
.actorId = kj::mv(id),
});
runBadDeserializationInIoContext(fixture, "actorId = MockActorId<testActorId>;"_kj);
}

} // namespace
} // namespace workerd::api
22 changes: 21 additions & 1 deletion src/workerd/api/actor-state.c++
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,25 @@ kj::Promise<void> updateStorageDeletes(IoContext& context,
metrics.addStorageDeletes(deleted);
};

// Return the id of the current actor (or the empty string if there is no current actor).
kj::Maybe<kj::String> getCurrentActorId() {
if (IoContext::hasCurrent()) {
IoContext& ioContext = IoContext::current();
KJ_IF_SOME(actor, ioContext.getActor()) {
KJ_SWITCH_ONEOF(actor.getId()) {
KJ_CASE_ONEOF(s, kj::String) {
return kj::heapString(s);
}
KJ_CASE_ONEOF(actorId, kj::Own<ActorIdFactory::ActorId>) {
return actorId->toString();
}
}
KJ_UNREACHABLE;
}
}
return kj::none;
}

} // namespace

jsg::Promise<jsg::JsRef<jsg::JsValue>> DurableObjectStorageOperations::get(
Expand Down Expand Up @@ -959,9 +978,10 @@ jsg::JsValue deserializeV8Value(jsg::Lock& js,
// include the key (to help find the data in the database if it hasn't been deleted), the
// length of the value, and the first three bytes of the value (which is just the v8-internal
// version header and the tag that indicates the type of the value, but not its contents).
kj::String actorId = getCurrentActorId().orDefault([]() { return kj::str(); });
KJ_FAIL_ASSERT("actor storage deserialization failed",
"failed to deserialize stored value",
exception.getHandle(js), key, buf.size(),
actorId, exception.getHandle(js), key, buf.size(),
buf.slice(0, std::min(static_cast<size_t>(3), buf.size())));
});
} catch (jsg::JsExceptionThrown&) {
Expand Down

0 comments on commit 2bdbe17

Please sign in to comment.