refactor: 重构安全防护，新增 SSE 支持#2
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe worker adds configuration-driven HTTP limits, connection rejection, keep-alive handling, WebSocket heartbeat processing, and Server-Sent Events support. SSE is exposed through PHP functions, integrated into HTTP callbacks, and covered by Rust and PHPUnit tests. ChangesWorker networking features
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handle_connection
participant PHPCallback
participant sse
Client->>handle_connection: request SSE route
handle_connection->>PHPCallback: invoke callback with stream fd
PHPCallback->>sse: start()
sse-->>Client: chunked SSE headers and retry
PHPCallback->>sse: send(event, data)
sse-->>Client: chunked SSE event
PHPCallback->>sse: end()
sse-->>Client: chunked stream terminator
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/command/Server.php (1)
237-256: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUpdate
stubs/lychee_worker.phpfor the expanded native API. Both startup paths now pass four limit arguments before the callbacks, while the shared stub still declares the old 3–14 argument contract.
src/command/Server.php#L237-L256: retain the 18-argument call and add the four parameters to the shared stub; this is the reported quality-check failure.src/Manager.php#L64-L83: ensure this call resolves against the same updated stub signature.🤖 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 `@src/command/Server.php` around lines 237 - 256, Update stubs/lychee_worker.php to declare the expanded native API with all four limit parameters before the callbacks, matching the 18-argument call in src/command/Server.php lines 237-256. Keep that Server.php call unchanged, and ensure the corresponding call in src/Manager.php lines 64-83 resolves against the same updated stub signature.Source: Linters/SAST tools
🧹 Nitpick comments (1)
rust/src/sse.rs (1)
130-142: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid unnecessary string allocations when formatting events.
The current implementation allocates new
Stringobjects for both the event name and each data line to filter out carriage returns and newlines. You can avoid these allocations by using the string slice'ssplititerator directly.⚡ Proposed refactor
- let clean: String = event - .chars() - .take_while(|c| *c != '\r' && *c != '\n') - .collect(); - buf.extend_from_slice(clean.as_bytes()); + let clean = event.split(|c| c == '\r' || c == '\n').next().unwrap_or(""); + buf.extend_from_slice(clean.as_bytes()); buf.extend_from_slice(b"\r\n"); } // data 换行 → 多个 data: 前缀 for line in data.split('\n') { buf.extend_from_slice(b"data: "); - let clean: String = line.chars().take_while(|c| *c != '\r').collect(); - buf.extend_from_slice(clean.as_bytes()); + let clean = line.split('\r').next().unwrap_or(""); + buf.extend_from_slice(clean.as_bytes()); buf.extend_from_slice(b"\r\n"); }🤖 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 `@rust/src/sse.rs` around lines 130 - 142, Update the event and data formatting logic in the SSE serializer to avoid allocating temporary Strings. Replace the clean collection in the event handling and each data-line handling with direct string-slice splitting/iteration that excludes carriage returns and newlines while preserving the existing output format and CRLF terminators.
🤖 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 `@rust/src/http.rs`:
- Around line 227-243: Update the request framing validation around header
parsing and read_body to strictly reject malformed or conflicting Content-Length
values, any Content-Length combined with Transfer-Encoding, and unsupported or
ignored transfer framing. Make read_body distinguish premature EOF/malformed
framing from body_max violations, returning appropriate 400 versus 413
responses. Preserve over-read bytes in the connection buffer for the next
request, and do not reuse the connection after ambiguous framing.
- Around line 177-180: Update the request handling flow around read_http_headers
and call_on_http to establish one deadline from request_timeout at request start
and pass only the remaining duration to every I/O phase. Run the synchronous PHP
callback behind an enforceable watchdog boundary so a hung call_on_http cannot
block the current-thread worker, and return the configured timeout response when
the deadline is exceeded.
- Around line 249-254: Replace the synchronous set_stream_fd(stream.as_raw_fd())
integration in the SSE setup with an async SSE writer/channel that does not
alter the socket’s nonblocking state. Route SSE data through Tokio’s async write
path and remove the shared-fd approach so slow writes cannot block the worker.
In `@rust/src/sse.rs`:
- Around line 91-94: Update the SSE write sequence around the header, data, and
trailing CRLF writes to use Result.and_then for short-circuiting. Ensure each
subsequent stream.write_all call executes only when the previous write succeeds,
while preserving the final boolean success result.
- Around line 65-75: Update clear_stream to restore the duplicated stream’s
non-blocking mode before it is dropped, ensuring the original Tokio TcpStream’s
O_NONBLOCK state is reinstated after PHP callback use. Apply this within the
ACTIVE_STREAM cleanup after flushing and before stream goes out of scope;
preserve the existing cleanup and SSE_STARTED reset behavior.
In `@rust/src/websocket.rs`:
- Around line 109-112: Move the outbound Message::Text, Pong, and Ping sends out
of the heartbeat select loop into a dedicated writer task fed by a channel.
Update the websocket loop so it enqueues messages without awaiting write.send,
while the writer task owns write and handles backpressure independently;
preserve the existing read polling and heartbeat timeout behavior.
- Around line 58-60: Validate the signed heartbeat inputs before converting them
to u64 in the websocket setup flow, using the existing
normalize_heartbeat_config path to reject or clamp negative values rather than
allowing wraparound. Ensure the resulting ping_interval and ping_timeout remain
safe for the last_msg_at + ping_timeout calculation, with checked_add as a
fallback if needed.
In `@tests/feature/SseTest.php`:
- Around line 43-47: Add the missing stream option to the Guzzle Client
configuration in the SseTest setup, setting it to true so SSE responses are read
incrementally without buffering the entire body. Preserve the existing base_uri,
http_errors, and timeout options.
---
Outside diff comments:
In `@src/command/Server.php`:
- Around line 237-256: Update stubs/lychee_worker.php to declare the expanded
native API with all four limit parameters before the callbacks, matching the
18-argument call in src/command/Server.php lines 237-256. Keep that Server.php
call unchanged, and ensure the corresponding call in src/Manager.php lines 64-83
resolves against the same updated stub signature.
---
Nitpick comments:
In `@rust/src/sse.rs`:
- Around line 130-142: Update the event and data formatting logic in the SSE
serializer to avoid allocating temporary Strings. Replace the clean collection
in the event handling and each data-line handling with direct string-slice
splitting/iteration that excludes carriage returns and newlines while preserving
the existing output format and CRLF terminators.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13320f48-ede8-4f7c-b3b2-6442ad64a379
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.gitignoreCargo.tomlcomposer.jsonrust/src/http.rsrust/src/lib.rsrust/src/runtime.rsrust/src/sse.rsrust/src/websocket.rssrc/Manager.phpsrc/command/Server.phpsrc/config/worker.phptests/feature/SseTest.phptests/stub/route/app.php
| // 1) 读取 header（带超时保护） | ||
| let buf_full = | ||
| match tokio::time::timeout(request_timeout, read_http_headers(&mut stream, header_max)) | ||
| .await |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Enforce the configured deadline around the PHP callback.
Each I/O phase receives a fresh timeout, while synchronous call_on_http is unbounded. A hung callback therefore blocks the current-thread worker indefinitely despite request_timeout_sec. Use one request deadline across phases and isolate PHP execution behind a enforceable watchdog boundary.
Also applies to: 256-283
🤖 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 `@rust/src/http.rs` around lines 177 - 180, Update the request handling flow
around read_http_headers and call_on_http to establish one deadline from
request_timeout at request start and pass only the remaining duration to every
I/O phase. Run the synchronous PHP callback behind an enforceable watchdog
boundary so a hung call_on_http cannot block the current-thread worker, and
return the configured timeout response when the deadline is exceeded.
| // 5) 读取 body（带超时和大小限制） | ||
| let body = match tokio::time::timeout( | ||
| request_timeout, | ||
| read_body(header_bytes, &mut stream, &buf_full[header_end..], body_max), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Ok(b)) => b, | ||
| Ok(Err(_)) => { | ||
| let _ = write_simple_response(&mut stream, 413, "Payload Too Large", "").await; | ||
| return Ok(()); | ||
| } | ||
| Err(_) => { | ||
| let _ = write_simple_response(&mut stream, 408, "Request Timeout", "").await; | ||
| return Ok(()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject ambiguous body framing before reusing the connection.
Invalid or duplicate Content-Length values become zero, Transfer-Encoding is ignored, premature EOF succeeds, and over-read bytes are discarded. With keep-alive this can desynchronize request boundaries and enable request smuggling behind a proxy. Strictly validate framing, reject Content-Length plus Transfer-Encoding, distinguish malformed bodies from oversized ones, and preserve buffered bytes for the next request.
Also applies to: 379-400, 422-424
🤖 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 `@rust/src/http.rs` around lines 227 - 243, Update the request framing
validation around header parsing and read_body to strictly reject malformed or
conflicting Content-Length values, any Content-Length combined with
Transfer-Encoding, and unsupported or ignored transfer framing. Make read_body
distinguish premature EOF/malformed framing from body_max violations, returning
appropriate 400 versus 413 responses. Preserve over-read bytes in the connection
buffer for the next request, and do not reuse the connection after ambiguous
framing.
| // 6a) 准备 SSE 写入通道：dup socket fd 供 PHP 扩展同步写入 | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::io::AsRawFd; | ||
| unsafe { crate::sse::set_stream_fd(stream.as_raw_fd()) }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import fcntl
import os
import socket
original, peer = socket.socketpair()
original.setblocking(False)
duplicate = socket.socket(fileno=os.dup(original.fileno()))
duplicate.setblocking(True)
flags = fcntl.fcntl(original.fileno(), fcntl.F_GETFL)
assert not flags & os.O_NONBLOCK
print("Changing the duplicate also cleared O_NONBLOCK on the original.")
PYRepository: watsonhaw5566/lychee-worker
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## rust/src/http.rs (around lines 220-280)\n'
sed -n '220,280p' rust/src/http.rs | cat -n
printf '\n## Search for set_stream_fd and related SSE fd handling\n'
rg -n "set_stream_fd|sse::|set_nonblocking|setblocking|AsRawFd|dup" rust/src -SRepository: watsonhaw5566/lychee-worker
Length of output: 4025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Candidate SSE files\n'
fd -a "sse" rust/src || true
printf '\n## Read the SSE implementation\n'
for f in $(git ls-files 'rust/src/*sse*' 'rust/src/**/sse*' 2>/dev/null); do
echo "--- $f"
wc -l "$f"
done
printf '\n## Search the whole rust/src tree for stream-fd handling\n'
rg -n "set_stream_fd|stream_fd|dup\(|set_nonblocking|setblocking|O_NONBLOCK" rust/src -SRepository: watsonhaw5566/lychee-worker
Length of output: 787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' rust/src/sse.rs | cat -nRepository: watsonhaw5566/lychee-worker
Length of output: 4697
Keep the SSE fd nonblocking set_stream_fd() duplicates stream and then calls set_nonblocking(false), which clears O_NONBLOCK on the shared socket. That makes the Tokio connection blocking too, so a slow SSE write can stall the single-thread worker. Use an async SSE writer/channel instead.
🤖 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 `@rust/src/http.rs` around lines 249 - 254, Replace the synchronous
set_stream_fd(stream.as_raw_fd()) integration in the SSE setup with an async SSE
writer/channel that does not alter the socket’s nonblocking state. Route SSE
data through Tokio’s async write path and remove the shared-fd approach so slow
writes cannot block the worker.
| pub(crate) fn clear_stream() { | ||
| ACTIVE_STREAM.with(|cell| { | ||
| let mut borrow = cell.borrow_mut(); | ||
| if let Some(mut stream) = borrow.take() { | ||
| // 仅 flush 未写的字节（若有）；如果是 SSE 模式已有 end() 已写 | ||
| // 终止符，否则流在 drop 时释放 fd 自动关闭 | ||
| let _ = stream.flush(); | ||
| } | ||
| }); | ||
| SSE_STARTED.with(|cell| cell.set(false)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Restore the non-blocking flag to prevent deadlocking Tokio.
Because libc::dup creates a file descriptor that shares the same underlying open file description, calling stream.set_nonblocking(false) in set_stream_fd also clears the O_NONBLOCK flag for the original Tokio TcpStream.
Since this flag is never restored, when the PHP callback returns and Tokio regains control, it will attempt to use a blocking socket. If Tokio attempts an asynchronous read or write on it, it will block the entire Tokio worker thread. For a current_thread runtime, this leads to a complete deadlock.
Restore the non-blocking flag before the stream is dropped.
🔒️ Proposed fix
pub(crate) fn clear_stream() {
ACTIVE_STREAM.with(|cell| {
let mut borrow = cell.borrow_mut();
if let Some(mut stream) = borrow.take() {
// 仅 flush 未写的字节（若有）；如果是 SSE 模式已有 end() 已写
// 终止符，否则流在 drop 时释放 fd 自动关闭
let _ = stream.flush();
+ // 恢复非阻塞状态，以免影响 Tokio 后续对原 fd 的操作
+ let _ = stream.set_nonblocking(true);
}
});
SSE_STARTED.with(|cell| cell.set(false));
}📝 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.
| pub(crate) fn clear_stream() { | |
| ACTIVE_STREAM.with(|cell| { | |
| let mut borrow = cell.borrow_mut(); | |
| if let Some(mut stream) = borrow.take() { | |
| // 仅 flush 未写的字节（若有）；如果是 SSE 模式已有 end() 已写 | |
| // 终止符，否则流在 drop 时释放 fd 自动关闭 | |
| let _ = stream.flush(); | |
| } | |
| }); | |
| SSE_STARTED.with(|cell| cell.set(false)); | |
| } | |
| pub(crate) fn clear_stream() { | |
| ACTIVE_STREAM.with(|cell| { | |
| let mut borrow = cell.borrow_mut(); | |
| if let Some(mut stream) = borrow.take() { | |
| // 仅 flush 未写的字节（若有）；如果是 SSE 模式已有 end() 已写 | |
| // 终止符，否则流在 drop 时释放 fd 自动关闭 | |
| let _ = stream.flush(); | |
| // 恢复非阻塞状态，以免影响 Tokio 后续对原 fd 的操作 | |
| let _ = stream.set_nonblocking(true); | |
| } | |
| }); | |
| SSE_STARTED.with(|cell| cell.set(false)); | |
| } |
🤖 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 `@rust/src/sse.rs` around lines 65 - 75, Update clear_stream to restore the
duplicated stream’s non-blocking mode before it is dropped, ensuring the
original Tokio TcpStream’s O_NONBLOCK state is reinstated after PHP callback
use. Apply this within the ACTIVE_STREAM cleanup after flushing and before
stream goes out of scope; preserve the existing cleanup and SSE_STARTED reset
behavior.
| let r1 = stream.write_all(header.as_bytes()); | ||
| let r2 = stream.write_all(data); | ||
| let r3 = stream.write_all(b"\r\n"); | ||
| r1.and(r2).and(r3).is_ok() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use short-circuiting to prevent writing to a broken stream.
In the current implementation, r1.and(r2).and(r3) evaluates stream.write_all(data) and stream.write_all(b"\r\n") eagerly before evaluating the result of r1. This means if the first write fails (e.g., due to a broken connection), the subsequent writes will still be executed on the same broken stream, leading to unnecessary system calls and potential unexpected behavior.
Use .and_then() to ensure true short-circuiting.
🐛 Proposed fix
- let r1 = stream.write_all(header.as_bytes());
- let r2 = stream.write_all(data);
- let r3 = stream.write_all(b"\r\n");
- r1.and(r2).and(r3).is_ok()
+ stream.write_all(header.as_bytes())
+ .and_then(|_| stream.write_all(data))
+ .and_then(|_| stream.write_all(b"\r\n"))
+ .is_ok()📝 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.
| let r1 = stream.write_all(header.as_bytes()); | |
| let r2 = stream.write_all(data); | |
| let r3 = stream.write_all(b"\r\n"); | |
| r1.and(r2).and(r3).is_ok() | |
| stream.write_all(header.as_bytes()) | |
| .and_then(|_| stream.write_all(data)) | |
| .and_then(|_| stream.write_all(b"\r\n")) | |
| .is_ok() |
🤖 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 `@rust/src/sse.rs` around lines 91 - 94, Update the SSE write sequence around
the header, data, and trailing CRLF writes to use Result.and_then for
short-circuiting. Ensure each subsequent stream.write_all call executes only
when the previous write succeeds, while preserving the final boolean success
result.
| // 心跳参数规范化：最小值保护，避免误填 0 或负数导致死循环 | ||
| let (ping_interval, ping_timeout) = | ||
| normalize_heartbeat_config(ping_interval_sec, ping_timeout_sec); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== rust/src/lib.rs (relevant slices) ==\n'
nl -ba rust/src/lib.rs | sed -n '140,240p'
printf '\n== rust/src/websocket.rs (relevant slices) ==\n'
nl -ba rust/src/websocket.rs | sed -n '1,260p'Repository: watsonhaw5566/lychee-worker
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
("rust/src/lib.rs", 140, 240),
("rust/src/websocket.rs", 1, 260),
]:
print(f"\n== {path} ({start}-{end}) ==")
lines = Path(path).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: watsonhaw5566/lychee-worker
Length of output: 13777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in ["rust/src/lib.rs", "rust/Cargo.toml", "Cargo.toml"]:
p = Path(path)
if p.exists():
print(f"\n== {path} ==")
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if "ping_interval" in line or "ping_timeout" in line or "ext-php-rs" in line or "php_function" in line:
start = max(1, i-8)
end = min(len(text), i+12)
for j in range(start, end+1):
print(f"{j:4d}: {text[j-1]}")
print()
PYRepository: watsonhaw5566/lychee-worker
Length of output: 15683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in ["rust/src/lib.rs", "rust/Cargo.toml", "Cargo.toml"]:
p = Path(path)
if not p.exists():
continue
print(f"\n== {path} ==")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "ping_interval" in line or "ping_timeout" in line or "ext-php-rs" in line or "php_function" in line:
start = max(1, i-8)
end = min(len(lines), i+12)
for j in range(start, end+1):
print(f"{j:4d}: {lines[j-1]}")
print()
PYRepository: watsonhaw5566/lychee-worker
Length of output: 15683
🌐 Web query:
Rust Instant add Duration overflow panic standard library
💡 Result:
In Rust, adding a Duration to an Instant using the + operator (via the Add trait) can panic if the resulting point in time exceeds the bounds of the underlying data structure [1][2][3]. The standard library implementation for this operation is: fn add(self, other: Duration) -> Instant { self.checked_add(other).expect("overflow when adding duration to instant") } Because this uses.expect, it will trigger a panic with the message "overflow when adding duration to instant" if the addition overflows [2][3][4]. To avoid this panic, you should use the Instant::checked_add method, which returns an Option instead of panicking [1][2][5]. - Instant::checked_add(duration) returns Some(t) if the addition is successful, or None if the result cannot be represented [1][2][5]. The specific range limits of an Instant are platform-dependent, as they rely on the underlying OS monotonic clock implementation [1][6]. Therefore, code that works on one platform may encounter overflows and panics on another when dealing with very large durations [6].
Citations:
- 1: https://doc.rust-lang.org/std/time/struct.Instant.html
- 2: https://doc.rust-lang.org/stable/src/std/time.rs.html
- 3: https://github.com/rust-lang/rust/blob/3dbade652ed8ebac70f903e01f51cd92c4e4302c/src/libstd/time.rs
- 4: Feature Request: saturating_add(Duration) for SystemTime / Instant rust-lang/rust#71224
- 5: https://dev-doc.rust-lang.org/std/time/struct.Instant.html
- 6: Improve
std::time::Instantdocumentation - OS behavior rust-lang/rust#79881
🌐 Web query:
Rust std::time::Instant + Duration overflow panic
💡 Result:
In Rust, the std::time::Instant implementation of the Add<Duration> trait will panic if the resulting point in time cannot be represented by the underlying data structure [1][2]. When you perform addition using the + operator (e.g., instant + duration), Rust calls the add method, which internally invokes checked_add and calls expect("overflow when adding duration to instant") if the operation fails [1][3]. This behavior is a deliberate design choice to enforce that Instant values remain valid within the limits of the platform-specific clock implementation [1][4]. Key points to consider: - Platform Variability: The internal representation of Instant is platform-specific (e.g., it may vary between macOS, Windows, and Linux) [5][4]. Consequently, a Duration that triggers an overflow and subsequent panic on one platform may be successfully added on another [5][4]. - Non-Panicking Alternative: To avoid panics, you should use the checked_add method instead of the + operator [1][2]. The checked_add method returns an Option<Instant>, which will be None if an overflow occurs, allowing you to handle the error gracefully in your code [6][7]. - Documentation Note: Official Rust documentation explicitly warns that mathematical operations like add may panic and suggests using checked_add as a safer alternative when working with durations that might result in values outside the representable range [1][7].
Citations:
- 1: https://doc.rust-lang.org/stable/src/std/time.rs.html
- 2: https://doc.rust-lang.org/std/time/struct.Duration.html
- 3: https://github.com/rust-lang/rust/blob/3dbade652ed8ebac70f903e01f51cd92c4e4302c/src/libstd/time.rs
- 4: Improve
std::time::Instantdocumentation - OS behavior rust-lang/rust#79881 - 5: std::time inconsistencies rust-lang/rust#48980
- 6: https://doc.rust-lang.org/1.62.0/std/time/struct.Instant.html
- 7: https://doc.rust-lang.org/nightly/std/time/struct.Instant.html
🌐 Web query:
Rust standard library Instant add overflow panic Duration
💡 Result:
In Rust, using the addition operator (+) to add a std::time::Duration to a std::time::Instant will panic if the resulting point in time cannot be represented by the underlying data structure [1][2][3]. This panic occurs because the standard library implementation of the Add trait for Instant internally calls checked_add(duration) and then executes.expect("overflow when adding duration to instant") [3][4][5]. If you need to perform this operation without risking a panic, you should use the Instant::checked_add method instead [2][6][7]. This method returns an Option, which will be None if an overflow occurs, allowing you to handle the scenario gracefully [6][8][7]. It is also worth noting that because Instant is backed by platform-specific system APIs, the exact point at which an overflow occurs can vary between different operating systems [9].
Citations:
- 1: https://doc.rust-lang.org/std/time/struct.Instant.html
- 2: https://doc.rust-lang.org/std/time/struct.Duration.html
- 3: https://github.com/rust-lang/rust/blob/d2f24127/library/std/src/time.rs
- 4: Feature Request: saturating_add(Duration) for SystemTime / Instant rust-lang/rust#71224
- 5: https://doc.rust-lang.org/stable/src/std/time.rs.html
- 6: https://doc.rust-lang.org/nightly/std/time/struct.Instant.html
- 7: https://doc.rust-lang.org/stable/std/time/struct.Instant.html
- 8: https://docs.rs/rustc-std-workspace-std/latest/std/time/struct.Instant.html
- 9: std::time inconsistencies rust-lang/rust#48980
Validate heartbeat values before the u64 cast.
ping_interval_sec and ping_timeout_sec are i64 in rust/src/lib.rs, but they’re cast to u64 before normalization. A negative PHP value becomes a huge duration, which can make last_msg_at + ping_timeout at rust/src/websocket.rs:168 panic on Instant overflow. Reject or clamp the signed inputs up front; checked_add is a useful fallback.
🤖 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 `@rust/src/websocket.rs` around lines 58 - 60, Validate the signed heartbeat
inputs before converting them to u64 in the websocket setup flow, using the
existing normalize_heartbeat_config path to reject or clamp negative values
rather than allowing wraparound. Ensure the resulting ping_interval and
ping_timeout remain safe for the last_msg_at + ping_timeout calculation, with
checked_add as a fallback if needed.
| if write | ||
| .send(Message::Text(String::from_utf8_lossy(&msg).into_owned())) | ||
| .await | ||
| .is_err() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the websocket loop in manageable slices.
file="rust/src/websocket.rs"
wc -l "$file"
sed -n '1,240p' "$file" | cat -n | sed -n '1,220p'Repository: watsonhaw5566/lychee-worker
Length of output: 9190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="rust/src/websocket.rs"
wc -l "$file"
sed -n '1,240p' "$file" | cat -n | sed -n '1,220p'Repository: watsonhaw5566/lychee-worker
Length of output: 9190
Keep writes out of the heartbeat loop.
When write.send(...) stalls on backpressure, this tokio::select! body stops returning and the loop no longer polls read.next() or the timeout branch. A slow/non-reading peer can then suppress heartbeat detection and keep the connection open. Move outbound writes to a dedicated task/channel; the same applies to the Pong and Ping sends below.
🤖 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 `@rust/src/websocket.rs` around lines 109 - 112, Move the outbound
Message::Text, Pong, and Ping sends out of the heartbeat select loop into a
dedicated writer task fed by a channel. Update the websocket loop so it enqueues
messages without awaiting write.send, while the writer task owns write and
handles backpressure independently; preserve the existing read polling and
heartbeat timeout behavior.
| $this->httpClient = new Client([ | ||
| 'base_uri' => 'http://127.0.0.1:8080', | ||
| 'http_errors' => false, | ||
| 'timeout' => 5, | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the missing stream option to the Guzzle client.
The test documentation specifies that stream: true is used to allow Guzzle to read the body as a stream and avoid waiting for the entire data (which is crucial for testing long-lived SSE connections). However, this option was not actually added to the client configuration. Without it, Guzzle will buffer the entire response body in memory before returning.
🐛 Proposed fix
$this->httpClient = new Client([
'base_uri' => 'http://127.0.0.1:8080',
'http_errors' => false,
'timeout' => 5,
+ 'stream' => true,
]);📝 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.
| $this->httpClient = new Client([ | |
| 'base_uri' => 'http://127.0.0.1:8080', | |
| 'http_errors' => false, | |
| 'timeout' => 5, | |
| ]); | |
| $this->httpClient = new Client([ | |
| 'base_uri' => 'http://127.0.0.1:8080', | |
| 'http_errors' => false, | |
| 'timeout' => 5, | |
| 'stream' => true, | |
| ]); |
🤖 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 `@tests/feature/SseTest.php` around lines 43 - 47, Add the missing stream
option to the Guzzle Client configuration in the SseTest setup, setting it to
true so SSE responses are read incrementally without buffering the entire body.
Preserve the existing base_uri, http_errors, and timeout options.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes