Skip to content

Fix outbound socket relays for container mounts - #841

Open
EhsanAzish80 wants to merge 3 commits into
apple:mainfrom
EhsanAzish80:fix/2101-publish-socket-tmpfs
Open

Fix outbound socket relays for container mounts#841
EhsanAzish80 wants to merge 3 commits into
apple:mainfrom
EhsanAzish80:fix/2101-publish-socket-tmpfs

Conversation

@EhsanAzish80

Copy link
Copy Markdown

Summary

  • start outbound Unix socket relays after the container process creates its mount namespace
  • resolve guest socket paths through /proc/<pid>/root so tmpfs and bind-mounted targets are visible
  • preserve pre-start staging for inbound socket relays

Fixesapple/container#2101.

Testing

  • swift test --filter LinuxContainerTests (6 tests passed)
  • swift format lint --strict --configuration .swift-format-nolint Sources/Containerization/LinuxContainer.swift Tests/ContainerizationTests/LinuxContainerTests.swift

I could not complete an end-to-end runtime check because the current apple/container CLI built with Xcode 27 routed commands to root help before system startup. The focused namespace-path regression and LinuxContainer suite pass.

@sturdy4days

Copy link
Copy Markdown

I reviewed this against the lifecycle and relay-manager paths while tracking apple/container issue 2101. The post-start proc-PID-root direction looks like the right fix for OCI tmpfs visibility, but two issues need addressing before it is safe to merge.

  1. Rollback on outbound setup failure. relayManager.start creates the host listener before relayAgent.relaySocket. If that RPC, or a later outbound relay, fails, the start catch closes the agent and stops the VM but does not stop the already-created relays. The state becomes errored, and the accepting listener/socket can remain orphaned. Please roll back relayManager before VM teardown, with coverage for first-relay and Nth-relay RPC failure.

  2. Process lifetime and PID reuse. vminitd retains the proc PID root path string and resolves it on each host connection. Once the init process exits, that proc entry can disappear or be reused before cleanup, so a later connection can fail or resolve in an unrelated namespace. The project issue 829 discussion already flags numeric PID re-resolution and procfs magic links as unsafe. This needs a pinned process/namespace identity or an atomic rule that the relay dies with the workload; the current path-only test cannot establish it.

Also, LinuxPod still starts outbound relays pre-process through the static rootfs path, so its parallel API retains the tmpfs failure. Please either include parity or make the scope explicit and track it separately.

The existing test verifies only path construction. An end-to-end dev-shm and explicit tmpfs relay test, plus rollback coverage, would make this much easier to validate.

@EhsanAzish80

Copy link
Copy Markdown
Author

Thanks — I addressed the review points. Outbound relays now start after the process in both LinuxContainer and LinuxPod, vminitd pins the container root descriptor for the proxy lifetime, and partial relay setup is rolled back. I also added VM integration coverage for a /dev/shm PING round trip and rollback when a later relay fails. Both integration tests and the full unit suite pass locally.

@sturdy4days

Copy link
Copy Markdown

Thanks for the follow-up. I reviewed f6d804d against the relay lifecycle paths. The update resolves the repeated PID-root lookup concern by pinning a root descriptor and adds useful LinuxContainer integration coverage, but I found two LinuxPod lifecycle gaps that still need correction before this is safe to merge.

  1. In LinuxPod.startContainer, the process is started before outbound relay setup, but it is added to pod state only after that setup. If an outbound relay fails, the catch removes relays and closes the agent but does not kill, wait for, or delete the newly started process. The API can return failure while an untracked workload continues in the pod VM. Please make this rollback transactional and add first- and Nth-relay failure coverage for LinuxPod.

  2. LinuxPod.stopContainer kills/deletes the process and releases its rootfs but does not stop that container relay or its guest proxy. The pinned root descriptor and host listener can therefore survive until whole-pod shutdown. Please give relays explicit per-container ownership, stop both sides before rootfs release, and prove idempotent stop/removal without retained listener or root FD.

The new LinuxContainer dev-shm PING and Nth-relay tests are a solid step, but they do not exercise these LinuxPod paths or guest-RPC failure. I would keep adoption blocked pending those fixes and runtime proof.

@sturdy4days

Copy link
Copy Markdown

Following up on the LinuxPod lifecycle gaps I reported, I prepared a non-competing signed branch rooted directly at this PR exact head f6d804d:

https://github.com/sturdy4days/containerization/tree/codex/relay-lifecycle-hardening-20260827

The two commits are:

  • 17d4825 — transactional relay/process teardown and per-container relay ownership
  • 52bfe6a — hotplug rollback for Nth-relay and post-relay agent-close failures

The correction records every attempted relay before setup, uses a fresh agent once close has begun, aggregates cleanup failures, and stops the VM/marks the pod errored before resource release if cleanup certainty is lost. Focused relay tests pass 8/8, the full host suite passes 601/601 across 82 suites, relevant builds and formatting pass, and the earlier pinned-root /dev/shm PING proof remains intact. Both commits are SSH-signed and GitHub reports valid signatures. An independent final source review found no confirmed lifecycle defect; remaining fault-injection gaps are limited to cleanup-error-content and secondary cleanup-failure assertions.

I have not opened a competing PR. Please feel free to inspect or cherry-pick the commits; I can adapt them narrowly to your preferred structure.

@EhsanAzish80

EhsanAzish80 commented Aug 28, 2026

Copy link
Copy Markdown
Author

Thanks — fixed both lifecycle gaps. Socket relays are now owned per container. Failed setup rolls back the host and guest relays, terminates and deletes the process, and marks the container errored. stopContainer now removes both sides before releasing the root filesystem and remains safe when called repeatedly.

Added VM coverage for first and later relay failures, guest RPC failure, /dev/shm round-trip, relay removal, and repeated stop. The focused socket suite passes 5/5 locally, and the full unit suite passes.

@sturdy4days

Copy link
Copy Markdown

Thanks for the update. I reviewed the signed ddf544f head. The nominal LinuxPod rollback, per-container relay ownership, and repeated-stop path are improved, but two failure-atomicity gaps remain before this is safe for hostile-workload adoption.

  1. Cleanup uncertainty can still leave effects untracked while the shared VM continues. Start records a relay only after setup succeeds, so the current guest RPC failure is not compensatingly removed; stop removes manager ownership before relay.stop succeeds, clears process state, and releases the rootfs even when relay/process cleanup failed. Please retain authoritative ownership until cleanup succeeds, compensate both halves of every attempted relay, and stop/poison the pod VM before releasing resources whenever cleanup certainty is lost.

  2. addContainer hotplug rollback remains non-transactional. Earlier inbound relays are not unwound on an Nth failure, cleanup reuses the same agent after close begins, cleanup errors are discarded, and resources are released without a fail-closed VM transition. Please track every attempted relay, use a fresh agent after close begins, aggregate cleanup failures, and stop the VM before unsafe release.

The new tests cover useful happy paths, host-path failure, and pre-proxy validation rejection, but not post-side-effect guest RPC loss or failures in host/guest stop, kill, wait, delete, agent close, or VM stop. Fault-injection coverage for those paths, plus assertions that the process, guest proxy, pinned root FD, and host listener are absent, would close the remaining gate.

The non-competing correction branch at 52bfe6a demonstrates these fail-closed transitions and remains available for inspection or selective adoption.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: publish-socket cannot relay a socket on tmpfs with read-only root

2 participants

@EhsanAzish80@sturdy4days