Skip to content

[uss_qualifier/astm] Make cleanup phase in down_uss use estimated extents as well#1405

Open
the-glu wants to merge 1 commit intointeruss:mainfrom
Orbitalize:fix_1400
Open

[uss_qualifier/astm] Make cleanup phase in down_uss use estimated extents as well#1405
the-glu wants to merge 1 commit intointeruss:mainfrom
Orbitalize:fix_1400

Conversation

@the-glu
Copy link
Contributor

@the-glu the-glu commented Mar 26, 2026

This PR should fix #1400 by also using estimated extends during cleanup phase.

Initial cleanup was not doing anything during initial phase since it was using scenario_execution_max_extents only, where as check was done on estimated_max_extents.

I didn't verify exact scenario in original issue, but looking at report, there is now a query to perform clean up:

image

(before, no query was done:

image

so potentially existing OP intents would not being deleted

)

And having a previous test leaving an OPI cleanup it as expected:

image

The other usage of estimate_scenario_execution_max_extents don't perform any cleanup (but does perform an 'area clear' check), so I didn't change anything there.

details=f"DSS responded code {del_query.status_code}; {e}",
query_timestamps=[del_query.request.timestamp],
)
for area in [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By always potentially clearing according to both areas, we're probably being a little less clear (scenario_execution_max_extents will never be relevant when calling this function from the setup step) and a little more broad than necessary (we don't need to clear estimated_max_extents in the end-of-scenario cleanup if nothing actually happened during the scenario). If we were still going to do this, I think we should still make just one query as queries are far, far more expensive than a tiny bit of local math:

            areas = Volume4DCollection([])
            if self.scenario_execution_max_extents:
                areas.append(self.scenario_execution_max_extents)
            if self.estimated_max_extents:
                areas.append(self.estimated_max_extents)
            if not areas:
                return
            area = areas.bounding_volume

But, it seems like we could instead simply have _clear_op_intents accept a Volume4D and call it with estimated_max_extents in setup and scenario_execution_max_extents in end-of-scenario cleanup.

self.scenario_execution_max_extents,
]:
if area is None:
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pre-existing problem, but we're initiating the check too early. Ideally, we should complete the action to collect data before we initiate the check to evaluate that data. But, it would be inconvenient to need to repeat the with initiation statement in two different places, so I think practically it's ok to initiate the check before performing the action (find_op_intent) since starting to perform the action necessarily means we're going to perform the check one way or another. But, we should not initiate the check if we're not actually going to perform the action (and therefore definitely not perform the check).

If area is None, currently the check will indicate as passing, but that is incorrect as we haven't checked anything. Instead, we should make sure we're actually going to check something before initiating the check since initiating a check and not recording a failure is defined as meaning that the check has positively passed.

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.

Some test scenarios do not clean up reliably

2 participants