Skip to content

fix(s3): honest per-source versioning across backends and multipart-upload leak sweep - #316

Merged
pmaxhogan merged 4 commits into
mainfrom
wave-1-s3-correctness
Aug 17, 2026
Merged

fix(s3): honest per-source versioning across backends and multipart-upload leak sweep#316
pmaxhogan merged 4 commits into
mainfrom
wave-1-s3-correctness

Conversation

@pmaxhogan

@pmaxhogan pmaxhogan commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Closes #220 (part 2). Closes #222.

#220: per-source versioning now really keeps versions on S3 and local folders

Versioning was honestly gated OFF on those destinations (part 1, v2.5.0) because their object id is derived from the file name: the executor versions a change by forcing the CREATE path, which there lands on the SAME object, so the retained file_versions row ended up pointing at the bytes that had just been overwritten. This is part 2 - making it work rather than making it unavailable.

The seam is one trait method, defaulting to today's behaviour:

async fn archive_version(&self, file_id: &str, content_token: &str)
    -> anyhow::Result<Option<String>>;   // default: Ok(None)

None = "no archival needed here", which is Google Drive (its create already mints a new object and the old one survives in the trash). Some(id) is where the superseded bytes now live, and the version row points there.

  • driven-s3 copies the object server-side into <root>.driven-versions/ - a plain CopyObject, or ranged UploadPartCopy above S3's 5 GiB single-copy ceiling. No bytes pass through the process.
  • driven-localfs copies data + sidecar into .driven-versions/, mirroring the live tree so a user browsing the drive can see which file an old copy belongs to.
  • SFTP stays gated off, honestly. It has the same path-keyed model but no server-side copy, so archiving a version would mean re-uploading the whole file over the link that is already the bottleneck. Shipping that silently would double the cost of every changed file.

Three properties are load-bearing, and each has a test:

  1. Deterministic archive id, a pure function of (object, content token) - the token being the OLD plaintext BLAKE3 the executor already had.
  2. Never re-copy over an existing archive. An op can die between the archive and its commit and be replayed, by which time the live object may already hold the NEW bytes; a blind re-copy would overwrite a correct archive with the content it exists to preserve. Together with (1) this also stops a replay stacking copies.
  3. trash is a no-op for an archived id. After a versioned change the executor "trashes" the object the version row names, meaning "out of the live tree, still restorable" - which is what Drive's trash does and what the archive already did. On these stores trash is a PERMANENT delete, so without this it would destroy the version just recorded. delete_permanent (the count-cap prune) still frees it.

The version store is invisible to the folder picker and excluded from list_source_object_ids, so neither the remote-existence audit nor the integrity scrub sees an archived version as an orphan to heal forever. LocalFsStore::about() still counts its bytes as Driven's footprint - they are real storage a versioning setting is spending.

When the copy fails, the upload proceeds as a plain in-place update and no version is recorded: a certain loss (this backup does not happen) must not be traded for a hypothetical one. But it is not silent - a new drive.version_archive_failed Warn row lands in the activity log, because a destination quietly not delivering the point-in-time restore its settings page offers is the #220 bug one level down.

Executor delta (additive; coordinated with the team lead since PR1/PR2 also touch this file): one field on VersionSupersede, one optional JSON field on PendingOpPayload (supersedes_archive_file_id - unmigrated; an older row without it decodes as None, pinned by a test), one call site after resolve_version_supersede, and the two NewFileVersion construction sites reading the archive id. supersedes_drive_file_id deliberately keeps naming the ORIGINAL object, because that is what the pre-flip file_state row points at and what every recovery guard compares against.

#222: abandoned multipart uploads no longer leak

upload_stage_resumable opened a fresh CreateMultipartUpload on every attempt, so a retry abandoned the previous upload id with nobody holding it. Its parts stayed on the bucket - billed, invisible to ListObjectsV2 - one per failed attempt, forever.

New RemoteStore::abandon_resumable_session is a real AbortMultipartUpload on S3 (a no-op default elsewhere), called wherever a session becomes permanently unreachable:

  • a fresh session replacing a persisted one,
  • an invalidated session discarded before a restart,
  • a streaming upload whose pending op is about to be DELETED,
  • every path by which reconcile's resume_persisted declines to resume (a changed file, an expired session, an encrypted source, a misbehaving server) - funnelled through one wrapper so a new decline condition cannot forget.

