Skip to content

fix: Report the real error when the flag push request cannot be built - #71

Merged
keelerm84 merged 1 commit into
mainfrom
mk/SDK-2822/push-request-errors
Aug 31, 2026
Merged

fix: Report the real error when the flag push request cannot be built#71
keelerm84 merged 1 commit into
mainfrom
mk/SDK-2822/push-request-errors

Conversation

@keelerm84

@keelerm84 keelerm84 commented Aug 31, 2026

Copy link
Copy Markdown
Member

Summary

featureLoop builds its push request from a URI assembled at run time, so construction can
fail on a malformed SALESFORCE_URL. It answered that failure with a fixed string of its
own -- misspelled as Failed constructiong flag push request -- and discarded the error
http.NewRequest produced, which for a malformed URL is the only thing worth knowing. The
loop's poll already returns the underlying error, so the push now agrees with it.

The log line stays alongside the return, deliberately. run() reads one error from each
loop and returns only the first to arrive, so an error from this site reaches the operator
only when featureLoop wins that race. Measured with a malformed SALESFORCE_URL and a
200ms flag poll latency, eventLoop won 40 of 40 runs and the parse error appeared nowhere
at all. A comment records this, so the line is not removed again on the reasoning that the
return already carries it. (It was removed on exactly that reasoning in an earlier revision
of this PR, and a review caught it.)

Also fixes the neighbouring log line, which read failed pushings flags to salesforce. That
one prints a client.Do error, which net/http redacts via stripPassword, so its content
is unchanged.

Scope reduced since the first review

eventLoop's push had the same defect, and that half has since landed with #69, which
rewrote the surrounding block. Only featureLoop remains here.

The test covering eventLoop is kept rather than dropped: it now guards the version of that
code which arrived by the other route, and it still passes.

Tests

A test per loop. A control character is what url.Parse refuses, which is how each forces
the failure, and each asserts the failure is reached before asserting anything about it --
so a future stdlib that accepts these URLs fails loudly rather than passing vacuously.

Verified discriminating three ways: against the original fixed strings; against a same-base
parse error with a different suffix, so the assertion pins which URI is named rather than
merely that an error occurred; and against mutating only the return while keeping the log
line, which confirms the assertion is on the returned error rather than the log.

The expected text is derived from a live http.NewRequest call rather than hard-coded,
because the two toolchains render a DEL byte differently in %q output -- Go 1.15 uses the
Unicode escape form and 1.26 uses the hex form, and CI pins 1.15. Green on both, and under
-race -count=20 and -shuffle=on.

Information disclosure

Returning http.NewRequest's error means the raw URL reaches log.Fatal, and url.Parse
does not redact userinfo the way client.Do does. This was reviewed specifically and found
immaterial: the variables whose parse failures can leak all pre-set Authorization, so
userinfo in them is discarded by Go and that configuration cannot work; the one variable
where userinfo is functional (OAUTH_URI) has no reachable leak path; and the category
already exists on main independent of this change. Nothing more sensitive than the
configured URL can reach these errors -- the parse happens before the body is even
type-asserted, and every Header.Set runs after construction succeeds.

@keelerm84
keelerm84 requested a review from a team as a code owner August 31, 2026 15:36
keelerm84 added a commit that referenced this pull request Aug 31, 2026
The retry loop kept the fixed string this site used before, which discards what
http.NewRequest said about the URI. #71 replaces that same string on main, and this
branch rewrote the surrounding block, so a merge would have put the two in conflict with
the fixed string as one of the candidate resolutions.

Carrying the change here instead makes the merge order stop mattering: both sides now
agree, so nothing is left for a resolution to drop.
@keelerm84
keelerm84 requested a review from a team August 31, 2026 17:34
featureLoop builds its push request from a URI assembled at run time, so construction can
fail on a malformed SALESFORCE_URL. It answered that with a fixed string of its own --
misspelled as "Failed constructiong flag push request" -- and discarded the error
http.NewRequest produced, which for a malformed URL is the only thing worth knowing. The
loop's poll already returns the underlying error, so the push now agrees with it.

The log line stays alongside the return, and that is deliberate rather than redundant.
run() reads one error from each loop and returns only the first to arrive, so an error from
this site reaches the operator only when featureLoop wins that race. Measured with a
malformed SALESFORCE_URL and a 200ms flag poll latency, eventLoop won 40 of 40 and the
parse error appeared nowhere at all. A comment records this so the line is not removed
again on the reasoning that the return already carries it.

Also fixes the neighbouring log line, which read "failed pushings flags to salesforce".
That one prints a client.Do error, which net/http redacts, so its content is unchanged.

eventLoop's push had the same defect. That half landed with the event push retry work,
which rewrote the surrounding block, so only featureLoop remains here. The test covering
eventLoop is kept: it now guards the version of that code which arrived by the other route.

A test per loop. A control character is what url.Parse refuses, which is how each forces
the failure, and each asserts the failure is reached before asserting anything about it, so
a future stdlib that accepts these URLs fails loudly rather than passing vacuously.
Verified discriminating against the fixed strings, and against a same-base error with a
different suffix -- the assertion pins which URI is named, not just that an error occurred.
@keelerm84
keelerm84 force-pushed the mk/SDK-2822/push-request-errors branch from eef15a2 to 2c3239f Compare August 31, 2026 18:38
@keelerm84 keelerm84 changed the title fix: Report the real error when a push request cannot be built fix: Report the real error when the flag push request cannot be built Aug 31, 2026
@keelerm84
keelerm84 merged commit 860b311 into main Aug 31, 2026
5 checks passed
@keelerm84
keelerm84 deleted the mk/SDK-2822/push-request-errors branch August 31, 2026 18:42
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.

2 participants