compact integrity gate: same-count hash drift has no writer-race defer path, so an ordinary bd update hard-quarantines the db and blocks reclaim (gc-800l fix gap) (gc-i52hj) - #147
Merged
Conversation
…(gc-i52hj)
The post-flatten integrity gate had three defer arms and every one of them was
guarded off by `same_row_count_hash_drift`. That is the category an ordinary
in-place `bd update` produces — the single most common write in a running city
— so a proven writer race whose only anomaly was same-count drift fell through
to a hard quarantine, and a quarantine marker blocks all future GC of that
database until a human clears it.
lx hit this on 2026-08-15 (bead lx-ey1xe). Both writer-race signals fired, the
concurrent writer was identifiable by name ("bd: update lx-szxza", six seconds
after the flatten), and DOLT_DIFF_STAT over the window showed rows_added 0,
rows_deleted 0, rows_modified 1, rows_unmodified 1237. Nothing was lost. The
marker still blocked reclaim for 4 days; lx reached 1.8G, 7-11x every other
database on a host already at 80%, and clearing it reclaimed ~900MB in 50s.
Third quarantine of this class after lx 2026-08-09 (gc-800l) and gc 2026-08-11.
The fix is the rule gc-800l's own "## Fix" section specified and its
implementation did not carry to this category: counts and hashes are the cheap
TRIGGER, the verdict comes from the diff. Any table value-hash drift — with a
row-count gain or at an unchanged count — is now settled by diffing the
pre-flight snapshot HEAD against the flatten commit per drifted table and
counting `removed` rows. Zero removals proves every pre-flight row is still
reachable, so it defers and retries next run. Added and modified rows are
ordinary live-store traffic. Any removed row, or a probe failure, fails closed
to the quarantine exactly as before.
Note this drops the `writer_race_detected` precondition for the drift path
rather than adding same-count as a fourth race-gated arm: the DOLT_DIFF proof
is direct evidence, strictly stronger than the HEAD proxy it replaces, so it
does not need the proxy to agree first. That also covers the absorbed-writer
race the HEAD gate structurally misses. Row-count decrease keeps its own
HEAD-proven arm — fewer rows with no removals is a contradiction, so admitting
it could only ever fail closed — and table-list drift and probe failures stay
excluded for the same fail-closed reason.
`diff_is_additive_only` is deliberately left alone. Upstream gastownhall#5049 gave it a
second consumer, the committed-root drift proof's first-committed table case,
where the table exists in no pre-flatten commit and so has no pre-flight rows
to preserve; there anything but a pure add really is unexplained. The two
predicates now ask different questions and a unit test asserts each keeps
asking its own, so a later cleanup cannot quietly collapse them.
Validation, positive control both ways per gc-800l's bar. The new
`same_row_count_row_loss` harness mode produces the identical count/hash
signature as the benign in-place update and differs only in what DOLT_DIFF
reports, so the two Go tests isolate the discriminator: same-count drift with
no removals defers and writes no quarantine marker
(TestCompactScriptDefersSameRowCountWriterDrift), and the same signature with
removals still quarantines before full GC with its drift evidence recorded
(TestCompactScriptQuarantinesSameRowCountDriftWithRemovedRows). A run that only
showed "no longer quarantines" would have removed the alarm, not fixed it.
Full `examples/bd/dolt` package green (153s); 14/14 in the shell unit test.
The harness's removal-probe arm has to sit ahead of the `SELECT COUNT(*) FROM
<table>` arms, whose patterns also match the proof query's text, and matches on
the `diff_type = 'removed'` clause so it stays clear of the additive-only probe
answered further down.
Recovered from 8f2a2b1 (PR#107), which landed this fix and was shed when main
was rebased onto upstream; that commit is an ancestor of no current ref. Ported
onto the evolved upstream rather than cherry-picked, since gastownhall#5049 split the
shared helper out from under it. Its bundled `quarantine_db_hash_field` change
— rendering the marker's two database-hash fields as four distinct states — is
a separate concern and is NOT included here; filed as gc-c845v.
…ft verdict (gc-i52hj)
Two self-review touch-ups on the row-preservation change. Both are
comment/prose only — no executable line moves.
run.sh: the new row-preservation proof was inserted into the header
summary as step 4b, but 4b was already the committed-root drift gate, so
the file listed two different steps under one label. The prose reference
added in the same commit ("settled by the removal-based proof in step
4b") points at the new one, so the committed-root gate becomes 4c.
dolt-bloat-recovery.md: the new paragraph stated the removal rule
unconditionally — "quarantines only when rows were removed" — but the
drift path is also guarded by saw_row_decrease, saw_table_list_change,
and saw_probe_failure. In any of those the run never reaches the
DOLT_DIFF proof and quarantines regardless of removals, which is exactly
the case an operator would misread the old wording against (the mixed
decrease-plus-same-count-drift run, where the same-count reason string
can be written with no removal probe run at all). Adds the scope
sentence rather than lengthening the two table cells above it, which the
paragraph now covers.
Gates: 14/14 shell unit test; examples/bd/dolt package green (72s), with
the four positive controls selected by name and passing in both
directions (defer on no removals, quarantine on removals);
go vet ./examples/bd/dolt/... clean; go build ./... clean;
make check-docs green.
Author
|
Codex signoff (pre-open, comment-only — not an approval): VERDICT: COMMENT Scope checked: full diff for docs/troubleshooting/dolt-bloat-recovery.md, examples/bd/dolt/assets/scripts/compact-gain-drift-proof.sh, examples/bd/dolt/commands/compact/run.sh, examples/bd/dolt/dog_exec_scripts_test.go, and test/dolt/compact_gain_drift_proof_test.sh. Checked the anchor bead requirements, the helper/caller rename surface at the pinned commit, the row-preservation defer path, pending-GC handoff, first-committed-table additive-only path, docs wording, and stale-reference risk. Findings: none blocking. No P0/P1 findings. Verification: ./test/dolt/compact_gain_drift_proof_test.sh -> 14 passed, 0 failed; go test ./examples/bd/dolt/... -> passed; make check-docs -> passed; git diff --check origin/main...e13bc84 -> passed. Not checked: full repository test suite beyond the focused Dolt example package and docs sync gate; refinery already recorded make build, make vet, make test, and make check-docs at this same commit. |
johnzook
approved these changes
Aug 20, 2026
zook-bot
added a commit
that referenced
this pull request
Aug 20, 2026
…(gc-0qbf5)
Nothing told an operator that merged fixes were not executing. A fix lands, the
bead closes, the PR merges -- and the running city keeps executing the older
image indefinitely, with every existing signal reading clean.
This is the missing link in a three-part chain:
origin/main --(new)--> on-disk binary --(gc start)--> supervisor
`gc start`'s DetectBinaryDrift already compares the supervisor's reported
buildID against the local binary's, catching a supervisor left on a stale
image. It is structurally blind to the failure mode here, where supervisor and
on-disk binary agree perfectly and BOTH are days behind main. Nothing in that
state looks wrong: the binary's mtime is recent enough to be plausible, the
beads are closed, the PRs are merged. The only evidence is code that silently
is not running, which is why this has now cost real time twice (gc-f1081 was
the same shape at five days, differing only in that a deleted inode held the
supervisor).
BinaryFreshnessCheck reads the running binary's stamped vcs.revision, finds the
configured rig whose object database CONTAINS that commit, and reports
`git rev-list --count <build>..origin/<default>`. Verified against this town:
build f475b68 (2026-08-17) is 4 commits behind origin/main (2026-08-20), and
the four it names are exactly the four the bead identified -- #146, #148, #147,
#149.
Three design points worth stating, since each rules out an easier alternative:
- Repo identity is COMMIT CONTAINMENT, not a name, path, or remote-URL match.
This repo is regularly built from a fork whose origin differs from the module
path, so a URL match would fail exactly here; and any name match would smuggle
a repository identity into Go. The repo that can resolve the commit is the
repo the binary was built from.
- It never fetches. Comparing against the last-fetched remote-tracking ref keeps
the check free of network I/O and side effects inside `gc doctor`. The cost is
understating drift when the checkout itself is stale, so the finding names the
ref it read and says how to get a current reading -- the reading is never
presented as live.
- Severity is advisory and CanFix is false. The remedy is a rebuild plus
`systemctl --user restart`, and that restart bounces the tmux server hosting
every agent session. That is an operator decision, not something a gate should
force. The fix_hint deliberately gives both halves as one command, because a
rebuild WITHOUT an immediate restart recreates the deleted-inode state
gc-f1081 tracked -- the two must happen together.
Every not-applicable state resolves to StatusOK rather than a warning: no
stamped revision (-buildvcs=false), git absent, no configured rig holding the
commit, or no fetched tracking ref. None of those is a stale binary, and
warning on them would produce exactly the unclearable noise this check exists to
replace.
Validation: eight tests over real git repos in t.TempDir covering at-tip,
behind, ahead-of-origin, unstamped revision, no-owning-rig, missing tracking
ref, a non-main default branch, and multi-rig selection.
Three notes from writing them, each a trap that cost a cycle:
- The helper seeds each repo's file content with its own temp path. Git commits
are content-addressed, so two repos built from identical trees, messages and
timestamps produce IDENTICAL SHAs -- which silently defeated the multi-rig
test until the trees were made distinct.
- The non-main-default-branch case exists because the linter flagged the branch
parameter as always-"main". That was a real coverage gap (the check resolves
EffectiveDefaultBranch), not a dead parameter, so the fix was a test rather
than a narrower signature.
- The test constructs NO os/exec commands of its own, reusing the package's
existing runGitForRigRootBranchTest and the production runGitCommand instead.
The repository budgets os/exec construction sites per file
(internal/testpolicy/resourcecensus), and a first draft that defined its own
two helpers pushed the ledger over baseline. Consolidating onto the existing
helpers keeps the new file at zero sites, so no census baseline is raised --
the budget is meant to be spent down, not ratcheted up.
`binary-freshness` added to cmd/gc/testdata/doctor_check_names.golden in
registration order. internal/doctor, internal/testpolicy, internal/testenv,
internal/productmetrics and `go test ./cmd/gc/ -run Doctor` all green;
go vet ./... clean.
Claude-Session: https://claude.ai/code/session_012NnWPBtpRiQFJBaYFQbYyz
zook-bot
added a commit
that referenced
this pull request
Aug 20, 2026
…(gc-0qbf5) (#152) Nothing told an operator that merged fixes were not executing. A fix lands, the bead closes, the PR merges -- and the running city keeps executing the older image indefinitely, with every existing signal reading clean. This is the missing link in a three-part chain: origin/main --(new)--> on-disk binary --(gc start)--> supervisor `gc start`'s DetectBinaryDrift already compares the supervisor's reported buildID against the local binary's, catching a supervisor left on a stale image. It is structurally blind to the failure mode here, where supervisor and on-disk binary agree perfectly and BOTH are days behind main. Nothing in that state looks wrong: the binary's mtime is recent enough to be plausible, the beads are closed, the PRs are merged. The only evidence is code that silently is not running, which is why this has now cost real time twice (gc-f1081 was the same shape at five days, differing only in that a deleted inode held the supervisor). BinaryFreshnessCheck reads the running binary's stamped vcs.revision, finds the configured rig whose object database CONTAINS that commit, and reports `git rev-list --count <build>..origin/<default>`. Verified against this town: build f475b68 (2026-08-17) is 4 commits behind origin/main (2026-08-20), and the four it names are exactly the four the bead identified -- #146, #148, #147, #149. Three design points worth stating, since each rules out an easier alternative: - Repo identity is COMMIT CONTAINMENT, not a name, path, or remote-URL match. This repo is regularly built from a fork whose origin differs from the module path, so a URL match would fail exactly here; and any name match would smuggle a repository identity into Go. The repo that can resolve the commit is the repo the binary was built from. - It never fetches. Comparing against the last-fetched remote-tracking ref keeps the check free of network I/O and side effects inside `gc doctor`. The cost is understating drift when the checkout itself is stale, so the finding names the ref it read and says how to get a current reading -- the reading is never presented as live. - Severity is advisory and CanFix is false. The remedy is a rebuild plus `systemctl --user restart`, and that restart bounces the tmux server hosting every agent session. That is an operator decision, not something a gate should force. The fix_hint deliberately gives both halves as one command, because a rebuild WITHOUT an immediate restart recreates the deleted-inode state gc-f1081 tracked -- the two must happen together. Every not-applicable state resolves to StatusOK rather than a warning: no stamped revision (-buildvcs=false), git absent, no configured rig holding the commit, or no fetched tracking ref. None of those is a stale binary, and warning on them would produce exactly the unclearable noise this check exists to replace. Validation: eight tests over real git repos in t.TempDir covering at-tip, behind, ahead-of-origin, unstamped revision, no-owning-rig, missing tracking ref, a non-main default branch, and multi-rig selection. Three notes from writing them, each a trap that cost a cycle: - The helper seeds each repo's file content with its own temp path. Git commits are content-addressed, so two repos built from identical trees, messages and timestamps produce IDENTICAL SHAs -- which silently defeated the multi-rig test until the trees were made distinct. - The non-main-default-branch case exists because the linter flagged the branch parameter as always-"main". That was a real coverage gap (the check resolves EffectiveDefaultBranch), not a dead parameter, so the fix was a test rather than a narrower signature. - The test constructs NO os/exec commands of its own, reusing the package's existing runGitForRigRootBranchTest and the production runGitCommand instead. The repository budgets os/exec construction sites per file (internal/testpolicy/resourcecensus), and a first draft that defined its own two helpers pushed the ledger over baseline. Consolidating onto the existing helpers keeps the new file at zero sites, so no census baseline is raised -- the budget is meant to be spent down, not ratcheted up. `binary-freshness` added to cmd/gc/testdata/doctor_check_names.golden in registration order. internal/doctor, internal/testpolicy, internal/testenv, internal/productmetrics and `go test ./cmd/gc/ -run Doctor` all green; go vet ./... clean. Claude-Session: https://claude.ai/code/session_012NnWPBtpRiQFJBaYFQbYyz
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…r path, so an ordinary bd update hard-quarantines the db and blocks reclaim (gc-800l fix gap) (gc-i52hj) (#147) * fix(dolt): settle same-count hash drift on removals, not on the hash (gc-i52hj) The post-flatten integrity gate had three defer arms and every one of them was guarded off by `same_row_count_hash_drift`. That is the category an ordinary in-place `bd update` produces — the single most common write in a running city — so a proven writer race whose only anomaly was same-count drift fell through to a hard quarantine, and a quarantine marker blocks all future GC of that database until a human clears it. lx hit this on 2026-08-15 (bead lx-ey1xe). Both writer-race signals fired, the concurrent writer was identifiable by name ("bd: update lx-szxza", six seconds after the flatten), and DOLT_DIFF_STAT over the window showed rows_added 0, rows_deleted 0, rows_modified 1, rows_unmodified 1237. Nothing was lost. The marker still blocked reclaim for 4 days; lx reached 1.8G, 7-11x every other database on a host already at 80%, and clearing it reclaimed ~900MB in 50s. Third quarantine of this class after lx 2026-08-09 (gc-800l) and gc 2026-08-11. The fix is the rule gc-800l's own "## Fix" section specified and its implementation did not carry to this category: counts and hashes are the cheap TRIGGER, the verdict comes from the diff. Any table value-hash drift — with a row-count gain or at an unchanged count — is now settled by diffing the pre-flight snapshot HEAD against the flatten commit per drifted table and counting `removed` rows. Zero removals proves every pre-flight row is still reachable, so it defers and retries next run. Added and modified rows are ordinary live-store traffic. Any removed row, or a probe failure, fails closed to the quarantine exactly as before. Note this drops the `writer_race_detected` precondition for the drift path rather than adding same-count as a fourth race-gated arm: the DOLT_DIFF proof is direct evidence, strictly stronger than the HEAD proxy it replaces, so it does not need the proxy to agree first. That also covers the absorbed-writer race the HEAD gate structurally misses. Row-count decrease keeps its own HEAD-proven arm — fewer rows with no removals is a contradiction, so admitting it could only ever fail closed — and table-list drift and probe failures stay excluded for the same fail-closed reason. `diff_is_additive_only` is deliberately left alone. Upstream gastownhall#5049 gave it a second consumer, the committed-root drift proof's first-committed table case, where the table exists in no pre-flatten commit and so has no pre-flight rows to preserve; there anything but a pure add really is unexplained. The two predicates now ask different questions and a unit test asserts each keeps asking its own, so a later cleanup cannot quietly collapse them. Validation, positive control both ways per gc-800l's bar. The new `same_row_count_row_loss` harness mode produces the identical count/hash signature as the benign in-place update and differs only in what DOLT_DIFF reports, so the two Go tests isolate the discriminator: same-count drift with no removals defers and writes no quarantine marker (TestCompactScriptDefersSameRowCountWriterDrift), and the same signature with removals still quarantines before full GC with its drift evidence recorded (TestCompactScriptQuarantinesSameRowCountDriftWithRemovedRows). A run that only showed "no longer quarantines" would have removed the alarm, not fixed it. Full `examples/bd/dolt` package green (153s); 14/14 in the shell unit test. The harness's removal-probe arm has to sit ahead of the `SELECT COUNT(*) FROM <table>` arms, whose patterns also match the proof query's text, and matches on the `diff_type = 'removed'` clause so it stays clear of the additive-only probe answered further down. Recovered from 8f2a2b1 (PR#107), which landed this fix and was shed when main was rebased onto upstream; that commit is an ancestor of no current ref. Ported onto the evolved upstream rather than cherry-picked, since gastownhall#5049 split the shared helper out from under it. Its bundled `quarantine_db_hash_field` change — rendering the marker's two database-hash fields as four distinct states — is a separate concern and is NOT included here; filed as gc-c845v. * docs(dolt): renumber the duplicated compact step 4b and scope the drift verdict (gc-i52hj) Two self-review touch-ups on the row-preservation change. Both are comment/prose only — no executable line moves. run.sh: the new row-preservation proof was inserted into the header summary as step 4b, but 4b was already the committed-root drift gate, so the file listed two different steps under one label. The prose reference added in the same commit ("settled by the removal-based proof in step 4b") points at the new one, so the committed-root gate becomes 4c. dolt-bloat-recovery.md: the new paragraph stated the removal rule unconditionally — "quarantines only when rows were removed" — but the drift path is also guarded by saw_row_decrease, saw_table_list_change, and saw_probe_failure. In any of those the run never reaches the DOLT_DIFF proof and quarantines regardless of removals, which is exactly the case an operator would misread the old wording against (the mixed decrease-plus-same-count-drift run, where the same-count reason string can be written with no removal probe run at all). Adds the scope sentence rather than lengthening the two table cells above it, which the paragraph now covers. Gates: 14/14 shell unit test; examples/bd/dolt package green (72s), with the four positive controls selected by name and passing in both directions (defer on no removals, quarantine on removals); go vet ./examples/bd/dolt/... clean; go build ./... clean; make check-docs green.
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…(gc-0qbf5) (#152) Nothing told an operator that merged fixes were not executing. A fix lands, the bead closes, the PR merges -- and the running city keeps executing the older image indefinitely, with every existing signal reading clean. This is the missing link in a three-part chain: origin/main --(new)--> on-disk binary --(gc start)--> supervisor `gc start`'s DetectBinaryDrift already compares the supervisor's reported buildID against the local binary's, catching a supervisor left on a stale image. It is structurally blind to the failure mode here, where supervisor and on-disk binary agree perfectly and BOTH are days behind main. Nothing in that state looks wrong: the binary's mtime is recent enough to be plausible, the beads are closed, the PRs are merged. The only evidence is code that silently is not running, which is why this has now cost real time twice (gc-f1081 was the same shape at five days, differing only in that a deleted inode held the supervisor). BinaryFreshnessCheck reads the running binary's stamped vcs.revision, finds the configured rig whose object database CONTAINS that commit, and reports `git rev-list --count <build>..origin/<default>`. Verified against this town: build f475b68 (2026-08-17) is 4 commits behind origin/main (2026-08-20), and the four it names are exactly the four the bead identified -- #146, #148, #147, #149. Three design points worth stating, since each rules out an easier alternative: - Repo identity is COMMIT CONTAINMENT, not a name, path, or remote-URL match. This repo is regularly built from a fork whose origin differs from the module path, so a URL match would fail exactly here; and any name match would smuggle a repository identity into Go. The repo that can resolve the commit is the repo the binary was built from. - It never fetches. Comparing against the last-fetched remote-tracking ref keeps the check free of network I/O and side effects inside `gc doctor`. The cost is understating drift when the checkout itself is stale, so the finding names the ref it read and says how to get a current reading -- the reading is never presented as live. - Severity is advisory and CanFix is false. The remedy is a rebuild plus `systemctl --user restart`, and that restart bounces the tmux server hosting every agent session. That is an operator decision, not something a gate should force. The fix_hint deliberately gives both halves as one command, because a rebuild WITHOUT an immediate restart recreates the deleted-inode state gc-f1081 tracked -- the two must happen together. Every not-applicable state resolves to StatusOK rather than a warning: no stamped revision (-buildvcs=false), git absent, no configured rig holding the commit, or no fetched tracking ref. None of those is a stale binary, and warning on them would produce exactly the unclearable noise this check exists to replace. Validation: eight tests over real git repos in t.TempDir covering at-tip, behind, ahead-of-origin, unstamped revision, no-owning-rig, missing tracking ref, a non-main default branch, and multi-rig selection. Three notes from writing them, each a trap that cost a cycle: - The helper seeds each repo's file content with its own temp path. Git commits are content-addressed, so two repos built from identical trees, messages and timestamps produce IDENTICAL SHAs -- which silently defeated the multi-rig test until the trees were made distinct. - The non-main-default-branch case exists because the linter flagged the branch parameter as always-"main". That was a real coverage gap (the check resolves EffectiveDefaultBranch), not a dead parameter, so the fix was a test rather than a narrower signature. - The test constructs NO os/exec commands of its own, reusing the package's existing runGitForRigRootBranchTest and the production runGitCommand instead. The repository budgets os/exec construction sites per file (internal/testpolicy/resourcecensus), and a first draft that defined its own two helpers pushed the ledger over baseline. Consolidating onto the existing helpers keeps the new file at zero sites, so no census baseline is raised -- the budget is meant to be spent down, not ratcheted up. `binary-freshness` added to cmd/gc/testdata/doctor_check_names.golden in registration order. internal/doctor, internal/testpolicy, internal/testenv, internal/productmetrics and `go test ./cmd/gc/ -run Doctor` all green; go vet ./... clean. Claude-Session: https://claude.ai/code/session_012NnWPBtpRiQFJBaYFQbYyz
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…r path, so an ordinary bd update hard-quarantines the db and blocks reclaim (gc-800l fix gap) (gc-i52hj) (#147) * fix(dolt): settle same-count hash drift on removals, not on the hash (gc-i52hj) The post-flatten integrity gate had three defer arms and every one of them was guarded off by `same_row_count_hash_drift`. That is the category an ordinary in-place `bd update` produces — the single most common write in a running city — so a proven writer race whose only anomaly was same-count drift fell through to a hard quarantine, and a quarantine marker blocks all future GC of that database until a human clears it. lx hit this on 2026-08-15 (bead lx-ey1xe). Both writer-race signals fired, the concurrent writer was identifiable by name ("bd: update lx-szxza", six seconds after the flatten), and DOLT_DIFF_STAT over the window showed rows_added 0, rows_deleted 0, rows_modified 1, rows_unmodified 1237. Nothing was lost. The marker still blocked reclaim for 4 days; lx reached 1.8G, 7-11x every other database on a host already at 80%, and clearing it reclaimed ~900MB in 50s. Third quarantine of this class after lx 2026-08-09 (gc-800l) and gc 2026-08-11. The fix is the rule gc-800l's own "## Fix" section specified and its implementation did not carry to this category: counts and hashes are the cheap TRIGGER, the verdict comes from the diff. Any table value-hash drift — with a row-count gain or at an unchanged count — is now settled by diffing the pre-flight snapshot HEAD against the flatten commit per drifted table and counting `removed` rows. Zero removals proves every pre-flight row is still reachable, so it defers and retries next run. Added and modified rows are ordinary live-store traffic. Any removed row, or a probe failure, fails closed to the quarantine exactly as before. Note this drops the `writer_race_detected` precondition for the drift path rather than adding same-count as a fourth race-gated arm: the DOLT_DIFF proof is direct evidence, strictly stronger than the HEAD proxy it replaces, so it does not need the proxy to agree first. That also covers the absorbed-writer race the HEAD gate structurally misses. Row-count decrease keeps its own HEAD-proven arm — fewer rows with no removals is a contradiction, so admitting it could only ever fail closed — and table-list drift and probe failures stay excluded for the same fail-closed reason. `diff_is_additive_only` is deliberately left alone. Upstream gastownhall#5049 gave it a second consumer, the committed-root drift proof's first-committed table case, where the table exists in no pre-flatten commit and so has no pre-flight rows to preserve; there anything but a pure add really is unexplained. The two predicates now ask different questions and a unit test asserts each keeps asking its own, so a later cleanup cannot quietly collapse them. Validation, positive control both ways per gc-800l's bar. The new `same_row_count_row_loss` harness mode produces the identical count/hash signature as the benign in-place update and differs only in what DOLT_DIFF reports, so the two Go tests isolate the discriminator: same-count drift with no removals defers and writes no quarantine marker (TestCompactScriptDefersSameRowCountWriterDrift), and the same signature with removals still quarantines before full GC with its drift evidence recorded (TestCompactScriptQuarantinesSameRowCountDriftWithRemovedRows). A run that only showed "no longer quarantines" would have removed the alarm, not fixed it. Full `examples/bd/dolt` package green (153s); 14/14 in the shell unit test. The harness's removal-probe arm has to sit ahead of the `SELECT COUNT(*) FROM <table>` arms, whose patterns also match the proof query's text, and matches on the `diff_type = 'removed'` clause so it stays clear of the additive-only probe answered further down. Recovered from 8f2a2b1 (PR#107), which landed this fix and was shed when main was rebased onto upstream; that commit is an ancestor of no current ref. Ported onto the evolved upstream rather than cherry-picked, since gastownhall#5049 split the shared helper out from under it. Its bundled `quarantine_db_hash_field` change — rendering the marker's two database-hash fields as four distinct states — is a separate concern and is NOT included here; filed as gc-c845v. * docs(dolt): renumber the duplicated compact step 4b and scope the drift verdict (gc-i52hj) Two self-review touch-ups on the row-preservation change. Both are comment/prose only — no executable line moves. run.sh: the new row-preservation proof was inserted into the header summary as step 4b, but 4b was already the committed-root drift gate, so the file listed two different steps under one label. The prose reference added in the same commit ("settled by the removal-based proof in step 4b") points at the new one, so the committed-root gate becomes 4c. dolt-bloat-recovery.md: the new paragraph stated the removal rule unconditionally — "quarantines only when rows were removed" — but the drift path is also guarded by saw_row_decrease, saw_table_list_change, and saw_probe_failure. In any of those the run never reaches the DOLT_DIFF proof and quarantines regardless of removals, which is exactly the case an operator would misread the old wording against (the mixed decrease-plus-same-count-drift run, where the same-count reason string can be written with no removal probe run at all). Adds the scope sentence rather than lengthening the two table cells above it, which the paragraph now covers. Gates: 14/14 shell unit test; examples/bd/dolt package green (72s), with the four positive controls selected by name and passing in both directions (defer on no removals, quarantine on removals); go vet ./examples/bd/dolt/... clean; go build ./... clean; make check-docs green.
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…(gc-0qbf5) (#152) Nothing told an operator that merged fixes were not executing. A fix lands, the bead closes, the PR merges -- and the running city keeps executing the older image indefinitely, with every existing signal reading clean. This is the missing link in a three-part chain: origin/main --(new)--> on-disk binary --(gc start)--> supervisor `gc start`'s DetectBinaryDrift already compares the supervisor's reported buildID against the local binary's, catching a supervisor left on a stale image. It is structurally blind to the failure mode here, where supervisor and on-disk binary agree perfectly and BOTH are days behind main. Nothing in that state looks wrong: the binary's mtime is recent enough to be plausible, the beads are closed, the PRs are merged. The only evidence is code that silently is not running, which is why this has now cost real time twice (gc-f1081 was the same shape at five days, differing only in that a deleted inode held the supervisor). BinaryFreshnessCheck reads the running binary's stamped vcs.revision, finds the configured rig whose object database CONTAINS that commit, and reports `git rev-list --count <build>..origin/<default>`. Verified against this town: build f475b68 (2026-08-17) is 4 commits behind origin/main (2026-08-20), and the four it names are exactly the four the bead identified -- #146, #148, #147, #149. Three design points worth stating, since each rules out an easier alternative: - Repo identity is COMMIT CONTAINMENT, not a name, path, or remote-URL match. This repo is regularly built from a fork whose origin differs from the module path, so a URL match would fail exactly here; and any name match would smuggle a repository identity into Go. The repo that can resolve the commit is the repo the binary was built from. - It never fetches. Comparing against the last-fetched remote-tracking ref keeps the check free of network I/O and side effects inside `gc doctor`. The cost is understating drift when the checkout itself is stale, so the finding names the ref it read and says how to get a current reading -- the reading is never presented as live. - Severity is advisory and CanFix is false. The remedy is a rebuild plus `systemctl --user restart`, and that restart bounces the tmux server hosting every agent session. That is an operator decision, not something a gate should force. The fix_hint deliberately gives both halves as one command, because a rebuild WITHOUT an immediate restart recreates the deleted-inode state gc-f1081 tracked -- the two must happen together. Every not-applicable state resolves to StatusOK rather than a warning: no stamped revision (-buildvcs=false), git absent, no configured rig holding the commit, or no fetched tracking ref. None of those is a stale binary, and warning on them would produce exactly the unclearable noise this check exists to replace. Validation: eight tests over real git repos in t.TempDir covering at-tip, behind, ahead-of-origin, unstamped revision, no-owning-rig, missing tracking ref, a non-main default branch, and multi-rig selection. Three notes from writing them, each a trap that cost a cycle: - The helper seeds each repo's file content with its own temp path. Git commits are content-addressed, so two repos built from identical trees, messages and timestamps produce IDENTICAL SHAs -- which silently defeated the multi-rig test until the trees were made distinct. - The non-main-default-branch case exists because the linter flagged the branch parameter as always-"main". That was a real coverage gap (the check resolves EffectiveDefaultBranch), not a dead parameter, so the fix was a test rather than a narrower signature. - The test constructs NO os/exec commands of its own, reusing the package's existing runGitForRigRootBranchTest and the production runGitCommand instead. The repository budgets os/exec construction sites per file (internal/testpolicy/resourcecensus), and a first draft that defined its own two helpers pushed the ledger over baseline. Consolidating onto the existing helpers keeps the new file at zero sites, so no census baseline is raised -- the budget is meant to be spent down, not ratcheted up. `binary-freshness` added to cmd/gc/testdata/doctor_check_names.golden in registration order. internal/doctor, internal/testpolicy, internal/testenv, internal/productmetrics and `go test ./cmd/gc/ -run Doctor` all green; go vet ./... clean. Claude-Session: https://claude.ai/code/session_012NnWPBtpRiQFJBaYFQbYyz
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
2 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.
Summary
The gc-800l fix (8f2a2b1, PR#107) taught the post-flatten integrity gate to
defer instead of quarantine when a concurrent writer is proven. It covers the
gain+drift and row-decrease categories. It does NOT cover
same_row_count_hash_drift -- which is the category an ordinary in-place bd
update produces, i.e. the single most common write in a running city.
Every defer path is guarded off by that one category. In
examples/bd/dolt/commands/compact/run.sh:
line 2582-2589 gain+drift defer requires same_count_hash_drift != 1
line 2633-2638 row-decrease defer requires same_count_hash_drift != 1
and the comment at 2579-2582 states the exclusion deliberately:
"Downgrade quarantine -> defer ONLY for the ambiguous gain+drift case when
a concurrent writer is proven. Every other integrity failure (row-count
decrease, same-count hash drift, table-list drift, probe failure) ...
still quarantine below unchanged."
So a proven writer race whose ONLY anomaly is a same-count hash drift falls
through to a hard quarantine marker, which blocks all future GC of that
database until a human clears it.
Observed: lx, 2026-08-15 (bead lx-ey1xe, closed 2026-08-19)
Both of the script's own writer-race signals fired (run.sh:2567-2574):
head_before_reset != head r1n23l2c... != bb2nrugp... -> race
post_verify_head != flatten_head vjv9i24f... != c6laencj... -> race
So writer_race_detected was 1. The concurrent writer is identifiable: the
post_verify HEAD vjv9i24f... IS the writer commit, "bd: update lx-szxza" by
committer beads at 20:34:58, six seconds after the flatten at 20:34:52.
Only anomaly was same-count drift on issues (1238 -> 1238, hash drifted).
No row decrease, no gain, no table-list change. Both defer paths were therefore
skipped on the same_count guard alone, and it quarantined.
The quarantine was a false positive. DOLT_DIFF_STAT over the drift window:
rows_added 0, rows_deleted 0, rows_modified 1, rows_unmodified 1237
cells_added 0, cells_deleted 0, cells_modified 3
old_row_count 1238, new_row_count 1238; issues was the only table in the diff
The single modified row is lx-szxza. Nothing was lost.
Cost
The marker blocked the post-flatten GC for 4 days. lx reached 1.8G on disk --
7-11x every other database (sl 144M, su 165M, gc 225M, tk 235M) -- on a host
already at 80% with prior ENOSPC history. Clearing the marker and re-running
gc dolt compact --gc-only --only-db lx reclaimed to 932M in 50s (~900MB, ~48%).
This is the third quarantine of this class: lx 2026-08-09 (gc-800l, the one the
fix was written for), gc db 2026-08-11 (lx-8nj9, still open, routed human),
lx 2026-08-15 (lx-ey1xe). Each one costs a human review and defers reclaim.
Fix
gc-800l's own "## Fix" section already specified the right rule and the
implementation did not carry it to this category. Quoting that bead:
Quarantine on removed > 0, or on a table disappearing.
Treat added and modified as expected on a live store.
modified IS same-count drift. So: when writer_race_detected is 1 and the only
anomaly is same_row_count_hash_drift, run the same per-table DOLT_DIFF
preservation proof already used by the gain+drift path and defer when
removed = 0 and no table disappeared. Keep the hard quarantine when the diff
shows removals, when no writer race is proven, or when a probe failed.
Verify
Positive control both ways, per gc-800l's own verification bar: with a city
running and doing ordinary bd updates, confirm a scheduled pass with same-count
drift completes and reclaims WITHOUT quarantining. Then construct a real row
deletion on a scratch db in the same shape and confirm it STILL quarantines. A
pass that only shows "no longer quarantines" has removed the alarm, not fixed it.
Implementation notes
2026-08-19 05:20Z: operator RELEASED for dispatch (tooling-work release rule satisfied). Dispatching to gascity polecat pool.
Implemented: post-flatten integrity gate now settles table value-hash drift on DOLT_DIFF removals instead of the hash. Same-count drift (an ordinary in-place bd update) had no defer path and hard-quarantined, blocking GC until a human cleared it. drift_preserves_preflight_rows counts removed rows per drifted table; zero removals defers, any removal or probe failure still quarantines. diff_is_additive_only kept distinct for the first-committed-table case, with a unit test pinning each predicate's query. Verified both directions per the bead's bar: defer with no removals, quarantine on the identical count/hash signature with removals. Gates: 14/14 shell unit test; examples/bd/dolt green; go vet + go build clean; make check-docs green; pre-push gate 10/10 jobs, RC=0. Self-review round added two fixes (e13bc84): renumbered a duplicated header step 4b -> 4c, and scoped the docs claim that the run quarantines only on removals (row-decrease, table-list drift, and probe failure bypass the DOLT_DIFF proof entirely). NOT included and flagged for a separate bead: the pre-existing HEAD-proven gain+drift arm still short-circuits ahead of the stronger removal proof; behavior there is unchanged from main.
Follow-up filed as gc-k76t1 (P2): the pre-existing HEAD-proven gain+drift arm still short-circuits ahead of the stronger removal proof, so for that case the weaker proxy still decides. Out of scope here — behavior there is unchanged from main — and it needs its own two-directional positive control.
Refinery handoff
gc-i52hj(bug, P1)polecat/gc-i52hjmaine13bc84d; PR opened codex-green.