fix(sync): keep the watch alive when the definition file cannot be read #9257
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| """DefinitionValidator for Validating YAML and JSON Files""" | ||
|
|
||
| import logging | ||
| import time | ||
| from pathlib import Path | ||
| from typing import Any, Dict, Optional | ||
|
|
||
|
|
@@ -11,6 +12,9 @@ | |
|
|
||
| LOG = logging.getLogger(__name__) | ||
|
|
||
| FILE_READ_ATTEMPTS = 3 | ||
| FILE_READ_RETRY_DELAY = 0.1 | ||
|
|
||
|
|
||
| class DefinitionValidator: | ||
| _path: Path | ||
|
|
@@ -75,7 +79,7 @@ def validate_file(self) -> bool: | |
| return False | ||
|
|
||
| try: | ||
| self._data = self._parse_file() | ||
| except (ValueError, yaml.YAMLError) as e: | ||
| LOG.debug( | ||
| "File %s failed to validate due to it file cannot be parsed. \ | ||
|
|
@@ -84,4 +88,32 @@ def validate_file(self) -> bool: | |
| exc_info=e, | ||
| ) | ||
| return False | ||
| except OSError as e: | ||
| LOG.warning( | ||
| "File %s failed to validate because it cannot be read. \ | ||
| The change will not be synced until the file is saved again.", | ||
| self._path, | ||
| exc_info=e, | ||
| ) | ||
| return False | ||
| return True | ||
|
|
||
| def _parse_file(self) -> Dict[str, Any]: | ||
| """Read and parse the definition file, retrying while it is locked by another process. | ||
|
|
||
| Returns | ||
| ------- | ||
| Dict[str, Any] | ||
| Parsed content of the definition file. | ||
| """ | ||
| remaining_attempts = FILE_READ_ATTEMPTS | ||
| delay = FILE_READ_RETRY_DELAY | ||
| while True: | ||
| try: | ||
| return parse_yaml_file(str(self._path)) | ||
| except PermissionError: | ||
|
There was a problem hiding this comment. [ERROR_HANDLING] The retry only covers
Since the non-existent-file case is already handled by the except OSError:
remaining_attempts -= 1
if not remaining_attempts:
raise
time.sleep(delay)
delay = 2If retrying every
Author
There was a problem hiding this comment. I do not think the missing file window reaches _parse_file. validate_file returns at the self._path.exists() check above, so parse_yaml_file is never called. I confirmed it: on an absent path validate_file returns False with parse_yaml_file call count 0. Retrying FileNotFoundError below that check would only cover the instant between exists() and open(). That instant also does not lose the edit. While the path is absent the write has not finished, so watchdog sends another create or modified event once it has. I ran a delete-then-rename save against a real TemplateTrigger and HandlerObserver: one callback fired and the validator ended with the new content. PermissionError is the opposite case. The file is there and holds its final content, the save is done, and no further event is coming. That is why it needs the retry. You are right about the message though. My earlier reply said the other OSErrors no longer get the lock wording, and that was wrong, they shared the same warning. I removed the lock claim from it in 3d69119. The logged exception already names the real error. |
||
| remaining_attempts -= 1 | ||
| if not remaining_attempts: | ||
| raise | ||
| time.sleep(delay) | ||
|
There was a problem hiding this comment. [PERFORMANCE]
Nothing is lost — watchdog's queue is unbounded — but code sync is held up by a problem that only concerns the template. Worth either shrinking the blocking window (a single short retry) or performing the re-read off the dispatch thread, so an unreadable template cannot slow down unrelated resources.
Author
There was a problem hiding this comment. This is the same point as the earlier round, and I acted on it there. The window went from 0.7s to 0.3s, and only the two waits between attempts are left. I would rather keep it at that. The wait only happens when a read fails, which is rare, and the queue is unbounded, so events are delayed but never lost. Cutting it further weakens the fix: the lock in the report is millisecond-scale, but a virus scanner can hold the file for longer than one short retry. Doing the read off the dispatch thread changes how the watcher runs. Ordering against _on_template_change and shutdown would both need work, and that is more than this fix should carry. |
||
| delay *= 2 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[PERFORMANCE] The retry sleeps block watchdog's shared dispatch thread, so a locked template file now delays code-sync events too.
HandlerObserverextends watchdog'sObserver(samcli/lib/utils/path_observer.py:130), andWatchManagerschedules every trigger's handlers on that one instance (watch_manager.py:172,watch_manager.py:194). Watchdog's emitters are per-watch threads, but they only enqueue; all handler callbacks — includingTemplateTrigger._validator_wrapperand every code-change trigger — are invoked serially from the single observer dispatch thread.time.sleepinside_parse_filetherefore stalls dispatch for all watched resources, not just the template.With
attempts=3, delay=0.05the waits are 0.1s + 0.2s + 0.4s ≈ 0.7s, and note thatretrysleeps after the final failed attempt as well, so the last 0.4s is pure waste. An atomic save typically fires several events in a row, so a genuinely unreadable definition file multiplies that stall per event while code sync silently lags behind.Two things worth tightening:
@retry(exc=OSError, attempts=2, delay=0.02, ...). The lock an atomic save holds is millisecond-scale, so hundreds of milliseconds of backoff buys nothing.OSErroralso covers non-transient cases such asFileNotFoundError(reachable as a race after theself._path.exists()check) andIsADirectoryError. Those burn the full backoff and then produce a warning claiming the file "may be locked by another process," which is misleading. Narrowing the retried exception toPermissionError— the case described in the issue — avoids both the wasted delay and the wrong message.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both points are correct. The dispatch thread is shared, so the wait held up code sync too, and the last sleep before the raise was wasted.
The new loop sleeps only between attempts: 0.1s then 0.2s, 0.3s in total instead of 0.7s. It also retries PermissionError only, so FileNotFoundError and IsADirectoryError return at once and no longer get the lock message. There is a test for that. Commit 4e31182.