Bug 636342: External Storage - Document Attachments does not delete blobs on attachment delete

[Groenbech96]

Fixes AB#636342 — External Storage – Document Attachments does not delete attachments on external storage.

The bug

When a row in Table 1173 "Document Attachment" is deleted in a BC28 environment that has the External Storage – Document Attachments app installed and configured with the default setup record, the corresponding blob in the configured Azure Blob container is not deleted. No error or warning surfaces to the user. The opt-in setting "Delete from External Storage" = true (default) is non-functional on the automatic deletion path. Every deleted attachment silently orphans its blob, so customer Azure storage bills grow monotonically.

Customer impact:

• Silent data leak with monetary cost
• Affects 100% of customers who enable the feature — the buggy code path is unconditional
• Present since the feature first shipped (External Storage - Document Attachments #4495, 2026-02-12) — original-design defect, not a regression
• Linked IcM: 51000001022543

Root cause

In App/src/DocumentAttachmentIntegration/DAExternalStorageImpl.Codeunit.al:

The OnAfterDeleteDocumentAttachment subscriber fires on Database::"Document Attachment", OnAfterDeleteEvent. By the time it runs, the row has already been deleted. The subscriber called ExternalStorageImpl.DeleteFromExternalStorage(Rec), whose first non-trivial line was:

if not DocumentAttachment.Find() then
    exit(false);

Find() always returned false for the deleted row, so the procedure exited before it ever reached ExternalFileStorage.DeleteFile(ExternalFilePath) or the LogFileDeleted telemetry call. A second latent defect: MarkAsNotUploadedToExternal() calls Modify(), which would also fail on a deleted row. The fix has to avoid both Find() and Modify() on the subscriber's code path.

A third dead-code defect: after the (silently failing) delete call, the subscriber attempted to clear Rec."Skip Delete On Copy" via Rec.Modify() — also impossible on a deleted row, and pointless because the row is gone.

The fix

1. Extract the actual file-delete + telemetry into a private helper TryDeleteExternalFile(ExternalFilePath; DocumentAttachmentForTelemetry). No Find(), no Modify(), no record-state assumptions — just GetSpecificFileAccount → DeleteFile → LogFileDeleted.
2. Refactor DeleteFromExternalStorage (used by the page action and the sync report, where the row is still live) to delegate the blob-delete + telemetry to the helper and then call MarkAsNotUploadedToExternal() on success. Behavior preserved for those callers.
3. Rewrite the OnAfterDeleteEvent subscriber to gate on Rec's still-populated field values (Stored Externally, External File Path, Skip Delete On Copy, IsFileFromAnotherEnvironmentOrCompany) and call the helper directly with Rec."External File Path". Remove the dead Modify() block.

The choice to keep one helper instead of duplicating a "by path" overload follows the existing convention for this internal codeunit — one method per concern, no parameter-only overloads in an internal impl.

Tests

New tests in Test/src/DAExtStorageImplTests.Codeunit.al, region OnAfterDelete Subscriber Tests:

• RecordDeleteRemovesBlobFromExternalStorage — the direct regression test. Upload a file, delete the row, assert the blob is gone via CheckIfFileExistInExternalStorage. Would have failed before the fix.
• RecordDeleteKeepsBlobWhenSkipDeleteOnCopyIsSet — guards the copied-attachment case: when a row is created via the attachment-copy flow, Skip Delete On Copy is set and the shared source blob must NOT be deleted when the copy is removed.
• RecordDeleteKeepsBlobWhenDeleteFromExternalStorageDisabled — guards the user opt-out: when the setting is off, deleting the row must not touch the blob.

The existing DeleteFromExternalSucceedsForUploadedFile, DeleteFailsWhenFeatureDisabled, and DeleteSkippedWhenSkipDeleteOnCopyIsSet tests still cover the manual DeleteFromExternalStorage entry point used by the page action and the sync report.

Risk

Low. The TryDeleteExternalFile helper is private to the codeunit. DeleteFromExternalStorage's observable behavior is unchanged for its two existing callers (DocumentAttachmentExternal.Page.al:202, DAExternalStorageSync.Report.al:75) — same gating, same MarkAsNotUploadedToExternal on success, same telemetry. Only the subscriber's behavior changes, and that change is from "silently does nothing" to "actually deletes the blob," which is the documented and intended behavior. The fix is bounded to one app; no platform, no other app, no schema change.

🤖 Generated with Claude Code

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Blob deletion failure silently orphans files

The return value of TryDeleteExternalFile in OnAfterDeleteDocumentAttachment is discarded. If the external blob deletion fails (e.g., storage unavailable, permissions error), the blob is permanently orphaned with no error log, telemetry event, or way to retroactively identify the stranded file.

Recommendation:

• Capture and act on the return value. At minimum, emit a telemetry failure event so operators can detect and clean up orphaned blobs.

Suggested change

	TryDeleteExternalFile(Rec."External File Path", Rec);
	if not TryDeleteExternalFile(Rec."External File Path", Rec) then
	DAFeatureTelemetry.LogFileDeletionFailed(Rec);

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Missing test for cross-environment guard in OnAfterDelete

The new IsFileFromAnotherEnvironmentOrCompany early-exit branch added to OnAfterDeleteDocumentAttachment (line 832 of the impl) has no corresponding test in the new OnAfterDelete Subscriber Tests region, leaving that code path untested.

Recommendation:

• Add a fourth test such as RecordDeleteKeepsBlobWhenFileIsFromAnotherEnvironment that sets up a cross-environment or cross-company attachment and asserts the blob is preserved after the row is deleted.

[Test]
[HandlerFunctions('ConfirmYesHandler')]
procedure RecordDeleteKeepsBlobWhenFileIsFromAnotherEnvironment()
var
    DocumentAttachment: Record "Document Attachment";
    DAExternalStorageImpl: Codeunit "DA External Storage Impl.";
    ExternalFilePath: Text;
begin
    // [SCENARIO] Files owned by another environment/company must not be deleted
    // when the local attachment row is removed.
    Initialize();
    SetupFileScenarioWithTestConnector();
    EnableFeatureWithDelete();

    // [GIVEN] An externally-stored attachment whose path belongs to another environment
    CreateDocumentAttachmentWithContent(DocumentAttachment);
    DAExternalStorageImpl.UploadToExternalStorage(DocumentAttachment);
    DocumentAttachment.SetRecFilter();
    DocumentAttachment.FindFirst();
    // Simulate cross-environment ownership by setting a foreign path prefix
    DocumentAttachment."External File Path" := 'other-env/' + DocumentAttachment."External File Path";
    DocumentAttachment.Modify();
    ExternalFilePath := DocumentAttachment."External File Path";

    // [WHEN] The row is deleted
    DocumentAttachment.Delete(true);

    // [THEN] The blob is still present (not deleted by this environment)
    Assert.IsTrue(DAExternalStorageImpl.CheckIfFileExistInExternalStorage(ExternalFilePath),
        'Blob should be preserved when the file belongs to another environment or company');
end;

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

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[Groenbech96]

Thanks @github-actions — added RecordDeleteKeepsBlobWhenFileIsFromAnotherEnvironment in fa7d079. The intent in your suggestion was right but the trigger needed adjusting: IsFileFromAnotherEnvironmentOrCompany keys off Source Environment Hash, not the path (see the existing IsFileFromAnotherEnvironmentReturnsTrueForDifferentEnvironment test at line 528), so the new test sets a foreign hash on the row before deleting.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

"Try" name without [TryFunction] attribute

TryDeleteExternalFile uses the AL Try-prefix convention, which implies the function catches runtime errors and returns false instead of throwing. Without the [TryFunction] attribute, any unhandled error thrown by ExternalFileStorage.DeleteFile() or FileScenarioCU.GetSpecificFileAccount() will still propagate as an unhandled exception, violating the contract callers expect from a Try-prefixed procedure.

Recommendation:

• Either add [TryFunction] to the procedure so runtime errors are caught and converted to a false return, or rename the procedure to DeleteExternalFile (dropping the misleading Try prefix) and let exceptions propagate naturally.

Suggested change

	// Use the stored external file path
	[TryFunction]
	local procedure TryDeleteExternalFile(ExternalFilePath: Text; DocumentAttachmentForTelemetry: Record "Document Attachment"): Boolean

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}