fix(background-tasks): atomic per-job hash storage to stop concurrent-load flicker
The per-user registry was a single pickled dict with read-modify-write, so concurrent loads clobbered each other's entries — phantom/flickering rows and stale 'done' bars in the widget. Store one HASH field per job_id (hset/hgetall/ hdel) so concurrent workers write independently; hgetall also reads straight from Redis (no frappe.local pin) for a consistent widget snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
15c626b007
commit
7d13af6f78
|
|
@ -5,18 +5,21 @@
|
||||||
# every long-running worker writes a progress snapshot here, the widget reads it
|
# every long-running worker writes a progress snapshot here, the widget reads it
|
||||||
# back and can request cancellation.
|
# back and can request cancellation.
|
||||||
#
|
#
|
||||||
|
# Storage: a per-user Redis HASH, one field per job_id. Each field is written/
|
||||||
|
# deleted independently (HSET/HDEL are atomic per field), so MULTIPLE concurrent
|
||||||
|
# jobs for the same user never clobber each other. (An earlier version stored the
|
||||||
|
# whole registry as one pickled dict and did read-modify-write, which raced under
|
||||||
|
# concurrent loads — phantom/flickering rows. The hash fixes that.)
|
||||||
|
#
|
||||||
# Design notes:
|
# Design notes:
|
||||||
# * Keyed by USER. `get_active_background_tasks()` / `cancel_background_task()`
|
# * Keyed by USER. `get_active_background_tasks()` / `cancel_background_task()`
|
||||||
# derive the user from `frappe.session.user` — the client never passes a user,
|
# derive the user from `frappe.session.user` — the client never passes a user,
|
||||||
# so one user can neither see nor cancel another user's jobs.
|
# so one user can neither see nor cancel another user's jobs.
|
||||||
# * The registry is the source of truth (survives page reloads). Realtime pings
|
# * `hgetall` reads straight from Redis (no frappe.local cache), so the widget
|
||||||
# are only a "something changed, re-fetch" hint — anything published before the
|
# always sees a fresh, consistent snapshot. The registry is the source of
|
||||||
# client subscribes is lost, so the widget ALSO polls this on a timer.
|
# truth (survives page reloads); realtime pings are just "re-fetch" hints.
|
||||||
# * Cancellation is best-effort: the worker checks `is_cancel_requested()`
|
# * Cancellation is best-effort: the worker checks `is_cancel_requested()`
|
||||||
# between steps and bails out after finishing the current step.
|
# between steps and bails out after finishing the current step.
|
||||||
# * Generalising this to the other bulk imports (sales/purchase/vat/amas) is
|
|
||||||
# ~10 lines each: new_job_id() -> register_task -> update_task ->
|
|
||||||
# is_cancel_requested -> finish_task.
|
|
||||||
|
|
||||||
import uuid
|
import uuid
|
||||||
|
|
||||||
|
|
@ -24,7 +27,7 @@ import frappe
|
||||||
|
|
||||||
REALTIME_EVENT = "background_tasks_changed"
|
REALTIME_EVENT = "background_tasks_changed"
|
||||||
|
|
||||||
_REGISTRY_TTL = 7200 # the per-user registry key self-expires after 2h
|
_REGISTRY_TTL = 7200 # the per-user hash self-expires after 2h (crash safety net)
|
||||||
_CANCEL_TTL = 3600
|
_CANCEL_TTL = 3600
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -38,12 +41,32 @@ def _cancel_key(job_id):
|
||||||
return f"invoice_az:bg_cancel:{job_id}"
|
return f"invoice_az:bg_cancel:{job_id}"
|
||||||
|
|
||||||
|
|
||||||
def _read(user):
|
def _touch_ttl(user):
|
||||||
return frappe.cache().get_value(_registry_key(user)) or {}
|
"""Keep the per-user hash key alive while jobs are running."""
|
||||||
|
try:
|
||||||
|
cache = frappe.cache()
|
||||||
|
cache.expire(cache.make_key(_registry_key(user)), _REGISTRY_TTL)
|
||||||
|
except Exception:
|
||||||
|
pass # TTL is only a crash safety net; finish_task removes tasks normally
|
||||||
|
|
||||||
|
|
||||||
def _write(user, tasks):
|
def _all_tasks(user):
|
||||||
frappe.cache().set_value(_registry_key(user), tasks, expires_in_sec=_REGISTRY_TTL)
|
"""Fresh-from-Redis list of the user's task snapshots (hgetall bypasses
|
||||||
|
frappe.local cache)."""
|
||||||
|
return list((frappe.cache().hgetall(_registry_key(user)) or {}).values())
|
||||||
|
|
||||||
|
|
||||||
|
def _get_task(user, job_id):
|
||||||
|
return frappe.cache().hget(_registry_key(user), job_id)
|
||||||
|
|
||||||
|
|
||||||
|
def _put_task(user, job_id, task):
|
||||||
|
frappe.cache().hset(_registry_key(user), job_id, task)
|
||||||
|
_touch_ttl(user)
|
||||||
|
|
||||||
|
|
||||||
|
def _del_task(user, job_id):
|
||||||
|
frappe.cache().hdel(_registry_key(user), job_id)
|
||||||
|
|
||||||
|
|
||||||
def _ping(user, payload=None):
|
def _ping(user, payload=None):
|
||||||
|
|
@ -62,8 +85,7 @@ def register_task(job_id, user, feature, label, total=0, cancellable=True):
|
||||||
"""Add a task to the user's registry in the `running` state."""
|
"""Add a task to the user's registry in the `running` state."""
|
||||||
if not job_id:
|
if not job_id:
|
||||||
return
|
return
|
||||||
tasks = _read(user)
|
_put_task(user, job_id, {
|
||||||
tasks[job_id] = {
|
|
||||||
"job_id": job_id,
|
"job_id": job_id,
|
||||||
"feature": feature,
|
"feature": feature,
|
||||||
"label": label,
|
"label": label,
|
||||||
|
|
@ -73,8 +95,7 @@ def register_task(job_id, user, feature, label, total=0, cancellable=True):
|
||||||
"status": "running", # running | cancelling | done | error | cancelled
|
"status": "running", # running | cancelling | done | error | cancelled
|
||||||
"cancellable": bool(cancellable),
|
"cancellable": bool(cancellable),
|
||||||
"message": "",
|
"message": "",
|
||||||
}
|
})
|
||||||
_write(user, tasks)
|
|
||||||
_ping(user)
|
_ping(user)
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -83,8 +104,7 @@ def update_task(job_id, user, current=None, total=None, percent=None, message=No
|
||||||
current/total unless an explicit `percent` is given."""
|
current/total unless an explicit `percent` is given."""
|
||||||
if not job_id:
|
if not job_id:
|
||||||
return
|
return
|
||||||
tasks = _read(user)
|
task = _get_task(user, job_id)
|
||||||
task = tasks.get(job_id)
|
|
||||||
if not task:
|
if not task:
|
||||||
return
|
return
|
||||||
if total is not None:
|
if total is not None:
|
||||||
|
|
@ -97,8 +117,7 @@ def update_task(job_id, user, current=None, total=None, percent=None, message=No
|
||||||
task["percent"] = max(0, min(100, int(percent)))
|
task["percent"] = max(0, min(100, int(percent)))
|
||||||
elif task.get("total"):
|
elif task.get("total"):
|
||||||
task["percent"] = max(0, min(100, int(round(task["current"] / task["total"] * 100))))
|
task["percent"] = max(0, min(100, int(round(task["current"] / task["total"] * 100))))
|
||||||
tasks[job_id] = task
|
_put_task(user, job_id, task)
|
||||||
_write(user, tasks)
|
|
||||||
_ping(user)
|
_ping(user)
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -106,20 +125,20 @@ def finish_task(job_id, user, status="done", message=None):
|
||||||
"""Remove the task from the registry and flash its final state to the widget.
|
"""Remove the task from the registry and flash its final state to the widget.
|
||||||
|
|
||||||
Removing it immediately keeps the poll-based fallback consistent: a task that
|
Removing it immediately keeps the poll-based fallback consistent: a task that
|
||||||
is gone from the registry is simply gone from the widget's list, so a lost
|
is gone from the registry is simply gone from the widget's list."""
|
||||||
realtime ping can't leave a stale "running" row behind."""
|
|
||||||
if not job_id:
|
if not job_id:
|
||||||
return
|
return
|
||||||
tasks = _read(user)
|
task = _get_task(user, job_id)
|
||||||
task = tasks.pop(job_id, None)
|
_del_task(user, job_id)
|
||||||
_write(user, tasks)
|
|
||||||
frappe.cache().delete_value(_cancel_key(job_id))
|
frappe.cache().delete_value(_cancel_key(job_id))
|
||||||
if task:
|
if task:
|
||||||
task["status"] = status
|
task["status"] = status
|
||||||
if message is not None:
|
|
||||||
task["message"] = message
|
|
||||||
if status == "done":
|
if status == "done":
|
||||||
task["percent"] = 100
|
task["percent"] = 100
|
||||||
|
task["current"] = task.get("total") or task.get("current") or 0
|
||||||
|
task["message"] = message if message is not None else "Done"
|
||||||
|
elif message is not None:
|
||||||
|
task["message"] = message
|
||||||
_ping(user, {"finished": task})
|
_ping(user, {"finished": task})
|
||||||
else:
|
else:
|
||||||
_ping(user)
|
_ping(user)
|
||||||
|
|
@ -128,15 +147,13 @@ def finish_task(job_id, user, status="done", message=None):
|
||||||
def request_cancel(job_id, user):
|
def request_cancel(job_id, user):
|
||||||
"""Set the cancel flag for a task owned by `user`. Returns False if the task
|
"""Set the cancel flag for a task owned by `user`. Returns False if the task
|
||||||
is not in this user's registry (so cross-user cancellation is impossible)."""
|
is not in this user's registry (so cross-user cancellation is impossible)."""
|
||||||
tasks = _read(user)
|
task = _get_task(user, job_id)
|
||||||
task = tasks.get(job_id)
|
|
||||||
if not task:
|
if not task:
|
||||||
return False
|
return False
|
||||||
frappe.cache().set_value(_cancel_key(job_id), 1, expires_in_sec=_CANCEL_TTL)
|
frappe.cache().set_value(_cancel_key(job_id), 1, expires_in_sec=_CANCEL_TTL)
|
||||||
task["status"] = "cancelling"
|
task["status"] = "cancelling"
|
||||||
task["message"] = "Cancellation requested"
|
task["message"] = "Cancellation requested"
|
||||||
tasks[job_id] = task
|
_put_task(user, job_id, task)
|
||||||
_write(user, tasks)
|
|
||||||
_ping(user)
|
_ping(user)
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
@ -158,7 +175,7 @@ def is_cancel_requested(job_id):
|
||||||
@frappe.whitelist()
|
@frappe.whitelist()
|
||||||
def get_active_background_tasks():
|
def get_active_background_tasks():
|
||||||
"""Return the current user's in-flight tasks. Source of truth for the widget."""
|
"""Return the current user's in-flight tasks. Source of truth for the widget."""
|
||||||
return list(_read(frappe.session.user).values())
|
return _all_tasks(frappe.session.user)
|
||||||
|
|
||||||
|
|
||||||
@frappe.whitelist()
|
@frappe.whitelist()
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue