feat(http): standalone HTTP server worker (duplicate of built-in iii-http) - #389
Merged
Conversation
HttpTriggerHandler bridges the SDK's register/unregister_trigger callbacks to RouteTable; unregister only relies on config.id since the SDK passes a stub for the rest.
Add server.rs (bind, router, CORS/timeout/concurrency, graceful shutdown), handler.rs (buffered dynamic_handler), boot.rs (register http trigger type + serve). Wire main.rs. Add literal-over-param precedence to match_route. Derive JsonSchema on HttpRequest/TriggerMetadata and Serialize on HttpTriggerConfig for the trigger-type builder.
Add tests/common/{engine,worker,backend,mod} (connect-or-skip engine,
per-test worker boot, echo backend) and e2e_methods.rs covering
GET/POST/PUT/PATCH/DELETE echo + unmatched-route 404.
README covers install, quickstart, configuration, and the http trigger type per worker-readme.md conventions. Closes the config.rs unit-test gap for RestApiConfig::json_schema() embedding nested $ref definitions, previously only covered indirectly via e2e.
get_or_init now panics (fails the test) if no engine is reachable, and returns Arc<IIIClient> directly. e2e tests only make sense with an engine, so a silent skip-as-pass gave false-green results.
Two requests to two routes; assert the respond-middleware counter == 2.
Replaces the static 404 test with one that registers a route, hits it (200), unregisters the trigger, and asserts the route now 404s -- exercising the TriggerHandler unregister -> RouteTable removal path end-to-end.
… when iii-http builtin active The built-in iii-http worker owns the `http` trigger type; two owners on one engine collide (last-write-wins). Default this worker to `http-ng` so it runs safely alongside iii-http, and add a boot guard that refuses to start when III_HTTP_TRIGGER_TYPE=http is set while iii-http is still connected, turning the silent collision into a fail-fast error.
…sting route Match the engine's iii-http (axum per-method routing) parity: a path that matches a registered route but with the wrong HTTP method now returns 405 Method Not Allowed with an Allow header, instead of falling through to the generic 404.
Covers the previously-untested direction: a function reading its streamed request body via ChannelReader on HttpRequest.request_body, for both non-JSON and JSON content types. Mirrors the e2e_streaming.rs harness.
Verifies (test-driven) that a slow handler function returns 504 Gateway Timeout, matching the engine's iii-http: the tower TimeoutLayer wraps the whole router, so its deadline always starts (and thus fires) before the handler's own per-invocation iii.trigger timeout of the same duration — confirmed with repeated runs and a tight-margin stress loop, so no production fix was needed. Also adds a throughput smoke test for the ConcurrencyLimitLayer (queues excess requests; all must still succeed). Adds start_http_worker_with_timeout / start_http_worker_with_concurrency_limit test helpers and register_slow_backend / register_sleep_backend fixtures.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new Changesiii-http worker crate
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DynamicHandler
participant Middleware
participant ConditionFn
participant BackendFunction
participant ResponseChannel
Client->>DynamicHandler: HTTP request
DynamicHandler->>DynamicHandler: match route, snapshot config, start span
DynamicHandler->>Middleware: execute_middleware(preHandler)
Middleware-->>DynamicHandler: Continue or short-circuit Response
DynamicHandler->>ConditionFn: check_condition(condition_function_id)
ConditionFn-->>DynamicHandler: true/false
DynamicHandler->>BackendFunction: trigger(HttpRequest)
BackendFunction->>ResponseChannel: write control frames + body chunks
ResponseChannel-->>DynamicHandler: streamed bytes / buffered return
DynamicHandler-->>Client: HTTP response (buffered or chunked)
sequenceDiagram
participant ConfigBus
participant ConfigTrigger
participant ConfigCell
participant HotRouter
participant NewServer
participant OldServer
ConfigBus->>ConfigTrigger: configuration:updated (http)
ConfigTrigger->>ConfigBus: fetch_config
ConfigBus-->>ConfigTrigger: RestApiConfig
alt same host/port
ConfigTrigger->>ConfigCell: apply_config (swap snapshot)
ConfigTrigger->>HotRouter: rebuild_layers
else host/port changed
ConfigTrigger->>NewServer: bind new listener
ConfigTrigger->>ConfigCell: apply_config (swap snapshot)
ConfigTrigger->>HotRouter: rebuild_layers
ConfigTrigger->>NewServer: spawn_server
ConfigTrigger->>OldServer: stop_old_server (graceful then hard abort)
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
skill-check — worker0 verified, 31 skipped (no docs/).
Four for four. Nicely done. |
…th engine) The SDK dispatches each function invocation via tokio::spawn, so http::on-config-change can run concurrently for overlapping configuration:updated events, letting two edits race on the ConfigCell/ ServerControlCell swap. Add an apply_lock: Arc<tokio::sync::Mutex<()>>, created in boot::start alongside the other shared cells and threaded through register_config_trigger into on_config_change, which now holds it for the whole re-fetch/validate/swap/rebuild-or-rebind sequence. Mirrors the engine's iii-http apply_lock in api_core.rs.
…ngine, dead_code, fmt)
Trigger type is now a constant `iii_http::TRIGGER_TYPE = "http"` (no
III_HTTP_TRIGGER_TYPE env, no http-ng default) and the boot guard
unconditionally refuses to start if the built-in iii-http worker is active,
since there's no longer a coexistence mode. Drops the now-obsolete cutover
test and updates the README accordingly.
Also makes the resulting CI (fmt/clippy/test, run with no engine) green:
- tests/common/engine.rs: get_or_init/connect_fresh go back to
connect-or-skip (Option<Arc<IIIClient>>, matching sibling workers) instead
of panicking when no engine is reachable, so cargo test doesn't fail in
CI. Set III_E2E_REQUIRE=1 to force the old panic-on-missing-engine
behavior for intentional e2e runs. All call sites updated to
`let Some(iii) = engine::get_or_init().await else { return };`.
- tests/common/{backend,engine,mod,worker}.rs: `#![allow(dead_code)]` per
file, since each e2e binary only exercises a subset of the shared
helpers and -D warnings was failing on dead_code.
- cargo fmt --all from inside http/ (its own workspace) to clear
pre-existing formatting drift across src/ and tests/.
…idation) iii worker add extracts the release archive by worker name, so the [[bin]] name and iii.worker.yaml bin must equal 'http', not 'iii-http'.
guibeira
marked this pull request as ready for review
July 2, 2026 19:22
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
http/src/config.rs (2)
424-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise automatic sorting.
rest_api_config_middleware_pre_sortedmanually callsconfig.middleware.sort_by_key(...)after deserializing rather than calling.normalized(), so it doesn't actually verify the sorting behavior implemented inRestApiConfig::normalized()(Lines 70-79). It only re-testsVec::sort_by_keyitself. Consider asserting viaconfig.normalized()instead to cover the real code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/src/config.rs` around lines 424 - 437, The test rest_api_config_middleware_pre_sorted is bypassing the real sorting logic by calling sort_by_key directly, so it does not cover RestApiConfig::normalized. Update the test to deserialize into RestApiConfig and then assert on the result of config.normalized() instead, using the middleware field and function_id/priority values to verify the automatic ordering path.
128-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
phaseis an unvalidated free-form string.Only
"preHandler"is currently supported per the doc comment, butphaseaccepts any string with no enum or validation — a typo (e.g."preHalndler") would silently passdeny_unknown_fieldsand JSON schema validation, and only fail (or silently no-op) downstream whereverphaseis consumed. Consider using a#[serde(rename_all = "camelCase")]enum with a singlePreHandlervariant (extensible later) instead ofString, so invalid values are rejected at deserialize/schema time rather than at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/src/config.rs` around lines 128 - 142, The MiddlewareConfig.phase field is currently an unvalidated String, so invalid values can slip through deserialization and schema checks. Replace it with a strongly typed enum (for example a single PreHandler variant) and update the defaulting logic currently in default_phase to return that enum value. Make sure the serde representation uses the existing camelCase naming so MiddlewareConfig, phase, and any downstream consumers reject typos at config parse time instead of failing later.http/tests/common/backend.rs (1)
22-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated register_function + register_trigger boilerplate.
Each of the seven
register_*helpers repeats the sameregister_function(...)+register_trigger(RegisterTriggerInput { trigger_type: iii_http::TRIGGER_TYPE, ... }).expect(...)shape, differing only in function id and config JSON. A small shared helper (e.g.fn bind_http_trigger(iii, function_id, config: Value)) would reduce duplication across this file.Separately,
register_slow_backend(Lines 232-258) andregister_sleep_backend(Lines 264-290) have identical bodies aside from the function-id prefix; one could delegate to the other.Also applies to: 54-78, 85-120, 232-258, 264-290, 296-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/tests/common/backend.rs` around lines 22 - 48, The HTTP backend helpers repeat the same register_function plus RegisterTriggerInput::register_trigger pattern, so extract that boilerplate into a shared helper such as bind_http_trigger and have register_echo_backend and the other register_* helpers call it with their function_id and config JSON. Also remove the duplication between register_slow_backend and register_sleep_backend by making one delegate to the other, keeping only the differing function-id prefix.http/src/main.rs (1)
128-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd SIGTERM handling for graceful shutdown.
Only
ctrl_c()is awaited before shutdown. In containerized/orchestrated environments (Docker, Kubernetes), stop/redeploy typically sends SIGTERM, not SIGINT — without aSIGTERMbranch this worker would be force-killed without runningboot.shutdown()/iii.shutdown_async(), skipping graceful connection/listener cleanup.♻️ Proposed fix to also await SIGTERM on unix
+ #[cfg(unix)] + let terminate = async { + tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate()) + .expect("failed to install SIGTERM handler") + .recv() + .await; + }; + #[cfg(not(unix))] + let terminate = std::future::pending::<()>(); + - tokio::signal::ctrl_c().await?; + tokio::select! { + _ = tokio::signal::ctrl_c() => {}, + _ = terminate => {}, + } tracing::info!("iii-http shutting down");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/src/main.rs` around lines 128 - 133, Add SIGTERM-aware shutdown handling in main so graceful cleanup runs in orchestrated environments. Update the shutdown wait logic around the existing tokio::signal::ctrl_c() call in main to also listen for SIGTERM on Unix (for example via tokio::signal::unix), then proceed through the same shutdown path that calls boot.shutdown() and iii.shutdown_async(). Keep the shutdown flow centralized so both ctrl_c and SIGTERM trigger the same cleanup sequence.http/tests/e2e_methods.rs (1)
16-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared boilerplate across the 5 method tests.
GET/POST/PUT/PATCH/DELETE tests are structurally identical (connect → boot → register → wait_for_route → request → assert → shutdown), differing only in method name, request body, and expected echoed field. Consolidating into a table-driven helper would cut ~90 lines of duplication and make adding new methods trivial.
♻️ Suggested direction (table-driven helper)
async fn run_method_test(method: reqwest::Method, path: &str, body: Option<Value>) { let Some(iii) = engine::get_or_init().await else { return; }; let boot = worker::start_http_worker(iii.clone()).await; backend::register_echo_backend(&iii, path, method.as_str()).await; common::wait_for_route(&boot.routes, method.as_str(), path).await; let url = format!("http://{}{}", boot.local_addr, path); let mut req = reqwest::Client::new().request(method.clone(), &url); if let Some(b) = &body { req = req.json(b); } let resp = req.send().await.unwrap(); assert_eq!(resp.status(), 200); let v: Value = resp.json().await.unwrap(); assert_eq!(v["method"], method.as_str()); if let Some(b) = &body { assert_eq!(&v["body"], b); } boot.shutdown().await; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/tests/e2e_methods.rs` around lines 16 - 128, The five tests in get_echoes_method_and_query, post_echoes_body, put_echoes_body, patch_echoes_body, and delete_echoes_method duplicate the same setup/request/assert/shutdown flow. Refactor this into a shared helper (for example, a table-driven runner near these test functions) that takes the HTTP method, route path, and optional JSON body, then performs engine::get_or_init, worker::start_http_worker, backend::register_echo_backend, common::wait_for_route, and the request/assertions. Keep each test as a thin invocation of the helper with its method-specific expectations.http/tests/e2e_reload.rs (1)
20-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the repeated poll-until helpers.
wait_for_middleware,wait_for_cors,wait_for_port,wait_until_serves, andwait_until_refusedall repeat the same "loop N times, sleep, check predicate, else panic/return false" shape. A single genericpoll_until(predicate, attempts, interval)helper (incommon/mod.rs, alongside the existingwait_for_route) would reduce duplication as more reload tests are added.♻️ Example generic helper
pub async fn poll_until<F, Fut>(mut check: F, attempts: usize, interval: Duration) -> bool where F: FnMut() -> Fut, Fut: std::future::Future<Output = bool>, { for _ in 0..attempts { if check().await { return true; } tokio::time::sleep(interval).await; } false }Also applies to: 238-276
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/tests/e2e_reload.rs` around lines 20 - 42, The reload test helpers repeat the same poll/sleep/check pattern across wait_for_middleware and wait_for_cors, and the same shape also appears in wait_for_port, wait_until_serves, and wait_until_refused. Extract this into a reusable generic polling helper, like poll_until, in common/mod.rs near wait_for_route, then update the existing helpers to delegate to it with their specific predicates and success/failure behavior. Keep the helper flexible enough to support both “return early” and “panic/false on timeout” call sites.http/src/configuration.rs (1)
120-133: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRetrying on
NOT_FOUNDadds needless boot latency.
configuration::getreturningNOT_FOUNDis the expected answer on first boot (no stored value yet), buttrigger_with_retrytreats everyErras transient and retriesCONFIG_RETRIEStimes with backoff (~250ms + 500ms ≈ 750ms) before theNOT_FOUNDbranch here even runs. Sincetry_get_config_valueis called by bothshould_seed_initial_valueandfetch_config, a fresh install pays this delay more than once. Consider short-circuitingNOT_FOUNDso it isn't retried (e.g., a non-retrying get, or a "retryable" predicate passed totrigger_with_retry).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http/src/configuration.rs` around lines 120 - 133, `try_get_config_value` is retrying the expected `NOT_FOUND` case via `trigger_with_retry`, which adds unnecessary boot delay. Update the configuration lookup path in `try_get_config_value` (and any shared helper used by `should_seed_initial_value`/`fetch_config`) so `NOT_FOUND` is treated as a non-retryable result and returns `Ok(None)` immediately, either by using a non-retrying get call or by adding a retryability predicate to `trigger_with_retry` that excludes `NOT_FOUND`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@http/iii.worker.yaml`:
- Around line 9-15: The seeded HTTP worker manifest uses a host default that
conflicts with the documented/configured default. Update the host value in the
http worker seed to match config.rs::default_host() and the README Configuration
table, or change those docs if the new default is intentional. Keep the seed
aligned with iii worker add http so new installs bind consistently.
In `@http/README.md`:
- Around line 70-77: The HTTP docs and the shipped worker template disagree on
the default host value, so update the README table to match the actual
new-install seed used by the HTTP worker config or otherwise align both sources.
Use the existing `RestApiConfig::default()`/`config.rs` default and the
`http/iii.worker.yaml` template as the reference points, and make sure the
`host` row in `http/README.md` reflects the same bind address users get after
`iii worker add http`.
In `@http/src/trigger.rs`:
- Around line 34-47: `route_signature` is not normalizing path segments the same
way as `extract_path_params` and `match_route`, so routes that differ only by
leading/trailing slashes can evade conflict detection. Update `route_signature`
to drop empty segments when building the signature, keeping its normalization
consistent with the other path-matching helpers and the `api_path` convention.
Use `route_signature` as the main place to align signature generation with
`extract_path_params`/`match_route`.
In `@http/tests/e2e_methods.rs`:
- Around line 16-145: The e2e tests currently rely on explicit
boot.shutdown().await calls in functions like get_echoes_method_and_query,
post_echoes_body, put_echoes_body, patch_echoes_body, delete_echoes_method, and
unmatched_route_returns_404_envelope, so an early panic can leave the worker
running. Add a panic-safe teardown path by introducing RAII ownership around
BootHandle or a guard that triggers shutdown automatically in Drop, and apply
the same pattern to the matching e2e_routing.rs, e2e_cors.rs, and e2e_errors.rs
tests so cleanup always runs even when assertions fail.
---
Nitpick comments:
In `@http/src/config.rs`:
- Around line 424-437: The test rest_api_config_middleware_pre_sorted is
bypassing the real sorting logic by calling sort_by_key directly, so it does not
cover RestApiConfig::normalized. Update the test to deserialize into
RestApiConfig and then assert on the result of config.normalized() instead,
using the middleware field and function_id/priority values to verify the
automatic ordering path.
- Around line 128-142: The MiddlewareConfig.phase field is currently an
unvalidated String, so invalid values can slip through deserialization and
schema checks. Replace it with a strongly typed enum (for example a single
PreHandler variant) and update the defaulting logic currently in default_phase
to return that enum value. Make sure the serde representation uses the existing
camelCase naming so MiddlewareConfig, phase, and any downstream consumers reject
typos at config parse time instead of failing later.
In `@http/src/configuration.rs`:
- Around line 120-133: `try_get_config_value` is retrying the expected
`NOT_FOUND` case via `trigger_with_retry`, which adds unnecessary boot delay.
Update the configuration lookup path in `try_get_config_value` (and any shared
helper used by `should_seed_initial_value`/`fetch_config`) so `NOT_FOUND` is
treated as a non-retryable result and returns `Ok(None)` immediately, either by
using a non-retrying get call or by adding a retryability predicate to
`trigger_with_retry` that excludes `NOT_FOUND`.
In `@http/src/main.rs`:
- Around line 128-133: Add SIGTERM-aware shutdown handling in main so graceful
cleanup runs in orchestrated environments. Update the shutdown wait logic around
the existing tokio::signal::ctrl_c() call in main to also listen for SIGTERM on
Unix (for example via tokio::signal::unix), then proceed through the same
shutdown path that calls boot.shutdown() and iii.shutdown_async(). Keep the
shutdown flow centralized so both ctrl_c and SIGTERM trigger the same cleanup
sequence.
In `@http/tests/common/backend.rs`:
- Around line 22-48: The HTTP backend helpers repeat the same register_function
plus RegisterTriggerInput::register_trigger pattern, so extract that boilerplate
into a shared helper such as bind_http_trigger and have register_echo_backend
and the other register_* helpers call it with their function_id and config JSON.
Also remove the duplication between register_slow_backend and
register_sleep_backend by making one delegate to the other, keeping only the
differing function-id prefix.
In `@http/tests/e2e_methods.rs`:
- Around line 16-128: The five tests in get_echoes_method_and_query,
post_echoes_body, put_echoes_body, patch_echoes_body, and delete_echoes_method
duplicate the same setup/request/assert/shutdown flow. Refactor this into a
shared helper (for example, a table-driven runner near these test functions)
that takes the HTTP method, route path, and optional JSON body, then performs
engine::get_or_init, worker::start_http_worker, backend::register_echo_backend,
common::wait_for_route, and the request/assertions. Keep each test as a thin
invocation of the helper with its method-specific expectations.
In `@http/tests/e2e_reload.rs`:
- Around line 20-42: The reload test helpers repeat the same poll/sleep/check
pattern across wait_for_middleware and wait_for_cors, and the same shape also
appears in wait_for_port, wait_until_serves, and wait_until_refused. Extract
this into a reusable generic polling helper, like poll_until, in common/mod.rs
near wait_for_route, then update the existing helpers to delegate to it with
their specific predicates and success/failure behavior. Keep the helper flexible
enough to support both “return early” and “panic/false on timeout” call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e62ce3ac-5546-4439-8448-a66f9ffc3143
⛔ Files ignored due to path filters (1)
http/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.github/workflows/release.ymlhttp/Cargo.tomlhttp/README.mdhttp/build.rshttp/iii.worker.yamlhttp/src/boot.rshttp/src/condition.rshttp/src/config.rshttp/src/configuration.rshttp/src/handler.rshttp/src/lib.rshttp/src/main.rshttp/src/manifest.rshttp/src/middleware.rshttp/src/observability.rshttp/src/server.rshttp/src/trigger.rshttp/src/types.rshttp/tests/common/backend.rshttp/tests/common/engine.rshttp/tests/common/mod.rshttp/tests/common/worker.rshttp/tests/e2e_condition.rshttp/tests/e2e_cors.rshttp/tests/e2e_errors.rshttp/tests/e2e_methods.rshttp/tests/e2e_middleware.rshttp/tests/e2e_otel.rshttp/tests/e2e_otel_metrics.rshttp/tests/e2e_reload.rshttp/tests/e2e_request_body.rshttp/tests/e2e_routing.rshttp/tests/e2e_streaming.rshttp/tests/e2e_timeout.rs
Comment on lines
+9
to
+15
| port: 3111 | ||
| host: 127.0.0.1 | ||
| default_timeout: 30000 | ||
| concurrency_request_limit: 1024 | ||
| cors: | ||
| allowed_origins: ['*'] | ||
| allowed_methods: [GET, POST, PUT, DELETE, OPTIONS] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
host seed value diverges from the code default and README doc.
Code default (config.rs::default_host()) and the README's Configuration table both document host as 0.0.0.0, but this manifest seeds new installs with 127.0.0.1. A user reading the README quickstart would expect the worker to listen on all interfaces by default, but iii worker add http will actually bind only to loopback.
💡 Align the seed with the documented default, or update the README
config:
port: 3111
- host: 127.0.0.1
+ host: 0.0.0.0
default_timeout: 30000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| port: 3111 | |
| host: 127.0.0.1 | |
| default_timeout: 30000 | |
| concurrency_request_limit: 1024 | |
| cors: | |
| allowed_origins: ['*'] | |
| allowed_methods: [GET, POST, PUT, DELETE, OPTIONS] | |
| port: 3111 | |
| host: 0.0.0.0 | |
| default_timeout: 30000 | |
| concurrency_request_limit: 1024 | |
| cors: | |
| allowed_origins: ['*'] | |
| allowed_methods: [GET, POST, PUT, DELETE, OPTIONS] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/iii.worker.yaml` around lines 9 - 15, The seeded HTTP worker manifest
uses a host default that conflicts with the documented/configured default.
Update the host value in the http worker seed to match config.rs::default_host()
and the README Configuration table, or change those docs if the new default is
intentional. Keep the seed aligned with iii worker add http so new installs bind
consistently.
Comment on lines
+70
to
+77
| | Field | Default | Description | | ||
| |---|---|---| | ||
| | `port` | `3111` | TCP port the HTTP server binds to. `0` binds an OS-assigned ephemeral port. | | ||
| | `host` | `0.0.0.0` | Host/interface to bind. | | ||
| | `default_timeout` | `30000` (ms) | Per-request timeout; on expiry the server returns `504`. | | ||
| | `cors.allowed_origins` | `[]` (permissive) | Allowed CORS origins. An empty list allows any origin. | | ||
| | `cors.allowed_methods` | `[]` (permissive) | Allowed CORS methods. An empty list allows any method. | | ||
| | `concurrency_request_limit` | `1024` | Maximum in-flight requests; requests over the limit wait for a slot. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Documented host default doesn't match the shipped worker config.
This table states host defaults to 0.0.0.0, matching RestApiConfig::default() in config.rs. However, http/iii.worker.yaml seeds new installs with host: 127.0.0.1, so users following this doc will see a different actual bind address after iii worker add http. Reconcile the two (see companion comment on http/iii.worker.yaml).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/README.md` around lines 70 - 77, The HTTP docs and the shipped worker
template disagree on the default host value, so update the README table to match
the actual new-install seed used by the HTTP worker config or otherwise align
both sources. Use the existing `RestApiConfig::default()`/`config.rs` default
and the `http/iii.worker.yaml` template as the reference points, and make sure
the `host` row in `http/README.md` reflects the same bind address users get
after `iii worker add http`.
Comment on lines
+34
to
+47
| pub fn route_signature(http_method: &str, http_path: &str) -> String { | ||
| let shape = http_path | ||
| .split('/') | ||
| .map(|segment| { | ||
| if segment.starts_with(':') { | ||
| "{}" | ||
| } else { | ||
| segment | ||
| } | ||
| }) | ||
| .collect::<Vec<&str>>() | ||
| .join("/"); | ||
| format!("{}:{}", http_method.to_uppercase(), shape) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
route_signature doesn't normalize empty segments, so conflict detection can be bypassed.
route_signature splits on / without dropping empty segments, whereas extract_path_params/match_route filter them (!s.is_empty()). As a result, two routes that differ only by leading/trailing slashes (e.g. "/a/:x" vs "a/:x", or "/a/:x/" vs "/a/:x") produce different signatures and escape the conflict check, yet both match the same request paths with identical param counts — leaving match_route's outcome dependent on nondeterministic HashMap iteration order. Normalizing here (filtering empty segments) keeps signature/matching consistent. This also aligns with the guidance that api_path should be free of leading slashes.
🛠️ Proposed normalization
pub fn route_signature(http_method: &str, http_path: &str) -> String {
let shape = http_path
.split('/')
+ .filter(|segment| !segment.is_empty())
.map(|segment| {
if segment.starts_with(':') {
"{}"
} else {
segment
}
})
.collect::<Vec<&str>>()
.join("/");
format!("{}:{}", http_method.to_uppercase(), shape)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn route_signature(http_method: &str, http_path: &str) -> String { | |
| let shape = http_path | |
| .split('/') | |
| .map(|segment| { | |
| if segment.starts_with(':') { | |
| "{}" | |
| } else { | |
| segment | |
| } | |
| }) | |
| .collect::<Vec<&str>>() | |
| .join("/"); | |
| format!("{}:{}", http_method.to_uppercase(), shape) | |
| } | |
| pub fn route_signature(http_method: &str, http_path: &str) -> String { | |
| let shape = http_path | |
| .split('/') | |
| .filter(|segment| !segment.is_empty()) | |
| .map(|segment| { | |
| if segment.starts_with(':') { | |
| "{}" | |
| } else { | |
| segment | |
| } | |
| }) | |
| .collect::<Vec<&str>>() | |
| .join("/"); | |
| format!("{}:{}", http_method.to_uppercase(), shape) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/src/trigger.rs` around lines 34 - 47, `route_signature` is not
normalizing path segments the same way as `extract_path_params` and
`match_route`, so routes that differ only by leading/trailing slashes can evade
conflict detection. Update `route_signature` to drop empty segments when
building the signature, keeping its normalization consistent with the other
path-matching helpers and the `api_path` convention. Use `route_signature` as
the main place to align signature generation with
`extract_path_params`/`match_route`.
Source: Learnings
Comment on lines
+16
to
+145
| #[tokio::test] | ||
| #[serial] | ||
| async fn get_echoes_method_and_query() { | ||
| let Some(iii) = engine::get_or_init().await else { | ||
| return; | ||
| }; | ||
| let boot = worker::start_http_worker(iii.clone()).await; | ||
| backend::register_echo_backend(&iii, "/echo-get", "GET").await; | ||
| common::wait_for_route(&boot.routes, "GET", "/echo-get").await; | ||
|
|
||
| let url = format!("http://{}/echo-get?q=1", boot.local_addr); | ||
| let resp = reqwest::Client::new().get(&url).send().await.unwrap(); | ||
| assert_eq!(resp.status(), 200); | ||
| let v: serde_json::Value = resp.json().await.unwrap(); | ||
| assert_eq!(v["method"], "GET"); | ||
| assert_eq!(v["query_params"]["q"], "1"); | ||
|
|
||
| boot.shutdown().await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn post_echoes_body() { | ||
| let Some(iii) = engine::get_or_init().await else { | ||
| return; | ||
| }; | ||
| let boot = worker::start_http_worker(iii.clone()).await; | ||
| backend::register_echo_backend(&iii, "/echo-post", "POST").await; | ||
| common::wait_for_route(&boot.routes, "POST", "/echo-post").await; | ||
|
|
||
| let url = format!("http://{}/echo-post", boot.local_addr); | ||
| let resp = reqwest::Client::new() | ||
| .post(&url) | ||
| .json(&json!({ "hi": 1 })) | ||
| .send() | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status(), 200); | ||
| let v: serde_json::Value = resp.json().await.unwrap(); | ||
| assert_eq!(v["method"], "POST"); | ||
| assert_eq!(v["body"]["hi"], 1); | ||
|
|
||
| boot.shutdown().await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn put_echoes_body() { | ||
| let Some(iii) = engine::get_or_init().await else { | ||
| return; | ||
| }; | ||
| let boot = worker::start_http_worker(iii.clone()).await; | ||
| backend::register_echo_backend(&iii, "/echo-put", "PUT").await; | ||
| common::wait_for_route(&boot.routes, "PUT", "/echo-put").await; | ||
|
|
||
| let url = format!("http://{}/echo-put", boot.local_addr); | ||
| let resp = reqwest::Client::new() | ||
| .put(&url) | ||
| .json(&json!({ "n": 7 })) | ||
| .send() | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status(), 200); | ||
| let v: serde_json::Value = resp.json().await.unwrap(); | ||
| assert_eq!(v["method"], "PUT"); | ||
| assert_eq!(v["body"]["n"], 7); | ||
|
|
||
| boot.shutdown().await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn patch_echoes_body() { | ||
| let Some(iii) = engine::get_or_init().await else { | ||
| return; | ||
| }; | ||
| let boot = worker::start_http_worker(iii.clone()).await; | ||
| backend::register_echo_backend(&iii, "/echo-patch", "PATCH").await; | ||
| common::wait_for_route(&boot.routes, "PATCH", "/echo-patch").await; | ||
|
|
||
| let url = format!("http://{}/echo-patch", boot.local_addr); | ||
| let resp = reqwest::Client::new() | ||
| .patch(&url) | ||
| .json(&json!({ "p": true })) | ||
| .send() | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status(), 200); | ||
| let v: serde_json::Value = resp.json().await.unwrap(); | ||
| assert_eq!(v["method"], "PATCH"); | ||
| assert_eq!(v["body"]["p"], true); | ||
|
|
||
| boot.shutdown().await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn delete_echoes_method() { | ||
| let Some(iii) = engine::get_or_init().await else { | ||
| return; | ||
| }; | ||
| let boot = worker::start_http_worker(iii.clone()).await; | ||
| backend::register_echo_backend(&iii, "/echo-delete", "DELETE").await; | ||
| common::wait_for_route(&boot.routes, "DELETE", "/echo-delete").await; | ||
|
|
||
| let url = format!("http://{}/echo-delete", boot.local_addr); | ||
| let resp = reqwest::Client::new().delete(&url).send().await.unwrap(); | ||
| assert_eq!(resp.status(), 200); | ||
| let v: serde_json::Value = resp.json().await.unwrap(); | ||
| assert_eq!(v["method"], "DELETE"); | ||
|
|
||
| boot.shutdown().await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn unmatched_route_returns_404_envelope() { | ||
| let Some(iii) = engine::get_or_init().await else { | ||
| return; | ||
| }; | ||
| let boot = worker::start_http_worker(iii.clone()).await; | ||
|
|
||
| let url = format!("http://{}/no-such-route", boot.local_addr); | ||
| let resp = reqwest::Client::new().get(&url).send().await.unwrap(); | ||
| assert_eq!(resp.status(), 404); | ||
| let v: serde_json::Value = resp.json().await.unwrap(); | ||
| assert_eq!(v["error"]["code"], "NOT_FOUND"); | ||
|
|
||
| boot.shutdown().await; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether BootHandle has Drop-based cleanup.
fd -e rs . http/src --exec rg -n -A5 'struct BootHandle|impl Drop for BootHandle' {}Repository: iii-hq/workers
Length of output: 354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant HTTP worker files first, then inspect BootHandle and shutdown logic.
fd -a -e rs . http | sort
printf '\n--- BootHandle outline ---\n'
ast-grep outline http/src -q 'BootHandle' || true
printf '\n--- Search for BootHandle / shutdown / Drop definitions ---\n'
rg -n -A8 -B4 'struct BootHandle|impl Drop for BootHandle|async fn shutdown|fn shutdown' http/srcRepository: iii-hq/workers
Length of output: 3106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- http/src/boot.rs ---'
sed -n '1,180p' http/src/boot.rs
printf '\n%s\n' '--- http/tests/common/worker.rs ---'
sed -n '1,220p' http/tests/common/worker.rs
printf '\n%s\n' '--- http/src/server.rs ---'
sed -n '1,260p' http/src/server.rsRepository: iii-hq/workers
Length of output: 22232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Drop implementations in http/src and http/tests/common ---'
rg -n 'impl Drop for|shutdown\(self\)|graceful\.send|join\.await|ServerControlCell|type ServerControlCell' http/src http/tests/commonRepository: iii-hq/workers
Length of output: 1683
Make worker cleanup panic-safe
BootHandle has no Drop, so an early panic skips boot.shutdown().await and can leave the test worker running for later #[serial] cases. Move teardown into RAII/Drop (or a guard that always shuts down) in these e2e tests and the matching e2e_routing.rs, e2e_cors.rs, and e2e_errors.rs cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/tests/e2e_methods.rs` around lines 16 - 145, The e2e tests currently
rely on explicit boot.shutdown().await calls in functions like
get_echoes_method_and_query, post_echoes_body, put_echoes_body,
patch_echoes_body, delete_echoes_method, and
unmatched_route_returns_404_envelope, so an early panic can leave the worker
running. Add a panic-safe teardown path by introducing RAII ownership around
BootHandle or a guard that triggers shutdown automatically in Drop, and apply
the same pattern to the matching e2e_routing.rs, e2e_cors.rs, and e2e_errors.rs
tests so cleanup always runs even when assertions fail.
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.
What
Standalone
httpworker in the registry that duplicates the engine's built-in iii-http (inbound HTTP server), running as a normal SDK worker binary instead of an in-process engine module. Registers thehttptrigger type, runs its own axum server, routes incoming requests to functions via the SDK. All code underhttp/; the engine is untouched.Features (parity with engine/src/workers/rest_api)
http(api_path, http_method, condition_function_id, middleware_function_ids); route table keyed by trigger id.:paramprecedence, conflict rejection, 404, 405 + Allow header.middleware/default_timeoutvia a per-request config cell;cors/timeout/concurrency_request_limitvia a swappable HotRouter (same-address layer rebuild);host/portvia live listener rebind (bind-new-before-stop-old, graceful drain + abort safety net). Overlapping config events serialized by an apply lock.iii.http.requestscounter metric, and the tracing→OTel bridge wired inmain.rsso spans export in production.III_HTTP_TRIGGER_TYPE, defaulthttp-ng) so it runs alongside the built-in; boot refuses if set tohttpwhile the built-in iii-http is active.Tests
Unit + e2e against a live engine (the e2e harness FAILS, not skips, when no engine is reachable): methods, routing, 405, middleware (with call-count assertions), condition, CORS, streaming (incl. trigger-wins-the-race + request-body), timeout/concurrency, config hot-reload (middleware, CORS same-address, host/port rebind), OTEL span/trace-context/metric capture, and cutover. Every change was reviewed (spec + quality) before landing.
Known non-blocking divergences (documented)
concurrency_request_limitis not a global cap — matches the engine's identicalConcurrencyLimitLayerconstruction (likely an upstream behavior).condition.rstimeout maps to 500 vs middleware's 504 (unreachable behind the tower TimeoutLayer).Transition plan (support both now, remove iii-http later)
Worker default trigger type
http-ng(coexist) → at cutover setIII_HTTP_TRIGGER_TYPE=httpand omitiii-httpfrom the engineconfig.yaml(with a config file, an unlisted default worker does not run) → later remove the built-in. Same trigger config / request / response / RestApiConfig contracts, so function authors change nothing.Summary by CodeRabbit
New Features
Bug Fixes