Skip to content

event loop: tolerate backward monotonic clock jumps #369

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

Open
VijayabaskarR-06 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

event loop: tolerate backward monotonic clock jumps #369

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5ce6f8
event loop: tolerate backward monotonic clock jumps
VijayabaskarR-06 Oct 1, 2026
02767e1
test: cover backward monotonic clock recovery
VijayabaskarR-06 Oct 1, 2026
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
28 changes: 28 additions & 0 deletions src/mp/proxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,11 +286,39 @@ EventLoop::~EventLoop()
// cleanup.wait(m_io_context.waitScope);
}

namespace {
#if CAPNP_VERSION < 1002000

@ryanofsky ryanofsky Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

nit: I'd suggest dropping the #if CAPNP_VERSION < 1002000 around the ClockErrorCallback definition and keeping it only around the registration in loop(). That way the class still gets compiled with Cap'n Proto 1.2 and later, so if a refactoring breaks it, that shows up right away in every build instead of only later in olddeps or in builds against distro packages. The comment above the class could mention that it is only used with versions before 1.2, so it's still clear what to delete when the minimum version is raised.

//! Ignore "can't advance backwards in time" errors from Cap'n Proto versions
//! before 1.2, which raise them when CLOCK_MONOTONIC briefly goes backwards
//! (https://github.com/capnproto/capnproto/issues/2261). Returning lets
//! TimerImpl::advanceTo() keep the previous time, like newer versions do.
Comment on lines +291 to +294

@ryanofsky ryanofsky Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

Note for reviewers: The comment above says that returning from the callback lets TimerImpl::advanceTo() keep the previous time "like newer versions do". Here are the two versions of advanceTo() it compares, from Cap'n Proto's kj/timer.c++:

// Before 1.2 (non-Apple). When the callback returns, the { return; } block runs.
KJ_REQUIRE(newTime >= time, "can't advance backwards in time") { return; }
time = newTime;
for (;;) { /* fire timers with deadline <= time */ }

// 1.2 and later.
time = std::max(time, newTime);
for (;;) { /* fire timers with deadline <= time */ }

When the clock goes backwards, both leave time unchanged, so the comment is right about that. The difference is that 1.2 still runs the loop that fires timers due at or before time, while the old version returns before reaching it.

That doesn't matter in practice. Timers due at or before time normally already fired during the previous advanceTo() call, the one that set time. The only exception is a timer added after that call with a deadline at or before time, for example afterDelay(0). Version 1.2 fires it right away, and with this workaround it fires on the next advanceTo() call instead. That call comes soon: the event loop sets its wait timeout to the timer's deadline minus the current clock reading, and since the clock is behind by the size of the regression, the timeout is at most that size (microseconds in the reports in capnproto/capnproto#2261). Nothing to change here.

class ClockErrorCallback : public kj::ExceptionCallback

@ryanofsky ryanofsky Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

Note: Not for this PR, but in case anyone wants to pick it up, I think the stack check that caused the ASan failure (comment above) could be fixed upstream in Cap'n Proto with a small patch, so kj::ExceptionCallback works in programs built with ASan against an uninstrumented libkj. That's the usual setup for sanitizer builds against distro packages, including the ASan CI jobs here and in Bitcoin Core.

The check is requireOnStack() in kj/exception.c++, which is also used by DisallowAsyncDestructorsScope and AllowAsyncDestructorsScope, and is unchanged on the v2 branch. It is skipped only when libkj itself is compiled with ASan or HWASan (capnproto/capnproto#1427, fixed in capnproto/capnproto#1478, and capnproto/capnproto#2500, where the maintainer said he was happy to accept a PR adding another condition). Since that is decided when libkj is built, programs built with ASan against a libkj built without it still fail the check whenever detect_stack_use_after_return moves the object to ASan's fake stack.

A fix that works regardless of how libkj was built would be to declare the public ASan functions from sanitizer/asan_interface.h as weak symbols and return early when the object is in the current thread's fake stack:

extern "C" {
void* __asan_get_current_fake_stack() __attribute__((weak));
void* __asan_addr_is_in_fake_stack(void* fake_stack, void* addr, void** beg, void** end) __attribute__((weak));
}

void requireOnStack(void* ptr, kj::StringPtr description) {
  if (__asan_get_current_fake_stack && __asan_addr_is_in_fake_stack) {
    void* fake_stack = __asan_get_current_fake_stack();
    if (fake_stack && __asan_addr_is_in_fake_stack(fake_stack, ptr, nullptr, nullptr)) return;
  }
  ...existing check...
}

I tried this in a small stand-in for libkj built without ASan, with a program built with and without ASan, and with detect_stack_use_after_return on and off. Objects on the stack passed in every case, heap-allocated objects were still rejected, and builds without ASan behaved the same as before. This also keeps the check working in ASan builds, while the current compile-time condition turns it off. Things still to work out for an upstream PR are Windows, which doesn't have weak symbols the same way, and HWASan, which has no fake stack, so capnproto/capnproto#2500 is a different case.

{
public:
explicit ClockErrorCallback(EventLoop& loop) : m_loop(loop) {}
void onRecoverableException(kj::Exception&& exception) override
{
if (exception.getType() == kj::Exception::Type::FAILED &&
exception.getDescription().endsWith("can't advance backwards in time")) {
MP_LOG(m_loop, Log::Warning) << "EventLoop: ignoring non-monotonic clock read.";
return;
}
next.onRecoverableException(kj::mv(exception));
}
EventLoop& m_loop;
};
#endif
} // namespace

void EventLoop::loop()
{
assert(!CurrentThread().loop_thread);
CurrentThread().loop_thread = true;
KJ_DEFER(CurrentThread().loop_thread = false);
#if CAPNP_VERSION < 1002000
// Registered with KJ for this thread until loop() returns.
ClockErrorCallback clock_error_callback{*this};
#endif
Comment on lines +318 to +321

@ryanofsky ryanofsky Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In commit "event loop: tolerate backward monotonic clock jumps" (f5ce6f8)

Fix is needed here. The ASan + UBSan Bitcoin Core CI job (log) shows a problem with this approach, which is my fault for suggesting it. test_bitcoin aborts on the first IPC test with:

kj/exception.c++:1039: failed: expected offset < 65536 && offset > -65536; ExceptionCallback must be allocated on the stack.

The kj::ExceptionCallback constructor checks that the object is within 64 KiB of a local variable in the constructor. KJ skips that check when KJ itself is built with ASan, but in this job only Bitcoin Core and libmultiprocess are built with ASan, linked against Ubuntu's uninstrumented Cap'n Proto 1.1. The job also sets ASAN_OPTIONS=detect_stack_use_after_return=1, which makes ASan move local variables like clock_error_callback into a heap-allocated "fake stack", so the check fails and the exception terminates the event loop thread. Bitcoin Core's own ASan CI job uses distro Cap'n Proto and the same option, so it would hit this too after a subtree update.

There's no way to turn the check off from outside KJ, but it works to disable ASan instrumentation for loop() and move the rest of its body into a separate method, so only the small function that registers the callback is uninstrumented. A diff on top of this PR:

diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h
index b226284..9e3bd29 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -266,6 +266,10 @@ public:
     //! the m_num_refs reference count is 0.
     void loop();

+    //! Implementation of loop(), separate so loop() can be excluded from ASan
+    //! instrumentation.
+    void loopImpl();
+
     //! Run function on event loop thread. Does not return until function completes.
     //! Must be called while the loop() function is active.
     void sync(kj::FunctionParam<void()> fn);
diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp
index 20ce5a7..cb4e475 100644
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -310,6 +310,15 @@ public:
 #endif
 } // namespace

+// ASan is disabled for this function so clock_error_callback stays on the real
+// stack. KJ requires ExceptionCallback objects to be on the stack, but ASan's
+// detect_stack_use_after_return option moves local variables to a
+// heap-allocated fake stack, and KJ only skips its check when KJ itself is
+// built with ASan. The event loop code is in loopImpl(), which is still
+// instrumented.
+#if defined(__GNUC__) || defined(__clang__)
+__attribute__((no_sanitize("address")))
+#endif
 void EventLoop::loop()
 {
     assert(!CurrentThread().loop_thread);
@@ -319,7 +328,11 @@ void EventLoop::loop()
     // Registered with KJ for this thread until loop() returns.
     ClockErrorCallback clock_error_callback{*this};
 #endif
+    loopImpl();
+}

+void EventLoop::loopImpl()
+{
     {
         const Lock lock(m_mutex);
         assert(!m_async_fns);

I tested this by building with ASan against Cap'n Proto 1.2 with the version check forced on, so the callback is registered. With detect_stack_use_after_return=1, mptest and mpclocktest crash with the current code and pass with this change, with both clang and gcc. I also checked that a stack buffer overflow added to loopImpl() is still reported by ASan, so the compiler doesn't inline it into the uninstrumented loop().


{
const Lock lock(m_mutex);
Expand Down