Repository navigation
CRW-862: one definition of the state backup, its integrity gate and an operator command - #787
Conversation
…n operator command Fix the three post-merge P1s of PR #735 in the state backup routine: the store WAL is compared by its identity in the two-listing comparison, the store sidecars are recognised by the resolved database path rather than the top-level name, and an ENOENT from the walk Info on the store log is recorded as gone before its copy. Add the integrity_check gate: the copied store is duplicated into a scratch file beside the copy, PRAGMA integrity_check runs on the duplicate, and the manifest records integrityCheck and restoreCandidate (a failure refuses). The copy bytes are never opened. Add the operator command crw install backup-state --to <dir>, which takes the same artifact outside an install, refuses while the relay service runs, and holds the store write gate exclusively during the copy. Red-first tests for each behaviour; docs/runtime-install.md updated.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b44eb34983
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…y duplicate Review fixes for the operator command and the sidecar identity: - sidecarsFor resolves the database path even when store.InPlaceRead refuses it (a log with frames and no usable index after an unclean shutdown), so a linked store log is carried under the restore-compatible name rather than walked as an ordinary file; - listingDiff treats a log that went during a listing walk as absence, not a moved identity, so the same checkpoint race no longer refuses on timing; - the integrity duplicate is streamed with a fixed buffer instead of os.ReadFile, and the free-space preflight charges for it, so a store of any size is checked without holding it in memory and a destination that fits only the backup refuses up front; - the operator command locks the write gate beside the resolved database (holdGate), not the state directory root, and validates every path flag for non-UTF-8.
…t took the backup The standalone backup now records its own route: the manifest and the result say the copy was taken by crw install backup-state outside an install (takenBy: operator), with the words the operator command's checks support, rather than the swap gate's. PRAGMA integrity_check runs through QueryContext, so an interrupted backup stops the check instead of holding the write gate until a large scan finishes.
thisisjun786
added
the
crw-lane
CI on head
|
| Job | Result |
|---|---|
| validate | pass |
| secrets | pass |
| go-product (lint) | pass |
| go-product (test-1) | pass |
| go-product (test-2) | pass |
| go-product (test-3) | pass |
| go-product (test-4) | pass |
| go-product (test-rest) | pass |
| go-product (dist) | pass |
| skill-scripts-node | pass |
| dev-gate | pass |
The earlier run on the same head (37526799002) ran the five test legs in light mode, where they skip their tests; the crw-lane label made the labeled event rerun them in full, and this run executed them.
Review resolution on this head
Devin and the GitHub Codex review each ran once. All twelve threads (six Devin, six Codex) were fixed with a red-first test each and resolved:
- resolved write gate beside the resolved database (
TestTheOperatorCommandLocksTheGateBesideTheResolvedStore); - sidecars resolved even when the WAL index is unavailable (
TestTheSidecarsAreResolvedEvenWhenTheLogCannotBeReadInPlace); - integrity duplicate streamed, not read whole (
TestTheOperatorCommandDuplicatesALargeStoreWithoutHoldingItInMemory); - scratch duplicate charged in the space preflight (
TestTheSpacePreflightCoversTheIntegrityScratchDuplicate); - a log that goes during a listing walk is absence, not a change (
TestTheBackupDoesNotRefuseALogThatGoesDuringTheSecondListing); - non-UTF-8 path flags refused (
TestTheOperatorCommandRefusesANonUTF8State); - the integrity query honors cancellation;
- the standalone artifact records its own route (
TestTheOperatorCommandRecordsItsOwnRoute).
The Codex security review was skipped by its usage limit ("You have reached your Codex usage limits for security reviews"), so it is not awaited.
…d string The two-listing comparison joined its key fields with "|", a byte a file name and a link target may legally hold, so two different entries could join to one string and a retargeted link pass. The keys are now comparable structs used as map keys and compared with ==, and the "gone or changed" entry is reported from the struct Path rather than by cutting a string. Nothing else in listingDiff changes. Red-first tests reproduce the independent review pair with real files: the store log retargeted to a path that collides under the delimiter, and the same for an ordinary symlinked file in the state directory.
Correction generation 2 — head
|
| Job | Result |
|---|---|
| validate | pass |
| secrets | pass |
| go-product (lint) | pass |
| go-product (test-1) | pass |
| go-product (test-2) | pass |
| go-product (test-3) | pass |
| go-product (test-4) | pass |
| go-product (test-rest) | pass |
| go-product (dist) | pass |
| skill-scripts-node | pass |
| dev-gate | pass |
Local: go test -count=1 ./internal/runtime/install/... ok; gofmt, go vet (linux and darwin), make lint, git diff --check clean.
thisisjun786
removed
the
crw-lane
The listing of a store whose relay.sqlite3 resolves to another path probed that file's -wal with os.Lstat and read every error as "there is no log". A probe that failed for any other reason therefore copied the database without its log, passed the integrity gate on the database alone and recorded a restore candidate although committed frames may have been left out. Only fs.ErrNotExist means the log is absent now; any other error refuses the backup, naming the log's path and the error, before anything is reported as made.
The integrity gate copied into its scratch directory every backed-up entry whose path starts with "relay.sqlite3" and flattened it to its base name. A healthy store beside a directory named relay.sqlite3-archive that holds a file named relay.sqlite3 therefore collided in the scratch directory (EEXIST), and the backup was recorded as not a restore candidate. A leftover journal that only starts with the same name was duplicated for nothing. The gate now duplicates exactly relay.sqlite3 and relay.sqlite3-wal, the only files it opens. Red tests on 1bfd207 cover both cases.
1 participant
What this changes
The state backup routine (
internal/runtime/install/zone.go) had three post-merge P1s from the state-backup sidecar work, and it could only be taken inside an install that happens to introduce the additive DAG zone or an ordinary index. This PR fixes the three, gives the routine one definition of the store sidecars with aPRAGMA integrity_checkgate, and adds an operator command that takes the same artifact outside an install.The three P1s
A retargeted WAL link escaped the comparison. The two-listing comparison dropped the store's
-walentry wholesale, so a WAL that is a symlink and is retargeted between the two listings was not noticed, and the copy read bytes from a file the state directory no longer named. The WAL is now compared by its identity (its kind, its link target and the file its bytes come from); only its size and its coming and going are left to the sidecar rules. A log whose link target or resolved source moved refuses with "the store's write-ahead log changed".A linked store's sidecars were recognised by top-level name only.
isStoreShm/isStoreSidecarmatched the stringsrelay.sqlite3-shm/relay.sqlite3-wal. Whenrelay.sqlite3is a link to another file (inside the state directory, e.g.data/real.sqlite3, or outside it), that file's-shmand-walwere walked as ordinary files: the-shmvanishing refused, and the-waltook the ordinary-file rule. The sidecars are now identified by the pathstore.InPlaceReadresolves the database to: the-shmis never listed, and the-walis listed under the restore-compatible namerelay.sqlite3-waland follows the sidecar rules.A WAL that vanished during the walk still stopped the backup.
listStatereturned anyd.Info()error, so arelay.sqlite3-walremoved betweenReadDirandInfofailed the whole backup. AnENOENTfromInfoon the store's-wal/-shmis now recorded as gone before its copy and passed; every other file'sENOENT, and every other error, still refuses.One definition, one integrity gate
The store's sidecars are now computed once (
sidecarsFor) as resolved absolute paths, and every place that needs to know "is this the store's sidecar" (the walk, the copy loop, the second listing's frame check, the verification digest loop and the comparison) uses that identity rather than a bare name.Before the manifest is written, the copy passes an integrity gate. A scratch duplicate of the copied store (its
relay.sqlite3and, when copied, its-wal) is written beside the copy, opened with SQLite and askedPRAGMA integrity_check. The duplicate is what SQLite touches, never the copy, so the backup's bytes are unchanged by the check; the duplicate is removed afterwards. The manifest recordsintegrityCheck(SQLite's answer) andrestoreCandidate(true only when the check passed), and a copy that fails the check is not a restore candidate and the command refuses. The install route runs the samebackupState, so it gets the same gate. The manifest fields are additive: no existing field changes.The operator command
crw install backup-state --to <dir>takes the same stopped byte copy outside any install. It calls the samebackupStateroutine, so the copy, its manifest and its integrity fields are the same, and the destination rules (refuseDestination,enoughRoom) are the same. It is routed before the generic install options, likefeatures,configandmigrate-state: it takes its own--toflag and prints its own JSON report, and the frozencrw installusage line still names only the commands it named before.It refuses while the relay service runs, reading the relay the same way the swap gate does (the selected relay's own
service status); a reading that could not be taken also refuses. While it copies it holds the store's write gate (<state>/write-gate.lock) exclusively, so no relay writer reaches the store under the copy; a writer that already holds it refuses the command rather than waiting. It takes no other lock, and it takes no backup while the service is unknown or running.Out of scope
The online
sqlite3_backupsnapshot and its schedule, restore, reconciliation and relay store code are not touched. The online snapshot schedule is a separate decision and is not built here; the doc says so.Expected behavior and acceptance criteria
zone.gosidecarsFor,listingDiff(WAL identity), the walk's ENOENT branch,sqliteIntegrityCheck,backup_cmd.gogo test -count=1 ./internal/runtime/install/...greendocs/runtime-install.mdupdatedCommands run, with results
Red before, green after
The new tests were run against the baseline commit (
749d3a0,origin/devat dispatch) in a throwaway worktree with the test files and the test seams only. They fail there and pass on this head:Files edited outside the declared regions
None. The declared regions are
internal/runtime/install/zone.go,zone_sidecar_test.go,export_test.go,cli.go, the newbackup_cmd.goandbackup_cmd_test.go, anddocs/runtime-install.md; every edited file is one of those.Contract and golden impact
No contract file, refusal reason, output field outside the backup's own manifest, CLI surface, golden or shipped SQL changes. The two new manifest fields (
integrityCheck,restoreCandidate) are additive, and the operator command's result is a new JSON report of its own. No zone statement was appended: the store's schema is untouched.D-19 contract line-citation drift
docs/port/decisions.mdsection 83 citesinternal/runtime/install/zone.go:110-115(backupState),:374-375(the sidecar naming),:473-500(listingDiff),cli.go:112-114(--backup-state-to) andinternal/runtime/swapgate/swapgate.go:55-59. At this head those line numbers have moved (this PR itself moves some of them, and siblings moved others):backupStateis atzone.go:182, the sidecar naming atzone.go:103-137,listingDiffatzone.go:616, and the--backup-state-toregistration atcli.go:115-120. The citations name the same functions and flags; only the offsets moved. The decision document is not edited here.Base currency
git merge-tree HEAD origin/devreports no conflict. This head was built and vetted on a temporary merge of itself with the latestorigin/dev(ffee50d5tree):go build ./...andgo vetofinstall,swapgate,storeanddoctorare clean there.devhas moved since the baseline (as expected for this run); the coordinator makes the PR current in its merge lane.