fix: move docker mutations to the mutations resolver (#1333)

Thanks to @S3ppo on Discord for flagging this issue


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Introduced a dedicated mutation field that centralizes Docker
operations via the API.
- Added a new method for Docker-related mutations in the mutation
resolver.

- **Refactor**
- Streamlined Docker container controls by renaming the start/stop
operations for improved clarity.
	- Removed legacy fields to provide a more unified mutation interface.

- **Bug Fixes**
- Enhanced error handling during container start/stop operations to
ensure consistent behavior.

- **Tests**
- Updated test cases to reflect the new naming and behavior, ensuring
reliable Docker operation validations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

api/src/graphql/generated/api/operations.ts

@@ -507,7 +507,6 @@ export function DockerSchema(): z.ZodObject<Properties<Docker>> {
     __typename: z.literal('Docker').optional(),
     containers: z.array(DockerContainerSchema()).nullish(),
     id: z.string(),
-    mutations: DockerMutationsSchema(),
     networks: z.array(DockerNetworkSchema()).nullish()
   })
 }

api/src/graphql/generated/api/types.ts

@@ -574,7 +574,6 @@ export type Docker = Node & {
   __typename?: 'Docker';
   containers?: Maybe<Array<DockerContainer>>;
   id: Scalars['ID']['output'];
-  mutations: DockerMutations;
   networks?: Maybe<Array<DockerNetwork>>;
 };
 
@@ -850,6 +849,7 @@ export type Mutation = {
   deleteNotification: NotificationOverview;
   /** Delete a user */
   deleteUser?: Maybe<User>;
+  docker?: Maybe<DockerMutations>;
   enableDynamicRemoteAccess: Scalars['Boolean']['output'];
   login?: Maybe<Scalars['String']['output']>;
   /** Pause parity check */
@@ -2558,7 +2558,6 @@ export type DisplayResolvers<ContextType = Context, ParentType extends Resolvers
 export type DockerResolvers<ContextType = Context, ParentType extends ResolversParentTypes['Docker'] = ResolversParentTypes['Docker']> = ResolversObject<{
   containers?: Resolver<Maybe<Array<ResolversTypes['DockerContainer']>>, ParentType, ContextType>;
   id?: Resolver<ResolversTypes['ID'], ParentType, ContextType>;
-  mutations?: Resolver<ResolversTypes['DockerMutations'], ParentType, ContextType>;
   networks?: Resolver<Maybe<Array<ResolversTypes['DockerNetwork']>>, ParentType, ContextType>;
   __isTypeOf?: IsTypeOfResolverFn<ParentType, ContextType>;
 }>;
@@ -2776,6 +2775,7 @@ export type MutationResolvers<ContextType = Context, ParentType extends Resolver
   deleteArchivedNotifications?: Resolver<ResolversTypes['NotificationOverview'], ParentType, ContextType>;
   deleteNotification?: Resolver<ResolversTypes['NotificationOverview'], ParentType, ContextType, RequireFields<MutationdeleteNotificationArgs, 'id' | 'type'>>;
   deleteUser?: Resolver<Maybe<ResolversTypes['User']>, ParentType, ContextType, RequireFields<MutationdeleteUserArgs, 'input'>>;
+  docker?: Resolver<Maybe<ResolversTypes['DockerMutations']>, ParentType, ContextType>;
   enableDynamicRemoteAccess?: Resolver<ResolversTypes['Boolean'], ParentType, ContextType, RequireFields<MutationenableDynamicRemoteAccessArgs, 'input'>>;
   login?: Resolver<Maybe<ResolversTypes['String']>, ParentType, ContextType, RequireFields<MutationloginArgs, 'password' | 'username'>>;
   pauseParityCheck?: Resolver<Maybe<ResolversTypes['JSON']>, ParentType, ContextType>;

api/src/graphql/schema/types/docker/docker.graphql

@@ -9,10 +9,12 @@ type Query {
 }
 
 type DockerMutations {
-    startContainer(id: ID!): DockerContainer!
+    """ Stop a container """
-    stopContainer(id: ID!): DockerContainer!
+    stop(id: ID!): DockerContainer!
+    """ Start a container """
+    start(id: ID!): DockerContainer!
 }
 
-extend type Docker {
+type Mutation {
-    mutations: DockerMutations!
+    docker: DockerMutations
-}
+}

api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.spec.ts

@@ -19,8 +19,8 @@ describe('DockerMutationsResolver', () => {
                 {
                     provide: DockerService,
                     useValue: {
-                        startContainer: vi.fn(),
+                        start: vi.fn(),
-                        stopContainer: vi.fn(),
+                        stop: vi.fn(),
                     },
                 },
             ],
@@ -34,7 +34,7 @@ describe('DockerMutationsResolver', () => {
         expect(resolver).toBeDefined();
     });
 
-    it('should start container', async () => {
+    it('should start', async () => {
         const mockContainer: DockerContainer = {
             id: '1',
             autoStart: false,
@@ -46,14 +46,14 @@ describe('DockerMutationsResolver', () => {
             state: ContainerState.RUNNING,
             status: 'Up 2 hours',
         };
-        vi.mocked(dockerService.startContainer).mockResolvedValue(mockContainer);
+        vi.mocked(dockerService.start).mockResolvedValue(mockContainer);
 
-        const result = await resolver.startContainer('1');
+        const result = await resolver.start('1');
         expect(result).toEqual(mockContainer);
-        expect(dockerService.startContainer).toHaveBeenCalledWith('1');
+        expect(dockerService.start).toHaveBeenCalledWith('1');
     });
 
-    it('should stop container', async () => {
+    it('should stop', async () => {
         const mockContainer: DockerContainer = {
             id: '1',
             autoStart: false,
@@ -65,10 +65,10 @@ describe('DockerMutationsResolver', () => {
             state: ContainerState.EXITED,
             status: 'Exited',
         };
-        vi.mocked(dockerService.stopContainer).mockResolvedValue(mockContainer);
+        vi.mocked(dockerService.stop).mockResolvedValue(mockContainer);
 
-        const result = await resolver.stopContainer('1');
+        const result = await resolver.stop('1');
         expect(result).toEqual(mockContainer);
-        expect(dockerService.stopContainer).toHaveBeenCalledWith('1');
+        expect(dockerService.stop).toHaveBeenCalledWith('1');
     });
 });

api/src/unraid-api/graph/resolvers/docker/docker.mutations.resolver.ts

@@ -9,23 +9,23 @@ import { DockerService } from '@app/unraid-api/graph/resolvers/docker/docker.ser
 export class DockerMutationsResolver {
     constructor(private readonly dockerService: DockerService) {}
 
-    @ResolveField('startContainer')
+    @ResolveField('start')
     @UsePermissions({
         action: AuthActionVerb.UPDATE,
         resource: Resource.DOCKER,
         possession: AuthPossession.ANY,
     })
-    public async startContainer(@Args('id') id: string) {
+    public async start(@Args('id') id: string) {
-        return this.dockerService.startContainer(id);
+        return this.dockerService.start(id);
     }
 
-    @ResolveField('stopContainer')
+    @ResolveField('stop')
     @UsePermissions({
         action: AuthActionVerb.UPDATE,
         resource: Resource.DOCKER,
         possession: AuthPossession.ANY,
     })
-    public async stopContainer(@Args('id') id: string) {
+    public async stop(@Args('id') id: string) {
-        return this.dockerService.stopContainer(id);
+        return this.dockerService.stop(id);
     }
 }

api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts

@@ -69,9 +69,4 @@ describe('DockerResolver', () => {
         expect(result).toEqual(mockContainers);
         expect(dockerService.getContainers).toHaveBeenCalledWith({ useCache: false });
     });
-
-    it('should return mutations object with id', () => {
-        const result = resolver.mutations();
-        expect(result).toEqual({ id: 'docker-mutations' });
-    });
 });

api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts

@@ -35,11 +35,4 @@ export class DockerResolver {
     public async networks() {
         return this.dockerService.getNetworks({ useCache: false });
     }
-
-    @ResolveField()
-    public mutations() {
-        return {
-            id: 'docker-mutations',
-        };
-    }
 }

api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts

@@ -183,7 +183,7 @@ describe('DockerService', () => {
         mockListContainers.mockResolvedValue(mockContainers);
         mockContainer.start.mockResolvedValue(undefined);
 
-        const result = await service.startContainer('abc123def456');
+        const result = await service.start('abc123def456');
 
         expect(result).toEqual({
             id: 'abc123def456',
@@ -234,7 +234,7 @@ describe('DockerService', () => {
         mockListContainers.mockResolvedValue(mockContainers);
         mockContainer.stop.mockResolvedValue(undefined);
 
-        const result = await service.stopContainer('abc123def456');
+        const result = await service.stop('abc123def456');
 
         expect(result).toEqual({
             id: 'abc123def456',
@@ -254,7 +254,7 @@ describe('DockerService', () => {
             mounts: [],
         });
 
-        expect(mockContainer.stop).toHaveBeenCalled();
+        expect(mockContainer.stop).toHaveBeenCalledWith({ t: 10 });
         expect(mockListContainers).toHaveBeenCalledWith({
             all: true,
             size: true,
@@ -265,8 +265,8 @@ describe('DockerService', () => {
         mockListContainers.mockResolvedValue([]);
         mockContainer.start.mockResolvedValue(undefined);
 
-        await expect(service.startContainer('abc123def456')).rejects.toThrow(
+        await expect(service.start('not-found')).rejects.toThrow(
-            'Container abc123def456 not found after starting'
+            'Container not-found not found after starting'
         );
     });
 
@@ -274,8 +274,8 @@ describe('DockerService', () => {
         mockListContainers.mockResolvedValue([]);
         mockContainer.stop.mockResolvedValue(undefined);
 
-        await expect(service.stopContainer('abc123def456')).rejects.toThrow(
+        await expect(service.stop('not-found')).rejects.toThrow(
-            'Container abc123def456 not found after stopping'
+            'Container not-found not found after stopping'
         );
     });
 

api/src/unraid-api/graph/resolvers/docker/docker.service.ts

@@ -11,10 +11,11 @@ import type { ContainerPort, DockerContainer, DockerNetwork } from '@app/graphql
 import { dockerLogger } from '@app/core/log.js';
 import { pubsub, PUBSUB_CHANNEL } from '@app/core/pubsub.js';
 import { catchHandlers } from '@app/core/utils/misc/catch-handlers.js';
+import { sleep } from '@app/core/utils/misc/sleep.js';
 import { ContainerPortType, ContainerState } from '@app/graphql/generated/api/types.js';
 import { getters } from '@app/store/index.js';
 
-interface ContainerListingOptions {
+interface ContainerListingOptions extends Docker.ContainerListOptions {
     useCache: boolean;
 }
 
@@ -26,6 +27,7 @@ interface NetworkListingOptions {
 export class DockerService implements OnModuleInit {
     private client: Docker;
     private containerCache: Array<DockerContainer> = [];
+    private autoStarts: string[] = [];
     private dockerWatcher: null | typeof DockerEE = null;
     private readonly logger = new Logger(DockerService.name);
 
@@ -124,53 +126,62 @@ export class DockerService implements OnModuleInit {
         await pubsub.publish(PUBSUB_CHANNEL.INFO, this.appUpdateEvent);
     }, 500);
 
-    public async getContainers({ useCache }: ContainerListingOptions): Promise<DockerContainer[]> {
+    public transformContainer(container: Docker.ContainerInfo): DockerContainer {
+        return camelCaseKeys<DockerContainer>(
+            {
+                labels: container.Labels ?? {},
+                sizeRootFs: undefined,
+                imageId: container.ImageID,
+                state:
+                    typeof container.State === 'string'
+                        ? (ContainerState[container.State.toUpperCase()] ?? ContainerState.EXITED)
+                        : ContainerState.EXITED,
+                autoStart: this.autoStarts.includes(container.Names[0].split('/')[1]),
+                ports: container.Ports.map<ContainerPort>((port) => ({
+                    ...port,
+                    type: ContainerPortType[port.Type.toUpperCase()],
+                })),
+                command: container.Command,
+                created: container.Created,
+                mounts: container.Mounts,
+                networkSettings: container.NetworkSettings,
+                hostConfig: {
+                    networkMode: container.HostConfig.NetworkMode,
+                },
+                id: container.Id,
+                image: container.Image,
+                status: container.Status,
+            },
+            { deep: true }
+        );
+    }
+
+    public async getContainers(
+        {
+            useCache = false,
+            all = true,
+            size = true,
+            ...listOptions
+        }: Partial<ContainerListingOptions> = { useCache: false }
+    ): Promise<DockerContainer[]> {
         if (useCache && this.containerCache.length > 0) {
             this.logger.debug('Using docker container cache');
             return this.containerCache;
         }
 
         this.logger.debug('Updating docker container cache');
-
         const rawContainers = await this.client
             .listContainers({
-                all: true,
+                all,
-                size: true,
+                size,
+                ...listOptions,
             })
             // If docker throws an error return no containers
             .catch(catchHandlers.docker);
-
+        this.autoStarts = await this.getAutoStarts();
-        const autoStarts = await this.getAutoStarts();
         // Cleanup container object
         this.containerCache = rawContainers.map((container) => {
-            const names = container.Names[0];
+            const containerData: DockerContainer = this.transformContainer(container);
-            const containerData: DockerContainer = camelCaseKeys<DockerContainer>(
-                {
-                    labels: container.Labels ?? {},
-                    sizeRootFs: undefined,
-                    imageId: container.ImageID,
-                    state:
-                        typeof container.State === 'string'
-                            ? (ContainerState[container.State.toUpperCase()] ?? ContainerState.EXITED)
-                            : ContainerState.EXITED,
-                    autoStart: autoStarts.includes(names.split('/')[1]),
-                    ports: container.Ports.map<ContainerPort>((port) => ({
-                        ...port,
-                        type: ContainerPortType[port.Type.toUpperCase()],
-                    })),
-                    command: container.Command,
-                    created: container.Created,
-                    mounts: container.Mounts,
-                    networkSettings: container.NetworkSettings,
-                    hostConfig: {
-                        networkMode: container.HostConfig.NetworkMode,
-                    },
-                    id: container.Id,
-                    image: container.Image,
-                    status: container.Status,
-                },
-                { deep: true }
-            );
             return containerData;
         });
         return this.containerCache;
@@ -193,7 +204,7 @@ export class DockerService implements OnModuleInit {
             );
     }
 
-    public async startContainer(id: string): Promise<DockerContainer> {
+    public async start(id: string): Promise<DockerContainer> {
         const container = this.client.getContainer(id);
         await container.start();
         const containers = await this.getContainers({ useCache: false });
@@ -204,11 +215,23 @@ export class DockerService implements OnModuleInit {
         return updatedContainer;
     }
 
-    public async stopContainer(id: string): Promise<DockerContainer> {
+    public async stop(id: string): Promise<DockerContainer> {
         const container = this.client.getContainer(id);
-        await container.stop();
+        await container.stop({ t: 10 });
-        const containers = await this.getContainers({ useCache: false });
+        let containers = await this.getContainers({ useCache: false });
-        const updatedContainer = containers.find((c) => c.id === id);
+        let updatedContainer: DockerContainer | undefined;
+        for (let i = 0; i < 5; i++) {
+            await sleep(500);
+            // Refresh the containers list on each attempt
+            containers = await this.getContainers({ useCache: false });
+            updatedContainer = containers.find((c) => c.id === id);
+            this.logger.debug(
+                `Container ${id} state after stop attempt ${i + 1}: ${updatedContainer?.state}`
+            );
+            if (updatedContainer?.state === ContainerState.EXITED) {
+                return updatedContainer;
+            }
+        }
         if (!updatedContainer) {
             throw new Error(`Container ${id} not found after stopping`);
         }

api/src/unraid-api/graph/resolvers/mutation/mutation.resolver.ts

@@ -8,4 +8,11 @@ export class MutationResolver {
             __typename: 'ArrayMutations',
         };
     }
+
+    @ResolveField()
+    public async docker() {
+        return {
+            __typename: 'DockerMutations',
+        };
+    }
 }