Skip to content

Update dependency go to v1.21.4 - #2

Merged
bootjp merged 1 commit into
mainfrom
renovate/go-1.x
Dec 2, 2023
Merged

Update dependency go to v1.21.4#2
bootjp merged 1 commit into
mainfrom
renovate/go-1.x

Conversation

@renovate

@renovaterenovateBot commented Nov 27, 2023

Copy link
Copy Markdown
Contributor

Mend Renovate logo banner

This PR contains the following updates:

PackageTypeUpdateChange
go (source)golangpatch1.21.0 -> 1.21.4

Release Notes

golang/go (go)

v1.21.4

v1.21.3

v1.21.2

v1.21.1


Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

@renovate
renovateBotforce-pushed the renovate/go-1.x branch 2 times, most recently from 27f369c to 09ebccbCompareDecember 1, 2023 18:29
@bootjp
bootjp merged commit 63cba0c into mainDec 2, 2023
@renovate
renovateBot deleted the renovate/go-1.x branch December 2, 2023 03:18
bootjp added a commit that referenced this pull request Apr 25, 2026
Codex round-7 P2 #1: foldIntoBucket appended every over-budget route
ID to bucket.MemberRoutes with no bound. Per-column payload size
scaled with total folded routes — defeating the MaxTrackedRoutes cap
when total routes greatly exceed it. Add MaxMemberRoutesPerSlot
(default 256, configurable via MemSamplerOptions). Past the cap the
bucket counters still absorb the route's traffic, but the routeID is
not appended. TestMemberRoutesCappedAtConfiguredCap pins both halves.
Codex round-7 P2 #2: drainRetiredSlots and advancePendingPrunes
compacted via keep := s[:0]; the dropped tail of the backing array
retained *routeSlot / *bucket pointers, keeping released slots
GC-reachable through the reused capacity. Add clearTail / clearPruneTail
helpers that zero the dropped tail before returning. Regression test
TestRetiredTailClearedAfterDrop holds a header into the original
backing array and verifies index 0 is nilled after the drop.
bootjp added a commit that referenced this pull request Apr 25, 2026
Codex round-11 P2 #1: cancelPendingPrune ran unconditionally at the
top of RegisterRoute. When a removed virtual member rejoined as an
individual slot (capacity freed up), the cancel suppressed the only
cleanup path for the old bucket's MemberRoutes — leaving the routeID
listed on the bucket forever despite no longer contributing traffic.
Replace with cancelPendingPruneFor(bucket, routeID), called only
after we know the route is rejoining the same bucket; prunes against
other buckets (or where the route lands as an individual slot) fire
normally and clean up the old bucket's metadata.
Codex round-11 P2 #2: a freshly created virtual bucket stamped its
RouteID with the first folded real route ID. If that real route was
later removed and re-registered as an individual slot, flushed
columns could show two rows with the same RouteID — one aggregate,
one individual. Add an atomic.Uint64 virtualIDCounter on MemSampler
and hand out synthetic IDs from the high end of uint64
(MaxUint64, MaxUint64-1, …). The synthetic space cannot collide with
real route IDs (assigned from the low end by the coordinator), so
row identity stays unambiguous under churn.
Claude bot nit: rename the local `bytes` variable in Observe to
`byteCount` so it no longer shadows the new `bytes` stdlib import.
Tests: TestVirtualBucketRouteIDIsSynthetic + TestRejoinAsIndividualLetsBucketPruneFire.
bootjp added a commit that referenced this pull request Apr 26, 2026
Round-2 review fixes for PR #646 (Codex P2 ×2):
P2 #1: foldIntoBucket dedup only consulted the visible MemberRoutes
slice, so re-registering a past-cap (hidden) routeID inside the
prune grace incremented MemberRoutesTotal a second time —
route_count drifted upward on remove/re-register churn even though
the bucket membership didn't grow.
P2 #2: pruneMemberRoute only decremented MemberRoutesTotal for
routes visible in MemberRoutes. Past-cap members were absent from
that list, so removals never reduced the total — route_count stayed
permanently inflated and route_ids_truncated stayed permanently
true even after the route was fully gone.
Add a `hiddenMembers map[uint64]struct{}` set on routeSlot. Past-cap
folds land there instead of being silently dropped from accounting:
- foldIntoBucket dedups against both MemberRoutes and hiddenMembers
before incrementing.
- pruneMemberRoute checks hiddenMembers when the routeID isn't in
the visible list, deletes from the set (releasing the map when it
empties), and decrements the total.
Refactor: extract addMemberToBucket helper so foldIntoBucket stays
under the cyclop budget.
Tests:
- TestPastCapMemberRejoinDoesNotInflateTotal — Remove + Register a
hidden member → MemberRoutesTotal stays the same, no drift.
- TestPastCapMemberPruneDecrementsTotal — Remove + grace expiry of a
hidden member → MemberRoutesTotal decrements after the prune fires
on the second post-grace flush.
- Updated TestMemberRoutesCappedAtConfiguredCap to assert
MemberRoutesTotal=8 alongside the existing visible-cap=3 check.
bootjp added a commit that referenced this pull request Apr 26, 2026
Two residual nits from claude review #2 on 2636173:
1) Missing regression test for the new handleList ErrBucketsForbidden
→ 403 mapping. Added TestS3Handler_ListBuckets_ForbiddenReturns403
mirroring TestS3Handler_DescribeBucket_ForbiddenReturns403 — both
read endpoints are now symmetric in coverage, locking down the
slice-2 role-gate contract.
2) Stale comment said "Reject it with 405" but the code (and the
pinned TestS3Handler_DescribeBucket_SubpathReturns404 test)
returns 404 — the comment was inherited from an earlier draft
when 405 was the intended status. Updated the comment to match
the code, and replaced the empty error-message string with
"no admin S3 handler is registered for this path" so the
response shape matches the convention used elsewhere in the
file (CodeRabbit minor + claude residual nit).
bootjp added a commit that referenced this pull request Apr 26, 2026
## Summary
- `keyTypeAt` was the leader's #2 CPU consumer in production (74.8% of
the 30s pprof, on top of XREAD busy-poll which #663 addresses). Each
call issues up to ~19 Pebble seeks across every collection family before
returning.
- Add `keyTypeAtExpect(ctx, key, readTS, expected)` — probes only the
prefixes for the expected type (2–3 seeks). On hit, applies the TTL
filter and returns; on miss, falls through to the full `keyTypeAt` slow
path so wrongType detection is preserved.
- Convert 21 stream / hash / zset call sites to the fast path. Set /
list / string / mixed-kind callers are left for a follow-up.
## CPU savings
| Branch | Slow path (before) | Fast path (this PR, hit) | Reduction |
|---|---:|---:|---:|
| Stream (XADD/XREAD/XLEN/XRANGE) | ~19 seeks | 2 seeks | 9.5× |
| Hash (HSET/HGET/HDEL/HLEN/HMGET/HEXISTS/HGETALL/HINCRBY) | ~19 | 3 |
6.3× |
| ZSet (ZADD/ZRANGE/ZINCRBY/ZREM/ZREMRANGEBYRANK/BZPOPMIN) | ~19 | 3 |
6.3× |
Steady-state: ZADD on an existing zset, XADD on an existing stream, HSET
on an existing hash all hit the fast path. First-write and wrongType
cases pay the same as before.
## Self-review (CLAUDE.md 5 lenses)
1. **Data loss** — None. Read-only probe path; the slow-path fallback is
the unchanged `keyTypeAt`, so any branch that previously detected the
right type still does.
2. **Concurrency** — No new shared state. Each call is read-only against
the existing MVCC snapshot at `readTS`.
3. **Performance** — 6–9× seek reduction on the hit case (the steady
state for live keys), no extra seeks on the miss case (caller pays the
same ~19 seeks they would have without this PR).
4. **Data consistency** — TTL filter is applied identically to the slow
path; the fast path returns "found at readTS" only after the same
`applyTTLFilter` call. WrongType detection is preserved by slow-path
delegation when expected probes come back empty.
5. **Test coverage** — new `TestRedis_StreamCommandsRejectWrongType`
locks down the stream wrongType fall-through. Existing collection-family
wrongType tests (`TestRedis_HGET_WRONGTYPE`, etc.) lock down hash/set.
Existing collection round-trip tests cover the hit path; all now run
through `keyTypeAtExpect`.
## Test plan
- [x] `go test -race -run
"TestRedis_Stream|TestRedis_Hash|TestRedis_Z|TestRedis_HGET|TestRedis_HEXISTS|TestRedis_HMGET|TestRedis_HDEL|TestRedis_HLEN|TestRedis_HGETALL|TestRedis_BullMQ|TestRedis_BZPOPMIN|TestNextXAddID|TestXAddEnforce"
./adapter/...` — passes (146s)
- [x] `golangci-lint run ./adapter/...` — clean
- [ ] CI: full adapter test suite under -race
- [ ] CI: jepsen redis suite
## Follow-up
- Set / list / string / mixed-kind (validateExactSetKind, smembers,
pfcount, getdel, incr, ltrim, lindex, etc.) — same pattern, lower
production traffic, deferred.
@claude review
@claudeclaudeBot mentioned this pull request Apr 26, 2026
8 tasks
bootjp added a commit that referenced this pull request Apr 26, 2026
Three findings from claude + Gemini review on 70213e0:
1) **Issue 1 — `logUnexpectedSourceError` slog key was "table"**
When called for bucket operations the field key was `"table"` but
the value was a bucket name. Log queries on `table=` would find
spurious bucket-error entries; queries on `bucket=` would miss
the audit lines entirely. Renamed the parameter and the slog
key to `resource` so the same forensic query works for both
resource families.
2) **Gemini security-high + Claude Issue 2 — validation divergence**
`handleCreateBucket` only checked `strings.TrimSpace(name) == ""`
while the HTTP path's `validateCreateBucketRequest` also rejects
whitespace-padded names like `" bucket "`. The forward path
would have accepted them, then hit the adapter's
`validateS3BucketName` with a less actionable error message —
different SPA behaviour depending on whether the request was
leader-direct or follower-forwarded. Fix: call
`validateCreateBucketRequest(body)` in `handleCreateBucket`
exactly like `decodeCreateTableRequest` is shared between the
table-side handlers.
3) **Issue 3 — only CREATE_BUCKET tested for nil-BucketsSource → 501**
`DELETE_BUCKET` and `PUT_BUCKET_ACL` had identical
`if s.buckets == nil` guards but no coverage. Replaced
`TestForwardServer_CreateBucket_NoBucketsSourceReturns501` with
a table-driven `TestForwardServer_BucketOps_NoBucketsSourceReturns501`
sweeping all three operations. A future op added without the
nil guard fails CI immediately.
Plus a new `TestForwardServer_CreateBucket_RejectsWhitespacePaddedName`
that pins the validation-parity fix from #2.
Rebased onto the latest `feat/admin-s3-writes` (which now carries
the slice 2a review fixes) so the stack stays clean.
bootjp added a commit that referenced this pull request Apr 26, 2026
Three findings from claude + Gemini review on 70213e0:
1) **Issue 1 — `logUnexpectedSourceError` slog key was "table"**
When called for bucket operations the field key was `"table"` but
the value was a bucket name. Log queries on `table=` would find
spurious bucket-error entries; queries on `bucket=` would miss
the audit lines entirely. Renamed the parameter and the slog
key to `resource` so the same forensic query works for both
resource families.
2) **Gemini security-high + Claude Issue 2 — validation divergence**
`handleCreateBucket` only checked `strings.TrimSpace(name) == ""`
while the HTTP path's `validateCreateBucketRequest` also rejects
whitespace-padded names like `" bucket "`. The forward path
would have accepted them, then hit the adapter's
`validateS3BucketName` with a less actionable error message —
different SPA behaviour depending on whether the request was
leader-direct or follower-forwarded. Fix: call
`validateCreateBucketRequest(body)` in `handleCreateBucket`
exactly like `decodeCreateTableRequest` is shared between the
table-side handlers.
3) **Issue 3 — only CREATE_BUCKET tested for nil-BucketsSource → 501**
`DELETE_BUCKET` and `PUT_BUCKET_ACL` had identical
`if s.buckets == nil` guards but no coverage. Replaced
`TestForwardServer_CreateBucket_NoBucketsSourceReturns501` with
a table-driven `TestForwardServer_BucketOps_NoBucketsSourceReturns501`
sweeping all three operations. A future op added without the
nil guard fails CI immediately.
Plus a new `TestForwardServer_CreateBucket_RejectsWhitespacePaddedName`
that pins the validation-parity fix from #2.
Rebased onto the latest `feat/admin-s3-writes` (which now carries
the slice 2a review fixes) so the stack stays clean.
bootjp added a commit that referenced this pull request Apr 26, 2026
Three findings from claude + Gemini review on 70213e0:
1) **Issue 1 — `logUnexpectedSourceError` slog key was "table"**
When called for bucket operations the field key was `"table"` but
the value was a bucket name. Log queries on `table=` would find
spurious bucket-error entries; queries on `bucket=` would miss
the audit lines entirely. Renamed the parameter and the slog
key to `resource` so the same forensic query works for both
resource families.
2) **Gemini security-high + Claude Issue 2 — validation divergence**
`handleCreateBucket` only checked `strings.TrimSpace(name) == ""`
while the HTTP path's `validateCreateBucketRequest` also rejects
whitespace-padded names like `" bucket "`. The forward path
would have accepted them, then hit the adapter's
`validateS3BucketName` with a less actionable error message —
different SPA behaviour depending on whether the request was
leader-direct or follower-forwarded. Fix: call
`validateCreateBucketRequest(body)` in `handleCreateBucket`
exactly like `decodeCreateTableRequest` is shared between the
table-side handlers.
3) **Issue 3 — only CREATE_BUCKET tested for nil-BucketsSource → 501**
`DELETE_BUCKET` and `PUT_BUCKET_ACL` had identical
`if s.buckets == nil` guards but no coverage. Replaced
`TestForwardServer_CreateBucket_NoBucketsSourceReturns501` with
a table-driven `TestForwardServer_BucketOps_NoBucketsSourceReturns501`
sweeping all three operations. A future op added without the
nil guard fails CI immediately.
Plus a new `TestForwardServer_CreateBucket_RejectsWhitespacePaddedName`
that pins the validation-parity fix from #2.
Rebased onto the latest `feat/admin-s3-writes` (which now carries
the slice 2a review fixes) so the stack stays clean.
bootjp added a commit that referenced this pull request Apr 29, 2026
High (claude[bot] r4 #2): local_epoch wrap to 0 re-issues nonce node_id||0||0 of the same DEK first write -- catastrophic AES-GCM nonce reuse. Add ErrLocalEpochExhausted to §9.1 startup refusal at local_epoch == 0xFFFF; only DEK rotation can recover.
High (claude[bot] r4 #3): node_id was described as 16-bit but DeriveNodeID() returns uint64 FNV-64a. Specify the truncation rule (lower 16 bits) and the startup ErrNodeIDCollision check that asserts no two voting members share the same uint16 truncated value.
Medium (claude[bot] r4 #4): §5.1 step 5 said raft_applied_index is persisted on the next sidecar write, contradicting §5.5 which expects the same write. Reword: the index is part of the same JSON payload as the new wrapped DEKs, eliminating any deferred-write race that would spuriously fire ErrSidecarBehindRaftLog.
Medium (claude[bot] r4 #5): encryption_capable heartbeat field path was unspecified. Pin it to the existing peer_metadata.go extension; cutover commands force a fresh poll within one election timeout rather than reading cached state.
Critical reinforcement (claude[bot] r4 #1): explicitly cross-reference kv/fsm.go raftEncodeSingle/raftEncodeBatch/raftEncodeHLCLease constants in §7.1 Phase 2 and pseudocode the exact FSM dispatch order so the implementor cannot accidentally re-use the colliding tag bytes. Index-gating remains the chosen mechanism (already in round-4 commit).
Medium (claude[bot] r3 #3): §6.1 sidecar.go entry now references the §5.1 crash-durable write protocol explicitly so an implementor reading §6.1 in isolation cannot omit the fsync.
Medium (claude[bot] r3 #4): §6.2 corrected the misleading "old build refuses to serve" wording -- old decodeValue actually returns a silent tombstone, but the 3-phase rollout makes that path unreachable on a real cluster. Bit-layout comment requirement added.
Minor (claude[bot] r3 #5): env KEK provider must os.Unsetenv("ELASTICKV_KEK_BASE64") immediately after read.
bootjp added a commit that referenced this pull request Apr 29, 2026
Codex round-27 P1 #2 on `e7b0c132` (still applicable to
`44d7cd45`): PR-B row claimed RegisterRoute is unconditionally
legacy-slot-only (round-26 fix), but later in the same row
said "MaxTrackedRoutes coarsening divides by slotsPerRoute
(= len(AllLabels)+1 when the flag is on, 1 when off)".
Implementer setting --keyvizLabelsEnabled=true on PR-B
binary would compute divisor=6 with only 1 slot per route,
expanding tracked-route capacity 6× beyond MaxTrackedRoutes.
Fixed by aligning the §7 PR-B row's divisor description with
round-26's unconditional model:
- "MaxTrackedRoutes coarsening check in PR-B uses
slotsPerRoute as the literal 1 — same unconditionality
reason as the RegisterRoute body above; the flag-gated
conditional lands in PR-C+D+E, not PR-B"
- "PR-B with slotsPerRoute=1 keeps the cap counting routes
(1 slot per route × N routes ≤ MaxTrackedRoutes),
behavior identical to today regardless of any flag value"
Also tightened the secondary "flag-default-false" reference
in the L0 chain failure-mode discussion: the legacy-only
behavior comes from the unconditional code structure, not
from the flag default. Updated to "With PR-B unconditionally
pre-creating only the legacy slot (regardless of flag, per
round-26 P2)".
Codex round-27 P1 #1 (PR-B labeled-siblings claim in §9 Q2)
was already addressed in round-27 commit 44d7cd4.
bootjp added a commit that referenced this pull request Apr 29, 2026
…ium/minor)
Critical (claude[bot] r7 #1): writer registry §4.1 only covered process-start and post-rotation registration. Phase 1 cluster-wide activation left every running nodes in a window where they encrypt before having proposed RegisterEncryptionWriter, defeating the registry collision guarantee. Add a third trigger: the bootstrap-encryption Raft entry (§5.6 step 1a) carries a batch writer-registry payload covering every voting member that passed the capability pre-check. FSM apply writes the wrapped DEKs and the registry entries in the same Pebble batch, closing the window without a separate round-trip. §4.1 reorganised to enumerate the three triggers explicitly with their coordinator-side gating.
Medium (claude[bot] r7 #2): §6.3 gained an explicit specification for the three new encryption-internal Raft tag constants (0x03 RegisterEncryptionWriter, 0x04 bootstrap, 0x05 rotation/rewrap/retire/enable-storage-envelope/enable-raft-envelope). FSM Apply dispatch order is now spelled out (envelope unwrap > encryption-internal > HLC-lease > regular). Notes that registration entries are replayed via Raft and therefore NOT in isEncryptionRelevant() (sidecar lag check), only the bootstrap/rotation family is.
Minor (claude[bot] r7 #3): §4.6 pebbleSnapshotMagic was a stale reference to a different snapshot layer; replaced with hlcSnapshotMagic / hlcSnapshotMagicV2 cross-referenced to §4.4.
bootjp added a commit that referenced this pull request Jun 1, 2026
…ments
claude[bot] PR #898 minor #2: CLAUDE.md convention forbids referencing
specific commits/symbols in code comments since they rot as the
codebase evolves. Dropped the "5ba2ef94 / rpushEventually /
lpushEventually" enumeration from both doc blocks; replaced with a
structural pointer to "the per-test retry wrappers in this file"
and a direction to the PR description for full history.
The ground truth for the flake-fix lineage lives in the PR
description, which is durable while file-local commit refs are not.
No behavior change. No caller audit needed (comment-only).
bootjp added a commit that referenced this pull request Jun 1, 2026
…ot] residual nit
codex P2 (adapter/test_util.go:367):
When a real re-election makes another node leader during the
readiness probe, nodes[0].engine.Propose returns errNotLeader on
every attempt for the rest of the budget. The previous loop kept
proposing through that follower engine until t.Fatalf fired —
spuriously failing the readiness gate even though the cluster
had recovered with a different leader.
Fix: on transient not-leader errors, re-run ensureNodeZeroIsLeader
before the next attempt. ensureNodeZeroIsLeader is idempotent in
the steady state so this costs nothing when leadership has not
actually moved. Gated on isTransientNotLeaderErr specifically
(not the broader isReadinessProbeRetryable) so ctx-timeout cases
do not pay for the re-nudge — they are likely just slow scheduler
ticks, not leadership changes.
Signature change: waitForWriteableLeader now takes (peers,
waitInterval) so the probe loop can do leader re-nudges. Sole
caller (waitForRaftReadiness in the same file) already had those
values to hand; verified by grep.
claude[bot] residual nit (round-2 review):
Stripped "gemini PR #898 HIGH point 1/2" attribution refs from
the proposeReadinessOnce / isReadinessProbeRetryable doc
comments. Same CLAUDE.md convention as Minor #2 — the WHY is
kept; the PR-process attribution rots.
Validation:
go test ./adapter/ -run TestRedis_LuaRPopLPushBullMQLikeLists -race -count=2 -> ok 7.9s
go test ./adapter/ -run TestSQSServer_Throttle -race -count=2 -> ok
golangci-lint run -> 0 issues
bootjp added a commit that referenced this pull request Jun 1, 2026
codex P2 #1 + claude Issue #1 — pre-cutover cleartext window
The 'cannot observe' block in §3 was unconditional; in reality it
applies only to entries with index > raftEnvelopeCutoverIndex. On
a cluster that enables Stage 6E after existing traffic, the WAL
interleaves cleartext pre-cutover entries with post-cutover
ciphertext until log compaction removes the pre-cutover segments
(snapshot install + segment purge). During that window an
adversary with raw WAL access can still read user data from the
pre-cutover segments. Added an 'Important caveat' note that
documents the 'eventually true post-compaction' framing, matching
the engine's strict-> dispatch semantics.
codex P2 #2 — proposal-ID leakage description corrected
The original wording claimed the 8B proposal ID 'resets on
leadership change' — wrong. Engine.nextID() at engine.go:3134 is
Engine.nextRequestID.Add(1), a process-wide atomic shared by
Propose, linearizable reads, and admin conf changes. It does
NOT reset across leadership transitions and is bumped by non-WAL
paths, so the on-disk WAL sequence has gaps relative to the
in-process counter. Updated §3 item 5 to describe the actual
semantics: process-local request IDs with gaps from non-WAL
traffic, useful for throughput estimation but NOT a reliable
leader-flip signal.
claude Issue #2 — nonce field widths in §2 diagram
Added explicit byte widths (node_id 2B + local_epoch 2B +
write_count 8B = 12B total, all big-endian) with a back-reference
to internal/encryption/nonce_factory.go:13-15. Matches the
precision the round-1 fix applied to the envelope fields.
claude Issue #3 — §6 forward-compat hook scope clarification
The original wording presented wal_store.go as 'the implementation
would land here as a transparent os.File-wrapping layer' — but
wal_store.go's WAL operations route through the upstream
go.etcd.io/etcd/server/v3/storage/wal package, which manages
*os.File internally and exposes no external io.Writer intercept
surface. Updated §6 to clarify that wal_store.go is the operator
control surface but the actual wrapping mechanism still has to
take one of the approaches §5 rules out (fork the upstream
package, or move to an FS-level interposition that's effectively
§4's infrastructure-FS-encryption answer). Adds a back-reference
to §5 so the reader sees the structural challenge consistently.
All four fixes improve threat-model fidelity without changing the
§4.3 'no file wrapping' decision.
bootjp added a commit that referenced this pull request Jun 1, 2026
Six substantive review findings on the M6 implementation PR. Headline
fix: switched the self-test buffer from in-memory bytes.Buffer to a
disk-backed temp file so large snapshots cannot OOM the encoder.
## Gemini high #1: SelfTest=true was buffering FSM in memory
Out Of Memory risk on production-scale snapshots (gemini high #904).
Fixed by writing the FSM through a tee (sha256 + os.File.WriteAt) into
an os.CreateTemp file under opts.SelfTestDecodeOptions.OutRoot, then
Seek(0) and stream through DecodeSnapshot for the self-test. The temp
file is os.Remove'd via defer on every exit path. The contract is
unchanged: corruption reaches the self-test decode but never reaches
the io.Writer (TestEncodeSnapshotSelfTestDetectsCorruption still asserts
out.Len()==0).
Memory cost: O(1) for the SelfTest=true path now (previously was an
FSM-sized bytes.Buffer on top of the sort working set).
## Gemini high #2: runSelfTest signature
Changed from runSelfTest(fsmBytes []byte, ...) to
runSelfTest(fsmFile io.Reader, ...) so the .fsm streams through
DecodeSnapshot instead of being held as []byte. Same OOM fix at the
runSelfTest layer.
## Gemini high #3: walkRegularFiles loaded every file's bytes into memory
For large S3 blobs this OOMs the encoder. Replaced walkRegularFiles
with walkRegularFilePaths (paths only) and streamFilesEqual (chunk
compare at streamCmpBufSize = 64 KiB). Per-file compare cost is now
O(streamCmpBufSize), not O(file size).
## Gemini medium: tempSuffixHexLen entropy was 4 bytes
Birthday-paradox collision risk in highly concurrent CI environments.
Bumped from 8 hex / 4 bytes (2^32 space) to 16 hex / 8 bytes (2^64
space). Same width crypto/rand uses for cryptographic nonces.
## Claude high + codex P2: fmt.Sscanf silently accepts partial parses
--last-commit-ts 0xffZZ parsed as 0xff (fmt.Sscanf stops at first
non-hex). --last-commit-ts 100oops parsed as 100. This means a
typo could silently set the snapshot HLC ceiling to something the
operator did not type. Replaced fmt.Sscanf with strconv.ParseUint
(base 10 / 16) which rejects trailing garbage as a parse error.
New TestParseLastCommitTS cases cover: 0xffZZ, 100oops, " 100 ext",
"-1".
## Codex P2: empty adapter selection produced an empty AdapterSet
--adapter ' ,' (e.g. a templated argument that expanded to spaces)
would yield a zero AdapterSet and the encoder would publish a valid
header-only .fsm with no adapter records — a silently-empty restore
artifact. Now rejected with a clear error. Pinned by
TestParseAdapterSetRejectsEmptySelection covering " ,", ",,,",
" ", and "," — plus a positive case (single "s3") asserting the
guard does not break legitimate single-adapter selections.
## Caller audit per CLAUDE.md semantic-change rule
- runSelfTest signature changed from ([]byte, opts) to (io.Reader, opts).
Sole caller is encodeBuffered (same package, just updated). No
external callers since the function is unexported.
- corruptBufferForTest hook type changed from func([]byte) to
func(*os.File). Sole caller is the same-package test
TestEncodeSnapshotSelfTestDetectsCorruption (updated to ReadAt/
WriteAt the temp file). External callers cannot set this field
(lowercase).
- streamFilesEqual replaces the bytes.Equal compare in diffOneSubdir.
Same return contract (bool, error) so the caller (diffOneSubdir)
is unchanged in behavior.
- parseAdapterSet now returns an error for empty-selection inputs.
Caller is parseFlags which already propagates the error to exit-1.
Tests + lint green.
bootjp added a commit that referenced this pull request Jun 2, 2026
claude[bot] PR #901 minor #2 (CLAUDE.md convention): inline comments
should state the underlying reason, not reference issue/PR numbers
that rot as the codebase evolves. Dropped "PR #887" / "PR #887
round 1" / "PR #887 verdict" / "PR #887 review" attributions from
the three doc blocks. The WHY content is kept (ctx-parent symmetry,
HLC-4 fence rationale, ErrCeilingExpired non-retryable shape).
The PR description is the durable home for the flake-fix lineage
and the related PR refs.
No behavior change. Comment-only.
bootjp added a commit that referenced this pull request Jun 2, 2026
…#905)
Six findings on f5d2ad7 — two codex P1, two claude P2, two
claude P3. All addressed:
* codex P1 #1 (encode table segments before choosing split
keys). My split key used the raw table name; the actual
table-route key uses dynamoRouteTableKey(encodeDynamoSegment(
tableName)) — base64 RawURLEncoding (adapter/dynamodb.go:
8337-8365). Split at the raw name sorts after every
base64-encoded segment and leaves every table on one side
of the split.
Fix: §3.2 now specifies the Clojure helper
encode-dynamo-segment that mirrors the Go encoding, and the
split key is constructed as
`(str "!ddb|route|table|" (encode-dynamo-segment "jepsen_append_t2"))`.
* codex P1 #2 (seed cross-group routes before claiming 2PC
coverage). Today's SplitRange is same-group only — both
children inherit the parent's GroupID. dispatchTxn enters
dispatchMultiShardTxn only when mutations group to ≥2 Raft
GROUPS, not ≥2 routes. An initial SplitRange in the
setup hook would leave tables 1 and 2-4 in the same group
and the workload would still take the single-shard path.
Fix: §3.3 rewritten substantially. Multi-group cluster
startup is now required — M5a extends
scripts/run-jepsen-local.sh to pass --shardRanges so the
cluster launches with ≥2 Raft groups, table-route keys for
t1..t4 statically split across groups (e.g. tables 1-2 →
group 1, tables 3-4 → group 2). The Jepsen db/setup! hook
no longer issues SplitRange — it calls ListRoutes (gated
to first node) and VERIFIES the multi-group routing is in
place, failing fast if not. Also added §3.3's "Why the
nemesis is still useful" paragraph explaining that
nemesis-driven SplitRange (same-group) churns catalog
versions without moving ownership, exercising M3 + M4
drift detection — a non-regression check. When a future
cross-group MoveRange lands, swapping it into the nemesis
upgrades the workload from non-regression to real
Composed-1 anomaly hunting.
* claude P2 #1 (success criterion in §3.4 will always pass).
My Clojure snippet used
`(filter #(= (:type %) :G1c) (:anomalies results))` but
:anomalies is a map (keyword → seq), not a flat list of
maps with :type. Iterating a map yields [key val] vectors
with no :type field — the filter always returns empty, the
check always passes silently.
Fix: §3.4 corrected to
`(nil? (get (:anomalies results) :G1c))`. Added doc note
about the prior broken form so future readers understand
the trap.
* claude P2 #2 (lexicographic direction inverted in §3.2).
My old explanation said `/split/<int>` was smaller than
workload keys, but `!` is ASCII 33 and `/` is 47, so
`/split/<int>` is LARGER than `!ddb|route|table|*`.
Fix: §3.2 corrected — workload keys would land on the LEFT
shard, not the rightmost. The fix (anchoring to the
table-route prefix) is correct in either direction.
* claude P3 #1 (NG3 contradicts the new multi-table variant).
Fix: NG3 tightened to "no new operation types or generators
BEYOND the multi-table workload variant and the route-
shuffle nemesis specified in §3."
* claude P3 #2 (setup hook route ID resolution not described).
Resolved differently from claude's suggestion: §3.3's
revision means the setup hook no longer needs route ID
resolution at all (no mutation), just a ListRoutes
verification.
* Added OQ-7 (codex P1 #1 follow-up) for the
--shardRanges boundary key encoding helper.
bootjp added a commit that referenced this pull request Jun 2, 2026
The 'M5a is a small, focused PR' line at the bottom of §4
was accurate for the original M5a scope (Go CLI + setup hook
+ docs) but became understated after the codex P1 #2 revision
expanded M5a to include the multi-table workload variant and
the five-change multi-group launch script.
Updated to reflect actual scope: M5a now ships:
* cmd/elastickv-split (Go CLI)
* dynamodb-append-multi-table-workload (Clojure)
* scripts/run-jepsen-local.sh multi-group launch extension
(5 coordinated changes per §3.3 table)
* setup-hook verification (read-only ListRoutes + assert)
* docs
Likely lands as a single PR for atomicity (workload variant
only makes sense alongside multi-group launch + verification)
but reviewable as four roughly independent diff sections.
bootjp added a commit that referenced this pull request Jun 2, 2026
…laude[bot] P2 on 3ca2a7f)
Two new findings on 3ca2a7f:
* codex P2 + claude[bot] P2 (same defect, independently flagged):
--raftBootstrapMembers is rejected on multi-group startup
(main.go:735-741 — ErrBootstrapMembersRequireSingleGroup
guard). My §3.3 item 4 said 'Per-group --raftBootstrapMembers',
which would abort startup before Jepsen could run.
Fix: §3.3 item 4 now correctly specifies --raftBootstrap
(boolean) for the tentative 2-single-node-groups topology.
Single-member groups need no peer discovery so --raftBootstrap
is necessary and sufficient. The existing --raftBootstrapMembers
in run-jepsen-local.sh must be REMOVED for multi-group
processes — it's only valid for single-group multi-node
formations.
* codex P2 #2 — split key reuse becomes no-op. My §3.2 example
showed the boundary key !ddb|route|table|<encode(t2)>, but
after the setup-time split that key is the Start of the right
child route. adapter/distribution_server.go:357-372's
validateSplitKey rejects splitKey == parent.Start/End with
'split key at route boundary' — so every subsequent nemesis
tick at the same key would fail. In a 5-minute run expecting
>=10 shuffles, the nemesis would no-op silently and the run
would appear clean while catalog churn never landed.
Fix: §3.2 now splits into TWO distinct split-key constructions:
(a) Setup-time boundary key (one-shot, in
scripts/run-jepsen-local.sh) — IS a route boundary by
design (the static partition between groups).
(b) Per-shuffle interior key (every nemesis :start) — MUST
lie strictly between the current route's Start and End.
Strategy: append a base64-alphabet byte plus a fresh
counter to route.start; the result sorts after start
but before any non-base64 byte (e.g. ASCII '|'=124).
Implementation details (counter persistence, collision
recovery) deferred to M5b; the design contract is: each
shuffle's split key is strictly interior to the route's
current range.
bootjp added a commit that referenced this pull request Jun 2, 2026
… summary
The §3 body was already corrected in round-2 (cleartext-window caveat
+ corrected proposal-ID semantics), but the §4 threat-model
justification summary still carried the obsolete language:
1. codex P2 #1 (line 131): "The residual cleartext above does not
reveal user data" was unconditional. Qualified to entries with
index > raftEnvelopeCutoverIndex whose WAL segments have already
been compacted. Added an explicit paragraph noting that
pre-cutover entries (and post-cutover entries above any
uncompacted segment) DO reveal user-data fields, with operators
directed to the FS-encryption layer until the compaction
watermark crosses the cutover.
2. codex P2 #2 (line 135): "Leader-flip cadence (proposal-ID
resets, term increments)" claimed proposal IDs reset on
leader-flip. They do not -- Engine.nextID() at engine.go:3134
is a process-wide atomic shared by Propose / reads / admin.
Removed the "proposal-ID resets" signal and back-referenced §3
for the correct counter semantics.
No code changes; doc-only.
bootjp added a commit that referenced this pull request Jun 2, 2026
…gram and §4 conditioning
claude round-3 review found two remaining issues after my round-3
codex P2 fixes:
1. §2 ASCII diagram label at line 42 still said "monotonic per
leader" -- a carry-over from round-2 that round-3's §4 cleanup
missed. Both §3 item 5 and §4 line 135 already say "per process
load" / "does NOT reset on leader flips", so the diagram was
the only spot left inconsistent. Updated to "monotonic per
process load" to align with the corrected three-way
semantics.
2. My round-3 §4 conditioning introduced a logical error:
- Line 131 implied post-cutover entries needed compaction
before user data was protected (false -- AEAD protects them
immediately).
- Line 137 implied post-cutover entries in uncompacted segments
reveal user data (false -- those bytes are ciphertext).
The actual semantics: AEAD protects entries with index >
raftEnvelopeCutoverIndex immediately, independent of
compaction. Compaction only matters for pre-cutover entries
(index <= cutoverIndex) which are cleartext until compacted
away. Rephrased §4 to separate the two cases cleanly:
post-cutover entries always ciphertext; pre-cutover entries
always cleartext until compacted.
No code changes; doc-only.
bootjp added a commit that referenced this pull request Jun 3, 2026
…x P1 + claude review)
Three findings from the v2 review:
1. Codex P1 (L27) + Gemini #1: uint32+omitempty made partition-0
indistinguishable from absent. Replace with *uint32: nil = pre-M5-3
dump or never-captured, non-nil = explicitly captured (incl. 0).
New ErrSQSEncodeMissingPartition fail-closed sentinel when
partition_count>1 AND rec.Partition==nil. Decision matrix table
added to make all 6 (partition_count × Partition shape) cases
explicit.
2. Gemini #2 (sort): include sequence_number in the per-partition
sort key. Now (partition, send_timestamp_millis, sequence_number,
message_id) — matches the classic path's tiebreakers.
3. Claude review (call-site plumbing): explicit notes that
HandleMessageData wires rec.Partition = &partition only when
isPartitioned; decodeSQSMessageValue keeps its current
(value)->(*record, error) signature; parseSQSGenericKey wrapper
updated mechanically to discard the new partition return.
New test pinned: TestSQSEncodeRejectsMissingPartitionOnPartitionedQueue.
bootjp added a commit that referenced this pull request Jun 3, 2026
…HA fallback policy (claude v913 v2 + codex P2)
Five findings addressed:
1. Codex P2 v913 v2 (L48): preserve KEYMAP.jsonl user-object
disambiguation. The existing isKeymapCollisionTracker (returns bool)
stays — it discriminates between collision-tracker file (tracker=true,
no sidecar) and a legitimate user object named KEYMAP.jsonl
(tracker=false, has paired .elastickv-meta.json sidecar). M4-2b only
calls loadBucketKeymap when tracker==true; the user-object case keeps
M4-2a's normal object-body emission. Pinned by
TestS3EncodeAcceptsLegitimateKeymapUserObject.
2. Claude v913 v2 Medium #1: function name accuracy. Doc previously said
checkBucketKeymap (returns error) becomes loadBucketKeymap; the actual
site is isKeymapCollisionTracker (returns (bool, error)) at
encode_s3_objects.go:108, called at line 91. Refactor target section
now reflects this; files-list also corrected.
3. Claude v913 v2 Medium #2: LoadKeymap last-wins vs M4-2b's fail-on-dup
contract. Explicitly documented divergence: loadBucketKeymap MUST NOT
call LoadKeymap; iterate KeymapReader.Next() in a manual loop and fail
closed on any duplicate Encoded. Rationale: S3 decoder writes exactly
one record per renamed object, so a duplicate means corrupt dump.
4. Claude v913 v2 Low: KindSHAFallback scope in S3. Documented as
forward-compat: S3 decoder currently emits only KindS3LeafData /
KindMetaCollision, but a KindSHAFallback record is honored by the
existing full-rel-path lookup. Unknown Kind values fail closed
(TestS3EncodeRejectsUnknownKeymapKind).
5. Claude v913 v2 Nit: test-name parity between PR body and doc. The
PR body's TestS3EncodeRejectsKeymapWithReservedPrefix and the doc's
TestS3EncodeRejectsKeymapTargetingReservedSubtree refer to the same
case; the doc is now annotated as the authoritative name.
bootjp added a commit that referenced this pull request Jun 3, 2026
…cher (claude v914 v3)
Four findings from the v3 claude review, all editorial / mechanical:
1. Minor #1: "classic path four-field sort" wording at L93 was wrong.
The live sortMessagesForEmit (sqs.go:815) is a 3-field tuple
(send_timestamp_millis, sequence_number, message_id). Partitioned
path prepends partition as the leading key. Reworded.
2. Minor #2: decodeSQSMessageValue return type at L75 — live sig is
(value []byte) (sqsMessageRecord, error), a value not a pointer.
Doc previously said *sqsMessageRecord — corrected.
3. Low #3: encodeMessageSideRecords partition param plumbing now
spelled out explicitly in the §"Encoder lift" side-record section.
Branches on partition != nil rather than re-reading meta.PartitionCount.
4. Low #4: addMessage / partitioned dispatch named. v4 threads
partition *uint32 through addMessage (rather than spawning a peer
addPartitionedMessage) so addMessage + encodeMessageSideRecords
share the same dispatch source.
5. Editorial: Files-to-modify comment for sqs.go now lists all four
touched functions (sqsMessageRecord +Partition, parseSQSMessageDataKey
+ parseSQSPartitionedQueueAndTrailer new returns, HandleMessageData
wiring, parseSQSGenericKey wrapper discard).
bootjp added a commit that referenced this pull request Jun 7, 2026
…oc + nits
Round-2 review fixes for PR #933:
Codex P1 #2 — Verify the cutover applied before installing the wrapper.
runRaftEnvelopeCutoverBarrier now re-reads the sidecar after
awaitCutoverApply and refuses to InstallWrap when
RaftEnvelopeCutoverIndex stayed 0. That state means a concurrent
RotateDEK changed Active.Raft between propose and apply, and the §6E-1a
applier consumed the cutover marker as a stale-DEK benign no-op
(constraint #3). Activating wrap with the sidecar still in pre-cutover
state would brick the cluster: the §6.3 strict-gt hook compares
against the (still-zero) cutover index, so post-install proposals at
index gt 0 would be treated as envelopes while pre-install admin
entries (the racing RotateDEK via cleartext ProposeAdmin) sit in the
apply queue at indexes gt 0 and halt every follower on unwrap-failure.
Recovery: releaseSafe stays true on this branch (no cutover took
effect, safe to resume user writes in cleartext pre-cutover mode), the
deferred End() closes the barrier, FailedPrecondition surfaces so the
operator's CLI retries against the updated Active.Raft.
New regression test
TestEncryptionAdmin_EnableRaftEnvelope_RefusesOnStaleDEKApplyRace pins:
InstallWrap NOT called, End called, sidecar.RaftEnvelopeCutoverIndex==0
preserved.
Codex P1 #1 — Wrap-gap admin RPC documentation.
RotateDEK / RegisterEncryptionWriter route through ProposeAdmin
(barrier-exempt by interface contract) and currently land at the raw
engine s.proposer. Between the cutover marker's commit and InstallWrap,
they would land cleartext at indexes gt cutoverIdx and halt on the §6.3
hook. 6E-2d does not close this gap — the barrier scaffold ships inert
(raftEnvelopeWrapEnabled = false) so production never opens the
wrap-gap window. Closing the gap is a 6E-2e blocker:
Option A (preferred): route admin RPCs through the wrap-aware
proposer.
Option B: extend cutoverSem to serialize them against
EnableRaftEnvelope.
Documented in BeginCutoverBarrier and the partial design doc's 6E-2e
table row as a BLOCKER for 6E-2f.
Claude r2 nits:
- Finding A — goroutine leak in TestDynamicWrappedProposer_BarrierDrainsInflight
fixed via sync.Once + t.Cleanup so the blockingFakeProposer goroutine
drains even when t.Fatal fires.
- Finding B — double "kv:" wrap in ShardGroup.WaitInflightDrained
resolved by changing the inner wrap from "kv: wait inflight drained"
to "drain await canceled" (no package prefix), with the
ShardGroup forwarder adding the single canonical "kv: shard group ..."
prefix.
- Finding C — equalStrings replaced by slices.Equal.
Caller audit for the new semantic changes:
- runRaftEnvelopeCutoverBarrier (added sidecar re-read + stale-DEK
early return with releaseSafe=true): only caller is
EnableRaftEnvelope which propagates the error to gRPC, no semantic
impact at caller.
- dynamicWrappedProposer.WaitInflightDrained (error chain prefix
change "kv: wait inflight drained" -> "drain await canceled"):
TestDynamicWrappedProposer_WaitDrainedRespectsCtx asserts via
errors.Is(err, context.DeadlineExceeded) which traverses the chain;
still passes. ShardGroup.WaitInflightDrained re-wraps with the
canonical package prefix.
bootjp added a commit that referenced this pull request Jun 11, 2026
- §5.0: explicit RouteStateActive-only candidacy precondition; exclude
WRITE_FENCED/MIGRATING_* (gemini medium #3).
- §3.1/§7.1: --autoSplit implies sub-range buckets (autoSplitDefaultBuckets,
default 16) + sampler-wiring condition keyvizEnabled||autoSplit, with an
implied-vs-explicit config table; closes the bare-keyviz-defaults no-op trap
(codex P2 #1).
- §5.3/§5.4.2/§6.4: compound single-key isolation as one atomic decision
(1 budget unit, 2 chained CAS SplitRange calls); intermediate child is
cooldown-exempt so the hot key can actually be isolated (codex P2 #2).
- §7.7a: bounded leader-local state map via per-cycle GC reconciliation against
the live catalog (gemini medium #1); also reconciles disappeared/manual-split
routes.
- §7.7b: add durable RouteDescriptor.SplitAtHLC for deterministic post-election
cooldown reconstruction; replaces the unimplementable 'recent per catalog'
seeding (gemini medium #2); resolves OQ-7; scoped to M3-PR1.
- §7.3 metrics normalized to bounded {reason} enum; added
autosplit_compound_partial_total. §8.1/§8.4 updated for new scope + criteria.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@bootjp