Skip to content

Fix forwarded process signal encoding - #1778

Merged
jglogan merged 1 commit into
apple:mainfrom
vincent-peng:fix-forwarded-process-signals
Oct 22, 2026
Merged

jglogan merged 1 commit into
apple:mainfrom
vincent-peng:fix-forwarded-process-signals

Conversation

@vincent-peng

@vincent-peng vincent-peng commented Jun 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Root cause

Foreground non-tty signal forwarding sent .signal as an integer, but the API server reads that field as a string before forwarding it to the runtime. That made forwarded resize signals fail with missing signal in xpc message. A numeric-only fix would also risk mis-mapping Darwin USR1/USR2 values to Linux.

Validation

  • swift test --filter ContainerAPIClientTests.ClientProcessTests
  • swift test --filter ContainerAPIClientTests
  • swift build --target ContainerAPIClient

@JeronimoColon

Copy link
Copy Markdown

I hit this independently while building on ContainerAPIClient, in a form broader than the resize noise in #1747: ClientProcess.kill fails for every signal, so there is no working way to signal an exec'd process at all. A timeout built on createProcess can stop waiting for a command, but cannot terminate it. I filed #1941 with a standalone reproducer and the root-cause trace.

Two notes from checking this change against the server path:

  • The encoding checks out. The server-side parse (Signal.init(_:) in containerization) uppercases, trims an optional SIG prefix, and resolves the name against the Linux table, so the prefix-less names this PR sends parse correctly. That parser and its tables are unchanged from containerization 0.33.3 through current main, so a client carrying this fix also works against already-installed 1.0.0 and 1.1.0 API servers.
  • Consider adding a KILL case to the tests. Timeout and cancel implementations mostly send SIGKILL, so it is the value most callers will hit first. It passes the guard today (Signal.platformName(SIGKILL) resolves to "KILL", which is in the Linux table); a test would pin that.

Sending names instead of numbers also avoids the Darwin/Linux mismatch on USR1 and USR2, which a numeric fix would have gotten wrong. Nice work.

@vincent-peng

Copy link
Copy Markdown
Contributor Author

Thanks, this was helpful. Added explicit SIGKILL coverage in b08d725 to pin the timeout/cancel path.

I also added a Darwin-only SIGINFO rejection case so the client-side guard keeps host-only signal names from reaching the runtime. Keeping the name-based encoding unchanged so USR1/USR2 continue mapping by signal name rather than Darwin numeric value.

@jglogan

jglogan commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

@vincent-peng We've been swamped with security issues and other backlog but your fix looks good if can rebase and add verified commit signatures (see https://github.com/apple/containerization/blob/0cb4d5bdd8e575e2bdbeb03278c0c07d7a80a989/CONTRIBUTING.md#pull-requests). Thank you for the contribution!

@jglogan

jglogan commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

@vincent-peng it looks like the original commits lack signatures but your last commit is good. When you rebase onto main, perhaps squash your changes into a single signed comment and we can do a CI run on that.

@vincent-peng
vincent-peng force-pushed the fix-forwarded-process-signals branch from c58fd89 to ca4884f Compare October 21, 2026 21:19
@vincent-peng vincent-peng reopened this Oct 21, 2026
@vincent-peng

vincent-peng commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

@jglogan Done. rebased onto latest main and squashed into a single signed commit (de0063a). Should be good for a CI run whenever you're ready, thanks!

@vincent-peng
vincent-peng force-pushed the fix-forwarded-process-signals branch from de0063a to b37ed58 Compare October 22, 2026 00:17
@vincent-peng vincent-peng reopened this Oct 22, 2026
@jglogan

jglogan commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Thanks for the quick turn! Building now.

@github-actions

Copy link
Copy Markdown

Code Coverage

Tier Line Coverage
Unit 22.78%
Integration 69.39%
Combined 77.97%

@vincent-peng
vincent-peng requested a review from jglogan October 22, 2026 07:17

@jglogan jglogan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincent-peng Everything looks good. I've just added an API compatibility note for documentation, and some suggestions for future work here that don't need to gate this PR. Thanks again for your work and for your patience!

