Skip to content

🐛 Fix signals.is() subscription race (#217)#218

Merged
taras merged 2 commits into
mainfrom
fix/signals-is-subscription-race
Jun 16, 2026
Merged

🐛 Fix signals.is() subscription race (#217)#218
taras merged 2 commits into
mainfrom
fix/signals-is-subscription-race

Conversation

@taras

@taras taras commented Jun 10, 2026

Copy link
Copy Markdown
Member

Motivation

is(signal, predicate) from @effectionx/signals could miss a matching state
change and hang forever. It checked the current value via valueOf() before
subscribing:

const result = predicate(stream.valueOf()); // 1. sync check
if (result) return;
for (const value of yield* each(stream)) {   // 2. subscribe (yields here)
  ...
}

Between the synchronous check and each(stream) establishing the subscription,
the operation yields. A producer that calls signal.set() to a matching value
in that gap sends to zero subscribers — the event is lost and is() waits
forever for a second update. This is a time-of-check/time-of-use race
(issue #217), and it forced producers to yield* sleep(0) before publishing,
contradicting the repo's no-sleep-test-sync policy that recommends is() as a
deterministic state waiter.

Approach

Establish the subscription before the decisive current-state check. is is
now a stateless function returning an Operation whose [Symbol.iterator]
subscribes first, then takes the valueOf() snapshot. With no yield between
subscribe and snapshot, a matching change during subscription setup is either
reflected in the snapshot or buffered in the subscription and consumed via
subscription.next(). Stream-close-before-match still returns cleanly.

  • signals/helpers.ts — subscribe-first is(). Public is() signature and the
    ValueSignal interface are unchanged; no replay added, so the fix stays
    independent of and compatible with the replay-ownership work in ♻️ Move replay semantics from signals to stream-helpers #213.
  • signals/helpers.test.ts — behavior coverage (already-matching;
    non-matching-then-matching) plus a is() can hang when publish lands between valueOf() check and each() subscription #217 regression that spawns a producer
    mutating the signal with no sleep, bounded by @effectionx/timebox purely
    as a diagnostic deadline.
  • signals/README.md and .policies/no-sleep-test-sync.md — describe is() as
    a deterministic state waiter that needs no producer sleep()/yield.
  • @effectionx/signals bumped 0.5.30.5.4; @effectionx/timebox added as
    a test-only dependency.

Verification

  • pnpm check, biome lint/format, pnpm sync — clean.
  • Signals suite 22/22 on Effection 4.0.2 (peer-range max-stable);
    reverting helpers.ts fails 3/4 (the race manifests on v4), proving the
    regression bites.
  • The is() behaviors also pass against Effection 3.0.0 (peer-range
    minimum). Note the race is v4-specific — v3 was already correct — so the fix
    corrects v4 without regressing v3, satisfying the declared ^3 || ^4 range.

Summary by CodeRabbit

  • New Features
    • Improved deterministic signal observation: the is helper now reliably completes based on the signal’s current value and subsequent matching updates without needing sleep/yield delays.
  • Bug Fixes
    • Prevents missed updates that can occur during subscription startup.
  • Documentation
    • Updated is helper and examples to reflect the deterministic behavior and remove unnecessary timing notes.
  • Tests
    • Expanded coverage for immediate satisfaction, later matching updates, array-driven changes, and subscription-timing regressions.
  • Chores
    • Version bumps: signals → 0.5.4, stream-helpers → 0.8.3, worker → 0.5.3.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6b940b8e-9b7b-4884-ab6a-fa0e7ae62410

📥 Commits

Reviewing files that changed from the base of the PR and between 20bb576 and c03bc7d.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (8)
  • .policies/no-sleep-test-sync.md
  • signals/README.md
  • signals/helpers.test.ts
  • signals/helpers.ts
  • signals/package.json
  • signals/tsconfig.json
  • stream-helpers/package.json
  • worker/package.json

📝 Walkthrough

Walkthrough

This PR fixes a race condition in the is() signal helper where matching updates could be lost during subscription initialization. The implementation now establishes a subscription before checking the current value, guaranteeing deterministic behavior without requiring sleep() calls in tests or examples.

Changes

is() helper deterministic synchronization

Layer / File(s) Summary
Core is() implementation rewrite
signals/helpers.ts
is() changed from generator function to Operation<void> with explicit iterator. New implementation subscribes before checking current value, then iterates updates until predicate matches, eliminating TOCTOU race condition. Imports adjusted to remove each helper dependency.
Test coverage and deterministic validation
signals/helpers.test.ts
Existing test updated to remove sleep(0) delay. Three new test cases added: immediate completion when current value matches, completion after first matching update following non-matching updates, and regression test verifying matching updates during subscription setup are not lost (with timebox timeout guarantee).
Documentation and policy updates
signals/README.md, .policies/no-sleep-test-sync.md
README now documents subscription-before-check behavior and guarantees matching updates are never lost. Policy documentation examples updated to remove sleep() calls and explain deterministic synchronization.
Package version and timebox dependency setup
signals/package.json, signals/tsconfig.json, stream-helpers/package.json, worker/package.json
@effectionx/signals version bumped 0.5.3 → 0.5.4. Added @effectionx/timebox workspace dev dependency and TypeScript project reference to support timeout-based regression test. Other packages versioned for consistency.

Sequence Diagram

sequenceDiagram
  participant Caller
  participant Operation as is() Operation
  participant Stream as ValueSignal
  participant Subscription
  Caller->>Operation: call is(stream, predicate)
  Operation->>Stream: yield* stream (subscribe)
  Stream-->>Subscription: subscription iterator
  Operation->>Stream: stream.valueOf() (check current)
  alt Current matches predicate
    Operation-->>Caller: return immediately
  else Current does not match
    loop Until match or completion
      Operation->>Subscription: subscription.next()
      Subscription-->>Operation: next value
      Operation->>Operation: check predicate(value)
    end
    alt Match found
      Operation-->>Caller: return
    else Stream completed
      Operation-->>Caller: return
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • cowboyd
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the primary fix: resolving a subscription race condition in signals.is() function.
Description check ✅ Passed The description is comprehensive and well-structured, covering motivation, approach, and verification against the required template.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Policy Compliance ✅ Passed PR passes all policy compliance checks: no AI marketing material in description or title (no-agent-marketing.md, Strict); all modified packages include required descriptions and valid keywords per...

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/signals-is-subscription-race

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread signals/helpers.ts
taras added a commit that referenced this pull request Jun 10, 2026
The swc-plugin-inline release build failed to link on current Rust:
`rust-lld: undefined symbol: __emit_diagnostics` (and the other SWC
host-ABI `__*_proxy` imports). Newer wasm-ld no longer leaves these
undefined symbols as imports by default, so the link needs
`-C link-arg=--allow-undefined`.

The cargo config that carries the plugin's rustflags lived at
inline/swc/.cargo/config.toml, but `build:bundle` runs cargo from inline/
(`--manifest-path swc/Cargo.toml`) and cargo discovers config relative to
the working directory — so those rustflags (including the pre-existing
`--cfg=swc_ast_unknown`) were silently ignored in CI. Move the config to
inline/.cargo/config.toml so it is actually applied, and add the
`--allow-undefined` link arg.

Unblocks CI on PR #218.
@taras taras force-pushed the fix/signals-is-subscription-race branch from 26fc16b to e6ce60c Compare June 10, 2026 16:16

@cowboyd cowboyd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎉

@pkg-pr-new

pkg-pr-new Bot commented Jun 10, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@effectionx/signals@218
npm i https://pkg.pr.new/@effectionx/stream-helpers@218
npm i https://pkg.pr.new/@effectionx/worker@218

commit: c03bc7d

Taras Mankovski added 2 commits June 16, 2026 12:17
is() checked the current value before subscribing, so a matching update
that landed in the gap was sent to zero subscribers and lost, hanging
until a second update. Establish the subscription first, then take the
valueOf() snapshot, so a change during subscription setup is either
reflected in the snapshot or buffered for consumption.

is() becomes a stateless function returning an Operation whose
[Symbol.iterator] subscribes before checking; the public signature and
ValueSignal interface are unchanged and no replay is added.

Add behavior and #217 regression coverage (bounded with @effectionx/timebox),
align the README and no-sleep policy with the deterministic guarantee, and
bump @effectionx/signals to 0.5.4.
stream-helpers and worker depend on @effectionx/signals at runtime via
workspace:* (replaced with the exact version on publish), so they need a
patch release to ship the is() subscription-race fix to their consumers.

- @effectionx/stream-helpers 0.8.2 -> 0.8.3
- @effectionx/worker 0.5.2 -> 0.5.3
@taras taras force-pushed the fix/signals-is-subscription-race branch from e6ce60c to c03bc7d Compare June 16, 2026 16:18
@taras taras merged commit 387f725 into main Jun 16, 2026
7 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.

4 participants