feat(testing): deterministic process-signal lifecycle support in Repl.Testing - #90
feat(testing): deterministic process-signal lifecycle support in Repl.Testing#90carldebilly wants to merge 9 commits into
Conversation
Repl.Testing carried `GenerateDocumentationFile=false`, the override the five test projects share. It is not a test project though — it is a package on NuGet, and without a documentation file its consumers get no IntelliSense on any of its public API. Turning generation on also arms CS1591, which is an error here, so a new public member can no longer ship undocumented. That surfaced 20 existing members with no `<summary>`: the types were documented, their members largely were not. This documents all 20 and leaves the build at 0 warnings. Separate from the process-signal work in this branch so it can be reviewed on its own.
Ctrl+C and Ctrl+Break have `ConsoleCancelKeyCoordinator.HandleCancelKeyForTesting`. SIGTERM had no counterpart: `HandleSigTerm` is private and reached only from the `PosixSignalRegistration` callback, so the shared claim logic was exercised in-process only through the console path, and SIGTERM's own first/second-signal behaviour was covered only by the out-of-process suite. `TryClaimSignal`'s first guard compares the delivering registration's captured generation against the current one. A seam must behave like a freshly installed registration, and reading the counter before taking the gate would race both `TryInitializeRegistrations`' failure path and the test isolation scope, which each advance it. So the parameter becomes nullable and the comparison moves inside the lock that already covers everything else — one signature change, no new locked region, and the two real call sites keep passing an `int` unchanged. What this does not cover, stated at the seam rather than implied: `HandleSigTerm`'s own translation of the decision into `PosixSignalContext.Cancel` needs a real signal context and stays covered only out-of-process. Four tests in Given_ProcessSignalCancellationScope, the class the stress script targets, so the changed locked region runs under its epoch races: the first SIGTERM carries 143; a SIGTERM after Ctrl+C escalates and leaves the first claim's 130 intact; SIGTERM is inert with no active scope; and a generation advanced under an active scope still claims, which is the null-generation contract. Suite: 783 tests, 782 passed, 1 skipped (Windows).
…stalling one The signal path had three platform decision points and only two were injectable: Ctrl+Break's `isWindows`, and the bridge-support predicate. The third — the `!OperatingSystem.IsWindows()` gate on the SIGTERM registration — read the host directly, so "what this platform wires up" could only be asserted on that platform. It is a wiring decision, not a capability limit. dotnet/runtime's PosixSignalRegistration.Windows.cs maps SIGINT/SIGQUIT/SIGTERM/SIGHUP onto console control events, SIGTERM to CTRL_SHUTDOWN_EVENT, so .NET accepts the registration on Windows too; the gate exists because the console coordinator already owns Ctrl+C and Ctrl+Break there. The decision therefore comes from a `SignalRegistrationPolicy` in force during test isolation rather than from the host. Whether a real registration may be created is a deliberately separate bit on that policy, defaulting to false. A test declaring a platform it is not running on must not install a live handler in the test runner's process, and — precisely because .NET would accept it — nothing else would have stopped it. Suppressing the registration does not suppress delivery: the in-process seams reach the claim logic either way. The one thing not suppressed is the framework's own process-lifetime `Console.CancelKeyPress` subscription, which `RegisterStandalone` installs once and never removes by design; a real Ctrl+C is still evaluated against the real host, never the declared platform. `TryInitializeRegistrations` split so both halves stay under the 60-line cap. Two observables for tests: whether the platform in force wants a SIGTERM registration, and whether one is live. Wanted-but-not-installed is what a platform test looks like, and is the pair that proves no registration was created on a declared platform's behalf. `When_RegistrationFailsAfterSigTerm_Then_TheOrphanedRegistrationIsReleased` loses its `[OSCondition(Exclude, Windows)]`: declaring a non-Windows platform with real registrations allowed reaches the fail-after-SIGTERM ordering on any host. It also gained an assertion pinning the failure to the injected fault — without it the test passes vacuously whenever the SIGTERM registration is what failed, leaving no orphan and never reaching the cleanup it exists for. Repl.Tests: 787 tests, 787 passed, 0 skipped — the suite now has no platform-skipped test.
Consumers had no supported way to test the signal lifecycle PR #80 introduced. It was reachable only through internals and through a runner internal to this repo's own integration tests, and Repl.Testing mentioned signals nowhere. ReplTestHost could not be the answer: sessions are isolated from one another while signal handling is process-global, and its handle serialises one command at a time, which makes a late joiner impossible to express. So ReplProcessSignalHarness is a sibling, not a member. It owns process-signal handling for its lifetime, runs the application through the only overload that installs the standalone bridge — the one taking no IServiceProvider, IHost or IReplHost, since every other overload drops the option with a diagnostic nobody asserts on — and delivers signals the way the operating system would. Every platform's decisions can be declared, so a Windows wiring decision is assertable from Linux and the other way round, and no operating-system registration is created on a declared platform's behalf. SendSignal is synchronous, not async: a signal callback owes the operating system a suppression decision before it returns, and the framework decides synchronously. An async signature would have described something the framework does not do. Only one harness can be alive at a time, enforced rather than documented. Taking ownership tears down and reinstalls shared registration state, so a second one corrupts the first's isolation instead of merely racing on the application — the failure has to be loud. The message and the type's own docs name the per-framework parallelism switches, because the package references no test framework and cannot apply one itself. Writing the tests first found three things worth keeping: - Starting a run guarantees a signal will reach it, not that the command body is executing: the scope is installed before arguments are parsed. A test asserting on cleanup must have its command say when it is running. Now documented on StartRunAsync rather than left to be discovered. - Signal diagnostics are written from whichever context delivers the signal, which in production is an operating-system callback thread with no session. They belong to the delivery, not to a run, so they are captured on the harness and ReplSignalRunResult.DiagnosticText says what it does and does not hold. - A late joiner needs the earlier run to still hold its scope. Once the last run of a claimed epoch drains, the epoch resets and the next signal is a first signal again — so the test holds the first run inside its cleanup instead of assuming the window stays open. The tokenizer and the ANSI normalizer moved to a shared internal helper rather than being copied for the second entry point. 13 tests. Suites: Repl.Tests 787/787, Repl.IntegrationTests 585 with 8 skips (the Linux-only real-process suite), McpTests 225/1 skip, SpectreTests 17, ProtocolTests 6. Solution at 0 warnings.
The spawn-and-signal machinery already existed, in the wrong place: ShellCompletionTestHostRunner and Given_ProcessSignals' helpers are internal to this repository's integration tests, so a consumer had to rediscover PID lifetime, output draining, timeouts and forced cleanup for themselves. ReplProcessProbe generalises them for an arbitrary executable. Output is drained from the moment the process starts, so a child that fills its pipe is never blocked by the probe; every wait carries what was captured into its failure message, because a test that waited on the wrong marker is otherwise indistinguishable from one whose signal never arrived; and disposal kills the tree, so a failed assertion cannot leak a blocked process into the rest of the suite. Signalling a process that has already exited is refused rather than attempted, since a reused id would reach something else. Signals are sent on Unix only, and refused loudly on Windows with a message naming what to use instead. Delivering one to another process there needs a console control event and console attachment, which would pre-empt the Windows decisions issue #83 exists to settle. Everything else — spawning, waiting on output, exit codes, cleanup — works on every platform, so a cross-platform suite shares all of it and skips only the delivery. Ctrl+Break is refused on every platform: it is a Windows console event, and the nearest Unix signal, SIGQUIT, is one this framework deliberately leaves unclaimed. Mapping it silently would have a test assert against a path that is not the one it named. The test host's process-signal scenario now echoes READY to standard output as well as to its marker file, so a caller can watch the stream. The file stays: it is the only way to observe what happened during a shutdown the process may not survive long enough to flush, and that distinction is documented on the probe's Output property rather than left as a trap. 6 tests. Two are Unix-only by nature — real delivery is the one guarantee no declared platform can substitute for — and the Windows-refusal test is the mirror image, so the CI matrix covers both sides. Suites: Repl.Tests 787/787, Repl.IntegrationTests 591 with 10 skips, McpTests 225/1, SpectreTests 17, ProtocolTests 6.
It drives the same process-global coordinator and cancel-key state as every other class carrying [DoNotParallelize], but carried none itself. That was safe only by accident: because every other class touching that state is tagged, MSTest scheduled them in its serial pass and left this one alone in the parallel one. The moment a second untagged class touches the same statics, both start racing — and the process-signal harness this branch adds is exactly the kind of thing that would.
docs/testing-toolkit.md had no mention of signals at all, so the two halves now have a section: the in-memory harness for every decision the framework makes, and the spawned-process probe for the guarantees only a real process can give. Most of it is the boundary rather than the API. A second signal makes the harness report WouldTerminateProcess, which is the framework's decision and nothing more — nothing dies in-process, so the run keeps unwinding and the code after the call keeps executing. StartRunAsync guarantees a signal will reach the run, not that the command is running, and a test asserting on cleanup has to have its command say when it started. Signal diagnostics belong to the delivery rather than to a run, because in production they are written from an operating-system callback thread with no session. A late joiner needs the earlier run to still hold its scope, since an epoch resets once its last run drains. Each of those cost a failing test to discover; none of them should cost a consumer one. The parallelism guidance is per framework — MSTest, xUnit and NUnit differ in whether they parallelise by default — because the package references none of them and cannot apply the setting itself. A table says what each half proves, so "the process actually terminated" is visibly the probe's alone. Cross-references from the configuration reference and best practices, both of which describe signal ownership without previously saying how to assert it, plus the package readme. Verified that docs/testing-toolkit.md is what publishes to /cookbook/testing/, so the anchor resolves.
Four reviewers ran before pushing. Two verdicts were approve-with-changes, skeptic's was fix-first, and three of its findings were right about code I had written and prose I had asserted. **The isolation claim was false, and so was its documentation.** `UseRealProcessSignalRegistrations` said that leaving it off kept the harness from competing for the test runner's own signals. Only the SIGTERM registration was ever gated by it: `ConsoleCancelKeyCoordinator.RegisterStandalone` is called unconditionally, and that is what subscribes the process-wide `Console.CancelKeyPress`. A real Ctrl+C during a harness run was claimed cooperatively whatever the flag said. The flag is gone. It had no observable effect through the public API either — the thing it existed to let you assert lives on an internal member, and exposing one now would pre-empt the observability API issue #84 is for. The harness now never installs an operating-system registration, which is a stronger promise than the one the flag was guarding. What cannot be isolated is documented instead of denied: starting a run registers a console cancel-key handler, because arbitrating Ctrl+C between an interactive session and a standalone run is part of what these tests exercise, so a real Ctrl+C aimed at the runner is claimed by the run under test and the first press does not stop it. **Disposal could strand process-signal ownership for the rest of the suite.** `DrainRunsAsync` enumerated the run list without the gate that `StartRunAsync` takes to append to it, and the cleanup after it had no `finally`. A throw from the drain left the exclusivity flag set and the coordinator isolated, so every later harness in the process refused to start — one failed disposal cascading into an entire suite. Disposal now takes the start gate and releases ownership unconditionally. Proven rather than asserted: reverting to the unguarded form fails three tests, including the new deterministic one that drains a run which timed out. **The stress script never covered any of this.** `Given_ProcessSignalHarness` does not contain the substring `Given_ProcessSignals` — there is no "s" after "Signal" — so the integration filter matched none of the new classes. The unit half did match its own filter, so the coordinator change was under stress; the harness and probe were not. Filter broadened, minimum raised from 8 to 32 so a future filter that silently stops matching fails loudly. **A documented example could not work.** The declared-platform snippet delivered a signal without starting a run, so it asserted `CancellationRequested` where the code returns `NotHandled`. A reader copying it failed on their first try. Also from the panel: two acceptance criteria were only provable through internals a package consumer cannot reach, so a failing cancellation callback and a run whose own outcome outranks a claimed signal now have tests at the public surface; `ReplSignalRunResult` drops its primary constructor, since a positional record bakes in a Deconstruct-arity break for any member added later; `ReplPlatformProfile` keeps its flags readable but no longer settable, so combinations no device has cannot be built; `ScopeRegisteredCallbackForTesting` folds into the isolation scope rather than sitting beside it as a fifth, unowned static; and `ReplSignalRun.Completion` documents the local-variable pattern that avoids VSTHRD003. Release build 0 warnings. Repl.Tests 787/787, IntegrationTests 596 with 10 skips, McpTests 225/1, Spectre 17, Protocol 6. Stress script green on the broadened filter.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e0edd9ff0d
ℹ️ 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".
| return await app.RunAsync( | ||
| ReplTestText.Tokenize(commandLine), | ||
| new ReplRunOptions { ProcessSignalHandling = ProcessSignalHandlingMode.Automatic }, | ||
| timeout?.Token ?? cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
When RunTimeout fires but the command catches the resulting cancellation and returns a result or exit code, app.RunAsync completes normally and this exception filter never checks the expired timeout. ReplSignalRun.Completion can therefore report a successful result even though no signal arrived, producing a false-positive signal test; inspect the timeout state after the await as well as in the exception path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in 79a0f31.
The timeout surfaced only through the exception filter, so an application that maps cancellation to an exit code — or, as one of this branch's own tests does, a command that catches it and returns Results.Exit(7) — came back normally and the run looked successful for a signal that never arrived.
It is now re-checked after a normal return as well. This is the same defect ReplSessionHandle.ThrowIfCancelledByTimeout already guards against; I had read that code and still did not carry the lesson across.
Guarded by When_TheCommandSwallowsTheTimeout_Then_TheRunStillFails.
|
|
||
| // Serialised so the readiness signal below cannot be consumed by a concurrent start. The runs | ||
| // themselves stay concurrent; only their starts queue. | ||
| await _startGate.WaitAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
If StartRunAsync passes its initial disposed check, then DisposeAsync acquires this gate first, the disposer can drain the old list, tear down isolation, and dispose the semaphore before the queued start resumes. That start can then launch an untracked run after disposal, without the harness isolation or registration callback, and overlap the next harness's process-global signal state; revalidate lifecycle ownership after acquiring the gate and coordinate semaphore disposal with queued starters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in 79a0f31.
ThrowIfDisposed ran before the gate wait, so a start that queued while disposal held the gate would resume afterwards and run outside an isolation that had already been dismantled. It is re-checked after acquiring the gate.
On the semaphore itself: a start still blocked on WaitAsync when Dispose runs gets ObjectDisposedException from the semaphore, which is the right answer for that starter, and When_DisposalRacesAStart_Then_OwnershipIsStillReleased treats it as a legitimate outcome. What it asserts is the part that must never vary — that ownership comes back — and its deterministic counterpart drains a run that faulted.
| if (_process.HasExited) | ||
| { | ||
| throw new InvalidOperationException( | ||
| Describe($"exited with code {_process.ExitCode} before writing '{expected}'")); |
There was a problem hiding this comment.
For a short-lived child that writes the expected marker and exits immediately, Process.HasExited can become true before the asynchronous OutputDataReceived callback appends the final buffered line. This branch then reports that the child exited without the marker even though it wrote it; drain the asynchronous output handlers and recheck the capture before throwing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in 79a0f31.
HasExited says the process is gone, not that its last line has reached the capture callback. The branch now calls the parameterless WaitForExit, which drains the asynchronous handlers, and re-checks the capture before it calls the wait a failure. Without that, a child that wrote the marker and exited promptly would fail a test intermittently — the worst kind.
| exitCode = await RunAsync(app, commandLine, timeout, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| ReplSessionIO.RemoveSession(sessionId); |
There was a problem hiding this comment.
When the application run faults or is converted to a TimeoutException, control skips this removal. Because the preceding SetSession receives an explicit sessionId, its scope does not remove that entry on disposal, so every failed or timed-out harness run leaves stale metadata in ReplSessionIO's process-wide session dictionary for the rest of the test process; move removal into a finally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in 79a0f31 — the removal moved into a finally.
The session scope restores the ambient writers but does not remove an explicitly named session, so every failed or timed-out run left an entry in the process-wide dictionary for the life of the test process. Since this branch also adds a test that deliberately times a run out, it would have started accumulating them immediately.
| ReplProcessSignal.Break => ConsoleCancelKeyCoordinator.HandleCancelKeyForTesting( | ||
| ConsoleSpecialKey.ControlBreak, | ||
| isWindows: _options.Platform.IsWindows), | ||
| ReplProcessSignal.Terminate => ProcessSignalCoordinator.HandleSigTermForTesting(), |
There was a problem hiding this comment.
When the declared platform is Windows or one of the unsupported mobile/browser profiles, the coordinator intentionally installs no SIGTERM registration, but this branch bypasses that wiring decision and calls the claim logic directly. SendSignal(Terminate) therefore cancels the run and reports CancellationRequested on platforms where the framework would never receive SIGTERM, yielding false-positive cross-platform lifecycle tests; return NotHandled unless the selected profile declares a usable SIGTERM bridge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this is the sharpest finding of the set — thank you.
The whole reason the platform is declarable is so a cross-platform test tells the truth about the platform it names. SendSignal(Terminate) went straight to the claim logic and bypassed the wiring decision, so on a declared Windows profile — where the framework installs no SIGTERM registration — or an unsupported one, it reported a cancellation that could never happen there.
Fixed in 79a0f31: delivery is now gated on SigTermRegistrationDeclaredForTesting, the observable that already exists for exactly this question.
The change immediately failed two of this branch's own tests. Both asserted a SIGTERM claim under the default profile, which on a Windows host is Windows — they were asserting the very false positive you describe. They declare ReplPlatformProfile.Unix now, which is both honest and portable. A new [DataRow] pair pins each direction: declared Unix claims, declared Windows does not.
| if (!_process.HasExited) | ||
| { | ||
| _process.Kill(entireProcessTree: true); | ||
| await _process.WaitForExitAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
When the child exits naturally after this HasExited check but before Kill, Kill can throw because there is no longer a running process to terminate, turning an otherwise successful test into a teardown failure. Treat the already-exited race as successful cleanup while preserving failures that indicate the child may still be alive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct. Fixed in 79a0f31.
Kill throws when the process has already exited, so winning the race turned into a teardown failure for a test that had otherwise passed. The InvalidOperationException is caught and treated as successful cleanup, which is the outcome that check wanted anyway; anything else still propagates.
| var isolation = ProcessSignalCoordinator.IsolateRegistrationsForTesting( | ||
| registrationFault: options.RegistrationFault, | ||
| policy: options.Platform.ToPolicy(), | ||
| scopeRegisteredCallback: scopeRegistered.Signal); |
There was a problem hiding this comment.
If any non-harness ReplApp.RunAsync with automatic signal handling is active when this harness is created, isolation tears down that run's live registrations while leaving its scope in ActiveScopes. A later synthetic signal from the harness then cancels both the harness run and the unrelated run, so the advertised ownership guard does not prevent cross-test corruption; acquire coordinator-wide exclusivity or refuse creation while any external scope is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it is a wider hole than the one I had guarded. Fixed in 79a0f31.
I had protected against a second harness but not against a plain RunAsync with automatic handling already in flight. Isolation tears down that run's registrations without removing its scope from ActiveScopes, so the first signal this harness delivered would cancel it too.
Create now refuses while any scope is active, with a message explaining why, using a new ActiveScopeCountForTesting on the coordinator. That matches the stance already taken for the two-harness case: for a shipped package used by unknown consumers, failing loudly beats being intermittently wrong.
Guarded by When_AnUnrelatedRunIsInFlight_Then_CreatingAHarnessIsRefused.
| var inQuotes = false; | ||
| foreach (var ch in value) | ||
| { | ||
| if (ch == '"') | ||
| { | ||
| inQuotes = !inQuotes; |
There was a problem hiding this comment.
The new signal harness exposes only a command-line string and documents it as shell-tokenized, but this tokenizer merely toggles on every double quote: it drops empty quoted arguments, treats single quotes as literal characters, and cannot represent escaped quotes correctly. Commands such as work "" or work 'two words' therefore reach the application with the wrong argument vector; either provide an argv overload/use a real command-line parser or narrow the contract and reject unsupported quoting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Half accepted, and I have narrowed the contract rather than widened the implementation.
You are right that the documentation overclaimed: StartRunAsync said "tokenized the way a shell would", which this tokenizer is not. 79a0f31 says what it does instead — split on whitespace, double quotes group, single quotes literal, no escape handling, an empty quoted argument produces no token — on the parameter and on ReplTestText itself.
I am not replacing the tokenizer in this PR. It is pre-existing behaviour, moved unchanged from ReplSessionHandle where it has always backed RunCommandAsync; swapping in a real command-line parser would change that shipped surface's behaviour for every existing consumer, which is a deliberate decision rather than a side effect of adding a signal harness. An argv overload is a reasonable addition, and a better one once someone needs it.
| /// The command's events in order: what it wrote, then each interaction it raised, then the result | ||
| /// it produced. Use this when the ordering between output and interactions is what matters. |
There was a problem hiding this comment.
This new documentation tells consumers to use TimelineEvents for ordering between output and interactions, but BuildTimeline always emits all captured output as one aggregate event before every interaction, regardless of their actual chronology. Tests following this contract can therefore assert an ordering that was never observed; either capture output writes as events in real time or remove the ordering guarantee.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it is my own new documentation that was wrong. Fixed in 79a0f31.
BuildTimeline emits all captured output as one aggregate event before every interaction, so there is no chronology between them to assert on. The doc now says exactly that: output is a single event captured at the end, then the interactions in the order they were observed, then the result — and that an assertion about output landing before or after an interaction is asserting this shape, not an observed ordering.
Capturing output writes as events in real time is the other fix, and a better one, but it changes CommandExecution for every existing consumer of the session handle. Not something to do in passing here.
| // Also on standard output, for a caller watching the stream rather than the file. The file | ||
| // stays because it is the only way to observe what happened during a shutdown the process | ||
| // may not survive long enough to flush. | ||
| Console.WriteLine("READY"); |
There was a problem hiding this comment.
Dogfooding: we should use our own IO, never write to the console directly.
There was a problem hiding this comment.
Fixed in 79a0f31 — you are right, and the rest of the repository does not do this.
The handler takes IReplIoContext and writes through io.Output, the same way samples/07-spectre does. The marker file stays alongside it, because it is the only thing that survives a shutdown the process may not live long enough to flush — that distinction is documented on ReplProcessProbe.Output.
Verified rather than assumed: When_ASignalIsSentOnWindows_Then_ItIsRefusedWithGuidance waits on READY arriving over stdout before it asserts anything, and it still passes, so the session writer does reach the redirected stream and is flushed.
Ten threads, all of them right. **Dogfooding, from the owner.** The test host wrote its readiness marker with `Console.WriteLine`. It goes through `IReplIoContext.Output` now, like any other command in this repository. **A declared platform could claim a signal it would never receive.** `SendSignal(Terminate)` called the claim logic directly, bypassing the wiring decision the declared platform makes. On a declared Windows profile the framework installs no SIGTERM registration, and on an unsupported one it installs nothing at all, so the harness reported a cancellation that could not happen on the platform the test named — the exact false positive the platform-declaration feature exists to prevent. Gated on whether SIGTERM is actually wired for the platform in force. Two of this branch's own tests failed immediately on that change, because both asserted a SIGTERM claim under the default profile, which on a Windows host is Windows. They were asserting the false positive. They declare Unix now, so they say what they mean and pass identically on every host. **A swallowed timeout was reported as success.** The timeout only surfaced through an exception filter, so an application that maps cancellation to an exit code — or a command that catches it — returned normally and the run looked like it had succeeded, for a signal that never arrived. The timeout is now checked after a normal return too, the way `ReplSessionHandle` already does. **A start queued behind disposal could launch into a torn-down harness.** `StartRunAsync` checked disposal before waiting on the start gate, so a start that queued while disposal held the gate would resume afterwards and run outside the isolation that had just been dismantled. Re-checked after acquiring it. **Isolating while someone else's run is in flight corrupts that run.** Taking ownership tears down registrations without touching the scopes using them, so an unrelated automatic run would keep its place in the epoch and be cancelled by this harness's first signal. Creation is refused while any scope is active, with a message saying why. Also: the probe could reject a child that had written its marker but not yet flushed it through the asynchronous capture, and could throw during disposal when the child exited between the check and the kill; a failed run left its explicitly named session in the process-wide dictionary; the tokenizer was documented as shell-like when it splits on whitespace and groups with double quotes and nothing more; and `TimelineEvents` promised an ordering between output and interactions that `BuildTimeline` does not produce — output is one aggregate event, not interleaved. Three new guards: SIGTERM honours the declared wiring, a swallowed timeout still fails, and a harness is refused while an unrelated run holds a scope. Release build 0 warnings. Repl.Tests 787/787, IntegrationTests 600 with 10 skips, McpTests 225/1, Spectre 17, Protocol 6.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79a0f318de
ℹ️ 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".
| // Taking ownership tears down the registrations without touching the scopes that were using | ||
| // them, so a run already in flight would keep its place in the epoch and be cancelled by the | ||
| // first signal this harness delivers. Refuse rather than corrupt something unrelated. | ||
| if (ProcessSignalCoordinator.ActiveScopeCountForTesting != 0) |
There was a problem hiding this comment.
Fresh evidence in the revised code is that this newly added guard only snapshots ActiveScopes; it does not exclude a normal automatic ReplApp.RunAsync from registering immediately after the check or while the harness remains alive. Such a run joins the harness's isolated epoch and invokes its shared registration callback, so it can make StartRunAsync return before the harness run has registered and will then be cancelled by SendSignal; acquire coordinator ownership atomically with ProcessSignalCoordinator.Register and retain it until disposal.
Useful? React with 👍 / 👎.
| await Task.Delay(TimeSpan.FromMilliseconds(25), Clock, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| throw new TimeoutException(Describe($"did not write '{expected}' within {_options.Timeout}")); |
There was a problem hiding this comment.
If the child writes the expected marker during the final 25 ms delay just before the configured deadline, the loop resumes after the deadline and throws here without reading the capture again. This can reject a readiness marker that arrived within the timeout, with the exception's captured output potentially containing that same marker; perform a final capture check before reporting the timeout or use an event-driven wait.
Useful? React with 👍 / 👎.
| using (ReplSessionIO.SetSession( | ||
| output, | ||
| TextReader.Null, | ||
| sessionId: sessionId, | ||
| commandOutput: output, |
There was a problem hiding this comment.
Pass isHostedSession: false when installing this capture session. ReplSessionIO.SetSession defaults it to true, so every supposedly standalone harness run is classified as ReplRuntimeChannel.Session rather than Cli; commands in CLI-only modules are consequently absent, handlers observe IReplIoContext.IsHostedSession == true, and output policies use the hosted surface, making signal tests exercise a different application path from the process-owning invocation they claim to model.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| _disposed = true; | ||
| if (!_process.HasExited) |
There was a problem hiding this comment.
When the probed executable starts a long-lived descendant and then exits before disposal, this guard skips Kill(entireProcessTree: true) solely because the parent has exited, leaving the descendant running after the probe is disposed. That violates the probe's stated process-tree cleanup guarantee and can leak servers or workers into later tests; the child needs to be launched in a trackable process group/job or its descendants otherwise retained so cleanup does not depend on the parent still being alive.
Useful? React with 👍 / 👎.
| exitCode = await app.RunAsync( | ||
| ReplTestText.Tokenize(commandLine), | ||
| new ReplRunOptions { ProcessSignalHandling = ProcessSignalHandlingMode.Automatic }, | ||
| timeout?.Token ?? cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
The post-await timeout check added for commands that swallow cancellation is still unreachable when a handler ignores the token or blocks indefinitely during cancellation cleanup: CancelAfter only requests cancellation, while this await continues waiting forever. In that scenario ReplSignalRun.Completion never produces the promised TimeoutException, and DisposeAsync also hangs while draining the run, defeating the timeout's stated purpose of preventing a stuck signal test; enforce a separate wall-clock wait or explicitly remove the completion/deadlock guarantee for non-cooperative runs.
Useful? React with 👍 / 👎.
1 participant
Closes #82.
PR #80 gave the framework process-wide first/second-signal coordination. Consumers of the shipped
Repl.Testingpackage had no supported way to test it: the behaviour was reachable only through internals, and through a runner internal to this repository's own integration tests.docs/testing-toolkit.mddid not mention signals at all.Two halves, because they prove different things
ReplProcessSignalHarnessdrives the lifecycle in memory — deterministic, fast, and able to declare any platform's decisions from any host.ReplProcessProbespawns an application and sends it real signals, for the one guarantee no in-memory test can make: that a process actually terminates, with the code a shell sees.It is a sibling of
ReplTestHostrather than part of it. Sessions are isolated from one another; signal handling is process-global.ReplSessionHandlealso serialises one command at a time, which makes a late joiner inexpressible.Every platform's decisions, from every platform
The signal path had three platform decision points and only two were injectable. The third — the gate on the SIGTERM registration — read the host directly, so "what this platform wires up" could only be asserted on that platform.
It is a wiring decision, not a capability limit:
dotnet/runtime'sPosixSignalRegistration.Windows.csmapsSIGTERMontoCTRL_SHUTDOWN_EVENT, so .NET accepts the registration on Windows too; the gate exists because the console coordinator already owns Ctrl+C and Ctrl+Break there. It now comes from a policy in force during test isolation.Because .NET would accept it, a declared platform must never install a live handler in the test runner's process — so that is a separate bit, and the harness never sets it. Measured result:
Repl.Testswent from one platform-skipped test to zero. The orphaned-registration test that was excluded on Windows now runs there, and its passing is itself the empirical confirmation of the runtime-source reading above.What it will not claim
A second signal reports
WouldTerminateProcess. That is the framework's decision and nothing more — nothing dies in-process, so the run keeps unwinding and the code after the call keeps executing.docs/testing-toolkit.mdcarries a table of what each half proves, so "the process actually terminated" is visibly the probe's alone.Three contract details cost a failing test to discover and are documented so they cost a consumer none: starting a run guarantees a signal will reach it, not that the command is running; signal diagnostics belong to the delivery rather than to a run, because in production they are written from a callback thread with no session; and a late joiner needs the earlier run to still hold its scope, since an epoch resets once its last run drains.
Signals are delivered by the probe on Unix only. Doing it on Windows needs a console control event and console attachment, which would pre-empt the decisions #83 exists to settle;
SendSignalAsyncrefuses there with a message naming the alternative, and everything else works everywhere.Reviewed locally before pushing
Four reviewers ran on the complete diff. Three findings were right about code and prose I had written, and all three are fixed in
e0edd9f:UseRealProcessSignalRegistrationsclaimed to gate whether the harness competed for the runner's own signals; it gated only the SIGTERM registration, while the console cancel-key registration — the one that subscribes the process-wideConsole.CancelKeyPress— was unconditional. The option is gone, the harness now never installs an operating-system registration at all, and what genuinely cannot be isolated is documented rather than denied.finally, so one throw left the exclusivity flag set and every later harness refused to start. Reverting the fix fails three tests, including a deterministic one.Given_ProcessSignalHarnessdoes not contain the substringGiven_ProcessSignals. Filter broadened, minimum raised 8 → 32.Also: a documented example delivered a signal without starting a run and asserted the wrong value; two acceptance criteria were only provable through internals and now have tests at the public surface;
ReplSignalRunResultdropped its primary constructor to avoid baking in aDeconstruct-arity break;ReplPlatformProfile's flags are readable but no longer settable.Verification
-warnaserror: 0 warnings.Repl.Tests787/787 (0 skipped, was 1) ·Repl.IntegrationTests596, 10 skipped (the platform-bound real-signal tests) ·Repl.McpTests225/1 ·Repl.SpectreTests17 ·Repl.ProtocolTests6.eng/ci/process-signal-stress.shgreen on the broadened filter.markdownlint-cli2 "docs/**/*.md": 0 issues.Repl.Testingalso stops shipping without XML documentation (first commit, isolated): it carried the test-project override although it is a package on NuGet, so consumers got no IntelliSense. Turning generation on armed CS1591 and surfaced 20 undocumented public members, all now documented.