Skip to content

Add killAllSessions gateway support (#526)#9

Draft
richardsimmonds wants to merge 1 commit into
mainfrom
feat/526-kill-all-sessions
Draft

Add killAllSessions gateway support (#526)#9
richardsimmonds wants to merge 1 commit into
mainfrom
feat/526-kill-all-sessions

Conversation

@richardsimmonds

Copy link
Copy Markdown
Owner

Description

Implements the killAllSessions command in the DocumentDB gateway. Addresses upstream issue documentdb#526 (see also documentdb#402).

Follows the same gateway-level session cleanup model already used for killSessions / endSessions — invalidating gateway-tracked cursors and aborting active gateway transactions — rather than terminating PostgreSQL backends directly.

Supported command shapes

  • killAllSessions: [] — kills all gateway-visible sessions (all cursors invalidated, all active transactions aborted).
  • killAllSessions: [{ user: "<user>", db: "admin" }, ...] — kills only sessions owned by the listed users. Patterns scoped to a db other than admin are ignored, since gateway principals authenticate against admin.

Behavior details

  • Rejected with Unauthorized (code 13) when run against any database other than admin, matching the existing killOp behavior.
  • Listed in listCommands with admin_only: true.
  • Returns ok: 1.
  • Cursor ids are deduped before the best-effort execute_kill_cursors call; failures there are logged as warnings (same as killSessions).

Changes

  • cursor_store.rs: add invalidate_all_cursors and invalidate_cursors_by_owner_name.
  • transaction_store.rs: add remove_all_transactions and remove_transactions_by_owner_names; refactor shared removal path into remove_transaction_by_key.
  • processor/session.rs: new kill_all_sessions handler + unit tests for pattern parsing.
  • processor/process.rs: route RequestType::KillAllSessions.
  • processor/constant.rs: register killAllSessions in SUPPORTED_COMMANDS.
  • Integration tests: killAllSessions removed from unsupported list; new tests for empty array, user-pattern, and non-admin-db rejection.

Testing

  • cargo fmt --all --check clean
  • cargo clippy --all-targets -- -D warnings clean for both touched crates
  • cargo test -p documentdb_gateway_core --lib: 241 passed, including 3 new parser tests
  • Gateway integration tests (sessions_tests) run in CI

Implements the killAllSessions command in the DocumentDB gateway,
following the same gateway-level session cleanup model already used
for killSessions / endSessions.

Supported command shapes:
- killAllSessions: []
  Invalidates all gateway-tracked cursors and aborts all active
  gateway transactions.
- killAllSessions: [{ user: "<user>", db: "admin" }, ...]
  Restricts cleanup to sessions owned by the listed users. Patterns
  scoped to a db other than admin are ignored since gateway
  principals authenticate against admin.

The command is rejected with Unauthorized when run against any
database other than admin (matching killOp), is marked admin_only
in listCommands, and returns ok: 1.

Closes documentdb#526

Signed-off-by: richardsimmonds <richardsimmonds314@gmail.com>

@patty-chow patty-chow left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: NEEDS CHANGES — one blocking correctness bug, two robustness concerns, one test gap. Overall shape is good: follows the established gateway-level cleanup model (killSessions/endSessions), the admin-db rejection mirrors killOp exactly (data_management.rs:262-269), DCO sign-off present on 62f219d, CHANGELOG updated, and the SUPPORTED_COMMANDS registration keeps alphabetical order with the count bumped 62→63 correctly.

Blocking

  1. Pattern list that filters to empty kills ALL sessions (processor/session.rs, parse_kill_all_sessions_users) — see inline comment. killAllSessions: [{ user: "carol", db: "otherdb" }] collapses to None and wipes every session in the gateway when it should kill none.

Should fix

  1. Transaction sweep is not best-effortremove_transactions_by_keys propagates the first abort error, halting the loop after the cursor store has already been irreversibly drained, and skipping the PG-side execute_kill_cursors call. Inline comments in transaction_store.rs and session.rs.
  2. No unit tests for the two new CursorStore methods — every other invalidate variant in that file has coverage; invalidate_all_cursors and invalidate_cursors_by_owner_name should too. Inline comment.

Questions for Patty

  • Privilege model: the only gate is "sent to the admin db" — any authenticated principal can wipe every user's cursors and abort every active transaction gateway-wide. Upstream MongoDB gates this behind the killAnySession privilege. It matches the existing killOp precedent here, so not blocking, but it's a deliberate security-posture decision worth confirming before merge.
  • Integration test flake risk: validate_kill_all_sessions_empty nukes all gateway state while sibling tests run concurrently against the shared gateway. Inline comment.

CI: no checks have run yet on 62f219d (status pending) — re-check once the blocking fix lands.

}
}

