Fixes #7316: Resynchronize the gateway after failed websocket config updates

[BobSong-dev]

Fixes #7316

Background

Failed WebSocket configuration application can leave gateway caches stale. A full snapshot with a valid payload followed by the same poison payload must not reset the retry budget on every replay and cause an unbounded reconnect loop.

Changes

• Address Aias00's review 5405630861: preserve failure counts across successful payloads and interleaved increments within a framed full-sync attempt. Reset only after the end frame and all registered application work succeed, independently of the sticky startup-readiness latch.
• Completion clears both the failure count and the pending cooldown. Failed or abandoned attempts cannot clear the budget through a late asynchronous callback. Follow-up d89fc0fe0 removes the remaining per-payload reset completely, including legacy MYSELF payloads, marked standalone snapshots, and increments. Only the complete framed-sync callback resets the budget. Legacy peers have no end-of-cycle acknowledgement: their budget stays conservative and failures continue to use cooldown/reconnect rather than treating one successful payload as full recovery. Config updates still apply; no manual restart is required to pick up repaired data. A legacy-only success does not clear the accumulated budget for future failures.
• Keep the first two failure-triggered reconnects and the 60-second cooldown at the cap. The health-check task remains alive and can request a fresh full snapshot without restarting the gateway. Unsupported envelopes/group/event types do not consume the apply-failure budget.
• Add valid-plus-poison replay/recovery coverage for framed, default unframed and marked standalone snapshot paths, deferred-completion coverage, and failed/abandoned completion-callback coverage. The two new unframed/standalone replay tests were also run against the previous implementation c0c53df12: both failed because a good payload reset the expected count of 1 to 0. Both pass with this fix. No assertions or retry limits are weakened.
• Merge current master (7925422b1) without rewriting published history. Resolve the client conflict by retaining master's namespace-scoped snapshot dispatch and this PR's protocol classification/recovery logic. The independent storage e2e infrastructure branch is not included.

Verification

• Local: ./mvnw.cmd -pl shenyu-sync-data-center/shenyu-sync-data-websocket -am test -Dtest=ShenyuWebsocketClientTest,InitialSyncStateTest,InitialSyncConnectionTest,*DataHandlerTest -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.javadoc.skip=true: BUILD SUCCESS; WebSocket 108 tests and plugin-base 7 tests, all zero failures/errors/skips; Checkstyle passed.
• Local: ./mvnw.cmd -pl shenyu-sync-data-center/shenyu-sync-data-websocket -am apache-rat:check: BUILD SUCCESS; RAT passed across the selected reactor.
• git diff --check origin/master...HEAD passed; master is included; working tree is clean.
• Tests intentionally log mocked poison/application errors and failed test-server connections; these did not fail the build.
• Not run locally: full-project or Docker IT/e2e suites.
• GitHub CI: new head d89fc0fe0 is pushed; its checks are not yet verified green. Earlier checks do not validate this new revision.

[Aias00]

The core of this is right and I want it: silently swallowing a failed config update is exactly what leaves a gateway running stale rules until someone restarts it, and close() → health-check reconnect → alreadySync = false → MYSELF is the correct way to force a full resynchronization. I verified that chain in the head version: onOpen:224-227 only sends MYSELF when alreadySync is false, and close():267 resets it, so the reconnect really does re-pull everything.

Requesting changes on one thing: the terminal state at MAX_CONSECUTIVE_SYNC_FAILURES has no recovery path.

handleSyncFailure:404 calls nowClose(), and nowClose():277-287 does two things beyond closing the socket:

this.manuallyClosed.set(true);
if (Objects.nonNull(timerTask)) { timerTask.cancel(); }

timerTask is the 10-second wheel-timer task whose doRun is healthCheck() (:196-199), and healthCheck():291-297 is the only thing that ever triggers a reconnect (RECONNECT_EXECUTOR.submit(this::doReconnect)). manuallyClosed is set to true here and never set back to false anywhere in the class.

So after three consecutive failures this client is permanently dead: no reconnect, no ping, no health check — and that includes the case the operator actually wants, which is "the bad config was fixed on the admin side". Nothing will ever pick the fix up; the only remedy is restarting the gateway. On master the same situation logged a warning, dropped one message, and kept the connection — so a later, good update still applied. I am not asking you to go back to that, but the recovery loop has to survive the give-up.

Concretely, one of these would work:

• Keep the timer alive. At the cap, stop closing on failure and go back to log-and-ignore, but leave healthCheck running. The connection stays up, and the next successful reconnect/MYSELF (or the next good message) re-syncs and resets consecutiveSyncFailures. This bounds the reconnect storm without giving up.
• Keep reconnecting, just stop re-syncing on failure. Set a resyncDisabled flag consulted by handleSyncFailure, let healthCheck/doReconnect continue with its existing capped backoff (calculateBackoff, up to 60s). A fixed admin config then recovers on its own.
• If you genuinely want a terminal state, it needs to be observable rather than a log line — but I would rather not have one at all here, because the failure it detects is "config the gateway cannot apply", which is precisely the state where you most want it to keep trying.

Note that testPoisonDataGivesUpAfterBoundedFailures asserts nowClose() is called, so whichever option you pick, that test changes with it.

Two other things worth considering while you are in here

1. Blast radius of the first failure. The counter is reset by any successful executor call (:56), which is good, but the very first failure now drops the connection and forces a full re-pull of every config group. Under a bad push, every gateway in the cluster does that at the same time. That is the intended fix for #7316 so I am not blocking on it — just be sure the log at WARN is enough for operators to correlate a reconnect storm with a bad config, since it is now a normal-ish event rather than an exceptional one.
2. The outer catch in onMessage now covers envelope parsing. JsonUtils.jsonToMap(result) and the RUNNING_MODE handling are inside the same try, so a frame that is not parseable JSON — or any other shape the client does not recognise — is now treated as a sync failure and closes the connection, where master ignored it. If admin can emit any non-JSON frame (heartbeat, error text), that is a new disconnect source.

[BobSong-dev]

The core of this is right and I want it: silently swallowing a failed config update is exactly what leaves a gateway running stale rules until someone restarts it, and close() → health-check reconnect → alreadySync = false → MYSELF is the correct way to force a full resynchronization. I verified that chain in the head version: onOpen:224-227 only sends MYSELF when alreadySync is false, and close():267 resets it, so the reconnect really does re-pull everything.

Requesting changes on one thing: the terminal state at MAX_CONSECUTIVE_SYNC_FAILURES has no recovery path.

handleSyncFailure:404 calls nowClose(), and nowClose():277-287 does two things beyond closing the socket:

this.manuallyClosed.set(true);
if (Objects.nonNull(timerTask)) { timerTask.cancel(); }

timerTask is the 10-second wheel-timer task whose doRun is healthCheck() (:196-199), and healthCheck():291-297 is the only thing that ever triggers a reconnect (RECONNECT_EXECUTOR.submit(this::doReconnect)). manuallyClosed is set to true here and never set back to false anywhere in the class.

So after three consecutive failures this client is permanently dead: no reconnect, no ping, no health check — and that includes the case the operator actually wants, which is "the bad config was fixed on the admin side". Nothing will ever pick the fix up; the only remedy is restarting the gateway. On master the same situation logged a warning, dropped one message, and kept the connection — so a later, good update still applied. I am not asking you to go back to that, but the recovery loop has to survive the give-up.

Concretely, one of these would work:

• Keep the timer alive. At the cap, stop closing on failure and go back to log-and-ignore, but leave healthCheck running. The connection stays up, and the next successful reconnect/MYSELF (or the next good message) re-syncs and resets consecutiveSyncFailures. This bounds the reconnect storm without giving up.
• Keep reconnecting, just stop re-syncing on failure. Set a resyncDisabled flag consulted by handleSyncFailure, let healthCheck/doReconnect continue with its existing capped backoff (calculateBackoff, up to 60s). A fixed admin config then recovers on its own.
• If you genuinely want a terminal state, it needs to be observable rather than a log line — but I would rather not have one at all here, because the failure it detects is "config the gateway cannot apply", which is precisely the state where you most want it to keep trying.

Note that testPoisonDataGivesUpAfterBoundedFailures asserts nowClose() is called, so whichever option you pick, that test changes with it.

Two other things worth considering while you are in here

1. Blast radius of the first failure. The counter is reset by any successful executor call (:56), which is good, but the very first failure now drops the connection and forces a full re-pull of every config group. Under a bad push, every gateway in the cluster does that at the same time. That is the intended fix for [BUG] Gateway ignores failed WebSocket configuration messages without resynchronizing #7316 so I am not blocking on it — just be sure the log at WARN is enough for operators to correlate a reconnect storm with a bad config, since it is now a normal-ish event rather than an exceptional one.
2. The outer catch in onMessage now covers envelope parsing. JsonUtils.jsonToMap(result) and the RUNNING_MODE handling are inside the same try, so a frame that is not parseable JSON — or any other shape the client does not recognise — is now treated as a sync failure and closes the connection, where master ignored it. If admin can emit any non-JSON frame (heartbeat, error text), that is a new disconnect source.

Thanks for the careful review — the terminal state was indeed a dead end, and option 1 is now
implemented (8e86395):

• At MAX_CONSECUTIVE_SYNC_FAILURES the client no longer calls nowClose(). It keeps the
  connection, stops closing on failure, and only logs further failures at ERROR ("keeping the
  connection and ignoring further failures until the next successful sync; an admin-side fix
  will be picked up by the running health check"). healthCheck and its capped backoff stay
  alive, and the next successful apply resets consecutiveSyncFailures, restoring the normal
  bounded recovery. No terminal state, no restart needed after an admin-side fix.
• testPoisonDataGivesUpAfterBoundedFailures is rewritten as
  testPoisonDataKeepsConnectionAfterBoundedFailures: failures 1-2 close the connection,
  failure 3+ is suppressed without closing, a successful apply resets the counter, and a
  following failure closes again (recovery restored). nowClose is verified to never run.
• Your second point is also addressed: envelope parsing (jsonToMap), RUNNING_MODE
  handling, and frames that fail to parse into a recognized config message are back to
  log-and-ignore, so a non-JSON heartbeat or error frame is no longer a disconnect source.
  Only the failure to apply a recognized config message now triggers the bounded resync.
• The first-failure WARN now names group, event type, consecutive-failure count and the
  server URI, and calls out a possible bad config push, so operators can correlate a
  reconnect burst with a bad push.

Verification: ./mvnw test checkstyle:check -pl shenyu-sync-data-center/shenyu-sync-data-websocket -am
— BUILD SUCCESS, ShenyuWebsocketClientTest 26/26 green. Not run locally: e2e.

[Aias00]

Two recovery-path issues still block this change. First, a syntactically valid config frame with an unknown eventType reaches AbstractDataHandler, throws from DataEventTypeEnum.acquireByName, and is counted as an apply failure; older gateways will close/resync on newer event types instead of preserving the documented log-and-ignore compatibility behavior. Please classify unsupported event types separately and assert they do not close the socket or consume the failure budget. Second, once MAX_CONSECUTIVE_SYNC_FAILURES is reached, handleSyncFailure keeps the socket open but neither closes nor sends MYSELF; health checks only ping/report instance info, while only MYSELF triggers a full admin snapshot. The skipped config can therefore remain stale indefinitely. Please add a bounded/backoff resync path at the cap and test automatic recovery without a manual restart.

[Aias00]

Re-check: the recovery-path issue I raised is still open — a syntactically valid config frame with an unknown eventType still reaches AbstractDataHandler, where it is treated as data rather than rejected. Please either reject unknown event types at the boundary or explain why reaching the handler is acceptable.

[Aias00]

Re-reviewed — both recovery-path issues I raised are fixed.

The unknown-eventType case is now handled before it can do damage: handleResult resolves ConfigGroupEnum.acquireByName and DataEventTypeEnum.acquireByName(eventType) up front and, on any RuntimeException, logs and ignores the frame instead of letting the throw propagate into a counted apply failure. Unparseable frames and non-object payloads are handled the same way, and onMessage no longer drops the connection for a protocol-shape mismatch.

On top of that, handleSyncFailure gives real recovery: bounded consecutive-failure counting, a backoff window, and a scheduled full resync — which is the actual point of the PR. ShenyuWebsocketClientTest covers it. CI is green. Approving.

[sunnysabor]

One recovery-loop case remains unbounded. handleResult resets consecutiveSyncFailures after any successful websocketDataHandler.executor call (line 464). Initial synchronization is delivered as multiple WebsocketSyncFrame payloads and InitialSyncState.accept applies each payload in sequence. If every full snapshot contains a valid payload before the same poison payload, the valid payload resets the counter to zero, then the poison payload increments it to one and closes the connection. The next reconnect replays the same sequence, so the counter never reaches MAX_CONSECUTIVE_SYNC_FAILURES, nextSyncRetryAt is never set, and the 60-second cooldown/full-resync path is bypassed. This can still produce an endless reconnect storm for a deterministic bad item later in each snapshot.

testHealthCheckRetriesFullSyncAfterFailureCap repeats only the poison payload, so it does not exercise this ordering. Please preserve the failure count across individual successful frames within a full snapshot and reset it only when the full snapshot completes successfully (or otherwise track repeated failure of the same item); add a regression test that applies a good payload followed by the same failing payload on each replay and verifies the bounded retry path is reached.

[sunnysabor]

I also checked the current merge conflict against upstream/master (09c6a528) and head cca4e49b with git merge-tree. The only conflicting path is ShenyuWebsocketClient.java; the test file and all other changed paths auto-merge. This should keep the rebase resolution localized to combining the client changes.

[sunnysabor]

Additional reproduction for the failure-budget concern at this head (cca4e49b): in a detached worktree I temporarily added a test that models three full-sync attempts, each with sequence 0 successfully applying and sequence 1 throwing for the same poison payload. I expected only the first two attempts to close and the third to reach the configured cap/backoff. The focused command

./mvnw -B -ntp -pl shenyu-sync-data-center/shenyu-sync-data-websocket -am -Dtest=ShenyuWebsocketClientTest -DfailIfNoTests=false -Djacoco.skip=true -Dmaven.javadoc.skip=true test

ran 31 tests with 30 passing and the temporary assertion failing: close() was called 3 times instead of 2. Each sequence-0 success resets consecutiveSyncFailures at ShenyuWebsocketClient.java:464 before the poison frame fails, so the count stays at 1 across reconnects and never reaches the cap. I removed the temporary test afterward. Could the failure budget be reset only once the complete initial snapshot is successfully applied (or otherwise track snapshot-level failures), while retaining the per-message reset for post-sync incremental updates?

[Aias00]

The sync failure counter is reset after any successful payload. A full snapshot that always contains a valid payload followed by a poison payload therefore resets the budget on every reconnect and can loop indefinitely. Please track failure at the full-sync attempt level, or reset only after the complete snapshot succeeds, and add a valid-plus-poison regression test.

[BobSong-dev]

The sync failure counter is reset after any successful payload. A full snapshot that always contains a valid payload followed by a poison payload therefore resets the budget on every reconnect and can loop indefinitely. Please track failure at the full-sync attempt level, or reset only after the complete snapshot succeeds, and add a valid-plus-poison regression test.

I will fix it.Thanks for your review.

[Aias00]

The updated head still resets consecutiveSyncFailures after any successful payload. A full snapshot that always applies a valid payload before a poison payload resets the budget on every reconnect, so recovery can loop indefinitely. Please reset only after the complete sync cycle succeeds, and add a valid-plus-poison snapshot regression test.

[BobSong-dev]

The updated head still resets consecutiveSyncFailures after any successful payload. A full snapshot that always applies a valid payload before a poison payload resets the budget on every reconnect, so recovery can loop indefinitely. Please reset only after the complete sync cycle succeeds, and add a valid-plus-poison snapshot regression test.

sorry,I missed it.Now it is healthy.

[Aias00]

Reviewed the updated head. Failure accounting is now reset only after the complete initial-sync attempt succeeds, so a valid payload followed by a poison payload can no longer reset the retry budget indefinitely. The previous blocker is resolved.