Skip to content

Merge upstream 1.0 - #220

Open
piupiuxiupa wants to merge 9 commits into
apache:masterfrom
piupiuxiupa:merge-upstream-1.0
Open

Merge upstream 1.0#220
piupiuxiupa wants to merge 9 commits into
apache:masterfrom
piupiuxiupa:merge-upstream-1.0

Conversation

@piupiuxiupa

Copy link
Copy Markdown

No description provided.

Jinn and others added 9 commits August 14, 2026 14:32
…ion release

Ported from the pre-1.0 fork. aiomysql's Pool only frees a slot inside
release(); calling ensure_closed() on a checked-out connection leaves the
slot permanently counted as in-use, slowly exhausting the pool:

- _cleanup_stale_connections: bad connections found by the periodic
  stale test are now closed-then-released in a finally block instead of
  only ensure_closed() (this ran every ~2x health_check_interval and
  leaked one slot per stale connection found - the root cause of the
  original connection-exhaustion outage).
- _warmup_pool: the failure path now discards held warmup connections
  through the same close-then-release path.
- release_connection: an already-closed checked-out connection is now
  still release()d so the pool's in-use counter stays correct.

Adds a _discard_pool_connection helper (force-close then release) next
to _force_close_raw_connection.

Co-Authored-By: Claude <noreply@anthropic.com>
Adds a configurable ROUTE_PREFIX (e.g. /doris-mcp) so the server can sit
behind a reverse proxy under a sub-path. Ported from the pre-1.0 fork and
adapted to the 1.0 transport structure:

- config: route_prefix field on DorisConfig, ROUTE_PREFIX env read in
  from_env() with "/<segment>" normalization.
- main: --route-prefix CLI arg, prefix stripping + root_path rewrite at
  the top of the single-worker mcp_app dispatcher, / root-info endpoint,
  ROUTE_PREFIX propagated to uvicorn workers via _multiworker_environment.
- multiworker_app: prefix stripping + root_path rewrite in the top-level
  app(); root_info now reports the prefixed endpoint paths.
- start_server.sh: ROUTE_PREFIX env (opt-in), prefixed URL hints, and
  conditional --route-prefix passthrough.

The prefix is stripped in the outer ASGI routers (before exact-path
dispatch), so DorisMCPHTTPTransport and the legacy adapter see clean
/mcp paths and need no changes. uvicorn's own root_path is deliberately
not used: it prepends the prefix to scope["path"] instead of allowing
us to strip it.

Co-Authored-By: Claude <noreply@anthropic.com>
Ported from the pre-1.0 fork onto the 1.0 logger (which now also carries
the SensitiveDataFilter - preserved untouched):

- Port-suffixed log base name: setup_logging() gains base_name and
  ConfigManager passes doris_mcp_server_<port> for http transport so
  multi-instance deployments on different ports do not collide.
- TimestampRotatingFileHandler: size-based rollover renames to
  <base>_<YYYYmmdd_HHMMSS>.log instead of .log.N, pruning oldest
  backups; used for level files, the combined log, and the audit log.
- LogCleanupManager.cleanup_at_startup (default False): first cleanup
  pass waits one full interval so logs from a previous run are not
  purged on boot; start_server.sh no longer deletes *.log on startup.
- RequestAuthContextFilter: injects user/token_id/client_ip from the
  shared mcp_auth_context_var into every log record (reads upstream's
  richer AuthContext), attached to console, level, combined, and audit
  handlers; formatters show [user=... token=... ip=...].

Co-Authored-By: Claude <noreply@anthropic.com>
Ported from the pre-1.0 fork. _convert_numpy_types still raised
"Object of type Decimal is not JSON serializable" for any ADBC query
selecting a DECIMAL column, because Arrow Flight SQL exposes DECIMAL as
decimal.Decimal. Now coerces Decimal (to float, str on overflow),
datetime/date (isoformat), and bytes/bytearray (utf-8 decode, hex
fallback) in both the numpy-available path and the ImportError
fallback. The non-ADBC path already had equivalent handling upstream
via _serialize_row_data.

Co-Authored-By: Claude <noreply@anthropic.com>
Ported from the pre-1.0 fork. get_table_comment_async failed with
"Unknown column 'table_comment'" on Doris versions whose
information_schema.tables lacks TABLE_COMMENT, returning no comment.
On that error it now retries via SHOW TABLE STATUS (Comment column,
present on all versions) and normalizes the key, so table comments
resolve across Doris versions.

Co-Authored-By: Claude <noreply@anthropic.com>
Ported from the pre-1.0 fork. These acquire sites were missed by the
1.0 refactor and leaked a pool slot whenever an exception (or early
error-dict return) preceded release:

- analysis_tools: get_table_summary, analyze_column,
  analyze_table_relationships, get_performance_stats
- dependency_analysis_tools: analyze_dependencies (never released at
  all, on any path)

Each now releases via try/finally using the same defensive
getattr(connection_manager, "release_connection") idiom as the already
fixed get_sql_explain/get_sql_profile.

Co-Authored-By: Claude <noreply@anthropic.com>
- .env.example: safer pool defaults (DORIS_MAX_CONNECTIONS=8,
  DORIS_MAX_CONNECTION_AGE=1800) with a note on the
  WORKERS x max_connections vs Doris max_user_connection_num budget.
- .gitignore: ignore .claude/, .sisyphus/, .pytest_cache/.
- examples/examples.py: DorisUnifiedClient example (HTTP + stdio) from
  the fork; the HTTP URL uses the /doris-mcp route prefix.

Co-Authored-By: Claude <noreply@anthropic.com>
…handler

Missed in the logging re-port; without it _all.log lines show
user/token/ip placeholders as '-' even for authenticated requests.

Co-Authored-By: Claude <noreply@anthropic.com>
@FreeOnePlus

Copy link
Copy Markdown
Member

Thanks for working on bringing these changes forward. However, this PR is not reviewable or mergeable in its current form.

The PR description is missing

The title is only Merge upstream 1.0, and the PR body is completely empty. There is no explanation of:

  • what problem this PR is solving;
  • which upstream repository, fork, branch, or commit range the changes came from;
  • which behaviors are intentionally changed;
  • which Doris/MCP versions and deployment modes were tested;
  • what compatibility risks exist;
  • what test commands and results support the change.

For a PR with 9 commits, 13 changed files, and changes across routing, authentication-related flows, connection management, metadata, serialization, logging, deployment configuration, and examples, an empty description is not acceptable.

Please provide a complete PR description with motivation, scope, before/after behavior, compatibility impact, source provenance, and verification evidence.

Too many unrelated changes are mixed into one PR

This PR currently combines several independent areas:

  1. reverse-proxy ROUTE_PREFIX support;
  2. connection-pool lifecycle and leak fixes;
  3. ADBC result serialization;
  4. old-Doris metadata compatibility fallback;
  5. logging format, rotation, cleanup, and per-request context;
  6. deployment defaults and a new example client.

These changes have different risks, regression surfaces, reviewers, and rollback requirements. Combining them makes it difficult to review correctness, verify coverage, identify regressions, or revert one behavior without reverting everything else.

Please stop stacking additional fixes into this PR and split the work into focused PRs. A reasonable split would be:

  • PR 1: reverse-proxy route-prefix support, including OAuth, token-management, discovery, redirect, form-action, and single/multi-worker tests;
  • PR 2: connection-pool release/discard fixes, including a real maxsize=1 blocked-waiter concurrency regression test;
  • PR 3: ADBC serialization, with lossless high-precision DECIMAL tests;
  • PR 4: old-version table-comment fallback, with exact table matching and structured permission/backend error tests;
  • PR 5: logging rotation/context changes, including backup_count=0 and multi-process rollover behavior;
  • PR 6: deployment defaults, documentation, and examples.

Each PR should have its own complete description, focused commits, regression tests, and compatibility notes.

Current technical blockers

The corresponding focused PRs also need to address the following issues.

1. ROUTE_PREFIX only handles inbound paths

The outer ASGI routers strip the prefix, but generated redirects, form actions, browser fetch() calls, token-management URLs, and OAuth discovery/challenge URLs still use root-absolute paths such as /doris-login, /token/*, and /auth/login.

A reverse proxy that only exposes /doris-mcp/ can reach the first endpoint, but subsequent authentication requests leave the prefix and fail.

The prefix check also uses a bare startswith(), so /foo incorrectly matches /foo-bar/....

Please introduce one shared prefix-aware public URL helper, use it for every generated URL, and require an exact path-segment boundary:

path == prefix or path.startswith(prefix + "/")

Please add end-to-end prefixed tests for both single-worker and multi-worker modes.

2. ADBC converts exact Decimal values to float

Converting Doris DECIMAL values to IEEE-754 floats silently loses precision. Very large values may also become Infinity instead of entering the current exception fallback.

Please serialize exact decimal values losslessly, normally as strings under one shared result-serialization contract, and add high-precision and large-magnitude regression cases.

3. The old-version table-comment fallback is not an exact lookup

SHOW TABLE STATUS ... LIKE 'table_name' treats _ and % as wildcards, so a valid Doris table name can match a different table.

The fallback helper also catches every exception and returns an empty result, which can hide permission and backend failures and weaken the existing Doris OAuth structured 403/502 error contract.

Please:

  • trigger the fallback only for a verified unknown-column error;
  • perform an exact table-name match;
  • propagate fallback-stage authorization and backend failures;
  • add tests for underscore-containing table names;
  • add a two-stage failure test such as 1054 followed by 1142.

4. Closing and releasing a bad aiomysql connection does not wake blocked acquirers

Calling release() on an already-closed aiomysql connection removes it from the used set, but does not schedule the normal pool wakeup path.

A request that was already blocked while the pool was full can therefore remain blocked until timeout.

Please explicitly handle waiter wakeup and add a real concurrency regression test with maxsize=1:

  1. acquire the only connection;
  2. start a second acquire and verify that it is waiting;
  3. discard the first connection;
  4. verify that the waiter completes promptly without waiting for the acquisition timeout.

5. The logging rollover implementation has unsafe edge cases

backup_count=0 is currently valid, but the new handler still creates timestamped backup files and never prunes them, causing unbounded disk growth.

In multi-worker mode, all workers on the same port also write and rotate the same files without inter-process synchronization, which can cause rollover races and log loss.

Please preserve correct zero-backup semantics and use a genuinely multiprocess-safe design, such as:

  • a queue-based single writer;
  • PID-separated files;
  • or another handler with inter-process coordination.

6. examples/examples.py is not suitable in its current form

The new file:

  • is missing the ASF license header;
  • performs network and SQL operations at import time;
  • assumes a non-default /doris-mcp/mcp endpoint;
  • hard-codes an environment-specific table.

Please add the ASF header and a __main__ guard, use the default /mcp endpoint or configurable input, and use a portable example such as SELECT 1.

Requested next step

Please close this PR or convert it into a tracking PR, and submit the actual implementation as independently reviewable PRs.

Do not continue adding unrelated fixes to this branch. Each replacement PR should include:

  • a complete description;
  • a clearly bounded scope;
  • source provenance where code is being carried over;
  • before/after behavior;
  • compatibility impact;
  • focused regression tests;
  • exact verification commands and results.

Until the scope is restructured, the PR description is completed, and the blockers above are addressed with focused tests, this PR should not be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants