Skip to content

relay: a re-REQ can cancel its own replacement, CLOSE during an invite mint is lost, and the frame queue is unbounded #948

Open

Description

@chappie-daemon

Tracked as

Each defect below is its own issue, so a factory run can own one without taking the set:

This issue stays as the evidence record: the line-by-line verification and the link to each original review comment.

Three concurrency and backpressure defects, plus two test gaps, found on #947 after it merged. Reported by Greptile's review, which completed after the merge; each one below was re-checked against the source, and the two I could not confirm are marked.

Nothing here is a regression from the port — the TypeScript capture has the same shapes in places — but the Rust rewrite makes two of them worse, and one (the frame queue) has no bound in either.

1. A replacement REQ can cancel the read that replaced it — connection.rs:213-241

Original review comment: #947 (comment)

begin_read inserts a new sender under the subscription id and signals the previous one to abort. end_read then removes by id, unconditionally:

pub fn end_read(&self, subscription_id: &str) {
    self.in_flight.lock()...remove(subscription_id);
}

So when a client re-REQs the same id with new filters, the old read keeps running until it notices the abort; when it finishes it calls end_read and drops the new read's sender. The new read holds the receiver, sees the channel close, and treats that as cancellation — it stops without sending EOSE, and it may stop before sending anything.

A socket that re-REQs a subscription can therefore get a silent subscription. A per-read token checked in end_read (remove only if the stored sender is still this read's) closes the window.

2. CLOSE during an invite mint cannot cancel anything — handlers/subscribe.rs:44-61

Original review comment: #947 (comment)

context.connection.subscribe(...);          // table entry only
maybe_issue_invite_code(&subscription_id, &filters, context).await;   // awaits invites.create
fetch_and_send(&subscription_id, &filters, context).await;            // begin_read happens here

No read is registered until fetch_and_send, so a CLOSE that lands while the mint is in flight finds nothing to abort. The handler then sends the invite event and starts the stored read — frames on a subscription the client has closed. Registering the cancellation before the first await and checking it before sending fixes both halves.

3. The frame queue is unbounded and the producer never yields — sink.rs:37-45

Original review comment: #947 (comment)

FrameSink::channel() is mpsc::unbounded_channel(). A stored read of thousands of large events queues every encoded frame as fast as it can decode them, while the connection task drains at socket speed. Nothing bounds queued bytes and nothing pauses the producer, so a slow client converts its own slowness into relay memory. This is the one defect with no equivalent bound anywhere in the design.

A bounded channel plus a policy for a full queue (pause the read, or close the connection as too slow) is the fix; the connection loop's liveness check also stops while a write is blocked, which is worth deciding at the same time.

4. Expiry is evaluated against a clock captured before the stream — handlers/subscribe.rs:257

Original review comment: #947 (comment)

is_expired_event(&event, now) uses now captured once before the loop, while the comment two lines above says the guard is asked per event so a mid-read AUTH applies. The authorization genuinely is per event; the clock is not. An event that expires while earlier records are being fetched is still delivered. The TypeScript path reads the clock per event.

5. Subscription id length is measured three ways — messages.rs:389, subscribe.rs:357

Original review comment: #947 (comment)

  • decode: subscription_id.chars().count()
  • REQ/COUNT handlers: subscription_id.len() as u64 (UTF-8 bytes)
  • TypeScript: UTF-16 units

With the shipped 256 limit, an id of 100 Chinese characters passes decoding (100 chars) and is rejected by both handlers (300 bytes), while the capture accepts it. encode_utf16().count() everywhere, with non-ASCII cases in the tests, makes the three agree.

6. Silent steps in the replay oracle are not asserted — tests/wire_parity.rs:111

Original review comment: #947 (comment)

none: value["send"].is_null() && value["drain"].is_null() && value["connect"].is_null(),

Silent steps carry a send object and an empty expect, so this is always false for them. collect then reads zero frames and returns immediately, which is why the recorded CLOSE in req-close-abort is never actually checked for silence. Verified against the fixture: req-close-abort has send=true with an empty expect.

Not confirmed: the claim that the same step's extra trailing frames go unchecked after the expected count is reached — plausible from the reader, not verified here.

7. Census rows that overstate coverage — tests/fixtures/ts-case-map.json:516

Original review comment: #947 (comment)

Not confirmed. The claim is that a_restricted_count_needs_a_scoped_authors_filter sends only an unauthenticated, unscoped COUNT, so the three cases mapped to it are counted as carried without the inputs that would exercise them. That needs reading the test, not the fixture.

Provenance

All seven came from Greptile's review of #947 (app.greptile.com/cameri/github/cameri/nostream/-/pull/947), which concluded failure on a confidence threshold (2/5 against a required 5/5) rather than on a defect. The check is not required and the merge did not wait on it. Items 1–6 above are my own confirmations from the source; 6's second half and 7 are unverified.

Priority order if only some are taken: 1 (silent subscription, client-visible), 3 (memory under a slow client), 2 (frames after CLOSE), then 5, 4, and the two test gaps.

Added after recon on job 010 (2026-10-24)

Two more, both found while fixing 1–3 above in 010-relay-concurrency-fixes, both measured rather than taken from the review:

Both are pre-existing and outside the diff of the branch that found them, which is why they are here rather than in that work.

Greptile static analysis on the 010 branch (2026-10-24)

Five findings, all outside #950–#952's diff, verified against the capture before filing:

Activity

  1. added
    rewritenostream 4.0 Rust/LMDB rewrite
    rustRust implementation work
    factoryBuilt by the software factory
    bugSomething isn't working
    on Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingfactoryBuilt by the software factoryrewritenostream 4.0 Rust/LMDB rewriterustRust implementation work

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions