Merge pull request #6041 from cloudflare/yagiz/fix-settimeout-inconsistencies-2

fix: use raw timer time for setTimeout scheduling to prevent stale clock clamping

Author: anonrig

src/workerd/api/tests/settimeout-test.js

@@ -1,18 +1,57 @@
 import { ok } from 'node:assert';
+import timers from 'node:timers/promises';
 
 // Allow up to 10ms of jitter because the precise_timers compat flag
 // can introduce +/-3ms of variance in timer resolution, and coverage
 // builds add additional overhead.
 const JITTER = 10;
 
-export default {
+// The first setTimeout was firing too early because
+// kj::Timer::now() was stale after script compilation/startup.
+// Ref: https://github.com/cloudflare/workerd/issues/6019
+export const basicAccuracy = {
   async test() {
     const t0 = Date.now();
-    await new Promise((accept) => setTimeout(accept, 100));
+    await timers.setTimeout(100);
     const t1 = Date.now();
     ok(t1 - t0 >= 100 - JITTER, `Received a difference of ${t1 - t0}`);
-    await new Promise((accept) => setTimeout(accept, 100));
+    await timers.setTimeout(100);
     const t2 = Date.now();
     ok(t2 - t1 >= 100 - JITTER, `Received a difference of ${t2 - t1}`);
   },
 };
+
+// After CPU-heavy work between setTimeout calls,
+// subsequent consecutive setTimeouts must still each wait their full delay
+// rather than all firing at the same instant.
+// Ref: https://github.com/cloudflare/workerd/issues/6037
+export const accuracyAfterCpuWork = {
+  async test() {
+    await timers.setTimeout(50);
+
+    // Let's burn some CPU
+    for (let j = 0; j < 1e9; j++);
+
+    const a = Date.now();
+    await timers.setTimeout(50);
+    const b = Date.now();
+    ok(
+      b - a >= 50 - JITTER,
+      `After CPU work, first sleep: expected ~50ms, got ${b - a}ms`
+    );
+
+    await timers.setTimeout(50);
+    const c = Date.now();
+    ok(
+      c - b >= 50 - JITTER,
+      `After CPU work, second sleep: expected ~50ms, got ${c - b}ms`
+    );
+
+    await timers.setTimeout(50);
+    const d = Date.now();
+    ok(
+      d - c >= 50 - JITTER,
+      `After CPU work, third sleep: expected ~50ms, got ${d - c}ms`
+    );
+  },
+};

src/workerd/io/io-channels.h

@@ -56,8 +56,10 @@ class TimerChannel {
   // Call each time control enters the isolate to set up the clock.
   virtual void syncTime() = 0;
 
-  // Return the current time.
-  virtual kj::Date now() = 0;
+  // Return the current time. `nextTimeout` is the time at which the next setTimeout() callback
+  // is scheduled; implementations performing Spectre mitigations should clamp to this value so
+  // that Date.now() never goes backwards or reveals timing side channels.
+  virtual kj::Date now(kj::Maybe<kj::Date> nextTimeout = kj::none) = 0;
 
   // Returns a promise that resolves once `now() >= when`.
   virtual kj::Promise<void> atTime(kj::Date when) = 0;

src/workerd/io/io-context.c++

@@ -278,9 +278,9 @@ void IoContext::IncomingRequest::delivered(kj::SourceLocation location) {
   }
 }
 
-kj::Date IoContext::IncomingRequest::now() {
+kj::Date IoContext::IncomingRequest::now(kj::Maybe<kj::Date> nextTimeout) {
   metrics->clockRead();
-  return ioChannelFactory->getTimer().now();
+  return ioChannelFactory->getTimer().now(kj::mv(nextTimeout));
 }
 
 IoContext::IncomingRequest::~IoContext_IncomingRequest() noexcept(false) {
@@ -730,8 +730,7 @@ void IoContext::TimeoutManagerImpl::setTimeoutImpl(IoContext& context, Iterator
 
   auto paf = kj::newPromiseAndFulfiller<void>();
 
-  // Always schedule the timeout relative to what Date.now() currently returns, so that the delay
-  // appear exact. Otherwise, the delay could reveal non-determinism containing side channels.
+  // Schedule relative to Date.now() so the delay appears exact to the application.
   auto when = context.now() + state.params.msDelay * kj::MILLISECONDS;
   // TODO(cleanup): The manual use of run() here (including carrying over the critical section) is
   //   kind of ugly, but using awaitIo() doesn't work here because we need the ability to cancel
@@ -892,17 +891,9 @@ kj::Date IoContext::now(IncomingRequest& incomingRequest) {
     return kj::UNIX_EPOCH + roundedMs * kj::MILLISECONDS;
   }
 
-  kj::Date adjustedTime = incomingRequest.now();
-
-  KJ_IF_SOME(maybeNextTimeout, timeoutManager->getNextTimeout()) {
-    // Don't return a time beyond when the next setTimeout() callback is intended to run. This
-    // ensures that Date.now() inside the callback itself always returns exactly the time at which
-    // the callback was scheduled (hiding non-determinism which could contain side channels), and
-    // that the time returned by Date.now() never goes backwards.
-    return kj::min(adjustedTime, maybeNextTimeout);
-  } else {
-    return adjustedTime;
-  }
+  // Let TimerChannel decide whether to clamp to the next timeout time. This is how Spectre
+  // mitigations ensure Date.now() inside a callback returns exactly the scheduled time.
+  return incomingRequest.now(timeoutManager->getNextTimeout());
 }
 
 kj::Date IoContext::now() {

src/workerd/io/io-context.h

@@ -166,7 +166,7 @@ class IoContext_IncomingRequest final {
   // Access the event loop's current time point. This will remain constant between ticks. This is
   // used to implement IoContext::now(), which should be preferred so that time can be adjusted
   // based on setTimeout() when needed.
-  kj::Date now();
+  kj::Date now(kj::Maybe<kj::Date> nextTimeout = kj::none);
 
   RequestObserver& getMetrics() {
     return *metrics;

src/workerd/server/server.c++

@@ -3381,12 +3381,12 @@ class Server::WorkerService final: public Service,
     // Nothing to do
   }
 
-  kj::Date now() override {
+  kj::Date now(kj::Maybe<kj::Date>) override {
     return kj::systemPreciseCalendarClock().now();
   }
 
   kj::Promise<void> atTime(kj::Date when) override {
-    auto delay = when - now();
+    auto delay = when - now(kj::none);
     // We can't use `afterDelay(delay)` here because kj::Timer::afterDelay() is equivalent to
     // `atTime(timer.now() + delay)`, and kj::Timer::now() only advances when the event loop
     // polls for I/O. If JavaScript executed for a significant amount of time since the last

src/workerd/tests/test-fixture.c++

@@ -57,7 +57,7 @@ class DummyErrorHandler final: public kj::TaskSet::ErrorHandler {
 struct MockTimerChannel final: public TimerChannel {
   void syncTime() override {}
 
-  kj::Date now() override {
+  kj::Date now(kj::Maybe<kj::Date>) override {
     return kj::systemPreciseCalendarClock().now();
   }
 
@@ -78,7 +78,7 @@ struct RealTimerChannel final: public TimerChannel {
 
   void syncTime() override {}
 
-  kj::Date now() override {
+  kj::Date now(kj::Maybe<kj::Date>) override {
     return kj::systemPreciseCalendarClock().now();
   }