It deliberately does NOT fire on the Fatal mid-stream path, which keeps the op precisely so the session can be RESUMED; aborting there would turn a recoverable upload into a full re-send. crash_mid_upload_resumes_persisted_session_byte_for_byte pins that, and caught an earlier, broader version of this change.

S3Store::sweep_abandoned_multipart_uploads adds the backstop: ListMultipartUploads + abort, spawned detached when the store is built (account assembly must not wait on a network round trip to a possibly-unreachable endpoint). Three guards, unit-tested as a pure function because an over-eager abort destroys a live transfer: prefix (a shared bucket keeps its own uploads), ownership (never an upload id this process opened), and age (7 days, longer than the 6-day persisted-session window, so it can never kill a session anything would still resume; an upload the server did not date is skipped rather than guessed at).

The chaos harness keeps a BOUNDED stranded-upload assertion rather than zero, with the old "unfixed finding" note replaced by an accurate one. The residual case is structural, not a missing hook: reconcile is a per-process STARTUP pass, while a scan later in the same run re-plans the failed path under a new op - so an upload kept on purpose for a crash-resume can be overtaken mid-run and sits until the next start, where reconcile now either resumes it (no waste) or aborts it. Bounded by restarts, not unbounded in time, with the sweep behind it.

Docs

  • README footnote 42 rewritten (the old text claimed versioning is Drive-only), plus a new S3 housekeeping paragraph covering the .driven-versions prefix and a lifecycle-rule recommendation for incomplete multipart uploads, and the local-folder "no trash" bullet corrected to distinguish deleted from changed files.
  • design/DESIGN.md §5.5.1's "Destination restriction" section replaced with the capability + archive_version design.
  • design/ROADMAP.md backend item updated.
  • UI: versionRetention.s3 / .local_folder copy rewritten, errors.drive.version_archive_failed added, fixtures flipped to match the real descriptors, gating tests re-pointed at SFTP, plus new tests asserting the POSITIVE direction (an S3 source DOES get the versioning editor and the point-in-time restore picker).

Verification (all run, real results)

Suite Result
cargo test --workspace (excl. e2e/bench) all green, incl. 560 driven-core and 442 driven-app
cargo clippy --workspace --all-targets -- -D warnings clean
cargo fmt --all --check clean
driven-s3 e2e vs local MinIO pass
driven-s3 e2e vs live Cloudflare R2 pass
driven-localfs e2e pass
chaos scenario run-all 74 PASS / 31 SKIP / 0 FAIL
pnpm -C ui run test:unit 791 passed (59 files)
pnpm -C ui run lint 0 errors
just e2e (containerized app-level) 9/9 PASS

The S3 e2e suite gained a section that drives the whole #220 round trip against a REAL server - archive, overwrite, prove the archive still holds the OLD bytes, prove it is invisible to the picker and the audit, prove trash does not destroy it and delete_permanent does - plus a #222 section for abandon + sweep. Both MinIO and R2 pass it, which is what validates the hand-built CopyObject / UploadPartCopy / ListMultipartUploads requests (rusty-s3 models none of the three).

just e2e passes 9/9 (wizard-first-run, local-folder-round-trip, settings-persistence, fake-drive-outage-surfaced, source-file-unreadable, dest-disk-full, s3-round-trip, s3-network-cut-mid-sync, sftp-round-trip).

One gotcha worth knowing for anyone else running it locally: it first failed to COMPILE on a reason unrelated to this branch. The Dockerfile's --mount=type=cache,target=/build/target cache mount was serving a stale driven-remote rlib that cargo treated as fresh, so driven-s3 and driven-core rebuilt from this branch while driven-remote did not, and the build died with "method archive_version is not a member of trait RemoteStore" while cargo check --workspace was clean on the host. The Docker CONTEXT was correct (the new symbols are present in a throwaway image built from it) and touching source mtimes, including into the future, did not bust it. docker builder prune --filter type=exec.cachemount -f then a cold build fixed it. CI starts cold and is unaffected, but a local run after a branch that changes a low-level crate's public API can hit this and it looks exactly like a broken branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Coverage

Area main this PR delta
Rust (lib crates) 84.51% 84.44% -0.07 (OK)
UI (vue/ts) 93.37% 93.37% +0.00 (OK)

Gate: passed - no coverage regression (epsilon 0.1 pp).

pmaxhogan and others added 4 commits August 17, 2026 15:30
…pload leak sweep

