Apply upstream Go signal fix through build overlay - #268
Conversation
Generate a targeted overlay from the selected Go toolchain using the upstream fix for golang/go#81009. Remove the DCP-specific exec shim and add a Darwin child-process regression test that proves the overlay is active. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Darwin regression test fails in supported direct and IDE test workflows because those test binaries are built without the overlay.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Applies the upstream Darwin Go runtime signal fix via a generated build overlay and removes the DCP-specific exec shim.
Changes:
- Generates and applies a validated Go runtime overlay on Darwin.
- Replaces the signal workaround with an
OSExecutorregression test. - Removes obsolete signal-disposition and exec-shim code.
| File | Description |
|---|---|
Makefile |
Generates and applies the Darwin runtime overlay. |
internal/dcp/commands/root.go |
Removes the hidden exec-shim command. |
internal/dcpproc/commands/fork_process.go |
Launches forked processes directly. |
internal/dcpproc/commands/fork_process_darwin_test.go |
Removes shim-specific tests. |
internal/dcpproc/commands/fork_process_exec.go |
Removes the exec-shim implementation. |
internal/dcpproc/commands/fork_process_test.go |
Removes exec-shim unit tests. |
internal/dcpproc/fork_process_darwin_test.go |
Removes workaround-specific integration tests. |
internal/tools/goruntimeoverlay/go-81009.patch |
Contains the upstream runtime patch. |
internal/tools/goruntimeoverlay/main.go |
Generates and validates the overlay. |
internal/tools/goruntimeoverlay/main_test.go |
Tests overlay generation and rejection paths. |
pkg/process/os_executor_darwin_test.go |
Adds the Darwin process-launch regression test. |
pkg/process/signal_disposition_darwin.go |
Removes Darwin signal manipulation. |
pkg/process/signal_disposition_darwin_test.go |
Removes signal workaround tests. |
pkg/process/signal_disposition_other.go |
Removes cross-platform workaround stubs. |
test/signaldisposition/main.go |
Adds the pre-runtime signal-state observer. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Build the signal regression launcher with the runtime overlay and have it start the observer through OSExecutor. This keeps the regression effective while allowing direct go test and IDE runs to use an unpatched test binary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Document the generator transition checks and Darwin signal regression as temporary coverage to remove when DCP requires a Go release containing golang/go#81009. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The dcptun container client is built for Linux, so it does not need the Darwin runtime overlay or its generation prerequisite. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Karol Zadora-Przylecki (karolz-ms)
left a comment
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Automated review pass focused on process-invocation handling, error handling, and build-time correctness. Findings below, ordered by severity.
One of these is a reproducible failure rather than a theoretical concern: the embedded go-81009.patch is line-ending sensitive, and go test ./... fails on a clean Windows checkout at head 9cea1d60. Details in the inline comment.
I could not execute anything on macOS, so the central claim — that the overlay eliminates the dirty SIGUSR1 disposition end-to-end — still rests on your validation. Two things I'd flag for a human on macOS that I did not file inline: nothing currently verifies that the shipped bin/dcp is actually built with the overlay (the Makefile is now the single point of failure for the fix, with no test guarding it), and the signal-disposition tool is the repo's first CGO_ENABLED=1 build, landing on a macos-latest runner pinned to architecture: 'x64'.
Overall the direction looks right to me: fixing the runtime rather than wrapping each launch path is the better layer, and the generator fails closed when the toolchain source is unrecognized.
Make Git patch application independent of line-ending configuration, distinguish Git launch failures, remove overlay arguments from Linux-only helper builds, and improve the signal regression launcher's timeout and self-exec handling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/backport to releasel/0.26 |
|
Started backporting to releasel/0.26: https://github.com/microsoft/dcp/actions/runs/35665203942 |
|
David Negstad (@danegsta) an error occurred while backporting to |
|
/backport to release/0.26 |
|
Started backporting to release/0.26: https://github.com/microsoft/dcp/actions/runs/35665528541 |
|
David Negstad (@danegsta) backport PR couldn't be created automatically, please create the backport PR manually! |
Summary
Go 1.27 on macOS can leave an exec'd child with a default
SIGUSR1handler while retainingSA_SIGINFO. This contradictory signal state causes NativeAOT .NET processes to crash and affects bothfork-processand DCP's general process-launching paths.Apply the upstream golang/go#81009 runtime fix to Darwin builds through a generated Go overlay. The generator starts from the selected toolchain's current
os_darwin.go, applies a checked-in Git patch containing only the upstream change, and fails closed if the source is neither patchable nor already fixed. This allows the DCP-specific exec shim and signal-disposition implementation to be removed.The overlay is temporary until the selected Go toolchain includes the fix. Generating the complete overlay file from the active toolchain avoids carrying a static standard-library copy that could omit unrelated servicing fixes.
Validation
OSExecutorand capturesSIGUSR1before the child Go runtime initializes.default=true flags=0x42without it.make lintmake testCloses #267