docs(stovepipe): record stage RFC, aligned with the shipped implementation - #394
Open
mnoah1 wants to merge 7 commits into
Open
docs(stovepipe): record stage RFC, aligned with the shipped implementation#394mnoah1 wants to merge 7 commits into
mnoah1 wants to merge 7 commits into
Conversation
mnoah1
marked this pull request as ready for review
July 16, 2026 19:33
mnoah1
force-pushed
the
mnoah1/record-step
branch
2 times, most recently
from
August 4, 2026 20:22
4dc7628 to
7b28df2
Compare
mnoah1
commented
Aug 4, 2026
| | `Project` | Empty for the whole repository; stable project id for Phase 2 | | ||
| | `Degree` | Health degree in the closed interval `[0, 1]`; `0` is green and `1` is fully broken | | ||
| | `RequestID` | Request that established the fact | | ||
| | `CreatedAt` | Millisecond timestamp at which the fact was first recorded | |
Contributor
Author
There was a problem hiding this comment.
Thoughts on timestamp? We don't track timestamps elsewhere in the system but for these more long-term persisted facts, do we want to maintain some kind of timestamped history?
roychying
reviewed
Aug 5, 2026
|
|
||
| A fact is written only when the request reaches a `succeeded` or `failed` verdict. A `cancelled` build is acked with no fact recorded. Callers gating deployments treat an absent fact as not green. | ||
|
|
||
| The fail-closed path also produces no fact, as the DLQ reconciler forces `failed` and never publishes to `record`. |
Contributor
There was a problem hiding this comment.
I don't think never publishes to record holds when the fail-close originates in build_dlq. if build ack keeps failing, once retry_count hits MaxAttempts, the subscriber itself moves it to the DLQ, leaving one or more live poll chains behind in buildsignal step, and publishRecord fires anyway, record then writes a degree-1 fact regardless of the build result.
Contributor
Author
There was a problem hiding this comment.
Updated this. publishRecord sits outside the guard that makes finishRequest idempotent, and the entry guard admits a request that already carries an outcome — deliberately, so a redelivery after the outcome was stamped but before the publish landed still reaches record. So a forced failed gets carried into a fact by any delivery arriving afterwards. "The reconciler never publishes to record" was only ever true of the reconciler, not of the path, and it contradicted the late-success edge case a few sections down.
mnoah1
force-pushed
the
mnoah1/record-step
branch
from
August 13, 2026 15:58
7b28df2 to
2f229dd
Compare
mnoah1
changed the title
…mework The record stage landed in #555 while this doc still read as a proposal, so the RFC described a design rather than the code. Re-frames it as-built and corrects the contract details that drifted: ValidationFact carries no Queue field (the queue is the store binding and the leading PK column), Get takes (uri, project), the Record payload carries queue_name beside the request id, degrees are the named DegreeGreen/DegreeBroken constants, LastGreenRequestID is shipped rather than proposed, and the bookmark's CAS conflict is absorbed by the retry loop rather than returned. Adds the two algorithm steps the doc omitted (deserialize, per-queue storage resolution) and the ordering rationale for advancing the bookmark only after the fact is durable. Replaces the inline Hooks design with integration against hook-framework.md, which supersedes it: a HookEvent published to a per-domain hook topic and dispatched to a pluggable extension, not a Notify() call out of the stage. Records where the publish belongs, the event shape, and the decision that `type` names the scope (validation.repository.recorded) rather than the outcome, since scope is the axis consumers cannot route on later. Documents that record_dlq has no consumer even though dead-lettering is enabled on the subscription, so rejected messages accumulate unread.
…uarantee Rebasing brought ref promotion and the two commit-timestamp metrics into the record stage, none of which the RFC described: promotion is gated on holding the last-green bookmark so an older green commit cannot drag the ref backwards, and the equal-id case reports holding it so a crash before promoting is recoverable. Also corrects an overstated claim. The fail-closed path was documented as never reaching record, but buildsignal publishes unconditionally once a build goes terminal, outside the guard that makes its state write idempotent. A request forced to failed by buildsignal_dlq reconciliation can therefore be carried into a permanent broken fact by a surviving poll chain that observed success. Records the trace, notes that build_dlq is orphaned alongside record_dlq, and opens the question of whether observed success should outrank a forced failure.
mnoah1
force-pushed
the
mnoah1/record-step
branch
from
August 13, 2026 16:18
2f229dd to
74f0a48
Compare
…osed path Two dead-letter triggers, not one: a delivery that fails with its budget spent is dead-lettered by the nack itself, but one that never reaches a nack — a crash, or an ack that fails and lets the visibility timeout redeliver — is dead-lettered by the poll loop without the controller running on that final attempt. So the exposure from a missing reconciler is wider than poison payloads. Retraces the wrong-fact path from that premise: build's ack fails, each redelivery triggers another build, and the poll chains outlive the dead-lettered build message because Hold resets retry_count rather than spending it. Also corrects build.md, which claimed a build_dlq reconciler frees the slot; none exists, so a build that dead-letters before persisting its row wedges a slot for good.
Rewrite the denser passages in plainer language and cut redundant scaffolding. No design content removed: the hook framework, Phase 2 sketches, and open questions stay in full, since the doc describes how the step is meant to work and not only what ships today.
… RFC Stop restating what hook-framework.md already settles (envelope reasoning, delivery promise, dispatcher, rejected alternatives) and link to it instead, leaving only the decisions Stovepipe owns for validation facts. Flag one conflict with the framework: it mints the event id with the causal message id standing in when there is no version, and a fact is create-only. Taken literally that puts the record message's id on the tail, but buildsignal republishes to record on redelivery, so one Request can yield two record messages and one fact would be emitted as two distinct events. Mint from the request id alone and confirm the deviation with the framework's owner.
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
3 participants
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.
Updating record step doc to align with current implementation (now mostly implemented through phase 1), and forward looking plans for Hooks and future phases.