Repository navigation
Update the data file as the specification says - #52
Merged
Merged
Conversation
jwrosewell
force-pushed
the
align/data-update
branch
from
November 9, 2026 16:30
4ec6315 to
f3d93af
Compare
The Data update page of the pipeline specification sets the If-Modified-Since fallback, when the first check is made, update on start-up with no data file, programmatic update of a memory data file and the log messages.
jwrosewell
force-pushed
the
align/data-update
branch
from
November 9, 2026 16:46
fd7a124 to
8490791
Compare
…e status for every update, and read engine dates as midnight UTC The Data update page says no file system watcher is created with autoUpdate disabled. A bad update URL, a download that is cut short and a failed copy or refresh now each report one status with the data file no longer marked as updating, and a download that cannot be removed after a good refresh is logged and reported as a success.
…ublished date A connection reset part way through the download must be reported once and schedule one check. A locked data file, a download that cannot be read, and an MD5 check that cannot read the download are each reported once. An on-premise engine with no data yet asks for the file with no If-Modified-Since header.
…wnload A reset raises an error on the request whilst the response is still arriving, and then closes the response. Both reported the update and each called checkNextUpdate, so the data file then polled on two timers. Each update now ends once, and an error on the request after the response has arrived is left to the response's handlers, which report it as a failed download. The MD5 check's read stream has an error listener, so a failed read is reported once rather than raising an unhandled error.
Once the response has arrived, the request's error handler does nothing, and the output's error handler now checks written as the response's close handler does, so either reports a failed download and the other does nothing. That covers every case the separate ended flag did.
jwrosewell
marked this pull request as ready for review
November 10, 2026 16:19
This was referenced Nov 12, 2026
Draft
Take the data update messages and levels .NET and 51degrees-node write
51Degrees/51degrees-python#98
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the Node data update service in line with the Data update page of the pipeline specification (read in full on
rc). Each change below was first checked against the code onrc.This draft depends on #58, which is now merged into
rcas efbea08. The messages here name the update URL, which carries the license keys, and the specification asks for each key to be masked to its last 4 characters. #58 masks everyPipeline.logentry, which is the route all of this service's messages take, and it keeps a full stop after the URL out of the key, as the "from <url>." messages here need. This draft merges cleanly withrcat efbea08.Other open drafts that touch nearby files are #45 and #46 (evidence). Neither changes
dataFileUpdateService.js,dataFile.jsor the on-premise engines, so there is no overlap with this draft. The Sequence Element and builder changes are in #53.What changed
D1.
If-Modified-Sincefalls back to the file's last modified time.Spec, Update polling: "If the data file does not have a publication date then the file system last modified date/time can be used instead."
Before,
dataFileUpdateService.jsswallowed the error fromgetDatePublishedand sent no header. Now the header is the published date, or the last modified time of the file at the data file's path when there is no usable published date (none, an error, or an invalid date), or no header if neither exists. It is sent as an HTTP date (toUTCString, the IMF-fixdate of RFC 9110 section 5.6.7).The two on-premise engines used to return
new Date()as the published date when they had no data yet, which would have sent "now" as the header. They return nothing instead.D2. Each check waits until the update is expected.
Spec, Automatic update via HTTP: "If the current data contains a data update expected timestamp, then polling will not start until after that time."
Before,
checkNextUpdateaddedgetNextUpdate().getMilliseconds(), which is only the 0 to 999 millisecond part of the date. Now, when the expected time is in the future, the wait is the time until then plus the random offset (as .NET does inDataUpdateService.cs). Otherwise it is the polling interval plus the random offset, as before. The wait is capped at 2147483647 ms, the longestsetTimeoutaccepts, as a longer one fires at once.D3. Update on start-up downloads a missing data file.
Spec, Update on start-up: "This option can be used to bootstrap an Aspect Engine that does not yet have any local data" and, in the process list, "on fail, fall back to local file if any, no local file, fail".
Before, both on-premise engines threw "There is no file at" before any update. Now, when update on start-up is on and a licence key or a custom update URL is set (the same condition that already switches the option off), the engine is built without the file, downloads it, then creates the native engine from it and updates the pipeline's property list.
process()waits meanwhile, as it already did. If the download fails with no file,process()rejects with "There is no file at '...' and the download of a new one ended with the status '...'". With update on start-up off, the build still throws as before. Applies to Device Detection and IP Intelligence.D4.
updateFromMemory(dataFile, buffer).Spec, Programmatic update: "For memory data sources, programmatic update MUST allow provision of the new memory data source."
New public method on
DataFileUpdateService, named as in .NET and Java. It sets the bytes on a memory data file (one with no path) and refreshes it. If the refresh fails the old data is kept and the failure is logged and reported throughupdateComplete. It throws for something that is not aBufferorUint8Array, and for a data file read from a path.D6. Log messages.
Spec, Logging: "These messages MUST also be consistent across languages". The text of every message in the Logging section is used exactly, at the level the page gives (the info messages have no level on the page, so they are
info, and "Warning" iswarn). The "Update will be attempted again later." sentence is added only when the update was started by the service andautoUpdateis on (a new optional second argument toupdateDataFile, defaulting to false so existing callers are unchanged). "Engine type" is the engine's class name, or its data key when the engine has no class of its own.A few repairs were needed to make the logged outcomes true. A failed read or write of the downloaded file used to carry on to a refresh and report success as well, and a failed gunzip threw. These now stop, log, and report one failure. The download is also read once the file is fully written, not when the response ends. A new optional
onUpdateCompletefunction onDataFileis called with each update's status, and the engines use it for D3.Notes for review
errorlevel with their own text. The page makes them "Error downloading file", a warning. A pipeline with nowarnlistener writes nothing for a warning, where it wrote anerrorto the console before. The existing test for this was changed to match.EBUSY. Other write errors use the "copying a data file" message.licenseKeys. Since specifications#55 and #123 merged, the Logging section and logging.md onrcrequire each key masked in every message. #58, now onrc, does that inPipeline.log.swigDateToDatein both engines used to start from the current time, so a published date carried the time of day it was converted. A file published today then sent anIf-Modified-Sinceof the time now, and the wait for the expected update was out by the time of day. It is now midnight UTC on the day the engine reports, with a test in each package.autoUpdateis on. Spec, Automatic update master switch: "With autoUpdate disabled, no file system watcher is created". An engine with no licence key hasautoUpdateforced off, so it no longer watches its file, which the Defaults section intends.updateCompletestatus, reported after the data file has stopped updating. A URL that cannot be parsed, a download that is cut short (reported when the response closes), a failed copy and a failed refresh each report one. A download that cannot be removed after a good refresh is logged and still reported as a success.checkNextUpdate, which left the data file polling on two timers. InupdateDataFilean error on the request after the response has arrived is now left to the response's handlers, and the output's error handler checkswrittenas the response's close handler already did, so whichever of the two comes first reports and the other does nothing. A reset is therefore reported once, as "An error occurred while downloading a data file update", with one scheduled check.errorlistener, so a failed read raised an unhandlederrorevent. It now logs throughwriteFailed(the page's "An error occurred when writing to" message, which the other failed reads of the download use too), removes the download and reportsError Reading Streamonce.Tests
Written first, in
pipeline/fiftyone.pipeline.engines/tests/dataUpdateSpecification.test.js(D1, D2, D4, D6), andupdateOnStartMissingFile.test.jsin the Device Detection and IP Intelligence on-premise packages (D3). Existing tests indataFileUpdateService.test.jsthat checked the old log text were updated.The third set of tests (d4a1801) covers notes 2, 7 and 8 and the D1 fallback in the engines.
A connection reset part way through the download (
request.socket.resetAndDestroy()) is reported once, logs one warning, callscheckNextUpdateonce and leaves no download behind.A data file that is locked (
EBUSY) logs the page's locked file message and reportsCannot Rename New Fileonce, leaving the old file in place.A download that cannot be read logs one error and reports
Cannot Rename Master Fileonce.A download the MD5 check cannot read is reported once as
Error Reading Streamand removed.An on-premise engine with no data yet asks for the file with no
If-Modified-Sinceheader, because itsgetDatePublishedreturns nothing and there is no file to take a date from. These two tests, one in each on-premise package, need the native build, so they run in the full build only.Red, the first set of new unit tests on
rccode (19 of 23 fail, lint passes): https://github.com/51Degrees/51degrees-node/actions/runs/42655195633Red, full build of this draft with the tests only (head
f3d93af). 23 tests fail, being the 19 above and the two start-up tests in each of the Device Detection and IP Intelligence packages, which need the native build. Every other suite passes. The run stopped its other jobs after the first failure: https://github.com/51Degrees/51degrees-node/actions/runs/42655474378Red, the second set of tests (watcher gate, one status per update, midnight UTC dates), on the code before those changes (9 fail): https://github.com/51Degrees/51degrees-node/actions/runs/42677919050
Green, lint and the pipeline and swig helper tests on
fed5dfd: https://github.com/51Degrees/51degrees-node/actions/runs/42677936948Red, the third set of tests on the previous head
fed5dfd(2 of 362 fail). The reset reportedHTTPS Errortwice, and the MD5 check's failed read raised an unhandlederrorevent ("Unhandled error. (Error: cannot read") and reported nothing: https://github.com/51Degrees/51degrees-node/actions/runs/42795471010Green, lint and the pipeline engines and core tests on
64887a5, the first form of the fix (362 passed): https://github.com/51Degrees/51degrees-node/actions/runs/42795497883Green, the same on the head
58743f6, which leaves the one report to the response's handlers without a second flag (362 passed): https://github.com/51Degrees/51degrees-node/actions/runs/42798165406Full build on the head
58743f6. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, Configure printed "Left out 2 macOS entries", and all 7Setting 'PullRequestSha'lines are58743f641baad847988c93eb9f3b76fcbf1c0e08: https://github.com/51Degrees/51degrees-node/actions/runs/42800996321macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
Not done here
memoryrefusal tests (a value that is not bytes, and a data file with a path) pass onrcfor the wrong reason, because the method does not exist yet and calling it throws. They check the error text in the fix commit.