Skip to content

Commit b07ed3a

Browse files
committed
fix(update,db): four fixes the 2.2.0 beta and its rollback depend on
2.1.4 is the version anyone testing the 2.2.0 beta will roll back TO, so the protections have to be here rather than in a later patch. Each fix has a test that was verified to fail without it. Pre-release version ordering (update_checker) _parse_version stopped at the first non-integer component, so every "2.2.0-beta.N" parsed to (2, 2) and successive betas compared EQUAL - a tester would have been stranded on whichever build they installed first. Now pads to (major, minor, patch, is_final, stage_rank, stage_number), which also makes "1.3" and "1.3.0" the same release and keeps a final above its own pre-releases. Two long-standing xfail(strict=True) cases now pass and have been converted to real tests, per their own "if this starts passing, update" note. Downgrade guard (database) On a downgrade the migration loop range(new, old) is empty, so the old code fell through: it still wrote a full copy of the database to a .bak on EVERY launch, and every write raised OperationalError - a sqlite3.Error subclass the handlers swallow. Reads kept working, so the app looked healthy while recording nothing, permanently. Now refuses outright: no backup, no writes, no maintenance, one clear log line. Verified live against a v8 database. Verified WAL-safe backup (database) _backup_database used shutil.copy2, which copies only the main .db - in WAL mode everything since the last checkpoint lives in the sidecar, and the installer force-kills the app (taskkill /F), so the WAL is hot exactly when the backup matters. Measured: zero of 30,000 committed rows recovered, and the result still passed integrity_check. Now VACUUM INTO (which reads through the connection and compacts ~8x), then the copy is opened and row counts compared before it is trusted. Handles the target-exists case, since the filename is second-resolution while init retries are 0.1s apart. Prunes to the newest two; nothing deleted them before. SMART update rate (config_controller) set_interval clamps with max(0.1, interval), so passing the -1.0 sentinel raw meant 100ms polling - twenty times the intended 2s, and the one rate update_mode.py rules out in its own header. Also a data bug: the raw tier's key is one-second with INSERT OR IGNORE, so ~9 of every 10 samples were discarded. calculate_timer_interval() already resolved the sentinel; this path just never called it. Verified beyond the suite: a full build, a live migration on a real 50MB database, a live downgrade, and the complete 2.1.4 -> 2.1.5 portable update (hand-off, swap, relaunch from the install path, leftover sweep).
1 parent 2394997 commit b07ed3a

8 files changed

Lines changed: 647 additions & 75 deletions

File tree

build/version_info.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
VSVersionInfo(
22
ffi=FixedFileInfo(
3-
filevers=(2, 1, 3, 0),
4-
prodvers=(2, 1, 3, 0),
3+
filevers=(2, 1, 4, 0),
4+
prodvers=(2, 1, 4, 0),
55
mask=0x3f,
66
flags=0x0,
77
OS=0x40004,
@@ -17,12 +17,12 @@ VSVersionInfo(
1717
[
1818
StringStruct('CompanyName', 'Erez C137'),
1919
StringStruct('FileDescription', 'NetSpeedTray'),
20-
StringStruct('FileVersion', '2.1.3.0'),
20+
StringStruct('FileVersion', '2.1.4.0'),
2121
StringStruct('InternalName', 'NetSpeedTray'),
2222
StringStruct('LegalCopyright', 'Copyright (c) Erez C137'),
2323
StringStruct('OriginalFilename', 'NetSpeedTray.exe'),
2424
StringStruct('ProductName', 'NetSpeedTray'),
25-
StringStruct('ProductVersion', '2.1.3'),
25+
StringStruct('ProductVersion', '2.1.4'),
2626
]
2727
)
2828
]

src/netspeedtray/core/config_controller.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
from netspeedtray import constants
1414
from netspeedtray.utils.window_state import set_window_always_on_top
1515
from netspeedtray.utils.config import ConfigManager
16+
from netspeedtray.utils.timer_utils import calculate_timer_interval
1617

1718
if TYPE_CHECKING:
1819
from netspeedtray.views.widget.main import NetworkSpeedWidget
@@ -175,8 +176,14 @@ def apply_all_settings(self) -> None:
175176

176177
if w.monitor_thread:
177178
w.monitor_thread.update_config(w.config)
179+
# Resolve the SMART sentinel (-1.0) BEFORE handing the value to the sampler.
180+
# set_interval() clamps with max(0.1, interval), so passing -1.0 through raw
181+
# set the poll interval to 0.1s - 100ms, twenty times the intended 2s, and the
182+
# one rate constants/update_mode.py explicitly rules out as "too jarring for
183+
# human perception". Startup was already correct (main.py resolves it); only
184+
# this path, taken on every settings save, was not.
178185
update_rate = w.config.get("update_rate", constants.config.defaults.DEFAULT_UPDATE_RATE)
179-
w.monitor_thread.set_interval(update_rate)
186+
w.monitor_thread.set_interval(calculate_timer_interval(update_rate) / 1000.0)
180187

181188
if w.widget_state:
182189
self.logger.debug("Applying widget state config...")

src/netspeedtray/core/database.py

Lines changed: 155 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ def __init__(self, db_path: Path, parent: Optional[QObject] = None) -> None:
4141
# no 100ms busy-poll, lower latency, near-zero idle CPU. None is a wake-up sentinel.
4242
self._queue: "queue.Queue[Optional[Tuple[str, Any]]]" = queue.Queue()
4343
self._stop_event = threading.Event()
44+
# Set when the file on disk was written by a NEWER build than this one. Every write
45+
# path is then refused - see _check_and_create_schema for why silence is not an option.
46+
self._schema_incompatible = False
47+
self._downgrade_warned = False
4448
self.logger = logging.getLogger(f"NetSpeedTray.{self.__class__.__name__}")
4549

4650

@@ -55,7 +59,11 @@ def run(self) -> None:
5559
try:
5660
self._initialize_connection()
5761
self._check_and_create_schema()
58-
self._ensure_indexes() # idempotent; runs regardless of schema version
62+
if not self._schema_incompatible:
63+
# Skipped on a downgrade: CREATE INDEX against a newer schema either errors
64+
# (a compatibility view cannot be indexed) or writes to a file this build
65+
# does not understand. Both are exactly what the guard exists to prevent.
66+
self._ensure_indexes() # idempotent; runs regardless of schema version
5967
initialized = True
6068
break
6169
except sqlite3.Error as e:
@@ -119,6 +127,18 @@ def _execute_task(self, task: str, data: Any) -> None:
119127
except Exception:
120128
pass
121129
return
130+
if self._schema_incompatible:
131+
# Downgrade: every task below writes (maintenance DELETEs and aggregates). Drop them
132+
# rather than letting them fail into a swallowed OperationalError. Warn once - this
133+
# fires per persist tick, and #263 was a 137,000-line flood from exactly this shape.
134+
if not self._downgrade_warned:
135+
self._downgrade_warned = True
136+
self.logger.warning(
137+
"Refusing database task '%s' and all further writes: the file was written by a "
138+
"newer version of NetSpeedTray. This is logged once per session.", task,
139+
)
140+
return
141+
122142
handlers = {
123143
"persist_speed": self._persist_speed_batch,
124144
"persist_hardware": self._persist_hardware_batch,
@@ -243,21 +263,132 @@ def _has_existing_data(self) -> bool:
243263
except Exception:
244264
return True
245265

266+
# Keep the newest N pre-migration backups; older ones are pruned. Nothing used to
267+
# delete these at all, and at a year-scale database that is hundreds of MB of
268+
# abandoned copies sitting in the user's roaming profile.
269+
_BACKUP_RETENTION = 2
270+
246271
def _backup_database(self) -> bool:
247-
"""Backs up the current database file before critical operations."""
272+
"""
273+
Make a **verified**, WAL-safe copy of the database before a migration.
274+
275+
Uses ``VACUUM INTO`` rather than a file copy. ``shutil.copy2`` copies only the
276+
main ``.db`` file, but in WAL mode everything committed since the last
277+
checkpoint lives in the ``-wal`` sidecar - so a copy taken while the WAL is hot
278+
silently loses it. That is precisely the state during an upgrade: the installer
279+
force-kills the running app (``taskkill /F`` in setup.iss), so no checkpoint
280+
runs. Measured against a hot WAL, a ``copy2`` backup recovered **zero of 30,000
281+
committed rows** - and the result still passed ``PRAGMA integrity_check``,
282+
because an empty database is a perfectly valid one.
283+
284+
``VACUUM INTO`` reads through this connection, so it sees WAL content, and it
285+
compacts the copy as a side effect.
286+
287+
The copy is then opened and checked against the source before this returns
288+
True. A backup that silently isn't one is worse than no backup at all, because
289+
the caller reasons about its existence.
290+
"""
291+
backup_path: Optional[Path] = None
248292
try:
293+
version = self._get_current_db_version()
249294
timestamp = datetime.now().strftime("%Y%m%d_%H%M%S")
250-
backup_path = self.db_path.with_suffix(f".db.bak.v{self._get_current_db_version()}_{timestamp}")
295+
backup_path = self.db_path.with_suffix(f".db.bak.v{version}_{timestamp}")
296+
297+
# VACUUM INTO refuses to write an existing file. The timestamp is only
298+
# second-resolution while run()'s init retry backs off in 0.1s steps, so two
299+
# attempts land in the same second - and that collision would fire in exactly
300+
# the retry path a backup exists to protect.
301+
suffix = 0
302+
while backup_path.exists():
303+
suffix += 1
304+
backup_path = self.db_path.with_suffix(f".db.bak.v{version}_{timestamp}_{suffix}")
305+
251306
self.logger.info("Backing up database to: %s", backup_path)
252-
shutil.copy2(self.db_path, backup_path)
307+
self.conn.execute("VACUUM INTO ?", (str(backup_path),))
308+
309+
if not self._verify_backup(backup_path, version):
310+
self.logger.error("Pre-migration backup FAILED VERIFICATION; treating as no backup.")
311+
backup_path.unlink(missing_ok=True)
312+
return False
313+
314+
self._prune_old_backups()
253315
return True
254316
except Exception as e:
255317
# Don't swallow silently: the migration's data-loss guard assumes a backup was made, so a
256318
# disk-full / permission / lock failure here must be visible in the log (and the bak's absence).
257319
self.logger.error("Pre-migration database backup FAILED: %s", e)
320+
if backup_path is not None:
321+
try:
322+
backup_path.unlink(missing_ok=True) # never leave a partial copy that looks real
323+
except Exception:
324+
pass
258325
return False
259326

260327

328+
def _verify_backup(self, backup_path: Path, expected_version: int) -> bool:
329+
"""
330+
Open the backup and prove it holds the same data as the source.
331+
332+
Structural checks alone are not enough: the failure this exists to catch (a
333+
WAL-blind copy) produces a *valid* database that is merely missing rows, which
334+
``integrity_check`` reports as ``ok``. So compare content - the schema version
335+
and the row count of every history table.
336+
"""
337+
check: Optional[sqlite3.Connection] = None
338+
try:
339+
# NOTE: `with sqlite3.connect(...)` commits/rolls back the transaction but does
340+
# NOT close the connection. On Windows the leaked handle keeps the file locked,
341+
# so _prune_old_backups() then fails with WinError 32. Close it explicitly.
342+
check = sqlite3.connect(f"file:{backup_path}?mode=ro", uri=True)
343+
344+
if check.execute("PRAGMA quick_check").fetchone()[0] != "ok":
345+
self.logger.error("Backup verification: quick_check did not return ok.")
346+
return False
347+
348+
row = check.execute("SELECT value FROM metadata WHERE key='db_version'").fetchone()
349+
if row is None or int(row[0]) != expected_version:
350+
self.logger.error(
351+
"Backup verification: db_version is %s, expected %d.",
352+
row[0] if row else "missing", expected_version,
353+
)
354+
return False
355+
356+
tables = [r[0] for r in self.conn.execute(
357+
"SELECT name FROM sqlite_master WHERE type='table' AND name NOT LIKE 'sqlite_%'"
358+
)]
359+
for table in tables:
360+
src = self.conn.execute(f"SELECT COUNT(*) FROM '{table}'").fetchone()[0]
361+
dst = check.execute(f"SELECT COUNT(*) FROM '{table}'").fetchone()[0]
362+
if src != dst:
363+
self.logger.error(
364+
"Backup verification: table '%s' has %d rows in the backup but %d in the "
365+
"source. The backup is incomplete.", table, dst, src,
366+
)
367+
return False
368+
return True
369+
except Exception as e:
370+
self.logger.error("Backup verification failed to read the copy: %s", e)
371+
return False
372+
finally:
373+
if check is not None:
374+
check.close()
375+
376+
377+
def _prune_old_backups(self) -> None:
378+
"""Keep only the newest `_BACKUP_RETENTION` backups; nothing else ever deleted them."""
379+
try:
380+
backups = sorted(
381+
self.db_path.parent.glob(f"{self.db_path.stem}.db.bak.*"),
382+
key=lambda p: p.stat().st_mtime,
383+
reverse=True,
384+
)
385+
for stale in backups[self._BACKUP_RETENTION:]:
386+
stale.unlink(missing_ok=True)
387+
self.logger.debug("Pruned old database backup: %s", stale.name)
388+
except Exception as e:
389+
self.logger.warning("Could not prune old database backups: %s", e)
390+
391+
261392
def _migrate_schema(self, current_version: int) -> None:
262393
"""Handles migration from current_version to _DB_VERSION."""
263394
self.logger.info("Migrating database from version %d to %d...", current_version, self._DB_VERSION)
@@ -413,6 +544,26 @@ def _check_and_create_schema(self) -> None:
413544
self.logger.error("DB version is UNKNOWN; refusing to migrate or rebuild - preserving the database as-is.")
414545
return
415546

547+
if current_version > self._DB_VERSION:
548+
# DOWNGRADE. The file was written by a newer build; this one cannot understand it.
549+
# There is no migration to run - range(new, old) is empty - so the old code fell
550+
# through and did two harmful things:
551+
# 1. _migrate_schema() still ran _backup_database() unconditionally, writing a
552+
# full copy of the database on EVERY launch, which nothing ever deletes.
553+
# 2. Reads kept working (a newer schema may expose compatibility views), while
554+
# every write raised OperationalError - a sqlite3.Error subclass that the
555+
# handlers below swallow. The app looked healthy and silently recorded nothing.
556+
# Refusing outright is the only honest option: no backup, no writes, no maintenance.
557+
self._schema_incompatible = True
558+
self.logger.error(
559+
"Database schema v%d was written by a NEWER version of NetSpeedTray than this one "
560+
"(which understands v%d). Running READ-ONLY: history will be shown but nothing new "
561+
"will be recorded, and no data will be modified or deleted. Upgrade again to resume "
562+
"recording, or move %s aside to start a fresh history.",
563+
current_version, self._DB_VERSION, self.db_path.name,
564+
)
565+
return
566+
416567
if current_version > 0:
417568
self.logger.info("Database version mismatch (Current: %d, Target: %d). Attempting migration...", current_version, self._DB_VERSION)
418569
try:

src/netspeedtray/core/update_checker.py

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,68 @@
2121
CHECK_INTERVAL_HOURS = 24
2222

2323

24+
# Pre-release stage ordering. An unrecognised stage sorts ABOVE these but still
25+
# below any final release, so an unexpected tag can never look newer than a real one.
26+
_PRERELEASE_STAGES = {"alpha": 0, "a": 0, "beta": 1, "b": 1, "rc": 2, "c": 2, "pre": 2}
27+
_UNKNOWN_STAGE_RANK = 3
28+
29+
2430
def _parse_version(version_str: str) -> Tuple[int, ...]:
25-
"""Parse 'v1.3.1' or '1.3.1' into a comparable tuple of ints."""
31+
"""
32+
Parse a release tag into a comparable tuple of ints.
33+
34+
The tag is split into a release core and an optional pre-release suffix, then
35+
padded to a fixed shape so plain tuple comparison implements the ordering we
36+
need for a beta cycle::
37+
38+
(major, minor, patch, is_final, stage_rank, stage_number)
39+
40+
- ``1.3`` and ``1.3.0`` are the same release -> both ``(1, 3, 0, 1, 0, 0)``.
41+
- A final release outranks every pre-release of it (``is_final`` 1 beats 0),
42+
so ``2.2.0`` > ``2.2.0-beta.4``.
43+
- Successive pre-releases order correctly: ``beta.1`` < ``beta.2`` < ``rc.1``.
44+
Without this a beta tester is stranded, because every ``2.2.0-beta.N``
45+
compared EQUAL and `is_newer` could never advance between them.
46+
- Build metadata (``+abc``) never affects precedence, per semver.
47+
48+
Never raises; an unparseable tag yields ``()`` so it sorts below everything.
49+
"""
2650
cleaned = version_str.lstrip("vV").strip()
27-
parts = []
28-
for part in cleaned.split("."):
51+
cleaned = cleaned.split("+", 1)[0] # build metadata is not precedence
52+
core, _, pre = cleaned.partition("-")
53+
54+
release: list[int] = []
55+
for part in core.split("."):
2956
try:
30-
parts.append(int(part))
57+
release.append(int(part))
3158
except ValueError:
3259
break
33-
return tuple(parts)
60+
if not release:
61+
return () # unparseable sorts below everything
62+
63+
while len(release) < 3: # 1.3 and 1.3.0 are one release
64+
release.append(0)
65+
66+
if not pre:
67+
return (*release, 1, 0, 0)
68+
69+
stage_rank, stage_number = _UNKNOWN_STAGE_RANK, 0
70+
for token in pre.replace("-", ".").split("."):
71+
if token.isdigit():
72+
stage_number = int(token)
73+
continue
74+
name = token.lower()
75+
if name in _PRERELEASE_STAGES:
76+
stage_rank = _PRERELEASE_STAGES[name]
77+
continue
78+
# digits glued to the stage name, e.g. 'beta2'
79+
head = name.rstrip("0123456789")
80+
tail = name[len(head):]
81+
if head in _PRERELEASE_STAGES:
82+
stage_rank = _PRERELEASE_STAGES[head]
83+
if tail:
84+
stage_number = int(tail)
85+
return (*release, 0, stage_rank, stage_number)
3486

3587

3688
def is_newer(latest: str, current: str) -> bool:

0 commit comments

Comments
 (0)