Handset pairing, sticky sessions, and remote status fix - #60
Conversation
Expose a stable product discriminator so handset probes do not rely on build metadata. Co-authored-by: Cursor <cursoragent@cursor.com>
Reuse login and WiThrottle StartPairing behind bounded IP/login rate limits, audit every outcome, and bind generated codes to the requesting device ID. Co-authored-by: Cursor <cursoragent@cursor.com>
Use canonical device keys, skip hidden command stations, return existing typed errors, scope audit events, and cover the public route without exposing PINs. Co-authored-by: Cursor <cursoragent@cursor.com>
Reuse unexpired Redis pairings on connect, treat unpaired drive as a pairing signal, restrict AllowAllVehicles to CanDrive, and sort the WiThrottle roster by DCC address. Co-authored-by: Cursor <cursoragent@cursor.com>
Lua TouchSeen rewrote empty vehicleIds/allowedAddrs arrays to {} in
Redis, which broke GET remotes/status unmarshalling. Tolerate {} on read
and omit empty scope tables before cjson.encode in pairing scripts.
Log loco-server HTTP 500 responses with error cause and stack in the
server log while keeping generic internal_error JSON for clients.
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 introduces several significant improvements across handset pairing, session management, and remote panel reliability. The changes address critical bugs, enhance security, and improve the overall user experience for both WiThrottle and Z21 clients. The comprehensive test coverage for these new features and fixes is commendable.
| // WiThrottle-only 6-digit pairing code (omitempty for other protocols). | ||
| PairingCode string `json:"pairingCode,omitempty"` | ||
| // ExpectedClientKey restricts completion to a pre-declared handset (optional). | ||
| ExpectedClientKey string `json:"expectedClientKey,omitempty"` |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The addition of ExpectedClientKey significantly enhances the security of WiThrottle pairing by allowing requests to be restricted to a specific client. This is a robust improvement.
| } | ||
| return w, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The custom UnmarshalJSON methods and the coerceLuaEmptyArrays function are critical fixes for handling Redis Lua cjson's behavior of encoding empty arrays as objects ({} instead of []). This directly addresses the reported 500 errors and greatly improves the robustness of data deserialization.
| func (r *Router) HandleSetFunction(ctx context.Context, actor Actor, resp Responder, p contract.LocoSetFunctionWire, _ string) Result { | ||
| vehicle, onLayout := r.roster.AllowedVehicle(p.Address) | ||
| if d := r.drive.CanDrive(actor.UserID, vehicle, onLayout); !d.Allowed { | ||
| _ = resp.SendLocoError(ctx, p.Address, d.Reason, "") |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: Sending a LocoError when a SetFunction command is unauthorized provides immediate and clear feedback to the client, improving the user experience and aiding in debugging authorization issues.
| remotes.RegisterGatewayFactory(withrottle.GatewayName, withrottle.NewGateway) | ||
| coordinator.RegisterPolicy(contract.RemoteProtocolWithrottle, remotes.ProtocolPolicy{ | ||
| IdleEvict: withrottle.IdleEvictAfter * time.Second, | ||
| StickyIdleEvict: contract.RemoteStickySessionIdle, |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: Setting StickyIdleEvict for the WiThrottle protocol policy ensures that paired WiThrottle sessions benefit from the extended 72-hour idle timeout, aligning with the sticky sessions feature.
| continue | ||
| } | ||
| delete(l.leases, addr) | ||
| l.dropLeaseBookkeepingLocked(addr) |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: Replacing delete(l.leases, addr) with l.dropLeaseBookkeepingLocked(addr) is a good refactoring that ensures all associated bookkeeping (e.g., perUser counts) is correctly updated when a lease is dropped. This prevents inconsistencies and improves resource management.
|
|
||
| // luaDropEmptyArrays omits empty Lua tables before cjson.encode so Redis | ||
| // lua-cjson does not rewrite vehicleIds/allowedAddrs from [] to {}. | ||
| const luaDropEmptyArrays = ` |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The luaDropEmptyArrays script snippet and its integration into touchSeenScript, touchSeenBatchScript, and updateSessionScopeScript are crucial for preventing Redis Lua cjson from encoding empty arrays as objects. This directly fixes the root cause of the 500 errors and ensures data integrity.
| return active, true, nil | ||
| } | ||
|
|
||
| // GetActiveByClientKeyTTL loads a paired session and its remaining Redis TTL. |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The GetActiveByClientKeyTTL method is a valuable addition for sticky sessions, allowing the system to retrieve an active session along with its remaining Redis TTL, which is essential for managing session longevity.
| if err != nil { | ||
| return contract.RemoteSessionWire{}, false, "", err | ||
| } | ||
| if req.ExpectedClientKey != "" && req.ExpectedClientKey != clientKey { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The check if req.ExpectedClientKey != "" && req.ExpectedClientKey != clientKey in PairViaWithrottleCode implements a critical security feature, ensuring that only the intended WiThrottle client can complete a pairing request.
| defaultClientsPublishMin = 2 * time.Second | ||
| defaultIdleEvict = 60 * time.Second | ||
| defaultStickyIdleEvict = 30 * time.Minute | ||
| defaultStickyIdleEvict = 72 * time.Hour |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: The change to defaultStickyIdleEvict from 30 minutes to 72 hours, along with the simplified bucket initialization and SessionExpiresAt logic, fully implements the sticky sessions feature. This provides a significantly improved and more persistent experience for paired handsets.
| } | ||
| } | ||
| c.SubscribedLocos = kept | ||
| if c.LastActiveLoco == addr { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE: Clearing LastActiveLoco when a loco is unsubscribed is a good cleanup, preventing the system from holding onto stale information and ensuring accurate state management.
keskad
changed the title
… idle Restore IPStickiness-gated idle eviction for Z21, drop presence-only sweeps for WiThrottle, harden public handset pairing, and add station-local track power plus remotes UI fixes from review. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Overall, this Pull Request introduces significant improvements across several key areas, including enhanced handset pairing, robust session management, and better error handling. The fixes for Redis Lua cjson encoding issues are critical for stability, and the new features for WiThrottle and Z21 protocols greatly expand functionality and user experience. The comprehensive test coverage for these changes is commendable.
| // RemoteStickySessionIdle is how long a paired handset session stays | ||
| // in Redis without activity (last-seen idle window). TouchSeen refreshes | ||
| // this TTL so a driving throttle keeps its pairing for days. | ||
| RemoteStickySessionIdle = 72 * time.Hour |
There was a problem hiding this comment.
✨ [POSITIVE] PRIORITY:HIGH. The change of RemoteStickySessionIdle from 30 minutes to 72 hours is a significant functional change that directly implements the "sticky sessions" feature. This greatly improves the user experience by reducing the frequency of re-pairing.
| // WiThrottle-only 6-digit pairing code (omitempty for other protocols). | ||
| PairingCode string `json:"pairingCode,omitempty"` | ||
| // ExpectedClientKey restricts completion to a pre-declared handset (optional). | ||
| ExpectedClientKey string `json:"expectedClientKey,omitempty"` |
There was a problem hiding this comment.
✨ [POSITIVE] PRIORITY:MEDIUM. The addition of ExpectedClientKey to RemotePendingWire is a good security enhancement, allowing pairing requests to be restricted to a specific handset. This helps prevent unauthorized pairing attempts.
| } | ||
| return w, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
✨ [POSITIVE] PRIORITY:HIGH. The custom UnmarshalJSON methods and the coerceLuaEmptyArrays function are critical fixes for the 500 on GET …/remotes/status issue. This directly addresses the problem of Redis Lua cjson encoding empty arrays as objects, which caused unmarshalling failures. Excellent work on identifying and fixing this subtle bug.
| @@ -0,0 +1,84 @@ | |||
| package contract | |||
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE. The new test file provides excellent coverage for the UnmarshalJSON fixes and ensures that the system correctly handles Lua cjson's empty object representation for arrays, preventing the 500 errors.
| }) | ||
| } | ||
|
|
||
| // TriggerStationTrackPowerOn turns main-track power on for this daemon's |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE. The TriggerStationTrackPowerOn method adds a valuable new capability, allowing handsets to control the track power of their connected command station. This enhances the overall control available to users.
| return active, true, nil | ||
| } | ||
|
|
||
| // GetActiveByClientKeyTTL loads a paired session and its remaining Redis TTL. |
There was a problem hiding this comment.
✨ [POSITIVE] PRIORITY:MEDIUM. The GetActiveByClientKeyTTL method is a useful addition for retrieving session information along with its remaining TTL. This can be valuable for UI elements displaying session expiry and for internal logic that needs to react to session longevity.
| if err != nil { | ||
| return contract.RemoteSessionWire{}, false, "", err | ||
| } | ||
| if req.ExpectedClientKey != "" && req.ExpectedClientKey != clientKey { |
There was a problem hiding this comment.
✨ [POSITIVE] PRIORITY:MEDIUM. The check for req.ExpectedClientKey in PairViaWithrottleCode enforces the security measure of restricting pairing completion to a pre-declared handset. This is a good step towards hardening the pairing process.
| } | ||
| } | ||
|
|
||
| func TestWithrottlePairingCanRestrictExpectedClient(t *testing.T) { |
There was a problem hiding this comment.
✨ [POSITIVE] POSITIVE. The new tests for ExpectedClientKey and the comprehensive tests for Lua empty object handling (TestGetActiveAcceptsLuaEmptyObjectVehicleIds, TestTouchSeenAllowAllDoesNotWriteObjectVehicleIds, TestUpdateSessionScopeEmptySlicesDoNotWriteObjectVehicleIds) are excellent. They provide strong validation for the security and critical bug fixes related to Redis JSON serialization.
| // TODO(withrottle): on expiry, emit a handset emergency stop before | ||
| // evicting instead of a plain idle evict. | ||
| HeartbeatTimeout time.Duration | ||
| // SweepKeepsPairing makes the idle sweep drop only in-process presence and |
There was a problem hiding this comment.
✨ [POSITIVE] PRIORITY:HIGH. The introduction of SweepKeepsPairing in ProtocolPolicy and the refactoring of runSweeper to use dropPresence for protocols like WiThrottle are fundamental to the sticky sessions feature. This allows TCP-based protocols to maintain their Redis pairing across disconnects, significantly improving reliability and user experience.
| c.publishIfDirty(ctx) | ||
| } | ||
|
|
||
| // idleEvictWindow is how long cl may stay silent before the sweep acts. |
There was a problem hiding this comment.
✨ [POSITIVE] PRIORITY:MEDIUM. The idleEvictWindow and pairingExpiryWindow helper functions are good encapsulations of complex logic related to session timeouts and expiry display. This improves readability and maintainability of the coordinator's sweep logic.
1 participant
Summary
This PR bundles five related changes around handset pairing (LongFred / WiThrottle), session persistence, and reliability of the remotes panel in the browser.
BigFred identification in
/api/v1/versionThe server returns a
product: "bigfred"field in the public version endpoint.UX impact: The handset (LongFred) can reliably verify it is connecting to BigFred, not another application at the same address. Fewer false "server not found" errors and fewer attempts to connect to the wrong backend.
Handset pairing without logging in via the browser
New public endpoints:
POST /api/v1/remotes/handset-pairing(pairing code) andPOST /api/v1/remotes/handset-session(check whether the session is still alive). The handset authenticates with login and PIN, same as the web app.UX impact: You can pair a handset with the layout without opening BigFred in the browser — login, PIN, and device number on the handset are enough. After a handset restart, the app can check whether pairing is still valid instead of asking for a new code every time.
Hardened and more secure pairing
Rate limits (IP and login), correct device keys, skipping hidden command stations, audit events without PIN leakage, better error messages in the UI (i18n).
UX impact: Harder to accidentally lock yourself out with wrong PINs (rate limit), and on errors you see a meaningful message instead of a generic "internal error". Administrators see what happened in the audit log without the password being exposed.
Sticky sessions and roster limited to driveable vehicles
The Redis session no longer expires after a few minutes of inactivity — it refreshes on handset activity (~72 h without activity). On reconnect, the handset reuses existing pairing. "All vehicles" means vehicles the user can drive (owned + borrowed), not the entire layout. The loco list on the handset is sorted by DCC address. Selecting a loco without prior pairing may trigger pairing.
UX impact: After pairing you don't have to enter the code constantly — the session "sticks" during operation and after a short break. The loco list shows only what you can actually drive, in a predictable order. Less frustration with "allow all", which previously could show locos outside your permissions.
Fix for 500 on
GET …/remotes/statusand logging of internal errorsRedis Lua when refreshing sessions was replacing empty
vehicleIdslists with{}, which broke reading in loco-server. Added tolerant reading and Lua script fixes. HTTP 500 errors are logged with stacktrace in the server console; the client still only getsinternal_error.UX impact: The remotes panel in the browser (Tauri / web) works reliably after active handset use — pairing status loads instead of showing a silent 500 error. For operators: when something goes wrong, the server logs contain the full cause, so issues can be fixed faster without guessing.
Test plan
GET /api/v1/versionreturnsproduct: "bigfred"GET …/remotes/statusin browser → 200, no 500GET …/remotes/clientsstill works alongside statusgo test ./pkgs/bigfred/contract/ ./pkgs/bigfred/remotepairing/ ./pkgs/bigfred/server/http/