Sweep leaked test databases daily with a one-day cutoff - #907
Conversation
Cancelled Test runs skip session cleanup, so their Glue databases and S3 Tables namespaces stayed until the weekly sweep removed those older than seven days. Run the sweep on a daily schedule instead of after the weekly scheduled Test run, and expire test databases and namespaces after one day, longer than a GitHub-hosted job can run. Raise the job timeout to 60 minutes so a larger backlog finishes in one run. Closes #892 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| ) | ||
| _TEST_NAMESPACE = re.compile(_PYATHENA_TEST_SCHEMA) | ||
| # Test databases and namespaces older than this are no longer in use. | ||
| _RETENTION = timedelta(days=1) |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN
- Base
8679f04a36f6ffd7190b892c32b1938672ab502f, heada1ee0099081d5dcf78a79621fd0d4d4aeca774d4. - Covered:
.github/workflows/database-sweep.yaml(trigger, job condition, checkout, timeout,DRY_RUN),scripts/sweep_databases.py(_RETENTIONin both sweeps, docstrings, header),scripts/tests/test_sweep_databases.py,docs/testing.md. - Behavior: a scheduled run has
github.ref == refs/heads/masterandDRY_RUN=false, so it applies. A dispatch from another branch is skipped, and a dispatch on master still defaults to preview. Checkout withoutrefgets the dispatched or scheduled master commit, and no other run's code or artifacts are involved. - Names: only per-session random names match. They come from
tests/__init__.py:34(pyathena_test_+ 10 characters) andtests/sqlalchemy/conftest.py:49-63(<ident>,_test_schema,_test_schema_2). No fixture reuses a database across sessions, and benchmarks use their own names. - Tests:
test_databases_expire_after_one_dayandtest_namespaces_expire_after_one_dayrun each sweep in preview with a 2-day-old and a 12-hour-old object. Against the old seven-day cutoff, the 2-day cases fail (2 failed). Recheck, pagination, and absence handling are unchanged and stay covered by the existing tests. just lintandjust scripts(112 passed): pass.
| if: github.repository == 'pyathena-dev/PyAthena' && github.ref == 'refs/heads/master' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| timeout-minutes: 60 |
There was a problem hiding this comment.
Self-review round two (claims and operations): CLEAN
- Base
8679f04a36f6ffd7190b892c32b1938672ab502f, heada1ee0099081d5dcf78a79621fd0d4d4aeca774d4. - "A GitHub-hosted job stops after six hours": no Test job sets
timeout-minutes;database-sweep.yamlis the only workflow with one. The longest recent Test run, 36526628480, took 4h27m of wall-clock time including reruns, each of which is its own session. - "Leftovers live at most about two days": an object created just after the 03:00 UTC run is younger than a day at the next run and is deleted at the one after.
- Leaks line up with cancellations. Cancelled/total Test runs: 09-20 15/38, 09-21 2/4, 09-23 22/37, 09-24 11/22, from
gh run list --created; 09-25 through 09-28 are from the issue. 09-22 had no runs, and 09-27 had no cancellations and left nothing. - Backlog counts (1,625 / 5,812 / 538) come from a read-only preview with this head against account 676287850544. The 09-25 and 09-28 totals are from the same inventory, grouped by
CreateTimedate. - Timeout: at about 0.53 s per database (the 09-27 run deleted 690 in 6m9s), the worst day of about 2,800 databases takes about 25 minutes, within 60.
- AWS operations: the daily API volume is the leak count. Glue metadata calls are not subject to the Athena metadata throttling measured in Investigate persistent metadata throttling during SQLAlchemy reflection #780. Retries stay botocore standard mode with 3 total attempts, and the script adds none.
- Stale prose: no remaining references to seven days or the weekly trigger (
git grep). - The
changesfilter skips the SQLAlchemy and Spark suites for this PR; only the PyAthena suite runs on Ready.
A GitHub-hosted job's six-hour limit bounds CI sessions only; a local session against the same account that stays open longer than a day can lose its database. The two-day bound also holds only while the daily runs succeed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| # | ||
| # Eligible databases must exactly match a PyAthena or SQLAlchemy fixture name | ||
| # and have a creation time more than seven days old. Resource links and federated | ||
| # and have a creation time more than one day old, longer than any test session |
There was a problem hiding this comment.
Independent review (relayed): Codex, via the codex-companion rescue task task-munrmc1k-wnkhab, session 01a0f126-072f-7b33-92bc-46019c01b18a. Static, read-only review of a detached snapshot at a1ee0099081d5dcf78a79621fd0d4d4aeca774d4 (base 8679f04a36f6ffd7190b892c32b1938672ab502f), given the literal diff and no PR framing. Covered surfaces: the four-file diff, the whole sweep script and its tests, test.yaml/test-suite.yaml, the fixture setup, and the repository conventions. Result: FINDINGS (2).
Finding 1. The one-day cutoff can delete resources still used by a local test session. A maintainer running tests against the project account could leave a session active overnight and into a second day, for example while debugging. The sweep checks creation time and identity, but not whether that session is active; it can delete its Glue database and S3 Tables namespace while tests still use them. The six-hour CI job limit cited in the comment does not bound local runs.
Codex also reported: the schedule and manual-dispatch branch guard, the checkout target, and the DRY_RUN expression are consistent with their stated behavior. The 2-day and 12-hour cases would catch a regression to the seven-day cutoff, and their margins avoid clock-boundary issues. There is no source-backed finding on the 60-minute timeout.
There was a problem hiding this comment.
Repaired in 8756735 and c986038. The one-day cutoff stays, as the maintainer decided, and the claims are narrowed:
- The module comment now says one day is longer than a CI session can run, and that a local session against the same account that stays open longer can lose its database.
- The
_RETENTIONcomment (the line this finding named) says "Longer than a CI test session can run" instead of "no longer in use". docs/testing.mdstates the local-session case.
Self-review of the repair, both perspectives: CLEAN. Only comments and docs change; _eligible/_eligible_namespace check only name, creation time, and ID, so the new statement matches the code. just lint and just scripts pass.
Independent follow-up (Codex, read-only, static; session 01a0f12a-edc2-77d0-a2cb-86b71541c120) on a1ee009..c986038: both findings resolved. It raised one new finding on the docs sentence, recorded on docs/testing.md:99.
| # defaults to preview. The job has a 15-minute timeout; a timeout or API failure | ||
| # can leave eligible databases for a later run. | ||
| # .github/workflows/database-sweep.yaml runs this script daily on master, so | ||
| # cancelled test runs leave at most about two days of leftovers. Manual sweep |
There was a problem hiding this comment.
Independent review (relayed): Codex, finding 2. "At most about two days of leftovers" is an unsupported cleanup guarantee. If a daily sweep times out or an API call fails before reaching a resource, that resource remains for a later run, as the following comment itself acknowledges. Amend the claim to describe the expected timing of a successful sweep.
There was a problem hiding this comment.
Repaired in 8756735. The comment now reads "while those runs succeed, a cancelled test run's leftovers are removed within about two days", next to the existing timeout/API-failure caveat. The PR description is amended the same way. Self-review of the repair: CLEAN. Independent follow-up (Codex, on a1ee009..c986038): resolved.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| The test identity needs `s3tables:CreateNamespace`, `s3tables:DeleteNamespace`, `s3tables:ListTables`, and `s3tables:DeleteTable` on the table bucket. | ||
| A session that stops early leaves its namespace behind; `scripts/sweep_databases.py` removes such namespaces once they are more than seven days old. | ||
| A session that stops early leaves its namespace behind; `scripts/sweep_databases.py` removes such namespaces once they are more than one day old. | ||
| It also removes the database and namespace of a session still running after one day, if they are in the account, region, and table bucket that the sweep covers. |
There was a problem hiding this comment.
Independent follow-up (relayed): Codex, read-only and static, on a1ee009..c986038. FINDINGS: the sentence at c986038 said the script removes a long-running local session's resources in the same account. The scheduled sweep uses one fixed region and S3 Tables bucket, so resources in another region or table bucket stay.
Repaired in 8a0e072: the sentence is now limited to "the account, region, and table bucket that the sweep covers". The PR description is amended too. Self-review of the repair: CLEAN. main builds the table-bucket ARN from the configured region and AWS_ATHENA_S3_TABLES_CATALOG, and the Glue client uses the session region. just docs lint passes.
Second independent follow-up (Codex, read-only, static) on c986038..8a0e072, covering docs/testing.md, main/sweep_databases/sweep_s3tables_namespaces, and database-sweep.yaml: CLEAN.
WHAT
.github/workflows/database-sweep.yamlschedule) instead of after the weekly scheduledTestrun (workflow_run). Manual dispatch on master still defaults to preview.refs/heads/master: scheduled runs always use the default branch.ref: master. That pin existed to avoid running code from the triggering test run, and nothing triggers from another run now.timeout-minutesgoes from 15 to 60, so a larger backlog finishes in one run.scripts/sweep_databases.py: test databases and S3 Tables namespaces now expire after one day instead of seven (_RETENTION). Both sweeps use it. The header comment and docstrings describe the new schedule, cutoff, and timeout.sweep_databasesgets Google-styleArgs/Returns.docs/testing.md: the namespace note says one day, and notes that a session still running after one day is swept too if it uses the swept account, region, and table bucket.scripts/tests/test_sweep_databases.py: regression tests for both sweeps. An object created 2 days ago is eligible, and one created 12 hours ago is kept.WHY
Closes #892.
Cancelled
Testruns (cancel-in-progress) never reachpytest_sessionfinish, so their per-session databases and namespaces stay behind. The weekly, seven-day sweep let up to two weeks of them pile up. With a daily run and a one-day cutoff, a leftover is removed within about two days while those runs succeed. A failed or timed-out run leaves the rest for the next one.Why one day is safe: the test jobs set no
timeout-minutes, so GitHub stops a hosted job after six hours. The longest recentTestrun, including reruns, took about 4.5 hours of wall-clock time. The sweep also rechecks each candidate's creation time immediately before deleting it. The six-hour bound covers CI only: a local session against the test account that stays open for more than a day can lose its database or namespace.docs/testing.mdsays so.The issue undercounted the backlog: it counted only
pyathena_test_*. On 2026-09-30, a preview with the new cutoff found:pyathena_test_*databasestest_<hex>,_test_schema,_test_schema_2)All of them were created on days with cancelled runs. Nothing was left from 2026-09-27, which had no cancellations. The worst day was 2026-09-25, with about 2,800 databases. After #863 (PR runs test only the newest Python), 2026-09-28 leaked 241, which is a few minutes of sweeping per day. The 60-minute timeout leaves room for busy days.
Option 3 of the issue (let cancelled sessions clean up after themselves) is not pursued. It would need measurement of signal delivery through
just/uv/xdist, and of whether an AthenaDROP DATABASEfinishes within the runner's grace period. With leftovers now bounded at about two days, that cost is not justified.TEST
Tested commit: 8a0e072.
just lint: pass.just scripts(ruff, mypy, cfn-lint, ShellCheck, actionlint, offline script tests): 112 passed.*_expire_after_one_daytests fail against the old seven-day cutoff: 2 failed, the 2-day cases.just docs lint: pass.--apply, at the maintainer's request. Result:Sweep databases: eligible=7437, deleted=7437, skipped=0andSweep S3 Tables namespaces: eligible=538, deleted=538, skipped=0. A preview afterwards found 0 eligible of either kind.Teston head 8a0e072:test / run (3.14)passed.Testjobs: once Ready, only the PyAthena suite runs. The SQLAlchemy and Spark suites are skipped by thechangesfilter, because no file under their paths changes.🤖 Generated with Claude Code