Ok((!users.is_empty()).then_some(users))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this conflates two distinct cases. killAllSessions: []users empty → None → kill everything (correct). But killAllSessions: [{ user: "carol", db: "otherdb" }] → every pattern filtered out → users empty → also None → kills every session in the gateway, when it should kill none. A pattern list that matches nothing must be a no-op, not an unscoped kill-all.

The all-vs-scoped decision needs to key off the raw input array, not the filtered output:

let is_kill_all = users_field.into_iter().next().is_none();
// ...
if is_kill_all { Ok(None) } else { Ok(Some(users)) }  // Some(vec![]) kills nothing

kill_sessions_matching_users already handles Some(&[]) safely (the per-user loop just does nothing). Please also add a unit test for the filtered-to-empty case — parse_kill_all_sessions_users_returns_admin_db_users never exercises it because alice/bob keep the vec non-empty.

let mut removed_lsids = Vec::new();

for key in keys {
if let Some((lsid, _)) = self.remove_transaction_by_key(key).await? {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ? here means the first failing abort stops the loop: remaining transactions stay alive, the command returns an error, and the caller has already irreversibly drained the cursor store. For a kill-all sweep, best-effort semantics would be more robust — log and continue, mirroring the cursor-kill failure handling in kill_sessions_matching_users:

for key in keys {
    match self.remove_transaction_by_key(key).await {
        Ok(Some((lsid, _))) => removed_lsids.push(lsid),
        Ok(None) => {}
        Err(e) => tracing::warn!("Failed to abort transaction during killAllSessions: {}", e),
    }
}

I know terminate_sessions has the same let _ = ...? shape (its comment says "best effort" but the ? makes it not so), so this is inherited — but kill-all amplifies the blast radius from one session to all of them.

} else {
let cursor_ids = cursor_store.invalidate_all_cursors();

let _ = transaction_store.remove_all_transactions().await?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering note: by this point invalidate_all_cursors() has already drained the gateway cursor store. If remove_all_transactions() errors here, the command fails and the execute_kill_cursors call below never runs — so the PG-side cursors leak until timeout while the gateway has already forgotten them, and the client gets an error for an operation that half-happened. Making the transaction sweep best-effort (see comment in transaction_store.rs) resolves this too.

}

#[must_use]
pub fn invalidate_all_cursors(&self) -> Vec<i64> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test module in this file covers every other invalidate variant (by_collection, by_database, by_session, kill_cursors ownership). The two new methods should get the same coverage — particularly invalidate_cursors_by_owner_name with a mixed-owner store, asserting only the target owner's cursor ids come back and other owners' cursors survive. The retain-push-false drain pattern itself is fine and consistent with the siblings.

}

#[tokio::test]
async fn validate_kill_all_sessions_empty() -> Result<(), Error> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flake risk: tests in this binary run on parallel threads against the shared gateway, and this one invalidates every cursor and aborts every active transaction — including those belonging to validate_kill_sessions_terminate or any other concurrently running session test. (validate_kill_all_sessions_with_users has the same issue: it kills everything owned by TEST_USERNAME, i.e. all test traffic.) If the harness doesn't serialize these, consider running them serially or scoping assertions so a mid-flight kill elsewhere can't break them.

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