feat: emit user_creation and tenant_disassociation lifecycle events - #1403
Conversation
Emit user_creation atomically with the mutation from the interactive sign-up paths (email-password sign-up and password-hash import, third-party sign-in-up creation, passwordless user creation, WebAuthn sign-up) and tenant_disassociation from Multitenancy.removeUserIdFromTenant, all through startAuditedTransaction using the connection-taking storage variants. Retire the corresponding @UnauditedTransaction allowlist entries (baseline 57 -> 55). Bulk-import user_creation emission is not included. Part of PLAN-010 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Switch the allowApproximate serving path's delta source from the joined-since query to the lifecycle-event fold (CountDeltaInterpreter): served value is now exact-count anchor + fold(events since the anchor), making the approximate count exact for deletions, account linking and unlinking as well as creations. No API surface change; approximate/asOf stay. A window over the interpreter's burst cap re-anchors immediately. Part of PLAN-010. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… shapes on 5.6 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Solid, well-scoped completion of the two deferred PLAN-010 unit-1 emit points. The retry-outside / mutation-inside pattern is right (each DuplicateUserIdException retry opens a fresh startAuditedTransaction, so no aborted-connection reuse), folding the fake-email verify onto the same connection is a genuine atomicity improvement over the old split transaction, and the withoutAudit no-op paths in removeUserIdFromTenant are correctly justified. The deferral of bulk-import user_creation is consistent — I confirmed the bulk paths go through the batch methods (createMultipleUsersWithPasswordHash / createMultipleThirdPartyUsers / createPasswordlessUsers), not the single-user methods touched here, so no proxy-storage collision.
One concrete gap and one behavioural question below. The gap: WebAuthn is listed as a converted user_creation path in the PR body but has no test asserting it emits the event — every other creation path got one. Since this swaps a raw startTransaction for startAuditedTransaction and adds the emit inside the pre-existing retry loop, it's changed behaviour that should be pinned by a test like the other four. (I did not run the build/matrix; findings are from reading the diff against the surrounding code.)
Add two direct serving-path assertions requested in review, previously covered only indirectly via the interpreter/fold unit tests: - apiApproximateReflectsUnlinkingViaFoldWithoutARefresh: an account unlink between two requests off one primed anchor (no refresh) is reflected immediately as +1, and agrees with the exact recompute. - apiApproximateReAnchorsOnCorruptLedgerPayload: a corrupt lifecycle payload in the fold window forces serve() to re-anchor to a fresh exact count; proven observable by a concurrent real deletion so the recompute (2) is distinct from the primed anchor (3). Both run green in the in-memory SQLite sandbox (ApproximateUserCountTest 11/11). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-removal FK invariant Address review on PR #1403: - add webAuthnSignUpEmitsUserCreation (emit inside the audited retry loop) - add fakeEmailSignUpVerifiesAndEmitsUserCreation (pre-verify folded onto the audited connection) - note the FK invariant behind the not-lockable no-op branch in removeUserIdFromTenant - shorten the CHANGELOG entry to a one-liner Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review of 9da1fb3 — the follow-up commit resolves all three points from my previous pass and I found nothing new to block on.
- WebAuthn test —
webAuthnSignUpEmitsUserCreationdrives the real HTTP register flow viawebauthn.Utils.registerUserWithCredentials(signature matches) and pins exactly oneUSER_CREATIONrow with the response's recipe id andpublictenant. This was the one emit landing inside the pre-existingwhile(true)retry loop, so a dedicated assertion is the right call. - Fake-email test —
fakeEmailSignUpVerifiesAndEmitsUserCreationusesst-user@stfakeemail.supertokens.com, whichUtils.isFakeEmailrecognises, and asserts both halves of the moved path: the user comes out verified (the pre-verify folded onto the audited connection) and exactly oneuser_creationis emitted. Matches the atomicity claim inEmailPassword.signUp. - Tenant-removal FK invariant — the added comment on the
UserNotFoundForLockingExceptionno-op branch spells out the invariant (an auth user's tenant mapping cannot exist without the user row) and the parity withaddUserIdToTenant, which is the right justification for the unreachable-in-a-consistent-DB branch. - CHANGELOG — now the one-liner the maintainer asked for.
readEvents filters on event_type across a fresh per-test process with a single created user, so the assertEquals(1, ...) counts are safe. I did not run the build/matrix; findings are from reading the diff against the surrounding code.
|
For the bulk import questions: |
Completes the deferred bulk-import point of PLAN-010 unit-1 per the maintainer decision on #1403: a bulk-imported user is recorded as a new `user_import` lifecycle event (counted toward user totals exactly like `user_creation`, but under its own type so the last-active rollup — which an interactive sign-up may feed, but an import must not — can exclude imports), plus one `tenant_association` for every remaining tenant the user lands in. - New `USER_IMPORT` event type, payload builder, and LifecycleAuditEvent factory (tenant-only payload, mirroring user_creation). - CountDeltaInterpreter folds user_import identically to user_creation (+1 in its tenant); the rollup reads only user_last_active/ account_linking, so it already excludes any lifecycle type. - BulkImport.processUsersImportSteps emits the events via startAuditedTransaction on the proxy's held connection, so they land atomically with the import and roll back with it. - Tests: interpreter/payload coverage for the new type and the emit sequence (in-memory); Postgres-gated integration test asserting the emitted user_import + tenant_association for single- and multi-tenant imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Implemented in 7097d1a, following your decision. Emit (bulk import): per imported user, one Counting vs. rollup: added a Tests: interpreter/payload unit coverage for the new type and for the exact emit sequence ( Updated the CHANGELOG bullet accordingly (dropped the "bulk-import |
…sue-1397-emit-creation-disassoc # Conflicts: # CHANGELOG.md # src/test/java/io/supertokens/test/accountlinking/LifecycleMutationEventTest.java
…to agent/issue-1379-default-flip # Conflicts: # CHANGELOG.md
It would be great if no |
tamassoltesz
left a comment
There was a problem hiding this comment.
It would be great if no user_last_active event was ever written, rather the actual activity would be counted in the last active fold. Is it possible?
|
Good question — but I think it lands on the wrong PR, and the encouraging part is that the design already works the way you're describing. Scope of this PR. #1403 only adds the lifecycle events — On "is it possible". In the current design What can't be removed for free is the event itself. Sign-ins/refreshes/sign-ups have no other representation in the activity log, so the throttled So there's nothing to change in this PR to satisfy that. If you'd like to reshape the last-active event model itself (e.g. a different activity-signal vocabulary the fold reads, or dropping the projection table and folding at read time), that's a change to the last-active rollup path rather than this lifecycle-emit unit — I'm happy to take it there, but I'd want your call on the target shape before re-architecting that path, since it's a behavioural decision with a read-latency / storage tradeoff. Want me to open a separate issue to track it? |
There was a problem hiding this comment.
Re-review of the user_import follow-up (7097d1a) added since my last pass — the bulk-import point that was deferred, now implemented per the maintainer's decision. It's correct and well-scoped:
- Atomicity —
emitBulkImportLifecycleEventsrunsstartAuditedTransactionon the sameBulkImportProxySQLStorage, so itsstartTransactionreuses the proxy's single held connection and itscommitTransactionis the proxy no-op. The events therefore land atcommitTransactionForBulkImportProxyStorageand rewind onrollbackTransactionForBulkImportProxyStorage/rollbackToSavepoint, identical to the sibling steps (verifyCollectedEmailAddressesForUsers, roles, etc.). Verified against both callers ofprocessUsersImportSteps: the single-userimportUserpath and the cronimportPartitionbatch path. - Count semantics —
USER_IMPORTfolds identically toUSER_CREATION(+1 in its tenant) inCountDeltaInterpreter, and the per-group emit sequence (oneuser_importfor the first tenant + onetenant_associationper remaining tenant, withgroupBeforebuilt up incrementally) yields exactly +1 per distinct tenant.CountShadowAudit.LIFECYCLE_EVENT_TYPESis derived fromLifecycleEventType.values(), so the new type is picked up by the exact-vs-fold reconcile automatically, and the last-active rollup query (event_type IN ('user_last_active','account_linking')) correctly excludes it. Payload serialize/parse/validateall handle the new case. - Tests —
LifecycleEventFoldTestpins the fold and the 3-tenant emit sequence;BulkImportLifecycleEventTest(Postgres-gated) asserts the single- and multi-tenant emits including the association's before-presence. Good coverage.
No blocking defects — event COMMENT. One minor coverage note inline. Separately, the maintainer's open question about not writing user_last_active events at all is a design thread on a different mechanism (this PR writes no user_last_active rows), so it doesn't gate this diff's correctness. Findings are from reading the diff against the surrounding code; I did not run the build/matrix.
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>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
…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>
feat: serve default user counts as anchor plus ledger fold
…ue-1378-ledger-fold
feat: serve approximate user counts from the lifecycle-event fold
There was a problem hiding this comment.
Re-review at cf0c4250.
The #1397-owned changes are unchanged and still sound. Diffing my last-reviewed commit (8b7e99dd) against this head, none of the actual issue-#1397 files moved — the user_creation / tenant_disassociation / user_import emit paths (EmailPassword, ThirdParty, Passwordless, WebAuthN, Multitenancy, BulkImport, auditlog/lifecycle/*) and their tests are byte-for-byte what I already reviewed as correct. No new defects there.
One thing to fix before this merges: the branch has absorbed two downstream stacked PRs. The only diff since 8b7e99dd is the merge commit cf0c4250 ("Merge pull request #1404 from supertokens/agent/issue-1378-ledger-fold") — a backwards merge that folds child branches into this parent. As a result PR #1403's diff now also carries, out of scope:
ApproximateUserCount.javaserve-from-fold rework (+152/-66) andApproximateUserCountTest.java(+280) — that's #1404 / issue #1378 (ledger-fold).UsersCountAPI.javadefault-flip — that's #1405 / issue #1379.- Two CHANGELOG bullets (
allowApproximate ... serves the exact anchor plus a fold;From CDI 5.6 the default ... serves anchor + fold) belonging to those same two PRs.
That code is fine (it was reviewed on its own PRs), but merging #1403 as-is would land #1404 and #1405 into feat/activity-log under this PR, bypassing their own review sign-off and muddying the ledger of what #1397 changed. Recommend restoring the branch to 1397-only scope — reset to 8b7e99dd and let stack-sync re-derive the forward merges — before this goes in. Flagging rather than blocking since it reads as a stack-sync mis-merge rather than an authoring mistake.
Separately, the maintainer's open question ("no user_last_active event ever written, count activity in the last-active fold instead") is a design thread on a different mechanism — this PR writes no user_last_active rows — and the author has already answered it and offered a separate issue. It doesn't gate this diff. Findings are from reading the diff against the surrounding code; I did not run the build/matrix.
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>
Add a BulkImportLifecycleEventTest case for a linked group whose two login methods live in different tenants (email-password in public, third-party in t1), so distinctTenantIdsInOrder must union tenants across login methods rather than read them off a single method. Asserts the same one user_import (public) + one tenant_association (t1, before=[public]) sequence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review at 8ce64d1e. The only change since my last pass (cf0c4250) is the cross-login-method bulk-import test linkedGroupSpanningLoginMethodsUnionsTenantsAcrossThem that @tamassoltesz asked for — no source moved. The test is correct and closes the gap the earlier two multi-tenant cases left (both reached the multi-tenant branch via a single login method carrying two tenantIds, so they never exercised the union across login methods). This one puts EP in public and TP in t1, and pins exactly one user_import (tenant public, group = primary EP id) plus one tenant_association (tenant t1, groupBefore=[public]) — which is precisely what distinctTenantIdsInOrder's order-preserving union produces, and a regression reading tenants off a single login method would drop the association. Constructor arg order (isVerified, isPrimary) and the $2a/BCRYPT placeholder match the existing BulkImportTestUtils convention. Like the sibling cases it's Postgres/MySQL-gated (early-returns on in-mem), so it runs in CI rather than the in-mem suite. No new blocking issues; the #1397 lifecycle-emit content remains as previously reviewed.
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>
…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>
…ctivity-events feat: replace user_last_active event with semantic activity events
Problem
PLAN-010 unit 1 (issue #1374) wired lifecycle events at the count-affecting
mutation points that were already transactional in core (
user_deletion/user_group_deletiononAuthRecipe.deleteUser,tenant_associationonMultitenancy.addUserIdToTenant). Two points were deferred because theyneeded connection-taking storage variants that didn't exist yet:
user_creation(the recipe sign-up paths don't own the insert transaction) and
tenant_disassociation(Multitenancy.removeUserIdFromTenantwasn'ttransactional at all).
Those storage variants have since landed on
feat/activity-log(plugin-interface #217, core in-memory #1399, postgresql #393), so this issue
(#1397) completes the two deferred points.
Fix
All emits go through
ActivityLogSQLStorage.startAuditedTransaction, so theevent and the mutation it records commit (or roll back) on the same connection.
user_creation— the interactive sign-up paths now create the user withthe connection-taking
signUp_Transaction/createUser_Transactionvariants and emit one
user_creationevent (carrying the tenant) inside thetransaction. The user-id retry loop on
DuplicateUserIdExceptionstaysoutside the transaction, as specified:
EmailPassword.signUp(interactive) — also folds the fake-email pre-verifyonto the same connection, so it no longer opens a raw transaction.
EmailPassword.createUserWithPasswordHash(password-hash import) — thecreate is audited; the duplicate-email password-update branch stays a
separate (still-allowlisted) transaction.
ThirdParty.createThirdPartyUser(the sign-in-up create path).Passwordless.createPasswordlessUser.WebAuthN.signUp— the whole method was already one transaction, so this isa clean swap to
startAuditedTransaction.tenant_disassociation—Multitenancy.removeUserIdFromTenantisrewritten around
startAuditedTransaction: it locks the user, snapshots thegroup's before-presence, calls
removeUserIdFromTenant_Transaction, and emitsonly when a mapping was actually removed (
withoutAuditotherwise). Thenon-auth-recipe cleanup that precedes it is not count-affecting and stays a
separate step, as specified.
LifecycleAuditEvent.forUserCreation/forTenantDisassociationfactory methods (the payload builders already existed).
@UnauditedTransactionon the two methods whose only rawtransaction was the (now-audited) creation:
EmailPassword.signUpandWebAuthN.signUp. The shrink-only baseline drops 57 → 55(
AuditEnforcementBaselineTest).Not included: bulk-import
user_creationThe issue also asks for one
user_creationper user created by the bulk-importpaths. I've left this out of this PR and want a decision before doing it,
because it isn't mechanical:
(
loginMethod.tenantIds), whereas an interactive creation always happens inexactly one tenant (extra tenants come later as
tenant_associationevents)."One event per created user" then undercounts a multi-tenant import by
N-1per user. The correct shape is either oneuser_creationfor a primarytenant plus
tenant_associationfor the rest, or oneuser_creationpertenant — a ledger-semantics call I don't want to guess.
BulkImportProxySQLStoragewith its owncommit/rollback (
commitTransactionForBulkImportProxyStorage), so it can'tuse the
startAuditedTransactioncombinator directly; events would have to bewritten via
createActivityLogEntry_Transactionon the proxy connectioninside
processUsersImportSteps, and the batch create methods returnvoid(no created-id list to iterate).
Consequences: on this branch, bulk-imported users currently have no
user_creationevent, so the ledger is not yet complete for that path (theshadow audit would flag it). Everything else is complete.
Tests
Added to
LifecycleMutationEventTest(in-memory SQLite):user_creation: email-password sign-up, password-hash import, third-partysign-in-up (event on create only, not on subsequent sign-in), passwordless
create — each asserts exactly one event, the correct recipe/group id, and the
schema-valid payload tenant.
tenant_disassociation: emits once with the group's before-presence and thetenant on a real removal; re-removal is a no-op that emits nothing; removing
a user never associated with the tenant emits nothing.
AuditEnforcementBaselineTestbaseline to 55.Atomicity (event commits with the mutation, rolls back together) is provided
structurally by the shared
startAuditedTransactioncombinator, which has itsown coverage; I did not re-prove per-path rollback with injected failures.
Ran locally (in-memory, SQLite)
LifecycleMutationEventTest+AuditEnforcementBaselineTest— 16/16 pass.webauthn.CredentialRegisterFlowTest,passwordless.PasswordlessConsumeCodeTest,emailpassword.UserMigrationTest,multitenant.TestTenantUserAssociation,thirdparty.api.ThirdPartySignInUpAPITest4_0,emailpassword.api.SignUpAPITest5_0,accountlinking.EmailPasswordTests—34/34 pass.
Not verified locally
heavy matrix is gated behind the
run-testslabel on PRs).Cross-SDK note
supertokens-node is the reference implementation; python/golang port its
semantics. This change is core-only and rides on the
feat/activity-logactivity-log ledger, which has no node/python/golang analogue yet — no port is
implied by this PR.
Fixes #1397