Repository navigation
wasm: yield the retry backoff without a time driver - #44
Conversation
ae2bc25 to
ec148ad
Compare
farhan-syah
left a comment
farhan-syah
left a comment
There was a problem hiding this comment.
Requesting changes. One blocker, and it comes from #45 merging underneath this branch. Everything else is ready.
Verified on this branch merged into current main (629aee0)
| Check | Result |
|---|---|
cargo clippy --all-targets --all-features -- -D warnings |
Clean |
cargo nextest run --all-features |
845 passed, 10 skipped |
pagedb-wasm-smoke under wasm-bindgen-test-runner 0.2.126 |
4 passed |
pagedb-wasm-smoke-no-clock |
3 passed |
- The corrupted-page smoke test opens with
Db::open_observer, so it reaches the retry loop'syield_nowarm. - The #41 blockers are fixed:
Ageis refused at open without a clock, and the retry loop no longer callstokio::time::sleepon wasm. - The unused root dev-dependency and the
CHANGELOG.mdhunk are gone.
Blocker: rebase onto main and drop the gates #45 made redundant. See the inline comment on src/txn/db/open/existing.rs.
Also needed before merge
- Update the description. Its claim that the counterpart-key open skips
open_with_modeis stale, as is "both option→config sites". Its evidence table was gathered on23f2f1f, so re-run it on the rebased head. - No CI has run on this PR, so the new
wasm-testsjob has never run on GitHub. Push the rebased branch so the checks run.
| // Every reopen path funnels through here — including the public | ||
| // `open_existing_with_counterpart_kek`, which does not pass through | ||
| // `open_with_mode` and therefore cannot rely on the check there. | ||
| // Repeating it is deliberate: this is the chokepoint that makes "no | ||
| // clock means no age retention" true of the API surface, not just of | ||
| // `Db::open`. | ||
| super::modes::check_policy_needs_clock( | ||
| &options.commit_history_retain, | ||
| crate::clock::clock_available(), | ||
| )?; | ||
| // Same derivation as `open_with_mode`, for the same reason: a mode | ||
| // that does not allow observer retries must not inherit a caller's | ||
| // retry budget, and this path is reachable without passing through | ||
| // that normalization. Without it the page-read retry loop — and its | ||
| // platform-specific backoff — would be reachable from a `Standalone` | ||
| // handle, contradicting the reachability the loop documents. | ||
| let options = if mode.open_capabilities().allows_observer_retry() { | ||
| options | ||
| } else { | ||
| OpenOptions { | ||
| observer_retry_count: 0, | ||
| ..options | ||
| } | ||
| }; |
There was a problem hiding this comment.
Blocker. #45 changed the ground this hunk stands on.
- Stale claim.
Db::open_existing_with_counterpart_keknow opens throughopen_with_mode. The claim in lines 124-129 is false onmain, and both additions rest on it. - Duplicate retry gate.
mainalready setsobserver_retry_countfromcapabilities.allows_observer_retry()where it buildsPagerConfig. Merged, this file gates the retry count twice. - Duplicate clock check. Every public open, including the counterpart-key open and a fresh bootstrap, reaches
check_policy_needs_clockat the top ofopen_with_mode. A fresh bootstrap never reaches this function, so the check inopen_with_modeis the one that must stay.
Correct end state: delete lines 124-147 and keep the single check in open_with_mode. age_retention_is_refused_by_the_rekey_resume_entry_point still passes that way, because the check in open_with_mode runs before the store probe.
ec148ad to
3206d51
Compare
farhan-syah
left a comment
farhan-syah
left a comment
There was a problem hiding this comment.
Requesting changes: preserve the portable clock already on current main when rebasing.
The previous review's duplicated gates are removed, and the description reflects the shared open path. The observer retry change passes its WASM smoke test.
Current main is c4bf37e. Commit c5b5e0d already provides WASM timestamps through web-time without requiring opfs. This branch conflicts in src/clock.rs, src/txn/db/catalog/history.rs, and src/txn/write/commit.rs. See the inline finding for the required behavior after rebasing.
Checks on 3206d51:
| Check | Result |
|---|---|
| cargo nextest run --all-features | 845 passed, 10 skipped |
| OPFS WASM smoke | 4 passed |
| No-clock WASM smoke | 3 passed |
| cargo fmt --all -- --check | Passed |
| WASM library check, default and opfs | Both passed |
| cargo clippy -p pagedb --all-targets --all-features -- -D warnings | Four pre-existing errors under Rust 1.99.0, reproduced on base 629aee0 |
Clippy reports deprecated fetch_update at tests/durability/unpublished_commit.rs:60 and clippy::assert_is_empty at src/btree/tree/core.rs:568, src/recovery/provenance/probe.rs:330, and src/vfs/native.rs:302. No lint suppression was added.
These checks cover the PR head, not a resolved merge with current main. GitHub reports CONFLICTING and exposes no status checks for this head.
| /// bindings. Callers either tolerate the absence (commit-history ordering keeps | ||
| /// its total order without timestamps) or refuse the configuration up front | ||
| /// (age retention) — see [`clock_available`]. | ||
| #[cfg(all(target_arch = "wasm32", target_os = "unknown", not(feature = "opfs")))] |
There was a problem hiding this comment.
[P1] Preserve main's WASM clock without requiring opfs
Current main already reads WASM time through web-time, introduced by c5b5e0d. RetainPolicy::Age therefore works without opfs, including MemVfs consumers. This arm returns None for that same build, and the new open gate rejects those consumers with RetainPolicyNeedsClock. Resolving the clock conflict in favor of this implementation removes existing support and breaks main's age-history WASM tests. Rebase onto current main and retain its portable clock. Remove the obsolete clockless refusal and its tests. Keep the observer retry change and runtime coverage.
`tokio::time::sleep` panics on `wasm32-unknown-unknown`, where the executor an embedder drives these futures on has no Tokio time driver. The backoff sits in the retry loop that reports `PagedbError::Corruption`, so the panic replaced a reportable error with a crash. That target yields now. Every other target keeps the 10 ms backoff, so a torn read still gets it. A yield passes no wall time, so against a writer on another thread the attempts are immediate and the loop reports the corruption instead of absorbing the torn read.
The `wasm` job only compiles. A wall-clock read in the txn layer compiles fine
on wasm32 and panics at the first commit ("time not implemented on this
platform"), and the pager's retry backoff had no runtime test on any target.
`wasm-smoke` is a separate crate because the pagedb dev-dependencies do not
compile for wasm32, and Cargo builds every dev-dependency of a crate's test
targets. It opens and commits on the build an embedder ships, commits under age
retention, and reads a store with every page past the two header slots flipped.
Its runtime is built without a time driver on purpose, so that last test reaches
the retry loop's backoff for real.
The threshold is `now - duration`, so a clock that answers 0 makes every threshold 0: no recorded timestamp is older than it, the walk ends at its first row, and `Age` behaves as `Unbounded` while still reporting itself age-based. Nothing in the API reports that absence. `Age(ZERO)` prunes everything older than the current second, so the wait only has to cross one second boundary, and a commit never prunes the row it inserts.
3206d51 to
4de5ea8
Compare
EnRaiha
changed the title
farhan-syah
left a comment
farhan-syah
left a comment
There was a problem hiding this comment.
Approved. The previous blocker is resolved, and this re-review finds no new correctness defects.
| Previous request | Result |
|---|---|
| Rebase onto current main | Resolved: based on c4bf37e, mergeable |
| Preserve main's portable WASM clock | Resolved: src/clock.rs is unchanged from main |
| Remove obsolete clockless refusal and tests | Resolved: clock_available, RetainPolicyNeedsClock, the open gate, and wasm-smoke-no-clock are removed |
| Keep observer retry change and runtime coverage | Resolved: retry yields on wasm32-unknown-unknown, and the corruption smoke test passes |
Checks rerun on 4de5ea8:
| Check | Result |
|---|---|
| cargo nextest run --all-features | 858 passed, 10 skipped |
| Node WASM smoke | 4 passed |
| Node WASM commit-history tests, without opfs | 5 passed |
| cargo fmt --all -- --check | Passed |
| WASM library checks, default and opfs | Both passed |
| cargo clippy -p pagedb --all-targets --all-features -- -A unknown_lints -D warnings | Four pre-existing errors under Rust 1.99.0 |
Clippy reports deprecated fetch_update at tests/durability/unpublished_commit.rs:60 and clippy::assert_is_empty at src/btree/tree/core.rs:568, src/recovery/provenance/probe.rs:330, and src/vfs/native.rs:302. These match the errors reproduced on the base during the previous review. All four source files are unchanged by this PR. The contributor's Rust 1.96.1 lint result does not establish a clean run under Rust 1.99.0.
The immediate WASM retries pass no wall time. The PR states that limitation explicitly. Wiring main's commit_history_wasm suite into CI remains a follow-up, as stated in the description.
GitHub exposes no status checks for this head. This approval records code review and local checks. CI still requires maintainer approval.
Problem
Two runtime defects survive on
main. The second is why the first stayed invisible.The retry backoff sleeps where no time driver exists.
Pager::read_pagebacks off between AEADretries with
tokio::time::sleep(src/pager/core.rs). An embedder driving these futures throughwasm-bindgen-futuresonwasm32-unknown-unknownhas no Tokio time driver, so the sleep panicsinside
std::time::Instant::now. That loop exists to absorb a torn read and then reportPagedbError::Corruption. The panic replaces the report with a crash.No CI job ran pagedb code on wasm. The
wasmjob only compiles, in both configurations. Awall-clock read in the txn layer passes a compile and panics at the first commit, which is how the
commit-path panic in #40 reached a consumer.
Change
The backoff yields on
wasm32-unknown-unknown. Every other target keeps the 10 ms sleep. Ayield passes no wall time, so against a writer on another thread the retries are immediate and the
loop reports the corruption instead of absorbing the torn read.
A
wasm-testsjob runs the smoke crate under node.wasm-smokeopens and commits on the build an embedder ships, commits under age retention, andreads a store with every page past the two header slots flipped, which reaches the retry loop. Its
runtime is built without a time driver on purpose.
wasm-smokeis a separate crate because the pagedb dev-dependencies do not compile for wasm32, andCargo builds every dev-dependency of a crate's test targets.
A lib test pins that age retention prunes.
Agedecides eligibility fromnow - duration, so aclock that answers 0 makes every threshold 0. The walk then ends at its first row, and
Agebehavesas
Unboundedwhile still reporting itself age-based. The integration test accepts either outcome,so nothing caught that. The new test uses
Age(ZERO), waits across one second boundary, andasserts the older commit is gone.
Both commit-path call sites already reach the clock through
clock::unix_seconds(), whichc5b5e0dput onmain. What this branch adds for #40 is the runtime coverage that would havecaught the panic before a consumer did.
Changes since the last review
main(c4bf37e).src/clock.rsis main's file, unmodified. The branch no longeradds
clock_available, theRetainPolicyNeedsClockvariant, its gate at the top ofopen_with_mode, or thewasm-smoke-no-clockcrate.RetainPolicy::Ageworks on every wasmbuild,
MemVfsconsumers included.src/txn/write/commit.rsandsrc/txn/db/catalog/history.rskeep main's clock call. The branchadds one test to
history.rsand changes nothing else there.btree/tree/core.rsclock gating is gone. Main's clock answers on every target, so theallocation bound runs everywhere.
taiki-eaction pins stay at main'sv2.87.18; the branch no longer carries the older pins.mainplus 8 files and 3 commits: the retry split, the node coverage, the prune test.tests/commit_history_wasm.rsinto the same job. Itruns green on this head (5 passed), and it would put the retention policies under the node runner
alongside the smoke crate. It stays out of this review round.
Evidence
All on
4de5ea8, head offix/wasm-commit-clock, rebased ontoc4bf37e. rustc/cargo 1.96.1,wasm-bindgen-test-runner 0.2.126 (the version the lockfile resolves), node 22.
cargo nextest run --all-featurescargo fmt --all -- --checkcargo clippy -p pagedb --all-targets --all-features -- -A unknown_lints -D warningsCARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER=wasm-bindgen-test-runner cargo test -p pagedb-wasm-smoke --target wasm32-unknown-unknown --test commit_smokecargo test -p pagedb --target wasm32-unknown-unknown --test commit_history_wasmcargo check -p pagedb --target wasm32-unknown-unknown --lib, then--features opfs-A unknown_lintsis for this host's clippy 1.96.1, which rejects 13 pre-existingclippy::unused_async_trait_implattributes in files this branch does not modify (src/vfs/*,src/pager/core.rs,src/txn/db/open/capability_tests.rs). The count is 13 onmainand 13 here.Without the flag clippy fails on those, on
mainas well.Red/green by mutation, each arm restored afterwards:
tokio::time::sleepa_corrupted_page_is_reported_as_corruption_not_a_panicfails, panicking instd::time::Instant::nowby way oftokio::time::instant::variant::nowclock::unix_seconds()returns0age_retention_prunes_entries_older_than_its_thresholdfails atsrc/txn/db/catalog/history.rs:499CI: no check has run on this PR yet, and the run needs a maintainer to approve it. The local runs
above stand in until then, and the job itself is what closes the gap after this merge.
Limits
Db::open_observer, the mode whoseretry budget is non-zero. With every page past the two header slots flipped, whichever page the
read path reaches is corrupt.
loop reports the corruption rather than absorbing the torn read. The
opfsbuild could use a JStimer, since
js-sysandwasm-bindgen-futuresare dependencies there. The job does notdistinguish the two builds in that loop.
DbnorDbStatsexposes acommit's recorded time or the history index, so the assertion is that the older commit is gone
(
CommitGone).tests/commit_history_wasm.rsis main's file, run here unchanged.Fixes #40.