test(oracle): graceful/brutal/rolling restart strategies for postgres oracle tests#4677
test(oracle): graceful/brutal/rolling restart strategies for postgres oracle tests#4677robacourt wants to merge 5 commits into
Conversation
The oracle property test could restart the stack every N batches
(RESTART_SERVER_EVERY) but only ever did a graceful shutdown. Introduce a
Support.OracleHarness.RestartStrategy behaviour with three implementations
selected via RESTART_TYPE:
- graceful (default) - clean stop_supervised + restore from disk
- brutal - Process.exit(pid, :kill) crash, then recover same stack_id
- rolling - a new stack contends on the same replication slot via the
advisory lock, then the old stack is gracefully stopped so the
new one takes over (faithful to cloud rolling deploys)
start_stack_supervisor! is now public with :id/:await_ready? opts, and Bandit
setup is factored into IntegrationSetup.start_bandit_client/2 with a shared
Finch pool so a second HTTP server can coexist during a rolling deploy.
Add RETRY_TRANSIENT_ERRORS (default off). When enabled, the shape checker retries a transient poll error (5xx response or a connection-level failure) within the check timeout instead of failing immediately. These are availability blips that production hides from clients during a restart/deploy (HTTP drains before teardown; the load balancer switches after the new instance is ready), so they are not data-consistency signals. Default behaviour is unchanged: any poll error fails. Regardless of the flag, 409/must-refetch, 4xx errors and data mismatches still fail, so real bugs are never masked.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4677 +/- ##
==========================================
- Coverage 60.61% 60.00% -0.62%
==========================================
Files 436 395 -41
Lines 45435 43747 -1688
Branches 12578 12581 +3
==========================================
- Hits 27541 26250 -1291
+ Misses 17816 17419 -397
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
robacourt
changed the title
Claude Code ReviewSummaryTest-only PR that generalizes the oracle property test's mid-run restart into three interchangeable strategies (graceful/brutal/rolling, via What's Working Well
Issues FoundCritical (Must Fix)None. Important (Should Fix)None. Suggestions (Nice to Have)One low-priority suggestion from earlier remains open — restating for tracking, not as a blocker: Optional Issue ConformanceNo linked issue. Per repo convention this is normally worth flagging, but the change is a self-contained developer-tooling improvement with a clear, detailed PR description and an explicit manual test plan. Implementation matches the described scope exactly, with no scope creep. The new commit's message accurately describes the change ("No change in the common single-strategy case"). Previous Review StatusIteration 4: the sole new commit ( Review iteration: 4 | 2026-07-02 |
Each strategy module was a one-line delegate to Support.ComponentSetup with no runtime polymorphism (selection is a static RESTART_TYPE string), so the behaviour + four modules were unnecessary indirection. Replace them with a private restart_stack/2 dispatch in oracle_harness.ex that pattern-matches the type and calls the ComponentSetup function directly; an unknown type raises.
With RETRY_TRANSIENT_ERRORS on, a window that timed out without a single successful poll previously returned stale state to assert_consistent!, which could pass silently if the oracle happened to be unchanged for that check. Thread a polled_ok? flag through do_await/3 and fail explicitly when the retry window completes no successful poll, so a genuinely-down server always fails rather than incidentally passing. Default behaviour (flag off) is unchanged.
…ategies graceful and brutal hardcoded the Electric.StackSupervisor child id while rolling read Map.get(ctx, :stack_supervisor_id, Electric.StackSupervisor). Make graceful/brutal use the same pattern (and preserve the id in their return merge) so the helpers stay correct if strategies are ever composed. No change in the common single-strategy case, where the id defaults to Electric.StackSupervisor.
1 participant
Summary
Extends the oracle property test's restart coverage (
RESTART_SERVER_EVERY) from a single hard-coded graceful shutdown to three interchangeable restart strategies, selected viaRESTART_TYPE, plus an opt-in flag to retry transient poll errors so the test never fails on a non-production availability blip.This is test-only — no product code changes, nothing ships to consumers, so no changeset.
Changes
Restart strategies (
RESTART_TYPE)A
Support.OracleHarness.RestartStrategybehaviour with three implementations, resolved byfor_type/1:graceful(default) — cleanstop_supervised+ restart the samestack_id, restoring from disk. Identical to the previous behaviour.brutal—Process.exit(pid, :kill)crash (no terminate callbacks), reconcile the ExUnit child spec, wait for the tree to fully terminate, then recover the samestack_id/storage.rolling— faithful rolling deploy: a new stack (ownstack_id/storage/HTTP server) contends on the same replication slot via the Postgres advisory lock, boots toread_only, then the old stack is gracefully stopped (releasing the lock; persistent slot + publication survive) and the new stack takes over as the single active writer.Supporting refactors:
ComponentSetup.start_stack_supervisor!made public with:id/:await_ready?opts (behaviour-preserving defaults).IntegrationSetup.start_bandit_client/2with a shared Finch pool so a second HTTP server can coexist during a rolling deploy.Opt-in transient-error retry (
RETRY_TRANSIENT_ERRORS, default off)When enabled, the shape checker retries a transient poll error (5xx response or a connection-level failure) within the check timeout instead of failing immediately. These are availability blips that production hides from clients during a restart/deploy (HTTP drains before teardown; the LB switches after the new instance is ready), so they are not data-consistency signals.
Default behaviour is unchanged. Regardless of the flag,
409/must-refetch,4xx, and data mismatches always fail, so real bugs are never masked.Notes for reviewers
RESTART_SERVER_EVERY). These strategies are a developer tool for hunting restart/deploy consistency bugs.gracefulis behaviour-identical to the previous path;brutalrecovers cleanly;rollingperforms the full lock hand-off and serves all shapes from the new stack's fresh resnapshot. The transient-error classifier was unit-checked ({5xx, 4xx, transport}->{retry, fail, retry}) and confirmed not to mask the real 409 signal.Test Plan
RESTART_TYPE=graceful RESTART_SERVER_EVERY=2 mix test test/integration/oracle_property_test.exs --include oracleRESTART_TYPE=brutal RESTART_SERVER_EVERY=2 mix test test/integration/oracle_property_test.exs --include oracleRESTART_TYPE=rolling RESTART_SERVER_EVERY=2 mix test test/integration/oracle_property_test.exs --include oracleRESTART_TYPE) unaffectedGenerated with Claude Code