Review tail: lifecycle correctness, pluggability, pgx, SSRF redirect#3
Merged
Conversation
…t guard framework: - Start no longer holds a.mu across bundle/server/component startup; the lock now guards only state-flag transitions (starting/running), so a component calling IsRunning() from its own Start no longer deadlocks and observers don't block for the whole startup. Start-vs-Stop race is well-defined (Stop wins; second Start rejected). - readiness-delay goroutine is now cancellable via readinessStop; Stop closes it so a shutdown during ReadinessInitialDelay can no longer flip the service back to ready. - WithLogging/WithObservability/WithHealthRegistry options make the infrastructure managers injectable (the nil-check defaults were dead code); added Observability() accessor. postgresql: migrate lib/pq (maintenance mode) -> jackc/pgx/v5/stdlib; drop-in database/sql driver, pool settings unchanged. httpclient: AllowedHosts SSRF guard now also rejects redirects to disallowed hosts, not just the initial URL. Co-Authored-By: Claude Fable 5 <[email protected]>
Co-Authored-By: Claude Fable 5 <[email protected]>
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
1 participant
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.
Non-security tail from the framework review, plus the SSRF redirect residual noted in PR #2.
Correctness
IsRunning()from its ownStartwould deadlock; any observer blocked for the whole startup. The lock now guards only the state-flag transitions (starting/running); startup I/O runs unlocked. Start-vs-Stop is now well-defined: Stop wins, a second concurrent Start is rejected.time.SleepthenSetReady(true)with no cancellation could flip the service back to ready after Stop set it not-ready. Now cancellable via a stop channel that Stop closes.Pluggability
WithLogging,WithObservability,WithHealthRegistryoptions — the framework had dead nil-check default branches but no way to inject these, contradicting the 'pluggable' claim. Added the missingObservability()accessor.Modernization
database/sqldriver. Pool settings unchanged; lib/pq fully removed from the module graph.Security follow-up
Verified: full
-racesuite,golangci-lint(0 issues), all 7 examples build.🤖 Generated with Claude Code