rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-10-04 14:25:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(memory): join an in-flight fetch instead of starting a duplicate (#503)

Browse Source 
* fix(memory): join an in-flight fetch instead of starting a duplicate

submit_fetch_request() had no notion of "already fetching this". request_id
embeds a millisecond timestamp, so every submit looked new, and
active_requests is keyed by that id rather than by what is being fetched.
Two submits for the same cache_key therefore started two identical fetches.

It is not a rare race. _fetch_data in the sports managers branches: the Live
manager fetches only today's games, but Recent and Upcoming both pull the
full season schedule under the SAME cache_key. On a cache miss both miss,
both submit, and nothing stops the second. On a running 512x64 board:

    138 background fetches in 24 hours, arriving in pairs at identical
    millisecond timestamps, roughly hourly:

      2  2026-08-25 11:47:26.612
      2  2026-08-25 10:46:28.064
      2  2026-08-25 08:01:55.962

Half of them redundant. Each duplicate costs a second download, a second
JSON parse -- the expensive part on a Pi -- and a second parsed copy
resident at the same time. Schedules on that board run 256KB to 20MB, 106MB
across all sports. Because the pairs land in the same millisecond they also
occupy two of the three executor slots with identical work, which is what
makes two large parses peak simultaneously.

A submit for a cache_key already in flight now joins that request: its
callback is added to the existing one and the existing request_id is
returned, so get_result() works for both. Different keys are untouched, and
dedupe applies only while a fetch is in flight -- a submit after completion
fetches again, because this is not a second cache layer.

Three details:

  - The in-flight entry is dropped and the callback list snapshotted in the
    SAME critical section as filing the result. Otherwise a submitter could
    join a fetch whose callbacks had already run and never be called back.

  - Cancellation is the other way a request leaves active_requests, so it
    releases the key too. And the join path looks the request up rather than
    trusting the id, so an entry stranded any other way cannot wedge a key
    permanently -- it is dropped and a fresh fetch starts.

  - One callback raising no longer prevents the others being delivered.
    Previously there was only ever one.

Interaction with #499, whichever merges second: that PR releases the payload
after the callback runs. With several callbacks the release must happen
after ALL of them, and must not happen at all if a joined submitter passed
no callback, since polling get_result() would then be its only delivery
path. The callback list built here is the hook for that.

test_background_fetch_dedupe.py -- 8 tests, covering the join, callback
delivery to both submitters, one callback raising, distinct keys not being
coalesced, a post-completion submit fetching again, cancellation releasing
the key, a stranded entry not wedging one, and the reported count. Verified
non-vacuous by removing only the join branch: 3 fail. The callback tests
assert the ids coalesced, without which they would pass trivially on two
independent requests.

Full suite: 3698 passed, 60 skipped, 1 failure that reproduces identically
on unmodified main (test_install_lowmem, environment-dependent: /var/tmp is
disk-backed on this machine).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

* fix(memory): discard a cancelled fetch instead of letting it commit

Review follow-up on the dedupe.

Cancelling releases the cache_key, so a replacement fetch for that key can
start immediately. But _fetch_data_worker() had no cancellation check: the
cancelled worker still wrote its response to the cache, flipped its own
status from CANCELLED to COMPLETED, and ran its callbacks. The stale
response could therefore land on top of the replacement's fresher data.

The worker cannot abort an HTTP call in flight, so the response is discarded
on return instead: no cache write, no callbacks, status left CANCELLED. The
check sits immediately before the cache write, which is the first
side effect.

Also fixed, found by the new test rather than by reading:

    request_id was f"{sport}_{year}_{milliseconds}", which is not unique.
    Two submits inside the same millisecond produced the SAME id -- the
    test's two sequential fetches collided on a fast mocked response, and
    one request silently replaced the other in active_requests and
    completed_requests. Rare before this PR; load-bearing now, because
    dedupe hands that id back to every joiner as their handle for
    get_result(). A per-service counter is appended.

Two test problems of my own, both fixed here rather than left to flake:

  - The cancellation test synchronised with time.sleep(0.4). A slow worker
    would have made it pass for the wrong reason. It now waits for the
    request to be filed in completed_requests.
  - The id-uniqueness test patched session.get, but submits are async: the
    50 workers outlived the patch and made real DNS calls to the dummy host.
    It stubs the executor instead, which is what a submit-time test should
    exercise.

20 consecutive runs of the dedupe file: 0 failures. Full suite: 3700 passed,
60 skipped, 1 failure that reproduces identically on unmodified main
(test_install_lowmem, environment-dependent).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

* fix(memory): make cancellation terminal, not advisory

Three paths wrote request.status without checking whether the request had
already been cancelled, so a cancel could be silently undone and the work it
was meant to stop went ahead anyway.

- A request cancelled while queued had CANCELLED overwritten with IN_PROGRESS
  the moment its worker started, defeating the discard check entirely: it
  downloaded, cached and called back for work the caller had withdrawn. It
  now skips the fetch outright, which is also the cheapest possible cancel.
- The cancelled-check and the cache write were separate critical sections, so
  a cancel landing between them left the payload in the cache with the
  callbacks suppressed -- every submitter that joined the fetch waited for a
  call that never came. The worker now claims the commit in the same critical
  section that reads the status, and cancel_request refuses once claimed. The
  write stays outside the lock: it serialises a multi-megabyte payload to the
  SD card, and holding the service lock across that would stall every submit,
  status query and cancel behind it.
- A cancelled request that then failed was relabelled FAILED, which slipped
  past the CANCELLED-only callback gate and delivered a spurious error
  callback. The except path now leaves CANCELLED alone.

get_request_status() also reported a cancelled request as FAILED, since it
inferred status from result.success; the final status is now recorded on the
result. Both early returns assign to `result` so completed_requests files the
outcome that was reported rather than the untouched placeholder.

Tests cover cancellation before worker start, during the commit, and during an
HTTP failure; all three fail against the unfixed code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

* test(memory): reach the exception path as a cancelled request

test_a_failure_after_cancelling_stays_cancelled cancelled the request while
its worker was still queued, so once the pre-start branch landed the worker
returned there and never reached the exception handler the test is named for.
It passed against the unfixed code only because that branch did not exist yet;
with it, the test passed for the wrong reason and reverting the except-path
guard did not fail it.

Cancel while the worker is parked inside the HTTP call instead, and assert the
fetch actually started so the test cannot silently degrade into the pre-start
case again. Reverting each of the three guards now fails exactly one test.

Also read the payload inside the callback rather than off the FetchResult
afterwards: #499 releases result.data once delivery is done, so the later read
saw the released object and not what the caller was handed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-08-26 10:03:43 -04:00
committed by GitHub
co-authored by Claude Opus 5
parent 5e5979973e
commit af96bd5cb0
2 changed files with 605 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/background_data_service.py
+155 -7
View File
@@ -14,11 +14,12 @@ Key Features:
- Memory-efficient data storage
"""
import itertools
import time
import logging
import threading
import requests
from typing import Dict, Any, Optional, Callable
from typing import Dict, Any, Optional, Callable, List
from dataclasses import dataclass, field
from enum import Enum
import queue
@@ -50,8 +51,19 @@ class FetchRequest:
max_retries: int = 3
priority: int = 1 # Higher number = higher priority
callback: Optional[Callable] = None
# Callbacks from submitters that JOINED this fetch instead of starting a
# duplicate one. The primary `callback` above belongs to whoever created
# the request; these belong to everyone who asked for the same cache_key
# while it was still in flight.
extra_callbacks: List[Callable] = field(default_factory=list)
created_at: float = field(default_factory=time.time)
status: FetchStatus = FetchStatus.PENDING
# Set once the worker has decided this response will be cached, while it
# still holds the lock. From that point cancelling is refused: the write
# is already authorised, and abandoning it here would put the payload in
# the cache with the callbacks suppressed -- joiners waiting forever for a
# fetch that did, in fact, succeed.
commit_claimed: bool = False
result: Optional[Any] = None
error: Optional[str] = None
@@ -74,6 +86,10 @@ class FetchResult:
fetch_time: float = 0.0
retry_count: int = 0
completed_at: float = field(default_factory=time.time) # Timestamp when request completed
# The request's final status, recorded so a finished request can still be
# reported accurately. Without it a caller can only be told COMPLETED or
# FAILED, which turns "you cancelled this" into "this errored".
final_status: Optional[FetchStatus] = None
class BackgroundDataService:
"""
@@ -98,6 +114,20 @@ class BackgroundDataService:
# Thread management
self.executor = ThreadPoolExecutor(max_workers=max_workers, thread_name_prefix="BackgroundData")
# cache_key -> request_id for fetches currently in flight. Submitting
# the same key twice used to start two identical fetches: request_id
# carries a millisecond timestamp, so every submit looked new, and
# active_requests is keyed by it rather than by what is being fetched.
# On a real board the season-schedule key is requested by both the
# Recent and the Upcoming manager, which miss the cache in the same
# millisecond and each download and parse the same payload.
self._inflight_by_cache_key: Dict[str, str] = {}
# request_id was sport_year_milliseconds, which is not unique: two
# submits inside the same millisecond produced the SAME id, so one
# silently replaced the other in active_requests and completed_requests.
# Rare before, but dedupe hands this id back to every joiner as their
# handle for get_result(), so it has to be unique. A counter is enough.
self._request_seq = itertools.count()
self.active_requests: Dict[str, FetchRequest] = {}
self.completed_requests: Dict[str, FetchResult] = {}
self.request_queue = queue.PriorityQueue()
@@ -185,7 +215,9 @@ class BackgroundDataService:
if cache_key is None:
cache_key = self.get_sport_cache_key(sport)
request_id = f"{sport}_{year}_{int(time.time() * 1000)}"
with self._lock:
request_id = (f"{sport}_{year}_{int(time.time() * 1000)}"
f"_{next(self._request_seq)}")
# Check cache first
cached_data = self.cache_manager.get(cache_key)
@@ -231,7 +263,29 @@ class BackgroundDataService:
)
with self._lock:
existing_id = self._inflight_by_cache_key.get(cache_key)
existing = self.active_requests.get(existing_id) if existing_id else None
if existing_id and existing is None:
# Stranded index entry: the request it names is gone. Drop it and
# fetch normally. Looking the request up rather than trusting the
# id is what stops a stale entry wedging a key forever.
del self._inflight_by_cache_key[cache_key]
if existing is not None:
# Someone is already fetching this key. Ride along rather than
# duplicating the download, the parse and the resident copy.
if callback:
existing.extra_callbacks.append(callback)
self.stats['deduplicated_requests'] = (
self.stats.get('deduplicated_requests', 0) + 1
)
logger.info(
"Joined in-flight fetch %s for %s (cache_key=%s) instead of "
"starting a duplicate", existing_id, sport, cache_key
)
return existing_id
self.active_requests[request_id] = request
self._inflight_by_cache_key[cache_key] = request_id
self.stats['total_requests'] += 1
self.stats['cache_misses'] += 1
@@ -256,7 +310,31 @@ class BackgroundDataService:
try:
with self._lock:
request.status = FetchStatus.IN_PROGRESS
# A request cancelled while it sat in the executor queue must
# stay cancelled. Overwriting the status here undid the cancel
# outright: the worker went on to download, cache and call back
# for work the caller had already withdrawn.
if request.status == FetchStatus.CANCELLED:
cancelled_before_start = True
else:
cancelled_before_start = False
request.status = FetchStatus.IN_PROGRESS
if cancelled_before_start:
logger.info(
"Request %s was cancelled before its worker started; "
"skipping the fetch entirely", request.id
)
# Assign before returning: the finally block stores `result`
# in completed_requests, so building a fresh one here would
# file the untouched placeholder instead of this outcome.
result = FetchResult(
request_id=request.id,
success=False,
error="cancelled",
fetch_time=time.time() - start_time,
retry_count=request.retry_count
)
return result
logger.info(f"Starting background fetch for {request.sport} {request.year}")
@@ -282,6 +360,37 @@ class BackgroundDataService:
# Log data validation
logger.debug(f"Validated {len(events)} events for {request.sport} {request.year}")
# A cancelled request must not commit anything. Cancelling
# releases the cache_key, so a replacement fetch for the same key
# may already be in flight or finished -- writing this response to
# the cache now would overwrite fresher data with the response
# nobody wanted. The worker has no way to abort the HTTP call, so
# this is where the work gets discarded.
with self._lock:
cancelled = request.status == FetchStatus.CANCELLED
if not cancelled:
# Claim the commit in the same critical section that read
# the status, so a cancel cannot slip in between the check
# and the cache write below. The write itself stays outside
# the lock: it serialises a multi-megabyte payload to the
# SD card, and holding the service lock across that would
# stall every submit, status query and cancel behind it.
request.commit_claimed = True
if cancelled:
logger.info(
"Discarding response for cancelled request %s; %s may "
"already belong to a replacement fetch",
request.id, request.cache_key
)
result = FetchResult(
request_id=request.id,
success=False,
error="cancelled",
fetch_time=time.time() - start_time,
retry_count=request.retry_count
)
return result
# Cache the data
self.cache_manager.set(request.cache_key, data)
@@ -307,7 +416,12 @@ class BackgroundDataService:
logger.error(f"Failed to fetch {request.sport} {request.year} data: {error_msg}")
with self._lock:
request.status = FetchStatus.FAILED
# Don't relabel a cancelled request. The callback gate in the
# finally block only suppresses CANCELLED, so promoting it to
# FAILED here delivered an error callback for a fetch nobody
# was waiting on any more.
if request.status != FetchStatus.CANCELLED:
request.status = FetchStatus.FAILED
request.error = error_msg
result = FetchResult(
@@ -321,9 +435,26 @@ class BackgroundDataService:
finally:
# Store result and clean up
with self._lock:
result.final_status = request.status
self.completed_requests[request.id] = result
if request.id in self.active_requests:
del self.active_requests[request.id]
# Stop accepting joiners and take the callback list in the same
# critical section. A submitter that arrives after this point
# finds no in-flight entry and either hits the cache (written
# above, before the result was built) or starts a fresh fetch --
# what it must never do is join a fetch whose callbacks have
# already run and then never be called.
if self._inflight_by_cache_key.get(request.cache_key) == request.id:
del self._inflight_by_cache_key[request.cache_key]
# A cancelled request delivers nothing: its joiners were told
# about a fetch that has been abandoned, and a replacement will
# call them via its own request.
if request.status == FetchStatus.CANCELLED:
callbacks = []
else:
callbacks = ([request.callback] if request.callback else [])
callbacks.extend(request.extra_callbacks)
# Update statistics
if result.success:
@@ -340,10 +471,11 @@ class BackgroundDataService:
# Periodic cleanup after storing result
self._cleanup_completed_requests()
# Call callback if provided
if request.callback:
# Call every callback: the original submitter's and any that joined
# this fetch. One raising must not stop the others being delivered.
for cb in callbacks:
try:
request.callback(result)
cb(result)
except Exception as e:
logger.error(f"Error in callback for request {request.id}: {e}")
# Delivered. Drop both references -- they point at the same
@@ -456,6 +588,8 @@ class BackgroundDataService:
return self.active_requests[request_id].status
elif request_id in self.completed_requests:
result = self.completed_requests[request_id]
if result.final_status is not None:
return result.final_status
return FetchStatus.COMPLETED if result.success else FetchStatus.FAILED
return None
@@ -472,8 +606,22 @@ class BackgroundDataService:
with self._lock:
if request_id in self.active_requests:
request = self.active_requests[request_id]
if request.commit_claimed:
# Too late: the worker holds an authorised commit. Report
# the failure rather than half-cancelling a request whose
# data is about to land in the cache.
logger.debug(
"Not cancelling %s: its response is already being "
"committed", request_id
)
return False
request.status = FetchStatus.CANCELLED
del self.active_requests[request_id]
# Cancelling is the other way a request leaves active_requests,
# so the in-flight index has to be released here too or the key
# stays pointed at a request that no longer exists.
if self._inflight_by_cache_key.get(request.cache_key) == request_id:
del self._inflight_by_cache_key[request.cache_key]
logger.info(f"Cancelled request {request_id}")
return True
return False