feat(storage): data-failure policy — fail visits on constraint violations (crosslink #46)

[vringar]

Data-failure policy (crosslink #46)

Implements the data-failure policy in the core StorageController / provider
path. This is the keystone that resolves the adversarial G1 / G1b / G2
robustness gaps and aligns the in-flight storage work.

Stacks on #1197 (fix/storage-forward-progress, the data-integrity
foundation) — this PR targets that branch, not master. Review/merge #1197
first. The forward-progress fix handles the transient half of the policy
(retry a write/flush blip); this PR adds the data-fault half.

The policy

A failed store_record / finalize is classified into two failure classes:

Class	What it is	Handling
Constraint violation (data fault)	A record tripped the storage schema: DB IntegrityError, type mismatch, NOT-NULL / PK / UNIQUE, unknown table/column, parquet ArrowInvalid	Catch, log a clear investigable error (visit_id, table, which constraint, record key context — PII/size-aware), mark the VISIT FAILED (recorded in incomplete_visits). Never silently dropped, never retried (a malformed record can't succeed on retry). The site surfaces by repeatedly failing with a good error message.
Transient error (infra fault)	A write/flush blip (e.g. an S3 PutObject failure)	Retried, per the forward-progress fix already on the base branch.

What changed

• openwpm/errors.py — new ConstraintViolation(message, *, table, visit_id, reason): the typed data-fault signal carrying enough context to investigate.
• openwpm/storage/sql_provider.py — store_record now raises ConstraintViolation on schema-tripping errors (IntegrityError / ProgrammingError / InterfaceError, and the schema-mismatch subset of OperationalError — "no such table/column"), instead of logging "Unsupported record" and silently dropping. Genuinely transient OperationalErrors ("database is locked", "disk I/O error") are re-raised for the retry path.
• openwpm/storage/arrow_storage.py — _create_batch raises ConstraintViolation on pa.lib.ArrowInvalid instead of silently dropping the batch.
• openwpm/storage/storage_controller.py:
  • _guarded_store_record isolates every per-record store at the task boundary: a ConstraintViolation (or any unexpected exception) is logged with a clear, investigable message and recorded in failed_visits; it never re-raises into the connection handler or the finalize path.
  • finalize_visit_id forces the visit's effective success to False if any of its records tripped the schema (even if the browser sent success=True), and handles a ConstraintViolation surfaced lazily by the Arrow provider at finalize time — re-finalizing the visit as interrupted so it still reaches a FAILED terminal state.
  • A permanent flush failure on shutdown surfaces the real error (not a masked "timeout") and still drains every pending visit as FAILED so shutdown neither hangs nor strands a visit.

How the invariants now hold

The four non-negotiables (and the adversarial invariants) hold:

1. Never strand — every started visit reaches a terminal state. A raising store no longer aborts finalize_visit_id, so the visit is always enqueued (as failed). (G1: test_raising_store_record_visit_still_finalizes, ..._unfinalized_visit_enqueued_on_shutdown)
2. Never hang — a callback CommandSequence blocked on a failed record always gets a terminal result. A permanent write fault no longer leaves a finalize token unresolved and blocking shutdown. (G2: test_permanent_write_table_fault_visit_still_completes)
3. Never break the shared DataSocket — the per-record failure is isolated so the connection (and the rest of the crawl) survives one bad record. (G1b: test_raising_store_record_does_not_break_shared_connection)
4. Never silently drop — a schema-tripping record fails the visit and is recorded in incomplete_visits. (test_constraint_violation_fails_visit_and_records_incomplete)

Tests

Brings the relevant adversarial scenarios in as passing tests (they were xfail(strict=True) in the adversarial suite — under this policy they now pass):

• test/storage/test_adversarial_storage_controller.py — G1, G1b, G2, a dedicated constraint-violation test, plus the surviving S4/S6 scenarios.
• test/storage/test_adversarial_socket.py — wire-protocol hostility (S3).
• test/storage/conftest.py — adversarial_mp_logger fixture (errors are expected).

The existing forward-progress liveness tests (test_storage_controller_liveness.py) stay green, as do test_storage_controller.py, test_storage_providers.py, and test_arrow_cache.py. mypy and pre-commit pass.

Cross-PR coordination

• test: adversarial robustness suite for the crawl pipeline #1198 (test/adversarial-robustness): the four xfail(strict=True) G1/G1b/G2 tests flip to PASS under this policy. That suite should rebase onto / be superseded by this PR — its xfail markers must be removed (they would XPASS-fail otherwise).
• feat(storage): SQLAlchemy storage provider with PostgreSQL support #1161 (SQLAlchemy/Postgres provider): its store_record needs the same constraint→fail-visit handling (raise ConstraintViolation on DB integrity/schema errors). Its provider isn't on this base, so that's a follow-up on its own branch.
• feat: add extension-to-BrowserManager finalize ack #1174: the error-surfacing concern (don't mask a real finalize error as a timeout) aligns — the shutdown drain here surfaces the genuine failure instead of a timeout.

Open sub-decision for a maintainer

On the Arrow path, a constraint violation that hits one of several tables in a visit keeps the already-validated batches from the other tables (partial-keep) and still records the visit as incomplete. This is the non-destructive default (don't roll back valid data). If a failed visit should instead be all-or-nothing (roll back every batch for that visit), that's a deliberate change — flagging it rather than guessing.

[vringar]

Sub-decision resolved (maintainer): partial-keep confirmed for the Arrow path. When one table's record trips the schema mid-visit, keep the already-validated batches from the other tables and mark the visit incomplete — no all-or-nothing rollback.

Rationale: the first postprocessing step routinely drops every record associated with a failed visit_id, so the partial data never pollutes analysis — but it stays useful for debugging why the visit failed.

[codecov]

Codecov Report

❌ Patch coverage is 19.59184% with 197 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.23%. Comparing base (1dba4a9) to head (8417fc9).

Files with missing lines	Patch %	Lines
...est/storage/test_adversarial_storage_controller.py	3.19%	182 Missing ⚠️
openwpm/storage/storage_controller.py	73.52%	9 Missing ⚠️
openwpm/storage/arrow_storage.py	62.50%	3 Missing ⚠️
openwpm/storage/sql_provider.py	62.50%	3 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           fix/storage-forward-progress    #1203      +/-   ##
================================================================
- Coverage                         62.72%   60.23%   -2.50%     
================================================================
  Files                                40       41       +1     
  Lines                              3965     4197     +232     
================================================================
+ Hits                               2487     2528      +41     
- Misses                             1478     1669     +191     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.