Skip to content

Harden reply-topic creation against a hardcoded replicas=0#694

Open
wbarnha wants to merge 1 commit into
masterfrom
claude/fix-76-reply-topic-replicas
Open

Harden reply-topic creation against a hardcoded replicas=0#694
wbarnha wants to merge 1 commit into
masterfrom
claude/fix-76-reply-topic-replicas

Conversation

@wbarnha

@wbarnha wbarnha commented Jul 19, 2026

Copy link
Copy Markdown
Member

Note: Before submitting this pull request, please review our contributing guidelines.

Description

Relates to #76.

ReplyConsumer._reply_topic() (faust/agents/replies.py) — the ephemeral topic backing agent .ask()/reply_to — hardcoded replicas=0. #76 originally reported this crashing topic creation with InvalidReplicationFactorError on brokers that reject a replication factor of 0 (most do, via min.insync.replicas).

What I found investigating this

The acute crash from #76 no longer reproduces on current code, so I'm not claiming this closes #76 outright — worth verifying against a real broker before closing it. Topic.declare() (faust/topics.py) already has a fallback:

if self.replicas:
    replicas = self.replicas
else:
    replicas = self.app.conf.topic_replication_factor

0 is falsy in Python, so the hardcoded replicas=0 was already being silently replaced by the configured default (topic_replication_factor, default 1) before ever reaching the broker. I verified this directly: constructing the reply topic and tracing declare()'s branch logic resolves to replicas=1, not 0.

Why this PR is still worth doing

That's a coincidence of Python truthiness, not an intentional contract — nothing stops a future change to that fallback (e.g. distinguishing an explicit 0 from an unset value, which would be a reasonable thing to want) from silently reintroducing today's crash. It's also just confusing to read: the line says "0 replicas" and means "use the app default."

Fix

Pass app.conf.topic_replication_factor explicitly instead of the magic 0, consistent with how every other topic Faust creates picks up its replication factor. No longer relies on the fallback to mask the wrong value.

Tests

  • The existing test_reply_topic asserted the old, wrong value (topic.replicas == 0) — updated to assert the topic gets the app's configured replication factor (and that it's > 0).
  • Verified the updated assertion actually catches a regression: reverting the fix while keeping the test makes it fail with AssertionError: assert 0 == 1.
  • flake8, black --check, isort --check-only clean. Full tests/unit/agents suite passes (123 passed, 1 pre-existing skip unrelated to this change).

🤖 Generated with Claude Code


Generated by Claude Code

_reply_topic() (the ephemeral topic backing agent .ask()/reply_to) hardcoded
replicas=0. #76 reported this crashing topic creation with
InvalidReplicationFactorError on brokers that reject a replication factor
of 0 -- most do, via min.insync.replicas.

On current code the acute crash no longer reproduces: Topic.declare()
already falls back to app.conf.topic_replication_factor whenever
`self.replicas` is falsy (`if self.replicas: ... else: ...`), and 0 is
falsy in Python, so the hardcoded 0 was already being silently replaced by
the configured default (1) before reaching the broker. That's a coincidence
of Python truthiness, not an intentional contract -- nothing stops a future
change to that fallback (e.g. distinguishing an explicit 0 from an unset
value) from reintroducing today's crash.

Pass app.conf.topic_replication_factor explicitly instead of the magic 0,
consistent with how every other topic faust creates picks up its
replication factor. No longer relies on the fallback to mask the wrong
value.

The existing test_reply_topic asserted the old, wrong value
(topic.replicas == 0); updated to assert the topic gets a real,
broker-acceptable replication factor.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HHPL4VFWQRQPpjR1gXSKyL
@codecov

codecov Bot commented Jul 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.15%. Comparing base (3073eb9) to head (39c3a0a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #694   +/-   ##
=======================================
  Coverage   94.14%   94.15%           
=======================================
  Files         104      104           
  Lines       11136    11136           
  Branches     1201     1201           
=======================================
+ Hits        10484    10485    +1     
+ Misses        551      550    -1     
  Partials      101      101           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

InvalidReplicationFactorError is raised if reply_create_topic is set

1 participant