From 4407b3bb161e061ceb449428e9fa6e57d233317e Mon Sep 17 00:00:00 2001 From: nntrivi2001 Date: Fri, 9 Oct 2026 15:21:31 +0700 Subject: [PATCH] fix(server-s3): list a member's tmux panes asynchronously A frozen member (SIGSTOPped tmux server) cost every user one synchronous 3 s block of the event loop per minute (F37's back-off ceiling). Member listing now runs through async execFile (same 3 s / SIGKILL / 1 MiB bounds, same back-off) into the listed cache the synchronous readers (liveSessions, listSessions, listOutput) already use: stale-while- revalidate, 15 s max age, cold or failing = that member's panes UNKNOWN (F38 unchanged). A session this process creates or kills is written into the member's cached answer at once, and a list in flight across that change is dropped. Host tmux calls, local mode and every click's call (new-session, send-keys, kill, has-session) are unchanged. review5's stalled-member test now polls for 4 s past the timeout and asserts each poll < 100 ms, event-loop stall < 500 ms and one sudo (old code: first poll 3157 ms). --- docs/SERVER-MODE.md | 8 +- docs/plans/server-mode/S3-PLAN.md | 2 +- server/__tests__/server-s3-panes.test.js | 14 +- server/__tests__/server-s3-review5.test.js | 64 ++++++-- server/lib/tmux.js | 175 ++++++++++++++++----- 5 files changed, 206 insertions(+), 57 deletions(-) diff --git a/docs/SERVER-MODE.md b/docs/SERVER-MODE.md index 445bdba9..393ef81f 100644 --- a/docs/SERVER-MODE.md +++ b/docs/SERVER-MODE.md @@ -567,7 +567,13 @@ a pane really is. Each case below is refused or bounded, with a test in - **A member's stopped or flooding tmux server.** Every call on it has a 3 s limit and a 1 MiB answer cap; one that hit either is not asked again for a minute. Meanwhile only that member's panes count as unknown (kept, not - ended); everyone else's are still checked. Every other synchronous call + ended); everyone else's are still checked. Listing a member's panes (every + poll: the pane list, the workspace list, the 10 s heal beat, the liveness + reaper) never waits for them: it runs in the background and the poll reads + its last answer, so a stopped server stalls nobody. Until a first answer, + or once the last is 15 s old, their panes count as unknown too. A call a + click makes on a member's pane (start, type, stop) still waits, up to the + 3 s. Every other synchronous call that runs as a member (git in their folder, `write-as`) stops after 5 s. - **A member's huge or sparse files.** Transcripts and journals are read line by line with a 64 MiB cap per line (a 512 MB line used to crash the server); diff --git a/docs/plans/server-mode/S3-PLAN.md b/docs/plans/server-mode/S3-PLAN.md index 7f6c1a01..2e0d440d 100644 --- a/docs/plans/server-mode/S3-PLAN.md +++ b/docs/plans/server-mode/S3-PLAN.md @@ -267,7 +267,7 @@ Review ran 2026-10-08 (four lenses: root/sudo, cross-member, secrets, local-mode - F34 a developer's extra workspace folder is checked against their own folder (`scope.folderAllowed`, real path) before anything probes it, and again on the path it records (`403 EFOLDER`); - F35 the Codex pool's quota reads a run's rollout only from the `CODEX_HOME` of the account its workspace runs as, never one dated in the future. Accepted ceiling (documented): a member can still forge their own session's reading. - F36 (critical) `GET /api/cc-config/file` never serves the data dir (`isSecretConfigPath`): an analyst read an admin gateway pane's live pane token from `proxy-settings/`, plus `vapid-keys.json` and the database; - - F37 every tmux call on a member's socket is bounded (`memberCall`: 3 s, SIGKILL, 1 MiB) with a 60 s back-off — a SIGSTOPped member server froze the event loop for good; + - F37 every tmux call on a member's socket is bounded (`memberCall`: 3 s, SIGKILL, 1 MiB) with a 60 s back-off — a SIGSTOPped member server froze the event loop for good. Member LISTING is async (`refreshList`: `execFile`, same limits) into the `listed` cache the sync readers use (stale-while-revalidate, 15 s max age, cold = unknown; a session this process makes or kills is written into the answer at once), so a frozen member no longer costs a 3 s block per back-off on the poll path; a click's call (new-session, send-keys, kill, has-session) is still a sync ≤ 3 s; - F38 a failing member server makes only that member's panes unknown (`liveSessions().has` true for them), never null for everyone; member list errors are cached; - F39 `execIn` and `write-as` (every sync sudo into a member) stop after 5 s (`MEMBER_EXEC`) — a FIFO `include.path` or a FIFO write target hung git and write-as; - F40 transcripts and journals are read with `readLines` (chunked, a line over 64 MiB is skipped, never built) — readline crashed the server on a one-line 512 MB sparse file (import-history, sessions, codex-usage); diff --git a/server/__tests__/server-s3-panes.test.js b/server/__tests__/server-s3-panes.test.js index 23e81f14..2670e943 100644 --- a/server/__tests__/server-s3-panes.test.js +++ b/server/__tests__/server-s3-panes.test.js @@ -134,12 +134,24 @@ describe("a member's panes on the member's own tmux server", () => { assert.ok(calls.every((a) => a[0] !== "-S")); }); - it("lists sessions across every server, believing a member's only about their own", () => { + it("lists sessions across every server, believing a member's only about their own", async () => { const orig = runAs.membersWithTmux; runAs.membersWithTmux = () => [ACCT]; try { + // A member's list is async: until it lands their panes are UNKNOWN. + const cold = tmux.liveSessions(); + assert.deepEqual([...cold].sort(), [HOST, "other"].sort()); + assert.equal(cold.has(MEMBER), true, "unknown, kept"); + assert.equal(cold.has("agenthub-lane-9"), false); + assert.deepEqual(tmux.listSessions("agenthub-lane-"), [HOST]); + await tmux.__settle(); assert.deepEqual([...tmux.liveSessions()].sort(), [MEMBER, HOST, "other"].sort()); assert.deepEqual(tmux.listSessions("agenthub-lane-").sort(), [HOST, MEMBER].sort()); + // A session this process ends or makes is in the next answer at once. + tmux.killSession(MEMBER); + assert.deepEqual(tmux.listSessions("agenthub-lane-"), [HOST]); + tmux.newSession({ name: MEMBER, cwd: HOME, argv: ["bash"], env: {} }); + assert.deepEqual(tmux.listSessions("agenthub-lane-").sort(), [HOST, MEMBER].sort()); } finally { runAs.membersWithTmux = orig; } diff --git a/server/__tests__/server-s3-review5.test.js b/server/__tests__/server-s3-review5.test.js index 959342d5..a699aae9 100644 --- a/server/__tests__/server-s3-review5.test.js +++ b/server/__tests__/server-s3-review5.test.js @@ -236,25 +236,46 @@ describe("a member's stopped tmux server or planted FIFO", () => { // sudo that never returns: a tmux client talking to a SIGSTOPped server. const bin = path.join(TMP, "fakebin"); fs.mkdirSync(bin, { recursive: true }); - fs.writeFileSync(path.join(bin, "sudo"), "#!/bin/sh\nexec sleep 60\n", { mode: 0o755 }); + const spawned = path.join(TMP, "sudo-spawns"); + fs.rmSync(spawned, { force: true }); + fs.writeFileSync(path.join(bin, "sudo"), `#!/bin/sh\necho x >> '${spawned}'\nexec sleep 60\n`, { + mode: 0o755, + }); try { + // Polls like the heal beat / GET /api/lanes for 4 s — past the 3 s + // timeout — while timing every poll and the event loop's worst stall. const child = inChild( `const runAs = require(${JSON.stringify(`${LIB}/run-as`)}); const tmux = require(${JSON.stringify(`${LIB}/tmux`)}); + const { monitorEventLoopDelay } = require("node:perf_hooks"); const proj = ${JSON.stringify(path.join(MIA, "proj"))}; const time = (f) => { const t = Date.now(); try { f(); return { ms: Date.now() - t }; } catch (e) { return { code: e.code, ms: Date.now() - t }; } }; - let live; - const first = time(() => (live = tmux.liveSessions())); - const again = time(() => tmux.liveSessions()); - process.stdout.write(JSON.stringify({ - isSet: live instanceof Set, - keeps: live.has("agenthub-lane-${lane}"), - other: live.has("agenthub-lane-999999"), - first, again, - git: time(() => runAs.execIn(proj, "git", ["status"])), - write: time(() => runAs.writeAs(runAs.accountForPath(proj), proj + "/f.txt", "", "x")), - }));`, + (async () => { + runAs.accountForSession("agenthub-lane-${lane}"); // opens the DB: not what is timed + const loop = monitorEventLoopDelay({ resolution: 10 }); + loop.enable(); + let live; + const first = time(() => (live = tmux.liveSessions())); + let slowest = 0; + const beat = setInterval(() => { + slowest = Math.max(slowest, time(() => tmux.liveSessions()).ms); + }, 50); + await new Promise((r) => setTimeout(r, 4000)); + clearInterval(beat); + loop.disable(); + const after = tmux.liveSessions(); + process.stdout.write(JSON.stringify({ + isSet: live instanceof Set, + keeps: live.has("agenthub-lane-${lane}"), + other: live.has("agenthub-lane-999999"), + keepsAfter: after.has("agenthub-lane-${lane}"), + first, slowest, stallMs: loop.max / 1e6, + sudos: require("node:fs").readFileSync(${JSON.stringify(spawned)}, "utf8").trim().split("\\n").length, + git: time(() => runAs.execIn(proj, "git", ["status"])), + write: time(() => runAs.writeAs(runAs.accountForPath(proj), proj + "/f.txt", "", "x")), + })); + })();`, { PATH: `${bin}:${process.env.PATH}` } ); assert.equal(child.signal, null, "nothing hung"); @@ -264,8 +285,14 @@ describe("a member's stopped tmux server or planted FIFO", () => { assert.equal(r.isSet, true, "the host's answer still counts"); assert.equal(r.keeps, true); assert.equal(r.other, false); - assert.ok(r.first.ms < 10_000, `first list: ${r.first.ms} ms`); - assert.ok(r.again.ms < 500, `backed off: ${r.again.ms} ms`); + assert.equal(r.keepsAfter, true, "still unknown once the hung list was killed"); + // Listing a member never waits for them: no poll, no event-loop turn + // takes anything like the 3 s a hung sudo is allowed. + assert.ok(r.first.ms < 100, `first list: ${r.first.ms} ms`); + assert.ok(r.slowest < 100, `slowest poll: ${r.slowest} ms`); + assert.ok(r.stallMs < 500, `event loop stalled ${r.stallMs} ms`); + // ONE sudo, killed at the timeout, then backed off — not one per poll. + assert.equal(r.sudos, 1); assert.equal(r.git.code, "ETIMEDOUT"); assert.equal(r.write.code, "ETIMEDOUT"); } finally { @@ -434,9 +461,12 @@ describe("who a pane runs as", () => { assert.throws(() => auth.assignWorkspace(lane, "ana"), { code: "EOWNERFOLDER" }); // One that got there anyway (assigned before this rule, or a demoted admin's). const theirs = addLane(path.join(HOST, "x"), "mia"); - assert.throws(() => pty.spawnRun({ laneId: theirs, cwd: path.join(HOST, "x"), tool: "claude" }), { - code: "EOWNERFOLDER", - }); + assert.throws( + () => pty.spawnRun({ laneId: theirs, cwd: path.join(HOST, "x"), tool: "claude" }), + { + code: "EOWNERFOLDER", + } + ); assert.throws(() => pty.spawnShell({ laneId: theirs, cwd: path.join(HOST, "x") }), { code: "EOWNERFOLDER", }); diff --git a/server/lib/tmux.js b/server/lib/tmux.js index 4d2c0c98..52f742e2 100644 --- a/server/lib/tmux.js +++ b/server/lib/tmux.js @@ -4,8 +4,9 @@ * feature. Every dashboard-managed session is named `agenthub-lane-` (see * `pty-run.js`) so a real terminal can attach to the exact same session with * `tmux attach -t agenthub-lane-` (or `agenthub lanes shell`). Never builds a - * shell string — every call is `execFileSync("tmux", [...argv])` with an - * explicit argument array (matches this repo's rule for git in worktree.js). + * shell string — every call is `execFileSync("tmux", [...argv])` (a member's + * listing: `execFile`) with an explicit argument array (matches this repo's + * rule for git in worktree.js). * No call composes a shell command line at all, `sendCommand` included: it * takes over an idle pane with `respawn-pane -- ` rather than typing a * quoted line into that pane's shell, which keeps an API key passed in `env` @@ -21,13 +22,14 @@ * that socket's folder, and a host client on it could be handed a fake server * or replaced by a member's command (`detach-client -E`). sudo logs its * command line, so a session's environment and argv travel in a launch file - * (`run-as.launchArgv`) and a paste on stdin. Listing covers every server. + * (`run-as.launchArgv`) and a paste on stdin. Listing covers every server; + * a member's runs in the background and a poll reads its last answer. * With no member (local mode, S3 not installed, the host's own workspaces) * nothing changes. * @author Nguyễn Ngọc Trí Vĩ */ -const { execFileSync } = require("node:child_process"); +const { execFile, execFileSync } = require("node:child_process"); // Test seam: swap the exec implementation so unit tests never invoke a real // tmux binary. Mirrors run-spawner.js's __injectChildForTest/__reset style. @@ -67,39 +69,77 @@ let scopeImpl = realScopeImpl; // stdin). Seams, so tests never sudo. let accountImpl = (name) => require("./run-as").accountForSession(name); // A member's server is theirs: they can stop it (SIGSTOP) or fill it with -// sessions, and every call here blocks the event loop. So each call on it is -// bounded (time, output), and a server that timed out or flooded us is not -// asked again for MEMBER_BACKOFF_MS — its panes read as UNKNOWN meanwhile. -// ponytail: a frozen member still costs one MEMBER_TIMEOUT_MS block per -// backoff; move member calls to async execFile if that ever shows. +// sessions. So each call on it is bounded (time, output), and a server that +// timed out or flooded us is not asked again for MEMBER_BACKOFF_MS — its +// panes read as UNKNOWN meanwhile. LISTING (every poll: GET /api/run and +// /api/lanes, the heal beat, the liveness reaper) never waits for a member: +// it runs async (`memberListImpl`) into `listed`, which the poll reads. A +// call a user's click makes (new-session, send-keys, kill, has-session) +// stays synchronous. +// ponytail: a click on a frozen member's pane (and a pane-token / gateway +// has-session check) still blocks up to MEMBER_TIMEOUT_MS once per backoff; +// make those async too if that ever shows. const MEMBER_TIMEOUT_MS = 3000; const MEMBER_BACKOFF_MS = 60_000; -const MEMBER_OPTS = { ...EXEC_OPTS, timeout: MEMBER_TIMEOUT_MS, killSignal: "SIGKILL", maxBuffer: 1 << 20 }; -const realMemberImpl = (acct, args, input) => { +const MEMBER_OPTS = { + ...EXEC_OPTS, + timeout: MEMBER_TIMEOUT_MS, + killSignal: "SIGKILL", + maxBuffer: 1 << 20, +}; +const { stdio: _piped, ...MEMBER_LIST_OPTS } = MEMBER_OPTS; // execFile always pipes +const sudoTmux = (acct, args) => { const runAs = require("./run-as"); - const [cmd, argv] = runAs.sudoArgv(acct, "tmux", ["-S", runAs.tmuxSocket(acct), ...args]); + return runAs.sudoArgv(acct, "tmux", ["-S", runAs.tmuxSocket(acct), ...args]); +}; +const realMemberImpl = (acct, args, input) => { + const [cmd, argv] = sudoTmux(acct, args); return execFileSync(cmd, argv, input == null ? MEMBER_OPTS : { ...MEMBER_OPTS, input }); }; +// The same, async, with execFileSync's error shape: `status` = exit code, +// ETIMEDOUT / ENOBUFS for a kill at the timeout / a flood. +const realMemberListImpl = (acct, args) => + new Promise((resolve, reject) => { + const [cmd, argv] = sudoTmux(acct, args); + execFile(cmd, argv, MEMBER_LIST_OPTS, (err, out) => { + if (!err) return resolve(out); + if (typeof err.code === "number") err.status = err.code; + if (err.killed) err.code = "ETIMEDOUT"; + if (err.code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER") err.code = "ENOBUFS"; + reject(err); + }); + }); let memberImpl = realMemberImpl; +let memberListImpl = realMemberListImpl; let unreachable = new Map(); // member login → backoff end -function memberCall(acct, args, input) { +function reachable(acct) { if ((unreachable.get(acct.login) || 0) > Date.now()) { throw Object.assign(new Error(`${acct.login}: tmux server not answering`), { code: "ETMUXHUNG", }); } +} +function noteHang(acct, err) { + if (err.code === "ETIMEDOUT" || err.code === "ENOBUFS") { + unreachable.set(acct.login, Date.now() + MEMBER_BACKOFF_MS); + } +} +function memberCall(acct, args, input) { + reachable(acct); try { return memberImpl(acct, args, input); } catch (err) { - if (err.code === "ETIMEDOUT" || err.code === "ENOBUFS") { - unreachable.set(acct.login, Date.now() + MEMBER_BACKOFF_MS); - } + noteHang(acct, err); throw err; } } +/** Test seam. A sync `member` fake also answers the async listing. */ function __setAccountImpl(fn, member) { accountImpl = fn; - if (member) memberImpl = member; + if (member) { + memberImpl = member; + memberListImpl = (acct, args) => new Promise((resolve) => resolve(member(acct, args))); + } } function accountOf(name) { try { @@ -128,6 +168,7 @@ function __setExecImpl(fn, { exact = true } = {}) { exactTargets = exact; listed = new Map(); + listGen++; } function __setScopeImpl(fn) { scopeImpl = fn; @@ -137,8 +178,10 @@ function __reset() { scopeImpl = realScopeImpl; accountImpl = (name) => require("./run-as").accountForSession(name); memberImpl = realMemberImpl; + memberListImpl = realMemberListImpl; exactTargets = null; listed = new Map(); + listGen++; unreachable = new Map(); } @@ -149,34 +192,90 @@ function __reset() { // own can read as live for up to a second. "No server" (exit 1) is an answer // too and is shared; any other failure is not. const LIST_TTL_MS = 1000; +const LIST_ARGS = ["list-sessions", "-F", "#{session_name}"]; let listed = new Map(); // member login ("" = ours) → { at, out } | { at, err } +let listGen = 0; // bumped by a change this process makes: drops lists in flight function listOutput(acct = null) { - const key = acct ? acct.login : ""; - const hit = listed.get(key); + if (acct) return memberListOutput(acct); + const hit = listed.get(""); if (hit && Date.now() - hit.at < LIST_TTL_MS) { if (hit.err) throw hit.err; return hit.out; } try { - const list = ["list-sessions", "-F", "#{session_name}"]; - let out = String(acct ? memberCall(acct, list) : execImpl(list)); + const out = String(execImpl(LIST_ARGS)); + listed.set("", { at: Date.now(), out }); + return out; + } catch (err) { + if (err.status === 1 || err.code === "ENOENT") listed.set("", { at: Date.now(), err }); + else listed.delete(""); + throw err; + } +} + +// A member's list is never waited for: a read answers from `listed` and, +// once that is LIST_TTL_MS old, starts one async re-list. Until a first +// answer — or once the last is MEMBER_LIST_MAX_AGE_MS old (nobody polled) — +// or while their server fails, it throws: their panes are UNKNOWN. A session +// this process creates or kills is written into the answer at once (`noteSession`). +const MEMBER_LIST_MAX_AGE_MS = 15_000; // > the 10 s heal beat +const refreshing = new Set(); // member logins with a list in flight +function memberListOutput(acct) { + const hit = listed.get(acct.login); + const age = hit ? Date.now() - hit.at : Infinity; + if (age >= LIST_TTL_MS) refreshList(acct); + if (age >= MEMBER_LIST_MAX_AGE_MS) { + throw Object.assign(new Error(`${acct.login}: tmux list pending`), { code: "ETMUXPENDING" }); + } + if (hit.err) throw hit.err; + return hit.out; +} +function refreshList(acct) { + const key = acct.login; + if (refreshing.has(key)) return; + refreshing.add(key); + const gen = listGen; + const hangs = unreachable; // a test's __reset starts a new one + const done = (entry) => { + refreshing.delete(key); + if (gen === listGen) listed.set(key, { at: Date.now(), ...entry }); + }; + new Promise((resolve) => { + reachable(acct); + resolve(memberListImpl(acct, LIST_ARGS)); + }).then( // A member's server answers for their own workspaces only: a name it // lists that belongs to anyone else (a server the member runs to lie // about another pane) is not believed. - if (acct) - out = out - .split("\n") - .filter((n) => ownedBy(n.trim(), acct)) - .join("\n"); - listed.set(key, { at: Date.now(), out }); - return out; - } catch (err) { - // A member's failure is shared too: a broken server of theirs is not - // asked again on every poll. - if (acct || err.status === 1 || err.code === "ENOENT") listed.set(key, { at: Date.now(), err }); - else listed.delete(key); - throw err; - } + (out) => + done({ + out: String(out) + .split("\n") + .filter((n) => ownedBy(n.trim(), acct)) + .join("\n"), + }), + (err) => { + if (hangs === unreachable) noteHang(acct, err); + // "No server" is an empty list; any other failure is shared too, so a + // broken server of theirs is not asked again on every poll. + done(err.status === 1 || err.code === "ENOENT" ? { out: "" } : { err }); + } + ); +} +/** A session this process made or ended: ours re-lists, a member's answer is + * corrected in place (an unknown one stays unknown). */ +function noteSession(name, acct, alive) { + listGen++; + listed.delete(""); + const hit = acct && listed.get(acct.login); + if (!hit || hit.err) return; + const names = hit.out.split("\n").filter((n) => n.trim() && n.trim() !== name); + if (alive) names.push(name); + listed.set(acct.login, { at: hit.at, out: names.join("\n") }); +} +/** Test seam: resolves once no member list is in flight. */ +async function __settle() { + while (refreshing.size) await new Promise((r) => setImmediate(r)); } function ownedBy(name, acct) { @@ -273,7 +372,7 @@ function envFlags(env) { function newSession({ name, cwd, argv, env }) { // No argv = tmux's own default shell (a Windows Terminal pane). const cmd = argv.length ? ["--", ...argv] : []; - listed = new Map(); + noteSession(name, null, false); const args = ["new-session", "-d", "-s", name, "-c", cwd, ...envFlags(env), ...cmd]; const acct = accountOf(name); if (acct) { @@ -281,6 +380,7 @@ function newSession({ name, cwd, argv, env }) { // the launch file, never on sudo's logged command line. const launch = require("./run-as").launchArgv(acct, env, argv); memberCall(acct, ["new-session", "-d", "-s", name, "-c", cwd, "--", ...launch]); + noteSession(name, acct, true); } else if (scopeImpl && process.platform === "linux" && !serverPid()) { try { scopeImpl(args); @@ -449,12 +549,12 @@ function pasteText(name, text) { /** Idempotent — a session that's already gone is not an error. */ function killSession(name) { - listed = new Map(); try { onServerOf(name, ["kill-session", "-t", session(name)]); } catch { /* already gone */ } + noteSession(name, accountOfQuiet(name), false); } /** Session names starting with `prefix`. Empty array if tmux has no server running at all. */ @@ -567,5 +667,6 @@ module.exports = { memberAttach, __setExecImpl, __setAccountImpl, + __settle, __reset, };