`TableStore::remove_head` discards all errors, not just the `NotFound` its comment intends to tolerate

[petejm]

Disclosure: investigated and written with the help of an AI agent (Claude), directed and checked by @petejm.

Description

TableStore::remove_head intends to tolerate a missing file, per its comment, but the implementation discards every error:

jj/lib/src/stacked_table.rs

Lines 476 to 482 in 5f56e2c

	fn remove_head(&self, table: &Arc<ReadonlyTable>) {
	// It's fine if the old head was not found. It probably means
	// that we're on a distributed file system where the locking
	// doesn't work. We'll probably end up with two current
	// heads. We'll detect that next time we load the table.
	std::fs::remove_file(self.dir.join("heads").join(&table.name)).ok();
	}

fn remove_head(&self, table: &Arc<ReadonlyTable>) {
    // It's fine if the old head was not found. It probably means
    // that we're on a distributed file system where the locking
    // doesn't work. We'll probably end up with two current
    // heads. We'll detect that next time we load the table.
    std::fs::remove_file(self.dir.join("heads").join(&table.name)).ok();
}

The same comment appears nearly verbatim (one word differs: "view" for "table") on SimpleOpHeadsStore::remove_op_head, which implements the stated intent correctly: it matches ErrorKind::NotFound and propagates everything else.

jj/lib/src/simple_op_heads_store.rs

Lines 84 to 99 in 5f56e2c

	fn remove_op_head(&self, id: &OperationId) -> Result<(), PathError> {
	let path = self.dir.join(id.hex());
	std::fs::remove_file(&path)
	.or_else(|err| {
	if err.kind() == io::ErrorKind::NotFound {
	// It's fine if the old head was not found. It probably means
	// that we're on a distributed file system where the locking
	// doesn't work. We'll probably end up with two current
	// heads. We'll detect that next time we load the view.
	Ok(())
	} else {
	Err(err)
	}
	})
	.context(path)
	}

Same intent, two implementations, one correct.

Why it matters

A swallowed removal failure leaves a stale head marker next to the current one. The next get_head() then sees more than one head and takes the multi-head resolve path. That is exactly the precondition of #9649: on current main, a parent and child marker pair with parent-first read_dir order deletes every head marker (about 49% of repos on ext4; measured, details in my comment on #9648). So the swallow converts a transient filesystem error into a coin flip on total head loss at the next plain read.

With the #9648 guard applied the data loss goes away, but the state still cannot be reported or cleaned up: the cleanup path for the stale marker is remove_head again, with the same swallow, so while the underlying condition persists the store silently re-resolves heads on every read.

The failure modes worth caring about are the asymmetric ones, where creating the new marker succeeds and deleting the old one fails:

• Windows: sharing violation. Another process holding the head file open without FILE_SHARE_DELETE (AV scanner, indexer, backup) makes the delete fail while creating a new file succeeds. No race needed. Note this maps to io::ErrorKind::Uncategorized, so it could not even be special-cased by kind downstream today.
• Network filesystems: ESTALE (ErrorKind::StaleNetworkFileHandle) on the existing entry, in the same environment the comment itself is worried about.

This is a code-review finding (made while reviewing #9648); I have not included a runtime repro.

Suggested fix

Mirror remove_op_head: match ErrorKind::NotFound, propagate the rest. Both call sites (save_table, and the multi-head resolve in get_head_locked) are in functions that already return TableStoreResult, so the plumbing is short. If propagating is judged too strict, a tracing::warn! on the discarded error would at least make the state observable.

Happy to send a PR.

Expected Behavior

A failed head-marker removal (other than the file already being gone) surfaces as an error or warning, as SimpleOpHeadsStore::remove_op_head already does in the same situation.

Actual Behavior

The error is discarded. The stale marker persists silently, and the store enters the multi-head resolve path on subsequent reads (the #9649 precondition).

Specifications

• Platform: any; the triggers above are platform-specific, the swallow is not
• Version: main @ 5f56e2c

[martinvonz]

FYI, there's no need to file a bug report in order to send a PR.

Also note that we're introducing a stricter AI policy in #10057 (largely thanks to you, I think :)), which I think would have you rewrite reports like this (and comments like this) in your own words instead of just pasting LLM output.