Skip to content

fix: image occlusion masks lost when adding notes#5155

Open
LTimothy wants to merge 2 commits into
ankitects:mainfrom
LTimothy:fix/io-occlusion-add-races
Open

fix: image occlusion masks lost when adding notes#5155
LTimothy wants to merge 2 commits into
ankitects:mainfrom
LTimothy:fix/io-occlusion-add-races

Conversation

@LTimothy

@LTimothy LTimothy commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Linked issue (required)

Closes #4754

Summary / motivation (required)

Image occlusion masks could silently go missing from a note at add time. Two separate problems caused this, both timing dependent, which is why the bug was rare and hard to reproduce.

Problem 1: the editor reset races the add (desktop). Clicking Add on an image occlusion note saved the note, queued the actual add, and reset the mask editor all at once. The reset clears the canvas, and clearing the canvas writes an empty value into the Occlusions field of the same note object the add was about to read. Depending on which message arrived first: the add worked normally (the common case), the note was added with no masks and no warning, or the add was wrongly rejected with "no occlusion created" while the masks were plainly visible. Fix: reset the mask editor only after the add succeeds, matching the ordering the non-legacy TS editor page already uses. This also means a rejected add no longer wipes the masks you drew.

Problem 2: one field's save cancels another's. All fields committed through a single shared save timer, and scheduling a save cancelled the timer's previous pending action. Mask edits commit the Occlusions field through that timer with a 600 ms debounce, so typing into Header or Back Extra within 600 ms of the last mask edit threw away the pending occlusions write. It was never retried, because later saves saw the store already held the same value and skipped notifying. Every mask drawn since the last committed write was then missing from the added note. Fix: give each field its own timer. This also surfaced a second bug in ChangeTimer itself: an action scheduled while fireImmediately() was awaiting a previous action was silently discarded when that await finished; fixed by taking the action before running it and letting a concurrent flush wait for the run in flight.

Steps to reproduce (required, use N/A if not applicable)

Problem 2 (reliably reproducible):

  1. Add an Image Occlusion note, draw a mask, wait for it to auto-commit (~1s).
  2. Draw a second mask.
  3. Immediately (within ~600 ms) switch to the fields view and type a character into Header.
  4. Wait a second, click Add.
  5. Before this fix: the added note is missing the second mask. No warning is shown.

Problem 1 (event-loop timing dependent, not reliably reproducible by hand):

  1. Add an Image Occlusion note, paste an image, draw one or more masks.
  2. Click Add.
  3. Before this fix: rarely, the note is added with an empty Occlusions field and no warning, or Add is rejected with "no occlusion created" while masks are visible on screen.

How to test (required)

Checklist (minimum)

  • I ran ./ninja check or an equivalent relevant check locally.
  • I added or updated tests when the change is non-trivial or behavior changed.

Details

  • ts/tests/e2e/io-mask-save-race.spec.ts reproduces problem 2 with real mouse drawing and real keystrokes at the real 600 ms timing. Fails on current main (payload contains 1 cloze instead of 2, consistently across repeated runs) and passes with this change.
  • ts/lib/editable/change-timer.test.ts covers the discarded-action case in ChangeTimer and fails without the fix.
  • Full Playwright e2e suite (just test-e2e) passes repeatedly across fresh profiles.
  • just fmt, just lint, and the vitest suite are clean.
  • Problem 1 has no reliable automated test: the outcome depends on how the Qt event loop interleaves two webChannel messages and a background task. I verified the mechanism with a scripted headless Anki that drives the real AddCards flow and delivers the racing write inside the vulnerable window, and confirmed by inspection that after this change the empty write can no longer be issued while an add is in flight.

Before / after behavior (optional)

Before: masks could be silently dropped from a newly added note (problem 2, common), or the note could be added completely empty of masks / spuriously rejected (problem 1, rare). After: neither race is possible; a rejected add also no longer clears the masks you've drawn, so you can fix the note and retry instead of redrawing.

Risk / compatibility / migration (optional)

Low risk. Both fixes are ordering/scoping changes with no behavior change outside the affected race windows. No schema, API, or migration changes.

UI evidence (required for visual changes; otherwise N/A)

N/A (no visual/UI changes).

Scope

  • This PR is focused on one change (no unrelated edits).

This PR contains two fixes rather than one. I considered splitting them, but both are root causes of the same reported bug (#4754) discovered during the same investigation, they are already isolated into two separate commits for review, and neither is a complete fix for the issue on its own. Happy to split into two PRs if you'd prefer.

@LTimothy LTimothy changed the title Fix image occlusion masks being lost when adding notes (#4754) fix: image occlusion masks lost when adding notes Jul 14, 2026
@LTimothy

LTimothy commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

I suspect this explains why some users on this thread hit this more reliably than others: a race condition between the mask editor's reset, which fires the moment Add is clicked, and the background save of the note. The script below reproduces it more often by injecting a small delay into that save; on my setup, before this fix it went from 0 or 1 empty out of 20 with no delay to 20/20 with a 20ms delay, and after this fix it stayed 0/20 either way. Suggests the trigger is whatever slows a machine down, not the user. The proposed PR removes the reset's ability to run before the note is saved, eliminating the race condition.

"""
Demo for #4754.

1. Save as <profile-folder>/addons21/io_4754_demo/__init__.py in a throwaway profile.
2. Launch Anki. It runs on its own once the profile loads, no clicking
   needed, a dialog reports the result when it's done.
3. Change DELAY_MS below and relaunch to compare.
"""

from pathlib import Path

import aqt
import aqt.editor
from anki.models import NotetypeId
from aqt import gui_hooks, mw
from aqt.qt import QMessageBox, QTimer

DELAY_MS = 0
N = 20
IMAGE = str(Path(aqt.editor.__file__).parent / "data/web/imgs/anki-logo-thin.png")

state = {"added": 0, "note_ids": []}


def slow_down_saves() -> None:
    if DELAY_MS <= 0:
        return
    orig = mw.taskman.run_in_background

    def delayed(task, on_done=None, args=None, uses_collection=True):
        QTimer.singleShot(
            DELAY_MS,
            lambda: orig(task, on_done, args=args, uses_collection=uses_collection),
        )

    mw.taskman.run_in_background = delayed


def start() -> None:
    QTimer.singleShot(2000, step_open)


def step_open() -> None:
    mw.col.add_image_occlusion_notetype()
    ntid = mw.col.models.id_for_name("Image Occlusion")
    state["addcards"] = aqt.dialogs.open("AddCards", mw)
    state["addcards"].notetype_chooser.selected_notetype_id = NotetypeId(ntid)
    state["ntid"] = ntid
    slow_down_saves()
    gui_hooks.add_cards_did_add_note.append(on_added)
    QTimer.singleShot(500, load_image)


def load_image() -> None:
    addcards = state["addcards"]
    addcards.editor.setup_mask_editor_for_new_note(IMAGE, state["ntid"])
    poll_canvas_ready(0)


def poll_canvas_ready(attempt: int) -> None:
    addcards = state["addcards"]

    def on_result(res):
        if res:
            draw_and_add()
        elif attempt < 30:
            QTimer.singleShot(200, lambda: poll_canvas_ready(attempt + 1))

    addcards.editor.web.evalWithCallback(
        "!!globalThis.canvas && globalThis.canvas.getObjects().length >= 1",
        on_result,
    )


DRAW_AND_SAVE_JS = """
(function() {
    const canvas = globalThis.canvas;
    const rect = new fabric.Rect({
        left: 20, top: 20, width: 40, height: 30,
        fill: "rgba(0,0,0,0.4)",
    });
    rect.id = "rect-" + Date.now();
    canvas.add(rect);
    const tmp = new fabric.Rect({left: -100, top: -100, width: 1, height: 1});
    canvas.add(tmp);
    canvas.remove(tmp);
})();
"""


def draw_and_add() -> None:
    addcards = state["addcards"]
    addcards.editor.web.eval(DRAW_AND_SAVE_JS)
    QTimer.singleShot(0, addcards.add_current_note)


def on_added(note) -> None:
    state["added"] += 1
    state["note_ids"].append(note.id)
    if state["added"] >= N:
        QTimer.singleShot(1000, check)
    else:
        load_image()


def check() -> None:
    empty = 0
    for nid in state["note_ids"]:
        if mw.col.get_note(nid).fields[0].strip() == "":
            empty += 1
    summary = f"DELAY_MS={DELAY_MS}: {empty}/{N} notes came out with an empty Occlusions field."
    print(f"[io-4754-demo] {summary}")
    QMessageBox.information(mw, "io-4754-demo result", summary)


gui_hooks.profile_did_open.append(start)

LTimothy added 2 commits July 14, 2026 20:10
reset_image_occlusion() was queued immediately after the save that
precedes _add_current_note(), so the reset's empty occlusions write
raced the background add of the same Note object. Depending on event
timing the note could be added with an empty Occlusions field and no
warning, or the add could be spuriously rejected with 'no occlusion
created' while masks were visibly present (ankitects#4754).

Resetting only after a successful add matches the ordering the
non-legacy TS path already uses, and means a rejected add no longer
wipes the drawn masks.
Field content was committed through a single shared ChangeTimer, so
scheduling one field's save silently cancelled any other field's
pending save. For image occlusion notes this dropped the debounced
Occlusions write when the user typed into another field within 600ms
of the last mask edit, and later saveOcclusions() calls never
re-scheduled it because the store already held the same value: masks
drawn since the last commit were missing from the added note (ankitects#4754).

Use one timer per field, and make ChangeTimer.fireImmediately() take
the action before running it, as an action scheduled while a flush was
in flight used to be discarded when the flush finished.
@LTimothy
LTimothy force-pushed the fix/io-occlusion-add-races branch from b6d4f28 to 81912b6 Compare July 15, 2026 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IO masks occasionally fail to save

1 participant