Skip to content

Conversation

@mafreud
Copy link
Contributor

@mafreud mafreud commented Oct 7, 2025

Summary by CodeRabbit

  • 新機能

    • GitHub Actions ジョブ時に Hetzner 上で一時セルフホストランナーを自動作成・初期化・起動するワークフローを追加(JIT 設定取得、SSH 実行、tmux 起動含む)。ジョブ完了時の自動削除も実装。
  • 既存機能の変更

    • issues ハンドラのコメントを更新。workflow_job.queued/completed でジョブ判定、ポーリング、リトライ、実行後クリーンアップを実行。
  • チョア

    • 起動スクリプトをビルド実行する形へ変更、Octokit/SSH 関連の依存追加、Hetzner 用シークレット登録、Cloud Function のメモリ/CPU増強。
  • 設定

    • TypeScript の厳格設定を緩和、スペル辞書に語句追加、CI に実行環境マトリクス導入。

@linear
Copy link

linear bot commented Oct 7, 2025

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

Walkthrough

Hetzner Cloud と GitHub Actions ランナーの JIT プロビジョニング(作成・稼働待機・初期化)と終了処理を行う Probot ハンドラ群と Hetzner 統合モジュールを追加し、依存・start スクリプト・TS 設定・シークレット定義・スペル辞書・CI マトリクスを更新しました。

Changes

Cohort / File(s) Summary
プロジェクト設定 / 依存
openci-runner/firebase/functions/package.json
start"npm run build && probot run ./lib/probot/index.js" に変更。依存に @octokit/restnode-ssh を追加。
Hetzner 統合モジュール
openci-runner/firebase/functions/probot/hetzner.ts
新規ファイル。Hetzner API 呼び出し(createServer, deleteServer, getServerStatusById)、GitHub Actions JIT 取得(getJitConfig)、ランナー初期化スクリプト生成(initRunner)と型をエクスポート。global fetch/crypto に依存。
Probot ハンドラの拡張
openci-runner/firebase/functions/probot/index.ts
issues.openedissues.reopened へ変更。workflow_job.queued ハンドラ追加(インストール認証→isJobRequired 判定→Hetzner サーバ作成→稼働待機→SSH 接続→JIT 取得→tmux でランナー起動、リトライあり)。workflow_job.completed ハンドラでサーバ削除を実行。Octokit/node-ssh 等を導入。
ジョブ判定ユーティリティ
openci-runner/firebase/functions/probot/github.ts
新規関数 isJobRequired(context: any): boolean を追加。ラベル openci-runner-beta を持つジョブのみ処理。
Firebase 関数設定更新
openci-runner/firebase/functions/src/index.ts
HETZNER_API_KEY, HETZNER_SSH_PASSPHRASE, HETZNER_SSH_PRIVATE_KEY を defineSecret で追加し、関数の secrets に含めプロボット実行環境へ注入。メモリを 16GiB、CPU を 8 に増強。await probot(...) にして handler を return。
TypeScript 設定
openci-runner/firebase/functions/tsconfig.json
noUnusedLocalsfalse に変更、strict オプションを削除(型チェック緩和)。
ワークスペース辞書
openci.code-workspace
cSpell.words に jitconfig, RUNASROOT を追加。
CI ワークフロー
.github/workflows/reusable-deploy-firebase-functions.yml
CI マトリクスに runner を追加(ubuntu-latest, openci-runner-beta)し runs-on を matrix.runner に変更。Deploy ステップ名を更新。

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant GH as GitHub (workflow_job)
  participant Probot as Probot App
  participant HZ as Hetzner API
  participant VM as Hetzner Server (SSH)
  participant GHAPI as GitHub API (Actions)

  GH->>Probot: workflow_job.queued
  Probot->>Probot: authenticate as installation (token)
  Probot->>Probot: isJobRequired?
  alt required
    Probot->>HZ: POST /servers (createServer)
    HZ-->>Probot: { serverId, ipv4 }
    loop Poll (1s, retry limit)
      Probot->>HZ: GET /servers/{id} (status)
      HZ-->>Probot: { status }
    end
    Probot->>VM: SSH 接続(node-ssh)→ 必要パッケージ・tmux を準備
    Probot->>GHAPI: POST /repos/{owner}/{repo}/actions/runners/generate-jitconfig
    GHAPI-->>Probot: { encoded_jit_config }
    Probot->>VM: 転送 & 実行 (initRunner を tmux で起動)
    Note over VM,Probot: ランナーが tmux で起動
  else not required
    Probot-->>GH: no-op
  end
Loading
sequenceDiagram
  autonumber
  participant GH as GitHub (workflow_job)
  participant Probot as Probot App
  participant HZ as Hetzner API

  GH->>Probot: workflow_job.completed
  Probot->>Probot: isJobRequired? -> derive runnerId
  alt runnerId found
    Probot->>HZ: DELETE /servers/{runnerId}
    HZ-->>Probot: 削除確認
    Probot-->>GH: ログ(クリーンアップ完了)
  else none
    Probot-->>GH: no-op
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

ぴょんと跳ねる小さな僕🐇
雲の鍵でサーバを芽吹かせる。
sshで歌わせ、tmuxで舞わせる、
仕事終われば静かに掘り消す。
また呼べば耳を立てて駆けるよ。

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed タイトルは「Hetznerサーバー管理の実装」と「Probot連携の更新」というプルリクの主要な変更点を的確かつ簡潔にまとめており、スキャンした際に内容を明確に把握できるため要件を十分に満たしています。
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch op-272

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a059c31 and ce5975e.

📒 Files selected for processing (1)
  • openci-runner/firebase/functions/probot/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/probot/index.ts (2)
openci-runner/firebase/functions/probot/hetzner.ts (6)
  • OctokitToken (5-7)
  • createServer (23-51)
  • getServerStatusById (53-64)
  • getJitConfig (66-88)
  • initRunner (90-99)
  • deleteServer (9-16)
openci-runner/firebase/functions/probot/github.ts (1)
  • isJobRequired (4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: deploy-firebase-functions-dev / ci_and_deploy_firebase_functions (ubuntu-latest)
🔇 Additional comments (3)
openci-runner/firebase/functions/probot/index.ts (3)

27-33: 環境変数チェックが追加されました。

過去のレビューコメントで指摘された環境変数の存在チェックが適切に実装されています。


94-98: 最大リトライ失敗時の例外処理が追加されました。

過去のレビューコメントで指摘されたリトライ失敗時のエラー処理が適切に実装されています。


112-115: runnerName が null の場合の早期リターンが追加されました。

過去のレビューコメントで指摘された runnerName が null の場合の処理不備が修正されています。


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
openci-runner/firebase/functions/tsconfig.json (1)

7-7: noUnusedLocals を false にする必要がありますか?

未使用ローカルの検出が止まると、タイポや未使用変数が紛れ込んでもビルド時に検知できず、今回のような外部連携コードでは不具合の温床になります。tsconfig.json 側で明示的な警告抑止を入れるのではなく、該当箇所で不要な変数を削除するか抑制コメントを個別に付与する方向をご検討ください。

openci-runner/firebase/functions/probot/index.ts (1)

81-90: GitHub ホストランナーの場合は早期 return してください。

runnerName が null のとき(GitHub ホストランナー)でも、後続で deleteServer が呼ばれ、undefined ID で API を叩いてしまいます。ログ出力後に return して処理を打ち切ってください。

 		if (runnerName == null) {
 			console.log("This runner is GitHub hosted one");
+			return;
 		}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 492dbbf and 3905bfb.

⛔ Files ignored due to path filters (1)
  • openci-runner/firebase/functions/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • openci-runner/firebase/functions/package.json (2 hunks)
  • openci-runner/firebase/functions/probot/hetzner.ts (1 hunks)
  • openci-runner/firebase/functions/probot/index.ts (1 hunks)
  • openci-runner/firebase/functions/tsconfig.json (1 hunks)
  • openci.code-workspace (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
openci-runner/firebase/functions/probot/index.ts (1)
openci-runner/firebase/functions/probot/hetzner.ts (6)
  • OctokitToken (5-7)
  • createServer (23-51)
  • getServerStatusById (53-64)
  • getJitConfig (66-88)
  • initRunner (90-99)
  • deleteServer (9-16)
openci-runner/firebase/functions/package.json (2)
openci-runner/firebase/functions/test/index.test.ts (3)
  • probot (28-76)
  • mock (47-70)
  • nock (32-45)
openci-runner/firebase/functions/src/index.ts (1)
  • req (15-31)
🪛 GitHub Actions: Deploy GitHub Apps
openci-runner/firebase/functions/probot/hetzner.ts

[error] 1-30: Cannot find module '@octokit/rest' or its corresponding type declarations.

Comment on lines +68 to +69
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

apt install tmux に非対話オプションを付けてください。

非対話セッションで apt install tmux を実行すると、確認プロンプトで待ち続けてしまい SSH コマンドがハングします。DEBIAN_FRONTEND=noninteractive-y を付け、必要なら apt-get update も合わせて実行してください。

-				await sshResult.execCommand("apt install tmux");
+				await sshResult.execCommand(
+					"DEBIAN_FRONTEND=noninteractive apt-get update -y && DEBIAN_FRONTEND=noninteractive apt-get install -y tmux",
+				);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
// install tmux in non-interactive mode (update first, then install with -y)
await sshResult.execCommand(
"DEBIAN_FRONTEND=noninteractive apt-get update -y && DEBIAN_FRONTEND=noninteractive apt-get install -y tmux",
);
await sshResult.execCommand(initRunner(encodedJitConfig));
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/probot/index.ts around lines 68-69, the code
runs "apt install tmux" in a non-interactive SSH session which can hang on
confirmation prompts; change it to run a non-interactive, affirmative install
(use DEBIAN_FRONTEND=noninteractive and -y) and prefer apt-get, and run apt-get
update beforehand if package lists may be stale (e.g. run apt-get update &&
DEBIAN_FRONTEND=noninteractive apt-get install -y tmux) so the SSH command won't
wait for user input.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
openci-runner/firebase/functions/probot/index.ts (3)

43-55: サーバー起動待ちのタイムアウト制御が未対応です。

この無限ループの問題は以前のレビューで指摘されています。Hetznerサーバーが「running」状態にならない場合、関数が永遠に待ち続けてしまいます。タイムアウト制御の追加が必要です。


57-88: リトライ上限到達時のエラー処理が未対応です。

SSH接続とrunner初期化のリトライが最大回数に達しても例外を投げていない問題は、以前のレビューで指摘されています。失敗時にワークフローを適切に失敗させる必要があります。


78-78: apt installの非対話オプションが未対応です。

apt install tmuxに非対話オプションが必要な問題は、以前のレビューで指摘されています。SSH セッションでプロンプト待ちによるハングを防ぐため、DEBIAN_FRONTEND=noninteractive-yオプションが必要です。

🧹 Nitpick comments (1)
openci-runner/firebase/functions/probot/index.ts (1)

23-101: エラーハンドリングの追加を検討してください。

各イベントハンドラにtry-catchブロックがないため、予期しないエラーが発生した場合にログに記録されない可能性があります。特にworkflow_job.queuedとworkflow_job.completedハンドラでは、外部API呼び出しやSSH接続など、失敗する可能性のある操作が多数あります。

トップレベルでtry-catchを追加し、エラーを適切にログ出力することをお勧めします。

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3905bfb and 1f74e06.

📒 Files selected for processing (1)
  • openci-runner/firebase/functions/probot/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/probot/index.ts (1)
openci-runner/firebase/functions/probot/hetzner.ts (6)
  • OctokitToken (5-7)
  • createServer (23-51)
  • getServerStatusById (53-64)
  • getJitConfig (66-88)
  • initRunner (90-99)
  • deleteServer (9-16)
🔇 Additional comments (2)
openci-runner/firebase/functions/probot/index.ts (2)

1-12: LGTM!

インポートは適切です。node:timers/promisesからのsetTimeoutの使用は正しいです。


17-21: LGTM!

シンプルなコメント投稿ハンドラで、問題ありません。

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
openci-runner/firebase/functions/probot/index.ts (4)

40-52: サーバー起動待ちに上限時間を設けてください。

この問題は以前のレビューで指摘されています。Hetzner側が「running」にならない場合、while (true)が永遠に実行され続けます。タイムアウト制御を追加して、一定時間後にエラーを投げるようにしてください。

以下の修正を適用してください:

-		while (true) {
+		const maxStatusChecks = 60;
+		let statusCheckCount = 0;
+		let status;
+		while (statusCheckCount < maxStatusChecks) {
-			const status = await getServerStatusById(
+			status = await getServerStatusById(
 				hetznerResponse.serverId,
 				process.env.HETZNER_API_KEY,
 			);
 			if (status === "running") {
 				console.log("Server is now running!");
 				break;
 			} else {
 				console.log("Waiting for the server init. Will try again in 1 second");
 				await setTimeout(1000);
+				statusCheckCount++;
 			}
 		}
+		if (status !== "running") {
+			console.error(`Server ${hetznerResponse.serverId} failed to reach running state after ${maxStatusChecks} attempts`);
+			throw new Error(`Timed out waiting for Hetzner server ${hetznerResponse.serverId} to become running`);
+		}

54-84: 最大リトライ失敗時に例外を投げてください。

この問題は以前のレビューで指摘されています。SSH接続やrunner初期化に最大回数失敗しても、そのまま関数が終了し、ジョブが成功したように見えます。retryCount >= maxRetryの場合はエラーを投げてください。

以下の修正を適用してください:

 		while (retryCount < maxRetry) {
 			console.log("retry:", retryCount);
 			try {
 				// ... existing code ...
 				console.log("Successfully start openci runner");
 				break;
 			} catch (e) {
 				console.log("error, will try again in 1 second", e);
 				await setTimeout(1000);
 				retryCount++;
 			}
 		}
+		if (retryCount >= maxRetry) {
+			console.error(`Failed to establish SSH connection and bootstrap runner after ${maxRetry} attempts for server ${hetznerResponse.serverId}`);
+			throw new Error(`Failed to establish SSH connection and bootstrap runner for server ${hetznerResponse.serverId}`);
+		}
 	});

75-76: apt install tmuxに非対話オプションを付けてください。

この問題は以前のレビューで指摘されています。非対話セッションでapt install tmuxを実行すると、確認プロンプトで待ち続けてSSHコマンドがハングします。DEBIAN_FRONTEND=noninteractive-yを付けてください。

以下の修正を適用してください:

-				await sshResult.execCommand("apt install tmux");
+				await sshResult.execCommand(
+					"DEBIAN_FRONTEND=noninteractive apt-get update -y && DEBIAN_FRONTEND=noninteractive apt-get install -y tmux",
+				);

93-102: runnerNameがnullの場合は早期returnしてください。

この問題は以前のレビューで指摘されています。Line 94-96でrunnerNameがnullの場合をログ出力していますが、その後もLine 98でrunnerIdを抽出し、Line 101でdeleteServerを呼び出しています。runnerNameがnullの場合、runnerIdもundefinedになり、deleteServerにundefinedが渡されてエラーになります。

以下の修正を適用してください:

 		const runnerName = context.payload.workflow_job.runner_name;
 		if (runnerName == null) {
 			console.log("This runner is GitHub hosted one");
+			return;
 		}
 		const defaultRunnerName = "OpenCI ランナー ";
 		const runnerId = runnerName?.replace(defaultRunnerName, "");
 		console.log("runner id", runnerId);
 
+		if (!runnerId) {
+			console.log("Failed to extract runner ID from runner name");
+			return;
+		}
+		
 		await deleteServer(runnerId, process.env.HETZNER_API_KEY);
 		console.log("Finish cleaning up");
 	});
