DM-52067: Make Sasquatch dispatch robust to an unresponsive or broken proxy by mfisherlevine · Pull Request #492 · lsst/analysis_tools

mfisherlevine · 2026-09-15T09:34:43Z

Rapid analysis publishes metric bundles from its worker pods through the SasquatchDatastore, where a Sasquatch outage should not fail the pipeline task doing the put.

No HTTP request had a timeout, so a proxy that accepted connections but never answered blocked the put, and the task, indefinitely. Every request now has one (30 s by default), set by the datastore's "timeout" config entry.
The cluster id lookup parsed the JSON body before checking the status, so an error response's error document escaped as a bare KeyError rather than SasquatchDispatchFailure, and failed the put. It now checks the status first.

Also fixes SingleValueRepacker for cp_verify results tables. Their per-detector and per-exposure rows have no amplifier, and set() over the masked values raised "unhashable type: 'MaskedConstant'", so analyzeFlatDetCore never wrote its flatTestsByDate plot.

mfisherlevine · 2026-10-09T18:07:15Z

+            except Exception:
+                # Publishing is best-effort: a broken proxy, or a bug in
+                # preparing the records, must never fail the butler put and
+                # with it the pipeline task.
+                log.exception(
+                    "Unexpected error dispatching metric bundle %s to Sasquatch; it was not published.", ref
+                )


@czwa I'm not sure whether you think this is correct or not - I actually don't quite know what the "right" behaviour is here, but this is at least mostly in keeping with the behaviour right above with the warning.

This might be a question for slack and #dm-analysis-tools. This looks reasonable to me, and I agree with the comment that the butler put should always succeed (unless that's the problem).

This I am not sure about. Currently if there is some sort of unexpected failure, bps marks it as such and things like re-tries can happen, this will break that existing behavior. There were also some sublties of datastores I don't remember off the top of my head related to how to signal in a chained datastore for others to keep trying (though I think that might be the DatasetTypeNotSupportedError). I'm not quite sure what is best here. It may be the change in behavior is not important, but it also may cause issues where people don't find issues because things just disappear.

You could preserve existing behavior with a swallow_exception or some kind of field, that defaults to false, but you could set to true in your specific butler config file.

Good point, I've dropped it - I wasn't thinking about bps (I never use it). This is now handled in RA anyway: a new butler wrapper swallows any failed metric-bundle put so there's not actually any need to change the default for everyone else (this was written first and I forgot to remove it when I made the new butler-wrapper in RA). What's left just makes a hung or erroring proxy end up as the existing SasquatchDispatchFailure warning, instead of hanging or failing the task, which I think is good for everyone.

mfisherlevine · 2026-10-09T18:07:55Z

        self.assertIs(mock_method.call_args[0][0], bundle)

+    def testPutSurvivesUnexpectedDispatchError(self) -> None:
+        # Publishing to Sasquatch is best-effort: an unexpected error from


And likewise, this assertion here.

Same answer, I'm afraid.

czwa

I think a post on #dm-analysis-tools pointing interested people to this PR would be useful. I'm not an expert on requests, but those changes look reasonable to me (and with unit tests).

czwa · 2026-10-12T20:16:50Z

+            except Exception:
+                # Publishing is best-effort: a broken proxy, or a bug in
+                # preparing the records, must never fail the butler put and
+                # with it the pipeline task.
+                log.exception(
+                    "Unexpected error dispatching metric bundle %s to Sasquatch; it was not published.", ref
+                )


This might be a question for slack and #dm-analysis-tools. This looks reasonable to me, and I agree with the comment that the butler put should always succeed (unless that's the problem).

czwa · 2026-10-12T21:45:55Z

        self.assertIs(mock_method.call_args[0][0], bundle)

+    def testPutSurvivesUnexpectedDispatchError(self) -> None:
+        # Publishing to Sasquatch is best-effort: an unexpected error from


Same answer, I'm afraid.

czwa · 2026-10-12T21:50:04Z

-        for i in range(len(uniquePanelKeys)):
-            repackedData[f"{uniquePanelKeys[i]}_x"] = []
-            repackedData[f"{uniquePanelKeys[i]}"] = []
+        repackedData: dict[str, list] = {}


