wakeword: treat empty config as disabled - #101
Conversation
ai-hpc
left a comment
There was a problem hiding this comment.
Closes #100 cleanly with a three-layer fix at exactly the right boundaries:
deploy/scripts/start_all.shdoesn't try to start the unit when wakeword is disabled — and proactivelysystemctl stop+reset-failedit to clear any stalefailedstate from a previous run. That alone fixes the dashboard symptom going forward, but doesn't help boxes that already have the failed state cached, hence the next two layers./api/servicesshort-circuits the merge whendisabled_reasonis set — returns a synthetic row withhealthy: true,source: "config",sub_state: "disabled", and the disabled-reason as the error string. Crucially, the wakeword target is also dropped fromunique_units()so the API doesn't even forksystemctl showfor it. Saves a probe and ensures the merge result genuinely doesn't depend on whatever stale systemd state the box is sitting on.dashboard.jsserviceStatus()gets aif (s.source === 'config') return { label: 'Disabled', ... }first-branch that renders gray rather than green/red, so the visual mapping matches the data model.
Each layer is necessary, none is redundant. Drop the systemd-stop and a box that previously failed stays red until reboot. Drop the API short-circuit and the dashboard still has to interpret a failed row as Disabled from JS alone (fragile across future renders). Drop the UI branch and source: "config" shows up as "Unknown".
disabled_reason: Option<String> on ServiceTarget is a clean extension point — other services could plumb the same shape later (e.g., homeassistant when no ha_token, mqtt when broker isn't configured) without re-architecting.
The trickiest piece is the __missing__ sentinel in read_wakeword_script:
END { if (!found) print "__missing__" }This distinguishes "key absent from the toml entirely" (legacy config, use the compiled default) from "key present but empty value" (explicit operator opt-out). Without that, a geniepod.toml from a pre-#100 deploy would suddenly start treating wakeword as disabled on the next start_all.sh, which would be a silent regression for anyone running on default config. The sentinel + fallback line preserves the existing semantics exactly. Right call.
Test coverage is the right shape:
dashboard_targets_mark_wakeword_disabled_when_config_empty— pins that empty config →disabled_reason: Some(...)AND that the unit gets dropped from the probe list. Catches a regression where someone restores theunique_unitsline and starts re-probing the disabled unit.disabled_wakeword_rows_ignore_stale_failed_systemd_stateis the killer test: even withactive_state: "failed"in the injectedSystemdRow, the disabled flag wins and the merged row reportshealthy: truewithsource: "config". This is the exact scenario from #100 (box has a cached failed state) and the test pins that the disabled path supersedes it.start_all_uses_configured_llm_backendextended with three new assertions: theread_wakeword_scriptfunction exists, thepush-to-talk modeskip reason string is present, andreset-failed "$unit"is called for disabled services. Pins the start_all-side behavior so a future shell refactor can't quietly undo thereset-failedstep.
The operator's Jetson dashboard snapshot in the PR body is the end-to-end verification — wakeword now renders as Disabled with n/a latency, the row no longer looks broken, and the rest of the stack reads exactly the same as before.
Two minor things to flag, neither blocking:
- The
disabled_reasonstring is hardcoded atdashboard_service_targetsrather than being i18n-able or pulled from config. That's fine for an alpha — single English deployment surface — and the symmetry with the existing error-string handling means contributors don't have to learn a new pattern. Worth noting in case a future PR wants to translate dashboard strings. unique_unitsfilter drops disabled targets from the probe list, butsystemdBTreeMap entries for those units could still appear if some other code path inserts them (e.g., a future probe that walks systemd state independently). Not a current issue — the only producer of the map isquery_systemd_unitinvoked from the loop that consumesunique_units— but amerge_service_rowstest asserting that a stalesystemdentry for a disabled unit is ignored would close the door on that class of regression. Out of scope for this PR.
All 8 CI checks green on d9d8835 (fmt, clippy, test, aarch64 cross-compile, --no-default-features, shellcheck, ruff, PR body checklist). LGTM.
|
Merged at |
Summary
[core].wakeword_scriptas explicit push-to-talk mode instart_all.shreset-failedgenie-wakeword.servicewhen wakeword is disabled by configDisabled/configin/api/servicesinstead of surfacing stale failed systemd stateFixes #100
Testing
wakeword_script = ""Real Behavior Proof
Jetson dashboard result after deploying this branch: