fix: make is_package() read timeout configurable, default 30s#443
Conversation
Without a read timeout, a stalled HTTPS connection to Apple's CDN (cvws.icloud-content.com) would block the sync process indefinitely with no recovery path (read timeout=None in urllib3). Adds drive.request_timeout config option (default: 30s). The timeout is threaded through collect_file_for_download and process_file to is_package(), which passes it to item.open() via the existing **kwargs passthrough in DriveNode.open() -> DriveService.get_file() -> session.get().
|
Hi @mandarons — just wanted to flag a couple of things: Tests pass. The Production context. This fix came from a real incident — my sync container was stuck in a kernel Happy to adjust anything if you'd like changes before merging. Thanks for maintaining the project! |
There was a problem hiding this comment.
Pull request overview
Adds a configurable timeout for the iCloud Drive “package detection” request path to prevent indefinite hangs when probing items via is_package(), wiring the value from config through the drive sync call chain.
Changes:
- Introduces
drive.request_timeoutwith a default of 30 seconds (DEFAULT_REQUEST_TIMEOUT_SEC). - Adds
config_parser.get_drive_request_timeout()and threads the resolved timeout intois_package(..., timeout=...). - Adds/updates tests and test config fixtures covering default/value behavior and
ReadTimeouthandling.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/__init__.py |
Adds the default request-timeout constant. |
src/config_parser.py |
Adds a getter for drive.request_timeout. |
src/drive_file_existence.py |
Applies the timeout to item.open() in is_package(). |
src/sync_drive.py |
Resolves timeout from config and passes it to is_package() in legacy process_file. |
src/drive_sync_directory.py |
Threads config through file processing to reach timeout resolution. |
src/drive_parallel_download.py |
Resolves timeout from config and passes it to is_package() during task collection. |
config.yaml |
Documents the new drive.request_timeout option. |
tests/data/test_config.yaml |
Adds request_timeout to the test fixture config. |
tests/test_config_parser.py |
Adds tests for explicit/default/None-config timeout retrieval. |
tests/test_sync_drive.py |
Adds test ensuring ReadTimeout is handled and timeout is passed to open(). |
| def get_drive_request_timeout(config: dict) -> int: | ||
| """Return drive request timeout from config. | ||
|
|
||
| Args: | ||
| config: Configuration dictionary | ||
|
|
||
| Returns: | ||
| Request timeout in seconds (default: DEFAULT_REQUEST_TIMEOUT_SEC) | ||
| """ | ||
| config_path = ["drive", "request_timeout"] | ||
| return get_config_value_or_default( | ||
| config=config, | ||
| config_path=config_path, | ||
| default=DEFAULT_REQUEST_TIMEOUT_SEC, | ||
| ) |
| files.add(local_file) | ||
| item_is_package = is_package(item=item) | ||
| timeout = config_parser.get_drive_request_timeout(config) | ||
| item_is_package = is_package(item=item, timeout=timeout) | ||
| if item_is_package: | ||
| if package_exists(item=item, local_package_path=local_file): |
| # Thread-safe file set update | ||
| with files_lock: | ||
| files.add(local_file) | ||
|
|
||
| item_is_package = is_package(item=item) | ||
| timeout = config_parser.get_drive_request_timeout(config) | ||
| item_is_package = is_package(item=item, timeout=timeout) | ||
| if item_is_package: |
| # HTTP read timeout in seconds for iCloud API requests (default: 30) | ||
| # Prevents the sync process from hanging indefinitely on stalled connections |
| """Test is_package returns False on read timeout and passes timeout=30 to open().""" | ||
| import requests.exceptions | ||
|
|
||
| from src.drive_file_existence import is_package | ||
|
|
||
| with patch.object(self.file_item, "open") as mock_open: | ||
| mock_open.side_effect = requests.exceptions.ReadTimeout("Read timed out.") | ||
| result = is_package(self.file_item) | ||
| self.assertFalse(result) | ||
| mock_open.assert_called_once_with(stream=True, timeout=30) | ||
|
|
Problem
is_package()insrc/drive_file_existence.pycallsitem.open(stream=True)with no timeout. When Apple's CDN (cvws.icloud-content.com) accepts the TCP connection but never delivers the HTTP response, urllib3 blocks indefinitely (read timeout=None). The sync process freezes with no log output and no recovery path.Observed in production: the sync process was stuck in a kernel
read()syscall on an ESTABLISHED TCP connection to Apple's CDN for weeks, with zero log output and no way to self-recover without a container restart.Root cause
The call chain passes
**kwargsall the way through tosession.get():Because no
timeoutis passed, urllib3 usesNone(infinite wait). A TCP connection that stays ESTABLISHED at the OS level but delivers no HTTP response data will block forever.Fix
Adds a
drive.request_timeoutconfig option (default: 30s). The timeout is threaded throughcollect_file_for_downloadandprocess_filetois_package(), which passes it toitem.open()via the existing**kwargspassthrough — no changes needed in icloudpy.Users who want to adjust or disable the timeout can set
request_timeoutin theirconfig.yaml:Changes
src/__init__.py—DEFAULT_REQUEST_TIMEOUT_SEC = 30src/config_parser.py—get_drive_request_timeout()gettersrc/drive_file_existence.py—is_package(item, timeout=DEFAULT_REQUEST_TIMEOUT_SEC)src/drive_parallel_download.py—collect_file_for_downloadresolves and passes timeoutsrc/drive_sync_directory.py— threadsconfigthrough_process_file_itemsrc/sync_drive.py—process_fileresolves and passes timeoutconfig.yaml— documents the new option (commented out, showing default)Tests
test_get_drive_request_timeout— config with value settest_get_drive_request_timeout_default— config missing the key (returns default)test_get_drive_request_timeout_none_config—Noneconfig (returns default)test_is_package_read_timeout—ReadTimeoutis caught, returnsFalse,open()called withtimeout=30Note: there are 20 pre-existing test failures in
test_sync.pyandtest_usage.pyin the local environment due to a missing/config/path — these fail identically on the unmodifiedmainbranch and are unrelated to this change.