🧹 Nitpick comments (2)
openci-runner/firebase/functions/probot/index.ts (2)

24-38: 環境変数の存在確認を追加してください。

初期セットアップは適切ですが、process.env.HETZNER_API_KEYなどの環境変数が未定義の場合の処理がありません。後続の処理で使用される前に、必要な環境変数の存在を確認してください。

以下の修正を適用してください:

 	app.on("workflow_job.queued", async (context) => {
 		console.log("workflow_job.queued");
+		
+		// 環境変数の検証
+		if (!process.env.HETZNER_API_KEY || !process.env.HETZNER_SSH_PRIVATE_KEY || !process.env.HETZNER_SSH_PASSPHRASE) {
+			console.error("Missing required environment variables");
+			throw new Error("HETZNER_API_KEY, HETZNER_SSH_PRIVATE_KEY, and HETZNER_SSH_PASSPHRASE must be set");
+		}
+		
 		const { token } = (await context.octokit.auth({
 			type: "installation",
 		})) as OctokitToken;

53-84: SSH接続のクリーンアップを追加してください。

SSH接続が確立された後、明示的にクローズされていません。接続が成功した場合でも失敗した場合でも、ssh.dispose()を呼び出してリソースをクリーンアップしてください。

以下の修正を適用してください:

 		const ssh = new NodeSSH();
 		const maxRetry = 10;
 		let retryCount = 0;
+		let connected = false;
 		while (retryCount < maxRetry) {
 			console.log("retry:", retryCount);
 			try {
 				const sshResult = await ssh.connect({
 					host: hetznerResponse.ipv4,
 					username: "root",
 					privateKey: process.env.HETZNER_SSH_PRIVATE_KEY,
 					passphrase: process.env.HETZNER_SSH_PASSPHRASE,
 				});
+				connected = true;
 
 				const octokit = new Octokit({
 					auth: token,
 				});
 				const encodedJitConfig = await getJitConfig(
 					octokit,
 					owner,
 					repo,
 					hetznerResponse.serverId,
 				);
 				await sshResult.execCommand("apt install tmux");
 				await sshResult.execCommand(initRunner(encodedJitConfig));
 				console.log("Successfully start openci runner");
 				break;
 			} catch (e) {
 				console.log("error, will try again in 1 second", e);
 				await setTimeout(1000);
 				retryCount++;
+			} finally {
+				if (connected) {
+					ssh.dispose();
+				}
 			}
 		}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5245bdb and e84d796.

📒 Files selected for processing (1)
  • openci-runner/firebase/functions/probot/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/probot/index.ts (2)
openci-runner/firebase/functions/probot/hetzner.ts (6)
  • OctokitToken (5-7)
  • createServer (23-51)
  • getServerStatusById (53-64)
  • getJitConfig (66-88)
  • initRunner (90-99)
  • deleteServer (9-16)
openci-runner/firebase/functions/probot/github.ts (1)
  • isJobRequired (4-6)
🔇 Additional comments (2)
openci-runner/firebase/functions/probot/index.ts (2)

1-13: インポートは適切です。

必要なモジュールが正しくインポートされています。Octokit、NodeSSH、Hetzner関連のユーティリティ関数が適切に読み込まれています。


18-22: LGTM!

イベントハンドラーがissues.reopenedに変更され、コメント本文も更新されています。テスト用途として適切な実装です。

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4844dab and 7c49e1a.

📒 Files selected for processing (1)
  • openci-runner/firebase/functions/src/index.ts (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: deploy-firebase-functions-dev / ci_and_deploy_firebase_functions
🔇 Additional comments (1)
openci-runner/firebase/functions/src/index.ts (1)

25-29: LGTM! (15 行目の修正が前提)

Hetzner 認証情報を環境変数として Probot に渡す実装は適切です。ただし、15 行目の secrets 配列に Hetzner シークレットを追加する必要があります(別のコメントを参照)。

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (5)
openci-runner/firebase/functions/probot/index.ts (5)

39-39: サーバー作成失敗時のエラーハンドリングが不足しています。

過去のレビューで指摘されたcreateServerのエラーハンドリングがまだ追加されていません。Hetzner APIの呼び出しが失敗した場合、適切にエラーを投げてワークフローを失敗させる必要があります。


40-52: サーバー起動待ちに上限時間を設定してください。

過去のレビューで指摘されたタイムアウト制御がまだ実装されていません。while(true)が無限ループになる可能性があり、Hetznerサーバーが「running」ステータスにならない場合、処理が永遠に停止します。一定回数または時間で打ち切る制御を追加してください。


54-84: 最大リトライ失敗時に例外を投げてください。

過去のレビューで指摘されたように、SSH接続やランナー初期化に最大回数失敗しても、そのまま関数が終了してしまいます。retryCount >= maxRetryの場合はエラーを投げてワークフローを失敗させ、運用者が検知できるようにしてください。


75-75: apt install tmuxに非対話オプションを付けてください。

過去のレビューで指摘されたように、非対話セッションでapt install tmuxを実行すると、確認プロンプトで待ち続けてSSHコマンドがハングします。DEBIAN_FRONTEND=noninteractive-yオプションを付けてください。


94-104: runnerNameがnullの場合の処理に不備があります。

過去のレビューで指摘されたように、Line 95-97でrunnerNamenullの場合をログ出力していますが、その後も処理が継続し、Line 102でdeleteServerundefinedが渡されてエラーになります。

runnerNamenullの場合は早期returnしてください。

以下の修正を適用してください:

 		const runnerName = context.payload.workflow_job.runner_name;
 		if (runnerName == null) {
 			console.log("This runner is GitHub hosted one");
+			return;
 		}
 		const defaultRunnerName = "OpenCI ランナー ";
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1b05ea2 and 15794c8.

📒 Files selected for processing (1)
  • openci-runner/firebase/functions/probot/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/probot/index.ts (2)
openci-runner/firebase/functions/probot/hetzner.ts (6)
  • OctokitToken (5-7)
  • createServer (23-51)
  • getServerStatusById (53-64)
  • getJitConfig (66-88)
  • initRunner (90-99)
  • deleteServer (9-16)
openci-runner/firebase/functions/probot/github.ts (1)
  • isJobRequired (4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: deploy-firebase-functions-dev / ci_and_deploy_firebase_functions

Comment on lines +39 to +84
const hetznerResponse = await createServer(process.env.HETZNER_API_KEY);
while (true) {
const status = await getServerStatusById(
hetznerResponse.serverId,
process.env.HETZNER_API_KEY,
);
if (status === "running") {
console.log("Server is now running!");
break;
} else {
console.log("Waiting for the server init. Will try again in 1 second");
await setTimeout(1000);
}
}
const ssh = new NodeSSH();
const maxRetry = 10;
let retryCount = 0;
while (retryCount < maxRetry) {
console.log("retry:", retryCount);
try {
const sshResult = await ssh.connect({
host: hetznerResponse.ipv4,
username: "root",
privateKey: process.env.HETZNER_SSH_PRIVATE_KEY,
passphrase: process.env.HETZNER_SSH_PASSPHRASE,
});

const octokit = new Octokit({
auth: token,
});
const encodedJitConfig = await getJitConfig(
octokit,
owner,
repo,
hetznerResponse.serverId,
);
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
console.log("Successfully start openci runner");
break;
} catch (e) {
console.log("error, will try again in 1 second", e);
await setTimeout(1000);
retryCount++;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

初期化失敗時にHetznerサーバーが削除されません。

サーバー作成後、SSH接続やランナー初期化に失敗した場合、作成されたHetznerサーバーが残り続けます。これはリソースリークとコスト増加につながります。

以下の修正を適用してください:

+	let hetznerResponse;
+	try {
-		const hetznerResponse = await createServer(process.env.HETZNER_API_KEY);
+		hetznerResponse = await createServer(process.env.HETZNER_API_KEY);
 		while (true) {
 			// ... ステータスチェック
 		}
 		const ssh = new NodeSSH();
 		// ... SSH接続とランナー初期化
+	} catch (error) {
+		console.error("Failed to initialize runner, cleaning up server", error);
+		if (hetznerResponse?.serverId) {
+			await deleteServer(String(hetznerResponse.serverId), process.env.HETZNER_API_KEY);
+		}
+		throw error;
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const hetznerResponse = await createServer(process.env.HETZNER_API_KEY);
while (true) {
const status = await getServerStatusById(
hetznerResponse.serverId,
process.env.HETZNER_API_KEY,
);
if (status === "running") {
console.log("Server is now running!");
break;
} else {
console.log("Waiting for the server init. Will try again in 1 second");
await setTimeout(1000);
}
}
const ssh = new NodeSSH();
const maxRetry = 10;
let retryCount = 0;
while (retryCount < maxRetry) {
console.log("retry:", retryCount);
try {
const sshResult = await ssh.connect({
host: hetznerResponse.ipv4,
username: "root",
privateKey: process.env.HETZNER_SSH_PRIVATE_KEY,
passphrase: process.env.HETZNER_SSH_PASSPHRASE,
});
const octokit = new Octokit({
auth: token,
});
const encodedJitConfig = await getJitConfig(
octokit,
owner,
repo,
hetznerResponse.serverId,
);
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
console.log("Successfully start openci runner");
break;
} catch (e) {
console.log("error, will try again in 1 second", e);
await setTimeout(1000);
retryCount++;
}
}
let hetznerResponse;
try {
hetznerResponse = await createServer(process.env.HETZNER_API_KEY);
while (true) {
const status = await getServerStatusById(
hetznerResponse.serverId,
process.env.HETZNER_API_KEY,
);
if (status === "running") {
console.log("Server is now running!");
break;
} else {
console.log("Waiting for the server init. Will try again in 1 second");
await setTimeout(1000);
}
}
const ssh = new NodeSSH();
const maxRetry = 10;
let retryCount = 0;
while (retryCount < maxRetry) {
console.log("retry:", retryCount);
try {
const sshResult = await ssh.connect({
host: hetznerResponse.ipv4,
username: "root",
privateKey: process.env.HETZNER_SSH_PRIVATE_KEY,
passphrase: process.env.HETZNER_SSH_PASSPHRASE,
});
const octokit = new Octokit({
auth: token,
});
const encodedJitConfig = await getJitConfig(
octokit,
owner,
repo,
hetznerResponse.serverId,
);
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
console.log("Successfully start openci runner");
break;
} catch (e) {
console.log("error, will try again in 1 second", e);
await setTimeout(1000);
retryCount++;
}
}
} catch (error) {
console.error("Failed to initialize runner, cleaning up server", error);
if (hetznerResponse?.serverId) {
await deleteServer(
String(hetznerResponse.serverId),
process.env.HETZNER_API_KEY,
);
}
throw error;
}

Comment on lines +53 to +84
const ssh = new NodeSSH();
const maxRetry = 10;
let retryCount = 0;
while (retryCount < maxRetry) {
console.log("retry:", retryCount);
try {
const sshResult = await ssh.connect({
host: hetznerResponse.ipv4,
username: "root",
privateKey: process.env.HETZNER_SSH_PRIVATE_KEY,
passphrase: process.env.HETZNER_SSH_PASSPHRASE,
});

const octokit = new Octokit({
auth: token,
});
const encodedJitConfig = await getJitConfig(
octokit,
owner,
repo,
hetznerResponse.serverId,
);
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
console.log("Successfully start openci runner");
break;
} catch (e) {
console.log("error, will try again in 1 second", e);
await setTimeout(1000);
retryCount++;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

SSH接続のクリーンアップ処理が不足しています。

SSH接続成功後、ssh.dispose()などのクリーンアップ処理が行われていません。リソースリークを防ぐため、処理完了後またはエラー時に接続を適切に閉じてください。

以下のように修正してください:

 		const ssh = new NodeSSH();
 		const maxRetry = 10;
 		let retryCount = 0;
+		let connected = false;
 		while (retryCount < maxRetry) {
 			console.log("retry:", retryCount);
 			try {
 				const sshResult = await ssh.connect({
 					// ... 接続設定
 				});
+				connected = true;
 
 				// ... ランナー初期化
 				console.log("Successfully start openci runner");
+				ssh.dispose();
 				break;
 			} catch (e) {
 				console.log("error, will try again in 1 second", e);
 				await setTimeout(1000);
 				retryCount++;
 			}
 		}
+		if (connected) {
+			ssh.dispose();
+		}
 		return;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const ssh = new NodeSSH();
const maxRetry = 10;
let retryCount = 0;
while (retryCount < maxRetry) {
console.log("retry:", retryCount);
try {
const sshResult = await ssh.connect({
host: hetznerResponse.ipv4,
username: "root",
privateKey: process.env.HETZNER_SSH_PRIVATE_KEY,
passphrase: process.env.HETZNER_SSH_PASSPHRASE,
});
const octokit = new Octokit({
auth: token,
});
const encodedJitConfig = await getJitConfig(
octokit,
owner,
repo,
hetznerResponse.serverId,
);
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
console.log("Successfully start openci runner");
break;
} catch (e) {
console.log("error, will try again in 1 second", e);
await setTimeout(1000);
retryCount++;
}
}
const ssh = new NodeSSH();
const maxRetry = 10;
let retryCount = 0;
let connected = false;
while (retryCount < maxRetry) {
console.log("retry:", retryCount);
try {
const sshResult = await ssh.connect({
host: hetznerResponse.ipv4,
username: "root",
privateKey: process.env.HETZNER_SSH_PRIVATE_KEY,
passphrase: process.env.HETZNER_SSH_PASSPHRASE,
});
connected = true;
const octokit = new Octokit({
auth: token,
});
const encodedJitConfig = await getJitConfig(
octokit,
owner,
repo,
hetznerResponse.serverId,
);
await sshResult.execCommand("apt install tmux");
await sshResult.execCommand(initRunner(encodedJitConfig));
console.log("Successfully start openci runner");
ssh.dispose();
break;
} catch (e) {
console.log("error, will try again in 1 second", e);
await setTimeout(1000);
retryCount++;
}
}
if (connected) {
ssh.dispose();
}
return;
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/probot/index.ts around lines 53 to 84, the
SSH connection created with new NodeSSH() is never disposed, risking resource
leaks; wrap the connect/command sequence in a try/finally (or add a nested
try/finally) so that after a successful connect and after running commands you
call await ssh.dispose(), and also ensure you call await ssh.dispose() in the
catch path before retrying (guarding the dispose call so it only runs if a
connection was established). Ensure dispose is awaited and that you still break
out of the loop after successful execution.

const runnerId = runnerName?.replace(defaultRunnerName, "");
console.log("runner id", runnerId);

await deleteServer(runnerId, process.env.HETZNER_API_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

deleteServerのエラーハンドリングを追加してください。

deleteServerの呼び出しにエラーハンドリングがありません。Hetzner APIの削除処理が失敗した場合でも、エラーをログに記録して適切に処理する必要があります。

以下の修正を適用してください:

-		await deleteServer(runnerId, process.env.HETZNER_API_KEY);
-		console.log("Finish cleaning up");
+		try {
+			await deleteServer(runnerId, process.env.HETZNER_API_KEY);
+			console.log("Finish cleaning up");
+		} catch (error) {
+			console.error("Failed to delete Hetzner server:", error);
+			// サーバー削除失敗は致命的ではないため、エラーを投げずにログのみ記録
+		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await deleteServer(runnerId, process.env.HETZNER_API_KEY);
app.on("workflow_job.completed", async (context) => {
const runnerName = context.payload.workflow_job.runner_name;
if (runnerName == null) {
console.log("This runner is GitHub hosted one");
return;
}
const defaultRunnerName = "OpenCI ランナー ";
const runnerId = runnerName?.replace(defaultRunnerName, "");
console.log("runner id", runnerId);
if (!runnerId) {
console.log("Failed to extract runner ID from runner name");
return;
}
- await deleteServer(runnerId, process.env.HETZNER_API_KEY);
try {
await deleteServer(runnerId, process.env.HETZNER_API_KEY);
console.log("Finish cleaning up");
} catch (error) {
console.error("Failed to delete Hetzner server:", error);
// サーバー削除失敗は致命的ではないため、エラーを投げずにログのみ記録
}
});
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/probot/index.ts around line 102, the call to
await deleteServer(runnerId, process.env.HETZNER_API_KEY) lacks error handling;
wrap the await in a try/catch, log the caught error with clear context (include
runnerId and that Hetzner delete failed) using the existing logger (or
console/error logger if none available), and then handle the failure
appropriately (either rethrow the error or return a handled response/status so
upstream code doesn't assume deletion succeeded).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants