feat(attachments): persist original attachments in Hub - #1684
feat(attachments): persist original attachments in Hub#1684techotaku39 wants to merge 4 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major] Make attachment row/blob deletion crash-safe — the file is unlinked before the SQLite row is removed. A process exit or database error between those operations leaves a live attachment row whose original permanently returns 404; startup cleanup only finds rows whose session is missing. The inverse create window can also leave untracked
.originalfiles. Evidencehub/src/store/attachments.ts:172.Suggested fix:
const result = this.db.prepare( 'DELETE FROM attachments WHERE id = ? AND namespace = ? AND session_id = ?' ).run(id, namespace, sessionId) if (Number(result.changes) === 0) return false await rm(attachment.originalPath, { force: true }) return true
Add startup reconciliation that removes blob files absent from the table, so a crash after the row-first delete is recoverable.
-
[Major] Preserve attachment-bearing duplicate turns without appending a second message —
knownKeysdeliberately ignores attachments, but this branch bypasses dedup for every source message that has any attachment. Two HAPI rows for the same Codex transcript therefore merge into repeated user prompts, and each additional duplicate adds another copy. Evidencehub/src/web/routes/codexDesktop.ts:1332.Suggested fix:
const existing = knownMessages.get(comparableKey) if (existing) { const copied = await store.attachments.cloneMessageAttachments( namespace, source.sessionId, canonical.sessionId, message.content, clonedAttachments ) store.messages.updateMessageContent( existing.id, mergeUserAttachments(existing.content, copied) ) continue }
Keep one canonical row and merge/replace its attachment metadata instead of copying the whole turn.
Summary
- Review mode: initial
- Two major issues found: crash windows can leave durable attachments permanently unreadable or leaked, and Codex duplicate-session merge deterministically duplicates attachment-bearing user turns.
Testing
- Not run (automation; static review only).
HAPI Bot
| const attachment = this.getForSession(id, namespace, sessionId) | ||
| if (!attachment) return false | ||
|
|
||
| await rm(attachment.originalPath, { force: true }) |
There was a problem hiding this comment.
[MAJOR] Make row/blob deletion crash-safe
This unlinks the only copy before the SQLite row is removed. If the Hub exits here, or the subsequent DELETE fails, the row survives under a valid session but every read returns 404 permanently; cleanupOrphanedAttachments() will not reclaim it because the owning session still exists. Creation has the inverse crash window (renamed blob before row insert), which leaks an untracked file.
Suggested fix:
const result = this.db.prepare(
'DELETE FROM attachments WHERE id = ? AND namespace = ? AND session_id = ?'
).run(id, namespace, sessionId)
if (Number(result.changes) === 0) return false
await rm(attachment.originalPath, { force: true })
return trueThen reconcile the attachment directory against table paths at startup, deleting files with no row.
| // Attachment-bearing messages must be copied even when their text | ||
| // matches the canonical transcript. Otherwise deleting the source | ||
| // session can delete the only durable bytes for that message. | ||
| if (knownKeys.has(comparableKey) && !hasUserAttachments(message.content)) { |
There was a problem hiding this comment.
[MAJOR] Do not duplicate an already-known user turn just because it has attachments
comparableKey for user messages is based on text, so this condition appends a second copy of the same Codex turn whenever the source copy has attachments. With multiple duplicate HAPI sessions, the merged transcript gains one repeated prompt per source row. Preserve the bytes by merging cloned attachment metadata into the existing canonical message instead of copying the entire message.
Suggested fix:
const existing = knownMessages.get(comparableKey)
if (existing) {
const copied = await store.attachments.cloneMessageAttachments(
namespace, source.sessionId, canonical.sessionId, message.content, clonedAttachments
)
store.messages.updateMessageContent(
existing.id,
mergeUserAttachments(existing.content, copied)
)
continue
}
There was a problem hiding this comment.
Findings
-
[Major] Durable attachment images are decoded at full resolution on the main actor —
GeneratedImageLoaderis@MainActor, so the newTaskinherits that actor andUIImage(data:)decodes an original of up to 50 MB without the downsampling already used by the preview path. A large-dimension compressed image can therefore freeze or terminate the app when its message scrolls into view. Evidence:ios/Hapi/Features/Chat/Blocks/ChatBlockCard.swift:89.Suggested fix:
guard let cgImage = await Self.decodeAttachment(payload.data) else { return nil } return UIImage(cgImage: cgImage) private nonisolated static func decodeAttachment(_ bytes: Data) async -> CGImage? { AttachmentPreparer.decodeDownsampled( bytes, maxDimension: AttachmentPolicy.previewMaxDimension ) }
Summary
- Review mode: follow-up after new commits
- One Major issue found in the latest full diff. The two findings from the prior HAPI Bot review appear addressed by the new commits.
Testing
- Not run (automation; static review only). Add an iOS regression test using a small encoded file with very large pixel dimensions and verify the durable path returns a bounded-size image.
HAPI Bot
| ) else { | ||
| return nil | ||
| } | ||
| return UIImage(data: payload.data) |
There was a problem hiding this comment.
[MAJOR] Downsample durable originals off the main actor
GeneratedImageLoader is @MainActor, so this Task inherits the main actor. The endpoint permits originals up to 50 MB, and UIImage(data:) decodes the full pixel buffer here. A highly compressed, large-dimension image can therefore freeze or terminate the app merely when its message scrolls into view. The existing AttachmentPreviewView path avoids this with AttachmentPreparer.decodeDownsampled.
Suggested fix:
guard let cgImage = await Self.decodeAttachment(payload.data) else {
return nil
}
return UIImage(cgImage: cgImage)
private nonisolated static func decodeAttachment(_ bytes: Data) async -> CGImage? {
AttachmentPreparer.decodeDownsampled(
bytes,
maxDimension: AttachmentPolicy.previewMaxDimension
)
}
There was a problem hiding this comment.
Findings
- None.
Summary
- Review mode: follow-up after new commits
- No new issues found in the latest full diff. The prior Major finding at
ios/Hapi/Features/Chat/Blocks/ChatBlockCard.swift:90is addressed by routing durable originals through the existing bounded ImageIO downsampling path off the main actor. - Residual risk: the native iOS test suite was not run in this review environment; the main CI
testjob was still pending when reviewed.
Testing
- Not run (automation; static review only).
app-build,build,integration,package-tests, anddrift-gatewere passing; maintestwas pending.
HAPI Bot
1 participant
Summary
attachmentIdreferences for new uploads.pathandpreviewUrlmetadata.Problem / Motivation
Normal chat attachments previously depended on CLI-local temporary files and could persist large image Data URLs inside message metadata.
This made historical attachments fragile across session lifecycle operations and increased message payload size. The new implementation stores original bytes under Hub-managed storage and keeps only compact attachment metadata in new messages.
Implementation
AttachmentStorefor original attachment bytes.attachmentsdatabase table through the V25 → V26 migration.attachmentId.pathandpreviewUrlmessages readable.Scope
This PR intentionally does not include:
Content-addressed deduplication and full attachment lifecycle management remain PR2 work.
Validation
bun typecheckbun run test:web— 272 files, 2871 tests passedbun run test:shared— 298 tests passedbun run gen:fixturesbun run buildterminal-wrap-fidelity.spec.ts— 2/2 passedgit diff --checkswiftandxcodebuildare unavailable on Windows; these suites are covered by repository CI.Related Issues
Refs #1681
AI Disclosure
Implemented with assistance from OpenAI Codex using GPT-5.6. All generated changes were reviewed and tested by the contributor.