Make debug command enqueue atomic with the engine's resume

[George-Payne]

a debug verb racing a resume could strand its caller forever and then replay as a stale command at the next pause. The verbs did an unsynchronized paused check-then-enqueue while the command loop exits on the first Continue/Step without draining the queue; since #267's teardown gate, the stranded caller holds a session op, so the hang propagated to Runner.Destroy and wedged the MCP server's closeSession under its mutex. Drain-at-exit alone can't fix it - a verb that passed the check can enqueue after any drain - so the enqueue and the resume are made mutually atomic.

• JintProjectionHandler.cs - _debugSync makes each verb's check+enqueue atomic with the command loop's resume+drain. Every loop exit (resume, exception, disposed-while-paused) goes through ResumeAndDrainDebugCommands: clear the pause state, then fail anything still queued, under the same lock - so an enqueued command is always either consumed or failed, never stranded and never carried across a pause (a stale Continue would have silently resumed a breakpoint the user never continued). All eight verbs check the command's error after their wait; Dispose drains as a backstop; an enqueue racing CompleteAdding gets the verb's own refusal message rather than the framework's.
• DebugTests.cs - two regression tests: 25 iterations of 3 racing continues with join timeouts as strand detectors (fails pre-fix on the first iteration), and a stale-replay guard asserting the next pause waits for its own resume.
• engine/runner_debug.go - the doc comments that recorded the hazard now state the guarantee (a verb always returns, so Destroy's wait can't wedge on one).
• .changeset/debug-command-strand.md - patch; the fix ships in the CLI's bundled runtime.

[linear-code]

UI-1822

[qodo-code-review]

PR Summary by Qodo

Make debug verb enqueue atomic with resume to prevent stranded/stale commands

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Prevent debug resume verbs from stranding callers by making pause-check+enqueue atomic with
 resume+drain
• Fail commands that lose the resume race instead of replaying them at the next pause
• Add regression tests covering resume races and stale-command replay

Diagram

graph TD
  V["Debug verbs (Continue/Step/Inspect)"] --> E["EnqueueDebugCommand"] --> L["_debugSync lock"] --> Q[("_debugCommands queue")]
  Q --> CL["Debug command loop"] --> R["ResumeAndDrainDebugCommands"] --> D["DrainDebugCommands (fail + Done)"]
  T["DebugTests"] --> V
  CLI["Runner.Destroy/drain wait"] --> V

  subgraph Legend
    direction LR
    _db[(Queue)] ~~~ _fn["Method"] ~~~ _lock["Lock"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Replace BlockingCollection with Channel + cancellation

• ➕ Clearer completion semantics (writer completion + reader cancellation)
• ➕ Potentially simpler to ensure no post-resume enqueues are accepted
• ➖ Larger refactor across producer/consumer code paths
• ➖ More behavioral surface area to validate vs. a targeted lock-based fix

2. Single state machine with Interlocked + concurrent queue

• ➕ Avoids coarse locking; can be highly performant under contention
• ➕ Explicit states can make invariants (paused/running/draining) clearer
• ➖ Harder to reason about correctness; higher risk of subtle races
• ➖ More code and more complex tests needed to prove strand-freedom

3. Drain-only on resume (no atomic enqueue)

• ➕ Minimal code changes (just drain queue on every exit)
• ➖ Does not close the race: a command can enqueue after the drain and still strand/replay
• ➖ Fails to provide the key guarantee that every verb always returns

Recommendation: Keep the PR’s approach: making verb enqueue mutually atomic with resume+drain via a dedicated lock is the smallest change that actually closes the race window. The added behavior (explicitly failing drained commands and checking cmd.Error in verbs) converts a hang/stale-replay into a deterministic error, which is safer for callers and matches the teardown guarantees described in the CLI comments.

Files changed (4) +196 / -39

Bug fix (1) +72 / -28

JintProjectionHandler.csSerialize debug enqueue with resume+drain and fail stale commands +72/-28

Serialize debug enqueue with resume+drain and fail stale commands

• Introduces a dedicated synchronization lock to make the verbs’ paused check+enqueue atomic with the command loop’s resume+drain. Ensures every command left queued on loop exit is failed (Error set) and unblocks callers by setting Done; verbs now rethrow cmd.Error after waiting. Adds Dispose-time draining as a backstop and normalizes the error surfaced when enqueue races collection completion.

runtime/Gaffer.Runtime/Projection/JintProjectionHandler.cs

Tests (1) +109 / -0

DebugTests.csAdd regression tests for resume-race strand and stale replay +109/-0

Add regression tests for resume-race strand and stale replay

• Adds a stress test that repeatedly races multiple Continue calls and asserts all callers return within a timeout. Adds a second test ensuring a resume race cannot leave a queued command that auto-resumes the next pause.

runtime/Gaffer.Runtime.Tests/DebugTests.cs

Documentation (2) +15 / -11

debug-command-strand.mdAdd changeset describing debug resume race fix (UI-1822) +5/-0

Add changeset describing debug resume race fix (UI-1822)

• Introduces a patch changeset documenting that racing resume verbs no longer wedge sessions or replay stale commands. Explains the new guarantee: commands that lose the race are failed with an error instead of being stranded or carried across pauses.

.changeset/debug-command-strand.md

runner_debug.goUpdate Runner debug/teardown comments to reflect new runtime guarantee +10/-11

Update Runner debug/teardown comments to reflect new runtime guarantee

• Adjusts commentary around optimistic paused flipping and Runner drain behavior to include the new runtime refusal mode and strand-free guarantee. Removes outdated explanation of the hazard now fixed in the runtime command loop.

cli/internal/engine/runner_debug.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Threads leak on timeout ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new debug-race regression tests use Join timeouts to detect a stranded caller, but if the Join
times out the Assert throws while the spawned Threads are still running. This can leave runaway
threads executing against the session after the test has failed, potentially wedging the test
process or cascading failures into subsequent tests.

Code

runtime/Gaffer.Runtime.Tests/DebugTests.cs[R827-853]

+			var feedThread = new Thread(() => {
+				try {
+					session.Feed(MakeEvent());
+				} catch (Exception ex) {
+					feedEx = ex;
+				}
+			});
+			feedThread.Start();
+			Assert.True(paused.Wait(TimeSpan.FromSeconds(5)));
+
+			var racers = new Thread[3];
+			for (var r = 0; r < racers.Length; r++) {
+				racers[r] = new Thread(() => {
+					try {
+						session.Continue();
+					} catch (InvalidOperationException) {
+						// Lost the race: "not paused", or drained with
+						// "resumed before the command ran". Both fine -
+						// only hanging is a bug.
+					}
+				});
+				racers[r].Start();
+			}
+			foreach (var racer in racers)
+				Assert.True(racer.Join(TimeSpan.FromSeconds(10)), "a resume verb stranded: its Done was never signalled");
+			Assert.True(feedThread.Join(TimeSpan.FromSeconds(10)));
+			Assert.Null(feedEx);

Evidence

Both newly added tests create multiple Thread instances and rely on Join(TimeSpan...) assertions
to detect hangs; on timeout those assertions fail without any cleanup/termination of the
still-running threads.

runtime/Gaffer.Runtime.Tests/DebugTests.cs[809-854]
runtime/Gaffer.Runtime.Tests/DebugTests.cs[859-916]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new tests create `Thread` instances and use `Join(timeout)` assertions as hang detectors. When a timeout occurs, the assertion throws immediately, but the associated thread(s) are still running/hung with no cleanup path.

## Issue Context
These tests are intended to catch the historical hang (UI-1822). Ironically, when the bug regresses, the test itself can leave hung foreground threads behind.

## Fix Focus Areas
- runtime/Gaffer.Runtime.Tests/DebugTests.cs[824-916]

### Suggested fix
For every `new Thread(...)` in the two new tests, set `IsBackground = true` before `Start()`, e.g.
```csharp
var t = new Thread(() => { /* ... */ }) { IsBackground = true };
t.Start();
```
Optionally (more robust), switch to `Task.Run` and use `Task.Wait(TimeSpan)` / `WhenAny` with a `CancellationToken` so the test has an explicit cancellation/cleanup path on failure.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---