-
Notifications
You must be signed in to change notification settings - Fork 938
host-service: integration test suite + 3 v2 bug fixes #3915
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
77581b9
0bf7966
db52694
15e8b34
1047e8a
cb62467
a9d1225
27c78b1
a316366
ca37c5f
8c35881
5a4e2d9
15c8e29
450144e
5812eaf
705f924
4f13464
53e5a4c
c913cc5
963a8e3
80e683b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,7 @@ import type { MiddlewareHandler } from "hono"; | |||||||||||||||||||||||||||||||||
| import { Hono } from "hono"; | ||||||||||||||||||||||||||||||||||
| import { cors } from "hono/cors"; | ||||||||||||||||||||||||||||||||||
| import { createApiClient } from "./api"; | ||||||||||||||||||||||||||||||||||
| import { createDb } from "./db"; | ||||||||||||||||||||||||||||||||||
| import { createDb, type HostDb } from "./db"; | ||||||||||||||||||||||||||||||||||
| import { EventBus, registerEventBusRoute } from "./events"; | ||||||||||||||||||||||||||||||||||
| import type { ApiAuthProvider } from "./providers/auth"; | ||||||||||||||||||||||||||||||||||
| import type { HostAuthProvider } from "./providers/host-auth"; | ||||||||||||||||||||||||||||||||||
|
|
@@ -35,29 +35,46 @@ export interface CreateAppOptions { | |||||||||||||||||||||||||||||||||
| credentials: GitCredentialProvider; | ||||||||||||||||||||||||||||||||||
| modelResolver: ModelProviderRuntimeResolver; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Test-harness override hooks. Production never sets these — `createApp` | ||||||||||||||||||||||||||||||||||
| * builds each subsystem itself when omitted. `db` is overridden so tests | ||||||||||||||||||||||||||||||||||
| * can swap in `bun:sqlite` (better-sqlite3 isn't loadable under Bun; | ||||||||||||||||||||||||||||||||||
| * prod uses it on bundled Node). `api`, `github`, `chatRuntime`, and | ||||||||||||||||||||||||||||||||||
| * `chatService` are overridden to keep tests off the network and out of | ||||||||||||||||||||||||||||||||||
| * mastra storage. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| db?: HostDb; | ||||||||||||||||||||||||||||||||||
| api?: ApiClient; | ||||||||||||||||||||||||||||||||||
| github?: () => Promise<Octokit>; | ||||||||||||||||||||||||||||||||||
| chatRuntime?: ChatRuntimeManager; | ||||||||||||||||||||||||||||||||||
| chatService?: ChatService; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| export interface CreateAppResult { | ||||||||||||||||||||||||||||||||||
| app: Hono; | ||||||||||||||||||||||||||||||||||
| injectWebSocket: ReturnType<typeof createNodeWebSocket>["injectWebSocket"]; | ||||||||||||||||||||||||||||||||||
| api: ApiClient; | ||||||||||||||||||||||||||||||||||
| dispose: () => Promise<void>; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| export function createApp(options: CreateAppOptions): CreateAppResult { | ||||||||||||||||||||||||||||||||||
| const { config, providers } = options; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const api = createApiClient(config.cloudApiUrl, providers.auth); | ||||||||||||||||||||||||||||||||||
| const db = createDb(config.dbPath, config.migrationsFolder); | ||||||||||||||||||||||||||||||||||
| const api = | ||||||||||||||||||||||||||||||||||
| options.api ?? createApiClient(config.cloudApiUrl, providers.auth); | ||||||||||||||||||||||||||||||||||
| const db = options.db ?? createDb(config.dbPath, config.migrationsFolder); | ||||||||||||||||||||||||||||||||||
| const git = createGitFactory(providers.credentials); | ||||||||||||||||||||||||||||||||||
| const github = async () => { | ||||||||||||||||||||||||||||||||||
| const token = await providers.credentials.getToken("github.com"); | ||||||||||||||||||||||||||||||||||
| if (!token) { | ||||||||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||||||||
| "No GitHub token available. Set GITHUB_TOKEN/GH_TOKEN or authenticate via git credential manager.", | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return new Octokit({ auth: token }); | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
| const github = | ||||||||||||||||||||||||||||||||||
| options.github ?? | ||||||||||||||||||||||||||||||||||
| (async () => { | ||||||||||||||||||||||||||||||||||
| const token = await providers.credentials.getToken("github.com"); | ||||||||||||||||||||||||||||||||||
| if (!token) { | ||||||||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||||||||
| "No GitHub token available. Set GITHUB_TOKEN/GH_TOKEN or authenticate via git credential manager.", | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return new Octokit({ auth: token }); | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const pullRequestRuntime = new PullRequestRuntimeManager({ | ||||||||||||||||||||||||||||||||||
| db, | ||||||||||||||||||||||||||||||||||
|
|
@@ -66,14 +83,16 @@ export function createApp(options: CreateAppOptions): CreateAppResult { | |||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| pullRequestRuntime.start(); | ||||||||||||||||||||||||||||||||||
| const filesystem = new WorkspaceFilesystemManager({ db }); | ||||||||||||||||||||||||||||||||||
| const chatRuntime = new ChatRuntimeManager({ | ||||||||||||||||||||||||||||||||||
| db, | ||||||||||||||||||||||||||||||||||
| runtimeResolver: providers.modelResolver, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| const chatRuntime = | ||||||||||||||||||||||||||||||||||
| options.chatRuntime ?? | ||||||||||||||||||||||||||||||||||
| new ChatRuntimeManager({ | ||||||||||||||||||||||||||||||||||
| db, | ||||||||||||||||||||||||||||||||||
| runtimeResolver: providers.modelResolver, | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| // Provider auth (Anthropic / OpenAI OAuth + API keys) is per-machine, not | ||||||||||||||||||||||||||||||||||
| // per-workspace. ChatService is a long-lived singleton wrapping mastra's | ||||||||||||||||||||||||||||||||||
| // auth storage; the `host.auth.*` router proxies to it. | ||||||||||||||||||||||||||||||||||
| const chatService = new ChatService(); | ||||||||||||||||||||||||||||||||||
| const chatService = options.chatService ?? new ChatService(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const runtime = { | ||||||||||||||||||||||||||||||||||
| auth: chatService, | ||||||||||||||||||||||||||||||||||
|
|
@@ -146,5 +165,29 @@ export function createApp(options: CreateAppOptions): CreateAppResult { | |||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return { app, injectWebSocket, api }; | ||||||||||||||||||||||||||||||||||
| const ownsDb = options.db === undefined; | ||||||||||||||||||||||||||||||||||
| const dispose = async (): Promise<void> => { | ||||||||||||||||||||||||||||||||||
| // Each step is best-effort and isolated: a throw in one cleanup must | ||||||||||||||||||||||||||||||||||
| // not skip the others, otherwise a flaky `.stop()` could leak the | ||||||||||||||||||||||||||||||||||
| // open SQLite handle for the rest of the process lifetime. | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| pullRequestRuntime.stop(); | ||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||
| console.warn("[host-service] pullRequestRuntime.stop failed:", err); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| eventBus.close(); | ||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||
| console.warn("[host-service] eventBus.close failed:", err); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if (ownsDb) { | ||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||
| (db as unknown as { $client?: { close: () => void } }).$client?.close(); | ||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||
| // best-effort close; tests should not fail on teardown | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
167
to
+188
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/host-service/src/app.ts
Line: 191-201
Comment:
**`dispose` may silently skip db close on runtime stop errors**
`pullRequestRuntime.stop()` and `eventBus.close()` are called without try-catch. If either throws, the `if (ownsDb)` branch that closes the SQLite connection is never reached, leaving the file handle open. In tests the `createTestHost` wrapper still closes `sqlite` directly, but in production this means a crash in `.stop()` can leave the db handle dangling.
```suggestion
const dispose = async (): Promise<void> => {
try { pullRequestRuntime.stop(); } catch {}
try { eventBus.close(); } catch {}
if (ownsDb) {
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return { app, injectWebSocket, api, dispose }; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import type { SimpleGit } from "simple-git"; | ||
|
|
||
| /** | ||
| * Run a `git config` write with bounded retries on `.git/config.lock` | ||
| * contention. | ||
| * | ||
| * `git config` takes a per-config flock that's held for milliseconds. | ||
| * Two concurrent writes (e.g. a renderer double-click on the base-branch | ||
| * picker, or `setBaseBranch` racing with `workspaceCreation.create`'s | ||
| * own config write) cause one to fail with: | ||
| * | ||
| * error: could not lock config file .git/config: File exists | ||
| * | ||
| * We catch that specific shape and retry with a short backoff so the | ||
| * second writer just waits its turn instead of bubbling a confusing 500 | ||
| * to the renderer. | ||
| */ | ||
| export async function gitConfigWrite( | ||
| git: SimpleGit, | ||
| args: string[], | ||
| options: { retries?: number; baseDelayMs?: number } = {}, | ||
| ): Promise<string> { | ||
|
Comment on lines
+18
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The function is documented and named as a Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/host-service/src/trpc/router/git/utils/config-write.ts
Line: 18-22
Comment:
**No guard that `args` actually target `git config`**
The function is documented and named as a `git config` write helper but accepts arbitrary `args`. A caller passing `["fetch", "--all"]` would silently get lock-contention retry behaviour for an unrelated command. Adding an assertion or renaming to something generic (e.g. `gitRawWithLockRetry`) would prevent misuse. Current call sites are all correct.
How can I resolve this? If you propose a fix, please make it concise. |
||
| // `retries` is the number of *additional* attempts after the first try, | ||
| // so default 4 == 1 initial + 4 retries (5 total), with backoff | ||
| // 30/60/120/240ms between them. Clamped at 0 to keep the loop sane. | ||
| const retries = Math.max(0, options.retries ?? 4); | ||
| const baseDelayMs = options.baseDelayMs ?? 30; | ||
| let lastErr: unknown = new Error("gitConfigWrite: no attempt completed"); | ||
| for (let attempt = 0; attempt <= retries; attempt++) { | ||
| try { | ||
| return await git.raw(args); | ||
| } catch (err) { | ||
| lastErr = err; | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| if (!message.includes("could not lock config file")) throw err; | ||
| if (attempt === retries) break; | ||
| await new Promise((resolve) => | ||
| setTimeout(resolve, baseDelayMs * 2 ** attempt), | ||
| ); | ||
| } | ||
| } | ||
| throw lastErr; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$clientinternal(db as unknown as { $client?: { close: () => void } }).$client?.close()reaches into an undocumented internal of drizzle-orm. There is no public contract for$client, so a minor Drizzle upgrade could silently skip the close. Consider wrappingdbin a thin interface that exposes aclose()method at construction time, or exporting a typedHostDbthat includes the close method.Prompt To Fix With AI