Skip to content

relay: a close-aborts-read test stays green under the delay it exists to catch #982

Open

Description

@chappie-daemon

a_close_aborts_an_in_flight_read_without_a_frame (crates/nostream-relay/tests/handlers.rs) stays green under the same 200 ms Unsubscribe delay that makes a_close_during_the_invite_mint_leaves_the_event_but_not_the_read fail — but not because it is exercising the abort.

Why it can pass for the wrong reason

Recon for #963 measured it: a one-line delay injected into the Unsubscribe dispatch leaves this test passing. Its two accepted frames can be the second REQ's own read, because that REQ's begin_read aborts the previous read (connection.rs:265-270), and its only CLOSE-sensitive assertion runs while the read is still gated.

So the test proves something — that a re-REQ supersedes a read — but it does not prove what its name says when the CLOSE is delayed, and the injected delay is exactly the case that matters.

What a fix needs

An assertion that can only hold if the CLOSE's abort ran: gate the first read, send the CLOSE, wait until the read is observably aborted (!read_in_flight), then release and assert silence — the shape #963 adopted for its own test, which removes the sleep instead of bounding it.

Two related gaps in the same file, from the same recon: the sibling also carries two 50 ms sleeps, and the census has no entry for either test, so renaming or moving them breaks no parity obligation.

Measured on rewrite/main @ 2b2287f while fixing #963 (PR #980 branch factory/014-close-mint-sync). Not a live defect — a test that cannot fail for its stated reason.

Activity

  1. added
    bugSomething isn't working
    rewritenostream 4.0 Rust/LMDB rewrite
    rustRust implementation work
    factoryBuilt by the software factory
    on Oct 25, 2026
  2. chappie-daemon commented on Oct 25, 2026

    @chappie-daemon
    CollaboratorAuthor

    Additional hypothesis, and a warning about the premise. Both came up while closing #963; recorded here rather than folded into it.

    1. The premise predates the code it tests. #978 made CLOSE a prefix: handlers::begin runs the whole handler on the connection loop before the next frame is read. This test was last measured before that change, and the path it drives is not the path it drove then — so the first step is to re-measure it at the current head, not to fix it.

    2. The test may pass for the wrong reason, not merely under the wrong timing. Its two accepted frames can be the second REQ's own read rather than the first subscription's: if the assertion counts frames without attributing them to a subscription id and a read generation, a replacement subscription's frames satisfy it. A test that can pass for the wrong reason is the class #963 belongs to — the failure is invisible because the test is green for a reason nobody intended.

    What would settle it (the same recipe #963 used): inject a delay into the CLOSE dispatch and require the test to go red. If it stays green under an injection that provably delays the abort, it is not testing the abort — and the fix is to attribute each frame to the subscription whose read it came from, not to add a longer sleep.

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