Skip to content

fix(cli): a refused UI configuration is not a failed daemon start - #1796

Open
liuchong wants to merge 1 commit into
DeusData:mainfrom
liuchong:fix/daemon-start-survives-ui-config
Open

fix(cli): a refused UI configuration is not a failed daemon start#1796
liuchong wants to merge 1 commit into
DeusData:mainfrom
liuchong:fix/daemon-start-survives-ui-config

Conversation

@liuchong

Copy link
Copy Markdown

The bug

daemon start reports failure for a daemon that started and is serving.

test / test-windows-guards goes red with:

RED: `daemon start` after the crash must launch a FRESH daemon:
error: the daemon did not accept the UI configuration; browser was not opened
RED (tests/windows/test_daemon_stability.py): section_crash_recovery failed

That message has exactly one source, in main_run_daemon_ctl. Reaching it means the daemon is already up — the comment directly above it says so, and the handshake is sent over the very control connection that satisfied the daemon's no-client startup window. Only the UI configuration came back short.

Root cause

The handshake is two requests, each bounded by MAIN_CONNECT_TIMEOUT_MS (one second), and missing that window was treated as a failed start.

One second is thin on a loaded machine, and section_crash_recovery manufactures exactly that state: it hard-kills a daemon and immediately starts a fresh one, on a runner still reclaiming the dead process's resources. This is the same environment #1772 addressed, but a different bounded wait — #1772 fixed the startup-transition lock, and this path sits downstream of it. The failure reproduces on a branch that already carries #1772.

The change

Whether a refused UI configuration is fatal now depends on what was asked for.

--port and --open make the UI the point of the command, so they keep the existing nonzero exit. A bare daemon start asks for a daemon and got one, so it warns and succeeds. It also skips the UI-warming notice, which would otherwise announce a port that nothing is serving.

Verification

A seam (CBM_TEST_DAEMON_UI_CONFIG_REFUSED, compiled out without TEST_SEAMS=1) forces the refusal, because reproducing it for real needs a machine loaded enough to miss a bounded handshake — not a state a test can ask for.

tests/windows/test_daemon_lifecycle.py covers both halves of the contract and reports a precondition skip against a non-UI binary. Checked locally against a scripts/build.sh --with-ui TEST_SEAMS=1 build:

  • with the fix: green, both new assertions pass
  • with the seam kept but the decision reverted to its previous unconditional failure: red on a refused UI configuration must not fail daemon start
  • against a non-UI binary: the new section skips, the existing ones stay green

lint-format and lint-cppcheck are clean.

The underlying timing only occurs on Windows CI, so the guard's behaviour there is what this PR is asking to confirm.

`daemon start` exited nonzero when the daemon came up but did not answer
the UI configuration handshake. By that point the daemon is running --
the control connection above it already satisfied the startup window --
so the command reported failure for something it had already achieved.

The two requests in that handshake are bounded at MAIN_CONNECT_TIMEOUT_MS,
one second. On a loaded machine, especially right after an abrupt
shutdown, that is thin. Windows CI hit it repeatedly: the crash-recovery
section of `tests/windows/test_daemon_stability.py` hard-kills a daemon
and immediately starts a fresh one, and the report that came back was
that no fresh daemon had started -- when one had, and was serving.

Whether the refusal is fatal now depends on what was asked for. `--port`
and `--open` make the UI the point of the command and keep the existing
nonzero exit. A bare `daemon start` asks for a daemon and got one, so it
now warns and succeeds; it also skips the UI-warming notice, which would
otherwise announce a port that nothing is serving.

A seam forces the refusal, because reproducing it for real needs a
machine loaded enough to miss a bounded handshake -- not a state a test
can ask for. The lifecycle guard covers both halves of the contract, and
with the decision reverted it reports the bare-start case as red.

Signed-off-by: 刘冲 <mail@liuchong.dev>
@liuchong
liuchong requested a review from DeusData as a code owner August 22, 2026 08:21
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

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.

1 participant