[jwrosewell]

The data update specification says a background failure is caught and logged, but not what happens to the Engine's data or to later checks when a refresh fails. Each language read that differently, and in three of them a failed refresh ended the process or stopped all later updates, as 51degrees-cxx#61 records. This change states the behavior that the native code and the fixes now provide, and says which checks are reported to the update callbacks and with what status.

What changes in pipeline-specification/features/data-updates.md

1. Aspect Engine features. A refresh whose new data source cannot be used MUST fail with an error, and the Engine MUST go on using the data source it had.
2. newFileAvailable.
   • On a failed load the Engine keeps its data, and the failure is logged and reported to onUpdateCallbacks as a failure, not a success.
   • Polling and the file watcher resume whether or not the file loaded.
   • newBufferAvailable says the same for memory.
3. Errors. "Will need to be caught and logged" becomes a MUST. A failure MUST NOT end the process, and MUST NOT stop later checks for updates.
4. Reporting each check, a new section under Update processes, following the .NET data update service exactly. A check MUST be reported to onUpdateCallbacks once it ends, including the checks made by update polling, by the file watcher, by the update on start-up and by a programmatic call, with one status for the data file, using .NET's status names.
   • Success (AUTO_UPDATE_SUCCESS).
   • Not needed (AUTO_UPDATE_NOT_NEEDED), for a 304 (Not modified) response, a downloaded or watched file that is not newer, or a check with no update URL set and no newer file.
   • In progress (AUTO_UPDATE_IN_PROGRESS), for a file watcher event that found nothing left to do, being one that waited while another event for the same file was applied, or one from a watcher that does not belong to exactly one registered data file.
   • A failure status, as the Errors table lists.
   • Two calls report nothing and give their result to the caller. One is a programmatic check for a data file with no configuration (AUTO_UPDATE_NO_CONFIGURATION), and the other is an update from memory.
5. The update processes. newFileAvailable, Programmatic update and Update polling point to the new section, and newFileAvailable triggers onUpdateCallbacks "with success". Programmatic update now reports not needed on its polling branch as well as at the end, returns the no configuration status with no report, and on a failure reports it and passes the error to the caller without logging it. newBufferAvailable says a failure with a polled buffer is handled as for newFileAvailable, and a failure of an update from memory fails that call and is not logged or reported.
6. Errors. Only a failure in a check started by update polling or by the file watcher is caught and logged. A programmatic check, the update on start-up and an update from memory pass the error to their caller. The table now gives .NET's messages, levels and statuses, as below, and the note on highlighted text ("Update will be attempted again later") is gone, as .NET has no such text.
7. Overview. A package whose processes last for one request, such as PHP's, has no data update service, and the Aspect Engine features still apply to it.
8. Errors, a callback that raises an error. A new row before the last one gives what .NET does. On update polling and the file watcher the error is logged at Error with the last row's message, An unhandled error occurred while checking for automatic updates for engine '<Engine type name>', and on a programmatic check and the update on start-up it passes to the caller. The status was already reported either way. The paragraph above the table says the same. .NET writes no message of its own for this case.

The behaviour this records

The failed refresh rules in points 1 to 3 record what the native code and the language fixes already do, with no decision from James Rosewell needed. Points 4 to 7 are his decisions of 10 November. The specification follows the .NET data update service exactly, for reporting and for the error messages and levels ("This is all about alignment."), and PHP is exempt from data updates. 51degrees-cxx 4.5.100 makes a reload that fails keep the data set in use, through 51degrees-cxx#60. GitHub shows that pull request as closed rather than merged, but its commit f4d10614 is on 51degrees-cxx main. Before that, a reload of a broken file could swap in a half built data set, and the next lookup crashed.

How .NET reports and logs each check

At 51degrees-dotnet b3e90b4, its rc on 10 November, DataUpdateService.cs raises CheckForUpdateComplete with a DataUpdateCompleteArgs holding the data file and an AutoUpdateStatus.

• Polled and programmatic checks report in the finally block of CheckForUpdate (lines 891 to 921). A check that finds no new data is AUTO_UPDATE_NOT_NEEDED (lines 855 to 858), and so is a 304 response (lines 1374 to 1375). A failure is a DataUpdateException carrying its status, which the catch blocks rethrow after setting the status (lines 878 to 889).
• The timer catches that exception, logs it with LogUpdateException and then raises the collected reports (lines 698 to 777). LogUpdateException logs a DataUpdateException at Error as Error during check for data update and any other exception at Error as An unhandled error occurred while checking for automatic updates for engine '{0}' (lines 667 to 689, and Messages.resx).
• A programmatic check goes through the same CheckForUpdate with no timer around it, so the failure is reported and then rethrown to the caller, and is not logged. For a data file with no configuration the public CheckForUpdate returns AUTO_UPDATE_NO_CONFIGURATION without raising the event (lines 340 to 363).
• The update on start-up calls the same CheckForUpdate from RegisterDataFile, which does not catch, so its error reaches the caller. A status other than success or not needed without an error throws Update on startup failed for {0} with status {1}. See log for details (lines 205 to 230).
• A file watcher event starts with AUTO_UPDATE_IN_PROGRESS (line 549). It is not needed when the file is not newer (line 608), which is checked before the lock and again inside it (lines 565 to 569), so an event that waited for the lock while another event for the same file was applied keeps AUTO_UPDATE_IN_PROGRESS. A file it cannot open within 10 minutes is AUTO_UPDATE_TIMEOUT (line 577). A failure is logged with LogUpdateException and every event is reported (lines 612 to 626).
• A callback that raises an error. OnUpdateComplete invokes CheckForUpdateComplete with no catch of its own (line 458). On update polling the reports are raised inside a try whose catch calls LogUpdateException (lines 744 to 754), and on the file watcher DataFileUpdated does the same (lines 519 to 523), so the error is logged with the last row's message. On a programmatic check and the update on start-up the report is raised in the finally of CheckForUpdate (line 917) with nothing around it, so the error passes to the caller.
• UpdateFromMemory raises no event. A refresh failure throws a DataUpdateException to the caller, and a failure to write the data file is a warning, after which the Engine is still refreshed (lines 381 to 435).

