Repository navigation
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed d7b5e1447f2d4122f3d2cb98394e4e6ba9ffbf16 against cb945254521ed6dab365072f7caec10c524e323b. All six changed files were reviewed, including auction callers, telemetry consumers, DataDome cache timing, configuration serialization, process cleanup, and CI execution.
No actionable issues introduced by this PR found.
Safety proof
- Cloudflare timer creation: Traced
/auctionthrough consent handling intoAuctionObservationContext. Confirmed thattest_cloudflare_enabled_auction_reaches_telemetryexecuted successfully in workerd in CI job 110442815930. The CI checkout had the same Git tree as the reviewed head. Runtime evidence, proven. - Native/WASI compatibility: Compiled direct assignments from the locked
web_time::Instanttype tostd::time::Instantfor native andwasm32-wasip1; both passed. Telemetry and DataDome scope tests passed natively and through Viceroy. Executed evidence, proven.
Validation and review context
Checks ran in an isolated snapshot of the exact head; the original worktree remained clean.
cargo fmt --all -- --checkand diff whitespace check passed.cargo test-cloudflare --locked --offline: 53 tests passed.- Focused core telemetry tests: 14 passed natively and 14 through
cargo test-fastly. - Focused DataDome protection-scope tests: 11 passed natively and 11 through
cargo test-fastly. - Integration test binary: 29 passed, 10 runtime tests ignored locally.
- Cross-adapter parity: 17 passed.
cargo check-cloudflare --locked --offlinepassed.CARGO_NET_OFFLINE=true TSJS_SKIP_BUILD=1 cargo clippy-cloudflare-wasmpassed.- All reported PR checks passed. No existing reviews, inline comments, review threads, or issue comments.
Residual risk: No local workerd rerun because Wrangler and the bundle were absent; the CI runtime result was inspected directly. Cloudflare DataDome cache execution remains untested and currently unreachable through that adapter's request handling.
std::time::Instant::now() panics on the Cloudflare adapter's wasm32-unknown-unknown target. Switch auction telemetry and the DataDome IP CIDR source cache to web_time::Instant, which re-exports the standard type on other targets. Add a Cloudflare integration test that starts wrangler dev with an auction-enabled config and posts to /auction. The request passes the consent gate and completes through the orchestrator, so it creates an AuctionObservationContext and its web_time::Instant in workerd. The config drops the fixture's providers so the auction makes no outbound calls. With std::time::Instant restored, the worker panics with "time not implemented on this platform" and the test fails. The config is built per test from the shared fixture, so the Fastly, Axum, and other Cloudflare tests keep auctions disabled. The DataDome IP CIDR source is not covered: only the Fastly adapter runs request filters, so Cloudflare never evaluates the DataDome protection scope. Closes #1075 Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
The template cache and origin_shared_ttl in publisher.rs still called std::time::Instant::now and SystemTime::now, which panic on wasm32-unknown-unknown. Move them to web_time. origin_shared_ttl_at keeps taking a std SystemTime because httpdate parses into that type, so build it from UNIX_EPOCH plus the web_time elapsed duration. Add scripts/lint-wasm-clock.sh, a clippy pass on wasm32-unknown-unknown with a dedicated config that denies std::time::Instant::now and std::time::SystemTime::now in trusted-server-core. It runs on that target only because elsewhere web_time re-exports the std types, so a workspace-wide rule would flag the correct web_time calls. Run it in CI and list it under the AGENTS.md CI gates. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
d7b5e14 to
2226836
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
Moves the remaining request-path std clocks in trusted-server-core (auction telemetry, the DataDome IP CIDR cache, the template cache and origin_shared_ttl) to web_time, adds a wasm32-unknown-unknown clippy gate, and adds a workerd regression test. I checked the main claims locally. With std::time::Instant restored in telemetry.rs, the new integration test fails under wrangler dev, and scripts/lint-wasm-clock.sh fails even when it runs right after cargo clippy-cloudflare-wasm; both pass at this head. The UNIX_EPOCH + elapsed bridge is pure arithmetic on std's unsupported-time backend, and chrono's wasmbind feature is enabled in the Cloudflare build, so Utc::now() is safe as stated. The one blocker is the conflict with main. The two lint suggestions close gaps I could reproduce.
2 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for both at once) to apply them as commits on the PR branch. The other two inline comments are a prose refactor that touches files outside this diff and an informational note.
Blocking
🔧 wrench
- Conflicts with
main, needs a rebase — see Cross-cutting below
Non-blocking
♻️ refactor / 📝 note
- The lint misses
std::time::SystemTime::elapsed()— see inline atscripts/clippy-wasm-clock/clippy.toml:4 - Lint the Cloudflare adapter's build graph, not core alone — see inline at
scripts/lint-wasm-clock.sh:2 - Second copy of the std
SystemTimebridge — see inline atcrates/trusted-server-core/src/publisher.rs:6470 - Workers timers only advance after I/O — see inline at
crates/trusted-server-core/src/auction/telemetry.rs:12
🌱 seedling / 🏕 camp site
- Declare chrono's
wasmbindfeature explicitly — see Cross-cutting below /check-cidoes not run the new gate — see Cross-cutting below
Cross-cutting / body-level findings
-
🔧 Conflicts with
main, needs a rebase — GitHub reports this branch asCONFLICTING. Since the merge base (7f610c0fc),mainchanged two of the files this PR touches:crates/trusted-server-core/src/publisher.rs: #1210 addeduse sha2::Digest as _;where this PR addsuse web_time::Instant;. Keep both,sha2first.AGENTS.mdCI Gates: #1179 addedcargo test-fastly-reuseto item 3, and #1210 added item 9. Keep main's item 3, and consider giving the lint its own numbered item (like item 9) instead of an indented line under item 2.
I resolved it that way in a scratch merge. The result passes
scripts/lint-wasm-clock.sh,cargo clippy-cloudflare-wasmandcargo fmt --all -- --check, so main's new commits add no std clock calls to core. The existing approvals are ond7b5e1447(the first commit only); thepublisher.rschange, lint script, CI step andAGENTS.mdedit in22268364came after them. -
🌱 Declare chrono's
wasmbindfeature explicitly — The PR leaveschrono::Utc::now()(auction/telemetry.rs:875,integrations/datadome/protection.rs:384,request_signing/rotation.rs:325) alone because chrono's defaultwasmbindfeature reads the JS clock. That holds today (cargo tree -p trusted-server-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflare -e features -i chronoshowswasmbindandjs-sys), but only because the workspace'schrono = "0.4.44"keeps default features. A laterdefault-features = falsewould make those calls panic on Cloudflare, and no clippy rule here can see inside chrono. Core already declares the equivalent requirement forgetrandomanduuidunder[target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dependencies]; addingchrono = { workspace = true, features = ["wasmbind"] }there would make it explicit. Fine as a follow-up. -
🏕
/check-cidoes not run the new gate —.claude/commands/check-ci.md("Run all CI checks locally, in order") doesn't callscripts/lint-wasm-clock.sh. That list was already behind CI (noclippy-cloudflare-wasm, Spin or CLI lints); adding the script after its Cloudflare clippy step would keep a local run in line with thecargo fmtjob.
CI Status
All checks ran against the pre-conflict merge base and need to re-run after the rebase.
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (both runs)
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required); this job runs the new
scripts/lint-wasm-clock.shstep - cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS; ran
test_cloudflare_enabled_auction_reaches_telemetry - integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Lint the Cloudflare adapter's build graph instead of core alone and also deny the std Instant and SystemTime elapsed methods, which call the panicking now(). Extract std_system_time_now for the publisher and S3 signing callers, declare chrono's wasmbind feature for wasm32-unknown-unknown, note the Workers timer behaviour on elapsed_ms, and run the lint from /check-ci. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
…me-instant Signed-off-by: dhruv8sh <dhruv8sh@proton.me> # Conflicts: # AGENTS.md # crates/trusted-server-core/src/publisher.rs
|
Merged In 466520b I also declared |
Summary
std::time::Instant::now()andstd::time::SystemTime::now()panic on the Cloudflare adapter'swasm32-unknown-unknowntarget (workerd: "time not implemented on this platform"), so any auction on Cloudflare crashed the worker when it started auction telemetry. This PR moves the request-path clock calls intrusted-server-core(auction telemetry, the DataDome IP CIDR source cache, and the publisher template cache) toweb_time. On every other targetweb_timere-exports the std types, so Fastly, Spin, and Axum behave the same as before.scripts/lint-wasm-clock.sh, run in CI, so the std clock calls (nowandelapsed) cannot come back into any crate built into the Cloudflare Worker.wrangler devwith an auction-enabled config and posts to/auction. The request passes the consent gate and runs to completion through the orchestrator, which creates anAuctionObservationContext(and itsInstant) inside workerd. Withstd::time::Instantrestored, the worker panics and the test fails. With this fix, it passes.Changes
crates/trusted-server-core/src/auction/telemetry.rsweb_time::Instantinstead ofstd::time::Instantcrates/trusted-server-core/src/integrations/datadome/protection_scope.rsweb_time::Instantfor the IP CIDR source cache timingcrates/trusted-server-core/src/publisher.rsweb_time::Instantfor template cache expiry.origin_shared_ttlgets itsSystemTimefromstd_system_time_now();origin_shared_ttl_atstill takes a stdSystemTimebecausehttpdateparses into that typecrates/trusted-server-core/src/ec/mod.rsstd_system_time_now(): a stdSystemTimederived fromweb_time::SystemTime::now(), shared by the publisher and S3 signing callers, with a unit testcrates/trusted-server-core/src/proxy.rss3_sigv4::sign_headerscall usesstd_system_time_now()crates/trusted-server-core/Cargo.tomlwasmbindfeature forwasm32-unknown-unknownexplicitlyscripts/lint-wasm-clock.sh,scripts/clippy-wasm-clock/clippy.tomlwasm32-unknown-unknownover the Cloudflare adapter's build graph (-p trusted-server-adapter-cloudflare --features cloudflare --lib) with a dedicated config that denies onlystd::time::{Instant,SystemTime}::{now,elapsed}.github/workflows/format.ymlcargo clippy-cloudflare-wasmAGENTS.md,.claude/commands/check-ci.md/check-cicrates/trusted-server-integration-tests/tests/integration.rstest_cloudflare_enabled_auction_reaches_telemetrycrates/trusted-server-integration-tests/tests/common/config.rscrates/trusted-server-integration-tests/tests/environments/cloudflare.rsspawn_with_config_jsonto start wrangler with a custom configcrates/trusted-server-adapter-cloudflare/.gitignorewrangler.integration.generated.tomlNotes:
disallowed-methodsrule in the workspaceclippy.tomlcan't work: on native andwasm32-wasip1,web_time::Instantandweb_time::SystemTimeare the std types, so the rule would also flag every correctweb_timecall. Onlywasm32-unknown-unknownhas distinct types, so the lint runs on that target alone. It covers every crate built into the Cloudflare Worker, including the adapter. Theelapsedmethods are denied too, since std builds them on the panickingnow().web_timekeeps that code safe on Cloudflare, and the lint guards it.chrono::Utc::now()calls. chrono'swasmbindfeature, now declared explicitly forwasm32-unknown-unknown, reads the JS clock there.performance.now()only advances after I/O, so Cloudflareelapsed_msvalues aren't comparable with the other adapters' wall-clock values (noted onAuctionObservationContext::elapsed_ms).Closes
Closes #1075
Closes #1240
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(Node 24.12.0)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm && cargo clippy-cli && cargo clippy-codegencargo test-cloudflare && cargo test-spin, pluscargo test-fastly-reuseand the build-digest test and lint after mergingmainscripts/lint-wasm-clock.shpasses; withstd::time::Instantrestored inauction/telemetry.rsit fails withuse of a disallowed method std::time::Instant::nowAGENTS.md(covers theAGENTS.mdedit)cargo clippy --manifest-path crates/trusted-server-integration-tests/Cargo.toml --all-targets -- -D warningscargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycrates/trusted-server-adapter-cloudflare/build.shand rantest_cloudflare_enabled_auction_reaches_telemetryagainst workerd (wrangler dev): passes. Withstd::time::Instantrestored, the worker panics with "time not implemented on this platform" and the test fails.Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)