diff options
Diffstat (limited to 'working/org-element-cache-persist-bug')
17 files changed, 1323 insertions, 0 deletions
diff --git a/working/org-element-cache-persist-bug/README.org b/working/org-element-cache-persist-bug/README.org new file mode 100644 index 00000000..2ad8b1bf --- /dev/null +++ b/working/org-element-cache-persist-bug/README.org @@ -0,0 +1,94 @@ +#+TITLE: org-element persistent-cache corruption — reproduction evidence +#+AUTHOR: Craig Jennings +#+DATE: 2026-07-28 + +* What this is + +Evidence for the org-element cache corruption I chased on 2026-07-28, kept +because the diagnosis outlived the fix. I shipped a fix as =d2103877=, put it +through four hostile reviews, and reverted it on their findings. These scripts +are what the reviews were arguing about. + +Run any of them with: + +: emacs --batch -q -l <script> + +Several of the failing ones never terminate once corruption sets in, because +=org-element--cache-self-verify= floods warnings (one run reached 26 million +lines). Use =timeout=. + +* The actual bug + +=org-element--cache-persist-before-write= (org-element.el:7694) resolves +=(get-file-buffer ...)= and destructively nils =:buffer= on the *live* cache. +The next edit to that buffer then signals =wrong-type-argument stringp nil= +from =org-element--cache-after-change=. + +It needs no second buffer and no dirvish. That is the finding that killed the +fix: I had diagnosed a *trigger* and written the fix against it. + +Which buffer gets corrupted is decided by =buffer-list= order, since +=get-file-buffer= returns the first buffer visiting the file. That is why the +symptom is intermittent, and why any future fix needs a test that controls the +order rather than accepting whatever the harness happens to produce. + +* The scripts + +| File | What it shows | +|----------------------------+-----------------------------------------------------------| +| =hostile-d-nopreview.el= | The root cause. One org buffer, no preview, no dirvish. | +| | =org-persist-write-all-buffer= alone strips =:buffer= | +| | from 6134 elements and the next insert signals. | +|----------------------------+-----------------------------------------------------------| +| =hostile-d-order.el= | Corruption depends on =buffer-list= order. Bury the real | +| | buffer and =get-file-buffer= returns the other one, and | +| | the run comes back clean. | +|----------------------------+-----------------------------------------------------------| +| =hostile-d-faithful.el= | A faithful replay of dirvish's preview, with the real | +| | =dirvish-preview-environment= bindings and buffer naming. | +| | Reproduces. My own replay below was looser. | +|----------------------------+-----------------------------------------------------------| +| =hostile-c-repro.el= | The regression the reverted fix introduced: with | +| | =org-element-cache-persistent= nil in a preview buffer, | +| | =org-element-cache-reset= unregisters the *real* file's | +| | persisted cache and removes it from disk. | +|----------------------------+-----------------------------------------------------------| +| =hostile-a-errorbranch.el= | dirvish.el:671-680 kills the preview buffer on a signal | +| | without ever running =dirvish-preview-setup-hook=, so a | +| | hook-based fix is skipped there. Needs =so-long= enabled | +| | to reach, which it is not here today. | +|----------------------------+-----------------------------------------------------------| +| =hostile-e-ffnoselect.el= | =dirvish--find-file-temporarily= returns the user's | +| | existing buffer, which dirvish later kills. Separate | +| | pre-existing bug, tracked as its own task. | +|----------------------------+-----------------------------------------------------------| +| =repro-preview-cache.el= | My original replay. Kept as the record of what I ran, | +| | not as good evidence: its control differs from it by | +| | three variables, so on its own it cannot attribute cause. | +|----------------------------+-----------------------------------------------------------| +| =control-no-preview.el= | That flawed control. | +|----------------------------+-----------------------------------------------------------| +| =variant-no-kill.el= | Preview created but not killed: clean. Supported the | +| | "the kill is required" framing, which turned out to be | +| | backwards. The persist *write* is causal; the kill only | +| | triggers it. | +|----------------------------+-----------------------------------------------------------| +| =variant-no-persist.el= | Same sequence with persistence off: clean. | +|----------------------------+-----------------------------------------------------------| +| =verify-shipped-fix.el= | The reverted fix suppressing the corruption on the | +| | dirvish path. It did work there. It was aimed one layer | +| | too low and carried a regression. | +|----------------------------+-----------------------------------------------------------| +| =d2103877-reverted.patch= | The reverted commit, kept so the approach is recoverable. | +|----------------------------+-----------------------------------------------------------| + +* What does not reproduce it + +Turning =org-element--cache-self-verify= on does *not* manufacture the error. +It reproduces at org's defaults; self-verify only makes it loud. I had worried +the instrumentation was creating the signal, and it is not. + +* Filed as + +See =todo.org=: the org-element persist task, and the separate dirvish +=find-file-noselect= buffer-kill task. diff --git a/working/org-element-cache-persist-bug/attempt2-tests-blind.el b/working/org-element-cache-persist-bug/attempt2-tests-blind.el new file mode 100644 index 00000000..4e9c4f37 --- /dev/null +++ b/working/org-element-cache-persist-bug/attempt2-tests-blind.el @@ -0,0 +1,190 @@ +;;; test-dirvish-config--preview-org-cache-env.el --- preview org-cache opt-out -*- lexical-binding: t; -*- + +;;; Commentary: +;; Dirvish's fallback preview builds a throwaway buffer, points `buffer-file-name' +;; at the real file (dirvish.el:667) and runs `set-auto-mode' (dirvish.el:671). +;; Previewing an .org file therefore makes a real org-mode buffer that registers +;; with org-persist and picks up a buffer-local `org-persist-write-all-buffer' on +;; `kill-buffer-hook'. When dirvish kills that buffer, the write fires, +;; `org-element--cache-persist-before-write' resolves `get-file-buffer' to the +;; user's REAL buffer, and nils `:buffer' across its live element cache. The next +;; access to a cached headline then runs +;; `org-element--headline-parse-title', whose first form is +;; `(with-current-buffer (org-element-property :buffer headline))' -- and +;; `set-buffer' on nil signals `wrong-type-argument stringp nil'. +;; +;; Dirvish let-binds `dirvish-preview-environment' around `set-auto-mode', so +;; adding `(org-element-use-cache . nil)' there stops the preview buffer ever +;; registering. Nothing is written mid-session and the real buffer is untouched. +;; +;; Coverage note, stated plainly: the wiring tests below assert the alist entry, +;; and the behavioral test drives the corruption sequence directly rather than +;; through dirvish. Neither runs dirvish's own preview machinery, so a change in +;; how dirvish applies the environment would not be caught here. That gap is +;; covered by a live check in the running daemon. + +;;; Code: + +(require 'ert) +(require 'cl-lib) +(require 'package) +(require 'org) +(require 'org-element) +(require 'org-persist) + +(setq package-user-dir (expand-file-name "elpa" user-emacs-directory)) +(package-initialize) +(add-to-list 'load-path (expand-file-name "modules" user-emacs-directory)) +(add-to-list 'load-path (expand-file-name "elpa/dirvish-2.3.0/extensions" + user-emacs-directory)) +(require 'user-constants) +(require 'keybindings) +(require 'dirvish-config) + +;;; ----------------------------- the wiring ----------------------------------- + +(ert-deftest test-dirvish-preview-env-disables-org-cache () + "Normal: `dirvish-preview-environment' turns the element cache off. +This is the entry dirvish let-binds around `set-auto-mode', so it is what keeps +an org preview buffer from registering with org-persist." + (should (member '(org-element-use-cache . nil) dirvish-preview-environment))) + +(ert-deftest test-dirvish-preview-env-keeps-upstream-entries () + "Boundary: the upstream entries survive alongside ours. +Replacing the alist rather than extending it would silently drop dirvish's own +bindings, and preview would start honouring dir-locals and printing messages." + (dolist (pair '((inhibit-message . t) + (non-essential . t) + (enable-dir-local-variables . nil) + (enable-local-variables . :safe))) + (should (member pair dirvish-preview-environment)))) + +(ert-deftest test-dirvish-preview-env-entry-is-not-duplicated () + "Boundary: the entry appears exactly once. +The module can be re-loaded into a running daemon, so the wiring has to be +idempotent rather than appending on every load." + (should (= 1 (cl-count 'org-element-use-cache dirvish-preview-environment + :key #'car)))) + +;;; --------------------------- the behavior ----------------------------------- + +(defun cj--test-preview-buffer (file env) + "Build a dirvish-style preview buffer for FILE with ENV bound, and return it. +Mirrors `dirvish--preview-file-maybe-truncate': real `buffer-file-name', then +`set-auto-mode' under the preview environment." + (with-current-buffer (get-buffer-create "*cj-test-preview*") + (with-silent-modifications + (insert-file-contents file nil 0 1048576) + (setq buffer-file-name file) + (goto-char (point-min)) + (rename-buffer (format "PREVIEW :: 1 :: %s" (file-name-nondirectory file)))) + ;; Dynamic `eval', matching dirvish.el:672 -- NOT lexical. Under lexical + ;; binding a non-special symbol would bind lexically and never reach + ;; `set-auto-mode', so the replay would silently no-op and the test would + ;; pass for the wrong reason. Same trap as the json-object-type gotcha. + (eval `(let ,(mapcar (lambda (e) `(,(car e) ',(cdr e))) env) + (setq-local delay-mode-hooks t) + (set-auto-mode) + (font-lock-mode 1))) + (current-buffer))) + +(defun cj--test-persisted-p (file) + "Return non-nil when FILE still has a registered org-element cache collection." + (and (org-persist--find-index + (org-persist--normalize-associated (list :file file))) + t)) + +(defun cj--test-cached-buffers (buf) + "Return the distinct `:buffer' values cached in BUF's element cache." + (with-current-buffer buf + (let (seen) + (avl-tree-mapc (lambda (el) (push (org-element-property :buffer el) seen)) + org-element--cache) + (delete-dups seen)))) + +(defmacro cj--with-org-fixture (var &rest body) + "Bind VAR to a live buffer visiting a populated org fixture, then run BODY." + (declare (indent 1)) + `(let* ((dir (make-temp-file "cj-preview-" t)) + (org-persist-directory (expand-file-name "persist" dir)) + (file (expand-file-name "notes.org" dir)) + (org-element-use-cache t) + (org-element-cache-persistent t) + (org-log-done nil)) + (with-temp-file file + (dotimes (i 30) (insert (format "* TODO task %d :t%d:\nbody %d\n" i i i)))) + (let ((,var (find-file-noselect file))) + (unwind-protect + (progn + (with-current-buffer ,var + (org-element-cache-reset) + (org-element-cache-map #'ignore :granularity 'headline) + ;; Closing a task is what actually lands parsed headline elements + ;; in the cache carrying a `:buffer'. Without it the cache stays + ;; empty and every assertion below passes vacuously against nil. + (goto-char (point-min)) + (re-search-forward "^\\* TODO task 0 ") + (beginning-of-line) + (org-todo "DONE")) + (should (cj--test-cached-buffers ,var)) + ,@body) + (when (buffer-live-p ,var) + (with-current-buffer ,var (set-buffer-modified-p nil)) + (kill-buffer ,var)) + (delete-directory dir t))))) + +(ert-deftest test-dirvish-preview-env-protects-the-real-buffer () + "Normal: previewing an already-open org file leaves its cache intact. +The whole point of the entry -- with it, killing the preview must not strip +`:buffer' from the real buffer's cached elements." + (cj--with-org-fixture real + (should (member real (cj--test-cached-buffers real))) + (let ((preview (cj--test-preview-buffer (buffer-file-name real) + dirvish-preview-environment))) + (let (kill-buffer-query-functions) (kill-buffer preview))) + (should (member real (cj--test-cached-buffers real))) + (should-not (member nil (cj--test-cached-buffers real))))) + +(ert-deftest test-dirvish-preview-env-real-buffer-still-parses () + "Normal: the real buffer still parses after the preview is killed. +Asserting on `:buffer' alone would pass against a cache that is broken in some +other way, so this drives the access path that actually signalled." + (cj--with-org-fixture real + (let ((preview (cj--test-preview-buffer (buffer-file-name real) + dirvish-preview-environment))) + (let (kill-buffer-query-functions) (kill-buffer preview))) + (with-current-buffer real + (should (equal "OK" + (condition-case err + (progn (org-element-cache-map + (lambda (el) (org-element-property :raw-value el)) + :granularity 'headline) + "OK") + (error (format "%S" err)))))))) + +(ert-deftest test-dirvish-preview-env-without-the-entry-corrupts () + "Error: the same sequence WITHOUT the entry does corrupt the real buffer. +This is the control. Without it the two tests above could pass because the +corruption never happens in this harness at all, rather than because the entry +prevents it." + (cj--with-org-fixture real + (let* ((bare (assq-delete-all 'org-element-use-cache + (copy-alist dirvish-preview-environment))) + (preview (cj--test-preview-buffer (buffer-file-name real) bare))) + (let (kill-buffer-query-functions) (kill-buffer preview))) + (should (member nil (cj--test-cached-buffers real))))) + +(ert-deftest test-dirvish-preview-env-spares-an-unopened-file () + "Boundary: previewing an org file that is NOT open harms nothing. +`org-element--cache-persist-before-write' only acts when `get-file-buffer' +finds a live buffer, so this path was never at risk and must stay cheap." + (cj--with-org-fixture real + (let* ((other (expand-file-name "other.org" (file-name-directory + (buffer-file-name real))))) + (with-temp-file other (insert "* TODO unrelated\n")) + (let ((preview (cj--test-preview-buffer other dirvish-preview-environment))) + (let (kill-buffer-query-functions) (kill-buffer preview)))) + (should-not (member nil (cj--test-cached-buffers real))))) + +(provide 'test-dirvish-config--preview-org-cache-env) +;;; test-dirvish-config--preview-org-cache-env.el ends here diff --git a/working/org-element-cache-persist-bug/control-no-preview.el b/working/org-element-cache-persist-bug/control-no-preview.el new file mode 100644 index 00000000..5f5ad3f2 --- /dev/null +++ b/working/org-element-cache-persist-bug/control-no-preview.el @@ -0,0 +1,62 @@ +;;; repro-preview-cache.el --- Does dirvish's text preview disturb an org buffer's element cache? -*- lexical-binding: t -*- + +;; Replays `dirvish--preview-file-maybe-truncate' (dirvish.el:653-684) against an +;; org file that is ALSO open for real, then kills the preview buffer the way +;; dirvish does (`dirvish--kill-buffer', dirvish.el:416). The kill is the step +;; that matters: killing an org-mode buffer with a `buffer-file-name' is when +;; org-persist writes that file's cache. + +(require 'org) +(require 'org-element) +(require 'org-persist) + +(setq org-element-use-cache t + org-element-cache-persistent t + org-element--cache-self-verify t + org-element--cache-self-verify-frequency 1.0) + +(defun repro--build-cache (buf) + "Actually populate the element cache of BUF by walking it." + (with-current-buffer buf + (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (and org-element--cache (avl-tree-size org-element--cache)))) + +(defun repro--warnings () + (when (get-buffer "*Warnings*") + (with-current-buffer "*Warnings*" (string-trim (buffer-string))))) + +(let* ((src (expand-file-name "todo.org" default-directory)) + (file (make-temp-file "repro-" nil ".org")) + (threshold 1048576)) + (copy-file src file t) + (let ((size (nth 7 (file-attributes file)))) + (message "== fixture %s (%d bytes, %s threshold) ==" + (file-name-nondirectory file) size + (if (>= size threshold) "over" "under"))) + + (let ((real (find-file-noselect file))) + (message "real buffer cache after full walk: %S" (repro--build-cache real)) + + (message "CONTROL: preview step skipped entirely") + + ;; --- dirvish kills the preview buffer when the session ends --- + (let ((kill-buffer-query-functions nil) + (preview (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (when (get-buffer preview) (kill-buffer preview)) + (message "preview buffer %S killed" preview)) + + ;; --- is the real buffer still coherent? --- + (with-current-buffer real + ;; Edit it the way Craig would after stepping back from dirvish. + (goto-char (point-max)) + (let ((inhibit-read-only t)) (insert "\n* Repro edit\n")) + (condition-case err + (progn (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (message "post-kill cache-map: clean (size=%S)" + (avl-tree-size org-element--cache))) + (error (message "post-kill cache-map: ERROR %S" err))) + (set-buffer-modified-p nil)) + + (message "warnings=%s" (or (repro--warnings) "none"))) + + (delete-file file)) diff --git a/working/org-element-cache-persist-bug/d2103877-reverted.patch b/working/org-element-cache-persist-bug/d2103877-reverted.patch new file mode 100644 index 00000000..512c6abd --- /dev/null +++ b/working/org-element-cache-persist-bug/d2103877-reverted.patch @@ -0,0 +1,199 @@ +commit d2103877f677463cbf113df951f388aaaa4a06ea +Author: Craig Jennings <c@cjennings.net> +Date: Tue Jul 28 18:25:03 2026 -0500 + + fix(dirvish): keep org previews out of the persistent element cache + + - Previewing an open .org file corrupted that file's element cache. + - The next edit to it signalled wrong-type-argument stringp nil. + - Dirvish's fallback preview claims the real file's buffer-file-name. + - set-auto-mode then makes it a second org-mode buffer. + - Killing that preview is what breaks the original buffer. + + I made the opt-out buffer-local, so real org buffers keep persistence. + + That error string is the one cj/org-clear-element-cache was written for, + so the workaround had been hiding this. + +diff --git a/modules/dirvish-config.el b/modules/dirvish-config.el +index edbb0b35..ef5966c9 100644 +--- a/modules/dirvish-config.el ++++ b/modules/dirvish-config.el +@@ -833,7 +833,41 @@ Returns nil if not in a project." + ;; No project found + (t nil))) + +- ++;;; ------------------- Dirvish Preview Org-Element Cache Opt-Out --------------- ++;; dirvish's `fallback' dispatcher previews any file it has no media handler for, ++;; and `dirvish--preview-file-maybe-truncate' builds that preview by pointing ++;; `buffer-file-name' at the real file and running `set-auto-mode'. Previewing an ++;; .org file therefore yields a SECOND org-mode buffer claiming an already-open ++;; file's name. When dirvish later kills that preview buffer, the real buffer's ++;; org-element cache is left corrupt, and the next edit to it signals ++;; `wrong-type-argument stringp nil' from `org-element--cache-after-change'. ++;; ++;; Reproduced 2026-07-28 with controls: no preview is clean, a preview left alive ++;; is clean, and the same sequence with `org-element-cache-persistent' nil is ++;; clean -- so the kill is required and the persistent cache is the mechanism. ++;; (That error string is the one `cj/org-clear-element-cache' was written for, ++;; which is how this hid for so long: the workaround was already in the config.) ++;; ++;; Opting the throwaway preview buffer out of the persistent cache is enough. It ++;; is deliberately buffer-local: the real org buffers must keep persistence. ++ ++;; A variable, not a function -- declared so the byte-compiler knows it is ++;; special without pulling org-element onto dirvish's load path. ++(defvar org-element-cache-persistent) ++ ++(defun cj/--dirvish-preview-disable-org-cache-persist () ++ "Opt a dirvish org-mode preview buffer out of the persistent element cache. ++Runs from `dirvish-preview-setup-hook' in the preview buffer. Does nothing ++outside org-mode, and nothing when `org-element-cache-persistent' is unbound -- ++a signal here would abort dirvish's preview rendering, so every branch is a ++no-op rather than an error. Returns nil." ++ (when (and (derived-mode-p 'org-mode) ++ (boundp 'org-element-cache-persistent)) ++ (setq-local org-element-cache-persistent nil)) ++ nil) ++ ++(add-hook 'dirvish-preview-setup-hook ++ #'cj/--dirvish-preview-disable-org-cache-persist) + + (provide 'dirvish-config) + ;;; dirvish-config.el ends here. +diff --git a/tests/test-dirvish-config--preview-org-cache.el b/tests/test-dirvish-config--preview-org-cache.el +new file mode 100644 +index 00000000..233b04a8 +--- /dev/null ++++ b/tests/test-dirvish-config--preview-org-cache.el +@@ -0,0 +1,129 @@ ++;;; test-dirvish-config--preview-org-cache.el --- preview org-cache opt-out tests -*- lexical-binding: t; -*- ++ ++;;; Commentary: ++;; Dirvish's fallback preview builds a buffer, points `buffer-file-name' at the ++;; real file, and runs `set-auto-mode' -- so previewing an .org file yields a ++;; second org-mode buffer claiming an open file's name. Killing that preview ++;; buffer corrupted the real buffer's element cache, surfacing as ++;; `wrong-type-argument stringp nil' on the next edit. Reproduced 2026-07-28 ++;; with controls: the kill is required, and `org-element-cache-persistent' is ++;; the mechanism (nil made the same sequence clean). ++;; ++;; `cj/--dirvish-preview-disable-org-cache-persist' opts the preview buffer out ++;; of the persistent cache, buffer-locally, leaving the real buffer alone. ++ ++;;; Code: ++ ++(require 'ert) ++(require 'org-element) ++(add-to-list 'load-path (expand-file-name "modules" user-emacs-directory)) ++(require 'dirvish-config) ++ ++(declare-function cj/--dirvish-preview-disable-org-cache-persist "dirvish-config" ()) ++ ++;;; ----------------------------- Normal cases --------------------------------- ++ ++(ert-deftest test-dirvish-preview-org-cache-disables-in-org-buffer () ++ "Normal: an org-mode preview buffer opts out of the persistent cache." ++ (with-temp-buffer ++ (delay-mode-hooks (org-mode)) ++ (cj/--dirvish-preview-disable-org-cache-persist) ++ (should (local-variable-p 'org-element-cache-persistent)) ++ (should-not org-element-cache-persistent))) ++ ++(ert-deftest test-dirvish-preview-org-cache-leaves-global-alone () ++ "Normal: the opt-out is buffer-local and never touches the global value. ++The real org buffers must keep their persistent cache; only the throwaway ++preview buffer opts out. Asserting the global default explicitly is the point ++of this test -- a buffer-local set that leaked to the default would disable ++persistence everywhere and still pass every other test here." ++ (let ((original (default-value 'org-element-cache-persistent))) ++ (unwind-protect ++ (progn ++ (setq-default org-element-cache-persistent t) ++ (with-temp-buffer ++ (delay-mode-hooks (org-mode)) ++ (cj/--dirvish-preview-disable-org-cache-persist) ++ (should-not org-element-cache-persistent)) ++ ;; The default survived the buffer-local set. ++ (should (eq (default-value 'org-element-cache-persistent) t)) ++ ;; And a fresh org buffer still inherits persistence. ++ (with-temp-buffer ++ (delay-mode-hooks (org-mode)) ++ (should org-element-cache-persistent))) ++ (setq-default org-element-cache-persistent original)))) ++ ++;;; ---------------------------- Boundary cases -------------------------------- ++ ++(ert-deftest test-dirvish-preview-org-cache-ignores-non-org-buffer () ++ "Boundary: a non-org preview buffer is left completely alone." ++ (with-temp-buffer ++ (fundamental-mode) ++ (cj/--dirvish-preview-disable-org-cache-persist) ++ (should-not (local-variable-p 'org-element-cache-persistent)))) ++ ++(ert-deftest test-dirvish-preview-org-cache-covers-derived-mode () ++ "Boundary: a mode derived from org-mode still opts out. ++The check is `derived-mode-p', not an `eq' on `major-mode', so org derivatives ++previewed by dirvish are covered too." ++ (with-temp-buffer ++ (delay-mode-hooks (org-mode)) ++ (define-derived-mode cj-test-org-derived-mode org-mode "TestOrg" ++ "Throwaway org derivative for this test.") ++ (delay-mode-hooks (cj-test-org-derived-mode)) ++ (cj/--dirvish-preview-disable-org-cache-persist) ++ (should (local-variable-p 'org-element-cache-persistent)) ++ (should-not org-element-cache-persistent))) ++ ++(ert-deftest test-dirvish-preview-org-cache-is-idempotent () ++ "Boundary: running twice in one buffer is a no-op the second time." ++ (with-temp-buffer ++ (delay-mode-hooks (org-mode)) ++ (cj/--dirvish-preview-disable-org-cache-persist) ++ (cj/--dirvish-preview-disable-org-cache-persist) ++ (should (local-variable-p 'org-element-cache-persistent)) ++ (should-not org-element-cache-persistent))) ++ ++;;; ------------------------------ Error cases --------------------------------- ++ ++(ert-deftest test-dirvish-preview-org-cache-survives-missing-variable () ++ "Error: no signal when `org-element-cache-persistent' is not bound. ++The function runs from a dirvish hook; an error there would break preview ++rendering, so a missing org internal must degrade to a no-op. ++ ++Drives real state rather than mocking `boundp'. `boundp' is a C primitive, and ++redefining one is unreliable under native-comp -- a natively-compiled caller can ++run the real primitive through a trampoline and quietly ignore the mock, which ++would turn this into a test that passes without exercising the guard. Org-mode ++is entered before the unbind so mode setup never reads the missing variable." ++ (let* ((bound (boundp 'org-element-cache-persistent)) ++ (original (and bound (default-value 'org-element-cache-persistent)))) ++ (unwind-protect ++ (with-temp-buffer ++ (delay-mode-hooks (org-mode)) ++ (makunbound 'org-element-cache-persistent) ++ (should-not (boundp 'org-element-cache-persistent)) ++ (should-not (cj/--dirvish-preview-disable-org-cache-persist)) ++ ;; The guard declined to create a binding rather than erroring. ++ (should-not (local-variable-p 'org-element-cache-persistent))) ++ (when bound (setq-default org-element-cache-persistent original))))) ++ ++(ert-deftest test-dirvish-preview-org-cache-runs-in-a-fileless-buffer () ++ "Error: a buffer with no `buffer-file-name' is handled without signalling." ++ (with-temp-buffer ++ (delay-mode-hooks (org-mode)) ++ (should-not (buffer-file-name)) ++ (cj/--dirvish-preview-disable-org-cache-persist) ++ (should-not org-element-cache-persistent))) ++ ++;;; ----------------------------- Registration --------------------------------- ++ ++(ert-deftest test-dirvish-preview-org-cache-hook-registered () ++ "Normal: the opt-out is on `dirvish-preview-setup-hook'. ++Registration is the half that makes the fix reach dirvish at all -- the ++function being correct is useless if nothing calls it." ++ (should (memq 'cj/--dirvish-preview-disable-org-cache-persist ++ (default-value 'dirvish-preview-setup-hook)))) ++ ++(provide 'test-dirvish-config--preview-org-cache) ++;;; test-dirvish-config--preview-org-cache.el ends here diff --git a/working/org-element-cache-persist-bug/hostile-a-errorbranch.el b/working/org-element-cache-persist-bug/hostile-a-errorbranch.el new file mode 100644 index 00000000..88711787 --- /dev/null +++ b/working/org-element-cache-persist-bug/hostile-a-errorbranch.el @@ -0,0 +1,76 @@ +;;; hostile-a-errorbranch.el --- attack the dirvish.el:680 error branch -*- lexical-binding: t -*- + +;; dirvish--preview-file-maybe-truncate (dirvish.el:671-680) wraps +;; (set-auto-mode) (font-lock-mode 1) (and so-long-detected-p (error ...)) +;; in a condition-case. On ANY error the buffer is KILLED at line 680 and +;; `dirvish-preview-setup-hook' at line 682 is NEVER reached. By then +;; set-auto-mode has already put the buffer in org-mode with buffer-file-name +;; pointing at the real file. So the shipped fix cannot run on that path. + +(require 'org) (require 'org-element) (require 'org-persist) +(add-to-list 'load-path (expand-file-name "modules" default-directory)) +(require 'dirvish-config) + +(setq org-element-use-cache t + org-element-cache-persistent t + org-element--cache-self-verify t + org-element--cache-self-verify-frequency 1.0) + +(defun h--walk (buf) + (with-current-buffer buf + (org-element-cache-map (lambda (_e) nil) :granularity 'element) + (and org-element--cache (avl-tree-size org-element--cache)))) + +;; Faithful replay of dirvish.el:657-684, including the condition-case and the +;; error-branch kill. ERROR-INJECT non-nil simulates so-long-detected-p / any +;; other signal raised AFTER set-auto-mode has entered org-mode. +(defun h--preview (file error-inject) + (with-current-buffer (get-buffer-create "*preview-temp*") + (let ((threshold 1048576) info jka-compr-verbose) + (with-silent-modifications + (setq buffer-read-only t) + (insert-file-contents file nil 0 threshold) + (setq buffer-file-name file) + (goto-char (point-min)) + (rename-buffer (format "PREVIEW :: 999 :: %s" (file-name-nondirectory file)))) + (condition-case err + (let ((enable-dir-local-variables nil) (enable-local-variables :safe) + (non-essential t) (inhibit-message t)) + (setq-local delay-mode-hooks t) + (set-auto-mode) (font-lock-mode 1) + (and error-inject (error "No preview of file with long lines"))) + (error (setq info (error-message-string err)))) + (message " preview: mode=%s file=%s persist-local=%s registered-kill-hook=%s" + major-mode (and buffer-file-name t) + (local-variable-p 'org-element-cache-persistent) + (and (memq 'org-persist-write-all-buffer kill-buffer-hook) t)) + (if info + (progn (message " ERROR BRANCH taken (%s) -> kill WITHOUT hook" info) + (let (kill-buffer-query-functions) (kill-buffer (current-buffer)))) + (run-hooks 'dirvish-preview-setup-hook) + (message " ok branch: hook ran, persist-local=%s value=%s" + (local-variable-p 'org-element-cache-persistent) + org-element-cache-persistent) + (let (kill-buffer-query-functions) (kill-buffer (current-buffer))))))) + +(defun h--trial (label error-inject) + (let* ((src (expand-file-name "todo.org" default-directory)) + (file (make-temp-file "hostile-" nil ".org"))) + (copy-file src file t) + (message "== %s ==" label) + (let ((real (find-file-noselect file))) + (message " real cache: %S" (h--walk real)) + (h--preview file error-inject) + (with-current-buffer real + (goto-char (point-max)) + (condition-case err + (progn (let ((inhibit-read-only t)) (insert "\n* Hostile edit\n")) + (org-element-cache-map (lambda (_e) nil) :granularity 'element) + (message " RESULT: CLEAN (size=%S)" (avl-tree-size org-element--cache))) + (error (message " RESULT: CORRUPT -> %S" err))) + (set-buffer-modified-p nil)) + (let (kill-buffer-query-functions) (kill-buffer real))) + (delete-file file))) + +(h--trial "control: no error, hook runs (shipped fix active)" nil) +(h--trial "attack: error after set-auto-mode -> hook skipped" t) diff --git a/working/org-element-cache-persist-bug/hostile-c-repro.el b/working/org-element-cache-persist-bug/hostile-c-repro.el new file mode 100644 index 00000000..17627e63 --- /dev/null +++ b/working/org-element-cache-persist-bug/hostile-c-repro.el @@ -0,0 +1,32 @@ +;;; -*- lexical-binding: t -*- +(setq org-persist-directory (expand-file-name "hostile-c-persist" default-directory)) +(require 'org) +(require 'org-element) +(require 'org-persist) +(let* ((f (expand-file-name "hostile-c-real.org" default-directory))) + (with-temp-file f (insert "* Head one\nbody\n* Head two\n")) + ;; 1. the REAL buffer, as Craig has open + (let ((real (find-file-noselect f))) + (with-current-buffer real (org-element-at-point (point-max))) + (princ (format "real registered: %S\n" + (and (org-persist--find-index + `(:container ,(org-persist--normalize-container + `((elisp org-element--cache) (version ,org-element-cache-version))) + :associated ,(org-persist--normalize-associated real))) + t))) + ;; 2. dirvish preview: second org buffer claiming the same file name + (let ((prev (get-buffer-create "PREVIEW :: hostile-c-real.org"))) + (with-current-buffer prev + (insert-file-contents f) + (setq buffer-file-name f) + (delay-mode-hooks (org-mode)) + ;; the commit's hook + (setq-local org-element-cache-persistent nil) + ;; 3. anything that resets the cache in this live preview buffer + (org-element-cache-reset)) + (princ (format "real registered AFTER preview reset: %S\n" + (and (org-persist--find-index + `(:container ,(org-persist--normalize-container + `((elisp org-element--cache) (version ,org-element-cache-version))) + :associated ,(org-persist--normalize-associated real))) + t)))))) diff --git a/working/org-element-cache-persist-bug/hostile-d-faithful.el b/working/org-element-cache-persist-bug/hostile-d-faithful.el new file mode 100644 index 00000000..bbd0aecb --- /dev/null +++ b/working/org-element-cache-persist-bug/hostile-d-faithful.el @@ -0,0 +1,48 @@ +;; Faithful replay: real dirvish env incl. delay-mode-hooks, inhibit-message, +;; timestamped buffer name, preview-hash text reuse, dirvish--kill-buffer shape. +(require 'cl-lib) (require 'org) (require 'org-element) (require 'org-persist) +(when (getenv "FIX") + (add-to-list 'load-path (expand-file-name "modules" default-directory)) + (require 'dirvish-config)) +(setq org-element-use-cache t org-element-cache-persistent t) +(defvar hash (make-hash-table :test 'equal)) +(defun nilbuf (b) (with-current-buffer b + (let ((k 0)) (avl-tree-mapc (lambda (e) (unless (org-element-property :buffer e) (cl-incf k))) org-element--cache) k))) +(defun preview (file name) + (with-current-buffer (get-buffer-create "*preview-temp*") + (let ((text (gethash file hash)) info jka-compr-verbose) + (with-silent-modifications + (setq buffer-read-only t) + (if text (insert text) (insert-file-contents file nil 0 1048576)) + (setq buffer-file-name file) (goto-char (point-min)) + (rename-buffer name)) + (condition-case err + (eval `(let ((inhibit-message t) (non-essential t) + (enable-dir-local-variables nil) (enable-local-variables :safe)) + (setq-local delay-mode-hooks t) + (set-auto-mode) (font-lock-mode 1))) + (error (setq info (error-message-string err)))) + (if info (message ">> preview aborted: %s" info) + (run-hooks 'dirvish-preview-setup-hook) + (unless text (puthash file (buffer-string) hash))) + (message ">> preview mode=%s delay-mode-hooks=%S persist=%S" + major-mode delay-mode-hooks org-element-cache-persistent) + (current-buffer)))) +(defun dv-kill (b) (and (buffer-live-p b) (let (kill-buffer-query-functions) (kill-buffer b)))) +(let* ((file (make-temp-file "fa-" nil ".org"))) + (copy-file (expand-file-name "todo.org" default-directory) file t) + (let ((real (find-file-noselect file))) + (with-current-buffer real (org-element-cache-map (lambda (_) nil) :granularity 'element)) + (message ">> real nil-buffer before=%d" (nilbuf real)) + ;; pass 1 + (dv-kill (preview file (format "PREVIEW :: %s :: %s" "1753000000" (file-name-nondirectory file)))) + (message ">> after pass1 kill nil-buffer=%d gfb=%S" (nilbuf real) (buffer-name (get-file-buffer file))) + ;; pass 2 (hash hit: text branch) + (dv-kill (preview file (format "PREVIEW :: %s :: %s" "1753000001" (file-name-nondirectory file)))) + (message ">> after pass2 kill nil-buffer=%d" (nilbuf real)) + (with-current-buffer real + (goto-char (point-max)) + (condition-case err (let ((inhibit-read-only t)) (insert "\n* E\n")) + (error (message ">> RESULT: SIGNALLED %S" err))) + (message ">> edit completed") (set-buffer-modified-p nil))) + (delete-file file)) diff --git a/working/org-element-cache-persist-bug/hostile-d-nopreview.el b/working/org-element-cache-persist-bug/hostile-d-nopreview.el new file mode 100644 index 00000000..91ae1ce7 --- /dev/null +++ b/working/org-element-cache-persist-bug/hostile-d-nopreview.el @@ -0,0 +1,23 @@ +(require 'cl-lib) (require 'org) (require 'org-element) (require 'org-persist) +(setq org-element-use-cache t org-element-cache-persistent t) +(when (getenv "SV") (setq org-element--cache-self-verify t org-element--cache-self-verify-frequency 1.0)) +(defun nilbuf (buf) (with-current-buffer buf + (let ((k 0)) (avl-tree-mapc (lambda (el) (unless (org-element-property :buffer el) (cl-incf k))) org-element--cache) k))) +(let* ((file (make-temp-file "np-" nil ".org"))) + (copy-file (expand-file-name "todo.org" default-directory) file t) + (let ((real (find-file-noselect file))) + (with-current-buffer real (org-element-cache-map (lambda (_) nil) :granularity 'element)) + (message ">> before: nil-buffer=%d" (nilbuf real)) + (message ">> MODE=%s" (getenv "MODE")) + (pcase (getenv "MODE") + ("writeall" (org-persist-write-all)) + ("writebuf" (with-current-buffer real (org-persist-write-all-buffer))) + (_ nil)) + (message ">> after trigger: nil-buffer=%d" (nilbuf real)) + (with-current-buffer real + (goto-char (point-max)) + (condition-case err (let ((inhibit-read-only t)) (insert "\n* Edit\n")) + (error (message ">> RESULT: insert SIGNALLED %S" err))) + (message ">> insert survived (nil-buffer=%d)" (nilbuf real)) + (set-buffer-modified-p nil))) + (delete-file file)) diff --git a/working/org-element-cache-persist-bug/hostile-d-order.el b/working/org-element-cache-persist-bug/hostile-d-order.el new file mode 100644 index 00000000..898e90fe --- /dev/null +++ b/working/org-element-cache-persist-bug/hostile-d-order.el @@ -0,0 +1,30 @@ +(require 'cl-lib) (require 'org) (require 'org-element) (require 'org-persist) +(setq org-element-use-cache t org-element-cache-persistent t) +(defun nilbuf (buf) (with-current-buffer buf + (let ((k 0)) (avl-tree-mapc (lambda (el) (unless (org-element-property :buffer el) (cl-incf k))) org-element--cache) k))) +(let* ((file (make-temp-file "ord-" nil ".org"))) + (copy-file (expand-file-name "todo.org" default-directory) file t) + (let ((real (find-file-noselect file))) + (with-current-buffer real (org-element-cache-map (lambda (_) nil) :granularity 'element)) + (with-current-buffer (get-buffer-create "*preview-temp*") + (let (jka-compr-verbose) + (with-silent-modifications + (setq buffer-read-only t) (insert-file-contents file nil 0 1048576) + (setq buffer-file-name file) (goto-char (point-min)) (rename-buffer "PREVIEW")) + (let ((enable-dir-local-variables nil) (enable-local-variables :safe) (non-essential t)) + (set-auto-mode) (font-lock-mode 1))) + (org-element-cache-map (lambda (_) nil) :granularity 'element)) + (when (equal (getenv "BURY") "1") + (bury-buffer real) + (message ">> buried real")) + (message ">> get-file-buffer -> %S (buffer-list head: %S)" + (buffer-name (get-file-buffer file)) + (mapcar #'buffer-name (seq-take (buffer-list) 4))) + (let ((kill-buffer-query-functions nil)) (kill-buffer "PREVIEW")) + (message ">> after kill nil-buffer=%d" (nilbuf real)) + (with-current-buffer real + (goto-char (point-max)) + (condition-case err (let ((inhibit-read-only t)) (insert "\n* E\n")) + (error (message ">> RESULT: SIGNALLED %S" err))) + (message ">> done") (set-buffer-modified-p nil))) + (delete-file file)) diff --git a/working/org-element-cache-persist-bug/hostile-e-ffnoselect.el b/working/org-element-cache-persist-bug/hostile-e-ffnoselect.el new file mode 100644 index 00000000..b4c474ba --- /dev/null +++ b/working/org-element-cache-persist-bug/hostile-e-ffnoselect.el @@ -0,0 +1,8 @@ +(let ((f (make-temp-file "h-e-" nil ".org"))) + (with-temp-file f (insert "* real\n")) + (let* ((real (find-file-noselect f)) + (temp (cdr `(buffer . ,(eval `(let ((vc-follow-symlinks t) (find-file-hook nil)) + (find-file-noselect ,f 'nowarn))))))) + (message "find-file-temporarily returned the USER'S buffer: %s (same=%s) name=%s" + (buffer-name temp) (eq real temp) (buffer-name real))) + (delete-file f)) diff --git a/working/org-element-cache-persist-bug/refute-fontify-rearms.el b/working/org-element-cache-persist-bug/refute-fontify-rearms.el new file mode 100644 index 00000000..fd55194b --- /dev/null +++ b/working/org-element-cache-persist-bug/refute-fontify-rearms.el @@ -0,0 +1,49 @@ +;; Does the preview buffer re-register after the let unwinds, on fontification? +(require 'org) (require 'org-element) (require 'org-persist) +(defvar rk-dir (make-temp-file "refute-" t)) +(setq org-persist-directory (expand-file-name "persist" rk-dir)) +(setq org-element-use-cache t org-element-cache-persistent t org-log-done nil) +(defvar rk-file (expand-file-name "notes.org" rk-dir)) +(with-temp-file rk-file + (dotimes (i 20) (insert (format "* TODO task %d\nbody with src_python{1+1} inline\n" i)))) + +(defun rk-bufs (buf) + (with-current-buffer buf + (let (s) (avl-tree-mapc (lambda (el) (push (org-element-property :buffer el) s)) + org-element--cache) + (delete-dups s)))) + +(let ((real (find-file-noselect rk-file))) + (with-current-buffer real + (org-element-cache-reset) + (org-element-cache-map #'ignore :granularity 'headline) + (goto-char (point-min)) (re-search-forward "^\\* TODO task 0") (beginning-of-line) + (org-todo "DONE")) + (message "before: %S" (rk-bufs real)) + ;; preview WITH the fix's environment, then let it LIVE and fontify (the step my tests skip) + (let ((prev (get-buffer-create "*prev*"))) + (with-current-buffer prev + (with-silent-modifications + (insert-file-contents rk-file nil 0 1048576) + (setq buffer-file-name rk-file)) + (eval `(let ((inhibit-message t) (non-essential t) + (enable-dir-local-variables nil) (enable-local-variables :safe) + (org-element-use-cache nil)) + (setq-local delay-mode-hooks t) (set-auto-mode) (font-lock-mode 1))) + (message "after set-auto-mode: local-cache=%S global=%S persist-hook=%S" + org-element-use-cache (default-value 'org-element-use-cache) + (and (memq 'org-persist-write-all-buffer kill-buffer-hook) t)) + ;; THE MISSING STEP: the preview lives in a window and gets fontified + (font-lock-ensure) + (message "after fontify: persist-hook=%S cache-active=%S" + (and (memq 'org-persist-write-all-buffer kill-buffer-hook) t) + (org-element--cache-active-p))) + (let (kill-buffer-query-functions) (kill-buffer prev))) + (message "after kill: %S" (rk-bufs real)) + (with-current-buffer real + (message "parse => %s" + (condition-case e (progn (org-element-cache-map + (lambda (el) (org-element-property :raw-value el)) + :granularity 'headline) "OK") + (error (format "ERROR %S" e)))))) +(delete-directory rk-dir t) diff --git a/working/org-element-cache-persist-bug/repro-preview-cache.el b/working/org-element-cache-persist-bug/repro-preview-cache.el new file mode 100644 index 00000000..2a527af1 --- /dev/null +++ b/working/org-element-cache-persist-bug/repro-preview-cache.el @@ -0,0 +1,88 @@ +;;; repro-preview-cache.el --- Does dirvish's text preview disturb an org buffer's element cache? -*- lexical-binding: t -*- + +;; Replays `dirvish--preview-file-maybe-truncate' (dirvish.el:653-684) against an +;; org file that is ALSO open for real, then kills the preview buffer the way +;; dirvish does (`dirvish--kill-buffer', dirvish.el:416). The kill is the step +;; that matters: killing an org-mode buffer with a `buffer-file-name' is when +;; org-persist writes that file's cache. + +(require 'org) +(require 'org-element) +(require 'org-persist) + +(setq org-element-use-cache t + org-element-cache-persistent t + org-element--cache-self-verify t + org-element--cache-self-verify-frequency 1.0) + +(defun repro--build-cache (buf) + "Actually populate the element cache of BUF by walking it." + (with-current-buffer buf + (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (and org-element--cache (avl-tree-size org-element--cache)))) + +(defun repro--warnings () + (when (get-buffer "*Warnings*") + (with-current-buffer "*Warnings*" (string-trim (buffer-string))))) + +(let* ((src (expand-file-name "todo.org" default-directory)) + (file (make-temp-file "repro-" nil ".org")) + (threshold 1048576)) + (copy-file src file t) + (let ((size (nth 7 (file-attributes file)))) + (message "== fixture %s (%d bytes, %s threshold) ==" + (file-name-nondirectory file) size + (if (>= size threshold) "over" "under"))) + + (let ((real (find-file-noselect file))) + (message "real buffer cache after full walk: %S" (repro--build-cache real)) + + ;; --- dirvish text preview of the same file --- + (with-current-buffer (get-buffer-create "*preview-temp*") + (let (jka-compr-verbose) + (with-silent-modifications + (setq buffer-read-only t) + (insert-file-contents file nil 0 threshold) + (when (>= (nth 7 (file-attributes file)) threshold) + (goto-char (point-max)) + (insert "\n\nFile truncated. End of partial preview.\n")) + (setq buffer-file-name file) + (goto-char (point-min)) + (rename-buffer (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (let ((enable-dir-local-variables nil) + (enable-local-variables :safe) + (non-essential t)) + (set-auto-mode) + (font-lock-mode 1))) + (message "preview buffer: mode=%s claims=%s cache=%S" + major-mode (file-name-nondirectory (or buffer-file-name "none")) + (repro--build-cache (current-buffer)))) + + (message "-- both buffers live: real=%S claiming-file=%d --" + (with-current-buffer real (avl-tree-size org-element--cache)) + (length (seq-filter (lambda (b) (equal (buffer-local-value 'buffer-file-name b) file)) + (buffer-list)))) + + ;; --- dirvish kills the preview buffer when the session ends --- + (let ((kill-buffer-query-functions nil) + (preview (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (kill-buffer preview) + (message "preview buffer %S killed" preview)) + + ;; --- is the real buffer still coherent? --- + (with-current-buffer real + ;; Edit it the way Craig would after stepping back from dirvish. + (goto-char (point-max)) + (condition-case err + (let ((inhibit-read-only t)) (insert "\n* Repro edit\n")) + (error (message "RESULT: insert into real buffer SIGNALLED %S" (car err)))) + (condition-case err + (progn (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (message "post-kill cache-map: clean (size=%S)" + (avl-tree-size org-element--cache))) + (error (message "post-kill cache-map: ERROR %S" err))) + (set-buffer-modified-p nil)) + + (message "warnings=%s" (or (repro--warnings) "none"))) + + (delete-file file)) diff --git a/working/org-element-cache-persist-bug/upstream-report.txt b/working/org-element-cache-persist-bug/upstream-report.txt new file mode 100644 index 00000000..81fcaaa7 --- /dev/null +++ b/working/org-element-cache-persist-bug/upstream-report.txt @@ -0,0 +1,87 @@ +Subject: [BUG] Persisting the element cache while its buffer is live corrupts it [9.7.11 (release_9.7.11 @ /usr/share/emacs/30.2/lisp/org/)] + +Writing the element cache to disk strips the :buffer property from every cached +element in the live buffer and never puts it back. The next access to a cached +headline signals (wrong-type-argument stringp nil), and it keeps signalling until +the cache is reset by hand. + +Minimal reproduction, self-contained, no dirvish or other packages involved. Save +as repro.el and run "emacs -Q --batch -l repro.el": + + (require 'org) + (require 'org-element) + (require 'org-persist) + + (let* ((dir (make-temp-file "org-persist-repro-" t)) + (org-persist-directory (expand-file-name "persist" dir)) + (file (expand-file-name "notes.org" dir)) + (org-element-use-cache t) + (org-element-cache-persistent t) + (org-log-done nil)) + (with-temp-file file + (dotimes (i 20) (insert (format "* TODO task %d\nbody %d\n" i i)))) + (let ((buffer (find-file-noselect file))) + (with-current-buffer buffer + (org-element-cache-reset) + (org-element-cache-map #'ignore :granularity 'headline) + (goto-char (point-min)) + (re-search-forward "^\\* TODO task 0") + (beginning-of-line) + (org-todo "DONE")) + (org-persist-write `((elisp org-element--cache) + (version ,org-element-cache-version)) + buffer t) + (with-current-buffer buffer + (message "%S" + (condition-case err + (progn (org-element-cache-map + (lambda (e) (org-element-property :raw-value e)) + :granularity 'headline) + "ok") + (error err)))) + (set-buffer-modified-p nil) + (kill-buffer buffer)) + (delete-directory dir t)) + +Expected: "ok". +Actual: (wrong-type-argument stringp nil). + +Adding a print of the cached :buffer values on either side of the write shows +(#<buffer notes.org>) before and (nil) after. + +The mechanism, as far as I can follow it. org-element--cache-persist-before-write +walks org-element--cache and clears :buffer on every element. That looks +deliberate, since buffer objects can't be printed, and a8286a5a9 describes it as +"Clear and restore non-printable buffer objects in :buffer property". The restore +half is org-element--cache-persist-after-read, which sets :buffer back to +(current-buffer). But that only runs on the read path. Nothing restores :buffer +after a write, so a write against a buffer that is still alive leaves the live +cache holding nils. + +The error itself comes from org-element--headline-parse-title. Cached elements +keep their deferred :title and :tags thunks, and resolving one runs +(with-current-buffer (org-element-property :buffer headline)). set-buffer on nil +is the signal. + +How I reached it without calling org-persist-write directly. Any second buffer +visiting the same file will do it, because org-persist registers a buffer-local +org-persist-write-all-buffer on kill-buffer-hook, and the write's hook resolves +(get-file-buffer FILE), which can return the other buffer. In my case dirvish +built a preview buffer, pointed buffer-file-name at a file I already had open, +and ran set-auto-mode. Killing that preview corrupted the cache of the buffer I +was working in. Any package that makes a throwaway buffer visiting a live file +can trigger it, so I don't think this is dirvish's bug to fix. + +Two details that might help narrow it. Which buffer gets damaged depends on +buffer-list order, since get-file-buffer returns the first buffer visiting the +file. And a nearly empty cache survives, because the next access reparses instead +of reading a stripped element. That combination is probably why this shows up as +an intermittent "org-element cache is broken again" rather than a clean repro. + +Setting org-element-cache-persistent to nil avoids it, which I see recommended on +this list fairly often. It looks like it may be working around this rather than a +separate problem. + +I haven't written a patch. From the outside the symmetric fix would be to restore +:buffer after an in-session write the way the read path does, rather than to stop +clearing it. Happy to test a patch against the reproduction above. diff --git a/working/org-element-cache-persist-bug/upstream-repro.el b/working/org-element-cache-persist-bug/upstream-repro.el new file mode 100644 index 00000000..d0a96774 --- /dev/null +++ b/working/org-element-cache-persist-bug/upstream-repro.el @@ -0,0 +1,70 @@ +;;; upstream-repro.el --- minimal reproduction for the org list -*- lexical-binding: t -*- + +;; Run with: emacs -Q --batch -l upstream-repro.el +;; +;; Writing the element cache to disk while the buffer is still live strips the +;; :buffer property from every cached element and never restores it. The next +;; access to a cached headline signals (wrong-type-argument stringp nil). + +(require 'org) +(require 'org-element) +(require 'org-persist) + +(let* ((dir (make-temp-file "org-persist-repro-" t)) + (org-persist-directory (expand-file-name "persist" dir)) + (file (expand-file-name "notes.org" dir)) + (org-element-use-cache t) + (org-element-cache-persistent t) + (org-log-done nil)) + + (with-temp-file file + (dotimes (i 20) + (insert (format "* TODO task %d\nbody %d\n" i i)))) + + (let ((buffer (find-file-noselect file))) + (with-current-buffer buffer + ;; Populate the cache with parsed headlines. + (org-element-cache-reset) + (org-element-cache-map #'ignore :granularity 'headline) + (goto-char (point-min)) + (re-search-forward "^\\* TODO task 0") + (beginning-of-line) + (org-todo "DONE")) + + (message "cached :buffer values before write: %S" + (with-current-buffer buffer + (let (values) + (avl-tree-mapc (lambda (element) + (push (org-element-property :buffer element) values)) + org-element--cache) + (delete-dups values)))) + + ;; Persist while the buffer is still alive. In a real session this is + ;; reached by any in-session write, e.g. `org-persist-write-all-buffer' from + ;; `kill-buffer-hook' in a *second* buffer visiting the same file. + (org-persist-write `((elisp org-element--cache) + (version ,org-element-cache-version)) + buffer t) + + (message "cached :buffer values after write: %S" + (with-current-buffer buffer + (let (values) + (avl-tree-mapc (lambda (element) + (push (org-element-property :buffer element) values)) + org-element--cache) + (delete-dups values)))) + + (with-current-buffer buffer + (message "reading a cached headline: %s" + (condition-case err + (progn (org-element-cache-map + (lambda (element) (org-element-property :raw-value element)) + :granularity 'headline) + "ok") + (error (format "%S" err))))) + + (set-buffer-modified-p nil) + (kill-buffer buffer)) + (delete-directory dir t)) + +;;; upstream-repro.el ends here diff --git a/working/org-element-cache-persist-bug/variant-no-kill.el b/working/org-element-cache-persist-bug/variant-no-kill.el new file mode 100644 index 00000000..180f6cd2 --- /dev/null +++ b/working/org-element-cache-persist-bug/variant-no-kill.el @@ -0,0 +1,88 @@ +;;; repro-preview-cache.el --- Does dirvish's text preview disturb an org buffer's element cache? -*- lexical-binding: t -*- + +;; Replays `dirvish--preview-file-maybe-truncate' (dirvish.el:653-684) against an +;; org file that is ALSO open for real, then kills the preview buffer the way +;; dirvish does (`dirvish--kill-buffer', dirvish.el:416). The kill is the step +;; that matters: killing an org-mode buffer with a `buffer-file-name' is when +;; org-persist writes that file's cache. + +(require 'org) +(require 'org-element) +(require 'org-persist) + +(setq org-element-use-cache t + org-element-cache-persistent t + org-element--cache-self-verify t + org-element--cache-self-verify-frequency 1.0) + +(defun repro--build-cache (buf) + "Actually populate the element cache of BUF by walking it." + (with-current-buffer buf + (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (and org-element--cache (avl-tree-size org-element--cache)))) + +(defun repro--warnings () + (when (get-buffer "*Warnings*") + (with-current-buffer "*Warnings*" (string-trim (buffer-string))))) + +(let* ((src (expand-file-name "todo.org" default-directory)) + (file (make-temp-file "repro-" nil ".org")) + (threshold 1048576)) + (copy-file src file t) + (let ((size (nth 7 (file-attributes file)))) + (message "== fixture %s (%d bytes, %s threshold) ==" + (file-name-nondirectory file) size + (if (>= size threshold) "over" "under"))) + + (let ((real (find-file-noselect file))) + (message "real buffer cache after full walk: %S" (repro--build-cache real)) + + ;; --- dirvish text preview of the same file --- + (with-current-buffer (get-buffer-create "*preview-temp*") + (let (jka-compr-verbose) + (with-silent-modifications + (setq buffer-read-only t) + (insert-file-contents file nil 0 threshold) + (when (>= (nth 7 (file-attributes file)) threshold) + (goto-char (point-max)) + (insert "\n\nFile truncated. End of partial preview.\n")) + (setq buffer-file-name file) + (goto-char (point-min)) + (rename-buffer (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (let ((enable-dir-local-variables nil) + (enable-local-variables :safe) + (non-essential t)) + (set-auto-mode) + (font-lock-mode 1))) + (message "preview buffer: mode=%s claims=%s cache=%S" + major-mode (file-name-nondirectory (or buffer-file-name "none")) + (repro--build-cache (current-buffer)))) + + (message "-- both buffers live: real=%S claiming-file=%d --" + (with-current-buffer real (avl-tree-size org-element--cache)) + (length (seq-filter (lambda (b) (equal (buffer-local-value 'buffer-file-name b) file)) + (buffer-list)))) + + ;; --- dirvish kills the preview buffer when the session ends --- + (let ((kill-buffer-query-functions nil) + (preview (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (ignore preview) + (message "VARIANT: preview buffer left ALIVE")) + + ;; --- is the real buffer still coherent? --- + (with-current-buffer real + ;; Edit it the way Craig would after stepping back from dirvish. + (goto-char (point-max)) + (condition-case err + (let ((inhibit-read-only t)) (insert "\n* Repro edit\n")) + (error (message "RESULT: insert into real buffer SIGNALLED %S" (car err)))) + (condition-case err + (progn (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (message "post-kill cache-map: clean (size=%S)" + (avl-tree-size org-element--cache))) + (error (message "post-kill cache-map: ERROR %S" err))) + (set-buffer-modified-p nil)) + + (message "warnings=%s" (or (repro--warnings) "none"))) + + (delete-file file)) diff --git a/working/org-element-cache-persist-bug/variant-no-persist.el b/working/org-element-cache-persist-bug/variant-no-persist.el new file mode 100644 index 00000000..75c85bc6 --- /dev/null +++ b/working/org-element-cache-persist-bug/variant-no-persist.el @@ -0,0 +1,88 @@ +;;; repro-preview-cache.el --- Does dirvish's text preview disturb an org buffer's element cache? -*- lexical-binding: t -*- + +;; Replays `dirvish--preview-file-maybe-truncate' (dirvish.el:653-684) against an +;; org file that is ALSO open for real, then kills the preview buffer the way +;; dirvish does (`dirvish--kill-buffer', dirvish.el:416). The kill is the step +;; that matters: killing an org-mode buffer with a `buffer-file-name' is when +;; org-persist writes that file's cache. + +(require 'org) +(require 'org-element) +(require 'org-persist) + +(setq org-element-use-cache t + org-element-cache-persistent nil + org-element--cache-self-verify t + org-element--cache-self-verify-frequency 1.0) + +(defun repro--build-cache (buf) + "Actually populate the element cache of BUF by walking it." + (with-current-buffer buf + (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (and org-element--cache (avl-tree-size org-element--cache)))) + +(defun repro--warnings () + (when (get-buffer "*Warnings*") + (with-current-buffer "*Warnings*" (string-trim (buffer-string))))) + +(let* ((src (expand-file-name "todo.org" default-directory)) + (file (make-temp-file "repro-" nil ".org")) + (threshold 1048576)) + (copy-file src file t) + (let ((size (nth 7 (file-attributes file)))) + (message "== fixture %s (%d bytes, %s threshold) ==" + (file-name-nondirectory file) size + (if (>= size threshold) "over" "under"))) + + (let ((real (find-file-noselect file))) + (message "real buffer cache after full walk: %S" (repro--build-cache real)) + + ;; --- dirvish text preview of the same file --- + (with-current-buffer (get-buffer-create "*preview-temp*") + (let (jka-compr-verbose) + (with-silent-modifications + (setq buffer-read-only t) + (insert-file-contents file nil 0 threshold) + (when (>= (nth 7 (file-attributes file)) threshold) + (goto-char (point-max)) + (insert "\n\nFile truncated. End of partial preview.\n")) + (setq buffer-file-name file) + (goto-char (point-min)) + (rename-buffer (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (let ((enable-dir-local-variables nil) + (enable-local-variables :safe) + (non-essential t)) + (set-auto-mode) + (font-lock-mode 1))) + (message "preview buffer: mode=%s claims=%s cache=%S" + major-mode (file-name-nondirectory (or buffer-file-name "none")) + (repro--build-cache (current-buffer)))) + + (message "-- both buffers live: real=%S claiming-file=%d --" + (with-current-buffer real (avl-tree-size org-element--cache)) + (length (seq-filter (lambda (b) (equal (buffer-local-value 'buffer-file-name b) file)) + (buffer-list)))) + + ;; --- dirvish kills the preview buffer when the session ends --- + (let ((kill-buffer-query-functions nil) + (preview (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (kill-buffer preview) + (message "preview buffer %S killed" preview)) + + ;; --- is the real buffer still coherent? --- + (with-current-buffer real + ;; Edit it the way Craig would after stepping back from dirvish. + (goto-char (point-max)) + (condition-case err + (let ((inhibit-read-only t)) (insert "\n* Repro edit\n")) + (error (message "RESULT: insert into real buffer SIGNALLED %S" (car err)))) + (condition-case err + (progn (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (message "post-kill cache-map: clean (size=%S)" + (avl-tree-size org-element--cache))) + (error (message "post-kill cache-map: ERROR %S" err))) + (set-buffer-modified-p nil)) + + (message "warnings=%s" (or (repro--warnings) "none"))) + + (delete-file file)) diff --git a/working/org-element-cache-persist-bug/verify-shipped-fix.el b/working/org-element-cache-persist-bug/verify-shipped-fix.el new file mode 100644 index 00000000..a8883085 --- /dev/null +++ b/working/org-element-cache-persist-bug/verify-shipped-fix.el @@ -0,0 +1,91 @@ +;;; repro-preview-cache.el --- Does dirvish's text preview disturb an org buffer's element cache? -*- lexical-binding: t -*- + +;; Replays `dirvish--preview-file-maybe-truncate' (dirvish.el:653-684) against an +;; org file that is ALSO open for real, then kills the preview buffer the way +;; dirvish does (`dirvish--kill-buffer', dirvish.el:416). The kill is the step +;; that matters: killing an org-mode buffer with a `buffer-file-name' is when +;; org-persist writes that file's cache. + +(require 'org) +(require 'org-element) +(require 'org-persist) +(add-to-list 'load-path (expand-file-name "modules" default-directory)) +(require 'dirvish-config) + +(setq org-element-use-cache t + org-element-cache-persistent t + org-element--cache-self-verify t + org-element--cache-self-verify-frequency 1.0) + +(defun repro--build-cache (buf) + "Actually populate the element cache of BUF by walking it." + (with-current-buffer buf + (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (and org-element--cache (avl-tree-size org-element--cache)))) + +(defun repro--warnings () + (when (get-buffer "*Warnings*") + (with-current-buffer "*Warnings*" (string-trim (buffer-string))))) + +(let* ((src (expand-file-name "todo.org" default-directory)) + (file (make-temp-file "repro-" nil ".org")) + (threshold 1048576)) + (copy-file src file t) + (let ((size (nth 7 (file-attributes file)))) + (message "== fixture %s (%d bytes, %s threshold) ==" + (file-name-nondirectory file) size + (if (>= size threshold) "over" "under"))) + + (let ((real (find-file-noselect file))) + (message "real buffer cache after full walk: %S" (repro--build-cache real)) + + ;; --- dirvish text preview of the same file --- + (with-current-buffer (get-buffer-create "*preview-temp*") + (let (jka-compr-verbose) + (with-silent-modifications + (setq buffer-read-only t) + (insert-file-contents file nil 0 threshold) + (when (>= (nth 7 (file-attributes file)) threshold) + (goto-char (point-max)) + (insert "\n\nFile truncated. End of partial preview.\n")) + (setq buffer-file-name file) + (goto-char (point-min)) + (rename-buffer (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (let ((enable-dir-local-variables nil) + (enable-local-variables :safe) + (non-essential t)) + (set-auto-mode) + (font-lock-mode 1)) + (run-hooks 'dirvish-preview-setup-hook)) + (message "preview buffer: mode=%s claims=%s cache=%S" + major-mode (file-name-nondirectory (or buffer-file-name "none")) + (repro--build-cache (current-buffer)))) + + (message "-- both buffers live: real=%S claiming-file=%d --" + (with-current-buffer real (avl-tree-size org-element--cache)) + (length (seq-filter (lambda (b) (equal (buffer-local-value 'buffer-file-name b) file)) + (buffer-list)))) + + ;; --- dirvish kills the preview buffer when the session ends --- + (let ((kill-buffer-query-functions nil) + (preview (format "PREVIEW :: %s" (file-name-nondirectory file)))) + (kill-buffer preview) + (message "preview buffer %S killed" preview)) + + ;; --- is the real buffer still coherent? --- + (with-current-buffer real + ;; Edit it the way Craig would after stepping back from dirvish. + (goto-char (point-max)) + (condition-case err + (let ((inhibit-read-only t)) (insert "\n* Repro edit\n")) + (error (message "RESULT: insert into real buffer SIGNALLED %S" (car err)))) + (condition-case err + (progn (org-element-cache-map (lambda (_el) nil) :granularity 'element) + (message "post-kill cache-map: clean (size=%S)" + (avl-tree-size org-element--cache))) + (error (message "post-kill cache-map: ERROR %S" err))) + (set-buffer-modified-p nil)) + + (message "warnings=%s" (or (repro--warnings) "none"))) + + (delete-file file)) |