The Errors table takes every message from these lines. Each DataUpdateException message is at line 425, 984, 1012, 1070, 1214, 1314, 1324, 1372, 1382, 1386 and 1461, and the two warnings are at line 406 and line 1357. Two differences are deliberate. The integrity check message in .NET has no space between the quoted Engine type name and "data update", and the table has one. The refresh message from UpdateFromMemory ends with a full stop and the one from update polling does not, and the table gives the second. The old table's rows for a download error, a write to the data file or temporary file, and a locked file with the temporary file option disabled have no .NET message, so they are gone. Such a failure is logged as any other error.

What each package must change

Language	State
C	Done in 51degrees-cxx 4.5.100, as above.
Node	Done in 51degrees-node#2, merged 3 November.
.NET	Done in 51degrees-dotnet#33, merged into rc on 3 November, which keeps the data update service watching after a refresh fails. The earlier drafts pipeline-dotnet#464 and ip-intelligence-dotnet#405 are in repositories that are now archived.
Go	Done in 51degrees-go#2 (the file watcher stops without blocking), 51degrees-go#7 and 51degrees-go#17 (a reload that fails is reported by its status, and pulling goes on), all merged 2 November. The earlier drafts of the same names, common-go#4, device-detection-go#105 and ip-intelligence-go#202, were closed.
Python	Done. The file watcher logs a failed refresh, reports it as AUTO_UPDATE_REFRESH_FAILED and schedules the next check whatever happens (datafile_update_service.py lines 297 to 338). The earlier draft pipeline-python#118 was closed with its repository archived.
Java, PHP and Rust	51degrees-cxx#61 checked these and found none that treats a failed reload as fatal. Rust retries one interval after a failed poll. Whether each reports the failure, not success, to its update callbacks was not checked here.
Reporting each check and the Errors table (points 4 to 6)	Only .NET was read for this pull request, as above. Java, Node, Python, Go and Rust change to .NET's messages and levels in their own pull requests this release, and to the reporting rule next week.
PHP	Exempt from the data update service (point 7).

Two of the faults these fix are now ruled out by the text. .NET stopped its update timer after a failed refresh until a restart. Node reported a success straight after a failure.

Review at d8b3a9c, and what changed at 8d43c73 and 95d6c97

The review of d8b3a9c found three MINOR points, fixed in 8d43c73, and the review of 8d43c73 asked that the two rows it added or kept outside .NET follow .NET, done in 95d6c97.

1. The integrity check row now gives .NET's message exactly, with no space between '<Engine type name>' and data update, because .NET concatenates the two parts with none (lines 1463 and 1464). Every row is therefore .NET's, as the paragraph above the table says.
2. The row for refreshing from data held in memory now ends with the full stop .NET writes (line 426).
3. The In progress bullet is wider. .NET reports AUTO_UPDATE_IN_PROGRESS when an event waited for the lock and then found the file already applied (lines 565 to 569), and when no single data file owns the watcher, because a null data file skips the whole body (line 556) and the report at line 622 still runs. The review also named an event whose file is not newer at the outer check, but that event takes the else at lines 606 to 609 and reports AUTO_UPDATE_NOT_NEEDED, which the Not needed bullet already covers, so it is not in the In progress bullet.

The callback row (point 8) was added in 8d43c73 with a message .NET does not write, and 95d6c97 changed it to .NET's behaviour and took out the note that two rows differed from .NET. The recheck of 95d6c97 found that a callback's error that is a DataUpdateException is logged as Error during check for data update (lines 673 to 676), and that .NET stops at the first error, so the callbacks after it and the timer's remaining reports are skipped (lines 744 to 754). 176a520 says both in the paragraph above the table, and rc at 96bd70a is merged in.

Not changed

The messages at start-up and during operation are unchanged. #55, which masks license keys in the <url> of these messages, has merged into rc, and this branch has it.

How it was checked

The state of each pull request was read from GitHub, and the Python code at the commit linked. rc at 67ead69 is merged into the branch with no conflict, and the diff against rc is only the change above. The .NET lines above were read at b3e90b4, and DataUpdateService.cs is the same at a1454289, the commit the review used. The base is now rc, because the "Only rc into main" check refuses a pull request into main from any branch but rc. There is nothing to build or run in this repository.

Outstanding

• Review of the wording.
• A check of Java, PHP and Rust against the failure reporting in point 2.
• The language changes for points 4 to 6.
• .NET's integrity check message has no space before data update. The table follows .NET as it is, so any fix starts in .NET and the table follows it.
• Node and Python change to the callback row's behaviour in their own drafts. 51degrees-node#65 at 41e3b662 logs the error as it is (this.log('error', e)), not with the last row's message. Python was not read for this row.