Repository navigation
fix(run): run attached containers in the foreground and return their exit code - #95
Conversation
…exit code A run without -d started the container detached, printed the log once and, with --rm, force-removed it, so it was killed after a few seconds with exit 0. Attached runs now hand the runtime's own `run` the terminal (posix_spawn, so -it reads from the tty) and return its exit status. `logs -f` re-reads the log until the container has exited, since the runtime's `logs -f` returns at once on an empty log and never returns on an exited container. `--tail` is honoured.
|
hey @rasmusbe thanks for great pr, will review it quickly! |
|
@us Friendly reminder on this one. CI is green and the branch merges cleanly. Happy to adjust anything if you'd like it split up or changed. 😄 |
us
left a comment
There was a problem hiding this comment.
Thanks for this, and sorry for the wait. I ran your repro on container 1.5.0: released mocker exits 0 with 6 of 15 lines written, this branch exits 7 with all 15 and prints done. The core fix is right.
Two things I hit before merging:
-
A failed attached run leaves a ghost record.
mocker run --name ghost1 nonexistent-image:1exits 1, thenmocker ps -ashowsghost1as Exited, andmocker run --name ghost1 alpine truefails with "Conflict ... already in use". The record is saved before the spawn and only deleted on--rm. Released mocker leaves nothing here. Suggest: afterwaitpid, if the runtime does not know the container (container inspect <name>fails), delete the record regardless of--rm. -
Ctrl-C ends mocker but not the child. In tmux, Ctrl-C on
mocker run --rm ... sh -c 'while true; do echo tick; sleep 1; done'returns the shell prompt whilecontainer runkeeps running and keeps printing into the terminal; the record stays "Up" and the container is never removed. Part of this is upstream:container runitself answers Ctrl-C withfailed to send signal: missing signal in xpc message(apple/container#1876, #1941). But mocker should not exit before its child does: ignore SIGINT/SIGTERM in mocker while waiting, and reset them to default for the child withposix_spawnattr_setsigdefault+POSIX_SPAWN_SETSIGDEFso the runtime still gets them.
Smaller, optional:
followLogs: thefirstandfinishedflags can go. Setprinted = max(0, lines.count - (tail ?? lines.count))once before the loop, then loop while running plus one final read.runAttachedrepeats the name-conflict check fromrun(); a shared 3-line helper keeps them in sync.compose runwithout-d(Compose.swift:1066) has the same detached-then-print-logs shape. Fine as a follow-up if you prefer to keep this PR focused.
… run - Drop the stored record when the runtime never created the container (unknown image, bad flag), so the name can be reused. It was only dropped for --rm. - Catch SIGINT and SIGTERM while an attached run is waiting and pass them to the container with `container kill -s`, as Docker's --sig-proxy does. The runtime's own `run` does not forward them without a tty, and mocker used to exit before its child. The child gets the default signal action back so a tty still delivers Ctrl-C directly. - `compose run` without -d takes the same attached path: it streams output, honours --rm, -i and -t, and returns the exit code instead of dumping the log once. - Share the name-conflict check between run and runAttached, and simplify followLogs. A follow on a --rm container that has already removed itself ends quietly.
|
Thanks for the review, and for running the repro. I pushed 83179b7 with both fixes and the smaller points. Ghost record: after the child exits I check Ctrl-C: ignoring the signals wasn't enough on its own, since Smaller points: I can move |
A container made outside mocker under the same name was adopted by an attached run: the record stayed, and `mocker rm` then deleted the other container. The forwarded `container kill` also echoed the name into the attached output.
us
left a comment
There was a problem hiding this comment.
Thanks @rasmusbe, great work. I added one small commit on top (d5fa7ff) so this could land today:
claimNamealso refuses a name the runtime already holds. A container made outside mocker under the same name was being adopted by an attached run, andmocker rm -fthen deleted it. Now it is a plain Conflict error and nothing is recorded.- The forwarded
container killno longer prints the container name into the attached output.
Verified on container 1.5.0, rebased on main: your repro (exit 7, 15/15 lines), failed run leaves nothing, Ctrl-C with trap ... INT returns 130 and cleans up, -it returns its exit code, compose run streams and returns 4. 347 tests pass. Keeping compose run here was fine.
A container started without
-dwas killed after about 7 seconds while the client exited with 0.Cause
mocker runwithout-dstarted the container detached, printed the log once and, with--rm, force-removed it while it was still running. Without--rmthe client returned at once and left the container running. The runtime does not report an exit code for a detached container, so it was always 0.Change
container runwithout-dand returns its exit status. Output streams live and--rmis handled by the runtime. It is spawned withposix_spawnrather thanProcess, becauseProcessputs the child in its own process group and a-itsession then ignores keyboard input.logs -f,attach,start -aandcompose logs -fre-read the log every 0.5 s and stop once the container is no longer running. The runtime'slogs -freturns at once without error while the log is empty, and never returns for an exited container.logs -n/--tailnow has an effect. It was parsed and ignored.Reproduce
Run each block with the released mocker (fails) and with this branch (passes). Needs the
containerruntime.Killed mid-run, exit 0:
doneprintedexitExit code without
--rm(before: 0 each time, after: 7):-itignored keyboard input when spawned withProcess(needs a real terminal; the Pythonptymodule also works). Typeecho hej, thenexit 3:logs -fon a container that writes late (before: returns at once with no output; after: printsfirstandsecond, returns after about 6 s):Testing
Container runtime 1.5.0, arm64. All of the above, plus
swift test: 339 tests pass, including a new one for the attached argument list.Not covered:
mocker waitstill prints a hardcoded 0, since the runtime exposes no exit code for detached containers.