The following discussions from !1686 should be addressed: - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1686#note_3716553615): > non-blocking: stalled or late-disconnecting clients book a `500` + Error log per request on both protocols now — make the alert mitigation real > > This arm is a deliberate, documented trade-off (a truncated relay must not read as success), but the review note from round 2 flagged the h1 shape and this commit extends it to h2: a client that stops reading until the budget runs out, or disconnects within the post-instant window, produces `CodeInternalServerError` + one Error line per request on every protocol, and a repeat offender sustains the rate at the connection concurrency. The arm's comment recommends deduplicating alerts by repository and reading the written-bytes count as the stall-vs-squeeze discriminator until #269 lands consumption-rate evidence — consider making that real: per-repository dedup and a rate-limited Error line. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1686#note_3716553631): > non-blocking: the abort's `Flush()` runs while the armed deadline is still in the future — a stalled client with a near-full send buffer can pin the handler until the budget elapses > > `controller.Flush()` (`:863`) precedes the backdated `SetWriteDeadline`, so a live-but-stalled client whose kernel send buffer is full makes the flush block until the pending deadline fires (tens of seconds), where the pre-MR code returned in microseconds. Bounded by the armed deadline and rarely triggered (needs an upstream source fault + a stall at the same moment — a fully blocked write would have reached the kill arm instead), but worth either a short deadline around the flush itself or a doc sentence noting the flush can wait out the remaining budget rather than fail fast. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1686#note_3716553645): > non-blocking: the flushed-prefix guarantee this doc promises is HTTP/1.1-specific — on h2 the abort's `RST_STREAM` control frame pops before the flush's `DATA` > > Verified against the h2 write scheduler: control and `RST_STREAM` frames pop first, and `DATA` frames queued after the stream closes are ignored, so the flushed prefix can be dropped on h2 — a healthy-but-slow h2 client reads the `200` head plus a stream error, not the "200 plus unexpected EOF" both this doc and the commit message claim. Integrity still holds (the client never reads a truncated response as complete), but the promise should be scoped to h1 unless a live h2 probe shows otherwise. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1686#note_3716553683): > non-blocking: the abort's flush-failure Warn is a guaranteed emission in the kill and churn arms — double log volume on the most common aborts > > In the kill arm the deadline has already elapsed when the abort runs, so the flush cannot succeed (h1: the poller rejects the write; h2: the stream is closed); in the churn arm the client is gone. Every such abort therefore emits a Warn carrying no signal, alongside the booking line that already names the event. Log at Debug when the write deadline has already elapsed or the copy error was a connection error; keep Warn only where the flush is genuinely expected to succeed. - [x] @mkhalifa3 started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1686#note_3716553702): > non-blocking: the zero-deadline fallback cannot classify h2-shaped breaches — dormant today, but pin it with monitoring > > When `armRemoteReadResponseDeadline` fails it returns the zero Time, and the fallback then relies on the `net.Error` `Timeout()` check — which the h2 stream-closed breach does not satisfy, so a hypothetical arm failure on h2 would book its deadline cut as client churn: exactly the misattribution this commit removed elsewhere. Today it is unreachable (`(*http2responseWriter).SetWriteDeadline` cannot fail), and the deployment's interceptor implements `Unwrap`, so the arm succeeds with the current wiring. A Grafana alert on the arm-failure warning rate would make the blind spot visible if the wiring ever regresses. --- ## Resolution All five discussions are addressed on `main` by [!1836](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1836) (merged `2026-08-24`, squash `ca17830d`). What each one got: | # | Asked for | Landed as | | --- | --- | --- | | 1 | Per-repository dedup and a rate-limited Error line | `remoteTarballKillLog` rations the line to one per repository per minute per process, and `gitlab_artifact_registry_npm_remote_tarball_deadline_kills_total` carries the count so no rationing can reach it | | 2 | A short deadline around the flush, or a doc sentence | The deadline: `boundRemoteTarballAbortFlush` shortens the response write deadline to `remoteTarballAbortFlushBudget` (5s) before the flush, and only ever shortens | | 3 | Scope the flushed-prefix promise to h1 unless a live h2 probe shows otherwise | The probe, `TestRemoteTarballHandler_UnframedTruncatedRelay_OverHTTP2`, which showed the prefix **does** survive on h2: an h2 flush blocks until its own frame write completes, so the prefix is on the wire before the reset exists. The promise was corrected rather than scoped, and the failure shape per protocol is now written out | | 4 | Debug where the flush cannot succeed, Warn where it can | `remoteTarballAbortFlushLevel`, recognising both endings through the same predicates the copy arms use | | 5 | A Grafana alert on the arm-failure rate | The alertable signal: `gitlab_artifact_registry_npm_remote_response_deadline_arm_failures_total`, label-less, registered at boot so the series is on the scrape path at zero from startup | Two things this issue does **not** cover, recorded so they are not read as part of its closure: - **The alert definition for item 5 is not in this repository.** Alerts live in the monitoring repository (S03-A), so what merged here is the counter an alert reads, not the alert. `internal/format/npm/metrics.md` records the recommendation: alert on any sustained rate rather than on a threshold, because the expected value is zero forever. - **S15 does not yet declare either counter.** That row rides in [!1882](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1882), which was open when this issue closed. Until it merges, `main` emits two `npm_remote_*` series the owner spec does not list, and S15's Observability section reads as if its four-family table were the whole surface. One follow-up came out of the review rather than out of the original five: #832, an upstream read timeout books `CodeInternalServerError` on the deadline-kill arm where the source-fault arm it pre-empts would have booked `CodeUpstreamUnavailable`. That code is byte-identical to what it was before !1836, so it is not a regression from this work; it is tracked separately because the counter item 1 added is what makes an operator meet it. #269 remains open and is unaffected: it carries the consumption-rate evidence needed to tell an undersized response budget from a stalled client, which is a distinction item 1 deliberately does not make.