Skip to content

fix: bound a PDU frame with one deadline - #20

Merged
marcinpsk merged 2 commits into
developfrom
fix/frame-deadline
Sep 15, 2026
Merged

marcinpsk merged 2 commits into
developfrom
fix/frame-deadline

Conversation

@marcinpsk

@marcinpsk marcinpsk commented Sep 15, 2026

Copy link
Copy Markdown
Owner

The session set a five second timeout on each read call, and read_exact
loops over read. Every partial read restarted the clock, so a peer that
sent one byte every four seconds held the session open forever and the
master never got its responses.

The receive step now computes one deadline when the first byte of a frame
arrives, and gives each following read only the time that remains. The
wait for the first byte stays unbounded, so an idle session does not
reconnect on its own. Exhausted time fails the frame before the socket
call, because a zero timeout means block forever and the standard library
rejects a zero duration.

The receive step takes the frame budget as an argument. The run loop
passes the existing constant. The tests pass a short budget, which pins
the boundary in under a second. This follows the netlink acquisition
step, which computes one deadline for a whole inventory and passes the
remaining time into each wait.

A fake master drips one frame byte every two seconds against the actual
binary, then asserts that the session ends near five seconds, names the
deadline in its log, and accepts the reconnection. A socket pair test
drives the receive step with a 100 ms budget and asserts expiry, success
inside the budget, and an unbounded idle wait. Both tests fail against
the unfixed code.

An adversarial review noted that a final read can return just after the
deadline and still complete the frame. The overrun is bounded by one
read, because the deadline is fixed and the peer cannot extend it.
Rejecting a complete PDU at that boundary would discard a valid request,
and the sub-millisecond window cannot be tested without flake.

Closes #15

Summary by CodeRabbit

  • Bug Fixes
    • Improved AgentX communication timeouts by applying a deadline to each complete frame rather than restarting the timeout for every read.
    • Sessions now close promptly when a frame arrives too slowly, preventing connections from remaining open indefinitely.
    • Frames received within the allotted time continue to be processed successfully.
    • Timeout failures now provide clearer deadline-related error reporting.

The session set a five second timeout on each read call, and read_exact
loops over read. Every partial read restarted the clock, so a peer that
sent one byte every four seconds held the session open forever and the
master never got its responses.

The receive step now computes one deadline when the first byte of a frame
arrives, and gives each following read only the time that remains. The
wait for the first byte stays unbounded, so an idle session does not
reconnect on its own. Exhausted time fails the frame before the socket
call, because a zero timeout means block forever and the standard library
rejects a zero duration.

The receive step takes the frame budget as an argument. The run loop
passes the existing constant. The tests pass a short budget, which pins
the boundary in under a second. This follows the netlink acquisition
step, which computes one deadline for a whole inventory and passes the
remaining time into each wait.

A fake master drips one frame byte every two seconds against the actual
binary, then asserts that the session ends near five seconds, names the
deadline in its log, and accepts the reconnection. A socket pair test
drives the receive step with a 100 ms budget and asserts expiry, success
inside the budget, and an unbounded idle wait. Both tests fail against
the unfixed code.

An adversarial review noted that a final read can return just after the
deadline and still complete the frame. The overrun is bounded by one
read, because the deadline is fixed and the peer cannot extend it.
Rejecting a complete PDU at that boundary would discard a valid request,
and the sub-millisecond window cannot be tested without flake.

Closes #15
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 138e4a86-92c3-412b-906a-98ce40389136

📥 Commits

Reviewing files that changed from the base of the PR and between af0244f and 3f0c3e5.

📒 Files selected for processing (2)
  • src/session.rs
  • tests/real_namespace.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The session now gives each started AgentX PDU one deadline. Header and payload reads share that deadline. Unit and end-to-end tests verify timeout and successful completion behavior.

Changes

Frame Deadline Enforcement

Layer / File(s) Summary
Deadline-bounded frame reading
src/session.rs
receive now accepts a frame budget and uses one deadline for the header and payload. read_before handles partial reads, interruptions, timeouts, EOF, and other errors. Session entry points pass IO_TIMEOUT.
Deadline behavior validation
src/session.rs, tests/real_namespace.rs
Tests verify that timely frames decode as Ping and that slow frames produce a frame deadline error. The real binary test verifies EOF, reconnect behavior, elapsed timing, and the logged error text.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant FakeMaster
  participant AgentXSession
  participant Supervisor
  FakeMaster->>AgentXSession: Send PDU bytes over time
  AgentXSession->>AgentXSession: Enforce one frame deadline
  AgentXSession-->>Supervisor: Report frame deadline error
  Supervisor->>FakeMaster: Attempt reconnect
Loading

Merge Risk: ⚪ Minimal · up to 5cdb1

Idle sessions continue waiting for their first byte, while started frames use the shared deadline. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The implementation in [#15] uses one Instant deadline for header and payload reads. read_before checks the remaining duration before setting the socket timeout, so zero-duration timeouts are not p… Keep the test writer open after the partial frame, or send enough delayed bytes to keep the unfixed receiver blocked past the bounded assertion. Then assert deadline-based closure and reconnection with a bounded wait. Retain the existing un…
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changes are limited to src/session.rs and tests/real_namespace.rs. The source changes implement the frame deadline, idle behavior, and timeout handling required by [#15]. The unit and end-to-e…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: applying one deadline to each PDU frame.
Full details: Linked Issues check

Explanation

The implementation in [#15] uses one Instant deadline for header and payload reads. read_before checks the remaining duration before setting the socket timeout, so zero-duration timeouts are not passed. The unit tests cover expiry, completion within the budget, and idle waiting. The end-to-end test does not meet the regression acceptance criterion that it fails against the unfixed code. In tests/real_namespace.rs, the dripper sends only four header bytes and then closes at about six seconds. The old per-read timeout would also end the session on EOF at about six seconds, which fits the test range. The current end-to-end test therefore does not prove the required failure against the old behavior.

Resolution

Keep the test writer open after the partial frame, or send enough delayed bytes to keep the unfixed receiver blocked past the bounded assertion. Then assert deadline-based closure and reconnection with a bounded wait. Retain the existing unit coverage for the short frame budget.

✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/frame-deadline
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/frame-deadline

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

I am a rabbit watching bytes arrive,
One frame clock keeps the session alive.
Slow drips now meet a timely end,
Quick frames pass and answers send.
I twitch my ears: the deadline is clear.

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

@marcinpsk
marcinpsk changed the base branch from main to develop September 15, 2026 11:10
@marcinpsk
marcinpsk merged commit 8b023d7 into develop Sep 15, 2026
8 checks passed
@marcinpsk
marcinpsk deleted the fix/frame-deadline branch September 15, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bound a PDU frame with one deadline

1 participant