PR 5 — Push-event ingestion - #26
Merged
Merged
Conversation
SHA-256 of compact UTF-8 JSON produced after recursively sorting all object keys.
One definition, no alternates — §7 says so twice and CLAUDE.md repeats it, because a
second algorithm would mean two rows for one payload and an occurrence count that no
longer counts occurrences.
Because payload_fingerprint is the *only* unique constraint on quarantined_events
(§7, Appendix D item 7 — a malformed event may be malformed precisely because it
lacks an event ID), this is a primary key, and a primary key has to be a pure
function of its input. So: no clock, no database, no configuration, and no
dependence on Hash iteration order.
What is fingerprinted is the whole envelope element, the same value written to
raw_payload — not envelope["payload"]. Corpus event 58000000007 carries
"payload": {}, so fingerprinting only the payload would collapse every typeless
envelope with an empty payload onto one row and discard the rest permanently, since
record! never refreshes raw_payload or error_code. Two specs pin that: the same event
ID with a different actor is a different identity, and an envelope with and without
its id are different identities.
JSON.generate, deliberately, not to_json. to_json routes a Hash through
ActiveSupport's encoder, which applies as_json coercions and HTML-escapes < > & when
ActiveSupport.escape_html_entities_in_json is true — an application-level setting.
Both are deterministic today and both are one initializer away from silently re-keying
every row already in the table.
The function is total over anything a JSON array element can be, because
EventSources::Base#events returns elements untouched including null, and §7 requires
those to be quarantinable: nil, [], {}, a bare String and a bare Integer all
canonicalize. Three inputs raise instead: a non-String object key (unreachable from
JSON.parse, and the one input that would make sort order ambiguous — sort_by is not
stable), a value outside the JSON data model, and a non-finite Float.
The last one is a real path, not a curiosity. JSON.parse("1e400") succeeds and yields
Float::INFINITY; JSON.generate then raises JSON::GeneratorError, and ActiveRecord
serialises jsonb through the same generator — so such a payload can be neither
fingerprinted nor stored, and its only terminal outcome is events_failed. Naming it
here means the log line says what happened.
Numeric representation is deliberately not normalized: 1 and 1.0 fingerprint
differently, pinned by a spec so nobody "fixes" it. Over-splitting costs one extra
row; merging them would fold two genuinely different payloads onto one row and lose
one of them for good.
Assertions are written against the canonical string rather than opaque hex, so each
example documents the algorithm. One golden digest is kept as a library-upgrade
tripwire.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§7's table has five rows, three of which quarantine. This turns them into ten codes,
a TAXONOMY hash naming which row each code implements, and a declared precedence
order — so the vocabulary is a refinement of the plan that a spec can check, rather
than a handful of strings someone typed.
CODES is derived from TAXONOMY, which means a new code cannot be added without naming
its plan row, and the spec asserts every row is exercised in both directions.
Precedence is declarative rather than a side effect of the order the processor happens
to run its checks in, because QuarantinedEvent::OCCURRENCE_MERGE deliberately never
refreshes error_code: the first classification is permanent, so the winning code has
to be a property of the taxonomy and not of control flow. Order is structure, envelope
identity, payload presence, payload shape, integrity, range. Two boundaries carry the
weight:
Envelope identity before payload — the quarantine row is indexed by github_event_id
and carries event_type, and both come from the envelope, so a row classified by a
payload defect would be the less useful of the two available facts.
Shape before integrity — with payload.repository_id == "1296269" (a String) and
repo.id == 1296269, Ruby's != is true, so an integrity-first order would report a
mismatch that does not exist while shape-first names the actual defect. Spec'd
directly.
There is deliberately no "could not classify" code. Such a row would be a permanent
lie about the taxonomy, since error_code is never revised; an unexpected error on the
write path is counted as events_failed instead, which is also what
PushEvent.insert_if_new's own comment asks for ("reaching this raise means the parser
let something through").
identifier_out_of_range has no §7 row and is called out as a refinement of "present
but unusable". It cannot be dropped: the four identifier columns are bigint, nothing
validates numericality, and an over-range value would otherwise reach PostgreSQL and
become an unclassified failure instead of a classified quarantine.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§7's tolerant parser: it requires repository_id, push_id, ref, head and before, tolerates unknown fields because GitHub can add response fields without a new API version, and retains the entire envelope in raw_payload regardless. The required-field set is deliberately wider than those five. It is the union of §7's list and every NOT NULL or validated attribute of the three models the write path touches — push_events' nine columns plus SHA_FORMAT, github_actors' github_id and login, github_repositories' github_id and full_name. Both upsert_stub! methods call validate! precisely so a malformed envelope cannot abort the ingest transaction, so anything they would reject has to be classified here instead: without the actor.login and repo.name clauses a real envelope with a null login would land in events_failed rather than in the taxonomy. A spec asserts both directions — every accepted attribute set validates against all three models, and each rejectable shape quarantines instead. Outcome is the transaction boundary, not a convenience. assert_committable! forbids a live request inside an application transaction, so PR 5's shape is fixed: interpret outside the database, write inside it. The three attribute Hashes carry exactly the keyword arguments of PR 3's model writers, so the writer body is upsert_stub!(**outcome.actor_attributes) and insert_if_new(outcome.push_event_attributes) — the models keep sole ownership of their merge rules and Outcome knows no SQL. There is no :failed kind, because a failure is discovered while writing, not while interpreting. push_type? is on the Outcome rather than in the counter, so push_events_seen counts envelopes GitHub typed as pushes whether or not they normalized — six of page 1's eight, not four. The processor is pure: no clock, no database, no network. That is required for correctness, not just for test speed — OCCURRENCE_MERGE never refreshes error_code, so the first classification of a payload is permanent and must not depend on anything outside the payload. Envelope holds the two readers the registry and the processor both need, so the rule for reading an event's id and type cannot drift between routing and normalizing. Time.iso8601, not Time.zone.parse: the latter answers nil for junk, which would travel as far as a NotNullViolation inside the ingest transaction instead of being classified. The integrity comparison runs only when both identifiers parsed as Integers. A String "1296269" is != the Integer 1296269, so an unguarded comparison would report a mismatch that does not exist and hide the real defect behind a fabricated one. Spec'd directly. #call never raises for malformed data, with one honest exception: a non-finite Float from a literal like 1e400 has no fingerprint and no jsonb representation, so it has no quarantine row available to it. Both halves of that boundary are pinned — it raises when it would have to fingerprint one, and it does not fingerprint a healthy envelope, so an unstorable value in an unknown field surfaces at the write and lands in events_failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§6's second seam. It deliberately mirrors EventSources::Base.registry / .for down to the shape of the error message, so a reviewer who has read the first seam recognizes this one: both answer which implementation serves a name, and what happens when none does. The registry owns exactly the type-agnostic half of §7's taxonomy — element is not an object, no usable type, a type nothing implements, otherwise delegate — so a future processor declares its own payload requirements without touching this file. Type before field checks is load-bearing. Roughly nine in ten events on the global feed are not pushes; running payload checks first would quarantine all of them for lacking payload.head, and §7 says a valid non-PushEvent is "Ignored and counted — not quarantined". A malformed event of an unimplemented type is still ignored, and that is a decision. This application has no WatchEvent processor and therefore no definition of a valid WatchEvent; validating fields it never persists would invent a schema for twenty-odd unverified event types and fill the quarantine table with rows no operator can act on. For an unprocessed type, "valid" can only mean "identifiable" — a Hash with a nameable type. An element that is not a Hash, or whose type is not a String, fails that and is quarantined. .for(event_types) is §6's "Configured event types must be validated against implemented processors. Unsupported types should fail fast with a clear configuration error", and it is what §12's "Processor registry validation" test exercises. There is deliberately no GITHUB_EVENT_TYPES environment variable: one processor ships, so the only value that would pass validation is the default, and §16 forbids dead or speculative infrastructure. The day a second processor lands it is one DEFAULTS entry and one #validate! line, with this method already in place. The omission is documented in .env.example's existing "Deliberately absent, and not oversights" section. The processor contract is checked by duck-typing real objects rather than by an abstract base class with one subclass — the same speculation critique Appendix A item 7 upheld the adapter design against. One spec is about the fingerprint's scope rather than the registry: two typeless envelopes sharing an event id but differing in actor produce different identities, which is what §7's "a different malformed payload is a different quarantine row" requires and what fingerprinting only envelope["payload"] would have broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The counters of one run as a value object, so the arithmetic is assertable with no database. Two identities hold for every run and both are spec'd against page 1 of the corpus: events_received = push_events_seen + events_ignored + invalid_envelope_quarantines push_events_seen = events_created + duplicates_skipped + push_quarantines + events_failed Immutable, so #record returns the next tally rather than mutating this one — a counter cannot be bumped from two places by accident, and a spec asserts the receiver is unchanged. push_type travels separately from result because push_events_seen counts what GitHub *typed* as a push whether or not it normalized (§8 step 4 filters PushEvent entries). Page 1's eight envelopes therefore give push_events_seen 6, not 4: the two malformed pushes are still pushes that were seen. The alternative reading — "everything not positively identified as another type" — would make events_ignored exactly derivable from the stored columns, and is rejected because calling a typeless envelope a push event seen misrepresents the column. events_ignored has no column. §7's ingestion_runs field list has none and PR 3 owns the schema, so it is logged and printed but not persisted — and it is honestly not reconstructible from the stored counters, because an envelope with no usable type is quarantined rather than ignored, so events_received - push_events_seen counts both. persistable_attributes and to_log make that split visible at the call site rather than hiding it, and a spec asserts persistable_attributes matches IngestionRun::COUNTERS exactly and produces a valid record. pages_fetched counts a page that decoded into an array and reached the processor, so a 304, a deferral, a transport failure and an undecodable body all contribute nothing. The attempt itself is already counted by github_api_budget.poll_used and by the DEBUG request line, and this way an undecodable page stays distinguishable from an empty one. Report carries the shared stdout formatting. §9 prints two blocks produced by two different objects and a reviewer reads them as one report, so the column width lives in one place — 34, which is the width that reproduces all four of §9's sample lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR 3 deferred the vocabulary here in the model's own comment. Five states:
running created before the fetch, completed_at still NULL — which is what a
process killed mid-run leaves behind, and the honest crash signal.
completed a page was fetched and every envelope reached a terminal outcome,
quarantines and per-event failures included.
not_modified a 304. §10: no event processing runs and the reservation stays debited.
Distinct from completed because "the feed had nothing new" and "the feed
returned an empty page" are different facts an operator must be able to
tell apart.
deferred the request never happened — budget denial, busy gate, rate-limit
response. §10 is explicit that these are not failures, so folding them
into failed would burn a healthy source's failure count on a busy system.
failed the attempt happened and did not produce usable events.
There is deliberately no busy status: the run row is created inside the source lock, so
Errors::SourceBusy provably cannot leave one behind.
enum … validate: true matches Enrichable and GithubApiBudget, the two vocabularies already
here. Unlike those two there is no matching CHECK constraint — one needs a migration and
PR 3 owns the schema — so the validation is the guard and the constraint is a one-line
follow-up whenever migrations are next touched. Called out rather than left as an
inconsistency.
run_id is generated in Ruby and passed on insert. §11 requires it on the correlated log
lines, the first of those is emitted as the run starts, and this codebase has no tagged
logging or log-context helper — so the value has to exist before the row does.
gen_random_uuid() stays as the column default and remains the safety net for a
hand-written insert, which is exactly what PR 3's model spec asserts; reading it back
instead would cost a SELECT and produce a run_started line with no correlation id on it.
Counters are written once in finish!, from the tally. Not incrementally and not inside the
per-envelope transaction: §7 calls this row a record of one polling cycle and §8 puts the
durable record elsewhere, so updating it per envelope would put a hot row and a held row
lock inside the write path in exchange for diagnostics push_events.created_at already
provides. A running row with a NULL completed_at is also more honest than counters that
could disagree with the events which actually committed.
latest_successful excludes unfinished runs as well as unsuccessful ones, because §9's
"Latest successful run" line prints completed_at and a run still in flight has none.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§8 steps 4–9. The constructor takes a registry and a clock, and deliberately no executor and no event source, so a live GitHub request cannot physically be issued from inside its transaction — the same structural technique PR 4 used when it never handed RequestExecutor an event_source_id. assert_committable! and LockOrder are the enforced halves. One transaction per envelope, not per page. §16 requires that malformed data "does not terminate the batch", and in PostgreSQL a failed statement aborts the entire enclosing transaction — a hazard spec/support/constraint_helpers.rb already documents here. With a page-wide transaction, one unexpected error on envelope five of eight would discard envelopes one to four that the log had already reported as persisted, along with the quarantine rows written for the others. §8 asks for the same shape from the other direction: its durability boundary is per event, and step 9 calls the transaction "short-lived — the advisory lock, not the transaction, spans the HTTP work". It also keeps PR 6 additive: no transaction is ever open across a fetch, so a Link-driven page loop needs no change here. What it gives up is page-level atomicity, and nothing needs it — github_event_id uniqueness makes reprocessing a partially written page a no-op, which is why §9 requires every fetched page to be processed in full. Interpretation is inside the same per-envelope boundary, which is why this is a page writer rather than the outcome writer the plan sketched. Classification is pure but not infallible: an envelope carrying a value JSON cannot represent has no fingerprint, so the fingerprinter raises rather than invent one. Interpreting the page in one pass and writing in another would let that single envelope take the whole page down — the exact failure §16 forbids. Github::Events::Outcome#push_type? was dropped in the same move: the writer reads the type from the envelope, so push_events_seen is one number computed one way whether the envelope persisted, duplicated, quarantined or failed. Activity updates happen only when RETURNING produced a row (§7 merge rule 3, Appendix D item 5). On a duplicate the transaction still commits, carrying rule 1's identity refresh and nothing else, so rule 4 holds structurally rather than by a later check. Six specs cover it, including the structural one — touch_activity! is never called on a replay — and one that plants a skipped_budget entity and proves the replay cannot reactivate it. The replay specs use update_all with updated_at held at the frozen instant, because IDENTITY_MERGE gates each assignment on EXCLUDED.updated_at >= the stored value and a wall-clock touch would make them pass for the wrong reason. last_seen_at is asserted with a later instant and first_seen_at with an earlier one, since GREATEST and LEAST respectively make the other direction vacuous. Quarantine is one statement outside any transaction, so a quarantine record can never be discarded by a later failure — a property a page-wide transaction would not have. It logs at INFO rather than DEBUG: §11 puts per-event lines at DEBUG, but the issue requires GitHub event IDs in the logs and §16 requires malformed data to be visibly quarantined, and an operator should not have to change log levels to learn that events are being rejected. Ignored events write nothing at all, not even stub entities: §8 quarantines at step 5 and upserts at step 6, so only envelopes that normalized produce entities. That is what makes page 1 give three actors and three repositories rather than four of each — and it matches the corpus, whose enrichment fixtures are exactly those three users and three repositories. FATAL_ERRORS re-raises rather than absorbing: a locking or transaction invariant is a programming error whose own doc comment asks it to be loud, and a dead connection is not a property of any one envelope — reporting "events_failed: 8, completed" for either would be a lie. Two facts were probed against PostgreSQL 16 rather than assumed, and both changed what the code says. A String carrying a NUL byte is refused (ArgumentError from the driver for a text column, PG::UntranslatableCharacter inside jsonb) — that is the events_failed path the isolation spec exercises. Float::INFINITY, on the other hand, is written into jsonb as null without raising, so a healthy envelope carrying one persists with that field nulled inside ADR 0001's semantic-retention tradeoff; the earlier comments claiming it could not be stored were wrong and are corrected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A run needs an event_sources row and nothing seeds one. PR 4 already wrote down why seeding is wrong, in BudgetLedger#bootstrap!'s comment: db/seeds.rb runs only when db:prepare creates a database and db:test:prepare never seeds at all, so a seeded row would exist in development and be absent in test — the worst possible split, and a migration is wrong for the same reason since schema.rb carries no rows. So this follows the pattern the ledger established rather than inventing a second answer: lazily, at the point of use. A boot initializer is also wrong. config/initializers/github.rb guarantees its validation touches no database, which is what lets db:prepare, rails runner and CI's schema load work before anything is migrated. source_type comes from EventSources::Base.for_mode, which PR 4 built for exactly this and says so — so the row a process provisions always matches the adapter it will poll with. event_sources.source_type is deliberately not unique: §6 anticipates per-repository sources and a PR 3 spec asserts several rows of one type are allowed. So provisioning cannot be made atomic with an ON CONFLICT clause, and two processes racing on a fresh database can both insert. Converging on the lowest id is what makes that harmless — both processes then derive the *same* advisory lock key from the same row, so the source lock keeps protecting the source, and the cost of the race is one extra row and one extra poll attempt with duplicate events absorbed by uniqueness. Adding a constraint would contradict a committed decision and belongs to PR 6, which owns this table. status is written once as "idle" and never touched again: the column is NOT NULL with no vocabulary because PR 6 owns the poll state machine. configuration stays empty, because §6 puts endpoint construction in the adapter and a URL stored here would be a second source of truth able to disagree with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§11 lists run_id among the fields every correlated line carries, and the DEBUG github.request line is exactly the line a reviewer turns on to trace one run. Today it cannot carry one: EventSources::Base#request_for hard-codes context to the source type. Three optional keyword arguments fix that, and nothing else changes. Request#to_log already merges context flat and FetchResult#to_log already merges request.to_log, so the executor and the formatter are untouched. Every default is unchanged and no existing spec inspected Request#context, so the blast radius is this file plus two shared examples. The adapter's own source_type merges last, so a caller's context cannot impersonate it — asserted, because a log field that can be spoofed by its own payload is the mistake JsonLogFormatter::RESERVED_KEYS already guards against at the formatter level. Correlation matters more than it looks: from PR 8 the worker's scheduled poll and a reviewer's one-shot interleave in one stdout stream, where correlating by timestamp adjacency stops working. §16's operability gate asks for correlation fields, not for correlation by inference. Also renames the formatter spec's sample event to ingestion.run_started, matching the dotted convention the two existing emitters already use (github.request, budget.window_rolled), so the example stops reading as a naming precedent the code contradicts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§5 names this component and says what it owns: "IngestionRunner owns the source lock for
the duration of a polling operation. RequestExecutor does not own or acquire SourceLock."
So the lock is taken here, and the run row is opened *inside* it — which is what makes
"a busy source leaves no stray running row" provable rather than merely likely, and the
SourceBusy spec asserts exactly that: no run row, no request, no budget spent.
Two orderings carry the design.
The fetch sits between two writes with no transaction open, because assert_committable!
refuses a reservation inside one ("an outer rollback would refund a request GitHub has
already counted"). A spec asserts the property directly at the moment the transport is
called, rather than trusting the ledger's own guard — a future refactor that wrapped the
run in a transaction would otherwise fail only in production, where the example transaction
does not mask it.
Classification is branched on *before* the body is decoded. Base#events calls
JSON.parse(body.to_s) and a 304 carries no body, so the other order turns a perfectly
healthy 304 into a MalformedResponse and records a failed run. The 304 spec is what catches
that inversion.
Outcomes map to §7's and §10's vocabulary rather than to HTTP: 304 is not_modified, a
budget denial or a busy gate is deferred (§10: they "never happened", so folding them into
failed would burn a healthy source's failure count), a rate limit is deferred too because
nothing is wrong with the request, and an unusable body is a failed run with no quarantine
row — §7's taxonomy row 5 says an invalid response body is a request failure, not an
individual quarantined event. A denial carries which of §10's four conditions refused it,
which is the one actionable fact about a deferral.
One Result type for every outcome, the same philosophy RequestExecutor#call applies to
requests: a caller branches once instead of rescuing four classes and eventually missing
one. Only SourceBusy and genuinely unexpected errors escape, and the unexpected ones
finalize the run row before re-raising so nothing is abandoned in running — including a
FixtureMiss, which §6 requires to stay loud.
--force is accepted and recorded on the run_started line, and does nothing else in PR 5:
what §9 says it bypasses — the configured cadence and the stored ETag — is PR 6's, and
neither exists to bypass yet. Shipping the flag now means the documented command works and
the CLI surface does not change when PR 6 gives it teeth.
Nothing is written to event_sources beyond provisioning, and no ETag is ever sent. The
corpus still scripts a 304 as its second response and a live 304 can arrive from any cache,
which is why the handling exists before the conditional request does — asserted by a spec
that checks no If-None-Match was sent and event_sources.etag is still nil.
The integration spec runs the real gate, the real ledger and the real URL policy over the
offline corpus, and pins §12's numbers: 8 received, 6 push events seen, 4 created, 3
quarantined under three distinct codes, 1 ignored, three actors and three repositories —
which is also exactly the set of entities the corpus ships enrichment fixtures for. It also
covers the replay (duplicates absorbed, identity refreshed, activity untouched, a planted
skipped_budget entity not reactivated, occurrence counts at two), retries recovering a page
without processing it twice, and each deferral path spending nothing.
One incidental finding, recorded so nobody repeats it: PostgreSQL reads lock_timeout = 0 as
"no timeout", so a zero gate wait blocks forever rather than deferring. The spec uses 0.1s,
as the executor's own spec already does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§9: "Its stdout always proves system state, even when deferred or busy." That rules out making this a projection of a run result — on the busy path no run happened at all — so it is a snapshot of what is persisted, from five statements and nothing else. It never initiates a GitHub request, the same guarantee §11 places on /status, and the guarantee is structural rather than a promise: the class holds no executor, no transport and no ledger, and its only collaborators are Active Record models. Two specs pin it. The fixture transport records every request it is handed and must have none. And the ledger row count must not change — which also catches the subtler mistake of calling BudgetLedger#bootstrap! from a read path, since the row is created by the first reservation and never by a read. Splitting the snapshot from its rendering is what lets a spec assert §9's "1,284" delimiter without inserting 1,284 rows, and it is the seam PR 10's /status consumes while the one-shot consumes #to_s. The rendered block is asserted line for line against §9's sample. Three budget states, not two, because §16 forbids a misleading guarantee and a fabricated zero on a fresh install is exactly one: no ledger row at all reads "not yet initialized", a row whose remaining is NULL reads "unknown (window uninitialized)" — §7 is explicit that 60 remaining must never be assumed — and only a real zero prints as 0. enrichment_candidates rather than a hand-written pending filter, so both counts are counts over index_*_on_enrichment_candidates' exact predicate. In PR 5 the two definitions are numerically identical because nothing transitions an entity until PR 7; §11's finer pending/skipped split arrives with PR 10. The rate-limit resource is read from the row rather than hardcoded, so a future authenticated configuration is a data change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§9's contract, whole: retry the source lock for up to SOURCE_LOCK_WAIT_SECONDS, print "source busy — poller cycle in progress" plus the state summary and exit 0 when it is still held, send every request through the same gate and ledger as the poller, and always prove system state on stdout — "even when deferred or busy". OneShot returns its exit code and never calls exit; bin/ingest does, and that one line is the only place in the application that ends a process. Returning the code is what makes the entire contract a plain unit spec instead of a shelled-out one, including the busy path, each deferral, and every printed line. The division of labour is the other half: OneShot owns the human text and the exit code, IngestionRunner owns the structured logs and every database write. Neither can drift into the other's stream, which matters because §11 puts both on stdout — so the operator block is composed and written once, and $stdout.sync is set, and a JSON log line can never split it. Three exit codes. 0 for ran *or* deferred: §9 pins it for a busy source, and §10's reasoning extends it to a budget denial, a held gate and a rate-limit response, all of which mean the request never happened. 1 for an attempt that failed. 2 for refusing to run at all — a bad option, a configuration the process must not run with, or a corpus gap. §6 requires fixture mode to fail closed and the executor deliberately re-raises a corpus gap, and "fix the corpus" is a different outcome from "GitHub failed", so it is not folded into 1. §9's "Ingestion deferred until T" takes T from effective_poll_time, which is PR 6's. The instant PR 5 can honestly name is the one the ledger already knows — the window reset — so the line reads "deferred until <reset> — <reason>", and drops the clause entirely when there is no instant at all, as for a held gate. Named as a deliberate reading of §9 rather than left implicit. --force is parsed and passed through, and does nothing else. Deferring the flag would mean a reviewer typing the documented command gets a usage error, which is the worst outcome for §16's reviewer-experience gate; shipping it inert means the command works today, --help says plainly that cadence gating and ETag reuse land with PR 6, and the CLI surface does not change when they do. bin/ingest is a thin shell over config/environment (not config/boot — this command uses models). It owns exactly one piece of logic, and it has to: the budget configuration is validated inside to_prepare, so a configuration error raises during the boot require, before OneShot is loaded and before any logger contract exists. It is matched by class name because the constant may not be autoloadable at the point boot failed. A spec asserts the file is executable — a lost exec bit breaks `docker compose run --rm ingest` and nothing else in the suite would catch it — and that it contains no branching of its own. The compose service uses an entrypoint with an explicitly empty command, so the plan's bare `docker compose run --rm ingest` and a flagged `... ingest --force` both work and the image's puma CMD can never arrive as arguments to bin/ingest. It inherits the shared environment anchor unmodified, unlike `test`, because the deterministic reviewer path needs GITHUB_MODE=fixture to reach this container. spec/docker_compose_spec.rb asserts §16's "plain up starts exactly db, setup and web" gate, which PR 2 shipped untested and which one mistyped profile key silently breaks. Verified against Compose itself as well: `config --services` prints three, `--profile tools` prints five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
§14 lists this ADR and PR 5 is where the write path it describes lands, so writing it now rather than later means documenting a decision as it is made instead of after the fact. It states the alternative that was rejected — chasing exactly-once execution across an HTTP client, a queue in a second database, and the business tables — names the four mechanisms that replace it, and is explicit about what the choice costs: repeated non-idempotent side effects, duplicate work already debited from the budget, events_quarantined counting observations rather than rows, and a permanent first classification. The operator consequence is the part worth having written down: re-running `docker compose run --rm ingest` is safe at any frequency, so nobody has to reason about it before typing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md requires documentation to stay accurate in the same PR that changes behavior, and "Nothing polls GitHub yet" stopped being true in this one. The new One-shot ingestion section carries the four invocations, both literal output blocks, the exit-code table, the busy contract, and a deterministic fixture walkthrough with the exact counts the corpus produces on a first run and on a second — the numbers a reviewer can check against psql in one command. Log expectations are named too: which events appear at the default level and what LOG_LEVEL=debug adds. Two README rows had to change rather than being quietly left alone. The one-shot row promised "--force, deferred/busy semantics" to PR 5; busy and deferred ship here, cadence gating and ETag reuse do not, so the row is replaced by the section itself plus an explicit PR 6 row for the half that is missing. The exact-counts row moves from PR 11, since PR 5 documents them now, and PR 11 keeps the full scenario matrix. The request-path diagram gains IngestionRunner as the holder of the source lock and PageWriter as the owner of the ingest transaction, so the section no longer stops at the transport. .env.example gains three notes and no variables. That the poll allowance is already enforced, so twelve live one-shot runs an hour is the ceiling at the defaults and the thirteenth is deferred rather than failed. That the fixture cursor lives on the transport instance, which is what makes duplicate absorption demonstrable by running the command twice. And GITHUB_EVENT_TYPES joins the "deliberately absent" list with its reasoning: §6 offers it as an option, one processor ships, so its only legal value is its default — ProcessorRegistry.for already performs the validation §6 requires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A missing `require "optparse"` survived all 758 examples and died on the first real
invocation: rspec loads optparse through another gem, so the constant resolves in the suite
and nowhere else. Rails does not require it.
The fix is one line; the interesting part is that no unit test could have caught it. The
suite boots Rails through rails_helper, so the one-shot's own boot path — its shebang, its
config/environment require, and every stdlib it depends on but the harness happens to have
loaded already — is unexercised by construction. Running the command once is the only thing
that exercises it, so both bin/ci and the GitHub Actions workflow now do, in fixture mode so
it touches no network and against the already-prepared test database.
Verified by hand against the built image after the fix, over the full outcome matrix:
live 100 events seen, 98 push events created, 2 non-push ignored, exit 0
fixture 4 created / 3 quarantined / 1 ignored, then 0 created / 4 duplicates on a
second run, with psql reporting 4 push events, 3 actors, 3 repositories and
3 quarantine rows at occurrence_count 2 — the numbers the README documents
busy source lock held by a second PostgreSQL session: "source busy — poller cycle
in progress" plus the state summary, exit 0, no ingestion_runs row, and the
ledger byte-identical before and after, which is the receipt proving no
request was issued
deferred "Ingestion deferred until 2026-07-30T15:24:13Z — class_allowance_exhausted",
exit 0, run recorded as deferred with no last_error
usage --help exits 0; --turbo and a stray positional exit 2 on stderr
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The deferred headline queried github_api_budget directly for reset_at, duplicating a value StateSummary captures a moment later and re-stating knowledge the summary owns. The snapshot is now taken first and handed to the headline builder, so both come from one read and the command asks the database nothing directly — which is the property that keeps "the state summary never initiates a GitHub request" a structural guarantee rather than a habit. No behaviour change; re-verified against the built image that the line still reads "Ingestion deferred until 2026-07-30T15:29:20Z — class_allowance_exhausted". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
batbrainy
commented
Jul 30, 2026
batbrainy
left a comment
batbrainy
left a comment
Owner
Author
There was a problem hiding this comment.
A few things to tighten before I’d approve this.
| def ensure!(mode: Github.configuration.mode, now: Time.current) | ||
| source_type = EventSources::Base.for_mode(mode).source_type | ||
|
|
||
| existing(source_type) || create(source_type, now) || existing(source_type) |
Owner
Author
There was a problem hiding this comment.
This still returns the row this process just created. Because source_type is not unique, two first-time one-shots can both miss existing, insert separate rows, and then take source locks on different event_source.ids. The comment below says the race converges on the lowest id, but that only happens for later callers. I’d re-select existing(source_type) after any create, or provision under a stable source-type lock, so the source-lock contract holds on a fresh database.
| # the ledger already knows: the window reset. When there is none — a held gate has no | ||
| # instant at all — the reason stands on its own. | ||
| def deferred_line(result, summary) | ||
| reset_at = Report.timestamp(summary.budget_reset_at) |
Owner
Author
There was a problem hiding this comment.
This uses the ledger reset timestamp for every deferred result. That is accurate for a reset-backed budget denial, but a gate_unavailable or secondary_limited result can inherit an unrelated reset from the state summary and print a misleading deferred until. I’d only include until when the result has a real deferral instant or the reason is explicitly reset-backed; otherwise just print the reason.
| def deferral(fetched) | ||
| reason = fetched.deferred? ? deferral_reason(fetched) : fetched.classification.to_s | ||
|
|
||
| Rails.logger.info(event: "ingestion.deferred", reason: reason, |
Owner
Author
There was a problem hiding this comment.
This standalone deferral log line drops the run_id, while the rest of the ingestion flow is correlated by it and the README says every run log line carries it. Could pass recorder.run_id into deferral and include it here, or let ingestion.run_completed be the single correlated deferral record.
…related log line Three review findings, all real. **The provisioning race was not harmless.** ensure! returned the row *this* process had just created, so on a fresh database two first-time one-shots could each end up on their own row, take source locks on two different event_source.id values, and poll the same feed concurrently — precisely the guarantee §9 asks the source lock to provide. The "converges on the lowest id" comment was only true for later callers, and the rescue ActiveRecord::RecordNotUnique it sat beside was dead code, since there is no unique constraint to violate. Fixed by serializing the check and the insert with LOCK TABLE event_sources IN SHARE ROW EXCLUSIVE MODE inside the provisioning transaction. It self-conflicts so two provisioners queue, does not conflict with SELECT so no reader waits, and releases at COMMIT so nothing can leak it. ensure! still reads first and outside any transaction, so the lock is a once-per-database cost and every later call is a single SELECT — asserted. Deliberately not a fourth advisory-lock namespace: this is one table's first-write problem, and Github::LockOrder exists to police the two locks that can actually deadlock against each other. The lowest-id selection stays, because duplicate rows can predate this code or be created by hand and every process still has to agree on one key. **The deferral headline borrowed an unrelated instant.** Every deferred result took the ledger's reset_at, so a held gate or a secondary limit printed "deferred until <reset>" — confidently wrong, which is worse than saying nothing. The clause is now limited to the two denial reasons that reset actually governs (class_allowance_exhausted, reserve_reached); a held gate has no instant at all, and a secondary limit clears on a Retry-After that PR 5 does not persist. Everything else states the reason alone. **ingestion.deferred dropped run_id**, making it the one line in the flow that could not be joined to its run — and it is exactly the line an operator greps for when a run produced no events. Threaded through. Verified against the built image: a fresh database provisions exactly one row at id 1; a budget denial still reads "Ingestion deferred until 2026-07-30T16:47:59Z — class_allowance_exhausted"; and with the request gate held by a second PostgreSQL session the one-shot prints "Ingestion deferred — gate_unavailable" with no instant, exits 0, and logs ingestion.deferred carrying the same run_id as ingestion.run_completed. 764 examples, 0 failures. RuboCop clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Closes #15 —
IMPLEMENTATION_PLAN.md§13, PR 5.Problem
PRs 1–4 built everything around ingestion and nothing that ingests. The request chain
works, the tables and their idempotent write primitives exist, both event-source adapters
decode a page — but no code path joins the two halves. There is no processor, no fingerprint
function, no transaction that turns an envelope into rows, no run lifecycle, and no way for a
human to trigger a poll.
docker compose upstarts a service that never fetches anything.Scope
In: the processor registry and the tolerant
PushEventparser; the quarantine taxonomywith canonical fingerprints; the ingest transaction — stub entity upserts under §7's envelope
mappings, idempotent insert,
RETURNING-gated activity updates; ingestion-run summaries withrun_idcorrelation in the logs; and the one-shot command with §9's contention contract andstate summary.
Deliberately out:
Linkpagination (page one only), ETag persistence,effective_poll_timeandcadence gating,
global_blocked_until,Retry-After, and everyevent_sourcespoll-statecolumn.
ingestion_runsis PR 5's poll history and is strictly richer than alast_polled_atscalar.enrichment_statustransition,skipped_budget, reactivation, TTLs.workercontainer./status. PR 5's state summary is plain stdout.No migration. Every column needed already exists.
Technical decisions
One transaction per envelope, not per page. §16 requires that malformed data "does not
terminate the batch", and PostgreSQL aborts an entire transaction on any failed statement —
a hazard
spec/support/constraint_helpers.rbalready documents in this repo. A page-widetransaction would let one unexpected error on envelope five of eight discard the four the log
had already reported as persisted. §8 asks for the same shape from the other direction: its
durability boundary is per event, and step 9 calls the transaction "short-lived". It also
keeps PR 6 additive — no transaction is ever open across a fetch, at any page count.
Quarantine writes stand outside any transaction, so a quarantine record can never be
discarded by a later failure.
Interpretation lives inside the same per-envelope boundary as the write. Classification is
pure but not infallible: an envelope carrying a value JSON cannot represent has no
fingerprint, so the fingerprinter raises rather than invent one. Interpreting a page in one
pass and writing in another would let that single envelope take the whole page down.
Activity gating is the whole of §7 merge rules 3 and 4.
touch_activity!runs only wheninsert_if_newreturned a row; on a duplicate the transaction still commits, carrying rule1's identity refresh and nothing else. §7 rule 3 also mentions applying
skipped_budgetreactivation, but §13 assigns that transition to PR 7 and
Enrichable's own comment says"Nothing here changes
enrichment_status" — and PR 5 cannot produce askipped_budgetentity, so the branch would be unreachable. PR 5 ships the gate; PR 7 adds the transition at
that one call site. Where §7 and §13 disagree about when, §13 decides, per CLAUDE.md's
per-issue protocol.
The fingerprint covers the whole envelope, not
envelope["payload"]. §7 requires the samegithub_event_idwith a different malformed payload to be a different row; corpus event58000000007carries"payload": {}, so fingerprinting only the payload would collapseevery typeless envelope with an empty payload onto one row and discard the rest permanently,
since
record!never refreshesraw_payload.JSON.generate, notto_json, becauseActiveSupport's encoder applies
as_jsoncoercions and HTML-escapes under anapplication-configurable flag — one initializer away from silently re-keying every row in the
table.
push_events_seencounts envelopes GitHub typed as pushes, whether or not theynormalized (§8 step 4), so page 1 gives 6 and not 4. The rejected alternative — "everything
not positively identified as another type" — would make
events_ignoredexactly derivablefrom the stored columns, but calling a typeless envelope a push event seen misrepresents the
column.
events_ignoredis logged and printed but not persisted. §7'singestion_runsfield listhas no column for it and PR 3 owns the schema. It is not reconstructible from the stored
counters either, because a typeless envelope is quarantined rather than ignored — stated
plainly rather than implied.
A parser escape counts as
events_failed, not as a quarantine.PushEvent.insert_if_new'sown comment says reaching its
validate!"means the parser let something through". §7 definesquarantine as a classified predicate and
error_codeis never refreshed on replay, so an"unclassified" row would be a permanent lie. A table-driven spec proves the path unreachable
across the whole taxonomy.
No
GITHUB_EVENT_TYPES. §6 offers it as an option, one processor ships, so its only legalvalue is its default — a knob that can only be set wrong (§16: no speculative
infrastructure). The validation §6 requires exists without it:
ProcessorRegistry.forrefuses an unimplemented type with a configuration error naming whatis implemented. Documented in
.env.example's "deliberately absent" section.--forceis accepted and inert. What §9 says it bypasses is PR 6's, and neither thecadence nor the stored ETag exists to bypass. Deferring the flag would mean a reviewer typing
the documented command gets a usage error — the worst outcome for §16's reviewer-experience
gate. It is recorded on the run's log line,
--helpsays plainly that cadence gating and ETagreuse land with PR 6, and the CLI surface will not change when they do.
Three exit codes.
0for ran or deferred — §9 pins it for a busy source, and §10'sreasoning extends it to a budget denial, a held gate and a rate-limit response, all of which
mean the request never happened.
1for an attempt that failed.2for refusing to run: abad option, a bad configuration, or a corpus gap.
Two facts were probed against PostgreSQL 16 rather than assumed, and both changed the code.
A String carrying a NUL byte is refused (
ArgumentErrorfrom the driver for a text column,PG::UntranslatableCharacterinside jsonb) — that is theevents_failedpath the isolationspec exercises.
Float::INFINITY, by contrast, is written into jsonb asnullwithoutraising, so a healthy envelope carrying one persists with that field nulled, inside ADR 0001's
semantic-retention tradeoff. Earlier comments claiming it could not be stored were wrong and
were corrected.
Two small changes to files this PR does not own, both additive: an optional
context:onEventSources::Base#request_for, sorun_idreaches the DEBUGgithub.requestline withoutthe executor or the formatter knowing about it (the adapter's
source_typemerges last, so acaller cannot impersonate it); and the status vocabulary on
IngestionRun, which PR 3's modelcomment explicitly deferred to this PR. No CHECK constraint accompanies the enum — that needs
a migration and PR 3 owns the schema.
Testing performed
758 examples, 0 failures. RuboCop clean, Brakeman clean, bundler-audit clean.
New specs, all deterministic and offline:
rather than opaque hex so each example documents the algorithm, plus one golden digest as a
library-upgrade tripwire.
asserted at each boundary, including the one that matters: a String
"1296269"against theInteger
1296269must report the unusable shape, not a mismatch that does not exist.directions of the model-union invariant (every accepted attribute set validates against all
three models; every rejectable shape quarantines instead).
touch_activity!is never called) and a planted
skipped_budgetentity that must survive; failure isolationproving a
events_failedenvelope loses neither the events around it nor an earlierquarantine row.
304, budget denial, held gate, rate limits, retries recovering a page without processing it
twice, an unusable body, and a source another session owns. Plus an explicit assertion that
no application transaction is open at the moment the transport is called.
summary asserted present on every path.
docker_compose_spec.rb— §16's "plainupstarts exactlydb,setup,web" gate,which PR 2 shipped untested and one mistyped profile key silently breaks.
A missing
require "optparse"survived all 758 examples and died on the first realinvocation — rspec loads it through another gem. No unit test could have caught it, because
the suite boots Rails through
rails_helperand never exercises the command's own boot path.bin/ciand the Actions workflow now runbin/ingestonce, in fixture mode.Docker verification
Against the built image, after the fix:
4 push_events, 3 actors, 3 repositories, 3 quarantined, 6 occurrencessource busy — poller cycle in progress+ state summary, exit 0, noingestion_runsrow, and the ledger byte-identical before and after — the receipt proving no request was issuedIngestion deferred until 2026-07-30T15:24:13Z — class_allowance_exhausted, exit 0, run recorded asdeferredwith nolast_error--helpexits 0;--turboand a stray positional exit 2 on stderrdocker compose config --servicesstill prints exactlydb setup web;--profile toolsprints five.
Documentation updates
the PR where the write path it describes lands.
One-shot ingestion section with both output blocks, the exit-code table, the busy contract
and the fixture walkthrough with exact counts;
IngestionRunnerandPageWriteradded tothe request-path section; two "Planned contents" rows corrected — the one-shot row promised
--forceto PR 5, so it is replaced by the section plus an explicit PR 6 row for the halfthat is missing.
.env.example— three notes, no new variables: the poll allowance already capsone-shot runs at twelve an hour, the fixture cursor is per process (which is what makes
duplicate absorption demonstrable by hand), and
GITHUB_EVENT_TYPESjoins the "deliberatelyabsent" list with its reasoning.
Known limitations
documentation never says otherwise (§8, §16).
MAX_PAGES_PER_POLLis an allowance-formula input until PR 6 wiresLinkpagination.JSON.parsenesting beyond100, or invalid UTF-8. Decoding is all-or-nothing by nature, and it maps cleanly to §7's
taxonomy row 5 (a request failure, not an individual quarantine).
ingestion_runs.status = "running"with zeroed counters, becausecounters are accumulated in memory and written once. Nothing sweeps stale
runningrows —that is PR 8's reconciler. The run row is a summary artifact; §8's durability boundary is the
committed
push_eventsrow.event_sources.source_typeis not unique — §6 anticipates per-repository sources and aPR 3 spec asserts several rows of one type are allowed — so two processes racing on a fresh
database can both provision. The provisioner converges on the lowest id, which keeps the race
harmless (both derive the same advisory lock key); the cost is one extra row and one extra
poll attempt. Worth a constraint decision in PR 6, which owns that table.
pending.🤖 Generated with Claude Code