Skip to content

fix(sync): skip no-op draft rewrites and treat \Deleted as removed - #95

Merged
ralflang merged 3 commits into
FRAMEWORK_6_0from
fix/deleted-flag-native-exchange
Jul 28, 2026
Merged

fix(sync): skip no-op draft rewrites and treat \Deleted as removed#95
ralflang merged 3 commits into
FRAMEWORK_6_0from
fix/deleted-flag-native-exchange

Conversation

@TDannhauer

Copy link
Copy Markdown
Contributor

Summary

  • Acknowledge draft Modify commands that carry no changes without performing the IMAP append+delete rewrite (breaks Gmail's Modify echo cycle).
  • Treat \Deleted per native Exchange semantics in all IMAP change-detection strategies: excluded from initial sync, exported as Remove when a tracked message gains the flag, re-exported as Add when it loses the flag.

Motivation

Issue #94: when a third-party IMAP client (e.g. Thunderbird) updates a draft via append + \Deleted flag, Gmail resurrects the old version. Two server-side defects combine:

  1. Gmail echoes a content-identical Modify for every draft Add it receives. The server applied it as a full rewrite: IMAP append+delete, a new UID, and the client's copy materialized over server state.
  2. \Deleted messages were synced as live mail, so the old draft version a third-party client flagged for deletion was never removed from the device — the device kept "reconciling" it back.

Native Exchange neither rewrites an unchanged item nor exposes messages flagged for deletion. This PR aligns Horde with both behaviors for all EAS clients and all mail folders — no Gmail-specific special-casing.

Relation to #93 / #77

This branch is stacked on #93 (fix/draft-uid-alias-wire-identity), which keeps the client's ServerId stable across draft UID churn. #93 fixes the identity bookkeeping; this PR removes the churn itself for no-op edits and completes the deletion semantics. Together they close the loop that the ServerId echo from #77 originally opened. The diff shows both commits until #93 merges, after which it reduces to this one.

Changes

  • lib/Horde/ActiveSync/Connector/Importer.php_isNoOpDraftModify(): before a draft rewrite, fetch the current server copy (untruncated, in the body type the client sent) and compare the EAS-visible fields (To/Cc/Bcc, subject, importance, read, follow-up flag, categories, body with normalized line endings). On a no-op, reply success with the same stable stat shape as a real edit and leave the message untouched. Conservative by design: attachment add/remove instructions, fetch failures, or any mismatch fall through to the regular rewrite path. Expected exception type caught: Horde_Exception (from Horde_ActiveSync_Driver_Base::fetch()).
  • lib/Horde/ActiveSync/Imap/Strategy/Initial.php — initial sync searches with UNDELETED; covers both the CONDSTORE priming path and non-CONDSTORE servers.
  • lib/Horde/ActiveSync/Imap/Strategy/Modseq.php — a tracked message that gained \Deleted is diverted from the change list into the removals (merged with VANISHED UIDs); an untracked one is never exported. A message that loses the flag is untracked by then and exports as a regular Add (genuine undelete).
  • lib/Horde/ActiveSync/Imap/Strategy/Plain.php — same semantics for servers without CONDSTORE, applied in the per-poll flag fetch.
  • Tests: five new importer tests (no-op skip with line-ending normalization, changed-body rewrite, attachment bypass, fetch-failure fallback, aliased-UID comparison) and a new StrategyDeletedFlagTest (Modseq remove/suppress/merge, Plain remove, Initial UNDELETED search).

Upgrade note

Messages already carrying \Deleted before this change converge on their next flag/MODSEQ event or a folder resync; fresh syncs are correct immediately.

Test plan

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

🔍 CI Results

Overall: ❌ 12/12 lanes failed

TL;DR: ❌ Quality issues: PHPStan: 243 unique errors in 11 lanes; PHP-CS-Fixer: 30 files.

Summary by PHP Version

PHP dev stable
8.0
8.1
8.2
8.3
8.4
8.5

Quality Metrics

  • PHPUnit: did not run (11 lanes exited non-zero with no test output) ❌
  • PHPStan (advisory): 243 unique errors in 11 lanes ⚠️
  • PHP-CS-Fixer: 30 unique files with issues (of 211 checked) ⚠️
❌ Failed Lanes

php8.0-dev

  • PHPUnit: 0 failures, 0 errors

php8.0-stable

  • PHPUnit: ❌ Setup failed (Composer install failed in /tmp/horde-ci/lanes/php8.0-stable/ActiveSync: Your requirements could not be resolved to an installable set of packages.)
  • PHPStan: ❌ Setup failed (Composer install failed in /tmp/horde-ci/lanes/php8.0-stable/ActiveSync: Your requirements could not be resolved to an installable set of packages.)

php8.1-dev

  • PHPUnit: 0 failures, 0 errors

php8.1-stable

  • PHPUnit: 0 failures, 0 errors

php8.2-dev

  • PHPUnit: 0 failures, 0 errors

php8.2-stable

  • PHPUnit: 0 failures, 0 errors

php8.3-dev

  • PHPUnit: 0 failures, 0 errors

php8.3-stable

  • PHPUnit: 0 failures, 0 errors

php8.4-dev

  • PHPUnit: 0 failures, 0 errors
  • PHP-CS-Fixer: 30 files with issues

php8.4-stable

  • PHPUnit: 0 failures, 0 errors

php8.5-dev

  • PHPUnit: 0 failures, 0 errors

php8.5-stable

  • PHPUnit: 0 failures, 0 errors

CI powered by horde-componentsView full results

TDannhauer added a commit that referenced this pull request Jul 26, 2026
Field testing of #95 (issue #94) showed Gmail hardcodes Read 0 in its
echoed draft Modifies, so any draft displayed (\Seen) by another
client in the meantime failed the no-op comparison and was rewritten
anyway, leaving a fresh \Deleted copy behind on every echo. A draft's
read state is metadata, not content: exclude it from the comparison.

Also log which field failed the comparison whenever a draft Modify is
applied as a rewrite, so traffic logs show why a rewrite happened.
@TDannhauer

Copy link
Copy Markdown
Contributor Author

Field testing (issue #94) surfaced two follow-ups, pushed as separate commits so they can be judged independently:

  • e0bc7fa — core fix: exclude the read state from the draft no-op comparison. Gmail hardcodes Read 0 in echoed Modifies, so any \Seen draft failed the comparison and was rewritten anyway. Comparison failures are now logged with the mismatching field.
  • 8003d81 — one-time convergence: messages flagged \Deleted before this branch (the flag was ignored by all prior releases) never produce another MODSEQ event, so they would linger on clients forever. A once-per-folder SEARCH DELETED sweep removes them; the swept state persists in the folder state. Without this, every upgrading user would need to re-provision their account.

Gmail echoes a Modify for every draft Add it receives even when the
user changed nothing. Applying it as IMAP append+delete rewrote the
draft under a new UID and materialized the client's copy over server
state, resurrecting prior versions a third-party IMAP client had
flagged \Deleted (#94).

Importer: before applying a draft Modify, compare the EAS-visible
fields (recipients, subject, importance, read, follow-up flag,
categories, body) against the current server copy and acknowledge a
no-op without rewriting the message. Any doubt (fetch failure,
attachment instructions, mismatch) falls through to the regular
rewrite path.

IMAP strategies: apply native Exchange semantics to \Deleted across
all mail folders. Initial syncs exclude \Deleted messages; a tracked
message that gains the flag is exported as a Remove (merged with
expunged UIDs); an untracked message carrying the flag is never
exported; losing the flag re-exports the message as a regular Add.

Fixes #94
Field testing of #95 (issue #94) showed Gmail hardcodes Read 0 in its
echoed draft Modifies, so any draft displayed (\Seen) by another
client in the meantime failed the no-op comparison and was rewritten
anyway, leaving a fresh \Deleted copy behind on every echo. A draft's
read state is metadata, not content: exclude it from the comparison.

Also log which field failed the comparison whenever a draft Modify is
applied as a rewrite, so traffic logs show why a rewrite happened.
Messages flagged \Deleted before the server honored the flag never
produce another MODSEQ event, so CONDSTORE change detection never
converges them and they linger on clients indefinitely (the flag was
ignored by all prior releases). Sweep tracked UIDs once per folder
with SEARCH DELETED, remove matches from the client, and persist the
swept state in the folder object; a failed search is retried on the
next poll. Non-CONDSTORE servers converge without this: the Plain
strategy re-reads all flags on every poll.

This is one-time convergence logic for the semantic change introduced
in this branch; the alternative would be requiring every user to
re-provision their account after the upgrade.
@ralflang
ralflang force-pushed the fix/deleted-flag-native-exchange branch from 8003d81 to c9247ab Compare July 28, 2026 07:55
@ralflang
ralflang merged commit 3c54b2b into FRAMEWORK_6_0 Jul 28, 2026
1 check failed
ralflang pushed a commit that referenced this pull request Jul 28, 2026
Field testing of #95 (issue #94) showed Gmail hardcodes Read 0 in its
echoed draft Modifies, so any draft displayed (\Seen) by another
client in the meantime failed the no-op comparison and was rewritten
anyway, leaving a fresh \Deleted copy behind on every echo. A draft's
read state is metadata, not content: exclude it from the comparison.

Also log which field failed the comparison whenever a draft Modify is
applied as a rewrite, so traffic logs show why a rewrite happened.
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