fix(sync): keep the watch alive when the definition file cannot be read

[lazerg]

Which issue(s) does this change fix?

#9173

Why is this change necessary?

sam sync --watch stops noticing template edits after a single read failure. Editors that save atomically briefly hold the file, so the watchdog read can hit PermissionError: [Errno 13] on Windows. DefinitionValidator.validate_file only catches ValueError and yaml.YAMLError, so the OSError travels up through _validator_wrapper into watchdog's dispatch loop and kills the observer thread. Code sync carries on, so nothing tells the user that infra sync is dead until they restart sam sync.

Reproduced by making parse_yaml_file raise PermissionError once for a real TemplateTrigger and HandlerObserver: the traceback matches the one in the issue, the observer thread is gone afterwards, and later template writes fire no callback.

How does it address the issue?

validate_file now retries the read up to three times on PermissionError, backing off 0.1s then 0.2s, which covers the millisecond-scale lock an atomic save takes. If the file is still locked, or the read fails for any other OSError, it logs a warning naming the file and returns False, the same way it already handles an unparseable file, so the observer keeps running.

What side effects does this change have?

A locked file costs up to 0.3s in watchdog's dispatch thread before the read gives up, which delays other watched resources for that long since they share the thread. Only PermissionError is retried, so any other read failure still returns immediately.

A definition file that stays unreadable now produces a warning per file event instead of a crash. TemplateTrigger.validate_template also changes at sync startup: a read failure there used to abort sam sync --watch, and now raises InvalidTemplateFile, which WatchManager already catches and reports the same way it reports an unparseable template.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Review the generative AI contribution guidelines
• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests
• Write/update functional tests if needed
• make pr passes
• make update-reproducible-reqs if dependencies were changed
• Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 94ee3f3..ab3e2d6
Files: 2
Comments: 2

[aws-sam-tooling-bot]

[ERROR_HANDLING] exc_raise=OSError discards the original exception, which defeats the exc_info=e on the new warning at line 93.

In samcli/lib/utils/retry.py the final raise sits after the loop, outside the except handler:

while remaining_attempts >= 1:
   try:
       return func(*args, kwargs)
   except exc:
       time.sleep(math.pow(2, retry_attempt)  delay)
       ...
raise excraise(exc_raise_msg)   # no active exception -> context is None

So what reaches the new handler is a bare OSError("File cannot be read.") with no __cause__/__context__ and a traceback that only spans retry.py. The logged warning therefore contains neither the errno, the winerror, nor the exception type — exactly the details (PermissionError: [Errno 13]) needed to tell a transient atomic-save lock apart from a real permissions problem or an unreadable network path. Users hitting this will report a warning that says nothing actionable.

Since retry has no way to preserve the original, capture it in _parse_file before the decorator swallows it:

@retry(exc=OSError, exc_raise=OSError, exc_raise_msg="File cannot be read.")
def parsefile(self) -> Dict[str, Any]:
   try:
       return parse_yaml_file(str(self._path))
   except OSError as e:
       LOG.debug("Failed to read file %s, retrying.", self._path, exc_info=e)
       raise

That keeps the retry behaviour while making the underlying error visible at debug level.

[lazerg]

You are right, I checked and the warning only showed OSError: File cannot be read. with a traceback ending in retry.py. The helper raises after the loop, so the real error is gone.

I dropped the helper and put a small retry loop in _parse_file that re-raises the last PermissionError. The warning now carries PermissionError: [Errno 13] Permission denied along with the path. Commit 4e31182.

[aws-sam-tooling-bot]

[PERFORMANCE] The retry sleeps block watchdog's shared dispatch thread, so a locked template file now delays code-sync events too.

HandlerObserver extends watchdog's Observer (samcli/lib/utils/path_observer.py:130), and WatchManager schedules 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 — including TemplateTrigger._validator_wrapper and every code-change trigger — are invoked serially from the single observer dispatch thread. time.sleep inside _parse_file therefore stalls dispatch for all watched resources, not just the template.

With attempts=3, delay=0.05 the waits are 0.1s + 0.2s + 0.4s ≈ 0.7s, and note that retry sleeps 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:

• Pass a shorter budget explicitly rather than relying on the defaults, e.g. @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.
• Retrying on all of OSError also covers non-transient cases such as FileNotFoundError (reachable as a race after the self._path.exists() check) and IsADirectoryError. 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 to PermissionError — the case described in the issue — avoids both the wasted delay and the wrong message.

[lazerg]

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.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 94ee3f3..4e31182
Files: 2
Comments: 2

[aws-sam-tooling-bot]

[ERROR_HANDLING] The retry only covers PermissionError, but the new handler swallows all of OSError, so the other transient failure mode of an atomic save is now quietly dropped instead of retried.

validate_file does a check-then-act: self._path.exists() and then open() inside parse_yaml_file. Editors that save by writing a temp file and replacing the target leave a window where the path is momentarily absent, which surfaces as FileNotFoundError (a subclass of OSError) rather than PermissionError. In that case _parse_file re-raises on the first attempt, validate_file logs the warning and returns False, and the template edit is not synced until the user saves the file again — the same user-visible symptom this PR is fixing, minus the crash.

Since the non-existent-file case is already handled by the exists() check above, retrying the whole OSError family only widens the fix to the race window:

except OSError:
               remaining_attempts -= 1
               if not remaining_attempts:
                   raise
               time.sleep(delay)
               delay = 2

If retrying every OSError is too broad, (PermissionError, FileNotFoundError) covers both halves of an atomic save.

[lazerg]

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.

[aws-sam-tooling-bot]

[PERFORMANCE] time.sleep here runs on watchdog's single event-dispatch thread, so a locked template file also delays code-sync events.

HandlerObserver extends watchdog's Observer (samcli/lib/utils/pathobserver.py:130), and WatchManager schedules every code trigger and template trigger on that one observer (watch_manager.py:172, watch_manager.py:194). Watchdog's emitters are per-watch threads, but they only enqueue events; the observer thread dequeues and calls each handler's dispatch synchronously. _validator_wrapper (resource_trigger.py:133) therefore executes on that shared thread, and the retry adds up to 0.3s (0.1 + 0.2) of blocking per file event. With several stacks each getting an event for the same save, the stalls are serialized, and any queued code-file events wait behind them.

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.

[lazerg]

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.