BCDA-10345: Core lambda refactors and cleanup by carlpartridge · Pull Request #1486 · CMSgov/bcda-app

carlpartridge · 2026-09-03T18:00:07Z

🎫 Ticket

https://jira.cms.gov/browse/BCDA-10345

🛠 Changes

Split s3_fileprocessor logic into its attr-import specific code (and moved to cclf.go) and into its s3 specific code (and moved to aws package)
Simplified some interfaces and removed others.
Removed local file processor as it is extraneous code that is not used in any actual env outside of local testing.
Moved s3 file handler into bcda/aws package. Merged the two versions (attr-import and bene-prefs had slight differences between their functions).
Added an init handler to bene-prefs lambda (mimics attr-import and best practices).
Converted some functions and data refs to unexported (private, lowercase starting letter).
Remove references to pkg/errors in favor of built in golang version.
Remove some unused cli functionality and related code.
Temporarily comment out tests that need ministack implementation.

ℹ️ Context

Refactoring and cleanup of core lambdas. Reduce complexity and cruft, increase readability, and make future changes and improvements easier.

🧪 Validation

Local linting and testing.

carlpartridge · 2026-09-04T13:26:47Z

Testing specific utils that I dont think we use nor seem useful. Easy to bring back if need be.

carlpartridge · 2026-09-04T13:31:26Z

 			},
 		},
-		{
-			Name:     "import-synthetic-cclf-package",


I dont think ive ever seen us use this.

I see it being referenced in scripts/bulk_import_synthetic_cclf_package.sh. If we are removing this, that script could also be removed.

carlpartridge · 2026-09-04T13:35:15Z


 type BenePrefsImporter struct {
-	FileHandler          BenePrefsFileHandler
+	FileClient           bcdaaws.CustomS3Client


Ive moved away from some more generic file clients and just set s3 as the default. I think thats fine as it simplifies many things and there are no plans to move away from s3 anytime in the distant future so we can approach this when necessary. This CustomS3Client allows for mocks as well as actual s3 clients (live use as well as ministack).

…com/CMSgov/bcda-app into carl/bcda-10345-core-lambda-cleanup

bhagatparwinder · 2026-09-04T20:46:03Z

-	ctx := context.Background()
-
-	assert := assert.New(s.T())
+// func (s *CCLFTestSuite) TestImportCCLF0() {


Now that I see this on the PR, I am a bit hesitant to comment out tests. These tests weren't just testing local file processor code, but they were also validating file header/trailer, CCLF0 and CCLF8 matching, pgx bulk imports, DB connections, and deduplication. We are basically losing the entire attribution and suppression pipeline.

Maybe instead of choosing between bringing back LocalFileProcessor or leaving tests commented out until ministack is ready, we can use the CustomS3Client interface that’s already in place.

We can wire the existing test fixtures by mocking GetObject and ListObjectsV2 to return the test file bytes/zips.

Hmm, ministack would do this for us without the need for customizing mocks for specific tests but I think your concern is very valid. Will dig into, ty for the push!

bhagatparwinder · 2026-09-04T20:50:02Z


 	if recordCount > validator.totalRecordCount {
-		err := fmt.Errorf("unexpected number of records imported for file %s (expected: %d, actual: %d)", fileMetadata.name, validator.totalRecordCount, recordCount)
+		err := fmt.Errorf("unexpected number of records imported for file %s (expected: %d, actual: %d), err: %w", fileMetadata.name, validator.totalRecordCount, recordCount, err)


err is nil here; we checked it above.

bhagatparwinder · 2026-09-04T20:50:52Z

+
+			if metadata.cclfNum == 0 {
+				if cclf0Metadata != nil {
+					readError = fmt.Errorf("multiple CCLF0 files found in zip (%s/%s), err: %w", bucket, *obj.Key, err)


same thing with err being nil here; we checked it above.

bhagatparwinder · 2026-09-04T20:52:53Z

+	}
+
+	for _, obj := range s3Objects {
+		metadata, err := parseMetadata(*obj.Key)


Are we not checking for err being nil here?

bhagatparwinder · 2026-09-04T20:53:31Z

+
+	for _, obj := range s3Objects {
+		metadata, err := parseMetadata(*obj.Key)
+		metadata.FilePath = fmt.Sprintf("s3://%s/%s", bucket, *obj.Key)


and mutation of metadata should be after a err check.

bhagatparwinder · 2026-09-04T20:58:04Z

+	if err != nil {
+		return fmt.Errorf("file %s failed to clean up properly, error occurred while deleting object: %w", filePath, err)
+	} else {
+		err = s3.NewObjectNotExistsWaiter(client).Wait(


We don't need this for PUT and DELETE operations in S3. This is only adding latency to our Lambda execution. S3 has strong read-after-write consistency for both operations. If you check the s3_file_handler.go file deleted in this PR, you'll see that we don't do this.

My understanding is that the s3 Delete method accepts the request (and returns success) but then async actually deletes the file. This polls to make sure that the file was actually deleted and is the only way to make 100% sure that things are as expected. I agree that this does cause extra delay and is likely unnecessary. As you mentioned s3 has good consistency and we also have a lifecycle policy to clean up files after ~2 weeks. I dont think adding (likely at most) a few seconds to each lambda invocation is the end of the world to keep things 100% but also happy to remove this if everyone agrees that it is excessive. One last point is that bene-prefs is currently a bit awkwardly built in that it retries every file in a directory each time it is run. Meaning that a file that fails to delete will be re-run, and if it has already been successfully imported but not deleted then it will throw an error on unique name check, which could send us an alert.

bhagatparwinder · 2026-09-04T21:05:55Z

 	file.data = data

-	err = importer.ProcessCSV(file)
+	err = importer.processCSV(file)


The parent function takes context from the caller, but we are not passing it to processCSV; instead, processCSV declares a new context.

Edit: We are doing the same for importCCLF0 but we are passing it to importCCLF8.

bhagatparwinder · 2026-09-04T21:12:58Z

+	return objects, nil
+}
+
+func OpenFileAsScanner(ctx context.Context, client CustomS3Client, filePath string) (*bufio.Scanner, func(), error) {


Please let's make a ticket for changing this implementation. We have validate and scanAndImport using this method. This method then calls OpenFileAsBytes below. We are reading entire files into memory. I remember we've had to increase memory limits in the past for this (because of large files). We can get a better performance and memory usage by using a stream.

Very valid concern. Both of the uses of this function are part of bene-prefs which only deal with very small files. Im also not happy with this implementation/functionality but didnt feel like it was part of the scope of this ticket to adjust this (and therefore the nitty gritty workings of validate and scanAndImport. We have a ticket designed to address memory concerns for attr-import that we could maybe adjust to take a look at this as well.

bhagatparwinder · 2026-09-04T21:15:35Z

 			},
 		},
-		{
-			Name:     "import-synthetic-cclf-package",


I see it being referenced in scripts/bulk_import_synthetic_cclf_package.sh. If we are removing this, that script could also be removed.

carlpartridge added 5 commits September 2, 2026 13:55

BCDA-10345: Core lambda refactoring and cleanup

0480117

More refactoring

d7532ac

More refactoring

c6ac647

More refactoring

75170c3

Comment out tests for now

35711ba

carlpartridge deployed to test September 3, 2026 18:00 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 18:01 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 18:02 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 18:03 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 18:04 — with GitHub Actions Active

Fix test

0b82cf0

carlpartridge deployed to test September 3, 2026 20:05 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:06 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:07 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:08 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:09 — with GitHub Actions Active

Fix linting issues

419ce6e

carlpartridge deployed to test September 3, 2026 20:37 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:39 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:40 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:41 — with GitHub Actions Active

carlpartridge deployed to test September 3, 2026 20:42 — with GitHub Actions Active

carlpartridge commented Sep 4, 2026

View reviewed changes

Merge branch 'main' into carl/bcda-10345-core-lambda-cleanup

41b39ef

carlpartridge marked this pull request as ready for review September 4, 2026 13:39

carlpartridge requested a review from a team as a code owner September 4, 2026 13:39

carlpartridge deployed to test September 4, 2026 13:39 — with GitHub Actions Active

carlpartridge deployed to test September 4, 2026 13:40 — with GitHub Actions Active

carlpartridge deployed to test September 4, 2026 13:41 — with GitHub Actions Active

carlpartridge deployed to test September 4, 2026 13:42 — with GitHub Actions Active

carlpartridge deployed to test September 4, 2026 13:43 — with GitHub Actions Active

carlpartridge added 2 commits September 4, 2026 10:09

Cleanup

438f294

Merge branch 'carl/bcda-10345-core-lambda-cleanup' of https://github.…

44fdf87

…com/CMSgov/bcda-app into carl/bcda-10345-core-lambda-cleanup

carlpartridge deployed to test September 4, 2026 14:10 — with GitHub Actions Active

carlpartridge deployed to test September 4, 2026 14:12 — with GitHub Actions Active

carlpartridge deployed to test September 4, 2026 14:13 — with GitHub Actions Active

carlpartridge deployed to test September 4, 2026 14:14 — with GitHub Actions Active

bhagatparwinder reviewed Sep 4, 2026

View reviewed changes

Uh oh!

Conversation

carlpartridge commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Changes

ℹ️ Context

🧪 Validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlpartridge Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

carlpartridge commented Sep 3, 2026 •

edited

Loading

carlpartridge Sep 4, 2026 •

edited

Loading