feat(http-invocation): classify client disconnects as HTTP 499 (#524) - #563
feat(http-invocation): classify client disconnects as HTTP 499 (#524)#563shobham-nv wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe change classifies confirmed client disconnects as HTTP 499 in invocation metrics and vanity-gateway proxy responses. It preserves server-generated statuses and genuine upstream failures as 502. ChangesClient disconnect classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant VanityGateway
participant HttpInvocationServer
participant InflightRequestGuard
participant InvocationMetrics
Client->>VanityGateway: send execution request
VanityGateway->>HttpInvocationServer: proxy execution request
HttpInvocationServer->>InflightRequestGuard: track in-flight invocation
Client-->>VanityGateway: cancel request context
InflightRequestGuard->>InvocationMetrics: record HTTP 499 without returned status
HttpInvocationServer-->>VanityGateway: proxy error
VanityGateway-->>Client: return 499 for canceled context
VanityGateway-->>Client: return 502 for genuine proxy failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
When a client disconnects after an invocation is dispatched, the request future is dropped before any status is produced, so the vanity gateway accounted it as a generic 502 - indistinguishable from genuine upstream faults and inflating 5xx/endpoint-unavailability metrics. Invocation service (authoritative): InflightRequestGuard now tracks whether a status was returned. On Drop with no status (client disconnect) it logs request/function/org context and records app.invocation.error with http_status_code=499; server-side errors keep their real status (already counted via AppError::into_response). Pre-initialize the 499 series so it reports 0 before the first disconnect. Vanity gateway (accounting): reverse-proxy errors are reclassified to 499 only when the inbound request context is canceled (confirmed disconnect); genuine upstream transport failures remain 502. Covers all routes on the shared pexec path (/pexec, /exec, TLB). Adds tests for client disconnect, server error, gateway timeout, and genuine proxy failure.
9ffbbd6 to
da503e7
Compare
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-30 05:23:04 UTC | Commit: da503e7 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/invocation-plane-services/vanity-gateway/gateway/vanity_director.go (1)
83-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd request/function context to the new disconnect log, and mark span errors on proxy failure.
The new
zap.L().Warn("client closed request before upstream response", zap.Error(err))doesn't includetarget.FunctionID/target.FunctionVersionID, even thoughServeExecalready has them and puts them on the span just above. Also, neither branch ofwriteProxyErrormarks the active span as failed (no error attribute/status), so cross-service traces for both 499 and 502 outcomes won't show the failure.As per path instructions, "Ensure every log line includes available context fields: Request ID, Function ID, Cluster ID, and Org/NCA ID." As per coding guidelines, "set error attributes on failures" is required for cross-service paths.
♻️ Proposed direction (signature change needed to thread function IDs through)
-func writeProxyError(writer http.ResponseWriter, request *http.Request, err error) { +func writeProxyError(writer http.ResponseWriter, request *http.Request, err error, functionID, functionVersionID string) { if clientClosedRequest(request) { - zap.L().Warn("client closed request before upstream response", zap.Error(err)) + zap.L().Warn("client closed request before upstream response", + zap.Error(err), zap.String("function_id", functionID), zap.String("function_version_id", functionVersionID)) + span := trace.SpanFromContext(request.Context()) + span.SetAttributes(attribute.Bool("error", true)) writer.Header().Set("Content-Type", "application/problem+json")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/invocation-plane-services/vanity-gateway/gateway/vanity_director.go` around lines 83 - 106, Update writeProxyError and its ServeExec call site to thread target.FunctionID and target.FunctionVersionID into the disconnect path. Include all available request context fields—Request ID, Function ID, Cluster ID, and Org/NCA ID—in the client-disconnect warning. In both the 499 and 502 branches, mark the active span failed and attach the proxy error as an error attribute before responding.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/invocation-plane-services/vanity-gateway/gateway/vanity_director.go`:
- Around line 83-106: Update writeProxyError and its ServeExec call site to
thread target.FunctionID and target.FunctionVersionID into the disconnect path.
Include all available request context fields—Request ID, Function ID, Cluster
ID, and Org/NCA ID—in the client-disconnect warning. In both the 499 and 502
branches, mark the active span failed and attach the proxy error as an error
attribute before responding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8cd868a1-fa6c-43aa-85f8-4dec2c837cb0
📒 Files selected for processing (4)
src/invocation-plane-services/http-invocation/crates/server/src/metrics/mod.rssrc/invocation-plane-services/http-invocation/crates/server/src/routes/post_pexec.rssrc/invocation-plane-services/vanity-gateway/gateway/vanity_director.gosrc/invocation-plane-services/vanity-gateway/gateway/vanity_director_test.go
| // Pre-init the 499 series so it reports 0 before the first disconnect. | ||
| counter!( | ||
| FUNCTION_INVOCATION_ERROR.name, | ||
| "http_status_code" => "499", |
There was a problem hiding this comment.
Probably should make 499 a env var and use it spread around
There was a problem hiding this comment.
Done, declared the var as a constant not a env var
| assert_eq!(value, DebugValue::Counter(1)); | ||
| } | ||
|
|
||
| /// A server-side early exit (status was returned) must NOT be counted as a 499. |
There was a problem hiding this comment.
Generally should reference 499 as client early disconnect. 499 is not a true standard error code
There was a problem hiding this comment.
I don't think we need changed to the vanity gateway. It should be recording the 499 regardless. Did you test e2e to see if that was the case?
There was a problem hiding this comment.
Tested - the gateway change is needed. Added an integration test running the real otelhttp middleware over ServeExec: a canceled inbound context records status_code=499 a genuine upstream error records 502. otelhttp records whatever status the error handler writes without the change it writes 502 for disconnects too. Not cross-service e2e, but it hits the exact metric-emitting path.
…layer Add integration tests that drive a request through the real otelhttp server middleware wrapping ServeExec and read back http.server.request.duration: a canceled inbound context records status_code=499, a genuine upstream transport error records 502. This is the concrete evidence that the gateway change is what makes disconnects countable as 499 (otelhttp records whatever status the proxy error handler writes). Also centralize the "499" literal into metrics::CLIENT_EARLY_DISCONNECT_STATUS and use "client early disconnect" wording in logs, comments, and tests per review feedback. Relates to #524 Signed-off-by: shobham <shobham@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/invocation-plane-services/http-invocation/crates/server/src/metrics/mod.rs (1)
210-218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd regression coverage for the pre-initialized 499 series.
The existing test covers status
504, not the zero-valuedapp.invocation.error{http_status_code="499",function_id=""}series before the first disconnect. Add this test or explain the omission in the Pull Request description.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/invocation-plane-services/http-invocation/crates/server/src/metrics/mod.rs` around lines 210 - 218, Add regression coverage near the existing metrics test for the pre-initialized CLIENT_EARLY_DISCONNECT_STATUS series: verify app.invocation.error with http_status_code "499" and an empty function_id is present with value 0 before any disconnect occurs. Keep the existing 504 coverage unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/invocation-plane-services/vanity-gateway/gateway/vanity_director_test.go`:
- Around line 582-610: Update gatewayMetricHasStatusCode to skip histogram data
points whose point.Count is zero before checking the http.response.status_code
attribute, so it only reports metrics containing a recorded observation.
---
Outside diff comments:
In
`@src/invocation-plane-services/http-invocation/crates/server/src/metrics/mod.rs`:
- Around line 210-218: Add regression coverage near the existing metrics test
for the pre-initialized CLIENT_EARLY_DISCONNECT_STATUS series: verify
app.invocation.error with http_status_code "499" and an empty function_id is
present with value 0 before any disconnect occurs. Keep the existing 504
coverage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca628f87-2158-4d30-ada0-74801b72c61f
📒 Files selected for processing (3)
src/invocation-plane-services/http-invocation/crates/server/src/metrics/mod.rssrc/invocation-plane-services/http-invocation/crates/server/src/routes/post_pexec.rssrc/invocation-plane-services/vanity-gateway/gateway/vanity_director_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/invocation-plane-services/http-invocation/crates/server/src/routes/post_pexec.rs
| func collectGatewayMetrics(t *testing.T, reader sdkmetric.Reader) []metricdata.Metrics { | ||
| t.Helper() | ||
| var rm metricdata.ResourceMetrics | ||
| require.NoError(t, reader.Collect(context.Background(), &rm)) | ||
| var out []metricdata.Metrics | ||
| for _, sm := range rm.ScopeMetrics { | ||
| out = append(out, sm.Metrics...) | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| func gatewayMetricHasStatusCode(metrics []metricdata.Metrics, code int) bool { | ||
| for _, metric := range metrics { | ||
| if metric.Name != "http.server.request.duration" { | ||
| continue | ||
| } | ||
| histogram, ok := metric.Data.(metricdata.Histogram[float64]) | ||
| if !ok { | ||
| continue | ||
| } | ||
| for _, point := range histogram.DataPoints { | ||
| if value, ok := point.Attributes.Value("http.response.status_code"); ok && | ||
| value == attribute.Int64Value(int64(code)) { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '540,630p' src/invocation-plane-services/vanity-gateway/gateway/vanity_director_test.go
printf '\n--- metric helper usages ---\n'
rg -n -C 3 'gatewayMetricHasStatusCode|collectGatewayMetrics|http\.server\.request\.duration' src/invocation-plane-services/vanity-gateway/gatewayRepository: NVIDIA/nvcf
Length of output: 7611
🏁 Script executed:
printf '%s\n' '--- OpenTelemetry versions ---'
rg -n -C 2 'opentelemetry|otelhttp' go.mod '**/go.mod' 2>/dev/null || true
printf '%s\n' '--- local metricdata definitions ---'
rg -n -C 4 'type Histogram|type HistogramDataPoint|Count[[:space:]]+uint64' . --glob '*.go' --glob '!vendor/**' | head -200
printf '%s\n' '--- module cache candidates ---'
find "${GOMODCACHE:-/go/pkg/mod}" -path '*opentelemetry*metricdata*data.go' -o -path '*opentelemetry*metricdata*' 2>/dev/null | head -40Repository: NVIDIA/nvcf
Length of output: 22721
🏁 Script executed:
git ls-files | rg '(^|/)go\.mod$|vendor/.*/opentelemetry|vanity_director_test\.go$'
printf '%s\n' '--- relevant module files ---'
for f in $(git ls-files | rg '(^|/)go\.mod$'); do
if rg -q 'opentelemetry|otelhttp' "$f"; then
printf '\n%s\n' "$f"
rg -n -C 2 'opentelemetry|otelhttp' "$f"
fi
doneRepository: NVIDIA/nvcf
Length of output: 35571
🌐 Web query:
OpenTelemetry Go v1.43.0 metricdata Histogram DataPoint Count zero-count data points SDK ManualReader
💡 Result:
In OpenTelemetry Go v1.43.0, the metricdata.HistogramDataPoint structure contains a Count field (a uint64) that represents the total number of updates recorded for that histogram [1][2]. For exponential histograms specifically, there is also a ZeroCount field. This field tracks the number of values whose absolute value falls within the histogram's "zero region," defined by the ZeroThreshold [1][2]. This is not a "zero-count" in the sense of zero updates, but rather a dedicated bucket for values too small to be represented by the exponential formula or values rounded to zero [1][2]. Regarding your query about ManualReader and histogram behavior: 1. Data Collection: ManualReader uses the Collect method to trigger the SDK to produce and gather metric data into metricdata.ResourceMetrics [3]. It does not inherently alter the internal logic of how HistogramDataPoint counts are calculated; it simply facilitates the export of the current state of those aggregations [3]. 2. Potential for Stale Data: There have been identified issues in the SDK regarding the reuse of HistogramDataPoint entries [4]. Specifically, if a HistogramDataPoint is reused across collection cycles, fields such as Sum, Min, Max, or Count may retain stale values if they are not explicitly cleared or if specific aggregation options (like NoMinMax=true) are changed between collections [4]. While not limited to ManualReader, this behavior affects any reader that collects from an aggregation where internal state is recycled [4]. 3. Recent SDK Updates: It is worth noting that v1.43.0 included significant updates to how exponential histograms handle their internal state, including switching to atomic operations for sum, min, max, and count to ensure concurrency safety [5]. Additionally, a race condition affecting LastValue aggregations—where a zero value could be incorrectly observed—was resolved in this release [6][5]. If you are seeing unexpected count values, ensure that your aggregation configuration is consistent and consider that stale data might persist if the SDK is reusing underlying aggregation objects across collection cycles [4].
Citations:
- 1: https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/metric/metricdata/data.go
- 2: https://pkg.go.dev/go.opentelemetry.io/otel/sdk/metric/metricdata
- 3: https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/metric/manual_reader.go
- 4: Histogram datapoint reuse can leak stale sum/min/max values open-telemetry/opentelemetry-go#8399
- 5: https://newreleases.io/project/github/open-telemetry/opentelemetry-go/release/v1.43.0
- 6: Fix race in the lastvalue aggregation where 0 could be observed open-telemetry/opentelemetry-go#8056
🏁 Script executed:
set -eu
base='https://raw.githubusercontent.com/open-telemetry/opentelemetry-go/v1.43.0'
for path in \
sdk/metric/metricdata/data.go \
sdk/metric/aggregation.go \
sdk/metric/aggregation_histogram.go \
sdk/metric/manual_reader.go
do
printf '\n--- %s ---\n' "$path"
curl -fsSL "$base/$path" | rg -n -C 5 'type Histogram|Count|DataPoints|metricdata\.Histogram|Export'
doneRepository: NVIDIA/nvcf
Length of output: 6018
🏁 Script executed:
set -eu
curl -fsSL 'https://api.github.com/repos/open-telemetry/opentelemetry-go/git/trees/v1.43.0?recursive=1' |
jq -r '.tree[].path' |
rg '^sdk/metric/.*(histogram|aggregation|manual_reader).*\.go$'Repository: NVIDIA/nvcf
Length of output: 198
🌐 Web query:
site:github.com/open-telemetry/opentelemetry-go/blob/v1.43.0/sdk/metric "HistogramDataPoint" "Count" "DataPoints"
💡 Result:
In OpenTelemetry Go v1.43.0:
Histogram.DataPointsis typed as[]HistogramDataPoint[N].HistogramDataPoint.Countis auint64representing the number of updates used to calculate the histogram.- The relevant definition is in
sdk/metric/metricdata/data.go, lines 971–1019. (github.com)
type Histogram[N int64 | float64] struct {
DataPoints []HistogramDataPoint[N]
}
type HistogramDataPoint[N int64 | float64] struct {
Count uint64
Bounds []float64
BucketCounts []uint64
Sum N
}Citations:
Require a non-zero histogram count.
Skip points with point.Count == 0 before matching http.response.status_code. This ensures the helper identifies a recorded observation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/invocation-plane-services/vanity-gateway/gateway/vanity_director_test.go`
around lines 582 - 610, Update gatewayMetricHasStatusCode to skip histogram data
points whose point.Count is zero before checking the http.response.status_code
attribute, so it only reports metrics containing a recorded observation.
2 participants
TL;DR
Client disconnects after dispatch were accounted as a generic 502 by the vanity gateway (the request future is dropped before any status is produced), making caller cancellations indistinguishable from genuine upstream faults and inflating 5xx / endpoint-unavailability metrics. This classifies confirmed client disconnects as HTTP 499 for internal telemetry — authoritatively in the invocation service and, where technically possible, in gateway request accounting — while leaving real upstream failures as 502. 499 is telemetry-only; the disconnected client never receives it.
Additional Details
Problem. When a client goes away after an invocation is dispatched,
pexecnever returns aResponse. The gateway's reverse-proxy error handler mapped the resulting transport error to a single generic 502 with a fixed body, so client cancellations and genuine upstream faults collapsed into one status — no 499 anywhere, and 502 root-cause was unreliable.Invocation service (authoritative signal).
InflightRequestGuardnow records whether a status was produced:handle_streaming_response(...)call site was changed from?to amatch; on theErrpath it marksstatus_returnedbefore returning, so a server-side early exit is not mistaken for a disconnect.Dropwhen the request did not complete and no status was produced (pure client disconnect), it logs request/function/org context and recordsapp.invocation.error{http_status_code="499"}.AppError::into_response— so no double counting.Vanity gateway (accounting). The reverse-proxy error handler reclassifies to 499 only when the inbound request context is canceled (the authoritative "client gone" signal). A transport error that merely wraps
context.Canceledwhile the inbound context is still live stays a genuine upstream fault (502). otelhttp records whatever status is written, so the 499 lands inhttp_server_request_duration_seconds_count. For a mid-stream SSE disconnect the200is already flushed, so it remains 200 — that's the "where technically possible" limit.Scope / limitations. The classification applies to all routes on the shared
pexecpath (/pexec,/exec, TLB). Existing 400 (body-read failure) handling is untouched, since that occurs before the guard exists. A true disconnect produces no IS response for that request, so the gateway detects it independently via the canceled inbound context rather than a propagated signal.For the Reviewer
Closest files to review:
src/invocation-plane-services/http-invocation/crates/server/src/routes/post_pexec.rs— theInflightRequestGuardstatus_returnedlogic and the?→matchchange.src/invocation-plane-services/vanity-gateway/gateway/vanity_director.go—clientClosedRequest/writeProxyErrorand the inbound-context check.Question for reviewers: comfortable keying the gateway classification off
request.Context().Err() == context.Canceledas the confirmed-disconnect signal (vs. inspecting the transport error)?For QA
Verified locally:
cargo check --tests,cargo clippy --tests,cargo fmt --checkclean;cargo test --lib— 75 pass, 4 docker-gated ignored. New tests: 499 recorded on client disconnect, not recorded on server error; existing 504 gateway-timeout test still passes.go build,go vet,gofmtclean;go test ./gateway/...passes. New tests: client disconnect (canceled context) → 499, genuine transport failure → 502; existingcontext.Canceled-with-live-context test still returns 502.QA needed: light — recommend confirming in a staging/prod dashboard that
app.invocation.error{http_status_code="499"}and the gateway 499 count appear on real client disconnects and that 502 drops correspondingly.Issues
Closes #524
Checklist
Summary by CodeRabbit