================================================================================================ PROPOSAL - [MINOR] functional - WebClientProtocol.onClose Heartbeat expiry reuses StatusCode.SHUTDOWN and is logged as a browser tab close ================================================================================================ REVIEW FINDING (abridged) "the heartbeat teardown reuses StatusCode.SHUTDOWN (1001), which onClose already maps to the fixed description 'SHUTDOWN (browser tab/window closed)' at Level.SEVERE [...] Every heartbeat expiry is therefore logged as a browser tab close that did not happen, immediately after the timer task's own LOG.severe for the same event - two SEVERE lines for one peer disconnect, one factually wrong, on the path support engineers triage - and a heartbeat expiry becomes indistinguishable from a genuine tab close by status code alone. [...] Behaviourally harmless [...] so a distinct code (SERVER_ERROR, as onError already uses, or an application 4xxx code) plus a matching case would keep the diagnostics honest at no functional cost." FILES src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js (log-name table only) ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ WebClientProtocol.java:1733 closes with StatusCode.SHUTDOWN, and onClose maps that code to a fixed description at :697-702: case StatusCode.SHUTDOWN: // RFC 6455 7.4.1: 1001 "Going Away" - browser tab/window closed or // navigated away, so technically not severe, however for FWD this complicates // the business code flow, so marked as severe codeDesc = "SHUTDOWN (browser tab/window closed)"; break; So a heartbeat expiry produces: SEVERE No websocket message received for 91234 ms (deadline=90000 ms), the peer is gone ... SEVERE WebSocket session closing: statusCode=1001 SHUTDOWN (browser tab/window closed), reason='client heartbeat stopped'. Two SEVERE lines for one disconnect, the second one asserting an event that did not occur. 1001 is also semantically wrong in the RFC sense: "Going Away" describes the *endpoint that sends the frame* going away, and here the server is not going anywhere - it is evicting a peer. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ 2.1 A distinct application close code -------------------------------------- Use the RFC 6455 4000-4999 private-use range rather than SERVER_ERROR (1011): the peer's silence is not a server error, and 1011 is already onError's code (WebClientProtocol.java:802), so reusing it would recreate the same ambiguity one code over. Add, next to the other keep-alive constants: + /** + * The web socket close code sent when the peer stops answering the keep-alive heartbeat. + *

+ * A private range code (RFC 6455 7.4.2) rather than SHUTDOWN, which means "this endpoint is going + * away" and is already mapped to a browser tab close, or SERVER_ERROR, which onError already uses. + * The distinction matters operationally: a heartbeat expiry and a tab close are triaged + * differently, and by status code alone they would otherwise be indistinguishable. + */ + private static final int SESSION_HEARTBEAT_LOST = 4001; and at the teardown site: - session.close(StatusCode.SHUTDOWN, "client heartbeat stopped", Callback.NOOP); + session.close(SESSION_HEARTBEAT_LOST, "client heartbeat stopped", Callback.NOOP); 2.2 Map it in onClose ---------------------- In the switch (WebClientProtocol.java:691-729), before the default: + case SESSION_HEARTBEAT_LOST: + codeDesc = "HEARTBEAT_LOST (the client stopped answering the keep-alive)"; + break; Placement note: the case labels are StatusCode constants; SESSION_HEARTBEAT_LOST is a private static final int, which is a compile-time constant and therefore a legal case label. Verify no other case in the switch collides with 4001 (none do - all are 10xx). Behaviour is preserved: the only status-sensitive logic is `long wd = statusCode == StatusCode.NORMAL ? 0 : wdtimeout;` (:753), and 4001 is not NORMAL, so the watchdog is armed exactly as before. logLevel also stays SEVERE, as for every non-NORMAL code. 2.3 Remove the duplicate SEVERE -------------------------------- Since onClose now logs an accurate SEVERE line for this event, the timer task's own line becomes the *detail* rather than the alarm. Demote it so one disconnect produces one SEVERE: - LOG.severe(String.format( + LOG.warning(String.format( "No websocket message received for %d ms (deadline=%d ms, ...), the peer is gone " + "or its page is no longer running; closing the session!", ... If the diagnostic detail is considered important enough to keep at SEVERE, leave it - the finding's substantive complaint is the factually wrong description, not the duplication. Pick one; do not leave two SEVERE lines where one of them says "browser tab/window closed". 2.4 Client-side log name ------------------------- p2j.socket.js builds its close-event log message from a name table (`ceCodes_names[(event.code).toString(10)]`, p2j.socket.js:5638 and :5645), so an unmapped 4001 prints `undefined`. Add to the ceCodes table (p2j.socket.js:781-...), after the standard codes: + /** + * FWD private code: the server closed the connection because this page stopped answering the + * keep-alive heartbeat. See WebClientProtocol.SESSION_HEARTBEAT_LOST. + */ + Heartbeat_Lost : 4001, Keep the two constants' comments pointing at each other; they are a wire contract across two languages with nothing to enforce agreement. ------------------------------------------------------------------------------------------------ 3. WHY 4001 IS SAFE ON THE WIRE ------------------------------------------------------------------------------------------------ - RFC 6455 7.4.2 reserves 4000-4999 for private use between endpoints; Jetty transmits it without complaint (it is neither in the "not transmittable" set - 1005, 1006, 1015 - nor outside the valid range). - Browsers surface it verbatim as CloseEvent.code, and p2j.socket.js does not branch on the code for anything except the log message (verified: the only uses are the two log lines above). - No FWD code compares against StatusCode.SHUTDOWN, so nothing loses a match. ------------------------------------------------------------------------------------------------ 4. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-major-performance-1 the composed tick body uses SESSION_HEARTBEAT_LOST defined here proposal-issue-minor-security-1 extends the same log line with the grace-limit figures Independent of all of them in principle - 2.1 and 2.2 can land alone - but the constant is referenced by the composed listings, so land this one first or adjust those hunks. ------------------------------------------------------------------------------------------------ 5. VERIFICATION ------------------------------------------------------------------------------------------------ 1. Force a heartbeat expiry (SIGSTOP the browser, or blackhole its route) and check the server log contains exactly one SEVERE, reading "statusCode=4001 HEARTBEAT_LOST (the client stopped answering the keep-alive), reason='client heartbeat stopped'". 2. Close a browser tab normally: still "statusCode=1001 SHUTDOWN (browser tab/window closed)". 3. Check the JS console on a forced expiry (a second tab of the same session, or the reconnect attempt) reads "Connection closed with code = Heartbeat_Lost", not "undefined". 4. Confirm the watchdog is still armed after a 4001 close (log line "Stopping pushWorker, webWorker and starting watchdog with timeout=120000").