Closes #220 (part 2) and #222.

their object id is derived from the file name, so the versioned "create"
overwrote the very bytes the retained version row pointed at. It now works
there for real.

New `RemoteStore::archive_version(file_id, content_token)` copies the
superseded object aside BEFORE the overwrite and returns the id the copy
lives at; the version row points there. Google Drive keeps the default
`Ok(None)` (its create already mints a new object). driven-s3 uses a
server-side `CopyObject`, or ranged `UploadPartCopy` above the 5 GiB
single-copy ceiling, into `<root>.driven-versions/`; driven-localfs copies
data + sidecar into `.driven-versions/`. The archive id is a pure function
of (object, content) and an existing archive is never re-copied, so a
crashed-and-replayed op can neither stack copies nor overwrite a correct
archive with the newer content. `trash` is a no-op for an archived id on
both stores (there it is a permanent delete, and the archive is already
parked); `delete_permanent` still frees it for the count-cap prune. The
version store is hidden from the folder picker and excluded from the
remote-existence audit's live set.

When the copy fails the upload proceeds as a plain in-place update and no
version is recorded - a certain loss must not be traded for a hypothetical
one - but it is NOT silent: a new `drive.version_archive_failed` Warn row
lands in the activity log. SFTP stays honestly gated off: it has no
server-side copy, so archiving would re-upload the whole file over the link
that is already the bottleneck.

attempt and abandoned the previous upload id, whose parts stayed on the
bucket, billed, invisible to `ListObjectsV2`, forever. New
`RemoteStore::abandon_resumable_session` is a real `AbortMultipartUpload` on
S3, called wherever a session becomes permanently unreachable: a fresh
session replacing a persisted one, a discarded invalid session, a streaming
upload whose op is about to be deleted, and every path by which reconcile's
`resume_persisted` declines to resume (funnelled through one wrapper so a
new decline condition cannot forget). It deliberately does NOT fire on the
`Fatal` mid-stream path, which keeps the op precisely so the session can be
resumed. `S3Store::sweep_abandoned_multipart_uploads` adds a
`ListMultipartUploads` + abort sweep, spawned detached when the store is
built, guarded by prefix + this-process ownership + a 7-day age floor (longer
than the 6-day persisted-session window, so it can never kill a resumable
session).

Verified against real servers: the driven-s3 e2e suite passes against local
MinIO and against live Cloudflare R2, exercising the archive round trip, the
version store's invisibility to the picker and the audit, the no-op trash,
the hard-delete prune, and abandon + sweep. Chaos rows keep a bounded
stranded-upload assertion with a rewritten explanation of the one structural
case that survives a single run (an upload kept on purpose for a crash-resume
that a later scan overtakes; reconcile is a startup pass).

README, DESIGN and ROADMAP updated, including an S3 lifecycle-rule
recommendation for incomplete multipart uploads.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v
…eams

The S3 archive-key doc understated the fixed overhead (the content token is a
full hex BLAKE3, so it is ~80 bytes, not ~40), and `abandon_persisted_session`
still described two call sites after the streaming-upload one was added.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v
Neither copy path can publish a partial object (a single CopyObject is
atomic; a multipart copy materializes only at CompleteMultipartUpload and
aborts on any earlier failure), so an archive that exists is a whole one.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v
…nd split

The SourceTable and Restore doc comments still named S3 and the local folder
as the destinations that cannot keep versions, which is now false: both keep
real versions in a `.driven-versions` area, and SFTP is the only exception.
Comment-only; the components already read the capability from the descriptors
rather than a hardcoded list.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v
@pmaxhogan
pmaxhogan force-pushed the wave-1-s3-correctness branch from 4a5883f to b20dac6 Compare August 17, 2026 20:35
@pmaxhogan
pmaxhogan enabled auto-merge (squash) August 17, 2026 20:35
@pmaxhogan
pmaxhogan merged commit d462592 into main Aug 17, 2026
18 checks passed
@pmaxhogan
pmaxhogan deleted the wave-1-s3-correctness branch August 17, 2026 20:58
@github-project-automation github-project-automation Bot moved this from Todo to Done in Driven Aug 17, 2026
pmaxhogan added a commit that referenced this pull request Aug 18, 2026
…quit drain

Rebases wave-1-debug-diag onto origin/main now that #310-#316 merged
(#312's quit-path restructuring in particular). Structural follow-up:

- The debug-logging-mode watchdog (debug_mode.rs) previously used the
  detached memlog.rs pattern (no shutdown tracking). #312 replaced the
  old shutdown_orchestrators() with a proper ShutdownHandles/
  drain_shutdown_handles structure that every other periodic background
  task (updater, telemetry, iostat, and now #311's bottleneck sampler)
  registers into for a no-orphan quit drain. Re-homed the watchdog into
  that same structure: a new DebugModeRuntime (task + shutdown watch,
  no shared hub - the watchdog only reads/writes settings directly)
  on AppState, set_debug_mode_task/shutdown_debug_mode_task mirroring
  set_bottleneck_task/shutdown_bottleneck_task exactly, a debug_mode
  field on ShutdownHandles, and spawn_watchdog now runs the same
  select!-on-shutdown-or-tick loop bottleneck_hub/iostat_hub use
  instead of a bare loop.
- Added app_state::tests::debug_mode_runtime_task_and_shutdown_round_trip,
  mirroring bottleneck's round-trip test.
- privacy.png (light+dark) and 9 shell.spec.ts baselines (light+dark)
  regenerated via `just visual-update` (Docker) - the shell baselines
  drifted independently of this PR's own diff (same delta across every
  scenario in both themes), consistent with normal headless-Chromium
  rendering drift between visual-update runs; all 106 visual specs pass
  against the regenerated set.

No other conflicts: README.md, dtos.rs, settings.rs's redaction code,
en-US.json, Activity.vue, and fixtures.ts all auto-merged cleanly with
#311's bottleneck-tile additions coexisting alongside this PR's debug
logging toggle and diagnostic-bundle changes.

Verified after rebase: cargo test -p driven-app --lib (494 passed),
cargo clippy --workspace --all-targets -- -D warnings (clean),
cargo fmt --all --check (clean), pnpm lint (0 errors), pnpm format:check
(clean), pnpm test:unit (861 passed, 64 files), pnpm build / vue-tsc
(clean).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v
pmaxhogan added a commit that referenced this pull request Aug 18, 2026
…dles (#314)

## Summary

Closes #309, closes #204. Part of the v2.12.0 wave (PR5).

**#204 (diagnostic bundle PII/secret leaks)** - `redact_settings()` used
to clone `GlobalSettings` verbatim and patch only `proxy_url`. It now
builds a field-for-field `RedactedGlobalSettings` struct, so a future
secret-bearing field added to `GlobalSettings` fails to *compile* here
until someone decides how to redact it, instead of leaking silently.
Fixes the three concrete leaks the issue named:
- `pre_backup_hook` / `post_backup_hook` (command lines - a classic home
for embedded secrets) are now redacted wholesale (`<hook-redacted: N
chars>`), not shipped raw.
- `custom_root_ca_path` is now hashed through the same `<path:hash>`
scheme the rest of the bundle already uses.
- `proxy_url` in PAC mode (a local file path, not a URL) is now hashed
instead of only having userinfo-stripping applied (which never matched a
bare path).
- The issue's "also worth fixing" item (`ProxyError`'s `Display`
embedding raw userinfo) was already fixed by #208 - verified via `git
blame`, not touched again.

**#309 (debug logging mode)** - a new Settings > Privacy & Data toggle
("Debug logging") with an always-visible amber warning panel (shown
before the toggle is ever switched on, per the approved mockup), backed
by:
- A real runtime-reloadable tracing filter (`logging.rs`,
`tracing_subscriber::reload`) - flipping the toggle now actually changes
the live process's verbosity, no restart needed. This also closes a
long-documented gap where `global.log_level` only ever exported
`RUST_LOG` for the *next* launch; it now reloads the live filter too
(deferred while debug mode is active, so it doesn't undo the debug-mode
filter).
- A persisted epoch-ms expiry + a boot-time reconcile and periodic
watchdog (`debug_mode.rs`) that auto-turns the toggle off 24h after
enabling - honoured across a restart, not just while the app keeps
running. The watchdog is registered on `AppState` and joined by #312's
no-orphan quit drain (`ShutdownHandles`/`drain_shutdown_handles`), the
same pattern #311's bottleneck sampler uses.
- A rolling log cap that widens from 25 MB to 250 MB while debug mode is
on.
- The diagnostic bundle gains `DEBUG_MODE.txt` and an unredacted
`debug/engine_state.txt` while debug mode is on - the one deliberate
exception to the #204 redaction rules, gated on the user's explicit
opt-in (every other bundle file stays redacted regardless).
- Every bundle now also ships `manifest.txt` (entry name + size), a
small bundle-usefulness improvement.
- Activity's "Export diagnostic bundle" button shows an amber "Debug
data included" chip while debug mode is on.

## Also in this PR

- **Rebased onto `main`** after #310-#316 merged. Re-homed the
debug-mode watchdog from a detached `memlog.rs`-style task into #312's
`ShutdownHandles`/`drain_shutdown_handles` no-orphan quit drain (new
`DebugModeRuntime` on `AppState`,
`set_debug_mode_task`/`shutdown_debug_mode_task` mirroring
`set_bottleneck_task`/`shutdown_bottleneck_task`).
- **CodeQL `rust/path-injection` fix** (not a dismissal): two test
helpers (`settings.rs`'s pre-existing `seeded_repo()` and this PR's new
`debug_mode.rs` one) hand-rolled a temp dir via
`std::env::temp_dir().join(format!(...))` before feeding it to
`SqliteStateRepo::open` - exactly the pattern this repo's CodeQL rule
flags (see the `tempfile` dependency comment in `src-tauri/Cargo.toml`,
and PR 151 precedent). Switched both to `tempfile::tempdir().keep()`, an
opaque external call CodeQL's dataflow can't see into, so the taint
chain never forms.
- **Also carries the h2 advisory fix** (RUSTSEC-2026-0258, low severity,
unbounded empty DATA frames) - `cargo update -p h2` (0.4.15 -> 0.4.16),
lockfile-only, no `Cargo.toml` changes. This advisory is unrelated to
this PR's own diff (`git diff` against the pre-PR base shows zero
`Cargo.lock` changes before this commit) and would fail `cargo deny`
repo-wide on `main` too; landing it here unblocks this PR's `cargo deny`
check and delivers the fix to `main` in the same step.

## Test plan

- [x] `cargo test -p driven-app --lib` - 494 passed (18 #204 redaction
tests with leak-shaped fixtures, incl. one asserting the full serialized
bundle JSON end-to-end; 5 debug-mode watchdog/expiry tests; 6
settings-persistence round-trip tests; 1 `AppState` debug-mode
task/shutdown round-trip test)
- [x] `cargo clippy --workspace --all-targets -- -D warnings` - clean
- [x] `cargo fmt --all -- --check` - clean
- [x] `cargo build --workspace --tests` - clean
- [x] `cargo deny check` - clean (advisories ok, bans ok, licenses ok,
sources ok)
- [x] `pnpm lint` / `pnpm format:check` / `pnpm test:unit` (861 passed,
64 files) / `pnpm build` (vue-tsc + vite) - all clean, run in the CI
job's exact order
- [x] Linux visual baselines regenerated via `just visual-update`
(Docker) - `privacy.png` (light+dark) plus 9 `shell.spec.ts` baselines
(light+dark) that had drifted independently of this PR; all 106 visual
specs pass
- [x] README updated (Features list + comparison-table footnote ³⁴)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_019xKUm9vH4ifb5LHR5szy1v

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
pmaxhogan added a commit that referenced this pull request Aug 18, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.12.0](v2.11.1...v2.12.0)
(2026-08-18)


### Features

* **app:** opt-in debug logging mode and safer, richer diagnostic
bundles ([#314](#314))
([33c281c](33c281c))
* **core:** allow nested backup sources when the parent excludes the
child ([#294](#294))
([0b62df9](0b62df9))
* **core:** live exclusion pickup and a visible pending-work queue
([#313](#313))
([e6427c7](e6427c7))
* live bottleneck indicator on the Activity dashboard
([#311](#311))
([2d9d763](2d9d763))
* **ui:** folder picker sort/filter/create/rename and exclusions size
rollups ([#315](#315))
([7e87341](7e87341))


### Bug Fixes

* **app:** never freeze on tray quit during a backup; quitting tray
state; honest recovery status
([#312](#312))
([f951cde](f951cde))
* clear the attention banner after a passing run and make source removal
backend-aware ([#310](#310))
([6d8e1ab](6d8e1ab))
* **s3:** honest per-source versioning across backends and
multipart-upload leak sweep
([#316](#316))
([d462592](d462592))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Abandoned S3 multipart uploads leak their parts (billed, never swept) Per-source versioning is a silent no-op on S3 and local-folder destinations

1 participant