Skip to content

INV-4b's EXCUSED list is maintenance-by-hand: every non-engine module under engine/ breaks CI until someone adds it #287

Description

@fcostaoliveira

Follow-up to #263, which added INV-4b to tests/overhead_invariants.rs and — for the first time — wired cargo test --test overhead_invariants --release into CI.

What happens today

INV-4b derives its engine list from the mod X; declarations in
src/bin/vector_db_benchmark/engine/mod.rs, then subtracts a hand-maintained
EXCUSED list (tests/overhead_invariants.rs:249). Anything left must contain
WorkerPool::new / StartGate::new.

That means any new module under engine/ — including a pure helper with no
engine in it — fails CI until a human adds it to EXCUSED. It just happened on
#286, which adds engine/geo.rs (359 lines of spherical geometry, zero
impl Engine for blocks):

INV-4b VIOLATED (#214):
  geo.rs: no `WorkerPool::new` / `StartGate::new` — the synchronized start is gone,
  so connection setup and the cold first query are back inside the measured window

The one-line EXCUSED addition fixes it, and #286 does exactly that. But the cost
is paid again by every future helper, and the failure it produces is a false
positive — which is the kind of failure that trains people to add names to
EXCUSED without reading why.

Note the existing list is already 5/6 "pure helper, no engine" / "transport
codegen, no engine" entries (index_naming, redis_utils, vertex_grpc,
weaviate_grpc, filter_guard). Only one entry — turbopuffer — is a real
engine that is deliberately ungated. The list is 83 % noise.

Proposed rule

Skip any file with no impl Engine for automatically, and keep EXCUSED only
for genuine engines that are deliberately ungated (today: turbopuffer).

This is strictly stronger, not weaker:

  • it removes the five helper entries, so the list stops being a place to dump
    names and each remaining entry is a real, reviewable claim;
  • a module that gains an impl Engine for is opted in automatically, which is
    the property fix(harness): thread-spawn failure and worker panics deadlocked the search start barrier #263 wanted — today it stays excused forever because its name is
    already on the list. That is the live hazard: vertex_grpc / weaviate_grpc
    are excused as "transport codegen", and if either ever grows an engine impl,
    INV-4b would silently stop checking it;
  • it needs no new source scanning — the file text is already read
    (read_engine_raw), and impl Engine for is the same marker the other
    invariants key on.

Suggested shape: keep the mod.rs derivation, drop a module when its source has no
impl Engine for, and assert that every surviving EXCUSED name is an engine
by that test — so a helper left in EXCUSED fails, the same way a stale
KNOWN_GAPS entry fails in engine/filter_guard.rs.

Also worth fixing: the message

For a file that never had a gate, "the synchronized start is gone" is wrong —
it reads as a regression in a file that never had the thing. It should distinguish
"this engine dropped its gate" from "this module has no gate and no engine impl".

Scope

Not doing this in #286 — that PR is geo filter builders and should not be
entangled with harness work. Filing it here against #263's guard.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions