fix(blaze): drain accepted HTTP connections - #2296
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb675c9ef5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Keep peer listeners and service events fair while the outer loop gives | ||
| // termination signals deterministic priority. | ||
| tokio::select! { | ||
| () = connections.reap_next(), if !connections.is_empty() => {} |
There was a problem hiding this comment.
When both the UDS and optional TCP accept queues remain ready under sustained short-lived traffic, this unbiased select! has two ready branches that add a JoinSet entry but only one branch that removes one. It therefore accepts tasks about twice as fast as it reaps completed records, allowing the supervisor's completed-task backlog—and daemon memory usage—to grow continuously until traffic subsides. The daemon explicitly supports both UDS and TCP API endpoints.
AGENTS.md reference: src/blaze/AGENTS.md:L27-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dfd28152a898. serve_one_event now calls reap_ready() before the unbiased service-event selection, draining every completion already queued while preserving fair polling of both listeners and reload events. The regression service_events_reap_all_ready_tasks_before_accepting waits until 64 supervised tasks are finished, queues both listener types, runs one service event, and verifies that only the newly accepted task remains. Exact-head Linux validation passed the default and all-feature build, strict Clippy, test, and strict rustdoc matrices; the focused daemon suite is 10/10.
cb675c9 to
dfd2815
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfd28152a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| use crate::state::ServerState; | ||
|
|
||
| const CONNECTION_DRAIN_TIMEOUT: Duration = Duration::from_secs(30); |
There was a problem hiding this comment.
When an accepted connection exceeds the graceful window under the packaged systemd service, src/blaze/dist/blazed.service:18 already sets TimeoutStopSec=30. That timer begins when systemd sends SIGTERM, before this equal 30-second application timer starts, so systemd can forcibly kill the daemon before abort_all(), joining the tasks, and returning the intended shutdown error. Use a shorter connection-drain timeout or increase the unit's stop timeout so the new fallback can complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in de1d7fdf242a. The application keeps its 30-second graceful connection window, while the packaged service now uses TimeoutStopSec=60, so the service-level deadline no longer coincides with the point where the daemon begins aborting and joining remaining tasks. packaged_stop_timeout_exceeds_connection_drain_timeout reads the shipped unit and enforces that ordering, and the English and Chinese shutdown documentation describes both bounds. Exact-head Linux validation passed the default and all-feature build, strict Clippy, test, and strict rustdoc matrices; the focused daemon suite is 11/11.
This enables daemon shutdown to stop accepting new UDS and TCP connections, ask accepted HTTP/1 connections to finish, and wait through a bounded graceful window before aborting and joining remaining tasks. A single supervisor owns every accepted connection. Termination signals take priority over an otherwise fair service loop, completed-task failures remain visible, and in-flight requests can finish before shutdown returns. This change does not propagate cancellation through manager operations or clean up runtime owners; those remain separately tracked work. Signed-off-by: Weisson Han <wenshu.hx@linux.alibaba.com>
dfd2815 to
de1d7fd
Compare
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Why
Blaze previously detached every accepted HTTP connection task from the daemon accept loop. After SIGTERM or SIGINT, the server could return while an accepted connection was still handling a request and using daemon state.
The daemon needs a clear shutdown boundary for work it has already accepted: stop taking new connections, let active HTTP/1 requests finish for a bounded graceful window, and account for every connection task before returning.
What changed
Before:
After:
flowchart TD A["SIGTERM or SIGINT"] --> B["Stop accepting connections"] B --> C["Notify accepted HTTP/1 connections"] C --> D{"All connection tasks finished within 30 seconds?"} D -- "Yes" --> E["Return cleanly"] D -- "No" --> F["Abort and join remaining tasks"] F --> G["Return a shutdown error"]The single commit,
de1d7fdf242a, contains the supervisor, listener and HTTP/1 shutdown flow, the packaged service timeout, focused regressions, and the matching English and Chinese documentation. Keeping them together makes the shutdown ownership invariant independently reviewable and testable.This PR does not propagate cancellation through every manager operation and does not release runtime owners. Those follow-ups remain tracked by #2235 and #2295.
Related issue
fixes #2294
User / Agent impact
Daemon shutdown now waits for accepted HTTP requests instead of dropping their connection tasks immediately. A shutdown may therefore take up to the 30-second graceful window, followed by cancellation cleanup, and reports an error when the graceful window expires or a supervised task panics.
The packaged service stop limit changes from 30 to 60 seconds. No HTTP route, request schema, response schema, or configuration key changes.
Risk and compatibility
The documented operational shutdown behavior changes, but request compatibility is unchanged. Both Unix and optional TCP listeners use the same connection supervisor.
Validation
Exact commit
de1d7fdf242a891dca18c961f026bdb8a6c83ae0was verified on Linux x86_64 from a Git archive in a fresh source directory with separate initially empty default and all-feature Cargo target directories.Passed:
cargo fmt --all -- --checkcargo metadata --locked --no-deps --format-version 1cargo build --workspace --all-targets --lockedcargo clippy --workspace --all-targets --locked -- -D warningscargo test --workspace --locked: 170 tests passedcargo test --workspace --all-features --locked: 180 tests passedcargo test --locked -p blazed daemon::tests: 11 focused tests passedscripts/docs-lint.shpython3 scripts/docs-link-check.pygit diff --checkHosted checks are pending.
Documentation and rollback
The English and Chinese Blaze README and user guide now document the accepted-connection shutdown boundary, the 30-second graceful window, the 60-second packaged service stop limit, the abort-and-join fallback, and the work that remains outside this PR.
Reverting
de1d7fdf242arestores the previous detached-connection shutdown behavior.