I'm hoping that a future ticket will make this obsolete, but I think this makes sense.

natelust · 2026-10-13T18:14:58Z

+            # LookupError and TypeError catch valid JSON of the wrong shape,
+            # ValueError a body that is not JSON at all.
            raise SasquatchDispatchFailure("Could not retrieve the cluster id for the specified url") from e
+        self._cluster_id = str(cluster_id)


This reads like a weird unrelated change that I don't quite understand. Is the idea that you want to move the cluster_id determination until after raise for status so it does not get set if there is an exception? Even if that is the case, why move the self._cluster_id stringification so far away, and outside of try? Is it trying to have the minimum amount of things inside the try block? I can kind of guess at things, but this feels a bit like just code churn, though low impact I admit

Yeah, fair. The only real bug is parsing the body before checking the status. When the proxy sends back a 4xx/5xx, the error JSON has no data key, so you get a bare KeyError that gets past the except RequestException and fails the put. I've cut it back: now, compared with main, it's just raise_for_status() moved above the parse, plus the timeout.

natelust · 2026-10-13T18:23:37Z

+            except Exception:
+                # Publishing is best-effort: a broken proxy, or a bug in
+                # preparing the records, must never fail the butler put and
+                # with it the pipeline task.
+                log.exception(
+                    "Unexpected error dispatching metric bundle %s to Sasquatch; it was not published.", ref
+                )


This I am not sure about. Currently if there is some sort of unexpected failure, bps marks it as such and things like re-tries can happen, this will break that existing behavior. There were also some sublties of datastores I don't remember off the top of my head related to how to signal in a chained datastore for others to keep trying (though I think that might be the DatasetTypeNotSupportedError). I'm not quite sure what is best here. It may be the change in behavior is not important, but it also may cause issues where people don't find issues because things just disappear.

You could preserve existing behavior with a swallow_exception or some kind of field, that defaults to false, but you could set to true in your specific butler config file.

Rapid analysis publishes metric bundles from its worker pods through the SasquatchDatastore, where a Sasquatch outage should not fail the pipeline task doing the put. - No HTTP request had a timeout, so a proxy that accepted connections but never answered blocked the put, and the task, indefinitely. Every request now has one, 30 s by default, set by the datastore's "timeout" config entry. - The cluster id lookup parsed the body before checking the status, so an error response's JSON error document raised a bare KeyError rather than SasquatchDispatchFailure, and failed the put. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

cp_verify's results tables stack per-detector and per-exposure rows, which have no amplifier, in with the per-amplifier rows. Those come back masked, and set() raised "TypeError: unhashable type: 'MaskedConstant'", so analyzeFlatDetCore never wrote its flatTestsByDate plot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

natelust

To be clear I only reviewed the dispatcher bit of this, but it looks fine to me with the changes. Those look good now. I'll leave the rest to Chris

czwa

I've re-read all the files that github told me I hadn't viewed, and I think it still makes sense.

mfisherlevine force-pushed the tickets/DM-52067 branch 4 times, most recently from f500d81 to 8d5f036 Compare October 9, 2026 18:03

mfisherlevine commented Oct 9, 2026

View reviewed changes

czwa approved these changes Oct 12, 2026

View reviewed changes

mfisherlevine force-pushed the tickets/DM-52067 branch from 8d5f036 to a946809 Compare October 13, 2026 13:09

natelust reviewed Oct 13, 2026

View reviewed changes

mfisherlevine and others added 2 commits October 14, 2026 16:08

mfisherlevine force-pushed the tickets/DM-52067 branch from aac4786 to aae6a67 Compare October 14, 2026 15:09

natelust approved these changes Oct 14, 2026

View reviewed changes

czwa approved these changes Oct 14, 2026

View reviewed changes

mfisherlevine merged commit a14a3ff into main Oct 15, 2026
14 checks passed

mfisherlevine deleted the tickets/DM-52067 branch October 15, 2026 12:30

Conversation

mfisherlevine commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natelust left a comment

Choose a reason for hiding this comment

Uh oh!

czwa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mfisherlevine commented Sep 15, 2026 •

edited

Loading