Fix external storage migration for unsafe attachment filenames

[Darrick (darjoo)]

What & why

Attachment filenames such as Invoice Nov'25.pdf fail migration because generated external paths violate storage validation. Storage Sync previously hid individual reasons behind an aggregate failure count.

• Sanitize generated storage/media-import filenames without changing displayed File Name or File Extension. Preserve GUID uniqueness, folder layout, literal hash/percent characters, and saved External File Path readback without regeneration or decoding.
• Isolate each synchronization step through Boolean worker.Run. Successful writes commit; runtime failures roll back that step and processing continues. Register every failed attachment locally; interactive runs also show Error Messages. Background runs register failures without intentionally failing the job queue entry.
• Retain uncertain uploaded files after failed attachment persistence. Record their created logical paths for recovery; no compensation/by-path deletion API remains.
• Eliminate all DA-app physical remote deletion calls, including the baseline helper/subscriber and both public compatibility overloads. The worker explicitly invokes RetireExternalReference, not a deletion step.
• Adopt the canonical local-retirement guard from #12432 at 5e1de1c93383b0bdc9df5b905ba47040b5cc53c0: fresh permanent row and media under update locks, unchanged requested identity/path/upload/source/media snapshot, Stored Internally, and actual nonempty Tenant Media Content. True means LOCAL reference retired only; remote bytes remain. Temporary, missing, stale and external-only rows are blocked.
• Copy preserves both references. To Internal Move first retires already-valid internal content without HTTP; otherwise it restores nonempty bytes and then attempts guarded local retirement. Summaries distinguish retained remote files, retired local references and blocked retirements. The page/setup text no longer promises remote deletion.
• Preserve per-record diagnostics and sanitized failure telemetry (0000VL5); retention uses canonical 0000W5Q/0000W5R with non-content dimensions. Add restored/external-only/stale worker and compatibility-overload regressions.

This PR is standalone against main and does not require #12438. That proposal concerns internal release, not remote ownership or deletion authority. Remote retention and guarded LOCAL external-reference retirement are intentional independently of whether #12438 ships; no future physical remote deletion is promised.

Linked work

Fixes AB#647832

GitHub issue: N/A; this fix is tracked by the approved Azure DevOps bug.

How I validated this

• I read the full diff and it contains only changes I intended.
• I built the affected app(s) locally with no new analyzer warnings.
• I ran the change in Business Central and confirmed it behaves as expected.
• I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required - be specific: scenarios, commands, screenshots for UI changes)

Current head: 234464d4f3990fd1a7b22463bd80dd40ae62f3ce (documentation-only follow-up, 2026-10-08). Code head inspected: 103e034068185e61930c946fbbadd16dc48ac6f2. The author merged main in 8c139c3a29 to include the upstream Expense API revert #12514, then corrected telemetry dimensions, permissions, and a translator comment. This follow-up changes documentation only; no runtime redesign, PR-base change, force push, or sibling source import.

• git diff --check: passed for the documentation-only follow-up. No new AL code or tests are introduced by this follow-up.
• gh run view 37742245234 --repo microsoft/BCApps --json headSha,status,conclusion,jobs: one-shot inspection showed the code-head run still in progress, 49 jobs completed, 113 in progress, and no failed completed jobs. This is not a passing run or current-head test evidence; no monitoring loop or automatic retry was started.
• At preceding head ea863079ededfb678e4d4359aeb883b8d9cc29f4, gh run download 37517120439 --repo microsoft/BCApps --pattern '*-TestResults-*' --dir <session-artifact-directory> succeeded. Parsing the actual XML proved all 56 declared attachment tests executed and passed in each of 23 country/default artifacts (1,288 passes, no skips or missing attachment cases), on platform 30.0.55814.0. W1/System Default and Clean builds passed. The run still failed in 20 Expense API integration jobs; matched server event logs show a SQL-connection/metadata OData null reference. This is historical evidence, not proof for 103e034068 or this documentation commit. Detailed evidence.
• Earlier own-head python verify_combined_retention.py . source assertions passed at 063c259a0a; that is source-only historical evidence, not current-head compilation or runtime proof.
• The old disposable three-head source integration is superseded and must not be applied. It predates current heads and the changed internal-release contract; its queued-cleanup assertions are obsolete. It is not a prerequisite or acceptance gate for this standalone PR.
• No local BC30 compilation or Run-TestsInBcContainer execution for this documentation follow-up: no approved local BC30 compiler/runtime is available and the local helper previously reported missing Docker permissions. Repository setup is documented in LOCAL_DEV_ENV.md; no container was created or deployed. Current code-head runtime results remain unverified.

Historical evidence only: preceding head da749476 compiled the app/tests in W1 Default/Clean with compiler 30.0.43.2701, with no diagnostics in their compiler blocks. Earlier author-reported validation reproduced the apostrophe failure, compiled in leased BC artifact 30.0.54812.0, ran Run-TestsInBcContainer for 136820 with 48/48 passing tests, and captured six interactive telemetry events plus a non-GUI invocation. That earlier invocation raised the first failure and used a temporary event tag. Those older results also do not validate the current head.

Risk & compatibility

No schema changes or existing external-file renames. Display names stay unchanged. The user-approved retention policy intentionally changes deletion semantics: both DeleteFromExternalStorage overloads return true only for verified LOCAL metadata retirement, never remote deletion. Remote files may remain after record deletion, successful restoration or failed uploads; a logged logical path is diagnostic, not ownership/deletion authority.

Remaining standalone review and validation gates; no merge-ready claim:

• Dedicated privacy-notice approval remains a product/privacy-owner decision. Setup disclaimer and manifest privacyStatement are not runtime approval. Uploads, downloads and outbound metadata require consistent policy; no interactive consent prompt was inserted into a write transaction.
• Missing Discovered uptake is pre-existing in setup/discovery paths. The existing Used event now follows successful persistence, but discovery lifecycle remains unresolved; no fake Discovered event was added at upload time.
• The public DeleteFromExternalStorage compatibility/deprecation decision remains open in two review threads. Both aliases retain their signatures but return true only for guarded LOCAL retirement. Restoring physical remote deletion would contradict the user-approved retention policy; preserving signatures does not erase the semantic change. No deprecation decision has been made on the owner's behalf.
• The report AddLoadFields performance review remains open and needs its own scoped disposition; it is unrelated to [AB#651782] Block unsafe internal attachment cleanup #12438.
• Internal release remains this PR's separate synchronous implementation against main, with fresh/stale checks. It is not an asynchronous queue and does not establish globally owner-safe internal-media release. No [AB#651782] Block unsafe internal attachment cleanup #12438 manager, provenance table, queue/finalizer, or cancellation hook is required or imported here.
• Any future combination with #12438 is optional work requiring a new, explicit integration decision. Its latest proposed internal-release contract blocks release and retains all internal references/bytes; it is not a remote-deletion foundation. Historical queued-success/source integration is superseded, not something this PR must restore or wait for.
• Zero reachable DA-app remote deletion remains this PR's own acceptance requirement. Local external-reference retirement remains allowed only after the locked/nonempty-content guard, independently of future internal-release work.
• #12064's field14 URL/Decoded Path support has merged into main and is included through the author's main merge. Literal filename percent/hash values and persisted readback paths must remain untouched. #12012 trailing-dot folders and #11143 broader colon/apostrophe restrictions remain separate gaps.
• Current-head BC30 compilation, analyzer output, and regression execution remain required. Historical standalone shared-media tests do not prove every ownership/concurrency scenario. No unsupported test or reviewer result is described as a pass; optional cross-PR validation is not a prerequisite for landing this PR.

[alexei-dobriansky]

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This PR sanitizes generated external-storage path components for document attachments, keeps the visible attachment name unchanged, surfaces per-attachment sync failures, and adds regression coverage. The main filename path is aimed at the right root cause, and the tests cover invalid characters, collisions, and round-trip behavior. Two blocking issues remain: the new telemetry path can log raw customer file data, and the changed test-library app does not compile with its current dependencies.

Problem-solution fit

Fit: Partial

The reported problem is clear, and the core path-sanitization design matches it. The solution is incomplete until telemetry uses sanitized error text and the test-library dependency/build issue is fixed.

Suggestions

S1 (🔴 High): Raw last error text can leak telemetry
Do not pass GetLastErrorText(true) to LogSyncFailed. Some storage errors include the external path or attachment name, so this can put customer file data into telemetry. Log a fixed error template for telemetry and keep the raw text only for the Error Messages page.

S2 (🔴 High): Missing dependency breaks System Application build
The test-library app now uses Temp Blob and Temp Blob List, but its app.json only depends on External File Storage and Any. Add the direct BLOB Storage dependency or use types already available to this app, otherwise the System Application module build cannot compile.

Risk assessment and necessity

Risk: The functional path is important because failed migration can leave many attachments unsynchronized. The current change also adds telemetry and test-library changes; the telemetry path can expose customer file data, and the test-library change currently breaks the System Application module build.

Necessity: The fix is necessary and the scope is appropriate: unsafe generated external paths should be sanitized while preserving the visible attachment name, and sync failures should be diagnosable per attachment. The implementation needs the blocking issues fixed before it is safe to merge.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11733 round=1 by=alexei-dobriansky at=2026-09-22T22:13:53Z lastSha=c881b90e3f6cf3d46189d75084b94ed99d37948e reviewKey=e887727f5350bf912f29310fb5bc71d72175548a23b60420f76a114b9f42ee5c suggestions=S1@af6b3ad2,S2@2cf91658

[alexei-dobriansky]

Good Sense Reviewer - Round 2

Recommendation: Request Changes

What this PR does

The new commit adds the missing BLOB Storage dependency for the test-library app. That fixes the compile blocker from round 1, but the telemetry failure path is still unchanged: LogFailure still sends GetLastErrorText(true) into LogSyncFailed.

Status of previous suggestions

ID	Title	Status	Author response
S1	Raw last error text can leak telemetry	Not addressed
S2	Missing dependency breaks System Application build	Addressed	The BLOB Storage dependency was added to the test-library app manifest.

New observations (commits since round 1)

None - the only new change fixes the test-library dependency.

Risk assessment and necessity

Risk: The attachment migration fix is still needed, and the manifest change reduces build risk. The remaining risk is telemetry exposure: detailed storage errors can include customer-specific filenames or external paths.

Necessity: The path sanitization and per-attachment diagnostics remain necessary for failed migrations. Before merge, telemetry should use a fixed safe error template while the detailed error stays only in the user-facing error page.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11733 round=2 by=alexei-dobriansky at=2026-09-23T22:16:21Z lastSha=30d3f4e9aea598b5493b81219049ac2092824a74 reviewKey=1db09f101610a5d10d3a463ef8097633a7066f6f030109111d98a135b01e3825 suggestions=S1@af6b3ad2:notaddressed,S2@2cf91658:addressed parentRound=1

[github-actions]

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.

---

🔄 How to trigger a new status check:

1. 📤 Push a new commit to the PR branch, or
2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.

[alexei-dobriansky]

Good Sense Reviewer - Round 3

Recommendation: Request Changes

What this PR does

The new commit adds specific per-attachment failure reasons and stores failures in the Error Message Register. This improves diagnostics, but detailed storage errors are still sent to telemetry, and background runs now finish successfully even when attachments fail.

Status of previous suggestions

ID	Title	Status	Author response
S1	Raw last error text can leak telemetry	Not addressed	GetLastErrorText(true) is still passed to telemetry.
S2	Missing dependency breaks System Application build	Addressed	The required dependency remains present.

New observations (commits since round 2)

S3 (🔴 High): Background failures now report successful completion
Keep the background run in a failed state when any attachment fails, after registering all messages. Returning normally marks the job queue entry as successful, so operators and monitoring can miss an incomplete migration.

Risk assessment and necessity

Risk: The filename sanitization is targeted and well tested. The remaining risks are customer file data entering telemetry and background synchronization reporting success despite failed attachments.

Necessity: Filename sanitization and per-attachment diagnostics are necessary. Persisting failures is useful, but it must not replace the background failure signal, and telemetry must use safe fixed text.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11733 round=3 by=alexei-dobriansky at=2026-10-01T22:33:48Z lastSha=7e467d82d3c89f7a51a006311df5ecf2a9313882 reviewKey=de2c81987e663d22cdad0df5b283b10f21c952b4b48e8ca92f594d1aa5e01bd1 suggestions=S1@af6b3ad2:notaddressed,S2@2cf91658:addressed,S3@7125f090:new parentRound=2

[Darrick (darjoo)]

Replies to the Good Sense Reviewer rounds (5785108869, 5803989380, 5946352127) and the stale status check note (5851486762):

• S1 (telemetry error text): partially accepted in 4ee3d46. Telemetry now only receives GetLastErrorText(true), which is platform-scrubbed and the pattern BCQuality recommends for LogError, captured right after the failing call, or a fixed precondition label, or a fixed locked fallback. It never falls back to raw GetLastErrorText(). The raw text stays on the Error Messages page and in the register.
• S2 (BLOB Storage dependency in the test library): already addressed in 30d3f4e.
• S3 (background runs should stay failed): declined on purpose. Failing the Job Queue entry would put the recurring sync into Error after its retries and stop all future syncs. Every failure is still written to the Error Message Register and logged to telemetry with SyncMode=Background for monitoring.
• Stale status check: the push of 4ee3d46 re-triggers the PR build.

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Privacy}$

The FeatureTelemetry custom dimensions include the current User ID, attachment File Name, and Document No.; these values can contain employee identifiers, customer content, or pseudonymous identifiers and are sent to telemetry verbatim.

Knowledge:

• microsoft/knowledge/privacy/featuretelemetry-customdimensions-no-pii.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.48.6}

[Darrick (darjoo)]

Batch update in 103e034 for the top-level review comments:

Accepted

• Telemetry privacy (6020985167, 6023986203, 6054409013): removed User ID, File Name and Document No. from the FeatureTelemetry custom dimensions. The remaining dimensions are operational only, and the failure telemetry was already PII-free.

Declined for this PR (pre-existing on main, follow-up candidates)

• Maximum Records to Process starvation (6015492445): the capped primary-key iteration predates this PR. A continuation cursor or retry ledger is a separate design change. This PR removes the invalid-filename failure that caused the incident, and every remaining failure is now listed in the Error Message Register.
• Hardcoded Message/Error in TestFileStorageConnector (6016513618, 6016513957): already on main in the shared test-library mock; this PR does not touch those lines.
• Manual Document Attachment fixture (6019186498): the helper predates this PR, and the test app has no Document Attachment factory library dependency.
• ErrorInfo for the setup disable error (6020984172, 6023984857, 6054407794): the plain Error is already on main; this PR only reworded the label. Fix-it/Show-it UX is a follow-up.

Good Sense Reviewer round 5, S5 (deletion behavior): I am keeping Jesper's retention-only design: guarded local-reference retirement and no remote deletion until #12432/#12438 land. Choosing between that design and restoring the configured external deletion is an owner decision. The related thread on DeleteFromExternalStorage stays open for it.

CI: main reverted #12325 in #12514, which fixed the Expense Agent API IntegrationTests failures. That revert is merged into the branch (8c139c3), and CI is rerunning on 103e034.

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Performance}$

The Upload to External action iterates with FindSet() but passes each iterator record to UploadToExternalStorage, which modifies that same Document Attachment. Use FindSet(true) so the read takes the update lock required by the per-row write instead of relying on a later lock upgrade.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

if DocumentAttachment.FindSet(true) then

Knowledge:

• microsoft/knowledge/performance/findset-true-applies-updlock-on-read.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.49.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Performance}$

The Copy from External To Internal action iterates with FindSet() but passes each iterator record to DownloadFromExternalStorageToInternal, which imports content and modifies that Document Attachment. Use FindSet(true) so the read takes the update lock required by the per-row write instead of relying on a later lock upgrade.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

if DocumentAttachment.FindSet(true) then

Knowledge:

• microsoft/knowledge/performance/findset-true-applies-updlock-on-read.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.49.6}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Performance}$

The background synchronization uploads the file to external storage using a newly generated GUID path, then updates the attachment row. If the local Modify fails after CreateFile succeeds, a retry can upload the same attachment again under a different path and leave another remote file. Derive the external request/path from a stable attachment identifier, or use an idempotency key that the connector enforces, before allowing the job-queue synchronization to retry.

Knowledge:

• microsoft/knowledge/performance/job-queue-external-effects-must-be-idempotent.md

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.49.6}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance}$

RunCompanyMigration iterates with FindSet(true), taking update locks on attachment rows, and then calls MigrateFileToCurrentEnvironment, which performs external GetFile/CreateFile operations before Modify. A slow storage request therefore holds database update locks for the duration of the remote I/O across the migration batch. Read the candidates without update locks, then reacquire an update lock only for the validated row update after the external copy, or otherwise move the external transfer outside the locked iteration.

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.49.6}

[alexei-dobriansky]

Good Sense Reviewer - Round 6

Recommendation: Accept

What this PR does

This change sanitizes generated attachment filenames, preserves display names and saved external paths, and records individual synchronization failures. It also retains remote files and clears local external references only after checking that valid internal content exists.

The new commits remove content-bearing usage telemetry dimensions, clarify standalone retention, and load synchronization fields before iteration. Passing the iterated attachment to later worker steps preserves the refreshed upload state and restored media reference. The locked retirement checks reject stale, temporary and external-only rows. Regression coverage exercises unsafe names, round-trip content, hard-error rollback, failure continuation and guarded retirement; no new defect was found in the author-only changes.

Status of previous suggestions

ID	Title	Status	Author response
S1	Raw last error text can leak telemetry	Disputed	The technical error-text form follows the platform telemetry contract. Raw connector reasons stay in local diagnostics, and usage dimensions no longer include filenames, document numbers or user identifiers. The original blocker is not sustained.
S2	Missing dependency breaks System Application build	Addressed	The required BLOB Storage dependency is present.
S3	Background failures now report successful completion	Disputed	Background completion intentionally allows later attachments and recurring runs to continue. Each failure is persisted and registered, with regression tests for hard-error continuation. The original blocker is not sustained.
S4	Worker filename breaks every Apps build	Addressed	The worker filename matches the object name.
S5	Keep configured external deletion behavior	Disputed	Retention is the explicit standalone contract, and the old deletion setting is read-only. The deletion-named aliases are in an internal codeunit, not a partner-public API. This clarification withdraws the previous scope blocker; it does not mean physical deletion was restored.

New observations (commits since round 5)

None — no new actionable issue was found. The disputed findings above no longer establish a blocker.

Risk assessment and necessity

Risk: Synchronization now has per-step transaction boundaries, and Move and deletion workflows intentionally retain remote bytes. Remote files can remain after their local attachment is removed, so storage retention and cleanup must account for this contract. Guarded local retirement does not establish a global ownership guarantee for internal media. git diff --check passed for the frozen net diff; current-head compilation and runtime results were not verified, and historical passing tests are not current-head proof.

Necessity: Generated filenames must satisfy storage validation without changing attachment display names, and users need specific failure reasons. Without this change, otherwise valid attachments can still fail migration without a useful diagnosis. The retention behavior is a separate, explicit part of this standalone change and does not require an optional future cleanup implementation.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11733 round=6 by=alexei-dobriansky at=2026-10-08T22:32:56Z lastSha=9b25c757ca570c036b154253a8faf03059a8e3c5 reviewKey=2389e37b76f9bcd0251be7f5dd32390cdb949d28964c5d85e805d2443277cb64 suggestions=S1@af6b3ad2:disputed,S2@2cf91658:addressed,S3@7125f090:disputed,S4@60959a31:addressed,S5@fb9fdddd:disputed parentRound=5