Skip to content

revert(bigtable): drop the stale-tempdir sweep - #18832

Open
mutianf wants to merge 1 commit into
bigtable-accelfrom
accel-drop-sweep
Open

mutianf wants to merge 1 commit into
bigtable-accelfrom
accel-drop-sweep

Conversation

@mutianf

@mutianf mutianf commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

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.

Change-Id: I9a54c012f86ad1562da507e0cd415f6befd644ed
@mutianf
mutianf requested a review from a team as a code owner October 23, 2026 18:32

@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 removes the background sweep mechanism (_sweep_stale_tempdirs) and its associated metadata files, constants, and tests from the Bigtable accelerator daemon. The cleanup of stale temp directories at startup is no longer performed, as the daemon is designed to exit and clean up its own socket when the parent process dies, making the sweep mechanism redundant and potentially hazardous. Unused imports and obsolete unit tests have also been removed. I have no feedback to provide as there are no review comments.

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