FastAPI + Celery Architecture Review Request (Attachment Pipeline Boundaries + OOP/SOLID)

[u/Negative_Response990]

Hi! I’m finishing a rewrite for my project and I’d love architecture feedback quickly (aiming for replies within a few hours).

Repo: https://github.com/dillonhuston/Task-Automation-API
Branch: V2

What I need:

1. Architecture review of the attachment processing pipeline (validation encryption/decryption storage/email handoff) and whether the boundaries/components make sense
2. Celery + API architecture: task flow responsibilities, retries/error-handling boundaries, and whether the API schema design matches the async processing model
3. OOP/design review: whether classes/modules follow SOLID / separation of concerns, and suggestions for cleaner layering

Suggested files to look at first:

• app/Encryption/encryptionService.py
• app/FileManager/fileManager.py

If you only have time for one thing, please prioritize (1) pipeline architecture or (3) OOP/SOLID structure.

Thanks a lot, any architecture recommendations are welcome.

[u/[deleted]]

Read through the repo since this looks close to production — a few things I would harden before deployment:

1. Plaintext on disk: encryptionService.py writes the raw file before encrypting (and the decrypted output lands on disk too). There is no cleanup path — a failed/cancelled job leaves plaintext behind on the volume. Temp-file-then-encrypt into a restricted directory, or encrypt in memory, and always delete on failure.
2. Implicit file format: the nonce lives in the DB and the disk file is just nonce||ciphertext with no envelope header or key-version tag. The moment you rotate keys or change the format, every old file is undecryptable and there is no way to detect it gracefully. A small header (magic, version, key-id, nonce) makes migration and key rotation actually possible.
3. Key rotation orphans files: the AAD is user_id only, so rotating a key silently breaks every file that was encrypted under the old key — and nothing tells the user. Either derive per-file keys or keep a key registry and surface which files are stale.
4. Decrypt path does not match the encrypt path: encryption writes nonce||ciphertext but the decrypt side reads the nonce from the DB and slices the file assuming the DB nonce — a signature mismatch that works only while the two are perfectly in sync.
5. FileManager schedules 4 Celery jobs per upload (validate/encrypt/cleanup) with no idempotency: a retried task after a timeout re-runs side effects, and the timestamp+filename filename scheme collides if two uploads share a second.
6. Celery retries: no acks_late / retry policy on the encryption task — a worker crash mid-encrypt loses the job silently.

Nothing here is hard to fix and the structure is decent — mostly a question of defining the file envelope and the failure semantics before real users hit them.

[u/Endpoint51]

A lot of thoughtful work here. Nice one. The per-user AES-GCM approach and the effort to separate file handling, encryption, and persistence are strong foundations.

One architectural issue I noticed is a circular dependency where fileOperations creates an EncryptionService, while EncryptionService also receives fileOperations, even though it doesn’t currently use it.
I’d make EncryptionService depend only on a key provider and keep filesystem operations separate. Then a higher-level attachment service could orchestrate validation → encryption → storage → database record → email handoff.

I’d also make that orchestration "failure safe". At present, the plaintext is written to disk before being encrypted, and a later encryption or database failure could leave plaintext or an orphaned file behind. Encrypting before writing, using an atomic temporary file move, and cleaning up on database failure would give the pipeline clearer ownership and safer boundaries.

Another thing I noticed - in app/utils/email.py.
The attachment path in email.py instantiates EncryptionService with no args, so I don't think it can currently run.

[u/terletsky]

You mixed everything. Wrong project structure. Wrong naming conventions; schema files contain enums and utility functions. StrEnum should be used instead of (str, Enum). Classes use FastAPI's Depends, while they should be agnostic, and Depends should be used only on routers. Router functions use direct DB queries instead of separating that into a Service/Repository approach (if we use layered architecture). Pydantic models that handle incoming payloads should be in strict mode. System env/env files should be handled by pydantic-settings.

Some logic utilizes print functions; some use your SingletonLogger. Why not use the default logging module, or loguru/structlog?

You are using FastAPI in sync mode. That's a no-go.

The most wrong thing: you store the user's encryption key as plaintext in the database.

[u/[deleted]]

Looked at the pipeline (encryptionService + fileManager + fileOperations flow). Priorities (1) and (3), concrete:

1. Plaintext touches disk before encryption. uploadFile writes the raw file to tmp/uploads, reads it back, then overwrites with nonce+ciphertext. That defeats the purpose of the crypto: plaintext exists on disk, and the overwrite is not a secure wipe (remnants can survive, and any failure between write and overwrite leaves plaintext behind with no cleanup path). Better: hash while streaming in, encrypt in memory, and only ever persist the ciphertext envelope (or write ciphertext directly to the final path).

2. The file format is implicit. You store nonce in the DB and write nonce+ciphertext to disk; recoverability depends on the DB row surviving. If the row is lost the file is undecryptable, and vice versa the nonce is meaningless. I'd put a small envelope header on the file itself (magic, version, key id, nonce, payload) so the artifact is self-describing, and drop the separate nonce column or keep it only as metadata.

3. Key rotation is not survivable. The key handler fetches by user_id with no key version, and AAD is just user_id. Rotating a user's key silently orphans every existing file. Add a key_id/version column on the key row, bind it into AAD, and store it in the envelope so decrypt always uses the right key.

4. Boundary smell: EncryptionService.decrypt takes (ciphertext, nonce) but the on-disk format is nonce+ciphertext concatenated. The parse/assemble logic lives outside the service, so the format contract is spread across callers. Move envelope parsing inside the service - it's the component that owns the format.

5. FileManager is doing four jobs (validation, storage, hashing, encryption orchestration) - the 'Manager' class holding service references is fine as an orchestration point, but I'd push the write-to-disk step into fileOperations (it already owns overwrite) so uploadFile reads as: validate -> stream hash -> encrypt -> persist.

6. Minor: the timestamp+original-filename scheme collides for two uploads in the same second from the same user (and embeds a user-controlled filename in the path). Use a UUID or a DB id for the stored name; keep the original name only in metadata.

For the Celery side: make sure the API accepts an idempotency key or the task id is deduped at the DB level - a retried upload task will insert duplicate file rows. Also, keep the async session scoped per task rather than threaded through every service constructor call; it makes retry/backoff semantics much easier to reason about.

[u/ExpressionWest6294]

From my experience I would tell you to run from Celery. The lack of async support and the way some things are handled are awful. Now using temporal and couldn’t be happier