feat(server): let liveSubscriptionAuth revoke a single subscriber without ending its subscription - #2039
Draft
kriszyp wants to merge 9 commits into
Draft
feat(server): let liveSubscriptionAuth revoke a single subscriber without ending its subscription#2039kriszyp wants to merge 9 commits into
kriszyp wants to merge 9 commits into
Conversation
…hout ending its subscription
registerLiveSubscription now accepts an optional `revoke?: () => void`, used as the
entry's terminate instead of the hard-coded end()/close()/emit('close') on the
subscription. It also returns an `{ unregister }` handle; when `revoke` is supplied
the subscription object is left untouched (no end() wrapping, no 'close' listener) —
the caller owns unregistration.
This is the prerequisite seam for sharing one live-subscription feed per topic across
its subscribers (a production node held 157,888 Subscription objects across 29
topics purely for per-connection bookkeeping). Wiring terminate to a shared feed
today would disconnect every subscriber on one user's revocation or token expiry;
this seam keeps revocation per-subscriber while feed sharing lands in a follow-up.
When `revoke` is omitted, behavior is byte-for-byte #1414: default terminate and the
existing end-wrapping/'close' self-wiring are unchanged, covered by regression tests.
Refs #1414
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tests Independent pre-push review (codex+gemini+grok+harper-domain, adjudicated) found two related majors in the seam: a custom `revoke` that throws was invoked a second time by sweep()'s fail-closed catch (the default terminate already caught its own errors, so this was latent but harmless before; a caller's non-idempotent revoke — e.g. a shared-feed refcount decrement — makes it consequential). And `unregister()` racing an in-flight `recheck()` was invisible to sweep, so terminate/revoke could still fire on an entry the caller had already torn down itself. Both share one missing invariant: terminate/revoke must run at most once per entry, and only while that entry is still registered. Enforce it with `claimAndTerminate`, using `Set.delete`'s boolean return as a lock-free claim token — no change to the recheck contract, fail-closed behavior, or interval/ITC triggers. Also gives terminate failures their own log message instead of being misattributed to "recheck error". Also: unitTests/server/liveSubscriptionAuth.test.js used sinon, which AGENTS.md forbids for new test files (unitTests/server/ has older sinon-based tests but is not the target shape). Replaced with a plain call-recording closure and added regression tests for both majors above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round-2 pre-push review (codex+gemini+harper-domain, adjudicated) found that
`revoke?: () => void` type-checks against an `async () => {...}` callback, but
claimAndTerminate's `try { entry.terminate() } catch` only ever caught a synchronous
throw — a rejected promise from an async revoke (the realistic shape for the
shared-feed use case this seam exists for, e.g. an awaited refcount release against a
backing store) would escape as an unhandled rejection and take down the whole worker,
dropping every unrelated connection on it. There's no production unhandledRejection
handler to fall back on.
Widen `terminate`/`revoke` to `() => void | Promise<void>` and `await` inside
claimAndTerminate's try/catch so a rejection is caught the same way a synchronous
throw already was. Also stop logging non-Error throws as "undefined" (a plain string
or object thrown by caller code is plausible, and swallowed the actual cause) via a
small errorMessage() helper, applied to both the terminate-failure and recheck-error
log sites this diff already touches. Added a docblock note on the two invariants a
`revoke` caller must uphold that this module can't enforce: owning the entry's full
lifetime, and not mutating recheck-shared state across subscribers on one feed.
Added a regression test for the rejecting-async-revoke case: the module-level
unhandledRejection handler in unitTests/testUtils.js would fail the whole suite were
this not fixed, so its passing is itself part of the proof.
Two review findings deliberately NOT addressed here, as out of scope for this seam
(flagged in the PR description instead): retrying a failed terminate (currently the
entry is dropped from tracking on the first failure — a design question for whoever
builds the shared-feed follow-up, not fixable without restructuring sweep()'s
iteration and risking new Set-mid-iteration bugs), and an AbortSignal-based backstop
for a caller that forgets unregister() (new API surface beyond this seam's scope).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g tracking Round-3 delta review (codex+gemini+grok+harper-domain, adjudicated across two consecutive rounds) escalated a finding first raised as moderate: claimAndTerminate used registry.delete()'s boolean return as both the claim AND the commit, so a revoke/terminate that throws or rejects still permanently removed the entry from the registry — the deauthorized subscriber keeps receiving data indefinitely, with only a log line as any trace. That's fail-open dressed as fail-closed. Split the claim from the commit: claimAndTerminate now checks `registry.has(entry)` (the caller hasn't already unregistered it) and only calls `registry.delete()` AFTER `terminate()` resolves successfully. On failure the entry is left exactly where it was in the Set — not re-added, so it can't be revisited within the same sweep pass (the delete-then-readd shape that would risk that was explicitly avoided) — and is picked up again on the next interval tick or ITC event. Since recheck() will fail the same way next time, retry is naturally bounded and self-terminating once teardown actually succeeds. This also fixes, for free, the round-2 minor about the recheck-error log line asserting a revocation that didn't happen: claimAndTerminate's return value is now strictly "did I just tear this down," so callers only log success when it's true. Consolidated the two ad-hoc log call sites into claimAndTerminate itself, at `error` (an actual failed security control, not a warning) for a failed attempt and `info` for a successful one — closing another minor about revocations never appearing in the log on the normal path. Updated the two tests that pinned the old drop-on-failure behavior to instead prove retry-until-success: revoke fails once, the entry stays registered and revoke is not called on other subscribers sharing the object, then a second sweep retries and the entry is removed once revoke actually succeeds. Two related findings deliberately left as documented, out-of-scope risk (called out explicitly in both this and the prior round's review as pre-existing, not introduced by this diff): an unbounded await inside sweep()'s serialized loop can wedge `sweeping` forever if a caller's recheck or terminate never settles (recheck already had this property before this PR); and the sweeping-reentrancy guard drops a concurrent ITC broadcast rather than deferring it. Both would require restructuring sweep()'s timing/reentrancy semantics, which this seam-only PR's brief explicitly excludes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round-4 full review (codex+gemini+grok+harper-domain, adjudicated) confirmed as major, across all four lenses: sweep()'s reentrancy latch (`sweeping`) is only cleared in a `finally` that never runs if an awaited call inside the loop never settles. `await entry.recheck()` already had this shape, but it calls registry-controlled Harper code (findAndValidateUser); `await entry.terminate()` is now arbitrary caller code (a shared-feed refcount release against a store that can wedge), which is a materially wider hole. A revoke that never settles would silently disable continuous re-authorization for every subscription on the worker, forever, with no log line. Race terminate() against a bounded timeout (5s default, env-overridable via HARPER_SUBSCRIPTION_TERMINATE_TIMEOUT_MS, read fresh per call so tests can override it without a require-cache reset) and treat a timeout as a terminate failure — same fail-closed retry claimAndTerminate already does for a throw/rejection. Deliberately does NOT touch recheck()'s await: that's pre-existing, registry-controlled, and outside this seam's brief. Also: made errorMessage() itself defensive (a thrown value whose own String() conversion throws — e.g. Object.create(null) — must not turn a contained failure into a new one propagating out of a catch handler with no outer guard), and added a third documented invariant a `revoke` caller must uphold: idempotency, since a failed attempt is retried and the registry.has() claim check only closes the pre-await race, not a caller's own teardown running concurrently with an in-flight revoke. Added a regression test: a subscription sharing an object with a hung revoke and a normal one — the hung entry doesn't block the normal one in the same sweep pass, and stays registered for retry rather than wedging sweep() indefinitely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ation; dedupe retries of a still-pending revoke Round-5 delta review (codex+gemini+grok+harper-domain, adjudicated) found two more concrete bugs: 1. registerLiveSubscription's validity guard (`!subscription || ... || subscription.closed`) ran unconditionally, even though the `revoke` path never reads `subscription` at all. A shared-feed caller with no meaningful per-subscriber subscription object — the expected shape for this seam — would silently get NOOP_HANDLE back, indistinguishable from a real registration, and that subscriber would never be re-authorized again. The guard is now skipped when `revoke` is supplied; it still applies in full to the default (registry-owned) path, unchanged. 2. The timeout added last round bounds how long a hung revoke can block one sweep, but didn't stop the NEXT sweep from invoking the same still-hanging revoke a second time (and a third, ...) — an unbounded pile of concurrent in-flight calls to a non-idempotent shared-feed teardown (e.g. a refcount decrement) rather than one contained hang. claimAndTerminate now caches the in-flight attempt on the entry (`pendingTerminate`) and reuses it across retries instead of calling terminate()/revoke() again, clearing the cache only once that specific attempt actually settles (success or failure) so a merely slow call gets retried fresh once it's actually done, while a genuinely stuck one is never re-invoked. Also, two cheap defensive fixes flagged as real but minor: `terminateTimeoutMs()` was read twice per attempt (now once), and the two `void sweep()` fire-and-forget trigger sites (the interval and the ITC listener) had no `.catch()` — a throw from claimAndTerminate's fail- closed catch arm (e.g. a broken logger) would have escaped as an unhandled rejection. Added tests for both fixes: registering with revoke against a null/undefined/closed subscription still tracks and revokes correctly; a revoke that stays pending across two sweep passes is invoked exactly once, and committing its eventual resolution never re-invokes it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… the racing sweep Round-6 delta review found a real bug in last round's fix, with a standalone repro: a revoke that times out (still pending) and then succeeds BETWEEN sweeps — after the timeout window closes but before the next sweep starts — was silently discarded. claimAndTerminate's own commit logic (registry.delete + success log) only ran after its own `await withTimeout(...)` resolved; if nothing was actively awaiting the cached attempt when it finally settled, that success was lost, and the next sweep, seeing `pendingTerminate` already cleared by the settle handler, invoked `revoke` again — exactly the double-invocation this cache was built to prevent, just shifted to a different timing window. The included test for this only passed because `resolveRevoke()` and the following `_sweepNow()` ran in the same synchronous turn in the test, which isn't representative of the real ~30s gap between sweeps. Fixed by moving the commit (and the failure log) into the handler attached to the cached attempt itself, at the point it's created — that handler fires whenever the attempt settles, whether or not a sweep happens to be actively racing it via `withTimeout` at that moment. `claimAndTerminate`'s own post-race code path no longer does any committing; it just reports whether ITS wait succeeded. A timeout with the attempt still pending logs a distinct "still pending, will retry" message so a persistently hung revoke doesn't go completely silent, without double-logging once the attempt's own handler has already logged its definitive outcome. Hardened the affected test to resolve the revoke and yield via `setImmediate` with no active sweep, asserting the entry is removed before any further sweep runs — this is the shape that would have caught the bug the same-turn version missed. Also (real but minor, cheap to close): wrapped every `hdbLogger.*` call this file makes in a new `safeLog()` helper. `hdbLogger.error` sits on top of an unguarded `fs.appendFileSync`; a throw from a log call inside the fail-closed catch arm would otherwise abort the sweep pass at that entry (leaving every entry after it unauthorized-but-still-delivering for the rest of that pass) and then escape `triggerSweep`'s own handler as an unhandled rejection. Two items raised again this round were deliberately NOT changed, with reasons now made explicit for the PR: - Bounding `recheck()` with the same timeout, as suggested, would change behavior on the DEFAULT (no-`revoke`) path — a legitimately slow-but-correct recheck under load could get spuriously treated as unauthorized — which conflicts directly with this seam's own acceptance criterion that revoke-absent behavior stay identical to #1414 in every respect. Bounding `terminate` is safe because it only ever engages on the brand-new, currently-unused `revoke` path; bounding `recheck` is not. - Serial per-entry timeout accumulation and the sweeping-guard dropping a concurrent ITC broadcast were reassessed this round as a latency regression bounded by the 30s interval backstop, not a re-authorization hole — deferred to whoever designs the real shared-feed sweep architecture, which is explicitly out of this task's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…top re-racing an already-pending revoke Round-7 delta review found the success log added two commits ago never actually fires on the DEFAULT (no-`revoke`) path — the only path any production caller uses today. The settle handler gated the log on `if (registry.delete(entry))`, but on the default path `entry.terminate()` calls `subscription.end()`, which is the registry's own wrapper — it synchronously calls `unregister()` (`registry.delete(entry)`) before the settle handler's microtask ever runs. By the time that handler checks, the entry is already gone, `registry.delete` returns false, and the log is skipped — silently, on every single default-path revocation. Since claimAndTerminate is the sole place that commits a successful termination, and an attempt's settle handler is only ever attached once per attempt, there was never a double-log risk to guard against; the guard was removed. Second finding: since the settle handler (not the racing `await` in claimAndTerminate) is what commits an outcome, re-racing an entry whose attempt is already in flight from a prior sweep serves no purpose — the boolean it produces isn't used by anything. It only costs a full `terminateTimeoutMs()` of serialized sweep time per stuck entry, on every single sweep pass, forever, while the entry accumulates more never-firing `.then()` reactions on the same hung promise. `claimAndTerminate` now returns immediately (no wait, no new log) when it finds `entry.pendingTerminate` already set — only the FIRST attempt for a given entry is ever raced against the timeout; every outcome, however it eventually arrives, is still handled by that first attempt's settle handler. Added a regression test asserting the default (`end()`-wrapping) path actually emits the info log — this would have failed against the previous two commits. Added a timing assertion to the existing pending-revoke test proving a sweep that finds an already-pending attempt returns near-instantly rather than re-racing the timeout. Two more re-raised findings again declined, unchanged from the reasoning already on record: bounding `recheck()` the same way would change default-path behavior, conflicting with this seam's own acceptance criterion that revoke-absent behavior stay byte-for-byte #1414; and an escalation ceiling / hard-teardown fallback for a permanently-failing `revoke` contradicts the seam's purpose (never end a shared feed over one subscriber's failure) — both are design questions for the shared-feed follow-up PR, not defects in this one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…a recheck on it; honest success wording Round-8 delta review found that last round's "don't re-race an already-pending attempt" fix, while correct on its own terms, had two side effects that made a permanently-stuck revoke strictly worse to operate against than before: 1. A revoke that never settles now produces exactly ONE log line, ever, then goes completely silent for the rest of the worker's life — worse than the previous round's "log every sweep" behavior, even though that behavior was also functionally inert (nothing was actually being retried). Restored a cheap, no-wait log on every sweep that finds an already-pending attempt, so a persistently stuck entry stays visible without reintroducing the timeout cost the previous fix removed. 2. `sweep()` still ran a full `recheck()` — in production a storage read (`findAndValidateUser`) plus user-overridable `allowRead` — for every entry whose terminate is already known to be pending, every single sweep, for as long as it stays stuck. `sweep()` now skips straight to `claimAndTerminate` (which just logs and returns) for an entry with `pendingTerminate` already set, and only computes `Date.now()` per entry rather than once before the loop, so a slow entry earlier in a pass can't make a later entry's expiry check stale (pre-existing #1414 shape, cheap to correct now that per-entry timeouts can stretch a pass). Also fixed, confirmed via the reviewer's probe: on the DEFAULT (no-`revoke`) path, a throwing `end()`/`close()` is swallowed internally (unchanged #1414 behavior) but the settle handler still logged "revoked subscription" as if delivery were confirmed stopped. Reworded the success log to "terminate completed" — honest about what this module actually knows (terminate ran without an error reaching it), not a claim about delivery state it can't verify on the swallow-everything legacy path. Replaced a `<15ms` wall-clock assertion (AGENTS.md names this exact pattern as a flakiness root cause) with a deterministic race: raise the timeout to 100s and race the sweep against a short marker — an implementation that re-raced the pending attempt could not win regardless of runner load. Extended the hung-revoke test to cover a second sweep, proving no re-invocation and the restored per-sweep log. One more re-raised finding declined again, same reasoning as the last three rounds: an escalation ceiling / re-invocation-after-N-sweeps for a permanently failing `revoke` is a design decision for the shared-feed follow-up (how many sweeps, and whether re-invoking a call that might still genuinely be in-flight is even safe, given nothing here can distinguish "dead" from "slow"), not a defect in this seam. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the live subscription authorization mechanism in liveSubscriptionAuth.ts by introducing support for a caller-supplied revoke callback, allowing shared feeds to manage revocation per subscriber. It also adds robust error handling, logging safety, and timeout bounds to prevent hung terminations from blocking the sweep process. Comprehensive unit tests are added to verify these behaviors. The reviewer recommends snapshotting the registry Set into an array before iterating over it in sweep() to avoid mutating the Set during iteration.
| sweeping = true; | ||
| try { | ||
| const now = Date.now(); | ||
| for (const entry of registry) { |
Contributor
There was a problem hiding this comment.
Avoid mutating a Map or Set (such as deleting or unregistering entries) while iterating over its keys or values. In sweep(), the code iterates over registry (which is a Set) directly using for (const entry of registry). During this iteration, claimAndTerminate can delete entries from registry (either synchronously or asynchronously via microtasks). To comply with the repository's general rules and prevent potential iteration issues, snapshot the registry into an array first (e.g., using Array.from(registry)) and iterate over that snapshot.
const entries = Array.from(registry);
for (const entry of entries) {References
- Avoid mutating a Map or Set (such as deleting or unregistering entries) while iterating over its keys or values. Instead, snapshot the target elements into an array first, and then perform the mutations or operations outside of the iteration loop.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
+24
to
+26
| function terminateTimeoutMs(): number { | ||
| return Number(process.env.HARPER_SUBSCRIPTION_TERMINATE_TIMEOUT_MS) || 5_000; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion (non-blocking): terminateTimeoutMs() is unclamped — Number(process.env.HARPER_SUBSCRIPTION_TERMINATE_TIMEOUT_MS) || 5_000. If this env var is ever set above the 32-bit setTimeout ceiling (2147483647ms), Node coerces the delay to ~1ms and withTimeout in claimAndTerminate fires almost immediately, marking every terminate attempt "timed out" even when the underlying revoke/terminate() actually settles quickly (harmless direction here since a spurious timeout just re-logs and retries rather than corrupting state, but still worth guarding). Consider clamping, e.g. Math.min(Math.max(raw, 1), 2_147_483_647).
Contributor
|
Reviewed; no blockers found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
registerLiveSubscription(the #1414 continuous re-authorization registry) now accepts an optionalrevoke?: () => void | Promise<void>, used as the entry's terminate instead of the hard-codedend()/close()/emit('close')on the subscription object, and returns an{ unregister }handle. Whenrevokeis supplied, the subscription object is left completely untouched (noendwrapping, no'close'listener) — the caller owns unregistration and teardown.Why: profiling a production node found ~157,888 live
Subscriptionobjects across only 29 distinct topics, almost entirely per-connection bookkeeping overhead (each carries its ownDatabaseTransaction,TableResource,Generator, etc.). The fix is to share one feed per topic across its subscribers while keeping authorization per subscriber — but today, revocation works by destroying the subscription object, which is exactly wrong once that object is shared: wiringterminateto a shared feed would disconnect every subscriber on one user's revocation or token expiry (and, sincesweep()fails closed on recheck errors, one transient error would tear the feed down for everyone). This PR is only the prerequisite seam — it does not implement shared feeds, refcounting, or any subscriber-list machinery (explicitly out of scope; that's the harper-pro follow-up).Revocation granularity must stay per subscriber even once feed granularity becomes per topic — that's the invariant this seam exists to establish.
When
revokeis omitted, behavior is unchanged from #1414 in every respect (covered by regression tests); the sole existing caller (registerLiveSubscriptionForContextinresources/Resource.ts) doesn't passrevokeand is verified unaffected by the full#1414integration suite (6/6 passing).Where to focus
This branch went through 8 rounds of independent cross-model review (codex + gemini + grok + a Harper-domain adjudication pass), each surfacing and fixing a real concurrency/observability bug in the parts of
claimAndTerminatethat only engage once a caller actually suppliesrevoke— currently nobody does, so all of that logic is dormant in this repo and will be exercised for real by the harper-pro shared-feed follow-up. The commit history documents each fix's reasoning; theclaimAndTerminatedocblock is the single source of truth for the current contract. Worth a careful read:server/liveSubscriptionAuth.ts—claimAndTerminate(the whole revocation state machine: at-most-once invocation viapendingTerminate, timeout-bounded first attempt, settle-handler-based commit so a late success isn't lost or double-invoked, fail-closed retry on failure).unitTests/server/liveSubscriptionAuth.test.js— 18 tests: regression-pins the no-revokepath byte-for-byte against Live subscriptions (SSE/MQTT/WS) continue delivering events after drop_user / role revocation — stale-auth leak #1414, and covers the seam's isolation/fail-closed/timeout/retry properties.Deliberately declined, with reasons (re-raised by reviewers across multiple rounds; keeping these open for the shared-feed PR to decide, not fixing here):
recheck()with the same timeout asterminate().recheckhas the identical unbounded-await shape and, in production, bottoms out in user-overridableresource.allowRead— arguably no safer thanrevoke. Not fixed here because it would change behavior on the default (no-revoke) path: a legitimately slow-but-correct recheck under load could get spuriously treated as unauthorized, which conflicts directly with the acceptance bar that revoke-absent behavior stay identical to Live subscriptions (SSE/MQTT/WS) continue delivering events after drop_user / role revocation — stale-auth leak #1414.revoke(e.g., falling back to a hardend()/close()after N consecutive failures). The registry currently retries forever with a per-sweep log; escalating to a hard teardown on a shared object is exactly the failure mode this seam exists to prevent, so it can't be decided generically here — it belongs with whoever wires up the actual shared-feedrevoke.K × timeoutcost when K entries go newly-stuck in one sweep pass). This is bounded by the sweep interval and only affects the still-unusedrevokepath, but fixing it means restructuringsweep()'s serial loop, which is explicitly out of this task's scope.Test coverage
18 unit tests in
unitTests/server/liveSubscriptionAuth.test.js(plainnode:assert+ hand-rolled call-recording closures, no sinon perAGENTS.md), plus the existingintegrationTests/security/subscription-revocation.test.ts(#1414 suite, 6/6 passing, unmodified) confirming the sole in-repo caller is unaffected.test:unit:security(397 passing) and full local build all green.Not covered, because no in-repo caller exercises
revokeyet: real transport/timer/ITC-triggered use of the seam. That will be exercised for real by the harper-pro shared-feed PR.Generated with Claude Sonnet 5. Independent review: codex + gemini + grok (experimental) + harper-domain, 8 rounds, adjudicated — see commit messages for what each round found and fixed.
🤖 Generated with Claude Code