Serialize PUN lifecycle operations and prevent concurrent startup and shutdown races - #5809
Open
GlazerMann wants to merge 35 commits into
Open
GlazerMann wants to merge 35 commits into
GlazerMann wants to merge 35 commits into
Conversation
Added logic to wait for the old PUN socket to shut down before starting a new one to prevent EADDRINUSE errors.
GlazerMann
marked this pull request as draft
September 18, 2026 14:35
GlazerMann
marked this pull request as ready for review
September 18, 2026 14:52
GlazerMann
marked this pull request as draft
September 18, 2026 16:59
GlazerMann
marked this pull request as ready for review
September 18, 2026 17:44
This was referenced Sep 18, 2026
6 tasks
Handle missing PUN config error when acquiring restart lock.
GlazerMann
marked this pull request as draft
September 19, 2026 12:23
Fix typo in the variable name for download file size limit.
Replace backticks with Open3.capture2e for better error handling when sending SIGTERM to the nginx process.
GlazerMann
marked this pull request as ready for review
September 19, 2026 17:21
GlazerMann
marked this pull request as draft
September 19, 2026 17:27
Clarify comments regarding PID path cleanup process.
GlazerMann
marked this pull request as ready for review
September 19, 2026 18:00
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.
I recommend merging #5816 first, then #5815, followed by #5813, and finally #5809. #5816 fixes the asynchronous FilesTest cleanup race that can otherwise make unrelated CI runs fail; #5815 fixes the MutationObserver timing race in the accessibility tests; and #5813 fixes the detached-DOM-node race in BatchConnect tests. Once those test-infrastructure races are merged, #5809 can be rebased and rerun against a more stable master, making any remaining failures much easier to attribute to the PUN lifecycle changes themselves.
nginx_stagehas several operations that can start, stop, regenerate, restart, or remove the same user's per-user NGINX (PUN). These operations were not serialized, allowing concurrent requests to race over the PUN configuration, PID file, andpassenger.sock.One observed failure occurs when
nginx_stage appstops a PUN and immediately starts its replacement.nginx -s stopcan return before the old process has finished removing its Unix socket, causing the replacement process to fail with:Concurrent initial PUN requests can also race. Multiple Apache requests can observe that
passenger.sockis absent and invokenginx_stage punfor the same user at nearly the same time, causing duplicate initialization, pre-hook execution, configuration generation, and nginx startup attempts.This change adds a common per-user lifecycle lock and keeps the relevant state checks and shutdown completion waits inside that serialized lifecycle.
Serialize PUN lifecycle operations
NginxStage::Generatornow provides a common per-user lifecycle lock used by operations that can start, stop, replace, or remove a PUN:nginx_stage punnginx_stage appnginx_stage nginxnginx_stage nginx_cleanThe lock:
flock;The lock file is intentionally separate from the PUN configuration. Some lifecycle paths remove or recreate the configuration file; locking the configuration inode itself could allow concurrent processes to synchronize on different old and new inodes.
Prevent duplicate PUN initialization
PunConfigGeneratornow holds the lifecycle lock across the complete initialization sequence.After acquiring the lock, it checks whether the PUN is already running. A running PUN requires both:
If another request has already completed startup, the later
nginx_stage puninvocation returns without repeating configuration generation, pre-hooks, or nginx startup.Performing this check after acquiring the lifecycle lock closes the scheduling window where two requests can both decide that the PUN is absent before either startup completes.
--skip-nginxremains config-generation-only behavior and is not suppressed by the running-PUN check.Wait for shutdown before replacement startup
Before
AppConfigGeneratorstarts a replacement PUN, it waits for the previouspassenger.sockpathname to disappear while still holding the lifecycle lock.The socket wait:
When a PID file exists, the replacement sequence is:
nginx -s stop;passenger.sockto disappear;If no PID file remains, the same lock is held while checking for a lingering socket before startup.
Keep shutdown serialized through socket cleanup
Lifecycle serialization now extends through asynchronous socket cleanup for other shutdown paths as well.
For
nginx_stage nginx:stopwaits for the PUN socket to disappear before releasing the lifecycle lock;quitdoes the same;reloaddo not wait for socket removal.For
nginx_stage nginx_clean:SIGTERMwhile holding the lifecycle lock and waits for socket removal when the signal succeeds.This prevents another lifecycle operation from starting a replacement PUN during the interval between signaling the old process and completion of its Unix-socket cleanup.
Failure behavior
If another lifecycle operation holds the lock longer than the configured timeout, the command fails rather than proceeding concurrently.
If a previous PUN socket remains beyond the shutdown timeout, replacement startup is not attempted.
nginx_stage appalso verifies the PUN configuration after acquiring the lifecycle lock and fails before invoking nginx if another operation removed it.Tests
Regression coverage includes:
stopandquitshutdown completion;nginx_cleanshutdown completion;--skip-nginxconfiguration generation.Together these changes serialize PUN lifecycle operations, prevent replacement startup while an old Unix socket is still being removed, and prevent duplicate concurrent PUN initialization.