request.set(key: .processIdentifier, value: id)
request.set(key: .signal, value: Int64(signal))
request.set(key: .processIdentifier, value: processId)
request.set(key: .signal, value: try forwardedSignalName(signal))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This is a breaking change to the XPC "wire protocol", but it fixes a client-server incompatibility that has existed for all 1.x releases (#628 added the Int64 signal on the server side and #1589 changed it to String on the server and in ContainerClient but not ClientProcess).

From a compatibility perspective, if we consider all application releases since 1.0.0 to use "version 1" of the XPC API, all clients prior to the next (1.7.0) release will see #1876 on a version 1 container-apiserver, and all subsequent version 1 API clients will work against any version 1 server.


try await xpcClient.send(request)
static func forwardedSignalName(_ signal: Int32) throws -> String {
guard let name = Signal.platformName(signal), Signal.linux[name] != nil else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-on for anyone who wants it: rework this check using the os value from the running container image's platform. The best solution may be to propose a change down in Containerization.Signal, as right now it's platform helper is based on the host platform, which doesn't make much sense for container signals.

}

try await xpcClient.send(request)
static func forwardedSignalName(_ signal: Int32) throws -> String {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chengboonrong Numeric fallback here now in #1997?

andrewkomkov pushed a commit to getgantry/gantry that referenced this pull request Nov 3, 2026
apple/container **1.6.1** is out (previously tracked: `1.2.0`).

Upstream notes: https://github.com/apple/container/releases/tag/1.6.1 —
mirrored in `docs/upstream/apple-container-1.6.1.md`.

## Review checklist

- [ ] New or changed CLI flags Gantry should surface (`container
run/create/machine/build`)
- [ ] Changed `--format json` shapes the DockerKit apple transport
decodes
- [ ] Fixed upstream bugs Gantry currently works around
- [ ] `ContainerTooling.recommendedVersion` / feature gates need moving
to `1.6.1`
- [ ] MCP tools and App Intents that expose the affected commands
- [ ] README and CHANGELOG entries for whatever is adopted

Merging records the version as reviewed. Implement the adopted parts on
this branch, or merge as-is and open follow-ups.

---

<details><summary>Upstream release notes</summary>

⌨️ denotes breaking CLI changes.

👩‍💻 denotes breaking API changes.

- Core
- [Fix signals not properly passed to container
processes](apple/container#2276)
- Kubernetes
- [Fix problems when creating clusters
concurrently](apple/container#2276)
- [Rename to `container
cluster`](apple/container#2427)

## What's Changed
* Fix host port conflicts and leftover failed nodes during cluster
creation by @dkovba in apple/container#2399
* Fix forwarded process signal encoding by @vincent-peng in
apple/container#1778
* Remove SIGWINCH from signal kill set by @ArasanDev in
apple/container#1854
* Infer platform and runtime from OCI image by @noah-thor in
apple/container#2422
* rename to container cluster by @crosbymichael in
apple/container#2427

## New Contributors
* @vincent-peng made their first contribution in
apple/container#1778
* @ArasanDev made their first contribution in
apple/container#1854

**Full Changelog**:
apple/container@1.6.0...1.6.1

</details>

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
benoitf pushed a commit to socktainer/socktainer that referenced this pull request Nov 3, 2026
)

apple/container 1.6.1 pins containerization 0.53.0. No source changes
are needed here.

The release fixes ClientProcess.kill (apple/container#1778), which sent
the signal as an integer while the API server reads it as a name, so
every kill of an exec'd process failed with "missing signal in xpc
message" (apple/container#1941). The health loop uses that kill to stop
a probe that runs past its timeout, so a hung probe was never stopped:
it stayed in the container, and ProbesInFlight failed every later probe
without running it until the hung one exited by itself. With this bump
the probe is killed at its deadline and the next probe runs.

The fix is in the client library, so it needs this rebuild, and it
works against a 1.6.0 API server as well: that server already reads
the signal as a name.

ProbesInFlight stays, since a probe can still outlive its kill by
ignoring SIGTERM or when the kill times out. Its comment no longer
cites the fixed issue.

Signed-off-by: Wes Morgan <git@wesmorgan.me>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants