fix(logging): fix logging naming by adcondev · Pull Request #9 · adcondev/scale-daemon

adcondev · 2026-02-09T23:42:33Z

This pull request introduces several improvements and refactors to how service naming and logging are handled, as well as enhances the HTTP server lifecycle management for the Bascula service. The changes ensure that log directory naming is consistent and injected via build flags, improve graceful shutdown of the HTTP server, and simplify the server setup. Additionally, minor fixes and clarifications were made to configuration and frontend code.

Service Naming & Logging Improvements:

Added new SVC_LOG_NAME_REMOTE and SVC_LOG_NAME_LOCAL variables to Taskfile.yml for consistent log directory naming, and switched build flags to inject LogServiceName instead of ServiceName for both local and remote builds. (Taskfile.yml) [1] [2] [3]
Introduced LogServiceName as a field in the Service struct (internal/daemon/program.go) and injected it via build flags, ensuring log directories match the new naming convention. (main.go, program.go) [1] [2] [3]
Updated logging setup to use LogServiceName if provided, falling back to environment config otherwise. (internal/daemon/program.go)

HTTP Server Lifecycle & Graceful Shutdown:

Refactored the HTTP server startup and shutdown logic to handle http.ErrServerClosed gracefully, avoiding double-close errors and ensuring proper resource cleanup. (internal/daemon/program.go) [1] [2]
Added a Shutdown method to the Server struct for graceful HTTP server shutdown, and ensured that the HTTP server is properly initialized and never nil. (internal/server/handler.go) [1] [2] [3] [4]

Configuration & Environment Clarifications:

Updated environment configuration to clarify DefaultMode semantics and changed service names to match the new log directory convention. (internal/config/config.go)

Frontend & WebSocket Endpoint Fixes:

Fixed WebSocket endpoint URL in the HTML frontend to use /ws and ensured correct path handling. (docs/old/html/index_envs.html)

Miscellaneous:

Added missing net/http import for server functionality. (internal/daemon/program.go)

These changes collectively improve maintainability, reliability, and clarity around service deployment, logging, and frontend connectivity.

Signed-off-by: Adrián Constante <ad_con.reload@proton.me>

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

) ## Fix Plan: Graceful Shutdown Deadlock & Log Directory Mismatch ✅ ### Issue 1: Graceful Shutdown Deadlock ✅ - [x] Review existing code - `handler.go` already has `httpServer` field and `Shutdown()` method - [x] Review existing code - `program.go` already calls `srv.Shutdown()` in `Stop()` - [x] Fix `run()` in `program.go` to handle `http.ErrServerClosed` as normal shutdown (not fatal) - [x] Add `net/http` import to `program.go` - [x] Verify shutdown flow: `Stop()` closes `s.quit` safely after server shutdown completes - [x] Add and refine comments to document the shutdown flow precisely ### Issue 2: Log Directory Naming Mismatch ✅ - [x] Add `LogServiceName` variable to `cmd/BasculaServicio/main.go` (renamed from `ServiceName`) - [x] Update `daemon.New()` to accept `logServiceName` parameter - [x] Store `LogServiceName` in `Service` struct with comprehensive documentation - [x] Use injected `LogServiceName` in `Init()` for logging setup instead of `s.env.ServiceName` - [x] Update Taskfile.yml to inject correct service names via `main.LogServiceName` - [x] Update config.go fallback defaults to match naming convention ### Code Review Feedback ✅ - [x] Rename `ServiceName` → `LogServiceName` to clarify it's for logging, not Windows SCM service name - [x] Align naming with Taskfile variables (`SVC_LOG_NAME_*`) - [x] Clarify shutdown flow with improved comments explaining how `s.quit` is closed - [x] Refine comment precision based on code review ### Testing & Verification ✅ - [x] Build and verify compilation succeeds - [x] Multiple code reviews - all issues resolved - [x] Run security scan - no vulnerabilities found - [x] Address all code review feedback  <details> <summary>Original prompt</summary> ## Context The `poster-tuis` TUI installer (https://github.com/adcondev/poster-tuis) manages the lifecycle of both `scale-daemon` and `ticket-daemon` Windows services. During testing, two critical issues were identified: 1. **Scale daemon hangs on stop/uninstall** — the service process never exits when `sc stop` is sent, leaving it in `STOP_PENDING` state until force-killed from Task Manager. The ticket-daemon stops cleanly with the exact same TUI code. 2. **TUI cannot open log files/folders** — the log directory path the TUI expects doesn't match the path the daemon actually writes to. --- ## Issue 1: Graceful Shutdown Deadlock (CRITICAL) ### Root Cause The `Stop()` method in `internal/daemon/program.go` deadlocks because: 1. `Stop()` calls `s.wg.Wait()` which waits for the `run()` goroutine to finish 2. `run()` is blocked on `<-s.quit` — waiting for the quit channel to close 3. The quit channel is **only** closed when `ListenAndServe()` returns with an error (line ~122 in `run()`) 4. `ListenAndServe()` in `internal/server/handler.go` uses the **raw** `http.ListenAndServe()` function which has **no way to be shut down gracefully** — there's no stored `*http.Server` reference 5. So `ListenAndServe()` never returns → `<-s.quit` blocks forever → `run()` never exits → `wg.Wait()` deadlocks → `Stop()` hangs → Windows SCM times out ### The deadlock chain: ``` SCM sends Stop signal → Stop() calls s.cancel() ✓ (context cancelled) → Stop() calls s.reader.Stop() ✓ (serial reader exits) → Stop() calls s.wg.Wait() ← BLOCKS HERE → run() blocked on <-s.quit ← BLOCKS HERE → s.quit only closed on ListenAndServe() error → http.ListenAndServe() has NO shutdown method ← ROOT CAUSE ``` ### Why ticket-daemon works The ticket-daemon's `Stop()` does it correctly: - Stores an `*http.Server` and calls `httpServer.Shutdown(ctx)` — gracefully shuts down HTTP server - Calls `wsServer.Shutdown()` — closes all WebSocket clients - Calls `close(p.quit)` — explicitly unblocks `run()` - Then calls `p.wg.Wait()` — now completes because `run()` can exit ### Required Fix **In `internal/server/handler.go`:** - Add an `httpServer *http.Server` field to the `Server` struct - In `ListenAndServe()`, create an `*http.Server{Addr: ..., Handler: mux}` and store it before calling `s.httpServer.ListenAndServe()` - Add a new `Shutdown(ctx context.Context) error` method that calls `s.httpServer.Shutdown(ctx)` if non-nil - Handle `http.ErrServerClosed` gracefully in `run()` (not as a fatal error — it's the normal shutdown path) **In `internal/daemon/program.go` `Stop()` method:** - After cancelling context and stopping reader, call `s.srv.Shutdown(ctx)` with a 10-second timeout to gracefully close the HTTP server - Explicitly call `close(s.quit)` to unblock `run()` - Then call `s.wg.Wait()` which will now complete - Update `run()` to check for `http.ErrServerClosed` and not treat it as a fatal error --- ## Issue 2: Log Directory Naming Mismatch ### What the Taskfile defines and TUI expects The Taskfile naming convention is `R2k_<<Daemon>><<Type>>_<<Env>>`. The Taskfile injects `main.ServiceName` via ldflags: - Scale Local: `-X main.ServiceName=R2k_BasculaServicio_Local` - Scale Remote: `-X main.ServiceName=R2k_BasculaServicio_Remote` The TUI installer (`poster-tuis`) constructs log paths using the **Windows service registry name** (RegistryName), which follows this convention: ``` C:\ProgramData\R2k_BasculaServicio_Local\R2k_BasculaServicio_Local.log C:\ProgramData\R2k_BasculaServicio_Remote\R2k_BasculaServicio_Remote.log ``` ### What the daemon actually does The `ServiceName` variable from `main` package is injected but **never forwarded to the logging setup**. Instead, `internal/config/config.go` has hardcoded `ServiceName` values in the `Environments` map: ```go var Environments = map[string]Environment{ "remote": { ServiceName: "BasculaServicio_Remoto", // ← Hardcoded, missing R2k_ prefix, uses "Remoto" instead of "Remote" }, "local": { ServiceName: "BasculaServicio_Local", // ← Hardcoded, missing R2k_ prefix }, } ``` The logging setup in `internal/logging/logging.go` uses this `ServiceName` for the log directory: ```go logDir := filepath.Join(os.Getenv("PROGRAMDATA"), serviceName) mgr.FilePath = filepath.Join(logDir, serviceName+".log") ``` So the actual log files end up at: ``` C:\ProgramData\BasculaServicio_Remoto\BasculaServicio_Remoto.log (remote) C:\ProgramData\BasculaServicio_Local\BasculaServicio_Local.log (local) ``` But the TUI and the desired convention expect: ``` C:\ProgramData\R2k_BasculaServicio_Remote\R2k_BasculaServicio_Remote.log C:\ProgramData\R2k_BasculaServicio_Local\R2k_BasculaServicio_Local.log ``` ### Required Fix — Make ServiceName injectable from ldflags The `main.ServiceName` variable (which IS correctly injected by the Taskfile as `R2k_BasculaServicio_Local` / `R2k_BasculaServicio_Remote`) needs to be forwarded through to the loggin... </details>  *This pull request was created from Copilot chat.* >  --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

