xftp-server: support postgresql backend by shumvgolove · Pull Request #1755 · simplex-chat/simplexmq

shumvgolove · 2026-03-31T12:59:24Z

No description provided.

When a file is concurrently deleted while addRecipient runs, the FK constraint on recipients.sender_id raises ForeignKeyViolation. Previously this propagated as INTERNAL; now it returns AUTH (file not found).

expireServerFiles unconditionally subtracted file_size from usedStorage for every expired file, including files that were never uploaded (no file_path). Since reserve only increments usedStorage during upload, expiring never-uploaded files caused usedStorage to drift negative.

setFilePath result was discarded with void. If it failed (file deleted concurrently, or double-upload where file_path IS NULL guard rejected the second write), the server still reported FROk, incremented stats, and left usedStorage permanently inflated. Now the error is checked: on failure, reserved storage is released and AUTH is returned.

The status field (e.g. "blocked,reason=spam,notice={...}") is quoted in CSV for COPY protocol, but embedded double quotes from BlockingInfo notice (JSON) were not escaped. This could break CSV parsing during import. Now double quotes are escaped as "" per CSV spec.

In Postgres mode, getFile returns a snapshot TVar for fileStatus. If a file is blocked between getFile and setFilePath, the stale status check passes but the upload should be rejected. Added status = 'active' to the UPDATE WHERE clause so blocked files cannot receive uploads.

Prevents negative or zero file_size values at the database level. Without this, corrupted data from import or direct DB access could cause incorrect storage accounting (getUsedStorage sums file_size, and expiredFiles casts to Word32 which wraps negative values).

importFileStore now checks if the target database already contains files and aborts with an error. Previously, importing into a non-empty database would fail mid-COPY on duplicate primary keys, leaving the database in a partially imported state.

When setFilePath fails (file deleted or blocked concurrently, or duplicate upload), the uploaded file was left orphaned on disk with no DB record pointing to it. Now the file is removed on failure, matching the cleanup in the receiveChunk error path.

The store action result (deleteFile/blockFile) was discarded with void. If the DB row was already deleted by a concurrent operation, the function still decremented usedStorage, causing drift. Now the error propagates via ExceptT, skipping the usedStorage adjustment.

deleteFile result was discarded with void. If a concurrent delete already removed the file, deleteFile returned AUTH but usedStorage was still decremented — causing double-decrement drift. Now the usedStorage adjustment and filesExpired stat only run on success.

- Move STMFileStore and its FileStoreClass instance from Store/STM.hs back into Store.hs — the separate file was unnecessary indirection for the always-present default implementation. - Parameterize xftpFileTests over store backend using HSpec SpecWith pattern (following SMP's serverTests approach). The same 11 tests now run against both memory and PostgreSQL backends via a bracket parameter, eliminating all *Pg test duplicates. - Extract shared run* functions (runTestFileChunkDeliveryAddRecipients, runTestWrongChunkSize, runTestFileChunkExpiration, runTestFileStorageQuota) from inlined test bodies.

- Remove internal helpers from Postgres.hs export list (withDB, withDB', handleDuplicate, assertUpdated, withLog are not imported externally) - Replace local isNothing_ with Data.Maybe.isNothing in Env.hs - Consolidate duplicate/unused imports in XFTPStoreTests.hs - Add file_path IS NULL and status guards to STM setFilePath, matching the Postgres implementation semantics

Remove old non-parameterized test wrapper functions that were superseded by the store-backend-parameterized test suites. All test bodies (run* and _ functions) are preserved and called from the parameterized specs. Clean up unused imports.

epoberezkin · 2026-04-11T06:35:25Z

src/Simplex/FileTransfer/Server/Store/Postgres/Migrations.hs

+import Text.RawString.QQ (r)
+
+xftpSchemaMigrations :: [(String, Text, Maybe Text)]
+xftpSchemaMigrations =


merge them to one, no need to have before it's released

once there is the second, there needs to be the test that generates a combined schema, but not needed now

Addressed in 6cac469

epoberezkin · 2026-04-11T06:36:21Z

src/Simplex/FileTransfer/Server/Store/Postgres/Migrations.hs

+m20260402_file_size_check :: Text
+m20260402_file_size_check =
+  [r|
+ALTER TABLE files ADD CONSTRAINT check_file_size_positive CHECK (file_size > 0);


why is int needed at all?

we usually don't do it in schema, it should be in code

FNEW protocol (Server.hs:491) — validates size ∈ {64KB, 256KB, 1MB, 4MB}, rejects with SIZE otherwise. Cannot produce 0 or any other invalid value.

database import (Postgres.hs:245, via StoreLog.hs:62 parser) — parses size :: Word32 from the store log with strP and COPYs it straight to
PostgreSQL. No validation at all — accepts any Word32 including 0 and any non-chunk-size value (e.g. 123).

The CHECK provides defense on the import path that the Haskell code doesn't cover.

epoberezkin · 2026-04-11T08:26:18Z

src/Simplex/FileTransfer/Server/Store/Postgres.hs

+        >>= either handleDuplicate (pure . Right)
+    withLog "addRecipient" st $ \s -> logAddRecipients s senderId (pure $ FileRecipient rId rKey)
+
+  getFile st party fId = runExceptT $ case party of


this needs refactoring similarly to how getQueue in SMP is done, so that shared part is extracted

Addressed in 26bcc72

epoberezkin · 2026-04-11T08:27:11Z

src/Simplex/FileTransfer/Server/Store/Postgres.hs

+      DB.execute db "DELETE FROM recipients WHERE recipient_id = ?" (Only rId)
+    withLog "ackFile" st $ \s -> logAckFile s rId
+
+  expiredFiles st old limit =


how are records for expired files are removed? If one by one, it's not good.

Current behaviour (one-by-one, confirmed):

expireServerFiles in Server.hs:659-673 calls expiredFiles st old 10000 to get a batch of up to 10,000 expired file records, then loops over them and calls deleteFile st sId for each one (line 667).

For the PG backend, every deleteFile call goes through withDB "deleteFile" which wraps a fresh withTransaction with a DELETE FROM files WHERE sender_id = ?. So a batch of 10,000 expired files = 10,000 PostgreSQL transactions = 10,000 commits + round trips. Each transaction also fires the ON DELETE CASCADE to recipients.

Options, in order of invasiveness:

Add deleteFiles :: s -> [SenderId] -> IO (Either XFTPErrorType ()) to FileStoreClass (with a default impl that loops deleteFile for STM). PG impl would run a single DELETE FROM files WHERE sender_id = ANY(?) in one transaction, and one logDeleteFile burst to the store log. Expire loop in Server.hs:661 does the filesystem removals first, collects sIds, then calls deleteFiles once.

Restructure expireServerFiles only: gather all sIds upfront (still via expiredFiles batch), do fs removals, then issue one batched delete. Same shape as option 1 but keeps deleteFile as-is.

epoberezkin · 2026-04-11T08:27:40Z

src/Simplex/FileTransfer/Server/Store/Postgres.hs

+
+  getUsedStorage st =
+    withTransaction (dbStore st) $ \db -> do
+      [Only total] <- DB.query_ db "SELECT COALESCE(SUM(file_size::INT8), 0)::INT8 FROM files"


why INT8 and not just INTEGER?

Because file_size is INT4 and a SUM over many INT4 values can overflow the 2^31-1 (~2.1 GB) INT4 range - summing even ~500 × 4 MB chunks already exceeds it. Casting each row to INT8 before summing and the final SUM to INT8 gives a 63-bit accumulator, matching the Haskell Int64 return type

epoberezkin · 2026-04-11T08:28:07Z

src/Simplex/FileTransfer/Server/Store/Postgres/Migrations.hs

+  [r|
+CREATE TABLE files (
+  sender_id BYTEA NOT NULL PRIMARY KEY,
+  file_size INT4 NOT NULL,


why int4 and not just integer?

They're the same - INT4 is PostgreSQL's internal name for the 4-byte integer type; INTEGER/INT are SQL standard aliases for the exact same type. No semantic difference.

epoberezkin · 2026-04-11T08:28:18Z

src/Simplex/FileTransfer/Server/Store/Postgres/Migrations.hs

+  file_digest BYTEA NOT NULL,
+  sender_key BYTEA NOT NULL,
+  file_path TEXT,
+  created_at INT8 NOT NULL,


created_at stores a Unix timestamp in seconds as Int64 on the Haskell side (RoundedFileTime = RoundedSystemTime 3600, backed by Int64). INTEGER/INT4 is 32-bit and overflows in year 2038 (2^31 seconds since epoch ≈ 2038-01-19). INT8 is 64-bit and has no practical overflow. This is the standard Y2038-safe timestamp encoding.

epoberezkin · 2026-04-11T08:30:13Z

src/Simplex/FileTransfer/Server/Store/Postgres.hs

+  sl <- openWriteStoreLog False storeLogFilePath
+  putStrLn "Exporting files..."
+  -- Load all recipients into a map for lookup
+  rcpMap <- withTransaction (dbStore pgStore) $ \db ->


I don't see how it can work. You are exporting recipients before you are exporting actual files. When server imports files it would just drop recipients for which it has no files (= all of them)

The rcpMap loaded upfront (Postgres.hs:277-285) is only an in-memory Data.Map used for O(1) lookup - it is not written to the store log.

The actual store log write order is per-file, inside the single fold at lines 287-305:

logAddFile → FNEW sId (line 296)

logAddRecipients → FADD sId (line 299)

logPutFile → FPUT sId (line 300)

So each file's FADD is written immediately after its own FNEW. On import, replay sees FNEW before FADD for every sId and addRecipient succeeds.

epoberezkin · 2026-04-11T08:31:45Z

src/Simplex/FileTransfer/Server/Store/Postgres.hs

+      M.empty
+      (\acc (rId, sId, rKeyBs :: ByteString) ->
+        case C.decodePubKey rKeyBs of
+          Right rKey -> pure $! M.insertWith (++) sId [FileRecipient rId rKey] acc


I also don't think we need to construct it in memory - do we do it for SMP? I thought we just stream lines to a file. Maybe that's why it works, but it's unnecessary wasteful.

SMP streams directly via foldQueueRecs (Main.hs:735), writing each QueueRec straight to the store log with no in-memory map. It can do this because each row is self-contained.

XFTP can't use that exact pattern because the log groups recipients per file (FADD sId [rcp,...]) and files/recipients are in two tables — so the current code preloads rcpMap to avoid N+1 lookups. Correct, but yes, wasteful — it materializes every recipient in memory.

Two streaming alternatives:

Two folds, relaxed log order: fold files → write FNEW+FPUT; then fold recipients ORDER BY sender_id, buffering the current sender's list and flushing FADD on sender change. Replay works because all FNEWs are processed before any FADD.

Single joined stream: SELECT f.*, r.recipient_id, r.recipient_key FROM files f LEFT JOIN recipients r USING (sender_id) ORDER BY f.sender_id, buffering per sender and flushing FNEW+FADD+FPUT on sender boundary. One query, constant memory, identical log layout to today.

epoberezkin · 2026-04-11T08:34:11Z

src/Simplex/FileTransfer/Server/Env.hs

+data XFTPStoreConfig s where
+  XSCMemory :: Maybe FilePath -> XFTPStoreConfig STMFileStore
+#if defined(dbServerPostgres)
+  XSCDatabase :: PostgresFileStoreCfg -> XFTPStoreConfig PostgresFileStore


what would happen if server with DB in INI started with exe compiled without postgres? will it correctly exit with error?

Env.hs:183 handles it in the #else branch with error "Error: server binary is compiled without support for PostgreSQL database...". It exits non-zero with a clear message, so the check works.

epoberezkin · 2026-04-11T08:34:48Z

src/Simplex/FileTransfer/Server/Env.hs

+#endif
+
+-- | Import StoreLog to PostgreSQL database.
+importToDatabase :: FilePath -> Ini -> MigrationConfirmation -> IO ()


this belongs in Main.hs I think

The current placement is deliberate - comment at Env.hs:165 says "CPP guards for Postgres are handled here so Main.hs stays CPP-free." Main.hs currently has zero CPP.

Moving importToDatabase/exportFromDatabase to Main.hs would drag #if defined(dbServerPostgres) ... #else ... #endif blocks plus the Postgres.Config/importFileStore imports into Main.hs, same as runWithStoreConfig and checkFileStoreMode.

epoberezkin · 2026-04-11T08:34:55Z

src/Simplex/FileTransfer/Server/Env.hs

+#endif
+
+-- | Export PostgreSQL database to StoreLog.
+exportFromDatabase :: FilePath -> Ini -> MigrationConfirmation -> IO ()


Same reason. exportFromDatabase sits alongside importToDatabase under the same CPP guard in Env.hs for the same reason: concentrate all #if defined(dbServerPostgres) in one file so Main.hs stays CPP-free. Same tradeoff applies.

epoberezkin · 2026-04-11T08:36:36Z

src/Simplex/FileTransfer/Server/Store.hs

-        writeTVar recipientIds $! S.insert rId rIds
-        TM.insert rId (senderId, rKey) recipients
+  setFilePath st sId fPath = atomically $
+    withSTMFile st sId $ \FileRec {filePath, fileStatus} -> do


withFile -> withSTMFile is a spurious rename, it increases diff

Addressed in 3f81291

epoberezkin · 2026-04-11T08:37:16Z

src/Simplex/FileTransfer/Server/Store.hs

-  TM.delete rId recipients
-  modifyTVar' recipientIds $ S.delete rId
-
-getFile :: FileStore -> SFileParty p -> XFTPFileId -> STM (Either XFTPErrorType (FileRec, C.APublicAuthKey))


getFile moved, get it back here

Addressed in 1f8bd6b

epoberezkin · 2026-04-11T08:38:48Z

src/Simplex/FileTransfer/Server.hs


-runXFTPServer :: XFTPServerConfig -> IO ()
-runXFTPServer cfg = do
+runXFTPServer :: FileStoreClass s => XFTPStoreConfig s -> XFTPServerConfig -> IO ()


I think XFTPServerConfig should be parameterized, and XFTPStoreConfig s should be inside it, same as with SMP

Addressed in fcbb13e

epoberezkin · 2026-04-11T08:39:21Z

src/Simplex/FileTransfer/Server.hs

-    stopServer :: M ()
+    stopServer :: M s ()
    stopServer = do
      withFileLog closeStoreLog


this likely should be inside closeFileStore

Addressed in e530463

epoberezkin · 2026-04-11T08:39:46Z

src/Simplex/FileTransfer/Server.hs

    verifyCmd party = do
      st <- asks store
-      atomically $ verify =<< getFile st party fId
+      liftIO (getFile st party fId) >>= \case


unnecessary inlining, restore old structure

Addressed in fcbb13e

epoberezkin · 2026-04-11T08:41:51Z

tests/XFTPAgent.hs

 #endif
-    . describe "agent XFTP API" $ do
-      it "should send and receive file" $ withXFTPServer testXFTPAgentSendReceive
+    . describe "agent XFTP API (memory)" $ do


reduce diff, it's unnecessary

Addressed in b5055ad

epoberezkin · 2026-04-11T08:42:34Z

tests/XFTPServerTests.hs

 xftpServerTests :: Spec
 xftpServerTests =
  before_ (createDirectoryIfMissing False xftpServerFiles) . after_ (removeDirectoryRecursive xftpServerFiles) $ do
-    describe "XFTP file chunk delivery" $ do


get them back, too much diff

Addressed in b5055ad

epoberezkin · 2026-04-11T08:43:21Z

tests/XFTPServerTests.hs

 import XFTPClient

+-- Memory-only tests (store log persistence and SNI/CORS transport tests)
 xftpServerTests :: Spec


SpecWith XFTPTestBracket, and type name is incorrect. Bracket is code, it should be named with what we pass.

Addressed in b5055ad and 50c387d

Embed XFTPStoreConfig s as serverStoreCfg field, matching SMP's ServerConfig. runXFTPServer and newXFTPServerEnv now take a single XFTPServerConfig s. Restore verifyCmd local helper structure.

Restore xftpServerTests and xftpAgentTests bodies to match master byte-for-byte (only type signatures change for XFTPTestBracket parameterization); inline the runTestXXX helpers that were split on this branch.

Move STM store log close responsibility into closeFileStore to match PostgresFileStore, removing the asymmetry where only PG's close was self-contained. STMFileStore holds the log in a TVar populated by newXFTPServerEnv after readWriteFileStore; stopServer no longer needs the explicit withFileLog closeStoreLog call. Writes still go through XFTPEnv.storeLog via withFileLog (unchanged).

xftp: add PostgreSQL backend design spec

704cdac

shumvgolove requested review from epoberezkin and spaced4ndy as code owners March 31, 2026 12:59

shumvgolove added 24 commits April 1, 2026 07:53

update doc

7a76102

adjust styling

1bf3211

add implementation plan

2caf2e5

refactor: move usedStorage from FileStore to XFTPEnv

d703cfa

refactor: add getUsedStorage, getFileCount, expiredFiles store functions

8e449b8

refactor: change file store operations from STM to IO

b0da982

refactor: extract FileStoreClass typeclass, move STM impl to Store.STM

6f4bf64

refactor: make XFTPEnv and server polymorphic over FileStoreClass

ff254b4

feat: add PostgreSQL store skeleton with schema migration

cde9f50

feat: implement PostgresFileStore operations

ae4888f

feat: add PostgreSQL INI config, store dispatch, startup validation

d6b6cd5

feat: add database import/export CLI commands

aacd873

test: add PostgreSQL backend tests

dea62cc

fix: map ForeignKeyViolation to AUTH in addRecipient

d101a9b

When a file is concurrently deleted while addRecipient runs, the FK constraint on recipients.sender_id raises ForeignKeyViolation. Previously this propagated as INTERNAL; now it returns AUTH (file not found).

shumvgolove force-pushed the sh/xftp-pg branch 2 times, most recently from eace7ab to 00ed151 Compare April 7, 2026 13:03

epoberezkin reviewed Apr 11, 2026

View reviewed changes

feat: add manual tests and guide

aee5558

epoberezkin reviewed Apr 11, 2026

View reviewed changes

shumvgolove added 8 commits April 11, 2026 09:01

refactor: merge file_size CHECK into initial migration

6cac469

refactor: extract rowToFileRec shared by getFile sender/recipient paths

26bcc72

refactor: parameterize XFTPServerConfig over store type

fcbb13e

Embed XFTPStoreConfig s as serverStoreCfg field, matching SMP's ServerConfig. runXFTPServer and newXFTPServerEnv now take a single XFTPServerConfig s. Restore verifyCmd local helper structure.

refactor: minimize diff in tests

b5055ad

Restore xftpServerTests and xftpAgentTests bodies to match master byte-for-byte (only type signatures change for XFTPTestBracket parameterization); inline the runTestXXX helpers that were split on this branch.

refactor: restore getFile position to match master

1f8bd6b

refactor: rename withSTMFile back to withFile

3f81291

refactor: rename XFTPTestBracket to XFTPTestServer

50c387d

Uh oh!

Conversation

shumvgolove commented Mar 31, 2026

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!

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!

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!