fix: pandas bulk path falls back to SQL on any failure (poisoned-buffer 500 loop) by vigneshnarayanaswamy · Pull Request #34 · block/model-ledger

vigneshnarayanaswamy · 2026-07-10T16:37:37Z

Production incident (2026-07-10, downstream deployment): on a session whose backing protocol cannot run PUT file transfers, write_pandas dies inside file_transfer_agent with KeyError('command'). That is not a privilege error, so the v0.7.10 fallback never engaged — the exception propagated to the caller, and because the write buffer was never cleared, every subsequent flush-before-read re-fired it: one buffered write turned every read on that process into a 500.

Fix: the pandas bulk path is an optimization, never a correctness requirement. Any failure now logs a warning (with exception type), best-effort drops the staging table, and returns False so the DDL-free SQL path — idempotent and, as of v0.7.10, carrying all columns — handles the flush.

Regression test simulates the exact scenario: pandas-capable-looking session + write_pandas raising KeyError('command') → flush must not raise, SQL MERGE must run, buffer must clear, second flush must be a no-op.

758 tests, ruff, mypy green. Version → 0.7.11.

🤖 Generated with Claude Code

…ilege errors Observed in production: on a session whose backing protocol cannot run PUT file transfers, write_pandas dies inside file_transfer_agent with KeyError('command') — not a privilege error — so the fallback never engaged, the exception propagated, and because the buffer was never cleared, every subsequent flush-before-read re-fired it (all reads 500 on that process). The pandas path is an optimization, not a correctness requirement: any failure now logs a warning, best-effort drops the staging table, and falls back to the DDL-free SQL path (idempotent, carries all columns as of v0.7.10). Regression test simulates the poisoned-buffer scenario and asserts fallback + buffer clear + quiet second flush. Bumps version to 0.7.11. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…35) Blind post-merge review of #33/#34 found the composition bug: Snowflake processes backslash escape sequences inside single-quoted constants, and _esc only doubled single quotes — so any JSON payload containing an embedded double quote (serialized by json.dumps as \") or a backslash reached PARSE_JSON mangled. On the SQL fallback path (which #34 routes all non-native-transport deployments onto) that meant: INSERT fails, buffer never clears, every subsequent flush-before-read 500s — the poisoned-buffer incident, reintroduced on common data. Verified live: PARSE_JSON('{"a": "x \" y"}') errors; the doubled form round-trips. Also from the review: privilege denials (the expected steady state of least-privilege roles) short-circuit quietly again instead of warning + issuing a doomed DROP per flush; unexpected failures log with exc_info; the failure-path cleanup DROP is gone (temp tables die with the session, and it could target a same-named permanent table); row construction sits inside the try; the null-payload regression test inspected the wrong side of FROM and could never fail — fixed; snapshot-path fallback and quote/backslash round-trip tests added. Version 0.7.12. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

…ter, trace depth, SQL lineage extraction) (#36) * fix: HttpLedgerBackend routed events to a phantom empty-name model Ledger(HttpLedgerBackend(url)) — the documented remote-backend path — corrupted the remote ledger three ways: - append_snapshot POSTed /record with model_name: "" (a comment claimed it was resolved server-side; it was not), so the server auto-registered a model literally named "" and attached every recorded event to it. It now resolves the real model name from the hash-to-name cache and raises ModelNotFoundError for unresolvable hashes instead of corrupting the remote inventory. Responses are raise_for_status()-checked. - register() double-logged the registration: save_model's POST /record already logs the registered event server-side, and the SDK's follow-up append_snapshot(registered) posted it again. The backend now skips that redundant post, and the record tool itself logs exactly one registered event per new model (Ledger.register grew an optional payload= merge so the caller's registration payload still lands on the single event). Re-registering an existing model is now idempotent (is_new_model=False, pointing at the original registration event) instead of appending duplicate registered events. - get_snapshot() always returned None. It now serves appended snapshots from a client-side cache and reconstructs older ones from GET /changelog (snapshot hashes are content-derived, so rebuilt snapshots recompute the same identity the server holds). Defense in depth: RecordInput.model_name now requires min_length=1, so old clients that still POST an empty model_name get a 422 instead of silently corrupting the ledger. New integration suite drives Ledger over HttpLedgerBackend against the real create_app() handlers (TestClient, not mocks): no phantom model, exactly one registered event, events attach to the named model, get_snapshot round-trips, and empty model_name is rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: query silently ignored the platform filter QueryInput, the REST /query endpoint, and the MCP query tool all advertise a platform parameter, but query() never read input.platform — any platform value returned the full inventory. Silently-wrong results for a documented filter. Platform is derived from snapshot data (newest discovered payload, then snapshot source), not a column on the model row, so it cannot ride the structural list_models pushdown. query() now resolves platforms for all structurally-matched models in one batched dispatch (batch_platforms — a single SQL round trip on SQL-backed backends) and filters/paginates on the result. An unknown platform now returns 0 models, not 35k. Adds platform filter tests (match, zero-match, text combination, pagination) and a schema-vs-implementation parity test that fails if QueryInput ever grows a filter field query() does not consume. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: registry dispatched xgboost sklearn-API models to the sklearn introspector Entry points load sorted by name (lightgbm < sklearn < xgboost) and find() returns the first can_handle match. XGBClassifier & friends are isinstance of sklearn.BaseEstimator, so the generic sklearn introspector claimed them and xgboost-specific extraction was lost (result carried introspector='sklearn', framework='scikit-learn'). LightGBM only escaped because 'lightgbm' happens to sort before 'sklearn'. SklearnIntrospector.can_handle now rejects estimators whose class comes from a wrapper framework with a dedicated introspector (xgboost.*, lightgbm.* modules), making dispatch independent of registry order. New dispatch tests (importorskip'd on the ML libs): XGBClassifier -> xgboost, LGBMClassifier -> lightgbm, plain LogisticRegression -> sklearn, and an order-independence check on can_handle itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: SQL lineage extraction silently dropped unqualified table names extract_tables_from_sql / extract_write_tables required 1-2 dotted segments, so INSERT INTO feature_store SELECT * FROM raw_events parsed to zero reads and zero writes — sql_connector(sql_column=...) produced zero lineage with no warning. The extract_write_tables docstring example (FROM source) was itself unextractable by its sibling function. Both extractors now accept bare (0-dot) identifiers, filtered against a small keyword blocklist so subqueries, table functions, and row sources (SELECT, LATERAL, TABLE(...), VALUES, ...) are not mistaken for tables. Qualified names are never keyword-filtered. INSERT OVERWRITE is also recognized instead of capturing OVERWRITE as a table name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: strip spaced template vars; correct strip_template_vars docstring example The regex only matched {{var}} with no interior whitespace, so standard spaced template variables like {{ ds }} passed through untouched and broke downstream SQL parsing. It now tolerates whitespace inside the braces. The docstring example also claimed an output the function never produced; it now shows the actual result, and a doctest-shaped test pins it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: use generic identifiers in DataPort case-normalization test Per the repo's boundary rule, examples in tests use generic names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: skip, don't fail, pandas-dependent tests when pandas is absent Six tests imported pandas mid-test with no importorskip (4 sample-model factory tests in test_datasets, plus one feature-names test each in test_introspect/test_sklearn and test_lightgbm). With sklearn/xgboost/ lightgbm installed but pandas absent — a combination no extra rules out, since pandas is only pulled in via the snowflake extra — the suite hard-failed instead of skipping. All six now importorskip pandas like every other optional dependency in the suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: remove vestigial excel extra The excel extra declared openpyxl>=3.1, but nothing in src/ or tests/ has imported openpyxl since the scanner module was removed (post-0.7.7). Drop the extra and its row in the installation guide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: create_app/create_server reject non-backend objects at construction Both factories accepted any object as backend — passing a Ledger (or a bare database path) by mistake failed deep inside the first request with a confusing AttributeError. A shared validate_backend() guard now raises a clear TypeError at construction, with targeted hints for the two common mistakes (a Ledger instance, a path string). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: sql_connector raises a clear error for tuple-row connections sql_connector addresses row values by column name, but a connection returning raw DB-API tuple rows (e.g. default sqlite3) crashed with an opaque 'TypeError: tuple indices must be integers or slices, not str' deep inside row mapping. discover() now checks each row is a mapping and raises a TypeError that names the requirement and the fix (dict row_factory / DictCursor). Docs: the connectors guide and the factory docstring now state the dict-row requirement with a sqlite3 row_factory example. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): export help said directory; it writes a single file model-ledger export --output described an 'Output directory' and the success message read 'exported to pack_out/', but the command writes one HTML file at exactly the given path (previously extensionless by default). Help now says 'Output file path', the default gained a proper .html extension, and the success message prints the real artifact path. A richer artifact layout (per-format extensions or a real directory) is a deliberate CLI change deferred to a later release. CLI test asserts the help text and success message match the artifact actually produced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: backfill CHANGELOG for 0.7.10-0.7.12; add 0.7.13 entry; bump version CHANGELOG.md stopped at v0.7.9 while the changelog URL is advertised in pyproject and the README. Backfills v0.7.10 (#32, #33), v0.7.11 (#34), and v0.7.12 (#35) from the release history, adds the v0.7.13 section for this fix batch, and bumps the package version to 0.7.13. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(trace): depth is the real BFS level, not flat-list position The trace tool computed each node's depth from its index in the flat transitive dependency list (depth = total - idx), so any model with N transitive upstreams rendered as an N-deep linear chain regardless of the graph's actual shape. Downstream traversal had the same fabrication. Traversal is now an explicit breadth-first walk that records each node's actual level (shortest edge distance from the traced model, one entry per node at its minimum depth) and terminates at input.depth, so depth-bounded queries do depth-bounded work instead of filtering after a full traversal. Regression fixture: 19-node fan-out DAG over 8 uneven levels with a diamond and a shortcut edge; 6 new tests fail on the old implementation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sql): CTE names and DELETE FROM targets are not read tables; handle INSERT OVERWRITE TABLE Bare-identifier acceptance turned every WITH-clause binding into a phantom read table — dbt-style multi-CTE SQL reported its own CTE names as lineage inputs, and a CTE sharing a real table's name created spurious cross-model dependency edges. WITH-clause names (including nested and RECURSIVE forms, with optional column lists) are now parsed and subtracted from extracted reads; DELETE FROM targets are likewise excluded (a delete target is not a read). Write extraction now recognizes the Hive/Spark form INSERT OVERWRITE TABLE t, which the keyword filter previously dropped. Docstrings document the keyword-blocklist tradeoff: a bare table genuinely named a filtered keyword must be schema-qualified to extract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(http): list_snapshots fetches the full history, not the changelog's 7-day default The changelog tool defaults since = now - 7 days when no bounds are given, and HttpLedgerBackend.list_snapshots called GET /changelog without one — so everything built on it was silently wrong for any model whose events are older than a week: get_snapshot reconstruction returned None, latest_snapshot returned None (Ledger.tag raised ModelNotFoundError on a healthy month-old model), the record tool's idempotency check missed the registration event, and the platform query filter returned total=0 for a known platform. list_snapshots now passes an explicit all-time since bound. Also cache the name mapping for hashes handed out by list_models: ModelSummary omits created_at, so those hashes are per-call placeholders; without the mapping, get_model/list_snapshots could not resolve them even for fresh events, which broke batch_platforms (and with it the platform filter) from any fresh client. Regression tests age server-side events 30 days and drive a cache-free second client through all four behaviors, plus the fresh-event hash-resolution case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(http): registration payload, tier, and actor reach the server Over HttpLedgerBackend the caller's registration payload was silently dropped: save_model posted payload={}, and the SDK's follow-up append_snapshot(registered) — the only carrier of the payload — was exactly the request the redundant-registration skip suppressed. Tier and actor were lost the same way (every remote registration landed as tier="unclassified", actor="user"). Ledger.register() now dispatches to an optional backend register_model hook when the backend defines one; HttpLedgerBackend implements it as a single POST /record carrying owner, model_type, purpose, tier, actor, and the caller's payload, then adopts the server's canonical hash as before. Backends without the hook keep the existing two-step save_model + append_snapshot path unchanged. RecordInput gains an optional tier field (REST body and MCP tool arg, additive); the record tool passes it through to register(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(connectors): accept sqlite3.Row and other keys()-style rows The dict-row guard used hasattr(row, "get") as a proxy for "addressable by column name", which rejected the stdlib's own mapping-style row type: sqlite3.Row supports keys() and row["col"] but has no .get(), so configurations that discovered nodes on 0.7.12 started raising the tuple-row TypeError. Rows with keys() are now materialized to dicts; only true positional rows are rejected. The docs example now recommends row_factory = sqlite3.Row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(backends): validate_backend duck-checks core methods instead of full-protocol isinstance The runtime_checkable isinstance check demanded all 14 protocol method names, so a handwritten partial backend (no list_snapshots_before or tag methods) that served create_app() traffic fine on 0.7.12 died with TypeError at construction — a compatibility break for third-party backends. The guard now rejects the two intended mistakes (a Ledger instance, a path string/PathLike) with the same hints and otherwise requires only a core-method subset, so previously-working partial backends keep constructing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(introspect): dispatch tests execute without the ML libraries The dispatch regression tests importorskipped numpy/sklearn/xgboost/ lightgbm, and neither CI nor the default dev install carries them — all four tests skipped everywhere, leaving the dispatch fix with zero executed coverage in the release pipeline. Add a stub-module layer: minimal sklearn/xgboost/lightgbm modules with the real inheritance shape (wrapper estimators subclass BaseEstimator) installed via monkeypatch, exercising registry.find() and SklearnIntrospector.can_handle in every environment, plus a __module__ = None safety case. With the can_handle fix reverted, the stub tests fail in a bare venv (verified). The real-library tests remain and still run where the ML extras are installed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * revert: keep 0.7.13 patch-safe — restore excel extra and export default filename Removing the excel extra broke `model-ledger[excel]` pins within a patch version (uv hard-errors on unknown extras), and changing the export default artifact from audit_pack to audit_pack.html broke scripts consuming the old default path. Restore both: the extra is marked deprecated (removal planned for 0.8.0) and the export help text keeps the corrected single-file wording. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tools): whitespace-only names rejected; trace total_nodes counts distinct models; public Ledger.backend Three review notes: - RecordInput.model_name rejects whitespace-only values (min_length=1 let " " register a near-invisible model); REST returns 422. - TraceOutput.total_nodes counts distinct models — a node reachable both upstream and downstream with direction="both" (e.g. a 2-cycle) was counted twice. - Ledger gains a public read-only backend property; tool functions use it instead of reaching into the private _backend attribute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: correct HTTP snapshot-identity claims; CHANGELOG for the fix round The get_snapshot comment implied rebuilt snapshots always recompute the identity the server holds; that is true only for server-minted events. Snapshots created client-side by Ledger.record() carry a client timestamp, so their hashes are process-local (cache-resolvable only) — say so explicitly. CHANGELOG: fold the review fix round into the v0.7.13 section (full changelog history over HTTP, registration fidelity, CTE/DELETE exclusions, sqlite3.Row, duck-typed backend validation, whitespace-name rejection, distinct total_nodes, stub-module dispatch coverage), add a compatibility note deferring the excel-extra removal and strict backend validation to 0.8.0, and drop the claims the review refuted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

vigneshnarayanaswamy merged commit e9ccd27 into main Jul 10, 2026
7 checks passed

vigneshnarayanaswamy mentioned this pull request Jul 10, 2026

fix: escape backslashes in SQL literals (poisoned-buffer on common payloads) + fallback hardening #35

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: pandas bulk path falls back to SQL on any failure (poisoned-buffer 500 loop)#34

fix: pandas bulk path falls back to SQL on any failure (poisoned-buffer 500 loop)#34
vigneshnarayanaswamy merged 1 commit into
mainfrom
fix/pandas-path-fallback-any-failure

vigneshnarayanaswamy commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vigneshnarayanaswamy commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant