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.

Code review 11645a revisions 16695..16700

I checked all the points below and they seem legit. I spent varying efforts on the points so please reasses.

• [MAJOR] functional WebClientProtocol.onConnect: the new pushWorker.discardQueued() throws away the whole pending output queue on the justification "initRemoteClient below resyncs", but GuiWebDriver.initRemoteClient() is literally a no-op; only ChuiWebSimulator.initRemoteClient() resyncs (clearCache / sendColorPalette / sendSwitchMode / triggerRepaint(true)). Trigger: the half-open reconnect — the page's silence watchdog runs closeWebSocketSafely + attemptToRestoreConnection, the server never saw the old CLOSE, so the new upgrade reaches the same endpoint object (GenericWebSocketCreator returns the singleton) and onConnect runs while pushWorker is still live. This is the only path where the discard has any effect, since the ordinary close path runs stopPushWorker(), which nulls pushWorker. Two consequences: (1) on Web GUI nothing repaints — handleOpenEvent takes the pageLoadedWasSent branch and sends only MSG_PING_PONG, which the server merely echoes — so every drawing message queued during the outage is lost and the canvas stays stale; (2) on both transports the discard also drops queued request/response traffic that no resync re-issues — StorageMessagingWebSocket.getStringValue/setStringValue/readClipboardValue/writeClipboardValue and showChangePasswordDialog all block in getResponse(msgId, 0) -> waitForResult(msgId, 0), which arms no timer and loops on lock.wait(), so the calling thread parks permanently on an otherwise healthy session (reachable from 4GL via GET-KEY-VALUE / PUT-KEY-VALUE through LocalStorageAccessor, and from OsPassExpirationChecker). The pre-existing sessionInvalid / offerFirst requeue machinery held exactly those messages for the replacement session. Recommend removing the discardQueued() call, or gating it so only genuinely stale drawing output is dropped.

• [MAJOR] functional PushMessagesWorker.discardQueued: it does not coordinate with the worker's failed-send requeue, so the discard is defeated. Interleaving: onConnect (Jetty thread, holding lock) closes the old session and runs discardQueued() -> messages.clear(); the worker, parked in latch.await() inside sendMessage, sees that very failure, sets sessionInvalid = true and re-inserts the message with messages.offerFirst(message) after the clear. onConnect then does collector.reset() and startWebWorker() before startPushWorker(session), and run() re-reads the session each iteration while sendMessage has no session or generation check — so the stale message is written to the brand-new socket ahead of the resync, defeating the generation guard queuePartial enforces. The mirror interleaving is worse: if sessionInvalid = true lands after setCurrentSession, nothing notifies sessionGuard again and the worker parks on a healthy socket, stopping all client output including the keep-alive, until the browser's watchdog forces another reconnect. Fix by stamping the in-flight message with the connection generation and checking it in sendMessage, mirroring queuePartial.

• [MAJOR] functional MessagesCollector.reset: partialMessages and payloadMessagesTypes are plain HashMap@s now mutated from three threads with no synchronization — @processPartialMesssage puts/gets on the webtaskworker thread (onMessage -> webWorker.post -> processBinaryMessage), AppendMessageTask.run gets on the collector's single-thread asynchIOExecutor, and the new reset() closes channels and clears both maps from the Jetty websocket thread via onConnect. onConnect's synchronized (lock) does not help: neither other path takes lock, and the worker is not quiesced (the replaced-session path skips onClose entirely, and WebTaskWorker.kill() only interrupts plus join(1000) while the run loop finishes its current batch). Trigger: an inbound MSG_PARTIAL transfer in flight (drag-drop upload via uploadFileToJava -> ReadFileChunkAndSend -> SendMsg -> sendPartialMessage, or any send() payload above client:web:maxBinaryMessage, e.g. a large paste) when a reconnect runs reset(): the executor thread then does partialMessages.get(msgId) -> null -> channel.write(payloadData), an NPE not covered by the surrounding catch (IOException) and buried in the discarded Future from submit, so AppendMessageTask.close never runs and the fd and map entry leak. The load-bearing hazard is the unsynchronized HashMap.clear() racing the worker's put() — structural mutation of a HashMap from two threads can lose entries or corrupt the table. Route reset() through webWorker.post (or drain the executor first), and null-check the channel in AppendMessageTask.run regardless, since reset() can always win the race against a queued task.

• [MAJOR] security MessagesCollector.processPartialMesssage: the inbound MSG_PARTIAL reassembly has no per-transfer cap, no aggregate cap and no expiry, whereas the same change added explicit MAX_TRANSFER_BYTES / MAX_CHUNK_BYTES plus dropTransfer / resetChunkTransfers on the JS side. Trigger: the page sends pieces carrying PARTIAL_FLAG_FIRST for N distinct transfer ids and never sets PARTIAL_FLAG_LAST; each id opens a temp file plus a FileChannel under java.io.tmpdir and every further piece is appended without limit. The only releases are reset() on the next onConnect (onClose does not reset) and process exit, both avoidable by holding the socket open. The per-process temp directory is mode 700, so there is no cross-user disclosure and the burned file descriptors only degrade the user's own spawned client; the cross-user leg is that java.io.tmpdir is the host's shared /tmp (no -Djava.io.tmpdir override on the spawn path, no quota), so one authenticated user can drive it to ENOSPC for every other user's client and for the FWD server. Two amplifiers: the new orphan-piece guard emits an unrate-limited LOG.warning per frame onto the same filesystem, and asynchIOExecutor is a single-thread executor with an unbounded queue holding each received payload array, so a sender faster than the disk grows the client heap with no backpressure. Mirror the JS caps (per-transfer and aggregate bytes, a bound on concurrent transfer ids, an idle expiry) and reset the collector from onClose as well as onConnect.

• [MAJOR] functional p2j.sso_reauth.js.handleSsoReauth: the countdown-expiry internal reload cannot "re-attach" a GUI session as its comment claims. On the reconnect WebClientProtocol.onConnect calls exactly one driver hook, callbacks.initRemoteClient(), and the GUI implementation is a no-op. The reloaded page does send MSG_PAGE_LOADED, but processBinaryMessage only parks it in receivedMessages for the one-shot startup waitForResult in EmbeddedWebServerImpl.waitInitialization; no MSG_DESKTOP_RESIZED is sent on load, and desktopResized only redraws zooming or maximized windows. redrawZoomingWindows() is also the only caller of clearPerWidgetCache / clearFullWindowCache, so the server still believes the browser holds hashes that died with the document. Result: the reloaded page attaches to a live GUI client and shows an empty virtual desktop — and is now also un-quittable, because internalReload suppresses the onpagehide exitTheApplication marker that isRequiredToRedirect() reads, so the old quit-on-reload path that used to release the session no longer runs. Concrete trigger with the session provably alive: the user completes sign-in, /reauthcb -> processReauthCallback clears reauthInProgress and cancels reauthTimeoutFuture (force-logout will now never fire), but the popup's postMessage is dropped by the event.origin !== serverOrigin check (serverOrigin falls back to window.location.origin when getLogoutPage() does not parse, so a differing logout origin suffices); the user closes the popup, reauthInFlight() goes false, the grace is skipped and the tick reloads against a fully re-authenticated session. Give GuiWebDriver.initRemoteClient() a real resync (clear both graphics caches and redraw all visible windows) before relying on a reload here, or keep the GUI expiry on a terminal path.

• [MINOR] functional PushMessagesWorker.discardQueued: queuedBytes.set(0L) is not atomic with respect to the worker's dequeue-then-send, so the counter can go negative. onConnect (Jetty thread, holding only lock) zeroes it while the worker holds only sendLock and is parked in latch.await() for a message it already removed with messages.pollFirst(); on completion releaseCapacity(msgSize) subtracts against the zeroed counter. While the value is negative, both awaitCapacity overloads return at their first get(), raising the effective threshold of both the 32 MB high-water gate and the 1 MB STREAM_QUEUE_LIMIT by the bias. Bounded rather than unbounded: only one message is ever in flight, and discardQueued re-floors to zero on every reconnect, so the bias cannot accumulate — but it should still be fixed by subtracting only what was actually removed, or by having sendMessage skip releaseCapacity when a discard intervened. Note the producer-side window is already closed: all pushMessage call sites run inside synchronized (lock).

• [MINOR] functional WebClientProtocol.sendBinaryMessageStreamed: widening the handler to catch (IOException | RuntimeException e) turns a real defect in the body stream into a silently malformed message. The trigger is the one the comment names and it is reachable end-to-end: GuiWebEmulatedWindow.encodeImage passes the unclipped image dimensions to defineImageStreamed while VirtualScreenImpl.drawImage clips to the desktop size, so RawImageInputStream indexes past its backing array and throws ArrayIndexOutOfBoundsException for any image larger than the desktop. The handler then queues a 6-byte last piece, so collectChunk sees a complete transfer; p2j.screen.js's defineImage derives the body length from the header itself and subarray clamps silently, caching a short pixel array under key. Because encodeImage already registered the seal via addImageUsage / mapSealToUniqueId before the send and returns ImageEncoding.HASH with no pixels, this and every later draw of that image references pixels that never arrived, and the browser's loadedImages.has(key) guard passes — a permanently garbled cached image, reported only at WARNING. Note this is genuinely new: RawImageInputStream.read declares no IOException, so the pre-change catch was inert on the image path and the AIOOBE used to propagate. Log at SEVERE and evict the image key (or register the seal only after the body is fully streamed).

• [MINOR] functional p2j.socket.js.messageHandler: the MSG_SERVER_PING echo goes through sendNotification() -> send(), which drops the message unless isWebSocketConnected() holds — and that predicate includes the module-level online flag, set false by the window offline listener in me.init and only ever restored by the matching online listener; nothing re-validates it against the live socket. A browser whose OS reports no non-loopback interface therefore stops echoing while the websocket is perfectly usable (same-host deployment on https://localhost:7443/gui, including the virtual-desktop iframe, across an Ethernet/Wi-Fi drop or VPN flap). startKeepAlivePing arms its deadline on inbound silence alone (lastDataNanos is stamped only in the two onMessage overloads), so with the shipped defaults it closes a working session with 4001 after 60 s and onClose arms the process-killing watchdog. The two ends disagree silently: handleMessageEvent resets idleTimer and zeroes lostPings on every inbound frame, so the server's own pings keep the page's watchdog satisfied and the page shows nothing wrong until the close. Since online is still false on the new socket, the cycle repeats. Exempt the heartbeat echo from the online gate, or drop online from isWebSocketConnected and rely on ws.readyState, which is authoritative for this socket.

• [MINOR] functional p2j.socket.js.beginInternalReload: internalReload is set once and never cleared, suppressing both the onpagehide quit-on-reload arming and the onbeforeunload confirmation for the remaining life of the document. window.location.reload() only starts a navigation — the old document stays live and interactive until the new response commits — and this path exists precisely because the client may have just been killed, while the page is served by the client's own embedded Jetty whose MAX_HTTP_IDLE_TIME defaults to INFINITE_TIMEOUT, so a stalled fetch can hold the flag true for an unbounded time on a page the user can still act on. Closing the tab in that window gives no leave confirmation and writes no exitTheApplication marker, so a same-tab return re-attaches silently instead of sending MSG_QUIT. The change's own comment ("The interval is left running so a navigation the user declines is recoverable") contradicts itself: with internalReload true the beforeunload prompt is suppressed, so the user can never decline. Clear the flag on a bounded timer or on pageshow, or scope the suppression to the one unload that belongs to the reload.

PushMessagesWorker has two capacity gates over the same queuedBytes counter (PushMessagesWorker.java:247-302).

awaitCapacity() is the general gate: parks a producer at highWater() (32 MB default), releases at lowWater() (16 MB default). Two thresholds, 16 MB apart, so a producer that just drained does not immediately re-park.

awaitCapacity(long limit) is the new streamed-transfer gate. Parks at limit, releases at the same limit. No hysteresis. And it checks the same global queuedBytes, not the calling transfer's own bytes.

WebClientProtocol.queuePartial calls this gate with STREAM_QUEUE_LIMIT (1 MB, line 333), once per chunk of sendBinaryMessageStreamed (line 1058). That is the path GuiWebSocket uses to stream a screen image without buffering it whole (GuiWebSocket.java:1316).

Ordinary sends only go through the general gate, so unrelated output can fill the queue to 32 MB before anything throttles. If that backlog is already sitting there (slow browser, several sendPendingDrawingOps batches queued) and a new streamed transfer starts, its first queuePartial call sees queuedBytes already above 1 MB and parks before sending anything. With no hysteresis, it does not wake up again until the whole backlog, not just this transfer's share, drains below 1 MB. That is a deeper drain than the general gate ever requires (its own low-water release point is 16 MB).

• [MINOR] functional WebClientProtocol.startKeepAlivePing: the javadoc states "the echo deadline is DATA_QUIET_MULTIPLIER intervals, or that idle timeout where it is longer, so a configured tolerance is never silently narrowed", but the deadline actually enforced is dataIdleMs - tickMs. The one-interval slack is intended and its rationale is in the inline comment — the defect is that the method javadoc asserts the opposite, and the logging carries the same inconsistency: with the shipped defaults (socketTimeout 90000, pingPongInterval 30000) the close is logged as "No websocket message received for 60003 ms (deadline=90000 ms)" and startup reports "heartbeat deadline=90000", both advertising a deadline the code does not apply — actively misleading when diagnosing an unexpected disconnect. Worth documenting in the same place: the slack costs a whole heartbeat and degenerates to a single heartbeat whenever the PING_INTERVAL_DIVISOR cap binds, which is the case in both shipped profiles (default: ping at 30 s, close at 60 s; DEBUG_PROFILE: ping at 400 s, close at 800 s of a 1200 s tolerance), so one late or dropped echo tears the session down with no retry. A divisor of 4 would keep the slack and still leave a retry.

• [MINOR] style WebConfigurationConstants.sanitizePingPongInterval: the new static methods sanitizePingPongInterval and effectivePingPongInterval are inserted between interface constant fields (after PING_INTERVAL_DIVISOR, before ENABLE_DEBUG_LOGGING), violating the required member ordering — all data members must precede all methods. Move them after the last constant field.