From a0e21109b25d0410ec856a68cd467b009c4b19dc Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 6 Nov 2026 03:03:05 +0100 Subject: [PATCH] fix(adopt): key adopted targets by location, not just socket and name Session 0 in two containers, or one name on this machine and on an ssh host with the same uid, shared a (socket, name) pair, so opening one returned the other's tab and its row read "Go to tab". foreignTargetKey() is now the one key, built from the location, the host identity, the socket and the name, and both the one-wrapper check and adoptedBy read it. Both also read live sessions instead of mux records, which only exist once the first attach starts the pane. An in-flight mark closes the rest of that window: the route awaits the registries and the layout lock before the session lands in the map. Docker targets are keyed per engine for the same reason, or a second daemon's container of one name was never scanned. Co-Authored-By: Claude Opus 5.5 --- src/foreign-tmux-discovery.ts | 9 +- src/foreign-tmux.ts | 50 ++++++++- src/web/routes/mux-routes.ts | 20 ++-- src/web/routes/session-routes.ts | 73 ++++++++---- test/mocks/mock-session.ts | 3 + test/routes/foreign-adoption-routes.test.ts | 116 +++++++++++++++++++- 6 files changed, 230 insertions(+), 41 deletions(-) diff --git a/src/foreign-tmux-discovery.ts b/src/foreign-tmux-discovery.ts index bd66b0a7..6c5e46a2 100644 --- a/src/foreign-tmux-discovery.ts +++ b/src/foreign-tmux-discovery.ts @@ -184,8 +184,11 @@ function dockerTargets(cases: DockerCase[], hosts: DockerHost[]): DockerTargetIn const seen = new Set(); for (const c of cases) { const container = c.container || `codeman-case-${c.name}`; - if (seen.has(container)) continue; - seen.add(container); + // Per engine, not global: two daemons can each run a container of one name, + // and collapsing them here would leave the second one unscanned. + const key = `${c.hostId}\u0000${container}`; + if (seen.has(key)) continue; + seen.add(key); const host = byId.get(c.hostId); out.push({ hostId: c.hostId, @@ -230,7 +233,7 @@ export async function discoverDockerForeign(cases: DockerCase[]): Promise { // Multi-user: this recovery/debug surface exposes every user's tmux + workdirs → admin-only // (requireAdmin is a no-op allow-all in single-user mode, so flag-off is unchanged). @@ -54,8 +55,9 @@ export function registerMuxRoutes(app: FastifyInstance, ctx: InfraPort): void { * per target, and having the home page fan those out on every load is the one * cost this design refuses to pay. * - * `adoptedBy` is filled from the live mux sessions, so a target Codeman already - * wraps renders as "open" rather than offering a second wrapper. + * `adoptedBy` is filled from the live sessions, so a target Codeman already wraps + * renders as "open" rather than offering a second wrapper. Sessions and not mux + * records: the tmux record only exists once the tab's first attach starts the pane. */ app.get('/api/mux/foreign', async (req, reply) => { if (isMultiUserMode() && !requireAdmin(req, reply)) return; @@ -77,18 +79,16 @@ export function registerMuxRoutes(app: FastifyInstance, ctx: InfraPort): void { remoteHosts: wantRemote ? remoteHosts : undefined, }); - // Match on the (socket, session) pair rather than on our opaque candidate id: - // the id encodes a host key that a restored wrapper does not carry, while the - // pair is exactly what the wrapper stores and what it re-attaches to. + // The same key the adopt route's one-wrapper check reads (see foreignTargetKey). const wrapped = new Map(); - for (const m of ctx.mux.getSessions()) { - if (m.adopt) wrapped.set(`${m.adopt.socketPath}\u0000${m.adopt.targetSession}`, m.sessionId); + for (const session of ctx.sessions.values()) { + if (session.adopt) wrapped.set(foreignTargetKey(adoptTargetIdentity(session.adopt)), session.id); } return { sessions: result.sessions.map((f) => ({ ...f, - adoptedBy: wrapped.get(`${f.socketPath}\u0000${f.sessionName}`), + adoptedBy: wrapped.get(foreignTargetKey(f)), })), scannedAt: result.scannedAt, notes: result.notes, diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 884e462c..e61bf953 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -41,9 +41,10 @@ import { readAllDockerCases, readAllRemoteHosts, } from '../../foreign-tmux-discovery.js'; -import { foreignViewSessionName } from '../../foreign-tmux.js'; +import { adoptTargetIdentity, foreignTargetKey, foreignViewSessionName } from '../../foreign-tmux.js'; import { requireAdmin } from '../route-helpers.js'; import type { SessionAdopt } from '../../types/session.js'; +import type { ForeignTmuxSession } from '../../types/foreign-tmux.js'; import { GuardedInputBusyError, Session, @@ -1499,6 +1500,21 @@ export function registerSessionRoutes( // ========== Adopt a foreign tmux session ========== + /** + * The wrapper pane for an adopted session runs `tmux attach`, never the agent, + * so it needs no workspace, and must not borrow the foreign one. Same value + * `resolveMuxAttachCwd()` gives remote and docker panes. + */ + const ADOPTED_WRAPPER_WORKING_DIR = '/tmp'; + + /** + * Targets with a wrapper being created right now. The live-session check alone + * leaves a gap: between it and `addSession` the route awaits the host registries, + * the history config and the owner's layout lock, so two devices opening one row + * could each pass it. + */ + const adoptsInFlight = new Set(); + /** * Wrap a tmux session a HUMAN started (local, in a container, or over ssh) in a * Codeman session, so it appears as a tab and can be driven from the browser. @@ -1518,24 +1534,14 @@ export function registerSessionRoutes( * double-click on one device. * 4. **Admin-only under multi-user.** Discovery already is (it exposes other * users' processes), and adopting someone's `shell` is arbitrary execution - * as the server account — which is exactly what the `can-bypass-permissions` + * as the server account, which is exactly what the `can-bypass-permissions` * grant gates elsewhere. The admin gate subsumes it, so there is deliberately * no second grant check here. */ - /** - * The wrapper pane for an adopted session runs `tmux attach`, never the agent, - * so it needs no workspace — and must not borrow the foreign one. Same value - * `resolveMuxAttachCwd()` gives remote and docker panes. - */ - const ADOPTED_WRAPPER_WORKING_DIR = '/tmp'; - app.post('/api/sessions/adopt', async (req, reply) => { if (isMultiUserMode() && !requireAdmin(req, reply)) return; const owner = ownerFor(req); - const capMsg = sessionCapacityMessage(ctx.sessions, owner); - if (capMsg) return createErrorResponse(ApiErrorCode.SESSION_BUSY, capMsg); - const body = parseBody(AdoptForeignSessionSchema, req.body, 'Invalid request body'); // Re-resolve rather than trust: point 1 and 2 above. @@ -1553,25 +1559,48 @@ export function registerSessionRoutes( // flaky link makes a perfectly live remote session read as deleted. Measured // against a real VM whose ssh path dropped ~10% of connections: clicking // Open failed with "no longer there" while the session was sitting right - // there. Discovery already knows which it was — it wrote a note — so say so. + // there. Discovery already knows which it was: it wrote a note, so say so. const reach = found.notes.filter((n) => !/skipped/.test(n)); return createErrorResponse( ApiErrorCode.NOT_FOUND, reach.length - ? `Could not reach it just now (${reach.join('; ')}). It may still be running — try again.` + ? `Could not reach it just now (${reach.join('; ')}). It may still be running, try again.` : 'That tmux session is no longer there. Refresh the list and try again.' ); } - // Point 3 — one wrapper per (socket, session). - const existing = ctx.mux - .getSessions() - .find((m) => m.adopt?.socketPath === target.socketPath && m.adopt?.targetSession === target.sessionName); - if (existing) { - const live = ctx.sessions.get(existing.sessionId); - if (live) return { session: ctx.getSessionStateWithRespawn(live), alreadyAdopted: true }; + // Point 3: one wrapper per target. Live sessions and not mux records, which + // only exist once the first attach starts the pane, so reading those let a + // second adopt through before the first tab had started. + const targetKey = foreignTargetKey(target); + for (const session of ctx.sessions.values()) { + if (session.adopt && foreignTargetKey(adoptTargetIdentity(session.adopt)) === targetKey) { + return { session: ctx.getSessionStateWithRespawn(session), alreadyAdopted: true }; + } + } + if (adoptsInFlight.has(targetKey)) { + return createErrorResponse(ApiErrorCode.SESSION_BUSY, 'That tmux session is already being opened'); } + // After the reuse above, which creates nothing and so must not be refused at capacity. + const capMsg = sessionCapacityMessage(ctx.sessions, owner); + if (capMsg) return createErrorResponse(ApiErrorCode.SESSION_BUSY, capMsg); + + adoptsInFlight.add(targetKey); + try { + return await createAdoptedSession(req, target, owner, body); + } finally { + adoptsInFlight.delete(targetKey); + } + }); + + /** Everything past the guards in `POST /api/sessions/adopt`, held under its in-flight mark. */ + async function createAdoptedSession( + req: FastifyRequest, + target: ForeignTmuxSession, + owner: string | undefined, + body: { name?: string; parentSessionId?: string } + ) { // Connection facts are copied onto the session rather than referenced by id: // a wrapper restored after a server restart must be able to rebuild its // command even if the host registry was edited in the meantime. @@ -1648,7 +1677,7 @@ export function registerSessionRoutes( const lightState = ctx.getSessionStateWithRespawn(session); ctx.broadcast(SseEvent.SessionCreated, lightState); return { session: lightState, adopted: true }; - }); + } // ========== Rename Session ========== diff --git a/test/mocks/mock-session.ts b/test/mocks/mock-session.ts index dd893e4c..f1132549 100644 --- a/test/mocks/mock-session.ts +++ b/test/mocks/mock-session.ts @@ -11,6 +11,7 @@ import type { SessionStatus, SessionRemote, } from '../../src/types.js'; +import type { SessionAdopt } from '../../src/types/session.js'; import type { GitFetchStatus } from '../../src/git-fetch.js'; /** @@ -33,6 +34,8 @@ export class MockSession extends EventEmitter { * boolean and the tests only ever need to set it. */ isAdopted: boolean = false; + /** Mirrors `Session.adopt`, the record the one-wrapper-per-target check keys on. */ + adopt?: SessionAdopt; /** Mirrors Session.attachmentHistory (the attachment panel's source of truth). */ attachmentHistory: SessionAttachmentHistoryItem[] = []; /** Mirrors Session.getAttachmentHistoryForPersist(). */ diff --git a/test/routes/foreign-adoption-routes.test.ts b/test/routes/foreign-adoption-routes.test.ts index 41ad6780..0c4e5cde 100644 --- a/test/routes/foreign-adoption-routes.test.ts +++ b/test/routes/foreign-adoption-routes.test.ts @@ -37,6 +37,8 @@ import { registerSessionRoutes } from '../../src/web/routes/session-routes.js'; import { registerRespawnRoutes } from '../../src/web/routes/respawn-routes.js'; import { registerRalphRoutes } from '../../src/web/routes/ralph-routes.js'; import { registerMuxRoutes } from '../../src/web/routes/mux-routes.js'; +import { adoptTargetIdentity, foreignTargetKey } from '../../src/foreign-tmux.js'; +import type { SessionAdopt } from '../../src/types/session.js'; const SID = 'adopted-1'; @@ -188,6 +190,11 @@ describe('POST /api/sessions/adopt', () => { }); }); +/** The adoption record of a local wrapper, as the route writes it. */ +function localAdopt(socketPath: string, targetSession: string): SessionAdopt { + return { location: 'local', socketPath, targetSession, viewSession: 'codeman-view-00000000' }; +} + describe('a successful adoption', () => { /** A candidate whose cwd is BOTH unsafe for SAFE_PATH_PATTERN and not on this host. */ const CANDIDATE = { @@ -230,13 +237,11 @@ describe('a successful adoption', () => { }); it('reuses the existing wrapper rather than making a second one for the same target', async () => { - // Keyed on (socket, session) and not on our opaque candidate id: a wrapper - // restored after a server restart does not carry that id. + // Keyed on the target and not on our opaque candidate id: a wrapper restored + // after a server restart does not carry that id. const { app, ctx } = await createRouteTestHarness(registerSessionRoutes); ctx.sessions.set('wrapper-1', ctx._session); - ctx.mux.getSessions = vi.fn(() => [ - { sessionId: 'wrapper-1', adopt: { socketPath: CANDIDATE.socketPath, targetSession: CANDIDATE.sessionName } }, - ]) as unknown as typeof ctx.mux.getSessions; + ctx._session.adopt = localAdopt(CANDIDATE.socketPath, CANDIDATE.sessionName); const res = await app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'cand-1' } }); expect(res.statusCode).toBe(200); expect(res.json().alreadyAdopted).toBe(true); @@ -244,6 +249,39 @@ describe('a successful adoption', () => { await app.close(); }); + it('sees a wrapper whose tab has not started yet, which has no mux record to find', async () => { + const { app, ctx } = await createRouteTestHarness(registerSessionRoutes); + const first = await app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'cand-1' } }); + expect(first.json().adopted).toBe(true); + expect(ctx.mux.getSessions()).toEqual([]); + const second = await app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'cand-1' } }); + expect(second.json().alreadyAdopted).toBe(true); + expect(ctx.addSession).toHaveBeenCalledTimes(1); + await app.close(); + }); + + it('refuses a second adopt of a target whose wrapper is still being created', async () => { + // Two devices on one row: both pass the live-session check, because the first + // is parked on the layout lock and not yet in the map. + const { app, ctx } = await createRouteTestHarness(registerSessionRoutes); + const add = ctx.addSession.getMockImplementation()!; + let release!: () => void; + const held = new Promise((resolve) => (release = resolve)); + ctx.addSession.mockImplementationOnce(async (s: never) => { + await held; + return add(s); + }); + const first = app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'cand-1' } }); + await vi.waitFor(() => expect(ctx.addSession).toHaveBeenCalledTimes(1)); + const second = await app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'cand-1' } }); + expect(second.json().success).toBe(false); + expect(second.json().errorCode).toBe('SESSION_BUSY'); + release(); + expect((await first).json().adopted).toBe(true); + expect(ctx.addSession).toHaveBeenCalledTimes(1); + await app.close(); + }); + it('says "could not reach it" rather than "gone" when discovery left a note', async () => { // On a flaky link the session is alive and sitting right there; reporting // it as deleted is the wrong answer, not just an unhelpful one. @@ -271,3 +309,71 @@ describe('a successful adoption', () => { await app.close(); }); }); + +describe('same-named sessions in different locations', () => { + // Containers commonly run tmux as root with default names, so `0` in two + // containers is the same (socket, name) pair, and the same goes for this machine + // and an ssh host with the same uid. + const SHARED = { + socketPath: '/tmp/tmux-0/default', + sessionName: '0', + mode: 'shell' as const, + workingDir: '/workspace', + command: 'bash', + createdAt: 1, + windows: 1, + attached: false, + }; + const IN_A = { ...SHARED, id: 'in-a', location: 'docker' as const, hostId: 'h1', containerName: 'box-a' }; + const IN_B = { ...SHARED, id: 'in-b', location: 'docker' as const, hostId: 'h1', containerName: 'box-b' }; + const OTHER_ENGINE = { ...SHARED, id: 'in-c', location: 'docker' as const, hostId: 'h2', containerName: 'box-a' }; + const ON_THIS_HOST = { ...SHARED, id: 'here', location: 'local' as const }; + + beforeEach(() => { + discoverForeignSessions.mockResolvedValue({ sessions: [IN_A, IN_B, OTHER_ENGINE, ON_THIS_HOST], notes: [] }); + }); + + it('gives each location its own key, and a wrapper the key of its candidate', () => { + const keys = [IN_A, IN_B, OTHER_ENGINE, ON_THIS_HOST].map(foreignTargetKey); + expect(new Set(keys).size).toBe(4); + expect( + foreignTargetKey( + adoptTargetIdentity({ + ...localAdopt(SHARED.socketPath, SHARED.sessionName), + location: 'docker', + docker: { hostId: 'h1', label: 'one', engine: 'docker', containerName: 'box-b' }, + }) + ) + ).toBe(foreignTargetKey(IN_B)); + }); + + it('adopting one never returns the tab of another', async () => { + const { app, ctx } = await createRouteTestHarness(registerSessionRoutes); + const a = await app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'in-a', docker: true } }); + const b = await app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'in-b', docker: true } }); + const local = await app.inject({ method: 'POST', url: '/api/sessions/adopt', payload: { id: 'here' } }); + for (const res of [a, b, local]) expect(res.json().adopted).toBe(true); + expect(ctx.addSession).toHaveBeenCalledTimes(3); + const ids = [a, b, local].map((res) => res.json().session.id); + expect(new Set(ids).size).toBe(3); + await app.close(); + }); + + it('marks only the row that is really open as open', async () => { + const { app, ctx } = await createRouteTestHarness(registerMuxRoutes); + ctx.sessions.set('wrapper-b', { + id: 'wrapper-b', + adopt: { + ...localAdopt(SHARED.socketPath, SHARED.sessionName), + location: 'docker', + docker: { hostId: 'h1', label: 'one', engine: 'docker', containerName: 'box-b' }, + }, + } as never); + const res = await app.inject({ method: 'GET', url: '/api/mux/foreign' }); + const adoptedBy = Object.fromEntries( + (res.json().sessions as { id: string; adoptedBy?: string }[]).map((s) => [s.id, s.adoptedBy]) + ); + expect(adoptedBy).toEqual({ 'in-a': undefined, 'in-b': 'wrapper-b', 'in-c': undefined, here: undefined }); + await app.close(); + }); +});