Conversation
Aaron 2026-04-25 invoked the DST no-flakes rule (Otto-272 / Otto-248) after `Zeta.Tests.Formal.SharderInfoTheoreticTests.Uniform traffic: consistent-hash is already near-optimal` flaked 3 times in this session on unrelated PRs (#454, #458, #473). Investigation identified the root cause: three call sites in this test file used `HashCode.Combine k` for hashing, which by .NET design re-seeds per-process to deter hash flooding. The Jump-consistent-hash output depends on the input hash, so different processes assign the same keys to different shards — sometimes the load distribution lands within `< 1.2 max/avg ratio`, sometimes not. Fix: introduce `detHash` helper using `XxHash3.HashToUInt64` on the key's 4-byte little-endian representation. Same convention `src/Core/Sketch.fs` `AddBytes` already uses. Replace all three `HashCode.Combine` call sites (lines 54, 91, 119) with `detHash`. Verified determinism by running the test 3 times in separate processes — all pass with identical output (no longer process-randomized). Drop the previous `DST-exempt` comment on the `InfoTheoreticSharder does not make skew worse` test — the test is now DST-clean, not exempt. Replaced with a `DST-clean per Otto-281 fix` comment that names the fix and the trigger. This is the "real flake" class per Otto-248 — not a registry 502 or transient infra issue but an actual determinism violation in test code. Otto-272 DST discipline applies: fix the flake, capture the reproduction, prevent the compounding. Other `HashCode.Combine` sites still in the codebase (production code in `src/Core/Sketch.fs`, `src/Core/Shard.fs`, `src/Core/NovelMathExt.fs`; one test in `tests/Tests.FSharp/Sketches/CountMin.Tests.fs`) are intentionally left alone in this PR — they are either not flaking under current observation or have different determinism contracts. A separate audit can sweep them if/when they become a problem.
There was a problem hiding this comment.
Pull request overview
Fixes a CI flake in SharderInfoTheoreticTests by replacing process-randomized hashing with a deterministic hash so Jump consistent hashing produces stable shard assignments across test runs.
Changes:
- Added a
detHashhelper based onXxHash3.HashToUInt64for deterministic per-key hashing. - Replaced three
HashCode.Combine kcall sites withdetHash kin the sharding distribution tests. - Updated test comments to reflect the new determinism contract (dropped “DST-exempt” wording).
Comment on lines
+17
to
+19
| /// Otto-281 fix (DST discipline, Aaron 2026-04-25): the prior | ||
| /// `HashCode.Combine`-based `Pick` made | ||
| /// `SharderInfoTheoreticTests` flake across CI runs because the |
There was a problem hiding this comment.
This doc comment includes direct contributor name attribution ("Aaron 2026-04-25"). Repo standing rule is to avoid contributor names in code/docs and use role references instead (e.g., "human maintainer"), keeping names only in the documented carve-outs (see docs/AGENT-BEST-PRACTICES.md:284-290). Please rewrite these attributions accordingly.
Comment on lines
+24
to
+25
| let private detHash (k: int) : uint64 = | ||
| XxHash3.HashToUInt64 (ReadOnlySpan (BitConverter.GetBytes k)) |
There was a problem hiding this comment.
detHash currently allocates a new byte[] on every call via BitConverter.GetBytes, and BitConverter uses native endianness (so it doesn't actually guarantee the "little-endian" representation claimed in the comment). Consider writing the int into a stack-allocated 4-byte span with an explicit little-endian write (e.g., via BinaryPrimitives) and hashing that span; this avoids per-key allocations and makes the hash stable across architectures as well as processes.
Comment on lines
+83
to
+87
| // DST-clean per Otto-281 fix (Aaron 2026-04-25): all three | ||
| // `HashCode.Combine`-based hashes in this file are now `detHash` | ||
| // (XxHash3 on the key's bytes), which is deterministic across | ||
| // processes. Earlier "DST-exempt" marker dropped — the test is | ||
| // no longer exempt; it's required to be deterministic and is. |
There was a problem hiding this comment.
This newly added comment block still includes direct contributor name attribution ("Aaron 2026-04-25"). Please rewrite to role refs (e.g., "human maintainer") per docs/AGENT-BEST-PRACTICES.md:284-290, since this is a forward-looking code/test surface rather than an exempt history/log surface.
This was referenced Apr 25, 2026
Merged
AceHack
added a commit
that referenced
this pull request
Apr 25, 2026
* fix(tests): HLL fuzz test — deterministic AddBytes (Otto-281) The `fuzz: HLL estimate within theoretical error bound` test in `tests/Tests.FSharp/Properties/Fuzz.Tests.fs` was calling `hll.Add i` on integer keys, which routes through `HashCode.Combine` — process-randomized by .NET design (anti- hash-flooding) and acknowledged in the `HyperLogLog.Add` docstring as a "process-randomization caveat (Otto-281 audit)". Different CI processes produced different bucket-landings for the same int sequence, occasionally pushing the estimate past the 4% tolerance and flaking the test. Most recent flake: PR #480 ubuntu-24.04 run 24932270073 (a memory-only PR that made no code changes — pure pre-existing test instability). Per Otto-281 (DST-exempt is deferred bug — fix the determinism not the comment), this routes int keys through `AddBytes` with a canonical 4-byte `BitConverter.GetBytes` representation. The hash distribution properties HLL needs (uniform avalanche over 64 bits) are preserved by `XxHash3.HashToUInt64`, and the result is deterministic across runs. Same fix shape as PR #478 (SharderInfoTheoreticTests). Rationale comment inline so future readers understand why we're not just calling `.Add` directly (Otto-282 — write the why). Verified: 3/3 fuzz tests pass locally. * fix(tests): HLL fuzz — endianness pin + reuse buffer (PR #482 review) Resolves two copilot review threads on PR #482: 1. **Comment / threshold mismatch.** Earlier comment said "we allow 3%" but the assertion is `err < 0.04` (4%). Comment now says "we allow 4%" + cites the empirical 1.96% max observed across the 500-trial sweep that informed the bound. 2. **Endianness + per-element allocation.** `BitConverter.GetBytes` is endianness-dependent on the host (big-endian hosts produce different bytes for the same int) and allocates a new `byte[]` per call. Replaced with `BinaryPrimitives.WriteInt32LittleEndian` writing into a single `byte[4]` allocated once outside the loop. Same determinism property + zero per-element alloc + cross- platform stable. Net: same Otto-281 fix shape, now also satisfying Otto-282 (write the WHY visibly for the endianness pin and the allocation discipline). Verified: build green, HLL fuzz test passes.
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
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.
Summary
Per Aaron 2026-04-25 invocation of the DST no-flakes rule (Otto-272 / Otto-248): `Zeta.Tests.Formal.SharderInfoTheoreticTests.Uniform traffic: consistent-hash is already near-optimal` flaked 3 times this session on unrelated PRs (#454, #458, #473). Diagnosed the root cause and shipped the fix.
Root cause
Three call sites in `tests/Tests.FSharp/Formal/Sharder.InfoTheoretic.Tests.fs` (lines 54, 91, 119) used `HashCode.Combine k` for hashing. `HashCode.Combine` is process-randomized by .NET design to deter hash-flooding attacks on dictionaries. The Jump-consistent-hash output depends on the input hash, so different CI processes assign the same keys to different shards — sometimes the load distribution lands within `< 1.2 max/avg ratio`, sometimes not.
Fix
Introduce `detHash` helper using `XxHash3.HashToUInt64` on the key's 4-byte little-endian representation. Same convention `src/Core/Sketch.fs` `AddBytes` already uses. Replace all three `HashCode.Combine` call sites with `detHash`.
```fsharp
let private detHash (k: int) : uint64 =
XxHash3.HashToUInt64 (ReadOnlySpan (BitConverter.GetBytes k))
```
Drop the obsolete "DST-exempt" comment on the `InfoTheoreticSharder does not make skew worse` test — the test is now DST-clean, not exempt.
Verification
Built clean (`0 Warning(s)` / `0 Error(s)`). Ran the affected tests 3 times in separate processes — all pass with identical output (`Passed: 3`).
Why fix-now (DST discipline)
Per Otto-272 DST (Deterministic Simulation Testing) and Otto-248 "never ignore flakes":
This is the "real flake" class — an actual determinism violation in test code, not a registry 502 or transient infra issue. Aaron 2026-04-25: "if not we need to stop and fix it now or it will compound, that's why the DST no flakes rule exist."
Out of scope
Other `HashCode.Combine` sites still in the codebase are intentionally left alone in this PR — they have different determinism contracts:
A separate audit can sweep them if/when they become a problem. This PR addresses the bleeding test today.
🤖 Generated with Claude Code