perf(cli): strategy-driven parallel provisioning for pg-delta next plan shadows #6215
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,9 +22,16 @@ import { | |
| import { legacyWaitForShadowReady } from "../../../shared/db-bootstrap/health-check.ts"; | ||
| import { | ||
| legacyAcquireShadowDatabase, | ||
| legacyPeekShadowBaseline, | ||
| type LegacyShadowBaselinePeek, | ||
| type LegacyShadowCacheOpts, | ||
| type LegacyShadowAcquiredHandle, | ||
| } from "../../../shared/db-bootstrap/shadow-cache.ts"; | ||
| import { | ||
| legacyBufferedShadowOutput, | ||
| legacyResolvePlanShadowStrategy, | ||
| legacyRunPlanShadowProvisions, | ||
| } from "./legacy-pgdelta-next-shadow.plan.ts"; | ||
| import { | ||
| legacyConnectShadowDatabase, | ||
| legacyMigrateNextShadowDatabase, | ||
|
|
@@ -82,20 +89,30 @@ interface NativeShadowBase { | |
| readonly image: string; | ||
| } | ||
|
|
||
| interface ProvisionedDeclarativeShadow { | ||
| readonly declarativeUrl: string; | ||
| readonly restoredFromPgDataSnapshot: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * Whether pg-delta's same-database guard must be bypassed for this plan's two shadows — i.e. | ||
| * whether they can legitimately report the same PostgreSQL identity (system identifier + | ||
| * database OID). That happens exactly when the declarative shadow was physically RESTORED from | ||
| * the same snapshot key that also produced the migrations shadow's cluster: same key means same | ||
| * tar, and the migrations side is that tar's lineage whether it warm-restored FROM the tar or | ||
| * cold-exported it this very run — the baseline handoff, where requiring the migrations handle | ||
| * itself to be a warm restore would leave the guard armed against its own clone and fail the | ||
| * first cold plan (review: Codex on #6215, P1). A freshly initdb'd declarative shadow always | ||
| * carries its own new identity, and different keys mean tars exported from different clusters, | ||
| * so both of those stay `false` and keep the guard armed. A `true` alongside identities that | ||
| * happen to differ is harmless by design: pg-delta's bypass only takes effect on an exact | ||
| * identity match (`schema-plan.ts`'s `trustedCloneBypass`), never on a same-lineage sibling. | ||
| */ | ||
| export function legacyAllowSameDatabaseIdentityForPlanShadows(opts: { | ||
| readonly declarativeRestoredFromPgDataSnapshot: boolean; | ||
| readonly sameSnapshotKey: boolean; | ||
| }): boolean { | ||
| return opts.declarativeRestoredFromPgDataSnapshot && opts.sameSnapshotKey; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -155,19 +172,23 @@ export const legacyPgDeltaNextShadowLayer = Layer.effect( | |
| const dbConnection = yield* LegacyDbConnection; | ||
| const httpClient = yield* HttpClient.HttpClient; | ||
|
|
||
| // Parameterized on the Output service so `provisionPlan` can hand a concurrently running | ||
| // provision a buffering decorator (`legacyBufferedShadowOutput`) instead of the live one. | ||
| const runtimeWith = (outputService: typeof Output.Service) => | ||
| Layer.mergeAll( | ||
| Layer.succeed(FileSystem.FileSystem, fs), | ||
| Layer.succeed(Path.Path, path), | ||
| Layer.succeed(LegacyDebugFlag, debugFlag), | ||
| Layer.succeed(LegacyExperimentalFlag, experimentalFlag), | ||
| Layer.succeed(LegacyNetworkIdFlag, networkIdFlag), | ||
| Layer.succeed(CliArgs, cliArgs), | ||
| Layer.succeed(Output, outputService), | ||
| Layer.succeed(RuntimeInfo, runtimeInfo), | ||
| Layer.succeed(LegacyDockerRun, docker), | ||
| Layer.succeed(LegacyDbConnection, dbConnection), | ||
| Layer.succeed(HttpClient.HttpClient, httpClient), | ||
| ); | ||
| const runtime = runtimeWith(output); | ||
|
|
||
| const nextPort = (excluded?: number) => | ||
| Effect.gen(function* () { | ||
|
|
@@ -248,19 +269,39 @@ export const legacyPgDeltaNextShadowLayer = Layer.effect( | |
| }, | ||
| ); | ||
|
|
||
| const provisionMigrations = ( | ||
| input: NativeShadowInput, | ||
| opts: LegacyShadowCacheOpts, | ||
| onBaselineSeam: Effect.Effect<void> = Effect.void, | ||
| ) => | ||
| Effect.gen(function* () { | ||
| const handle = yield* acquireShadow(input, opts); | ||
| // The baseline-handoff strategy (`legacy-pgdelta-next-shadow.plan.ts`) waits on | ||
| // `onBaselineSeam` before warm-restoring the declarative shadow. A snapshot-cold handle | ||
| // reaches that seam when its export publishes the tar; any other handle (warm because | ||
| // another process published between peek and acquire, or uncached) never runs a | ||
| // snapshot, so signal immediately — the waiter then just re-peeks current disk state. | ||
| const seamWillRun = handle.snapshotRequired && !handle.baselinePresent; | ||
| const seamHandle: LegacyShadowAcquiredHandle = seamWillRun | ||
| ? { | ||
| ...handle, | ||
| snapshotBaseline: handle.snapshotBaseline.pipe(Effect.ensuring(onBaselineSeam)), | ||
| } | ||
| : handle; | ||
| if (!seamWillRun) yield* onBaselineSeam; | ||
| yield* awaitShadowReady(input, seamHandle); | ||
| const setup = setupRunInput(input, seamHandle); | ||
| yield* legacyMigrateNextShadowDatabase(input.spawner, setup, seamHandle); | ||
| return { | ||
| migrationsUrl: legacyToPostgresURL(setup.connConfig), | ||
| } satisfies LegacyPgDeltaNextMigrationsShadow; | ||
| }).pipe(Effect.provide(runtime), Effect.mapError(nextShadowError)); | ||
|
|
||
| const provisionDeclarative = ( | ||
| input: NativeShadowInput, | ||
| opts: LegacyShadowCacheOpts, | ||
| outputService: typeof Output.Service = output, | ||
| ) => | ||
| Effect.gen(function* () { | ||
| const handle = yield* acquireShadow(input, opts); | ||
| yield* awaitShadowReady(input, handle); | ||
|
|
@@ -279,7 +320,7 @@ export const legacyPgDeltaNextShadowLayer = Layer.effect( | |
| declarativeUrl: legacyToPostgresURL(setup.connConfig), | ||
| restoredFromPgDataSnapshot: handle.baselinePresent, | ||
| } satisfies ProvisionedDeclarativeShadow; | ||
| }).pipe(Effect.provide(runtimeWith(outputService)), Effect.mapError(nextShadowError)); | ||
|
|
||
| const cacheOpts = ( | ||
| opts: LegacyPgDeltaNextShadowInput, | ||
|
|
@@ -304,18 +345,79 @@ export const legacyPgDeltaNextShadowLayer = Layer.effect( | |
| const built = yield* buildNativeBase(opts); | ||
| const migrationsInput = buildNativeInput(opts, built, migrationsPort); | ||
| const declarativeInput = buildNativeInput(opts, built, declarativePort); | ||
| // The two shadows are independent — anonymous containers on the distinct host ports | ||
| // allocated above, per-invocation scoped temp dirs, and a race-tolerant network | ||
| // ensure — so warm provisions run fully concurrently. How much can safely overlap | ||
| // when a baseline still has to be BUILT is the strategy question: peek both cache | ||
| // states up front and dispatch (see `legacy-pgdelta-next-shadow.plan.ts` for the | ||
| // three strategies and their transcript guarantees). The peeks also resolve the | ||
| // cache-key inputs once; passing them back through `precomputedKeyInputs` keeps the | ||
| // acquire from repeating a live JWKS discovery request. | ||
| const [migrationsPeek, declarativePeek] = yield* Effect.all([ | ||
| legacyPeekShadowBaseline(migrationsInput.base, cacheOpts(opts, "config")), | ||
| legacyPeekShadowBaseline(declarativeInput.base, cacheOpts(opts, "disabled")), | ||
| ]); | ||
| const withPeek = ( | ||
| cache: LegacyShadowCacheOpts, | ||
| peek: LegacyShadowBaselinePeek, | ||
| ): LegacyShadowCacheOpts => | ||
| peek.state === "uncachable" | ||
| ? cache | ||
| : { ...cache, precomputedKeyInputs: peek.keyInputs }; | ||
| const strategy = legacyResolvePlanShadowStrategy(migrationsPeek, declarativePeek); | ||
| // Peeked inputs are only reused where the acquire follows the peek IMMEDIATELY: the | ||
| // migrations acquire always does, the declarative one only under `parallel`. In the | ||
| // handoff (waits for the seam) and sequential (waits for the whole migrations | ||
| // provision) strategies the declarative acquire is DELAYED, and the key hashes | ||
| // `supabase/roles.sql` while the cold setup re-reads that file at its own time — | ||
| // reusing a stale peek there could publish a baseline under a key that no longer | ||
| // describes it (review: Codex on #6215). Re-resolving at acquire time also | ||
| // self-corrects a handoff whose key genuinely changed mid-run: the recomputed key | ||
| // misses the just-exported tar and the declarative side correctly cold-provisions | ||
| // with the current inputs. The key's OTHER live input, the JWKS resolver, is | ||
| // deliberately exempt from this refresh: it is memoized per shadow input | ||
| // (`legacyShadowRunInputFromLocalContainerInputs`), so the 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). | ||
| 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 Useful? React with 👍 / 👎.
Member
Author
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 — Generated by Claude Code |
||
| const declarativeOpts = | ||
| strategy === "parallel" | ||
| ? withPeek(cacheOpts(opts, "disabled"), declarativePeek) | ||
| : cacheOpts(opts, "disabled"); | ||
|
Comment on lines
+384
to
+387
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 Useful? React with 👍 / 👎.
Member
Author
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 Generated by Claude Code |
||
|
|
||
| // In the concurrent strategies the declarative fiber's writes are buffered and | ||
| // flushed after the join, so nothing can land between two of the migrations fiber's | ||
| // live lines; sequential needs no buffer (one fiber at a time). `Effect.ensuring` on | ||
| // the JOIN (not the declarative fiber, which can finish first) so anomaly warnings | ||
| // survive failures without ever interleaving. | ||
| const buffered = | ||
| strategy === "sequential" ? undefined : legacyBufferedShadowOutput(output); | ||
| const provisions = legacyRunPlanShadowProvisions({ | ||
| strategy, | ||
| provisionMigrations: (onBaselineSeam) => | ||
| provisionMigrations(migrationsInput, migrationsOpts, onBaselineSeam), | ||
| provisionDeclarative: provisionDeclarative( | ||
| declarativeInput, | ||
| declarativeOpts, | ||
| buffered === undefined ? output : buffered.output, | ||
| ), | ||
| }); | ||
| const [migrations, declarative] = yield* buffered === undefined | ||
| ? provisions | ||
| : provisions.pipe(Effect.ensuring(buffered.flush)); | ||
| return { | ||
| migrationsUrl: migrations.migrationsUrl, | ||
| declarativeUrl: declarative.declarativeUrl, | ||
| // Key equality comes from the peeks (deterministic over inputs, not disk state), | ||
| // so a between-fibers eviction or publish cannot make it lie about lineage. | ||
| allowSameDatabaseIdentity: legacyAllowSameDatabaseIdentityForPlanShadows({ | ||
| declarativeRestoredFromPgDataSnapshot: declarative.restoredFromPgDataSnapshot, | ||
| sameSnapshotKey: | ||
| migrationsPeek.state !== "uncachable" && | ||
| declarativePeek.state !== "uncachable" && | ||
| migrationsPeek.key === declarativePeek.key, | ||
| }), | ||
| } satisfies LegacyPgDeltaNextPlanShadows; | ||
| }).pipe(Effect.mapError(nextShadowError)), | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itsprecomputedKeyInputshere means a concurrent edit or removal ofsupabase/roles.sqlis not reflected in the cache key, whilelegacySetupDatabaselater 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
parallelstrategy. 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 (andallowSameDatabaseIdentitystays consistent, since a cold declarative reportsrestoredFromPgDataSnapshot: false).One residual note for completeness: even with acquire-time resolution,
roles.sqlis read once for the key and again bylegacySetupDatabasea 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
legacyResolveDbSetupPreludeconsume 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