fix(act): grant a zero-length lease a holder, bound reads before emitting - #1613
Open
Rotorsoft wants to merge 1 commit into
Open
fix(act): grant a zero-length lease a holder, bound reads before emitting#1613Rotorsoft wants to merge 1 commit into
Rotorsoft wants to merge 1 commit into
Conversation
…ting Three InMemoryStore divergences from both SQL adapters, all a bound applied after the effect instead of before it. `lease` recorded the holder only for a positive duration, so a zero-length lease came back to the caller with nobody holding the stream — and `ack` is gated on the holder, so every ack was dropped, the watermark never advanced, and every event was redelivered on every drain. `leaseMillis: 0` is legal config and the drain passes it straight through (`??` does not coalesce 0), so this is reachable from type-checked source. The holder is now recorded whatever the duration; `millis` governs only the expiry, matching PG and SQLite. `query_streams` and `query_stats` counted a row and then checked the limit, so `limit: 0` returned exactly one row where both SQL adapters return none. The bound now runs before the row is emitted. Three TCK cases pin all of it, each with a control that exercises the same path with an ordinary argument. Red against the unmodified InMemoryStore, green against PostgresStore and SqliteStore unchanged. Closes #1600 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015xAdM431gFeFBZTW5kRrjg
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1600. Branched from master, so it can merge independently of #1611 / #1612 — it touches
store-tck.tsandports.tsin different regions than #1612 does.Three places where the in-memory store disagreed with both SQL adapters, all the same mistake: a bound applied after the thing it was meant to bound. The framework never passes these arguments, which is why they drifted — and why the compatibility kit, written from the framework's habits, never caught it.
The one that actually bites
Leasing a stream takes a duration, and the in-memory store recorded the lease holder only when that duration was positive. Both SQL adapters record the holder unconditionally and let the duration decide only the expiry — which is the reading that matches how acknowledging works, since
ackis gated on being the holder. Grant a lease with no holder recorded and the caller still gets a lease, processes the events, acknowledges them, and the acknowledgement is dropped because nobody holds the stream. The watermark never moves and the next drain redelivers the same events. Forever, with no error anywhere.It is reachable from fully type-checked source:
leaseMillisaccepts0, and the drain passes it through with??, which does not coalesce zero. At app level that reads asfirst_ran: 1, total_ran: 3with the watermark stuck at-1. Severity stays low because the in-memory store is dev/test-only by design — which is also exactly where a person meets this without a production adapter to compare against.The two smaller ones
query_streamsandquery_statscounted a row and then checked the limit, solimit: 0returned exactly one row where both SQL adapters return none.What we checked and left alone
query({limit: 0})looked like the obvious fourth member of the family. It isn't — Postgres and the in-memory store both treat a zero limit as unbounded and returned the same two rows in a differential probe, so there is nothing to reconcile.limit: -1is a genuine three-way disagreement (in-memory returns 1, SQLite returns all, Postgres throws a raw driver error), and this PR deliberately leaves it. It belongs to the nonsense-input family already recorded in #1199; writing a contract for it would be specifying behavior for a call nobody should make. The distinction we're drawing: a boundary value that legal configuration can produce earns a rule, an input that is simply wrong does not.Cost
No behavior change for any caller that passes ordinary arguments, and no change at all to Postgres or SQLite. A caller relying on the in-memory store returning one row for
limit: 0would see zero — that is the divergence being removed, not a new risk.Tests
Three TCK cases, each with a control that runs the same path with an ordinary argument so the boundary is the only variable: a zero-length lease whose
acklands and advances the watermark, andlimit: 0on both enumerating reads. All three were red against the unmodifiedInMemoryStoreand green againstPostgresStore,SqliteStore, and act-notify'swithBroker(PostgresStore)with no adapter changes — the outlier is named by the test.Docs: the
millisparameter onStore.claimnow says it bounds the expiry only and that the holder is recorded regardless; bothlimitdoc-comments say the bound is applied before a row is emitted. Two new rows inbehavior-contracts.md. Narrative inbook/1600-the-bound-goes-before-the-effect.md.Test plan
pnpm test— 238 files, 3643 passed, 54 skippedwithBroker(PostgresStore)pnpm typecheckpnpm lintStability charter impact
None functionally: no signature, return shape, or method changed. The in-memory adapter now matches the contract the SQL adapters already implemented, so this narrows an implementation divergence rather than moving the contract. The two doc-comments in
libs/act/src/types/ports.tswrite down what the contract already required.rfc-gate: exempt — the snapshot grew from doc-comment and implementation text in
ports.ts,in-memory-store.ts, andstore-tck.ts. No public export, builder method, port method, lifecycle event, or exported-type field was added.Follow-ups
limit: -1stays open as awareness-only under #1199, per the reasoning above.🤖 Generated with Claude Code
https://claude.ai/code/session_015xAdM431gFeFBZTW5kRrjg