Skip to content

Commit 3af8fc8

Browse files
pmaxhoganclaude
andcommitted
fix(drive): tombstone dead resumable sessions (real-Drive contract fidelity)
Running the M4 real-Drive contract suite against live Google (now that the e2e creds exist) surfaced a fake-vs-real gap: scenario_resumable_non_multiple_rejected failed because GoogleDriveStore rejected a non-256-KiB non-final chunk CLIENT-SIDE (returning SessionInvalid without ever reaching Drive), so Drive never learned the session was dead and ACCEPTED a subsequent valid 256-KiB chunk against the same session URL -> InProgress instead of the contract-required SessionInvalid. Fix: GoogleDriveStore now tombstones a session URL on any SessionInvalid (client-side non-multiple rejection OR a Drive 4xx from push_chunk) in a bounded parking_lot::Mutex<HashSet<String>>, and short-circuits any further chunk on a tombstoned session to SessionInvalid - matching the InMemoryRemoteStore + DESIGN s5.4 ("never issue a fresh resume_chunk against the old session URL after a 4xx"). The set is capped (cleared at 1024) as a memory bound; the executor discards a session on SessionInvalid and never reuses its URL, so a tombstoned URL is never legitimately re-sent. Not production-reachable (the executor sends only 256-KiB multiples and restarts on SessionInvalid), but it closes the M4 acceptance "contract suite green against real Google" - now 7/7 against live Drive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CyiRqk2DVwmJjEu5gcD1m
1 parent 9693a91 commit 3af8fc8

1 file changed

Lines changed: 42 additions & 2 deletions

File tree

  • crates/driven-drive/src/google

crates/driven-drive/src/google/mod.rs

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ pub mod resumable;
4242
pub mod retry;
4343
pub mod token_store;
4444

45-
use std::collections::HashMap;
45+
use std::collections::{HashMap, HashSet};
4646
use std::pin::Pin;
4747
use std::task::{Context, Poll};
4848
use std::time::Duration;
@@ -485,6 +485,14 @@ pub struct GoogleDriveStore {
485485
/// between-bytes timeout (DESIGN s5.8.4 `*`).
486486
http_stream: reqwest::Client,
487487
tokens: RefreshingTokenSource,
488+
/// URLs of resumable sessions that have returned [`ResumeProgress::SessionInvalid`]
489+
/// and are therefore dead. A client-side non-multiple-chunk rejection never
490+
/// reaches Drive, so Drive would still accept a later valid chunk against the
491+
/// same session URL; tombstoning the URL here makes EVERY further chunk return
492+
/// SessionInvalid, matching the fake + DESIGN s5.4 ("never issue a fresh
493+
/// resume_chunk against the old session URL after a 4xx"). Bounded in
494+
/// [`GoogleDriveStore::mark_session_dead`].
495+
dead_sessions: parking_lot::Mutex<HashSet<String>>,
488496
}
489497

490498
impl GoogleDriveStore {
@@ -512,6 +520,7 @@ impl GoogleDriveStore {
512520
http,
513521
http_stream,
514522
tokens,
523+
dead_sessions: parking_lot::Mutex::new(HashSet::new()),
515524
}
516525
}
517526

@@ -526,9 +535,27 @@ impl GoogleDriveStore {
526535
http,
527536
http_stream,
528537
tokens,
538+
dead_sessions: parking_lot::Mutex::new(HashSet::new()),
529539
})
530540
}
531541

542+
/// Whether `url` belongs to a session already tombstoned as dead.
543+
fn is_session_dead(&self, url: &str) -> bool {
544+
self.dead_sessions.lock().contains(url)
545+
}
546+
547+
/// Tombstones `url` so every further chunk on it returns SessionInvalid.
548+
/// Bounded: the executor discards a session on SessionInvalid and never
549+
/// re-sends its URL, so a tombstoned URL is never legitimately reused; the
550+
/// cap is a pure memory safety-bound for a long-running process.
551+
fn mark_session_dead(&self, url: &str) {
552+
let mut dead = self.dead_sessions.lock();
553+
if dead.len() >= 1024 {
554+
dead.clear();
555+
}
556+
dead.insert(url.to_string());
557+
}
558+
532559
/// Mints a fresh bearer token for an authorized request (SPEC s4.1).
533560
pub(crate) async fn bearer(&self) -> anyhow::Result<String> {
534561
self.tokens.access_token().await
@@ -1350,12 +1377,25 @@ impl RemoteStore for GoogleDriveStore {
13501377
// 256 KiB. Enforce at the trait layer so the contract's
13511378
// `scenario_resumable_non_multiple_rejected` returns SessionInvalid
13521379
// exactly as the fake does (matching the wire-level 400 Drive returns).
1380+
// A session already tombstoned (a prior SessionInvalid) stays dead:
1381+
// Drive never saw our client-side rejection, so it would otherwise accept
1382+
// a later valid chunk against the same URL (DESIGN s5.4 forbids reusing a
1383+
// dead session). Fail fast, matching the fake.
1384+
if self.is_session_dead(&session.url) {
1385+
return Ok(ResumeProgress::SessionInvalid);
1386+
}
13531387
let is_final = offset + chunk.len() as u64 == session.size;
13541388
if !is_final && (chunk.len() as u64) % resumable::CHUNK_MULTIPLE != 0 {
1389+
self.mark_session_dead(&session.url);
13551390
return Ok(ResumeProgress::SessionInvalid);
13561391
}
13571392
let token = self.bearer().await?;
1358-
resumable::push_chunk(self.http_stream(), &token, session, offset, chunk).await
1393+
let progress =
1394+
resumable::push_chunk(self.http_stream(), &token, session, offset, chunk).await?;
1395+
if matches!(progress, ResumeProgress::SessionInvalid) {
1396+
self.mark_session_dead(&session.url);
1397+
}
1398+
Ok(progress)
13591399
}
13601400

13611401
async fn trash(&self, file_id: &str) -> anyhow::Result<()> {

0 commit comments

Comments
 (0)