apache / apache/texera

File-handle leak aborts the operator suite, plus an unseeded spec and a vacuous assertion

Open
#7,799 1 comment 0 reactions 1 assignee Claimed by @aglinxinyuan View on GitHub
Dominant language
Scala
Stars
314
Forks
187
Avg merge
1d 21h
Merged PRs (30d)
214

Description

### Describe the bug

Three defects in test code. The first blocks coverage measurement outright on Windows.

**1. `FileScanSourceOpExecSpec` leaks a file handle, aborting the suite and killing the whole module's coverage run.**

In `"create LargeBinary when reading file with LARGE_BINARY type"`, `createTuplesFromFile` throws mid-`map` with `IllegalStateException: LargeBinaryManager.create() requires a base URI, but none was set on the current thread`. That abandons the `AutoClosingIterator` before exhaustion, so its close-on-exhaustion hook never fires and the handle stays open. `afterAll` then fails with `test_large_binary.txt: The process cannot access the file because it is being used by another process`, which **aborts the suite**.

The consequence is bigger than one suite: `WorkflowOperator/jacoco` exits 1 and **emits no report directory at all**, so the whole module cannot be measured. POSIX `unlink` hides this on Linux CI, which is why it has gone unnoticed.

(The `.zip` fixture is stranded too — it only *looks* fine because `.gitignore:10` is `*.zip`. `afterAll` never reaches it, because the `.txt` delete throws first.)

**2. `IntervalOpExecSpec` is unseeded-random.** It imports `scala.util.Random.{nextInt, nextLong}` and uses them at four sites — input ordering twice, a 1k-row dataset, and the interval constant. The coverage footprint of `IntervalJoinOpExec` therefore drifts between runs of identical source: CI has reported 2 missed + 14 partial where a local run gave 0 missed + 13 partial.

**3. `PveResourceSpec` has a vacuous path-traversal assertion.** `getPythonBin(testCuid, "..") shouldBe None` claims to pin the guard at `PveManager.scala:91` (`if (!resolved.startsWith(root)) return None`), but `".."` **matches** the name regex at line 88 (`^[A-Za-z0-9._-]+$` admits dots) and `//../pve` normalizes to `/pve` — still under root. The `None` actually comes from the `Files.exists` check on line 92.

Proof: deleting line 91 entirely leaves that test passing, with results identical to baseline. Line 91 is in fact **unreachable by construction** — the regex forbids `/`, and `cuid` is an `Int` — so it is defensive code, not untested code.

### How to reproduce?

Run `sbt "WorkflowOperator/jacoco"` on Windows for (1) — the suite aborts and no report is produced. For (2), run `IntervalOpExecSpec` twice and compare the per-line jacoco counters for `IntervalJoinOpExec`. For (3), delete `PveManager.scala:91` and observe that the traversal test still passes.

### Version/Branch

main

### Task Type

- [ ] Refactor / Cleanup
- [ ] DevOps / Deployment / CI
- [x] Testing / QA
- [ ] Documentation
- [ ] Performance
- [ ] Other

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.