From 668aa424e37b6867bd0d040773b27c9a35ea4ab9 Mon Sep 17 00:00:00 2001 From: Umang Date: Fri, 31 Jul 2026 20:48:26 -0500 Subject: [PATCH 1/2] Submission polish: verification-doc accuracy and final hardening Documentation accuracy against the assignment text (timing inside the verify section, --confirm on documented verify_recovery.sh invocations, explicit Extension A/B/D claims, an append-only-retention limitation, a port-collision troubleshooting row), a worker-restore EXIT-trap fix in phase_scenarios, a CI job for the three gates config/ci.rb already declares, and consistency fixes (Appendix E wording, stale step-7 comments, .claude/ in .gitignore). Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 19 ++++++++++ .gitignore | 3 ++ IMPLEMENTATION_PLAN.md | 3 +- README.md | 35 +++++++++++++++---- app/services/github/ingestion/page_writer.rb | 4 +-- script/verify_recovery.sh | 17 +++++---- .../github/enrichment/end_to_end_spec.rb | 4 +-- 7 files changed, 68 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 349f201..23f53bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,6 +26,25 @@ jobs: # missing. test -f docs/DESIGN_BRIEF.md + # The same three gates config/ci.rb declares for the local bin/ci pipeline, so the + # declared tooling has a CI execution record and cannot silently rot. None of the + # three needs a database: RuboCop and Brakeman are static, and bundler-audit reads + # the lockfile against the freshly cloned advisory database. + analysis: + name: Style and security analysis + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + bundler-cache: true + - name: Ruby style (RuboCop) + run: bin/rubocop + - name: Gem vulnerability audit (bundler-audit) + run: bin/bundler-audit + - name: Static security analysis (Brakeman) + run: bin/brakeman --quiet --no-pager --exit-on-warn --exit-on-error + # The real suite runs from PR 3 onward (IMPLEMENTATION_PLAN.md §13). Ruby is pinned # from .ruby-version — the same pin the Dockerfile uses — so CI and the image can # never drift. diff --git a/.gitignore b/.gitignore index 69258ae..0602475 100644 --- a/.gitignore +++ b/.gitignore @@ -23,3 +23,6 @@ /coverage/ /spec/reports/ /spec/examples.txt + +# AI tooling local state — never part of the submission +/.claude/ diff --git a/IMPLEMENTATION_PLAN.md b/IMPLEMENTATION_PLAN.md index 313bdf5..8d596a1 100644 --- a/IMPLEMENTATION_PLAN.md +++ b/IMPLEMENTATION_PLAN.md @@ -1305,7 +1305,8 @@ Sections 1–17 and Appendices A–D preserve the frozen architecture from 2026- 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 +The plan held. Every P0 story and every attempted extension (A, B, D) shipped — C stayed +the declared non-goal it always was — 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. diff --git a/README.md b/README.md index 4452139..f2bf179 100644 --- a/README.md +++ b/README.md @@ -153,6 +153,12 @@ docker compose run --rm test docker compose logs -f ``` +**How long before results appear:** the fixture phases below produce rows immediately. +In live mode the worker's first poll attempt can land within about a minute of startup +and repeats on the default 5-minute cadence, but the public feed itself delivers a push +with a documented latency of 30 seconds to 6 hours — see +[Expected time before records appear](#expected-time-before-records-appear). + 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. @@ -528,9 +534,9 @@ bypass, so allow a minute between successive ingestion scenarios. | `rate_limited` | `GITHUB_FIXTURE_SCENARIO=rate_limited … ingest` | primary exhaustion → `global_blocked_until` | **a real one-hour global block** | The last two leave durable state behind on purpose — that is the behaviour being demonstrated. -`script/verify_recovery.sh --phase=cleanup` runs the SQL under +`script/verify_recovery.sh --confirm --phase=cleanup` runs the SQL under [Recovering from a fixture rate-limit run](#recovering-from-a-fixture-rate-limit-run), and -`--phase=rate-limit` plays the rate-limit scenario and cleans up immediately after. +`--confirm --phase=rate-limit` plays the rate-limit scenario and cleans up immediately after. ## Inspecting the data @@ -1422,6 +1428,7 @@ state can be reconstructed. | `ingest` exits `2` | Unknown option, a refused configuration, or a fixture-corpus gap | `docker compose run --rm ingest --help` | | Worker logs nothing | `setup` did not complete, so `worker` never started | `docker compose logs setup` | | `/health/ready` fails while `/health/live` is fine | Database unreachable or schema not loaded. `web`'s healthcheck curls `/health/live`, so `web` stays green through a `db` outage | `docker compose ps`, `docker compose logs db` | +| `up` fails with `Bind for 0.0.0.0:3000 failed: port is already allocated` | Another process owns host port 3000 | Free port 3000, or edit `web`'s `ports:` mapping in `docker-compose.yml` | | Enrichment stuck at `pending` | The allowance is spent for this window, or the window is not `active` yet | `curl -s localhost:3000/status \| jq .ledger` | ### When a source goes out of service @@ -1476,7 +1483,7 @@ Three levels, least destructive first. once: ```bash -script/verify_recovery.sh --phase=cleanup +script/verify_recovery.sh --confirm --phase=cleanup ``` **Level 2 — drop the business data, keep the volume.** @@ -1521,8 +1528,9 @@ The feed retains 30 days. At `MAX_PAGES_PER_POLL=1` each poll sees at most the newest ~100 events, and the feed moves considerably faster than that. **This service 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. +The nine limitations that follow are consequences of that, of the 60-request hourly +ceiling, and of deliberate scope decisions. None is a gap to be closed later; each is a +stated boundary. **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 @@ -1561,7 +1569,22 @@ no never-enriched candidate is eligible anywhere. **8. Extension C (object storage) was deliberately not attempted.** A decision with a stated reason, not an omission — the remaining budget went to rate-limit correctness, -durability, and reviewer experience. +durability, and reviewer experience. The other optional extensions are implemented: +Extension A (rate limiting and fan-out control) is the class-aware ledger, request gate, +and background processing under [Rate limits and the request +budget](#rate-limits-and-the-request-budget) and [Continuous +ingestion](#continuous-ingestion); Extension B (idempotency and restart safety) is +[Processing guarantees](#processing-guarantees) and [Crash recovery +verification](#crash-recovery-verification); Extension D (testing strategy) is +[Deterministic fixture verification](#deterministic-fixture-verification) and the suite +described there. + +**9. Business tables grow without bound, by design.** `push_events`, `ingestion_runs`, +and `quarantined_events` are append-only — this service is the system of record, and +retention, pruning, and archival were deliberately not built. The 60-request hourly +ceiling keeps the worst case modest (twelve poll attempts and at most ~100 events per +hour); the only shipped pruning is Solid Queue's finished-job cleanup in the queue +database, which holds no business data. ## Development diff --git a/app/services/github/ingestion/page_writer.rb b/app/services/github/ingestion/page_writer.rb index 71c16d1..10e14bd 100644 --- a/app/services/github/ingestion/page_writer.rb +++ b/app/services/github/ingestion/page_writer.rb @@ -152,8 +152,8 @@ def touch_activity(outcome, received_at:) # # §11 puts "reactivated" at INFO, and it belongs there rather than at DEBUG: an # entity coming back from skipped_budget is the observable half of §10's bounded - # backlog, and reviewer verification step 7 asks for exactly this to be absent on a - # replay. + # backlog, and the README's Phase B replay check greps for exactly this event to be + # absent on a replay. def reactivate(outcome, run_id:, received_at:) [ [ GithubActor, outcome.actor_attributes, :github_actor_id ], [ GithubRepository, outcome.repository_attributes, :github_repository_id ] ].each do |model, attributes, log_key| diff --git a/script/verify_recovery.sh b/script/verify_recovery.sh index 9a92e29..c2a1bb1 100755 --- a/script/verify_recovery.sh +++ b/script/verify_recovery.sh @@ -160,16 +160,17 @@ 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. +# If an unexpected command fails after a phase stops the worker — test isolation and the +# fixture scenarios both do — the EXIT trap is the last line of defence against leaving the +# reviewer's stack disabled. Each phase also performs 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 + echo "warning: could not restore the worker this verification stopped" >&2 fi fi } @@ -874,7 +875,9 @@ 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" - compose stop worker >/dev/null 2>&1 + if compose stop worker >/dev/null 2>&1; then + TEST_WORKER_RESTART_PENDING=1 + fi sleep 3 echo @@ -884,7 +887,9 @@ phase_scenarios() { "an off-host redirect is refused by the URL policy" echo "\$ docker compose start worker" - compose start worker >/dev/null 2>&1 + if compose start worker >/dev/null 2>&1; then + TEST_WORKER_RESTART_PENDING=0 + fi echo count_header diff --git a/spec/services/github/enrichment/end_to_end_spec.rb b/spec/services/github/enrichment/end_to_end_spec.rb index 7e9b7fd..3ccc99c 100644 --- a/spec/services/github/enrichment/end_to_end_spec.rb +++ b/spec/services/github/enrichment/end_to_end_spec.rb @@ -150,8 +150,8 @@ def enrich!(cycles: 1, **arguments) expect(actor.reload).to have_attributes(enrichment_status: "pending", skipped_at: nil) end - # §7 rule 4, and reviewer verification step 7: "confirm … that no skipped entity was - # reactivated by the replay". + # §7 rule 4, and the README's Phase B replay check: a duplicate replay must emit no + # enrichment.reactivated event. it "never reactivates a skipped entity on a duplicate replay" do GithubActor.where(github_id: 583_231) .update_all(enrichment_status: "skipped_budget", skipped_at: now) From fc0d61d83f80e427bb83e2275419472ef9582405 Mon Sep 17 00:00:00 2001 From: Umang Date: Fri, 31 Jul 2026 21:03:08 -0500 Subject: [PATCH 2/2] Address review: arm scenario trap before stopping, fix growth arithmetic The scenario phase now arms the worker-restore trap before the stop attempt so an interrupted stop is covered, records stop/start outcomes as checks that fail the verdict instead of swallowing them, and skips the scenarios when the worker could not be stopped. README known limitation 9 now states the correct worst case: twelve polls at ~100 events each is ~1,200 rows an hour, not ~100. Co-Authored-By: Claude Fable 5 --- README.md | 7 ++++--- script/verify_recovery.sh | 25 +++++++++++++++++++++++-- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index f2bf179..6ec0c86 100644 --- a/README.md +++ b/README.md @@ -1582,9 +1582,10 @@ described there. **9. Business tables grow without bound, by design.** `push_events`, `ingestion_runs`, and `quarantined_events` are append-only — this service is the system of record, and retention, pruning, and archival were deliberately not built. The 60-request hourly -ceiling keeps the worst case modest (twelve poll attempts and at most ~100 events per -hour); the only shipped pruning is Solid Queue's finished-job cleanup in the queue -database, which holds no business data. +ceiling bounds the worst case: twelve poll attempts an hour at up to ~100 events each +is at most ~1,200 rows an hour, fewer after duplicate skips. The only shipped pruning +is Solid Queue's finished-job cleanup in the queue database, which holds no business +data. ## Development diff --git a/script/verify_recovery.sh b/script/verify_recovery.sh index c2a1bb1..352a7a3 100755 --- a/script/verify_recovery.sh +++ b/script/verify_recovery.sh @@ -875,9 +875,25 @@ 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" + # Armed before the attempt, not after: an interrupted stop can leave the worker down + # while reporting a nonzero status, and the trap must cover that window too. Starting a + # worker that never stopped is harmless. + TEST_WORKER_RESTART_PENDING=1 if compose stop worker >/dev/null 2>&1; then - TEST_WORKER_RESTART_PENDING=1 + scenario_stop_status=0 + else + scenario_stop_status=$? fi + check "the worker stopped for the scenario phase" "0" "$scenario_stop_status" + + if [ "$scenario_stop_status" -ne 0 ]; then + echo + echo "The worker could not be stopped, so the scenarios are skipped rather than run" + echo "against a candidate set a concurrent poll can re-order. The final verdict" + echo "remains failed." + return 0 + fi + sleep 3 echo @@ -887,9 +903,14 @@ phase_scenarios() { "an off-host redirect is refused by the URL policy" echo "\$ docker compose start worker" - if compose start worker >/dev/null 2>&1; then + if compose start worker >/dev/null 2>&1 && \ + wait_for "the worker stopped for the scenario phase to be running again" 60 running worker; then + scenario_worker_start_status=0 TEST_WORKER_RESTART_PENDING=0 + else + scenario_worker_start_status=$? fi + check "the worker restarted after the scenario phase" "0" "$scenario_worker_start_status" echo count_header