Skip to content

Add graceful shutdown with readiness drain and chart termination grace period#90

Merged
0lut merged 11 commits into
mainfrom
devin/1781167195-graceful-shutdown
Jun 11, 2026
Merged

Add graceful shutdown with readiness drain and chart termination grace period#90
0lut merged 11 commits into
mainfrom
devin/1781167195-graceful-shutdown

Conversation

@0lut

@0lut 0lut commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Summary

The API server previously died instantly on SIGTERM, cutting in-flight clones/upload-pack streams on every rollout or pod eviction. This adds a three-phase graceful shutdown plus matching chart settings.

App (git-cache-api): the shutdown orchestration lives in git_cache_api::serve() (which main now uses after the repo-access-flush refactor on main; the flush still runs after the drain):

  1. On SIGTERM/SIGINT, a ReadinessGate flips and /healthz starts returning 503 {ok:false}, so Kubernetes/LBs pull the pod out of rotation while it still accepts traffic for shutdown.readiness_delay_seconds (default 5s — covers the endpoint-propagation race, replacing the usual preStop sleep hack).
  2. The server then stops accepting connections via with_graceful_shutdown and drains in-flight requests.
  3. The drain is bounded: after shutdown.drain_timeout_seconds (default 60s) the process exits anyway (tokio::select! against the serve future); lingering git children are reaped by the existing kill_on_drop(true) contract.

New config (env or TOML, both defaulted so existing deployments are unaffected):

[shutdown]
readiness_delay_seconds = 5   # GIT_CACHE_SHUTDOWN_READINESS_DELAY_SECONDS
drain_timeout_seconds = 60    # GIT_CACHE_SHUTDOWN_DRAIN_TIMEOUT_SECONDS

New API: app_with_shutdown_async(config) -> (Router, ReadinessGate) for embedders; app_result_async is unchanged.

Chart: terminationGracePeriodSeconds: 75 on the StatefulSet (> readiness delay + drain timeout, vs the K8s default 30s which would have SIGKILLed mid-drain) and the two shutdown env vars wired through the shared env helper.

S3 kill-safety audit (mid-flight death cannot corrupt S3)

Audited the publish path for the "worker dies mid-publish" concern; ordering is already kill-safe:

  1. GenerationPublish::publish_pack_files uploads packs first — content-addressed keys (packs/pack-{sha256}.pack), skipped if already present.
  2. Generation + commit/ref manifests are written via write_json_if_absent_or_matches (conditional PUT; a differing existing object errors instead of being overwritten).
  3. Only then does finish_generation_publish flip the head pointer, via a versioned compare-and-swap (write_repo_head_if_version_matches, bounded retries) that refuses to clobber a newer head.

So a kill at any point leaves at worst orphaned packs/manifests (cleaned up by compaction, which is itself idempotent and conditional-PUT-safe) — readers can never observe a head referencing missing or partial data. The compaction CronJob therefore needs no special shutdown handling.

Testing

  • cargo fmt / cargo clippy --workspace -- -D warnings clean; unit tests pass incl. new healthz_fails_after_shutdown_begins.
  • helm lint + helm template verified terminationGracePeriodSeconds and the new env vars render.

Link to Devin session: https://app.devin.ai/sessions/2b5742d5f71145ffb644f9bf14f8ad72
Requested by: @0lut


Open in Devin Review

devin-ai-integration Bot and others added 6 commits June 11, 2026 07:11
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
…ace period

Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@devin-ai-integration
devin-ai-integration Bot changed the base branch from devin/1781161705-helm-chart to main June 11, 2026 18:36
devin-ai-integration Bot and others added 2 commits June 11, 2026 18:39
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
@0lut

0lut commented Jun 11, 2026

Copy link
Copy Markdown
Owner Author

@devin let's add an integration test that tests 2 things:

  • graceful shutdown does not kill inflight request if request can be done before the shutdown
  • graceful shutdown kills the request post grace period
  • make sure that the test does not take long, we can override options for testing so that we don't need to wait 1 minute

Co-Authored-By: Şahin Olut <olut.sahin@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

Added in 2ff739a. I extracted the serve/drain orchestration into a run_until_shutdown helper that takes the readiness delay / drain timeout as Durations and a generic shutdown signal, so the tests drive shutdown with a oneshot and millisecond timings — both tests together run in well under a second:

  • graceful_shutdown_lets_in_flight_request_finish_within_drain_timeout: real TCP request to a 300ms handler, shutdown triggered mid-flight, asserts the request completes 200/done, the shutdown flag flipped, and the server exits promptly after the drain.
  • graceful_shutdown_kills_request_still_in_flight_after_drain_timeout: 60s handler with a 200ms drain timeout, asserts the server force-exits at the drain deadline with the request still in flight (in production this is the point where the process exits and cuts the connection; in-process the connection task can't be killed without exiting the test binary, so the assertion is that the server gave up on it).

serve() itself is unchanged behaviorally — it now just calls run_until_shutdown with the configured second-granularity values and the real SIGTERM/SIGINT signal.

@0lut
0lut merged commit c75ecd5 into main Jun 11, 2026
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.

1 participant