Skip to content

fix(act): normalize omitted lanes in the dynamic lane-conflict report - #1611

Open
Rotorsoft wants to merge 1 commit into
masterfrom
act-1598-dynamic-lane-default-normalization
Open

fix(act): normalize omitted lanes in the dynamic lane-conflict report#1611
Rotorsoft wants to merge 1 commit into
masterfrom
act-1598-dynamic-lane-default-normalization

Conversation

@Rotorsoft

Copy link
Copy Markdown
Owner

Closes #1598.

A reaction that routes to a stream without naming a lane runs on the default lane. Two reactions that disagree about a stream's lane are a configuration error, and the framework refuses to build when it can see the disagreement in the declaration. It could not see this one: when one side left the lane off and the other asked for "slow", the runtime check that covers reactions whose target is computed at runtime skipped the comparison entirely and logged nothing. The stream quietly landed on the default lane, and a worker started to serve only the slow lane never picked it up. This makes the runtime check read an omitted lane as the default lane, which is what it has always meant everywhere else.

What was actually broken

Act has two halves of the same rule. When a reaction's target is written out literally, act().build() throws on a lane disagreement. When the target is a function, there is nothing to inspect until an event arrives, so correlate applies the same rule at resolution time and logs instead of throwing — a throw there would stop event correlation for the whole application.

The build-time half learned in #1583 to compare the resolved lane name, so an omitted lane and an explicit lane: "default" agree instead of being reported as a conflict. The runtime half was left comparing the raw values, and a raw comparison of undefined against "slow" fails the "are both lanes named?" precondition and falls through. So the exact pair the build-time guard rejects — .to({ target: "T" }) against .to({ target: "T", lane: "slow" }) — was silent in its dynamic form.

The cost is not cosmetic. First discovery wins, so the target sits on the default lane. The slow reaction still runs, but inside the default lane's lease and stream budget on a process nobody sized for it, and a process restricted to the slow lane via onlyLanes never runs it at all. Every symptom points at the deployment; the one line that would have named the cause never printed.

Two things the one-line fix would have broken

Normalizing both sides is the fix, and on its own it opens a hole immediately. A target no resolution has ever produced has no lane because there is no record of it — not because its record says "default". Normalize that and every first sighting of a laned target reports a conflict against a stream that does not exist yet. So the report is now gated on the earlier resolution existing, not on its lane being defined.

The second one was already broken before this PR. The report is only meant for ties, where neither side outranks the other and the winner is arbitrary from the operator's chair; when priority decides, the outcome is the documented maximum rule and not a surprise. But the tie was tested against the running accumulator, which for a resolution that beat the target's recorded priority had already taken that resolution's own priority — so the check compared a number with itself, passed, and reported a conflict for a clean priority upgrade. It now reads the priority the lane is actually held at. That is a false-positive fix nobody filed, found by reading the two sources of "what lane does this target already carry?" against each other as the ticket asked.

The undeclared-lane reroute (#1564) sets the lane back to undefined before this comparison, so it used to walk into the same blind spot. It now normalizes too: a typo'd lane rerouted onto a stream already on "slow" reports both halves, the reroute and the conflict.

Reporting semantics are unchanged — log and continue, never throw, once per offending declaration.

Cost and limits

No behavior changes except which messages get logged: one class of real conflict now reports, one class of false conflict stops reporting. Nothing is re-laned, no lane is corrected mid-run, and no public API moved. The blind spot this closes is the report, not the outcome — a disagreeing pair still keeps the first-discovered lane, because re-laning a live stream would move it out from under a worker holding its lease. Aligning the resolvers is still the operator's job; the fix is that they now find out there is something to align.

What this cannot cover: the runtime check only fires once a resolver has actually produced both sides of the disagreement. A pair that disagrees only for events that have not arrived yet stays undiscovered, which is inherent to a target that is a function.

Tests

Eight cases in libs/act/test/dynamic-lane-guard.spec.ts, four of which fail without the source change, each paired with the static control that already throws. Both discovery orders; the across-scans path where the held lane is read back from the subscription row rather than from the running scan; the rerouted-undeclared-lane case. Three controls pin what must not report: an omitted lane against an explicit "default", a first resolution onto a never-seen target, and a higher-priority resolution that outranks rather than conflicts.

Docs: the dynamic bullets in configuration.md § Conflicting lane assignments now state the resolved-name rule and that priority decides rather than conflicts, and behavior-contracts.md gains a row pinning both halves to their tests. Narrative in book/1598-an-omitted-lane-is-a-lane-name.md.

Test plan

  • pnpm test — 238 files, 3639 passed, 54 skipped
  • Coverage: 100% statements / 100% branches / 100% functions / 100% lines.
  • pnpm typecheck
  • pnpm lint
  • pnpm build
  • New tests verified red before the fix
  • CI green
  • Review

Note: the first full run flaked twice in store-tck.spec.ts ("keeps the mark across unblock, defer, and prioritize", a Date.now() - 1 defer race) against freshly started Postgres containers. Both passed on re-run and neither touches this code path.

Stability charter impact

None. The change is confined to libs/act/src/internal/correlate-cycle.ts; no builder API, IAct method, port contract, lifecycle event, or exported type changed. The stability snapshot moved because it captures source text and the rewritten comment plus two locals are longer than what they replaced.

rfc-gate: exempt — snapshot growth is internal correlate-cycle.ts implementation text (comments and two local consts). 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

The correlate-side lane guard compared resolutions as spelled, so an
omitted lane (undefined) never compared against a declared one and the
disagreement the static guard throws on went unreported. Compare both
sides by resolved lane name, as build-classify does since #1583.

Normalizing alone would report every first sighting of a laned target
against a "default" lane no record holds, so the report is gated on the
prior resolution existing rather than on its lane being defined. The tie
test now reads the held resolution's own priority: comparing against the
accumulating entry made a priority upgrade tie with itself and report a
conflict where the max() rule had already decided.

Closes #1598

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.

The dynamic lane-conflict guard misses an omitted lane vs a declared one (#1583 on the dynamic path)

1 participant