fix(db): drop the premature event_order index that crash-loops Postgres upgrades - #2513
fix(db): drop the premature event_order index that crash-loops Postgres upgrades#2513133Felix wants to merge 1 commit into
Conversation
…es upgrades
`initSchema()` applies `migrations/000_combined.sql` top-to-bottom inside a
single transaction and re-throws fatally, so one statement referencing a column
that does not exist yet rolls the whole apply back and crash-loops the boot.
`idx_workflow_events_run_order` was created twice: once inside the
`CREATE TABLE IF NOT EXISTS remote_agent_workflow_events` section, and again in
the additive block directly after the `ALTER TABLE ... ADD COLUMN IF NOT EXISTS
event_order` that creates the column it indexes.
On a FRESH database the first one works — `CREATE TABLE` runs for real and
declares `event_order` inline. On an EXISTING database `CREATE TABLE IF NOT
EXISTS` is a no-op, so the column is absent ~265 lines before the ALTER that
adds it, and the index aborts with:
ERROR: 42703: column "event_order" does not exist
That asymmetry is why this shipped green: only upgrades are affected, and only
those from a database predating coleam00#2414.
The fix is to delete the premature copy. The one in the additive block is
correct for both cases — it runs on every apply, after the column is guaranteed
to exist — so nothing is lost for fresh installs.
Adds a statement-ordering guard that reads the SQL and asserts every reference
to `event_order` (the unique index, the sequence's OWNED BY, and the ALTER
COLUMN ... SET DEFAULT) follows the ADD COLUMN, and that the CREATE TABLE body
still declares the column so fresh installs stay correct. No mocks, no database.
coleam00#2418 applied the same reordering to the SQLite path; this is the Postgres one.
📝 WalkthroughWalkthroughThe migration defers the workflow-events index until after ChangesMigration ordering
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/db/migration-statement-order.test.ts`:
- Around line 34-36: Guard the match offset in the indexRef loop before adding
it to indexOffsets, skipping or otherwise handling matches where m.index is
undefined. Ensure only numeric offsets are pushed into the number[] while
preserving the existing CREATE UNIQUE INDEX matching behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bc70e37-21ef-4334-8115-f1a251a27540
⛔ Files ignored due to path filters (1)
packages/core/src/db/bundled-schema.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (3)
migrations/000_combined.sqlpackages/core/package.jsonpackages/core/src/db/migration-statement-order.test.ts
| const indexOffsets: number[] = []; | ||
| const indexRef = /CREATE\s+UNIQUE\s+INDEX[^;]*idx_workflow_events_run_order/gi; | ||
| for (const m of SCHEMA_SQL.matchAll(indexRef)) indexOffsets.push(m.index); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep -F 'packages/core/src/db/migration-statement-order.test.ts' || true
echo "== package tsconfig options =="
fd -a 'tsconfig.*json' . | sed 's#^\./##' | head -50
for f in $(fd 'tsconfig.*json' .); do
echo "--- $f"
jq '{compilerOptions?: .compilerOptions}' "$f" 2>/dev/null | head -40
done
echo "== relevant test lines =="
cat -n packages/core/src/db/migration-statement-order.test.ts | sed -n '1,90p'
echo "== TypeScript availability/version =="
if command -v node >/dev/null 2>&1; then node --version; fi
if command -v bun >/dev/null 2>&1; then bun --version; fi
if command -v tsc >/dev/null 2>&1; then tsc --version; fi
echo "== local TS lib declaration excerpt if available =="
node - <<'JS'
try {
const libPath = require.resolve('typescript/lib/lib.es5.d.ts');
const fs = require('fs');
const text = fs.readFileSync(libPath, 'utf8');
const matches = [...text.matchAll(/interface\s+RegExpMatchArray[\s\S]*?^}/m)];
console.log('found lib:', libPath);
for (const m of matches) console.log(m[0]);
} catch (e) {
console.log('typescript lib not resolvable:', e.message);
}
JSRepository: coleam00/Archon
Length of output: 909
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package/core tsconfig =="
cat -n packages/core/tsconfig.json
echo "== jsconfig/tslint/eslint rules if present =="
for f in tsconfig.json jsconfig.json tslint.json eslint.config.js eslint.config.mjs eslint.config.cjs; do
if [ -f "$f" ]; then echo "--- $f"; sed -n '1,240p' "$f"; fi
done
echo "== relevant test lines =="
cat -n packages/core/src/db/migration-statement-order.test.ts | sed -n '1,100p'
echo "== RegExpMatchArray declaration excerpt =="
node - <<'JS'
try {
const libPath = require.resolve('typescript/lib/lib.es5.d.ts');
const fs = require('fs');
const text = fs.readFileSync(libPath, 'utf8');
const matches = [...text.matchAll(/interface\s+RegExpMatchArray[\s\S]*?^}/m)];
console.log('found lib:', libPath);
for (const m of matches) {
console.log(m[0]);
break;
}
} catch (e) {
console.log('typescript lib not resolvable:', e.message);
}
try {
const ts = require('typescript');
console.log('typescript version:', ts.version);
} catch (e) {
console.log('typescript package not resolvable:', e.message);
}
JSRepository: coleam00/Archon
Length of output: 10557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package manifests mentioning typescript/bun/types =="
for f in package.json bun.lockb bun.lock pnpm-lock.yaml yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f"
case "$f" in
package.json) jq '{dependencies, devDependencies}' "$f" ;;
bun.lockb|bun.lock|pnpm-lock.yaml|yarn.lock) sed -n '1,220p' "$f" ;;
esac
fi
done
echo "== check installed bun-types declarations for RegExpMatchArray (if available) =="
node - <<'JS'
const paths = [
process.env.NODE_PATH,
'/usr/local/lib/node_modules/bun-types/global.d.ts',
'/usr/local/lib/node_modules/@types/bun/global.d.ts',
'/usr/local/lib/node_modules/@types/bun/index.d.ts',
].filter(Boolean);
for (const p of paths) {
try {
const fs = require('fs');
const text = fs.readFileSync(p, 'utf8');
console.log('FOUND:', p);
const idx = text.indexOf('interface RegExpMatchArray');
if (idx === -1) {
console.log(' no RegExpMatchArray');
} else {
console.log(text.slice(idx, text.indexOf('\n}', idx) + 3));
}
} catch (e) {}
}
try {
// Read from any node_modules available, if present.
const fs = require('fs');
const base = require('path').join(__dirname, 'node_modules', '`@types`', 'bun', 'global.d.ts');
const text = fs.readFileSync(base, 'utf8');
console.log('FOUND-local:', base);
console.log(text.includes('interface RegExpMatchArray') ? 'contains' : 'missing');
} catch (e) {}
JS
echo "== run package check for TypeScript type errors =="
if command -v bun >/dev/null 2>&1; then
bun run --filter `@archon/core` check
fiRepository: coleam00/Archon
Length of output: 7547
🌐 Web query:
TypeScript RegExpMatchArray index optional official types
💡 Result:
In TypeScript, the index and input properties on the RegExpMatchArray type are defined as optional (e.g., index?: number, input?: string) [1]. This design reflects the runtime behavior of the String.prototype.match method, which returns these properties only when the regular expression does not use the global (g) flag [2][1][3]. When the global flag is used, String.prototype.match returns an array of matched strings without the additional properties such as index and input, which is why TypeScript marks them as optional in the type definition [2][1][3]. If you are working in an environment where you know these properties will be present (i.e., you are not using the global flag), you may need to use a type assertion or a non-null assertion operator (e.g., match!.index) to inform the TypeScript compiler that the values are defined [4]. Alternatively, if you need guaranteed index and input properties, you might consider using RegExp.prototype.exec instead, which returns a RegExpExecArray, where these properties are traditionally treated as present [5][1].
Citations:
- 1: https://docs.syntblaze.com/typescript/built-in-object-types/regexp
- 2: Bug: RegExpMatchArray is missing properties
indexandinputmicrosoft/TypeScript#35157 - 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match
- 4: Make RegExpMatchArray index and input field non-optional microsoft/TypeScript#50211
- 5:
RegExpMatchArrayindex 0 incorrectly possiblyundefinedwhennoUncheckedIndexedAccessis enabled microsoft/TypeScript#42296
Handle the optional match offset before pushing it.
RegExpMatchArray.index is optional, so indexOffsets.push(m.index) passes number | undefined to number[]. Add a guard for m.index === undefined before pushing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/db/migration-statement-order.test.ts` around lines 34 - 36,
Guard the match offset in the indexRef loop before adding it to indexOffsets,
skipping or otherwise handling matches where m.index is undefined. Ensure only
numeric offsets are pushed into the number[] while preserving the existing
CREATE UNIQUE INDEX matching behavior.
Source: Coding guidelines
|
Independent verification of this fix, from a production install that hit #2508. Real-world impact first: this bug took our Archon deployment down for ~13 minutes on 2026-08-09. We bumped 0.5.0 → 0.8.0 having passed workflow validation, our full contract suite, unit tests at 100% coverage, runtime gate probes and green CI — every one of which runs against a fresh database. The upgrade crash-looped immediately on our existing Postgres and we rolled back. #2508's own line about why it shipped green is exactly why our checks were blind to it too:
Verification method. Schema-only Results — three paths, same fixture:
Post-state assertions on the upgrade path with this PR applied:
I checked the index specifically because "delete a Rollback behaviour confirmed too: after the failing v0.8.0 apply, This fix resolves the failure for us. We've pinned to 0.7.0 in the meantime, since 0.7.1 and 0.8.0 both reproduce it against a pre-#2414 database. One suggestion, offered rather than asked for: the reason this class escapes CI is that every migration test starts from an empty database. A CI job that applies |
|
Follow-up to my earlier SQL-level verification — I've now tested the branch itself, built and booted, rather than just the migration file. End-to-endBuilt Archon from For contrast, on the same fixture: The shipped SQL in the built image is also correct — exactly one real statement, with the deletion documented in place: The new regression test earns its placeI checked it isn't a tautology by running it against v0.8.0's unfixed SQL — it fails, at the It's also broader than the reported bug: it asserts every Other checks
One note for anyone else verifying: running No issues found. This resolves the crash-loop for us; we're pinned to 0.7.0 until a release carries it. |
|
@133Felix — apologies, this PR was closed by mistake and I've reopened it. It was not a rejection, and nothing about the fix prompted it. What happened: we carry a downstream image that pins Archon as a base. Yesterday we shipped a temporary in-image patch of #2508 so we could run 0.8.0 before a release carries this fix. The commit message for that change contained the line:
I'm sorry. Having a correct, independently-verified fix closed silently by an unrelated downstream merge is precisely the kind of noise that makes contributing annoying, and you had no way to tell what had happened. For the record, restating what I posted before the accidental close, since it now sits above it:
#2508 is still open and I don't see another PR addressing it, so as far as I can tell every Postgres install predating #2414 still can't upgrade. From our side this cost ~13 minutes of production downtime and we're now pinned behind a local patch until something lands. I've changed our commit conventions so a downstream reference can't do this again. |
2 participants
Summary
migrations/000_combined.sqlagainst an existing database aborts withERROR: 42703: column "event_order" does not exist.idx_workflow_events_run_orderis created in theCREATE TABLE IF NOT EXISTS remote_agent_workflow_eventssection (line 285), ~265 lines before theALTER TABLE ... ADD COLUMN IF NOT EXISTS event_orderthat creates the column it indexes (line 549).initSchema()wraps the whole file in one transaction and re-throwsfatal, so the apply rolls back and the server crash-loops — every Postgres install predating feat: summarize verbose workflow JSON nodes #2414 fails to start after upgrading, and every CLI invocation fails too (both apply schema on connect). Fresh installs are unaffected, because the realCREATE TABLEdeclaresevent_orderinline — which is exactly why this shipped green.CREATE UNIQUE INDEX. The identical statement in the additive block (line 556) already creates it directly after theADD COLUMN, which is correct for both a fresh and an upgrading database. Added a statement-ordering regression test that reads the SQL and asserts on offsets.event_order BIGINTin theCREATE TABLEbody (kept — it is what makes fresh installs correct), and the two sibling instances of this same bug class (idx_conversations_hidden,idx_sessions_parent) which are already covered by the open fix(migrations): create indexes after ADD COLUMN for existing Postgres DBs #2443.UX Journey
Before
After
Fresh installs take the same path and are unchanged:
CREATE TABLEdeclaresevent_orderinline, theADD COLUMNis a no-op, and the single index creation succeeds.Architecture Diagram
Before
After
Connection inventory
migrations/000_combined.sqlbundled-schema.generated.tsbun run generate:bundled-schema;check:bundled-schemagreenpostgres.ts initSchema()migrations/000_combined.sqlmigration-statement-order.test.tsmigrations/000_combined.sqlsqlite.ts createSchema()migrations/000_combined.sqlLabel Snapshot
risk: lowsize: Scorecore:dbChange Metadata
bugcoreLinked Issue
idx_conversations_hidden/idx_sessions_parent)Validation Evidence (required)
bun run validate # EXIT=0 — all nine checks greenbun run validateexits 0:check:bundled,check:bundled-skill,check:bundled-schema,check:pi-vendor-map,check:capability-matrix,type-check,lint --max-warnings 0,format:check,test:install,test.git stash push -- migrations/000_combined.sql):3 pass, 0 fail.Security Impact (required)
NoNoNoNo— the new test readsmigrations/000_combined.sqlfrom the repo, the same filegetSchemaSQL()already reads in source builds.Compatibility / Migration
YesNoNo— this repairs the migration itself. Fresh installs are byte-identical in outcome (same column, same sequence, same index, same default); upgrades now converge instead of aborting.Human Verification (required)
db.postgres_schema_init_failed→database_connection_failed→ exit 1, repeating indefinitely;/api/healthnever answered. Log:ADD COLUMNby hand and restarting brought the app up immediately (healthy after ~6s), which isolates this statement as the sole cause.output_rootpresent,event_ordercarryingDEFAULT nextval(...),idx_workflow_events_run_orderpresent,remote_agent_schema_versioncreated (18 → 19 tables).event_order IS NULL— no backfill, exactly as the migration comment intends, and what theCOALESCE(event_order, 0)read path expects.CREATE TABLEbody still declaresevent_order, asserted by the third test so a later cleanup cannot remove it and silently break new installs.OWNED BYand theALTER COLUMN ... SET DEFAULTare asserted to follow theADD COLUMNtoo — they fail with the same 42703 if reordered.CREATE TABLEbody is unchanged) plus the third test, not by an end-to-end fresh apply.hiddenandparent_session_idalready existed in my database.Side Effects / Blast Radius (required)
initSchema()), which every server start and every CLI invocation runs. Nothing at runtime — no query, no read path, no API surface changes.db.postgres_schema_init_failedis already logged atfatalwith the SQLSTATE and byte position, which is what made this diagnosable in minutes. The new test fails at CI time if any statement referencingevent_ordermigrates back above theADD COLUMN.Rollback Plan (required)
git revert <sha>— the change is one deleted SQL statement plus a test. Reverting restores the previous (broken-for-upgrades) behaviour, so it should only be done if the deletion turns out to break fresh installs, which the third test guards against.column "..." does not existat boot indb.postgres_schema_init_failed, oridx_workflow_events_run_ordermissing after a successful apply (\d remote_agent_workflow_events).Risks and Mitigations
CREATE TABLEsection could leave a fresh install without it, if the additive block were ever made conditional.ADD COLUMN, and that theCREATE TABLEbody still declares the column.migrations/000_combined.sql.event_orderrather than a general "no index before its column" rule.Summary by CodeRabbit
Bug Fixes
Tests