fix(storage): no-op-commit gate — value-identical bd update mints no Dolt commit (vp-5u7i, ADR-0023 L-A) - #27
fix(storage): no-op-commit gate — value-identical bd update mints no Dolt commit (vp-5u7i, ADR-0023 L-A)#27voxist-merge-bot[bot] wants to merge 4 commits into
Conversation
bourgois
left a comment
bourgois
left a comment
There was a problem hiding this comment.
Code review — CHANGES REQUESTED. The no-op-commit gate is well-designed and row_lock-safe on the issueops path, but there's a real, untested parity break on the critical write path.
Blocking
1. The uow/domain path omits the UpdateWouldSideEffect guard that the issueops path has — silent side-effect loss.
UpdateWouldSideEffect is called only at internal/storage/uow/issueops/update.go:590. The gate in internal/storage/domain/db/issue.go:203 suppresses purely on AllColumnsEqualInTx over the requested columns, but that path still runs ManageClosedAt/ManageStartedAt. So:
- Idempotent
status=closedon an already-closed row: issueops deliberately does NOT suppress (itsUpdateWouldSideEffectreturns true; the helper comment says "ManageClosedAt stamps closed_at = now on EVERY implicit close, even when already closed — never suppress that"). The uow gate now suppresses it → loses theclosed_atre-stamp AND theEventClosedemission. status=in_progresswhenstarted_at IS NULL(legacy/migrated rows): issueops stampsstarted_at; the uow gate suppresses →started_atstays NULL.
This contradicts the PR body's claim that both paths keep the auto-managed stamps. It's bounded (no genuine open→closed transition is lost — those compare unequal and still write), which keeps it below a hard block on data integrity, but it's real behavior loss on the bd write path and is untested (update_nochange_test.go covers in_progress with started_at already set, but not the closed-already-closed re-stamp nor the started_at IS NULL case).
Fix: call UpdateWouldSideEffect(oldIssue, updates) before the issue.go suppression too (fetch oldIssue as issueops does), OR explicitly justify + test the divergence.
Required
- (a) Apply
UpdateWouldSideEffectto theissue.gopath, or document + test the intended divergence. - (b) Add the
status=closed-already-closed andstarted_at IS NULLcases toupdate_nochange_test.go. - (c) Rebase — the branch is on the 0051 line (
117bb41e7);row_lockwas added on the 0054 line now on main. Post-rebase the composition is still correct (the gate returns beforerow_lockis appended, androw_lockis never a compared column), but re-verify it after rebasing, and note the interaction with the vp-bijm v54→v51 store-revert decision.
Non-blocking
issueops/nochange.go:504UpdateWouldSideEffectdoesn't account forManageLeaseOnUpdate; harmless in all realistic states (stale-lease clearing isReclaimExpiredLeases' job) — add a one-line doc note that lease-clearing is intentionally out of scope.
Nice work on the in-tx probe, fall-through-on-error default, and the binary-collation case-only test. Once (a)-(c) land this is good to merge.
…olt commit (vp-5u7i, ADR-0023 L-A)
A bd update that changes no tracked field used to bump updated_at and
insert an event row on every call, so each enclosing DOLT_COMMIT minted a
real commit: measured 1403/1403 (100%) no-op bd:update commits per store
per day, and each redundant write was a stale-snapshot race window that
could clobber a concurrent claim (va-v1i9 forensics).
Both update paths (issueops.updateIssueInTx and the domain/db repository
used by the proxied uow path) now build their column/value pairs once and
probe the row in-tx via NULL-safe equality (issueops.AllColumnsEqualInTx;
metadata compared as JSON so key order/whitespace differences are not
deltas). Only a positive "unchanged" suppresses the write — probe errors
fall through to the legacy write path, sql.ErrNoRows is preserved, and
UpdateWouldSideEffect keeps auto-managed stamps (pinned auto-clear,
ManageClosedAt, ManageStartedAt) writing. With the working set clean, the
existing nothing-to-commit tolerance (dolt wrappers' isDoltNothingToCommit,
uow's isNothingToCommit) yields zero new commits with no error surfacing.
Tests: domain/db suite pins clean-working-set/no-event/no-updated_at on
no-ops plus the data-loss guards (real change still writes exactly once,
mixed update writes, case-only change writes under binary collation,
NULL-to-NULL is a no-op, missing id still ErrNoRows, wisp variant); cmd/bd
embedded tests prove bd update end-to-end: no-op (plain/--set-metadata/
--status) advances dolt_log by 0, real change by exactly 1.
The equality probe scans its boolean result into a Go bool: the server
wire protocol returns 1/0 (or []byte("1")) but the embedded engine driver
returns a native bool, and an int64 scan destination errors on it — which
silently disabled the gate in embedded mode via the designed probe-error
fall-through. Caught by the cmd/bd embedded tests; bool scan accepts all
three driver representations via database/sql driver.Bool.
…w coverage Addresses bourgois' PR #27 review: - Cover the two UpdateWouldSideEffect cases the domain/db gate previously missed parity tests for: re-closing an already-closed issue (must still re-stamp closed_at and emit EventClosed) and an in_progress re-stamp on a legacy row with started_at IS NULL (must still stamp started_at). - Fix a bug the new closed-already-closed test exposed: when the side-effect gate lets a value-identical write through, the UPDATE can report RowsAffected=0 (MySQL/Dolt count rows changed, not rows matched) even though the row exists — the existing rows==0 check then raised a false sql.ErrNoRows. Since oldIssue is only ever populated by a read that already proved the row exists, only fall back to RowsAffected as an existence check when that read didn't happen. - Add a one-line doc note on UpdateWouldSideEffect: it deliberately does not account for ManageLeaseOnUpdate, since a stale lease left on a suppressed no-op is ReclaimExpiredLeases' job, not this gate's.
|
Addressed all three items from the review (head now 3ce4a47): (a) UpdateWouldSideEffect parity — fixed. (b) Regression tests added in
Writing the first test surfaced a real bug: when the side-effect gate lets a value-identical write through, the UPDATE can report (c) Rebased onto current
Verified: |
…le 2 This implements the commit-rate watchdog as specified in vp-5u7i bead. Creates a new 'monitor-commit-rate' command that samples dolt_log per DB and alerts when any DB exceeds N no-op commits/min with a flat distinct-bead count. The signature of the issue is: many commits, few beads, identical content_hash. This serves as a local backstop that catches recurrence of the no-op commit storm without waiting on upstream changes.
Applied from the cli-docs-freshness-patch artifact of https://github.com/Voxist/beads/actions/runs/30327138940 (generated with CI's canonical pinned build). See scripts/check-cli-docs-drift.sh for how drift is attributed.
|
Pushed Note: this commit was pushed with the default workflow token, which does not retrigger PR checks - re-run them (or push any commit) to refresh the gate. Configuring a |
bourgois
left a comment
bourgois
left a comment
There was a problem hiding this comment.
PA validation gate — APPROVED (platform-architect, ADR-0023 LEAD va-v1i9)
Re-reviewed live after the rebase; not trusting "CHANGES_REQUESTED addressed." Verified each of @bourgois's three required items (2026-07-17) against the current diff at d05dc48:
(a) UpdateWouldSideEffect on the domain/db path — DONE.
internal/storage/domain/db/issue.go now calls issueops.UpdateWouldSideEffect(oldIssue, updates) before AllColumnsEqualInTx, with oldIssue fetched via the existing status-snapshot read. Parity with the issueops path confirmed. A redundant status=closed re-stamp is no longer suppressed.
(b) Test cases — DONE, and they assert the fixed behavior.
AlreadyClosedStatusReStampsClosedAtAndEmitsEvent: re-close assertsEventClosedcount increments AND working-set dirties (closed_at re-stamp). Would fail if the side-effect gate regressed.InProgressWithNullStartedAtStillStampsStartedAt: forces legacystarted_at IS NULLdirectly, asserts it gets stamped.NullableClearToNullIsNoOpWhenAlreadyNull: pins the NULL-safe<=>semantics.
These pin the correct behavior — not the buggy output (the va-6sx trap).
(c) Rebase off 0051 → main — DONE.
Base is 9fa3ea83a = current Voxist/beads main HEAD = deployed fleet bd 1.1.0. main-not-in-head = 0 (not behind). The 0051/117bb41e7 precondition is obsolete: vp-bijm is CLOSED because the fleet converged UP to schema 54 uniformly (9/9 stores), and the deployed binary is 1ccfeb841 (1.1.0), not 1.0.5/117bb41e7. The vp-bijm interaction the review flagged is resolved — rebase-to-main was correct and has happened.
Root-cause layer check — the right path is gated. The 2026-07-23 finding (prior bd fix PR #28 guarded only maybeAutoCommit in embedded mode while this fleet runs SQL-server mode) is ruled out: this PR gates the mode-agnostic storage write decision in both issueSQLRepositoryImpl.Update and issueops.updateIssueInTx — not a mode-specific commit hook. Column/value pairs are built once (probe and UPDATE see identical args → no probe/write TOCTOU). Errors fall through to the legacy write (fail-open; only a positive "unchanged" suppresses). sql.ErrNoRows contract preserved via the probe.
bool/int64 silent-disable trap — caught. AllColumnsEqualInTx scans into a Go bool, with a documented note that the server wire protocol returns 1/0 while the embedded driver returns a native bool — an int64 destination would error on bool and silently disable the gate in embedded mode. This is exactly the failure mode that made PR #28 a non-fix on this fleet; it is addressed here.
CI note. GitHub Actions suites report action_required (an action_required gate, not absence of CI); workflows exist (main.yml, conformance.yml, migration-test.yml). The added tests target a containerized real Dolt server (server mode = what this fleet runs). Acceptable for the gate; flag the action_required runs to the operator if a green pipeline is wanted before merge.
Out of scope for this PR (tracked separately on va-v1i9). The 2026-07-24 wipe-to-"" pull-race class is NOT covered by content-hash-skip (a wipe to empty is a genuine content change; hashes differ). That requires "make gc.routed_to writes pull-race-safe," which vp-5u7i explicitly excludes. I am landing that as a separate decision/bead off the va-v1i9 umbrella — do not block this merge on it.
Supersedes my 2026-07-17 CHANGES_REQUESTED. Ready to merge.
1 participant
What
bdpersisted no-op updates as real Dolt commits: abd updatethat changes no tracked field still bumpedupdated_atand inserted an event row, so the enclosingDOLT_COMMITminted a commit every time — measured 1403/1403 (100%) no-opbd: updatecommits in 24h on one store (va), and each redundant write is a stale-snapshot read-modify-write window that can clobber a concurrent claim on an unrelated bead (va-v1i9 forensics; ADR-0023 L-A — this is the storm amplifier and the claim-evaporation risk multiplier).Both update paths (
issueops.updateIssueInTx, and thedomain/dbrepository used by the uow path) now build their column/value pairs once and probe the row in-tx via NULL-safe equality (issueops.AllColumnsEqualInTx; metadata compared as JSON so key order/whitespace are not deltas). Only a positive "unchanged" suppresses the write — probe errors fall through to the legacy write path,sql.ErrNoRowsis preserved, andUpdateWouldSideEffectkeeps auto-managed stamps (pinned auto-clear, ManageClosedAt, ManageStartedAt) writing.The probe scans its boolean result into a Go
bool, notint64: the embedded engine's driver returns a native bool where the server wire protocol returns 1/0 — an int64 destination errors on bool and silently disabled the gate in embedded mode via the designed fall-through. Caught by the new cmd/bd embedded tests during verification.Tested
internal/storage/domain/dbsuite (containerized real Dolt server): no-op leaves the working set clean / no event / noupdated_atbump; real change still writes exactly once; mixed update writes; case-only change writes under binary collation; NULL→NULL is a no-op; missing id keepssql.ErrNoRows; wisp-table variant.BEADS_TEST_EMBEDDED_DOLT=1 go test ./cmd/bd/ -run NoOp: end-to-endbd updateno-op (plain /--set-metadata/--status) advancesdolt_logby 0, real change by exactly 1 — 3/3 pass.dolt log/dolt diff).Reviewer flags (deliberate, for the PA gate)
117bb41e7(0051 migration line) per the architect's re-base precondition on vp-5u7i. Today's upstream resync (Resync bfork/main with upstream (2026-07-15, 181 commits) #26) movedmainto the 0054 line; there is one content conflict vsmainininternal/storage/domain/db/issue.go(CHANGELOG andissueops/update.goauto-merge). Rebase/base strategy deliberately left to the PA — it interacts with the vp-bijm v54→v51 store-revert decision.Bead:
vp-5u7i(voxist-platform store). Decision doc:voxist-city/docs/decisions/ADR-0023-dolt-no-op-commit-storm-and-routing-convergence.md.