fix(connect): decode registration into a fresh WgData to prevent stale hcStatus on reconnect#413
Open
lafoush wants to merge 1 commit into
Open
fix(connect): decode registration into a fresh WgData to prevent stale hcStatus on reconnect#413lafoush wants to merge 1 commit into
lafoush wants to merge 1 commit into
Conversation
…e hcStatus handleConnect decoded the newt/wg/connect registration payload into the persistent n.wgData with json.Unmarshal(jsonData, &n.wgData). Decoding a JSON array into an existing non-empty slice merges element-by-element by position, and a JSON null is a no-op for non-pointer fields. So on reconnect, when the server returns the health-check rows in a different order (the config query has no ORDER BY), a target whose hcStatus is null inherits the stale expected status code from whatever target previously occupied that slice slot. Concretely: a resource with no custom expected code (hcStatus null, so newt uses the 2xx default) could inherit a sibling resource's 401 when the two shared a backend and the rows reordered on reconnect, leaving the resource permanently Unhealthy until newt restarted. handleSync was unaffected because it decodes into a fresh local SyncData. Decode into a fresh WgData and assign on success. Adds a regression test covering the reused-slice hazard and the fresh-decode fix.
Author
|
For reference, we are hot patching Newt in our product, the patch is getting rolled out to a portion of our fleet for testing. I'll update this issue if our monitoring raise any other issue. |
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
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.
Problem
A resource with a health check can be reported permanently Unhealthy after a reconnect, even though it responds correctly, until newt is restarted. It's intermittent and only happens on reconnect (never on a clean boot).
Root cause
handleConnectdecodes thenewt/wg/connectregistration payload into the persistentn.wgData:Go's
encoding/jsonmerges a JSON array into an existing non-empty slice element-by-element, by position, and a JSONnullis a no-op for a non-pointer field such ashealthcheck.Config.Status(hcStatus, anint).n.wgDatais reused across reconnects, so:wgData.HealthCheckTargets, e.g. slot 0 ={id:47, hcStatus:401}, slot 1 ={id:5, hcStatus:null}.ORDER BY), e.g.[{id:5, hcStatus:null}, {id:47, hcStatus:401}].id:5,hcStatus:null) into old-slot-0 (id:47,hcStatus:401).idis overwritten to 5, but thenullhcStatusdoes not overwrite the retained401.401. Its check (GET /manifest.json→200) fails forever withunexpected status code: 200 (expected: 401).handleSyncis unaffected because it decodes into a fresh localSyncData. A clean boot is unaffected becausewgDatastarts empty. This matches the observed profile exactly: reconnect-only, intermittent (needs a row reorder), self-heals on restart.Fix
Decode the registration payload into a fresh
WgDataand assign it on success, so no state is ever carried over from a previous connection:n.wgDatais written nowhere else, so a wholesale replace is safe.Test
Adds
newt/wgdata_reconnect_test.go, which pins both halves:WgDatareproduces the stale401inheritance;WgData(whathandleConnectnow does) keeps target 5 free of target 47's expected code.go build ./...,go vet ./newt/, and the full./newt/+./healthcheck/suites pass.