[PR #129] [MERGED] feat(exec): honor DetachKeys — detach the stream without killing the process #128

New issue

Closed

opened 2026-10-15 12:52:13 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 12:52:13 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/games-on-whales/LXC2Docker/pull/129
Author: @JBailes
Created: 7/1/2026
Status: ✅ Merged
Merged: 7/1/2026
Merged by: @JBailes

Base: main ← Head: fix/exec-detach-keys

📝 Commits (2)

bc25831 feat(exec): honor DetachKeys — detach the stream without killing the process
d30cfee fix(exec): close conn on runner error paths (closeConn is now false)

📊 Changes

4 files changed (+606 additions, -66 deletions)

View changed files

📝 internal/api/exec.go (+161 -66)
➕ internal/api/exec_detach.go (+154 -0)
➕ internal/api/exec_detach_runner_test.go (+121 -0)
➕ internal/api/exec_detach_test.go (+170 -0)

📄 Description

Design-approved deferred item (the most complex). POST /exec/{id}/start stored + echoed DetachKeys but never acted on it — a client couldn't detach an exec (ctrl-p ctrl-q) and had to drop the connection.

What it does

parseDetachKeys: DetachKeys string → byte sequence (ctrl-a..z, ctrl-@, ctrl-[/\/]/^/_, single chars). Default ctrl-p,ctrl-q. Malformed keys fall back to the default (Docker is lenient; the client validated at create).
escapeReader: wraps the client stdin; forwards non-matching bytes, withholds a partial match across reads, swallows the full sequence and returns errDetach so io.Copy stops.
Detach keeps the process running (the hard part): output is redirected through a switchableWriter (→ io.Discard) that swallows write errors so the single drain goroutine keeps the pty/pipes drained (the process never blocks on a full buffer); the client connection is closed; cmd.Wait runs in the background and the exec record is finished via onExit only when the process actually exits (Running stays true until then).
TTY (runExecTTY) keeps the pty master open (closing it would SIGHUP the shell). Non-TTY (runExecMux) closes stdinW on detach so os/exec's stdin-copy goroutine ends and cmd.Wait can complete — the client's input stream is gone (Docker's non-TTY detach behaves the same). (This was a real bug I caught via a -race runner test: leaving stdinW open hangs cmd.Wait.)

Wolf safety

escapeReader is a pure pass-through until the exact escape bytes match, so only streams that actually contain the sequence are affected — every other byte stream is byte-for-byte unchanged. Wolf's fake-udev exec sends no such bytes, so it's unaffected. The 101/200 preamble and the #98 fix are untouched.

Tests

exec_detach_test.go — parseDetachKeys (all mappings + errors); escapeReader (passthrough, one-read match, split-across-reads, broken partial, double-start, empty keys); switchableWriter (swallow + swap).
exec_detach_runner_test.go — the teardown proof: detach returns true, the process is still alive afterward, and onExit fires only once the process exits; plus the normal-exit path. Race-clean under -race.

go test ./..., go vet, gofmt, go build clean. Merge only on verified-green CI.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/games-on-whales/LXC2Docker/pull/129 **Author:** [@JBailes](https://github.com/JBailes) **Created:** 7/1/2026 **Status:** ✅ Merged **Merged:** 7/1/2026 **Merged by:** [@JBailes](https://github.com/JBailes) **Base:** `main` ← **Head:** `fix/exec-detach-keys` --- ### 📝 Commits (2) - [`bc25831`](https://github.com/games-on-whales/LXC2Docker/commit/bc258317c084751fbb21cae1e4f4ec860bf6b3e9) feat(exec): honor DetachKeys — detach the stream without killing the process - [`d30cfee`](https://github.com/games-on-whales/LXC2Docker/commit/d30cfeee471a24c3446401456f5e04439145e94e) fix(exec): close conn on runner error paths (closeConn is now false) ### 📊 Changes **4 files changed** (+606 additions, -66 deletions) <details> <summary>View changed files</summary> 📝 `internal/api/exec.go` (+161 -66) ➕ `internal/api/exec_detach.go` (+154 -0) ➕ `internal/api/exec_detach_runner_test.go` (+121 -0) ➕ `internal/api/exec_detach_test.go` (+170 -0) </details> ### 📄 Description Design-approved deferred item (the most complex). `POST /exec/{id}/start` stored + echoed `DetachKeys` but never acted on it — a client couldn't detach an exec (ctrl-p ctrl-q) and had to drop the connection. ## What it does - **`parseDetachKeys`**: `DetachKeys` string → byte sequence (`ctrl-a`..`z`, `ctrl-@`, `ctrl-[`/`\`/`]`/`^`/`_`, single chars). Default `ctrl-p,ctrl-q`. Malformed keys fall back to the default (Docker is lenient; the client validated at create). - **`escapeReader`**: wraps the client stdin; forwards non-matching bytes, **withholds a partial match across reads**, swallows the full sequence and returns `errDetach` so `io.Copy` stops. - **Detach keeps the process running** (the hard part): output is redirected through a `switchableWriter` (→ `io.Discard`) that **swallows write errors** so the single drain goroutine keeps the pty/pipes drained (the process never blocks on a full buffer); the client connection is closed; `cmd.Wait` runs in the background and the exec record is finished via `onExit` **only when the process actually exits** (`Running` stays true until then). - **TTY** (`runExecTTY`) keeps the pty master open (closing it would SIGHUP the shell). **Non-TTY** (`runExecMux`) closes `stdinW` on detach so `os/exec`'s stdin-copy goroutine ends and `cmd.Wait` can complete — the client's input stream is gone (Docker's non-TTY detach behaves the same). *(This was a real bug I caught via a `-race` runner test: leaving `stdinW` open hangs `cmd.Wait`.)* ## Wolf safety `escapeReader` is a pure pass-through until the exact escape bytes match, so **only streams that actually contain the sequence are affected** — every other byte stream is byte-for-byte unchanged. Wolf's fake-udev exec sends no such bytes, so it's unaffected. The 101/200 preamble and the #98 fix are untouched. ## Tests `exec_detach_test.go` — `parseDetachKeys` (all mappings + errors); `escapeReader` (passthrough, one-read match, **split-across-reads**, broken partial, double-start, empty keys); `switchableWriter` (swallow + swap). `exec_detach_runner_test.go` — **the teardown proof**: detach returns `true`, the process is still **alive** afterward, and `onExit` fires only once the process exits; plus the normal-exit path. **Race-clean under `-race`.** `go test ./...`, `go vet`, `gofmt`, `go build` clean. Merge only on verified-green CI. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-15 12:52:13 +00:00