fix(server): Stop re-probing hosting CLIs for unrecognized hosts

apps/server/src/sourceControl/SourceControlProviderRegistry.test.ts

@@ -2,12 +2,23 @@ import { SourceControlProviderKind } from "@t3tools/contracts";
 import { assert, it } from "@effect/vitest";
 import * as NodeServices from "@effect/platform-node/NodeServices";
 import * as DateTime from "effect/DateTime";
+import * as Deferred from "effect/Deferred";
 import * as Effect from "effect/Effect";
+import * as Fiber from "effect/Fiber";
 import * as Layer from "effect/Layer";
 import * as Option from "effect/Option";
+import * as PlatformError from "effect/PlatformError";
+import * as PubSub from "effect/PubSub";
 import * as Redacted from "effect/Redacted";
+import * as Stream from "effect/Stream";
+import * as TestClock from "effect/testing/TestClock";
 import { ChildProcessSpawner } from "effect/process";
-import { VcsRepositoryDetectionError } from "@t3tools/contracts";
+import {
+  DEFAULT_SERVER_SETTINGS,
+  VcsProcessSpawnError,
+  VcsProcessTimeoutError,
+  VcsRepositoryDetectionError,
+} from "@t3tools/contracts";
 
 import * as ServerSettings from "../serverSettings.ts";
 import * as ServerConfig from "../config.ts";
@@ -54,6 +65,8 @@ function makeRegistry(input: {
   readonly process?: Partial<VcsProcess.VcsProcess["Service"]>;
   readonly githubApi?: Partial<GitHubApi.GitHubApi["Service"]>;
   readonly gitlab?: Partial<GitLabCli.GitLabCli["Service"]>;
+  readonly forgejo?: Partial<ForgejoCli.ForgejoCli["Service"]>;
+  readonly settings?: Partial<ServerSettings.ServerSettingsService["Service"]>;
   readonly resolve?: VcsDriverRegistry.VcsDriverRegistry["Service"]["resolve"];
 }) {
   const driver = {
@@ -118,7 +131,9 @@ function makeRegistry(input: {
         Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({}),
         Layer.mock(BitbucketApi.BitbucketApi)({}),
         Layer.mock(BitbucketPullRequestApi.BitbucketPullRequestApi)({}),
-        ServerSettings.ServerSettingsService.layerTest(),
+        input.settings
+          ? Layer.mock(ServerSettings.ServerSettingsService)(input.settings)
+          : ServerSettings.ServerSettingsService.layerTest(),
         Layer.mock(GitHubPullRequestApi.GitHubPullRequestApi)({}),
         Layer.mock(GitHubApi.GitHubApi)({
           // No GitHub credential unless a test supplies one, so custom hosts stay unclaimed.
@@ -128,7 +143,10 @@ function makeRegistry(input: {
         Layer.mock(GitVcsDriver.GitVcsDriver)({}),
         Layer.mock(GitLabCli.GitLabCli)(input.gitlab ?? {}),
         Layer.mock(GitLabPullRequestCli.GitLabPullRequestCli)({}),
-        Layer.mock(ForgejoCli.ForgejoCli)({ listLogins: () => Effect.succeed([]) }),
+        Layer.mock(ForgejoCli.ForgejoCli)({
+          listLogins: () => Effect.succeed([]),
+          ...input.forgejo,
+        }),
         Layer.mock(GitCafeApi.GitCafeApi)({}),
         Layer.mock(GitCafeCredentials.GitCafeCredentials)({
           cliEnv: () => Effect.succeed({}),
@@ -459,3 +477,150 @@ it.effect("skips GitHub discovery for the identity resolver's empty base URL", (
     assert.deepStrictEqual(hosts, ["githubenterprise.dev.example.com"]);
   }),
 );
+
+const notFound = (command: string, cwd: string) =>
+  new VcsProcessSpawnError({
+    operation: "test",
+    command,
+    cwd,
+    cause: PlatformError.systemError({ _tag: "NotFound", module: "ChildProcess", method: "spawn" }),
+  });
+
+const resolveUnknown = (
+  registry: SourceControlProviderRegistry.SourceControlProviderRegistry["Service"],
+  repository: string,
+) =>
+  registry.resolveHandle({
+    cwd: `/${repository}`,
+    context: {
+      provider: {
+        kind: SourceControlProviderKind.make("unknown"),
+        name: "git.example.internal",
+        baseUrl: "https://git.example.internal",
+      },
+      remoteName: "origin",
+      remoteUrl: `git@Git.Example.Internal:team/${repository}.git`,
+    },
+  });
+
+it.effect("asks once for every checkout of a server no integration claims", () =>
+  Effect.gen(function* () {
+    const asked: Array<string> = [];
+    const release = yield* Deferred.make<void>();
+    const registry = yield* makeRegistry({
+      remotes: [],
+      process: {
+        run: ({ command, args, cwd }) => {
+          asked.push([command, ...args].join(" "));
+          return Deferred.await(release).pipe(Effect.andThen(Effect.fail(notFound(command, cwd))));
+        },
+      },
+      githubApi: {
+        credential: (host) => {
+          asked.push(`credential ${host}`);
+          return Effect.fail(new GitHubCredentials.GitHubNotSignedInError({ host }));
+        },
+      },
+      forgejo: {
+        listLogins: ({ command = "tea", cwd }) => {
+          asked.push(command);
+          const cause = notFound(command, cwd);
+          return Effect.fail(new ForgejoCli.ForgejoCliError({ command, cwd, detail: "", cause }));
+        },
+      },
+    });
+
+    // Three checkouts read while the first probe is still running share it.
+    const started = yield* Effect.forEach(["a", "b", "c"], () => Deferred.make<void>());
+    const callers = yield* Effect.forEach(
+      ["a", "b", "c"],
+      (repository, index) =>
+        Deferred.succeed(started[index]!, undefined).pipe(
+          Effect.andThen(resolveUnknown(registry, repository)),
+        ),
+      { concurrency: "unbounded" },
+    ).pipe(Effect.forkChild);
+    yield* Effect.forEach(started, Deferred.await);
+    yield* Deferred.succeed(release, undefined);
+    const handles = yield* Fiber.join(callers);
+    yield* resolveUnknown(registry, "d");
+    assert.deepStrictEqual(
+      handles.map((handle) => handle.provider.kind),
+      ["unknown", "unknown", "unknown"],
+    );
+    const once = asked.slice();
+    // glab is asked about the remote's host as the remote spells it.
+    assert.sameMembers(once, [
+      "credential git.example.internal",
+      "glab auth status --hostname Git.Example.Internal",
+      "fj",
+      "tea",
+    ]);
+
+    // A login made in a terminal sends no signal, so the server is asked again later.
+    yield* TestClock.adjust("5 minutes");
+    yield* resolveUnknown(registry, "a");
+    assert.deepStrictEqual(asked, [...once, ...once]);
+  }),
+);
+
+it.effect("asks again after a check that could not answer", () =>
+  Effect.gen(function* () {
+    let asks = 0;
+    const registry = yield* makeRegistry({
+      remotes: [],
+      process: {
+        run: ({ command, cwd }) => {
+          asks += 1;
+          return Effect.fail(
+            new VcsProcessTimeoutError({ operation: "test", command, cwd, timeoutMs: 1 }),
+          );
+        },
+      },
+    });
+    yield* resolveUnknown(registry, "a");
+    yield* resolveUnknown(registry, "b");
+    assert.strictEqual(asks, 2);
+  }),
+);
+
+it.effect("asks again once Settings change", () =>
+  Effect.gen(function* () {
+    const changes = yield* PubSub.unbounded<typeof DEFAULT_SERVER_SETTINGS>();
+    const handled = yield* Deferred.make<void>();
+    let signedIn = false;
+    const registry = yield* makeRegistry({
+      remotes: [],
+      settings: {
+        subscribeChanges: PubSub.subscribe(changes).pipe(
+          Effect.map((subscription) =>
+            // A change is handled once the registry asks for the next one.
+            Stream.fromSubscription(subscription).pipe(
+              Stream.flatMap((settings) =>
+                Stream.concat(
+                  Stream.make(settings),
+                  Stream.fromEffectDrain(Deferred.succeed(handled, undefined)),
+                ),
+              ),
+            ),
+          ),
+        ),
+      },
+      process: { run: ({ command, cwd }) => Effect.fail(notFound(command, cwd)) },
+      githubApi: {
+        credential: (host) =>
+          signedIn
+            ? Effect.succeed({ token: Redacted.make("token"), fingerprint: `${host}:fingerprint` })
+            : Effect.fail(new GitHubCredentials.GitHubNotSignedInError({ host })),
+      },
+    });
+    assert.strictEqual((yield* resolveUnknown(registry, "a")).provider.kind, "unknown");
+    signedIn = true;
+    assert.strictEqual((yield* resolveUnknown(registry, "b")).provider.kind, "unknown");
+
+    // A token saved for the host in Settings.
+    yield* PubSub.publish(changes, DEFAULT_SERVER_SETTINGS);
+    yield* Deferred.await(handled);
+    assert.strictEqual((yield* resolveUnknown(registry, "c")).provider.kind, "github");
+  }),
+);

apps/server/src/sourceControl/SourceControlProviderRegistry.ts

@@ -3,8 +3,11 @@ import * as Cache from "effect/Cache";
 import * as Context from "effect/Context";
 import * as Duration from "effect/Duration";
 import * as Effect from "effect/Effect";
+import * as Equal from "effect/Equal";
 import * as Exit from "effect/Exit";
+import * as Hash from "effect/Hash";
 import * as Layer from "effect/Layer";
+import * as Stream from "effect/Stream";
 import {
   SourceControlProviderError,
   type SourceControlProviderDiscoveryItem,
@@ -18,14 +21,19 @@ import {
   probeSourceControlProvider,
   refineUnknownRemoteProvider,
   type SourceControlProviderDiscoverySpec,
+  type UnknownRemoteVerdict,
 } from "@t3tools/source-control-core/server/discovery";
 import * as SourceControlHost from "@t3tools/source-control-core/server/SourceControlHost";
 
 import * as ServerConfig from "../config.ts";
+import * as ServerSettings from "../serverSettings.ts";
 import * as VcsDriverRegistry from "../vcs/VcsDriverRegistry.ts";
 
 const PROVIDER_DETECTION_CACHE_CAPACITY = 2_048;
 const PROVIDER_DETECTION_CACHE_TTL = Duration.seconds(5);
+const UNKNOWN_REMOTE_CACHE_CAPACITY = 512;
+// Outlasts the one-minute pull request sweep. A login made since is noticed after this.
+const UNKNOWN_REMOTE_CACHE_TTL = Duration.minutes(5);
 
 export interface SourceControlProviderRegistration {
   readonly kind: SourceControlProviderKind;
@@ -150,6 +158,38 @@ function selectProviderContext(
   );
 }
 
+/**
+ * One lookup per server: remotes that differ only in `owner/repo` share a verdict, and callers
+ * that arrive while it runs wait on it. `cwd` is left out because it only matters when a probe
+ * fails there, and a failed probe is not kept. Equality ignores `lookup`, so this is not a
+ * `Data.Class`.
+ */
+class UnknownRemoteLookup {
+  readonly key: string;
+  readonly lookup: Effect.Effect<UnknownRemoteVerdict>;
+
+  constructor(
+    context: SourceControlProvider.SourceControlProviderContext,
+    lookup: Effect.Effect<UnknownRemoteVerdict>,
+  ) {
+    this.lookup = lookup;
+    this.key = [
+      context.provider.name,
+      context.provider.baseUrl,
+      // Strip a trailing `owner/repo`, never the host after `//`, so one-segment remotes stay apart.
+      context.remoteUrl.trim().replace(/(?<!\/\/[^/]*)[^/:]+\/[^/:]+\/?$/u, ""),
+      context.requestedHost ?? "",
+    ].join("\n");
+  }
+
+  [Equal.symbol](that: unknown): boolean {
+    return that instanceof UnknownRemoteLookup && that.key === this.key;
+  }
+  [Hash.symbol](): number {
+    return Hash.string(this.key);
+  }
+}
+
 function bindProviderContext(
   provider: SourceControlProvider.SourceControlProvider["Service"],
   context: SourceControlProvider.SourceControlProviderContext | null,
@@ -198,6 +238,7 @@ function bindProviderContext(
 export const makeWithProviders = Effect.fn("makeSourceControlProviderRegistryWithProviders")(
   function* (registrations: ReadonlyArray<SourceControlProviderRegistration>) {
     const config = yield* ServerConfig.ServerConfig;
+    const serverSettings = yield* ServerSettings.ServerSettingsService;
     const { process } = yield* SourceControlHost.SourceControlHost;
     const vcsRegistry = yield* VcsDriverRegistry.VcsDriverRegistry;
     const providers = new Map<
@@ -209,6 +250,36 @@ export const makeWithProviders = Effect.fn("makeSourceControlProviderRegistryWit
     const get: SourceControlProviderRegistry["Service"]["get"] = (kind) =>
       Effect.succeed(providers.get(kind) ?? unsupportedProvider(kind));
 
+    // "No integration holds this host" is the common verdict and the one worth keeping. A verdict
+    // reached while some probe failed is handed to the callers waiting on it, then dropped.
+    const unknownRemoteCache = yield* Cache.makeWith(
+      (request: UnknownRemoteLookup) => request.lookup,
+      {
+        capacity: UNKNOWN_REMOTE_CACHE_CAPACITY,
+        timeToLive: (exit) =>
+          Exit.isSuccess(exit) && exit.value.settled ? UNKNOWN_REMOTE_CACHE_TTL : Duration.zero,
+      },
+    );
+    // A token saved or a host toggled in Settings must show on the next read, not after the TTL.
+    const settingsChanges = yield* serverSettings.subscribeChanges;
+    yield* Stream.runForEach(settingsChanges, () => Cache.invalidateAll(unknownRemoteCache)).pipe(
+      Effect.forkScoped,
+    );
+
+    const refineContext = (
+      cwd: string,
+      context: SourceControlProvider.SourceControlProviderContext | null,
+    ): Effect.Effect<SourceControlProvider.SourceControlProviderContext | null> =>
+      context === null || context.provider.kind !== "unknown"
+        ? Effect.succeed(context)
+        : Cache.get(
+            unknownRemoteCache,
+            new UnknownRemoteLookup(
+              context,
+              refineUnknownRemoteProvider({ specs: discoverySpecs, process, cwd, context }),
+            ),
+          ).pipe(Effect.map(({ provider }) => (provider ? { ...context, provider } : context)));
+
     const detectProviderContext = Effect.fn("SourceControlProviderRegistry.detectProviderContext")(
       function* (cwd: string) {
         const handle = yield* vcsRegistry.resolve({ cwd }).pipe(
@@ -235,14 +306,7 @@ export const makeWithProviders = Effect.fn("makeSourceControlProviderRegistryWit
               }),
           ),
         );
-        const context = selectProviderContext(remotes.remotes);
-
-        return yield* refineUnknownRemoteProvider({
-          specs: discoverySpecs,
-          process,
-          cwd,
-          context,
-        });
+        return yield* refineContext(cwd, selectProviderContext(remotes.remotes));
       },
     );
 
@@ -258,12 +322,7 @@ export const makeWithProviders = Effect.fn("makeSourceControlProviderRegistryWit
     const resolveHandle: SourceControlProviderRegistry["Service"]["resolveHandle"] = (input) =>
       (input.context === undefined
         ? Cache.get(providerContextCache, input.cwd)
-        : refineUnknownRemoteProvider({
-            specs: discoverySpecs,
-            process,
-            cwd: input.cwd,
-            context: input.context,
-          })
+        : refineContext(input.cwd, input.context)
       ).pipe(
         Effect.map((context) => {
           const kind = context?.provider.kind ?? SourceControlProviderKind.make("unknown");