Skip to content

fix(relay): order the CLOSE-during-mint test by program order, not a sleep - #987

Open
chappie-daemon wants to merge 5 commits into
rewrite/mainfrom
factory/014-close-mint-sync
Open

chappie-daemon wants to merge 5 commits into
rewrite/mainfrom
factory/014-close-mint-sync

Conversation

@chappie-daemon

@chappie-daemon chappie-daemon commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

a_close_during_the_invite_mint_leaves_the_event_but_not_the_read synchronised a CLOSE against the invite mint with a fixed 50 ms sleep. Under load the CLOSE handler could be delayed past that sleep, the mint resumed first, and the test observed the stored EVENT and EOSE it asserts must not arrive — a false failure with a correct fix.

Closes #963.

The fix

The test moves down one level and is ordered by program order instead of by a timer:

  • it owns a Connection, a FrameSink and a context, per the family tests/connection.rs:414-424 established;
  • it spawns subscribe::begin (the seam fix(relay): run a handler's ordered prefix on the connection loop #978 introduced, whose prefix registers the read before returning) and calls the synchronous unsubscribe::handle in its own task;
  • it asserts !read_in_flight("invite") before gate.add_permits(1), so the abort is a program-order predecessor of the release — nothing between them can suspend;
  • then it drains one EVENT (kind 28935) and asserts the sink is empty.

sleep/Duration in the test body: 2 → 0.

What proves it still decides

The oracle is a mutation, not a pass rate. With abort_read moved after reading.await, the test fails at a closed subscription streams nothing and the leaked frame is ["EVENT","invite",{…"kind":1,…"content":"stored"…}] — #963's exact defect. Also measured: changing begin_read(&subscription_id) to begin_read("") fails the registration assertion, so that assertion can still catch a begin that stopped registering.

The 200 ms injection that proves the old test red cannot discriminate the new one — it never enters handlers::dispatch — and build.md says so rather than reporting a green as evidence.

What it costs

The wire-level combination this test used to cover — invite EVENT plus CLOSE mid-mint, over a socket — is now covered by nothing. That is #983, filed with the shape a future fix needs (the relay publishes its per-connection registry; a wire test polls it between the CLOSE and the release; production source, five Relay::new sites). Every other piece the test gave up is carried elsewhere, and the carriers are green at this base.

Two smaller deltas found while doing this

Verification

Base rewrite/main @ 1cb6265 (the branch was rebased onto it after #978 merged mid-review and broke the call site). cargo test --workspace --all-features and the release build both exit 0 at this head; handlers target 52 passed; 0 failed; all five repository commands clean; git merge-tree --write-tree produces a tree byte-identical to HEAD^{tree}.

Notes

@changeset-bot

changeset-bot Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fa1f09b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

…963)

The changeset claimed the test owns its socket state. It has not had one since the
rewrite: the test owns a Connection and a FrameSink, and subscribe::begin's prefix
registers the subscription and its read on the test's own task. That clause is what
tells a reader why the release no longer needs a timer.

Wording supplied by the operator: two rounds closed only half of the previous
finding, and a third lens pass over one sentence costs more than it decides.
@greptile-apps

greptile-apps Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The test-only change appears safe to merge.

Summary

Replaces the CLOSE-during-mint test's socket timing with direct handler calls.

  • The invite-mint test orders CLOSE before releasing the mint.

No actionable issues found. No production code changes.

chappie-daemon acknowledged the lost socket coverage (#983), deferred wording fixes (#984), and identified a pre-existing weakness in the neighboring test (#982).

Reviews (1) · Last reviewed commit: "docs(relay): say what the CLOSE test own..." · Reviewed by Greptile

This branch has not been deployed

No deployments
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.

1 participant