Skip to content

Retry target setup reads without creating another target #3767

Open

Description

@adamtheturtle

When a read fails after target creation succeeds, the target_id fixture retries the whole create-and-wait operation. The retry should retain the target ID and retry the failed read instead.

Observed in this CI job for PR #3766:

  • Fixture setup reached attempt 10 with 90 seconds of retry sleep before raising TooManyRequestsError.
  • The final failure was HTTP 429 on GET /summary/64200cadb64e44c6b42ea38020804170, after a target ID had already been obtained. The response included x-envoy-ratelimited: true.
  • The traceback does not show the outcomes of all preceding attempts; it does establish that the outer create-and-wait helper exhausted its retry budget.

In api_contract.py at the tested commit, RETRY_ON_TRANSIENT_VWS_FAILURE wraps _add_target_which_processed_successfully, which calls _add_target, waits for processing, and reads the target record. _wait_for_target_processed locally retries only UnknownTargetError. A 429 therefore escapes to the outer retry, whose next invocation starts by creating another target with a new name. The same restart can follow a transient failure of get_target_record.

This adds unnecessary writes while the service is throttling. Targets created before a read failure also bypass the helper's explicit failed-processing cleanup; the log does not establish how many extra targets remained.

Acceptance criteria:

  • Once creation returns a target ID, transient failures while polling or fetching its record retry against that same ID within a bounded budget, without calling add_target again.
  • Preserve the narrow eventual-visibility retry for a known, newly created target. Do not globally classify unexpected UnknownTargetError as transient.
  • Preserve the separate, bounded behavior from A target which finishes processing in a non-success state fails the test rather than being retried #3426 that deletes a target whose processing actually failed and tries a replacement. A transient read error must not be treated as failed processing.
  • Define cleanup ownership for targets created before setup ultimately fails, preserving the original failure if cleanup also fails.
  • Add deterministic regression coverage for a 429 during processing polling, a transient record-read failure, exhaustion, and genuine failed processing. Assert that transient read recovery creates only one target and reuses its ID; require no live credentials or real sleeps.

The affected code is also present on current main; it is outside the Model Target retry-policy files changed by #3766. Related: #3738 concerns ownership of real-service policy in the reusable suite. Rate-limit delay policy is tracked separately in #3768.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions