Input can be discarded on failed socket writes; reconnect and keepalive paths can silently stop

[adkins17]

Input can be discarded on failed socket writes; reconnect and keepalive paths can silently stop

Clean patch: https://github.com/adkins17/potato/commit/68705bd43fd40ebc1936e50613f060fd1002d82d.patch
Patch branch: https://github.com/adkins17/potato/tree/fix/socket-input-reliability-v2
Compared against upstream: 46d18387bd6b156b85176fa588a60991cbba6235

Summary

Potato can erase a submitted command even when the command is not accepted by the socket layer. The input handler clears the widget before attempting delivery, the intermediate send helpers do not return delivery status, and ioWrite catches and discards every puts error.

This makes a connection appear usable while commands disappear without reaching the world or producing an actionable client error. The race is easier to encounter while switching between connections or when a socket becomes invalid between the UI event and the write.

The same source paths contain related reliability problems:

• EOF detected before read disconnects without scheduling automatic reconnect.
• boot_reconnect calls after cancel conn($c,reconnectId) without dereferencing the timer ID.
• The retry callback invokes reconnect directly, so a synchronous failed attempt can end the retry loop.
• Keepalive timer IDs are not tracked; duplicate invocations can create multiple loops, while an uncaught per-connection error can stop the loop.
• NOP keepalive is gated on hasProtocol telnet, although Telnet NOP itself has no option negotiation requirement.
• Null-byte keepalive uses the normal text path, appending the world's configured line ending instead of sending exactly one NUL byte.
• When NOP and null-byte keepalives are both enabled, Potato sends both. Some servers retain the NUL and discard the next command line even though Potato's socket write succeeded.

Deterministic reproduction

The attached patch includes tests/socket-input-reliability.test.tcl, which extracts and executes the actual Potato procedure bodies.

Without the patch:

1. Make ioWrite raise a channel error during sendRaw.
2. Submit text through send_mushage.
3. Observe that the input widget has already been cleared and no failure reaches the caller.
4. Trigger boot_reconnect twice and observe that the original timer is not reliably cancelled.
5. Enable null-byte keepalive with CRLF and observe 00 0D 0A instead of a single 00.
6. Enable both NOP and null-byte keepalive, wait for FF F1 00, then submit numbered commands. On an affected server, the first command after each NUL is absent from the server command log while the following command succeeds.

Expected behavior

• Input should only be cleared after the local socket write is accepted.
• Rejected input should remain editable and produce a visible warning.
• The focused input widget should determine its owning connection rather than relying only on global active state.
• Unexpected EOF/read/write failures should use one disconnect-and-reconnect path.
• A failed automatic reconnect should re-arm the retry loop while automatic reconnect remains enabled.
• Only one keepalive timer should exist.
• Only one keepalive method should be sent per interval: Telnet NOP (FF F1) as the primary method, or null (00) as a fallback when NOP is disabled. A user-submitted blank line with CRLF should remain 0D 0A.

Patch behavior

The patch:

• propagates ioWrite errors and makes sendRaw, send_to_real, send_to, and send_to_noparse return success/failure;
• preserves input when delivery is rejected and shows Message not sent; input retained.;
• resolves the destination from the input widget that generated the event;
• centralizes unexpected connection-loss handling;
• fixes reconnect timer cancellation and adds a generation-guarded, self-rearming retry callback;
• validates sockets during keepalive and isolates per-connection errors;
• tracks a single keepalive timer;
• sends NOP and NUL using their exact raw wire forms;
• gives NOP precedence and uses NUL only as an exclusive fallback, preventing the fallback byte from consuming the next command on affected servers.

Validation

Validated on Windows with Tcl/Tk 8.6:

• focused reliability test: all 16 assertions passed;
• NOP precedence plus explicit blank-line bytes: FF F1 0D 0A;
• null-only fallback byte: 00;
• failed-send input retention: passed;
• focused-input connection routing: passed;
• reconnect timer replacement and retry rearming: passed;
• all 341 procedures in the two changed production files parsed and compiled successfully;
• git diff --check: clean.

The advanced packet capture also traced two missing commands through Input-Key,
Widget-Resolve, Input-Dispatch, Application-Text, Wire-Write, and Channel-Queue.
Both writes were accepted and drained on the same ordered TCP socket used by
commands that succeeded, but the missing commands never appeared in PennMUSH's
server command log. The loss followed a raw NUL keepalive. After making NOP and
NUL mutually exclusive, five consecutive numbered live tests passed.

Run the included test with:

tclkit-8.6.18-win64-intel-tcl.exe tests\socket-input-reliability.test.tcl

[adkins17]

Related split-session finding and patch: https://github.com/adkins17/potato/compare/46d18387bd6b156b85176fa588a60991cbba6235...feature/split-sessions.patch

While live-testing the socket/input reliability report, I found a second path that produced the same visible symptom but did not reach the socket layer. This path exists in the proposed split-session implementation for #506, not in current upstream Potato.

When moving from the right output pane to the left, pane activation was queued with after idle. A quick Return could therefore be handled by the output Text widget before the left input became active; the second Return worked because the idle callback had then completed. The fix places a dedicated activation bind tag after the Text class binding, so normal selection/link handling completes first and input focus becomes the final state synchronously.

The new regression starts on the right, switches to the left, generates one Return, and verifies that the left input owns that first send. A live packet-debugger test also recorded a single look as 6C 6F 6F 6B 0D 0A.

This does not replace or weaken the core fix in this issue: failed socket writes still need to propagate failure and retain input. It documents a separate UI-layer cause and links it to the patch where that behavior is introduced.