Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
🦋 Changeset detectedLatest commit: 65726e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
bflad
force-pushed
the
bflad/age-1630-bug-do-not-create-domains-on-transient-lookup-errors
branch
from
2b7c319 to
6c4d7de
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| func TestVerifyCustomDomain_TransientDBError(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| const orgID = "org-transient" | ||
| const domain = "transient.example.com" | ||
| ctx, ti := newTestInstance(t, orgID, domain) | ||
| activity := newActivity(t, ti) | ||
|
|
||
| // Close the pool to simulate a transient DB error during GetCustomDomainByDomain | ||
| ti.conn.Close() | ||
|
|
||
| err := activity.Do(ctx, activities.VerifyCustomDomainArgs{ | ||
| OrgID: orgID, | ||
| Domain: domain, | ||
| CreatedBy: urn.NewPrincipal(urn.PrincipalTypeUser, "test-user"), | ||
| }) | ||
| require.Error(t, err) | ||
|
|
||
| // The error should NOT be about domain creation — it should be the lookup failure. | ||
| // Before the fix, this would have attempted to create a domain. | ||
| assert.NotContains(t, err.Error(), "error creating custom domain") | ||
| } |
There was a problem hiding this comment.
🚩 Test for transient DB error exercises Begin() failure, not the new switch default case
In TestVerifyCustomDomain_TransientDBError (line 155-176), ti.conn.Close() closes the entire connection pool. When activity.Do runs, d.db.Begin(ctx) at verify_custom_domain.go:68 fails immediately because the pool is closed, returning the error "failed to access custom domains" before reaching the new switch at line 77. The test assertion NotContains(t, err.Error(), "error creating custom domain") passes, but it doesn't actually exercise the new default branch at line 102-103. To truly test a transient error during GetCustomDomainByDomain, the test would need to inject a failure after Begin succeeds (e.g., via a custom DBTX wrapper that fails on the query). The sanity-check test TestGetCustomDomainByDomain_ReturnsErrNoRows at line 349-356 is a good addition that validates the ErrNoRows assumption, but the transient error scenario's specific code path remains untested.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Custom dbtx wrapper, eh? Not sure the juice is worth the squeeze right now, its bad enough this had like zero testing before.
bflad
force-pushed
the
bflad/age-1630-bug-do-not-create-domains-on-transient-lookup-errors
branch
from
6c4d7de to
4f3b70b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Rebasing and fixing the things Devin found. |
https://linear.app/speakeasy/issue/AGE-1630/bug-do-not-create-domains-on-transient-lookup-errors Previously, any error from `GetCustomDomainByDomain` triggered domain creation. Now only `pgx.ErrNoRows` does — other errors (e.g. connection failures) return immediately instead of creating spurious domain records. Introduces a `dns.Resolver` interface whose method signatures match `*net.Resolver`, enabling deterministic DNS testing without network calls. `MockResolver` exposes a `Resolver()` method that returns a real `*net.Resolver` whose `Dial` function routes queries through configurable mock functions via in-memory `net.Pipe` connections with TCP-framed `dnsmessage` wire protocol. This mirrors the approach used internally by Go's own `net` package tests (`fakeDNSServer` in `dnsclient_unix_test.go`). An alternative using `miekg/dns` to spin up a real localhost UDP server was considered — it would simplify the wire format handling but adds an external dependency and real sockets per test; the stdlib-only dial approach was chosen to avoid both. Adds lint rules via `forbidigo` banning direct `net.Lookup*` calls in favor of `dns.NewNetResolver()`.
bflad
force-pushed
the
bflad/age-1630-bug-do-not-create-domains-on-transient-lookup-errors
branch
from
4f3b70b to
65726e5
Compare
bflad
deleted the
bflad/age-1630-bug-do-not-create-domains-on-transient-lookup-errors
branch
2 participants
Previously, any error from
GetCustomDomainByDomaintriggered domain creation. Now onlypgx.ErrNoRowsdoes — other errors (e.g. connection failures) return immediately instead of creating spurious domain records.Introduces a
dns.Resolverinterface whose method signatures match*net.Resolver, enabling deterministic DNS testing without network calls.MockResolverexposes aResolver()method that returns a real*net.ResolverwhoseDialfunction routes queries through configurable mock functions via in-memorynet.Pipeconnections with TCP-frameddnsmessagewire protocol. This mirrors the approach used internally by Go's ownnetpackage tests (fakeDNSServerindnsclient_unix_test.go). An alternative usingmiekg/dnsto spin up a real localhost UDP server was considered — it would simplify the wire format handling but adds an external dependency and real sockets per test; the stdlib-only dial approach was chosen to avoid both.Adds lint rules via
forbidigobanning directnet.Lookup*calls in favor ofdns.NewNetResolver().Linear: https://linear.app/speakeasy/issue/AGE-1630/bug-do-not-create-domains-on-transient-lookup-errors