Fix core reliability issues and add E2E test suite#4
Conversation
Critical fixes: - DAP event ingestion is now active via background reader task that continuously reads from adapter stdout, preventing events from being missed or blocking the adapter - wait_stopped now properly handles timeouts and error propagation - Configured DAP timeouts (initialize, request) are now enforced - Output buffer now respects both max_events and max_bytes_mb limits New features: - Thread selection (debugger thread <id>) - Frame selection and navigation (debugger frame, up, down) - Breakpoint enable/disable commands - Capability checks before using advanced breakpoint features (function breakpoints, conditions, hit counts) - Restart command (for adapters that support it) Test infrastructure: - Added E2E test suite with C/Python/Rust fixtures - Tests cover: status, breakpoints, stepping, evaluation, threads, frame navigation, output capture - Breakpoint markers in fixtures for dynamic line number lookup - Tests isolated via temp XDG_CONFIG_HOME/XDG_RUNTIME_DIR Architecture improvements: - Background reader task in DapClient handles async event ingestion - Session tracks selected thread/frame separately from stopped thread - Frame cache for efficient up/down navigation - Proper cleanup of reader task and adapter on shutdown
Summary of ChangesHello @akiselev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the debugger's stability, functionality, and test coverage. It addresses core reliability issues by revamping DAP event ingestion and timeout handling, ensuring a more robust debugging experience. New features like thread and frame navigation, along with breakpoint management and restart capabilities, provide users with more control. Crucially, a new end-to-end test suite has been introduced to validate these improvements across multiple languages, establishing a solid foundation for future development. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@claude review |
|
Claude finished @akiselev's task in 2m 11s —— View job Code ReviewI've completed a thorough review of this PR. This is an impressive set of improvements that significantly enhances the debugger's reliability and functionality. The code quality is generally excellent, but I've identified several issues that should be addressed. Critical Issues1. Race Condition in Event Re-sending (src/dap/client.rs:359)Issue: In // Re-send other events so they're not lost
// (they'll be received when session takes the receiver)
let _ = self.event_tx.send(event);Impact: Events could be silently dropped during initialization, leading to missed stopped events or output. Recommendation: Either buffer these events in a Vec within DapClient and drain them when the receiver is taken, or redesign the initialization flow to avoid consuming events before the session is ready. Location: src/dap/client.rs:359 2. Unsafe Shutdown in Drop Implementation (src/dap/client.rs:621-638)Issue: The
impl Drop for DapClient {
fn drop(&mut self) {
if let Some(tx) = self.shutdown_tx.take() {
let _ = tx.try_send(()); // Can silently fail
}
if let Some(task) = self.reader_task.take() {
task.abort(); // Abrupt termination
}
let _ = self.adapter.start_kill();
}
}Impact: The adapter process may not be properly cleaned up, potentially leaving zombie processes or file descriptors open. Recommendation: Document this limitation clearly, or consider using a shutdown channel with a larger buffer. The existing Location: src/dap/client.rs:621-638 3. Incorrect Breakpoint Marker Line Calculation (tests/integration.rs:130)Issue: The marker detection adds 2 to line_num, but the comment says "NEXT line after the marker": // Breakpoint goes on the NEXT line after the marker
markers.insert(marker_name, (line_num + 2) as u32);Analysis: If the marker is on line 19 (zero-indexed), Impact: Breakpoints may be set on the wrong lines, causing tests to fail or pass incorrectly. Recommendation: Clarify the indexing logic. If line_num is zero-indexed and DAP uses 1-indexed lines, the formula should be Location: tests/integration.rs:130 High Priority Issues4. Missing Capability Check for Conditions/Hit Counts (src/daemon/session.rs:542-564)Issue: When adding source breakpoints, the code doesn't verify that conditions/hit counts are supported before collecting them: fn collect_source_breakpoints(&self, file: &Path) -> Vec<SourceBreakpoint> {
// ... creates SourceBreakpoint with condition and hit_condition
// but doesn't check capabilities first
}Impact: If an adapter doesn't support conditions but the user added one, it will be silently ignored or cause an error from the adapter. Recommendation: Check capabilities before including condition/hit_condition fields in SourceBreakpoint, or return an error earlier. The handler checks capabilities before calling add_breakpoint, but internal consistency checks would be more robust. Location: src/daemon/session.rs:542-564 5. Potential Memory Leak in Output Buffer (src/daemon/session.rs:406-432)Issue: The output buffer enforcement logic has a subtle issue. When enforcing the byte limit, it removes items but still adds the new output even if it exceeds max_output_bytes: while self.current_output_bytes + output_bytes > self.max_output_bytes
&& !self.output_buffer.is_empty()
{
// Remove old items...
}
// Always adds new output, even if output_bytes > max_output_bytes
self.output_buffer.push_back(OutputEvent { ... });Impact: A single large output event (e.g., binary data) could exceed the buffer limit permanently. Recommendation: Add a check to truncate or skip output events that exceed max_output_bytes by themselves. Location: src/daemon/session.rs:406-432 6. Frame Cache Invalidation Issues (src/daemon/session.rs:950-956)Issue: When selecting a thread, the frame cache is cleared, but when stopping on a new thread, the selected_thread is not updated to match stopped_thread: Event::Stopped(body) => {
self.stopped_thread = body.thread_id;
// selected_thread is NOT reset to stopped_thread
self.cached_frames.clear();
}Impact: If a user selects thread 2, then thread 1 stops at a breakpoint, the selected_thread remains 2 but stopped_thread is 1. Commands will target thread 2 (via get_thread_id preference), which may not be stopped, causing errors. Recommendation: Consider resetting selected_thread to None when a different thread stops, or document that selected_thread persists across stops. Location: src/daemon/session.rs:327-349, 950-956 Medium Priority Issues7. Error Propagation in Event Processing (src/dap/client.rs:137-139)Issue: Errors from if let Err(e) = Self::process_message(&json, &event_tx, &pending).await {
tracing::error!("Error processing DAP message: {}", e);
}Impact: Protocol errors are silently ignored, potentially causing state inconsistencies. Recommendation: Consider sending error events to the session or maintaining an error flag that can be queried. Location: src/dap/client.rs:137-139 8. Thread List Staleness (src/daemon/session.rs:944-948)Issue: The pub fn select_thread(&mut self, thread_id: i64) -> Result<()> {
// Verify thread exists
if !self.threads.iter().any(|t| t.id == thread_id) {
// Thread list might be stale, but we'll allow selection anyway
}
// ...
}Impact: Users can select non-existent threads, and errors only surface during the next operation. Recommendation: Either refresh the thread list before validation, or return a clear error/warning when selecting an unknown thread. Location: src/daemon/session.rs:944-948 9. Timeout Calculation Issues (src/daemon/session.rs:444-447)Issue: Uses let remaining = deadline.saturating_duration_since(tokio::time::Instant::now());
if remaining.is_zero() {
return Err(Error::AwaitTimeout(timeout_secs));
}
match tokio::time::timeout(remaining, self.events_rx.recv()).await {Analysis: This is actually correct - it checks for zero and returns early. However, the same pattern in wait_initialized_with_timeout (line 347) could benefit from the same check. Recommendation: The code is correct but could be more consistent. Location: src/daemon/session.rs:439-474 10. String Comparison for EOF Detection (src/dap/client.rs:143-144)Issue: Checks for EOF using string matching on the error message: let is_eof = e.to_string().contains("unexpected eof")
|| e.to_string().contains("UnexpectedEof");Impact: Fragile error detection that could break with error message changes. Recommendation: Match on error kind instead: Location: src/dap/client.rs:143-144 Low Priority / Suggestions11. Missing Test CoverageIssue: While the E2E tests are comprehensive, they're all marked with Recommendation:
12. Documentation GapsAreas needing better documentation:
13. Code Quality ImprovementsMinor improvements:
Positive ObservationsThis PR demonstrates excellent engineering:
The issues identified are relatively minor compared to the quality and scope of the work. Great job! SummaryMust Fix:
Should Fix:
Consider Addressing:
Overall Assessment: This is high-quality code that makes significant improvements to the debugger. The critical issues should be addressed before merging, but they're fixable without major refactoring. Tasks
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a fantastic pull request that introduces significant reliability and feature improvements. The move to a background reader task for DAP events is a major architectural enhancement that will make the debugger much more robust. The addition of timeouts, capability checks, and a comprehensive E2E test suite are all excellent changes that improve the quality and stability of the project.
My review includes a few suggestions for refactoring to reduce code duplication and fix some minor issues, including removing some leftover debug prints and dead code. Overall, this is a very strong contribution.
src/daemon/session.rs
Outdated
| if self.capabilities.supports_restart_request { | ||
| self.client.restart(false).await?; | ||
| self.state = SessionState::Running; | ||
| Ok(()) | ||
| } else { | ||
| // Manual restart: stop and relaunch | ||
| // Store launch parameters | ||
| let program = self.program.clone(); | ||
| let args = self.args.clone(); | ||
| let adapter = self.adapter_name.clone(); | ||
|
|
||
| // Stop current session | ||
| self.stop().await?; | ||
|
|
||
| // Relaunch - this is complex because we need to reinitialize | ||
| // For now, return an error indicating manual restart is needed | ||
| Err(Error::Internal( | ||
| "Adapter does not support restart. Use 'debugger stop' then 'debugger start' manually.".to_string() | ||
| )) | ||
| } |
There was a problem hiding this comment.
The else block in this restart function appears to be dead code. The only call site in src/daemon/handler.rs (line 91) already checks sess.capabilities().supports_restart_request before calling this method. As a result, the if condition here will always be true, and the else branch is unreachable.
You should remove the capability check and the else block from this function, making the caller solely responsible for verifying the capability. The function signature can also be simplified as it no longer needs the config parameter. You will also need to update the call in src/daemon/handler.rs to no longer pass config.
if self.capabilities.supports_restart_request {
self.client.restart(false).await?;
self.state = SessionState::Running;
Ok(())
}
src/dap/client.rs
Outdated
| result = codec::read_message(&mut reader) => { | ||
| match result { | ||
| Ok(json) => { | ||
| eprintln!("DAP <<< {}", json); |
There was a problem hiding this comment.
This eprintln! prints every incoming DAP message to stderr. This is likely a leftover debugging statement and will create a lot of noise, potentially obscuring actual error messages. It should be removed or replaced with a tracing::trace! call for conditional logging.
| eprintln!("DAP <<< {}", json); | |
| tracing::trace!("DAP <<< {}", json); |
Cargo.toml
Outdated
| [dev-dependencies] | ||
| which = "7" |
There was a problem hiding this comment.
The which crate is listed here as a dev-dependency, but it's already included as a regular dependency in this Cargo.toml. This is redundant and can be removed from the [dev-dependencies] section.
| Commands::Up => { | ||
| // TODO: Track current frame | ||
| println!("Frame navigation not yet implemented"); | ||
| let mut client = DaemonClient::connect().await?; | ||
|
|
||
| let result = client.send_command(Command::FrameUp).await?; | ||
| let frame_index = result["selected"].as_u64().unwrap_or(0); | ||
|
|
||
| // Get the frame info | ||
| if let Ok(frame_info) = serde_json::from_value::<StackFrameInfo>(result["frame"].clone()) { | ||
| let source = frame_info.source.as_deref().unwrap_or("?"); | ||
| let line = frame_info.line.map(|l| l.to_string()).unwrap_or_else(|| "?".to_string()); | ||
| println!("#{} {} at {}:{}", frame_index, frame_info.name, source, line); | ||
| } else { | ||
| println!("Switched to frame {}", frame_index); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| Commands::Down => { | ||
| println!("Frame navigation not yet implemented"); | ||
| let mut client = DaemonClient::connect().await?; | ||
|
|
||
| let result = client.send_command(Command::FrameDown).await?; | ||
| let frame_index = result["selected"].as_u64().unwrap_or(0); | ||
|
|
||
| // Get the frame info | ||
| if let Ok(frame_info) = serde_json::from_value::<StackFrameInfo>(result["frame"].clone()) { | ||
| let source = frame_info.source.as_deref().unwrap_or("?"); | ||
| let line = frame_info.line.map(|l| l.to_string()).unwrap_or_else(|| "?".to_string()); | ||
| println!("#{} {} at {}:{}", frame_index, frame_info.name, source, line); | ||
| } else { | ||
| println!("Switched to frame {}", frame_index); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The logic for handling Commands::Up and Commands::Down is almost identical. To improve maintainability and reduce code duplication, you could extract the response handling into a shared helper function like fn handle_frame_nav_output(result: serde_json::Value) -> Result<()> and call it from both command arms.
| Command::FrameUp => { | ||
| let sess = session.as_mut().ok_or(Error::SessionNotActive)?; | ||
| let frame = sess.frame_up().await?; | ||
| let index = sess.get_current_frame_index(); | ||
|
|
||
| let frame_info = StackFrameInfo { | ||
| id: frame.id, | ||
| name: frame.name.clone(), | ||
| source: frame.source.as_ref().and_then(|s| s.path.clone()), | ||
| line: Some(frame.line), | ||
| column: Some(frame.column), | ||
| }; | ||
|
|
||
| Ok(json!({ | ||
| "selected": index, | ||
| "frame": frame_info | ||
| })) | ||
| } | ||
|
|
||
| Command::FrameDown => { | ||
| let sess = session.as_mut().ok_or(Error::SessionNotActive)?; | ||
| let frame = sess.frame_down().await?; | ||
| let index = sess.get_current_frame_index(); | ||
|
|
||
| let frame_info = StackFrameInfo { | ||
| id: frame.id, | ||
| name: frame.name.clone(), | ||
| source: frame.source.as_ref().and_then(|s| s.path.clone()), | ||
| line: Some(frame.line), | ||
| column: Some(frame.column), | ||
| }; | ||
|
|
||
| Ok(json!({ | ||
| "selected": index, | ||
| "frame": frame_info | ||
| })) | ||
| } |
There was a problem hiding this comment.
The logic for handling Command::FrameUp and Command::FrameDown is very similar, with significant code duplication in how the StackFrameInfo and the final JSON response are constructed. This pattern is also present in the FrameSelect handler. To improve maintainability, consider extracting this object creation into a helper function, for example fn create_frame_response(frame: crate::dap::StackFrame, index: usize) -> serde_json::Value.
| if self.current_frame_index == 0 { | ||
| return Err(Error::FrameNotFound(0)); | ||
| } |
There was a problem hiding this comment.
Returning Error::FrameNotFound(0) when at the bottom of the stack (frame 0) can be misleading, as frame 0 does exist. The issue is that you cannot navigate "down" from it. A more descriptive error would improve clarity for the user.
Consider using a more specific error, for example by using Error::invalid_state("frame down", "at bottom of stack") or by adding a new error variant like Error::InvalidFrameNavigation.
if self.current_frame_index == 0 {
return Err(Error::invalid_state("frame down", "at bottom of stack"));
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87c0ebcd81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/dap/client.rs
Outdated
| let seq = self.send_request(command, arguments).await?; | ||
|
|
||
| // Read messages until we get the response | ||
| loop { | ||
| let msg = self.read_message().await?; | ||
|
|
||
| let msg_type = msg | ||
| .get("type") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("unknown"); | ||
|
|
||
| match msg_type { | ||
| "response" => { | ||
| let response: ResponseMessage = serde_json::from_value(msg)?; | ||
|
|
||
| if response.request_seq == seq { | ||
| if response.success { | ||
| let body = response.body.unwrap_or(Value::Null); | ||
| return serde_json::from_value(body).map_err(|e| { | ||
| Error::DapProtocol(format!( | ||
| "Failed to parse {} response: {}", | ||
| command, e | ||
| )) | ||
| }); | ||
| } else { | ||
| return Err(Error::dap_request_failed( | ||
| command, | ||
| &response.message.unwrap_or_else(|| "Unknown error".to_string()), | ||
| )); | ||
| } | ||
| } else if let Some(tx) = self.pending.remove(&response.request_seq) { | ||
| let _ = tx.send(response); | ||
| } | ||
| } | ||
| "event" => { | ||
| let event_msg: EventMessage = serde_json::from_value(msg)?; | ||
| let event = Event::from_message(&event_msg); | ||
| let _ = self.event_tx.send(event); | ||
| } | ||
| _ => { | ||
| tracing::warn!("Unknown message type: {}", msg_type); | ||
| } | ||
| } | ||
| // Create a oneshot channel to receive the response | ||
| let (tx, rx) = oneshot::channel(); | ||
|
|
There was a problem hiding this comment.
Because request_with_timeout calls send_request before registering the response channel (the insert happens a few lines below), the background reader can process a fast adapter response first and drop it as an “unknown request seq”. In that case the caller will wait until timeout even though the adapter replied successfully. This is most likely with low-latency adapters or quick commands like initialize/threads. Consider inserting the pending sender before writing to the adapter (or buffering early responses) to avoid this race.
Useful? React with 👍 / 👎.
| /// Get a thread ID (preferring selected > stopped > first) | ||
| async fn get_thread_id(&mut self) -> Result<i64> { | ||
| // Prefer explicitly selected thread | ||
| if let Some(id) = self.selected_thread { | ||
| return Ok(id); |
There was a problem hiding this comment.
get_thread_id always returns selected_thread if it is set, even if that thread has exited. Since Event::Thread with reason "exited" only prunes self.threads and does not clear selected_thread, selecting a short-lived thread can leave a stale ID that subsequent stack/locals requests will use, causing DAP errors after that thread dies. Clearing selected_thread on thread-exit (or validating it against the current thread list here) would prevent this.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses critical reliability issues in the debugger CLI and adds comprehensive E2E testing infrastructure. The main focus is on fixing event ingestion in the DAP client through a background reader task, implementing proper timeout handling, and fixing output buffer limits.
Changes:
- Refactored DAP client to use a background reader task for continuous event ingestion, preventing missed events and blocking issues
- Implemented timeout enforcement for DAP initialization and requests with configurable durations
- Added new features: thread/frame selection and navigation, breakpoint enable/disable, restart command with capability checks
- Created E2E test suite with C/Python/Rust fixtures and breakpoint markers for dynamic line number resolution
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Fixed Rust edition from "2024" to "2021", added lib/bin configuration for test integration |
| src/lib.rs | New library entry point exposing modules for integration tests |
| src/main.rs | Refactored to use library modules instead of local mod declarations |
| src/dap/client.rs | Major refactor: background reader task for event ingestion, timeout support, improved resource cleanup |
| src/dap/types.rs | Added supports_restart_request capability |
| src/ipc/protocol.rs | Added FrameUp and FrameDown commands for stack navigation |
| src/daemon/session.rs | Thread/frame tracking, output buffer with byte limits, capability checks, new command implementations |
| src/daemon/handler.rs | Implemented restart, breakpoint enable/disable, thread/frame selection commands |
| src/cli/mod.rs | Implemented up/down commands for frame navigation |
| tests/integration.rs | Comprehensive E2E test suite with fixtures for C/Rust/Python programs |
| tests/fixtures/*.c/rs/py | Test fixtures with breakpoint markers for dynamic line lookup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/integration.rs
Outdated
| // Optionally clean up temp directory | ||
| // let _ = fs::remove_dir_all(&self.temp_dir); |
There was a problem hiding this comment.
The cleanup logic comments out the removal of the temp directory. This could lead to disk space accumulation during development. Consider either enabling cleanup by default or adding a command-line flag or environment variable to control whether test artifacts are preserved for debugging purposes.
| // Optionally clean up temp directory | |
| // let _ = fs::remove_dir_all(&self.temp_dir); | |
| // Optionally clean up temp directory. | |
| // By default we preserve artifacts for debugging; set | |
| // PRESERVE_DEBUGGER_TEST_ARTIFACTS=0/false/no to enable cleanup. | |
| let preserve = env::var("PRESERVE_DEBUGGER_TEST_ARTIFACTS") | |
| .unwrap_or_else(|_| "1".to_string()) | |
| .to_ascii_lowercase(); | |
| if preserve == "0" || preserve == "false" || preserve == "no" { | |
| let _ = fs::remove_dir_all(&self.temp_dir); | |
| } |
src/dap/client.rs
Outdated
| let is_eof = e.to_string().contains("unexpected eof") | ||
| || e.to_string().contains("UnexpectedEof"); |
There was a problem hiding this comment.
The error matching logic checks for error messages containing "unexpected eof" or "UnexpectedEof" using to_string(). This string comparison is fragile and may not work correctly with all error types. Consider using error downcast methods (error.downcast_ref::io::Error()) to check for specific io::ErrorKind::UnexpectedEof instead of string matching.
src/main.rs
Outdated
| use debugger::{cli, commands, daemon}; | ||
| use commands::Commands; |
There was a problem hiding this comment.
The Commands type is imported from the debugger crate but also separately from commands. This creates ambiguity about where Commands comes from. The line use commands::Commands; should be use debugger::commands::Commands; for clarity and consistency with the module organization, or the commands import should be removed from line 7 if only Commands is needed.
| use debugger::{cli, commands, daemon}; | |
| use commands::Commands; | |
| use debugger::{cli, daemon}; | |
| use debugger::commands::Commands; |
src/dap/client.rs
Outdated
| // Re-send other events so they're not lost | ||
| // (they'll be received when session takes the receiver) | ||
| let _ = self.event_tx.send(event); |
There was a problem hiding this comment.
The wait_initialized_with_timeout method re-sends non-Initialized events back to the event_tx channel to avoid losing them. However, this could create a risk of events being re-ordered or duplicated if the session starts receiving from event_rx while wait_initialized is still processing events. Consider documenting this behavior more clearly or using a queue to buffer events in order.
|
|
||
| // Set multiple breakpoints | ||
| for marker in ["main_start", "before_add", "before_factorial"] { | ||
| let line = markers.get(marker).expect(&format!("Missing {} marker", marker)); |
There was a problem hiding this comment.
The format! macro is unnecessary here since marker is already a &str slice. The use of format! in the error message "Missing {} marker" is creating a temporary string that could be avoided by restructuring the error handling.
| let line = markers.get(marker).expect(&format!("Missing {} marker", marker)); | |
| let line = markers | |
| .get(marker) | |
| .unwrap_or_else(|| panic!("Missing {} marker", marker)); |
src/daemon/session.rs
Outdated
| // Thread list might be stale, but we'll allow selection anyway | ||
| // The next operation will fail with a clearer error if invalid |
There was a problem hiding this comment.
The select_thread method doesn't validate that the thread ID exists in the threads list and proceeds anyway with a comment that "the next operation will fail with a clearer error". This could lead to confusing error messages later. Consider either returning an error immediately when the thread doesn't exist, or at minimum refresh the thread list before checking to ensure it's up to date.
| // Thread list might be stale, but we'll allow selection anyway | |
| // The next operation will fail with a clearer error if invalid | |
| // Fail fast if the thread is not known in the current thread list. | |
| // This avoids leaving the session in an inconsistent state and | |
| // provides a clearer error to the caller. | |
| return Err(Error::Internal(format!("Thread {} not found", thread_id))); |
| // Abort the reader task if it's still running | ||
| if let Some(task) = self.reader_task.take() { | ||
| task.abort(); |
There was a problem hiding this comment.
The Drop implementation has a potential race condition. After sending the shutdown signal with try_send, the code immediately calls task.abort() without giving the task time to shut down gracefully. This could lead to the task being aborted while it's in the middle of processing a message or updating shared state. Consider adding a small timeout before abort, similar to the terminate method.
| // Abort the reader task if it's still running | |
| if let Some(task) = self.reader_task.take() { | |
| task.abort(); | |
| // Give the reader task a brief chance to shut down gracefully before aborting | |
| if let Some(task) = self.reader_task.take() { | |
| // If we're inside a Tokio runtime, spawn a delayed abort. | |
| if let Ok(handle) = tokio::runtime::Handle::try_current() { | |
| handle.spawn(async move { | |
| tokio::time::sleep(Duration::from_millis(100)).await; | |
| if !task.is_finished() { | |
| task.abort(); | |
| } | |
| }); | |
| } else { | |
| // If no runtime is available, fall back to immediate abort. | |
| task.abort(); | |
| } |
tests/integration.rs
Outdated
| // Breakpoint goes on the NEXT line after the marker | ||
| markers.insert(marker_name, (line_num + 2) as u32); |
There was a problem hiding this comment.
The find_breakpoint_markers function adds 2 to line_num to calculate the breakpoint line, but line_num is 0-indexed from enumerate(). This means the comment "Breakpoint goes on the NEXT line after the marker" is implemented correctly (line_num + 1 for the next line, + 1 again for 1-indexed line numbers = line_num + 2). However, this logic is fragile and could be confusing. Consider making the intent clearer with a comment explaining that enumerate gives 0-indexed lines and DAP expects 1-indexed lines.
| // Breakpoint goes on the NEXT line after the marker | |
| markers.insert(marker_name, (line_num + 2) as u32); | |
| // enumerate() gives 0-based line numbers, but DAP expects 1-based. | |
| // We also want the breakpoint on the *next* line after the marker: | |
| // - +1 to move to the next source line | |
| // - +1 to convert from 0-based to 1-based indexing | |
| let breakpoint_line = (line_num as u32) + 2; | |
| markers.insert(marker_name, breakpoint_line); |
| Command::Restart => { | ||
| // TODO: Implement restart | ||
| Err(Error::Internal("Restart not yet implemented".to_string())) | ||
| let sess = session.as_mut().ok_or(Error::SessionNotActive)?; | ||
|
|
||
| // Check if adapter supports restart | ||
| if sess.capabilities().supports_restart_request { | ||
| sess.restart(config).await?; | ||
| Ok(json!({ "status": "restarted" })) | ||
| } else { | ||
| // Return helpful error message | ||
| Err(Error::Internal( | ||
| "Debug adapter does not support restart. Use 'debugger stop' then 'debugger start' manually.".to_string() | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new features added in this PR (restart command, breakpoint enable/disable, thread selection via ThreadSelect command) lack E2E test coverage. While test_frame_navigation_c tests frame navigation (up/down commands), there are no tests for the new restart functionality, breakpoint enable/disable commands, or explicit thread selection. Consider adding integration tests for these features to ensure they work end-to-end.
| let output_bytes = output.len(); | ||
|
|
||
| // Enforce byte limit - remove oldest entries until we have space | ||
| while self.current_output_bytes + output_bytes > self.max_output_bytes | ||
| && !self.output_buffer.is_empty() | ||
| { | ||
| if let Some(removed) = self.output_buffer.pop_front() { | ||
| self.current_output_bytes = self.current_output_bytes.saturating_sub(removed.output.len()); | ||
| } | ||
| } | ||
|
|
||
| // Enforce event count limit | ||
| while self.output_buffer.len() >= self.max_output_events && !self.output_buffer.is_empty() { | ||
| if let Some(removed) = self.output_buffer.pop_front() { | ||
| self.current_output_bytes = self.current_output_bytes.saturating_sub(removed.output.len()); | ||
| } | ||
| } | ||
|
|
||
| // Add the new output | ||
| self.output_buffer.push_back(OutputEvent { | ||
| category: category.to_string(), | ||
| output: output.to_string(), | ||
| timestamp: std::time::Instant::now(), | ||
| }); | ||
| self.current_output_bytes += output_bytes; |
There was a problem hiding this comment.
If the new output message is larger than max_output_bytes, the method will empty the entire buffer but then still add the oversized message, exceeding the limit. Consider either rejecting messages that exceed max_output_bytes, truncating them, or logging a warning that the limit has been exceeded.
Fixes from review comments: - Fix race condition: register pending response handler before sending request - Replace eprintln debug statements with tracing::trace! - Remove dead code in restart method (unused else branch) - Fix frame_down error message to reference innermost frame - Clear selected_thread when thread exits - Fix buffer_output to truncate oversized messages with warning - Add thread validation in select_thread - Add documentation to Drop impl about async task cleanup limitations - Add documentation to wait_initialized about event ordering - Create helper functions to reduce duplicate code in handler.rs and cli/mod.rs - Fix import clarity in main.rs - Remove duplicate which dependency from Cargo.toml - Fix duplicate breakpoint marker in simple.c - Improve breakpoint marker line calculation comment - Add env var control for test temp directory cleanup (PRESERVE_DEBUGGER_TEST_ARTIFACTS)
Comprehensive guide covering: - Architecture overview (CLI/daemon split, DAP communication) - Project structure with file responsibilities - Step-by-step guide for adding new commands - Working with the DAP client (requests, events, race conditions) - IPC protocol details - Error handling patterns - Configuration system - Testing guide with fixtures - Debugging tips and common issues
Critical fixes:
continuously reads from adapter stdout, preventing events from being
missed or blocking the adapter
New features:
(function breakpoints, conditions, hit counts)
Test infrastructure:
frame navigation, output capture
Architecture improvements: