fix(sidecar): harden band-math eval, job caps, and result bounds - #1574
Merged
giswqs merged 8 commits intoAug 1, 2026
Conversation
Block ndarray attribute I/O in raster calculator expressions, refuse unbounded in-flight conversion/Whitebox jobs, cap Sedona result size, sanitize conversion errors, and enforce the shared feature limit on Whitebox layer inputs.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConversion and Whitebox jobs now enforce in-flight limits, embedded inputs are size-bounded, raster expressions undergo AST validation, SQL results are capped, and conversion errors are sanitized in persisted job state. ChangesExecution and input safeguards
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Ayush7614
force-pushed
the
harden/sidecar-eval-and-job-limits
branch
from
July 30, 2026 16:29
8e9faad to
01b0b97
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@backend/geolibre_server/geolibre_server/app/conversion.py`:
- Around line 1134-1142: The conversion failure path in _job_update must remove
or redact captured subprocess messages before marking the job failed, while
retaining raw details only in the server-side logger.warning call. Update
backend/geolibre_server/geolibre_server/app/conversion.py:1134-1142 accordingly,
and extend backend/geolibre_server/tests/test_conversion.py:479-509 with a
subprocess/stub that emits a known stderr secret, asserting the secret appears
in neither the job error nor messages.
- Around line 1168-1172: Roll back the newly registered pending job when worker
thread startup raises RuntimeError: in
backend/geolibre_server/geolibre_server/app/conversion.py lines 1168-1172,
update the thread-start path around the conversion job registration to remove
that job under _JOBS_LOCK before propagating the failure; apply the same cleanup
in backend/geolibre_server/geolibre_server/app/whitebox.py lines 1085-1090
around its worker thread startup. Ensure both paths remove only the newly
registered job and preserve normal successful startup behavior.
In `@backend/geolibre_server/geolibre_server/sedona_ops.py`:
- Around line 154-158: Update the query-result handling around
result.to_pandas() to apply LIMIT MAX_FEATURES + 1 before materializing the
result, then use the presence of the extra row to raise SqlInputTooLarge.
Preserve returning at most MAX_FEATURES rows for valid results and avoid
converting the unbounded query result to a full pandas frame.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 309a0fd8-5fbc-4424-a3ce-919f41dc5be4
📒 Files selected for processing (8)
backend/geolibre_server/geolibre_server/app/conversion.pybackend/geolibre_server/geolibre_server/app/raster.pybackend/geolibre_server/geolibre_server/app/whitebox.pybackend/geolibre_server/geolibre_server/sedona_ops.pybackend/geolibre_server/tests/test_conversion.pybackend/geolibre_server/tests/test_raster.pybackend/geolibre_server/tests/test_sql.pybackend/geolibre_server/tests/test_whitebox_endpoints.py
Member
|
/claude-review |
Contributor
🔍 Cloudflare PR preview
|
Contributor
🔍 GitHub Pages PR preview
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/geolibre_server/geolibre_server/sedona_ops.py (1)
150-158: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap the result before materializing it (still unresolved).
result.to_pandas()(Line 153) fully materializes the query result before theMAX_FEATUREScheck runs. The comment on Lines 155-157 explicitly calls out cross joins/generate_seriesas row-expanding vectors, yet the code still pulls the entire expanded result into memory before rejecting it — the OOM risk this check is meant to guard against isn't actually mitigated for large/malicious result sets.This is a previously raised, still-open concern.
🔧 Proposed fix
result = connection.sql(sql) + # Fetch one row past the cap so an oversized result is detected + # without materializing more than MAX_FEATURES + 1 rows. + result = result.limit(MAX_FEATURES + 1) # to_pandas() returns a GeoDataFrame when the result has a geometry # column, otherwise a plain DataFrame. frame = result.to_pandas() if len(frame) > MAX_FEATURES: # Input registration already caps each layer, but a query can still # expand rows (cross joins, generate_series, etc.). Bound the # response the same way vector/PostGIS paths bound payloads. raise SqlInputTooLarge(f"Query result exceeds the {MAX_FEATURES}-feature limit")Please confirm the SedonaDB
sql()result object supports.limit()for the pinnedapache-sedona==1.7.0version before applying.🤖 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 `@backend/geolibre_server/geolibre_server/sedona_ops.py` around lines 150 - 158, Update the Sedona query handling around `connection.sql(sql)` to cap rows before materialization, using the pinned SedonaDB 1.7.0 result API only after confirming it supports `limit()`. Apply the limit while preserving the existing `MAX_FEATURES` overflow check and `SqlInputTooLarge` behavior, then call `to_pandas()` only on the bounded result.backend/geolibre_server/geolibre_server/app/conversion.py (1)
1158-1190: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoll back the reserved job slot when worker thread startup fails. Both
_start_job(conversion) andwhitebox_runregister a "pending"JobStateunder_JOBS_LOCKand then callthread.start()outside the lock with no rollback; a thread-creation failure leaves a permanently stuck slot that counts againstMAX_IN_FLIGHT_JOBSforever.
backend/geolibre_server/geolibre_server/app/conversion.py#L1158-L1190: wrapthread.start()in try/except and remove the job from_JOBSunder_JOBS_LOCKbefore re-raising on failure.backend/geolibre_server/geolibre_server/app/whitebox.py#L1065-L1102: apply the identical try/except-and-remove cleanup around itsthread.start()call.🤖 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 `@backend/geolibre_server/geolibre_server/app/conversion.py` around lines 1158 - 1190, Thread startup can fail after _start_job and whitebox_run reserve pending jobs, leaving permanent in-flight slots. In backend/geolibre_server/geolibre_server/app/conversion.py lines 1158-1190, wrap the _start_job thread.start() call in try/except, remove its job_id from _JOBS under _JOBS_LOCK, then re-raise; apply the identical cleanup in backend/geolibre_server/geolibre_server/app/whitebox.py lines 1065-1102 around whitebox_run’s thread.start().
🤖 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 `@backend/geolibre_server/geolibre_server/app/raster.py`:
- Around line 1028-1054: Extend the AST validation around the parsed tree in the
raster expression evaluator to whitelist only the documented scalar/array
arithmetic node types, operators, and permitted names before calling eval.
Reject list/set/dict constructions, exponentiation, unsupported literals, and
any other nodes outside that grammar so expressions such as [0] * 10**9 cannot
allocate resources; preserve the existing curated safe_funcs call validation.
---
Duplicate comments:
In `@backend/geolibre_server/geolibre_server/app/conversion.py`:
- Around line 1158-1190: Thread startup can fail after _start_job and
whitebox_run reserve pending jobs, leaving permanent in-flight slots. In
backend/geolibre_server/geolibre_server/app/conversion.py lines 1158-1190, wrap
the _start_job thread.start() call in try/except, remove its job_id from _JOBS
under _JOBS_LOCK, then re-raise; apply the identical cleanup in
backend/geolibre_server/geolibre_server/app/whitebox.py lines 1065-1102 around
whitebox_run’s thread.start().
In `@backend/geolibre_server/geolibre_server/sedona_ops.py`:
- Around line 150-158: Update the Sedona query handling around
`connection.sql(sql)` to cap rows before materialization, using the pinned
SedonaDB 1.7.0 result API only after confirming it supports `limit()`. Apply the
limit while preserving the existing `MAX_FEATURES` overflow check and
`SqlInputTooLarge` behavior, then call `to_pandas()` only on the bounded result.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 4d91408c-7cd0-4360-81c9-a1a47cae1a68
📒 Files selected for processing (8)
backend/geolibre_server/geolibre_server/app/conversion.pybackend/geolibre_server/geolibre_server/app/raster.pybackend/geolibre_server/geolibre_server/app/whitebox.pybackend/geolibre_server/geolibre_server/sedona_ops.pybackend/geolibre_server/tests/test_conversion.pybackend/geolibre_server/tests/test_raster.pybackend/geolibre_server/tests/test_sql.pybackend/geolibre_server/tests/test_whitebox_endpoints.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Clear conversion job messages on failure, roll back pending slots when Thread.start fails, cap Sedona results before to_pandas, and reject list/dict AST nodes in band math.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/geolibre_server/geolibre_server/app/raster.py (1)
1043-1060: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winComplete the AST allowlist.
The denylist still permits
b"x" * 1_000_000_000and(0,) * 1_000_000_000; both can allocate unbounded memory before the shape check. Rejectast.Tupleand non-numeric constants, and whitelist permitted node/operator types rather than only blocking selected containers.🤖 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 `@backend/geolibre_server/geolibre_server/app/raster.py` around lines 1043 - 1060, Update the AST validation around the existing container/comprehension rejection to use an explicit allowlist of permitted expression and operator node types for curated band math. Reject ast.Tuple and any non-numeric ast.Constant, while preserving only the numeric constants, names, calls, and operators required by the supported expressions so large bytes or tuple multiplication cannot allocate memory before shape validation.
🤖 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 `@backend/geolibre_server/tests/test_sql.py`:
- Around line 143-144: Update the test stub’s limit method to record the
requested n value, then assert after run_sql() that it equals MAX_FEATURES + 1.
Ensure the test fails when run_sql() omits the limit or supplies an incorrect
bound.
---
Duplicate comments:
In `@backend/geolibre_server/geolibre_server/app/raster.py`:
- Around line 1043-1060: Update the AST validation around the existing
container/comprehension rejection to use an explicit allowlist of permitted
expression and operator node types for curated band math. Reject ast.Tuple and
any non-numeric ast.Constant, while preserving only the numeric constants,
names, calls, and operators required by the supported expressions so large bytes
or tuple multiplication cannot allocate memory before shape validation.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d1352abb-a6ef-4662-af6e-b29f2c0687a3
📒 Files selected for processing (6)
backend/geolibre_server/geolibre_server/app/conversion.pybackend/geolibre_server/geolibre_server/app/raster.pybackend/geolibre_server/geolibre_server/app/whitebox.pybackend/geolibre_server/geolibre_server/sedona_ops.pybackend/geolibre_server/tests/test_conversion.pybackend/geolibre_server/tests/test_sql.py
Reject bytes/tuple allocations before eval, and lock the SQL overflow test to MAX_FEATURES + 1 so the bound cannot drift silently.
Ayush7614
force-pushed
the
harden/sidecar-eval-and-job-limits
branch
from
July 30, 2026 20:41
58b0afa to
4e39697
Compare
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 `@backend/geolibre_server/geolibre_server/app/raster.py`:
- Around line 1037-1068: Update raster expression validation around
_allowed_nodes and the AST-checking logic to statically inspect ast.BinOp nodes
using ast.Pow. Reject nested exponentiation chains and reject literal exponents
whose magnitude exceeds the permitted limit before native eval() runs, while
preserving existing validation for other binary operations.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9446ae8b-991d-4afb-88bf-e7bb5dc27e56
📒 Files selected for processing (7)
backend/geolibre_server/geolibre_server/app/conversion.pybackend/geolibre_server/geolibre_server/app/raster.pybackend/geolibre_server/geolibre_server/app/whitebox.pybackend/geolibre_server/geolibre_server/sedona_ops.pybackend/geolibre_server/tests/test_conversion.pybackend/geolibre_server/tests/test_raster.pybackend/geolibre_server/tests/test_sql.py
Resolve the test_whitebox_endpoints.py conflict by keeping both sides: main's COG-conversion tests for _ensure_raster_outputs_are_cogs and this branch's in-flight job cap / oversized layer input tests.
- raster.py: cap `**` in band math. The AST allowlist permitted ast.Pow with no bound, so `9**9**6` or `9**999999` built a multi-megabyte exact integer and pinned a core for tens of seconds before the post-eval shape check could reject it. An exponent must now be a numeric literal within MAX_POW_EXPONENT (64, far past squares/gamma/roots) unless it derives from a band or curated call, where NumPy's element-wise float power is bounded by the raster shape. - tests/test_raster.py: cover chained (`9**9**6`), oversized (`9**999999`), and computed (`9 ** (500 * 500)`) exponents, plus a positive case asserting ordinary powers including a negative literal exponent still evaluate.
Member
|
/claude-review |
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- conversion.py: run each conversion/raster script through a small driver
that re-emits a `SystemExit` message on a marker line. The failure handler
previously scrubbed `error` *and* `messages` for every failure, which also
swallowed this PR's own curated validation text ("Expression may not use
Attribute ...", "Exponent may not exceed 64 ...", dimension/CRS checks).
Both RasterToolsDialog and ConversionDialog render `job.error` as the only
failure feedback, so a user who mistyped a band-math expression saw just
"See the sidecar logs for details" with no way to read those logs. Curated
messages now surface verbatim; tracebacks and streamed subprocess output
stay generic, so the absolute-path leak remains closed.
- test_raster.py: drop the "may not access attributes" assertion the
implementation never emits and assert the exact messages instead —
"Call to 'Attribute' is not allowed in band math" for `np.where(...)`, and
"Expression may not use Subscript" for the tuple-wrapped `A.tofile(...)`.
Add a bare `A.tofile(...)` case so the test actually covers attribute
access rather than passing incidentally on the subscript wrapper.
- test_conversion.py: cover the new split — a curated SystemExit reaches
the client, an unexpected exception stays generic, and scripts still read
their params from `sys.argv[1]` through the driver.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A.tofile,np.*, etc.) in the raster calculator by AST-whitelisting band-math expressions beforeeval.MAX_IN_FLIGHT_JOBS) with HTTP 429, and enforce the shared feature limit on Whiteboxlayer_inputs.MAX_FEATURES, and stop leaking raw conversion/raster job errors to browser-proxied clients.Test plan
uv run --project backend/geolibre_server --extra test pytestfor the new conversion/whitebox/raster/sql casestest_raster.py -k raster_calculator,test_conversion.py,test_whitebox_endpoints.py,test_sql.pygreen(A.tofile('/tmp/x'), A)[1]fails and writes nothing/runcalls past the in-flight cap returns 429Summary by CodeRabbit
raster-calc/spectral-indexwith expression validation.