Skip to content

Add daemon-managed CLI runtime and manual test lab#274

Open
tuhalf wants to merge 18 commits into
masterfrom
tuhalf/daemon_support
Open

Add daemon-managed CLI runtime and manual test lab#274
tuhalf wants to merge 18 commits into
masterfrom
tuhalf/daemon_support

Conversation

@tuhalf
Copy link
Copy Markdown
Collaborator

@tuhalf tuhalf commented Apr 9, 2026

This pull request introduces several significant improvements to the diode application's root command handling, daemon mode management, and test coverage. The main focus is on refactoring flag registration for better maintainability, adding robust support for daemon mode lifecycle management, and introducing comprehensive unit tests for new and existing behaviors.

Key changes include:

Refactoring and Flag Handling

  • Extracted root flag registration into a reusable registerRootFlags function and added a newRootConfig constructor for consistent config initialization. This improves maintainability and enables easier reuse in tests and daemon logic. (cmd/diode/app.go)
  • Added a new -no-daemon flag to allow running the app in standalone mode, bypassing daemon management. (cmd/diode/app.go)

Daemon Mode and Application Lifecycle

  • Introduced new fields and methods in the Diode struct to manage daemon mode lifecycle, including mutex-protected state, mode-specific deferrals, and clean shutdown of mode-specific resources. (cmd/diode/app.go) [1] [2] [3] [4]
  • Refactored the application start and shutdown logic to ensure proper resource management and prevent double initialization. (cmd/diode/app.go) [1] [2] [3]

Error Handling and Status Reporting

  • Added a dedicated exitStatusError type for improved error reporting with custom exit codes, supporting better CLI feedback and integration. (cmd/diode/cli_status.go)

Daemon Mode Configuration and Hot Reload

  • Enhanced the config API server to support hot-reloading of configuration in daemon mode without restarting the process, improving uptime and user experience. (cmd/diode/config_server.go)

Testing and Reliability

  • Added a comprehensive test suite for daemon mode argument parsing, config sanitization, environment variable management, and command-line argument filtering to ensure correctness and prevent regressions. (cmd/diode/daemon_test.go)

These changes collectively improve the robustness, maintainability, and testability of the application's root command and daemon management infrastructure.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a daemon mode for the Diode CLI, allowing it to run as a background process to manage persistent states like published ports, SOCKS proxies, and join controllers. The changes include a new daemon handler, inter-process communication via Unix sockets or Windows pipes, and a management CLI. My review identified several critical issues, including race conditions in global state management, state leakage due to incomplete global variable resets, a lack of panic recovery in connection handling, and a potential deadlock in the mode shutdown sequence.

Comment thread cmd/diode/daemon.go
Comment on lines +492 to +494
*config.AppConfig = cloneDaemonConfig(&daemonState.baseConfig)
resetTransientConfig(config.AppConfig)
resetRequestGlobals()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Overwriting the global config.AppConfig struct content and resetting request globals while a background mode (like publish or join) is active creates a significant race condition. Background goroutines belonging to the active mode rely on these global states. For example, resetRequestGlobals will clear network or dryRun settings used by a running join controller, potentially causing it to malfunction or switch networks unexpectedly during its next reconciliation loop.

Comment thread cmd/diode/daemon.go
Comment thread cmd/diode/daemon.go
Comment thread cmd/diode/app.go Outdated
Copy link
Copy Markdown
Member

@dominicletz dominicletz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our discussion architecture changes:

  1. Start with a spec file for this feature for claude code
  2. Ensure to have one implementation module for changing configs. E.g. with this PR we now we have 3 options to change publishing ports 1. CLI, 2. API 3. JOIN and 3x times code duplication. It should really only be one module registering the commands and parameters into a table, and each "INTERFACE" (CLI, API, JOIN) should read the table and based on that build what it can support.
  3. Default behavior should be: "stay in foreground" - we still launch daemon but the CLI waits for the daemon and prints it's output. The same way docker run works -- Only when the user provides the -d flag should the client go to background mode.
  4. Key question: How to manage multiple walllets? We want to also run multiple wallets as daemon as well, would we for that spawn multiple background daemons or enable the CLI to manage multiple wallets.

tuhalf added 2 commits May 7, 2026 13:12
# Conflicts:
#	cmd/diode/app.go
#	cmd/diode/config_server.go
#	cmd/diode/gateway.go
#	cmd/diode/join.go
#	cmd/diode/publish.go
#	cmd/diode/socksd.go
#	cmd/diode/ssh.go
#	config/flag.go
@dominicletz
Copy link
Copy Markdown
Member

Crash on first test:

time ./diode -logdatetime -debug ssh ubuntu@miner2023.diode echo hi 
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x85e2b6]

goroutine 1 [running, locked to thread]:
github.com/diodechain/diode_client/config.(*Logger).Info(0x0, {0x163ac10?, 0x18fc600?}, {0xc00052d7b8?, 0x163acce?, 0x2?})
	/home/dominicletz/projects/diode/diode_client/config/logger.go:143 +0x36
github.com/diodechain/diode_client/config.(*Config).PrintLabel(0xc0002abb08, {0x16567c2?, 0x1?}, {0xc0004e23f0, 0xf})
	/home/dominicletz/projects/diode/diode_client/config/flag.go:269 +0x178
main.runSSHViaDaemonLease({0xc0000400a0, 0x4, 0x4}, {0x1, {0x0, 0x0}, {0x0, 0x0}, 0x0, {0x0, ...}, ...})
	/home/dominicletz/projects/diode/diode_client/cmd/diode/ssh.go:193 +0x89
main.maybeHandleDaemonCLI({0xc000040080?, 0xc00052df40?, 0x419c88?})
	/home/dominicletz/projects/diode/diode_client/cmd/diode/daemon.go:290 +0x5f8
main.main()
	/home/dominicletz/projects/diode/diode_client/cmd/diode/diode.go:30 +0x9a


@tuhalf
Copy link
Copy Markdown
Collaborator Author

tuhalf commented May 8, 2026

@gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a daemon-by-default architecture for the Diode client, enabling background execution of services like publish, gateway, and socksd with management via a new diode daemon command. The implementation includes cross-platform transport support (Unix sockets and Windows named pipes), attached log streaming, and a refactored output system to handle remote command execution. Feedback focuses on critical concurrency and state management issues, including a race condition in writer cleanup, missing global variable resets between requests that cause configuration leakage, and potential daemon unresponsiveness during updates. Additionally, improvements were suggested for the daemon startup specification to ensure correct flag persistence and compatibility checks.

Comment thread cmd/diode/daemon.go Outdated
Comment thread cmd/diode/daemon.go
Comment thread cmd/diode/daemon.go Outdated
Comment thread cmd/diode/daemon.go Outdated
Comment thread cmd/diode/daemon.go
@tuhalf tuhalf marked this pull request as ready for review May 8, 2026 12:14
@tuhalf
Copy link
Copy Markdown
Collaborator Author

tuhalf commented May 8, 2026

@gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a background daemon architecture for the Diode client, allowing long-running commands like publish, gateway, and socksd to run persistently. By default, these commands now stay attached to the foreground to stream output, mimicking docker run behavior, but can be detached using the new -d or --detach flags. A new daemon management command has been added to inspect status, restart, or stop the background process. The implementation includes cross-platform support for IPC via Unix sockets and Windows named pipes. Feedback indicates that a block of code in the fetch command's file handling is unreachable and should be removed.

Comment thread cmd/diode/fetch.go Outdated
@tuhalf tuhalf requested a review from dominicletz May 8, 2026 13:55
@dominicletz
Copy link
Copy Markdown
Member

Issues during testing:

  1. The first call (e.g. to diode time) always prints Client address + Fleet address but subsequent calls are missing it. All calls should include it or none
  2. diode gateway doesn't print anything anymore
  3. daemon status doesn't seem to report correctly

@dominicletz
Copy link
Copy Markdown
Member

diode ssh is broken now:

$ ./diode ssh ubuntu@miner2023.diode echo hi
proxy lease did not expose an address
INFO Using diode daemon proxy :                                           
Couldn't execute command: missing -proxy-addr
Connection closed by UNKNOWN port 65535

@dominicletz
Copy link
Copy Markdown
Member

As we discussed diode update needs to restart the daemon too.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants