Repository navigation
Conversation
A delta snapshot holds the items appended since the snapshot before it, and the chain still links back through those appends. get_state_tail took the newest delta's items and then kept walking into the appends that delta already covers, collecting them a second time. With a delta every 3 appends, the last 6 of 11 items came back as [9, 7, 8, 9, 10, 11]. A tail within the newest delta, or as long as the state, was right. Past a delta the walk now skips what it covers until the snapshot before it, as reconstruction does. The differential tests for batched appends found this: two chains holding the same items took their deltas at different points, and only one of them walked past a delta. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
StateOperation::AppendMany(Vec<Vec<u8>>) appends several items to an append-log state as one state_update record, at one sequence, so that a presentation event in context-manager's per-agent index (#291) costs one record and not one per row. It follows the TreeBatch precedent: one record, one operation toward the snapshot cadence, however many items. Every read sees what the same items appended one Append at a time would give. Each place an operation's meaning lives has an arm for it: fold, length-only count and per-op apply in operations.rs; the head accounting (item_count by the batch's length, one op, append-only), each item into the branch's field indexes at its own ordinal (an unparsable item poisons them, as it would alone), the item cache advanced without re-materializing, and the head-record fast path for the last item in manager.rs; the tail walk, compute_delta_items (whose catch-all arm would have dropped a batch's items from the next delta snapshot) and the item iterator in store.rs. find_chain_head_at gets the count through array_len_from. update_state refuses an empty batch. The dead get_state_tail_full_reconstruct goes rather than gain an arm. It is a stored-format change: an earlier release fails to decode the variant, skips it during replay and then refuses the store as corrupt at the next update to the same state, or fails every read that reaches it. So the first AppendMany a store records rewrites MANIFEST from version 1 to version 2 with a feature bit for it, through a synced temporary file and a rename, before the record is written; if that fails nothing is recorded. Earlier releases refuse a version 2 store at open with "Unsupported store version: 2". A store that never records a batch keeps version 1 and opens everywhere as before. Opening a store whose replayed tail holds a batch the MANIFEST doesn't name marks it again, and this release refuses a feature bit it doesn't know. Tests: a differential suite writes the same random workload as single appends to one state and as batches to another and compares every read after each step and after a reopen; the materialization fuzz compares each batch with its expansion; batches go into the cache workload, the encoding round-trips, crash replay (whole batches kept or dropped, the MANIFEST marked again), per-branch field indexes and the branch_at accounting; and the MANIFEST tests check the bytes against the version check every earlier release makes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and record reads
context-manager's per-agent presentation index (#291) writes one batch of
entries per presentation event, and reads entries back by ordinal and
records by id. This adds the calls it needs to the store and the Node API.
Each is one call where the single forms take one per item.
appendManyToStateJson(stateId, items) records CH-1's AppendMany and returns
{ record, firstOrdinal }. The first ordinal is the state's item count on
the branch, read inside the update builder. The builder runs under the
write lock, so no other write can land between the count and the batch,
and item i is at firstOrdinal + i without a read. An empty batch throws,
because update_state refuses it. In Rust: Store::append_many_to_state(_on)
and append_many_to_state_json(_on), which return an AppendedBatch.
getStateItemsJson(stateId, ordinals) answers, for each ordinal in order,
what getStateItemJson answers, all from one version of the state: the warm
item cache, the head record when only the last item is asked for, or one
materialization for the whole call. A negative or out-of-range ordinal
answers null. In Rust: Store::get_state_items_by_ordinal(_on), served by
StateManager::get_state_items_by_ordinal. get_state_item's head-record
fast path moves into a helper that both use.
getRecords(ids) answers, for each id in order, what getRecord answers, and
null for an unknown id. The ids are looked up under one read of the id map
and the records read under one hold of the log file
(RecordIndex::get_offsets_by_id, RecordLog::read_many_at,
Store::get_records). A malformed id throws before anything is read.
A batch reaches subscribers as one state_delta event that carries
{"AppendMany": [[..], ..]}, and the event docs now say so. The README, a
changelog fragment, and the breaking fragment's "nothing writes one yet"
sentence are updated to match.
Tests: tests/batched_store_api.rs checks ordinals across single appends,
batches, redacts, snapshots and branches, under racing writers, after a
reopen and across interleaved states. It checks batched item reads against
cold single reads byte for byte after every kind of write, the head-record
path, get_records' order and unknown ids, and the state-delta event.
test-batches.mjs checks the same through the binding and runs in npm test.
The racing-writers test runs without automatic snapshots. With several
threads writing one state, an automatic snapshot reads the state after the
write lock is released, and can record stale content. That predates this
change, corrupts a state with single appends alone, and is not fixed here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… place store_blob wrote a blob's file at its final path and synced it, and returned early whenever a file was already there. A crash during the write left the file cut short, and every later store of the same content found it and returned, so the blob stayed torn and every get_blob of it failed. This was one of #291's smaller fixes. A store now writes the file under a .chronicle-blob-* name in blobs/, syncs it, and renames it into its shard, so a crash leaves the file whole or absent. On Unix it then syncs the shard's directory, and blobs/ when the shard is new, using the directory-sync helpers the metadata checkpoints use: a blob whose store returned survives a power loss where the system syncs directories. Store::create and Store::open remove the staging files that stores killed before their rename left, with the lock held. The staging files lie beside the shards, so list and total_size skip them, in every release. A store of content that isn't cached now reads the file under its hash and returns early only when get would return it; a file that isn't whole, or holds other content, is replaced. A whole one is left as it is, with the content type it was first stored with. get reads the file in one read and parses it with one decoder that checks each length against the file's size first, so a damaged length is an error and no longer an allocation of that many bytes. Its errors are otherwise the ones it gave. A content type longer than its 16-bit length field is refused, where its length used to wrap and write a file no read could parse. The blob file format is unchanged. Earlier releases open a store this version wrote. Tests: storage.rs unit tests replace a file torn at each field and one zero-filled past its first page, replace a file holding other content, leave a whole file untouched, keep get's errors for each kind of damage, refuse a damaged length without the allocation and an over-long content type, race stores and reads of one blob, and run a child that exits in the middle of writing to show nothing reaches the final path and the sweep removes the stage. tests/blob_files.rs checks the repair, the sweep at open, and that a locked open leaves staging files alone, through the store. Against the old store and get, the torn, other-content, damaged- length, content-type and killed-store tests fail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RecordIterator::next read a record with read_at, released the file, then took the file again, sought back and read the whole record a second time to learn where it ended. The index rebuild at every open goes through it, so open read each record three times: once in the validating scan, twice in the rebuild. #291's per-agent presentation index adds records, so this was one of its smaller fixes. next now seeks, reads the record and takes the stream position under one hold of the file, so it reads each record once. Holding the file from the seek to the position matters: any other read moves the shared offset. An error ends the iteration after it is returned, as before; a failed position query is now returned too, where it used to end the iteration silently. Store::iter_from(_on) and the record replay in catchUpSubscription use the same iterator. Measured with a new ignored probe (perf_probe.rs, probe_open_time_over_a_large_log, release build, warm page cache, min of 3): with 50,000 small records, open went from 732 ms to 563 ms and one iterator pass from 478 ms to 284 ms; with 200,000, open from 2.90 s to 2.00 s and one pass from 1.89 s to 0.98 s. Open doesn't halve, because the validating scan is still a pass of its own. Tests: a test-only count of record decodes shows one pass of the iterator decoding each record once (24 for 12 records before, 12 now) and an open of a store decoding each twice (3n before, 2n now); boundaries are right for records of varied lengths and links and from a middle offset; reads between steps don't move the iterator; and a damaged record ends the iteration with its error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, so it can't drop a batched store's mark Store::create wrote MANIFEST, truncating it to format version 1, before it took the lock, and then went on to create over a directory that already held a store. Aimed at a store that had recorded an AppendMany, even one open elsewhere so that create then failed with Locked, it dropped the version 2 mark. The open handle already named the feature, so later batches didn't mark the store again, and once they were behind the checkpoint no open did either, so a build that predates batches opened the store and failed reading it with "unknown variant `AppendMany`" (CH-A). This carries #34's fix (origin/fix/create-refuses-existing-store: 66e5231, 0b8a741, ae6750b, ff2c733), merged by hand onto the version 2 MANIFEST code. create takes the lock before it writes anything, refuses with InvalidOperation a directory holding MANIFEST, records.log, state.bin or branches.bin, sweeps staging files, and writes the version 1 MANIFEST through atomic_file, so it never truncates an existing one. verify_manifest still returns the feature bits, and for a directory with no MANIFEST it now says whether that is a damaged store or none. open_or_create decides by MANIFEST: a store opens and a missing path is created, as before; an empty directory, a fresh volume's root, or a create that stopped before its manifest is now created into; and a directory of other files is still refused, now with InvalidFormat naming a file instead of a bare IO error. #34's tests and fragments come along unchanged. The breaking fragment now says that create refuses an existing store, that before that fix it dropped the mark, and that an earlier release's create still does; that an open marks the store again only for batches past the checkpoint; and that when a batch is its state's newest update past the checkpoint, an earlier release opens the store with a warning and that state silently lacks the batch's items (CH-K). The README says create never writes a store's MANIFEST back to 1. Tests: append_many.rs gains a create aimed at an open batched store (fails with Locked, and the MANIFEST bytes stay the same through a later batch, a close and a reopen, so an older build still refuses it) and a create over a closed one (refused with InvalidOperation; MANIFEST and value kept). test-batches.mjs makes the reviewers' probe a check on the Node API. All three fail on the code before this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e blob write's syncs and failure paths store_blob checked that a content type fits its 16-bit length field only in write, after the early returns for cached content and for a whole file already stored. So a content type over 65,535 bytes was refused for new content but accepted, and the hash returned, for content already there (CH-G). store now checks it before anything else, so the refusal doesn't depend on what the store holds; the doc, the fixed fragment and the README say so. Nothing tested the directory syncs that make a renamed blob survive a power loss: taking both sync_directory calls out of write passed every test. A test-only thread-local list of synced directories, like RECORD_DECODES, now shows a new blob syncing its shard, and blobs/ first when it made the shard; a store of cached content, or of content whose whole file it finds, syncing nothing; and the replacement of a torn file syncing only its shard. A test-only fault on the storage stands in for a device error at the rename and at the shard sync after it. A failed rename removes the staging file, returns the error, leaves a file at the blob's path as it was and caches nothing. A failed sync after the rename returns the error with the blob in place and nothing cached, and the next store of the content finds the file whole and returns the hash without that sync, which store's doc now spells out (the fix for that masking is CH-D, left for later). tests/blob_files.rs adds the content-type refusal through the store, cached and after a reopen, and a real failed rename, with a directory in the way at the blob's path. store and get also say that a cached blob is answered from the cache, so damage done to its file from outside the store shows only once the blob leaves the cache or the store is reopened (CH-I). The content-type tests fail on the code before this change. Removing either directory sync, or keeping the staging file when the rename fails, fails the new sync and rename tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…batch's one record
context-manager's window index writes each entry with
appendToStateJsonWithIdentity, which stamps the record id and sequence into
the item, so a write-down or a presentation costs one record per row. This
is the batched form it needs to write them as one record (CH-4 of #291's
step 3, ahead of CM-9; AF-6 is gated on both).
appendManyToStateJsonWithIdentity(stateId, items, idField, sequenceField)
in the Node API, and Store::append_many_to_state_json_with_identity(_on) in
Rust, record one AppendMany and return { record, firstOrdinal }, as
appendManyToStateJson does. Before the record is written each item gets the
id and sequence it is about to take, exactly as the single form stamps its
one item: the id as a string at idField, the sequence as a number at
sequenceField, overwriting what was there, serialized by serde_json. Both
forms now stamp through one helper, stamp_identity. The batch is one
record, so every item of it carries the same id and sequence, and a branch
cut at that sequence keeps the whole batch while one at the sequence before
drops it. An empty batch, or one with an item that isn't an object, is
refused before anything is written, MANIFEST included. append_many_to_state
and the new call share append_batch_with_builder, which reads the first
ordinal under the write lock as before.
The MANIFEST flips to version 2 on the first batch, by either call, as
today: a store that only takes single identity appends keeps version 1 and
opens on older builds. The docs, the README and the breaking fragment say
so, and a new added fragment describes the call.
Tests: in tests/append_many.rs, a differential runs one random workload on
two stores, identity appends (single and batched) on one and plain appends
of the same items stamped by hand with the twin's record on the other; the
records, every read, the field indexes on the stamps and on the items' own
ids, and branch cuts at and before each batch agree byte for byte, after
each step and after a reopen. Others check n single identity appends
against one batch (stamps name the record that holds each item; the bytes
match once the stamps are swapped; /id answers each ordinal; cuts drop a
single item or a whole batch), the refusals, the MANIFEST mark and the
older-build check, and a batch on another branch. A crash scenario in
tests/crash_state_recovery.rs replays identity batches past the checkpoint
and reads them again from the checkpoint. test-batches.mjs checks the same
through the binding.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and the snapshot stays due update_state_with_builder writes the record, moves the state's head and the branch head, and only then takes the snapshot the write's thresholds call for. It returned that snapshot's error with `?`, so every write form threw for a write already recorded: update_state and every call through it, appendToStateJsonWithIdentity, appendManyToStateJson and appendManyToStateJsonWithIdentity among them (issue #39). The review of CH-4 reproduced it with a poisoned item (J-CH1): the identity batch threw, yet its sequence and items survived a reopen. context-manager's window index and agent-framework's adopt read a throw as nothing written, and a caller that retries a write that isn't idempotent makes it twice. The realistic trigger is a full disk failing the larger delta snapshot that follows every 50th window write. The snapshot's failure is now logged as a tracing warning naming the error, state, branch and sequence, and the write returns its record. The state's count of operations since its last snapshot isn't reset, since only a snapshot record resets it, so the next write to the state finds the snapshot due and tries it again. Refusals before the record (an empty batch, an item that isn't an object, a MANIFEST that can't be marked, a pending checkpoint) are unchanged and still write nothing. A call whose own work is the snapshot, compact_state or create_snapshot_if_needed, still returns its error. The Rust docs of update_state, append_to_state_json_with_identity, append_many_to_state and append_many_to_state_json_with_identity, and the napi docs of appendToStateJsonWithIdentity, appendManyToStateJson and appendManyToStateJsonWithIdentity, now say a recorded write doesn't throw from its own snapshot, which is logged and retried; update_state's #39 caveat is gone. A fixed fragment (39-snapshot-after-write) describes the change, and append-many-identity.added.md points to it. Tests: tests/auto_snapshot_failure.rs writes a non-JSON item to a state with a delta every 2 operations, then each form (update_state Append, the single identity append, the plain and the identity batch): the write returns its record, no snapshot record follows, the length advances, the warning is logged, the next write tries again and logs again, the explicit create_snapshot_if_needed still errors, and all of it survives a reopen, where the snapshot is still due. A second test, with a full snapshot every 2 operations, shows the snapshot the failure left due taken by the next write that can take it, a Set that replaces the poisoned value. Both fail on the unfixed code. test-batches.mjs turns the reviewer's throw-after-write probe into the same checks through the binding (FW-CH-1). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ninth catch-up The catch-up brings #60's blob staging. Two files conflicted, both between #60 and this branch's CH-3 (9370ab7) and FW-CH-2 (a67b7d6). Both sides fixed the same thing, so main's shape is taken, and only what main lacks is kept from this branch. src/blobs/storage.rs is main's file whole. That covers the staging-name write, the per-shard durability counters, the directory syncs, the read-back check with links and non-regular files, and the refusal of a newer blob format. Main checks the content type first, as FW-CH-2 did. Main also retries a failed sync at the next store of the same content, which is the CH-D fix FW-CH-2 had left open. Kept from this branch: - The test-only SYNCED_DIRECTORIES list, now recorded in main's sync_directory. - The fails_at/WriteStep fault hooks, now at main's rename and at its sync after the rename. - Doc lines on remove_staging_files, exists, total_size and the cache. - Eight unit tests that still apply to main's code: - a file torn within each field, empty or zero-filled is replaced; - a whole file is left, and the cache answers its first content type; - an over-long content type is refused for content already stored; - the syncs, rewritten to main's order (the parent entry once per storage, blobs/ for a new shard, the shard after a rename); - a failed rename leaves a damaged file at the path as it was; - a failed sync after the rename, rewritten and renamed: the test used to pin the masking, and now pins main's retry; - racing stores and reads; - the sweep removes only staging files. Dropped from this branch in favour of main's code and tests: - The one-read decode() parser. Main's read_file has the same check of each length against the file's size. - The warning logged once for a directory the process may not open. Main skips that sync without a log, as checkpoint saves do. - The warning logged before a file that isn't whole is replaced. - The EXIT_WHILE_WRITING killed-child test. Main's SIGXFSZ tests cover a store killed partway. - Two tests that pinned get's old errors. Main reports a cut-short file as damaged, naming the file, and a newer version as unsupported. - The duplicate other-content and over-long-type tests. The error texts are either/or: main's "can be at most 65535 bytes ..., so nothing was stored" and "cannot rename the new blob ... into place" replace this branch's. In src/store.rs both sides added blobs.remove_staging_files() at open. Main's line is taken without this branch's comment, because the function's doc now says why the sweep is safe. The blob sweep in create stays, since main's create lacks it. #46 is not in 515ce0f, so 570ef2a's create is unchanged. Resolved outside the conflict markers: - src/atomic_file.rs merged cleanly. sync_directory and directory_open_refused are private again, as on main, because main's sync_directory_at now serves the blob store. STAGING_PREFIX stays pub(crate) for create's foreign_entry. - tests/blob_files.rs drops two tests: - a_torn_blob_is_replaced_by_storing_its_content_after_a_reopen repeats main's cut-short case. - a_store_whose_rename_fails_removes_its_staging_file no longer reaches a rename: main refuses a directory at the blob's name before it writes. Main's directory test and rename test cover both cases. - changelog.d/atomic-blob-writes.fixed.md is cut to the one note that main's blob-staging.fixed.md lacks, the unchanged file format. Its other bullets duplicated main's or were no longer true: the warning logged once, and get's errors unchanged. AppendMany, the identity batch, the napi batch reads, the get_state_tail delta fix and 72a5388's post-write auto-snapshot fix merged without conflicts. Main lacks them, and they are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…enth catch-up The catch-up brings main's #56 as the Dendrite branch settled it: the non-append flag a createBranchAt fork takes from its chain, a Delta counted as a non-append change, the full-snapshot fallback for a delta compute_delta_items refuses, getStateTail capped at `count`, the Redact range clamp in the field index, and the chain walks through one ChainWalk (src/state/chain_walk.rs). One file conflicted, in three places. src/store.rs: - get_state_tail_on, the walk. This branch's e7639b9 walked the chain by hand with a `hit_delta` flag that skipped the appends a delta snapshot holds; the catch-up walks through ChainWalk, whose behind-delta skip does the same, and reads the whole value, sliced to `count`, when `count` reaches the stored item count. The catch-up's walk is taken. - get_state_tail_on, the arms. This branch's AppendMany arm (newest item first, only as many as the tail still needs) is kept, beside the catch-up's take_last for DeltaSnapshot, Snapshot and Set and its is_non_append_change arm, which sends a Redact, an Edit or a Delta to the full read. - compute_delta_items, doc and signature. This branch documented the AppendMany items it collects; the catch-up returns Result<Option<Vec<u8>>>, None where the walk meets a Redact, an Edit, a Set or a Delta, and create_snapshot_if_needed_internal then takes the full snapshot. The catch-up's signature is taken, with a doc that names both. The body (this branch's AppendMany arm, the catch-up's is_non_append_change arm) merged cleanly. Resolved outside the conflict markers: - store.rs test_compute_delta_items_takes_every_item_of_a_batch unwraps the Option compute_delta_items now returns. - changelog.d/state-tail-delta.fixed.md is removed. It described e7639b9's fix, and #56's fork-keeps-non-append.fixed.md has a bullet for the same fix ("getStateTail no longer returns some items twice"). Merged without conflicts, and checked against this branch's batches: - manager.rs record_update_with sets the flag through is_non_append_change before its match, and keeps the AppendMany arm. AppendMany is not a non-append change, so a delta snapshot still consolidates it. - StateItemIterator reads its operations through ChainWalk and keeps the AppendMany arm. - find_chain_head_at counts an AppendMany as one operation, as record_update does, and array_len_from counts its items. - src/napi.rs is unchanged. getRecords reads the record index; appendManyToStateJson and appendManyToStateJsonWithIdentity write through update_state_with_builder and record_update_with and read firstOrdinal from the branch's head; getStateItemsJson reads the branch's head, the head record, or a cold value through reconstruct_from_disk and ChainWalk; getStateTail goes through the capped get_state_tail_on. cargo test: 211 lib and 320 integration tests pass (append_many 17, batched_store_api 11, damaged_branch_reads 9, fork_keeps_non_append 10, redact_field_index 1, selective_fork 5, branch_at_snapshot_accounting 6; perf_probe's 4 and the 3 doc-tests ignored). The timing tests (reconstruction_scaling, scaling) passed in the full run at a load average near 4. `cargo test --features napi-bindings` was not run; its binaries do not link on this Mac (`_napi_create_error`). Native module rebuilt (npm run build); index.d.ts carries getRecords, appendManyToStateJson, appendManyToStateJsonWithIdentity and getStateItemsJson. test.mjs (40), test-views.mjs (14), test-batches.mjs (17) and the release-script suite (29) pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Eleventh catch-up merged into this branch: — FableDendrite |
…'s count fix nissa's house's first pushes onto the Dendrite branch: a Delta sets an append-log state's item count as a Set does, and a replace with no bytes sets it to 0 (54d1b7d, tests/replace_counts.rs), with two changelog commits. The -main twin is retired; this branch follows the PR branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The house's count fix merged into this branch: — FableDendrite |
…'s changelog narrowing and the known_full retry test Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Also merged: the house's — FableDendrite |
…eleased #40 is merged and published; this branch now sits on the release. Its own release is the next minor the registry gives it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
0.5.0 is published from main without the batched append (appendManyToStateJson, appendManyToStateJsonWithIdentity, getStateItemsJson, getRecords), so this branch's release is the next minor the registry gives it; context-manager's and agent-framework's presentation floors rise to ^0.6.0 beside it (review finding CH-1). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Review pass, chronicle half: core batch code checked out (item counts after a batch, the capped tail and delta snapshots over batches, forks never splitting a batch, per-item field indexing, identity on the batch's one record, torn batches dropped whole). One should-fix, taken: this branch's release is 0.6.0 — 0.5.0 is published without the batched append — so it's numbered here (3a7dd82) and the dependents' floors rise to — FableDendrite |
…cut with npm version minor (0.6.0) Reverts the version fields of 3a7dd82. Per CONTRIBUTING ("Version bumps are a maintainer release-time action"), a feature branch keeps main's version (0.5.0); the maintainer's `npm version minor` folds the fragments and cuts 0.6.0, the release that carries the batched append (review finding CH-1; the process decision follows finding CM-2 on context-manager). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y request reads nothing (CH-2) getStateItemsJson is documented to return, for each index, what getStateItemJson returns for it. It didn't when no index could be answered from the head: an empty request, or one of negative or out-of-range indices, fell through to the full materialization, which on a state whose value isn't an array threw "invalid type: map, expected a sequence" where getStateItemJson(-1) answers null. - napi: getStateItemsJson answers a negative index null without a read, as getStateItemJson does, and sends the store only the others; a request with none (empty, or negative only) returns before touching the store. - StateManager::get_state_items_by_ordinal returns an empty list for an empty request before reading anything. - Both point lookups (get_state_item, get_state_items_by_ordinal, so getStateItemJson and getStateItemsJson) answer None for a state whose value isn't an array (serde_json's data-error category), where the single form also threw for an index of zero or more. get_state_items and get_state_slice still return the error. The new item-lookups-non-array.changed.md fragment says so for getStateItemJson, which 0.5.0 ships. - An index of zero or more past the end still costs, on a cold cache, the one materialization getStateItemJson pays for it: the head's item_count isn't exact for every store (counts stored by older builds stay as they were), so answering from it could return null for an item that exists. Tests: batched_store_api.rs a_state_whose_value_is_not_an_array_has_no_items (Snapshot states holding an object and a string, an append-log state Set to an object, cold and warm; the slice still refuses) and an_empty_request_reads_nothing (no materialization on a cold state; a mixed in/out-of-range request equals the single reads from one materialization); both fail on the unfixed code. test-batches.mjs: [-1], [], [0] and a mixed request on a Snapshot state holding an object, negative-only and mixed requests on the append log. batched-calls.added.md now says what the code does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… (CH-3)
AppendLogViewer handled only `operation.Append`, so a state written in
batches (appendManyToStateJson / appendManyToStateJsonWithIdentity, one
state_delta event `{"AppendMany": [[..], ..]}` per batch) never updated its
live view or totalCount until a refetch. It now decodes each item of an
AppendMany as it decodes an Append's item, in order, prepends them newest
first and adds the batch's length to the counts.
Not verified by a test: the dev UI has no tests and no installed
dependencies (no vue-tsc). The handler's script logic was type-checked with
tsc --strict and run on simulated Append / AppendMany events outside the
repo; the component was not rendered.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ecode (CH-4) The "Without the mark" paragraph described 0.5.0's behaviour as every earlier release's. Read from the tags: - 0.5.0 (v0.5.0 = origin/main 2529765, Store::open): past the checkpoint, RecordIndex::rebuild_from_log_with replays state updates, skips one that doesn't decode (counted, warned), and fails with Corruption ("cannot replay state ... does not match checkpoint/replayed head") at the next update whose prev_update_offset names the skipped batch; with no such update it opens with the warning and the state lacks the batch. - 0.4.0 and every earlier tag: open calls RecordIndex::rebuild_from_log with no replay, so the state is what state.bin's checkpoint holds; a batch past it, and every other update past it, is left out without a word. - 0.3.0 to 0.5.0 decode state updates as MessagePack with no AppendMany variant, and rmp-serde writes variant names, so a chain read reaching a batch before the checkpoint fails with "unknown variant `AppendMany`"; 0.2.7 and earlier decode state updates as JSON only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Fix wave, chronicle half (four commits, head 8c63ff3):
— FableDendrite |
There was a problem hiding this comment.
Automated review
Recommendation: merge. The review found nothing of medium or high severity.
Reviewed commit 8c63ff3 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. This fits. It adds batched appends and batched reads to chronicle's append_log states and Node API, which is the storage layer's own job.
5 reasons with sources
The on-disk format change is opt-in, gated by a MANIFEST version, and documented the way CONTRIBUTING.md asks.
- Batched appends and batched reads extend features the store already has, through the same layers those features use. The README describes append_log states read with getStateItemJson, getStateSlice and getStateTail, and records read with getRecord. The diff adds AppendMany to StateOperation in src/state/operations.rs and wires it through update_state in src/state/manager.rs. It adds getRecords and getStateItemsJson in src/store.rs and src/napi.rs. That is the same core-then-napi layering the existing calls use. (README.md (What a store keeps: Records, States))
- The change to the on-disk format is the 'sharp edge' CONTRIBUTING.md names, and the PR handles it as CONTRIBUTING.md asks: it is opt-in, versioned and documented. MANIFEST moves to version 2 only when a store first records a batch, so older builds refuse such a store cleanly. Stores that never write a batch stay at version 1 and still open on earlier releases. changelog.d/append-many.breaking.md sets out who needs to act and what migration is possible. Because a store with a batch can't be rolled back, a maintainer should consciously accept this, but it is not out of scope. (CONTRIBUTING.md (What a PR should contain: format and on-disk compatibility))
- The PR declares that it is stacked on another PR and that it has companion work in other repositories, as the contributing guide requires. The body says it is stacked on #40 and that it is the chronicle part of agent-framework#291. It names the context-manager and agent-framework branches that will depend on ^0.6.0. CONTRIBUTING.md notes that changes here often need companion work across the ecosystem, which the README lists as chronicle's consumers. (CONTRIBUTING.md (How changes land))
- The two fixes bundled in, to get_state_tail and to auto-snapshot failures, concern behavior the store already has and are tied to this feature's write path. The tail fix concerns delta snapshots, which the README documents. The snapshot fix stops a write from throwing after its record is already durable, which bears directly on the durability guarantees. Both were found by this feature's tests, so bundling them is reasonable. The blob-storage diff is mostly test hooks and tests for behavior that is already on main. (README.md (States, Durability))
- The supporting edits sit where the project keeps such things. Changelog fragments go in changelog.d/ under the prescribed names. Rust tests go under tests/. The inspector UI in ui/ gets a small change so AppendLogViewer can decode the new AppendMany operation. (CONTRIBUTING.md (Changelog); README.md (Looking inside))
Findings
No findings of medium or high severity.
3 more findings, less severe
- Low severity, maintainability,
src/store.rs:633-641. getRecords holds the log file's write lock for the whole batch, with no limit on its size.get_recordspasses every offset it finds toRecordLog::read_many_at, which takesself.file.write()once and reads every record while holding it. A largeidslist from JS (getRecordshas no limit), or one with large payloads, blocks every append,read_atand iterator on the log until the whole batch is read. An ID that repeats is also read again from disk. Read in bounded chunks, or release the lock between records, so a large read can't stall writers. - Low severity, documentation,
changelog.d/atomic-blob-writes.fixed.md:1-4. Changelog fragment repeats a blob change that an earlier release already shipped. CHANGELOG.md already describes the staging-name blob writes as released (line 352, thestoreBlobstaging-file entry). The PR description also says this work is now main's (#60), taken whole. Even so, the PR addschangelog.d/atomic-blob-writes.fixed.md, and the release script will fold it into 0.6.0's Fixed section, so the 0.6.0 notes will present a past change as new. Drop this fragment, or fold its compatibility note into the original entry. - Low severity, documentation,
changelog.d/34-create-existing-store.fixed.md:1-2. #34 create-fix fragments ship in a PR that says the fix belongs to main. The description says the create-takes-the-lock-first fix is now main's (#46), taken whole, with only this PR's tests kept. Yet the PR adds34-create-existing-store.fixed.mdand.changed.md. If main already carries fragments or CHANGELOG entries for #46, these will either produce duplicate entries or conflict at release time. If it doesn't, the description misstates where the fix lives. Make the fragments and the description agree.
For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.
…_missing says open_or_create opened a directory only when it held a MANIFEST. A directory holding a store's data files without one (records.log, state.bin or branches.bin) passed foreign_entry, whose allowed set names those files, and then went to create when create_if_missing was set, which refused it as damaged after creating LOCK there, or got NotInitialized when it was unset. NotInitialized reads as "no store here" and invites a create, which then refuses the directory as damaged; open_or_create's doc promised the damaged refusal either way. Now any directory holding one of a store's files goes to open, which refuses a damaged store with InvalidFormat naming the file before it takes the lock, so open_or_create writes nothing there whichever way create_if_missing is set. NotInitialized is left for a directory a store could be created in. foreign_entry, whose only caller is open_or_create, no longer allows the store's file names: a directory holding one now goes to open first, so the allowance could only take effect where a directory lists a name it won't stat, and its doc's clause that create refuses those files no longer described what happens. The 34-create-existing-store.changed fragment's open_or_create sentence said "any directory with no MANIFEST" gets NotInitialized with create_if_missing unset, though one holding other files got InvalidFormat first; it now names both cases and the damaged one. The doc on open_or_create says what it now decides by, and the test file's header drops its clause saying it decided by MANIFEST. Tests: open_or_create on a directory holding each data file alone, with create_if_missing set and unset, fails with InvalidFormat naming the file and "damaged", and leaves the directory holding that file alone. The link test from the previous commit now expects open's InvalidFormat from open_or_create. create_refuses_each_data_file_of_a_store_without_its_manifest no longer checks open_or_create, which the new test covers.
…lock open read the MANIFEST's format features before acquire_lock and kept them: they seed the store's record of what the MANIFEST names, which the open-time re-mark and every later require_features write back as `features | required`. A process that held the store could mark a feature and close between that read and the lock. Then this open admitted the store with the bits it had read before, and its next mark rewrote the MANIFEST without the feature that process added: with a build that knows a feature this one doesn't, the very bit that keeps this build out of a store it can't read. Features are meant to be only ever added. open now reads the MANIFEST again once it holds the lock, when no other process can be writing it (create and every mark write it under the lock), and takes the features from that read. The read before the lock stays, so a directory that isn't a store is refused before LOCK is created in it. No test: the window lies between two calls, and a test can only race for it.
…t holds for root A first batch whose MANIFEST couldn't be rewritten returned the bare I/O error of whatever step failed. Where the directory won't take the staging file, that's creating it, which names neither the file nor the step, so the caller read "IO error: Permission denied (os error 13)" and couldn't tell whether the batch was recorded or a retry was safe. require_features now wraps an I/O error with the MANIFEST's path and that the batch was not recorded, keeping the error's kind. a_batch_whose_manifest_mark_fails_is_not_recorded made the mark fail with a read-only directory and returned before any assertion where that didn't stop a write, as under root, leaving a stray probe file. It also checked only in-memory state after the failure, so a batch appended to records.log before a failed mark would have passed it and come back at the next open's replay. It now puts a directory where the MANIFEST was, which a checkpoint save refuses for every user (atomic_file: "isn't a regular file, so nothing was written"), checks the error's message, restores the MANIFEST, and reopens the store to check the head, the record count, the value and a version 1 MANIFEST before the retry goes through.
…warning carries its sequence contains_append_many looks inside a Field so that a record holding a batch anywhere, which a build that predates batches can't decode either, marks the MANIFEST before it is written. The only Field-wrapped batch in the tests was the empty one, refused before any mark, so nothing showed a nonempty nested batch marks it. a_batch_nested_in_a_field_marks_the_manifest records one on a Snapshot state and checks the value and a version 2 MANIFEST. The 39-snapshot-after-write fragment promises the warning names the error, state, branch and sequence; the failure test checked the first three. It now checks the sequence field is the recorded write's, the field that says which write's snapshot failed.
…eps or drops a batch whole The MANIFEST is marked before a batch's record is encoded and appended, as it must be for an older build to refuse a store holding one, so a first batch whose record then fails to be written (an append error on a full disk, as well as a crash) leaves a version 2 store with no batch. Five texts said a store that never records a batch keeps version 1: the breaking fragment's Unchanged line, README's On disk paragraph, the docs of update_state and append_many_to_state_json_with_identity, and the napi doc of appendManyToStateJsonWithIdentity. They now say it of a store never asked to record one, or, in the README and napi, key the version on the first batch that passes its checks, which an empty or refused batch doesn't; the README, update_state and napi say what a first batch that fails after the mark leaves, and the fragment's Migration paragraph names the error case beside the crash. A batch is one record at one sequence, so a historical read or a fork cut by sequence keeps or drops it whole, as the identity forms' docs and fragment already say. The append-many.added fragment listed historical reads and forks among the reads that match per-item Appends, and the docs of StateOperation::AppendMany and append_many_to_state said "every read"; each now states the cut as the exception.
…tch's failed mark says only what holds
present_store_file answered "absent" for any error from symlink_metadata, so
create and open_or_create went ahead where they couldn't tell whether a store
file was there; for a store path that is a regular file, open_or_create
went on to create. It now answers only NotFound with absent and returns any
other error, naming the path and the file it was checking, its kind kept.
open's message for a MANIFEST that is a symbolic link to nothing said the
link's target doesn't exist, which is wrong for a chain of links whose first
hop is itself a link; it now says the link leads to nothing, naming its
first hop.
The batch-mark error said it "cannot mark" the MANIFEST even when the new
MANIFEST was in place and only the directory sync after the rename failed,
which the inner error says. It now says the batch was not recorded because
of an error marking the MANIFEST, and leaves the MANIFEST's state to the
inner error; update_state's doc says the MANIFEST is already at version 2 in
that case.
The open_or_create doc said a directory whose MANIFEST is missing or
unusable is refused "as a damaged store", but a bad magic or version gets
open's own account; it now says open's account of what it found. It also
names a checkpoint's staging file, as a create leaves when it stops during
its MANIFEST save, among what a new store can sit beside. foreign_entry's
private doc goes: it described a private function, and its staging clause
was narrower than the check.
Tests:
- The store-file link test calls open_or_create first, with
create_if_missing set and unset, and checks the refusal is the presence
check's ("leads to nothing" for the MANIFEST, "damaged" for a data file,
which the foreign-entry refusal can't be taken for) and that only the link
is left, then that create refuses and writes nothing through the link.
- open_or_create on a store path that is a regular file fails with an I/O
error of kind NotADirectory naming the path, with create_if_missing set
and unset, and leaves the file alone.
- The mark-failure test checks the error keeps its kind (InvalidInput, a
MANIFEST that isn't a regular file), retries on the live handle and checks
the whole version 2 MANIFEST then, which a reopen before the retry had
stopped checking, and reopens to find the one batch. The nested-batch test
checks the whole MANIFEST, feature bit included.
…e Node batches README and the napi doc of appendManyToStateJsonWithIdentity said the first batch that passes its checks moves the MANIFEST to version 2, but the mark can fail after the checks and leave version 1. They now say it moves just before the store's first batch is written, stays even if that batch then fails, and that a failed move throws without recording the batch (the MANIFEST may already be at version 2 when only its directory sync failed). The append-many.added fragment's lead said "Every read" before a list that no longer holds every read; it says "These reads". README's and napi's texts for appendManyToStateJson, and the batched-calls fragment, said reads see the items as if each were appended alone, with no cut rule; they now say a historical read or a branch cut by sequence keeps or drops the batch whole, as the identity forms' texts do.
… account of one says how to repair it create refused a directory whose MANIFEST is a symbolic link to nothing with "already holds a Chronicle store; open it with open or open_or_create", which open then refused as a link that leads to nothing. create now says the MANIFEST is such a link, naming its first hop, that it won't write through it, and to open the store once the link leads to the store's MANIFEST. open's account of such a MANIFEST said the store can't be opened "until what it leads to is back", which never comes for a link pointed at the wrong place; it now says the store can't be opened until the link leads to the store's MANIFEST again, by mounting the volume it points into or pointing the link where that MANIFEST is. Both messages share link_to_nothing. Tests: the store-file link test checks that create's refusal for a MANIFEST link to nothing says it leads to nothing rather than that a store is there, and the file-as-store-path test checks the error names the MANIFEST it was looking for as well as the path.
The MANIFEST mark is an atomic rewrite: the new MANIFEST is renamed into place, then its directory is synced, and a failed sync is returned after the rename. A first batch whose mark fails that way throws without being recorded, with the MANIFEST already at version 2, as update_state's doc says. README and the napi doc of appendManyToStateJsonWithIdentity said only that a failed move throws and records nothing, and the breaking fragment counted only failures after the rewrite, by a crash or by an error writing the record; each now names the directory sync too.
…nd open's link message fits a link never right The breaking fragment said a first batch that fails once the rewritten MANIFEST is in place, by a crash or an error, leaves a version 2 store that holds no batch. A crash after the batch's record is written leaves the batch in the store, and a power loss before the MANIFEST's directory sync can bring back version 1. It now says this of a batch whose MANIFEST rewrite is in place but whose record never is, and that it can leave such a store, as the sentence it replaced said of a crash between the rewrite and the record. open's account of a MANIFEST that links to nothing said the store can't be opened until the link leads to the store's MANIFEST "again", which doesn't fit a link pointed at the wrong place from the start; the word goes.
- The fixed-in-0.5.0 fragment for staging-name blob writes goes: 0.5.0's CHANGELOG already carries that change, and this branch's blob write path differs from main's only by test seams, so 0.6.0's notes would have announced it again. - The injected shard-sync failure now fails inside sync_name, where the real sync would, so the test of a failed sync being tried again also checks that a failed sync isn't counted as done. - BlobStorage::exists' doc no longer says get checks a blob is whole: get answers a cached blob without reading its file, and checks the content, not the content type. - blob_files: a staging file is left out of the blob count and size while it is there, checked with the store open, before the sweep at the next open removes it. - materialize_equivalence: a batch inside a Field is compared against one Field append per item, not against an unchanged copy of itself. - state_update_encoding: the batch's large item has bytes of 128 and over, which a per-byte integer encoding would double, so the size bound tells a bin item from a sequence. - branch_at_snapshot_accounting: the inherited-batch case checks that each child took a delta snapshot past the threshold, and no full, so it tests the delta over an inherited batch it is named for.
…ging file The staging-file check compared the store's blob count and size before and during the staging file, so a store reporting no blobs at all passed it. It now also checks the one stored blob is counted, and that its bytes are, as the test before this branch's rewrite did for the count.
The size check asked only for at least the content's four bytes, so a size that counted the blob twice, or added a fixed amount, passed. It now compares the reported size with the blob file's own length on disk, taken before the staging file is written.
|
@tel-0s We reviewed #62 at What the commits change comes first, then the #46 line, then what they leave as 0.5.0 had it, then the runs. Line numbers are What the commits changeStore files, Meg-2646's eleven commits:
Tests and one fragment, Basil-1007's three commits:
#46#62's description says "Blob staging-name writes and the create-takes-the-lock-first fix were ours too; both are now main's (#60, #46)". #60 merged on Oct 26. #46 is open, at What the commits leave as 0.5.0 had itOpening a store whose
A store directory that is itself such a link gets Runs
Not verified
Meg-2646 made the eleven store commits and wrote the link test file, and Basil-1007 made the three test commits. An internal review read #62 in four areas, and the set twice on earlier versions, once whole and once what changed between them. Ambrose-2449 weighed what each part costs. Lottie-2661 made the runs at — Lottie-2661 |
…econd MANIFEST read, the refused batch's words, the docs fix(store): refuse a MANIFEST that links to nothing, have open_or_create send a damaged store to open, and say a refused batch wasn't recorded
|
Meg, Lottie and the house — received, and #66 is merged into — FableDendrite |
|
Follow-up on #66's merge (956bfd3): the napi module is rebuilt from this head on our rig and the node suites run green — — FableDendrite |
… their key order; the test pass slimmed Brings in #64 (tests that pin no requirement removed or rewritten, the cold-read gates timed by a rebuild, each suite run once in CI) and #67 (serde_json's preserve_order: a JSON object keeps the order its keys were written in; JsStore's docs promise it). Conflicts: - package.json, the test script: this branch added test-batches.mjs, main added test-key-order.mjs. Both run, batches first. - src/state/manager.rs, get_state_item's byte-fidelity doc: main's text (a parsed Value object keeps its parse order; raw bytes are served as given on the head-record path), with this branch's naming of the writers it holds for: append_to_state_json_with_identity and its batched form. The batched append and the item readers needed no code change for key order. preserve_order is a crate-wide serde_json feature, and appendManyToStateJson(WithIdentity) take Vec<serde_json::Value>, serialize with serde_json::to_vec and stamp through the same stamp_identity as the single append; getStateItemJson and getStateItemsJson parse into serde_json::Value. Nothing in the crate sorts object keys or removes from a serde_json Map. Two of this branch's tests pinned a batch item's keys sorted, which main's change makes false; they now expect the written order: - operations.rs test_append_many: [1,2,{"b":1,"a":[3]}] - store.rs test_compute_delta_items_takes_every_item_of_a_batch: [1,2,{"b":3,"a":3},4,5,6] tests/append_many.rs no longer lists unsorted keys among the ways an item differs from serde_json's own spelling. Version stays 0.5.0. Main's CHANGELOG consumed no fragment; every fragment of this branch is kept, and keep-key-order.fixed.md is added beside them. Tests: cargo test 529 passed, 0 failed, 7 ignored (33 binaries); npm test: test.mjs 47 steps, test-views.mjs 14, test-batches.mjs 18, test-key-order.mjs 4, node --test 29 of 29. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test-key-order.mjs covered every JSON-writing method main had; this branch's batched appends were not among them. The append log now also takes a batch through appendManyToStateJson and one through appendManyToStateJsonWithIdentity (an item with its own id and sequence keys, and one without), both before the historical cut, so getStateJsonAt reads them too. Straight after each batch, getStateItemsJson reads its last item alone and then all its items, and the batch's firstOrdinal is checked. In every phase (fresh, reopened, compacted, compacted and reopened), getStateItemsJson reads the whole log, beside the existing getStateJson, getState, getStateItemJson, slice, tail, historical and subscription reads, which now include the batches' items. Checked against a copy whose expected batch items are sorted: it fails on both getStateItemsJson reads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ntil a store records a batch Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Main caught up, pushed, three commits:
— FableDendrite |
…ease
0.5.1 consumed changelog.d/keep-key-order.fixed.md, which this branch had
qualified ("still opens on 0.5.0 until it records a batch"); the fragment
goes with the release, and append-many.breaking.md already says what a
recorded batch does to an earlier release's open. Version taken as 0.5.1;
the batched append still moves the format to version 2 and ships as 0.6.0.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
# Conflicts:
# changelog.d/keep-key-order.fixed.md
|
Main caught up again, pushed: — FableDendrite |
Stacked on #40 (
feat/dendrite-views), as asked on anima-research/agent-framework#291: chronicle's part of the presentation index.appendManyToStateJsonWithIdentity), each item carrying the batch's one record, and batched readsgetStateItemsJson(ordinals)/getRecords(ids)(CH-2, CH-4).get_state_tailno longer repeats the appends a delta snapshot already holds (found by the differential tests); a write whose own auto-snapshot fails no longer throws after its record is durable.Store::createwrites theMANIFESTbefore it takes the lock, so the lock-firstcreateand thechangelog.d/34-create-existing-store.*fragments describe a change from 0.5.0. At 956bfd3 the branch also carries the house's fix(store): refuse a MANIFEST that links to nothing, have open_or_create send a damaged store to open, and say a refused batch wasn't recorded #66 (14 commits): store files that are links to nothing count as there and are refused by name; a directory holding a store's data files and noMANIFESTgoes toopen; the presence check returns errors other thanNotFound;opentakes theMANIFEST's features from a second read under the lock; a batch refused at its mark says it was not recorded; the docs say when theMANIFESTmoves to version 2 and how a cut treats a batch.Review history, design and decisions: anima-research/agent-framework#291 (design note, wave-1 and wave-2 reviews, the merge review). Release: 0.6.0 — 0.5.0 is published from
main(Dendrite, 2026-10-29) without the batched append. The branch keeps main's version (0.5.0) and a maintainer'snpm version minorcuts 0.6.0 once this lands; the context-manager and agent-framework presentation branches declare^0.6.0(review finding CH-1).🤖 Generated with Claude Code
Companion PRs, merge order (CONTRIBUTING: cross-repo companions declared with the order). The presentation set ships as four PRs, each released at the next minor the registry gives it, each floor commit landing once its dependency is on npm: chronicle #62 (→ 0.6.0, the batched append) → context-manager anima-research/context-manager#350 (→ 0.25.0, declares chronicle
^0.6.0) → agent-framework anima-research/agent-framework#844 (→ 0.31.0, declares context-manager^0.25.0and chronicle^0.6.0) → connectome-host anima-research/connectome-host#485 (declares agent-framework^0.31.0). Until each dependency is published its PR cannot install from the registry — the lockfiles resolve the Dendrite releases andnpm cirefuses them — so CI on these PRs is red by construction, as the Dendrite set's was; the suites run on the linked presentation trees. The lockfiles are regenerated at each floor commit.