Conversation
…itten
`WalWriter::flush_buffer` wrote the batch with a `#[cfg(unix)]` arm and had no
other arm. `cfg(unix)` is false on `wasm32-wasip1` — `target_family` is `wasm` —
so on that target the write was not compiled at all, while everything after it
still ran: `file_offset` advanced by the batch length, the buffer was cleared,
`durability.record_flush()` was called and the flush returned `Ok(())`. `sync()`
therefore reported a durable append for bytes that never reached the file, and
`file_offset()` agreed with it.
The wasm arm seeks to `file_offset` and writes. It covers every wasm32 target,
including `wasm32-unknown-unknown` where libc defines no `pwrite` at all, and
std's wasi positional API is still unstable; the segment is only ever appended
to, so seeking to the end and writing is equivalent to a positional write. A
failed write returns before the shared bookkeeping, so the buffer and the offset
survive for a byte-for-byte retry — the property the `pwrite` arm documents.
`write_error` is now `classify_write_error`, taking the failed write's own
`io::Error` instead of re-reading the thread's errno. The full-device branch is
keyed on `ErrorKind::StorageFull` rather than `libc::ENOSPC`, for two reasons:
`libc` defines no constants at all for `wasm32-unknown-unknown`, which this crate
is compiled for by the existing `wasm32-decoders` CI job, and std maps the
full-device errno to that kind on every target that has a filesystem. So a full
device stays `WalError::OutOfSpace` on wasi, which is the rule the function's own
doc states, rather than degrading to a transient `Io`. A `compile_error!` guard
refuses a target family that is neither unix nor wasm32: that target would
compile no arm at all and reinstate the silent success this change removes.
`fsync_directory` had the same shape of problem: it opened the directory and
called `sync_all`, which wasi preview1 cannot do, so every caller that renames
and then fsyncs a directory failed on that target. It is a documented no-op
there now, with the crash-injection failpoint still evaluated first.
`tests/wasi_append.rs` decides both from the file rather than the return value:
the segment's length must equal the offset the writer reported, and the payload
must be in it. Against the code above it fails with
assertion `left == right` failed: sync() reported success with 73 bytes
written, but the segment holds 0 bytes
and, filtered to the other test,
wasi preview1 has no directory fsync, not a failure:
Io(Os { code: 8, kind: Uncategorized, message: "Bad file descriptor" })
Both pass with the change. Run it with `--nocapture`: a panic aborts the process
on this target, so otherwise the failure arrives as a wasm trap with no message.
A third test arms `wal::wasm_flush_write`, which sits inside the wasi arm, and
asserts that the failed write is reported as a failure, is classified
`OutOfSpace` — the injected error carries the kind a real full device produces,
so the branch is exercised on wasm rather than argued — leaves the segment empty
and does not advance `file_offset`. That injection is the only way to reach
`classify_write_error` from the arm on a runtime whose writes cannot be made to
fail on demand, and it is a chokepoint for the arm: compiling the arm out makes
the injection unfireable and the test fails with "an armed write failpoint must
not be reported as a flush". It needs `--features failpoints`; without the
feature the injection expands to nothing and the assertions would describe a
successful flush, so it is compiled out instead of weakened.
The crate's `tokio` (workspace `features = ["full"]`) and `fluxbench`
dev-dependencies are native-only now. Tokio rejects
fs/io-std/net/process/rt-multi-thread/signal on wasm, Cargo unifies
dev-dependency features across the whole `cargo test` invocation, and neither is
reachable from a wasm test: this crate has no tokio call sites, and `fluxbench`
is used only by benches/wal_throughput.rs, which `cargo test` does not build.
Without that gate no test target in this crate builds for wasm, which is why the
probe above could not run at all before it.
`double_write/raw_io.rs` gains the reason `pwrite_all` reports `Unsupported` on
wasm rather than falling back to a seek-and-write: a fallback would mirror the
slot successfully and the writer would report `DwbProtection::Active` for
protection that `recover_record` cannot read back on that target. That is the
`Direct` path; `Buffered` still reports protection it cannot deliver, which is a
separate defect and needs a read half rather than a write gate.
wasm32-wasip1 under wasmtime 35.0.0: 2 passed by default, 3 with
`--features failpoints`. `cargo check --target wasm32-unknown-unknown -p
nodedb-codec -p nodedb-columnar -p nodedb-strict` (the `wasm32-decoders` job)
still exits 0. Native unchanged at 270 passed, 0 failed. fmt and clippy clean for
this crate on native, on wasm32-wasip1 and on wasm32-unknown-unknown.
Member
|
Thanks for the careful analysis. The defect is real: on wasm32 the flush advanced the offset and reported success without writing. We are closing this as superseded. The Origin WAL writer does not target wasm. The change is in |
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
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.
Problem
On
wasm32-wasip1,WalWriter::flush_bufferwrote nothing and returnedOk(()). The batch is written by a#[cfg(unix)]arm with no other arm, andcfg(unix)is false on this target (target_familyiswasm), so the write wasnot compiled at all while the statements after it still ran:
file_offsetadvanced by the batch length, the buffer was cleared,
record_flush()was calledand the flush returned success.
sync()reported a durable append for bytes thatnever reached the file.
fsync_directoryhad the same shape of problem: it opened the directory andcalled
sync_all, which wasi preview1 cannot do, so every caller that renames andthen fsyncs a directory failed on that target.
Both are pre-existing at
1ff35512b: the wasip1 test job that would haveexercised them is the subject of a separate change, not this one.
Change
writer/flush.rs— the unix arm iscfg(all(unix, not(target_arch = "wasm32")))now, and wasm gets its own arm: seek to
file_offset, then write. It covers everywasm32 target, including
wasm32-unknown-unknownwhere libc defines nopwriteat all, and std's wasi positional API is still unstable; the segment is only
ever appended to, so seeking to the end and writing is equivalent to a
positional write. A failed write returns before the shared bookkeeping, so the buffer
and the offset survive for a byte-for-byte retry — the property the
pwritearmdocuments.
write_errorbecameclassify_write_error, which takes the failedwrite's own
io::Errorinstead of re-reading the thread's errno, and itsfull-device branch is keyed on
ErrorKind::StorageFullrather thanlibc::ENOSPC:libcdefines no constants at all forwasm32-unknown-unknown, which this crate is compiled for by the existingwasm32-decodersjob, and std maps the full-device errno to that kind on everytarget with a filesystem. A full device therefore stays
WalError::OutOfSpaceon wasi instead of degrading to a transient
Io. Acompile_error!guardrefuses a target family that is neither unix nor wasm32, which would otherwise
compile no arm at all.
segment/atomic_io.rs—fsync_directoryis a documented no-op on wasm32.The rename still succeeds; what the target cannot provide is the assurance that
the directory entry survives a host crash. The crash-injection failpoint is still
evaluated first, so
wal::fsync_directorykeeps working there.double_write/raw_io.rs— the reasonpwrite_allreportsUnsupportedonwasm rather than falling back to a seek-and-write: a fallback would mirror the
slot and the writer would report
DwbProtection::Activefor protection thatrecover_recordcannot read back on that target. That is theDirectpath.Bufferedstill reports protection it cannot deliver — that is a separate defectand needs a read half, not a write gate.
Cargo.toml—tokio(workspacefeatures = ["full"]) andfluxbencharenative-only dev-dependencies now. Tokio rejects fs/io-std/net/process/
rt-multi-thread/signal on wasm, Cargo unifies dev-dependency features across the
whole
cargo testinvocation, and neither is reachable from a wasm test: thiscrate has no tokio call sites, and
fluxbenchis used only bybenches/wal_throughput.rs, whichcargo testdoes not build. Without that gateno test target in this crate builds for wasm, which is why the probe below could
not run at all before it.
Evidence
On
51e17220(fix/wal-wasi-write, based on1ff35512b), rustc/cargo 1.96.1,wasmtime 35.0.0. The wasip1 probe is run with
--nocaptureon purpose: a panicaborts the process on this target, so without it a failed assertion arrives as a
wasm trap with no message.
CARGO_TARGET_WASM32_WASIP1_RUNNER="wasmtime --dir=." cargo test -p nodedb-wal --target wasm32-wasip1 --test wasi_append -- --nocapture--features failpointscargo test -p nodedb-walwal_suite), exit 0cargo fmt --all -- --checkcargo clippy -p nodedb-wal --all-targets -- -D warningscargo clippy -p nodedb-wal --target wasm32-wasip1 --lib -- -D warningscargo check -p nodedb-wal --target wasm32-wasip1 --libcargo check --target wasm32-unknown-unknown -p nodedb-codec -p nodedb-columnar -p nodedb-strict(thewasm32-decodersjob this PR triggers)cargo clippy -p nodedb-wal --target wasm32-unknown-unknown --lib -- -D warningsRed, with the two source files reverted to
1ff35512band the probe unchanged:And the arm itself is the chokepoint, not the wrapper above it: with the wasi
arm compiled out (
#[cfg(any())]) and the rest of the change intact, the armedtest fails —
Green, with the change in place:
2 passedby default,3 passedwith--features failpoints.Limits, stated rather than implied
--features failpoints:wal::wasm_flush_writesits inside the arm, the errorit raises goes through the same
classify_write_errorcall the real failuredoes, and the test asserts
OutOfSpace, an unchangedfile_offsetand anempty segment. Compiling the arm out makes the injection unfireable and that
test fails, so it is a chokepoint for the arm rather than for the wrapper above
it. Without the feature the test is compiled out rather than weakened — the
default run is 2 tests, the failpoint run 3.
write_allfailure on wasm. The errno comes fromthe runtime and cannot be injected here, so the arm's own
map_errclosure isexercised only through the same classifier the failpoint path calls, and the
real full device is verified by the mapping std owns — the classifier keys on
ErrorKind::StorageFull, and the probe's injected error carries that kindrather than a raw errno, so what it pins is the classifier's rule, not the
runtime's errno translation.
fsync_directoryon wasi is a weaker guarantee than the name suggests, and thedoc now says so. Callers on that target keep the rename but not the
survives-a-crash assurance.
use_direct_iodefaults to true inthe writer config, and on targets without
O_DIRECTthe open ignores the flagwhile
flush_bufferstill writes the padded aligned slice. The probe usesopen_without_direct_io, so only the unpadded path runs.defect it would have caught impossible. Until that job exists, the probe runs
only when someone invokes it as above, with
--features failpointsfor thefull set.
Fixes #387.