[ShawnGarello]

What changes were proposed in this PR?

FileScanSourceOpExecSpec's test "create LargeBinary when reading file with LARGE_BINARY type" streams a real object to S3 whenever an endpoint is reachable (s3://texera-large-binaries/objects/8888/<uuid>), and nothing deleted it. Since #7800 made the test actually reach S3, every local run with the dev stack up (bin/local-dev.sh up publishes RustFS on :9000 with the credentials storage.conf uses) has added one more object to the developer's persistent rustfs_data volume.

CI never noticed: the amber job has no S3, so the test cancels at its probe, and the amber-integration jobs start RustFS but only run IntegrationTest-tagged tests (AMBER_TEST_FILTER=integration-only), which this one isn't.

This change is test-only, apart from one doc comment:

1. Calls LargeBinaryManager.deleteByExecution(testExecutionId) in the test's existing outer finally, so the cleanup also runs when the scan throws. deleteByExecution logs failures instead of throwing, so it can't hide the test's own failure.
2. Asserts that the produced object's URI is under objects/<testExecutionId>/, and at the end of the test that nothing is left under that prefix (S3StorageClient.directoryExists). Without the cleanup, the test fails.
3. Changes testExecutionId from 8888L to -8670L. The new cleanup is a prefix delete against whatever S3 the config points at, which outside CI is a developer's own. Real execution ids are positive (workflow_executions.eid is SERIAL), so objects/-8670/ can never hold a real execution's binaries, while 8888 could be a real execution on a long-lived dev instance. directoryExists/deleteDirectory add the trailing /, so -8670 doesn't match -86700. I also considered a random negative id per run, which would also keep two checkouts that share one RustFS apart, but chose a fixed one so that any leftover is easy to find if the cleanup ever fails.
4. Updates baseUriForExecution's doc in LargeBinaryManager to say tests may use a negative id. Its rule (use a persisted EID) exists to avoid collisions with real executions, which a negative id can't have.

Not covered: objects that earlier runs already left under objects/8888/ are not removed by this change. They can be deleted by hand, e.g. aws s3 rm s3://texera-large-binaries/objects/8888/ --recursive --endpoint-url http://localhost:9000.

Possible follow-ups, not in this PR:

• Tag this test IntegrationTest, so the amber-integration job (which has RustFS) runs it.
• Give WorkflowOperator's test scope the S3StorageTestBase testcontainers RustFS, so the test is hermetic and runs in CI.

Any related issues, documentation, discussions?

Closes #8670. Follow-up to #7800.

Backport: not needed. On release/v1.2 and release/v1.3 this spec has no 8888 and no setCurrentBaseUri, so the leak isn't there. @mengw15 @xuang7, if backport labels get auto-applied, please drop them.

How was this PR tested?

Locally against Postgres and RustFS (rustfs/rustfs:1.0.0-rc.6 on localhost:9000, storage.conf defaults), with sbt "WorkflowOperator/testOnly *FileScanSourceOpExecSpec":

1. Red: with only the new assertion (id 8888, no cleanup), the test fails with directoryExists(...) was true objects/8888/, and one 26-byte object is left behind.
2. Green: with the full change, on a fresh RustFS, two runs both pass (7/7), and the bucket is empty after each.
3. With the cleanup line commented out, the test fails with ... was true objects/-8670/, and the object sits at objects/-8670/<uuid>. With the line restored, the next run passes and removes it. An object placed under objects/-86700/ survives.
4. Without S3: succeeded 6, failed 0, canceled 1 with the existing skip message, the same as the CI amber job.
5. sbt "WorkflowOperator/test" with S3 up: 2702 succeeded, 0 failed, 12 canceled, 2 pending.
6. sbt scalafmtCheckAll "scalafixAll --check": clean.
7. After review: with the scan seeded under a different execution id (1234), the new URI assertion fails with "s3://texera-large-binaries/objects/1234/<uuid>" did not contain "objects/-8670/". On the updated head, steps 2 and 6 and the cleanup-removed check from step 3 give the same results as above.

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

Generated-by: Claude Code (Opus 5.5)

[github-actions]

Automated Reviewer Suggestions

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

• Contributors with relevant context: @Ma77Ball, @aglinxinyuan
  You can notify them by mentioning @Ma77Ball, @aglinxinyuan in a comment.

[github-actions]

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. A release/* label nominates a backport target; the branch's release manager approving this PR is what sends the fix there. The required Backport Approvals check stays red until every label below is approved, so each manager either approves or removes their own label — which is why the labels left on a merged PR are exactly the branches it reached.

Release branch	Analysis
✅ release/v1.3	Already labeled — this fix is queued to backport here. @mengw15 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.
✅ release/v1.2	Already labeled — this fix is queued to backport here. @xuang7 decides: approving sends the fix here, removing this label declines it. The merge waits on one or the other.

Auto-label run.

[github-actions]

👋 Thanks for your first contribution to Texera, @ShawnGarello!

If you're looking for a good place to start, browse issues labeled starter-task; they're scoped to be approachable for newcomers.

You can drive common housekeeping yourself by commenting one of these commands on its own line:

• Issues. Comment /take to assign an open issue to yourself, or /untake to release it. You can find unclaimed work with the search filter is:issue is:open no:assignee.
• Sub-issues. To link issues into a parent/child hierarchy, comment /sub-issue #5166 #5222 on the parent to attach those children (or /unsub-issue #5166 #5222 to detach them). From a child issue, comment /parent-issue #5166 to set its parent, or /unparent-issue to clear it (the current parent is detected automatically). References may be written as #5166 or as a bare 5166; cross-repository references are not supported.
• Pull requests (author only). Comment /request-review @user to request a review from someone, or /unrequest-review @user to withdraw that request.

Each command must match exactly: /take this will not work, only /take does. For the full contribution flow, see CONTRIBUTING.md.

[ShawnGarello]

/request-review @aglinxinyuan @Yicong-Huang

[Ma77Ball]

Review summary

Test-only change that stops FileScanSourceOpExecSpec from leaking a real S3 object into the developer's rustfs_data volume on every local run. It deletes the uploaded object in the test's existing outer finally (via deleteByExecution, which logs rather than throws, so it can't displace the test's own failure), asserts the prefix is empty afterward, and moves the write prefix off 8888 to a negative id so the prefix-delete can never touch a real execution's binaries (real EIDs are positive SERIAL).

The change is sound. The write (baseUriForExecution), delete (executionPrefix), and assertion all resolve to the same objects/-8670/ prefix, and both S3StorageClient.directoryExists and deleteDirectory add the trailing / only when missing, so there is no double-slash and -8670/ cannot match -86700/ -- the boundary the description relies on holds. The fixed-id vs random-id tradeoff for two checkouts sharing one RustFS is already weighed in the PR description and is a reasonable call for debuggability; no objection. No blocking issues.

Action items: Nothing blocking; the single P3 below is optional polish.

---

Findings

1 total: 0 P0, 0 P1, 0 P2, 1 P3.

Nit (P3)

• Tie the produced pointer to the cleaned prefix - common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/scan/file/FileScanSourceOpExecSpec.scala:193 - see inline.

---

Verification

• No blocking findings to substantiate. Traced the three paths and confirmed they agree on objects/-8670/: write base URI baseUriForExecution(-8670) = s3://<bucket>/objects/-8670/ with create() appending a UUID; cleanup deleteByExecution(-8670) deletes executionPrefix(-8670) = objects/-8670 (normalized to objects/-8670/); the assertion checks directoryExists(DEFAULT_BUCKET, "objects/-8670/"). Both S3 helpers normalize the trailing slash, so no // and no prefix match against -86700/. Negative id is safe because workflow_executions.eid is positive SERIAL.
• Did not run the suite (no S3 in this environment). Relied on the author's documented red/green local runs for the empty-bucket result; statically verified the prefix arithmetic and the negative-id safety argument.

[ShawnGarello]

@aglinxinyuan @Yicong-Huang, could one of you take a look when you have a chance? @Ma77Ball approved it, and their comments are addressed in 47f5665.

[Yicong-Huang]

Review summary

Test-only change that stops FileScanSourceOpExecSpec from leaking a real S3 object into the developer's rustfs_data volume on every local run. It deletes the uploaded object in the test's existing outer finally (via deleteByExecution, which logs rather than throws, so it can't displace the test's own failure), asserts the prefix is empty afterward, and moves the write prefix off 8888 to a negative id so the prefix-delete can never touch a real execution's binaries (real EIDs are positive SERIAL).

The change is sound. The write (baseUriForExecution), delete (executionPrefix), and assertion all resolve to the same objects/-8670/ prefix, and both S3StorageClient.directoryExists and deleteDirectory add the trailing / only when missing, so there is no double-slash and -8670/ cannot match -86700/ -- the boundary the description relies on holds. The fixed-id vs random-id tradeoff for two checkouts sharing one RustFS is already weighed in the PR description and is a reasonable call for debuggability; no objection. No blocking issues.

Action items: Nothing blocking; the single P3 below is optional polish.

Findings

1 total: 0 P0, 0 P1, 0 P2, 1 P3.

Nit (P3)

• Tie the produced pointer to the cleaned prefix - common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/scan/file/FileScanSourceOpExecSpec.scala:193 - see inline.

Verification

• No blocking findings to substantiate. Traced the three paths and confirmed they agree on objects/-8670/: write base URI baseUriForExecution(-8670) = s3://<bucket>/objects/-8670/ with create() appending a UUID; cleanup deleteByExecution(-8670) deletes executionPrefix(-8670) = objects/-8670 (normalized to objects/-8670/); the assertion checks directoryExists(DEFAULT_BUCKET, "objects/-8670/"). Both S3 helpers normalize the trailing slash, so no // and no prefix match against -86700/. Negative id is safe because workflow_executions.eid is positive SERIAL.
• Did not run the suite (no S3 in this environment). Relied on the author's documented red/green local runs for the empty-bucket result; statically verified the prefix arithmetic and the negative-id safety argument.

That looks like the one we run on spark lol

[Yicong-Huang]

🟢 0 must-fix · 0 advisory · 0 polish: test-only fix that stops the LARGE_BINARY scan test from leaking S3 objects, with the cleanup pinned by an assertion that fails if it ever stops working.

Verification trace

Traced the write, delete, and assert paths end to end. baseUriForExecution(-8670) writes under s3:///objects/-8670/ with a UUID suffix, deleteByExecution deletes executionPrefix(-8670), and the test asserts directoryExists on objects/-8670/. directoryExists and deleteDirectory both normalize a trailing slash (S3StorageClient.scala:70, 114), so the prefix cannot match objects/-86700/ and there is no double slash. Real execution ids are positive because workflow_executions.eid is a SERIAL primary key (sql/texera_ddl.sql:317), so the negative test id cannot collide with a real execution. deleteByExecution catches Exception and logs at error (LargeBinaryManager.scala:97-108), so a cleanup failure cannot mask the test's own failure, and it sits in the outer finally, so it also runs when the scan throws.

[ShawnGarello]

Thanks @Yicong-Huang! @mengw15 @xuang7, could you decide on the release/v1.3 and release/v1.2 labels when you have a chance?

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.81%. Comparing base (1299992) to head (236c29b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8978   +/-   ##
=========================================
  Coverage     91.81%   91.81%           
- Complexity     5069     5070    +1     
=========================================
  Files          1271     1271           
  Lines         55208    55208           
  Branches       6987     6987           
=========================================
+ Hits          50688    50689    +1     
  Misses         2800     2800           
+ Partials       1720     1719    -1     

Flag	Coverage Δ		*Carryforward flag
access-control-service	77.44% <ø> (ø)
agent-service	99.16% <ø> (ø)		Carriedforward from 1299992
amber	85.97% <ø> (+<0.01%)	⬆️
computing-unit-managing-service	61.84% <ø> (ø)
config-service	87.50% <ø> (ø)
file-service	81.92% <ø> (ø)
frontend	96.50% <ø> (ø)		Carriedforward from 1299992
notebook-migration-service	83.77% <ø> (ø)
pyamber	98.54% <ø> (ø)		Carriedforward from 1299992
workflow-compiling-service	72.61% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ 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

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

Compared against main 1299992 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	718	0.439	13,470/18,517/18,517 us	🔴 +11.6% / 🔴 +51.5%
⚪	bs=100 sw=10 sl=64	1,716	1.047	54,909/85,483/85,483 us	⚪ within ±5% / 🟢 -31.3%
🔴	bs=1000 sw=10 sl=64	2,062	1.258	473,631/651,052/651,052 us	🔴 +24.3% / 🟢 +45.7%

Baseline details

Latest main 1299992 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	718 tuples/sec	786 tuples/sec	1,019 tuples/sec	-8.7%	-29.6%
bs=10 sw=10 sl=64	MB/s	0.439 MB/s	0.48 MB/s	0.622 MB/s	-8.5%	-29.4%
bs=10 sw=10 sl=64	p50	13,470 us	12,070 us	10,144 us	+11.6%	+32.8%
bs=10 sw=10 sl=64	p95	18,517 us	19,248 us	12,225 us	-3.8%	+51.5%
bs=10 sw=10 sl=64	p99	18,517 us	19,248 us	15,527 us	-3.8%	+19.3%
bs=100 sw=10 sl=64	throughput	1,716 tuples/sec	1,724 tuples/sec	1,371 tuples/sec	-0.5%	+25.1%
bs=100 sw=10 sl=64	MB/s	1.047 MB/s	1.052 MB/s	0.837 MB/s	-0.5%	+25.1%
bs=100 sw=10 sl=64	p50	54,909 us	55,721 us	79,910 us	-1.5%	-31.3%
bs=100 sw=10 sl=64	p95	85,483 us	87,380 us	87,155 us	-2.2%	-1.9%
bs=100 sw=10 sl=64	p99	85,483 us	87,380 us	106,643 us	-2.2%	-19.8%
bs=1000 sw=10 sl=64	throughput	2,062 tuples/sec	2,129 tuples/sec	1,415 tuples/sec	-3.1%	+45.7%
bs=1000 sw=10 sl=64	MB/s	1.258 MB/s	1.299 MB/s	0.864 MB/s	-3.2%	+45.6%
bs=1000 sw=10 sl=64	p50	473,631 us	466,104 us	780,407 us	+1.6%	-39.3%
bs=1000 sw=10 sl=64	p95	651,052 us	523,808 us	817,814 us	+24.3%	-20.4%
bs=1000 sw=10 sl=64	p99	651,052 us	523,808 us	848,683 us	+24.3%	-23.3%

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,278.37,200,128000,718,0.439,13470.45,18516.56,18516.56
1,100,10,64,20,1165.49,2000,1280000,1716,1.047,54909.34,85483.09,85483.09
2,1000,10,64,20,9701.16,20000,12800000,2062,1.258,473631.30,651051.93,651051.93