fix(security): address PR #139 review findings

[AlexanderWagnerDev]

Builds on #139 (cap ModEx chain depth + bound client connect I/O) and fixes the review findings raised on it.

Changes

• EINTR bypasses deadline: poll_until_deadline now recomputes the remaining time on every EINTR retry instead of reusing the timeout computed once up front, so a signal arriving near the deadline can no longer restart a full poll interval and exceed the intended wall-clock deadline for connect/handshake I/O.
• TLS ignores connect deadline: Transport::connect_tls now takes the caller's remaining connect budget and uses it for the TLS handshake's socket read/write timeouts, instead of a fresh fixed 10s timeout. Client::connect passes the remaining time on the shared deadline through, so RTMPS connects can no longer run past the overall connect cap.
• Connect timeout not configurable: added Client::set_connect_timeout(Duration) so embedders (e.g. librtmp2-server's E2E tests) can raise the connect budget for slower/loaded hosts now that handshake + AMF connect I/O share that same deadline. Defaults to the previous TCP_CONNECT_TIMEOUT_SECS when unset.
• ModEx cap alters codecs: documented on normalize_modex_payload that exceeding MAX_MODEX_CHAIN_LAYERS intentionally leaves the payload opaque, so downstream codec/multitrack detection (detected_video_codec / detected_audio_codec) can be absent for such frames — this is a deliberate CPU-amplification tradeoff, not a bug.

Test plan

• cargo build --all-features
• cargo build --no-default-features
• cargo test --all-features (268 unit tests + integration suites, all passing)
• cargo test --no-default-features
• cargo clippy --all-features --all-targets (clean)
• cargo fmt

---

Generated by Claude Code

---

^{Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.}

Summary by CodeRabbit

• New Features

  • Added an optional overall timeout for connection setup, covering network, TLS, handshake, and session initialization.
  • TLS connection setup now honors a caller-provided timeout.

• Bug Fixes

  • Prevented excessively deep media metadata chains from causing unbounded processing; deeply nested payloads remain safely opaque.
  • Added handling for zero-duration TLS timeouts.

[coderabbitai]

Warning

Review limit reached

@AlexanderWagnerDev, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 42dfb116-ea7b-4fab-b02b-08527dd0383f

📥 Commits

Reviewing files that changed from the base of the PR and between 37fc4f3 and 2864f77.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/client/mod.rs
• src/transport.rs

📝 Walkthrough

Walkthrough

The client gains an optional wall-clock connection deadline covering TCP/TLS setup, RTMP handshake, and AMF commands. TLS accepts explicit timeouts, ModEx normalization limits chained layers, and one multitrack test assertion is reformatted.

Changes

Connection deadline enforcement

Layer / File(s)	Summary
Timeout contracts and transport bounds src/client/mod.rs, src/transport.rs	Client exposes connect_timeout and its setter; Transport::connect_tls accepts a duration, rejects zero timeouts, and applies the duration to socket operations.
Deadline-aware I/O primitives src/client/mod.rs	Handshake, command exchange, receive loops, reads, sends, and readiness polling use optional absolute deadlines.
Deadline propagation through connect src/client/mod.rs	connect() propagates one deadline through transport setup, RTMP handshake, and AMF connect/createStream; publish() and play() pass no explicit deadline.

ModEx normalization depth guard

Layer / File(s)	Summary
Bound chained ModEx peeling src/media/modex.rs	ModEx peeling stops at MAX_MODEX_CHAIN_LAYERS, returns the original payload as borrowed, and tests the opaque result.

Multitrack test formatting

Layer / File(s)	Summary
Reformat multitrack assertion src/ertmp/multitrack_media.rs	The excessive-subtrack test reformats its callback without changing the assertion behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Transport
  participant RTMPHandshake
  participant AMFConnection
  Client->>Transport: connect TCP/TLS within deadline
  Client->>RTMPHandshake: perform C0/C1/C2 handshake
  Client->>AMFConnection: send connect and createStream
  AMFConnection-->>Client: return responses or timeout

Loading

Possibly related issues

• Client connect handshake lacks wall-clock deadline enabling indefinite thread hang #138 — Adds and propagates the wall-clock deadline through client connection setup.

Possibly related PRs

• OpenRTMP/librtmp2#99 — Also changes Client::connect to apply bounded connection timeouts.
• OpenRTMP/librtmp2#120 — Also applies a wall-clock deadline to connection setup and DNS resolution.
• OpenRTMP/librtmp2#87 — Also modifies client connection and TLS handshake timeout behavior.

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is related to the security-focused follow-up work, though it is high-level and does not describe the specific timeout and ModEx changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/librtmp2-findings-fix-0ss055

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Fix connect/handshake deadline enforcement and make connect timeout configurable

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Enforce a single wall-clock connect deadline across DNS, TCP/TLS, RTMP handshake, and AMF setup.
• Fix EINTR polling to not reset remaining time, preventing deadline overruns.
• Add configurable client connect timeout and document ModEx chain-cap behavior.

Diagram

graph TD
  A["Client::connect()"] --> B["Resolve DNS"] --> C["TCP connect"] --> D{"TLS?"}
  D -->|yes| E["Transport::connect_tls(remaining)"] --> F["RTMP handshake (bounded)"] --> G["AMF connect/createStream (bounded)"]
  D -->|no| F --> G
  H["normalize_modex_payload"] --> I["Cap ModEx layers"]
  A -."media frames".-> H

  subgraph Legend
    direction LR
    _fn["Function"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize deadline-aware I/O in Transport

• ➕ Single implementation of bounded send/recv/poll shared by handshake and command paths
• ➕ Reduces risk of future call sites forgetting to pass deadlines
• ➖ Larger refactor surface area; may require API changes across the crate

2. Use socket-level timeouts exclusively (set_*_timeout)

• ➕ Simpler control flow; fewer explicit poll loops
• ➖ Harder to enforce a single wall-clock budget across mixed operations
• ➖ Timeout behavior can be less precise across partial reads/writes and EINTR handling

3. Adopt an async runtime for connect/handshake

• ➕ Deadline propagation and cancellation become more uniform
• ➕ Potentially simpler to reason about nonblocking I/O
• ➖ Major dependency and API shift; not appropriate for a small sync library without async commitments

Recommendation: The PR’s approach is the best incremental fix: it preserves the sync API, enforces an absolute deadline across all connect phases (including TLS), and closes the EINTR time-budget loophole. Consider a follow-up to encapsulate bounded I/O primitives inside Transport to avoid duplicating deadline plumbing as more call sites become deadline-sensitive.

Files changed (4) +187 / -35

Bug fix (3) +184 / -30

mod.rsPropagate connect deadline through TLS, handshake, and AMF setup +131/-26

Propagate connect deadline through TLS, handshake, and AMF setup

• Adds an optional per-client connect timeout override and uses it to compute a single connect deadline. Passes remaining budget into TLS setup and threads an absolute deadline through RTMP handshake, AMF connect/createStream send/wait paths, adding bounded send/recv helpers and deadline-aware polling.

src/client/mod.rs

modex.rsCap ModEx unwrapping depth and document opaque-over-cap behavior +30/-0

Cap ModEx unwrapping depth and document opaque-over-cap behavior

• Introduces a MAX_MODEX_CHAIN_LAYERS limit to prevent CPU amplification from deeply nested ModEx wrappers. Documents that over-cap payloads remain wrapped (codec/multitrack detection may not trigger) and adds a unit test asserting the opaque behavior.

src/media/modex.rs

transport.rsBound TLS handshake timeouts by caller’s remaining connect budget +23/-4

Bound TLS handshake timeouts by caller’s remaining connect budget

• Extends Transport::connect_tls to accept a timeout duration representing remaining connect budget, applies it to socket read/write timeouts, and returns Timeout immediately when the budget is zero. Updates TLS client tests to pass an explicit timeout parameter.

src/transport.rs

Tests (1) +3 / -5

multitrack_media.rsTidy multitrack test formatting +3/-5

Tidy multitrack test formatting

• Reformats a foreach_track over-limit test to a more compact closure style without changing behavior.

src/ertmp/multitrack_media.rs

[coderabbitai]

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 `@src/client/mod.rs`:
- Around line 237-240: Update the deadline calculation in the connection flow to
use checked addition with the resolved connect timeout, including the default
from TCP_CONNECT_TIMEOUT_SECS. When the addition overflows, return the method’s
existing error type instead of panicking; preserve the normal deadline behavior
for valid durations.

In `@src/transport.rs`:
- Around line 168-192: Update connect_tls to enforce one absolute handshake
deadline rather than per-I/O read/write timeouts: track the caller-provided
deadline, drive Ssl::connect in a nonblocking/retry loop, and return
ErrorCode::Timeout when it expires while preserving other handshake errors. Add
a test covering a stalled or drip-fed TLS handshake that verifies it cannot
exceed the supplied timeout and reports Timeout.

🪄 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 Plus

Run ID: 35ca857b-dd75-4464-b625-fa60272c8adc

📥 Commits

Reviewing files that changed from the base of the PR and between 01b4f51 and 37fc4f3.

📒 Files selected for processing (4)

• src/client/mod.rs
• src/ertmp/multitrack_media.rs
• src/media/modex.rs
• src/transport.rs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 11 rules

✅ Cross-repo context

  Explored: repo: OpenRTMP/librtmp2-server (sha: a8bb8752)

1. Clamped poll times out early ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

Transport::connect_tls_with_timeout clamps poll()’s timeout to i32::MAX ms, but still returns
ErrorCode::Timeout when poll() returns 0; if the remaining deadline exceeds ~24.8 days, this can
time out before the absolute deadline is actually reached. Client’s new poll_until_deadline
uses the same clamping pattern, and Client::set_connect_timeout() now makes such large deadlines
possible.

Code

src/transport.rs[R277-295]

+            let Some(remaining) = deadline.checked_duration_since(Instant::now()) else {
+                return Err(ErrorCode::Timeout);
+            };
+            // `poll(2)`'s granularity is milliseconds; round a sub-ms
+            // remainder down to an expired deadline instead of up to a full
+            // 1ms wait, so the caller's absolute deadline can't be overshot.
+            if remaining.as_millis() == 0 {
+                return Err(ErrorCode::Timeout);
+            }
+            let timeout_ms = remaining.as_millis().min(i32::MAX as u128) as i32;
+            let mut pfd = libc::pollfd {
+                fd: pending.get_ref().as_raw_fd(),
+                events: tls_poll_interest(&pending).poll_events(),
+                revents: 0,
+            };
+            let rc = unsafe { libc::poll(&mut pfd, 1, timeout_ms) };
+            if rc == 0 {
+                return Err(ErrorCode::Timeout);
+            }

Relevance

⭐⭐⭐ High

Team repeatedly accepts deadline/timeout correctness hardening (single absolute deadline, poll
EINTR/timeout fixes) in prior PRs.

PR-#99
PR-#87
PR-#120

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The TLS handshake loop clamps the poll timeout and immediately treats rc==0 as Timeout, which is
only valid if the absolute deadline has passed; the same clamp exists in poll_until_deadline.
Since Client::connect() now allows caller-supplied durations via connect_timeout, deadlines
larger than i32::MAX ms are possible, making this a real (though rare) premature-timeout case.

src/transport.rs[276-301]
src/client/mod.rs[1171-1181]
src/client/mod.rs[237-246]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`poll(2)` takes an `int` timeout in milliseconds. The new code clamps large remaining durations to `i32::MAX` ms, but still treats `poll()` returning 0 as a hard timeout, which is only correct when the *actual* absolute deadline has elapsed.

## Issue Context
This is reachable now that `Client` exposes a public connect timeout override; a caller can set a duration larger than ~24.8 days without overflowing `Instant`, but the clamped `poll()` will return 0 after ~24.8 days and the code will incorrectly return `ErrorCode::Timeout` even though there is still remaining time before the true deadline.

## Fix Focus Areas
- src/transport.rs[276-307]
- src/client/mod.rs[1172-1180]
- src/client/mod.rs[237-246]

## Suggested fix
When `remaining.as_millis() > i32::MAX as u128` and `poll()` returns 0, do **not** return `Timeout`; instead, loop and recompute `remaining` (or explicitly check `Instant::now() >= deadline` before returning `Timeout`). Apply the same logic in `poll_until_deadline`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Instant add can panic ✓ Resolved 🐞 Bug ☼ Reliability

Description

Client::connect() computes deadline via Instant::now() + timeout, and the new public
set_connect_timeout(Duration) allows arbitrary durations; sufficiently large values can overflow
Instant arithmetic and panic instead of returning a Result error.

Code

src/client/mod.rs[R212-224]

+    /// Override the overall wall-clock budget for subsequent `connect()`
+    /// calls (DNS resolution, TCP connect, TLS handshake, RTMP handshake, and
+    /// the AMF `connect`/`createStream` exchange all share this one budget).
+    /// Defaults to `TCP_CONNECT_TIMEOUT_SECS` when never called. Useful for
+    /// slower CI hosts or higher-latency TLS handshakes where the default is
+    /// too tight.
+    pub fn set_connect_timeout(&mut self, timeout: Duration) {
+        self.connect_timeout = Some(timeout);
+    }
+
    /// Connect to an RTMP(S) server at `rtmp://host[:port]/app/streamKey` or
    /// `rtmps://host[:port]/app/streamKey`.
    ///

Relevance

⭐⭐⭐ High

Team previously accepted guarding public setters against overflow/saturation; similar hardening in
PR #58 and deadline work in #99.

PR-#58
PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR introduces a public setter accepting arbitrary Duration and uses unchecked Instant
addition to compute the deadline, which can overflow/panic for out-of-range durations.

src/client/mod.rs[212-220]
src/client/mod.rs[237-241]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Client::set_connect_timeout(Duration)` stores an arbitrary `Duration`, and `connect()` later computes `deadline` with `Instant::now() + duration`. For extremely large durations, `Instant` addition can overflow and panic, producing a caller-visible crash from a public API path.

### Issue Context
This is a robustness issue: callers may set `Duration::MAX` or similarly large values when trying to “disable” timeouts.

### Fix Focus Areas
- src/client/mod.rs[212-220]
- src/client/mod.rs[237-241]

### Suggested fix
- Replace `Instant::now() + timeout` with `Instant::now().checked_add(timeout).ok_or(ErrorCode::Internal)?` (or another appropriate error code).
- Optionally also validate/clamp in `set_connect_timeout()` (e.g., reject absurdly large values) to fail earlier with a clear error.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Deadline rounding overshoots ✓ Resolved 🐞 Bug ≡ Correctness

Description

poll_until_deadline() rounds any positive sub-millisecond remaining time up to a 1ms poll()
timeout, which can block past the absolute deadline and exceed the intended wall-clock
connect/handshake budget.

Code

src/client/mod.rs[R1166-1171]

+        let remaining = deadline.saturating_duration_since(Instant::now());
+        if remaining.is_zero() {
+            return Err(ErrorCode::Timeout);
+        }
+        let timeout_ms = remaining.as_millis().min(i32::MAX as u128).max(1) as i32;
+        let mut pfd = libc::pollfd {

Relevance

⭐⭐ Medium

They care about deadline correctness (EINTR/timeout fixes accepted in PR #87/#66), but no prior
feedback on sub-ms rounding.

PR-#87
PR-#66

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly rounds the computed remaining time up to at least 1ms (.max(1)), so any
remaining budget below 1ms will still wait up to 1ms in poll(), which can exceed the absolute
deadline.

src/client/mod.rs[1154-1187]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`poll_until_deadline()` computes `timeout_ms` as `max(1)` ms. When the remaining budget is `>0` but `<1ms`, this rounds to 1ms and can cause `poll()` to wait beyond the absolute `deadline`, violating the helper’s strict wall-clock semantics.

### Issue Context
This helper is used to bound connect/handshake/AMF waits to a shared absolute deadline; rounding up undermines that guarantee.

### Fix Focus Areas
- src/client/mod.rs[1166-1171]

### Suggested fix
- If `remaining < 1ms`, call `poll(..., 0)` (non-blocking poll) and treat `rc == 0` as `Timeout`, or explicitly return `Timeout` when `remaining` is below the granularity you can represent.
- Remove `.max(1)` and handle the `0ms` timeout case intentionally.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. Connect budget behavior change ✓ Resolved 🔗 Cross-repo conflict ☼ Reliability

Description

Client::connect() now enforces one wall-clock budget across DNS/TCP/TLS/RTMP handshake/AMF, still
defaulting to TCP_CONNECT_TIMEOUT_SECS (10s). librtmp2-server’s E2E tests call
Client::connect() without set_connect_timeout(), so upgrading to a librtmp2 version containing
this PR may introduce timeouts/flakes on slower CI hosts unless those tests increase the budget.

Code

src/client/mod.rs[R237-241]

+        let deadline = Instant::now()
+            + self
+                .connect_timeout
+                .unwrap_or(Duration::from_secs(TCP_CONNECT_TIMEOUT_SECS));
        let addrs = resolve_socket_addrs(&host, port, deadline)?;

Relevance

⭐⭐ Medium

History shows they intentionally enforce a single overall connect deadline (PR #99, #120); impact on
downstream tests unclear.

PR-#99
PR-#120

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR explicitly keeps the default connect budget at TCP_CONNECT_TIMEOUT_SECS = 10 and uses it as
the overall deadline for Client::connect(); that deadline is then shared across subsequent phases
(including handshake and AMF exchange). In librtmp2-server, E2E tests invoke Client::connect()
directly without calling set_connect_timeout(), so upgrading to a librtmp2 release with this
behavior can cause timeouts on slower environments unless the downstream tests adopt the new setter.

src/client/mod.rs[41-47]
src/client/mod.rs[212-292]
External repo: OpenRTMP/librtmp2-server, tests/rtmp_http_e2e.rs [80-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Client::connect()` now applies a single wall-clock deadline across more connection phases than before (DNS + TCP + TLS + RTMP handshake + AMF connect/createStream). Downstream consumers like `librtmp2-server` that rely on the default budget and run on slower/loaded CI can start seeing `Timeout` failures during E2E tests when they upgrade.

## Issue Context
- The default connect budget is still driven by `TCP_CONNECT_TIMEOUT_SECS` (10 seconds), but it now bounds the full connect sequence.
- `librtmp2-server` E2E tests currently construct `Client::new()` and call `connect()` directly, with no opportunity to raise the budget unless they adopt `set_connect_timeout()`.

## Fix Focus Areas
- CHANGELOG.md[14-20]
- src/client/mod.rs[41-47]
- src/client/mod.rs[212-292]
- /cross_repos/librtmp2-server/tests/rtmp_http_e2e.rs[80-93]

## Suggested fix
1. In this repo, add a short entry under `CHANGELOG.md` → `[Unreleased]` noting that `Client::connect()` now uses a single overall deadline (and that embedders can use `Client::set_connect_timeout()` to raise it).
2. In `librtmp2-server` (when bumping to the new librtmp2 version), update E2E tests to set a larger timeout before calling `connect()`, e.g.:
  - `let mut c = Client::new(); c.set_connect_timeout(Duration::from_secs(30)); c.connect(...)?;`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Client::connect branches on use_tls 📘 Rule violation ⌂ Architecture

Description

Connection setup in Client::connect selects between TLS and plaintext transports based on
use_tls, leaking TLS/plaintext branching above the transport abstraction layer. This violates the
requirement to confine transport-security decisions to the transport layer or below.

Code

src/client/mod.rs[R263-274]

        let mut transport = if use_tls {
-            Transport::connect_tls(
+            let remaining = deadline.saturating_duration_since(Instant::now());
+            Transport::connect_tls_with_timeout(
                stream,
                &host,
                self.tls_ca_file.as_deref(),
                self.tls_insecure,
+                remaining,
            )?
        } else {
            Transport::new_plain(stream.into_raw_fd())
        };

Relevance

⭐ Low

Repo repeatedly branches on TLS at call sites (e.g., client/transport work in PR #39/#101); no
evidence of enforcing this rule.

PR-#39
PR-#101

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2107515 prohibits TLS/plaintext branching above the transport abstraction. The
modified Client::connect code conditionally creates a TLS transport via
Transport::connect_tls_with_timeout(...) or a plaintext transport via Transport::new_plain(...)
based on use_tls, demonstrating the prohibited branching in client logic.

Rule 2107515: Prohibit TLS/plaintext branching above the transport abstraction layer
src/client/mod.rs[263-274]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Client::connect` branches on `use_tls` to choose TLS vs plaintext transport creation, which violates the rule that TLS/plaintext branching must not exist above the transport abstraction layer.

## Issue Context
The client module is higher-level code that should not inspect or branch on transport security mode. Transport selection should be encapsulated behind the transport layer API (e.g., a single constructor/factory that takes a scheme/mode and returns a `Transport`).

## Fix Focus Areas
- src/client/mod.rs[263-274]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 2864f77

Results up to commit 450e7c0

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Instant add can panic ✓ Resolved 🐞 Bug ☼ Reliability

Description

Client::connect() computes deadline via Instant::now() + timeout, and the new public
set_connect_timeout(Duration) allows arbitrary durations; sufficiently large values can overflow
Instant arithmetic and panic instead of returning a Result error.

Code

src/client/mod.rs[R212-224]

+    /// Override the overall wall-clock budget for subsequent `connect()`
+    /// calls (DNS resolution, TCP connect, TLS handshake, RTMP handshake, and
+    /// the AMF `connect`/`createStream` exchange all share this one budget).
+    /// Defaults to `TCP_CONNECT_TIMEOUT_SECS` when never called. Useful for
+    /// slower CI hosts or higher-latency TLS handshakes where the default is
+    /// too tight.
+    pub fn set_connect_timeout(&mut self, timeout: Duration) {
+        self.connect_timeout = Some(timeout);
+    }
+
    /// Connect to an RTMP(S) server at `rtmp://host[:port]/app/streamKey` or
    /// `rtmps://host[:port]/app/streamKey`.
    ///

Relevance

⭐⭐⭐ High

Team previously accepted guarding public setters against overflow/saturation; similar hardening in
PR #58 and deadline work in #99.

PR-#58
PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR introduces a public setter accepting arbitrary Duration and uses unchecked Instant
addition to compute the deadline, which can overflow/panic for out-of-range durations.

src/client/mod.rs[212-220]
src/client/mod.rs[237-241]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Client::set_connect_timeout(Duration)` stores an arbitrary `Duration`, and `connect()` later computes `deadline` with `Instant::now() + duration`. For extremely large durations, `Instant` addition can overflow and panic, producing a caller-visible crash from a public API path.

### Issue Context
This is a robustness issue: callers may set `Duration::MAX` or similarly large values when trying to “disable” timeouts.

### Fix Focus Areas
- src/client/mod.rs[212-220]
- src/client/mod.rs[237-241]

### Suggested fix
- Replace `Instant::now() + timeout` with `Instant::now().checked_add(timeout).ok_or(ErrorCode::Internal)?` (or another appropriate error code).
- Optionally also validate/clamp in `set_connect_timeout()` (e.g., reject absurdly large values) to fail earlier with a clear error.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Deadline rounding overshoots ✓ Resolved 🐞 Bug ≡ Correctness

Description

poll_until_deadline() rounds any positive sub-millisecond remaining time up to a 1ms poll()
timeout, which can block past the absolute deadline and exceed the intended wall-clock
connect/handshake budget.

Code

src/client/mod.rs[R1166-1171]

+        let remaining = deadline.saturating_duration_since(Instant::now());
+        if remaining.is_zero() {
+            return Err(ErrorCode::Timeout);
+        }
+        let timeout_ms = remaining.as_millis().min(i32::MAX as u128).max(1) as i32;
+        let mut pfd = libc::pollfd {

Relevance

⭐⭐ Medium

They care about deadline correctness (EINTR/timeout fixes accepted in PR #87/#66), but no prior
feedback on sub-ms rounding.

PR-#87
PR-#66

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly rounds the computed remaining time up to at least 1ms (.max(1)), so any
remaining budget below 1ms will still wait up to 1ms in poll(), which can exceed the absolute
deadline.

src/client/mod.rs[1154-1187]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`poll_until_deadline()` computes `timeout_ms` as `max(1)` ms. When the remaining budget is `>0` but `<1ms`, this rounds to 1ms and can cause `poll()` to wait beyond the absolute `deadline`, violating the helper’s strict wall-clock semantics.

### Issue Context
This helper is used to bound connect/handshake/AMF waits to a shared absolute deadline; rounding up undermines that guarantee.

### Fix Focus Areas
- src/client/mod.rs[1166-1171]

### Suggested fix
- If `remaining < 1ms`, call `poll(..., 0)` (non-blocking poll) and treat `rc == 0` as `Timeout`, or explicitly return `Timeout` when `remaining` is below the granularity you can represent.
- Remove `.max(1)` and handle the `0ms` timeout case intentionally.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Connect budget behavior change ✓ Resolved 🔗 Cross-repo conflict ☼ Reliability

Description

Client::connect() now enforces one wall-clock budget across DNS/TCP/TLS/RTMP handshake/AMF, still
defaulting to TCP_CONNECT_TIMEOUT_SECS (10s). librtmp2-server’s E2E tests call
Client::connect() without set_connect_timeout(), so upgrading to a librtmp2 version containing
this PR may introduce timeouts/flakes on slower CI hosts unless those tests increase the budget.

Code

src/client/mod.rs[R237-241]

+        let deadline = Instant::now()
+            + self
+                .connect_timeout
+                .unwrap_or(Duration::from_secs(TCP_CONNECT_TIMEOUT_SECS));
        let addrs = resolve_socket_addrs(&host, port, deadline)?;

Relevance

⭐⭐ Medium

History shows they intentionally enforce a single overall connect deadline (PR #99, #120); impact on
downstream tests unclear.

PR-#99
PR-#120

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR explicitly keeps the default connect budget at TCP_CONNECT_TIMEOUT_SECS = 10 and uses it as
the overall deadline for Client::connect(); that deadline is then shared across subsequent phases
(including handshake and AMF exchange). In librtmp2-server, E2E tests invoke Client::connect()
directly without calling set_connect_timeout(), so upgrading to a librtmp2 release with this
behavior can cause timeouts on slower environments unless the downstream tests adopt the new setter.

src/client/mod.rs[41-47]
src/client/mod.rs[212-292]
External repo: OpenRTMP/librtmp2-server, tests/rtmp_http_e2e.rs [80-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Client::connect()` now applies a single wall-clock deadline across more connection phases than before (DNS + TCP + TLS + RTMP handshake + AMF connect/createStream). Downstream consumers like `librtmp2-server` that rely on the default budget and run on slower/loaded CI can start seeing `Timeout` failures during E2E tests when they upgrade.

## Issue Context
- The default connect budget is still driven by `TCP_CONNECT_TIMEOUT_SECS` (10 seconds), but it now bounds the full connect sequence.
- `librtmp2-server` E2E tests currently construct `Client::new()` and call `connect()` directly, with no opportunity to raise the budget unless they adopt `set_connect_timeout()`.

## Fix Focus Areas
- CHANGELOG.md[14-20]
- src/client/mod.rs[41-47]
- src/client/mod.rs[212-292]
- /cross_repos/librtmp2-server/tests/rtmp_http_e2e.rs[80-93]

## Suggested fix
1. In this repo, add a short entry under `CHANGELOG.md` → `[Unreleased]` noting that `Client::connect()` now uses a single overall deadline (and that embedders can use `Client::set_connect_timeout()` to raise it).
2. In `librtmp2-server` (when bumping to the new librtmp2 version), update E2E tests to set a larger timeout before calling `connect()`, e.g.:
  - `let mut c = Client::new(); c.set_connect_timeout(Duration::from_secs(30)); c.connect(...)?;`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Client::connect branches on use_tls 📘 Rule violation ⌂ Architecture

Description

Connection setup in Client::connect selects between TLS and plaintext transports based on
use_tls, leaking TLS/plaintext branching above the transport abstraction layer. This violates the
requirement to confine transport-security decisions to the transport layer or below.

Code

src/client/mod.rs[R263-274]

        let mut transport = if use_tls {
-            Transport::connect_tls(
+            let remaining = deadline.saturating_duration_since(Instant::now());
+            Transport::connect_tls_with_timeout(
                stream,
                &host,
                self.tls_ca_file.as_deref(),
                self.tls_insecure,
+                remaining,
            )?
        } else {
            Transport::new_plain(stream.into_raw_fd())
        };

Relevance

⭐ Low

Repo repeatedly branches on TLS at call sites (e.g., client/transport work in PR #39/#101); no
evidence of enforcing this rule.

PR-#39
PR-#101

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2107515 prohibits TLS/plaintext branching above the transport abstraction. The
modified Client::connect code conditionally creates a TLS transport via
Transport::connect_tls_with_timeout(...) or a plaintext transport via Transport::new_plain(...)
based on use_tls, demonstrating the prohibited branching in client logic.

Rule 2107515: Prohibit TLS/plaintext branching above the transport abstraction layer
src/client/mod.rs[263-274]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Client::connect` branches on `use_tls` to choose TLS vs plaintext transport creation, which violates the rule that TLS/plaintext branching must not exist above the transport abstraction layer.

## Issue Context
The client module is higher-level code that should not inspect or branch on transport security mode. Transport selection should be encapsulated behind the transport layer API (e.g., a single constructor/factory that takes a scheme/mode and returns a `Transport`).

## Fix Focus Areas
- src/client/mod.rs[263-274]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 93d5b7d

[qodo-code-review]

Code review by qodo was updated up to the latest commit 2864f77

[AlexanderWagnerDev]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 2864f77697

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".