fix(orchestrator): optionally retry retriable container-refresh failures by morgan-wowk · Pull Request #312 · TangleML/tangle

morgan-wowk · 2026-08-06T17:43:59Z

What

When refreshing a running container's state fails, the orchestrator used to immediately mark it SYSTEM_ERROR and skip its whole downstream DAG — even when the failure was just the backing platform being briefly unavailable (e.g. a 500 … RST_STREAM ENHANCE_YOUR_CALM from the Kubernetes API server). We learned nothing about the container, so throwing away the task and its DAG is wrong.

This PR lets the orchestrator tolerate a few consecutive retriable refresh failures before terminalizing.

Whether a failure is retriable is the launcher's decision. LauncherError gains an is_retriable flag; the Kubernetes launcher sets it for 5xx while refreshing a pod/job. The orchestrator acts on the flag and no longer inspects HTTP status codes.
Each running execution gets a budget of consecutive retriable failures (default 3). A tolerated failure is naturally sent to the back of the queue; any successful refresh resets the budget. Non-retriable errors terminalize immediately, as before.

Behind a feature flag (off by default)

The behaviour change is gated on the TANGLE_RETRY_CONTAINER_REFRESH_FAILURES env var. Disabled by default → identical to today (any refresh failure terminalizes immediately). Enabling it turns on the retry budget.

Scope

Only the refresh is retried. The container is never relaunched, so non-idempotent programs are never re-run by this path.

morgan-wowk · 2026-08-06T17:44:12Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Ark-kun · 2026-08-06T21:18:19Z

                        )
