ci: run the whole etcd suite, measure coverage, unskip remote tests - #226
Merged
Conversation
CI had infrastructure it wasn't using and blind spots it didn't report. Run all 19 etcd-backed tests. The workflow already downloaded and started etcd, then ran exactly one test by name, leaving the scheduler, routes, and state etcd paths untested. Turning them on surfaced four failures in tests that had never once executed: - scan_reconciles_removed_experiments was a real bug. StatsStore::write_snapshot early-returns on empty rows to stop a round of total observe failures from blanking the table and starving the compaction and WAL-merge sweeps. But it tested rows.is_empty() rather than why, conflating that with "every experiment was legitimately deregistered" -- so a deployment draining to zero experiments served stale rows forever and kept sweeping datasets whose registry entries were gone. Masked at N>1. Split out replace_snapshot, which honors empty; the scanner picks between them using the failure count it already has. - startup_requeues_interrupted_local_tasks was a stale precondition, not a recovery bug. Dropping a TaskClaim stops lease renewal but does not revoke the lease, so etcd holds the claim key for the rest of the TTL and the task is correctly still Running. Added abandon_claim_for_test to revoke it, reaching the post-crash key state without sleeping out the TTL. - The two routes endpoint tests predated the LSM write path: add() only appends to that handle's memtable, so rows are invisible to the separate handle the endpoint opens until flush() seals them. Added the flush; all assertions intact. Measure coverage. Nothing measured it before. New job collects the normal and etcd-backed runs into one lcov report and uploads to Codecov, informational only until a baseline exists. Stop skipping the remote tests. test_rollout_remote.py is the only coverage of the remote/HTTP client path -- lance-context-client has no tests of its own -- and all 4 silently skipped because CI installs a wheel and never built the server binary. Build it, and make a missing binary a hard failure under CI so this cannot regress to skipping again. Schedule the harness. test/harness/ asserts the ?source=fragments|wal|all selector semantics that nothing else covers, and no workflow referenced it. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI had infrastructure it was not using and blind spots it did not report. Four changes, one of which uncovered a real bug.
1. Run all 19 etcd-backed tests (was: 1)
rust-test.ymldownloaded and started etcd 3.7.0, then ran exactly one test by name with--ignored --exact. The other 18 — thescheduler.rs,routes.rs, andstate.rsetcd paths — never ran. Switching tocargo test -p lance-context-master --lib -- --ignoredis nearly free since etcd is already up.Turning them on surfaced 4 deterministic failures in tests that had never once executed:
scan_reconciles_removed_experiments— real production bug.StatsStore::write_snapshotearly-returns on emptyrowsto stop a round of total observe failures from blanking the table and silently starving the compaction and WAL-merge sweeps (they read this table to decide what to enqueue). But the guard testedrows.is_empty()rather than why it was empty, conflating that hazard with "every experiment was legitimately deregistered." Impact beyond the test: a deployment draining to zero registered experiments serves stale rows indefinitely, and those rows keep feeding sweeps that open datasets whose registry entries are gone. Masked at N>1, which is why only a single-experiment test surfaces it. Fixed by splitting outreplace_snapshot(honors empty); the scanner disambiguates at the call site using the failure count it already has.write_snapshotstill refuses empty, so the original hazard stays defended.startup_requeues_interrupted_local_tasks— stale test, production correct. Dropping aTaskClaimstops lease renewal but does not revoke the lease, so etcd holds the claim key for the rest of the TTL andrecover_orphanedcorrectly declines to requeue — the task genuinely is still running. Addedabandon_claim_for_testto revoke the lease, reaching the same key state a real crash reaches once the TTL elapses, without a 5s sleep. Assertions kept verbatim; mutation-tested (stubbingrecover_orphanedtoOk(0)fails the test) to confirm it is not vacuous.The two
routes.rsendpoint tests — stale, predate the LSM write path.add()only appends to that handles in-memory memtable; rows are invisible to the separate handle the endpoint opens untilflush()` seals them into a committed generation. Added the flush, all assertions intact. Not a polling situation — the seal is caller-triggered, so a timeout loop would just spin.2. Measure coverage
Nothing measured it. New
coveragejob collects the normal run and the etcd-backed run into one lcov report viacargo llvm-cov --no-report+report, uploads to Codecov.codecov.ymlis informational only — no gate can fail a build until we have a baseline.3. Stop skipping the remote tests
test_rollout_remote.pyis the only coverage of the remote/HTTP client path —lance-context-clienthas zero tests of its own — and all 4 tests silently skipped because CI installs a prebuilt wheel and never built the server binary. Now builds it, and a missing binary is a hard failure underCI(still a graceful skip locally) so this cannot regress to silent skipping.4. Schedule the harness
test/harness/asserts the?source=fragments|wal|allselector semantics that nothing else exercises, and no workflow referenced it. Added a nightlyworkflow_dispatch-able job with log dumping on failure andif: always()teardown.Verification
--test-threads=1, repeated runscargo test --workspace --all-targetsgreen (214 core, 62 server, 33 master, 18 api, ...)cargo fmt --checkandcargo clippy --workspace --all-targetscleanNote:
python-test.ymltimeout raised 30 → 45 min and the root workspace added to itsrust-cachelist, since that job now builds the server binary.🤖 Generated with Claude Code