Skip to content

Keep playground process after detachment#414

Merged
canercidam merged 1 commit intomainfrom
caner/keep-playground-process-after-detachment
Apr 21, 2026
Merged

Keep playground process after detachment#414
canercidam merged 1 commit intomainfrom
caner/keep-playground-process-after-detachment

Conversation

@canercidam
Copy link
Copy Markdown
Member

This makes playground stop/clean all resources when the stop/clean commands are used and avoid orphan processes.

Fixes #377

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Claude finished @canercidam's task in 6m 4s —— View job


Review

Focused on the new detached-fork mechanism. Inline comments on the specific spots; summary of the notable issues below.

Bugs / correctness

  • utils/detached.go:119-125cmd.Wait() after a waitForReady error can block indefinitely if the child closed its notify fd but is still running. Tests don't exercise this because the helpers os.Exit right after closing. Real misbehaving children could deadlock the parent. Send SIGKILL (or use a timeout) before Wait.
  • utils/detached.go:119 — No timeout on the parent's wait for readiness. The user's --timeout flag only applies to the child after startup, so a stuck startup hangs the parent forever.
  • main.go:927-931detachedNotifyFd > 0 gates the child-side notify logic, but a user who passes --detached-notify-fd=N on the command line (hidden but not forbidden) trips it too and hits a cryptic invalid notify fd error. Consider gating on something the library controls (env var, or guard the flag so it only works in child-mode).

Minor

  • utils/detached.go:89-108readyW is only closed explicitly; fine today but a future edit that adds an early return between pipe creation and cmd.Start would leak it. A deferred close guarded by a "consumed" flag would be more robust.
  • utils/detached.go:224-246 (forwardSignalsTo) — narrow race: after cmd.Process.Release() the handle's PID is zeroed on unix, so a forwarded signal delivered between Release and the deferred stopForward() is silently dropped. Stopping signal forwarding before Release closes the window.
  • The PR description mentions the stop/clean commands but doesn't change them. The actual mechanism is: the child now properly runs the watchdog/select loop, so when stop/clean tears down containers, ExitErr fires and the child calls dockerRunner.Stop(keepFlag) on its way out. Worth calling out explicitly in the description — the fix is indirect, and nothing registers the detached child's PID with KillSessionPIDs, so cleanup relies on container teardown propagating back.
    Branch

Comment thread utils/detached.go
Comment thread utils/detached.go
Comment thread utils/detached.go
Comment thread main.go
Comment thread utils/detached.go
@canercidam canercidam merged commit fbb735a into main Apr 21, 2026
25 checks passed
@canercidam canercidam deleted the caner/keep-playground-process-after-detachment branch April 21, 2026 16:07
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.

Stop command only stops docker services

1 participant