Repository navigation
MCP long-poll: an agent waits for an investigation request to open for its routes (#5822) - #5856
Conversation
…r its routes (#5822) wait_for_investigation_requests waits up to 25 seconds for a request to open on the named agent routes and answers the request id, route, server, family and open time with a cursor. The dispatcher fires an in-process signal on Opened only; the woken call reads the store past its cursor, so nothing is queued. The claim stays pull. The MCP transport stays stateless.
# Conflicts: # Darling/Darling.Tests/InvestigationToolsLiveTests.cs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| internal static bool TryParse(string text, out WaitCursor? cursor) | ||
| { | ||
| cursor = null; | ||
| var parts = text.Split('.'); | ||
| if (parts.Length != 3 || parts[0] != Version | ||
| || !long.TryParse(parts[1], NumberStyles.None, CultureInfo.InvariantCulture, out var ticks) | ||
| || ticks > DateTime.MaxValue.Ticks) |
There was a problem hiding this comment.
A cursor this parser ACCEPTS can make the read throw, so after lands on the error envelope instead of the refusal.
TryParse bounds the ticks only from above (ticks > DateTime.MaxValue.Ticks). long.TryParse with NumberStyles.None already rejects a sign, so the accepted range is [0, DateTime.MaxValue.Ticks] — which includes every watermark inside the first five minutes of year 1. The first thing WaitAsync does with the parsed cursor is subtract the overlap:
Darling/.../DarlingMcpInvestigationTools.cs:233
var rows = await read(cursor.Watermark - CursorOverlap, routes, WaitReadLimit, cancellationToken);
DateTime - TimeSpan throws ArgumentOutOfRangeException when the result falls below DateTime.MinValue, so after = "v1.0." (parses: ["v1","0",""], ticks 0, empty seen set) raises inside the try and comes back through McpHelpers.FormatError(WaitToolName, ex) as status = "error", "Error during wait_for_investigation_requests: …".
That is the wrong one of the four outcomes McpHelpers documents (McpHelpers.cs:816-824): "a parameter the tool cannot honor" is a refusal — status = invalid with hints.parameter, which is exactly what the sentence three lines above ("Invalid after value. Pass the cursor a previous … call answered") promises. Advance has the same subtraction at line 318, so a cursor that parses but cannot be reasoned about reaches two sites.
The range check belongs where the other cursor validation is:
| internal static bool TryParse(string text, out WaitCursor? cursor) | |
| { | |
| cursor = null; | |
| var parts = text.Split('.'); | |
| if (parts.Length != 3 || parts[0] != Version | |
| || !long.TryParse(parts[1], NumberStyles.None, CultureInfo.InvariantCulture, out var ticks) | |
| || ticks > DateTime.MaxValue.Ticks) | |
| internal static bool TryParse(string text, out WaitCursor? cursor) | |
| { | |
| cursor = null; | |
| var parts = text.Split('.'); | |
| if (parts.Length != 3 || parts[0] != Version | |
| || !long.TryParse(parts[1], NumberStyles.None, CultureInfo.InvariantCulture, out var ticks) | |
| || ticks < CursorOverlap.Ticks || ticks > DateTime.MaxValue.Ticks) |
InvestigationLongPollTests.ABadTimeoutOrCursor_IsRefused_WithoutWaiting covers the two shapes that fail the grammar ("not-a-cursor", "v1.123.nothex"); a row for "v1.0." would fail on the defect and pass on the fix.
| /* #5822: the investigation long poll is not a read to time or admit. It waits up to 25 seconds by design, so | ||
| timing it would fill the read-latency percentiles and the slow-read record with its waits, and admitting it | ||
| would hold a read slot for the whole wait. Its catch-up reads are small and keyed on opened_at. */ | ||
| if (string.Equals(toolName, DarlingMcpInvestigationTools.WaitToolName, StringComparison.Ordinal)) | ||
| { | ||
| return await next(request, cancellationToken); | ||
| } |
There was a problem hiding this comment.
The second bypass is on no roster, and the census that exists to notice one stays green by spelling.
I agree with the decision — a 25-second wait has no business in the read-latency percentiles or holding an admission slot, and ReadAdmission.SkippedRoutes could not take it (ReadAdmissionCoverageCensusTests.TheSkipRoster_IsTheHealthReads_EachWithAReason_AndEachAToolThatExists requires every entry's reason to start with "health read: "). What is missing is the record.
ReadAdmissionCoverageCensusTests exists for exactly this file (its summary: "every read that opens a ReadScope goes through read admission, or is a health read on the skip roster with its reason … and so does a change to the MCP filter order or the filter's one skipped tool"). After this hunk its two relevant assertions both pass while no longer describing the code:
TheFilterSkipsOnlyTheComposedPanelTool_AndTheSharedRunnerAdmitsIt(line 135) matchesAssert.Single(Regex.Matches(filter, @"string\.Equals\(toolName, SkippedToolName"))— this hunk spells the toolDarlingMcpInvestigationTools.WaitToolName, so the regex cannot see it and the test's own name is now false.EveryReadScopeSite_IsFollowedByAnAdmissionCallcountsReadScope.Open(sites, which is still 1 here — the new bypass returns before the scope, so it is invisible to that count too.
This is the #3699 roster shape in the traps file ("A roster-based census does not see the file it does not roster") pointed at an admission contract rather than a page. The cheap fix is to make the exemption nameable instead of inline — e.g. an internal static readonly IReadOnlySet<string> SkippedToolNames (or UnadmittedToolNames) holding run_custom_view_panel and WaitToolName with each one's reason, and let the census assert over that set. Then a third pass-through has to say why.
Worth stating in the comment while you are there: the early return also means the long poll never opens a ReadScope, so its catch-up reads contribute no statements to get_slow_reads (ReadStatementCapture.Sample samples only inside a scope with CaptureStatements). That is harmless — diagnostics only, and SensitiveStatementOutputFilter is a separate filter that still sweeps the answer — but it is a second consequence of the bypass that neither the comment nor the PR body names.
| /* #5822: the long-poll's catch-up read. Every request opened after $1 on the routes in $2 (all routes when NULL), oldest | ||
| first, whatever its state now: it answers what opened, and the claim decides what is still claimable. */ | ||
| internal const string OpenedSinceSql = @" | ||
| SELECT request_id, route_id, server_name, family, opened_at | ||
| FROM config.investigation_requests | ||
| WHERE opened_at > $1 | ||
| AND ($2::integer[] IS NULL OR route_id = ANY ($2)) | ||
| ORDER BY opened_at, request_id | ||
| LIMIT $3"; |
There was a problem hiding this comment.
The default call shape — route_ids omitted — has no index to use, and this is the read a polling agent runs forever.
The PR body's claim is carefully scoped and holds: with $2 non-null, ix_investigation_requests_route_opened (route_id, opened_at) (InvestigationTables.cs:117) can serve route_id = ANY ($2) AND opened_at > $1. But route_ids is optional and the tool documents omitting it as the normal case ("omit them for every route"), and with $2::integer[] IS NULL the predicate is opened_at > $1 alone. No index on config.investigation_requests leads with opened_at — the three are (server_id, family, state), (route_id, opened_at) and (state, expires_at) — so that read is a seq scan plus a sort for ORDER BY opened_at, request_id.
What makes it worth a line rather than a shrug is the traffic and the growth together:
- traffic: one read per call plus one per wake, per waiting agent, for as long as the agent loops — a 20-second poll is ~4,300 reads/day/agent;
- growth: nothing prunes this table. The expiry sweep moves rows to
state = 'expired'and the onlyDELETEreaching it is theON DELETE CASCADEfrom the child tables, soopened_at > now() - 5 minutesis a predicate over every request the store has ever held.
An ix_investigation_requests_opened (opened_at) would make both shapes index reads and give the planner the sort for free; (opened_at, route_id) would serve both from one index if you would rather not add a fourth.
I can't size the win from a read-only tree — it needs EXPLAIN (ANALYZE, BUFFERS) of OpenedSinceSql with $2 = NULL on a store with a real request history, which is on the reviewer checklist in my verdict.
|
Two non-blocking notes from the blind read, neither of them a finding: 1. The answer's page count is spelled While on the cap: 2. |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Claims
- VERIFIED — the transport is stateless, so nothing can be pushed outside a request's own response —
options.Stateless = true(DarlingMcpHostService.cs:1224), pinned byHostHeaderGuardTests.cs:161; the diff touches neither. - VERIFIED —
route_ids/after/timeout_seconds(0-25, default 20), an ordinary read —DarlingMcpInvestigationTools.cs:185-193, refusals at:203-207, no clamp anywhere; the name is in the test-sideReadOnlyTools. - VERIFIED — the answer is
statusok/empty, the five row fields and the cursor, no lease and no brief —WaitAnswerat:251-268, asserted field-for-field byInvestigationLongPollTests.AWait_WakesWhenARequestOpensOnItsRoute_AndAnswersOnlyTheIdServerFamilyRouteAndTime. - VERIFIED — one waiter per call, leaving on answer, timeout or cancellation, fired only on
Opened—InvestigationOpenSignal.cs:538-547/:572-623withusing var waiterat:220, and_openSignal?.Opened(route.RouteId)insidecase OpenedatInvestigationDispatcher.cs:157. - VERIFIED — one process-wide
Shared, worker fires and host waits —DarlingWorker.cs:3382-3383,DarlingMcpHostService.cs:1174;new InvestigationDispatcher(has exactly one production call site, so the optional parameter leaves no production path unfired. - VERIFIED —
opened_atis taken before the open-or-join lock waits, which is what the 5-minute overlap is for —Now()atInvestigationRequestStore.cs:501, locks at:508/:531, commit at:562, each wait capped byInvestigationDispatcher.LockTimeoutMilliseconds = 5000. - VERIFIED — rearmed before each catch-up read, so no wake is lost —
:223, ahead of both read branches. TheRearm/WakeCAS can drop a wake, and it is harmless for exactly that ordering reason: the read after the rearm sees the committed row. - VERIFIED — the census moves 186 to 187 (95 to 96 Darling-only), the tool is excluded from
/api/read, and the three named pins moved —DarlingMcpInstructions.cs:138against 187 distinctMcpServerTool(Name = "…")names in the Mcp folder (91 + 96, Lite overlap unchanged),DarlingWebEndpoints.cs:166, and the DI-seat / exclusion / read-only pins in the diff;McpServedSchema.IsServiceParameterclassifiesInvestigationOpenSignal, sosignalresolves from DI and is never served as a client argument. - UNVERIFIED — "the route-filtered read uses the existing index" — the index exists with the right leading column (
InvestigationTables.cs:117) and the predicate matches, but the plan (and how it satisfiesORDER BY opened_at, request_idover several route ids) needsEXPLAINon a real store. The$2 = NULLshape has no index at all — see the inline comment. - UNVERIFIED — the run totals (targeted 470; gate 4181 / 1134; rig 1061; the 3-test run) — build and rig results, unreachable from a read-only tree.
- UNVERIFIED — the new live test passed on a rig — it reads as consistent with
OpenedSinceSql(strict>floor, oldest first, the limit and route arms asserted), but it needs the darling-pg job. - UNVERIFIED — SDK 2.2.0's stateless behaviour (session id
"", GET refused,subscriptions/listenacknowledges empty) — the whole push-to-poll argument rests on it and only a runtime check settles it.
Riskiest lines
Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs:333—TryParsebounds the watermark only from above, andWatermark - CursorOverlap(:233,:318) throws belowDateTime.MinValue, so an ACCEPTED cursor reaches the error envelope instead of the refusal — UNPINNED (ABadTimeoutOrCursor_IsRefused_WithoutWaitingcovers the two grammar failures only).Darling/PerformanceMonitor.Darling.Service/Mcp/McpToolLatencyFilter.cs:92— a second, unrostered bypass of timing and admission; the census written to catch a change here matches theSkippedToolNamespelling and cannot see it — UNPINNED (ReadAdmissionCoverageCensusTests.TheFilterSkipsOnlyTheComposedPanelTool_AndTheSharedRunnerAdmitsItpasses while its name is false).Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpInvestigationTools.cs:311-318—Advance: the watermark moves only over rows put inknown, andseenis re-cut at the overlap floor; this is what answers a late commit once and nothing twice — pinned byARequestThatCommitsAfterANewerOneWasAnswered_IsStillAnswered_AndNeitherIsAnsweredTwiceandARequestOpenedBetweenTwoCalls_IsAnsweredByTheNextCallsCursor_Once.
Traps near this diff
- "Truncation inferred from a full page:
truncated = rows.Count >= limit… Fetchlimit + 1; the flag isfetched.Count > limit." — does not apply — no cut flag is published, andMaxWaitRowskeeps the 101st row out ofknownso the watermark never passes it (:311-318); the next call returns it. - "A
total_*assigned from a.Countis a count of the page unless the set is provably whole." — does not apply — the only count iscount = rows.Count(:258), the page's own count under a neutral noun (see my vocabulary note). - "
hours_back,limit,top,days_backare refused when out of range, never clamped." — applies, honoured for the numbers (:203-207) and BROKEN forafter: an accepted cursor can answerstatus = errorinstead of the refusal its own sentence promises. Finding 1. - "A JSON number above 2^53 is rounded by every JavaScript client … emit 64-bit ids as strings." — does not apply —
request_idis a Guid androute_ida small int (:261-262); the ticks ride inside the opaque cursor string (:321-323). - "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding." — does not apply — the investigation store is Darling-only (
McpToolGuideHeads.Investigations.cs:18: "Darling only (Lite has no investigation store, so there is no twin file)"), the cross-SKU head census compares only shared names, and the body scopes Lite to #5820. - "A roster-based census does not see the file it does not roster." — applies, to the admission contract rather than a page:
ReadAdmissionCoverageCensusTests.cs:135-140still asserts the filter skips only the composed panel. Finding 2. - "Tests are load-bearing (#5833) … a trap is answered with a behaviour test that fails on the defect." — does not apply as a violation — the eight new tests drive the real dispatcher and the real
WaitAsync(the fake is the store seam only), the SQL is exercised live, and no census was added.
Divergence (blind read vs PR body)
Three places. None is about the mechanism the body describes: the signal, the cursor and the rearm are all in the tree as written.
- "a bad timeout or cursor is refused" holds for the cursors the test sends, not for every cursor the parser accepts.
v1.0.parses, then throws on the first subtraction and answersstatus = error. The body's "worth a second look: the 5-minute cursor overlap" asks the right question and stops one step short of the parser's lower bound. - "Not timed or admitted" names the intent and leaves the exemption unrecorded.
ReadAdmission.SkippedRoutescould not hold it (its census demands a"health read: "reason), so inline was the only door — but the body does not note that the file's own census now describes code that is gone, nor that this is the one MCP read with no roster saying it is exempt. The same hunk also takes the tool out ofReadScope, so its reads reachget_slow_readsnot at all: harmless, unmentioned. - "The route-filtered read uses the existing index" is the true half.
route_idsis optional, the guide tells agents to omit it, and with$2NULL the predicate isopened_at > $1against a table whose three indexes all lead with something else and which nothing prunes.
Reviewer checklist
-
EXPLAIN (ANALYZE, BUFFERS)OpenedSinceSqlboth ways ($2 = NULLand$2 = '{1}') on a store with real request history, and decide whether(opened_at)or(opened_at, route_id)earns a fourth index before an agent polls it every 20 seconds for a year. - Call the tool with
after = "v1.0."and read the envelope: todaystatus = "error", not theinvalid+hints.parameterrefusal. - Run a real agent through a real proxy for several waits: confirm nothing cuts a 25-second response, and that concurrent waits do not disturb the admission limiter they now bypass.
- Watch the darling-pg job for
OpenedSince_AnswersRequestsOpenedAfterATime_…, reported from a local rig and not from the gate.
… untimed wait tool A cursor whose watermark is at or under the five-minute overlap (v1.0.) parsed, then threw on the read's subtraction and answered status error; the parser now refuses it with the invalid envelope. The latency filter's pass-through for the long poll is now the UntimedTools roster with its reason, and a behavior test calls the wait tool and a probe read through the real filter with admission full: the wait is neither admitted nor timed, the probe is both. The spelling census that counted the filter's skips is reduced to the shared-runner half it still checks.
Claude posting for Erik Darling. Round 1 answered at cd29752 (the cursor refusal, the UntimedTools roster with its behavior test, the index question answered with measurements in the body, the checklist answered); round 2 on cd29752 reached the bot's 30-turn cap with no review. Own run 38771663529 green.
erikdarlingdata
added
the
merge-train
1 participant
Claude posting for Erik Darling.
Closes #5822.
Why the issue's push became a long poll
The issue asked for an MCP notification on an open session (Erik's decision 1). That cannot be built on Darling's transport:
options.Stateless = trueinDarlingMcpHostService), for MCP server doesn't expose tools in Google Antigravity #1074 (clients that do not returnMcp-Session-Id) and for the/coretool subset, which holds only becauseConfigureSessionOptionsruns on every request.HostHeaderGuardTests.McpHost_KeepsTheTransportStatelessWhileCoreIsMappedpins it."", the GET stream is refused, andsubscriptions/listensends an empty acknowledgement and returns at once. There is no server-to-client channel outside a request's own response.Design
wait_for_investigation_requests(investigation family, an ordinary read likelist_investigation_requests). Parameters:route_ids(empty = every route),after(an opaque cursor),timeout_seconds(0-25, default 20; 25 so a proxy or client request timeout never cuts it).statusok or empty,requestsas{request_id, route_id, server_name, family, opened_at}, and the newcursor. No lease and no brief. The claim stays pull, and the first claim wins.InvestigationOpenSignal, an in-process waiter registry with one waiter per waiting call. A waiter leaves on answer, timeout or cancellation. The dispatcher fires it onOpenedonly, never onJoinedorLimitReached. One process-wide instance (Shared): the worker fires it and the MCP host waits on it.config.investigation_requestspast its cursor (InvestigationRequestStore.OpenedSinceAsync). Nothing is held in memory for an agent that is not waiting.opened_atanswered, plus the ids answered within 5 minutes of it.opened_atis taken before the open-or-join lock waits, so a request can commit after a newer one was answered. The read goes 5 minutes behind and skips the ids it has already answered, so a late commit is answered once and nothing is answered twice.McpToolLatencyFilterpasses the tool straight through. A 20-second wait would fill the read-latency percentiles and the slow-read record, and admission would hold a read slot for the whole wait.What changed
Darling/PerformanceMonitor.Darling.Service/Investigations/InvestigationOpenSignal.cs(new): the waiter registry.InvestigationDispatcher.cs: an optionalInvestigationOpenSignal, fired onOpened.DarlingWorkerpassesInvestigationOpenSignal.Shared.InvestigationRequestStore.cs:InvestigationOpenedRow,OpenedSinceSqlandOpenedSinceAsync(requests opened after a time on some routes, oldest first, any state). The route-filtered read uses the existing indexix_investigation_requests_route_opened (route_id, opened_at).DarlingMcpInvestigationTools.cs: the tool, itsWaitAsyncbody, andWaitCursor.DarlingMcpHostService.cs:AddSingleton<InvestigationOpenSignal>(InvestigationOpenSignal.Shared)inConfigureMcpServices.McpToolLatencyFilter.cs: the pass-through.DarlingWebEndpoints.ExcludedToolNames: excludes the tool from/api/read, because a long poll is not a page read.DarlingMcpInstructions.cs: the catalog census moves from 186 to 187 tools (95 to 96 Darling-only).Darling/README.md: one bullet besidelist_investigation_requestsin "Hand an alert to your own agent". It covers the tool, the answer, that the claim stays pull, that nothing is queued, and why it is not a push.McpServiceParameterDiSeatCensusTests) gainsInvestigationOpenSignal;DarlingWebEndpointsTests.ExcludedToolNames_AreTheNonReadSurfaceToolsgains the tool;DarlingMcpToolClassification.ReadOnlyToolsgains the tool.Commits: a884a48, ba3a067, f9c5e12.
Test plan
InvestigationLongPollTests(new, behavior, in process; the dispatcher plus the tool body over an in-memory store). It covers:InvestigationLongPollTests,InvestigationDispatcherTests,DarlingMcpInvestigationToolsTests,DarlingMcpInstructionsBudgetTests,McpServiceParameterDiSeatCensusTests(after its pin moved),*McpToolsList*,*McpToolGuide*,*McpHost*,McpPayloadContractCensusTests,*McpToolLatency*,DarlingMcpWritePolicyTests. Result:Total: 470, Errors: 0, Failed: 0, Skipped: 0.DarlingWebEndpointsTests.ReadEndpoints_AreExactlyTheReadToolCatalog_MinusTheDocumentedExclusions;CrossAppMcpToolInventoryPinTests.DarlingMcpTools_AreASupersetOfLite_ExceptKnownNamingDrift, because the tool attribute used a constant name and the scan needs the literal.Total: 4179, Errors: 0, Failed: 1, which wasExcludedToolNames_AreTheNonReadSurfaceTools. Its pin moved in f9c5e12.Total: 1134, Errors: 0, Failed: 0.classes: darling 2639, lite 656. Every class batch that had finished when this body was written:Failed: 0.InvestigationToolsLiveTests.OpenedSince_AnswersRequestsOpenedAfterATime_OnTheNamedRoutes_AndTheWaitToolAnswersThroughIt(new, live) was not run locally. The context and time wall came first, so no rig was started. CI's darling-pg job runs it.Sync with dev
origin/dev(fdcd88c, get_investigation_reports says when its window was cut, and the brief's field cut keeps emoji whole (#5834) #5850) into the branch. Head before: f9c5e12.Darling/Darling.Tests/InvestigationToolsLiveTests.cs. Both sides added a test at the same place in the file: this branch'sOpenedSince_AnswersRequestsOpenedAfterATime_OnTheNamedRoutes_AndTheWaitToolAnswersThroughItand get_investigation_reports says when its window was cut, and the brief's field cut keeps emoji whole (#5834) #5850'sReportsAndList_SayWhenTheWindowHeldMoreThanThePage. Both are kept, one after the other.DarlingMcpInvestigationTools.csandInvestigationRequestStore.csmerged automatically, with no overlap. The list and reports tools keep get_investigation_reports says when its window was cut, and the brief's field cut keeps emoji whole (#5834) #5850'srequests_returned,truncatedandjournal_truncated.wait_for_investigation_requestskeeps its own shape (route_ids, after, timeout_seconds, the five row fields, cursor). It has no page, so it carries notruncated.DarlingMcpInstructions.cs,DarlingWebEndpoints.csand the census tests had no conflict: get_investigation_reports says when its window was cut, and the brief's field cut keeps emoji whole (#5834) #5850 did not change the tool count or the tool list.pre-push.sh --skip-build, after a Debug build of both projects on the same head): GATE GREEN.Total: 4181, Errors: 0, Failed: 0, Skipped: 0. This covers item 1:DarlingWebEndpointsTests.ExcludedToolNames_AreTheNonReadSurfaceToolspasses.Total: 1134, Errors: 0, Failed: 0, Skipped: 0.classes: darling 2642, lite 657. All 23 batches hadFailed: 0. This run had no rig, so the live classes skipped. An earlier run of the same gate with the rig was stopped after 6 batches (allFailed: 0) because it would have passed the time limit.InvestigationToolsLiveTests,InvestigationLongPollTests,InvestigationRequestStoreLiveTests,InvestigationDispatcherTests,DarlingMcpInvestigationToolsTests,DarlingWebEndpointsTests,McpToolsListBudgetTests,McpServiceParameterDiSeatCensusTests,McpPayloadContractCensusTests,EffectiveReadWindowTests,InvestigationBriefComposerTests,AlertHistoryRangePageTests,CrossAppMcpToolInventoryPinTests,*McpTool*,*ToolsList*,*Census*,*DarlingMcpInstructions*:Total: 1061, Errors: 0, Failed: 0, Skipped: 0.OpenedSince_AnswersRequestsOpenedAfterATime_OnTheNamedRoutes_AndTheWaitToolAnswersThroughItran on the rig. Run by method together with get_investigation_reports says when its window was cut, and the brief's field cut keeps emoji whole (#5834) #5850'sReportsAndList_SayWhenTheWindowHeldMoreThanThePageandExcludedToolNames_AreTheNonReadSurfaceTools:Total: 3, Errors: 0, Failed: 0, Skipped: 0.Not done / for the coordinator
DarlingMcpInstructions.cshas no per-tool investigation text to sit beside: it only counts tools. Only the census count moved, and no sentence was added (it would cost every session's instructions).list_investigation_requests(server_name,family), not theserver/alert_familyof the ruling message, so an agent sees one shape across both tools.InvestigationOpenSignal.Shared, which follows theStoreHostProfileCache.Sharedpattern.Review round 1
Answers review 5535818262 (head 4e256f3). Commits 9dc3d20 (the fixes) and cd29752 (the cursor test bounded by a task timeout, not a stopwatch).
1. A cursor the parser accepted could answer
status = error(fixed).WaitCursor.TryParsenow refuses a watermark at or underCursorOverlap(3,000,000,000 ticks) as well as one aboveDateTime.MaxValue, soafter = "v1.0."answers theinvalidrefusal withhints.parameter = "after"before any waiter is registered, instead of throwing onWatermark - CursorOverlapand answering the error envelope. Because the read's floor is always aboveDateTime.MinValue, it never reaches Npgsql's -infinity mapping either. Test:InvestigationLongPollTests.ABadTimeoutOrCursor_IsRefused_WithoutWaitinggets three new rows (v1.0.,v1.2999999999.,v1.3000000000.). It now assertsstatus = invalidand thehints.parameterit names (before, it asserted only "not ok", which the error envelope passed), and that the call answers within 2 seconds with no waiter. A sibling,ACursorJustPastTheOverlap_IsAccepted_AndAnswersFromItsWatermark, pins the other side (v1.3000000001.answersemptywith its watermark). Proved against the old bound: the three new rows fail, the rest pass.2. The wait tool's admission and timing bypass had no roster (fixed).
McpToolLatencyFilter.UntimedToolsis now a roster of tool name to reason: the long poll, with its reason. The filter passes a call straight through when its tool is listed. It is notReadAdmission.SkippedRoutes, because that roster holds health reads, which are still timed, and its census requires a"health read: "reason. The spelling censusReadAdmissionCoverageCensusTests.TheFilterSkipsOnlyTheComposedPanelTool_AndTheSharedRunnerAdmitsItlost its regex count of the filter'sstring.Equalsskips, and is renamedTheSharedRunnerAdmitsTheComposedPanelToolfor the half it still checks (pre-existing, not this PR's). It is replaced by a behavior test,McpToolLatencyRecordingTests.TheInvestigationWait_IsNeitherTimedNorAdmitted_WhileAnotherRead_IsBoth. Over the realConfigureMcpServiceshost, with both admission slots and the whole queue taken and both tools planted as heavy, it calls the realwait_for_investigation_requestsand a probe read. The wait answers its owntimeout_secondsrefusal (so it was not admitted, or it would have answered busy) and leaves no read-latency sample. The probe answers busy and records oneBusysample. Proved against the old shape: with the roster check disabled, the test fails. The same pass-through keeps the tool out ofReadScope, so its catch-up reads never appear inget_slow_readsorget_read_latency. That is deliberate: a 25-second wait would otherwise dominate both.3. The unfiltered read and an
opened_atindex (no index, by measurement; the coordinator agreed). Planted on the rig (PostgreSQL 18.6, the managed runtime): 73,000 rows inconfig.investigation_requests. That is 20 routes at 10 requests a day for the 365-dayInvestigationRetentionDays, with a ~3 kB brief on the last 90 days (InvestigationBriefRetentionDays), a 13 MB heap (25 MB total), thenVACUUM ANALYZE.OpenedSinceSqlran as a prepared statement with$1 = now() - 5 minutesand$3 = 1000:$2 = NULL, custom planix_investigation_requests_route_opened,Index Cond: (opened_at > ...), Index Searches: 21 (skip scan)$2 = '{1}', custom planroute_id = ANY ('{1}') AND opened_at > ..., 1 search$2 = NULL, generic planopened_at > $1, 21 searches), Bitmap Heap Scan,Filter: ($2 IS NULL OR ...)$2 = '{1}', generic plan$2 = NULL, three timed runsThe caveat is PostgreSQL 16 and 17. A bring-your-own store can run them (the README minimum is 16), and neither has skip scan. With index scans and bitmap scans turned off to stand in for that worst case, the unfiltered read is a Seq Scan reading 1,722 buffers and removing 73,000 rows: 8.3 ms on the first run, 4.4 ms warm. A real 16/17 planner would more likely full-scan the route index (about 280 pages); I could not run that here. At one poll every 20 seconds, even the worst case is seconds of work per day per agent, on a table the retention bounds. A V186 rung (and the schema-probe chain a version bump carries), colliding with three other V186 branches, costs more than this read. If a store ever shows this read as a cost at scale, an
(opened_at)index is the first thing to add. Note thatget_slow_readscannot show it, because the tool is outsideReadScope(finding 2), so the signal would have to come frompg_stat_statementson the store.4. Checklist.
after = "v1.0.": now theinvalidrefusal (finding 1's test).InvestigationOpenSignalkeeps an unbounded list (one entry per waiting call, removed when the call ends), and the host sets no Kestrel connection limit or rate limiter. A waiter holds an HTTP request and a list entry, not a store connection. Its catch-up reads borrow a pooled connection only for the read, so the Npgsql pool's maximum bounds concurrent reads, not concurrent waits.OpenedSince_AnswersRequestsOpenedAfterATime_…: ran green in CI's darling-pg job on run 38759611602 (head 4e256f3), and again on the rig this round (below).Test plan (this round)
[guard-darling] Darling.Tests Total: 4181, Errors: 0, Failed: 0and[guard-lite] Lite.Tests Total: 1134, Errors: 0, Failed: 0;classes: darling 2654, lite 667 (symbols: 78 candidates). The first run's Guard failedTimingTestCensusTestson aStopwatchin the new cursor test; the follow-up commit bounds that test with a task timeout instead.InvestigationLongPollTests,McpToolLatencyRecordingTests,ReadAdmissionCoverageCensusTests:Total: 46, Errors: 0, Failed: 0.InvestigationLongPollTestsandTimingTestCensusTestsafter the follow-up commit:Total: 20, Errors: 0, Failed: 0.Total: 9, Failed: 4).Not done (this round)
[FAIL]before the stop) when a hold on window-opening runs arrived, and the lane's 60-minute wall came before the hold ended.McpToolsListBudgetTestsandMcpServiceParameterDiSeatCensusTests: not run locally; CI runs them. Neither names a changed symbol (no tool description or service parameter changed).InvestigationToolsLiveTestson the rig: not run this round. The live SQL is unchanged, and the class ran green in CI's darling-pg job on run 38759611602.SECTION: Added
ENTRY: - MCP long-poll: an agent waits for an investigation request to open for its routes ([#PR]) - New MCP tool
wait_for_investigation_requestswaits up to 25 seconds for an investigation request to open on the agent routes it names, then answers each new request's id, route, server, family and open time with a cursor for the next call. The claim staysclaim_investigation_request, and nothing is queued for an agent that is not waiting.