Read users and roles through the record cache, ending user-cache refresh storms

[kriszyp]

Authentication now reads hdb_user and hdb_role through the primary store's record cache instead of keeping a per-thread copy of both tables, so a user or role change needs no broadcast and there is nothing left to rebuild. The replicated-write refresh storm this PR was opened for has no mechanism left: the replication apply loop no longer knows about users at all (the userRoleUpdate flag and its signal are deleted).

• Lookups read the records. readUserEntries point-reads the user by name and its role by id, then re-checks the user after reading its role: RocksDB point reads share no snapshot, so one transaction that moves a user from role A to B and grants A super_user must not be read as "user on A, with A's grant". The re-check costs one native verifyVersion on RocksDB (isUnchanged).
• Derived data follows record versions. The role's system-table permissions and expanded operations are memoized per role version, and each returned user gets its own role and permission objects. A record a resequenced write left on a reused version (stampOf, holdsStamp) is compared by value, since its version no longer identifies one value.
• Cached credentials are re-checked, not flushed. Every authorizationCache hit runs isCurrentUser against the versions the principal was built from (isCurrentUser), so a password, role or activity change takes effect on the next request on every worker. A component's server.getUser principal is checked against the versions read for its name before it was resolved (trackUserRecords).
• Only holders of a user are notified. onUserChange feeds live-subscription revocation and MCP list-changed from per-thread hdb_user/hdb_role subscriptions. It never subscribes to an unaudited table, because subscribe() would enable and persist auditing on it, and it retries a failed subscription with backoff.

For the human reviewer

1. Framing (step-6 planning review): Framing-Verdict: better-alternative-exists, resolved mostly by adopting it. The reviewer endorsed removing the copy, then asked for durable versions instead of object identity, a torn-read guard, no silent audit enablement, and a subscription failure contract. All four are in. Two parts were overruled, each on a fact:

   • Reset LMDB's read txn on every lookup for strict post-ack freshness: lmdb-js abandons a read txn that other code still references (node_modules/lmdb/read.js:1062-1070), so each in-flight transaction would hold its own reader slot, and a reset costs ~1.5 µs plus a timer per lookup (measured below). Kept instead: LMDB lookups see another thread's commit from the next event-loop turn, like every other LMDB read. On a single busy turn, a request that arrives after an acknowledgement can still see the pre-change snapshot. RocksDB lookups have no such window.
   • A non-auditing after-commit observer for notifications: none exists. Cross-thread subscription delivery is driven by the audit log (resources/transactionBroadcast.ts addSubscription), so without auditing there is nothing to observe from.

2. The requirement itself (your direction in this PR's thread): warranted. The storm was one instance of a design in which eight producers had to remember a broadcast, and a spurious broadcast cost an O(users + roles) scan on every thread. Removing the copy removes both the missed-signal and the extra-signal failure classes. The earlier per-commit mark plus coalescer is gone rather than kept as a second layer; utility/coalesceRefresh.ts survives only for the live-subscription sweep.

3. Per-request cost, the price of reading records. Measured on this machine, unit environment, 50,000 iterations, RocksDB / LMDB:

   path	before	after
   authentication() Basic cache hit	2.3 / 1.5 µs	3.9 / 6.0 µs
   findAndValidateUser (session, mTLS, token user, local bypass)	0.4 / 0.3 µs	3.2 / 4.9 µs

   The cache-hit check is two native verifyVersion calls on RocksDB (0.4 µs each) and two validated reads on LMDB. A change costs nothing extra on any thread. Tell me if the hit-path cost is too high: the alternative is to drop the per-hit check and rely on onUserChange to flush the cache, which brings back the asynchronous window.

4. Cross-worker completion semantics changed. alter_user, alter_role and the rest no longer await every worker. They do not need to for lookups: the first read after the commit on any worker sees it (subject to item 1 on LMDB). The push consumers (live-subscription revocation, MCP notifications) were never awaited on main either; they now fire from the table subscription.

5. Audit-only push channel. On a node with logging.auditLog: false, live-subscription revocation falls back to its 30-second backstop sweep, and MCP sessions get no user-change list_changed. Each thread logs this once at info. Authentication itself is unaffected. main enables auditing on hdb_certificate implicitly (security/keys.ts subscribes unconditionally); I did not want to extend that to tables holding password hashes.

6. Component principals are tracked by the Basic username. A component's server.getUser principal is re-verified once the hdb_user/hdb_role records named by the Basic username change. A component that maps that username to a different user, or takes the role from elsewhere, keeps its principal until authentication.cacheTTL (30 s) or server.invalidateUser(). main flushed the whole cache on any user event; adding that flush back through onUserChange is a one-liner if you want both.

7. Deleted outright, not deprecated: the USER ITC event type, signalUserChange, UserEventMsg, serverHandlers.userHandler, setUsersWithRolesCache and getUsersWithRolesCache. harper-pro outside core has no callers (grep of its main). An external component calling them would fail at load.

8. VERSION_REUSED records. An out-of-order replicated write (for example, create_authentication_tokens on two nodes) leaves a user record on a reused version until its next in-order write. Such a record is compared by _.isEqual of its value, so cache hits and the role memo still work, at the cost of one decode plus a deep compare per hit. The first version of this redesign looped forever on such a record; the review caught it, and a test now pins it.

9. Backports. Fix user-cache refresh storm from replicated system-database writes (v5.1) (#2777) stays the narrow per-commit fix, which suits a patch line. This redesign is main (v5.3) only. v5.2 still has the original defects; backport scope there is your call.

10. Deliberately unchanged: listUsers() still scans both tables through the legacy search, now only on cold paths (list_users, the last-super-user guard, and the first getSuperUser call). getSuperUser remembers the super user it found and re-checks it per call, instead of iterating a map.

11. Rebased onto main's 07b2c9015 (the deferred-CF-reclamation merge). One textual conflict, in resources/Table.ts's import list: kept main's new dropColumnFamily/markDropInProgress/recordRetiredGeneration/sweepDroppedTableBlobs/storeNameFor/storeNamesFor imports, dropped UserEventMsg per this PR's removal of the ITC user broadcast. No semantic overlap — that work is table-drop generation reclamation, disjoint from user/role lookups.

12. Two correctness bugs the post-rebase review found, both fixed. appendSystemTablesToRole now tolerates a role record with no permission (reachable only via a direct table write or a replicated write, since the operations API requires it) instead of throwing and taking down listUsers() — and with it assertActiveSuperUserRemains (drop_user, alter_user, alter_role, drop_role). readEntry now guards every user/role store read against an id that can't be a key: a username over LMDB's 1978-byte encoded-key limit (measuring the ordered-binary length, not UTF-8 bytes — escaped low control characters expand it) or a non-string/non-number id (a malformed stored role reference) previously reached store.getEntry and threw, turning an unauthenticatable credential into an internal fault instead of a clean 401.

13. readUserEntries's consistency retry is now bounded at 50 attempts (MAX_USER_ENTRY_ATTEMPTS), falling back to "no such user" rather than looping the event loop forever under a sustained write storm on one user record. The alternative — throwing an internal fault instead of a credential rejection after exhaustion — is a one-line change if you'd rather distinguish the two; either fails closed.

14. Three declined findings, left as-is:

    • The hdb_user/hdb_role table subscriptions (security/user.ts around L649) make every commit to system trigger an audit-log scan on a worker with no other system subscription. In the default config, TLS listeners already subscribe to system.hdb_certificate, so the added cost there is per-record dispatch only; a node with no TLS listener pays the new scan.
    • userView allocates a provenance object and a WeakMap entry on every principal, including the session-cookie path, which never calls isCurrentUser on it.
    • The uncached Basic-auth path pre-reads user/role versions before calling server.getUser (security/auth.ts around L263); with the default resolver this is redundant, since findAndValidateUser re-reads the same records. Both are cheap per-request costs, not correctness issues.
    • integrationTests/security/user-change-across-workers.test.ts's rejectedEverywhere samples 32 responses without identifying which worker answered (a 401 carries no threadId), so the revocation half of the test is probabilistic rather than exhaustive (~1e-4 chance of missing a worker, not zero).

Changes

• security/user.ts: the map and its rebuild are gone (usersWithRolesMap, setUsersWithRolesCache). New: systemStore (throws a plain, untagged error when a system table is missing, so a storage outage is never read as a credential rejection); withSystemTablePermissions (now tolerates a role with no permission), shared with listUsers; userView and getUserWithRole; findAndValidateUser on the record; dropUser's existence check; the last-super-user guard on an explicit scan; the subscription runs outside any request context; notifications coalesced to one per event-loop turn. addUser/alterUser/dropUser no longer refresh or signal. readEntry rejects an id too large or the wrong shape to be a key before it reaches the store. Look hardest at the re-check in readUserEntries and holdsStamp: they are the whole consistency argument.
• security/auth.ts: the per-hit isCurrentUser check, versions read before a Basic server.getUser, and the ITC listener that flushed the cache is removed.
• server/liveSubscriptionAuth.ts: the sweep is triggered by onUserChange (header), and a change that lands mid-sweep gets a trailing sweep instead of being dropped by the sweeping guard.
• components/mcp/listChanged.ts: user changes come from onUserChange; schema and resource changes still arrive over ITC.
• security/impersonation.ts (scoped-token name check, lookupUser), security/authn/oidc/tokenExchange.ts (policy user) and security/authn/oidc/trustPolicyOperations.ts (add, list): getUserWithRole replaces map reads.
• Producers that no longer signal: security/role.ts (add, alter, drop), security/tokenAuthentication.ts (refresh-token update), and resources/Table.ts (the apply-loop mark, the post-commit signal).
• ITC USER plumbing deleted: utility/signalling.ts (signalUserChange), server/itc/serverHandlers.js (userHandler, exports), server/threads/itc.js (UserEventMsg), server/threads/manageThreads.js (listener registration), utility/hdbTerms.ts (USER type).
• Startup warm-ups removed: server/operationsServer.ts (setUp), server/fastifyRoutes.ts (side-effect import kept for server.getUser), server/jobs/jobProcess.ts (same).
• resources/Resource.ts: comment now says where live-subscription rechecks read from.
• utility/coalesceRefresh.ts: one run in flight plus one trailing run, carried over from the first version of this PR, now used by the sweep.
• Docs: security/DESIGN.md replaces the coalescing note with the lookup invariant; root index; server/DESIGN.md wording (rejection provenance, system-table carrier).

Verification

Route: new unit tests against real hdb_user/hdb_role records (no stubs of the lookup path), an in-process replication apply-loop test, and a new multi-worker integration test.

• unitTests/security/userRecordLookups.test.js (19 cases; 18 on LMDB, where the reused-version case does not apply):
  • a user written straight to hdb_user is found with no refresh or signal; deactivation and deletion and a role permission change take effect on the next lookup; the derived role is shared until its record changes;
  • a torn read is re-read, for a numbered and a reused version (the stale first read is injected); a real out-of-order hdb_user write returns (it hung the first revision) and is compared by value; getSuperUser re-checks the user it remembered; isCurrentUser transitions;
  • authentication() cache hits: password change, deactivation and role change, and a component-provided principal;
  • onUserChange fires for both tables, contains a throwing or rejecting listener, and a change during a sweep gets its own sweep.
• unitTests/resources/replicatedUserWrites.test.js drives Table.sourcedFrom with a fake source: a replicated user write is visible to the next lookup and notifies; later non-user commits do not notify; a replicated role change reaches the next lookup; a failed commit leaves no user, notification or unhandled rejection; a late write in begin_txn.
• integrationTests/security/user-change-across-workers.test.ts, 4 workers, every worker's authorization cache primed first, with the WhoAmI fixture (config): a role change is seen on every worker, identified by thread id; after a password change and a deactivation, 32 fresh connections all get 401 (a 401 does not name its worker; the chance of missing one of 4 workers is ~1e-4). Passes on RocksDB and LMDB. Negative control: with both the per-hit check and the notification flush removed from dist/, all 3 cases fail ("worker 3 served the role as it was before alter_role").
• Fails on base, on a base dist/ build with a base-compatible subset: a user written straight to hdb_user is not found ("Login failed"), and a cached Basic credential survives a password change (200, expected 401). Both pass here.
• Suites migrated from injected maps to real records through a new testUtils.seedUsers, which writes in one transaction, restores what it replaced, and waits for the notification its writes cause: auth-fastify (role fields asserted instead of a deep-equal on the old cached shape), authn/oidc/tokenExchange, authn/oidc/trustPolicyOperations, impersonation, tokenAuthentication, tokenRejectionClassification (a missing system.hdb_user still propagates untagged), tokenScopeRefresh; user.test.js now restores the module instances it re-requires, which otherwise left a second user.ts installed as server.getUser. Removed with the APIs they tested: signalUserChange, UserEventMsg (and its ipc twin), userHandler; serverHandlers.test.js preloads components/status, which serverHandlers no longer loads transitively. coalesceRefresh.test.js is unchanged apart from a lint fix.
• Gates at c556abce9 (pre-rebase):
  • npm run test:unit:main: 5843 passing, 1 failing: gitCredentials.test.js:395, the dispatch environment's GIT_CONFIG_GLOBAL/GIT_EDITOR (19/19 with them unset).
  • npm run test:unit:resources: 3003 passing, 0 failing.
  • npm run test:integration:all: 2138 pass, 0 fail, 6 cancelled (all in ollama-backend.test.ts, which needs a local Ollama; the same on base).
  • Touched suites on LMDB as well: all pass.
  • npm run typecheck, prettier --check, oxlint on changed files, npm run check:design-docs: clean. Repo-wide oxlint --deny-warnings reports 15 warnings, all in files this PR does not touch.
• Gates at 9eff24291 (post-rebase, current head): test:unit:main 5873 passing, 1 failing (the same known gitCredentials artifact); test:unit:resources 3034 passing, 0 failing; the security integration suite (integrationTests/security/**, 24 suites including the new one) 128 passing, 0 failing. CI green on every leg, including the two prior LMDB-alias-close segfaults (#2721, still open) that hit this PR's earlier heads — a clean re-run each time.
• Review: planning round, then 3 implementation rounds (full, delta, delta) with Codex, Gemini, Cursor Kimi and Harper domain adjudication. Round 1 found the reused-version hang (Gemini, Kimi, domain) and the component-principal gap (Codex); round 2 found the reused-version torn read; round 3 converged with no surviving findings. Post-rebase maintenance added a full round plus 4 delta rounds: found and fixed the malformed-role crash in listUsers, an unbounded consistency-retry loop, and (over two rounds, since the first attempt only checked UTF-8 byte length) the LMDB oversized/malformed-key guard in readEntry; declined the four items in "For the human reviewer" Introduce dependency lockfile #14 as accepted trade-offs or out-of-scope test-coverage gaps. Converged with only previously-adjudicated and pre-existing findings remaining.

— Claude Opus 5.5

Complexity: complicated

Origin — the dispatch brief this PR was written from

Fix user-cache refresh storm from replicated system-database writes

LIVE CONVERSATION about #2776.

You are answering a person, in a thread, one turn at a time. Every turn:

1. Read the whole thread in this dispatch file's # Log — it is the conversation so far, and
   each of your previous turns is in it. Read the PR/issue and the code as needed.
2. Answer the LAST message. Append your answer to # Log as your turn. Prose, not a report:
   they are talking to you, and a status template is not an answer.
3. Set status: needs-input and stop. The thread stays open; their next message resumes it.

Each turn arrives as ASK (answer it, change nothing) or PERFORM (do it, then say what you did) —
the person chose which when they sent it, and the run's own prompt tells you which one this is.
Never infer it from the wording: an unrequested commit in the middle of a discussion and a polite
description of work that was supposed to happen are the two failures this exists to prevent.

Never mark a PR ready and never merge from this conversation.

_{Dispatch: task chat-pr-harper-2776-kriszyp · queued by unknown · ran by claude/opus/xhigh · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=cursor-kimi,codex,gemini; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-muse; rounds=11; full=3 @ 9eff242}

_{Human-Review-Need: 4 (decisions: read-through-user-lookups, retry-exhaustion-as-unknown-user, unkeyable-id-as-absent, notification-requires-audit, remove-user-itc-surface, component-principal-provenance, impersonated-principal-shape) @ 9eff242}