Skip to content

refactor(client): consolidate Subscribe/SubscribeList into shared subscribeCore - #145

Closed
benitogf wants to merge 1 commit into
mainfrom
refactor/subscribe-generic-core
Closed

refactor(client): consolidate Subscribe/SubscribeList into shared subscribeCore#145
benitogf wants to merge 1 commit into
mainfrom
refactor/subscribe-generic-core

Conversation

@benitogf

@benitogf benitogf commented Jun 9, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #144

The single-object and list-glob subscription paths in the Go client were ~280 lines of byte-identical code wrapping a small per-variant decode step. PR #143 itself had to fix the cancel-suppression bug twice in lockstep — the failure mode duplication invites. Now consolidated into one shared core, with each entry point as a thin wrapper that injects the variant-specific decode + delivery.

Behavior is byte-equivalent end-to-end:

  • single-object decode: any failure is fatal (terminates the read loop, triggers reconnect)
  • list decode: parse failure is fatal; per-item unmarshal failures fire the error callback and continue
  • cancel-time error suppression applied at both connect and read failure sites
  • cache + retry-count lifecycle preserved exactly as before

Net code change: client/subscribe.go drops 560 → 433 lines.

Test plan

  • Full race-enabled test suite green across all packages at 10x sequential iterations locally
  • PR fix(client): suppress Subscribe OnError after context cancellation #143's cancel-suppression regression tests (4 subtests for Subscribe/SubscribeList across dial-failure and read-error paths) continue to fail without the production fix and pass with it — verified by stashing the fix on the new code shape
  • Multi-round adversarial self-review converged after two consecutive clean passes (rounds 3 and 4 returned zero confirmed findings)
  • All five behavioral invariants (single-fatal-decode, list-skip-on-item-error, both-sites-cancel-suppressed, cache-on-error-path, retry-reset-placement) explicitly verified line-by-line against the pre-refactor baseline

This refactor is stacked on top of #143 — when that merges, this PR rebases onto main cleanly.


🤖 Generated with Claude Code

…scribeCore

The two halves of client/subscribe.go were ~280 lines of byte-identical
code wrapping a small per-variant decode step. PR #143 itself had to
apply the cancel-suppression guard in both copies — that's the bug
duplication invites.

Subscribe and SubscribeList now thinly wrap a single non-generic
subscribeCore that holds connect/readLoop/waitRetry/startCloseWatcher/
isClosing. The only varying piece — message decode + per-item
unmarshal policy + typed OnMessage delivery — is a closure each entry
point attaches as core.handle. The closure captures the user payload
type T directly, so the core itself needs no type parameters.

Semantic equivalence preserved end-to-end:
- Subscribe handle: any decode failure is fatal (terminates read
  loop, fires OnError, triggers reconnect after backoff)
- SubscribeList handle: PatchList parse error fatal; per-item
  unmarshal errors fire OnError and skip, continuing the result
- Cancel-time OnError suppression via isClosing() guard applied in
  both connect() and readLoop() error paths
- cache assignment happens before error check (preserves
  pre-refactor messages.Patch / PatchList semantics)
- retryCount reset only after successful decode, before OnMessage

Net diff: -112 net lines (client/subscribe.go drops 560→433). The
existing regression test suite (TestSubscribeSuppressesOnErrorAfterCancel,
TestWebSocketReadListFilterAllowsIndividualSubscribe, and the rest of
the client suite) continues to fail without the cancel-suppression
guard and pass with it — verified by stashing the production fix.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@benitogf

benitogf commented Jun 9, 2026

Copy link
Copy Markdown
Owner Author

Closing after a tougher review on the headline numbers.

The PR description claimed ~280 LOC of duplication; the actual net reduction was 127 lines. Honest accounting:

  • ~150 lines of cross-method duplication genuinely eliminated (connect, isClosing, readLoop, waitRetry, startCloseWatcher, logPrefix, retry loop body).
  • The per-variant handle logic ended up about the same size — the duplicated Meta[T]{Created, Updated, Index, Data} wrapping and the decode+OnError pattern still appear in both closures.
  • New cost: function-pointer dispatch via s.handle, plus a runtime-attached closure that the reader has to trace back to which entry point wired it.

The actual bug class that motivated the entry — the cancel-suppression fix needing two copies in lockstep — was already addressed at the per-site level on #143 with regression coverage that catches a one-side-only regression. The Meta-wrapping duplication that remains is small and parallel-structured.

If consolidation comes back, go:generate (write the single version, generate the list version) is the lower-cost shape — no dispatch indirection, no closure capture.

@benitogf benitogf closed this Jun 9, 2026
@benitogf
benitogf deleted the refactor/subscribe-generic-core branch June 9, 2026 07:25

@CBosch101 CBosch101 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean consolidation of the two near-identical subscription paths into one subscribeCore, with the variant-specific decode/delivery injected as a handle closure. Exported Subscribe[T]/SubscribeList[T] signatures are unchanged, so callers are unaffected. No blockers.

Verified

  • Behavior invariants preserved line-for-line: single-object decode fatal-on-error; list parse fatal but per-item unmarshal fires OnError and continues; cache reassigned before the error check; retryCount=0 reset placed before OnMessage in both variants; cancel-suppression guard (isClosing()) intact at both connect() and readLoop() sites.
  • meta import dropped cleanly — obj's type is now inferred from messages.Patch/PatchList; build and vet clean.
  • run() loop matches both pre-refactor loops exactly; handle is always assigned before run(), no nil-call path.
  • go test -race ./client/ . green, including the PR #143 cancel-suppression regression subtests.

The reduced test independence on the Subscribe-vs-SubscribeList axis (both wrappers now share connect/readLoop) is already documented in the updated test comments — agreed it's the right trade-off given the connect/readLoop axis still gates guard regressions.


🤖 Generated with Claude Code

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.

Consolidate Subscribe/SubscribeList duplicate implementations

2 participants