fix(gsd): sync milestone DB status in parkMilestone and unparkMilestone (#2696)

* fix: sync milestone DB status in parkMilestone and unparkMilestone

parkMilestone only wrote the PARKED.md filesystem marker but never
updated the DB milestones.status field. Similarly, unparkMilestone
deleted the marker but left the DB at 'parked'. Because
deriveStateFromDb checks BOTH the filesystem marker AND m.status,
an unparked milestone was still skipped — the user saw 'All milestones
complete' despite the milestone being unparked on disk.

The fix adds updateMilestoneStatus() to gsd-db.ts and calls it from
both parkMilestone (→ 'parked') and unparkMilestone (→ 'active'),
guarded by isDbAvailable() with non-fatal try/catch.

Closes #2694

* review: log DB sync failures instead of silently swallowing

Replace empty catch blocks with process.stderr.write so park/unpark
DB sync failures are visible. Matches the pattern used in gsd-db.ts
for non-fatal DB errors.

Addresses review feedback from igouss on PR #2696.
This commit is contained in:
mastertyko 2026-03-26 23:17:25 +01:00 committed by GitHub
parent 6cc6c36a69
commit c6c194b7e9
3 changed files with 114 additions and 0 deletions

View file

@ -1485,6 +1485,18 @@ export function getMilestone(id: string): MilestoneRow | null {
return rowToMilestone(row);
}
/**
* Update a milestone's status in the database.
* Used by park/unpark to keep the DB in sync with the filesystem marker.
* See: https://github.com/gsd-build/gsd-2/issues/2694
*/
export function updateMilestoneStatus(milestoneId: string, status: string): void {
if (!currentDb) throw new GSDError(GSD_STALE_STATE, "gsd-db: No database open");
currentDb.prepare(
`UPDATE milestones SET status = :status WHERE id = :id`,
).run({ ":status": status, ":id": milestoneId });
}
export function getActiveMilestoneFromDb(): MilestoneRow | null {
if (!currentDb) return null;
const row = currentDb.prepare(

View file

@ -20,6 +20,7 @@ import {
} from "./paths.js";
import { invalidateAllCaches } from "./cache.js";
import { loadQueueOrder, saveQueueOrder } from "./queue-order.js";
import { isDbAvailable, updateMilestoneStatus } from "./gsd-db.js";
// ─── Park ──────────────────────────────────────────────────────────────────
@ -52,6 +53,14 @@ export function parkMilestone(basePath: string, milestoneId: string, reason: str
].join("\n");
writeFileSync(parkedPath, content, "utf-8");
// Sync DB status so deriveStateFromDb also skips this milestone (#2694)
if (isDbAvailable()) {
try {
updateMilestoneStatus(milestoneId, "parked");
} catch (err) {
process.stderr.write(`gsd: parkMilestone DB sync failed for ${milestoneId}: ${(err as Error).message}\n`);
}
}
invalidateAllCaches();
return true;
}
@ -70,6 +79,14 @@ export function unparkMilestone(basePath: string, milestoneId: string): boolean
if (!existsSync(parkedPath)) return false; // not parked
unlinkSync(parkedPath);
// Sync DB status so deriveStateFromDb picks up the unparked milestone (#2694)
if (isDbAvailable()) {
try {
updateMilestoneStatus(milestoneId, "active");
} catch (err) {
process.stderr.write(`gsd: unparkMilestone DB sync failed for ${milestoneId}: ${(err as Error).message}\n`);
}
}
invalidateAllCaches();
return true;
}

View file

@ -0,0 +1,85 @@
/**
* Regression test for #2694: parkMilestone and unparkMilestone must
* update the DB milestone status alongside the filesystem marker.
*
* Without this, deriveStateFromDb skips unparked milestones because
* the DB still has status='parked', causing "All milestones complete".
*/
import { test } from "node:test";
import assert from "node:assert/strict";
import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs";
import { join } from "node:path";
import { tmpdir } from "node:os";
import { parkMilestone, unparkMilestone } from "../milestone-actions.ts";
import {
openDatabase,
closeDatabase,
insertMilestone,
getMilestone,
} from "../gsd-db.ts";
function createBase(): string {
const base = mkdtempSync(join(tmpdir(), "gsd-park-db-"));
mkdirSync(join(base, ".gsd", "milestones", "M001"), { recursive: true });
writeFileSync(
join(base, ".gsd", "milestones", "M001", "M001-CONTEXT.md"),
"# M001\n\nContext.",
);
return base;
}
test("parkMilestone updates DB status to 'parked' (#2694)", () => {
const base = createBase();
try {
openDatabase(":memory:");
insertMilestone({ id: "M001", title: "Test", status: "active" });
assert.equal(getMilestone("M001")!.status, "active", "starts active");
parkMilestone(base, "M001", "deprioritized");
assert.equal(getMilestone("M001")!.status, "parked", "DB status should be parked");
closeDatabase();
} finally {
closeDatabase();
rmSync(base, { recursive: true, force: true });
}
});
test("unparkMilestone updates DB status to 'active' (#2694)", () => {
const base = createBase();
try {
openDatabase(":memory:");
insertMilestone({ id: "M001", title: "Test", status: "active" });
// Park first
parkMilestone(base, "M001", "deprioritized");
assert.equal(getMilestone("M001")!.status, "parked");
// Unpark
unparkMilestone(base, "M001");
assert.equal(getMilestone("M001")!.status, "active", "DB status should be active after unpark");
closeDatabase();
} finally {
closeDatabase();
rmSync(base, { recursive: true, force: true });
}
});
test("park/unpark are safe when DB is not available (#2694 guard)", () => {
const base = createBase();
try {
// No openDatabase — DB not available
// park/unpark should still work (filesystem-only, no throw)
const parked = parkMilestone(base, "M001", "test");
assert.ok(parked, "parkMilestone succeeds without DB");
const unparked = unparkMilestone(base, "M001");
assert.ok(unparked, "unparkMilestone succeeds without DB");
} finally {
rmSync(base, { recursive: true, force: true });
}
});