Skip to content

fix(server): gracefully shut down API server - #476

Open
yash27git wants to merge 4 commits into
vxcontrol:mainfrom
yash27git:http/Shutdown
Open

fix(server): gracefully shut down API server#476
yash27git wants to merge 4 commits into
vxcontrol:mainfrom
yash27git:http/Shutdown

Conversation

@yash27git

@yash27git yash27git commented Oct 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Description of the Change

Problem

The API HTTP server was started in a goroutine but was not explicitly shut down before telemetry and shared database resources were cleaned up. An unexpected server error used a fatal exit path that could skip deferred cleanup.

Solution

  • Keep the HTTP server available to the shutdown path and stop it with a fresh, bounded 15-second context.
  • Force-close remaining connections if graceful shutdown fails or times out, and report shutdown errors.
  • Wait for the serve goroutine so a real server error racing with a termination signal is not lost.
  • Drain telemetry on a fresh 5-second context after HTTP shutdown.
  • Return an exit code so deferred cleanup can run before the process exits.
  • Add tests for in-flight requests, timeout/forced close, unexpected server errors, telemetry ordering, and non-zero exit behavior.

Closes #480

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • 🧪 Test update

Areas Affected

  • Core Services (Frontend UI/Backend API)
  • Monitoring Stack (Grafana/OpenTelemetry)

Testing and Verification

Test Configuration

PentAGI Version: PR head 46791a3647030e749ffd15ecf81a833669bee361
Docker Version: Not recorded
Host OS: Not yet verified locally
LLM Provider: Not applicable to the shutdown tests
Enabled Features: HTTP server shutdown and telemetry-drain paths

Test Steps

  1. Run go test -race ./cmd/pentagi from the backend directory.
  2. Run go vet ./cmd/pentagi from the backend directory.
  3. Review the PR CI checks after GitHub Actions completes.

Test Results

The production source was restored to backend/cmd/pentagi/main.go; test code remains in backend/cmd/pentagi/main_test.go. Tests and vet have not been run from this chat environment, so no passing result is claimed. CI status for the repaired commit is pending/needs verification.

Security Considerations

No authentication, authorization, or network exposure changes are intended. Shutdown uses bounded contexts to avoid hanging indefinitely.

Performance Impact

No steady-state request-path changes are intended. Shutdown can wait up to 15 seconds for active HTTP requests before forcing close.

Documentation Updates

No user-facing API or configuration changes are intended.

Deployment Notes

No new environment variables, migrations, or deployment steps are required.

Checklist

Code Quality

  • My code follows the project's coding standards
  • I have added/updated necessary documentation
  • I have added tests to cover my changes
  • All new and existing tests pass
  • I have run go fmt and go vet (for Go code)
  • I have run pnpm run lint (for TypeScript/JavaScript code)

Security

  • I have considered security implications
  • Changes maintain or improve the security model
  • Sensitive information has been properly handled

Compatibility

  • Changes are backward compatible
  • Breaking changes are clearly marked and documented
  • Dependencies are properly updated

Documentation

  • Documentation is clear and complete
  • Comments are added for non-obvious code
  • API changes are documented

Additional Notes

The source/test separation was repaired in commit 46791a3647030e749ffd15ecf81a833669bee361. Please review the resulting diff and CI before merging.

@yash27git
fix(server): shut down HTTP server gracefully and keep exit status on…
f6b7b74 
… failure

The server goroutine was never shut down explicitly, and an unexpected
server error called logrus.Fatalf, which exits immediately and skips
deferred cleanup.

- Keep the http.Server reachable and call Shutdown on a fresh 15s
  context (the signal context is already cancelled). If it fails or
  times out, log it and force-close with Close, reporting any close error.
- Treat http.ErrServerClosed as expected only after a requested
  shutdown; any other server exit is a failure.
- Shut down HTTP before draining telemetry (fresh 5s context) so
  requests finishing during shutdown can still emit it.
- Replace Fatalf with a returned exit code: main now calls
  os.Exit(runApplication()), so deferred cleanup runs before a
  non-zero exit.
- Close the shared sql.DB pool via defer, after the HTTP server stops.
- Cancel the signal context after shutdown so context-bound background
  work stops before the pools close.

Shutdown does not stop running flows or background services, and does
not wait for hijacked (WebSocket) connections.

Signed-off-by: Yaash Singhh <YASHVSINGH27+git@GMAIL.COM>
@yash27git
test(server): deadline expiry and failure exit status
ac51393 
- Active request completes before graceful shutdown returns, and Close
  is not called.
- Deadline expiry is reported (context.DeadlineExceeded), Close is
  called once, and the stuck connection is cut.
- A forced-close error is reported alongside the shutdown error.
- http.ErrServerClosed after a requested shutdown is not an error; an
  unrequested one is. A server error racing the signal is not lost.
- Unexpected server failure shuts the server down, drains telemetry,
  runs deferred cleanup in that order, and returns exit code 1.
- A real listen failure (address in use) returns 1 and still drains.
- A re-executed child process confirms the real exit status is 1 and
  deferred cleanup ran before exit.

No sleeps, external network, PostgreSQL, Docker or LLM providers.

Signed-off-by: Yaash Singhh <YASHVSINGH27+git@GMAIL.COM>
@yash27git
fix(server): shut down HTTP server gracefully and keep exit status on…
cda783d 
… failure

The server goroutine was never shut down explicitly, and an unexpected
server error called logrus.Fatalf, which exits immediately and skips
deferred cleanup.

- Keep the http.Server reachable and call Shutdown on a fresh 15s
  context (the signal context is already cancelled). If it fails or
  times out, log it and force-close with Close, reporting any close error.
- Treat http.ErrServerClosed as expected only after a requested
  shutdown; any other server exit is a failure.
- Shut down HTTP before draining telemetry (fresh 5s context) so
  requests finishing during shutdown can still emit it.
- Replace Fatalf with a returned exit code: main now calls
  os.Exit(runApplication()), so deferred cleanup runs before a
  non-zero exit.
- Close the shared sql.DB pool via defer, after the HTTP server stops.
- Cancel the signal context after shutdown so context-bound background
  work stops before the pools close.

Shutdown does not stop running flows or background services, and does
not wait for hijacked (WebSocket) connections.

Signed-off-by: Yaash Singhh <YASHVSINGH27+git@GMAIL.COM>
@yash27git yash27git mentioned this pull request Oct 27, 2026
Open
@yash27git
fix(server): restore HTTP shutdown implementation
3fcf33f 
Signed-off-by: Yaash Singhh <YASHVSINGH27+git@GMAIL.COM>
@yash27git

yash27git commented Oct 30, 2026

Copy link
Copy Markdown
Author

I found and fixed a source/test separation mistake: backend/cmd/pentagi/main.go had been replaced with test code, so the production entry point was missing. Commit 46791a3647030e749ffd15ecf81a833669bee361 restores the production implementation and keeps tests in main_test.go. I also linked issue #480 and updated the PR description. I have not been able to run Go tests/vet from this chat environment, so I am not claiming they passed.

Could a maintainer please help with the remaining merge blockers?

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: API server does not gracefully shut down on termination

1 participant

@yash27git