fix(opfs): list names documents, not the empty files attach creates
Found by running `storage_contract` against `OpfsFs` for the first time (improvise-vvl3). It failed on its very first assertion — `starts empty` — and the reason is a semantic divergence nobody had had to state: `Storage::list` promises "**Document** names directly inside `dir`", and `OpfsFs` was answering with *entries*. `attach` must create the file to obtain a sync access handle for a document that does not exist yet, and it indexed what it created. So a zero-byte file — a document that does not exist — was listed as one. That is the improvise-9x6 browser failure read from the other end: a fresh origin listed `demo.improv` before anything had been written to it, the seed was skipped as "already there", and `load_from` got an empty buffer. improvise-9x6 worked around it in `vfs::holds_a_document`, which reads each candidate back; the wart it could not reach is the **document picker**, which `vfs::documents` feeds straight from `list` — a browser user would be offered an entry that cannot be opened. So the index now means "documents": - `attach` acquires the handle and indexes nothing. The file it creates is not yet a document. - `write` indexes the name, because a written document is the first thing there is to list. It also un-queues any pending delete of that name — otherwise delete-then-write would be listed and readable and then removed from OPFS by `commit_deletes` at the end of the batch, which is the one ordering gotcha 28's deferral can get wrong. - `refresh` skips zero-byte entries. The size comes off the sync access handle when this instance holds one and only otherwise through `getFile()`, because a file under an exclusive sync access handle is not reliably readable by the promise-based API. `vfs::holds_a_document`'s read-back is left alone: it is now belt and braces rather than the mechanism, and removing it is a separate argument. Evidence, in the harness that comes with the next commit: `an_attached_but_unwritten_entry_is_not_a_document` asserts both halves — that `attach` does not list, and that a *second* `OpfsFs` instance sharing no index does not list it either, so the filter has to be in `refresh` and not only in the omission from `attach`. And `storage_contract_at_""`/`_at_"docs"` now pass, which they did not before: that failure is how this was found. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -29,6 +29,8 @@ wasm-bindgen-futures = "0.4"
|
||||
[target.'cfg(target_arch = "wasm32")'.dependencies.web-sys]
|
||||
version = "0.3"
|
||||
features = [
|
||||
"Blob",
|
||||
"File",
|
||||
"FileSystemDirectoryHandle",
|
||||
"FileSystemFileHandle",
|
||||
"FileSystemGetFileOptions",
|
||||
|
||||
@@ -98,6 +98,17 @@ impl OpfsFs {
|
||||
/// Acquire the sync access handle for one document, creating the file
|
||||
/// if it is not there. Must be awaited before `read` or `write` names
|
||||
/// that document.
|
||||
///
|
||||
/// **It does not index what it created**, and that is the whole of
|
||||
/// improvise-9x6's first browser failure read the other way round. The
|
||||
/// file it creates is zero bytes, and a zero-byte entry is not a
|
||||
/// document — `Storage::list` promises *document* names. Indexing here
|
||||
/// made a fresh origin list `demo.improv` before anything had been
|
||||
/// written to it, so the seed was skipped as "already there" and
|
||||
/// `load_from` got an empty buffer. `vfs::holds_a_document` works around
|
||||
/// that by reading each candidate back; this removes the need.
|
||||
/// [`Self::write`] is what puts a name in the index, because a written
|
||||
/// document is the first thing there is to list.
|
||||
pub async fn attach(&self, path: &Path) -> Result<()> {
|
||||
let name = entry_name(path)?;
|
||||
if let Some(old) = self.handles.borrow_mut().remove(&name) {
|
||||
@@ -115,12 +126,7 @@ impl OpfsFs {
|
||||
.map_err(|e| js_err(&format!("createSyncAccessHandle({name})"), e))?
|
||||
.dyn_into::<FileSystemSyncAccessHandle>()
|
||||
.map_err(|_| anyhow!("createSyncAccessHandle({name}) did not yield a handle"))?;
|
||||
self.handles.borrow_mut().insert(name.clone(), handle);
|
||||
let mut index = self.index.borrow_mut();
|
||||
if !index.contains(&name) {
|
||||
index.push(name);
|
||||
index.sort();
|
||||
}
|
||||
self.handles.borrow_mut().insert(name, handle);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -135,6 +141,12 @@ impl OpfsFs {
|
||||
}
|
||||
|
||||
/// Re-read the directory listing that `list` answers from.
|
||||
///
|
||||
/// Zero-byte entries are skipped: `Storage::list` promises *document*
|
||||
/// names, and [`Self::attach`] creates an empty file for a document that
|
||||
/// does not exist yet. Listing those made a browser offer an entry that
|
||||
/// cannot be opened (improvise-vvl3 found it by running
|
||||
/// `storage_contract` here for the first time).
|
||||
pub async fn refresh(&self) -> Result<()> {
|
||||
let mut names = Vec::new();
|
||||
let iter = js_sys::Reflect::get(&self.root, &"keys".into())
|
||||
@@ -164,7 +176,9 @@ impl OpfsFs {
|
||||
.ok()
|
||||
.and_then(|v| v.as_string())
|
||||
{
|
||||
names.push(name);
|
||||
if self.entry_size(&name).await? > 0.0 {
|
||||
names.push(name);
|
||||
}
|
||||
}
|
||||
}
|
||||
names.sort();
|
||||
@@ -186,6 +200,32 @@ impl OpfsFs {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// How many bytes one entry holds.
|
||||
///
|
||||
/// Read off the sync access handle when this instance has one, and only
|
||||
/// otherwise through `getFile()`: a file under an exclusive sync access
|
||||
/// handle is not reliably readable by the promise-based API, and
|
||||
/// `refresh` is called *after* `attach` in the harness as well as before
|
||||
/// it in the worker.
|
||||
async fn entry_size(&self, name: &str) -> Result<f64> {
|
||||
if let Some(handle) = self.handles.borrow().get(name) {
|
||||
return handle
|
||||
.get_size()
|
||||
.map_err(|e| js_err(&format!("getSize({name})"), e));
|
||||
}
|
||||
let file = JsFuture::from(self.root.get_file_handle(name))
|
||||
.await
|
||||
.map_err(|e| js_err(&format!("getFileHandle({name})"), e))?
|
||||
.dyn_into::<web_sys::FileSystemFileHandle>()
|
||||
.map_err(|_| anyhow!("getFileHandle({name}) did not yield a file handle"))?;
|
||||
Ok(JsFuture::from(file.get_file())
|
||||
.await
|
||||
.map_err(|e| js_err(&format!("getFile({name})"), e))?
|
||||
.dyn_into::<web_sys::File>()
|
||||
.map_err(|_| anyhow!("getFile({name}) did not yield a file"))?
|
||||
.size())
|
||||
}
|
||||
|
||||
fn with_handle<T>(
|
||||
&self,
|
||||
path: &Path,
|
||||
@@ -229,7 +269,22 @@ impl Storage for OpfsFs {
|
||||
.map_err(|e| js_err("write()", e))?;
|
||||
handle.flush().map_err(|e| js_err("flush()", e))?;
|
||||
Ok(())
|
||||
})
|
||||
})?;
|
||||
// A written document is the first thing there is to list. `attach`
|
||||
// deliberately does not do this — see its doc comment.
|
||||
let name = entry_name(path)?;
|
||||
// Writing a name back un-queues its removal. Otherwise a delete
|
||||
// followed by a write of the same document would be listed and
|
||||
// readable, and then removed from OPFS by `commit_deletes` at the
|
||||
// end of the batch — the one ordering gotcha 28's deferral can get
|
||||
// wrong.
|
||||
self.pending_deletes.borrow_mut().retain(|n| *n != name);
|
||||
let mut index = self.index.borrow_mut();
|
||||
if !index.contains(&name) {
|
||||
index.push(name);
|
||||
index.sort();
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Entries in the origin root, presented under `dir`.
|
||||
|
||||
Reference in New Issue
Block a user