Skip to content

Commit 9782f91

Browse files
komen205claude
andcommitted
Address review feedback: simplify to bounded availability check
Per review on #220: - Revert the docker-tunnel-proxy rule callback to its original inline form and drop the per-request DOCKER_PROXY_SETTING_TIMEOUT_MS timeout. The 3s Docker-availability timeout already bounds the network monitor (which awaits isDockerAvailable), so this still fails open: only requests in the first few seconds of a wedged-Docker startup wait, then everything works. - Move the timeout duration into TimeoutError/withTimeout so every timeout reports "Timeout after <n>ms"; isDockerAvailable's catch returns to its original one-line form. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent ace728c commit 9782f91

3 files changed

Lines changed: 19 additions & 104 deletions

File tree

src/interceptors/docker/docker-interception-services.ts

Lines changed: 13 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,11 @@ import {
2121
stopDockerTunnel,
2222
} from './docker-tunnel-proxy';
2323
import { ensureDockerInjectionVolumeExists } from './docker-data-injection';
24-
import { TimeoutError, withTimeout } from '../../util/promise';
24+
import { withTimeout } from '../../util/promise';
2525

2626
let dockerAvailableCache: Promise<boolean> | undefined;
2727

2828
const DOCKER_AVAILABILITY_TIMEOUT_MS = 3_000;
29-
const DOCKER_PROXY_SETTING_TIMEOUT_MS = 250;
30-
31-
type DockerNetworkRouteMonitor = {
32-
dockerRoutedAliases: ReadonlySet<string>
33-
};
3429

3530
export const isDockerAvailable = (options: { logError?: boolean } = {}) => {
3631
if (dockerAvailableCache) return dockerAvailableCache;
@@ -48,12 +43,7 @@ export const isDockerAvailable = (options: { logError?: boolean } = {}) => {
4843
}
4944
})
5045
.catch((error) => {
51-
if (options.logError) {
52-
const errorMessage = error instanceof TimeoutError
53-
? `timed out after ${DOCKER_AVAILABILITY_TIMEOUT_MS}ms`
54-
: error.message;
55-
console.warn('Docker not available:', errorMessage);
56-
}
46+
if (options.logError) console.warn('Docker not available:', error.message);
5747
return false;
5848
});
5949

@@ -66,48 +56,6 @@ export const isDockerAvailable = (options: { logError?: boolean } = {}) => {
6656

6757
const IPv4_IPv6_PREFIX = "::ffff:";
6858

69-
// This proxy setting is included in the default passthrough rules, so it can run for
70-
// browser/Android/etc traffic too. It must fail open quickly if Docker is unavailable.
71-
export function getDockerTunnelProxySetting(
72-
proxyPort: number,
73-
networkMonitor: Promise<DockerNetworkRouteMonitor | undefined>,
74-
options: { timeoutMs?: number } = {}
75-
): ProxySettingCallback {
76-
const timeoutMs = options.timeoutMs ?? DOCKER_PROXY_SETTING_TIMEOUT_MS;
77-
let hasLoggedTimeout = false;
78-
let hasLoggedFailure = false;
79-
80-
return async ({ hostname }: { hostname: any }) => {
81-
hostname = hostname.startsWith(IPv4_IPv6_PREFIX)
82-
? hostname.slice(IPv4_IPv6_PREFIX.length)
83-
: hostname;
84-
85-
const monitor = await withTimeout(timeoutMs, networkMonitor).catch((error) => {
86-
if (error instanceof TimeoutError) {
87-
if (!hasLoggedTimeout) {
88-
hasLoggedTimeout = true;
89-
console.warn(
90-
`Docker route lookup timed out after ${timeoutMs}ms; skipping Docker tunnel routing`
91-
);
92-
}
93-
} else {
94-
if (!hasLoggedFailure) {
95-
hasLoggedFailure = true;
96-
logError(error);
97-
}
98-
}
99-
100-
return undefined;
101-
});
102-
103-
if (monitor?.dockerRoutedAliases.has(hostname)) {
104-
return {
105-
proxyUrl: `socks5://127.0.0.1:${await getDockerTunnelPort(proxyPort)}`
106-
};
107-
}
108-
};
109-
}
110-
11159
// On shutdown, clean up every container & image that we created, disappearing
11260
// into the mist as if we were never here...
11361
// (Those images/containers are unusable without us, so leaving them breaks things).
@@ -146,10 +94,17 @@ export async function startDockerInterceptionServices(
14694

14795
const networkMonitor = monitorDockerNetworkAliases(proxyPort);
14896

149-
ruleParameters[`docker-tunnel-proxy-${proxyPort}`] = getDockerTunnelProxySetting(
150-
proxyPort,
151-
networkMonitor
152-
);
97+
ruleParameters[`docker-tunnel-proxy-${proxyPort}`] = async ({ hostname }: { hostname: any }) => {
98+
hostname = hostname.startsWith(IPv4_IPv6_PREFIX)
99+
? hostname.slice(IPv4_IPv6_PREFIX.length)
100+
: hostname;
101+
102+
if ((await networkMonitor)?.dockerRoutedAliases.has(hostname)) {
103+
return {
104+
proxyUrl: `socks5://127.0.0.1:${await getDockerTunnelPort(proxyPort)}`
105+
};
106+
}
107+
};
153108
console.log(`Created docker-tunnel-proxy-${proxyPort}`);
154109

155110
await Promise.all([

src/util/promise.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,11 @@ export async function waitUntil<T extends unknown>(
2323
}
2424

2525
export class TimeoutError extends CustomError {
26-
constructor() {
27-
super('Timeout', { code: 'timeout' });
26+
constructor(timeoutMs?: number) {
27+
super(
28+
timeoutMs === undefined ? 'Timeout' : `Timeout after ${timeoutMs}ms`,
29+
{ code: 'timeout' }
30+
);
2831
}
2932
}
3033

@@ -35,6 +38,6 @@ export async function withTimeout<T>(
3538
return Promise.race([
3639
promise,
3740
delay(timeoutMs, { unref: true })
38-
.then(() => { throw new TimeoutError(); })
41+
.then(() => { throw new TimeoutError(timeoutMs); })
3942
]);
4043
}

test/unit/docker-interception-services.spec.ts

Lines changed: 0 additions & 43 deletions
This file was deleted.

0 commit comments

Comments
 (0)