fix(#275): Prevent silent connection initiation failures
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
Fixed silent connection initiation failures where HTTP errors were caught but success was returned to the user, leaving zombie connections in PENDING state forever. Changes: - Delete failed connection from database when HTTP request fails - Throw BadRequestException with clear error message - Added test to verify connection deletion and exception throwing - Import BadRequestException in connection.service.ts User Impact: - Users now receive immediate feedback when connection initiation fails - No more zombie connections stuck in PENDING state - Clear error messages indicate the reason for failure Testing: - Added test case: "should delete connection and throw error if request fails" - All 21 connection service tests passing - Quality gates: lint, typecheck, build all passing Fixes #275 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -85,6 +85,7 @@ describe("ConnectionService", () => {
|
|||||||
findUnique: vi.fn(),
|
findUnique: vi.fn(),
|
||||||
findMany: vi.fn(),
|
findMany: vi.fn(),
|
||||||
update: vi.fn(),
|
update: vi.fn(),
|
||||||
|
delete: vi.fn(),
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
@@ -208,6 +209,36 @@ describe("ConnectionService", () => {
|
|||||||
})
|
})
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("should delete connection and throw error if request fails", async () => {
|
||||||
|
const mockAxiosResponse: AxiosResponse = {
|
||||||
|
data: mockRemoteIdentity,
|
||||||
|
status: 200,
|
||||||
|
statusText: "OK",
|
||||||
|
headers: {},
|
||||||
|
config: {} as never,
|
||||||
|
};
|
||||||
|
|
||||||
|
vi.spyOn(httpService, "get").mockReturnValue(of(mockAxiosResponse));
|
||||||
|
vi.spyOn(httpService, "post").mockReturnValue(
|
||||||
|
throwError(() => new Error("Connection refused"))
|
||||||
|
);
|
||||||
|
const createSpy = vi
|
||||||
|
.spyOn(prismaService.federationConnection, "create")
|
||||||
|
.mockResolvedValue(mockConnection);
|
||||||
|
const deleteSpy = vi
|
||||||
|
.spyOn(prismaService.federationConnection, "delete")
|
||||||
|
.mockResolvedValue(mockConnection);
|
||||||
|
|
||||||
|
await expect(service.initiateConnection(mockWorkspaceId, mockRemoteUrl)).rejects.toThrow(
|
||||||
|
"Failed to initiate connection"
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(createSpy).toHaveBeenCalled();
|
||||||
|
expect(deleteSpy).toHaveBeenCalledWith({
|
||||||
|
where: { id: mockConnection.id },
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("acceptConnection", () => {
|
describe("acceptConnection", () => {
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ import {
|
|||||||
NotFoundException,
|
NotFoundException,
|
||||||
UnauthorizedException,
|
UnauthorizedException,
|
||||||
ServiceUnavailableException,
|
ServiceUnavailableException,
|
||||||
|
BadRequestException,
|
||||||
} from "@nestjs/common";
|
} from "@nestjs/common";
|
||||||
import { HttpService } from "@nestjs/axios";
|
import { HttpService } from "@nestjs/axios";
|
||||||
import { FederationConnectionStatus, Prisma } from "@prisma/client";
|
import { FederationConnectionStatus, Prisma } from "@prisma/client";
|
||||||
@@ -68,7 +69,7 @@ export class ConnectionService {
|
|||||||
const signature = await this.signatureService.signMessage(request);
|
const signature = await this.signatureService.signMessage(request);
|
||||||
const signedRequest: ConnectionRequest = { ...request, signature };
|
const signedRequest: ConnectionRequest = { ...request, signature };
|
||||||
|
|
||||||
// Send connection request to remote instance (fire-and-forget for now)
|
// Send connection request to remote instance
|
||||||
try {
|
try {
|
||||||
await firstValueFrom(
|
await firstValueFrom(
|
||||||
this.httpService.post(`${remoteUrl}/api/v1/federation/incoming/connect`, signedRequest)
|
this.httpService.post(`${remoteUrl}/api/v1/federation/incoming/connect`, signedRequest)
|
||||||
@@ -76,7 +77,16 @@ export class ConnectionService {
|
|||||||
this.logger.log(`Connection request sent to ${remoteUrl}`);
|
this.logger.log(`Connection request sent to ${remoteUrl}`);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
this.logger.error(`Failed to send connection request to ${remoteUrl}`, error);
|
this.logger.error(`Failed to send connection request to ${remoteUrl}`, error);
|
||||||
// Connection is still created in PENDING state, can be retried
|
|
||||||
|
// Delete the failed connection to prevent zombie connections in PENDING state
|
||||||
|
await this.prisma.federationConnection.delete({
|
||||||
|
where: { id: connection.id },
|
||||||
|
});
|
||||||
|
|
||||||
|
const errorMessage = error instanceof Error ? error.message : "Unknown error";
|
||||||
|
throw new BadRequestException(
|
||||||
|
`Failed to initiate connection to ${remoteUrl}: ${errorMessage}`
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
return this.mapToConnectionDetails(connection);
|
return this.mapToConnectionDetails(connection);
|
||||||
|
|||||||
82
docs/scratchpads/275-silent-connection-failures.md
Normal file
82
docs/scratchpads/275-silent-connection-failures.md
Normal file
@@ -0,0 +1,82 @@
|
|||||||
|
# Issue #275: Fix silent connection initiation failures
|
||||||
|
|
||||||
|
## Objective
|
||||||
|
|
||||||
|
Fix silent connection initiation failures where HTTP errors are caught but success is returned to the user, leaving zombie connections in PENDING state forever.
|
||||||
|
|
||||||
|
## Location
|
||||||
|
|
||||||
|
`apps/api/src/federation/connection.service.ts:72-80`
|
||||||
|
|
||||||
|
## Problem
|
||||||
|
|
||||||
|
Current code:
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
try {
|
||||||
|
await firstValueFrom(
|
||||||
|
this.httpService.post(`${remoteUrl}/api/v1/federation/incoming/connect`, signedRequest)
|
||||||
|
);
|
||||||
|
this.logger.log(`Connection request sent to ${remoteUrl}`);
|
||||||
|
} catch (error) {
|
||||||
|
this.logger.error(`Failed to send connection request to ${remoteUrl}`, error);
|
||||||
|
// Connection is still created in PENDING state, can be retried
|
||||||
|
}
|
||||||
|
|
||||||
|
return this.mapToConnectionDetails(connection);
|
||||||
|
```
|
||||||
|
|
||||||
|
Issues:
|
||||||
|
|
||||||
|
- Catches HTTP failures but returns success
|
||||||
|
- Connection stays in PENDING state forever
|
||||||
|
- Creates zombie connections
|
||||||
|
- User sees success message but connection actually failed
|
||||||
|
|
||||||
|
## Solution
|
||||||
|
|
||||||
|
1. Delete the failed connection from database
|
||||||
|
2. Throw exception with clear error message
|
||||||
|
3. User gets immediate feedback that connection failed
|
||||||
|
|
||||||
|
## Implementation
|
||||||
|
|
||||||
|
```typescript
|
||||||
|
try {
|
||||||
|
await firstValueFrom(
|
||||||
|
this.httpService.post(`${remoteUrl}/api/v1/federation/incoming/connect`, signedRequest)
|
||||||
|
);
|
||||||
|
this.logger.log(`Connection request sent to ${remoteUrl}`);
|
||||||
|
} catch (error) {
|
||||||
|
this.logger.error(`Failed to send connection request to ${remoteUrl}`, error);
|
||||||
|
|
||||||
|
// Delete the failed connection to prevent zombie connections
|
||||||
|
await this.prisma.federationConnection.delete({
|
||||||
|
where: { id: connection.id },
|
||||||
|
});
|
||||||
|
|
||||||
|
throw new BadRequestException(
|
||||||
|
`Failed to initiate connection to ${remoteUrl}: ${error instanceof Error ? error.message : "Unknown error"}`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
## Testing
|
||||||
|
|
||||||
|
Test scenarios:
|
||||||
|
|
||||||
|
1. Remote instance is unreachable - should throw exception and delete connection
|
||||||
|
2. Remote instance returns error - should throw exception and delete connection
|
||||||
|
3. Remote instance times out - should throw exception and delete connection
|
||||||
|
4. Remote instance returns success - should create connection in PENDING state
|
||||||
|
|
||||||
|
## Progress
|
||||||
|
|
||||||
|
- [ ] Create scratchpad
|
||||||
|
- [ ] Implement fix in connection.service.ts
|
||||||
|
- [ ] Add/update tests
|
||||||
|
- [ ] Run quality gates
|
||||||
|
- [ ] Commit changes
|
||||||
|
- [ ] Create PR
|
||||||
|
- [ ] Merge to develop
|
||||||
|
- [ ] Close issue #275
|
||||||
Reference in New Issue
Block a user