core: avoid push-to-talk loop under systemd - #72
Conversation
|
Jetson acceptance for #71:
Validated on Jetson after rebuilding the aarch64 binary from PR head and redeploying binaries. |
ai-hpc
left a comment
ai-hpc
left a comment
There was a problem hiding this comment.
Verified the bug independently against the deployed genie-core.service — StandardInput=null + default voice_enabled = true + wakeword_script = "" means push-to-talk enters, immediately reads stdin EOF, exits 0, and systemd's Restart=always loops it forever (the issue body shows restart counter at 344). Fix is the right shape: gate voice-mode entry on voice_requested && (stdin_interactive || wakeword_available) so a daemon-mode genie-core with no wakeword cleanly falls through to the HTTP API.
The follow-up refactor commit (9ab3e71) is a clean improvement on the original (0847541) — extracts the decision into a proper runtime_mode module with a StartupDecision { mode, reason } shape, so the four startup paths each get a named StartupReason variant:
VoiceNotRequested→ HTTP APIInteractivePushToTalk→ voice loop (terminal)WakewordDaemon→ voice loop (daemon with wakeword)PushToTalkNeedsTerminal→ HTTP API (the new fallback path)
That gives observability and future logging a real handle on why a particular startup path was selected, not just a boolean. The wakeword-before-interactive ordering is the right precedence — if an operator runs an interactive genie-core with a wakeword script configured, they want wakeword mode, not push-to-talk. Same outcome (voice mode), more accurate reason.
blocked_push_to_talk() is a precise predicate for "we tried, we couldn't" — used to fire the single warn log only on the actual stdin-EOF-would-have-killed-us path, not on the voice_requested = false quiet path. Right choice.
The four runtime_mode::tests cover the 2×2 matrix of (interactive, wakeword_available) for voice_requested = true plus the voice_requested = false short-circuit — that's exhaustive for a 3-bool input space.
wakeword_available is checked as !config.core.wakeword_script.as_os_str().is_empty() && config.core.wakeword_script.exists() — both presence in config AND on disk. Catches the case where the operator sets a wakeword path but the script never got deployed; we don't false-claim "wakeword daemon mode" then crash because the script is missing.
The #[cfg(not(feature = "voice"))] branch keeps let _ = startup_decision; so a chat-only build stays clippy-clean. That's exactly the kind of small thing #62's no-default-features CI job would have caught if missed — and that job is green here.
Contributor's Jetson acceptance test (in the PR comment) is the smoking gun: service goes from activating (auto-restart) → active (running), the expected WARN push-to-talk voice mode requested … running HTTP API only line appears once in journalctl, and genie-ctl status reports Core: ok. End-to-end verified on the same hardware the bug was reported against.
All 5 CI checks green on 9ab3e71 (fmt, clippy, test, aarch64 cross-compile, --no-default-features axis). Going in.
|
Merged at |
Fixes #75. Issue evidence on Jetson Orin Nano 8 GB shows the runtime's auto-clamp behavior is order-sensitive: same `-c 4096` request fits only ~1.7k context when the full stack is already resident, but fits 4k / 6k / 8k cleanly when `genie-ai-runtime` loads first. This PR pins the right startup order so the runtime claims its KV cache before memory-heavy services occupy DRAM. Three coordinated changes: 1. `Before=genie-whisper.service genie-whisper-warmup.service homeassistant.service genie-core.service` on `genie-ai-runtime.service`. systemd ordering directive that makes the LLM unit's load complete before the other memory-heavy services start. Combined with PR #72's existing `After=genie-ai-runtime.service genie-llm.service` on `genie-core`, the dependency is now bidirectional. `Before=` is a no-op for units that aren't installed on this host, so this is safe for installs that don't ship homeassistant or whisper. 2. `GENIEPOD_AI_RUNTIME_CONTEXT` default bumped from `2048` to `8192` — the largest context the issue verified loads cleanly with `--int8-kv` on Orin Nano 8 GB. The env knob stays settable via systemd drop-in for smaller Jetsons. 3. `deploy/scripts/start_all.sh` reorders the `UNITS=(...)` array so the configured LLM unit + warmup run before `homeassistant`, `genie-whisper`, `genie-whisper-warmup` in the manual lifecycle path too, mirroring the systemd `Before=`. Tests added to `tool_dispatch_test.rs` lock both invariants: - `start_all_uses_configured_llm_backend` asserts `$configured_llm_unit` appears before `homeassistant.service` and `genie-whisper.service` in the `UNITS=` array. - `genie_ai_runtime_service_preserves_model_page_cache` asserts `GENIEPOD_AI_RUNTIME_CONTEXT=8192` and that the new `Before=` clause is present. Compatibility with PR #70 (warm page cache across restart) is preserved — `Before=` only affects boot-time ordering, not `systemctl restart genie-ai-runtime` alone. End-user verified on the same Jetson the issue was filed against. Worth a follow-up: PR #74's `GENIE_RUNTIME_MAX_BODY_BYTES = 4 KB` body-compaction threshold is now leaving performance on the table at the new 8192-token runtime context (the client compacts prompts the runtime could now handle). Right path is to make the threshold a function of `GENIEPOD_AI_RUNTIME_CONTEXT` or probe runtime capacity at connection time. Not blocking this PR. All 7 CI checks green on `c1cae29` (fmt, clippy, test, aarch64 cross-compile, shellcheck, ruff, `--no-default-features`).
Raises the `genie-core` release binary size budget from the alpha-era `5.0 MB` to `6.0 MB`. The 5 MB ceiling was set when `genie-core` was much smaller; legitimate growth since then (LLM backend facade #35-#43, voice cargo feature #57, telegram voice in/out #53/#64, runtime_mode module #72, LocalSet concurrent server #87, per-call STT nonce #68) pushed the binary to `5.07 MB`, making the test a known-flaky drag on every recent PR. The new ceiling is intentionally tight: ~0.93 MB of headroom over the current 5.07 MB. That's enough to absorb the next legitimate growth bump but small enough that a future PR adding ~1 MB of dependencies or modules will trip the assert and force a deliberate raise-or-shrink decision — exactly what a size budget should do. Implementation: - Extracted `RELEASE_BINARY_SIZE_BUDGET_MB: f64 = 6.0` constant with a `///` doc comment explaining the alpha-era origin, the load-bearing growth, and the "keep it tight to force deliberate decision" principle. - Assertion message now echoes the budget back to the failure output, so future contributors hitting the assert know exactly which constant to inspect without grepping. All 6 CI checks green on `ea85c53` (fmt, clippy, test, aarch64 cross-compile, `--no-default-features`, PR body checklist).
2 participants
Summary
Fixes #71.
genie-core.serviceruns withStandardInput=null, but the deployed config can request voice mode withvoice_enabled = trueand no wakeword script. That selects push-to-talk mode, which immediately reads stdin EOF under systemd, exits successfully, and then restarts forever because the unit hasRestart=always.This PR gates voice mode so it only runs when:
Otherwise
genie-corelogs a warning and runs the HTTP API instead of entering the stdin-driven push-to-talk loop.Validation
cargo fmt --all --checkcargo test -p genie-core --bin genie-core voice_mode_testscargo test -p genie-core --no-default-features --bin genie-core voice_mode_testsgit diff --checkNotes
This keeps issue #71 separate from issue #69 / PR #70.
genie-ai-runtimeand the OpenAI-compatible chat endpoint were already validated; this fixes thegenie-coredaemon mode selection.