Skip to content

test(bigtable): add accelerator daemon lifecycle system tests - #18788

Open
mutianf wants to merge 1 commit into
accel12from
accel13
Open

mutianf wants to merge 1 commit into
accel12from
accel13

Conversation

@mutianf

@mutianf mutianf commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Add Cloud Bigtable accelerator daemon lifecycle system tests (test_lifecycle_async.py, test_lifecycle_autogen.py) verifying subprocess/UDS tempdir reaping, zero process/FD/tempdir leaks across repeated lifecycles, crash teardown, and orphan exit on owner SIGKILL.

Part of Stack #18691 (stacked on #18787).

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces system tests for the Bigtable accelerator daemon lifecycle, including both asynchronous (test_lifecycle_async.py) and auto-generated synchronous (test_lifecycle_autogen.py) test suites. The tests verify daemon reaping, resource reclamation after crashes, and leak prevention. Feedback on the changes highlights that the asynchronous test file uses synchronous time.sleep() calls inside async def methods, which blocks the event loop. It is recommended to import asyncio and replace these with non-blocking await asyncio.sleep() calls.

Comment on lines +15 to +20
import multiprocessing
import os
import shutil
import signal
import time
import uuid

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.

medium

Import asyncio to enable non-blocking sleeps in asynchronous test methods.

Suggested change
import multiprocessing
import os
import shutil
import signal
import time
import uuid
import asyncio
import multiprocessing
import os
import shutil
import signal
import time
import uuid

os.kill(pid, signal.SIGKILL)
deadline = time.monotonic() + 10.0
while _harness.pid_alive(pid) and time.monotonic() < deadline:
time.sleep(0.05)

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.

medium

Using synchronous time.sleep() inside an async def test method blocks the entire event loop. Use await asyncio.sleep() instead to keep the event loop responsive.

Suggested change
time.sleep(0.05)
await asyncio.sleep(0.05)

assert not owner.is_alive(), "owner process not reaped after SIGKILL"
deadline = time.monotonic() + 30.0
while _harness.pid_alive(daemon_pid) and time.monotonic() < deadline:
time.sleep(0.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.

medium

Using synchronous time.sleep() inside an async def test method blocks the entire event loop. Use await asyncio.sleep() instead to keep the event loop responsive.

Suggested change
time.sleep(0.1)
await asyncio.sleep(0.1)

This branch has not been deployed

No deployments
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