Session sync performance optimizations#63
Conversation
- Add notify() helper: emoji + toast + log in one call - startupSync: 🔄 Syncing..., ✅ ready, ❌ error - pull: 📥 Pulling..., 📥 result - push: 📤 Pushing..., 📤 result - link: 🔗 Linking..., 🔗 result - init: 🚀 Sync configured
- session-db.ts: read/write sessions from SQLite and JSON dir - session-merge.ts: syncSessions() — union-merge per record (messages, parts, todos, session_messages, shares by id+time_updated) - service.ts: integrate syncSessions in pull, push, runStartup - @types/node: ^22.15.0 for node:sqlite types
- Config field includeProjects (bool, default false) - resolveProjectsFilePath(): Windows → %APPDATA%\ai.opencode.desktop\, Linux/macOS → /opencode - SyncItem with preserveWhenMissing: true for graceful handling
- shell.ts: createNodeShell() wraps child_process.execSync with Bun-compatible tagged-template API - service.ts: detect ctx.$ === undefined at createSyncService entry, replace with createNodeShell() - Enables gh/git commands in OpenCode Desktop (Electron/Node.js)
…ned/objects - All insert* functions now pass values through v() = asSQLValue() - asSQLValue() now also serializes objects/arrays via JSON.stringify - Fixes 'Provided value cannot be bound to SQLite parameter' on startup
projects-merge.ts: syncGlobalData() — union-merge проектов * server.projects.local — объединение по worktree * layout.page.lastProjectSession — объединение по директории, новее at * остальные ключи — если нет в локальном, берётся с удалённого service.ts: вызов syncGlobalData в pull/push/startup paths.ts: удалён старый SyncItem для простого копирования
При пустой директории remoteSessions (первый запуск, нет удалённых сессий) syncSessions пишет только JSON-файлы, не перезаписывая локальную БД. Это избегает ошибки 'Provided value cannot be bound to SQLite parameter 1', которая возникала из-за NOT NULL constraint в таблицах session/message/part при DELETE+INSERT
- Убирает deleteSession + каскадное удаление - Все insert* функции заменены на upsert* (INSERT OR REPLACE) - Избегает NOT NULL constraint violations при перезаписи сессий - Предотвращает ошибку 'Provided value cannot be bound to SQLite parameter 1'
- Убрано копирование opencode.db и storage/* (SESSION_DIRS) - Сессии синхронизируются только через per-session JSON merge (syncSessions) - Предотвращает затирание локальных сессий при pull
There was a problem hiding this comment.
Code Review
This pull request refactors the synchronization service by replacing legacy bash scripts with a more integrated startup sync process that handles auto-committing, background pushing, and merging of session and project data. It also introduces a new SQLite-based session database and a Node-based shell wrapper. Feedback highlights a synchronization logic flaw where the 'shouldFetch' optimization might miss remote updates from other devices. Additionally, reviewers pointed out a race condition in the background push state updates and performance issues in the database layer, specifically N+1 queries and inefficient SQL statement preparation.
| }); | ||
| await showToast(ctx.client, 'Config updated. Restart opencode to apply.', 'info'); | ||
| return; | ||
| const shouldFetch = !head || head !== state.lastHead || dirty; |
There was a problem hiding this comment.
The 'shouldFetch' optimization breaks multi-device synchronization. It assumes that if the local repository is clean and the local HEAD matches the cached 'state.lastHead', no remote changes exist. However, another device could have pushed changes to the remote repository in the meantime. Without a 'fetch' (or at least an 'ls-remote' check), this client will not detect those changes on startup. Correctness should be prioritized over skipping a network request in a sync service. Consider using 'git ls-remote' if you want a faster check than a full fetch.
| pushBranch(ctx.$, repoRoot, branch).catch((err) => | ||
| log.warn('Background push failed', { error: formatError(err) }) | ||
| ); | ||
| await updateState(locations, { | ||
| lastPush: new Date().toISOString(), | ||
| lastHead: commitHead ?? undefined, | ||
| }); | ||
| log.info('Auto-committed and pushed pending changes'); | ||
| await showToast(ctx.client, 'Pending changes committed and pushed', 'info'); |
There was a problem hiding this comment.
Updating the sync state ('lastPush', 'lastHead') and notifying the user immediately after starting a background push is unsafe. If the push fails (e.g., due to network issues or remote rejection), the local state will incorrectly reflect a successful synchronization. This, combined with the 'shouldFetch' optimization, will cause the client to skip future sync attempts for these changes. The state update and success notification should be moved inside the completion handler of the push operation.
pushBranch(ctx.$, repoRoot, branch)
.then(async () => {
await updateState(locations, {
lastPush: new Date().toISOString(),
lastHead: commitHead ?? undefined,
});
log.info('Auto-committed and pushed pending changes');
await showToast(ctx.client, 'Pending changes committed and pushed', 'info');
})
.catch((err) =>
log.warn('Background push failed', { error: formatError(err) })
);| function readMessages(db: DatabaseSync, sessionId: string): Message[] { | ||
| const rows = db | ||
| .prepare(`SELECT ${MESSAGE_COLUMNS.join(', ')} FROM message WHERE session_id = ?`) | ||
| .all(sessionId) as Record<string, unknown>[]; | ||
| return rows.map((row) => { | ||
| const msg = row as unknown as Message; | ||
| const partRows = db | ||
| .prepare(`SELECT ${PART_COLUMNS.join(', ')} FROM part WHERE message_id = ?`) | ||
| .all(msg.id) as Record<string, unknown>[]; | ||
| msg.parts = partRows.map((pr) => pr as unknown as Part); | ||
| return msg; | ||
| }); | ||
| } |
There was a problem hiding this comment.
This function has an N+1 query problem and redundant statement preparation. It prepares and executes a query for the 'part' table for every single message in the session. It is significantly more efficient to fetch all parts for the session in one query and group them by 'message_id' in memory.
function readMessages(db: DatabaseSync, sessionId: string): Message[] {
const messages = db
.prepare('SELECT ' + MESSAGE_COLUMNS.join(', ') + ' FROM message WHERE session_id = ?')
.all(sessionId) as unknown as Message[];
const allParts = db
.prepare('SELECT ' + PART_COLUMNS.join(', ') + ' FROM part WHERE session_id = ?')
.all(sessionId) as unknown as Part[];
const partsByMessage = new Map<string, Part[]>();
for (const part of allParts) {
const list = partsByMessage.get(part.message_id) ?? [];
list.push(part);
partsByMessage.set(part.message_id, list);
}
for (const msg of messages) {
msg.parts = partsByMessage.get(msg.id) ?? [];
}
return messages;
}| db.prepare( | ||
| `INSERT OR REPLACE INTO session (${SESSION_COLUMNS.join(', ')}) VALUES (${SESSION_COLUMNS.map(() => '?').join(',')})` | ||
| ).run(...values); |
🤖 Review Jules RelayI found 1 Gemini suggestion so far. Type |
Summary
Five independent optimizations for the SQLite session synchronization, each verified with tests.
Optimizations
Compact JSON — \JSON.stringify(session)\ without
ull, 2. Saves ~30% file size on session JSON files.
Single WAL checkpoint — checkpoint moved out of \openDB()\ into \syncSessions(), run once per cycle instead of on every DB open.
Single DB handle — \syncSessions()\ opens SQLite once, uses handle-variant functions (\listSessionIdsFromHandle,
eadSessionFromHandle, \writeSessionsToHandle) for all read/write operations within one cycle.
Batch DB writes — all merged sessions written in a single \BEGIN…COMMIT\ transaction instead of one INSERT per session.
Background git push — \pushBranch(...).catch(...)\ doesn't block startup. HEAD cache (\getHeadHash()\ + \state.lastHead) skips unnecessary remote fetches when HEAD hasn't changed.
Impact
Metrics from local testing (Windows, ~200 sessions):