refactor(util): host one exponential-backoff retry

[aglinxinyuan]

What changes were proposed in this PR?

The same doubling-backoff loop was written three times, and no module could share the others' copy: amber's Utils sits above common/workflow-core and file-service in the module graph.

New dependency-free common/util module with RetryUtil.withBackoff, and both blocking copies now call it:

Site	Before	After
LakeFSStorageClient.healthCheck	private 27-line retryWithBackoff	RetryUtil.withBackoff("connect to lake fs server", 5, 200, ...)
FileService.awaitDependency	private 36-line loop	7-line delegation keeping its 6/200 defaults + logger
Utils.retry (amber, non-blocking)	unchanged	cross-references the blocking sibling

RetryUtil.withBackoff(description, maxAttempts, initialDelayMillis, onRetry, sleep)(operation)

The util owns everything the copies duplicated: attempt counting, the doubling, what counts as transient (NonFatal), interrupt fail-fast with the interrupt status restored, and the message wording. description is a verb phrase ("connect to lake fs server"), interpolated into all three messages, so LakeFS's give-up text is byte-identical to before. Retries are reported through an onRetry hook carrying a RetryAttempt (attempt, budget, delay, cause, and the standard message), which callers log with their own logger — so a LakeFS retry still logs under LakeFS, not under the util.

Bug fixed on the way in. LakeFS's sleep call sat inside the catch block, so an interrupt raised while waiting escaped raw with the interrupt flag left cleared — a catch cannot catch what its own body throws:

Before (LakeFS):  op fails -> catch -> sleep interrupted -> InterruptedException escapes, flag cleared
After  (shared):  op fails -> sleep interrupted -> interrupt restored + wrapped, fails fast

FileService's copy already plugged that hole with an inner try; the shared util keeps the better behavior for both.

Why a new module rather than common/workflow-core, which was proposal 1 in #7095: workflow-core is not reachable from Auth, ConfigService, or AccessControlService, so hosting it there would have left future callers in those modules stuck writing their own loop — the exact thing this PR is meant to stop. common/config is reachable from everything and already carries org.apache.texera.amber.util.ConfigParserUtil, but a retry helper isn't configuration. A dependency-free module any other module may depend on avoids both problems, at the cost of one build.sbt entry.

Why the non-blocking variant stays in amber. Utils.retry needs com.twitter:util-core, declared only in amber/build.sbt. Moving it down would put util-core on the classpath of every service that depends on the new module and force LICENSE-binary resyncs, so the pair is: blocking in common/util, non-blocking in amber, each documented in terms of the other. The new module is dependency-free for the same reason.

One behavior change, deliberate. Both replaced loops caught case e: Exception; the util uses NonFatal, for parity with Utils.retry. NonFatal is wider: non-fatal Errors — AssertionError, java.io.IOError, ServiceConfigurationError — are now retried and wrapped rather than propagating immediately. On the S3/LakeFS startup paths that means such a failure costs the full backoff before surfacing. RetryUtilSpec pins it in both directions (a non-fatal Error is retried; a fatal throwable is not).

Two retry sites deliberately left alone, because converting them changes behavior rather than just structure — tracked with the full reasoning in #7124:

Site	Why not now
URLFetchUtil.getInputStreamFromURL	Retries 5x with no delay and returns Option. Adopting backoff adds up to ~3 s before a dead URL gives up — arguably a fix (it is an unthrottled retry storm today), but a behavior change in an operator path.
PythonProxyClient Flight connect loop	Constant delay, closes the Flight client between attempts, and throws WorkflowRuntimeException on give-up. Needs a delay-multiplier knob plus a give-up type decision.

Any related issues, documentation, discussions?

Closes #7095

Follows #7088, which introduced the non-blocking variant; the consolidation was split out of it to keep that fix narrow. Context: the review discussion on #7103 (the LakeFS health-check backport) asked for one util rather than a fourth copy.

How was this PR tested?

