api: polish api service shutdown and top-level context handling

[tychoish]

This makes shutdown, in response to SIGTERM, cleaner and more correct:

• avoided creating a new context in several key places, notably, the base context handed to requests.
• the context canceled when the signal is caught (SIGTERM).
• the process will not exit until all request handlers have returned and any shutdown operations.

As an added bonus, I made some changes to error handling and logging which should remove the case of "process exited without logging anything useful".

There are still some aspects which could require more attention in a follow up PR:

• I haven't looked at all the places where we either create a background context (should only really be done in tests (maybe) and in main(), but this requires more churn and should happen in a later PR.
• I haven't audited all the places where we create go routines to make sure that we give them reasonable contexts and wait for them during shut down, if needed.

The changes for shutdown handling are pretty generic (though not implemented here. If needed later, we can either use stuff from one of my repos (if suitable) or factor the generic parts out to be used in other processes.

✨

Description by Callstackai

This PR improves the API service shutdown handling and top-level context management. It ensures that the shutdown process is cleaner and more correct, avoiding unnecessary context creation and ensuring all request handlers complete before exiting. Additionally, it enhances error handling and logging.

Diagrams of code changes

sequenceDiagram
    participant Main
    participant APIStore
    participant Server
    participant Cleanup

    Main->>Main: Create root context
    Main->>Main: Setup signal handling (SIGTERM)
    Main->>APIStore: NewAPIStore(ctx)
    Main->>Server: NewGinServer(signalCtx, apiStore, swagger, port)
    
    par HTTP Server
        Server->>Server: ListenAndServe()
    and Signal Handling
        Main->>Main: Wait for shutdown signal
    end

    alt Signal Received
        Main->>Server: Shutdown()
        Main->>Cleanup: Execute cleanup operations
        Cleanup->>APIStore: Close()
        Note over APIStore: Close all clients:<br/>- Analytics<br/>- Posthog<br/>- Orchestrator<br/>- TemplateManager<br/>- Database
    end

    Main->>Main: Exit with status code

Loading

Files Changed

File	Summary
packages/api/internal/handlers/store.go	Refactored the APIStore initialization to accept a context and improved error handling in the Close method.
packages/api/main.go	Updated the main function to handle shutdown signals more effectively and improved context management for the server.
packages/shared/pkg/db/client.go	Modified the Close method to return an error instead of ignoring it.
packages/shared/pkg/telemetry/otel.go	Updated the InitOTLPExporter function to accept a context and return a function that also accepts a context for cleanup.

This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions .go. See list of supported languages.

[jakubno]

Hey @tychoish, I saw you started adding some changes in orchestrator and template-manager. Are those relevant to API part? Ideally we would have several smaller PRs rather than one PR.

[tychoish]

@jakubno definitely can split it up: the change to the API wanted a changed to a shared package. There's no way to change the shared code and the calling code (in the API and orchestrator packages,) in different commits, but I can pull the shared change from the larger API changes. Regardless the orchestrator change is pretty small.

[jakubno]

No problem with that, just making sure it has limited scope

[tychoish]

this PR, is a superset of #248 (which can be merged at any time.)

Getting #248 out of the way will make this easier to review, but it's testable in its current form, will set this PR to draft while I test, but there's no further work to do here.

[tychoish]

Status:

• With shared(api): context infrastructure plumbing for shared packages #248 merged, this PR is more minimally scoped, and ready to review/merge
• would like to do one final round of of validation/smoke test before merging

[tychoish]

Update: I've thoroughly tested this and am confident that it's safe to merge and deploy.

[tychoish]

Based on:

• https://developer.hashicorp.com/nomad/tutorials/job-updates/job-update-handle-signals
• https://developer.hashicorp.com/nomad/tutorials/job-updates/job-rolling-update

It seems like we should make this handle SIGINT in addition to SIGTERM.

SIGTERM is slightly more common for this in other non-nomad tools, so I think handling both as "graceful shutdown triggers" is correct.