Feat:headless mode daemon with telemetry output and command listening#249
Merged
sudip-mondal-2002 merged 18 commits intoMay 28, 2026
Merged
Conversation
github-actions
Bot
added
High Impact
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CliOptions contract, enforces headless preset validation, implements headless startup/runtime (no GUI, preset load, optional device auto-routing, stdin command thread, telemetry), makes Effect::enabled atomic and serialized, updates CLI tests, adds headless integration tests, and wires the test into CMake. ChangesHeadless Mode Feature
sequenceDiagram
participant User
participant CLI as handle_cli_args
participant Main as main.cpp
participant Audio as AudioEngine
participant Stdin as StdinListener
User->>CLI: amplitron --headless --preset live.json
CLI->>Main: return CliOptions (is_headless,preset_path,input_device,output_device,exit_reason)
Main->>Main: if exit_early -> print exit_reason & return
alt is_headless
Main->>Audio: load preset (GUI=nullptr)
Main->>Audio: auto-select devices (substring match)
Main->>Audio: start audio stream
Main->>Stdin: spawn detached stdin command thread
Stdin->>Audio: enqueue commands (gain,bypass,enable,chain)
Main->>Main: print telemetry periodically (if enabled)
else
Main->>Main: init GUI and restore session
end
Main->>Audio: shutdown engine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Code Coverage Report 📊Line Coverage: 80.0% ✅ Coverage meets threshold: 80.0% >= 60% Full Coverage Summary |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integration/test_headless.cpp (1)
12-13: ⚡ Quick winAssert
engine.initialize()succeeds before exercising behavior.Both tests continue even if engine init fails, which can hide root-cause failures behind downstream assertions.
Suggested patch
TEST(headless_offline_dsp_gain_staging) { AudioEngine engine; - engine.initialize(); + ASSERT_TRUE(engine.initialize()); @@ TEST(headless_command_queue_execution) { AudioEngine engine; - engine.initialize(); + ASSERT_TRUE(engine.initialize());Also applies to: 40-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_headless.cpp` around lines 12 - 13, Add an assertion that engine.initialize() succeeded immediately after calling it (e.g., use ASSERT_TRUE/EXPECT_TRUE or ASSERT_NO_THROW as appropriate for your test framework) so the test stops if initialization fails; update both occurrences of engine.initialize() (the one at the top and the one around lines 40–41) to assert success before proceeding to exercise other behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli.h`:
- Around line 39-47: The flag handlers for "--preset", "--input", and "--output"
currently skip when a value is missing; update the parsing logic in the CLI loop
that assigns options.preset_path, options.input_device, and
options.output_device so that if i+1 >= argc you set exit_early = true and emit
a clear error (e.g., "missing value for --preset/--input/--output") instead of
silently ignoring the flag; keep the existing assignment path (argv[++i]) when a
value exists but add an explicit else branch for each flag to fail early.
In `@src/main.cpp`:
- Line 229: The GuiManager is always being constructed (Amplitron::GuiManager
gui(engine)), including in headless runs; move its construction into the GUI
branch so it is only instantiated when not headless. Update the code paths
around the headless check (the GUI branch where GUI setup and event loop run) to
create Amplitron::GuiManager gui(engine) there and remove the global/early
construction; ensure any code that needs gui is also inside or guarded by the
same non-headless branch (or use a pointer/optional to lazily construct
GuiManager) so headless runs never depend on GuiManager.
- Around line 349-417: The stdin_listener thread is detached while capturing
engine by reference and the process calls engine.shutdown() then std::_Exit(0),
creating a race; also the "chain" command reads engine.graph().get_nodes() and
node.pedal->is_enabled() without the audio-side synchronization
(Effect::enabled_ is non-atomic). Fix by not detaching stdin_listener: keep the
thread object and join it during shutdown after signalling g_running=false (or
reuse show_telemetry/another atomic) so the listener exits before
engine.shutdown()/std::_Exit(0); for the chain command avoid reading
engine.graph() and Effect internals directly from this thread — either call a
GUI-thread-safe snapshot API (e.g., add Engine::snapshot_graph_nodes() that
returns a copy) or expose a thread-safe accessor for effect state (e.g.,
Effect::is_enabled_atomic() or protect access with the same mutex used by
AudioEngine::drain_commands()); update usages of stdin_listener,
engine.shutdown(), std::_Exit(0), engine.graph().get_nodes(), and
node.pedal->is_enabled() accordingly.
---
Nitpick comments:
In `@tests/integration/test_headless.cpp`:
- Around line 12-13: Add an assertion that engine.initialize() succeeded
immediately after calling it (e.g., use ASSERT_TRUE/EXPECT_TRUE or
ASSERT_NO_THROW as appropriate for your test framework) so the test stops if
initialization fails; update both occurrences of engine.initialize() (the one at
the top and the one around lines 40–41) to assert success before proceeding to
exercise other behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7e6d560-34f4-477a-b724-d599d57be3a7
📒 Files selected for processing (5)
CMakeLists.txtsrc/cli.hsrc/main.cpptests/integration/test_headless.cpptests/unit/test_cli.cpp
Contributor
Author
|
@sudip-mondal-2002 please test it out with an actual instrument to check if the audio thread is stable. |
sudip-mondal-2002
requested changes
May 27, 2026
Comment on lines
+209
to
213
| //CLI argument parsing | ||
| Amplitron::CliOptions cli_opts = Amplitron::handle_cli_args(argc, argv); | ||
| if(cli_opts.exit_early){ | ||
| return 0; | ||
| } |
Owner
|
Check the conflict and |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.cpp (1)
347-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when the headless audio stream cannot start.
Headless mode still drops into the command loop after
engine.start()fails, which leaves a "ready" daemon running without an active audio stream. Make this fatal in the headless path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.cpp` around lines 347 - 349, If engine.start() returns false, make the process fatal when running in headless mode so we don't fall through into the command loop; after the existing engine.start() check, detect the headless flag used by the program (e.g. headless / isHeadless / headlessMode) and replace the current nonfatal warning with an error and immediate exit (return EXIT_FAILURE or std::exit(1)) when headless is true, otherwise keep the existing warning behavior for interactive runs; update the log message to be an error and ensure cleanup if necessary before exiting.
♻️ Duplicate comments (2)
src/cli.h (1)
42-70:⚠️ Potential issue | 🟠 MajorReject option tokens as missing values for value-taking flags.
i + 1 < argconly proves there is another token.--headless --preset --input miccurrently stores"--input"as the preset path, bypasses the required headless-preset validation, and--input/--outputat end still fall through because the outer&& i + 1 < argcmakes the innerelseunreachable. Treat a missing next token or a next option token as an error for all three flags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.h` around lines 42 - 70, The parsing logic for flags "--preset", "--input", and "--output" accepts any next token (including other option tokens) as a value; update the loop that inspects arg/argv/argc so that for options.preset_path, options.input_device, and options.output_device you first verify (i + 1 < argc) AND that argv[i+1] does not begin with '-' (or "--") before consuming it (argv[++i]); if the next token is missing or starts with '-' treat it as an error, set options.exit_early and options.exit_reason and return options; remove the redundant outer "&& i + 1 < argc" guards that make inner else unreachable so the explicit follow-up-token check handles both absence and option-token cases consistently.src/main.cpp (1)
367-375:⚠️ Potential issue | 🔴 CriticalDon't detach a thread that borrows stack state.
stdin_listenercapturescli_mutexandcli_commandsby reference, thendetach()lets it outlive the headless block. Once this block exits those references dangle, so any later stdin wakeup can lock freed storage. Keep the queue ownership outside the stack frame or make the listener joinable with a real shutdown path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.cpp` around lines 367 - 375, The stdin_listener thread captures cli_mutex and cli_commands by reference then is detached, risking use-after-free when the stack frame ends; fix by giving the listener ownership of shared lifetime objects (e.g., wrap cli_commands and cli_mutex in shared_ptr and capture those by value in stdin_listener) or change the threading model to keep stdin_listener joinable by introducing a shutdown flag (atomic<bool> done) and calling stdin_listener.join() at program shutdown; ensure you update the capture list of stdin_listener and remove detach() when using the joinable approach so references never dangle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main.cpp`:
- Around line 491-492: sessionManager.clearSession() is being called
unconditionally at shutdown which wipes any pending GUI crash-recovery session
even for headless/daemon runs; guard or relocate the clear so it only runs for a
GUI session (e.g. check engine.isHeadless() or an equivalent
Application::isHeadless()/Engine::isGuiMode() flag) or only when SessionManager
indicates a GUI session was created/restored (e.g. SessionManager::wasGuiUsed()
or SessionManager::createdThisRun()); move the clearSession() call out of the
global shutdown path and into the GUI-specific teardown (or wrap it with if
(!engine.isHeadless()) { sessionManager.clearSession(); }) so headless daemons
do not erase recovery state before engine.shutdown().
- Around line 213-219: The CLI headless flag is being applied after validation,
allowing a headless-only build to bypass required CLI checks and still return 0
on invalid args; set cli_opts.is_headless before running
Amplitron::handle_cli_args() or immediately re-validate and adjust behavior:
ensure AMPLITRON_HEADLESS forces cli_opts.is_headless prior to any validation
(or, if keeping current ordering, check cli_opts for missing required fields
after applying AMPLITRON_HEADLESS) and change the early-exit return to a
non-zero status when cli_opts.exit_early is true; update references to
Amplitron::CliOptions, Amplitron::handle_cli_args, cli_opts.is_headless,
cli_opts.exit_early, cli_opts.exit_reason, engine.initialize, and load_preset
accordingly.
---
Outside diff comments:
In `@src/main.cpp`:
- Around line 347-349: If engine.start() returns false, make the process fatal
when running in headless mode so we don't fall through into the command loop;
after the existing engine.start() check, detect the headless flag used by the
program (e.g. headless / isHeadless / headlessMode) and replace the current
nonfatal warning with an error and immediate exit (return EXIT_FAILURE or
std::exit(1)) when headless is true, otherwise keep the existing warning
behavior for interactive runs; update the log message to be an error and ensure
cleanup if necessary before exiting.
---
Duplicate comments:
In `@src/cli.h`:
- Around line 42-70: The parsing logic for flags "--preset", "--input", and
"--output" accepts any next token (including other option tokens) as a value;
update the loop that inspects arg/argv/argc so that for options.preset_path,
options.input_device, and options.output_device you first verify (i + 1 < argc)
AND that argv[i+1] does not begin with '-' (or "--") before consuming it
(argv[++i]); if the next token is missing or starts with '-' treat it as an
error, set options.exit_early and options.exit_reason and return options; remove
the redundant outer "&& i + 1 < argc" guards that make inner else unreachable so
the explicit follow-up-token check handles both absence and option-token cases
consistently.
In `@src/main.cpp`:
- Around line 367-375: The stdin_listener thread captures cli_mutex and
cli_commands by reference then is detached, risking use-after-free when the
stack frame ends; fix by giving the listener ownership of shared lifetime
objects (e.g., wrap cli_commands and cli_mutex in shared_ptr and capture those
by value in stdin_listener) or change the threading model to keep stdin_listener
joinable by introducing a shutdown flag (atomic<bool> done) and calling
stdin_listener.join() at program shutdown; ensure you update the capture list of
stdin_listener and remove detach() when using the joinable approach so
references never dangle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c934d877-c493-4a8a-a939-c8f1c81be946
📒 Files selected for processing (5)
CMakeLists.txtsrc/audio/effects/effect.hsrc/cli.hsrc/main.cpptests/integration/test_headless.cpp
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_headless.cpp
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.h (1)
54-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnreachable error handling for
--inputand--outputflags.The outer conditions at lines 54 and 65 include
&& i+1 < argc, so the innerif(i + 1 < argc)check is always true when reached. Theelsebranches (lines 57-62 and 68-73) are dead code. When a user provides--inputor--outputwithout a value, the flag is silently ignored instead of producing an error.The
--presethandling (lines 43-52) shows the correct pattern: check only the flag name in the outer condition, then validate argument availability inside.🐛 Proposed fix to match --preset pattern
- else if(arg == "--input" && i+1 < argc){ + else if(arg == "--input"){ if(i + 1 < argc){ options.input_device = argv[++i]; } else{ std::cerr << "Error: --input requires a device name argument.\n"; options.exit_early = true; options.exit_code = 1; options.exit_reason="Input Field Empty."; return options; } } - else if(arg == "--output" && i+1 < argc){ + else if(arg == "--output"){ if(i + 1 < argc){ options.output_device = argv[++i]; }else{ std::cerr << "Error: --output requires a device name argument.\n"; options.exit_early = true; options.exit_code = 1; options.exit_reason="Output Field Empty"; return options; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.h` around lines 54 - 74, The argument-parsing branches for the --input and --output flags currently include a redundant outer guard "&& i+1 < argc" which makes the inner availability checks and their error branches unreachable; remove the "&& i+1 < argc" from the outer else-if conditions for "--input" and "--output" so the code first matches the flag (arg == "--input" / arg == "--output") and then performs the inner if(i + 1 < argc) validation; if the inner check fails, set options.exit_early = true, options.exit_code = 1, options.exit_reason to a descriptive message and return options (same pattern used by the --preset handling), otherwise consume argv[++i] into options.input_device or options.output_device.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cli.h`:
- Around line 54-74: The argument-parsing branches for the --input and --output
flags currently include a redundant outer guard "&& i+1 < argc" which makes the
inner availability checks and their error branches unreachable; remove the "&&
i+1 < argc" from the outer else-if conditions for "--input" and "--output" so
the code first matches the flag (arg == "--input" / arg == "--output") and then
performs the inner if(i + 1 < argc) validation; if the inner check fails, set
options.exit_early = true, options.exit_code = 1, options.exit_reason to a
descriptive message and return options (same pattern used by the --preset
handling), otherwise consume argv[++i] into options.input_device or
options.output_device.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 319b4fe0-be2c-4160-9bdf-afc4de2cc2a7
📒 Files selected for processing (3)
CMakeLists.txtsrc/cli.hsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- src/main.cpp
Owner
|
Test coverage is soo less for both cli.cpp and main.cpp Can you push the coverage to 90%+ |
Contributor
Author
|
I can push cli.h past 90%, but main.cpp is very hard to do without hanging test runner since it has multiple infinite loops(like while(g_running)).It usually is excluded from lcov for this reason.Do tell how you would prefer to handle that. |
Owner
|
Okay ignore main.cpp for now, please cover cli.cpp |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.h (1)
43-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject option tokens as values for
--preset,--input, and--output.At Line 44, Line 55, and Line 66, the parser only checks bounds. If the next token is another flag, it is consumed as a value (e.g.,
--preset --headless), which silently misparses arguments and can defeat the intended fail-fast behavior.Suggested patch
inline CliOptions handle_cli_args(int argc, char* argv[]) { CliOptions options; + auto has_value_token = [&](int idx) { + return idx < argc && argv[idx] != nullptr && argv[idx][0] != '-'; + }; + for (int i = 1; i < argc; ++i) { std::string arg = argv[i]; @@ else if(arg == "--preset"){ - if(i + 1 < argc){ + if(has_value_token(i + 1)){ options.preset_path = argv[++i];//grabs next word(the preset path) } else{ std::cerr << "Error: --preset requires a file path argument.\n"; @@ else if(arg == "--input"){ - if(i + 1 < argc){ + if(has_value_token(i + 1)){ options.input_device = argv[++i]; } else{ std::cerr << "Error: --input requires a device name argument.\n"; @@ else if(arg == "--output"){ - if(i + 1 < argc){ + if(has_value_token(i + 1)){ options.output_device = argv[++i]; }else{ std::cerr << "Error: --output requires a device name argument.\n";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.h` around lines 43 - 75, The parser currently accepts the next argv token for --preset, --input, and --output without rejecting option tokens, so flags like "--preset --headless" get misparsed; update the handling for the "--preset", "--input", and "--output" branches to also check that argv[i+1] does not start with '-' (e.g., argv[i+1][0] != '-') before consuming it, and if it does start with '-' treat it as a missing argument: set options.exit_early=true, options.exit_code=1, options.exit_reason to an appropriate message and return options instead of assigning the flag as the value for options.preset_path, options.input_device, or options.output_device.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cli.h`:
- Around line 43-75: The parser currently accepts the next argv token for
--preset, --input, and --output without rejecting option tokens, so flags like
"--preset --headless" get misparsed; update the handling for the "--preset",
"--input", and "--output" branches to also check that argv[i+1] does not start
with '-' (e.g., argv[i+1][0] != '-') before consuming it, and if it does start
with '-' treat it as a missing argument: set options.exit_early=true,
options.exit_code=1, options.exit_reason to an appropriate message and return
options instead of assigning the flag as the value for options.preset_path,
options.input_device, or options.output_device.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a82a511c-b96a-4060-aac8-55603bc093d7
📒 Files selected for processing (2)
CMakeLists.txtsrc/cli.h
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
sudip-mondal-2002
added
gssoc:approved
This was referenced May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Implements a pure headless daemon mode for Amplitron, allowing it to run as a dedicated DSP rig on resource-constrained hardware (like a Linux server or Raspberry Pi) without a display server along with integration tests for the same.
Related Issue
Fixes #143
Type of Change
-[ ] 🐛 Bug fix
-[x] ✨ New feature
-[ ] ♻️ Refactor / code cleanup
-[ ] 📝 Documentation
-[x] ✅ Tests
-[x] 🔧 Build / CI / Configuration
How Was This Tested?
Platform(s) tested on: Linux (Fedora)
Test command run: make build followed by ./build/amplitron-tests (All 611 tests passing)
Manual test steps :
1. Booted with --headless without a preset to verify the failsafe exit.
2. Booted with a valid dummy preset and verified the audio engine instantiated correctly.
3. Sent live chain commands via stdin to verify the background thread communicated with the audio thread safely.
4. Used Ctrl+C to verify the SIGINT signal successfully exited the loop and safely tore down the hardware.
5. Tested telemetry toggling during live execution.
Checklist
-[x] Code compiles and builds successfully on my platform
-[x] All existing tests pass (./amplitron-tests)
-[x] New tests added for new functionality
-[ x] Documentation updated (README, code comments)
-[x] No blocking calls on the audio thread (no std::mutex::lock() on the hot path)
-[x] Tested on: Linux (Fedora)
Screenshots / Demo
manual_tests.md
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Serialization