From 6bee6b856edcba3c01b655a237dc596635ca7fdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sun, 16 Aug 2026 03:18:47 +0000 Subject: [PATCH] a member's background job can be read and stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A member started `bun dev`, watched it run, and got no logs at all — then `{"stopped":false,"reason":"no such task"}` from the Stop button. Two symptoms, one cause, and it is the same one `read-as-user.ts` was written for. `findTaskOutput` globbed `/tmp/claude-*/…` as the platform's own uid. `/tmp/claude-` is `drwx------` (checked, not assumed), so for a member that directory cannot even be traversed. A glob that cannot traverse does not throw — it returns an empty iterator. Both callers read that as "no such task": the log view showed "Nothing written yet" forever, and Stop refused an id that was perfectly real. Every other filesystem access in that module already goes through the as-user helpers. This one path never did, and it was invisible for as long as only the owner used chat. So the lookup moves into `findPathAs`, and Stop runs as the member too — both halves of it. `fuser` reads `/proc//fd`, which only the process's own user and root may open, so as the platform it would have found no pids and reported the job already stopped; the kill after it would have been cross-user EPERM, swallowed by a catch. Neither would have said anything. ── The exit code is ignored, and that is the point ── The first version of this fix located the file and threw it away. `find` exits 1 when it cannot descend into something, and searching `/tmp` from ANY identity hits another account's `drwx------` — it prints the match on stdout first, then returns 1. Treating that as failure reproduced the original bug through a new mechanism. Caught by testing the owner path against a real task file rather than assuming the refactor was behaviour-preserving; a missing root still exits 1 with empty stdout, so absence still reads as absence. Owner and member now take the same `find`, deliberately: one set of matching semantics, rather than a glob for us and a find for them, which is how a bug ends up reproducing on one account and not the other. Verified on the owner path here against a live task file. The member path is covered by `read-as-user.live.test.ts` — same opt-in shape as `spawn-as-member.live.test.ts`, asserting the two identities DISAGREE, which a world-readable tree would not satisfy. It cannot run on a machine with one account, so it skips here and needs OFFICER_TEST_MEMBER on the host. tsgo clean, frontend builds, 865 pass / 7 fail unchanged, 3 new skips. Co-Authored-By: Claude Opus 5 --- src/servers/api/chat/claude-sessions.ts | 51 ++++++++++++---- src/servers/read-as-user.live.test.ts | 80 +++++++++++++++++++++++++ src/servers/read-as-user.ts | 32 ++++++++++ 3 files changed, 151 insertions(+), 12 deletions(-) create mode 100644 src/servers/read-as-user.live.test.ts diff --git a/src/servers/api/chat/claude-sessions.ts b/src/servers/api/chat/claude-sessions.ts index e3a32c3f..e393b3be 100644 --- a/src/servers/api/chat/claude-sessions.ts +++ b/src/servers/api/chat/claude-sessions.ts @@ -3,6 +3,7 @@ import { dirname, join } from 'node:path'; import { DATA_PATH } from '../../data-path'; import { appendTextAs, + findPathAs, listTranscriptsAs, readHeadAs, readTailAs, @@ -10,6 +11,7 @@ import { removeAs, type AsUser, } from '../../read-as-user'; +import { runAsArgv } from '../../os-user'; // ── Claude session store (source of truth) ── // The `claude` CLI persists every session as a JSONL transcript at @@ -694,20 +696,32 @@ export type BackgroundTaskDetail = | { kind: 'agent'; messages: ClaudeChatMessage[] } | { kind: 'log'; text: string; truncated: boolean }; +/** + * Where a task's file is, looked for AS the account that owns it. + * + * This used to be a `Bun.Glob` scan running as the platform's own uid, and it worked for exactly one + * account: the owner. `/tmp/claude-` is `drwx------`, so for a member the scan matched nothing — and + * a glob that cannot traverse does not throw, it returns an empty iterator. Both callers read that as "no + * such task", which is why a member saw a job that ran fine, streamed no logs at all, and answered + * `{"stopped":false,"reason":"no such task"}` when they pressed Stop. One unreachable directory, two + * symptoms that look unrelated. + * + * The depths are the old glob's shape, kept as explicit bounds so neither `find` wanders: a task output is + * `/claude-///tasks/.output` (5), a subagent transcript is + * `///subagents/agent-.jsonl` (4). `taskId` is `TASK_ID_RE`-checked above, so + * it carries no glob or shell metacharacter into either. + */ function findTaskOutput(who: ChatIdentity, taskId: string): string | null { + // The member's `claude` runs under `--reset-env`, so its TMPDIR is the passwd default, not ours. Both + // resolve to /tmp on Linux; a server started with a TMPDIR of its own would look in the wrong place. const tmpRoot = process.env.TMPDIR ?? '/tmp'; - const candidates: [string, string][] = [ - [tmpRoot, `claude-*/*/*/tasks/${taskId}.output`], - [claudeProjectsDir(who.home), `*/*/subagents/agent-${taskId}.jsonl`], + const candidates: [string, string[]][] = [ + [tmpRoot, ['-mindepth', '5', '-maxdepth', '5', '-name', `${taskId}.output`]], + [claudeProjectsDir(who.home), ['-mindepth', '4', '-maxdepth', '4', '-name', `agent-${taskId}.jsonl`]], ]; - for (const [root, pattern] of candidates) { - try { - for (const rel of new Bun.Glob(pattern).scanSync({ cwd: root, followSymlinks: false, onlyFiles: false })) { - return join(root, rel); - } - } catch { - /* root doesn't exist — try the next one */ - } + for (const [root, predicate] of candidates) { + const hit = findPathAs(who.osUser, root, predicate); + if (hit) return hit; } return null; } @@ -743,7 +757,12 @@ export async function stopBackgroundTask( if (!found) return { stopped: false, reason: 'no such task' }; if (found.endsWith('.jsonl')) return { stopped: false, reason: 'subagents cannot be stopped this way' }; - const proc = Bun.spawn(['fuser', found], { stdout: 'pipe', stderr: 'ignore' }); + // As the member, for both halves. `fuser` reads `/proc//fd`, which only the process's own user and + // root may open, so run as the platform it would find no pids for a member's job and report it already + // stopped. And the kill would then be cross-user: EPERM, silently swallowed by the catch below. + const asUser = (command: string[]) => (who.osUser ? runAsArgv(who.osUser, command) : command); + + const proc = Bun.spawn(asUser(['fuser', found]), { stdout: 'pipe', stderr: 'ignore' }); const out = await new Response(proc.stdout).text(); await proc.exited; @@ -754,6 +773,14 @@ export async function stopBackgroundTask( if (pids.length === 0) return { stopped: false, reason: 'already stopped' }; + if (who.osUser) { + // One fork for the whole set rather than one per pid, and the kernel checks the member's own identity + // against processes that are already theirs. + const kill = Bun.spawn(asUser(['kill', '-TERM', ...pids.map(String)]), { stdout: 'ignore', stderr: 'ignore' }); + await kill.exited; + return { stopped: true }; + } + for (const pid of pids) { try { process.kill(pid, 'SIGTERM'); diff --git a/src/servers/read-as-user.live.test.ts b/src/servers/read-as-user.live.test.ts new file mode 100644 index 00000000..2f4ea7a6 --- /dev/null +++ b/src/servers/read-as-user.live.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, test } from 'bun:test'; +import { join } from 'node:path'; +import { runAsArgv } from './os-user'; +import { findPathAs } from './read-as-user'; + +// Can the platform LOCATE a member's file? Same shape as `spawn-as-member.live.test.ts`, and for the same +// reason: the failure only exists where a second Linux account does, which is the production host and no +// developer machine. +// +// ── The bug this pins ── +// +// `findTaskOutput` globbed `/tmp/claude-*/…` as the platform's own uid. `/tmp/claude-` is `drwx------`, +// so for a member the glob matched nothing — and a glob that cannot traverse does not throw, it returns an +// empty iterator. Downstream that read as "no such task", which a member saw as a background job that +// streamed no logs and answered `{"stopped":false,"reason":"no such task"}` on Stop. +// +// The assertion is the two identities DISAGREEING. A test that only checked `findPathAs(member, …)` finds +// the file would pass just as happily on a world-readable tree, which is not the situation that broke. +// +// ── Running it ── +// +// OFFICER_TEST_MEMBER=green bun test src/servers/read-as-user.live.test.ts +// +// Skips without it. A skipped run is not a pass; the log says which it was. + +const osUser = process.env.OFFICER_TEST_MEMBER; +const live = Boolean(osUser); + +const runAs = (user: string, command: string[]) => { + const result = Bun.spawnSync(runAsArgv(user, command), { stdout: 'pipe', stderr: 'pipe' }); + if (result.exitCode !== 0) { + throw new Error(`as ${user}: ${new TextDecoder().decode(result.stderr).trim() || `exit ${result.exitCode}`}`); + } + return new TextDecoder().decode(result.stdout).trim(); +}; + +describe.skipIf(!live)(`findPathAs (live, as ${osUser ?? 'nobody'})`, () => { + const user = osUser!; + // The real layout, at the real depth: /claude-///tasks/.output + const uid = live ? runAs(user, ['id', '-u']) : ''; + const taskId = 'blivetest01'; + const dir = join('/tmp', `claude-${uid}`, '-live-test-slug', 'live-test-session', 'tasks'); + const file = join(dir, `${taskId}.output`); + + test('the member can be found where the platform is blind', () => { + // Created BY the member, so its modes are the ones `claude` would produce rather than ones we chose. + runAs(user, ['mkdir', '-p', dir]); + runAs(user, ['sh', '-c', 'printf "live\\n" > "$1"', '_', file]); + runAs(user, ['chmod', '700', join('/tmp', `claude-${uid}`)]); + + try { + const predicate = ['-mindepth', '5', '-maxdepth', '5', '-name', `${taskId}.output`]; + + // As the member: found. + expect(findPathAs(user, '/tmp', predicate)).toBe(file); + + // As the platform: NOT found — this is the half that used to be the only half. + expect(findPathAs(null, '/tmp', predicate)).toBeNull(); + } finally { + runAs(user, ['rm', '-rf', join('/tmp', `claude-${uid}`, '-live-test-slug')]); + } + }); + + test('a task id that does not exist is null, not a throw', () => { + expect(findPathAs(user, '/tmp', ['-mindepth', '5', '-maxdepth', '5', '-name', 'no-such-task.output'])).toBeNull(); + }); + + test('an unreadable directory in the walk does not discard a real match', () => { + // `find /tmp` from any identity hits another account's `drwx------` and exits 1 having already printed + // what it found. This is the regression that cost the first version of the fix: the match was located + // and then thrown away with the exit code. + runAs(user, ['mkdir', '-p', dir]); + runAs(user, ['sh', '-c', 'printf "live\\n" > "$1"', '_', file]); + try { + expect(findPathAs(user, '/tmp', ['-mindepth', '5', '-maxdepth', '5', '-name', `${taskId}.output`])).toBe(file); + } finally { + runAs(user, ['rm', '-rf', join('/tmp', `claude-${uid}`, '-live-test-slug')]); + } + }); +}); diff --git a/src/servers/read-as-user.ts b/src/servers/read-as-user.ts index 58d1bdf2..27752fe6 100644 --- a/src/servers/read-as-user.ts +++ b/src/servers/read-as-user.ts @@ -165,6 +165,38 @@ function runSync(osUser: string, command: string[]): string { return new TextDecoder().decode(result.stdout); } +/** + * The first path under `root` matching `predicate`, as the owner of the tree. + * + * Locating a file needs `x` on every directory above it, which the service user does not have on a + * member's tree — the header explains why the ACL cannot give it. `/tmp/claude-` is `drwx------`, + * so a glob run as the platform's own uid does not fail: it silently matches NOTHING, which reads + * downstream as "no such task". + * + * `predicate` is `find` arguments, so callers state their own depth bounds — an unbounded walk of `/tmp` + * or a member's home is not something to leave to chance. + * + * Owner and member take the same `find`, deliberately. One set of matching semantics beats a glob for us + * and a `find` for them, which is exactly the kind of split that makes a bug reproduce on one account and + * not the other. + */ +export function findPathAs(osUser: AsUser, root: string, predicate: string[]): string | null { + const command = ['find', root, ...predicate]; + const result = Bun.spawnSync(osUser ? runAsArgv(osUser, command) : command, { stdout: 'pipe', stderr: 'ignore' }); + + // The EXIT CODE IS IGNORED, and that is the whole trick. `find` exits 1 when it could not descend into + // something — and searching `/tmp` from any identity is guaranteed to hit another account's + // `drwx------`. It still prints what it DID find, on stdout, before returning 1. Treating that as + // failure threw away the match: verified here, where the file was located and then discarded. + // + // A genuinely missing root also exits 1, with nothing on stdout, so absence still reads as absence. + const first = new TextDecoder() + .decode(result.stdout) + .split('\n') + .find((line) => line.length > 0); + return first ?? null; +} + /** The whole file, as text. Throws on any failure, so existing `try`/`catch` around reads keeps working. */ export function readTextAs(osUser: AsUser, path: string): string { if (!osUser) return readFileSync(path, 'utf-8');