Repository navigation
Promote dev to stable: Genie observability resume-anchor fix - #2475
Conversation
`pg_dump --clean --if-exists` (used by `genie db backup`) drops every IDENTITY-bearing table and recreates it via DDL — resetting the implicit identity sequence to its initial value (1). The subsequent COPY restores rows with their original explicit ids (1..N), but pg_dump does NOT emit a setval() to advance the sequence past N. The next IDENTITY-generated INSERT then collides with an existing row's PK, breaking every PG-touching CLI command. Observed on khal-os 2026-05-22: `_genie_migrations` had 63 rows but `_genie_migrations_id_seq` was at 48; the first attempt to apply pending migration 064 hit `duplicate key value violates unique constraint "_genie_migrations_pkey"`. Every `genie ls`, `genie task list`, `genie events` invocation failed identically — total CLI lockout until a manual `setval()`. Two-pronged fix: 1. **`rebalanceIdentitySequences()` in db-backup.ts**, called at the end of `restore()`. Walks every `attidentity` column in `public` via a single PL/pgSQL DO block and `setval(seq, GREATEST(MAX(col), 1), is_called)`. Idempotent: empty tables get `setval(1, false)` so the next nextval() yields 1 (canonical IDENTITY-start behavior); non-empty tables advance past MAX(col). Exported so operators on a pre-fix host can recover without re-restoring. Injectable `runner` seam keeps it unit-testable. 2. **Defensive setval in `runMigrations()`** (db-migrations.ts), guarded on `pending.length > 0` so the cost is paid only when actually about to INSERT. Self-heals any host that already drifted via a pre-fix restore, so upgrading to this version is enough — no manual SQL needed. 10 new tests pin the SQL shape, runner contract, error paths (non-zero exit, null status timeout, string-vs-Buffer stderr), and the restore-then-rebalance ordering in `restore()`.
…identity-sequences fix(db): rebalance IDENTITY sequences after restore + defensive setval
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR updates the genie package to version 4.260522.8 across all manifests, refines the session health flag condition to require absence of both linkage and anchor fields, adds database IDENTITY sequence rebalancing to compensate for pg_dump desynchronization, and ensures executor records properly set ChangesVersion and Manifest Updates
Session Health Flag and Executor Lifecycle
Database Recovery and Migrations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request includes version updates across several configuration files and introduces a mechanism to rebalance PostgreSQL identity sequences after a database restore to prevent primary key collisions. It also updates agent observability logic to consider claudeSessionId when determining session health and modifies the executor registry to record ended_at timestamps for terminal states. Feedback was provided regarding the use of ESM-compatible directory variables in tests and improving the robustness of SQL identifier quoting in the rebalance logic.
|
|
||
| describe('restore() rebalance wiring (source-shape lock)', () => { | ||
| test('restore() calls rebalanceIdentitySequences after the psql pipe', () => { | ||
| const source = readFileSync(join(__dirname, 'db-backup.ts'), 'utf-8'); |
There was a problem hiding this comment.
__dirname is not available in ES modules, which this project uses (as indicated by "type": "module" in package.json). Using it will cause a runtime error in a pure ESM environment. Use import.meta.dir instead, which is the idiomatic equivalent in Bun and ESM.
| const source = readFileSync(join(__dirname, 'db-backup.ts'), 'utf-8'); | |
| const source = readFileSync(join(import.meta.dir, 'db-backup.ts'), 'utf-8'); |
| 'SELECT setval(pg_get_serial_sequence(%L, %L), GREATEST(COALESCE((SELECT MAX(%I) FROM %I.%I), 0), 1), COALESCE((SELECT MAX(%I) FROM %I.%I), 0) > 0)', | ||
| r.sch || '.' || r.tbl, r.col, | ||
| r.col, r.sch, r.tbl, | ||
| r.col, r.sch, r.tbl | ||
| ); |
There was a problem hiding this comment.
The current concatenation r.sch || '.' || r.tbl is not robust against schema or table names that contain special characters or dots (e.g., a schema named my.schema). While unlikely in the current database structure, it is best practice to use quote_ident when constructing qualified names for functions like pg_get_serial_sequence to ensure they are correctly parsed by PostgreSQL.
| 'SELECT setval(pg_get_serial_sequence(%L, %L), GREATEST(COALESCE((SELECT MAX(%I) FROM %I.%I), 0), 1), COALESCE((SELECT MAX(%I) FROM %I.%I), 0) > 0)', | |
| r.sch || '.' || r.tbl, r.col, | |
| r.col, r.sch, r.tbl, | |
| r.col, r.sch, r.tbl | |
| ); | |
| EXECUTE format( | |
| 'SELECT setval(pg_get_serial_sequence(quote_ident(%L) || ''.'' || quote_ident(%L), %L), GREATEST(COALESCE((SELECT MAX(%I) FROM %I.%I), 0), 1), COALESCE((SELECT MAX(%I) FROM %I.%I), 0) > 0)', | |
| r.sch, r.tbl, r.col, | |
| r.col, r.sch, r.tbl, | |
| r.col, r.sch, r.tbl | |
| ); |
Promotes dev to stable after fixing Genie observability/session-linkage false degraded states.\n\nKey fix:\n- align observe health with actual resume anchors by not flagging missing_session when executors.claude_session_id exists\n- stamp ended_at for terminal executors created in terminal initial states\n- add regression coverage\n\nEvidence:\n- local pre-push checks passed\n- CI success on dev commit 38d1866\n- dev release v4.260522.8 published successfully\n\nThis should allow stable channel to advance beyond stale installer behavior once merged/released.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores