logseq / logseq/db-test

fix-invalid-blocks! silently mutates blocks during "Validate graph" — the error is logged, the repair never is

Open
#1,067 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
No language data
Stars
28
Forks
2
PR merge metrics
No merged PRs in 30d

Description

## Summary

`validate-db` has a sixth mutating pass that is easy to miss: when `fix` is true
(its default), the validation phase itself runs
`validate-and-fix-invalid-blocks!`, which calls `fix-invalid-blocks!` on every
entity the validator flagged, in a loop, until no further fixes are produced.

`fix-invalid-blocks!` retracts attributes and, in some branches, deletes whole
entities. **It logs nothing.** The validation *errors* are printed at `:debug`;
the *repairs* are not printed at all. The final report shows `:errors nil`
precisely because the loop already fixed them — so the user sees a clean bill of
health for a run that modified their data.

This is a companion to logseq/db-test#1068, which covers the same silence in the five
`when fix` passes. This issue is about the sixth site, which that issue does not
reach.

## Version

Logseq DB version, desktop macOS arm64 (build 2287).
`db-worker-node` build-time `2026-07-28T15:15:45Z`, revision `9a11243-dirty`.

## Observed

A block in my graph carried a legacy `:block/pre-block? true` plus a
`:block/properties` value. Validation flagged it:

```
:debug :entity {:block/tx-id …, :block/pre-block? true,
:block/uuid #uuid "…",
:block/properties [[{:db/ident :logseq.property/created-by-ref,
:db/valueType :db.type/ref,
:db/cardinality :db.cardinality/one,
:logseq.property/type :entity} 175]],
:block/title "…"}
:error {:block/pre-block? {"normal-block" ["disallowed key"],
"whiteboard-block" ["disallowed key"]}}
```

Running `validate --fix` on a copy of that graph, the block **survived** (it has
`:block/page` and `:block/parent`) but **both `:block/pre-block?` and
`:block/properties` were retracted from it**.

The command reported:

```
result {:errors nil :invalid-entity-ids () …}
```

Nothing in the log records the retraction. The only way I found it was by
querying DataScript for the entity before and after.

The repair itself may well be correct — `:block/pre-block?` is a file-graph
legacy key that has no place in a DB graph. The problem is that a command
called "Validate" modified stored data and then reported no errors, with no
record of what it changed.

## Why the silence matters more here than elsewhere

`fix-invalid-blocks!` contains branches that go considerably further than
stripping a legacy key:

```clojure
;; whole-entity deletion — every one of these is a :db/retractEntity
(and (:block/title entity) (nil? (:block/page entity))
(nil? (:block/parent entity)) (nil? (:block/name entity)))

(and (:block/tx-id entity) (nil? (:block/title entity)))

(and (= "External URL" (:block/title entity)) (nil? (:block/tags entity)))

(and (:logseq.property/created-by-ref entity)
(not (de/entity? (:logseq.property/created-by-ref entity))))

(vector? (:logseq.property/value entity))

(= :whiteboard-shape (:logseq.property/ls-type entity))

(and (:logseq.property.asset/remote-metadata entity)
(nil? (:logseq.property.asset/type entity)))
```

Two of these are worth singling out. `(= "External URL" (:block/title entity))`
deletes a whole entity on an **exact title-string match** — any untagged block
titled "External URL" is destroyed. And `(and (:block/tx-id entity) (nil?
(:block/title entity)))` covers every title-less entity with a transaction id,
which is a very large class of internal records.

`:db/retractEntity` also removes **inbound reference datoms**. So deleting one
flagged entity silently strips property assertions off other blocks — blocks
that were never flagged, are never named in any log line, and whose
`:block/updated-at` is not bumped. The blast radius of a single validation error
is not the erroring entity.

And a sweep over every datom in the database whose attribute namespace is
`user.class`:

```clojure
(->> (d/datoms db :eavt)
(filter (fn [d] (= (namespace (:a d)) "user.class")))
(mapcat (fn [d]
(let [class-title (:block/title (d/entity db (:a d)))
property (get-property-by-title db class-title)]
(if property
[[:db/retract (:e d) (:a d) (:v d)]
[:db/add (:e d) (:db/ident property) (:v d)]]
[[:db/retract (:e d) (:a d) (:v d)]]))))) ; ← retract, no re-add
```

The lookup is keyed on **title**. When it misses — a renamed property, a
title-cased differently — the datom is retracted with no replacement. That is
unconditional data loss on a title match, and it is likewise unlogged.

I have not observed either of these branches fire. I am reporting them because
the one branch I *did* observe proved the pass mutates data without saying so,
which makes the more destructive branches worth a look before someone hits them.

## The loop

```clojure
(defn- validate-and-fix-invalid-blocks!
[conn]
(loop [{:keys [errors] :as result} (validate-db-result @conn)]
(log-validation-errors! errors)
(if (and (seq errors) (fix-invalid-blocks! conn errors))
(recur (validate-db-result @conn))
result)))
```

Each round re-validates and re-fixes, so effects can compound within one
invocation — and only the *last* round's `errors` reaches the report. A run that
fixed several rounds' worth of problems still reports `:errors nil`.

## Steps to reproduce

1. Have a graph containing a block with a disallowed legacy key — in my case
`:block/pre-block? true`, which appears on blocks carried over from earlier
graph versions.
2. Query DataScript for that entity's datoms and record them.
3. Run **Validate graph** (or `logseq graph validate --fix`).
4. Query the entity again.

**Expected:** either the block is left alone and reported as invalid, or it is
repaired and the repair is reported.

**Actual:** attributes are retracted, the report says `:errors nil`, and no log
line describes the change.

## Suggested fixes

1. **Log every fix `fix-invalid-blocks!` applies**, at the same level as the
errors, and include a summary in the report the user sees.
2. **Report the errors that were fixed**, not just those remaining after the
loop. `:errors nil` after repairs is actively misleading.
3. **Reconsider `:db/retractEntity` in a pass called "fix"** — or at minimum
log which inbound references were severed, since those land on blocks that
were never flagged.
4. **Do not key the `class-as-properties` migration on `:block/title`**, or
preserve the datom when the lookup misses instead of retracting it.

## Related

- logseq/db-test#1068 — the same silence in the five `when fix` passes
- logseq/db-test#1069 — the property-retype state that triggered the loss in logseq/db-test#1068

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with validate-and-fix-invalid-blocks!, validate-db-result, and fix-invalid-blocks!, then trace how log-validation-errors! and the validation command produce the final report. Reproduce the issue by comparing DataScript datoms before and after Validate graph --fix. Done means repairs, including destructive or inbound-reference effects, are recorded and the report does not hide fixes behind a final nil error list.

Written by the indexing model from the issue text.

Assessment

Tech stack
clojure
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.