Skip to content

Commit 26386bd

Browse files
committed
Fix header callback
1 parent a188acb commit 26386bd

19 files changed

Lines changed: 916 additions & 332 deletions

File tree

AGENTS.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,9 @@ vespera/
8181
| `vespera_macro/src/openapi_generator.rs` | ~808 | OpenAPI doc assembly |
8282
| `vespera_macro/src/collector.rs` | ~707 | Filesystem route scanning |
8383
| `vespera_inprocess/src/lib.rs` | ~1184 | In-process dispatch + app factory + streaming + binary wire |
84-
| `vespera_jni/src/jni_impl.rs` | ~833 | JNI RUNTIME + jni_app! macro + 7 JNI symbols (incl. direct-buffer path) |
85-
| `vespera_jni/src/streaming_closures.rs` | ~406 | Streaming closure factories (`make_pull_closure`, `make_push_closure`, `call_header_consumer`, `complete_future`) + `OnceLock<MethodCache>` caching `JMethodID`+`GlobalRef<JClass>` for `InputStream.read`, `OutputStream.write`, `Consumer.accept`, `CompletableFuture.complete``call_method_unchecked` on the hot path |
84+
| `vespera_jni/src/jni_impl.rs` | ~880 | JNI RUNTIME + jni_app! macro + 7 JNI symbols (incl. direct-buffer path) |
85+
| `vespera_jni/src/streaming_closures.rs` | ~410 | Streaming closure factories (`make_pull_closure`, `make_push_closure`, `call_header_consumer`, `complete_future`) + `OnceLock<MethodCache>` caching `JMethodID`+`GlobalRef<JClass>` for `InputStream.read`, `OutputStream.write`, `Consumer.accept`, `CompletableFuture.complete``call_method_unchecked` on the hot path. Pull/push/header closures attach via [`daemon_env::with_cached_daemon_env`] (TLS-cached daemon attach), not `attach_current_thread` per chunk |
86+
| `vespera_jni/src/daemon_env.rs` | ~130 | `with_cached_daemon_env(jvm, cb)` — attaches the current OS thread once as a daemon (`AttachCurrentThreadAsDaemon`), caches the `JNIEnv` in a `thread_local!` `Cell`, and reuses it for every JNI callback on that thread (streaming chunk pull/push, header callbacks, async `CompletableFuture.complete`). Replaces the prior per-chunk attach/detach churn; per-call local frame + exception scrub preserved |
8687

8788
## CRATE DEPENDENCY GRAPH
8889

crates/vespera/src/multipart.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,10 @@ impl<S: Send + Sync> TryFromFieldWithState<S> for tempfile::NamedTempFile {
509509

510510
let mut total = 0usize;
511511
while let Some(chunk) = field.chunk().await? {
512-
total += chunk.len();
512+
// `saturating_add` (matching `read_field_data`) prevents a
513+
// pathological chunk size from wrapping `total` and slipping
514+
// past the limit check below.
515+
total = total.saturating_add(chunk.len());
513516
if let Some(limit) = limit_bytes
514517
&& total > limit
515518
{

crates/vespera_core/src/schema.rs

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,21 @@ where
5959
{
6060
match value {
6161
Some(v) if v.fract() == 0.0 => {
62-
// Practical OpenAPI constraints are well within i64 range
62+
// Float→int casts saturate in Rust, so an out-of-range
63+
// constraint (e.g. `1e20`) would silently become `i64::MAX`
64+
// and corrupt the generated spec. Emit the integer form
65+
// only when it round-trips exactly back to the original
66+
// value; otherwise keep the `f64` rendering.
6367
#[allow(clippy::cast_possible_truncation)]
6468
let int_val = *v as i64;
65-
serializer.serialize_some(&int_val)
69+
// Exact round-trip check is intentional: we emit the integer
70+
// form only when `i64 → f64` reproduces the original bits.
71+
#[allow(clippy::cast_precision_loss, clippy::float_cmp)]
72+
if int_val as f64 == *v {
73+
serializer.serialize_some(&int_val)
74+
} else {
75+
serializer.serialize_some(v)
76+
}
6677
}
6778
Some(v) => serializer.serialize_some(v),
6879
None => serializer.serialize_none(),
@@ -503,6 +514,29 @@ mod tests {
503514
);
504515
}
505516

517+
#[test]
518+
fn serialize_out_of_i64_range_constraint_stays_float() {
519+
// A whole-number constraint beyond i64 range must NOT saturate to
520+
// i64::MAX — it stays a float so the spec keeps the real value.
521+
let schema = Schema {
522+
maximum: Some(1e20),
523+
..Schema::number()
524+
};
525+
let json = serde_json::to_string(&schema).unwrap();
526+
assert!(
527+
!json.contains(&i64::MAX.to_string()),
528+
"must not saturate to i64::MAX: {json}"
529+
);
530+
// Parse back: the constraint value must be preserved exactly,
531+
// regardless of serde's float formatting.
532+
let parsed: serde_json::Value = serde_json::from_str(&json).unwrap();
533+
assert_eq!(
534+
parsed["maximum"].as_f64(),
535+
Some(1e20),
536+
"constraint value must be preserved: {json}"
537+
);
538+
}
539+
506540
#[test]
507541
fn serialize_multiple_of_whole_number_as_integer() {
508542
let schema = Schema {

crates/vespera_inprocess/benches/dispatch.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ use criterion::{BenchmarkId, Criterion, Throughput, criterion_group, criterion_m
3434
use serde::{Deserialize, Serialize};
3535
use tokio::runtime::Runtime;
3636
use vespera_inprocess::{
37-
RequestEnvelope, dispatch_bidirectional_streaming, dispatch_from_bytes, dispatch_owned,
38-
dispatch_streaming_async, dispatch_typed, register_app,
37+
RequestChunk, RequestEnvelope, dispatch_bidirectional_streaming, dispatch_from_bytes,
38+
dispatch_owned, dispatch_streaming_async, dispatch_typed, register_app,
3939
};
4040

4141
// ── Test fixtures ────────────────────────────────────────────────────
@@ -426,7 +426,13 @@ fn bench_streaming_path(c: &mut Criterion) {
426426
|b, _| {
427427
b.iter(|| {
428428
let chunks_iter = Mutex::new(request_chunks.clone().into_iter());
429-
let pull = move || -> Option<Vec<u8>> { chunks_iter.lock().unwrap().next() };
429+
let pull = move || -> RequestChunk {
430+
chunks_iter
431+
.lock()
432+
.unwrap()
433+
.next()
434+
.map_or(RequestChunk::End, RequestChunk::Data)
435+
};
430436
let mut sink = 0usize;
431437
runtime.block_on(dispatch_bidirectional_streaming(
432438
header_only.clone(),
@@ -448,14 +454,14 @@ fn bench_streaming_path(c: &mut Criterion) {
448454
|b, _| {
449455
b.iter(|| {
450456
let remaining = Mutex::new(body_kb * 1024);
451-
let pull = move || -> Option<Vec<u8>> {
457+
let pull = move || -> RequestChunk {
452458
let mut remaining = remaining.lock().unwrap();
453459
if *remaining == 0 {
454-
return None;
460+
return RequestChunk::End;
455461
}
456462
let len = (*remaining).min(pull_chunk_size);
457463
*remaining -= len;
458-
Some(vec![0xA5u8; len])
464+
RequestChunk::Data(vec![0xA5u8; len])
459465
};
460466
let mut sink = 0usize;
461467
runtime.block_on(dispatch_bidirectional_streaming(

crates/vespera_inprocess/src/internal.rs

Lines changed: 109 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,14 @@ pub async fn dispatch_parts<'h>(
5656
builder = builder.header("content-type", "application/json");
5757
}
5858

59-
let request = builder
60-
.body(Body::from(body_bytes))
61-
.expect("request construction should not fail with valid URI");
59+
// A malformed wire `path` (e.g. a raw space → not a valid
60+
// `http::Uri`) or an invalid header name/value surfaces here as a
61+
// builder error; convert it to a 400 so the contract "every failure
62+
// returns a wire response" holds instead of panicking.
63+
let request = match builder.body(Body::from(body_bytes)) {
64+
Ok(req) => req,
65+
Err(e) => return Err((400, format!("invalid request: {e}"))),
66+
};
6267

6368
let response = router
6469
.oneshot(request)
@@ -122,9 +127,14 @@ where
122127
builder = builder.header("content-type", "application/json");
123128
}
124129

125-
let request = builder
126-
.body(Body::from(body_bytes))
127-
.expect("request construction should not fail with valid URI");
130+
// A malformed wire `path` (e.g. a raw space → not a valid
131+
// `http::Uri`) or an invalid header name/value surfaces here as a
132+
// builder error; convert it to a 400 so the contract "every failure
133+
// returns a wire response" holds instead of panicking.
134+
let request = match builder.body(Body::from(body_bytes)) {
135+
Ok(req) => req,
136+
Err(e) => return Err((400, format!("invalid request: {e}"))),
137+
};
128138

129139
let response = router
130140
.oneshot(request)
@@ -205,7 +215,12 @@ async fn collect_response_parts(response: axum::response::Response) -> ResponseP
205215
/// [`http::HeaderMap`].
206216
pub fn to_response_envelope_text(parts: ResponseParts) -> ResponseEnvelope {
207217
let (status, headers, body_bytes, metadata) = parts;
208-
let body = String::from_utf8(body_bytes.to_vec()).unwrap_or_default();
218+
// `Vec::from(Bytes)` reuses the underlying buffer when the `Bytes`
219+
// is uniquely owned (the common case for a collected response body),
220+
// copying only for a shared/static slice — unlike `to_vec()`, which
221+
// always allocates and copies. Semantics preserved: a non-UTF-8
222+
// body still yields the empty string.
223+
let body = String::from_utf8(Vec::from(body_bytes)).unwrap_or_default();
209224
ResponseEnvelope {
210225
status,
211226
headers: collect_header_map(&headers),
@@ -250,9 +265,12 @@ pub async fn dispatch_and_split<'h>(
250265
builder = builder.header("content-type", "application/json");
251266
}
252267

253-
let request = builder
254-
.body(body)
255-
.expect("request construction should not fail with valid URI");
268+
// Same contract as dispatch_parts: a malformed path/header must
269+
// surface as a 400 wire response, not a panic.
270+
let request = match builder.body(body) {
271+
Ok(req) => req,
272+
Err(e) => return Err((400, format!("invalid request: {e}"))),
273+
};
256274

257275
let response = router
258276
.oneshot(request)
@@ -267,3 +285,84 @@ pub async fn dispatch_and_split<'h>(
267285
body,
268286
))
269287
}
288+
289+
#[cfg(test)]
290+
mod tests {
291+
use super::*;
292+
293+
fn block_on<F: std::future::Future>(fut: F) -> F::Output {
294+
tokio::runtime::Builder::new_current_thread()
295+
.enable_all()
296+
.build()
297+
.expect("build current-thread runtime")
298+
.block_on(fut)
299+
}
300+
301+
/// A wire `path` that cannot be parsed into an [`http::Uri`] (a raw
302+
/// space is illegal) must surface as an `Err((4xx, _))` the caller
303+
/// turns into a wire response — never a panic. Guards the
304+
/// "all failure modes return a valid wire response" contract for
305+
/// every `request_builder` call site.
306+
#[test]
307+
fn malformed_path_returns_error_not_panic() {
308+
let result = block_on(async {
309+
dispatch_parts(
310+
crate::Router::new(),
311+
"GET",
312+
"bad path with spaces",
313+
"",
314+
std::iter::empty(),
315+
Bytes::new(),
316+
)
317+
.await
318+
});
319+
match result {
320+
Err((status, _)) => assert!(
321+
(400..500).contains(&status),
322+
"expected 4xx for malformed path, got {status}"
323+
),
324+
Ok(_) => panic!("malformed path should not produce a successful dispatch"),
325+
}
326+
}
327+
328+
#[test]
329+
fn malformed_path_streaming_returns_error_not_panic() {
330+
let result = block_on(async {
331+
let mut sink = |_: &[u8]| {};
332+
dispatch_response_streaming(
333+
crate::Router::new(),
334+
"GET",
335+
"bad path with spaces",
336+
"",
337+
std::iter::empty(),
338+
Bytes::new(),
339+
&mut sink,
340+
)
341+
.await
342+
});
343+
assert!(
344+
result.is_err(),
345+
"streaming dispatch must reject malformed path"
346+
);
347+
}
348+
349+
#[test]
350+
fn malformed_path_split_returns_error_not_panic() {
351+
let result = block_on(async {
352+
dispatch_and_split(
353+
crate::Router::new(),
354+
"GET",
355+
"bad path with spaces",
356+
"",
357+
std::iter::empty(),
358+
Body::empty(),
359+
false,
360+
)
361+
.await
362+
});
363+
assert!(
364+
result.is_err(),
365+
"dispatch_and_split must reject malformed path"
366+
);
367+
}
368+
}

crates/vespera_inprocess/src/lib.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ pub use envelope::{
8080
};
8181
pub use registry::{DEFAULT_APP_NAME, register_app, register_app_named};
8282
pub use streaming::{
83-
dispatch_bidirectional_streaming, dispatch_bidirectional_streaming_with_header,
84-
dispatch_streaming_async, dispatch_streaming_with_header_async,
83+
RequestChunk, StreamAbort, dispatch_bidirectional_streaming,
84+
dispatch_bidirectional_streaming_with_header, dispatch_streaming_async,
85+
dispatch_streaming_with_header_async,
8586
};
8687
pub use wire::error_wire;

0 commit comments

Comments
 (0)