Harden the feed download against paging, empty and pinless responses - #95
Merged
Merged
Conversation
Four fixes from a review of pipeline.py, each with a regression test that failed first. download_cases pages by key, OBJECTID > <last seen>, instead of by resultOffset. By offset, one case deleted mid-download shifted a live case out of the next page and stamped it vanished for that build, and a server maxRecordCount of 995 lost 5 rows a page (20 of 4,300), both under FEED_COUNT_TOLERANCE. check_download_complete refuses an empty feed while the DB holds Open cases not yet vanished. 0 of 0 passed the tolerance and would have stamped all 498 open cases on the 2026-09-23 release vanished. A partial purge is still stamped, as data-quality.md says it should be. map_cases no longer KeyErrors on a feature with no geometry, which ArcGIS omits for a null shape and which crashed every build. The coordinates are REAL NOT NULL and key the geocode cache, and dropping the constraint is not an additive migration, so restore_pins gives such a case the pin the DB last stored for it; a case never pinned is set aside and printed until the feed pins it. The DB therefore never holds NULL coordinates and geocoding and the site are unchanged. backfill_reduced_pressure reads a NULL feed flag as 0. On the release DB the 7 NULL-flag rows do not match the pattern, so no published number moves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0168hLW2X3mkV3Jhm26LJSwQ
From a code review of this PR: - An empty point written as "NaN" passed the None check, became a nan coordinate and crashed the build on a NOT NULL column. Coordinates that are not finite numbers now count as missing. - restore_pins assumed a cases table whenever the DB file existed; geocode_all can create the file first. Both readers go through one helper that answers "no table yet". - The empty-feed guard counted open rows, but the vanish stamp touches every row not yet vanished, closed ones included (4,306 on the release, not 498). It now counts those, and its dead `not features` branch and misleading message are gone. - Key paging trusted the server's order. A page that is not ascending now fails the build, and a fake feed stored out of order makes dropping orderByFields fail a test. - A case the feed has never pinned is surfaced as a ::warning:: on the run rather than a log line, and the decision is in data-quality.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0168hLW2X3mkV3Jhm26LJSwQ
From a second code review of the review-fix commit: - Read-only connections are closed on every path (a context manager over the cases table replaces `with conn:`, which never closes), the column probe runs once, and the DB URI is built with as_uri(). - The empty-feed refusal keys on an empty download, which is what the vanish stamp acts on; a 0 count with a full download now builds. - The page check is one strictly-increasing test from the last id, so a server ignoring `OBJECTID > n` is refused rather than looped over, and a feature with no OBJECTID fails with the page named. - Tests for both refusal directions and the ignored filter; the test's own sqlite connection is closed. CLAUDE.md's vanished_at row is amended and gains a row for features with no pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0168hLW2X3mkV3Jhm26LJSwQ
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.
Ingest fixes from the retroactive review (PR F of the set). This PR changes no published number on the current data.
Changes (
src/uisce/pipeline.py)Paging by key.
download_casespages withOBJECTID > <last seen>instead ofresultOffset. With offsets, two things went wrong, and both stayed inside the 1% count tolerance:maxRecordCountbelow 1,000 silently dropped the difference on every page.Every page must be strictly ascending from the last id, or the build fails with the page named. That also covers a server that ignores the filter and re-serves a page, and a feature with no
OBJECTID.Features with no pin. ArcGIS leaves out
geometryfor a null shape, or writes an empty point as"NaN", and one such feature used to crash every build. The coordinates areNOT NULLand key the geocode cache, so allowing NULL would mean a non-additive schema change. Instead, a new function,restore_pins, handles these cases:::warning::on the Actions run, until the feed pins it.Empty download.
check_download_completerefuses an empty download while the database holds rows that aren't vanished. The vanish stamp touches every such row, closed ones included: 4,306 on the release, of which 498 are open. A count of 0 alongside a full download still builds.backfill_reduced_pressurenow usesCOALESCE(reduced_pressure, 0) = 0. None of the 7 rows with a NULL flag matches today.Read-only database access. It goes through one context manager that always closes the connection, probes the columns once, and builds the URI with
as_uri().Review
First round, high effort, 8 findings.
"NaN"point;casestable yet.orderByFieldswas removed.Second round, medium effort, 10 findings.
OBJECTID.CLAUDE.mdrows needed amending and adding.All are addressed.
Notes
notes/data-quality.md: a dated paragraph in the vanished-cases section, and "A feature with no pin (2026-09-24)".CLAUDE.md: the vanished_at row is amended, and there is a new row for features with no pin.Tests
tests/test_pipeline.py, each failing before its fix:LiveFeedfakes a server that deletes a row mid-download, one that caps pages, one that ignoresorderByFields, one with unsorted storage, and one that re-serves the first page (it fails fast instead of looping)."NaN"point.TestRestorePins, including a database with nocasestable.uv run ruff checkanduv run pytestpass: 702 tests, merged with currentmain.🤖 Generated with Claude Code
https://claude.ai/code/session_0168hLW2X3mkV3Jhm26LJSwQ