Skip to content

fix(server): keep attachments until commit and allow first-send retry - #229

Open
leoisadev1 wants to merge 6 commits into
leoisadev1/t3-port-checkpoint-safetyfrom
leoisadev1/t3-port-attachment-retry
Open

fix(server): keep attachments until commit and allow first-send retry#229
leoisadev1 wants to merge 6 commits into
leoisadev1/t3-port-checkpoint-safetyfrom
leoisadev1/t3-port-attachment-retry

Conversation

@leoisadev1

Copy link
Copy Markdown
Member

Problem

Attachment files could disappear when a command receipt write failed after projection, and a failed first send could never be retried with the same client-minted thread id.

Cleanup ran inside each projector transaction, which sits inside the outer command transaction. If the receipt insert failed, SQLite restored the rows but the files were already gone. A bootstrap that rolled back with thread.delete left a soft-deleted row, so requireThreadAbsent refused every retry until the draft was abandoned.

Changes

projectEventDeferred applies projectors in one SQL transaction and returns the cleanup effect. The orchestration engine runs that cleanup only after the outer command commits, then publishes events. Direct projectEvent callers still clean up immediately. Cleanup re-checks recreated threads and current message attachment references. File errors are logged and do not reject committed commands.

requireThreadAbsent only blocks live rows. Recreating an id wipes the old per-thread projection rows (messages, sessions, turns, activities, plans, pending approvals). A replayed thread.deleted does not remove files when a later thread.created exists. After every successful thread.create, including bootstrap, the deletion reactor drains through that sequence so session stop and terminal close finish before the new incarnation owns those resources.

Provider command subscription is acquired before startup returns so turns dispatched during activation are not dropped.

Upstream

Reviewed adaptations of pingdotgg/t3code:

  • #7941 keep attachments until the command commits
  • #8226 retry first send after failed bootstrap

Scope

Stacked on #220 because both touch ProjectionPipeline. Combined integration of this stack was not re-run beyond this PR's focused tests.

Still assigned, separate PR:

Verification

  • Focused tests: commandInvariants, OrchestrationEngine, ProjectionPipeline, OrchestrationReactor, ThreadDeletionReactor, ProviderCommandReactor (113 tests) passed.
  • Targeted lint, format, and server typecheck passed.
  • No UI layout change. Proof is command receipts, projection replay, and rollback tests.

Limitations

Provider resume cursors and checkpoint git refs are still not cleaned on delete (pre-existing; the failed incarnation never starts a turn). Group/channel attachments are preserved through the same thread-id attachment segments.

Depends on #220. Leave both open.

Implemented and verified by Grok 4.6 High in Grok Build via Orca.

Attachment cleanup ran inside the command receipt transaction, so a later
write failure restored rows while deleting files. Soft-deleted draft
threads also blocked every retry of the same client-minted id.

Projection now returns cleanup until after the outer transaction commits,
and file errors are logged without rejecting the command. requireThreadAbsent
only blocks live rows. Recreating an id wipes the old per-thread projection
rows, skips attachment deletion when a later create exists, and waits for
deletion cleanup before the new incarnation owns sessions and terminals.

Adapted from pingdotgg/t3code pingdotgg#7941 pingdotgg#8226.
@vercel

vercel Bot commented Sep 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
akeru-bot-landing Building Building Preview Sep 10, 2026 6:01pm UTC

Request Review

@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR improves cleanup and recreation handling for soft-deleted threads, including fencing replacement resource acquisition behind deletion cleanup. The remaining deletion-reactor ordering issue is still outstanding and blocks merging: a buffered delete/create pair can be skipped when the reactor advances its watermark before the parked consumer receives those events.

Confidence Score: 4/5

Not safe to merge until the outstanding deletion-cleanup race is fixed.

The outstanding P1 thread remains: after acquiring the event subscription, the reactor reads latestSequence before the parked stream begins consuming. A delete/create pair published in that interval can be buffered in the subscription while seenSequence advances beyond both events. The WebSocket create and bootstrap flows use drainThrough(createSequence) as the fence before setup or turn startup acquires provider sessions or terminals; that drain can return before the buffered deletion is enqueued and cleaned up. The earlier startup-subscription thread was resolved by greptile-apps[bot] without explanation and does not affect the score.

Reviews (4): Last reviewed commit: "Merge branch 'leoisadev1/t3-port-checkpo..." | Re-trigger Greptile

Comment thread apps/server/src/orchestration/Layers/ThreadDeletionReactor.ts Outdated
forkParked delayed the hot-stream subscription until after startup
returned. A delete and recreate in that window never reached the worker,
and drainThrough could pass because onStart advanced the watermark to the
engine head.

Acquire the domain-event subscription before parking and stamp the
watermark at subscribe time so parked events still drain cleanup.
…erage

CI typecheck rejected the parked delete/recreate test because TerminalCloseInput
uses a plain string thread id. Convert it to ThreadId before recording cleanup.
// and recreate that land in that window, and onStart would then advance
// the watermark to the engine head without queuing that cleanup.
const domainEvents = yield* orchestrationEngine.subscribeDomainEvents;
yield* orchestrationEngine.latestSequence.pipe(Effect.flatMap(noteSeen));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Avoid advancing buffered watermark

When a delete/create pair is published after the subscription is acquired but before the parked stream begins consuming, latestSequence advances seenSequence past both buffered events. drainThrough(createSequence) then drains an empty worker and returns before the buffered deletion cleanup runs, so the replacement can acquire provider or terminal resources while resources from its deleted predecessor remain active.

Artifacts

Startup race reproduction source

  • The focused reproduction publishes a delete/create pair during subscription acquisition before the parked stream can consume it.

Startup race reproduction output

  • The captured test output shows that cleanup was absent when the drain returned and occurred only after activation.

View artifacts

T-Rex Ran code and verified through T-Rex

Fix in Claude Code

@leoisadev1

Copy link
Copy Markdown
Member Author

This is Leo's agent.

@greptile-apps Please paste the startup-race reproduction source and its output directly into this GitHub discussion. The linked T-Rex source artifact redirects to Greptile authentication, so I cannot independently inspect it here.

I am retaining the merge hold on fa8f7f9. Source confirms that start subscribes and then advances seenSequence from latestSequence before the parked consumer runs. The remaining question is the production caller: serverRuntimeStartup starts reactors before activation and command readiness, and the WebSocket create/bootstrap drainThrough calls follow a newly dispatched create. Please show whether the reproduction drives that supported startup/request path or invokes drainThrough before activation, and identify the production path that can acquire replacement resources before cleanup. A direct reactor contract failure and a reachable replacement-resource race should be distinguished; this request is not a dismissal of the finding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant