bug: commit_builder duplicate commit ID check skipped for non-default index backends

[scalecode-solutions]

Summary

In lib/src/commit_builder.rs, the duplicate commit ID guard in CommitBuilder::write is only enforced when backed by the default index. The FIXME comment notes that Google's backend has_id() always returns true, causing the guard to be skipped entirely for that backend.

Location

lib/src/commit_builder.rs, function write, line 397:

pub async fn write(mut self, mut_repo: &mut MutableRepo) -> BackendResult<Commit> {
    // ...
    let commit = write_to_store(&self.store, self.commit, &self.sign_settings).await?;
    // FIXME: Google's index.has_id() always returns true.
    if mut_repo.is_backed_by_default_index()
        && mut_repo
            .index()
            .has_id(commit.id())
            .map_err(|err| BackendError::Other(err.into()))?
    {
        // Recording existing commit as new would create cycle in
        // predecessors/parent mappings within the current transaction, and
        // in predecessors graph globally.
        return Err(BackendError::Other(
            format!("Newly-created commit {id} already exists", id = commit.id()).into(),
        ));
    }
    mut_repo.add_head(&commit)?;
    mut_repo.set_predecessors(commit.id().clone(), self.predecessors);

Impact

The guard exists to prevent commit ID collisions from creating cycles in the predecessors/parent mappings — both within the current transaction and globally. For backends where has_id() always returns true (noted as Google's backend), the is_backed_by_default_index() short-circuit means the check never runs, and a colliding commit ID would be silently added as a head, potentially corrupting the graph.

Note

This may be intentional if Google's backend handles deduplication at a lower level, but the FIXME comment suggests it is an unresolved issue rather than a deliberate design decision.

---

Found via static analysis with mvtk

[PhilipMetzger]

This was done explicitly to support Google's use-case (cc @martinvonz) so I'd argue that this is not a bug but something which requires another solution long-term.