fix(helm): give the migrate Job the session key; add an upgrade test - #6
Merged
Conversation
The chart has never been installable. The pre-install/pre-upgrade migrate
Job injected KNOT_DATABASE_URL but not KNOT_SESSION_KEY, and the binary
loads and validates the entire config before dispatching the subcommand.
Validation rejects an empty session key, so the hook exited 2 with
"KNOT_SESSION_KEY is required" and every helm install/upgrade failed
before it ever reached the Deployment.
Present identically in the published 0.1.0 and 0.2.0 charts. It survived
because `ct install` is disabled in chart CI, so nothing had ever actually
deployed the chart -- lint alone cannot see a hook that fails at runtime.
The new helm-upgrade workflow is what caught it. It installs the previous
PUBLISHED release (chart pulled from ghcr, not `git archive` of the tag --
Chart.yaml carries 0.0.0 placeholders in-tree and release.yaml injects the
real version at package time), seeds a document through that release's own
API, upgrades to the working tree, and then asserts:
1. /api/version changed -> the binary actually swapped
2. the old cookie still authorises -> session format + signing survived
3. the seeded document reads back -> schema + migrations survived
Verified end to end against a local kind cluster: 0.2.0 -> working tree,
with the pre-upgrade hook Job completing successfully once the session key
is wired in.
Two things the local run forced:
- Images are loaded via `docker save --platform` + `kind load image-archive`
rather than `kind load docker-image`. The released image is a multi-arch
index and kind imports with --all-platforms, which fails on manifests that
were never pulled ("content digest ...: not found").
- The base release is installed with migrations.enabled=false and migrated
out-of-band, because the base chart carries the very bug being fixed. That
workaround is marked for deletion once the resolved base is >= 0.2.1.
Promotes the Unreleased section to [0.2.1] and teaches release.yaml to lift that section out of CHANGELOG.md into the GitHub Release body. Until now the release page carried only the artifact blurb plus GitHub's generated commit list, so the actual "what changed and what do I have to do about it" lived only in the repo. Written into a single file because action-gh-release honours `body` OR `body_path`, not both. A tag with no matching changelog section still releases, but emits a warning and links to CHANGELOG.md. 0.2.1 is a chart-only release: no crates/, web/ or e2e/ files changed, so the image is behaviourally identical to 0.2.0.
helm/kind-action v1.14.0 ships kind v0.31.0, and pointing it at kindest/node:v1.35.5 fails the moment anything touches the node: ERROR: unknown containerd config version: 4 (supported versions: 2 and 3) The CLI and the node image are a matched pair. Letting the action pick its own default keeps them in step; the exact Kubernetes version is irrelevant to what this test asserts.
Restoring a snapshot corrupted the document for every connected client. ReplaceWithMarkdown clears the "default" fragment and applies the restored content in ONE transaction, but it persisted and fanned out the caller's `update_bytes`, which encodes only the insertion. Peers received the insert without the delete, so they kept what they already had and appended the restored text: expected: "First version of the doc." actual: "Completely different content.First version of the doc" The persisted update was missing the deletion too, so this was a data integrity bug rather than a display glitch -- replaying the update log reproduced the merged content. Fixed by capturing what the transaction actually produced via observe_update_v1, the same pattern PatchTaskChecked already used 40 lines below, and persisting/broadcasting that. The existing replace_with_markdown_swaps_content test could not catch this: it asserts on the room's OWN document, which was always correct. The new replace_broadcast_replaces_peer_content asserts on the frame a peer receives, applied to a peer holding the pre-restore state. Reverting only the two broadcast/persist lines turns it red with the exact production symptom while the old test stays green. This is the real cause of the flaky e2e history.spec failure. Earlier I attributed that to React 19, then react-router, then yrs 0.27, and finally to a prosemirror-view minor bump -- all wrong. main has been red since v0.2.0 was tagged; the one green run on PR #5 was luck, since whether the client re-syncs full state or applies the incremental update is timing dependent.
history.spec picked snapButtons.last() and called it "the OLDEST snapshot
which should be V1". With KNOT_SNAPSHOT_EVERY_N=1 the writer snapshots after
every persisted batch, so typing V1 leaves a trail of PREFIX snapshots
("F", "First version of the", ...). The list is ORDER BY snapshot_seq DESC,
so the oldest entry is the complete V1 only when every keystroke happened to
land in a single batch -- true on a fast machine, false on a loaded runner.
That is the second half of this spec's flakiness. With the CRDT broadcast bug
fixed the restore stopped merging, and the failure changed shape:
before: "Completely different content.First version of the doc" (merged)
after: "First version of the" (prefix)
The preview assertion could not catch it either: it only required "First
version", which any prefix satisfies, so the spec failed later at the restore
assertion and looked like a restore bug.
Now polls the NEWEST snapshot until its preview contains all of V1, pins that
one by seq, re-selects it after V2, and asserts the full string in both places.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The Helm chart has never been installable with default values.
The
pre-install/pre-upgrademigrate Job injectsKNOT_DATABASE_URLbut notKNOT_SESSION_KEY. The binary loads and validates the entire config before dispatching the subcommand (main.rs:41), andvalidate()rejects an empty session key (lib.rs:203) — so the hook exits 2 withKNOT_SESSION_KEY is requiredand everyhelm install/helm upgradefails before reaching the Deployment.Present identically in the published
0.1.0and0.2.0charts. It survived becausect installis disabled in chart CI, so nothing had ever deployed the chart — lint cannot see a hook that fails at runtime.migratenever reads the session key; it just has to pass validation.The test that caught it
New
helm-upgrade.yamlworkflow. It installs the previous published release (chart pulled from ghcr, real image), seeds a document through that release's API, upgrades to the working tree, and asserts:/api/versionchanged → the binary actually swappedTriggers on chart/migration/Dockerfile changes, weekly against
main, andworkflow_dispatch. Split fromhelm-ci.yamlbecause it builds an image and runs kind (~10 min) wherect linttakes ~10 s.Verified locally
Ran end to end against a local kind cluster before pushing:
Two things the local run forced
docker save --platform+kind load image-archive, notkind load docker-image. The released image is a multi-arch index and kind imports with--all-platforms, which fails on manifests that were never pulled (content digest ...: not found).migrations.enabled=falseand is migrated out-of-band, because the base chart carries the very bug being fixed. Marked for deletion once the resolved base is >= 0.2.1.Follow-up
The published
0.2.0chart is broken. This fix needs a0.2.1before the chart is usable; until then installs need--set migrations.enabled=falseplus a manual/knot-server migrate.🤖 Generated with Claude Code