Skip to content

fix(sqlite): restore must not report ACTIVE before the data copy completes - #291

Open
LeeroyHannigan wants to merge 1 commit into
mainfrom
fix/sqlite-restore-defer-active
Open

fix(sqlite): restore must not report ACTIVE before the data copy completes#291
LeeroyHannigan wants to merge 1 commit into
mainfrom
fix/sqlite-restore-defer-active

Conversation

@LeeroyHannigan

Copy link
Copy Markdown
Collaborator

What

SQLite's RestoreTableFromBackup could report the restore target as ACTIVE before the data copy finished, so a client that waits-for-ACTIVE can read an empty or partial table.

The restore path created the target through the ordinary create-table flow, which either starts the table ACTIVE outright (creation delay 0) or schedules a CREATING -> ACTIVE transition on the creation-delay timer. The control-plane worker applies that transition without consulting the copy, and a row-by-row copy of a large backup outlasts the timer.

This is the same defect already fixed for Postgres in cf5d497 and covered for MongoDB in f80dd9d. Both of those backends carry a defer_active flag; SQLite was never given it. Nothing exercised the gap until #244 added the run-rust-integration-sqlite job, which is why it surfaced today.

RestoreTableToPointInTime delegates to RestoreTableFromBackup on this backend, so it is covered by the same change.

How

create_table_impl gains a defer_active flag. The restore path passes true, writing the row CREATING with nostatus_transition_at, so the worker's query can never match it:

WHERE table_status ='CREATING'AND status_transition_at IS NOT NULLAND status_transition_at <= ?

The existing explicit ACTIVE update after the copy commits becomes the only flip. TableEngine::create_table passes false and is unchanged. The flag is checked before the delay_secs <= 0.0 branch, so a deferred create holds CREATING even at a zero configured delay.

Five of the six changed files only thread false through existing test callers.

Verification

The existing test is a race detector, and its own module doc warns that passes are weak evidence: on an idle server the copy finishes inside the transition window and the race goes unobserved. That is not a theoretical caveat, it is what happens here. On unfixed code at CI's delay of 0.05s:

result
unfixed, delay 0.05, 6 runs6/6 PASS (race not observed)

So a loop is not a control. Setting control_plane_delay_seconds = 0 makes the defect deterministic rather than timing-dependent, because unfixed code then creates the target ACTIVE outright:

result
unfixed, delay 03/3 FAIL - restored table reported ACTIVE with 0/40000 items present
fixed, delay 03/3 PASS
fixed, delay 0.05 (CI setting)3/3 PASS

Run live against a release build with a real 40,000-item restore.

Gates: cargo fmt --all --check clean; cargo clippy --all-targets -D warnings clean on the sqlite and postgres feature sets; 1015 lib tests passed, 0 failed, 0 filtered on both.

Note for reviewers

This explains the intermittent run-rust-integration-sqlite failures appearing since #244 merged. It is a true positive, not a flake: the test cannot fail when the ordering is correct. Re-running turns it green without fixing anything.

…letes
DescribeTable's contract is that the first ACTIVE observation on a restore
target implies the restored data is fully present. The SQLite restore path
created the target through the normal create-table flow, which either starts
the table ACTIVE outright (creation delay 0) or schedules a CREATING->ACTIVE
transition on the creation-delay timer. The control-plane worker applies that
transition without consulting the copy, and the row-by-row copy of a large
backup outlasts the timer, so a client that waits-for-ACTIVE can observe an
empty or partial table.
This is the same defect fixed for Postgres in cf5d497 and covered for MongoDB
in f80dd9d. Both of those backends already carry the `defer_active` flag;
SQLite was never given it, and nothing exercised the gap until #244 added the
`run-rust-integration-sqlite` job.
Fix: create_table_impl gains a defer_active flag. The restore path passes
true, writing the row CREATING with no scheduled transition, so the worker's
query (`WHERE table_status = 'CREATING' AND status_transition_at IS NOT NULL
AND status_transition_at <= ?`) can never match it; the existing explicit
ACTIVE update at the end of the copy is now the only flip. The public
TableEngine::create_table path passes false and is unchanged. The flag is
checked before the `delay_secs <= 0.0` branch, so a deferred create holds
CREATING even when the configured delay is zero.
RestoreTableToPointInTime delegates to RestoreTableFromBackup on this backend,
so it is covered by the same change.
Verification (live, SQLite, release build, 40k-item restore):
The existing test is a race detector and its own documentation warns that
passes are weak evidence: on an idle server the copy finishes inside the
transition window and the race goes unobserved. Confirmed here, on unfixed
code at the CI delay of 0.05s: 6/6 PASS. A loop is therefore not a control.
Setting control_plane_delay_seconds = 0 makes the defect deterministic
rather than timing-dependent, because unfixed code then creates the target
ACTIVE outright:
unfixed, delay 0 3/3 FAIL "restored table reported ACTIVE with
0/40000 items present"
fixed, delay 0 3/3 PASS
fixed, delay 0.05 3/3 PASS (the CI setting)
fmt clean; clippy --all-targets -D warnings clean on the sqlite and postgres
feature sets; 1015 lib tests passed, 0 failed, 0 filtered on both.
Five of the six changed files only thread `false` through existing test
callers, so the public CreateTable behaviour is unchanged.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@LeeroyHannigan