perf(gs): queue sheet exports, cap unbounded queries and monitor event-loop delay - #4346
Conversation
|
4 review passes to zero findings. Fixed along the way: worker slots could wedge permanently on a never-settling query (item timeout added); timed-out queued exports no longer execute after their caller already got an error (load-shedding in the shared queue util, with new spec coverage); the default maxLine cap moved from the DTO into the service so truncation is detected and WARN-logged per bank_tx sub-query instead of on the concatenated total; |
e2b81d3 to
a35462e
Compare
Status 2026-07-27 — rebased, and a correction to this PR's premiseRebased onto current The premise was partly wrongThis PR was written to address the API-wide slow episodes, on the theory that sheet-export post-processing was starving the Node event loop. Measurements taken during a live episode today refute that theory: DB-backed routes run 4–94× slower while The time is being spent waiting on database round-trips, not in JavaScript. What is still worth merging here, independent of that theory
What is now weakerThe event-loop-delay monitor was included as proof-of-mechanism for a mechanism that turns out not to apply here. It is cheap and still reasonable instrumentation, but it is no longer the point of this PR — happy to drop it for a tighter diff if a reviewer prefers. How to read this PR nowLoad reduction and a bounded-query fix, not the cure for the slow episodes. Sheet-export volume is flat at 15–16k requests/h around the clock and does not rise when an episode starts, so this work reduces a standing background cost rather than removing a trigger. The instrument that would settle the remaining question — locks versus disk I/O on the database side — is the slow-query and lock-wait logging prepared in the infrastructure config, which is still unmerged. |
…ault cap, consistent 400 on custom exports
… false-positive warns
…e emits no LIMIT with joins)
a35462e to
c44d5b6
Compare
❌ TypeScript: 4 errors |
The rebase onto develop combined two non-overlapping import additions for the same identifiers without git flagging a conflict: develop had added its own DbQueryDto/UserRole imports to this spec while the branch added a second set. tsc reported four TS2300 "Duplicate identifier" errors, so the suite could not compile. Merge DbReturnData into the existing absolute-path DbQueryDto import and drop the duplicate relative-path and UserRole lines.
- queue-handler: defer the action call into the promise chain. A synchronous throw previously escaped doWork without ever calling reject, leaving the queue item unsettled and hanging the caller until the queue timeout. - monitor-event-loop: implement OnModuleDestroy and disable the histogram, so its 20ms sampling timer stops on teardown instead of running on. - gs.service: route identifier and table through Util.sanitizeLogValue in warnIfCapped; client-controlled values must never land raw in a log line. - gs.service: replace any[] with Record<string, unknown>[] in the getExtendedBankTxData return type per the no-any rule.
Commit 1547091 deferred the action call in QueueItem.doWork so a synchronous throw rejects the item instead of leaving it unsettled. Add the regression test that pins that behaviour: a synchronously throwing action must reject with its own error (not with a queue timeout), and the worker slot must be free for the next item afterwards. Also switch the spec to the absolute import path and move the QueueHandler import into the src/shared group, per CONTRIBUTING.
|
Rebased onto current Conflict resolution — one conflict, in
Fixed during review:
Deliberately left for the author to decide — please confirm or reject:
|
|
Note on the Those four errors were the duplicate Evidence at the current head ( All 12 checks are green, including |
Problem
Part of #4227. Investigation of the reported internal-tooling slowness (07-21/22) showed that the dominant share of slow
/gs/dbrequests is not DB work: queries returning 0 rows from tiny tables (e.g.country, 250 rows) took 5–7.5s because sheet-export syncs fire in aligned bursts and their result post-processing runs synchronously on the event loop. Every in-flight request — including interactive support/compliance calls — absorbs those stalls. Additionally, ~22% of slow exports set nomaxLineat all (the DTO leaves it optional), so unbounded full-table pulls are possible by omission.Changes
GsService):POST /gs/dbandPOST /gs/db/customnow run through aQueueHandlerwithmaxWorkParallel = 2, a 240s queue timeout (below the Apps-Script 6-min execution cap) and a 240s item timeout (a query that never settles frees its worker slot instead of wedging the queue). Exports are latency-tolerant batch consumers; interactive requests no longer compete with a whole burst at once. Both endpoints report failures (incl. queue timeouts) consistently as HTTP 400 —/gs/db/custompreviously surfaced errors as opaque 500s.QueueHandlerload-shedding (shared util): items whose queue timeout fired while still waiting are discarded instead of executed — previously their action ran anyway and the result was silently dropped, doubling load exactly when the queue is backed up. Covered by a newqueue-handler.spec.ts.maxLinedefault cap (GsService): requests withoutmaxLine(absent ornull) now run with a cap of 10000 rows instead of unlimited (on the custom bank_tx export the cap applies per sub-query, and the truncation signal is derived per sub-query). Explicit values are untouched (the escape hatch for deliberate large exports, which also keeps the existing >100k alert mail reachable), and the largest observed no-limit export returns ~1.4k rows, so no existing consumer changes behavior. When a default-capped export actually hits the cap, a WARN log names the identifier and table — truncation is visible, not silent.MonitorEventLoopService): logsmean / p95 / maxevent-loop delay every 10s (same cadence/level as the existing connection-pool monitor, gated by a newProcess.MONITOR_EVENT_LOOP). Makes the next slowness report attributable in one log query instead of an elimination hunt.Tests
gs.service.spec.ts: queue caps concurrency at 2 and preserves per-call results; custom exports route through the same queue; default cap applies for absent andnullmaxLine, explicit values pass through; queue loops stopped in teardown.queue-handler.spec.ts(new): normal execution, discarded-after-timeout items never run, item timeout frees a wedged slot.queue-handler.spec.ts: additionally covers the synchronous-throw case — a synchronously throwing action must reject with its own error rather than sit until the queue timeout, and the worker slot must be free afterwards. Verified by mutation: reverting the deferral indoWorkmakes exactly that test fail (Expected substring: "boom"/Received message: "Queue timeout"after 1074 ms).developon 07-30 (the branch had been conflicting since 07-27). Numbers below are from that rebased head, not the original branch state.Verification after deploy
EventLoop delaylines appear in the API log; during sheet-sync bursts p95 shows the stall magnitude.msvalues) during burst minutes should drop;/gs/dbthroughput is unchanged, individual syncs may queue briefly.hit the default maxLine capWARN identifies a sheet that needs pagination or an explicit limit.