Sergey, can you review the changes?

I have done the testing with the ping (checked network interaction in the dev tools with some parameters which allows to monitor the behavior easily), including connection restoration.

Teodor Gorghe wrote:

I will investigate!

There is another issue like ping pong cadence related to new hard coded constants and java web client logic. Please use Claude Code to assist in this work.

Fix proposals for the r16675 code review¶

Wrote 11 proposals in .tmp/, one per non-style finding of .tmp/code-review.textile. The two [MINOR] style findings - the GuiWebDriver.init blank line and the p2j.sso_reauth.js separator width - were skipped as requested, though the first is incidentally removed by proposal-issue-major-functional-3.

Naming note: the review has no severe findings and one security finding, so that file uses -security- in the category slot rather than forcing it into functional|performance. Numbering restarts per severity+category pair, in review order.

Two things worth knowing before acting on these.

The five keep-alive findings are one rework, not five patches¶

Their fixes interact - one review bullet asks for outbound-write evidence to extend tolerance while another warns that outbound evidence must not substitute for peer liveness. That is resolved with two clocks: lastDataNanos stays inbound-only and sets the hard deadline; completed outbound writes (a new PushMessagesWorker.getTransmittedCount(), which unlike getQueueSize() sees the in-flight message) postpone it by a bounded, absolutely-capped grace. Each file owns a distinct mechanism, proposal-issue-major-performance-1 carries the composed tick body, and each states its dependencies. In particular, major-functional-1 must not land without minor-security-1 - the grace would be unbounded.

Verification is against the code, not the review text¶

Confirmed in the tree: the ConfigItem constructor contract behind the wrong key name; the three-reader split on pingPongInterval (WebPageHandler.java:251 unclamped vs. the two drivers); all three addWebSocketHandler registrations plus ReportWebServer.java:148 setting maxIdleTime to 36000000; that client/web/ slash notation exists only on the three lines this diff added; and that p2j.keyboard.js:319-336 already contains the exact listener-removal code needed for the SSO fix - inlined in init() with no way to call it, which is why extracting it is a refactor rather than new logic.

Nothing was compiled or run, so the code hunks are proposals rather than verified patches. Each file ends with concrete verification steps, and several note that the interval/deadline arithmetic has no test coverage at all today and would be worth extracting into a testable static helper.

The following changes

=== modified file 'src/com/goldencode/p2j/ui/client/driver/web/res/p2j.js'
--- old/src/com/goldencode/p2j/ui/client/driver/web/res/p2j.js    2026-05-26 07:34:53 +0000
+++ new/src/com/goldencode/p2j/ui/client/driver/web/res/p2j.js    2026-07-30 10:44:18 +0000
@@ -4,7 +4,7 @@
 **
 ** Copyright (c) 2014-2026, Golden Code Development Corporation.
 **
-** -#- -I- --Date-- ------------------------------Description----------------------------------
+** -#- -I- --Date-- ---------------------------------------Description----------------------------------------
 ** 001 MAG 20140110 First version.
 ** 002 MAG 20140204 Prevent back navigation.  Disable browser context menu.
 ** 003 MAG 20140702 Add tty.clipboard module.
@@ -90,6 +90,7 @@
 **                  changeDisplayStyle/changeVisibilityStyle. Removed unnecessary async from
 **                  overrideDialogStyle/overrideTitleStyle. Added cachedTitle guard to setTitle.
 **                  Added buttonsDiv null guard to setButtons.
