mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
feat(docker): let one adopted container back several cases in different dirs
Once a container is adopted, it could not be adopted a second time. But a
container usually holds more than one project directory, and opening a case for
another one had no path forward except starting a second container — precisely
what adoption exists to avoid.
The original reason was in a comment: two cases sharing an adopted container
would make one case's teardown race the other's launch on the same tmux server.
That reason does not hold. The in-container tmux session name is
dockerTmuxSessionName(sessionId), i.e. codeman-dkr-<id8>, keyed by SESSION and
not by case, and buildDockerKillCommand tears down exactly that name, so killing
A never touches B — hosting multiple sessions is what a tmux server is for.
The other three routes into an adopted container's lifecycle do not pass through
here either, confirmed one by one: the stop and remove builders throw outright;
recreate refuses `owned === false` before it even resolves the container name;
and orphan reaping filters on `label=codeman.managed=1`, which a user-built
container does not carry — a structural exclusion.
That leaves exactly three cases worth refusing, none of them tmux-related, split
into the pure, unit-tested classifyAdoptContainerConflict:
- owned-case the container belongs to a Codeman-created case, whose lifecycle
Codeman manages: one recreate or delete there would pull the
container out from under the adopting case.
⚠️ `owned` may be absent and absent means owned (cases predate
the field), so the test is `!== false`, not truthiness.
- other-owner already adopted by a different user. Adoption hands out a shell
inside someone else's container.
- duplicate same container, same directory. The second case would behave
identically to the first, so name the existing one rather than
silently minting a twin. A different in-container directory is
the case this change exists to support and passes.
(cherry picked from commit 1cb6bde891)
This commit is contained in:
committed by
Codeman maintainer
parent
e5684d0bba
commit
cbb7f635ff
@@ -293,6 +293,64 @@ export function toSessionDocker(host: DockerHost, dockerCase: DockerCase): Sessi
|
|||||||
return session;
|
return session;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Which existing case, if any, blocks adopting `container` at `containerWorkdir`.
|
||||||
|
*
|
||||||
|
* One container may back SEVERAL adopted cases, each pointing at a different
|
||||||
|
* directory inside it — that is the whole reason to adopt the same container
|
||||||
|
* twice, and it is safe because the in-container tmux session is named per
|
||||||
|
* SESSION (`dockerTmuxSessionName`, `codeman-dkr-<id8>`) and not per case, so a
|
||||||
|
* session teardown kills exactly one session and its siblings on the shared
|
||||||
|
* in-container tmux server are untouched. Nothing else reaches an adopted
|
||||||
|
* container's lifecycle either: stop/remove throw at the builder, recreate
|
||||||
|
* refuses `owned === false`, and the orphan reaper filters on the
|
||||||
|
* `codeman.managed=1` label that only Codeman-created containers carry.
|
||||||
|
*
|
||||||
|
* So the conflicts that remain are NOT about the tmux server:
|
||||||
|
* - `owned-case` the container backs a case Codeman CREATED, whose lifecycle
|
||||||
|
* it owns; a recreate or delete there would destroy the
|
||||||
|
* adopted case's container out from under it.
|
||||||
|
* - `other-owner` already adopted by a different user. Adoption hands out a
|
||||||
|
* shell inside someone else's container, so it stays scoped.
|
||||||
|
* - `duplicate` same container AND same directory: the second case would
|
||||||
|
* behave identically to the first, so name the first instead
|
||||||
|
* of silently creating a twin. A DIFFERENT directory is the
|
||||||
|
* supported case and returns null.
|
||||||
|
*/
|
||||||
|
export type AdoptContainerConflict =
|
||||||
|
| { kind: 'owned-case'; caseName: string }
|
||||||
|
| { kind: 'other-owner'; caseName: string }
|
||||||
|
| { kind: 'duplicate'; caseName: string }
|
||||||
|
| null;
|
||||||
|
|
||||||
|
export function classifyAdoptContainerConflict(params: {
|
||||||
|
container: string;
|
||||||
|
/** Directory inside the container this adoption targets (already defaulted). */
|
||||||
|
containerWorkdir: string;
|
||||||
|
existing: ReadonlyArray<
|
||||||
|
Pick<DockerCase, 'name' | 'container' | 'containerWorkdir' | 'hostWorkspacePath' | 'owned' | 'owner'>
|
||||||
|
>;
|
||||||
|
/** Owner visibility test (canAccessOwned bound to the caller). */
|
||||||
|
canAccess: (owner?: string) => boolean;
|
||||||
|
}): AdoptContainerConflict {
|
||||||
|
const { container, containerWorkdir, existing, canAccess } = params;
|
||||||
|
const sharing = existing.filter((item) => (item.container ?? dockerContainerName(item.name)) === container);
|
||||||
|
if (sharing.length === 0) return null;
|
||||||
|
|
||||||
|
// `owned` is optional and an ABSENT flag means owned (legacy cases predate the
|
||||||
|
// field), so this must test `!== false` rather than truthiness.
|
||||||
|
const owned = sharing.find((item) => item.owned !== false);
|
||||||
|
if (owned) return { kind: 'owned-case', caseName: owned.name };
|
||||||
|
|
||||||
|
const foreign = sharing.find((item) => !canAccess(item.owner));
|
||||||
|
if (foreign) return { kind: 'other-owner', caseName: foreign.name };
|
||||||
|
|
||||||
|
const twin = sharing.find((item) => (item.containerWorkdir ?? item.hostWorkspacePath) === containerWorkdir);
|
||||||
|
if (twin) return { kind: 'duplicate', caseName: twin.name };
|
||||||
|
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* An ADOPTED container is one the user built and runs themselves. Codeman may
|
* An ADOPTED container is one the user built and runs themselves. Codeman may
|
||||||
* only exec into it; it must never create, start, stop, restart or remove it.
|
* only exec into it; it must never create, start, stop, restart or remove it.
|
||||||
|
|||||||
@@ -79,6 +79,7 @@ import {
|
|||||||
dockerContainerName,
|
dockerContainerName,
|
||||||
dockerDisplayPath,
|
dockerDisplayPath,
|
||||||
probeAdoptableContainer,
|
probeAdoptableContainer,
|
||||||
|
classifyAdoptContainerConflict,
|
||||||
listDockerContainers,
|
listDockerContainers,
|
||||||
browseInContainer,
|
browseInContainer,
|
||||||
dockerAdoptProbeModes,
|
dockerAdoptProbeModes,
|
||||||
@@ -906,12 +907,35 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config
|
|||||||
) {
|
) {
|
||||||
return createErrorResponse(ApiErrorCode.ALREADY_EXISTS, 'Case already exists');
|
return createErrorResponse(ApiErrorCode.ALREADY_EXISTS, 'Case already exists');
|
||||||
}
|
}
|
||||||
// Two cases must never share one adopted container: session close kills the
|
// One container may back SEVERAL adopted cases, each pointing at a different
|
||||||
// in-container tmux by session id, but a shared adoption would let one case's
|
// directory inside it. What still blocks it, and why, lives in
|
||||||
// teardown and another's launch race over the same tmux server.
|
// classifyAdoptContainerConflict — note that none of it is about the shared
|
||||||
|
// in-container tmux server, which is safe precisely because sessions there
|
||||||
|
// are named per SESSION id (`codeman-dkr-<id8>`), never per case.
|
||||||
const container = dockerCase.container;
|
const container = dockerCase.container;
|
||||||
if (dockerCases.some((item) => (item.container ?? dockerContainerName(item.name)) === container)) {
|
const conflict = classifyAdoptContainerConflict({
|
||||||
return createErrorResponse(ApiErrorCode.ALREADY_EXISTS, `Container "${container}" is already linked to a case`);
|
container,
|
||||||
|
containerWorkdir: dockerCase.containerWorkdir ?? dockerCase.hostWorkspacePath,
|
||||||
|
existing: dockerCases,
|
||||||
|
canAccess: (owner) => canAccessOwned(getAuthUser(req), owner),
|
||||||
|
});
|
||||||
|
if (conflict?.kind === 'owned-case') {
|
||||||
|
return createErrorResponse(
|
||||||
|
ApiErrorCode.ALREADY_EXISTS,
|
||||||
|
`Container "${container}" belongs to case "${conflict.caseName}", which Codeman created and whose lifecycle it manages. Adopt a container you started yourself, or open that case directly.`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
if (conflict?.kind === 'other-owner') {
|
||||||
|
return createErrorResponse(
|
||||||
|
ApiErrorCode.FORBIDDEN,
|
||||||
|
`Container "${container}" is already adopted by another user.`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
if (conflict?.kind === 'duplicate') {
|
||||||
|
return createErrorResponse(
|
||||||
|
ApiErrorCode.ALREADY_EXISTS,
|
||||||
|
`Case "${conflict.caseName}" already adopts "${container}" at that same directory. Point this one at another directory inside the container.`
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!isWorkingDirAllowed(getAuthUser(req), dockerCase.hostWorkspacePath)) {
|
if (!isWorkingDirAllowed(getAuthUser(req), dockerCase.hostWorkspacePath)) {
|
||||||
|
|||||||
@@ -19,6 +19,8 @@ import {
|
|||||||
checkDockerConfigDrift,
|
checkDockerConfigDrift,
|
||||||
dockerConfigHash,
|
dockerConfigHash,
|
||||||
dockerAdoptProbeModes,
|
dockerAdoptProbeModes,
|
||||||
|
classifyAdoptContainerConflict,
|
||||||
|
dockerContainerName,
|
||||||
} from '../src/docker-hosts.js';
|
} from '../src/docker-hosts.js';
|
||||||
import { enabledCliIds, getCli } from '../src/config/cli-registry/index.js';
|
import { enabledCliIds, getCli } from '../src/config/cli-registry/index.js';
|
||||||
import {
|
import {
|
||||||
@@ -451,3 +453,113 @@ describe('adopted container: a missing container means different things per owne
|
|||||||
expect(routes).toContain('...(dockerCase.owned === false ? { owned: false } : {}),');
|
expect(routes).toContain('...(dockerCase.owned === false ? { owned: false } : {}),');
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('adopted container: one container may back several cases', () => {
|
||||||
|
const base = {
|
||||||
|
type: 'docker' as const,
|
||||||
|
hostId: 'h1',
|
||||||
|
hostWorkspacePath: '/srv/work',
|
||||||
|
};
|
||||||
|
const mk = (over: Record<string, unknown>) => ({ ...base, ...over }) as never;
|
||||||
|
const mine = () => true;
|
||||||
|
|
||||||
|
it('allows a second adoption of the same container at a DIFFERENT directory', () => {
|
||||||
|
// The whole point of the feature: one container, two folders, two cases.
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: 'devbox',
|
||||||
|
containerWorkdir: '/app/api',
|
||||||
|
existing: [mk({ name: 'web', container: 'devbox', containerWorkdir: '/app/web', owned: false })],
|
||||||
|
canAccess: mine,
|
||||||
|
});
|
||||||
|
expect(conflict).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses an exact twin (same container AND same directory) and names the first case', () => {
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: 'devbox',
|
||||||
|
containerWorkdir: '/app/web',
|
||||||
|
existing: [mk({ name: 'web', container: 'devbox', containerWorkdir: '/app/web', owned: false })],
|
||||||
|
canAccess: mine,
|
||||||
|
});
|
||||||
|
expect(conflict).toEqual({ kind: 'duplicate', caseName: 'web' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('falls back to hostWorkspacePath when containerWorkdir is absent on either side', () => {
|
||||||
|
// containerWorkdir defaults to hostWorkspacePath, so an absent field on the
|
||||||
|
// stored case must compare equal to an incoming adoption that omits it too —
|
||||||
|
// otherwise the twin check silently stops firing for the default case.
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: 'devbox',
|
||||||
|
containerWorkdir: '/srv/work',
|
||||||
|
existing: [mk({ name: 'web', container: 'devbox', owned: false })],
|
||||||
|
canAccess: mine,
|
||||||
|
});
|
||||||
|
expect(conflict).toEqual({ kind: 'duplicate', caseName: 'web' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('still refuses a container backing a case Codeman CREATED', () => {
|
||||||
|
// Codeman owns that container's lifecycle: a recreate or case-delete there
|
||||||
|
// would destroy the adopted case's container out from under it.
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: 'codeman-case-web',
|
||||||
|
containerWorkdir: '/app/api',
|
||||||
|
existing: [mk({ name: 'web', container: 'codeman-case-web', owned: true })],
|
||||||
|
canAccess: mine,
|
||||||
|
});
|
||||||
|
expect(conflict).toEqual({ kind: 'owned-case', caseName: 'web' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('treats an ABSENT owned flag as owned, so legacy cases keep the old refusal', () => {
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: 'legacy',
|
||||||
|
containerWorkdir: '/app/api',
|
||||||
|
existing: [mk({ name: 'old', container: 'legacy' })],
|
||||||
|
canAccess: mine,
|
||||||
|
});
|
||||||
|
expect(conflict).toEqual({ kind: 'owned-case', caseName: 'old' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('derives the container name from the case name when the field is absent', () => {
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: dockerContainerName('web'),
|
||||||
|
containerWorkdir: '/app/api',
|
||||||
|
existing: [mk({ name: 'web', owned: true })],
|
||||||
|
canAccess: mine,
|
||||||
|
});
|
||||||
|
expect(conflict).toEqual({ kind: 'owned-case', caseName: 'web' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses a container another user already adopted', () => {
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: 'devbox',
|
||||||
|
containerWorkdir: '/app/api',
|
||||||
|
existing: [mk({ name: 'theirs', container: 'devbox', owned: false, owner: 'bob' })],
|
||||||
|
canAccess: (owner) => owner === 'alice',
|
||||||
|
});
|
||||||
|
expect(conflict).toEqual({ kind: 'other-owner', caseName: 'theirs' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('an owned case outranks a foreign adoption, so the message names the real blocker', () => {
|
||||||
|
const conflict = classifyAdoptContainerConflict({
|
||||||
|
container: 'devbox',
|
||||||
|
containerWorkdir: '/app/api',
|
||||||
|
existing: [
|
||||||
|
mk({ name: 'theirs', container: 'devbox', owned: false, owner: 'bob' }),
|
||||||
|
mk({ name: 'built', container: 'devbox', owned: true }),
|
||||||
|
],
|
||||||
|
canAccess: (owner) => owner === 'alice',
|
||||||
|
});
|
||||||
|
expect(conflict).toEqual({ kind: 'owned-case', caseName: 'built' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('leaves an unrelated container alone', () => {
|
||||||
|
expect(
|
||||||
|
classifyAdoptContainerConflict({
|
||||||
|
container: 'fresh',
|
||||||
|
containerWorkdir: '/app',
|
||||||
|
existing: [mk({ name: 'web', container: 'devbox', owned: false })],
|
||||||
|
canAccess: mine,
|
||||||
|
})
|
||||||
|
).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user