From e65e8f7c31e69af815d52fbdb3d0aecf2636f775 Mon Sep 17 00:00:00 2001 From: Torlando <281092095+torlando-agent[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:01:03 +0000 Subject: [PATCH] fix: make marker reclaim conditional; require own marker at commit Greptile round 11 remediation: Reclamation of a stale cross-marker was check-then-act: the stale token was read, a delete decision made, and the file removed in separate steps. A cross-installer renewing inside that window had its renewed live claim deleted, and the victim's commit-time verification accepted a MISSING own marker -- both activations could then proceed into the same record-write window. Both producers now: (a) reclaim conditionally: the marker is re-read immediately before deletion; a token that became fresh after the original stale check aborts the reclaim instead of deleting the live claim, (b) require the own marker at commit time when the install acquired one: a missing own file (the claim was reclaimed from under us) or a foreign owner aborts the activation before any record write. A sub-microsecond renewal between the conditional re-read and the unlink is still bounded: the victim aborts at commit, so the two activations never reach the record writes together. Tests: conditional reclaim against a racing renewal (CLI + flasher, deterministic seams), commit abort when the own marker is missing or foreign (both producers). CLI 92 passed; flasher 29 passed. --- docs/flasher/js/map-installer.js | 43 +++++++++++++--- tests/tools/test_build_map_pack.py | 80 ++++++++++++++++++++++++++++++ tests/web/test_map_installer.mjs | 73 +++++++++++++++++++++++++++ tools/maps/build_map_pack.py | 45 +++++++++++++---- 4 files changed, 224 insertions(+), 17 deletions(-) diff --git a/docs/flasher/js/map-installer.js b/docs/flasher/js/map-installer.js index 519eed1f..efae25ca 100644 --- a/docs/flasher/js/map-installer.js +++ b/docs/flasher/js/map-installer.js @@ -100,13 +100,22 @@ async function readInstallMarker(pyxis, name = INSTALL_MARKER_NAME) { catch (error) { if (error?.name === 'NotFoundError') return null; throw error; } return new TextDecoder('utf-8').decode(new Uint8Array(await (await handle.getFile()).arrayBuffer())); } +export {readInstallMarker}; // Respect the other producers' marker files. A fresh cross marker (the // CLI builder's, or the legacy shared one) means a live cross-producer // installer: refuse. A stale or malformed one is an abandoned install; // reclaim it only when reclaim is true (at acquire). Fresh cross // markers are never touched. -async function checkCrossInstallers(pyxis, reclaim) { +// +// Reclaim is conditional: the file is re-read immediately before +// deletion and a token that became fresh after the original check (the +// owner renewed while we were deciding) aborts the reclaim instead of +// deleting a live claim. A sub-millisecond renewal can still land +// between that final read and the removeEntry; the owner's commit-time +// verification requires its own marker, so such an overlap aborts on +// its side instead of proceeding. +export async function checkCrossInstallers(pyxis, reclaim) { for (const name of CROSS_INSTALL_MARKERS) { const raw = await readInstallMarker(pyxis, name); if (raw === null) continue; @@ -114,7 +123,14 @@ async function checkCrossInstallers(pyxis, reclaim) { if (parsed && installMarkerIsFresh(parsed.epochMs)) { fail('Another map installer is already running on this card; wait for it to finish and retry'); } - if (reclaim) { try { await pyxis.removeEntry(name); } catch {} } + if (reclaim) { + const confirm = await readInstallMarker(pyxis, name); + const confirmParsed = confirm !== null ? parseInstallMarker(confirm) : null; + if (confirmParsed && installMarkerIsFresh(confirmParsed.epochMs)) { + fail('Another map installer is already running on this card; wait for it to finish and retry'); + } + try { await pyxis.removeEntry(name); } catch {} + } } } @@ -201,16 +217,29 @@ export async function renewInstallMarker(pyxis, owner) { // already-published pack is kept; a retry re-derives from the new state and // converges (a pack no record names is device-harmless: the firmware only // reads packs enumerated in the active selection). -async function verifyActivationState(pyxis, mapSets, styleName, markerToken, state) { +export async function verifyActivationState(pyxis, mapSets, styleName, markerToken, state) { // Cross producers: a fresh marker in the CLI/legacy files means the // other installer started after we acquired. await checkCrossInstallers(pyxis, false); const marker = await readInstallMarker(pyxis); - if (marker !== null) { - // Compare by owner: our own marker may carry a renewed (newer) epoch. + if (markerToken !== null) { + // Our own marker is REQUIRED: a missing file means the claim was + // reclaimed from under us (a cross-reclaimer raced our last + // renewal) and the activation would proceed alongside the new + // holder. Abort instead. + const ours = parseInstallMarker(markerToken); + if (marker === null) { + fail('The install claim was reclaimed during installation; wait for the other installer to finish and retry'); + } const parsed = parseInstallMarker(marker); - const ours = markerToken !== null ? parseInstallMarker(markerToken) : null; - if (parsed && installMarkerIsFresh(parsed.epochMs) && (ours === null || parsed.owner !== ours.owner)) { + if (!parsed || parsed.owner !== ours.owner) { + fail('The map-install marker was reclaimed during installation; wait for the other installer to finish and retry'); + } + } else if (marker !== null) { + // No token was acquired (resume path without a marker): only a + // FRESH marker in our own file is a live foreign claim. + const parsed = parseInstallMarker(marker); + if (parsed && installMarkerIsFresh(parsed.epochMs)) { fail('Another map installer is running on this card; wait for it to finish and retry'); } } diff --git a/tests/tools/test_build_map_pack.py b/tests/tools/test_build_map_pack.py index c319dea0..272374b4 100644 --- a/tests/tools/test_build_map_pack.py +++ b/tests/tools/test_build_map_pack.py @@ -10,6 +10,7 @@ from pathlib import Path import struct import subprocess import sys +import time import zlib import pytest @@ -1480,3 +1481,82 @@ def test_fresh_cross_marker_during_install_aborts_at_renewal( assert "web-racer" in marker_path.read_text() # Our own claim is released even on the abort. assert not (sd / "pyxis-map/.pyxis-installing-cli").exists() + + +def test_cross_reclaim_refuses_when_owner_renews_between_check_and_delete( + tmp_path: Path) -> None: + # Round 12 (Greptile): reclamation is check-then-act. If the + # cross-installer renews while we are deciding, the delete would + # clobber a live claim. The conditional re-read must see the fresh + # token and abort instead of deleting. + tool = load_tool() + sd = tmp_path / "sd" + sd.mkdir() + (sd / "pyxis-map").mkdir() + marker = sd / "pyxis-map/.pyxis-installing-web" + marker.write_text(f"PYXI 1 web-owner {int(time.time() * 1000) - 16 * 60 * 1000}\n") + + real_read = tool._read_selection_at + + def racing_read(parent_fd, name, _calls=[0]): + if name == ".pyxis-installing-web": + _calls[0] += 1 + if _calls[0] == 2: + marker.write_text(f"PYXI 1 web-owner {int(time.time() * 1000)}\n") + return real_read(parent_fd, name) + + tool._read_selection_at = racing_read + try: + pyxis_fd = os.open(str(sd / "pyxis-map"), os.O_RDONLY | os.O_DIRECTORY) + except OSError: + pyxis_fd = os.open(str(sd / "pyxis-map"), os.O_RDONLY) + try: + with pytest.raises(tool.PackError, match="another map installer is already running"): + tool._check_cross_installers(pyxis_fd, reclaim=True) + assert marker.exists() + assert tool._marker_is_fresh( + tool._parse_marker(marker.read_bytes())[1]) + finally: + os.close(pyxis_fd) + tool._read_selection_at = real_read + + +def test_commit_abort_when_own_marker_was_reclaimed( + tmp_path: Path) -> None: + # Round 12 (Greptile): a missing own marker at commit time means our + # claim was reclaimed from under us. Verification must abort, not + # proceed with the activation writes. + tool = load_tool() + sd = tmp_path / "sd" + sd.mkdir() + (sd / "pyxis-map").mkdir() + (sd / "pyxis-map/map-sets").mkdir() + token = "PYXI 1 cli-self 12345" + pyxis_fd = os.open(str(sd / "pyxis-map"), os.O_RDONLY | os.O_DIRECTORY) + map_sets_fd = os.open(str(sd / "pyxis-map/map-sets"), os.O_RDONLY | os.O_DIRECTORY) + try: + # No marker file exists at all: the claim was reclaimed. + with pytest.raises(tool.PackError, match="reclaimed during installation"): + tool._verify_activation_state_at( + pyxis_fd, map_sets_fd, + slot_raw=[None, None], style_raw=None, + style_name="osm-bright.pmas", marker_token=token) + # A foreign owner in our own file: also an abort. + (sd / "pyxis-map/.pyxis-installing-cli").write_text(token.replace("cli-self", "cli-other") + "\n") + with pytest.raises(tool.PackError, match="reclaimed during installation"): + tool._verify_activation_state_at( + pyxis_fd, map_sets_fd, + slot_raw=[None, None], style_raw=None, + style_name="osm-bright.pmas", marker_token=token) + # Our own marker (any epoch: renewals advance it): passes the + # marker check; the slot/style re-reads against identical state + # still pass. + (sd / "pyxis-map/.pyxis-installing-cli").write_text( + f"PYXI 1 cli-self {int(time.time() * 1000)}\n") + tool._verify_activation_state_at( + pyxis_fd, map_sets_fd, + slot_raw=[None, None], style_raw=None, + style_name="osm-bright.pmas", marker_token=token) + finally: + os.close(pyxis_fd) + os.close(map_sets_fd) diff --git a/tests/web/test_map_installer.mjs b/tests/web/test_map_installer.mjs index 46fa334e..cfede97d 100644 --- a/tests/web/test_map_installer.mjs +++ b/tests/web/test_map_installer.mjs @@ -939,3 +939,76 @@ test('a fresh cross marker during installation aborts at renewal', async () => { assert.equal(pyxis.children.has('.pyxis-installing-cli'), true); assert.equal(pyxis.children.has(MARKER), false, 'our marker released on abort'); }); + + +// --- Round 12 (Greptile): the cross-marker reclaim is a check-then-act, +// and the owner can renew in the window. Reclaim must therefore be +// conditional: a token that became fresh between the original check and +// the deletion aborts the reclaim (the claim is live again), and the +// owner's commit-time verification must REQUIRE its own marker -- a +// missing claim is an abort, not a pass. + +import {checkCrossInstallers, readInstallMarker, verifyActivationState} from '../../docs/flasher/js/map-installer.js'; + +test('reclaim aborts when the owner renews between check and delete', async () => { + const root = new MemoryDirectoryHandle('sd'); + const pyxis = await root.getDirectoryHandle('pyxis-map', {create: true}); + const staleToken = `PYXI 1 cli-owner ${Date.now() - 16 * 60 * 1000}`; + const freshToken = `PYXI 1 cli-owner ${Date.now()}`; + const writeMarker = async text => { + // Overwrite semantics (the seam simulates the owner's renewal + // rewrite): no exclusive, since the file already holds the stale + // token. + const handle = await pyxis.getFileHandle('.pyxis-installing-cli', {create: true}); + const writable = await handle.createWritable({keepExistingData: false}); + await writable.write(new TextEncoder().encode(text)); + await writable.close(); + }; + await writeMarker(staleToken); + // Deterministic seam: each marker read goes through + // pyxis.getFileHandle. The first call is the original check (stale + // token); the owner's renewal lands before the second call (the + // conditional re-read), which must see the fresh token and abort + // instead of deleting. + let readCount = 0; + const realGet = pyxis.getFileHandle.bind(pyxis); + pyxis.getFileHandle = async (name, options) => { + // Count plain reads only: the marker writes (create:true) are the + // test's own seam writes, not reclaim reads. + if (name === '.pyxis-installing-cli' && !options?.create) { + readCount += 1; + if (readCount === 2) await writeMarker(freshToken); + } + return realGet(name, options); + }; + const realRemove = pyxis.removeEntry.bind(pyxis); + let removed = false; + pyxis.removeEntry = async name => { removed = true; return realRemove(name); }; + try { + await assert.rejects(checkCrossInstallers(pyxis, true), MapInstallerError, 'renewal must abort the reclaim'); + assert.equal(removed, false, 'the renewed live claim must not be deleted'); + const surviving = await readInstallMarker(pyxis, '.pyxis-installing-cli'); + assert.equal(surviving, freshToken, 'the renewed token survives the reclaim attempt'); + } finally { + pyxis.removeEntry = realRemove; + pyxis.getFileHandle = realGet; + } +}); + +test('commit-time verification aborts when the own marker is missing', async () => { + const root = new MemoryDirectoryHandle('sd'); + const pyxis = await root.getDirectoryHandle('pyxis-map', {create: true}); + const mapSets = await pyxis.getDirectoryHandle('map-sets', {create: true}); + const owner = `web-${Date.now().toString(16)}`; + const token = `PYXI 1 ${owner} ${Date.now()}`; + // The claim existed at acquire time but a cross-reclaimer deleted it + // after our last renewal: verify must abort, not proceed. + const handle = await pyxis.getFileHandle(MARKER, {create: true, exclusive: true}); + const writable = await handle.createWritable({keepExistingData: false}); + await writable.write(new TextEncoder().encode(token)); + await writable.close(); + await pyxis.removeEntry(MARKER); + await assert.rejects( + verifyActivationState(pyxis, mapSets, 'osm-bright.pmas', token, {slotBytes: [null, null], styleBytes: null}), + MapInstallerError, 'a reclaimed claim must abort the commit'); +}); diff --git a/tools/maps/build_map_pack.py b/tools/maps/build_map_pack.py index c6c6e887..cd381aca 100644 --- a/tools/maps/build_map_pack.py +++ b/tools/maps/build_map_pack.py @@ -1410,6 +1410,14 @@ def _check_cross_installers(pyxis_fd: int, *, reclaim: bool) -> None: malformed one is an abandoned install; reclaim it only when the caller says so (at acquire, where claiming is the intent). Fresh foreign markers are never touched. + + Reclaim is conditional: the file is re-read immediately before + deletion and a token that became fresh after the original check + (the owner renewed while we were deciding) aborts the reclaim + instead of deleting a live claim. A sub-microsecond renewal can + still land between that final read and the unlink; the owner's + commit-time verification requires its own marker, so such an + overlap aborts on its side instead of proceeding. """ for name in _CROSS_INSTALL_MARKERS: raw = _read_selection_at(pyxis_fd, name) @@ -1420,6 +1428,11 @@ def _check_cross_installers(pyxis_fd: int, *, reclaim: bool) -> None: raise PackError("another map installer is already running on " "this card; wait for it to finish and retry") if reclaim: + confirm = _read_selection_at(pyxis_fd, name) + confirm_parsed = _parse_marker(confirm) if confirm is not None else None + if confirm_parsed is not None and _marker_is_fresh(confirm_parsed[1]): + raise PackError("another map installer is already running on " + "this card; wait for it to finish and retry") _unlink_quietly_at(pyxis_fd, name) @@ -1563,22 +1576,34 @@ def _verify_activation_state_at(pyxis_fd: int, map_sets_fd: int, *, is kept; a retry re-derives from the new state and converges (the pack is device-harmless while it is not named by any record: the firmware only reads packs enumerated in the active selection). + + The own marker is REQUIRED when this install acquired one: a missing + own file means the claim was reclaimed from under us (a stale + cross-reclaimer raced our last renewal) and the activation would + proceed alongside the new holder. Abort instead. """ # Cross producers: a fresh marker in the web/legacy files means the # other installer started (or is mid-install) after we acquired. _check_cross_installers(pyxis_fd, reclaim=False) marker_raw = _read_selection_at(pyxis_fd, _INSTALL_MARKER_NAME) - if marker_raw is not None: - # Compare by owner, not the full token: we renew the marker's - # epoch during publication (heartbeat), so our own marker may - # carry a newer epoch than the one acquired at install start. - own_owner = None - if marker_token is not None: - parsed_own = _parse_marker(marker_token.encode("ascii")) - own_owner = parsed_own[0] if parsed_own is not None else None + if marker_token is not None: + parsed_own = _parse_marker(marker_token.encode("ascii")) + own_owner = parsed_own[0] if parsed_own is not None else None + if marker_raw is None: + raise PackError("the install claim was reclaimed during " + "installation; wait for the other installer " + "to finish and retry") parsed = _parse_marker(marker_raw) - if parsed is not None and _marker_is_fresh(parsed[1]) \ - and parsed[0] != own_owner: + if parsed is None or parsed[0] != own_owner: + raise PackError("the map-install marker was reclaimed during " + "installation; wait for the other installer " + "to finish and retry") + elif marker_raw is not None: + # No token was acquired (resume path without a marker): only a + # FRESH marker in our own file is a live foreign claim; a + # stale one is an abandoned install we are resuming. + parsed = _parse_marker(marker_raw) + if parsed is not None and _marker_is_fresh(parsed[1]): raise PackError("another map installer is running on this card; " "wait for it to finish and retry") for index, slot_name in enumerate(("active-pack.0", "active-pack.1")):