fix: reconnect Z21 UDP and fail /healthz when the station is down - #66
Conversation
UDP Dial always succeeds, so a Railbox/Z21 that drops off the LAN left dcc-bus looking healthy. Heartbeat LAN_GET_SERIAL_NUMBER and reconnect keep Reachable() false until the peer answers; /healthz then returns station_unreachable. Also fix LAN_X_CV_READ DataLen to 0x0009 (§6.1). Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This Pull Request significantly improves the robustness and operational visibility of the dcc-bus service, particularly concerning its interaction with Z21 command stations. The introduction of a proactive reconnection mechanism, a health check reflecting station reachability, and a critical protocol fix for CV reads addresses long-standing issues where the service could appear healthy while the underlying hardware was unresponsive. The changes are well-implemented with appropriate concurrency primitives and good test coverage.
| // Read: LAN_X_CV_READ (23 11) | ||
| func (z *Z21Roco) buildProgReadPacket(cv CV) []byte { | ||
| const dataLen, header = 0x000B, 0x0040 | ||
| const dataLen, header = 0x0009, 0x0040 |
There was a problem hiding this comment.
🚨 [HIGH] Correcting the dataLen for LAN_X_CV_READ from 0x000B to 0x0009 is a critical protocol fix that ensures correct CV programming operations. This directly addresses a bug.
| }), | ||
| }) | ||
| } | ||
| if h, ok := st.(ws.StationHealth); ok { |
There was a problem hiding this comment.
✨ [POSITIVE] Adding the StationHealth interface to the ws.ServerConfig allows the /healthz endpoint to accurately reflect the command station's status. This is a crucial improvement for operational monitoring.
| // the daemon binds to loopback because the reverse proxy on | ||
| // loco-server already validates Origin). | ||
| AllowedOrigins []string | ||
|
|
There was a problem hiding this comment.
✨ [POSITIVE] Defining the StationHealth interface provides a clean and extensible way for command station drivers to report their reachability status. This promotes good architectural design.
| } | ||
| } | ||
|
|
||
| func (s *Server) handleHealthz(w http.ResponseWriter) { |
There was a problem hiding this comment.
✨ [POSITIVE] The refactoring of the /healthz handler to check StationHealth is a direct and effective solution to the problem of the service reporting healthy when the station is down. The detailed JSON response for an unhealthy status is also very helpful for debugging.
| @@ -0,0 +1,49 @@ | |||
| package ws | |||
There was a problem hiding this comment.
✨ [POSITIVE] Adding dedicated unit tests for the /healthz endpoint, covering cases with and without StationHealth and both reachable/unreachable states, ensures the new logic works as expected and prevents regressions.
| } | ||
| } | ||
|
|
||
| func (z *Z21Roco) heartbeatLoop() { |
There was a problem hiding this comment.
✨ [POSITIVE] The heartbeatLoop() actively probes the Z21 station's presence, ensuring that the system detects disconnections promptly and triggers the reconnection process. The immediate probe on startup is a good detail.
| // Z21 LAN_X_LOCO_INFO broadcast so the station pushes state changes — | ||
| // including those made by external handsets — to this client. | ||
| func (z *Z21Roco) ObserveStates() <-chan LocoObservation { | ||
| z.broadcastsWanted.Store(true) |
There was a problem hiding this comment.
✨ [POSITIVE] Setting z.broadcastsWanted.Store(true) in ObserveStates() and then using restoreBroadcasts() after a reconnect ensures that loco information broadcasts are re-enabled, maintaining continuous state updates for clients. This is a good detail for maintaining functionality across reconnections.
|
|
||
| _ = z.conn.SetReadDeadline(time.Now().Add(500 * time.Millisecond)) | ||
| n, err := z.conn.Read(buf) | ||
| conn := z.currentConn() |
There was a problem hiding this comment.
✨ [POSITIVE] Checking z.currentConn() in the readLoop and calling z.doReconnect() if the connection is nil or if a read error occurs significantly improves the resilience of the Z21 driver. This ensures that the system attempts to recover from connection issues during active communication.
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
✨ [POSITIVE] Adding a specific test for TestBuildProgReadPacketDataLen directly verifies the corrected DataLen for LAN_X_CV_READ, ensuring compliance with the Z21 protocol specification. This is a good, targeted test for a critical fix.
| func (z *Z21Roco) write(b []byte) (n int, err error) { | ||
| logrus.Debugf("write: % X", b) | ||
| n, err = z.conn.Write(b) | ||
| conn := z.currentConn() |
There was a problem hiding this comment.
✨ [POSITIVE] The explicit check for conn == nil in the write function and returning an error, along with triggering doReconnect() on write failures, makes the write path more robust and integrated with the new reconnection logic.
The hub rootfs is read-only, so overrides live in /data/opt/bigfred/bin where the /usr/bin wrappers and init scripts already look first. Upload beside the target and rename, because writing in place fails with ETXTBSY once the hub runs the /data copy. dcc-bus is the same binary invoked as a subcommand, so its daemons are restarted too or they keep the old inode. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
This Pull Request significantly improves the robustness and observability of the Z21 command station integration. The new reconnection logic, heartbeat mechanism, and health check endpoint address critical reliability issues and provide better insight into the station's status. The fix for the LAN_X_CV_READ DataLen is also a crucial correction.
| return nil | ||
| } | ||
|
|
||
| func (z *Z21Roco) doReconnect() { |
There was a problem hiding this comment.
🚨 [HIGH] The implementation of doReconnect(), heartbeatLoop(), and pingSerial() significantly improves the Z21 driver's robustness. This addresses the critical issue of the command station becoming unreachable without the system detecting it, ensuring continuous operation and automatic recovery.
| // Read: LAN_X_CV_READ (23 11) | ||
| func (z *Z21Roco) buildProgReadPacket(cv CV) []byte { | ||
| const dataLen, header = 0x000B, 0x0040 | ||
| const dataLen, header = 0x0009, 0x0040 |
There was a problem hiding this comment.
🚨 [HIGH] Correcting the dataLen constant for LAN_X_CV_READ from 0x000B to 0x0009 is a critical bug fix, aligning with the Z21 LAN protocol specification (§6.1).
| .PHONY: vet | ||
| vet: ## Run go vet against code. | ||
| go vet ./... | ||
|
|
There was a problem hiding this comment.
✨ [POSITIVE] The addition of deploy-hub targets provides a robust and atomic deployment mechanism for ARM64 binaries to a remote hub. This is a significant improvement for the project's operational efficiency and maintainability.
| Verifier: verifier, | ||
| }), | ||
| }) | ||
| } |
There was a problem hiding this comment.
✨ [POSITIVE] Integrating the StationHealth interface into the ws.Server configuration is a necessary and positive change. It allows the /healthz endpoint to accurately reflect the Z21 command station's reachability, enhancing system observability.
| // the daemon binds to loopback because the reverse proxy on | ||
| // loco-server already validates Origin). | ||
| AllowedOrigins []string | ||
|
|
There was a problem hiding this comment.
✨ [POSITIVE] The introduction of the StationHealth interface and its integration into the /healthz endpoint is a major improvement. This allows the health check to accurately report the command station's connectivity status, returning a 503 Service Unavailable with a clear station_unreachable code when the station is down. This significantly enhances the system's monitoring capabilities.
|
|
||
| type Z21Roco struct { | ||
| conn net.Conn | ||
| addr string |
There was a problem hiding this comment.
✨ [POSITIVE] The use of sync.RWMutex for conn and atomic.Bool for reachable, reconnecting, and broadcastsWanted is a good practice for ensuring thread safety and efficient concurrent access in the Z21 driver.
| return z.obsCh | ||
| } | ||
|
|
||
| func (z *Z21Roco) restoreBroadcasts() { |
There was a problem hiding this comment.
✨ [POSITIVE] The restoreBroadcasts() logic ensures that LAN_X_LOCO_INFO broadcasts are re-enabled after a reconnection, which is crucial for maintaining real-time state updates from the command station.
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
✨ [POSITIVE] Adding a dedicated test case TestBuildProgReadPacketDataLen to verify the DataLen for LAN_X_CV_READ is excellent. This confirms the fix for the protocol discrepancy and ensures its correctness.
| // SetTrackPower implements TrackPowerController via LAN_X_SET_TRACK_POWER_*. | ||
| func (z *Z21Roco) SetTrackPower(on bool) error { | ||
| if z == nil || z.conn == nil { | ||
| if z == nil || z.currentConn() == nil { |
There was a problem hiding this comment.
✨ [POSITIVE] Using z.currentConn() for SetTrackPower and write ensures thread-safe access to the underlying UDP connection, which is important for the new concurrent reconnection logic.
| if err != nil { | ||
| z.metrics.incr(&z.metrics.txErrors) | ||
| logrus.WithError(err).Warn("z21 command station: UDP write failed") | ||
| go z.doReconnect() |
There was a problem hiding this comment.
✨ [POSITIVE] Proactively calling z.doReconnect() on write failures in the write method further enhances the Z21 driver's resilience by immediately attempting to re-establish connectivity when communication issues arise.
1 participant
Summary
Dial(which always succeeds). dcc-bus kept serving and/healthzstayed 200.LAN_GET_SERIAL_NUMBER(~10s, first probe immediately), reconnects with 2s backoff, and re-sends loco-info broadcast flags on a new LAN session./healthzreturns 503station_unreachablewhileReachable()is false (Z21 only; LocoNet unchanged).LAN_X_CV_READDataLen is 0x0009 per Z21 LAN §6.1 (was 0x000B).Test plan
go test ./pkgs/loco/commandstation/ ./pkgs/bigfred/dcc-bus/ws//healthzis 200 and programming/drive still work/healthzbecomes 503station_unreachable/healthzreturns 200 without restarting dcc-busMade with Cursor