fix(file-sync): don't rate-limit retry after a failed sync cycle
FileSyncManager.sync() is rate-limited to once per _sync_interval via _last_sync_time, and its docstring promises that on failure "state rolls back so the next cycle retries everything". But the except handler also set _last_sync_time = time.monotonic() on failure, so the next non-forced sync() within the interval hit the rate-limit guard and returned early — suppressing the retry the rollback had just prepared. Because the non-forced sync() runs before every command on the SSH, Modal and Daytona backends, a single transient upload failure (network blip, dropped channel) left the remote with stale files for the next command (up to _sync_interval, default 5s). Forced syncs bypass the guard, which is why it was intermittent. Remove the failure-path timestamp bump so the clock only advances on a successful or no-op cycle, matching the documented contract. Add a regression test that fails before this change and passes after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
f904f185ee
commit
f9158b818b
|
|
@ -225,6 +225,42 @@ class TestRateLimiting:
|
|||
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.time, "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):
|
||||
|
|
|
|||
|
|
@ -232,7 +232,12 @@ class FileSyncManager:
|
|||
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)
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue