File sync rate-limit clock advances on error

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #67927 fix(file-sync): don't rate-limit retry after a failed sync cycle (salvage #39946)

  merged to main

  files: tests/tools/test_file_sync.py, tools/environments/file_sync.py

  Show the diff

  • tests/tools/test_file_sync.py

    @@ -225,6 +225,42 @@ def test_env_var_forces_sync(self, tmp_files, tmp_path):
                 mgr.sync()
             assert upload.call_count == 1
     
    +    def test_failed_sync_does_not_suppress_next_retry(self, tmp_files, monkeypatch):
    +        """A failed sync must not advance the rate-limit clock.
    +
    +        Regression: the failure path used to set ``_last_sync_time`` on
    +        rollback, so the next non-forced ``sync()`` within ``sync_interval``
    +        hit the rate-limit guard and returned early — silently suppressing the
    +        retry the rollback had just prepared and leaving the remote stale.
    +        """
    +        from tools.environments import file_sync
    +
    +        clock = {"t": 1000.0}
    +        monkeypatch.setattr(file_sync, "_monotonic", lambda: clock["t"])
    +
    +        upload = MagicMock(side_effect=RuntimeError("transport down"))
    +        mgr = FileSyncManager(
    +            get_files_fn=_make_get_files(tmp_files),
    +            upload_fn=upload,
    +            delete_fn=MagicMock(),
    +            sync_interval=10.0,
    +        )
    +
    +        # First sync fails (forced bypasses the guard); state rolls back.
    +        mgr.sync(force=True)
    +        assert upload.call_count >= 1
    +
    +        # Transport recovers; advance the clock by LESS than the interval.
    +        upload.reset_mock()
    +        upload.side_effect = None
    +        clock["t"] = 1002.0  # 2s later, < 10s interval
    +
    +        # The next non-forced cycle must retry, not be rate-limited away.
    +        mgr.sync()
    +        assert upload.call_count == 3, (
    +            "a failed sync must not rate-limit the next retry"
    +        )
    +
     
     class TestEdgeCases:
         def test_empty_file_list(self):

  • tools/environments/file_sync.py

    @@ -35,6 +35,9 @@
     # ``time.sleep`` globally because ``time`` is the module object; under xdist
     # that lets unrelated background threads inflate retry-test call counts.
     _sleep = time.sleep
    +# Same rationale for the rate-limit clock: tests patch ``_monotonic``
    +# instead of ``time.monotonic`` on the shared module object.
    +_monotonic = time.monotonic
     
     _SYNC_INTERVAL_SECONDS = 5.0
     _FORCE_SYNC_ENV = "HERMES_FORCE_FILE_SYNC"
    @@ -169,7 +172,7 @@ def sync(self, *, force: bool = False) -> None:
             On failure, state rolls back so the next cycle retries everything.
             """
             if not force and not os.environ.get(_FORCE_SYNC_ENV):
    -            now = time.monotonic()
    +            now = _monotonic()
                 if now - self._last_sync_time < self._sync_interval:
                     return
     
    @@ -193,7 +196,7 @@ def sync(self, *, force: bool = False) -> None:
             to_delete = [p for p in self._synced_files if p not in current_remote_paths]
     
             if not to_upload and not to_delete:
    -            self._last_sync_time = time.monotonic()
    +            self._last_sync_time = _monotonic()
                 return
     
             # Snapshot for rollback (only when there's work to do)
    @@ -227,12 +230,17 @@ def sync(self, *, force: bool = False) -> None:
                     self._pushed_hashes.pop(p, None)
     
                 self._synced_files = new_files
    -            self._last_sync_time = time.monotonic()
    +            self._last_sync_time = _monotonic()
     
             except Exception as exc:
                 self._synced_files = prev_files
                 self._pushed_hashes = prev_hashes
    -            self._last_sync_time = time.monotonic()
    +            # Do NOT advance _last_sync_time here: a failed cycle rolls state
    +            # back so the next cycle can retry. Bumping the rate-limit clock on
    +            # failure would make the next non-forced sync() return early (the
    +            # guard above), suppressing that retry for up to _sync_interval and
    +            # leaving the remote with stale files — contradicting this method's
    +            # documented "next cycle retries everything" contract.
                 logger.warning("file_sync: sync failed, rolled back state: %s", exc)
     
         # ------------------------------------------------------------------