perf(cli): strategy-driven parallel provisioning for pg-delta next plan shadows - #6215
Conversation
A declarative sync provisions two shadow databases — a migrations shadow and a declarative shadow — strictly sequentially, so the declarative shadow's whole create/ready/baseline cost (seconds cold or warm) was added on top of the migrations shadow's instead of overlapping it. The two are fully independent: anonymous containers on distinct pre-allocated host ports, per-invocation scoped temp dirs, and a race-tolerant network ensure. provisionPlan now runs them with Effect.all concurrency 2; a failure on either side interrupts the other and the scope finalizers still remove whatever was created. The one shared artifact is the baseline snapshot cache: the two shadows hash to the SAME cache key whenever their effective webhooks booleans agree, and legacyExportPgDataTar names its temp file by pid alone, so two same-process cold exports would share the temp path — the second writer's pre-clean unlinks the first's live temp file, and the first's rename could then publish the second's half-written bytes under the final tar name. Cold exports are now serialized by an in-process mutex, and a writer that finds the tar published while it waited skips its own export (a same-key sibling's snapshot is the same baseline). Cross- process writers were never affected (distinct pids). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYVv1jjXdrTzqDTVrU4Had
![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: 04da465f86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const published = yield* input.fs.exists(tarPath).pipe(Effect.orElseSucceed(() => false)); | ||
| if (published) return; |
There was a problem hiding this comment.
When a warm restore fails during docker cp—including because the archive is corrupt—legacyAcquireShadowDatabase deliberately retains the existing tar and falls back to a cold shadow so that snapshotBaseline can atomically replace it. This new existence check instead returns before exporting, leaving the unusable tar in place; every later invocation attempts the same broken restore and pays for another cold provision. Deduplication should only skip a tar published after this cold acquisition began, not one that caused the warm-path fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ac55002. The skip now only applies on the cold path whose tar was absent when the acquisition began (legacyColdCachedShadow takes skipIfPublished, true from the !cached branch, false from the warm-fallback branch), so a retained-but-unusable tar is still atomically replaced by the fallback's own export. Also strengthened the existing warm-fallback test to corrupt the tar's bytes up front and assert the republished bytes — it previously only asserted the tar count, which is why the skip slipped through.
Generated by Claude Code
… at acquire The export dedupe skipped whenever a tar existed at the final path, but the warm-fallback cold path deliberately RETAINS an unusable tar (an extraction failure does not implicate its contents) precisely so the fallback's own export atomically replaces it. Skipping there left a genuinely corrupt tar in place forever, failing every later warm restore into another cold provision. The skip now applies only on the cold path whose tar was absent when the acquisition began, where a tar found at export time can only be a same-key sibling's fresh publish. The existing warm-fallback test asserted only the tar COUNT after the republish, which is why the skip slipped through — it now corrupts the tar's bytes up front and asserts the exported bytes replaced them. Review: Codex on #6215. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYVv1jjXdrTzqDTVrU4Had
Provisioning the two plan shadows now dispatches on a peek of each
shadow's baseline-cache state instead of always running both
concurrently, so a cold sync never pays the platform baseline twice and
the user-visible transcript can never interleave:
- warm + warm: both shadows restore in parallel, as before. A warm
restore skips the baseline entirely, so only the migrations fiber
prints ("Applying migration ..."), live and in order.
- cold + cold under one cache key: baseline handoff. The migrations
shadow cold-provisions and its snapshot export at the baseline seam
(before migration replay) signals the declarative fiber, which then
warm-restores from the just-published tar concurrently with the
replay. The baseline is built exactly once; a handle that will never
snapshot (warm-raced or uncached acquire) signals immediately, and the
runner ensures the signal on the whole provision as a liveness
backstop so the waiter cannot deadlock.
- everything else (different keys, mixed states, --no-cache, cache env
off, PG<=14/OrioleDB): sequential, exactly the pre-parallel flow and
transcript.
The peek helper (legacyPeekShadowBaseline) also returns the resolved
cache-key inputs, which the acquire reuses via precomputedKeyInputs so
the JWKS discovery request embedded in the key is not resolved twice.
In the concurrent strategies the declarative fiber's Output raw/rawBytes
writes are buffered and flushed after the join, making "no line lands
between two live lines" a hard guarantee rather than an emergent
property of warm restores being silent; post-flush writes pass through
live so late teardown warnings are never lost. Debug-only writes that
bypass Output (SUPABASE_SHADOW_DEBUG, failure-path container log dumps)
deliberately stay live.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NYVv1jjXdrTzqDTVrU4Had
avallete
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9a2f2ba1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| declarative.state === "cold" && | ||
| migrations.key === declarative.key | ||
| ) { | ||
| return "baseline-handoff"; |
There was a problem hiding this comment.
When the cache is empty and effective webhooks are disabled (the default), this strategy exports the migrations shadow's PGDATA and restores that exact snapshot into the declarative shadow, so both servers have the same PostgreSQL system identity. However, the migrations handle still reports baselinePresent: false, causing legacyAllowSameDatabaseIdentityForRestoredShadows to return false and the engine call to omit allowSameDatabaseIdentity; the first cold declarative plan can therefore be rejected for comparing databases with the same identity. Track the shared snapshot lineage (or the handoff strategy) when setting that option rather than requiring both handles to have been warm restores.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b817476. The flag now encodes snapshot lineage instead of "both handles were warm restores": legacyAllowSameDatabaseIdentityForPlanShadows returns true exactly when the declarative shadow was restored from the same snapshot key that also produced the migrations cluster — same key means same tar, and the migrations side is that tar's lineage whether it warm-restored from it or cold-exported it this run (the handoff). Key equality is taken from the peeks (deterministic over inputs, not disk state), a fresh initdb stays false, and a spurious true against differing identities is inert since pg-delta's bypass only takes effect on an exact identity match.
Generated by Claude Code
…tive shadow pg-delta's planSchemaFiles refuses to load declarative SQL when the shadow and target report the same PostgreSQL identity unless allowSameDatabaseIdentity is set. The baseline handoff restores the declarative shadow from the tar the migrations shadow just exported — an exact physical clone, same system identifier and database OIDs — but the flag was computed as "both handles were warm restores", which the cold migrations side fails, so the first cold plan would be rejected by the guard it exists to bypass. The flag now encodes actual snapshot lineage: true exactly when the declarative shadow was RESTORED from the same snapshot key that also produced the migrations cluster — same key means same tar, and the migrations side is that tar's lineage whether it warm-restored from it or cold-exported it this run. A freshly initdb'd declarative shadow has its own identity and different keys mean different originating clusters, so those stay false and keep the guard armed. Key equality is taken from the peeks (deterministic over inputs), so disk-state races cannot make it lie. A true alongside identities that happen to differ is harmless: pg-delta's bypass only takes effect on an exact identity match, never on a same-lineage sibling. Review: Codex on #6215 (P1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYVv1jjXdrTzqDTVrU4Had
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b817476b5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| peek.state === "uncachable" | ||
| ? cache | ||
| : { ...cache, precomputedKeyInputs: peek.keyInputs }; |
There was a problem hiding this comment.
When the strategy is sequential, both peeks happen before the migrations shadow runs, but the declarative shadow may not acquire until a long migration replay finishes. Passing its precomputedKeyInputs here means a concurrent edit or removal of supabase/roles.sql is not reflected in the cache key, while legacySetupDatabase later reads and applies the current file (db-setup.ts:1084-1103); the resulting baseline is therefore published under the stale key and subsequent commands can silently restore the wrong roles. Recompute mutable key inputs when the delayed acquire begins, or carry the resolved roles content through to setup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in c0511f1. Peeked key inputs are now reused only where the acquire follows the peek immediately — the migrations acquire always, the declarative one only under the parallel strategy. The delayed declarative acquires (handoff and sequential) re-resolve at acquire time, which also self-corrects a handoff whose key genuinely changed mid-run: the recomputed key misses the just-exported tar and the declarative side cold-provisions with the current inputs (and allowSameDatabaseIdentity stays consistent, since a cold declarative reports restoredFromPgDataSnapshot: false).
One residual note for completeness: even with acquire-time resolution, roles.sql is read once for the key and again by legacySetupDatabase a few seconds later during the same cold provision — that acquire→setup window predates this PR (it's inherent to #6184's key design) and is unchanged here.
Generated by Claude Code
There was a problem hiding this comment.
The memoization is deliberate and keeping it is the correct behavior here, so no code change — only the comment overclaiming "remote JWKS" as refreshed was wrong, corrected in 5390632.
The property that made the roles.sql half of this thread a real bug was key/content divergence. JWKS cannot diverge: the cache-key resolution and legacyResolveDbSetupPrelude consume the same memoized effect on the same shadow input, so a published tar always carries exactly the value its key was computed from — that consistency guarantee is what the memo was added for on #6184 ("guarantees the published snapshot carries the exact value its key was computed from, even if the issuer rotates mid-run"). Resetting the resolver at delayed acquisition would trade that away for freshness the cache doesn't promise anywhere: a warm hit already serves a baseline whose JWKS was resolved up to 14 days earlier under its matching key, so command-start vs acquire-time (seconds to minutes apart) is immaterial — and it would add a second live discovery request per cold sync.
Generated by Claude Code
The declarative shadow's acquire is delayed under the handoff (waits for the baseline seam) and sequential (waits for the whole migrations provision, including a possibly long replay) strategies, and the cache key hashes mid-run-mutable inputs — supabase/roles.sql and the remote JWKS — that a cold setup re-reads at its own time. Reusing the up-front peek's resolved inputs there could publish a baseline under a key that no longer describes it, letting a later run warm-restore the wrong roles. Peeked inputs are now passed through only where the acquire follows the peek immediately: the migrations acquire always, the declarative one only under the parallel strategy. A delayed acquire re-resolves at acquire time, which also self-corrects a handoff whose key genuinely changed mid-run — the recomputed key misses the just-exported tar and the declarative side cold-provisions with the current inputs. Review: Codex on #6215 (P2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYVv1jjXdrTzqDTVrU4Had
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0511f1211
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const declarativeOpts = | ||
| strategy === "parallel" | ||
| ? withPeek(cacheOpts(opts, "disabled"), declarativePeek) | ||
| : cacheOpts(opts, "disabled"); |
There was a problem hiding this comment.
When realtime is enabled on PG15+ and the remote issuer rotates its JWKS during a long migrations provision, omitting precomputedKeyInputs here still does not refresh that input: legacyShadowRunInputFromLocalContainerInputs wraps the resolver with legacyMemoizeSuccess (shadow-database.ts:522-529), and the initial declarative peek has already memoized the old value. Consequently, the delayed sequential/handoff acquire and its cold setup both use the command-start JWKS rather than the value available when provisioning actually begins, unlike the pre-change sequential flow. Fresh evidence beyond the earlier stale-key review is that the follow-up re-resolution reuses this already-evaluated memo; construct/reset the declarative resolver at delayed acquisition or otherwise defer its first evaluation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The memoization is deliberate and keeping it is the correct behavior, so no code change — only my comment overclaiming "remote JWKS" as refreshed was wrong, corrected in 5390632.
What made the roles.sql half of this review a real bug was key/content divergence. JWKS cannot diverge: the cache-key resolution and legacyResolveDbSetupPrelude consume the same memoized effect on the same shadow input, so a published tar always carries exactly the value its key was computed from — the memo was added on #6184 for precisely that guarantee ("guarantees the published snapshot carries the exact value its key was computed from, even if the issuer rotates mid-run"). Resetting the resolver at delayed acquisition would trade that consistency away for freshness the cache doesn't promise anywhere — a warm hit already serves a baseline whose JWKS was resolved up to 14 days earlier under its matching key, so command-start vs acquire-time is immaterial — and it would add a second live discovery request per cold sync.
Generated by Claude Code
The stale-peek refresh comment listed the remote JWKS among the inputs a delayed acquire re-resolves, but that resolver is deliberately memoized per shadow input (shadow-database.ts, review: Codex on #6184) so the cache key and the baked baseline always carry the same value and cannot diverge — a delayed acquire keeps the command-start JWKS, well inside the staleness the snapshot cache accepts by design (a warm hit serves a tar up to 14 days old under its matching key). Scope the comment to roles.sql, the one input the refresh actually re-reads, and state the JWKS exemption explicitly. Review: Codex on #6215. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYVv1jjXdrTzqDTVrU4Had
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5390632fcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // always carry the SAME value and cannot diverge; a delayed acquire keeps the | ||
| // command-start JWKS, well inside the staleness the snapshot cache accepts by design | ||
| // (a warm hit serves a tar up to 14 days old under its matching key). | ||
| const migrationsOpts = withPeek(cacheOpts(opts, "config"), migrationsPeek); |
There was a problem hiding this comment.
When realtime is enabled and the declarative peek's JWKS resolution is slow, the migrations peek may have already read roles.sql, but the Effect.all at lines 356–359 waits for the slower peer before this acquisition begins. If roles.sql changes during that interval, these precomputed inputs select the old cache key while setup later reads and executes the current file (db-setup.ts:1084–1103), either restoring a stale baseline or publishing the new roles under the wrong key. Fresh evidence beyond the prior delayed-declarative comment is that this rendezvous also delays the supposedly immediate migrations acquire; re-resolve its mutable inputs after both peeks finish.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No change here — this window is not materially reducible and is already dominated by one this layer cannot close.
The gap flagged (migrations peek → acquire) is bounded by the declarative peek's duration: essentially one JWKS discovery request, and only when realtime on PG15+ makes that a live fetch — otherwise it's filesystem reads measured in milliseconds. The window that follows the acquire on every cold provision — docker create, the root-key docker cp, docker start, the readiness wait, the setup prelude — is multiple seconds before legacySetupDatabase re-reads roles.sql at its own time, and is inherent to #6184's key design. Re-resolving the migrations key after the rendezvous moves one read a fraction of a second later while that larger, irreducible window persists, so the mid-run-edit race's reachability and consequences are identical either way. The fix earlier in this review targeted a minutes-long widening (the full migration replay); this is seconds in front of seconds. Fully closing the race means carrying resolved roles content into setup — a #6184 design change out of this PR's scope.
Generated by Claude Code
2 participants
Summary
Stacked on #6203 (← #6184 ← #6102).
A declarative sync provisions two shadow databases — a migrations shadow and a declarative shadow — strictly sequentially:
provisionPlanawaited the migrations shadow completely (create → ready → baseline → apply migrations) before even starting the declarative one, so the declarative shadow's whole provisioning cost was added to the runtime instead of overlapping it.Provisioning is now dispatched on a peek of each shadow's baseline-cache state (
legacyPeekShadowBaseline, new inshadow-cache.ts), with three strategies (legacy-pgdelta-next-shadow.plan.ts):parallel(steady state — both snapshots published): both shadows warm-restore concurrently. A warm restore skips the platform baseline entirely, so only the migrations fiber prints (Applying migration ...), live and in order. Saves roughly one warm provision (~3s) per sync.baseline-handoff(first run — both cold under one cache key, i.e. webhooks agree): the platform baseline is built and paid exactly once. The migrations shadow cold-provisions; its snapshot export at the baseline seam (after platform setup, before migration replay) signals the declarative fiber, which warm-restores from the just-published tar concurrently with the migration replay. A handle that will never snapshot (warm-raced or uncached acquire) signals immediately, and the runnerEffect.ensurings the signal onto the whole provision as a liveness backstop — the waiter cannot deadlock.sequential(different keys, mixed states,--no-cache, cache env off, PG≤14/OrioleDB): no baseline can be shared, so this keeps the pre-parallel flow and transcript byte for byte.Output ordering is a hard guarantee, not an emergent property. In the concurrent strategies the declarative fiber's
Outputwrites go through a buffering decorator (legacyBufferedShadowOutput) flushed after the join — nothing can land between two of the migrations fiber's live lines, even on anomaly paths (cache warnings, cold fallback). Post-flush writes pass through live so late teardown warnings are never lost. Debug-only writes that bypassOutput(SUPABASE_SHADOW_DEBUGtiming lines, failure-path container-log dumps) deliberately stay live.Supporting changes in
shadow-cache.ts:legacyPeekShadowBaselineanswers "what would the acquire do right now" (warm/cold/uncachable + key) without provisioning; the acquire always re-checks current disk state, so a stale peek is merely suboptimal, never incorrect.LegacyShadowCacheOpts.precomputedKeyInputs, so the JWKS discovery request embedded in the key (realtime on PG15+) is not resolved twice.legacyExportPgDataTar's temp name is pid-scoped, so two same-process writers would otherwise share the temp file and the atomic rename could publish half-written bytes. The absent-at-acquire dedupe skips a re-export only when the tar appeared after the cold acquisition began; a tar retained through a failed warm restore is still atomically replaced by the fallback's own export.Transcript note: in the handoff case, the declarative shadow's duplicate
Initialising schema.../Seeding globals...pair disappears (it restores warm now, printing nothing). Strictly less noise; migration-replay lines and all result output are unchanged.The orchestration (strategy choice, handoff signal, buffering) is extracted into
legacy-pgdelta-next-shadow.plan.tsand unit-tested withDeferred-gated fakes — concurrency overlap, seam ordering, no-hang backstop, fail-fast interruption, and flush ordering — alongside integration coverage for the peek states, the precomputed-key JWKS contract, concurrent same-key exports, and corrupt-tar replacement.Linked issue
Closes #
open-for-contributionlabel (or I'm a Supabase maintainer).Checklist
fix(cli): …).pnpm check:allandpnpm testpass for the workspace(s) I touched.🤖 Generated with Claude Code
https://claude.ai/code/session_01NYVv1jjXdrTzqDTVrU4Had