feat: add support to debug workers [DRAFT] #31527
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe PR switches the workspace Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cli/lib/worker.rs
Outdated
| fs: Arc<dyn deno_fs::FileSystem>, | ||
| maybe_coverage_dir: Option<PathBuf>, | ||
| maybe_inspector_server: Option<Arc<InspectorServer>>, | ||
| main_inspector_session_tx: std::sync::Mutex<Option<deno_core::futures::channel::mpsc::UnboundedSender<deno_core::InspectorSessionProxy>>>, |
There was a problem hiding this comment.
Please use a wrapper type a give it a comment to explain what it does - I can't really figure it out from main_inspector_session_tx. IS it meant to be used by workers to forward their proxies?
runtime/web_worker.rs
Outdated
| deno_core::futures::channel::mpsc::unbounded::<String>(); | ||
|
|
||
| let main_to_worker_sync_tx_clone = main_to_worker_sync_tx.clone(); | ||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
Can you explain why do we need another thread for this?
fraidev
force-pushed
the
debug_worker
branch
2 times, most recently
from
2c53e80 to
27c4f8b
Compare
fraidev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli/lib/worker.rs (1)
376-377: Add a wrapper type with documentation as previously requested.A previous review comment requested a wrapper type with documentation to explain the purpose of
main_inspector_session_tx. This helps clarify that workers use this channel to register their inspector session proxies with the main runtime.+/// Channel for workers to send their inspector session proxies to the main runtime. +/// When a web worker is created, it creates a pair of inspector session proxies and +/// sends one to the main runtime via this channel, enabling debugger communication +/// between the main thread and workers. +struct MainInspectorSessionSender(UnboundedSender<InspectorSessionProxy>); + struct LibWorkerFactorySharedState<TSys: DenoLibSys> { // ... other fields ... - main_inspector_session_tx: - Mutex<Option<UnboundedSender<InspectorSessionProxy>>>, + main_inspector_session_tx: Mutex<Option<MainInspectorSessionSender>>, // ... other fields ... }
🧹 Nitpick comments (1)
runtime/inspector_server.rs (1)
428-436: Remove commented-out debug code.These commented
eprintln!statements appear to be debug artifacts from development. Remove them before merging.OpCode::Text => { if let Ok(s) = String::from_utf8(msg.payload.to_vec()) { - // eprintln!("Debugger sent: {}", s); let _ = inbound_tx.unbounded_send(s); } } OpCode::Close => { // Users don't care if there was an error coming from debugger, // just about the fact that debugger did disconnect. log::info!("Debugger session ended"); - // eprintln!("Debugger session ended"); break 'pump; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(1 hunks)cli/lib/worker.rs(5 hunks)runtime/inspector_server.rs(3 hunks)runtime/web_worker.rs(5 hunks)runtime/worker.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
runtime/worker.rscli/lib/worker.rsruntime/web_worker.rsruntime/inspector_server.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
runtime/worker.rscli/lib/worker.rsruntime/web_worker.rsruntime/inspector_server.rs
runtime/worker.rs
📄 CodeRabbit inference engine (CLAUDE.md)
runtime/worker.rs: Worker and runtime initialization is implemented inruntime/worker.rs
Register new extensions inruntime/worker.rsafter creating them inext/<extension_name>/
Files:
runtime/worker.rs
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Worker and runtime initialization is implemented in `runtime/worker.rs`
Applied to files:
runtime/worker.rscli/lib/worker.rsruntime/web_worker.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Register new extensions in `runtime/worker.rs` after creating them in `ext/<extension_name>/`
Applied to files:
runtime/worker.rsruntime/web_worker.rs
🧬 Code graph analysis (3)
runtime/worker.rs (2)
ext/node/ops/v8.rs (1)
None(312-312)ext/napi/node_api.rs (1)
None(816-816)
cli/lib/worker.rs (1)
runtime/inspector_server.rs (3)
mpsc(70-70)new(65-98)new(464-480)
runtime/web_worker.rs (2)
runtime/inspector_server.rs (1)
mpsc(70-70)cli/lsp/tsc.rs (1)
mpsc(427-427)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (4)
runtime/worker.rs (1)
1123-1123: LGTM!Setting
worker_id: Nonefor the main worker is correct since worker IDs are only relevant for web workers.runtime/inspector_server.rs (1)
197-205: LGTM!The updated
InspectorSessionProxyconstruction usingInspectorSessionChannels::Regularaligns with the new deno_core API for channel-based session handling.runtime/web_worker.rs (1)
658-658: LGTM!Setting
worker_id: Some(options.worker_id.0)correctly identifies web workers in the runtime options.cli/lib/worker.rs (1)
739-742: Verify this is the intended behavior for inspector session sharing.This captures the main worker's inspector session sender and stores it for workers to use. Confirm that:
- The main inspector can handle sessions from multiple workers
- The session sender remains valid for the lifetime of the worker factory
| # REMOVE IT | ||
| # TEMPORARY, JUST TO TEST WITH CI | ||
| deno_core = { git = "https://github.com/fraidev/deno_core.git", branch = "debug_worker" } | ||
| # deno_core = { version = "0.370.0" } | ||
| # deno_core = { path = "../deno_core/core/" } |
There was a problem hiding this comment.
This must be reverted before merging.
The git-based dependency on a forked branch is clearly marked as temporary for CI testing. Before this PR can be merged:
- The
deno_corechanges need to be merged upstream - A new version of
deno_coreneeds to be published - This dependency must be switched back to a version pin
🤖 Prompt for AI Agents
In Cargo.toml around lines 64-68, the deno_core dependency is temporarily
pointing to a forked git branch for CI; revert this before merging by removing
the git-based entry and replacing it with a version-pinned dependency (e.g.,
deno_core = "x.y.z") once the upstream changes are merged and a new deno_core
release is published; ensure you wait for upstream merge + published version,
update the Cargo.toml to the published version, run cargo update/cargo build to
verify, and remove any leftover comments referencing the temporary CI
workaround.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/testdata/inspector/multi_worker_main.js (1)
14-28: Consider extracting duplicate handler logic.Both
onmessagehandlers have identical logic for tracking readiness. You could DRY this up:+function handleWorkerMessage(workerName, e) { + console.log(`${workerName} message:`, e.data); + if (e.data === "worker_ready") { + readyCount++; + if (readyCount === 2) console.log("all_workers_ready"); + } +} + -worker1.onmessage = (e) => { - console.log("Worker1 message:", e.data); - if (e.data === "worker_ready") { - readyCount++; - if (readyCount === 2) console.log("all_workers_ready"); - } -}; - -worker2.onmessage = (e) => { - console.log("Worker2 message:", e.data); - if (e.data === "worker_ready") { - readyCount++; - if (readyCount === 2) console.log("all_workers_ready"); - } -}; +worker1.onmessage = (e) => handleWorkerMessage("Worker1", e); +worker2.onmessage = (e) => handleWorkerMessage("Worker2", e);That said, for test data scripts, explicit duplication is often fine for readability.
tests/integration/inspector_tests.rs (1)
1881-1883: Remove hardcoded line number references from comments.Comments like "This tests the handler at inspector.rs lines 1198-1216" will become stale as the codebase evolves. Consider removing the specific line numbers or replacing with a more durable reference (e.g., function/method name).
- // Test 1: Verify NodeWorker.enable sends a response - // This tests the handler at inspector.rs lines 1198-1216 + // Test 1: Verify NodeWorker.enable sends a response- // Test 2: Verify NodeWorker.sendMessageToWorker sends a response - // This tests the handler at inspector.rs lines 1218-1229 + // Test 2: Verify NodeWorker.sendMessageToWorker sends a responseAlso applies to: 1919-1921
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/integration/inspector_tests.rs(3 hunks)tests/testdata/inspector/multi_worker_main.js(1 hunks)tests/testdata/inspector/worker_main.js(1 hunks)tests/testdata/inspector/worker_target.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/testdata/inspector/multi_worker_main.jstests/testdata/inspector/worker_target.jstests/testdata/inspector/worker_main.js
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
tests/integration/inspector_tests.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
tests/integration/inspector_tests.rs
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Worker and runtime initialization is implemented in `runtime/worker.rs`
Applied to files:
tests/testdata/inspector/multi_worker_main.jstests/testdata/inspector/worker_main.js
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to **/*.{ts,tsx,js} : For JavaScript runtime debugging, enable V8 inspector with `--inspect-brk` flag and connect Chrome DevTools to `chrome://inspect`
Applied to files:
tests/testdata/inspector/worker_main.js
🧬 Code graph analysis (2)
tests/testdata/inspector/multi_worker_main.js (1)
cli/tsc/dts/node/cluster.d.cts (1)
Worker(136-389)
tests/integration/inspector_tests.rs (1)
cli/tools/repl/session.rs (2)
serde_json(320-320)serde_json(322-324)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-aarch64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (8)
tests/testdata/inspector/worker_target.js (1)
1-10: LGTM!Clean, minimal worker script for inspector testing. Correctly signals readiness and echoes messages.
tests/testdata/inspector/worker_main.js (1)
1-15: LGTM!Standard pattern for inspector test scripts - spawns worker, wires up handlers, and keeps alive indefinitely for debugger interaction.
tests/integration/inspector_tests.rs (6)
902-966: LGTM!The helper functions properly filter script messages by URL field rather than arbitrary string matching, which correctly handles V8's inclusion of stack traces that may reference test files.
946-1013: LGTM!The collection loop correctly handles non-deterministic CDP message ordering. The conditional check for
debugger_responseavoids waiting for a message that may have already arrived.
1504-1614: LGTM!Test correctly validates multi-worker auto-attach scenario. The collection loops with bounded iterations and flexible message matching handle CDP's async nature well.
1616-1720: LGTM!Test validates target discovery flow correctly. The check for
Target.targetCreatedwith "worker" is sufficient for test purposes.
1722-1855: LGTM!Good end-to-end test that validates auto-attach and then verifies bidirectional communication with the worker session.
1504-1505: All new worker inspector tests are marked flaky.This is appropriate given the non-deterministic CDP message ordering, but worth noting that these may need stabilization before the PR exits draft status. The underlying implementation should ideally make these tests deterministic.
Also applies to: 1616-1617, 1722-1723, 1860-1861
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/integration/inspector_tests.rs (2)
1207-1278: Same duplication pattern as inspector_memory.This test uses the same boolean flags + loop approach. See comment on inspector_memory test for refactoring suggestion.
1664-1768: Same testdata verification needed.This test also relies on
inspector/worker_main.js. Same verification comment applies as inspector_multiple_workers.
🧹 Nitpick comments (1)
tests/integration/inspector_tests.rs (1)
1089-1135: Consider extracting message collection pattern.The boolean flags + loop pattern appears in multiple tests (inspector_memory, inspector_profile, inspector_multiple_workers, etc.). While functional, this creates significant duplication.
Consider a helper that takes a list of message prefixes and returns when all are found:
async fn collect_messages( tester: &mut InspectorTester, expected: &[&str], max_attempts: usize, ) -> Vec<String> { let mut found = vec![false; expected.len()]; let mut messages = Vec::new(); for _ in 0..max_attempts { let msg = tester.recv().await; for (i, prefix) in expected.iter().enumerate() { if !found[i] && msg.starts_with(prefix) { found[i] = true; messages.push(msg); } } if found.iter().all(|&f| f) { break; } } for (i, prefix) in expected.iter().enumerate() { assert!(found[i], "Expected message starting with: {}", prefix); } messages }This would reduce the ~30 lines of flag tracking to a single call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/inspector_tests.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
tests/integration/inspector_tests.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
tests/integration/inspector_tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (3)
tests/integration/inspector_tests.rs (3)
902-918: LGTM - Helper function correctly filters by URL field.The function properly parses the CDP message and checks the actual URL field rather than relying on string matching, which is more robust.
1877-1900: Good coverage of worker session communication.Unlike the previous worker tests, this one actually sends a CDP message to the worker session and verifies the response, which provides better functional validation.
1552-1662: Unable to verify testdata file and timeout configuration without repository access.The review comment raises valid concerns about the existence of
inspector/multi_worker_main.jsand whether the 25-iteration limit is sufficient for collecting worker attachment events, but these cannot be definitively verified due to repository access constraints. Manual review of the test implementation and empirical CI results would be needed to confirm if the iteration counts are adequate for the async worker spawn timing.
| // Collect scriptParsed messages for test files until we have all 3. | ||
| // With recent V8 changes, internal scripts may include stackTrace with test file references, | ||
| // so we need to filter by the actual URL field rather than just string matching. | ||
| let mut scripts = Vec::new(); | ||
| let mut debugger_response = None; | ||
| while scripts.len() < 3 { | ||
| let msg = tester.recv().await; | ||
| if is_test_script(&msg) { | ||
| scripts.push(msg); | ||
| } else if msg.starts_with(r#"{"id":2,"result":{"debuggerId":"#) { | ||
| debugger_response = Some(msg); | ||
| } | ||
| // Ignore other scriptParsed messages (internal scripts) | ||
| } |
There was a problem hiding this comment.
Add upper bound to prevent infinite loop.
The while scripts.len() < 3 loop has no maximum iteration limit. If expected messages don't arrive (e.g., test file change, runtime issue), this will hang indefinitely rather than failing with a timeout.
Add a maximum iteration count:
- while scripts.len() < 3 {
+ let mut attempts = 0;
+ while scripts.len() < 3 && attempts < 50 {
let msg = tester.recv().await;
+ attempts += 1;
if is_test_script(&msg) {
scripts.push(msg);
} else if msg.starts_with(r#"{"id":2,"result":{"debuggerId":"#) {
debugger_response = Some(msg);
}
// Ignore other scriptParsed messages (internal scripts)
}
+ assert_eq!(scripts.len(), 3, "Expected to collect 3 test scripts, got {}", scripts.len());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Collect scriptParsed messages for test files until we have all 3. | |
| // With recent V8 changes, internal scripts may include stackTrace with test file references, | |
| // so we need to filter by the actual URL field rather than just string matching. | |
| let mut scripts = Vec::new(); | |
| let mut debugger_response = None; | |
| while scripts.len() < 3 { | |
| let msg = tester.recv().await; | |
| if is_test_script(&msg) { | |
| scripts.push(msg); | |
| } else if msg.starts_with(r#"{"id":2,"result":{"debuggerId":"#) { | |
| debugger_response = Some(msg); | |
| } | |
| // Ignore other scriptParsed messages (internal scripts) | |
| } | |
| // Collect scriptParsed messages for test files until we have all 3. | |
| // With recent V8 changes, internal scripts may include stackTrace with test file references, | |
| // so we need to filter by the actual URL field rather than just string matching. | |
| let mut scripts = Vec::new(); | |
| let mut debugger_response = None; | |
| let mut attempts = 0; | |
| while scripts.len() < 3 && attempts < 50 { | |
| let msg = tester.recv().await; | |
| attempts += 1; | |
| if is_test_script(&msg) { | |
| scripts.push(msg); | |
| } else if msg.starts_with(r#"{"id":2,"result":{"debuggerId":"#) { | |
| debugger_response = Some(msg); | |
| } | |
| // Ignore other scriptParsed messages (internal scripts) | |
| } | |
| assert_eq!(scripts.len(), 3, "Expected to collect 3 test scripts, got {}", scripts.len()); |
🤖 Prompt for AI Agents
In tests/integration/inspector_tests.rs around lines 946 to 959, the while
scripts.len() < 3 loop can hang forever if the expected messages never arrive;
add a maximum iteration count (e.g., const MAX_ITER: usize = 1000) and a loop
counter that increments each iteration, and when the counter exceeds MAX_ITER
break out and fail the test with a clear error (panic! or return Err) that
includes scripts.len() and whether debugger_response was seen so the failure is
informative; ensure the counter check runs each loop before awaiting another
message to prevent infinite wait.
9d2c747 to
c668f58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
runtime/web_worker.rs (1)
681-696: Handle potential send failures gracefully.The
unbounded_send().unwrap()calls will panic if receivers are dropped. While a dropped receiver during worker init likely indicates a fatal issue, logging and continuing would be more resilient.if let Some(main_session_tx) = services.main_inspector_session_tx { let (main_proxy, worker_proxy) = deno_core::create_worker_inspector_session_pair( options.main_module.to_string(), ); // Send worker proxy to the main runtime - main_session_tx.unbounded_send(main_proxy).unwrap(); + if main_session_tx.unbounded_send(main_proxy).is_err() { + log::warn!("Failed to send inspector session proxy to main runtime"); + } // Send worker proxy to the worker runtime - js_runtime + if js_runtime .inspector() .get_session_sender() .unbounded_send(worker_proxy) - .unwrap(); + .is_err() + { + log::warn!("Failed to send inspector session proxy to worker runtime"); + } }cli/lib/worker.rs (1)
376-377: Consider using a wrapper type with documentation.A previous review requested a wrapper type to clarify the purpose of this field. The raw
Mutex<Option<UnboundedSender<InspectorSessionProxy>>>doesn't convey that this channel allows workers to forward their inspector session proxies to the main runtime for debugging./// Channel for workers to register their inspector sessions with the main runtime. /// When set, workers will create an inspector session pair and send the main-facing /// proxy through this channel, enabling the main runtime to debug workers. struct MainInspectorSessionSender(Option<UnboundedSender<InspectorSessionProxy>>);tests/integration/inspector_tests.rs (2)
1905-1995: NodeWorker tests are a solid start; consider exercising a real worker sessionThe
inspector_node_worker_enabletest correctly verifies:
NodeWorker.enablereturns a response alongside Runtime/Debugger setup.NodeWorker.sendMessageToWorkerreturns a response even when targeting a non-existent session.However, this still doesn't prove that messages are successfully delivered to actual worker sessions; it only validates that the handler responds. A prior review already suggested deepening this coverage by:
- Resuming the main script to spawn a worker.
- Capturing a real worker session ID (via
Target.attachedToTargetor NodeWorker notifications).- Sending
NodeWorker.sendMessageToWorkerto that session.- Asserting that the worker receives/acts on the message.
This would turn the test into a full end-to-end validation of the NodeWorker protocol implementation. Fine to defer if you're just getting the basic plumbing in first, but worth tracking.
946-960: Bound the script collection loop to avoid pathological hangs
while scripts.len() < 3has no iteration cap. If, for any reason, fewer than 3 matchingscriptParsedevents are ever produced, this loop will keep draining messages until the connection errors out, which can make the test slow and hard to diagnose. This was already called out in a prior review; recommending you address it now.Suggested fix:
- let mut scripts = Vec::new(); - let mut debugger_response = None; - while scripts.len() < 3 { - let msg = tester.recv().await; + let mut scripts = Vec::new(); + let mut debugger_response = None; + let mut attempts = 0usize; + const MAX_ATTEMPTS: usize = 50; + while scripts.len() < 3 && attempts < MAX_ATTEMPTS { + let msg = tester.recv().await; + attempts += 1; if is_test_script(&msg) { scripts.push(msg); } else if msg.starts_with(r#"{"id":2,"result":{"debuggerId":"#) { debugger_response = Some(msg); } // Ignore other scriptParsed messages (internal scripts) } + assert_eq!( + scripts.len(), + 3, + "Expected 3 scriptParsed events for test.ts, foo.ts, bar.js; got {}", + scripts.len() + );
🧹 Nitpick comments (2)
runtime/inspector_server.rs (1)
428-436: Remove commented-out debug code before merge.These
eprintln!lines appear to be debugging artifacts. Remove them or convert to properlog::debug!calls if needed for production debugging.if let Ok(s) = String::from_utf8(msg.payload.to_vec()) { - // eprintln!("Debugger sent: {}", s); let _ = inbound_tx.unbounded_send(s); } } OpCode::Close => { // Users don't care if there was an error coming from debugger, // just about the fact that debugger did disconnect. log::info!("Debugger session ended"); - // eprintln!("Debugger session ended"); break 'pump; }tests/integration/inspector_tests.rs (1)
1664-1768: Tighten worker target detection to avoid string-based false positives
inspector_worker_target_discoverycurrently detects worker targets with:if msg.contains("Target.targetCreated") && msg.contains("worker") { found_worker_target = true; }This can, in theory, be tripped by unrelated payloads that happen to contain
"worker"somewhere in the JSON.You could make this more robust by parsing the message and checking
targetInfo.typeexplicitly, similar to how other tests parse JSON:- if msg.contains("Target.targetCreated") && msg.contains("worker") { - found_worker_target = true; - } + if msg.contains("Target.targetCreated") { + if let Ok(v) = serde_json::from_str::<serde_json::Value>(&msg) { + if v["params"]["targetInfo"]["type"] == "worker" { + found_worker_target = true; + } + } + }Not urgent, but it would make the test more precise and future-proof.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(1 hunks)cli/lib/worker.rs(5 hunks)runtime/inspector_server.rs(3 hunks)runtime/web_worker.rs(5 hunks)runtime/worker.rs(1 hunks)tests/integration/inspector_tests.rs(5 hunks)tests/testdata/inspector/multi_worker_main.js(1 hunks)tests/testdata/inspector/worker_main.js(1 hunks)tests/testdata/inspector/worker_target.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/testdata/inspector/worker_main.js
- Cargo.toml
- tests/testdata/inspector/multi_worker_main.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
runtime/inspector_server.rstests/integration/inspector_tests.rscli/lib/worker.rsruntime/web_worker.rsruntime/worker.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
runtime/inspector_server.rstests/integration/inspector_tests.rscli/lib/worker.rsruntime/web_worker.rsruntime/worker.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/testdata/inspector/worker_target.js
runtime/worker.rs
📄 CodeRabbit inference engine (CLAUDE.md)
runtime/worker.rs: Worker and runtime initialization is implemented inruntime/worker.rs
Register new extensions inruntime/worker.rsafter creating them inext/<extension_name>/
Files:
runtime/worker.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Worker and runtime initialization is implemented in `runtime/worker.rs`
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Worker and runtime initialization is implemented in `runtime/worker.rs`
Applied to files:
cli/lib/worker.rsruntime/web_worker.rsruntime/worker.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Register new extensions in `runtime/worker.rs` after creating them in `ext/<extension_name>/`
Applied to files:
runtime/web_worker.rsruntime/worker.rs
🧬 Code graph analysis (2)
tests/integration/inspector_tests.rs (1)
tests/util/server/src/lib.rs (1)
deno_cmd(583-586)
cli/lib/worker.rs (1)
runtime/inspector_server.rs (3)
mpsc(70-70)new(65-98)new(464-480)
🔇 Additional comments (14)
runtime/worker.rs (1)
1123-1123: LGTM!Setting
worker_id: Nonefor the main worker is correct - this field distinguishes the main runtime from web workers, which will haveSome(id).tests/testdata/inspector/worker_target.js (1)
1-10: LGTM!Clean and minimal test worker script. The ready signal pattern is useful for synchronizing test setup.
runtime/inspector_server.rs (1)
197-205: LGTM!The migration to
InspectorSessionChannels::Regularwrapper is correct and aligns with the updated deno_core API.runtime/web_worker.rs (2)
381-382: LGTM!The
main_inspector_session_txfield enables inspector session sharing between main and worker runtimes.
658-658: LGTM!Setting
worker_id: Some(options.worker_id.0)correctly identifies this runtime as a web worker to deno_core.cli/lib/worker.rs (2)
739-742: Verify ordering: session sender stored after bootstrap.This stores the inspector session sender after
bootstrap_from_optionscompletes. Since web workers are created via JS callbacks (which run after bootstrap), the sender should be available when workers are created. However, if any future code creates workers during bootstrap, they won't have access to this channel.
466-469: LGTM!The
lock().clone()pattern is correct - it acquires the lock, clones theOption<UnboundedSender>(which is cheap sinceUnboundedSenderisClone), and releases the lock immediately.tests/integration/inspector_tests.rs (7)
902-918: Robust scriptParsed filtering helper looks goodUsing
serde_json+ URL inspection inis_test_scriptto distinguish real testdata/inspector scripts from internal ones is a solid way to avoid stackTrace-based false positives. No changes requested.
961-1003: Script selection by URL is clear and appropriately strictUsing
get_script_url+contains("testdata/inspector/...")to pick outtest.ts,foo.ts, andbar.jsfrom the collectedscriptParsedmessages is straightforward and gives nice failure messages via theexpect("should have ...")calls. No changes requested.
1008-1013: Debugger.enable response handling is correct but slightly asymmetricConditionally waiting for the
{"id":2,"result":{"debuggerId":...}}response only if it was not seen during script collection is a reasonable way to keep ordering-agnostic behavior without over-consuming messages. Just be aware this assumes there is exactly one such response, which matches the current protocol.
1089-1136: Batching setup + tolerant collection for memory test looks goodThe new approach in
inspector_memoryof sending Runtime/Debugger/runIfWaitingForDebugger/HeapProfiler enables in one batch and then collecting responses/notifications via boolean flags is much more robust to CDP reordering than strict sequential asserts. Once theDebugger.pausedprefix bug above is fixed, this structure looks solid.
1207-1253: Profile test uses the same robust pattern; OK afterDebugger.pausedfix
inspector_profilemirrors the memory test pattern (batched enables + boolean flags + bounded loops). This should significantly reduce flakiness from out-of-order CDP messages. After correcting theDebugger.pausedstarts_withliteral, the logic is sound.Also applies to: 1262-1278
1552-1662: Multiple-worker auto-attach test structure looks good
inspector_multiple_workersexercisesTarget.setAutoAttachwithflatten: true, then:
- Waits for all the expected setup responses + initial
Debugger.paused.- Resumes, collects
Target.attachedToTargetnotifications, and extracts workersessionIds.- Asserts exactly two worker sessions were attached.
This is a good end-to-end check for multi-worker auto-attach and tolerates message reordering via the boolean flags and bounded loops. Once the
Debugger.pausedprefix issue is fixed, this test looks in good shape.
1770-1903: Worker auto-attach + session messaging test is well designed
inspector_worker_auto_attach:
- Enables Runtime/Debugger/Target.autoAttach + run in one batch with ordering-tolerant collection.
- Resumes, then waits for
Target.attachedToTargetand captures a workersessionId.- Sends a
Runtime.enablewith thatsessionIdand verifies a response containing both"id":6and the sessionId.This gives a good integration-level check that auto-attach works and that messages can be routed into the worker session. Once the shared
Debugger.pausedprefix is fixed, this test looks solid.
fraidev
force-pushed
the
debug_worker
branch
from
406bae9 to
8e0f405
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/integration/inspector_tests.rs (1)
946-959: Unbounded loop can hang the test.This
while scripts.len() < 3loop has no iteration limit. Whiletester.recv()has a 60-second timeout, the test would wait up to 3 minutes if expected messages never arrive. Add a max iteration count for faster, more informative failures.+ let mut attempts = 0; + const MAX_ATTEMPTS: usize = 50; - while scripts.len() < 3 { + while scripts.len() < 3 && attempts < MAX_ATTEMPTS { let msg = tester.recv().await; + attempts += 1; if is_test_script(&msg) { scripts.push(msg); } else if msg.starts_with(r#"{"id":2,"result":{"debuggerId":"#) { debugger_response = Some(msg); } // Ignore other scriptParsed messages (internal scripts) } + assert!( + scripts.len() >= 3, + "Expected 3 test scripts, got {} after {} attempts", + scripts.len(), + attempts + );
🧹 Nitpick comments (1)
tests/integration/inspector_tests.rs (1)
902-918: Helper function placement could be improved.
is_test_scriptis defined inside the test function butnotification_filter(line 920) performs similar logic. Consider either consolidating them or moving them outside the test for reuse by other worker-related inspector tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
runtime/web_worker.rs(5 hunks)tests/integration/inspector_tests.rs(8 hunks)tests/testdata/inspector/multi_worker_main.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
runtime/web_worker.rstests/integration/inspector_tests.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
runtime/web_worker.rstests/integration/inspector_tests.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/testdata/inspector/multi_worker_main.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Worker and runtime initialization is implemented in `runtime/worker.rs`
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Worker and runtime initialization is implemented in `runtime/worker.rs`
Applied to files:
runtime/web_worker.rstests/testdata/inspector/multi_worker_main.js
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to runtime/worker.rs : Register new extensions in `runtime/worker.rs` after creating them in `ext/<extension_name>/`
Applied to files:
runtime/web_worker.rs
🧬 Code graph analysis (1)
tests/testdata/inspector/multi_worker_main.js (1)
cli/tsc/dts/node/cluster.d.cts (1)
Worker(136-389)
🔇 Additional comments (9)
tests/integration/inspector_tests.rs (5)
1564-1674: New multi-worker test looks solid.The test properly validates Target.setAutoAttach flow and verifies two worker sessions are created. The batched command sending and out-of-order message collection is appropriate for CDP's async nature.
1676-1780: Target discovery test implementation is correct.Validates Target.setDiscoverTargets with proper response collection and verifies Target.targetCreated is received for the spawned worker.
1782-1915: Auto-attach test with session communication is thorough.Good coverage: enables auto-attach, verifies worker attachment, then validates bidirectional communication by sending Runtime.enable to the worker session and checking for a response.
1917-2007: NodeWorker.enable test verifies protocol handlers exist.Tests that NodeWorker.enable and NodeWorker.sendMessageToWorker return responses. The sendMessageToWorker test with a non-existent session is limited but acceptable for verifying the handler doesn't crash.
1089-1141: Out-of-order message collection pattern is good.The refactored approach of collecting multiple expected responses/notifications in a single loop handles CDP's non-deterministic ordering well. The explicit assertions for each expected message improve debuggability.
tests/testdata/inspector/multi_worker_main.js (1)
1-40: Test fixture is appropriate for multi-worker inspector testing.Spawns two workers, tracks readiness, and keeps alive for debugger inspection. The code is straightforward and serves its purpose.
runtime/web_worker.rs (3)
681-701: Inspector session proxy wiring looks correct with graceful error handling.The past review concern about
unwrap()panics has been addressed. Failures now log warnings and continue, which is appropriate for an optional debugging feature. The proxy pair correctly links main and worker runtime inspector sessions.
658-658: Worker ID propagation enables per-worker inspector identification.Passing
worker_idtoRuntimeOptionsallows the inspector to distinguish between different worker instances.
382-382: Unable to verify: Repository access required.The verification of call sites constructing
WebWorkerServiceOptionscould not be completed due to sandbox environment limitations. Please verify locally that all instantiations ofWebWorkerServiceOptionsinclude the newmain_inspector_session_txfield.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration/inspector_tests.rs (1)
949-962: Add iteration limit to prevent slow failures.This loop lacks a maximum iteration count. While
recv()has a 60-second timeout preventing infinite hangs, the test could run for a long time if expected messages don't arrive. A past review comment suggested this fix.Add a loop counter and max iterations:
+ let max_attempts = 50; + let mut attempts = 0; - while scripts.len() < 3 { + while scripts.len() < 3 && attempts < max_attempts { let msg = tester.recv().await; + attempts += 1; if is_test_script(&msg) { scripts.push(msg); } else if msg.starts_with(r#"{"id":2,"result":{"debuggerId":"#) { debugger_response = Some(msg); } - // Ignore other scriptParsed messages (internal scripts) } + assert_eq!(scripts.len(), 3, "Expected 3 test scripts, got {} after {} attempts", scripts.len(), attempts);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
tests/integration/inspector_tests.rs(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
tests/integration/inspector_tests.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
tests/integration/inspector_tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test debug macos-x86_64
- GitHub Check: bench release linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: build libs
- GitHub Check: test debug windows-x86_64
- GitHub Check: test release macos-aarch64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (6)
tests/integration/inspector_tests.rs (6)
206-206: LGTM on wait() calls after kill().Adding
.wait()after.kill()ensures proper cleanup and prevents zombie processes. Good fix.Also applies to: 430-430, 760-760, 1565-1565
905-921: Good refactor to use URL field filtering.Using JSON parsing to check the actual
urlfield inparamsis more reliable than string matching the entire message. This correctly filters test scripts from internal V8 scripts.
1568-1679: Good coverage for multiple worker debugging.This test properly validates that multiple workers can be attached and debugged. The batching approach and flexible message collection handles timing issues well. The use of
flaky_testis appropriate for worker scenarios.(Note: The
Debugger.pausedbug at line 1624 is flagged separately)
1681-1786: Good coverage for target discovery.This test validates the
Target.setDiscoverTargetsAPI correctly notifies about worker creation. Consistent structure with other worker tests.(Note: The
Debugger.pausedbug at line 1737 is flagged separately)
1788-1922: Good end-to-end validation for auto-attach.This test validates the complete flow: enabling auto-attach, receiving the worker session, and successfully communicating with it. The session message exchange proves the worker session is functional.
(Note: The
Debugger.pausedbug at line 1844 is flagged separately)
1924-2015: Good basic coverage for NodeWorker protocol.This test validates the NodeWorker domain handlers (Node.js-style inspector protocol). The test intentionally focuses on protocol-level responses rather than full end-to-end worker interaction, which is appropriate for initial coverage of these handlers.
| if msg.starts_with(r#"{"method":"Debugger.paused""#) { | ||
| got_paused = true; |
There was a problem hiding this comment.
CRITICAL: Malformed Debugger.paused prefix will never match.
This bug was flagged in a previous review but remains unfixed. The string literal contains an extra " after Debugger.paused, so it will never match real CDP messages like {"method":"Debugger.paused","params":...}. The got_paused flag will stay false and assertions will fail even when pause events are actually received.
This affects 5 tests:
inspector_memory(line 1126)inspector_profile(line 1250)inspector_multiple_workers(line 1624)inspector_worker_target_discovery(line 1737)inspector_worker_auto_attach(line 1844)
Fix by removing the extra quotation mark:
- if msg.starts_with(r#"{"method":"Debugger.paused""#) {
+ if msg.starts_with(r#"{"method":"Debugger.paused","#) {Apply this fix to all 5 locations.
Also applies to: 1250-1251, 1624-1625, 1737-1738, 1844-1845
🤖 Prompt for AI Agents
In tests/integration/inspector_tests.rs around lines 1126-1127, 1250-1251,
1624-1625, 1737-1738, and 1844-1845 the string literal used to detect a
Debugger.paused CDP message contains an extra double-quote after Debugger.paused
so the prefix will never match real messages; remove the stray double-quote in
each location so the starts_with check uses r#"{"method":"Debugger.paused"#
(i.e. no extra quote) and re-run the tests to confirm got_paused correctly
becomes true when pause events arrive.
a51625e to
16aa66b
Compare
fraidev
force-pushed
the
debug_worker
branch
from
16aa66b to
a52851e
Compare
2 participants
(still a draft, just opened to run CI)