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

[liuchong]

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.

[github-actions]

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.

[DeusData]

Thank you for isolating this downstream startup result and for using a deterministic test seam instead of a timing-dependent test. I checked current main: after the daemon control connection succeeds, main_run_daemon_ctl() still returns an error whenever the UI configuration is not accepted, without distinguishing a bare daemon start from an explicit --port or --open request. That grounds the behavior this PR changes.

I have labeled this as a high-priority stability bug and routed it for review. The current CI surface is green. Our review queue is full, so detailed review may take a little time. Thank you for keeping the proposed behavior split at the command contract rather than simply extending the timeout.

[liuchong]

Understood, and nothing needed from me here.

One thing worth stating so you do not have to ask: when this merges I will rebase #1723–#1728 onto it right away. That stack is where the defect surfaced — test-windows-guards was intermittently red there for exactly this reason — so the rebase is what makes those six reliably green rather than green-on-retry. All six are clean and currently zero commits behind main, so it is a base move rather than a reshaping.

Take the review time you need.

[DeusData]

Approved on merit. The reasoning is right, and I verified each step against main.

Verified

• MAIN_CONNECT_TIMEOUT_MS = 1000 (src/main.c:45) — one second, as you say.
• main.c:2401-2408 guards the message with !context_set || cbm_daemon_application_client_set_ui_config(...) != OK, so it really is two separately-bounded requests, each at that one second.
• Reaching it requires start_result.client to be valid, which means the daemon accepted a control connection. The daemon is up.

That last point is what makes this a correctness fix rather than a flake workaround, and it is worth stating in exactly those terms: the message conflates two different facts. daemon start reports "the daemon did not start" for a daemon that started and is serving; only its UI configuration came back short. A one-second bound on a loaded machine is the trigger, but the bug is the conflation, and it would be a bug at any timeout.

And you did not fix it by widening the window, which is the tempting and wrong move here — a budget bump would have made the failure rarer without making the report true. Deciding fatality by what the user actually asked for is the correct axis: --port/--open make the UI the point of the command and keep the nonzero exit, while a bare daemon start asked for a daemon and got one. Skipping the UI-warming notice on that path is the detail that keeps it honest — announcing a port nothing is serving would replace one wrong signal with another.

The test seam is fine: CBM_ENABLE_TEST_SEAMS already gates six blocks in main.c alone, and forcing the refusal deterministically beats trying to reproduce a load-dependent race.

One scope correction, in your favour and mine

I came to this PR believing it might clear the test-windows-guards red that is currently reddening other contributors' PRs. Checking rather than assuming: it does not, and it cannot.

The red I have been attributing on other PRs is test_daemon_stability.py :: section_start_status_port, and that section runs:

run_cli(binary, cache, ["daemon", "start", "--port=%d" % busy_port], timeout=60)

--port is precisely the case your change deliberately leaves fatal. So you fix section_crash_recovery, exactly as you claim, and section_start_status_port is a separate defect that this correctly does not touch. Your description is accurate; my hypothesis was the loose one. Worth recording so nobody later expects this PR to have cleared both.

Before merge

You are 205 commits behind main, so the green here is stale. Normally I would rebase for you, but our Actions pool has been saturated all day and every rebase adds a full matrix to a 24-deep queue, so I am holding off deliberately. I will refresh it once the queue drains.

Note this does change a CLI exit code for a real case — a bare daemon start that previously exited nonzero now warns and succeeds. That is the intended correction, and I am satisfied the --port/--open carve-out keeps the contract intact for anyone scripting against the UI.

Thank you — particularly for tying it to #1772 and stating that this path sits downstream of it, which saved me from re-deriving that relationship.