Retire stateful connections without reset proof - #103
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
🟡 Changes recommended
The new stateful error-finalization path has a non-exhaustive match (won’t compile) and the hybrid/stateful error wrapping can misclassify client disconnect/write errors as backend errors, impacting runtime logging/behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refines stateful connection lifecycle handling so pooled backend leases are always finalized, and backend reuse/retirement decisions are based on explicit session-exit classification and “clean vs dirty” response-transfer outcomes.
Changes:
- Adds explicit stateful session exit/disposition tracking and uses it to decide whether to reuse or retire pooled backend connections.
- Adjusts response-transfer semantics so a completed response with no queued backend bytes can keep the backend reusable even if the final client write fails.
- Prevents retired deadpool objects from re-entering the idle pool by taking the object before dropping, and extends pool-status coverage in tests.
File summaries
| File | Description |
|---|---|
| src/session/response_transfer.rs | Updates backend reuse decision for ClientWrite when the connection is otherwise reusable, and updates tests accordingly. |
| src/session/handlers/stateful.rs | Introduces explicit exit/disposition types and finalizes pooled connection guards on all paths (but currently has a blocking error-path issue). |
| src/session/handlers/hybrid.rs | Centralizes backend lease finalization via a new finalize() helper and applies stateful-loop disposition to pooled lease retirement. |
| src/pool/provider.rs | Ensures retired connections cannot be returned to deadpool by dropping Object::take(conn) and extends test assertions around pool size. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d6fbd37 to
4f3244a
Compare
7afa6cc
into
nntpp-27-cancel-safe-stateful-forwarding
Summary
Branch-added tests