call monitor on container creation by anonrig · Pull Request #4371 · cloudflare/workerd

anonrig · 2025-06-18T16:45:29Z

Addresses #4362 from a different perspective

Used AI to generate the tests.

kentonv

Most applications will have called monitor() themselves, making this call redundant at best and possibly leading to inconsistent state.

The underlying problem is that if an application does not call monitor(), then the running boolean isn't updated when the container exits. This is actually independent of destroy(), but destroy() is one way to cause the inconsistency.

What we actually need to do is make a call to monitor() automatically internally, so that we can keep the running boolean accurate. The JS monitor() method will have to piggy-back on the internal call's result.

kentonv · 2025-06-18T16:59:08Z

What we actually need to do is make a call to monitor() automatically internally, so that we can keep the running boolean accurate. The JS monitor() method will have to piggy-back on the internal call's result.

To clarify, we should call monitor() automatically in start() (or in the constructor, if the container is already running).

src/workerd/api/container.h

src/workerd/api/container.c++

src/workerd/api/container.h

anonrig · 2025-07-09T14:48:43Z

@kentonv This is ready for another round of reviews. Would you mind taking a look?

kentonv

I don't see any tests in this PR nor a PR to the internal codebase modifying the test there. Where is the new functionality tested?

src/workerd/api/actor-state.c++

src/workerd/api/container.c++

src/workerd/api/container.h

src/workerd/api/container.c++

jasnell · 2026-02-27T16:44:43Z

src/workerd/api/container.h

+  // The background monitor promise, held to keep it alive. Uses eagerlyEvaluate()
+  // to ensure it runs to completion. Held here (rather than via addTask()) to avoid
+  // preventing DO hibernation.
+  kj::Maybe<IoOwn<kj::Promise<void>>> backgroundMonitor;


Eventually, it might be nice to bundle all the IoOwns into a struct so that there's just one IoOwn in use. Non-blocking for this tho.

jasnell

AI Review Summary

Findings posted as inline comments. Two additional items not pinned to diff lines:

visitForMemoryInfo not updated: The PR adds monitorState, monitorKjPromise, monitorJsPromise, and backgroundMonitor but doesn't update visitForMemoryInfo() to track them. Memory profiling will undercount the Container object's true size.
Missing test coverage for destroy() + active background monitor: If destroy() is called while the background monitor is running, the capnp docs say "If a call to monitor() is waiting when destroy() is invoked, monitor() will also return (with no error)." This edge case isn't exercised by the test suite.

This review was generated by an AI assistant and may contain inaccuracies. Please verify the findings independently.

src/workerd/api/container.c++

gabivlj · 2026-02-27T20:52:14Z

src/workerd/server/container-client.c++

-    cmd.set(idx++, "--disable-ipv6");
-  }
+  cmd.set(idx++, "--address");
+  cmd.set(idx++, address);


this is going to break in some MacOS docker desktops as they dont have v6 enabled altogether. Thats why I decided against creating a network ourselves as it's complicated to accommodate all use-cases,

gabivlj · 2026-02-27T20:54:47Z

src/workerd/api/container.h

+  // Promise that resolves when the current start() RPC completes. The background
+  // monitor chains after this to avoid racing with the start RPC (which creates/
+  // starts the container). Without this, the monitor RPC's Docker /wait could see
+  // a stale container from a previous run and return its old exit code.


I think this is an impl. detail of the container service. I think it should be up to the container capnp service to hold the monitor inflight if the start is already inflight.

jasnell · 2026-02-27T21:07:47Z

src/workerd/server/container-client.c++

