Repository navigation
turnloop-mongodb and turnloop-smtp: settlement, write verdicts, state query, entropy docs, required TLS, streaming bodies - #102
Merged
Conversation
`rows(&self) -> impl Iterator<Item = Result<&'a RawDocument>>` is defined in an edition-2024 crate, so its opaque return type captured every lifetime in scope, including the anonymous `&self` borrow. The rows only borrow the reply (`'a`), but the iterator was tied to the `CursorBatch` value: a host that parsed a batch in a helper and returned `batch.rows()` got E0597, and an edition-2021 host hit the same error on `match batch.rows().next()` in tail position, whose temporaries outlive the block's locals. The return type now says `+ use<'a>`, which is what rustc's own diagnostic suggests: the closure copies the `&'a RawArray` out of `self` and holds nothing borrowed from the batch. Callers that used rows() in place are unaffected. tests/cursor_rows_capture.rs compiles both patterns and asserts the rows they return. Without the change the test target fails to build with E0597 at the function returning `batch.rows()`. Closes #69
`Connection::receive` refuses bytes unless a handshake, authentication or command reply is outstanding, but the only state a host could observe was `is_ready()`. Every host therefore kept its own `expecting_reply` flag to decide when reading was allowed, a second copy of the state machine free to drift from the real one. `accepts_receive()` now reports exactly that condition, and `receive` itself calls it, so the two cannot disagree. A named query was chosen over exposing the private `State` enum: it answers the host's actual question and leaves the state machine free to grow states without a breaking change. The new test walks a connection through every state (New, TLS upgrade, handshake, SCRAM conversation, Ready, command, unreleased reply, draining unacknowledged write, Closed) and checks at each step that `accepts_receive()` agrees with whether `receive(&[])` is accepted. The README's driving steps mention the query. Closes #70
MongoDB reports per-document failures inside a successful command: a duplicate-key insert answers `ok: 1` with a `writeErrors` array, and an unmet write concern answers `ok: 1` with `writeConcernError`. `WriteResult::parse` only consulted `Error::from_response` when `ok` was 0, so a host that trusted its `Ok` treated a rejected insert as a successful one. The safe order (check `from_response` first) was documented nowhere. The issue offered two fixes: make `parse` itself fail, or document that callers must run `from_response` first. Documentation alone leaves the trap armed, so `parse` now runs `Error::from_response` unconditionally. A nonempty `writeErrors` fails as `ErrorKind::BulkWrite` and a `writeConcernError` as `ErrorKind::Server`, each with the full server response attached, which is also what the Node driver throws. A write-concern error does fail even though the write was applied, because the durability the caller asked for was not confirmed. Bulk aggregation still needs the errors as data, so the old lenient behaviour moves to `WriteResult::decode` (fails only on `ok: 0` or a malformed reply), and `BulkResult::accept` uses it. `WriteResult::succeeded()` gives `decode` callers the verdict. The type docs and README explain which to use and that `parse` needs no prior `from_response`. Behaviour change: `WriteResult::parse` now returns `Err` for `ok: 1` replies with write or write-concern errors. Callers that aggregated those errors from `parse` should switch to `decode`. The new test covers the duplicate-key and write-concern replies, clean replies with and without an empty `writeErrors`, `ok: 0`, and `BulkResult` aggregation with original indices. With `parse` reverted to the old check, it fails on the duplicate-key `unwrap_err`. Closes #72
`Connection::fail()` pushed `Failed` only when no reply was held. With a reply received but not yet released it pushed nothing: the token's reply stayed readable on a connection that had just been declared broken, and a host that settles operations from the event stream never learned that the operation it was still holding had failed. The settlement contract is now explicit and applied on every path: an accepted token is settled exactly once, by `Failed`, by `Unacknowledged`, or by `release_reply()` after its `Reply` event. `fail()` before that release revokes the reply (`reply()` and `release_reply()` then return errors) and pushes `Failed` for the token ahead of `Closed`. If the host has not polled the `Reply` event yet, it is withdrawn from the queue, so that token sees exactly one event. A reply already released was already settled, and a later failure reports no token for it. `close()` is unchanged: it is the orderly path (the async adapter uses it on EOF) and still keeps a received reply readable until released, as the existing close test requires. Revoking in `fail()` rather than preserving is deliberate. `fail()` means the host has declared the transport or protocol broken, and a network error after the request was sent is already the ambiguous may-or-may-not-have-applied outcome the retry rules are built for. Behaviour change: after `fail()`, an unreleased reply can no longer be read. The new test covers a reply whose event was never polled, one whose event was polled but not released, and one already released, asserting the exact event sequence and that the reply accessors fail. Against the previous `fail()` it reports `["reply"]` where `["failed"]` is required. Closes #73
The crate needs the host to supply every random or unique value, but those obligations were scattered: the SCRAM nonce was mentioned in the README's driving steps, the `ObjectIdGenerator` requirement lived in a sentence of the command-results section, and session UUIDs and selection entropy only in item docs. The `_id` one in particular fails silently: an insert without `_id` succeeds, and the server assigns an id the host never learns. The README now has a "Host entropy" section, placed before the driving steps where a new integrator starts, listing all four obligations (SCRAM nonce, document `_id`, session identity, server-selection entropy), where each value goes, what to supply, and what goes wrong without it. It also states which ones the `turnloop` feature's client fills on its own; `_id` is not one of them. The crate docs carry a short `# Host entropy` section linking to it, and `Connection::connected`, the `command` and `auth` modules, `ObjectIdGenerator` and `Session::new` link to that section. The two old README mentions now point to it instead of restating it. Documentation only; rustdoc builds with `-D warnings`. Closes #71
The issue reports that a `Tls::Required` session reaches `Ready` in plaintext
when the server offers no STARTTLS, so hosts such as Perry's P6 lane guard
`on_ready` themselves. On current main the reachable paths already refuse:
`after_ehlo` fails with `ETLS` when STARTTLS is not advertised (EHLO or HELO
fallback), and a refused STARTTLS fails through `unexpected`. None of this was
documented, though, and the refusal lived in one branch of the STARTTLS
decision rather than at the point the issue names, so any future path to
`ready()` that skipped that branch would silently hand out a plaintext
session.
`ready()` is the only transition to `Ready`, and it now re-checks the policy:
under `Tls::Required` (or `Implicit`) without an established TLS layer it emits
`Failed` with code `ETLS` ("TLS is required but the SMTP session was never
encrypted"), then `CloseTransport` and `Closed`, and never `Ready`. The
earlier check in `after_ehlo` stays, because it must also stop AUTH
credentials from being sent in the clear, which happens before `Ready`. The
`Tls` variants and the README now state the guarantee, so hosts can drop their
own guard.
Tests:
- `tests::ready_refuses_plaintext_under_mandatory_tls` (unit) enters
authentication directly, skipping the STARTTLS decision, and asserts
`Failed(ETLS)`, `CloseTransport`, `Closed` for Required and Implicit, and
`Ready` for Opportunistic and None. With the new check disabled it fails
("Required: [Ready]").
- `required_tls_never_reaches_ready_in_the_clear` pins the wire behaviour:
no STARTTLS advertised means `ETLS` with no AUTH bytes emitted; a 454 reply
to STARTTLS means `ETLS` and no `Ready`; an accepted STARTTLS reaches
`Ready` only after `tls_established` and a fresh EHLO; Opportunistic reaches
`Ready` in the clear. This test also passes on the previous code, which
already handled these paths.
Closes #56
`Connection::send` took the whole message as one `&[u8]` and `encode_data`
copied it again into DATA storage, so a 25 MB attachment meant two full copies
of the body in memory before the first byte reached the socket.
A streaming path now sits beside the one-shot `send`, shaped like the HTTP/1
client's `send_body`/`finish_body`:
- `start_send(token, envelope, message_id, StreamBody { size, eight_bit }, now)`
runs the same envelope checks and MAIL/RCPT/DATA exchange as `send`.
- On the server's 354 the core emits the new `Event::BodyReady { token }`.
- The host then calls `send_chunk(bytes, now)` any number of times and
`finish_body(now)`, and `can_send_body()` reports when that is allowed.
- The token settles with the usual single `Sent` or `Failed`.
Draining `output()` between chunks keeps memory bounded by one chunk. `send`
stays as the convenience and now shares its checks with `start_send`.
Encoding is incremental. `DataEncoder` (public) carries two bits of state
across calls: whether the next byte starts a line (for dot-stuffing) and
whether the previous byte was a CR (so a CR|LF split across chunks collapses
to one CRLF). `encode_data` is now `DataEncoder` in one call, so both paths
emit identical bytes by construction.
SIZE and BODY=8BITMIME go in MAIL FROM before any content exists, so the
stream declares them up front. `size` is optional and is checked against the
server's advertised limit; `eight_bit` must be supported by the server. Once
DATA content has been written it cannot be retracted, and RSET would be read
as message text. So an undeclared 8-bit chunk, or any server reply while the
body is open, fails the message and closes the transport rather than
terminating a partial message.
Breaking: `Event` gains the `BodyReady` variant, so exhaustive matches need
an arm (the crate's own adapter already uses a wildcard).
Tests:
- `incremental_dot_stuffing_is_split_invariant` encodes a message full of
hazards (an embedded CRLF.CRLF, leading dots, CR/LF/CRLF mixes, CR then a
line-leading dot) at every two- and three-way split and byte by byte, and
requires output and SIZE identical to `encode_data`. It also covers
CRLF.CRLF split as CRLF|.CRLF, CR|LF.CRLF, CRLF.|CRLF and one byte per
chunk, which must encode as `..`.
- `streamed_send_writes_the_one_shot_bytes` requires the full client
transcript of a streamed send, at several chunk sizes, to equal the one-shot
transcript byte for byte, followed by the same `Sent`. It also covers an
empty body without SIZE and chunks refused before 354 or after finish.
- `streamed_send_failures_never_terminate_a_partial_body` covers undeclared
8-bit content (no terminator written; Failed, CloseTransport, Closed), a
declared 8-bit body, a premature server reply, a refused DATA (RSET, no
BodyReady), and declarations rejected before any envelope byte.
Carrying no state across `encode` calls fails the split tests. The
allocation gate for the one-shot path is unchanged.
Closes #55
The test spells out the tail-position match from #69 on purpose, which clippy's manual_map and needless_match lints flag under -D warnings. Allow both on that one function, with the reason stated.
|
Warning Review limit reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
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 |
This was referenced Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes five
turnloop-mongodbissues and twoturnloop-smtpissues. The changes stay insideprotocols/turnloop-mongodbandprotocols/turnloop-smtp. There is one commit per issue, plus a small commit that keeps the #69 test clippy-clean.turnloop-mongodb
#69:
CursorBatch::rows()captured&self. The return type now saysimpl Iterator<...> + use<'a>, which is what rustc suggests, so the iterator borrows only the reply and not the batch. The newtests/cursor_rows_capture.rsreturnsbatch.rows()from a helper and compiles the tail-positionmatch batch.rows().next()pattern from the issue. Without the change it fails to build with E0597.Closes #69
#70: no state query.
Connection::accepts_receive()reports whether a handshake, auth or command reply is outstanding.receive()now calls it for its own check, so the two can't disagree. A new test walks every state (New, TLS, handshake, SCRAM, Ready, command, unreleased reply, draining unacknowledged write, Closed) and checks thataccepts_receive()matches whetherreceive(&[])is accepted.Closes #70
#71: host-entropy obligations scattered. The README has a new "Host entropy" section, placed before the driving steps. It lists all four obligations: the SCRAM nonce, the document
_id(viaObjectIdGenerator), session UUIDs, and server-selection entropy. For each it says where the value goes, what to supply and what goes wrong without it, and it names which ones theturnloop-feature client fills itself (_idis never one of them). The crate docs have a short# Host entropysection, andConnection::connected, thecommandandauthmodules,ObjectIdGeneratorandSession::newlink to it.Closes #71
#72:
WriteResult::parsepassedok: 1writes that hadwriteErrors.parseis now the verdict: it runsError::from_responsefirst and fails on a non-emptywriteErrors(BulkWrite) or on awriteConcernError(Server), with the full response attached, as the Node driver does. The old lenient behaviour moves to the newWriteResult::decode, which fails only onok: 0or a malformed reply, andBulkResult::acceptnow uses it. The newWriteResult::succeeded()gives the verdict fordecoderesults. The docs state thatparseneeds no earlierfrom_responsecall. The test covers duplicate-key, write-concern, clean, emptywriteErrors,ok: 0and bulk aggregation with the original indices, and it fails against the oldparse.Closes #72
#73:
fail()with an unreleased reply pushed noFailed. The contract is now explicit: an accepted token is settled exactly once, byFailed, byUnacknowledged, or byrelease_reply()after itsReply. Iffail()runs before that release, it revokes the reply (reply()andrelease_reply()then return errors) and pushesFailedfor the token ahead ofClosed. AReplyevent that hasn't been polled yet is taken out of the queue, so that token gets exactly one event.close(), the orderly path the async adapter uses on EOF, still keeps a received reply readable. The test covers a Reply that was never polled, one polled but not released, and one already released. Against the oldfail()it fails, reporting["reply"]where["failed"]is expected.Closes #73
turnloop-smtp
#56:
Tls::Requiredmust never reachReadyin the clear. Onmain, the paths that can actually be reached already refused:after_ehlofails withETLSwhen STARTTLS isn't advertised, and a refused STARTTLS fails too. That behaviour wasn't documented, though, and the check lived in one branch of the STARTTLS decision.ready(), the only transition toReady, now checks the policy again. UnderRequiredorImplicitwith no TLS established, it emitsFailed(ETLS),CloseTransportandClosed. TheTlsvariant docs and the README now state the guarantee, so hosts can drop their ownon_readyguard. A unit test enters authentication directly, skipping the STARTTLS decision, and it fails without the new check. A wire test pins the observable behaviour: no STARTTLS or a 454 givesETLSand no AUTH bytes, and an accepted STARTTLS reachesReadyonly aftertls_established. Reviewer note: the wire test also passes on the previous code, since those paths were already handled. The new code is the check at theReadytransition plus the documentation.Closes #56
#55: streaming body. This adds
start_send(token, envelope, message_id, StreamBody { size, eight_bit }, now), thenEvent::BodyReady { token }when the server answers 354, thensend_chunk(bytes, now)any number of times andfinish_body(now).can_send_body()reports when chunks are allowed. The token settles with the usual singleSentorFailed. The one-shotsendstays as a convenience. The new publicDataEncodernormalizes line endings and dot-stuffs incrementally, carrying line-start and after-CR state across chunks.encode_datanow calls it, so both paths emit identical bytes. SIZE and BODY=8BITMIME are declared up front because they go in MAIL FROM. Because DATA content can't be taken back, an undeclared 8-bit chunk or a server reply while the body is open fails the message and closes the transport instead of sending a terminator. The tests:encode_data.CRLF|.CRLF,CR|LF.CRLF,CRLF.|CRLFand one byte per chunk still encodes to...Closes #55
Breaking changes (pre-1.0 alpha)
turnloop_mongodb::command::WriteResult::parsenow returnsErrforok: 1replies that carrywriteErrorsorwriteConcernError. Callers that collected those errors throughparseshould useWriteResult::decode.turnloop_mongodb::Connection::fail()now revokes a reply that hasn't been released: afterwardsreply()andrelease_reply()return errors, and a queuedReplyevent for it is replaced byFailed.turnloop_smtp::Eventhas a newBodyReady { token }variant, so exhaustive matches need an arm.Checks run locally
cargo +nightly-2026-08-20 fmt --all --checkpython3 scripts/ci/check-paths.pycargo +nightly-2026-08-20 clippy --locked --workspace --all-targets --all-features -- -D warnings -D clippy::undocumented_unsafe_blocksRUSTDOCFLAGS='-D warnings' cargo +nightly-2026-08-20 doc --locked --workspace --all-features --no-depscargo +nightly-2026-08-20 test --locked -p turnloop-mongodb --all-featuresand-p turnloop-smtp --all-features, plus both with default featurescargo +1.97.1 check --locked --workspace --all-targets --all-features