feat: add a progress-aware write deadline for initialization deliveries - #786
Open
kinyoklion wants to merge 3 commits into
Open
feat: add a progress-aware write deadline for initialization deliveries#786kinyoklion wants to merge 3 commits into
kinyoklion wants to merge 3 commits into
Conversation
Add internal/initwrite: a ResponseWriter wrapper that arms a write deadline sized to the payload it is about to send. The deadline combines a throughput floor with an absolute cap, so a client that stalls or reads slower than the floor is cut promptly while a healthy client sending a large payload is not. It writes strings without copying the payload per connection and exposes the underlying connection via Unwrap so http.NewResponseController can reach it. To let that (and any other) ResponseController operation reach the real connection, implement Unwrap on the logging and metrics ResponseWriter wrappers, which otherwise hide it. No runtime effect on its own: nothing wraps a connection with initwrite yet.
Make the write-deadline writer robust on every delivery-end path, not just the normal end-of-batch flush: - Done is now never nil. Outside a delivery it is an already-closed channel, so a producer that waits on it to release its budget slot is released at once instead of pinning the slot for the life of the connection. This also removes the need for a caller to capture the channel before closing its batch. - Add Finish: an idempotent backstop that clears the deadline and closes Done on any exit path. Without it, a delivery that ended without a final flush (an error or a cancelled context) left an armed deadline behind, which on HTTP/2 is a self-firing timer that resets an otherwise idle stream. - Begin closes any still-open Done before starting a new delivery, so a second Begin cannot orphan a waiter. - arm records a deadline as current only when SetWriteDeadline succeeds, so a transient failure does not leave the writer believing it armed a deadline it never set and then stop retrying. Rewrite the tests around an injected clock so the per-chunk deadline math is asserted exactly (floor, slack, cap, per-chunk re-arming) and the full gated lifecycle is covered (no deadline before Begin, armed during, cleared once at end, Done fail-safe, Finish backstop, string/byte parity). Coverage of the package rises from ~39% to ~94%. Also make the package doc self-contained: it no longer references a config file or default value that lives in the wiring change rather than here.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1b95108. Configure here.
| // Wrap returns a Writer for a poll (request/response) delivery: the deadline is armed on every | ||
| // write. net/http resets the connection deadline when the handler returns. | ||
| func Wrap(w http.ResponseWriter, maxHold time.Duration) *Writer { | ||
| return newWriter(w, maxHold, true) |
There was a problem hiding this comment.
Poll wrap leaves write deadline
Medium Severity
Wrap arms write deadlines but never clears them, relying on net/http to reset the connection after the handler returns. That only happens when WriteTimeout is set; this server sets none, so on HTTP/1 keep-alive the deadline can linger and fail or block a later request. WrapGated already clears via flush/Finish; poll deliveries need the same cleanup.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1b95108. Configure here.
The previous Finish cleared the write deadline on any exit path. Called mid-send -- which is where a cancelled delivery lands, since the completion signal never fires on cancel -- clearing stripped the only bound on the remaining bytes, so a stalled client could hold the connection and its budget slot indefinitely. Clearing is correct only for a delivery that actually finished. Replace it with two operations that match the two ways a gated delivery ends: - A clean finish still clears the deadline at the end-of-delivery flush, so the now-idle persistent stream is left alone. - Abort ends a delivery abnormally by moving the deadline to now, forcing any in-flight or subsequent write to fail at once rather than running unbounded. It is safe on any exit path, including mid-send. - WaitAndFinish holds until the delivery is flushed (slot held across the real send) or the context is cancelled, aborting in the latter case. This is the one correct ordering, so the producer no longer has to assemble it. Also make teardown race-safe: a generation counter bumped in Begin lets a flush tear down only the delivery it observed, never a newer one that started meanwhile, and the deadline clear now runs under the lock like arm does. Remove Flush's pre-flush re-arm, which could stamp the delivery's start before its first payload byte and shrink a healthy client's budget. Guard the chunk loop against a (0, nil) writer. Rework the tests around the injected clock to assert the exact per-chunk budget with literal values, arm-before-write ordering, the error path, the gated string path (the production SSE path), post-delivery inertness across heartbeats, Abort vs clean-finish semantics, WaitAndFinish on both branches, and that a stale flush cannot clear a newer delivery's deadline.
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
1 participant
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.
Adds the write-deadline primitive that the init-concurrency wiring will use to reclaim a slot from a client that cannot keep up, without disturbing a healthy client mid-delivery.
What changes
internal/initwritepackage. AResponseWriterwrapper that, for each initialization delivery, arms a write deadline sized to the payload: a throughput floor (so a stalled or too-slow client is cut promptly) combined with an absolute cap (so a client stuck right at the floor on a very large payload is still bounded). It writes strings directly to avoid copying the payload once per connection, and implementsUnwrapsohttp.NewResponseControllercan reach the underlying connection to set the deadline.Unwrapon the logging and metricsResponseWriterwrappers. These wrap the connection for their own purposes and otherwise hide it, which would stop aResponseControllerfrom reaching the real connection. Each now returns its inner writer.No behavior change
Nothing wraps a connection with
initwriteyet, so this is inert at runtime. It builds, vets, and passes-raceon./internal/initwrite/,./internal/logging/, and./internal/middleware/.Second of the PRs splitting #782. Builds on #785 (the config + limiter refinement); the wiring PR will follow and depend on both.
Note
Low Risk
New isolated package and small
Unwrapadditions with no production call sites yet; behavior change is deferred to a follow-up wiring PR.Overview
Introduces
internal/initwrite, aResponseWriterwrapper that will bound how long slow or stalled clients can hold init-delivery budget slots. It arms per-chunk write deadlines from a 64 KiB/s throughput floor plus slack, with an optionalmaxHoldcap; large writes are sliced so deadlines re-arm mid-payload.Wrapalways arms (poll responses);WrapGatedarms only betweenBeginand anEnd+ flush teardown so SSE heartbeats and idle HTTP/2 streams are not left with a lingering deadline.Abort/WaitAndFinishtear down stalled sends and release slot waiters.Adds
Unwrapon the logging and metricsResponseWriterwrappers sohttp.NewResponseControllercan reach the real connection when this layer is stacked under middleware—without it, write deadlines would silently no-op.No runtime wiring yet; this PR is the primitive plus tests and middleware compatibility only.
Reviewed by Cursor Bugbot for commit 06b36f0. Bugbot is set up for automated code reviews on this repo. Configure here.