Skip to content

Handle 404 Messages when processing#120

Merged
nekufa merged 3 commits into
basis-company:mainfrom
chazzbg:handle-404
Feb 4, 2026
Merged

Handle 404 Messages when processing#120
nekufa merged 3 commits into
basis-company:mainfrom
chazzbg:handle-404

Conversation

@chazzbg

@chazzbg chazzbg commented Feb 2, 2026

Copy link
Copy Markdown
Contributor

This one is a bit tricky.
There is a bug in the fetchAll logic when using fetch no wait ( consumer timeout 0 )
If we are waiting for a batch of 50 messages, the fetch logic has two options - either wait for a timeout or wait for the all 50 messages.
BUT when we publish a fetch no wait command ( PUB $JS.API.CONSUMER.MSG.NEXT.test.tester handler.0bf1604f 27 {"batch":50,"no_wait":true} ) to NATS it responds with all available messages and an empty message with 404 status as a termination. If the collected messages and more than 1 but less than the expected batch, the code will wait for the whole timeout or for all of the 50 expected messages.

Basically the fix makes the message handling to break the waiting it the moment it receives a message with 404 status and proceed to return whatever was fetched from the server until the 404 status.

This greatly improves situations where there a lot of clients fetching from NATS and the server itself is struggling to provide the whole expected batches from the clients

I'm struggling a bit to introduce this into the test suite and i would love some help there so we can verify the behavior and validate it with tests, until i figure it out on my own.

Tests is added handling both processing of a 404 message and performance testing ensuring timeout is not waited

@nekufa

nekufa commented Feb 4, 2026

Copy link
Copy Markdown
Member

hi @chazzbg
thanks for the contribution!

@nekufa nekufa merged commit e32c578 into basis-company:main Feb 4, 2026
12 checks passed
@ro0NL

ro0NL commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

this broke our tests https://github.com/etrias-nl/php-toolkit/actions/runs/21662573253/job/62450396026, they keep hanging

ro0NL added a commit to etrias-nl/php-toolkit that referenced this pull request Feb 4, 2026
@ro0NL

ro0NL commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

This one is a bit tricky.

@nekufa @chazzbg i figured out at some we assert the queue is empty (we fetch 1 by 1)

self::assertSame([], $transport->get());

This one keeps hanging now.

@chazzbg

chazzbg commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

@ro0NL personally i experiencesd similar behavior in the perf tests for the 500k iterations when running locally. They just hanged there for over an hour. Never investigated what have happened.

@ro0NL

ro0NL commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

To me it's just a bc break now. We have pinned the version. And we'll figure it out later.

@chazzbg

chazzbg commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

I'm trying to se what could have cause this and fix it eventualy

@nekufa

nekufa commented Feb 4, 2026

Copy link
Copy Markdown
Member

looks like we have incomplete edge cases test package.. :(

@ro0NL

ro0NL commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

im not really sure it's an edge case, we rely on

if (null === $this->queue) {
    $this->queue = $this->getConsumer()->getQueue();
    $this->client->setTimeout(self::SOCKET_TIMEOUT);
}

$message = $this->queue->next();
$payload = $message->payload;

and the final next() - when empty - seems to be the hanging one.

@chazzbg

chazzbg commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

i think i got a hold of it
#121

@nekufa

nekufa commented Feb 5, 2026

Copy link
Copy Markdown
Member

im not really sure it's an edge case

i mean that this is not covered by tests. @ro0NL feel free to contribute any tests that cover your workflow - that would help improve release stability

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.

3 participants