[TT-17486] fix: stop liveness probe blocking on node re-registration#8314
Merged
Conversation
Contributor
🎯 Recommended Merge TargetsBased on JIRA ticket TT-17486: [regression] /hello and /ready Return Incorrect Health Status When Redis Fails First Fix Version: Tyk 5.8.15Required:
Fix Version: Tyk 5.13.1Required:
Fix Version: Tyk 5.14.0
Required:
📋 Workflow
|
Contributor
|
This PR fixes a critical bug where the gateway's liveness ( The root cause was that the liveness probe would trigger a blocking, unbounded re-registration attempt when the Dashboard returned a
Extensive new regression tests have been added to verify the fix and prevent similar issues in the future. Files Changed Analysis
Architecture & Impact AssessmentThis PR addresses a critical flaw in the gateway's health checking mechanism, significantly improving its reliability and stability, especially in orchestrated environments like Kubernetes.
Architectural Flow ChangeThe following diagrams illustrate how the liveness probe's interaction with the Dashboard service has changed. Before (Problematic Flow): A 403 response from the Dashboard caused the liveness probe to enter an unbounded, blocking retry loop for re-registration. sequenceDiagram
participant K8s as Kubernetes Probe
participant Gateway as "/hello endpoint"
participant GHC as "gatherHealthChecks()"
participant Ping as "DashService.Ping()"
participant SHB as "sendHeartBeat()"
participant Register as "DashService.Register()"
participant Dashboard
K8s->>Gateway: GET /hello
Gateway->>GHC: run checks
GHC->>Ping: check dashboard
Ping->>SHB: send heartbeat
SHB->>Dashboard: GET /ping
Dashboard-->>SHB: 403 Forbidden (Redis down)
SHB->>Register: Attempt re-registration
loop Unbounded Retry
Register->>Dashboard: POST /register/node
Dashboard-->>Register: 409 Conflict (Redis down)
end
Note right of Register: Blocks forever!
Note right of GHC: wg.Wait() never returns
Note right of Gateway: Serves stale 'pass' status
After (Fixed Flow): The liveness probe now treats a 403 as a sign of reachability and succeeds quickly. Re-registration is handled separately by the background heartbeat loop. sequenceDiagram
participant K8s as Kubernetes Probe
participant Gateway as "/hello endpoint"
participant GHC as "gatherHealthChecks()"
participant Ping as "DashService.Ping()"
participant DHB as "doHeartBeat()"
participant Dashboard
K8s->>Gateway: GET /hello
Gateway->>GHC: run checks (with timeout)
GHC->>Ping: check dashboard
Ping->>DHB: send heartbeat
DHB->>Dashboard: GET /ping
Dashboard-->>DHB: 403 Forbidden
DHB-->>Ping: errHeartBeatForbidden
Ping-->>GHC: return nil (success)
GHC->>Gateway: return current status
Gateway-->>K8s: 200 OK
%% Separate, asynchronous heartbeat loop handles recovery
participant HeartbeatLoop
participant SHB as "sendHeartBeat()"
participant Register as "DashService.Register()"
HeartbeatLoop->>SHB: send heartbeat
SHB->>DHB: send heartbeat
DHB->>Dashboard: GET /ping
Dashboard-->>DHB: 403 Forbidden
DHB-->>SHB: errHeartBeatForbidden
SHB->>Register: Attempt re-registration (recovers node)
Scope Discovery & Context ExpansionThe impact of this change is concentrated within the The changes are self-contained and do not appear to have dependencies on other microservices beyond the Gateway-Dashboard interaction. The new tests provide excellent context by programmatically defining the exact failure scenarios that this PR is intended to solve. Metadata
Powered by Visor from Probelabs Last updated: 2026-06-17T09:25:45.129Z | Triggered by: pr_updated | Commit: 87874d8 💡 TIP: You can chat with Visor using |
Contributor
✅ Security Check PassedNo security issues found – changes LGTM. Architecture Issues (1)
✅ Quality Check PassedNo quality issues found – changes LGTM. Powered by Visor from Probelabs Last updated: 2026-06-17T09:25:37.839Z | Triggered by: pr_updated | Commit: 87874d8 💡 TIP: You can chat with Visor using |
MFCaballero
approved these changes
Jun 12, 2026
tbuchaillot
reviewed
Jun 12, 2026
sredxny
reviewed
Jun 12, 2026
sredxny
approved these changes
Jun 12, 2026
MFCaballero
approved these changes
Jun 16, 2026
When Redis failed while the Dashboard was still up, the Dashboard answered the gateway heartbeat with 403 and registration with 409 + Status "Error". The liveness probe (Ping) shared sendHeartBeat, whose 403 branch calls Register() — since TT-16865 an unbounded retry loop — with context.Background(). The dashboard probe goroutine never returned, gatherHealthChecks' wg.Wait() never released, and /hello and /ready served the last pre-failure "pass" snapshot for the whole outage, so Kubernetes kept routing traffic to gateways without Redis. - Split the probe path from the heartbeat path: Ping() never calls Register(); a 403 means the dashboard is reachable and is reported healthy (v5.8.13 parity). Re-registration stays owned by the StartBeating loop. - Bound the probe with a context derived from gw.ctx, capped at min(check_duration, 5s). - Bound the gatherHealthChecks barrier by the check interval; commit a snapshot of completed probes and report the rest as "health check timed out" so no hung probe can wedge the loop.
… ctx in probe Code-review follow-ups: a regression test now pins the recovery path the probe relies on (heartbeat loop re-registers on 403), and Ping falls back to context.Background() when the gateway was built without a context instead of panicking.
…ests into existing suites The probe timeout is now half the health-check interval instead of a fixed 5s cap, so it scales with operator config and the probe always reports its own error before the round's barrier expires. The TT-17486 regression tests move from tt17486_repro_test.go into dashboard_register_test.go and health_check_test.go.
Review follow-up: rpc.Login takes no context but is internally bounded (30s call timeout, max 3 retries, singleflight), so the bounded barrier is sufficient; the 1.7s test assertion sits between the 1s expected bound and the 2s pre-fix behaviour.
…parameter lint The hanging-dashboard assertion now computes probeTimeout (checkDuration/2, mirroring Ping) plus named scheduling slack, with a guard that the sum stays below the pre-fix 2s behaviour — no magic 1700ms. Unused ResponseWriter parameters renamed to _ for revive.
Adds a writeBody helper alongside writeJSON so the raw-JSON dashboard responses keep wire fidelity while satisfying errcheck.
- Name the 10s fallback (defaultHealthCheckInterval) per tbuchaillot. - Use NewTimer with deferred Stop for the barrier timeout per sredxny (cosmetic on Go >= 1.23, where time.After timers are collectable). - Widen the hanging-dashboard test to checkDuration 4s with 1s scheduling slack for loaded CI runners, keeping the guard that the bound stays below the pre-fix timeout.
kofoworola
force-pushed
the
fix/TT-17486/health-probe-no-reregister
branch
from
June 17, 2026 09:23
c1d46fd to
87874d8
Compare
kofoworola
enabled auto-merge (squash)
June 17, 2026 09:23
|
SentinelOne CNS Hardcoded Secret Detector SentinelOne CNS is a cloud-agnostic, agentless CSPM & CWPP solution that continuously detects and prevents vulnerabilities that have the highest probability of being exploited in Azure, AWS, Google Cloud, and Kubernetes. |
Contributor
🚨 Jira Linter FailedCommit: The Jira linter failed to validate your PR. Please check the error details below: 🔍 Click to view error detailsNext Steps
This comment will be automatically deleted once the linter passes. |
|
Contributor
|
/release to release-5.8 |
Contributor
|
/release to release-5.13 |
Contributor
|
✅ Cherry-pick successful. A PR was created: #8327 |
7 tasks
Contributor
|
✅ Cherry-pick successful. A PR was created: #8328 |
7 tasks
kofoworola
added a commit
that referenced
this pull request
Jun 17, 2026
…on node re-registration (#8314) (#8327) [TT-17486] fix: stop liveness probe blocking on node re-registration (#8314) ## Description `/hello` and `/ready` served a stale "pass" status forever when Redis failed before the Dashboard (regression in 5.8.14 / 5.13.0). Root cause: the liveness probe `Ping()` shared `sendHeartBeat`, whose 403 branch calls `DashService.Register(ctx)` — since #8188 an unbounded 5s retry loop — with `context.Background()`. With Redis down and the Dashboard up, the Dashboard answers the heartbeat with 403 and every registration attempt with `409 + Status:"Error"`, so `Register()` never returned, the dashboard probe goroutine never released `gatherHealthChecks`' `wg.Wait()`, the cached health map was never refreshed, and the synchronous ticker loop wedged for the duration of the outage. Kubernetes kept routing traffic to Gateways without Redis. The fix, in three layers (none of which reverts the TT-16865 behaviour — startup registration retry is unchanged): 1. **The probe no longer triggers re-registration.** The heartbeat HTTP exchange is extracted into `doHeartBeat()`, returning a sentinel on 403. `sendHeartBeat()` (heartbeat loop) keeps 403 → `Register(ctx)`; `Ping()` treats 403 as "dashboard reachable → healthy", preserving v5.8.13 endpoint payloads exactly. Re-registration is owned by the `StartBeating` loop. 2. **The probe is bounded.** `Ping()` runs under a context derived from `gw.ctx`, bounded at half the configured `liveness_check.check_duration` (so the probe always reports its own error before the round's barrier expires), so no probe request can outlive a check interval. 3. **The health-check barrier is wedge-proof.** `gatherHealthChecks()` waits for its probes only up to the check interval, commits a race-safe snapshot of what completed, and reports any unfinished component as `fail` / `"health check timed out"` — no single hung dependency can ever freeze `/hello` and `/ready` again, whatever the failure order. ## Related Issue https://tyktech.atlassian.net/browse/TT-17486 ## Motivation and Context In Tyk Pro deployments on Kubernetes the readiness probe is the only signal gating traffic to Gateway Pods. With this regression, a Gateway that lost Redis kept answering `/ready` with `200/pass` indefinitely, so traffic continued to be routed to instances that could not serve it. The failure was order-dependent (only when Redis died before the Dashboard), which is what made it hard to diagnose; the storage-library theory was ruled out (all good/bad tags pin `storage v1.3.1`; `health_check.go` is byte-identical across them — only the #8188 `Register()` rewrite tracks the good/bad split). ## How This Has Been Tested TDD throughout — every regression test was watched failing on the unfixed code first: - `TestPing_RedisDownDashboardUp_DoesNotBlockOrReRegister` — stubs the Dashboard's Redis-down wire behaviour (heartbeat 403, registration `409 + Status:"Error"`). Before: `Ping()` blocked >10s retrying registration; after: returns `nil` in ~20ms with **zero** registration attempts. - `TestPing_HangingDashboard_BoundedByTimeout` — unresponsive Dashboard with the production-default 30s client; probe now fails at ~5s. - `TestGatherHealthChecks_HungProbeDoesNotWedgeTheLoop` — injected `DashService` whose `Ping` blocks forever; the round completes within the interval with `redis: pass`, `dashboard: fail/"health check timed out"`. - `TestSendHeartBeat_Forbidden_ReRegisters` — pins the recovery path: the heartbeat loop still re-registers on 403 (node identity and nonce refreshed). - Characterization tests written before the refactor pin preserved behaviour: 200 heartbeat updates the nonce; transport errors still report `"dashboard is down? Heartbeat is failing"`. - Full dashboard-register + health-check suites green, including the TT-16865 tests (`TestRegister_Retries`, `TestRegister_DuplicateSession409`); `-race` clean on the new tests; `go vet`/`gofmt` clean. - Differential validation of the root cause: the Redis-down probe test passes on a v5.8.13 worktree unmodified and fails on 5.8.14+ code. Not covered here (tracked in the ticket's test plan for QA/e2e): full-stack failure-order matrix, recovery/flapping soak, and the Kubernetes readiness scenario. ## Types of changes - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist - [x] I ensured that the documentation is up to date - [x] I explained why this PR updates go.mod in detail with reasoning why it's required (go.mod untouched) - [x] I would like a code coverage CI quality gate exception and have explained why (not needed) --------- Co-authored-by: Kofo Okesola <[email protected]> Co-authored-by: Ilija Bojanovic <[email protected]> [TT-17486]: https://tyktech.atlassian.net/browse/TT-17486?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: Matias <[email protected]> Co-authored-by: Kofo Okesola <[email protected]> Co-authored-by: Ilija Bojanovic <[email protected]>
kofoworola
added a commit
that referenced
this pull request
Jun 17, 2026
… on node re-registration (#8314) (#8328) [TT-17486] fix: stop liveness probe blocking on node re-registration (#8314) ## Description `/hello` and `/ready` served a stale "pass" status forever when Redis failed before the Dashboard (regression in 5.8.14 / 5.13.0). Root cause: the liveness probe `Ping()` shared `sendHeartBeat`, whose 403 branch calls `DashService.Register(ctx)` — since #8188 an unbounded 5s retry loop — with `context.Background()`. With Redis down and the Dashboard up, the Dashboard answers the heartbeat with 403 and every registration attempt with `409 + Status:"Error"`, so `Register()` never returned, the dashboard probe goroutine never released `gatherHealthChecks`' `wg.Wait()`, the cached health map was never refreshed, and the synchronous ticker loop wedged for the duration of the outage. Kubernetes kept routing traffic to Gateways without Redis. The fix, in three layers (none of which reverts the TT-16865 behaviour — startup registration retry is unchanged): 1. **The probe no longer triggers re-registration.** The heartbeat HTTP exchange is extracted into `doHeartBeat()`, returning a sentinel on 403. `sendHeartBeat()` (heartbeat loop) keeps 403 → `Register(ctx)`; `Ping()` treats 403 as "dashboard reachable → healthy", preserving v5.8.13 endpoint payloads exactly. Re-registration is owned by the `StartBeating` loop. 2. **The probe is bounded.** `Ping()` runs under a context derived from `gw.ctx`, bounded at half the configured `liveness_check.check_duration` (so the probe always reports its own error before the round's barrier expires), so no probe request can outlive a check interval. 3. **The health-check barrier is wedge-proof.** `gatherHealthChecks()` waits for its probes only up to the check interval, commits a race-safe snapshot of what completed, and reports any unfinished component as `fail` / `"health check timed out"` — no single hung dependency can ever freeze `/hello` and `/ready` again, whatever the failure order. ## Related Issue https://tyktech.atlassian.net/browse/TT-17486 ## Motivation and Context In Tyk Pro deployments on Kubernetes the readiness probe is the only signal gating traffic to Gateway Pods. With this regression, a Gateway that lost Redis kept answering `/ready` with `200/pass` indefinitely, so traffic continued to be routed to instances that could not serve it. The failure was order-dependent (only when Redis died before the Dashboard), which is what made it hard to diagnose; the storage-library theory was ruled out (all good/bad tags pin `storage v1.3.1`; `health_check.go` is byte-identical across them — only the #8188 `Register()` rewrite tracks the good/bad split). ## How This Has Been Tested TDD throughout — every regression test was watched failing on the unfixed code first: - `TestPing_RedisDownDashboardUp_DoesNotBlockOrReRegister` — stubs the Dashboard's Redis-down wire behaviour (heartbeat 403, registration `409 + Status:"Error"`). Before: `Ping()` blocked >10s retrying registration; after: returns `nil` in ~20ms with **zero** registration attempts. - `TestPing_HangingDashboard_BoundedByTimeout` — unresponsive Dashboard with the production-default 30s client; probe now fails at ~5s. - `TestGatherHealthChecks_HungProbeDoesNotWedgeTheLoop` — injected `DashService` whose `Ping` blocks forever; the round completes within the interval with `redis: pass`, `dashboard: fail/"health check timed out"`. - `TestSendHeartBeat_Forbidden_ReRegisters` — pins the recovery path: the heartbeat loop still re-registers on 403 (node identity and nonce refreshed). - Characterization tests written before the refactor pin preserved behaviour: 200 heartbeat updates the nonce; transport errors still report `"dashboard is down? Heartbeat is failing"`. - Full dashboard-register + health-check suites green, including the TT-16865 tests (`TestRegister_Retries`, `TestRegister_DuplicateSession409`); `-race` clean on the new tests; `go vet`/`gofmt` clean. - Differential validation of the root cause: the Redis-down probe test passes on a v5.8.13 worktree unmodified and fails on 5.8.14+ code. Not covered here (tracked in the ticket's test plan for QA/e2e): full-stack failure-order matrix, recovery/flapping soak, and the Kubernetes readiness scenario. ## Types of changes - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist - [x] I ensured that the documentation is up to date - [x] I explained why this PR updates go.mod in detail with reasoning why it's required (go.mod untouched) - [x] I would like a code coverage CI quality gate exception and have explained why (not needed) --------- Co-authored-by: Kofo Okesola <[email protected]> Co-authored-by: Ilija Bojanovic <[email protected]> [TT-17486]: https://tyktech.atlassian.net/browse/TT-17486?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: Matias <[email protected]> Co-authored-by: Kofo Okesola <[email protected]> Co-authored-by: Ilija Bojanovic <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
/helloand/readyserved a stale "pass" status forever when Redis failed before the Dashboard (regression in 5.8.14 / 5.13.0).Root cause: the liveness probe
Ping()sharedsendHeartBeat, whose 403 branch callsDashService.Register(ctx)— since #8188 an unbounded 5s retry loop — withcontext.Background(). With Redis down and the Dashboard up, the Dashboard answers the heartbeat with 403 and every registration attempt with409 + Status:"Error", soRegister()never returned, the dashboard probe goroutine never releasedgatherHealthChecks'wg.Wait(), the cached health map was never refreshed, and the synchronous ticker loop wedged for the duration of the outage. Kubernetes kept routing traffic to Gateways without Redis.The fix, in three layers (none of which reverts the TT-16865 behaviour — startup registration retry is unchanged):
doHeartBeat(), returning a sentinel on 403.sendHeartBeat()(heartbeat loop) keeps 403 →Register(ctx);Ping()treats 403 as "dashboard reachable → healthy", preserving v5.8.13 endpoint payloads exactly. Re-registration is owned by theStartBeatingloop.Ping()runs under a context derived fromgw.ctx, bounded at half the configuredliveness_check.check_duration(so the probe always reports its own error before the round's barrier expires), so no probe request can outlive a check interval.gatherHealthChecks()waits for its probes only up to the check interval, commits a race-safe snapshot of what completed, and reports any unfinished component asfail/"health check timed out"— no single hung dependency can ever freeze/helloand/readyagain, whatever the failure order.Related Issue
https://tyktech.atlassian.net/browse/TT-17486
Motivation and Context
In Tyk Pro deployments on Kubernetes the readiness probe is the only signal gating traffic to Gateway Pods. With this regression, a Gateway that lost Redis kept answering
/readywith200/passindefinitely, so traffic continued to be routed to instances that could not serve it. The failure was order-dependent (only when Redis died before the Dashboard), which is what made it hard to diagnose; the storage-library theory was ruled out (all good/bad tags pinstorage v1.3.1;health_check.gois byte-identical across them — only the #8188Register()rewrite tracks the good/bad split).How This Has Been Tested
TDD throughout — every regression test was watched failing on the unfixed code first:
TestPing_RedisDownDashboardUp_DoesNotBlockOrReRegister— stubs the Dashboard's Redis-down wire behaviour (heartbeat 403, registration409 + Status:"Error"). Before:Ping()blocked >10s retrying registration; after: returnsnilin ~20ms with zero registration attempts.TestPing_HangingDashboard_BoundedByTimeout— unresponsive Dashboard with the production-default 30s client; probe now fails at ~5s.TestGatherHealthChecks_HungProbeDoesNotWedgeTheLoop— injectedDashServicewhosePingblocks forever; the round completes within the interval withredis: pass,dashboard: fail/"health check timed out".TestSendHeartBeat_Forbidden_ReRegisters— pins the recovery path: the heartbeat loop still re-registers on 403 (node identity and nonce refreshed)."dashboard is down? Heartbeat is failing".TestRegister_Retries,TestRegister_DuplicateSession409);-raceclean on the new tests;go vet/gofmtclean.Not covered here (tracked in the ticket's test plan for QA/e2e): full-stack failure-order matrix, recovery/flapping soak, and the Kubernetes readiness scenario.
Types of changes
Checklist