Skip to content

Commit cc6d3a3

Browse files
committed
fix(routing): tighten router contracts after review
Remove direct router access to broker private task storage and add an explicit broker-owned storage boundary for router-managed task registration. Clarify prepared route snapshots, subscription task-name semantics and setup-time router mutation contracts. Extend routing docs with flow resolution precedence and custom task base class middleware behavior. Add regression coverage for shared task local registration, custom base_cls middleware hooks and scheduler fallback when a route is removed before dispatch.
1 parent 99a34ca commit cc6d3a3

8 files changed

Lines changed: 115 additions & 10 deletions

File tree

docs/guide/routing-and-flows.md

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,16 @@ keep a snapshot of the route that was resolved at prepare time, so later changes
150150
to the mutable kicker object or router route table do not change that prepared
151151
send.
152152

153+
Flow selection is resolved in this order:
154+
155+
| Source | Effect |
156+
| --- | --- |
157+
| Explicit route | `.with_route(route)` uses that route's broker and flow. A later `.with_flow(flow)` updates the route flow, while a later `.with_broker(broker)` clears the explicit route. |
158+
| Explicit broker | `.with_broker(broker)` sends through that broker. If `.with_flow(flow)` is also set, that flow wins; otherwise Taskiq uses the same-broker task route flow, then the broker `default_flow`. |
159+
| Explicit flow | `.with_flow(flow)` without a broker override replaces the flow of the resolved route or broker default route. |
160+
| Router task route | `router.route_task(task, broker=..., flow=...)` is the default outbound route for that task. |
161+
| Broker default flow | `InMemoryBroker(..., default_flow=...)` and other brokers with `default_flow` provide the fallback flow for that broker. |
162+
153163
## Subscriptions
154164

155165
Routers also keep inbound flow subscriptions. Routing and subscribing are
@@ -179,6 +189,11 @@ The deprecated `route_task(..., subscribe=True)` shim still performs this
179189
subscription when a flow is resolved, but new code should call `subscribe()`
180190
directly.
181191

192+
`TaskiqSubscription.task_names` is diagnostic listen-plan metadata. It records
193+
which task names caused the broker to listen to a flow, but it does not make the
194+
flow an inbound task router. Workers still execute messages by
195+
`TaskiqMessage.task_name`.
196+
182197
Existing brokers can keep implementing `listen()` as before. New flow-aware
183198
brokers may use `get_subscribed_flows()` to subscribe to queues, topics,
184199
subjects or streams while the routing rules stay in the router.
@@ -187,6 +202,10 @@ subjects or streams while the routing rules stay in the router.
187202
subscriptions. It deduplicates by flow identity and checks broker options for
188203
conflicts.
189204

205+
Router routes and subscriptions are mutable setup-time configuration. They are
206+
not thread-safe runtime coordination primitives; configure them before starting
207+
concurrent worker or client activity.
208+
190209
## Scheduler and requeue
191210

192211
`ScheduledTask` remains a transport-neutral invocation payload. It stores task
@@ -277,6 +296,9 @@ When the final application registers this definition, Taskiq creates the bound
277296
task using `TracingTask`. If `base_cls` is not provided, Taskiq uses the native
278297
decorated task class.
279298

299+
Custom task classes do not bypass broker middleware. Send and execute lifecycle
300+
hooks are still owned by the selected broker and worker path.
301+
280302
For low-level integrations, `TaskDefinition.message(...)` builds a
281303
`TaskiqMessage` without binding the task. It uses the same argument and label
282304
preparation contract as a normal `.kicker().prepare(...)` invocation.

taskiq/abc/broker.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,19 @@ def bind_task_definition(
439439
register=register,
440440
)
441441

442+
def store_registered_task(
443+
self,
444+
task: AsyncTaskiqDecoratedTask[Any, Any],
445+
) -> None:
446+
"""
447+
Store a task after router-owned registration accepted it.
448+
449+
Application code should use `task` or `register_task`. This method is
450+
the broker/router integration boundary for cases where the router owns
451+
registration ordering and the broker owns local task storage.
452+
"""
453+
self._store_task(task.task_name, task)
454+
442455
def _decorate_task(
443456
self,
444457
func: Callable[_FuncParams, _ReturnType],

taskiq/kicker.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,14 @@ async def _legacy_kiq(
321321
def _prepare_route_snapshot(
322322
self,
323323
) -> tuple[AsyncBroker, TaskiqRoute | None, FlowProtocol | None]:
324-
"""Resolve the route that a prepared invocation must keep."""
324+
"""
325+
Resolve route state that a prepared invocation must keep.
326+
327+
The returned tuple is `(broker, route, explicit_flow_override)`.
328+
When a route is present, the route already carries its flow, so the
329+
separate flow override is `None`. A separate flow is returned only for
330+
legacy broker paths that cannot snapshot a `TaskiqRoute`.
331+
"""
325332
router = getattr(self.broker, "router", None)
326333
if not isinstance(router, TaskiqRouter):
327334
return self.broker, None, self.route_flow

taskiq/router.py

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -132,11 +132,7 @@ def register_task(
132132
broker=target_broker,
133133
flow=flow,
134134
)
135-
# Router and broker share this internal registration boundary.
136-
target_broker._store_task( # noqa: SLF001
137-
registered_task.task_name,
138-
registered_task,
139-
)
135+
target_broker.store_registered_task(registered_task)
140136
return registered_task
141137

142138
return self._register_bound_task(task, broker=broker, flow=flow)
@@ -178,7 +174,7 @@ def register(
178174
func: Callable[_FuncParams, _ReturnType],
179175
) -> AsyncTaskiqDecoratedTask[_FuncParams, _ReturnType]:
180176
target_broker = self._brokers.resolve(broker)
181-
real_task_name = task_name if not callable(task_name) else None
177+
real_task_name: str | None = None if callable(task_name) else task_name
182178
task = target_broker.task(task_name=real_task_name, **labels)(func)
183179
if flow is not None:
184180
self.route_task(task, broker=target_broker, flow=flow)

taskiq/routing/models.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,13 @@ def broker_name(self) -> str:
2626

2727
@dataclass(frozen=True, slots=True)
2828
class TaskiqSubscription:
29-
"""Inbound flow subscription owned by a router."""
29+
"""
30+
Inbound flow subscription owned by a router.
31+
32+
`task_names` is diagnostic listen-plan metadata. It records which task
33+
declarations caused a broker to listen to this flow; worker execution still
34+
resolves inbound messages by `TaskiqMessage.task_name`.
35+
"""
3036

3137
broker: AsyncBroker
3238
flow: FlowProtocol

taskiq/routing/subscriptions.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,12 @@
1515

1616

1717
class SubscriptionPlan:
18-
"""Inbound listen-plan subscriptions for flow-aware brokers."""
18+
"""
19+
Inbound listen-plan subscriptions for flow-aware brokers.
20+
21+
This is mutable setup-time configuration, not a thread-safe runtime
22+
coordination primitive.
23+
"""
1924

2025
def __init__(self, brokers: BrokerRegistry) -> None:
2126
self.brokers = brokers

tests/routing/test_shared_tasks.py

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import pytest
22

33
from taskiq import AsyncTaskiqDecoratedTask, Flow, task_builder
4-
from tests.routing.models import TracingTask
4+
from tests.routing.models import RecordingMiddleware, TracingTask
55
from tests.utils import RecordingBroker
66

77

@@ -41,6 +41,7 @@ async def shared_task() -> None:
4141

4242
assert source.sent == []
4343
assert target.find_task("shared.routed") is registered
44+
assert target.local_task_registry["shared.routed"] is registered
4445
assert target.sent[0][0].task_name == "shared.routed"
4546
assert target.sent[0][1] == flow
4647

@@ -64,6 +65,27 @@ async def traced(value: int) -> int:
6465
assert broker.sent[0][0].task_name == "shared.traced"
6566

6667

68+
async def test_task_builder_custom_base_cls_uses_broker_middleware() -> None:
69+
broker = RecordingBroker()
70+
events: list[tuple[str, str, str]] = []
71+
broker.add_middlewares(RecordingMiddleware("broker", events))
72+
73+
@task_builder("shared.traced.middleware", base_cls=TracingTask)
74+
async def traced(value: int) -> int:
75+
return value + 1
76+
77+
registered = broker.register_task(traced)
78+
79+
assert isinstance(registered, TracingTask)
80+
81+
await registered.kiq(1)
82+
83+
assert events == [
84+
("broker", "pre_send", "shared.traced.middleware"),
85+
("broker", "post_send", "shared.traced.middleware"),
86+
]
87+
88+
6789
def test_task_definition_default_flow_does_not_create_subscription() -> None:
6890
flow = Flow("shared.default")
6991
broker = RecordingBroker(default_flow=flow)

tests/scheduler/test_scheduler.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,40 @@ async def test_scheduler_without_route_uses_scheduler_broker() -> None:
114114
assert scheduled_task.labels == {"source": "database"}
115115

116116

117+
async def test_scheduler_removed_route_uses_scheduler_broker() -> None:
118+
router = TaskiqRouter()
119+
scheduler_flow = Flow("scheduler.default")
120+
scheduler_broker = RecordingBroker(
121+
router=router,
122+
broker_name="scheduler",
123+
default_flow=scheduler_flow,
124+
)
125+
target_broker = RecordingBroker(router=router, broker_name="target")
126+
source = RecordingScheduleSource()
127+
scheduler = TaskiqScheduler(broker=scheduler_broker, sources=[source])
128+
129+
@scheduler_broker.task(task_name="demo.task")
130+
async def demo_task() -> None:
131+
return None
132+
133+
router.route_task(demo_task, broker=target_broker, flow=Flow("target"))
134+
scheduled_task = ScheduledTask(
135+
task_name="demo.task",
136+
labels={},
137+
args=[],
138+
kwargs={},
139+
cron="* * * * *",
140+
)
141+
del router.routes["demo.task"]
142+
143+
await scheduler.on_ready(source, scheduled_task)
144+
145+
sent_message, sent_flow = scheduler_broker.sent[0]
146+
assert target_broker.sent == []
147+
assert sent_message.task_name == "demo.task"
148+
assert sent_flow == scheduler_flow
149+
150+
117151
async def test_created_schedule_kiq_does_not_mutate_schedule_payload() -> None:
118152
router = TaskiqRouter()
119153
source_broker = RecordingBroker(router=router, broker_name="source")

0 commit comments

Comments
 (0)