diff --git a/USAGE.md b/USAGE.md index 4142950..83af356 100644 --- a/USAGE.md +++ b/USAGE.md @@ -61,8 +61,8 @@ instead: | Invalid arguments | No argument, more than one argument, an empty or whitespace-only argument, an argument starting with `-` (including `--help`), or a path the operating system cannot represent (for example one containing a NUL character). Shows the usage line. | | Folder not found | `` does not exist or is not a folder. | | No workspace here | `` exists but has no `.ykanban`. The **Initialize workspace** button creates one, with the columns To Do / In Progress / Done (named in the current interface language). | -| Workspace locked | Another YKanBan window already has this workspace open. Close that window first. | -| Cannot open workspace | The database was created by a newer YKanBan, is damaged or cannot be opened, or a file-system or permission error occurred (also when initializing fails). The English error details can be selected and copied. | +| Workspace locked | Another YKanBan window already has this workspace open. Close that window first. Also shown when another window takes the lock while you initialize. | +| Cannot open workspace | The database was created by a newer YKanBan, is damaged or cannot be opened, or a file-system or permission error occurred (also when initializing fails for a reason other than the lock). The English error details can be selected and copied. | The window title is ` - YKanBan` while a workspace is open and just `YKanBan` on these pages. diff --git a/YKanBan.Tests/Storage/SqliteDatabaseTests.cs b/YKanBan.Tests/Storage/SqliteDatabaseTests.cs index 1bf3be8..98868ef 100644 --- a/YKanBan.Tests/Storage/SqliteDatabaseTests.cs +++ b/YKanBan.Tests/Storage/SqliteDatabaseTests.cs @@ -80,4 +80,48 @@ public class SqliteDatabaseTests Assert.ThrowsExactly(() => SqliteDatabase.Open(databasePath, broken)); } + + [TestMethod] + public void CreateBuildsSchemaAndSeedTogether() + { + using var directory = new TempDirectory(); + string databasePath = Path.Combine(directory.FullPath, "create.db"); + + using SqliteConnection connection = SqliteDatabase.Create(databasePath, WorkspaceSchema.Migrations, + (seedConnection, transaction) => + { + using SqliteCommand command = seedConnection.CreateCommand(); + command.Transaction = transaction; + command.CommandText = "INSERT INTO tags (name, color) VALUES ('seed', '#000000');"; + command.ExecuteNonQuery(); + }); + Assert.AreEqual(WorkspaceSchema.CurrentVersion, SqliteDatabase.ReadUserVersion(connection)); + Assert.AreEqual(1L, SqliteTestHelper.ScalarLong(connection, "SELECT COUNT(*) FROM tags;")); + Assert.AreEqual("wal", SqliteTestHelper.ScalarString(connection, "PRAGMA journal_mode;")); + Assert.AreEqual(1L, SqliteTestHelper.ScalarLong(connection, "PRAGMA foreign_keys;")); + } + + [TestMethod] + public void CreateWithFailingSeedLeavesNoDatabaseBehind() + { + using var directory = new TempDirectory(); + string databasePath = Path.Combine(directory.FullPath, "failed.db"); + + Assert.ThrowsExactly(() => SqliteDatabase.Create( + databasePath, WorkspaceSchema.Migrations, (_, _) => throw new InvalidOperationException("seed failed"))); + Assert.IsFalse(File.Exists(databasePath)); + Assert.IsFalse(File.Exists(databasePath + "-wal")); + Assert.IsFalse(File.Exists(databasePath + "-shm")); + } + + [TestMethod] + public void CreateRefusesAnExistingDatabaseAndKeepsIt() + { + using var directory = new TempDirectory(); + string databasePath = Path.Combine(directory.FullPath, "existing.db"); + SqliteDatabase.Create(databasePath, WorkspaceSchema.Migrations).Dispose(); + + Assert.ThrowsExactly(() => SqliteDatabase.Create(databasePath, WorkspaceSchema.Migrations)); + Assert.IsTrue(File.Exists(databasePath)); + } } diff --git a/YKanBan.Tests/Storage/Workspace/WorkspacePresetTests.cs b/YKanBan.Tests/Storage/Workspace/WorkspacePresetTests.cs index 813ddc7..7e185dd 100644 --- a/YKanBan.Tests/Storage/Workspace/WorkspacePresetTests.cs +++ b/YKanBan.Tests/Storage/Workspace/WorkspacePresetTests.cs @@ -29,8 +29,7 @@ public class WorkspacePresetTests ]; using var directory = new TempDirectory(); - WorkspaceInitializer.Initialize(directory.FullPath); - WorkspacePreset.AddPresetColumns(directory.FullPath); + WorkspaceSession.Initialize(directory.FullPath).Dispose(); using SqliteConnection connection = SqliteTestHelper.OpenWorkspace(directory.FullPath); CollectionAssert.AreEqual(expected, SqliteTestHelper.GetColumnTitles(connection)); @@ -56,8 +55,7 @@ public class WorkspacePresetTests ]; using var directory = new TempDirectory(); - WorkspaceInitializer.Initialize(directory.FullPath); - WorkspacePreset.AddPresetColumns(directory.FullPath); + WorkspaceSession.Initialize(directory.FullPath).Dispose(); using SqliteConnection connection = SqliteTestHelper.OpenWorkspace(directory.FullPath); CollectionAssert.AreEqual(expected, SqliteTestHelper.GetColumnTitles(connection)); @@ -83,8 +81,7 @@ public class WorkspacePresetTests ]; using var directory = new TempDirectory(); - WorkspaceInitializer.Initialize(directory.FullPath); - WorkspacePreset.AddPresetColumns(directory.FullPath); + WorkspaceSession.Initialize(directory.FullPath).Dispose(); // Switching the language after adding the presets must not rewrite stored data. Resources.Culture = new CultureInfo("zh-Hans"); diff --git a/YKanBan.Tests/Storage/Workspace/WorkspaceSessionTests.cs b/YKanBan.Tests/Storage/Workspace/WorkspaceSessionTests.cs index 7bade97..8401c72 100644 --- a/YKanBan.Tests/Storage/Workspace/WorkspaceSessionTests.cs +++ b/YKanBan.Tests/Storage/Workspace/WorkspaceSessionTests.cs @@ -57,4 +57,36 @@ public class WorkspaceSessionTests Assert.AreEqual("bug", session.Repository.GetTags().Single().Name); Assert.AreEqual(tag.Id, session.Repository.GetTags().Single().Id); } + + [TestMethod] + public void InitializeAddsPresetColumnsAndHoldsTheLock() + { + using var directory = new TempDirectory(); + + using WorkspaceSession session = WorkspaceSession.Initialize(directory.FullPath); + Assert.AreEqual(3, session.Repository.GetColumns().Count); + Assert.ThrowsExactly(() => WorkspaceSession.Open(directory.FullPath)); + } + + [TestMethod] + public void InitializeWhileAnotherInstanceHoldsTheLockThrowsLockConflict() + { + using var directory = new TempDirectory(); + Directory.CreateDirectory(WorkspacePaths.Root(directory.FullPath)); + using WorkspaceLock held = WorkspaceLock.Acquire(directory.FullPath); + + Assert.ThrowsExactly(() => WorkspaceSession.Initialize(directory.FullPath)); + Assert.IsFalse(File.Exists(WorkspacePaths.Database(directory.FullPath))); + } + + [TestMethod] + public void FailedInitializeReleasesTheLock() + { + using var directory = new TempDirectory(); + WorkspaceInitializer.Initialize(directory.FullPath); + + // The database already exists, so initialization fails after taking the lock. + Assert.ThrowsExactly(() => WorkspaceSession.Initialize(directory.FullPath)); + using WorkspaceSession reopened = WorkspaceSession.Open(directory.FullPath); + } } diff --git a/YKanBan/Services/AppServices.cs b/YKanBan/Services/AppServices.cs index 37cb201..02cac8e 100644 --- a/YKanBan/Services/AppServices.cs +++ b/YKanBan/Services/AppServices.cs @@ -48,6 +48,20 @@ public sealed class AppServices : IDisposable return Session; } + /// + /// Initializes a workspace with its preset columns and opens it, holding + /// the lock from the creation of .ykanban onwards. + /// + /// The existing folder to initialize. + /// The open session. + /// The folder does not exist on disk. + /// Another instance already holds the lock. + public WorkspaceSession InitializeWorkspace(string folderPath) + { + Session = WorkspaceSession.Initialize(folderPath); + return Session; + } + /// /// Persists the configuration when this session changed a setting; called once at shutdown. /// diff --git a/YKanBan/Storage/SqliteDatabase.cs b/YKanBan/Storage/SqliteDatabase.cs index f9ae236..bc3df4e 100644 --- a/YKanBan/Storage/SqliteDatabase.cs +++ b/YKanBan/Storage/SqliteDatabase.cs @@ -33,16 +33,7 @@ public static class SqliteDatabase /// An open connection with all PRAGMAs applied and the schema migrated. public static SqliteConnection Open(string databasePath, IReadOnlyList migrations) { - var connection = new SqliteConnection(new SqliteConnectionStringBuilder - { - DataSource = databasePath, - Mode = SqliteOpenMode.ReadWriteCreate, - - // Each process holds its own single connection; a pool would only add noise. - Pooling = false, - }.ToString()); - - connection.Open(); + SqliteConnection connection = OpenConnection(databasePath); try { ApplyPragmas(connection); @@ -57,6 +48,74 @@ public static class SqliteDatabase } } + /// + /// Creates a new database file with the mandatory PRAGMAs, then applies + /// every migration and the optional seed in one transaction, so the file + /// ends up either fully built and seeded or still at version 0. + /// + /// Path of the SQLite database file; it must not exist yet. + /// Contiguous migration list ordered by version, starting at 1. + /// Initial content written in the same transaction, when any. + /// An open connection with all PRAGMAs applied and the schema migrated. + /// The migration list is not contiguous from version 1. + /// The database file already exists. + public static SqliteConnection Create( + string databasePath, + IReadOnlyList migrations, + Action? seed = null) + { + ValidateMigrations(migrations); + if (File.Exists(databasePath)) + { + throw new InvalidOperationException($"The database already exists: {databasePath}"); + } + + SqliteConnection connection = OpenConnection(databasePath); + try + { + ApplyPragmas(connection); + RegisterFunctions(connection); + + using var transaction = connection.BeginTransaction(); + foreach (SchemaMigration migration in migrations) + { + Execute(connection, migration.Sql, transaction); + Execute(connection, $"PRAGMA user_version={migration.Version};", transaction); + } + seed?.Invoke(connection, transaction); + transaction.Commit(); + return connection; + } + catch + { + connection.Dispose(); + + // Leave no empty version-0 file behind that a later open would migrate into an empty board. + DeleteDatabaseFilesBestEffort(databasePath); + throw; + } + } + + /// + /// Deletes a database file together with its WAL and shared-memory files, + /// ignoring failures. + /// + /// Path of the SQLite database file. + private static void DeleteDatabaseFilesBestEffort(string databasePath) + { + foreach (string path in new[] { databasePath, databasePath + "-wal", databasePath + "-shm", databasePath + "-journal" }) + { + try + { + File.Delete(path); + } + catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) + { + // Best effort: the original failure is what gets reported. + } + } + } + /// /// Reads the stored PRAGMA user_version value. /// @@ -105,14 +164,7 @@ public static class SqliteDatabase /// The database was written by a newer build. internal static void ApplyMigrations(SqliteConnection connection, IReadOnlyList migrations) { - // Contract check: the list must be ordered, contiguous and start at version 1. - for (int index = 0; index < migrations.Count; index++) - { - if (migrations[index].Version != index + 1) - { - throw new ArgumentException("Migrations must be contiguous and start at version 1.", nameof(migrations)); - } - } + ValidateMigrations(migrations); long current = ReadUserVersion(connection); int latest = migrations.Count; @@ -138,6 +190,42 @@ public static class SqliteDatabase } } + /// + /// Opens a connection to the file, creating it if needed, without any configuration. + /// + /// Path of the SQLite database file. + /// The open connection. + private static SqliteConnection OpenConnection(string databasePath) + { + var connection = new SqliteConnection(new SqliteConnectionStringBuilder + { + DataSource = databasePath, + Mode = SqliteOpenMode.ReadWriteCreate, + + // Each process holds its own single connection; a pool would only add noise. + Pooling = false, + }.ToString()); + + connection.Open(); + return connection; + } + + /// + /// Checks that the migration list is ordered, contiguous and starts at version 1. + /// + /// The migration list. + /// The list breaks the contract. + private static void ValidateMigrations(IReadOnlyList migrations) + { + for (int index = 0; index < migrations.Count; index++) + { + if (migrations[index].Version != index + 1) + { + throw new ArgumentException("Migrations must be contiguous and start at version 1.", nameof(migrations)); + } + } + } + /// /// Executes a non-query SQL script, optionally inside an explicit transaction. /// diff --git a/YKanBan/Storage/Workspace/WorkspaceInitializer.cs b/YKanBan/Storage/Workspace/WorkspaceInitializer.cs index 25a5307..150ec06 100644 --- a/YKanBan/Storage/Workspace/WorkspaceInitializer.cs +++ b/YKanBan/Storage/Workspace/WorkspaceInitializer.cs @@ -3,10 +3,10 @@ using Microsoft.Data.Sqlite; namespace YKanBan.Storage.Workspace; /// -/// Creates the .ykanban structure for a folder: the folder itself and an -/// empty, fully migrated database. Adding preset content is deliberately a -/// separate concern handled by , so the -/// database can be created without any rows. +/// Creates the .ykanban structure for a folder in the mandated order: the +/// .ykanban folder, then the exclusive lock, then the database. The database +/// schema and the optional preset content are written in one transaction, so a +/// failure never leaves a half-built database behind. /// public static class WorkspaceInitializer { @@ -19,27 +19,50 @@ public static class WorkspaceInitializer public static bool IsWorkspace(string folderPath) => Directory.Exists(WorkspacePaths.Root(folderPath)); /// - /// Initializes a fresh workspace: creates .ykanban and an empty migrated - /// database, without adding any preset content. + /// Initializes a fresh workspace without any preset content and releases + /// the lock again. /// /// The existing folder to initialize. /// The folder does not exist on disk. + /// Another instance holds the workspace lock. /// The workspace database already exists. public static void Initialize(string folderPath) + { + using WorkspaceLock workspaceLock = InitializeLocked(folderPath, addPresetColumns: false); + } + + /// + /// Initializes a fresh workspace and returns the lock still held, so the + /// caller can open the workspace without ever releasing it. + /// + /// The existing folder to initialize. + /// Whether to add the preset columns in the schema transaction. + /// The held workspace lock. + /// The folder does not exist on disk. + /// Another instance holds the workspace lock. + /// The workspace database already exists. + internal static WorkspaceLock InitializeLocked(string folderPath, bool addPresetColumns) { if (!Directory.Exists(folderPath)) { throw new WorkspaceDirectoryMissingException(folderPath); } - string databasePath = WorkspacePaths.Database(folderPath); - if (File.Exists(databasePath)) - { - throw new InvalidOperationException($"The workspace database already exists: {databasePath}"); - } - - // Create the .ykanban folder, then the migrated but still empty database. + // .ykanban first: the lock file lives inside it. Directory.CreateDirectory(WorkspacePaths.Root(folderPath)); - using SqliteConnection connection = SqliteDatabase.Open(databasePath, WorkspaceSchema.Migrations); + WorkspaceLock workspaceLock = WorkspaceLock.Acquire(folderPath); + try + { + using SqliteConnection connection = SqliteDatabase.Create( + WorkspacePaths.Database(folderPath), + WorkspaceSchema.Migrations, + addPresetColumns ? WorkspacePreset.AddPresetColumns : null); + return workspaceLock; + } + catch + { + workspaceLock.Dispose(); + throw; + } } } diff --git a/YKanBan/Storage/Workspace/WorkspacePreset.cs b/YKanBan/Storage/Workspace/WorkspacePreset.cs index db27551..2807c54 100644 --- a/YKanBan/Storage/Workspace/WorkspacePreset.cs +++ b/YKanBan/Storage/Workspace/WorkspacePreset.cs @@ -12,15 +12,14 @@ public static class WorkspacePreset { /// /// Inserts the three preset columns (the current language's equivalents of - /// To Do / In Progress / Done) in a single transaction. + /// To Do / In Progress / Done) inside the caller's transaction, so the + /// presets land together with the schema or not at all. /// - /// The initialized workspace folder. - /// The workspace database does not exist or a title collides with an existing column. - public static void AddPresetColumns(string folderPath) + /// The connection to the freshly created database. + /// The transaction that also builds the schema. + /// A title collides with an existing column. + public static void AddPresetColumns(SqliteConnection connection, SqliteTransaction transaction) { - using SqliteConnection connection = SqliteDatabase.Open( - WorkspacePaths.Database(folderPath), WorkspaceSchema.Migrations); - long now = UnixTime.Now; // Preset titles are data at creation time: whatever the current language says gets stored. @@ -31,8 +30,6 @@ public static class WorkspacePreset Resources.PresetColumn_Done, ]; - // Insert all three atomically so a partially populated workspace can never exist. - using var transaction = connection.BeginTransaction(); foreach (string title in titles) { using var command = connection.CreateCommand(); @@ -45,6 +42,5 @@ public static class WorkspacePreset command.Parameters.AddWithValue("$now", now); command.ExecuteNonQuery(); } - transaction.Commit(); } } diff --git a/YKanBan/Storage/Workspace/WorkspaceSession.cs b/YKanBan/Storage/Workspace/WorkspaceSession.cs index 4d11333..c39f44f 100644 --- a/YKanBan/Storage/Workspace/WorkspaceSession.cs +++ b/YKanBan/Storage/Workspace/WorkspaceSession.cs @@ -41,9 +41,29 @@ public sealed class WorkspaceSession : IDisposable /// The opened session; dispose it to release the lock and connection. /// The folder has no .ykanban structure. /// Another instance already holds the lock. - public static WorkspaceSession Open(string folderPath) + public static WorkspaceSession Open(string folderPath) => OpenLocked(folderPath, WorkspaceLock.Acquire(folderPath)); + + /// + /// Initializes a workspace with its preset columns and opens it. The lock + /// is taken right after .ykanban is created and is never released in + /// between, so no other instance can interleave with the initialization. + /// + /// The existing folder to initialize. + /// The opened session; dispose it to release the lock and connection. + /// The folder does not exist on disk. + /// Another instance holds the workspace lock. + /// The workspace database already exists. + public static WorkspaceSession Initialize(string folderPath) => + OpenLocked(folderPath, WorkspaceInitializer.InitializeLocked(folderPath, addPresetColumns: true)); + + /// + /// Opens the repository under an already held lock; the lock is released if that fails. + /// + /// The workspace folder. + /// The held workspace lock. + /// The opened session. + private static WorkspaceSession OpenLocked(string folderPath, WorkspaceLock workspaceLock) { - WorkspaceLock workspaceLock = WorkspaceLock.Acquire(folderPath); try { var repository = new WorkspaceRepository(folderPath); diff --git a/YKanBan/ViewModels/MainWindowViewModel.cs b/YKanBan/ViewModels/MainWindowViewModel.cs index 1c8c62d..ea712c7 100644 --- a/YKanBan/ViewModels/MainWindowViewModel.cs +++ b/YKanBan/ViewModels/MainWindowViewModel.cs @@ -105,15 +105,21 @@ public sealed partial class MainWindowViewModel : ViewModelBase /// Opens the workspace and switches the content to the normal view. /// /// The workspace folder to open. - private void OpenWorkspace(string folderPath) + private void OpenWorkspace(string folderPath) => ShowWorkspace(folderPath, _services.OpenWorkspace(folderPath)); + + /// + /// Switches the content to the normal view of an opened workspace. + /// + /// The workspace folder. + /// The opened workspace session. + private void ShowWorkspace(string folderPath, WorkspaceSession session) { - WorkspaceSession session = _services.OpenWorkspace(folderPath); Content = new WorkspaceViewModel(session, ShowSettings, ShowAbout, _services.Config, _dialogs); Title = $"{new DirectoryInfo(folderPath).Name} - {Resources.App_Name}"; } /// - /// Initializes the folder shown by the not-initialized page, adds the preset + /// Initializes the folder shown by the not-initialized page with its preset /// columns and opens the resulting workspace. /// /// A completed task once initialization has been attempted. @@ -126,9 +132,12 @@ public sealed partial class MainWindowViewModel : ViewModelBase try { - WorkspaceInitializer.Initialize(page.FolderPath); - WorkspacePreset.AddPresetColumns(page.FolderPath); - OpenWorkspace(page.FolderPath); + ShowWorkspace(page.FolderPath, _services.InitializeWorkspace(page.FolderPath)); + } + catch (WorkspaceLockException) + { + // Another instance got there first and holds the workspace: same page as at startup. + Content = new LockConflictPageViewModel(page.FolderPath); } catch (Exception exception) {