Step 2 drive write only keypair - #1703
Merged
Merged
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>
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>
A user makes channel drives at will, so there are arbitrarily many and none has a fixed alias -- they are the one kind of drive the tree cannot list, and the reason DriveSlugGenerator keeps its derivation at all. The upgrade now hands them to the feed app with a slug derived from the drive name and a type slug of "channel". Fills rather than corrects, unlike the drives the tree declares. The tree is authoritative for what it declares and it does not declare these: their slug comes from a name the owner chose and may since have changed, so re-deriving on every upgrade would move an address other identities resolve against. A channel drive that already has one is left alone. Ordering is load-bearing. It runs after the tree drives are stamped, so FeedDrive and PublicPostsChannelDrive already hold "feed" and "posts" in storage and the taken set read back is complete -- otherwise a channel named Posts could take "posts" first. Uniqueness is scoped to the feed app, matching UNIQUE(identityId, AppId, DriveSlug), and the taken set accumulates inside the loop so two channels named News become news and news-2. The type slug is stated rather than looked up. These drives came from a query on ChannelDriveType so the type is already known, and TypeSlugFor can return null -- which AssertValidOrNull permits, so a miss would have been stored silently. Validation now checks the type slug too; it previously asserted only the owning app and slug, so a null would have survived the upgrade unnoticed. That matters because DriveTypeSlug is what ?type=channel will filter on once those routes exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KnownAppSlugs listed five apps by hand while the tree named far more, so any app it missed had its slug coined from the display name instead: "Homebase - Recovery" became homebase-recov where the tree said recovery. The v13->v14 stamp then had to correct an address other identities may already hold. Reading BuiltinApps.All removes the second list rather than lengthening it. The tree is validated as it is indexed -- a duplicate app id or a malformed slug is a mistake worth naming before it reaches a registration. This settles one disagreement in the tree's favour: the System app derives as 'system' now, not 'owner', which is what the v14 stamp was already forcing. EveryAppInTheTreeGetsTheSlugTheTreeNames is the test that would have caught this, and cannot drift as the tree grows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The circle was declared twice. EnsureBuiltInCirclesExistAsync created it from BuiltInCircleConstants' own copy, whose CreateCircleRequest left out AppId, GrantOn and Designation -- and because that runs before provisioning in EnsureInitialOwnerSetupAsync, EnsureCircleExistsAsync then found the circle present and skipped it. Every identity ended up with the unowned row, new ones included, so the owner console showed the circle as belonging to no app while the tree named Location as its owner. UpdateAsync will not set AppId by design, so the reconcile branch could not repair it either: it rewrote everything except the field that was wrong. The definition now lives only in the tree and the row is only ever written by EnsureCircleExistsAsync, which carries all three fields. v9 is where this circle arrives, so that migration asks for it by name instead of relying on a side effect of reconciling the system circles. Existing identities still need the v13->v14 stamp, which corrects rather than fills and already covers this circle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Webdrop joins the tree as a built-in: its app id, drive, drive slug, circle and registration request, and provisioning registers it like the rest. The v13->v14 upgrade also gains a final pass over drives nothing declares -- created through the owner console or the setup wizard's own request.Drives, so neither the tree nor the channel query reaches them. They would otherwise come out of the upgrade with no address. Slug only: the owning app stays as it was, which means the unique index cannot police these rows, so the pass dedupes them itself and validation fails the upgrade if any drive is left without a slug or two unowned drives share one. ApplyTreeAddressAsync becomes ApplyAddressAsync, with a nullable app id and type slug, since it now serves drives that have neither. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It was in the built-in list, so provisioning created its drive, its circle and its registration on every identity that upgraded -- and the drive allows anonymous reads, which makes HandleDriveAdded grant read on it to both system circles. A new public drive on every identity is a product decision, not a side effect of a tree entry. Wellknown is the list for this, and Lists, Moments and Vault are the precedent: nothing is created for anyone, while the v13->v14 stamp still gives the drive an owner and an address on identities that already have one. The drive is made by chat-kmp, so those identities exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The services read the Circle and AppRegistrations tables and nothing else, but the rows only arrive in v12 -> v13, and that upgrade runs when the owner logs in. Between a deploy and that login an identity has no apps and no circles: app tokens resolve to a null registration, and IsEnabledAsync reports every circle disabled, because a null definition is not a disabled one. A dormant identity could sit there indefinitely. So the four read paths fall back to the blob. LegacyDefinitionStore holds the legacy keys and the frozen registration shape, which the migration now shares rather than keeping its own copy. The gate is the tenant's data version, not an empty table. The move deliberately leaves the blob rows in place, so after it a missing row means the owner deleted something -- falling back on a miss would resurrect it. The list reads are a union, table first, since anything written during the window is already there and is the newer of the two. AssignSlugAsync now reserves the legacy slugs too: a slug free in the table could still be one the move is about to coin, and the move would then fail on UNIQUE(identityId, AppSlug). Delete all of it once every environment reports v13 or later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… they were Rewriting both BuiltinApps lists by hand in "Test the slug-addressed routes" changed which apps an identity is provisioned with, in ways that commit did not intend and its V2-only test run could not see. Mail was dropped from both lists outright. EnsureAppsAsync walks Builtin, so the Mail app stopped being registered at setup -- "there should be 3 app grants; mail, chat, and the app created in this test" is that, stated by the test. With MailAppId off the tree the slug generator no longer recognised it either and slugged it off its display name as homebase-mail, which is exactly the bug the generator reads the tree to avoid. Webdrop, Moments and Vault went the other way, from Wellknown into Builtin. That seeds WebDropDrive, WalletDrive and VaultDrive for every identity, none of which are in BuiltinDrives.Protected -- 17 drives provisioned against a Protected of 14. Protected's own remark says to keep the two in step, and the initialize test says WalletDrive is deliberately absent because it is no longer seeded. Webdrop's promotion also silently reversed "Webdrop is known, not shipped with every identity". Membership returns to what it was before that commit. Lists stays commented out of Wellknown, which that commit did deliberately and explained. MailDrive leaves SystemCircleCarryOverDrives with it. It only joined to stop identity setup throwing invalidGrantNonExistingDrive once its app was gone; with Mail owning it in Builtin again it is a seeded drive, and the carry-over list is back to the two whose apps really are not built-in. Provisioned drives are again the 14 Protected names. The seven tests that failed on this branch since that commit now pass: AppSlugGeneratorTests.KnownSystemAppsGetTheirName, the two SystemInitializeConfigTests drive-count tests, and the five CircleNetworkServiceAppTests app-grant tests. Odin.Services.Tests: 573 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…drive-write-only-keypair
…ck where they were" This reverts commit 0c4815d.
…drive-write-only-keypair
…on-tables' into step-1-circle-and-app-registration-tables
…drive-write-only-keypair
…ault built-in
The tree changed in "Test the slug-addressed routes" and the tests were never
brought with it, which is what has been red since. Mail's app is gone for good,
Moments and Vault ship with every identity; these tests still described the world
before that.
Mail is no longer a known system app, so the slug generator has nothing to say
about it -- KnownSystemAppsGetTheirName stops asserting a slug for an app the
tree does not name.
Only Chat and Mail authorised the system circles, so with Mail unregistered a
new connection carries one built-in app grant instead of two. The five
CircleNetworkServiceAppTests counts go 3 -> 2.
Protected regains the drives provisioning actually creates, WebDropDrive and
VaultDrive, and is again the same set: 16 either way. Its remark asks for the
two to be kept in step and they had drifted.
Two things this turned up that were not just test text.
VaultAppRegistrationRequest granted ReadWrite on WalletDrive. Vault is built-in,
so it is registered at setup, and a grant for a drive that does not exist throws
-- with WalletDrive no longer seeded, identity setup failed outright for every
new identity, not only in these tests. The grant goes. WalletDrive is now owned
by nothing, so an identity that already has one keeps it with a derived slug and
no owning app, reachable by guid rather than by an /apps/{appSlug}/drives path.
Both v13->v14 remarks that said otherwise are corrected.
WebDropDrive is anonymous-read and Webdrop is built-in, so HandleDriveAdded
grants the system circles read on it as it is created. The connected circle
holds 10 drive grants now, not 9, and the test says why.
Odin.Services.Tests: 567 passed, 0 failed. The nine tests in the two affected
Hosting.Tests classes all pass, including the two that only failed once
WalletDrive stopped being seeded.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…drive-write-only-keypair
…onsole
The keypair has been minted and stored since "Mint a write-only keypair for every
drive", but nothing could read it: no endpoint served the public half and no API
returned it, so the thing that makes write-without-read possible was invisible.
GET /api/v2/peer/{odinId}/apps/{appSlug}/drives/{driveSlug}/public-key, in the
three parts the slug routes already use -- outgoing controller, refit client,
perimeter. The perimeter resolves the address itself rather than calling
ResolveDriveAddressAsync, which accepts any grant: naming a drive discloses
nothing, but handing out the means to write to it does.
Write on the drive is required, and only Write. A reader has no use for this key,
and a caller without Write gets a security exception rather than a 404 -- unlike
resolution, which conflates the two so nobody can enumerate drives by name. Here
the caller has already resolved the address, so the drive's existence is not a
secret from it, and a 404 would send it hunting for a drive that is there.
The public half is not cached on the outgoing side, though the address is. A slug
is immutable; a keypair is not. A cached public half would keep this identity
sealing to a key the remote had rotated away from -- deposits nobody can open,
with nothing anywhere to say so.
EnsureWriteOnlyKeyPairAsync now takes the storage key from the caller's grant
rather than by decrypting MasterKeyEncryptedStorageKey with the master key. Same
key either way, but the grant is what the escrow actually means: the private half
is sealed under the key that grants access to this drive.
OwnerClientDriveData carries the public half and its crc32 so the owner console
can show which key a drive holds. Public half only -- the private half stays
escrowed and never leaves the server, and the peer endpoint hands this same value
to any caller with write access.
Tests. PeerAppDrivePublicKeyTests: a writer gets the key, a reader is refused with
403, unknown app and drive slugs are not found. The one that matters seals to the
key the endpoint served and has the drive's owner open it -- "a key came back"
would pass against any key at all.
DriveWriteOnlyKeyPairTests gains the escrow assertions it was missing.
ThePrivateHalfIsEscrowedUnderThatDrivesStorageKey was named for a claim it never
checked: it compared two public halves and never opened anything. It now fetches
the drive's real storage key and opens the private half with it, alongside the
negatives (another drive's storage key, the master key) and a seal/unseal round
trip that survives every write path through ToRecord.
docs/drive-addressing.md: "lazily on first request" is struck through rather than
deleted, with why. Lazy minting cannot work -- minting needs the drive's storage
key and the caller who triggers the first request never holds it. AllowDeposits is
struck with it: it was to say which drives get a keypair, and every drive gets one.
Odin.Services.Tests: 573 passed. Odin.Hosting.Tests.V2: 553 passed, 3 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An upgrade on the integration box stopped mid-run and reported nothing. It logged the master-key pre-pass, logged "Ensuring system drives exist", created a drive, called GrantAnonymousRead -- and then nothing, ever. No exception, so the outer catch never fired and no failed version was recorded; the scheduler kept saying "previously failed build version: none". The job stays queued under its unique hash, every later attempt gets "already exists", and the tenant sits below the release version answering 503 to every owner endpoint. The symptom reported was "no apps return", which is that 503 and nothing to do with apps. Every phase now says when it starts and when it finishes, with elapsed. A phase that never finishes is then visible as the last unmatched start, rather than as an absence of anything at all. A phase that stops making progress for twenty minutes is turned into a real failure. It is raced against a timer rather than driven by a cancellation token: EnsureSystemDrivesExist takes no token, nor do the notification handlers it fans out to, so cancelling one would be ignored and the hang would stay invisible -- which is exactly the bug. The phase cannot actually be stopped, so it is abandoned rather than cancelled and the log says so, and a fault arriving in it later is logged instead of disappearing as an unobserved task exception. The failure also unsticks the job: recording it lets the next attempt start rather than collide with the queued one. The two places the trail went cold now speak. Drives are named before creation, not only after -- creating one publishes DriveDefinitionAddedNotification and its handlers re-grant circles to every member, so the name has to be in the log before the slow part, or a stall names nothing. And UpdateCircleDefinitionAsync, where the last line came from, logs the member count up front and each member as it goes: stuck on one member and grinding through thousands look identical from outside, and each iteration ends in SaveIcrAsync, which publishes into an outbox path already failing on that host for unreachable peers. Odin.Services.Tests: 573 passed. One unrelated flake found on the way, ItShouldDeleteExpiredUnsuccessfulJobsInTheBackground(Sqlite,0), recorded in docs/flakytests.md -- it passed alone and passed on a stashed clean tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…unwinding A version upgrade stopped dead with no error and left the tenant below the release version, answering 503 to every owner request until something retried it. The cause is a disagreement between two members of ScopedConnectionFactory. Transaction disposal decrements the ref count to zero, commits, then runs the post-commit actions, and only nulls the transaction after that. In that window HasTransaction is still true, because it asks whether the transaction object exists -- while AddPostCommitAction refuses anything once the ref count is zero. So PublishDriveDefinitionAddedAsync asks whether it is in a transaction, is told yes, defers, and is thrown at. Creating the last drive of the EnsureSystemDrivesExist pass lands exactly there, and the throw escaped where nothing logged it. Publishing immediately is not a fallback here, it is the same guarantee by a shorter route: post-commit actions run only after CommitAsync, so a zero ref count means the commit has already happened. Observers still see committed state, which is the whole reason the deferral exists. Narrow on purpose. HasTransaction keeps its meaning, so TransactionalCache's InDatabaseTransaction and every table's caching are untouched. The three other callers of AddPostCommitAction -- TransactionalCache twice, ShamirConfigurationService twice -- can still reach the same window; they have not, because nothing creates a drive inside their post-commit actions. The disagreement itself is worth fixing at the source, separately and with the whole suite behind it. Not covered by a test: reaching the window needs a drive created from inside a post-commit action of the enclosing transaction, which is what the upgrade pass does on an identity missing drives. The suites cover that the ordinary path still defers. The proof is a v12-era tenant upgrading on the first attempt rather than the second. Odin.Services.Tests 573 passed; Odin.Core.Storage.Tests 513 passed, 3 skipped; Odin.Hosting.Tests.V2 553 passed, 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The v14 -> v15 code is all here -- the migration, its registration, the keypair minting -- but nothing asks a tenant to run it while DataVersionNumber says 14. A tenant already at v14 is current and the upgrade never starts. This is a stop, not a fix. Old tenants on the integration box hang partway through an upgrade: r.r at v0 and f.hobbit at v3 both stop on the first connected identity they touch, in two different phases, holding a transaction open with the database sitting at ClientRead waiting for a command that never comes. Not a lock, not the connection pool, not entropy: Postgres is idle and so is the process. Whatever it is lives in the per-identity ICR work, on the application side, and is not understood yet. Holding at 14 keeps that away from anyone while it is worked out. It also restores the comparison the diagnosis needs: step-1 declares 14 as well, so a v14 tenant behaves the same on both branches, and any difference that remains is this branch's code rather than the fact that only this branch asks for an upgrade. Whether the hang belongs to this branch at all is still open. r.r and f.hobbit have sat at v0 and v3 for a long time and may never have upgraded successfully on any recent build; bumping the version is what made every tenant try, which is a different thing from causing the failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Temporary, and marked so in the code. Do not build on this commit. A v12 -> v14 climb hangs on step-2 and completes on step-1, same tenant, same data, same database schema -- step-1 already has the WriteOnlyKeyPair column and there are no core storage changes between the branches. So the cause is in the handful of lines step-2 adds to that path, not somewhere deeper in the framework. The whole functional delta on that path is the keypair: minted in CreateDriveAsync, serialized in ToRecord, deserialized in ToStorageDriveData. At runtime the only difference is that a drive row carries a serialized ECC keypair where step-1 writes NULL, written on every upsert and read back on every drive read -- and CreateExchangeGrantAsync reads a drive per grant per member, inside the loop that stops on member one. So: mint nothing, write NULL like step-1, change nothing else. If the hang goes with it, the keypair data is the cause and serialization, row size and the drive cache are the next questions. If the hang stays, the keypair is innocent and the only remaining delta is the OdinDatabaseException catch added to PublishDriveDefinitionAddedAsync, which can be reverted by itself. DataVersionNumber is back to 15 so tenants below it actually attempt the climb; at 14 the v14 tenants are skipped and the test says nothing. DriveWriteOnlyKeyPairTests will fail on this build. Nine of its cases assert that a created drive has a keypair, which is exactly what this commit removes. That is the bisect working, not a regression, and CI will be red for that reason. Restore the minting either way -- the branch is built on drives having a keypair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… to it CS8600 in the release build -- "converting null literal to non-nullable type" -- which the Docker image build treats as an error and a local Debug build does not, so it passed here and failed there. No local now: the record takes NULL directly, which is what step-1 writes. Same bisect, one less line. Verified with a Release build this time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SaveIcrAsync held the tenant lock across a write whose transaction disposed inside that lock. Disposing a transaction runs its post-commit actions, and one of those is the deferred DriveDefinitionAddedNotification: its handler goes HandleDriveAdded -> GrantAnonymousRead -> UpdateCircleDefinitionAsync and calls SaveIcrAsync again for the same tenant. The lock is keyed per tenant and is not re-entrant, so the second call waited on a lock the first was still holding, and neither ever returned. It presented as a version upgrade that simply stopped. No exception, so nothing was logged and no failed version recorded; no thread, because both frames were parked async continuations rather than blocked threads; ~2% CPU with an idle thread pool; and Postgres sitting idle-in-transaction at ClientRead holding a connection nobody would ever come back for. Tenants stayed below the release version and answered 503 to every owner request until something retried them -- which worked only because the retry had no drives left to create, so nothing re-entered. Found with dumpasync against a dump of a wedged host: the async stack showed UpdateCircleDefinitionAsync -> SaveIcrAsync -> NodeLock -> KeyedAsyncLock parked beneath a second copy of the same chain, one holding what the other wanted. The transaction now opens outside the lock. Transactions are ref-counted, so the one CircleNetworkStorage.UpsertAsync opens is no longer outermost and no longer runs the post-commit actions; they run when this outer one disposes, by which time the lock is released and the re-entrant call can take it. Ordering changes, semantics do not: the notification still fires only after the commit, which is the reason it was deferred. Not step-2's bug, though that is where it surfaced. This code is identical on both branches; f.hobbit hangs here on step-1 too. What made it reachable was the up-front EnsureSystemDrivesExist pre-pass, which creates drives on every upgrade run regardless of version -- before it, a tenant at v10 or above never created a drive during an upgrade and so never re-entered. No regression test yet, deliberately. The one I wrote does not discriminate: the drive-create-inside-a-transaction shape leaks a ConnectionWrapper whose finalizer aborts the run whether the fix is present or not, so it proved nothing. The verification is f.hobbit on the integration host, which reproduces reliably. A test that guards this needs the harness problem solved first. Odin.Services.Tests: 567 passed. Release build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit ce6b7cc. The fix was wrong. Releasing the lock before the post-commit notification did stop the deadlock, and immediately replaced it with an infinite loop: on the integration host gandalf ran GrantAnonymousRead about sixteen times a second, 1474 calls and climbing, all under one correlation id -- one operation recursing, not many requests. The cycle was there all along: GrantAnonymousRead -> UpdateCircleDefinitionAsync -> SaveIcrAsync -> transaction disposes -> post-commit -> DriveDefinitionAddedNotification -> HandleDriveAdded -> GrantAnonymousRead -> ... The tenant lock was the only thing stopping it, by blocking the second pass forever. That is what the hang was. Removing the block without removing the cycle turns a stall into a spin, which is worse on a live host: it burns CPU and rewrites connection rows rather than sitting still. So the deadlock diagnosis stands -- dumpasync showed the chain parked on KeyedAsyncLock beneath a second copy of itself -- but the lock is a symptom of the cycle, not the defect. The fix belongs where the cycle closes: either HandleDriveAdded stops when the circle already grants the drive, so the second pass is a no-op and the chain ends, or SaveIcrAsync's write stops republishing a drive notification it did not cause. Which of those is right depends on why updating a circle definition raises a drive-added notification at all, and I do not understand that yet. Reverting first so nothing spins on a live host while that is worked out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The post-commit loop ran inside the dispose, while the action list was still populated and _transaction was still set, even though the ref count had already dropped to zero. In that window a post-commit action that touched the database did not stack onto anything: it started a fresh outermost transaction, and disposing that one iterated the same uncleared list again and republished the notification that had just been handled. With a per-tenant lock already held by the outer frame, the re-entry deadlocked, which is what stalled the version upgrade when a new anonymous-read drive was created. Snapshot the actions first, commit or roll back, dispose and clear, and only then run them. Same actions, same order, still only after a successful commit, still caught and logged one by one, and a throwing commit still skips them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts the two BISECT commits. CreateDriveAsync mints the write-only keypair again and writes it to the column, as the branch intends. The bisect answered its question the hard way: the keypair was innocent. The hang was in ScopedConnectionFactory, which ran post-commit actions while the transaction was still half torn down, and step-2 only made it visible by climbing past v14. Fixed on step-1 and merged here. DataVersionNumber stays at 15 so tenants below it actually make the climb; the revert would have put it back to 14, where nothing runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…drive-write-only-keypair
…already unwinding" This reverts commit d53ac3f. The catch existed because HasTransaction said "yes" while AddPostCommitAction said "the ref count is zero" -- a window inside the transaction teardown where the two disagreed. That window is gone: the post-commit actions now run after the transaction is disposed and _transaction is nulled, so HasTransaction is false by the time any action calls back in, and the ordinary else branch handles it. Swallowing OdinDatabaseException here would now hide a real fault rather than paper over a known one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The squash gave main step-1's content as a new commit with no shared ancestry, so the merge base fell back to c28d138 and an ordinary merge tried to apply all of step-1 onto a branch that already has it -- a conflict in every file step-1 touched, none of them real. step-2's tree is already main's tree plus step-2's own work: the main..step-2 diff is exactly the 24 files of the keypair, the public-key endpoints and the v15 migration. So the merge is recorded with step-2's tree, which discards nothing.
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.
No description provided.