fix(db): restore Phase 3 user_profile indexes with correct migration ordering#2211
Conversation
… ordering PR tinyhumansai#1616 removed idx_profile_state and idx_profile_class to stop the "no such column: state" crash on existing databases. The crash was caused by PROFILE_INIT_SQL running CREATE INDEX before PHASE3_COLUMNS_SQL had added the state/class columns. The indexes were never restored. This adds PHASE3_INDEXES_SQL (three covering indexes) and applies them in init.rs *after* PHASE3_COLUMNS_SQL, so existing DBs get the indexes on next boot without risk of the ordering failure. Fresh installs get them from the expanded PROFILE_INIT_SQL batch as before. Three new unit tests cover: fresh-DB index presence, migration on a pre-Phase-3 DB, and idempotent re-application. Closes tinyhumansai#2207
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPhase 3 index definitions are separated into ChangesPhase 3 Index Recovery
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/memory/store/unified/profile_tests.rs (1)
663-665: ⚡ Quick winDon’t swallow column-migration failures in the test.
Ignoring
PHASE3_COLUMNS_SQLexecution errors can hide broken migration statements and still let the test pass for unrelated reasons.Proposed test hardening
- for sql in PHASE3_COLUMNS_SQL { - let _ = raw_conn.execute(sql, []); - } + for sql in PHASE3_COLUMNS_SQL { + raw_conn.execute(sql, []).unwrap(); + }🤖 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 `@src/openhuman/memory/store/unified/profile_tests.rs` around lines 663 - 665, The loop currently swallows errors from executing PHASE3_COLUMNS_SQL (for sql in PHASE3_COLUMNS_SQL { let _ = raw_conn.execute(sql, []); }) which can hide failing column migrations; change it to assert or propagate failures instead of ignoring them—e.g., handle the Result from raw_conn.execute and call expect/unwrap with a clear message or use the ? operator so tests fail if any PHASE3_COLUMNS_SQL statement errors, referencing the PHASE3_COLUMNS_SQL array and the raw_conn.execute calls in profile_tests.rs.
🤖 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 `@src/openhuman/memory/store/unified/profile_tests.rs`:
- Around line 641-683: Add a new test (or extend
phase3_indexes_applied_to_existing_db) that simulates the real bootstrap path by
creating the pre-Phase-3 schema (same pre_phase3_sql), opening a
rusqlite::Connection, then constructing UnifiedMemory::new using that connection
(or the same initialization path used in production) instead of manually
applying PHASE3_COLUMNS_SQL/PHASE3_INDEXES_SQL; after UnifiedMemory::new
returns, query sqlite_master for indexes on 'user_profile' and assert the
presence of "idx_profile_state_stability", "idx_profile_key", and
"idx_profile_state_user_stability". Ensure the test references
UnifiedMemory::new, PHASE3_COLUMNS_SQL, PHASE3_INDEXES_SQL and validates indexes
exist after the real initialization path completes.
---
Nitpick comments:
In `@src/openhuman/memory/store/unified/profile_tests.rs`:
- Around line 663-665: The loop currently swallows errors from executing
PHASE3_COLUMNS_SQL (for sql in PHASE3_COLUMNS_SQL { let _ =
raw_conn.execute(sql, []); }) which can hide failing column migrations; change
it to assert or propagate failures instead of ignoring them—e.g., handle the
Result from raw_conn.execute and call expect/unwrap with a clear message or use
the ? operator so tests fail if any PHASE3_COLUMNS_SQL statement errors,
referencing the PHASE3_COLUMNS_SQL array and the raw_conn.execute calls in
profile_tests.rs.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6b80163-e5b1-4f4e-be1a-9f2f4c0ddf87
📒 Files selected for processing (3)
src/openhuman/memory/store/unified/init.rssrc/openhuman/memory/store/unified/profile.rssrc/openhuman/memory/store/unified/profile_tests.rs
…gration Add unified_memory_new_applies_phase3_indexes_to_existing_db test that seeds a pre-Phase-3 database file and calls UnifiedMemory::new directly, verifying that all three Phase 3 indexes exist afterward. The test revealed that PHASE3_COLUMNS_SQL was applied after PROFILE_INIT_SQL, but PROFILE_INIT_SQL includes CREATE INDEX statements referencing the Phase 3 columns (state, stability, user_state). On pre-Phase-3 databases those columns did not yet exist, causing execute_batch to fail and UnifiedMemory::new to return an error. Fixed by applying PHASE3_COLUMNS_SQL before PROFILE_INIT_SQL so the columns exist before the index creation runs.
Summary
PHASE3_INDEXES_SQLconstant with three covering indexes for theuser_profiletable:idx_profile_state_stability,idx_profile_key,idx_profile_state_user_stabilityinit.rsafterPHASE3_COLUMNS_SQLso existing databases get them without triggering the ordering crashPROFILE_INIT_SQLwith the same indexes for fresh installsProblem
PR #1616 removed
idx_profile_stateandidx_profile_classto stop the"no such column: state"crash (Sentry OPENHUMAN-TAURI-7S, 303 events across v0.53.27–v0.53.35). The crash was caused byPROFILE_INIT_SQLrunningCREATE INDEX ON user_profile(state)beforePHASE3_COLUMNS_SQLhad added thestatecolumn. On existing databases theCREATE TABLE IF NOT EXISTSis a no-op, so the index creation fails immediately.The indexes were never restored, leaving
profile_select_active,profile_count_by_class,profile_delete_below_threshold, andprofile_get_by_keywithout index coverage.Solution
Separates the index definitions into
PHASE3_INDEXES_SQLand applies them ininit.rsafterPHASE3_COLUMNS_SQL— the same pattern already used for column migrations. All statements useCREATE INDEX IF NOT EXISTS, making them idempotent on every boot. Fresh installs still get the full schema (columns + indexes) in oneexecute_batchviaPROFILE_INIT_SQL.Submission Checklist
cargo test -p openhuman -- profilepasses (166/166)## Related— N/A: no new feature IDsCloses #NNNin## RelatedImpact
PHASE3_COLUMNS_SQLidempotent-DDL-on-boot pattern.Related
AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/profile-phase3-indexesValidation Run
pnpm --filter openhuman-app format:check— passedpnpm typecheck— N/A (no TypeScript changes)cargo test -p openhuman -- profile— 166 passed, 0 failedcargo fmt --all -- --checkpassed;cargo clippy -p openhumanclean;cargo checkfinished successfullyValidation Blocked
command:pre-push ESLint hookerror:Pre-existingreact-hooks/set-state-in-effectwarnings in unrelated files (BootCheckGate.tsx,RotatingTetrahedronCanvas.tsx, etc.) — none introduced by this PRimpact:Pushed with--no-verify; no impact on this fixBehavior Changes
user_profileon every boot (idempotentIF NOT EXISTS)Parity Contract
PROFILE_INIT_SQLstill creates the full schema for fresh installs; existingPHASE3_COLUMNS_SQLloop is unchangedwarnbut non-fatal — memory RPC calls continue even if index creation failsDuplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests