Skip to content

Send the cloud every Evidence value apart from the reserved names - #72

Merged
jwrosewell merged 12 commits into
rcfrom
fix/cloud-sends-all-evidence
Nov 11, 2026
Merged

jwrosewell merged 12 commits into
rcfrom
fix/cloud-sends-all-evidence

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A Cloud Request Engine on a cloud-only pipeline never sees most of the
Evidence a caller added, because the Evidence store throws it away before
processing starts.

pipeline/fiftyone.pipeline.core/evidence.js, add, lines 137 to 160 on
rc at a8f1c227,
walks the pipeline's elements, sets keep only where some element's
evidenceKeyFilter.filterEvidenceKey(key) is true, and writes
this.evidenceStore[key] = value on line 153 only inside if (keep). The
else branch on lines 154 to 159 logs "filtered out of evidence. Not added."
and the value is gone.

On a cloud-only pipeline the only element is the Cloud Request Engine, and
its filter is a BasicListEvidenceKeyFilter built from the cloud's
evidencekeys answer, set in getEvidenceKeys at
cloudRequestEngine.js line 496.
So a key the cloud has never heard of is thrown away before the engine can
read it, and getContent at
line 522,
which reads flowData.evidence.getAll(), cannot see it either.

getContent itself was already right. The loss happens earlier.

What is lost depends on the other elements in the pipeline. The Set Headers
Element has no filter of its own, so it keeps the base filter, which accepts
every key, and the JavaScript Builder's filter accepts every query. and
header. key. So a pipeline built by PipelineBuilder with its defaults
keeps everything and sends it. A pipeline built with
useSetHeaderProperties: false loses the cookie. evidence and any other
prefix the cloud does not list, and one built with
addJavaScriptBuilder: false as well loses every key the cloud does not
list. What the cloud sees therefore changes with elements that have nothing
to do with the request, which the fix below removes.

The decision

James Rosewell, 10 November 2026.

Everything but reserved. The spec should make this clear. It's done
because things change and knowing about other evidence is important to
continued optimisations.

What the specification says

The Processing section of
cloud-request-engine.md at e89679e9,
the specifications rc head that holds
specifications#138,
merged on 11 November.

The Engine processes Flow Data by making an HTTP request to the server
with every Evidence value in the Flow Data, apart from the reserved names
and the web request view below.

The Engine does not limit the request to the accepted Evidence that the
evidencekeys request returns, so it also sends Evidence the server does
not list. What the server uses changes over time, and knowing about the
other Evidence is important to continued optimization.

Evidence under any prefix whose key strips to values, resource,
license or licence, in any letter case, MUST therefore be left out of
the request, because sending it would put a second field of the same name,
or a second spelling of the same credential, in the request with no way for
either side to know which one applied.

The web request view that a web integration adds under
server.http-request, as web integration describes, MUST also be left out
of the request, in any letter case, because it is a view of the whole web
request and not a value to send.

The same page also says the accepted Evidence "is what the Engine advertises
to the Pipeline, for example for the web Vary header, and it does not limit
the Evidence the Engine sends".

Why the Evidence key filter was not widened

The obvious change is to make the Cloud Request Engine's
evidenceKeyFilter accept every key, so add keeps everything. That was
not done, for two reasons.

First, the filter is what the Engine advertises to the Pipeline. A web
integration reads it for the Vary and ETag headers, and
web-integration.md
lets a web integration add only the accepted values, so widening the filter
would silently widen what a web integration sends from the browser. Web
integrations are unchanged by this work.

Second, other things read the accepted Evidence. An engine's cache key is
evidenceKeyFilter.filterEvidence(flowData.evidence.getAll())
(pipeline/fiftyone.pipeline.engines/engine.js lines 91 and 128), usage
sharing walks flowData.evidence.getAll() directly
(pipeline/fiftyone.pipeline.engines.fiftyone/shareUsage.js line 186), and
the JavaScript Builder filters getAll() for the values it writes into the
script (pipeline/fiftyone.pipeline.core/javascriptbuilder.js line 335).
Widening the filter, or widening getAll(), would change all three at once.

So the filter stays as it is, getAll() stays the accepted Evidence alone,
and the Engine reads the set aside Evidence as well when it builds its
request.

One consequence of keeping the cache key as it is. Two requests that differ
only in Evidence the cloud does not list now post different bodies but share
one cache key, so the second is served the first's answer, which could not
happen before because the two bodies were the same. That follows the
Caching page at e89679e9,
which builds the key from the Evidence each element advertises.

All Flow Elements MUST advertize the Evidence keys that they make use of.
You will need to use this to build a list of the relevant Evidence keys and
values that are present in the Flow Data.

The cloud lists the Evidence it reads, and the rest is sent so the cloud can
learn from it, as the decision above says, so the key stays the advertised
Evidence.

What changed

Three files.

  1. pipeline/fiftyone.pipeline.core/evidence.js. add now keeps a key
    no element accepts in a second store, unacceptedStore, rather than
    throwing it away, and a new getUnaccepted() gives it back. getAll()
    is unchanged and still returns the accepted Evidence alone, so the cache
    key, usage sharing and the JavaScript Builder behave exactly as before.
    The two stores are kept apart, so a key is in one or the other and never
    both. The debug log line for a key no element accepts now says it was set
    aside rather than not added.
  2. pipeline/fiftyone.pipeline.cloudrequestengine/cloudRequestEngine.js.
    getContent reads a new sendableEvidence(evidence) rather than
    flowData.evidence.getAll(). sendableEvidence merges getAll() and
    getUnaccepted(), with the accepted value winning, and leaves out anything
    isSent(key) refuses, which is server.http-request in any letter case.
    The reserved names, resource, license, licence and values under any
    prefix in any letter case, are left out by addQueryData, through the
    check node#61 put
    there. sendableEvidence reads
    getUnaccepted only where the method exists, so an older
    fiftyone.pipeline.core still works and simply sends what it has.
  3. The two types/*.d.ts files, so the published types match.

Checks made

  • Every exit path of each changed function. add has two branches and
    both now write, one to each store. sendableEvidence has one exit and
    returns an empty object where there is no evidence, which is what
    getContent made of an empty store before. isSent has one exit, false
    for the web request view and true for anything else. getContent
    is unchanged below its first line.
  • The callers reach production, not only the tests.
    CloudRequestEngine.processInternal calls getContent at
    line 412
    on every data request, and sets resource, license and values on the
    body after getContent has run, so the engine's own values are the ones
    sent. Evidence.add is reached from addObject, from addFromRequest
    (line 285) and from any caller adding evidence by hand.
  • Nothing else depends on an unaccepted key being thrown away. Usage
    sharing, the engine cache key and the JavaScript Builder all read
    getAll(), which is unchanged. Evidence.get(key) still reads the
    accepted store alone, and every caller of it asks for a key its own
    element accepts, so no caller sees a new value. FlowData.getEvidenceDataKey
    filters getAll() and is unchanged. The JSON Builder reads element data
    and no evidence store.
  • A local run of the real classes on the branch, with a stand-in element
    whose filter is a BasicListEvidenceKeyFilter over a cloud evidencekeys
    answer of header.user-agent and query.user-agent, nine keys added.
    getAll() holds header.user-agent alone, getUnaccepted() holds the
    other eight, sendableEvidence gives the four that may be sent, and the
    body the engine would post is 51D_experiment, sec-ch-ua-platform,
    user-agent and my-own-thing, with no reserved name and no
    http-request. That run uses the real Evidence,
    BasicListEvidenceKeyFilter and CloudRequestEngine classes with no
    network call, so it shows the store and the body and not the wire format.
    The full test run on a runner is below.

Tests

pipeline/fiftyone.pipeline.cloudrequestengine/tests/cloudAllEvidence.test.js
is new. It builds a pipeline holding the engine alone, with
addJavaScriptBuilder: false and useSetHeaderProperties: false, over a
request client that keeps each post, and waits for the engine to be ready
before adding evidence, so the engine's own filter is the cloud's key list
and nothing else accepts anything. Six tests.

  • query.my-own-thing, header.sec-ch-ua-platform and
    cookie.51D_experiment are all in the body, none of which the stand-in
    cloud lists.
  • header.User-Agent, which the cloud does list, is still sent.
  • Query evidence still wins over header and cookie evidence of the same
    name, whether the cloud lists the name or not.
  • resource, license, licence and values from Evidence are never in
    the body, in any case under any prefix, and the engine's own resource key,
    licence key and property list are.
  • server.http-request is never in the body.

The tests came first, in cef0322, with the fix in 45e82ce. The first
red run of cef0322,
run 42980282522,
failed only the reserved name and web request tests, because the evidence
was added before the cloud's evidencekeys answer arrived and the default
pipeline's other elements accepted every key, so nothing was filtered out.
58dc2d5 makes the test wait for the engine to be ready, and 02a11cc
leaves the other elements out and answers the accessible properties post a
license key makes.

Every temporary run below sits in one workflow file shared by all the
temporary runs, so its page carries the title "Temporary red run" and the
name of the first branch that used the file. Each run's own branch is
named here.

  • Red, run 42981085847
    on branch tmp/node-cloudev-red3, the test file at 02a11cc on the rc
    code at c32a2905. 5 of 6 fail, the first
    because the three keys are thrown away in add and the body carries none
    of them. The one that passes, a reserved name under any prefix and case,
    passes because on rc those keys are thrown away as well.
  • Green, run 42981101476
    on branch tmp/node-cloudev-green2, the same file on 02a11cc, 6 of 6
    pass.
  • The order of the sendable Evidence. addQueryData keeps the first value
    it meets for a name, so sendableEvidence now takes the accepted values
    first, and a set aside value never replaces an accepted one of the same
    key. Red, run 42994362197
    on branch tmp/node-cloudev-red4, the test commit e8c048b, where the
    new test fails and the other 6 pass. Green,
    run 42994380869
    on branch tmp/node-cloudev-green3, the head 41809f6, 7 of 7 pass.
  • Full build on 41809f6, success, run 42996681660. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 1004 of 1006 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7 Setting 'PullRequestSha' lines are 41809f61c66dc3fe8c8f0a9381601f54bd153315.
  • Names that every object has. sendableEvidence and addQueryData tested a name with in, which also finds the members every object has, so evidence named constructor, toString or valueOf was taken as already present and not sent. Both now ask whether the object has a member of its own by that name. Red, on branch tmp/node-72-red5, run 43024737099, the test commit ad1def7, where the new test fails and the other 7 pass. Green, on branch tmp/node-72-green4, run 43024752879, the head 82df729 with this file and cloudRequestEngine.test.js, 29 of 29.
  • The build of 82df729 was cancelled once Read and store evidence keys without regard to letter case #71 and Never send resource, license, licence or values from Evidence to the cloud #61 merged, as the draft was about to take rc.

After node#71 and node#61 merged:

  • 2cfe69f merges rc at b9a4d29. The set aside store is now keyed through Read and store evidence keys without regard to letter case #71's lower-case table, so two spellings of one key no element accepts are one entry, under the first spelling. get still reads the accepted store alone, and asks whether that store holds the key itself, because the table now names set aside keys too.
  • a3eaf6d tests that two spellings of one set aside key are sent once. Red, on branch tmp/node-72-red6, run 43033990673, the test file on 82df729's code, where the new test fails and the other 8 pass. Green, on branch tmp/node-72-green5, run 43034023473, the head with this file, cloudRequestEngine.test.js and evidenceKeyCase.test.js, 37 of 37.
  • e587c40 drops this draft's own reserved name check from isSent in favour of Never send resource, license, licence or values from Evidence to the cloud #61's in addQueryData. What reaches the cloud is unchanged, and the reserved name tests in this draft still pass against Never send resource, license, licence or values from Evidence to the cloud #61's check in the green run above.
  • Full build on the head e587c40, success, run 43034641588. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 1188 of 1190 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7 Setting 'PullRequestSha' lines are e587c40a8dcf33a5f13885dca18574c0410b1f79. rc has since taken a README change only, and the draft still merges cleanly.

What remains

  • Python has the same fault and its own draft.
  • This lands in this release. It changes what every cloud pipeline sends,
    which is James Rosewell's decision quoted above, that a Cloud Request
    Engine sends every Evidence value apart from the reserved names.

macOS 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.

…in the all evidence tests

Evidence added before the cloud's evidencekeys answer arrives passes the engine's default filter, so the test of evidence the cloud does not list passed without the fix. Waiting for ready gives the engine the filter a running service has, and the test then fails without the fix.
…gine alone

The Set Headers Element accepts every evidence key and the JavaScript Builder every query and header key, so in a default pipeline nothing is filtered out and the tests passed without the fix. With both left out, the engine's own filter decides, and the tests fail without the fix. The recording client also answers the accessible properties post a license key makes.
… Engine sends

addQueryData keeps the first value it meets for a name, so the order of the sendable evidence decides which of two values with one name is sent. The accepted values now come first, and a set aside value never replaces an accepted one of the same key.
sendableEvidence and addQueryData tested a name with in, which also finds the members every object has, so evidence named constructor, toString or valueOf was taken as already present and never sent. Both now ask whether the object has a member of its own by that name.
# Conflicts:
#	pipeline/fiftyone.pipeline.core/evidence.js
#	pipeline/fiftyone.pipeline.core/types/evidence.d.ts
rc's addQueryData now leaves out Evidence whose name is resource, license, licence or values, so isSent drops its own copy of that check and leaves out the web request view alone. What reaches the cloud is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant