fix(runtime): cancel standalone runs on process signals - #80
Conversation
Bridge SIGTERM into the standalone RunAsync cancellation token so handlers can complete cleanup before the process exits with code 143. Keep externally hosted runs caller-owned and expose an opt-out for custom process lifecycles.\n\nAdd a real child-process regression test for yllibed#79.
Handle SIGINT cooperatively for standalone runs on Unix and Windows while yielding to the existing interactive CancelKeyHandler semantics. Keep the embedded console profile caller-owned by default and cover second-signal fallback with a real child process.
autocarl
changed the title
|
@codex A review of this please. |
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: 5d5ae4286b
ℹ️ 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".
Centralize Ctrl+C and SIGTERM ownership in process-wide coordinators. Preserve profile defaults, drain cancellation epochs before reset, and resolve exit codes only after releasing signal ownership. Add deterministic race coverage plus real sync/async signal integration tests.
Document profile inheritance, first- and second-signal behavior, 130/143 conventions, exit-code precedence, token lifetime, diagnostics, host ownership, and platform limitations.
Preserve caller ownership for unprofiled and externally managed runs, cover Ctrl+Break and Mac Catalyst, diagnose callback and registration failures, and add deterministic concurrency plus replayable stress coverage.
|
Review-hardening update, superseding the earlier summary, is published through Highlights:
Exact-head local validation:
Broader follow-ups remain tracked in #81, #82, #83, and #84. Exact-head CI for |
Keep Mac Catalyst on the unsupported mobile path, leave Unix SIGQUIT to the operating system, and clarify one-shot versus interactive cancellation-token contracts with real-process regression coverage.
|
@codex a review please. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
… None Make `ProcessSignalHandlingMode.None` the zero value so an unset configuration field or a zero-initialized value agrees with the caller-owned application default. Enum values are baked into consumer assemblies, so this is only free before release; every existing use is by name. Treat a rejected signal registration as a degraded bridge rather than a fatal error: diagnose it, latch the attempt, and continue caller-owned, matching the unsupported-platform path. Automatic is the CLI-profile default, so a restricted environment previously turned a working command into one that never executed, for that run and every later run in the process. Also: - run the process-signal stress harness in CI at low iteration counts, so the epoch races it guards are gated instead of relying on a local run - state signal ownership, exit codes and the per-run opt-out in the `UseCliProfile` and `UseDefaultInteractive` summaries, where a consumer meets them, and pin the interactive profile's ownership with a test - record the exit-code and handler-token changes for existing profile users under a CHANGELOG `Changed` heading - assert the second-signal escalation diagnostic, list the unclaimed SIGQUIT exit code in the reference table, and cover each platform flag on its own row instead of restating the predicate once - scope the race and reentrancy test descriptions to what they actually exercise: the gate serializes both dispatches, so neither proves atomicity
The script built the solution without `--no-restore`, so the incremental restore audited no projects and tripped the CI-only NuGet audit assertion in `src/Directory.Solution.targets` whenever the caller had already restored. Follow the repository convention used by every workflow job: an explicit `dotnet restore --force`, then a build with `--no-restore`. The script stays self-contained when run on its own.
Three parts of this contract are hard to hold in the head from prose alone: which of the three inputs decides a run's mode, how the process-wide epoch moves between its states, and which console key each owner actually claims. Add a mermaid diagram for each, next to the prose it illustrates. They carry the skeleton only — the epoch diagram labels its edges with the step numbers of the list above it rather than restating them, so the two cannot drift apart. Each diagram was rendered and read before landing; the ownership flow also records that a rejected registration degrades to caller-owned handling.
|
@codex last review? |
Review Finding[P2] Restored signal registrations use an invalidated generation
No code changes were made, so no commit or follow-up pull request was created. Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57fa296df
ℹ️ 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".
| runExitCode = await RunWithServicesAsync(args, provider, runOptions, signals.Token) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
When automatic signal handling is combined with HostedServiceLifecycleMode.Head and SIGINT/SIGTERM arrives while an IHostedService.StartAsync is pending, this token cancellation throws OperationCanceledException, but HostedServiceLifecycleCoordinator.StartAsync wraps it in HostedServiceLifecycleException; RunWithServicesAsync then converts it to exit code 1, and ResolveExitCode preserves that nonzero value. Consequently the newly documented cooperative signal path returns 1 instead of 130/143 whenever the signal interrupts hosted-service startup; cancellation caused by the signal should be allowed to reach the outer signal-aware catch rather than being treated as a lifecycle failure.
Useful? React with 👍 / 👎.
An independent review of the previous three commits found the registration fault scope was a trap. Registrations capture the generation counter they were created under; the scope put a saved registration object back after the counter had moved, so it came back permanently stale. Any scope test running after a fault test then got `NotHandled` instead of a claimed signal, and the latch made sure the bridge was never reinstalled. It only stayed green because MSTest happens to run this class in declaration order, and the stress harness loops exactly this class. Replace it with an isolation scope that tears registrations down on both entry and exit and lets the next run install fresh ones. A regression test warms up real registrations first, since the trap only exists when there is something to restore. Also from that review: - collapse the two registration-degradation channels — an `out bool` for "unsupported" and a nullable record for "rejected" — into one outcome with a single diagnostic site, so a third reason does not need a third channel - let the fault be injected after the SIGTERM registration, which is the only ordering that reaches the orphaned-registration cleanup, and cover it - correct three claims that were wrong: `IsCancellationRequested` does not throw on a disposed source, it silently reports `false`, which is the dangerous case a consumer needs told; `UseDefaultInteractive` does not report 130 for a Ctrl+C inside an interactive command, which cancels only that command; and the console-key result enum is aggregated across handlers, so callers are not the only reader of its third state - correct the two diagrams the previous commit got wrong: a run whose bridge failed to install still receives a linked run-scoped token, and a degraded process still starts and stops runs, it just never reaches Claimed - state the ownership rule once on `ProcessSignalHandlingMode.Automatic` and have both profile summaries point at it, rather than three drifting copies - assert that the run-scoped token is linked to the caller token, not merely distinct from it - rename the concurrency test to what it asserts, now that its description admits it cannot prove atomicity - give the stress harness its own CI job, so unrelated setup cannot silently drop signal coverage and the check name describes what it gates
2 participants
Summary
ReplApp.Run(...)/RunAsync(...)calls into cooperative handler cancellationSIGTERMon supported Unix platformsReplApp.Create(), embedded profiles, and externally managed host/provider overloadsAutomaticrequests, and consumer cancellation-callback failuresFixes #79.
Ownership model
ReplRunOptions.ProcessSignalHandlingis nullable.nullinherits the active app/profile default:UseCliProfile()/UseDefaultInteractive():AutomaticReplApp.Create():NoneUseEmbeddedConsoleProfile():NoneSupplying unrelated options therefore no longer makes an embedded console silently install standalone process handlers. An embedded or unprofiled caller can still opt in explicitly for one run.
Automaticapplies to standalone overloads that use internally configured services. ExternalIServiceProvider,IHost, andIReplHostoverloads retain ownership of process signals. Their one-shot handlers receive the caller token unchanged. An explicitAutomaticrequest on those overloads is diagnosed and ignored. If one of those runs enters Repl's interactive loop, each command receives a command-scoped token linked to the caller token so the loop can retain its separate Ctrl+C policy.The process registrations are installed lazily once and remain inert without an active automatic run. This avoids runtime callback-snapshot races during registration teardown. A first signal atomically claims an epoch, cancels every active scope, and immediately cancels late joiners. A second signal falls through to the operating-system default. The epoch resets only after its final scope and all signal-triggered cancellation callbacks have drained.
Cancellation-callback draining is intentionally unbounded. Resetting an epoch while a callback remains active could cause a later signal to be suppressed as a new first signal. If draining never completes, every later supported signal falls through to operating-system termination.
Android, browser, iOS (including Mac Catalyst), and tvOS diagnose the unavailable bridge and leave cancellation to their platform host. .NET classifies Mac Catalyst in its iOS-like mobile family and compiles the platform-not-supported POSIX signal registration there.
Exit behavior
130(128 + 2)130as a Repl compatibility policy on Windows143(128 + 15)ControlBreak, but Repl does not reinterpret it as SIGINTThe
128 + signal numbercalculation is documented as a widely adopted Unix/Bash convention, not a universal POSIX, .NET, or Windows guarantee.Regression coverage
Real child-process tests cover:
130143ProcessSignalHandlingMode.NoneDeterministic unit tests cover:
ControlBreakon UnixValidation
dotnet build src/Repl.slnx -c Release -warnaserror --no-restore --nologo— 0 warnings, 0 errors./eng/ci/process-signal-stress.sh— 50/50 unit iterations × 17 tests and 20/20 integration iterations × 8 testsnpx --yes markdownlint-cli2@0.19.1 CHANGELOG.md docs/best-practices.md docs/configuration-reference.md src/Repl.Defaults/README.md— 0 errorsgit diff --check— cleanFollow-up design work
Kept out of this focused change and tracked separately:
Repl.Testing