Repository navigation
The starvation stand-in was starving the SECURITY reads too, and hanging the create - #1427
Merged
Merged
Conversation
…ing the create Shard 1, 2026-08-13: SourceDiscoveryUnavailableTest failed at line 153 — the CreateNode of the healthy source node — with the starve exception and 'emitted nothing at all'. It was called a flake. It is not. The stand-in's own comment claims: 🚨 Matched on the namespace: FORM, not on the bare path, so only source DISCOVERY starves. Node creation and single-node reads under the same subtree use path:-shaped queries and must keep working That is false. The SECURITY layer reads its policy and grants with namespace:-shaped queries over the SAME subtree: namespace:…/StarvedSourceType/Source id:_Policy nodeType:PartitionAccessPolicy namespace:…/StarvedSourceType/Source/_Access nodeType:AccessAssignment so the marker starved those too. The CI log shows both being evicted as faulted synced queries, and the hub snapshot shows the consequence: Hub portal/nodeops-… Executing(CreateNodeRequest, 33014ms) The create sat for 33 s waiting on security reads that could never answer, and the 30 s assertion gave up first. It passed whenever those queries were already cached and failed when they were cold — which is exactly why it reproduced on a CI shard and never locally (10/10 green on main before this change). CodeQueryResolver ANDs nodeType:Code into every source query it emits, and the security queries carry their own node types, so that clause separates them exactly. The starve now requires it. Not a masking change — the starve still bites: - narrowed: the test passes and the starved type still reaches Unavailable; - starve disabled entirely: the test FAILS, because the type compiles Ok — so the source-discovery leg is provably still starved. It also stops the test paying for the hang it never meant to cause: 30.2 s (soft-deadline warning on CI, and ~30 s locally) → a stable 4 s over 5 runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR stabilizes SourceDiscoveryUnavailableTest by narrowing the starvation stand-in so it only faults source discovery queries (nodeType:Code) and no longer starves security policy/grant reads under the same namespace (which could cause CreateNode to hang until the assertion timeout).
Changes:
- Require
nodeType:Codein the query-matching predicate that triggers starvation inStarvedSourceQueryProvider. - Add a dedicated
SourceDiscoveryMarkerconstant and expand the inline documentation to explain why the discriminator is necessary (security reads also usenamespace:queries).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+278
to
+279
| /// <c>CodeQueryResolver</c> ANDs <c>nodeType:Code</c> into every source query it emits, and the | ||
| /// security queries carry their own node types — so that clause separates them exactly.</para> |
Contributor
Contributor
Contributor
Contributor
Contributor
Test Results (shard 2)2 454 tests - 122 2 453 ✅ - 122 6m 8s ⏱️ -36s Results for commit 53e8216. ± Comparison against base commit a83f5fd. This pull request removes 335 and adds 213 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 1)1 498 tests +123 1 498 ✅ +123 7m 30s ⏱️ +16s Results for commit 53e8216. ± Comparison against base commit a83f5fd. This pull request removes 213 and adds 336 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Copilot is right, and it caught a claim I had copied rather than checked: CodeQueryResolver appends nodeType:Code only when the query does not already name a node type (line 145) — so 'ANDs it into every source query it emits' is an overstatement. The starve marker is still correct HERE, and the test proves it rather than asserting it: the starved type declares no Sources, so its discovery runs the DEFAULT query and does carry the clause. The note now says exactly that, and names the case it would not cover — a declared source query that names some other node type. Also corrected CodeQueryResolver's own summary, which is where the overstated claim came from and where the next person would copy it from too. Comments only — SourceDiscoveryUnavailableTest still green in 4s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Right, and thank you — I had copied that claim rather than checked it. The marker is still correct here, and the test proves that rather than asserting it: the starved type declares no I also corrected Comments only; |
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
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.
Shard 1 of #1418 failed
SourceDiscoveryUnavailableTestand I re-ran it green, calling it a flake. Following up: it is not a flake, and the same root cause was silently costing this test ~26 seconds on every run.What actually happened
The failure was at line 153 — the
CreateNodeof the healthy source node — with the starve exception and "emitted nothing at all". The stand-in's own comment claims that cannot happen:That is false. The security layer reads its policy and grants with
namespace:-shaped queries over the same subtree:so the marker starved those too. The CI log shows both evicted as faulted synced queries, and the hub snapshot shows the consequence:
The create sat 33 seconds waiting on security reads that could never answer, and the 30 s assertion gave up first. It passed whenever those queries were already cached and failed when they were cold — which is exactly why it reproduced on a CI shard and never locally (10/10 green on main before this change).
The fix
CodeQueryResolverANDsnodeType:Codeinto every source query it emits, and the security queries carry their own node types — so that clause separates them exactly. The starve now requires it.Proof it does not MASK the thing under test
Unavailable.Ok— so the source-discovery leg is provably still starved.It also stops the test paying for a hang it never meant to cause: 30.2 s (the CI soft-deadline warning, and ~30 s locally) → a stable 4 s across 5 runs.
🚩 Worth a separate look
The product behaviour underneath is that a starved security-policy read leaves
CreateNodeRequestexecuting for 33 s rather than failing fast. In a real cross-silo starvation — the very scenario this suite exists for (#1218) — node creation into the affected namespace would hang the same way. The faulted-query eviction machinery already recovers the query; the in-flight caller still waits. That is a robustness gap of its own and I have not touched it here.🤖 Generated with Claude Code