feat: replace user_last_active event with semantic activity events - #1408
Conversation
Retire the synthetic user_last_active activity event in favour of a semantic activity vocabulary (sign_in, token_refresh, session_create, sign_out, oauth_token_exchange, oauth_authorize) emitted from the matching API handlers. Sign-up and created-new-user paths emit no activity event and drop their updateLastActive call — the in-transaction user_creation lifecycle event already records that activity and the fold reads it. sign_in/sign_out are unthrottled; the rest keep the shared 5-minute throttle. The in-memory last-active fold and hasUnfoldedActivitySince now read the new fold set (the six activity events plus user_creation and account_linking); user_import and every other lifecycle type stay excluded, and no writer of user_last_active remains. Linking drops its emitLastActiveAuditLog call — the account_linking event now both credits the primary in the fold and drives the reconcile. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review of the semantic-activity-event replacement. The core change reads cleanly and is correct on the tested in-memory store: all updateLastActive call sites are migrated to the typed overloads, tenantIdentifier is in scope at every emit site I checked (OAuth/Refresh/Session), the fold and hasUnfoldedActivitySince both draw from the single LastActiveFoldEvents set (so they can't drift), and the disjoint reads are right — the count fold pulls lifecycle types by an explicit IN list while the last-active fold pulls FOLD_EVENT_TYPES, with user_creation/account_linking in both by design. Removing the redundant emitLastActiveAuditLog from updateLastActiveAfterLinking (now that account_linking is folded and credits the primary) and dropping the sign-up pings (now that user_creation is folded) correctly eliminates the double-count the description calls out. ActivityEventTypeTest + foldCreditsEachIncludedTypeAndIgnoresExcludedTypes give good coverage of the type vocabulary and fold membership.
Two things below — one reinforcing @tamassoltesz's open comment, one a test-coverage gap. Not blocking beyond the change already requested; flagging with event=COMMENT.
- Audit integrity (agreeing with the requested change): the semantic events go through best-effort
AuditLog.emitoutside any transaction, whereas the fold's reliable recency anchors are the two transactional lifecycle events (user_creation,account_linking). So for DAU/MAU counting a dropped best-effort row is self-healing (the next event re-credits the user), which is why the tested behavior holds. But if these rows are meant to stand as a true audit trail — which is @tamassoltesz's point — best-effort/out-of-transaction undermines that, and the direction of moving them onto the audited-transaction mechanism is the right call to settle before merge. - No CDI/plugin-interface surface change and this targets the feature branch, so no semver concern and no node/python/golang port beyond the paired
#398.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review. Since my last pass (12:14) the only change on this branch is the changelog condensation (9957bcb) — the semantic-activity code is unchanged — so this isn't a review of new code.
The blocking item is still @tamassoltesz's requested rework (each activity event on its own startAuditedTransaction, fail-loud, throttle dropped), now confirmed in his last reply. Not re-litigating that; it's the substantive work still to come. Below is one orthogonal correctness concern that the audited-transaction rework won't fix on its own, plus the test gap that hides it.
Per review on #1408: activity events are a true audit trail, so ActiveUsers.updateLastActive now writes each event through startAuditedTransaction — the same fail-loud, atomic mechanism the lifecycle events use — instead of a best-effort AuditLog.emit. The per-(app,user) throttle and its recency cache are removed (a true audit cannot skip rows), so every activity is recorded and a write failure fails the originating auth request. Call sites that swallowed the write failure (session create/refresh/revoke, oauth token/authorize) now let it propagate. Adds an API-level test pinning the createdNewUser gate (sign-up writes no sign_in row, returning sign-in writes exactly one). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review of the audited-transaction rework (2e1886b) that implements @tamassoltesz's requested direction. The mechanism swap is clean and correct on the tested store: every activity emit now goes through startAuditedTransaction (the same combinator lifecycle events use), the throttle/recency cache are fully removed with no dangling wasRecentlyActive/markRecentlyActive/clearCacheForTesting callers, markDirty is preserved (and correctly only on the success path), and the OAuth sites carefully distinguish a vanished session (UnauthorisedException → skip, not a failure) from a real write failure. The new SemanticActivityEventGatingTest directly closes the createdNewUser coverage gap I flagged earlier — good.
One substantive concern to settle before merge, raised inline: at every one of these emit sites there is no surrounding mutation transaction to co-commit into, so unlike the lifecycle events (which are truly atomic with their mutation) "fail-loud" here fires a separate audited transaction after the primary auth operation has already committed. The net effect is that a transient activity_log write error now turns an already-succeeded — and for OAuth, externally non-reversible — auth operation into a client-visible 500. Worth an explicit call from the maintainer on whether that trade is acceptable for these specific paths (the earlier markDirty gap on the lifecycle fold sources is unchanged and still tracked in my prior comments). Flagging with event=COMMENT.
…figurable Per review on #1408. Reverts the fail-loud audited-transaction write for the semantic activity events (2e1886b) back to a best-effort AuditLog.emit, so a transient activity_log write error can no longer turn an already-committed — and for OAuth externally non-reversible — auth operation into a 500 or drop an already-rotated refresh token (@tamassoltesz: "keep them best-effort, but don't drop already refreshed tokens and lie to the user"). Restores the per-(app, user) throttle and its recency cache, and gates them on a new protected config activity_log_throttle_enabled (boolean, default true, per connection URI domain, mirroring activity_log_retention_days). When off the throttle and its cache are bypassed and every activity is recorded as its own row; the RefreshSessionAPI short-circuit is likewise bypassed so every refresh records. Keeps the API-level createdNewUser gating test from 2e1886b. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Round-4 reviewer pass on 35f861039 (the config-throttle commit; prior passes covered 145a376b/9957bcb/2e1886b0). This commit is a faithful git-revert of the round-2 fail-loud audited-transaction back to best-effort, plus the new activity_log_throttle_enabled core config — exactly the two calls @tamassoltesz made on the RefreshSessionAPI and throttle threads ("keep them best-effort, don't drop already-refreshed tokens" and "tie the throttle to a core config"). It reads correctly:
- Best-effort restored consistently.
updateLastActiveno longer throwsStorageQueryException; the write goes back throughAuditLog.emit(self-swallowing), and all five sites (RefreshSessionAPI/SessionAPI/SessionRemoveAPIviacatch StorageQueryException, both OAuth helpers viacatch Exception) swallow again, so a transientactivity_logerror no longer 500s an already-committed refresh/session/sign-out or strands an already-issued OAuth token. ThemarkDirtyon the semantic events is restored with them. - Config mirrors
activity_log_retention_dayscleanly —@NotConflictingInConnectionUriDomain+@JsonProperty+@HideFromDashboard, added toPROTECTED_CONFIGS, getter, and commented in bothconfig.yaml/devConfig.yaml. Defaulttruepreserves current behavior.RefreshSessionAPIcorrectly gates itswasRecentlyActiveshort-circuit on the config so that off ⇒ every refresh records (cache stays empty ⇒wasRecentlyActivefalse), which is internally consistent withupdateLastActiveskipping the cache when off. - Semver/API-contract: purely a self-hosted core knob (protected, hidden from dashboard), no CDI/plugin-interface surface, no breaking change; the paired PostgreSQL fold change (#398) is unaffected by the throttle config (throttle is core-side in-memory).
No new defects in this delta. Two items for you, both non-blocking:
- Default decision is still yours. It ships
true(preserves today's throttled behavior). If you'd rather the complete audit trail be the out-of-the-box behavior, flip the default tofalse— flagged on the throttle thread. - The previously-flagged
markDirtygap onuser_creation/account_linking(fold-on-backstop-not-tick for signup-only/link-only users) is unchanged here and remains the right separate follow-up.
The last-active fold credits the user_creation and account_linking lifecycle events, but those are written via startAuditedTransaction, which — unlike updateLastActive — never marks the RollupUserLastActive dirty signal. So a user who only signed up (or was only linked) and produced no other activity was not folded into user_last_active until the periodic backstop pass (up to BACKSTOP_EVERY_N_TICKS intervals), a promptness regression versus the pre-semantic-event behaviour where sign-up went through updateLastActive -> markDirty and folded on the next tick. Restore the signal at the layer it was lost: the sign-up API handlers (emailpassword/thirdparty/passwordless/webauthn) mark the rollup dirty for the created-new-user branch via a new ActiveUsers helper, and updateLastActiveAfterLinking marks it for the account_linking credit. This re-adds only the dirty nudge, not the redundant sign_in activity row the semantic-event work deliberately removed. Marking dirty only signals that there is something to fold, so it is idempotent and safe to over-signal. Adds LastActiveFoldPromptnessTest: after an HTTP sign-up, a non-forced rollup pass (one that folds only when the dirty flag is set) folds the new user — which fails if the sign-up's dirty signal is missing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review of the head commit da767fe1 ("wake the last-active rollup on user_creation and account_linking") — the only new code since round 4. It directly implements @tamassoltesz's "implement, don't defer to another issue" ask, and it's correct on the tested in-memory store.
The gap it closes is real and precisely diagnosed: user_creation/account_linking feed the fold but are written via startAuditedTransaction, which never touches RollupDirtySignal, so a sign-up-only / link-only user folded only on the periodic backstop. The new markLastActiveRollupDirty re-adds just the dirty nudge (not the sign_in row the semantic-event work deliberately dropped) at the four sign-up handlers' created-new-user branches and in updateLastActiveAfterLinking. Branch placement is right (new-user branches only; existing-user branches keep the sign_in ping), the dirty pool is the app public-tenant pool on both paths (matches emitActivityAuditLog), and the nudge is co-located with the sole account_linking emit path (LinkAccountsAPI, only when !wasAlreadyLinked). LastActiveFoldPromptnessTest pins the sign-up half well by folding through a non-forced pass that only the sign-up's markDirty can arm.
Two non-blocking observations below; the change looks good to me.
Replace the core-local io.supertokens.auditlog.lifecycle.ActivityEventType and LastActiveFoldEvents with the shared plugin-interface vocabulary (io.supertokens.pluginInterface.auditlog.ActivityEventType and RollupEventTypes.FOLD_SET), merged in supertokens-plugin-interface#220, so the fold's event-type set has a single source of truth across core and the SQL plugins. The plugin-interface enum deliberately carries no throttle flag (its javadoc keeps throttling policy core-side), so the throttle classification moves to ActiveUsers.isThrottled(ActivityEventType) with identical behavior (sign_in/sign_out unthrottled, the rest throttled). No behavior change: same event_type strings, same fold set and SQL IN-list, same throttle policy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review of the head commit 65aaf78d ("consume shared plugin-interface activity-event vocabulary"), which addresses the last CHANGES_REQUESTED (adopt the vocabulary merged in plugin-interface #220).
The change does exactly that, and faithfully. I verified it is behavior-preserving:
- Event strings — the shared
pluginInterface.auditlog.ActivityEventTypecarries the same six values (sign_in,token_refresh,session_create,sign_out,oauth_token_exchange,oauth_authorize) as the deleted core-local enum; only the declaration order ofSIGN_OUTdiffers, which is irrelevant since the fold uses set membership. - Fold set —
RollupEventTypes.FOLD_SET= the six activity events +user_creation+account_linking, identical to the oldLastActiveFoldEvents.FOLD_EVENT_TYPES;user_importand the retireduser_last_activeremain excluded. The SQLIN-list order doesn't affect the query. - Throttle policy —
sign_in/sign_outunthrottled, the rest throttled — unchanged. - The emit sites are pure import swaps (constant names are identical across the two enums), and
ActivityEventTypeTeststill hard-pins the contract viaSet.of(...)equality onFOLD_SET, so any future PI drift breaks the test rather than silently disabling the fold.
Dependency is satisfied: PI #220 merged into feat/activity-log at 04:12 (before this commit) and RollupEventTypes exists there. CI is green (the heavy test matrix is label-gated, not failing).
One open decision for you to ratify (line comment below) — the throttle was kept core-side rather than moved into the plugin interface. Otherwise this is a clean, faithful adoption. No blocking issues; leaving as a comment.
…s test updateLastActiveAfterLinking marks the rollup dirty so a link with no other activity folds the primary on the next tick rather than the backstop pass. Add a link-only variant of the promptness test that folds through a non-forced (dirty-gated) cron pass; it fails if the markDirty nudge is removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
0712835
into
agent/issue-1397-emit-creation-disassoc
Stacked on
agent/issue-1397-emit-creation-disassoc(parent issue #1397). The diff shown here is only this issue's commit relative to that parent branch; the PR targets it, notdev/feat/activity-log.Problem / root cause
The last-active rollup fed on a single synthetic
user_last_activeactivity event thatActiveUsers.updateLastActiveappended for every interaction. Per the discussion on #1403, that synthetic event should not exist: the concrete interaction (a sign-in, a refresh, a session create, …) is the activity, and the fold should count those directly. A synthetic-event indirection also double-recorded sign-up activity — once asuser_last_activeand once as the in-transactionuser_creationlifecycle event.Fix
ActivityEventTypevocabulary (alongsideLifecycleEventType):sign_in,token_refresh,session_create,sign_out,oauth_token_exchange,oauth_authorize. Each carries its throttle class.ActiveUsers.updateLastActivenow takes the activity type. Cache, throttle, and dirty signal stay centralized.sign_in/sign_outare unthrottled; the rest keep the shared 5-minute per-(app, user)throttle. Every emit refreshes the recency cache, sowasRecentlyActivekeeps its meaning. A tenant-taking overload emits with the request's tenant (tenant_idcolumn) where the handler has it; the app-only overload preserves today's public-tenant behavior. The activity log/projection still live on the app's public-tenant storage, so the fold (grouped byapp_id) and the count read see the same rows.SignInAPI, webauthnSignInAPI, thirdpartySignInUpAPI(sign-in branch only), passwordlessConsumeCodeAPI(existing-user branch only) →sign_in;RefreshSessionAPI→token_refresh;SessionAPI→session_create;SessionRemoveAPI→sign_out;OAuthTokenAPI(both call sites) →oauth_token_exchange;OAuthAuthAPI→oauth_authorize.updateLastActivecall (emailpasswordSignUpAPI, webauthnSignUpWithCredentialRegisterAPI, and the created-new-user branches of thirdparty/passwordless) — the in-transactionuser_creationlifecycle event records that activity and the fold reads it.emitLastActiveAuditLogcall and throttle bypass fromupdateLastActiveAfterLinking; theaccount_linkingeventAuthRecipe.linkAccountsemits atomically now both credits the primary in the fold (viaprimary_or_recipe_user_id) and drives the reconcile delete of the recipe user's row. The projection-delete + cache eviction stay as a latency optimization.hasUnfoldedActivitySinceread the new fold set: the six activity events +user_creation+account_linking.user_importand every other lifecycle type stay excluded (imported != active — decided on feat: emit user_creation and tenant_disassociation lifecycle events #1403), as does the retireduser_last_active. The set is defined once inLastActiveFoldEventsso the fold query and the existence check cannot drift.user_last_activestring no longer appears as a written or read event type (the identically-named projection table stays).Storage half — must merge together
supertokens-postgresql-plugin#398is the fold/hasUnfoldedActivitySincechange for the PostgreSQL plugin; it uses the same fold set. Either half alone leaves the branch's emit and fold sets mismatched, so the two must merge together. The plugin half has not been pushed yet at time of writing; when it is, it should use a branch name matching this one so CI builds them together (the heavy matrix that would exercise that coupling is skipped for this PR — see CI note below).Tests
Added / updated (all against the in-memory SQLite store;
-p 1not applicable — core in-memory suite):ActivityEventTypeTest(new): event-type strings, throttle classification (sign_in/sign_outunthrottled, rest throttled), and that the fold set is exactly the six activity events +user_creation+account_linking(excludesuser_importanduser_last_active).ActivityLogRollupTest: newfoldCreditsEachIncludedTypeAndIgnoresExcludedTypes(each included type credited,user_import/account_unlinking/user_deletionignored,account_linkingcredits the primary); existing fold tests reseeded with a folded activity type;reconcileRemovesRecipeUserLinkedAwayInWindownow asserts the primary is credited at the link time (new behavior —account_linkingis folded).RollupUserLastActiveTest,ActivityLogRollupParityTest,ActivityLogWindowReadTest, andTestAppDataoff the retired event name onto folded activity events.Ran locally (in-memory SQLite), all pass except one pre-existing red:
ActivityLogRollupTest,RollupUserLastActiveTest,ActivityLogRollupParityTest,ActivityLogWindowReadTest,ActivityEventTypeTest,LifecycleEventPayloadTest— green.ActiveUsersTest— green excepttestMauSeriesWithActivityAcrossDayBuckets, which fails with amaus-stat NPE. Pre-existing: it fails identically on the parent branchagent/issue-1397-emit-creation-disassoc(license-gated EE stat, unavailable offline), unrelated to this change.accountlinking.api.ActiveUserTest,accountlinking.LinkAccountsTest,multitenant.api.TestTenantUserAssociation— green.SessionAPITest*(incl.activeUsersTest),RefreshSessionAPITest*,SessionRemoveAPITest*, webauthn sign-in/up API suites,InMemoryDBTest— green.Not verified locally
OAuthTokenAPI/OAuthAuthAPI) — they need the OAuth provider container; the handler changes compile and CI covers them.Notes for the reviewer
updateLastActive. Now sign-up activity is theuser_creationlifecycle event, which lands on the mutation's storage. On the in-memory store all tenants share one DB (getUserPoolId()=="same-user-pool"), sotestThatActiveUserDataIsSavedInPublicTenantStoragestill passes. For a non-public tenant on its own database, a user who only ever signs up there (never signs in) would fold into that tenant's storage rather than the app's public storage — the same pre-existing "active users are tracked on the public tenant" limitation the code's own TODOs call out. Flagging for the PostgreSQL half's review.Release Branch Testsmatrix skips by design for a non-release-branch base, so the cross-repo (core↔plugin) coupling is not exercised here; local in-memory runs are the functional check.Cross-SDK note
supertokens-node is the reference implementation. This is an internal activity-log/fold change with no CDI-visible surface (event types are internal strings, no new API, no plugin-interface change), so there is no node/python/golang port to flag beyond the paired PostgreSQL storage change (#398).
Part of PLAN-011.
Fixes #1407