Skip to content

[fix][R2-2346] Add terminationGracePeriodSeconds and preStop hook to agent-executor proxy - #354

Merged
emas-rt merged 2 commits into
mainfrom
update-termination-grace-period-helm
Jul 30, 2026
Merged

[fix][R2-2346] Add terminationGracePeriodSeconds and preStop hook to agent-executor proxy#354
emas-rt merged 2 commits into
mainfrom
update-termination-grace-period-helm

Conversation

@emas-rt

@emas-rt emas-rt commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds terminationGracePeriodSeconds: 30 to the agent-executor proxy pod spec
  • Adds lifecycle.preStop sleep of 5s to the proxy container

Without terminationGracePeriodSeconds, kubelet's default 30s still applies but there's no preStop hook, so SIGTERM can arrive before kube-proxy finishes removing the endpoint — new connections still route to the terminating pod. The preStop sleep covers that gap, and terminationGracePeriodSeconds: 30 ensures kubelet doesn't SIGKILL during the drain window (proxy has a 10s DRAIN_TIMEOUT_MS).

Closes R2-2346.

Test Plan

  • CI green
  • helm template diff shows only the two new fields on the proxy deployment, not the controller

…agent-executor proxy

Mirrors the lifecycle config from retool_development PR #82013, which only
affects CI. The real values live here in retool-helm.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
{{ toYaml $as.labels | indent 8 }}
{{- end }}
spec:
terminationGracePeriodSeconds: 30

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.

P2 Add proxy shutdown rendering coverage

The agent-sandbox fixtures do not assert that these shutdown fields render only on the proxy deployment. Add a focused Helm rendering test covering both terminationGracePeriodSeconds and the preStop hook so future template changes cannot silently remove or misplace them.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

The PR appears safe to merge, although the previously requested regression coverage remains absent.

The shutdown fields are correctly scoped to the proxy, but existing CI only schema-validates rendered manifests and does not protect their presence or proxy-only placement against future template regressions.

Files Needing Attention: charts/retool/templates/deployment_agent_sandbox.yaml

Important Files Changed

Filename Overview
charts/retool/templates/deployment_agent_sandbox.yaml Adds the proxy pod termination grace period and container preStop lifecycle hook at the intended deployment and container scopes.
charts/retool/Chart.yaml Bumps the chart patch version to publish the template update.

Reviews (2): Last reviewed commit: "[chore] bump chart version to 6.11.13" | Re-trigger Greptile

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@emas-rt
emas-rt merged commit 669795c into main Jul 30, 2026
13 checks passed
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