Skip to content

fix: flush downstream ResponseWriter on upstream watch stream close to ensure clean EOF delivery - #2793

Open
WorrierKhushal wants to merge 3 commits into
openyurtio:masterfrom
WorrierKhushal:fix/2792-watch-proxy-upstream-disconnect
Open

fix: flush downstream ResponseWriter on upstream watch stream close to ensure clean EOF delivery#2793
WorrierKhushal wants to merge 3 commits into
openyurtio:masterfrom
WorrierKhushal:fix/2792-watch-proxy-upstream-disconnect

Conversation

@WorrierKhushal

@WorrierKhushal WorrierKhushal commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

In pkg/yurthub/proxy/remote/remote.go, the watch request proxy path streams the upstream cloud API server response to downstream clients (Kubelet, controllers) via io.Copy. When the upstream connection closes (cloud disconnect, server-side timeout, or network error), io.Copy returns — but the downstream ResponseWriter was never explicitly
flushed before the handler returned.

Go's net/http write path buffers data internally. Without an explicit
Flush(), the final bytes (including the chunked-encoding EOF trailer) may sit in the write buffer until the handler returns and the server closes the connection. In practice, this means Kubelet's watch goroutine can remain blocked on Read() — receiving neither events nor a clean EOF signal — for the duration of the OS-level TCP keepalive timeout (minutes to hours depending on node configuration) rather than detecting the disconnect promptly.ruring this stall window, Kubelet believes its existing watch is still active and does not re-issue it via YurtHub's edge-autonomous cache
path. Any pod, ConfigMap, or Secret changes during the cloud disconnect period that YurtHub has correctly cached are invisible to Kubelet until the TCP connection eventually times out or Kubelet restarts — directly undermining the edge autonomy guarantee.

The fix is a single Flush() call immediately after io.Copy returns,
matching the pattern already used in pkg/yurthub/multiplexer/ where every event write to downstream consumers explicitly calls http.Flusher.Flush(). No new pattern introduced — this brings the
remote proxy path in line with the existing multiplexer convention.

Added TestWatchProxyFlushesOnUpstreamClose — a deterministic test using a channel-based fake upstream that sends watch events then closes, asserting the downstream receives EOF within 1 second. The test reliably
fails before the fix (downstream hangs) and passes after.

Which issue(s) this PR fixes:

Fixes #2792

Special notes for your reviewer:

  • go test -race -v ./pkg/yurthub/proxy/remote/... — zero races, all
    tests pass including new test
  • go build and go vet on pkg/yurthub/proxy/remote/ — clean
  • golangci-lint run ./pkg/yurthub/proxy/remote/... — clean
  • No exported function signatures changed
  • No interface changes — ServeHTTP is registered as http.Handler,
    single registration site confirmed via grep
  • New test is deterministic (channel + time.After), not relying on
    timing or -race to occasionally catch the issue
  • Note for reviewer: Go's net/http does close the underlying TCP
    connection when the handler returns, but the explicit Flush() ensures
    the chunked EOF trailer is sent immediately rather than waiting for
    the deferred connection close — this is particularly important for
    Kubelet's HTTP/1.1 watch clients that distinguish between "no data"
    and "stream closed" at the chunked-transfer level. If you believe
    Go's automatic cleanup is sufficient here and Flush() is unnecessary,
    happy to discuss and adjust.

Does this PR introduce a user-facing change?

fix(yurthub): explicitly flush downstream watch connections when the upstream cloud connection closes, ensuring Kubelet receives a clean EOF signal and can promptly re-establish its watch via the edge-autonomous cache path rather than stalling for TCP keepalive timeout duration.

…ade responses

- Replace pooled bufio reader with unpooled bufio.NewReader in getResponse to prevent premature recycling before serveUpgradeRequest writes the response body.
- Add TestServeUpgradeRequest_ConcurrentRace to validate thread safety under concurrent requests.
@WorrierKhushal
WorrierKhushal requested a review from a team as a code owner August 30, 2026 08:28
@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.34%. Comparing base (ddf22f7) to head (2dfb485).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
pkg/yurthub/cachemanager/cache_manager.go 75.00% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2793      +/-   ##
==========================================
+ Coverage   46.20%   46.34%   +0.14%     
==========================================
  Files         405      405              
  Lines       27540    27585      +45     
==========================================
+ Hits        12724    12784      +60     
+ Misses      13649    13619      -30     
- Partials     1167     1182      +15     
Flag Coverage Δ
unittests 46.34% <78.26%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant