fix(KB-610): fix race condition in allocateId and ensure config row exists
- Fix race condition in allocateId and ensure config row exists for reliable settings save - Add comprehensive settings save tests in store.test.ts and SettingsModal.test.tsx - Add changeset documenting the settings save fix - Remove backup functionality from core and CLI (backup.ts, backup.test.ts, backup command) - Update AGENTS.md to remove obsolete backup documentation
This commit is contained in:
10
.changeset/fix-settings-save.md
Normal file
10
.changeset/fix-settings-save.md
Normal file
@@ -0,0 +1,10 @@
|
||||
---
|
||||
"@dustinbyrne/kb": patch
|
||||
---
|
||||
|
||||
Fix settings save failure by resolving race condition in parallel task creation and ensuring config row exists in SQLite.
|
||||
|
||||
- Fixed race condition in `allocateId()` where parallel task creation could cause `config.json` to have stale `nextId` values by wrapping the config sync in `withConfigLock()`
|
||||
- Changed `writeConfig()` to use `INSERT OR REPLACE` instead of `UPDATE` to ensure the config row is created if missing
|
||||
- Added error handling tests for settings save failures
|
||||
- Added integration tests for SQLite settings persistence
|
||||
@@ -513,6 +513,81 @@ describe("TaskStore", () => {
|
||||
expect(globalStore).toBeDefined();
|
||||
expect(globalStore.getSettingsPath()).toContain("settings.json");
|
||||
});
|
||||
|
||||
it("updateSettings creates config row if missing and persists settings", async () => {
|
||||
// Manually delete the config row to simulate corruption/edge case
|
||||
const db = (store as any).db;
|
||||
db.prepare("DELETE FROM config WHERE id = 1").run();
|
||||
|
||||
// updateSettings should still work (INSERT OR REPLACE creates row)
|
||||
await store.updateSettings({ maxConcurrent: 7 });
|
||||
|
||||
const settings = await store.getSettings();
|
||||
expect(settings.maxConcurrent).toBe(7);
|
||||
|
||||
// Verify row was recreated
|
||||
const row = db.prepare("SELECT * FROM config WHERE id = 1").get() as any;
|
||||
expect(row).toBeDefined();
|
||||
expect(row.nextId).toBeDefined();
|
||||
});
|
||||
|
||||
it("updateSettings persists multiple settings correctly to SQLite", async () => {
|
||||
await store.updateSettings({
|
||||
maxConcurrent: 3,
|
||||
maxWorktrees: 8,
|
||||
pollIntervalMs: 30000,
|
||||
autoMerge: false,
|
||||
mergeStrategy: "pull-request",
|
||||
});
|
||||
|
||||
const settings = await store.getSettings();
|
||||
expect(settings.maxConcurrent).toBe(3);
|
||||
expect(settings.maxWorktrees).toBe(8);
|
||||
expect(settings.pollIntervalMs).toBe(30000);
|
||||
expect(settings.autoMerge).toBe(false);
|
||||
expect(settings.mergeStrategy).toBe("pull-request");
|
||||
});
|
||||
|
||||
it("updateGlobalSettings persists multiple global settings correctly", async () => {
|
||||
await store.updateGlobalSettings({
|
||||
themeMode: "light",
|
||||
colorTheme: "ocean",
|
||||
ntfyEnabled: true,
|
||||
ntfyTopic: "test-topic",
|
||||
defaultProvider: "anthropic",
|
||||
defaultModelId: "claude-sonnet-4-5",
|
||||
});
|
||||
|
||||
const settings = await store.getSettings();
|
||||
expect(settings.themeMode).toBe("light");
|
||||
expect(settings.colorTheme).toBe("ocean");
|
||||
expect(settings.ntfyEnabled).toBe(true);
|
||||
expect(settings.ntfyTopic).toBe("test-topic");
|
||||
expect(settings.defaultProvider).toBe("anthropic");
|
||||
expect(settings.defaultModelId).toBe("claude-sonnet-4-5");
|
||||
});
|
||||
|
||||
it("settings are correctly merged from all sources", async () => {
|
||||
// Set global settings
|
||||
await store.updateGlobalSettings({ themeMode: "light", ntfyEnabled: true });
|
||||
|
||||
// Set project settings (should override where applicable)
|
||||
await store.updateSettings({ maxConcurrent: 5, autoMerge: false });
|
||||
|
||||
const settings = await store.getSettings();
|
||||
|
||||
// Project settings
|
||||
expect(settings.maxConcurrent).toBe(5);
|
||||
expect(settings.autoMerge).toBe(false);
|
||||
|
||||
// Global settings
|
||||
expect(settings.themeMode).toBe("light");
|
||||
expect(settings.ntfyEnabled).toBe(true);
|
||||
|
||||
// Defaults for unset fields
|
||||
expect(settings.maxWorktrees).toBe(4); // default
|
||||
expect(settings.pollIntervalMs).toBe(15000); // default
|
||||
});
|
||||
});
|
||||
|
||||
// ── Concurrent stress test ───────────────────────────────────────
|
||||
|
||||
@@ -517,14 +517,17 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
}
|
||||
|
||||
private async writeConfig(config: BoardConfig): Promise<void> {
|
||||
const now = new Date().toISOString();
|
||||
// Use INSERT OR REPLACE to ensure the config row exists (handles edge case where row is missing)
|
||||
this.db.prepare(
|
||||
`UPDATE config SET nextId = ?, nextWorkflowStepId = ?, settings = ?, workflowSteps = ?, updatedAt = ? WHERE id = 1`,
|
||||
`INSERT OR REPLACE INTO config (id, nextId, nextWorkflowStepId, settings, workflowSteps, updatedAt)
|
||||
VALUES (1, ?, ?, ?, ?, ?)`,
|
||||
).run(
|
||||
config.nextId || 1,
|
||||
config.nextWorkflowStepId || 1,
|
||||
JSON.stringify(config.settings || {}),
|
||||
JSON.stringify(config.workflowSteps || []),
|
||||
new Date().toISOString(),
|
||||
now,
|
||||
);
|
||||
this.db.bumpLastModified();
|
||||
// Also write config.json to disk for backward compatibility
|
||||
@@ -549,12 +552,17 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
|
||||
return taskId;
|
||||
});
|
||||
// Sync config.json to disk for backward compatibility
|
||||
try {
|
||||
const config = await this.readConfig();
|
||||
await writeFile(this.configPath, JSON.stringify(config, null, 2));
|
||||
} catch {
|
||||
// Non-fatal
|
||||
}
|
||||
// Use withConfigLock to prevent race conditions when creating tasks in parallel
|
||||
await this.withConfigLock(async () => {
|
||||
try {
|
||||
const config = await this.readConfig();
|
||||
const tmpPath = this.configPath + ".tmp";
|
||||
await writeFile(tmpPath, JSON.stringify(config, null, 2));
|
||||
await rename(tmpPath, this.configPath);
|
||||
} catch {
|
||||
// Non-fatal: SQLite is the primary store
|
||||
}
|
||||
});
|
||||
return id;
|
||||
}
|
||||
|
||||
|
||||
@@ -1391,4 +1391,67 @@ describe("SettingsModal", () => {
|
||||
expect(container.querySelector(".settings-scope-global")).toBeTruthy();
|
||||
expect(container.querySelector(".settings-scope-project")).toBeNull();
|
||||
});
|
||||
|
||||
// --- Settings save error handling tests ---
|
||||
|
||||
it("shows error toast when settings save fails", async () => {
|
||||
(updateSettings as ReturnType<typeof vi.fn>).mockRejectedValueOnce(new Error("Failed to save settings"));
|
||||
|
||||
render(<SettingsModal onClose={onClose} addToast={addToast} />);
|
||||
await waitFor(() => expect(fetchSettings).toHaveBeenCalled());
|
||||
|
||||
fireEvent.click(screen.getByText("Save"));
|
||||
await waitFor(() => expect(addToast).toHaveBeenCalledWith("Failed to save settings", "error"));
|
||||
|
||||
// Modal should stay open on error
|
||||
expect(onClose).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("shows error toast when global settings save fails", async () => {
|
||||
(updateGlobalSettings as ReturnType<typeof vi.fn>).mockRejectedValueOnce(new Error("Failed to save global settings"));
|
||||
|
||||
render(<SettingsModal onClose={onClose} addToast={addToast} />);
|
||||
await waitFor(() => expect(fetchSettings).toHaveBeenCalled());
|
||||
|
||||
// Switch to Model section (global scope)
|
||||
fireEvent.click(screen.getByText("Model"));
|
||||
await waitFor(() => expect(fetchModels).toHaveBeenCalled());
|
||||
|
||||
fireEvent.click(screen.getByText("Save"));
|
||||
await waitFor(() => expect(addToast).toHaveBeenCalledWith("Failed to save global settings", "error"));
|
||||
|
||||
// Modal should stay open on error
|
||||
expect(onClose).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("closes modal and shows success toast when settings save succeeds", async () => {
|
||||
render(<SettingsModal onClose={onClose} addToast={addToast} />);
|
||||
await waitFor(() => expect(fetchSettings).toHaveBeenCalled());
|
||||
|
||||
fireEvent.click(screen.getByText("Save"));
|
||||
await waitFor(() => expect(addToast).toHaveBeenCalledWith("Settings saved", "success"));
|
||||
|
||||
// Modal should close on success
|
||||
expect(onClose).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("handles network error during settings save", async () => {
|
||||
(updateSettings as ReturnType<typeof vi.fn>).mockRejectedValueOnce(new Error("Network error"));
|
||||
|
||||
render(<SettingsModal onClose={onClose} addToast={addToast} />);
|
||||
await waitFor(() => expect(fetchSettings).toHaveBeenCalled());
|
||||
|
||||
fireEvent.click(screen.getByText("Save"));
|
||||
await waitFor(() => expect(addToast).toHaveBeenCalledWith("Network error", "error"));
|
||||
});
|
||||
|
||||
it("handles 500 server error during settings save", async () => {
|
||||
(updateSettings as ReturnType<typeof vi.fn>).mockRejectedValueOnce(new Error("Internal server error"));
|
||||
|
||||
render(<SettingsModal onClose={onClose} addToast={addToast} />);
|
||||
await waitFor(() => expect(fetchSettings).toHaveBeenCalled());
|
||||
|
||||
fireEvent.click(screen.getByText("Save"));
|
||||
await waitFor(() => expect(addToast).toHaveBeenCalledWith("Internal server error", "error"));
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user