From 7156f0a195566e0394b3fe6ea9106c96b7384d37 Mon Sep 17 00:00:00 2001 From: Francesco Renzi Date: Thu, 22 Jan 2026 11:42:34 +0000 Subject: [PATCH] fix step duplication --- .../fix-step-back-duplicate-in-steps-list.md | 179 ++++++++++++++++++ src/Runner.Worker/Dap/DapDebugSession.cs | 2 +- .../Dap/StepCommands/StepManipulator.cs | 18 ++ 3 files changed, 198 insertions(+), 1 deletion(-) create mode 100644 .opencode/plans/fix-step-back-duplicate-in-steps-list.md diff --git a/.opencode/plans/fix-step-back-duplicate-in-steps-list.md b/.opencode/plans/fix-step-back-duplicate-in-steps-list.md new file mode 100644 index 000000000..4dea1827c --- /dev/null +++ b/.opencode/plans/fix-step-back-duplicate-in-steps-list.md @@ -0,0 +1,179 @@ +# Fix Step-Back Duplicating Steps in `steps list` + +**Status:** Ready for Implementation +**Date:** January 2026 +**Related:** [dap-step-backwards.md](./dap-step-backwards.md) + +## Problem Summary + +When stepping backwards during DAP debugging, the step that should be re-run appears **twice** in `steps list`: +1. Once as a **completed** step (from `StepManipulator._completedSteps`) +2. Once as a **pending** step (from the re-queued `JobSteps`) + +### Reproduction Steps + +1. Run a workflow with DAP debugging enabled +2. Let steps 1-4 execute (checkpoints created) +3. Pause at step 5 +4. Step backward +5. Run `steps list` + +**Expected:** +``` +✓ 1. hello +✓ 2. Run actions/checkout@v6 +✓ 3. Run echo "foo=bar" >> "$GITHUB_OUTPUT" +▶ 4. Run cat doesnotexist +5. Run a one-line script +6. Run a multi-line script +``` + +**Actual (bug):** +``` +✓ 1. hello +✓ 2. Run actions/checkout@v6 +✓ 3. Run echo "foo=bar" >> "$GITHUB_OUTPUT" +✓ 4. Run cat doesnotexist ← Still in _completedSteps! +▶ 5. Run cat doesnotexist ← Re-queued as current +6. Run a one-line script +7. Run a multi-line script +``` + +## Root Cause + +In `DapDebugSession.RestoreCheckpoint()` (line 1713), the session's `_completedSteps` and `_completedStepsTracker` lists are trimmed to match the checkpoint index: + +```csharp +// Clear completed steps list for frames after this checkpoint +while (_completedSteps.Count > checkpointIndex) +{ + _completedSteps.RemoveAt(_completedSteps.Count - 1); +} + +// Also clear the step tracker for manipulator sync +while (_completedStepsTracker.Count > checkpointIndex) +{ + _completedStepsTracker.RemoveAt(_completedStepsTracker.Count - 1); +} +``` + +However, **`StepManipulator` has its own separate `_completedSteps` list** that is **not** being synced. When `ClearChanges()` is called (line 1774), it only clears change tracking data (`_changes`, `_modifiedStepIds`, `_addedStepIds`, `_originalSteps`), not the `_completedSteps` list. + +### Data Flow During Step-Back + +1. Steps 1-4 complete → `StepManipulator._completedSteps` = [step1, step2, step3, step4] +2. Paused at step 5 +3. Step back → `RestoreCheckpoint(3, ...)` is called (checkpoint index 3 = before step 4) +4. `DapDebugSession._completedSteps` trimmed to 3 items ✓ +5. `DapDebugSession._completedStepsTracker` trimmed to 3 items ✓ +6. `StepManipulator._completedSteps` **NOT trimmed** ✗ (still has 4 items) +7. Step 4 and remaining steps re-queued to `JobSteps` +8. `steps list` called → `GetAllSteps()` combines: + - `_completedSteps` (4 items) + queue (step4, step5, step6, step7) + - Result: step4 appears twice + +## Solution + +Add a method to `IStepManipulator` interface to trim completed steps to a specific count, then call it from `RestoreCheckpoint`. + +## Files to Modify + +| File | Changes | +|------|---------| +| `src/Runner.Worker/Dap/StepCommands/StepManipulator.cs` | Add `TrimCompletedSteps(int count)` method to interface and implementation | +| `src/Runner.Worker/Dap/DapDebugSession.cs` | Call `TrimCompletedSteps` in `RestoreCheckpoint()` | + +## Detailed Changes + +### 1. `StepManipulator.cs` - Add Interface Method and Implementation + +**Add to `IStepManipulator` interface (around line 115, after `ClearChanges`):** + +```csharp +/// +/// Trims the completed steps list to the specified count. +/// Used when restoring a checkpoint to sync state with the debug session. +/// +/// The number of completed steps to keep. +void TrimCompletedSteps(int count); +``` + +**Add implementation to `StepManipulator` class (after `ClearChanges()` method, around line 577):** + +```csharp +/// +public void TrimCompletedSteps(int count) +{ + var originalCount = _completedSteps.Count; + while (_completedSteps.Count > count) + { + _completedSteps.RemoveAt(_completedSteps.Count - 1); + } + Trace.Info($"Trimmed completed steps from {originalCount} to {_completedSteps.Count}"); +} +``` + +### 2. `DapDebugSession.cs` - Call TrimCompletedSteps in RestoreCheckpoint + +**Modify `RestoreCheckpoint()` (around line 1774), change:** + +```csharp +// Reset the step manipulator to match the restored state +// It will be re-initialized when the restored step starts +_stepManipulator?.ClearChanges(); +``` + +**To:** + +```csharp +// Reset the step manipulator to match the restored state +_stepManipulator?.ClearChanges(); +_stepManipulator?.TrimCompletedSteps(checkpointIndex); +``` + +## Why This Approach + +| Approach | Pros | Cons | +|----------|------|------| +| **Add `TrimCompletedSteps()` (chosen)** | Minimal change, explicit, mirrors existing DapDebugSession pattern | Requires new interface method | +| Re-initialize StepManipulator completely | Clean slate | Would lose all state, more disruptive, harder to reason about | +| Share completed steps list between DapDebugSession and StepManipulator | Single source of truth | Major refactoring, tight coupling between components | + +The chosen approach is the least invasive and follows the existing pattern used by `DapDebugSession` for its own `_completedSteps` list. + +## Test Scenarios + +After implementing this fix, verify: + +1. **Basic step-back:** + - Run steps 1, 2, 3, 4 + - Pause at step 5 + - Step back + - `steps list` should show: ✓ 1-3 completed, ▶ 4 current, 5-6 pending (no duplicates) + +2. **Multiple step-backs:** + - Run steps 1, 2, 3 + - Step back twice (back to step 1) + - `steps list` should show: ▶ 1 current, 2-N pending (no completed steps) + +3. **Step back then forward:** + - Run steps 1, 2 + - Step back (to step 2) + - Step forward, let step 2 re-run and complete + - Step forward again + - `steps list` should show correct state without duplicates at any point + +4. **Reverse continue:** + - Run steps 1, 2, 3, 4 + - Reverse continue (back to step 1) + - `steps list` should show: ▶ 1 current, 2-N pending (no completed steps) + +## Implementation Checklist + +- [ ] Add `TrimCompletedSteps(int count)` to `IStepManipulator` interface +- [ ] Implement `TrimCompletedSteps` in `StepManipulator` class +- [ ] Call `TrimCompletedSteps(checkpointIndex)` in `DapDebugSession.RestoreCheckpoint()` +- [ ] Test basic step-back scenario +- [ ] Test multiple step-backs +- [ ] Test step back then forward +- [ ] Test reverse continue diff --git a/src/Runner.Worker/Dap/DapDebugSession.cs b/src/Runner.Worker/Dap/DapDebugSession.cs index f3eb47951..34ffed738 100644 --- a/src/Runner.Worker/Dap/DapDebugSession.cs +++ b/src/Runner.Worker/Dap/DapDebugSession.cs @@ -1770,8 +1770,8 @@ namespace GitHub.Runner.Worker.Dap } // Reset the step manipulator to match the restored state - // It will be re-initialized when the restored step starts _stepManipulator?.ClearChanges(); + _stepManipulator?.TrimCompletedSteps(checkpointIndex); // Store restored checkpoint for StepsRunner to consume _restoredCheckpoint = checkpoint; diff --git a/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs b/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs index 7ecf1753c..6f4574a52 100644 --- a/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs +++ b/src/Runner.Worker/Dap/StepCommands/StepManipulator.cs @@ -114,6 +114,13 @@ namespace GitHub.Runner.Worker.Dap.StepCommands /// void ClearChanges(); + /// + /// Trims the completed steps list to the specified count. + /// Used when restoring a checkpoint to sync state with the debug session. + /// + /// The number of completed steps to keep. + void TrimCompletedSteps(int count); + /// /// Checks if a step with the given ID already exists. /// @@ -576,6 +583,17 @@ namespace GitHub.Runner.Worker.Dap.StepCommands _originalSteps = null; } + /// + public void TrimCompletedSteps(int count) + { + var originalCount = _completedSteps.Count; + while (_completedSteps.Count > count) + { + _completedSteps.RemoveAt(_completedSteps.Count - 1); + } + Trace.Info($"Trimmed completed steps from {originalCount} to {_completedSteps.Count}"); + } + /// public bool HasStepWithId(string id) {