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
d2b6a696bd
commit
156539204a
3 changed files with 181 additions and 12 deletions
|
|
@ -12,7 +12,7 @@ Imported from `CLAUDE.md` via pointer.
|
|||
- `lib/stats/event-emit.mjs` — single-source stats event emitter for autonomy-gate transitions and main-merge-gate (v3.4.0)
|
||||
- `lib/validators/{brief,research,plan,progress,session-state}-validator.mjs` — schema validators with CLI shims (`node lib/validators/X.mjs --json <path>`)
|
||||
- `lib/validators/architecture-discovery.mjs` — drift-WARN external-contract discovery for `architecture/overview.md`
|
||||
- `lib/util/research-loop-cap.mjs` — stateful, **default-off** turn budget for the `/trekresearch` bounded conversation loop. `allowTurn()` derives the used-turn count from its own append-only JSONL ledger; it never asks the caller how many turns it has spent, because a cap that does is not a cap. Budget = `TREKRESEARCH_MAX_CONV_TURNS` (default `3`, invalid values fall back to `3`) × `maxDimensions` (8, `settings.json:16`). Grants 0 unless `VOYAGE_STORM_ENABLED=1`, which is also the second condition on Phase 4.5's skip-guard (that phase does not call this module): unset, **both** STORM phases are inert. `resolveDataRoot()` is the single root for everything the loop writes — `CLAUDE_PLUGIN_DATA` when the harness sets it, `~/.claude/voyage` when it does not (it is empty in the Bash tool's process env, which is where the loop actually runs); the cap hook resolves through the same function, so writer and reader cannot disagree. A ledger that cannot be **written** denies the turn, and one that exists but cannot be **read** denies it too — only `ENOENT` counts as zero turns spent, that being the legitimate first-turn state (fail-closed — the opposite of `event-emit.mjs`, which is telemetry and must never block). The exported `readLedger()` is the single counting rule; the cap hook calls it rather than keeping a private copy. CLI shim: `node lib/util/research-loop-cap.mjs --run-id ID --dimension D --effort E`
|
||||
- `lib/util/research-loop-cap.mjs` — stateful, **default-off** turn budget for the `/trekresearch` bounded conversation loop. `allowTurn()` derives the used-turn count from its own append-only JSONL ledger; it never asks the caller how many turns it has spent, because a cap that does is not a cap. Each grant first claims a turn **slot** with `O_EXCL` under `trekresearch-loop-claims/`, so the bound survives several callers deciding at once — counting the ledger and then appending is read-then-write, and Phase 4.5/5 can spawn several agents in one message. Budget = `TREKRESEARCH_MAX_CONV_TURNS` (default `3`, invalid values fall back to `3`) × `maxDimensions` (8, `settings.json:16`). Grants 0 unless `VOYAGE_STORM_ENABLED=1`, which is also the second condition on Phase 4.5's skip-guard (that phase does not call this module): unset, **both** STORM phases are inert. `resolveDataRoot()` is the single root for everything the loop writes — `CLAUDE_PLUGIN_DATA` when the harness sets it, `~/.claude/voyage` when it does not (it is empty in the Bash tool's process env, which is where the loop actually runs); the cap hook resolves through the same function, so writer and reader cannot disagree. A ledger that cannot be **written** denies the turn, and one that exists but cannot be **read** denies it too — only `ENOENT` counts as zero turns spent, that being the legitimate first-turn state (fail-closed — the opposite of `event-emit.mjs`, which is telemetry and must never block). The exported `readLedger()` is the single counting rule; the cap hook calls it rather than keeping a private copy. CLI shim: `node lib/util/research-loop-cap.mjs --run-id ID --dimension D --effort E`
|
||||
- `lib/validators/query-privacy-gate.mjs` — gates **every** outbound research query before it leaves the machine; the hard-block tier (secret-shaped strings) is not operator-overridable, so a query that trips it must be reformulated rather than forced through. CLI shim: `node lib/validators/query-privacy-gate.mjs "<query>"`
|
||||
|
||||
Wiring points (replaces previous prose-grep instructions):
|
||||
|
|
|
|||
|
|
@ -8,7 +8,9 @@
|
|||
// leaves the mechanism off, and adopt is flipping this one constant.
|
||||
// (b) The cap counts itself — allowTurn() derives used-turn count from an
|
||||
// append-only JSONL ledger, never from a caller-supplied number. A cap
|
||||
// that asks the caller how many turns it has used is not a cap.
|
||||
// that asks the caller how many turns it has used is not a cap. Each
|
||||
// grant additionally claims a turn SLOT with O_EXCL, so the bound holds
|
||||
// when several callers decide at once instead of only when they queue.
|
||||
// (c) Correct size bound — worst case is max_conv_turns × max_total_dimensions,
|
||||
// where max_total_dimensions is the WHOLE list (interview + discovered)
|
||||
// under settings.json:16's cap of 8 — not × discovered-only.
|
||||
|
|
@ -31,7 +33,7 @@
|
|||
// node lib/util/research-loop-cap.mjs --run-id ID --dimension D --effort E
|
||||
// → JSON: { ok, used, budget, reason? } (exit 0 = granted, exit 1 = denied)
|
||||
|
||||
import { existsSync, mkdirSync, appendFileSync, readFileSync } from 'node:fs';
|
||||
import { existsSync, mkdirSync, appendFileSync, readFileSync, writeFileSync } from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { homedir } from 'node:os';
|
||||
|
||||
|
|
@ -39,6 +41,7 @@ export const MAX_CONV_TURNS = 3;
|
|||
export const MAX_TOTAL_DIMENSIONS = 8; // settings.json:16 maxDimensions — whole list, not discovered-only
|
||||
|
||||
const LEDGER_FILENAME = 'trekresearch-loop-ledger.jsonl';
|
||||
const CLAIM_DIRNAME = 'trekresearch-loop-claims';
|
||||
|
||||
export function isStormEnabled(env = process.env) {
|
||||
return env.VOYAGE_STORM_ENABLED === '1';
|
||||
|
|
@ -80,6 +83,48 @@ export function resolveLedgerPath(env = process.env) {
|
|||
return join(resolveDataRoot(env), LEDGER_FILENAME);
|
||||
}
|
||||
|
||||
/**
|
||||
* Where the per-turn claim files live. A claim is the ATOMIC record that a turn
|
||||
* slot is taken; the ledger is the readable record of what that turn was for.
|
||||
*
|
||||
* The claim exists because the ledger alone cannot bound the loop. Counting the
|
||||
* ledger and then appending is read-then-write: N callers that all observe
|
||||
* `used == budget - 1` all decide to grant, and the bound is exceeded by N-1 —
|
||||
* precisely the concurrent case (several agents spawned in one message) that
|
||||
* allowTurn's own comment named as the reason it had to be append-only.
|
||||
*
|
||||
* Claim files are empty, at most `budget` per run, and never cleaned up — the
|
||||
* same standing as the ledger itself, which also grows for the life of the data
|
||||
* root. Two consequences worth stating rather than discovering: reusing a
|
||||
* runId across runs finds its slots already taken and denies, and two runIds
|
||||
* that collide after filename sanitisation block each other. Both err toward
|
||||
* denying a turn, which is the safe direction for a budget control.
|
||||
*/
|
||||
export function resolveClaimDir(env = process.env) {
|
||||
return join(resolveDataRoot(env), CLAIM_DIRNAME);
|
||||
}
|
||||
|
||||
function claimFileName(runId, slot) {
|
||||
return `${String(runId).replace(/[^A-Za-z0-9._-]/g, '_')}-${slot}.claim`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Try to take turn slot `slot` for `runId`. `wx` is O_CREAT|O_EXCL: the kernel
|
||||
* decides the winner, so exactly one caller can ever create a given slot file.
|
||||
*
|
||||
* @returns {boolean} true when this caller took the slot, false when it was already taken
|
||||
* @throws on any IO error other than EEXIST — the caller turns that into a denial
|
||||
*/
|
||||
function claimSlot(claimDir, runId, slot) {
|
||||
try {
|
||||
writeFileSync(join(claimDir, claimFileName(runId, slot)), '', { flag: 'wx' });
|
||||
return true;
|
||||
} catch (e) {
|
||||
if (e && e.code === 'EEXIST') return false;
|
||||
throw e;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Read one run's turn count off the append-only ledger.
|
||||
*
|
||||
|
|
@ -125,10 +170,15 @@ export function readLedger(ledgerPath, runId) {
|
|||
}
|
||||
|
||||
/**
|
||||
* Decide whether one more research-loop turn may run. Append-only: never
|
||||
* read-modify-write, because Phase 4.5/5 may spawn multiple agents in a
|
||||
* single message and a read-modify-write counter would lose concurrent
|
||||
* grants.
|
||||
* Decide whether one more research-loop turn may run.
|
||||
*
|
||||
* Phase 4.5/5 may spawn several agents in a single message, so the decision has
|
||||
* to survive concurrent callers. It does that by CLAIMING a turn slot with
|
||||
* O_EXCL (see claimSlot) and only then appending to the ledger. The comment
|
||||
* that used to sit here asserted "append-only: never read-modify-write" as if
|
||||
* appending were itself the concurrency guarantee — but the decision path was
|
||||
* count-then-append, which is read-then-write, so the claim was unsupported by
|
||||
* the code beneath it. The kernel now picks the winner for each slot.
|
||||
*
|
||||
* @param {{runId: string, dimension: string, effort: string}} args
|
||||
* @param {{env?: object, now?: Date}} [opts]
|
||||
|
|
@ -158,19 +208,36 @@ export function allowTurn({ runId, dimension, effort } = {}, opts = {}) {
|
|||
} catch (e) {
|
||||
return { ok: false, used: 0, budget, reason: `ledger-read-failed: ${e.message}` };
|
||||
}
|
||||
if (used >= budget) {
|
||||
// Claim a turn SLOT before spending anything. The ledger count only says
|
||||
// where to start looking; the claim is what makes the grant exclusive. Slot
|
||||
// numbers are bounded by `budget`, and each can be created exactly once, so
|
||||
// the total number of grants for a run can never exceed the budget however
|
||||
// many callers arrive at once.
|
||||
const claimDir = resolveClaimDir(env);
|
||||
let slot = used + 1;
|
||||
let claimed = false;
|
||||
try {
|
||||
mkdirSync(claimDir, { recursive: true });
|
||||
while (slot <= budget) {
|
||||
if (claimSlot(claimDir, runId, slot)) { claimed = true; break; }
|
||||
slot++;
|
||||
}
|
||||
} catch (e) {
|
||||
return { ok: false, used, budget, reason: `claim-failed: ${e.message}` };
|
||||
}
|
||||
if (!claimed) {
|
||||
return { ok: false, used, budget, reason: 'budget_exhausted' };
|
||||
}
|
||||
|
||||
try {
|
||||
const dir = dirname(ledgerPath);
|
||||
if (!existsSync(dir)) mkdirSync(dir, { recursive: true });
|
||||
appendFileSync(ledgerPath, JSON.stringify({ ts: now.toISOString(), runId, dimension, effort }) + '\n');
|
||||
appendFileSync(ledgerPath, JSON.stringify({ ts: now.toISOString(), runId, dimension, effort, slot }) + '\n');
|
||||
} catch (e) {
|
||||
return { ok: false, used, budget, reason: `ledger-write-failed: ${e.message}` };
|
||||
}
|
||||
|
||||
return { ok: true, used: used + 1, budget };
|
||||
return { ok: true, used: slot, budget };
|
||||
}
|
||||
|
||||
// ---- CLI shim ----------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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