FIX: re-raise what concurrency test workers throw - #786
Conversation
run_in_threads only let a worker's exception print to stderr: join()
still succeeded and the worker's slot stayed None, so a test could carry
on with None results. Some assertions catch that by accident (len({id(x)
for x in results}) == 1 is satisfied by four Nones), and the ones that do
fail report an AttributeError on None rather than the real error.
Workers now record what they raise and the helper re-raises it after
joining, grouping them when more than one fails. Adds tests for the
helper itself, since a fault in it weakens every concurrency test
silently.
Reported by CodeRabbit on #779 after that PR merged.
📝 WalkthroughWalkthroughChangesThreaded test helper
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b45d94770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(errors) == 1: | ||
| raise errors[0] | ||
| if errors: | ||
| raise ExceptionGroup("workers raised", errors) |
There was a problem hiding this comment.
When two or more workers fail under Python 3.10, this branch raises NameError because the built-in ExceptionGroup was introduced in Python 3.11, while pyproject.toml still declares support for Python 3.10. The newly added multiple-exception test likewise references the unavailable name, so the test suite cannot validate this helper on a supported Python version; use a compatible aggregation mechanism or explicitly raise the minimum supported version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in 875f15d. pyproject.toml declares requires-python = ">=3.10" (line 30) while ExceptionGroup is 3.11+, and the CI matrix only runs 3.11-3.14, so nothing here would have caught it.
Rather than add a compatibility shim I dropped the aggregation: the helper now re-raises the first worker failure. That is 3.10-safe, simpler, and later failures being dropped is the normal contract for a test helper (you fix the first and re-run). See the sibling thread — the BaseException problem pushed the same way.
| if len(errors) == 1: | ||
| raise errors[0] | ||
| if errors: | ||
| raise ExceptionGroup("workers raised", errors) |
There was a problem hiding this comment.
When multiple workers raise values derived directly from BaseException—for example, two workers calling pytest.skip() or pytest.fail()—ExceptionGroup rejects them with TypeError: Cannot nest BaseExceptions in an ExceptionGroup. Since the worker deliberately catches BaseException, this replaces the actual worker outcomes instead of propagating them as promised; the aggregation must support the full type being caught.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and the sharper of the two. Fixed in 875f15d.
The worker catches BaseException deliberately, so two workers calling pytest.skip or pytest.fail would have hit TypeError: Cannot nest BaseExceptions in an ExceptionGroup — replacing the outcomes the helper exists to propagate, which is worse than the swallowing this PR set out to fix.
BaseExceptionGroup would have satisfied this one, but not the 3.10 floor in the sibling thread, so the helper now just re-raises the first failure: no nesting rules, no version floor. Added test_base_exception_from_worker_propagates to pin it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #786 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 164 164
Lines 17589 17707 +118
===========================================
+ Hits 17589 17705 +116
- Misses 0 2 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Two problems with aggregating, both raised by codex: ExceptionGroup is 3.11+, and pyproject declares requires-python >=3.10, so the aggregating branch would have been a NameError on the oldest supported interpreter (CI only runs 3.11+, so nothing would have caught it). It also rejects BaseException members. The worker deliberately catches BaseException, so two workers using pytest.skip or pytest.fail would have produced "TypeError: Cannot nest BaseExceptions in an ExceptionGroup" in place of the outcomes it promised to propagate. Re-raising the first failure is 3.10-safe, handles BaseException, and is simpler. Later failures are dropped, which is the normal contract for a test helper. Adds a test for the BaseException case.
Testing the test helper is more machinery than the helper warrants; the concurrency tests that use it already exercise it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/conftest.py`:
- Line 141: Replace the assert-based completion check in the thread cleanup
logic with an explicit is_alive() conditional that raises AssertionError("thread
never finished; possible deadlock") when the worker remains alive, ensuring the
check remains active under Python optimization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2084263b-b614-4665-b783-9c4f17100939
📒 Files selected for processing (1)
tests/conftest.py
| thread.start() | ||
| for thread in threads: | ||
| thread.join(timeout) | ||
| assert not thread.is_alive(), "thread never finished; possible deadlock" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching conftest.py =="
fd -a 'conftest\.py$' . | sed 's#^\./##' || true
echo "== tests/conftest.py around reported line =="
if [ -f tests/conftest.py ]; then
wc -l tests/conftest.py
sed -n '110,170p' tests/conftest.py | nl -ba -v110
else
echo "tests/conftest.py not found"
fi
echo "== assert usage in tests/conftest.py =="
rg -n '\bassert |\bassertion|is_alive|threading|Thread|daemon' tests/conftest.py || true
echo "== Python assert optimization behavior probe =="
python3 - <<'PY'
import tempfile, os, subprocess
for opt in ("", "-O"):
ok = False
for stmt in ("if thread.is_alive(): raise AssertionError('alive')", "assert not thread.is_alive(), 'alive'"):
with tempfile.NamedTemporaryFile("w", suffix=".py", delete=False) as f:
f.write(stmt + "\n")
f.flush()
p = subprocess.run([opt, "python3", f.name], capture_output=True, text=True)
print({
"opt_flag": opt,
"stmt": stmt,
"rc": p.returncode,
"stdout": p.stdout.strip(),
"stderr": p.stderr.strip(),
})
os.unlink(f.name)
PYRepository: DASDAE/dascore
Length of output: 426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests/conftest.py around reported line =="
if [ -f tests/conftest.py ]; then
awk 'NR>=110 && NR<=170 { printf "%7d\t%s\n", NR, $0 }' tests/conftest.py
else
echo "tests/conftest.py not found"
fi
echo "== assert usage in tests/conftest.py =="
rg -n '\bassert |\bassertion|is_alive|threading|Thread|daemon' tests/conftest.py || true
echo "== Python assert optimization behavior probe =="
python3 - <<'PY'
import tempfile, os, subprocess
for opt in ("", "-O"):
for stmt in ("if thread.is_alive(): raise AssertionError('alive')", "assert not thread.is_alive(), 'alive'"):
with tempfile.NamedTemporaryFile("w", suffix=".py", delete=False) as f:
f.write(stmt + "\n")
f.flush()
p = subprocess.run([opt, "python3", f.name], capture_output=True, text=True)
print({
"opt_flag": opt,
"stmt": stmt,
"rc": p.returncode,
"stdout": p.stdout.strip(),
"stderr": p.stderr.strip(),
})
os.unlink(f.name)
PYRepository: DASDAE/dascore
Length of output: 4055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import tempfile, os, subprocess, threading, time
def test_assert_case(name, stmt):
path = tempfile.mktemp()
code = f"thread = threading.Thread(target=time.sleep, args=(0.01,))\nthread.start()\nthread.join(0)\n{stmt}\nprint('ran')\n"
with open(path, "w") as f:
f.write(code)
try:
for opt in ("", "-O"):
cmd = [opt, "python3", path]
p = subprocess.run(cmd, capture_output=True, timeout=5)
print(f"{name}: opt={op r or '<empty>'} rc={p.returncode} stdout={p.stdout!r} stderr={p.stderr!r}")
print(f"{name}: assertion removed with -O = 'ran' not found")
finally:
os.unlink(path)
test_assert_case("alive-check", "if thread.is_alive(): raise AssertionError('alive')")
test_assert_case("assert-check", "assert not thread.is_alive(), 'alive'")
PYRepository: DASDAE/dascore
Length of output: 292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import tempfile, os, subprocess, threading, time
def run_case(name, stmt):
path = tempfile.mktemp()
code = f"thread = threading.Thread(target=time.sleep, args=(0.01,))\nthread.start()\nthread.join(0)\n{stmt}\nprint('ran')\n"
with open(path, "w") as f:
f.write(code)
try:
for opt in ("", "-O"):
cmd = [opt + "python3", path] if opt else ["python3", path]
p = subprocess.run(cmd, capture_output=True, timeout=5)
opt_repr = "<empty>" if not opt else opt
print(f"{name}: opt={opt_repr} rc={p.returncode} stdout={p.stdout!r} stderr={p.stderr!r}")
finally:
os.unlink(path)
run_case("alive-check", "if thread.is_alive(): raise AssertionError('alive')")
run_case("assert-check", "assert not thread.is_alive(), 'alive'")
PYRepository: DASDAE/dascore
Length of output: 1020
🌐 Web query:
Python optimize -O removes assert statements documentation
💡 Result:
In Python, assert statements are designed to be used for debugging and testing purposes rather than for mandatory runtime logic [1][2]. When Python is run in optimized mode, the interpreter effectively removes these statements [1]. How to remove assert statements: You can disable assert statements by running the Python interpreter in optimized mode using one of the following methods [1]: 1. Command-line options: - Use the -O flag: This sets the built-in debug constant to False and removes assert statements from the compiled bytecode [1]. - Use the -OO flag: This performs the same optimization as -O, but also discards docstrings from the compiled bytecode [1]. 2. Environment variable: - Set the PYTHONOPTIMIZE environment variable to a non-empty string (e.g., 1 or 2) to achieve the same effect as the -O or -OO flags, respectively [1]. Important Considerations: - Assertions are not safe for production runtime checks because they are silently skipped when optimized mode is enabled [2]. Code required for program logic or security (such as input validation or permission checks) should never be placed within an assert statement [2]. - If you need to ensure code always runs, regardless of optimization settings, use standard conditional if statements instead [2]. - The behavior is linked to the debug constant; when this is False, any code explicitly written within an if debug: block will also be skipped, in addition to assert statements [1].
Citations:
- 1: https://realpython.com/python-assert-statement/
- 2: https://snyk.io/blog/the-dangers-of-assert-in-python/
Do not rely on assert for deadlock detection.
With Python optimization turned on, line 141 is removed, so live workers can pass this check and return partial results or keep non-daemon threads alive. Use if thread.is_alive(): raise AssertionError("thread never finished; possible deadlock") instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` at line 141, Replace the assert-based completion check in
the thread cleanup logic with an explicit is_alive() conditional that raises
AssertionError("thread never finished; possible deadlock") when the worker
remains alive, ensuring the check remains active under Python optimization.
1 participant
Description
Fixes a fault in the
run_in_threadstest helper added by #779, reported by CodeRabbit after that PR merged.The helper only let a worker's exception print to stderr.
join()still succeeded and the worker's slot stayedNone, so a test could carry on withNoneresults:assert len({id(x) for x in results}) == 1passes just as happily when all four workers raised and every result isNone.AttributeErroronNone— instead of the error the worker actually hit.Workers now record what they raise, and the helper re-raises the first failure after joining.
It deliberately does not aggregate into an
ExceptionGroup, which codex caught on the first revision: that type is 3.11+ whilepyproject.tomldeclaresrequires-python = ">=3.10", and it rejectsBaseExceptionmembers — so two workers callingpytest.skip/pytest.failwould have produced aTypeErrorin place of the outcomes the helper exists to propagate. Later failures are dropped, which is the normal contract for a test helper.The helper itself is not directly tested — that is more machinery than it warrants. The concurrency tests across io core, namespaces, units, the catalog and the remote cache exercise it, and they are what the fix protects.
Validation
pre-commit run --all: passed.Changelog
none
Checklist
I have (if applicable):