Repository navigation
feat(bigtable): sweep stale accelerator daemon tempdirs at startup - #18688
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic cleanup of stale accelerator daemon temporary directories at startup by scanning for leftover directories, checking if their sockets are active, and removing them if they are older than a threshold. The review feedback correctly identifies potential compatibility issues on non-Unix platforms (such as Windows) where Unix domain sockets are unsupported. It is recommended to add a defensive guard in the sweep function to avoid AttributeErrors and to skip the new unit tests on unsupported platforms.
Change-Id: Icd8495d9c7f1bdcf39ae984b0037f813ef4c4011
|
Note to reviewer: logic changes are in daemon.py. Rest are formatting changes |
…ilure Change-Id: Ie5c45b75e0785a284a0d241415cd5b8492e14e9c
| try: | ||
| with open(pid_path) as _f: | ||
| pid = int(_f.read().strip()) | ||
| os.kill(pid, 0) |
There was a problem hiding this comment.
PID reuse makes stale tempdirs potentially immortal. Once the OS recycles this PID onto any long-lived service (common on Linux after wrap-around), os.kill(pid, 0) returns True forever and the dir is never reaped. Cheap mitigation: also read /proc/<pid>/stat field 22 (start_time, in clock ticks since boot) and compare to the tempdir's mtime — a mismatch means PID reuse and the dir is safe to sweep. Not a blocker (worst case is leaked /tmp entries), but worth tracking as a follow-up.
| pid_path = os.path.join(path, _PID_FILENAME) | ||
| try: | ||
| with open(pid_path) as _f: | ||
| pid = int(_f.read().strip()) |
There was a problem hiding this comment.
If start() crashes between the deadline-file write and the pid-file write, the pid file ends up empty / truncated. int(_f.read().strip()) then raises ValueError, the outer except OSError doesn't catch it, and (depending on where the exception goes) the dir gets skipped and lives forever. Two options: (a) write to daemon.pid.tmp and os.rename into place atomically, or (b) broaden the except to also catch ValueError and treat an unparseable pid as "deadline-only, fall back to the age heuristic".
sushanb
left a comment
sushanb
left a comment
There was a problem hiding this comment.
LGTM. All three prior findings addressed (FD leak with-block, blank-line nit, scope confirmed). Two new follow-up suggestions above on PID reuse and partial-pid-write robustness — both can land as follow-ups, not blockers for this PR.
Change-Id: I2dcd04d457e90e628ebd17d69987174a58489379
Reverts the stale-tempdir sweep added in #18688, keeping the two sweep-independent cleanups that came with it. ## Why The sweep walked `/tmp` at every `start()` and deleted `bt-accel-*` directories whose owning process looked dead. Reviewing the Go daemon (`bigtable/internal/accelerator`) shows most of what it was defending against is already handled there, and the rest is not worth the mechanism: - **No stray process.** `Server.monitorParentPid` polls `os.Getppid()` every 500ms and shuts the daemon down when it changes. A `SIGKILL` of the Python process takes the daemon with it within half a second, independent of the stdin-EOF watchdog. - **No stray socket.** The daemon removes its own `sock` on shutdown. - **What actually leaks** is `daemon.log`, plus `identity.json` if the client died before reading it. A few KB of dead files per crashed client, in a directory `systemd-tmpfiles` already clears on its own schedule (and which is tmpfs or ephemeral on most hosts and in every container). Against that, the sweep was the only code in the module that deletes files it does not own, in a world-writable directory, by inferring the liveness of processes in other namespaces. That inference was the source of every bug found in review: two paths that could delete a *live* daemon's socket and identity file, a PID-namespace confusion under a shared `/tmp`, an `OverflowError` escaping from a planted pid file, and a test fixture that swept the developer's real `/tmp`. Each was fixable, and each fix added more machinery to protect a few KB. ## What's kept `_socket_is_live`, factored out of the probe `_wait_until_ready` had open-coded, and the named filename/timeout constants. Both are independent of the sweep. A comment on `_TEMPDIR_PREFIX` records the reasoning so this does not get reinvented. ## Testing 61 tests in `test_accelerator_daemon.py`, 2527 in `tests/unit/data`. Verified the result is identical to the pre-sweep baseline apart from the kept items. ## Note for the stack accel9 / accel10 still branch from the pre-squash commits and need a rebase. This should make accel10's conflict substantially smaller, since it is the only other branch that edits `_daemon.py` and most of what it collided with is removed here.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕
Stack created with GitHub Stacks CLI • Give Feedback 💬