ReviewOS

stacks/stx

signals-api's onMount/onDestroy queues are never drained — module-path lifecycle hooks are silent no-ops (and leak)

#1811
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Found while fixing the useClipboard timer leak in #1805.

Summary

packages/stx/src/signals-api.ts declares two module-level arrays:

const mountCallbacks: LifecycleCallback[] = []
const destroyCallbacks: LifecycleCallback[] = []

Both are written and never read. onMount pushes to the first, onDestroy pushes to the second, and nothing anywhere drains either:

$ grep -rn 'destroyCallbacks\|mountCallbacks' packages/stx/src --include='*.ts' | grep -v 'signals.ts:'
packages/stx/src/signals-api.ts:132:const mountCallbacks: LifecycleCallback[] = []
packages/stx/src/signals-api.ts:133:const destroyCallbacks: LifecycleCallback[] = []
packages/stx/src/signals-api.ts:492:  mountCallbacks.push(callback)
packages/stx/src/signals-api.ts:509:  destroyCallbacks.push(callback)

The client runtime's equivalents are drained (signals.ts — destroyCallbacks is consumed by cleanupContainer, and scope-level __destroyCallbacks by the scope teardown path). So this is a dual-impl parity gap in the sense of CLAUDE.md item 40: the same public API does something on one side and nothing on the other.

Why it matters

Two distinct consequences:

  1. Silent no-op. Any composable or store in the module tree that registers cleanup via onDestroy gets no cleanup. The code reads as correct, passes review, and does nothing. useClipboard is the case that surfaced it — I had to add an explicit stop() there because the onDestroy I registered would never have fired.
  2. Unbounded growth. Because nothing ever clears the arrays, every onMount/onDestroy call on the module path retains its closure for the life of the process. In a long-running SSR server that is a slow leak whose retained objects are exactly the ones a developer expected to be released.

Repro

import { onDestroy } from '@stacksjs/stx'

let ran = false
onDestroy(() => { ran = true })
// there is no exported way to make `ran` true

Suggested direction

Not obvious enough for me to just pick one, so flagging rather than fixing:

  • (a) Export drains (runMountCallbacks() / runDestroyCallbacks()) and call them from whatever owns the module-path lifecycle — SSR render teardown, and tests. Smallest change; needs an owner identified for each entry point.
  • (b) Scope the queues the way the runtime does (per render/per scope) rather than module-global, so the arrays cannot outlive their unit. Closer to the runtime's semantics, larger change.
  • (c) Make them throw or warn outside a scope, so the no-op is loud instead of silent. Weakest, but strictly better than today.

My inclination is (a) plus a parity test asserting both impls actually invoke a registered onDestroy, since (b) is the kind of change that wants its own design pass.

  • #1805 — where this surfaced; useClipboard now exposes stop() precisely because onDestroy alone was dead code there.
  • CLAUDE.md item 40 — dual reactive implementations must behave equivalently.
10 days ago

Fixed in f35c8704e6.

Took option (a) — exported runMountCallbacks/runDestroyCallbacks — rather than re-scoping the queues per render. Scoping is the better end state, but it changes how the module path models component boundaries and wouldn't be reviewable alongside a release; draining makes cleanup reachable today and removes the leak, and the queues can be scoped later without changing the public surface.

Two details worth recording:

  • Drains before invoking, so a hook that registers another is queued for the next drain rather than recursing.
  • Errors are contained per callback. One throwing hook must not cost every hook after it — the same lesson as the page-setup loop in #1805.

pendingLifecycleCounts() is exported for tests and diagnostics, since "the queue only ever grows" is the shape of the defect.

This also closes the loop on the thing that surfaced it: useClipboard's onDestroy now demonstrably fires, and there's a test covering that path alongside the explicit stop(). I've kept stop() — it's still the right call outside a component scope, and it matches the shape that family of composables already uses.

Sign in to comment on this issue.