Skip to content

fix: tighten micro service semantics - #70

Closed
matkomat wants to merge 5 commits into
mainfrom
fix/tighten-micro-service-semantics
Closed

matkomat wants to merge 5 commits into
mainfrom
fix/tighten-micro-service-semantics

Conversation

@matkomat

Copy link
Copy Markdown
Contributor

Summary

  • remove decorative micro config surface and tighten service lifecycle semantics
  • make stop() a graceful drain entrypoint and make reset() start a new stats window
  • expose runtime response/send failures through service-level state and tests

Breaking change

  • removed ServiceConfig::$endpoint because it did not correspond to real runtime behavior
  • added Service::lastError() / ServiceInterface::lastError() as the service-level runtime error signal
  • added RequestInterface::hasResponded() and made duplicate or impossible replies explicit failures
  • addEndpoint() and addGroup() now reject stopped services
  • stop() now drains owned subscriptions instead of unsubscribing them immediately when the connection is live

Details

  • reset() now clears endpoint counters, clears the current service error, and starts a new stats window timestamp
  • grouped endpoints now preserve pending limit settings when building derived endpoint configs
  • control-subject reply failures are recorded on the service instead of bubbling out silently

Validation

  • ./vendor/bin/phpunit tests/Integration/Micro --testdox
  • ./vendor/bin/phpunit tests/Unit/Micro --testdox
  • ./vendor/bin/phpstan analyse --no-progress
  • ./vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php --dry-run --diff src/Micro/Group.php src/Micro/Request.php src/Micro/RequestInterface.php src/Micro/Service.php src/Micro/ServiceConfig.php src/Micro/ServiceInterface.php tests/Integration/Micro/ServiceTest.php docs/examples/micro/01-service.php

Closes #42

Comment thread src/Micro/Service.php

@matkomat-agent-2 matkomat-agent-2 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I left one inline note on Service::stop(): the new drain path never completes local subscription cleanup, so a stopped service remains registered in Connection::$subscriptions and can be replayed on reconnect.

I verified the rest of the surface locally with:

  • vendor/bin/phpunit tests/Integration/Micro/ServiceTest.php
  • vendor/bin/phpunit tests/Integration/Micro
  • vendor/bin/phpstan analyse src --no-progress

I also reproduced the stop issue with a one-off script: after stop(), numSubscriptions() stayed unchanged instead of dropping the service subscriptions.

@matkomat-agent-2 matkomat-agent-2 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The latest commit addresses my previous Service::stop() cleanup concern. I reran the micro coverage on the updated head and did not find further issues.

Verified locally with:

  • vendor/bin/phpunit tests/Integration/Micro/ServiceTest.php
  • vendor/bin/phpunit tests/Integration/Micro/GroupQueueTest.php
  • vendor/bin/phpunit tests/Integration/Micro/ServiceExtendedTest.php
  • vendor/bin/phpunit tests/Integration/Micro/StatsHandlerTest.php
  • vendor/bin/phpunit tests/Integration/Micro
  • vendor/bin/phpstan analyse src --no-progress

I also reran the self-stop repro that previously left service subscriptions behind; after the fix the connection drops back to only the request mux subscription as expected.

Comment thread src/Micro/Service.php

@matkomat-agent matkomat-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Solid PR — the lifecycle tightening, drain-based stop, and runtime error visibility are all well-motivated. Two edge cases worth considering:

1. errorHandler can break cleanup flow

recordError() calls the user-provided errorHandler without a try/catch. If that handler throws, it can leave finishPendingStopCleanup() and stop() in an inconsistent state — e.g. the unsubscribe() fallback loop after a failed flush() never runs:

// finishPendingStopCleanup()
try {
    $this->connection->flush(0.5);
} catch (\Throwable $e) {
    $this->recordError($e); // if errorHandler throws here → subs never cleaned up
    foreach ($subs as $sub) {
        $sub->unsubscribe();
    }
    return;
}

Consider wrapping the errorHandler invocation in recordError():

private function recordError(\Throwable $e): void
{
    $this->lastError = $e;

    if ($this->config->errorHandler !== null) {
        try {
            ($this->config->errorHandler)($this, $e);
        } catch (\Throwable) {
            // User handler must not break service internals
        }
    }
}

2. doneHandler fires before deferred cleanup completes

When stop() is called inside a handler (handlerDepth > 0), doneHandler is invoked immediately — but subscriptions aren't cleaned up until the handler unwinds via finishPendingStopCleanup(). This means from the doneHandler's perspective numSubscriptions() is still non-zero and the service isn't truly shut down yet. If that's intentional, a doc comment on the timing would help.

Everything else looks clean — hasResponded() guard, the subjectFor()/queueGroupFor() extractions, the Group pending-limits fix, and the test coverage are all good.

@matkomat-agent matkomat-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both concerns from my previous review are addressed in 0756d64f:

  1. recordError() now wraps errorHandler in try/catch — a throwing user callback can no longer break cleanup or reply flow. Covered by testThrowingErrorHandlerDoesNotBreakErrorFlow.

  2. doneHandler is now deferred until after cleanup — invokeDoneHandler() with a doneHandlerCalled guard ensures it fires exactly once, after subscriptions are finalized. testDoneHandlerRunsAfterDeferredStopCleanup verifies numSubscriptions() === 0 at doneHandler invocation time.

doneHandler is also wrapped in try/catch, and the invokeDoneHandler idempotency guard handles the multiple call-sites in stop() cleanly.

The rest (drain-based stop, hasResponded(), lastError(), Group pending-limits passthrough, test coverage) was already solid. LGTM.

@matkomat

Copy link
Copy Markdown
Contributor Author

I found 2 behavioral regressions here (ignoring the intentional API break):

  1. Request::hasResponded() does not cover replies sent through Request::msg()->respond().

    In Service::addEndpoint() the catch path now skips the fallback request->error(...) only when hasResponded() is true, but Request only flips that flag for Request::respond() / error(). If a handler calls $req->msg()->respond(...) and then throws, hasResponded() is still false, so the service publishes a second 500 response on the same reply subject. I reproduced this and received both the original payload and the generated error reply.

  2. $SRV.STATS is not fully hardened yet.

    handleStats() evaluates $this->stats()->toArray() before entering replyControl(), and stats() still executes the user statsHandler. If that callback throws, the exception escapes through the subscription callback and bubbles out of process() instead of being recorded via lastError() / errorHandler(). I reproduced this with a throwing statsHandler; a $SRV.STATS.<name> request raised RuntimeException: stats boom.

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.

Tighten remaining micro package behavior gaps in PHP

1 participant