fix: pid reuse after reboot, missing pid in instance.json, Windows non-ASCII locale - #38
Merged
Merged
Conversation
…ale on Windows initdb - Only treat an instance as running when the saved pid matches postmaster.pid and is a live postgres process, so a pid reused after reboot no longer blocks start or gets signalled by stop/drop (#37). - Make InstanceInfo.pid optional so instance.json without pid (or null/0) loads (#36). - On Windows, run initdb ourselves with --locale=C for new clusters so localized non-ASCII locale names don't fail (#35).
- Extract check_not_running (start's stale-pidfile handling) and kill_if_postgres (guarded SIGKILL fallback) so they can be tested. - Tests: is_postgres_process, running_pid, check_not_running (live, reused, garbage, missing pidfile), kill_if_postgres, wait_for_shutdown with a reused pid, health check without a live postmaster, instance.json with missing/null pid and round-trip, and init_data_dir running the real bundled initdb with --locale=C. - Run cargo test in the macOS CI job; the suite was never run in CI.
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.
Fixes #37, fixes #36, fixes #35.
#37: saved pid reused after reboot
Every running check was
kill -0 <saved pid>. After a reboot, macOS gave that pid to another process, sostartrefused forever. Worse,stop/dropcould fall back tokill -9on that unrelated process.is_process_runningis nowis_postgres_process: the pid must be alive and its executable must bepostgres(viaps -o comm=on Unix,QueryFullProcessImageNameWon Windows). pid 0 is rejected.running_pid(&info): the saved pid must also match<data_dir>/postmaster.pid.stop,drop,psql,install-extensionand the health check use it.startchecks the pid inpostmaster.pidrather than the saved one, and never removes the pidfile of a live postgres.#36: instance.json without pid
InstanceInfo.pidis nowOption<u32>, so a missingpidornullloads.0is treated as not running.#35: Windows non-ASCII locale
postgresql_embeddedgives no way to pass--localeto initdb. On Windows, when the data dir isn't initialized yet, pg0 now runs initdb itself with the crate's exact arguments plus--locale=C, andsetup()then skips its own initdb. Existing clusters and Linux/macOS behavior are unchanged.Testing
Unit tests (
cargo test, now run in the macOS CI job; the suite wasn't run in CI before):is_postgres_process: a live fakepostgrescounts; a live non-postgres pid (reused), pid 0, and an exited process don't.running_pid: a matching live postgres counts; a missing/0 pid, a reused pid, a pid that doesn't matchpostmaster.pid, and a missing pidfile don't.check_not_running(start's pre-check): refuses a live postmaster and keeps its pidfile; removes a reused-pid, garbage, or absent pidfile.kill_if_postgres: leaves a non-postgres process alive; kills postgres.wait_for_shutdown: not done while postgres is alive; done when the pid is reused and the pidfile is gone.instance.json: missingpid,nullpid, round trip.init_data_dir: runs the real bundledinitdb; assertslc_* = C, password auth, temp password file removed.Manual run on macOS: start → stop → remove pid → start ✅. pid=1 in both instance.json and postmaster.pid: info says not running, stop signals nothing, start recovers ✅. Double start still refuses ✅.
Windows: the Windows build and Windows SDK tests passed on the first commit, so
QueryFullProcessImageNameWcompiles and new clusters get initialized through the newinitdbpath.