Skip to content

wasm: read the clock through one helper, and test a commit on wasm32 - #41

Closed
EnRaiha wants to merge 1 commit into
mainfrom
fix/wasm-commit-clock
Closed

EnRaiha wants to merge 1 commit into
mainfrom
fix/wasm-commit-clock

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 23, 2026

Copy link
Copy Markdown

Problem

wasm32-unknown-unknown has no std clock. 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 (src/txn/write/commit.rs:113).
  • The age-based retention policy computes its pruning threshold from the clock on every commit (src/txn/db/catalog/history.rs:151).

Observed under wasm-bindgen-test:

panicked at library/std/src/sys/time/unsupported.rs:35:9:
time not implemented on this platform

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

Change

  • clock::unix_seconds() owns the target split: js_sys::Date::now() on the OPFS build, std everywhere else (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. Two tests: a commit, and a commit under age retention.
  • A wasm-tests CI job runs them under node, so the compile-only wasm job is no longer the last line of defence.

Crash point

Per CONTRIBUTING: the timestamp is metadata on the commit-history entry; the commit point is the A/B header write and is unchanged. A crash before the history write leaves the previous entry, after it the new one. Recovery does not read the clock.

Evidence

Check Result
Red: both smoke tests before the fix 2 failed, time not implemented on this platform
Green: both smoke tests after 2 passed
cargo check native / --target wasm32-unknown-unknown --features opfs / --target wasm32-wasip1 all clean
Graph: unix_seconds callers (c2g, exact) exactly the two sites named above, nothing else

Fixes #40.

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.
Copilot AI lite review requested due to automatic review settings September 23, 2026 02:05

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.

@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. The fix is at the right layer, and your red/green claim holds. Two defects block merge.

Verified

Claim Result
Red before the fix Reproduced. With history.rs and commit.rs reverted to main, both smoke tests fail with time not implemented on this platform.
Green after Reproduced. Both tests pass under wasm-bindgen-test-runner 0.2.126.
Runner matches the lockfile Yes. cargo metadata resolves wasm-bindgen 0.2.126.

Blockers

  1. src/clock.rs:24: the 0 fallback turns RetainPolicy::Age into Unbounded without an error. See the inline note.
  2. Same error class, outside this diff: src/pager/core.rs:1047 calls tokio::time::sleep in the page-read retry loop.
    • Db::open_observer keeps observer_retry_count, which defaults to 3. Db::open_existing_with_counterpart_kek also keeps it, because it skips the mode gate in open_with_mode.
    • So on wasm32, an AEAD error on a page read through either handle reaches the sleep. It does not return Corruption.
    • The sleep needs a Tokio time driver. The smoke runtime is built without enable_time(). An embedder that drives futures through wasm-bindgen-futures has no Tokio runtime at all.
    • I confirmed this by reading the code. I did not run it on wasm.
    • Your PR says it removes the wasm time panics from the commit path. The read path has the same panic. Fix it here, and add a smoke test that reads a corrupted page and expects PagedbError::Corruption.

Should-fix

These are inline. Each one is raised once.

Comment thread src/clock.rs
Comment on lines +24 to +25
pub(crate) fn unix_seconds() -> u64 {
0

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. 0 silently disables age retention.

  • The prune loop stops at the first row where unix_seconds >= threshold (history.rs:192).
  • With now = 0, threshold is 0, so no row is ever pruned.
  • RetainPolicy::Age then acts as Unbounded, and history grows without limit. Nothing reports it.

The doc comment says the ordering tolerates 0. The ordering does, but the policy does not.

Correct end state: a build with no clock refuses RetainPolicy::Age at open with a typed PagedbError. The error names the policy and the missing clock. Unbounded and Count keep working, with timestamp 0.

/// The age-based retention policy computes a threshold from the clock on
/// every commit; pruning must not panic either.
#[wasm_bindgen_test]
fn commit_under_age_retention_prunes_without_a_panic() {

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.

This test passes whether or not pruning runs. It asserts only that nothing panics.

  • It runs with opfs, so the 0 branch in clock.rs never executes on any CI job.
  • Commit twice with RetainPolicy::Age, and assert on the retained history. The recorded timestamp must be non-zero, and old rows must be pruned.
  • Add a no-opfs case that asserts the typed refusal from the clock.rs finding.

//!
//! `wasm32-unknown-unknown` has no std clock. `WriteTxn::commit` reads the
//! clock for the commit-history entry, and the age-based retention policy
//! reads it for the pruning threshold. Before the fix both called

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.

"Before the fix both called SystemTime::now() directly" describes code that no longer exists once this merges. State the invariant the test protects: a commit must not read the std clock on wasm32-unknown-unknown.

Comment thread Cargo.toml
]

[target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dev-dependencies]
wasm-bindgen-test = "0.3"

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.

No code in the pagedb crate uses wasm-bindgen-test. Only wasm-smoke does, and it declares the dependency itself. Remove this block and its Cargo.lock entries.

Comment thread CHANGELOG.md

### Fixed

- **Commits no longer need a wall clock.** `WriteTxn::commit` and the age-based retention threshold read `SystemTime::now()` directly, which panics on `wasm32-unknown-unknown` ("time not implemented on this platform") — the first write from an embedded build failed. Both go through `clock::unix_seconds()` now: `js_sys::Date::now()` on the OPFS build, std elsewhere, and `0` for a wasm build without the JS bindings instead of a panic. `wasm-smoke/` commits once per policy under node so the target stays covered.

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.

Remove this hunk. Changelog entries are maintainer-owned in this repo. The same hunk was removed from the other open PRs. The entry also names clock::unix_seconds(), which is a private module and not public API.

@EnRaiha

EnRaiha commented Sep 27, 2026

Copy link
Copy Markdown
Author

FYI: superseded by #44.

Thanks for the review — both blockers and all four should-fix items are addressed there:

  • clock::unix_seconds() now returns Option<u64>, and RetainPolicy::Age is refused at open with PagedbError::RetainPolicyNeedsClock, checked at the top of open_with_mode and of open_existing_inner_with_counterpart. Your note that Db::open_existing_with_counterpart_kek skips the mode gate was right, and it is why the second call site exists.
  • The page-read retry yields instead of sleeping on wasm32-unknown-unknown, and the retry budget is normalized at both option-to-config sites, so with both in place only Observer reaches that loop.
  • The corrupted-page smoke test is there: every page past the two header slots is flipped mid-page, an Observer read must return PagedbError::Corruption, and against the old sleep arm it fails with "time not implemented on this platform".
  • The no-opfs case is a new crate, wasm-smoke-no-clock, which builds pagedb with no clock and asserts the typed refusal at every opening entry point — plus that Count, Unbounded and Disabled still open and commit there.
  • The CHANGELOG hunk is gone, the test docs state the invariant instead of the pre-fix code, and the redundant wasm-bindgen-test dev-dependency on pagedb is removed.

On why this arrives as a new PR: this PR's head branch lives in NodeDB-Lab/pagedb rather than in a fork and I have no push access to it, so the response could not be pushed here. #44 carries the same first commit rebased onto current main plus the response commit.

One gate is disclosed rather than clean: src/errors.rs grows 981 to 990 lines and trips C5. The typed variant is what the review asked for, the file has no #[cfg(test)] block, so any addition trips the ratchet; the split is tracked in #43. Every other preflight check is clean.

@EnRaiha

EnRaiha commented Sep 27, 2026

Copy link
Copy Markdown
Author

Closing as superseded by #44, which carries this work rebased onto current main plus the review response. Reopen or re-request review there if anything looks off.

@EnRaiha EnRaiha closed this Sep 27, 2026
@farhan-syah
farhan-syah deleted the fix/wasm-commit-clock branch September 27, 2026 21:30
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