RetryUtilSpec (new, in common/util, registered in build.yml's hand-maintained jacoco module list so it actually runs) is the union of what the two hand-rolled loops were tested for, plus what neither covered:

• returns the operation's value on first success without sleeping; retries until success with a doubling progression; honors a custom base delay; succeeds on the final permitted attempt (the attempt >= maxAttempts boundary); gives up naming the description and preserving the cause; maxAttempts = 1 means no retry and no sleep;
• the onRetry hook fires once per wait with 1-based attempt numbers and never after the last attempt, and its message wording is pinned;
• interrupt during the operation and interrupt during a backoff sleep both restore the interrupt status and fail fast (the second is the LakeFS hole above);
• a fatal throwable is not retried and passes through unwrapped, while a non-fatal Error is retried (the NonFatal-vs-Exception widening above).

At the call sites: FileServiceSpec keeps 8 tests that exercise the delegation, its own 6/200 defaults, and the description in its messages (the 4 that only re-tested util mechanics moved into RetryUtilSpec). LakeFSStorageClientSpec's 4 retry tests moved out with the loop; its remaining 5 pass unchanged. amber's UtilsSpec and RegionExecutionManagerSpec are untouched and still green.

sbt "Util/jacoco" "FileService/testOnly org.apache.texera.service.FileServiceSpec" "WorkflowCore/testOnly org.apache.texera.amber.core.storage.util.LakeFSStorageClientSpec"

[info] Tests: succeeded 11, failed 0, canceled 0, ignored 0, pending 0   (Util)
[info] Tests: succeeded 12, failed 0, canceled 0, ignored 0, pending 0   (FileService)
[info] Tests: succeeded 5,  failed 0, canceled 0, ignored 0, pending 0   (WorkflowCore / LakeFS)

Lint plus test-source compiles for every module the new dependency edge touches, and amber's two retry specs:

sbt "Util/scalafixAll --check" "Util/scalafmtCheckAll" "WorkflowCore/Test/compile" "WorkflowCore/scalafixAll --check" "FileService/Test/compile" "FileService/scalafixAll --check" "WorkflowExecutionService/Test/compile" "WorkflowExecutionService/scalafixAll --check" "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.common.UtilsSpec org.apache.texera.amber.engine.architecture.scheduling.RegionExecutionManagerSpec"

[success] all scalafix / scalafmt checks and Test/compile tasks
[info] Tests: succeeded 27, failed 0, canceled 0, ignored 0, pending 0

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 5)

[github-actions]

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

• Committers with relevant context: @parshimers
  You can request their reviews formally with /request-review @parshimers.

• Contributors with relevant context: @eugenegujing, @Yicong-Huang, @Ma77Ball
  You can notify them by mentioning @eugenegujing, @Yicong-Huang, @Ma77Ball in a comment.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 66.66667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.54%. Comparing base (8c255b8) to head (d4f6982).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...cala/org/apache/texera/common/util/RetryUtil.scala	77.77%	3 Missing and 3 partials ⚠️
.../amber/core/storage/util/LakeFSStorageClient.scala	0.00%	6 Missing ⚠️
.../scala/org/apache/texera/service/FileService.scala	77.77%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7119      +/-   ##
============================================
- Coverage     79.56%   79.54%   -0.03%     
+ Complexity     3834     3831       -3     
============================================
  Files          1159     1160       +1     
  Lines         46145    46147       +2     
  Branches       5127     5129       +2     
============================================
- Hits          36717    36709       -8     
- Misses         7798     7807       +9     
- Partials       1630     1631       +1     

Flag	Coverage Δ
access-control-service	70.00% <ø> (ø)
agent-service	77.42% <ø> (ø)
amber	73.01% <63.63%> (-0.05%)	⬇️
computing-unit-managing-service	20.49% <ø> (ø)
config-service	66.66% <ø> (ø)
file-service	66.80% <77.77%> (-0.41%)	⬇️
frontend	83.05% <ø> (ø)
notebook-migration-service	78.94% <ø> (ø)
pyamber	97.36% <ø> (ø)
workflow-compiling-service	26.31% <ø> (ø)

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 5 worse · ⚪ 8 noise (<±5%) · 0 without baseline

Compared against main 8c255b8 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

	config	throughput	MB/s	latency	max Δ latest / 7d
🔴	bs=10 sw=10 sl=64	657	0.401	15,025/20,030/20,030 us	🔴 +18.3% / 🔴 +27.0%
🔴	bs=100 sw=10 sl=64	1,421	0.867	68,447/98,321/98,321 us	🔴 +20.4% / 🟢 +42.1%
⚪	bs=1000 sw=10 sl=64	1,655	1.01	601,913/638,705/638,705 us	⚪ within ±5% / 🟢 +60.5%

Baseline details

Latest main 8c255b8 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	657 tuples/sec	748 tuples/sec	786.12 tuples/sec	-12.2%	-16.4%
bs=10 sw=10 sl=64	MB/s	0.401 MB/s	0.457 MB/s	0.48 MB/s	-12.3%	-16.4%
bs=10 sw=10 sl=64	p50	15,025 us	12,701 us	12,305 us	+18.3%	+22.1%
bs=10 sw=10 sl=64	p95	20,030 us	21,617 us	15,774 us	-7.3%	+27.0%
bs=10 sw=10 sl=64	p99	20,030 us	21,617 us	18,978 us	-7.3%	+5.5%
bs=100 sw=10 sl=64	throughput	1,421 tuples/sec	1,404 tuples/sec	999.71 tuples/sec	+1.2%	+42.1%
bs=100 sw=10 sl=64	MB/s	0.867 MB/s	0.857 MB/s	0.61 MB/s	+1.2%	+42.1%
bs=100 sw=10 sl=64	p50	68,447 us	71,956 us	100,616 us	-4.9%	-32.0%
bs=100 sw=10 sl=64	p95	98,321 us	81,643 us	107,356 us	+20.4%	-8.4%
bs=100 sw=10 sl=64	p99	98,321 us	81,643 us	113,255 us	+20.4%	-13.2%
bs=1000 sw=10 sl=64	throughput	1,655 tuples/sec	1,654 tuples/sec	1,031 tuples/sec	+0.1%	+60.5%
bs=1000 sw=10 sl=64	MB/s	1.01 MB/s	1.009 MB/s	0.63 MB/s	+0.1%	+60.4%
bs=1000 sw=10 sl=64	p50	601,913 us	602,426 us	980,328 us	-0.1%	-38.6%
bs=1000 sw=10 sl=64	p95	638,705 us	639,722 us	1,027,528 us	-0.2%	-37.8%
bs=1000 sw=10 sl=64	p99	638,705 us	639,722 us	1,054,298 us	-0.2%	-39.4%

Raw CSV

config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,304.26,200,128000,657,0.401,15024.92,20029.72,20029.72
1,100,10,64,20,1407.55,2000,1280000,1421,0.867,68446.79,98320.82,98320.82
2,1000,10,64,20,12085.80,20000,12800000,1655,1.010,601912.86,638705.07,638705.07

[Yicong-Huang]

🔴 1 must-fix · 6 advisory · 3 polish — the design is right; the new module just never runs its tests in CI.

Design & architecture (1)

• build.sbt:122 — Util is missing from build.yml's hand-maintained common-module jacoco list, so RetryUtilSpec never runs in CI while 8 tests that did run were deleted in the same PR (must-fix, see inline)

Correctness (3)

• Utils.scala:73 — the comment at :97 claiming the blocking loops catch Exception is made false by this PR (advisory, see inline)
• RetryUtil.scala:93 — Exception -> NonFatal widens retry to non-fatal Errors; unstated, and no test can observe it (advisory, see inline)
• common/util/build.sbt:51 — "every other module depends on it" is false for five modules and three services (advisory, see inline)

Conventions (3)

• Title: common is a directory, not a module — AGENTS.md says to use the module name, and no merged common/* title uses it; refactor(util): host one exponential-backoff retry is 49 chars vs. the current 67 against the ~60 cap (advisory)
• Description: say why the util landed in a new common/util module rather than common/workflow-core as #7095's proposal 1 specified (advisory)
• Description: file a follow-up issue for the two loops left alone (URLFetchUtil, PythonProxyClient) — that reasoning lives only in the description, which disappears on merge (advisory)

Polish: 3 quick touch-ups (see inline comments).

Verification trace

Traced every sbt invocation under .github/workflows/ to confirm Util's Test config is never executed: build.yml:308-316 enumerates the common modules for jacoco and omits Util; the comment at :283-285 states sbt's test does not transit dependsOn, so WorkflowCore/jacoco cannot reach it; :648 is WorkflowExecutionService/test, :660 is dist-only, :796/:932 are the six-service platform matrix, and no root sbt test exists. grep -rn "Util" .github/workflows/ returns only two unrelated Utils.amberHomePath comments.

Separately traced the transient-set change: NonFatal admits java.io.IOError, ServiceConfigurationError and AssertionError, which case e: Exception rejected, and neither remaining fatal-throwable fixture (ControlThrowable in RetryUtilSpec:177, StackOverflowError in FileServiceSpec:125) can observe the difference.

[aglinxinyuan]

All ten addressed — the must-fix in 93fb507, plus the three description-level notes:

• Title → refactor(util): host one exponential-backoff retry (49 chars). You're right that common is a directory, not a module.
• Why a new module rather than common/workflow-core (proposal 1 in Exponential-backoff retry is hand-rolled in three modules; host one util they can all reach #7095) is now a paragraph in the description: workflow-core isn't reachable from Auth, ConfigService or AccessControlService, so hosting it there leaves future callers in those modules writing their own loop — the thing this PR exists to stop. common/config is reachable from everything and already carries ConfigParserUtil, but a retry helper isn't configuration.
• The two loops left alone are now Two retry loops still hand-rolled: URLFetchUtil retries without delay, PythonProxyClient waits a constant delay #7124, with the specific blocker for each recorded there rather than only in this description: URLFetchUtil retries with no delay at all (backoff adds ~3 s before a dead URL fails), and PythonProxyClient needs a delay-multiplier knob plus a decision about its WorkflowRuntimeException give-up type.

Re-verified after the changes: Util/jacoco 11 passed (up from 10 with the new non-fatal-Error case), FileServiceSpec 12, LakeFSStorageClientSpec 5, amber's two retry specs 27, and scalafix/scalafmt clean on all four modules.

[Yicong-Huang]

🟢 7 resolved · 0 open · 0 new — every prior finding addressed correctly; nothing new across design, correctness, or conventions.

Verification trace

Confirmed RetryUtil.withBackoff is behavior-equivalent to the two blocking loops it replaces and to its non-blocking sibling Utils.retry: same NonFatal predicate, same doubling progression, same 1-based indexing, and a give-up wrapping ("Failed to $description after N attempts") that reproduces both replaced loops — the LakeFS give-up text is byte-identical. The one deliberate delta — NonFatal widening Exception, so non-fatal Errors (AssertionError, java.io.IOError, ServiceConfigurationError) are now retried on the S3/LakeFS startup paths — is pinned in both directions by RetryUtilSpec (a non-fatal Error is retried and wrapped; a ControlThrowable is not), and "Util/jacoco" (build.yml:312) makes that spec run in CI.