Skip to content

Test the production Android backup archive writer - #2773

Open
kavemang wants to merge 3 commits into
ryanbr:mainfrom
kavemang:agent/android-backup-production-writer-tests-2708
Open

kavemang wants to merge 3 commits into
ryanbr:mainfrom
kavemang:agent/android-backup-production-writer-tests-2708

Conversation

@kavemang

@kavemang kavemang commented Oct 11, 2026 •

Copy link
Copy Markdown

What this PR does

BackupSettingsCodecTest exercised a duplicate ZIP writer that no production export called. Route Android exportTo through the context-free stream writer and test the same archive construction used at runtime.

The copy stays inside the existing Room write transaction and output-stream scope. The writer returns the copied database byte count used for restore-ceiling warnings, preserves database/settings/manifest entry order, and finishes the ZIP before verification. Tests cover exact UTF-8 manifest/settings payloads, a readable central directory, database-only byte accounting, legacy and manifest-only shapes, restore round-trips, and propagated output failures with cleanup.

Android-only JVM/SAF extraction: the container entries, settings whitelist, provenance payload and database schema are unchanged; no Swift storage or analytics change is needed. Required derived parity metadata accompanies the production call-graph change.

Type of change

  • Refactor / cleanup

How it was tested

  • Focused Linux JVM harness: all 12 committed BackupSettingsCodecTest methods and the changed BackupSyncFileIoTest archive round-trip pass against extracted production writer/staging/codec code, real ZIP/file I/O and JSON, with verbatim production entry-name and hosted-card constants. This does not compile Android Context/Room/SAF or exercise a live database transaction.
  • Private manifest-payload mutation is rejected by the new exact payload assertion; no mutant is committed.
  • Guarded parity refresh passed (289 known findings), core tools passed all 153 tests, and source hygiene, whitespace and diff-scoped translation checks passed. Exact out-of-scope twin registration points to the existing Apple app-layer archive writer.
  • On actual head d1c1abc9ff7644bf63dc418761359ae01f81c17d, Android CI compiled the complete app and built the debug APK (4m 35s), then passed the full testFullDebugUnitTest suite (1m 53s). No warnings name the changed production or test files. Individual JVM test counts are not emitted by this workflow.
  • On that same head, parity governance passed 160 tests; Tools CI passed 234 capture tests, 50 repository-acceptance tests, 2 R-R preservation tests and 153 core tests. All five expected checks passed, including hygiene and translation coverage.
  • No BLE behavior changed or hardware validation claimed. Room snapshot concurrency and real SAF providers were not exercised on a device.

Checklist

  • Android full unit suite and compile pass on current PR head
  • No new build warnings introduced
  • No hardcoded hex frame bytes; protocol facts live in the schema / decoders
  • Follows the conventions in docs/CONTRIBUTING.md
  • No generated output, secrets or signing files committed

Related issues

Refs #2708

@kavemang
kavemang marked this pull request as ready for review October 11, 2026 06:12
@ryanbr

ryanbr commented Oct 15, 2026

Copy link
Copy Markdown
Owner

Reviewed and verified locally. Green at d1c1abc, 5/5.

This is the one in the batch that removes an actual divergence rather than a duplicate, and it is worth more than its line count suggests. Before it, exportTo inlined its own ZIP writer producing three entries while writeBackupZip was a separate test-only helper producing two, with no manifest, under a doc comment calling it a "context-free twin of the stream the live exportTo writes". It was not a twin. Every test that believed it was round-tripping a real archive was round-tripping a shape production never writes. Collapsing both onto one writer is the right call.

I checked the obvious risk, that making manifestJson nullable quietly drops the manifest in production: BackupManifest.json at BackupProvenance.kt:51 returns a non-null String and exportTo passes it straight through, so the live archive still always carries all three entries. Entry order being asserted explicitly now matters too, because the comment at DataBackup.kt:269 calls order part of the cross-platform container contract and nothing had pinned it.

archiveWriteFailurePropagatesAndClosesOutput is a good addition. A content-provider stream that throws mid-write is exactly the Android failure the export path meets in the field, and nothing covered it.

Separate from this change, something worth knowing about one of the files it touches. android/app/src/test/java/com/noop/data/BackupSyncFileIoTest.kt carries a literal NUL byte at offset 1233, inside the doc comment that spells out the SQLite magic as SQLite format 3 followed by the raw byte rather than an escape. Git's binary heuristic keys on a NUL in the first 8000 bytes, so it classifies the file as binary: this PR's one-line change to it reports as 0 additions and 0 deletions with no rendered diff on GitHub, and so will every future change to that file. I had to pull both blobs and compare them by hand to confirm the change is only the backup to backup.outputStream() call update. Writing the magic as escape text would restore normal diffs. Worth its own issue, since it is a review-visibility hole rather than a bug in this change.

Good to merge.

@ryanbr

ryanbr commented Oct 15, 2026

Copy link
Copy Markdown
Owner

Heads up: the other five in this batch are on main now, and #2772 went in just ahead of this one. Both PRs rewrite the same two authority lines in Tools/parity_twin_map.json, so this branch no longer applies.

The conflict is confined to the ledger metadata. Everything that makes this PR worth having, the DataBackup.kt collapse onto one writer, the new archive tests and the parity_dispositions.json entry, is untouched by it. Tools/parity_ledger_baseline.json merges cleanly too, since your android/data line and #2772's Packages/WhoopProtocol line are different lines.

Only parity_twin_map.json genuinely clashes, and neither side of it is right any more:

  • main now carries files 509 / functions 4537, which accounts for Test bundled protocol schema through the production loader #2772 removing WhoopProtocolInfo but knows nothing about your writeBackupZip signature change.
  • this branch carries files 510 / functions 4538 with a new functions digest, which accounts for your change but predates the removal.

The correct values are neither, so please do not hand-resolve that hunk. Rebase onto main, take main's side of those lines, then let the tool recompute:

python3 Tools/parity_ledger.py --refresh-derived

Per docs/CONTRIBUTING.md:195 that rewrites the snapshots only if the ratchet accepts the result. If it refuses because the base itself is stale, that refusal is explicitly not yours to absorb, and --refresh-derived --repair-stale-base is the documented remedy. Please do not reach for --migrate-authority.

Nothing here reflects on the change itself. It is the strongest one in the batch and the review above stands; it is purely the cost of two PRs touching the same generated file. Ping me once it is rebased and I will take it straight in.

Regenerate parity authority from the merged declarations while retaining
the production ZIP writer and archive regression coverage.

Refs #2708
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