diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index adbfa0a26..befbb0f4f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -50,14 +50,22 @@ is missing. Report all blockers; do not mark the PR ready until the checks pass and the other requirements are satisfied. If there is no PR, stay on Case 1. ```bash -npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed +npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed npm run prettier-fix +npm run l10n:check # always; CI validates localization even when generation is skipped npm run lint -npx vitest run # full suite npm run build -npm run package # catches bundling and missing-asset failures +npm run typecheck:tests # strict package-test checks across all workspaces, as in CI +npx vitest run # full runtime suite; does not replace typecheck:tests +npm run package # catches bundling and missing-asset failures ``` +`npm run build` checks the root project and package source, but does not check package +tests under their stricter `tsconfig.test.json` settings. Vitest runtime success (or +an ad hoc `vitest --typecheck` invocation) is not a substitute for the exact +`npm run typecheck:tests` script. Keep this list aligned with the +[CI static-check job](workflows/main.yml), and report that script's result separately. + Also confirm the AI pre-review (CONTRIBUTING.md §6) has run and its review file is committed under `docs/ai-and-plans/features//iterations/`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e8f9bec79..ee68af67c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -248,14 +248,22 @@ Nothing else. `l10n`, `prettier-fix`, `lint`, and `package` each cost minutes an All of these must pass locally. The same checks run in CI, so catching failures here saves time. ```bash -npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed +npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed npm run prettier-fix +npm run l10n:check # always; CI validates localization even when generation is skipped npm run lint -npx vitest run # full suite npm run build -npm run package # catches bundling and missing-asset failures +npm run typecheck:tests # strict package-test checks across all workspaces, as in CI +npx vitest run # full runtime suite; does not replace typecheck:tests +npm run package # catches bundling and missing-asset failures ``` +`npm run build` checks the root project and package source, but does not check package +tests under their stricter `tsconfig.test.json` settings. Vitest runtime success (or +an ad hoc `vitest --typecheck` invocation) is not a substitute for the exact +`npm run typecheck:tests` script. Keep this list aligned with the +[CI static-check job](.github/workflows/main.yml), and report that script's result separately. + Commit whatever these change, including regenerated files under `l10n/` and anything Prettier reformats. Case 2 is also where the AI pre-review in [§6](#6-ai-assisted-review-workflow) is due. Do not mark a PR ready for human review until that pass has run and its review file is committed under the feature's `iterations/` folder. @@ -263,7 +271,9 @@ Case 2 is also where the AI pre-review in [§6](#6-ai-assisted-review-workflow) ### 4.3. Notes on individual steps - **`npm run l10n`** regenerates the localization bundle from the strings passed to `vscode.l10n.t()`. **Never resolve a conflict in `l10n/bundle.l10n.json` by hand** — the file is generated. Take either side, or delete it, then re-run `npm run l10n` and commit the result. +- **`npm run l10n:check`** validates localization against the source without regenerating files. Run it even when no strings changed, matching CI. - **`npm run prettier-fix`** covers Markdown as well as source, so documentation formatting does not drift whenever a file is touched. Fenced code blocks are left exactly as authored: they are often partial or illustrative, and reformatting them to the Markdown tab width would leave every example disagreeing with the code it documents. +- **`npm run typecheck:tests`** checks every participating workspace's tests with its own TypeScript configuration, including strict checks that the root build and Vitest runtime do not cover. - **`npm run package`** catches Vite host and webview bundling issues and missing assets that unit tests alone will not surface. ## 5. Documenting Work with AI diff --git a/docs/ai-and-plans/features/srv-dns-fallback/README.md b/docs/ai-and-plans/features/srv-dns-fallback/README.md index d13ce79e9..cf7eec8dc 100644 --- a/docs/ai-and-plans/features/srv-dns-fallback/README.md +++ b/docs/ai-and-plans/features/srv-dns-fallback/README.md @@ -85,6 +85,20 @@ Increment 1 ([plan](./iterations/01-srv-fallback/plan.md), progress recorded per | `docs/user-manual/troubleshooting-srv-dns.md` | Docs | Symptoms, confirmation, workarounds, timing, trace, switches | | `src/services/localQuickStart/QuickStartService.ts`, `connectToClient.ts` | — | Out of scope (D0007); `connectToClient` is deprecated | +## Test isolation + +The resolver package tests do not query external DNS servers. Transport tests use UDP/TCP +stubs bound to `127.0.0.1`; system DNS server discovery and driver resolver probes are mocked. +Transport tests freeze both the clock and resolver timers, advancing time only for timeout +scenarios, so slow loopback I/O cannot exhaust the resolver's time budget. TCP frame assembly +is tested by splitting client data events into individual bytes, not by delaying server writes. +Retry tests do not assume randomly generated query IDs differ. Stub startup cleans up partial +binds and retries address-in-use conflicts up to three attempts; other bind failures remain errors. +The UDP refusal tests inject `ECONNREFUSED` and `ECONNRESET` without sending a datagram, and +check immediate rejection and timer cleanup without advancing fake timers. They do not rely on +closed-port ICMP errors, which may be suppressed on Windows or restricted runners. +Loopback sockets must still be permitted. + ## Timeline | Date | PR | What changed | Docs | diff --git a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts index 4faea677f..f12044b74 100644 --- a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts +++ b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts @@ -3,11 +3,12 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import * as dnsPacket from 'dns-packet'; import dgram from 'node:dgram'; import dns from 'node:dns'; +import net from 'node:net'; import { FallbackDnsError, resolveSrvFallback, resolveTxtFallback } from './fallbackDnsClient.js'; import { DnsStubServer } from './test/dnsStubServer.js'; @@ -72,11 +73,22 @@ function compressedReply(query: dnsPacket.Packet, targets = ['fc-abc-000']): Buf describe('fallback DNS client', (): void => { const stubs: DnsStubServer[] = []; + beforeEach((): void => { + vi.useFakeTimers({ toFake: ['Date', 'setTimeout', 'clearTimeout'] }); + vi.spyOn(dns, 'getServers').mockImplementation((): string[] => { + throw new Error('Tests must explicitly supply loopback DNS servers'); + }); + }); + afterEach(async (): Promise => { - vi.useRealTimers(); - await Promise.all(stubs.map((stub): Promise => stub.close())); - stubs.length = 0; - vi.restoreAllMocks(); + try { + await Promise.all(stubs.map((stub): Promise => stub.close())); + expect(vi.getTimerCount()).toBe(0); + } finally { + stubs.length = 0; + vi.restoreAllMocks(); + vi.useRealTimers(); + } }); async function stub(handler: (request: StubRequest) => void): Promise { @@ -119,7 +131,7 @@ describe('fallback DNS client', (): void => { ); }); - it('ignores replies that do not match the query or come from another source', async (): Promise => { + it('ignores malformed replies and replies that do not match the query', async (): Promise => { const server = await stub((request): void => { const wrongId = reply(request); wrongId.writeUInt16BE(((request.query.id ?? 0) + 1) % 65536, 0); @@ -133,7 +145,7 @@ describe('fallback DNS client', (): void => { }), ); request.send(Buffer.from([0xff, 0xff, 0xff])); - server.later((): void => request.send(reply(request)), 10); + request.send(reply(request)); }); const result = await resolveSrvFallback(srvName, options([server])); expect(result.records.map((record): string => record.name)).toEqual([targetName]); @@ -144,13 +156,33 @@ describe('fallback DNS client', (): void => { if (request.transport === 'udp') { request.send(reply(request, [], dnsPacket.TRUNCATED_RESPONSE)); } else { - request.send(compressedReply(request.query), true); + request.send(compressedReply(request.query)); } }); + const emit: (this: net.Socket, event: string | symbol, ...args: unknown[]) => boolean = + net.Socket.prototype.emit; + let fragments = 0; + // TCP can coalesce writes, so split the client's data events rather than timing server writes. + vi.spyOn(net.Socket.prototype, 'emit').mockImplementation(function ( + this: net.Socket, + event: string | symbol, + ...args: unknown[] + ): boolean { + if (event === 'data' && `127.0.0.1:${this.remotePort}` === server.address && Buffer.isBuffer(args[0])) { + const bytes = args[0]; + for (let offset = 0; offset < bytes.length; offset++) { + fragments++; + emit.call(this, event, bytes.subarray(offset, offset + 1)); + } + return true; + } + return emit.call(this, event, ...args) === true; + }); const result = await resolveSrvFallback(srvName, options([server])); expect(result).toMatchObject({ transport: 'tcp', serversTried: 1 }); expect(result.records[0].name).toBe(targetName); expect(server.queries.map((request): string => request.transport)).toEqual(['udp', 'tcp']); + expect(fragments).toBeGreaterThan(2); }); it.each([ @@ -180,7 +212,6 @@ describe('fallback DNS client', (): void => { }); it('tries the next server after a silent server times out', async (): Promise => { - vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); const first = await stub((): void => { vi.advanceTimersByTime(25); }); @@ -190,7 +221,6 @@ describe('fallback DNS client', (): void => { }); it('retries a server once after a UDP timeout', async (): Promise => { - vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); const server = await stub((request): void => { if (server.queries.length === 1) { vi.advanceTimersByTime(25); @@ -202,7 +232,6 @@ describe('fallback DNS client', (): void => { expect(result.serversTried).toBe(1); expect(server.queries).toHaveLength(2); - expect(server.queries[0].query.id).not.toBe(server.queries[1].query.id); }); it('does not retry a server after SERVFAIL', async (): Promise => { @@ -226,7 +255,6 @@ describe('fallback DNS client', (): void => { }); it('bounds all servers by one shared deadline', async (): Promise => { - vi.useFakeTimers({ toFake: ['Date', 'setTimeout', 'clearTimeout'] }); const first = await stub((): void => { vi.advanceTimersByTime(200); }); @@ -272,23 +300,25 @@ describe('fallback DNS client', (): void => { await expect(outcome).rejects.toMatchObject({ code: dns.CONNREFUSED, syscall: 'queryTxt', serversTried: 0 }); }); - it('fails fast when a UDP server refuses the query', async (): Promise => { - const port = await new Promise((resolve, reject): void => { - const socket = dgram.createSocket('udp4'); - socket.once('error', reject); - socket.bind(0, '127.0.0.1', (): void => { - const address = socket.address(); - socket.close((): void => resolve(address.port)); + it.each(['ECONNREFUSED', 'ECONNRESET'])('fails fast when a UDP socket reports %s', async (code): Promise => { + const connect = vi.spyOn(dgram.Socket.prototype, 'connect'); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + // Closed-port ICMP errors are not reliably delivered on every OS or restricted runner. + const send = vi.spyOn(dgram.Socket.prototype, 'send').mockImplementation(function (this: dgram.Socket): void { + queueMicrotask((): void => { + this.emit('error', Object.assign(new Error(code), { code })); }); }); - const started = Date.now(); const outcome = resolveSrvFallback(srvName, { - servers: [`127.0.0.1:${port}`], - deadlineAt: started + 3000, + servers: ['127.0.0.1:53'], + deadlineAt: Date.now() + 3000, udpTimeoutMs: 2000, }); await expect(outcome).rejects.toMatchObject({ code: dns.CONNREFUSED, serversTried: 1 }); - expect(Date.now() - started).toBeLessThan(1000); + expect(connect).toHaveBeenCalledExactlyOnceWith(53, '127.0.0.1', expect.any(Function)); + expect(send).toHaveBeenCalledTimes(1); + expect(close).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); }); }); diff --git a/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts b/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts index 925131be0..b2243d2c8 100644 --- a/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts +++ b/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts @@ -164,17 +164,20 @@ describe('resolveSrvForDriver', (): void => { it('cancels a pending probe and rejects with the abort reason', async (): Promise => { const controller = new AbortController(); let rejectProbe!: (error: unknown) => void; - probe.mockImplementation( - (): Promise => - new Promise((_resolve, reject): void => { - rejectProbe = reject; - }), - ); + const probeStarted = new Promise((resolve): void => { + probe.mockImplementation( + (): Promise => + new Promise((_resolve, reject): void => { + rejectProbe = reject; + resolve(); + }), + ); + }); const cancel = vi .spyOn(dns.promises.Resolver.prototype, 'cancel') .mockImplementation((): void => rejectProbe(probeError('ECANCELLED'))); const result = run({ signal: controller.signal }); - await Promise.resolve(); + await probeStarted; const reason = new Error('stop'); controller.abort(reason); await expect(result).rejects.toBe(reason); diff --git a/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts b/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts index 2ed98ea67..1edd33084 100644 --- a/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts +++ b/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { MongoClient } from 'mongodb'; import * as dns from 'node:dns'; @@ -167,6 +167,12 @@ function rewriteOutcome(fixture: Fixture): Outcome { } describe('SRV rewrite differential against the installed MongoDB API driver', (): void => { + beforeEach((): void => { + const error = new Error('Driver DNS queries must use the mocked resolve function'); + vi.spyOn(dns.promises, 'resolveSrv').mockRejectedValue(error); + vi.spyOn(dns.promises, 'resolveTxt').mockRejectedValue(error); + }); + afterEach((): void => { vi.restoreAllMocks(); }); @@ -178,10 +184,13 @@ describe('SRV rewrite differential against the installed MongoDB API driver', () if (rrtype === 'SRV') { return srvRecords.map((record): SrvRecord => ({ ...record })); } - if (fixture.txtRecords === undefined) { - throw Object.assign(new Error('queryTxt ENODATA'), { code: 'ENODATA' }); + if (rrtype === 'TXT') { + if (fixture.txtRecords === undefined) { + throw Object.assign(new Error('queryTxt ENODATA'), { code: 'ENODATA' }); + } + return fixture.txtRecords.map((record): string[] => [...record]); } - return fixture.txtRecords.map((record): string[] => [...record]); + throw new Error(`Unexpected DNS record type: ${rrtype}`); }, ); const expected = await driverOutcome(fixture); diff --git a/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.test.ts b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.test.ts new file mode 100644 index 000000000..3a9ac7ae9 --- /dev/null +++ b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.test.ts @@ -0,0 +1,98 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import dgram from 'node:dgram'; +import net from 'node:net'; + +import { DnsStubServer } from './dnsStubServer.js'; + +describe('DNS stub server lifecycle', (): void => { + const stubs: DnsStubServer[] = []; + + afterEach(async (): Promise => { + try { + await Promise.all(stubs.map((stub): Promise => stub.close())); + } finally { + stubs.length = 0; + vi.restoreAllMocks(); + } + }); + + it('retries a TCP port conflict after closing the partially started stub', async (): Promise => { + const error = Object.assign(new Error('port in use'), { code: 'EADDRINUSE' }); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + const listen = vi.spyOn(net.Server.prototype, 'listen').mockImplementationOnce(function ( + this: net.Server, + ): net.Server { + queueMicrotask((): void => { + this.emit('error', error); + }); + return this; + }); + + const server = await DnsStubServer.start((): void => {}); + stubs.push(server); + + expect(server.address).toMatch(/^127\.0\.0\.1:\d+$/); + expect(listen).toHaveBeenCalledTimes(2); + expect(close).toHaveBeenCalledTimes(1); + }); + + it.each([ + ['EACCES', 1], + ['EADDRINUSE', 3], + ])('cleans up and reports %s after %i attempts', async (code, attempts): Promise => { + const error = Object.assign(new Error(code), { code }); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + const listen = vi.spyOn(net.Server.prototype, 'listen').mockImplementation(function ( + this: net.Server, + ): net.Server { + queueMicrotask((): void => { + this.emit('error', error); + }); + return this; + }); + + await expect(DnsStubServer.start((): void => {})).rejects.toBe(error); + + expect(listen).toHaveBeenCalledTimes(attempts); + expect(close).toHaveBeenCalledTimes(attempts); + }); + + it('preserves UDP bind errors without trying to close unbound sockets', async (): Promise => { + const error = Object.assign(new Error('bind denied'), { code: 'EACCES' }); + const bind = vi.spyOn(dgram.Socket.prototype, 'bind').mockImplementationOnce(function ( + this: dgram.Socket, + ): dgram.Socket { + queueMicrotask((): void => { + this.emit('error', error); + }); + return this; + }); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + const listen = vi.spyOn(net.Server.prototype, 'listen'); + + await expect(DnsStubServer.start((): void => {})).rejects.toBe(error); + + expect(bind).toHaveBeenCalledTimes(1); + expect(close).not.toHaveBeenCalled(); + expect(listen).not.toHaveBeenCalled(); + }); + + it('can close an already closed stub', async (): Promise => { + const server = await DnsStubServer.start((): void => {}); + stubs.push(server); + const closeUdp = vi.spyOn(dgram.Socket.prototype, 'close'); + const closeTcp = vi.spyOn(net.Server.prototype, 'close'); + + await server.close(); + await server.close(); + + expect(closeUdp).toHaveBeenCalledTimes(1); + expect(closeTcp).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts index b7a16c291..4f9b6bb8f 100644 --- a/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts +++ b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts @@ -12,7 +12,7 @@ import { type DnsTransport } from '../fallbackDnsClient.js'; export interface StubRequest { readonly query: dnsPacket.DecodedPacket; readonly transport: DnsTransport; - send(bytes: Buffer, split?: boolean): void; + send(bytes: Buffer): void; } export class DnsStubServer { @@ -21,7 +21,7 @@ export class DnsStubServer { public address = ''; private readonly udp = dgram.createSocket('udp4'); private readonly tcp = net.createServer(); - private readonly timers = new Set(); + private udpBound = false; private closed = false; private constructor(private readonly handler: (request: StubRequest) => void) { @@ -50,21 +50,10 @@ export class DnsStubServer { } const bytes = received.subarray(2, length + 2); received = received.subarray(length + 2); - this.dispatch(bytes, 'tcp', (response, split): void => { + this.dispatch(bytes, 'tcp', (response): void => { const prefix = Buffer.alloc(2); prefix.writeUInt16BE(response.length); - const framed = Buffer.concat([prefix, response]); - if (!split) { - socket.write(framed); - return; - } - socket.write(framed.subarray(0, 1)); - this.later((): void => { - socket.write(framed.subarray(1, 5)); - }, 5); - this.later((): void => { - socket.write(framed.subarray(5)); - }, 10); + socket.write(Buffer.concat([prefix, response])); }); } }); @@ -72,55 +61,66 @@ export class DnsStubServer { } public static async start(handler: (request: StubRequest) => void): Promise { - const stub = new DnsStubServer(handler); - await new Promise((resolve, reject): void => { - stub.udp.once('error', reject); - stub.udp.bind(0, '127.0.0.1', (): void => { - stub.udp.removeListener('error', reject); - resolve(); - }); - }); - const port = stub.udp.address().port; - await new Promise((resolve, reject): void => { - stub.tcp.once('error', reject); - stub.tcp.listen(port, '127.0.0.1', (): void => { - stub.tcp.removeListener('error', reject); - resolve(); - }); - }); - stub.address = `127.0.0.1:${port}`; - return stub; - } - - public later(callback: () => void, delayMs: number): void { - const timer = setTimeout((): void => { - this.timers.delete(timer); - if (!this.closed) { - callback(); + for (let attempt = 0; ; attempt++) { + const stub = new DnsStubServer(handler); + try { + await new Promise((resolve, reject): void => { + stub.udp.once('error', reject); + stub.udp.bind(0, '127.0.0.1', (): void => { + stub.udpBound = true; + stub.udp.removeListener('error', reject); + resolve(); + }); + }); + const port = stub.udp.address().port; + await new Promise((resolve, reject): void => { + stub.tcp.once('error', reject); + stub.tcp.listen(port, '127.0.0.1', (): void => { + stub.tcp.removeListener('error', reject); + resolve(); + }); + }); + stub.address = `127.0.0.1:${port}`; + return stub; + } catch (error) { + await stub.close(); + // A UDP-assigned port can already be occupied by an unrelated TCP listener. + if (!(error instanceof Error && 'code' in error && error.code === 'EADDRINUSE' && attempt < 2)) { + throw error; + } } - }, delayMs); - this.timers.add(timer); + } } public async close(): Promise { - this.closed = true; - for (const timer of this.timers) { - clearTimeout(timer); + if (this.closed) { + return; } + this.closed = true; for (const socket of this.tcpSockets) { socket.destroy(); } await Promise.all([ - new Promise((resolve): void => { - this.udp.close((): void => resolve()); - }), - new Promise((resolve): void => { - this.tcp.close((): void => resolve()); - }), + this.udpBound + ? new Promise((resolve): void => { + this.udp.close((): void => resolve()); + }) + : Promise.resolve(), + this.tcp.listening + ? new Promise((resolve, reject): void => { + this.tcp.close((error): void => { + if (error) { + reject(error); + } else { + resolve(); + } + }); + }) + : Promise.resolve(), ]); } - private dispatch(bytes: Buffer, transport: DnsTransport, send: (response: Buffer, split?: boolean) => void): void { + private dispatch(bytes: Buffer, transport: DnsTransport, send: (response: Buffer) => void): void { const request: StubRequest = { query: dnsPacket.decode(bytes), transport, send }; this.queries.push(request); this.handler(request);