fix(ui): seed Wan params fields in the v3->v4 persist migration (release-upgrade params wipe) - #9408
Conversation
FindingsMedium: incomplete fix - upgrades from v6.12.0 and older still wipe the slice. Evidence chain:
The PR body acknowledges this as out of scope, but the title/summary claim to fix the "release-upgrade params wipe", and the same one-line-per-field seed in the existing v2 -> v3 block would close it. Anyone upgrading from v6.12.0 or earlier still loses prompts, model selection, and dimensions. To expose this issue, add a test that migrates a v2 blob built from an explicit v6.12.0-release key list (not Medium: the new regression tests do not lock in what their comments claim, and cannot catch the next occurrence. The fixture is described as a "field-accurate released-build v3 blob", but it is To expose this issue, add a test that derives the fixture key set from an explicit per-release constant (or a checked-in JSON snapshot of a real v6.13.x blob) and, better, a schema-completeness test that asserts every top-level key of Low: A single missing nullable key discards the user's entire persisted params slice with only a |
Addresses review feedback on invoke-ai#9408. Finding 1 (incomplete fix): the v3->v4 Wan seeds fixed upgrades from v6.13.x, but releases v6.10.0 - v6.12.0 persist _version 2 blobs, and 15 keys added to zParamsState after v3 was cut are required (no .default(), .optional() or .catch()) and seeded nowhere: fluxDype{Preset,Scale, Exponent}, zImageShift, zImageSeedVariance{Enabled,Strength, RandomizePercent}, anima{VaeModel,Qwen3EncoderModel,Scheduler}, klein{VaeModel,Qwen3EncoderModel} and qwenImage{ComponentSource, Quantization,Shift}. Seed them conditionally in the v2->v3 step, so released v2 blobs migrate cleanly and dev-build v2 blobs keep the values they already hold. Verified by running the real migrate() over a blob built from the v6.10.0 release key set: it threw on exactly those 15 paths before this change. Finding 2 (tests can't catch the next occurrence): the fixtures spread getInitialParamsState(), so they carry every current key and are inert against the general defect. Replace them with the top-level zParamsState key sets as actually shipped, read out of the release tags and checked in, one per persisted version still in the wild (v6.10.0 for v2 and v6.13.7 for v3 - each the narrowest key set among the releases writing that version, so a subset of every real blob). Add a schema-completeness test that runs the version steps over each release blob and asserts no key of the current schema is left unhandled, naming the offending keys and the step to fix. It fails on any future required-no-default key added without a seed. Finding 3 (fail-open-and-destroy): a single missing key made zParamsState.parse() throw, and the caller in store.ts falls back to the initial state, wiping prompts, model selection and dimensions with only a log.warn. Add backfillMissingParamsKeys(): after the version steps, fill any key that is absent and that the schema cannot fill itself, and warn with the key names. Narrow by design - a key that is present but invalid still throws, and anything with a .default()/.catch()/.optional() is left to zod. So a forgotten seed now costs one field at its default instead of the user's whole params slice. The completeness test above deliberately bypasses the net so it still fails CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8d4247e to
4d0ccb0
Compare
|
Thanks — all three findings confirmed and fixed, and finding 1 turned out to be wider than either of us had it. Rebased onto Finding 1 — incomplete fix, v2 blobs still wipeConfirmed. Your v6.12.0 probe found 7 keys; the real number is 21, because v6.12.0 is the widest v2 blob, not the narrowest. Four stable releases persist
Running the version steps over a v6.7.0-shaped blob throws on 21 paths — your 7, plus All 21 are now seeded in the v6.6.0 ( Finding 2 — fixtures were inertAgreed, and thanks for the precise diagnosis. Fixtures no longer derive from
Picking the narrowest is the whole point: my first pass at this used v6.10.0 as the v2 fixture and missed six keys precisely because it isn't the oldest v2 release. On top of the per-release migrate tests, the schema-completeness test you asked for: applyParamsVersionMigrations(blob);
const unseeded = backfillMissingParamsKeys(blob);
expect(unseeded).toEqual([]);For each released blob version it asserts that no top-level key of the current schema is left unhandled — a key is acceptable only if the schema can fill it itself ( Finding 3 — fail-open-and-destroyFixed rather than deferred, scoped to this slice. After the version steps,
The obvious risk is that the net makes the completeness test vacuous, since it repairs exactly what that test looks for. So that test deliberately runs Three edges the net did not cover, also closed:
I did not change the generic Not coveredv6.2.0a1 – v6.5.1 persist a blob with no VerificationEvery fix is mutation-checked — reverting any one of them fails at least one test, and dropping the 21 seeds fails the completeness test. |
|
This fixes the reported problem, and the core of it is well-evidenced. I verified the migration Verified correct
Worth a follow-up PR
None of 2-4 blocks this PR. Item 1 is the one I would not leave open for long, since it only gets |
|
Thanks — all four are in this PR rather than a follow-up. Two of them turned out to be wider than reported, and item 2 in particular was hiding a real invariant violation. 1 — The v4 tier is unguardedConfirmed, including the dating: the bump is Fixed by giving the four PiD fields zod defaults rather than bumping to v5. That follows the precedent already set in this tier: I took the structural suggestion too: 2 — No v0 fixture in the invariantAdded, and the exercise justified itself: the v0 tier was actually violating the invariant, and the safety net was the only thing hiding it. First, the range in my own comment was wrong. Reading
So v0 runs to v6.6.0rc2, not v6.5.1; v1 includes v6.7.0rc1; v2 runs through v6.13.0.rc1. (My v1/v2/v3/v4 fixtures are unaffected — v6.7.0 really is the intersection of all 21 v2 builds, and v6.13.7 of all 7 v3 builds.) The consequence:
Second, and this applies to your fixture as much as mine: a v0 fixture asserting only that 3 — Stale comments describing the presence checkFixed both, and I folded your second point into the comment rather than dropping the value check: // Value check rather than `!('_version' in state)`. The two are equivalent on real input, since
// the only production caller feeds this `JSON.parse` output, which cannot produce an explicit
// `undefined` — but a value check is what the rest of this file uses, and with a presence check a
// blob carrying `_version: undefined` would match no branch at all, reach the parse and take the
// whole slice down.4 — The
|
Pfannkuchensack
left a comment
Pfannkuchensack
left a comment
There was a problem hiding this comment.
Looks good beside the merge conflict. we need to check more for if a version bump is needed.
|
@copilot resolve the merge conflicts in this pull request |
The Wan video PR (invoke-ai#9163) added five keys to zParamsState (wanTransformerLowNoise, wanComponentSource, wanVaeModel, wanT5EncoderModel, wanGuidanceScaleLowNoise) while the persisted params schema was still at _version 3, without a version bump or migration seed. The keys are .nullable() with no .default(), which zod treats as required, and migrate() ends with zParamsState.parse() whose failure makes the store silently replace the slice with its initial state. Released v6.13.x builds write v3 blobs without these keys, so any user upgrading from a release to a build containing Wan loses their entire params slice (prompts, prompt history, model selection, dimensions, generation settings) on first launch. Dev machines don't reproduce it because v3 blobs written after the Wan merge already carry the keys. The seeds themselves are no longer this commit's job: they reached main with the FLUX.2 [dev] merge (f10d2a4), together with a field-accurate released-build v3 fixture that fails without them. What main does not cover is the other half of the contract — that the seeds are written with `??` rather than assigned, so a v3 blob from a dev build after the Wan merge keeps the values it already holds instead of having them reset to null. Add that test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses review feedback on invoke-ai#9408. Finding 1 (incomplete fix): the v3->v4 Wan seeds fixed upgrades from v6.13.x, but releases v6.10.0 - v6.12.0 persist _version 2 blobs, and 15 keys added to zParamsState after v3 was cut are required (no .default(), .optional() or .catch()) and seeded nowhere: fluxDype{Preset,Scale, Exponent}, zImageShift, zImageSeedVariance{Enabled,Strength, RandomizePercent}, anima{VaeModel,Qwen3EncoderModel,Scheduler}, klein{VaeModel,Qwen3EncoderModel} and qwenImage{ComponentSource, Quantization,Shift}. Seed them conditionally in the v2->v3 step, so released v2 blobs migrate cleanly and dev-build v2 blobs keep the values they already hold. Verified by running the real migrate() over a blob built from the v6.10.0 release key set: it threw on exactly those 15 paths before this change. Finding 2 (tests can't catch the next occurrence): the fixtures spread getInitialParamsState(), so they carry every current key and are inert against the general defect. Replace them with the top-level zParamsState key sets as actually shipped, read out of the release tags and checked in, one per persisted version still in the wild (v6.10.0 for v2 and v6.13.7 for v3 - each the narrowest key set among the releases writing that version, so a subset of every real blob). Add a schema-completeness test that runs the version steps over each release blob and asserts no key of the current schema is left unhandled, naming the offending keys and the step to fix. It fails on any future required-no-default key added without a seed. Finding 3 (fail-open-and-destroy): a single missing key made zParamsState.parse() throw, and the caller in store.ts falls back to the initial state, wiping prompts, model selection and dimensions with only a log.warn. Add backfillMissingParamsKeys(): after the version steps, fill any key that is absent and that the schema cannot fill itself, and warn with the key names. Narrow by design - a key that is present but invalid still throws, and anything with a .default()/.catch()/.optional() is left to zod. So a forgotten seed now costs one field at its default instead of the user's whole params slice. The completeness test above deliberately bypasses the net so it still fails CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up from an adversarial review of the previous commit.
The v2 fixture was not the narrowest v2 release. The earlier survey
globbed tags as v6.1*, which silently excluded v6.7.0 - v6.9.0 — four
stable releases that also persist _version 2, with only 46 keys against
v6.10.0's 52. Six further keys are required-with-no-default and seeded
nowhere: fluxScheduler, zImageScheduler, colorCompensation,
zImageVaeModel, zImageQwen3EncoderModel and zImageQwen3SourceModel.
Verified by running the version steps over a v6.7.0-shaped blob: parse()
throws on exactly those six. Seed them in the v2 -> v3 step and replace
the fixture with the true narrowest set (v6.7.0, confirmed a strict
subset of v6.10.0/v6.11.x/v6.12.0). Add a v6.6.0 (_version 1) fixture
too; it is the v6.7.0 set minus positivePromptHistory, which the v1 step
already seeds.
Also close three edges the safety net did not cover:
- The v0 step dereferenced state.dimensions.rect unguarded, so a blob
lacking dimensions threw a TypeError straight out of migrate() — the
one remaining path that could still wipe the slice. Guard it and let
the backfill repair dimensions instead.
- The v0 branch tested key presence (!('_version' in state)) while the
backfill tests value (!== undefined). A blob with an explicit
undefined _version matched no branch, reached the parse and took the
slice down. Detect v0 by value so the two agree.
- Exclude _version from the backfill loop, so a future change cannot
turn it into a version-detection bypass that stamps a blob current
without running a single step.
Each fix is mutation-checked: reverting any one of them fails at least
one test, and the six seeds fail the schema-completeness test.
Not covered: v6.2.0a1 - v6.5.1 persist a blob with no _version at all
and predate the current dimensions shape, so a faithful fixture can't be
built by filtering getInitialParamsState(). Noted in the test file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the four follow-ups from Pfannkuchensack's second review. The v4 tier was unguarded: the PiD fields landed a day after the _version 3 -> 4 bump, so dev builds from that window persist v4 blobs without them, and a v4 blob matches no branch in the migration chain. Give the four fields zod defaults, matching the ernieImage* precedent set by the other two post-bump additions, and pin a RELEASE_PARAMS_KEYS entry to the bump commit's key set so the invariant covers the tier no version step can reach. Add v0 fixtures. The claimed v0 range was wrong: it spans v6.0.0a1 - v6.6.0rc2, and the oldest builds have no `dimensions` key at all, which no step seeded — the invariant only held there because the safety net caught it. Seed `dimensions` in the v0 step and cover both v0 shapes. Widen the safety net from omissions to any key whose persisted value fails its own field schema, so a `dimensions` the v0 guard left incomplete, or a `model` whose base has since left zBaseModelType, costs that one field instead of the whole slice. This makes true what the guard's comment already claimed. Fix the comments that still described the presence check this branch replaced with a value check. Also close three holes in the tests themselves, all found by mutating the production code and watching nothing fail: the _version guard test never reached the guard (the version steps normalise _version first), the PiD test could not distinguish the new defaults from the safety net backfilling the same values, and the fixtures carried initial values throughout — including `model: null`, so nothing noticed a model being silently cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HiDiffusion (invoke-ai#8787) landed in main while this PR was in review, adding five keys to zParamsState after the _version 3 -> 4 bump. Keys added after a bump land in a tier the migration chain cannot reach — a v4 blob matches no branch — so they were seeded by an ad-hoc block inside migrate() instead. That works, but it sits outside applyParamsVersionMigrations(), so the completeness invariant added by this PR cannot see it and reports the five keys as unseeded. Give them zod defaults, the same route the ERNIE-Image and PiD fields take, and drop the now-redundant block: the defaults carry the identical values, and the repair pass covers the blob before the parse either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by mutation-testing this branch's own suite: 116 single-line mutations of the production code, each run against the tests. Three classes of mutation left the suite green. Conditional seeds. Turning `state.X = state.X ?? V` into `state.X = V` went undetected for 21 of the 31 conditional seeds, because only 10 were probed by the two hand-written "preserves ... dev-build" tests. The `??` is the entire point of those lines: the field landed in the schema before the bump that seeds it, so a blob from that window already holds a real user value, and an unconditional assignment resets it. Replace the two tests with a table covering every conditional seed, one probe per key. The table self-validates — each probe must satisfy the field's own schema and must differ from what the step would seed — so a probe that stops discriminating fails rather than going quiet. Verified: seven representative mutations now fail, each naming its own key. Fixture erosion. `buildReleaseBlob` sources values from getInitialParamsState(), so a fixture key that leaves zParamsState is dropped from the blob silently while still sitting in the table, and the fixture stops reproducing the shape it names. That already happened: kleinVaeModel left the schema when the v4 -> v5 step folded it into flux2VaeModel, and it is the input to that fold. Add REMOVED_SCHEMA_KEY_VALUES so removed keys are still reproduced, plus a guard test that fails on any fixture key in neither the schema nor that table. Post-bump defaults. Three mechanisms now write the PiD fields with identical values — the v3 -> v4 seed, the v4 -> v5 seed and the zod default — so any one could be reverted with the other two covering for it, and the test that claims to prove the defaults could not see it. Pin the property directly: every key added after the last bump must satisfy `shape[key].safeParse(undefined)`, which is what lets it survive on a tier that has no migration step. Also assert the version steps leave no key holding a schema-rejecting value, and note in the completeness test what it does not cover: fixture values are initial values, so a tightened *nested* schema still costs a top-level key via `reset` without any fixture noticing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5a72576 to
f740de7
Compare
2 participants
Summary
Upgrading from any released v6.13.x build to current
mainsilently wipes the user's entire persisted params slice (prompts, prompt history, model selection, dimensions, all generation settings) on first launch. This PR fixes it by seeding the five Wan component fields in the v3→v4 params persist migration.Root cause
The Wan video PR (#9163) added five keys to
zParamsState—wanTransformerLowNoise,wanComponentSource,wanVaeModel,wanT5EncoderModel,wanGuidanceScaleLowNoise— while the persisted schema was still at_version: 3, without a version bump or a migration seed. All five are.nullable()with no.default(), which zod 4 treats as required: a persisted blob missing the key failsparse().migrate()ends withzParamsState.parse(state), and when that throws,unserialize(store.ts) catches it and silently replaces the slice withgetInitialState().Released v6.13.x builds write v3 blobs without these keys (no release contains the Wan merge), so a release → main upgrade hits: migrate runs → all seeds applied except wan →
parse()throws on exactly the five wan paths → whole slice wiped, with only alog.warn. Verified with a field-accurate pre-Wan v3 blob: parse fails on precisely those five keys and nothing else.Two effects masked this:
getInitialParamsState(), which carries the keys, so the suite couldn't catch it.The fix
Seed the five keys conditionally (
?? null) in the existing v3→v4 step. Conditional, because v3 blobs written by dev builds after the Wan merge can already hold real values (e.g. a selected Wan VAE) that must not be clobbered.Tests:
Related
colorCompensation,fluxDype*,anima*,zImageSeedVariance*— affecting upgrades from pre-v6.13 releases), and the throw-and-wipe failure mode itself may be worth revisiting (repair-and-log, or a.default(null)convention for nullable slots). Kept out of scope here to keep this release-blocking fix minimal; can file a follow-up issue.QA Instructions
mainwithout this PR: params are reset on first launch.Or:
pnpm vitest run src/features/controlLayers/store/paramsSlice.test.ts.Checklist
🤖 Generated with Claude Code