Skip to content

[Brook] Ported the harness to the reviewed Streams API. - #7

Closed
moetemp wants to merge 22 commits into
moe/brook/v3/h1-streamsfrom
moe/brook/v5/h2-reviewed-api
Closed

moetemp wants to merge 22 commits into
moe/brook/v3/h1-streamsfrom
moe/brook/v5/h2-reviewed-api

Conversation

@moetemp

@moetemp moetemp commented Oct 24, 2026 •

Copy link
Copy Markdown
Owner

This PR ports the harness to the reviewed Streams API of sdk-python p35 and fixes the harness findings of an independent review.

p35 renamed the Streams API, moved it into temporalio.common, and made each process register its Stream backend on the client. The harness uses Streams for agent events, so every Stream import and call site changes.

What changed?

Port.

  • The Stream names come from temporalio.common, such as common.defn, common.topic, common.StreamBackend and ActivityStream. Redis comes from temporalio.contrib.stream.
  • stream_backends_from_env() replaces streams_from_env(), and each process registers it with stream_backends=. The env backend has default=True.
  • client.stream(...) replaces StreamClient(client).get_handle(...). Reads use offset= and topic_prefixes=.
  • The docs no longer say that the Stream closes with the run chain. A run end closes nothing, and the 5 s liveness check ends the reads.
  • The Nexus agent contract and its generated Python and Go files name temporalio.common. They're regenerated with nexgen v0.2.2, the version of the checked-in files, so only the descriptions change. The Go and TypeScript comments name it too. The generate-from-local target of the ui_connector Makefile uses the contract under python/.
  • The tests that run on a released temporalio check for the Streams API with hasattr(temporalio.common, "ActivityStream"), since the names load on first use.
  • The justfile pins p35 415fb36a.

Review fixes.

  • Public SDK calls. The root Stream handle comes from WorkflowHandle.stream, not from the private _ids module. So its place names the default backend of the client, the same as the agent's own AgentStream. ContribSink._write writes each event with the public topic(name, type=AgentEvent).publish(event), not the private _append. Its done callback gets the error of a refused write, so only that event is lost and the run doesn't fail at its end.
  • Reads. An SSE read continues through UNAVAILABLE, DEADLINE_EXCEEDED, RESOURCE_EXHAUSTED and ABORTED from describe. NOT_FOUND still ends it, and other errors raise.
  • Docs. Retention is a record's age, also on an open Stream. The README says that a store outage delays events and the run end. The contract and the README say that Nexus pollMessages gives offset=0 on a Stream. So a caller can't apply attempt_superseded, and a retried model call shows the text of both attempts. The stream_merge docs name attach(resume=...). The comments use literal wording.

Known limits.

  • The contract change that gives pollMessages the Record offsets is deferred.

Why?

p35 changed the API, so the harness had to change with it. The independent review of Brook and Jetty also found private SDK calls on the default write path and an SSE read that ended on a transient error. Some docs didn't match the code.

How did you test it?

On the pinned p35 415fb36a, the memory lane and the Redis lane pass. A new test checks that a refused write loses its event and doesn't fail the run. The read test checks that UNAVAILABLE doesn't end a read and PERMISSION_DENIED raises, and it fails without the fix. go test ./agent/... and go vet in nexus/ui_connector pass. bazel test //typescript/packages/client/... //typescript/apps/ui/... passes. The tests for a released temporalio pass against the release. The fork has no CI for this branch.

The superproject branch temporalio/internal-ai-prototypes 2026/10/ai198-streams pins the head, 909bdec4.

  • Unit Tests
  • Staging
  • End to End Tests

The SDK moved the API to `temporalio.common.streams` and the Redis and memory
backends to `temporalio.contrib.stream`. `stream_backends_from_env` gives the backend
with `default=True`, since `Client.stream` reads a handle with no backend on it.
A release has no `stream_backends=`, so `stream_backend_options` passes the option
only when `STREAMS_STORE` is set. The docstrings name `temporalio.common.streams`.
It adds only a docstring to 74b759cf, so the lanes that ran on 74b759cf stand: memory
925 passed, Redis 923 passed, conformance 54 on each.
Regenerated with nexgen v0.2.2, the version in the generated headers. The Python models
keep two older docstring wraps that v0.2.2 does not make, so the diff is the four fields.
An SDK rename of the private id helper would break every agent's Stream with no type error. The write path keeps _append, since no public call gives the write's future.
One UNAVAILABLE in a quiet stretch ended the attach, though the agent still ran.
Each StreamItem has offset 0 there. The contract keeps its shape, and the generated files change only in the field's comment (nexgen v0.2.2).
p35 moved the Streams API out of temporalio.common.streams and loads its names on first use. So the tests check for the names with hasattr, and the fake release hides temporalio.common._streams.
publish now starts the write and returns its future, so the private _append call goes. The done callback takes a refused write's error, so the run doesn't fail at its end.
@moetemp

moetemp commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Closed in favor of the review series. The net change of #6 and this PR is H1, #8, cut fresh onto upstream main (plan: Brook: code review and upstream plan). The branch stays, since the superproject pins it.

@moetemp moetemp closed this Oct 25, 2026
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