Skip to content

fix(cold): track stream producer tasks for graceful shutdown#40

Merged
prestwich merged 2 commits intomainfrom
fix/cold-stream-task-tracking
Mar 20, 2026
Merged

fix(cold): track stream producer tasks for graceful shutdown#40
prestwich merged 2 commits intomainfrom
fix/cold-stream-task-tracking

Conversation

@prestwich
Copy link
Member

Summary

  • Stream producer tasks were spawned via bare tokio::spawn, escaping the task runner's TaskTracker — not awaited during shutdown, not visible to the read/write barrier
  • Add stream_tracker: TaskTracker to ColdStorageTaskInner for long-lived stream producers
  • Streams are tracked for graceful shutdown but intentionally NOT drained before writes (backends provide their own read isolation via MDBX snapshots / PG REPEATABLE READ / anchor-hash detection)
  • Strengthen ColdStorage::produce_log_stream docs to make the concurrency contract explicit

Closes ENG-1988

Test plan

  • cargo clippy -p signet-cold --all-features --all-targets
  • cargo clippy -p signet-cold --no-default-features --all-targets
  • cargo +nightly fmt
  • cargo t -p signet-cold — conformance suite passes

🤖 Generated with Claude Code

prestwich and others added 2 commits March 11, 2026 09:39
Stream producers were spawned via bare `tokio::spawn`, escaping the
task runner's `TaskTracker`. This meant they were not awaited during
graceful shutdown and ran outside the read/write serialization barrier.

Add a separate `stream_tracker` on `ColdStorageTaskInner` for
long-lived stream producers. Streams are tracked for shutdown but not
drained before writes — backends provide their own read isolation
(MDBX snapshots, PostgreSQL REPEATABLE READ, anchor-hash detection).

Strengthen `ColdStorage::produce_log_stream` docs to make the
concurrency contract explicit.

Closes ENG-1988

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@prestwich prestwich requested a review from Fraser999 March 17, 2026 12:09
Comment on lines +390 to +391
// Graceful shutdown: drain reads first (short-lived), then streams
// (bounded by deadline). Reads must drain first because StreamLogs
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we apply a tokio::time::timeout to the stream tracker shutdown here rather than relying on the backend impls doing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

we already rely on backends to provide read isolation. we could add some safety here, but i'm not sure it's necessary? do you have a strong opinion we should?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, not a strong opinion. It just seems like a lightweight failsafe we could add, but ultimately this will more than likely be run in an environment where it will be forcefully killed if it doesn't stop within a reasonable time, so not a big deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

let's keep as is for now. can always revisit. any other concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope. Sorry - I thought I already approved!

@prestwich prestwich merged commit 19695d5 into main Mar 20, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants