Skip to content

Stop own writers gracefully for full database maintenance - #5191

Merged
7 commits merged into
mainfrom
work/lp2437b-stopped-writer-hold
Oct 28, 2026
Merged

7 commits merged into
mainfrom
work/lp2437b-stopped-writer-hold

Conversation

@shiny-code-app

@shiny-code-app shiny-code-app Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Frozen advisory-lock holders caused two full-backup admissions to refuse. Gracefully stop all six Launchplane writers and require exact stopped-state, restart-policy and zero-client readback before admitting the backup. Existing schema/deploy/admission locks and executor observations close admission first; only own no-XID queries blocked directly by the keeper are canceled after that original's persisted stop intent. Kernel process handles bind TERM to the exact original, with no delayed Docker stop or SIGKILL escalation. Three Odoo claims now honor the existing maintenance admission and executor census.

Before reopening writers, the matching image runs real service startup and HTTP health against the actual own database with both pools read-only, schedulers omitted, and networking disabled. It consumes the existing bootstrap and a host-namespace PostgreSQL socket mount in place. The mount's device/inode must agree with the actual container socket; a restart can rebind only within the original container and database system identifier. A durable database/health proof permits restoration of every original restart policy before bounded API/worker waits, so a late graceful exit can recover through Docker. Both readback samples require a live, non-restarting process, with the same PID and start time across two seconds. A disappearing original during recovery is settled through the existing exact identity/policy checks; initial signaling stays strict.

Full snapshots, private backup, actual isolated restore, exact rehearsal and retirement verification remain mandatory. No API runs against the restored copy. This PR performs no retirement and creates no credential or grant. Part of #2437; its operational finish line remains open in #5153.

Validation: 226 affected safety/bootstrap/schema/migration/Odoo/outbox tests, 23 real PostgreSQL maintenance tests including before/after read-only startup and claim admission, Ruff, format, mypy (1,900 files) and role-word checks. Inert native fixtures reproduced the socket-mount behavior, API graceful exit status and late-exit automatic restart. The final10-test stop module passed after deleting a duplicate mock-only regression. Independent review and final-head CI are recorded separately before Ready. Native PyCharm returned UNKNOWN/native_timeout for the13-file selection at prior head00b3550f6; the worker exited and its private snapshot was removed. The final recovery correction is not re-inspected under the continuing exception. The continuing Director exception is https://github.com/cbusillo/direction/issues/98#issuecomment-6428617448 and the ordinary-use sample is recorded on cbusillo/codex-skills#2923. No clean inspection is claimed. Actual deployed-image qualification is required after landing.

Removed: cgroup freezer control, nftables/API thaw-refreeze phase and second watchdog, frozen-client backend termination, the unused maintenance health-query branch and cached lifespan state, repeated heavyweight verification after the durable safety proof, write-only rearm progress and duplicate policy updates. Net lines: +112 (1,426 added, 1,314 removed). Production code is194 lines smaller; growth is behavior regressions for reproduced shutdown and recovery failures.

CI repair: Closes #5195. The base-added accepted-recipient fixture now derives monitor time from persisted inventory as well as its receipt and controlled clock. A delayed synthetic check fails before and passes after. Production freshness rules are unchanged. This repairs both failed train candidate paths. Final integrated head: 59348d0. Another-provider review found no defects; 25 module tests and the real PostgreSQL wrapper pass, plus Ruff/format/mypy. Maintenance bytes remain identical to a4b0218. Updated aggregate: Removed: the stale fixture clock assumption. Net lines: +120; production-code reduction remains 194 lines.

Stop exact own writer processes before full backup and verify matching-image API startup read-only through the existing PostgreSQL socket. Preserve keeper locks, snapshots, isolated rehearsal and retirement gates.

Client-impact: none
Exercise the real keeper release through PostgreSQL so connection lifetime includes every held advisory scope.

Client-impact: none
@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Independent source review of 23ec510 completed using default selection: Anthropic Claude Opus 5.5, confirmed by its CLI. The draft remains held from Ready.

Reproduced before qualification: actual main-thread Uvicorn finishes graceful shutdown with status 143; the proposed proc-root socket bind fails, while the host-namespace merged directory matches the actual socket inode and passes an inert read-only mount probe. A separate inert native pidfd-TERM fixture proved that restoring the original restart policy before a delayed clean exit causes Docker to restart the same container with a new process. All fixtures were removed. No live service or Client data changed.

The review's claim that VeriReel capture is absent from the executor census was refuted with the actual observation function and claim lock. Other admission gaps are real, including completed-only provider calls. A fresh read-only agent completed the whole-kind audit; an outside design look is checking reuse of the existing shared provider-dispatch key and existing claim boundaries, plus recovery/socket corrections. This is progress evidence, not a weighed Ready handoff. Current-head CI is green; source changes, tests, review disposition and native IDE remain before Ready.

@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review result: weighed
Head: a4b0218

Review disposition for a4b0218f3bca5c69d02d3886517492d22664145d.

Default-selection review of record: Anthropic claude-opus-5-5, confirmed by the CLI, independent of the OpenAI author. The final source review reports no HIGH, MEDIUM or LOW behavior defect. Two fresh-agent/outside-design looks preceded the corresponding redesigns after repeated review rounds. No source change follows this reviewed head.

Acted on: qualified API graceful exit; host-namespace socket mounting and same-container/system-identifier rebind; rearm every original policy before waits; transient health deadlines with real startup failure still blocked; cancellation only after a fresh durable stop intent; three missing Odoo admission claims and census; removal of freezer-era health surface; disappearance during recovery signaling; positive, non-restarting stable-process readback in both samples. Removed duplicate heavyweight verification, policy updates, write-only rearm state and a duplicate mock-only regression.

Reproduced: the real keeper release first failed with ResourceClosedError and passed after its connection lifetime fix; the restart-backoff regression failed before its source correction and passed after; the false running readback regression failed before its source fix. The final native-primitive tests cover ESRCH, disappearing cgroup, second-inspect exit and strict foreign-membership refusal. Full affected modules226 passed, real PostgreSQL23 passed, and the final10-test stop module passed after deleting the duplicate. Ruff/format, terminology, mypy1900 and generated API drift passed.

Weighed notes: the three Odoo operation families now block the existing bounded self-deploy drain while running, and new claims defer during drain/maintenance. That prevents a normal self-deploy from killing those operations after Docker's default grace. It adds no Client action or authority. The bounded systemd attempt can still time out on a slow abort; restored policies, saved completed-resume progress and supported manual release preserve recovery. No additional mechanism is added. Broad shared provider locks were declined after the fresh design review showed normal self-deploy behavior changes and no data-safety need. Private socket/plan fields remain custody evidence. A final live reread sweep was declined because historical released evidence is not a new live-health assertion.

The proposed deletion of the explicit HTTP200 check was a non-defect: an inert loopback probe demonstrated urllib returns201 without HTTPError. Keeping the required200 check is correct. The final review's document line-wrap nit is cosmetic; its prose matches the code and no behavior finding remains.

Inspection limitation: native PyCharm at prior00b3550f6 returned UNKNOWN/native_timeout for the13-file selection with settled cleanup. Final correction is not re-inspected under the continuing Director exception https://github.com/cbusillo/direction/issues/98#issuecomment-6428617448 . Ordinary-use receipt: cbusillo/codex-skills#2923 (comment) . No clean IDE verdict is claimed. Two seconds of stable running is bounded startup readback, not proof against every later crash.

Current-head CI/train, actual deployed-image qualification and the entire new Q777 backup/actual isolated restore/rehearsal sequence remain required. No new real hold, backup or retirement was executed during these corrections. No API runs against the restored clone. #2437 and #5153 remain open for that operational finish line.

Removed: freezer/firewall/thaw/refreeze/second-watchdog machinery, backend termination, unused maintenance health branch and cached state, repeated verification and policy writes, write-only rearm progress. Net lines: +112 (1426 added,1314 removed), including194 fewer production-code lines and regression coverage for reproduced failures.

Final raw reviewer report follows; this is weighed evidence, not a merge approval or Director decision.

Verdict: I found no HIGH, MEDIUM or LOW behaviour defect in a4b0218f3. The corrections since 00b3550f6 resolve all of the previous round's findings. They add no authority, grant, credential, Client action or Director-owned decision. I checked this by reading the source; I did not run anything.

Corrections checked:
1. Signal race during resume (database_maintenance_host.py:632-650).
   - Only resume_original catches ProcessLookupError, FileNotFoundError and RecoveryRequired.
   - It then inspects again, and re-raises if the exact original (running_process, same Pid and StartedAt) is still live. Otherwise it hands off to wait_for_exit, which still refuses an image or project change, and a restart under a policy other than the recorded one.
   - PermissionError and other OSErrors are not caught.
   - stop_services (line 584) and prepare's check_only probe (line 1171) call signal_original with no handler, so both stay strict.
   - signal_original still binds the pidfd before it inspects and reads the cgroup. A reaped or replaced process therefore gets ESRCH or a refusal, never a signal. A restarted replacement has a new StartedAt and fails the identity check. Sending TERM to a zombie is a harmless no-op, and wait_for_exit then takes over.
   - The four subtests (pid_gone, membership_gone, restarting, foreign_membership) run the real signal_original. They assert pidfd_send_signal is never called, and that a foreign membership with the exact original still running stays RecoveryRequired.
2. Readback (lines 653-664).
   - Both samples use healthy_process (Running, not Restarting, Pid>0, healthy or no healthcheck), and Pid and StartedAt must match across the 2 s interval.
   - Permanent backoff with Pid 0 now runs out the 120 s deadline as a transient RuntimeError, and docker start is never called on a container that reports Running=true.
3. Removals.
   - rearmed_containers and the duplicate policy update/readback after resume are gone.
   - finish_release still sets resume_started, then calls rearm_original_policies (full-dict readback for every original), and only then calls any resume_original or wait (lines 923-925).
   - complete_service_verification still runs before that and requires policy "no", every original stopped, and health_passed. So the order is unchanged: strict full hold, verify, then rearm.

All sibling process-state readers checked:
- Initial API inventory (line 1090) and project inventory (line 1135) use running_process.
- refresh_verification_socket (line 268) uses running_process.
  - If PostgreSQL restarts between its inspect and the /proc stat, the result is an uncaught OSError. That is transient.
  - Even a reused PID could only match a candidate that is the same physical socket inode.
- check_originals(stopped), complete_service_verification and the operator-controller wait read only Running. Each of them runs where the restart policy is "no", so Restarting cannot occur there.
- wait_for_exit treats Running=false as an exit and Pid/StartedAt drift as a restart. During backoff it returns the Restarting observation only after resume_started and with the original policy, and readback then decides.
- Moby sets Restarting under its lock when it handles an exit, so a Running=false window before an automatic restart is not expected. I found no realistic double-start trigger.
- No path sends a signal to a replacement process.

Accepted residuals (not defects):
- If dockerd lags behind the shim's reap across the second inspect, the original ProcessLookupError or FileNotFoundError is re-raised. That is transient and costs one of the three automatic attempts. A zombie is still listed in its cgroup on v2, so I found no realistic trigger for a terminal RecoveryRequired.
- Existing limits are unchanged: the abort-path budget can still exceed TimeoutStartSec, TERM is re-sent to a crash-before-signal original, and there is no final re-read sweep after release.

Tests, pins and gaps:
- No new literal, wording or config pins. The policy version now comes from LaunchplaneAuthzPolicy().schema_version, the timeout unit mocks parse_authz_policy_toml, and the scheduler check asserts the set is not empty.
- The startup-failure subtest (False, False) keeps RecoveryRequired with VERIFICATION_BLOCKED_EVENT, so making that path transient by mistake would now fail.
- test_readback_requires_two_healthy_samples_of_the_same_live_process asserts on the last two inspect reads. That catches the old one-sample code. It survives an added trailing check and does not pin a helper name.
- test_socket_refresh_survives_the_same_database_process_restart still asserts the private verification_socket["pid"]. It was accepted earlier as an evidence field. The device/inode match and the different-cluster refusal are the behaviour assertions.
- test_a_late_exit_cannot_be_reported_as_resumed still mocks signal_original. Its behaviour assertion is that docker start is never called before the exit.

HTTP status check: declining that deletion is correct. urllib's default handler accepts every 2xx, so a 201 reaches the explicit 200 check, and the 200 check is not dead code.

Nit (docs only): docs/operations.md:5924 is one overlong unwrapped line ("startup evidence; normal live monitoring owns later failures. A partial resume records progress; subsequent cleanup retries"). Re-wrap it to match the surrounding text. The prose itself matches the code.

Still needed before ready: current-head CI and the merge train, real deployed-image qualification, and Q777/Q966 evidence (a new full private backup and an actual isolated restore/rehearsal). This commit changes none of those conditions.

Removed by the corrections: rearmed_containers state, the duplicate rearm after resume, and the duplicate mock-only backoff test. I add nothing to remove.

@shiny-code-app
shiny-code-app Bot marked this pull request as ready for review October 28, 2026 10:34
@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Ready for the merge train

Head: a4b0218

@shiny-code-app shiny-code-app Bot added the ready-to-merge Every Code PR is ready for repository owner merge review label Oct 28, 2026
@shiny-code-app

shiny-code-app Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Launchplane completed the merge-train step for this pull request.

Launchplane landed this pull request through protected batch PR #5196.

Launchplane manages this comment and will update it as the train moves.

@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Review result: weighed
Head: 59348d0

Weighed review for 59348d00af362d42728159ea2afb12f93d21afa4: the default helper selected Anthropic claude-opus-5-5, confirmed by its CLI. No findings. I independently verified the reviewer limitation: the eight changed maintenance/health/storage files are byte-identical to previously reviewed a4b0218. The integrated base contributes its already landed recipient recovery behavior; this head adds only a test clock repair.

The merged-base fixture failed with a three-second delay in its synthetic upstream check: health observed at 11:58:45 preceded persisted inventory.updated_at at 11:58:46, so the production freshness gate correctly refused recovery. The same delayed probe passes after deriving observation time from persisted inventory plus receipt and fixture clocks. All 25 recovery tests and its real PostgreSQL wrapper pass on this integrated head; Ruff, formatting and full mypy pass. No production freshness rule changed. #5195 records this candidate-CI repair.

Native IDE inspection remains UNKNOWN: the prior supported native attempt timed out. The continuing Director exception is cbusillo/direction#98 comment 6428617448 while cbusillo/codex-skills#2923 remains open. I do not claim an IDE pass.

Removed: the fixture assumption that final receipt time is later than runtime publication. Net lines: +8 for this repair; PR aggregate +120, with 194 fewer production-code lines. The real backup/rehearsal/retirement remain pending landing and successful own-service deploy.

shiny-code-app Bot added a commit that referenced this pull request Oct 28, 2026
@shiny-code-app

Copy link
Copy Markdown
Contributor Author

Ready for the merge train

Head: 59348d0

@shiny-code-app shiny-code-app Bot closed this pull request by merging all changes into main in 1234c53 Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Every Code PR is ready for repository owner merge review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Observe accepted-recipient recovery after persisted runtime evidence

0 participants