fix(connect): serialize nginx reload requests - #2066
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. Walkthrough
ChangesNginx reload coordination
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change serializes concurrent API-triggered nginx reloads while preserving sequential behavior and existing results; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c42e51082
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| if (activeReload) { | ||
| this.logger.debug('Nginx reload already in progress; waiting for it to complete'); | ||
| return activeReload; |
There was a problem hiding this comment.
When an enable and disable transition overlap, the second handler updates connect.config.wanaccess (wan-access.events.ts:18-29) but this branch merely joins the reload already in progress. Because rc.nginx reads that setting during execution (rc-nginx.modification.ts:51-54), a reload that already consumed the earlier value can finish successfully while the later value is never applied—for example, leaving WAN access enabled after a disable request. Preserve serialization by scheduling a trailing reload after the active command rather than coalescing the request into it.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2066 +/- ##
==========================================
+ Coverage 53.13% 53.17% +0.03%
==========================================
Files 1041 1041
Lines 72425 72476 +51
Branches 8354 8363 +9
==========================================
+ Hits 38482 38537 +55
+ Misses 33816 33812 -4
Partials 127 127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🔄 PR Merged - Plugin Redirected to StagingThis PR has been merged and the preview plugin has been updated to redirect to the staging version. For users testing this PR:
Staging URL: Thank you for testing! 🚀 |
🤖 I have created a release *beep* *boop* --- ## [4.37.3](v4.37.2...v4.37.3) (2026-08-25) ### Bug Fixes * **connect:** serialize nginx reload requests ([#2066](#2066)) ([d65341d](d65341d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary
Concurrent Connect WAN-access transitions could start overlapping
rc.nginxrebuilds. This change coalesces in-process reload requests so only one rebuild runs at a time.Why This Exists
The Unraid 7.3.2
rc.nginxscript rebuilds the sharedservers.conffile with truncating and append writes. Connect WAN-access event handlers run asynchronously, so overlapping reloads can write duplicate server blocks, makenginx -tfail, and make the local WebGUI unreachable.Resolution
NginxServicenow stores the active reload promise at module scope. Concurrent callers wait for that promise and receive the same result. A later caller starts a new reload after the active command completes.This shape protects all
NginxServiceinstances in the API process and does not change the existing success or failure result.Reviewer Considerations
finally.rc.nginxprocess from another OS process.rc.nginxscript or the event sources.Behavior Changes
Concurrent API-triggered nginx reload requests now run one
rc.nginx reloadcommand. Sequential requests after completion still run normally.Implementation Summary
api/src/unraid-api/nginx/nginx.service.ts.Verification
pnpm --filter ./api exec vitest run src/unraid-api/nginx/nginx.service.spec.ts— 2 passed.pnpm --filter ./api exec vitest run src/unraid-api/nginx src/unraid-api/unraid-file-modifier— 44 passed.pnpm --filter ./api type-check— passed.@unraid/libvirtmodule is unavailable in this environment.Audit Dependenciesfailed on existing transitive dependency advisories, includingpostcssandtar. This PR changes no dependency files.Risk
Low for API-triggered reloads. The remaining risk is limited to reloads started outside this API process.