Copilot

Pull request overview

This PR refactors service/log naming to ensure log directories are consistently named via build-time injection, and improves HTTP server lifecycle management by centralizing server setup and adding a graceful shutdown path.

Changes:

Inject LogServiceName via Taskfile.yml/ldflags and plumb it through cmd/BasculaServicio → daemon.Service → logging.Setup().
Refactor HTTP server construction into server.NewServer() and add Server.Shutdown() to support graceful shutdown.
Update environment service naming and adjust the legacy HTML frontend’s WebSocket endpoint to /ws.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`Taskfile.yml`	Adds `SVC_LOG_NAME_*` vars and injects `main.LogServiceName` for service builds.
`cmd/BasculaServicio/main.go`	Introduces `LogServiceName` build var and passes it into `daemon.New(...)`.
`internal/daemon/program.go`	Uses injected log name for logging and updates HTTP server start/stop/shutdown behavior.
`internal/server/handler.go`	Builds a persistent `http.Server` in `NewServer()` and adds `Shutdown(ctx)`.
`internal/config/config.go`	Updates environment `ServiceName` values and modifies `DefaultMode` comment.
`docs/old/html/index_envs.html`	Updates WebSocket URL to include `/ws`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-09T23:46:50Z

 		if err := s.srv.ListenAndServe(); err != nil {
-			log.Printf("[X] Error al iniciar servidor: %v", err)
-			close(s.quit)
+			// http.ErrServerClosed is expected during graceful shutdown.
+			// Stop() closes s.quit after Shutdown() completes, avoiding double-close.
+			if err != http.ErrServerClosed {
+				log.Printf("[X] Error al iniciar servidor: %v", err)
+				close(s.quit)
+			}
 		}


When ListenAndServe() returns an unexpected error, run() is unblocked by closing s.quit, but the broadcaster/reader goroutines keep running because s.cancel() is not called in that error path. Consider canceling the service context (and/or stopping the reader) when the HTTP server fails to start so the process doesn’t continue in a degraded state.

Copilot · 2026-02-09T23:46:50Z

 	ListenAddr  string
 	DefaultPort string
-	DefaultMode bool // true = test mode (simulated weights)
+	DefaultMode bool // false = test mode (simulated weights)


DefaultMode is assigned directly to Config.ModoPrueba, and both environments set DefaultMode: true, which implies “true = test mode”. The updated comment says “false = test mode”, which is inverted/misleading; please correct the comment to match the actual semantics.

Suggested change

DefaultMode bool // false = test mode (simulated weights)

DefaultMode bool // true = test mode (simulated weights)

