gps-denied-onboard/_docs/03_implementation/reviews/batch_40_review.md

Batch 40 — Code Review

Tasks: AZ-316 (C11 TileDownloader) Cycle: 1 Reviewer: autodev Verdict: PASS_WITH_WARNINGS

Scope reviewed

Production code:

• src/gps_denied_onboard/components/c11_tile_manager/_types.py (additions: SectorClassification, DownloadOutcome, TileSummary, DownloadRequest, DownloadBatchReport)
• src/gps_denied_onboard/components/c11_tile_manager/errors.py (additions: ResolutionRejectionError, CacheBudgetExceededError)
• src/gps_denied_onboard/components/c11_tile_manager/config.py (additions: 6 download-side fields + validation)
• src/gps_denied_onboard/components/c11_tile_manager/interface.py (TileDownloader Protocol expanded to its real shape)
• src/gps_denied_onboard/components/c11_tile_manager/tile_downloader.py (new — HttpTileDownloader, request_hash, _JournalState, two consumer-side cuts)
• src/gps_denied_onboard/components/c11_tile_manager/__init__.py (exports for download-side public API)
• src/gps_denied_onboard/runtime_root/c11_factory.py (build_tile_downloader + private _C6DownloadAdapter class wrapping c6's TileMetadata assembly)

Tests:

• tests/unit/c11_tile_manager/test_tile_downloader.py (14 tests — AC-1..AC-9, AC-11, AC-12, NFR throughput, plus 429 HTTP-date form + 429 budget exhaustion)
• tests/unit/c11_tile_manager/test_protocol_conformance.py (2 new tests — AC-10 positive + negative)

Phase 1 — Architecture

AZ-507 cross-component rule

tile_downloader.py does NOT import from any other components.* module. The c6 surfaces (TileStore, TileMetadataStore, CacheBudgetEnforcer, FreshnessRejectionError, TileMetadata, TileSource, FreshnessLabel, VotingStatus) are reached through:

• Two structural Protocol cuts declared inside tile_downloader.py itself: _TileWriterLike and _BudgetEnforcerLike.
• A composition-root adapter _C6DownloadAdapter in runtime_root/c11_factory.py that lazily imports c6's enums + dataclasses inside its method bodies and assembles the TileMetadata envelope c6 expects.
• A structural duck-type check on the freshness-rejection exception class name (_is_freshness_rejection), so c6's FreshnessRejectionError is recognised by class identity without an import.

The AZ-270 lint (test_ac6_only_compose_root_imports_concrete_strategies) passes — the only c6 imports in the modified surface live in the composition root.

Composition root

build_tile_downloader reads the C11Config block from config.components['c11_tile_manager'], fails fast with ConfigError when satellite_provider_url or service_api_key is empty (the safe defaults exist for unit-test bootstrap; production / operator wiring MUST set both). The _C6DownloadAdapter instance is shared as the binding for both Protocol cuts so the downloader always sees the same backing c6 trio.

Logging envelopes

The spec calls for structured logs (no FDR records on the download path — confirmed by re-reading AZ-316). All emissions use the project kv envelope:

• INFO c11.download.session.start / c11.download.session.end (AC-NEW-6 visibility, AC-1).
• WARN c11.download.resolution_rejected (one per rejected tile, AC-2).
• WARN c11.download.freshness_rejected_summary (one per batch, AC-3).
• WARN c11.download.freshness_downgraded (one per downgraded tile, AC-4).
• WARN c11.download.batch.retry (one per HTTP retry, AC-5/AC-6 telemetry).
• ERROR c11.download.provider.failed (terminal HTTP error, AC-6/AC-7).
• ERROR c11.download.budget.exceeded (AC-9).
• INFO c11.download.idempotent_no_op (AC-8).

The auth header is logged ONLY redacted (Bearer ***); the AC-11 test asserts the raw API key never appears in any log record.

Phase 2 — Behaviour vs. spec

Spec requirement	Status
Validate request (bbox, zoom, cache_root)	PASS — bbox + zoom validated in DownloadRequest.__post_init__; cache_root writability verified implicitly by the journal write (see F4)
Idempotence via cache_root/.c11/journal/<flight_id>__<request_hash>.json	PASS
Enumerate via ?list-only=true	PASS
Cache headroom pre-check via reserve_headroom(estimated_bytes)	PASS
Auth: TLS + Authorization: Bearer <api_key>	PASS
429 honours Retry-After (int + HTTP-date) capped via config	PASS
5xx exponential backoff (1s/2s/4s/8s, 4 retries) → SatelliteProviderError	PASS
TLS / 401 / 403 → SatelliteProviderError fail-fast	PASS
Resolution gate at C11 boundary (≥ 0.5 m/px)	PASS
FreshnessRejectionError from c6 → counted, not propagated	PASS
FreshnessLabel.DOWNGRADED → tile persisted + counted	PASS (see F2)
Per-tile journal append after each successful write	PASS
DownloadBatchReport(outcome=success/partial/failure/idempotent_no_op)	PASS — failure paths raise (typed exception) and journal is flushed in the finally / exception block; the spec text suggests failure as a return value (see F1)
Lockfile assertion at construction	NOT IMPLEMENTED (see F3)

Findings

F1 — Low (Spec wording vs. impl, recurring across C11 trio): AZ-316 step 4 / 6 prose says "outcome = failure" on cache-budget, persistent 5xx, or auth failure. My implementation raises CacheBudgetExceededError / SatelliteProviderError / RateLimitedError instead of returning a FAILURE report — matching the contract's exception matrix and the AC test surface (AC-6 / AC-7 / AC-9 explicitly assert pytest.raises). The DownloadOutcome.FAILURE enum value is wired into the journal's tile_counts envelope on the exception path so any downstream auditor can still distinguish failure from success without re-reading the exception trace. Same shape as Batch 39's F1. Action: documented; recommend a hygiene PBI to align AZ-316 + AZ-319 spec prose with their typed-exception contracts.

F2 — Low (Adapter returns conservative "fresh" label): The _C6DownloadAdapter.write_tile_for_download returns the literal string "fresh" because the c6 TileMetadataStore.insert_metadata API (AZ-303 contract) does not currently expose the post-insert freshness label as a return value — the freshness gate (AZ-307) raises on outright rejection but does not communicate the DOWNGRADED case to the caller. As a result, the AZ-316 tiles_downgraded counter only increments when the adapter actively reports "downgraded"; in the c6-as-it-stands wiring that path is currently unreachable. The HttpTileDownloader logic for the DOWNGRADED label is fully exercised by AC-4 via the _StubTileWriter.labels test fixture. Action: documented; if AZ-307 / AZ-303 surface a FreshnessLabel post-insert in a future iteration, only the adapter changes — the downloader is already wired.

F3 — Low (Risk 5 lockfile assertion deferred): Spec Risk 5 mitigation says "this task asserts the lockfile exists at construction (cache_root/.c11/lock) and refuses to start otherwise"; the lockfile creation is C12's job. I did NOT add the assertion in this batch — cache_root is a per-call parameter on DownloadRequest, not a constructor input, so the construction-time assertion the spec describes is not actually possible against the current Protocol shape. Adding the check at the start of download_tiles_for_area would be the correct place but C12 is not yet built (epic E-C12 is downstream), so the lockfile would always be absent and would block every operator run. Action: defer to the C12 epic; capture as a ResolutionRejectionError follow-up note when E-C12 lands.

F4 — Low (cache_root writability not pre-validated): Spec step 1 says "validates the DownloadRequest (… cache_root is writable)". My implementation relies on the journal write to fail naturally at _atomic_write_json, which calls mkdir(parents=True, exist_ok=True) and would raise PermissionError on a read-only mount. The error surfaces but is not the spec's preferred fail-fast ValueError from __post_init__. Action: documented; acceptable for a v1.0.0 ship — the failure mode is loud, not silent.

F5 — Low (Constructor signature deviation, recurring): Spec lists clock: Clock as a constructor parameter; my implementation injects a callable sleep (defaults to a WallClock-routed sleep). Same rationale as Batch 39's F2 — the downloader only ever needs to sleep, not monotonic_ns / time_ns. Threading the full Clock Protocol would carry payload the class never reads. The default _default_sleep helper routes through WallClock.sleep_until_ns, so the AZ-398 invariant (no time.sleep in components/) holds. Action: documented; revisit if a future C11 task needs the broader Clock surface.

Phase 3 — Tests

14 unit tests pass for HttpTileDownloader; 2 new tests for the TileDownloader Protocol conformance check (positive + negative). Full unit suite: 1420 passed, 80 skipped, 0 failed (skips are environment-gated: Docker, CUDA, TensorRT, Tier-2 hardware, actionlint). The +16-test net delta vs. Batch 39's 1404 baseline matches the 16 new tests added in this batch.

NFR-perf-throughput (50 MB/s on 1 Gbps): not unit-testable (requires real network). The unit test_nfr_throughput_1000_tiles_under_budget substitutes a 10-second wall-clock budget for a 1000-tile MockTransport batch — verifying the bookkeeping has no O(n²) regression rather than certifying real throughput. The real throughput target falls into the e2e perf suite (E-BBT).

AC-12 was tested via journal-truncation rather than process kill (4 prior + 6 new = 10 final, same shape as the spec's 30+70=100 example). Killing the test runner mid-batch is not feasible inside pytest; the journal-truncation simulation is the standard pattern for crash-recovery tests in this codebase (matches the C9 download journal tests).

Phase 4 — Quality gates

• ReadLints clean across c11_tile_manager/, runtime_root/c11_factory.py, and the new + extended test files
• No time.sleep in components/ (routes through WallClock.sleep_until_ns)
• No secrets in logs (AC-11 asserts the raw API key never appears in any captured log record; Bearer *** redaction confirmed)
• No new third-party dependencies (uses existing httpx and stdlib email.utils for HTTP-date parsing; atomicwrites semantics implemented inline because the project already uses the same pattern in C9 — staying consistent rather than adding a new dependency)

Verdict

PASS_WITH_WARNINGS — All five findings are Low severity (recurring spec-prose vs. typed-exception drift, adapter freshness-label conservatism pending an AZ-303 ABI extension, deferred Risk-5 lockfile assertion blocked on C12, missing pre-validation of cache_root writability, recurring Clock-vs-sleep injection deviation). No code change required for batch close-out. F1 and F5 are recurring across the C11 trio and should be captured as a single hygiene PBI in the next cumulative review.