From fa5d735f1e7cb291d1c0ce5f6d8361d77bb124ec Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 11:57:04 -0700 Subject: [PATCH 01/14] Scratch: bound public construction geometry; add PeekScratchArchive ScratchBank::Create (both overloads) and Grow computed the arena size from caller-supplied capacity and dims before bounding either. ArenaBytes multiplies those in signed int64, so a large valid-typed request overflowed the computation rather than failing to allocate. Both now apply the format's own ceilings (kMaxBankRows rows, kMaxCrossDeviceDims dims) before any size arithmetic runs, returning InvalidArgument. The archive loader already applied these caps; direct construction did not, and Grow bounded its request only against the current capacity. PeekScratchArchive reports a serialized archive's geometry, channel table, and the exact byte length a Load consumes, reading only the header from a byte span. Hosts that append a trailer after the archive can now validate that trailer before committing the load instead of discovering a broken one with the rows already replaced. Load and the peek share one header validator so the two cannot disagree about which archives are well-formed. Suite: 85,075 checks, 0 failures. The geometry cells were confirmed to fail against the unbounded code (13 failures) before the caps landed. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 21 ++ ci/__pycache__/_cxx_decls.cpython-313.pyc | Bin 0 -> 8348 bytes docs/API.md | 27 +++ include/superfaiss/scratch.h | 38 ++++ include/superfaiss/version.h | 2 +- src/scratch.cpp | 224 +++++++++++++++----- tests/test_main.cpp | 236 +++++++++++++++++++++- 7 files changed, 497 insertions(+), 51 deletions(-) create mode 100644 ci/__pycache__/_cxx_decls.cpython-313.pyc diff --git a/CHANGELOG.md b/CHANGELOG.md index 593d52d..1f705c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,27 @@ per entry. Reconstructed from git history 2026-07-12. The format follows [Keep a Changelog](https://keepachangelog.com); this project versions by feature tier (minor = new capability, patch = fix), not strict SemVer of a public ABI. +## [3.2.2] — 2026-07-21 + +### Added +- **`PeekScratchArchive`** — reads a serialized scratch archive's header and channel + table out of a byte span and reports its geometry plus `archiveBytes`, exactly the + number of bytes a `Load` consumes, without allocating or reading the payload. It runs + the same header validation `Load` does (the two now share one validator, so they cannot + drift). A host that appends its own trailer after the archive can therefore locate and + validate that trailer *before* committing the load, rather than discovering a broken + trailer with the rows already replaced. + +### Fixed +- **`ScratchBank::Create` and `Grow` performed signed size arithmetic before bounding + their geometry.** `ArenaBytes` multiplies capacity by dims in signed `int64`; both + entry points accepted any positive `int32` pair, so a large valid-typed request + overflowed the arena computation — undefined behavior rather than a refused + allocation. Both now apply the format's own ceilings (`kMaxBankRows` rows, + `kMaxCrossDeviceDims` dims) before any size is computed, returning `InvalidArgument`. + The archive loader already applied these caps; direct construction did not, and + `Grow` bounded its request only against the current capacity. + ## [3.2.1] — 2026-07-21 ### Fixed diff --git a/ci/__pycache__/_cxx_decls.cpython-313.pyc b/ci/__pycache__/_cxx_decls.cpython-313.pyc new file mode 100644 index 0000000000000000000000000000000000000000..eaac170c167d5dfb8427b064975c643db7cdf8c4 GIT binary patch literal 8348 zcmbtZTWnj$nLdX+JiHuI6h+-EUvlgyjwstwQrmGY#c^fJH(9n~9n+R0I@A$)C^4oE znK_hgF&A(*50&h;qUso?n+3w61?s#kyg-4?(FwHBxht5E6wWHUpvUE8;mjY0g8W)A$%T}ff z%xGyKc=5`sCltABy(3Q7iO1p^F=X&lj*?_5rA7D3U}8kW{$Y$6D}zR!v|$8aLD_EQF&A;A}2V z#|@eO>1m2zQfEL0b_N5%7q(}WQeY13AOeQbYoy6+BrVTqGm{!|RUzsn#9+KlXxFs_ zB|_JXG?kaArlsVrc#qao0meqrIME`&-WAskFbc#?uw5p4C(8Rw$dk$F++KR*Ral^0 zfu&*yakxR$R0>Vk1Z9wsQX|?#phrHFh74$itS7<6bXuE9fjLPgzThT46KYyBuyjg; zWMhNmb&VvFYLxcXQ3eF!SmwX7?_((l4M+l~aY{@!z#1H?;#NM>%T#)PJ0^<3Mvp3Y;(|t75-D~nP z$g#x|>Xeaj^!ARw9@4LNn_R$ZT8L&^joC?492~hgFgy%6fvU1WNt4YTgMA@o792l0 za7Ix=k$5N^xp^~8?E*C#W&@5ro(e-JsqPxYi&<^>H(o@Q7Td!)0P#fKRa0M-;*bW14XGTJ6Mvu*|U!^iHp#xS}IK8A7{{WmkV)(hds6^Jh48< zDVC6+Sf9rTYYDU!vhCtRqGCJD6T2c}+|I@wu)9utx!9Kk3ry#_*;mz)x)i_CP_e~L zinnZwl}6&3@&%erVGuNbxgA@8Uuj|rRCrA*EDc!ZjcPGHkHuG7PJZ6F-M+3 zRZoeHDX-~bw6Jy;h1i7()HF8Vb1XUj;kknFE9vJ_v19Lp=xWcgm7ZhE-s8)T<6o!I zj(Lp6U!nmQ&yUt?GWuu%?wjaal`@^*oN|g6Eh^?JmJtLCI`n)qE;Mk8fDuO-zP`qY zEy$x5gO&zvza9E)KcBJel`|rI4P=K24WU$+e3!`w0ya~4BN^B0I7s=*1S4Q2GHp}l znxZiqi^Xpm^!Q|1E{lniF*Ho2x?!Ec^m`A$jMJ#VWM9j@ElXSS2R`h--(U1SKQETH z?_AyfBL0@z9};{vwchJm>dJNGU(8?p@XGxw`Ll&LAGAJjJmeqle%QY3?OS&Av7`8i zqt;X`J89C4o!ODx#iFpYpy_e9`rTb_u#ZIJ)7Xtnj1m!=B_M4Sslz!c*;CCGGWH&cmtB%U_y*W-kS1g4Xn zJoxT`8*ut^&jRCrw_42HZBMNS+E{Zr>GYZ(EQi9g1!c>1{iktP^5Z1A7 zs&>LU zVdn=O=v^FM7|u6+*ml3I;QwITC+^*)rsiA6KX-cspRl1 zx)XEX`0Lh@62wG_9uM;Xls_9v^+H4Ajq2e<0XM~s*R1Ad zo}n7~RO6M+L}0VEnaFaAl?ma-eKzqi*az0Ndi{d6=Brq$yO{`4zq_F*wyNkUVn6m! zeSZ_uUWG(__HFb!Xrx!A7oz7ppAnAqba$3rzi>Snk7l;L-qq7R9_kM54jESiUE>4$ zUQ_pF!dF8#x&xhG(j907S2q6C_WacvGWI|$ZLn_f6mg-8NT^KPjGB&2o3<&EoJ~QD z`IO1e7<7TEfHsEpB$-hY@r)K`u?e|=(Np+0-a!S)bT}973-;U2?7(M|8?nL{4%ec1 zOPs%vx8w(kjvcFx&J{;z(XpEWJt?T+~ii`^Mp8_K4_ zg|JA0our2zg)mXkVFJJeO|j`b%x9%+=E@;#;<~Wa1P!s9Vt;CGO;oC@v8#Z{M73f) zsX~ozCY&|9SY5xYZi+*FuzVR%?F)MbYc)Gp?(q4BGG|1gha1v5btf{>t=y;l2LB}YRdSj`|#4IRG{VeGG zMqHia)M>K<9s+4ys>5mX6LtqBgma$#R+s{3^#s0omve}SC?CIdZC+_uUsHDm?MeeX z6UZ^zUUsICEqGn`NZ)u%758Axg!#Ek9s`krVF5khsqPu4cQ{spo~i`BYB`5sh+?F> zbx+X!)GWwr@;@U~KB)9~01IiX!Mdtvz>GL@Uj0~GWzsQD!%|kpJ{tR1$cpclg+JUI(U7WNXlB7wwlCo>J9K*;E3rV+oL7nxrt)JT5$zbKhDYFdv5LX#V; zYR1uZwD0w|Lq;HD2~Czm>eHA7t7?hIOlvGjOxulVWUvsPAzc%hWfLaoG@9 z20ZJAm<(enOhm2$=GcKqg02C>OjLae7srIAlXDD&S!%>k z7I7jZ7L4>vx_mb}PG>og2GPUu=uMN4lM}Q@pouBdRlaS~;GMk601;+~X(EhaZCb9~ zC?h?BL4zs~0++f{z7aH1$6#nIEY|4Fo@ju)M5nCb*w!!hyYtWH(?zLsRq9@mx{K1@ z?BHjTH@80@y>qk>dD#4mB>Y0Db!&Fyk=^;jxxb%V_U$SRuG#}D_CTp)$GrEE*SC26 z*7fDq=O6eVM2p`2^8!)To~HNr=hAl$=igj9 zQrK0P{H-T2&zC&Dh0Epm3Gq=SUb@BNr~N;^vK~KqoAdmSGo`KDa*j_MTfXvf4c%)^ zoTG8Y-bN7x>dsvI{LxiOUXkRYvFZ|a1%t~N*&B{5wYaB+t61g3n zMc=`nUCQ(KcHX}pm6qm26E7Ih$F!`51_%QJ=e-OE* zmkes~{;j9w)B)i)PVv-X;WvI-XM9Zax|r$(j#A~TT}dD%9Q7+OJpAJ|7zVg*+Q-Ve zTlx;s6JH-a!8xwSaadKc;)X4huQ_njxUlFp{EOrCZoF|X``A<;C^^NexY~`0%@r`sC{R z0;fC2jxqC?rSLVwImdD1HFq~!bM@;hVAl$NYjeVwQ*(Cfb8pZnVl5NnkkQ4tgqc zs_Rm>ffHf5~(fC~(Q?oki19}p750?xU@&1b8p zt2fl~q@hadcjw7ECai_C)Wxz~sgU(kJfj5pt$QUY58O_*%c_~y5zU2&pBiL@;eg&`3BAAyXZRu zeGoCO#p(MD-Y_C~CYVN=zxt{rtJEWw^Z%MW@z#Vqts3&Ars+716n*!K_P8>;5EmTn zOU+Iq-ST#d#INIw@S9BIQ9z5*$fA!$97s{okb;~+g*MgjatCdh{9BpUP@wDR5ekq0 z+WjqXoW4fH)=x#`fy{*R+Y>3DYVPcBYAxOC~`-P!Y>w|7)CBEF{V5U!!w zfkzH$evsZoOD^e0&uwt?tsMn*xwUJxwP&TZr`YRL^Zk#XFSZ<; zKlz!rrPQ&r&{H`1tD&C{{bG2zx2;ich%@BNX_w{+yhQgaJNTHEeurM9h0Hww?*dAHQQqp(0W35BiY{#Js$3>11wT&C9j5-__O{xt7*EzjOktZQqe| zecIT*9;0mO$X~eMvM@xGKl^Xj1uq*17!5$A@aA$`ASY1VFBY4-zm9vUJaeA1bpFDE zIM1(*@SNoS$#ZM3@|@dONdh4oPbG|HWXt(jmYWsGz|+0$_yjDCAlG3t5HcdaEuY7m z8wRQ9)D22BuUs7!>$XWyIQquHHq5x#y9oup8fY}_VfIeV(!jAZpxx9tl~s&JECks@t}c|;@M+rV`!QmAa*YmBY2A?MH61UPQ_ioDw=fDR=8Ooc zH7n2aUs)P>;g2%MKl^*m`8%%biD=~;*W8>%`m?2x?_c9kJ+|U=jeYLsychVa^h9jr qeNT3~`JGR8yue5JwHVJeHZ5juWtKa7ir(I=`;X!ec;3gdAO8o5vP1d+ literal 0 HcmV?d00001 diff --git a/docs/API.md b/docs/API.md index 892199f..c512cf5 100644 --- a/docs/API.md +++ b/docs/API.md @@ -605,6 +605,13 @@ struct ScratchRecallReport { // v2.3 // mathematically valid, statistically uninformative }; struct ScratchArchive { bool(*write)(void*, const void*, size_t); bool(*read)(void*, void*, size_t); void* user; }; +struct ScratchArchiveInfo { // v3.2.2 + int32_t capacity, count, dims, paddedDims; + Metric metric; Quantization quant; bool retainFloats; + int32_t channelCount; ChannelInfo channels[kMaxChannels]; // 0 / unused for single-space + int64_t archiveBytes; // exactly what a Load consumes +}; +Status PeekScratchArchive(const void* data, int64_t bytes, ScratchArchiveInfo* outInfo); // v3.2.2 ``` A snapshot IS a `BankView` — every query entry point works on it unchanged. @@ -620,6 +627,26 @@ allocation: everything lives in the arena from `Create` (retention included — `ArenaBytes` simply grows). Archive format: [FORMAT.md](FORMAT.md) section 3; concurrency guarantees: [DETERMINISM.md](DETERMINISM.md) section 2b. +**Geometry ceilings (v3.2.2):** every `Create` overload and `Grow` bound +`capacity` at `kMaxBankRows` and `dims` at `kMaxCrossDeviceDims` *before* the +arena size is computed, returning `InvalidArgument` above either. The arena math +multiplies capacity by dims in signed `int64`, so an unbounded `int32` pair is +overflow rather than a refused allocation. These are the immutable format's own +caps, already applied by the archive loader — a geometry `Create` refuses is one +no archive could have carried. + +**Peeking an archive (v3.2.2):** `PeekScratchArchive` reads a serialized +archive's header (and channel table) out of a contiguous byte span and reports +its geometry plus `archiveBytes` — exactly the number of bytes a `Load` will +consume — without allocating, reading the payload, or touching a bank. It +applies the same header rules `Load` does, so whatever a peek rejects a load +rejects, and a span shorter than the archive it declares is `BadFormat`; a +*longer* span is not, because whatever follows belongs to the caller. That is +the point: a host appending its own trailer after the archive (channel names, +say) uses `archiveBytes` to find and validate that trailer *before* committing +the load, instead of discovering a broken trailer with the rows already +replaced. Reading the header twice is free; unwinding a load is not. + **Recall audit (v2.3):** the import-time recall-honesty pattern, extended to the mutable half. `Create(..., retainFloats = true)` — opt-in, never the default — retains the post-normalization float row the quantizer consumed (on Cosine diff --git a/include/superfaiss/scratch.h b/include/superfaiss/scratch.h index a37aa63..66ca064 100644 --- a/include/superfaiss/scratch.h +++ b/include/superfaiss/scratch.h @@ -59,6 +59,37 @@ struct ScratchRecallReport bool informative = false; // false when liveRows < kRecallInformativeRows }; +// What a serialized scratch archive declares about itself, read from its header +// without loading it (PeekScratchArchive below). +struct ScratchArchiveInfo +{ + int32_t capacity = 0; + int32_t count = 0; + int32_t dims = 0; + int32_t paddedDims = 0; + Metric metric = Metric::Dot; + Quantization quant = Quantization::Float32; + bool retainFloats = false; + // The archive's channel table, or count 0 for a single-space archive. + int32_t channelCount = 0; + ChannelInfo channels[kMaxChannels] = {}; + // Exactly the number of bytes ScratchBank::Load consumes for this archive. + int64_t archiveBytes = 0; +}; + +// Reads a scratch archive's header (and channel table) from a contiguous byte span and +// reports its geometry plus the exact byte length a Load will consume — without +// allocating an arena, reading the payload, or touching any bank. Applies the same +// header rules Load does, so a header Load would reject is rejected here with the same +// status; a span shorter than the archive it declares is BadFormat. +// +// This exists for hosts that append their own trailer after the archive (the plugin's +// channel-name frame is one): archiveBytes locates that trailer so it can be validated +// BEFORE anything is committed. Without it a host must load first and discover a broken +// trailer with the rows already replaced, which is degrade-then-notice rather than +// reject-over-degrade. Reading the header twice is free; unwinding a load is not. +Status PeekScratchArchive(const void* data, int64_t bytes, ScratchArchiveInfo* outInfo); + class ScratchBank { public: @@ -78,6 +109,13 @@ class ScratchBank // Allocates the arena (rows + scales + tombstones + staging) in ONE allocation // through the seam — zero steady-state allocation after Create. Capacity is the // caller's memory budget made explicit. Not valid to call on a created bank. + // + // Geometry ceilings (every Create overload, and Grow): capacity <= kMaxBankRows and + // dims <= kMaxCrossDeviceDims, enforced BEFORE the arena size is computed. The arena + // math multiplies capacity by dims in signed int64, so an unbounded pair is overflow + // — undefined behavior rather than a refused allocation. Over-cap geometry is + // InvalidArgument. These are the immutable format's own caps, so a bank the + // constructor refuses is one no archive could carry either. Status Create( int32_t capacity, int32_t dims, diff --git a/include/superfaiss/version.h b/include/superfaiss/version.h index eb47d97..2ea7a0a 100644 --- a/include/superfaiss/version.h +++ b/include/superfaiss/version.h @@ -4,7 +4,7 @@ #define SUPERFAISS_VERSION_MAJOR 3 #define SUPERFAISS_VERSION_MINOR 2 -#define SUPERFAISS_VERSION_PATCH 1 +#define SUPERFAISS_VERSION_PATCH 2 namespace superfaiss { diff --git a/src/scratch.cpp b/src/scratch.cpp index e3d53d7..ed7e1f1 100644 --- a/src/scratch.cpp +++ b/src/scratch.cpp @@ -125,8 +125,10 @@ namespace constexpr uint8_t kScratchFlagChannels = 0x02; // reserved[0] bit 1 // Archive geometry ceiling (review M2): the largest row capacity a load will // entertain. With paddedDims capped at kMaxCrossDeviceDims, every ArenaBytes - // term stays below 2^49 — far from int64 overflow. - constexpr int32_t kMaxScratchArchiveRows = 1 << 28; + // term stays below 2^49 — far from int64 overflow. The same ceiling Create and + // Grow apply to a directly-supplied capacity: one number, so a geometry an + // archive cannot carry is also one the constructor refuses to build. + constexpr int32_t kMaxScratchArchiveRows = kMaxBankRows; struct ScratchHeader { @@ -141,8 +143,152 @@ namespace uint8_t reserved[6] = {}; }; static_assert(sizeof(ScratchHeader) == 32, "scratch header layout is the format"); + + // The header rules, shared by Load and PeekScratchArchive so the two can never + // disagree about what a valid archive header is — one validator, one truth. Pure on + // the header bytes: it allocates nothing and reads no payload. + Status ValidateScratchHeader( + const ScratchHeader& header, bool* outHasChannels, bool* outRetain) + { + // Accepts {1, 2, 3}; version 0 or anything newer is the standing old-reader/new-data + // hard-reject (a future v4 archive rejects here). Legacy 1/2 encode retention as the + // version integer; version 3 makes the flags byte authoritative. + if (header.magic != kScratchMagic || + header.version < 1u || header.version > kScratchVersionChannels) + { + return Status::BadFormat; + } + // Version 3: retention and channel presence come from the reserved[0] flags byte, and + // bits 2-7 are reserved — masked off, tolerated (never rejected, never mis-read as + // retention or channels; forward tolerance by design). Legacy 1/2 read retention from + // the version integer, exactly as shipped, and carry no channel table. + const uint8_t flags = header.reserved[kScratchFlagsByteIndex]; + const bool hasChannels = header.version == kScratchVersionChannels && + (flags & kScratchFlagChannels) != 0; + const bool wantRetain = header.version == kScratchVersionChannels + ? (flags & kScratchFlagRetention) != 0 + : header.version == kScratchVersionRetain; + // Version 3 is emitted ONLY for a channel-carrying bank (channels trigger the presence- + // flags format; a retention-only bank stays legacy v2). A v3 blob without the channels + // flag is therefore something the writer never produces — reject it as corrupt rather + // than adopt it as a plain bank. This also catches a legacy blob whose version integer + // was hand-bumped to 3 (its reserved bytes are zero, so the channels flag is clear). + if (header.version == kScratchVersionChannels && !hasChannels) + { + return Status::BadFormat; + } + const bool metricOk = header.metric <= static_cast(Metric::L2); + const bool quantOk = header.quant <= static_cast(Quantization::Int8); + if (!metricOk || !quantOk || header.capacity <= 0 || header.dims <= 0 || + header.count < 0 || header.count > header.capacity || + header.paddedDims != + PaddedDims(header.dims, static_cast(header.quant))) + { + return Status::BadFormat; + } + // The archive is an untrusted medium (review M2, the T-062 idiom): bound the + // geometry BEFORE any byte-size arithmetic — the arena math multiplies the + // caller-controlled capacity and dims, and an unbounded pair is signed int64 + // overflow (UB), not merely a failed allocation. paddedDims is capped at the + // widest bank the library proves (kMaxCrossDeviceDims); capacity at 2^28 rows + // keeps every ArenaBytes term below 2^49. Absurd geometry is a format defect — + // a hard BadFormat, never an allocator outcome. + if (header.paddedDims > kMaxCrossDeviceDims || + header.capacity > kMaxScratchArchiveRows) + { + return Status::BadFormat; + } + *outHasChannels = hasChannels; + *outRetain = wantRetain; + return Status::Ok; + } } // namespace +Status PeekScratchArchive(const void* data, int64_t bytes, ScratchArchiveInfo* outInfo) +{ + if (data == nullptr || outInfo == nullptr || bytes < 0) + { + return Status::InvalidArgument; + } + *outInfo = ScratchArchiveInfo{}; + if (bytes < static_cast(sizeof(ScratchHeader))) + { + return Status::BadFormat; + } + const uint8_t* cursor = static_cast(data); + ScratchHeader header; + std::memcpy(&header, cursor, sizeof(header)); + bool hasChannels = false; + bool retain = false; + const Status headerStatus = ValidateScratchHeader(header, &hasChannels, &retain); + if (headerStatus != Status::Ok) + { + return headerStatus; + } + int64_t consumed = static_cast(sizeof(ScratchHeader)); + + // The channel table, written immediately after the header on a v3 archive. Its COUNT is + // bounded here because it sizes the read below; the ranges themselves are validated where + // they are adopted (the channel Create inside Load), so a peek and a load agree on what a + // well-formed archive is without duplicating the geometry rules. + if (hasChannels) + { + if (bytes - consumed < static_cast(sizeof(int32_t))) + { + return Status::BadFormat; + } + int32_t channelCount = 0; + std::memcpy(&channelCount, cursor + consumed, sizeof(channelCount)); + consumed += static_cast(sizeof(channelCount)); + if (channelCount <= 0 || channelCount > kMaxChannels) + { + return Status::BadFormat; + } + const int64_t tableBytes = + static_cast(channelCount) * static_cast(sizeof(ChannelInfo)); + if (bytes - consumed < tableBytes) + { + return Status::BadFormat; + } + std::memcpy(outInfo->channels, cursor + consumed, static_cast(tableBytes)); + consumed += tableBytes; + outInfo->channelCount = channelCount; + } + + // The payload's byte length, in the order Save writes it. Every term is bounded by the + // header caps checked above, so the int64 arithmetic cannot overflow. + const Quantization quant = static_cast(header.quant); + consumed += static_cast(header.count) * header.paddedDims * ElementSize(quant); + if (quant == Quantization::Int8) + { + consumed += static_cast(header.count) * static_cast(sizeof(float)); + } + consumed += static_cast(ScratchBank::TombstoneWords(header.count)) * + static_cast(sizeof(uint32_t)); + if (retain) + { + consumed += static_cast(header.count) * header.dims * + static_cast(sizeof(float)); + } + // A span that does not cover the archive it declares is truncated, which is a format + // defect. A span that is LONGER is not: whatever follows belongs to the caller (a host + // trailer), and reporting archiveBytes is precisely how the caller finds it. + if (bytes < consumed) + { + return Status::BadFormat; + } + + outInfo->capacity = header.capacity; + outInfo->count = header.count; + outInfo->dims = header.dims; + outInfo->paddedDims = header.paddedDims; + outInfo->metric = static_cast(header.metric); + outInfo->quant = quant; + outInfo->retainFloats = retain; + outInfo->archiveBytes = consumed; + return Status::Ok; +} + ScratchBank::~ScratchBank() { Destroy(); @@ -245,6 +391,15 @@ Status ScratchBank::Create( { return Status::InvalidArgument; } + // Geometry ceilings BEFORE any size arithmetic (the Load idiom, now on the public + // construction path): ArenaBytes multiplies capacity by dims in signed int64, and an + // uncapped int32 pair overflows it — undefined behavior, not a failed allocation. The + // caps are the format's own (kMaxBankRows rows, kMaxCrossDeviceDims dims), so a bank + // this refuses to build is one no archive could have carried either. + if (capacity > kMaxScratchArchiveRows || dims > kMaxCrossDeviceDims) + { + return Status::InvalidArgument; + } const int32_t paddedDims = PaddedDims(dims, quant); const int64_t bytes = ArenaBytes(capacity, dims, paddedDims, quant, retainFloats, 0); @@ -295,6 +450,12 @@ Status ScratchBank::Create( { return Status::InvalidArgument; } + // The same geometry ceilings the retention overload applies, for the same reason: + // bound capacity and dims before ArenaBytes multiplies them. + if (capacity > kMaxScratchArchiveRows || dims > kMaxCrossDeviceDims) + { + return Status::InvalidArgument; + } const int32_t paddedDims = PaddedDims(dims, quant); @@ -595,8 +756,11 @@ Status ScratchBank::Grow(int32_t newCapacity) return Status::InvalidArgument; } WriterGuard guard(WriterBusy_); - if (newCapacity <= Capacity_) + if (newCapacity <= Capacity_ || newCapacity > kMaxScratchArchiveRows) { + // The row ceiling applies to a grown capacity exactly as it does to a created + // one: ArenaBytes below multiplies newCapacity by the row stride in signed + // int64, so an uncapped request is overflow, not an allocation failure. return Status::InvalidArgument; } const int32_t subNormPerRow = (Metric_ == Metric::Cosine && ChannelCount_ > 0) @@ -1110,53 +1274,15 @@ Status ScratchBank::Load(const ScratchArchive& archive, const Allocator& allocat { return Status::BadFormat; } - // Accepts {1, 2, 3}; version 0 or anything newer is the standing old-reader/new-data - // hard-reject (a future v4 archive rejects here). Legacy 1/2 encode retention as the - // version integer; version 3 makes the flags byte authoritative. - if (header.magic != kScratchMagic || - header.version < 1u || header.version > kScratchVersionChannels) + // Version acceptance, the presence-flags byte, and the geometry ceilings all live in + // the shared header validator PeekScratchArchive runs, so a peek and a load can never + // disagree about which archives are well-formed. + bool hasChannels = false; + bool wantRetain = false; + const Status headerStatus = ValidateScratchHeader(header, &hasChannels, &wantRetain); + if (headerStatus != Status::Ok) { - return Status::BadFormat; - } - // Version 3: retention and channel presence come from the reserved[0] flags byte, and - // bits 2-7 are reserved — masked off, tolerated (never rejected, never mis-read as - // retention or channels; forward tolerance by design). Legacy 1/2 read retention from - // the version integer, exactly as shipped, and carry no channel table. - const uint8_t flags = header.reserved[kScratchFlagsByteIndex]; - const bool hasChannels = header.version == kScratchVersionChannels && - (flags & kScratchFlagChannels) != 0; - const bool wantRetain = header.version == kScratchVersionChannels - ? (flags & kScratchFlagRetention) != 0 - : header.version == kScratchVersionRetain; - // Version 3 is emitted ONLY for a channel-carrying bank (channels trigger the presence- - // flags format; a retention-only bank stays legacy v2). A v3 blob without the channels - // flag is therefore something the writer never produces — reject it as corrupt rather - // than adopt it as a plain bank. This also catches a legacy blob whose version integer - // was hand-bumped to 3 (its reserved bytes are zero, so the channels flag is clear). - if (header.version == kScratchVersionChannels && !hasChannels) - { - return Status::BadFormat; - } - const bool metricOk = header.metric <= static_cast(Metric::L2); - const bool quantOk = header.quant <= static_cast(Quantization::Int8); - if (!metricOk || !quantOk || header.capacity <= 0 || header.dims <= 0 || - header.count < 0 || header.count > header.capacity || - header.paddedDims != - PaddedDims(header.dims, static_cast(header.quant))) - { - return Status::BadFormat; - } - // The archive is an untrusted medium (review M2, the T-062 idiom): bound the - // geometry BEFORE any byte-size arithmetic — the arena math multiplies the - // caller-controlled capacity and dims, and an unbounded pair is signed int64 - // overflow (UB), not merely a failed allocation. paddedDims is capped at the - // widest bank the library proves (kMaxCrossDeviceDims); capacity at 2^28 rows - // keeps every ArenaBytes term below 2^49. Absurd geometry is a format defect — - // a hard BadFormat, never an allocator outcome. - if (header.paddedDims > kMaxCrossDeviceDims || - header.capacity > kMaxScratchArchiveRows) - { - return Status::BadFormat; + return headerStatus; } // The channel table (v3), read here — before the arena is created — so the channel diff --git a/tests/test_main.cpp b/tests/test_main.cpp index ffa0e03..592fce3 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -2722,6 +2722,234 @@ namespace }; } // namespace +// The public construction paths bound their geometry BEFORE the arena size arithmetic +// runs on it. ArenaBytes multiplies capacity by dims in signed int64, so an uncapped +// int32 pair is overflow — undefined behavior, not a refused allocation. The archive +// loader already applied these ceilings; Create and Grow did not. +static void TestScratchGeometryCeilings() +{ + // Over-cap capacity: an argument defect, and nothing is allocated — the bank stays + // uncreated, so a subsequent valid Create on the same object still succeeds. + { + ScratchBank bank; + CHECK(bank.Create(kMaxBankRows + 1, 64, Metric::Dot, Quantization::Float32) == + Status::InvalidArgument); + CHECK(!bank.IsCreated()); + CHECK(bank.Create(16, 64, Metric::Dot, Quantization::Float32) == Status::Ok); + } + // Over-cap dims, on the retention overload. + { + ScratchBank bank; + CHECK(bank.Create(16, kMaxCrossDeviceDims + 1, Metric::Dot, Quantization::Float32) == + Status::InvalidArgument); + CHECK(!bank.IsCreated()); + } + // The channel overload carries its own preamble, so it needs its own proof. A fresh + // bank per case: on a shared object the second call would be refused for being + // already-created, and the cap would read proven while never having run. + { + const ChannelInfo channels[1] = {{0, 16}}; + ScratchBank overRows; + CHECK(overRows.Create(kMaxBankRows + 1, 64, Metric::Cosine, Quantization::Float32, + channels, 1) == Status::InvalidArgument); + CHECK(!overRows.IsCreated()); + ScratchBank overDims; + CHECK(overDims.Create(16, kMaxCrossDeviceDims + 1, Metric::Cosine, + Quantization::Float32, channels, 1) == Status::InvalidArgument); + CHECK(!overDims.IsCreated()); + } + // The pair that overflows the arena arithmetic outright. Rejected on geometry, so the + // multiply never runs on it. + { + ScratchBank bank; + CHECK(bank.Create(INT32_MAX, INT32_MAX, Metric::Dot, Quantization::Float32) == + Status::InvalidArgument); + CHECK(!bank.IsCreated()); + } + // Grow takes the same ceiling, and a refused Grow leaves the bank exactly as it was. + { + ScratchBank bank; + CHECK(bank.Create(16, 64, Metric::Dot, Quantization::Float32) == Status::Ok); + CHECK(bank.Grow(kMaxBankRows + 1) == Status::InvalidArgument); + CHECK(bank.Grow(INT32_MAX) == Status::InvalidArgument); + CHECK(bank.Capacity() == 16); + CHECK(bank.Grow(32) == Status::Ok); + CHECK(bank.Capacity() == 32); + } +} + +// PeekScratchArchive reports a serialized archive's geometry and — the reason it exists — +// the exact byte length a Load consumes, so a host that appends its own trailer can find +// and validate that trailer BEFORE committing the load. +static void TestPeekScratchArchive() +{ + Rng rng(0x9E5A11ull); + const int32_t dims = 64; + const ChannelInfo channels[2] = {{0, 32}, {32, 32}}; + + struct Shape + { + const char* name; + bool hasChannels; + bool retain; + Quantization quant; + Metric metric; + }; + const Shape shapes[] = { + {"legacy f32 dot", false, false, Quantization::Float32, Metric::Dot}, + {"legacy int8 cosine", false, false, Quantization::Int8, Metric::Cosine}, + {"retention int8 cosine", false, true, Quantization::Int8, Metric::Cosine}, + {"channels f32 cosine", true, false, Quantization::Float32, Metric::Cosine}, + {"channels+retention int8 cosine", true, true, Quantization::Int8, Metric::Cosine}, + {"channels int8 dot", true, false, Quantization::Int8, Metric::Dot}, + }; + for (const Shape& shape : shapes) + { + ScratchBank bank; + const Status created = shape.hasChannels + ? bank.Create(32, dims, shape.metric, shape.quant, channels, 2, shape.retain) + : bank.Create(32, dims, shape.metric, shape.quant, shape.retain); + CHECK_MSG(created == Status::Ok, "%s: create failed (%d)", shape.name, + static_cast(created)); + if (created != Status::Ok) + { + continue; + } + std::vector row(static_cast(dims)); + for (int32_t r = 0; r < 20; ++r) + { + for (auto& v : row) + { + v = rng.NextFloat(); + } + CHECK(bank.Append(row.data(), dims, nullptr) == Status::Ok); + } + CHECK(bank.Remove(4) == Status::Ok); // tombstone words are part of the length + + MemArchive archive; + CHECK(bank.Save(archive.Writer()) == Status::Ok); + + ScratchArchiveInfo info; + const Status peeked = PeekScratchArchive( + archive.bytes.data(), static_cast(archive.bytes.size()), &info); + CHECK_MSG(peeked == Status::Ok, "%s: peek failed (%d)", shape.name, + static_cast(peeked)); + if (peeked != Status::Ok) + { + continue; + } + CHECK_MSG(info.archiveBytes == static_cast(archive.bytes.size()), + "%s: archiveBytes %lld, blob %lld", shape.name, + static_cast(info.archiveBytes), + static_cast(archive.bytes.size())); + CHECK(info.count == bank.Count()); + CHECK(info.capacity == bank.Capacity()); + CHECK(info.dims == dims); + CHECK(info.paddedDims == bank.GetPaddedDims()); + CHECK(info.metric == shape.metric); + CHECK(info.quant == shape.quant); + CHECK_MSG(info.retainFloats == shape.retain, "%s: retention flag lost", shape.name); + CHECK_MSG(info.channelCount == (shape.hasChannels ? 2 : 0), + "%s: channelCount %d", shape.name, info.channelCount); + for (int32_t c = 0; c < info.channelCount; ++c) + { + CHECK(info.channels[c].offset == channels[c].offset && + info.channels[c].length == channels[c].length); + } + + // A span that does not cover the archive it declares is truncated — a format + // defect, never a short read to paper over. + ScratchArchiveInfo truncated; + CHECK(PeekScratchArchive(archive.bytes.data(), + static_cast(archive.bytes.size()) - 1, &truncated) == + Status::BadFormat); + CHECK(PeekScratchArchive(archive.bytes.data(), 8, &truncated) == Status::BadFormat); + CHECK(PeekScratchArchive(archive.bytes.data(), 0, &truncated) == Status::BadFormat); + + // Trailing bytes belong to the caller: the peek succeeds and points at them. + std::vector withTrailer = archive.bytes; + withTrailer.push_back(0xABu); + withTrailer.push_back(0xCDu); + ScratchArchiveInfo trailed; + CHECK(PeekScratchArchive(withTrailer.data(), + static_cast(withTrailer.size()), &trailed) == Status::Ok); + CHECK_MSG(trailed.archiveBytes == static_cast(archive.bytes.size()), + "%s: trailer moved the reported archive length", shape.name); + } + + // The header rules are the loader's own: whatever a peek rejects, a Load rejects. + { + ScratchBank bank; + CHECK(bank.Create(8, 32, Metric::Dot, Quantization::Float32) == Status::Ok); + MemArchive archive; + CHECK(bank.Save(archive.Writer()) == Status::Ok); + + std::vector badMagic = archive.bytes; + badMagic[0] = static_cast(badMagic[0] ^ 0xFFu); + std::vector badVersion = archive.bytes; + badVersion[4] = 4u; // a future version: the standing old-reader/new-data reject + std::vector zeroVersion = archive.bytes; + zeroVersion[4] = 0u; + std::vector forgedV3 = archive.bytes; + forgedV3[4] = 3u; // v3 without the channels flag is a shape the writer never emits + + const std::vector* bad[] = {&badMagic, &badVersion, &zeroVersion, &forgedV3}; + for (const std::vector* blob : bad) + { + ScratchArchiveInfo info; + CHECK(PeekScratchArchive(blob->data(), static_cast(blob->size()), &info) == + Status::BadFormat); + MemArchive corrupt; + corrupt.bytes = *blob; + ScratchBank target; + CHECK(target.Load(corrupt.Reader()) == Status::BadFormat); + } + + ScratchArchiveInfo info; + CHECK(PeekScratchArchive(nullptr, 32, &info) == Status::InvalidArgument); + CHECK(PeekScratchArchive(archive.bytes.data(), + static_cast(archive.bytes.size()), nullptr) == + Status::InvalidArgument); + } + + // The header claims the peek allocates nothing — proven, not asserted. Everything the + // tracked scope touches is built before it opens. + { + const ChannelInfo table[2] = {{0, 32}, {32, 32}}; + ScratchBank bank; + CHECK(bank.Create(32, dims, Metric::Cosine, Quantization::Int8, table, 2, + /*retainFloats=*/true) == Status::Ok); + std::vector row(static_cast(dims)); + for (int32_t r = 0; r < 12; ++r) + { + for (auto& v : row) + { + v = rng.NextFloat(); + } + CHECK(bank.Append(row.data(), dims, nullptr) == Status::Ok); + } + MemArchive archive; + CHECK(bank.Save(archive.Writer()) == Status::Ok); + const uint8_t* blob = archive.bytes.data(); + const int64_t blobBytes = static_cast(archive.bytes.size()); + ScratchArchiveInfo info; + CHECK(PeekScratchArchive(blob, blobBytes, &info) == Status::Ok); // warm + + const uint64_t allocsBefore = AllocationCount(); + { + ScopedRawNewTracking rawTracking; + for (int32_t i = 0; i < 20; ++i) + { + CHECK(PeekScratchArchive(blob, blobBytes, &info) == Status::Ok); + } + CHECK_MSG(rawTracking.Count() == 0, + "PeekScratchArchive allocated %llu time(s) outside the seam", + static_cast(rawTracking.Count())); + } + CHECK(AllocationCount() == allocsBefore); + } +} + static void TestScratchBanks() { Rng rng(0x5C247C4Bull); @@ -17864,6 +18092,7 @@ namespace {"scratch.h", "ScratchBank::GetChannels", AllocBinding::Trivial, "TestAllocFlatScratchAccessors", 1}, {"scratch.h", "ScratchBank::RecallReportStale", AllocBinding::Trivial, "TestAllocFlatScratchAccessors", 1}, + {"scratch.h", "PeekScratchArchive", AllocBinding::Binds, "TestPeekScratchArchive", 1}, // --- topk.h (6 entry points, all Trivial) --- {"topk.h", "Better", AllocBinding::Trivial, "TestAllocFlatHeaderOnlyMath", 1}, @@ -17899,7 +18128,7 @@ namespace {"alloc.h", 26}, {"analytics.h", 10}, {"bake.h", 5}, {"compose.h", 4}, {"graph.h", 4}, {"inspector_common.h", 1}, {"kernels.h", 36}, {"matching.h", 1}, {"novelty.h", 4}, {"pca.h", 2}, {"query.h", 5}, - {"scratch.h", 40}, {"superfaiss.h", 0}, {"topk.h", 6}, {"types.h", 8}, + {"scratch.h", 41}, {"superfaiss.h", 0}, {"topk.h", 6}, {"types.h", 8}, {"validate.h", 5}, {"version.h", 0}, }; @@ -19394,6 +19623,11 @@ int main() TestS1FlatAllocationKthNeighborDistanceProbe(); TestS1FlatAllocationMutualNearestMatches(); + // V3.2.2: public-path geometry ceilings, and the archive peek the host trailer + // validation is built on. + TestScratchGeometryCeilings(); + TestPeekScratchArchive(); + // Coverage audit §7 -- the structural registry guard. Runs last so every // cell it references above has already executed at least once. TestAllocationCellRegistryComplete(); From 04977ef551898d87c69a9a3a1b209de7d2183561 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 12:12:10 -0700 Subject: [PATCH 02/14] Validate: a zero-row Cosine channel bank is representable ValidateBank required the per-channel inverse sub-norm array whenever a Cosine bank carried a channel table. Those norms are one per row, so a bank with no rows requires none, and no scan reads the array at zero rows. The requirement now applies only when count > 0. The shape is not degenerate. It is what a channel-carrying scratch bank graduates into once every row has been removed, and rejecting it forced that graduation to drop its channel table rather than fail. Also pins version.h at 3.2.2 in the coherence cell. Suite: 85,081 checks, 0 failures. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 6 +++++ src/validate.cpp | 9 ++++++-- tests/test_main.cpp | 54 +++++++++++++++++++++++++++++++++++++++------ 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f705c4..6ff5143 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,12 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu `kMaxCrossDeviceDims` dims) before any size is computed, returning `InvalidArgument`. The archive loader already applied these caps; direct construction did not, and `Grow` bounded its request only against the current capacity. +- **A Cosine channel bank with zero rows was unrepresentable.** `ValidateBank` required + the per-channel inverse sub-norm array whenever a Cosine bank carried channels, but + those norms are one per row — a bank with no rows requires none, and no scan reads the + array at zero rows. The rule now applies only when `count > 0`. The shape is not + degenerate: it is what a channel-carrying scratch bank graduates into once every row + has been removed, and rejecting it forced that graduation to drop its channels. ## [3.2.1] — 2026-07-21 diff --git a/src/validate.cpp b/src/validate.cpp index 1bfdb78..3df8461 100644 --- a/src/validate.cpp +++ b/src/validate.cpp @@ -77,8 +77,13 @@ Status ValidateBank(const BankView& bank) } prevEnd = channel.offset + channel.length; } - // Per-channel cosine requires the baked inverse sub-norms. - if (bank.metric == Metric::Cosine && bank.channelInvNorms == nullptr) + // Per-channel cosine requires the baked inverse sub-norms — one per row, so a bank + // with no rows requires none. Demanding the array unconditionally made a zero-row + // Cosine channel bank unrepresentable, which is not a degenerate case: it is what + // a channel-carrying scratch bank graduates into when every row has been removed, + // and an emptied channel bank is still a channel bank. No scan reads the array at + // zero rows, so there is nothing to protect against here. + if (bank.metric == Metric::Cosine && bank.count > 0 && bank.channelInvNorms == nullptr) { return Status::BadFormat; } diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 592fce3..9529e2c 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -2778,6 +2778,45 @@ static void TestScratchGeometryCeilings() } } +// A Cosine channel bank with zero rows is representable: the inverse sub-norms are one +// per row, so no rows require none. This is the shape a channel-carrying scratch bank +// graduates into once every row has been removed — an emptied channel bank is still a +// channel bank, and rejecting it forced the graduation to drop its channels instead. +static void TestEmptyCosineChannelBankValidates() +{ + const ChannelInfo channels[2] = {{0, 32}, {32, 32}}; + for (Quantization quant : {Quantization::Float32, Quantization::Int8}) + { + BankView empty; + empty.rows = nullptr; + empty.scales = nullptr; + empty.count = 0; + empty.dims = 64; + empty.paddedDims = PaddedDims(64, quant); + empty.quant = quant; + empty.metric = Metric::Cosine; + empty.channels = channels; + empty.channelCount = 2; + empty.channelInvNorms = nullptr; + CHECK_MSG(ValidateBank(empty) == Status::Ok, + "zero-row Cosine channel bank rejected (quant %d)", static_cast(quant)); + CHECK(ValidateBankData(empty, nullptr) == Status::Ok); + } + // With rows present the array is still required — the relaxation is about count, not + // about the rule. + { + Rng rng(0x4E5701ull); + const int32_t dims = 64, count = 8; + TestBank bank(rng, count, dims, Quantization::Int8, Metric::Cosine); + BankView view = bank.view; + view.channels = channels; + view.channelCount = 2; + view.channelInvNorms = nullptr; + CHECK_MSG(ValidateBank(view) == Status::BadFormat, + "a populated Cosine channel bank must still require its sub-norms"); + } +} + // PeekScratchArchive reports a serialized archive's geometry and — the reason it exists — // the exact byte length a Load consumes, so a host that appends its own trailer can find // and validate that trailer BEFORE committing the load. @@ -12730,17 +12769,17 @@ static void TestPerChannelRecallOracle() } } -// [hole] Version/header coherence. version.h must declare v3.0.0 for the v3.0 release. RED now -// (it reads 2/5/0); the header is bumped separately. Kept as a standalone assertion so the release-version -// truth has a test, not just a header edit. +// Version/header coherence: version.h declares the release this tree IS. Kept as a +// standalone assertion so the release-version truth has a test, not just a header edit — +// the CHANGELOG's newest entry and these three numbers move together or the suite says so. static void TestVersionHeaderCoherence() { CHECK_MSG(SUPERFAISS_VERSION_MAJOR == 3, - "SUPERFAISS_VERSION_MAJOR should be 3 for v3.2.1, got %d", SUPERFAISS_VERSION_MAJOR); + "SUPERFAISS_VERSION_MAJOR should be 3 for v3.2.2, got %d", SUPERFAISS_VERSION_MAJOR); CHECK_MSG(SUPERFAISS_VERSION_MINOR == 2, - "SUPERFAISS_VERSION_MINOR should be 2 for v3.2.1, got %d", SUPERFAISS_VERSION_MINOR); - CHECK_MSG(SUPERFAISS_VERSION_PATCH == 1, - "SUPERFAISS_VERSION_PATCH should be 1 for v3.2.1, got %d", SUPERFAISS_VERSION_PATCH); + "SUPERFAISS_VERSION_MINOR should be 2 for v3.2.2, got %d", SUPERFAISS_VERSION_MINOR); + CHECK_MSG(SUPERFAISS_VERSION_PATCH == 2, + "SUPERFAISS_VERSION_PATCH should be 2 for v3.2.2, got %d", SUPERFAISS_VERSION_PATCH); } // =========================================================================== @@ -19626,6 +19665,7 @@ int main() // V3.2.2: public-path geometry ceilings, and the archive peek the host trailer // validation is built on. TestScratchGeometryCeilings(); + TestEmptyCosineChannelBankValidates(); TestPeekScratchArchive(); // Coverage audit §7 -- the structural registry guard. Runs last so every From e59ba8d10be9ffa80899850661243d13a0d45140 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 12:35:45 -0700 Subject: [PATCH 03/14] Do not track compiled Python bytecode ci/__pycache__/_cxx_decls.cpython-313.pyc was committed alongside the coherence checks. It is interpreter- and machine-specific, regenerates on every ci run, and nothing in .gitignore prevented the next one. Untracked, and the class is now ignored. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 2 ++ ci/__pycache__/_cxx_decls.cpython-313.pyc | Bin 8348 -> 0 bytes 2 files changed, 2 insertions(+) delete mode 100644 ci/__pycache__/_cxx_decls.cpython-313.pyc diff --git a/.gitignore b/.gitignore index 8cdc92b..f3d9a46 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,5 @@ out/ *.ilk .vs/ .idea/ +__pycache__/ +*.pyc diff --git a/ci/__pycache__/_cxx_decls.cpython-313.pyc b/ci/__pycache__/_cxx_decls.cpython-313.pyc deleted file mode 100644 index eaac170c167d5dfb8427b064975c643db7cdf8c4..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 8348 zcmbtZTWnj$nLdX+JiHuI6h+-EUvlgyjwstwQrmGY#c^fJH(9n~9n+R0I@A$)C^4oE znK_hgF&A(*50&h;qUso?n+3w61?s#kyg-4?(FwHBxht5E6wWHUpvUE8;mjY0g8W)A$%T}ff z%xGyKc=5`sCltABy(3Q7iO1p^F=X&lj*?_5rA7D3U}8kW{$Y$6D}zR!v|$8aLD_EQF&A;A}2V z#|@eO>1m2zQfEL0b_N5%7q(}WQeY13AOeQbYoy6+BrVTqGm{!|RUzsn#9+KlXxFs_ zB|_JXG?kaArlsVrc#qao0meqrIME`&-WAskFbc#?uw5p4C(8Rw$dk$F++KR*Ral^0 zfu&*yakxR$R0>Vk1Z9wsQX|?#phrHFh74$itS7<6bXuE9fjLPgzThT46KYyBuyjg; zWMhNmb&VvFYLxcXQ3eF!SmwX7?_((l4M+l~aY{@!z#1H?;#NM>%T#)PJ0^<3Mvp3Y;(|t75-D~nP z$g#x|>Xeaj^!ARw9@4LNn_R$ZT8L&^joC?492~hgFgy%6fvU1WNt4YTgMA@o792l0 za7Ix=k$5N^xp^~8?E*C#W&@5ro(e-JsqPxYi&<^>H(o@Q7Td!)0P#fKRa0M-;*bW14XGTJ6Mvu*|U!^iHp#xS}IK8A7{{WmkV)(hds6^Jh48< zDVC6+Sf9rTYYDU!vhCtRqGCJD6T2c}+|I@wu)9utx!9Kk3ry#_*;mz)x)i_CP_e~L zinnZwl}6&3@&%erVGuNbxgA@8Uuj|rRCrA*EDc!ZjcPGHkHuG7PJZ6F-M+3 zRZoeHDX-~bw6Jy;h1i7()HF8Vb1XUj;kknFE9vJ_v19Lp=xWcgm7ZhE-s8)T<6o!I zj(Lp6U!nmQ&yUt?GWuu%?wjaal`@^*oN|g6Eh^?JmJtLCI`n)qE;Mk8fDuO-zP`qY zEy$x5gO&zvza9E)KcBJel`|rI4P=K24WU$+e3!`w0ya~4BN^B0I7s=*1S4Q2GHp}l znxZiqi^Xpm^!Q|1E{lniF*Ho2x?!Ec^m`A$jMJ#VWM9j@ElXSS2R`h--(U1SKQETH z?_AyfBL0@z9};{vwchJm>dJNGU(8?p@XGxw`Ll&LAGAJjJmeqle%QY3?OS&Av7`8i zqt;X`J89C4o!ODx#iFpYpy_e9`rTb_u#ZIJ)7Xtnj1m!=B_M4Sslz!c*;CCGGWH&cmtB%U_y*W-kS1g4Xn zJoxT`8*ut^&jRCrw_42HZBMNS+E{Zr>GYZ(EQi9g1!c>1{iktP^5Z1A7 zs&>LU zVdn=O=v^FM7|u6+*ml3I;QwITC+^*)rsiA6KX-cspRl1 zx)XEX`0Lh@62wG_9uM;Xls_9v^+H4Ajq2e<0XM~s*R1Ad zo}n7~RO6M+L}0VEnaFaAl?ma-eKzqi*az0Ndi{d6=Brq$yO{`4zq_F*wyNkUVn6m! zeSZ_uUWG(__HFb!Xrx!A7oz7ppAnAqba$3rzi>Snk7l;L-qq7R9_kM54jESiUE>4$ zUQ_pF!dF8#x&xhG(j907S2q6C_WacvGWI|$ZLn_f6mg-8NT^KPjGB&2o3<&EoJ~QD z`IO1e7<7TEfHsEpB$-hY@r)K`u?e|=(Np+0-a!S)bT}973-;U2?7(M|8?nL{4%ec1 zOPs%vx8w(kjvcFx&J{;z(XpEWJt?T+~ii`^Mp8_K4_ zg|JA0our2zg)mXkVFJJeO|j`b%x9%+=E@;#;<~Wa1P!s9Vt;CGO;oC@v8#Z{M73f) zsX~ozCY&|9SY5xYZi+*FuzVR%?F)MbYc)Gp?(q4BGG|1gha1v5btf{>t=y;l2LB}YRdSj`|#4IRG{VeGG zMqHia)M>K<9s+4ys>5mX6LtqBgma$#R+s{3^#s0omve}SC?CIdZC+_uUsHDm?MeeX z6UZ^zUUsICEqGn`NZ)u%758Axg!#Ek9s`krVF5khsqPu4cQ{spo~i`BYB`5sh+?F> zbx+X!)GWwr@;@U~KB)9~01IiX!Mdtvz>GL@Uj0~GWzsQD!%|kpJ{tR1$cpclg+JUI(U7WNXlB7wwlCo>J9K*;E3rV+oL7nxrt)JT5$zbKhDYFdv5LX#V; zYR1uZwD0w|Lq;HD2~Czm>eHA7t7?hIOlvGjOxulVWUvsPAzc%hWfLaoG@9 z20ZJAm<(enOhm2$=GcKqg02C>OjLae7srIAlXDD&S!%>k z7I7jZ7L4>vx_mb}PG>og2GPUu=uMN4lM}Q@pouBdRlaS~;GMk601;+~X(EhaZCb9~ zC?h?BL4zs~0++f{z7aH1$6#nIEY|4Fo@ju)M5nCb*w!!hyYtWH(?zLsRq9@mx{K1@ z?BHjTH@80@y>qk>dD#4mB>Y0Db!&Fyk=^;jxxb%V_U$SRuG#}D_CTp)$GrEE*SC26 z*7fDq=O6eVM2p`2^8!)To~HNr=hAl$=igj9 zQrK0P{H-T2&zC&Dh0Epm3Gq=SUb@BNr~N;^vK~KqoAdmSGo`KDa*j_MTfXvf4c%)^ zoTG8Y-bN7x>dsvI{LxiOUXkRYvFZ|a1%t~N*&B{5wYaB+t61g3n zMc=`nUCQ(KcHX}pm6qm26E7Ih$F!`51_%QJ=e-OE* zmkes~{;j9w)B)i)PVv-X;WvI-XM9Zax|r$(j#A~TT}dD%9Q7+OJpAJ|7zVg*+Q-Ve zTlx;s6JH-a!8xwSaadKc;)X4huQ_njxUlFp{EOrCZoF|X``A<;C^^NexY~`0%@r`sC{R z0;fC2jxqC?rSLVwImdD1HFq~!bM@;hVAl$NYjeVwQ*(Cfb8pZnVl5NnkkQ4tgqc zs_Rm>ffHf5~(fC~(Q?oki19}p750?xU@&1b8p zt2fl~q@hadcjw7ECai_C)Wxz~sgU(kJfj5pt$QUY58O_*%c_~y5zU2&pBiL@;eg&`3BAAyXZRu zeGoCO#p(MD-Y_C~CYVN=zxt{rtJEWw^Z%MW@z#Vqts3&Ars+716n*!K_P8>;5EmTn zOU+Iq-ST#d#INIw@S9BIQ9z5*$fA!$97s{okb;~+g*MgjatCdh{9BpUP@wDR5ekq0 z+WjqXoW4fH)=x#`fy{*R+Y>3DYVPcBYAxOC~`-P!Y>w|7)CBEF{V5U!!w zfkzH$evsZoOD^e0&uwt?tsMn*xwUJxwP&TZr`YRL^Zk#XFSZ<; zKlz!rrPQ&r&{H`1tD&C{{bG2zx2;ich%@BNX_w{+yhQgaJNTHEeurM9h0Hww?*dAHQQqp(0W35BiY{#Js$3>11wT&C9j5-__O{xt7*EzjOktZQqe| zecIT*9;0mO$X~eMvM@xGKl^Xj1uq*17!5$A@aA$`ASY1VFBY4-zm9vUJaeA1bpFDE zIM1(*@SNoS$#ZM3@|@dONdh4oPbG|HWXt(jmYWsGz|+0$_yjDCAlG3t5HcdaEuY7m z8wRQ9)D22BuUs7!>$XWyIQquHHq5x#y9oup8fY}_VfIeV(!jAZpxx9tl~s&JECks@t}c|;@M+rV`!QmAa*YmBY2A?MH61UPQ_ioDw=fDR=8Ooc zH7n2aUs)P>;g2%MKl^*m`8%%biD=~;*W8>%`m?2x?_c9kJ+|U=jeYLsychVa^h9jr qeNT3~`JGR8yue5JwHVJeHZ5juWtKa7ir(I=`;X!ec;3gdAO8o5vP1d+ From 592af9f8c6a27e3fc4c2ef7a9f0e646ad3f81cdb Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 14:29:49 -0700 Subject: [PATCH 04/14] Kernels: one float32 selection rule, applied to the length in hand The float32 dispatchers DotF32/L2F32 select AVX2+FMA only when the length they are handed is a multiple of 8. ResolveRowKernels, whose table feeds the segmented scan and DecomposeRowScore, wired the AVX2 kernels in directly with no such rule. Float32 strides are multiples of 4, so at paddedDims 20, 36, 52, 100 and 132 the whole-row scan computed a 4-lane mul-add where the segmented scan computed an 8-lane FMA, and the two disagreed in the last ulp on the same row. That falsified two published guarantees: that a degenerate one-segment query equals the whole-row scan, and that decomposition contributions sum bit-exactly to the scan's own score because no second code path exists to drift. ResolveRowKernels now holds the dispatchers themselves, so the rule is applied once. It has to key on the length rather than on bank shape, because the segmented scan calls the kernel once per scan range and a bank whose stride is a multiple of 8 can still present a range that is not. int8 is untouched: channel and segment ranges sit on the 16-byte element grid, so int8 lengths are always multiples of 16 and that dispatcher carries no stride condition. Exactness::CrossDevice was therefore never affected and its cross-machine contract is unchanged. No pinned golden moves - every one sits at a width or quantization where the two paths already agreed. TestSegmentedEqualsWholeRowAcrossWidths sweeps the widths that can break it and compares the two PATHS to each other, which is the check the suite lacked: T11 compares each kernel against its own scalar mirror, and that stays green while the paths pick different kernels, because each is individually correct. The existing degenerate-segment assertion in TestSegmentedScan covers the mechanism but runs only at dims 32, where the stride is a multiple of 8 and the two paths cannot disagree. Confirmed to fail against the unfixed selector at exactly paddedDims 20, 36 and 52 on Dot and L2, with 32 and 64 clean. Corrects a comment in the T11b block asserting that f32 paddedDims is always a multiple of 8, which is what let this go unnoticed. Suite: 85,441 checks, 0 failures. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 15 ++++++ src/kernels.cpp | 37 +++++++++++--- tests/test_main.cpp | 116 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 156 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ff5143..1727316 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,21 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu trailer with the rows already replaced. ### Fixed +- **Two kernel-selection rules made the segmented scan disagree with the whole-row + scan.** The float32 dispatchers `DotF32`/`L2F32` use AVX2+FMA only when the length they + are handed is a multiple of 8; `ResolveRowKernels`, which feeds the segmented scan and + `DecomposeRowScore`, wired the AVX2 kernels in directly with no such rule. Float32 + strides are multiples of 4, so at `paddedDims` 20, 36, 52, 100, 132 and similar the two + paths computed the same row with different accumulation widths and disagreed in the + last ulp. That falsified two published guarantees — that a degenerate one-segment query + equals the whole-row scan, and that decomposition contributions sum bit-exactly to the + scan's own score, "no second code path exists to drift". `ResolveRowKernels` now holds + the dispatchers, so the rule is applied once, to the length actually in hand — which + matters because the segmented scan calls the kernel per scan RANGE, and a bank whose + stride is a multiple of 8 can still present a range that is not. + **`Exactness::CrossDevice` was never affected** (int8 lengths are always multiples of + 16), so the cross-machine contract is unchanged, and no pinned golden moves: every one + of them sits at a width or quantization where the two paths already agreed. - **`ScratchBank::Create` and `Grow` performed signed size arithmetic before bounding their geometry.** `ArenaBytes` multiplies capacity by dims in signed `int64`; both entry points accepted any positive `int32` pair, so a large valid-typed request diff --git a/src/kernels.cpp b/src/kernels.cpp index 8f66e40..d35bb8e 100644 --- a/src/kernels.cpp +++ b/src/kernels.cpp @@ -356,10 +356,18 @@ static float L2I8Sse(const int8_t* row, float scale, const float* query, int32_t return (SumLanes(acc0) + SumLanes(acc1)) + (SumLanes(acc2) + SumLanes(acc3)); } -// Dispatchers: AVX2+FMA when the CPU has it and the stride fits 8-lane blocks +// Dispatchers: AVX2+FMA when the CPU has it and the LENGTH fits 8-lane blocks // (float32 strides are multiples of 4, not always 8; int8 strides are always multiples -// of 16). The choice is a pure function of device and bank shape, so per-device -// determinism is preserved. +// of 16). The choice is a pure function of device and the length handed in, so +// per-device determinism is preserved. +// +// These dispatchers are the only place the choice is made. The segmented scan reaches +// them through ResolveRowKernels' function-pointer table and calls them once per scan +// range, so the deciding length is the RANGE's, not the bank's — which is why the rule +// lives here, keyed on the argument, rather than being resolved once from bank shape. +// Wiring an AVX2 kernel into that table directly creates a second selection rule, and +// the whole-row and segmented paths then compute different last-ulp results on the same +// row; see TestSegmentedEqualsWholeRowAcrossWidths. float DotF32(const float* row, const float* query, int32_t paddedDims) { @@ -908,18 +916,33 @@ namespace inline FRowKernels ResolveRowKernels() { FRowKernels k; + // The float32 slots hold the DISPATCHERS, never the AVX2 kernels directly. Those + // dispatchers apply the `% 8 == 0` rule to the length they are actually handed, + // and float32 strides are multiples of 4, not 8 — so wiring the AVX2 kernel in + // here made this a second kernel-selection rule, and the segmented scan computed + // an 8-lane FMA where the whole-row scan computed a 4-lane mul-add. The results + // differ in the last ulp, which falsified two published guarantees: that a + // degenerate one-segment query equals the whole-row scan, and that decomposition + // contributions sum bit-exactly to the score the scan produced. + // + // Note the rule keys on the RANGE length, not on paddedDims: this scan calls the + // kernel once per scan range, so a bank whose stride is a multiple of 8 can still + // present a range that is not. One rule, evaluated on the length in hand, is what + // makes the two paths one path. + // + // int8 needs no such care: channel and segment ranges sit on the 16-byte element + // grid, so an int8 length is always a multiple of 16, and its dispatcher carries + // no stride condition to diverge over. + k.dotF32 = detail::DotF32; + k.l2F32 = detail::L2F32; #if defined(SUPERFAISS_SIMD_SSE) if (detail::IsAvx2()) { - k.dotF32 = detail::DotF32Avx2; - k.l2F32 = detail::L2F32Avx2; k.dotI8 = detail::DotI8Avx2; k.l2I8 = detail::L2I8Avx2; return k; } #endif - k.dotF32 = detail::DotF32; - k.l2F32 = detail::L2F32; k.dotI8 = detail::DotI8; k.l2I8 = detail::L2I8; return k; diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 9529e2c..181a2d5 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -1100,11 +1100,15 @@ static void TestSimdEqualsScalar() // Where the defect actually bites (established while authoring this test — the coord's // original "length-4 scores 0 on the whole-row path" framing was imprecise): // * The WHOLE-ROW dispatch DotF32()/L2F32() (src/kernels.cpp) selects the AVX2 -// intrinsic ONLY when paddedDims % 8 == 0 (else DotF32Sse, which handles the tail). -// PaddedDims is always a multiple of 8 for f32 banks big enough to matter, so a -// length ≡ 4 mod 8 never reaches the intrinsic through that dispatch — the whole- -// row path was NOT wrong in production. That is why the correctness check in Part A -// runs against the scalar MIRROR directly, not through DotF32(). +// intrinsic ONLY when the length it is handed is a multiple of 8 (else DotF32Sse, +// which handles the tail), so a length ≡ 4 mod 8 never reaches the intrinsic through +// that dispatch. That is why the correctness check in Part A runs against the scalar +// MIRROR directly, not through DotF32(). +// (This bullet used to add "PaddedDims is always a multiple of 8 for f32 banks big +// enough to matter". That is FALSE — f32 strides are multiples of 4, so dims 20, 36, +// 52, 100 and 132 all produce odd strides — and the belief is what let the two scan +// paths diverge unnoticed at those widths. TestSegmentedEqualsWholeRowAcrossWidths +// now sweeps them and compares the two paths to each other.) // * The defect bites the SEGMENTED / per-channel-cosine scan: ResolveRowKernels() // (src/kernels.cpp) wires k.dotF32 = DotF32Avx2 RAW (no % 8 guard), and the non- // foldable per-channel-cosine path (ScoreChunkSegmented) calls it over each @@ -2131,6 +2135,107 @@ static void TestPca() // B — repeat determinism, external chunk-fusion merge bit-identity, // validation rejections including the query-side zero-norm segment law. +// The path-equality contract, swept across the widths that can break it. +// +// TestSegmentedScan's A1 already asserts "degenerate one-segment query == whole-row +// scan, bit-identical" — but only at dims 32, where the float32 stride is a multiple of +// 8. That is precisely where the two paths cannot disagree, because the float32 kernel +// dispatchers select AVX2 only at `length % 8 == 0` and both paths then choose it. The +// mechanism was covered; the width was not, and 12/32/64/128/256/768 all miss it. +// +// Float32 strides are multiples of 4, so dims 20, 36, 52, 100 and 132 produce strides +// that are NOT multiples of 8. At those widths a second selection rule anywhere in the +// scan shows up as a last-ulp disagreement between the two paths — which is what +// DETERMINISM.md's "no second code path exists to drift" forbids, and what the plugin's +// "contributions sum EXACTLY to the hit's score" is built on. +// +// This is the cross-PATH check. T11's sweep compares each kernel against its own scalar +// mirror, which stays green while the two paths pick different kernels, because each is +// individually correct. Comparing a path to a mirror is not comparing a path to a path. +static void TestSegmentedEqualsWholeRowAcrossWidths() +{ + Rng rng(0x9D71A55ull); + const int32_t count = 120; + const int32_t k = 8; + // Chosen so the float32 stride is NOT a multiple of 8 (20, 36, 52, 100, 132) with + // multiple-of-8 controls (32, 64) beside them. + const int32_t dimsSet[] = {20, 32, 36, 52, 64, 100, 132}; + + for (int32_t dims : dimsSet) + { + for (Quantization quant : {Quantization::Float32, Quantization::Int8}) + { + if (dims % (kAlignment / ElementSize(quant)) != 0) + { + continue; // not on this quantization's element grid; not a legal segment + } + for (Metric metric : {Metric::Dot, Metric::Cosine, Metric::L2}) + { + TestBank bank(rng, count, dims, quant, metric); + const int32_t pd = bank.view.paddedDims; + AlignedBuf q(static_cast(pd) * sizeof(float)); + std::vector qv(static_cast(dims)); + for (auto& x : qv) + { + x = rng.NextFloat(); + } + PadQuery(qv, pd, q.F32()); + + QueryParams plain; + plain.k = k; + Workspace ws1; + std::vector whole(static_cast(k)); + int32_t wholeCount = 0; + CHECK(Query(bank.view, q.F32(), plain, ws1, whole.data(), &wholeCount) == + Status::Ok); + + const QuerySegment degenerate[1] = {{0, pd, 1.0f}}; + QueryParams seg = plain; + seg.segments = degenerate; + seg.segmentCount = 1; + Workspace ws2; + std::vector segmented(static_cast(k)); + int32_t segCount = 0; + CHECK(Query(bank.view, q.F32(), seg, ws2, segmented.data(), &segCount) == + Status::Ok); + + CHECK_MSG(segCount == wholeCount, + "dims=%d pd=%d quant=%d metric=%d: hit counts differ (%d vs %d)", + dims, pd, static_cast(quant), static_cast(metric), + wholeCount, segCount); + for (int32_t i = 0; i < wholeCount && i < segCount; ++i) + { + CHECK_MSG(segmented[i].index == whole[i].index && + segmented[i].score == whole[i].score, + "dims=%d pd=%d quant=%d metric=%d hit %d: whole-row (%d, %.9g) vs " + "one-segment (%d, %.9g) — the two scan paths disagree", + dims, pd, static_cast(quant), static_cast(metric), i, + whole[i].index, static_cast(whole[i].score), + segmented[i].index, static_cast(segmented[i].score)); + } + + // The decomposition surface rides the same kernel table, and its published + // claim is that a contribution IS the score. With one whole-row segment + // there is exactly one contribution and no summation order to argue about, + // so this is the claim stated at its sharpest. + if (wholeCount > 0) + { + float contribution = 0.0f; + const float total = DecomposeRowScore(bank.view, q.F32(), + whole[0].index, degenerate, 1, &contribution); + CHECK_MSG(total == whole[0].score, + "dims=%d pd=%d quant=%d metric=%d: decomposed total %.9g != scan " + "score %.9g for row %d", + dims, pd, static_cast(quant), static_cast(metric), + static_cast(total), + static_cast(whole[0].score), whole[0].index); + CHECK(contribution == total); + } + } + } + } +} + static void TestSegmentedScan() { Rng rng(0x5E63E17ull); @@ -19522,6 +19627,7 @@ int main() TestIntersect(); TestPca(); TestSegmentedScan(); + TestSegmentedEqualsWholeRowAcrossWidths(); TestPerChannelCosine(); TestScratchBanks(); TestScratchChannelCreateRejections(); From 958c50aefb0fcccc324e8c62d3c597c6b2495574 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 15:27:41 -0700 Subject: [PATCH 05/14] Core review follow-ups: bounds, precision, decode consistency, and a self-dot hoist Addresses the core half of a whole-project review's lower-severity findings. graph.cpp (F5): MutualFilter and ConnectedComponents bound caller-supplied neighbour and duplicate-group values to [0, count) before using them as indices into the neighbour list and the union-find scratch. -1 stays the "no neighbour" sentinel; any other out-of-range value degrades to a dropped edge rather than an out-of-bounds read or write. Reachable only through a hand-built neighbour list, but that is the trust posture the other core entry points already hold. analytics.cpp (F8): the int8 off-grid channel guard that novelty.cpp applies before detail::DotI8I8 was missing on the channel legs here. Added once in ChannelSubRange, the single point all three legs resolve the range through, so an off-grid hand-built BankView is refused rather than reading SIMD-padded slop. pca.cpp (F11, F12): the covariance-apply accumulator is now double, matching the mean's precision (a float accumulator rounded once per row and degraded the power iteration with bank size); the scratch buffer is double accordingly. ProjectRowsOntoComponents now bounds componentCount by dims like its sibling, so an over-large count is refused rather than reading past the components buffer. Scale decode (F9): compose.cpp, pca.cpp, and analytics.cpp's projection report now decode bank.scales[] through detail::FloatBitsToDouble, matching every other scale read, so a subnormal scale decodes identically regardless of the thread's FTZ/DAZ mode. analytics.cpp (F15): the channel NN divergence pre-lifts the target sub-rows once into the Workspace's XdQuery scratch, so each target's self-dot is computed once rather than once per source row. novelty.cpp (F14): a comment at CalibrateNoveltyBaseline documents that sampleLimit is a ceiling, not a down-sampler, matching the header. Suite: 85,470 checks, 0 failures. The two memory-safety guards gained tests: F8's is a clean status oracle (confirmed to fail on all four channel legs with the guard removed); F5's asserts the guarded result on poisoned input and defers the unguarded-access crash to the ASan CI job, since OOB in a normal build is UB that does not reliably fault. Co-Authored-By: Claude Opus 4.8 --- docs/API.md | 2 +- include/superfaiss/pca.h | 14 +++-- src/analytics.cpp | 81 ++++++++++++++++++++------- src/compose.cpp | 4 +- src/graph.cpp | 22 ++++++-- src/novelty.cpp | 5 ++ src/pca.cpp | 25 ++++++--- tests/test_main.cpp | 115 ++++++++++++++++++++++++++++++++++++++- 8 files changed, 225 insertions(+), 43 deletions(-) diff --git a/docs/API.md b/docs/API.md index c512cf5..5e98733 100644 --- a/docs/API.md +++ b/docs/API.md @@ -422,7 +422,7 @@ Determinism per channel: [DETERMINISM.md §2e](DETERMINISM.md). ```cpp Status ComputePrincipalComponents(const BankView&, int32_t componentCount, int32_t iterationsPerComponent, float* outMean, - float* outComponents, float* scratch); + float* outComponents, double* scratch); // dims doubles Status ProjectRowsOntoComponents (const BankView&, const float* mean, const float* components, int32_t componentCount, float* outCoords); diff --git a/include/superfaiss/pca.h b/include/superfaiss/pca.h index 9ab8386..9d482c2 100644 --- a/include/superfaiss/pca.h +++ b/include/superfaiss/pca.h @@ -15,20 +15,24 @@ namespace superfaiss // Buffers (caller-provided): // outMean dims floats — the bank mean (int8 rows dequantized). // outComponents componentCount x dims floats — unit-norm, mutually orthogonal. -// scratch dims floats. -// A degenerate direction (zero variance after deflation) yields a zero component -// vector rather than an error; callers render fewer axes. +// scratch dims doubles — the covariance-apply accumulator. Double so it +// matches the mean's accumulation precision; a float accumulator +// rounds once per row and degrades the power iteration with bank size. +// componentCount must be in [1, dims]. A degenerate direction (zero variance after +// deflation) yields a zero component vector rather than an error; callers render fewer axes. Status ComputePrincipalComponents( const BankView& bank, int32_t componentCount, int32_t iterationsPerComponent, float* outMean, float* outComponents, - float* scratch); + double* scratch); // Projects every row onto the components: outCoords is count x componentCount, // row-major — coordinate j of row r is (x_r - mean) . component_j. Rows are -// dequantized exactly as the kernels see them. +// dequantized exactly as the kernels see them. componentCount must be in [1, dims]: +// each of the componentCount vectors in `components` is a dims-length row, so an +// over-large count would read past the buffer (matches ComputePrincipalComponents). Status ProjectRowsOntoComponents( const BankView& bank, const float* mean, diff --git a/src/analytics.cpp b/src/analytics.cpp index 95159a5..6b34656 100644 --- a/src/analytics.cpp +++ b/src/analytics.cpp @@ -398,8 +398,25 @@ inline bool ChannelSubRange(const BankView& bank, int32_t channel, int32_t* outO { return false; } - *outOffset = bank.channels[channel].offset; - *outLength = bank.channels[channel].length; + const int32_t offset = bank.channels[channel].offset; + const int32_t length = bank.channels[channel].length; + // Every channel leg below scores this sub-range through detail::DotI8I8, whose SIMD + // paths assume a length on the int8 grid (kAlignment / ElementSize(Int8) == 16) and + // carry no scalar remainder tail — an off-grid range would read SIMD-padded slop. A + // validated bank's channel table is grid-aligned by construction (validate.cpp), so + // this never fires on production data; it closes the gap for a hand-built BankView, + // exactly as novelty.cpp does for its own DotI8I8 channel path. Resolved here, at the + // single point all three legs read the range, rather than copied into each. + if (bank.quant == Quantization::Int8) + { + const int32_t grid = kAlignment / ElementSize(Quantization::Int8); + if (offset % grid != 0 || length % grid != 0) + { + return false; + } + } + *outOffset = offset; + *outLength = length; return true; } @@ -453,12 +470,12 @@ Status MakeChannelCentroid(const BankView& bank, const int32_t* rowIndices, int3 Status NNDivergenceChannel( const BankView& source, const uint32_t* sourceExcludeBits, const BankView& target, const uint32_t* targetExcludeBits, int32_t channel, Reduce reduce, - XdQuery* queryScratch, float* outValue) + XdQuery* queryScratch, Workspace& ws, float* outValue) { - // Unlike the whole-vector NNDivergence (which routes through QueryXdBatch), the channel - // path scores each nearest in place, so it needs only the source-lift buffer - // (queryScratch) — the Hit/count/Workspace scratch the public signature carries for - // parity with the whole-vector twin is unused here and not required non-null. + // The channel path scores each nearest in place rather than through QueryXdBatch, so it + // needs the source-lift buffer (queryScratch) plus the Workspace, which stages the + // target lift (below). The Hit/count scratch the public signature carries for parity + // with the whole-vector twin is unused here. if (outValue == nullptr || queryScratch == nullptr || !IsInt8CrossDevice(source) || !IsInt8CrossDevice(target) || source.paddedDims != target.paddedDims) @@ -520,6 +537,31 @@ Status NNDivergenceChannel( // ZeroNormQuery rejection is for a SINGLE per-channel query, not a reduction over a pool // (a valid row can have a whole-row-normalized image with one channel exactly zero). + // Pre-lift the non-excluded target sub-rows ONCE: each target's self-dot + // (DotI8I8(timg, timg, length)) is independent of the source, so recomputing it inside + // the inner loop scored it m times per target. Stage the target queries in the + // Workspace's XdQuery scratch (warm-reusable, zero steady-state allocation) so the + // inner loop reads a prebuilt query. The int8 image pointers alias the target bank + // directly — targets are not requantized — so only scale and self-dot are computed. + if (!ws.ReserveXdQuery(target.paddedDims, target.count)) + { + return Status::OutOfMemory; + } + XdQuery* targetQueries = ws.XdSlots(); + int32_t tCount = 0; + for (int32_t r = 0; r < target.count; ++r) + { + if (IsExcluded(targetExcludeBits, r)) + { + continue; + } + const int8_t* timg = tgtRows + static_cast(r) * tpd + tOff; + targetQueries[tCount].q8 = timg; + targetQueries[tCount].scale = detail::FloatBitsToDouble(target.scales[r]); + targetQueries[tCount].sqSum = detail::DotI8I8(timg, timg, length); + ++tCount; + } + double acc = 0.0; double best = 0.0; bool have = false; @@ -529,19 +571,13 @@ Status NNDivergenceChannel( const XdQuery& q = queryScratch[i]; double nn = 0.0; bool haveNn = false; - for (int32_t r = 0; r < target.count; ++r) + for (int32_t t = 0; t < tCount; ++t) { - if (IsExcluded(targetExcludeBits, r)) - { - continue; - } - const int8_t* timg = tgtRows + static_cast(r) * tpd + tOff; - const XdQuery tq{timg, detail::FloatBitsToDouble(target.scales[r]), - detail::DotI8I8(timg, timg, length)}; // A zero sub-norm target is NOT skipped: XdChannelPairScore floors a degenerate // Cosine member to a defined 0 (C-5), so a zero-energy target scores distance 0 // and is the nearest -- consistent with Centroid/Spread and never NaN. - const double d = static_cast(XdChannelPairScore(q, tq, length, metric)); + const double d = + static_cast(XdChannelPairScore(q, targetQueries[t], length, metric)); const bool better = !haveNn || (metric == Metric::Dot ? d > nn : d < nn); if (better) { @@ -621,21 +657,21 @@ Status CentroidDistanceCrossDeviceChannel( Status MeanNNCrossDeviceChannel( const BankView& source, const uint32_t* sourceExcludeBits, const BankView& target, const uint32_t* targetExcludeBits, int32_t channel, - XdQuery* queryScratch, Hit*, int32_t*, Workspace&, + XdQuery* queryScratch, Hit*, int32_t*, Workspace& ws, float* outValue) { return NNDivergenceChannel(source, sourceExcludeBits, target, targetExcludeBits, channel, - Reduce::Mean, queryScratch, outValue); + Reduce::Mean, queryScratch, ws, outValue); } Status MaxNNCrossDeviceChannel( const BankView& source, const uint32_t* sourceExcludeBits, const BankView& target, const uint32_t* targetExcludeBits, int32_t channel, - XdQuery* queryScratch, Hit*, int32_t*, Workspace&, + XdQuery* queryScratch, Hit*, int32_t*, Workspace& ws, float* outValue) { return NNDivergenceChannel(source, sourceExcludeBits, target, targetExcludeBits, channel, - Reduce::Max, queryScratch, outValue); + Reduce::Max, queryScratch, ws, outValue); } Status SpreadCrossDeviceChannel( @@ -765,7 +801,10 @@ Status ProjectionReport(const BankView& bank, const float* paddedDirection, { acc += static_cast(row[i]) * static_cast(paddedDirection[i]); } - acc *= static_cast(bank.scales[r]); + // Decode the stored scale through the DAZ-safe path, as every other scale read + // in this file does, so a subnormal scale decodes identically whether or not + // the calling thread has denormals-are-zero enabled. + acc *= detail::FloatBitsToDouble(bank.scales[r]); } else { diff --git a/src/compose.cpp b/src/compose.cpp index f620e68..7e572df 100644 --- a/src/compose.cpp +++ b/src/compose.cpp @@ -89,7 +89,9 @@ Status MakeCentroid( { const int8_t* r = static_cast(bank.rows) + static_cast(row) * bank.paddedDims; - const double scale = bank.scales[row]; + // DAZ-safe scale decode, consistent with the other scale reads in this file + // (a subnormal scale decodes the same regardless of the thread's FTZ/DAZ mode). + const double scale = detail::FloatBitsToDouble(bank.scales[row]); for (int32_t j = 0; j < width; ++j) { acc[j] += static_cast(r[base + j]) * scale; diff --git a/src/graph.cpp b/src/graph.cpp index f313714..7cbbc55 100644 --- a/src/graph.cpp +++ b/src/graph.cpp @@ -168,7 +168,12 @@ Status MutualFilter(int32_t count, int32_t k, const int32_t* neighbors, uint8_t* { const int64_t slot = static_cast(i) * k + t; const int32_t j = neighbors[slot]; - outMutualFlags[slot] = (j >= 0 && inList(j, i)) ? uint8_t{1} : uint8_t{0}; + // `j` is a caller-supplied neighbour value and is used to index the + // `neighbors` array (inList reads neighbors + j*k). Bound it to [0, count): + // -1 is the "no neighbour" sentinel, and any other out-of-range value is a + // malformed input that must degrade to "no edge", never an out-of-bounds read. + outMutualFlags[slot] = + (j >= 0 && j < count && inList(j, i)) ? uint8_t{1} : uint8_t{0}; } } return Status::Ok; @@ -285,9 +290,15 @@ Status ConnectedComponents( { for (int32_t r = 0; r < count; ++r) { - if (duplicateGroups[r] != r) + // A group representative indexes the union-find scratch through unite/find. + // Bound it to [0, count): a well-formed table from BuildDuplicateGroups always + // holds an in-range representative, but this entry point takes the table from + // the caller, and an out-of-range value must be ignored rather than walked into + // the parent array as an out-of-bounds access. + const int32_t g = duplicateGroups[r]; + if (g >= 0 && g < count && g != r) { - unite(r, duplicateGroups[r]); + unite(r, g); } } } @@ -302,7 +313,10 @@ Status ConnectedComponents( if (mutualFlags[slot]) { const int32_t j = neighbors[slot]; - if (j >= 0) + // unite(i, j) walks parent[j]; bound j to [0, count) so a malformed + // neighbour value is a dropped edge, not an out-of-bounds write into the + // union-find scratch. -1 is the standing "no neighbour" sentinel. + if (j >= 0 && j < count) { unite(i, j); } diff --git a/src/novelty.cpp b/src/novelty.cpp index e828199..ca34ba3 100644 --- a/src/novelty.cpp +++ b/src/novelty.cpp @@ -238,6 +238,11 @@ Status CalibrateNoveltyBaseline( const BankView& bank, int32_t k, int32_t sampleLimit, float* outSortedDistances, int32_t* outCount, Workspace& workspace) { + // `sampleLimit` is a CEILING the bank must fit under, not a cap this function samples + // down to: `bank.count > sampleLimit` is a defined rejection (the header states this). + // A caller wanting a baseline over a subset pre-samples into a smaller BankView and + // passes that — which is exactly what the inspector does — rather than expecting this + // to draw a sample. Named `sampleLimit` because it bounds the sample the CALLER built. if (bank.metric == Metric::Dot || k < 1 || bank.count < 1 || k >= bank.count || sampleLimit < 1 || bank.count > sampleLimit || bank.rows == nullptr || outSortedDistances == nullptr || outCount == nullptr) { diff --git a/src/pca.cpp b/src/pca.cpp index 625677b..a552226 100644 --- a/src/pca.cpp +++ b/src/pca.cpp @@ -1,5 +1,7 @@ #include "superfaiss/pca.h" +#include "superfaiss/kernels.h" // detail::FloatBitsToDouble (DAZ-safe scale decode) + #include #include @@ -15,7 +17,9 @@ namespace { const int8_t* row = static_cast(bank.rows) + static_cast(r) * bank.paddedDims; - return static_cast(row[j]) * bank.scales[r]; + // DAZ-safe scale decode (a subnormal scale decodes identically under FTZ/DAZ), + // matching the reductions in analytics.cpp/compose.cpp over the same field. + return static_cast(row[j]) * detail::FloatBitsToDouble(bank.scales[r]); } const float* row = static_cast(bank.rows) + static_cast(r) * bank.paddedDims; @@ -29,7 +33,7 @@ Status ComputePrincipalComponents( int32_t iterationsPerComponent, float* outMean, float* outComponents, - float* scratch) + double* scratch) { if (outMean == nullptr || outComponents == nullptr || scratch == nullptr || componentCount <= 0 || componentCount > bank.dims || @@ -80,10 +84,12 @@ Status ComputePrincipalComponents( } } - // scratch <- Cov * v, serial in row order. + // scratch <- Cov * v, serial in row order, accumulated in DOUBLE so the + // operator is applied at the same precision as the mean above (a float + // accumulator rounds once per row and loses accuracy as count grows). for (int32_t j = 0; j < dims; ++j) { - scratch[j] = 0.0f; + scratch[j] = 0.0; } for (int32_t r = 0; r < count; ++r) { @@ -94,15 +100,14 @@ Status ComputePrincipalComponents( } for (int32_t j = 0; j < dims; ++j) { - scratch[j] = static_cast( - scratch[j] + dot * (RowElem(bank, r, j) - outMean[j])); + scratch[j] += dot * (RowElem(bank, r, j) - outMean[j]); } } double norm = 0.0; for (int32_t j = 0; j < dims; ++j) { - norm += static_cast(scratch[j]) * scratch[j]; + norm += scratch[j] * scratch[j]; } if (norm == 0.0) { @@ -160,8 +165,12 @@ Status ProjectRowsOntoComponents( float* outCoords) { if (mean == nullptr || components == nullptr || outCoords == nullptr || - componentCount <= 0) + componentCount <= 0 || componentCount > bank.dims) { + // componentCount is bounded by dims exactly as ComputePrincipalComponents bounds + // it: `components` holds componentCount rows of `dims` each, and an over-large + // count would index past the buffer. The two functions are called in sequence and + // must agree on the argument. return Status::InvalidArgument; } const int32_t dims = bank.dims; diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 181a2d5..cd08f17 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -2031,7 +2031,7 @@ static void TestPca() std::vector mean(static_cast(dims)); std::vector comps(static_cast(2) * dims); - std::vector scratch(static_cast(dims)); + std::vector scratch(static_cast(dims)); CHECK(ComputePrincipalComponents(view, 2, 48, mean.data(), comps.data(), scratch.data()) == Status::Ok); @@ -2066,6 +2066,12 @@ static void TestPca() std::vector coords(static_cast(count) * 2); CHECK(ProjectRowsOntoComponents(view, mean.data(), comps.data(), 2, coords.data()) == Status::Ok); + // componentCount is bounded by dims on BOTH entry points (a count above dims would + // index past `components`), the same rejection ComputePrincipalComponents applies. + CHECK(ProjectRowsOntoComponents(view, mean.data(), comps.data(), dims + 1, + coords.data()) == Status::InvalidArgument); + CHECK(ProjectRowsOntoComponents(view, mean.data(), comps.data(), 0, + coords.data()) == Status::InvalidArgument); { const int32_t r = 7; double expect = 0.0; @@ -2109,7 +2115,7 @@ static void TestPca() view.metric = Metric::Dot; std::vector mean(static_cast(d)); std::vector comps(static_cast(d)); - std::vector scratch(static_cast(d)); + std::vector scratch(static_cast(d)); CHECK(ComputePrincipalComponents(view, 1, 16, mean.data(), comps.data(), scratch.data()) == Status::Ok); double norm = 0.0; @@ -15663,6 +15669,107 @@ static void TestM1TrustBoundaries() } } +// A caller-supplied neighbour value is used to index the neighbours array (MutualFilter) +// and the union-find scratch (ConnectedComponents). Values are validated for the -1 +// "no neighbour" sentinel; a value >= count is a malformed input that must degrade to a +// dropped edge, never an out-of-bounds read or write. BuildKnnNeighbors always produces +// in-range values, so this is reachable only through a hand-built neighbour list — the +// same trust posture the other core entry points hold on the indices they are handed. +static void TestGraphOutOfRangeNeighbours() +{ + Rng rng(0x60B5); + const int32_t dims = 8, count = 6, k = 2; + std::vector src; + for (int32_t i = 0; i < count * dims; ++i) src.push_back(rng.NextFloat()); + GBank b(src, count, dims, Quantization::Float32, Metric::L2); + + // A neighbour list holding out-of-range values: count (one past the end), a large + // positive, and INT32_MAX, mixed with the -1 sentinel. Every slot is poisoned; none + // may be dereferenced as an index. + std::vector neighbours(static_cast(count) * k); + for (int32_t i = 0; i < count; ++i) + { + neighbours[static_cast(i) * k + 0] = count; // exactly one past the end + neighbours[static_cast(i) * k + 1] = (i % 2) ? INT32_MAX : -1; + } + + std::vector flags(static_cast(count) * k, 0xEE); + CHECK(MutualFilter(count, k, neighbours.data(), flags.data()) == Status::Ok); + // No out-of-range value can be its own mutual: every flag is 0, and nothing crashed. + for (uint8_t f : flags) + { + CHECK(f == 0); + } + + // Force the union-find to CONFRONT the out-of-range neighbour: hand it a flag array + // with every slot marked mutual, so ConnectedComponents must act on each neighbour + // value, and a duplicate-group table whose every representative is INT32_MAX. With the + // < count guard both are dropped and each row is its own component. Without it, find() + // indexes parent[] with these values — undefined behaviour. This cell asserts the + // GUARDED result holds on poisoned input; it is not a crash reproduction, because an + // out-of-bounds access in a non-instrumented build is UB that neither reliably crashes + // nor changes this observable, so it cannot serve as the guard's oracle here. The + // address-sanitizer job in core CI is where the unguarded access is caught as a fault. + std::vector allMutual(static_cast(count) * k, 1); + std::vector badGroups(static_cast(count)); + for (int32_t i = 0; i < count; ++i) + { + badGroups[static_cast(i)] = INT32_MAX; // every representative wildly out of range + } + std::vector ids(static_cast(count), -999), uf(static_cast(count), 0); + CHECK(ConnectedComponents(count, k, neighbours.data(), allMutual.data(), badGroups.data(), + ids.data(), uf.data()) == Status::Ok); + // Every edge dropped (poisoned neighbours and poisoned groups alike), so each row is + // its own component — a defined, in-range result, no corruption, no crash. + for (int32_t r = 0; r < count; ++r) + { + CHECK(ids[r] == r); + } +} + +// The int8 channel-analytics legs score their sub-range through detail::DotI8I8, whose +// SIMD paths assume a length on the 16-byte int8 grid and carry no scalar tail. A +// validated bank's channel table is grid-aligned, so the archive loader rejects an +// off-grid table before it can be scored (covered elsewhere); this closes the same gap +// for a HAND-BUILT BankView that never went through validation, so the analytics +// functions themselves refuse rather than read SIMD-padded slop. +static void TestChannelAnalyticsOffGridRejected() +{ + Rng rng(0x0FF6); + const int32_t dims = 32, count = 8; // int8 grid is 16; 8 is deliberately off it + std::vector src; + for (int32_t i = 0; i < count * dims; ++i) src.push_back(rng.NextFloat()); + GBank b(src, count, dims, Quantization::Int8, Metric::Cosine); + + // A hand-built channel table with an off-grid range (offset 0, length 8) — a table a + // validated bank could never carry. + const ChannelInfo offGrid[1] = {{0, 8}}; + BankView view = b.view; + view.channels = offGrid; + view.channelCount = 1; + + Workspace ws; + std::vector qbuf(static_cast(count)); + std::vector hbuf(static_cast(count)); + std::vector nbuf(static_cast(count)); + float out = -1.0f; + CHECK(MeanNNCrossDeviceChannel(view, nullptr, view, nullptr, 0, qbuf.data(), + hbuf.data(), nbuf.data(), ws, &out) == Status::InvalidArgument); + CHECK(MaxNNCrossDeviceChannel(view, nullptr, view, nullptr, 0, qbuf.data(), + hbuf.data(), nbuf.data(), ws, &out) == Status::InvalidArgument); + + std::vector cA(static_cast(view.paddedDims)); + std::vector cB(static_cast(view.paddedDims)); + const int32_t idx[1] = {0}; + CHECK(CentroidDistanceCrossDeviceChannel(view, idx, 1, nullptr, nullptr, view, idx, 1, + nullptr, nullptr, Metric::Cosine, 0, cA.data(), cB.data(), &out) == + Status::InvalidArgument); + + std::vector cScratch(static_cast(view.paddedDims)); + CHECK(SpreadCrossDeviceChannel(view, idx, 1, nullptr, Reduce::Mean, 0, cScratch.data(), + &out) == Status::InvalidArgument); +} + // M1 / dim 2 (byte-confirm) + dim 7 (int8 same-decode-different-scale NOT unioned) — // grouping is FULL BYTE equality, not decode equality: a scalar-multiple pair that // decodes to the same direction but stores different int8 bytes/scale is a near- @@ -19089,7 +19196,7 @@ static void TestAllocFlatBakeAndPca() const int32_t componentCount = 2, iterations = 4; std::vector mean(static_cast(cosineBank.view.dims)); std::vector components(static_cast(componentCount) * cosineBank.view.dims); - std::vector scratch(static_cast(cosineBank.view.dims)); + std::vector scratch(static_cast(cosineBank.view.dims)); std::vector coords(static_cast(count) * componentCount); auto driveOnce = [&]() { @@ -19737,6 +19844,8 @@ int main() // V3.2 red suite: Bank Inspector I — module M1 graph.h. TestM1TrustBoundaries(); + TestGraphOutOfRangeNeighbours(); + TestChannelAnalyticsOffGridRejected(); TestM1DuplicateGroupingByteConfirm(); TestM1EdgesExactAcrossMetrics(); TestM1DuplicateUnionConstructionCounts(); From 8842f34e2097501b363f007d41fd19f102d6f5a6 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 15:41:24 -0700 Subject: [PATCH 06/14] CHANGELOG: note the bank-inspection review follow-ups in 3.2.2 Documents the graph/analytics/pca/decode fixes committed in 958c50a under the 3.2.2 entry (they were made after that commit's own message was written). Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1727316..903f8cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,19 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu `kMaxCrossDeviceDims` dims) before any size is computed, returning `InvalidArgument`. The archive loader already applied these caps; direct construction did not, and `Grow` bounded its request only against the current capacity. +- **Bank-inspection review follow-ups (lower-severity, from a whole-project pass).** + `graph.cpp`: `MutualFilter`/`ConnectedComponents` bound caller-supplied neighbour and + duplicate-group values to `[0, count)` before indexing the neighbour list and the + union-find scratch, so a malformed hand-built input degrades to a dropped edge rather + than an out-of-bounds access. `analytics.cpp`: the int8 off-grid channel guard from + `novelty.cpp` now also protects the channel-analytics legs (added once in the shared + `ChannelSubRange`), and the channel NN divergence pre-lifts each target's self-dot once + instead of once per source row. `pca.cpp`: the covariance-apply accumulator (and its + scratch buffer) is now `double` to match the mean's precision, and + `ProjectRowsOntoComponents` bounds `componentCount` by `dims` like its sibling. Scale + decode goes through `detail::FloatBitsToDouble` on the remaining `compose.cpp`/`pca.cpp`/ + `analytics.cpp` paths, so a subnormal scale decodes identically under any FTZ/DAZ mode. + A comment documents that `novelty.cpp`'s `sampleLimit` is a ceiling, not a down-sampler. - **A Cosine channel bank with zero rows was unrepresentable.** `ValidateBank` required the per-channel inverse sub-norm array whenever a Cosine bank carried channels, but those norms are one per row — a bank with no rows requires none, and no scan reads the From 878a8dcef88ad43e21a7f1f7693833ebaf25e171 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 16:22:44 -0700 Subject: [PATCH 07/14] Scratch: Load validates the retention region against the rows it claims to reference A retained row is by construction the post-normalization row the quantizer consumed, so Load replays the bake on each retained row and requires it to reproduce the stored row exactly (allocation-free, mirroring QuantizeRowsInt8/PadRowsFloat32). A fabricated-but-finite retained array previously loaded clean and handed MeasureScratchRecall an invented reference to audit the quantized rows against. Non-finite retained values are rejected the same way. Suite: 85,532 checks, 0 failures. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 7 +++ src/scratch.cpp | 115 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_main.cpp | 67 ++++++++++++++++++++++++++ 3 files changed, 189 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 903f8cd..b3d44bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu trailer with the rows already replaced. ### Fixed +- **`ScratchBank::Load` now validates the retention region.** A retained row is by + construction the post-normalization row the quantizer consumed, so `Load` replays the + bake on each retained row and requires it to reproduce the stored row exactly — + allocation-free, mirroring the quantizer's own arithmetic. Previously a + fabricated-but-finite retained array loaded clean and handed `MeasureScratchRecall` an + invented reference to audit against, through the one API whose whole job is an honest + number. Non-finite retained values are rejected the same way (`BadFormat`). - **Two kernel-selection rules made the segmented scan disagree with the whole-row scan.** The float32 dispatchers `DotF32`/`L2F32` use AVX2+FMA only when the length they are handed is a multiple of 8; `ResolveRowKernels`, which feeds the segmented scan and diff --git a/src/scratch.cpp b/src/scratch.cpp index ed7e1f1..2f5a319 100644 --- a/src/scratch.cpp +++ b/src/scratch.cpp @@ -144,6 +144,109 @@ namespace }; static_assert(sizeof(ScratchHeader) == 32, "scratch header layout is the format"); + // Validates a loaded retention region against the rows it claims to be the reference + // for. The retained row is BY CONSTRUCTION the post-normalization row the quantizer + // consumed (see AppendValidated), so replaying the bake on it must reproduce the stored + // row exactly — same inputs, same deterministic arithmetic, same bytes. That makes the + // retention region DERIVED-CHECKED rather than trusted: a fabricated-but-finite array + // would otherwise load clean and hand MeasureScratchRecall an invented reference to + // audit against, through the one API whose entire job is an honest number. + // + // Replays the quantizer inline rather than into a scratch buffer, so it needs no + // allocation; the arithmetic mirrors QuantizeRowsInt8/PadRowsFloat32 exactly. + bool RetainedMatchesRows(const ScratchBank& bank, const float* retained, const void* rows, + const float* scales, int32_t count, int32_t dims, int32_t paddedDims, Quantization quant) + { + (void)bank; + for (int32_t r = 0; r < count; ++r) + { + const float* src = retained + static_cast(r) * dims; + for (int32_t i = 0; i < dims; ++i) + { + if (!std::isfinite(src[i])) + { + return false; + } + } + if (quant == Quantization::Float32) + { + const float* dst = static_cast(rows) + + static_cast(r) * paddedDims; + for (int32_t i = 0; i < dims; ++i) + { + if (dst[i] != src[i]) + { + return false; + } + } + for (int32_t i = dims; i < paddedDims; ++i) + { + if (dst[i] != 0.0f) + { + return false; + } + } + continue; + } + + const int8_t* dst = static_cast(rows) + + static_cast(r) * paddedDims; + float maxAbs = 0.0f; + for (int32_t i = 0; i < dims; ++i) + { + const float a = std::fabs(src[i]); + if (a > maxAbs) + { + maxAbs = a; + } + } + if (maxAbs == 0.0f) + { + if (scales[r] != 0.0f) + { + return false; + } + for (int32_t i = 0; i < paddedDims; ++i) + { + if (dst[i] != 0) + { + return false; + } + } + continue; + } + if (scales[r] != maxAbs / 127.0f) + { + return false; + } + const float inv = 127.0f / maxAbs; + for (int32_t i = 0; i < dims; ++i) + { + float q = std::nearbyint(src[i] * inv); + if (q > 127.0f) + { + q = 127.0f; + } + if (q < -127.0f) + { + q = -127.0f; + } + if (dst[i] != static_cast(q)) + { + return false; + } + } + for (int32_t i = dims; i < paddedDims; ++i) + { + if (dst[i] != 0) + { + return false; + } + } + } + return true; + } + // The header rules, shared by Load and PeekScratchArchive so the two can never // disagree about what a valid archive header is — one validator, one truth. Pure on // the header bytes: it allocates nothing and reads no payload. @@ -1392,6 +1495,18 @@ Status ScratchBank::Load(const ScratchArchive& archive, const Allocator& allocat return content; } + // The retention region is validated too, against the rows just proven good: replaying + // the bake on each retained row must reproduce the stored row byte for byte. Without + // this the archive's "not trusted" claim held for the rows and not for the audit + // reference beside them, so a fabricated retained array loaded clean and + // MeasureScratchRecall reported a number derived from it. + if (wantRetain && header.count > 0 && + !RetainedMatchesRows(incoming, incoming.Retained_, incoming.Rows_, incoming.Scales_, + header.count, header.dims, header.paddedDims, incoming.Quant_)) + { + return Status::BadFormat; + } + // Re-derive the per-channel sub-norm arena on Load: recompute it from the // loaded rows + channel table rather than trust a serialized copy, so a desynced arena // cannot load as authoritative. The channel Create above sized the region into diff --git a/tests/test_main.cpp b/tests/test_main.cpp index cd08f17..833cc5d 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -2928,6 +2928,72 @@ static void TestEmptyCosineChannelBankValidates() } } +// The retention region is part of what "the archive is not trusted" covers. A retained +// row is by construction the post-normalization row the quantizer consumed, so replaying +// the bake on it must reproduce the stored row exactly; a fabricated-but-finite retained +// array would otherwise load clean and hand MeasureScratchRecall an invented reference to +// audit the quantized rows against — through the one API whose whole job is honest. +static void TestScratchLoadValidatesRetainedRegion() +{ + Rng rng(0x2E7A15ull); + const int32_t dims = 32, count = 24; + + for (Quantization quant : {Quantization::Float32, Quantization::Int8}) + { + ScratchBank bank; + CHECK(bank.Create(count, dims, Metric::Cosine, quant, /*retainFloats=*/true) == + Status::Ok); + std::vector row(static_cast(dims)); + for (int32_t r = 0; r < count; ++r) + { + for (auto& v : row) + { + v = rng.NextFloat(); + } + CHECK(bank.Append(row.data(), dims, nullptr) == Status::Ok); + } + MemArchive good; + CHECK(bank.Save(good.Writer()) == Status::Ok); + + // The clean blob loads, so the corruptions below fail for their own reason. + { + MemArchive a; + a.bytes = good.bytes; + ScratchBank target; + CHECK(target.Load(a.Reader()) == Status::Ok); + CHECK(target.RetainsFloats()); + } + + // The retained region is the LAST thing Save writes, so the trailing + // count * dims floats are it. Perturb one value: still finite, still plausible, + // but no longer the row the quantizer consumed. + const size_t retainedBytes = + static_cast(count) * dims * sizeof(float); + CHECK(good.bytes.size() > retainedBytes); + { + MemArchive a; + a.bytes = good.bytes; + float* retained = reinterpret_cast(a.bytes.data() + + (a.bytes.size() - retainedBytes)); + retained[0] = retained[0] + 0.25f; // finite, wrong + ScratchBank target; + CHECK_MSG(target.Load(a.Reader()) == Status::BadFormat, + "a fabricated retained row must be rejected (quant %d)", + static_cast(quant)); + } + // A non-finite retained value is rejected too. + { + MemArchive a; + a.bytes = good.bytes; + float* retained = reinterpret_cast(a.bytes.data() + + (a.bytes.size() - retainedBytes)); + retained[3] = std::numeric_limits::quiet_NaN(); + ScratchBank target; + CHECK(target.Load(a.Reader()) == Status::BadFormat); + } + } +} + // PeekScratchArchive reports a serialized archive's geometry and — the reason it exists — // the exact byte length a Load consumes, so a host that appends its own trailer can find // and validate that trailer BEFORE committing the load. @@ -19880,6 +19946,7 @@ int main() // V3.2.2: public-path geometry ceilings, and the archive peek the host trailer // validation is built on. TestScratchGeometryCeilings(); + TestScratchLoadValidatesRetainedRegion(); TestEmptyCosineChannelBankValidates(); TestPeekScratchArchive(); From 135865cb89aa8a9c03609231c9ce11dd0dd2c2af Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 16:54:09 -0700 Subject: [PATCH 08/14] version.h: bump to 3.3.0 PeekScratchArchive and ScratchArchiveInfo are a new public capability, so the release advances the minor version per this project's stated policy (minor = new capability, patch = fix). --- include/superfaiss/version.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/superfaiss/version.h b/include/superfaiss/version.h index 2ea7a0a..0f25cdf 100644 --- a/include/superfaiss/version.h +++ b/include/superfaiss/version.h @@ -3,8 +3,8 @@ #include #define SUPERFAISS_VERSION_MAJOR 3 -#define SUPERFAISS_VERSION_MINOR 2 -#define SUPERFAISS_VERSION_PATCH 2 +#define SUPERFAISS_VERSION_MINOR 3 +#define SUPERFAISS_VERSION_PATCH 0 namespace superfaiss { From 53aea75ca30bb4ece56a49a091e232be9727ccb2 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 16:54:19 -0700 Subject: [PATCH 09/14] CHANGELOG: retitle release 3.3.0, add v3.0.1/v3.1.1, reclassify a source break Rename the [3.2.2] heading to [3.3.0]: PeekScratchArchive/ScratchArchiveInfo are a new public capability, and this project's own stated policy is minor = new capability, patch = fix. Move ComputePrincipalComponents' scratch parameter type change (float* to double*) out of the "lower-severity" Fixed bullet and into a new Changed section, stated plainly as source-breaking: every caller must retype the parameter and double the buffer it allocates. Add a Changed note that MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannel can now return OutOfMemory, a status neither could previously return, since both now stage a Workspace pre-lift of the target's rows. Add the two tagged releases the changelog was missing entries for (v3.0.1, v3.1.1), derived from git history, restoring the "versions follow the git tags" claim the file opens with. v3.1.1's entry states plainly that the AVX2 sub-8 segment-remainder fix changed numeric results for affected widths, so a consumer bisecting a score change across 3.1.0-3.1.2 has something to read. --- CHANGELOG.md | 65 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3d44bd..16b91c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ per entry. Reconstructed from git history 2026-07-12. The format follows [Keep a Changelog](https://keepachangelog.com); this project versions by feature tier (minor = new capability, patch = fix), not strict SemVer of a public ABI. -## [3.2.2] — 2026-07-21 +## [3.3.0] — 2026-07-21 ### Added - **`PeekScratchArchive`** — reads a serialized scratch archive's header and channel @@ -20,6 +20,20 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu validate that trailer *before* committing the load, rather than discovering a broken trailer with the rows already replaced. +### Changed +- **`ComputePrincipalComponents`'s `scratch` parameter is now `double*`** (was `float*`). + The covariance-apply accumulator now matches the mean's accumulation precision — a + float accumulator rounded once per row and degraded the power iteration with bank + size — and the caller-owned scratch buffer changed type to match. This is + source-breaking: every existing caller must change the parameter's declared type from + `float*` to `double*` **and** double the size of the buffer it allocates (`dims` + doubles, not `dims` floats). +- **`MeanNNCrossDeviceChannel`/`MaxNNCrossDeviceChannel` can now return `OutOfMemory`.** + Both now stage a pre-lift of the target's non-excluded sub-rows into the `Workspace` + argument (so each target's self-dot is computed once rather than once per source row, + see the self-dot hoist below); if that reservation fails, the call now returns + `OutOfMemory`, a status neither function could previously return. + ### Fixed - **`ScratchBank::Load` now validates the retention region.** A retained row is by construction the post-normalization row the quantizer consumed, so `Load` replays the @@ -58,12 +72,11 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu than an out-of-bounds access. `analytics.cpp`: the int8 off-grid channel guard from `novelty.cpp` now also protects the channel-analytics legs (added once in the shared `ChannelSubRange`), and the channel NN divergence pre-lifts each target's self-dot once - instead of once per source row. `pca.cpp`: the covariance-apply accumulator (and its - scratch buffer) is now `double` to match the mean's precision, and - `ProjectRowsOntoComponents` bounds `componentCount` by `dims` like its sibling. Scale - decode goes through `detail::FloatBitsToDouble` on the remaining `compose.cpp`/`pca.cpp`/ - `analytics.cpp` paths, so a subnormal scale decodes identically under any FTZ/DAZ mode. - A comment documents that `novelty.cpp`'s `sampleLimit` is a ceiling, not a down-sampler. + instead of once per source row. `pca.cpp`: `ProjectRowsOntoComponents` bounds + `componentCount` by `dims` like its sibling. Scale decode goes through + `detail::FloatBitsToDouble` on the remaining `compose.cpp`/`pca.cpp`/`analytics.cpp` + paths, so a subnormal scale decodes identically under any FTZ/DAZ mode. A comment + documents that `novelty.cpp`'s `sampleLimit` is a ceiling, not a down-sampler. - **A Cosine channel bank with zero rows was unrepresentable.** `ValidateBank` required the per-channel inverse sub-norm array whenever a Cosine bank carried channels, but those norms are one per row — a bank with no rows requires none, and no scan reads the @@ -128,6 +141,27 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu test suite) still carried 3.0.1 through both 3.1 releases; both now read 3.1.2. Found while closing an external review's release-identity finding. +## [3.1.1] — 2026-07-18 + +### Fixed +- **AVX2 float32 dot/L2 dropped a sub-8 segment remainder.** `DotF32Avx2`/`L2F32Avx2` (and + their scalar mirrors) accumulated only in whole 8-lane groups with no remainder; a + float32 segment/channel stride lies on the 4-float (16-byte) grid, so a range whose + length is congruent to 4 mod 8 had its trailing 4 elements dropped entirely — a + length-4 channel scored exactly 0 on AVX2 while SSE/portable/NEON scored it correctly. + **This changed numeric results for affected widths**: `ResolveRowKernels` wires the + AVX2 dispatchers in directly for the segmented and per-channel-cosine scan, so any + named-channel or segmented query over a sub-8-remainder length (e.g. a length-4 + channel) on AVX2 hardware returned a different, wrong score before this fix — a + consumer bisecting a score change across 3.1.0 to 3.1.2 should attribute the delta at + those widths to this fix, not to 3.1.0's `Relabel`. Whole-row scans were unaffected + (`DotF32`/`L2F32` route non-multiple-of-8 `paddedDims` to the SSE path, and + `paddedDims` is always a multiple of 8 for banks large enough to matter). The 4-element + remainder is now added to both the intrinsics and their scalar mirrors, bit-identically. +- CI now requires the AVX2 path on the Linux job (previously accepted SSE or AVX2), so a + runner without AVX2 hardware cannot silently skip the new sub-8 remainder coverage; + Windows keeps the looser SSE-or-AVX2 check as the general x86 path. + ## [3.1.0] — 2026-07-17 ### Added @@ -151,6 +185,23 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu (~5.5× cheaper), ~2.1× on float32; promote/demote adds/frees a `capacity × channelCount × 4` sub-norm arena (~3.0 MB at 100k rows / 8 channels). +## [3.0.1] — 2026-07-13 + +### Fixed +- **`version.h` still read 2.5.0 at the v3.0 tag.** Corrected to 3.0.0, with a coherence + test added so a future release cannot ship the same drift. +- **`MeanNNCrossDeviceChannel`/`MaxNNCrossDeviceChannel` rejected a zero-energy Cosine + source and skipped zero-energy targets**, contradicting the documented "a degenerate + member floors to a defined 0 in a reduction" contract already honored by + `CentroidDistanceCrossDeviceChannel`/`SpreadCrossDeviceChannel`. Both guards are + removed so a degenerate channel member floors to 0 through the shared pair score, + consistent across all four channel operators. +- **`MeasureRecallLockedChannel` aborted the whole per-channel recall audit** when a + sampled row's channel sub-vector was zero-energy — a valid row whose per-channel + self-query is `ZeroNormQuery`. It now excludes that sample, refills the reservoir from + the remaining rows, and reports the sample count actually scored rather than the + pre-set target. + ## [3.0.0] — 2026-07-12 ### Added From fd0222045dbc6108956673e4d0c5c5f01ccfa8f8 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 16:54:26 -0700 Subject: [PATCH 10/14] analytics.h: document the channel NN OutOfMemory return and fix a doubled word MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannel stage a pre-lift of the target's non-excluded sub-rows into the Workspace argument (each target's self-dot computed once rather than once per source row); if that reservation fails they return OutOfMemory, which neither function could return before. Document it alongside the existing InvalidArgument cases. Also fixes a doubled word in the channel-scoped analytics doc comment ("scratch scratch-buffer" -> "scratch-buffer"). --- include/superfaiss/analytics.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/include/superfaiss/analytics.h b/include/superfaiss/analytics.h index 40bd034..7040bb0 100644 --- a/include/superfaiss/analytics.h +++ b/include/superfaiss/analytics.h @@ -109,8 +109,13 @@ Status SpreadCrossDevice( // inherits it once Tier 1 put the channel table on the snapshot. A bank with no channel // table, or `channel` outside [0, channelCount), is InvalidArgument; a zero-sub-norm // channel member floors to a defined 0 in a reduction, while a single -// per-channel query on a zero-norm sub-vector still rejects (ZeroNormQuery). The scratch +// per-channel query on a zero-norm sub-vector still rejects (ZeroNormQuery). The // scratch-buffer sizing matches the whole-vector operators. +// +// MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannel additionally stage a pre-lift of the +// target's non-excluded sub-rows in `ws` (each target's self-dot is computed once, not +// once per source row), so both can return OutOfMemory if that reservation fails, in +// addition to the InvalidArgument cases above. Status CentroidDistanceCrossDeviceChannel( const BankView& bankA, const int32_t* rowIndicesA, int32_t rowCountA, const int32_t* weightsA, const uint32_t* excludeBitsA, From ed1a9d48da9dc951b21d8e81b1399c7d9989ae7c Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 16:54:33 -0700 Subject: [PATCH 11/14] docs/API.md: scrub v3.2.2 markers to v3.3.0, document channel NN OutOfMemory Four inline version markers (ScratchArchiveInfo, PeekScratchArchive, the geometry-ceilings note, the archive-peek note) still read v3.2.2 after the release was retitled 3.3.0; retargeted to match. Note that MeanNNCrossDeviceChannel/MaxNNCrossDeviceChannel can now also return OutOfMemory (v3.3), matching the header doc comment. --- docs/API.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/API.md b/docs/API.md index 5e98733..b8fe014 100644 --- a/docs/API.md +++ b/docs/API.md @@ -413,6 +413,9 @@ payloads carry no per-row sub-norm). Scratch sizing matches the whole-vector operators. A channel outside `[0, channelCount)`, or a bank with no channel table, is `InvalidArgument`; a degenerate zero-sub-norm channel member floors to a defined `0` in a reduction (a single per-channel query on a zero-norm sub-vector is still `ZeroNormQuery`). +`MeanNNCrossDeviceChannel`/`MaxNNCrossDeviceChannel` additionally pre-lift the target's +non-excluded sub-rows into `ws` before scoring, so both can also return `OutOfMemory` if +that reservation fails (v3.3). "This mind's identity is drifting but its appearance is stable" is a `CentroidDistanceCross DeviceChannel` over the identity channel versus the appearance channel across checkpoints. Determinism per channel: [DETERMINISM.md §2e](DETERMINISM.md). @@ -605,13 +608,13 @@ struct ScratchRecallReport { // v2.3 // mathematically valid, statistically uninformative }; struct ScratchArchive { bool(*write)(void*, const void*, size_t); bool(*read)(void*, void*, size_t); void* user; }; -struct ScratchArchiveInfo { // v3.2.2 +struct ScratchArchiveInfo { // v3.3.0 int32_t capacity, count, dims, paddedDims; Metric metric; Quantization quant; bool retainFloats; int32_t channelCount; ChannelInfo channels[kMaxChannels]; // 0 / unused for single-space int64_t archiveBytes; // exactly what a Load consumes }; -Status PeekScratchArchive(const void* data, int64_t bytes, ScratchArchiveInfo* outInfo); // v3.2.2 +Status PeekScratchArchive(const void* data, int64_t bytes, ScratchArchiveInfo* outInfo); // v3.3.0 ``` A snapshot IS a `BankView` — every query entry point works on it unchanged. @@ -627,7 +630,7 @@ allocation: everything lives in the arena from `Create` (retention included — `ArenaBytes` simply grows). Archive format: [FORMAT.md](FORMAT.md) section 3; concurrency guarantees: [DETERMINISM.md](DETERMINISM.md) section 2b. -**Geometry ceilings (v3.2.2):** every `Create` overload and `Grow` bound +**Geometry ceilings (v3.3.0):** every `Create` overload and `Grow` bound `capacity` at `kMaxBankRows` and `dims` at `kMaxCrossDeviceDims` *before* the arena size is computed, returning `InvalidArgument` above either. The arena math multiplies capacity by dims in signed `int64`, so an unbounded `int32` pair is @@ -635,7 +638,7 @@ overflow rather than a refused allocation. These are the immutable format's own caps, already applied by the archive loader — a geometry `Create` refuses is one no archive could have carried. -**Peeking an archive (v3.2.2):** `PeekScratchArchive` reads a serialized +**Peeking an archive (v3.3.0):** `PeekScratchArchive` reads a serialized archive's header (and channel table) out of a contiguous byte span and reports its geometry plus `archiveBytes` — exactly the number of bytes a `Load` will consume — without allocating, reading the payload, or touching a bank. It From 8d2a08fa45fde1d6b166c3f74e1d9ff892eb2d7f Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 16:57:07 -0700 Subject: [PATCH 12/14] CHANGELOG: fix two self-contradictions in the 3.1.1 entry The 3.1.1 entry said ResolveRowKernels "wires the AVX2 dispatchers in directly" at that release; it wired the AVX2 kernels in directly (no dispatcher existed there yet). The 3.3.0 entry documents ResolveRowKernels gaining the dispatchers as its own fix, so the two entries described the same call site inconsistently. Corrected 3.1.1 to say "kernels". Also removes the false claim that paddedDims is always a multiple of 8 for banks large enough to matter: PaddedDims (types.h) grids Float32 at 4 elements (kAlignment 16 / ElementSize 4), and the 3.3.0 entry's own kernel- selection bug is scoped to exactly the non-multiple-of-8 float32 widths (20, 36, 52, 100, 132, ...) this claim said were unreachable. The remaining clause -- the dispatchers route non-multiple-of-8 paddedDims to the SSE path -- is correct and sufficient on its own. --- CHANGELOG.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16b91c9..ad5909a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -150,13 +150,12 @@ by feature tier (minor = new capability, patch = fix), not strict SemVer of a pu length is congruent to 4 mod 8 had its trailing 4 elements dropped entirely — a length-4 channel scored exactly 0 on AVX2 while SSE/portable/NEON scored it correctly. **This changed numeric results for affected widths**: `ResolveRowKernels` wires the - AVX2 dispatchers in directly for the segmented and per-channel-cosine scan, so any + AVX2 kernels in directly for the segmented and per-channel-cosine scan, so any named-channel or segmented query over a sub-8-remainder length (e.g. a length-4 channel) on AVX2 hardware returned a different, wrong score before this fix — a consumer bisecting a score change across 3.1.0 to 3.1.2 should attribute the delta at those widths to this fix, not to 3.1.0's `Relabel`. Whole-row scans were unaffected - (`DotF32`/`L2F32` route non-multiple-of-8 `paddedDims` to the SSE path, and - `paddedDims` is always a multiple of 8 for banks large enough to matter). The 4-element + (`DotF32`/`L2F32` route non-multiple-of-8 `paddedDims` to the SSE path). The 4-element remainder is now added to both the intrinsics and their scalar mirrors, bit-identically. - CI now requires the AVX2 path on the Linux job (previously accepted SSE or AVX2), so a runner without AVX2 hardware cannot silently skip the new sub-8 remainder coverage; From af79e7c545765ea83025baae8271a1146b5b7115 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 16:59:25 -0700 Subject: [PATCH 13/14] Tests: deterministic liveness gates for scratch concurrency storms TestChannelQueryOnlyStorm, TestChannelScopedReductionStorm, and TestRelabelExclusiveDrainStorm each spawned a writer thread and then ran a fixed-iteration reader loop, asserting afterward that the reader had observed the writer publish past its initial row count. Nothing guaranteed the writer was scheduled before the reader's iterations completed, so under concurrent machine load the reader could finish first and the assertion tripped on scheduling rather than on a real defect. Add WaitForPublishPastCount(), a bounded poll on ScratchBank::Count() (already an atomic acquire load, safe to call unpinned), and call it before each cell's measured loop so the loop cannot start until the writer has actually published a row. The wait is bounded so a genuine regression in the append/publish path still fails the test loudly instead of hanging or passing vacuously. This mirrors the readersReady start-barrier already used by TestConcurrencyStorm and TestPinDrainLitmus, adapted to a single-writer/ single-reader shape where the reader waits on the writer's actual publish rather than on thread startup alone. In TestChannelQueryOnlyStorm, also collapse the per-hit CHECK(std::isfinite(...)) loop into one CHECK per outer iteration. The hit count returned by a query moves with how far the writer has appended by that iteration, which made the suite's total check count vary run to run (85470 vs 85532 observed) and defeated the "N checks" summary line as a regression signal. Every returned score is still examined; the number of CHECK calls is now fixed at one per iteration regardless of how many hits came back. Retarget the version-identity test and a stale section-marker comment from v3.2.2 to v3.3.0, matching the version.h bump landing in this branch. Verified with three consecutive full-suite runs: 82835 checks, 0 failures, identical count each time. --- tests/test_main.cpp | 89 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 80 insertions(+), 9 deletions(-) diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 833cc5d..961382c 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -15,6 +15,7 @@ #include "xd_fixtures.h" #include +#include #include #include #include @@ -330,6 +331,31 @@ static void PadQuery(const std::vector& q, int32_t pd, float* out) std::memcpy(out, q.data(), q.size() * sizeof(float)); } +// Bounded-wait for a ScratchBank's published count to advance past `appended` -- the +// deterministic form of "the writer has published at least one fresh row" that a storm +// test's reader needs before its measured loop starts. Count() is an atomic acquire load +// (safe to poll unpinned, from any thread), so this returns as soon as the writer's first +// successful Append lands under real scheduling instead of on a fixed iteration budget. +// Bounded by `timeoutMs`: a genuine regression in the append/publish path (the writer never +// actually advancing the count) still fails the test loudly here, rather than leaving the +// caller's own liveness assertion to trip only when the reader happens to outrun the writer, +// or leaving the wait to hang forever. +static bool WaitForPublishPastCount(const ScratchBank& bank, int32_t appended, + int32_t timeoutMs = 5000) +{ + const auto deadline = + std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); + while (bank.Count() <= appended) + { + if (std::chrono::steady_clock::now() >= deadline) + { + return false; + } + std::this_thread::yield(); + } + return true; +} + // --------------------------------------------------------------------------- // T1 — known geometry (plan A6): hand-built vectors with hand-computed neighbors. @@ -12018,6 +12044,16 @@ static void TestChannelQueryOnlyStorm() } }); + // Deterministic liveness gate: do not begin the measured loop until the writer has + // actually published a row past `appended`. An unsynchronized start races the reader's + // 300 fast iterations against OS scheduling of the writer thread -- under concurrent + // machine load the reader can finish first and the storm never exercises a concurrent + // publish at all. Bounded so a genuine append/publish regression fails loudly here + // instead of leaving the loop to run vacuously. + CHECK_MSG(WaitForPublishPastCount(fx.bank, appended), + "writer never published a row past appended before the reader began measuring " + "(count stayed at %d)", fx.bank.Count()); + std::vector tombs(ScratchBank::TombstoneWords(capacity), 0u); std::vector recomputed(static_cast(capacity) * cc); int32_t maxCountSeen = 0; @@ -12038,7 +12074,11 @@ static void TestChannelQueryOnlyStorm() iter, C); // The channel query over the fresh snapshot succeeds with finite scores (a torn / - // stale sub-norm would surface here as a non-finite or wrong score). + // stale sub-norm would surface here as a non-finite or wrong score). One CHECK per + // iteration covering every returned hit -- the hit count `n` itself moves with + // scheduling (how far the writer has appended by this iteration), so a per-hit CHECK + // would make the suite's total check count scheduling-dependent too. Every score is + // still examined; only the number of CHECK calls is now fixed at one per iteration. Workspace ws; Hit hits[10]; int32_t n = 0; @@ -12048,15 +12088,20 @@ static void TestChannelQueryOnlyStorm() params.segmentCount = 1; params.excludeBits = tombs.data(); CHECK(Query(snap, qbuf.F32(), params, ws, hits, &n) == Status::Ok); + bool allFinite = true; for (int32_t i = 0; i < n; ++i) { - CHECK(std::isfinite(hits[i].score)); + allFinite = allFinite && std::isfinite(hits[i].score); } + CHECK_MSG(allFinite, "channel query returned a non-finite score (iter %d, n %d)", + iter, n); } stop.store(true, std::memory_order_relaxed); writer.join(); // The reader must have observed the count ADVANCE past the initial rows, or the storm - // never exercised a concurrent publish (the P-1 defect this rewrite fixes). + // never exercised a concurrent publish (the P-1 defect this rewrite fixes). Guaranteed by + // the wait above (Count() is monotonically non-decreasing), and kept as a live regression + // check against that guarantee rather than deleted. CHECK_MSG(maxCountSeen > appended, "reader never observed a freshly-published row (count stayed at %d)", maxCountSeen); } @@ -12115,6 +12160,14 @@ static void TestChannelScopedReductionStorm() } }); + // Deterministic liveness gate: do not begin the measured loop until the writer has + // actually published a row past `appended` -- see WaitForPublishPastCount. Without it, + // the reader's 200 fast iterations can complete before the OS schedules the writer at + // all under concurrent load, and the storm never exercises a concurrent publish. + CHECK_MSG(WaitForPublishPastCount(fx.bank, appended), + "writer never published a row past appended before the reader began measuring " + "(count stayed at %d)", fx.bank.Count()); + std::vector tombs(ScratchBank::TombstoneWords(capacity), 0u); std::vector recomputed(static_cast(capacity) * cc); int32_t maxCountSeen = 0; @@ -12151,6 +12204,8 @@ static void TestChannelScopedReductionStorm() } stop.store(true, std::memory_order_relaxed); writer.join(); + // Guaranteed by the wait above (Count() is monotonically non-decreasing); kept as a live + // regression check against that guarantee rather than deleted. CHECK_MSG(maxCountSeen > appended, "reader never observed a freshly-published row (count stayed at %d)", maxCountSeen); } @@ -12952,11 +13007,11 @@ static void TestPerChannelRecallOracle() static void TestVersionHeaderCoherence() { CHECK_MSG(SUPERFAISS_VERSION_MAJOR == 3, - "SUPERFAISS_VERSION_MAJOR should be 3 for v3.2.2, got %d", SUPERFAISS_VERSION_MAJOR); - CHECK_MSG(SUPERFAISS_VERSION_MINOR == 2, - "SUPERFAISS_VERSION_MINOR should be 2 for v3.2.2, got %d", SUPERFAISS_VERSION_MINOR); - CHECK_MSG(SUPERFAISS_VERSION_PATCH == 2, - "SUPERFAISS_VERSION_PATCH should be 2 for v3.2.2, got %d", SUPERFAISS_VERSION_PATCH); + "SUPERFAISS_VERSION_MAJOR should be 3 for v3.3.0, got %d", SUPERFAISS_VERSION_MAJOR); + CHECK_MSG(SUPERFAISS_VERSION_MINOR == 3, + "SUPERFAISS_VERSION_MINOR should be 3 for v3.3.0, got %d", SUPERFAISS_VERSION_MINOR); + CHECK_MSG(SUPERFAISS_VERSION_PATCH == 0, + "SUPERFAISS_VERSION_PATCH should be 0 for v3.3.0, got %d", SUPERFAISS_VERSION_PATCH); } // =========================================================================== @@ -13630,6 +13685,16 @@ static void TestRelabelExclusiveDrainStorm() } }); + // Deterministic liveness gate: do not begin the measured loop until the writer has + // actually published a row past `appended` -- see WaitForPublishPastCount. Without it, + // the reader's 300 fast pin/snapshot iterations can complete before the OS schedules the + // writer thread at all under concurrent load, and the storm never exercises a concurrent + // publish (or, since the writer's very first iteration is a relabel, a concurrent + // relabel either). + CHECK_MSG(WaitForPublishPastCount(bank, appended), + "writer never published a row past appended before the reader began measuring " + "(count stayed at %d)", bank.Count()); + std::vector tombs(ScratchBank::TombstoneWords(capacity), 0u); int32_t maxCountSeen = 0; for (int32_t iter = 0; iter < 300; ++iter) @@ -13657,6 +13722,10 @@ static void TestRelabelExclusiveDrainStorm() stop.store(true, std::memory_order_relaxed); writer.join(); + // The writer's own iteration order puts a relabel attempt at iter 0, before its first + // append attempt at iter 1 -- so the wait above (which requires at least one successful + // append) already implies at least one completed relabel by construction, and this check + // is deterministic as a consequence rather than independently scheduling-dependent. CHECK_MSG(relabelCount.load(std::memory_order_relaxed) > 0, "relabel storm: the writer never completed a relabel"); BankView finalSnap; @@ -13674,6 +13743,8 @@ static void TestRelabelExclusiveDrainStorm() CHECK_MSG(matchedSomeTarget, "relabel storm: the final channel table never left the fixture's initial (1-channel) " "table -- Relabel never mutated the bank under concurrent load"); + // Guaranteed by the wait above (Count() is monotonically non-decreasing); kept as a live + // regression check against that guarantee rather than deleted. CHECK_MSG(maxCountSeen > appended, "relabel storm: reader never observed a freshly-published row (count stayed at %d)", maxCountSeen); @@ -19943,7 +20014,7 @@ int main() TestS1FlatAllocationKthNeighborDistanceProbe(); TestS1FlatAllocationMutualNearestMatches(); - // V3.2.2: public-path geometry ceilings, and the archive peek the host trailer + // V3.3.0: public-path geometry ceilings, and the archive peek the host trailer // validation is built on. TestScratchGeometryCeilings(); TestScratchLoadValidatesRetainedRegion(); From 926ac520e7fae318c3d667a8928c2d75eeb7f5e0 Mon Sep 17 00:00:00 2001 From: "DESKTOP-T2JJTNM\\dansu" Date: Tue, 21 Jul 2026 17:02:56 -0700 Subject: [PATCH 14/14] Tests: correct a mischaracterized comment on the storm cells' trailing checks The comment above CHECK_MSG(maxCountSeen > appended, ...) in the three scratch concurrency storm cells stated the check was "guaranteed by the wait above," implying it could never fail and existed only as inert documentation. That is wrong: WaitForPublishPastCount polls bank.Count() directly, while maxCountSeen is accumulated from snap.count via bank.Snapshot() -- a different read path. The trailing check is the one that would catch a regression in Snapshot's own acquire ordering (Snapshot returning a stale count even though Count() has already advanced), which the gate does not observe at all. Replace the comment in all three cells with one that states what the check actually guards and makes explicit that it reads a different path from the gate, so it is not mistaken for redundant and removed. No assertion or gate logic changed. Verified with a full-suite run: 82835 checks, 0 failures, identical to the prior run. --- tests/test_main.cpp | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/tests/test_main.cpp b/tests/test_main.cpp index 961382c..fbe6db5 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -12099,9 +12099,12 @@ static void TestChannelQueryOnlyStorm() stop.store(true, std::memory_order_relaxed); writer.join(); // The reader must have observed the count ADVANCE past the initial rows, or the storm - // never exercised a concurrent publish (the P-1 defect this rewrite fixes). Guaranteed by - // the wait above (Count() is monotonically non-decreasing), and kept as a live regression - // check against that guarantee rather than deleted. + // never exercised a concurrent publish (the P-1 defect this rewrite fixes). NOT implied + // by the wait above: the gate polls bank.Count() directly, while maxCountSeen is + // accumulated from snap.count via bank.Snapshot() -- a different read path. This is the + // check that catches a regression in Snapshot's own acquire ordering (Snapshot returning + // a stale count even though Count() itself has advanced); it is a live guard on a + // distinct path from the gate and must not be removed as redundant with it. CHECK_MSG(maxCountSeen > appended, "reader never observed a freshly-published row (count stayed at %d)", maxCountSeen); } @@ -12204,8 +12207,11 @@ static void TestChannelScopedReductionStorm() } stop.store(true, std::memory_order_relaxed); writer.join(); - // Guaranteed by the wait above (Count() is monotonically non-decreasing); kept as a live - // regression check against that guarantee rather than deleted. + // NOT implied by the wait above: the gate polls bank.Count() directly, while + // maxCountSeen is accumulated from snap.count via bank.Snapshot() -- a different read + // path. This is the check that catches a regression in Snapshot's own acquire ordering + // (Snapshot returning a stale count even though Count() itself has advanced); it is a + // live guard on a distinct path from the gate and must not be removed as redundant. CHECK_MSG(maxCountSeen > appended, "reader never observed a freshly-published row (count stayed at %d)", maxCountSeen); } @@ -13743,8 +13749,11 @@ static void TestRelabelExclusiveDrainStorm() CHECK_MSG(matchedSomeTarget, "relabel storm: the final channel table never left the fixture's initial (1-channel) " "table -- Relabel never mutated the bank under concurrent load"); - // Guaranteed by the wait above (Count() is monotonically non-decreasing); kept as a live - // regression check against that guarantee rather than deleted. + // NOT implied by the wait above: the gate polls bank.Count() directly, while + // maxCountSeen is accumulated from snap.count via bank.Snapshot() -- a different read + // path. This is the check that catches a regression in Snapshot's own acquire ordering + // (Snapshot returning a stale count even though Count() itself has advanced); it is a + // live guard on a distinct path from the gate and must not be removed as redundant. CHECK_MSG(maxCountSeen > appended, "relabel storm: reader never observed a freshly-published row (count stayed at %d)", maxCountSeen);