fix(editor): bound CSV export, guard concurrent runs, audit query can… - #76
Merged
venkateshsakamuri-lab merged 1 commit intoAug 22, 2026
Conversation
…cels Four defects found in a production-readiness review of the Editor tab, all verified live against the running stack (real browser, real API, real DB) rather than by reading code. **CSV export was unbounded and outran the proxy.** `handleExportResults` re-ran the query with `limit: null` and `timeoutSeconds: 600`. With no limit, `QueryExecutorService` never calls `setMaxRows` and never appends a LIMIT, so every row accumulates into an unbounded ArrayList, is serialized to JSON, then held again in the browser as an array + CSV string + Blob. Observed live: a 50,000-row table returned a 7.1MB JSON payload in one response. The blast radius is the whole instance, not the exporter — a large enough export is an OOM for every tenant on that backend. The 600s timeout also violated the rule already documented for this file: `docker/nginx/default.conf` gives up at `proxy_read_timeout 300s`, so a long export returned an opaque 504 while the query kept running, and it sent no executionId, making it uncancellable by any means. Export now uses EXPORT_ROW_LIMIT (100k), the same 240s budget as Run, and carries an executionId + abort signal. The button label no longer promises "all N rows" when it will return at most 100k. **Cmd+Enter had no in-flight guard.** The Run button correctly swaps to Stop during a run, but the keyboard shortcut bypassed that and called `handleRunQuery` unconditionally. Both runs shared one `abortControllerRef` / `executionIdRef` slot, so a second run discarded the first's cancel handle, and whichever response landed last won the results panel — a user could read rows from a query they'd already replaced, on the same screen where they decide what to UPDATE or DELETE. Adds `isRunningRef` (re-entry guard) and `runSeqRef` (per- run stamp) so a superseded response is dropped and only the current run clears the shared refs. `handleStopQuery` releases the guard so Stop-then-rerun still works. Verified: 14 rapid-fire attempts across one 5s query window produce exactly 1 request. `handleExportResults` gets the same guard. It is currently unreachable during a run — `handleRunQuery` clears `results`, which hides the Export button — but that is incidental to unrelated state handling, not a guarantee, so the check is explicit rather than load-bearing on a side effect. **containsWhereClause matched the literal string " WHERE ".** A newline before WHERE (any multi-line formatted statement) read as *no* WHERE clause, while a commented-out `-- WHERE ...` satisfied the guard. Now word-bounded and comment-stripped. Only reachable via the keyword-fallback path: JSqlParser 5.2 parses ordinary multi-line DELETE/UPDATE fine and those use `Delete.getWhere()` / `Update.getWhere()`, but it cannot parse `EXPLAIN UPDATE`/`EXPLAIN DELETE` at all, so those fall through to this check. The two new tests cover exactly that path and were confirmed to fail before the fix and pass after. **Cancelling a query wrote no deliberate audit event.** Every execute outcome is audited; cancel was not. The only trace of a successful cancel was the killed query's own thread logging `pg_terminate_backend`'s error as an ordinary EDITOR_QUERY_FAILED — indistinguishable from any other failure, and dependent on a Postgres-specific error string. A cancel that missed its target (already finished, or a guessed/replayed executionId) left nothing at all. Adds EDITOR_QUERY_CANCELLED (SUCCESS with the terminated pid / INFO for a no-op) plus an executionId in the audit metadata, and audits the authorization rejection that was previously a silent 404. Verified end-to-end on the running stack: cross-user cancel returns 404, is audited as EDITOR_QUERY_BLOCKED against the *caller's* user id, and the target query provably survives (ran its full 14019ms and returned success). Mutation confirm flow, non-admin mutation block, and DELETE ... WHERE all re-checked for regressions with before/after DB row counts. Backend suite: 58/58 green. Not addressed here: the client-side `hasMultipleStatementsWithoutSemicolons` guard rejects valid multi-line SQL — including the output of the Editor's own Format button — before any request is sent. Left for a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
notSumit25
requested review from
a team,
geekypunk and
venkateshsakamuri-lab
as code owners
August 22, 2026 07:35
Contributor
|
@notSumit25 add the screenshots of the UI changes. |
Collaborator
Author
venkateshsakamuri-lab
approved these changes
Aug 22, 2026
venkateshsakamuri-lab
deleted the
fix/editor-concurrency-export-cap-cancel-audit
branch
August 22, 2026 08:44
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.

…cels
Four defects found in a production-readiness review of the Editor tab, all verified live against the running stack (real browser, real API, real DB) rather than by reading code.
CSV export was unbounded and outran the proxy.
handleExportResultsre-ran the query withlimit: nullandtimeoutSeconds: 600. With no limit,QueryExecutorServicenever callssetMaxRowsand never appends a LIMIT, so every row accumulates into an unbounded ArrayList, is serialized to JSON, then held again in the browser as an array + CSV string + Blob. Observed live: a 50,000-row table returned a 7.1MB JSON payload in one response. The blast radius is the whole instance, not the exporter — a large enough export is an OOM for every tenant on that backend. The 600s timeout also violated the rule already documented for this file:docker/nginx/default.confgives up atproxy_read_timeout 300s, so a long export returned an opaque 504 while the query kept running, and it sent no executionId, making it uncancellable by any means. Export now uses EXPORT_ROW_LIMIT (100k), the same 240s budget as Run, and carries an executionId + abort signal. The button label no longer promises "all N rows" when it will return at most 100k.Cmd+Enter had no in-flight guard. The Run button correctly swaps to Stop during a run, but the keyboard shortcut bypassed that and called
handleRunQueryunconditionally. Both runs shared oneabortControllerRef/executionIdRefslot, so a second run discarded the first's cancel handle, and whichever response landed last won the results panel — a user could read rows from a query they'd already replaced, on the same screen where they decide what to UPDATE or DELETE. AddsisRunningRef(re-entry guard) andrunSeqRef(per- run stamp) so a superseded response is dropped and only the current run clears the shared refs.handleStopQueryreleases the guard so Stop-then-rerun still works. Verified: 14 rapid-fire attempts across one 5s query window produce exactly 1 request.handleExportResultsgets the same guard. It is currently unreachable during a run —handleRunQueryclearsresults, which hides the Export button — but that is incidental to unrelated state handling, not a guarantee, so the check is explicit rather than load-bearing on a side effect.containsWhereClause matched the literal string " WHERE ". A newline before WHERE (any multi-line formatted statement) read as no WHERE clause, while a commented-out
-- WHERE ...satisfied the guard. Now word-bounded and comment-stripped. Only reachable via the keyword-fallback path: JSqlParser 5.2 parses ordinary multi-line DELETE/UPDATE fine and those useDelete.getWhere()/Update.getWhere(), but it cannot parseEXPLAIN UPDATE/EXPLAIN DELETEat all, so those fall through to this check. The two new tests cover exactly that path and were confirmed to fail before the fix and pass after.Cancelling a query wrote no deliberate audit event. Every execute outcome is audited; cancel was not. The only trace of a successful cancel was the killed query's own thread logging
pg_terminate_backend's error as an ordinary EDITOR_QUERY_FAILED — indistinguishable from any other failure, and dependent on a Postgres-specific error string. A cancel that missed its target (already finished, or a guessed/replayed executionId) left nothing at all. Adds EDITOR_QUERY_CANCELLED (SUCCESS with the terminated pid / INFO for a no-op) plus an executionId in the audit metadata, and audits the authorization rejection that was previously a silent 404.Verified end-to-end on the running stack: cross-user cancel returns 404, is audited as EDITOR_QUERY_BLOCKED against the caller's user id, and the target query provably survives (ran its full 14019ms and returned success). Mutation confirm flow, non-admin mutation block, and DELETE ... WHERE all re-checked for regressions with before/after DB row counts. Backend suite: 58/58 green.
Not addressed here: the client-side
hasMultipleStatementsWithoutSemicolonsguard rejects valid multi-line SQL — including the output of the Editor's own Format button — before any request is sent. Left for a separate change.