Project

General

Profile

code-review.textile

Sergey Ivanovskiy, 08/14/2026 05:13 AM

Download (25.1 KB)

 
1
*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.main (1 java), com.goldencode.p2j.util (1 java), com.goldencode.p2j.web (1 java) — 15 files, 2437 diff lines
2

    
3
*Domains reviewed:* fwd-web-gui-driver, fwd-web-login-process, fwd-server-infrastructure, fwd-chui-driver-streams, plus style, security and performance passes
4

    
5
h2. Confirmed findings
6

    
7
* *[MAJOR]* _functional_ @p2j.sso_reauth@.@handleSsoReauth@: nothing stops @countdownInterval@ when the session ends by the server's own force-logout, so the expiry tick fires into the shutdown and destroys the logout redirect. @SsoTokenManager.handleInvalidToken@ schedules @forceLogout@ at @reauthTimeoutSec@ (SsoTokenManager.java:518) *before* calling @sendSsoReauth@ (:539), while this module sets @reauthDeadline@ only on receipt, so the client deadline is always the server deadline plus delivery latency — the tick fires milliseconds *after* @forceLogout@, inside the shutdown. @cleanupReauthUi@ (the only thing that clears the interval) is reachable solely from this module's own success/failure/logout handlers; there is no cleanup hook on @MSG_SHUTTINGDOWN@, @MSG_QUIT@ or @doRedirectToLogoutPage@. The tick therefore runs @beginInternalReload()@ + @window.location.reload()@ mid-handshake (p2j.sso_reauth.js:262-264): the browser's @MSG_SHUTTINGDOWN@ ack is sent only from the @.finally@ of @requestDeleteAuthCookie@ and is discarded by the navigation, so @quit()@ burns the full @clientResponseTimeout@ (5000 ms) and @MSG_QUIT@ goes nowhere; even if @MSG_QUIT@ arrived first, @doRedirectToLogoutPage@'s @window.top.location.replace(logoutPage)@ sits inside a @fetch().then()@ that the reload aborts. The reloaded iframe cannot reach the login page either — @web_landing.html:123@ points it at the spawned client's own embedded server, which @shutdownServer()@ has just torn down, and @beginInternalReload()@ deliberately suppresses @exitTheApplication@ so @isRequiredToRedirect()@ returns false. The user is stranded on a dead iframe. This is a regression: the previous expiry path called @cleanupReauthUi(true)@ and only replaced the body, leaving the socket module intact to complete the redirect.
8
* *[MAJOR]* _functional_ @WebClientProtocol@.@onConnect@: the new @pushWorker.discardQueued()@ (WebClientProtocol.java:480-486) throws away all pending server→browser output, justified by the comment "initRemoteClient below resyncs" — but @GuiWebDriver.initRemoteClient()@ is literally a no-op (GuiWebDriver.java:1033-1036); only @ChuiWebSimulator.initRemoteClient()@ resyncs (ChuiWebSimulator.java:200-215). Trigger: a half-open-socket outage where the browser's @checkLostPings@/@onServerSilenceExpired@ → @attemptToRestoreConnection()@ reconnect lands before Jetty delivers @onClose@ for the dead session, so @onConnect@ takes its @this.session != null && this.session.isOpen()@ replacement branch with @pushWorker != null@ and a full backlog. The page was never reloaded, so its canvas is intact and stale and nothing regenerates the lost ops; on that path the browser sends @MSG_PING_PONG@, never @MSG_PAGE_LOADED@ (p2j.socket.js:5762), so no repaint is requested. Note the discard has effect *only* on this stale-session path — on a first connect @pushWorker == null@ and after a real @onClose@ @stopPushWorker()@ nulls it — i.e. exactly where it is unsafe. Cross-generation partial tails are already handled by @queuePartial@'s generation check plus the client's @resetChunkTransfers()@, so discarding whole messages is not needed for transfer safety.
9
* *[MAJOR]* _style_ @WebClientProtocol@.@startKeepAlivePing@: added javadoc line is 130 characters, over the 110 limit — WebClientProtocol.java:1860.
10
* *[MAJOR]* _style_ @p2j.socket.js@.@types@: the new block comment describing @MSG_PARTIAL@'s wire format was inserted after @MSG_SERVER_PING@ but before the unrelated @MSG_INVALIDATE_SELECTION@ field, orphaning it from any subject, while the actual @MSG_PARTIAL@ property still carries its stale one-line comment — p2j.socket.js:644-647 vs :679-680. The orphaned text is also incomplete: it documents only flags bit 0 (last), not bit 1 (first), which @PARTIAL_FLAG_FIRST@ made central.
11
* *[MAJOR]* _style_ @p2j.sso_reauth.js@.@cleanupReauthUi@: the javadoc block with its @@param removeOverlay@ is no longer immediately above its subject — the new @closeReauthPopup@ function and its own comment were inserted between the javadoc (:435-441) and @cleanupReauthUi@ (:452).
12
* *[MINOR]* _functional_ @WebClientProtocol.MessagesCollector@.@reset@: called from @onConnect@ on the Jetty thread under @lock@ (:490), it closes the @FileChannel@s and clears the plain @HashMap@s @partialMessages@/@payloadMessagesTypes@ (:2052/:2055) while two other threads own that state with no synchronization on their side. In the replacement branch the old session's @onClose@ has not run and is later skipped by the @session != this.session@ gate (:809), and @startWebWorker()@ no-ops when @webWorker != null@ (:1990), so the previous @WebTaskWorker@ keeps calling @processPartialMesssage@ (@get@/@put@, no @lock@ on that path); @asynchIOExecutor@ is never shut down at all, so its @AppendMessageTask@ backlog survives every reconnect. A queued task then gets @null@ from @partialMessages.get(msgId)@ and NPEs on @channel.write@ (:2206); the catch handles only @IOException@, so @submit()@'s @FutureTask@ swallows it and the only symptom is a silently truncated upload. Additionally @nextMsgId@ is a page-scoped @var@ (p2j.socket.js:1025) restarting at 0 on any real page load, and @AppendMessageTask@ resolves its channel by @msgId@ at run time with no identity or generation check — so a stale queued task for old id 0 can write into the new page's id 0 channel and, if it carries @isLast@, post @processChannel@ to read a payload byte as the message type. @PARTIAL_FLAG_FIRST@ blocks that only through @processPartialMesssage@, not through the executor queue. Fix direction: have @AppendMessageTask@ carry its @FileChannel@ and generation instead of re-resolving by id, make the maps concurrent, and drain or replace the executor on reset.
13
* *[MINOR]* _functional_ @WebClientProtocol@.@onConnect@: the new @if (collector != null)@ (:488) reads the non-volatile, non-final, unguarded @collector@ field (:387) from the Jetty thread, while it is lazily assigned in @getMessagesCollector()@ (:1676-1683) on the @webtaskworker@ thread via the @MSG_PARTIAL@ case (:637) — one of the few switch cases that never enters @synchronized (lock)@. @pushWorker@ was made @volatile@ in this same change (:357) but @collector@ was not, and the session-replacement branch reaches @onConnect@ without the @stopWebWorker()@/@join()@ edge that would publish the write. Trigger: the first @MSG_PARTIAL@ of a session creates the collector on the worker thread, then a reconnect runs @onConnect@, observes @collector == null@, skips the reset, and leaks exactly the open @FileChannel@s, temp files and @payloadMessagesTypes@ entries the reset was added to drop — silently, since the stale-head guard (:2117) only logs "Discarding orphaned message piece". Making the field volatile fixes only the publication, not the concurrent map mutation noted above.
14
* *[MINOR]* _functional_ @PushMessagesWorker@.@discardQueued@: (:332-341) mutates @messages@ and @queuedBytes@ with no mutual exclusion against the worker thread's in-flight send — @onConnect@ holds @WebClientProtocol.lock@, the worker holds only @sendLock@, disjoint monitors. Two effects: (1) @sendMessage@ captures @msgSize@ at :556 and calls @releaseCapacity(msgSize)@ at :672, so a @queuedBytes.set(0L)@ landing between them leaves the counter at @-msgSize@ with nothing to floor it, raising both water marks by that amount until the next discard re-bases it; the mirror case also exists, since @pushMessage@'s @addAndGet(+size)@ precedes @offerLast@, leaking positive. (2) On the failure path @messages.offerFirst(message)@ (:666) runs *after* @messages.clear()@, so one message built for the previous @connectionGeneration@ survives the discard and is the first thing written to the fresh socket, ahead of the resync — the comment at :665 ("its bytes remain accounted for") is false once a discard intervened. Downgraded from MAJOR: there is one worker thread so at most one @msgSize@ races per discard, every discard re-bases with an absolute @set@, and pieces are capped at @PARTIAL_PAYLOAD_SIZE@ (64 KB), so the 32 MB guard is perturbed by tens of KB, not progressively raised. The stale-message leg is also largely absorbed by the peer (@collectChunk@ drops pieces without @PARTIAL_FLAG_FIRST@, @handleOpenEvent@ calls @resetChunkTransfers()@); residual exposure is a stale non-partial message rendered ahead of the resync, or a stale @FIRST@ piece opening a transfer that never completes.
15
* *[MINOR]* _functional_ @WebClientProtocol@.@queuePartial@: an abandoned streamed transfer is dropped silently — @queuePartial@ returns @false@ (:1054-1074) on a generation mismatch, null worker or closed session, and both @sendBinaryMessageStreamed@ bail-outs (:995, :1020) return with no @receiveResult@ and no notify. @GuiWebSocket.createFont@ (:4051 then :4059) then blocks forever: @waitForResult(msgId)@ delegates to @waitForResult(msgId, 0)@ and @0@ is implemented as "indefinitely" — no @Timer@ is scheduled, so the @lock.wait()@ loop is broken only by @receiveResult@ for that exact id. Nothing else releases the waiter: @onClose@ only stops workers and arms the watchdog, @onConnect@ bumps the generation without resetting @receivedMessages@, and the watchdog is stopped once the reconnect succeeds — so the client lives on with a dead drawing thread. Reachable: @FontManager@'s lazy cache-miss path calls @GuiWebDriver.createFont@ mid-session for any font under @deploy/server/fonts@, and a TTF spans many 64 KB pieces with the producer parked at @STREAM_QUEUE_LIMIT@, so the window is wide. The right fix is to fail the pending wait on abandonment (or release generation-scoped waiters in @onConnect@/@onClose@); note the same hang applies to non-streamed @sendBinaryMessage@ + @waitForResult(msgId)@ pairs, since @discardQueued()@ drops those too — @timeout 0@ waits are systemically unsafe across a reconnect.
16
* *[MINOR]* _security_ @GuiWebSocket@.@processChannel@: the reassembled @MSG_FILE_UPLOADING@ message is dispatched to @callbacks.saveDropTargetFile(fileName, channel, fileSize)@ (:2419-2441) with @fileName@ read verbatim from the peer's UTF-16 name field and no validation and no pending-upload gate; @GuiWebDriver@ (:1935) does @DragDropHelper.readDataAndSaveAsFile(tmpDir.concat(name), ...)@ where @tmpDir@ ends with @File.separator@, and @DragDropHelper@ (:254-261) does @Paths.get(name)@ → @Files.newByteChannel@ with no @normalize()@, no @getFileName()@ and no containment check. A crafted peer sending a name of @../../../../home/<user>/.bashrc@ writes attacker-chosen bytes anywhere that process's OS user can write. The trust model does cross a boundary: the @GuiWebSocket@/@GuiWebDriver@ process is the *server-side* spawned web client, its websocket endpoint is registered at the fixed @AJAX_TARGET@ path with no Origin check, and under @OS_USER_OVERRIDE@ (or no SSO OS-user mapping) every user's client runs as the shared @DEFAULT_OS_USER@ — so an authenticated application user with no shell rights gains arbitrary file write on the app-server host. The project's own HTTP upload path already does the missing check (@UploadHandler@:231-233 does @getFileName()@ + @uploadDir.resolve@); the websocket path omits it. *Scoped MINOR for this diff* because the sink and its two twins (@GuiWebSocket@:3540, @GuiWebDriver.startDropTarget@:1890) are pre-existing and untouched here — this diff only reworks the delivery route — but the underlying defect warrants its own ticket at higher severity.
17
* *[MINOR]* _security_ @WebClientProtocol.MessagesCollector@.@processPartialMesssage@: inbound reassembly (:2107) has no aggregate size cap and no per-transfer expiry, the mirror image of the caps this same diff added on the JS receive path (@MAX_TRANSFER_BYTES@/@MAX_CHUNK_BYTES@); Jetty's @setMaxBinaryMessageSize@ bounds a single message only. A scripted page in the authenticated session sends one piece with @PARTIAL_FLAG_FIRST@ and then pieces forever without @PARTIAL_FLAG_LAST@, growing a temp file under @java.io.tmpdir@ (plain @Files.createTempDirectory(null)@, *not* a per-session FWD dir) until ENOSPC — shared by every spawned client and the server on that host, which is what keeps this a security rather than purely robustness finding. Variant: ~7-byte messages with distinct transfer ids each open a @FileChannel@ that is never closed, to the fd rlimit; when @FileChannel.open@ finally throws it is swallowed at @Level.FINE@ (:2145-2149), so the session silently stops accepting large inbound messages. Mitigations that do apply (and correct the original report): the temp files are 0600 and @DELETE_ON_CLOSE@, so nothing survives process death. But @onClose@ does *not* call @collector.reset()@ (:836-841) — only the next @onConnect@ does — so while the attacker holds the socket open neither reset nor exit occurs and the growth window is attacker-controlled. Also note @processBinaryMessage@ gates on @length > PARTIAL_HEADER_SIZE@ (:635), so a header-only 6-byte piece is dropped even carrying @PARTIAL_FLAG_LAST@, leaving the transfer permanently open — and the Java *sender* emits exactly such a 6-byte terminator (:1029), so the guard should be @>=@.
18
* *[MINOR]* _functional_ @ConfigItem@.@MAX_IDLE_TIME@: the new javadoc (:363-367) says the setting applies "only while @client:web:webSocketTimeout@ is not positive", but no such key exists in any resolution path. @WEB_SOCKET_TIMEOUT@ (:377) is declared with nodeName @webSocketTimeout@ and category/group/key @client@/@web@/@socketTimeout@ — so the bootstrap option is @client:web:socketTimeout@ and the *directory* node is @webClient/webSocketTimeout@. Both consumers read it through @BootstrapConfig@, which resolves a @ConfigItem@ only as @category():group():key()@ with no alias table. An administrator following the javadoc adds @client:web:webSocketTimeout@ to @p2j.cfg.xml@, @getConfigItem@ returns null, and the default applies (90000 ms on the @ChuiWebSimulator@/@WebClientBuilderOptions@ path) while the operator believes the timeout was raised. Especially misleading because neighbouring comments use the same @client:web:@ form for keys that *do* exist (@client:web:maxIdleTime@, @client:web:clientResponseTimeout@). A tree-wide grep finds @webSocketTimeout@ only as the directory node name, never as a bootstrap key.
19
* *[MINOR]* _performance_ @WebClientProtocol@.@sendBinaryMessageStreamed@: @PARTIAL_PAYLOAD_SIZE@ is @64 * 1024@ (:324) where the pre-change continuation-frame path used @ChunkTransferDaemon.getChunkSizeCached()@ (1 MB default) with a single buffer hoisted out of the loop, so every streamed transfer pays ~16x the per-unit overhead. Nothing forces 64 KB: the server's @setMaxBinaryMessageSize@ is *inbound*-only and defaults to @-1@, the browser's @maxBinaryMessage@ governs only client→server pieces, @collectChunk@ has no per-piece cap, and the pre-existing @sendPendingDrawingOps@ already ships unbounded whole messages. The stated constraint (a piece must stay well under the echo deadline of 3 ping intervals) is met by a far larger piece — 1 MB only breaches a 90 s deadline below ~93 KB/s — and the heartbeat is enqueued with @pushMessageFirst@ so it waits only behind the piece in flight. Trigger: @GuiWebEmulatedWindow.encodeImage@ → @GuiWebSocket.defineImageStreamed@ on first use of any embedded image; a 3840x2160 image is 33,177,600 bytes = 507 body pieces + 1 header = *508* messages (the 528 figure used MiB) versus 33 frames before; a 1920x1080 background is 127 versus 9. Per piece: a @synchronized (lock)@ acquisition in @queuePartial@ on the *same* monitor as @GuiWebEmulatedWindow.offer@ (both receive @GuiWebDriver.lock@), two @queuedBytes@ CAS ops, a deque node, a @synchronized (messages) { notify() }@, then a @CountDownLatch@, one @Callback@ and a @Throwable[1]@ plus a park/unpark round trip on the worker thread. Two refinements: the fresh @byte[]@ per piece is inherent to the async-queue design (the queue owns the bytes until written), so a larger piece reduces the allocation *count*, not the volume; and separately — Jetty 12.0.34 defaults @DEFAULT_MAX_FRAME_SIZE@ = 65536 with @DEFAULT_AUTO_FRAGMENT@ = true and FWD overrides neither, so each 65542-byte piece (6-byte header + 65536) crosses the frame boundary by exactly 6 bytes and is auto-fragmented into a full frame plus a 6-byte continuation frame — 508 extra near-empty frames per 4K image. If 64 KB is kept, the payload should be @(64 * 1024) - PARTIAL_HEADER_SIZE@.
20
* *[MINOR]* _performance_ @p2j.socket.js@.@handleMessageEvent@: the per-message fixed work — @new Date()@, @idleTimer.reset()@ (a @clearInterval@ + @setInterval@ pair inside @ControlTimer.reset@), the wait-cursor @setTimeout(…, 500)@ and the trailing @setTimeout(…, 0)@ that does the @clearTimeout@, i.e. *five* timer operations — runs unconditionally at :5936-5944, before the @data instanceof ArrayBuffer@ branch, so @MSG_PARTIAL@ pieces are not short-circuited. It is now paid once per 64 KB piece instead of once per logical message: the removed javadoc on @sendBinaryMessageStreamed@ stated verbatim that "the browser transparently reassembles the continuation frames into one message before its @onmessage@ handler runs, so no JavaScript change is required", and the new code replaces that with discrete application-level messages. A 33 MB @defineImageStreamed@ turns 1 inbound message into 528, i.e. ~2640 timer create/destroy operations plus 528 extra macrotasks on the main thread for one image; a 1920x1080 image yields 127 pieces / ~635 ops. The code itself is unchanged — the regression is purely in how many times it runs.
21
* *[MINOR]* _performance_ @p2j.socket.js@.@collectChunk@: reassembly (:3196-3259) holds every piece and then allocates the full-size @Uint8Array@ and copies into it, so peak transient main-thread memory is ~2x the message: a streamed 33 MB @MSG_DEFINE_IMAGE@ retains ~528 pieces (~34.6 MB, since each @message.subarray(PARTIAL_HEADER_SIZE)@ pins its whole 64 KB+6 frame @ArrayBuffer@) and then allocates a 33 MB @whole@ before any piece is released — pieces are dropped only by @delete chunkTransfers[id]@ at :3256, after the copy loop. The code's own limits permit ~128 MB (@parts.bytes@ may reach @MAX_TRANSFER_BYTES@ = 64 MB, since the check at :3221 is @>@). Separately, @total@ at :3240-3245 is recomputed with a loop over @parts@ although @parts.bytes@ already holds exactly that value (set to 0 at :3214, incremented by @piece.length@ at :3218, never otherwise mutated) — replace the loop with @var total = parts.bytes;@. Mitigation for the peak: null out @parts[i]@ as each piece is copied, or carry the total length in the first piece so @whole@ can be preallocated. Note the original "one full copy the previous native reassembly did not need" claim was dropped on verification — a browser reassembling fragments accumulates then allocates one contiguous buffer and copies too, so the ~2x peak is not new; the work merely moved from browser-native code into main-thread JS. Continuation frames were also abandoned deliberately (a fragmented message admits no interleaved data frames, so it holds the socket and starves the peer's silence watchdog), so this is not revertible.
22
* *[MINOR]* _performance_ @p2j.socket.js@.@sendPartialMessage@: the payload is copied one byte at a time at :1897 (a line this diff edited, changing @j = 10@ to @j = PARTIAL_HEADER_SIZE@) instead of @msg.set(data.subarray(position, limit), PARTIAL_HEADER_SIZE)@. Both operands are typed arrays — @msg@ is @new Uint8Array(payloadLength + PARTIAL_HEADER_SIZE)@ (:1892) and @data@ is always a @Uint8Array@ (@send()@ coerces via @Uint8Array.from()@) — so the replacement is exactly equivalent. Every piece goes through the loop from @SendMsg.execute@ (:1139-1164) with @payloadLength = maxBinaryMessage - 26@, i.e. 32742 iterations per piece at the 32768 default, reachable from a plain file upload (@p2j.screen.js@:1630 → @uploadFileToJava@ → @ReadFileChunkAndSend@ → @SendMsg@). Measured on V8, 10 MB in 32742-byte pieces: 13.6 ms for the byte loop vs 1.2 ms for @set(subarray)@ — ~12 ms of avoidable main-thread work per 10 MB, split across ~320 calls of ~42 µs, so no dropped frame. Low-impact polish, but a correct one-line change on a line the diff already touches. The same loop exists in @p2j.network_socket.js@:294-298 if a consistent fix is wanted. (Incidentally, @createFileUploadingMessage@ sizes the first message at the whole @fileSize@ (:1751) while filling only the first 2 MB chunk, and @SendMsg@ sends its full @byteLength@, so a 10 MB upload actually pushes ~18 MB through this loop.)
23
* *[MINOR]* _style_ @WebClientMessageTypes@.@MSG_SERVER_PING@: two consecutive blank lines before the new field's javadoc instead of one — WebClientMessageTypes.java:501-502.
24
* *[MINOR]* _style_ @WebClientProtocol@.@stopKeepAlivePing@: two consecutive blank lines before this method's javadoc instead of one — WebClientProtocol.java:1968-1969.
25
* *[MINOR]* _style_ @p2j.sso_reauth.js@.@handleSsoReauth@: two consecutive blank lines after this function's closing brace instead of one — p2j.sso_reauth.js:433-434.
26
* *[MINOR]* _style_ @WebClientProtocol@.@queuePartial@: the wrapped parameter continuation has 30 leading spaces where 32 are needed to align with @int generation@ after the opening paren — WebClientProtocol.java:1055.
27
* *[MINOR]* _style_ @p2j.sso_reauth.js@.@header@: added history-entry line is 113 characters, over the 110 limit — p2j.sso_reauth.js:16.
28
* *[MINOR]* _style_ @p2j.socket.js@.@sendPartialMessage@: @for(@ is missing the required space after the keyword — p2j.socket.js:1897 (a line this diff modifies).
29
* *[MINOR]* _style_ @p2j.socket.js@.@types@: added line is whitespace-only rather than empty — p2j.socket.js:648.
30

    
31
h2. Rejected in the challenge pass
32

    
33
Recorded so they are not re-raised; the uncurated list is at @.tmp/code-review_uncurated.textile@.
34

    
35
* @WebClientProtocol.startKeepAlivePing@ — "the @- tickMs@ slack narrows the configured tolerance to 60 s". Rejected: the slack is load-bearing. @effectivePingPongInterval@ caps @interval@ at @idleMs/PING_INTERVAL_DIVISOR@, so @dataIdleMs@ always equals Jetty's own idle deadline exactly; the client echoes @MSG_SERVER_PING@ synchronously from @onmessage@, so @lastDataNanos@ normally sits one round trip *after* a tick and a strict @>= dataIdleMs@ compare would miss the tick at @dataIdleMs - rtt@ and only fire a whole interval later (~120 s) — past the configured tolerance and past Jetty's idle close, losing the diagnostic 4001. In the shipped steady state the close actually lands at ≈90 s with 2 unanswered pings; the 60 s figure needs a phase-unlucky corner where only *one* ping went unanswered. The proposed fix would be a regression. (Residual below reporting threshold: the javadoc and the WARNING describe the deadline as a single point, so the log can print "No websocket message received for 60000 ms (deadline=90000 ms)".)
36
* @p2j.socket.messageHandler@ — "the removed inbound @MSG_PING_PONG@ case leaves dead protocol traffic". Facts confirmed (no inbound case remains, the client still sends it from :5764 and :6125, the server still replies at WebClientProtocol.java:641-646, the @default:@ is empty) but the consequence is wrong: @handleMessageEvent@ unconditionally stamps @lastServerMessageAt@, resets @idleTimer@, zeroes @lostPings@ and calls @reportPingRecovery()@ for *any* inbound frame, and the server stamps @lastDataNanos@ in @onMessage@ generically, so the reply already delivers the side effect the handler existed for. Vestigial 1-byte reply in unchanged code; hygiene, not a defect. (Incidental: the comment at p2j.socket.js:5881 "can be set by the pong handler" is now stale.)
37
* @p2j.sso_reauth.handleSsoReauth@ — "the expiry path has no session-gone guard and no login-page fallback". Rejected as a duplicate: same defect, same trigger and same one-line fix as the MAJOR finding above, whose text already covers both the missing guard and the stranded-user consequence. Its supporting evidence was folded into that finding.
38
* @PushMessagesWorker.awaitCapacity@ — "the per-limit wait is never notified in @[lowWater, STREAM_QUEUE_LIMIT)@, collapsing streamed transfers to ~13 KB/s". Facts confirmed (@STREAM_QUEUE_LIMIT@ = 1 MB hard-coded, @lowWater()@ from @web-push-queue-low-bytes@ defaulting to 16 MB, notify gated on @lowWater()@ only) but the impact does not survive: the wait is timed (@capacityGuard.wait(WAIT_TO_DIE)@, 5 s), and on each wake the producer re-queues everything that drained, so the rate is socket-bound, not one piece per 5 s; holding the counter inside the un-notified band for a full 5 s requires a second unthrottled producer, i.e. the socket is already saturated. The key also appears nowhere outside the Java source, so tuning it below the hidden 1 MB constant is not a plausible operator action. What remains is a nit 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.