Skip to content

Stop publishing Failed for a drop that is about to be retried - #20

Open
adventures92 wants to merge 1 commit into
mainfrom
fix/failed-state-on-drop
Open

adventures92 wants to merge 1 commit into
mainfrom
fix/failed-state-on-drop

Conversation

@adventures92

Copy link
Copy Markdown
Owner

Two contradictions of the documented ConnectionState contract, one cause. Found by tests
written during the CI investigation and deliberately held back until they could be explained.

What the library actually did

reconnection = true:   [Disconnected, Connecting, Connected, Failed(PingTimeout), Reconnecting(1)]
reconnection = false:  expected Failed(PingTimeout), was Disconnected

Failed published for a drop about to be retried. guide/connection-state.md states
"Nothing recovers a Failed namespace automatically", and the guide's own example does this:

is ConnectionState.Failed -> when (state.error) {
    is SocketError.ConnectError -> refreshTokenAndReconnect()
    else -> showRetryButton()
}

A consumer following that prompted the user over a transient drop the library recovered from
moments later, unaided.

Failed lost when it was genuinely terminal. With reconnection off the same timeout settled
on a bare Disconnected, discarding the error — so a deliberate close() and a connection that
died produced identical state, with nothing in connectionState to tell them apart.

Cause

ConnectionManager.onEngineError wrote Failed straight to the client state, bypassing
updateAggregateState(). An engine error always arrives before the close it triggers, and that
close recomputes the aggregate — overwriting it. Meanwhile NamespaceSocketImpl.onEngineError
never touched state at all, so onEngineClose fell through to Disconnected and dropped the cause.

Both symptoms, one root: the error was written to the wrong level, at the wrong time.

Fix

Record the error; hand it to the namespaces at close time, and only when the close ends the
attempt. updateAggregateState() already derives Failed from namespace state, so the client
level follows without a second write.

  • While reconnecting the namespaces go to Reconnecting and the error stays on the errors
    flow, where a retried drop belongs. No Failed is published at all.
  • When nothing will retry, the namespace settles on Failed(cause) and the aggregate follows.
  • terminalError is cleared unconditionally, not only when consumed — otherwise an error from
    a connection since retried could resurface on a later clean close().

Invariants checked

  • State is still mutated only on the WorkQueue worker. The engine emits on the shared queue
    and these handlers run inline, so the new field is only ever touched there.
  • No public API movement. apiCheck passes; the change is confined to internal classes and
    ConnectionState.Failed already existed.

Tests

PingTimeoutConnectionStateTest pins both halves at the SocketClient level, which
EngineConnectionHeartbeatTest does not — that drives EngineConnection directly and asserts only
the errors flow. It needed startEmbedded to forward sendServerPings, a parameter it accepted
and silently dropped, which is why it could not compile before.

Verified across 8 consecutive full jvmTest runs, plus apiCheck, spotlessCheck, detekt,
check-docs.sh, and an mdbook build.

Documentation

Consumer-visible, so CHANGELOG.md gets a ### Fixed entry under ## [Unreleased].

Two guide corrections — the code was wrong and the prose was right, but the prose was imprecise:

  • reconnection.md conflated the retried and terminal cases. A ping timeout under
    reconnection = true is not terminal, and now says so.
  • connection-state.md now states that Disconnected versus Failed is how a consumer tells a
    deliberate shutdown from a dropped connection, without watching the errors flow alongside.

Worth releasing promptly

Sockit is now listed on klibs.io, showing 0.0.2
the affected version. Indexed projects pick up new versions the day after they reach Central.

🤖 Generated with Claude Code

Two contradictions of the documented ConnectionState contract, one cause.

With reconnection on, a ping timeout produced:

  [Disconnected, Connecting, Connected, Failed(PingTimeout), Reconnecting(1)]

Failed is documented as terminal — "nothing recovers a Failed namespace
automatically" — and the guide's own example reacts to it by refreshing a token
or showing a retry button. A consumer following that prompted the user over a
transient drop the library then recovered from unaided.

With reconnection off, the same timeout settled on a bare Disconnected rather
than Failed(PingTimeout): the error was discarded, leaving a deliberate close()
and a connection that died indistinguishable.

ConnectionManager.onEngineError wrote Failed straight to the client state,
bypassing updateAggregateState. An engine error always arrives before the close
it triggers, and that close recomputes the aggregate — overwriting it. Meanwhile
NamespaceSocketImpl.onEngineError never touched state at all, so onEngineClose
fell through to Disconnected and dropped the cause.

The error is now recorded and handed to the namespaces at close time, and only
when the close ends the attempt. updateAggregateState already derives Failed
from namespace state, so the client level follows without a second write. While
reconnecting the namespaces go to Reconnecting and the error stays on the errors
flow, where a retried drop belongs. terminalError is cleared unconditionally so
an error from a connection since retried cannot resurface on a later clean close.

State is still mutated only on the WorkQueue worker: the engine emits on the
shared queue and these handlers run inline.

PingTimeoutConnectionStateTest pins both halves at the SocketClient level, which
EngineConnectionHeartbeatTest does not — it drives EngineConnection directly and
asserts only the errors flow. It needed startEmbedded to forward sendServerPings,
which it accepted but silently dropped.

Verified across 8 consecutive full jvmTest runs. apiCheck clean: the change is
confined to internal classes and moves no public API.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant