fix(rfc): own scan state per execution, lease connection slots, pin credentials - #140
Merged
Merged
Conversation
SetLevel, SetTraceDirectory, SetOutputMode, SetMaxFileSize and SetRotation each held
trace_mutex across an Info() call. Info() reaches WriteToFile(), which locks the same
plain std::mutex, so any of these called while file tracing was active deadlocked the
calling thread -- and every later tracer call behind it.
It presents as a hung query rather than a lock error, which is why it survived: the
sequence CLAUDE.md documents for debugging SAP communication
SET erpl_trace_enabled = TRUE;
SET erpl_trace_level = 'DEBUG';
SET erpl_trace_output = 'file';
is exactly the sequence that triggers it. I hit this earlier in this work and wrote it
off as trace verbosity.
SetEnabled already scoped its lock and logged afterwards; the others now do the same.
EnsureTraceFile stays inside the lock in SetTraceDirectory because it touches
trace_file.
The test runs each setter on a DETACHED thread with a deadline. It cannot use
std::async: that future joins in its destructor, so the watchdog would hang on the very
deadlock it is meant to catch -- which it did on the first attempt.
…redentials
Three defects with one root cause: mutable per-scan state living on RfcReadTableBindData,
which DuckDB reuses across executions of a bound plan.
1. A re-scanned sap_read_table returned nothing, silently.
PREPARE q AS SELECT count(*) FROM sap_read_table('SFLIGHT');
EXECUTE q; -- 94
EXECUTE q; -- 0
The column state machines were bind-owned, so the second scan resumed from an
exhausted cursor. Every re-scanned plan hit it: a prepared statement, a nested-loop
join, an un-materialised CTE referenced twice. Serial scans now build their machines
in the global state, which is created per execution -- the pattern the partitioned
path already used, which is why PARTITIONS was never affected.
Step() and HasMoreResults() now take the machine set explicitly instead of reading it
from bind data. That is what found the other three sites with the same defect:
sap_show_tables, sap_odp_show_subscriptions and the table lister behind ATTACH. A
convenience overload reading bind data would have left all three broken.
2. The persistent-connection budget was spent permanently. TryReservePersistentSlot was a
monotonic fetch_add on bind data that never released a slot -- not on failure, not
when the connection was dropped -- so once `cap` attempts had been made no machine
ever won a slot again, including in later executions. Slots are now leased: returned
on a failed reservation, returned in InvalidateCachedConnection, reset per execution.
3. A long scan could follow a secret replaced underneath it. OpenNewConnection resolved
the DuckDB secret on EVERY open, so replacing it mid-query sent later windows to a
different SAP system with no error and nothing in the result to show it. Credentials
are resolved once per execution and pinned.
Adds sap_rfc_live_connections() / _opened() / _closed(). Every open connection is a
session and a work-process reservation on the SAP system, and client-side timing shows
nothing when one is never released -- the cost is entirely on the source system. The live
count must be 0 between queries.
Tests, written red first:
sap_read_table_rescan.test -- prepared statement, content checksum (a
count-only assertion passes while rows differ),
two scans in one statement, early LIMIT, and the
partitioned path as non-regression guard
sap_rfc_connection_release.test -- live count returns to baseline after a plain
scan, a partitioned scan, an early LIMIT, a
FAILING scan, and repeated executions; plus a
guard that the counters actually move, so a stub
returning 0 would fail
Suites: offline partition 11/11, batching 6/6, tracing 1/1. Live, all with zero failures
and zero known gaps: RFC nwrfc, RFC proto, ODP nwrfc, ODP proto, BICS nwrfc.
…n-scan-state # Conflicts: # CHANGELOG.md
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.
Implements the plan from a five-persona crew investigation of erpl's RFC connection
lifecycle. The investigation overturned my own diagnosis, and the earlier review's: the
defect is bind-data ownership of mutable scan state, not connection churn. The
churn-based fix I had proposed is not in this PR, and the previous review's destructor
fix was retracted by its own authors as a no-op.
1. A re-scanned
sap_read_tablereturned nothing, silentlyColumn state machines lived on bind data, which DuckDB reuses across executions of a
bound plan, so the second scan resumed from an exhausted cursor. Any re-scanned plan
hits it: a prepared statement, a nested-loop join, an un-materialised CTE referenced
twice. This is in released erpl and returns wrong data rather than failing.
Serial scans now build their machines in the global state, which is created per
execution — the pattern the partitioned path already used, which is why
PARTITIONSwasnever affected and serves as the non-regression guard.
Step()andHasMoreResults()now take the machine set explicitly rather than readingit from bind data. That is what found three more sites with the same defect —
sap_show_tables,sap_odp_show_subscriptions, and the table lister behindATTACH. Aconvenience overload would have left all three broken and the compiler silent.
2. The persistent-connection budget was spent permanently
TryReservePersistentSlotwas a monotonicfetch_addon bind data that never released aslot — not on a failed reservation, not when the connection was dropped. Once
capattempts had been made, no machine won a slot again, including in later executions.
Slots are now leased: returned on failure, returned in
InvalidateCachedConnection,reset per execution.
3. A long scan could follow a secret replaced underneath it
OpenNewConnectionre-resolved the DuckDB secret on every open. Replacing the secretmid-query sent later windows of the same scan to a different SAP system — no error, and
nothing in the result to show it. Credentials are now resolved once per execution.
Observability:
sap_rfc_live_connections()Also
sap_rfc_connections_opened()/_closed(). Every open connection is a session anda work-process reservation on the SAP system, and client-side timing reveals nothing
when one is never released — the entire cost lands on the source system. Between
queries the live count must be 0.
This existed to make the fix testable. Measured before it: disabling the connection cache
entirely (
erpl_rfc_max_persistent_connections = 0) changed wall clock by nothing atpartitions=8(1.52s vs 1.49s) orpartitions=1(44.9s vs 42.3s) — which is why atiming-based test would have proved nothing here.
Tests, written red first
sap_read_table_rescan.test— prepared statement executed repeatedly; anorder-insensitive content checksum (a count-only assertion passes while the rows
differ); one statement scanning the function twice; an early
LIMITthat leaves themachines mid-table rather than exhausted; and the partitioned path as non-regression.
sap_rfc_connection_release.test— the live count returns to baseline after a plainscan, a partitioned scan, an early
LIMIT, a failing scan, and three executions ofone prepared plan. Plus a guard that the counters actually move, so a stub returning 0
would fail every other assertion in the file.
Asserted as a delta against a baseline taken in the same session, not an absolute, so
the file does not depend on what ran before it.
Bumps the
odpgitlink for DataZooDE/erpl-odp#11, which carries the matchingsap_odp_show_subscriptionsfix.Not in this PR
The partitioned scan still invalidates its connection at every window boundary. With
slots now leased that is accounted correctly, but whether the connection should survive a
window is a separate question — and the measurement above says it is not a throughput
question. It wants its own change and its own logon-rate evidence.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.