fix(hstore): preserve ordered scans under the Gremlin sandbox - #3213
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3213 +/- ##
============================================
+ Coverage 37.90% 41.13% +3.23%
- Complexity 6600 7212 +612
============================================
Files 800 802 +2
Lines 69035 69393 +358
Branches 9186 9237 +51
============================================
+ Hits 26167 28546 +2379
+ Misses 39795 37585 -2210
- Partials 3073 3262 +189 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- propagate completed worker failures before inline scans - keep draining nonblocking while other workers are active - cover original failure preservation and source cleanup
There was a problem hiding this comment.
Review score: 9/10. Five independent review lanes found one bounded failure-ordering issue, fixed in cd5ccfc: after worker submission is denied, consume already-completed initializer results before starting the next inline source. This preserves the original worker failure instead of delaying or masking it with an inline scan failure; polling remains nonblocking. The final patch received a separate independent review with no actionable findings.
Validation on Java 11: the regression fails against the previous implementation and passes with the fix; all 21 iterator tests and 15 security tests pass. Formatting and all-module clean compilation also pass. The previous head had 24 successful checks; CI for this new head must finish before merging.
No remaining confirmed blocker in this PR. The full PD + three-Store HTTP paging matrix remains a post-integration check for #2994, as already documented. A broader question about promptly waking blocked scan receives when sandbox cancellation is denied predates this patch and needs separate runtime confirmation; I have left those close semantics unchanged.
CI update at cd5ccfcc: codecov/patch passes, but the current codecov/project report is 33.85% versus 37.90% on the base. Six checks are still running, so coverage aggregation may still change; this is not yet a green merge gate. No coverage threshold or unrelated code was changed. Please wait for the completed CI/coverage result before merging.
Purpose of the PR
Fixes #3214.
Fix the ordered-scan worker-creation failure reported in #2994, independently of its LABEL-query changes. This branch starts from master
1a15e762.HStore's
OrderedKvIteratorprimes multiple scan sources with a bounded executor. Its workers are created lazily and expire when idle. Creating a worker under Gremlin evaluation can raiseSecurityException: Not allowed to access thread group via Gremlin. Merely constructing or warming the pool at startup does not cover later worker expiry.Main Changes
SecurityException, initialize the remaining sources on the calling thread for that iterator. Stop retrying the denied executor, and drain any already-submitted tasks before completing initialization.HugeSecurityManager, add package allowlists, or grant scripts thread-creation privileges. The fallback may increase first-result latency when a sandbox denies worker creation; no performance improvement is claimed.The diagram shows the changed failure path; no new privileges are introduced:
flowchart TB G[Gremlin evaluation] --> O[OrderedKvIterator initialization] O --> S[Submit source initialization] S --> Q{Submission permitted?} Q -->|Yes| W[Bounded worker initialization] Q -->|SecurityException - before| E[Query fails] Q -->|SecurityException - after| C[Initialize remaining sources on caller] W --> D[Drain submitted tasks] C --> D D --> M[Sorted merge with existing limit and cursor] U[Script creates a thread directly] --> B[Still denied by sandbox]Verifying these changes
The failure was reproduced on #2994 after merging master (
bcb8c1f3), using the realGremlinGroovyScriptEngine.eval(),HugeSecurityManager, and productionOrderedKvIterator, with controlled scan-source iterators. The new security tests also cover a worker expiring before the next query and confirm direct thread creation, including through the store-client thread factory, remains denied. They are registered inUnitTestSuite.Iterator regressions cover denied submission before/after a prior submission, a still-running source, sorted results, source errors, source security exceptions, interruption, and closure even when cancellation is denied. Existing concurrency, saturation, limit, and cursor tests remain in the suite.
Passed on the SSH test host with Java 11: 15 security tests and 20 unique iterator tests, with no failures or skips. Formatting and the all-module clean compilation also passed. Commands (without style or coverage skips):
This is an isolated sandbox/iterator reproduction and regression suite, not a rerun of the reporter's complete PD + three-store HTTP paging matrix. That matrix should be repeated after this fix is integrated into #2994. CI results are not claimed here.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No NeedNo public API or configuration change. The internal fallback and its latency trade-off are described above and in code.
Visual summary