-
Notifications
You must be signed in to change notification settings - Fork 95
Fix error serialization in 4 catch blocks where JSON.stringify(error) produces "{}"
#341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -430,7 +430,7 @@ export async function execCpToPod( | |
| attempt++ | ||
| if (attempt >= 30) { | ||
| throw new Error( | ||
| `cpToPod failed after ${attempt} attempts: ${JSON.stringify(error)}` | ||
| `cpToPod failed after ${attempt} attempts: ${error instanceof Error ? error.message : String(error)}` | ||
| ) | ||
| } | ||
| await sleep(1000) | ||
|
|
@@ -527,7 +527,7 @@ export async function execCpFromPod( | |
| attempt++ | ||
| if (attempt >= 30) { | ||
| throw new Error( | ||
| `execCpFromPod failed after ${attempt} attempts: ${JSON.stringify(error)}` | ||
| `execCpFromPod failed after ${attempt} attempts: ${error instanceof Error ? error.message : String(error)}` | ||
| ) | ||
| } | ||
| await sleep(1000) | ||
|
|
@@ -574,7 +574,7 @@ export async function waitForJobToComplete(jobName: string): Promise<void> { | |
| return | ||
| } | ||
| } catch (error) { | ||
| throw new Error(`job ${jobName} has failed: ${JSON.stringify(error)}`) | ||
| throw new Error(`job ${jobName} has failed: ${error instanceof Error ? error.message : String(error)}`) | ||
| } | ||
| await backOffManager.backOff() | ||
| } | ||
|
|
@@ -697,7 +697,7 @@ export async function waitForPodPhases( | |
| } | ||
| } catch (error) { | ||
| throw new Error( | ||
| `Pod ${podName} is unhealthy with phase status ${phase}: ${JSON.stringify(error)}` | ||
| `Pod ${podName} is unhealthy with phase status ${phase}: ${error instanceof Error ? error.message : String(error)}` | ||
| ) | ||
|
Comment on lines
699
to
701
|
||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| import { WritableStreamBuffer } from 'stream-buffers' | ||
|
|
||
|
jeanschmidt marked this conversation as resolved.
Outdated
|
||
| const mockExec = jest.fn() | ||
| const mockReadNamespacedPod = jest.fn() | ||
| const mockReadNamespacedJob = jest.fn() | ||
|
|
||
| jest.mock('@kubernetes/client-node', () => { | ||
| return { | ||
| KubeConfig: jest.fn().mockImplementation(() => ({ | ||
| loadFromDefault: jest.fn(), | ||
| makeApiClient: jest.fn().mockImplementation(ApiClass => { | ||
| const name = ApiClass?.name || ApiClass?.toString() || '' | ||
| if (name.includes('Batch')) { | ||
| return { readNamespacedJob: mockReadNamespacedJob } | ||
| } | ||
| if (name.includes('Authorization')) { | ||
| return { createSelfSubjectAccessReview: jest.fn() } | ||
| } | ||
| return { readNamespacedPod: mockReadNamespacedPod } | ||
| }), | ||
| getContexts: jest | ||
| .fn() | ||
| .mockReturnValue([{ namespace: 'test-namespace' }]) | ||
| })), | ||
| Exec: jest.fn().mockImplementation(() => ({ exec: mockExec })), | ||
| CoreV1Api: class CoreV1Api {}, | ||
| BatchV1Api: class BatchV1Api {}, | ||
| AuthorizationV1Api: class AuthorizationV1Api {}, | ||
| Log: jest.fn() | ||
| } | ||
| }) | ||
|
|
||
| jest.mock('tar-fs', () => ({ | ||
| default: { | ||
| pack: jest.fn().mockReturnValue({ pipe: jest.fn() }), | ||
| extract: jest.fn().mockReturnValue({ | ||
| on: jest.fn(), | ||
| pipe: jest.fn() | ||
| }) | ||
| }, | ||
| __esModule: true | ||
| })) | ||
|
|
||
| jest.mock('../src/k8s/utils', () => { | ||
| const actual = jest.requireActual('../src/k8s/utils') | ||
| return { | ||
| ...actual, | ||
| sleep: jest.fn().mockResolvedValue(undefined) | ||
| } | ||
| }) | ||
|
|
||
| import { | ||
| execCpToPod, | ||
| execCpFromPod, | ||
| waitForJobToComplete, | ||
| waitForPodPhases | ||
| } from '../src/k8s' | ||
| import { PodPhase } from '../src/k8s/utils' | ||
|
|
||
| describe('error serialization', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
| process.env['ACTIONS_RUNNER_KUBERNETES_NAMESPACE'] = 'test-namespace' | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| delete process.env['ACTIONS_RUNNER_KUBERNETES_NAMESPACE'] | ||
| }) | ||
|
|
||
| describe('execCpToPod', () => { | ||
| it('should include Error.message in thrown error after retries', async () => { | ||
| mockExec.mockRejectedValue(new Error('connection refused')) | ||
|
|
||
| await expect( | ||
| execCpToPod('test-pod', '/tmp/src', '/workspace') | ||
| ).rejects.toThrow('cpToPod failed after 30 attempts: connection refused') | ||
| }) | ||
|
|
||
| it('should use String() for non-Error throwables', async () => { | ||
| mockExec.mockRejectedValue('raw string error') | ||
|
|
||
| await expect( | ||
| execCpToPod('test-pod', '/tmp/src', '/workspace') | ||
| ).rejects.toThrow('cpToPod failed after 30 attempts: raw string error') | ||
| }) | ||
|
|
||
| it('should not produce empty braces in error message', async () => { | ||
| mockExec.mockRejectedValue(new Error('ETIMEOUT')) | ||
|
|
||
| await expect( | ||
| execCpToPod('test-pod', '/tmp/src', '/workspace') | ||
| ).rejects.toThrow( | ||
| expect.not.objectContaining({ message: expect.stringContaining('{}') }) | ||
| ) | ||
|
jeanschmidt marked this conversation as resolved.
Outdated
|
||
| }) | ||
| }) | ||
|
|
||
| describe('execCpFromPod', () => { | ||
| it('should include Error.message in thrown error after retries', async () => { | ||
| mockExec.mockRejectedValue(new Error('container not found')) | ||
|
|
||
| await expect( | ||
| execCpFromPod('test-pod', '/workspace/output', '/tmp/dst') | ||
| ).rejects.toThrow( | ||
| 'execCpFromPod failed after 30 attempts: container not found' | ||
| ) | ||
| }) | ||
|
|
||
| it('should use String() for non-Error throwables', async () => { | ||
| mockExec.mockRejectedValue(42) | ||
|
|
||
| await expect( | ||
| execCpFromPod('test-pod', '/workspace/output', '/tmp/dst') | ||
| ).rejects.toThrow('execCpFromPod failed after 30 attempts: 42') | ||
| }) | ||
| }) | ||
|
|
||
| describe('waitForJobToComplete', () => { | ||
| it('should include Error.message when job fails', async () => { | ||
| mockReadNamespacedJob.mockResolvedValue({ | ||
| status: { failed: 1 } | ||
| }) | ||
|
|
||
| await expect(waitForJobToComplete('my-job')).rejects.toThrow( | ||
| 'job my-job has failed: job my-job has failed' | ||
| ) | ||
| }) | ||
|
|
||
| it('should include Error.message when API call throws', async () => { | ||
| mockReadNamespacedJob.mockRejectedValue( | ||
| new Error('403 Forbidden') | ||
| ) | ||
|
|
||
| await expect(waitForJobToComplete('my-job')).rejects.toThrow( | ||
| 'job my-job has failed: 403 Forbidden' | ||
| ) | ||
| }) | ||
|
|
||
| it('should use String() for non-Error throwables from API', async () => { | ||
| mockReadNamespacedJob.mockRejectedValue('unexpected API failure') | ||
|
|
||
| await expect(waitForJobToComplete('my-job')).rejects.toThrow( | ||
| 'job my-job has failed: unexpected API failure' | ||
| ) | ||
| }) | ||
| }) | ||
|
|
||
| describe('waitForPodPhases', () => { | ||
| it('should include error message when pod enters unhealthy phase', async () => { | ||
| mockReadNamespacedPod.mockResolvedValue({ | ||
| status: { phase: 'Failed' } | ||
| }) | ||
|
|
||
| await expect( | ||
| waitForPodPhases( | ||
| 'test-pod', | ||
| new Set([PodPhase.RUNNING]), | ||
| new Set([PodPhase.PENDING]) | ||
| ) | ||
| ).rejects.toThrow( | ||
| /Pod test-pod is unhealthy with phase status Failed/ | ||
| ) | ||
| }) | ||
|
|
||
| it('should include Error.message when API call throws', async () => { | ||
| mockReadNamespacedPod.mockRejectedValue( | ||
| new Error('network timeout') | ||
| ) | ||
|
|
||
| await expect( | ||
| waitForPodPhases( | ||
| 'test-pod', | ||
| new Set([PodPhase.RUNNING]), | ||
| new Set([PodPhase.PENDING]) | ||
| ) | ||
| ).rejects.toThrow( | ||
| 'Pod test-pod is unhealthy with phase status Unknown: network timeout' | ||
| ) | ||
| }) | ||
|
|
||
| it('should not produce empty braces from Error objects', async () => { | ||
| mockReadNamespacedPod.mockRejectedValue( | ||
| new Error('socket hang up') | ||
| ) | ||
|
|
||
| try { | ||
| await waitForPodPhases( | ||
| 'test-pod', | ||
| new Set([PodPhase.RUNNING]), | ||
| new Set([PodPhase.PENDING]) | ||
| ) | ||
| fail('Expected waitForPodPhases to throw') | ||
| } catch (error) { | ||
| const msg = (error as Error).message | ||
| expect(msg).not.toContain('{}') | ||
| expect(msg).toContain('socket hang up') | ||
| } | ||
| }) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wrap can produce duplicated messages when the underlying error already includes
job ${jobName} has failed(e.g. fromisJobSucceeded), resulting injob X has failed: job X has failed. Consider rethrowing the original error in that case, or changing the wrapper message to add new context (and/or usenew Error(msg, { cause: error })to preserve the original error).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically valid observation: isJobSucceeded throws "job X has failed", and the catch wraps it as "job X has failed: job X has failed". But this is pre-existing behavior — it was "job X has failed: {}" before. Our
PR just makes the duplication visible instead of hiding it behind {}. I'd say this is out of scope for a bug-fix PR — it's a separate improvement. The suggested fix also adds complexity (conditional rethrow) that goes beyond the intent of this change.