Skip to content

Refactor api-proxy SIGTERM/SIGINT shutdown flow into a shared handler#3209

Merged
lpcox merged 3 commits into
mainfrom
copilot/fix-duplicate-signal-handlers
May 15, 2026
Merged

Refactor api-proxy SIGTERM/SIGINT shutdown flow into a shared handler#3209
lpcox merged 3 commits into
mainfrom
copilot/fix-duplicate-signal-handlers

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 15, 2026

Bug Fix

What was the bug?

containers/api-proxy/server.js had duplicated graceful-shutdown logic in both SIGTERM and SIGINT handlers (same OIDC shutdown + log stream close + exit path). This created avoidable maintenance risk in a security-critical proxy because future shutdown changes had to be kept in sync manually.

How did you fix it?

  • Shared shutdown path
    • Extracted the duplicated logic into shutdownGracefully(signal), including provider shutdown, closeLogStream(), and process.exit(0).
  • Signal-specific registration only
    • Updated SIGTERM and SIGINT handlers to delegate to the shared function, preserving signal-specific log messages via the signal parameter.
  • Async handler parity
    • Kept signal callbacks async wrappers so shutdown remains an awaited async path from the signal entrypoint.
async function shutdownGracefully(signal) {
  logRequest('info', 'shutdown', { message: `Received ${signal}, shutting down gracefully` });
  for (const adapter of registeredAdapters) {
    if (typeof adapter.getOidcProvider === 'function') adapter.getOidcProvider()?.shutdown();
    if (typeof adapter.getAwsOidcProvider === 'function') adapter.getAwsOidcProvider()?.shutdown();
  }
  await closeLogStream();
  process.exit(0);
}

process.on('SIGTERM', async () => shutdownGracefully('SIGTERM'));
process.on('SIGINT', async () => shutdownGracefully('SIGINT'));

Testing

Behavior is unchanged by design; this change centralizes existing shutdown steps into a single implementation to prevent divergence.

Copilot AI changed the title [WIP] Refactor duplicate signal handlers in api-proxy server.js Refactor api-proxy SIGTERM/SIGINT shutdown flow into a shared handler May 15, 2026
Copilot finished work on behalf of lpcox May 15, 2026 13:42
Copilot AI requested a review from lpcox May 15, 2026 13:42
@lpcox lpcox marked this pull request as ready for review May 15, 2026 13:53
@lpcox lpcox requested a review from Mossaka as a code owner May 15, 2026 13:53
Copilot AI review requested due to automatic review settings May 15, 2026 13:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR centralizes the API proxy’s signal shutdown behavior so SIGTERM and SIGINT share the same graceful shutdown path.

Changes:

  • Extracts duplicated shutdown logic into shutdownGracefully(signal).
  • Updates SIGTERM and SIGINT handlers to delegate to the shared helper while preserving signal-specific logging.
Show a summary per file
File Description
containers/api-proxy/server.js Refactors duplicated signal shutdown handlers into a shared helper for OIDC provider shutdown, log stream closing, and process exit.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test: Copilot BYOK (Offline) Mode

Test Result
GitHub MCP connectivity ❌ 401 Bad credentials
GitHub.com HTTP connectivity ⚠️ Pre-step data not injected (template vars unresolved)
File write/read ❌ File not found at expected path
BYOK inference (api-proxy → api.githubcopilot.com) ✅ Responding via BYOK offline mode

Note: Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com

Overall: FAIL — Pre-step smoke data was not injected (template variables ${{ steps.smoke-data.outputs.* }} unresolved), GitHub MCP returned 401, and smoke file was absent.

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results

  • ❌ GitHub API: HTTP 401 (gh auth unavailable in sandbox)
  • ✅ Playwright: github.com navigation successful, title verified
  • ✅ File verify: smoke-test file readable

Result: 2/3 PASS (1 environment limitation, 2 core functions working)

💥 [THE END] — Illustrated by Smoke Claude

@github-actions
Copy link
Copy Markdown
Contributor

🤖 Smoke Test Results

Test Status
GitHub MCP connectivity ❌ (401 Unauthorized)
GitHub.com HTTP connectivity ❓ (template vars not resolved)
File write/read (smoke-test-copilot-25921035449.txt)

Overall: FAIL — GitHub MCP returned 401 credentials error; workflow template variables were not expanded for HTTP/PR data tests.

📰 BREAKING: Report filed by Smoke Copilot

@github-actions
Copy link
Copy Markdown
Contributor

PR titles: unavailable (safeinputs-gh missing; gh returned 401)
❌ GitHub PR review/query
✅ Playwright title check
❌ Tavily search / discussion query
✅ File write/read and AWF build
Overall status: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions
Copy link
Copy Markdown
Contributor

Chroot Version Comparison Results

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3
Node.js v24.14.1 v20.20.2
Go go1.22.12 go1.22.12

Overall: ❌ Not all versions match — Python and Node.js differ between host and chroot environments.

Tested by Smoke Chroot

@github-actions
Copy link
Copy Markdown
Contributor

Smoke test results: FAIL (GitHub MCP and Connectivity issues)

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • localhost

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "localhost"

See Network Configuration for more information.

💎 Faceted by Smoke Gemini

@github-actions
Copy link
Copy Markdown
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx All passed ✅ PASS
Node.js execa All passed ✅ PASS
Node.js p-limit All passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #3209 · ● 4.9M ·

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results

  • ❌ Redis (PING): timeout — no response from host.docker.internal:6379
  • ❌ PostgreSQL pg_isready: no response from host.docker.internal:5432
  • ❌ PostgreSQL SELECT 1: timeout

Overall: FAILhost.docker.internal is unreachable from this runner environment. Service containers may not be running or the host alias is not resolvable.

🔌 Service connectivity validated by Smoke Services

@github-actions github-actions Bot mentioned this pull request May 15, 2026
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 15, 2026

@copilot address review feedback

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 15, 2026

@copilot address review feedback

Addressed in commit 2aa01fb: signal handlers now use async wrappers (SIGTERM/SIGINT) while delegating to the shared shutdownGracefully(signal) path.

Copilot finished work on behalf of lpcox May 15, 2026 14:35
@lpcox lpcox merged commit e9e5e92 into main May 15, 2026
70 of 82 checks passed
@lpcox lpcox deleted the copilot/fix-duplicate-signal-handlers branch May 15, 2026 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Duplicate Code] SIGTERM and SIGINT graceful-shutdown handlers share identical body in api-proxy server.js

3 participants