Repository navigation
fix(db): add 8->9 migration; stop destructive fallback that wipes configs - #68
Merged
Merged
Conversation
…r configs SagerDatabase was declared version=9 with autoMigrations only up to 7->8 and a blanket fallbackToDestructiveMigration(). On upgrade from a v8 device, Room could not find an 8->9 path and silently dropped/recreated the database, wiping all saved proxy configs, groups, and rules (the irreversible data loss AGENTS.md forbids). The v8->v9 delta is purely additive (two new nullable BLOB columns masterDnsVpnBean, awgBean on proxy_entities), so it is auto-migratable: - Add AutoMigration(from=8, to=9) so 3->9 is fully covered. - Remove the blanket fallbackToDestructiveMigration() from SagerDatabase and PublicDatabase so an unmatched version change can never silently wipe data. - Add an instrumented MigrationTestHelper test proving v8 rows survive 8->9 (DB migrated, not recreated). Wire androidTest deps + testInstrumentationRunner. - Add a Depot CI workflow to run the instrumented test on a KVM-accelerated emulator (Depot CI sandboxes expose /dev/kvm). Namespace stays the APK build host.
📝 WalkthroughWalkthroughAdds an instrumented Android Room migration test for ChangesAndroid Room migration validation
Sequence Diagram(s)sequenceDiagram
participant Workflow as android-instrumented.yml
participant GradleTask as connectedOssDebugAndroidTest
participant MigrationTest as SagerDatabaseMigrationTest
participant Helper as MigrationTestHelper
participant Database as SagerDatabase
Workflow->>GradleTask: runs connectedOssDebugAndroidTest
GradleTask->>MigrationTest: executes migrate8To9_preservesExistingRows()
MigrationTest->>Helper: createDatabase(TEST_DB, 8)
MigrationTest->>Database: runMigrationsAndValidate(..., 9)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![greptile-apps[bot]](/s/com/githubusercontent/avatars/G.https/in/867647?s=60&v=4){kind=small}
Comment on lines
+63
to
+67
| run: | | ||
| find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh \ | ||
| | sort | xargs cat | sha1sum | awk '{print $1}' > libcore_status | ||
| git ls-files libcore | sort | xargs cat | sha1sum | awk '{print $1}' >> libcore_status | ||
| cat libcore_status |
There was a problem hiding this comment.
find buildScript without -type f may include directories in the hash
find buildScript recurses into the directory and emits both files and subdirectories. xargs cat silently errors on directory entries (exit 1 per entry), so the SHA-1 is computed only over files cat succeeds on — but the error output is discarded, making the cache key non-deterministic if the directory list changes between runs. Adding -type f ensures only regular file contents are hashed.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](/s/com/githubusercontent/avatars/G.https/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.depot/workflows/android-instrumented.yml:
- Around line 62-84: The libcore cache fingerprint is missing the ./run wrapper,
so changes to that script can reuse a stale app/libs/libcore.aar. Update the
libcore status generation in the android-instrumented workflow to include ./run
in the hashed inputs used by the libcore-cache key, alongside the existing
buildScript and libcore files, so the Build libcore (Go + gomobile) step
invalidates the cache when the wrapper changes.
- Around line 64-66: The fingerprint generation in the workflow is currently
including directories from the `find` output, which causes the `xargs cat` step
to fail in the cache hash pipeline. Update the hashing logic in the workflow
step that builds `libcore_status` to only operate on regular files by filtering
with `find ... -type f` (or by replacing the `find | xargs cat` pattern with an
`-exec cat`-based approach), while keeping the existing `git ls-files` hash
computation intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da6c24f8-dd47-4706-9af8-9ed7c94de23f
📒 Files selected for processing (7)
.depot/workflows/android-instrumented.ymlapp/build.gradle.ktsapp/src/androidTest/java/io/nekohasekai/sagernet/database/SagerDatabaseMigrationTest.ktapp/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.ktapp/src/main/java/io/nekohasekai/sagernet/database/preference/PublicDatabase.ktbuildSrc/src/main/kotlin/Helpers.ktdepot.json
💤 Files with no reviewable changes (1)
- app/src/main/java/io/nekohasekai/sagernet/database/preference/PublicDatabase.kt
Comment on lines
+62
to
+84
| - name: libcore status | ||
| run: | | ||
| find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh \ | ||
| | sort | xargs cat | sha1sum | awk '{print $1}' > libcore_status | ||
| git ls-files libcore | sort | xargs cat | sha1sum | awk '{print $1}' >> libcore_status | ||
| cat libcore_status | ||
|
|
||
| - name: libcore cache | ||
| id: libcore-cache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: app/libs/libcore.aar | ||
| key: depot-libcore-${{ env.GO_VERSION }}-${{ env.NDK_VERSION }}-${{ hashFiles('libcore_status') }} | ||
|
|
||
| - name: Install Go | ||
| if: steps.libcore-cache.outputs.cache-hit != 'true' | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: ${{ env.GO_VERSION }} | ||
|
|
||
| - name: Build libcore (Go + gomobile) | ||
| if: steps.libcore-cache.outputs.cache-hit != 'true' | ||
| run: ./run lib core |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include ./run in the libcore cache fingerprint.
Line 84 builds the AAR through ./run lib core, but the cache key only fingerprints buildScript and libcore. A wrapper-only change will hit the old cache and compile against a stale app/libs/libcore.aar.
Suggested fix
- find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh -type f -print0 \
+ find run buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh -type f -print0 \
| sort -z | xargs -0 cat | sha1sum | awk '{print $1}' > libcore_status🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.depot/workflows/android-instrumented.yml around lines 62 - 84, The libcore
cache fingerprint is missing the ./run wrapper, so changes to that script can
reuse a stale app/libs/libcore.aar. Update the libcore status generation in the
android-instrumented workflow to include ./run in the hashed inputs used by the
libcore-cache key, alongside the existing buildScript and libcore files, so the
Build libcore (Go + gomobile) step invalidates the cache when the wrapper
changes.
Comment on lines
+64
to
+66
| find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh \ | ||
| | sort | xargs cat | sha1sum | awk '{print $1}' > libcore_status | ||
| git ls-files libcore | sort | xargs cat | sha1sum | awk '{print $1}' >> libcore_status |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Non-file paths currently emitted by the workflow input set:"
find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh ! -type f -print
echo
echo "If anything prints above, the current xargs cat pipeline is unsafe."Repository: hawkff/NekoBoxForAndroid
Length of output: 417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant workflow lines and surrounding context.
nl -ba .depot/workflows/android-instrumented.yml | sed -n '50,80p'
echo
# Reproduce the exact input set and the resulting xargs behavior in a read-only way.
find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh | sort | xargs -r cat >/dev/null
status=$?
echo "xargs cat exit status: $status"Repository: hawkff/NekoBoxForAndroid
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant workflow lines:"
sed -n '58,70p' .depot/workflows/android-instrumented.yml | cat -n
echo
echo "Entries emitted by the first find expression that are not regular files:"
find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh ! -type f -print
echo
echo "Exit status of the exact first pipeline:"
set +e
find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh | sort | xargs cat >/dev/null
status=$?
set -e
echo "$status"Repository: hawkff/NekoBoxForAndroid
Length of output: 1455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant workflow lines:"
sed -n '58,70p' .depot/workflows/android-instrumented.yml | cat -n
echo
echo "Entries emitted by the first find expression that are not regular files:"
find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh ! -type f -print
echo
echo "Exit status of the exact first pipeline:"
set +e
find buildScript libcore/*.sh buildScript/lib/core/get_source_env.sh | sort | xargs cat >/dev/null
status=$?
set -e
echo "$status"Repository: hawkff/NekoBoxForAndroid
Length of output: 1455
Limit the fingerprint pipeline to regular files. find buildScript ... emits directories like buildScript and buildScript/lib/core, so xargs cat exits non-zero and the cache step fails under the workflow shell. Add -type f before hashing, or switch to an -exec cat pipeline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.depot/workflows/android-instrumented.yml around lines 64 - 66, The
fingerprint generation in the workflow is currently including directories from
the `find` output, which causes the `xargs cat` step to fail in the cache hash
pipeline. Update the hashing logic in the workflow step that builds
`libcore_status` to only operate on regular files by filtering with `find ...
-type f` (or by replacing the `find | xargs cat` pattern with an `-exec
cat`-based approach), while keeping the existing `git ls-files` hash computation
intact.
…e-migration # Conflicts: # app/build.gradle.kts
This was referenced Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SagerDatabasewas declaredversion = 9with auto-migrations only up to7->8and a blanketfallbackToDestructiveMigration(). On upgrade from a v8 install, Room could not find an8->9path and silently dropped/recreated the database, wiping all saved proxy configs, groups, and rules.The v8->v9 delta is purely additive (two new nullable BLOB columns
masterDnsVpnBean,awgBeanonproxy_entities), so it is auto-migratable.Changes
AutoMigration(from = 8, to = 9)so3->9is fully covered.fallbackToDestructiveMigration()fromSagerDatabaseandPublicDatabaseso an unmatched version change can never silently wipe data.MigrationTestHelpertest proving a v8 row survives8->9(DB migrated, not recreated).androidTestdeps +testInstrumentationRunner..depot/workflows/android-instrumented.ymlto run the instrumented test on a KVM-accelerated emulator (Depot CI). The APK build stays on Namespace.Testing
grep fallbackToDestructiveMigrationin the database package returns nothing.Notes
:appandroidTest variant compiles the whole app module (references libcore), so the Depot job buildslibcore.aarfrom source (cached). The migration test itself uses no JNI.Greptile Summary
This PR fixes a silent data-loss bug where
SagerDatabasewas declared at version 9 but had no8->9auto-migration, causing Room to invokefallbackToDestructiveMigration()and wipe all proxy configs on upgrade. The fix adds the missingAutoMigration(from = 8, to = 9)and removes the blanket destructive fallback from bothSagerDatabaseandPublicDatabase.AutoMigration(from = 8, to = 9)is added; the v8→v9 delta is purely additive (two nullable BLOB columns), making it safe for auto-migration.fallbackToDestructiveMigration()is dropped from both databases.PublicDatabaseis at v1 with no prior versions so this is immediately safe;SagerDatabasenow has a complete auto-migration chain from v3 to v9.MigrationTestHelpertest proves an existing v8 row survives the migration, backed by a Depot CI workflow running it on a KVM-accelerated emulator.Confidence Score: 4/5
The 8→9 migration fix is correct and well-tested; the only unhandled edge case is users on very old v1/v2 databases who will now see a crash instead of a silent wipe, as noted in the prior review.
The core change — adding AutoMigration(from=8,to=9) and removing fallbackToDestructiveMigration — is sound: the schema delta is purely additive (two nullable BLOBs), the instrumented test validates row preservation end-to-end, and PublicDatabase at v1 has no prior versions so removing destructive fallback there is safe. The remaining gap is that the auto-migration chain starts at v3 while schema files for v1 and v2 exist in the repo, meaning users on those ancient installs will crash on launch rather than wipe — a trade-off acknowledged in the prior review thread but not yet resolved.
app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt — the migration chain starting at v3 leaves v1/v2 databases with no upgrade path.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD V1[DB version 1] -->|no migration path| CRASH[IllegalStateException on launch] V2[DB version 2] -->|no migration path| CRASH V3[DB version 3] --> M34[AutoMigration 3→4] M34 --> M45[AutoMigration 4→5] M45 --> M56[AutoMigration 5→6] M56 --> M67[AutoMigration 6→7] M67 --> M78[AutoMigration 7→8] M78 --> M89["AutoMigration 8→9 ✅ NEW"] M89 --> V9[DB version 9 current] style M89 fill:#22c55e,color:#fff style CRASH fill:#ef4444,color:#fff style V9 fill:#3b82f6,color:#fff%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD V1[DB version 1] -->|no migration path| CRASH[IllegalStateException on launch] V2[DB version 2] -->|no migration path| CRASH V3[DB version 3] --> M34[AutoMigration 3→4] M34 --> M45[AutoMigration 4→5] M45 --> M56[AutoMigration 5→6] M56 --> M67[AutoMigration 6→7] M67 --> M78[AutoMigration 7→8] M78 --> M89["AutoMigration 8→9 ✅ NEW"] M89 --> V9[DB version 9 current] style M89 fill:#22c55e,color:#fff style CRASH fill:#ef4444,color:#fff style V9 fill:#3b82f6,color:#fffComments Outside Diff (1)
app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt, line 19-26 (link)The auto-migration chain starts at
from = 3, and there are no manualaddMigrations()calls. RemovingfallbackToDestructiveMigration()means that any user with a v1 or v2 database will now get anIllegalStateException(crash loop on launch) instead of the previous silent data wipe. Schema files for versions 1 and 2 are present in the repo, confirming those versions existed. While a crash is arguably more transparent than a silent wipe, it does leave those users stuck unless they clear app data. Worth addingaddMigrations()entries or a targetedfallbackToDestructiveMigrationFrom(1, 2)to handle these legacy versions explicitly.Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile