Skip to content

test(pubsub): add regression coverage for subscribe race edges#609

Merged
marcus-pousette merged 3 commits intodao-xyz:masterfrom
Faolain:test/pr596-regression-tests
Feb 28, 2026
Merged

test(pubsub): add regression coverage for subscribe race edges#609
marcus-pousette merged 3 commits intodao-xyz:masterfrom
Faolain:test/pr596-regression-tests

Conversation

@Faolain
Copy link
Contributor

@Faolain Faolain commented Feb 26, 2026

Summary

This PR adds focused pubsub regression coverage based on historical issues discussed in #596.

master already has coverage for:

  • immediate pending-subscribe tracking / local cleanup
  • strict direct delivery while subscribe is pending

But it did not have explicit regression tests for a few race/design edges that #596 attempted to protect.

What this adds

New file:

  • packages/transport/pubsub/test/subscribe-races.spec.ts

New regression tests:

  1. concurrent subscribe + connect
  • Verifies peers still discover each other when subscribe() and connect() happen concurrently.
  1. non-subscriber topic tracking guard
  • Verifies a peer that never subscribed does not start tracking a topic just from incoming subscription traffic.
  1. cancelled pending subscription is not advertised
  • Verifies subscribe() followed by unsubscribe() within the debounce window does not leave stale remote subscriber state.

Why these tests are useful

These are race-sensitive behaviors that are easy to regress during refactors around:

  • subscription debounce/pending state handling
  • topic map lifecycle
  • fanout control-plane synchronization during connect/subscribe interleaving

They complement existing fanout-topics.spec.ts checks by asserting network-visible behavior (peer discovery / non-discovery), not only local state transitions.

Notes from PR #596 analysis

Verification

Run:

node ./node_modules/aegir/src/index.js run test --roots ./packages/transport/pubsub -- -t node

Result locally: passing (53 passing).

Looped regression test 50 times and it passes every time.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8aea70e95a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@marcus-pousette
Copy link
Member

Thanks for this, the direction is good and the added race coverage is valuable

I have two test-hardening requests before merge:

  1. packages/transport/pubsub/test/subscribe-races.spec.ts:95
    The delay(250) + negative assertion can false-pass on slow/partial propagation. Please anchor this test on an observed subscribe/control-plane event (or a waitForResolved condition that proves B actually reached subscribed state) before asserting A does not track the topic.

  2. packages/transport/pubsub/test/subscribe-races.spec.ts:133
    The if (bTopics) branch makes the key assertion optional. Please make it strict:

    • assert bTopics exists
    • assert bTopics does not contain a.publicKeyHash

With those tightened, I’m +1 to merge.

@Faolain
Copy link
Contributor Author

Faolain commented Feb 27, 2026

Updated! Also I checked off to allow maintainers edit access so feel free to modify my commit as you wish if it's faster!

Copy link
Member

@marcus-pousette marcus-pousette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

Copy link
Member

@marcus-pousette marcus-pousette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed: regression coverage is valuable and aligns with current pubsub behavior. Approving.

@marcus-pousette marcus-pousette merged commit 1929680 into dao-xyz:master Feb 28, 2026
6 checks passed
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.

2 participants