Skip to content

Commit 477522c

Browse files
pmaxhoganclaude
andcommitted
fix(drive): classify quota/auth/rate in query_offset (M4 recheck-2)
R2-P1-2: query_offset() collapsed every non-2xx/non-308 probe response into ResumableSessionInvalid, so a quota/auth/rate error during the "308 without Range" recovery or the post-transient offset re-query (push_chunk_resilient) was misclassified instead of surfacing drive.quota_exhausted / drive.daily_quota_exhausted / auth.invalid_grant / drive.rate_limited. query_offset() now uses the SAME status classification as push_chunk via the shared chunk_status_outcome + DriveError::from_response: 400/404/410 stay session-dead (ResumableSessionInvalid); 401/403/429 (and 5xx / any other status) read the body and return the typed classified error so the breaker sees the stable code. Added two unit tests proving the two wire paths share classification and that the typed branch maps to the SPEC s24 codes. Also corrected the now-honest comment on DriveError::ChecksumMismatch in the executor: the DESIGN s498-500 "3 consecutive mismatches -> status=corrupt" per-file counter is NOT present on this path; it is deferred to M5 (real store executor wiring). Today a checksum mismatch maps to drive.checksum_mismatch and fails the op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CyiRqk2DVwmJjEu5gcD1m
1 parent 27313d4 commit 477522c

2 files changed

Lines changed: 105 additions & 5 deletions

File tree

crates/driven-core/src/executor.rs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3387,9 +3387,19 @@ enum DriveError {
33873387
/// The store's post-upload md5 verify failed (codex R-P1-1): the real
33883388
/// `GoogleDriveStore` verifies INSIDE the store, so a checksum mismatch
33893389
/// arrives here as a typed error rather than the executor doing its own
3390-
/// verify. It maps to the `drive.checksum_mismatch` code (NOT the generic
3391-
/// `drive.unreachable`), so the orchestrator's 3-consecutive-mismatch ->
3392-
/// `status='corrupt'` defence is driven by the real store too.
3390+
/// verify. Today (M4) it maps to the `drive.checksum_mismatch` code (NOT the
3391+
/// generic `drive.unreachable`) and fails the op.
3392+
///
3393+
/// NOTE (codex R2-P1-3, honesty): the DESIGN s498-500
3394+
/// "3 consecutive checksum mismatches -> `status='corrupt'`" defence is NOT
3395+
/// present on this path - there is no per-file mismatch counter and no
3396+
/// transition to `FileStateStatus::Corrupt` here. A mismatch maps to
3397+
/// `UploadError::Failed`, deletes the pending op, and the orchestrator only
3398+
/// defers scan timestamps + logs activity. Implementing the persistent
3399+
/// per-file counter is DEFERRED to M5 (when the real `GoogleDriveStore` is
3400+
/// wired into the prod executor; in M4 the executor runs the fake and the CLI
3401+
/// bypasses the pending-op machinery). Tracked in design/CODEX_NOTES.md
3402+
/// "M4 recheck-2 deferrals -> M5".
33933403
ChecksumMismatch,
33943404
Other,
33953405
}

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

Lines changed: 92 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,18 @@ fn chunk_status_outcome(status: u16) -> ChunkStatusOutcome {
283283
/// `Content-Range: bytes */<total>` probe) so a resumed upload knows where to
284284
/// continue (SPEC s3, DESIGN s5.4 resume). Returns the count of bytes Drive
285285
/// holds (the next offset to send from). A `200/201` means the upload already
286-
/// completed (returns `total`); a 4xx surfaces as a session-invalid error.
286+
/// completed (returns `total`).
287+
///
288+
/// Non-2xx / non-308 statuses use the SAME classification as [`push_chunk`]
289+
/// (codex R2-P1-2): this probe is reachable on the "308 without Range" recovery
290+
/// path AND the post-transient offset re-query in `push_chunk_resilient`, so a
291+
/// quota/auth/rate error DURING the probe must surface its stable code, not be
292+
/// collapsed into `ResumableSessionInvalid`. Only a genuinely session-dead 4xx
293+
/// (400/404/410) maps to [`DriveError::ResumableSessionInvalid`]; 401/403/429
294+
/// (and 5xx / any other status) read the body and return the TYPED classified
295+
/// error via [`DriveError::from_response`] so `drive.quota_exhausted`,
296+
/// `drive.daily_quota_exhausted`, `auth.invalid_grant`, and
297+
/// `drive.rate_limited` reach the breaker instead of looping a session restart.
287298
pub async fn query_offset(
288299
http: &reqwest::Client,
289300
access_token: &str,
@@ -308,7 +319,23 @@ pub async fn query_offset(
308319
// Already complete.
309320
return Ok(session.size);
310321
}
311-
Err(anyhow::Error::new(DriveError::ResumableSessionInvalid))
322+
// R2-P1-2: classify exactly like push_chunk - reserve ResumableSessionInvalid
323+
// for session-dead 4xx (400/404/410); read + classify everything else
324+
// (401/403/429/5xx/other) into the typed error.
325+
match chunk_status_outcome(status) {
326+
ChunkStatusOutcome::SessionInvalid => {
327+
Err(anyhow::Error::new(DriveError::ResumableSessionInvalid))
328+
}
329+
ChunkStatusOutcome::Typed => {
330+
let retry_after = super::parse_retry_after(&resp);
331+
let body = resp.bytes().await.map_err(DriveError::from_transport)?;
332+
Err(anyhow::Error::new(DriveError::from_response(
333+
status,
334+
&body,
335+
retry_after,
336+
)))
337+
}
338+
}
312339
}
313340

314341
/// Parses Drive's resumable completion response body into a [`RemoteEntry`]
@@ -438,6 +465,69 @@ mod tests {
438465
}
439466
}
440467

468+
#[test]
469+
fn query_offset_and_push_chunk_share_status_classification() {
470+
// R2-P1-2: query_offset() (used on the "308 without Range" recovery and
471+
// the post-transient offset re-query in push_chunk_resilient) must use
472+
// the SAME session-dead-vs-typed split as push_chunk. Both call
473+
// chunk_status_outcome, so proving that one function classifies every
474+
// relevant status correctly proves the two wire paths are consistent
475+
// without standing up an HTTP server for each.
476+
//
477+
// Session-dead 4xx -> ResumableSessionInvalid on BOTH paths.
478+
for s in [400u16, 404, 410] {
479+
assert_eq!(
480+
chunk_status_outcome(s),
481+
ChunkStatusOutcome::SessionInvalid,
482+
"status {s} must be session-dead in query_offset + push_chunk"
483+
);
484+
}
485+
// 401/403/429 + 5xx + any other -> read + classify (typed) on BOTH
486+
// paths, so a quota/auth/rate error during the offset probe surfaces its
487+
// stable code instead of collapsing to ResumableSessionInvalid.
488+
for s in [401u16, 403, 429, 500, 502, 503, 418] {
489+
assert_eq!(
490+
chunk_status_outcome(s),
491+
ChunkStatusOutcome::Typed,
492+
"status {s} must be typed in query_offset + push_chunk"
493+
);
494+
}
495+
}
496+
497+
#[test]
498+
fn query_offset_typed_branch_maps_to_stable_codes() {
499+
use crate::remote_store::DriveErrorClassification;
500+
// R2-P1-2: the typed branch query_offset now takes for 401/403/429 hands
501+
// the body to DriveError::from_response, the SAME call push_chunk's typed
502+
// branch makes. Confirm the auth/quota/rate bodies map to the SPEC s24
503+
// classes the breaker needs (NOT ResumableSessionInvalid / Other).
504+
let invalid_grant = br#"{"error":"invalid_grant"}"#;
505+
assert!(matches!(
506+
DriveError::from_response(401, invalid_grant, None).classification(),
507+
DriveErrorClassification::AuthInvalidGrant
508+
));
509+
let storage = br#"{"error":{"errors":[{"reason":"storageQuotaExceeded"}],"code":403}}"#;
510+
assert!(matches!(
511+
DriveError::from_response(403, storage, None).classification(),
512+
DriveErrorClassification::StorageQuota
513+
));
514+
let daily = br#"{"error":{"errors":[{"reason":"dailyLimitExceeded"}],"code":403}}"#;
515+
assert!(matches!(
516+
DriveError::from_response(403, daily, None).classification(),
517+
DriveErrorClassification::DailyQuota
518+
));
519+
assert!(matches!(
520+
DriveError::from_response(429, b"", None).classification(),
521+
DriveErrorClassification::RateLimited { .. }
522+
));
523+
// And the session-dead status query_offset reserves for
524+
// ResumableSessionInvalid is NOT one of the typed classes.
525+
assert_eq!(
526+
chunk_status_outcome(410),
527+
ChunkStatusOutcome::SessionInvalid
528+
);
529+
}
530+
441531
#[test]
442532
fn typed_statuses_classify_to_their_stable_codes() {
443533
use crate::remote_store::DriveErrorClassification;

0 commit comments

Comments
 (0)