Skip to content

fix(daemon): preserve disabled UI on manual start - #2205

Open
vincentkoc wants to merge 10 commits into
DeusData:mainfrom
vincentkoc:fix/daemon-preserve-ui-setting-20260914
Open

vincentkoc wants to merge 10 commits into
DeusData:mainfrom
vincentkoc:fix/daemon-preserve-ui-setting-20260914

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 14, 2026 •

Copy link
Copy Markdown

What does this PR do?

A cold daemon start now preserves a saved ui_enabled=false setting. Previously it unconditionally requested UI configuration, rewrote the preference to true, and started the HTTP listener.

An explicit cold --port or --open request keeps its existing UI-enabling behavior. Repeated bare starts report the disabled UI without incorrectly saying a browser request failed, and the configuration hint points to config set ui_enabled true. The configuration docs describe these semantics.

Fixes #2114

Validation

  • The new disabled-UI regression failed against the unchanged base and passed with this fix. All seven real-process readiness cases passed, covering cold and warm starts, saved enabled/disabled settings, explicit UI requests, delayed readiness, and foreign-listener rejection.

  • scripts/test.sh BUILD_DIR=build/full-tests passed on macOS arm64: 8,022 tests passed, 10 platform skips, plus all prescribed process guards and security/ordering checks.

  • git diff --check, formatting, cppcheck, and the NOLINT suppression check passed.

  • Full lint is not green: scripts/lint.sh exited 2 with 6,972 repo-wide clang-tidy diagnostics. An exact-path VFS comparison of unchanged and candidate src/main.c produced 125 diagnostics each with the same diagnostic categories; none point to changed lines. The existing main_run_daemon_ctl cognitive-complexity diagnostic increases from 82 to 89, already above the threshold of 25. Tools: Apple clang-format 21, clang-tidy 22.1.8, cppcheck 2.21.0.

  • Hosted CI at e0a6133293556d87de6c2ca43de674e3a643e339 is not green: Windows guards failed section_cold_storm when client 0 reported secure CLI coordination could not be created (endpoint). The same failure was observed on sibling head c1dd44151fee8cfd815c693392a7c1c72def98fd, which shares base 339b3f4097aa6ede22fc382ab7fd320d93c498b8. The guard runs six concurrent cli list_projects calls; the error occurs in local CLI endpoint creation, before the daemon-control UI path changed here. This comparison does not replace an exact-head Windows pass. A maintainer needs to rerun/investigate that guard; no assertion has been weakened.

Checklist

  • Every commit is signed off (git commit -s).
  • Tests pass locally.
  • Full lint passes (the existing clang-tidy debt is described above).
  • New behavior is covered by a test, reproduced before the fix.

AI-assisted implementation and follow-up; submitted on behalf of @vincentkoc.

Signed-off-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com>
@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.

@vincentkoc
vincentkoc marked this pull request as ready for review September 14, 2026 09:07
@DeusData DeusData added bug Something isn't working ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thank you for the cold-start configuration fix. Current startup has separate loaded-configuration and explicit CLI override paths; review should prove that configured values survive a cold start while explicit flags retain precedence. Please keep the cold/warm and explicit-override cases as the acceptance boundary. This is queued for focused review.

@DeusData

Copy link
Copy Markdown
Owner

@vincentkoc, thank you for this, and for the unusually honest PR description. Calling out the clang-tidy baseline and the Windows guard failure yourself saved us real time.

We've settled the contract question this raised: a bare daemon start honours the saved ui_enabled value, and only an explicit --port/--open turns the UI back on and saves that choice. That is exactly what your change does. It brings the manual and auto-spawn start paths into agreement, and it fixes #2114 as @0x4nur4g described it. The corrected hint pointing to config set ui_enabled true is a nice catch too.

Two small things before we merge:

  • Could you rebase onto current main? It merges cleanly today but is about 180 commits behind, so CI should run on the exact head.
  • We'll look at the section_cold_storm Windows guard on our side. Your comparison with the sibling head points to something that already existed, and you were right not to weaken the assertion. We'll also add a release-notes line saying a bare daemon start no longer re-enables a disabled UI.

Thanks again. This is a careful, well-tested fix.

@DeusData

Copy link
Copy Markdown
Owner

Thanks again for the disabled-UI fix and the clear validation limits. The earlier rebase request remains recorded; attribution of the Windows guard and the release-note follow-up are maintainer-side tasks. This sweep has not rerun that guard or changed the agreed startup semantics.

@DeusData

Copy link
Copy Markdown
Owner

Thanks again for the disabled-UI fix. The branch refresh remains the recorded contributor step; Windows-guard attribution and the release-note follow-up remain ours. Sorry for the delay on our part—I’ll follow up soon.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

Copy link
Copy Markdown
Owner

@vincentkoc, thank you again for this fix and for the unusually careful validation notes, and sorry for how long it has waited. Your change does exactly what we settled on: a bare daemon start honours the saved ui_enabled value, and only an explicit --port or --open turns the UI back on and saves that choice. Since the branch allows maintainer edits, we are doing the remaining steps ourselves instead of asking you for the rebase. The first is done: we merged current main into the branch (one signed-off merge commit, 95fc0d3; your commit is untouched). About the Windows guard: you were right, the section_cold_storm failure came from a race when several first-run processes create the runtime directory at the same moment; it was fixed on main in #2275 after your branch was cut, and the branch now includes that fix. Next we will add a small unit test for the start rule (the readiness script is a manual guard that CI does not run) and explain it here. If you push to this branch again, please pull first so our commits are kept.

The cold daemon start rule this PR introduces (a bare start honours a saved
ui_enabled=false; only an explicit --port=N or --open turns the UI on and
persists that choice) was covered only by tests/test_daemon_open_readiness.py,
a real-process guard that no CI leg runs.

Move the rule into a pure helper, cbm_ui_config_daemon_start_ui() in
src/ui/config.c, and call it from the cold start path in main.c; the
CBM_EMBEDDED_FILE_COUNT gate stays in main.c and the behaviour is unchanged.
Add config_daemon_start_honours_saved_ui_choice to the ui suite, which CI
runs. It fails if the old always-configure rule comes back (a bare start with
a saved disabled UI) and if --port=N or --open stop re-enabling a disabled UI.

Refs DeusData#2114

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
config set now stores ui_enabled and ui_port in the shared settings database
(_config.db), so the readiness guard read of config.json raised
FileNotFoundError before its first assertion. Read the pair back with
config get ui_enabled and config get ui_port instead; they load the same
value every start path uses. The guard compares it after config set, after
the first start, and, for a disabled UI, after the repeated start, status and
start --open commands. Drop the now unused json import.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Nov 1, 2026

Copy link
Copy Markdown
Owner

@vincentkoc, thank you again for this fix. The rule you implemented is exactly the one we settled on: a bare daemon start honours the saved ui_enabled value, and only an explicit --port or --open turns the UI back on and saves that choice. Your real-process cases (cold and warm starts, saved enabled and disabled settings, explicit UI requests) were the right acceptance boundary, and running them is how we found the two small things below. Since the branch allows maintainer edits, we added three commits on top of yours; your commit is untouched.

  1. 65dd99d refactor(daemon): extract the cold start UI rule into a tested helper. tests/test_daemon_open_readiness.py is a manual guard that no CI leg runs, so CI would not notice if the always-enable behaviour came back. The rule now lives in a small pure function, cbm_ui_config_daemon_start_ui() in src/ui/config.c, which main.c calls on the cold start path (same behaviour; the UI-build check stays in main.c). A new ui-suite test, config_daemon_start_honours_saved_ui_choice, checks the combinations of saved setting, --port and --open. We confirmed it fails both ways: with the old always-configure rule (a bare start with a saved disabled UI) and with a rule that ignores --port and --open.
  2. f31d7e8 test(daemon): read the saved UI setting through config get. Current main keeps the UI keys written by config set in the shared settings database (_config.db) rather than config.json, so the guard stopped with FileNotFoundError before its first assertion. It now reads ui_enabled and ui_port back with config get, which loads the same value every start path uses. All seven guard cases pass on macOS, and with the always-configure rule put back the disabled bare-start case fails again, so the guard still catches daemon start silently overrides persisted ui_enabled=false and re-enables the graph UI listener #2114.
  3. da7df66 merges current main into the branch (no conflicts), so CI runs on an up-to-date tree.

Locally the ui and daemon suites (ui, daemon, daemon_runtime, daemon_application, daemon_ipc) pass: 257 passed, plus 2 skips that apply only on Linux. Once CI is green on this head, it merges.

If you push to this branch again, please pull first so these commits are kept. Thanks again for the careful work and the unusually honest validation notes.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Thanks for your patience, @vincentkoc: CI went green, but another change touched docs/CONFIGURATION.md on main in the meantime, so we merged current main into your branch once more (c7e7b23, a plain signed-off merge commit; your change is unchanged) to let CI cover the exact tree that will merge. It merges as soon as this run is green. Please pull before pushing again.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Thanks for your patience, @vincentkoc: the index resource-worker change (#1724) merged into main last night and touched src/main.c and docs/CONFIGURATION.md next to your change, so we merged current main into your branch once more (a plain signed-off merge commit, no conflicts; your change is unchanged) to get a fresh run on the exact tree that will merge. Please pull before pushing again.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Thank you again, @vincentkoc. Another change merged into main tonight and touches src/main.c and docs/CONFIGURATION.md, which this PR also changes, so we merged current main into your branch - a plain signed-off merge commit, your change is untouched - to get a fresh CI run on the exact tree that will merge. Nothing is needed from you: it merges as soon as that run is green. If you push again, please pull first so the merge commit is kept.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

daemon start silently overrides persisted ui_enabled=false and re-enables the graph UI listener

2 participants