Skip to content

Adjust timeout to ignore pull activity#112

Open
adrum wants to merge 2 commits into
Mcrich23:mainfrom
adrum:fix/up-wait-pull-timeout
Open

Adjust timeout to ignore pull activity#112
adrum wants to merge 2 commits into
Mcrich23:mainfrom
adrum:fix/up-wait-pull-timeout

Conversation

@adrum

@adrum adrum commented Jun 20, 2026

Copy link
Copy Markdown

I noticed some bigger images on slower networks could exceed the 30s timeout. This modifies container-compose up -d to consider pull activity progress to determine if the command is dormant or not.

@Cyb3rDudu Cyb3rDudu self-assigned this Jun 23, 2026
@Cyb3rDudu Cyb3rDudu self-requested a review June 23, 2026 04:18
@Cyb3rDudu

Copy link
Copy Markdown
Collaborator

The idle-timeout approach is the right call, and the doc comment is ok. A few things before I'll merge:

  1. Dropping the deadline for while true also drops the old hard ceiling. In -d mode it's fine, but a container that keeps spitting logs every few seconds without ever reaching running would now hang up -d forever instead of failing at 30s. I'd keep an absolute backstop (a few minutes) on top of the idle timeout.
  2. When the idle timeout does fire, the container run task/process isn't cancelled or stopped - it just leaks in the background while up moves on to the next service. Cancel the task and stop the container in that error path.
  3. Mind adding a test for the new ActivityClock? It's private right now, so it'd need to be internal or extracted into its own file,but it's the one piece that's cleanly unit-testable, the rest of the wait logic needs the real container runtime. A small test covering touch() / lastActivity under concurrent access would cover the new behavior; if you inject the
    clock instead of calling Date() directly you can even test the idle-window logic without leaning on Task.sleep.

Address review feedback on the idle-timeout readiness wait:

- Add an absolute maxWait backstop (default 300s) on top of the idle
  timeout. Without it, a container that keeps emitting output every few
  seconds but never reaches 'running' would refresh the activity clock
  forever and hang 'up -d' indefinitely.
- On a failed wait, cancel the streaming run task and stop the container
  so the 'container run' subprocess doesn't leak in the background while
  'up' moves on to the next service.
- Extract ActivityClock into its own file with an injectable clock, and
  add unit tests covering touch()/lastActivity under concurrent access
  and the idle-window timing logic.
@adrum

adrum commented Jun 25, 2026

Copy link
Copy Markdown
Author

@Cyb3rDudu Great feedback! I have addressed all three. I set the ceiling to 5 minutes, as I felt 3 would be on the edge of too short.

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