Skip to content

storage/aws: accept a caller-owned MySQL DB - #1252

Closed
grempe wants to merge 1 commit into
transparency-dev:mainfrom
grempe:aws-provided-db
Closed

grempe wants to merge 1 commit into
transparency-dev:mainfrom
grempe:aws-provided-db

Conversation

@grempe

@grempe grempe commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Part of #1251: this adds option 1 there. Pools that Tessera opens itself are still never closed, as on GCP.

The AWS driver could only be given a DSN, and opened its own MySQL pools from it, for the sequencer and for the persistent antispam. It never closed them, so every in-process reopen of a log leaked the pools' goroutines and idle connections.

This change adds an optional DB *sql.DB to aws.Config and to AntispamOpts. It mirrors Config.SpannerClient on the GCP driver, and AntispamOpts.SpannerClient from #1054:

  • If DB is set, Tessera uses it, and never closes it. The pool settings (MaxOpenConns and MaxIdleConns) are ignored, and aws.Config also ignores its DSN.
  • The antispam checks the DB's database, using SELECT DATABASE(), against the database its dsn names, as storage/gcp/antispam: add optional SpannerClient to AntispamOpts #1054 does for its client. A DB connected to another database would otherwise keep a separate, incomplete record of the log's entries, and silently miss duplicates.
  • If DB is unset, behaviour is unchanged.

Testing

  • TestAntispam now runs the full append, follow and duplicate lookup cycle twice, with the antispam's own pool and with a provided DB.
  • New TestAntispamSharedDBWrongDatabase: a DB connected to another, real database is rejected. Without the check, NewAntispam accepted it.
  • New TestStorageUsesProvidedDB: the sequencer uses Config.DB, not its DSN, which the test points at an unreachable address. Without the change, the sequencer fails to connect.
  • Race tests: go test -race ./storage/aws/... -is_mysql_test_optional=false against MySQL 8.4 passes.
  • Lint: golangci-lint v2.13.2 reports 0 issues.
  • Reopening, measured as in AWS: MySQL pools are never closed, so reopening a log in one process leaks goroutines and connections #1251 on main at b2feb46: with caller-owned pools passed in, 5 reopens with MaxIdleConns 64 and the antispam left +4 goroutines and +6 MySQL connections, and the connections held at 5–6 from the first round. Without them, it was +36 and +28, growing every round.

This touches the same functions as #1244, but the two merge cleanly in either order. NewAntispam hands a provided DB straight to a new newAntispam, as newMySQLSequencerWithDB does for the sequencer, so the pool-settings lines that #1244 changes are left as they are. The new tests sit next to related tests, not at the ends of the files.

My first contribution, #1238, adds me to AUTHORS and CONTRIBUTORS under the individual CLA (commit 69ba233), so this PR doesn't repeat that change.


Reviewed and submitted by me, a human. The investigation, the fix and its testing were developed with the assistance of an AI coding agent.

@grempe

grempe commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main. NewAntispam now hands a provided DB to a new newAntispam rather than re-indenting the pool setup, so this merges cleanly with #1244 in either order. The description is updated.

The AWS driver opens its own MySQL pools, for the sequencer and for the
persistent antispam, and never closes them, so every in-process reopen
of a log leaks the pools' goroutines and idle connections.

Add an optional DB to Config and to AntispamOpts, mirroring the GCP
driver's SpannerClient options. If it is set, Tessera uses it and never
closes it, and ignores the pool settings, and Config also ignores its
DSN. The antispam checks that the DB is connected to the database its
dsn names, since a mismatch would silently break duplicate detection.
@grempe

grempe commented Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main, and moved the new AntispamOpts.DB field above PushbackMaxOutstanding, so this still merges cleanly with #1244 in either order now that #1244 documents the pool settings next to it.

@mhutchinson

Copy link
Copy Markdown
Contributor

Hi @grempe! IMO the root bug would be better fixed by having Tessera clean up the resources it opened (#1111). The approach in this PR opens up the API considerably, and doesn't resolve the issue for anyone that continues using a DSN.

If the issue was resolved with proper termination for any opened resources, would that be sufficient for you? Or is allowing the DB to be injected what you're actually looking for?

@grempe

grempe commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! Yes, proper termination is enough for us. Our log passes only a DSN, and I went with injection only because it mirrored the GCP driver's SpannerClient and was the smaller change. We came to this through our own log's stress testing, which reopens logs in one process and ran into MySQL's connection limit.

I've opened #1276, which takes the termination approach for all three drivers: each appender closes what it opened once its context is done and its background jobs have returned, construction that fails closes what it opened, and the antispams get a Close method. It fixes #1251 and is part of #1111, as it doesn't add a way to wait for the cleanup. It's stress-tested on all three drivers, with the details in its description. So I'm closing this one in favour of it.

@grempe grempe closed this Nov 9, 2026
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.

2 participants