Skip to content

Cancel the SIGWINCH forwarding task when a build ends#1909

Open
anxkhn wants to merge 1 commit into
apple:mainfrom
anxkhn:patch-4
Open

Cancel the SIGWINCH forwarding task when a build ends#1909
anxkhn wants to merge 1 commit into
apple:mainfrom
anxkhn:patch-4

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 7, 2026

Copy link
Copy Markdown
## Type of Change
- [x] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Documentation update

## Motivation and Context
In `Builder.build` (`Sources/ContainerBuild/Builder.swift`), when a terminal is
attached the SIGWINCH terminal-resize watcher is launched as a bare, unstructured
`Task {}` that returns `Void` and is never stored or cancelled. That causes three
lifecycle and error-handling problems:

- Dropped errors: because the task returns `Void`, every `try` inside it silently
  discards its error. A single throw from `terminal.size` (for example a momentarily
  unavailable controlling tty) tears down the whole `for await` resize loop, so
  terminal-resize forwarding to the remote build stops for the rest of the build with
  no diagnostic.
- Leaked handler and task: the fire-and-forget task keeps a process-wide
  `AsyncSignalHandler` registered for SIGWINCH and stays parked in `for await` after
  `build` returns (via either the normal completion or the `catch Error.buildComplete`
  path), so in a long-lived invocation both outlive the build.
- Use-after-finish: `build` finishes the request stream in a `defer`, but the
  still-live task can `continuation.yield` to the finished stream on the next resize.
  That is a silent no-op today, but the resize is lost and the ownership is wrong.

This is the build path (`Builder.build`), which is distinct from the in-flight
`container run` process-signal changes (#1854, #1871, #1778); no open PR touches this
resize-forwarding code.

The fix extracts the forwarding loop into a small `forwardTerminalResize` helper that
logs read/send errors instead of dropping them, mirroring the existing pattern in
`ProcessIO.handleProcess`, which already logs "failed to send terminal resize event"
rather than swallowing it. So a transient failure no longer permanently stops
forwarding. `build` now stores the watcher task and its signal handler and tears both
down alongside the continuation finish in the same `defer`, so the watcher never
outlives the build. Initial-size behavior is unchanged: the first update always sends
the current size, then re-sends only when the size changes.

## Testing
- [x] Tested locally
- [x] Added/updated tests
- [ ] Added/updated docs

Added `Tests/ContainerBuildTests/BuilderResizeForwardingTests.swift`:
- `readSizeFailureIsLoggedAndForwardingContinues`: a transient `readSize` failure is
  logged and forwarding continues, so a later changed size is still forwarded (two
  sends across the error, and the error is logged). This fails on the old
  drop-and-teardown behavior and passes with the fix.
- `forwarderReturnsWhenSignalStreamFinishes`: the helper returns once the signal
  stream finishes, which is what task cancellation triggers via the handler teardown,
  so the watcher does not outlive the build.

Verified with the Xcode toolchain:
- `swift build --target ContainerBuild` - build complete.
- `swift test --filter ContainerBuildTests` - 42 tests across 4 suites pass,
  including both new tests.
- `swift format lint --strict` on the two changed files - clean.
- License header on the new file applied via `make update-licenses`.

Files changed (2, both intended; no unrelated edits)

  • Sources/ContainerBuild/Builder.swift (+70/-29): extract forwardTerminalResize
    (log-not-drop errors); build creates the AsyncSignalHandler eagerly only when a
    terminal is set, stores the watcher Task, and cancels handler+task in the existing
    defer next to continuation.finish().
  • Tests/ContainerBuildTests/BuilderResizeForwardingTests.swift (new, 131 lines):
    the two regression tests + a minimal collecting LogHandler.

Where to open the PR

  • Base: apple/container main. Head: fork anxkhn/container branch patch-4.
  • Fork/push/PR happen ONLY at the gated ship step. The upstream commit must be
    cryptographically signed (GPG/SSH) per apple/containerization CONTRIBUTING (this is
    signing, NOT DCO -s); the local implement commit is unsigned, so signing is applied
    when the commit is created/amended at ship.

In Builder.build, the SIGWINCH watcher was launched as a bare, unstructured
Task that returned Void and was never stored or cancelled. This caused three
problems, all in the task's lifecycle and error handling:

- Dropped errors: because the task returned Void, every `try` inside it
  silently discarded its error. A single throw from `terminal.size` tore down
  the whole resize loop, so terminal-resize forwarding stopped for the rest of
  the build with no diagnostic.
- Leaked handler and task: the fire-and-forget task kept a process-wide
  AsyncSignalHandler registered and stayed parked in `for await` after build
  returned, so in a long-lived invocation both outlived the build.
- Use-after-finish: build finishes the request stream in a `defer`, but the
  still-live task could yield to the finished continuation on the next resize.

Extract the forwarding loop into forwardTerminalResize, which logs read/send
errors instead of dropping them (mirroring ProcessIO.handleProcess) so a
transient failure no longer stops forwarding. Store the task and its signal
handler, and tear both down alongside the continuation finish so the watcher
does not outlive the build. Add unit tests that cover the recovery-after-error
and stream-finish paths.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.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

Development

Successfully merging this pull request may close these issues.

1 participant