You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Every catch in these files exists so the surrounding work carries on, and each reported the caught failure through the telemetry sink directly. A sink that throws therefore takes the recovery with it — the exact failure mode #196 removed from RedisConnector and RedisPlannedMaintenance, still present in four other places.
TelemetrySafeguards landed with #196, so these can route through it now.
Site
What a refused report costs today
RedisDistributedLock acquire (×2)
The catch exists to degrade acquisition to unavailable. A throw hands the caller an exception instead of the no-op lease, so a telemetry outage becomes a lock failure. The TrackEvent(EventUnavailable) that follows has the same exposure.
RedisDistributedLock release
The throw escapes the release path.
FactoryTimeout
The record sits between the cancellation and throw new TimeoutException(...), so a refused one replaces the exception the caller is documented to receive.
MemoryCacheSetter
The record sits in a finally, where a throw replaces whatever exception is already on its way out — the original cause is lost.
RedisConnectionWarmup
The warm-up is explicitly best effort, and this was the one thing that could make it not be.
What
Each of those routes through TryTrackException / TryTrackEvent. TryTrackException gains an optional properties parameter, since the lock sites carry them.
After this, no raw TrackException remains in src outside TelemetrySafeguards itself.
Review then found the same defect in three more places, and enumerating the rest turned up a fourth:
Site
What a refused report cost
RedisDistributedLock acquired / timeout
Redis had already granted the lock, so a throw past the releaser left it held until it expired; the timeout path threw instead of returning the no-op lease. Every telemetry call in the class is now guarded.
RehydrationCoordinator triggered
A throw skipped rehydrateAsync entirely — the whole point of the spawn.
RehydrationCoordinator timed out / failed
A throw skipped handles = null, so the finally released the per-key locks instead of holding them for the cooldown. deduped and succeeded are guarded too, so no path in the method depends on the sink.
RedisStreamSubjectWriter.HandleInvalidEvent
Reported before adding the id to the acknowledgement list, leaving the poison entry pending forever.
RedisStreamSubjectWriter.TraceReceipt
Same shape, found by enumeration rather than review: ids.Add follows it on the valid path.
Not included
Five raw TrackEvent calls remain, in RedisConnector and RedisPlannedMaintenance. Those files are rewritten by #197, so guarding them here would only conflict; they follow once it merges.
Verification
Release build with -warnaserror clean. Full suite on both target frameworks: net10.0 1846, net8.0 1825, zero failures.
Seven new tests, each red against the raw TrackEvent it guards:
FactoryTimeoutTests — RunAsync still throws TimeoutException when the sink refuses, and a sink that accepts still records the event, so the first cannot pass by the record going missing.
RedisDistributedLockTests — the releaser still reaches the caller and still releases; the timeout path still returns the no-op lease.
RehydrationCoordinatorTests — the rehydrate still runs, and the locks stay held for the cooldown.
RedisStreamSubjectWriterTests — an invalid event and a same-source event are both still acknowledged.
The cooldown test synchronises on the outer catch rather than on the lock handle: ReleaseInFlight runs at the top of the finally, before the await that releases the locks, so a retrigger succeeding says nothing about the handle. A refusal that escapes is logged in the outer catch, which happens before the finally runs at all.
Contributor declaration
I signed off my commits per the DCO (git commit -s).
I am contributing on behalf of my employer, or in the course of employment / using employer resources.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟢 Approval recommended
The changes are localized, consistent with existing TelemetrySafeguards patterns, and remove a verified failure mode without altering the primary control flow beyond making telemetry best-effort where required.
Review effort: Lite Findings: None
What changed in this PR
This PR hardens cache/Redis components against telemetry sinks that throw, ensuring telemetry reporting cannot defeat the recovery paths (catch/finally/best-effort sections) that are intended to keep core operations running.
Changes:
Extend TelemetrySafeguards.TryTrackException to accept optional telemetry properties and route through TrackException safely.
Replace direct TrackException/TrackEvent calls in recovery-sensitive paths (lock acquire/release, factory timeout, warm-up, finally blocks) with TryTrackException / TryTrackEvent.
Add clarifying comments at the call sites where a throwing telemetry sink previously could have changed observable behavior.
File
Description
src/UiPath.Caching/TelemetrySafeguards.cs
Adds optional properties to TryTrackException and forwards them safely to TrackException.
…at made it
Every catch here exists so the surrounding work carries on, and each
reported through the sink directly, so a sink that threw took the
recovery with it.
- RedisDistributedLock: the acquire catches degrade to an unavailable
lease; a refused report handed the caller an exception instead. The
release catch escaped the release.
- FactoryTimeout: a refused record replaced the TimeoutException the
caller is documented to receive.
- MemoryCacheSetter: the record sits in a finally, where a throw
replaces whatever exception is already on its way out.
- RedisConnectionWarmup: the warm-up is best effort, and this was the
one thing that could make it not be.
TryTrackException takes properties now, since these sites carry them.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
BuildAcquiredLease reported the acquisition after Redis had granted the lock
but before the releaser reached the caller, so a refusing sink threw past it
and left the lock held until expiry. TrackTimeoutNoOp could likewise throw in
place of the no-op lease. Both now go through TryTrackEvent, so every
telemetry call in the class is guarded.
Adds call-site tests rather than only helper tests: RedisDistributedLock keeps
returning the releaser and the no-op lease, and FactoryTimeout.RunAsync still
surfaces TimeoutException, each with a sink that refuses the event in question.
RefusingTelemetryProvider is the shared double.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
…nd the stream writer
Each of these reports before the statement that does the work, so a refusing
sink changed behaviour rather than losing a record:
- RehydrationCoordinator: a throw from the triggered event skipped the
rehydrate entirely; from the timed-out and failed events it skipped
`handles = null`, so the finally released the per-key locks instead of
holding them for the cooldown. Deduped and succeeded are guarded too, so no
path in the method depends on the sink.
- RedisStreamSubjectWriter: HandleInvalidEvent reports before adding the id to
the acknowledgement list, leaving the poison entry pending forever.
TraceReceipt has the same shape -- `ids.Add` follows it on the valid path.
Two regression tests, both red without the guards: the rehydrate still runs,
and the locks stay held for the cooldown.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
The spawn is fire-and-forget and the refusal is recorded inside the catch,
before the code that follows it runs, so asserting the lease was not disposed
straight after the refusal could win the race against a regression that lets
the finally release it.
The key stays reserved until the finally releases it, so a second trigger
succeeding is the signal that the first spawn is fully done. Two lock handles
keep that second run from muddying the assertion about the first.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
ReleaseInFlight runs at the top of the finally, before the await that releases
the locks, so a retrigger succeeding said nothing about the handle. The outer
catch is the deterministic signal instead: a refusal that escapes is logged
there, and that happens before the finally runs at all, so waiting for the
reservation to clear proves the log would already be written. The probe that
drains it takes its own lock handle and its own key.
Adds the two stream-writer regression tests the guards were missing: an
invalid event and a same-source event are both still acknowledged when the
sink refuses their record. Both are red against the raw TrackEvent.
Trims the comments and summaries this branch added.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
_inFlight is keyed by cache-key name, so the probe on a different key was
reserved independently and succeeded straight away, proving nothing about the
spawn under test. The second lock handle is what keeps the retry from
disposing the handle being asserted on, so the key can stay the same.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
Guarding the event call left TrackTopicReadMetric on the same-source path
still able to strand the entry: it reports before the id joins the
acknowledgement list, so a sink that refuses metrics reaches the outer handler
and the entry stays pending. Guarding call by call only holds until the next
one is added.
Both paths now add the id first. The entry is handled either way, so nothing
reported afterwards can leave it pending, and the ordering makes the defect
structurally impossible rather than guarded.
The same-source test refuses metrics as well as the event, and is red without
the reorder even with the event guarded. TrackTopicReadMetric in ChangeToken
is the last statement of its branch, so a refusal there costs only the record.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
This regression comment still describes the old ordering, but the implementation now adds the ID before calling TraceReceipt. Rephrase it as historical context so the test does not contradict the code it covers.
Lock acquire failure on both paths, lock release, the memory-cache
refresh callback, the stream receipt on the dispatch path, and the
rehydrate failed outcome, which strands the cooldown locks the same way
timed_out does. Each fails against the raw call it guards.
Signed-off-by: Cosmin Staicu <cosmin.staicu@uipath.com>
The existing StartAsync_TracksException_WhenConnectFails test uses CapturingTelemetry, whose TrackException succeeds, so it passes unchanged with the old raw call and does not exercise this new guard. Because WarmUpAsync is fire-and-forget, a refusing exception sink would fault that background task; add a regression test with a throwing TrackException provider and a completion signal that verifies warm-up remains best effort.
This branch has not been deployed
No deployments
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
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.
Why
Every
catchin these files exists so the surrounding work carries on, and each reported the caught failure through the telemetry sink directly. A sink that throws therefore takes the recovery with it — the exact failure mode #196 removed fromRedisConnectorandRedisPlannedMaintenance, still present in four other places.TelemetrySafeguardslanded with #196, so these can route through it now.RedisDistributedLockacquire (×2)TrackEvent(EventUnavailable)that follows has the same exposure.RedisDistributedLockreleaseFactoryTimeoutthrow new TimeoutException(...), so a refused one replaces the exception the caller is documented to receive.MemoryCacheSetterfinally, where a throw replaces whatever exception is already on its way out — the original cause is lost.RedisConnectionWarmupWhat
Each of those routes through
TryTrackException/TryTrackEvent.TryTrackExceptiongains an optional properties parameter, since the lock sites carry them.After this, no raw
TrackExceptionremains insrcoutsideTelemetrySafeguardsitself.Review then found the same defect in three more places, and enumerating the rest turned up a fourth:
RedisDistributedLockacquired / timeoutRehydrationCoordinatortriggeredrehydrateAsyncentirely — the whole point of the spawn.RehydrationCoordinatortimed out / failedhandles = null, so thefinallyreleased the per-key locks instead of holding them for the cooldown.dedupedandsucceededare guarded too, so no path in the method depends on the sink.RedisStreamSubjectWriter.HandleInvalidEventRedisStreamSubjectWriter.TraceReceiptids.Addfollows it on the valid path.Not included
Five raw
TrackEventcalls remain, inRedisConnectorandRedisPlannedMaintenance. Those files are rewritten by #197, so guarding them here would only conflict; they follow once it merges.Verification
Release build with
-warnaserrorclean. Full suite on both target frameworks: net10.0 1846, net8.0 1825, zero failures.Seven new tests, each red against the raw
TrackEventit guards:FactoryTimeoutTests—RunAsyncstill throwsTimeoutExceptionwhen the sink refuses, and a sink that accepts still records the event, so the first cannot pass by the record going missing.RedisDistributedLockTests— the releaser still reaches the caller and still releases; the timeout path still returns the no-op lease.RehydrationCoordinatorTests— the rehydrate still runs, and the locks stay held for the cooldown.RedisStreamSubjectWriterTests— an invalid event and a same-source event are both still acknowledged.The cooldown test synchronises on the outer catch rather than on the lock handle:
ReleaseInFlightruns at the top of thefinally, before the await that releases the locks, so a retrigger succeeding says nothing about the handle. A refusal that escapes is logged in the outer catch, which happens before thefinallyruns at all.Contributor declaration
git commit -s).