Repository navigation
refactor(sqlite): reduce repeated schema validation work - #139366
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 5, 2026, 5:18 PM ET / 21:18 UTC. ClawSweeper reviewWhat this changesReuse the existing database statement cache for three schema queries, reducing repeated preparation while preserving schema validation results. Warning SQLite table change This PR modifies persisted SQLite tables in Merge readiness⛔ Blocked before merge - 2 items remain The optimization remains useful and has no identified blocking defect. The previous migration blocker is resolved: this diff changes neither persisted representations nor migration policy, and the clarification supplies relevant upgrade validation. Priority: P2 Review scores
Verification
How this fits togetherSQLite schema validation checks database structure before OpenClaw exposes stored state. These reads now share the bounded statement cache used by synchronous database queries, while lifecycle owners retain cleanup responsibility. flowchart TD
A[Database open or validation] --> B[Schema contract reader]
B --> C{Statement caching enabled and allowed?}
C -->|Yes| D[Existing bounded statement cache]
C -->|No| E[Fresh prepared statement]
D --> F[Current SQLite schema rows]
E --> F
F --> G[Validation result]
H[Close or invalidation] --> D
Before merge
Agent review detailsSecurityNone. PR surfaceSource +14, Tests +97, Other +1. Total +112 across 6 files. View PR surface stats
Review metrics
Stored data modelPersistent data-model change detected: Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep schema reads within the existing bounded execution owner, preserving fresh results and lifecycle invalidation without adding a second cache or migration. Do we have a high-confidence way to reproduce the issue? Not applicable as a correctness bug: current main visibly repeats statement preparation, and the supplied benchmark measures that overhead; this read-only review did not execute benchmarks or tests. Is this the best way to solve the issue? Yes. Reusing the established cache avoids competing ownership, and the unchanged SELECTs and validation policy do not require a migration. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 523d4c1840a7. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Data-model review clarificationThe schema-change warning in the automated review does not describe this diff. No table/index/trigger definitions, schema versions, migrations, or persisted values change. The three SQL SELECT texts and bindings are unchanged; only their existing prepared-statement execution owner is reused. The canonical schema and migration files are untouched. No new migration is needed. The requested upgrade compatibility evidence is also present in the current 370-pass/one-skip run: all three existing cases passed in openclaw-state-db.test.ts: v15 Workshop ownership migration preserves rows; a v15 database without Workshop tables upgrades to v16; and a v15 database missing a stable table remains rejected by runtime open and Doctor repair. These exercise existing migrations, not migrations introduced by this PR. The separately reported actual compiled v16 fresh-open/reopen/read-only/preflight proof also passed. This addresses the review's compatibility-proof request; additional schema changes would be unrelated and are intentionally not added. Exact-head CI 33990278224 passed. |
|
Merged via squash.
|
|
The 21:18 review refresh acknowledges that the migration blocker is resolved and that the diff changes neither persisted representations nor migration policy. Its remaining performance warning matches limitations already stated in the PR: historical schema-v15 timings do not establish schema-v16 production tails or memory improvement. Neither claim is made. The maintainer decision is to accept the bounded reuse refactor with unchanged queries and cache/lifecycle policy, supported by the scoped timings, all retained adverse observations, current upgrade/refusal tests, compiled-owner proof and green exact-head CI. No second cache, migration, widened benchmark claim, or additional database change is introduced. The refreshed review identifies no correctness finding. |
What Problem This Solves
Repeated schema validation adds avoidable overhead to state-database operations that already hold an open connection.
Why This Change Was Made
Reuse the existing per-database statement cache for three fixed schema reads, with its implementation in the shared SQLite execution owner. SQL, bindings, validation results, cache limits/admission, active-statement isolation, authorizer/deserialization invalidation, and close behavior remain unchanged.
Production is +14 lines (208 added/194 removed, largely relocation); the growth connects schema reads to the existing synchronous statement-lifetime boundary. Tests are +97, the guard registry +1, and the assertion baseline records two removed assertions. No second cache is introduced.
User Impact
Less work for repeated schema checks on cache-enabled handles. There are no schema/version, persisted-data, configuration, or public API changes. Cache-disabled reads preserve their behavior. No Gateway-turn or universal tail/memory improvement is claimed.
Evidence
Current schema-v16 composition, base
5702b801: normal frozen install; all six owner/sibling test files passed 370 cases plus one unchanged Linux-only skip; all 34 normal changed-gate stages passed; one mapped normal build passed. The actual emitted owner passed fresh-open/reopen schema-v16 metadata checks, complete schema validation, read-only preflight and cleanup. Preflight preserved the closed source family's bytes/metadata and removed its snapshots; all native/adapter processes closed. Subsequent normal reopen changed ctime only, with bytes/mtime unchanged. Independent P0–P2 review completed all 11 partitions with no actionable findings.Historical performance cohort, schema v15 at
db2348: fixed B0/C0/C1/B1 order, 256 batch means, with complete results checked throughout. These measurements have not been relabeled as v16 results or rerun for a better outcome. Percentages compare candidate with baseline; negative means less time.The pressure rows are synthetic controls, not an observed production query mix. All 25 adverse entries out of 110 comparisons are retained below. Notable opposing results include read-only forward batch p95 +48.10%, reverse pressure-row p95 +24.08% / +7.67%, and sampled family RSS +0.21% / +1.20% despite lower kernel-child peaks. Four timing hosts do not establish fleet behavior; p95 here describes 16-cycle batch means, not individual-call latency.
All 25 adverse observations
With 16 samples, nearest-rank p95 equals the maximum; duplicate p95/maximum entries are not independent evidence. Lifecycle observations are single-host measurements. Forward compares B0/C0; reverse compares B1/C1 from the reverse execution order.
The Linux initialization-failure descriptor assertion remains skipped on macOS. Existing Vite import-analysis and build EVAL/source-map/browser-externalization advisories remain recorded; no warning suppression, timeout change, or test bypass was added.
Data-model review clarification
The schema-change warning in the automated review does not describe this diff. No table/index/trigger definitions, schema versions, migrations, or persisted values change. The three SQL SELECT texts and bindings are unchanged; only their existing prepared-statement execution owner is reused. The canonical schema and migration files are untouched. No new migration is needed.
The requested upgrade compatibility evidence is also present in the current 370-pass/one-skip run: all three existing cases passed in openclaw-state-db.test.ts: v15 Workshop ownership migration preserves rows; a v15 database without Workshop tables upgrades to v16; and a v15 database missing a stable table remains rejected by runtime open and Doctor repair. These exercise existing migrations, not migrations introduced by this PR. The separately reported actual compiled v16 fresh-open/reopen/read-only/preflight proof also passed.
This addresses the review's compatibility-proof request; additional schema changes would be unrelated and are intentionally not added. Exact-head CI 33990278224 passed.