Skip to content

Give each test session its own S3 Tables namespace - #815

Merged
laughingman7743 merged 7 commits into
masterfrom
fix/s3tables-per-run-namespace
Sep 25, 2026
Merged

laughingman7743 merged 7 commits into
masterfrom
fix/s3tables-per-run-namespace

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

WHAT

Per-session namespace. The test session creates its own S3 Tables namespace, named like its schema (pyathena_test_<id>), in AWS_ATHENA_S3_TABLES_CATALOG's table bucket. At the end it deletes the namespace with any table left in it (tests/pyathena/conftest.py). If a later setup step fails, pytest_sessionstart deletes the namespace itself, because pytest then skips pytest_sessionfinish. AWS_ATHENA_S3_TABLES_NAMESPACE is no longer read, and docs/testing.md lists the S3 Tables permissions the session needs.

Sweep. scripts/sweep_databases.py also sweeps that table bucket. It removes namespaces named like a test schema that are more than seven days old, deleting their tables first. The same rules as for Glue databases apply:

  • the inventory is complete before anything is deleted
  • each namespace is rechecked just before deletion, by its namespace ID
  • only concurrent absence (NotFoundException) is ignored, including for a single table

The namespace ID is checked before the tables are deleted and again before the namespace is deleted. Each table is deleted only if GetTable shows it still belongs to that namespace, passing its versionToken to DeleteTable (ListTables does not return the namespace ID). DeleteNamespace takes only a name. So the one remaining gap is an empty namespace recreated under the same name between the last check and DeleteNamespace. Namespace names are random per session (10 characters).

The sweep workflow sets AWS_ATHENA_S3_TABLES_CATALOG.

Template. cloudformation/github_actions_oidc.yaml no longer creates the fixed pyathena namespace or its S3TablesNamespaceName parameter.

WHY

The S3 Tables tests shared one fixed namespace across all CI jobs. When another job dropped its table during a listing, the listing failed with EntityNotFoundException (Service: S3Tables), even when filtered by table name:

  • Glue GetTables in run 36009777585
  • Athena ListTableMetadata in run 36080115567

This made TestGlueMetadataClient::test_reads_s3_tables_catalog intermittent, and running the suites in parallel (#814) made it more frequent.

A probe on 2026-09-25 created a namespace in the CI table bucket in 0.9 s. Athena and Glue both saw it immediately. Creating, listing, getting and dropping a table in it all worked, and the namespace could then be deleted.

Order for the template change.

  1. Merge this PR. After that no test uses the fixed namespace.
  2. Rebase branches based on older master, which still expect it.
  3. Empty the pyathena namespace.
  4. Update the stack, which deletes the namespace. That deletion fails if the namespace still contains tables.

TEST

Tested commit: 9aa3f31.

  • just lint passed, including cfn-lint. just docs lint reported 0 errors.
  • uv run --locked python -m pytest scripts/tests/: 49 passed.
    • The namespace sweep tests fail with the recheck removed (2 fail).
    • They also fail when concurrent absence is no longer ignored (1 fails).
    • test_a_table_already_gone_does_not_stop_the_namespace fails against 883510b.
    • Removing the table's namespace-ID check, the final namespace recheck, or the versionToken each makes a test fail.
  • Live sweep against the real API (no CI running, cutoff patched): a namespace created for the check with two tables gave eligible=1, deleted=1, skipped=0. Only default and pyathena remained.
  • Live setup-failure check, without xdist and with a nonexistent staging bucket so that _upload_rows fails:
    • 883510b leaked a namespace (deleted afterwards).
    • 70e3082 left only default and pyathena.
    • With xdist, the controller fails in _upload_rows before any worker starts, so no namespace is created.
  • scripts/sweep_databases.py preview against the CI account: Preview databases: eligible=0 and Preview S3 Tables namespaces: eligible=0.
  • Live, on d28077f before the review repairs: pytest -n 1 tests/pyathena/test_glue.py tests/pyathena/sqlalchemy/test_base.py -k "s3tables or s3_tables" gave 6 passed, 0 skipped, and left no namespace.
  • CI run 36103732783 on a9c0a0c (test constants moved to the top of the file): all 19 checks green, 40.9 min, and the run left no namespace in the bucket. Run 36100312229 on 9aa3f31 was also all green (39.9 min). Runs on 70e3082 (36094239865) and b77865d (36096956008) were also all green.
  • CI run 36089493551 on 883510b:
    • All 15 test jobs passed, with all 19 checks green.
    • The stages ran sequentially under master's workflow and took 39.9 min. The runs on 70e3082 and b77865d took 38.8 and 43.4 min. The 12 other PR Test runs over 2026-09-23..25 that passed on their first attempt took 38–46 min (median 41.5), so the namespaces add no measurable time.
    • The run left no namespace in the bucket.
  • A cancelled earlier run (60a83ab) left 45 empty namespaces, because cancellation skips pytest_sessionfinish. Glue test databases already behave this way. These 45 were deleted by hand, and the weekly sweep removes such leftovers 7–14 days after creation.

🤖 Generated with Claude Code

laughingman7743 and others added 3 commits September 25, 2026 11:45
The S3 Tables tests shared one fixed namespace across the CI matrix. A
table another job dropped during a listing made the listing fail with
EntityNotFoundException, first in Glue GetTables and then in Athena
ListTableMetadata itself, even when filtered by table name, so
TestGlueMetadataClient.test_reads_s3_tables_catalog failed
intermittently, more often once the suites ran in parallel.

The session now creates a namespace named like its schema in the
configured table bucket and deletes it, with any table left in it, when
it finishes. AWS_ATHENA_S3_TABLES_NAMESPACE is no longer used.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
scripts/sweep_databases.py also sweeps AWS_ATHENA_S3_TABLES_CATALOG's
table bucket for namespaces named like a test schema and more than seven
days old, deleting their tables first, with the same inventory-first and
recheck-before-delete rules as databases. The sweep workflow sets the
catalog. The CloudFormation template no longer creates the fixed
namespace the tests used to share.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The namespace is created first in pytest_sessionstart, so a failure leaves
no Glue database or S3 rows behind; each pytest_sessionfinish step runs
even if an earlier one fails; the xdist controller, which runs no tests,
creates no namespace; and the table-bucket ARN uses the S3 Tables client's
region, looked up once. The Glue S3 Tables test no longer lists the
bucket's namespaces, which other sessions create and delete.

The sweep reports each result as it finishes, rejects a malformed
AWS_ATHENA_S3_TABLES_CATALOG before sweeping, and shares the test schema
pattern between databases and namespaces. The testing docs say each test
process creates a namespace and name both S3 Tables test files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@@ -13,16 +14,90 @@


def pytest_sessionstart(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.

Self-review round one (behavior and implementation): FINDINGS, repaired

Base 08425ffe3c04144cb643f88ab5fedb5d4e67eab3, head 883510b73b3c55993a5ff11e5e2bdda941855111. Pass: /code-review (high) on a frozen snapshot at 60a83ab. It covered the session hooks under pytest-xdist and pytest-rerunfailures, the SQLAlchemy suites' separate conftest, the sweep, the workflow, the template and the docs.

Repaired in 883510b:

  • tests/pyathena/test_glue.py: the S3 Tables test also listed every namespace in the bucket. That listing races with other sessions' namespace create/delete exactly as table listings did, so it is removed. list_databases stays covered in AwsDataCatalog.
  • tests/pyathena/conftest.py:16:
    • The namespace was created last in pytest_sessionstart. A failure then left the Glue database and S3 rows behind, because pytest skips pytest_sessionfinish after a failed sessionstart. It is now created first.
    • Each pytest_sessionfinish step now runs even if an earlier one fails.
    • The xdist controller, which runs no tests, no longer creates a namespace. Before this, pytest -n 8 made 9 namespaces per job.
    • The bucket ARN now comes from the S3 Tables client's own region, instead of AWS_DEFAULT_REGION while the client could resolve another region. The account lookup is cached.
  • scripts/sweep_databases.py:
    • Each sweep reports its counts as it finishes, so a failing namespace sweep no longer discards the database report.
    • A malformed AWS_ATHENA_S3_TABLES_CATALOG is rejected before sweeping, instead of raising IndexError.
    • The test schema pattern is shared between databases and namespaces.
  • docs/testing.md: says each test process creates a namespace (xdist workers, not the controller), and names both S3 Tables test files.

Deferred: the dry-run continue loop, which keeps the shape of sweep_databases.

Observed after CI: a superseded run (60a83ab, cancelled) left 45 empty namespaces, because cancellation kills the processes before pytest_sessionfinish. Glue test databases already behave this way. The successful run left none. The 45 were deleted by hand at the maintainer's request, and the weekly sweep covers such leftovers after this PR.

Comment thread docs/testing.md
```

The S3 Tables tests live in `tests/pyathena/sqlalchemy/test_base.py` and run under `just test pyathena`, not the SQLAlchemy compliance-suite commands.
Each test process creates its own namespace in the table bucket, named like its schema, and deletes it with any remaining tables at the end; with pytest-xdist that is each worker, not the controller.

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, callers, operations): CLEAN

Base 08425ffe3c04144cb643f88ab5fedb5d4e67eab3, head 883510b73b3c55993a5ff11e5e2bdda941855111.

Claims checked:

  • docs/testing.md:96-99:
    • One namespace per test process: _runs_tests skips only the xdist controller.
    • Deletion with remaining tables: _delete_s3tables_namespace.
    • Permissions: exactly the four s3tables calls used by the session.
    • Sweep after seven days: sweep_s3tables_namespaces.
  • The PR body's probe and CI claims:
    • Run 36089493551 on 883510b passed 15 of 15 test jobs (19 checks).
    • That run left no namespace; the 45 leftovers were all created 03:08–03:10, during the cancelled run.

Callers and operations:

  • AWS_ATHENA_S3_TABLES_NAMESPACE is no longer read. Branches based on older master still set it and still expect the fixed pyathena namespace, so they need a rebase before the stack update removes that namespace. This is noted in the PR body.
  • The template removal deletes pyathena on the next stack update, and that deletion fails while the namespace holds tables. Empty it first; the PR body gives the order.
  • The sweep workflow runs master's code, so namespace sweeping starts after merge, in the weekly run after the scheduled Test. A leftover is removed 7–14 days after creation.

CI duration: the sequential stages took 39.9 min, against 46.5 min before. Per-session namespaces add no measurable time, and the difference is within run-to-run variance.

… tables in the sweep

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# The namespace goes first: a failure here then leaves nothing behind, as
# pytest skips pytest_sessionfinish after a failed pytest_sessionstart.
if _runs_tests(session.config):
_create_s3tables_namespace()

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 (Codex, gpt-6-sol, read-only at 883510b) — P1: the namespace is created before the upload and database setup. If a later step fails, pytest skips pytest_sessionfinish, so the namespace leaks, and the comment claiming nothing is left behind is wrong.

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.

Fixed in 70e3082: the setup steps after namespace creation now run in a try, and a failure deletes the namespace before re-raising; the comment was corrected. Checked live without xdist, with a nonexistent staging bucket so _upload_rows fails: 883510b leaked pyathena_test_g67abp2xgn (deleted afterwards) and 70e3082 left only default and pyathena. With xdist the controller runs _upload_rows first and fails before any worker starts, so that path never created a namespace.

Comment thread scripts/sweep_databases.py Outdated
if (
not _eligible_namespace(current, cutoff)
or current["createdAt"] != namespace["createdAt"]
):

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 (Codex) — P1: after the get_namespace recheck, the tables and the namespace are deleted by name without checking identity again, so a namespace recreated under the same name in between could be hit.

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.

Not changed in the logic. Namespace names are pyathena_test_ plus 10 random characters per session, and the recheck already rejects any namespace younger than seven days, so a namespace recreated under the same name is not a realistic case. Another recheck before DeleteNamespace would not protect tables deleted before it anyway, and DeleteNamespace has no identity condition. 70e3082 documents this in the script header, whose previous wording (a recheck immediately before each deletion) described only the database 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.

Codex follow-up (session 01a0d6cb-e4bc-7e41-90cd-f2a07372b0cf, gpt-6-sol, read-only at 70e3082) keeps this at P1: random naming makes reuse unlikely but does not rule out a namespace recreated after GetNamespace.

Kept as a documented residual risk. The S3 Tables API gives no way to close the window: DeleteNamespace takes only tableBucketARN and namespace, with no identity condition. Recreating the name would take another session drawing the same 10 random characters (36^10) within the seconds between the recheck and the deletes, in the test bucket. This is left for the maintainer to decide.

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 9aa3f31, at the maintainer's request.

  • Identity. It is now the namespace ID (namespaceId) rather than createdAt, and eligibility requires an ID.
  • Tables. ListTables does not return namespaceId (checked live), so each table is read with GetTable. It is deleted only if its namespaceId matches, and DeleteTable receives that GetTable response's versionToken.
  • Namespace. The namespace ID is rechecked right before DeleteNamespace. A namespace that still holds tables cannot be deleted. What remains is an empty namespace recreated between that last recheck and DeleteNamespace, which takes only a name. The script header documents this.
  • Tests.
    • The 49 script tests pass.
    • Removing the table filter, the final recheck, or versionToken each makes a test fail, and the new tests fail against b77865d.
  • Live run. A run against the real API swept a namespace I created with two tables (cutoff patched, no CI running): eligible=1, deleted=1, skipped=0. Only default and pyathena remained.

Codex follow-up (session 01a0d71e-7b2f-77d1-ba1c-fd4980445d28, gpt-6-sol, read-only at 9aa3f31): CLEAN.

tableBucketARN=table_bucket_arn, namespace=name
)
for table in page["tables"]
]

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 (Codex) — P2: a NotFoundException from one delete_table goes to the outer handler, so the remaining tables and the namespace are skipped until the next 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.

Fixed in 70e3082: each delete_table suppresses NotFoundException, so the remaining tables and the namespace are still deleted. Added test_a_table_already_gone_does_not_stop_the_namespace (Stubber). It fails against 883510b and passes now; the scripts suite passes 47 tests.

_create_table(cursor)
except BaseException:
if runs_tests:
_delete_s3tables_namespace()

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 follow-up (Codex, session 01a0d6cb-e4bc-7e41-90cd-f2a07372b0cf, gpt-6-sol, read-only at 70e3082) — P2: if namespace cleanup fails while handling a setup failure, its exception masks the original setup error.

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.

Kept as is. Python chains the exceptions implicitly, so the setup error still appears in pytest's INTERNALERROR output, above "During handling of the above exception, another exception occurred". Suppressing the cleanup error instead would hide the leak, which only the weekly sweep would then catch.

laughingman7743 and others added 2 commits September 25, 2026 13:44
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 marked this pull request as ready for review September 25, 2026 06:31
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@laughingman7743
laughingman7743 merged commit 00828a3 into master Sep 25, 2026
19 checks passed
@laughingman7743
laughingman7743 deleted the fix/s3tables-per-run-namespace branch September 25, 2026 07:22
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