fix: always release canvas leadership when a demotion handler throws - #8
Merged
Merged
Conversation
- leader: wrap demotion handlers in async so a synchronous throw becomes a rejection; previously it escaped Promise.allSettled and skipped the advisory unlock and client release (typescript:S4123) - Dockerfile: keep the entrypoint root-owned so the runtime node user cannot modify it (docker:S6504) - sonar: exclude Convex codegen output convex/_generated/** from analysis (javascript:S7724 on generated files) Powered by human calories and mass GPU cycles.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
0 New Issues
2 Fixed Issues
0 Accepted Issues
No data about coverage (0.00% Estimated after merge)
Merged
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.
Note
🤖 Claude Opus 5.5 responding on behalf of beastyrabbit
SonarQube's main analysis flagged a critical issue in the canvas backend's leadership handoff. If a demotion handler threw synchronously, the error skipped the cleanup that releases the PostgreSQL advisory lock and the retained connection. Another replica could then not take over the canvas until that connection died. Handler failures are now always collected and reported together, and the lock is always released.
Two smaller findings are fixed as well. The frontend image's entrypoint is now root-owned, so the runtime
nodeuser can no longer modify it. Convex codegen output (convex/_generated/**) is excluded from SonarQube analysis because it isn't hand-written source.Verification
stop()reject with anAggregateErrorand leaves no advisory lock held. It fails against the previousleader.ts.root:root 755, not writable bynode, and the app answers with HTTP 200.The
disposalFinishedcheck inlifecycle.test.tsfails intermittently on unmodifiedmainas well. That timing flake is not addressed here.The remaining SonarQube findings were triaged separately. The hard-coded
moddrop:moddropURLs are the loopback-only dev database default; production readsDATABASE_URLfrom the cluster secret. Thehttp://bases only parse URLs. Thevmcall executes the repo's own build output in a verification script. None of these warrant code changes. The cognitive-complexity findings are left for a dedicated refactor.Somewhere a GPU is overheating so I don't have to think.