+** 035 TG  20260728 Drove the loading progress bar easing from elapsed time instead of a tick count.
 */
 /*
 ** This program is free software: you can redistribute it and/or modify
@@ -686,7 +687,9 @@
          }
          loadingProgressBar.set({"value" : value});
          var v0 = loadingProgressBar.get("value");
-         var time = 1;
+         // the easing below is driven by elapsed wall-clock time rather than by a tick count, so a
+         // throttled or delayed timer still yields the same curve instead of advancing more slowly
+         var started = Date.now();
          if (progressTimer)
          {
             clearInterval(progressTimer);
@@ -722,8 +725,8 @@
                         progressTimer = null;
                         return;
                      }
+                     var time = Math.max(1, (Date.now() - started) / 100);
                      var v = v0 + (v1 - v0) * ( 1 - 1 /(1 + Math.log2(time)));
-                     time++;
                      loadingProgressBar.set({"value" : v});
                   },
                   100);

Code review and fix proposals - r16675 (server-driven websocket keep-alive)¶

Scope¶

Reviewed the branch diff against trunk: 15 files, ~830 added and ~300 removed lines. The revision does three things: it moves the websocket keep-alive heartbeat from the JS client to the server (new MSG_SERVER_PING, a server-side Timer in WebClientProtocol, dead-peer detection closing with private code 4001); it replaces websocket continuation frames with application-level MSG_CHUNK messages reassembled in the browser; and it hardens the SSO re-auth countdown (deadline-based ticks, a grace window, and a page reload on expiry in place of the deleted showSessionExpiredPage).

Modified areas: com.goldencode.p2j.ui.client.driver.web (5 java + 3 js), com.goldencode.p2j.ui.client.gui.driver.web (2 java + 1 js), com.goldencode.p2j.ui.client.chui.driver.web (1 java), com.goldencode.p2j.web (1), com.goldencode.p2j.util (1), com.goldencode.p2j.main (1).

Domains reviewed: fwd-web-gui-driver, fwd-chui-driver-streams, fwd-server-infrastructure, fwd-web-login-process.

Method¶

Seven parallel review passes (style, security, performance, and one functional pass per matched domain) produced 21 candidate findings. Each of the 16 non-style findings was then handed to an independent adversarial challenge pass instructed to identify the concrete trigger in the current code, or reject. Ten were confirmed, six rejected as theoretical. The challenge pass also downgraded three findings and rewrote one whose causal narrative was wrong.

Nothing was compiled or run. The proposals are code hunks with verification steps, not verified patches.

Confirmed findings¶

Five [MINOR] style findings (javadoc line length, two blank-line issues, header separator width in five files, one stray blank line) were reported without challenge, as style is judged against the rulebook rather than reachability.

The critical finding¶

queueChunk validates each piece against the current session and keeps no record of which session the transfer began on. Two independent routes carry a transfer's tail onto a later connection: the push queue survives the swap, because startPushWorker reuses the existing worker and onConnect never calls stopPushWorker (the sole caller is onClose's active-session branch, which the session != this.session guard skips for a replaced session); and the producer thread still inside the streaming loop simply observes the new session on its next call. This is the designed ordering on a half-open link - the page gives up after maxLostPings periods, roughly half the server idle timeout, so onConnect(new) precedes onClose(old) - and the endpoint really is the same object across a reconnect, since GenericWebSocketCreator returns the one GuiWebSocket instance for every upgrade.

On the page, collectChunk keys only on the transfer id with no notion of where a transfer starts, so an arriving tail begins a fresh reassembly and the piece carrying the last-flag re-enters messageHandler with whole[0] set to raw pixel or font data. 0x85 is MSG_QUIT, which calls doRedirectToLogoutPage() - a spurious logout roughly one time in 256.

Rejected findings¶

Recorded so they are not re-raised. Each was rejected after an attempt to construct a concrete trigger failed.

Fix proposals¶

One file per non-style finding, in .tmp/. Each states root cause with verified line references, concrete code hunks, dependencies, and verification steps.

The three chunk findings are one change¶

The critical finding, the major one, and minor-functional-1 all end at the same place: a piece that arrives with no matching reassembly in progress is dispatched with an arbitrary payload byte as its message type. The common remedy is a first-piece flag on the chunk header, so the peer refuses to begin a reassembly without it.

   /** MSG_CHUNK flags bit 1: this piece opens the transfer. */
   public static final byte CHUNK_FLAG_FIRST = 0x02;

   if (!parts)
   {
      if (!first)
      {
         p2j.logger.error("Discarding orphaned message piece, transfer id " + id);
         return null;
      }
      parts = chunkTransfers[id] = [];
   }

The other two proposals state this as a hard dependency and are not safe to land alone: clearing state on close, or abandoning an oversized transfer, both leave a suffix that then starts a new reassembly - the same defect wearing a different hat. If only one change from this review is taken, take the first-piece flag.

The critical proposal additionally binds each transfer to a connection generation, which stops the producer feeding a dead transfer, and optionally purges queued output on session replacement.

The three SSO findings are one edit¶

minor-functional-2, -3 and -4 modify the same five lines of the countdown expiry branch and share one internalReload flag exported from p2j.socket. That flag makes onpagehide skip the quit-on-reload sessionStorage write and makes onbeforeunload skip the leave-confirmation, which together are what turn the reload into the re-attach the code comment already claims it performs. Apply them as a single change.

Risks worth reading before applying¶

Two proposals carry real risk and each offers a lower-risk variant alongside.

• critical-functional-1 section 2.3 - purging the push queue on session replacement drops draw output the peer has not yet received. onConnect resynchronizes through initRemoteClient, but this is the one part that changes behaviour for non-chunk traffic. Sections 2.1 and 2.2 are sufficient for correctness without it.
• minor-performance-1 section 2.1 - the in-flight chunk semaphore introduces permit-lifecycle obligations on teardown and abandon paths, and a leak would hang the producer, which is worse than the footprint it fixes. Section 2.2 is an approximate fix at a fraction of the complexity, and is the recommended option unless a measurement shows the footprint matters.

minor-functional-2 section 2.1 also puts a deliberate hole in the long-standing quit-on-reload invariant. The hole is narrow - one caller, set immediately before an unload the page itself requested - but a future caller that sets the flag without unloading would leave a GUI page that no longer arms quit-on-reload.

Housekeeping¶

.tmp/ also contains proposal-issue-major-functional-2.txt, -major-functional-3.txt, -major-performance-1.txt and -minor-security-1.txt from an earlier review round. That round has already been applied - the code now has SESSION_HEARTBEAT_LOST = 4001 rather than a reused StatusCode.SHUTDOWN, a volatile pushWorker, client:web: key notation and clamping in WebPageHandler, which were exactly its subjects. Those four filenames imply membership in the current set and do not belong to it; delete or move them aside.

Fix proposals for the current r16675 code review¶

Wrote 10 proposals in .tmp/, one per non-style finding of .tmp/code-review.textile. The five [MINOR] style findings (javadoc line length, blank lines, header separator width) were skipped as requested.

Numbering restarts per severity+category pair, in review order.

Stale files from the previous round¶

.tmp/ also contains proposal-issue-major-functional-2.txt, -major-functional-3.txt, -major-performance-1.txt and -minor-security-1.txt, which belong to an earlier review round (all written 2026-08-12 ~09:2x) and are not part of this set. That round appears to have been applied already - the code now has SESSION_HEARTBEAT_LOST = 4001 rather than a reused StatusCode.SHUTDOWN, a volatile pushWorker, client:web: key notation, and clamping in WebPageHandler, which were exactly its subjects. Delete them or move them aside before working from this directory; their filenames imply membership in the current set and they do not belong to it.

The chunk findings are one change, not four¶

critical-functional-1, major-functional-1 and minor-functional-1 all end at the same place: a piece that arrives with no matching reassembly in progress is dispatched with an arbitrary payload byte as its message type. The common remedy is the CHUNK_FLAG_FIRST bit specified in critical-functional-1 section 2.2 - the peer refuses to begin a reassembly without it. The other two proposals state that dependency and are not safe to land alone: clearing state on close, or abandoning an oversized transfer, both leave a suffix that then starts a new reassembly, which is the same defect wearing a different hat.

Likewise minor-functional-2, -3 and -4 edit the same five lines of the SSO expiry branch and share the internalReload flag; apply them as one edit.

Verification is against the code, not the review text¶

Confirmed in the tree while writing these: startPushWorker reuses the worker and onConnect never calls stopPushWorker, so the queue genuinely survives a session swap; closeWebSocketSafely nulls ws synchronously before the close event lands, so handleCloseEvent's this != ws early return really does skip the new cleanup; onpagehide writes the exitTheApplication sessionStorage object on a scripted reload and isRequiredToRedirect honours it for the whole watchdogTimeout window; getQueueSize has no caller anywhere in src/, test/ or tools/; and cleanupReauthUi is the only code path that closes reauthPopup.

Nothing was compiled or run, so the code hunks are proposals rather than verified patches. Each file ends with concrete verification steps. Two carry real risk and say so: critical-functional-1 section 2.3 (purging queued output on reconnect drops draw output not yet delivered) and minor-performance-1 section 2.1 (a permit leak would hang the producer, which is worse than the footprint it fixes) - both offer a lower-risk variant alongside.

I changed the task state to WIP - 90%.

Sergey Ivanovskiy wrote:

There is a hard-coded constant that should be fixed:
[...]
It looks like WebConfigurationConstants.PING_PONG_INTERVAL can be used here instead.

Right.
Done in 11645a/r16698.

The committed rev 16699, 16700 looks good but the AI review found 4 major issues that seems worth to be taken into account.

Task #11645 - code review of r16696..r16699¶

Scope reviewed¶

Branch 11645a, diff r16695..r16699 - r16695 is the most recent trunk-nicked ancestor, so this is the full branch delta. 15 files, 2437 diff lines. The review was performed against r16699; the style fixes it produced are committed as r16700.

Subject of the change: the websocket keep-alive heartbeat moves from the JS client to the server (new MSG_SERVER_PING), large messages are reframed onto MSG_PARTIAL in both directions with first/last flags instead of native continuation frames, and the SSO re-auth expiry becomes an internal reload.

Method¶

Seven parallel review passes - style, security, performance, and one functional pass per matched domain (fwd-web-gui-driver, fwd-web-login-process, fwd-server-infrastructure, fwd-chui-driver-streams) - followed by 17 independent adversarial challenge passes, one per non-style finding. Each challenger was required to identify a concrete trigger in the current code or reject the finding.

Results at a glance¶

No MAJOR security or performance finding survived the challenge pass: the path-traversal finding was scoped down because its sink is pre-existing and untouched here, and the discardQueued accounting finding was downgraded because its impact is bounded.

Style fixes applied - committed as r16700¶

All 10 style findings are fixed. Verified: 0 violations remain among the lines this branch adds, both JS files pass node --check, and no double blank lines involving added lines remain.

• WebClientProtocol.java:1860 - javadoc line was 130 characters; rewrapped to four lines under 110.
• WebClientProtocol.java:1055 - wrapped parameter continuation had 30 leading spaces where 32 are needed to align with int generation.
• WebClientProtocol.java:1968 - two consecutive blank lines before stopKeepAlivePing's javadoc.
• WebClientProtocol.java:312 - added line was whitespace-only rather than empty (found by a second scan pass; the first missed whitespace-only lines).
• WebClientMessageTypes.java:501 - two consecutive blank lines before MSG_SERVER_PING's javadoc.
• p2j.socket.js:644-648 - the new MSG_PARTIAL wire-format comment was orphaned above the unrelated MSG_INVALIDATE_SELECTION field, with a whitespace-only line after it; moved onto MSG_PARTIAL itself, replacing the stale one-line comment there, and completed to document flags bit 1 (first piece) as well as bit 0 (last).
• p2j.socket.js:1897 - for( was missing the required space after the keyword.
• p2j.socket.js:7032 - whitespace-only line plus a double blank line after beginInternalReload.
• p2j.sso_reauth.js:435-452 - cleanupReauthUi's javadoc, with its @param removeOverlay, had been separated from its subject by the newly inserted closeReauthPopup; moved closeReauthPopup above the javadoc so each comment sits on its own subject.
• p2j.sso_reauth.js:16 - history-entry line was 113 characters; rewrapped onto a continuation line.

History entries and copyright years were checked across all 15 files and are correct - every file has an entry, sequence IDs are successive, and where a file gained several entries only the first carries the ID.

MAJOR findings¶

Both are regressions introduced by this branch, and both have a written proposal.

• [MAJOR] functional p2j.sso_reauth handleSsoReauth - the expiry tick destroys the server-driven logout redirect. SsoTokenManager.handleInvalidToken arms forceLogout at SsoTokenManager.java:518 before calling sendSsoReauth at :539, while the module sets reauthDeadline only on receipt, so the server always kills the session first and the client tick always fires inside the shutdown. Nothing cancels countdownInterval on that path - cleanupReauthUi is reachable only from this module's own handlers, and there is no hook on MSG_SHUTTINGDOWN, MSG_QUIT or doRedirectToLogoutPage - so the tick runs beginInternalReload() + window.location.reload() mid-handshake. The MSG_SHUTTINGDOWN ack is lost so quit() burns the full clientResponseTimeout, the pending window.top.location.replace(logoutPage) is aborted, and the reloaded iframe points at an embedded server shutdownServer() has just torn down. The user lands on a browser network-error page instead of the login page. Before this branch the expiry path only replaced the document body, leaving the socket module intact to finish the redirect. See proposal-major-functional-1.txt.
• [MAJOR] functional WebClientProtocol onConnect - the new pushWorker.discardQueued() at :480-486 throws away all pending server-to-browser output, justified by the comment "initRemoteClient below resyncs", but GuiWebDriver.initRemoteClient() is literally a no-op at GuiWebDriver.java:1033-1036; only ChuiWebSimulator resyncs. Trigger: a half-open socket where the browser's silence watchdog reconnects before Jetty delivers onClose, so onConnect takes its session-replacement branch with a full backlog. The page was never reloaded, so the browser sends MSG_PING_PONG not MSG_PAGE_LOADED and nothing requests a repaint - the canvas is left permanently stale. Note the discard has effect only on this path (elsewhere pushWorker is null), so it takes effect exactly where it is unsafe, and the cross-generation hazard it guards against is already covered twice by queuePartial's generation check and the client's resetChunkTransfers(). See proposal-major-functional-2.txt.

MINOR findings¶

Full text with verified triggers is in code-review.textile. Summarised here by area.

Reconnect and session-replacement races - one root cause, four faces¶

Five of the seven review passes converged independently on onConnect's session-replacement branch. It runs on the Jetty thread while the previous session's webWorker and the collector's asynchIOExecutor are still live, because startWebWorker no-ops when the worker exists and the replaced session's onClose is skipped by the session != this.session gate.

• WebClientProtocol.MessagesCollector.reset - clears the plain unsynchronized HashMap@s @partialMessages and payloadMessagesTypes while two other threads mutate them; a queued AppendMessageTask then gets null from partialMessages.get(msgId) and NPEs on channel.write, and the catch handles only IOException so submit()'s FutureTask swallows it - a silently truncated upload. Because nextMsgId is page-scoped and restarts at 0, a stale queued task can also write into the new page's transfer id 0; PARTIAL_FLAG_FIRST blocks that through processPartialMesssage but not through the executor queue.
• WebClientProtocol.onConnect - reads the non-volatile, unguarded collector field from the Jetty thread while it is lazily assigned on the webtaskworker thread via the one MSG_PARTIAL switch case that never takes lock. pushWorker was made volatile in this same change; collector was not. A reconnect can see null, skip the reset, and silently leak the open @FileChannel@s and temp files the reset exists to drop.
• PushMessagesWorker.discardQueued - queuedBytes.set(0L) races the worker's in-flight send, leaving the counter negative by one message size, and messages.offerFirst runs after messages.clear() so a message from the superseded generation survives the discard. Downgraded from MAJOR: one worker thread means at most one message races per discard, every discard re-bases with an absolute set, and pieces are capped at 64 KB - so the 32 MB guard is perturbed by tens of KB, not progressively raised. See proposal-major-performance-1.txt.
• WebClientProtocol.queuePartial - an abandoned streamed transfer is dropped with no notification to the waiter, so GuiWebSocket.createFont blocks forever: waitForResult(msgId) passes timeout 0, which is implemented as "indefinitely", and nothing releases pending waiters on reconnect or close. Reachable through FontManager's lazy cache-miss path for any font under deploy/server/fonts. The same hang applies to non-streamed sendBinaryMessage + waitForResult(msgId) pairs - timeout 0 waits are systemically unsafe across a reconnect.

Security¶

• GuiWebSocket.processChannel - the MSG_FILE_UPLOADING filename reaches Paths.get() and Files.newByteChannel with no normalize(), no getFileName() and no containment check, and tmpDir ends with a separator so concat yields a traversable prefix. This is a real privilege-boundary crossing, not a user writing their own files: the spawned web client runs server-side, its websocket endpoint has no Origin check, and under OS_USER_OVERRIDE every user's client runs as the shared DEFAULT_OS_USER. FWD's own UploadHandler:231-233 already does the missing check. Scoped MINOR for this diff because the sink and its two twins are pre-existing and untouched - this branch only reworks the delivery route. See proposal-major-security-1.txt; raise as its own ticket.
• WebClientProtocol.MessagesCollector.processPartialMesssage - inbound reassembly has no aggregate size cap and no per-transfer expiry, the mirror image of the MAX_TRANSFER_BYTES / MAX_CHUNK_BYTES caps this same branch added on the JS side. A page that sets PARTIAL_FLAG_FIRST and never sets PARTIAL_FLAG_LAST grows a temp file in the shared java.io.tmpdir until ENOSPC; onClose does not reset the collector, so while the socket is held open the window is attacker-controlled. Files are 0600 and DELETE_ON_CLOSE, so nothing survives process death. Related: processBinaryMessage gates on length > PARTIAL_HEADER_SIZE, so a header-only 6-byte piece is dropped even carrying PARTIAL_FLAG_LAST - and the Java sender emits exactly such a terminator, so the guard should be >=.

Performance¶

• WebClientProtocol.sendBinaryMessageStreamed - PARTIAL_PAYLOAD_SIZE is 64 KB where the previous path used 1 MB with a reused buffer, so a 3840x2160 image becomes 508 messages instead of 33 frames, each paying a lock acquisition on the same monitor as GuiWebEmulatedWindow.offer, two CAS ops, a deque node, a notify, and a latch plus park/unpark round trip. Nothing forces 64 KB - the server's setMaxBinaryMessageSize is inbound-only and defaults to -1. Separately: Jetty 12.0.34 defaults DEFAULT_MAX_FRAME_SIZE to 65536 with auto-fragment on, and a piece is 6 + 65536 = 65542 bytes, so every piece is split into a full frame plus a 6-byte continuation frame. The payload should be (64 * 1024) - PARTIAL_HEADER_SIZE.
• p2j.socket.js handleMessageEvent - five timer operations per inbound message (new Date(), idleTimer.reset()'s clearInterval + setInterval, and a setTimeout / clearTimeout pair) now run once per 64 KB piece instead of once per logical message, because the browser no longer reassembles natively. A 33 MB streamed image means ~2640 timer create/destroy operations and 528 extra macrotasks on the main thread. The code is unchanged; only the number of times it runs regressed.
• p2j.socket.js collectChunk - holds every piece and then allocates the full-size Uint8Array and copies into it, releasing pieces only after the copy loop, so peak transient main-thread memory is ~2x the message and the code's own limits permit ~128 MB. Also total is recomputed with a loop over parts although parts.bytes already holds exactly that value. The original "extra copy the browser did not need" claim was dropped on verification - a browser reassembling fragments does the same accumulate-then-copy.
• p2j.socket.js sendPartialMessage - the payload is copied one byte at a time instead of msg.set(data.subarray(position, limit), PARTIAL_HEADER_SIZE); both operands are confirmed typed arrays so the replacement is exact. Measured on V8: 13.6 ms versus 1.2 ms per 10 MB. Low impact, but a correct one-line change on a line this branch already touches. The same loop exists in p2j.network_socket.js:294-298.

Documentation¶

• ConfigItem.MAX_IDLE_TIME - the new javadoc says the setting applies "only while client:web:webSocketTimeout is not positive", but no such key exists in any resolution path. The bootstrap option is client:web:socketTimeout and the directory node is webClient/webSocketTimeout; BootstrapConfig resolves only category():group():key() with no alias table. An administrator following the javadoc gets a silently ignored key while believing the timeout was raised. Especially misleading because neighbouring comments use the same client:web: form for keys that do exist.

Proposals¶

Four written proposals, in .tmp/rev16699/. Each covers problem, evidence and trigger, root cause, a recommended fix with code sketches, alternatives considered with reasons, risk, and a test plan.

Because no MAJOR security or performance finding survived the challenge pass, the security and performance proposals cover the highest-ranked finding in each category - both of which were reported MAJOR before being adjusted.

Two proposals are coupled and should be settled together: functional-2 recommends removing the discardQueued() call entirely, which makes the performance-1 defect unreachable from onConnect but leaves the method broken. Fix or remove the method in the same change.

Rejected in the challenge pass¶

Recorded so they are not raised again. Full reasoning is at the end of code-review.textile.

• WebClientProtocol.startKeepAlivePing - "the - tickMs slack narrows the configured tolerance to 60 s". The slack is load-bearing: because effectivePingPongInterval caps the interval at idleMs/PING_INTERVAL_DIVISOR, the heartbeat deadline and Jetty's own idle deadline are the same instant, and the client echoes synchronously from onmessage so the last inbound stamp sits one round trip after a tick. A strict >= dataIdleMs compare would miss that tick and fire a whole interval later - past the configured tolerance and past Jetty's idle close, losing the diagnostic 4001. The close actually lands at about 90 s with two unanswered pings; the proposed fix would be a regression. Residual below reporting threshold: the javadoc and WARNING describe the deadline as a single point, so the log can print "No websocket message received for 60000 ms (deadline=90000 ms)".
• p2j.socket messageHandler - "the removed inbound MSG_PING_PONG case leaves dead protocol traffic". The facts hold but the consequence does not: handleMessageEvent unconditionally stamps lastServerMessageAt, resets idleTimer, zeroes lostPings and calls reportPingRecovery() for any inbound frame, and the server stamps lastDataNanos generically in onMessage - so the reply already delivers the side effect the handler existed for. A vestigial one-byte reply in unchanged code; hygiene, not a defect. Incidental: the comment at p2j.socket.js:5881 is now stale.
• p2j.sso_reauth handleSsoReauth - "the expiry path has no session-gone guard and no login-page fallback". Rejected as a duplicate of the MAJOR finding: same defect, same trigger, same one-line fix. Its supporting evidence was folded into that finding.
• PushMessagesWorker.awaitCapacity - "the per-limit wait is never notified between lowWater and STREAM_QUEUE_LIMIT, collapsing throughput to about 13 KB/s". The thresholds really are uncoupled, but the wait is timed at 5 s and on each wake the producer re-queues everything that drained, so the rate is socket-bound rather than notify-starved; holding the counter inside the un-notified band for a full 5 s needs a second unthrottled producer, meaning the socket is already saturated. The web-push-queue-low-bytes key also appears nowhere outside the Java source. Worth noting in passing: the javadoc invariant "a limit well below the high-water mark" is asserted in both files but unenforced, so STREAM_QUEUE_LIMIT would be better derived from highWater() / lowWater() than hard-coded at 1 MB.

Artefacts¶

All under .tmp/rev16699/.

The reviewed diff was r16695..r16699; branch-r16695-r16700.diff additionally contains the r16700 style fixes. The two are identical apart from those fixes, and byte-identical to the reviewed diff apart from file mtimes in the +++ headers.

Hynek, please review rev 16700. The changes looks good.