Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds Rust FFI bindings to the C++ autogate system, using extern "C" linkage and #[repr(C)] enums. It also refactors the C++ autogate to use an X-macro (WD_AUTOGATE_KEYS) as a single source of truth.
Findings (highest severity first):
- [MEDIUM]
ffi.h'sstatic_assertis never compiled —//src/rust/autogate:ffi-hdrhas zero reverse dependencies - [MEDIUM]
ffi.rsis a dead file — never referenced viamod ffi;fromlib.rs - [LOW] Stale paths in
autogate.hcomments referencesrc/rust/jsg/autogate/instead ofsrc/rust/autogate/
This review was generated by an AI assistant and may contain errors. Please verify any suggestions before applying them.
| wd_cc_library( | ||
| name = "ffi-hdr", | ||
| hdrs = ["ffi.h"], | ||
| visibility = ["//visibility:public"], | ||
| deps = ["//src/workerd/util:autogate"], | ||
| ) |
There was a problem hiding this comment.
ffi-hdr has zero reverse dependencies — nothing in the build graph depends on //src/rust/autogate:ffi-hdr. This means the static_assert in ffi.h:43 (which validates that the Rust and C++ enums stay in sync) is never compiled by any target. The key safety net of this design is inert.
Consider either:
- Adding
//src/rust/autogate:ffi-hdras a dep of theautogateRust crate (so it's always compiled when the crate is), or - Moving the
static_assertintoautogate.c++(which IS compiled), alongside theworkerd_autogate_is_enabledfunction.
| // Copyright (c) 2026 Cloudflare, Inc. | ||
| // Licensed under the Apache 2.0 license found in the LICENSE file or at: | ||
| // https://opensource.org/licenses/Apache-2.0 | ||
|
|
||
| // When adding a gate: | ||
| // 1. Add WD_AUTOGATE_KEYS entry in src/workerd/util/autogate.h (single source of truth) | ||
| // 2. Add matching entry to WD_AUTOGATE_FFI_KEYS in src/rust/autogate/ffi.h | ||
| // 3. Add the variant here (value from the C++ enum via ffi.h) | ||
| // 4. Update the static_assert count in ffi.h | ||
|
|
||
| /// Mirrors `workerd::util::AutogateKey`. | ||
| /// | ||
| /// `#[repr(C)]` with discriminants that match the C++ enum ordinals. | ||
| /// Defined once here — `ffi.h` builds its `AutogateKey` enum from this same | ||
| /// list (via `WD_AUTOGATE_FFI_KEYS`) so both sides always agree. | ||
| #[repr(C)] | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub enum AutogateKey { | ||
| TestWorkerd = 0, | ||
| V8FastApi = 1, | ||
| StreamingTailWorker = 2, | ||
| TailStreamRefactor = 3, | ||
| RustBackedNodeDns = 4, | ||
| RpcUseExternalPusher = 5, | ||
| WasmShutdownSignalShim = 6, | ||
| EnableFastTextencoder = 7, | ||
| EnableDrainingReadOnStandardStreams = 8, | ||
| } | ||
|
|
||
| unsafe extern "C" { | ||
| // Implemented in src/workerd/util/autogate.c++ — one array lookup. | ||
| #[link_name = "workerd_autogate_is_enabled"] | ||
| fn autogate_is_enabled(key: AutogateKey) -> bool; | ||
| } | ||
|
|
||
| pub fn is_enabled(key: AutogateKey) -> bool { | ||
| // SAFETY: reads from a process-global bool array written once at startup. | ||
| unsafe { autogate_is_enabled(key) } | ||
| } |
There was a problem hiding this comment.
This file is never referenced from lib.rs (no mod ffi; declaration). Rust only compiles files reachable from the crate root, so even though Bazel's native.glob(["**/*.rs"]) includes it in the source set, rustc never compiles it. The AutogateKey enum and extern "C" block here are exact duplicates of what's in lib.rs.
This file should either be removed, or lib.rs should use mod ffi; and re-export from it (eliminating the duplication).
| // This list is the single source of truth — it drives: | ||
| // - The AutogateKey enum (below) | ||
| // - KJ_STRINGIFY (autogate.c++) | ||
| // - The Rust FFI constexpr accessors (src/rust/jsg/autogate/ffi.h) |
There was a problem hiding this comment.
Path is stale — the file is at src/rust/autogate/ffi.h, not src/rust/jsg/autogate/ffi.h.
| // - The Rust FFI constexpr accessors (src/rust/jsg/autogate/ffi.h) | |
| // - The Rust FFI constexpr accessors (src/rust/autogate/ffi.h) |
| // - The Rust FFI constexpr accessors (src/rust/jsg/autogate/ffi.h) | ||
| // | ||
| // To add a gate: add one WD_AUTOGATE_KEY line here, then add a matching | ||
| // method in src/rust/jsg/autogate/lib.rs. |
There was a problem hiding this comment.
Same stale path — should be src/rust/autogate/lib.rs.
| // method in src/rust/jsg/autogate/lib.rs. | |
| // method in src/rust/autogate/lib.rs. |
|
I'm Bonk, and I've done a quick review of your PR. This PR adds Rust FFI bindings to the C++ autogate system and refactors autogate.h to use an X-macro. I posted a review on PR #6366 with 4 inline comments covering 3 findings:
|
|
I'm not thrilled that the list of autogates in this PR would apparently need to be maintained across 5 different files (one in C++ the others in rust). Also keep in mind that a parallel change likely needs to be made internally. I don't have many strong opinions about how autogates are implemented so long as the checks are fast and the maintenance is easier. With the changes here, I'm not convinced of the latter. |
What an understatement |
I agree. I couldn't find a better way as well. I'll keep this in draft so we can think over it. |
Merging this PR will improve performance by 26.15%
Performance Changes
Comparing 
Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6366 +/- ##
==========================================
- Coverage 70.71% 70.54% -0.18%
==========================================
Files 420 422 +2
Lines 113713 114032 +319
Branches 18735 18780 +45
==========================================
+ Hits 80414 80439 +25
- Misses 22070 22363 +293
- Partials 11229 11230 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This to me feels like a good use case for proc macros. I would explore ingesting the autogate.h file in Rust using a macro and then outputting the AST for an equivalent enum in Rust. Parsing the |
5 participants
Found a workaround for keeping the C++ and Rust implementations in sync. We could potentially get rid of the C++ Autogate in favor of Rust, if it makes sense in the future.