+    for (auto config: ipamConfig) {
+      auto subnet = kj::str(config.getSubnet());
+      auto gateway = kj::str(config.getGateway());
+      if (subnet.findFirst(':') != kj::none) {


Nit: consider checking the findFirst first, then doing to allocation/copy for the subnet and gateway

jasnell · 2026-02-27T21:08:49Z

src/workerd/server/container-client.c++

  // we simply return false while avoiding an unnecessary error.
  if (response.statusCode == 404) {
-    co_return InspectResponse{.isRunning = false, .ports = {}};
+    co_return InspectResponse{.isRunning = false, .ports = {}, .hostsPath = kj::none};


Suggested change

co_return InspectResponse{.isRunning = false, .ports = {}, .hostsPath = kj::none};

co_return InspectResponse{

.isRunning = false,

.ports = {},

.hostsPath = kj::none,

};

jasnell · 2026-02-27T21:09:13Z

src/workerd/server/container-client.c++

+  auto hostsPath = getHostsPathFromInspectResponse(response.body);
+
+  co_return InspectResponse{
+    .isRunning = running, .ports = kj::mv(portMappings), .hostsPath = kj::mv(hostsPath)};


Suggested change

.isRunning = running, .ports = kj::mv(portMappings), .hostsPath = kj::mv(hostsPath)};

.isRunning = running,

.ports = kj::mv(portMappings),

.hostsPath = kj::mv(hostsPath),

};

anonrig requested review from danlapid and kentonv June 18, 2025 16:45

anonrig requested review from a team as code owners June 18, 2025 16:45

anonrig mentioned this pull request Jun 18, 2025

Revert "update internal state on container.destroy" #4370

Merged

kentonv requested changes Jun 18, 2025

View reviewed changes

anonrig force-pushed the yagiz/call-monitor-on-destroy branch from 8c5b79f to 7cba1d2 Compare June 18, 2025 17:27

anonrig requested a review from kentonv June 18, 2025 17:27

anonrig force-pushed the yagiz/call-monitor-on-destroy branch from 7cba1d2 to d6d3d08 Compare June 18, 2025 17:29

anonrig changed the title ~~call monitor on container destroy~~ call monitor on container creation Jun 18, 2025

anonrig force-pushed the yagiz/call-monitor-on-destroy branch from d6d3d08 to d1ef96f Compare June 18, 2025 17:35

graphite-app bot reviewed Jun 18, 2025

View reviewed changes

src/workerd/api/container.h Outdated Show resolved Hide resolved

anonrig force-pushed the yagiz/call-monitor-on-destroy branch from d1ef96f to f9861d7 Compare June 18, 2025 17:39

kentonv reviewed Jun 18, 2025

View reviewed changes

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

anonrig force-pushed the yagiz/call-monitor-on-destroy branch from f9861d7 to 00733e7 Compare June 26, 2025 20:00

anonrig requested a review from kentonv June 26, 2025 20:00

kentonv reviewed Jun 27, 2025

View reviewed changes

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

anonrig force-pushed the yagiz/call-monitor-on-destroy branch from 00733e7 to bdf8348 Compare July 9, 2025 14:27

jasnell reviewed Jul 9, 2025

View reviewed changes

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

jasnell reviewed Jul 9, 2025

View reviewed changes

src/workerd/api/container.h Outdated Show resolved Hide resolved

kentonv requested changes Jul 17, 2025

View reviewed changes

anonrig force-pushed the yagiz/call-monitor-on-destroy branch 2 times, most recently from 0932785 to 01dfc58 Compare February 27, 2026 14:34

This comment was marked as off-topic.

Sign in to view

anonrig force-pushed the yagiz/call-monitor-on-destroy branch 2 times, most recently from 5aa78e6 to b17d89c Compare February 27, 2026 15:25

anonrig requested review from jasnell and kentonv February 27, 2026 15:25

anonrig requested a review from gabivlj February 27, 2026 15:26

gabivlj approved these changes Feb 27, 2026

View reviewed changes

jasnell reviewed Feb 27, 2026

View reviewed changes

ghostwriternr mentioned this pull request Feb 27, 2026

Treat monitor-not-found as transient startup error cloudflare/containers#158

Closed

jasnell reviewed Feb 27, 2026

View reviewed changes

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

src/workerd/api/container.c++ Outdated Show resolved Hide resolved

src/workerd/api/container.c++ Show resolved Hide resolved

anonrig added 3 commits February 27, 2026 12:17

automatically call monitor() on container start

2870499

call monitor on container creation

55e192c

iterate

32150f7

anonrig force-pushed the yagiz/call-monitor-on-destroy branch from b17d89c to 32150f7 Compare February 27, 2026 20:46

gabivlj reviewed Feb 27, 2026

View reviewed changes

jasnell reviewed Feb 27, 2026

View reviewed changes

anonrig mentioned this pull request Mar 2, 2026

improve container-client tests #6217

Merged

Conversation

anonrig commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentonv left a comment

Choose a reason for hiding this comment

Uh oh!

kentonv commented Jun 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anonrig commented Jul 9, 2025

Uh oh!

kentonv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

jasnell Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

AI Review Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabivlj Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

gabivlj Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anonrig commented Jun 18, 2025 •

edited

Loading