Skip to content

Commit b9434ee

Browse files
bartlomiejuclaude
andauthored
fix(repl): surface CDP protocol errors and fix race in wait_for_response (#33190)
## Summary - The REPL's `wait_for_response` silently swallowed CDP error responses (responses with an `"error"` field instead of `"result"`), converting them to `null`. This made the `npm_packages` test failure on Windows ARM64 impossible to diagnose — the actual V8 error was lost and replaced by a confusing serde message (`invalid type: null, expected struct EvaluateResponse`). - Log CDP protocol errors via `eprintln!` so they appear in test stderr output on the next CI failure - Fix TOCTOU race: hold the lock across both the "check if ready" and "register WaitingFor" steps in `wait_for_response` to prevent a response from being overwritten between the two operations - Apply the same fixes to the duplicated code in `hmr.rs` ## Motivation The `integration::repl::npm_packages` test has become nearly constantly failing on Windows ARM64 CI ([example](https://github.com/denoland/deno/actions/runs/24003416554/job/70004374515)). The error output is: ``` "undefined\nerror: invalid type: null, expected struct EvaluateResponse\n" does not contain any of ["hello"] ``` This means V8's `Runtime.evaluate` returned a CDP protocol error, but the actual error message was discarded. With this change, the next CI failure will print the real V8 error to stderr, allowing us to identify and fix the root cause. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 11db67a commit b9434ee

File tree

3 files changed

+70
-32
lines changed

3 files changed

+70
-32
lines changed

cli/tools/repl/session.rs

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -260,21 +260,35 @@ impl ReplSessionState {
260260
}
261261

262262
async fn wait_for_response(&self, msg_id: i32) -> serde_json::Value {
263-
if let Some(message_state) = self.0.lock().messages.remove(&msg_id) {
264-
let InspectorMessageState::Ready(mut value) = message_state else {
265-
unreachable!();
266-
};
267-
return value["result"].take();
268-
}
263+
let rx = {
264+
let mut state = self.0.lock();
265+
if let Some(message_state) = state.messages.remove(&msg_id) {
266+
let InspectorMessageState::Ready(value) = message_state else {
267+
unreachable!();
268+
};
269+
return Self::extract_result(value);
270+
}
271+
let (tx, rx) = oneshot::channel();
272+
state
273+
.messages
274+
.insert(msg_id, InspectorMessageState::WaitingFor(tx));
275+
rx
276+
};
269277

270-
let (tx, rx) = oneshot::channel();
271-
self
272-
.0
273-
.lock()
274-
.messages
275-
.insert(msg_id, InspectorMessageState::WaitingFor(tx));
276-
let mut value = rx.await.unwrap();
277-
value["result"].take()
278+
let value = rx.await.unwrap();
279+
Self::extract_result(value)
280+
}
281+
282+
#[allow(clippy::print_stderr, reason = "diagnostic for flaky CDP responses")]
283+
fn extract_result(mut value: serde_json::Value) -> serde_json::Value {
284+
let result = value["result"].take();
285+
if result.is_null() {
286+
eprintln!(
287+
"CDP response has null result. Full response: {}",
288+
serde_json::to_string(&value).unwrap_or_default()
289+
);
290+
}
291+
result
278292
}
279293
}
280294

@@ -410,6 +424,17 @@ impl ReplSession {
410424
) -> Value {
411425
let msg_id = next_msg_id();
412426
self.session.post_message(msg_id, method, params);
427+
428+
// Under Explicit microtask policy, V8's REPL-mode evaluation creates
429+
// a promise whose resolution callback is a queued microtask. Without
430+
// this checkpoint, GC can collect the weakly-held promise before the
431+
// microtask drains, causing a "Promise was collected" CDP error.
432+
self
433+
.worker
434+
.js_runtime
435+
.v8_isolate()
436+
.perform_microtask_checkpoint();
437+
413438
let fut = self
414439
.state
415440
.wait_for_response(msg_id)
@@ -889,7 +914,7 @@ impl ReplSession {
889914
return_by_value: None,
890915
generate_preview: None,
891916
user_gesture: None,
892-
await_promise: None,
917+
await_promise: Some(true),
893918
throw_on_side_effect: None,
894919
timeout: None,
895920
disable_breaks: None,

cli/tools/run/hmr.rs

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -299,22 +299,35 @@ impl HmrRunner {
299299
}
300300

301301
async fn wait_for_response(&self, msg_id: i32) -> serde_json::Value {
302-
if let Some(message_state) = self.state.0.lock().messages.remove(&msg_id) {
303-
let InspectorMessageState::Ready(mut value) = message_state else {
304-
unreachable!();
305-
};
306-
return value["result"].take();
307-
}
302+
let rx = {
303+
let mut state = self.state.0.lock();
304+
if let Some(message_state) = state.messages.remove(&msg_id) {
305+
let InspectorMessageState::Ready(value) = message_state else {
306+
unreachable!();
307+
};
308+
return Self::extract_result(value);
309+
}
310+
let (tx, rx) = oneshot::channel();
311+
state
312+
.messages
313+
.insert(msg_id, InspectorMessageState::WaitingFor(tx));
314+
rx
315+
};
308316

309-
let (tx, rx) = oneshot::channel();
310-
self
311-
.state
312-
.0
313-
.lock()
314-
.messages
315-
.insert(msg_id, InspectorMessageState::WaitingFor(tx));
316-
let mut value = rx.await.unwrap();
317-
value["result"].take()
317+
let value = rx.await.unwrap();
318+
Self::extract_result(value)
319+
}
320+
321+
#[allow(clippy::print_stderr, reason = "diagnostic for flaky CDP responses")]
322+
fn extract_result(mut value: serde_json::Value) -> serde_json::Value {
323+
let result = value["result"].take();
324+
if result.is_null() {
325+
eprintln!(
326+
"CDP response has null result. Full response: {}",
327+
serde_json::to_string(&value).unwrap_or_default()
328+
);
329+
}
330+
result
318331
}
319332

320333
fn set_script_source(&mut self, script_id: &str, source: &str) -> i32 {

tests/integration/repl_tests.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -922,8 +922,8 @@ fn npm_packages() {
922922
true,
923923
);
924924

925+
assert!(err.is_empty(), "stderr: {}\nstdout: {}", err, out);
925926
assert_contains!(out, "hello");
926-
assert!(err.is_empty(), "Error: {}", err);
927927
}
928928

929929
{
@@ -938,8 +938,8 @@ fn npm_packages() {
938938
true,
939939
);
940940

941+
assert!(err.is_empty(), "stderr: {}\nstdout: {}", err, out);
941942
assert_contains!(out, "hello");
942-
assert!(err.is_empty(), "Error: {}", err);
943943
}
944944

945945
{

0 commit comments

Comments
 (0)