-                        record_system_error_exception(
-                            execution=execution_node, exception=ex
+                        running_container_execution.last_processed_at = retry_at


Not sure this is needed. Processed executions are already sent to the back of the queue (First few lines of the internal_process_one_running_execution code).

Agreed — there's no manual last_processed_at reordering anymore. The retry path just leaves the execution in place and returns; the existing ordering by last_processed_at already sends a just-processed execution to the back, so nothing else is delayed (covered by test_failing_execution_does_not_hold_up_the_queue).

Ark-kun · 2026-08-06T21:34:16Z

+    return None
+
+
+def _is_transient_observation_failure(exception: BaseException) -> bool:


I think these checks are too Kubernetes-specific and belong to the Kubernetes launcher. We can add is_retriable to LauncherError and then detect that in the Orchestrator.

Done. LauncherError now carries is_retriable; the Kubernetes launcher sets it for 5xx in _launcher_error_from_api_exception, and the orchestrator only checks ex.is_retriable — no status codes in the orchestrator.

Ark-kun · 2026-08-06T21:35:32Z

        sleep_seconds_between_queue_sweeps: float = 1.0,
        output_data_purge_duration: datetime.timedelta = None,
+        max_running_execution_poll_failures: int = 3,
+        running_execution_poll_retry_delay: datetime.timedelta = datetime.timedelta(


I'm not sure we really need this (right now). We can just put the execution to the end of the queue (it's already done automatically).

Removed the retry-delay parameter — relying on the automatic back-of-queue ordering as you suggested.

Ark-kun · 2026-08-06T21:37:08Z

+
+
+def _is_transient_observation_failure(exception: BaseException) -> bool:
+    """Whether failing to observe a container is worth re-observing later.


Nit: The "observe" terminology sounds weird here. It's not used anywhere in orchestrator and is clashing with Observability.

We "handle" queue items.

We "refresh" the launched container state.

Renamed to "refresh" throughout (the warning log, the counters, and the tests).

Ark-kun · 2026-08-06T21:39:30Z

+        # Number of consecutive failures observed while *polling* the status of a
+        # running container execution, keyed by `ContainerExecution.id`. Entries
+        # are dropped as soon as a poll succeeds or the execution is terminalized.
+        self._running_execution_poll_failures: dict[Any, int] = {}


We could store this information in the extra_data of the container execution.
A clever way to do that is to store the failure history: a list of timestamps and maybe the associated errors - good for analysis. The number of failures is then the size of history.

Pros:

Does not disappear after backend restart

We do not keep the whole table in memory (although the size is really small)

Can be analyzed by querying the DB.

Cons:

Requires a DB commit to persist.

OK to do in different PR/later.

Agreed — leaving the DB/extra_data failure-history for a follow-up. The in-memory counter is dropped on success/terminalization, and I removed the unbounded-growth safety valve here per the other comment.

Ark-kun · 2026-08-06T21:41:18Z

-                    for execution_node in execution_nodes:
-                        _mark_all_downstream_executions_as_skipped(
-                            session=session, execution=execution_node
+                    else:


Can we avoid extra nesting by putting return True here?

Done — early return True on the retry path, no extra nesting.

Ark-kun · 2026-08-06T21:43:12Z

        default_task_annotations: dict[str, Any] | None = None,
        sleep_seconds_between_queue_sweeps: float = 1.0,
        output_data_purge_duration: datetime.timedelta = None,
+        max_running_execution_poll_failures: int = 3,


_max_container_execution_refresh_error_retries

Let's make it experimental for now. And let's move it to the keyword-only section (after *).

Done — renamed to _max_container_execution_refresh_error_retries and moved into the keyword-only Internal/experimental block.

Ark-kun · 2026-08-06T21:47:31Z

+# Safety valve for `OrchestratorService_Sql._running_execution_poll_failures`.
+# Entries are normally removed as soon as a poll succeeds or the execution is
+# terminalized, but an execution row that disappears while PENDING/RUNNING would
+# otherwise leak one entry in a long-lived process.
+_MAX_TRACKED_POLL_FAILURES = 10_000


I don't think we need this limit for now.

Later we can upgrade to storing the retry info in the DB (via extra_data).

Removed _MAX_TRACKED_POLL_FAILURES.

Ark-kun · 2026-08-06T21:49:45Z


+# Safety valve for `OrchestratorService_Sql._running_execution_poll_failures`.
+# Entries are normally removed as soon as a poll succeeds or the execution is
+# terminalized, but an execution row that disappears while PENDING/RUNNING would


...terminalized...?

That constant and its comment are gone now (safety valve removed). "Terminalize" only remains in test names, meaning "move to a terminal status" — happy to rename if you'd prefer.

Ark-kun · 2026-08-07T01:00:16Z

 _MAIN_CONTAINER_NAME = "main"


+def _launcher_error_from_api_exception(


Nit: Let's move this utility function to the end of the file.

Done — moved _launcher_error_from_api_exception to the bottom of the file.

Ark-kun · 2026-08-07T01:07:04Z

+_RETRY_CONTAINER_REFRESH_FAILURES = os.environ.get(
+    "TANGLE_RETRY_CONTAINER_REFRESH_FAILURES", ""
+).lower() in ("1", "true", "yes")
+


It's OK not to gate. The feature is strtaightforward enough and easy to disable.

Removed the TANGLE_RETRY_CONTAINER_REFRESH_FAILURES gate — the retry is on by default now, and as you say it's easy to disable by reverting. Dropped the disabled-flag test with it.

Refreshing a running container's state is a read-only operation. Until now, any exception raised while refreshing one immediately marked the ContainerExecution and its ExecutionNode SYSTEM_ERROR and skipped every downstream execution. When the launcher's backing platform sheds load or breaks, that verdict is wrong: we learned nothing about the container, so destroying the task and its downstream DAG throws away work for no reason. In tangle-orchestrator production, `500 ... ResourceExhausted ... RST_STREAM ENHANCE_YOUR_CALM` from the Kubernetes API server is a recurring source of exactly this. Whether a failure is worth retrying is the launcher's decision, not the orchestrator's: only the launcher understands its platform's errors. So `LauncherError` grows an `is_retriable` flag, the Kubernetes launcher sets it for 5xx responses while refreshing a pod or job, and the orchestrator simply acts on the flag -- it no longer inspects HTTP status codes and stays free of platform specifics. The new behaviour is off by default and gated behind a feature flag, `TANGLE_RETRY_CONTAINER_REFRESH_FAILURES` (env var). With the flag disabled -- the default -- any refresh failure terminalizes immediately, exactly as before. With it enabled, each running execution gets a budget of consecutive retriable failures (default 3) before it terminalizes; a tolerated failure keeps the execution PENDING/RUNNING, and because `internal_process_one_running_execution` already bumps `last_processed_at`, the execution is naturally sent to the back of the queue. Any successful refresh clears the counter, so the budget applies per incident, not per lifetime. This retries the refresh only. The container is never relaunched and the user's program is never re-run, so non-idempotent tasks cannot be re-executed by this path. Co-authored-by: Morgan Wowk <morgan.wowk@shopify.com>

 import json
 import datetime
 import logging
+import os


morgan-wowk · 2026-08-11T23:45:44Z

Merge activity

Aug 11, 11:45 PM UTC: A user started a stack merge that includes this pull request via Graphite.
Aug 11, 11:45 PM UTC: @morgan-wowk merged this pull request with Graphite.

morgan-wowk marked this pull request as ready for review August 6, 2026 17:59

morgan-wowk requested a review from a team August 6, 2026 17:59

morgan-wowk requested a review from Ark-kun as a code owner August 6, 2026 17:59

morgan-wowk mentioned this pull request Aug 6, 2026

feat(orchestrator): count vanished workloads #313

Merged

Ark-kun reviewed Aug 6, 2026

View reviewed changes

morgan-wowk force-pushed the river-orchestrator-poll-failure-budget branch from c3a74c9 to 6d972ae Compare August 6, 2026 22:49

morgan-wowk mentioned this pull request Aug 6, 2026

docs: keep platform specifics out of the generic core #314

Open

morgan-wowk force-pushed the river-orchestrator-poll-failure-budget branch from 6d972ae to 8e67db9 Compare August 6, 2026 23:21

morgan-wowk changed the title ~~fix(orchestrator): re-observe 5xx-failed executions instead of failing them~~ fix(orchestrator): optionally retry retriable container-refresh failures Aug 6, 2026

Ark-kun approved these changes Aug 7, 2026

View reviewed changes

morgan-wowk force-pushed the river-orchestrator-poll-failure-budget branch from 8e67db9 to 1c961ba Compare August 11, 2026 23:03

morgan-wowk force-pushed the river-orchestrator-poll-failure-budget branch from 1c961ba to da47a47 Compare August 11, 2026 23:24

github-code-quality Bot found potential problems Aug 11, 2026

View reviewed changes

Comment thread cloud_pipelines_backend/orchestrator_sql.py

import json

import datetime

import logging

import os

morgan-wowk merged commit e389b61 into master Aug 11, 2026
7 checks passed

		return None


		def _is_transient_observation_failure(exception: BaseException) -> bool:



		def _is_transient_observation_failure(exception: BaseException) -> bool:
		"""Whether failing to observe a container is worth re-observing later.

		_MAIN_CONTAINER_NAME = "main"


		def _launcher_error_from_api_exception(

Uh oh!

Conversation

morgan-wowk commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Behind a feature flag (off by default)

Scope

Uh oh!

morgan-wowk commented Aug 6, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Ark-kun Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morgan-wowk commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

morgan-wowk commented Aug 6, 2026 •

edited

Loading

morgan-wowk commented Aug 6, 2026 •

edited

Loading

Ark-kun Aug 6, 2026 •

edited

Loading

morgan-wowk commented Aug 11, 2026 •

edited

Loading