From 2bdbe17c6230c0c5ea7781ae412ceb323b6e00d9 Mon Sep 17 00:00:00 2001 From: Justin Mazzola Paluska Date: Thu, 26 Oct 2023 15:51:06 -0400 Subject: [PATCH] Log actor id when V8 deserialization fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/workerd/api/BUILD.bazel | 9 ++ .../api/actor-state-iocontext-test.c++ | 96 +++++++++++++++++++ src/workerd/api/actor-state.c++ | 22 ++++- 3 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 src/workerd/api/actor-state-iocontext-test.c++ diff --git a/src/workerd/api/BUILD.bazel b/src/workerd/api/BUILD.bazel index c3d995592b5..4d42593d5da 100644 --- a/src/workerd/api/BUILD.bazel +++ b/src/workerd/api/BUILD.bazel @@ -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"], diff --git a/src/workerd/api/actor-state-iocontext-test.c++ b/src/workerd/api/actor-state-iocontext-test.c++ new file mode 100644 index 00000000000..f69e87daf36 --- /dev/null +++ b/src/workerd/api/actor-state-iocontext-test.c++ @@ -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 + +#include +#include + +#include +#include + +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 getName() const override { + return kj::none; + } + + bool equals(const ActorId& other) const override { + return false; + } + + kj::Own clone() const override { + return kj::heap(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 { + 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 mockActorId = kj::heap(kj::str("testActorId")); + Worker::Actor::Id id = kj::mv(mockActorId); + TestFixture fixture(TestFixture::SetupParams{ + .actorId = kj::mv(id), + }); + runBadDeserializationInIoContext(fixture, "actorId = MockActorId;"_kj); +} + +} // namespace +} // namespace workerd::api diff --git a/src/workerd/api/actor-state.c++ b/src/workerd/api/actor-state.c++ index 0c4e0964718..58c95af9b97 100644 --- a/src/workerd/api/actor-state.c++ +++ b/src/workerd/api/actor-state.c++ @@ -210,6 +210,25 @@ kj::Promise 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 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) { + return actorId->toString(); + } + } + KJ_UNREACHABLE; + } + } + return kj::none; +} + } // namespace jsg::Promise> DurableObjectStorageOperations::get( @@ -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(3), buf.size()))); }); } catch (jsg::JsExceptionThrown&) {