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.
There was a problem hiding this comment.
Resolved in de0fc89d71c0.
The published revision calls reap_ready() before selecting another service event. It drains every completion already queued while preserving fair polling of Unix, TCP, and reload events, so dual-listener traffic cannot add completed task records faster than this boundary removes them.
The regression service_events_reap_all_ready_tasks_before_accepting starts 64 completed supervised tasks, makes both listener types ready, executes one service event, and proves only the newly accepted task remains. The exact commit passed the Linux default/all-feature build, strict Clippy, workspace tests (52+214 / 52+224), strict rustdoc, and the complete daemon::tests filter (25/25).
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.
There was a problem hiding this comment.
Resolved in de0fc89d71c0.
The application retains a 30-second graceful connection-drain window, while the published service unit uses TimeoutStopSec=60. The service deadline therefore no longer coincides with the point where the daemon begins aborting and joining remaining connection tasks.
packaged_stop_timeout_exceeds_connection_drain_timeout reads the shipped unit and enforces that ordering, and the English and Chinese README/user-guide text documents both bounds. The exact commit passed the Linux default/all-feature build, strict Clippy, workspace tests (52+214 / 52+224), strict rustdoc, and the complete daemon::tests filter (25/25).
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". |
de1d7fd to
de0fc89
Compare
|
@codex review This PR establishes bounded ownership and shutdown for accepted Unix and TCP HTTP connection tasks, including completion reaping, timeout fallback, failure aggregation, service headroom, tests, and bilingual documentation. Please review the complete pull request: every commit, the cumulative diff against the base branch, and the implementation, tests, and documentation as one submitted change. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de0fc89d71
ℹ️ 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".
| state.manager.cancel_template_imports(); | ||
| if let Some(sync_loop) = sync_loop.as_mut() { | ||
| let (connection_result, sync_result) = tokio::join!( | ||
| connections.shutdown(CONNECTION_DRAIN_TIMEOUT), |
There was a problem hiding this comment.
This call makes accepted-connection draining part of the daemon shutdown architecture, but the paired component design docs still describe the opposite invariant: src/blaze/docs/design/storage-artifact-synchronization.md:76-80 and src/blaze/docs/design/template-catalog.md:124-128 (with equivalent Chinese text) say that draining accepted connections remains a separate, unimplemented responsibility. Update the English and Chinese design documents with the supervisor, ordering, and timeout boundary so architectural readers do not rely on stale behavior; the mandatory documentation standard requires architecture or protocol changes to be reflected under the component's docs/design/.
AGENTS.md reference: AGENTS.md:L339-L343
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in the published commit cffe1fde3fdf.
I updated both bilingual design pairs so they now describe the implemented shutdown contract instead of the earlier “not yet implemented” boundary:
- storage synchronization (English) and Chinese now cover the shared connection supervisor, parallel storage-worker shutdown, 30-second drain window, abort/join fallback, failure retention, import wait ordering, and the remaining manager/runtime-owner boundary;
- template catalog (English) and Chinese now describe cancellation before drain, the same bounded connection shutdown, parallel synchronization shutdown, and the final registered-import wait.
The exact published commit passed the bilingual docs lint and relative-link check locally. Its fresh Linux archive also passed default/all-feature build, strict Clippy, workspace tests (52+214 and 52+224), strict rustdoc, and the focused daemon shutdown suite (25/25).
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 events that admit new work, already-visible service failures remain visible, and in-flight requests can finish before shutdown returns. The connection drain runs alongside existing synchronization shutdown after new template imports are cancelled, and failures from every stage remain visible. This change does not propagate cancellation through manager operations or clean up runtime owners; those remain separately tracked work. Fixes: 1f0cfac ("feat(anvil): scaffold local orchestrator crate skeleton") Signed-off-by: Weisson Han <wenshu.hx@linux.alibaba.com>
de0fc89 to
cffe1fd
Compare
1 participant
Why
The daemon detached every accepted HTTP connection task from its accept loop.
After SIGTERM or SIGINT, shutdown could return while a previously accepted
request was still using daemon state.
Before this change, stopping the listeners did not establish a boundary for
already accepted work, and a connection-task panic or permanently stalled
request could disappear from the shutdown result. After this change, one
supervisor owns every accepted Unix and TCP connection task, gives active
HTTP/1 requests a bounded graceful window, and accounts for every task before
the daemon returns.
What changed
sustained traffic cannot accumulate completed task records.
ordinary listener polling fair.
seconds, then abort and join any remaining tasks and report a shutdown error.
termination signal.
cancel new template imports first, drain accepted connections in parallel
with storage synchronization shutdown, retain failures from both, and then
wait for already-registered imports.
30-second application drain window.
and Chinese Blaze README and user guide.
documents with the implemented supervisor, shutdown ordering, timeout, and
remaining manager/runtime-owner boundaries.
flowchart TD A["SIGTERM or SIGINT"] --> B["Stop Unix and TCP listeners"] B --> C["Cancel admission of new template imports"] C --> D["Drain accepted connections (30 s)"] C --> E["Stop storage synchronization"] D --> F["Abort and join remaining connection tasks"] D --> G["Collect connection result"] E --> H["Collect synchronization result"] F --> G G --> I["Wait for already-registered imports"] H --> I I --> J["Return all visible shutdown failures"]Commit
cffe1fde3fdf— drain accepted connection tasks. Adds the tasksupervisor, bounded graceful shutdown and fallback, completion reaping,
failure aggregation with existing shutdown stages, packaged-service
headroom, focused regressions, and matching bilingual user and design
documentation.
These changes belong in one PR because task ownership, listener ordering,
timeout fallback, shutdown error aggregation, service timeout, tests, and
documentation jointly define one accepted-connection shutdown invariant.
Splitting them would leave an intermediate revision that either cannot bound
accepted work or cannot complete its fallback before the service deadline.
Still to do
under [blaze] feat: propagate daemon request cancellation #2235.
Related issue
fixes #2294
User / Agent impact
Daemon shutdown now waits for accepted HTTP requests rather than abandoning
their connection tasks. It may use the 30-second graceful window and reports an
error if a task remains stuck or panics. The packaged service permits up to 60
seconds for the process to finish shutdown.
No HTTP route, request schema, response schema, or configuration key changes.
Risk and compatibility
The operational shutdown contract changes, but request compatibility is
unchanged. Both Unix and optional TCP listeners use the same supervisor.
Validation
Exact commit:
cffe1fde3fdf1b87d53a453fea3213ef6bdb8cceParent:
51b8e6e93759f67dca272a670c035d8fe5b4493aTree:
47d3c9ec4cab5618ad94e7f2aee105b48cc73275The exact commit was exported with
git archive; the matching local andLinux archive SHA-256 is:
a166548e9bc4e60feccb16ba1df28a4527002a07a5d584eab08dd7f712b6b2bcFresh Linux x86_64 source and separate initially empty default/all-feature
targets passed:
cargo fmt --all -- --checkcargo metadata --locked --format-version 1 --no-depscargo test --workspace --locked— 52 core + 214 daemon testscargo test --workspace --all-features --locked— 52 core + 224 daemontests
cargo test --locked -p blazed daemon::tests -- --nocapture— 25/25git diff --check, andpublic-boundary scan
All listed tests completed with zero failures. Hosted checks and complete-PR
review must be confirmed again for this published revision.
Documentation and rollback
The bilingual README, user guide, storage-synchronization design, and
template-catalog design describe the accepted-connection ownership boundary,
the 30-second graceful window, the 60-second packaged service limit, the
abort-and-join fallback, its ordering with existing shutdown work, and work
intentionally left to #2235 and #2295.
Reverting the single commit restores detached connection tasks and the previous
service timeout; no persisted-data conversion is required.