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(); + }); +});