From f1b2e167a09f8d0ed2e6e3765b121bdb243eede5 Mon Sep 17 00:00:00 2001 From: Umang Date: Fri, 31 Jul 2026 14:49:13 -0500 Subject: [PATCH] Harden post-merge verification --- CLAUDE.md | 14 +- IMPLEMENTATION_PLAN.md | 112 ++--- README.md | 326 ++++++++------- app/controllers/api/push_events_controller.rb | 2 +- app/jobs/application_job.rb | 2 +- app/jobs/reconcile_pending_enrichments_job.rb | 7 +- app/models/push_event.rb | 4 +- app/services/github.rb | 2 +- app/services/github/budget_ledger.rb | 9 +- app/services/github/enrichment/dispatch.rb | 5 +- app/services/github/ingestion_runner.rb | 4 +- app/services/inspection/push_event_view.rb | 4 +- config/application.rb | 6 +- config/recurring.yml | 11 +- docker-compose.yml | 7 +- docs/DESIGN_BRIEF.md | 393 +++++------------- docs/SUBMISSION_CHECKLIST.md | 283 ++++++++++--- ...05-at-least-once-with-idempotent-writes.md | 142 +++---- ...nqueue-and-entity-scoped-reconciliation.md | 24 +- docs/adr/0012-solid-queue-over-kafka.md | 32 +- .../2026-07-31-container-kill-recovery.md | 40 +- fixtures/github/README.md | 16 +- script/verify_recovery.sh | 340 ++++++++++++--- spec/docker_compose_spec.rb | 113 ++++- spec/recovery/duplicate_job_execution_spec.rb | 9 +- spec/requests/api/push_events_spec.rb | 4 +- spec/services/github/budget_ledger_spec.rb | 2 +- .../github/enrichment/end_to_end_spec.rb | 4 +- .../github/enrichment/fairness_stress_spec.rb | 8 +- spec/services/github/ingestion_runner_spec.rb | 4 +- spec/services/github/status/snapshot_spec.rb | 2 +- spec/stress/budget_ledger_spec.rb | 4 +- .../shared_examples/enrichable_entity.rb | 2 +- 33 files changed, 1157 insertions(+), 780 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d16892d..d85d66a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,11 +53,15 @@ acquire `SourceLock` — they take only the request gate. ### Processing semantics (plan §8) -At-least-once execution + idempotent writes + unique constraints = -effectively-once persisted outcomes. Never claim or code against exactly-once -execution. +Repeated observation and job execution are expected. State only the two proved ingestion +invariants: a duplicate GitHub event ID cannot create another `push_events` row, and that +duplicate cannot register entity activity or reactivate a `skipped_budget` entity. +Executions, ingestion runs, quarantine occurrence counts, budget debits, and logs may +repeat or change. Recovery before commit is conditional on the event remaining in a later +sliding-feed response. Never claim or code against exactly-once execution or universal +idempotency of persisted state. -### Idempotency invariants (plan §7) +### Duplicate-event invariants (plan §7) - `push_events` inserts use `ON CONFLICT (github_event_id) DO NOTHING RETURNING id`. - Entity activity fields (`last_seen_at`, `latest_event_at`, reactivation) update @@ -75,7 +79,7 @@ Fixture mode fails closed — never a live fallback. ## Database changes Schema changes go through migrations with intentional indexes, constraints where -correctness depends on them, and tests for uniqueness/idempotency (plan §7, §12). +correctness depends on them, and tests for uniqueness and replay behavior (plan §7, §12). ## Testing rules (plan §12) diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md index 1cecb37..29a3c67 100644 --- a/IMPLEMENTATION_PLAN.md +++ b/IMPLEMENTATION_PLAN.md @@ -12,7 +12,7 @@ The implementation will prioritize: - Correctness and durability - Clear separation of responsibilities -- Idempotent and restart-safe processing +- Duplicate-safe accepted-event persistence and explicit restart recovery - Predictable GitHub API usage driven by an **explicit, formula-derived, class-aware request budget** - Simple local operation through Docker Compose - Observable system behavior @@ -33,7 +33,7 @@ The delivered implementation will satisfy the required public-events source whil - Raw event payload retention (semantic retention via `jsonb` — see Section 7) - Structured push-event fields - Actor and repository enrichment (**budget-bounded, best-effort sampling with per-class fairness** — see Section 10) -- Idempotent ingestion +- Duplicate-safe `push_events` persistence - Pagination via the `Link` response header - ETag and `304 Not Modified` handling (bandwidth/correctness measure, scoped to the canonical first-page request — see Sections 9–10) - `X-Poll-Interval` support (treated as a server-imposed floor, never the cadence) @@ -175,7 +175,7 @@ Child issues: 5. Implement `PushEvent` filtering and tolerant normalization 6. Add one-shot ingestion command with defined contention semantics (Section 9) 7. Add recurring polling via Solid Queue recurring task -8. Add idempotency and restart safety +8. Add duplicate-safe event persistence and restart recovery ### Story 2 — Persist Raw and Structured Data @@ -237,19 +237,19 @@ Child issues: 9. Implement enrichment fairness shares (floor/remainder rounding) with eligibility-aware borrowing 10. Implement the per-window budget bootstrap (first real poll initializes each window) -### Extension B — Idempotency and Restart Safety +### Extension B — Duplicate-safe Event Persistence and Restart Safety Child issues: 1. Add unique GitHub event constraint -2. Add idempotent event persistence (`ON CONFLICT DO NOTHING RETURNING id`) -3. Add idempotent actor and repository upserts (`ON CONFLICT DO UPDATE` with merge rules; activity updates only for newly inserted events) +2. Add conflict-safe event persistence (`ON CONFLICT DO NOTHING RETURNING id`) +3. Add merge-rule actor and repository upserts (`ON CONFLICT DO UPDATE`; activity updates only for newly inserted events) 4. Use PostgreSQL-backed jobs (Solid Queue) 5. Preserve pending work in business tables 6. Reconcile work not enqueued before a crash 7. Add crash-safe source ownership (session advisory lock; verify release on session death) 8. Bound the enrichment backlog via the eligibility window and `skipped_budget` state (no unbounded growth) -9. Add Docker restart policies; test crash-window scenarios including container kills +9. Add Docker restart policies; test API-stop and main-process-crash paths separately 10. Document processing guarantees ### Extension C — Object Storage @@ -606,7 +606,7 @@ A GitHub event is considered accepted only after its `push_events` row is commit 4. Filter `PushEvent` entries; route valid non-push events to counters. 5. Normalize required attributes tolerantly; route failures to `quarantined_events`. 6. Upsert stub actor and repository rows (identity fields only). -7. Insert raw and structured records idempotently (`RETURNING id`). +7. Insert the raw and structured event row with conflict skipping (`RETURNING id`). 8. For rows actually inserted: apply entity activity updates and reactivation (Section 7). 9. Commit the PostgreSQL transaction (short-lived — the advisory lock, not the transaction, spans the HTTP work). 10. Enqueue enrichment after commit — Solid Queue lives in its own database, so same-transaction enqueue is not available; the committed entity state is the durable record of pending work (outbox-style recovery, per Section 2A). @@ -617,7 +617,9 @@ A GitHub event is considered accepted only after its `push_events` row is commit #### Crash before commit -The transaction rolls back; the session advisory lock releases with the dead session. The event may appear again in an overlapping GitHub poll. +The transaction rolls back; the session advisory lock releases with the dead session. The +event is recoverable only if it remains present in a later response from GitHub's sliding +feed. If the window advances past it first, this service does not recover it. #### Crash after commit @@ -629,27 +631,25 @@ The job is retried. #### Worker crash after enrichment commit but before acknowledgement -The job may execute again. Idempotent upserts produce the same durable outcome. +The job may execute again. Selector leases, freshness checks, and constrained upserts +prevent duplicate entity rows, but execution and operational side effects may repeat. ### Processing semantics -The system provides: - -```text -At-least-once execution -+ -Idempotent writes -+ -Unique constraints -= -Effectively-once persisted outcomes -``` - -The system does not claim exactly-once execution. +Repeated observation and job delivery are expected. The system enforces two narrower +invariants: a duplicate GitHub event ID cannot create another `push_events` row, and that +duplicate cannot register entity activity or reactivate a `skipped_budget` entity. A +duplicate may refresh permitted identity fields. Executions, `ingestion_runs`, quarantine +occurrence counts, budget debits, and logs may repeat or change. The system does not claim +exactly-once execution or universal idempotency of persisted state. ### Docker persistence -PostgreSQL will use a named Docker volume. Normal container stop, restart, crash, recreation, or `docker compose down` must not remove stored events. An explicit volume deletion is treated as an intentional destructive reset. Restart policies (Section 2A) make crash recovery automatic; Section 15 verifies it with actual container kills. +PostgreSQL will use a named Docker volume. Normal container stop, restart, crash, +recreation, or `docker compose down` must not remove stored events. An explicit volume +deletion is treated as an intentional destructive reset. Restart policies (Section 2A) +recover main-process crashes automatically; a daemon API stop such as `docker kill` leaves +the container down until an operator recreates it. Section 15 verifies both paths. ## 9. Polling, Pagination, and Concurrency @@ -995,7 +995,7 @@ Testing focuses on correctness boundaries rather than exhaustive framework behav - Worker failure before completion - Advisory locks released on session death (simulated connection kill) - Multiple pollers attempt the same source -- Container kill + Docker restart-policy recovery (manual reviewer steps — Section 15) +- Daemon API-stop semantics plus host-PID-namespace process-crash recovery (manual reviewer script — Section 15) ### End-to-end verification @@ -1018,7 +1018,7 @@ Rails 8.1 API-only app on Ruby 3.4.10; PostgreSQL; Dockerfile; Docker Compose pe `Github::SourceLock` (namespaced session advisory lock, polling-only) and `Github::RequestGate` with the lock-order invariant; **`Github::BudgetLedger` core: transactional reservation, allowance formula, startup validation, per-window bootstrap**; live + fixture transports; public + fixture event sources; protocol headers; `Github::UrlPolicy`; timeout/retry defaults; basic response classification; fixture corpus ### PR 5 — Push-event ingestion -Processor registry; tolerant `PushEvent` processor; quarantine taxonomy + canonical fingerprints; stub entity upserts with envelope mappings and distinct-event activity gating (`RETURNING id`); raw + structured persistence; idempotent insert/upsert semantics; one-shot ingestion command with contention contract and state summary; ingestion run summaries and persisted/duplicate/quarantined logging +Processor registry; tolerant `PushEvent` processor; quarantine taxonomy + canonical fingerprints; stub entity upserts with envelope mappings and distinct-event activity gating (`RETURNING id`); raw + structured persistence; conflict-safe event insertion and explicit entity merge rules; one-shot ingestion command with contention contract and state summary; ingestion run summaries and persisted/duplicate/quarantined logging ### PR 6 — Poll budget and scheduling Poll-attempt allowance enforcement; `Link`-header pagination with budget-bounded stops (no known-event stop, ETag scoped to page 1); corrected `304` debit; `effective_poll_time` with independent components; `global_blocked_until` vs derived class blocking; secondary-limit global handling; `Retry-After` handling; persisted poll state; **committed dated live-probe transcript for the 304 finding (required validation gate)** @@ -1073,7 +1073,7 @@ Must include: - Expected time before records appear — grounded in GitHub’s documented 30s–6h event latency plus the 5-minute default poll cadence, and the 30-day retention window - Fixture-based deterministic verification with exact expected counts - Rate-limit behavior: the allowance formula, the budget table, global-vs-class blocking, and per-window bootstrap -- Crash-recovery verification steps (container kills — Section 15) +- Separate API-stop and main-process-crash verification steps (Section 15) - Reset instructions - Known limitations (sampling-based enrichment coverage; no guaranteed complete capture; shared-IP budget interference) @@ -1096,11 +1096,11 @@ Keep within one to two pages — the brief is the reviewer’s primary architect - Durability boundary - **The request-budget formula and table, and the unauthenticated `304` finding** — worded precisely: the endpoint documentation contains a general statement that `304` responses do not affect the rate limit, while the REST best-practices documentation limits that exemption to correctly authorized requests; dated unauthenticated probes showed `x-ratelimit-used` increasing across a `304`; this implementation therefore budgets unauthenticated conditional requests as one request - **Enrichment as bounded best-effort sampling with per-class fairness; eligibility windows, `skipped_budget`, and distinct-event reactivation as the answer to unbounded growth** -- Idempotency and restart safety (advisory-lock ownership; outbox-style recovery; Docker restart policies) +- Duplicate-safe event persistence and restart recovery (advisory-lock ownership; outbox-style recovery; Docker restart policies) - Enrichment strategy and the SSRF boundary - Tradeoffs and assumptions (including `jsonb` semantic retention) - Intentional omissions (Extension C; authentication; complete capture) -- Future scaling path (authenticated token → 5,000 req/hr changes the enrichment story entirely; API-version upgrade to `2026-03-10` after payload re-verification) +- Future scaling path (a larger authenticated allowance could materially increase feasible coverage without guaranteeing capture or enrichment; API-version upgrade to `2026-03-10` after payload re-verification) ### Plan history @@ -1112,7 +1112,7 @@ Short ADRs for: - Solid Queue + post-commit enqueue with durable work-state reconciliation (outbox-style recovery; separate queue database; `enqueue_after_transaction_commit`) - Session advisory locks for source ownership and the global request gate (vs `FOR UPDATE` row claims; lock-order invariant) -- At-least-once processing with idempotent writes +- Repeated execution with duplicate-safe event writes and distinct-event activity gating - Event-source adapter and transport seams, each with a shipped fixture implementation - Class-aware budget ledger, allowance formula, global-vs-class blocking, and the enrichment fairness/sampling policy - `jsonb` semantic retention (not byte-exact) @@ -1130,23 +1130,21 @@ The README must provide exact steps to: 5. Query persisted push events. 6. Inspect PostgreSQL record counts. 7. Run the fixture replay scenario and confirm `duplicates_skipped > 0` in the summary — and that no skipped entity was reactivated by the replay. (Live re-runs are not relied upon to demonstrate dedup: probe-dated observations showed little or no overlap between consecutive live polls.) -8. **Verify crash recovery with actual container kills:** +8. **Verify operator-stop semantics and restart-policy crash recovery as separate paths:** ```bash -# Record existing state -docker compose exec db psql -U postgres -d github_push_ingestor_development \ - -c "SELECT COUNT(*) FROM push_events;" - -# Simulate worker failure; confirm Docker restarts it and pending work resumes -docker kill github-push-ingestor-worker-1 -docker compose ps -docker compose logs worker --since 2m - -# Simulate database failure; confirm restart and that the count is unchanged -docker kill github-push-ingestor-db-1 -docker compose ps +GITHUB_MODE=fixture docker compose up --build -d +GITHUB_MODE=fixture script/verify_recovery.sh --confirm ``` +The script exercises `db`, `web`, and `worker` twice each. Its `docker kill` path proves +that a daemon API stop leaves an `unless-stopped` container down, records the unchanged +`push_events` count, and performs the required operator recreation. Its process-crash path +uses a privileged helper in the host PID namespace to signal the container's main process; +that path must increment `RestartCount`, recover without an operator step, preserve the +event count, and retain fixture mode in the recreated worker. Neither result may be used as +evidence for the other. + 9. Restart containers normally (`docker compose restart`) and confirm existing records remain. 10. Run the full deterministic fixture scenario and compare against the documented expected counts. 11. Run tests. @@ -1160,7 +1158,7 @@ docker compose ps - Required fields are structured, typed, and `NOT NULL`; unknown payload fields tolerated; 40- and 64-char SHAs accepted - Raw payload is retained (semantic retention, documented) - **Both actor and repository enrichment demonstrably occur** within their fairness guarantees -- Duplicate ingestion is safe — and duplicate replays never reactivate skipped entities +- Duplicate event IDs cannot create another `push_events` row, and duplicate replays never reactivate skipped entities - `Link`-header pagination is handled; every fetched page fully processed - Rate-limit behavior is demonstrated: `304` quota accounting, class-aware ledger enforcement, global-vs-class blocking, per-window bootstrap, scheduling rules - Malformed data is quarantined durably per the taxonomy (canonical fingerprints, occurrence-counted) and does not terminate the batch @@ -1168,12 +1166,13 @@ docker compose ps ### Durability - PostgreSQL uses a named volume -- Docker restart policies recover crashed `db`/`web`/`worker` containers automatically (verified by container kills) +- Docker restart policies recover main-process crashes in `db`/`web`/`worker` automatically (verified by the script's host-PID-namespace crash path) +- A daemon API stop leaves an `unless-stopped` container down until operator recreation (verified independently by the script's `docker kill` path) - Application restart preserves events - Worker restart preserves pending work -- An event committed before a crash remains recoverable +- An event row committed before a crash remains stored - Advisory locks provably release on session death (tested) -- Duplicate jobs do not duplicate durable data +- The covered enrichment redelivery cannot create another entity row - Reconciliation recovers missing enrichment scheduling - The enrichment backlog is bounded (eligibility window + `skipped_budget` + distinct-event reactivation) @@ -1296,7 +1295,7 @@ A fourth review round validated the external facts (GitHub, Rails, Ruby, Solid Q | 1 | **Source lock separated from the request executor**: polling path takes `SourceLock` then the gate; enrichment takes only the gate; lock-order invariant stated; advisory keys namespaced `(SOURCE_LOCK, id)` / `(REQUEST_GATE, 1)` | Enrichment requests belong to no event source — routing them through a source lock was wrong; unnamespaced keys risk collisions | Sections 2A, 5, 8, 10 | | 2 | **Global vs class blocking split**: `global_blocked_until` stores only truly global conditions (primary exhaustion, reserve reached, secondary limits); class blocking derived from counters (`poll_used >= poll_allowance ? reset_at : nil`); separate `effective_enrichment_time`; **secondary limits block globally** (they can arise on enrichment requests, which have no source row) | One timestamp could not defer “only that class”: enrichment exhaustion would have stopped polling and vice versa — a direct contradiction in the C-round text | Sections 7, 9, 10 | | 3 | **Bootstrap = the first real poll of every window**, not an extra discovery request: window lifecycle (`uninitialized → active → globally_blocked`), counters reset per window, enrichment ineligible until initialized from authoritative headers | An extra quota-discovery request wastes budget; per-window (not just fresh-install) matters because IP co-tenants may spend immediately after each reset | Sections 7, 10 | -| 4 | **Docker restart policies added** (`unless-stopped` for `db`/`web`/`worker`, `stop_grace_period: 30s` on worker, `no` for one-shots) plus container-kill verification steps in Section 15 | Docker’s default restart policy is `no` — the durability story silently assumed restarts that would never happen | Sections 2A, 8, 15, 16 | +| 4 | **Docker restart policies added** (`unless-stopped` for `db`/`web`/`worker`, `stop_grace_period: 30s` on worker, `no` for one-shots), with the recovery runbook later corrected by Appendix E's API-stop/process-crash distinction | Docker’s default restart policy is `no` — the durability story silently assumed restarts that would never happen | Sections 2A, 8, 15, 16 | | 5 | **Entity activity gated on distinct events**: `last_seen_at`/`latest_event_at`/reactivation update only when `INSERT … RETURNING id` produces a row; duplicate replays may refresh identity fields but never reactivate | “Every observed event updates `last_seen_at`” let a replayed duplicate resurrect a `skipped_budget` entity with no new activity | Sections 5, 7, 8, 12 | | 6 | **SHA columns widened to `varchar(64)`** accepting 40- or 64-char hex | Git object names are 40 hex (SHA-1) or 64 hex (SHA-256); hard-coding 40 contradicted the tolerant-parser goal | Section 7 | | 7 | **Quarantine identity made unambiguous**: `payload_fingerprint` is the sole unique key (`github_event_id` indexed, not unique); one canonicalization definition (SHA-256 of compact UTF-8 JSON with recursively sorted keys) | Dual unique keys left an unhandled conflict path (same event ID, different malformed payload); “or equivalently normalized `jsonb`” specified two algorithms | Section 7 | @@ -1304,14 +1303,21 @@ A fourth review round validated the external facts (GitHub, Rails, Ruby, Solid Q ## Appendix E — Execution summary (2026-07-31) -**The plan body above is unchanged.** Sections 1–17 and Appendices A–D are exactly as frozen on 2026-07-29. This appendix records how the build diverged from that plan and why, as Section 14 requires at completion. +Sections 1–17 and Appendices A–D preserve the frozen architecture from 2026-07-29. A +2026-07-31 hardening amendment narrows Section 8's guarantee wording and pre-commit recovery +claim without changing behavior or architecture. This appendix records that clarification +and the implementation deltas required by Section 14. -The plan held. Every P0 story and extension shipped, no descope rung was used, and the architecture — the executor chain, the lock-order invariant, the class-aware ledger, at-least-once with idempotent writes — is what was frozen. What follows is the delta, and most of it is the plan meeting a fact it could not have known in advance. +The plan held. Every P0 story and extension shipped, no descope rung was used, and the +executor chain, lock-order invariant, class-aware ledger, event uniqueness constraint, and +distinct-event activity gate are what was frozen. What follows is the delta, and most of it +is the plan meeting a fact it could not have known in advance. | What the plan said | What was built | Why | Record | |---|---|---|---| +| Section 8 used a broad persisted-outcome equivalence and implied every uncommitted event would return on the next poll | The documented guarantee is limited to one `push_events` row per GitHub event ID and no activity/reactivation from duplicate observations; pre-commit recovery depends on the event remaining in a later sliding-feed response | Quarantine counters, run summaries, budget debits, executions, and logs may repeat, while the upstream window can advance past an uncommitted event. The old shorthand overstated both persistence and source-delivery guarantees; this is a wording correction, not an architecture change | ADR 0005; Section 8 amendment | | Section 16 gates on “plain `docker compose up --build` starts exactly `db`, `setup`, `web`, `worker`” | `web` and `worker` no longer declare a `build:`; `setup` builds the shared image and they wait on it, while the `tools` one-shots keep their own build plus `pull_policy: build` | **The clean-checkout verification found the gate was false.** Compose Bake — on by default in Docker Desktop — makes every service with a `build:` its own bake target, and targets exporting the same `image:` tag race. From a cold image the reviewer's first command failed with `image "github-push-ingestor-app:latest": already exists` and started **zero** containers. It reproduces only when the image is absent, so every prior run on a warm machine passed. This is the defect the deliverable exists to catch | `docker-compose.yml`, `spec/docker_compose_spec.rb` | -| Section 15 step 8 verifies restart policies with `docker kill` | `script/verify_recovery.sh` performs **both** the documented `docker kill` and a real in-container process crash, and reports both outcomes separately | `docker kill` is an API stop, and `restart: unless-stopped` is *defined* to skip a container the daemon recorded as manually stopped. The plan's own command cannot exercise the policy it verifies. Substituting the kill that works and staying quiet about it would have been the dishonest fix | [`docs/evidence/2026-07-31-container-kill-recovery.md`](docs/evidence/2026-07-31-container-kill-recovery.md), README “Crash recovery, verified” | +| Section 15 step 8 originally treated `docker kill` as restart-policy verification | `script/verify_recovery.sh` performs **both** the documented API stop and a host-PID-namespace main-process crash, and reports both outcomes separately | `docker kill` is an API stop, and `restart: unless-stopped` skips a container the daemon recorded as manually stopped. Only the independently observed process-crash path exercises automatic restart; substituting that path without recording the distinction would hide the original defect | [`docs/evidence/2026-07-31-container-kill-recovery.md`](docs/evidence/2026-07-31-container-kill-recovery.md), README “Crash recovery verification” | | `ENABLED_LIVE_SOURCE_COUNT` is the allowance formula's source-count input | Demoted to a **fallback**. The formula counts enabled, in-service `event_sources` rows of the running mode at window initialization and rollover, and logs `budget.source_allocation_drift` when the two disagree | A configured count that drifts from the table silently mis-sizes every allowance. Boot validation still reads no database, so the refuse-to-boot check is unchanged | ADR 0009 | | Secondary-limit backoff is “≥ 1 minute with exponential backoff when `Retry-After` is absent” | A persisted `github_api_budget.consecutive_secondary_limits` counter escalates 60 → 120 → 240s capped at one hour, **survives window rollover**, and is cleared by one clean response | Secondary limits are IP-scoped, not window-scoped. A counter that reset with the window would restart the ladder at 60s every hour against a limit that had not relented | ADR 0010 | | Section 10's fairness ladder covers the pending pool | The **TTL-refresh pool** allocates by the same prefer-then-borrow steps as the pending pool | The ladder was specified for pending candidates only, which left the refresh pool able to starve one class. Found by reading merged code against the plan rather than by a failing test | ADR 0010 | @@ -1320,7 +1326,7 @@ The plan held. Every P0 story and extension shipped, no descope rung was used, a | ADRs are written alongside the code they describe | ADR 0011 (pinned API version) and ADR 0012 (Solid Queue over Kafka) were written in PR 12 | Both are Section 14 deliverables that no implementation PR owned, because neither records a decision made *during* a PR — they record decisions made before PR 1 and never written down. PR 12 is the last opportunity, and Section 16 forbids a plan that points at documents which do not exist | ADR 0011, ADR 0012 | | The plan is the reviewer's architecture document until the brief exists | [`docs/DESIGN_BRIEF.md`](docs/DESIGN_BRIEF.md) is the reviewer's entry point; this plan is the internal execution and traceability artifact | As Section 14 intended. Stated here because the README's pointer changed with it | README “Development” | -Two things worth stating that are not divergences: +Two things worth stating after hardening: -- **No forbidden claim was ever written.** The scan in [`docs/SUBMISSION_CHECKLIST.md`](docs/SUBMISSION_CHECKLIST.md) §7 returns only negations, in every document, at every revision. +- **Guarantee wording now names only proved invariants.** Broad outcome shorthand was removed; any reference to exactly-once behavior or complete capture/enrichment is a negation or limitation. - **The 304 finding survived first-party re-verification.** PR 6's required gate re-ran the probe under `X-GitHub-Api-Version: 2022-11-28` and committed a dated transcript; `x-ratelimit-used` incremented across an unauthenticated `304`, exactly as the review-supplied evidence in Appendix A had reported. The budget arithmetic that rests on it did not have to change. diff --git a/README.md b/README.md index 4b82a8e..4452139 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ and worker crashes. - [Architecture](#architecture) - [Continuous ingestion](#continuous-ingestion) - [Processing guarantees](#processing-guarantees) -- [Crash recovery, verified](#crash-recovery-verified) +- [Crash recovery verification](#crash-recovery-verification) - [Troubleshooting and reset](#troubleshooting-and-reset) - [Known limitations](#known-limitations) - [Development](#development) @@ -58,9 +58,11 @@ unauthenticated quota** — twelve poll requests an hour at the default cadence, enrichment inside its allowance. That is the intended runtime behaviour (plan §2A); `GITHUB_MODE=fixture docker compose up --build` runs the same flow entirely offline. -The processing guarantee is stated exactly: at-least-once execution plus idempotent writes -plus unique constraints gives **effectively-once persisted outcomes**. This system does not -claim exactly-once execution. See [Processing guarantees](#processing-guarantees). +Poll observations and job deliveries may repeat. The proven write invariant is narrower: +observing the same valid event again cannot add another `push_events` row or reactivate an +entity that was skipped for budget. Executions, ingestion-run rows, quarantine occurrence +counters, budget use, and logs may repeat. This system does not claim exactly-once execution. See +[Processing guarantees](#processing-guarantees). [`docs/DESIGN_BRIEF.md`](docs/DESIGN_BRIEF.md) is the two-page architecture summary and the best place to start. The authoritative execution plan is @@ -88,8 +90,8 @@ This starts, in dependency order (plan §2A): 3. `web` — the Rails API on http://localhost:3000, started only after `setup` completes successfully 4. `worker` — the Solid Queue supervisor, started on the same condition. - **Continuous polling begins here**: its scheduler fires the 60-second tick, so - the first poll happens within a minute and every 300 seconds after that. + **Continuous polling begins here**: its scheduler nominally fires every 60 seconds; + due sources are considered on the next successful tick, subject to worker availability. Those four are the whole stack. `ingest`, `enrich`, and `test` sit behind a compose profile, so a plain `up` never starts them. @@ -121,10 +123,10 @@ budget-ledger stress specs, which open several real PostgreSQL sessions and ther their own process ([`spec/stress/README.md`](spec/stress/README.md) explains why). CI runs the same two steps. -**`--build` is not optional if you have edited anything.** The application code is baked -into the image rather than bind-mounted, so `docker compose run --rm test` against a stale -image silently tests the code the image was built from — green, and meaningless. On an -unmodified checkout the flag costs a cached no-op. +The application code is baked into the image rather than bind-mounted. The `test`, `ingest`, +and `enrich` services therefore declare `pull_policy: build`: even the assignment's bare +`docker compose run --rm test` asks Compose to build the current checkout. Adding `--build` +is harmless and makes that intent explicit, but is not required for those tool services. Rails, Active Job and application logs are one structured JSON stream; PostgreSQL and Puma startup output remain their own plain-text formats: @@ -151,140 +153,147 @@ docker compose run --rm test docker compose logs -f ``` -`IMPLEMENTATION_PLAN.md` §15 asks a reviewer to walk eleven steps. Each one below is the -command plus the result to look for; the linked section carries the explanation. +For a reproducible review, run the phases below **in order** from a fresh clone. Compose fixes +the project name to `github-push-ingestor`, so every clone on a Docker host refers to the same +`github-push-ingestor_pgdata` volume. Do not assume a fresh clone means a fresh database. -**1. Start all services.** +### Phase A — cold-image live startup -```bash -docker compose up --build # live mode — spends real quota -GITHUB_MODE=fixture docker compose up --build # the same flow, zero network -``` - -Exactly four containers come up: `db`, `setup`, `web`, `worker`. See -[Quick start](#quick-start). - -**2. Wait for health checks.** +This phase deliberately uses the default live mode and can spend unauthenticated GitHub +quota. Archive any valued global-volume data first, then remove prior project resources and +prove the volume is absent. Build without cache, remove only the local application image, and +prove that `up --build` can recreate it: ```bash +docker compose down -v --remove-orphans +if docker volume inspect github-push-ingestor_pgdata >/dev/null 2>&1; then + echo "github-push-ingestor_pgdata still exists" >&2 + exit 1 +fi +docker compose build --no-cache --pull +docker image rm github-push-ingestor-app:latest +docker compose up --build -d +docker compose ps --all until curl -fsS http://localhost:3000/health/ready; do sleep 2; done ``` -`{"status":"ok"}`. `/health/live` reports the process; `/health/ready` reports the database -and schema. Neither consumes budget. +The only services are `db` (healthy), `setup` (exited 0), `web` (healthy), and `worker` +(running); `ingest`, `enrich`, and `test` do not start. The readiness response is +`{"status":"ok"}`. `/health/live`, `/health/ready`, and `/status` read local state only and +never call GitHub or reserve budget. -**3. Run one-shot ingestion.** +A live one-shot may complete, defer until `cadence_due_at`, or report that the source is busy +because the worker owns it. Each is a valid state report: ```bash docker compose run --rm ingest +docker compose logs --since 5m worker ``` -Either a completed-run summary or a deferral — `Ingestion deferred until … — cadence_due_at` -or `source busy — poller cycle in progress`. **A deferred or busy result is a valid result**: -the state summary prints on every path and the exit code is 0, so the command always proves -system state rather than just its own outcome. See [One-shot ingestion](#one-shot-ingestion). +### Phase B — reset, then verify the fixture from an empty volume -**4. Follow application and worker logs.** +The expected fixture totals are absolute, so they are valid only after this destructive phase +boundary. Preserve any valued database first. `down -v` removes the globally named volume and +every stored event; the explicit inspection must fail before the fixture stack starts. ```bash -docker compose logs -f -docker compose logs -f worker +docker compose down -v --remove-orphans +if docker volume inspect github-push-ingestor_pgdata >/dev/null 2>&1; then + echo "github-push-ingestor_pgdata still exists" >&2 + exit 1 +fi +GITHUB_MODE=fixture docker compose up --build -d db setup web +until curl -fsS http://localhost:3000/health/ready; do sleep 2; done ``` -One structured JSON stream. See [Logs](#logs). - -**5. Query persisted push events.** +Starting only `db`, `setup`, and `web` keeps the continuous worker from racing the one-shot +fixture commands. Capture the one-shot output directly: `docker compose run --rm` removes its +container, so there is no `ingest` service log to inspect afterward. ```bash -curl -s 'http://localhost:3000/api/push_events?limit=5' | jq -curl -s http://localhost:3000/api/push_events/ | jq .data.raw_payload -``` - -The list nests each event's actor and repository with their `enrichment_status`; the -detail endpoint returns the retained raw payload. See -[Inspecting the data](#inspecting-the-data). - -**6. Inspect PostgreSQL record counts.** +set -o pipefail +fixture_ingest_output="$(mktemp)" +GITHUB_MODE=fixture docker compose run --rm ingest 2>&1 | tee "$fixture_ingest_output" +grep -E 'Push events created:[[:space:]]+4' "$fixture_ingest_output" +grep -E 'Events quarantined:[[:space:]]+3' "$fixture_ingest_output" +grep -E 'Non-push events ignored:[[:space:]]+1' "$fixture_ingest_output" -```bash +GITHUB_MODE=fixture docker compose run --rm enrich --limit 6 docker compose exec db psql -U postgres -d github_push_ingestor_development -c " SELECT (SELECT COUNT(*) FROM push_events) AS push_events, (SELECT COUNT(*) FROM github_actors) AS actors, (SELECT COUNT(*) FROM github_repositories) AS repositories, (SELECT COUNT(*) FROM quarantined_events) AS quarantined, - (SELECT SUM(occurrence_count) FROM quarantined_events) AS occurrences;" + (SELECT SUM(occurrence_count) FROM quarantined_events) AS occurrences; + SELECT 'actor' AS class, enrichment_status, COUNT(*) FROM github_actors GROUP BY 2 + UNION ALL + SELECT 'repository', enrichment_status, COUNT(*) FROM github_repositories GROUP BY 2;" ``` -More queries in [Database inspection](#database-inspection); what each table means is in -[The data model](#the-data-model). +The first query must return `4 / 3 / 3 / 3 / 3`. The status query must return +`complete 2 / permanent_failure 1` for each entity class. -**7. Run the fixture replay and confirm duplicates are absorbed.** +To verify replay behavior, wait for the fixture's poll floor and capture the second command's +output too. The removed one-shot container cannot be queried with `docker compose logs`. ```bash -GITHUB_MODE=fixture docker compose run --rm ingest -sleep 60 # GitHub's X-Poll-Interval floor, which --force deliberately does not bypass -GITHUB_MODE=fixture docker compose run --rm ingest --force +sleep 60 +fixture_replay_output="$(mktemp)" +GITHUB_MODE=fixture docker compose run --rm ingest --force 2>&1 | tee "$fixture_replay_output" +grep -E 'Duplicates skipped:[[:space:]]+4' "$fixture_replay_output" +if grep -q 'enrichment.reactivated' "$fixture_replay_output"; then + echo "duplicate replay reactivated an entity" >&2 + exit 1 +fi +rm -f "$fixture_ingest_output" "$fixture_replay_output" ``` -Two things to confirm, not one. `Duplicates skipped: 4` in the second summary — and that -**no skipped entity was reactivated**. The second is falsifiable: `enrichment.reactivated` -is emitted only on a real reactivation, so it should be absent, and the `skipped_budget` -count should be unchanged. +Run the isolated suite; `pull_policy: build` means this bare assignment command builds the +current source before it runs: ```bash -docker compose logs ingest | grep enrichment.reactivated # expect no output -docker compose exec db psql -U postgres -d github_push_ingestor_development -c " - SELECT COUNT(*) FROM github_actors WHERE enrichment_status = 'skipped_budget';" +docker compose run --rm test ``` -See [Deterministic fixture verification](#deterministic-fixture-verification). +### Phase C — recovery and persistence -**8. Verify crash recovery with actual container kills.** +Reset once more so recovery starts from a deterministic fixture database, then start the +full offline stack. The script keeps every recreation in fixture mode and asserts that the +worker remains there: ```bash +docker compose down -v --remove-orphans GITHUB_MODE=fixture docker compose up --build -d -script/verify_recovery.sh --confirm +GITHUB_MODE=fixture script/verify_recovery.sh --confirm +docker compose exec worker printenv GITHUB_MODE # fixture ``` -Read §15's literal `docker kill` command knowing what it does: it is an API stop, and -`restart: unless-stopped` is *defined* to skip exactly that case, so the documented command -leaves the container down. The script performs both that and a real process crash, and -reports both. See [Crash recovery, verified](#crash-recovery-verified). +`docker kill` is an API-requested stop, so it is the negative control: `unless-stopped` leaves +that container down. The script separately kills each container's main process from the host +PID namespace; that is the crash path on which the policy restarts `db`, `web`, and `worker`. +It also requires `push_events` to remain unchanged after every crash and recovery. -**9. Restart normally and confirm records remain.** +Finally, compare the count around both a normal restart and a Compose down/up cycle. Prefix +every recreation with fixture mode so the worker never spends live quota: ```bash docker compose exec db psql -U postgres -d github_push_ingestor_development \ - -c "SELECT COUNT(*) FROM push_events;" + -Atc "SELECT COUNT(*) FROM push_events;" docker compose restart docker compose exec db psql -U postgres -d github_push_ingestor_development \ - -c "SELECT COUNT(*) FROM push_events;" -``` - -Identical counts. The volume is named; only `down -v` removes it. - -**10. Run the full deterministic fixture scenario.** - -```bash -GITHUB_MODE=fixture docker compose run --rm ingest -GITHUB_MODE=fixture docker compose run --rm enrich --limit 6 -``` - -From an empty database: **4 push events created, 3 quarantined, 1 ignored, 3 actors, 3 -repositories** — then `complete 2 / permanent_failure 1` in each entity class. Eleven more -scenarios are in the [fixture scenario matrix](#fixture-scenario-matrix). - -**11. Run tests.** - -```bash -docker compose run --rm test + -Atc "SELECT COUNT(*) FROM push_events;" +docker compose down --remove-orphans +GITHUB_MODE=fixture docker compose up --build -d +docker compose exec db psql -U postgres -d github_push_ingestor_development \ + -Atc "SELECT COUNT(*) FROM push_events;" ``` -Two `rspec` invocations against isolated `*_test` databases, the same two CI runs. On an -unmodified checkout that is the whole story; **if you have edited anything, add `--build`** -— the code is baked into the image, not mounted. +All three values must be identical. Plain `down` preserves the named volume; only `down -v` +deletes it. See [Crash recovery verification](#crash-recovery-verification) for the +API-stop/process-crash distinction. -A transcript of this whole walk, run from a fresh clone on a machine with no image, is at +A historical transcript of an earlier clean-checkout walk, run from a fresh clone on a +machine with no image, is at [`docs/evidence/2026-07-31-clean-checkout-verification.md`](docs/evidence/2026-07-31-clean-checkout-verification.md) — including the live half, which is what shows the public API works with no token. @@ -362,7 +371,21 @@ to do it. ## Deterministic fixture verification Fixture mode resolves every request inside [`fixtures/github/`](fixtures/github/) -with no network at all, so the numbers are exact: +with no network at all. The numbers below are absolute and require an empty volume; a fresh +checkout is not sufficient because Compose's fixed project name makes the volume global to +the Docker host. Preserve any valued data, then establish the fixture-only baseline: + +```bash +docker compose down -v --remove-orphans +if docker volume inspect github-push-ingestor_pgdata >/dev/null 2>&1; then + echo "github-push-ingestor_pgdata still exists" >&2 + exit 1 +fi +GITHUB_MODE=fixture docker compose up --build -d db setup web +``` + +The worker is intentionally absent so it cannot consume the fixture before the one-shot. +Now the numbers are exact: ```bash GITHUB_MODE=fixture docker compose run --rm ingest @@ -407,7 +430,8 @@ Once past it, nothing is created: the same page is absorbed as **4 duplicates**, three quarantine rows stay three rows with their occurrence counts at 2, and no entity's activity moves — and **no skipped entity is reactivated**, which is the half of plan §7's merge rules a re-polled window would otherwise break. Re-running -ingestion is safe at any frequency — see +ingestion cannot duplicate accepted event rows or reactivate a skipped entity from the +same event ID; run summaries, quarantine counters, budget use, and logs may still change. See [ADR 0005](docs/adr/0005-at-least-once-with-idempotent-writes.md). ### Enrichment, offline @@ -416,7 +440,6 @@ The same corpus resolves every entity the page referenced, so the whole flow — persist, stub, enrich — runs with no network: ```bash -GITHUB_MODE=fixture docker compose run --rm ingest GITHUB_MODE=fixture docker compose run --rm enrich --limit 6 ``` @@ -656,9 +679,11 @@ Seven business tables in three groups: - **The global ledger** — `github_api_budget`, one row. A second database, `github_push_ingestor_queue_development`, holds Solid Queue's tables. -That is job state, not business state: deleting all of it loses nothing, because pending -work is rebuilt from committed entity rows -([ADR 0008](docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md)). +Queued enrichment dispatches are hints, not the pending-enrichment source of truth: removing +those hints does not erase eligible entity state committed in the business database, and a +later successful reconciler tick can rediscover that work. Other operational queue state is +not claimed to be reconstructible. See +[ADR 0008](docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md). | Table | What a row is | Identity | Written by | |---|---|---|---| @@ -730,7 +755,7 @@ limits are IP-scoped rather than window-scoped but never reactivates a skipped entity — that is the half of §7's merge rules a re-polled window would otherwise break. -### The idempotency contract +### Replay behavior by table | Table | Write | Effect on replay | |---|---|---| @@ -763,8 +788,9 @@ truth: docker compose exec db psql -U postgres -d github_push_ingestor_development -c "\d+ push_events" ``` -The entity-relationship diagram is Figure 2 of -[`docs/DESIGN_BRIEF.md`](docs/DESIGN_BRIEF.md). +The compact architecture figure in +[`docs/DESIGN_BRIEF.md`](docs/DESIGN_BRIEF.md) shows how the runtime and PostgreSQL state +fit together. ## Logs @@ -1087,7 +1113,7 @@ There is deliberately no variable for the API host or the API version — see ## Architecture [`docs/DESIGN_BRIEF.md`](docs/DESIGN_BRIEF.md) is the two-page architecture summary, with -rendered request-path and data-model figures. This section is the annotated call chain. +one compact architecture figure. This section is the annotated call chain. Every live GitHub request — polling and enrichment, from the poller, the worker, or the one-shot — takes one chain, and nothing outside it calls GitHub @@ -1234,7 +1260,7 @@ and debited twice. Decisions behind this are recorded in [`docs/adr/`](docs/adr/): `jsonb` semantic retention (0001), advisory locks and the gate (0002), the source and transport seams (0003), the class-aware ledger (0004), -at-least-once processing with idempotent writes (0005), decomposed poll deferral state +repeated execution with duplicate-safe event writes (0005), decomposed poll deferral state (0006), enrichment fairness shares and borrowing (0007), post-commit enqueue with entity-scoped reconciliation (0008), runtime source allocation with shared-IP observability (0009), secondary-limit escalation with refresh-pool fairness (0010), the @@ -1251,19 +1277,20 @@ threads from [`config/queue.yml`](config/queue.yml), and a scheduler running inside its advisory lock and applies §9's five components before spending anything. At the default 300-second cadence roughly four ticks in five cost one indexed `SELECT` and nothing else. The tick exists so a source that becomes due at T is -polled within a minute of T — not so that polls happen every minute, which the -allowance formula (twelve poll requests an hour, no headroom) could not pay for. +eligible on the next successful scheduled tick — not so that polls happen every minute, +which the allowance formula (twelve poll requests an hour, no headroom) could not pay for. A source another process is polling is reported at INFO and left alone; the tick -never retries it, because the next tick is 60 seconds away. +does not retry it in the same execution; another tick is nominally scheduled 60 seconds later. **Enrichment is scheduled twice over, deliberately.** A run that created events schedules one cycle per class as soon as its rows are committed and its advisory lock is released. That enqueue is a *hint*: the durable record of pending work is the entity rows themselves, so `ReconcilePendingEnrichmentsJob` sweeps them every 60 seconds and schedules a cycle for any class that has claimable work and is not -blocked by the ledger. Work committed before a crash but never enqueued is -rediscovered on the next tick — no operator step, no cleanup job, and no queue -inspection (plan §8, [ADR 0008](docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md)). +blocked by the ledger. If a crash loses an enrichment-dispatch hint, the committed eligible +entity state remains discoverable on a later successful scheduled tick without a special +cleanup job or queue inspection (plan §8, +[ADR 0008](docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md)). Each cycle enriches at most one entity, chosen by §10's fairness policy under a lease, so a backlog of ninety pending actors is one queued job rather than ninety. @@ -1273,21 +1300,21 @@ requests an hour split between the two classes. ## Processing guarantees **An event is accepted when its `push_events` row commits** — not when GitHub returns it, -not when a job is enqueued, not when a log line says so. That commit is the durability -boundary; everything upstream of it is retryable and everything downstream is derived. +not when a job is enqueued, not when a log line says so. That row is durable. Work lost +before its commit is recoverable only while the event remains in a later GitHub feed window; +pending enrichment can be reconstructed from committed entity state. -```text -At-least-once execution -+ Idempotent writes -+ Unique constraints -= Effectively-once persisted outcomes -``` +The demonstrated invariant is deliberately specific: a duplicate observation of a valid +event cannot create another `push_events` row, and cannot reactivate an entity already in +`skipped_budget`. That does not make the surrounding execution singular. A retry may create +another ingestion-run row, increment a malformed payload's occurrence counter, spend another +budget reservation, execute another job, or emit another log line, depending on its crash +boundary. ### What is not claimed, and why -- **Not exactly-once execution.** A job may run twice; the second run changes nothing - durable. Side effects that are not idempotent writes — a log line, a duration metric — - can and will repeat. +- **Not exactly-once execution.** A job may run twice. The unique event row and guarded entity + activity update are replay-safe; execution records, counters, budget use, and logs may repeat. - **Not complete upstream capture.** The feed is a sliding window with hours of latency; see [Known limitations](#known-limitations). - **Not complete enrichment coverage.** Demand exceeds the hourly budget by roughly fifty @@ -1297,10 +1324,10 @@ At-least-once execution | Crash point | What survives | What recovers it | |---|---|---| -| Before the event commits | Nothing from this page | The next overlapping poll re-fetches it — there is no stop-on-known-event | -| After the event commits, before enrichment is enqueued | The `push_events` row and its stub entities | `ReconcilePendingEnrichmentsJob`, from committed entity rows, within 60s | +| Before the event commits | Nothing from this page | A later overlapping poll can re-fetch it only while it remains in GitHub's feed window; there is no stop-on-known-event | +| After the event commits, before enrichment is enqueued | The `push_events` row and its stub entities | `ReconcilePendingEnrichmentsJob`, from committed entity rows, on a later successful tick (scheduled every 60s) | | Worker dies before the enrichment commit | The entity row, still `pending`, its lease expiring by arithmetic | The next reconcile tick after `next_retry_at` passes | -| Worker dies after the enrichment commit, before acknowledgement | The enriched entity row | Solid Queue re-runs the job; the re-run is a no-op | +| Worker dies after the enrichment commit, before acknowledgement | The enriched entity row | Solid Queue may re-run the job; the freshness check leaves that row unchanged | ### The mechanisms @@ -1319,20 +1346,22 @@ At-least-once execution [ADR 0005](docs/adr/0005-at-least-once-with-idempotent-writes.md) argues the choice; [ADR 0008](docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md) covers -recovery; [Crash recovery, verified](#crash-recovery-verified) demonstrates it. +recovery; [Crash recovery verification](#crash-recovery-verification) exercises it. -## Crash recovery, verified +## Crash recovery verification -`IMPLEMENTATION_PLAN.md` §15 step 8 asks a reviewer to kill containers and watch them come -back. Run it with one command, against a stack started in fixture mode: +`IMPLEMENTATION_PLAN.md` §15 step 8 asks a reviewer to exercise both operator-requested stops +and main-process crashes. Run it with one command against a stack started in fixture mode: ```bash GITHUB_MODE=fixture docker compose up --build -d -script/verify_recovery.sh --confirm +GITHUB_MODE=fixture script/verify_recovery.sh --confirm ``` -A dated transcript of that run is committed at +A historical transcript from an earlier script revision is committed at [`docs/evidence/2026-07-31-container-kill-recovery.md`](docs/evidence/2026-07-31-container-kill-recovery.md). +Its prominent erratum explains why its green verdict is not the current submission gate; the +corrected script must be rerun against the final default-branch SHA. Nothing in CI runs the script — it kills containers and writes to the development databases, and `spec/docker_compose_spec.rb` asserts that no workflow references it. @@ -1343,7 +1372,7 @@ that case. So the documented command leaves the container down, and ```bash docker kill github-push-ingestor-worker-1 docker compose ps # Exited (137) — and it stays that way -docker compose up -d worker +GITHUB_MODE=fixture docker compose up -d worker ``` is the honest sequence. The restart policy is sound; it is the *kill* that does not exercise @@ -1355,15 +1384,13 @@ One reading tip for §15 step 8's `docker compose ps` output: `web`'s container curls `/health/live`, which never touches the database, so `web` stays green throughout a `db` kill. `/health/ready` is the observable that flips. -**The stack comes back in live mode, so put it back offline when you are done.** Bringing a -killed container back means `docker compose up -d `, which recreates it from the -*current* shell environment — and `GITHUB_MODE` defaults to `live` there. A reviewer who -started the stack with `GITHUB_MODE=fixture` and then ran the script ends up with a worker -polling real GitHub and spending real quota. Check it and reset it: +The recovery script passes fixture mode to every internal Compose recreation and asserts the +worker's mode after recreation and again before exit. When performing a recovery manually, +carry the mode on every `docker compose up` command and verify it before watching the worker: ```bash -docker compose exec worker printenv GITHUB_MODE # `live` after a recovery run GITHUB_MODE=fixture docker compose up -d --force-recreate worker +docker compose exec worker printenv GITHUB_MODE # fixture ``` Recovery is also watchable by hand in under a minute — stop the worker, put the entities back @@ -1379,8 +1406,9 @@ docker compose start worker docker compose logs -f worker # enrichment.dispatched, then enrichment.completed ``` -Emptying the queue loses nothing, which is the point: the work is rebuilt from committed -entity rows. +In this scenario, emptying the queue does not discard pending enrichment state: that work is +rebuilt from committed entity rows. This does not claim that arbitrary operational queue +state can be reconstructed. ## Troubleshooting and reset @@ -1468,13 +1496,19 @@ broken system. **Level 3 — destroy everything, including the volume.** ```bash -docker compose down -v +docker compose down -v --remove-orphans +if docker volume inspect github-push-ingestor_pgdata >/dev/null 2>&1; then + echo "github-push-ingestor_pgdata still exists" >&2 + exit 1 +fi docker compose up --build ``` -`-v` deletes the named `pgdata` volume and **every stored event**. Plain -`docker compose down` does not: it stops and removes the containers and leaves the volume -intact, which is why step 9 of the verification walk finds identical counts afterwards. +Because the Compose project name is fixed, `pgdata` resolves to the globally named +`github-push-ingestor_pgdata`; every clone on the same Docker host refers to it. `-v` deletes +that volume and **every stored event**. Plain `docker compose down` does not: it stops and +removes the containers and leaves the volume intact, which is why the persistence check finds +identical counts afterward. ## Known limitations @@ -1490,7 +1524,7 @@ samples the public feed rather than mirroring it.** The eight limitations that follow are consequences of that, and of the 60-request hourly ceiling. None is a gap to be closed later; each is a stated boundary. -**1. Enrichment coverage is a sample, not coverage.** One observed live page held ~92–95 +**1. Enrichment is sampled, not exhaustive.** One observed live page held ~92–95 `PushEvent` records with ~89 distinct actors and ~92 distinct repositories — 181 cold entity requests per page, ~2,172 an hour at twelve polls, against 40 available. That is roughly 1.8% theoretical cold coverage. `skipped_budget` is a normal documented outcome, @@ -1516,9 +1550,9 @@ count, but a second *live* source is a documented seam rather than shipped behav key order, and duplicate keys are lost; array order is preserved because it is meaningful ([ADR 0001](docs/adr/0001-jsonb-semantic-retention.md)). -**6. No authentication.** A token would raise the ceiling from 60 to 5,000 requests an -hour and change the enrichment story entirely — which is the point of the constraint, not -an oversight. See the scaling path in +**6. No authentication.** A larger authenticated budget could materially increase feasible +coverage, but would not make upstream capture or enrichment complete. The unauthenticated +constraint is deliberate, not an oversight. See the scaling path in [`docs/DESIGN_BRIEF.md`](docs/DESIGN_BRIEF.md). **7. An enriched entity can be up to 24 hours stale.** `ACTOR_REFRESH_TTL_SECONDS` and diff --git a/app/controllers/api/push_events_controller.rb b/app/controllers/api/push_events_controller.rb index 99d187b..2888b8b 100644 --- a/app/controllers/api/push_events_controller.rb +++ b/app/controllers/api/push_events_controller.rb @@ -22,7 +22,7 @@ def index # ambiguous — real GitHub event ids are numeric strings, so only one reading can be # right. §7 keeps the surrogate key out of this application's identity vocabulary # entirely (even the foreign keys target github_id), github_event_id is the unique index - # the whole idempotency story rests on, and it is the identifier §11 puts on every log + # that prevents a second event row, and it is the identifier §11 puts on every log # line — which is what makes "log line -> record" a URL a reviewer can type. # # find_by! rather than find_by plus an explicit render: ApplicationController maps diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb index 559dbde..6a182ca 100644 --- a/app/jobs/application_job.rb +++ b/app/jobs/application_job.rb @@ -12,7 +12,7 @@ # uncoordinated Active Job ladder would re-poll a source whose backoff was just written and # spend the hourly allowance twice on the same failure. The 60-second recurring tick is the # retry — an escaped exception is a defect, so it fails the execution, job.failed says why, -# and the next tick starts from committed state. +# and a later successful scheduled tick starts from committed state. # # No discard_on ActiveJob::DeserializationError either: no job here takes a record argument. class ApplicationJob < ActiveJob::Base diff --git a/app/jobs/reconcile_pending_enrichments_job.rb b/app/jobs/reconcile_pending_enrichments_job.rb index 4f02b27..864e7e5 100644 --- a/app/jobs/reconcile_pending_enrichments_job.rb +++ b/app/jobs/reconcile_pending_enrichments_job.rb @@ -3,10 +3,9 @@ # seconds. # # **This is the crash-recovery mechanism, and its input is committed entity state.** The -# post-commit enqueue in Github::IngestionRunner is a hint; a process killed between the -# COMMIT and the enqueue, a worker that was down for an hour, a job that failed permanently -# — all of them lose the hint, and none of them lose the work, because the entity rows still -# say `pending` and the partial index that answers that predicate has existed since PR 3. +# post-commit enqueue in Github::IngestionRunner is a hint. If a crash loses that hint while +# the entity row remains eligible and `pending`, a later successful scheduled tick can find +# it through the partial index that has answered that predicate since PR 3. # # **Entity-scoped, structurally.** It reads github_actors, github_repositories and one # github_api_budget row through Github::Enrichment::Dispatch, and never push_events. §8's diff --git a/app/models/push_event.rb b/app/models/push_event.rb index bc58fb8..f8445f3 100644 --- a/app/models/push_event.rb +++ b/app/models/push_event.rb @@ -27,8 +27,8 @@ class PushEvent < ApplicationRecord format: { with: SHA_FORMAT, message: "must be 40 or 64 hexadecimal characters" } - # The idempotent insert the whole durability story rests on (§7, §8). Returns the - # new row's id, or nil when the event was already persisted — and the caller uses + # The duplicate-event insert gate the accepted-row guarantee rests on (§7, §8). Returns + # the new row's id, or nil when the event was already persisted — and the caller uses # exactly that distinction to decide whether entity activity may be updated, so a # re-polled window cannot resurrect skipped entities. # diff --git a/app/services/github.rb b/app/services/github.rb index c793de7..22fdfa9 100644 --- a/app/services/github.rb +++ b/app/services/github.rb @@ -12,7 +12,7 @@ # # Configuration is memoised per process and cleared by config/initializers/github.rb # on every reload, so a development edit takes effect without a restart while a -# production process reads the environment exactly once. +# production process retains its first environment-derived configuration. module Github class << self def configuration diff --git a/app/services/github/budget_ledger.rb b/app/services/github/budget_ledger.rb index 0eb6619..70c14f2 100644 --- a/app/services/github/budget_ledger.rb +++ b/app/services/github/budget_ledger.rb @@ -488,9 +488,8 @@ def debit!(request_class, now:, share_cap:, borrow:) # # Emitted from the debit that *reached* the allowance, not from denial_reason. A # denial recurs on every attempt — under PR 8's recurring task that is a line a minute - # for the rest of the window, burying the stream §11 explicitly sizes. The debit that - # takes used to allowance happens exactly once per class per window, which is what a - # transition means. + # for the rest of the window, burying the stream §11 explicitly sizes. Only the debit + # that first takes used to allowance represents the class transition for that window. def log_class_exhausted(budget, request_class) used = class_used(budget, request_class) allowance = class_allowance(budget, request_class) @@ -511,8 +510,8 @@ def log_class_exhausted(budget, request_class) # # Two edges worth naming. With a guarantee of zero the INFO never fires — the # post-debit share is 1 and never 0 — which is correct, because nothing transitioned: - # that class was borrowing from its first request. And under a sustained borrow the - # INFO still fires exactly once, at the guarantee, with budget.class_exhausted + # that class was borrowing from its first request. Under a sustained borrow, the INFO + # fires on the debit that first reaches the guarantee, with budget.class_exhausted # following later at the class cap. def log_share_transitions(budget, request_class, borrow:) return unless enrichment?(request_class) diff --git a/app/services/github/enrichment/dispatch.rb b/app/services/github/enrichment/dispatch.rb index b743f1e..3ac0026 100644 --- a/app/services/github/enrichment/dispatch.rb +++ b/app/services/github/enrichment/dispatch.rb @@ -7,8 +7,9 @@ module Enrichment # "is there durable enrichment work this class could do right now?" — and the answer is # read from the committed entity rows and the ledger, never from the queue. That is what # makes the enqueue a *hint*: §2A's outbox-style recovery says "the committed entity - # state is the durable record of pending work", so a process killed between the COMMIT - # and the enqueue loses the hint and never the work. + # state is the durable record of pending work". A process killed between the COMMIT and + # enqueue can lose that hint while leaving eligible pending entity state discoverable by + # a later successful reconciler tick. # # **At most one job per class per call**, however deep the backlog. §5 gives each class # one job and Github::EnrichmentRunner enriches at most one entity per call, so the diff --git a/app/services/github/ingestion_runner.rb b/app/services/github/ingestion_runner.rb index 169e5a9..48d9bdf 100644 --- a/app/services/github/ingestion_runner.rb +++ b/app/services/github/ingestion_runner.rb @@ -48,8 +48,8 @@ module Github # jobs being class-scoped: the runner chooses the entity by fairness, so N enqueues carry # no more information than one, and §8's property that matters — the enqueue happens after # the rows are durable — is stronger here, where every commit of the run is behind us. - # Losing the dispatch to a crash loses nothing: ReconcilePendingEnrichmentsJob sweeps the - # same committed state every 60 seconds. + # If a crash loses this dispatch hint, the committed pending entity state remains. + # ReconcilePendingEnrichmentsJob can rediscover eligible work on a later successful tick. class IngestionRunner MONOTONIC = -> { Process.clock_gettime(Process::CLOCK_MONOTONIC) } diff --git a/app/services/inspection/push_event_view.rb b/app/services/inspection/push_event_view.rb index c645e94..c09592e 100644 --- a/app/services/inspection/push_event_view.rb +++ b/app/services/inspection/push_event_view.rb @@ -3,8 +3,8 @@ module Inspection # # A module of functions rather than a method on PushEvent, following the convention this # codebase already holds twenty times over: there are twenty #to_log methods and not one - # of them lives on a model. A model owns its columns, its constraints and its idempotent - # write; how a row is shaped for a reader belongs to the reader — and there are two + # of them lives on a model. A model owns its columns, constraints, and duplicate-event + # insert gate; how a row is shaped for a reader belongs to the reader — and there are two # readers here whose answers deliberately differ, so a single #to_api would need a mode # flag on the model. # diff --git a/config/application.rb b/config/application.rb index 10acc84..5146ed6 100644 --- a/config/application.rb +++ b/config/application.rb @@ -76,9 +76,9 @@ class Application < Rails::Application # long for in-flight work and no longer. docker-compose.yml pairs it with # stop_grace_period: 30s, leaving margin for the supervisor to reap its children. # - # A job still *waiting* for the gate has reserved nothing and is safe to kill: §8's - # at-least-once execution with idempotent writes is what makes that true, and both - # advisory locks die with the session. + # A job still *waiting* for the gate has not reserved budget, so terminating it cannot + # leak a debit. A redelivery re-enters the normal selector, and both advisory locks die + # with the session. config.solid_queue.shutdown_timeout = 20.seconds end end diff --git a/config/recurring.yml b/config/recurring.yml index bf8374d..71fb780 100644 --- a/config/recurring.yml +++ b/config/recurring.yml @@ -4,14 +4,15 @@ # PollEventSourceJob computes effective_poll_time and no-ops unless a poll is due". A tick # is not a poll: against the default POLL_INTERVAL_SECONDS (300) roughly four ticks in five # find nothing due and cost one indexed SELECT. That ratio is the point — it is what makes a -# source that becomes due at T get polled within a minute of T, without a cadence that the -# allowance formula (12 poll requests an hour, zero headroom) cannot pay for. +# source that becomes due at T become eligible on the next successful scheduled tick, +# without a cadence that the allowance formula (12 poll requests an hour, zero headroom) +# cannot pay for. # # reconcile_pending_enrichments — §8 step 11, the sweep behind the outbox-style recovery. # The post-commit enqueue in Github::IngestionRunner is only a hint; a SIGKILL between the -# COMMIT and the enqueue loses the hint and never the work, because the committed entity -# rows are the durable record. This task rediscovers that work, entity-scoped, and enqueues -# nothing when there is none or when the ledger says the class is spent. +# COMMIT and the enqueue can lose the hint while the committed pending entity state remains. +# On a later successful tick, this task can rediscover eligible work from that state and +# enqueues nothing when there is none or when the ledger says the class is spent. # # The cadences live here rather than in an environment variable because §9's cadence is # POLL_INTERVAL_SECONDS and it is enforced in the scheduling components, not in the tick. diff --git a/docker-compose.yml b/docker-compose.yml index 7a56319..31fc57d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -133,10 +133,9 @@ services: # stop_grace_period pairs with SolidQueue.shutdown_timeout (20s, set in # config/application.rb from §2A's pinned HTTP timeouts): 20s for an in-flight # attempt that already holds the request gate, and 10s of margin for the - # supervisor to reap its children. A job still *waiting* for the gate has - # reserved nothing and is safe to kill — §8's at-least-once execution with - # idempotent writes is what makes that true, and both advisory locks die with - # the session. + # supervisor to reap its children. A job still *waiting* for the gate has not + # reserved budget, so terminating it cannot leak a debit; a redelivery re-enters + # the normal selector, and both advisory locks die with the session. # # No healthcheck. Nothing depends on this service, `unless-stopped` already # covers process death, and the only probe that could tell a hung worker from diff --git a/docs/DESIGN_BRIEF.md b/docs/DESIGN_BRIEF.md index bb92725..2c7fe75 100644 --- a/docs/DESIGN_BRIEF.md +++ b/docs/DESIGN_BRIEF.md @@ -1,303 +1,128 @@ # Design brief — github-push-ingestor -A Rails 8.1 API-only service that polls GitHub's public Events API, persists `PushEvent` -records with their raw payloads in PostgreSQL, and enriches the actors and repositories -they reference — all without a token, inside an unauthenticated ceiling of 60 requests per -hour per IP. +This Rails 8.1 API service samples GitHub's public Events API, stores `PushEvent` +records and their raw payloads in PostgreSQL, and enriches referenced actors and +repositories without a token. The README is the runbook; this brief explains the +decisions. Detailed arguments live in the [ADRs](adr/), and execution history lives in +[`IMPLEMENTATION_PLAN.md`](../IMPLEMENTATION_PLAN.md). -> **The README says how. This brief says why. The ADRs hold the argument; this brief holds -> the decision.** +## Business problem and constraint -Two pages is a deliberate budget: the figures and tables carry what prose would otherwise -spend, and every "why" is one sentence plus a link. The execution and traceability artifact -is [`IMPLEMENTATION_PLAN.md`](../IMPLEMENTATION_PLAN.md) — its pre-implementation revision -history lives in Git and in its Appendices A–D, and Appendix E records how the build -diverged from it. +The assignment asks for durable ingestion, enrichment, and restart safety. The difficult +constraint is the source: `/events` is a sliding window with documented delivery latency, +while unauthenticated callers share 60 requests per hour per outbound IP. An event can +leave the window before this service observes it, and enrichment demand can exceed the +remaining budget by orders of magnitude. The product is therefore an observable, +bounded sampler, not a mirror. It makes no guarantee of complete upstream capture or +complete enrichment. -## The problem - -GitHub publishes a firehose of public activity at `/events`. The assignment is to ingest -the `PushEvent` slice of it durably, enrich it with actor and repository detail, and -survive restarts. - -The hard part is not volume. It is that the source is a **sliding window behind a hard -quota**. The feed retains roughly the last 300 events with a documented 30-second-to-6-hour -delivery latency and a 30-day retention window, and an unauthenticated caller gets 60 -requests per hour keyed to its outbound IP — shared with anything else behind that IP. -Every design decision here falls out of that arithmetic: what to poll, how often, what to -enrich, what to admit cannot be done. - -So this system is honest about being a **sampler, not a mirror**. It does not claim -complete upstream capture, and it does not claim complete enrichment coverage. What it does -claim, it proves — with a test, a constraint, or a dated transcript. - -## Architecture - -Two request paths — polling and enrichment — share one executor, and nothing outside that -chain calls GitHub. Polling takes a per-source advisory lock; enrichment never does. A -second, global advisory lock makes outbound concurrency exactly one, application-wide, so -the budget ledger is never racing itself. The lock order (source, then gate, never the -reverse) is enforced at runtime by `Github::LockOrder`, not merely documented. +## Architecture, data, and durability ```mermaid flowchart LR - subgraph oneshot["one-shot commands"] - BI["bin/ingest"] - BE["bin/enrich"] + subgraph runtime["Docker Compose runtime"] + P["Poll job / bin/ingest"] --> I["IngestionRunner
source lock"] + E["Enrichment jobs / bin/enrich"] --> N["EnrichmentRunner
fair selection + lease"] + W["Web
health · status · event API"] + Q["Solid Queue"] --> P + Q --> E end - subgraph worker["worker container — Solid Queue supervisor"] - PJ["PollEventSourceJob — 60s tick"] - RJ["ReconcilePendingEnrichmentsJob — 60s tick"] - EJ["EnrichActorJob / EnrichRepositoryJob"] - end - subgraph web["web container — read-only, never calls GitHub"] - API["/health/live · /health/ready · /status · /api/push_events"] - end - IR["Github::IngestionRunner
holds SourceLock for the whole cycle"] - ER["Github::EnrichmentRunner
age-out · fairness · lease · one entity"] - RX["Github::RequestExecutor
RequestGate → BudgetLedger → UrlPolicy → Transport"] - GH["api.github.com
unauthenticated · 60 req/hr per IP"] - subgraph pg["db container — PostgreSQL 16 on the pgdata named volume"] - APPDB[("app database
7 business tables")] - QDB[("queue database
Solid Queue")] - end - BI --> IR - PJ --> IR - BE --> ER - EJ --> ER - RJ -.->|"enqueues"| EJ - IR --> RX - ER --> RX - RX --> GH - IR -->|"PageWriter · PollState"| APPDB - ER -->|"EntityState"| APPDB - API -->|"read only"| APPDB - APPDB -.->|"post-commit enqueue"| QDB - QDB --> PJ - QDB --> EJ -``` - -Every path to GitHub funnels through one `RequestExecutor` node — that single arrow is the -whole rate-limit design. The `web` container has no arrow to GitHub at all, which is why -`/status` and `/health/*` can never spend budget: it is structural, not a promise. - -## Data model - -Seven business tables in three groups: source and run state (`event_sources`, -`ingestion_runs`), business records (`push_events`, `github_actors`, -`github_repositories`, `quarantined_events`), and one global ledger (`github_api_budget`). -Raw payloads are `jsonb` and retention is **semantic, not byte-exact** — content-equivalent -to what GitHub sent, with whitespace and key order lost and array order preserved -([ADR 0001](adr/0001-jsonb-semantic-retention.md)). Enrichment state lives on the shared -entity rows rather than per-event, because a thousand events referencing one actor are one -enrichment obligation, not a thousand. - -```mermaid -erDiagram - EVENT_SOURCES ||--o{ INGESTION_RUNS : "records" - GITHUB_ACTORS ||--o{ PUSH_EVENTS : "performed" - GITHUB_REPOSITORIES ||--o{ PUSH_EVENTS : "received" - EVENT_SOURCES { - text source_type - text status - text etag - timestamp cadence_due_at - timestamp poll_floor_until - timestamp retry_not_before_at - } - INGESTION_RUNS { - uuid run_id UK - bigint event_source_id FK - text status - int events_created - int duplicates_skipped - int events_quarantined - } - PUSH_EVENTS { - text github_event_id UK - bigint github_push_id - bigint github_actor_id FK - bigint github_repository_id FK - text ref - jsonb raw_payload - } - GITHUB_ACTORS { - bigint github_id UK - text login - text enrichment_status - timestamp latest_event_at - timestamp next_retry_at - } - GITHUB_REPOSITORIES { - bigint github_id UK - text full_name - text enrichment_status - timestamp latest_event_at - timestamp next_retry_at - } - QUARANTINED_EVENTS { - text payload_fingerprint UK - text github_event_id - text error_code - int occurrence_count - } - GITHUB_API_BUDGET { - int id - text window_status - int poll_used - int enrichment_used - timestamp global_blocked_until - } -``` - -The two tables with no edges are the point. `quarantined_events` has no foreign key because -a malformed event may be malformed *precisely because* it lacks the field a key would -reference — its identity is `payload_fingerprint` alone, a SHA-256 of compact UTF-8 JSON -with recursively sorted keys. `github_api_budget` is a `CHECK (id = 1)` singleton because -the unauthenticated quota is keyed to the outbound IP, not to a source. Note also that -`push_events`' foreign keys target `github_id`, GitHub's own identifier, not the surrogate -primary key. - -## The durability boundary - -**An event is accepted when its `push_events` row commits** — not when GitHub returns it, -not when a job is enqueued, not when a log line says so. Everything upstream of that commit -is retryable; everything downstream is derived. - -```text -At-least-once execution -+ Idempotent writes -+ Unique constraints -= Effectively-once persisted outcomes + I --> X["RequestExecutor
gate → ledger → URL policy → transport"] + N --> X + X --> G["api.github.com
60 requests/hour/IP"] + I --> D[("PostgreSQL
business data")] + N --> D + W --> D + D -. "post-commit enqueue" .-> Q ``` -That is a database guarantee, not an application convention. `push_events` inserts with -`ON CONFLICT (github_event_id) DO NOTHING RETURNING id`, and entity activity updates happen -**only when `RETURNING` produced a row** — so a duplicate replay may refresh identity -fields but registers no activity and can never reactivate an entity a budget skip had -terminated. Transactions are per event, not per page, so one malformed envelope cannot -discard the events persisted beside it ([ADR 0005](adr/0005-at-least-once-with-idempotent-writes.md)). - -Restart safety needs no cleanup path because nothing needs cleaning. Session advisory locks -die with their session, so a crashed poller releases its source the moment its connection -drops. Entity leases are a `next_retry_at` timestamp that expires by arithmetic rather than -a lock someone must release. Pending enrichment work is the state of committed entity rows, -so `enqueue_after_transaction_commit` makes the enqueue a *hint* and an entity-scoped -reconciler rebuilds the work list from committed state every 60 seconds -([ADR 0008](adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md)). Docker's -`restart: unless-stopped` handles the container. **This is not exactly-once execution and -the system never claims it is** — a job may run twice, and the second run changes nothing. - -## The request budget, and the 304 finding - -One derived formula, never a configured number: +Every outbound call passes through `Github::RequestExecutor`. A global session advisory +lock serializes request reservation and execution, so the singleton budget ledger cannot +race itself. Polling also holds a per-source advisory lock for its whole cycle; the only +valid order is source lock, then request gate. Enrichment takes only the gate. Web routes +have no path to the executor, so `/health/*` and `/status` cannot spend GitHub budget. + +PostgreSQL is the system of record. Seven business tables hold source/run state, +`push_events`, shared actor and repository state, quarantined payloads, and the global +budget ledger; Solid Queue uses a separate database in the same server. Raw payloads use +`jsonb`, preserving JSON meaning rather than byte layout. Quarantine identity is a +canonical-payload SHA-256 because malformed data may have no usable GitHub event ID. + +Acceptance occurs when a `push_events` row commits. Inserts use +`ON CONFLICT (github_event_id) DO NOTHING RETURNING id`. A duplicate observation cannot +create a second event row, and entity activity or `skipped_budget` reactivation occurs only +when `RETURNING` yields a new row. A replay may still refresh permitted identity fields. +These are deliberately narrow invariants: executions, ingestion runs, quarantine +occurrence counts, budget debits, and logs can repeat or change. + +Work committed before a crash remains durable. Advisory locks disappear with their +sessions, entity leases expire by timestamp, and a reconciler rebuilds missing enrichment +work from committed entity rows, making cross-database enqueue a hint rather than the +durability boundary. Work lost before commit is recoverable only if the event remains in a +later response from the sliding feed; advancing out of that window is an acknowledged loss +mode. This is not exactly-once execution ([ADR 0005](adr/0005-at-least-once-with-idempotent-writes.md), +[ADR 0008](adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md)). + +## Request budget and the `304` finding + +The hourly allocation is derived rather than guessed: ```text -poll_attempt_allowance = ceil(3600 / POLL_INTERVAL_SECONDS) - × MAX_PAGES_PER_POLL × ENABLED_LIVE_SOURCE_COUNT -enrichment_allowance = rate_limit − RATE_LIMIT_RESERVE − poll_attempt_allowance -actor_guarantee = floor(enrichment_allowance × ACTOR_ENRICHMENT_SHARE) -repository_guarantee = enrichment_allowance − actor_guarantee +poll_allowance = ceil(3600 / POLL_INTERVAL_SECONDS) + × MAX_PAGES_PER_POLL × enabled_live_source_count +enrichment_allowance = rate_limit - RATE_LIMIT_RESERVE - poll_allowance +actor_guarantee = floor(enrichment_allowance × ACTOR_ENRICHMENT_SHARE) +repository_guarantee = enrichment_allowance - actor_guarantee ``` -| `MAX_PAGES_PER_POLL` | Poll allowance | Enrichment allowance | Actor / repository guarantee | -|---|---|---|---| -| 1 (default) | 12 | 40 | 20 / 20 | -| 2 | 24 | 28 | 14 / 14 | -| 3 | 36 | 16 | 8 / 8 | - -Against a limit of 60 with a reserve of 8. The process **refuses to boot** if polling would -leave no capacity for enrichment ([ADR 0004](adr/0004-class-aware-budget-ledger.md)). - -**The `304` finding.** GitHub's endpoint documentation contains a general statement that -`304` responses do not affect the rate limit, while its REST best-practices documentation -limits that exemption to correctly authorized requests. Dated unauthenticated probes showed -`x-ratelimit-used` increasing across a `304`. This implementation therefore budgets -unauthenticated conditional requests as one request. The asymmetry decides it: budgeting a -free `304` wastes one attempt an hour, while not budgeting a charged one overruns a -60-request window and blocks everything. ETag remains a bandwidth and correctness measure -here, never a quota saver. The transcript is the argument — -[`docs/evidence/2026-07-30-unauthenticated-304-quota-probe.md`](evidence/2026-07-30-unauthenticated-304-quota-probe.md). - -## Enrichment is a bounded sample - -One observed live page held ~92–95 `PushEvent` records with ~89 distinct actors and ~92 -distinct repositories. That is 181 cold entity requests per page, ~2,172 an hour at twelve -polls, against **40 available** — roughly 1.8% theoretical cold coverage. Partial coverage -is therefore the design, not a shortfall, and `skipped_budget` is a normal documented -outcome rather than a failure state. - -Repository candidates alone exceed the whole hourly allowance, so a naive repo-first policy -would starve actor enrichment to zero indefinitely — which Story 3 forbids. Hence per-class -guarantees with **eligibility-aware borrowing**: a class may spend past its guarantee only -when the other has no *currently eligible* candidate, and the ledger enforces the -arithmetic the fairness policy proposes, so a wrong answer produces a refused reservation -rather than an overspend ([ADR 0007](adr/0007-enrichment-fairness-shares-and-borrowing.md)). -Unbounded backlog growth is answered by three mechanisms together: an eligibility window -past which a candidate ages into `skipped_budget`, `skipped_budget` as a terminal state -that no retry sweeps back, and reactivation **only** on a genuinely new push event. The -backlog is bounded by construction, and `/status` publishes the real sampling rate so an -operator sees a sample rather than a mysteriously growing queue. - -Enrichment URLs arrive inside GitHub payloads, `Link` headers, and `Location` headers — -attacker-influenceable data — so the SSRF boundary is strict: HTTPS only, host exactly -`api.github.com`, no userinfo, no non-default port, no IP literals, and bounded redirects -each re-validated and separately debited. Every URL is rebuilt from validated components, -and the allowed host is a frozen constant rather than an environment variable, because a -deployment setting there would make the trust boundary configurable. Fixture mode fails -closed: an unknown URL is an error, never a live fallback +With the defaults, polling receives 12 attempts, the reserve is 8, and enrichment receives +40 attempts split 20/20. Startup rejects configurations leaving no enrichment allowance. +The source count comes from enabled in-service rows at window initialization, with the +configured count as a boot-time fallback ([ADR 0004](adr/0004-class-aware-budget-ledger.md), +[ADR 0009](adr/0009-runtime-source-allocation-and-shared-ip-observability.md)). + +GitHub's endpoint documentation broadly describes `304` responses as free, while its REST +best-practices guidance scopes that behavior to correctly authorized requests. A dated +unauthenticated probe observed `x-ratelimit-used` increase across a `304`, so this system +debits every unauthenticated conditional request. ETags still save bandwidth but not +budget. Treating a charged response as free risks exhausting the shared window; treating a +free one as charged costs only local opportunity. The [probe transcript](evidence/2026-07-30-unauthenticated-304-quota-probe.md) +records the evidence. + +## Bounded, fair, and safe enrichment + +One observed page contained about 90 distinct actors and 90 repositories: roughly 180 cold +lookups competing for 40 hourly attempts. Candidates therefore age from `pending` to the +terminal `skipped_budget` state after an eligibility window. Only a newly inserted push +event can reactivate a skipped entity. This bounds actionable backlog while `/status` +reports pending, skipped, and sampled proportions. + +Each entity class receives a guaranteed share. A class may borrow beyond it only when the +other has no currently eligible candidate; the ledger independently refuses invalid +reservations. Never-enriched candidates precede stale refreshes, and selection plus leases +prevents concurrent duplicate work. This keeps a repository-heavy feed from starving actor +enrichment without wasting an idle share ([ADR 0007](adr/0007-enrichment-fairness-shares-and-borrowing.md)). + +Payload, pagination, and redirect URLs are attacker-influenceable. The SSRF boundary allows +only HTTPS URLs whose host is exactly `api.github.com`, with no userinfo, non-default port, +or IP literal. Redirects are bounded, revalidated, and separately debited. Fixture mode +fails closed on an unknown URL and never falls back to the network ([ADR 0003](adr/0003-event-source-and-transport-seams.md)). -## Tradeoffs and assumptions - -| Decision | Cost accepted | Record | -|---|---|---| -| `jsonb` semantic retention, not byte-exact | Whitespace, key order, and duplicate keys are lost | [ADR 0001](adr/0001-jsonb-semantic-retention.md) | -| Session advisory locks over `FOR UPDATE` row claims | Lock state is invisible to `SELECT`; needs its own observability | [ADR 0002](adr/0002-advisory-locks-and-request-gate.md) | -| At-least-once execution over exactly-once | Non-idempotent side effects repeat; duplicate work is spent work | [ADR 0005](adr/0005-at-least-once-with-idempotent-writes.md) | -| Five decomposed scheduling columns over one `next_poll_at` | More state to reason about; `next_poll_at` is a cache, never an input | [ADR 0006](adr/0006-decomposed-poll-deferral-state.md) | -| Fairness guarantees with borrowing, not hard caps | A borrowing class can consume the window when the other is idle | [ADR 0007](adr/0007-enrichment-fairness-shares-and-borrowing.md) | -| Solid Queue in a second database, not Kafka | Queue throughput bounded by PostgreSQL; no cross-service fan-out | [ADR 0012](adr/0012-solid-queue-over-kafka.md) | - -The load-bearing assumption is that the bottleneck is upstream quota rather than local -throughput. It holds at 60 requests an hour and would need re-examining at 5,000. - -## What was deliberately not built - -**Extension C (object storage) was not attempted** — a decision, not an oversight. The -remaining budget went to rate-limit correctness, durability, and reviewer experience, which -carry more signal than a fourth extension. - -**No authentication.** The 60-requests-per-hour ceiling is the entire design constraint; -removing it would remove the problem this submission is actually about. - -**No guarantee of complete upstream capture** and **no guarantee of complete enrichment -coverage.** Both are arithmetic consequences stated in the README's known limitations, not -gaps to be closed later. - -**No `/api/actors/:id`, `/api/repositories/:id`, or `/api/ingestion_runs`** — listed as -optional in the plan and left unbuilt rather than half-built. - -## Scaling path - -**An authenticated token does not tune the enrichment story; it deletes it.** At 5,000 -requests an hour, sampling becomes coverage, `skipped_budget` becomes a bug rather than a -state, and the fairness shares become an accounting curiosity. That is also the point where -Solid Queue's throughput ceiling starts to matter and where a broker would first earn its -place. The seams are already in place: `ENABLED_LIVE_SOURCE_COUNT` and runtime source -allocation ([ADR 0009](adr/0009-runtime-source-allocation-and-shared-ip-observability.md)) -mean adding a second event source is configuration plus one adapter, not a redesign. - -The API version is pinned to `2022-11-28` because every live probe behind this design ran -under it. Upgrading to `2026-03-10` is a deliberate follow-up gated on re-verifying payload -shape and re-running the `304` probe first — not on the version being available -([ADR 0011](adr/0011-pinned-api-version-2022-11-28.md)). - -## Where the detail lives +## Tradeoffs, omissions, and scaling -| Artifact | What it holds | +| Decision | Deliberate cost | |---|---| -| [`README.md`](../README.md) | How to run, verify, inspect, and reset it | -| [`IMPLEMENTATION_PLAN.md`](../IMPLEMENTATION_PLAN.md) | Execution and traceability; revision history in Appendices A–D, execution summary in E | -| [`docs/adr/`](adr/) | Twelve decisions, each with its context, cost, and consequences | -| [`docs/evidence/`](evidence/) | Dated first-party verifications of contested claims | +| `jsonb` semantic retention | Whitespace, object-key order, and duplicate keys are lost | +| Session advisory locks | Lock ownership needs dedicated observability | +| At-least-once job execution | Duplicate work and non-event side effects may repeat | +| Solid Queue over Kafka | PostgreSQL bounds queue throughput and fan-out | + +Authentication, object storage, extra entity APIs, Kafka, and a frontend were omitted to +keep the submission focused on correctness at the stated quota. The current bottleneck is +upstream allowance, not local throughput. A larger authenticated budget could materially +increase feasible coverage, but would not establish complete capture or enrichment: feed +window loss, failures, demand, and shared-budget effects still apply. At higher sustained +throughput, queue capacity, database contention, and source partitioning should be measured +before introducing a broker. The API version remains pinned to `2022-11-28`; any upgrade +must revalidate payloads, redirects, headers, and `304` accounting first. diff --git a/docs/SUBMISSION_CHECKLIST.md b/docs/SUBMISSION_CHECKLIST.md index d1ae763..4e09337 100644 --- a/docs/SUBMISSION_CHECKLIST.md +++ b/docs/SUBMISSION_CHECKLIST.md @@ -2,35 +2,170 @@ Repository: https://github.com/batbrainy/github-push-ingestor -Verified against: ``, `` +This is a reusable runbook, not a record of one run. Keep the boxes unchecked in the +repository. The external findings report records the default-branch SHA, UTC date, command, +exit code, salient output, duration, and one classification for every gate: pass, repository +defect, environment issue, or documentation mismatch. -Pre-flight for `IMPLEMENTATION_PLAN.md` §16. **Every box is checked against the default -branch after PR 12 merges, from a fresh clone into an empty directory — never against a -working tree.** A working tree can pass gates a clone would fail: an untracked `.env`, a -`config/master.key`, a stale image, a warm database volume. +Run every gate against the default branch after the final hardening change merges, from a +fresh clone — never against a working tree. A working tree can hide an untracked `.env` or +`config/master.key`, while Compose's fixed project name can make even a fresh clone reuse a +stale image or the globally named `github-push-ingestor_pgdata` volume. --- -## 1. Clean-checkout verification - -- [ ] `git clone` of the default branch into an empty directory; `git status --porcelain` - is empty -- [ ] No `.env`, no `config/master.key`, no token anywhere in the clone -- [ ] `docker compose build --no-cache --pull` succeeds — a genuinely cold image build with - `BUNDLE_FROZEN=1` against the committed `Gemfile.lock` -- [ ] `docker compose up --build` starts exactly `db`, `setup`, `web`, `worker` — and - nothing else -- [ ] `curl http://localhost:3000/health/ready` returns `{"status":"ok"}` -- [ ] `GITHUB_MODE=fixture docker compose run --rm ingest` reports **4 created, 3 - quarantined, 1 ignored** from an empty database -- [ ] `docker compose run --rm test` is green across both rspec invocations -- [ ] `docker compose down` then `up`, and `SELECT COUNT(*) FROM push_events` is unchanged -- [ ] No local Ruby, PostgreSQL, or `psql` was used at any point - -Pre-merge run against `6eab84c`: -[`docs/evidence/2026-07-31-clean-checkout-verification.md`](evidence/2026-07-31-clean-checkout-verification.md). -It found and fixed a defect that made `docker compose up --build` fail from a cold image — -which is why this section is checked again, from a fresh clone, after the merge. +## 1. Authoritative clean-checkout verification + +### Fresh clone and cold-image live startup + +- [ ] Clone into a newly created parent directory, record the full SHA externally, and prove + the clone is clean: + + ```bash + verification_parent="$(mktemp -d)" + git clone https://github.com/batbrainy/github-push-ingestor.git \ + "$verification_parent/repository" + cd "$verification_parent/repository" + git rev-parse HEAD + test -z "$(git status --porcelain)" + test ! -e .env + test ! -e config/master.key + ``` + +- [ ] Before destructive Docker commands, archive any valued data already stored in + `github-push-ingestor_pgdata`. Because the project name is fixed, another clone can own + that same volume. +- [ ] Remove prior project resources and the global volume, then require volume inspection to + fail. Continuing past this point asserts the prior data is archived or disposable: + + ```bash + docker compose down -v --remove-orphans + if docker volume inspect github-push-ingestor_pgdata >/dev/null 2>&1; then + echo "github-push-ingestor_pgdata still exists" >&2 + exit 1 + fi + ``` + +- [ ] Exercise both cold build paths. Remove only the application image while no project + containers exist: + + ```bash + docker compose build --no-cache --pull + docker image rm github-push-ingestor-app:latest + docker compose up --build -d + docker compose ps --all + ``` + +- [ ] Exactly `db` healthy, `setup` exited 0, `web` healthy, and `worker` running; no + tool-profile service started. +- [ ] `curl -fsS http://localhost:3000/health/ready` returns `{"status":"ok"}`. +- [ ] The live worker reaches the public GitHub Events API without a token. Record the + corresponding worker log and budget change; do not infer this from a health endpoint. + +### Empty-volume fixture phase + +- [ ] End the live phase with an explicit destructive boundary, prove the global volume is + absent, then start only `db`, `setup`, and `web` offline so the worker cannot race the + one-shots: + + ```bash + docker compose down -v --remove-orphans + if docker volume inspect github-push-ingestor_pgdata >/dev/null 2>&1; then + echo "github-push-ingestor_pgdata still exists" >&2 + exit 1 + fi + GITHUB_MODE=fixture docker compose up --build -d db setup web + curl -fsS http://localhost:3000/health/ready + ``` + +- [ ] Capture fixture ingestion output directly. The `run --rm` container is removed, so no + retained service log can validate it later: + + ```bash + set -o pipefail + fixture_ingest_output="$(mktemp)" + GITHUB_MODE=fixture docker compose run --rm ingest 2>&1 | tee "$fixture_ingest_output" + grep -E 'Push events created:[[:space:]]+4' "$fixture_ingest_output" + grep -E 'Events quarantined:[[:space:]]+3' "$fixture_ingest_output" + grep -E 'Non-push events ignored:[[:space:]]+1' "$fixture_ingest_output" + ``` + +- [ ] `GITHUB_MODE=fixture docker compose run --rm enrich --limit 6` exits 0 and leaves + `complete 2 / permanent_failure 1` in each entity class. +- [ ] SQL state is exactly 4 events, 3 actors, 3 repositories, 3 quarantine rows, and 3 total + quarantine occurrences: + + ```bash + docker compose exec -T db psql -U postgres -d github_push_ingestor_development -c " + SELECT (SELECT COUNT(*) FROM push_events) AS push_events, + (SELECT COUNT(*) FROM github_actors) AS actors, + (SELECT COUNT(*) FROM github_repositories) AS repositories, + (SELECT COUNT(*) FROM quarantined_events) AS quarantined, + (SELECT SUM(occurrence_count) FROM quarantined_events) AS occurrences; + SELECT 'actor' AS class, enrichment_status, COUNT(*) FROM github_actors GROUP BY 2 + UNION ALL + SELECT 'repository', enrichment_status, COUNT(*) FROM github_repositories GROUP BY 2;" + ``` + +- [ ] After the 60-second poll floor, capture `ingest --force`; require 4 duplicates and no + `enrichment.reactivated` line in that captured output: + + ```bash + sleep 60 + fixture_replay_output="$(mktemp)" + GITHUB_MODE=fixture docker compose run --rm ingest --force 2>&1 | tee "$fixture_replay_output" + grep -E 'Duplicates skipped:[[:space:]]+4' "$fixture_replay_output" + if grep -q 'enrichment.reactivated' "$fixture_replay_output"; then + echo "duplicate replay reactivated an entity" >&2 + exit 1 + fi + rm -f "$fixture_ingest_output" "$fixture_replay_output" + ``` + +### Tests, recovery, and persistence + +- [ ] Record development `push_events`, development `solid_queue_jobs`, and the setup + container ID/finish time. Run the requested suite and two consecutive repeats, one with + fixed seed 4242, then prove those three development observations are unchanged: + + ```bash + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_development -Atc 'SELECT COUNT(*) FROM push_events;' + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_queue_development -Atc 'SELECT COUNT(*) FROM solid_queue_jobs;' + setup_container_id="$(docker compose ps --all --quiet setup)" + docker inspect --format '{{.Id}} {{.State.FinishedAt}}' "$setup_container_id" + + docker compose run --rm --build test + docker compose run --rm test + docker compose run --rm test bash -c \ + 'bin/rails db:test:prepare && bundle exec rspec --seed 4242 && bundle exec rspec spec/stress --seed 4242' + + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_development -Atc 'SELECT COUNT(*) FROM push_events;' + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_queue_development -Atc 'SELECT COUNT(*) FROM solid_queue_jobs;' + docker inspect --format '{{.Id}} {{.State.FinishedAt}}' "$setup_container_id" + ``` + +- [ ] Reset to another empty volume, start the full stack offline, and run recovery. Every + subprocess and assertion exits 0, and the worker is still offline afterward: + + ```bash + docker compose down -v --remove-orphans + GITHUB_MODE=fixture docker compose up --build -d + GITHUB_MODE=fixture script/verify_recovery.sh --confirm + test "$(docker compose exec -T worker printenv GITHUB_MODE)" = fixture + ``` + +- [ ] `push_events` is equal before and after `docker compose restart`. +- [ ] `push_events` is equal before and after `docker compose down --remove-orphans` followed + by `GITHUB_MODE=fixture docker compose up --build -d`. +- [ ] No local Ruby, PostgreSQL, or `psql` was used at any point. + +The dated evidence files are historical inputs, not substitutes for this post-merge run. In +particular, read the erratum atop +[`2026-07-31-container-kill-recovery.md`](evidence/2026-07-31-container-kill-recovery.md). --- @@ -47,7 +182,7 @@ which is why this section is checked again, from a fresh clone, after the merge. - [ ] **Both** actor and repository enrichment demonstrably occur within their fairness guarantees — `spec/services/github/enrichment/end_to_end_spec.rb`; fixture run gives `complete 2 / permanent_failure 1` per class -- [ ] Duplicate ingestion is safe — and duplicate replays never reactivate skipped entities +- [ ] A duplicate event ID cannot add another `push_events` row or reactivate a skipped entity — fixture replay: 4 duplicates absorbed, no `enrichment.reactivated` - [ ] `Link`-header pagination is handled; every fetched page fully processed — `spec/services/github/ingestion/page_loop_spec.rb`; `paginated` scenario @@ -64,16 +199,19 @@ which is why this section is checked again, from a fresh clone, after the merge. ## 3. Durability gates — §16 - [ ] PostgreSQL uses a named volume — `docker-compose.yml`, `spec/docker_compose_spec.rb` -- [ ] Docker restart policies recover crashed `db`/`web`/`worker` automatically (verified by - container kills) — `docs/evidence/2026-07-31-container-kill-recovery.md` -- [ ] Application restart preserves events — verification step 9 +- [ ] Docker restart policies recover process crashes in `db`/`web`/`worker` automatically. + `docker kill` is retained separately as an API-stop negative control and must leave an + `unless-stopped` container down — `script/verify_recovery.sh` +- [ ] Application restart preserves events — runtime comparisons in §1 - [ ] Worker restart preserves pending work — `spec/recovery/worker_crash_lease_spec.rb` -- [ ] An event committed before a crash remains recoverable — - `spec/recovery/crash_window_spec.rb` +- [ ] An event that did not commit before a crash can be observed again only if it remains in + a later feed window; an event that did commit remains durable and its derived pending + work is reconciled — `spec/recovery/crash_window_spec.rb` - [ ] Advisory locks provably release on session death (tested) — `spec/recovery/advisory_lock_session_death_spec.rb`, real `pg_terminate_backend` -- [ ] Duplicate jobs do not duplicate durable data — - `spec/recovery/duplicate_job_execution_spec.rb` +- [ ] The covered actor-job redelivery leaves an already-complete actor row unchanged — + `spec/recovery/duplicate_job_execution_spec.rb`; this does not imply universal + idempotency of persisted state - [ ] Reconciliation recovers missing enrichment scheduling — `spec/recovery/pending_enrichment_recovery_spec.rb` - [ ] The enrichment backlog is bounded (eligibility window + `skipped_budget` + @@ -92,6 +230,31 @@ which is why this section is checked again, from a fresh clone, after the merge. - [ ] `/status` reports window status, poll state, per-class ledger state, pending/skipped counts, and coverage percentages by the defined formulas — without initiating GitHub requests — `spec/requests/status_spec.rb`, `Github::Enrichment::Coverage` +- [ ] During §1's empty-volume fixture phase, while the worker has never been started, hash + the complete budget row and record all request counters. Call `/health/live`, + `/health/ready`, and `/status` repeatedly, then require the hash and counters to be + byte-for-byte unchanged. A running worker is a concurrent writer and invalidates this + measurement: + + ```bash + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_development -Atc \ + "SELECT md5(COALESCE(row_to_json(b)::text, '')) FROM github_api_budget b;" + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_development -Atc \ + 'SELECT poll_used, enrichment_used, actor_share_used, repository_share_used FROM github_api_budget;' + for endpoint in health/live health/ready status; do + curl -fsS "http://localhost:3000/$endpoint" >/dev/null + curl -fsS "http://localhost:3000/$endpoint" >/dev/null + curl -fsS "http://localhost:3000/$endpoint" >/dev/null + done + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_development -Atc \ + "SELECT md5(COALESCE(row_to_json(b)::text, '')) FROM github_api_budget b;" + docker compose exec -T db psql -U postgres \ + -d github_push_ingestor_development -Atc \ + 'SELECT poll_used, enrichment_used, actor_share_used, repository_share_used FROM github_api_budget;' + ``` - [ ] Retry behavior is visible — `github.retry_scheduled` / `github.retry_exhausted` at the default level - [ ] Failures contain actionable context — every failure line carries classification, @@ -106,10 +269,17 @@ which is why this section is checked again, from a fresh clone, after the merge. - [ ] Commands match the assignment - [ ] Plain `docker compose up --build` starts exactly `db`, `setup`, `web`, `worker` — `profiles: ["tools"]` on the other three -- [ ] `docker compose run --rm test` never touches the development databases (app or queue) - and never triggers the development `setup` service — `spec/docker_compose_spec.rb` +- [ ] `docker compose run --rm test` rebuilds through `pull_policy: build`, never touches the + development databases (app or queue), and never triggers the development `setup` + service — runtime comparison in §1 and `spec/docker_compose_spec.rb` - [ ] Documentation is accurate; the README points to the plan and its appendix revision record +- [ ] Every repository-local Markdown target and heading anchor resolves; verify the final + GitHub-rendered README, design brief, ADR links, and Mermaid architecture figure after + the hardening PR merges +- [ ] Render `docs/DESIGN_BRIEF.md` temporarily as US Letter at 11pt with 0.75-inch margins; + `pdfinfo` reports at most two pages, and visual inspection finds no clipping, overlap, + malformed table, or awkward page break. The QA PDF is not committed - [ ] No secrets or token are required - [ ] Tests are deterministic — WebMock denies net connect; no VCR; fixture rate-limit resets are relative, so the corpus does not rot @@ -123,17 +293,26 @@ which is why this section is checked again, from a fresh clone, after the merge. - [ ] **No secrets** ```bash - bin/brakeman - bin/bundler-audit - git log -p | grep -inE 'ghp_|github_pat_|ghs_|gho_|BEGIN [A-Z]+ PRIVATE KEY' + docker compose run --rm test bin/brakeman --quiet --no-pager --exit-on-warn --exit-on-error + docker compose run --rm test bin/bundler-audit + + find . -type f \( -name '.env' -o -path '*/config/master.key' \) -print + token_pattern='([g]hp|[g]ho|[g]hu|[g]hs|[g]hr)_[A-Za-z0-9]{20,}|github_pat_[A-Za-z0-9_]{20,}|Authorization:[[:space:]]*(Bearer|token)[[:space:]]+[A-Za-z0-9._~+/-]{20,}|BEGIN [A-Z0-9 ]*PRIVATE KEY' + git grep -nEI "$token_pattern" -- . + git log -p --all | grep -nEI "$token_pattern" + git log --all --name-only --pretty=format: | \ + grep -nE '(^|/)\.env$|(^|/)config/master\.key$' ``` -- [ ] **No personal access token** — `grep -rn "Authorization" app lib config` returns only + Every search must print nothing; the `grep` searches exiting 1 means no match and + passes. Any output is a stop-ship finding, not something to redact in place. + +- [ ] **No personal access token** — `git grep -n "Authorization" -- app lib config` returns only the SSRF-policy and header code - [ ] **No stale documentation** ```bash - grep -rn "lands with PR\|available now\|Planned contents\|PR 12 (design brief)" README.md + git grep -nEI '[l]ands with PR|[a]vailable now|[P]lanned contents|after PR [0-9]+ merges|PR 12 [(]design brief[)]' -- README.md docs ``` returns nothing. @@ -149,18 +328,14 @@ which is why this section is checked again, from a fresh clone, after the merge. ## 7. Forbidden-claim scan ```bash -grep -rniE "exactly.once|complete capture|complete(ly)? enrich" \ - README.md docs/ IMPLEMENTATION_PLAN.md CLAUDE.md +claim_pattern='exactly[ -]?once|effectively[ -]?once|once-only[[:space:]]+(execution|processing|delivery)|(complete|full|exhaustive)[[:space:]]+(upstream[[:space:]]+)?(event[[:space:]]+)?capture|captur(e|es|ed|ing)[[:space:]]+(all|every)[[:space:]]+(upstream[[:space:]]+|public[[:space:]]+|GitHub[[:space:]]+)?events?|(complete|full|exhaustive)[[:space:]]+enrichment([[:space:]]+coverage)?|enrich(es|ed|ing)?[[:space:]]+(all|every)[[:space:]]+(actors?|repositories|entities)|sampling[[:space:]]+becomes[[:space:]]+coverage|100%[[:space:]]+(capture|enrichment|coverage)' +git grep -nEI "$claim_pattern" -- . ``` -**The rule: every hit must be a negation.** A hit that asserts the claim fails the gate. - -Legitimate existing hits are the disclaimers themselves — `README.md`'s "does not claim -exactly-once execution" and "not complete enrichment coverage", `CLAUDE.md`'s "Never claim -or code against exactly-once", `IMPLEMENTATION_PLAN.md` §8's "The system does not claim -exactly-once execution" and §16's four `No …` bullets, and -[ADR 0005](adr/0005-at-least-once-with-idempotent-writes.md)'s "this is not exactly-once -execution, and the system must never claim it is". +**The rule: every prose hit must explicitly reject the guarantee.** The regex assignment +itself is scan vocabulary, not a claim. Any affirmative system-level promise of singular +execution, exhaustive upstream capture, or exhaustive enrichment fails the gate; do not +approve it merely because it avoids one exact phrase. - [ ] Every hit is a negation @@ -168,8 +343,9 @@ execution, and the system must never claim it is". ## 8. Submission email — POST-MERGE -> **Post-merge step.** Send only after PR 12 is merged into the default branch and §1 above -> passes against a fresh clone of that branch. Nothing in this repository sends it. +> **Post-merge step.** Send only after every section passes against a fresh clone of the +> current default-branch SHA and the external findings report records a green verdict. +> Nothing in this repository sends it. The subject uses a plain ASCII hyphen-minus, matching `IMPLEMENTATION_PLAN.md` §13 and §16 exactly. Do not let a formatter turn it into an en dash. @@ -204,4 +380,5 @@ Thank you for your time. Umang Brahmakshatriya ``` -- [ ] Sent on `` +Sending is external to this runbook. Record the recipient, UTC send time, verified SHA, and +the green findings-report reference outside the repository. diff --git a/docs/adr/0005-at-least-once-with-idempotent-writes.md b/docs/adr/0005-at-least-once-with-idempotent-writes.md index a759c6b..e6047b3 100644 --- a/docs/adr/0005-at-least-once-with-idempotent-writes.md +++ b/docs/adr/0005-at-least-once-with-idempotent-writes.md @@ -1,95 +1,79 @@ -# 5. At-least-once processing with idempotent writes +# 5. Repeated execution with duplicate-safe event writes Date: 2026-07-30 -Status: Accepted +Status: Accepted; guarantee wording clarified 2026-07-31 ## Context -Polling a third-party feed over HTTP, from a process that can be killed at any moment, -gives no way to make "fetch a page" and "persist its events" one atomic act. Three -mechanisms in this system each guarantee *at least* one delivery and none guarantees -exactly one: +Fetching a third-party HTTP feed and persisting its events cannot be one atomic act. +Repetition and loss are both possible at different boundaries: -- GitHub's `/events` feed is a sliding window with a documented 30s–6h latency, and - `IMPLEMENTATION_PLAN.md` §9 processes every fetched page in full with no - stop-on-known-event, so consecutive polls overlap by design. -- A crash between the HTTP response and the commit loses the work, and the same events - reappear on the next poll (§8, "Crash before commit"). -- From PR 8, Solid Queue re-runs a job whose worker died before acknowledgement (§8, - "Worker crash after enrichment commit but before acknowledgement"). +- Consecutive `/events` polls intentionally overlap when an event remains in GitHub's + sliding feed. Processing never stops at the first known event. +- A crash between an HTTP response and the database commit loses that attempt. The event + can be recovered by a later poll only if it is still present in the sliding feed. +- Solid Queue may run a job again when a worker dies before acknowledgement, including + after the job's business write committed. -Two responses were available. Chase exactly-once execution — a distributed-transaction -or dedup-ledger design across an HTTP client, a queue in a second database, and the -business tables. Or accept repetition in *execution* and make it invisible in -*outcomes*. +The feed does not promise that this service observes every event before the window +advances. The queue and polling paths also cannot provide exactly-once execution across an +HTTP service, a queue database, and the business database. ## Decision -Accept at-least-once execution and make every write idempotent, so the durable outcome -is the same however many times an event is processed: - -```text -At-least-once execution -+ Idempotent writes -+ Unique constraints -= Effectively-once persisted outcomes -``` - -Four mechanisms implement it, and each is a database guarantee rather than an -application convention: - -1. **`push_events`** inserts with `ON CONFLICT (github_event_id) DO NOTHING RETURNING id`. - A re-polled event is a no-op, and an accepted raw event is never mutated by a re-poll. -2. **Stub entities** upsert with `ON CONFLICT (github_id) DO UPDATE` under §7's merge - rules, which refresh identity fields and never touch an enrichment payload. -3. **`quarantined_events`** upserts on `payload_fingerprint` — SHA-256 of compact UTF-8 - JSON with recursively sorted keys — incrementing `occurrence_count`. The fingerprint - is the sole unique key, because a malformed event may be malformed precisely because - it lacks an event ID. -4. **Entity activity updates are gated on `RETURNING` producing a row.** A duplicate - replay may refresh identity fields but registers no activity and, from PR 7, can - never reactivate an entity that a budget skip had terminated. - -The durability boundary is the committed `push_events` row (§8). Not the HTTP response, -not the queue, not the `ingestion_runs` row — that last one is a summary artifact, and a -process killed mid-page leaves it in `running` with zeroed counters, which is the honest -signal rather than a bug. - -Transactions are per envelope, not per page. PostgreSQL aborts an entire transaction on -any failed statement, so a page-wide transaction would let one malformed envelope -discard the events already reported as persisted beside it — exactly what §16's -"malformed data … does not terminate the batch" forbids. Quarantine writes stand outside -any transaction, so a quarantine record can never be discarded by a later failure. +Accept repeated execution and enforce two narrow ingestion invariants at the database +boundary: + +1. A duplicate observation of a GitHub event cannot create a second `push_events` row. +2. A duplicate observation cannot register new entity activity or reactivate an entity in + `skipped_budget`. + +Those invariants are implemented by +`ON CONFLICT (github_event_id) DO NOTHING RETURNING id` and by applying entity activity +updates only when `RETURNING` yields a newly inserted row. A duplicate may still refresh +permitted identity fields under Section 7's merge rules, but never an enrichment payload. + +Other writes have intentionally different repetition semantics: + +- Stub entities upsert on `github_id` and may refresh identity fields. +- Quarantine rows upsert on a canonical `payload_fingerprint` and increment + `occurrence_count` for every observation. +- Each ingestion attempt may create or update an `ingestion_runs` summary. +- Request reservations, job executions, logs, and other operational effects may repeat. + +The durability boundary is the committed `push_events` row (Section 8), not the HTTP +response, queue entry, or run summary. Transactions are per envelope rather than per page, +so one malformed envelope cannot roll back valid neighbors. Quarantine writes are isolated +from the event transaction so a later event failure cannot erase the observed malformed +payload. ## Consequences What this buys: -- **Re-running ingestion is always safe.** `docker compose run --rm ingest` can be - invoked at any frequency: duplicates are absorbed by uniqueness, replays never register - false activity, and a deferral is exit 0 rather than a failure — so an operator never - has to reason about whether a re-run is safe before typing it. -- Crash recovery needs no reconciliation of partial writes. Whatever committed is - correct; whatever did not reappears on the next overlapping poll. -- Restart safety costs no extra infrastructure: no dedup table, no distributed - transaction, no coordination between the queue database and the business tables. - -What it costs, stated plainly: - -- **This is not exactly-once execution, and the system must never claim it is** (§8, - §16, `CLAUDE.md`). Side effects that are not idempotent writes — a log line, a metric, - an outbound notification — can and will repeat. -- Duplicate work is spent work. A re-polled page re-runs classification for events - already stored, and its request already debited the budget ledger. -- `events_quarantined` counts observations, not rows: the same malformed payload seen - twice reports 1 and 1 while `occurrence_count` climbs to 2. Consistent with - `duplicates_skipped`, which also counts observations. -- The first classification of a quarantined payload is permanent — `error_code` is never - refreshed on replay — which is why classification is a pure, order-stable function of - the payload with a declared precedence order rather than whatever check happened to run - first. - -The suite asserts the outcome rather than the mechanism: ingesting the same fixture page -twice produces four push events, three quarantine rows with `occurrence_count` at two, -unchanged entity activity, and a planted `skipped_budget` entity still skipped. +- Re-polling or manually re-running ingestion cannot duplicate an accepted event row or + falsely reactivate a skipped entity from the same event ID. +- Committed events survive process and container restarts without a separate dedup table + or distributed transaction. +- Committed entity state is sufficient for the reconciler to rediscover pending enrichment + after a cross-database enqueue gap. + +What it does not buy: + +- **This is not exactly-once execution.** A job, HTTP request, run record, quarantine + observation, budget debit, or log entry may occur more than once. +- An event lost before commit is not guaranteed to reappear; recovery depends on GitHub + retaining it in a later sliding-feed response. +- Duplicate work still consumes CPU, and another poll or HTTP retry consumes quota before + uniqueness can reject an event row. +- `events_quarantined` counts observations rather than unique rows: replaying the same bad + payload increments `occurrence_count` even though its row remains unique. +- The first quarantine classification is retained, so classification must remain a pure, + precedence-ordered function of the payload. + +The suite tests the stated boundaries: fixture replay leaves four `push_events` rows, +increments the three quarantine occurrence counters, does not register duplicate entity +activity, and leaves a planted `skipped_budget` entity skipped. A separate recovery test +shows a delivered enrichment job may execute again without creating another entity row; +that scenario does not widen the ingestion guarantees above. diff --git a/docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md b/docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md index f57df04..6d40bf5 100644 --- a/docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md +++ b/docs/adr/0008-post-commit-enqueue-and-entity-scoped-reconciliation.md @@ -38,17 +38,19 @@ So an enqueue cannot mean "enrich this actor". It can only mean "there may be ac row of the run has committed, the run row is finalized, and the source advisory lock is gone. -3. **`ReconcilePendingEnrichmentsJob` runs every 60 seconds and is the recovery +3. **`ReconcilePendingEnrichmentsJob` is scheduled every 60 seconds and is the recovery mechanism.** It asks the same object the same question, from committed state alone. Work - whose enqueue was lost — to a kill, to a queue-database failure, to a discarded job — - is rediscovered on the next tick, within a minute, with no operator step and no cleanup - job. + whose enqueue was lost — to a crash, to a queue-database failure, to a discarded job — + is eligible for rediscovery on a later successful scheduled tick, with no cleanup job. + The schedule is nominal; queueing, worker downtime, or database unavailability can delay + execution. 4. **At most one job per class per dispatch**, whatever the backlog depth. 5. **No `retry_on` on any job.** The durable retry ladders already exist and are coordinated with the ledger: `Github::Ingestion::PollState` for a source, - `Github::Enrichment::EntityState` for an entity. The 60-second tick is the retry. + `Github::Enrichment::EntityState` for an entity. A later successful scheduled tick + initiates eligible retry work. ## Consequences @@ -67,8 +69,8 @@ Positive: Negative, and accepted: -- Enrichment latency after a lost enqueue is up to 60 seconds. That is the cadence's - cost, and it is far inside the 30s–6h latency of the feed being sampled. +- Recovery after a lost enqueue is not immediate. The 60-second schedule sets the nominal + opportunity; queueing and service outages can extend the delay. - A dispatch that finds work still only *schedules* one cycle per class, so a large backlog drains at the reconciler's cadence rather than in a burst. `bin/enrich --limit N` remains the operator's handle when a burst is wanted. @@ -91,10 +93,10 @@ precede the commit it is supposed to follow. **Solid Queue concurrency limits keyed by source id** (§9's third multi-poller bullet). Rejected for PR 8 with a reason, not deferred silently: a `limits_concurrency` semaphore has -a fixed `duration`, so a container killed mid-poll would suppress that source until the +a fixed `duration`, so a process crash mid-poll would suppress that source until the semaphore expired. The session advisory lock this system already holds is released by PostgreSQL the instant the backend dies. Adopting a weaker, crash-unsafe duplicate of an -existing lock — in the PR whose subject is surviving container kills — would be a +existing lock — in the PR whose subject is surviving process crashes — would be a regression. The source lock, the global request gate and the unique event constraint are the protections in force; revisit when PR 11's multi-poller tests can measure a gap. @@ -138,6 +140,6 @@ each held a database connection. Deferred to PR 9, which owns multi-source alloc - ADR 0002 — advisory locks and the request gate (the crash-safety property this decision leans on) - ADR 0004 — the class-aware budget ledger (what bounds enrichment throughput) -- ADR 0005 — at-least-once execution with idempotent writes (why a duplicate delivery is - safe) +- ADR 0005 — repeated execution with duplicate-safe event writes (the narrow event-row and + skipped-reactivation guarantees under redelivery) - ADR 0007 — enrichment fairness shares and borrowing (why a job cannot carry an entity id) diff --git a/docs/adr/0012-solid-queue-over-kafka.md b/docs/adr/0012-solid-queue-over-kafka.md index f88f8db..c28120d 100644 --- a/docs/adr/0012-solid-queue-over-kafka.md +++ b/docs/adr/0012-solid-queue-over-kafka.md @@ -17,8 +17,9 @@ The arithmetic: - The source is **one polled HTTP endpoint** under an unauthenticated ceiling of 60 requests per hour per IP. At the default cadence the system issues twelve poll requests an hour and reads at most ~100 events per page. -- Peak sustained ingest is therefore on the order of **a few hundred events an hour**, - bounded by upstream quota rather than by anything downstream. +- At the default one-page cap, the arithmetic upper bound is roughly **1,200 event + envelopes an hour** before filtering, still bounded by upstream quota rather than by + anything downstream. - Enrichment is bounded by the same 60-request ceiling — around 40 requests an hour after the poll allowance and reserve, which §10 states plainly is a *sample* of demand rather than coverage of it. @@ -38,17 +39,18 @@ Three reasons, in order of weight: durability boundary is the committed `push_events` row (ADR 0005), and pending enrichment work is not a message — it is the state of a committed entity row (`enrichment_status`, `next_retry_at`). `ReconcilePendingEnrichmentsJob` rebuilds the - work list from those rows every 60 seconds, which is why ADR 0008 can call the enqueue - a *hint*. Delete every queued job and the system loses nothing: the next tick - re-derives the same work. A broker would be a second, weaker copy of a record - PostgreSQL already holds under constraints. -2. **One database is one backup, one restore, one transaction boundary.** The queue lives - beside the business tables in the same PostgreSQL instance, so a `pg_dump` captures - both consistently, `docker compose exec db psql` inspects the queue with the same tool - as everything else, and `enqueue_after_transaction_commit` gives a real ordering - guarantee between a committed event and the job that reacts to it. With a broker, - "committed but not enqueued" and "enqueued but not committed" both become states - someone has to reason about. + work list from those rows on a nominal 60-second schedule, which is why ADR 0008 can call + the enqueue a *hint*. Removing queued enrichment hints does not erase eligible pending + entity state; a later successful reconciler tick can derive class-scoped work again. + This does not claim that arbitrary queue history or operational state is reconstructible. +2. **One PostgreSQL service and toolchain, with an explicit two-database boundary.** The + queue and business databases share one server and are inspected with the same SQL tools, + but they remain separate backup/restore units: `pg_dump` targets one database and cannot + create a transactionally consistent snapshot across both. Likewise, + `enqueue_after_transaction_commit` orders enqueue after the business commit but cannot + make the two writes atomic. The resulting "committed but not enqueued" gap is accepted + and recovered from committed entity state by ADR 0008; "enqueued before commit" is + avoided by the post-commit hook. 3. **A component a reviewer has to run is a component that has to earn its place.** `IMPLEMENTATION_PLAN.md` §17 states the target: small enough to understand, complete enough to trust. Kafka adds a broker, a coordination layer, its own durability @@ -68,8 +70,8 @@ What this buys: - Queue state is inspectable with SQL, which is what makes the crash-recovery drill in the README runnable by hand — truncate `solid_queue_jobs`, restart the worker, and watch reconciliation rebuild the work from committed rows. -- The job system inherits PostgreSQL's durability rather than having its own. There is one - answer to "what survives a crash", not two. +- The job system uses the same PostgreSQL operational toolchain. Business durability remains + in the business database, while queue state has its own database and recovery semantics. What it costs, stated plainly: diff --git a/docs/evidence/2026-07-31-container-kill-recovery.md b/docs/evidence/2026-07-31-container-kill-recovery.md index 20c4c9f..2724592 100644 --- a/docs/evidence/2026-07-31-container-kill-recovery.md +++ b/docs/evidence/2026-07-31-container-kill-recovery.md @@ -2,7 +2,17 @@ Date: 2026-07-31 -Status: First-party observation +Status: Historical first-party observation; superseded for submission gating + +> [!CAUTION] +> **Erratum — do not use the green verdict in this file as the submission gate.** The script +> revision that produced the raw transcript suppressed non-zero fixture ingestion and +> enrichment exits with `|| true`, printed `push_events` counts without asserting equality +> after every recovery, and did not carry `GITHUB_MODE=fixture` into every Compose +> recreation. A recreated worker could therefore default to live mode and spend GitHub +> budget. The transcript is preserved verbatim below as a historical observation. Re-run the +> corrected script against the final default-branch SHA and record that result in the +> external findings report. ## Why this verification exists @@ -99,29 +109,37 @@ application image declares a non-root user which cannot signal postgres. outcome for *that* repository rather than a property of the selection policy. The policy itself is asserted in `spec/services/github/enrichment/redirect_boundary_spec.rb`. -## Every check in this run +## Every check reported by this historical run -The script records a verdict per check and exits non-zero if any failed, so a committed -transcript and a green exit code cannot disagree. This run: **19 checks, all passing.** The -per-check list is in the transcript below, under each phase. +This run reported **19 checks, all passing**. That is what the preserved transcript says, not +a current submission verdict: the erratum above identifies failures that were outside those +19 checks or whose exit statuses were suppressed. ## Reproducing it ```bash GITHUB_MODE=fixture docker compose up --build -d -script/verify_recovery.sh --confirm +GITHUB_MODE=fixture script/verify_recovery.sh --confirm +docker compose exec worker printenv GITHUB_MODE # must print fixture ``` -The script refuses to run under `CI`, without `--confirm`, against a stack whose worker is not -in fixture mode, with `RAILS_ENV` set to anything but `development`, or when the container -names do not match the literals §15 uses. It never runs `docker compose down`, never drops a -database, and never issues psql against a `_test` database. +The corrected script refuses to run under `CI`, without `--confirm`, against a stack whose +worker is not in fixture mode, with `RAILS_ENV` set to anything but `development`, or when the +container names do not match the literals §15 uses. It passes fixture mode to every internal +Compose invocation, checks every fixture command's status, asserts `push_events` equality, +and verifies the recreated worker is still offline. It never runs `docker compose down`, +never drops a database, and never issues psql against a `_test` database. + +The two kill paths remain intentionally distinct. `docker kill` asks the Docker API to stop a +container, so it is a negative control and `restart: unless-stopped` leaves the container +down. The host-PID-namespace kill terminates the main process without recording an operator +stop; that process-crash path must restart automatically. Nothing in the suite, in `config/ci.rb`, in `bin/ci` or in the workflows executes it — `spec/docker_compose_spec.rb` asserts that, so "CI never runs the verification" is a red test rather than a promise. -## Raw transcript +## Raw historical transcript — preserved verbatim Captured by `script/verify_recovery.sh` on the date above. `login`, `display_login`, `full_name`, `api_url`, `avatar_url` and the corpus-miss payload are redacted in place — names diff --git a/fixtures/github/README.md b/fixtures/github/README.md index 4fff4d1..92e21e2 100644 --- a/fixtures/github/README.md +++ b/fixtures/github/README.md @@ -12,8 +12,8 @@ One corpus serves three consumers, which is what §12 requires: 3. `GITHUB_MODE=fixture` at runtime, for the reviewer's offline scenario. It lives at the repository root rather than under `spec/`, because in fixture mode it is -runtime data read by the `web` and `worker` containers. Excluding `spec/` from the image -is a normal future hardening step and must not be able to break the demo. +runtime data read by the `web` and `worker` containers. Keeping runtime fixtures separate +also means an image that excludes test files cannot break the offline demo. ## Layout @@ -109,8 +109,8 @@ commands. ## What is in `bodies/events/page-1.json` -Eight event envelopes, deliberately mixed so PR 5's tolerant parser and quarantine -taxonomy have real material: +Eight event envelopes, deliberately mixed so the tolerant parser and quarantine taxonomy +have real material: | Entry | Event id | Shape | |---|---|---| @@ -129,5 +129,9 @@ absorption across pages is exercisable. The `PushEvent` payloads carry exactly the five fields the post-2025-10-07 API documents — `repository_id`, `push_id`, `ref`, `head`, `before` — and no `commits` array. -**What each of those becomes once processed is PR 5's to define and assert.** This file -describes the corpus, not outcomes that no code produces yet. +The shipped processor outcomes are asserted in +`spec/services/github/ingestion_runner_spec.rb`: four event rows, one ignored non-push +event, and three quarantined envelopes with the classifications described above. Entity +enrichment outcomes are asserted in +`spec/services/github/enrichment/end_to_end_spec.rb`. This file remains the corpus contract; +the specs are the executable outcome contract. diff --git a/script/verify_recovery.sh b/script/verify_recovery.sh index a507b84..943bff8 100755 --- a/script/verify_recovery.sh +++ b/script/verify_recovery.sh @@ -70,6 +70,10 @@ fi PHASE="all" CONFIRMED="" +# Never inherit GITHUB_FIXTURE_SCENARIO from the caller. The baseline and every recovery +# operation use the default corpus unless a scenario phase creates a dynamically scoped +# override with this verifier-only variable. +RECOVERY_FIXTURE_SCENARIO="default" for argument in "$@"; do case "$argument" in @@ -145,16 +149,41 @@ fi # Plumbing # --------------------------------------------------------------------------------------- +# This verifier is an offline operation even when the caller exported neither variable — or +# exported a non-default scenario. Pinning every Compose invocation here prevents `compose up +# -d worker` from recreating the preflighted fixture worker in live mode and makes the baseline +# independent of the caller's shell. Preflight still inspects the environment inside the +# already-running worker; this wrapper changes interpolation, not what `printenv` reports from +# an existing container. +compose() { + GITHUB_MODE=fixture GITHUB_FIXTURE_SCENARIO="$RECOVERY_FIXTURE_SCENARIO" docker compose "$@" +} + +# If an unexpected command fails after test isolation stops the worker, the EXIT trap is the +# last line of defence against leaving the reviewer's stack disabled. The phase also performs +# and verifies an explicit restart on its normal path; this is only emergency cleanup. +TEST_WORKER_RESTART_PENDING=0 +restore_test_worker_on_exit() { + if [ "$TEST_WORKER_RESTART_PENDING" = "1" ]; then + if compose start worker >/dev/null 2>&1; then + TEST_WORKER_RESTART_PENDING=0 + else + echo "warning: could not restore the worker stopped for test isolation" >&2 + fi + fi +} +trap restore_test_worker_on_exit EXIT + # Every psql call in this file goes through one of these two functions, and both hard-code a # _development database name. There is deliberately no helper that takes a database # argument: §16 gates that the test databases are never touched by anything but the suite, # and the cheapest way to keep that true is to make the wrong database unspellable here. psql_dev() { - docker compose exec -T db psql -U postgres -d "$DEV_DB" -tAc "$1" | tr -d '[:space:]' + compose exec -T db psql -U postgres -d "$DEV_DB" -tAc "$1" | tr -d '[:space:]' } psql_queue_dev() { - docker compose exec -T db psql -U postgres -d "$DEV_QUEUE_DB" -tAc "$1" | tr -d '[:space:]' + compose exec -T db psql -U postgres -d "$DEV_QUEUE_DB" -tAc "$1" | tr -d '[:space:]' } # Exactly one id, always. `docker compose ps -q` lists running containers only, and every @@ -163,8 +192,8 @@ psql_queue_dev() { # prefer the running container, fall back to the most recent stopped one. container_id() { local id - id="$(docker compose ps -q "$1" 2>/dev/null | head -1)" - [ -n "$id" ] || id="$(docker compose ps -aq "$1" 2>/dev/null | head -1)" + id="$(compose ps -q "$1" 2>/dev/null | head -1)" + [ -n "$id" ] || id="$(compose ps -aq "$1" 2>/dev/null | head -1)" printf '%s' "$id" } @@ -222,6 +251,24 @@ live() { curl --silent --show-error --fail -o /dev/null "$LIVE_URL" } +worker_mode() { + compose exec -T worker printenv GITHUB_MODE | tr -d '[:space:]' +} + +worker_fixture_scenario() { + compose exec -T worker printenv GITHUB_FIXTURE_SCENARIO | tr -d '[:space:]' +} + +verify_final_worker_mode() { + local mode + + if mode="$(worker_mode 2>/dev/null)"; then + check "the worker is still in fixture mode before verifier exit" "fixture" "$mode" + else + check "the worker is still in fixture mode before verifier exit" "fixture" "unavailable" + fi +} + push_events() { psql_dev "SELECT COUNT(*) FROM push_events;"; } actors() { psql_dev "SELECT COUNT(*) FROM github_actors;"; } repositories() { psql_dev "SELECT COUNT(*) FROM github_repositories;"; } @@ -281,6 +328,18 @@ redact() { -e 's#("api_url":")[^"]*#\1#g' } +# A redacted transcript still has to retain the command's real exit status. `set +e` lets the +# transcript continue far enough to print a final verdict; PIPESTATUS records Compose rather +# than sed, and each caller turns a non-zero value into a failed check immediately after the +# closing code fence. +LAST_COMPOSE_STATUS=0 +run_redacted_compose() { + set +e + compose "$@" 2>&1 | redact + LAST_COMPOSE_STATUS="${PIPESTATUS[0]}" + set -e +} + # --------------------------------------------------------------------------------------- # preflight # --------------------------------------------------------------------------------------- @@ -302,8 +361,8 @@ preflight() { # puts third-party logins, avatar URLs embedding numeric user ids, and repository names # into `docker compose logs worker` — which this script then prints into a transcript # somebody commits. - local mode - mode="$(docker compose exec -T worker printenv GITHUB_MODE | tr -d '[:space:]')" + local mode scenario + mode="$(worker_mode)" if [ "$mode" != "fixture" ]; then echo "refusing: the worker is running in GITHUB_MODE=${mode:-unset}, not fixture." >&2 @@ -311,6 +370,13 @@ preflight() { exit 2 fi + scenario="$(worker_fixture_scenario)" + if [ "$scenario" != "default" ]; then + echo "refusing: the worker is running GITHUB_FIXTURE_SCENARIO=${scenario:-unset}, not default." >&2 + echo " recreate the offline stack without an inherited fixture scenario." >&2 + exit 2 + fi + # The shared compose anchor reads RAILS_ENV: ${RAILS_ENV:-development}, so a reviewer with # RAILS_ENV=test exported gets `docker compose run --rm ingest` writing into the *test* # databases — every documented count wrong, and the §16 isolation gate broken by the very @@ -352,26 +418,35 @@ preflight() { phase_baseline() { heading "Baseline" + local ingest_status enrich_status + echo '```' - GITHUB_MODE=fixture docker compose run --rm ingest 2>&1 | redact || true + run_redacted_compose run --rm ingest + ingest_status="$LAST_COMPOSE_STATUS" echo '```' echo + check "baseline fixture ingestion exited successfully" "0" "$ingest_status" + echo + echo '```' - GITHUB_MODE=fixture docker compose run --rm enrich --limit 6 2>&1 | redact || true + run_redacted_compose run --rm enrich --limit 6 + enrich_status="$LAST_COMPOSE_STATUS" echo '```' echo - # Expected, and worth keeping in the transcript rather than engineering around: a - # development database that has previously polled live GitHub holds actors and repositories - # whose api_urls are real. Fixture mode has no entry for them, so the cycle refuses with a - # corpus gap and exit 2 instead of quietly reaching api.github.com. That is §6's fail-closed - # rule holding in the running stack, which no unit test can show. - echo "A corpus gap above is not a fault. It means this development database holds entities" - echo "from an earlier live run, and fixture mode refused to fetch them rather than falling" - echo "back to the network — exit 2, \"refused to run\", per §9's exit-code contract." + check "baseline fixture enrichment exited successfully" "0" "$enrich_status" echo + if [ "$ingest_status" -ne 0 ] || [ "$enrich_status" -ne 0 ]; then + # A corpus gap is worth retaining in a failed transcript: it proves fixture mode refused + # to fall back to api.github.com. It is nevertheless a failed authoritative baseline. + echo "If the output above reports a corpus gap, fixture mode failed closed instead of using" + echo "the network. Its non-zero exit still fails this verification; rerun the authoritative" + echo "baseline from the documented empty fixture volume." + echo + fi + if [ "$MODE" = "absolute" ]; then echo "The development database was empty at the start of this run, so the counts below are" echo "the README's documented absolutes." @@ -404,7 +479,7 @@ api_kill_observation() { echo "\$ docker compose ps" echo '```' - docker compose ps + compose ps echo '```' echo @@ -433,8 +508,14 @@ api_kill_observation() { echo echo "\$ docker compose up -d ${service}" - docker compose up -d "$service" >/dev/null 2>&1 + compose up -d "$service" >/dev/null 2>&1 echo + + if [ "$service" = "worker" ]; then + wait_for "the recreated worker container to be running" 60 running worker + check "the recreated worker remains in fixture mode" "fixture" "$(worker_mode)" + echo + fi } # The crash the restart policy actually exists for: SIGKILL delivered to the container's main @@ -447,8 +528,9 @@ api_kill_observation() { crash_recovery_observation() { local service="$1" wait_description="$2" wait_timeout="$3" shift 3 + CRASH_OBSERVATION_COMPLETED=0 - local pid before_started before_restarts + local pid before_started before_restarts helper_status pid="$(docker inspect -f '{{.State.Pid}}' "$(container_id "$service")")" before_started="$(started_at "$service")" before_restarts="$(restart_count "$service")" @@ -459,10 +541,21 @@ crash_recovery_observation() { # a helper running as that user can signal the worker and web processes (same uid) but not # postgres, which runs as its own user inside the db container — `kill` answers "Operation # not permitted" and only the database phase fails, which is a confusing way to find out. - if ! docker run --rm --pid=host --privileged --user 0 "$APP_IMAGE" sh -c "kill -9 ${pid}" >/dev/null 2>&1; then + if docker run --rm --pid=host --privileged --user 0 "$APP_IMAGE" \ + sh -c "kill -9 ${pid}" >/dev/null 2>&1; then + helper_status=0 + else + helper_status=$? + fi + + check "the ${service} process-crash helper exited successfully" "0" "$helper_status" + + if [ "$helper_status" -ne 0 ]; then echo - echo "The crash-kill helper exited non-zero. The observation below is still taken, but" - echo "treat it as inconclusive rather than as evidence the policy worked." + echo "The crash-kill helper exited non-zero, so no restart-policy observation is claimed." + echo "The failed check remains in the final verdict; later independent checks still run." + echo + return 0 fi echo @@ -483,6 +576,7 @@ crash_recovery_observation() { check "${service} was restarted by its policy after a process crash" "true" "$running" check "${service}'s RestartCount incremented" \ "$((before_restarts + 1))" "$after_restarts" + CRASH_OBSERVATION_COMPLETED=1 echo echo "Docker restarted \`${service}\` on its own. No operator step." echo @@ -491,7 +585,7 @@ crash_recovery_observation() { phase_kill_worker() { heading "Worker kill (§15 step 8)" - local before + local before after_api after_crash before="$(push_events)" echo "Count before the kill: push_events = ${before}" echo @@ -499,25 +593,40 @@ phase_kill_worker() { echo "### Part 1 — the command §15 documents" echo api_kill_observation worker - wait_for "the worker container to be running again" 60 running worker + + after_api="$(push_events)" + echo "Count after API-stop recovery: push_events = ${after_api}" + check "push_events is unchanged across the worker API stop and operator recovery" \ + "$before" "$after_api" + echo echo "### Part 2 — a process crash, which is what the policy is for" echo crash_recovery_observation worker "the worker container to be running again" 60 running worker + after_crash="$(push_events)" + if [ "$CRASH_OBSERVATION_COMPLETED" = "1" ]; then + echo "Count after process-crash recovery: push_events = ${after_crash}" + check "push_events is unchanged across the worker process crash and policy recovery" \ + "$after_api" "$after_crash" + check "the policy-restarted worker remains in fixture mode" "fixture" "$(worker_mode)" + else + echo "Count after the failed process-crash attempt: push_events = ${after_crash}" + check "push_events is unchanged during the failed worker process-crash attempt" \ + "$after_api" "$after_crash" + fi + echo + echo "\$ docker compose logs worker --since 2m" echo '```' - docker compose logs worker --since 2m --no-log-prefix 2>/dev/null | tail -20 | redact + compose logs worker --since 2m --no-log-prefix 2>/dev/null | tail -20 | redact echo '```' - echo - - echo "Count after recovery: push_events = $(push_events)" } phase_kill_db() { heading "Database kill (§15 step 8)" - local before + local before after_api after_crash before="$(push_events)" echo "Count before the kill: push_events = ${before}" echo @@ -528,25 +637,46 @@ phase_kill_db() { wait_for "the database to report healthy again" 180 db_healthy wait_for "/health/ready to return 200 once the database is back" 180 ready - echo "### Part 2 — a process crash, which is what the policy is for" + after_api="$(push_events)" + echo "Count after API-stop recovery: push_events = ${after_api}" + check "push_events is unchanged across the database API stop and operator recovery" \ + "$before" "$after_api" echo - crash_recovery_observation db "the database to report healthy again" 180 db_healthy - wait_for "/health/ready to return 200 once the database is back" 180 ready - local after - after="$(push_events)" - echo "Count after recovery: push_events = ${after}" + echo "### Part 2 — a process crash, which is what the policy is for" echo + crash_recovery_observation db "the database to report healthy again" 180 db_healthy + if [ "$CRASH_OBSERVATION_COMPLETED" = "1" ]; then + wait_for "/health/ready to return 200 once the database is back" 180 ready + fi - check "push_events is unchanged across two SIGKILLs of the database" "$before" "$after" + after_crash="$(push_events)" + if [ "$CRASH_OBSERVATION_COMPLETED" = "1" ]; then + echo "Count after process-crash recovery: push_events = ${after_crash}" + check "push_events is unchanged across the database process crash and policy recovery" \ + "$after_api" "$after_crash" + else + echo "Count after the failed process-crash attempt: push_events = ${after_crash}" + check "push_events is unchanged during the failed database process-crash attempt" \ + "$after_api" "$after_crash" + fi echo - echo "PostgreSQL replayed its WAL onto the same named volume, which is §15 step 8's actual" - echo "question." + if [ "$CRASH_OBSERVATION_COMPLETED" = "1" ]; then + echo "PostgreSQL replayed its WAL onto the same named volume, which is §15 step 8's actual" + echo "question." + else + echo "The crash helper failed, so this run makes no WAL-replay or automatic-restart claim." + fi } phase_kill_web() { heading "Web kill (beyond §15's list — verifies the Dockerfile's pid-file claim)" + local before after_api after_crash + before="$(push_events)" + echo "Count before the kill: push_events = ${before}" + echo + echo "§16's durability gate names db, web and worker; §15 step 8 kills only two. The" echo "Dockerfile runs puma directly rather than \`bin/rails server\` so that a stale" echo "tmp/pids/server.pid left by an ungraceful kill cannot block the restart" @@ -558,12 +688,34 @@ phase_kill_web() { api_kill_observation web wait_for "/health/ready to return 200 again" 180 ready + after_api="$(push_events)" + echo "Count after API-stop recovery: push_events = ${after_api}" + check "push_events is unchanged across the web API stop and operator recovery" \ + "$before" "$after_api" + echo + echo "### Part 2 — a process crash, which is what the policy is for" echo crash_recovery_observation web "/health/ready to return 200 from the restarted container" 180 ready - echo "\`/health/live\` and \`/health/ready\` both answer again after an uncooperative kill, so no" - echo "pid file survived to block the restart." + after_crash="$(push_events)" + if [ "$CRASH_OBSERVATION_COMPLETED" = "1" ]; then + echo "Count after process-crash recovery: push_events = ${after_crash}" + check "push_events is unchanged across the web process crash and policy recovery" \ + "$after_api" "$after_crash" + else + echo "Count after the failed process-crash attempt: push_events = ${after_crash}" + check "push_events is unchanged during the failed web process-crash attempt" \ + "$after_api" "$after_crash" + fi + echo + + if [ "$CRASH_OBSERVATION_COMPLETED" = "1" ]; then + echo "\`/health/live\` and \`/health/ready\` both answer again after an uncooperative kill, so no" + echo "pid file survived to block the restart." + else + echo "The crash helper failed, so this run makes no web process-crash recovery claim." + fi } phase_restart() { @@ -575,7 +727,7 @@ phase_restart() { echo "\$ docker compose restart" started="$(date +%s)" echo '```' - docker compose restart + compose restart echo '```' elapsed=$(( $(date +%s) - started )) echo @@ -586,12 +738,21 @@ phase_restart() { wait_for "/health/ready to return 200 after the restart" 120 ready - echo "push_events before: ${before}, after: $(push_events)" + local after + after="$(push_events)" + echo "push_events before: ${before}, after: ${after}" + echo + check "push_events is unchanged across a normal Compose restart" "$before" "$after" + check "the normally restarted worker remains in fixture mode" "fixture" "$(worker_mode)" } phase_test_isolation() { heading "Test isolation (§16's reviewer-experience gate)" + local stop_status test_status worker_start_status + local before_primary before_queue before_setup after_primary after_queue after_setup + local before_primary_status before_queue_status after_primary_status after_queue_status + echo "§16 requires that \`docker compose run --rm test\` never touches the development" echo "databases (app or queue) and never triggers the development \`setup\` service." echo "spec/docker_compose_spec.rb asserts the declarations; this measures the behaviour." @@ -605,27 +766,78 @@ phase_test_isolation() { # 56 -> 62 and still printed that the gate held. Stopping the worker removes the only other # writer, so a difference here can mean nothing except the suite. echo "\$ docker compose stop worker # the only other writer to these databases" - docker compose stop worker >/dev/null 2>&1 + if compose stop worker >/dev/null 2>&1; then + stop_status=0 + else + stop_status=$? + fi + check "the worker stopped for the test-isolation measurement" "0" "$stop_status" + + if [ "$stop_status" -ne 0 ]; then + echo + echo "The worker could not be stopped, so the isolation measurement is skipped rather" + echo "than producing counts with a concurrent writer. The final verdict remains failed." + return 0 + fi + + TEST_WORKER_RESTART_PENDING=1 sleep 3 echo - local before_primary before_queue before_setup - before_primary="$(push_events)" - before_queue="$(queue_jobs)" + if before_primary="$(push_events)"; then + before_primary_status=0 + else + before_primary_status=$? + before_primary="unavailable" + fi + + if before_queue="$(queue_jobs)"; then + before_queue_status=0 + else + before_queue_status=$? + before_queue="unavailable" + fi + before_setup="$(docker inspect -f '{{.State.FinishedAt}}' "$(container_id setup)" 2>/dev/null || echo "absent")" echo '```' - docker compose run --rm test 2>&1 | tail -8 + run_redacted_compose run --rm test + test_status="$LAST_COMPOSE_STATUS" echo '```' echo - local after_primary after_queue after_setup - after_primary="$(push_events)" - after_queue="$(queue_jobs)" + if after_primary="$(push_events)"; then + after_primary_status=0 + else + after_primary_status=$? + after_primary="unavailable" + fi + + if after_queue="$(queue_jobs)"; then + after_queue_status=0 + else + after_queue_status=$? + after_queue="unavailable" + fi + after_setup="$(docker inspect -f '{{.State.FinishedAt}}' "$(container_id setup)" 2>/dev/null || echo "absent")" echo "\$ docker compose start worker" - docker compose start worker >/dev/null 2>&1 + if compose start worker >/dev/null 2>&1 && \ + wait_for "the worker stopped for test isolation to be running again" 60 running worker; then + worker_start_status=0 + TEST_WORKER_RESTART_PENDING=0 + else + worker_start_status=$? + fi + echo + + check "the test service exited successfully" "0" "$test_status" + check "the worker restarted after the test-isolation measurement" "0" "$worker_start_status" + check "push_events could be measured before the suite" "0" "$before_primary_status" + check "the development queue could be measured before the suite" "0" "$before_queue_status" + check "push_events could be measured after the suite" "0" "$after_primary_status" + check "the development queue could be measured after the suite" "0" "$after_queue_status" echo echo "| observable | before | after |" @@ -661,7 +873,7 @@ phase_scenarios() { # matching id, which RepositoryDocument.parse checks), and the worker is stopped for the # duration so nothing else can move last_seen_at underneath the selection. echo "\$ docker compose stop worker # so nothing re-orders the candidate set mid-scenario" - docker compose stop worker >/dev/null 2>&1 + compose stop worker >/dev/null 2>&1 sleep 3 echo @@ -671,7 +883,7 @@ phase_scenarios() { "an off-host redirect is refused by the URL policy" echo "\$ docker compose start worker" - docker compose start worker >/dev/null 2>&1 + compose start worker >/dev/null 2>&1 echo count_header @@ -681,6 +893,8 @@ phase_scenarios() { # One redirect scenario, end to end, with a verdict rather than a hope. redirect_scenario() { local scenario="$1" expected_status="$2" description="$3" + local RECOVERY_FIXTURE_SCENARIO="$scenario" + local enrich_status echo "### ${scenario} — ${description}" echo @@ -706,11 +920,14 @@ redirect_scenario() { fi echo '```' - GITHUB_MODE=fixture GITHUB_FIXTURE_SCENARIO="$scenario" \ - docker compose run --rm enrich --limit 1 --class repository 2>&1 | redact || true + run_redacted_compose run --rm enrich --limit 1 --class repository + enrich_status="$LAST_COMPOSE_STATUS" echo '```' echo + check "${scenario} fixture enrichment exited successfully" "0" "$enrich_status" + echo + local actual actual="$(psql_dev "SELECT enrichment_status FROM github_repositories WHERE github_id = ${CORPUS_REPOSITORY_ID};")" @@ -721,15 +938,22 @@ redirect_scenario() { phase_rate_limit() { heading "Rate-limit scenario (opt-in)" + local RECOVERY_FIXTURE_SCENARIO="rate_limited" + local ingest_status + echo "\`rate_limited\` writes a real one-hour \`global_blocked_until\`. It is never part of the" echo "default run, because every later phase would report a deferral. Cleanup runs immediately." echo echo '```' - GITHUB_MODE=fixture GITHUB_FIXTURE_SCENARIO=rate_limited docker compose run --rm ingest || true + run_redacted_compose run --rm ingest + ingest_status="$LAST_COMPOSE_STATUS" echo '```' echo + check "rate-limit fixture ingestion exited successfully" "0" "$ingest_status" + echo + echo "Budget row after the scenario:" echo '```' psql_dev "SELECT window_status || ' blocked_until=' || COALESCE(global_blocked_until::text, 'none') FROM github_api_budget;" @@ -793,7 +1017,7 @@ Verification date: $(date -u +%Y-%m-%d) Compose project: ${PROJECT} Git revision: $(git rev-parse HEAD 2>/dev/null || echo "unknown") Docker version: $(docker version -f '{{.Server.Version}}') -Compose version: $(docker compose version --short) +Compose version: $(compose version --short) Host: $(uname -srm) Worker mode: fixture (enforced by preflight) Count mode: ${MODE} @@ -830,6 +1054,8 @@ case "$PHASE" in ;; esac +verify_final_worker_mode + heading "Verdict" if [ "$FAILURES" -eq 0 ]; then diff --git a/spec/docker_compose_spec.rb b/spec/docker_compose_spec.rb index 2cbd44d..f812018 100644 --- a/spec/docker_compose_spec.rb +++ b/spec/docker_compose_spec.rb @@ -168,11 +168,11 @@ def unprofiled end end - # Everything below is what §16's durability gate — "Docker restart policies recover crashed - # db/web/worker containers automatically (verified by container kills)" — actually rests on. - # script/verify_recovery.sh proves the runtime behaviour once, on one host, on one date. - # These examples prove the declarations that behaviour comes from, on every push, and they - # are the reason a compose edit cannot silently invalidate the committed transcript. + # Everything below is what §16's durability gate for db/web/worker actually rests on. + # script/verify_recovery.sh records `docker kill` as an API-stop negative control, then uses + # a host-PID SIGKILL to verify automatic process-crash recovery on one host and date. These + # examples prove the declarations behind that behaviour on every push, so a compose edit + # cannot silently invalidate the committed transcript. describe "the db service" do let(:db) { services.fetch("db") } @@ -414,11 +414,12 @@ def unprofiled # network boundary, and this script makes no network calls at all — fixture mode is one of # its preflight requirements. describe "script/verify_recovery.sh" do - it "exists and is executable, because the README tells a reviewer to run it" do - script = Rails.root.join("script/verify_recovery.sh") + let(:script_path) { Rails.root.join("script/verify_recovery.sh") } + let(:script_source) { script_path.read } - expect(script).to exist - expect(script).to be_executable + it "exists and is executable, because the README tells a reviewer to run it" do + expect(script_path).to exist + expect(script_path).to be_executable end it "is referenced by no workflow, no compose service, and no CI entry point" do @@ -433,5 +434,99 @@ def unprofiled expect(referencing).to be_empty end + + it "forces every internal Compose invocation to fixture mode without replacing preflight" do + expect(script_source).to include(<<~'SHELL'.strip) + compose() { + GITHUB_MODE=fixture GITHUB_FIXTURE_SCENARIO="$RECOVERY_FIXTURE_SCENARIO" docker compose "$@" + } + SHELL + expect(script_source).to include('RECOVERY_FIXTURE_SCENARIO="default"') + + # Remove the quoted usage document, then admit only comments and pure echo statements + # that cannot evaluate a command substitution. Everything else is executable shell, so + # this catches direct, environment-prefixed and negated Compose calls alike. + without_usage = script_source.sub(/cat >&2 <<'USAGE'\n.*?^USAGE\n/m, "") + compose_lines = without_usage.lines.select { |line| line.include?("docker compose") } + executable_compose_lines = compose_lines.reject do |line| + stripped = line.strip + safe_echo = stripped.match?(/\Aecho (?:".*"|'.*')(?:\s+>&2)?\z/) && + !stripped.include?("$(") && !stripped.match?(/(?&1 | redact') + expect(script_source).to include('LAST_COMPOSE_STATUS="${PIPESTATUS[0]}"') + expect(script_source).to include( + 'check "baseline fixture ingestion exited successfully" "0" "$ingest_status"', + 'check "baseline fixture enrichment exited successfully" "0" "$enrich_status"' + ) + + executable_one_shots = script_source.lines.grep(/run --rm (?:ingest|enrich)/) + .reject { |line| line.lstrip.start_with?("#", "echo") } + expect(executable_one_shots).to all(satisfy { |line| !line.include?("|| true") }) + end + + it "records a failed test service and restores the worker before continuing" do + expect(script_source).to include( + "trap restore_test_worker_on_exit EXIT", + "run_redacted_compose run --rm test", + 'test_status="$LAST_COMPOSE_STATUS"', + 'check "the test service exited successfully" "0" "$test_status"', + 'check "the worker restarted after the test-isolation measurement" "0" "$worker_start_status"', + "TEST_WORKER_RESTART_PENDING=0" + ) + expect(script_source).not_to include("compose run --rm test 2>&1 | tail") + end + + it "asserts event-count preservation for every recovery phase" do + expect(script_source).to include( + "push_events is unchanged across the worker API stop and operator recovery", + "push_events is unchanged across the worker process crash and policy recovery", + "push_events is unchanged across the database API stop and operator recovery", + "push_events is unchanged across the database process crash and policy recovery", + "push_events is unchanged across the web API stop and operator recovery", + "push_events is unchanged across the web process crash and policy recovery", + "push_events is unchanged across a normal Compose restart" + ) + end + + it "keeps API stops distinct from process crashes" do + expect(script_source).to include( + "`docker kill` is an API stop", + "SIGKILL delivered to the container's main", + "process from *outside* its PID namespace", + 'check "the ${service} process-crash helper exited successfully" "0" "$helper_status"', + "CRASH_OBSERVATION_COMPLETED=0", + "CRASH_OBSERVATION_COMPLETED=1" + ) + end + + it "asserts fixture mode again immediately before the final verdict" do + expect(script_source).to include( + 'check "the worker is still in fixture mode before verifier exit" "fixture" "$mode"', + "verify_final_worker_mode\n\nheading \"Verdict\"" + ) + end end end diff --git a/spec/recovery/duplicate_job_execution_spec.rb b/spec/recovery/duplicate_job_execution_spec.rb index 9695049..a002eba 100644 --- a/spec/recovery/duplicate_job_execution_spec.rb +++ b/spec/recovery/duplicate_job_execution_spec.rb @@ -1,9 +1,10 @@ require "rails_helper" -# §12's "Enrichment job executed twice", which is §8's processing semantics stated as a test: -# at-least-once execution + idempotent writes + unique constraints = effectively-once -# persisted outcomes. Never exactly-once execution — the second execution really happens here, -# and what is asserted is that it changes nothing. +# §12's "Enrichment job executed twice" exercises one redelivery case. The ingestion-wide +# guarantees are narrower: a duplicate event ID cannot create another push_events row or +# reactivate a skipped entity. Executions, run summaries, quarantine counters, budget use, +# and logs can repeat. Here the second execution really happens, and this scenario asserts +# only that an already-complete actor is left unchanged by the freshness check. # # The redelivery is modelled as the *same job instance* performed twice: one job id, two # executions, which is what Solid Queue produces when a worker dies after the job ran and diff --git a/spec/requests/api/push_events_spec.rb b/spec/requests/api/push_events_spec.rb index ec2626c..71ce51c 100644 --- a/spec/requests/api/push_events_spec.rb +++ b/spec/requests/api/push_events_spec.rb @@ -114,8 +114,8 @@ end # Both identifiers are numeric strings, so :id is genuinely ambiguous and only one - # reading can be right. github_event_id is the unique index the idempotency story rests - # on and the identifier §11 puts on every log line — which is what makes "log line -> + # reading can be right. github_event_id is the unique index that prevents a second event + # row and the identifier §11 puts on every log line — which is what makes "log line -> # record" a URL a reviewer can type. it "resolves :id as the GitHub event id, never as the surrogate primary key" do get "/api/push_events/#{event.id}" diff --git a/spec/services/github/budget_ledger_spec.rb b/spec/services/github/budget_ledger_spec.rb index c5f00a3..6e3fa8a 100644 --- a/spec/services/github/budget_ledger_spec.rb +++ b/spec/services/github/budget_ledger_spec.rb @@ -372,7 +372,7 @@ def superseding_snapshot end end - it "rolls the window exactly once, however the move is detected" do + it "emits one window-roll transition when the response advances the window" do active_window ledger.reserve!(:poll, now: window_reset - 1) diff --git a/spec/services/github/enrichment/end_to_end_spec.rb b/spec/services/github/enrichment/end_to_end_spec.rb index c996005..7e9b7fd 100644 --- a/spec/services/github/enrichment/end_to_end_spec.rb +++ b/spec/services/github/enrichment/end_to_end_spec.rb @@ -36,14 +36,14 @@ def enrich!(cycles: 1, **arguments) expect(GithubRepository.count).to eq(3) end - it "enriches every actor the corpus can resolve" do + it "enriches both resolvable actors in the finite fixture corpus" do expect(GithubActor.find_by(github_id: 583_231)) .to have_attributes(enrichment_status: "complete", name: "The Octocat", fetched_at: now) expect(GithubActor.find_by(github_id: 1_024_025)) .to have_attributes(enrichment_status: "complete", name: "Mona Lisa Octocat") end - it "enriches every repository with §7's four enrichment-owned columns" do + it "writes §7's enrichment-owned columns for the fixture's resolved repository" do expect(GithubRepository.find_by(github_id: 1_296_269)).to have_attributes( enrichment_status: "complete", description: "My first repository on GitHub!", language: "Ruby", owner_github_id: 583_231 diff --git a/spec/services/github/enrichment/fairness_stress_spec.rb b/spec/services/github/enrichment/fairness_stress_spec.rb index 999ffb3..c7d652e 100644 --- a/spec/services/github/enrichment/fairness_stress_spec.rb +++ b/spec/services/github/enrichment/fairness_stress_spec.rb @@ -74,7 +74,7 @@ def flood_actors(count) end # Story 3's requirement, measured over a window rather than asserted about one choice. - it "enriches every actor candidate, which is what §10 says a repo-first policy would not" do + it "serves all three actor candidates in this finite contention setup" do expect(sequence.count { |(key, _)| key == :actor }).to eq(3) end @@ -93,7 +93,7 @@ def flood_actors(count) expect(first_borrow).to be > last_actor end - it "spends the window exactly once, with the two shares summing to the class counter" do + it "records all 40 debits with the two shares summing to the class counter" do budget = current_budget expect(budget.enrichment_used).to eq(40) @@ -136,7 +136,7 @@ def flood_actors(count) drain! end - it "still enriches every repository candidate" do + it "serves all three repository candidates in this finite contention setup" do expect(sequence.count { |(key, _)| key == :repository }).to eq(3) end @@ -155,7 +155,7 @@ def flood_actors(count) expect(borrowed).to eq(40 - 20 - 3) end - it "spends the window exactly once, mirrored" do + it "records all 40 debits with the mirrored share split" do budget = current_budget expect(budget.enrichment_used).to eq(40) diff --git a/spec/services/github/ingestion_runner_spec.rb b/spec/services/github/ingestion_runner_spec.rb index 4cfef4f..1312c1d 100644 --- a/spec/services/github/ingestion_runner_spec.rb +++ b/spec/services/github/ingestion_runner_spec.rb @@ -376,8 +376,8 @@ def ingest(runner = fixture_runner, **options) end # §10 makes every retry its own reservation "through the same gate and ledger", so the - # ledger is where the retries are visible — and the page must be processed exactly once. - it "processes a page recovered by retries exactly once" do + # ledger exposes all three attempts while only the recovered response reaches processing. + it "processes the recovered response after two failed request attempts" do result = ingest(fixture_runner(transport: fixture_transport(scenario: "transient_failure"))) expect(result).to be_completed diff --git a/spec/services/github/status/snapshot_spec.rb b/spec/services/github/status/snapshot_spec.rb index ae0e308..edd82c1 100644 --- a/spec/services/github/status/snapshot_spec.rb +++ b/spec/services/github/status/snapshot_spec.rb @@ -192,7 +192,7 @@ def payload # The reason this is one aggregate rather than StateSummary and Summary side by side. # Three independent reads could straddle a committing reservation and produce a body # whose poll block contradicts its ledger block. - it "reads the ledger row exactly once" do + it "performs one ledger-row read for a snapshot" do active_budget_window(now: now) allow(GithubApiBudget).to receive(:find_by).and_call_original diff --git a/spec/stress/budget_ledger_spec.rb b/spec/stress/budget_ledger_spec.rb index 9576f19..e0d7acb 100644 --- a/spec/stress/budget_ledger_spec.rb +++ b/spec/stress/budget_ledger_spec.rb @@ -67,7 +67,7 @@ def reserve_all(count, request_class, borrow: false) expect(budget.poll_used).to eq(8) end - it "counts every granted reservation exactly once, with no lost debits" do + it "records one debit per granted reservation, with no lost debits" do active_window(poll_allowance: 30) outcomes = reserve_all(30, :poll) @@ -150,7 +150,7 @@ def reserve_all(count, request_class, borrow: false) # second caller arriving mid-roll must either wait for it or find it already done. The # failure this rules out is two rollovers — which would zero a counter that had already # been debited in the new window. - it "rolls exactly once, and every reservation lands in the new window" do + it "performs one rollover before every reservation lands in the new window" do active_window(poll_allowance: 12, reset_at: now - 1, poll_used: 12) outcomes = reserve_all(12, :poll) diff --git a/spec/support/shared_examples/enrichable_entity.rb b/spec/support/shared_examples/enrichable_entity.rb index 23a22f1..7b6b3db 100644 --- a/spec/support/shared_examples/enrichable_entity.rb +++ b/spec/support/shared_examples/enrichable_entity.rb @@ -191,7 +191,7 @@ def skip!(**overrides) expect(record.reload).to have_attributes(enrichment_status: "pending", skipped_at: nil) end - # §7 line 572: "A complete enrichment is not reset to pending by any duplicate." + # §7 line 572: "An entity in the complete state is not reset to pending by a duplicate." it "leaves every other status alone, so no enriched or terminally failed row is disturbed" do (Enrichable::ENRICHMENT_STATUSES - [ "skipped_budget" ]).each do |status| described_class.where(id: record.id).update_all(enrichment_status: status)