fix: use stream idle deadline after reply handle arrives - #23
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Comment |
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 3 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Changes requested Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Before merge
How this fits togetherflowchart LR
n0["Connection<br/>changed<br/>3 findings"]:::blocking
n1["StreamLimits<br/>changed"]:::changed
n2["StreamRegistry<br/>changed"]:::changed
n3["StreamWriter<br/>changed"]:::changed
n4["new"]:::impacted
n5["Err"]:::impacted
n6["send_streamed_reply"]:::impacted
n7["open"]:::impacted
n8["write"]:::impacted
n9["pair"]:::impacted
n2 -->|uses| n1
n3 -->|uses| n0
n6 -->|uses| n0
n6 -->|calls| n4
n7 -->|calls| n5
n8 -->|calls| n5
n9 -->|uses| n0
n9 -->|calls| n4
n9 -->|tests| n4
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0041 · 129,415 in / 17,144 out · 6,024 cached (5%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 357 embedded
critique: $0.0016 · 48,273 in / 4,795 out · 2,110 cached (4%) · gpt-5.6-luna, deepseek-v4-flash
security: $0.0017 · 58,337 in / 3,002 out · 1,866 cached (3%) · gpt-5.6-luna
tests: $0.0004 · 13,908 in / 4,050 out · 1,024 cached (7%) · deepseek-v4-flash
description: $0.0002 · 5,165 in / 3,197 out · 1,024 cached (20%) · deepseek-v4-flash
| member: MemberName::new("StreamReply").expect("literal member name is valid"), | ||
| timeout_ms: timeout.as_millis() as u64, | ||
| })??; | ||
| let bytes = reader.read_to_end_capped(limit).await?; |
There was a problem hiding this comment.
Preserve the overall deadline for streamed replies
This removes the caller-supplied call deadline from streamed-reply consumption. read_to_end_capped can continue returning chunks indefinitely, and the per-read idle timeout resets after every chunk, so a peer that sends data periodically can keep the call and its resources alive without bound. Retain the overall call deadline while using the idle timeout only to detect stalled progress.
Additional critique observation
Retain an absolute deadline for streamed replies
[RULE] missing-deadline
This awaits the entire streamed response using only StreamReader's per-read idle timeout. A peer can send a small chunk just before each idle timeout and keep the call alive indefinitely (for example, one byte every 19ms with a 20ms idle timeout), even though the original call deadline has elapsed. The streamed body is part of the call and must retain an absolute deadline; apply the original deadline to the full read, or carry a separate absolute deadline alongside the progress timeout.
[RULE] missing-deadline ·
| .unwrap(); | ||
| let stream = writer.stream_ref(); | ||
| let send = tokio::spawn(async move { | ||
| tokio::time::sleep(Duration::from_millis(30)).await; |
There was a problem hiding this comment.
Use deterministic synchronization instead of sleeping
This test uses a fixed sleep to coordinate the delayed writer, which violates the repository rule against sleep-based synchronization and can become flaky under scheduler or CI load. Coordinate the writer with an explicit in-memory signal or another awaitable synchronization primitive, and use a timeout only as the test deadline.
[RULE] no-sleep-synchronization ·
tinysweeper
Bot
added
the
priority: p1
1 participant
Summary
Scope
call_streaming_with_timeoutto receipt of the reply handle, as its contract states. The streamed body now uses a per-chunk idle deadline fromStreamLimits, so a progressing transfer may exceed the handle timeout while a stalled sender still fails.Validation
cargo fmt --all -- --checkcargo test -p tinybus a_streamed_reply -- --nocapture(3 passed)cargo test --workspace --all-features --quiet(all passed)Context
CodeRabbit raised the same source-level issue on the OpenHuman module refresh PRs for TinyJuice, TinyMCP, TinyRuntime, and TinyWallet. Those repositories will advance their TinyBus gitlinks after this lands.
Summary by CodeRabbit