From eb16df7a0c137af0eb781a852597039a4c884c7a Mon Sep 17 00:00:00 2001 From: Felix Hanau Date: Tue, 22 Sep 2026 18:55:23 -0400 Subject: [PATCH 1/2] [nfc] Add IWYU guidelines to AGENTS.md, move tracked-wasm-instance into its own target --- .opencode/skills/iwyu/SKILL.md | 30 +++++++++++++++++++ src/workerd/io/BUILD.bazel | 22 +++++++++++--- src/workerd/io/limit-enforcer.h | 2 +- src/workerd/io/worker.c++ | 1 + src/workerd/server/factory/worker-factory.c++ | 1 + src/workerd/tests/test-fixture.c++ | 1 + 6 files changed, 52 insertions(+), 5 deletions(-) create mode 100644 .opencode/skills/iwyu/SKILL.md diff --git a/.opencode/skills/iwyu/SKILL.md b/.opencode/skills/iwyu/SKILL.md new file mode 100644 index 00000000000..1f2eb5eddf5 --- /dev/null +++ b/.opencode/skills/iwyu/SKILL.md @@ -0,0 +1,30 @@ +--- +name: iwyu +description: Follow IWYU guidelines to improve build caching. This entails adding/removing C++ +includes as well as Bazel deps and implementation_deps when appropriate. Load when making non- +trivial changes to C++ includes or Bazel target declarations. +--- + +## IWYU FOR C++ TARGETS + +Adapted from [include-what-you-use](https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md). +Small targets with accurate deps mean a change rebuilds and retests fewer targets. + +- Don't add `#include`s a file doesn't use or `deps` a target doesn't use. When a change removes + the last use of a header, delete its `#include`, then the dep if it's now unused. +- Put deps whose headers are included only from `.c++` files in `implementation_deps`, so they + don't propagate to dependents. +- Prefer direct includes and deps over relying on transitive ones, but don't add a long list just + to spell out what you already get transitively. For instance, most workerd C++ sources depend on + KJ but we don't declare this dependency for each target separately, or include kj/common.h, + kj/string.h and so on each time. Direct includes that are already present and being used should + generally stay in place. +- Give each `.h`/`.c++` pair its own `wd_cc_library`, named after the file (e.g. + `:tracked-wasm-instance`). A header without a `.c++` gets its own target too, with the deps it + needs to compile on its own. +- Keep each header in exactly one target, together with its `.c++` if it has one. Exposing a + header from a target that doesn't build its `.c++` lets code call its functions without + depending on their definitions, which shows up later as undefined-symbol link errors. +- If splitting a pair out would create a dependency cycle, leave it in its combined target (e.g. + `//src/workerd/io`) instead of separating the header from its `.c++`. Don't add new files to + such targets if they can have their own. \ No newline at end of file diff --git a/src/workerd/io/BUILD.bazel b/src/workerd/io/BUILD.bazel index 8951d43ed93..30b9a8172d8 100644 --- a/src/workerd/io/BUILD.bazel +++ b/src/workerd/io/BUILD.bazel @@ -102,6 +102,7 @@ wd_cc_library( ":observer", ":release-version", ":trace", + ":tracked-wasm-instance", ":wasm-instantiate-shim", ":worker-interface", ":worker-source", @@ -356,15 +357,28 @@ wd_cc_library( wd_cc_library( name = "limit-enforcer", - srcs = ["tracked-wasm-instance.c++"], hdrs = [ "limit-enforcer.h", - "tracked-wasm-instance.h", ], visibility = ["//visibility:public"], deps = [ ":outcome_capnp", - "//src/workerd/jsg", + "@capnp-cpp//src/kj", + "@capnp-cpp//src/kj:kj-async-core", + "@workerd-v8//:v8", + ], +) + +wd_cc_library( + name = "tracked-wasm-instance", + srcs = ["tracked-wasm-instance.c++"], + hdrs = [ + "tracked-wasm-instance.h", + ], + visibility = ["//visibility:public"], + deps = [ + "@capnp-cpp//src/kj", + "@workerd-v8//:v8", ], ) @@ -595,7 +609,7 @@ wd_test( kj_test( src = "tracked-wasm-instance-test.c++", deps = [ - ":limit-enforcer", + ":tracked-wasm-instance", ], ) diff --git a/src/workerd/io/limit-enforcer.h b/src/workerd/io/limit-enforcer.h index f8d6f31673c..2cb2c8fa866 100644 --- a/src/workerd/io/limit-enforcer.h +++ b/src/workerd/io/limit-enforcer.h @@ -5,7 +5,6 @@ #pragma once #include -#include #include @@ -18,6 +17,7 @@ namespace workerd { class IsolateObserver; class RequestObserver; +class TrackedWasmInstanceList; struct ActorCacheSharedLruOptions; class IoContext; diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index 821fb39c5d1..552a69f8f5a 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include diff --git a/src/workerd/server/factory/worker-factory.c++ b/src/workerd/server/factory/worker-factory.c++ index c9386fe65a7..8f5bda19922 100644 --- a/src/workerd/server/factory/worker-factory.c++ +++ b/src/workerd/server/factory/worker-factory.c++ @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include diff --git a/src/workerd/tests/test-fixture.c++ b/src/workerd/tests/test-fixture.c++ index 1daeecd7dc8..2d7f8143cb1 100644 --- a/src/workerd/tests/test-fixture.c++ +++ b/src/workerd/tests/test-fixture.c++ @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include From 7fc447aa9693b8dfcd42431e734ded2f1907c38c Mon Sep 17 00:00:00 2001 From: Felix Hanau Date: Wed, 23 Sep 2026 11:40:04 -0400 Subject: [PATCH 2/2] [build] Remove rust_io_link_check We already enforce this using the bazel aquery somepath check, no need to check it twice. --- build/fixtures/BUILD.bazel | 5 +- build/fixtures/rust_io_link_check.sh | 83 ---------------------------- build/rust_io_graph_check.sh | 3 +- src/workerd/server/AGENTS.md | 2 +- src/workerd/server/BUILD.bazel | 25 --------- src/workerd/util/setup-async-io.c++ | 5 +- 6 files changed, 5 insertions(+), 118 deletions(-) delete mode 100755 build/fixtures/rust_io_link_check.sh diff --git a/build/fixtures/BUILD.bazel b/build/fixtures/BUILD.bazel index 095b67e5b9e..e85ef5d729c 100644 --- a/build/fixtures/BUILD.bazel +++ b/build/fixtures/BUILD.bazel @@ -1,4 +1 @@ -exports_files([ - "kj_test.sh", - "rust_io_link_check.sh", -]) +exports_files(["kj_test.sh"]) diff --git a/build/fixtures/rust_io_link_check.sh b/build/fixtures/rust_io_link_check.sh deleted file mode 100755 index ed46c381a62..00000000000 --- a/build/fixtures/rust_io_link_check.sh +++ /dev/null @@ -1,83 +0,0 @@ -#!/bin/bash -# Link-truth check for the Rust I/O backend: inspects the symbol names of the linked workerd -# binary and fails if kj's own C++ event-loop setup made it into the link. -# -# kj::setupAsyncIo() is supplied by //src/workerd/util:setup-async-io -# (tokio-backed) by symbol override, and the concrete kj OS I/O layer (kj-async-os) must not be -# linked. The dependency-graph check (build/rust_io_graph_check.sh) forbids dependency EDGES to it; -# this test checks the RESULT: when kj-async-os does get pulled in (e.g. through the -# @capnp-cpp//src/kj:kj-async umbrella), the static link picks whichever setupAsyncIo() it meets -# first, and a binary quietly running on kj::UnixEventPort passes every test that builds its own -# event loop. So assert on the linked symbols themselves: -# * kj's setupAsyncIo() has a local class `BasicContext`, and kj's async-io-unix.c++ has -# `LowLevelAsyncIoProviderImpl`; neither may be present. -# * the shim's TU references kj_rs_io::TokioAsyncIoContext; it must be present. -# The kj::UnixEventPort::* symbols ARE expected: the shim defines an inert UnixEventPort because -# kj::AsyncIoContext names the type (see setup-async-io.c++). -# -# How: the symbol names are matched in their MANGLED form (Itanium ABI, identical for GNU/LLVM/ -# Apple toolchains) as strings of the binary itself, in one `grep -a` pass. In an unstripped -# binary every linked symbol's mangled name sits NUL-terminated in the string table (plus DWARF -# linkage names and RTTI type names), and none of these names can appear unless its defining -# translation unit was linked. This deliberately does not use nm: no nm is hermetically available -# to the test -- the C++ toolchain's is autoconfigured on the host that runs bazel and need not -# exist on a remote executor, and the Rust toolchain does not export its llvm-nm -- whereas -# grep is a given wherever this bash script runs. `12setupAsyncIo` is the length-prefixed -# identifier inside kj::setupAsyncIo's mangled name, and so on. -# -# Unix only (target_compatible_with in the BUILD): MSVC mangles differently -# (`?setupAsyncIo@kj@@...`), so on Windows this would inspect nothing. -set -euo pipefail - -# $1 is `$(rlocationpath :workerd)`: relative to the runfiles root, whatever repository workerd -# is built from (`_main/...` standalone, `+local_repository+workerd/...` as a dependency). -BIN="${TEST_SRCDIR:-.}/$1" -[ -f "$BIN" ] || BIN="$1" -[ -f "$BIN" ] || { echo "rust-io-link-check: workerd binary not found: $1"; exit 1; } - -PAT_KJ_SETUP='12setupAsyncIoEvEN?12BasicContext' # kj::setupAsyncIo()::BasicContext (+ members) -PAT_KJ_LOWLEVEL='28LowLevelAsyncIoProviderImpl' # kj::(anon)::LowLevelAsyncIoProviderImpl -PAT_SHIM='8kj_rs_io19TokioAsyncIoContext' # kj_rs_io::TokioAsyncIoContext -PAT_UNIXPORT='N2kj13UnixEventPort' # kj::UnixEventPort -PAT_ANY_KJ='N2kj' # any kj:: symbol at all - -# One pass over the (large, debug-build) binary, extracting just the matching names; count -# from that small file. LC_ALL=C: byte-wise matching, no multibyte decoding of binary data. -SYMS="${TEST_TMPDIR:-/tmp}/rust-io-link-check.syms" -{ LC_ALL=C grep -a -o -E "$PAT_KJ_SETUP|$PAT_KJ_LOWLEVEL|$PAT_SHIM|$PAT_UNIXPORT|$PAT_ANY_KJ" "$BIN" || true; } > "$SYMS" -count() { { grep -c -E "$1" "$SYMS" || true; } | tr -d ' '; } - -kj_total=$(count "$PAT_ANY_KJ") -if [ "$kj_total" -eq 0 ]; then - echo "rust-io-link-check: FAIL -- $BIN carries no kj:: symbol names. This check needs an" - echo " unstripped binary (it is tagged no-asan because the ASAN lane builds with --strip=always);" - echo " a configuration that strips cannot be verified this way and must rely on the" - echo " dependency-graph check (build/rust_io_graph_check.sh)." - exit 1 -fi - -kj_setup=$(count "$PAT_KJ_SETUP") -kj_lowlevel=$(count "$PAT_KJ_LOWLEVEL") -shim=$(count "$PAT_SHIM") -unixport=$(count "$PAT_UNIXPORT") - -echo "rust-io-link-check: $BIN" -echo " kj setupAsyncIo()::BasicContext names : $kj_setup (must be 0)" -echo " kj LowLevelAsyncIoProviderImpl names : $kj_lowlevel (must be 0)" -echo " shim kj_rs_io::TokioAsyncIoContext : $shim (must be > 0)" -echo " kj::UnixEventPort::* names : $unixport (informational; the shim's inert port)" - -status=0 -if [ "$kj_setup" -ne 0 ] || [ "$kj_lowlevel" -ne 0 ]; then - echo "FAIL: kj's C++ setupAsyncIo()/OS I/O provider is linked into the workerd binary." - echo " Some dependency reaches @capnp-cpp//src/kj:kj-async-os (usually via the :kj-async" - echo " umbrella). Find every offending edge with:" - echo " bazel cquery 'rdeps(deps(//src/workerd/server:workerd), @capnp-cpp//src/kj:kj-async, 1)'" - echo " and retarget it to :kj-async-core / :kj-async-io." - status=1 -fi -if [ "$shim" -eq 0 ]; then - echo "FAIL: the tokio setupAsyncIo() shim (//src/workerd/util:setup-async-io) is not in the link." - status=1 -fi -exit $status diff --git a/build/rust_io_graph_check.sh b/build/rust_io_graph_check.sh index d6bb169fec4..54193463fa2 100755 --- a/build/rust_io_graph_check.sh +++ b/build/rust_io_graph_check.sh @@ -6,8 +6,7 @@ # and with static archives the linker keeps whichever it meets first, silently. # # One query, empty output means clean; otherwise it prints one offending dependency path. Extra -# arguments are passed to bazel (CI passes its --config flags). The linked binary's symbols are -# checked separately by //src/workerd/server:rust-io-link-check. +# arguments are passed to bazel (CI passes its --config flags). set -euo pipefail cd "$(dirname "$0")/.." diff --git a/src/workerd/server/AGENTS.md b/src/workerd/server/AGENTS.md index d651c5108a8..959b0434930 100644 --- a/src/workerd/server/AGENTS.md +++ b/src/workerd/server/AGENTS.md @@ -126,7 +126,7 @@ Bazel labels: `//src/workerd/server:workerd` (and `:workerd_cross`, which `wd_te - Never depend on the `@capnp-cpp//src/kj:kj-async` umbrella: it drags in `kj-async-os`, whose definitions collide with the shim's (an ODR violation). Use `:kj-async-core` / `:kj-async-io`. `just check-io-backend-graph` (one `bazel cquery somepath`, run by the lint CI lane) rejects any - such path from `:workerd`; `:rust-io-link-check` inspects the linked binary's symbols on unix. + such path from `:workerd`. - Ownership: the returned `kj::AsyncIoContext` borrows a heap holder attached to its `lowLevelProvider`; the holder owns the tokio context and the inert event port, so the context's references are valid for its whole lifetime and torn down once. The inert `kj::UnixEventPort` is diff --git a/src/workerd/server/BUILD.bazel b/src/workerd/server/BUILD.bazel index 5d120e01fd0..a9608278142 100644 --- a/src/workerd/server/BUILD.bazel +++ b/src/workerd/server/BUILD.bazel @@ -1,7 +1,6 @@ load("@bazel_skylib//lib:selects.bzl", "selects") load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") load("@bazel_skylib//rules:copy_file.bzl", "copy_file") -load("@rules_shell//shell:sh_test.bzl", "sh_test") load("//:build/kj_test.bzl", "kj_test") load("//:build/linking.bzl", "CC_LIBRARY_LINKSTATIC") load("//:build/wd_capnp_library.bzl", "wd_capnp_library") @@ -134,30 +133,6 @@ wd_cc_library( ], ) -# The link-time complement of build/rust_io_graph_check.sh: that query forbids dependency EDGES to -# the kj OS I/O layer in the graph; this test inspects the symbols of the linked :workerd binary and -# fails if kj's own setupAsyncIo()/OS provider made it in anyway (or the tokio shim did not). It -# exists because a binary that silently links kj-async-os and lets kj's setupAsyncIo() win the -# link passes every test that constructs its own event loop. Unix, unstripped only. -sh_test( - name = "rust-io-link-check", - # One grep pass over the unstripped workerd binary: seconds locally, longer on CI runners. - size = "large", - srcs = ["//build/fixtures:rust_io_link_check.sh"], - # rlocationpath, not location: the binary is looked up under $TEST_SRCDIR so the check also - # works when workerd is built as an external repository (the internal build). - args = ["$(rlocationpath :workerd)"], - data = [":workerd"], - # Needs an unstripped binary: the ASAN lane strips (--strip=always), so it is excluded there - # by tag (test:asan filters -no-asan) rather than passing vacuously. - tags = ["no-asan"], - target_compatible_with = select({ - # The script matches Itanium-mangled names; MSVC mangling is not covered. - "@platforms//os:windows": ["@platforms//:incompatible"], - "//conditions:default": [], - }), -) - wd_cc_library( name = "alarm-scheduler", srcs = [ diff --git a/src/workerd/util/setup-async-io.c++ b/src/workerd/util/setup-async-io.c++ index 9b2dd9b1f3b..876f4552272 100644 --- a/src/workerd/util/setup-async-io.c++ +++ b/src/workerd/util/setup-async-io.c++ @@ -22,9 +22,8 @@ // If kj-async-os is ever accidentally linked in, this TU's definitions collide with the native // ones, and with static archives the winner is LINK-ORDER-DEPENDENT -- a duplicate-symbol error if // you are lucky, the native setupAsyncIo silently winning (wrong event loop) or pairing with this -// inert UnixEventPort (KJ_UNIMPLEMENTED at runtime) if you are not. The guards against that are -// build/rust_io_graph_check.sh (the dependency graph) and //src/workerd/server:rust-io-link-check -// (the linked binary's symbols). +// inert UnixEventPort (KJ_UNIMPLEMENTED at runtime) if you are not. The guard against that is +// build/rust_io_graph_check.sh (based on bazel aquery). #include