fix(gsd): wrap decision and requirement saves in transaction to prevent ID races
nextDecisionId() and nextRequirementId() compute the next ID via SELECT MAX then pass it to a separate upsertDecision/upsertRequirement call. When parallel tool calls hit these functions concurrently, both read the same MAX value and produce the same ID — the second insert silently overwrites the first. Move the SELECT MAX + INSERT into a single transaction() call from gsd-db.ts, which uses BEGIN/COMMIT/ROLLBACK and works on both better-sqlite3 and node:sqlite providers. The transaction is re-entrant safe (nested calls skip the BEGIN). Same fix applied to saveRequirementToDb for consistency. Closes #3326, closes #3339, closes #3459
This commit is contained in:
parent
a061e3c276
commit
c70eacea89
2 changed files with 105 additions and 27 deletions
|
|
@ -272,6 +272,10 @@ export interface SaveRequirementFields {
|
|||
/**
|
||||
* Save a new requirement to DB and regenerate REQUIREMENTS.md.
|
||||
* Auto-assigns the next ID via nextRequirementId().
|
||||
*
|
||||
* The ID computation and insert are wrapped in a single transaction
|
||||
* to prevent parallel race conditions (same pattern as saveDecisionToDb).
|
||||
*
|
||||
* Returns the assigned ID.
|
||||
*/
|
||||
export async function saveRequirementToDb(
|
||||
|
|
@ -281,24 +285,37 @@ export async function saveRequirementToDb(
|
|||
try {
|
||||
const db = await import('./gsd-db.js');
|
||||
|
||||
const id = await nextRequirementId();
|
||||
// Atomic ID assignment + insert inside a transaction.
|
||||
const id = db.transaction(() => {
|
||||
const adapter = db._getAdapter();
|
||||
if (!adapter) throw new GSDError(GSD_STALE_STATE, "gsd-db: No database open");
|
||||
|
||||
const requirement: Requirement = {
|
||||
id,
|
||||
class: fields.class,
|
||||
status: fields.status ?? 'active',
|
||||
description: fields.description,
|
||||
why: fields.why,
|
||||
source: fields.source,
|
||||
primary_owner: fields.primary_owner ?? '',
|
||||
supporting_slices: fields.supporting_slices ?? '',
|
||||
validation: fields.validation ?? '',
|
||||
notes: fields.notes ?? '',
|
||||
full_content: '',
|
||||
superseded_by: null,
|
||||
};
|
||||
const row = adapter
|
||||
.prepare('SELECT MAX(CAST(SUBSTR(id, 2) AS INTEGER)) as max_num FROM requirements')
|
||||
.get();
|
||||
const maxNum = row ? (row['max_num'] as number | null) : null;
|
||||
const nextId = (maxNum == null || isNaN(maxNum))
|
||||
? 'R001'
|
||||
: `R${String(maxNum + 1).padStart(3, '0')}`;
|
||||
|
||||
db.upsertRequirement(requirement);
|
||||
const requirement: Requirement = {
|
||||
id: nextId,
|
||||
class: fields.class,
|
||||
status: fields.status ?? 'active',
|
||||
description: fields.description,
|
||||
why: fields.why,
|
||||
source: fields.source,
|
||||
primary_owner: fields.primary_owner ?? '',
|
||||
supporting_slices: fields.supporting_slices ?? '',
|
||||
validation: fields.validation ?? '',
|
||||
notes: fields.notes ?? '',
|
||||
full_content: '',
|
||||
superseded_by: null,
|
||||
};
|
||||
|
||||
db.upsertRequirement(requirement);
|
||||
return nextId;
|
||||
});
|
||||
|
||||
// Fetch all requirements for full file regeneration
|
||||
const adapter = db._getAdapter();
|
||||
|
|
@ -358,6 +375,11 @@ export interface SaveDecisionFields {
|
|||
/**
|
||||
* Save a new decision to DB and regenerate DECISIONS.md.
|
||||
* Auto-assigns the next ID via nextDecisionId().
|
||||
*
|
||||
* The ID computation (SELECT MAX) and insert are wrapped in a single
|
||||
* transaction to prevent parallel tool calls from computing the same ID
|
||||
* and silently overwriting each other (#3326, #3339, #3459).
|
||||
*
|
||||
* Returns the assigned ID.
|
||||
*/
|
||||
export async function saveDecisionToDb(
|
||||
|
|
@ -367,18 +389,33 @@ export async function saveDecisionToDb(
|
|||
try {
|
||||
const db = await import('./gsd-db.js');
|
||||
|
||||
const id = await nextDecisionId();
|
||||
// Atomic ID assignment + insert inside a transaction to prevent
|
||||
// parallel calls from racing on the same MAX(id) value.
|
||||
const id = db.transaction(() => {
|
||||
const adapter = db._getAdapter();
|
||||
if (!adapter) throw new GSDError(GSD_STALE_STATE, "gsd-db: No database open");
|
||||
|
||||
db.upsertDecision({
|
||||
id,
|
||||
when_context: fields.when_context ?? '',
|
||||
scope: fields.scope,
|
||||
decision: fields.decision,
|
||||
choice: fields.choice,
|
||||
rationale: fields.rationale,
|
||||
revisable: fields.revisable ?? 'Yes',
|
||||
made_by: fields.made_by ?? 'agent',
|
||||
superseded_by: null,
|
||||
const row = adapter
|
||||
.prepare('SELECT MAX(CAST(SUBSTR(id, 2) AS INTEGER)) as max_num FROM decisions')
|
||||
.get();
|
||||
const maxNum = row ? (row['max_num'] as number | null) : null;
|
||||
const nextId = (maxNum == null || isNaN(maxNum))
|
||||
? 'D001'
|
||||
: `D${String(maxNum + 1).padStart(3, '0')}`;
|
||||
|
||||
db.upsertDecision({
|
||||
id: nextId,
|
||||
when_context: fields.when_context ?? '',
|
||||
scope: fields.scope,
|
||||
decision: fields.decision,
|
||||
choice: fields.choice,
|
||||
rationale: fields.rationale,
|
||||
revisable: fields.revisable ?? 'Yes',
|
||||
made_by: fields.made_by ?? 'agent',
|
||||
superseded_by: null,
|
||||
});
|
||||
|
||||
return nextId;
|
||||
});
|
||||
|
||||
// Fetch all decisions (including superseded for the full register)
|
||||
|
|
|
|||
|
|
@ -358,6 +358,47 @@ describe('db-writer', () => {
|
|||
}
|
||||
});
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
// Parallel save race condition regression (#3326, #3339, #3459)
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
|
||||
test('parallel saveDecisionToDb calls produce unique IDs', async () => {
|
||||
const tmpDir = makeTmpDir();
|
||||
const dbPath = path.join(tmpDir, '.gsd', 'gsd.db');
|
||||
openDatabase(dbPath);
|
||||
|
||||
try {
|
||||
// Fire 5 saves concurrently — before the fix, all would get D001
|
||||
const results = await Promise.all([
|
||||
saveDecisionToDb({ scope: 'a', decision: 'd1', choice: 'c1', rationale: 'r1' }, tmpDir),
|
||||
saveDecisionToDb({ scope: 'b', decision: 'd2', choice: 'c2', rationale: 'r2' }, tmpDir),
|
||||
saveDecisionToDb({ scope: 'c', decision: 'd3', choice: 'c3', rationale: 'r3' }, tmpDir),
|
||||
saveDecisionToDb({ scope: 'd', decision: 'd4', choice: 'c4', rationale: 'r4' }, tmpDir),
|
||||
saveDecisionToDb({ scope: 'e', decision: 'd5', choice: 'c5', rationale: 'r5' }, tmpDir),
|
||||
]);
|
||||
|
||||
const ids = results.map((r) => r.id);
|
||||
const uniqueIds = new Set(ids);
|
||||
|
||||
// All 5 IDs must be unique
|
||||
assert.equal(uniqueIds.size, 5, `Expected 5 unique IDs, got ${uniqueIds.size}: ${ids.join(', ')}`);
|
||||
|
||||
// IDs should be D001-D005 (order may vary due to concurrency)
|
||||
for (const id of ids) {
|
||||
assert.match(id, /^D\d{3}$/, `ID ${id} should match D### pattern`);
|
||||
}
|
||||
|
||||
// Verify all 5 exist in DB
|
||||
for (const id of ids) {
|
||||
const row = getDecisionById(id);
|
||||
assert.ok(row, `Decision ${id} should exist in DB`);
|
||||
}
|
||||
} finally {
|
||||
closeDatabase();
|
||||
cleanupDir(tmpDir);
|
||||
}
|
||||
});
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
// updateRequirementInDb Tests
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue