fix(storage): preserve create_time on KV replacement upserts - #3872
Conversation
JsonKV, RedisKV, and OpenSearchKV were dropping or resetting storage-managed create_time when callers replaced a value with business fields only. Keep the original create_time (or 0 for legacy rows), advance only update_time, and add regression tests. Fixes HKUDS#3870
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c6633f51ff
ℹ️ 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".
Upsert create_time mget raced concurrent delete/flush and broke offline tests that stubbed mget assuming upsert never touched OpenSearch. Run the lookup under _flush_lock, revalidate on buffer swap, and update mocks to echo ids / assert read paths after reset_mock.
…ites The previous commits fixed the semantics of issue HKUDS#3870 but paid for them with a client-side read-modify-write: OpenSearch issued an mget per upsert() call (latterly while holding the cross-process namespace lock), and Redis replaced its EXISTS pipeline with GET, pulling every stored value back over the wire just to read one integer. Mongo and PostgreSQL need neither read because they push the conditional write to the server ($setOnInsert, ON CONFLICT that never assigns create_time); this commit gives the other three backends the same shape. OpenSearch: upsert() reads nothing again. The flush emits scripted_upsert update actions whose painless script replaces the business value and puts the stored create_time back (0 for a row predating the field), with retry_on_conflict so concurrent updates of one id resolve server-side instead of failing the bulk item. The buffered create_time is documented as an optimistic estimate the script supersedes. Redis: create_time is serialized as the FIRST key of every value, so an update recovers it with a bounded 64-byte GETRANGE instead of a full GET. A row that does not match the prefix (older layout, hand-edited) falls back to one full read and is rewritten in the new layout, so the fallback is self-healing. Undecodable storage now warns instead of silently recording an unknown. JsonKV: one dict lookup per key instead of __contains__ + __getitem__ -- two IPC round trips on a Manager().dict() proxy, the second shipping the whole value. Also: the invariant is now written down in BaseKVStorage.upsert (it binds five backends and nothing recorded it), stored values reach int through the shared normalize_kv_create_time (None/float/garbage were handled differently by each backend), and the painless semantics are pinned by opt-in integration tests validated against OpenSearch 3.6.0 and Redis 8. Fixes HKUDS#3870
…_time Two follow-ups on the create_time contract, both aimed at the same property: create_time is the moment the row was FIRST created, and only update_time may move afterwards. Redis -- concurrent first inserts could move create_time. Resolving the stored timestamp and writing the value are two round trips, so two writers that both saw the key absent each stamped their own clock and the later SET won, reporting a creation that never happened. Only the insert is contended: for an existing row every writer derives the SAME timestamp from the same stored value (preserved, or 0 for a legacy row, with deterministic normalization in between), so read-modify-write is idempotent there. A presumed insert therefore goes out as SET ... NX, and a writer whose NX is refused re-reads the winner's timestamp with one bounded prefix read and rewrites the row with it. The resolution step moved into _resolve_stored_create_times so both rounds share it. OpenSearch -- the painless script restored the stored value verbatim, so a row written by an older release kept its float or numeric-string shape. Mixed types across rows are enough to make the LLM-cache ordering in operate.py compare str with int and raise TypeError. The script now normalizes to a long, mirroring normalize_kv_create_time, which also repairs the row's shape on its next write like the other backends do. Integration tests pin the two normalizations to the same answers over int / float / numeric string / padded string / non-numeric string / null / array / zero; a boolean is out of scope (dynamic mapping types the field boolean and then refuses the long, loudly, and no release ever wrote one). JsonKV needs neither change: its resolve-and-write pair already runs under the namespace lock. BaseKVStorage.upsert now states the concurrency half of the contract -- the first creation defines create_time, and a backend whose insert is not naturally atomic must make it so. Verified against real services: OpenSearch 3.6.0 (20 integration tests) and Redis 8.6.3 (14). The Redis race has a unit-level reproduction that fails behaviorally on the previous commit (create_time 200 instead of 100), driven through the fake server rather than storage internals.
|
Thanks — the diagnosis and the invariant in the first two commits were right, and the real-workspace evidence in #3870 made the failure mode unambiguous. I pushed two commits onto this branch that keep your semantics and change how the three backends obtain the previous timestamp. Why the mechanism changedThe first version paid for the invariant with a client-side read-modify-write: OpenSearch issued an Mongo and PostgreSQL need no read at all, because they push the conditional write to the server ( ca89219 — server-side conditional writesOpenSearch. Redis. JsonKV. One Contract. The invariant binds five backends and nothing recorded it, so it is now written down in b647456 — two defects found while reviewing the aboveRedis: concurrent first inserts could still move OpenSearch: the script restored the value verbatim. A row written by an older release kept its float or numeric-string shape, and mixed types across rows are enough to make the LLM-cache ordering in JsonKV needed neither fix: its resolve-and-write pair already runs under the namespace lock. VerificationBoth backends were validated against real services, and those checks are committed as opt-in integration tests (marked
One thing worth flagging from that exercise: the Redis prefix regex initially did not tolerate Accepted residues, documented next to the code
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6474563bf
ℹ️ 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".
Two Codex findings on b647456, both about the same property: create_time is the moment the row was FIRST created. Redis -- SET ... NX closed the insert race but not the delete race. A delete() completing between a writer's read and its write let the row come back carrying the deleted incarnation's timestamp, and the docstring's claim that "the next write settles it" was simply wrong: every later update preserves what it finds, so nothing ever repaired it. Measured on the previous commit, an upsert racing a delete resurrected the stale timestamp in 39 of 40 rounds. Redis was also the only backend with the hole -- JsonKV serializes both steps under the namespace lock, Mongo and PG decide in one statement, and OpenSearch decides in the flush script. The decision now happens inside a Lua script, atomically with the write: absent key -> the write's own clock; prefix carries an int -> keep it; prefix does not match -> write nothing and answer needs_hint, so the caller reads the value, normalizes it through normalize_kv_create_time (kept as the single authority rather than reimplemented in Lua) and calls again. A non-empty hint always writes, so the exchange cannot loop. No cjson and no arithmetic in Lua: the timestamp travels as a string and is spliced in front of the payload, so a large row is never decoded server-side. SET NX and its repair round are gone, and the steady state drops from two round trips to one -- the script's own GETRANGE replaces the client-side prefix read. The caller's dict now adopts whatever the server decided. normalize_kv_create_time -- OverflowError was not handled. JSON 1e309 decodes to float infinity and int(inf) raises it, so one hand-edited row aborted the whole upsert instead of taking the documented 0 fallback. Verified against Redis 8.6.3: 23 integration tests, including the delete-race stress (0/40 resurrections, versus 39/40 on the previous commit), one-script-call command accounting, and an equivalence test pinning the Lua prefix pattern to the Python regex over nine raw shapes. FakeRedis cannot run Lua, so it models the script in Python and the unit tests pin the storage's side of the protocol.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 403618cfa9
ℹ️ 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".
GETRANGE answers "" for a missing key and for a key holding an empty string alike, so the upsert script classified an existing empty row as an insert and stamped it with the write's own clock. That is the direction issue HKUDS#3870 is about -- a fabricated create_time -- and it contradicted both the contract ("a stored row carrying no create_time records 0; never invent an original timestamp") and the corrupt-value row next door in the same function, which correctly records 0 with a warning. Redis was again the only backend to differ: JsonKV takes the update branch for an empty dict and OpenSearch finds a null prev, and both answer 0. The script now disambiguates with EXISTS, and only on the empty-prefix branch, so the steady-state update still reads once. An empty row therefore falls through to needs_hint, and the hint read no longer treats it as absent: only None counts as gone, so an empty string reaches the decode that records 0 and warns, like any other unusable value. Both new tests fail on the previous commit with the fabricated timestamp (1700000700 instead of 0) -- the unit one through the fake's model of the script, the integration one against Redis 8.6.3 after asserting that EXISTS and GETRANGE really do disagree there.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
What Problem
Replacement KV upserts were losing or resetting storage-managed
create_timewhen callers sent business fields only (common for entity/relation chunk tracking). JsonKV and RedisKV dropped the field on full value replace; OpenSearch reset it viasetdefault(create_time, current_time)on updates.Why
create_timeshould be assigned on first insert and stay stable. Mongo/Postgres already do this; Json/Redis/OpenSearch should match that invariant without merging stale business fields.How verified
pytestcreate_time suites + TestKVStorage + json copy-on-read → 34 passedFixes #3870