Skip to content

Fix background start falsely reporting success when the server fails to start - #255

Merged
k1LoW merged 1 commit into
k1LoW:mainfrom
kiakiraki:fix/background-start-verification
Jul 22, 2026
Merged

Fix background start falsely reporting success when the server fails to start#255
k1LoW merged 1 commit into
k1LoW:mainfrom
kiakiraki:fix/background-start-verification

Conversation

@kiakiraki

Copy link
Copy Markdown
Contributor

Problem

When starting a server in the background, waitForReady treats any HTTP 200 on /_/api/status as "ready" — a JSON decode failure is even short-circuited to success (return nil, nil //nolint:nilerr) — and it never verifies that the spawned child is the process actually serving the port. Combined with the probe→listen TOCTOU in the CLI, this produces false success reports in three related ways (all reproduced on Linux):

  1. Concurrent startup race loses files silently. Two mo invocations on the same port both pass the ~500ms probe and both spawn a server. The loser's child dies with EADDRINUSE, but the loser's parent receives the winner's status, prints mo: serving at ... (pid <dead pid>) with the winner's deeplinks, and exits 0. The loser's files are opened nowhere. Realistic triggers: git ls-files '*.md' | xargs -n1 mo, editor integrations, or a loaded machine where the existing server's probe response exceeds 500ms.
  2. A non-mo server occupying the port also yields exit 0. If whatever is listening returns 200 to /_/api/status, mo reports success although the child died and the port owner is unchanged. If it returns non-200, the user gets a misleading "check log file" timeout after 10s.
  3. The child's fatal error is unrecorded. Cobra writes RunE errors to stderr, which is /dev/null for background children, so cannot listen on ...: address already in use appears nowhere — the log only shows a secondary shutdown error: context deadline exceeded, caused by watchLoop never exiting on the listen-failure path (its exit depends on watcher.Close(), whose cleanup hook is registered only after a successful net.Listen).

Fix

  • waitForReady only treats a response as ready when the status decodes and carries a non-empty version (same validation as probeServer). It also takes the spawned child's PID:
    • If a mo server responds with a different PID, it returns errServerConflict with the winner's status. startBackground then falls back to POSTing the requested files/patterns/uploads to the winning server (server-side AddFile is idempotent for duplicate paths), and errors out if nothing could be added instead of claiming success.
    • If the child dies, it fails fast after a 1s grace period (giving a race winner a moment to start serving) instead of waiting out the full 10s. Child death is detected with a non-blocking wait4 on Unix, because the unwaited child remains a zombie for which kill(pid, 0) still succeeds. On Windows, os.FindProcess is used best-effort; PID reuse degrades gracefully to the old full-timeout behavior.
    • The timeout message now points at the likely cause: the port may be in use by another (non-mo) server.
  • The restore file is only removed once the child is confirmed dead — a slow-but-alive child may not have consumed it yet.
  • Fatal RunE errors are slog.Error'd before the log file closes, so the root cause lands in mo-<port>.log. This is only registered when logging to a file, avoiding duplicate stderr output in --foreground mode.
  • On listen failure, startServer closes the watcher so watchLoop exits and donegroup cleanup no longer times out after 5s.
  • The race-lost fallback gates openBrowser on isNewGroup || open, matching the existing add-to-running-server path.

Known limitations (accepted): a server self-restart during the 10s polling window would be classified as a conflict (benign — the fallback POST is idempotent and the files still end up served), and Windows PID reuse can defeat the fail-fast (falls back to the previous timeout behavior).

Behavior before / after

Concurrent race (mo -p 17700 race-a.md and mo -p 17700 race-b.md simultaneously):

# before (loser's output): false success, race-a.md lost, pid is a dead process
mo: serving at http://localhost:17700 (pid 23511)   # deeplink shows race-b.md

# after (loser's output): files added to the winner, nothing lost
mo: another mo server is already running at http://localhost:17700 (pid 49352); added 1 item(s) to it

Non-mo server on the port (python3 -m http.server variant returning 200):

# before: exit 0, "serving at ..." with a dead pid, empty log
# after: exit 1 in ~1.1s, and the log records the root cause
Error: server process exited unexpectedly; the port may be in use by another server (check log file for details) (spawned pid 48885)
# mo-17662.log: "cannot listen on localhost:17662: ... address already in use"

Tests

  • TestWaitForReady_Success / TestWaitForReady_PIDMismatch / TestWaitForReady_NonMoServer / TestWaitForReady_ChildExited — ready validation, conflict detection, non-mo rejection, and fail-fast on child death (zombie-based, deterministic).
  • TestProcessAlive_ZombieChild — regression for the zombie case: kill(pid, 0) succeeds for an unwaited dead child, so processAlive must reap.
  • TestAddToRunningServer / TestAddToRunningServer_AllPostsFail — fallback POSTs and the all-failed error path.
  • Unix-specific tests are skipped on Windows.

Verification

  • go build ./..., go test ./... -race, golangci-lint run / gostyle: clean.
  • Both failure scenarios reproduced on the parent commit and verified fixed end-to-end with the built binary (isolated XDG_STATE_HOME, ports 17xxx), including log contents and exit codes.

Reported in kiakiraki#2 (with full reproduction steps).

🤖 Generated with Claude Code

…to start

waitForReady treated any HTTP 200 on /_/api/status as ready, even when the
response was not from a mo server, and never checked whether the spawned
child was the process actually serving the port. As a result:

- When two mo processes raced for the same port, the loser received the
  winner's status, reported success with a dead pid, and its files were
  silently lost.
- When a non-mo server occupied the port, mo exited 0 while the child had
  died with EADDRINUSE.
- The child's fatal error was written only to stderr (/dev/null for
  background children), leaving no trace in the log file, and the failed
  child hung for 5s in donegroup cleanup because watchLoop never exited.

waitForReady now requires a decodable status with a non-empty version and
returns errServerConflict when the responding server's PID differs from
the spawned child; startBackground then falls back to POSTing the files
to the winning server, erroring out if none could be added. When the
child dies, waitForReady fails fast after a short grace period that lets
a race winner start serving — child death is detected with a non-blocking
reap on Unix since the unwaited child stays a zombie. The restore file is
only removed once the child is confirmed dead. Fatal RunE errors are
logged before the log file closes (only when logging to a file, to avoid
duplicate stderr output in foreground mode), and a listen failure closes
the watcher so cleanup does not time out.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@k1LoW k1LoW added the bug Something isn't working label Jul 22, 2026

@k1LoW k1LoW left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kiakiraki Thank you!!

@k1LoW
k1LoW merged commit f4a0203 into k1LoW:main Jul 22, 2026
3 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 22, 2026
kiakiraki pushed a commit to kiakiraki/mo that referenced this pull request Jul 31, 2026
…g server

`addToRunningServer` and the direct add-to-running-server path both counted
`len(patterns)` unconditionally as "added" items. `postPatterns` may fail
silently (logs warn and continues) on any HTTP error, so a pattern-only
invocation whose POSTs were all rejected by the winning server would still
be reported as "added N item(s)" and exit 0, defeating the
`attempted > 0 && added == 0` guard introduced in k1LoW#255.

`len(entries)` alone cannot substitute for a success count because a valid
pattern may legitimately match zero files. Have `postPatterns` return
`(entries, added int)` and use that count at both callsites. The direct
path also switches from `len(files)` to `len(fileEntries)` so a partial
file-POST failure no longer overstates the total either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants