feat(http): operator-configurable [http] host limits + follow-up fixes#1054
Merged
Conversation
…ble via [http] config The astrid:http host had seven hardcoded tunable constants (timeouts, redirect and stream caps, buffered-body ceiling). A new [http] config section makes each operator-configurable, defaulting to today's exact constants so an absent section changes nothing. The daemon resolves [http] once into a typed HttpLimits (seconds pre-converted to Duration off the request hot path), the kernel stores it as one global value and forwards it unmodified to every capsule's WasmEngine via the loader, and it is snapshotted onto every pooled HostState at load. The request path reads its defaults and ceilings off HostState.http_limits instead of module constants: a per-request request-options value may still tighten a limit but a configured ceiling can only lower the bound, never let a caller exceed it. max_response_bytes stays hard-clamped to the shared absolute MAX_GUEST_PAYLOAD_LEN host payload limit. Operator config only: a project/workspace layer cannot set or widen [http] (merge::restrict reverts the whole section to the operator baseline), mirroring [security.capsule_local_egress].
…map DNS errors Three Copilot review findings on the astrid:http@1.1.0 host, each with a regression test that fails without the fix: A. total-timeout explicitness. The streaming path cleared the total when it equalled the configured default (a value check), so a caller who EXPLICITLY set timeouts.total-ms to a value equal to the configured default had their deadline silently cleared. Track explicitness (total_explicit) instead of value: set true in from_options only when the caller passed total-ms; the streaming path clears the default total only when !total_explicit. An explicit total equal to the default is now honoured. B. dead redirect branch. The manual-follow loop already owns the hop ceiling (TooManyRedirects when redirect_count >= opts.max_redirects), and only checked classify_redirect == Block, so the Stop arm and prior_hops/max_redirects threading were dead and misleading. Replaced classify_redirect (3 args, 3-variant enum) with redirect_target_blocked(host) -> bool doing ONLY the per-hop IP-literal airlock check. No behavioural change; removes the dead RedirectAction enum and the ceiling param. C. typed DnsError. reqwest collapses a dns_resolver failure into an opaque is_connect() error, so a resolution miss was indistinguishable from a dead server. Mirror the airlock tripped channel: thread a second dns_failed Arc<AtomicBool> into SafeDnsResolver, set it on a genuine NXDOMAIN/no-address miss, and have airlock_or map it to DnsError (airlock takes precedence). TLS-error detection is left as a focused TODO: the rustls backend is transitive, so there is no stable typed error to downcast to without a brittle string matcher; the reliable DNS half ships now.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the astrid:http host’s previously hardcoded limits configurable via a new global [http] configuration section, threading the resolved limits from astrid-config through the daemon/kernel/loader into each capsule HostState. It also includes follow-up fixes to redirect handling and error typing, plus related test updates and documentation/defaults updates.
Changes:
- Add
[http]config (HttpSection) and enforce operator-only control viamerge::restrict, with defaults matching historical constants. - Thread resolved
HttpLimitsthrough daemon → kernel → capsule loader/engine →HostState, and update the HTTP host implementation to read limits fromHostState.http_limits. - Apply follow-up behavior fixes (redirect hop checks, streaming timeout explicitness, DNS error typing) and add/adjust tests + defaults/docs + changelog entry.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/astrid-daemon/src/lib.rs | Resolves [http] config into typed HttpLimits and passes into Kernel::new. |
| crates/astrid-kernel/src/lib.rs | Stores http_limits and forwards it into capsule loader creation. |
| crates/astrid-capsule/src/engine/wasm/limits.rs | Introduces HttpLimits (typed, Duration-based) with historical-default Default. |
| crates/astrid-capsule/src/engine/wasm/host_state.rs | Adds http_limits to HostState and includes it in debug output. |
| crates/astrid-capsule/src/engine/wasm/mod.rs | Threads http_limits into pooled HostState creation; lifecycle path currently uses defaults. |
| crates/astrid-capsule/src/loader.rs | Extends loader to carry HttpLimits to WasmEngine. |
| crates/astrid-capsule/src/lib.rs | Re-exports HttpLimits. |
| crates/astrid-capsule/src/engine/wasm/host/http/backend.rs | Uses HostState.http_limits for quotas/timeouts; adds DNS-miss typing via resolver flags. |
| crates/astrid-capsule/src/engine/wasm/host/http/options.rs | Resolves request options against HttpLimits; fixes streaming explicit-total handling. |
| crates/astrid-capsule/src/engine/wasm/host/http/ssrf.rs | Simplifies redirect hop logic; adds dns_failed flag + typed DNS error mapping helpers. |
| crates/astrid-capsule/src/engine/wasm/host/http/mod.rs | Updates host trait impls to resolve options using HostState.http_limits. |
| crates/astrid-capsule/src/engine/wasm/host/http/tests.rs | Updates/unit-tests redirect behavior and option resolution against limits. |
| crates/astrid-capsule/src/engine/wasm/host/http/regression.rs | Adds regressions for configured limits + DNS error typing and updates existing regressions. |
| crates/astrid-config/src/types.rs | Adds HttpSection config type with defaults + parsing tests. |
| crates/astrid-config/src/merge/restrict.rs | Blocks workspace/project overrides of [http] as operator-only. |
| crates/astrid-config/src/merge/tests.rs | Adds tests asserting workspace layers cannot introduce/override [http]. |
| crates/astrid-config/src/show.rs | Adds a config show test ensuring [http] is visible. |
| crates/astrid-config/src/defaults.toml | Documents [http] defaults and semantics. |
| crates/astrid-hooks/src/handler/wasm.rs | Initializes hook HostState.http_limits to defaults (not resolved config). |
| crates/astrid-integration-tests/tests/*.rs | Updates integration tests for CapsuleLoader::new/Kernel::new signature changes. |
| crates/astrid-capsule/src/engine/wasm/test_fixtures.rs | Updates minimal host state fixture to include default http_limits. |
| CHANGELOG.md | Adds an Unreleased entry documenting the new [http] config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces operator-tunable configuration for astrid:http host limits, allowing for greater control over outbound HTTP traffic. It also includes several critical follow-up fixes related to streaming request timeouts, redirect target blocking, and DNS error handling, while ensuring that these security-sensitive limits cannot be bypassed by untrusted project configurations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Limits set by operator hand, HTTP traffic across the land. Configured tight, the bounds are clear, With DNS errors no longer to fear. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces operator-configurable HTTP limits for the astrid:http host, replacing previously hardcoded constants with a new [http] configuration section. These limits (including timeouts, redirect hops, concurrent streams, and response body size caps) are resolved by the daemon, stored on the kernel, and snapshotted onto each capsule's HostState. The changes also include a fix mapping DNS resolution failures to a typed DnsError and several tests. Feedback on the changes points out a redundant per-principal stream count check in backend.rs that can be removed to avoid an unnecessary O(N) iteration on every stream start.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ttp] semantics; fix test clippy
Addresses CI clippy and review comments on the [http] config branch.
Clippy (--all-targets): the three new config tests used
field_reassign_with_default (let mut x = T::default(); x.field = v). Converted
each to struct-init form (T { field: v, ..Default::default() }). CI clippy now
runs --all-targets, which surfaced these.
Threading: two HostState build sites used HttpLimits::default() instead of the
configured limits, so operator [http] policy didn't reach all HTTP-capable WASM.
Both now receive the real resolved limits:
- lifecycle hooks: added http_limits to LifecycleConfig; astrid-capsule-install
resolves it from the global [http] config and threads it in (mod.rs uses
cfg.http_limits).
- WASM hook handler (astrid-hooks): WasmHandler resolves [http] once at
construction and applies it to every hook HostState (self.http_limits).
Both load global config ([http] is operator-only global policy). Added a shared
HttpLimits::from_config_values constructor so the seconds->Duration conversion
lives in one place (daemon, install, hooks). New tests: from_config_values
conversion + defaults, and a hook HostState reflecting a configured limit.
Docs: corrected the [http] field docs (types.rs, defaults.toml, daemon
resolve_http_limits, HttpLimits) that overclaimed every field is a hard ceiling.
Precise now: the four timeout fields are per-request DEFAULTS a caller may
override with a LARGER value; max_redirects/max_concurrent_streams are caller
ceilings; max_response_bytes is a default+ceiling hard-clamped to
MAX_GUEST_PAYLOAD_LEN (the one hard cap). The operator is the trust root and may
raise or lower the soft limits; only a workspace/project layer is barred from
setting/widening [http].
…, warn on config-load failure Three PR re-review findings on the [http] config branch: 1. Redundant per-principal stream-quota clause (backend.rs http_stream_backend). The gate was per_principal_count >= max || active_http_streams.len() >= max with the SAME ceiling; since per_principal_count <= active_http_streams.len() the per-principal clause could never trip first — dead. Removed it (and the unused per_principal_count computation); kept active_http_streams.len() >= max as the per-capsule cap (matches the field doc). A real per-principal limit would need a separate, smaller bound — noted as out of scope. Behaviour-preserving; both quota tests still pass. 2. dns_failed was over-broad (ssrf.rs SafeDnsResolver::resolve): it set the flag for ANY lookup_host error, mislabeling a transient resolver timeout / I/O error as DnsError. Narrowed to ErrorKind::NotFound (genuine NXDOMAIN) via a new lookup_err_is_not_found helper; other kinds leave the flag clear and fall through to the generic reqwest classification. The empty-resolved-addresses path still sets dns_failed unconditionally (unambiguous no-resolve). Safe degradation: a not-found that surfaces as a different kind under-produces DnsError. 3. Silent fallback on config-load failure (resolve_http_limits in astrid-hooks and astrid-capsule-install): a malformed global config silently used HttpLimits::default(). Now emits tracing::warn! on the load-error branch before falling back, so the divergence is signalled. No other behaviour change. Tests: the prior .invalid resolver test asserted dns_failed on the lookup_host error path, but that host's error kind is platform-dependent (macOS reports Uncategorized, not NotFound), so after narrowing it would be flaky. Replaced it with two hermetic, platform-independent tests: one asserts the narrowing decision directly via lookup_err_is_not_found (NotFound -> set, TimedOut/Other -> clear), the other asserts flag_error maps the flags to DnsError/AirlockRejected with airlock precedence.
Both configured_max_concurrent_streams_lowers_quota and stream_quota_triggers_and_releases opened real streams via http_stream_backend against a one_shot_server that serves a single connection then closes. An open with no live responder blocked on the 120s streaming header-deadline floor, so the tests sat ~120s (reported as a 60s+ hang on CI). A test defect, not an impl bug. Fix (hermetic approach): the concurrency quota gate runs at the TOP of http_stream_backend, BEFORE any network, reading only active_http_streams.len(). The tests now pre-populate active_http_streams to the cap with synthetic streams (no sockets, no network) and assert a single over-cap http_stream_backend call returns ErrorCode::Quota immediately; close/drop are asserted to release a slot. The synthetic stream comes from a new #[cfg(test)] ActiveHttpStream::dummy_for_test that builds a no-network reqwest::Response via reqwest::Response::from(http::Response). Added http as a dev-dependency (test-only) for that construction. Both test bodies are wrapped in tokio::time::timeout(5s) as a hard backstop so this can never hang CI again. Assertions are unchanged (cap=2 -> 3rd open Quota; fill-to-cap -> over-cap Quota; close/drop release the slot). Each now runs in ~0.01s; the full http test module completes in 0.01s with no hang.
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
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.
Linked Issue
Closes #1053
Summary
Makes the seven hardcoded
astrid:httphost limits operator-tunable via a new global[http]config section, and folds in three follow-up review fixes from #1050. Builds on the now-mergedastrid:http@1.1.0host work (#1050); defaults equal today's constants, so an absent[http]section changes nothing.Changes
[http]operator config (astrid-config→ daemon → kernel →HostState, mirroring[security.capsule_local_egress]):HttpSection(7 knobs:default_timeout_secs,stream_connect_timeout_secs,stream_read_timeout_secs,header_deadline_secs,max_redirects,max_concurrent_streams,max_response_bytes) → resolved by the daemon into a typedHttpLimits→ stored on the kernel → snapshotted onto everyHostState(global, like the runtime concurrency limits). The http host reads its defaults/ceilings offHostState.http_limitsinstead of module constants.merge::restrictreverts the whole[http]section to the operator baseline if a workspace/project layer touches it — untrusted project config cannot widen the host's HTTP limits.request-optionsmay tighten within the ceiling; a configured ceiling never lets a caller exceed it;max_response_bytesis additionally hard-clamped toMAX_GUEST_PAYLOAD_LEN.config show+defaults.toml(documented, commented).Follow-up fixes from #1050 review (separate commit):
total-msequal to the configured default on the streaming path (explicit-flag instead of value-equality).classify_redirect→redirect_target_blocked(IP-literal airlock only); the manual-follow loop already owns the hop ceiling.DnsError(adns_failedresolver flag mirroring the airlocktrippedchannel; airlock takes precedence). TLS error typing left as a documented TODO — rustls is transitive (hyper-rustls), so there's no stable typed error to downcast to without a brittle string matcher.Verification
cargo build --workspace,cargo clippy --workspace --all-features -- -D warnings(0),cargo fmt --all --check: green on the rebased branch. 45 http tests + 104astrid-configtests pass locally.config showsurfaces the section; the operator-only merge guard rejects a workspace override; and config actually changes behaviour (max_concurrent_streams=2→ 3rd streamQuota;default_timeout_secs=5drives the resolved default;max_redirects=3clamps a larger caller value). Plus regressions for each of the three follow-up fixes.test_http_section_cannot_be_set_by_workspaceproves a workspace raisingmax_redirects=100/max_concurrent_streams=64/max_response_bytes=1GiBis fully reverted to the operator baseline.Checklist
[Unreleased])