Skip to content

wasm: read the clock through one helper and refuse age retention without it - #44

Open
EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:fix/wasm-commit-clock
Open

EnRaiha wants to merge 2 commits into
NodeDB-Lab:mainfrom
EnRaiha:fix/wasm-commit-clock

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 27, 2026 •

Copy link
Copy Markdown

Problem

wasm32-unknown-unknown has no std clock: SystemTime::now() panics with
"time not implemented on this platform". Two sites read it directly and both sit
in the commit path, so the first write from an embedded build panicked:

  • WriteTxn::commit builds CommitHistoryMeta, whose timestamp is the history
    entry's unix_seconds (origin/main: src/txn/write/commit.rs:113).
  • The age-based retention policy computes its pruning threshold from the clock
    on every commit (origin/main: src/txn/db/catalog/history.rs:151).

A consumer hits it today: NodeDB-Lite's WASM test
array::create_put_slice_roundtrip fails at its first write for this reason.

Change

The clock is read through one helper. clock::unix_seconds() owns the target
split — js_sys::Date::now() on the OPFS build, std elsewhere (wasm32-wasip1
included) — and answers None on a wasm build without the JS bindings.
clock::clock_available() answers the separate question a policy has to ask.

A build with no clock refuses RetainPolicy::Age instead of neutering it.
threshold = 0.saturating_sub(d) is 0, the prune walk ends at the first row
whose timestamp is >= it, and Age would behave as Unbounded while still
reporting itself age-based. It is now refused at open with
PagedbError::RetainPolicyNeedsClock, before anything is read or written, at
both chokepoints: the top of open_with_mode (every Db::open*), and the top of
open_existing_inner_with_counterpart, which every reopen funnels through —
including the public Db::open_existing_with_counterpart_kek, which does not
pass through open_with_mode. Count, Unbounded and Disabled consult no
clock and are served unchanged.

The page-read retry no longer sleeps where no time driver exists.
pager/core.rs called tokio::time::sleep between AEAD attempts; an embedder
driving these futures through wasm-bindgen-futures has no Tokio runtime, so a
corrupted page panicked instead of reporting PagedbError::Corruption. On
wasm32-unknown-unknown it yields now; every other target keeps the 10 ms
backoff. The retry budget is normalized for every mode whose capabilities deny
observer retries, at both option→config sites, so only Observer reaches that
loop.

One wall-clock regression test keeps its loop and drops its bound on wasm.
The B+ tree allocation-cost test measures elapsed time, which a clockless target
cannot do; the loop still runs on every target, and the assertion runs only
where a clock exists (src/btree/tree/core.rs).

Coverage. wasm-smoke/ (pagedb with opfs) commits on the build an embedder
ships and reads through the retry loop with every page past the two header slots
flipped mid-page. wasm-smoke-no-clock/ (pagedb without opfs) is the build
where no clock exists at all: it asserts that all four opening entry points
refuse Age with the typed error, that the refusal happens before the store
probe, and that Count/Unbounded/Disabled still open and commit there. A
wasm-tests CI job runs both crates under node, so the compile-only wasm job is
no longer the last line of defence, and clock.rs asserts at compile time that a
wasm32-unknown-unknown build without the JS bindings cannot report a clock.

Crash point

Per CONTRIBUTING: the timestamp is metadata on the commit-history entry, and
this change does not move the durability boundary — that remains the A/B header
swap, exactly as before. A clockless build records 0 in that metadata instead
of panicking, and nothing on the recovery path reads it.

Evidence

All on ec148ad, head of fix/wasm-commit-clock, rebased onto 23f2f1f;
rustc/cargo 1.96.1, wasm-bindgen-test-runner 0.2.126 (the version the lockfile
resolves).

Check Command Result
wasm smoke, opfs build CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER=wasm-bindgen-test-runner cargo test -p pagedb-wasm-smoke --target wasm32-unknown-unknown --test commit_smoke 4 passed, exit 0
wasm smoke, no-clock build same runner, -p pagedb-wasm-smoke-no-clock --test refusal 3 passed, exit 0
native unit tests cargo test -p pagedb --lib 535 passed, 0 failed, 1 ignored, exit 0
format / lints cargo fmt --all -- --check; RUSTFLAGS="-A unknown_lints" cargo clippy -p pagedb --lib --all-targets both clean
both wasm configurations compile cargo check -p pagedb --target wasm32-unknown-unknown --lib (no clock) and --features opfs (clock) exit 0 each
the no-clock crate really is clockless cargo tree -p pagedb-wasm-smoke-no-clock --target wasm32-unknown-unknown -e features opfs absent, so clock_available() is false there

Red/green by mutation, each restored afterwards:

Arm Mutation Result
pre-fix retry backoff wasm arm back to tokio::time::sleep a_corrupted_page_is_reported_as_corruption_not_a_panic fails with time not implemented on this platform at sys/time/unsupported.rs:13
the reopened-store chokepoint drop the check_policy_needs_clock call in open_existing_inner_with_counterpart age_retention_is_refused_by_the_rekey_resume_entry_point fails: reached the store before checking the retention policy: Io(Kind(NotFound))
a clock that answers zero unix_seconds() returns Some(0) while clock_available() stays true age_retention_prunes_entries_older_than_its_threshold fails: an entry older than the age threshold must be pruned
the refusal itself guard neutered age_is_refused_when_the_build_has_no_clock fails

Graph checks (c2g, blast_radius): unix_seconds has exactly the two production
callers named above, and the public open_existing_with_counterpart_kek has five
callers, all in src/txn/db/rekey/main.rs tests — it is the one public route that
skips open_with_mode, which is why it carries its own check.

Limits, stated rather than implied

  • The pruned row count is not asserted through the public API: neither Db
    nor DbStats exposes a commit's recorded time or the history index. The
    clocked half is asserted instead by a historical commit disappearing
    (begin_read_at → CommitGone).
  • The clockless arm of the age prune in history.rs stays defensive: Age is
    refused at open on that build, so nothing reaches it. It is a typed error
    rather than a silent zero, not a covered path.
  • The refusal's decision has both branches tested on any target
    (policy_clock_tests); its call sites are exercised end to end only by the
    no-clock crate, which is why that crate exists.

Note on file size

src/errors.rs is 994 lines at main and this change adds 9 more: the typed
RetainPolicyNeedsClock { policy } variant the review asked for, which no
existing variant can carry, since every refusal in this crate is its own variant.
CONTRIBUTING calls 500 lines a smell rather than a limit, and this file has no
#[cfg(test)] block, so it counts whole — it is the only file in this change
that grows past that line. The split belongs in a change of its own and is
tracked in #43.

Follows the review on #41, which had to be closed: its branch lives in this
repository rather than in a fork, so there was no way to push the review response
onto it. The review asked for a typed refusal at open, a corrupted-page smoke
test, and removal of the maintainer-owned CHANGELOG hunk; all three are in this
branch, and the mutation table above is the evidence for each. This branch is
rebased onto current main (23f2f1f).

Fixes #40.

Copilot AI lite review requested due to automatic review settings September 27, 2026 13:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

wasm32-unknown-unknown has no std clock: SystemTime::now() panics with
"time not implemented on this platform". Two sites read it directly and
both sit in the commit path, so the first write from an embedded build
panicked:

- WriteTxn::commit builds CommitHistoryMeta, whose timestamp is the
  history entry's unix_seconds.
- The age-based retention policy computes its pruning threshold from the
  clock on every commit.

Route both through crate::clock::unix_seconds(): js_sys on the OPFS
build, std elsewhere (wasm32-wasip1 included), and 0 for a wasm build
without the JS bindings — the history ordering tolerates that instead of
panicking.

The btree allocation-cost test keeps its loop on every target but runs
the wall-clock bound only where a clock exists.

wasm-smoke/ is a separate crate because the pagedb dev-dependencies
(tokio rt-multi-thread, tempfile) do not compile for wasm32, and Cargo
builds every dev-dependency for a crate's test targets. Its two tests
fail with the panic above before this change and pass after; the new
`wasm-tests` CI job runs them under node, so the compile-only wasm job is
no longer the last line of defence.
…ping

Two defects blocked merge on the wasm clock work. Both are one class: a clock
the build does not have, replaced by a value that looks valid.

**The zero fallback silently disabled age retention.** clock::unix_seconds()
returned 0 for a wasm32-unknown-unknown build without opfs. The ordering
tolerates that; the policy does not. threshold = 0.saturating_sub(d) is 0, the
prune walk ends at the first row whose timestamp is >= it, and Age behaves as
Unbounded while still reporting itself age-based — an unbounded history nobody
asked for, reported by nothing.

unix_seconds() now returns Option<u64>, and clock_available() answers the
separate question a policy has to ask. Age is refused at open with
PagedbError::RetainPolicyNeedsClock. Unbounded, Count and Disabled consult no
clock and are served unchanged.

The refusal is checked at the top of open_with_mode, before any lock, probe or
read, which covers Db::open, open_read_only and open_observer; and it is checked
again at the top of open_existing_inner_with_counterpart, which every reopen
funnels through. The second site is not redundant: review found that
Db::open_existing_with_counterpart_kek is public, bypasses open_with_mode, and
would otherwise have reached the age policy unchecked.

The check is a pure function over (policy, clock_available), so both branches
are tested on a target that has a clock. Disabling the guard fails the refusal
test with "age retention must be refused by name when no clock exists" and
leaves the others passing.

