Skip to content

Sweep leaked test databases daily with a one-day cutoff - #907

Merged
laughingman7743 merged 4 commits into
masterfrom
chore/892-daily-sweep
Oct 1, 2026
Merged

laughingman7743 merged 4 commits into
masterfrom
chore/892-daily-sweep

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

WHAT

  • .github/workflows/database-sweep.yaml
    • Runs the sweep daily at 03:00 UTC (schedule) instead of after the weekly scheduled Test run (workflow_run). Manual dispatch on master still defaults to preview.
    • The job condition is reduced to the repository and refs/heads/master: scheduled runs always use the default branch.
    • Checkout no longer pins ref: master. That pin existed to avoid running code from the triggering test run, and nothing triggers from another run now.
    • timeout-minutes goes 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_databases gets Google-style Args/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 Test runs (cancel-in-progress) never reach pytest_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 recent Test run, 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.md says so.

The issue undercounted the backlog: it counted only pyathena_test_*. On 2026-09-30, a preview with the new cutoff found:

Kind Eligible
pyathena_test_* databases 1,625
SQLAlchemy suite databases (test_<hex>, _test_schema, _test_schema_2) 5,812
S3 Tables namespaces 538

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 Athena DROP DATABASE finishes 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.
  • The new *_expire_after_one_day tests fail against the old seven-day cutoff: 2 failed, the 2-day cases.
  • just docs lint: pass.
  • Read-only preview against the test account with the new script: the counts above.
  • The backlog was then removed from a local checkout of this branch with --apply, at the maintainer's request. Result: Sweep databases: eligible=7437, deleted=7437, skipped=0 and Sweep S3 Tables namespaces: eligible=538, deleted=538, skipped=0. A preview afterwards found 0 eligible of either kind.
  • AWS Test on head 8a0e072: test / run (3.14) passed.
  • Not run before merge: the workflow itself. Dispatch is master-only and the schedule starts after merge. After merge, check the first scheduled run's step summary.
  • AWS Test jobs: once Ready, only the PyAthena suite runs. The SQLAlchemy and Spark suites are skipped by the changes filter, because no file under their paths changes.

🤖 Generated with Claude Code

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (implementation behavior): CLEAN

  • Base 8679f04a36f6ffd7190b892c32b1938672ab502f, head a1ee0099081d5dcf78a79621fd0d4d4aeca774d4.
  • Covered: .github/workflows/database-sweep.yaml (trigger, job condition, checkout, timeout, DRY_RUN), scripts/sweep_databases.py (_RETENTION in both sweeps, docstrings, header), scripts/tests/test_sweep_databases.py, docs/testing.md.
  • Behavior: a scheduled run has github.ref == refs/heads/master and DRY_RUN=false, so it applies. A dispatch from another branch is skipped, and a dispatch on master still defaults to preview. Checkout without ref gets 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) and tests/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_day and test_namespaces_expire_after_one_day run 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 lint and just 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims and operations): CLEAN

  • Base 8679f04a36f6ffd7190b892c32b1938672ab502f, head a1ee0099081d5dcf78a79621fd0d4d4aeca774d4.
  • "A GitHub-hosted job stops after six hours": no Test job sets timeout-minutes; database-sweep.yaml is 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 CreateTime date.
  • 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 changes filter 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>
Comment thread scripts/sweep_databases.py Outdated
#
# 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _RETENTION comment (the line this finding named) says "Longer than a CI test session can run" instead of "no longer in use".
  • docs/testing.md states 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.

Comment thread scripts/sweep_databases.py Outdated
# 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

laughingman7743 and others added 2 commits September 30, 2026 16:14
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread docs/testing.md
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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@laughingman7743
laughingman7743 marked this pull request as ready for review September 30, 2026 07:22
@laughingman7743
laughingman7743 merged commit 809b380 into master Oct 1, 2026
15 checks passed
@laughingman7743
laughingman7743 deleted the chore/892-daily-sweep branch October 1, 2026 00:02
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.

Cancelled test runs leave Glue databases and S3 Tables namespaces until the weekly sweep

1 participant