Let's try to have both local and remote environments in real weights, so by default both avoid to use test mode weights

adcondev · 2026-02-09T23:54:37Z

@copilot open a new pull request to apply changes based on the comments in this thread

Both environments should avoid using test weights by default.

Copilot · 2026-02-09T23:54:44Z

@adcondev I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

#10) The `DefaultMode` configuration field had an inverted comment ("false = test mode") and both environments defaulted to test mode with simulated weights instead of reading from actual hardware. **Configuration Changes:** * Fixed `DefaultMode` comment to reflect actual semantics: `true = test mode (simulated weights), false = real weights` * Changed both `local` and `remote` environments to `DefaultMode: false` for real scale readings by default ```go // Before DefaultMode bool // false = test mode (simulated weights) // ... "remote": { DefaultMode: true, // Defaults to test mode }, // After DefaultMode bool // true = test mode (simulated weights), false = real weights // ... "remote": { DefaultMode: false, // Defaults to real weights }, ``` Both deployment environments now read from actual scale hardware by default, with test mode available via runtime configuration updates when needed.  --- ✨ Let Copilot coding agent [set things up for you](https://github.com/adcondev/scale-daemon/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adrián Constante <ad_con.reload@proton.me>

Copilot · 2026-02-10T16:44:51Z

@adcondev I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you.

* Initial plan * fix: cancel context when HTTP server fails to start Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

adcondev and others added 8 commits February 6, 2026 16:36

fix(server): enhance HTTP server setup and graceful shutdown handling

4f69319

Signed-off-by: Adrián Constante <ad_con.reload@proton.me>

Initial plan

d1fd6a0

Fix graceful shutdown deadlock and log directory naming

d959ec4

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

Improve ServiceName field documentation

6e22590

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

Rename ServiceName to LogServiceName for clarity

0ecee0c

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

Clarify shutdown flow with improved comments

88333a8

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

Refine shutdown flow comments for clarity

455fc46

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

adcondev requested a review from Copilot February 9, 2026 23:42

adcondev assigned adcondev and Copilot Feb 9, 2026

adcondev added this to POS RED2000 Feb 9, 2026

Copilot started reviewing on behalf of adcondev February 9, 2026 23:43 View session

Copilot AI reviewed Feb 9, 2026

View reviewed changes

Initial plan

3d3e837

Copilot AI mentioned this pull request Feb 9, 2026

fix(config): correct DefaultMode semantics and default to real weights #10

Merged

Copilot AI and others added 5 commits February 9, 2026 23:56

fix: set DefaultMode to false for real weights by default

cb2145d

Co-authored-by: adcondev <38170282+adcondev@users.noreply.github.com>

fix(dashboard): apply suggestion from @Copilot

a09a2cd

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adrián Constante <ad_con.reload@proton.me>

fix(server): apply suggestion from @Copilot

6655199

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adrián Constante <ad_con.reload@proton.me>

fix(server): apply suggestion from @Copilot

37c4fe5

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adrián Constante <ad_con.reload@proton.me>

Copilot AI mentioned this pull request Feb 10, 2026

fix: cancel context on HTTP server startup failure #11

Merged

adcondev merged commit 2ece0e2 into master Feb 10, 2026

adcondev deleted the fix/tuis branch February 10, 2026 16:52

github-project-automation Bot moved this to Done in POS RED2000 Feb 10, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(logging): fix logging naming#9

fix(logging): fix logging naming#9
adcondev merged 15 commits into
masterfrom
fix/tuis

adcondev commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

adcondev Feb 9, 2026

Uh oh!

adcondev commented Feb 9, 2026

Uh oh!

Copilot AI commented Feb 9, 2026

Uh oh!

Copilot AI commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	DefaultMode bool // false = test mode (simulated weights)
	DefaultMode bool // true = test mode (simulated weights)

Conversation

adcondev commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

adcondev Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

adcondev commented Feb 9, 2026

Uh oh!

Copilot AI commented Feb 9, 2026

Uh oh!

Copilot AI commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants