Skip to content

fix(connections): remote database file copy correctness and cleanup - #2842

Merged
datlechin merged 1 commit into
mainfrom
fix/remote-file-copy-correctness
Sep 14, 2026
Merged

datlechin merged 1 commit into
mainfrom
fix/remote-file-copy-correctness

Conversation

@datlechin

Copy link
Copy Markdown
Member

Five correctness and data-safety defects in the read-only "Remote Database File" copy path (shipped in #2553, v0.69.0). Found while investigating #2831; each was reproduced before fixing. Independent of the live-session feature, and independent of each other.

Fixes

1. Stale -wal replayed over a fresh copy. DatabaseFileLayout.staleAfterReplaceSuffixes documents the sidecars that must be cleared after the main file is replaced, but nothing called it. After a force-quit leaves an app.db-wal beside a WAL-mode copy, the next fetch renames a fresh main file into place and SQLite replays the stale log against bytes it no longer matches. RemoteDatabaseFileTransfer.clearStaleSidecars now runs after the replace: a snapshot (fully checkpointed) clears every stale sidecar, a direct copy keeps the ones it just fetched and clears the rest.

2. Copy reused after a WAL commit that didn't grow the log. RemoteFileFingerprint compared main size/mtime and WAL size. On Linux with the default journal_size_limit = -1, a commit plus checkpoint can leave all three unchanged and move only the log's mtime, so a busy database read as unchanged and the stale copy was reused. The fingerprint (and the manifest) now record writeAheadLogModified and compare it. Old manifests without the field decode as nil and refetch once to gain a full baseline.

3. Killed VACUUM INTO reported as success. libssh2_channel_get_exit_status returns the stored 0 for a process killed by a signal, so an OOM-killed or interrupted snapshot command looked successful and its half-written snapshot was adopted. RemoteCommandResult now reads the exit signal via libssh2_channel_get_exit_signal, and succeeded requires both a zero status and no signal.

4. Interrupted snapshot files left on the server. The VACUUM INTO snapshot is removed by a defer that cannot run when the session dies mid-fetch, so a dropped connection left a full copy of the database behind. The snapshot command now sweeps prior *.tablepro-snapshot-* leftovers before it writes a new one.

5. Local working copies never deleted. RemoteDatabaseFileStore.discard(_:) had no caller, and the abandonedCopies() a comment referenced did not exist, so a deleted connection or a changed path left multi-GB copies forever. pruneAbandoned now runs at launch and removes copies unused for 30 days; a reuse touches the copy so a daily-used database stays fresh. A remote-file copy is read-only and re-fetched on next open, so a swept copy costs one download and never loses data.

Security

World-readable snapshot. VACUUM INTO created the snapshot with the server umask (0644 under umask 022) beside a 0600 database, exposing a full copy to every account on the host. The snapshot command now runs under umask 077, so it is 0600 from creation.

Tests

RemoteDatabaseFileCorrectnessTests: the WAL-mtime fingerprint change and the identical case, a manifest missing the new field decoding as nil, clearStaleSidecars for both plans, succeeded false on a signal, and pruneAbandoned removing only a stale copy. The existing RemoteDatabaseFileTests still pass.

Verification

  • build (app + bundled plugins): PASS
  • test (RemoteDatabaseFileCorrectnessTests, RemoteDatabaseFileTests): PASS, 25 cases
  • lint: clean

The umask/sweep behaviour on the server was validated in Docker during the #2831 investigation; the fingerprint WAL-mtime miss was reproduced on Ubuntu 24.04.

@datlechin
datlechin merged commit 6284b5c into main Sep 14, 2026
8 checks passed
@datlechin
datlechin deleted the fix/remote-file-copy-correctness branch September 14, 2026 12:31
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.

1 participant