Repository navigation
Hold instance_settings to exactly one row (#229) - #234
Merged
Merged
Conversation
Fails first: nothing yet holds the table to one row, so rows with id 2 and 7 go in. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…#229) A data migration keeps the lowest id's row, as first read it, gives it id 1 and removes the rest; the schema migration adds the check. Plain SQL, reversible: down lifts the check and keeps the row. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
find_by(id: 1) || create_or_find_by!(id: 1): two first reads at once leave one row, because one insert wins and the other finds it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
) Replaces #218's no-spec-source mark for ruling 3 with the rule. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…s departed from (#229) Review findings: a restored backup brings its own row and the migrations never create one; Sources lists the fixed issue apart from the open one. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Resolves the shared lines of tenancy-and-accounts.md: main's text minus this branch's own clause (#229). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
plainprogrammer
marked this pull request as ready for review
November 9, 2026 17:25
This was referenced Nov 9, 2026
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
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.
Closes #229
Refs #218 (ruling 3)
What
instance_settingsis held to one row by the database, andInstanceSetting.currentmakes that row race-safely.CHECK (id = 1)(instance_settings_one_row). With the primary key, a second row, or a row with any other id, is refused whatever inserts it. ACHECKon the id rather than a unique index on a constant column: no extra column,schema.rbdumps it, and the row's identity is fixed.20261109200001 NormaliseInstanceSettingsToOneRow(data step): keeps the lowest id's row, asfirstread it, gives it id 1 and removes the rest (the old first-read race's leftovers).downdoes nothing. It creates no row.20261109200002 AddOneRowCheckToInstanceSettings(schema step): adds the check;downlifts it and keeps the row. Idempotent.InstanceSetting.currentisfind_by(id: 1) || create_or_find_by!(id: 1): two first reads at once leave one row, because one insert wins and the other finds it.docs/architecture/tenancy-and-accounts.mdreplaces Accounts and sessions: six behaviours no spec describes #218's "no spec source" mark for ruling 3 with the rule, citing Accounts and sessions: six behaviours no spec describes #218 and Hold instance_settings to exactly one row #229. Two lessons are indocs/lessons/.Deviation from the issue's text (approved by the coordinator, who has told the maintainer)
#229 says the migration creates the row and
currentno longer creates it.currentstill makes the row on first use (race-safe now). The reason:db:prepareon a blank volume loadsdb/schema.rband runs no migration, and the test database is built the same way, so a row made only by a migration would be missing on a fresh instance, and first sign-up would break. A restored older backup brings its own row (or none); the first start's migrations normalise it and never create one. The migration therefore makes no row on an empty table either. The area doc states the departure.Tests (written first)
spec/models/instance_setting_spec.rbwas committed red first (31e74e58): rows with ids 2 and 7 went in. It also coverscurrenton an empty table, repeated reads, and a stale read that loses the insert race (a stub offind_bysimulates the stale read).spec/migrations/normalise_instance_settings_to_one_row_spec.rbandadd_one_row_check_to_instance_settings_spec.rb: duplicates, one row with another id, no row, running twice, down and up, and a table the guard accepts.LoadErrorand landed in the same commit as the migrations, so the history doesn't show the data step failing on its own (lesson recorded).spec/backup_upgrade_spec.rb(rolls every migration since the last release back and forward again) andspec/backup_guards_spec.rbpass; a blankdb:preparefollowed bycurrentmakes id 1.Implementation review (Mode B, Opus, read-only, no spec; criteria #229, #218 ruling 3 and the coordinator's ruling)
SPEC-ALIGNED. No blocking or major findings. Minor: (1) the test-first gap above; (2) the area doc said a restored older backup gets its row from the first start's migrations, which is false: fixed in
50f7b66b; (3) the shared test database could keep running without the check if an around-hook's cleanup failed, the same pattern as the existing add_currency and add_location specs. It verified the data migration's SQL on 0, 1 and 2+ rows, the table copy on SQLite, down/redo, the timestamps,create_or_find_by!inside the sign-up and command transactions, and the upgrade proof (expected_changes.ymlstays[]).The area doc change was also reviewed (Fable, read-only): changes needed, all applied in
50f7b66b.CI
Final head
e75a6eab(this branch merged withmainthrough #236): the pre-push fullbin/cipassed, and GitHub CI is green (RSpec 1-4/4, checks,Imageon amd64 and arm64, which runs the upgrade smoke with these migrations). Earlier on736d146c: the first pre-push attempt failed on one unrelated timing flake,spec/system/bulk_move_spec.rb:54(#233, fixed in #235), and GitHub's amd64 image job hit the known upgrade-smoke shutdown race #177 (exit 137 before the comparison); both passed on re-run or on a later head. The suite has 2852 examples by--dry-run.Owed to the maintainer
🤖 Generated with Claude Code