Forgejo: workspace index caches failed origin lookups as "not connected" #59

New issue

Closed

opened 2026-10-22 17:58:45 +02:00 by omp-deck · 0 comments

omp-deck commented

2026-10-22 17:58:45 +02:00

Collaborator

Implementation: #60

Problem

On 2026-10-22, a comment on PR #56 (delivery 959dc166-9682-448b-91ce-7c53e39af6b8, 15:23:59Z) was dropped with "repository is not connected to a workspace". The bot posted NOT_CONNECTED, even though /workspace/omp-deck is registered and its origin is ssh://git@git.sclu1034.dev/lucas/omp-deck.git. A PR #58 trigger at 15:22:08Z had matched the same workspace. The process that dropped the comment is gone. In the current environment, resolveRepoFromCwd("/workspace/omp-deck", "https://git.sclu1034.dev") returns lucas/omp-deck.

Cause: apps/server/src/forgejo/workspace-index.ts stores the null from repoKey in the index whether the origin really is somewhere else or git remote get-url origin failed. readOriginUrl (forgejo/repo.ts) turns every nonzero exit into null and throws away git's error. matchWorkspaces only resolves workspaces that have no entry yet, so after one failed git call (e.g. when the directory is not mounted yet at container start) the workspace stays "not connected" until the next restart or until it is registered again. Nothing is logged.

runGit tells the cases apart:

exit 0: origin read.
exit 2 (error: No such remote 'origin'): the repo has no origin. This is a real answer.
exit 128 (fatal: not a git repository): this is also what an unmounted directory looks like.
code -1: spawn failed, timeout, missing cwd (ENOENT), or aborted.

Decisions:

Only exit 0 and exit 2 count as answers and get cached. Every other result is a failure and is not cached.
A workspace whose lookup failed is retried on every matchWorkspaces call until a lookup gives an answer. There is no backoff, because it is one local git call per failed workspace.
Every failure is logged with log.warn, giving the workspace path and git's error text.

Plan

Context

Make the workspace index cache only real answers about a workspace's origin, retry failed lookups on the next match, and log failures. Then a transient git failure can no longer make a registered repository "not connected" until restart.

Approach

Step 1 builds on its own and keeps existing tests green. Step 2 depends on step 1.

1. Report failures separately from "no origin" (`apps/server/src/forgejo/repo.ts`)

Add, next to readOriginUrl:

/** Result of reading `origin`: `url` is null when the repo has no `origin` remote. */
export type OriginLookup = { ok: true; url: string | null } | { ok: false; error: string };

/**
 * Read `origin` as git reports it (after `insteadOf` rewriting). Exit 2 means
 * the repo has no `origin` and counts as an answer; any other failure (not a
 * repo, missing directory, spawn error, timeout, abort) is `ok: false` with
 * git's error text, because it may be transient.
 */
export async function lookupOrigin(cwd: string, signal?: AbortSignal): Promise<OriginLookup>

Implementation:

Call runGit(cwd, ["remote", "get-url", "origin"], { readOnly: true, signal }).
code === 0 → { ok: true, url: res.stdout.trim() || null }.
code === 2 → { ok: true, url: null }.
Anything else → { ok: false, error: gitErrorText(res, "git remote get-url origin failed") }. Import gitErrorText from ../bridge/git-tools/run.ts; runGit is already imported there.

Rewrite readOriginUrl on top of it: const r = await lookupOrigin(cwd, signal); return r.ok ? r.url : null;. Keep its signature and doc comment. Its callers (checkout.ts, resolveRepoFromCwd) and resolveRepoFromCwd's callers (routes-forgejo.ts, tools/index.ts) stay unchanged. They don't cache anything.

2. Cache only answers in the index (`apps/server/src/forgejo/workspace-index.ts`)

Add import { logger } from "../log.ts"; and const log = logger("forgejo:workspace-index");, following the same pattern as checkout.ts.
Change repoKey(path, instanceUrl) to return Promise<string | null | undefined>. undefined means the lookup failed and must not be cached. It calls lookupOrigin(path) instead of resolveRepoFromCwd:
- !r.ok → log.warn(\could not read origin of workspace ${path}: ${r.error}`), then return undefined`.
- r.url === null → null.
- Otherwise → const ref = matchRemoteToInstance(r.url, instanceUrl); return ref ? \${ref.owner}/${ref.repo}`.toLowerCase() : null;`
- Replace the resolveRepoFromCwd import with lookupOrigin, matchRemoteToInstance.
ensureBuilt: only call next.set(path, key) when key !== undefined.
matchWorkspaces: in the unindexed.forEach, only set entries whose key is not undefined. A workspace without an entry is already in unindexed on every lookup, so this alone gives the retry on every lookup. Don't add any other retry state.
noteWorkspaceAdded: only call target.set(ws.path, key) in the .then when key !== undefined. Leave the !cfg branch (index.set(ws.path, null)) as is: no Forgejo config is an answer.
Update the type comment on index to: /** Workspace path → lowercased \owner/repo`, or null when its origin is not on the configured instance or absent. Workspaces whose origin could not be read have no entry. */`. In the module doc comment, replace the sentence "Every lookup also resolves registered workspaces the index has no entry for yet (registered while the build ran, or whose resolution is still in flight), so a fresh registration is never missed." with "Every lookup also resolves registered workspaces the index has no entry for (registered while the build ran, resolution still in flight, or a previous attempt failed with a git error), so a fresh registration or a transient failure is never stuck."

Critical files & anchors

apps/server/src/forgejo/workspace-index.ts — repoKey, ensureBuilt, matchWorkspaces, noteWorkspaceAdded.
apps/server/src/forgejo/repo.ts — readOriginUrl (~147), new lookupOrigin.
apps/server/src/forgejo/workspace-index.test.ts — the boot() and checkout() helpers to reuse.

Verification

Add these to apps/server/src/forgejo/workspace-index.test.ts, reusing boot(), tmp() and checkout(). Spawn git with Bun.spawnSync(["git", ...], { cwd }) as checkout() does.

"a workspace whose origin could not be read is retried on the next lookup": boot(). const dir = tmp("omp-deck-ws-index-repo-"). This is a plain directory (not a repo, so git exits 128), the way an unmounted directory looks. const ws = createWorkspace({ path: dir, name: "late" }). expect(await matchWorkspaces("o/late")).toEqual([]). Then run git init -q and git remote add origin https://git.example.com/o/late.git in dir. expect((await matchWorkspaces("o/late")).map((w) => w.id)).toEqual([ws.id]). This test fails on the current code.
"a repo without origin is an answer and stays cached": boot(). Make a dir with git init -q only and createWorkspace it. expect(await matchWorkspaces("o/x")).toEqual([]). Then git remote add origin https://git.example.com/o/x.git. expect(await matchWorkspaces("o/x")).toEqual([]). This is the documented behaviour: a changed origin is only picked up after a restart or registering again.
In apps/server/src/forgejo/repo.test.ts, add one test for lookupOrigin:
- a repo with origin → { ok: true, url };
- tmpRepo() with no origin → { ok: true, url: null };
- a plain temp dir → ok: false with error containing not a git repository;
- a missing path → ok: false.

Run from the repo root: bun test apps/server/src/forgejo, then bun run typecheck. Both must pass, and the existing resolveRepoFromCwd null cases in repo.test.ts (~162–172) must stay green.

Implementation: #60 ## Problem On 2026-10-22, a comment on PR #56 (delivery `959dc166-9682-448b-91ce-7c53e39af6b8`, 15:23:59Z) was dropped with "repository is not connected to a workspace". The bot posted `NOT_CONNECTED`, even though `/workspace/omp-deck` is registered and its origin is `ssh://git@git.sclu1034.dev/lucas/omp-deck.git`. A PR #58 trigger at 15:22:08Z had matched the same workspace. The process that dropped the comment is gone. In the current environment, `resolveRepoFromCwd("/workspace/omp-deck", "https://git.sclu1034.dev")` returns `lucas/omp-deck`. Cause: `apps/server/src/forgejo/workspace-index.ts` stores the `null` from `repoKey` in the index whether the origin really is somewhere else or `git remote get-url origin` failed. `readOriginUrl` (`forgejo/repo.ts`) turns every nonzero exit into `null` and throws away git's error. `matchWorkspaces` only resolves workspaces that have *no* entry yet, so after one failed git call (e.g. when the directory is not mounted yet at container start) the workspace stays "not connected" until the next restart or until it is registered again. Nothing is logged. `runGit` tells the cases apart: - exit 0: origin read. - exit 2 (`error: No such remote 'origin'`): the repo has no origin. This is a real answer. - exit 128 (`fatal: not a git repository`): this is also what an unmounted directory looks like. - code -1: spawn failed, timeout, missing cwd (ENOENT), or aborted. Decisions: - Only exit 0 and exit 2 count as answers and get cached. Every other result is a failure and is not cached. - A workspace whose lookup failed is retried on every `matchWorkspaces` call until a lookup gives an answer. There is no backoff, because it is one local git call per failed workspace. - Every failure is logged with `log.warn`, giving the workspace path and git's error text. ## Plan ### Context Make the workspace index cache only real answers about a workspace's origin, retry failed lookups on the next match, and log failures. Then a transient git failure can no longer make a registered repository "not connected" until restart. ### Approach Step 1 builds on its own and keeps existing tests green. Step 2 depends on step 1. #### 1. Report failures separately from "no origin" (`apps/server/src/forgejo/repo.ts`) - Add, next to `readOriginUrl`: ```ts /** Result of reading `origin`: `url` is null when the repo has no `origin` remote. */ export type OriginLookup = { ok: true; url: string | null } | { ok: false; error: string }; /** * Read `origin` as git reports it (after `insteadOf` rewriting). Exit 2 means * the repo has no `origin` and counts as an answer; any other failure (not a * repo, missing directory, spawn error, timeout, abort) is `ok: false` with * git's error text, because it may be transient. */ export async function lookupOrigin(cwd: string, signal?: AbortSignal): Promise<OriginLookup> ``` Implementation: - Call `runGit(cwd, ["remote", "get-url", "origin"], { readOnly: true, signal })`. - `code === 0` → `{ ok: true, url: res.stdout.trim() || null }`. - `code === 2` → `{ ok: true, url: null }`. - Anything else → `{ ok: false, error: gitErrorText(res, "git remote get-url origin failed") }`. Import `gitErrorText` from `../bridge/git-tools/run.ts`; `runGit` is already imported there. - Rewrite `readOriginUrl` on top of it: `const r = await lookupOrigin(cwd, signal); return r.ok ? r.url : null;`. Keep its signature and doc comment. Its callers (`checkout.ts`, `resolveRepoFromCwd`) and `resolveRepoFromCwd`'s callers (`routes-forgejo.ts`, `tools/index.ts`) stay unchanged. They don't cache anything. #### 2. Cache only answers in the index (`apps/server/src/forgejo/workspace-index.ts`) - Add `import { logger } from "../log.ts";` and `const log = logger("forgejo:workspace-index");`, following the same pattern as `checkout.ts`. - Change `repoKey(path, instanceUrl)` to return `Promise<string | null | undefined>`. `undefined` means the lookup failed and must not be cached. It calls `lookupOrigin(path)` instead of `resolveRepoFromCwd`: - `!r.ok` → `log.warn(\`could not read origin of workspace ${path}: ${r.error}\`)`, then return `undefined`. - `r.url === null` → `null`. - Otherwise → `const ref = matchRemoteToInstance(r.url, instanceUrl); return ref ? \`${ref.owner}/${ref.repo}\`.toLowerCase() : null;` - Replace the `resolveRepoFromCwd` import with `lookupOrigin, matchRemoteToInstance`. - `ensureBuilt`: only call `next.set(path, key)` when `key !== undefined`. - `matchWorkspaces`: in the `unindexed.forEach`, only set entries whose key is not `undefined`. A workspace without an entry is already in `unindexed` on every lookup, so this alone gives the retry on every lookup. Don't add any other retry state. - `noteWorkspaceAdded`: only call `target.set(ws.path, key)` in the `.then` when `key !== undefined`. Leave the `!cfg` branch (`index.set(ws.path, null)`) as is: no Forgejo config is an answer. - Update the type comment on `index` to: `/** Workspace path → lowercased \`owner/repo\`, or null when its origin is not on the configured instance or absent. Workspaces whose origin could not be read have no entry. */`. In the module doc comment, replace the sentence "Every lookup also resolves registered workspaces the index has no entry for yet (registered while the build ran, or whose resolution is still in flight), so a fresh registration is never missed." with "Every lookup also resolves registered workspaces the index has no entry for (registered while the build ran, resolution still in flight, or a previous attempt failed with a git error), so a fresh registration or a transient failure is never stuck." ### Critical files & anchors - `apps/server/src/forgejo/workspace-index.ts` — `repoKey`, `ensureBuilt`, `matchWorkspaces`, `noteWorkspaceAdded`. - `apps/server/src/forgejo/repo.ts` — `readOriginUrl` (~147), new `lookupOrigin`. - `apps/server/src/forgejo/workspace-index.test.ts` — the `boot()` and `checkout()` helpers to reuse. ### Verification Add these to `apps/server/src/forgejo/workspace-index.test.ts`, reusing `boot()`, `tmp()` and `checkout()`. Spawn git with `Bun.spawnSync(["git", ...], { cwd })` as `checkout()` does. - **"a workspace whose origin could not be read is retried on the next lookup"**: `boot()`. `const dir = tmp("omp-deck-ws-index-repo-")`. This is a plain directory (not a repo, so git exits 128), the way an unmounted directory looks. `const ws = createWorkspace({ path: dir, name: "late" })`. `expect(await matchWorkspaces("o/late")).toEqual([])`. Then run `git init -q` and `git remote add origin https://git.example.com/o/late.git` in `dir`. `expect((await matchWorkspaces("o/late")).map((w) => w.id)).toEqual([ws.id])`. This test fails on the current code. - **"a repo without origin is an answer and stays cached"**: `boot()`. Make a dir with `git init -q` only and `createWorkspace` it. `expect(await matchWorkspaces("o/x")).toEqual([])`. Then `git remote add origin https://git.example.com/o/x.git`. `expect(await matchWorkspaces("o/x")).toEqual([])`. This is the documented behaviour: a changed origin is only picked up after a restart or registering again. - In `apps/server/src/forgejo/repo.test.ts`, add one test for `lookupOrigin`: - a repo with origin → `{ ok: true, url }`; - `tmpRepo()` with no origin → `{ ok: true, url: null }`; - a plain temp dir → `ok: false` with `error` containing `not a git repository`; - a missing path → `ok: false`. Run from the repo root: `bun test apps/server/src/forgejo`, then `bun run typecheck`. Both must pass, and the existing `resolveRepoFromCwd` null cases in `repo.test.ts` (~162–172) must stay green.