**The page-read retry slept where no time driver exists.** pager/core.rs called
tokio::time::sleep between AEAD attempts. An embedder driving these futures
through wasm-bindgen-futures has no runtime, and one built without enable_time()
panics on first poll — replacing the Corruption the loop exists to report with a
panic. On wasm32-unknown-unknown it now yields; every other target keeps the
backoff.

Reachability: the sleep needs attempt > 0, and the retry budget is zeroed for
every mode whose capabilities report allows_observer_retry false — Standalone,
Follower and ReadOnly. Review found that zeroing happened only in
open_with_mode, so the same normalization now also runs in
open_existing_inner_with_counterpart; without it a Standalone handle opened
through the rekey-resume entry could still have reached the loop. With both
sites normalized, only Observer reaches it.

**Coverage.** The smoke tests now state the invariant rather than the pre-fix
code, and add the case the retry loop exists for: every page past the two header
slots is flipped mid-page, an Observer handle reads through the retry loop, and
the result must be PagedbError::Corruption rather than a panic. That test
against the pre-fix `sleep` arm fails with "time not implemented on this
platform" at sys/time/unsupported.rs:13; against the `yield_now` arm all four
wasm tests pass. clock.rs also asserts the wasm32 configurations at compile
time, in the direction a build can get wrong silently.

The no-clock configuration has a crate of its own now, wasm-smoke-no-clock,
which depends on pagedb without `opfs` and is the only build that can produce
it: wasm-smoke's own dependency on opfs fixes the feature for every target in
that build, so no test inside it can reach the refusal. The new crate asserts at
runtime that every opening entry point refuses Age with
PagedbError::RetainPolicyNeedsClock — including
Db::open_existing_with_counterpart_kek, whose call site this commit adds, and on
an empty store, so an entry point that probed before checking would report
NotFound instead. Deleting that call site makes the test fail with
`got Some(Io(Kind(NotFound)))`. The same crate asserts the refusal is not a
blanket one: Count, Unbounded and Disabled still open and commit on a build
whose clock answers None.

history.rs gains the clocked half of the same invariant, which no wasm test can
reach because it has to wait for the wall clock: with a real clock,
RetainPolicy::Age(Duration::ZERO) must prune an entry older than the current
second, after which begin_read_at on that commit reports CommitGone while the
commit just made stays readable. Against a clock that answers Some(0) it fails
with "an entry older than the age threshold must be pruned" — the silent
Unbounded observed rather than asserted.

What is still not asserted: the pruned row count through the public API, and the
timestamp a clockless commit records. Neither Db nor DbStats exposes a commit's
time, and on the no-clock build the only reader of it, Age, is refused. Both
gaps are stated in the test files and here rather than implied covered.

**Also, from review:** the redundant wasm-bindgen-test dev-dependency on the
pagedb crate is gone (only wasm-smoke uses it, and declares it itself), and the
maintainer-owned CHANGELOG hunk is removed.

535 passed, 0 failed natively; 4 passed on the opfs wasm32 build and 3 on the
no-clock one, both under wasm-bindgen-test-runner.

Preflight is clean except C5: src/errors.rs goes from 994 to 1003 non-test
lines. That file is already around twice the 500-line cap on main and holds no
#[cfg(test)] block, so every line counts and any addition trips the ratchet;
the nine lines are the typed variant this change needs, and no existing variant
names both the policy and the missing clock. Splitting that file is a change of
its own and out of scope here, so the exception is recorded in the PR body
rather than hidden. fmt clean, clippy clean for this change (the crate's
pre-existing clippy::unused_async_trait_impl sites are unknown to clippy 1.96.1
and unrelated).

@farhan-syah farhan-syah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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's yield_now arm.
  • The #41 blockers are fixed: Age is refused at open without a clock, and the retry loop no longer calls tokio::time::sleep on wasm.
  • The unused root dev-dependency and the CHANGELOG.md hunk 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_mode is stale, as is "both option→config sites". Its evidence table was gathered on 23f2f1f, so re-run it on the rebased head.
  • No CI has run on this PR, so the new wasm-tests job has never run on GitHub. Push the rebased branch so the checks run.

Comment on lines +124 to +147
// 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
}
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker. #45 changed the ground this hunk stands on.

  • Stale claim. Db::open_existing_with_counterpart_kek now opens through open_with_mode. The claim in lines 124-129 is false on main, and both additions rest on it.
  • Duplicate retry gate. main already sets observer_retry_count from capabilities.allows_observer_retry() where it builds PagerConfig. 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_clock at the top of open_with_mode. A fresh bootstrap never reaches this function, so the check in open_with_mode is 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.

This branch has not been deployed

No deployments
Sign up for free to 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.

wasm: every commit reads SystemTime::now() for the history entry \u2014 the first write panics

3 participants