fix(research-loop-cap): claim each turn slot with O_EXCL so the bound survives concurrency
countTurns-then-appendFileSync is read-then-write. N callers that all observe used == budget-1 all decide to grant, and the bound is exceeded by N-1. The comment above allowTurn asserted "Append-only: never read-modify-write" and named the concurrent case - Phase 4.5/5 may spawn several agents in a single message - as the reason it had to be. The decision path was exactly what the comment denied, so the concurrency claim had nothing under it. Each grant now creates <data root>/trekresearch-loop-claims/<runId>-<slot>.claim with flag 'wx' (O_CREAT|O_EXCL) before appending. The kernel picks the winner per slot, slot numbers are bounded by the budget, and each can be created exactly once - so total grants for a run cannot exceed the budget however many callers arrive together. The ledger count now only says where to start looking for a free slot. Two of the three tests are deterministic and do not race anything: they assert the invariant directly by pre-creating claims, including the state a mid-append competitor leaves behind (ledger 7, slots 1-8 claimed, budget 8 -> deny). That matters because the third test - six real concurrent shim processes at the boundary - passed even BEFORE the fix, since process startup jitter serialised them. A race test that passes by luck is not evidence, so it ships as a real-world regression guard next to the two that are. Stated rather than left to be discovered: claim files are empty, at most budget per run, and never cleaned - the same standing as the ledger, which also grows for the life of the data root. Reusing a runId across runs, or two runIds colliding after filename sanitisation, both deny a turn, which is the safe direction for a budget control. Review finding 3994491ef1fdba6e0e3645b5b713cbdbdeb2b328 (MINOR). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LuGhWAbWyRFBFeemfhxoVv
This commit is contained in:
parent
869dc9cf4a
commit
1d279fb875
3 changed files with 181 additions and 12 deletions
|
|
@ -6,8 +6,8 @@
|
|||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, rmSync, existsSync } from 'node:fs';
|
||||
import { execFileSync, execFile } from 'node:child_process';
|
||||
import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync, existsSync } from 'node:fs';
|
||||
import { tmpdir } from 'node:os';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
|
@ -324,6 +324,108 @@ test('allowTurn — an unreadable ledger DENIES the turn instead of granting a f
|
|||
});
|
||||
});
|
||||
|
||||
// ---- the bound holds under concurrency --------------------------------------
|
||||
//
|
||||
// countTurns-then-appendFileSync had no atomic claim, while the comment above
|
||||
// allowTurn asserted "Append-only: never read-modify-write" and named the
|
||||
// concurrent case (Phase 4.5/5 may spawn several agents in one message) as the
|
||||
// reason. The decision path WAS read-then-write: N callers that all observe
|
||||
// used == budget-1 all grant, and the bound is exceeded by N-1.
|
||||
//
|
||||
// Two of these tests are deterministic. They do not race anything — they assert
|
||||
// the invariant the claim introduces: a slot that is already claimed is spent,
|
||||
// even when the ledger has not caught up yet, which is exactly the state a
|
||||
// mid-append competitor leaves behind. The third runs real processes.
|
||||
|
||||
function seedLedger(dir, runId, n) {
|
||||
writeFileSync(
|
||||
join(dir, 'trekresearch-loop-ledger.jsonl'),
|
||||
Array.from({ length: n }, (_, i) =>
|
||||
JSON.stringify({ ts: new Date().toISOString(), runId, dimension: `d${i}`, effort: 'high' }),
|
||||
).join('\n') + (n ? '\n' : ''),
|
||||
);
|
||||
}
|
||||
|
||||
function seedClaims(dir, runId, slots) {
|
||||
const claimDir = join(dir, 'trekresearch-loop-claims');
|
||||
mkdirSync(claimDir, { recursive: true });
|
||||
for (const s of slots) writeFileSync(join(claimDir, `${runId}-${s}.claim`), '');
|
||||
}
|
||||
|
||||
function runShimAsync(args, env) {
|
||||
return new Promise((resolve) => {
|
||||
execFile(
|
||||
process.execPath,
|
||||
[SHIM, ...args],
|
||||
{ encoding: 'utf-8', env: { PATH: process.env.PATH, ...env } },
|
||||
(err, stdout) => resolve({ code: err ? (err.code ?? 1) : 0, out: stdout ?? '' }),
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
test('allowTurn — a slot already claimed is spent even when the ledger has not caught up', () => {
|
||||
withTmpDataDir((dir) => {
|
||||
const env = { CLAUDE_PLUGIN_DATA: dir, VOYAGE_STORM_ENABLED: '1', TREKRESEARCH_MAX_CONV_TURNS: '1' };
|
||||
// budget = 1 × 8 = 8. Ledger shows 7 turns; a competitor already claimed
|
||||
// slot 8 and has not appended yet. Counting the ledger alone says "one slot
|
||||
// free" and grants a 9th turn overall — the breach this closes.
|
||||
seedLedger(dir, 'r-race', 7);
|
||||
seedClaims(dir, 'r-race', [1, 2, 3, 4, 5, 6, 7, 8]);
|
||||
const r = allowTurn({ runId: 'r-race', dimension: 'd0', effort: 'high' }, { env });
|
||||
assert.equal(r.ok, false, 'every slot up to the budget is claimed, so there is nothing to grant');
|
||||
assert.equal(r.reason, 'budget_exhausted');
|
||||
assert.equal(r.budget, 8);
|
||||
});
|
||||
});
|
||||
|
||||
test('allowTurn — it takes the first FREE slot and claims it, so a repeat call cannot retake it', () => {
|
||||
withTmpDataDir((dir) => {
|
||||
const env = { CLAUDE_PLUGIN_DATA: dir, VOYAGE_STORM_ENABLED: '1', TREKRESEARCH_MAX_CONV_TURNS: '1' };
|
||||
seedLedger(dir, 'r-slot', 7);
|
||||
seedClaims(dir, 'r-slot', [1, 2, 3, 4, 5, 6, 7]);
|
||||
const first = allowTurn({ runId: 'r-slot', dimension: 'd0', effort: 'high' }, { env });
|
||||
assert.equal(first.ok, true, 'slot 8 is free and must be grantable');
|
||||
assert.equal(first.used, 8, 'used is the slot number, so it never double-counts a claimed slot');
|
||||
assert.ok(
|
||||
existsSync(join(dir, 'trekresearch-loop-claims', 'r-slot-8.claim')),
|
||||
'the grant must leave the claim behind as the atomic record of the slot',
|
||||
);
|
||||
const second = allowTurn({ runId: 'r-slot', dimension: 'd0', effort: 'high' }, { env });
|
||||
assert.equal(second.ok, false);
|
||||
assert.equal(second.reason, 'budget_exhausted');
|
||||
});
|
||||
});
|
||||
|
||||
test('allowTurn — a different runId is unaffected by another run’s claims', () => {
|
||||
withTmpDataDir((dir) => {
|
||||
const env = { CLAUDE_PLUGIN_DATA: dir, VOYAGE_STORM_ENABLED: '1', TREKRESEARCH_MAX_CONV_TURNS: '1' };
|
||||
seedClaims(dir, 'r-other', [1, 2, 3, 4, 5, 6, 7, 8]);
|
||||
const r = allowTurn({ runId: 'r-mine', dimension: 'd0', effort: 'high' }, { env });
|
||||
assert.equal(r.ok, true, 'claims are per-run; one run must not exhaust another');
|
||||
});
|
||||
});
|
||||
|
||||
test('allowTurn — parallel processes at the boundary cannot exceed the budget', async () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'research-loop-cap-par-'));
|
||||
try {
|
||||
const env = { CLAUDE_PLUGIN_DATA: dir, VOYAGE_STORM_ENABLED: '1', TREKRESEARCH_MAX_CONV_TURNS: '1' };
|
||||
seedLedger(dir, 'r-par', 7); // budget 8 → exactly one turn left
|
||||
const results = await Promise.all(
|
||||
Array.from({ length: 6 }, (_, i) =>
|
||||
runShimAsync(['--run-id', 'r-par', '--dimension', `p${i}`, '--effort', 'high'], env),
|
||||
),
|
||||
);
|
||||
const granted = results.filter((r) => r.code === 0).length;
|
||||
assert.equal(granted, 1, `exactly one of six concurrent callers may take the last slot, got ${granted}`);
|
||||
|
||||
const lines = readFileSync(join(dir, 'trekresearch-loop-ledger.jsonl'), 'utf-8')
|
||||
.split('\n').filter(Boolean);
|
||||
assert.equal(lines.length, 8, `the ledger must never exceed the budget, got ${lines.length}`);
|
||||
} finally {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// ---- (g) shim contract --------------------------------------------------------
|
||||
|
||||
test('CLI shim — grants and exits 0 when enabled + high effort + budget available', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue