Skip to main content

max / makenotwork

server: guard concurrent-index migration hygiene Forward-only guard test fails CI if a migration past the frozen high-water mark creates an index on a growth table without CONCURRENTLY, uses CONCURRENTLY without the -- no-transaction prefix, or omits IF NOT EXISTS. CONTRIBUTING documents the lane. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-05 15:49 UTC
Commit: 049dea0962533924c1f857175e69b878cabd019f
Parent: 1899933
2 files changed, +320 insertions, -0 deletions
M server/CONTRIBUTING.md +9
@@ -229,6 +229,15 @@ Migrations live in `server/migrations/` and are numbered sequentially (e.g., `00
229 229 - Create indexes after the table definition, in the same migration.
230 230 - Prefer additive migrations (add columns, add tables). Destructive changes (drop columns, rename tables) need careful planning.
231 231 - Name migrations descriptively: `NNN_what_it_does.sql`.
232 + - **Indexes on growth tables must be `CONCURRENTLY`.** A plain `CREATE INDEX` takes an `ACCESS EXCLUSIVE` lock and blocks writes for the whole build — fine on a small table, a production write-stall on `transactions`/`page_views`/`subscriptions`/etc. `CREATE INDEX CONCURRENTLY` cannot run inside a transaction, so a migration that uses it **must start with the exact line `-- no-transaction`** (sqlx runs that file outside its per-migration transaction). Note the trade-off: a `-- no-transaction` migration is not atomic, so keep it to the single concurrent index build.
233 +
234 + ```sql
235 + -- no-transaction
236 + CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_transactions_seller_created
237 + ON transactions (seller_user_id, created_at DESC);
238 + ```
239 +
240 + The `migration_hygiene` test (`tests/migration_hygiene.rs`) enforces both rules — concurrent-on-growth-table and `IF NOT EXISTS` — for every migration past the frozen high-water mark (historical migrations can't change: sqlx checksums applied files). Bump `HIGH_WATER` there only after deliberately reviewing the migrations you're grandfathering.
232 241
233 242 ```sql
234 243 -- Example: 004_file_scan_status.sql
A server/tests/migration_hygiene.rs +311
@@ -0,0 +1,311 @@
1 + //! CI guard for migration safety hygiene (forward-only).
2 + //!
3 + //! sqlx checksums every *applied* migration, so historical migrations can never
4 + //! be edited — retrofitting `CONCURRENTLY` / `IF NOT EXISTS` onto migrations
5 + //! `001..=HIGH_WATER` is impossible without breaking the checksum guard. This
6 + //! test therefore freezes the existing set and enforces the conventions only on
7 + //! NEW migrations (number > `HIGH_WATER`):
8 + //!
9 + //! 1. A `CREATE INDEX` on a high-write "growth" table must be `CONCURRENTLY`.
10 + //! A plain `CREATE INDEX` takes an `ACCESS EXCLUSIVE` lock and blocks writes
11 + //! for the whole build; on a grown `transactions`/`page_views` table that is
12 + //! a production write-stall.
13 + //! 2. `CREATE INDEX CONCURRENTLY` cannot run inside a transaction, so any file
14 + //! that uses it must opt out of sqlx's per-migration transaction by starting
15 + //! with the exact bytes `-- no-transaction` (sqlx-core 0.8 `source.rs:127`
16 + //! does `sql.starts_with("-- no-transaction")` — it must be the file prefix,
17 + //! no leading blank line).
18 + //! 3. `CREATE TABLE` / `CREATE INDEX` must be `IF NOT EXISTS` (re-run safety —
19 + //! a partially-applied migration set can be re-run without erroring).
20 + //!
21 + //! Bump `HIGH_WATER` only after deliberately reviewing every migration at or below
22 + //! the new mark and accepting its state.
23 + //!
24 + //! Run with: cargo test --test migration_hygiene
25 +
26 + use std::fs;
27 + use std::path::Path;
28 +
29 + /// Highest migration number whose contents are grandfathered. Landed at the
30 + /// current max during the Run 23 remediation (2026-07-04). Anything strictly
31 + /// greater must satisfy the conventions above.
32 + const HIGH_WATER: u32 = 167;
33 +
34 + /// Tables large/hot enough that a blocking (non-concurrent) index build is a real
35 + /// production write-stall. A `CREATE INDEX` touching one of these in a new
36 + /// migration must be `CONCURRENTLY`.
37 + const GROWTH_TABLES: &[&str] = &[
38 + "transactions",
39 + "page_views",
40 + "follows",
41 + "subscriptions",
42 + "items",
43 + "item_versions",
44 + "sync_blobs",
45 + "webhook_events",
46 + "processed_webhook_events",
47 + ];
48 +
49 + const MIGRATIONS_DIR: &str = "migrations";
50 +
51 + #[test]
52 + fn new_migrations_follow_index_and_rerun_safety_conventions() {
53 + let mut violations = Vec::new();
54 +
55 + let mut files: Vec<_> = fs::read_dir(MIGRATIONS_DIR)
56 + .expect("read migrations dir")
57 + .filter_map(|e| e.ok())
58 + .map(|e| e.path())
59 + .filter(|p| p.extension().is_some_and(|x| x == "sql"))
60 + .collect();
61 + files.sort();
62 +
63 + for path in &files {
64 + let Some(num) = leading_number(path) else {
65 + violations.push(format!("{}: filename has no leading number", show(path)));
66 + continue;
67 + };
68 + if num <= HIGH_WATER {
69 + continue; // grandfathered — frozen by sqlx checksum
70 + }
71 +
72 + let raw = fs::read_to_string(path).expect("read migration");
73 + let no_tx = raw.starts_with("-- no-transaction");
74 + let stmts = statements(&raw);
75 +
76 + for stmt in &stmts {
77 + if let Some(idx) = parse_create_index(stmt) {
78 + if idx.growth_target() && !idx.concurrently {
79 + violations.push(format!(
80 + "{}: CREATE INDEX on growth table `{}` must be CONCURRENTLY \
81 + (plain build takes ACCESS EXCLUSIVE and stalls writes)",
82 + show(path), idx.table
83 + ));
84 + }
85 + if idx.concurrently && !no_tx {
86 + violations.push(format!(
87 + "{}: uses CREATE INDEX CONCURRENTLY but the file does not start \
88 + with `-- no-transaction` — sqlx will wrap it in a transaction \
89 + and Postgres will reject it at runtime",
90 + show(path)
91 + ));
92 + }
93 + if !idx.if_not_exists {
94 + violations.push(format!(
95 + "{}: CREATE INDEX must be IF NOT EXISTS for re-run safety",
96 + show(path)
97 + ));
98 + }
99 + } else if is_create_table(stmt) && !has_if_not_exists(stmt) {
100 + violations.push(format!(
101 + "{}: CREATE TABLE must be IF NOT EXISTS for re-run safety",
102 + show(path)
103 + ));
104 + }
105 + }
106 + }
107 +
108 + assert!(
109 + violations.is_empty(),
110 + "migration hygiene violations (see tests/migration_hygiene.rs for the rules):\n{}",
111 + violations.join("\n")
112 + );
113 + }
114 +
115 + /// The guard must not silently pass because every new migration was skipped by a
116 + /// wrong `HIGH_WATER`: assert the mark is not below the real maximum on disk.
117 + /// (If someone adds `168_*.sql`, the mark stays at 167 and 168 is checked — good.
118 + /// This only fires if `HIGH_WATER` is set *above* the newest file, which would
119 + /// disable the guard entirely.)
120 + #[test]
121 + fn high_water_mark_is_not_ahead_of_disk() {
122 + let max = fs::read_dir(MIGRATIONS_DIR)
123 + .expect("read migrations dir")
124 + .filter_map(|e| e.ok())
125 + .filter_map(|e| leading_number(&e.path()))
126 + .max()
127 + .expect("at least one migration");
128 + assert!(
129 + HIGH_WATER <= max,
130 + "HIGH_WATER ({HIGH_WATER}) is ahead of the newest migration ({max}) — \
131 + the hygiene guard would skip every file. Lower HIGH_WATER."
132 + );
133 + }
134 +
135 + // ---- helpers -------------------------------------------------------------
136 +
137 + fn show(p: &Path) -> String {
138 + p.file_name().unwrap().to_string_lossy().into_owned()
139 + }
140 +
141 + fn leading_number(p: &Path) -> Option<u32> {
142 + let name = p.file_name()?.to_str()?;
143 + let digits: String = name.chars().take_while(|c| c.is_ascii_digit()).collect();
144 + digits.parse().ok()
145 + }
146 +
147 + /// Split into statements: strip `--` line comments and `/* */` block comments,
148 + /// lowercase, collapse whitespace, split on `;`.
149 + fn statements(raw: &str) -> Vec<String> {
150 + // Strip block comments.
151 + let mut no_block = String::with_capacity(raw.len());
152 + let bytes = raw.as_bytes();
153 + let mut i = 0;
154 + while i < bytes.len() {
155 + if i + 1 < bytes.len() && &raw[i..i + 2] == "/*" {
156 + if let Some(end) = raw[i..].find("*/") {
157 + i += end + 2;
158 + no_block.push(' ');
159 + continue;
160 + }
161 + break;
162 + }
163 + no_block.push(bytes[i] as char);
164 + i += 1;
165 + }
166 + // Strip line comments and normalize.
167 + let cleaned: String = no_block
168 + .lines()
169 + .map(|l| match l.find("--") {
170 + Some(pos) => &l[..pos],
171 + None => l,
172 + })
173 + .collect::<Vec<_>>()
174 + .join(" ");
175 + let norm = cleaned.to_lowercase();
176 + norm.split(';')
177 + .map(|s| s.split_whitespace().collect::<Vec<_>>().join(" "))
178 + .filter(|s| !s.is_empty())
179 + .collect()
180 + }
181 +
182 + fn is_create_table(stmt: &str) -> bool {
183 + stmt.starts_with("create table") || stmt.starts_with("create unlogged table")
184 + }
185 +
186 + fn has_if_not_exists(stmt: &str) -> bool {
187 + stmt.contains("if not exists")
188 + }
189 +
190 + struct IndexStmt {
191 + concurrently: bool,
192 + if_not_exists: bool,
193 + table: String,
194 + }
195 +
196 + impl IndexStmt {
197 + fn growth_target(&self) -> bool {
198 + GROWTH_TABLES.contains(&self.table.as_str())
199 + }
200 + }
201 +
202 + /// Parse a `CREATE [UNIQUE] INDEX [CONCURRENTLY] [IF NOT EXISTS] name ON table ...`.
203 + /// Returns None if the statement is not a create-index.
204 + fn parse_create_index(stmt: &str) -> Option<IndexStmt> {
205 + let tokens: Vec<&str> = stmt.split_whitespace().collect();
206 + // Must begin create [unique] index
207 + let idx_pos = tokens.iter().position(|t| *t == "index")?;
208 + if tokens.first() != Some(&"create") {
209 + return None;
210 + }
211 + match idx_pos {
212 + 1 => {} // create index
213 + 2 if tokens.get(1) == Some(&"unique") => {} // create unique index
214 + _ => return None,
215 + }
216 + let after: Vec<&str> = tokens[idx_pos + 1..].to_vec();
217 + let concurrently = after.first() == Some(&"concurrently");
218 + let if_not_exists = stmt.contains("if not exists");
219 + // Table name is the token following `on`.
220 + let on_pos = after.iter().position(|t| *t == "on")?;
221 + let raw_table = after.get(on_pos + 1)?;
222 + let table = raw_table
223 + .trim_matches(|c| c == '"' || c == '(')
224 + .split('(')
225 + .next()
226 + .unwrap_or(raw_table)
227 + .to_string();
228 + Some(IndexStmt {
229 + concurrently,
230 + if_not_exists,
231 + table,
232 + })
233 + }
234 +
235 + // ---- self-tests: prove the detector fires, so the on-disk pass is not vacuous ----
236 +
237 + #[cfg(test)]
238 + mod detector_tests {
239 + use super::*;
240 +
241 + fn only(stmt: &str) -> String {
242 + statements(stmt).into_iter().next().unwrap()
243 + }
244 +
245 + #[test]
246 + fn flags_plain_index_on_growth_table() {
247 + let idx = parse_create_index(&only(
248 + "CREATE INDEX IF NOT EXISTS idx_x ON transactions (seller_user_id);",
249 + ))
250 + .unwrap();
251 + assert!(idx.growth_target());
252 + assert!(!idx.concurrently, "plain build must be caught");
253 + assert!(idx.if_not_exists);
254 + }
255 +
256 + #[test]
257 + fn accepts_concurrent_index_on_growth_table() {
258 + let idx = parse_create_index(&only(
259 + "CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_x ON transactions (created_at);",
260 + ))
261 + .unwrap();
262 + assert!(idx.growth_target());
263 + assert!(idx.concurrently);
264 + assert!(idx.if_not_exists);
265 + }
266 +
267 + #[test]
268 + fn small_table_index_is_not_a_growth_target() {
269 + let idx =
270 + parse_create_index(&only("CREATE INDEX IF NOT EXISTS i ON promo_codes (id);")).unwrap();
271 + assert!(!idx.growth_target(), "non-hot table needn't be concurrent");
272 + }
273 +
274 + #[test]
275 + fn detects_missing_if_not_exists() {
276 + let idx = parse_create_index(&only("CREATE INDEX i ON items (slug);")).unwrap();
277 + assert!(!idx.if_not_exists);
278 + assert!(idx.growth_target());
279 + }
280 +
281 + #[test]
282 + fn no_transaction_directive_must_be_file_prefix() {
283 + // sqlx does sql.starts_with("-- no-transaction") — a leading blank line breaks it.
284 + assert!("-- no-transaction\nCREATE INDEX CONCURRENTLY ...".starts_with("-- no-transaction"));
285 + assert!(!"\n-- no-transaction\n...".starts_with("-- no-transaction"));
286 + }
287 +
288 + #[test]
289 + fn unique_index_parsed_and_table_extracted_without_paren() {
290 + let idx = parse_create_index(&only(
291 + "CREATE UNIQUE INDEX CONCURRENTLY i ON subscriptions(user_id);",
292 + ))
293 + .unwrap();
294 + assert_eq!(idx.table, "subscriptions");
295 + assert!(idx.concurrently);
296 + }
297 +
298 + #[test]
299 + fn create_table_without_ine_is_flagged() {
300 + let s = only("CREATE TABLE foo (id UUID PRIMARY KEY);");
301 + assert!(is_create_table(&s));
302 + assert!(!has_if_not_exists(&s));
303 + }
304 +
305 + #[test]
306 + fn line_and_block_comments_are_stripped() {
307 + let s = statements("/* c */ CREATE INDEX i ON items (a); -- trailing\n");
308 + assert_eq!(s.len(), 1);
309 + assert!(parse_create_index(&s[0]).is_some());
310 + }
311 + }