Skip to content

DB File Lock#1050

Closed
grfwings wants to merge 22 commits into
UsefulSoftwareCo:mainfrom
grfwings:daemon-child-timeout
Closed

DB File Lock#1050
grfwings wants to merge 22 commits into
UsefulSoftwareCo:mainfrom
grfwings:daemon-child-timeout

Conversation

@grfwings

@grfwings grfwings commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

This PR has three semi-related fixes.

Daemon timeout and desktop attach

A daemon could get stuck in a blocked startup state, preventing other daemons from starting until it was manually killed.

Changes:

  • Daemons use read-side reclamatino
  • Daemons now report their PID to the parent so they can be killed when needed.
  • A second desktop instance now attaches to an already-running local server instead of failing.

V1 to V2 migration hardening

I found the daemon issue while investigating a migration failure where multiple v1 scopes had the same integration slug. That semantic collision is not fixed in this PR. The correct migration behavior needs a product decision, especially with tenant changes coming.

This PR does harden the migration so failures are safer and recoverable:

  • Adds a migration journal: data.db.v1-v2-migration.json.
  • Builds the v2 database in staging, then flips it into place only after verification. The flip is one atomic rename, with crashes on either side recovered from the journal. The canonical database file is never in a partial state.
  • Adds recovery for crashes during migration.
  • Adds durability and integrity checks: PRAGMA wal_checkpoint(TRUNCATE), PRAGMA integrity_check, row-count verification, and fsyncs around critical writes/renames.
  • Moves migrated secret writes into insertPlan, before the migration ledger stamp and commit, with auth/keychain backups tracked by the journal. Secrets are cross-store, so they're written idempotently and the journal backs them up for recovery, since they can't participate in the SQL transaction.

Local DB ownership

The previous coordination model used server-control/startup.lock and server-control/server.json. That serialized startup for cooperating entrypoints, but it was not a lifetime database ownership lock. It was fragile around the v1->v2 migration because two processes could both open data.db, observe v1, and attempt file-level migration work.

This PR replaces that startup lock with a single local DB ownership gate:

  • Adds openOwnedLocalDatabase().
  • Adds a dedicated SQLite lock database at data.db.owner-lock.
  • Acquires BEGIN EXCLUSIVE on that lock DB and holds it for the lifetime of the local DB handle.
  • Keeps server.json as an attach/discovery hint only, not an ownership proof.

Acquires BEGIN EXCLUSIVE on the lock DB and holds it for the lifetime of the owning process. The lock is backed by libsql's file locking on the lock DB, so it is released automatically when the owner exits, including abnormal exit (SIGKILL, crash, power loss). This is the reason a separate lock DB is used rather than a PID lockfile: there is no stale-lock state to reclaim, because a dead process holds no lock. (verified: B blocks while A holds the lock, B acquires immediately after A is SIGKILLed).

This means you can guarantee single-writer ownership as long as all database connections are routed through openOwnedLocalDatabase()

@grfwings grfwings marked this pull request as draft June 18, 2026 22:44
@grfwings grfwings changed the title Daemon child timeout DB File Lock Jun 18, 2026
@greptile-apps

greptile-apps Bot commented Jun 18, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces an SQLite-based exclusive lock (data.db.owner-lock) so that only one process may own the local server's data directory at a time, replacing the previous file-based start lock. It also adds a crash-safe journal-driven v1→v2 migration with atomic renames, and teaches the CLI daemon to attach to an already-running server when the lock is already held.

  • Ownership lock (data-dir-ownership.ts, owned-database.ts): Opens a separate lock DB with journal_mode=DELETE and holds BEGIN EXCLUSIVE for the process lifetime; the kernel releases it automatically on any exit, including SIGKILL.
  • Journaled migration (v1-v2-migration.ts): Stages the v2 build at data.db.building-<nonce>, fsync-hardens each phase, and leaves a recovery journal so a successor process can complete or roll back a partial migration.
  • Attach path (cli/main.ts, sidecar.ts): When the lock is already held, the daemon emits EXECUTOR_ATTACHED and exits 0 — but the desktop sidecar has no handler for this sentinel and will reject with "exited before ready".

Confidence Score: 3/5

Not safe to merge as-is: the desktop sidecar's missing EXECUTOR_ATTACHED handler will cause the attach-to-already-running-server path to always fail with 'exited before ready' in the desktop app.

The core ownership-lock and journaled-migration machinery is well-designed and handles crash recovery correctly. However, a clear functional regression exists: the CLI daemon emits EXECUTOR_ATTACHED when it attaches to an existing server, but sidecar.ts only parses lines prefixed with 'EXECUTOR_READY:' — the EXECUTOR_ATTACHED sentinel is silently discarded, the sidecar sees a clean process exit before it resolved its ready-promise, and rejects. This makes the entire multi-instance attach feature broken end-to-end for desktop users, which is the primary stated goal of the PR.

apps/desktop/src/main/sidecar.ts — missing handler for the EXECUTOR_ATTACHED sentinel

Important Files Changed

Filename Overview
apps/local/src/db/data-dir-ownership.ts New file: implements acquireDataDirOwnership via BEGIN EXCLUSIVE on a separate SQLite lock DB (data.db.owner-lock). Lock is released by the OS on abnormal exit; findDataDirOwnershipHeld uses a WeakSet for cycle-safe error-chain traversal.
apps/local/src/db/owned-database.ts New file: composes ownership lock, crash-resumable v1→v2 migration, and serving DB handle. Releases the ownership lock on both normal and error paths including when Bun.serve fails after handler boot.
apps/local/src/db/v1-v2-migration.ts Major rework: adds journal-based staged migration with fsync-hardened atomic rename, crash recovery for all journal phases, and PRAGMA wal_checkpoint(TRUNCATE) + integrity_check before the flip.
apps/desktop/src/main/sidecar.ts Removes startup lock; switches sentinel parsing to line-buffered EXECUTOR_READY: only. Missing handler for the new EXECUTOR_ATTACHED sentinel — sidecar incorrectly rejects instead of attaching when a second desktop instance starts.
apps/cli/src/main.ts Replaces acquireLocalServerStartLock with startServerOrAttachOwnedDataDir; adds waitForDaemonStartupTarget; emits EXECUTOR_ATTACHED sentinel in daemon session — but the sidecar does not parse this sentinel.
apps/cli/src/daemon.ts Returns SpawnedDetachedProcess with PID from spawnDetached; adds terminateSpawnedDetachedProcess which sends SIGTERM to the process group on POSIX (falling back to the single PID) and to the PID on Windows.
apps/local/src/executor.ts Replaces separate migration + createSqliteFumaDb with openOwnedLocalDatabase. Adds lifecycle chain (sharedHandleLifecycle) to serialize disposeExecutor/getExecutor calls and self-heal the shared handle on failed creation.
apps/local/src/serve.ts Adds disposeOwnedResources to release DB ownership when Bun.serve fails after handlers are created, preventing the ownership lock from leaking on port-conflict or other startup failures.
apps/local/src/main.ts Adds disposeExecutor() to closeServerHandlers and as a belt-and-suspenders call in disposeServerHandlers; partial handler boot now cleans up correctly.
apps/cli/src/local-server-manifest.ts Removes the startup.lock PID-lockfile mechanism and LocalServerStartLock; adds a clarifying comment that server.json is now a discovery hint only, not an ownership proof.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Desktop as Desktop Sidecar
    participant Daemon as CLI Daemon
    participant Lock as data.db.owner-lock
    participant DB as data.db (SQLite)

    Desktop->>Daemon: "spawn(EXECUTOR_CLIENT=desktop)"
    Daemon->>Lock: "BEGIN EXCLUSIVE (busy_timeout=0)"
    alt Lock acquired
        Lock-->>Daemon: OK
        Daemon->>DB: migrateLocalV1ToV2IfNeeded()
        DB-->>Daemon: migration complete
        Daemon->>DB: createSqliteFumaDb()
        Daemon-->>Desktop: "stdout EXECUTOR_READY:<port>"
        Desktop->>Desktop: resolve(port) ✓
    else Lock held (DataDirOwnershipHeld)
        Lock-->>Daemon: SQLITE_BUSY
        Daemon->>Daemon: attachToExistingServer()
        Daemon-->>Desktop: stdout EXECUTOR_ATTACHED
        Note over Desktop: No handler for EXECUTOR_ATTACHED, sidecar rejects exited before ready
        Desktop->>Desktop: reject(Sidecar exited before ready) ✗
    end

    Note over Daemon,Lock: On crash/SIGKILL: SQLite releases EXCLUSIVE lock automatically
    Desktop->>Daemon: SIGTERM (process group)
    Daemon->>Lock: connection closed, lock released
    Daemon->>DB: WAL checkpoint-truncate
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Desktop as Desktop Sidecar
    participant Daemon as CLI Daemon
    participant Lock as data.db.owner-lock
    participant DB as data.db (SQLite)

    Desktop->>Daemon: "spawn(EXECUTOR_CLIENT=desktop)"
    Daemon->>Lock: "BEGIN EXCLUSIVE (busy_timeout=0)"
    alt Lock acquired
        Lock-->>Daemon: OK
        Daemon->>DB: migrateLocalV1ToV2IfNeeded()
        DB-->>Daemon: migration complete
        Daemon->>DB: createSqliteFumaDb()
        Daemon-->>Desktop: "stdout EXECUTOR_READY:<port>"
        Desktop->>Desktop: resolve(port) ✓
    else Lock held (DataDirOwnershipHeld)
        Lock-->>Daemon: SQLITE_BUSY
        Daemon->>Daemon: attachToExistingServer()
        Daemon-->>Desktop: stdout EXECUTOR_ATTACHED
        Note over Desktop: No handler for EXECUTOR_ATTACHED, sidecar rejects exited before ready
        Desktop->>Desktop: reject(Sidecar exited before ready) ✗
    end

    Note over Daemon,Lock: On crash/SIGKILL: SQLite releases EXCLUSIVE lock automatically
    Desktop->>Daemon: SIGTERM (process group)
    Daemon->>Lock: connection closed, lock released
    Daemon->>DB: WAL checkpoint-truncate
Loading

Reviews (2): Last reviewed commit: "Fix loadSharedHandle being able to poiso..." | Re-trigger Greptile

Comment thread apps/local/src/db/v1-v2-migration.ts Outdated
Comment thread apps/local/src/db/data-dir-ownership.ts
Comment thread apps/cli/src/daemon.ts
@grfwings grfwings force-pushed the daemon-child-timeout branch from 0e41265 to 6d366e7 Compare June 23, 2026 20:16
@grfwings grfwings force-pushed the daemon-child-timeout branch from 9127e3f to 2f0b117 Compare June 23, 2026 23:35
…cannot reopen while a previous handle is still disposing
@grfwings grfwings marked this pull request as ready for review June 24, 2026 00:30
@grfwings grfwings force-pushed the daemon-child-timeout branch from d640c4a to aff8273 Compare June 24, 2026 00:34
RhysSullivan added a commit that referenced this pull request Jun 28, 2026
* Kill daemon child if parent times out

* Safer migration

* Make flip idempotent, remove reduntant db copies, fix auth.json mutating eraly, derive expected counts from plan arrays

* Fix various correctness issues

* Add sqlite lock db ownership primitive and test

* Use pathToFile not string concatenation for dir

* Add owned-database.ts entry point

* Implement new database ownership process in cli and desktop sidecar

* Delete startup lock and v1->v2 migration lock

* Remove double-probe from v1-v2 migration

* Add integration tests and update errors, comments

* Don't change scope -> tenant mappings

* Add polling to desktop sidecar and fix infinite loop in findDataDirOwnershipHeld

* Inculde keychainBackups in migration journal

* Add guard to JSON.parse in readMigrationJournal

* Add coverage for corrupt journals, increase timeout for SIGKILL recovery sweep

* readMigrationJournal now distinguishes missing, valid, and unreadable journals

* writeSidecarManifest should create server-control before writing server.json

* Desktop can't attach to CLI daemon because it might be foreground; revert to old behavior

* Add test for SIGKILL after journal write and after secret write

* Shared executor should wait behind a lifecycle hook so getExecutor() cannot reopen while a previous handle is still disposing

* Fix loadSharedHandle being able to poison future getExecutor() calls

---------

Co-authored-by: Griffin Evans <griffin@ibm.com>
Co-authored-by: Griffin <38366015+grfwings@users.noreply.github.com>
@RhysSullivan

Copy link
Copy Markdown
Collaborator

Landed. This was rebased onto current main and merged as a stack:

Both are merged, so closing this in favor of #1175. Thanks @grfwings for the original work and design here. Full credit retained on the merged commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants