fix(cli): stop killing the background listen child with --pid-file - #508
Merged
Merged
Conversation
`relayfile listen --background` never worked. The parent appended
`--pid-file <path>` to the child's argv, but `runListen` never registered
that flag, so the child exited at flag parsing:
$ relayfile listen --background
Listen started in background. Logs: ~/.relayfile/listen.log
$ cat ~/.relayfile/listen.log
error: flag provided but not defined: -pid-file
The parent had already printed success and released the process, so the
failure was invisible unless the user opened the log.
`runListen` parses with flag.ContinueOnError and discards the flagset's
output, which is why nothing surfaced on a terminal.
Nothing ever read the listen pid file — there is no `listen stop` or
`listen status`, and `listenPIDFile` had no other caller — so the flag is
dropped rather than registered, and the dead helper goes with it.
Child argv construction moves into `backgroundListenChildArgs` so the
invariant is testable: the new test asserts the argv `--background`
actually re-invokes parses under the real flagset, instead of pinning a
literal argv that would drift. Reintroducing `--pid-file` fails it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Session-Id: d458bd97-53d8-4f02-be9c-48b67b93c916
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
relayfile listen --backgroundhas never worked. It reports success and starts nothing.Why
spawnBackgroundListenProcessappended--daemonized --pid-file <path>to the child argv, butrunListen's flagset registers onlyserver,token,provider,path,event,run,format,background,daemonized. Nopid-file.It parses with
flag.ContinueOnErrorandfs.SetOutput(io.Discard), so the unknown flag produced no diagnostic anywhere a user looks — and the parent had already printed "Listen started in background" and calledProcess.Release().--pid-filewas also pointless: nothing reads the listen pid file. There is nolisten stoporlisten status, andlistenPIDFile()had exactly one caller — the spawn site that passed it to the child. So the fix drops the flag rather than registering it, and removes the dead helper.(
mount --backgroundis unaffected: its flagset does register--pid-file, andwaitForBackgroundMountRegistrationgenuinely reads that file.)Change
--pid-fileto the listen child; deletelistenPIDFile.backgroundListenChildArgsso the child argv is testable in isolation.Verification
Built and ran against a scratch
HOME:listen --daemonized --pid-file /tmp/x→error: flag provided but not defined: -pid-file;listen --backgroundprinted success and wrote that error to the logHOME, which is correctNew test
TestBackgroundListenChildArgsOnlyUseRegisteredFlagsruns the produced argv through the realrunListenflagset rather than pinning a literal argv, so a future flag added to the child is checked too. Mutation-tested — reintroducing--pid-filefails it:go vetclean,gofmtclean, and theBackground|Listen|Stopsuites pass.🤖 Generated with Claude Code
Note
Low Risk
Narrow CLI spawn/argv change for listen background mode; mount background PID handling is unchanged.
Overview
Fixes
relayfile listen --background, which previously printed success while the child exited immediately on an unregistered--pid-fileflag (with no user-visible error becauserunListendiscards flag parse output).The spawn path no longer passes
--pid-fileor useslistenPIDFile()(nothing consumed that file). Child argv is built viabackgroundListenChildArgs: strip--background, append--daemonizedonly.Adds
TestBackgroundListenChildArgsOnlyUseRegisteredFlags, which runs the produced args through realrunListenflag parsing so future unknown flags regress visibly.Reviewed by Cursor Bugbot for commit 6f09fc9. Bugbot is set up for automated code reviews on this repo. Configure here.