chore: write comments for the merged tree, not its history - #60
Merged
Conversation
oreofeolurin
added a commit
that referenced
this pull request
Aug 30, 2026
Same pass as #60: comments that narrated what used to be wrong now state the constraint that still applies, and issue numbers no longer stand in for reasons. Test names lose their issue tags. Nothing behavioural. The dashboard's logical-count comment keeps the part that still matters — that avoiding ual.read is deliberate, because the zero-copy read returns null for hot-ring-wrapping payloads and would undercount them.
oreofeolurin
force-pushed
the
chore/comment-cleanup
branch
from
August 30, 2026 09:35
af4b316 to
c97a057
Compare
oreofeolurin
added a commit
that referenced
this pull request
Aug 30, 2026
Same pass as #60: comments that narrated what used to be wrong now state the constraint that still applies, and issue numbers no longer stand in for reasons. Test names lose their issue tags. Nothing behavioural. The dashboard's logical-count comment keeps the part that still matters — that avoiding ual.read is deliberate, because the zero-copy read returns null for hot-ring-wrapping payloads and would undercount them.
oreofeolurin
added a commit
that referenced
this pull request
Aug 30, 2026
…items 2, 3) (#45) * fix(stream): rebuild name registry and record counts on recovery (#42) Issue #42 items 2 and 3, from a production-shaped deployment report. ## Item 2 — `stream list` empty after restart The replay path registered the *bare* stream key while the live append path registers the namespace-qualified name. `stream list` filters on the namespace prefix, so recovered streams in a non-default namespace vanished from listings while `stream info` — which resolves by hash — still found them. The default namespace happened to work because its qualified form is the bare name. A `stream_append` entry carries only `namespace_hash`, not the namespace string, so replay cannot qualify the name on its own. Wired the same hash→name resolver the queue projection already uses (`Shard.resolveQueueNamespace` over the namespace registry, whose `nameForHash` was written for exactly this). ## Item 3 — `stream info` counts disagree with `stream read` Not a recovery gap: it reproduced before any restart. Every append is stored batch-wrapped, so one UAL entry may carry N user records, but the projection counted *entries*. A 3-record batch reported `Records: 1`. `Size` was hardcoded to 0 and never computed at all. `StreamRecord` now carries `record_count` and `byte_len`, decoded from the durable value on both the live and replay paths, with running totals on `StreamState`. Two further defects fell out of the same confusion: - **Duplicate StreamIDs.** A batch reserved one sequence but expands across N at read time, so the next append in the same millisecond reused IDs the batch had already handed out. `nextBatchAt` existed for this and had no caller. Same cursor-identity failure as item 1. - **`last_id` reported the batch's first record**, not its last. - **Count-based retention kept N batches, not N records.** `resolveNthRecordId` now walks batch contents, cutting at the last entry boundary that drops no more than the requested count — retention is a cap, so keeping a few extra beats deleting records still inside the window. The dashboard had already worked around the miscount by re-reading every UAL entry to sum batch headers, sampling the first 16k and extrapolating past that. That is now an O(1) projection read — and it had used the zero-copy `ual.read`, which returns null on hot-ring-wrapping payloads and silently counted those as one record. ## Tests Projection unit tests cover batch sequence reservation, logical counts and bytes, trim decrements, and record-based retention. E2E tests reproduce the report's exact shape — a namespaced stream fed by a batch append, across a restart — asserting on real output rather than exit status. All four verified as genuine controls: they fail against the unfixed code. A fifth e2e test for the duplicate-ID case was dropped rather than kept, because CLI round-trips are ~60ms apart and it could never land two appends in one millisecond — it passed unfixed. The projection unit test covers it instead. test-unit and test-integration pass. test-e2e: 575/579, the 4 failures being the `kv/cluster` set that fails identically on a stashed baseline. * chore: write these comments for the merged tree, not its history Same pass as #60: comments that narrated what used to be wrong now state the constraint that still applies, and issue numbers no longer stand in for reasons. Test names lose their issue tags. Nothing behavioural. The dashboard's logical-count comment keeps the part that still matters — that avoiding ual.read is deliberate, because the zero-copy read returns null for hot-ring-wrapping payloads and would undercount them.
oreofeolurin
added a commit
that referenced
this pull request
Aug 30, 2026
* fix(stream): key stream metadata by namespace-qualified name (#46) Stream metadata — partition count and all three retention settings — was keyed by the bare stream name while `stream_names` was already keyed by the namespace-qualified one. Two namespaces holding a same-named stream therefore shared a single metadata entry, and creating the second silently rewrote the first's configuration: flo stream create solo --partitions 4 -n ns1 # info: Partitions: 4 flo stream create solo --partitions 1 -n ns2 flo stream info solo -n ns1 # Partitions: 1 Partition count drives routing, so a tenant could re-route another tenant's stream by picking the same name, with no error and no log line. Retention collided identically, letting one namespace shorten another's and delete its data. Metadata is now qualified at every read and write, matching the name registry. Two distinct read paths needed it, not one. Qualifying the key fixed `stream info`, but `stream list` still reported the wrong partition count: it strips the namespace prefix before looking metadata back up, and it is served by the ShardWalker (`serializeWalkStreamNames`) rather than by `handleList`. Both serializers now take the namespace and re-qualify — the same shape `queue_list`, `processing_list` and `action_list` already use. `deleteStream` removes the qualified key and still sweeps the raw one, so entries written by earlier builds do not leak. Three e2e tests cover partition count via `info`, partition count via `list` (the separate path), and retention. All three fail against the unfixed code. test-unit and test-integration pass. test-e2e: 576/580, the 4 failures being the `kv/cluster` set that fails identically on a stashed baseline. * chore(): clean up handler mode * chore: write these comments for the merged tree, not its history Same pass as #60 and #45: comments state the constraint that still applies rather than what used to be broken, and issue numbers no longer stand in for reasons. Test names lose their issue tags. Nothing behavioural.
Comments landed with the cluster and port fixes narrated what used to be wrong rather than what the code now guarantees — "the old condition compared…", "previously parsed and discarded", "the banner used to print Raft port: 0", plus issue numbers standing in for reasons. A reader of the merged tree has no use for any of it. Each one is rewritten to state the constraint that still applies. The banner comment now says a derived Raft port reads as 0 in config; the port-derivation comment says an ephemeral base cannot be offset; the cluster_status doc says a lone node is the leader of a one-member cluster. Three test names lose their issue tags, and one test drops a "for the record" assertion whose only purpose was to demonstrate the old bug — the assertion itself is kept, since a derived Raft port always differing from listen_port is exactly why it cannot be used to infer clustering. No behaviour change. Build, test-unit and test-integration all pass.
oreofeolurin
force-pushed
the
chore/comment-cleanup
branch
from
August 30, 2026 13:25
c97a057 to
728dec7
Compare
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.
Follow-up to the comment cleanup in #55, applied to the comments that already landed on
devvia #47, #51 and #53.