Skip to content

Hold instance_settings to exactly one row (#229) - #234

Merged
plainprogrammer merged 8 commits into
mainfrom
229-one-instance-setting
Nov 9, 2026
Merged

plainprogrammer merged 8 commits into
mainfrom
229-one-instance-setting

Conversation

@plainprogrammer

@plainprogrammer plainprogrammer commented Nov 9, 2026 •

Copy link
Copy Markdown
Owner

Closes #229
Refs #218 (ruling 3)

What

instance_settings is held to one row by the database, and InstanceSetting.current makes that row race-safely.

  • Guard: 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. A CHECK on the id rather than a unique index on a constant column: no extra column, schema.rb dumps it, and the row's identity is fixed.
  • Migrations (plain SQL, no application models, safe with 0, 1 or 2+ rows):
    • 20261109200001 NormaliseInstanceSettingsToOneRow (data step): keeps the lowest id's row, as first read it, gives it id 1 and removes the rest (the old first-read race's leftovers). down does nothing. It creates no row.
    • 20261109200002 AddOneRowCheckToInstanceSettings (schema step): adds the check; down lifts it and keeps the row. Idempotent.
  • InstanceSetting.current is 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.
  • Area doc: docs/architecture/tenancy-and-accounts.md replaces 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 in docs/lessons/.

Deviation from the issue's text (approved by the coordinator, who has told the maintainer)

#229 says the migration creates the row and current no longer creates it. current still makes the row on first use (race-safe now). The reason: db:prepare on a blank volume loads db/schema.rb and 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.rb was committed red first (31e74e58): rows with ids 2 and 7 went in. It also covers current on an empty table, repeated reads, and a stale read that loses the insert race (a stub of find_by simulates the stale read).
  • spec/migrations/normalise_instance_settings_to_one_row_spec.rb and add_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.
  • Honest gap, noted by the review: the migration specs were red only as a LoadError and landed in the same commit as the migrations, so the history doesn't show the data step failing on its own (lesson recorded).
  • Backup and restore: spec/backup_upgrade_spec.rb (rolls every migration since the last release back and forward again) and spec/backup_guards_spec.rb pass; a blank db:prepare followed by current makes 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.yml stays []).

The area doc change was also reviewed (Fable, read-only): changes needed, all applied in 50f7b66b.

CI

Final head e75a6eab (this branch merged with main through #236): the pre-push full bin/ci passed, and GitHub CI is green (RSpec 1-4/4, checks, Image on amd64 and arm64, which runs the upgrade smoke with these migrations). Earlier on 736d146c: 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

  • Nothing manual. The upgrade smoke ran in GitHub's image job on the final head and passed on both architectures.

🤖 Generated with Claude Code

plainprogrammer and others added 6 commits November 9, 2026 11:27
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 plainprogrammer self-assigned this Nov 9, 2026
Takes main's text for the shared lines of tenancy-and-accounts.md (#235, #236) minus this branch's own clause, and recounts: two behaviours remain with no spec source (#229).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@plainprogrammer
plainprogrammer marked this pull request as ready for review November 9, 2026 17:25
@plainprogrammer
plainprogrammer merged commit ca24456 into main Nov 9, 2026
9 checks passed
@plainprogrammer
plainprogrammer deleted the 229-one-instance-setting branch November 9, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hold instance_settings to exactly one row

1 participant