Skip to content

WB-186: fix sse N+1 Pattern#5

Merged
kacpercierzniewski merged 3 commits into
mainfrom
WB-186-fix-sse-n1-issue
May 27, 2026
Merged

WB-186: fix sse N+1 Pattern#5
kacpercierzniewski merged 3 commits into
mainfrom
WB-186-fix-sse-n1-issue

Conversation

@kacpercierzniewski
Copy link
Copy Markdown
Contributor

refactor(backend): wire N+1 SSE fix into the actually-used endpoint

The first pass at WB-186 introduced fetchEventsAfter + drainEventsSince but only wired them into a new GET /:id handler. That endpoint isn't called by the frontend — use-backend-execution.ts opens EventSource on the streamUrl returned by POST /execute, which is /:id/stream. Result: drainEventsSince was unit-tested and dead-pathed, while the hot SSE endpoint kept the original full-history-fetch-then-filter pattern. The N+1 bug was unchanged in production traffic. Helper-only tests masked the drift because the route had no integration coverage.

This commit:

  • Restores GET /:id as a JSON metadata endpoint (REST convention, reverses the accidental co-opting into SSE).
  • Rewrites GET /:id/stream to use drainEventsSince + fetchEventsAfter for both the snapshot and live drains — one query shape across the route. Heartbeat + abort cleanup preserved.
  • Drops the now-unused executionEvents Drizzle import the in-route query previously needed; the route reaches the table only through fetchEventsAfter.

Cleanups:

  • drain-events.ts: EventWriter and DrainResult demoted from export type to module-local types. knip flagged them as unused exports; consumer code reaches them only by inference from drainEventsSince's signature.
  • drain-events.test.ts: extract an inline fixedFetcher helper to satisfy unicorn/consistent-function-scoping.

The earlier deletion of fetch-events-after.test.ts is preserved — the helper now carries zero testing scaffolding in its signature; SQL coverage moves to a separate integration test path when wanted.

@kacpercierzniewski kacpercierzniewski changed the title WB:186 fix sse N+1 Pattern WB-186: fix sse N+1 Pattern May 22, 2026
Comment thread apps/backend/src/routes/executions.ts Outdated
@kacpercierzniewski kacpercierzniewski force-pushed the WB-186-fix-sse-n1-issue branch 3 times, most recently from 3a49950 to 698d7ba Compare May 22, 2026 07:08
refactor(backend): wire N+1 SSE fix into the actually-used endpoint

The first pass at WB-186 introduced fetchEventsAfter + drainEventsSince
but only wired them into a new GET /:id handler. That endpoint isn't
called by the frontend — use-backend-execution.ts opens EventSource on
the streamUrl returned by POST /execute, which is /:id/stream. Result:
drainEventsSince was unit-tested and dead-pathed, while the hot SSE
endpoint kept the original full-history-fetch-then-filter pattern. The
N+1 bug was unchanged in production traffic. Helper-only tests masked
the drift because the route had no integration coverage.

This commit:

- Restores GET /:id as a JSON metadata endpoint (REST convention,
  reverses the accidental co-opting into SSE).
- Rewrites GET /:id/stream to use drainEventsSince + fetchEventsAfter
  for both the snapshot and live drains — one query shape across the
  route. Heartbeat + abort cleanup preserved.
- Drops the now-unused executionEvents Drizzle import the in-route
  query previously needed; the route reaches the table only through
  fetchEventsAfter.

Cleanups:

- drain-events.ts: EventWriter and DrainResult demoted from
  export type to module-local types. knip flagged them as unused
  exports; consumer code reaches them only by inference from
  drainEventsSince's signature.
- drain-events.test.ts: extract an inline fixedFetcher helper to
  satisfy unicorn/consistent-function-scoping.

The earlier deletion of fetch-events-after.test.ts is preserved — the
helper now carries zero testing scaffolding in its signature; SQL
coverage moves to a separate integration test path when wanted.

Refs: WB-186

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kacpercierzniewski kacpercierzniewski force-pushed the WB-186-fix-sse-n1-issue branch from 698d7ba to a5ef265 Compare May 22, 2026 07:08
@kacpercierzniewski kacpercierzniewski marked this pull request as draft May 26, 2026 11:23
The serialize-coalesce concurrency logic lived inline in the /:id/stream
route and could only be exercised by re-implementing it inside a test,
so the test guarded a copy rather than the shipped code. Extract it into
createSerializedDrainer and test the real function.

- serialized-drainer.ts: owns the concurrency policy (one drain at a
  time, coalesce a burst into a single follow-up pass, track done/stop).
  The route wires fetchEventsAfter + drainEventsSince through it.
- Close the snapshot->subscribe lost-wakeup window: a terminal event
  inserted between the snapshot read and subscribe fired its NOTIFY into
  the void and the stream hung in "running". A catch-up drain after
  subscribe replays anything missed from the snapshot cursor.
- Log SSE write failures at the route boundary instead of swallowing
  them silently; document that DrainResult.writeFailed means the client
  disconnected, not data corruption.
- drain-events.test.ts: drop the duplicated harness test, now covered
  against the real createSerializedDrainer.

Refs: WB-186

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kacpercierzniewski kacpercierzniewski marked this pull request as ready for review May 27, 2026 05:01
# Conflicts:
#	apps/backend/src/routes/executions.ts
@kacpercierzniewski kacpercierzniewski merged commit 92c6ce6 into main May 27, 2026
2 checks passed
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.

2 participants