Repository navigation
fix(lint): check the three return values errcheck found in accounts and store tests - #1301
Merged
Merged
Conversation
…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
enabled auto-merge
November 4, 2026 03:30
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
mainat 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 threeerrcheckfindings, all in tests:module/services/accounts/code/pkg/adapters/rpc_policy_denied_test.go:95recordPolicyDenialreturn droppedmodule/services/store/code/migration_65_scope_settings_test.go:93defer runner.Close()uncheckeddefer func() { _, _ = runner.Close() }(), the idiom every othermigration_*_test.goin the package usesmodule/services/store/code/migration_65_scope_settings_test.go:164defer rows.Close()uncheckeddefer func() { _ = rows.Close() }(), asmigration_upgrade_test.godoesClassification: fix, at the place that owns the behaviour
No
.golangci.ymlexclusion 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=0withGOWORK=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 veton both edited packages: clean.Unblocks the v0.0.116 deploy-counter tag, which
RELEASE_GATES.mdonly allows on an all-greenmain.🤖 Generated with Claude Code