Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces HTTP/HTTPS server support to BAML: BAML Server and TlsConfig contracts, a native Hyper+Tokio server runtime with TLS, callable-handle VM dispatch for per-request handlers, io_impls integration and platform stubs, expanded workspace/sys_native dependencies, and comprehensive tests and snapshots. ChangesHTTP/HTTPS Server with Callable Handler Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
baml_language/crates/baml_tests/baml_src/ns_http_server/http_server.baml (1)
145-145: ⚡ Quick winPotential race condition with fixed sleep duration.
The hardcoded 100ms sleep assumes the cancelled serve releases the "serving" flag within that window. On slow or heavily loaded systems, this may be insufficient, causing the test to flake when
server.serveis called again before the flag clears.Consider either:
- Increasing the sleep to a more conservative value (e.g., 500ms-1s), or
- Implementing a retry loop that attempts
server.servewith small delays until it succeeds (to handle varying system performance).🤖 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 `@baml_language/crates/baml_tests/baml_src/ns_http_server/http_server.baml` at line 145, The test uses a fixed 100ms sleep (baml.sys.sleep(100)) to wait for the cancelled serve to clear the "serving" flag, which risks a race on slow systems; update the test around server.serve and the "serving" flag to either increase the wait (e.g., 500–1000ms) or, preferably, replace the fixed sleep with a small retry loop that repeatedly calls server.serve (with short sleeps between attempts) until it succeeds or a timeout is reached, so the test robustly waits for the "serving" flag to clear regardless of system load.baml_language/crates/bex_engine/src/lib.rs (1)
1768-1784: ⚡ Quick winUse the callable's real name in root span events.
Hard-coding
"<callable>"makes every callback invocation look identical in tracing. In the HTTP server path, that collapses all request-handler root spans even though the innerFunctionmetadata is already available here.Proposed change
- let (return_type, throws_type, arity, param_types) = match thread.vm.get_object(func_ptr) { + let (callable_label, return_type, throws_type, arity, param_types) = + match thread.vm.get_object(func_ptr) { Object::Function(func) => { // A value referencing an unresolved native builtin can't be an // entry point (parity with `call_function_bound_args`). if matches!(func.kind, bex_vm_types::FunctionKind::NativeUnresolved) { return Err(EngineError::NotInvokableAsEntry { @@ } ( + func.name.clone(), func.return_type.clone(), func.throws_type.clone(), func.arity, func.param_types.clone(), ) @@ - "<callable>".to_string(), + callable_label, args_snapshot, call_id, host_ctx, collectors, cancel,Also applies to: 1832-1840
🤖 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 `@baml_language/crates/bex_engine/src/lib.rs` around lines 1768 - 1784, The root tracing span currently uses the hard-coded string "<callable>" which collapses all invocations; update the span creation to use the actual callable name available in the match arm for Object::Function by passing func.name (or a formatted version like format!("{}{}", func.name, /* optional qualifier */)) instead of "<callable>" so each Function invocation creates a unique root span; apply the same replacement in the other analogous location referenced around the Object::Function handling (the second block near 1832-1840) so both call sites use func.name rather than the literal "<callable>".baml_language/crates/baml_builtins2_codegen/src/codegen_io.rs (1)
517-590: ⚡ Quick winAdd a small unit-test matrix for the callable-param codegen, and run the Rust lib tests locally.
These branches now need
is_callable_param,clean_param_type, andglue_extract_exprto stay in lockstep forfunctionandfunction?. A focused unit test here would guard that contract more directly than only relying on end-to-end coverage. Please also runcargo test --liblocally for this Rust change.As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible" and "Always run
cargo test --libif you changed any Rust code".Also applies to: 1230-1232, 1506-1509, 2122-2127
🤖 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 `@baml_language/crates/baml_builtins2_codegen/src/codegen_io.rs` around lines 517 - 590, Add a small unit-test matrix that exercises callable-param codegen cases (plain `function` and optional `function?`) to assert the outputs of is_callable_param, clean_param_type, and glue_extract_expr remain consistent: call is_callable_param(...) for Named("function") and Optional(Named("function")), verify clean_param_type returns bex_external_types::Handle or Option<bex_external_types::Handle> accordingly, and verify glue_extract_expr produces the callable extraction branch (e.g., uses as_callable_handle / as_optional_callable_handle) for those same types; add tests near the existing codegen tests for this module and then run `cargo test --lib` locally to ensure the library tests pass.Source: Coding guidelines
🤖 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 `@baml_language/crates/bex_heap/src/accessor.rs`:
- Around line 266-295: The as_callable_handle and as_optional_callable_handle
accessors currently accept any BexExternalValue::Handle without resolving or
validating it; update both to resolve the handle against the heap (using BexHeap
lookup/resolution APIs) while using the provided PermitProof, verify the
resolved object is the callable kind (the same kind BexEngine::call_callable
expects), and return an appropriate AccessError (e.g., TypeMismatch or
InvalidHandle) when resolution fails or the object is not callable; apply the
same null-handling in as_optional_callable_handle and ensure you clone/return
the validated handle only after successful resolution/validation.
In `@baml_language/crates/sys_native/src/http_server.rs`:
- Around line 449-462: The header-folding logic that builds the
IndexMap<String,String> from parts.headers must special-case Cookie headers:
instead of always joining repeated values with ", " do a case-insensitive check
for the header name (e.g. name.eq_ignore_ascii_case("cookie")) and use "; " as
the separator for Cookie entries; leave the existing ", " behavior for all other
headers. Update the loop where
headers.entry(...).and_modify(...).or_insert_with(...) is used so the and_modify
closure (or an outer conditional) chooses the separator based on the header
name.
- Around line 319-371: The accept loop currently spawns unbounded per-connection
tasks via conns.spawn(...) and performs TLS handshake (acceptor.accept),
serve_connection, and handle_request with no timeouts or fan-out limits; add a
bounded concurrency gate (e.g., a tokio::sync::Semaphore or a
TokioOwnedSemaphorePermit) acquired just before spawning to cap concurrent
connections, and enforce per-connection timeouts by wrapping the TLS handshake
(acceptor.accept(...)), the call to serve_connection(...), and the handler
invocation (handle_request(...)) in tokio::time::timeout(...) with sensible
durations; ensure the semaphore permit is dropped when the connection task ends
(or when a timeout triggers) so capacity is reclaimed, and log/close the socket
on timeout/failure instead of leaving tasks/sockets open.
In `@baml_language/crates/sys_ops/src/lib.rs`:
- Around line 1022-1080: with_http_instance currently only wires
fetch/send/response read/SSE; you need to also override the HTTP constructors
and server ops so injected platform implementations aren't bypassed. In
IoSysOpsBuilder::with_http_instance, add assignments that replace DefaultIoOps
implementations for io::IoClassHttpResponse::new,
io::IoClassHttpTlsConfig::_new, and io::IoClassHttpServer::bind and _serve with
the corresponding methods from the provided http instance (so those calls route
to the injected platform implementation rather than returning
VmBamlError::Unsupported). Update the builder wiring to reference those exact
symbols (with_http_instance, IoSysOpsBuilder, io::IoClassHttpResponse::new,
io::IoClassHttpTlsConfig::_new, io::IoClassHttpServer::bind,
io::IoClassHttpServer::_serve) so platform-specific behavior is used.
---
Nitpick comments:
In `@baml_language/crates/baml_builtins2_codegen/src/codegen_io.rs`:
- Around line 517-590: Add a small unit-test matrix that exercises
callable-param codegen cases (plain `function` and optional `function?`) to
assert the outputs of is_callable_param, clean_param_type, and glue_extract_expr
remain consistent: call is_callable_param(...) for Named("function") and
Optional(Named("function")), verify clean_param_type returns
bex_external_types::Handle or Option<bex_external_types::Handle> accordingly,
and verify glue_extract_expr produces the callable extraction branch (e.g., uses
as_callable_handle / as_optional_callable_handle) for those same types; add
tests near the existing codegen tests for this module and then run `cargo test
--lib` locally to ensure the library tests pass.
In `@baml_language/crates/baml_tests/baml_src/ns_http_server/http_server.baml`:
- Line 145: The test uses a fixed 100ms sleep (baml.sys.sleep(100)) to wait for
the cancelled serve to clear the "serving" flag, which risks a race on slow
systems; update the test around server.serve and the "serving" flag to either
increase the wait (e.g., 500–1000ms) or, preferably, replace the fixed sleep
with a small retry loop that repeatedly calls server.serve (with short sleeps
between attempts) until it succeeds or a timeout is reached, so the test
robustly waits for the "serving" flag to clear regardless of system load.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1768-1784: The root tracing span currently uses the hard-coded
string "<callable>" which collapses all invocations; update the span creation to
use the actual callable name available in the match arm for Object::Function by
passing func.name (or a formatted version like format!("{}{}", func.name, /*
optional qualifier */)) instead of "<callable>" so each Function invocation
creates a unique root span; apply the same replacement in the other analogous
location referenced around the Object::Function handling (the second block near
1832-1840) so both call sites use func.name rather than the literal
"<callable>".
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 11da329d-c884-4086-9e2c-021622afceed
⛔ Files ignored due to path filters (10)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/http_server.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
baml_language/Cargo.tomlbaml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_http/server.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_builtins2_codegen/src/codegen_io.rsbaml_language/crates/baml_lsp_server/src/playground_http.rsbaml_language/crates/baml_tests/baml_src/ns_http_server/http_server.bamlbaml_language/crates/baml_tests/tests/http.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_heap/src/accessor.rsbaml_language/crates/bridge_wasm/src/wasm_http.rsbaml_language/crates/sys_native/Cargo.tomlbaml_language/crates/sys_native/src/http_server.rsbaml_language/crates/sys_native/src/io_impls.rsbaml_language/crates/sys_native/src/lib.rsbaml_language/crates/sys_native/src/registry.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/crates/sys_types/src/lib.rs
- `baml.http.Server` defines a listener and serves on a given port. - Passing an optional `baml.http.TlsConfig` will make it an HTTPS server.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@baml_language/crates/sys_native/src/http_server.rs`:
- Around line 114-125: The current take_client uses try_lock() which reports
"already consumed" on lock contention; change fn take_client to be async (async
fn take_client(...)) and use slot.lock().await to acquire the mutex before
calling guard.take(), returning the same VmBamlError::Io when None; update the
callers (e.g., read_bytes and read_text) to .await the new async take_client and
propagate the Result/await usage accordingly so contention yields a true check
of guard.take() rather than a misleading try_lock() failure.
- Around line 470-474: The HTTP/2-only branch (allow_http2) currently calls
hyper::server::conn::http2::Builder::new(TokioExecutor::new()).serve_connection(...)
without applying any read/keep-alive timeouts, so mirror the timeout behavior
used for HTTP/1 by configuring the http2 builder before serve_connection: obtain
the same header_read_timeout (or a configured Duration) and call the HTTP/2
keep-alive methods (e.g., keep_alive_interval and keep_alive_timeout or any
available keep-alive/read-timeout APIs on hyper::server::conn::http2::Builder)
on the Builder returned by hyper::server::conn::http2::Builder::new(...) and
then call serve_connection(io, service). Ensure you convert the timeout to the
appropriate Duration type (tokio::time::Duration) and use the same configured
value used elsewhere so connection liveness is enforced for HTTP/2 as well.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4463764e-0a7f-4d3b-b008-f1f0f77a8a99
⛔ Files ignored due to path filters (11)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/http_server.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (19)
baml_language/Cargo.tomlbaml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_http/server.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_builtins2_codegen/src/codegen_io.rsbaml_language/crates/baml_lsp_server/Cargo.tomlbaml_language/crates/baml_lsp_server/src/playground_http.rsbaml_language/crates/baml_tests/baml_src/ns_http_server/http_server.bamlbaml_language/crates/baml_tests/tests/http.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_heap/src/accessor.rsbaml_language/crates/bridge_wasm/src/wasm_http.rsbaml_language/crates/sys_native/Cargo.tomlbaml_language/crates/sys_native/src/http_server.rsbaml_language/crates/sys_native/src/io_impls.rsbaml_language/crates/sys_native/src/lib.rsbaml_language/crates/sys_native/src/registry.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/crates/sys_types/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- baml_language/crates/sys_native/Cargo.toml
- baml_language/crates/baml_tests/tests/http.rs
- baml_language/crates/bex_heap/src/accessor.rs
- baml_language/Cargo.toml
- baml_language/crates/sys_native/src/registry.rs
- baml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.baml
- baml_language/crates/baml_lsp_server/src/playground_http.rs
- baml_language/crates/sys_types/src/lib.rs
- baml_language/crates/baml_builtins2_codegen/src/codegen_io.rs
- baml_language/crates/bex_engine/src/lib.rs
- baml_language/crates/sys_native/src/io_impls.rs
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.
baml.http.Serverdefines a listener and serves on a given port.baml.http.TlsConfigwill make it an HTTPS server.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores