From 297196ff2fa2d52ae903f4359a64921ba3c6dc95 Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Fri, 28 Aug 2026 21:59:06 -0400 Subject: [PATCH 1/8] fix(obsidian): contain timed out capability probes --- .../obsidian/src/obsidian-cli-probe-worker.js | 78 +++++++++++++++++++ .../obsidian/src/vault-mutation-adapter.js | 19 ++++- .../tests/vault-mutation-adapter.test.js | 20 ++++- 3 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js new file mode 100644 index 00000000..97df28cc --- /dev/null +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -0,0 +1,78 @@ +'use strict'; + +// This helper owns the process group used by a synchronous capability probe. +// Keeping the CLI's stdio inside this process prevents a timed-out descendant +// that inherited its parent's pipes from holding the caller open. +const { spawn } = require('node:child_process'); + +const MAX_CAPTURE_BYTES = 64 * 1024; +const request = JSON.parse(Buffer.from(process.argv[2] || '', 'base64url').toString('utf8')); +const timeoutMs = Math.max(1, Number(request.timeoutMs) || 10_000); + +function capture(stream) { + let value = ''; + stream?.on('data', (chunk) => { + if (Buffer.byteLength(value) >= MAX_CAPTURE_BYTES) return; + value += String(chunk).slice(0, MAX_CAPTURE_BYTES - Buffer.byteLength(value)); + }); + return () => value; +} + +function finish(value) { + process.stdout.write(JSON.stringify(value)); +} + +let child; +try { + child = spawn(request.command, request.args, { + detached: process.platform !== 'win32', + stdio: ['ignore', 'pipe', 'pipe'], + windowsHide: true, + }); +} catch (error) { + finish({ ok: false, code: error.code, message: error.message }); + return; +} + +const stdout = capture(child.stdout); +const stderr = capture(child.stderr); +let timedOut = false; +let settled = false; + +function terminateOwnedTree() { + if (!child?.pid) return; + if (process.platform === 'win32') { + // /t targets the child process tree by PID; it is not a process-name kill. + const killer = spawn('taskkill', ['/pid', String(child.pid), '/t', '/f'], { stdio: 'ignore', windowsHide: true }); + killer.on('error', () => child.kill('SIGKILL')); + return; + } + try { process.kill(-child.pid, 'SIGTERM'); } catch { child.kill('SIGTERM'); } + setTimeout(() => { + try { process.kill(-child.pid, 'SIGKILL'); } catch { child.kill('SIGKILL'); } + }, 100).unref(); +} + +const timer = setTimeout(() => { + timedOut = true; + terminateOwnedTree(); +}, timeoutMs); + +child.on('error', (error) => { + if (settled) return; + settled = true; + clearTimeout(timer); + finish({ ok: false, code: error.code, message: error.message, stdout: stdout(), stderr: stderr() }); +}); + +child.on('close', (code, signal) => { + if (settled) return; + settled = true; + clearTimeout(timer); + if (timedOut) { + finish({ ok: false, code: 'ETIMEDOUT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms`, stdout: stdout(), stderr: stderr() }); + return; + } + if (code === 0) return finish({ ok: true, stdout: stdout(), stderr: stderr() }); + finish({ ok: false, code: code == null ? signal : code, message: stderr() || stdout() || `Obsidian CLI exited with ${code == null ? signal : code}`, stdout: stdout(), stderr: stderr() }); +}); diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js b/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js index cf94fe8a..e879103f 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js @@ -9,6 +9,7 @@ const { createVaultMutationLedger } = require('./vault-mutation-ledger'); const RESULT_STORE = '__jarvosVaultMutationResults'; const CAPABILITY_STATES = Object.freeze(['available', 'cli_missing', 'app_stopped', 'app_busy', 'app_unreachable', 'cli_disabled', 'cli_unsupported', 'wrong_vault', 'api_incompatible']); +const OBSIDIAN_CLI_PROBE_WORKER = path.join(__dirname, 'obsidian-cli-probe-worker.js'); function sleepSync(milliseconds) { if (milliseconds > 0) Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, milliseconds); @@ -16,7 +17,23 @@ function sleepSync(milliseconds) { function parseEvalResult(output) { const match = [...String(output || '').matchAll(/^=>\s*(.+)$/gm)].at(-1); return match ? JSON.parse(match[1]) : null; } function runObsidianEval(code, { vaultName, command = process.env.OBSIDIAN_CLI || 'obsidian', timeoutMs = 10_000, execute = execFileSync } = {}) { - try { return parseEvalResult(execute(command, [`vault=${vaultName}`, 'eval', `code=${code}`], { encoding: 'utf8', timeout: timeoutMs, stdio: ['ignore', 'pipe', 'pipe'] })); } + const args = [`vault=${vaultName}`, 'eval', `code=${code}`]; + try { + // Test seams that supply their own executor retain the old direct contract. + if (execute !== execFileSync) return parseEvalResult(execute(command, args, { encoding: 'utf8', timeout: timeoutMs, stdio: ['ignore', 'pipe', 'pipe'] })); + const request = Buffer.from(JSON.stringify({ command, args, timeoutMs }), 'utf8').toString('base64url'); + const response = JSON.parse(execFileSync(process.execPath, [OBSIDIAN_CLI_PROBE_WORKER, request], { + encoding: 'utf8', + timeout: timeoutMs + 1_000, + stdio: ['ignore', 'pipe', 'pipe'], + })); + if (!response.ok) { + const error = new Error(String(response.stderr || response.stdout || response.message || 'Obsidian CLI failed')); + error.code = response.code; + throw error; + } + return parseEvalResult(response.stdout); + } catch (error) { const wrapped = new Error(String(error.stderr || error.stdout || error.message || 'Obsidian CLI failed')); wrapped.code = error.code; throw wrapped; } } function payload(operation) { return Buffer.from(JSON.stringify(operation), 'utf8').toString('base64'); } diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index 5ca240bd..b2653e7a 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -5,7 +5,7 @@ const os = require('node:os'); const path = require('node:path'); const vm = require('node:vm'); const test = require('node:test'); -const { createVaultMutationAdapter } = require('../adapters/obsidian/src/vault-mutation-adapter'); +const { createVaultMutationAdapter, runObsidianEval } = require('../adapters/obsidian/src/vault-mutation-adapter'); const { buildObsidianInvariantProgram, buildObsidianMutationProgram } = require('../adapters/obsidian/src/vault-mutation-adapter'); const { createJarvosVaultTransforms } = require('../src/vault-transform-registry'); @@ -54,6 +54,24 @@ test('timeouts and ambiguous CLI failures never prove that Obsidian is stopped', assert.equal(stoppedAdapter.capability().state, 'app_stopped'); }); +test('capability probe times out and contains only its fake CLI process group', () => { + if (process.platform === 'win32') return; + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-')); + const fixture = path.join(root, 'fake-obsidian-cli.js'); + const descendantPid = path.join(root, 'descendant.pid'); + fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); const { spawn } = require('node:child_process'); const child = spawn(process.execPath, ['-e', \"setInterval(() => {}, 1000)\"], { stdio: 'ignore' }); fs.writeFileSync(${JSON.stringify(descendantPid)}, String(child.pid)); setInterval(() => {}, 1000);\n`); + fs.chmodSync(fixture, 0o755); + const started = Date.now(); + try { + assert.throws(() => runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', command: fixture, timeoutMs: 500 }), (error) => error.code === 'ETIMEDOUT'); + assert.ok(Date.now() - started < 3_000); + const pid = Number(fs.readFileSync(descendantPid, 'utf8')); + assert.throws(() => process.kill(pid, 0), { code: 'ESRCH' }); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +}); + test('unavailable capability retains planned intent for reconciliation', () => { const adapter = createVaultMutationAdapter({ vaultRoot: '/vault', vaultId: 'vault-a', ledgerPath: ledgerPath(), probe: () => ({ state: 'app_stopped' }) }); assert.equal(adapter.execute(operation()).status, 'unavailable'); From b730d53f40f9497eb31addc28710d1e062305fdd Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Fri, 28 Aug 2026 22:07:35 -0400 Subject: [PATCH 2/8] test(obsidian): harden probe containment coverage --- .../obsidian/src/obsidian-cli-probe-worker.js | 116 +++++++++--------- .../tests/vault-mutation-adapter.test.js | 20 ++- package.json | 2 +- 3 files changed, 76 insertions(+), 62 deletions(-) diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js index 97df28cc..5f0c1105 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -1,78 +1,74 @@ 'use strict'; -// This helper owns the process group used by a synchronous capability probe. -// Keeping the CLI's stdio inside this process prevents a timed-out descendant -// that inherited its parent's pipes from holding the caller open. const { spawn } = require('node:child_process'); const MAX_CAPTURE_BYTES = 64 * 1024; -const request = JSON.parse(Buffer.from(process.argv[2] || '', 'base64url').toString('utf8')); -const timeoutMs = Math.max(1, Number(request.timeoutMs) || 10_000); -function capture(stream) { - let value = ''; +function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { + const chunks = []; + let byteLength = 0; stream?.on('data', (chunk) => { - if (Buffer.byteLength(value) >= MAX_CAPTURE_BYTES) return; - value += String(chunk).slice(0, MAX_CAPTURE_BYTES - Buffer.byteLength(value)); + if (byteLength >= limit) return; + const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(String(chunk)); + const retained = buffer.subarray(0, Math.min(buffer.length, limit - byteLength)); + chunks.push(retained); + byteLength += retained.length; }); - return () => value; + return Object.freeze({ bytes: () => byteLength, value: () => Buffer.concat(chunks, byteLength).toString('utf8') }); } -function finish(value) { - process.stdout.write(JSON.stringify(value)); -} - -let child; -try { - child = spawn(request.command, request.args, { - detached: process.platform !== 'win32', - stdio: ['ignore', 'pipe', 'pipe'], - windowsHide: true, - }); -} catch (error) { - finish({ ok: false, code: error.code, message: error.message }); - return; -} - -const stdout = capture(child.stdout); -const stderr = capture(child.stderr); -let timedOut = false; -let settled = false; - -function terminateOwnedTree() { +function terminateOwnedTree(child, { platform = process.platform, spawnProcess = spawn, schedule = setTimeout } = {}) { if (!child?.pid) return; - if (process.platform === 'win32') { + const killDirect = () => { try { child.kill('SIGKILL'); } catch {} }; + if (platform === 'win32') { // /t targets the child process tree by PID; it is not a process-name kill. - const killer = spawn('taskkill', ['/pid', String(child.pid), '/t', '/f'], { stdio: 'ignore', windowsHide: true }); - killer.on('error', () => child.kill('SIGKILL')); + let killer; + try { killer = spawnProcess('taskkill', ['/pid', String(child.pid), '/t', '/f'], { stdio: 'ignore', windowsHide: true }); } catch { killDirect(); return; } + if (!killer || typeof killer.once !== 'function') { killDirect(); return; } + let fellBack = false; + const fallback = () => { if (!fellBack) { fellBack = true; killDirect(); } }; + killer.once('error', fallback); + killer.once('close', (code) => { if (code !== 0) fallback(); }); return; } - try { process.kill(-child.pid, 'SIGTERM'); } catch { child.kill('SIGTERM'); } - setTimeout(() => { - try { process.kill(-child.pid, 'SIGKILL'); } catch { child.kill('SIGKILL'); } - }, 100).unref(); + try { process.kill(-child.pid, 'SIGTERM'); } catch { try { child.kill('SIGTERM'); } catch {} } + schedule(() => { try { process.kill(-child.pid, 'SIGKILL'); } catch { killDirect(); } }, 100).unref?.(); } -const timer = setTimeout(() => { - timedOut = true; - terminateOwnedTree(); -}, timeoutMs); - -child.on('error', (error) => { - if (settled) return; - settled = true; - clearTimeout(timer); - finish({ ok: false, code: error.code, message: error.message, stdout: stdout(), stderr: stderr() }); -}); - -child.on('close', (code, signal) => { - if (settled) return; - settled = true; - clearTimeout(timer); - if (timedOut) { - finish({ ok: false, code: 'ETIMEDOUT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms`, stdout: stdout(), stderr: stderr() }); +function runProbe(request, { spawnProcess = spawn, platform = process.platform, output = process.stdout, schedule = setTimeout } = {}) { + const timeoutMs = Math.max(1, Number(request.timeoutMs) || 10_000); + const finish = (value) => output.write(JSON.stringify(value)); + let child; + try { + child = spawnProcess(request.command, request.args, { detached: platform !== 'win32', stdio: ['ignore', 'pipe', 'pipe'], windowsHide: true }); + } catch (error) { + finish({ ok: false, code: error.code, message: error.message }); return; } - if (code === 0) return finish({ ok: true, stdout: stdout(), stderr: stderr() }); - finish({ ok: false, code: code == null ? signal : code, message: stderr() || stdout() || `Obsidian CLI exited with ${code == null ? signal : code}`, stdout: stdout(), stderr: stderr() }); -}); + const stdout = createOutputCapture(child.stdout); + const stderr = createOutputCapture(child.stderr); + let timedOut = false; + let settled = false; + const timer = schedule(() => { timedOut = true; terminateOwnedTree(child, { platform, spawnProcess, schedule }); }, timeoutMs); + child.on('error', (error) => { + if (settled) return; + settled = true; + clearTimeout(timer); + finish({ ok: false, code: error.code, message: error.message, stdout: stdout.value(), stderr: stderr.value() }); + }); + child.on('close', (code, signal) => { + if (settled) return; + settled = true; + clearTimeout(timer); + if (timedOut) return finish({ ok: false, code: 'ETIMEDOUT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms`, stdout: stdout.value(), stderr: stderr.value() }); + if (code === 0) return finish({ ok: true, stdout: stdout.value(), stderr: stderr.value() }); + finish({ ok: false, code: code == null ? signal : code, message: stderr.value() || stdout.value() || `Obsidian CLI exited with ${code == null ? signal : code}`, stdout: stdout.value(), stderr: stderr.value() }); + }); +} + +if (require.main === module) { + const request = JSON.parse(Buffer.from(process.argv[2] || '', 'base64url').toString('utf8')); + runProbe(request); +} + +module.exports = { MAX_CAPTURE_BYTES, createOutputCapture, runProbe, terminateOwnedTree }; diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index b2653e7a..8ff7fe01 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -3,11 +3,13 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); +const { EventEmitter } = require('node:events'); const vm = require('node:vm'); const test = require('node:test'); const { createVaultMutationAdapter, runObsidianEval } = require('../adapters/obsidian/src/vault-mutation-adapter'); const { buildObsidianInvariantProgram, buildObsidianMutationProgram } = require('../adapters/obsidian/src/vault-mutation-adapter'); const { createJarvosVaultTransforms } = require('../src/vault-transform-registry'); +const { MAX_CAPTURE_BYTES, createOutputCapture, terminateOwnedTree } = require('../adapters/obsidian/src/obsidian-cli-probe-worker'); const operation = () => ({ schemaVersion: 1, operationId: 'op-20260806-adapter-test', vaultId: 'vault-a', vaultRelativePath: 'Notes/A.md', sequence: 1, operationKind: 'create', content: 'hello' }); const ledgerPath = () => path.join(os.tmpdir(), `jarvos-adapter-${Math.random()}.json`); @@ -59,7 +61,7 @@ test('capability probe times out and contains only its fake CLI process group', const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-')); const fixture = path.join(root, 'fake-obsidian-cli.js'); const descendantPid = path.join(root, 'descendant.pid'); - fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); const { spawn } = require('node:child_process'); const child = spawn(process.execPath, ['-e', \"setInterval(() => {}, 1000)\"], { stdio: 'ignore' }); fs.writeFileSync(${JSON.stringify(descendantPid)}, String(child.pid)); setInterval(() => {}, 1000);\n`); + fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); const { spawn } = require('node:child_process'); const child = spawn(process.execPath, ['-e', \"setInterval(() => {}, 1000)\"], { stdio: ['ignore', 'inherit', 'inherit'] }); fs.writeFileSync(${JSON.stringify(descendantPid)}, String(child.pid)); setInterval(() => {}, 1000);\n`); fs.chmodSync(fixture, 0o755); const started = Date.now(); try { @@ -72,6 +74,22 @@ test('capability probe times out and contains only its fake CLI process group', } }); +test('probe output capture caps multibyte chunks by bytes', () => { + const stream = new EventEmitter(); + const capture = createOutputCapture(stream); + stream.emit('data', Buffer.from('€'.repeat(MAX_CAPTURE_BYTES))); + assert.equal(capture.bytes(), MAX_CAPTURE_BYTES); +}); + +test('Windows taskkill failure falls back to direct-child termination', () => { + const taskkill = new EventEmitter(); + const signals = []; + const child = { pid: 12345, kill: (signal) => signals.push(signal) }; + terminateOwnedTree(child, { platform: 'win32', spawnProcess: () => taskkill }); + taskkill.emit('close', 1); + assert.deepEqual(signals, ['SIGKILL']); +}); + test('unavailable capability retains planned intent for reconciliation', () => { const adapter = createVaultMutationAdapter({ vaultRoot: '/vault', vaultId: 'vault-a', ledgerPath: ledgerPath(), probe: () => ({ state: 'app_stopped' }) }); assert.equal(adapter.execute(operation()).status, 'unavailable'); diff --git a/package.json b/package.json index 9f3a21bb..673af43e 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ "clawpatch:map": "node scripts/clawpatch-runner.js map", "clawpatch:status": "node scripts/clawpatch-runner.js status", "clawpatch:review:dry-run": "node scripts/clawpatch-runner.js review --limit 1 --dry-run", - "test": "bash tests/smoke-test.sh && node --test modules/jarvos-ontology/test/*.test.js && node --test modules/jarvos-agent-context/test/agent-context.test.js modules/jarvos-agent-context/test/projects-context.test.js && node --test modules/jarvos-control-plane/test/*.test.js && node --test modules/jarvos-coding/test/*.test.js && node --test modules/jarvos-runtime-kit/test/*.test.js && node --test modules/jarvos-skills/test/*.test.js && node --test modules/jarvos-memory/test/transcript-retrieval.test.js && node tests/modules-smoke-test.js && node tests/cli-smoke-test.js && node --test tests/doctor-checks-test.js tests/doctor-modules-test.js && node --test tests/openclaw-plugin-persistence-docs-test.js && node --test tests/pack-manifest-test.js && node --test tests/release-please-config-test.js && node --test tests/release-readiness-check-test.js tests/release-status-test.js && node --test tests/unreleased-drift-check-test.js && node --test tests/public-journal-boundary.test.js tests/active-assistant-runtime-bridge-test.js tests/active-assistant-synthesis-contract-test.js && node --test modules/jarvos-secondbrain/tests/config-resolution.test.js modules/jarvos-secondbrain/tests/journal-lifecycle.test.js modules/jarvos-secondbrain/tests/journal-health.test.js modules/jarvos-secondbrain/tests/journal-maintenance.test.js modules/jarvos-secondbrain/tests/journal-maintenance-schedule.test.js modules/jarvos-secondbrain/tests/journal-maintenance-paths.test.js modules/jarvos-secondbrain/tests/journal-projects-section.test.js modules/jarvos-secondbrain/tests/projects.test.js modules/jarvos-secondbrain/tests/project-context.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/records.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/project-context.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/projects-context.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/provider-contracts.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/migrate.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/journal-projection.test.js modules/jarvos-secondbrain/tests/link-to-journal.test.js modules/jarvos-secondbrain/tests/vault-storage-adapter-journal.test.js modules/jarvos-secondbrain/tests/personality-note-journal-contract.test.js modules/jarvos-secondbrain/tests/journal-backlink-recovery.test.js && node --test tests/secondbrain-external-integrations-doc-test.js && node --test tests/v05-shipgate-test.js && bash scripts/smoke-test.sh", + "test": "bash tests/smoke-test.sh && node --test modules/jarvos-ontology/test/*.test.js && node --test modules/jarvos-agent-context/test/agent-context.test.js modules/jarvos-agent-context/test/projects-context.test.js && node --test modules/jarvos-control-plane/test/*.test.js && node --test modules/jarvos-coding/test/*.test.js && node --test modules/jarvos-runtime-kit/test/*.test.js && node --test modules/jarvos-skills/test/*.test.js && node --test modules/jarvos-memory/test/transcript-retrieval.test.js && node tests/modules-smoke-test.js && node tests/cli-smoke-test.js && node --test tests/doctor-checks-test.js tests/doctor-modules-test.js && node --test tests/openclaw-plugin-persistence-docs-test.js && node --test tests/pack-manifest-test.js && node --test tests/release-please-config-test.js && node --test tests/release-readiness-check-test.js tests/release-status-test.js && node --test tests/unreleased-drift-check-test.js && node --test tests/public-journal-boundary.test.js tests/active-assistant-runtime-bridge-test.js tests/active-assistant-synthesis-contract-test.js && node --test modules/jarvos-secondbrain/tests/config-resolution.test.js modules/jarvos-secondbrain/tests/journal-lifecycle.test.js modules/jarvos-secondbrain/tests/journal-health.test.js modules/jarvos-secondbrain/tests/journal-maintenance.test.js modules/jarvos-secondbrain/tests/journal-maintenance-schedule.test.js modules/jarvos-secondbrain/tests/journal-maintenance-paths.test.js modules/jarvos-secondbrain/tests/journal-projects-section.test.js modules/jarvos-secondbrain/tests/projects.test.js modules/jarvos-secondbrain/tests/project-context.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/records.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/project-context.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/projects-context.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/provider-contracts.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/migrate.test.js modules/jarvos-secondbrain/packages/jarvos-secondbrain-projects/test/journal-projection.test.js modules/jarvos-secondbrain/tests/link-to-journal.test.js modules/jarvos-secondbrain/tests/vault-storage-adapter-journal.test.js modules/jarvos-secondbrain/tests/personality-note-journal-contract.test.js modules/jarvos-secondbrain/tests/journal-backlink-recovery.test.js modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js && node --test tests/secondbrain-external-integrations-doc-test.js && node --test tests/v05-shipgate-test.js && bash scripts/smoke-test.sh", "test:bootstrap": "bash tests/smoke-test.sh", "test:modules": "node tests/modules-smoke-test.js", "test:structure": "bash scripts/smoke-test.sh", From be4489b07378e948638111e9151697df7aed60c0 Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Fri, 28 Aug 2026 22:12:52 -0400 Subject: [PATCH 3/8] fix(obsidian): wait for owned probe containment --- .../obsidian/src/obsidian-cli-probe-worker.js | 54 ++++++++++++++----- .../tests/vault-mutation-adapter.test.js | 35 +++++++++--- 2 files changed, 68 insertions(+), 21 deletions(-) diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js index 5f0c1105..f45514a6 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -17,22 +17,35 @@ function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { return Object.freeze({ bytes: () => byteLength, value: () => Buffer.concat(chunks, byteLength).toString('utf8') }); } -function terminateOwnedTree(child, { platform = process.platform, spawnProcess = spawn, schedule = setTimeout } = {}) { - if (!child?.pid) return; - const killDirect = () => { try { child.kill('SIGKILL'); } catch {} }; +function terminateOwnedTree(child, { platform = process.platform, spawnProcess = spawn, schedule = setTimeout, signalProcess = process.kill, onComplete = () => {} } = {}) { + if (!child?.pid) { onComplete({ contained: false, reason: 'missing_child_pid' }); return; } + let completed = false; + const complete = (result) => { if (!completed) { completed = true; onComplete(result); } }; if (platform === 'win32') { // /t targets the child process tree by PID; it is not a process-name kill. - let killer; - try { killer = spawnProcess('taskkill', ['/pid', String(child.pid), '/t', '/f'], { stdio: 'ignore', windowsHide: true }); } catch { killDirect(); return; } - if (!killer || typeof killer.once !== 'function') { killDirect(); return; } - let fellBack = false; - const fallback = () => { if (!fellBack) { fellBack = true; killDirect(); } }; - killer.once('error', fallback); - killer.once('close', (code) => { if (code !== 0) fallback(); }); + const taskkillArgs = ['/pid', String(child.pid), '/t', '/f']; + const runTaskkill = (attempt) => { + let killer; + try { killer = spawnProcess('taskkill', taskkillArgs, { stdio: 'ignore', windowsHide: true }); } catch { complete({ contained: false, reason: 'taskkill_spawn_failed' }); return; } + if (!killer || typeof killer.once !== 'function') { complete({ contained: false, reason: 'taskkill_unavailable' }); return; } + killer.once('error', () => complete({ contained: false, reason: 'taskkill_failed' })); + killer.once('close', (code) => { + if (code === 0) complete({ contained: true }); + else if (attempt === 0) runTaskkill(1); + else complete({ contained: false, reason: 'taskkill_failed' }); + }); + }; + runTaskkill(0); return; } - try { process.kill(-child.pid, 'SIGTERM'); } catch { try { child.kill('SIGTERM'); } catch {} } - schedule(() => { try { process.kill(-child.pid, 'SIGKILL'); } catch { killDirect(); } }, 100).unref?.(); + try { signalProcess(-child.pid, 'SIGTERM'); } + catch (error) { complete({ contained: error?.code === 'ESRCH', reason: error?.code === 'ESRCH' ? undefined : 'process_group_unavailable' }); return; } + // Keep the worker alive through escalation even if the direct child exits + // after SIGTERM and leaves no inherited pipes open. + schedule(() => { + try { signalProcess(-child.pid, 'SIGKILL'); complete({ contained: true }); } + catch (error) { complete({ contained: error?.code === 'ESRCH', reason: error?.code === 'ESRCH' ? undefined : 'process_group_kill_failed' }); } + }, 100); } function runProbe(request, { spawnProcess = spawn, platform = process.platform, output = process.stdout, schedule = setTimeout } = {}) { @@ -49,18 +62,31 @@ function runProbe(request, { spawnProcess = spawn, platform = process.platform, const stderr = createOutputCapture(child.stderr); let timedOut = false; let settled = false; - const timer = schedule(() => { timedOut = true; terminateOwnedTree(child, { platform, spawnProcess, schedule }); }, timeoutMs); + const finishTimeout = (containment) => { + if (settled) return; + settled = true; + clearTimeout(timer); + if (containment.contained) return finish({ ok: false, code: 'ETIMEDOUT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms`, stdout: stdout.value(), stderr: stderr.value() }); + finish({ ok: false, code: 'ECONTAINMENT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms and owned process-tree containment could not be confirmed (${containment.reason || 'unknown'})`, stdout: stdout.value(), stderr: stderr.value() }); + }; + const timer = schedule(() => { + timedOut = true; + terminateOwnedTree(child, { platform, spawnProcess, schedule, onComplete: finishTimeout }); + }, timeoutMs); child.on('error', (error) => { if (settled) return; + if (timedOut) return; settled = true; clearTimeout(timer); finish({ ok: false, code: error.code, message: error.message, stdout: stdout.value(), stderr: stderr.value() }); }); child.on('close', (code, signal) => { if (settled) return; + // A timeout has begun owned-tree containment. The direct child may close + // before same-group descendants receive the SIGKILL escalation. + if (timedOut) return; settled = true; clearTimeout(timer); - if (timedOut) return finish({ ok: false, code: 'ETIMEDOUT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms`, stdout: stdout.value(), stderr: stderr.value() }); if (code === 0) return finish({ ok: true, stdout: stdout.value(), stderr: stderr.value() }); finish({ ok: false, code: code == null ? signal : code, message: stderr.value() || stdout.value() || `Obsidian CLI exited with ${code == null ? signal : code}`, stdout: stdout.value(), stderr: stderr.value() }); }); diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index 8ff7fe01..2fda7953 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -56,16 +56,17 @@ test('timeouts and ambiguous CLI failures never prove that Obsidian is stopped', assert.equal(stoppedAdapter.capability().state, 'app_stopped'); }); -test('capability probe times out and contains only its fake CLI process group', () => { +test('capability probe escalates after its direct child exits and contains a SIGTERM-resistant descendant', () => { if (process.platform === 'win32') return; const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-')); const fixture = path.join(root, 'fake-obsidian-cli.js'); const descendantPid = path.join(root, 'descendant.pid'); - fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); const { spawn } = require('node:child_process'); const child = spawn(process.execPath, ['-e', \"setInterval(() => {}, 1000)\"], { stdio: ['ignore', 'inherit', 'inherit'] }); fs.writeFileSync(${JSON.stringify(descendantPid)}, String(child.pid)); setInterval(() => {}, 1000);\n`); + fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); const { spawn } = require('node:child_process'); const child = spawn(process.execPath, ['-e', \"process.on('SIGTERM', () => {}); setInterval(() => {}, 1000)\"], { stdio: 'ignore' }); fs.writeFileSync(${JSON.stringify(descendantPid)}, String(child.pid)); setInterval(() => {}, 1000);\n`); fs.chmodSync(fixture, 0o755); const started = Date.now(); try { assert.throws(() => runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', command: fixture, timeoutMs: 500 }), (error) => error.code === 'ETIMEDOUT'); + assert.ok(Date.now() - started >= 550); assert.ok(Date.now() - started < 3_000); const pid = Number(fs.readFileSync(descendantPid, 'utf8')); assert.throws(() => process.kill(pid, 0), { code: 'ESRCH' }); @@ -81,13 +82,33 @@ test('probe output capture caps multibyte chunks by bytes', () => { assert.equal(capture.bytes(), MAX_CAPTURE_BYTES); }); -test('Windows taskkill failure falls back to direct-child termination', () => { - const taskkill = new EventEmitter(); +test('Windows taskkill retries process-tree containment and reports success only from taskkill', () => { + const firstTaskkill = new EventEmitter(); + const secondTaskkill = new EventEmitter(); const signals = []; + const results = []; const child = { pid: 12345, kill: (signal) => signals.push(signal) }; - terminateOwnedTree(child, { platform: 'win32', spawnProcess: () => taskkill }); - taskkill.emit('close', 1); - assert.deepEqual(signals, ['SIGKILL']); + let calls = 0; + terminateOwnedTree(child, { platform: 'win32', spawnProcess: () => [firstTaskkill, secondTaskkill][calls++], onComplete: (result) => results.push(result) }); + firstTaskkill.emit('close', 1); + secondTaskkill.emit('close', 0); + assert.equal(calls, 2); + assert.deepEqual(signals, []); + assert.deepEqual(results, [{ contained: true }]); +}); + +test('Windows taskkill double failure reports unconfirmed containment without direct-child fallback', () => { + const firstTaskkill = new EventEmitter(); + const secondTaskkill = new EventEmitter(); + const signals = []; + const results = []; + const child = { pid: 12345, kill: (signal) => signals.push(signal) }; + let calls = 0; + terminateOwnedTree(child, { platform: 'win32', spawnProcess: () => [firstTaskkill, secondTaskkill][calls++], onComplete: (result) => results.push(result) }); + firstTaskkill.emit('close', 1); + secondTaskkill.emit('close', 1); + assert.deepEqual(signals, []); + assert.deepEqual(results, [{ contained: false, reason: 'taskkill_failed' }]); }); test('unavailable capability retains planned intent for reconciliation', () => { From 9d67b22df82fb14f500b6ed00785b40c131a415a Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Sat, 29 Aug 2026 21:05:08 -0400 Subject: [PATCH 4/8] fix(obsidian): fail closed on missing process group --- .../obsidian/src/obsidian-cli-probe-worker.js | 4 +-- .../tests/vault-mutation-adapter.test.js | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js index f45514a6..939cf41e 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -39,12 +39,12 @@ function terminateOwnedTree(child, { platform = process.platform, spawnProcess = return; } try { signalProcess(-child.pid, 'SIGTERM'); } - catch (error) { complete({ contained: error?.code === 'ESRCH', reason: error?.code === 'ESRCH' ? undefined : 'process_group_unavailable' }); return; } + catch (error) { complete({ contained: false, reason: error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_unavailable' }); return; } // Keep the worker alive through escalation even if the direct child exits // after SIGTERM and leaves no inherited pipes open. schedule(() => { try { signalProcess(-child.pid, 'SIGKILL'); complete({ contained: true }); } - catch (error) { complete({ contained: error?.code === 'ESRCH', reason: error?.code === 'ESRCH' ? undefined : 'process_group_kill_failed' }); } + catch (error) { complete({ contained: false, reason: error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_kill_failed' }); } }, 100); } diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index 2fda7953..13743aec 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -111,6 +111,34 @@ test('Windows taskkill double failure reports unconfirmed containment without di assert.deepEqual(results, [{ contained: false, reason: 'taskkill_failed' }]); }); +test('missing Unix process group does not prove descendant containment', () => { + const results = []; + const error = Object.assign(new Error('process group missing'), { code: 'ESRCH' }); + terminateOwnedTree({ pid: 12345 }, { + platform: 'darwin', + signalProcess: () => { throw error; }, + onComplete: (result) => results.push(result), + }); + assert.deepEqual(results, [{ contained: false, reason: 'process_group_absent' }]); +}); + +test('missing Unix process group during escalation does not prove descendant containment', () => { + const signals = []; + const results = []; + const error = Object.assign(new Error('process group missing'), { code: 'ESRCH' }); + terminateOwnedTree({ pid: 12345 }, { + platform: 'darwin', + signalProcess: (pid, signal) => { + signals.push([pid, signal]); + if (signal === 'SIGKILL') throw error; + }, + schedule: (callback) => callback(), + onComplete: (result) => results.push(result), + }); + assert.deepEqual(signals, [[-12345, 'SIGTERM'], [-12345, 'SIGKILL']]); + assert.deepEqual(results, [{ contained: false, reason: 'process_group_absent' }]); +}); + test('unavailable capability retains planned intent for reconciliation', () => { const adapter = createVaultMutationAdapter({ vaultRoot: '/vault', vaultId: 'vault-a', ledgerPath: ledgerPath(), probe: () => ({ state: 'app_stopped' }) }); assert.equal(adapter.execute(operation()).status, 'unavailable'); From 30678f1fafd97adee2ae8dd3ecf86fd198f9dcc7 Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Sat, 29 Aug 2026 21:12:41 -0400 Subject: [PATCH 5/8] fix(obsidian): stop child on containment failure --- .../obsidian/src/obsidian-cli-probe-worker.js | 19 +++++++++++++------ .../tests/vault-mutation-adapter.test.js | 12 ++++++++---- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js index 939cf41e..0c0383a6 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -21,30 +21,37 @@ function terminateOwnedTree(child, { platform = process.platform, spawnProcess = if (!child?.pid) { onComplete({ contained: false, reason: 'missing_child_pid' }); return; } let completed = false; const complete = (result) => { if (!completed) { completed = true; onComplete(result); } }; + const stopKnownChild = () => { + try { child.kill?.('SIGKILL'); } catch {} + for (const stream of [child.stdin, child.stdout, child.stderr]) { + try { stream?.destroy?.(); } catch {} + } + }; + const failContainment = (reason) => { stopKnownChild(); complete({ contained: false, reason }); }; if (platform === 'win32') { // /t targets the child process tree by PID; it is not a process-name kill. const taskkillArgs = ['/pid', String(child.pid), '/t', '/f']; const runTaskkill = (attempt) => { let killer; - try { killer = spawnProcess('taskkill', taskkillArgs, { stdio: 'ignore', windowsHide: true }); } catch { complete({ contained: false, reason: 'taskkill_spawn_failed' }); return; } - if (!killer || typeof killer.once !== 'function') { complete({ contained: false, reason: 'taskkill_unavailable' }); return; } - killer.once('error', () => complete({ contained: false, reason: 'taskkill_failed' })); + try { killer = spawnProcess('taskkill', taskkillArgs, { stdio: 'ignore', windowsHide: true }); } catch { failContainment('taskkill_spawn_failed'); return; } + if (!killer || typeof killer.once !== 'function') { failContainment('taskkill_unavailable'); return; } + killer.once('error', () => failContainment('taskkill_failed')); killer.once('close', (code) => { if (code === 0) complete({ contained: true }); else if (attempt === 0) runTaskkill(1); - else complete({ contained: false, reason: 'taskkill_failed' }); + else failContainment('taskkill_failed'); }); }; runTaskkill(0); return; } try { signalProcess(-child.pid, 'SIGTERM'); } - catch (error) { complete({ contained: false, reason: error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_unavailable' }); return; } + catch (error) { failContainment(error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_unavailable'); return; } // Keep the worker alive through escalation even if the direct child exits // after SIGTERM and leaves no inherited pipes open. schedule(() => { try { signalProcess(-child.pid, 'SIGKILL'); complete({ contained: true }); } - catch (error) { complete({ contained: false, reason: error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_kill_failed' }); } + catch (error) { failContainment(error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_kill_failed'); } }, 100); } diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index 13743aec..f87d4771 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -97,7 +97,7 @@ test('Windows taskkill retries process-tree containment and reports success only assert.deepEqual(results, [{ contained: true }]); }); -test('Windows taskkill double failure reports unconfirmed containment without direct-child fallback', () => { +test('Windows taskkill double failure stops the known child and reports unconfirmed containment', () => { const firstTaskkill = new EventEmitter(); const secondTaskkill = new EventEmitter(); const signals = []; @@ -107,26 +107,29 @@ test('Windows taskkill double failure reports unconfirmed containment without di terminateOwnedTree(child, { platform: 'win32', spawnProcess: () => [firstTaskkill, secondTaskkill][calls++], onComplete: (result) => results.push(result) }); firstTaskkill.emit('close', 1); secondTaskkill.emit('close', 1); - assert.deepEqual(signals, []); + assert.deepEqual(signals, ['SIGKILL']); assert.deepEqual(results, [{ contained: false, reason: 'taskkill_failed' }]); }); test('missing Unix process group does not prove descendant containment', () => { const results = []; + const signals = []; const error = Object.assign(new Error('process group missing'), { code: 'ESRCH' }); - terminateOwnedTree({ pid: 12345 }, { + terminateOwnedTree({ pid: 12345, kill: (signal) => signals.push(signal) }, { platform: 'darwin', signalProcess: () => { throw error; }, onComplete: (result) => results.push(result), }); + assert.deepEqual(signals, ['SIGKILL']); assert.deepEqual(results, [{ contained: false, reason: 'process_group_absent' }]); }); test('missing Unix process group during escalation does not prove descendant containment', () => { const signals = []; + const childSignals = []; const results = []; const error = Object.assign(new Error('process group missing'), { code: 'ESRCH' }); - terminateOwnedTree({ pid: 12345 }, { + terminateOwnedTree({ pid: 12345, kill: (signal) => childSignals.push(signal) }, { platform: 'darwin', signalProcess: (pid, signal) => { signals.push([pid, signal]); @@ -136,6 +139,7 @@ test('missing Unix process group during escalation does not prove descendant con onComplete: (result) => results.push(result), }); assert.deepEqual(signals, [[-12345, 'SIGTERM'], [-12345, 'SIGKILL']]); + assert.deepEqual(childSignals, ['SIGKILL']); assert.deepEqual(results, [{ contained: false, reason: 'process_group_absent' }]); }); From 5048426aa7ea188b34e73f9f4204bfe9767b9151 Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Sun, 30 Aug 2026 13:13:01 -0400 Subject: [PATCH 6/8] fix(obsidian): preserve bounded probe compatibility --- .../obsidian/src/obsidian-cli-probe-worker.js | 64 +++++++++---- .../obsidian/src/vault-mutation-adapter.js | 11 ++- .../tests/vault-mutation-adapter.test.js | 91 ++++++++++++++++++- 3 files changed, 143 insertions(+), 23 deletions(-) diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js index 0c0383a6..e81da142 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -1,8 +1,10 @@ 'use strict'; const { spawn } = require('node:child_process'); +const fs = require('node:fs'); const MAX_CAPTURE_BYTES = 64 * 1024; +const TASKKILL_TIMEOUT_MS = 250; function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { const chunks = []; @@ -17,7 +19,14 @@ function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { return Object.freeze({ bytes: () => byteLength, value: () => Buffer.concat(chunks, byteLength).toString('utf8') }); } -function terminateOwnedTree(child, { platform = process.platform, spawnProcess = spawn, schedule = setTimeout, signalProcess = process.kill, onComplete = () => {} } = {}) { +function terminateOwnedTree(child, { + platform = process.platform, + spawnProcess = spawn, + schedule = setTimeout, + signalProcess = process.kill, + onComplete = () => {}, + taskkillTimeoutMs = TASKKILL_TIMEOUT_MS, +} = {}) { if (!child?.pid) { onComplete({ contained: false, reason: 'missing_child_pid' }); return; } let completed = false; const complete = (result) => { if (!completed) { completed = true; onComplete(result); } }; @@ -33,10 +42,18 @@ function terminateOwnedTree(child, { platform = process.platform, spawnProcess = const taskkillArgs = ['/pid', String(child.pid), '/t', '/f']; const runTaskkill = (attempt) => { let killer; + let watchdog; + let attemptSettled = false; + const clearWatchdog = () => { if (watchdog) { try { clearTimeout(watchdog); } catch {} watchdog = undefined; } }; + const stopKiller = () => { try { killer?.kill?.('SIGKILL'); } catch {} }; + const settleAttempt = () => { if (attemptSettled) return false; attemptSettled = true; clearWatchdog(); return true; }; try { killer = spawnProcess('taskkill', taskkillArgs, { stdio: 'ignore', windowsHide: true }); } catch { failContainment('taskkill_spawn_failed'); return; } if (!killer || typeof killer.once !== 'function') { failContainment('taskkill_unavailable'); return; } - killer.once('error', () => failContainment('taskkill_failed')); + watchdog = schedule(() => { if (!settleAttempt()) return; stopKiller(); failContainment('taskkill_timeout'); }, Math.max(1, Number(taskkillTimeoutMs) || TASKKILL_TIMEOUT_MS)); + watchdog?.unref?.(); + killer.once('error', () => { if (!settleAttempt()) return; failContainment('taskkill_failed'); }); killer.once('close', (code) => { + if (!settleAttempt()) return; if (code === 0) complete({ contained: true }); else if (attempt === 0) runTaskkill(1); else failContainment('taskkill_failed'); @@ -45,17 +62,26 @@ function terminateOwnedTree(child, { platform = process.platform, spawnProcess = runTaskkill(0); return; } - try { signalProcess(-child.pid, 'SIGTERM'); } - catch (error) { failContainment(error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_unavailable'); return; } - // Keep the worker alive through escalation even if the direct child exits - // after SIGTERM and leaves no inherited pipes open. - schedule(() => { - try { signalProcess(-child.pid, 'SIGKILL'); complete({ contained: true }); } - catch (error) { failContainment(error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_kill_failed'); } - }, 100); + // The dedicated group is the containment boundary. A direct SIGKILL avoids + // graceful handlers and the fork window they can open; success means only + // that the owned process group accepted the signal, not that arbitrary + // processes which detached before the timeout were in that group. + try { + signalProcess(-child.pid, 'SIGKILL'); + complete({ contained: true }); + } + catch (error) { + failContainment(error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_kill_failed'); + } } -function runProbe(request, { spawnProcess = spawn, platform = process.platform, output = process.stdout, schedule = setTimeout } = {}) { +function runProbe(request, { + spawnProcess = spawn, + platform = process.platform, + output = process.stdout, + schedule = setTimeout, + taskkillTimeoutMs = TASKKILL_TIMEOUT_MS, +} = {}) { const timeoutMs = Math.max(1, Number(request.timeoutMs) || 10_000); const finish = (value) => output.write(JSON.stringify(value)); let child; @@ -74,11 +100,11 @@ function runProbe(request, { spawnProcess = spawn, platform = process.platform, settled = true; clearTimeout(timer); if (containment.contained) return finish({ ok: false, code: 'ETIMEDOUT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms`, stdout: stdout.value(), stderr: stderr.value() }); - finish({ ok: false, code: 'ECONTAINMENT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms and owned process-tree containment could not be confirmed (${containment.reason || 'unknown'})`, stdout: stdout.value(), stderr: stderr.value() }); + finish({ ok: false, code: 'ECONTAINMENT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms and dedicated process-group containment could not be confirmed (${containment.reason || 'unknown'})`, stdout: stdout.value(), stderr: stderr.value() }); }; const timer = schedule(() => { timedOut = true; - terminateOwnedTree(child, { platform, spawnProcess, schedule, onComplete: finishTimeout }); + terminateOwnedTree(child, { platform, spawnProcess, schedule, taskkillTimeoutMs, onComplete: finishTimeout }); }, timeoutMs); child.on('error', (error) => { if (settled) return; @@ -89,8 +115,8 @@ function runProbe(request, { spawnProcess = spawn, platform = process.platform, }); child.on('close', (code, signal) => { if (settled) return; - // A timeout has begun owned-tree containment. The direct child may close - // before same-group descendants receive the SIGKILL escalation. + // A timeout has begun group containment. The direct child may close + // after the group signal; the timeout callback owns final classification. if (timedOut) return; settled = true; clearTimeout(timer); @@ -100,7 +126,13 @@ function runProbe(request, { spawnProcess = spawn, platform = process.platform, } if (require.main === module) { - const request = JSON.parse(Buffer.from(process.argv[2] || '', 'base64url').toString('utf8')); + // The legacy base64url argument remains accepted for direct invocations. + // The adapter uses stdin so large CLI programs are not copied into a second + // oversized argument merely to reach this worker. + const encoded = process.argv[2]; + const request = encoded + ? JSON.parse(Buffer.from(encoded, 'base64url').toString('utf8')) + : JSON.parse(fs.readFileSync(0, 'utf8')); runProbe(request); } diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js b/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js index e879103f..29d00361 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js @@ -21,11 +21,16 @@ function runObsidianEval(code, { vaultName, command = process.env.OBSIDIAN_CLI | try { // Test seams that supply their own executor retain the old direct contract. if (execute !== execFileSync) return parseEvalResult(execute(command, args, { encoding: 'utf8', timeout: timeoutMs, stdio: ['ignore', 'pipe', 'pipe'] })); - const request = Buffer.from(JSON.stringify({ command, args, timeoutMs }), 'utf8').toString('base64url'); - const response = JSON.parse(execFileSync(process.execPath, [OBSIDIAN_CLI_PROBE_WORKER, request], { + // Keep the worker envelope off argv. The underlying CLI still receives + // the exact same args (including large mutation programs), while the + // worker request itself travels through stdin and does not add base64 + // expansion to the platform's per-argument limit. + const request = JSON.stringify({ command, args, timeoutMs }); + const response = JSON.parse(execFileSync(process.execPath, [OBSIDIAN_CLI_PROBE_WORKER], { encoding: 'utf8', + input: request, timeout: timeoutMs + 1_000, - stdio: ['ignore', 'pipe', 'pipe'], + stdio: ['pipe', 'pipe', 'pipe'], })); if (!response.ok) { const error = new Error(String(response.stderr || response.stdout || response.message || 'Obsidian CLI failed')); diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index f87d4771..c8ff4cfd 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -56,7 +56,7 @@ test('timeouts and ambiguous CLI failures never prove that Obsidian is stopped', assert.equal(stoppedAdapter.capability().state, 'app_stopped'); }); -test('capability probe escalates after its direct child exits and contains a SIGTERM-resistant descendant', () => { +test('capability probe kills a same-process-group descendant', () => { if (process.platform === 'win32') return; const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-')); const fixture = path.join(root, 'fake-obsidian-cli.js'); @@ -66,7 +66,7 @@ test('capability probe escalates after its direct child exits and contains a SIG const started = Date.now(); try { assert.throws(() => runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', command: fixture, timeoutMs: 500 }), (error) => error.code === 'ETIMEDOUT'); - assert.ok(Date.now() - started >= 550); + assert.ok(Date.now() - started >= 500); assert.ok(Date.now() - started < 3_000); const pid = Number(fs.readFileSync(descendantPid, 'utf8')); assert.throws(() => process.kill(pid, 0), { code: 'ESRCH' }); @@ -75,6 +75,54 @@ test('capability probe escalates after its direct child exits and contains a SIG } }); +test('an ordinary timeout preserves ETIMEDOUT and leaves no child', () => { + if (process.platform === 'win32') return; + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-normal-')); + const fixture = path.join(root, 'fake-obsidian-cli.js'); + const childPid = path.join(root, 'child.pid'); + const termMarker = path.join(root, 'sigterm.marker'); + fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); fs.writeFileSync(${JSON.stringify(childPid)}, String(process.pid)); process.on('SIGTERM', () => fs.writeFileSync(${JSON.stringify(termMarker)}, 'unexpected')); setInterval(() => {}, 1000);\n`); + fs.chmodSync(fixture, 0o755); + try { + assert.throws(() => runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', command: fixture, timeoutMs: 300 }), (error) => error.code === 'ETIMEDOUT'); + const pid = Number(fs.readFileSync(childPid, 'utf8')); + assert.throws(() => process.kill(pid, 0), { code: 'ESRCH' }); + assert.equal(fs.existsSync(termMarker), false); + } finally { + try { const pid = Number(fs.readFileSync(childPid, 'utf8')); process.kill(pid, 'SIGKILL'); } catch {} + fs.rmSync(root, { recursive: true, force: true }); + } +}); + +test('direct group SIGKILL prevents a SIGTERM handler from creating work', () => { + if (process.platform === 'win32') return; + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-term-handler-')); + const fixture = path.join(root, 'fake-obsidian-cli.js'); + const descendantPid = path.join(root, 'descendant.pid'); + const termMarker = path.join(root, 'sigterm.marker'); + fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); const { spawn } = require('node:child_process'); process.on('SIGTERM', () => { fs.writeFileSync(${JSON.stringify(termMarker)}, 'unexpected'); const child = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { detached: true, stdio: 'ignore' }); fs.writeFileSync(${JSON.stringify(descendantPid)}, String(child.pid)); child.unref(); }); setInterval(() => {}, 1000);\n`); + fs.chmodSync(fixture, 0o755); + try { + assert.throws(() => runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', command: fixture, timeoutMs: 300 }), (error) => error.code === 'ETIMEDOUT'); + assert.equal(fs.existsSync(termMarker), false); + assert.equal(fs.existsSync(descendantPid), false); + } finally { + try { const pid = Number(fs.readFileSync(descendantPid, 'utf8')); process.kill(pid, 'SIGKILL'); } catch {} + fs.rmSync(root, { recursive: true, force: true }); + } +}); + +test('large CLI programs are passed through the worker without envelope argv expansion', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-large-')); + const fixture = path.join(root, 'fake-obsidian-cli.js'); + fs.writeFileSync(fixture, `#!${process.execPath}\nprocess.stdout.write('=> ' + JSON.stringify({ length: process.argv[4]?.length || 0 }) + '\\n');\n`); + fs.chmodSync(fixture, 0o755); + const code = 'x'.repeat(70 * 1024); + try { + assert.deepEqual(runObsidianEval(code, { vaultName: 'fake-vault', command: fixture, timeoutMs: 1_000 }), { length: code.length + 'code='.length }); + } finally { fs.rmSync(root, { recursive: true, force: true }); } +}); + test('probe output capture caps multibyte chunks by bytes', () => { const stream = new EventEmitter(); const capture = createOutputCapture(stream); @@ -111,6 +159,41 @@ test('Windows taskkill double failure stops the known child and reports unconfir assert.deepEqual(results, [{ contained: false, reason: 'taskkill_failed' }]); }); +test('Windows taskkill watchdog fails closed when the helper does not finish', () => { + const killer = new EventEmitter(); + const killerSignals = []; + killer.kill = (signal) => killerSignals.push(signal); + const childSignals = []; + const results = []; + const timers = []; + terminateOwnedTree({ pid: 12345, kill: (signal) => childSignals.push(signal) }, { + platform: 'win32', + spawnProcess: () => killer, + schedule: (callback, delay) => { const timer = { callback, delay, unref() {} }; timers.push(timer); return timer; }, + onComplete: (result) => results.push(result), + }); + assert.equal(timers.length, 1); + assert.equal(timers[0].delay, 250); + timers[0].callback(); + assert.deepEqual(killerSignals, ['SIGKILL']); + assert.deepEqual(childSignals, ['SIGKILL']); + assert.deepEqual(results, [{ contained: false, reason: 'taskkill_timeout' }]); + killer.emit('close', 0); + assert.deepEqual(results, [{ contained: false, reason: 'taskkill_timeout' }]); +}); + +test('an accepted POSIX group SIGKILL reports the existing timeout contract', () => { + const signals = []; + const results = []; + terminateOwnedTree({ pid: 12345, kill: () => {} }, { + platform: 'darwin', + signalProcess: (pid, signal) => signals.push([pid, signal]), + onComplete: (result) => results.push(result), + }); + assert.deepEqual(signals, [[-12345, 'SIGKILL']]); + assert.deepEqual(results, [{ contained: true }]); +}); + test('missing Unix process group does not prove descendant containment', () => { const results = []; const signals = []; @@ -124,7 +207,7 @@ test('missing Unix process group does not prove descendant containment', () => { assert.deepEqual(results, [{ contained: false, reason: 'process_group_absent' }]); }); -test('missing Unix process group during escalation does not prove descendant containment', () => { +test('a failed POSIX group SIGKILL does not prove descendant containment', () => { const signals = []; const childSignals = []; const results = []; @@ -138,7 +221,7 @@ test('missing Unix process group during escalation does not prove descendant con schedule: (callback) => callback(), onComplete: (result) => results.push(result), }); - assert.deepEqual(signals, [[-12345, 'SIGTERM'], [-12345, 'SIGKILL']]); + assert.deepEqual(signals, [[-12345, 'SIGKILL']]); assert.deepEqual(childSignals, ['SIGKILL']); assert.deepEqual(results, [{ contained: false, reason: 'process_group_absent' }]); }); From 679f8229cf0ffe251cd3b448a4480a3e170c83d4 Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Mon, 31 Aug 2026 22:18:06 -0400 Subject: [PATCH 7/8] fix(obsidian): preserve terminal probe results --- .../obsidian/src/obsidian-cli-probe-worker.js | 36 +++++++++++++++---- .../tests/vault-mutation-adapter.test.js | 11 ++++++ 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js index e81da142..7b73d38f 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -7,16 +7,38 @@ const MAX_CAPTURE_BYTES = 64 * 1024; const TASKKILL_TIMEOUT_MS = 250; function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { - const chunks = []; - let byteLength = 0; + const boundedLimit = Math.max(0, Number(limit) || 0); + const headLimit = Math.ceil(boundedLimit / 2); + const tailLimit = boundedLimit - headLimit; + const headChunks = []; + let headByteLength = 0; + let tail = Buffer.alloc(0); + const appendTail = (buffer) => { + if (!tailLimit || !buffer.length) return; + const retained = buffer.length > tailLimit ? buffer.subarray(buffer.length - tailLimit) : buffer; + const combined = Buffer.concat([tail, retained]); + tail = combined.length > tailLimit + ? Buffer.from(combined.subarray(combined.length - tailLimit)) + : combined; + }; stream?.on('data', (chunk) => { - if (byteLength >= limit) return; const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(String(chunk)); - const retained = buffer.subarray(0, Math.min(buffer.length, limit - byteLength)); - chunks.push(retained); - byteLength += retained.length; + let offset = 0; + if (headByteLength < headLimit) { + const retainedLength = Math.min(buffer.length, headLimit - headByteLength); + headChunks.push(Buffer.from(buffer.subarray(0, retainedLength))); + headByteLength += retainedLength; + offset = retainedLength; + } + // Obsidian writes the eval result as a terminal `=> ...` line. Keep a + // bounded tail as well as the leading diagnostics so verbose output does + // not hide a valid result and make a healthy CLI look incompatible. + if (offset < buffer.length) appendTail(buffer.subarray(offset)); + }); + return Object.freeze({ + bytes: () => headByteLength + tail.length, + value: () => Buffer.concat([...headChunks, tail], headByteLength + tail.length).toString('utf8'), }); - return Object.freeze({ bytes: () => byteLength, value: () => Buffer.concat(chunks, byteLength).toString('utf8') }); } function terminateOwnedTree(child, { diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index c8ff4cfd..e5f659ca 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -123,6 +123,17 @@ test('large CLI programs are passed through the worker without envelope argv exp } finally { fs.rmSync(root, { recursive: true, force: true }); } }); +test('bounded probe output preserves a terminal eval result after more than 64 KiB of noise', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-output-')); + const fixture = path.join(root, 'fake-obsidian-cli.js'); + const result = { vaultName: 'fake-vault', hasVault: true }; + fs.writeFileSync(fixture, `#!${process.execPath}\nprocess.stdout.write('x'.repeat(${MAX_CAPTURE_BYTES + 1024}) + '\\n=> ' + ${JSON.stringify(JSON.stringify(result))} + '\\n');\n`); + fs.chmodSync(fixture, 0o755); + try { + assert.deepEqual(runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', command: fixture, timeoutMs: 1_000 }), result); + } finally { fs.rmSync(root, { recursive: true, force: true }); } +}); + test('probe output capture caps multibyte chunks by bytes', () => { const stream = new EventEmitter(); const capture = createOutputCapture(stream); From fc9a7f2a8457cb6550e9778785ac20d988dde4ad Mon Sep 17 00:00:00 2001 From: Andrew Levine Date: Mon, 31 Aug 2026 23:00:28 -0400 Subject: [PATCH 8/8] fix(obsidian): contain detached probe descendants --- .../obsidian/src/obsidian-cli-probe-worker.js | 160 ++++++++++++++++-- .../obsidian/src/vault-mutation-adapter.js | 8 +- .../tests/vault-mutation-adapter.test.js | 46 ++++- 3 files changed, 193 insertions(+), 21 deletions(-) diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js index 7b73d38f..a492943c 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/obsidian-cli-probe-worker.js @@ -1,20 +1,81 @@ 'use strict'; -const { spawn } = require('node:child_process'); +const { execFileSync, spawn } = require('node:child_process'); const fs = require('node:fs'); const MAX_CAPTURE_BYTES = 64 * 1024; +const DEFAULT_PROBE_TIMEOUT_MS = 10_000; +const PROBE_CLEANUP_GRACE_MS = 1_000; +const MAX_PROBE_TIMEOUT_MS = 2_147_483_647 - PROBE_CLEANUP_GRACE_MS; +const PROCESS_SCAN_INTERVAL_MS = 25; +const PROCESS_SNAPSHOT_TIMEOUT_MS = 50; +const MAX_PROCESS_SNAPSHOT_CALLS = 512; const TASKKILL_TIMEOUT_MS = 250; +const TRUNCATION_SEPARATOR = Buffer.from('\n[... output truncated ...]\n', 'utf8'); + +function normalizeProbeTimeoutMs(value, fallback = DEFAULT_PROBE_TIMEOUT_MS) { + if (value === undefined || value === null) return fallback; + const numeric = typeof value === 'number' + ? value + : typeof value === 'string' && value.trim() + ? Number(value) + : NaN; + if (!Number.isFinite(numeric) || numeric <= 0 || numeric > MAX_PROBE_TIMEOUT_MS) throw new RangeError(`timeoutMs must be a finite number between 1 and ${MAX_PROBE_TIMEOUT_MS}`); + return Math.max(1, Math.floor(numeric)); +} + +function readProcessSnapshot() { + return execFileSync('ps', ['-axo', 'pid=,ppid='], { + encoding: 'utf8', + timeout: PROCESS_SNAPSHOT_TIMEOUT_MS, + maxBuffer: 2 * 1024 * 1024, + }); +} + +function descendantPids(rootPid, snapshot) { + const children = new Map(); + for (const line of String(snapshot || '').split(/\r?\n/)) { + const match = line.trim().match(/^(\d+)\s+(\d+)$/); + if (!match) continue; + const pid = Number(match[1]); + const ppid = Number(match[2]); + if (!children.has(ppid)) children.set(ppid, []); + children.get(ppid).push(pid); + } + const result = new Set(); + const pending = [Number(rootPid)]; + while (pending.length) { + const parent = pending.shift(); + for (const pid of children.get(parent) || []) { + if (result.has(pid)) continue; + result.add(pid); + pending.push(pid); + } + } + return result; +} + +function collectDescendantPids(rootPid, { + platform = process.platform, + snapshotProcesses = readProcessSnapshot, +} = {}) { + if (platform === 'win32') return { pids: new Set(), available: true }; + try { return { pids: descendantPids(rootPid, snapshotProcesses()), available: true }; } catch { return { pids: new Set(), available: false }; } +} function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { const boundedLimit = Math.max(0, Number(limit) || 0); - const headLimit = Math.ceil(boundedLimit / 2); - const tailLimit = boundedLimit - headLimit; + const separatorLength = TRUNCATION_SEPARATOR.length <= boundedLimit ? TRUNCATION_SEPARATOR.length : 0; + const payloadLimit = boundedLimit - separatorLength; + const headLimit = Math.ceil(payloadLimit / 2); + const tailLimit = payloadLimit - headLimit; const headChunks = []; let headByteLength = 0; let tail = Buffer.alloc(0); + let truncated = false; const appendTail = (buffer) => { if (!tailLimit || !buffer.length) return; + if (buffer.length > tailLimit || tail.length + buffer.length > tailLimit) truncated = true; const retained = buffer.length > tailLimit ? buffer.subarray(buffer.length - tailLimit) : buffer; const combined = Buffer.concat([tail, retained]); tail = combined.length > tailLimit @@ -26,6 +87,7 @@ function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { let offset = 0; if (headByteLength < headLimit) { const retainedLength = Math.min(buffer.length, headLimit - headByteLength); + if (retainedLength < buffer.length) truncated = true; headChunks.push(Buffer.from(buffer.subarray(0, retainedLength))); headByteLength += retainedLength; offset = retainedLength; @@ -36,8 +98,11 @@ function createOutputCapture(stream, limit = MAX_CAPTURE_BYTES) { if (offset < buffer.length) appendTail(buffer.subarray(offset)); }); return Object.freeze({ - bytes: () => headByteLength + tail.length, - value: () => Buffer.concat([...headChunks, tail], headByteLength + tail.length).toString('utf8'), + bytes: () => headByteLength + tail.length + (truncated ? separatorLength : 0), + value: () => { + const separator = truncated ? TRUNCATION_SEPARATOR.subarray(0, separatorLength) : Buffer.alloc(0); + return Buffer.concat([...headChunks, separator, tail], headByteLength + tail.length + separator.length).toString('utf8'); + }, }); } @@ -48,16 +113,21 @@ function terminateOwnedTree(child, { signalProcess = process.kill, onComplete = () => {}, taskkillTimeoutMs = TASKKILL_TIMEOUT_MS, + ownedPids = new Set(), + ownershipScanFailed = false, } = {}) { if (!child?.pid) { onComplete({ contained: false, reason: 'missing_child_pid' }); return; } let completed = false; const complete = (result) => { if (!completed) { completed = true; onComplete(result); } }; - const stopKnownChild = () => { - try { child.kill?.('SIGKILL'); } catch {} + const closeKnownStreams = () => { for (const stream of [child.stdin, child.stdout, child.stderr]) { try { stream?.destroy?.(); } catch {} } }; + const stopKnownChild = () => { + try { child.kill?.('SIGKILL'); } catch {} + closeKnownStreams(); + }; const failContainment = (reason) => { stopKnownChild(); complete({ contained: false, reason }); }; if (platform === 'win32') { // /t targets the child process tree by PID; it is not a process-name kill. @@ -76,7 +146,7 @@ function terminateOwnedTree(child, { killer.once('error', () => { if (!settleAttempt()) return; failContainment('taskkill_failed'); }); killer.once('close', (code) => { if (!settleAttempt()) return; - if (code === 0) complete({ contained: true }); + if (code === 0) { closeKnownStreams(); complete({ contained: true }); } else if (attempt === 0) runTaskkill(1); else failContainment('taskkill_failed'); }); @@ -84,17 +154,36 @@ function terminateOwnedTree(child, { runTaskkill(0); return; } - // The dedicated group is the containment boundary. A direct SIGKILL avoids - // graceful handlers and the fork window they can open; success means only - // that the owned process group accepted the signal, not that arbitrary - // processes which detached before the timeout were in that group. + // The dedicated group is the primary containment boundary. A process can + // deliberately call setsid/detach and escape that group, so the worker also + // kills the exact descendant PIDs observed while the probe was alive. This + // is still an owned-process boundary, unlike a process-name kill. + let groupError; try { signalProcess(-child.pid, 'SIGKILL'); - complete({ contained: true }); } catch (error) { - failContainment(error?.code === 'ESRCH' ? 'process_group_absent' : 'process_group_kill_failed'); + groupError = error; + } + for (const pid of ownedPids) { + if (!pid || pid === child.pid || pid === process.pid) continue; + try { signalProcess(pid, 'SIGKILL'); } + catch (error) { + if (error?.code !== 'ESRCH') { + failContainment('owned_process_kill_failed'); + return; + } + } + } + if (ownershipScanFailed) { + failContainment('descendant_scan_failed'); + return; } + if (!groupError || (groupError.code === 'ESRCH' && ownedPids.size)) { + stopKnownChild(); + complete({ contained: true }); + } + else failContainment(groupError.code === 'ESRCH' ? 'process_group_absent' : 'process_group_kill_failed'); } function runProbe(request, { @@ -103,8 +192,9 @@ function runProbe(request, { output = process.stdout, schedule = setTimeout, taskkillTimeoutMs = TASKKILL_TIMEOUT_MS, + snapshotProcesses = readProcessSnapshot, } = {}) { - const timeoutMs = Math.max(1, Number(request.timeoutMs) || 10_000); + const timeoutMs = normalizeProbeTimeoutMs(request.timeoutMs); const finish = (value) => output.write(JSON.stringify(value)); let child; try { @@ -115,24 +205,59 @@ function runProbe(request, { } const stdout = createOutputCapture(child.stdout); const stderr = createOutputCapture(child.stderr); + const ownedPids = new Set(); let timedOut = false; let settled = false; + let ownershipTimer; + let ownershipScanFailed = false; + let ownershipScanCalls = 0; + const clearOwnershipTimer = () => { if (ownershipTimer) { try { clearTimeout(ownershipTimer); } catch {} ownershipTimer = undefined; } }; + const scanOwnedPids = () => { + if (platform === 'win32') return; + if (ownershipScanCalls >= MAX_PROCESS_SNAPSHOT_CALLS) { + ownershipScanFailed = true; + return; + } + ownershipScanCalls += 1; + const scan = collectDescendantPids(child.pid, { platform, snapshotProcesses }); + if (!scan.available) { + ownershipScanFailed = true; + return; + } + for (const pid of scan.pids) ownedPids.add(pid); + }; + const refreshOwnedPids = () => { + if (settled || timedOut || platform === 'win32') return; + scanOwnedPids(); + if (!settled && !timedOut && !ownershipScanFailed) { + ownershipTimer = schedule(refreshOwnedPids, PROCESS_SCAN_INTERVAL_MS); + ownershipTimer?.unref?.(); + } + }; const finishTimeout = (containment) => { if (settled) return; settled = true; clearTimeout(timer); + clearOwnershipTimer(); if (containment.contained) return finish({ ok: false, code: 'ETIMEDOUT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms`, stdout: stdout.value(), stderr: stderr.value() }); finish({ ok: false, code: 'ECONTAINMENT', message: `Obsidian CLI probe timed out after ${timeoutMs}ms and dedicated process-group containment could not be confirmed (${containment.reason || 'unknown'})`, stdout: stdout.value(), stderr: stderr.value() }); }; const timer = schedule(() => { timedOut = true; - terminateOwnedTree(child, { platform, spawnProcess, schedule, taskkillTimeoutMs, onComplete: finishTimeout }); + // Take one final bounded snapshot while the direct child is still the + // ownership root. PIDs observed here or in earlier snapshots are the only + // exact descendants eligible for POSIX cleanup. + scanOwnedPids(); + terminateOwnedTree(child, { platform, spawnProcess, schedule, taskkillTimeoutMs, ownedPids, ownershipScanFailed, onComplete: finishTimeout }); }, timeoutMs); + ownershipTimer = schedule(refreshOwnedPids, 0); + ownershipTimer?.unref?.(); child.on('error', (error) => { if (settled) return; if (timedOut) return; settled = true; clearTimeout(timer); + clearOwnershipTimer(); finish({ ok: false, code: error.code, message: error.message, stdout: stdout.value(), stderr: stderr.value() }); }); child.on('close', (code, signal) => { @@ -142,6 +267,7 @@ function runProbe(request, { if (timedOut) return; settled = true; clearTimeout(timer); + clearOwnershipTimer(); if (code === 0) return finish({ ok: true, stdout: stdout.value(), stderr: stderr.value() }); finish({ ok: false, code: code == null ? signal : code, message: stderr.value() || stdout.value() || `Obsidian CLI exited with ${code == null ? signal : code}`, stdout: stdout.value(), stderr: stderr.value() }); }); @@ -158,4 +284,4 @@ if (require.main === module) { runProbe(request); } -module.exports = { MAX_CAPTURE_BYTES, createOutputCapture, runProbe, terminateOwnedTree }; +module.exports = { MAX_CAPTURE_BYTES, PROBE_CLEANUP_GRACE_MS, createOutputCapture, normalizeProbeTimeoutMs, runProbe, terminateOwnedTree }; diff --git a/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js b/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js index 29d00361..9423669e 100644 --- a/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js +++ b/modules/jarvos-secondbrain/adapters/obsidian/src/vault-mutation-adapter.js @@ -6,6 +6,7 @@ const path = require('node:path'); const { execFileSync } = require('node:child_process'); const { createInternalReceipt, hashUtf8, validateOperation, validateVaultRelativeMarkdownPath } = require('./vault-mutation-contract'); const { createVaultMutationLedger } = require('./vault-mutation-ledger'); +const { normalizeProbeTimeoutMs, PROBE_CLEANUP_GRACE_MS } = require('./obsidian-cli-probe-worker'); const RESULT_STORE = '__jarvosVaultMutationResults'; const CAPABILITY_STATES = Object.freeze(['available', 'cli_missing', 'app_stopped', 'app_busy', 'app_unreachable', 'cli_disabled', 'cli_unsupported', 'wrong_vault', 'api_incompatible']); @@ -17,19 +18,20 @@ function sleepSync(milliseconds) { function parseEvalResult(output) { const match = [...String(output || '').matchAll(/^=>\s*(.+)$/gm)].at(-1); return match ? JSON.parse(match[1]) : null; } function runObsidianEval(code, { vaultName, command = process.env.OBSIDIAN_CLI || 'obsidian', timeoutMs = 10_000, execute = execFileSync } = {}) { + const normalizedTimeoutMs = normalizeProbeTimeoutMs(timeoutMs); const args = [`vault=${vaultName}`, 'eval', `code=${code}`]; try { // Test seams that supply their own executor retain the old direct contract. - if (execute !== execFileSync) return parseEvalResult(execute(command, args, { encoding: 'utf8', timeout: timeoutMs, stdio: ['ignore', 'pipe', 'pipe'] })); + if (execute !== execFileSync) return parseEvalResult(execute(command, args, { encoding: 'utf8', timeout: normalizedTimeoutMs, stdio: ['ignore', 'pipe', 'pipe'] })); // Keep the worker envelope off argv. The underlying CLI still receives // the exact same args (including large mutation programs), while the // worker request itself travels through stdin and does not add base64 // expansion to the platform's per-argument limit. - const request = JSON.stringify({ command, args, timeoutMs }); + const request = JSON.stringify({ command, args, timeoutMs: normalizedTimeoutMs }); const response = JSON.parse(execFileSync(process.execPath, [OBSIDIAN_CLI_PROBE_WORKER], { encoding: 'utf8', input: request, - timeout: timeoutMs + 1_000, + timeout: normalizedTimeoutMs + PROBE_CLEANUP_GRACE_MS, stdio: ['pipe', 'pipe', 'pipe'], })); if (!response.ok) { diff --git a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js index e5f659ca..61c7c4de 100644 --- a/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js +++ b/modules/jarvos-secondbrain/tests/vault-mutation-adapter.test.js @@ -9,7 +9,7 @@ const test = require('node:test'); const { createVaultMutationAdapter, runObsidianEval } = require('../adapters/obsidian/src/vault-mutation-adapter'); const { buildObsidianInvariantProgram, buildObsidianMutationProgram } = require('../adapters/obsidian/src/vault-mutation-adapter'); const { createJarvosVaultTransforms } = require('../src/vault-transform-registry'); -const { MAX_CAPTURE_BYTES, createOutputCapture, terminateOwnedTree } = require('../adapters/obsidian/src/obsidian-cli-probe-worker'); +const { MAX_CAPTURE_BYTES, createOutputCapture, normalizeProbeTimeoutMs, runProbe, terminateOwnedTree } = require('../adapters/obsidian/src/obsidian-cli-probe-worker'); const operation = () => ({ schemaVersion: 1, operationId: 'op-20260806-adapter-test', vaultId: 'vault-a', vaultRelativePath: 'Notes/A.md', sequence: 1, operationKind: 'create', content: 'hello' }); const ledgerPath = () => path.join(os.tmpdir(), `jarvos-adapter-${Math.random()}.json`); @@ -56,6 +56,14 @@ test('timeouts and ambiguous CLI failures never prove that Obsidian is stopped', assert.equal(stoppedAdapter.capability().state, 'app_stopped'); }); +test('probe timeout values are normalized and rejected consistently before spawning', () => { + assert.equal(normalizeProbeTimeoutMs('12.9'), 12); + for (const timeoutMs of [0, -1, NaN, Infinity, '', 'not-a-number', true]) { + assert.throws(() => runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', timeoutMs }), /timeoutMs/); + assert.throws(() => runProbe({ command: 'unused', args: [], timeoutMs }, { spawnProcess: () => { throw new Error('must not spawn'); } }), /timeoutMs/); + } +}); + test('capability probe kills a same-process-group descendant', () => { if (process.platform === 'win32') return; const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-')); @@ -75,6 +83,25 @@ test('capability probe kills a same-process-group descendant', () => { } }); +test('timeout contains a detached descendant that inherits probe pipes and returns promptly', () => { + if (process.platform === 'win32') return; + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-inherited-pipes-')); + const fixture = path.join(root, 'fake-obsidian-cli.js'); + const descendantPid = path.join(root, 'descendant.pid'); + fs.writeFileSync(fixture, `#!${process.execPath}\nconst fs = require('node:fs'); const { spawn } = require('node:child_process'); const child = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { detached: true, stdio: 'inherit' }); child.unref(); fs.writeFileSync(${JSON.stringify(descendantPid)}, String(child.pid)); setInterval(() => {}, 1000);\n`); + fs.chmodSync(fixture, 0o755); + const started = Date.now(); + try { + assert.throws(() => runObsidianEval('JSON.stringify({ok:true})', { vaultName: 'fake-vault', command: fixture, timeoutMs: 300 }), (error) => error.code === 'ETIMEDOUT'); + assert.ok(Date.now() - started < 1_000); + const pid = Number(fs.readFileSync(descendantPid, 'utf8')); + assert.throws(() => process.kill(pid, 0), { code: 'ESRCH' }); + } finally { + try { const pid = Number(fs.readFileSync(descendantPid, 'utf8')); process.kill(pid, 'SIGKILL'); } catch {} + fs.rmSync(root, { recursive: true, force: true }); + } +}); + test('an ordinary timeout preserves ETIMEDOUT and leaves no child', () => { if (process.platform === 'win32') return; const root = fs.mkdtempSync(path.join(os.tmpdir(), 'jarvos-obsidian-probe-normal-')); @@ -139,6 +166,7 @@ test('probe output capture caps multibyte chunks by bytes', () => { const capture = createOutputCapture(stream); stream.emit('data', Buffer.from('€'.repeat(MAX_CAPTURE_BYTES))); assert.equal(capture.bytes(), MAX_CAPTURE_BYTES); + assert.match(capture.value(), /\[\.\.\. output truncated \.\.\.\]/); }); test('Windows taskkill retries process-tree containment and reports success only from taskkill', () => { @@ -237,6 +265,22 @@ test('a failed POSIX group SIGKILL does not prove descendant containment', () => assert.deepEqual(results, [{ contained: false, reason: 'process_group_absent' }]); }); +test('an uncertain descendant snapshot fails closed after killing only observed descendants', () => { + const signals = []; + const childSignals = []; + const results = []; + terminateOwnedTree({ pid: 12345, kill: (signal) => childSignals.push(signal) }, { + platform: 'darwin', + signalProcess: (pid, signal) => signals.push([pid, signal]), + ownedPids: new Set([23456]), + ownershipScanFailed: true, + onComplete: (result) => results.push(result), + }); + assert.deepEqual(signals, [[-12345, 'SIGKILL'], [23456, 'SIGKILL']]); + assert.deepEqual(childSignals, ['SIGKILL']); + assert.deepEqual(results, [{ contained: false, reason: 'descendant_scan_failed' }]); +}); + test('unavailable capability retains planned intent for reconciliation', () => { const adapter = createVaultMutationAdapter({ vaultRoot: '/vault', vaultId: 'vault-a', ledgerPath: ledgerPath(), probe: () => ({ state: 'app_stopped' }) }); assert.equal(adapter.execute(operation()).status, 'unavailable');