Skip to content

fix(pump): preserve factory arity 2 so per-pumpGroup state managers are honored#84

Merged
jbiskur merged 1 commit intomainfrom
fix/pump-state-factory-arity
May 5, 2026
Merged

fix(pump): preserve factory arity 2 so per-pumpGroup state managers are honored#84
jbiskur merged 1 commit intomainfrom
fix/pump-state-factory-arity

Conversation

@jbiskur
Copy link
Copy Markdown
Contributor

@jbiskur jbiskur commented May 5, 2026

Summary

createPostgresPumpStateManagerFactory returned a function with a default value on pumpGroup:

return (flowType: string, pumpGroup: string = DEFAULT_PUMP_GROUP) => 

JavaScript's Function.prototype.length skips parameters with default values, so this function's .length === 1. PathwayPump's arity check at pathway-pump.ts:239:

if (this.stateManagerFactoryArity <= 1) {
  // legacy fallback — call with flowType only
  return (this.stateManagerFactory as unknown as (flowType: string) => PumpStateManager)(flowType)
}

…treats the library's own factory as a "legacy" single-arg factory and invokes it with flowType only. The defaulted pumpGroup becomes "default" for every pump, so two pumps on the same flowType with different pumpGroup values share a single PostgresPumpStateManager keyed (flowType, "default").

Production evidence

Verified in service-tenant-store-api@1.6.3 (which registers tenant.0/api-key.used.0 with pumpGroup: "hot" and 12 other tenant.0 event types under the implicit default group):

  • _pathway_pump_state contained only one row for tenant.0 (the default group). The (tenant.0, hot) row was never inserted, even though the schema has PRIMARY KEY (flow_type, pump_group) and supports it.
  • The default row's event_id was verifiably an api-key.used.0 event id (the only event type with activity in that bucket). The default pump — whose eventTypes filter excludes api-key.used.0 — was advancing past api-key.used.0 events because its state manager was shared with the hot pump.
  • The hot pump silently no-op'd: shared cursor advanced past its events, fetches returned empty batches, no acks, no setState calls, no row.

Fix

Drop the default value on the second parameter so .length === 2. Move the runtime fallback inside the body:

-  return (flowType: string, pumpGroup: string = DEFAULT_PUMP_GROUP): PumpStateManager => {
-    return new PostgresPumpStateManager(adapter, flowType, pumpGroup, table)
+  return (flowType: string, pumpGroup: string): PumpStateManager => {
+    return new PostgresPumpStateManager(adapter, flowType, pumpGroup ?? DEFAULT_PUMP_GROUP, table)
   }

PathwayPump now takes the non-legacy branch and forwards both (flowType, pumpGroup) to the factory. Each pair gets its own state manager and its own row in _pathway_pump_state. The ?? DEFAULT_PUMP_GROUP inside the body preserves runtime safety for any external caller passing undefined — same observable behaviour as before for legacy callers.

Tests

  • tests/postgres-pump-state.test.ts — new step factory function declares arity 2 …. Asserts factory.length === 2. This is the direct regression catch.
  • tests/pathway-pump.test.ts — new step arity-2 state factory receives both (flowType, pumpGroup) so each group gets its own manager. Constructs a PathwayPump with an arity-2 mock factory, runs startPumpForGroup for hot and default groups, asserts the factory was called with [("orders", "hot"), ("orders", "default")], two distinct managers were instantiated, and no legacy-arity warning was emitted.

The existing legacy test ("legacy single-arg state factory is accepted with a deprecation warning") still passes — backward compatibility for genuinely arity-1 user-provided factories is preserved.

Test plan

  • deno fmt --check clean
  • deno lint clean
  • deno check src/mod.ts clean
  • deno test -A --ignore=tests/postgres-*.test.ts15 passed (140 steps)
  • Postgres tests after this lands — covered by the existing test:postgres task.

Risk notes

  • Tiny blast radius. One-line library change. The factory's runtime semantics for any well-formed caller are identical.
  • Existing user factories with arity 1 continue to work unchanged via the legacy fallback path.
  • No data migration. The composite-PK migration on _pathway_pump_state already shipped in 2.4.0. New (flow_type, pump_group) rows will be inserted on first hot-pump setState per consumer service after they bump.

Related fragments

  • f98a5d4b-… — original 2.4.0 pumpGroup release.
  • c2ee688f-… — 2.4.1 pulse pathwayId fix.
  • f7736088-… — service-tenant-store-api 1.6.0 → 1.6.3 production rollout (same service that surfaced this bug).

…re honored

createPostgresPumpStateManagerFactory returned a function with a default value
on `pumpGroup`, making `Function.prototype.length === 1`. PathwayPump's arity
check (`stateManagerFactoryArity <= 1`) treated the factory as legacy and
called it with `flowType` only — so the defaulted `pumpGroup` collapsed every
group onto a single shared state manager keyed `(flowType, "default")`.

Production effect (verified in service-tenant-store-api@1.6.3):
- _pathway_pump_state had ONE row for tenant.0 (the default group).
- The default pump's stored event_id was an api-key.used.0 id, even though
  the default pump's eventTypes filter excludes that event type.
- The hot pump (pumpGroup="hot", eventTypes=["api-key.used.0"]) never wrote
  state — both pumps shared the same manager, default pump's setState calls
  advanced the shared cursor past hot pump's events, hot pump fetched empty
  batches, never accumulated events to ack, never called setState. The
  isolation guarantee promised by 2.4.0 was silently broken.

Fix: drop the default value on pumpGroup so .length === 2. PathwayPump now
takes the non-legacy branch and forwards both (flowType, pumpGroup) to the
factory. Each (flowType, pumpGroup) pair gets its own PostgresPumpStateManager
instance and its own row in _pathway_pump_state. `pumpGroup ?? DEFAULT_PUMP_GROUP`
inside the body keeps runtime safety for any external caller passing
undefined — same observable behavior as before for legacy callers.

Tests:
- postgres-pump-state.test.ts — assert factory.length === 2 (the direct
  regression catch).
- pathway-pump.test.ts — assert PathwayPump forwards (flowType, pumpGroup)
  to an arity-2 factory and creates one manager per (flowType, pumpGroup),
  with no legacy-arity warning.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jbiskur jbiskur merged commit a082767 into main May 5, 2026
1 check passed
@jbiskur jbiskur deleted the fix/pump-state-factory-arity branch May 5, 2026 14:33
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