Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
7ee3f4d
feat: add network detector that uses notices endpoint
kaizencc Nov 4, 2025
7718108
feat(toolkit-lib): network detector
kaizencc Nov 4, 2025
91d3441
chore: refactor network detector to ping once an hour and write to disk
kaizencc Nov 5, 2025
51ffbf6
Merge branch 'main' into conroy/ping
kaizencc Nov 5, 2025
d7dcdc6
update funnle test
kaizencc Nov 5, 2025
f69b420
mock network detector in notices
kaizencc Nov 5, 2025
f7cd018
chore: self mutation
invalid-email-address Nov 5, 2025
d0d4e93
merge
kaizencc Nov 6, 2025
ec0768f
chore: self mutation
invalid-email-address Nov 6, 2025
60f2c12
udpate tests
kaizencc Nov 6, 2025
c342cc2
skip network check property
kaizencc Nov 11, 2025
671b1ee
update network-detector
kaizencc Nov 11, 2025
995765b
actually skip cache
kaizencc Nov 11, 2025
5365dfb
one line
kaizencc Nov 11, 2025
d037ec8
Merge branch 'main' into conroy/ping
kaizencc Nov 11, 2025
2cbbc6b
delete connection cache
kaizencc Nov 12, 2025
22f49d4
add logs
kaizencc Nov 12, 2025
4e65441
eslint
kaizencc Nov 13, 2025
c05df0e
logs
kaizencc Nov 13, 2025
0d975e9
await
kaizencc Nov 13, 2025
e647834
type
kaizencc Nov 13, 2025
34683ea
omg
kaizencc Nov 13, 2025
12e07e2
reverse
kaizencc Nov 13, 2025
0fc2d90
chore: self mutation
invalid-email-address Nov 13, 2025
ae56f62
merge
kaizencc Nov 13, 2025
0818fb2
omgggg
kaizencc Nov 13, 2025
7f2d4ea
update call
kaizencc Nov 13, 2025
c33ec6c
hail mary
kaizencc Nov 14, 2025
b2822d2
add back timeout
kaizencc Nov 14, 2025
0dd91d8
refactor back to what i want to merge
kaizencc Nov 14, 2025
ae20ea5
add back head request
kaizencc Nov 14, 2025
2adf47e
fix test
kaizencc Nov 14, 2025
d1355f1
add reasonable timeout
kaizencc Nov 14, 2025
1dc3a75
fix tests
kaizencc Nov 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as https from 'node:https';
import type { Notice, NoticeDataSource } from './types';
import { ToolkitError } from '../../toolkit/toolkit-error';
import { formatErrorMessage, humanHttpStatusError, humanNetworkError } from '../../util';
import { NetworkDetector } from '../../util/network-detector';
import type { IoHelper } from '../io/private';

/**
Expand Down Expand Up @@ -44,6 +45,12 @@ export class WebsiteNoticeDataSource implements NoticeDataSource {
}

async fetch(): Promise<Notice[]> {
// Check connectivity before attempting network request
const hasConnectivity = await NetworkDetector.hasConnectivity(this.agent);
if (!hasConnectivity) {
throw new ToolkitError('No internet connectivity detected');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is throwing the right thing here? Is that error caught elsewhere? Asking because Notices should just silently fail. A comment might help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the right thing to do here. we are throwing errors in web-data-source on failures and expecting to swallow them elsewhere (which we do)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[non-blocking] Since this pattern will be very common (get the result, check whether it's true and throw an error if not), we could also have a method that takes a callback and does this for you:

return NetworkDetector.ifConnected(async() => {...});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good thought and i have considered this. im not entirely against it, but i feel like my more (naive) approach works more intuitively even if it reuses the same pattern. we can always refactor in the future if it turns out that ifConnected is a cleaner API

}

// We are observing lots of timeouts when running in a massively parallel
// integration test environment, so wait for a longer timeout there.
//
Expand Down
3 changes: 3 additions & 0 deletions packages/@aws-cdk/toolkit-lib/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,6 @@ export * from './api/cloud-assembly';
export * from './api/io';
export * from './api/tags';
export * from './api/plugin';

// Utilities
export { NetworkDetector } from './util/network-detector';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a public package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually i do need this so endpoint-sink can access network detector in aws-cdk. but ive updated how its imported to mirror that of notices

90 changes: 90 additions & 0 deletions packages/@aws-cdk/toolkit-lib/lib/util/network-detector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import type { Agent } from 'https';
import { request } from 'https';
import * as path from 'path';
import * as fs from 'fs-extra';
import { cdkCacheDir } from './';

interface CachedConnectivity {
expiration: number;
hasConnectivity: boolean;
}

const TIME_TO_LIVE_SUCCESS = 60 * 60 * 1000; // 1 hour
const CACHE_FILE_PATH = path.join(cdkCacheDir(), 'connection.json');

/**
* Detects internet connectivity by making a lightweight request to the notices endpoint
*/
export class NetworkDetector {
/**
* Check if internet connectivity is available
*/
public static async hasConnectivity(agent?: Agent): Promise<boolean> {
const cachedData = await this.load();
const expiration = cachedData.expiration ?? 0;

if (Date.now() > expiration) {
try {
const connected = await this.ping(agent);
const updatedData = {
expiration: Date.now() + TIME_TO_LIVE_SUCCESS,
hasConnectivity: connected,
};
await this.save(updatedData);
return connected;
} catch {
return false;
}
} else {
return cachedData.hasConnectivity;
}
}

private static readonly TIMEOUT_MS = 500;

private static async load(): Promise<CachedConnectivity> {
const defaultValue = {
expiration: 0,
hasConnectivity: false,
};

try {
return fs.existsSync(CACHE_FILE_PATH)
? await fs.readJSON(CACHE_FILE_PATH) as CachedConnectivity
: defaultValue;
} catch {
return defaultValue;
}
}

private static async save(cached: CachedConnectivity): Promise<void> {
try {
await fs.ensureFile(CACHE_FILE_PATH);
await fs.writeJSON(CACHE_FILE_PATH, cached);
} catch {
// Silently ignore cache save errors
}
}

private static ping(agent?: Agent): Promise<boolean> {
return new Promise((resolve) => {
const req = request({
hostname: 'cli.cdk.dev-tools.aws.dev',
path: '/notices.json',
method: 'HEAD',
agent,
timeout: this.TIMEOUT_MS,
}, (res) => {
resolve(res.statusCode !== undefined && res.statusCode < 500);
});

req.on('error', () => resolve(false));
req.on('timeout', () => {
req.destroy();
resolve(false);
});

req.end();
});
}
}
19 changes: 19 additions & 0 deletions packages/@aws-cdk/toolkit-lib/test/api/notices.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { FilteredNotice, NoticesFilter } from '../../lib/api/notices/filter';
import type { BootstrappedEnvironment, Component, Notice } from '../../lib/api/notices/types';
import { WebsiteNoticeDataSource } from '../../lib/api/notices/web-data-source';
import { Settings } from '../../lib/api/settings';
import { NetworkDetector } from '../../lib/util/network-detector';
import { TestIoHost } from '../_helpers';

const BASIC_BOOTSTRAP_NOTICE = {
Expand Down Expand Up @@ -540,6 +541,24 @@ function parseTestComponent(x: string): Component {
describe(WebsiteNoticeDataSource, () => {
const dataSource = new WebsiteNoticeDataSource(ioHelper);

beforeEach(() => {
// Mock NetworkDetector to return true by default for existing tests
jest.spyOn(NetworkDetector, 'hasConnectivity').mockResolvedValue(true);
});

afterEach(() => {
jest.restoreAllMocks();
});

test('throws error when no connectivity detected', async () => {
const mockHasConnectivity = jest.spyOn(NetworkDetector, 'hasConnectivity').mockResolvedValue(false);

await expect(dataSource.fetch()).rejects.toThrow('No internet connectivity detected');
expect(mockHasConnectivity).toHaveBeenCalledWith(undefined);

mockHasConnectivity.mockRestore();
});

test('returns data when download succeeds', async () => {
const result = await mockCall(200, {
notices: [BASIC_NOTICE, MULTIPLE_AFFECTED_VERSIONS_NOTICE],
Expand Down
169 changes: 169 additions & 0 deletions packages/@aws-cdk/toolkit-lib/test/util/network-detector.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
import * as https from 'https';
import * as fs from 'fs-extra';
import { NetworkDetector } from '../../lib/util/network-detector';

// Mock the https module
jest.mock('https');
const mockHttps = https as jest.Mocked<typeof https>;

// Mock fs-extra
jest.mock('fs-extra');
const mockFs = fs as jest.Mocked<typeof fs>;

// Mock cdkCacheDir
jest.mock('../../lib/util', () => ({
cdkCacheDir: jest.fn(() => '/mock/cache/dir'),
}));

describe('NetworkDetector', () => {
let mockRequest: jest.Mock;

beforeEach(() => {
jest.clearAllMocks();
mockRequest = jest.fn();
mockHttps.request.mockImplementation(mockRequest);
});

test('returns true when server responds with success status', async () => {
const mockReq = {
on: jest.fn(),
end: jest.fn(),
destroy: jest.fn(),
};

mockRequest.mockImplementation((_options, callback) => {
callback({ statusCode: 200 });
return mockReq;
});

mockFs.existsSync.mockReturnValue(false);
(mockFs.ensureFile as jest.Mock).mockResolvedValue(undefined);
(mockFs.writeJSON as jest.Mock).mockResolvedValue(undefined);

const result = await NetworkDetector.hasConnectivity();
expect(result).toBe(true); // Should return true for successful HTTP response
});

test('returns false when server responds with server error', async () => {
const mockReq = {
on: jest.fn(),
end: jest.fn(),
destroy: jest.fn(),
};

mockRequest.mockImplementation((_options, callback) => {
callback({ statusCode: 500 });
return mockReq;
});

mockFs.existsSync.mockReturnValue(false);
(mockFs.ensureFile as jest.Mock).mockResolvedValue(undefined);
(mockFs.writeJSON as jest.Mock).mockResolvedValue(undefined);

const result = await NetworkDetector.hasConnectivity();
expect(result).toBe(false); // Should return false for server error status codes
});

test('returns false on network error', async () => {
const mockReq = {
on: jest.fn((event, handler) => {
if (event === 'error') {
setTimeout(() => handler(new Error('Network error')), 0);
}
}),
end: jest.fn(),
destroy: jest.fn(),
};

mockRequest.mockReturnValue(mockReq);
mockFs.existsSync.mockReturnValue(false);

const result = await NetworkDetector.hasConnectivity();
expect(result).toBe(false); // Should return false when network request fails
});

test('returns cached result from disk when not expired', async () => {
const cachedData = {
expiration: Date.now() + 30000, // 30 seconds in future
hasConnectivity: true,
};

mockFs.existsSync.mockReturnValue(true);
(mockFs.readJSON as jest.Mock).mockResolvedValue(cachedData);

const result = await NetworkDetector.hasConnectivity();

expect(result).toBe(true); // Should return cached connectivity result
expect(mockRequest).not.toHaveBeenCalled(); // Should not make network request when cache is valid
});

test('performs ping when disk cache is expired', async () => {
const expiredData = {
expiration: Date.now() - 1000, // 1 second ago
hasConnectivity: true,
};

const mockReq = {
on: jest.fn(),
end: jest.fn(),
destroy: jest.fn(),
};

mockRequest.mockImplementation((_options, callback) => {
callback({ statusCode: 200 });
return mockReq;
});

mockFs.existsSync.mockReturnValue(true);
(mockFs.readJSON as jest.Mock).mockResolvedValue(expiredData);
(mockFs.ensureFile as jest.Mock).mockResolvedValue(undefined);
(mockFs.writeJSON as jest.Mock).mockResolvedValue(undefined);

const result = await NetworkDetector.hasConnectivity();

expect(result).toBe(true); // Should return fresh connectivity result
expect(mockRequest).toHaveBeenCalledTimes(1); // Should make network request when cache is expired
});

test('handles cache save errors gracefully', async () => {
const mockReq = {
on: jest.fn(),
end: jest.fn(),
destroy: jest.fn(),
};

mockRequest.mockImplementation((_options, callback) => {
callback({ statusCode: 200 });
return mockReq;
});

mockFs.existsSync.mockReturnValue(false);
(mockFs.ensureFile as jest.Mock).mockRejectedValue(new Error('Disk full'));

const result = await NetworkDetector.hasConnectivity();

expect(result).toBe(true); // Should still return connectivity result despite cache save failure
});

test('handles cache load errors gracefully', async () => {
const mockReq = {
on: jest.fn(),
end: jest.fn(),
destroy: jest.fn(),
};

mockRequest.mockImplementation((_options, callback) => {
callback({ statusCode: 200 });
return mockReq;
});

mockFs.existsSync.mockReturnValue(true);
(mockFs.readJSON as jest.Mock).mockRejectedValue(new Error('Read failed'));
(mockFs.ensureFile as jest.Mock).mockResolvedValue(undefined);
(mockFs.writeJSON as jest.Mock).mockResolvedValue(undefined);

const result = await NetworkDetector.hasConnectivity();

expect(result).toBe(true); // Should still return connectivity result despite cache load failure
});
});
9 changes: 8 additions & 1 deletion packages/aws-cdk/lib/cli/telemetry/sink/endpoint-sink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { IncomingMessage } from 'http';
import type { Agent } from 'https';
import { request } from 'https';
import { parse, type UrlWithStringQuery } from 'url';
import { ToolkitError } from '@aws-cdk/toolkit-lib';
import { ToolkitError, NetworkDetector } from '@aws-cdk/toolkit-lib';
import { IoHelper } from '../../../api-private';
import type { IIoHost } from '../../io-host';
import type { TelemetrySchema } from '../schema';
Expand Down Expand Up @@ -89,6 +89,13 @@ export class EndpointTelemetrySink implements ITelemetrySink {
url: UrlWithStringQuery,
body: { events: TelemetrySchema[] },
): Promise<boolean> {
// Check connectivity before attempting network request
const hasConnectivity = await NetworkDetector.hasConnectivity(this.agent);
if (!hasConnectivity) {
await this.ioHelper.defaults.trace('No internet connectivity detected, skipping telemetry');
return false;
}

try {
const res = await doRequest(url, body, this.agent);

Expand Down
28 changes: 28 additions & 0 deletions packages/aws-cdk/test/cli/telemetry/sink/endpoint-sink.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as https from 'https';
import { NetworkDetector } from '@aws-cdk/toolkit-lib';
import { createTestEvent } from './util';
import { IoHelper } from '../../../../lib/api-private';
import { CliIoHost } from '../../../../lib/cli/io-host';
Expand All @@ -9,12 +10,23 @@ jest.mock('https', () => ({
request: jest.fn(),
}));

// Mock NetworkDetector
jest.mock('@aws-cdk/toolkit-lib', () => ({
...jest.requireActual('@aws-cdk/toolkit-lib'),
NetworkDetector: {
hasConnectivity: jest.fn(),
},
}));

describe('EndpointTelemetrySink', () => {
let ioHost: CliIoHost;

beforeEach(() => {
jest.resetAllMocks();

// Mock NetworkDetector to return true by default for existing tests
(NetworkDetector.hasConnectivity as jest.Mock).mockResolvedValue(true);

ioHost = CliIoHost.instance();
});

Expand Down Expand Up @@ -312,4 +324,20 @@ describe('EndpointTelemetrySink', () => {
expect.stringContaining('Telemetry Error: POST example.com/telemetry:'),
);
});

test('skips request when no connectivity detected', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the true result: we do not ping the telemetry endpoint in environments without internet access

// GIVEN
(NetworkDetector.hasConnectivity as jest.Mock).mockResolvedValue(false);

const testEvent = createTestEvent('INVOKE', { foo: 'bar' });
const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });

// WHEN
await client.emit(testEvent);
await client.flush();

// THEN
expect(NetworkDetector.hasConnectivity).toHaveBeenCalledWith(undefined);
expect(https.request).not.toHaveBeenCalled();
});
});
Loading
Loading