Skip to content

Commit fe245b3

Browse files
committed
fix: Harden the code
1 parent 8bd9322 commit fe245b3

5 files changed

Lines changed: 57 additions & 13 deletions

File tree

src/batch_import.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,9 @@ struct BatchExecutionContext {
166166
// to `workers_number` (e.g. 32) ROUTE calls at startup. The TTL expiry of
167167
// the last route determines when sessions are refreshed (see Run).
168168
if (config.routed_connection) {
169-
sessions[thread_i] = utils::bolt::MakeRoutedBoltSession(config, &expiry);
169+
sessions.push_back(utils::bolt::MakeRoutedBoltSession(config, &expiry));
170170
} else {
171-
sessions[thread_i] = MakeBoltSession(config);
171+
sessions.push_back(MakeBoltSession(config));
172172
}
173173
if (!sessions[thread_i].get()) {
174174
MG_FAIL("a session uninitialized");
@@ -311,7 +311,8 @@ int Run(const utils::bolt::Config &bolt_config, int batch_size, int workers_numb
311311
// refresh out of the parallel rounds where it would be race-prone.
312312
if (bolt_config.routed_connection && std::chrono::steady_clock::now() >= execution_context.expiry) {
313313
for (uint64_t thread_i = 0; thread_i < execution_context.max_concurrent_executions; ++thread_i) {
314-
execution_context.sessions[thread_i] = utils::bolt::MakeRoutedBoltSession(bolt_config, &execution_context.expiry);
314+
execution_context.sessions[thread_i] =
315+
utils::bolt::MakeRoutedBoltSession(bolt_config, &execution_context.expiry);
315316
if (!execution_context.sessions[thread_i].get()) {
316317
MG_FAIL("failed to re-route a worker session after TTL expiry");
317318
}

src/interactive.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,12 @@ int Run(utils::bolt::Config &bolt_config, const std::string &history, bool no_hi
188188
}
189189
continue;
190190
}
191-
auto ret = query::ExecuteQuery(session.Get(), query->query, param_store.AsMap().get(), bolt_config.db);
191+
// Resolve the session (may proactively re-route) before observing this query: the re-route decision must
192+
// use the transaction state as it was *before* this query, so a COMMIT/ROLLBACK still runs on the session
193+
// that holds the open transaction.
194+
auto *session_ptr = session.Get();
195+
session.ObserveQuery(query->query);
196+
auto ret = query::ExecuteQuery(session_ptr, query->query, param_store.AsMap().get(), bolt_config.db);
192197
if (ret.records.size() > 0) {
193198
Output(ret.header, ret.records, output_opts, csv_opts);
194199
}

src/serial_import.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,11 @@ int Run(const utils::bolt::Config &bolt_config, const format::CsvOptions &csv_op
3636
}
3737

3838
try {
39-
auto ret = query::ExecuteQuery(session.Get(), query->query, nullptr, bolt_config.db);
39+
// Resolve the session (may proactively re-route) before observing this query so transaction-control
40+
// statements in a dump stream are not interrupted by a re-route mid-transaction.
41+
auto *session_ptr = session.Get();
42+
session.ObserveQuery(query->query);
43+
auto ret = query::ExecuteQuery(session_ptr, query->query, nullptr, bolt_config.db);
4044
if (ret.records.size() > 0) {
4145
Output(ret.header, ret.records, output_opts, csv_opts);
4246
}

src/utils/bolt.cpp

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,9 @@ mg_memory::MgSessionPtr MakeRoutedBoltSession(const Config &config, std::chrono:
139139
return mg_memory::MakeCustomUnique<mg_session>(nullptr);
140140
}
141141
mg_map_insert(routing.get(), "address", mg_value_make_string(coord_address.c_str()));
142-
mg_map_insert(extra.get(), "db", mg_value_make_string(config.db.c_str()));
142+
if (!config.db.empty()) {
143+
mg_map_insert(extra.get(), "db", mg_value_make_string(config.db.c_str()));
144+
}
143145

144146
mg_map *rt_raw = nullptr;
145147
const int status = mg_session_route(coord.get(), routing.get(), nullptr, extra.get(), &rt_raw);
@@ -152,7 +154,7 @@ mg_memory::MgSessionPtr MakeRoutedBoltSession(const Config &config, std::chrono:
152154

153155
// 3. Parse the routing table: TTL (seconds) and the WRITE (main) instance.
154156
const mg_value *ttl_val = mg_map_at(rt.get(), "ttl");
155-
if (ttl_val != nullptr && expiry_out != nullptr) {
157+
if (ttl_val != nullptr && mg_value_get_type(ttl_val) == MG_VALUE_TYPE_INTEGER && expiry_out != nullptr) {
156158
*expiry_out = std::chrono::steady_clock::now() + std::chrono::seconds(mg_value_integer(ttl_val));
157159
}
158160

@@ -224,10 +226,18 @@ void RoutedSession::Rebuild() {
224226
}
225227

226228
mg_session *RoutedSession::Get() {
227-
if (routed_ && std::chrono::steady_clock::now() >= expiry_) {
228-
// Proactive TTL re-route: the previous routing table has expired, fetch a
229-
// fresh one (and possibly a new main) before handing back the session.
230-
Rebuild();
229+
// Proactive TTL re-route: once the previous routing table has expired, fetch a fresh one (and possibly a new
230+
// main) before handing back the session. Suppressed while an explicit transaction is open (re-routing would
231+
// silently drop it) and made non-destructive: a failed re-route keeps the existing working session rather than
232+
// discarding a healthy connection over a transient coordinator hiccup.
233+
if (routed_ && !in_transaction_ && std::chrono::steady_clock::now() >= expiry_) {
234+
auto refreshed = MakeRoutedBoltSession(config_, &expiry_);
235+
if (refreshed.get() != nullptr || session_.get() == nullptr) {
236+
session_ = std::move(refreshed);
237+
} else {
238+
// Keep the still-usable session; back off so we retry periodically instead of on every call.
239+
expiry_ = std::chrono::steady_clock::now() + kRerouteRetryBackoffSec;
240+
}
231241
}
232242
return session_.get();
233243
}
@@ -236,4 +246,15 @@ void RoutedSession::Reconnect() { Rebuild(); }
236246

237247
bool RoutedSession::Connected() const { return session_.get() != nullptr; }
238248

249+
void RoutedSession::ObserveQuery(const std::string &query) {
250+
// Track explicit-transaction state from the transaction-control keyword so proactive re-routing can avoid
251+
// tearing down an open transaction. Best-effort: matches the leading keyword of the trimmed query.
252+
const auto upper = utils::ToUpperCase(utils::Trim(query));
253+
if (upper.rfind("BEGIN", 0) == 0) {
254+
in_transaction_ = true;
255+
} else if (upper.rfind("COMMIT", 0) == 0 || upper.rfind("ROLLBACK", 0) == 0) {
256+
in_transaction_ = false;
257+
}
258+
}
259+
239260
} // namespace utils::bolt

src/utils/bolt.hpp

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,10 @@ class RoutedSession {
5151
explicit RoutedSession(Config config);
5252

5353
// Returns the underlying session. In routed mode, if the routing table TTL
54-
// has expired, transparently re-routes (rebuilds the session) first. May
55-
// return nullptr if a (re-)connection attempt failed.
54+
// has expired (and no explicit transaction is open), transparently re-routes
55+
// first; a failed re-route keeps the existing session. May return nullptr if
56+
// a (re-)connection attempt failed and there is no prior session to fall back
57+
// on.
5658
mg_session *Get();
5759

5860
// Forces a rebuild of the session (direct or routed per config). Used by the
@@ -62,13 +64,24 @@ class RoutedSession {
6264
// True if a session is currently established.
6365
bool Connected() const;
6466

67+
// Observes a query about to be executed so the session can track explicit
68+
// transaction boundaries (BEGIN/COMMIT/ROLLBACK) and suppress proactive
69+
// re-routing while a transaction is open. Safe to call in direct mode (no-op
70+
// effect on routing).
71+
void ObserveQuery(const std::string &query);
72+
6573
private:
74+
// Backoff applied after a failed proactive re-route so we don't retry on
75+
// every Get() while the coordinator is briefly unreachable.
76+
static constexpr std::chrono::seconds kRerouteRetryBackoffSec{2};
77+
6678
void Rebuild();
6779

6880
Config config_;
6981
mg_memory::MgSessionPtr session_;
7082
std::chrono::steady_clock::time_point expiry_{};
7183
bool routed_;
84+
bool in_transaction_{false};
7285
};
7386

7487
} // namespace utils::bolt

0 commit comments

Comments
 (0)