fix: is_current stat guard, lazy Zotero utime only on success, docs (refs #615)
is_current() stat'd every source file bare, so a file removed between iterdir() and stat() aborted the whole extract run; a vanished source now just doesn't affect currency. The lazy Zotero snapshot stamped the snapshot with the source's mtime even when shutil.copy2 had failed, which would have passed a stale snapshot off as current on every later run. Copying moved into _copy_snapshot() so both callers know whether it worked. Docs: upsert_status spells out the merge rules (extra_json exempt from empty-keeps-stored, tags/collections union-merged) and `backfill-comments --force` says it bypasses the docket seal only.
This commit is contained in:
@@ -260,6 +260,17 @@ class Store:
|
|||||||
is written, no ``access_date``/``updated_at`` stamp, no tag row
|
is written, no ``access_date``/``updated_at`` stamp, no tag row
|
||||||
churn. A re-farm over a complete docket is therefore a read-only
|
churn. A re-farm over a complete docket is therefore a read-only
|
||||||
pass over ``items``.
|
pass over ``items``.
|
||||||
|
|
||||||
|
Merge rules, all of which make ``unchanged`` a comparison against
|
||||||
|
the *merged* row rather than the incoming one:
|
||||||
|
|
||||||
|
* columns: an empty incoming value keeps the stored one (a
|
||||||
|
list-walk row carries no body, and must not blank an enriched
|
||||||
|
abstract). ``extra_json`` is exempt — it is always a full
|
||||||
|
serialization, so an incoming ``{}`` is a real value and
|
||||||
|
replaces what is stored.
|
||||||
|
* tags and collections: union-merged with what is stored, never
|
||||||
|
replaced. Removal goes through :meth:`remove_tag`.
|
||||||
"""
|
"""
|
||||||
if tags:
|
if tags:
|
||||||
for tag in tags:
|
for tag in tags:
|
||||||
|
|||||||
@@ -391,7 +391,10 @@ def backfill_comments(
|
|||||||
force: bool = typer.Option(
|
force: bool = typer.Option(
|
||||||
False,
|
False,
|
||||||
"--force",
|
"--force",
|
||||||
help="Run even if the docket is sealed.",
|
help="Run even if the docket is sealed. Bypasses the docket seal "
|
||||||
|
"only: items already tagged enriched:ok (or enriched:gone) are "
|
||||||
|
"still skipped, so this resumes an interrupted enrichment — it "
|
||||||
|
"never re-enriches an item.",
|
||||||
),
|
),
|
||||||
) -> None:
|
) -> None:
|
||||||
"""Enrich every reg-gov comment stub with body + attachments.
|
"""Enrich every reg-gov comment stub with body + attachments.
|
||||||
|
|||||||
@@ -372,6 +372,17 @@ def _source_mtime_ns(src: Path) -> int:
|
|||||||
return newest
|
return newest
|
||||||
|
|
||||||
|
|
||||||
|
def _copy_snapshot(src: Path, snap: Path) -> bool:
|
||||||
|
"""Copy the Zotero database to *snap*. False (logged) on failure —
|
||||||
|
the caller must not treat whatever is at *snap* as fresh."""
|
||||||
|
try:
|
||||||
|
shutil.copy2(src, snap)
|
||||||
|
except OSError as e:
|
||||||
|
log.warning("zotero snapshot failed (%s) — no Zotero PDF fallback", e)
|
||||||
|
return False
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
class ZoteroPdfIndex:
|
class ZoteroPdfIndex:
|
||||||
"""bib/Zotero item key → storage PDFs, read from a *snapshot copy* of
|
"""bib/Zotero item key → storage PDFs, read from a *snapshot copy* of
|
||||||
zotero.sqlite (the live file is locked by the Zotero desktop and its
|
zotero.sqlite (the live file is locked by the Zotero desktop and its
|
||||||
@@ -399,13 +410,21 @@ class ZoteroPdfIndex:
|
|||||||
newest = _source_mtime_ns(src)
|
newest = _source_mtime_ns(src)
|
||||||
if snap.exists() and src.exists() and snap.stat().st_mtime_ns >= newest:
|
if snap.exists() and src.exists() and snap.stat().st_mtime_ns >= newest:
|
||||||
return cls._read(snap, Path(storage_dir))
|
return cls._read(snap, Path(storage_dir))
|
||||||
index = cls.snapshot(src, storage_dir, Path(tmp_dir))
|
if not src.exists():
|
||||||
if snap.exists() and newest:
|
log.warning("zotero db %s missing — no Zotero PDF fallback", src)
|
||||||
|
return {}
|
||||||
|
Path(tmp_dir).mkdir(parents=True, exist_ok=True)
|
||||||
|
if not _copy_snapshot(src, snap):
|
||||||
|
# Only a *successful* copy may be stamped: stamping the
|
||||||
|
# stale snapshot left behind by a failed one would pass
|
||||||
|
# it off as current on every future run.
|
||||||
|
return {}
|
||||||
|
if newest:
|
||||||
# copy2 carries the *main* file's mtime, which is older
|
# copy2 carries the *main* file's mtime, which is older
|
||||||
# than the WAL; stamp what we actually captured so the
|
# than the WAL; stamp what we actually captured so the
|
||||||
# next run doesn't re-copy 1.95 GB for nothing.
|
# next run doesn't re-copy 1.95 GB for nothing.
|
||||||
os.utime(snap, ns=(newest, newest))
|
os.utime(snap, ns=(newest, newest))
|
||||||
return index._by_key
|
return cls._read(snap, Path(storage_dir))
|
||||||
|
|
||||||
inst._loader = _load
|
inst._loader = _load
|
||||||
return inst
|
return inst
|
||||||
@@ -419,10 +438,7 @@ class ZoteroPdfIndex:
|
|||||||
return cls({})
|
return cls({})
|
||||||
tmp_dir.mkdir(parents=True, exist_ok=True)
|
tmp_dir.mkdir(parents=True, exist_ok=True)
|
||||||
snap = tmp_dir / "zotero.sqlite"
|
snap = tmp_dir / "zotero.sqlite"
|
||||||
try:
|
if not _copy_snapshot(Path(sqlite_path), snap):
|
||||||
shutil.copy2(sqlite_path, snap)
|
|
||||||
except OSError as e:
|
|
||||||
log.warning("zotero snapshot failed (%s) — no Zotero PDF fallback", e)
|
|
||||||
return cls({})
|
return cls({})
|
||||||
return cls(cls._read(snap, Path(storage_dir)))
|
return cls(cls._read(snap, Path(storage_dir)))
|
||||||
|
|
||||||
|
|||||||
@@ -143,7 +143,11 @@ def is_current(comment_dir: Path) -> bool:
|
|||||||
except FileNotFoundError:
|
except FileNotFoundError:
|
||||||
return False
|
return False
|
||||||
for p in source_paths(comment_dir):
|
for p in source_paths(comment_dir):
|
||||||
if p.stat().st_mtime_ns > out_m:
|
try:
|
||||||
|
newer = p.stat().st_mtime_ns > out_m
|
||||||
|
except FileNotFoundError:
|
||||||
|
continue # vanished between iterdir() and stat() — not our business
|
||||||
|
if newer:
|
||||||
return False
|
return False
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
|||||||
@@ -244,3 +244,33 @@ class TestLazyZotero:
|
|||||||
# doesn't re-copy 1.95 GB for the same unchanged WAL
|
# doesn't re-copy 1.95 GB for the same unchanged WAL
|
||||||
ZoteroPdfIndex.lazy(src, tmp_path / "storage", snap_dir).pdfs_for("ABCD1234")
|
ZoteroPdfIndex.lazy(src, tmp_path / "storage", snap_dir).pdfs_for("ABCD1234")
|
||||||
assert snap.stat().st_mtime_ns == m2
|
assert snap.stat().st_mtime_ns == m2
|
||||||
|
|
||||||
|
def test_failed_copy_does_not_stamp_the_stale_snapshot(self, tmp_path, monkeypatch):
|
||||||
|
"""Stamping after a failed copy would pass a stale snapshot off as
|
||||||
|
current forever."""
|
||||||
|
import shutil
|
||||||
|
import sqlite3
|
||||||
|
|
||||||
|
src = tmp_path / "zotero.sqlite"
|
||||||
|
con = sqlite3.connect(src)
|
||||||
|
con.executescript(
|
||||||
|
"CREATE TABLE items(itemID INTEGER, key TEXT); CREATE TABLE itemAttachments(itemID INTEGER, parentItemID INTEGER, path TEXT); CREATE TABLE deletedItems(itemID INTEGER);"
|
||||||
|
)
|
||||||
|
con.close()
|
||||||
|
snap_dir = tmp_path / "snap"
|
||||||
|
ZoteroPdfIndex.lazy(src, tmp_path / "storage", snap_dir).pdfs_for("ABCD1234")
|
||||||
|
snap = snap_dir / "zotero.sqlite"
|
||||||
|
m1 = snap.stat().st_mtime_ns
|
||||||
|
|
||||||
|
bump = m1 + 10**9
|
||||||
|
os.utime(src, ns=(bump, bump)) # source now newer → a copy is due
|
||||||
|
monkeypatch.setattr(
|
||||||
|
shutil, "copy2", lambda *a, **k: (_ for _ in ()).throw(OSError("no space"))
|
||||||
|
)
|
||||||
|
assert (
|
||||||
|
ZoteroPdfIndex.lazy(src, tmp_path / "storage", snap_dir).pdfs_for(
|
||||||
|
"ABCD1234"
|
||||||
|
)
|
||||||
|
== []
|
||||||
|
)
|
||||||
|
assert snap.stat().st_mtime_ns == m1 # untouched, so the next run retries
|
||||||
|
|||||||
@@ -115,3 +115,12 @@ def test_walker_skip_dockets(tmp_path: Path):
|
|||||||
)
|
)
|
||||||
assert stats == {"written": 0, "skipped": 0, "failed": 0, "skipped_sealed": 1}
|
assert stats == {"written": 0, "skipped": 0, "failed": 0, "skipped_sealed": 1}
|
||||||
assert not (d / "combined.md").exists()
|
assert not (d / "combined.md").exists()
|
||||||
|
|
||||||
|
|
||||||
|
def test_is_current_ignores_a_source_that_vanished(tmp_path: Path, monkeypatch):
|
||||||
|
"""A file removed between iterdir() and stat() must not kill the run."""
|
||||||
|
d = _dir(tmp_path)
|
||||||
|
(d / "combined.md").write_text("c")
|
||||||
|
ghost = d / "attachment_9.pdf"
|
||||||
|
monkeypatch.setattr("rex.comments.combine.source_paths", lambda _d: [ghost])
|
||||||
|
assert is_current(d) is True
|
||||||
|
|||||||
Reference in New Issue
Block a user