Step 1: put the Circle and AppRegistrations tables to use - #1688
Open
toddmitchell wants to merge 23 commits into
Open
Step 1: put the Circle and AppRegistrations tables to use#1688toddmitchell wants to merge 23 commits into
toddmitchell wants to merge 23 commits into
Conversation
Definitions lived in the shared key-three-value blob, which is why AppId and GrantOn could never be queried or constrained -- the columns shipped dormant with the drive-addressing schema work and nothing was ever wired to them. TableCircle had no caller at all. Putting the definitions in the table is what makes those columns usable: asking "which circles enrol on connect?" is a WHERE GrantOn = ? against an indexed column, not a load-all-deserialize-filter over opaque rows. - CircleDefinition gains AppId, GrantOn, Designation and Emoji. CircleDefinition is both the stored shape and the wire shape -- CircleDefinitionControllerBase serves it directly and takes one as an update body -- so the fields stay on the wire and the blob copy is cleared inside ToRecord instead, the same clear-before-serialize trick ToConnectionsRecord uses for the grant collections. Nothing in the blob can drift from the column, because deserializing the blob alone yields defaults. - Equality and GetHashCode account for the four, since EnsureSystemCirclesExist reconciles definitions by comparing them. - CircleGrantOn and CircleDesignation are new enums matching the column values. Every existing circle is None/Personal, so nothing changes behaviour until something sets them. - AppId is not taken from an update request. Ownership is set when the circle is created and must not be reassignable by anyone who can PUT a definition. - TableCircle grows UpsertAsync and GetAllAsync; TableCircleCached wraps both and invalidates the all-key alongside the per-circle key. - CircleDefinitionService moves off ThreeKeyValueStorage onto db.CircleCached with ToRecord/FromRecord doing the column-vs-blob split. - v12 -> v13 copies existing definitions across. Idempotent and additive: a definition already in the table is left alone, so a partial run repeats safely. The blob rows are deliberately left in place -- if this goes wrong the source data is still there. Cleaning them up is a separate job. Tests pin both directions: no promoted value survives into the blob, the caller's object is intact afterwards, the fields round-trip through the record, they are visible on the wire, and an update body echoed back does not reset GrantOn. No behaviour change. The four fields take their defaults for every existing circle, which is what they already were. Squashed from PR #1661 (commits 0795220, 3ea043d) onto main; the migration is renumbered v13->v14 to v12->v13 because the review-stamp work it originally sat on is not here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The other half of the drive-addressing groundwork. The table shipped with the schema work and had no caller; AppRegistrationService still wrote to the shared key-three-value blob, where UNIQUE(identityId, AppSlug) cannot be expressed at all. Since the slug is a wire address other identities resolve against, a best-effort uniqueness check over opaque rows is not good enough. The table also gives Circle.AppId and Drives.AppId a real target. Slugs have to be coined, because no registration has one and the column is NOT NULL: - Known system apps get their obvious name -- chat, feed, mail, photo, owner. These are the addresses drive addressing assumes. - Everything else derives from the registration's display name, the only human-meaningful thing on the record. - The whole set is resolved and checked before anything is written. GenerateAll orders known apps first, so an app called "Chat" cannot take the chat app's address, disambiguates collisions with a numeric suffix, and throws rather than returning a duplicate. The migration re-checks distinctness on top of that. A half-migrated app table with a slug collision is much worse than a migration that refuses to start. - A name that slugifies to nothing falls back to the app id. Unreadable, always available, and better than refusing to migrate. AppId, AppSlug, Name and CorsHostName become columns and are [JsonIgnore]d out of grantJson, so a query on a column cannot disagree with the hydrated object. Everything else still rides the JSON. Registering a new app derives a slug the same way, so registration and migration land on the same value; the request has no slug field yet, that arrives with drive addressing. Updates carry the existing slug forward -- it is immutable, and other identities may already hold it. Dedupe seeds from stored slugs rather than re-derived ones, so an app holding "acme-2" still holds it whatever its name would slugify to today. The rows move in v12 -> v13, the same version step that moves the circle definitions -- both are the same job, they ship together, and a tenant is either on the tables or on the blob. Idempotent and additive: an app already present is skipped so its slug is never reassigned, and the blob rows are left in place as a fallback. Cherry-picked from PR #1662 onto main; its v14 -> v15 migration is folded into this branch's v12 -> v13 rather than burning a second version number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The columns landed with nowhere to be set from and nothing to read them, which left the tables moved but not actually in use. Both halves are lifted from the Cat 3 branch, and both are inert until something declares a circle that enrols. - TableCircle.GetByGrantOnAsync is a WHERE GrantOn = ? against Idx1Circle. That query is the entire reason GrantOn is a column, and until the definitions moved into the table it could not have been written. TableCircleCached caches it under a ByGrantOn tag and invalidates that tag alongside the per-circle and all keys. No caller yet -- the auto-connect pipeline is Cat 3. - CreateCircleRequest gains AppId, GrantOn, Designation and Emoji, and the create path writes them. Until now nothing could set AppId at all: create ignored it and update refuses it by design, so an app-owned circle was unrepresentable. Omitting all four yields null/None/Personal/null, which is what every existing circle already is. - AssertDepositOnlyIfAmbientAsync enforces the invariant the moment GrantOn becomes settable: a circle that enrols without the owner present may hand out write/react and read on already-anonymous drives, and nothing else. Checked at definition-write time rather than grant-mint time, because an app can plant a definition and the next owner-driven grant would mint it with the master key in scope. Run on create and on every update, since an update is how a circle becomes ambient. Error codes 3013 and 3014 are new. 3010 is untouched here -- retiring it belongs to the review work. Tests pin the guard before there is a caller that can trip it: an ambient circle carrying a permission key is refused, and a manual-membership circle carrying the same key is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 26, 2026
CI builds with --warnaserror; my local runs did not, so five warnings I read past were six hard errors there. None of them were noise. - TableCircle captured the primary-constructor parameter it also passes to the base (CS9107). TableCircleMember already keeps an explicit field for exactly this; TableCircle does the same now. - RevokeApp/RemoveAppRevocation passed a possibly-null registration to SaveAsync (CS8604). Under the old blob store that wrote a row whose payload was the literal "null"; ToRecord would throw instead. Neither is wanted, so a missing app now returns early, which is what the null check was always shaped like. - FromRecord dereferenced a deserialize that can return null (CS8602). An unreadable grantJson is a corrupt row, so it throws with the app id rather than a bare NullReferenceException. - Two tests dereferenced a nullable deserialize. Verified with the same command the sqlite/debug job runs: 0 warnings, 0 errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerated CRUD and migration from the generator.
Circle declared circleId BYTEA NOT NULL UNIQUE alongside the correct
UNIQUE(identityId, circleId). The column-level constraint is global, and system
circle ids are fixed constants shared by every identity, so on Postgres -- where
all tenants share one database -- the second identity to run
config/system/initialize collided with the first:
23505: duplicate key value violates unique constraint
circlemigrationsv202608040942_circleid_key
The constraint dates to Postgres support (#854) and has been harmless until now
only because TableCircle had no caller: the table was empty everywhere. This
branch is the first code to write to it. SQLite never sees it because each tenant
gets its own file, which is why sqlite/debug and sqlite/release both passed while
postgres/release failed 337 tests.
Drives is the model: DriveId carries no column-level UNIQUE, only
UNIQUE(identityId, DriveId).
v202608261644 rebuilds the table without it, keeping the composite unique and
both indexes. Existing deployments already carry the constraint, so the DDL edit
alone would not have reached them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerated from the generator, with UpAsync/DownAsync hand-edited.
AppRegistrations shipped with the drive-addressing DDL carrying a version-0
migration only. The identity database keeps one migration version, and going up
the migrator runs only the groups above it -- so version 0 was unreachable on
every database already past it, and the table was simply never created there.
Fresh databases start at -1, run the version-0 group, and get it; which is every
CI database, so all three legs stayed green while a real deployment threw
42P01: relation "appregistrations" does not exist
as soon as AppRegistrationService queried it. main got away with it because
nothing read the table; this branch is its first reader.
v202608271000 restamps it above every released version, so the migrator reaches
it. Two populations have to arrive there, hence the branch in UpAsync:
- Table absent: create at this version and rename into place. No CopyDataAsync
and no rename of a table that is not there.
- Table present at version 0: the generated rebuild. Restamping requires it --
on SQLite the version marker lives inside the stored CREATE TABLE text.
DownAsync mirrors it: with no AppRegistrationsMigrationsV0 to restore, Up must
have created the table outright, so undoing means dropping it.
Uses cn.TableExistsAsync rather than GetTableVersionAsync, which on Postgres goes
through obj_description('AppRegistrations'::regclass) and throws 42P01 on a
missing relation before it can report anything.
The generator needs the same branch for any table introduced after the first
release, or the next regen reverts this. Noted in the file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main's #1686 moved the flag off a private _isRunning field onto the injected VersionUpgradeRunState. The v12 -> v13 step this branch adds was still assigning the field, which merges cleanly and then does not compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The v12 -> v13 app move read nothing, silently. It deserialized blob rows into AppRegistration, which [JsonIgnore]s AppId, AppSlug, Name and CorsHostName -- correct for writing, since those are columns now and a second copy in grantJson could drift from them, and fatal for reading the blob, where that JSON is the only place the values exist. Every legacy row came back with a null AppId, the Where filtered the lot, and the migration logged "no app registrations in blob storage; nothing to move" and committed. The tenant reached v13 with an empty AppRegistrations table while the blob still held every app -- and AppRegistrationService reads the table only, so the identity presents as having no apps at all. Seen on the demo box. The migration now holds LegacyAppRegistration: the blob shape frozen as it was before the columns were promoted. A migration reads history, so it owns a copy of the shape history was written in rather than borrowing a type that has moved on -- the same reasoning as the frozen context and category keys above it. Note the asymmetry that caused this: CircleDefinition solves the same blob-versus-column problem by clearing the fields inside ToRecord and keeping them serializable, so the circle half of this migration was never affected. Tests seed a legacy row in the old JSON shape and assert the app lands with its real name, its CorsHostName and a slug derived from that name -- a slug of the app id would mean Name came back null. Plus idempotency: a second pass must not mint a new slug, since it is an address other identities may already hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Overwrite_Encrypted_PayloadManyTimes_Concurrently_MultipleThreads runs 20 threads against one target drive, and each thread called CreateDrive for that same drive from inside PrepareEncryptedFile. Only one create can win; a loser's first upload could land before the winner's drive was visible, so the setup assertion on IsSuccessStatusCode failed. Seen on the ubuntu/postgres job of run 33076953471 (3 of 20 threads), where sqlite and windows passed on the same commit. Create the drive once in the test method, before the threads start. The concurrency under test - 20 threads overwriting their own file 50 times - is unchanged. Also make the two counters Interlocked: all 20 threads increment them and the final assertion reads them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AppId, DriveSlug and DriveTypeSlug shipped with the drive-addressing DDL and were invisible above the SQL layer: DriveManager.ToRecord never wrote them, ToStorageDriveData never read them, and StorageDrive had nowhere to put them. The columns could not be populated even by hand. - StorageDriveData and StorageDrive gain the three. They stay out of StorageDriveDetails on purpose -- UNIQUE(identityId, AppId, DriveSlug) constrains the columns, and a copy inside detailsJson could disagree with what the constraint is enforcing. Same discipline the circle work used. - ToRecord writes them, ToStorageDriveData reads them, and the create path persists what the request carried. - CreateDriveRequest accepts them, all optional. Omitting them leaves a drive addressed by Guid exactly as before, which is every drive today. - OwnerClientDriveData carries them, so a client can read what a drive holds. - OdinSlug validates the format from docs/drive-addressing.md: lowercase, digits, internal hyphens, 1-12 characters. Validate and reject, never coerce -- the value ends up in other identities' URLs, so lowercasing 'Chat' would hand back an address the caller did not ask for. The reserved-segment list is empty and deliberately present: /apps roots the slug tree so neither position has a literal sibling today, and it must grow when one appears. WriteOnlyKeyPair is deliberately not plumbed. It is key material for write-only deposits with escrow, rotation and deleted-drive questions still open in the doc, and it must never reach a client shape. Nothing derives a slug or assigns ownership. Every drive still carries null for all three until the mapping is settled; this only makes the columns reachable. Tests: the columns round-trip through TableDrives and null stays null; the slug rule accepts what the doc allows and rejects encoding, path-separator, case and length violations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Derived working checklist across the two table moves, the chat circle ownership change and the drive addressing columns: what is done, what is blocked on a decision only Todd can make, what backfill and enforcement is owed, and the deploy-safety items the demo box taught us. Follows docs/connection-defaults-checklist.md: a derived list, not a spec. The two design docs remain the source of truth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OwnerClientDriveData got AppId, DriveSlug and DriveTypeSlug; ClientDriveData did not, so a client reading drives over the app or V2 route could not see them. - V2DriveMetadataController maps all three. - ClientTokenDriveMetadataController redacts them for third parties exactly as it already redacts Name and Attributes. The slug is designed to be a remote-resolvable address, but resolution happens on the recipient side (drive-addressing.md, "Slugs are resolved by the recipient"), so a guest does not need the list to use one. One-line change if we decide otherwise. The peer route is deliberately untouched: PeerQueryControllerBase maps PerimeterDriveData, the cross-identity wire shape, which carries only TargetDrive and Attributes. Publishing slugs to another identity is a separate decision about what an identity discloses, not a mapping change. Null on every drive today, so nothing changes for any caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drive-addressing.md has the app choosing its slug: it is a package name, not a role -- "a second chat implementation does not get to call itself chat; it picks its own slug (chatty)" -- and "registration is first-come", which only makes sense if the app is asking and can be refused. The field never existed; the server coined one from the display name because pre-existing registrations had none and the column is NOT NULL. That was the migration's stopgap, not the design. AppRegistrationRequest.AppSlug is optional and unenforced: - Omitted: derived from Name exactly as before, so nothing that registers today starts failing. - Supplied: validated for format and taken verbatim, or refused. Never quietly replaced with a derived one -- it is an address other identities resolve against, and handing back a different one is worse than saying no. - Already held by another app: refused with a clear client error rather than a UNIQUE(identityId, AppSlug) constraint violation from the database. Immutability is unchanged: updates carry the stored slug forward, and no update request carries a slug field at all. Tests cover all four paths. Fixed my own invented expectation while writing them: "Acme Receipts" derives to "acme-receipt", not "acme-receipts" -- the generator truncates at the 12-character cap. The migration test written earlier had the same wrong value and had never run, since port 4444 was busy; it would have failed in CI. Not done here: protecting the system slugs (chat, mail, feed, photo, owner) from a caller claiming them. AppSlugGenerator orders known apps first when deriving, but nothing stops a supplied slug taking one on an identity where that app is not yet registered -- checklist 3.9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3.1 is done: AppRegistrationRequest carries an optional AppSlug, validated and taken verbatim or refused. Records what shipped (0.10, 0.11), and the two things that decision leaves behind: - 3.3a: slug derivation now lives in both the registration service and the migration, deliberately, rather than making the column nullable. - 3.9 changes character. It used to be theoretical; a caller can now supply 'chat' on an identity where the chat app is not yet registered and take it first-come. Same question as drive-addressing.md OQ2. Plus 3.3b, the 12-character truncation, and 7.5 for clients that want to name themselves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The drive addressing columns have been in place but nothing ever put a value in them, so every drive carried a null AppId, DriveSlug and DriveTypeSlug. This fills them in from a fixed mapping. Move the thirteen app-owned drives out of SystemDriveConstants into WellKnownAppDrives, leaving only TransientTempDrive behind. Add CommunityDrive there too. The rename is mechanical -- same namespace, so it is an identifier swap the compiler checks -- and touches 451 call sites. Coin app ids for the apps that own a drive but have no registration yet: Community, Contacts, Email, HomePage, Lists, Location, Moments, Recovery, Vault, and a System app for the transient drive. An app id is permanent, since it is what Drives.AppId points at and what a drive slug is unique within. DriveSlugGenerator mirrors AppSlugGenerator: fixed slugs for the known drives, derived from the drive name otherwise, whole set resolved up front. It is wired into drive creation as a fallback -- a caller-supplied slug is taken verbatim or refused, never quietly replaced. Two details worth calling out: Slugs are deduped per owning app, not per identity. The constraint is UNIQUE(identityId, AppId, DriveSlug), so feed/news and chat/news may coexist; deduping identity-wide would hand the second one "news-2", a permanent address nobody asked for, for a collision the schema allows. Nothing is derived for a drive with no owning app. AppId and DriveSlug are set together or both NULL: NULLs are distinct in a unique index in both dialects, so a slug on an AppId-less row is unconstrained and two drives could claim it. Raise OdinSlug.MaxLength from 12 to 14 so "shard-recovery" fits. The database caps these at 64, so there is room. Existing tests that asserted 12-character truncation are updated to the new cap. Whitespace-only is now treated as "not set" for app and drive slugs alike. Clients serialize an unset field as "" or " " routinely, and the three spellings had diverged: null and "" derived a slug while " " failed validation and threw. A value with real content is still validated and rejected, so " chat " is an error rather than being trimmed to "chat". Seeding is deliberately unchanged: EnsureSystemDrivesExist still creates all fourteen drives, now via WellKnownAppDrives. Which of them a new identity should get is a separate decision. Known gaps, to be addressed next: - Profile, Wallet and HomePageConfig share one drive type but are given the type slugs profile, wallet and profile; Moments and Lists share a type across two different apps. Both break the one-slug-per-type and one-app-per-type rules in docs/drive-addressing.md. Type slugs are keyed by drive alias for now so the mapping is stored as given rather than resolved by guesswork. - Photo Library and Vault are named by the mapping but have no Guids yet. - The WellKnownAppDrives header still says its drives are absent from SystemDrives and never server-created. Both are untrue until seeding is settled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every drive already had an owning app; this gives every app its circles, and decides which apps a new identity starts with. Nine apps are built-in and get registered: Chat, Contacts, Email, Feed, HomePage, Location, Mail, Recovery and System. Contacts, Email, HomePage, Location, Recovery and System had no registration before -- each is granted ReadWrite on the drives it owns, with no permission keys and no authorized circles, since none were specified. The three near-identical Register*App helpers collapse into one RegisterAppIfNotExistsAsync. WellKnownAppCircles holds the sixteen app-owned circles. Ten belong to built-in apps and are provisioned by EnsureBuiltInAppCirclesExistAsync; the other six arrive only with their app. Friends, Family, Work and Acquaintances are in code for the first time -- until now the owner console's setup wizard created them client-side. The two system circles are deliberately untouched: they are retired in a later step, and until then they still have to work. Chat is the only GrantOn=Connect circle, so it is granted ambiently to auto-connections with no owner review. It carries write/react only, which is what the deposit-only invariant requires. Seeded drives and SystemDrives are now the same set, which is the point of this change and not a coincidence: SystemDrives is what makes a drive immutable (DriveManager refuses to rename, re-mode or archive anything in it), so a seeded drive missing from it is one the owner can archive out from under the system. WalletDrive leaves both -- Vault is not built-in -- and EmailAppDrive joins both, because Email is built-in and its registration is granted the drive, and a grant cannot be issued for a drive that does not exist. ListsDrive and MomentsDrive are seeded even though Lists and Moments are not built-in. The system circles grant them, and issuing those grants throws if the drive is absent. Both go when those circles do. Move ChannelDriveType into WellKnownAppDrives to break a static-initializer cycle. SystemDrives lists drives declared in WellKnownAppDrives, and PublicPostsChannelDrive read SystemDriveConstants.ChannelDriveType, so touching WellKnownAppDrives first ran SystemDriveConstants mid-initialization and built SystemDrives out of fields that were still null. It resolved by declaration order until adding EmailAppDrive to the list changed which type was touched first; the failure is a NullReferenceException far from the cause, never an error at the source. WellKnownAppDrives now reads nothing from SystemDriveConstants, so the dependency runs one way. WellKnownAppDrivesTests guarded that EmailAppDrive was never auto-created. That stopped being true, and the test kept passing because it only checked list membership. It now guards what replaced it: anything seeded must be immutable. Not done: the conversion for the six apps that are not built-in, which has to stamp ownership and slugs onto what existing identities already hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six lists currently state which app owns which drives and circles, all keyed by app id and never joined: EnsureSystemDrivesExist, EnsureBuiltInApps, SystemDrives, BuiltInAppIds, the slug tables in DriveSlugGenerator, and the circle constants. Answering "what is Chat?" means grepping four files, and nothing stops two of those lists disagreeing -- which is how the seeded drives and SystemDrives drifted apart earlier on this branch. Odin.Services.Apps.Builtin declares it once instead: SystemApp.cs the SystemApp and AppDriveGrant records BuiltinDrives.cs 18 drives -- identity, address, settings BuiltinCircles.cs 17 circles -- id, grants, GrantOn BuiltinAppDriveGrants.cs 25 cross-app grants, flat BuiltinApps.cs 15 apps, and the projections over them Ownership is a tree, so drives and circles nest under the app that owns them. Grants are not: eleven of the eighteen supplied rows cross app boundaries -- Chat holds ReadWrite on ContactDrive, which Contacts owns -- so nesting them would mean one app's node referencing another's, which is the static initializer cycle that already bit us once. They sit in a flat sibling list that references the drive constants directly. Circle drive-grants are the opposite and do nest: every circle grants only drives its own app owns, with no exceptions. The two system circles are the one thing that does not fit -- owned by no app, granting across six drives -- so they stay in SystemCircleConstants until they retire. Nothing reads any of this yet. The values are copied, not moved, and SystemDriveConstants, WellKnownAppCircles and BuiltInCircleConstants are still what runs. Both copies were diffed field by field: drive settings match on name, anonymous reads, owner-only, subscriptions, CDN, target drive and app id; circles match on id, owning app, GrantOn and drive grants. Slugs are stated here rather than derived. Each drive carries its own, so the 36 entries in DriveSlugGenerator's lookup tables stop being a second place for them to live. Apps carry one too: only five were fixed before, so ten would have been derived from their display name, and the one for the app formerly called Owner still read "owner" after the rename. Name, AppSlug and Permissions have no reader yet. They exist to build an AppRegistrationRequest, which cannot be derived until AuthorizedCircles has somewhere to point -- today Chat and Mail aim theirs at the system circles. Verified against the four supplied mappings: 15 apps and their built-in flags, 18 drives with owning app and both slugs, 17 circles with owning app and GrantOn, and 18 drive grants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BuiltinProvisioner reads the tree and creates what an identity starts with, so the set provisioned is a projection rather than a list restated in TenantConfigService. EnsureInitialOwnerSetupAsync calls EnsureAllAsync; the version ladder keeps calling EnsureDrivesAsync on its own, since VersionUpgradeService does one up-front pass so migrations can assume every drive exists. TenantConfigService keeps EnsureSystemDrivesExist and EnsureBuiltInApps as forwarders so those callers are untouched. Verified the same 14 drives, 9 apps and 13 circles as before -- diffed against the previous commit, not assumed. Provisioning order is drives, then circles, then apps, which is a change. Circles used to come first, and that made a check misreport: the deposit-only guard runs whether or not validation is skipped, and for an ambient circle with a read grant it reads the drive to see if it allows anonymous reads. With no drive there yet the lookup returned nothing and the error blamed the read grant rather than the ordering. Nothing needed circles first -- HandleDriveAdded only touches the two system circles, and those are created by the caller before the provisioner runs. Within drives, non-anonymous first. Creating an anonymous-read drive makes HandleDriveAdded grant read on it to the system circles, so every drive those circles already grant has to exist by then. All six are non-anonymous, so ordering on that flag satisfies the constraint by construction. It used to be a comment asking the next person to keep ListsDrive above the anonymous ones. The tree now separates the two groups instead of flagging them: Builtin is the nine an identity is configured with, Wellknown the six that arrive only when the owner installs them. The BuiltIn property is gone -- list membership is the fact, and holding both invites an app in one list claiming the other. SystemAppConstants.BuiltInAppIds and IsBuiltInApp are deleted for the same reason; their only caller moved into the provisioner. Three of the six own a drive every identity already has, seeded long before ownership existed: ListsDrive, MomentsDrive and WalletDrive. Those need stamping by the conversion, which is still to write. The other three own nothing that exists. Also record why drive creation must not check that the owning app exists. It does not today, but only by omission, and the dependency runs the other way: a registration is granted drives, and a grant cannot be issued for a drive that is absent. Validating the app at drive creation would make the two constraints unsatisfiable for every built-in app. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The addressing columns landed several commits ago and nothing ever filled them, so on a tenant that predates this every drive carries AppId, DriveSlug and DriveTypeSlug as NULL, and the circles the owner console's setup wizard created carry no AppId. v13 -> v14 fills them in from BuiltinApps. Two halves, and they are different jobs. Stamping fills rows that already exist. Provisioning then creates what is missing, and is the same BuiltinProvisioner a new identity gets -- so an upgraded identity converges on what a fresh one has, rather than the two paths drifting. Stamping runs first, so provisioning sees a drive that is already owned rather than trying to create one that is there. It walks the whole tree, not just the built-in apps. ListsDrive, MomentsDrive and WalletDrive belong to apps that are not built-in, yet sit on every identity because they were seeded long before ownership existed. Those three are the reason the Wellknown list is not simply inert. Additive throughout: anything that already has an owner is skipped rather than reassigned, so a partial run repeats safely and a value set by hand is never overwritten. Nothing is deleted -- WalletDrive stops being seeded for new identities, but the ones that have it keep it, stamped like the rest. This needs two setters that deliberately did not exist. Ownership is not reassignable through the normal write paths -- CircleDefinitionService.UpdateAsync refuses to take AppId from a request precisely so nobody who can PUT a definition can hand a circle to an app, and nothing updates a drive's slug because it is a wire address other identities resolve against. Both new methods are internal, refuse an item that already has an owner, and exist only to give a row the values it would have been created with today. The drive one also skips the system-drive guard every other setter has: all fourteen are system drives, so guarding would make it useless for its one job. Version.DataVersionNumber goes to 14. That constant is the gate -- RequiresUpgradeAsync compares against it -- so without the bump the rung would exist and never run. Supersedes #1691, now closed. That branch numbered a different v13 -> v14 which gave the relationship circles to chat; the mapping puts them under Contacts, so it was contradicted rather than merely renumbered. It was never deployed, so no identity is recorded at a v14 that meant something else. Untested. Nothing here has been exercised: the migration path is hosting integration territory, and the checklist already notes that every migration test starts from an empty database, which is the case that works. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 15 CreateDriveRequest constants were duplicated by BuiltinDrives and read by nothing but one test file, so they are deleted and AllowCdnTests points at the tree instead. That last part matters more than the line count: those tests pin CDN settings, and aimed at the copy the provisioner no longer uses they would have kept passing while the real values drifted. SystemDrives becomes BuiltinDrives.Protected. It was never a list of system drives -- ListsDrive and MomentsDrive are in it and belong to apps that are not even built-in, provisioned only because the system circles grant them and a grant for an absent drive throws. What the list actually decides is whether the owner may rename, re-mode or archive a drive, which is its only use: three guards in DriveManager and the flag the owner console renders. It is named for that now, and says out loud that protected means "we provisioned it" rather than "it is systemic". Verified the new list identical to the old, same entries in the same order, before repointing any of the twelve usages. SystemDriveConstants drops from 218 lines to 33: the transient drive's identity, which still has around ninety references, and a forwarder for the channel type. Moving those two would retire the file, but it is a couple of hundred call sites and better done deliberately. WellKnownAppCircles is gone, deleted separately; BuiltinCircles is now the only declaration of the app-owned circles, and its comment no longer claims otherwise. Protected is still hand-listed rather than derived from what is actually provisioned. The two drifted once already -- WalletDrive left the seeded set and EmailAppDrive joined it, and neither was reflected -- so deriving it is the real fix. It needs the provisioned set named first, which is currently computed inline in EnsureDrivesAsync, and the static initializer cycle between these types has bitten once, so that is worth doing on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tree is the source of truth for the drives, circles and apps it declares, so the upgrade now makes them match rather than filling in whatever is empty. Each stamp used to skip anything that already had a value, which meant a row written by an earlier build kept that value forever. That is not hypothetical. An earlier branch briefly gave the relationship circles to chat before the mapping put them under Contacts, and any identity that ran it holds Chat-owned Friends, Family, Work and Acquaintances that a fill-only stamp would never correct. Same shape for app slugs: registrations built before the tree was authoritative derived the slug from the display name, so "Homebase - Location" was registered as homebase-locat -- and a slug is immutable through every normal path, since other identities resolve against it, so nothing else would ever fix it. Three methods, all internal and migration-only, all returning whether they changed anything so the log records corrections rather than visits: ApplyTreeAddressAsync drive AppId, DriveSlug, DriveTypeSlug ApplyTreeDefinitionAsync circle AppId, GrantOn, Designation ApplyTreeSlugAsync app AppSlug -- new; there was no way in at all Each one is an exception to a rule that exists for a reason. UpdateAsync refuses to take AppId from a request so that nobody who can PUT a definition can hand a circle to an app; an update carries the stored app slug forward because it is a wire address. The remarks on each say why the exception is warranted, so the rule is not quietly weakened. Validation is stronger to match: it used to check a drive or circle had an owner, and now checks the value equals what the tree says. A correction that silently fails now fails the upgrade instead of recording success. The app slug path checks uniqueness before writing, since UNIQUE(identityId, AppSlug) would otherwise surface as a constraint violation. It throws naming the conflicting app. One limitation: two apps that need to swap slugs cannot, because whichever is corrected second still finds the first holding its target. No mapping we have does that, and failing loudly beats half-applying a rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Step 1 of the drive-addressing groundwork, replayed onto
mainso the two tables the schema workshipped can actually be put to use. This is the code from #1661 and #1662 and nothing else -- no
review stamp, no ladder recut, no enrollment model.
Both tables landed dormant with the drive-addressing DDL (#1589) and have never had a caller.
Definitions and registrations still live in the shared key-three-value blob, where
Circle.AppId,Circle.GrantOnandUNIQUE(identityId, AppSlug)cannot be queried or constrained at all.Circle definitions -> the
CircletableCircleDefinitiongainsAppId,GrantOn,DesignationandEmoji. It is both the stored shapeand the wire shape --
CircleDefinitionControllerBaseserves it directly and takes one as anupdate body -- so the fields stay on the wire and the blob copy is cleared inside
ToRecord, thesame clear-before-serialize trick
ToConnectionsRecorduses for the grant collections. Nothing inthe blob can drift from the column, because deserializing the blob alone yields defaults.
CircleGrantOnandCircleDesignationare new enums matching the column values. Every existingcircle is
None/Personal.AppIdis not taken from an update request: ownership is set at creation and must not bereassignable by anyone who can PUT a definition.
TableCirclegrowsUpsertAsync/GetAllAsync;TableCircleCachedinvalidates the all-keyalongside the per-circle key on insert, upsert and delete.
App registrations -> the
AppRegistrationstableAppId,AppSlug,NameandCorsHostNamebecome columns and are[JsonIgnore]d out ofgrantJson. Everything else still rides the JSON.AppSlugGeneratorcoins the slugs no registration has: system apps get their fixed name(owner, chat, feed, photo, mail), everything else derives from its display name, collisions take a
numeric suffix, and a name that slugifies to nothing falls back to the app id. The whole set is
resolved and checked before anything is written.
holding
acme-2keeps it whatever its name would slugify to today.Putting the columns to use
Lifted from the Cat 3 branch, because the two moves above otherwise leave the tables migrated but
still unused. Both are inert until something declares a circle that enrols:
TableCircle.GetByGrantOnAsync--WHERE GrantOn = ?againstIdx1Circle. This query is theentire reason
GrantOnis a column, and it could not have been written while definitions wereblob rows. Cached under a
ByGrantOntag, invalidated alongside the per-circle and all keys. Nocaller yet; the auto-connect pipeline is Cat 3.
CreateCircleRequestgainsAppId,GrantOn,DesignationandEmoji, and the create pathwrites them. Until now nothing could set
AppIdat all -- create ignored it and update refuses itby design -- so an app-owned circle was unrepresentable. Omitting all four yields
null/
None/Personal/null, which is what every existing circle already is.AssertDepositOnlyIfAmbientAsyncenforces the invariant the momentGrantOnbecomes settable: acircle that enrols without the owner present may hand out write/react and read on already-anonymous
drives, and nothing else. Checked at definition-write time rather than grant-mint time, because an
app can plant a definition and the next owner-driven grant would mint it with the master key in
scope. Runs on create and on every update, since an update is how a circle becomes ambient. New
error codes 3013 and 3014; 3010 is untouched, retiring it belongs to the review work.
Migration
One step,
v12 -> v13, moving both. Idempotent and additive: a row already in the destination tableis left alone, so a partial run repeats safely and an app's slug is never reassigned. The blob rows
are deliberately left in place as a fallback -- cleaning them up is a separate job.
No behaviour change
Every promoted field takes the default that every existing circle and app already had. Nothing reads
GrantOn,Designation,EmojiorAppIdyet, and no existing client sends the four newCreateCircleRequestfields. The honest caveat: those four are a wire addition, and an owner-consolecaller can now set
AppIdat creation time.Differences from #1661 / #1662
v12 -> v13there; onmainthe next free number is 12.either on the tables or on the blob; the state in between is not worth being able to stop in.
[JsonIgnore]on the promoted fields and thesecond undid it (it silently reset
GrantOnon client PUTs); replaying that intermediate statehad no value. Final code is identical to the branch tip.
docs/connection-defaults-checklist.mdis not included -- it belongs to the review-stamp PR.Testing
Full solution suite green:
dotnet test ./odin-core.sln-- 2,740 passed, 68 skipped, 0 failedacross all 11 projects.
Odin.Hosting.Tests-- 776 passed, 60 skipped. This is the one that matters here: it exercises thecircle-definition and app-registration paths through a real server, so the table moves and the
v12 -> v13 migration are covered end to end, not just at the unit level.
Odin.Hosting.Tests.V2-- 482 passed, 3 skipped.Odin.Services.Tests-- 525 passed, includingCircleDefinitionStorageTests(blob/column splitplus the deposit-only guard) and
AppSlugGeneratorTests.Odin.Core.Storage.Tests-- 512 passed, 3 skipped.Not verified here: the migration has only been exercised on the CI/dev dialect this suite runs
against. Worth a look at the Postgres leg before merge.
🤖 Generated with Claude Code