Skip to content

[Brook] Removed the auto close from the Stores, the service and the protos. - #41

Closed
moetemp wants to merge 17 commits into
moe/brook/v4/s22-client-objectsfrom
moe/brook/v5/s23-no-auto-close
Closed

moetemp wants to merge 17 commits into
moe/brook/v4/s22-client-objectsfrom
moe/brook/v5/s23-no-auto-close

Conversation

@moetemp

@moetemp moetemp commented Oct 24, 2026 •

Copy link
Copy Markdown
Owner

This PR removes the auto close from the Streams crate and fixes the crate findings of an independent review.

The SDK review made the Stream lifecycle a resource manager. The owner closes a Stream explicitly or at the end of an async with block. So the Store needs no idle timer, and no layer closes a Stream on its own.

What changed?

Auto close.

  • lua/auto_close.lua and the auto_close() helper are gone. Create, append and close check closed directly. A create on a closed Stream still returns closed, an append is refused, and a close returns already closed.
  • A repeat create is idempotent on Retention only, and create.lua takes one ARGV less.
  • Capabilities::AUTO_CLOSE, Store::auto_close and the auto_close parameter of Store::create are gone.
  • The proto reserves CreateStreamRequest field 5 and DescribeStreamResponse field 4. close_with_run = 7 stays for native.
  • docs/redis.md drops the ac and la meta fields.

Review fixes.

  • Close owner. On a Workflow-owned Stream, only the close Activity's Workflow Producer (wf:<run id>) can close. A Local Activity runs before its Workflow Task commits, so its close is refused. A close whose ref has no run chain is refused too. AppendRecords accepts withdraw_run only from a later run's Local Activity.
  • Reads. A filtered read scans past the scan cap with no wait, and reads the log in chunks of 1,000 entries. Each chunk checks the trim watermark. A read of an expired Stream fails with stream_expired, also when the Stream is closed.
  • Writers. An Activity retry opens its writer on a closed or expired Stream, so the first write fails with the real reason. The dedupe entry stores a batch sequence as sent, up to 2^53 - 1. A larger sequence is refused.
  • Redis ACLs. docs/redis.md "Access" lists the commands of each role. A Reader needs read and write key access (~), not %R~, since redis-rs sends EVALSHA. RedisStore::connect skips the version check when INFO returns NOPERM.
  • Vectors. testdata/withdrawals.json has new cases with no epoch and cases of Local Activity Producers.
  • Text. The WIT note on an empty stream-id applies to native only. "plugin" is "backend" in extension.rs, ids.rs and extension.proto. The comments use literal wording and keep only the why.

The PR also pins sdk-rust Brook c8 f7826366.

Please note that a filtered last read still stops after the scan cap. It can return fewer Records than asked when the matches are far back. The WIT close still has previous-completed-task and failed-tasks, which the code doesn't read.

Why?

The review found that a Local Activity could close a Workflow's Stream before its Workflow Task committed. If that task failed, the Stream stayed closed, and the retried task's write failed the run. The other fixes are for reads that stalled or waited on an expired Stream, ACL docs that didn't work, and gaps in the shared vectors.

How did you test it?

Each code fix has a test that fails without it. tests/acl.rs runs create, append, close and read as a Producer user and a Reader user with only the documented commands. cargo test --workspace passes on memory, and on Redis 8.10.1 standalone and as a cluster. cargo fmt --check and clippy with -D warnings pass. The sdk-streams fork CI passes on the head. It runs the unit and vector tests, the Redis tests, fmt, clippy, the API proto copy check and the FIPS build.

The superproject branch temporalio/internal-ai-prototypes 2026/10/ai198-streams pins ee6d1e51, the tested commit. The head commit after it changes only comments and docs, so the pin doesn't change.

  • Unit Tests
  • Staging
  • End to End Tests

A Stream closes only on a close call. Each script checks `closed` where the auto close step did,
so a create on a closed Stream still answers closed, an append is refused and a close answers
already.
A read writes nothing again, so a Reader needs read access only.
The revert touches only sdk-core, which the crate doesn't use.
A Local Activity writes before its Workflow Task commits, so its close could outlive a failed task and fail the retried task's create.
A filtered read waited for a new append though entries up to the newest one were never scanned, and max_records 1 made one Store call per entry.
The withdraw-only batch failed on such a Stream, so a retry that only reads it failed for good before user code ran.
A user with the documented ACLs couldn't connect or run the scripts. A test now runs each role with exactly the listed commands.
Redis and memory refuse an empty id. Only the native frontend assigns one.
The new c8 commits touch only the Core CHANGELOG and Core tests, which the crate doesn't use.
@moetemp

moetemp commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Closed in favor of the review series. The crate is K1 to K10, #43 to #52, cut fresh onto upstream main (plan: Brook: code review and upstream plan). The branch stays, since the superproject pins it and Jetty's #42 builds on it.

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