[SPARK-58021][CONNECT] Reap orphaned local pool members by ericm-db · Pull Request #58247 · apache/spark · GitHub

What changes were proposed in this pull request?

This is layer 5 of the ten-PR local Connect pool stack:

#57684 -> #57685 -> #57907 -> #57686 -> #58247 ->
#58367 -> #57687 -> #58248 -> #57102 -> #57688

This layer adds recovery after a pool member has been published or claimed:

a janitor for ready, claimed, and retiring members;
retirement of dead, unreachable, incompatible, idle, and malformed ready members;
retirement of claims whose client or server process generation has disappeared;
persisted claimant process identities so PID reuse cannot strand an orphaned claim;
recovery of independently valid server handles from malformed records; and
bounded, PID-reuse-safe completion of retirement.

It also addresses the two follow-ups from the review of #57686: successful SIGTERM delivery is
persisted so young retirement passes do not repeat the process inspection, and the daemon-pid
fallback now has coverage for removing state after that process exits.

Pending-launch, attendant, conf-file, and unreferenced member-directory recovery remain in
#58367.

Why are the changes needed?

Pool members outlive individual Python call frames. A client can be killed without releasing its
claim, a ready server can die or become unreachable, and persisted process IDs can be reused.
Without a conservative janitor these cases can strand JVMs or make unusable members count toward
the pool indefinitely.

Separating post-publication recovery from launch recovery gives each process-ownership model its
own review unit: this layer authorizes server cleanup from persisted process generations, while the
next layer handles attendant commands and launch process groups.

Does this PR introduce any user-facing change?

No. The pool is not wired into SparkSession in this layer.

How was this patch tested?

The focused pool suite's 50 tests passed:

PYTHONPATH=python:python/lib/pyspark.zip:python/lib/py4j-0.10.9.9-src.zip \
  SPARK_TESTING=1 \
  .venv/bin/python -m unittest -v \
  pyspark.sql.tests.connect.test_connect_local_server_pool

The changed files also passed Python AST parsing, Ruff checking and formatting, targeted mypy,
git diff --check, and changed-file ASCII and 100-column checks.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5)

### What changes were proposed in this pull request? This is layer 4 of the nine-PR local Connect pool stack: #57684 -> #57685 -> #57907 -> #57686 -> #58247 -> #57687 -> #58248 -> #57102 -> #57688 The three lower layers are merged, so GitHub shows only this layer's two-file diff. The review unit introduced here is commit `37dbe4b5ae7`. This layer adds the normal server-retirement path on top of member claiming: - atomic JSON state replacement and cleanup of interrupted-write temporary files; - validated retired-state records with recovery of independently valid process IDs; - crash-safe state-to-retired renames; - graceful SIGTERM shutdown followed by bounded SIGKILL escalation; and - idempotent claimed-member release, including forked-child and retry handling. Janitor recovery, acquisition, forceful purge, SparkSession integration, and warmup remain in later PRs. ### Why are the changes needed? A claimed server must be released without losing the only process handle if the client crashes during the state transition. Isolating retirement keeps atomic persistence, graceful shutdown, and release semantics independently reviewable before orphan scanning and launch orchestration are added. ### Does this PR introduce _any_ user-facing change? No. The pool is not wired into SparkSession in this layer. ### How was this patch tested? Added eight focused tests at this layer, bringing the suite to 33 tests, and extended the storage test with interrupted-write and stale-temporary-file coverage. The focused command is: ```bash python/run-tests --testnames pyspark.sql.tests.connect.test_connect_local_server_pool ``` At the stack tip, the equivalent direct `unittest` invocation passed all 56 pool tests, including the two real-server E2E tests. The rebuilt commit passed Python AST parsing, `git diff --check`, and changed-file ASCII and 100-column checks. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5) Closes #57686 from ericm-db/local-connect-pool-lifecycle. Authored-by: Eric Marnadi <eric.marnadi@databricks.com> Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

### What changes were proposed in this pull request? This is layer 4 of the nine-PR local Connect pool stack: #57684 -> #57685 -> #57907 -> #57686 -> #58247 -> #57687 -> #58248 -> #57102 -> #57688 The three lower layers are merged, so GitHub shows only this layer's two-file diff. The review unit introduced here is commit `37dbe4b5ae7`. This layer adds the normal server-retirement path on top of member claiming: - atomic JSON state replacement and cleanup of interrupted-write temporary files; - validated retired-state records with recovery of independently valid process IDs; - crash-safe state-to-retired renames; - graceful SIGTERM shutdown followed by bounded SIGKILL escalation; and - idempotent claimed-member release, including forked-child and retry handling. Janitor recovery, acquisition, forceful purge, SparkSession integration, and warmup remain in later PRs. ### Why are the changes needed? A claimed server must be released without losing the only process handle if the client crashes during the state transition. Isolating retirement keeps atomic persistence, graceful shutdown, and release semantics independently reviewable before orphan scanning and launch orchestration are added. ### Does this PR introduce _any_ user-facing change? No. The pool is not wired into SparkSession in this layer. ### How was this patch tested? Added eight focused tests at this layer, bringing the suite to 33 tests, and extended the storage test with interrupted-write and stale-temporary-file coverage. The focused command is: ```bash python/run-tests --testnames pyspark.sql.tests.connect.test_connect_local_server_pool ``` At the stack tip, the equivalent direct `unittest` invocation passed all 56 pool tests, including the two real-server E2E tests. The rebuilt commit passed Python AST parsing, `git diff --check`, and changed-file ASCII and 100-column checks. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Fable 5) and OpenAI Codex (GPT-5) Closes #57686 from ericm-db/local-connect-pool-lifecycle. Authored-by: Eric Marnadi <eric.marnadi@databricks.com> Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com> (cherry picked from commit a1857f5) Signed-off-by: Daniel Tenedorio <daniel.tenedorio@databricks.com>

The approach generally looks reasonable. I just had a couple extra testing ideas. LGTM after all comments are resolved

Should we also have tests for:

_idle_timeout() returning <= 0 (disabled) or falling back on a non-integer env value
A healthy, non-idle server member survives a full janitor() pass in isolation (it's covered incidentally by the f2e5 case, but not directly)

added!

ericm-db force-pushed the local-connect-pool-recovery branch from 6883333 to beee54e Compare August 27, 2026 23:41

ericm-db added 2 commits August 27, 2026 23:56

[SPARK-58021][CONNECT] Reap orphaned local pool members

0fe123a

[SPARK-58021][CONNECT] Track local pool claimant process generations

3de7b10

ericm-db force-pushed the local-connect-pool-recovery branch from beee54e to 3de7b10 Compare August 28, 2026 00:08

ericm-db mentioned this pull request Aug 28, 2026

[SPARK-58021][CONNECT] Recover abandoned local pool launches #58367

Open

dtenedor approved these changes Aug 28, 2026

View reviewed changes

[SPARK-58021][CONNECT] Test local pool idle retirement boundaries

b1b21c8

[SPARK-58021][CONNECT] Reap orphaned local pool members by ericm-db · Pull Request #58247 · apache/spark · GitHub

Uh oh!

[SPARK-58021][CONNECT] Reap orphaned local pool members - #58247

[SPARK-58021][CONNECT] Reap orphaned local pool members#58247
ericm-db wants to merge 3 commits into
apache:masterfrom
ericm-db:local-connect-pool-recovery

ericm-db commented Aug 24, 2026 •
edited
Loading

Uh oh!

Uh oh!

dtenedor left a comment

Uh oh!

dtenedor Aug 28, 2026

Uh oh!

ericm-db Aug 28, 2026

Uh oh!

dtenedor commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

+                          self.assertNotIn("process_start_id", retired)
+                      self.assertIsNone(unreadable_pid.poll())
+                  def test_reap_claimed_of_dead_client(self) -> None:

[SPARK-58021][CONNECT] Reap orphaned local pool members by ericm-db · Pull Request #58247 · apache/spark · GitHub

Uh oh!

[SPARK-58021][CONNECT] Reap orphaned local pool members - #58247

[SPARK-58021][CONNECT] Reap orphaned local pool members#58247ericm-db wants to merge 3 commits intoapache:masterapache/spark:masterfrom ericm-db:local-connect-pool-recoveryericm-db/spark:local-connect-pool-recoveryCopy head branch name to clipboard

Conversation

ericm-db commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

dtenedor left a comment

Choose a reason for hiding this comment

Uh oh!

dtenedor Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

ericm-db Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

dtenedor commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[SPARK-58021][CONNECT] Reap orphaned local pool members#58247
ericm-db wants to merge 3 commits into
apache:masterfrom
ericm-db:local-connect-pool-recovery

ericm-db commented Aug 24, 2026 •
edited
Loading