docs: specify durable data backfills - #7326
TheSentinel454 wants to merge 12 commits into
Conversation
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
🔐 Codex Security Review
|
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Combined review from Paul + Thufir — both reviewed independently, findings reconciled and deduped, per the beekeepers-channel request.
OSS-genericity: passes. Plain PostgreSQL, generic deployment-operator authorization consistent with the multi-tenant relay model, no Block-specific endpoints, identity systems, deployment tooling, or vendor dependency. Component mapping targets only OSS crates.
1. Make the write-path prerequisite explicit (Correctness — both reviewers flagged this independently; Luke clarified intent in-channel). As written (docs/data-backfills.md lines 185–204), the captured upper bound plus a never-rewinding checkpoint do not by themselves freeze the admitted source set: a transaction in flight at start can commit a row at a key ≤ bound after the checkpoint has passed it, and validation can reach immutable completed before that late commit lands — leaving a permanently unrepaired row behind a validated completion. Luke's clarification — new write paths roll out before any backfill starts, so no concurrent write produces a backfill-actionable row — resolves the design question, but the spec never states that prerequisite. Suggested fix: (a) add a normative requirement that each definition document its admission/visibility guarantee (e.g., write paths deployed before start enforce the repaired postcondition for every insert/update at any key, including transactions already in flight at bound capture); (b) add a controlled late-commit conformance case proving a conforming deployment either repairs such a row or can never produce one. Line 203 currently covers only rows beyond the bound.
2. retry has no destination state (MINOR). The lifecycle table lists validating among next states for both blocked and failed, but the retry action contract (line 145) never names the state it enters. resume spells this out ("running, or validating when no admitted work remains"); retry should use the same formula — otherwise blocked/failed → validating is an edge with no defined trigger.
3. Admin surface reachability while the readiness gate is closed (MINOR). Automatic mode holds serving unready on paused/blocked/failed (lines 284–298), and every path out of those states runs through the admin API. State explicitly that the authorized admin boundary stays reachable while ordinary serving is gated, and exercise pause/remediate/retry through the admin boundary in that condition — otherwise a conforming implementation can lock operators out of their only recovery controls.
Solid throughout: PostgreSQL sole authority, atomic mutation+checkpoint commits, commit-time generation fencing, immutable completion, honest-projection client rules, and a falsifiable conformance suite.
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
The rolling-deployment section said a violated rollout ordering means "rerunning or replacing the repair." Rerunning is not reachable for the same stable ID: `retry` preserves the bound and checkpoint, workers select strictly after the checkpoint, and `completed` is terminal and immutable. State instead that the existing checkpoint may have advanced past writes that were never repaired, that no rerun under the same stable ID can revisit them, and that operators must run a new repair under a new stable ID -- possibly repeatedly -- until the normal write path is safe. This matches the existing rule that incompatible work uses a new stable ID. Wording only; the rollout guidance stays procedural and no runtime enforcement, conformance test, gate, fence, lease, or state is added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: tornquist <tornquist@squareup.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: no blocking defects found in the documentation contract. Reviewed head 1f033eb3d6ff0d3ec81f01583c9bb5ae2618f17b against base 01bacb8df3d2f5718e0a468828e07ae874a38eae.
The latest text closes the earlier rollout-recovery, retry-destination, and admin-reachability findings. The one inline suggestion clarifies worker fencing versus operator transitions; it is non-blocking and does not request new lifecycle machinery.
Source-only review of lifecycle, persistence/fencing, readiness, operator authority, and client projections. No code or tests executed. Existing CI was inspected, not rerun; it does not establish runtime conformance for this design. Current deployment routing/auth implementation work remains a future implementation obligation, not a prerequisite for this documentation PR.
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: no blocking defects found in the documentation contract. The previous optional clarification is addressed: worker execution commits retain current-owner/live-claim checks, while claimless operator transitions and execution-advancing validation/completion retain their lifecycle/generation fences.
Reviewed HEAD bb2b0a7d75840c1041758d09a4c6065f2d8f520f against BASE 01bacb8df3d2f5718e0a468828e07ae874a38eae, focusing re-review on the single fencing-language hunk since previously clear 1f033eb3d6ff0d3ec81f01583c9bb5ae2618f17b and its consequences across the full lifecycle contract. No new rollout machinery or runtime implementation prerequisites are requested.
Source-only review on the pinned Blox; no PR code or tests executed. Observed existing exact-head CI checks passed, but do not prove future runtime conformance to this design. This is a non-approving review comment.
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Re-review at bb2b0a7d75840c1041758d09a4c6065f2d8f520f (base 01bacb8df). Thanks for working through the earlier round. Review clear: no blocking findings.
The three things from my earlier review at 136816646 are closed:
- Write-path prerequisite. Lines 124–132 now require the normal write path to be fully rolled out, with old in-flight behavior drained, before
start. They make that an operator/developer responsibility rather than something the framework detects, and they say plainly that a violation needs a new stable ID. Carl's P1 about the "rerun" wording is folded in too. I'm fine without the late-commit conformance case now that the requirement is explicit. retrydestination. Line 155 now uses the samerunning/validatingformula asresume, which matches the lifecycle table.- Admin reachability while gated. Lines 307–309 state it, and conformance case 13 (lines 482–487) exercises recovery from
paused/blocked/failedthrough the production admin boundary while serving stays gated.
The fencing split at lines 255–272 also reads correctly. Worker execution commits carry the owner/live-claim checks. Operator transitions and execution-advancing validation keep the lifecycle and generation fences without needing a claim.
Non-blocking:
- MINOR / Correctness: automatic mode can't honor the rollout prerequisite if the writer and the backfill ship together. In the
on/onandoff/oncells,starthappens at relay startup. Under the chart'sRollingUpdatewithmaxUnavailable: 0, the first new pod would capture the bound and begin repairing while old pods are still serving and writing with the old behavior. That's exactly the case lines 124–132 say leaves unrepaired rows behind the checkpoint. The spec already implies the answer, but I think one sentence would save an implementer from the trap: a required automatic backfill must ship in a later release than the write path it depends on, or else be started manually after the writer rollout completes.
Two implementation notes, not spec defects: on current main, admin routes sit on the same readiness-filtered Service and router as ordinary traffic, and startup runs before the health listener binds. So the admin reachability and "remain unready" requirements will need a path that doesn't depend on ready endpoints, plus a non-blocking gate rather than a pre-listener wait. The spec states the outcomes without claiming current code provides them, which seems right for a design contract.
Signed-off-by: tornquist <tornquist@squareup.com> Co-authored-by: Codex <noreply@openai.com>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Re-review at 72a07511fbeaff2ad0e6192b5aae59b468137c1e (base 01bacb8df). Review clear: no blocking findings.
The new paragraph at L301-L305 closes the MINOR from my last review. A required automatic backfill now has to ship in a release after its writer is rolled out and drained, so startup capturing the bound in the off/on and on/on cells can't race old writers the way L124-L132 warns about. A definition that ships alongside its writer is manual-only for that release. I also checked the deadlock case: a new pod gated on a pending repair means the rollout can't finish, and manual start isn't allowed until the rollout finishes. L302-L303 already excludes that, because by the L69 definition an ID that's part of startup readiness in that release is a required automatic backfill, and it can't ship with its writer. The L307-L311 serving gate is untouched.
One optional wording nit: "MUST NOT auto-start" on its own could be read as "stays in the required set but just isn't started". If you touch it again, I think saying it must not be a required automatic backfill in that release (neither started nor gated on) would make the exclusion explicit, instead of leaving it to be inferred from the sentence before. Not blocking.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: no blocking documentation defects found. The release-ordering paragraph addresses the remaining non-blocking rollout note: a required automatic repair ships after its safe write path is fully deployed and old writers are drained; a same-release definition waits for manual initiation after rollout. The manual configuration cells provide that path without adding a readiness gate or framework rollout-detection machinery.
Reviewed HEAD 72a07511fbeaff2ad0e6192b5aae59b468137c1e against BASE 01bacb8df3d2f5718e0a468828e07ae874a38eae, focusing on the six-line delta since the previous clear review and its consequences for configuration, startup readiness, and the settled lifecycle contract. No further changes requested for this documentation PR; deployment routing and persistence details remain future implementation obligations.
Source-only review on the pinned Blox; no PR code, builds, or tests executed. Existing CI associated with this head was still pending at the inspected snapshot. CI does not establish runtime conformance to a documentation-only design. This is a non-approving review comment.
Why
Data backfills need a durable contract outside schema migrations. The contract must survive crashes and support automatic or manual operation without adding version-selection machinery.
What
How
A backfill captures one immutable upper bound, processes finite batches, and commits each target mutation with its checkpoint. PostgreSQL owns lifecycle state, claims, progress, and completion.
Registration order and build metadata do not choose a definition. Compatible processes are interchangeable. A material behavior change requires a new stable ID. The orchestrator does not resolve different behavior registered under one active ID. Diagnostic validation on completed rows records only its bounded outcome and audit; it does not create an execution claim.
The spec now starts with a six-step mental model and follows the operator flow. It removes repeated requirements and cuts the document from 614 lines and 4,835 words to 474 lines and 3,656 words.
Risk
Low. This PR changes documentation only. It commits Buzz to a design contract, but it does not add runtime, schema, configuration, or protocol behavior.
Testing
No manual runtime testing. A fresh full-spec review checked the lifecycle, safety rules, four configuration cells, admin contract, and production-seam conformance requirements.
git diff --checkand the repository file-size policy pass at13681664674d4667b181a6211f012090fa40a2cf.Bigger picture
The design keeps the relay as orchestrator, a focused backfill crate as domain owner, and
buzz-dbas narrow transaction infrastructure. It removes deployment generations and definition precedence from the implementation plan.Generated with Codex