fix(storage): read GSI propagation delay live so runtime changes apply - #245
Merged
Conversation
… immediately The write path decided sync-vs-async GSI propagation from an AtomicU64 cache of the gsi_propagation_delay_ms setting, refreshed by a background worker only every 30 seconds. An out-of-process change via 'extenddb settings set gsi_propagation_delay_ms 0' was therefore invisible for up to 30 seconds: writes kept enqueueing async index updates, and an immediate GSI query returned nothing. This is the exact failure in test_gsi_sync_path_with_zero_delay, which sets the value to 0 and asserts the very next write is synchronously visible. Fix: gsi_default_delay() now reads the settings row from the catalog on each write. SQLite is a local file, so this is an indexed point lookup with negligible cost next to the write it precedes. On a read error the cached value (still refreshed by the poll worker) is the fallback; on a successful read the cache is re-warmed so fallback values stay fresh. A missing row means the default 10ms, matching poll_gsi_delay. Verification: test_gsi_sync_path_with_zero_delay fails on the previous binary (negative control) and passes 20/20 consecutive runs with this change. Full pytest phase 1: 903 passed; the only failures are the known console-test CERTIFICATE_VERIFY_FAILED self-signed-cert artifact of the local runner, identical counts on unmodified main.
LeeroyHannigan
requested review from
amrith,
c33howard,
jcshepherd,
pdf-amzn and
yesyayen
as code owners
August 7, 2026 14:06
The prior commit was formatted with a standalone rustfmt invocation using --edition 2021; the workspace is edition 2024, whose import ordering differs. Imports-only change, verified with cargo fmt --all -- --check on toolchain 1.97 (matching CI).
3 tasks
The previous commit fixed SQLite only. PostgreSQL had the identical defect and this completes the fix, following the precedent of #248 where the same shared defect was corrected in both backends in one change. Proven rather than assumed. Before this commit, a fresh PostgreSQL deployment failed test_gsi_sync_path_with_zero_delay with the same symptom as SQLite did, "assert 0 == 1", reproduced twice: all four write paths resolved the delay from the cached atomic (put_item, update_item, delete_item, transactions), which is seeded once at construction and refreshed only by a 30-second poll worker, so a runtime change to 0 was ignored for up to 30 seconds. After this commit the same test passes 20 consecutive runs. The synchronous path itself already existed in the backend, so only the stale read had to be corrected. Callers keep their `indexes.is_empty()` short-circuit, so a table with no secondary indexes performs no extra query at all. Three improvements from reviewing the SQLite half: * The delay is now read BEFORE `write_lock.lock().await` at all four SQLite call sites. It is a runtime setting rather than an invariant of the write, so it does not need to be read under the lock, and that lock serialises every write in the process: work inside it is the backend's throughput bottleneck. The value was already up to 30 seconds stale before this fix, so nothing depended on reading it there. * The error arm no longer swallows the failure. Both poll workers log through `tracing::debug!`, so a live read that falls back to the cache now logs too; a degraded catalog serves a stale delay loudly rather than silently. * The default is a named constant, `DEFAULT_GSI_PROPAGATION_DELAY_MS`, in each backend crate, used by both the live read and the poll worker. The literal was previously repeated, and the live read added a third copy. One pre-existing failure was found and deliberately left alone: test_gsi_configured_delay_range fails on PostgreSQL, observing about 1010 ms against a 500 ms bound with the delay set to 50 ms. An A/B on a fresh server shows it failing identically with and without this change, so it is not a regression here. It looks like worker scheduling granularity dominating the configured delay, which is a separate investigation. Verification: SQLite and PostgreSQL each pass the zero-delay test 20/20 consecutive; the full TestGsiAsyncPropagation class is 5/5 on SQLite; 16/16 and 14/14 crate unit tests with 0 filtered out; `cargo fmt --all -- --check` exit 0; `cargo clippy -D warnings` exit 0 on both feature sets.
LeeroyHannigan
changed the title
…propagation honours the configured delay Every asynchronous GSI propagation on PostgreSQL took ~1 second regardless of the configured delay. With gsi_propagation_delay_ms=50, observed propagation was consistently ~1010 ms. This was invisible before the live delay-read fix in the previous commit, because the configured value was never actually exercised; making the setting work exposed it. Root cause, found by instrumented timeline rather than code reading. Since PostgreSQL 14, EXTRACT(EPOCH FROM ...) returns `numeric`, and sqlx refuses to decode `numeric` into f64. next_ready_wait's query therefore FAILED on every partition that had a pending row, and `.ok().flatten()` collapsed that error into `None`, indistinguishable from "no rows pending". The worker then slept its full 1 s idle backstop instead of sleeping until the row's `ready_at`. The sequence, from a single-run correlated timeline: a write commits and notifies; all four workers wake within the same millisecond; the row's jittered `ready_at` is ~25-50 ms in the future, so the claim query correctly finds nothing ready; next_ready_wait should then return ~40 ms but instead errors and reports None; the worker parks for the full backstop; the row is claimed ~1 s later. This exonerates the notification design entirely. Three earlier hypotheses (shared notify_waiters losing wakes, notify_one permit routing, registering the waiter before the readiness check) were each implemented and measured at ~1 s, because the wake was never the problem. The fix is the ::float8 cast. The error is also now logged through tracing::error instead of being swallowed, so a decode or connection failure in this query can never silently degrade propagation latency again; the negative-control run logged it five times, once per write. Verification: test_gsi_configured_delay_range passes 20/20 consecutive runs (previously 0/anything, ~1010 ms vs the 500 ms bound); negative control removing only the cast reproduces the exact original failure; zero-delay test still passes; full TestGsiAsyncPropagation class green; fmt exit 0; clippy -D warnings exit 0.
diegotoledano95
added a commit
to diegotoledano95/extenddb
that referenced
this pull request
Aug 10, 2026
Pull in ExtendDB#239 (GSI pagination tiebreaker), ExtendDB#245 (SQLite stale GSI delay), and ExtendDB#246 (Postgres restore race) — the three pre-existing backend bugs the dual-target suite surfaced. Resolve the add/add conflict on restore_active_completeness.rs in favor of this branch version, which carries the reviewer-requested retry-on-scan-error oracle fix and a bounded observer; main branch Postgres restore fix lands via its own source change.
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.
What
Makes both storage backends honour a runtime change of the GSI propagation delay
immediately. The delay getter now reads
gsi_propagation_delay_msfrom the catalogon each write instead of serving a value cached with a 30-second refresh, in
crates/storage-sqlite/src/store.rsandcrates/storage-postgres/src/lib.rs, withthe four write paths in each backend updated to await it.
PostgreSQL callers keep their
indexes.is_empty()short-circuit, so a table with nosecondary indexes performs no extra query at all.
Three further changes, from reviewing the SQLite half:
write_lock.lock().awaitat all four SQLite callsites. It is a runtime setting rather than an invariant of the write, so it does
not need to be read under the lock, and that lock serialises every write in the
process, so work inside it is the backend's throughput bottleneck.
tracing::debug!; now a live read that falls back to the cache does too, so adegraded catalog serves a stale delay loudly rather than silently.
DEFAULT_GSI_PROPAGATION_DELAY_MS, per backendcrate, shared by the live read and the poll worker. The literal was previously
duplicated and the live read had added a third copy.
Why
Setting
gsi_propagation_delay_msto 0 at runtime is supposed to make index updatessynchronously visible. With the cached value, writes kept using the stale non-zero
delay for up to 30 seconds, so the index update stayed async and queries against the
GSI returned 0 items immediately after a write. This is one of the red CI jobs on
#207, where the zero-delay GSI sync test sets the delay to 0 and immediately expects
synchronous visibility.
PostgreSQL had the identical defect. Both backends are fixed here rather than in two
PRs, following #248 where the same shared defect was corrected in both backends in
one change.
Closes #
Testing done
Every claim below was executed, on a fresh deployment per backend and on ports away
from any other local instance.
test_gsi_sync_path_with_zero_delaypasses 20/20 consecutive runs.Full
TestGsiAsyncPropagationclass 5/5.symptom as SQLite,
assert 0 == 1, and now passes 20/20 consecutive runs.keeping the
asyncsignature so it still compiles, reproduces the failure exactly.So the test discriminates and the fix is what makes it pass.
cargo fmt --all -- --checkexit 0.cargo clippy -D warningsexit 0 on both thesqliteandpostgresfeature sets.One pre-existing failure found and deliberately not touched.
test_gsi_configured_delay_rangefails on PostgreSQL, observing about 1010 ms againsta 500 ms bound with the delay set to 50 ms. An A/B on a fresh server shows it failing
identically with and without this change, so it is not a regression here. It looks
like worker scheduling granularity dominating the configured delay, which deserves its
own investigation.
Worth knowing about CI.
run-integration(PostgreSQL) was green on the previousrevision of this PR while PostgreSQL was still broken, so CI did not catch this class
of defect. Whether that is because the harness restarts the server, letting PostgreSQL
pick the setting up at construction, is unconfirmed.
Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -D warningson both feature sets; the repocarries pre-existing
-W clippy::pedanticwarnings and this change adds none)zero-delay test in the Python suite already covers the behaviour on both
backends, and it is what the fix makes pass
documented meaning is unchanged, it is now actually honoured
Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
ADR / RFC: n/a. Internal bug fix, no contract surface changed.
Breaking changes
None.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.