Skip to content

UN-4223 [FIX] Kill a stuck PG consumer child instead of restarting the whole pod - #2312

Closed
johnyrahul wants to merge 2 commits into
mainfrom
UN-4223-pg-supervisor-stuck-child
Closed

johnyrahul wants to merge 2 commits into
mainfrom
UN-4223-pg-supervisor-stuck-child

Conversation

@johnyrahul

@johnyrahul johnyrahul commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

What

  • The PG-queue prefork supervisor now SIGKILLs and re-forks a single child whose heartbeat stays frozen past a per-task cap, instead of letting that child fail liveness for the whole pod.
  • The fleet /health probe goes 503 only once at least half of the children are stale (previously: the single oldest child).

Why

  • UN-4223: four prod US outages in three days. Each one started with one pg-executor child hung inside an LLM call that never returned.
  • freshness() reported the oldest child's age, so that one child turned /health into a 503. Liveness then restarted the container, all 20 children stopped consuming on SIGTERM, and the container waited out terminationGracePeriodSeconds (7260s) for the hung call. That meant about 2h of zero consumption per pod, and on three occasions both replicas were down at once.
  • UN-4179 is the same mechanism on staging. The hung child's lease thread kept renewing forever, so the reaper never recovered the claim.

How

  • Stuck-child kill (_kill_stuck_children, runs every monitor tick):
    • A child's heartbeat only advances between tasks, so a heartbeat older than the cap means one task has overrun.
    • The supervisor SIGKILLs that child. SIGTERM wouldn't work, because a child only acts on it between tasks. The existing reap path then re-forks the slot.
    • Killing the child also kills its lease thread. The claim lapses after LEASE_SECONDS, the reaper redelivers the message, and max_attempts bounds how often that repeats.
    • A killed slot isn't re-killed before it is reaped.
    • A fresh replacement isn't killed for its predecessor's old heartbeat: the child itself must also have been up longer than the cap.
    • Skipped during shutdown, where _join_children takes over.
    • Only children that have finished loading are considered, so a slow bootstrap is never mistaken for a stuck task.
    • When a killed slot is reaped, its heartbeat is reseeded, so the replacement doesn't inherit the frozen age during bootstrap. Without this, a two-child fleet (where one child is the quorum) would still fail the probe. Crash exits are not reseeded.
  • Cap (stuck_child_seconds_from_env):
    • Defaults to WORKER_PG_QUEUE_CONSUMER_HEALTH_STALE_SECONDS, the threshold the consumer already documents as the upper bound on one task and that used to restart the whole container. The bound is unchanged; it now costs one slot instead of the pod.
    • Override with WORKER_PG_QUEUE_CONSUMER_STUCK_CHILD_SECONDS, which must be finite and > 0.
    • With no HEALTH_PORT (no probe to enforce the bound), nothing is killed unless the override is set.
    • A startup warning fires if the override is set above HEALTH_STALE_SECONDS, where a stuck child could fail the probe before it is killed.
  • Health:
    • freshness() returns quorum_age(), the age at least ceil(n × 0.5) children have reached: 10 of 20, 2 of 3, 1 of 2, and 1 of 1 (unchanged).
    • The crash-loop inf override is unchanged.
    • The /health JSON age key is now quorum_child_seconds_since_poll. The body still carries oldest_child_seconds_since_poll and adds stale_children.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • Long tasks: a task running past HEALTH_STALE_SECONDS is now SIGKILLed and redelivered. Before, it triggered a full container restart, which also killed it after the grace period. So no task that used to finish is cut short. Every chart and compose consumer sets HEALTH_STALE_SECONDS >= VT.
  • Redelivery: a killed task is redelivered and may run again, the same at-least-once behaviour as a pod restart. If it hangs every time, max_attempts stops it.
  • Probe behaviour: fewer than half of the children wedged no longer restarts the pod. The stuck-child kill is what recovers them now.
  • /health JSON: the age key was renamed. No consumer of the old key was found in unstract or unstract-cloud; it is still present in the body.
  • pg_consumer_heartbeat_age_seconds: now reports the half-of-fleet age instead of the oldest child's.
  • New metrics: pg_consumer_oldest_child_age_seconds (the most-stale child) and pg_consumer_stuck_child_kills_total, so one stuck child and each kill stay visible while the half-of-fleet heartbeat looks healthy.

Database Migrations

  • None

Env Config

  • New, optional: WORKER_PG_QUEUE_CONSUMER_STUCK_CHILD_SECONDS (default: WORKER_PG_QUEUE_CONSUMER_HEALTH_STALE_SECONDS when a health port is set, else off).
  • Follow-up in unstract-cloud: prod workerPgExecutor has HEALTH_STALE_SECONDS: 7260, so without an override a hung child is killed after about 2h. Setting WORKER_PG_QUEUE_CONSUMER_STUCK_CHILD_SECONDS: "3660" aligns it with the executor's 1h task and RPC limit.

Relevant Docs

Related Issues or PRs

Dependencies Versions

  • None

Notes on Testing

  • 23 new unit cases (plus 1 metrics case) in workers/tests/test_pg_consumer_supervisor.py:
    • The half-of-fleet threshold (20, 3, 2 and 1 children), and that one stale child of 20 doesn't age the fleet.
    • Env parsing for the cap: default, off without a port, override, and invalid values.
    • The kill: only the stuck child, not re-killed before reap, reaped and re-forked without counting as a crash, a fresh replacement spared, no kill while stopping or with the cap disabled, and an already-gone pid tolerated.
    • The /health body keys.
  • test_pg_consumer_supervisor.py and test_pg_metrics.py pass: 109 tests.

Screenshots

Checklist

I have read and understood the Contribution Guidelines.

🤖 Generated with Claude Code

…e whole pod

One hung task (an LLM call that never returns) froze its child's heartbeat.
The fleet probe reported the OLDEST child's age, so that single child failed
liveness, and the container restart then waited out the full termination
grace for the hung call, leaving the pod consuming nothing for up to ~2h.

- The supervisor now SIGKILLs a child whose heartbeat stays frozen past the
  stuck-child cap and re-forks it. Its lease stops renewing, so the reaper
  redelivers the message (bounded by max_attempts). The cap defaults to
  HEALTH_STALE_SECONDS (the existing per-task bound) and is overridable via
  WORKER_PG_QUEUE_CONSUMER_STUCK_CHILD_SECONDS; with no health port it is off
  unless set explicitly.
- /health now goes 503 only once at least half the children are stale. The
  oldest child's age and the stale-child count remain in the body.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnyrahul
johnyrahul marked this pull request as ready for review October 5, 2026 07:12
@greptile-apps

greptile-apps Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

via Greptile

RetriggerConfidence Score: 5/5

[High risk] Changes how the queue consumer supervisor detects and handles stuck workers.

The PR appears safe to merge; the remaining issue is a non-blocking inaccuracy in the kill counter.

Fix All in Claude CodeFindings

  1. P2 Kill counter can overcount ▶
Fix with agent prompt
### Issue 1
workers/pg_queue_consumer/supervisor.py:400
If a child exits just before the supervisor sends SIGKILL, `ProcessLookupError` is suppressed but this line still increments `pg_consumer_stuck_child_kills_total`. The metric then reports a stuck-child kill that did not happen, making spontaneous exits look like supervisor interventions.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary

The supervisor now kills and replaces an overlong PG-queue consumer child instead of relying on a pod restart. Fleet liveness uses a half-stale threshold, and the health and metrics endpoints expose additional child-level diagnostics.

  • The follow-up reseeds a killed slot’s heartbeat on reap and excludes children still loading from stuck-task kills.
  • It adds an oldest-child age gauge and a stuck-child kill counter.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  H[Child heartbeat ages past cap] --> K[Supervisor attempts SIGKILL]
  K --> R[Reap child and reseed slot]
  R --> F[Re-fork replacement]
  F --> P[Replacement polls and publishes heartbeat]
  K --> M[Increment stuck-child kill counter]
Loading

Reviews (2) · Last reviewed commit: "UN-4223 [FIX] Address review: two-child ..."

Comment thread workers/pg_queue_consumer/supervisor.py
Comment thread workers/pg_queue_consumer/supervisor.py
Comment thread workers/queue_backend/pg_queue/metrics.py
…ll metrics

- Reseed a stuck-killed slot's heartbeat when it is reaped. In a two-child
  fleet one slot is the quorum, so the frozen age kept /health at 503 through
  the replacement's bootstrap and could still restart the pod. Crash exits
  are not reseeded, so crash-loop detection is unchanged.
- Only kill children that have finished loading. With a cap shorter than the
  bootstrap, a child that had not polled yet could be killed before starting.
- Warn at startup when STUCK_CHILD_SECONDS exceeds HEALTH_STALE_SECONDS, where
  a stuck child can fail the probe before it is killed.
- Export pg_consumer_oldest_child_age_seconds and
  pg_consumer_stuck_child_kills_total, so a stuck child and each kill are
  visible while the half-of-fleet heartbeat stays healthy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Oct 5, 2026

Copy link
Copy Markdown

def mark_stuck_killed(self, slot: int) -> None:
self._validate(slot)
self._stuck_killed.add(slot)
self._stuck_kill_count += 1

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 Kill counter can overcount If a child exits just before the supervisor sends SIGKILL, ProcessLookupError is suppressed but this line still increments pg_consumer_stuck_child_kills_total. The metric then reports a stuck-child kill that did not happen, making spontaneous exits look like supervisor interventions.

Prompt To Fix With AI
This is a comment left during a code review.
Path: workers/pg_queue_consumer/supervisor.py
Line: 400

Comment:
**Kill counter can overcount** If a child exits just before the supervisor sends SIGKILL, `ProcessLookupError` is suppressed but this line still increments `pg_consumer_stuck_child_kills_total`. The metric then reports a stuck-child kill that did not happen, making spontaneous exits look like supervisor interventions.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Unstract test results

Per-group results

Status Group Tier Passed Failed Errors Skipped Duration (s)
✅ e2e-api-deployment e2e 3 0 0 0 8.3
✅ e2e-coowners e2e 1 0 0 0 1.1
✅ e2e-etl e2e 1 0 0 0 12.1
✅ e2e-login e2e 2 0 0 0 1.2
✅ e2e-prompt-studio e2e 1 0 0 0 4.8
✅ e2e-smoke e2e 2 0 0 0 2.2
✅ e2e-workflow e2e 1 0 0 0 12.0
✅ frontend unit 620 0 0 0 18.0
✅ integration-backend integration 603 0 0 26 40.9
✅ integration-connectors integration 1 0 0 7 6.7
✅ integration-workers integration 164 0 0 1 26.1
❌ ui e2e 0 1 0 0 0.0
✅ unit-backend unit 1422 0 0 1 46.1
✅ unit-connectors unit 72 0 0 0 9.7
✅ unit-core unit 276 0 0 0 3.2
✅ unit-platform-service unit 15 0 0 0 2.8
✅ unit-rig unit 120 0 0 0 4.4
✅ unit-runner unit 10 0 0 0 2.9
✅ unit-sdk1 unit 718 0 0 0 33.1
✅ unit-workers unit 1410 0 0 1 132.6
TOTAL 5442 1 0 36 368.3

Critical paths

⚠️ Critical paths not yet covered

  • workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)
✅ Covered critical paths
  • auth-login — covered by e2e-login
  • adapter-register-llm — covered by integration-backend
  • workflow-author — covered by integration-backend
  • co-owner-manage — covered by integration-backend, e2e-coowners
  • workflow-create-execute — covered by e2e-workflow
  • api-deployment-provision — covered by integration-backend
  • api-deployment-auth — covered by integration-backend
  • api-deployment-run — covered by e2e-api-deployment
  • mcp-server-auth — covered by integration-backend
  • mcp-platform-auth — covered by integration-backend
  • platform-key-whoami — covered by integration-backend
  • prompt-studio-author — covered by integration-backend
  • prompt-studio-fetch-response — covered by e2e-prompt-studio
  • connector-register-test — covered by integration-backend
  • pipeline-etl-execute — covered by e2e-etl
  • usage-aggregate-read — covered by integration-backend
  • usage-token-tracking — covered by e2e-api-deployment
  • callback-result-delivery — covered by e2e-api-deployment

@johnyrahul johnyrahul closed this Oct 5, 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