Skip to content

fix(act): keep a stream's lane when the subscriber has forgotten it - #1612

Open
Rotorsoft wants to merge 1 commit into
act-1598-dynamic-lane-default-normalizationfrom
act-1599-eviction-relanes-dynamic-target
Open

fix(act): keep a stream's lane when the subscriber has forgotten it#1612
Rotorsoft wants to merge 1 commit into
act-1598-dynamic-lane-default-normalizationfrom
act-1599-eviction-relanes-dynamic-target

Conversation

@Rotorsoft

Copy link
Copy Markdown
Owner

Closes #1599. Stacked on #1611 (base is that branch, so the diff here is only this fix; GitHub retargets to master when #1611 merges).

A stream could silently move to the wrong drain lane and stop being processed. When two reactions point at the same target and one outranks the other, the winner's lane is the one the stream should keep. The part of the framework that remembers which target is on which lane has a size limit, and once it forgets a target, the losing reaction was treated as the first one ever seen and got to set the lane. A worker deployed to serve only the winner's lane then stops picking that stream up, and the work on it quietly stops. Nothing errors, and the priority column still looks right.

What we found that the ticket didn't say

Eviction is the smaller half. A missing record reads as never-seen, and a restart misses everything — a fresh process starts with an empty map while every subscription row persists in the database. So the same re-laning happens on the first low-priority resolution after any deploy, with no memory pressure involved, on an application whose maxSubscribedStreams is generous enough that eviction never occurs. A fix scoped to eviction would have closed the narrow half and left the half that fires on every deploy.

That reframing is what picked the fix. Reading the row back before deciding whether a target is new covers both halves, but the store's stream filter has no way to ask for a named set of streams (the portable filter grammar is anchors, dots and literals — no alternation), so a read-back costs one round trip per unknown target, and under the access pattern that overflows the map every scan is full of unknown targets. Reporting the drift instead — which is what the sibling tickets in this family chose — isn't implementable here: the thing that would notice is the record, and the record is what went missing.

The fix

The lane now rides the priority merge in subscribe, exactly as priority already does: a subscribe whose priority is at or above the stored priority sets the lane; one below leaves the lane alone. The highest priority registered for a stream owns its lane, durably, and the bounded map goes back to being the optimization its own comment always claimed it was.

Equal priority still writes the lane, and that is deliberate rather than a compromise: moving a stream between lanes is done by editing the declaration and restarting, and a restart re-subscribes at the same declared priority. A strict-increase rule would freeze every lane assignment at whatever it was first given and turn a config edit into a data migration.

Cost

Adapter authors: this is a Store contract change. All three in-tree adapters (InMemory, Postgres, SQLite) implement it in this PR, and four new cases in the store TCK pin it. A third-party Store adapter keeps working but fails those TCK cases until it adopts the rule — which is the signal an executable contract exists to send.

We are shipping it as a fix rather than a major. The charter's letter says changed method semantics are breaking, but the behavior-contract row for #1363 already stated that the highest-priority resolution owns a target's lane; the store simply stopped enforcing that once the orchestrator forgot the record. No application code changes, and the invariant users were told about is the one that now holds.

One corner follows from the rule and is documented rather than fixed: a stream whose stored priority was raised out of band with prioritize() keeps its lane until a subscribe reaches that priority. Closing it would need a flag on the write to mark it authoritative — more public surface than the corner is worth.

Tests

Four TCK cases in the lanes block (a lower-priority subscribe keeps the lane; a mark-only subscribe keeps the lane; an outranking subscribe re-lanes; equal priority re-lanes, which is what keeps re-laning restart-driven). The first two were confirmed red against all three unmodified adapters before the fix.

correlate-lane.spec.ts gains an orchestrator-level block that pins what an adapter test cannot see: with maxSubscribedStreams: 1 the target keeps its lane, under onlyLanes the reaction on it still runs (three of those cases were red before the fix), and a second Act over the same store — the restart case — does not re-lane a target it never recorded. Controls at a roomy bound accompany each.

Docs updated in the same pass: configuration.md § Lanes and maxSubscribedStreams, correlation-and-drain.md (eviction cost), priority-lanes.md, extension-points.md, writing-a-store.md, the SubscribeInput.lane doc-comment, the CLAUDE.md lane one-liner, and a new row in behavior-contracts.md. Narrative in book/1599-forgetting-is-not-a-fact.md.

Test plan

  • pnpm test — 238 files, 3660 passed, 54 skipped
  • Coverage: 100% statements / 100% branches / 100% functions / 100% lines.
  • Store TCK green on InMemory, Postgres, SQLite, and act-notify's withBroker(PostgresStore)
  • New tests verified red before the fix (TCK on all three adapters; 3 of 5 orchestrator cases)
  • pnpm typecheck
  • pnpm lint
  • CI green
  • Review

Stability charter impact

Charter-covered: Store.subscribe behavior and the SubscribeInput.lane doc-comment in libs/act/src/types/ports.ts. No signature, no return shape, no new or removed method — the write rule for one column changed, from unconditional to riding the priority max. Shipping as a fix per the reasoning under Cost above; the TCK is the enforcement mechanism for adapters.

rfc-gate: exempt — the snapshot grew from doc-comment and implementation text in ports.ts, the three adapters, and store-tck.ts. No public export, builder method, port method, lifecycle event, or exported-type field was added.

Follow-ups

None parked.

🤖 Generated with Claude Code

https://claude.ai/code/session_015xAdM431gFeFBZTW5kRrjg

A dynamic target's record lives in a bounded LRU, and a missing record
reads as never-seen — so a lower-priority resolution won a lane it had
already lost, and a worker sharded on the declared lane stopped claiming
the stream. Eviction is only half of it: a restart starts every process
with an empty map while the rows persist, so the same re-laning fires on
the first low-priority resolution after any deploy.

The record cannot hold the invariant, so the store does. `subscribe` now
writes a stream's lane on the same rule it merges priority: at or above
the stored priority sets the lane, below it leaves the lane alone.
Restart-driven re-laning still works, because a restart re-subscribes at
the same declared priority and equal priority writes.

Adapter authors: the rule is a Store contract change, covered by four new
cases in the store TCK. All three in-tree adapters implement it here; a
third-party adapter fails those cases until it does.

Closes #1599

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015xAdM431gFeFBZTW5kRrjg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:shared Shared libraries bug Something isn't working priority:medium Nice-to-have improvements

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant