diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 61a41f24..20a7c8be 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -12758,6 +12758,60 @@ Aarav. msbuild-expert. **Source:** Copilot thread `PRRT_kwDOSF9kNM59geKB` on PR #147. +## P2 — DST-compatible HashDoS-resistant Shard.OfKey research (Otto-281 follow-up) + +- [ ] **Investigate whether `Shard.OfKey` can be redesigned to be + DST-compatible AND retain HashDoS defence simultaneously.** + Current state (post Otto-281 audit): `OfKey` uses + `HashCode.Combine(key, Shard.Salt)` which is process-randomized + for security (anti-hash-flooding) and therefore NOT + cross-process deterministic; tests use `OfFixed` / + `OfFixedBytes` (deterministic XxHash3) for DST-compatibility + and accept the separation. Aaron 2026-04-25 directive: + *"backlog if we can keep all performance and tradeoffs we + wanting and design it in a different way that is DST + compatable and safe. We may already be there, 1 is a good + choice, but we should just backlog that research."* + + **Research questions:** + + - Can a **per-test-suite-pinned salt** combined with a + deterministic mixer (XxHash3 instead of `HashCode.Combine`) + deliver both HashDoS defence in production and DST + determinism in tests, via a single API? + - Does the production HashDoS posture actually require + *runtime-randomized* salt, or is a **per-deployment static + salt** (set at process start from secret config) sufficient + against the attacker model? If the latter, tests can run + with a fixed salt without weakening production. + - Is `HashCode.Combine` strictly required for HashDoS defence, + or is the *salt-mixed-into-deterministic-XxHash* sufficient? + The attacker doesn't know the salt; XxHash on + `(salt-bytes ⊕ key-bytes)` is just as hard to attack as + HashCode.Combine without leaking the salt. + - What's the perf cost of `XxHash3.HashToUInt64` vs + `HashCode.Combine` on the hot path? Earlier perf concern + on `Sketch.fs::Add` was Gen-0 allocations per Add (`new + byte[]`); a stack-allocated `Span` for 4-byte + primitive keys avoids that entirely. + + **If the research lands "yes":** unify `OfKey` and `OfFixed` + into a single `OfKey(key, shards, ?salt)` that is both + cross-process-deterministic-given-salt AND HashDoS-resistant. + Tests pass `salt: 0u`; production reads salt from process + config. + + **If the research lands "no":** keep the current two-API + design (option 1 from the Otto-281 PR triage). Document the + trade-off as final; this BACKLOG row resolves to a decision + ADR. + + **Effort:** S (research-only; design doc + benchmark + comparison; no shipped code unless conclusion is "yes"). + **Owner:** performance-engineer + security-researcher + (cross-cut). **Composes with:** Otto-281 audit, Otto-272 + DST-everywhere, GOVERNANCE §threat-model section on HashDoS. + --- ## Source of this backlog diff --git a/memory/feedback_dst_exempt_is_deferred_bug_not_containment_otto_281_2026_04_25.md b/memory/feedback_dst_exempt_is_deferred_bug_not_containment_otto_281_2026_04_25.md new file mode 100644 index 00000000..0f3f0eda --- /dev/null +++ b/memory/feedback_dst_exempt_is_deferred_bug_not_containment_otto_281_2026_04_25.md @@ -0,0 +1,148 @@ +--- +name: DST-EXEMPT IS DEFERRED-BUG — never a safe state, no comment that says "DST-exempt" should ship without a deadline; the SharderInfoTheoreticTests case: a previous tick marked the test "DST-exempt: uses HashCode.Combine which is process-randomized" + filed a BACKLOG row, treated as containment; in reality the exemption masked the determinism violation which compounded into 3 unrelated flakes this session (#454 / #458 / #473) before Aaron 2026-04-25 surfaced the rule "see how that one DST exception caused the flake, this is why DST is so important, when we violate, we introduce random failures"; ship the FIX (Otto-281: detHash via XxHash3), not the EXEMPTION; counterweight to Otto-272 DST-everywhere +description: Otto-281 counterweight memory. DST exemptions are not containment; they are deferred bugs. The SharderInfoTheoreticTests "DST-exempt" comment masked a flake that fired 3 times on unrelated PRs before getting fixed. Pattern: when tempted to write "DST-exempt", instead either fix the determinism or delete the test. Never ship a long-lived DST-exempt tag. +type: feedback +--- +## The rule + +**"DST-exempt" is not a safe state. It is a deferred bug.** + +When a test or code path is marked "DST-exempt" with a comment +that says "uses X which is process-randomized" or "depends on +environment Y", that is NOT containment. It is *masking* the +determinism violation behind a label that sounds like an escape +hatch. + +Aaron's verbatim framing 2026-04-25: + +> *"see how that one DST exception caused the fake [flake], +> this is why DST is so important, when we violate, we +> introduce random failures."* + +## The case that triggered the rule + +`tests/Tests.FSharp/Formal/Sharder.InfoTheoretic.Tests.fs` +contained three uses of `HashCode.Combine k` for hashing +integer keys: + +```fsharp +let h = uint64 (HashCode.Combine k) +let s = JumpConsistentHash.Pick(h, shards) +``` + +`System.HashCode.Combine` is **process-randomized by .NET +design** to deter hash-flooding attacks on dictionaries. So +the same int produces different hashes in different processes. +Jump-consistent-hash output depends on the input hash, so +shard assignments varied across CI runs. The +`< 1.2 max/avg ratio` assertion sometimes held, sometimes +didn't. + +A previous tick (Aaron 2026-04-24 directive territory) added +this comment above one of the tests: + +```fsharp +// DST-exempt: uses `HashCode.Combine` which is process-randomized +// per-run. Flake analysis + fix pipeline tracked in docs/BACKLOG.md +// "SharderInfoTheoreticTests 'Uniform traffic' flake" row. DST +// marker convention + lint rule: docs/BACKLOG.md "DST-marker +// convention + lint rule" row (Aaron 2026-04-24 directive). +``` + +**That comment treated the issue as contained.** It identified +the cause (`HashCode.Combine` process-randomization), filed a +BACKLOG row, and added a "DST-marker convention" idea — all of +which are good housekeeping. But it left the determinism +violation in the test code. + +## What the masking cost + +The "Uniform traffic: consistent-hash is already near-optimal" +test flaked **three times in this session** on unrelated PRs: + +- **#454** (FSharp.Core 10.1.202 → 10.1.203) — pure dep bump, + failed on a probabilistic sharder test. +- **#458** (System.Numerics.Tensors 10.0.6 → 10.0.7) — same. +- **#473** (Dependabot grouping config) — yaml-only change, + same flake. + +Three unrelated PRs each had to be diagnosed, the flake ruled +"unrelated to this change", and the job rerun. Three rerun +cycles of compute. Three opportunities for an autonomous-loop +agent to misidentify a real bug as "the same flake — rerun" +and ship a regression. Three eyeball-time costs for the +maintainer. + +**That's the compounding cost of a DST exemption left to live.** +The exemption didn't contain the cost — it spread the cost +across N PRs and N reruns instead of concentrating it on one +fix. + +## The correct response patterns + +**When you encounter a flake that uses non-deterministic +primitives:** + +1. **Fix the determinism.** Replace the non-deterministic + primitive with a deterministic one of the same kind. + For `HashCode.Combine` → `XxHash3.HashToUInt64` (Otto-281). + For `Random` (no seed) → `Random seed`. For `DateTime.UtcNow` + inside a property check → fixed `DateTimeOffset` constant. + +2. **If the determinism cannot be fixed, delete the test.** A + test that is *probabilistic in CI* is not a test — it's a + coin flip that gets logged. Either commit to fixing the + determinism or delete the test entirely. Don't ship a + dual-state "sometimes-pass-sometimes-fail" thing under a + "DST-exempt" label. + +3. **If the determinism cannot be fixed AND the test cannot be + deleted (e.g., it tests an inherently-stochastic property + like a Monte Carlo bound), wrap the entire test body in a + loop with a fixed seed and assert the *aggregate* property + over N runs.** That converts the stochastic property into a + deterministic-meta-property over fixed seeds. + +**Never** leave the test in CI with a "DST-exempt" comment. The +comment doesn't make the determinism violation safe — it just +defers the cost. + +## The fix shape (Otto-281) + +```fsharp +open System.IO.Hashing + +let private detHash (k: int) : uint64 = + XxHash3.HashToUInt64 (ReadOnlySpan (BitConverter.GetBytes k)) + +// All three call sites changed: +// let h = uint64 (HashCode.Combine k) +// becomes: +// let h = detHash k +``` + +Three iterations in separate processes — all pass with +identical output. Determinism restored. PR #478. + +## Composes with + +- **Otto-272** *DST-ify the stabilization process* — counterweight + discipline must be deterministic. Same energy: don't carve + out exceptions; fix the root. +- **Otto-248** *never ignore flakes* — flakes ARE the determinism + violation, not "transient infra noise". Same shape applied + to test-side code. +- **Otto-264** *rule of balance* — every found mistake triggers + a counterweight. This memory IS the counterweight to the + "DST-exempt" mistake. +- **GOVERNANCE.md §section on DST** — DST-everywhere as the + default mode, not the special mode. + +## Pre-commit-lint candidate + +A grep for `DST-exempt` / `DST exempt` / `dst-exempt` in +comments inside `tests/**` should fire as a warning at +pre-commit time. Each occurrence is a deferred bug that +needs a deadline. The lint comment can include the DST +discipline reminder: "DST-exempt is not containment — fix or +delete; don't ship dual-state tests." diff --git a/src/Core/ConsistentHash.fs b/src/Core/ConsistentHash.fs index b90ed2f1..f40216ce 100644 --- a/src/Core/ConsistentHash.fs +++ b/src/Core/ConsistentHash.fs @@ -73,10 +73,8 @@ type RendezvousHash(bucketSeeds: uint64 array) = [] static member private Mix(a: uint64, b: uint64) : uint64 = // SplitMix64 of (a xor b) — good enough for HRW scoring. - let mutable z = (a ^^^ b) * 0x9E3779B97F4A7C15UL - z <- (z ^^^ (z >>> 30)) * 0xBF58476D1CE4E5B9UL - z <- (z ^^^ (z >>> 27)) * 0x94D049BB133111EBUL - z ^^^ (z >>> 31) + // See `src/Core/SplitMix64.fs` for the constant rationale. + SplitMix64.mix (a ^^^ b) /// Pick a bucket by maximum-score-wins. O(bucketCount). member _.Pick(key: uint64) : int = diff --git a/src/Core/Core.fsproj b/src/Core/Core.fsproj index aef02625..bbc5c96c 100644 --- a/src/Core/Core.fsproj +++ b/src/Core/Core.fsproj @@ -27,6 +27,7 @@ + diff --git a/src/Core/FastCdc.fs b/src/Core/FastCdc.fs index fc4ecccb..8419eb6a 100644 --- a/src/Core/FastCdc.fs +++ b/src/Core/FastCdc.fs @@ -53,11 +53,9 @@ open System.Runtime.CompilerServices module private Gear = // Deterministic table — generated once via SplitMix64 seeded // with the paper's seed so cross-process chunking is repeatable. + // See `src/Core/SplitMix64.fs` for the constant rationale. let private seedAt (i: int) : uint64 = - let mutable z = uint64 i * 0x9E3779B97F4A7C15UL - z <- (z ^^^ (z >>> 30)) * 0xBF58476D1CE4E5B9UL - z <- (z ^^^ (z >>> 27)) * 0x94D049BB133111EBUL - z ^^^ (z >>> 31) + SplitMix64.mix (uint64 i) let Table : uint64 array = Array.init 256 seedAt diff --git a/src/Core/NovelMathExt.fs b/src/Core/NovelMathExt.fs index f4771fe4..1627a0b0 100644 --- a/src/Core/NovelMathExt.fs +++ b/src/Core/NovelMathExt.fs @@ -119,6 +119,21 @@ type InfoTheoreticSharder(shardCount: int, epsilon: double, delta: double, seed: /// index computed from the key's hash — so on a cold start /// (all shards at zero load) load is distributed by the hash /// rather than always falling on shard 0. + /// + /// **Process-randomization caveat (Otto-281 audit):** the + /// `hashTieBreak` uses `HashCode.Combine` which re-seeds + /// per-process. On a *cold start*, two processes will pick + /// different tie-break shards for the same key — the + /// load-distribution is process-dependent at the cold-start + /// boundary. Once observed loads diverge (after a few + /// `Observe` + `Pick` cycles), the load-based picker + /// dominates and the assignment becomes deterministic given + /// the CMS seed. Tests asserting cross-process determinism + /// of cold-start `Pick`s would flake; tests asserting + /// post-warmup load-based picks are robust. The trade-off + /// is intentional: cold-start tie-breaking by hash is a + /// load-distribution flexibility feature, not a correctness + /// invariant. member _.Pick(key: 'K) : int = let predicted = cms.Estimate key let hash32 = uint32 (HashCode.Combine key) diff --git a/src/Core/Shard.fs b/src/Core/Shard.fs index b71304dd..319e48ec 100644 --- a/src/Core/Shard.fs +++ b/src/Core/Shard.fs @@ -3,6 +3,7 @@ namespace Zeta.Core open System open System.Collections.Generic +open System.IO.Hashing open System.Runtime.CompilerServices open System.Runtime.InteropServices open System.Threading @@ -51,6 +52,16 @@ type Shard = /// Shard a key using `HashCode.Combine` mixed with the per-process /// salt. Good default for any ingest path that touches user-controlled /// keys — rules out the HashDoS attack. + /// + /// **Intentionally non-deterministic across processes.** `HashCode.Combine` + /// is process-randomized by .NET design; combining with the per-process + /// `Shard.Salt` doubles down on that. Two processes will assign the same + /// key to different shards. That is the *point* — it's HashDoS defence: + /// an attacker cannot pre-compute a worst-case input set for our shard + /// distribution because they don't know our seed. + /// + /// If you need cross-process / cross-restart determinism, use + /// `OfFixed`, which uses `XxHash3` instead. [] static member OfKey(key: 'K, shards: int) : int = let h = uint32 (HashCode.Combine(key, Shard.Salt)) @@ -59,9 +70,51 @@ type Shard = /// Shard without the per-process salt — stable across restarts. /// Prefer `OfKey` unless you *specifically* need cross-process /// determinism (e.g. for Kafka key-to-partition consistency). + /// + /// **Determinism contract** (Otto-281 honesty audit): + /// + /// - For **value-type keys** (`int`, `int64`, `uint32`, `byte`, etc.): + /// fully deterministic across processes. `int.GetHashCode()` returns + /// `this` and the rest is deterministic mixing. + /// - For **string keys**: NOT deterministic across processes. + /// `string.GetHashCode()` is per-process-randomized in .NET Core+ + /// (anti-hash-flooding), and we cannot recover from that within a + /// generic `'K` API. String-key callers who need cross-process + /// consistency MUST hash their UTF-8 bytes themselves and call + /// `Shard.Of(uint32 hash, shards)` directly — for example with + /// `XxHash3.HashToUInt64(Encoding.UTF8.GetBytes(s))`. + /// - For **other reference types**: deterministic only if the type's + /// `GetHashCode()` is deterministic (most user-defined records are; + /// classes that depend on instance identity are not). + /// + /// Otto-281 fix replaced an earlier implementation that used + /// `HashCode.Combine key`, which is *also* process-randomized for + /// value types (because `HashCode.Combine`'s mixer is randomized + /// regardless of input). The new implementation is strictly better + /// for value types and no-worse for strings. + /// + /// String-key cross-process consistency is tracked as a separate + /// follow-up: typed overloads `OfFixedString(s: string, shards)` and + /// `OfFixedBytes(bytes: ReadOnlySpan<byte>, shards)`. [] static member OfFixed(key: 'K, shards: int) : int = - Shard.Of(uint32 (HashCode.Combine key), shards) + let intHash = key.GetHashCode() + let bytes = BitConverter.GetBytes intHash + let h64 = XxHash3.HashToUInt64 (ReadOnlySpan bytes) + Shard.Of(uint32 h64, shards) + + /// Shard a UTF-8 byte sequence without per-process salt — fully + /// deterministic across processes and machines. Use this for any + /// cross-process / cross-restart shard assignment where the key + /// has a canonical byte representation. + /// + /// String callers: `Shard.OfFixedBytes(Encoding.UTF8.GetBytes s, shards)` + /// is the cross-process-consistent shard for `s`. The plain + /// `Shard.OfFixed("...", shards)` is NOT (see `OfFixed` doc). + [] + static member OfFixedBytes(bytes: ReadOnlySpan, shards: int) : int = + let h64 = XxHash3.HashToUInt64 bytes + Shard.Of(uint32 h64, shards) /// Exchange operator — partitions a Z-set across `shards` sub-streams by diff --git a/src/Core/Sketch.fs b/src/Core/Sketch.fs index f89f8458..83ecd64c 100644 --- a/src/Core/Sketch.fs +++ b/src/Core/Sketch.fs @@ -54,14 +54,26 @@ type HyperLogLog(logBuckets: int) = /// ≥ ~65 k the 32-bit floor begins to dominate; use `AddHash` /// directly with a proper `XxHash3`/`XxHash64` on bytes if you need /// billions-scale accuracy. + /// + /// **Process-randomization caveat (Otto-281 audit):** + /// `HashCode.Combine` re-seeds per-process by .NET design (anti- + /// hash-flooding). Two processes will produce different cardinality + /// estimates for the same input stream — the *bound* is correct + /// (within ~2% relative error per HLL's `alpha`), but the exact + /// estimate jitters. For tests requiring deterministic estimates + /// across runs, use `AddBytes` with a canonical byte representation; + /// see `tests/Tests.FSharp/Sketches/HyperLogLog.Tests.fs` for the + /// XxHash3 path. The jittery estimate is a *deliberate* trade-off + /// for hot-path performance: an earlier revision called + /// `XxHash3.HashToUInt64` on a 4-byte heap array per Add; for a 1 M + /// element stream that's 1 M Gen-0 allocations purely for HLL. [] member this.Add(value: 'T) = let h32 = HashCode.Combine value |> uint64 - // SplitMix64 finaliser — public-domain constants, 5 ops, no alloc. - let mutable z = h32 * 0x9E3779B97F4A7C15UL - z <- (z ^^^ (z >>> 30)) * 0xBF58476D1CE4E5B9UL - z <- (z ^^^ (z >>> 27)) * 0x94D049BB133111EBUL - this.AddHash (z ^^^ (z >>> 31)) + // SplitMix64 finaliser — see `src/Core/SplitMix64.fs` for the + // constant rationale (golden-ratio + Vigna's BigCrush-validated + // multipliers). 5 ops, no alloc, hot-path safe. + this.AddHash (SplitMix64.mix h32) /// Add a byte span directly — lets callers with a canonical byte /// representation (serialised key, UTF-8 string) bypass the 32-bit diff --git a/src/Core/SplitMix64.fs b/src/Core/SplitMix64.fs new file mode 100644 index 00000000..46a38182 --- /dev/null +++ b/src/Core/SplitMix64.fs @@ -0,0 +1,67 @@ +namespace Zeta.Core + +open System.Runtime.CompilerServices + + +/// SplitMix64 finaliser — Sebastiano Vigna's mixer from +/// [arxiv 1410.0530 §3](https://arxiv.org/abs/1410.0530), validated +/// against BigCrush. Public domain; reference implementation at +/// . +/// +/// **Why the three constants are these specific values:** +/// +/// - `GoldenRatio = 0x9E3779B97F4A7C15` = `floor(2^64 / phi)` where +/// phi is the golden ratio `(1 + sqrt 5) / 2`. From Knuth TAOCP +/// §6.4 multiplicative hashing. Golden-ratio-derived constants +/// produce low-correlation multiplication output across any +/// reasonable bit pattern. Same constant Murmur3 final-mix and +/// Fibonacci hashing use. +/// - `VignaA = 0xBF58476D1CE4E5B9` and `VignaB = 0x94D049BB133111EB` +/// are the SplitMix64 finaliser-pair multipliers. Empirically +/// validated by Vigna to give strong avalanche after the +/// shift-xor steps; passes BigCrush statistical tests. +/// +/// **Why we extracted this** (Aaron 2026-04-25 directive): +/// the three constants used to be inlined in three call sites +/// (`Sketch.fs`, `ConsistentHash.fs`, `FastCdc.fs`) plus once in +/// a test (`CountMin.Tests.fs`). When a magic number repeats, it +/// belongs in one place with one comment that explains why we +/// picked it. Otherwise the rationale rots in three different +/// sites and a reader has to re-derive the constant's provenance +/// every time. +/// +/// **Hot-path performance.** All operations are inlined (5 ops: +/// `mul`, `xor-shift`, `mul`, `xor-shift`, `mul`, `xor-shift`). +/// No allocation. Safe to call on millions of values per second. +[] +module SplitMix64 = + + /// `floor(2^64 / phi)` where `phi = (1 + sqrt 5) / 2` is the + /// golden ratio. Knuth TAOCP §6.4 multiplicative-hashing + /// constant. Also used by Murmur3 final-mix and Fibonacci + /// hashing. + [] + let GoldenRatio = 0x9E3779B97F4A7C15UL + + /// First Vigna SplitMix64 finaliser multiplier + /// (arxiv 1410.0530 §3). Empirically validated to give strong + /// avalanche after the `xor (z >>> 30)` step. + [] + let VignaA = 0xBF58476D1CE4E5B9UL + + /// Second Vigna SplitMix64 finaliser multiplier + /// (arxiv 1410.0530 §3). Combined with `VignaA` and the + /// final `xor (z >>> 31)` step, the full finaliser passes + /// BigCrush. + [] + let VignaB = 0x94D049BB133111EBUL + + /// Apply the SplitMix64 finaliser to a 64-bit input. 5 ops + /// total, no allocation. Suitable for any inner-loop mixing + /// step where a 64-bit input needs uniform avalanche. + [] + let inline mix (x: uint64) : uint64 = + let mutable z = x * GoldenRatio + z <- (z ^^^ (z >>> 30)) * VignaA + z <- (z ^^^ (z >>> 27)) * VignaB + z ^^^ (z >>> 31) diff --git a/tests/Tests.FSharp/Sketches/CountMin.Tests.fs b/tests/Tests.FSharp/Sketches/CountMin.Tests.fs index fc334b6e..2efb2fb2 100644 --- a/tests/Tests.FSharp/Sketches/CountMin.Tests.fs +++ b/tests/Tests.FSharp/Sketches/CountMin.Tests.fs @@ -2,11 +2,20 @@ module Zeta.Tests.Sketches.CountMinTests #nowarn "0893" open System +open System.IO.Hashing open FsUnit.Xunit open global.Xunit open Zeta.Core +/// Deterministic 64-bit hash for a string key, used in CountMin tests +/// to bypass `string.GetHashCode()`'s per-process randomization. Same +/// pattern as `tests/Tests.FSharp/Formal/Sharder.InfoTheoretic.Tests.fs` +/// `detHash` (Otto-281 fix). +let private detStringHash (s: string) : uint64 = + XxHash3.HashToUInt64 (ReadOnlySpan (System.Text.Encoding.UTF8.GetBytes s)) + + // ═══════════════════════════════════════════════════════════════════ // Count-Min Sketch (moved from Round6Tests) // ═══════════════════════════════════════════════════════════════════ @@ -28,8 +37,11 @@ let ``CountMinSketch handles retractions via median`` () = cms.Add("x", 5L) cms.Add("x", -2L) let est = cms.EstimateMedian( - let h = HashCode.Combine("x") |> uint64 - h * 0x9E3779B97F4A7C15UL) + // Mix the deterministic XxHash3 of "x" with the + // SplitMix64 golden-ratio multiplier; any 64-bit input + // through `SplitMix64.mix` gives uniform avalanche. + // See `src/Core/SplitMix64.fs` for the constant rationale. + SplitMix64.mix (detStringHash "x")) // Just verify it ran — for single-key the result should be in range. est |> should be (greaterThanOrEqualTo 0L)