Skip to content

fix(phx_channel): buffer pushes that arrive before on() is registered - #2

Merged
calvin-archastro merged 2 commits into
mainfrom
fix/channel-push-handler-race
Apr 17, 2026
Merged

fix(phx_channel): buffer pushes that arrive before on() is registered#2
calvin-archastro merged 2 commits into
mainfrom
fix/channel-push-handler-race

Conversation

@calvin-archastro

Copy link
Copy Markdown
Contributor

Summary

The first main-branch CI run after PR #1 merged was red with 2/641 tests failing on Python 3.11 and 1/641 on 3.12 — all in the generated on_<event>_delivers_contract_valid_payloads tests, all with TimeoutError. Same code passed on the PR run immediately prior, so the tests are flaky, not broken. Root cause is a real race condition.

Root cause

The generated channel contract tests do:

```python
await client.register_scenario({
"topic": ...,
"onJoin": [{"type": "autoReply"}, {"type": "autoPush", "event": E}],
})
channel = await Channel.join_x(socket, ...) # (1)
channel.on(E, handler) # (2)
await asyncio.wait_for(future, timeout=1.0)
```

The server's onJoin scenario fires the join reply and the autoPush back-to-back. The client's join future resolves at (1); (2) runs next. On slower runners the autoPush frame is already on the asyncio queue and gets dispatched by Channel._on_message before (2) registers the handler — so the push hits an empty handler list and is silently dropped. Local macOS is fast enough to dodge it; GHA runners aren't.

Fix

If a user-event push arrives with no handlers yet, buffer it in _pending_pushes[event]. When on(event, cb) is later called, replay any buffered payloads to the callback in arrival order. Bounded at 32 events/type so orphaned topics don't leak unbounded memory.

This matches how Phoenix clients in other languages avoid the race — they register all handlers on the channel before calling join(). Our generated Python API conflates create+join into a single Channel.join_x() class method, so post-join handler registration inherits the race unless the client buffers.

Scope / risk

Low. Pure runtime change in src/phx_channel/channel.py — 2 new small dicts + a replay path in on(). The existing hot path (handler present) is unchanged. Two regression tests added:

  • test_pushes_before_handler_are_replayed_on_registration
  • test_pending_push_buffer_is_bounded

Local verification

  • uv run pytest src/phx_channel/tests/test_unit.py → 35 pass (33 original + 2 new)
  • ARCHASTRO_RUN_CHANNEL_CONTRACT_TESTS=1 uv run pytest tests/contract → 641 pass, no flakes across repeated runs
  • uv run ruff check && uv run ruff format --check → clean

Follow-ups

  • The rename to archastro-sdk + version reset to 0.1.0 is still on feat/initial-setup — didn't get picked up by the PR feat: initial Python SDK workspace #1 merge. I'll open a separate PR rebasing it onto main after this lands.
  • Long-term: the generator could emit a two-step API (`channel = Channel(...); channel.on(...); await channel.join()`) to mirror Phoenix conventions and obviate the need for a buffer, but the buffer is a strict improvement even with such an API (defensive against any future dispatch-order quirks).

🤖 Generated with Claude Code

The first CI run on main failed flakily (2/641 tests on Python 3.11,
1/641 on 3.12) on "on_<event>_delivers_contract_valid_payloads" tests:

  await client.register_scenario({
      "topic": ...,
      "onJoin": [{"type": "autoReply"}, {"type": "autoPush", "event": E}],
  })
  channel = await Channel.join_x(socket, ...)       # (1)
  channel.on(E, handler)                            # (2)
  await asyncio.wait_for(future, timeout=1.0)

The server's onJoin scenario fires the join reply AND the autoPush
back-to-back. The client's join future resolves at (1), then (2) runs,
but on slower runners the autoPush frame can already be on the asyncio
queue and dispatched before (2) registers the handler — so the push
hits Channel._dispatch_event with an empty handler list and gets
silently dropped. Local macOS was fast enough to dodge this; GHA
runners aren't.

Fix: if a user-event push arrives with no handlers yet, buffer it in
`_pending_pushes[event]`. When `on(event, cb)` is later called, replay
any buffered payloads into the callback in arrival order. Bounded at
32 events/type to keep orphaned topics from leaking unbounded memory.

Matches Phoenix-client behavior in other languages where the handler
is always registered before `join()` — our generated Python API
conflates create+join into a single `.join_x()` class method, so
without buffering, post-join handler registration inherits the race.

Added two unit tests covering replay order + bounded buffer. Full
contract suite (641 tests) passes locally across 10 consecutive runs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
if len(buf) < self._pending_pushes_cap:
buf.append(payload)
else:
# Drop the oldest to bound memory for orphan events.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

_pending_pushes_cap is an unguarded mutable int — set it to 0 and len(buf) < 0 is always False, so the else branch fires immediately, calling buf.pop(0) on an empty list → IndexError propagating out of _on_message and killing the receive loop. Fix: use collections.deque(maxlen=self._pending_pushes_cap) which is also O(1) eviction instead of O(n) pop(0), and validate the cap is >= 1 in __init__ (or just hardcode the constant — it isn't a tunable anyone should be touching).

buf.append(payload)
else:
# Drop the oldest to bound memory for orphan events.
buf.pop(0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Silent drop — when the buffer is full, the oldest payload is discarded with no observable signal; a user seeing mysteriously missing events will have zero clue why. Add logger.warning("Pending push buffer full for %s:%s — dropping oldest payload", self._topic, event) before the evict-and-append.

"""
handlers = self._event_handlers.setdefault(event, [])
handlers.append(callback)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Buffer replay is cleared on the first on() registration — a second ch.on("evt", cb2) gets nothing from the buffer even though _on_message fans out to all handlers once any are registered. So cb1 gets buffered payloads and cb2 silently doesn't, which is asymmetric with live-delivery behavior. Either fan-out replay to all current registrations at the time on() is called (iterate all handlers in the list, not just callback), or document that _pending_pushes is drained by the first subscriber and intentionally not delivered to late ones.

CI flagged the previous commit's edit for formatting; run ruff format
so the check passes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@calvin-archastro
calvin-archastro merged commit dd5ac6b into main Apr 17, 2026
2 checks passed
calvin-archastro added a commit that referenced this pull request Apr 17, 2026
Fresh-start rename — ship as archastro-sdk 0.1.0 rather than inheriting
archastro-platform-sdk 0.77.0 from the firstlanding import. These
changes were on feat/initial-setup but got left behind when PR #1
merged; rebasing onto main now that PR #2's flake fix is in.

- pyproject.toml: name → archastro-sdk, version → 0.1.0
- scripts/sdk-generator-config.json: matches
- README: pip install archastro-sdk
- src/archastro/platform/__init__.py: patched to look up
  \"archastro-sdk\" via importlib.metadata. This file is
  auto-generated; the generator fix that will make this automatic is
  ArchAstro/archastro-openapi#4 — once that ships in
  @archastro/sdk-generator@0.1.1+, future regens produce the right
  output without patching.
- uv.lock: regenerated

The Python import path (`archastro`) is unchanged — only the
pip-install name and the __version__ lookup key change.

Local verification
  uv run python -c \"import archastro.platform; print(archastro.platform.__version__)\"
  → 0.1.0

Test suite: 51 non-contract tests + 641 contract tests (incl. channel
suites over a real @archastro/channel-harness subprocess) all pass;
ruff check + format clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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