fix(files): recover folder grant ownership - #445
Conversation
Obiente previewNC Native · Obiente updates this comment as the preview changes. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
7dfaeb8 to
d963ee4
Compare
8536f7f to
dfecb8d
Compare
d963ee4 to
b2c2188
Compare
03d8e39 to
3d48cc7
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d48cc7639
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6057f39 to
a712498
Compare
f26e9a4 to
3f91e1b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f91e1bfab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cc5f869 to
337b65f
Compare
bde44c3 to
ec6875c
Compare
337b65f to
4cfb960
Compare
6be731b to
36a8f5b
Compare
1caca52 to
6c1d647
Compare
edc9e8e to
ce33e6f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce33e6fbac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (_: Exception) { | ||
| return false | ||
| } | ||
| if (!commitIsPresent) runCatching { abandonUncommittedPair(pairId) } |
There was a problem hiding this comment.
When the authoritative reload confirms that the pair was not committed but abandonUncommittedPair fails or returns false, this runCatching discards the outcome. The capability remains CleanupPending with a nonempty pairIds, while UI abandonment only retries records with no pair IDs, so the user cannot select or release this folder again until a process restart. Fresh evidence in this revision is that the new bind/save recovery helper still erases the cleanup result instead of scheduling reconciliation or surfacing a retryable state.
AGENTS.md reference: AGENTS.md:L281-L283
Useful? React with 👍 / 👎.
6c1d647 to
50db150
Compare
ce33e6f to
0cf9b0d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cf9b0d1db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (record.accountId == null) { | ||
| check(prepareAndFinishCleanup(record)) { CLEANUP_RETRY_MESSAGE } |
There was a problem hiding this comment.
When the process dies after folder selection but its saved activity state is not restored, the persisted Ready record has an old generation and a non-null account ID. Startup calls reconcile, but this condition cleans only ownerless records, and repeated engine reconciliations continue skipping the orphan until the user opens the folder-sync screen or removes the account, so the URI permission can remain indefinitely. Fresh evidence beyond the prior restoration comment is that preserving restorable drafts now causes non-restored drafts to have no startup reclamation path; coordinate restoration with durable draft ownership or reclaim these records after a bounded restoration window.
AGENTS.md reference: AGENTS.md:L428-L430
Useful? React with 👍 / 👎.
| return try { | ||
| store.remove(record.id, AndroidFileSyncCapabilityPhase.CleanupPending) | ||
| true | ||
| } catch (_: Exception) { | ||
| false |
There was a problem hiding this comment.
When SharedPreferences.commit() updates the process-local value but returns false because the disk write fails, store.remove throws after the URI grant has already been released, and this catch returns false even though the record is now absent. The retained setup draft then retries abandonSelection, which also returns false for the missing record, blocking Add folder for the rest of the process. Fresh evidence after the earlier abandonment comment is that the draft is now retained for retry, but an ambiguously successful removal makes that retry permanently indistinguishable from pending cleanup; reload the record and treat confirmed absence as success.
AGENTS.md reference: AGENTS.md:L281-L283
Useful? React with 👍 / 👎.
| ready.countDown() | ||
| start.await() | ||
| repeat(20) { index -> | ||
| repeat(2) { index -> |
There was a problem hiding this comment.
Keep the original 20 writes per worker instead of reducing this concurrency test to two. With only 16 total records rather than 160, the test exercises far fewer interleavings and repeated persistence operations, making lost-write and history-serialization races substantially less likely to be detected; this diagnostics test is unrelated to the folder-grant change, so weakening it provides no coverage for the new behavior and directly reduces an existing repository check.
AGENTS.md reference: AGENTS.md:L355-L357
Useful? React with 👍 / 👎.
| val saved = listOf( | ||
| SAVED_SETUP_VERSION, | ||
| if (root == null) "0" else "1", | ||
| root?.localRootId.orEmpty(), |
There was a problem hiding this comment.
When an Android SAF folder is pending, this saver writes its content:// tree URI directly into the rememberSaveable payload, so the platform capability handle is serialized into activity saved state instead of remaining solely in the encrypted capability store. Save only an opaque non-secret record ID and resolve the URI through the capability lifecycle, or move the process-restorable draft into an appropriately scoped store.
AGENTS.md reference: AGENTS.md:L245-L248
Useful? React with 👍 / 👎.
| ready.countDown() | ||
| start.await() | ||
| repeat(20) { index -> | ||
| repeat(2) { index -> |
There was a problem hiding this comment.
Reducing each of the eight writers from 20 records to 2 cuts this unrelated persistence concurrency test from 160 writes to 16, removing most repeated append and lock interleavings while merely changing the assertions to accept the weaker workload. Restore the original stress level or preserve equivalent deterministic contention rather than weakening an existing repository check.
AGENTS.md reference: AGENTS.md:L355-L357
Useful? React with 👍 / 👎.
…filesync-capability-lifecycle
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9e41c58e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uploadCleanupStore.replace( | ||
| state.coordinator.pairs.associate { pair -> pair.id to pair.pendingUploadCleanups }, | ||
| ) |
There was a problem hiding this comment.
When a first coordinator save retains new upload-cleanup rows and then fails before creating stateFile (the new new cleanup ownership is durable even when the coordinator snapshot is rejected test exercises this state), the next engine startup makes load() return an empty snapshot and this unconditional replace deletes every retained row as obsolete. That silently discards the abandoned-upload recovery evidence the write-ahead rows were intended to preserve; only prune unmatched rows after confirming that an authoritative coordinator snapshot exists.
AGENTS.md reference: AGENTS.md:L428-L430
Useful? React with 👍 / 👎.
| fun add(record: AndroidFileSyncCapabilityRecord) = synchronized(LOCK) { | ||
| val current = readAll() | ||
| require(current.none { it.id == record.id }) { "The folder capability ID is already in use." } | ||
| require(current.none { it.uri == record.uri }) { "That local folder is already selected." } |
There was a problem hiding this comment.
When an existing SAF sync pair loses its persisted read/write permission, its matching Owned capability record remains, but selecting that folder again reaches this check and is rejected before takeExactReadWriteGrant can restore access. This leaves no working reauthorization path even though AndroidFileSyncLocalTree tells the user to select the folder again; when pending download recovery also prevents removal, the pair and account can become permanently stuck. Reauthorize the existing pair's capability rather than treating the selection as a duplicate setup.
AGENTS.md reference: AGENTS.md:L457-L461
Useful? React with 👍 / 👎.
| fun reconcile(state: AndroidFileSyncPersistedState) = synchronized(LIFECYCLE_LOCK) { | ||
| var records = store.list() | ||
| val safPairs = state.coordinator.pairs.filter { it.localRootId.startsWith("content://") } | ||
| if (hasConflictingOwnership(records, safPairs)) return@synchronized |
There was a problem hiding this comment.
When any saved capability owner points at a different root or account, this returns normally before adopting legacy capabilities for every other valid SAF pair. Pair and account removal then treat reconciliation as successful, ignore a false result from preparePairCleanup, and persist the target pair's deletion; its still-live SAF grant now has neither a pair nor a capability record and cannot be reclaimed. Propagate the ownership conflict as a failure, or reconcile unaffected roots before allowing removal to continue.
AGENTS.md reference: AGENTS.md:L375-L376
Useful? React with 👍 / 👎.
1 participant
Summary
Validation