fix: move kv and static onto the record-level path (TASK-0022) - #30
Draft
lr00rl wants to merge 1 commit into
Draft
fix: move kv and static onto the record-level path (TASK-0022)#30lr00rl wants to merge 1 commit into
lr00rl wants to merge 1 commit into
Conversation
The buckets were half-wired in both directions. `Store.PutKV`/`PutStatic`
mutated memory and called `Save()`, which encrypts and rewrites the whole JSON
state and fsyncs it — that was the authoritative copy. `BoltStateStore.PutKV`
and `PutStatic` existed, were correct, and had no caller. `ExportState` read
both buckets and `mergeRuntimeBoltHotState` then discarded them.
So neither "vestigial" nor "functional": guessing either way and acting on it
would have been wrong. The tree already carries the reason to move rather than
delete, written when snapshots were excluded — "potentially megabytes each …
would make every unrelated write pay for them". `static` is designed for
file-sized content and that argument applies verbatim.
Three changes that have to land together or the store disagrees with itself:
- write-through to bolt when the hot store is enabled, following
UpsertProxyUser;
- the merge takes bolt UNCONDITIONALLY rather than the `len(hot.X) > 0`
fallback the other domains use. They are still written to both places, so
falling back is right for them; here an empty bolt means empty, and
treating it as permission to keep the JSON copy would resurrect every
deleted entry;
- a one-time migration for entries written under the old behaviour, guarded
by a `_meta` flag.
The flag is not padding. Without it the migration re-runs on every open, and
because the JSON file keeps its stale copy until the next write, the second run
pushes back everything deleted since the first. The test written before the
flag existed caught exactly that: a deleted KV entry came back after a reopen.
The stale JSON copy is dropped explicitly on the migrating open, because
otherwise the file keeps carrying the bytes this change exists to remove.
Also: `static` had no delete at all. Tolerable for a handful of hand-uploaded
assets, not once something writes an object per record — replacing it would
leave every previous version behind forever.
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.
Closes the decision TASK-0022 was waiting on, and implements it.
The ruling
The exclusion list was the oversight. The task's draft understated the
state — re-read against the current tree it is not "written to both places":
Store.PutKV/PutStaticSave()→ whole JSON state re-encrypted, rewritten, fsynced. Authoritative.BoltStateStore.PutKV/PutStaticImportState.ExportStatemergeRuntimeBoltHotStateHalf-wired in both directions — neither vestigial nor functional. Guessing
"vestigial" and deleting the buckets would have been wrong.
The reason to move rather than delete is already in the tree, written when
snapshots were excluded: "potentially megabytes each … would make every
unrelated write pay for them."
staticis designed to hold file-sized content;the argument applies verbatim.
Three changes that must land together
UpsertProxyUser.dst.KV = hot.KV, not thelen(hot.X) > 0fallbackthe other domains use. Those are still written to both places, so falling back
is right for them. Here an empty bolt means empty, and treating it as
permission to keep the JSON copy would resurrect every deleted entry.
_metaflag.The flag is not defensive padding. Without it the migration re-runs on every
open, and because the JSON file keeps its stale copy until the next write, the
second run pushes back everything deleted since the first. The test written
before the flag existed caught exactly that — a deleted KV entry came back after
a reopen.
The stale JSON copy is dropped explicitly on the migrating open, or the file
keeps carrying the bytes this change exists to remove.
Also: static had no delete
Objects could only be written. Tolerable for a handful of hand-uploaded assets;
not once something writes an object per record, where replacing it would leave
every previous version behind forever.
Store.DeleteStaticandBoltStateStore.DeleteStaticare added, and deleting a missing object is not anerror.
What forced it
Sub-Store file generator scripts: 25–59 KB each, 529 KB in one operator's real
set. Putting those on the full-rewrite path means every unrelated state write
re-encrypts and rewrites half a megabyte of JavaScript.
Verification
go test -race ./internal/store/... ./internal/server/...— exit 0(store 25s, server 983s). No model change, so this is server-only — no SDK
release needed ahead of it.