Skip to content

fix(lint): check the three return values errcheck found in accounts and store tests - #1301

Merged
antoinetoussaint-byte merged 1 commit into
mainfrom
fix/go-lint-accounts-store
Nov 4, 2026
Merged

antoinetoussaint-byte merged 1 commit into
mainfrom
fix/go-lint-accounts-store

Conversation

@antoinetoussaint-byte

Copy link
Copy Markdown
Contributor

What this fixes

main at 061b7ad is red on Go lint (module/services/accounts/code) and Go lint (module/services/store/code). The job introduced in #1297 was clean at its own head; #1298 (accounts) and #1295 (store) merged test code written before that job existed. golangci-lint v2.14.0 reports three errcheck findings, all in tests:

File Finding Fix
module/services/accounts/code/pkg/adapters/rpc_policy_denied_test.go:95 recordPolicyDenial return dropped assert it, as lines 77–79 and 114 of the same file already do: the function passes the refusal through unchanged
module/services/store/code/migration_65_scope_settings_test.go:93 defer runner.Close() unchecked defer func() { _, _ = runner.Close() }(), the idiom every other migration_*_test.go in the package uses
module/services/store/code/migration_65_scope_settings_test.go:164 defer rows.Close() unchecked defer func() { _ = rows.Close() }(), as migration_upgrade_test.go does

Classification: fix, at the place that owns the behaviour

No .golangci.yml exclusion widened, no //nolint. None of the three is a false positive. Three lines changed, nothing refactored.

Verified

  • golangci-lint run --path-mode=abs --max-issues-per-linter=0 --max-same-issues=0 with GOWORK=off, golangci-lint v2.14.0 built with go1.27.3 (the CI job's exact version and install mode), in both module directories: 0 issues each.
  • go vet on both edited packages: clean.
  • Not run locally: the Postgres-backed store migration test and the accounts suite; the change in both is a deferred-close idiom and an added equality assertion on a value the sibling tests already assert, and CI runs them.

Unblocks the v0.0.116 deploy-counter tag, which RELEASE_GATES.md only allows on an all-green main.

🤖 Generated with Claude Code

…nd store tests

The Go lint job added in #1297 was clean at its own head, but #1298 (accounts)
and #1295's base (store) landed test code written before that job existed.
golangci-lint v2.14.0 reports three errcheck findings on main:

- accounts rpc_policy_denied_test.go:95 dropped recordPolicyDenial's return.
  The function passes the refusal through, and the sibling tests in the same
  file assert exactly that, so this one now asserts it too.
- store migration_65_scope_settings_test.go:93 and :164 deferred runner.Close
  and rows.Close bare; both now use the idiom every other migration test in
  the package already uses.

No exclusion, no nolint. Both modules report 0 issues under the CI command.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@antoinetoussaint-byte
antoinetoussaint-byte added this pull request to the merge queue Nov 4, 2026
Merged via the queue into main with commit fdb228c Nov 4, 2026
33 checks passed
antoinetoussaint-byte added a commit that referenced this pull request Nov 4, 2026
…iting for a backend to come back

Two separate things the first CI run and the first suite run found.

errcheck: the migration-66 test dropped (*migrate.Migrate).Close's two
returns. Fixed in the shape #1301 used for the same call in the migration-65
test, which landed while this branch was open and is why CI reported only this
one.

TestRequestConnectionCarriesNoBindingAfterRelease released the poisoned
connection to the shared pool and then waited five seconds for that backend's
PID to reappear idle, which needs the reset to have cleaned the session AND
nothing else in the package to be holding every request connection for the
length of the wait. The second is not a property of this code, and a failure
could not say which half it was. It now drives the pool's own AfterRelease
judgement on the connection it poisoned: the verdict the pool acts on, plus the
session the next borrower would be handed, carrying none of the four names and
back to app_tenant. The poison is set on all four, because inside the
compatibility window a predicate falls through to the retired name when the
live one is empty.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
crisfiorini-oss pushed a commit to crisfiorini-oss/module-saas-starter that referenced this pull request Nov 4, 2026
…fuse a catalog that cannot be served (codefly-dev#1303)

* fix(store): make the scope-setting rename deployable, and refuse a catalog that cannot be served

Migration 65 renamed the two RLS scope settings onto the platform Postgres
library's names in one step, which is not a deployable move: the catalog and
the binary that binds it ship separately, and whichever loses the race reads an
unset setting — zero rows on every scoped SELECT, a WITH CHECK failure on every
INSERT, and nothing raised. A rolling update puts pre-rename pods against the
renamed catalog; rolling the image back leaves the whole fleet there.

Migration 66 opens a compatibility window: every policy predicate and routine
body reads the library's name with the host's retired one behind it, and the
host binds both generations, so one image serves a catalog from either side of
the rename. It is catalog-driven rather than generated, and it ships as
tools/rls-scope-settings-transition.sql so a consumer whose own migrations
wrote policies on the retired names reaches the same state; the migration is
that file byte for byte, held in step by a test.

Accounts refuses to start against a database whose policies read only the
retired names: the two pools service-postgres owns bind the library's names and
take no more, so no binding could make such a catalog serve and a started
process would report healthy while answering nothing through them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(accounts): bind both generations everywhere one is bound, and judge a released connection on both

The effect-receipts tenant scope bound the two library names with its own
statement; it now uses the one request binding, so no path is left behind when
the compatibility window closes. The release reset reads back all four names:
inside the window a predicate falls through to the retired name when the live
one is empty, so a session-level binding on either generation hands the next
borrower a scope.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(accounts): say why the catalog refusal is fatal for the served process and not for the tooling store

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(store): say what the migration that closes the window actually is

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* refactor(accounts): give the catalog check's two helpers names that cannot collide in pkg/infra

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(accounts): pin the users_select predicate as the compatibility window renders it

The pinned text is the catalog's own rendering, so opening the window moves it:
users_select now reads the library's user setting and falls through to the
host's retired one. Owner, SECURITY DEFINER, the app_tenant EXECUTE grant and
the absence of a PUBLIC grant are unchanged. Closing the window moves it back,
which is why the text is pinned rather than asserted to mention a setting.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(accounts,store): check the migrate runner's returns, and stop waiting for a backend to come back

Two separate things the first CI run and the first suite run found.

errcheck: the migration-66 test dropped (*migrate.Migrate).Close's two
returns. Fixed in the shape codefly-dev#1301 used for the same call in the migration-65
test, which landed while this branch was open and is why CI reported only this
one.

TestRequestConnectionCarriesNoBindingAfterRelease released the poisoned
connection to the shared pool and then waited five seconds for that backend's
PID to reappear idle, which needs the reset to have cleaned the session AND
nothing else in the package to be holding every request connection for the
length of the wait. The second is not a property of this code, and a failure
could not say which half it was. It now drives the pool's own AfterRelease
judgement on the connection it poisoned: the verdict the pool acts on, plus the
session the next borrower would be handed, carrying none of the four names and
back to app_tenant. The poison is set on all four, because inside the
compatibility window a predicate falls through to the retired name when the
live one is empty.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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